| Message ID | d86ccd4f97469cfe67cbce549b37d4df7cd8cb27.1595631457.git.thinhn@synopsys.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb: Handle different sublink speeds | expand |
On Fri, 2020-07-24 at 16:39 -0700, Thinh Nguyen wrote: > Add a new common function to parse maximum_speed property string for > the specified number of lanes and transfer rate. > > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > Changes in v3: > - Add new function to parse "maximum-speed" for lanes and transfer rate > - Remove separate functions getting num_lanes and transfer rate properties > Changes in v2: > - New commit > > drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- > include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ > 2 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > index 1433260d99b4..53b4950c49e4 100644 > --- a/drivers/usb/common/common.c > +++ b/drivers/usb/common/common.c > @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) > } > EXPORT_SYMBOL_GPL(usb_speed_string); > > -enum usb_device_speed usb_get_maximum_speed(struct device *dev) > +void usb_get_maximum_speed_and_num_lanes(struct device *dev, > + enum usb_device_speed *speed, > + enum usb_phy_gen *gen, > + u8 *num_lanes) > { > const char *maximum_speed; > + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; > + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; > + u8 matched_num_lanes = 0; > int ret; > > ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); > if (ret < 0) > - return USB_SPEED_UNKNOWN; > + goto done; > > ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); > + if (ret >= 0) { > + matched_speed = ret; > + goto done; > + } > + > + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { > + matched_speed = USB_SPEED_SUPER_PLUS; > + matched_gen = USB_PHY_GEN_2; > + matched_num_lanes = 2; > + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { > + matched_speed = USB_SPEED_SUPER_PLUS; > + matched_gen = USB_PHY_GEN_2; > + matched_num_lanes = 1; > + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { > + matched_speed = USB_SPEED_SUPER_PLUS; > + matched_gen = USB_PHY_GEN_1; > + matched_num_lanes = 2; > + } > + > +done: > + if (speed) > + *speed = matched_speed; > + > + if (num_lanes) > + *num_lanes = matched_num_lanes; > + > + if (gen) > + *gen = matched_gen; > +} > +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); > + > +enum usb_device_speed usb_get_maximum_speed(struct device *dev) > +{ > + enum usb_device_speed speed; > > - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; > + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); > + return speed; > } > EXPORT_SYMBOL_GPL(usb_get_maximum_speed); > > diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h > index 01191649a0ad..46cfd72e7082 100644 > --- a/include/linux/usb/ch9.h > +++ b/include/linux/usb/ch9.h > @@ -57,6 +57,13 @@ enum usb_link_protocol { > USB_LP_SSP = 1, > }; > > +/* USB phy signaling rate gen */ > +enum usb_phy_gen { > + USB_PHY_GEN_UNKNOWN, > + USB_PHY_GEN_1, > + USB_PHY_GEN_2, > +}; The GEN_1, GEN_2 will describe the capability of not only PHY but also MAC, add _PHY_ seems a little ambiguous, I think usb_get_maximum_speed_and_num_lanes() is mainly used to get the capability of MAC. Another, not suitable to add property about PHY capablity in MAC nodes. > + > /** > * struct usb_sublink_speed - sublink speed attribute > * @id: sublink speed attribute ID (SSID) > @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type); > */ > extern const char *usb_speed_string(enum usb_device_speed speed); > > +/** > + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number > + * of lanes for a given USB controller. > + * @dev: Pointer to the given USB controller device > + * @speed: Where to output enum usb_device_speed > + * @gen: Where to output phy signaling rate gen > + * @num_lanes: Where to output number of requested lanes > + * > + * This function gets the maximum speed string from property "maximum-speed" > + * and output the appropriate speed of the device. If the maximum-speed string > + * is super-speed-plus-gen*, then it also outputs the number of lanes and phy > + * signaling rate 'Gen' value. > + */ > +extern void usb_get_maximum_speed_and_num_lanes(struct device *dev, > + enum usb_device_speed *speed, > + enum usb_phy_gen *gen, > + u8 *num_lanes); > + > /** > * usb_get_maximum_speed - Get maximum requested speed for a given USB > * controller.
Chunfeng Yun wrote: > On Fri, 2020-07-24 at 16:39 -0700, Thinh Nguyen wrote: >> Add a new common function to parse maximum_speed property string for >> the specified number of lanes and transfer rate. >> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> Changes in v3: >> - Add new function to parse "maximum-speed" for lanes and transfer rate >> - Remove separate functions getting num_lanes and transfer rate properties >> Changes in v2: >> - New commit >> >> drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- >> include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ >> 2 files changed, 69 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c >> index 1433260d99b4..53b4950c49e4 100644 >> --- a/drivers/usb/common/common.c >> +++ b/drivers/usb/common/common.c >> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) >> } >> EXPORT_SYMBOL_GPL(usb_speed_string); >> >> -enum usb_device_speed usb_get_maximum_speed(struct device *dev) >> +void usb_get_maximum_speed_and_num_lanes(struct device *dev, >> + enum usb_device_speed *speed, >> + enum usb_phy_gen *gen, >> + u8 *num_lanes) >> { >> const char *maximum_speed; >> + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; >> + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; >> + u8 matched_num_lanes = 0; >> int ret; >> >> ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); >> if (ret < 0) >> - return USB_SPEED_UNKNOWN; >> + goto done; >> >> ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); >> + if (ret >= 0) { >> + matched_speed = ret; >> + goto done; >> + } >> + >> + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_2; >> + matched_num_lanes = 2; >> + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_2; >> + matched_num_lanes = 1; >> + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_1; >> + matched_num_lanes = 2; >> + } >> + >> +done: >> + if (speed) >> + *speed = matched_speed; >> + >> + if (num_lanes) >> + *num_lanes = matched_num_lanes; >> + >> + if (gen) >> + *gen = matched_gen; >> +} >> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); >> + >> +enum usb_device_speed usb_get_maximum_speed(struct device *dev) >> +{ >> + enum usb_device_speed speed; >> >> - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; >> + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); >> + return speed; >> } >> EXPORT_SYMBOL_GPL(usb_get_maximum_speed); >> >> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >> index 01191649a0ad..46cfd72e7082 100644 >> --- a/include/linux/usb/ch9.h >> +++ b/include/linux/usb/ch9.h >> @@ -57,6 +57,13 @@ enum usb_link_protocol { >> USB_LP_SSP = 1, >> }; >> >> +/* USB phy signaling rate gen */ >> +enum usb_phy_gen { >> + USB_PHY_GEN_UNKNOWN, >> + USB_PHY_GEN_1, >> + USB_PHY_GEN_2, >> +}; > The GEN_1, GEN_2 will describe the capability of not only PHY but also > MAC, add _PHY_ seems a little ambiguous, I think > usb_get_maximum_speed_and_num_lanes() is mainly used to get the > capability of MAC. Another, not suitable to add property about PHY > capablity in MAC nodes. > From USB 3.2 spec: "Gen 1 is an adjective used to refer to the Physical layer associated with a 5.0 Gbps signaling rate. The original USB SuperSpeed Phy and a Gen 1 Phy refer to the same Phy." "Gen 2 is an adjective used to refer to the Physical layer associated with a 10 Gbps signaling rate." If you have a better name, I'm open for suggestions. BR, Thinh
On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote: > Add a new common function to parse maximum_speed property string for > the specified number of lanes and transfer rate. > > Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > --- > Changes in v3: > - Add new function to parse "maximum-speed" for lanes and transfer rate > - Remove separate functions getting num_lanes and transfer rate properties No, why have you split this into a single function that for some reason "can not fail"? > Changes in v2: > - New commit > > drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- > include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ > 2 files changed, 69 insertions(+), 3 deletions(-) > > diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > index 1433260d99b4..53b4950c49e4 100644 > --- a/drivers/usb/common/common.c > +++ b/drivers/usb/common/common.c > @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) > } > EXPORT_SYMBOL_GPL(usb_speed_string); > > -enum usb_device_speed usb_get_maximum_speed(struct device *dev) > +void usb_get_maximum_speed_and_num_lanes(struct device *dev, > + enum usb_device_speed *speed, > + enum usb_phy_gen *gen, > + u8 *num_lanes) What is wrong with just having multiple different functions instead? > { > const char *maximum_speed; > + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; > + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; > + u8 matched_num_lanes = 0; > int ret; > > ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); > if (ret < 0) > - return USB_SPEED_UNKNOWN; > + goto done; > > ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); > + if (ret >= 0) { > + matched_speed = ret; > + goto done; > + } > + > + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { > + matched_speed = USB_SPEED_SUPER_PLUS; > + matched_gen = USB_PHY_GEN_2; > + matched_num_lanes = 2; > + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { > + matched_speed = USB_SPEED_SUPER_PLUS; > + matched_gen = USB_PHY_GEN_2; > + matched_num_lanes = 1; > + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { Where are all of these device properties documented? > + matched_speed = USB_SPEED_SUPER_PLUS; > + matched_gen = USB_PHY_GEN_1; > + matched_num_lanes = 2; > + } > + > +done: > + if (speed) > + *speed = matched_speed; > + > + if (num_lanes) > + *num_lanes = matched_num_lanes; > + > + if (gen) > + *gen = matched_gen; > +} > +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); > + > +enum usb_device_speed usb_get_maximum_speed(struct device *dev) > +{ > + enum usb_device_speed speed; > > - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; > + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); Here's an example of why this isn't a good function. It isn't self-describing. Why do you pass in 3 pointers yet the name only contains 2 things? usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is not a good thing, right? Again, 3 different functions please. > + return speed; > } > EXPORT_SYMBOL_GPL(usb_get_maximum_speed); > > diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h > index 01191649a0ad..46cfd72e7082 100644 > --- a/include/linux/usb/ch9.h > +++ b/include/linux/usb/ch9.h > @@ -57,6 +57,13 @@ enum usb_link_protocol { > USB_LP_SSP = 1, > }; > > +/* USB phy signaling rate gen */ > +enum usb_phy_gen { > + USB_PHY_GEN_UNKNOWN, > + USB_PHY_GEN_1, > + USB_PHY_GEN_2, > +}; > + > /** > * struct usb_sublink_speed - sublink speed attribute > * @id: sublink speed attribute ID (SSID) > @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type); > */ > extern const char *usb_speed_string(enum usb_device_speed speed); > > +/** > + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number > + * of lanes for a given USB controller. You forgot that it also determines the "gen". thanks, greg k-h
Greg Kroah-Hartman wrote: > On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote: >> Add a new common function to parse maximum_speed property string for >> the specified number of lanes and transfer rate. >> >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >> --- >> Changes in v3: >> - Add new function to parse "maximum-speed" for lanes and transfer rate >> - Remove separate functions getting num_lanes and transfer rate properties > No, why have you split this into a single function that for some reason > "can not fail"? This patch was updated to read from a single property "maximum-speed" to get the device speed, gen, and number of lanes. So we use a single function to parse this property. This came up from on Rob's comments: https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2 https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/T/#m87191333cb10a7f0caaf533bfa198724d33c2519 > >> Changes in v2: >> - New commit >> >> drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- >> include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ >> 2 files changed, 69 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c >> index 1433260d99b4..53b4950c49e4 100644 >> --- a/drivers/usb/common/common.c >> +++ b/drivers/usb/common/common.c >> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) >> } >> EXPORT_SYMBOL_GPL(usb_speed_string); >> >> -enum usb_device_speed usb_get_maximum_speed(struct device *dev) >> +void usb_get_maximum_speed_and_num_lanes(struct device *dev, >> + enum usb_device_speed *speed, >> + enum usb_phy_gen *gen, >> + u8 *num_lanes) > What is wrong with just having multiple different functions instead? See my comment above. > >> { >> const char *maximum_speed; >> + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; >> + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; >> + u8 matched_num_lanes = 0; >> int ret; >> >> ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); >> if (ret < 0) >> - return USB_SPEED_UNKNOWN; >> + goto done; >> >> ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); >> + if (ret >= 0) { >> + matched_speed = ret; >> + goto done; >> + } >> + >> + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_2; >> + matched_num_lanes = 2; >> + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_2; >> + matched_num_lanes = 1; >> + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { > Where are all of these device properties documented? It's coming from a single property "maximum-speed", documented in patch 6/12 "usb: devicetree: Include USB SSP Gen X x Y" https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@synopsys.com/T/#u > > > >> + matched_speed = USB_SPEED_SUPER_PLUS; >> + matched_gen = USB_PHY_GEN_1; >> + matched_num_lanes = 2; >> + } >> + >> +done: >> + if (speed) >> + *speed = matched_speed; >> + >> + if (num_lanes) >> + *num_lanes = matched_num_lanes; >> + >> + if (gen) >> + *gen = matched_gen; > > >> +} >> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); >> + >> +enum usb_device_speed usb_get_maximum_speed(struct device *dev) >> +{ >> + enum usb_device_speed speed; >> >> - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; >> + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); > Here's an example of why this isn't a good function. > > It isn't self-describing. Why do you pass in 3 pointers yet the name > only contains 2 things? Right... I can revise. > > usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is > not a good thing, right? > > Again, 3 different functions please. Should we have 3 separate properties to describe the device? If so, this was done in v2 of this series, but Rob has different ideas. Please advise. > >> + return speed; >> } >> EXPORT_SYMBOL_GPL(usb_get_maximum_speed); >> >> diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h >> index 01191649a0ad..46cfd72e7082 100644 >> --- a/include/linux/usb/ch9.h >> +++ b/include/linux/usb/ch9.h >> @@ -57,6 +57,13 @@ enum usb_link_protocol { >> USB_LP_SSP = 1, >> }; >> >> +/* USB phy signaling rate gen */ >> +enum usb_phy_gen { >> + USB_PHY_GEN_UNKNOWN, >> + USB_PHY_GEN_1, >> + USB_PHY_GEN_2, >> +}; >> + >> /** >> * struct usb_sublink_speed - sublink speed attribute >> * @id: sublink speed attribute ID (SSID) >> @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type); >> */ >> extern const char *usb_speed_string(enum usb_device_speed speed); >> >> +/** >> + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number >> + * of lanes for a given USB controller. > You forgot that it also determines the "gen". Ok. Will fix. Thanks! Thinh
On Sat, Jul 25, 2020 at 10:51:07AM +0000, Thinh Nguyen wrote: > Greg Kroah-Hartman wrote: > > On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote: > >> Add a new common function to parse maximum_speed property string for > >> the specified number of lanes and transfer rate. > >> > >> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> > >> --- > >> Changes in v3: > >> - Add new function to parse "maximum-speed" for lanes and transfer rate > >> - Remove separate functions getting num_lanes and transfer rate properties > > No, why have you split this into a single function that for some reason > > "can not fail"? > > This patch was updated to read from a single property "maximum-speed" to > get the device speed, gen, and number of lanes. So we use a single > function to parse this property. > > This came up from on Rob's comments: > https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/#mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2 > > https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/T/#m87191333cb10a7f0caaf533bfa198724d33c2519 > > > > > >> Changes in v2: > >> - New commit > >> > >> drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- > >> include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ > >> 2 files changed, 69 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c > >> index 1433260d99b4..53b4950c49e4 100644 > >> --- a/drivers/usb/common/common.c > >> +++ b/drivers/usb/common/common.c > >> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) > >> } > >> EXPORT_SYMBOL_GPL(usb_speed_string); > >> > >> -enum usb_device_speed usb_get_maximum_speed(struct device *dev) > >> +void usb_get_maximum_speed_and_num_lanes(struct device *dev, > >> + enum usb_device_speed *speed, > >> + enum usb_phy_gen *gen, > >> + u8 *num_lanes) > > What is wrong with just having multiple different functions instead? > > See my comment above. > > > > > >> { > >> const char *maximum_speed; > >> + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; > >> + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; > >> + u8 matched_num_lanes = 0; > >> int ret; > >> > >> ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); > >> if (ret < 0) > >> - return USB_SPEED_UNKNOWN; > >> + goto done; > >> > >> ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); > >> + if (ret >= 0) { > >> + matched_speed = ret; > >> + goto done; > >> + } > >> + > >> + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { > >> + matched_speed = USB_SPEED_SUPER_PLUS; > >> + matched_gen = USB_PHY_GEN_2; > >> + matched_num_lanes = 2; > >> + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { > >> + matched_speed = USB_SPEED_SUPER_PLUS; > >> + matched_gen = USB_PHY_GEN_2; > >> + matched_num_lanes = 1; > >> + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { > > Where are all of these device properties documented? > > It's coming from a single property "maximum-speed", documented in patch > 6/12 "usb: devicetree: Include USB SSP Gen X x Y" > > https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@synopsys.com/T/#u > > > > > > > > >> + matched_speed = USB_SPEED_SUPER_PLUS; > >> + matched_gen = USB_PHY_GEN_1; > >> + matched_num_lanes = 2; > >> + } > >> + > >> +done: > >> + if (speed) > >> + *speed = matched_speed; > >> + > >> + if (num_lanes) > >> + *num_lanes = matched_num_lanes; > >> + > >> + if (gen) > >> + *gen = matched_gen; > > > > > >> +} > >> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); > >> + > >> +enum usb_device_speed usb_get_maximum_speed(struct device *dev) > >> +{ > >> + enum usb_device_speed speed; > >> > >> - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; > >> + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); > > Here's an example of why this isn't a good function. > > > > It isn't self-describing. Why do you pass in 3 pointers yet the name > > only contains 2 things? > > Right... I can revise. > > > > > usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is > > not a good thing, right? > > > > Again, 3 different functions please. > > Should we have 3 separate properties to describe the device? If so, this > was done in v2 of this series, but Rob has different ideas. Please advise. I don't care about the properties, that should be independant of the function call made to determine this information. Don't let the data formats force you to write horrible code :) thanks, greg k-h
Greg Kroah-Hartman wrote: > On Sat, Jul 25, 2020 at 10:51:07AM +0000, Thinh Nguyen wrote: >> Greg Kroah-Hartman wrote: >>> On Fri, Jul 24, 2020 at 04:39:11PM -0700, Thinh Nguyen wrote: >>>> Add a new common function to parse maximum_speed property string for >>>> the specified number of lanes and transfer rate. >>>> >>>> Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> >>>> --- >>>> Changes in v3: >>>> - Add new function to parse "maximum-speed" for lanes and transfer rate >>>> - Remove separate functions getting num_lanes and transfer rate properties >>> No, why have you split this into a single function that for some reason >>> "can not fail"? >> This patch was updated to read from a single property "maximum-speed" to >> get the device speed, gen, and number of lanes. So we use a single >> function to parse this property. >> >> This came up from on Rob's comments: >> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/f68f395c-0821-9e34-f6cf-75fa60c9a35b@synopsys.com/T/*mac3a4d0b1c02866e3bdbd6809506fbbee8bf84c2__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI-lKdFEE$ >> >> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/0818e876-ea5b-9ebc-7427-8e26b627e6b4@synopsys.com/T/*m87191333cb10a7f0caaf533bfa198724d33c2519__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI0SFG1Fk$ >> >> >>>> Changes in v2: >>>> - New commit >>>> >>>> drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- >>>> include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ >>>> 2 files changed, 69 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c >>>> index 1433260d99b4..53b4950c49e4 100644 >>>> --- a/drivers/usb/common/common.c >>>> +++ b/drivers/usb/common/common.c >>>> @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) >>>> } >>>> EXPORT_SYMBOL_GPL(usb_speed_string); >>>> >>>> -enum usb_device_speed usb_get_maximum_speed(struct device *dev) >>>> +void usb_get_maximum_speed_and_num_lanes(struct device *dev, >>>> + enum usb_device_speed *speed, >>>> + enum usb_phy_gen *gen, >>>> + u8 *num_lanes) >>> What is wrong with just having multiple different functions instead? >> See my comment above. >> >> >>>> { >>>> const char *maximum_speed; >>>> + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; >>>> + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; >>>> + u8 matched_num_lanes = 0; >>>> int ret; >>>> >>>> ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); >>>> if (ret < 0) >>>> - return USB_SPEED_UNKNOWN; >>>> + goto done; >>>> >>>> ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); >>>> + if (ret >= 0) { >>>> + matched_speed = ret; >>>> + goto done; >>>> + } >>>> + >>>> + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { >>>> + matched_speed = USB_SPEED_SUPER_PLUS; >>>> + matched_gen = USB_PHY_GEN_2; >>>> + matched_num_lanes = 2; >>>> + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { >>>> + matched_speed = USB_SPEED_SUPER_PLUS; >>>> + matched_gen = USB_PHY_GEN_2; >>>> + matched_num_lanes = 1; >>>> + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { >>> Where are all of these device properties documented? >> It's coming from a single property "maximum-speed", documented in patch >> 6/12 "usb: devicetree: Include USB SSP Gen X x Y" >> >> https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/3bc4fb6a0d7c1a9ea4d5d9384ade26c9066c87d0.1595631457.git.thinhn@synopsys.com/T/*u__;Iw!!A4F2R9G_pg!JvGMuHeacSdTRkN-1SCUsebqfPskVCdtFj6xUkMuvtkwBcGK5N4v5nukhHZKI6d-EKTE$ >> >>> >>> >>>> + matched_speed = USB_SPEED_SUPER_PLUS; >>>> + matched_gen = USB_PHY_GEN_1; >>>> + matched_num_lanes = 2; >>>> + } >>>> + >>>> +done: >>>> + if (speed) >>>> + *speed = matched_speed; >>>> + >>>> + if (num_lanes) >>>> + *num_lanes = matched_num_lanes; >>>> + >>>> + if (gen) >>>> + *gen = matched_gen; >>> >>>> +} >>>> +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); >>>> + >>>> +enum usb_device_speed usb_get_maximum_speed(struct device *dev) >>>> +{ >>>> + enum usb_device_speed speed; >>>> >>>> - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; >>>> + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); >>> Here's an example of why this isn't a good function. >>> >>> It isn't self-describing. Why do you pass in 3 pointers yet the name >>> only contains 2 things? >> Right... I can revise. >> >>> usb_get_maximum_speed_and_num_lanes_and_generation() shows that this is >>> not a good thing, right? >>> >>> Again, 3 different functions please. >> Should we have 3 separate properties to describe the device? If so, this >> was done in v2 of this series, but Rob has different ideas. Please advise. > I don't care about the properties, that should be independant of the > function call made to determine this information. Don't let the data > formats force you to write horrible code :) Ok. Will revise. Thanks, Thinh
diff --git a/drivers/usb/common/common.c b/drivers/usb/common/common.c index 1433260d99b4..53b4950c49e4 100644 --- a/drivers/usb/common/common.c +++ b/drivers/usb/common/common.c @@ -77,18 +77,59 @@ const char *usb_speed_string(enum usb_device_speed speed) } EXPORT_SYMBOL_GPL(usb_speed_string); -enum usb_device_speed usb_get_maximum_speed(struct device *dev) +void usb_get_maximum_speed_and_num_lanes(struct device *dev, + enum usb_device_speed *speed, + enum usb_phy_gen *gen, + u8 *num_lanes) { const char *maximum_speed; + enum usb_device_speed matched_speed = USB_SPEED_UNKNOWN; + enum usb_phy_gen matched_gen = USB_PHY_GEN_UNKNOWN; + u8 matched_num_lanes = 0; int ret; ret = device_property_read_string(dev, "maximum-speed", &maximum_speed); if (ret < 0) - return USB_SPEED_UNKNOWN; + goto done; ret = match_string(speed_names, ARRAY_SIZE(speed_names), maximum_speed); + if (ret >= 0) { + matched_speed = ret; + goto done; + } + + if (strncmp("super-speed-plus-gen2x2", maximum_speed, 23) == 0) { + matched_speed = USB_SPEED_SUPER_PLUS; + matched_gen = USB_PHY_GEN_2; + matched_num_lanes = 2; + } else if (strncmp("super-speed-plus-gen2x1", maximum_speed, 23) == 0) { + matched_speed = USB_SPEED_SUPER_PLUS; + matched_gen = USB_PHY_GEN_2; + matched_num_lanes = 1; + } else if (strncmp("super-speed-plus-gen1x2", maximum_speed, 23) == 0) { + matched_speed = USB_SPEED_SUPER_PLUS; + matched_gen = USB_PHY_GEN_1; + matched_num_lanes = 2; + } + +done: + if (speed) + *speed = matched_speed; + + if (num_lanes) + *num_lanes = matched_num_lanes; + + if (gen) + *gen = matched_gen; +} +EXPORT_SYMBOL_GPL(usb_get_maximum_speed_and_num_lanes); + +enum usb_device_speed usb_get_maximum_speed(struct device *dev) +{ + enum usb_device_speed speed; - return (ret < 0) ? USB_SPEED_UNKNOWN : ret; + usb_get_maximum_speed_and_num_lanes(dev, &speed, NULL, NULL); + return speed; } EXPORT_SYMBOL_GPL(usb_get_maximum_speed); diff --git a/include/linux/usb/ch9.h b/include/linux/usb/ch9.h index 01191649a0ad..46cfd72e7082 100644 --- a/include/linux/usb/ch9.h +++ b/include/linux/usb/ch9.h @@ -57,6 +57,13 @@ enum usb_link_protocol { USB_LP_SSP = 1, }; +/* USB phy signaling rate gen */ +enum usb_phy_gen { + USB_PHY_GEN_UNKNOWN, + USB_PHY_GEN_1, + USB_PHY_GEN_2, +}; + /** * struct usb_sublink_speed - sublink speed attribute * @id: sublink speed attribute ID (SSID) @@ -95,6 +102,24 @@ extern const char *usb_ep_type_string(int ep_type); */ extern const char *usb_speed_string(enum usb_device_speed speed); +/** + * usb_get_maximum_speed_and_num_lanes - Get maximum requested speed and number + * of lanes for a given USB controller. + * @dev: Pointer to the given USB controller device + * @speed: Where to output enum usb_device_speed + * @gen: Where to output phy signaling rate gen + * @num_lanes: Where to output number of requested lanes + * + * This function gets the maximum speed string from property "maximum-speed" + * and output the appropriate speed of the device. If the maximum-speed string + * is super-speed-plus-gen*, then it also outputs the number of lanes and phy + * signaling rate 'Gen' value. + */ +extern void usb_get_maximum_speed_and_num_lanes(struct device *dev, + enum usb_device_speed *speed, + enum usb_phy_gen *gen, + u8 *num_lanes); + /** * usb_get_maximum_speed - Get maximum requested speed for a given USB * controller.
Add a new common function to parse maximum_speed property string for the specified number of lanes and transfer rate. Signed-off-by: Thinh Nguyen <thinhn@synopsys.com> --- Changes in v3: - Add new function to parse "maximum-speed" for lanes and transfer rate - Remove separate functions getting num_lanes and transfer rate properties Changes in v2: - New commit drivers/usb/common/common.c | 47 ++++++++++++++++++++++++++++++++++++++++++--- include/linux/usb/ch9.h | 25 ++++++++++++++++++++++++ 2 files changed, 69 insertions(+), 3 deletions(-)