Re: [RFC PATCH 02/14] usb: gadget: Add callback to set lane and transfer rate

[Thinh Nguyen <Thinh.Nguyen@synopsys.com>]

Hi,

Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes:
>> Introduce gadget opts udc_set_sublink_speed callback to set the lane
>> count and transfer rate (in lane speed mantissa of Gbps) for SuperSpeed
>> Plus capable gadgets. In the same way udc_set_speed, this function can
>> control the gadget's sublink attributes for SuperSpeed Plus.
>>
>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com>
>> ---
>>   drivers/usb/gadget/composite.c           |  2 ++
>>   drivers/usb/gadget/legacy/mass_storage.c |  2 ++
> I would rather not add new features to the legacy gadgets and focus on
> our configfs interface for anything new. Moreover, using the feature
> you introduced could, arguably, be done as a separate patch.

Sure. I'll revise this.

>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 3b4f67000315..a4de5a8c0f19 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -2353,6 +2353,8 @@ int usb_composite_probe(struct usb_composite_driver *driver)
>>   	gadget_driver->function =  (char *) driver->name;
>>   	gadget_driver->driver.name = driver->name;
>>   	gadget_driver->max_speed = driver->max_speed;
>> +	gadget_driver->max_lane_count = driver->max_lane_count;
>> +	gadget_driver->max_lsm = driver->max_lsm;
>>   
>>   	return usb_gadget_probe_driver(gadget_driver);
>>   }
>> diff --git a/drivers/usb/gadget/legacy/mass_storage.c b/drivers/usb/gadget/legacy/mass_storage.c
>> index f18f77584fc2..a0912c5afffc 100644
>> --- a/drivers/usb/gadget/legacy/mass_storage.c
>> +++ b/drivers/usb/gadget/legacy/mass_storage.c
>> @@ -223,6 +223,8 @@ static struct usb_composite_driver msg_driver = {
>>   	.name		= "g_mass_storage",
>>   	.dev		= &msg_device_desc,
>>   	.max_speed	= USB_SPEED_SUPER_PLUS,
>> +	.max_lane_count	= 2,
>> +	.max_lsm	= 10,
> Right, as mentioned, I'd prefer not touch the legacy gadgets. But in any
> case, why is it so that the gadget is telling you about max lane count
> and lsm? That should be abstracted away from the gadget driver. Gadget
> driver shouldn't have knowledge of number of lanes because, at the end
> of the day, that doesn't really change anything in practice. Unlike HS
> vs SS which changes a bunch of things.

Ok, that makes sense. I'll remove this.

>
>>   	.needs_serial	= 1,
>>   	.strings	= dev_strings,
>>   	.bind		= msg_bind,
>> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
>> index 51fa614b4079..a3b106a22a6e 100644
>> --- a/drivers/usb/gadget/udc/core.c
>> +++ b/drivers/usb/gadget/udc/core.c
>> @@ -1120,6 +1120,35 @@ static inline void usb_gadget_udc_set_speed(struct usb_udc *udc,
>>   	}
>>   }
>>   
>> +/**
>> + * usb_gadget_udc_set_sublink_attr - tells usb device controller the sublink
>> + *    attributes supported by the current driver
>> + * @udc: The device we want to set maximum speed
>> + * @lane_count: The maximum number of lanes to connect
>> + * @lsm: The maximum lane speed mantissa in Gbps to run
>> + *
>> + * In the same way as usb_gadget_udc_set_speed(), this function can set the
>> + * gadget's sublink attributes for SuperSpeed Plus.
>> + *
>> + * This call is issued by the UDC Class driver before calling
>> + * usb_gadget_udc_start() in order to make sure that we don't try to
>> + * connect on speeds the gadget driver doesn't support.
>> + */
>> +static inline void usb_gadget_udc_set_sublink_attr(struct usb_udc *udc,
>> +						   unsigned int lane_count,
>> +						   unsigned int lsm)
> do we envision a possibility of future USB spec releases adding more
> data here? How about introducing a struct usb_sublink_attr to be passed
> around? Could be used by both host and gadget stacks.

Good idea. That'd be much better. Thanks.

>
>> +{
>> +	if (udc->gadget->ops->udc_set_sublink_attr) {
>> +		unsigned int rate;
>> +		unsigned int lanes;
>> +
>> +		rate = min(lsm, udc->gadget->max_lsm);
>> +		lanes = min(lane_count, udc->gadget->max_lane_count);
> considering that lsm and lane_count are from 0 to their respective max
> values, do you need a min() here? Might be better to WARN() when either
> in over their max values.

Sure. That'd be better.

>
>> +		udc->gadget->ops->udc_set_sublink_attr(udc->gadget,
>> +						       lanes, rate);
> indentation using spaces? (same above, please fix)

It's both tab and spaces to align to next to the above open parentheses. 
It's based on checkpatch.

>
>> @@ -1353,7 +1382,14 @@ static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver *dri
>>   	udc->dev.driver = &driver->driver;
>>   	udc->gadget->dev.driver = &driver->driver;
>>   
>> -	usb_gadget_udc_set_speed(udc, driver->max_speed);
>> +	if (udc->gadget->ops->udc_set_sublink_attr &&
>> +	    udc->gadget->max_speed == USB_SPEED_SUPER_PLUS &&
>> +	    driver->max_lsm && driver->max_lane_count &&
>> +	    driver->max_speed == USB_SPEED_SUPER_PLUS)
> So if driver doesn't provide max_lsm and max_speed you don't set sublink
> attr? Won't this cause problems? Also, the sublink_attr is still,
> conceptually, setting the max speed for the bus, right? So you may want
> to call usb_gadget_udc_set_sublink_attr() from inside
> usb_gadget_udc_set_speed(), then we don't need to modify all the callers.
>

The idea was that if the driver doesn't provide max_lsm and max_speed, 
then it's not constrained by the number of lanes or lsm. It will 
fallback to the usb_gadget_udc_set_speed().

I didn't think about creating a new usb_sublink_attr structure, so I 
couldn't use usb_gadget_udc_set_sublink_attr() inside 
usb_gadget_udc_set_speed() before.

Thanks for the review!

Thinh