| Message ID | 20230315144433.3095859-1-fabrice.gasnier@foss.st.com |
|---|---|
| State | New |
| Headers | show |
| Series | usb: dwc2: fix a race, don't power off/on phy for dual-role mode | expand |
Hi Fabrice, On 3/15/23 15:44, Fabrice Gasnier wrote: > When in dual role mode (dr_mode == USB_DR_MODE_OTG), platform probe > successively basically calls: > - dwc2_gadget_init() > - dwc2_hcd_init() > - dwc2_lowlevel_hw_disable() since recent change [1] > - usb_add_gadget_udc() > > The PHYs (and so the clocks it may provide) shouldn't be disabled for all > SoCs, in OTG mode, as the HCD part has been initialized. > > On STM32 this creates some weird race condition upon boot, when: > - initially attached as a device, to a HOST > - and there is a gadget script invoked to setup the device part. > Below issue becomes systematic, as long as the gadget script isn't > started by userland: the hardware PHYs (and so the clocks provided by the > PHYs) remains disabled. > It ends up in having an endless interrupt storm, before the watchdog > resets the platform. > > [ 16.924163] dwc2 49000000.usb-otg: EPs: 9, dedicated fifos, 952 entries in SPRAM > [ 16.962704] dwc2 49000000.usb-otg: DWC OTG Controller > [ 16.966488] dwc2 49000000.usb-otg: new USB bus registered, assigned bus number 2 > [ 16.974051] dwc2 49000000.usb-otg: irq 77, io mem 0x49000000 > [ 17.032170] hub 2-0:1.0: USB hub found > [ 17.042299] hub 2-0:1.0: 1 port detected > [ 17.175408] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in Host mode > [ 17.181741] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in Host mode > [ 17.189303] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in Host mode > ... > > The host part is also not functional, until the gadget part is configured. > > The HW may only be disabled for peripheral mode (original init), e.g. > dr_mode == USB_DR_MODE_PERIPHERAL, until the gadget driver initializes. > > But when in USB_DR_MODE_OTG, the HW should remain enabled, as the HCD part > is able to run, while the gadget part isn't necessarily configured. > > I don't fully get the of purpose the original change, that claims disabling > the hardware is missing. It creates conditions on SOCs using the PHY > initialization to be completely non working in OTG mode. Original > change [1] should be reworked to be platform specific. > > [1] https://urldefense.com/v3/__https://lore.kernel.org/r/20221206-dwc2-gadget-dual-role-v1-2-36515e1092cd@theobroma-systems.com__;!!OOPJP91ZZw!gKDR7WKb_MJfqvHDitpGCxrXjMC0sSgfG7hMxQad2X2atPHRu9ePfzJhI8K1frODjhcFQYPQ7xsEs0GqPq6IDFUB4T1EJqsmRojKJrzOiA$ > Thanks for adding me to the recipients. So.. basically Dual Role mode doesn't work on Theobroma Ringneck (based on Rockchip PX30) module, at all, without the three patches listed in the patch series you linked to. Now, even with the whole patch series applied, I still get stability issues for dual role mode, but at least it **sometimes** works. I applied your patch and it seems I still have support for forced HOST and DEVICE mode on the OTG port. Considering dual role mode is not stable anyway and that this patch alone doesn't seem to make things better/worse, Reviewed-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> Tested-by: Quentin Schulz <quentin.schulz@theobroma-systems.com> I wish I had some access to some datasheet/technical manual or something but right now sinking time for Dual Role mode support is not in our top priority list so we'll have to live without it. However, any help is much welcomed. BTW, I assume you would want to backport this to stable releases too (v6.2). Cheers, Quentin > Fixes: ade23d7b7ec5 ("usb: dwc2: power on/off phy for peripheral mode in dual-role mode") > Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> > --- > drivers/usb/dwc2/gadget.c | 6 ++---- > drivers/usb/dwc2/platform.c | 3 +-- > 2 files changed, 3 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c > index 62fa6378d2d7..8b15742d9e8a 100644 > --- a/drivers/usb/dwc2/gadget.c > +++ b/drivers/usb/dwc2/gadget.c > @@ -4549,8 +4549,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, > hsotg->gadget.dev.of_node = hsotg->dev->of_node; > hsotg->gadget.speed = USB_SPEED_UNKNOWN; > > - if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || > - (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) { > + if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) { > ret = dwc2_lowlevel_hw_enable(hsotg); > if (ret) > goto err; > @@ -4612,8 +4611,7 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget) > if (!IS_ERR_OR_NULL(hsotg->uphy)) > otg_set_peripheral(hsotg->uphy->otg, NULL); > > - if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || > - (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) > + if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) > dwc2_lowlevel_hw_disable(hsotg); > > return 0; > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index 23ef75996823..262c13b6362a 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -576,8 +576,7 @@ static int dwc2_driver_probe(struct platform_device *dev) > dwc2_debugfs_init(hsotg); > > /* Gadget code manages lowlevel hw on its own */ > - if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || > - (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) > + if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) > dwc2_lowlevel_hw_disable(hsotg); > > #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
Hi Fabrice, >On 3/15/2023 6:45 PM, Fabrice Gasnier <fabrice.gasnier@foss.st.com> wrote: >From: Fabrice Gasnier <fabrice.gasnier@foss.st.com> >Sent: Wednesday, March 15, 2023 6:45 PM >To: Minas Harutyunyan <hminas@synopsys.com>; gregkh@linuxfoundation.org; >quentin.schulz@theobroma-systems.com >Cc: linux-usb@vger.kernel.org; linux-kernel@vger.kernel.org; linux-stm32@st- >md-mailman.stormreply.com; amelie.delaunay@foss.st.com; >alexandre.torgue@foss.st.com; fabrice.gasnier@foss.st.com >Subject: [PATCH] usb: dwc2: fix a race, don't power off/on phy for dual-role >mode > >When in dual role mode (dr_mode == USB_DR_MODE_OTG), platform probe >successively basically calls: >- dwc2_gadget_init() >- dwc2_hcd_init() >- dwc2_lowlevel_hw_disable() since recent change [1] >- usb_add_gadget_udc() > >The PHYs (and so the clocks it may provide) shouldn't be disabled for all >SoCs, in OTG mode, as the HCD part has been initialized. > >On STM32 this creates some weird race condition upon boot, when: >- initially attached as a device, to a HOST >- and there is a gadget script invoked to setup the device part. >Below issue becomes systematic, as long as the gadget script isn't started >by userland: the hardware PHYs (and so the clocks provided by the >PHYs) remains disabled. >It ends up in having an endless interrupt storm, before the watchdog resets >the platform. > >[ 16.924163] dwc2 49000000.usb-otg: EPs: 9, dedicated fifos, 952 entries >in SPRAM >[ 16.962704] dwc2 49000000.usb-otg: DWC OTG Controller >[ 16.966488] dwc2 49000000.usb-otg: new USB bus registered, assigned bus >number 2 >[ 16.974051] dwc2 49000000.usb-otg: irq 77, io mem 0x49000000 >[ 17.032170] hub 2-0:1.0: USB hub found >[ 17.042299] hub 2-0:1.0: 1 port detected >[ 17.175408] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in >Host mode >[ 17.181741] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in >Host mode >[ 17.189303] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in >Host mode >... > >The host part is also not functional, until the gadget part is configured. > >The HW may only be disabled for peripheral mode (original init), e.g. >dr_mode == USB_DR_MODE_PERIPHERAL, until the gadget driver initializes. > >But when in USB_DR_MODE_OTG, the HW should remain enabled, as the HCD part >is able to run, while the gadget part isn't necessarily configured. > >I don't fully get the of purpose the original change, that claims disabling >the hardware is missing. It creates conditions on SOCs using the PHY >initialization to be completely non working in OTG mode. Original change [1] >should be reworked to be platform specific. > >[1] https://urldefense.com/v3/__https://lore.kernel.org/r/20221206-dwc2- >gadget-dual-role-v1-2-36515e1092cd@theobroma- >systems.com__;!!A4F2R9G_pg!Y21e8pRbIOVyLQTRP5HjdeDUHpSjbtiRQVFGVCOBBDu9yH32W >tdppqmP-8TLyGrBjyOBG5iI4qw6XMFEdfRJDhZ8HVc$ > >Fixes: ade23d7b7ec5 ("usb: dwc2: power on/off phy for peripheral mode in >dual-role mode") >Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> Acked-by: Minas Harutyunyan <hminas@synopsys.com> >--- > drivers/usb/dwc2/gadget.c | 6 ++---- > drivers/usb/dwc2/platform.c | 3 +-- > 2 files changed, 3 insertions(+), 6 deletions(-) > >diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index >62fa6378d2d7..8b15742d9e8a 100644 >--- a/drivers/usb/dwc2/gadget.c >+++ b/drivers/usb/dwc2/gadget.c >@@ -4549,8 +4549,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget >*gadget, > hsotg->gadget.dev.of_node = hsotg->dev->of_node; > hsotg->gadget.speed = USB_SPEED_UNKNOWN; > >- if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || >- (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) >{ >+ if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) { > ret = dwc2_lowlevel_hw_enable(hsotg); > if (ret) > goto err; >@@ -4612,8 +4611,7 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget >*gadget) > if (!IS_ERR_OR_NULL(hsotg->uphy)) > otg_set_peripheral(hsotg->uphy->otg, NULL); > >- if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || >- (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) >+ if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) > dwc2_lowlevel_hw_disable(hsotg); > > return 0; >diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index >23ef75996823..262c13b6362a 100644 >--- a/drivers/usb/dwc2/platform.c >+++ b/drivers/usb/dwc2/platform.c >@@ -576,8 +576,7 @@ static int dwc2_driver_probe(struct platform_device >*dev) > dwc2_debugfs_init(hsotg); > > /* Gadget code manages lowlevel hw on its own */ >- if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || >- (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) >+ if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) > dwc2_lowlevel_hw_disable(hsotg); > > #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \ >-- >2.25.1
diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index 62fa6378d2d7..8b15742d9e8a 100644 --- a/drivers/usb/dwc2/gadget.c +++ b/drivers/usb/dwc2/gadget.c @@ -4549,8 +4549,7 @@ static int dwc2_hsotg_udc_start(struct usb_gadget *gadget, hsotg->gadget.dev.of_node = hsotg->dev->of_node; hsotg->gadget.speed = USB_SPEED_UNKNOWN; - if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || - (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) { + if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) { ret = dwc2_lowlevel_hw_enable(hsotg); if (ret) goto err; @@ -4612,8 +4611,7 @@ static int dwc2_hsotg_udc_stop(struct usb_gadget *gadget) if (!IS_ERR_OR_NULL(hsotg->uphy)) otg_set_peripheral(hsotg->uphy->otg, NULL); - if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || - (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) + if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) dwc2_lowlevel_hw_disable(hsotg); return 0; diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index 23ef75996823..262c13b6362a 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -576,8 +576,7 @@ static int dwc2_driver_probe(struct platform_device *dev) dwc2_debugfs_init(hsotg); /* Gadget code manages lowlevel hw on its own */ - if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL || - (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) + if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL) dwc2_lowlevel_hw_disable(hsotg); #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \
When in dual role mode (dr_mode == USB_DR_MODE_OTG), platform probe successively basically calls: - dwc2_gadget_init() - dwc2_hcd_init() - dwc2_lowlevel_hw_disable() since recent change [1] - usb_add_gadget_udc() The PHYs (and so the clocks it may provide) shouldn't be disabled for all SoCs, in OTG mode, as the HCD part has been initialized. On STM32 this creates some weird race condition upon boot, when: - initially attached as a device, to a HOST - and there is a gadget script invoked to setup the device part. Below issue becomes systematic, as long as the gadget script isn't started by userland: the hardware PHYs (and so the clocks provided by the PHYs) remains disabled. It ends up in having an endless interrupt storm, before the watchdog resets the platform. [ 16.924163] dwc2 49000000.usb-otg: EPs: 9, dedicated fifos, 952 entries in SPRAM [ 16.962704] dwc2 49000000.usb-otg: DWC OTG Controller [ 16.966488] dwc2 49000000.usb-otg: new USB bus registered, assigned bus number 2 [ 16.974051] dwc2 49000000.usb-otg: irq 77, io mem 0x49000000 [ 17.032170] hub 2-0:1.0: USB hub found [ 17.042299] hub 2-0:1.0: 1 port detected [ 17.175408] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in Host mode [ 17.181741] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in Host mode [ 17.189303] dwc2 49000000.usb-otg: Mode Mismatch Interrupt: currently in Host mode ... The host part is also not functional, until the gadget part is configured. The HW may only be disabled for peripheral mode (original init), e.g. dr_mode == USB_DR_MODE_PERIPHERAL, until the gadget driver initializes. But when in USB_DR_MODE_OTG, the HW should remain enabled, as the HCD part is able to run, while the gadget part isn't necessarily configured. I don't fully get the of purpose the original change, that claims disabling the hardware is missing. It creates conditions on SOCs using the PHY initialization to be completely non working in OTG mode. Original change [1] should be reworked to be platform specific. [1] https://lore.kernel.org/r/20221206-dwc2-gadget-dual-role-v1-2-36515e1092cd@theobroma-systems.com Fixes: ade23d7b7ec5 ("usb: dwc2: power on/off phy for peripheral mode in dual-role mode") Signed-off-by: Fabrice Gasnier <fabrice.gasnier@foss.st.com> --- drivers/usb/dwc2/gadget.c | 6 ++---- drivers/usb/dwc2/platform.c | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-)