Linux-Amlogic Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
@ 2020-07-13 16:05 Dan Robertson
  2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson
  2020-07-14  6:56 ` [PATCH 0/1] " Anand Moon
  0 siblings, 2 replies; 14+ messages in thread
From: Dan Robertson @ 2020-07-13 16:05 UTC (permalink / raw)
  To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman
  Cc: linux-amlogic, linux-usb, Dan Robertson, linux-arm-kernel

When testing suspend for another driver I noticed the following warning:

WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
Hardware name: Hardkernel ODROID-N2 (DT)
[..]
pc : reset_control_assert+0x184/0x19c
lr : dwc3_meson_g12a_suspend+0x68/0x7c
[..]
Call trace:
 reset_control_assert+0x184/0x19c
 dwc3_meson_g12a_suspend+0x68/0x7c
 platform_pm_suspend+0x28/0x54
 __device_suspend+0x590/0xabc
 dpm_suspend+0x104/0x404
 dpm_suspend_start+0x84/0x1bc
 suspend_devices_and_enter+0xc4/0x4fc

In my limited experience and knowlege it appears that we hit this
because the reset control was switched to shared and the the use
of the reset control was not changed.

> * Calling reset_control_assert without first calling reset_control_deassert
> * is not allowed on a shared reset control. Calling reset_control_reset is
> * also not allowed on a shared reset control.

The above snippet from reset_control_get_shared() seems to indicate that
this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
and reset_control_deassert is not guaranteed to have been called
before dwc3_meson_g12a_suspend() and reset_control_assert().

After some basic tests with the following patch I no longer hit the
warning. Comments and critiques on the patch are welcome. If there is a
reason for the current use of the reset control, I'd love to learn why!
Like I said before, I have not really looked at this driver before and
have verify limited experience with reset controls... Was working on
another driver, hit the warning, and thought I'd take a shot at the
fix :-)

Cheers,

 - Dan

Dan Robertson (1):
  usb: dwc3: meson-g12a: fix shared reset control use

 drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)



_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson
@ 2020-07-13 16:05 ` Dan Robertson
  2020-07-18  8:47   ` Neil Armstrong
  2020-07-14  6:56 ` [PATCH 0/1] " Anand Moon
  1 sibling, 1 reply; 14+ messages in thread
From: Dan Robertson @ 2020-07-13 16:05 UTC (permalink / raw)
  To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman
  Cc: linux-amlogic, linux-usb, Dan Robertson, linux-arm-kernel

The reset is a shared reset line, but reset_control_reset is still used
and reset_control_deassert is not guaranteed to have been called before
the first reset_control_assert call. When suspending the following
warning may be seen:

WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
Hardware name: Hardkernel ODROID-N2 (DT)
[..]
pc : reset_control_assert+0x184/0x19c
lr : dwc3_meson_g12a_suspend+0x68/0x7c
[..]
Call trace:
 reset_control_assert+0x184/0x19c
 dwc3_meson_g12a_suspend+0x68/0x7c
 platform_pm_suspend+0x28/0x54
 __device_suspend+0x590/0xabc
 dpm_suspend+0x104/0x404
 dpm_suspend_start+0x84/0x1bc
 suspend_devices_and_enter+0xc4/0x4fc
 pm_suspend+0x198/0x2d4

Fixes: 6d9fa35a347a87 ("usb: dwc3: meson-g12a: get the reset as shared")
Signed-off-by: Dan Robertson <dan@dlrobertson.com>
---
 drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index 1f7f4d88ed9d..88b75b5a039c 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -737,13 +737,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
 		goto err_disable_clks;
 	}
 
-	ret = reset_control_reset(priv->reset);
+	ret = reset_control_deassert(priv->reset);
 	if (ret)
-		goto err_disable_clks;
+		goto err_assert_reset;
 
 	ret = dwc3_meson_g12a_get_phys(priv);
 	if (ret)
-		goto err_disable_clks;
+		goto err_assert_reset;
 
 	ret = priv->drvdata->setup_regmaps(priv, base);
 	if (ret)
@@ -752,7 +752,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
 	if (priv->vbus) {
 		ret = regulator_enable(priv->vbus);
 		if (ret)
-			goto err_disable_clks;
+			goto err_assert_reset;
 	}
 
 	/* Get dr_mode */
@@ -765,13 +765,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
 
 	ret = priv->drvdata->usb_init(priv);
 	if (ret)
-		goto err_disable_clks;
+		goto err_assert_reset;
 
 	/* Init PHYs */
 	for (i = 0 ; i < PHY_COUNT ; ++i) {
 		ret = phy_init(priv->phys[i]);
 		if (ret)
-			goto err_disable_clks;
+			goto err_assert_reset;
 	}
 
 	/* Set PHY Power */
@@ -809,6 +809,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
 	for (i = 0 ; i < PHY_COUNT ; ++i)
 		phy_exit(priv->phys[i]);
 
+err_assert_reset:
+	reset_control_assert(priv->reset);
+
 err_disable_clks:
 	clk_bulk_disable_unprepare(priv->drvdata->num_clks,
 				   priv->drvdata->clks);


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson
  2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson
@ 2020-07-14  6:56 ` Anand Moon
  2020-07-14 13:30   ` Dan Robertson
  1 sibling, 1 reply; 14+ messages in thread
From: Anand Moon @ 2020-07-14  6:56 UTC (permalink / raw)
  To: Dan Robertson
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel


[-- Attachment #1: Type: text/plain, Size: 2093 bytes --]

hi Dan,

On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
>
> When testing suspend for another driver I noticed the following warning:
>
> WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> Hardware name: Hardkernel ODROID-N2 (DT)
> [..]
> pc : reset_control_assert+0x184/0x19c
> lr : dwc3_meson_g12a_suspend+0x68/0x7c
> [..]
> Call trace:
>  reset_control_assert+0x184/0x19c
>  dwc3_meson_g12a_suspend+0x68/0x7c
>  platform_pm_suspend+0x28/0x54
>  __device_suspend+0x590/0xabc
>  dpm_suspend+0x104/0x404
>  dpm_suspend_start+0x84/0x1bc
>  suspend_devices_and_enter+0xc4/0x4fc
>
> In my limited experience and knowlege it appears that we hit this
> because the reset control was switched to shared and the the use
> of the reset control was not changed.
>
> > * Calling reset_control_assert without first calling reset_control_deassert
> > * is not allowed on a shared reset control. Calling reset_control_reset is
> > * also not allowed on a shared reset control.
>
> The above snippet from reset_control_get_shared() seems to indicate that
> this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> and reset_control_deassert is not guaranteed to have been called
> before dwc3_meson_g12a_suspend() and reset_control_assert().
>
> After some basic tests with the following patch I no longer hit the
> warning. Comments and critiques on the patch are welcome. If there is a
> reason for the current use of the reset control, I'd love to learn why!
> Like I said before, I have not really looked at this driver before and
> have verify limited experience with reset controls... Was working on
> another driver, hit the warning, and thought I'd take a shot at the
> fix :-)
>
Thanks, I was also looking into this issue
So best Fix to this issue is to drop the call of reset_control_assert
from dwc3_meson_g12a_suspend
and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
With these changes we do not see the warning on suspend and resume

Can you try this attached patch?

Best Regards
-Anand

[-- Attachment #2: 0001-usb-dwc3-meson-g12a-drop-reset-controller-call-from-.patch --]
[-- Type: application/octet-stream, Size: 1097 bytes --]

[-- Attachment #3: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-14  6:56 ` [PATCH 0/1] " Anand Moon
@ 2020-07-14 13:30   ` Dan Robertson
  2020-07-14 15:27     ` Anand Moon
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Robertson @ 2020-07-14 13:30 UTC (permalink / raw)
  To: Anand Moon
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

[-- Attachment #1.1: Type: text/plain, Size: 3277 bytes --]

Hi Anand!

On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:
> hi Dan,
> 
> On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
> >
> > When testing suspend for another driver I noticed the following warning:
> >
> > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > Hardware name: Hardkernel ODROID-N2 (DT)
> > [..]
> > pc : reset_control_assert+0x184/0x19c
> > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > [..]
> > Call trace:
> >  reset_control_assert+0x184/0x19c
> >  dwc3_meson_g12a_suspend+0x68/0x7c
> >  platform_pm_suspend+0x28/0x54
> >  __device_suspend+0x590/0xabc
> >  dpm_suspend+0x104/0x404
> >  dpm_suspend_start+0x84/0x1bc
> >  suspend_devices_and_enter+0xc4/0x4fc
> >
> > In my limited experience and knowlege it appears that we hit this
> > because the reset control was switched to shared and the the use
> > of the reset control was not changed.
> >
> > > * Calling reset_control_assert without first calling reset_control_deassert
> > > * is not allowed on a shared reset control. Calling reset_control_reset is
> > > * also not allowed on a shared reset control.
> >
> > The above snippet from reset_control_get_shared() seems to indicate that
> > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> > and reset_control_deassert is not guaranteed to have been called
> > before dwc3_meson_g12a_suspend() and reset_control_assert().
> >
> > After some basic tests with the following patch I no longer hit the
> > warning. Comments and critiques on the patch are welcome. If there is a
> > reason for the current use of the reset control, I'd love to learn why!
> > Like I said before, I have not really looked at this driver before and
> > have verify limited experience with reset controls... Was working on
> > another driver, hit the warning, and thought I'd take a shot at the
> > fix :-)
> >
> Thanks, I was also looking into this issue

Awesome!

> So best Fix to this issue is to drop the call of reset_control_assert
> from dwc3_meson_g12a_suspend
> and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
> With these changes we do not see the warning on suspend and resume

We definitely would avoid hitting the warning without the
reset_control_(de)assert calls in suspend and resume. That is a valid use of
the reset control, but why would that be best?

From reset_control_reset():

> * Consumers must not use reset_control_(de)assert on shared reset lines when
> * reset_control_reset has been used.

If we use reset_control_reset() then we can not (de)assert the reset line
on suspend/resume or any other time. Wouldn't we want to be able to
(de)assert the reset line? And why do we no longer want to (de)assert the
reset line on suspend/resume?

> > Can you try this attached patch?
> 
> Best Regards
> -Anand

I think I had already tested something similar. Removing the (de)assert calls
but keeping reset will definitely remove the warning, but it means we can not
(de)assert the line. My guess is that this is not what we want, but I could be
wrong. Thoughts, input, or references to documentation on this would be
appreciated.

Cheers,

 - Dan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-14 13:30   ` Dan Robertson
@ 2020-07-14 15:27     ` Anand Moon
  2020-07-15  2:58       ` Dan Robertson
  0 siblings, 1 reply; 14+ messages in thread
From: Anand Moon @ 2020-07-14 15:27 UTC (permalink / raw)
  To: Dan Robertson
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

Hi Dan,

On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote:
>
> Hi Anand!
>
> On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:
> > hi Dan,
> >
> > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
> > >
> > > When testing suspend for another driver I noticed the following warning:
> > >
> > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > > Hardware name: Hardkernel ODROID-N2 (DT)
> > > [..]
> > > pc : reset_control_assert+0x184/0x19c
> > > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > > [..]
> > > Call trace:
> > >  reset_control_assert+0x184/0x19c
> > >  dwc3_meson_g12a_suspend+0x68/0x7c
> > >  platform_pm_suspend+0x28/0x54
> > >  __device_suspend+0x590/0xabc
> > >  dpm_suspend+0x104/0x404
> > >  dpm_suspend_start+0x84/0x1bc
> > >  suspend_devices_and_enter+0xc4/0x4fc
> > >
> > > In my limited experience and knowlege it appears that we hit this
> > > because the reset control was switched to shared and the the use
> > > of the reset control was not changed.
> > >
> > > > * Calling reset_control_assert without first calling reset_control_deassert
> > > > * is not allowed on a shared reset control. Calling reset_control_reset is
> > > > * also not allowed on a shared reset control.
> > >
> > > The above snippet from reset_control_get_shared() seems to indicate that
> > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> > > and reset_control_deassert is not guaranteed to have been called
> > > before dwc3_meson_g12a_suspend() and reset_control_assert().
> > >
> > > After some basic tests with the following patch I no longer hit the
> > > warning. Comments and critiques on the patch are welcome. If there is a
> > > reason for the current use of the reset control, I'd love to learn why!
> > > Like I said before, I have not really looked at this driver before and
> > > have verify limited experience with reset controls... Was working on
> > > another driver, hit the warning, and thought I'd take a shot at the
> > > fix :-)
> > >
> > Thanks, I was also looking into this issue
>
> Awesome!
>
> > So best Fix to this issue is to drop the call of reset_control_assert
> > from dwc3_meson_g12a_suspend
> > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
> > With these changes we do not see the warning on suspend and resume
>
> We definitely would avoid hitting the warning without the
> reset_control_(de)assert calls in suspend and resume. That is a valid use of
> the reset control, but why would that be best?
>
> From reset_control_reset():

Before entering the suspend state the code tries to do following
     clk_bulk_disable_unprepare
     regulator_disable
     phy_power_off
     phy_exit

After this operation it's needless to call *reset_control_assert*
I tried to move this call before all the above operations
but still no success with this.

Similarly on resume we should avoid calling resume
*reset_control_deassert* earlier
as the core is just reinitializing the devices.
      clk_bulk_prepare_enable
      usb_init
      phy_init
      phy_power_on
      regulator_enable
>
> > * Consumers must not use reset_control_(de)assert on shared reset lines when
> > * reset_control_reset has been used.
>
> If we use reset_control_reset() then we can not (de)assert the reset line
> on suspend/resume or any other time. Wouldn't we want to be able to
> (de)assert the reset line? And why do we no longer want to (de)assert the
> reset line on suspend/resume?
>
> > > Can you try this attached patch?
> >
> > Best Regards
> > -Anand
>
> I think I had already tested something similar. Removing the (de)assert calls
> but keeping reset will definitely remove the warning, but it means we can not
> (de)assert the line. My guess is that this is not what we want, but I could be
> wrong. Thoughts, input, or references to documentation on this would be
> appreciated.
>

As per my knowledge suspend to mem will do complete power down of the
devices with support suspend and resume feature sequentially and then it tries
to bring the device up one by one.
So it should also take care of reset lines as well.

> Cheers,
>
>  - Dan

Best Regards
-Anand

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-14 15:27     ` Anand Moon
@ 2020-07-15  2:58       ` Dan Robertson
  2020-07-15 16:23         ` Anand Moon
  0 siblings, 1 reply; 14+ messages in thread
From: Dan Robertson @ 2020-07-15  2:58 UTC (permalink / raw)
  To: Anand Moon
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

[-- Attachment #1.1: Type: text/plain, Size: 4925 bytes --]

On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote:
> Hi Dan,
> 
> On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote:
> >
> > Hi Anand!
> >
> > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:
> > > hi Dan,
> > >
> > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
> > > >
> > > > When testing suspend for another driver I noticed the following warning:
> > > >
> > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > > > Hardware name: Hardkernel ODROID-N2 (DT)
> > > > [..]
> > > > pc : reset_control_assert+0x184/0x19c
> > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > > > [..]
> > > > Call trace:
> > > >  reset_control_assert+0x184/0x19c
> > > >  dwc3_meson_g12a_suspend+0x68/0x7c
> > > >  platform_pm_suspend+0x28/0x54
> > > >  __device_suspend+0x590/0xabc
> > > >  dpm_suspend+0x104/0x404
> > > >  dpm_suspend_start+0x84/0x1bc
> > > >  suspend_devices_and_enter+0xc4/0x4fc
> > > >
> > > > In my limited experience and knowlege it appears that we hit this
> > > > because the reset control was switched to shared and the the use
> > > > of the reset control was not changed.
> > > >
> > > > > * Calling reset_control_assert without first calling reset_control_deassert
> > > > > * is not allowed on a shared reset control. Calling reset_control_reset is
> > > > > * also not allowed on a shared reset control.
> > > >
> > > > The above snippet from reset_control_get_shared() seems to indicate that
> > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> > > > and reset_control_deassert is not guaranteed to have been called
> > > > before dwc3_meson_g12a_suspend() and reset_control_assert().
> > > >
> > > > After some basic tests with the following patch I no longer hit the
> > > > warning. Comments and critiques on the patch are welcome. If there is a
> > > > reason for the current use of the reset control, I'd love to learn why!
> > > > Like I said before, I have not really looked at this driver before and
> > > > have verify limited experience with reset controls... Was working on
> > > > another driver, hit the warning, and thought I'd take a shot at the
> > > > fix :-)
> > > >
> > > Thanks, I was also looking into this issue
> >
> > Awesome!
> >
> > > So best Fix to this issue is to drop the call of reset_control_assert
> > > from dwc3_meson_g12a_suspend
> > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
> > > With these changes we do not see the warning on suspend and resume
> >
> > We definitely would avoid hitting the warning without the
> > reset_control_(de)assert calls in suspend and resume. That is a valid use of
> > the reset control, but why would that be best?
> >
> > From reset_control_reset():
> 
> Before entering the suspend state the code tries to do following
>      clk_bulk_disable_unprepare
>      regulator_disable
>      phy_power_off
>      phy_exit
> 
> After this operation it's needless to call *reset_control_assert*
> I tried to move this call before all the above operations
> but still no success with this.

How so? Once the reset() is removed prope() and deassert() is guaranteed
to have been called before suspend, like what is in the patch and similar
to other uses of shared reset controllers, this is possible.

> Similarly on resume we should avoid calling resume
> *reset_control_deassert* earlier
> as the core is just reinitializing the devices.
>       clk_bulk_prepare_enable
>       usb_init
>       phy_init
>       phy_power_on
>       regulator_enable
> >
> > > * Consumers must not use reset_control_(de)assert on shared reset lines when
> > > * reset_control_reset has been used.
> >
> > If we use reset_control_reset() then we can not (de)assert the reset line
> > on suspend/resume or any other time. Wouldn't we want to be able to
> > (de)assert the reset line? And why do we no longer want to (de)assert the
> > reset line on suspend/resume?
> >
> > > > Can you try this attached patch?
> > >
> > I think I had already tested something similar. Removing the (de)assert calls
> > but keeping reset will definitely remove the warning, but it means we can not
> > (de)assert the line. My guess is that this is not what we want, but I could be
> > wrong. Thoughts, input, or references to documentation on this would be
> > appreciated.
> >
> 
> As per my knowledge suspend to mem will do complete power down of the
> devices with support suspend and resume feature sequentially and then it tries
> to bring the device up one by one.
> So it should also take care of reset lines as well.

So do we only _actually_ care about the reset line in the probe? Seems like we
should remove the reset controller from the structure if that is the case.

Cheers,

 - Dan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 167 bytes --]

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-15  2:58       ` Dan Robertson
@ 2020-07-15 16:23         ` Anand Moon
  2020-07-17  9:01           ` Anand Moon
  0 siblings, 1 reply; 14+ messages in thread
From: Anand Moon @ 2020-07-15 16:23 UTC (permalink / raw)
  To: Dan Robertson
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

Hi Dan,

On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@dlrobertson.com> wrote:
>
> On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote:
> > Hi Dan,
> >
> > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote:
> > >
> > > Hi Anand!
> > >
> > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:
> > > > hi Dan,
> > > >
> > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
> > > > >
> > > > > When testing suspend for another driver I noticed the following warning:
> > > > >
> > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > > > > Hardware name: Hardkernel ODROID-N2 (DT)
> > > > > [..]
> > > > > pc : reset_control_assert+0x184/0x19c
> > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > > > > [..]
> > > > > Call trace:
> > > > >  reset_control_assert+0x184/0x19c
> > > > >  dwc3_meson_g12a_suspend+0x68/0x7c
> > > > >  platform_pm_suspend+0x28/0x54
> > > > >  __device_suspend+0x590/0xabc
> > > > >  dpm_suspend+0x104/0x404
> > > > >  dpm_suspend_start+0x84/0x1bc
> > > > >  suspend_devices_and_enter+0xc4/0x4fc
> > > > >
> > > > > In my limited experience and knowlege it appears that we hit this
> > > > > because the reset control was switched to shared and the the use
> > > > > of the reset control was not changed.
> > > > >
> > > > > > * Calling reset_control_assert without first calling reset_control_deassert
> > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is
> > > > > > * also not allowed on a shared reset control.
> > > > >
> > > > > The above snippet from reset_control_get_shared() seems to indicate that
> > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> > > > > and reset_control_deassert is not guaranteed to have been called
> > > > > before dwc3_meson_g12a_suspend() and reset_control_assert().
> > > > >
> > > > > After some basic tests with the following patch I no longer hit the
> > > > > warning. Comments and critiques on the patch are welcome. If there is a
> > > > > reason for the current use of the reset control, I'd love to learn why!
> > > > > Like I said before, I have not really looked at this driver before and
> > > > > have verify limited experience with reset controls... Was working on
> > > > > another driver, hit the warning, and thought I'd take a shot at the
> > > > > fix :-)
> > > > >
> > > > Thanks, I was also looking into this issue
> > >
> > > Awesome!
> > >
> > > > So best Fix to this issue is to drop the call of reset_control_assert
> > > > from dwc3_meson_g12a_suspend
> > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
> > > > With these changes we do not see the warning on suspend and resume
> > >
> > > We definitely would avoid hitting the warning without the
> > > reset_control_(de)assert calls in suspend and resume. That is a valid use of
> > > the reset control, but why would that be best?
> > >
> > > From reset_control_reset():
> >
> > Before entering the suspend state the code tries to do following
> >      clk_bulk_disable_unprepare
> >      regulator_disable
> >      phy_power_off
> >      phy_exit
> >
> > After this operation it's needless to call *reset_control_assert*
> > I tried to move this call before all the above operations
> > but still no success with this.
>
> How so? Once the reset() is removed prope() and deassert() is guaranteed
> to have been called before suspend, like what is in the patch and similar
> to other uses of shared reset controllers, this is possible.
>
> > Similarly on resume we should avoid calling resume
> > *reset_control_deassert* earlier
> > as the core is just reinitializing the devices.
> >       clk_bulk_prepare_enable
> >       usb_init
> >       phy_init
> >       phy_power_on
> >       regulator_enable
> > >
> > > > * Consumers must not use reset_control_(de)assert on shared reset lines when
> > > > * reset_control_reset has been used.
> > >
> > > If we use reset_control_reset() then we can not (de)assert the reset line
> > > on suspend/resume or any other time. Wouldn't we want to be able to
> > > (de)assert the reset line? And why do we no longer want to (de)assert the
> > > reset line on suspend/resume?
> > >
> > > > > Can you try this attached patch?
> > > >
> > > I think I had already tested something similar. Removing the (de)assert calls
> > > but keeping reset will definitely remove the warning, but it means we can not
> > > (de)assert the line. My guess is that this is not what we want, but I could be
> > > wrong. Thoughts, input, or references to documentation on this would be
> > > appreciated.
> > >
> >
> > As per my knowledge suspend to mem will do complete power down of the
> > devices with support suspend and resume feature sequentially and then it tries
> > to bring the device up one by one.
> > So it should also take care of reset lines as well.
>
> So do we only _actually_ care about the reset line in the probe? Seems like we
> should remove the reset controller from the structure if that is the case.
>
> Cheers,
>
>  - Dan

Sorry I am not the _expert_ in suspend/resume feature but I can debug this,
Certainly we need to look into whether reset controller calls are needed
to suspend or resume for this driver.

First thing we need to get the RTC driver to work on Odroid N2 for
suspend wakeup
to work properly.
For this I have created the following patches.

[0] https://patchwork.kernel.org/cover/11665865/

With these patches the RTC feature for wake up is broken right now in
my testing.
Once I get something to work further I will update you.

--Anand

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-15 16:23         ` Anand Moon
@ 2020-07-17  9:01           ` Anand Moon
  2020-07-17 16:38             ` Anand Moon
  0 siblings, 1 reply; 14+ messages in thread
From: Anand Moon @ 2020-07-17  9:01 UTC (permalink / raw)
  To: Dan Robertson
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

Hi Dan,

On Wed, 15 Jul 2020 at 21:53, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Dan,
>
> On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@dlrobertson.com> wrote:
> >
> > On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote:
> > > Hi Dan,
> > >
> > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote:
> > > >
> > > > Hi Anand!
> > > >
> > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:
> > > > > hi Dan,
> > > > >
> > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
> > > > > >
> > > > > > When testing suspend for another driver I noticed the following warning:
> > > > > >
> > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > > > > > Hardware name: Hardkernel ODROID-N2 (DT)
> > > > > > [..]
> > > > > > pc : reset_control_assert+0x184/0x19c
> > > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > > > > > [..]
> > > > > > Call trace:
> > > > > >  reset_control_assert+0x184/0x19c
> > > > > >  dwc3_meson_g12a_suspend+0x68/0x7c
> > > > > >  platform_pm_suspend+0x28/0x54
> > > > > >  __device_suspend+0x590/0xabc
> > > > > >  dpm_suspend+0x104/0x404
> > > > > >  dpm_suspend_start+0x84/0x1bc
> > > > > >  suspend_devices_and_enter+0xc4/0x4fc
> > > > > >
> > > > > > In my limited experience and knowlege it appears that we hit this
> > > > > > because the reset control was switched to shared and the the use
> > > > > > of the reset control was not changed.
> > > > > >
> > > > > > > * Calling reset_control_assert without first calling reset_control_deassert
> > > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is
> > > > > > > * also not allowed on a shared reset control.
> > > > > >
> > > > > > The above snippet from reset_control_get_shared() seems to indicate that
> > > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> > > > > > and reset_control_deassert is not guaranteed to have been called
> > > > > > before dwc3_meson_g12a_suspend() and reset_control_assert().
> > > > > >
> > > > > > After some basic tests with the following patch I no longer hit the
> > > > > > warning. Comments and critiques on the patch are welcome. If there is a
> > > > > > reason for the current use of the reset control, I'd love to learn why!
> > > > > > Like I said before, I have not really looked at this driver before and
> > > > > > have verify limited experience with reset controls... Was working on
> > > > > > another driver, hit the warning, and thought I'd take a shot at the
> > > > > > fix :-)
> > > > > >
> > > > > Thanks, I was also looking into this issue
> > > >
> > > > Awesome!
> > > >
> > > > > So best Fix to this issue is to drop the call of reset_control_assert
> > > > > from dwc3_meson_g12a_suspend
> > > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
> > > > > With these changes we do not see the warning on suspend and resume
> > > >
> > > > We definitely would avoid hitting the warning without the
> > > > reset_control_(de)assert calls in suspend and resume. That is a valid use of
> > > > the reset control, but why would that be best?
> > > >
> > > > From reset_control_reset():
> > >
> > > Before entering the suspend state the code tries to do following
> > >      clk_bulk_disable_unprepare
> > >      regulator_disable
> > >      phy_power_off
> > >      phy_exit
> > >
> > > After this operation it's needless to call *reset_control_assert*
> > > I tried to move this call before all the above operations
> > > but still no success with this.
> >
> > How so? Once the reset() is removed prope() and deassert() is guaranteed
> > to have been called before suspend, like what is in the patch and similar
> > to other uses of shared reset controllers, this is possible.
> >
> > > Similarly on resume we should avoid calling resume
> > > *reset_control_deassert* earlier
> > > as the core is just reinitializing the devices.
> > >       clk_bulk_prepare_enable
> > >       usb_init
> > >       phy_init
> > >       phy_power_on
> > >       regulator_enable
> > > >
> > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when
> > > > > * reset_control_reset has been used.
> > > >
> > > > If we use reset_control_reset() then we can not (de)assert the reset line
> > > > on suspend/resume or any other time. Wouldn't we want to be able to
> > > > (de)assert the reset line? And why do we no longer want to (de)assert the
> > > > reset line on suspend/resume?
> > > >
> > > > > > Can you try this attached patch?
> > > > >
> > > > I think I had already tested something similar. Removing the (de)assert calls
> > > > but keeping reset will definitely remove the warning, but it means we can not
> > > > (de)assert the line. My guess is that this is not what we want, but I could be
> > > > wrong. Thoughts, input, or references to documentation on this would be
> > > > appreciated.
> > > >
> > >
> > > As per my knowledge suspend to mem will do complete power down of the
> > > devices with support suspend and resume feature sequentially and then it tries
> > > to bring the device up one by one.
> > > So it should also take care of reset lines as well.
> >
> > So do we only _actually_ care about the reset line in the probe? Seems like we
> > should remove the reset controller from the structure if that is the case.
> >
> > Cheers,
> >
> >  - Dan
>
> Sorry I am not the _expert_ in suspend/resume feature but I can debug this,
> Certainly we need to look into whether reset controller calls are needed
> to suspend or resume for this driver.
>
> First thing we need to get the RTC driver to work on Odroid N2 for
> suspend wakeup
> to work properly.
> For this I have created the following patches.
>
> [0] https://patchwork.kernel.org/cover/11665865/
>
> With these patches the RTC feature for wake up is broken right now in
> my testing.
> Once I get something to work further I will update you.
>
> --Anand

Sorry for the _noise_ :‑(
This feature seems to be working fine with VRTC drivers.
I have tested this with a pre-compiled image of Archlinux distro.

[root@alarm alarm]# uname -a
Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux
[root@alarm alarm]# rtcwake -s 30 -m mem
rtcwake: assuming RTC uses UTC ...
rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan  1 00:10:14 1970
[  583.591477] PM: suspend entry (deep)
[  583.593737] Filesystems sync: 0.002 seconds
[  583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done.
[  583.825802] OOM killer disabled.
[  583.828966] Freezing remaining freezable tasks ... (elapsed 0.003
seconds) done.
[  583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache
[  584.020094] PM: suspend devices took 0.190 seconds
[  584.070586] Disabling non-boot CPUs ...
[  584.075037] CPU1: shutdown
[  584.075223] psci: CPU1 killed (polled 0 ms)
[  584.097199] CPU2: shutdown
[  584.098546] psci: CPU2 killed (polled 0 ms)
[  584.115370] CPU3: shutdown
[  584.116500] psci: CPU3 killed (polled 0 ms)
[  584.128116] CPU4: shutdown
[  584.129235] psci: CPU4 killed (polled 10 ms)
[  584.140122] CPU5: shutdown
[  584.147289] psci: CPU5 killed (polled 0 ms)
bl30 get wakeup sources!
process command 00000006
bl30 enter suspend!
Little core clk suspend rate 1896000000
Big core clk suspend rate 24000000
store restore gp0 pll
suspend_counter: 3
Enter ddr suspend
ddr suspend time: 16us
alarm=31S
process command 00000001
GPIOA_11/13 off
cec ver:2018/04/19
CEC cfg:0x0000
WAKEUP GPIO cfg:0x00000000
use vddee new table!
use vddee new table!
exit_reason:0x03
Enter ddr resume
DMC_DRAM_STAT3: 0x544
ddr resume time: 3188us
store restore gp0 pll
cfg15 33b00000
cfg15 33b00000
Li[  584.148720] Enabling non-boot CPUs ...
[  584.149124] Detected VIPT I-cache on CPU1
[  584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
[  584.149594] CPU1 is up
[  584.160687] Detected VIPT I-cache on CPU2
[  584.160730] arch_timer: CPU2: Trapping CNTVCT access
[  584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092]
[  584.161327] CPU2 is up
[  584.177645] Detected VIPT I-cache on CPU3
[  584.177668] arch_timer: CPU3: Trapping CNTVCT access
[  584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092]
[  584.178036] CPU3 is up
[  584.195338] Detected VIPT I-cache on CPU4
[  584.195361] arch_timer: CPU4: Trapping CNTVCT access
[  584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092]
[  584.195762] CPU4 is up
[  584.213002] Detected VIPT I-cache on CPU5
[  584.213024] arch_timer: CPU5: Trapping CNTVCT access
[  584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092]
[  584.213450] CPU5 is up
ttle core clk resume rate 1896000000
Big core clk resume rate 50000000
[  584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety
Features support found
[  584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for
phy/rgmii link mode
[  584.401216] usb usb1: root hub lost power or was reset
[  584.401470] usb usb2: root hub lost power or was reset
[  584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode
[  584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2
using xhci-hcd
[  584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd
[  585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3
using xhci-hcd
[  585.333303] PM: resume devices took 1.100 seconds
[  585.333507] OOM killer enabled.
[  585.335549] Restarting tasks ... done.
[  585.378044] PM: suspend exit
rtcwake: read rtc alarm failed: Invalid argument
[root@alarm alarm]#

-Anand

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-17  9:01           ` Anand Moon
@ 2020-07-17 16:38             ` Anand Moon
  2020-07-18  6:31               ` Anand Moon
  0 siblings, 1 reply; 14+ messages in thread
From: Anand Moon @ 2020-07-17 16:38 UTC (permalink / raw)
  To: Dan Robertson
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

Hi Dan,

On Fri, 17 Jul 2020 at 14:31, Anand Moon <linux.amoon@gmail.com> wrote:
>
> Hi Dan,
>
> On Wed, 15 Jul 2020 at 21:53, Anand Moon <linux.amoon@gmail.com> wrote:
> >
> > Hi Dan,
> >
> > On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@dlrobertson.com> wrote:
> > >
> > > On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote:
> > > > Hi Dan,
> > > >
> > > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote:
> > > > >
> > > > > Hi Anand!
> > > > >
> > > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote:
> > > > > > hi Dan,
> > > > > >
> > > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote:
> > > > > > >
> > > > > > > When testing suspend for another driver I noticed the following warning:
> > > > > > >
> > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > > > > > > Hardware name: Hardkernel ODROID-N2 (DT)
> > > > > > > [..]
> > > > > > > pc : reset_control_assert+0x184/0x19c
> > > > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > > > > > > [..]
> > > > > > > Call trace:
> > > > > > >  reset_control_assert+0x184/0x19c
> > > > > > >  dwc3_meson_g12a_suspend+0x68/0x7c
> > > > > > >  platform_pm_suspend+0x28/0x54
> > > > > > >  __device_suspend+0x590/0xabc
> > > > > > >  dpm_suspend+0x104/0x404
> > > > > > >  dpm_suspend_start+0x84/0x1bc
> > > > > > >  suspend_devices_and_enter+0xc4/0x4fc
> > > > > > >
> > > > > > > In my limited experience and knowlege it appears that we hit this
> > > > > > > because the reset control was switched to shared and the the use
> > > > > > > of the reset control was not changed.
> > > > > > >
> > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert
> > > > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is
> > > > > > > > * also not allowed on a shared reset control.
> > > > > > >
> > > > > > > The above snippet from reset_control_get_shared() seems to indicate that
> > > > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
> > > > > > > and reset_control_deassert is not guaranteed to have been called
> > > > > > > before dwc3_meson_g12a_suspend() and reset_control_assert().
> > > > > > >
> > > > > > > After some basic tests with the following patch I no longer hit the
> > > > > > > warning. Comments and critiques on the patch are welcome. If there is a
> > > > > > > reason for the current use of the reset control, I'd love to learn why!
> > > > > > > Like I said before, I have not really looked at this driver before and
> > > > > > > have verify limited experience with reset controls... Was working on
> > > > > > > another driver, hit the warning, and thought I'd take a shot at the
> > > > > > > fix :-)
> > > > > > >
> > > > > > Thanks, I was also looking into this issue
> > > > >
> > > > > Awesome!
> > > > >
> > > > > > So best Fix to this issue is to drop the call of reset_control_assert
> > > > > > from dwc3_meson_g12a_suspend
> > > > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume
> > > > > > With these changes we do not see the warning on suspend and resume
> > > > >
> > > > > We definitely would avoid hitting the warning without the
> > > > > reset_control_(de)assert calls in suspend and resume. That is a valid use of
> > > > > the reset control, but why would that be best?
> > > > >
> > > > > From reset_control_reset():
> > > >
> > > > Before entering the suspend state the code tries to do following
> > > >      clk_bulk_disable_unprepare
> > > >      regulator_disable
> > > >      phy_power_off
> > > >      phy_exit
> > > >
> > > > After this operation it's needless to call *reset_control_assert*
> > > > I tried to move this call before all the above operations
> > > > but still no success with this.
> > >
> > > How so? Once the reset() is removed prope() and deassert() is guaranteed
> > > to have been called before suspend, like what is in the patch and similar
> > > to other uses of shared reset controllers, this is possible.
> > >
> > > > Similarly on resume we should avoid calling resume
> > > > *reset_control_deassert* earlier
> > > > as the core is just reinitializing the devices.
> > > >       clk_bulk_prepare_enable
> > > >       usb_init
> > > >       phy_init
> > > >       phy_power_on
> > > >       regulator_enable
> > > > >
> > > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when
> > > > > > * reset_control_reset has been used.
> > > > >
> > > > > If we use reset_control_reset() then we can not (de)assert the reset line
> > > > > on suspend/resume or any other time. Wouldn't we want to be able to
> > > > > (de)assert the reset line? And why do we no longer want to (de)assert the
> > > > > reset line on suspend/resume?
> > > > >
> > > > > > > Can you try this attached patch?
> > > > > >
> > > > > I think I had already tested something similar. Removing the (de)assert calls
> > > > > but keeping reset will definitely remove the warning, but it means we can not
> > > > > (de)assert the line. My guess is that this is not what we want, but I could be
> > > > > wrong. Thoughts, input, or references to documentation on this would be
> > > > > appreciated.
> > > > >
> > > >
> > > > As per my knowledge suspend to mem will do complete power down of the
> > > > devices with support suspend and resume feature sequentially and then it tries
> > > > to bring the device up one by one.
> > > > So it should also take care of reset lines as well.
> > >
> > > So do we only _actually_ care about the reset line in the probe? Seems like we
> > > should remove the reset controller from the structure if that is the case.
> > >
> > > Cheers,
> > >
> > >  - Dan
> >
> > Sorry I am not the _expert_ in suspend/resume feature but I can debug this,
> > Certainly we need to look into whether reset controller calls are needed
> > to suspend or resume for this driver.
> >
> > First thing we need to get the RTC driver to work on Odroid N2 for
> > suspend wakeup
> > to work properly.
> > For this I have created the following patches.
> >
> > [0] https://patchwork.kernel.org/cover/11665865/
> >
> > With these patches the RTC feature for wake up is broken right now in
> > my testing.
> > Once I get something to work further I will update you.
> >
> > --Anand
>
> Sorry for the _noise_ :‑(
> This feature seems to be working fine with VRTC drivers.
> I have tested this with a pre-compiled image of Archlinux distro.
>
> [root@alarm alarm]# uname -a
> Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux
> [root@alarm alarm]# rtcwake -s 30 -m mem
> rtcwake: assuming RTC uses UTC ...
> rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan  1 00:10:14 1970
> [  583.591477] PM: suspend entry (deep)
> [  583.593737] Filesystems sync: 0.002 seconds
> [  583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done.
> [  583.825802] OOM killer disabled.
> [  583.828966] Freezing remaining freezable tasks ... (elapsed 0.003
> seconds) done.
> [  583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> [  584.020094] PM: suspend devices took 0.190 seconds
> [  584.070586] Disabling non-boot CPUs ...
> [  584.075037] CPU1: shutdown
> [  584.075223] psci: CPU1 killed (polled 0 ms)
> [  584.097199] CPU2: shutdown
> [  584.098546] psci: CPU2 killed (polled 0 ms)
> [  584.115370] CPU3: shutdown
> [  584.116500] psci: CPU3 killed (polled 0 ms)
> [  584.128116] CPU4: shutdown
> [  584.129235] psci: CPU4 killed (polled 10 ms)
> [  584.140122] CPU5: shutdown
> [  584.147289] psci: CPU5 killed (polled 0 ms)
> bl30 get wakeup sources!
> process command 00000006
> bl30 enter suspend!
> Little core clk suspend rate 1896000000
> Big core clk suspend rate 24000000
> store restore gp0 pll
> suspend_counter: 3
> Enter ddr suspend
> ddr suspend time: 16us
> alarm=31S
> process command 00000001
> GPIOA_11/13 off
> cec ver:2018/04/19
> CEC cfg:0x0000
> WAKEUP GPIO cfg:0x00000000
> use vddee new table!
> use vddee new table!
> exit_reason:0x03
> Enter ddr resume
> DMC_DRAM_STAT3: 0x544
> ddr resume time: 3188us
> store restore gp0 pll
> cfg15 33b00000
> cfg15 33b00000
> Li[  584.148720] Enabling non-boot CPUs ...
> [  584.149124] Detected VIPT I-cache on CPU1
> [  584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
> [  584.149594] CPU1 is up
> [  584.160687] Detected VIPT I-cache on CPU2
> [  584.160730] arch_timer: CPU2: Trapping CNTVCT access
> [  584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092]
> [  584.161327] CPU2 is up
> [  584.177645] Detected VIPT I-cache on CPU3
> [  584.177668] arch_timer: CPU3: Trapping CNTVCT access
> [  584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092]
> [  584.178036] CPU3 is up
> [  584.195338] Detected VIPT I-cache on CPU4
> [  584.195361] arch_timer: CPU4: Trapping CNTVCT access
> [  584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092]
> [  584.195762] CPU4 is up
> [  584.213002] Detected VIPT I-cache on CPU5
> [  584.213024] arch_timer: CPU5: Trapping CNTVCT access
> [  584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092]
> [  584.213450] CPU5 is up
> ttle core clk resume rate 1896000000
> Big core clk resume rate 50000000
> [  584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety
> Features support found
> [  584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for
> phy/rgmii link mode
> [  584.401216] usb usb1: root hub lost power or was reset
> [  584.401470] usb usb2: root hub lost power or was reset
> [  584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode
> [  584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2
> using xhci-hcd
> [  584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd
> [  585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3
> using xhci-hcd
> [  585.333303] PM: resume devices took 1.100 seconds
> [  585.333507] OOM killer enabled.
> [  585.335549] Restarting tasks ... done.
> [  585.378044] PM: suspend exit
> rtcwake: read rtc alarm failed: Invalid argument
> [root@alarm alarm]#
>
> -Anand

After confirming that the suspend resume feature is working correctly
I found the solution to the reset warning on 5.8.x kernel
Please can you try this following patch.

$ cat resetwarn.patch
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c
b/drivers/usb/dwc3/dwc3-meson-g12a.c
index 1f7f4d88ed9d..60a6f49139fd 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct
platform_device *pdev)

        platform_set_drvdata(pdev, priv);

-       priv->reset = devm_reset_control_get_shared(dev, NULL);
+       priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson");
        if (IS_ERR(priv->reset)) {
                ret = PTR_ERR(priv->reset);
                dev_err(dev, "failed to get device reset, err=%d\n", ret);

-Anand

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-17 16:38             ` Anand Moon
@ 2020-07-18  6:31               ` Anand Moon
  2020-07-18  8:46                 ` Neil Armstrong
  0 siblings, 1 reply; 14+ messages in thread
From: Anand Moon @ 2020-07-18  6:31 UTC (permalink / raw)
  To: Dan Robertson
  Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

Hi Dan,

> > Sorry for the _noise_ :‑(
> > This feature seems to be working fine with VRTC drivers.
> > I have tested this with a pre-compiled image of Archlinux distro.
> >
> > [root@alarm alarm]# uname -a
> > Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux
> > [root@alarm alarm]# rtcwake -s 30 -m mem
> > rtcwake: assuming RTC uses UTC ...
> > rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan  1 00:10:14 1970
> > [  583.591477] PM: suspend entry (deep)
> > [  583.593737] Filesystems sync: 0.002 seconds
> > [  583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done.
> > [  583.825802] OOM killer disabled.
> > [  583.828966] Freezing remaining freezable tasks ... (elapsed 0.003
> > seconds) done.
> > [  583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> > [  584.020094] PM: suspend devices took 0.190 seconds
> > [  584.070586] Disabling non-boot CPUs ...
> > [  584.075037] CPU1: shutdown
> > [  584.075223] psci: CPU1 killed (polled 0 ms)
> > [  584.097199] CPU2: shutdown
> > [  584.098546] psci: CPU2 killed (polled 0 ms)
> > [  584.115370] CPU3: shutdown
> > [  584.116500] psci: CPU3 killed (polled 0 ms)
> > [  584.128116] CPU4: shutdown
> > [  584.129235] psci: CPU4 killed (polled 10 ms)
> > [  584.140122] CPU5: shutdown
> > [  584.147289] psci: CPU5 killed (polled 0 ms)
> > bl30 get wakeup sources!
> > process command 00000006
> > bl30 enter suspend!
> > Little core clk suspend rate 1896000000
> > Big core clk suspend rate 24000000
> > store restore gp0 pll
> > suspend_counter: 3
> > Enter ddr suspend
> > ddr suspend time: 16us
> > alarm=31S
> > process command 00000001
> > GPIOA_11/13 off
> > cec ver:2018/04/19
> > CEC cfg:0x0000
> > WAKEUP GPIO cfg:0x00000000
> > use vddee new table!
> > use vddee new table!
> > exit_reason:0x03
> > Enter ddr resume
> > DMC_DRAM_STAT3: 0x544
> > ddr resume time: 3188us
> > store restore gp0 pll
> > cfg15 33b00000
> > cfg15 33b00000
> > Li[  584.148720] Enabling non-boot CPUs ...
> > [  584.149124] Detected VIPT I-cache on CPU1
> > [  584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
> > [  584.149594] CPU1 is up
> > [  584.160687] Detected VIPT I-cache on CPU2
> > [  584.160730] arch_timer: CPU2: Trapping CNTVCT access
> > [  584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092]
> > [  584.161327] CPU2 is up
> > [  584.177645] Detected VIPT I-cache on CPU3
> > [  584.177668] arch_timer: CPU3: Trapping CNTVCT access
> > [  584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092]
> > [  584.178036] CPU3 is up
> > [  584.195338] Detected VIPT I-cache on CPU4
> > [  584.195361] arch_timer: CPU4: Trapping CNTVCT access
> > [  584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092]
> > [  584.195762] CPU4 is up
> > [  584.213002] Detected VIPT I-cache on CPU5
> > [  584.213024] arch_timer: CPU5: Trapping CNTVCT access
> > [  584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092]
> > [  584.213450] CPU5 is up
> > ttle core clk resume rate 1896000000
> > Big core clk resume rate 50000000
> > [  584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety
> > Features support found
> > [  584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for
> > phy/rgmii link mode
> > [  584.401216] usb usb1: root hub lost power or was reset
> > [  584.401470] usb usb2: root hub lost power or was reset
> > [  584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode
> > [  584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2
> > using xhci-hcd
> > [  584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd
> > [  585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3
> > using xhci-hcd
> > [  585.333303] PM: resume devices took 1.100 seconds
> > [  585.333507] OOM killer enabled.
> > [  585.335549] Restarting tasks ... done.
> > [  585.378044] PM: suspend exit
> > rtcwake: read rtc alarm failed: Invalid argument
> > [root@alarm alarm]#
> >
> > -Anand
>
> After confirming that the suspend resume feature is working correctly
> I found the solution to the reset warning on 5.8.x kernel
> Please can you try this following patch.
>
> $ cat resetwarn.patch
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c
> b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index 1f7f4d88ed9d..60a6f49139fd 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct
> platform_device *pdev)
>
>         platform_set_drvdata(pdev, priv);
>
> -       priv->reset = devm_reset_control_get_shared(dev, NULL);
> +       priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson");
>         if (IS_ERR(priv->reset)) {
>                 ret = PTR_ERR(priv->reset);
>                 dev_err(dev, "failed to get device reset, err=%d\n", ret);
>
> -Anand

Apologize once again above changes break the usb functionality.
the correct fix along with these changes should be as below.
reset controllers need *resets* and *reset-names* to work correctly.

But the _reset controller_ warning continues on suspend / resume features,
I am looking to find a FIX into this issue.

diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
index 593a006f4b7b..6d34dfa9825c 100644
--- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
+++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
@@ -2318,6 +2318,7 @@ usb: usb@ffe09000 {

                        clocks = <&clkc CLKID_USB>;
                        resets = <&reset RESET_USB>;
+                       reset-names = "dwc3_meson";

                        dr_mode = "otg";

-Anand

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-18  6:31               ` Anand Moon
@ 2020-07-18  8:46                 ` Neil Armstrong
  2020-07-18  9:54                   ` Anand Moon
  0 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2020-07-18  8:46 UTC (permalink / raw)
  To: Anand Moon, Dan Robertson
  Cc: Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List,
	linux-arm-kernel, linux-amlogic

Hi Anand,

Le 18/07/2020 à 08:31, Anand Moon a écrit :
> Hi Dan,
> 
>>> Sorry for the _noise_ :‑(
>>> This feature seems to be working fine with VRTC drivers.
>>> I have tested this with a pre-compiled image of Archlinux distro.
>>>
>>> [root@alarm alarm]# uname -a
>>> Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux
>>> [root@alarm alarm]# rtcwake -s 30 -m mem
>>> rtcwake: assuming RTC uses UTC ...
>>> rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan  1 00:10:14 1970
>>> [  583.591477] PM: suspend entry (deep)
>>> [  583.593737] Filesystems sync: 0.002 seconds
>>> [  583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done.
>>> [  583.825802] OOM killer disabled.
>>> [  583.828966] Freezing remaining freezable tasks ... (elapsed 0.003
>>> seconds) done.
>>> [  583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache
>>> [  584.020094] PM: suspend devices took 0.190 seconds
>>> [  584.070586] Disabling non-boot CPUs ...
>>> [  584.075037] CPU1: shutdown
>>> [  584.075223] psci: CPU1 killed (polled 0 ms)
>>> [  584.097199] CPU2: shutdown
>>> [  584.098546] psci: CPU2 killed (polled 0 ms)
>>> [  584.115370] CPU3: shutdown
>>> [  584.116500] psci: CPU3 killed (polled 0 ms)
>>> [  584.128116] CPU4: shutdown
>>> [  584.129235] psci: CPU4 killed (polled 10 ms)
>>> [  584.140122] CPU5: shutdown
>>> [  584.147289] psci: CPU5 killed (polled 0 ms)
>>> bl30 get wakeup sources!
>>> process command 00000006
>>> bl30 enter suspend!
>>> Little core clk suspend rate 1896000000
>>> Big core clk suspend rate 24000000
>>> store restore gp0 pll
>>> suspend_counter: 3
>>> Enter ddr suspend
>>> ddr suspend time: 16us
>>> alarm=31S
>>> process command 00000001
>>> GPIOA_11/13 off
>>> cec ver:2018/04/19
>>> CEC cfg:0x0000
>>> WAKEUP GPIO cfg:0x00000000
>>> use vddee new table!
>>> use vddee new table!
>>> exit_reason:0x03
>>> Enter ddr resume
>>> DMC_DRAM_STAT3: 0x544
>>> ddr resume time: 3188us
>>> store restore gp0 pll
>>> cfg15 33b00000
>>> cfg15 33b00000
>>> Li[  584.148720] Enabling non-boot CPUs ...
>>> [  584.149124] Detected VIPT I-cache on CPU1
>>> [  584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
>>> [  584.149594] CPU1 is up
>>> [  584.160687] Detected VIPT I-cache on CPU2
>>> [  584.160730] arch_timer: CPU2: Trapping CNTVCT access
>>> [  584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092]
>>> [  584.161327] CPU2 is up
>>> [  584.177645] Detected VIPT I-cache on CPU3
>>> [  584.177668] arch_timer: CPU3: Trapping CNTVCT access
>>> [  584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092]
>>> [  584.178036] CPU3 is up
>>> [  584.195338] Detected VIPT I-cache on CPU4
>>> [  584.195361] arch_timer: CPU4: Trapping CNTVCT access
>>> [  584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092]
>>> [  584.195762] CPU4 is up
>>> [  584.213002] Detected VIPT I-cache on CPU5
>>> [  584.213024] arch_timer: CPU5: Trapping CNTVCT access
>>> [  584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092]
>>> [  584.213450] CPU5 is up
>>> ttle core clk resume rate 1896000000
>>> Big core clk resume rate 50000000
>>> [  584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety
>>> Features support found
>>> [  584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for
>>> phy/rgmii link mode
>>> [  584.401216] usb usb1: root hub lost power or was reset
>>> [  584.401470] usb usb2: root hub lost power or was reset
>>> [  584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode
>>> [  584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2
>>> using xhci-hcd
>>> [  584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd
>>> [  585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3
>>> using xhci-hcd
>>> [  585.333303] PM: resume devices took 1.100 seconds
>>> [  585.333507] OOM killer enabled.
>>> [  585.335549] Restarting tasks ... done.
>>> [  585.378044] PM: suspend exit
>>> rtcwake: read rtc alarm failed: Invalid argument
>>> [root@alarm alarm]#
>>>
>>> -Anand
>>
>> After confirming that the suspend resume feature is working correctly
>> I found the solution to the reset warning on 5.8.x kernel
>> Please can you try this following patch.
>>
>> $ cat resetwarn.patch
>> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c
>> b/drivers/usb/dwc3/dwc3-meson-g12a.c
>> index 1f7f4d88ed9d..60a6f49139fd 100644
>> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
>> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
>> @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct
>> platform_device *pdev)
>>
>>         platform_set_drvdata(pdev, priv);
>>
>> -       priv->reset = devm_reset_control_get_shared(dev, NULL);
>> +       priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson");
>>         if (IS_ERR(priv->reset)) {
>>                 ret = PTR_ERR(priv->reset);
>>                 dev_err(dev, "failed to get device reset, err=%d\n", ret);
>>
>> -Anand
> 
> Apologize once again above changes break the usb functionality.
> the correct fix along with these changes should be as below.
> reset controllers need *resets* and *reset-names* to work correctly.
> 
> But the _reset controller_ warning continues on suspend / resume features,
> I am looking to find a FIX into this issue.
> 
> diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> index 593a006f4b7b..6d34dfa9825c 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> @@ -2318,6 +2318,7 @@ usb: usb@ffe09000 {
> 
>                         clocks = <&clkc CLKID_USB>;
>                         resets = <&reset RESET_USB>;
> +                       reset-names = "dwc3_meson";
> 
>                         dr_mode = "otg";
> 
> -Anand
> 


Theses 2 changes :

>> -       priv->reset = devm_reset_control_get_shared(dev, NULL);
>> +       priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson");

and

> +                       reset-names = "dwc3_meson";

are a no-op, since devm_reset_control_get_shared(dev, NULL) takes the
first reset, with or without a reset-names.

Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson
@ 2020-07-18  8:47   ` Neil Armstrong
  2020-07-18 22:57     ` Dan Robertson
  0 siblings, 1 reply; 14+ messages in thread
From: Neil Armstrong @ 2020-07-18  8:47 UTC (permalink / raw)
  To: Dan Robertson, Martin Blumenstingl, Kevin Hilman
  Cc: linux-amlogic, linux-usb, linux-arm-kernel

Hi,

Le 13/07/2020 à 18:05, Dan Robertson a écrit :
> The reset is a shared reset line, but reset_control_reset is still used
> and reset_control_deassert is not guaranteed to have been called before
> the first reset_control_assert call. When suspending the following
> warning may be seen:
> 
> WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> Hardware name: Hardkernel ODROID-N2 (DT)
> [..]
> pc : reset_control_assert+0x184/0x19c
> lr : dwc3_meson_g12a_suspend+0x68/0x7c
> [..]
> Call trace:
>  reset_control_assert+0x184/0x19c
>  dwc3_meson_g12a_suspend+0x68/0x7c
>  platform_pm_suspend+0x28/0x54
>  __device_suspend+0x590/0xabc
>  dpm_suspend+0x104/0x404
>  dpm_suspend_start+0x84/0x1bc
>  suspend_devices_and_enter+0xc4/0x4fc
>  pm_suspend+0x198/0x2d4
> 
> Fixes: 6d9fa35a347a87 ("usb: dwc3: meson-g12a: get the reset as shared")
> Signed-off-by: Dan Robertson <dan@dlrobertson.com>
> ---
>  drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> index 1f7f4d88ed9d..88b75b5a039c 100644
> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> @@ -737,13 +737,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
>  		goto err_disable_clks;
>  	}
>  
> -	ret = reset_control_reset(priv->reset);
> +	ret = reset_control_deassert(priv->reset);
>  	if (ret)
> -		goto err_disable_clks;
> +		goto err_assert_reset;
>  
>  	ret = dwc3_meson_g12a_get_phys(priv);
>  	if (ret)
> -		goto err_disable_clks;
> +		goto err_assert_reset;
>  
>  	ret = priv->drvdata->setup_regmaps(priv, base);
>  	if (ret)
> @@ -752,7 +752,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
>  	if (priv->vbus) {
>  		ret = regulator_enable(priv->vbus);
>  		if (ret)
> -			goto err_disable_clks;
> +			goto err_assert_reset;
>  	}
>  
>  	/* Get dr_mode */
> @@ -765,13 +765,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
>  
>  	ret = priv->drvdata->usb_init(priv);
>  	if (ret)
> -		goto err_disable_clks;
> +		goto err_assert_reset;
>  
>  	/* Init PHYs */
>  	for (i = 0 ; i < PHY_COUNT ; ++i) {
>  		ret = phy_init(priv->phys[i]);
>  		if (ret)
> -			goto err_disable_clks;
> +			goto err_assert_reset;
>  	}
>  
>  	/* Set PHY Power */
> @@ -809,6 +809,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
>  	for (i = 0 ; i < PHY_COUNT ; ++i)
>  		phy_exit(priv->phys[i]);
>  
> +err_assert_reset:
> +	reset_control_assert(priv->reset);
> +
>  err_disable_clks:
>  	clk_bulk_disable_unprepare(priv->drvdata->num_clks,
>  				   priv->drvdata->clks);
> 


This should do the trick, I'll need to check first if it doesn't break the GXL/GXM
support first.

thanks for the fix

Neil

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-18  8:46                 ` Neil Armstrong
@ 2020-07-18  9:54                   ` Anand Moon
  0 siblings, 0 replies; 14+ messages in thread
From: Anand Moon @ 2020-07-18  9:54 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Dan Robertson, Martin Blumenstingl, Kevin Hilman,
	Linux USB Mailing List, linux-amlogic, linux-arm-kernel

Hi Neil,

On Sat, 18 Jul 2020 at 14:16, Neil Armstrong <narmstrong@baylibre.com> wrote:
>
> Hi Anand,
>
> Le 18/07/2020 à 08:31, Anand Moon a écrit :
> > Hi Dan,
> >
> >>> Sorry for the _noise_ :‑(
> >>> This feature seems to be working fine with VRTC drivers.
> >>> I have tested this with a pre-compiled image of Archlinux distro.
> >>>
> >>> [root@alarm alarm]# uname -a
> >>> Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux
> >>> [root@alarm alarm]# rtcwake -s 30 -m mem
> >>> rtcwake: assuming RTC uses UTC ...
> >>> rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan  1 00:10:14 1970
> >>> [  583.591477] PM: suspend entry (deep)
> >>> [  583.593737] Filesystems sync: 0.002 seconds
> >>> [  583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done.
> >>> [  583.825802] OOM killer disabled.
> >>> [  583.828966] Freezing remaining freezable tasks ... (elapsed 0.003
> >>> seconds) done.
> >>> [  583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache
> >>> [  584.020094] PM: suspend devices took 0.190 seconds
> >>> [  584.070586] Disabling non-boot CPUs ...
> >>> [  584.075037] CPU1: shutdown
> >>> [  584.075223] psci: CPU1 killed (polled 0 ms)
> >>> [  584.097199] CPU2: shutdown
> >>> [  584.098546] psci: CPU2 killed (polled 0 ms)
> >>> [  584.115370] CPU3: shutdown
> >>> [  584.116500] psci: CPU3 killed (polled 0 ms)
> >>> [  584.128116] CPU4: shutdown
> >>> [  584.129235] psci: CPU4 killed (polled 10 ms)
> >>> [  584.140122] CPU5: shutdown
> >>> [  584.147289] psci: CPU5 killed (polled 0 ms)
> >>> bl30 get wakeup sources!
> >>> process command 00000006
> >>> bl30 enter suspend!
> >>> Little core clk suspend rate 1896000000
> >>> Big core clk suspend rate 24000000
> >>> store restore gp0 pll
> >>> suspend_counter: 3
> >>> Enter ddr suspend
> >>> ddr suspend time: 16us
> >>> alarm=31S
> >>> process command 00000001
> >>> GPIOA_11/13 off
> >>> cec ver:2018/04/19
> >>> CEC cfg:0x0000
> >>> WAKEUP GPIO cfg:0x00000000
> >>> use vddee new table!
> >>> use vddee new table!
> >>> exit_reason:0x03
> >>> Enter ddr resume
> >>> DMC_DRAM_STAT3: 0x544
> >>> ddr resume time: 3188us
> >>> store restore gp0 pll
> >>> cfg15 33b00000
> >>> cfg15 33b00000
> >>> Li[  584.148720] Enabling non-boot CPUs ...
> >>> [  584.149124] Detected VIPT I-cache on CPU1
> >>> [  584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034]
> >>> [  584.149594] CPU1 is up
> >>> [  584.160687] Detected VIPT I-cache on CPU2
> >>> [  584.160730] arch_timer: CPU2: Trapping CNTVCT access
> >>> [  584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092]
> >>> [  584.161327] CPU2 is up
> >>> [  584.177645] Detected VIPT I-cache on CPU3
> >>> [  584.177668] arch_timer: CPU3: Trapping CNTVCT access
> >>> [  584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092]
> >>> [  584.178036] CPU3 is up
> >>> [  584.195338] Detected VIPT I-cache on CPU4
> >>> [  584.195361] arch_timer: CPU4: Trapping CNTVCT access
> >>> [  584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092]
> >>> [  584.195762] CPU4 is up
> >>> [  584.213002] Detected VIPT I-cache on CPU5
> >>> [  584.213024] arch_timer: CPU5: Trapping CNTVCT access
> >>> [  584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092]
> >>> [  584.213450] CPU5 is up
> >>> ttle core clk resume rate 1896000000
> >>> Big core clk resume rate 50000000
> >>> [  584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety
> >>> Features support found
> >>> [  584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for
> >>> phy/rgmii link mode
> >>> [  584.401216] usb usb1: root hub lost power or was reset
> >>> [  584.401470] usb usb2: root hub lost power or was reset
> >>> [  584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode
> >>> [  584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2
> >>> using xhci-hcd
> >>> [  584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd
> >>> [  585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3
> >>> using xhci-hcd
> >>> [  585.333303] PM: resume devices took 1.100 seconds
> >>> [  585.333507] OOM killer enabled.
> >>> [  585.335549] Restarting tasks ... done.
> >>> [  585.378044] PM: suspend exit
> >>> rtcwake: read rtc alarm failed: Invalid argument
> >>> [root@alarm alarm]#
> >>>
> >>> -Anand
> >>
> >> After confirming that the suspend resume feature is working correctly
> >> I found the solution to the reset warning on 5.8.x kernel
> >> Please can you try this following patch.
> >>
> >> $ cat resetwarn.patch
> >> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c
> >> b/drivers/usb/dwc3/dwc3-meson-g12a.c
> >> index 1f7f4d88ed9d..60a6f49139fd 100644
> >> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> >> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> >> @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct
> >> platform_device *pdev)
> >>
> >>         platform_set_drvdata(pdev, priv);
> >>
> >> -       priv->reset = devm_reset_control_get_shared(dev, NULL);
> >> +       priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson");
> >>         if (IS_ERR(priv->reset)) {
> >>                 ret = PTR_ERR(priv->reset);
> >>                 dev_err(dev, "failed to get device reset, err=%d\n", ret);
> >>
> >> -Anand
> >
> > Apologize once again above changes break the usb functionality.
> > the correct fix along with these changes should be as below.
> > reset controllers need *resets* and *reset-names* to work correctly.
> >
> > But the _reset controller_ warning continues on suspend / resume features,
> > I am looking to find a FIX into this issue.
> >
> > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> > b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> > index 593a006f4b7b..6d34dfa9825c 100644
> > --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> > +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi
> > @@ -2318,6 +2318,7 @@ usb: usb@ffe09000 {
> >
> >                         clocks = <&clkc CLKID_USB>;
> >                         resets = <&reset RESET_USB>;
> > +                       reset-names = "dwc3_meson";
> >
> >                         dr_mode = "otg";
> >
> > -Anand
> >
>
>
> Theses 2 changes :
>
> >> -       priv->reset = devm_reset_control_get_shared(dev, NULL);
> >> +       priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson");
>
> and
>
> > +                       reset-names = "dwc3_meson";
>
> are a no-op, since devm_reset_control_get_shared(dev, NULL) takes the
> first reset, with or without a reset-names.
>

Thanks for resolving my query. Sorry for the noise.

> Neil

-Anand

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use
  2020-07-18  8:47   ` Neil Armstrong
@ 2020-07-18 22:57     ` Dan Robertson
  0 siblings, 0 replies; 14+ messages in thread
From: Dan Robertson @ 2020-07-18 22:57 UTC (permalink / raw)
  To: Neil Armstrong
  Cc: Martin Blumenstingl, Kevin Hilman, linux-usb, linux-arm-kernel,
	linux-amlogic

On Sat, Jul 18, 2020 at 10:47:55AM +0200, Neil Armstrong wrote:
> Hi,
> 
> Le 13/07/2020 à 18:05, Dan Robertson a écrit :
> > The reset is a shared reset line, but reset_control_reset is still used
> > and reset_control_deassert is not guaranteed to have been called before
> > the first reset_control_assert call. When suspending the following
> > warning may be seen:
> > 
> > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
> > Hardware name: Hardkernel ODROID-N2 (DT)
> > [..]
> > pc : reset_control_assert+0x184/0x19c
> > lr : dwc3_meson_g12a_suspend+0x68/0x7c
> > [..]
> > Call trace:
> >  reset_control_assert+0x184/0x19c
> >  dwc3_meson_g12a_suspend+0x68/0x7c
> >  platform_pm_suspend+0x28/0x54
> >  __device_suspend+0x590/0xabc
> >  dpm_suspend+0x104/0x404
> >  dpm_suspend_start+0x84/0x1bc
> >  suspend_devices_and_enter+0xc4/0x4fc
> >  pm_suspend+0x198/0x2d4
> > 
> > Fixes: 6d9fa35a347a87 ("usb: dwc3: meson-g12a: get the reset as shared")
> > Signed-off-by: Dan Robertson <dan@dlrobertson.com>
> > ---
> >  drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
> > index 1f7f4d88ed9d..88b75b5a039c 100644
> > --- a/drivers/usb/dwc3/dwc3-meson-g12a.c
> > +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
> > @@ -737,13 +737,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> >  		goto err_disable_clks;
> >  	}
> >  
> > -	ret = reset_control_reset(priv->reset);
> > +	ret = reset_control_deassert(priv->reset);
> >  	if (ret)
> > -		goto err_disable_clks;
> > +		goto err_assert_reset;
> >  
> >  	ret = dwc3_meson_g12a_get_phys(priv);
> >  	if (ret)
> > -		goto err_disable_clks;
> > +		goto err_assert_reset;
> >  
> >  	ret = priv->drvdata->setup_regmaps(priv, base);
> >  	if (ret)
> > @@ -752,7 +752,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> >  	if (priv->vbus) {
> >  		ret = regulator_enable(priv->vbus);
> >  		if (ret)
> > -			goto err_disable_clks;
> > +			goto err_assert_reset;
> >  	}
> >  
> >  	/* Get dr_mode */
> > @@ -765,13 +765,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> >  
> >  	ret = priv->drvdata->usb_init(priv);
> >  	if (ret)
> > -		goto err_disable_clks;
> > +		goto err_assert_reset;
> >  
> >  	/* Init PHYs */
> >  	for (i = 0 ; i < PHY_COUNT ; ++i) {
> >  		ret = phy_init(priv->phys[i]);
> >  		if (ret)
> > -			goto err_disable_clks;
> > +			goto err_assert_reset;
> >  	}
> >  
> >  	/* Set PHY Power */
> > @@ -809,6 +809,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
> >  	for (i = 0 ; i < PHY_COUNT ; ++i)
> >  		phy_exit(priv->phys[i]);
> >  
> > +err_assert_reset:
> > +	reset_control_assert(priv->reset);
> > +
> >  err_disable_clks:
> >  	clk_bulk_disable_unprepare(priv->drvdata->num_clks,
> >  				   priv->drvdata->clks);
> > 
> 
> 
> This should do the trick, I'll need to check first if it doesn't break the GXL/GXM
> support first.

Awesome. Let me know if you find anything.

Cheers,

 - Dan


_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic

^ permalink raw reply	[flat|nested] 14+ messages in thread

end of thread, back to index

Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson
2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson
2020-07-18  8:47   ` Neil Armstrong
2020-07-18 22:57     ` Dan Robertson
2020-07-14  6:56 ` [PATCH 0/1] " Anand Moon
2020-07-14 13:30   ` Dan Robertson
2020-07-14 15:27     ` Anand Moon
2020-07-15  2:58       ` Dan Robertson
2020-07-15 16:23         ` Anand Moon
2020-07-17  9:01           ` Anand Moon
2020-07-17 16:38             ` Anand Moon
2020-07-18  6:31               ` Anand Moon
2020-07-18  8:46                 ` Neil Armstrong
2020-07-18  9:54                   ` Anand Moon

Linux-Amlogic Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-amlogic/0 linux-amlogic/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-amlogic linux-amlogic/ https://lore.kernel.org/linux-amlogic \
		linux-amlogic@lists.infradead.org
	public-inbox-index linux-amlogic

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-amlogic


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git