| Message ID | 25394a433bb79ddf36d0ec52bc998b164e373bf4.1576118671.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: dwc3: Introduce DWC_usb32 | expand |
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. > 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. > .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. > +{ > + 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. > + udc->gadget->ops->udc_set_sublink_attr(udc->gadget, > + lanes, rate); indentation using spaces? (same above, please fix) > @@ -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.
On Thu, 12 Dec 2019, 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. > > > 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. I agree completely. Furthermore, it isn't at all clear where those two numbers came from. Why would g-mass-storage care that lane_count <= 2 and lsm <= 10? Alan Stern
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
Hi, Alan Stern wrote: > On Thu, 12 Dec 2019, 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. >> >>> 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. > I agree completely. Furthermore, it isn't at all clear where those two > numbers came from. Why would g-mass-storage care that lane_count <= 2 > and lsm <= 10? > > Alan Stern > Right, I'll remove this and update according to Felipe's suggestions. Thanks, Thinh
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, .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) +{ + 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); + udc->gadget->ops->udc_set_sublink_attr(udc->gadget, + lanes, rate); + } +} + /** * usb_udc_release - release the usb_udc struct * @dev: the dev member within usb_udc @@ -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) + usb_gadget_udc_set_sublink_attr(udc, driver->max_lane_count, + driver->max_lsm); + else + usb_gadget_udc_set_speed(udc, driver->max_speed); ret = driver->bind(udc->gadget, driver); if (ret) diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index cb7531a6f784..a8ee2480b408 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -319,6 +319,9 @@ struct usb_gadget_ops { struct usb_gadget_driver *); int (*udc_stop)(struct usb_gadget *); void (*udc_set_speed)(struct usb_gadget *, enum usb_device_speed); + void (*udc_set_sublink_attr)(struct usb_gadget *, + unsigned int lane_count, + unsigned int lsm); struct usb_ep *(*match_ep)(struct usb_gadget *, struct usb_endpoint_descriptor *, struct usb_ss_ep_comp_descriptor *);
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 ++ drivers/usb/gadget/udc/core.c | 38 +++++++++++++++++++++++++++++++- include/linux/usb/gadget.h | 3 +++ 4 files changed, 44 insertions(+), 1 deletion(-)