From: Stefan Eichenberger <[email protected]>
Currently we have the following two call chains:
dwc3_probe -> dwc3_core_init -> dwc3_phy_init -> usb_phy_init
dwc3_probe -> dwc3_core_init -> dwc3_phy_power_on -> usb_phy_set_suspend
If we look at phy-generic we see the following calls:
usb_gen_phy_init -> regulator_enable
usb_gen_phy_init -> clk_prepare_enable
If we call usb_phy_set_suspend we call the following in phy-generic:
nop_set_suspend -> clk_prepare_enable
and we sent a patch to also call:
nop_set_suspend -> regulator_enable
Because clk_prepare_enable and regulator_enable do reference counting we
increased the reference counter of the clock and regulator to two. If we
want to put the system into suspend we only decrease the reference
counters by one and therefore the clock and regulator stay on.
This change fixes it by not calling usb_phy_set_suspend in
dwc3_phy_power_on but only in dwc3_suspend_common.
Fixes: 8ba007a971bb ("usb: dwc3: core: enable the USB2 and USB3 phy in probe")
Signed-off-by: Stefan Eichenberger <[email protected]>
Signed-off-by: Francesco Dolcini <[email protected]>
---
drivers/usb/dwc3/core.c | 17 +++++++----------
1 file changed, 7 insertions(+), 10 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9c6bf054f15d..fae24a9c480d 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -770,12 +770,9 @@ static int dwc3_phy_power_on(struct dwc3 *dwc)
{
int ret;
- usb_phy_set_suspend(dwc->usb2_phy, 0);
- usb_phy_set_suspend(dwc->usb3_phy, 0);
-
ret = phy_power_on(dwc->usb2_generic_phy);
if (ret < 0)
- goto err_suspend_usb3_phy;
+ return ret;
ret = phy_power_on(dwc->usb3_generic_phy);
if (ret < 0)
@@ -785,9 +782,6 @@ static int dwc3_phy_power_on(struct dwc3 *dwc)
err_power_off_usb2_phy:
phy_power_off(dwc->usb2_generic_phy);
-err_suspend_usb3_phy:
- usb_phy_set_suspend(dwc->usb3_phy, 1);
- usb_phy_set_suspend(dwc->usb2_phy, 1);
return ret;
}
@@ -796,9 +790,6 @@ static void dwc3_phy_power_off(struct dwc3 *dwc)
{
phy_power_off(dwc->usb3_generic_phy);
phy_power_off(dwc->usb2_generic_phy);
-
- usb_phy_set_suspend(dwc->usb3_phy, 1);
- usb_phy_set_suspend(dwc->usb2_phy, 1);
}
static int dwc3_clk_enable(struct dwc3 *dwc)
@@ -2018,6 +2009,9 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
break;
}
+ usb_phy_set_suspend(dwc->usb2_phy, 1);
+ usb_phy_set_suspend(dwc->usb3_phy, 1);
+
return 0;
}
@@ -2027,6 +2021,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
int ret;
u32 reg;
+ usb_phy_set_suspend(dwc->usb2_phy, 0);
+ usb_phy_set_suspend(dwc->usb3_phy, 0);
+
switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
ret = dwc3_core_init_for_resume(dwc);
--
2.25.1
On Fri, Nov 03, 2023 at 11:22:36AM +0100, Francesco Dolcini wrote:
> From: Stefan Eichenberger <[email protected]>
>
> Currently we have the following two call chains:
> dwc3_probe -> dwc3_core_init -> dwc3_phy_init -> usb_phy_init
> dwc3_probe -> dwc3_core_init -> dwc3_phy_power_on -> usb_phy_set_suspend
>
> If we look at phy-generic we see the following calls:
> usb_gen_phy_init -> regulator_enable
> usb_gen_phy_init -> clk_prepare_enable
>
> If we call usb_phy_set_suspend we call the following in phy-generic:
> nop_set_suspend -> clk_prepare_enable
> and we sent a patch to also call:
> nop_set_suspend -> regulator_enable
>
> Because clk_prepare_enable and regulator_enable do reference counting we
> increased the reference counter of the clock and regulator to two. If we
> want to put the system into suspend we only decrease the reference
> counters by one and therefore the clock and regulator stay on.
>
> This change fixes it by not calling usb_phy_set_suspend in
> dwc3_phy_power_on but only in dwc3_suspend_common.
>
> Fixes: 8ba007a971bb ("usb: dwc3: core: enable the USB2 and USB3 phy in probe")
> Signed-off-by: Stefan Eichenberger <[email protected]>
> Signed-off-by: Francesco Dolcini <[email protected]>
> ---
> drivers/usb/dwc3/core.c | 17 +++++++----------
> 1 file changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 9c6bf054f15d..fae24a9c480d 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -770,12 +770,9 @@ static int dwc3_phy_power_on(struct dwc3 *dwc)
> {
> int ret;
>
> - usb_phy_set_suspend(dwc->usb2_phy, 0);
> - usb_phy_set_suspend(dwc->usb3_phy, 0);
> -
> ret = phy_power_on(dwc->usb2_generic_phy);
> if (ret < 0)
> - goto err_suspend_usb3_phy;
> + return ret;
>
> ret = phy_power_on(dwc->usb3_generic_phy);
> if (ret < 0)
> @@ -785,9 +782,6 @@ static int dwc3_phy_power_on(struct dwc3 *dwc)
>
> err_power_off_usb2_phy:
> phy_power_off(dwc->usb2_generic_phy);
> -err_suspend_usb3_phy:
> - usb_phy_set_suspend(dwc->usb3_phy, 1);
> - usb_phy_set_suspend(dwc->usb2_phy, 1);
>
> return ret;
> }
> @@ -796,9 +790,6 @@ static void dwc3_phy_power_off(struct dwc3 *dwc)
> {
> phy_power_off(dwc->usb3_generic_phy);
> phy_power_off(dwc->usb2_generic_phy);
> -
> - usb_phy_set_suspend(dwc->usb3_phy, 1);
> - usb_phy_set_suspend(dwc->usb2_phy, 1);
> }
>
> static int dwc3_clk_enable(struct dwc3 *dwc)
> @@ -2018,6 +2009,9 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> break;
> }
>
> + usb_phy_set_suspend(dwc->usb2_phy, 1);
> + usb_phy_set_suspend(dwc->usb3_phy, 1);
> +
> return 0;
> }
>
> @@ -2027,6 +2021,9 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
> int ret;
> u32 reg;
>
> + usb_phy_set_suspend(dwc->usb2_phy, 0);
> + usb_phy_set_suspend(dwc->usb3_phy, 0);
> +
> switch (dwc->current_dr_role) {
> case DWC3_GCTL_PRTCAP_DEVICE:
> ret = dwc3_core_init_for_resume(dwc);
> --
> 2.25.1
>
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- You have marked a patch with a "Fixes:" tag for a commit that is in an
older released kernel, yet you do not have a cc: stable line in the
signed-off-by area at all, which means that the patch will not be
applied to any older kernel releases. To properly fix this, please
follow the documented rules in the
Documentation/process/stable-kernel-rules.rst file for how to resolve
this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
On Fri, Nov 03, 2023 at 11:22:36AM +0100, Francesco Dolcini wrote:
> From: Stefan Eichenberger <[email protected]>
>
> Currently we have the following two call chains:
> dwc3_probe -> dwc3_core_init -> dwc3_phy_init -> usb_phy_init
> dwc3_probe -> dwc3_core_init -> dwc3_phy_power_on -> usb_phy_set_suspend
>
> If we look at phy-generic we see the following calls:
> usb_gen_phy_init -> regulator_enable
> usb_gen_phy_init -> clk_prepare_enable
>
> If we call usb_phy_set_suspend we call the following in phy-generic:
> nop_set_suspend -> clk_prepare_enable
> and we sent a patch to also call:
> nop_set_suspend -> regulator_enable
>
> Because clk_prepare_enable and regulator_enable do reference counting we
> increased the reference counter of the clock and regulator to two. If we
> want to put the system into suspend we only decrease the reference
> counters by one and therefore the clock and regulator stay on.
No, this does not seem to be a correct description of the current
implementation.
The driver always calls both usb_phy_set_suspend() and
usb_phy_init()/usb_phy_shutdown() so those usage counters would still be
balanced (e.g. see dwc3_core_init() and dwc3_core_exit()).
> This change fixes it by not calling usb_phy_set_suspend in
> dwc3_phy_power_on but only in dwc3_suspend_common.
> static int dwc3_clk_enable(struct dwc3 *dwc)
> @@ -2018,6 +2009,9 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
> break;
> }
>
> + usb_phy_set_suspend(dwc->usb2_phy, 1);
> + usb_phy_set_suspend(dwc->usb3_phy, 1);
This is also broken as you're now calling usb_phy_set_suspend() in paths
that do not expect it as well as after usb_phy_shutdown() in case
dwc3_core_exit() was called above.
The suspend implementation in this driver is indeed messy and probably
not tested much. It seems the expectation for the legacy PHY
implementation is to only call init()/shutdown() at probe/remove and
then use set_suspend() to handle the suspend state. The dwc3 driver is
for some reason calling both set_suspend() and shutdown() which should
not be needed. Care needs to be taken so that no one has started relying
on this behaviour if you want to change this.
When reviewing the driver I did find a bug in the xhci-plat driver which
is likely the cause for the imbalance you're seeing. I just sent a fix
here in case you want to give it a try:
https://lore.kernel.org/lkml/[email protected]/
But, also, why are you using legacy PHYs? Which platform is this for?
Johan
Hi Johan,
> From: Johan Hovold <[email protected]>
>
> On Fri, Nov 03, 2023 at 11:22:36AM +0100, Francesco Dolcini wrote:
> > From: Stefan Eichenberger <[email protected]>
> >
> > Currently we have the following two call chains:
> > dwc3_probe -> dwc3_core_init -> dwc3_phy_init -> usb_phy_init
> > dwc3_probe -> dwc3_core_init -> dwc3_phy_power_on -> usb_phy_set_suspend
> >
> > If we look at phy-generic we see the following calls:
> > usb_gen_phy_init -> regulator_enable
> > usb_gen_phy_init -> clk_prepare_enable
> >
> > If we call usb_phy_set_suspend we call the following in phy-generic:
> > nop_set_suspend -> clk_prepare_enable
> > and we sent a patch to also call:
> > nop_set_suspend -> regulator_enable
> >
> > Because clk_prepare_enable and regulator_enable do reference counting we
> > increased the reference counter of the clock and regulator to two. If we
> > want to put the system into suspend we only decrease the reference
> > counters by one and therefore the clock and regulator stay on.
>
> No, this does not seem to be a correct description of the current
> implementation.
>
> The driver always calls both usb_phy_set_suspend() and
> usb_phy_init()/usb_phy_shutdown() so those usage counters would still be
> balanced (e.g. see dwc3_core_init() and dwc3_core_exit()).
>
You are right I missunderstood that part.
> When reviewing the driver I did find a bug in the xhci-plat driver which
> is likely the cause for the imbalance you're seeing. I just sent a fix
> here in case you want to give it a try:
>
> https://lore.kernel.org/lkml/[email protected]/
I tested it and it solves the issue we have. Thanks a lot for the fix!
Before the use count for our regulator always went up to 2 and now it is
1 as expected.
root@verdin-imx8mp-14773241:~# cat /sys/kernel/debug/regulator/CTRL_SLEEP_MOCI#/use_count
1
Also when going to suspend the regulator is turned off now. With the
suspend patch applied from us the use count will be one more but
everything still works as expected.
>
> But, also, why are you using legacy PHYs? Which platform is this for?
We have an external hub that we want to turn off when the system goes
into suspend. For the i.MX8MM we use the phy-generic driver to achieve
this. When I saw that the dwc3 driver would support the phy-generic via
usb-phy, I thought we could use the same approach for the i.MX8MP and,
in the future, the AM62. Maybe I misunderstood, would the right solution
be to add a suspend function to the fsl,imx8mp-usb-phy driver and use
vbus instead? But what would we do for the AM62, as it doesn't have a
phy driver if I'm not mistaken.
Regards,
Stefan
On Sat, Nov 04, 2023 at 11:51:22AM +0000, Stefan Eichenberger wrote:
> > From: Johan Hovold <[email protected]>
> > When reviewing the driver I did find a bug in the xhci-plat driver which
> > is likely the cause for the imbalance you're seeing. I just sent a fix
> > here in case you want to give it a try:
> >
> > https://lore.kernel.org/lkml/[email protected]/
>
> I tested it and it solves the issue we have. Thanks a lot for the fix!
> Before the use count for our regulator always went up to 2 and now it is
> 1 as expected.
> root@verdin-imx8mp-14773241:~# cat /sys/kernel/debug/regulator/CTRL_SLEEP_MOCI#/use_count
> 1
>
> Also when going to suspend the regulator is turned off now. With the
> suspend patch applied from us the use count will be one more but
> everything still works as expected.
Thanks for testing.
> > But, also, why are you using legacy PHYs? Which platform is this for?
>
> We have an external hub that we want to turn off when the system goes
> into suspend. For the i.MX8MM we use the phy-generic driver to achieve
> this. When I saw that the dwc3 driver would support the phy-generic via
> usb-phy, I thought we could use the same approach for the i.MX8MP and,
> in the future, the AM62. Maybe I misunderstood, would the right solution
> be to add a suspend function to the fsl,imx8mp-usb-phy driver and use
> vbus instead? But what would we do for the AM62, as it doesn't have a
> phy driver if I'm not mistaken.
That's not how the phy driver is supposed be used, and for on-board hubs
we now have:
drivers/usb/misc/onboard_usb_hub.c
Have you tried using that one instead?
Johan