Hi,
Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen <Thinh.Nguyen@xxxxxxxxxxxx> 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@xxxxxxxxxxxx>
>> ---
>> 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
