| Message ID | 20240621021135.1600847-2-caleb.connolly@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc3: respect role-switch-default-mode | expand |
On 6/21/24 4:11 AM, Caleb Connolly wrote: > * Factor out the common pattern of checking the dr_mode property on > the usb node and it's parent > * Respect the usb-role-switch property, rather than requiring dr_mode be > set > * Override OTG mode with the value in role-switch-default-mode > > The devicetree bindings don't dictate that dr_mode is a required > property, but the dwc3 driver would refuse to probe if it was unset > (this breaks the Qualcomm RB2 board which doesn't set it). > > Here, we update the driver to respect the other properties that can be > used to describe the controller, and more gracefully handle some of the > edge cases. > > Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> > --- > Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> > Cc: Neil Armstrong <neil.armstrong@linaro.org> > --- > drivers/usb/dwc3/dwc3-generic.c | 56 +++++++++++++++++++++++++-------- > 1 file changed, 43 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c > index cbb5d61dfb0b..1cf8b90e8bbf 100644 > --- a/drivers/usb/dwc3/dwc3-generic.c > +++ b/drivers/usb/dwc3/dwc3-generic.c > @@ -49,8 +49,46 @@ struct dwc3_generic_host_priv { > struct dwc3_generic_priv gen_priv; > struct udevice *vbus_supply; > }; > > +/** > + * dwc3_get_preferred_dr_mode() - Get the preferred DR mode for the USB node > + * > + * Since we don't support dynamic role switching yet in dwc3, this is a slightly > + * opinionated wrapper around usb_get_dr_mode() which will respect the > + * role-switch-default-mode property if it is present and the dr_mode is OTG. > + * > + * @child: Node to get the DR mode for > + */ > +static enum usb_dr_mode dwc3_get_preferred_dr_mode(ofnode node) > +{ > + enum usb_dr_mode mode; > + bool is_role_switch = ofnode_has_property(node, "usb-role-switch"); The usb-role-switch DT property is not DWC3 specific, even in U-Boot it is used by e.g. STM32MP15xx which is DWC2. This should be added to core code. Also, it would be really good to sync the DWC3 driver with more up to date Linux state, but that's out of the scope of this patch obviously.
Hi Marek, On 24/06/2024 02:33, Marek Vasut wrote: > On 6/21/24 4:11 AM, Caleb Connolly wrote: >> * Factor out the common pattern of checking the dr_mode property on >> the usb node and it's parent >> * Respect the usb-role-switch property, rather than requiring dr_mode be >> set >> * Override OTG mode with the value in role-switch-default-mode >> >> The devicetree bindings don't dictate that dr_mode is a required >> property, but the dwc3 driver would refuse to probe if it was unset >> (this breaks the Qualcomm RB2 board which doesn't set it). >> >> Here, we update the driver to respect the other properties that can be >> used to describe the controller, and more gracefully handle some of the >> edge cases. >> >> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >> --- >> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> >> Cc: Neil Armstrong <neil.armstrong@linaro.org> >> --- >> drivers/usb/dwc3/dwc3-generic.c | 56 +++++++++++++++++++++++++-------- >> 1 file changed, 43 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/dwc3/dwc3-generic.c >> b/drivers/usb/dwc3/dwc3-generic.c >> index cbb5d61dfb0b..1cf8b90e8bbf 100644 >> --- a/drivers/usb/dwc3/dwc3-generic.c >> +++ b/drivers/usb/dwc3/dwc3-generic.c >> @@ -49,8 +49,46 @@ struct dwc3_generic_host_priv { >> struct dwc3_generic_priv gen_priv; >> struct udevice *vbus_supply; >> }; >> +/** >> + * dwc3_get_preferred_dr_mode() - Get the preferred DR mode for the >> USB node >> + * >> + * Since we don't support dynamic role switching yet in dwc3, this is >> a slightly >> + * opinionated wrapper around usb_get_dr_mode() which will respect the >> + * role-switch-default-mode property if it is present and the dr_mode >> is OTG. >> + * >> + * @child: Node to get the DR mode for >> + */ >> +static enum usb_dr_mode dwc3_get_preferred_dr_mode(ofnode node) >> +{ >> + enum usb_dr_mode mode; >> + bool is_role_switch = ofnode_has_property(node, "usb-role-switch"); > > The usb-role-switch DT property is not DWC3 specific, even in U-Boot it > is used by e.g. STM32MP15xx which is DWC2. This should be added to core > code. I couldn't see an obvious way to add this desired behaviour without risking breaking some existing boards. Do you mean keep this as a new function but just move it to core code? Or update the existing get_dr_mode() function? > > Also, it would be really good to sync the DWC3 driver with more up to > date Linux state, but that's out of the scope of this patch obviously. Yes, I'm definitely feeling this. Last time I tried updating dwc3 as you suggested (reverting all patches since the Linux import, updating, re-applying)... But there have just been too many changes on the Linux side for this to work anymore, and too many arch specific dwc3 glue drivers which call into the core that might break. I would love it if we could figure out some path forward here, even if that's just someone spending more than half a day on the porting effort :D
On 6/24/24 1:41 PM, Caleb Connolly wrote: > Hi Marek, > > On 24/06/2024 02:33, Marek Vasut wrote: >> On 6/21/24 4:11 AM, Caleb Connolly wrote: >>> * Factor out the common pattern of checking the dr_mode property on >>> the usb node and it's parent >>> * Respect the usb-role-switch property, rather than requiring dr_mode be >>> set >>> * Override OTG mode with the value in role-switch-default-mode >>> >>> The devicetree bindings don't dictate that dr_mode is a required >>> property, but the dwc3 driver would refuse to probe if it was unset >>> (this breaks the Qualcomm RB2 board which doesn't set it). >>> >>> Here, we update the driver to respect the other properties that can be >>> used to describe the controller, and more gracefully handle some of the >>> edge cases. >>> >>> Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> >>> --- >>> Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> >>> Cc: Neil Armstrong <neil.armstrong@linaro.org> >>> --- >>> drivers/usb/dwc3/dwc3-generic.c | 56 +++++++++++++++++++++++++-------- >>> 1 file changed, 43 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/usb/dwc3/dwc3-generic.c >>> b/drivers/usb/dwc3/dwc3-generic.c >>> index cbb5d61dfb0b..1cf8b90e8bbf 100644 >>> --- a/drivers/usb/dwc3/dwc3-generic.c >>> +++ b/drivers/usb/dwc3/dwc3-generic.c >>> @@ -49,8 +49,46 @@ struct dwc3_generic_host_priv { >>> struct dwc3_generic_priv gen_priv; >>> struct udevice *vbus_supply; >>> }; >>> +/** >>> + * dwc3_get_preferred_dr_mode() - Get the preferred DR mode for the >>> USB node >>> + * >>> + * Since we don't support dynamic role switching yet in dwc3, this >>> is a slightly >>> + * opinionated wrapper around usb_get_dr_mode() which will respect the >>> + * role-switch-default-mode property if it is present and the >>> dr_mode is OTG. >>> + * >>> + * @child: Node to get the DR mode for >>> + */ >>> +static enum usb_dr_mode dwc3_get_preferred_dr_mode(ofnode node) >>> +{ >>> + enum usb_dr_mode mode; >>> + bool is_role_switch = ofnode_has_property(node, "usb-role-switch"); >> >> The usb-role-switch DT property is not DWC3 specific, even in U-Boot >> it is used by e.g. STM32MP15xx which is DWC2. This should be added to >> core code. > > I couldn't see an obvious way to add this desired behaviour without > risking breaking some existing boards. Do you mean keep this as a new > function but just move it to core code? Yes, make this available to other drivers by placing it into core code. > Or update the existing > get_dr_mode() function? Better keep the get_dr_mode() as-is , I think the get_preferred_dr_mode() is a superset, right ? You could prepare a conversion series and switch other drivers over to it, people can then test and selectively revert if something breaks. >> Also, it would be really good to sync the DWC3 driver with more up to >> date Linux state, but that's out of the scope of this patch obviously. > > Yes, I'm definitely feeling this. Last time I tried updating dwc3 as you > suggested (reverting all patches since the Linux import, updating, > re-applying)... But there have just been too many changes on the Linux > side for this to work anymore, and too many arch specific dwc3 glue > drivers which call into the core that might break. I am hoping these arch/ drivers should have been cleaned up by now, what is there that is still open ? > I would love it if we could figure out some path forward here, even if > that's just someone spending more than half a day on the porting effort :D I'd love to find a way to update too.
diff --git a/drivers/usb/dwc3/dwc3-generic.c b/drivers/usb/dwc3/dwc3-generic.c index cbb5d61dfb0b..1cf8b90e8bbf 100644 --- a/drivers/usb/dwc3/dwc3-generic.c +++ b/drivers/usb/dwc3/dwc3-generic.c @@ -49,8 +49,46 @@ struct dwc3_generic_host_priv { struct dwc3_generic_priv gen_priv; struct udevice *vbus_supply; }; +/** + * dwc3_get_preferred_dr_mode() - Get the preferred DR mode for the USB node + * + * Since we don't support dynamic role switching yet in dwc3, this is a slightly + * opinionated wrapper around usb_get_dr_mode() which will respect the + * role-switch-default-mode property if it is present and the dr_mode is OTG. + * + * @child: Node to get the DR mode for + */ +static enum usb_dr_mode dwc3_get_preferred_dr_mode(ofnode node) +{ + enum usb_dr_mode mode; + bool is_role_switch = ofnode_has_property(node, "usb-role-switch"); + + mode = usb_get_dr_mode(node); + /* Attempt to search the parent node too */ + if (mode == USB_DR_MODE_UNKNOWN) { + node = ofnode_get_parent(node); + mode = usb_get_dr_mode(node); + is_role_switch |= ofnode_has_property(node, "usb-role-switch"); + } + + /* If the usb-role-switch property is present, but dr_mode isn't, then the + * default is OTG. + */ + if (is_role_switch && mode == USB_DR_MODE_UNKNOWN) + mode = USB_DR_MODE_OTG; + + /* Respect the role-switch-default-mode property */ + if (mode == USB_DR_MODE_OTG && is_role_switch) { + mode = usb_get_role_switch_default_mode(node); + if (mode == USB_DR_MODE_UNKNOWN) + mode = USB_DR_MODE_OTG; + } + + return mode; +} + static int dwc3_generic_probe(struct udevice *dev, struct dwc3_generic_priv *priv) { int rc; @@ -178,17 +216,12 @@ static int dwc3_generic_of_to_plat(struct udevice *dev) pr_info("No USB maximum speed specified. Using super speed\n"); plat->maximum_speed = USB_SPEED_SUPER; } - plat->dr_mode = usb_get_dr_mode(node); + plat->dr_mode = dwc3_get_preferred_dr_mode(node); if (plat->dr_mode == USB_DR_MODE_UNKNOWN) { - /* might be a leaf so check the parent for mode */ - node = dev_ofnode(dev->parent); - plat->dr_mode = usb_get_dr_mode(node); - if (plat->dr_mode == USB_DR_MODE_UNKNOWN) { - pr_err("Invalid usb mode setup\n"); - return -ENODEV; - } + pr_err("Invalid usb mode setup\n"); + return -ENODEV; } return 0; } @@ -541,12 +574,9 @@ static int dwc3_glue_bind_common(struct udevice *parent, ofnode node) int ret; debug("%s: subnode name: %s\n", __func__, name); - /* if the parent node doesn't have a mode check the leaf */ - dr_mode = usb_get_dr_mode(dev_ofnode(parent)); - if (!dr_mode) - dr_mode = usb_get_dr_mode(node); + dr_mode = dwc3_get_preferred_dr_mode(node); if (CONFIG_IS_ENABLED(DM_USB_GADGET) && (dr_mode == USB_DR_MODE_PERIPHERAL || dr_mode == USB_DR_MODE_OTG)) { debug("%s: dr_mode: OTG or Peripheral\n", __func__); @@ -698,9 +728,9 @@ int dwc3_glue_probe(struct udevice *dev) while (child) { enum usb_dr_mode dr_mode; - dr_mode = usb_get_dr_mode(dev_ofnode(child)); + dr_mode = dwc3_get_preferred_dr_mode(dev_ofnode(child)); device_find_next_child(&child); if (ops && ops->glue_configure) ops->glue_configure(dev, index, dr_mode); index++;
* Factor out the common pattern of checking the dr_mode property on the usb node and it's parent * Respect the usb-role-switch property, rather than requiring dr_mode be set * Override OTG mode with the value in role-switch-default-mode The devicetree bindings don't dictate that dr_mode is a required property, but the dwc3 driver would refuse to probe if it was unset (this breaks the Qualcomm RB2 board which doesn't set it). Here, we update the driver to respect the other properties that can be used to describe the controller, and more gracefully handle some of the edge cases. Signed-off-by: Caleb Connolly <caleb.connolly@linaro.org> --- Cc: Ilias Apalodimas <ilias.apalodimas@linaro.org> Cc: Neil Armstrong <neil.armstrong@linaro.org> --- drivers/usb/dwc3/dwc3-generic.c | 56 +++++++++++++++++++++++++-------- 1 file changed, 43 insertions(+), 13 deletions(-)