[PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[Manivannan Sadhasivam]

Hi,

This series allows the dwc3-qcom driver to do runtime PM itself without
userspace intervention. Still, userspace is required to enable runtime PM
for dwc3 glue and xhci drivers as we cannot enable runtime PM for them.
But this series avoids one more additional step.

While enabling runtime PM, I noticed that the xhci driver suspends before
catching the xhci interrupts during resume. This ended up deferring the
device enumeration for some time. So I included a patch adding autosuspend
delay of 200ms to the xhci driver. With this delay, usb enumeration happens
properly.

This series has been tested on SC8280XP-CRD and RB5 devices.

Thanks,
Mani

Manivannan Sadhasivam (5):
  arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks
  xhci: host: Use 200ms autosuspend delay for runtime suspend
  usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
  usb: dwc3: qcom: Clear pending interrupt before enabling wake
    interrupt
  usb: dwc3: qcom: Allow runtime PM

 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
 drivers/usb/dwc3/dwc3-qcom.c           | 13 +++++++++----
 drivers/usb/host/xhci-plat.c           |  2 ++
 3 files changed, 25 insertions(+), 4 deletions(-)

-- 
2.25.1

[PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Manivannan Sadhasivam]

Add missing quirks for the USB DWC3 IP.

Cc: stable@vger.kernel.org # 5.20
Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index 0d02599d8867..266a94c712aa 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
 				iommus = <&apps_smmu 0x820 0x0>;
 				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
 				phy-names = "usb2-phy", "usb3-phy";
+				snps,hird-threshold = /bits/ 8 <0x0>;
+				snps,usb2-gadget-lpm-disable;
+				snps,is-utmi-l1-suspend;
+				snps,dis-u1-entry-quirk;
+				snps,dis-u2-entry-quirk;
+				snps,has-lpm-erratum;
+				tx-fifo-resize;
 
 				port {
 					usb_0_role_switch: endpoint {
@@ -3100,6 +3107,13 @@ usb_1_dwc3: usb@a800000 {
 				iommus = <&apps_smmu 0x860 0x0>;
 				phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
 				phy-names = "usb2-phy", "usb3-phy";
+				snps,hird-threshold = /bits/ 8 <0x0>;
+				snps,usb2-gadget-lpm-disable;
+				snps,is-utmi-l1-suspend;
+				snps,dis-u1-entry-quirk;
+				snps,dis-u2-entry-quirk;
+				snps,has-lpm-erratum;
+				tx-fifo-resize;
 
 				port {
 					usb_1_role_switch: endpoint {
-- 
2.25.1

[PATCH 2/5] xhci: host: Use 200ms autosuspend delay for runtime suspend

[Manivannan Sadhasivam]

On the Qcom platforms making use of DWC3 IP, xhci IRQs are received after
some delay while resuming from runtime suspend. Due to this, the xhci
driver once resumed, quickly goes into runtime suspend mode again, thus
deferring the usb enumeration.

So let's add a 200ms autosuspend delay to the xhci driver so that it get's
enough time to catch the IRQs.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/usb/host/xhci-plat.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index b9f9625467d6..f09510d3effe 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -173,6 +173,8 @@ int xhci_plat_probe(struct platform_device *pdev, struct device *sysdev, const s
 		return ret;
 
 	pm_runtime_set_active(&pdev->dev);
+	pm_runtime_set_autosuspend_delay(&pdev->dev, 200);
+	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_enable(&pdev->dev);
 	pm_runtime_get_noresume(&pdev->dev);
 
-- 
2.25.1

[PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()

[Manivannan Sadhasivam]

When runtime PM is enabled during probe, the PM core suspends this driver
before probing the dwc3 driver. Due to this, the dwc3_qcom_is_host()
function dereferences the driver data of the dwc platform device which
will only be set if the dwc driver has been probed. This causes null
pointer dereference during boot time.

So let's add a check for dwc drvdata in the callers of dwc3_qcom_is_host()
such as dwc3_qcom_suspend() and dwc3_qcom_resume() functions. There is no
need to add the same check in another caller dwc3_qcom_resume_irq() as the
wakeup IRQs will only be enabled at the end of dwc3_qcom_suspend().

Note that the check should not be added to dwc3_qcom_is_host() function
itself, as there is no provision to pass the context to callers.

Fixes: a872ab303d5d ("usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 959fc925ca7c..bbf67f705d0d 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -411,10 +411,11 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
 
 static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 {
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 	u32 val;
 	int i, ret;
 
-	if (qcom->is_suspended)
+	if (qcom->is_suspended || !dwc)
 		return 0;
 
 	val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
@@ -444,10 +445,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
 
 static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
 {
+	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
 	int ret;
 	int i;
 
-	if (!qcom->is_suspended)
+	if (!qcom->is_suspended || !dwc)
 		return 0;
 
 	if (dwc3_qcom_is_host(qcom) && wakeup)
-- 
2.25.1

[PATCH 4/5] usb: dwc3: qcom: Clear pending interrupt before enabling wake interrupt

[Manivannan Sadhasivam]

It is possible that there may be a pending interrupt logged into the dwc IP
while the interrupts were disabled in the driver. And when the wakeup
interrupt is enabled, the pending interrupt might fire which is not
required to be serviced by the driver.

So always clear the pending interrupt before enabling wake interrupt.

Cc: stable@vger.kernel.org # 5.20
Fixes: 360e8230516d ("usb: dwc3: qcom: Add helper functions to enable,disable wake irqs")
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index bbf67f705d0d..f1059dfcb0e8 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -346,6 +346,8 @@ static void dwc3_qcom_enable_wakeup_irq(int irq, unsigned int polarity)
 	if (!irq)
 		return;
 
+	irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, 0);
+
 	if (polarity)
 		irq_set_irq_type(irq, polarity);
 
-- 
2.25.1

[PATCH 5/5] usb: dwc3: qcom: Allow runtime PM

[Manivannan Sadhasivam]

dwc3-qcom driver is capable of doing runtime PM on its own, but currently
it requires userspace intervention to enable it. But there is no harm in
letting the driver to enable runtime PM on its own. So let's get rid of the
"pm_runtime_forbid()" and make sure that the dependency is maintained with
child devices using "pm_suspend_ignore_children(dev, false)".

Also during remove(), the device needs to be waken up first if it was
runtime suspended. Finally, pm_runtime_allow() can be removed.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/usb/dwc3/dwc3-qcom.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index f1059dfcb0e8..5f26bb66274f 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
 	qcom->is_suspended = false;
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
-	pm_runtime_forbid(dev);
+	pm_suspend_ignore_children(dev, false);
 
 	return 0;
 
@@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int i;
 
+	pm_runtime_get_sync(dev);
+
 	device_remove_software_node(&qcom->dwc3->dev);
 	of_platform_depopulate(dev);
 
@@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
 	dwc3_qcom_interconnect_exit(qcom);
 	reset_control_assert(qcom->resets);
 
-	pm_runtime_allow(dev);
 	pm_runtime_disable(dev);
 
 	return 0;
-- 
2.25.1

Re: [PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[Konrad Dybcio]

On 25.03.2023 17:52, Manivannan Sadhasivam wrote:
> Hi,
> 
> This series allows the dwc3-qcom driver to do runtime PM itself without
> userspace intervention. Still, userspace is required to enable runtime PM
> for dwc3 glue and xhci drivers as we cannot enable runtime PM for them.
> But this series avoids one more additional step.
What sort of 'userspace intervention' are we talking about?
echo mem > /sys/power/state?

Konrad
> 
> While enabling runtime PM, I noticed that the xhci driver suspends before
> catching the xhci interrupts during resume. This ended up deferring the
> device enumeration for some time. So I included a patch adding autosuspend
> delay of 200ms to the xhci driver. With this delay, usb enumeration happens
> properly.
> 
> This series has been tested on SC8280XP-CRD and RB5 devices.
> 
> Thanks,
> Mani
> 
> Manivannan Sadhasivam (5):
>   arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks
>   xhci: host: Use 200ms autosuspend delay for runtime suspend
>   usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
>   usb: dwc3: qcom: Clear pending interrupt before enabling wake
>     interrupt
>   usb: dwc3: qcom: Allow runtime PM
> 
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>  drivers/usb/dwc3/dwc3-qcom.c           | 13 +++++++++----
>  drivers/usb/host/xhci-plat.c           |  2 ++
>  3 files changed, 25 insertions(+), 4 deletions(-)
> 

Re: [PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[Manivannan Sadhasivam]

On Mon, Mar 27, 2023 at 11:01:35AM +0200, Konrad Dybcio wrote:
> 
> 
> On 25.03.2023 17:52, Manivannan Sadhasivam wrote:
> > Hi,
> > 
> > This series allows the dwc3-qcom driver to do runtime PM itself without
> > userspace intervention. Still, userspace is required to enable runtime PM
> > for dwc3 glue and xhci drivers as we cannot enable runtime PM for them.
> > But this series avoids one more additional step.
> What sort of 'userspace intervention' are we talking about?
> echo mem > /sys/power/state?
> 

I forgot to add that bit:

echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/xhci-hcd.1.auto/power/control
echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/power/control

You need to set "auto" for the runtime control for both xhci and dwc drivers.

Then if you don't connect a usb device, all 3 drivers (dwc3-qcom, dwc3, and
xhci) will become runtime suspended after a delay of 5s (default delay).

This can be confirmed by:

cat /sys/devices/platform/soc@0/a8f8800.usb/power/runtime_status

After connecting a usb device, they will all become "active".

Thanks,
Mani

> Konrad
> > 
> > While enabling runtime PM, I noticed that the xhci driver suspends before
> > catching the xhci interrupts during resume. This ended up deferring the
> > device enumeration for some time. So I included a patch adding autosuspend
> > delay of 200ms to the xhci driver. With this delay, usb enumeration happens
> > properly.
> > 
> > This series has been tested on SC8280XP-CRD and RB5 devices.
> > 
> > Thanks,
> > Mani
> > 
> > Manivannan Sadhasivam (5):
> >   arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks
> >   xhci: host: Use 200ms autosuspend delay for runtime suspend
> >   usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
> >   usb: dwc3: qcom: Clear pending interrupt before enabling wake
> >     interrupt
> >   usb: dwc3: qcom: Allow runtime PM
> > 
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
> >  drivers/usb/dwc3/dwc3-qcom.c           | 13 +++++++++----
> >  drivers/usb/host/xhci-plat.c           |  2 ++
> >  3 files changed, 25 insertions(+), 4 deletions(-)
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[Konrad Dybcio]

On 27.03.2023 11:17, Manivannan Sadhasivam wrote:
> On Mon, Mar 27, 2023 at 11:01:35AM +0200, Konrad Dybcio wrote:
>>
>>
>> On 25.03.2023 17:52, Manivannan Sadhasivam wrote:
>>> Hi,
>>>
>>> This series allows the dwc3-qcom driver to do runtime PM itself without
>>> userspace intervention. Still, userspace is required to enable runtime PM
>>> for dwc3 glue and xhci drivers as we cannot enable runtime PM for them.
>>> But this series avoids one more additional step.
>> What sort of 'userspace intervention' are we talking about?
>> echo mem > /sys/power/state?
>>
> 
> I forgot to add that bit:
> 
> echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/xhci-hcd.1.auto/power/control
> echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/power/control
> 
> You need to set "auto" for the runtime control for both xhci and dwc drivers.
> 
> Then if you don't connect a usb device, all 3 drivers (dwc3-qcom, dwc3, and
> xhci) will become runtime suspended after a delay of 5s (default delay).
> 
> This can be confirmed by:
> 
> cat /sys/devices/platform/soc@0/a8f8800.usb/power/runtime_status
> 
> After connecting a usb device, they will all become "active".
Thanks! And if I'm following correctly, we can't enable runtime PM
for the DWC3 glue and XHCI drivers, as that would cause havoc on
other, non-qc platforms. Is that correct?

Konrad
> 
> Thanks,
> Mani
> 
>> Konrad
>>>
>>> While enabling runtime PM, I noticed that the xhci driver suspends before
>>> catching the xhci interrupts during resume. This ended up deferring the
>>> device enumeration for some time. So I included a patch adding autosuspend
>>> delay of 200ms to the xhci driver. With this delay, usb enumeration happens
>>> properly.
>>>
>>> This series has been tested on SC8280XP-CRD and RB5 devices.
>>>
>>> Thanks,
>>> Mani
>>>
>>> Manivannan Sadhasivam (5):
>>>   arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks
>>>   xhci: host: Use 200ms autosuspend delay for runtime suspend
>>>   usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
>>>   usb: dwc3: qcom: Clear pending interrupt before enabling wake
>>>     interrupt
>>>   usb: dwc3: qcom: Allow runtime PM
>>>
>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>>>  drivers/usb/dwc3/dwc3-qcom.c           | 13 +++++++++----
>>>  drivers/usb/host/xhci-plat.c           |  2 ++
>>>  3 files changed, 25 insertions(+), 4 deletions(-)
>>>
> 

Re: [PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[Manivannan Sadhasivam]

On Mon, Mar 27, 2023 at 11:24:58AM +0200, Konrad Dybcio wrote:
> 
> 
> On 27.03.2023 11:17, Manivannan Sadhasivam wrote:
> > On Mon, Mar 27, 2023 at 11:01:35AM +0200, Konrad Dybcio wrote:
> >>
> >>
> >> On 25.03.2023 17:52, Manivannan Sadhasivam wrote:
> >>> Hi,
> >>>
> >>> This series allows the dwc3-qcom driver to do runtime PM itself without
> >>> userspace intervention. Still, userspace is required to enable runtime PM
> >>> for dwc3 glue and xhci drivers as we cannot enable runtime PM for them.
> >>> But this series avoids one more additional step.
> >> What sort of 'userspace intervention' are we talking about?
> >> echo mem > /sys/power/state?
> >>
> > 
> > I forgot to add that bit:
> > 
> > echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/xhci-hcd.1.auto/power/control
> > echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/power/control
> > 
> > You need to set "auto" for the runtime control for both xhci and dwc drivers.
> > 
> > Then if you don't connect a usb device, all 3 drivers (dwc3-qcom, dwc3, and
> > xhci) will become runtime suspended after a delay of 5s (default delay).
> > 
> > This can be confirmed by:
> > 
> > cat /sys/devices/platform/soc@0/a8f8800.usb/power/runtime_status
> > 
> > After connecting a usb device, they will all become "active".
> Thanks! And if I'm following correctly, we can't enable runtime PM
> for the DWC3 glue and XHCI drivers, as that would cause havoc on
> other, non-qc platforms. Is that correct?
> 

Kind of. Actually dwc3 glue is our qcom driver, other one is just dwc3 driver.

The havoc would apply to xhci driver because, once it is suspended, one of its
parent drivers has to resume it. And that requires runtime PM support for all
the parent drivers which is currently not available.

But for dwc3 driver, I'm not sure about the consequence though. Maybe I should
send it as a separate patch later on and see what other platforms folks think
of it.

Thanks,
Mani

> Konrad
> > 
> > Thanks,
> > Mani
> > 
> >> Konrad
> >>>
> >>> While enabling runtime PM, I noticed that the xhci driver suspends before
> >>> catching the xhci interrupts during resume. This ended up deferring the
> >>> device enumeration for some time. So I included a patch adding autosuspend
> >>> delay of 200ms to the xhci driver. With this delay, usb enumeration happens
> >>> properly.
> >>>
> >>> This series has been tested on SC8280XP-CRD and RB5 devices.
> >>>
> >>> Thanks,
> >>> Mani
> >>>
> >>> Manivannan Sadhasivam (5):
> >>>   arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks
> >>>   xhci: host: Use 200ms autosuspend delay for runtime suspend
> >>>   usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
> >>>   usb: dwc3: qcom: Clear pending interrupt before enabling wake
> >>>     interrupt
> >>>   usb: dwc3: qcom: Allow runtime PM
> >>>
> >>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
> >>>  drivers/usb/dwc3/dwc3-qcom.c           | 13 +++++++++----
> >>>  drivers/usb/host/xhci-plat.c           |  2 ++
> >>>  3 files changed, 25 insertions(+), 4 deletions(-)
> >>>
> > 

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 0/5] usb: dwc3: qcom: Allow runtime PM

[Konrad Dybcio]

On 27.03.2023 12:10, Manivannan Sadhasivam wrote:
> On Mon, Mar 27, 2023 at 11:24:58AM +0200, Konrad Dybcio wrote:
>>
>>
>> On 27.03.2023 11:17, Manivannan Sadhasivam wrote:
>>> On Mon, Mar 27, 2023 at 11:01:35AM +0200, Konrad Dybcio wrote:
>>>>
>>>>
>>>> On 25.03.2023 17:52, Manivannan Sadhasivam wrote:
>>>>> Hi,
>>>>>
>>>>> This series allows the dwc3-qcom driver to do runtime PM itself without
>>>>> userspace intervention. Still, userspace is required to enable runtime PM
>>>>> for dwc3 glue and xhci drivers as we cannot enable runtime PM for them.
>>>>> But this series avoids one more additional step.
>>>> What sort of 'userspace intervention' are we talking about?
>>>> echo mem > /sys/power/state?
>>>>
>>>
>>> I forgot to add that bit:
>>>
>>> echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/xhci-hcd.1.auto/power/control
>>> echo auto > /sys/devices/platform/soc@0/a8f8800.usb/a800000.usb/power/control
>>>
>>> You need to set "auto" for the runtime control for both xhci and dwc drivers.
>>>
>>> Then if you don't connect a usb device, all 3 drivers (dwc3-qcom, dwc3, and
>>> xhci) will become runtime suspended after a delay of 5s (default delay).
>>>
>>> This can be confirmed by:
>>>
>>> cat /sys/devices/platform/soc@0/a8f8800.usb/power/runtime_status
>>>
>>> After connecting a usb device, they will all become "active".
>> Thanks! And if I'm following correctly, we can't enable runtime PM
>> for the DWC3 glue and XHCI drivers, as that would cause havoc on
>> other, non-qc platforms. Is that correct?
>>
> 
> Kind of. Actually dwc3 glue is our qcom driver, other one is just dwc3 driver.
> 
> The havoc would apply to xhci driver because, once it is suspended, one of its
> parent drivers has to resume it. And that requires runtime PM support for all
> the parent drivers which is currently not available.
Makes sense, thanks for the explanation!

> 
> But for dwc3 driver, I'm not sure about the consequence though. Maybe I should
> send it as a separate patch later on and see what other platforms folks think
> of it.
May be worth a shot!

Konrad

> 
> Thanks,
> Mani
> 
>> Konrad
>>>
>>> Thanks,
>>> Mani
>>>
>>>> Konrad
>>>>>
>>>>> While enabling runtime PM, I noticed that the xhci driver suspends before
>>>>> catching the xhci interrupts during resume. This ended up deferring the
>>>>> device enumeration for some time. So I included a patch adding autosuspend
>>>>> delay of 200ms to the xhci driver. With this delay, usb enumeration happens
>>>>> properly.
>>>>>
>>>>> This series has been tested on SC8280XP-CRD and RB5 devices.
>>>>>
>>>>> Thanks,
>>>>> Mani
>>>>>
>>>>> Manivannan Sadhasivam (5):
>>>>>   arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks
>>>>>   xhci: host: Use 200ms autosuspend delay for runtime suspend
>>>>>   usb: dwc3: qcom: Fix null ptr access during runtime_suspend()
>>>>>   usb: dwc3: qcom: Clear pending interrupt before enabling wake
>>>>>     interrupt
>>>>>   usb: dwc3: qcom: Allow runtime PM
>>>>>
>>>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>>>>>  drivers/usb/dwc3/dwc3-qcom.c           | 13 +++++++++----
>>>>>  drivers/usb/host/xhci-plat.c           |  2 ++
>>>>>  3 files changed, 25 insertions(+), 4 deletions(-)
>>>>>
>>>
> 

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Johan Hovold]

On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
> Add missing quirks for the USB DWC3 IP.

This is not an acceptable commit message generally and certainly not for
something that you have tagged for stable.

At a minimum, you need to describe why these are needed and what the
impact is.

Also, why are you sending as part of a series purporting to enable
runtime PM when it appears to be all about optimising specific gadget
applications?

Did you confirm that the below makes any sense or has this just been
copied verbatim from the vendor devicetree (it looks like that)?

The fact that almost none of the qcom SoCs sets these also indicates
that something is not right here.

> Cc: stable@vger.kernel.org # 5.20
> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index 0d02599d8867..266a94c712aa 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
>  				iommus = <&apps_smmu 0x820 0x0>;
>  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
>  				phy-names = "usb2-phy", "usb3-phy";
> +				snps,hird-threshold = /bits/ 8 <0x0>;
> +				snps,usb2-gadget-lpm-disable;

Here you are disabling LPM for gadget mode, which makes most of the
other properties entirely pointless.

> +				snps,is-utmi-l1-suspend;
> +				snps,dis-u1-entry-quirk;
> +				snps,dis-u2-entry-quirk;

These appear to be used to optimise certain gadget application and
likely not something that should be set in a dtsi.

> +				snps,has-lpm-erratum;
> +				tx-fifo-resize;

Same here.

>  				port {
>  					usb_0_role_switch: endpoint {

Johan

Re: [PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()

[Johan Hovold]

On Sat, Mar 25, 2023 at 10:22:15PM +0530, Manivannan Sadhasivam wrote:
> When runtime PM is enabled during probe, the PM core suspends this driver
> before probing the dwc3 driver. Due to this, the dwc3_qcom_is_host()
> function dereferences the driver data of the dwc platform device which
> will only be set if the dwc driver has been probed. This causes null
> pointer dereference during boot time.

So this does not really appear to be an issue before your later patch
which enables runtime PM at probe.

But the layering violations we have in this driver are indeed fragile
and should be fixed properly at some point.

> So let's add a check for dwc drvdata in the callers of dwc3_qcom_is_host()
> such as dwc3_qcom_suspend() and dwc3_qcom_resume() functions. There is no
> need to add the same check in another caller dwc3_qcom_resume_irq() as the
> wakeup IRQs will only be enabled at the end of dwc3_qcom_suspend().
> 
> Note that the check should not be added to dwc3_qcom_is_host() function
> itself, as there is no provision to pass the context to callers.
> 
> Fixes: a872ab303d5d ("usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup")

This is not the right fixes tag in any case as this layering violation
was first added by:

6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")

which started accessing the dwc3 platform data and xhci host data from
the glue driver (and broke gadget mode).

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 959fc925ca7c..bbf67f705d0d 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -411,10 +411,11 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
>  
>  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>  {
> +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  	u32 val;
>  	int i, ret;
>  
> -	if (qcom->is_suspended)
> +	if (qcom->is_suspended || !dwc)
>  		return 0;

I think we should try to keep the layering violations confined to the
helper functions. So how about amending dwc3_qcom_is_host() and check
for NULL before dereferencing the xhci pointer?

If the dwc3 driver hasn't probed yet, we're clearly not in host mode
either...

Johan

Re: [PATCH 4/5] usb: dwc3: qcom: Clear pending interrupt before enabling wake interrupt

[Johan Hovold]

On Sat, Mar 25, 2023 at 10:22:16PM +0530, Manivannan Sadhasivam wrote:
> It is possible that there may be a pending interrupt logged into the dwc IP
> while the interrupts were disabled in the driver. And when the wakeup
> interrupt is enabled, the pending interrupt might fire which is not
> required to be serviced by the driver.
> 
> So always clear the pending interrupt before enabling wake interrupt.
> 
> Cc: stable@vger.kernel.org # 5.20
> Fixes: 360e8230516d ("usb: dwc3: qcom: Add helper functions to enable,disable wake irqs")
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index bbf67f705d0d..f1059dfcb0e8 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -346,6 +346,8 @@ static void dwc3_qcom_enable_wakeup_irq(int irq, unsigned int polarity)
>  	if (!irq)
>  		return;
>  
> +	irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, 0);
> +

This looks like a hack (and a layering violation). Note that there are
no other non-irqchip drivers calling this function.

>  	if (polarity)
>  		irq_set_irq_type(irq, polarity);

Johan

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
> On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
> > Add missing quirks for the USB DWC3 IP.
> 
> This is not an acceptable commit message generally and certainly not for
> something that you have tagged for stable.
> 
> At a minimum, you need to describe why these are needed and what the
> impact is.
> 

I can certainly improve the commit message. But usually the quirks are copied
from the downstream devicetree where qualcomm engineers would've added them
based on the platform requirements.

> Also, why are you sending as part of a series purporting to enable
> runtime PM when it appears to be all about optimising specific gadget
> applications?
> 

It's not related to this series I agree but just wanted to group it with a
series touching usb so that it won't get lost.

I could respin it separately though in v2.

> Did you confirm that the below makes any sense or has this just been
> copied verbatim from the vendor devicetree (it looks like that)?
> 

As you've mentioned, most of the quirks are for gadget mode which is not
supported by the upstream supported boards. So I haven't really tested them but
for I assumed that Qcom engineers did.

> The fact that almost none of the qcom SoCs sets these also indicates
> that something is not right here.
> 
> > Cc: stable@vger.kernel.org # 5.20
> > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
> >  1 file changed, 14 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > index 0d02599d8867..266a94c712aa 100644
> > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
> >  				iommus = <&apps_smmu 0x820 0x0>;
> >  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
> >  				phy-names = "usb2-phy", "usb3-phy";
> > +				snps,hird-threshold = /bits/ 8 <0x0>;
> > +				snps,usb2-gadget-lpm-disable;
> 
> Here you are disabling LPM for gadget mode, which makes most of the
> other properties entirely pointless.
> 
> > +				snps,is-utmi-l1-suspend;
> > +				snps,dis-u1-entry-quirk;
> > +				snps,dis-u2-entry-quirk;
> 
> These appear to be used to optimise certain gadget application and
> likely not something that should be set in a dtsi.
> 

I will cross check these with Qcom and respin accordingly.

- Mani

> > +				snps,has-lpm-erratum;
> > +				tx-fifo-resize;
> 
> Same here.
> 
> >  				port {
> >  					usb_0_role_switch: endpoint {
> 
> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 5/5] usb: dwc3: qcom: Allow runtime PM

[Johan Hovold]

On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote:
> dwc3-qcom driver is capable of doing runtime PM on its own, but currently
> it requires userspace intervention to enable it. But there is no harm in
> letting the driver to enable runtime PM on its own. So let's get rid of the
> "pm_runtime_forbid()" and make sure that the dependency is maintained with
> child devices using "pm_suspend_ignore_children(dev, false)".

Well, the potential harm is that these paths have hardly been tested so
enabling it by default is a risk (e.g. as you noticed when trying to
enable this by default). And especially if we don't address the layering
violations first.

> Also during remove(), the device needs to be waken up first if it was
> runtime suspended. Finally, pm_runtime_allow() can be removed.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index f1059dfcb0e8..5f26bb66274f 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
>  	qcom->is_suspended = false;
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
> -	pm_runtime_forbid(dev);
> +	pm_suspend_ignore_children(dev, false);

There's no need to explicitly disable ignore-children as that is the
default.

>  	return 0;
>  
> @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int i;
>  
> +	pm_runtime_get_sync(dev);

This call needs to be balanced. But this is a fix for a bug in the
current implementation that should go in a separate patch.

> +
>  	device_remove_software_node(&qcom->dwc3->dev);
>  	of_platform_depopulate(dev);
>  
> @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
>  	dwc3_qcom_interconnect_exit(qcom);
>  	reset_control_assert(qcom->resets);
>  
> -	pm_runtime_allow(dev);
>  	pm_runtime_disable(dev);
>  
>  	return 0;

Johan

Re: [PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 11:23:32AM +0200, Johan Hovold wrote:
> On Sat, Mar 25, 2023 at 10:22:15PM +0530, Manivannan Sadhasivam wrote:
> > When runtime PM is enabled during probe, the PM core suspends this driver
> > before probing the dwc3 driver. Due to this, the dwc3_qcom_is_host()
> > function dereferences the driver data of the dwc platform device which
> > will only be set if the dwc driver has been probed. This causes null
> > pointer dereference during boot time.
> 
> So this does not really appear to be an issue before your later patch
> which enables runtime PM at probe.
> 

right.

> But the layering violations we have in this driver are indeed fragile
> and should be fixed properly at some point.
> 
> > So let's add a check for dwc drvdata in the callers of dwc3_qcom_is_host()
> > such as dwc3_qcom_suspend() and dwc3_qcom_resume() functions. There is no
> > need to add the same check in another caller dwc3_qcom_resume_irq() as the
> > wakeup IRQs will only be enabled at the end of dwc3_qcom_suspend().
> > 
> > Note that the check should not be added to dwc3_qcom_is_host() function
> > itself, as there is no provision to pass the context to callers.
> > 
> > Fixes: a872ab303d5d ("usb: dwc3: qcom: fix use-after-free on runtime-PM wakeup")
> 
> This is not the right fixes tag in any case as this layering violation
> was first added by:
> 
> 6895ea55c385 ("usb: dwc3: qcom: Configure wakeup interrupts during suspend")
> 
> which started accessing the dwc3 platform data and xhci host data from
> the glue driver (and broke gadget mode).
> 

ah, I missed it, thanks for spotting.

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index 959fc925ca7c..bbf67f705d0d 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -411,10 +411,11 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
> >  
> >  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> >  {
> > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >  	u32 val;
> >  	int i, ret;
> >  
> > -	if (qcom->is_suspended)
> > +	if (qcom->is_suspended || !dwc)
> >  		return 0;
> 
> I think we should try to keep the layering violations confined to the
> helper functions. So how about amending dwc3_qcom_is_host() and check
> for NULL before dereferencing the xhci pointer?
> 
> If the dwc3 driver hasn't probed yet, we're clearly not in host mode
> either...
> 

Well, that's what I initially did but then I reverted to this approach as
returning true/false from dwc3_qcom_is_host() based on the pointer availability
doesn't sound right.

For example, if we return true then it implies that the driver is in host mode
which is logically wrong (before dwc3 probe) even though there is no impact.

- Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 4/5] usb: dwc3: qcom: Clear pending interrupt before enabling wake interrupt

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 11:28:32AM +0200, Johan Hovold wrote:
> On Sat, Mar 25, 2023 at 10:22:16PM +0530, Manivannan Sadhasivam wrote:
> > It is possible that there may be a pending interrupt logged into the dwc IP
> > while the interrupts were disabled in the driver. And when the wakeup
> > interrupt is enabled, the pending interrupt might fire which is not
> > required to be serviced by the driver.
> > 
> > So always clear the pending interrupt before enabling wake interrupt.
> > 
> > Cc: stable@vger.kernel.org # 5.20
> > Fixes: 360e8230516d ("usb: dwc3: qcom: Add helper functions to enable,disable wake irqs")
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index bbf67f705d0d..f1059dfcb0e8 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -346,6 +346,8 @@ static void dwc3_qcom_enable_wakeup_irq(int irq, unsigned int polarity)
> >  	if (!irq)
> >  		return;
> >  
> > +	irq_set_irqchip_state(irq, IRQCHIP_STATE_PENDING, 0);
> > +
> 
> This looks like a hack (and a layering violation). Note that there are
> no other non-irqchip drivers calling this function.
> 

I can check if this can be achieved by clearing some dwc specific registers.

- Mani

> >  	if (polarity)
> >  		irq_set_irq_type(irq, polarity);
> 
> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()

[Johan Hovold]

On Tue, Mar 28, 2023 at 03:17:18PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 28, 2023 at 11:23:32AM +0200, Johan Hovold wrote:
> > On Sat, Mar 25, 2023 at 10:22:15PM +0530, Manivannan Sadhasivam wrote:

> > >  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> > >  {
> > > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > >  	u32 val;
> > >  	int i, ret;
> > >  
> > > -	if (qcom->is_suspended)
> > > +	if (qcom->is_suspended || !dwc)
> > >  		return 0;
> > 
> > I think we should try to keep the layering violations confined to the
> > helper functions. So how about amending dwc3_qcom_is_host() and check
> > for NULL before dereferencing the xhci pointer?
> > 
> > If the dwc3 driver hasn't probed yet, we're clearly not in host mode
> > either...
> 
> Well, that's what I initially did but then I reverted to this approach as
> returning true/false from dwc3_qcom_is_host() based on the pointer availability
> doesn't sound right.
> 
> For example, if we return true then it implies that the driver is in host mode
> which is logically wrong (before dwc3 probe) even though there is no impact.

No, you should return false of course as we are *not* in host mode as I
mentioned above.

Johan

Re: [PATCH 5/5] usb: dwc3: qcom: Allow runtime PM

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 11:46:36AM +0200, Johan Hovold wrote:
> On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote:
> > dwc3-qcom driver is capable of doing runtime PM on its own, but currently
> > it requires userspace intervention to enable it. But there is no harm in
> > letting the driver to enable runtime PM on its own. So let's get rid of the
> > "pm_runtime_forbid()" and make sure that the dependency is maintained with
> > child devices using "pm_suspend_ignore_children(dev, false)".
> 
> Well, the potential harm is that these paths have hardly been tested so
> enabling it by default is a risk (e.g. as you noticed when trying to
> enable this by default). And especially if we don't address the layering
> violations first.
> 

I certainly tested this on a couple of boards with host and gadget mode and
noticed no issue (except one issue noticed by Steev on a docking station with
display but that should be related to orientation switch).

Even if we allow runtime PM on this driver, still userspace needs to enable it
for dwc3 and xhci drivers. So this essentially reduces one step in that process
if someone tries to enable runtime PM for usb intentionally. So I don't forsee a
potential harm here.

> > Also during remove(), the device needs to be waken up first if it was
> > runtime suspended. Finally, pm_runtime_allow() can be removed.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/usb/dwc3/dwc3-qcom.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > index f1059dfcb0e8..5f26bb66274f 100644
> > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> >  	qcom->is_suspended = false;
> >  	pm_runtime_set_active(dev);
> >  	pm_runtime_enable(dev);
> > -	pm_runtime_forbid(dev);
> > +	pm_suspend_ignore_children(dev, false);
> 
> There's no need to explicitly disable ignore-children as that is the
> default.
> 

Other dwc drivers were doing it, so I thought someone (maintainer) wanted to
explicitly disable ignore_children. But if that's not the case, I can remove it.

> >  	return 0;
> >  
> > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> >  	struct device *dev = &pdev->dev;
> >  	int i;
> >  
> > +	pm_runtime_get_sync(dev);
> 
> This call needs to be balanced. But this is a fix for a bug in the
> current implementation that should go in a separate patch.
> 

Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable.

- Mani

> > +
> >  	device_remove_software_node(&qcom->dwc3->dev);
> >  	of_platform_depopulate(dev);
> >  
> > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> >  	dwc3_qcom_interconnect_exit(qcom);
> >  	reset_control_assert(qcom->resets);
> >  
> > -	pm_runtime_allow(dev);
> >  	pm_runtime_disable(dev);
> >  
> >  	return 0;
> 
> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 3/5] usb: dwc3: qcom: Fix null ptr access during runtime_suspend()

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 11:51:53AM +0200, Johan Hovold wrote:
> On Tue, Mar 28, 2023 at 03:17:18PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Mar 28, 2023 at 11:23:32AM +0200, Johan Hovold wrote:
> > > On Sat, Mar 25, 2023 at 10:22:15PM +0530, Manivannan Sadhasivam wrote:
> 
> > > >  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> > > >  {
> > > > +	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> > > >  	u32 val;
> > > >  	int i, ret;
> > > >  
> > > > -	if (qcom->is_suspended)
> > > > +	if (qcom->is_suspended || !dwc)
> > > >  		return 0;
> > > 
> > > I think we should try to keep the layering violations confined to the
> > > helper functions. So how about amending dwc3_qcom_is_host() and check
> > > for NULL before dereferencing the xhci pointer?
> > > 
> > > If the dwc3 driver hasn't probed yet, we're clearly not in host mode
> > > either...
> > 
> > Well, that's what I initially did but then I reverted to this approach as
> > returning true/false from dwc3_qcom_is_host() based on the pointer availability
> > doesn't sound right.
> > 
> > For example, if we return true then it implies that the driver is in host mode
> > which is logically wrong (before dwc3 probe) even though there is no impact.
> 
> No, you should return false of course as we are *not* in host mode as I
> mentioned above.
> 

Yes, but I interpreted it as "we are in device mode" in that case. But looking
at it again, I think it just conveys that the controller is not in host mode
only.

- Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 5/5] usb: dwc3: qcom: Allow runtime PM

[Johan Hovold]

On Tue, Mar 28, 2023 at 03:35:01PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 28, 2023 at 11:46:36AM +0200, Johan Hovold wrote:
> > On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote:
> > > dwc3-qcom driver is capable of doing runtime PM on its own, but currently
> > > it requires userspace intervention to enable it. But there is no harm in
> > > letting the driver to enable runtime PM on its own. So let's get rid of the
> > > "pm_runtime_forbid()" and make sure that the dependency is maintained with
> > > child devices using "pm_suspend_ignore_children(dev, false)".
> > 
> > Well, the potential harm is that these paths have hardly been tested so
> > enabling it by default is a risk (e.g. as you noticed when trying to
> > enable this by default). And especially if we don't address the layering
> > violations first.
> > 
> 
> I certainly tested this on a couple of boards with host and gadget mode and
> noticed no issue (except one issue noticed by Steev on a docking station with
> display but that should be related to orientation switch).
> 
> Even if we allow runtime PM on this driver, still userspace needs to enable it
> for dwc3 and xhci drivers. So this essentially reduces one step in that process
> if someone tries to enable runtime PM for usb intentionally. So I don't forsee a
> potential harm here.

Well this whole driver is a mess so I don't have any problem imagining
ways in which things can break. ;)

> > > Also during remove(), the device needs to be waken up first if it was
> > > runtime suspended. Finally, pm_runtime_allow() can be removed.
> > > 
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  drivers/usb/dwc3/dwc3-qcom.c | 5 +++--
> > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > index f1059dfcb0e8..5f26bb66274f 100644
> > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > >  	qcom->is_suspended = false;
> > >  	pm_runtime_set_active(dev);
> > >  	pm_runtime_enable(dev);
> > > -	pm_runtime_forbid(dev);
> > > +	pm_suspend_ignore_children(dev, false);
> > 
> > There's no need to explicitly disable ignore-children as that is the
> > default.
> > 
> 
> Other dwc drivers were doing it, so I thought someone (maintainer) wanted to
> explicitly disable ignore_children. But if that's not the case, I can remove it.

Yeah, please remove it. I doubt these runtime pm implementations have
gotten much review.

Note how several dwc3 glue drivers just do an unconditional get in
probe(), which means that these paths are probably never exercised at
all and effectively amounts to that pm_runtime_forbid() you are removing
here.

Probably there to tick off "runtime pm" on some internal project
manager's list of "features".

> > >  	return 0;
> > >  
> > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> > >  	struct device *dev = &pdev->dev;
> > >  	int i;
> > >  
> > > +	pm_runtime_get_sync(dev);
> > 
> > This call needs to be balanced. But this is a fix for a bug in the
> > current implementation that should go in a separate patch.
> > 
> 
> Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable.

You should do it after disabling runtime pm.

> > > +
> > >  	device_remove_software_node(&qcom->dwc3->dev);
> > >  	of_platform_depopulate(dev);
> > >  
> > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> > >  	dwc3_qcom_interconnect_exit(qcom);
> > >  	reset_control_assert(qcom->resets);
> > >  
> > > -	pm_runtime_allow(dev);
> > >  	pm_runtime_disable(dev);
> > >  
> > >  	return 0;

Johan

Re: [PATCH 5/5] usb: dwc3: qcom: Allow runtime PM

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 02:18:16PM +0200, Johan Hovold wrote:
> On Tue, Mar 28, 2023 at 03:35:01PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Mar 28, 2023 at 11:46:36AM +0200, Johan Hovold wrote:
> > > On Sat, Mar 25, 2023 at 10:22:17PM +0530, Manivannan Sadhasivam wrote:
> > > > dwc3-qcom driver is capable of doing runtime PM on its own, but currently
> > > > it requires userspace intervention to enable it. But there is no harm in
> > > > letting the driver to enable runtime PM on its own. So let's get rid of the
> > > > "pm_runtime_forbid()" and make sure that the dependency is maintained with
> > > > child devices using "pm_suspend_ignore_children(dev, false)".
> > > 
> > > Well, the potential harm is that these paths have hardly been tested so
> > > enabling it by default is a risk (e.g. as you noticed when trying to
> > > enable this by default). And especially if we don't address the layering
> > > violations first.
> > > 
> > 
> > I certainly tested this on a couple of boards with host and gadget mode and
> > noticed no issue (except one issue noticed by Steev on a docking station with
> > display but that should be related to orientation switch).
> > 
> > Even if we allow runtime PM on this driver, still userspace needs to enable it
> > for dwc3 and xhci drivers. So this essentially reduces one step in that process
> > if someone tries to enable runtime PM for usb intentionally. So I don't forsee a
> > potential harm here.
> 
> Well this whole driver is a mess so I don't have any problem imagining
> ways in which things can break. ;)
> 
> > > > Also during remove(), the device needs to be waken up first if it was
> > > > runtime suspended. Finally, pm_runtime_allow() can be removed.
> > > > 
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  drivers/usb/dwc3/dwc3-qcom.c | 5 +++--
> > > >  1 file changed, 3 insertions(+), 2 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> > > > index f1059dfcb0e8..5f26bb66274f 100644
> > > > --- a/drivers/usb/dwc3/dwc3-qcom.c
> > > > +++ b/drivers/usb/dwc3/dwc3-qcom.c
> > > > @@ -920,7 +920,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
> > > >  	qcom->is_suspended = false;
> > > >  	pm_runtime_set_active(dev);
> > > >  	pm_runtime_enable(dev);
> > > > -	pm_runtime_forbid(dev);
> > > > +	pm_suspend_ignore_children(dev, false);
> > > 
> > > There's no need to explicitly disable ignore-children as that is the
> > > default.
> > > 
> > 
> > Other dwc drivers were doing it, so I thought someone (maintainer) wanted to
> > explicitly disable ignore_children. But if that's not the case, I can remove it.
> 
> Yeah, please remove it. I doubt these runtime pm implementations have
> gotten much review.
> 
> Note how several dwc3 glue drivers just do an unconditional get in
> probe(), which means that these paths are probably never exercised at
> all and effectively amounts to that pm_runtime_forbid() you are removing
> here.
> 
> Probably there to tick off "runtime pm" on some internal project
> manager's list of "features".
> 

Agree.

> > > >  	return 0;
> > > >  
> > > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> > > >  	struct device *dev = &pdev->dev;
> > > >  	int i;
> > > >  
> > > > +	pm_runtime_get_sync(dev);
> > > 
> > > This call needs to be balanced. But this is a fix for a bug in the
> > > current implementation that should go in a separate patch.
> > > 
> > 
> > Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable.
> 
> You should do it after disabling runtime pm.
> 

May I know why?

Thanks,
Mani

> > > > +
> > > >  	device_remove_software_node(&qcom->dwc3->dev);
> > > >  	of_platform_depopulate(dev);
> > > >  
> > > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> > > >  	dwc3_qcom_interconnect_exit(qcom);
> > > >  	reset_control_assert(qcom->resets);
> > > >  
> > > > -	pm_runtime_allow(dev);
> > > >  	pm_runtime_disable(dev);
> > > >  
> > > >  	return 0;
> 
> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 5/5] usb: dwc3: qcom: Allow runtime PM

[Johan Hovold]

On Tue, Mar 28, 2023 at 06:27:05PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 28, 2023 at 02:18:16PM +0200, Johan Hovold wrote:

> > > > > @@ -948,6 +948,8 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> > > > >  	struct device *dev = &pdev->dev;
> > > > >  	int i;
> > > > >  
> > > > > +	pm_runtime_get_sync(dev);
> > > > 
> > > > This call needs to be balanced. But this is a fix for a bug in the
> > > > current implementation that should go in a separate patch.
> > > 
> > > Ok. For balancing I could add pm_runtime_put_noidle() before pm_runtime_disable.
> > 
> > You should do it after disabling runtime pm.
> 
> May I know why?

The usage counter should be balanced after disabling runtime PM so that
there is no window where you could get a racing suspend request.

> > > > > +
> > > > >  	device_remove_software_node(&qcom->dwc3->dev);
> > > > >  	of_platform_depopulate(dev);
> > > > >  
> > > > > @@ -960,7 +962,6 @@ static int dwc3_qcom_remove(struct platform_device *pdev)
> > > > >  	dwc3_qcom_interconnect_exit(qcom);
> > > > >  	reset_control_assert(qcom->resets);
> > > > >  
> > > > > -	pm_runtime_allow(dev);
> > > > >  	pm_runtime_disable(dev);
> > > > >  
> > > > >  	return 0;

Johan

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Manivannan Sadhasivam]

On Tue, Mar 28, 2023 at 03:09:03PM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
> > On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
> > > Add missing quirks for the USB DWC3 IP.
> > 
> > This is not an acceptable commit message generally and certainly not for
> > something that you have tagged for stable.
> > 
> > At a minimum, you need to describe why these are needed and what the
> > impact is.
> > 
> 
> I can certainly improve the commit message. But usually the quirks are copied
> from the downstream devicetree where qualcomm engineers would've added them
> based on the platform requirements.
> 
> > Also, why are you sending as part of a series purporting to enable
> > runtime PM when it appears to be all about optimising specific gadget
> > applications?
> > 
> 
> It's not related to this series I agree but just wanted to group it with a
> series touching usb so that it won't get lost.
> 
> I could respin it separately though in v2.
> 
> > Did you confirm that the below makes any sense or has this just been
> > copied verbatim from the vendor devicetree (it looks like that)?
> > 
> 
> As you've mentioned, most of the quirks are for gadget mode which is not
> supported by the upstream supported boards. So I haven't really tested them but
> for I assumed that Qcom engineers did.
> 
> > The fact that almost none of the qcom SoCs sets these also indicates
> > that something is not right here.
> > 
> > > Cc: stable@vger.kernel.org # 5.20
> > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > ---
> > >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
> > >  1 file changed, 14 insertions(+)
> > > 
> > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > index 0d02599d8867..266a94c712aa 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
> > >  				iommus = <&apps_smmu 0x820 0x0>;
> > >  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
> > >  				phy-names = "usb2-phy", "usb3-phy";
> > > +				snps,hird-threshold = /bits/ 8 <0x0>;
> > > +				snps,usb2-gadget-lpm-disable;
> > 
> > Here you are disabling LPM for gadget mode, which makes most of the
> > other properties entirely pointless.
> > 

Checked with Qcom on these quirks. So this one is just disabling lpm for USB2
and rest of the quirks below are for SS/SSP modes.

> > > +				snps,is-utmi-l1-suspend;
> > > +				snps,dis-u1-entry-quirk;
> > > +				snps,dis-u2-entry-quirk;
> > 
> > These appear to be used to optimise certain gadget application and
> > likely not something that should be set in a dtsi.
> > 
> 
> I will cross check these with Qcom and respin accordingly.
> 

These quirks are needed as per the DWC IP integration with this SoC it seems.
But I got the point that these don't add any values for host only
configurations. At the same time, these quirks still hold true for the SoC even
if not exercised.

So I think we should keep these in the dtsi itself.

- Mani

> - Mani
> 
> > > +				snps,has-lpm-erratum;
> > > +				tx-fifo-resize;
> > 
> > Same here.
> > 
> > >  				port {
> > >  					usb_0_role_switch: endpoint {
> > 
> > Johan
> 
> -- 
> மணிவண்ணன் சதாசிவம்

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Johan Hovold]

On Wed, Mar 29, 2023 at 10:56:00AM +0530, Manivannan Sadhasivam wrote:
> On Tue, Mar 28, 2023 at 03:09:03PM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
> > > On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
> > > > Add missing quirks for the USB DWC3 IP.
> > > 
> > > This is not an acceptable commit message generally and certainly not for
> > > something that you have tagged for stable.
> > > 
> > > At a minimum, you need to describe why these are needed and what the
> > > impact is.
> > > 
> > 
> > I can certainly improve the commit message. But usually the quirks are copied
> > from the downstream devicetree where qualcomm engineers would've added them
> > based on the platform requirements.
> > 
> > > Also, why are you sending as part of a series purporting to enable
> > > runtime PM when it appears to be all about optimising specific gadget
> > > applications?
> > > 
> > 
> > It's not related to this series I agree but just wanted to group it with a
> > series touching usb so that it won't get lost.
> > 
> > I could respin it separately though in v2.

That's also generally best for USB patches as Greg expects series to be
merged through a single tree.

> > > Did you confirm that the below makes any sense or has this just been
> > > copied verbatim from the vendor devicetree (it looks like that)?
> > > 
> > 
> > As you've mentioned, most of the quirks are for gadget mode which is not
> > supported by the upstream supported boards. So I haven't really tested them but
> > for I assumed that Qcom engineers did.
> > 
> > > The fact that almost none of the qcom SoCs sets these also indicates
> > > that something is not right here.
> > > 
> > > > Cc: stable@vger.kernel.org # 5.20
> > > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > ---
> > > >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
> > > >  1 file changed, 14 insertions(+)
> > > > 
> > > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > index 0d02599d8867..266a94c712aa 100644
> > > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
> > > >  				iommus = <&apps_smmu 0x820 0x0>;
> > > >  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
> > > >  				phy-names = "usb2-phy", "usb3-phy";
> > > > +				snps,hird-threshold = /bits/ 8 <0x0>;
> > > > +				snps,usb2-gadget-lpm-disable;
> > > 
> > > Here you are disabling LPM for gadget mode, which makes most of the
> > > other properties entirely pointless.
> 
> Checked with Qcom on these quirks. So this one is just disabling lpm for USB2
> and rest of the quirks below are for SS/SSP modes.

No, snps,hird-threshold is for USB2 LPM and so is
snps,is-utmi-l1-suspend and snps,has-lpm-erratum as you'll see if you
look at the implementation.

> > > > +				snps,is-utmi-l1-suspend;
> > > > +				snps,dis-u1-entry-quirk;
> > > > +				snps,dis-u2-entry-quirk;
> > > 
> > > These appear to be used to optimise certain gadget application and
> > > likely not something that should be set in a dtsi.
> > > 
> > 
> > I will cross check these with Qcom and respin accordingly.
> > 
> 
> These quirks are needed as per the DWC IP integration with this SoC it seems.
> But I got the point that these don't add any values for host only
> configurations. At the same time, these quirks still hold true for the SoC even
> if not exercised.
> 
> So I think we should keep these in the dtsi itself.

Please take a closer look at the quirks you're enabling first. Commit
729dcffd1ed3 ("usb: dwc3: gadget: Add support for disabling U1 and U2
entries") which added 

> > > > +				snps,dis-u1-entry-quirk;
> > > > +				snps,dis-u2-entry-quirk;

explicitly mentions

	Gadget applications may have a requirement to disable the U1 and U2
	entry based on the usecase.

which sounds like something that needs to be done in a per board dts at
least.

Perhaps keeping all of these in in the dtsi is correct, but that's going
to need some more motivation than simply that some vendor does so (as
they often do all sorts of things they should not).

Johan

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Konrad Dybcio]

On 29.03.2023 10:34, Johan Hovold wrote:
> On Wed, Mar 29, 2023 at 10:56:00AM +0530, Manivannan Sadhasivam wrote:
>> On Tue, Mar 28, 2023 at 03:09:03PM +0530, Manivannan Sadhasivam wrote:
>>> On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
>>>> On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
>>>>> Add missing quirks for the USB DWC3 IP.
>>>>
>>>> This is not an acceptable commit message generally and certainly not for
>>>> something that you have tagged for stable.
>>>>
>>>> At a minimum, you need to describe why these are needed and what the
>>>> impact is.
>>>>
>>>
>>> I can certainly improve the commit message. But usually the quirks are copied
>>> from the downstream devicetree where qualcomm engineers would've added them
>>> based on the platform requirements.
>>>
>>>> Also, why are you sending as part of a series purporting to enable
>>>> runtime PM when it appears to be all about optimising specific gadget
>>>> applications?
>>>>
>>>
>>> It's not related to this series I agree but just wanted to group it with a
>>> series touching usb so that it won't get lost.
>>>
>>> I could respin it separately though in v2.
> 
> That's also generally best for USB patches as Greg expects series to be
> merged through a single tree.
> 
>>>> Did you confirm that the below makes any sense or has this just been
>>>> copied verbatim from the vendor devicetree (it looks like that)?
>>>>
>>>
>>> As you've mentioned, most of the quirks are for gadget mode which is not
>>> supported by the upstream supported boards. So I haven't really tested them but
>>> for I assumed that Qcom engineers did.
>>>
>>>> The fact that almost none of the qcom SoCs sets these also indicates
>>>> that something is not right here.
>>>>
>>>>> Cc: stable@vger.kernel.org # 5.20
>>>>> Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
>>>>> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
>>>>> ---
>>>>>  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
>>>>>  1 file changed, 14 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> index 0d02599d8867..266a94c712aa 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>>>> @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
>>>>>  				iommus = <&apps_smmu 0x820 0x0>;
>>>>>  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
>>>>>  				phy-names = "usb2-phy", "usb3-phy";
>>>>> +				snps,hird-threshold = /bits/ 8 <0x0>;
>>>>> +				snps,usb2-gadget-lpm-disable;
>>>>
>>>> Here you are disabling LPM for gadget mode, which makes most of the
>>>> other properties entirely pointless.
>>
>> Checked with Qcom on these quirks. So this one is just disabling lpm for USB2
>> and rest of the quirks below are for SS/SSP modes.
> 
> No, snps,hird-threshold is for USB2 LPM and so is
> snps,is-utmi-l1-suspend and snps,has-lpm-erratum as you'll see if you
> look at the implementation.
> 
>>>>> +				snps,is-utmi-l1-suspend;
>>>>> +				snps,dis-u1-entry-quirk;
>>>>> +				snps,dis-u2-entry-quirk;
>>>>
>>>> These appear to be used to optimise certain gadget application and
>>>> likely not something that should be set in a dtsi.
>>>>
>>>
>>> I will cross check these with Qcom and respin accordingly.
>>>
>>
>> These quirks are needed as per the DWC IP integration with this SoC it seems.
>> But I got the point that these don't add any values for host only
>> configurations. At the same time, these quirks still hold true for the SoC even
>> if not exercised.
>>
>> So I think we should keep these in the dtsi itself.
> 
> Please take a closer look at the quirks you're enabling first. Commit
> 729dcffd1ed3 ("usb: dwc3: gadget: Add support for disabling U1 and U2
> entries") which added 
> 
>>>>> +				snps,dis-u1-entry-quirk;
>>>>> +				snps,dis-u2-entry-quirk;
> 
> explicitly mentions
> 
> 	Gadget applications may have a requirement to disable the U1 and U2
> 	entry based on the usecase.
> 
> which sounds like something that needs to be done in a per board dts at
> least.
> 
> Perhaps keeping all of these in in the dtsi is correct, but that's going
> to need some more motivation than simply that some vendor does so (as
> they often do all sorts of things they should not).
I'm looking at the DWC3 code and admittedly I don't understand much,
but is there any harm to keeping them? What if somebody decides to
plug in a laptop as a gadget device?

Konrad

> 
> Johan

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Johan Hovold]

On Wed, Mar 29, 2023 at 01:24:27PM +0200, Konrad Dybcio wrote:
> On 29.03.2023 10:34, Johan Hovold wrote:

> > Perhaps keeping all of these in in the dtsi is correct, but that's going
> > to need some more motivation than simply that some vendor does so (as
> > they often do all sorts of things they should not).

> I'm looking at the DWC3 code and admittedly I don't understand much,
> but is there any harm to keeping them? What if somebody decides to
> plug in a laptop as a gadget device?

We should the add the bits that are really needed with a proper
descriptions of what they do (like all commit messages should).

Besides the commit message, the problem here is that these have just
been copied from some vendor kernel and some properties are conflicting
(e.g. both disabling LPM and configuring LPM settings) while others
appear to be application specific.

Johan

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Manivannan Sadhasivam]

On Wed, Mar 29, 2023 at 10:34:56AM +0200, Johan Hovold wrote:
> On Wed, Mar 29, 2023 at 10:56:00AM +0530, Manivannan Sadhasivam wrote:
> > On Tue, Mar 28, 2023 at 03:09:03PM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
> > > > On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:
> > > > > Add missing quirks for the USB DWC3 IP.
> > > > 
> > > > This is not an acceptable commit message generally and certainly not for
> > > > something that you have tagged for stable.
> > > > 
> > > > At a minimum, you need to describe why these are needed and what the
> > > > impact is.
> > > > 
> > > 
> > > I can certainly improve the commit message. But usually the quirks are copied
> > > from the downstream devicetree where qualcomm engineers would've added them
> > > based on the platform requirements.
> > > 
> > > > Also, why are you sending as part of a series purporting to enable
> > > > runtime PM when it appears to be all about optimising specific gadget
> > > > applications?
> > > > 
> > > 
> > > It's not related to this series I agree but just wanted to group it with a
> > > series touching usb so that it won't get lost.
> > > 
> > > I could respin it separately though in v2.
> 
> That's also generally best for USB patches as Greg expects series to be
> merged through a single tree.
> 

Ok, good to know.

> > > > Did you confirm that the below makes any sense or has this just been
> > > > copied verbatim from the vendor devicetree (it looks like that)?
> > > > 
> > > 
> > > As you've mentioned, most of the quirks are for gadget mode which is not
> > > supported by the upstream supported boards. So I haven't really tested them but
> > > for I assumed that Qcom engineers did.
> > > 
> > > > The fact that almost none of the qcom SoCs sets these also indicates
> > > > that something is not right here.
> > > > 
> > > > > Cc: stable@vger.kernel.org # 5.20
> > > > > Fixes: 152d1faf1e2f ("arm64: dts: qcom: add SC8280XP platform")
> > > > > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > > > > ---
> > > > >  arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 14 ++++++++++++++
> > > > >  1 file changed, 14 insertions(+)
> > > > > 
> > > > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > > index 0d02599d8867..266a94c712aa 100644
> > > > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > > @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
> > > > >  				iommus = <&apps_smmu 0x820 0x0>;
> > > > >  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
> > > > >  				phy-names = "usb2-phy", "usb3-phy";
> > > > > +				snps,hird-threshold = /bits/ 8 <0x0>;
> > > > > +				snps,usb2-gadget-lpm-disable;
> > > > 
> > > > Here you are disabling LPM for gadget mode, which makes most of the
> > > > other properties entirely pointless.
> > 
> > Checked with Qcom on these quirks. So this one is just disabling lpm for USB2
> > and rest of the quirks below are for SS/SSP modes.
> 
> No, snps,hird-threshold is for USB2 LPM and so is
> snps,is-utmi-l1-suspend and snps,has-lpm-erratum as you'll see if you
> look at the implementation.
> 

Correct me if I'm wrong. When I look into the code, "snps,is-utmi-l1-suspend"
and "snps,hird-threshold" are used independently of the LPM mode atleast in one
place:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc3/gadget.c#n2867

But I could be completely wrong here as my understanding of the usb stack is not
that great.

> > > > > +				snps,is-utmi-l1-suspend;
> > > > > +				snps,dis-u1-entry-quirk;
> > > > > +				snps,dis-u2-entry-quirk;
> > > > 
> > > > These appear to be used to optimise certain gadget application and
> > > > likely not something that should be set in a dtsi.
> > > > 
> > > 
> > > I will cross check these with Qcom and respin accordingly.
> > > 
> > 
> > These quirks are needed as per the DWC IP integration with this SoC it seems.
> > But I got the point that these don't add any values for host only
> > configurations. At the same time, these quirks still hold true for the SoC even
> > if not exercised.
> > 
> > So I think we should keep these in the dtsi itself.
> 
> Please take a closer look at the quirks you're enabling first. Commit
> 729dcffd1ed3 ("usb: dwc3: gadget: Add support for disabling U1 and U2
> entries") which added 
> 
> > > > > +				snps,dis-u1-entry-quirk;
> > > > > +				snps,dis-u2-entry-quirk;
> 
> explicitly mentions
> 
> 	Gadget applications may have a requirement to disable the U1 and U2
> 	entry based on the usecase.
> 
> which sounds like something that needs to be done in a per board dts at
> least.
> 

Going by this commit message it sounds like it. But...

> Perhaps keeping all of these in in the dtsi is correct, but that's going
> to need some more motivation than simply that some vendor does so (as
> they often do all sorts of things they should not).
> 

If you read my last reply one more time, I didn't reason it based on the vendor
code.

But I hear a contradict reply from Qcom saying that these properties are
required as a part of the DWC3 IP integration with the SoC. I need to recheck
with them again tomorrow.

Also, if these properties are application specific then they shouldn't be in
devicetree atleast :/

- Mani

- Mani

> Johan

-- 
மணிவண்ணன் சதாசிவம்

Re: [PATCH 1/5] arm64: dts: qcom: sc8280xp: Add missing dwc3 quirks

[Johan Hovold]

On Wed, Mar 29, 2023 at 06:53:43PM +0530, Manivannan Sadhasivam wrote:
> On Wed, Mar 29, 2023 at 10:34:56AM +0200, Johan Hovold wrote:
> > On Wed, Mar 29, 2023 at 10:56:00AM +0530, Manivannan Sadhasivam wrote:
> > > On Tue, Mar 28, 2023 at 03:09:03PM +0530, Manivannan Sadhasivam wrote:
> > > > On Tue, Mar 28, 2023 at 10:54:53AM +0200, Johan Hovold wrote:
> > > > > On Sat, Mar 25, 2023 at 10:22:13PM +0530, Manivannan Sadhasivam wrote:

> > > > > > diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > > > index 0d02599d8867..266a94c712aa 100644
> > > > > > --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > > > +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> > > > > > @@ -3040,6 +3040,13 @@ usb_0_dwc3: usb@a600000 {
> > > > > >  				iommus = <&apps_smmu 0x820 0x0>;
> > > > > >  				phys = <&usb_0_hsphy>, <&usb_0_qmpphy QMP_USB43DP_USB3_PHY>;
> > > > > >  				phy-names = "usb2-phy", "usb3-phy";
> > > > > > +				snps,hird-threshold = /bits/ 8 <0x0>;
> > > > > > +				snps,usb2-gadget-lpm-disable;
> > > > > 
> > > > > Here you are disabling LPM for gadget mode, which makes most of the
> > > > > other properties entirely pointless.
> > > 
> > > Checked with Qcom on these quirks. So this one is just disabling lpm for USB2
> > > and rest of the quirks below are for SS/SSP modes.
> > 
> > No, snps,hird-threshold is for USB2 LPM and so is
> > snps,is-utmi-l1-suspend and snps,has-lpm-erratum as you'll see if you
> > look at the implementation.
> > 
> 
> Correct me if I'm wrong. When I look into the code, "snps,is-utmi-l1-suspend"
> and "snps,hird-threshold" are used independently of the LPM mode atleast in one
> place:
> 
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/usb/dwc3/gadget.c#n2867
> 
> But I could be completely wrong here as my understanding of the usb stack is not
> that great.

Yeah, it's not that obvious from just looking at the code, but L1 (and
BESL) are USB2 LPM concepts and if you disable LPM then there is no need
to override these values in the BOS descriptor either (as is done in
dwc3_gadget_config_params() and bos_desc()).

> > > > > > +				snps,is-utmi-l1-suspend;
> > > > > > +				snps,dis-u1-entry-quirk;
> > > > > > +				snps,dis-u2-entry-quirk;
> > > > > 
> > > > > These appear to be used to optimise certain gadget application and
> > > > > likely not something that should be set in a dtsi.
> > > > > 
> > > > 
> > > > I will cross check these with Qcom and respin accordingly.
> > > > 
> > > 
> > > These quirks are needed as per the DWC IP integration with this SoC it seems.
> > > But I got the point that these don't add any values for host only
> > > configurations. At the same time, these quirks still hold true for the SoC even
> > > if not exercised.
> > > 
> > > So I think we should keep these in the dtsi itself.
> > 
> > Please take a closer look at the quirks you're enabling first. Commit
> > 729dcffd1ed3 ("usb: dwc3: gadget: Add support for disabling U1 and U2
> > entries") which added 
> > 
> > > > > > +				snps,dis-u1-entry-quirk;
> > > > > > +				snps,dis-u2-entry-quirk;
> > 
> > explicitly mentions
> > 
> > 	Gadget applications may have a requirement to disable the U1 and U2
> > 	entry based on the usecase.
> > 
> > which sounds like something that needs to be done in a per board dts at
> > least.
> > 
> 
> Going by this commit message it sounds like it. But...
> 
> > Perhaps keeping all of these in in the dtsi is correct, but that's going
> > to need some more motivation than simply that some vendor does so (as
> > they often do all sorts of things they should not).
> > 
> 
> If you read my last reply one more time, I didn't reason it based on the vendor
> code.

I was referring to the fact that these properties had been copied from
the vendor dtsi and seemingly so without further review or
justification in the commit message (e.g. to explain the
inconsistencies).

> But I hear a contradict reply from Qcom saying that these properties are
> required as a part of the DWC3 IP integration with the SoC. I need to recheck
> with them again tomorrow.
> 
> Also, if these properties are application specific then they shouldn't be in
> devicetree atleast :/

I fully agree with that.

Johan