[PATCH 0/3] usb: dwc2: fix USB peripheral role in dual-role mode on PX30

[PATCH 0/3] usb: dwc2: fix USB peripheral role in dual-role mode on PX30

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

On Theobroma Ringneck SoM, USB peripheral role when in dual-role mode does not
work and displays the following error message:
dwc2 ff300000.usb: dwc2_core_reset: HANG! Soft Reset timeout GRSTCTL_CSFTRST

The USB sniffer shows nothing and dumping the host registers is stuck on
HCDMA(0) register.

Note that for some reason it works "fine" on PX30-EVB (there's another issue but
not related to/fixed in this patch series).

Since there's no documentation available for this IP, this patch series is
basically just slightly adapted downstream BSP vendor kernel patches and I
cannot unfortunately give more information than what I have.

This patch series was tested on Theobroma Ringneck SoM on Haikou devkit and PX30
EVB. It fixes Ringneck support and does not break PX30-EVB's.

For reference, the content of those commits can be found in tag
linux-5.10-gen-rkr1, and the following commits have been used:
964d50060bf53a8defd1fc561b9261424f25ddad
ad81c375602819a538ad68d979906c05663046e2
6e6adab8f735bc4fe27a67bdc3144d8fa89250d4
7c3a4e60247fd7f7b04d95d15cb12c63a5c20408

Note that Rockchip kernel called a slightly different implementation of
__dwc2_lowlevel_hw_enable instead of dwc2_lowlevel_hw_enable but it seemed more
right to me to call dwc2_lowlevel_hw_enable as done for the forced peripheral
mode.

I'm assuming the patch disabling phy initialization might need to be made
Rockchip-specific?

To: Minas Harutyunyan <hminas@synopsys.com>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-usb@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: William Wu <william.wu@rock-chips.com>
Cc: Frank Wang <frank.wang@rock-chips.com>
Cc: Bin Yang <yangbin@rock-chips.com>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>

---
Bin Yang (1):
      usb: dwc2: prevent core phy initialisation

Quentin Schulz (2):
      usb: dwc2: disable lpm feature on Rockchip SoCs
      usb: dwc2: power on/off phy for peripheral mode in dual-role mode

 drivers/usb/dwc2/gadget.c   | 6 ++++--
 drivers/usb/dwc2/hcd.c      | 7 +++++++
 drivers/usb/dwc2/params.c   | 4 ++++
 drivers/usb/dwc2/platform.c | 3 ++-
 4 files changed, 17 insertions(+), 3 deletions(-)
---
base-commit: 76dcd734eca23168cb008912c0f69ff408905235
change-id: 20221206-dwc2-gadget-dual-role-aac67e6d42fd

Best regards,
-- 
Quentin Schulz <quentin.schulz@theobroma-systems.com>

[PATCH 1/3] usb: dwc2: disable lpm feature on Rockchip SoCs

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

LPM feature of DWC2 module integrated in Rockchip SoCs doesn't work
properly or needs some additional handling, so disable it for now.
Without disabling LPM feature, the USB ADB communication fail with
the following error log:

dwc2 ff580000.usb: new address 27
dwc2 ff580000.usb: Failed to exit L1 sleep state in 200us.
dwc2 ff580000.usb: dwc2_hsotg_send_reply: cannot queue req
dwc2 ff580000.usb: dwc2_hsotg_process_req_status: failed to send reply
dwc2 ff580000.usb: dwc2_hsotg_enqueue_setup: failed queue (-11)
dwc2 ff580000.usb: Failed to exit L1 sleep state in 200us.

Signed-off-by: William Wu <william.wu@rock-chips.com>
Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
[diff vs vendor kernel: added lpm_clock_gating, besl and
 hird_threshold_en settings as seen in  commit 53febc956900 ("usb: dwc2:
 disable Link Power Management on STM32MP15 HS OTG")]
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 drivers/usb/dwc2/params.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c
index 8eab5f38b1101..9ed9fd9569404 100644
--- a/drivers/usb/dwc2/params.c
+++ b/drivers/usb/dwc2/params.c
@@ -113,6 +113,10 @@ static void dwc2_set_rk_params(struct dwc2_hsotg *hsotg)
 	p->ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
 		GAHBCFG_HBSTLEN_SHIFT;
 	p->power_down = DWC2_POWER_DOWN_PARAM_NONE;
+	p->lpm = false;
+	p->lpm_clock_gating = false;
+	p->besl = false;
+	p->hird_threshold_en = false;
 }
 
 static void dwc2_set_ltq_params(struct dwc2_hsotg *hsotg)

-- 
b4 0.10.1

[PATCH 2/3] usb: dwc2: power on/off phy for peripheral mode in dual-role mode

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

The PHY power is handled for peripheral mode but only when the device is
forced into this peripheral mode. It is missing when the device is
operating in peripheral mode when dual-role mode is enabled, so let's
update the condition to match this scenario.

Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 drivers/usb/dwc2/gadget.c   | 6 ++++--
 drivers/usb/dwc2/platform.c | 3 ++-
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 8b15742d9e8aa..62fa6378d2d73 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -4549,7 +4549,8 @@ 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) {
+	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL ||
+	    (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg))) {
 		ret = dwc2_lowlevel_hw_enable(hsotg);
 		if (ret)
 			goto err;
@@ -4611,7 +4612,8 @@ 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)
+	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL ||
+	    (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg)))
 		dwc2_lowlevel_hw_disable(hsotg);
 
 	return 0;
diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index ec4ace0107f5f..1d4f1ba22b92a 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -576,7 +576,8 @@ 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)
+	if (hsotg->dr_mode == USB_DR_MODE_PERIPHERAL ||
+	    (hsotg->dr_mode == USB_DR_MODE_OTG && dwc2_is_device_mode(hsotg)))
 		dwc2_lowlevel_hw_disable(hsotg);
 
 #if IS_ENABLED(CONFIG_USB_DWC2_PERIPHERAL) || \

-- 
b4 0.10.1

[PATCH 3/3] usb: dwc2: prevent core phy initialisation

[Quentin Schulz]

From: Quentin Schulz <quentin.schulz@theobroma-systems.com>

From: Bin Yang <yangbin@rock-chips.com>

The usb phys need to be controlled dynamically on some Rockchip SoCs.
So set the new HCD flag which prevents USB core from trying to manage
our phys.

Signed-off-by: Bin Yang <yangbin@rock-chips.com>
Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
---
 drivers/usb/dwc2/hcd.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 657f1f659ffaf..757a66fa32fa8 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
 	if (!IS_ERR_OR_NULL(hsotg->uphy))
 		otg_set_host(hsotg->uphy->otg, &hcd->self);
 
+	/*
+	 * do not manage the PHY state in the HCD core, instead let the driver
+	 * handle this (for example if the PHY can only be turned on after a
+	 * specific event)
+	 */
+	hcd->skip_phy_initialization = 1;
+
 	/*
 	 * Finish generic HCD initialization and start the HCD. This function
 	 * allocates the DMA buffer pool, registers the USB bus, requests the

-- 
b4 0.10.1

Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation

[Greg Kroah-Hartman]

On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
> From: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> 
> From: Bin Yang <yangbin@rock-chips.com>

Can't have multiple "From:" lines, odd.  I'll try to fix this up on my
end...

Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation

[Greg Kroah-Hartman]

On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
> From: Bin Yang <yangbin@rock-chips.com>
> 
> The usb phys need to be controlled dynamically on some Rockchip SoCs.
> So set the new HCD flag which prevents USB core from trying to manage
> our phys.
> 
> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> ---
>  drivers/usb/dwc2/hcd.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> index 657f1f659ffaf..757a66fa32fa8 100644
> --- a/drivers/usb/dwc2/hcd.c
> +++ b/drivers/usb/dwc2/hcd.c
> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>  	if (!IS_ERR_OR_NULL(hsotg->uphy))
>  		otg_set_host(hsotg->uphy->otg, &hcd->self);
>  
> +	/*
> +	 * do not manage the PHY state in the HCD core, instead let the driver
> +	 * handle this (for example if the PHY can only be turned on after a
> +	 * specific event)
> +	 */
> +	hcd->skip_phy_initialization = 1;

Wait, doesn't this mess with the phy logic for all other chips that use
this IP block?  Have you tested this on other systems?

I'd like some verification first before taking this change as it seems
very specific-platform.

thanks,

greg k-h

Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation

[Quentin Schulz]

Hi Greg,

On 12/8/22 16:53, Greg Kroah-Hartman wrote:
> On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
>> From: Bin Yang <yangbin@rock-chips.com>
>>
>> The usb phys need to be controlled dynamically on some Rockchip SoCs.
>> So set the new HCD flag which prevents USB core from trying to manage
>> our phys.
>>
>> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>> ---
>>   drivers/usb/dwc2/hcd.c | 7 +++++++
>>   1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>> index 657f1f659ffaf..757a66fa32fa8 100644
>> --- a/drivers/usb/dwc2/hcd.c
>> +++ b/drivers/usb/dwc2/hcd.c
>> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>>   	if (!IS_ERR_OR_NULL(hsotg->uphy))
>>   		otg_set_host(hsotg->uphy->otg, &hcd->self);
>>   
>> +	/*
>> +	 * do not manage the PHY state in the HCD core, instead let the driver
>> +	 * handle this (for example if the PHY can only be turned on after a
>> +	 * specific event)
>> +	 */
>> +	hcd->skip_phy_initialization = 1;
> 
> Wait, doesn't this mess with the phy logic for all other chips that use
> this IP block?  Have you tested this on other systems?
> 

I have not. I asked this in the cover-letter but I guess I should have 
made the patch series an RFC for this reason?

> I'd like some verification first before taking this change as it seems
> very specific-platform.
> 

There's already some platform-specific callbacks for the driver (see
dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called 
too early, before hcd structure is actually allocated. So we either need 
to use some "proxy"/shadow variable in dwc2_core_params and then update 
it right after hcd gets allocated or have another platform-specific 
callback only for hcd (post-)initialization.

Nothing too fancy so shouldn't take too long to implement. Any 
preference? Something else?

Also on a side note, after further testing, USB peripheral mode in 
dual-role mode does not seem to be entirely fixed with this patch 
series, I still have occasional locks. But considering that it 
absolutely didn't work before, it is some kind of progress.
There are also some issues related to USB host mode in dual-role mode 
but I saw those happening before the patch series too. I'll see what I 
can do, really frustrating to work with IPs for which there's no 
documentation :/

Cheers,
Quentin

Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation

[Greg Kroah-Hartman]

On Fri, Dec 09, 2022 at 12:15:34PM +0100, Quentin Schulz wrote:
> Hi Greg,
> 
> On 12/8/22 16:53, Greg Kroah-Hartman wrote:
> > On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
> > > From: Bin Yang <yangbin@rock-chips.com>
> > > 
> > > The usb phys need to be controlled dynamically on some Rockchip SoCs.
> > > So set the new HCD flag which prevents USB core from trying to manage
> > > our phys.
> > > 
> > > Signed-off-by: Bin Yang <yangbin@rock-chips.com>
> > > Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
> > > Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
> > > ---
> > >   drivers/usb/dwc2/hcd.c | 7 +++++++
> > >   1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
> > > index 657f1f659ffaf..757a66fa32fa8 100644
> > > --- a/drivers/usb/dwc2/hcd.c
> > > +++ b/drivers/usb/dwc2/hcd.c
> > > @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
> > >   	if (!IS_ERR_OR_NULL(hsotg->uphy))
> > >   		otg_set_host(hsotg->uphy->otg, &hcd->self);
> > > +	/*
> > > +	 * do not manage the PHY state in the HCD core, instead let the driver
> > > +	 * handle this (for example if the PHY can only be turned on after a
> > > +	 * specific event)
> > > +	 */
> > > +	hcd->skip_phy_initialization = 1;
> > 
> > Wait, doesn't this mess with the phy logic for all other chips that use
> > this IP block?  Have you tested this on other systems?
> > 
> 
> I have not. I asked this in the cover-letter but I guess I should have made
> the patch series an RFC for this reason?

Ah, should I drop the first 2 in this series that I already applied?

> > I'd like some verification first before taking this change as it seems
> > very specific-platform.
> > 
> 
> There's already some platform-specific callbacks for the driver (see
> dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too
> early, before hcd structure is actually allocated. So we either need to use
> some "proxy"/shadow variable in dwc2_core_params and then update it right
> after hcd gets allocated or have another platform-specific callback only for
> hcd (post-)initialization.
> 
> Nothing too fancy so shouldn't take too long to implement. Any preference?
> Something else?

Which ever you think would be simplest.

thanks,

greg k-h

Re: [PATCH 3/3] usb: dwc2: prevent core phy initialisation

[Quentin Schulz]

Hi Greg,

On 12/9/22 13:13, Greg Kroah-Hartman wrote:
> On Fri, Dec 09, 2022 at 12:15:34PM +0100, Quentin Schulz wrote:
>> Hi Greg,
>>
>> On 12/8/22 16:53, Greg Kroah-Hartman wrote:
>>> On Wed, Dec 07, 2022 at 02:19:18PM +0100, Quentin Schulz wrote:
>>>> From: Bin Yang <yangbin@rock-chips.com>
>>>>
>>>> The usb phys need to be controlled dynamically on some Rockchip SoCs.
>>>> So set the new HCD flag which prevents USB core from trying to manage
>>>> our phys.
>>>>
>>>> Signed-off-by: Bin Yang <yangbin@rock-chips.com>
>>>> Signed-off-by: Frank Wang <frank.wang@rock-chips.com>
>>>> Signed-off-by: Quentin Schulz <quentin.schulz@theobroma-systems.com>
>>>> ---
>>>>    drivers/usb/dwc2/hcd.c | 7 +++++++
>>>>    1 file changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
>>>> index 657f1f659ffaf..757a66fa32fa8 100644
>>>> --- a/drivers/usb/dwc2/hcd.c
>>>> +++ b/drivers/usb/dwc2/hcd.c
>>>> @@ -5315,6 +5315,13 @@ int dwc2_hcd_init(struct dwc2_hsotg *hsotg)
>>>>    	if (!IS_ERR_OR_NULL(hsotg->uphy))
>>>>    		otg_set_host(hsotg->uphy->otg, &hcd->self);
>>>> +	/*
>>>> +	 * do not manage the PHY state in the HCD core, instead let the driver
>>>> +	 * handle this (for example if the PHY can only be turned on after a
>>>> +	 * specific event)
>>>> +	 */
>>>> +	hcd->skip_phy_initialization = 1;
>>>
>>> Wait, doesn't this mess with the phy logic for all other chips that use
>>> this IP block?  Have you tested this on other systems?
>>>
>>
>> I have not. I asked this in the cover-letter but I guess I should have made
>> the patch series an RFC for this reason?
> 
> Ah, should I drop the first 2 in this series that I already applied?
> 

Up to you. I need the three patches to have it (somewhat) working, so 
only merging the first two isn't going to improve the current situation 
much for me. I don't mind carrying them over for a v2 (or how many 
versions are needed).

>>> I'd like some verification first before taking this change as it seems
>>> very specific-platform.
>>>
>>
>> There's already some platform-specific callbacks for the driver (see
>> dwc2_set_rk_params in drivers/usb/dwc2/params.c) but this gets called too
>> early, before hcd structure is actually allocated. So we either need to use
>> some "proxy"/shadow variable in dwc2_core_params and then update it right
>> after hcd gets allocated or have another platform-specific callback only for
>> hcd (post-)initialization.
>>
>> Nothing too fancy so shouldn't take too long to implement. Any preference?
>> Something else?
> 
> Which ever you think would be simplest.
> 

Got it.

Cheers,
Quentin