[U-Boot] [PATCH v2 05/13] drivers: usb: common: add support to get maximum speed from dt

[Marek Vasut]

On 06/14/2017 11:16 AM, Vignesh R wrote:
> 
> 
> On Tuesday 13 June 2017 07:31 PM, Marek Vasut wrote:
>> On 06/13/2017 02:10 PM, Vignesh R wrote:
>>> From: Mugunthan V N <mugunthanvnm at ti.com>
>>>
>>> Add support to get maximum speed from dt so that usb drivers
>>> makes use of it for DT parsing.
>>>
>>> Signed-off-by: Mugunthan V N <mugunthanvnm at ti.com>
>>> Signed-off-by: Vignesh R <vigneshr at ti.com>
>>> Reviewed-by: Simon Glass <sjg at chromium.org>
>>> ---
>>>  drivers/usb/common/common.c | 29 +++++++++++++++++++++++++++++
>>>  include/linux/usb/otg.h     |  9 +++++++++
>>>  scripts/Makefile.spl        |  1 +
>>>  3 files changed, 39 insertions(+)
>>>
>>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c
>>> index 35c2dc18d955..c11689dc36fa 100644
>>> --- a/drivers/usb/common/common.c
>>> +++ b/drivers/usb/common/common.c
>>> @@ -10,6 +10,7 @@
>>>  #include <common.h>
>>>  #include <libfdt.h>
>>>  #include <linux/usb/otg.h>
>>> +#include <linux/usb/ch9.h>
>>>  
>>>  DECLARE_GLOBAL_DATA_PTR;
>>>  
>>> @@ -38,3 +39,31 @@ enum usb_dr_mode usb_get_dr_mode(int node)
>>>  
>>>  	return USB_DR_MODE_UNKNOWN;
>>>  }
>>> +
>>> +static const char *const speed_names[] = {
>>> +	[USB_SPEED_UNKNOWN] = "UNKNOWN",
>>> +	[USB_SPEED_LOW] = "low-speed",
>>> +	[USB_SPEED_FULL] = "full-speed",
>>> +	[USB_SPEED_HIGH] = "high-speed",
>>> +	[USB_SPEED_WIRELESS] = "wireless",
>>> +	[USB_SPEED_SUPER] = "super-speed",
>>> +};
>>> +
>>> +enum usb_device_speed usb_get_maximum_speed(int node)
>>> +{
>>> +	const void *fdt = gd->fdt_blob;
>>> +	const char *max_speed;
>>> +	int i;
>>> +
>>> +	max_speed = fdt_getprop(fdt, node, "maximum-speed", NULL);
>>> +	if (!max_speed) {
>>> +		error("usb dr_mode not found\n");
>>
>> Really dr_mode ?
> 
> Sorry, will fix this.
> 
>>
>>> +		return USB_DR_MODE_UNKNOWN;
>>> +	}
>>> +
>>> +	for (i = 0; i < ARRAY_SIZE(speed_names); i++)
>>> +		if (!strcmp(max_speed, speed_names[i]))
>>> +			return i;
>>
>> Shouldn't this somehow take into account the controller capabilities if
>> the maximum-speed node is missing ?
> 
> Idea was, this is just a helper function and will return
> USB_SPEED_UNKNOWN when "maximum-speed" property is missing. The
> particular USB controller driver (like dwc3) can take decision of
> falling back to default capabilities.
> I will leave this as is, but will update patch 6/13 to fall back to
> default capability when USB_DR_MODE_UNKNOWN is returned.

I think the function should take into account the caps of the controller
or if it doesn't, it's a misnomer.

>>> +	return USB_SPEED_UNKNOWN;
>>> +}
>>> diff --git a/include/linux/usb/otg.h b/include/linux/usb/otg.h
>>> index 8f8ac6aeefe3..b61ef19b22f3 100644
>>> --- a/include/linux/usb/otg.h
>>> +++ b/include/linux/usb/otg.h
>>> @@ -26,4 +26,13 @@ enum usb_dr_mode {
>>>   */
>>>  enum usb_dr_mode usb_get_dr_mode(int node);
>>>  
>>> +/**
>>> + * usb_get_maximum_speed() - Get maximum speed for given device
>>> + * @node: Node offset to the given device
>>> + *
>>> + * The function gets phy interface string from property 'maximum-speed',
>>> + * and returns the correspondig enum usb_device_speed
>>> + */
>>> +enum usb_device_speed usb_get_maximum_speed(int node);
>>> +
>>>  #endif /* __LINUX_USB_OTG_H */
>>> diff --git a/scripts/Makefile.spl b/scripts/Makefile.spl
>>> index ac3c2c7f1342..bffea085b329 100644
>>> --- a/scripts/Makefile.spl
>>> +++ b/scripts/Makefile.spl
>>> @@ -78,6 +78,7 @@ endif
>>>  
>>>  libs-$(CONFIG_SPL_LIBDISK_SUPPORT) += disk/
>>>  libs-y += drivers/
>>> +libs-y += drivers/usb/common/
>>
>> This looks weird, drivers/ is already included so why this explicit path
>> addition ?
> 
> Makefile under drivers/ neither includes usb/ directory nor does usb/
> directory has a Makefile. Hence, above entry is needed.
> I guess, this is how it is for SPL build.
> 

That's something to fix then, not work around like this ...

-- 
Best regards,
Marek Vasut