[PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson]

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

[PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson]

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Neil Armstrong]

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson]

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:

> 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:

And now the same type of warning maybe seen on boot. This is
happening for me on the libretech-cc (s905x - gxl).

[    1.863469] ------------[ cut here ]------------
[    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
[    1.876525] Modules linked in:
[    1.879548] CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.9.0-rc1-00167-ga2e4e3a34775 #64
[    1.887737] Hardware name: Libre Computer AML-S905X-CC V2 (DT)
[    1.893525] Workqueue: events deferred_probe_work_func
[    1.898607] pstate: 80000005 (Nzcv daif -PAN -UAO BTYPE=--)
[    1.904126] pc : reset_control_reset+0x130/0x150
[    1.908700] lr : phy_meson_gxl_usb2_init+0x24/0x70
[    1.913439] sp : ffff8000123ebad0
[    1.916717] x29: ffff8000123ebad0 x28: 0000000000000000 
[    1.921978] x27: ffff00007c4b1ac8 x26: ffff00007c4b1ab0 
[    1.927239] x25: ffff00007fc149b0 x24: ffff00007c4b1ab0 
[    1.932500] x23: ffff00007cd03800 x22: ffff800011fb9000 
[    1.937761] x21: ffff00007c60db08 x20: ffff00007c468a80 
[    1.943023] x19: ffff00007c466b00 x18: ffffffffffffffff 
[    1.948284] x17: 0000000000000000 x16: 000000000000000e 
[    1.953545] x15: ffff800011fb9948 x14: ffffffffffffffff 
[    1.958806] x13: ffffffff00000000 x12: ffffffffffffffff 
[    1.964068] x11: 0000000000000020 x10: 7f7f7f7f7f7f7f7f 
[    1.969329] x9 : 78676f2c32617274 x8 : 0000000000000000 
[    1.974590] x7 : ffffffffffffffff x6 : 0000000000000000 
[    1.979851] x5 : 0000000000000000 x4 : 0000000000000000 
[    1.985112] x3 : 0000000000000000 x2 : ffff8000107aa370 
[    1.990374] x1 : 0000000000000001 x0 : 0000000000000001 
[    1.995636] Call trace:
[    1.998054]  reset_control_reset+0x130/0x150
[    2.002279]  phy_meson_gxl_usb2_init+0x24/0x70
[    2.006677]  phy_init+0x78/0xd0
[    2.009784]  dwc3_meson_g12a_probe+0x2c8/0x560
[    2.014182]  platform_drv_probe+0x58/0xa8
[    2.018149]  really_probe+0x114/0x3d8
[    2.021770]  driver_probe_device+0x5c/0xb8
[    2.025824]  __device_attach_driver+0x98/0xb8
[    2.030138]  bus_for_each_drv+0x74/0xd8
[    2.033933]  __device_attach+0xec/0x148
[    2.037726]  device_initial_probe+0x24/0x30
[    2.041868]  bus_probe_device+0x9c/0xa8
[    2.045663]  deferred_probe_work_func+0x7c/0xb8
[    2.050150]  process_one_work+0x1f0/0x4b0
[    2.054115]  worker_thread+0x210/0x480
[    2.057824]  kthread+0x158/0x160
[    2.061017]  ret_from_fork+0x10/0x18
[    2.064550] ---[ end trace 94d737efe593c6f6 ]---
[    2.069158] phy phy-d0078000.phy.0: phy init failed --> -22
[    2.074858] dwc3-meson-g12a: probe of d0078080.usb failed with error -22

This breaks USB on this device. reverting the change brings it back.

Looking at the reset framework code, I don't think drivers sharing the
same reset line should mix using reset_control_reset() VS
reset_control_assert()/reset_control_deassert()

>
> 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);

The change introduced here is significant. If the reset is not initially
asserted, it never will be before the life of the device.

This means that Linux will have to deal which whatever state is left by the
bootloader. This looks sketchy ...

I think the safer way solve the problem here would be to keep using
reset_control_reset() and introduce a new API in the reset
framework to decrement the reset line "triggered_count"
(reset_control_clear() ??)

That way, if all device using the reset line go to suspend, the line will
be "reset-able" again by the first device coming out of suspend ?

Philip, would you be ok with such new API ?

In the meantime, I think this change should be reverted. A warning on
suspend seems less critical than a regression breaking USB completly.

>  	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


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Kevin Hilman]

Jerome Brunet <jbrunet@baylibre.com> writes:

> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
>
>> 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:
>
> And now the same type of warning maybe seen on boot. This is
> happening for me on the libretech-cc (s905x - gxl).
>
> [    1.863469] ------------[ cut here ]------------
> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
> [    1.876525] Modules linked in:
> [    1.879548] CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.9.0-rc1-00167-ga2e4e3a34775 #64
> [    1.887737] Hardware name: Libre Computer AML-S905X-CC V2 (DT)
> [    1.893525] Workqueue: events deferred_probe_work_func
> [    1.898607] pstate: 80000005 (Nzcv daif -PAN -UAO BTYPE=--)
> [    1.904126] pc : reset_control_reset+0x130/0x150
> [    1.908700] lr : phy_meson_gxl_usb2_init+0x24/0x70
> [    1.913439] sp : ffff8000123ebad0
> [    1.916717] x29: ffff8000123ebad0 x28: 0000000000000000 
> [    1.921978] x27: ffff00007c4b1ac8 x26: ffff00007c4b1ab0 
> [    1.927239] x25: ffff00007fc149b0 x24: ffff00007c4b1ab0 
> [    1.932500] x23: ffff00007cd03800 x22: ffff800011fb9000 
> [    1.937761] x21: ffff00007c60db08 x20: ffff00007c468a80 
> [    1.943023] x19: ffff00007c466b00 x18: ffffffffffffffff 
> [    1.948284] x17: 0000000000000000 x16: 000000000000000e 
> [    1.953545] x15: ffff800011fb9948 x14: ffffffffffffffff 
> [    1.958806] x13: ffffffff00000000 x12: ffffffffffffffff 
> [    1.964068] x11: 0000000000000020 x10: 7f7f7f7f7f7f7f7f 
> [    1.969329] x9 : 78676f2c32617274 x8 : 0000000000000000 
> [    1.974590] x7 : ffffffffffffffff x6 : 0000000000000000 
> [    1.979851] x5 : 0000000000000000 x4 : 0000000000000000 
> [    1.985112] x3 : 0000000000000000 x2 : ffff8000107aa370 
> [    1.990374] x1 : 0000000000000001 x0 : 0000000000000001 
> [    1.995636] Call trace:
> [    1.998054]  reset_control_reset+0x130/0x150
> [    2.002279]  phy_meson_gxl_usb2_init+0x24/0x70
> [    2.006677]  phy_init+0x78/0xd0
> [    2.009784]  dwc3_meson_g12a_probe+0x2c8/0x560
> [    2.014182]  platform_drv_probe+0x58/0xa8
> [    2.018149]  really_probe+0x114/0x3d8
> [    2.021770]  driver_probe_device+0x5c/0xb8
> [    2.025824]  __device_attach_driver+0x98/0xb8
> [    2.030138]  bus_for_each_drv+0x74/0xd8
> [    2.033933]  __device_attach+0xec/0x148
> [    2.037726]  device_initial_probe+0x24/0x30
> [    2.041868]  bus_probe_device+0x9c/0xa8
> [    2.045663]  deferred_probe_work_func+0x7c/0xb8
> [    2.050150]  process_one_work+0x1f0/0x4b0
> [    2.054115]  worker_thread+0x210/0x480
> [    2.057824]  kthread+0x158/0x160
> [    2.061017]  ret_from_fork+0x10/0x18
> [    2.064550] ---[ end trace 94d737efe593c6f6 ]---
> [    2.069158] phy phy-d0078000.phy.0: phy init failed --> -22
> [    2.074858] dwc3-meson-g12a: probe of d0078080.usb failed with error -22
>
> This breaks USB on this device. reverting the change brings it back.

[...]

> In the meantime, I think this change should be reverted. A warning on
> suspend seems less critical than a regression breaking USB completly.

Fully agree.

Phillip, can you please revert this patch until the right solution is
found?  Boot failure is much worse than a suspend warning.

Kevin

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Thu 20 Aug 2020 at 20:02, Kevin Hilman <khilman@baylibre.com> wrote:

> Jerome Brunet <jbrunet@baylibre.com> writes:
>
>> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
>>
>>> 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:
>>
>> And now the same type of warning maybe seen on boot. This is
>> happening for me on the libretech-cc (s905x - gxl).
>>
>> [    1.863469] ------------[ cut here ]------------
>> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
>> [    1.876525] Modules linked in:
>> [    1.879548] CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.9.0-rc1-00167-ga2e4e3a34775 #64
>> [    1.887737] Hardware name: Libre Computer AML-S905X-CC V2 (DT)
>> [    1.893525] Workqueue: events deferred_probe_work_func
>> [    1.898607] pstate: 80000005 (Nzcv daif -PAN -UAO BTYPE=--)
>> [    1.904126] pc : reset_control_reset+0x130/0x150
>> [    1.908700] lr : phy_meson_gxl_usb2_init+0x24/0x70
>> [    1.913439] sp : ffff8000123ebad0
>> [    1.916717] x29: ffff8000123ebad0 x28: 0000000000000000 
>> [    1.921978] x27: ffff00007c4b1ac8 x26: ffff00007c4b1ab0 
>> [    1.927239] x25: ffff00007fc149b0 x24: ffff00007c4b1ab0 
>> [    1.932500] x23: ffff00007cd03800 x22: ffff800011fb9000 
>> [    1.937761] x21: ffff00007c60db08 x20: ffff00007c468a80 
>> [    1.943023] x19: ffff00007c466b00 x18: ffffffffffffffff 
>> [    1.948284] x17: 0000000000000000 x16: 000000000000000e 
>> [    1.953545] x15: ffff800011fb9948 x14: ffffffffffffffff 
>> [    1.958806] x13: ffffffff00000000 x12: ffffffffffffffff 
>> [    1.964068] x11: 0000000000000020 x10: 7f7f7f7f7f7f7f7f 
>> [    1.969329] x9 : 78676f2c32617274 x8 : 0000000000000000 
>> [    1.974590] x7 : ffffffffffffffff x6 : 0000000000000000 
>> [    1.979851] x5 : 0000000000000000 x4 : 0000000000000000 
>> [    1.985112] x3 : 0000000000000000 x2 : ffff8000107aa370 
>> [    1.990374] x1 : 0000000000000001 x0 : 0000000000000001 
>> [    1.995636] Call trace:
>> [    1.998054]  reset_control_reset+0x130/0x150
>> [    2.002279]  phy_meson_gxl_usb2_init+0x24/0x70
>> [    2.006677]  phy_init+0x78/0xd0
>> [    2.009784]  dwc3_meson_g12a_probe+0x2c8/0x560
>> [    2.014182]  platform_drv_probe+0x58/0xa8
>> [    2.018149]  really_probe+0x114/0x3d8
>> [    2.021770]  driver_probe_device+0x5c/0xb8
>> [    2.025824]  __device_attach_driver+0x98/0xb8
>> [    2.030138]  bus_for_each_drv+0x74/0xd8
>> [    2.033933]  __device_attach+0xec/0x148
>> [    2.037726]  device_initial_probe+0x24/0x30
>> [    2.041868]  bus_probe_device+0x9c/0xa8
>> [    2.045663]  deferred_probe_work_func+0x7c/0xb8
>> [    2.050150]  process_one_work+0x1f0/0x4b0
>> [    2.054115]  worker_thread+0x210/0x480
>> [    2.057824]  kthread+0x158/0x160
>> [    2.061017]  ret_from_fork+0x10/0x18
>> [    2.064550] ---[ end trace 94d737efe593c6f6 ]---
>> [    2.069158] phy phy-d0078000.phy.0: phy init failed --> -22
>> [    2.074858] dwc3-meson-g12a: probe of d0078080.usb failed with error -22
>>
>> This breaks USB on this device. reverting the change brings it back.
>
> [...]
>
>> In the meantime, I think this change should be reverted. A warning on
>> suspend seems less critical than a regression breaking USB completly.
>
> Fully agree.
>
> Phillip, can you please revert this patch until the right solution is
> found?  Boot failure is much worse than a suspend warning.

I was asking Philip about the reset API suggestion.
I think the maintainer is Felipe for this change :)

>
> Kevin


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Kevin Hilman]

Jerome Brunet <jbrunet@baylibre.com> writes:

> On Thu 20 Aug 2020 at 20:02, Kevin Hilman <khilman@baylibre.com> wrote:
>
>> Jerome Brunet <jbrunet@baylibre.com> writes:
>>
>>> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
>>>
>>>> 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:
>>>
>>> And now the same type of warning maybe seen on boot. This is
>>> happening for me on the libretech-cc (s905x - gxl).
>>>
>>> [    1.863469] ------------[ cut here ]------------
>>> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
>>> [    1.876525] Modules linked in:
>>> [    1.879548] CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.9.0-rc1-00167-ga2e4e3a34775 #64
>>> [    1.887737] Hardware name: Libre Computer AML-S905X-CC V2 (DT)
>>> [    1.893525] Workqueue: events deferred_probe_work_func
>>> [    1.898607] pstate: 80000005 (Nzcv daif -PAN -UAO BTYPE=--)
>>> [    1.904126] pc : reset_control_reset+0x130/0x150
>>> [    1.908700] lr : phy_meson_gxl_usb2_init+0x24/0x70
>>> [    1.913439] sp : ffff8000123ebad0
>>> [    1.916717] x29: ffff8000123ebad0 x28: 0000000000000000 
>>> [    1.921978] x27: ffff00007c4b1ac8 x26: ffff00007c4b1ab0 
>>> [    1.927239] x25: ffff00007fc149b0 x24: ffff00007c4b1ab0 
>>> [    1.932500] x23: ffff00007cd03800 x22: ffff800011fb9000 
>>> [    1.937761] x21: ffff00007c60db08 x20: ffff00007c468a80 
>>> [    1.943023] x19: ffff00007c466b00 x18: ffffffffffffffff 
>>> [    1.948284] x17: 0000000000000000 x16: 000000000000000e 
>>> [    1.953545] x15: ffff800011fb9948 x14: ffffffffffffffff 
>>> [    1.958806] x13: ffffffff00000000 x12: ffffffffffffffff 
>>> [    1.964068] x11: 0000000000000020 x10: 7f7f7f7f7f7f7f7f 
>>> [    1.969329] x9 : 78676f2c32617274 x8 : 0000000000000000 
>>> [    1.974590] x7 : ffffffffffffffff x6 : 0000000000000000 
>>> [    1.979851] x5 : 0000000000000000 x4 : 0000000000000000 
>>> [    1.985112] x3 : 0000000000000000 x2 : ffff8000107aa370 
>>> [    1.990374] x1 : 0000000000000001 x0 : 0000000000000001 
>>> [    1.995636] Call trace:
>>> [    1.998054]  reset_control_reset+0x130/0x150
>>> [    2.002279]  phy_meson_gxl_usb2_init+0x24/0x70
>>> [    2.006677]  phy_init+0x78/0xd0
>>> [    2.009784]  dwc3_meson_g12a_probe+0x2c8/0x560
>>> [    2.014182]  platform_drv_probe+0x58/0xa8
>>> [    2.018149]  really_probe+0x114/0x3d8
>>> [    2.021770]  driver_probe_device+0x5c/0xb8
>>> [    2.025824]  __device_attach_driver+0x98/0xb8
>>> [    2.030138]  bus_for_each_drv+0x74/0xd8
>>> [    2.033933]  __device_attach+0xec/0x148
>>> [    2.037726]  device_initial_probe+0x24/0x30
>>> [    2.041868]  bus_probe_device+0x9c/0xa8
>>> [    2.045663]  deferred_probe_work_func+0x7c/0xb8
>>> [    2.050150]  process_one_work+0x1f0/0x4b0
>>> [    2.054115]  worker_thread+0x210/0x480
>>> [    2.057824]  kthread+0x158/0x160
>>> [    2.061017]  ret_from_fork+0x10/0x18
>>> [    2.064550] ---[ end trace 94d737efe593c6f6 ]---
>>> [    2.069158] phy phy-d0078000.phy.0: phy init failed --> -22
>>> [    2.074858] dwc3-meson-g12a: probe of d0078080.usb failed with error -22
>>>
>>> This breaks USB on this device. reverting the change brings it back.
>>
>> [...]
>>
>>> In the meantime, I think this change should be reverted. A warning on
>>> suspend seems less critical than a regression breaking USB completly.
>>
>> Fully agree.
>>
>> Phillip, can you please revert this patch until the right solution is
>> found?  Boot failure is much worse than a suspend warning.
>
> I was asking Philip about the reset API suggestion.
> I think the maintainer is Felipe for this change :)

Oops, yeah.  I meant Felipe too.  Names are close enough for my brain to
mix up.

@Felipe: can you revert this change please?

Kevin



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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson]

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

On Wed, Aug 19, 2020 at 05:03:32PM +0200, Jerome Brunet wrote:
> 
> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
> 
> > 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:
> 
> And now the same type of warning maybe seen on boot. This is
> happening for me on the libretech-cc (s905x - gxl).
> 
> [    1.863469] ------------[ cut here ]------------
> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
> [    1.876525] Modules linked in:
> [    1.879548] CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.9.0-rc1-00167-ga2e4e3a34775 #64
> [    1.887737] Hardware name: Libre Computer AML-S905X-CC V2 (DT)
> [    1.893525] Workqueue: events deferred_probe_work_func
> [    1.898607] pstate: 80000005 (Nzcv daif -PAN -UAO BTYPE=--)
> [    1.904126] pc : reset_control_reset+0x130/0x150
> [    1.908700] lr : phy_meson_gxl_usb2_init+0x24/0x70
> [    1.913439] sp : ffff8000123ebad0
> [    1.916717] x29: ffff8000123ebad0 x28: 0000000000000000 
> [    1.921978] x27: ffff00007c4b1ac8 x26: ffff00007c4b1ab0 
> [    1.927239] x25: ffff00007fc149b0 x24: ffff00007c4b1ab0 
> [    1.932500] x23: ffff00007cd03800 x22: ffff800011fb9000 
> [    1.937761] x21: ffff00007c60db08 x20: ffff00007c468a80 
> [    1.943023] x19: ffff00007c466b00 x18: ffffffffffffffff 
> [    1.948284] x17: 0000000000000000 x16: 000000000000000e 
> [    1.953545] x15: ffff800011fb9948 x14: ffffffffffffffff 
> [    1.958806] x13: ffffffff00000000 x12: ffffffffffffffff 
> [    1.964068] x11: 0000000000000020 x10: 7f7f7f7f7f7f7f7f 
> [    1.969329] x9 : 78676f2c32617274 x8 : 0000000000000000 
> [    1.974590] x7 : ffffffffffffffff x6 : 0000000000000000 
> [    1.979851] x5 : 0000000000000000 x4 : 0000000000000000 
> [    1.985112] x3 : 0000000000000000 x2 : ffff8000107aa370 
> [    1.990374] x1 : 0000000000000001 x0 : 0000000000000001 
> [    1.995636] Call trace:
> [    1.998054]  reset_control_reset+0x130/0x150
> [    2.002279]  phy_meson_gxl_usb2_init+0x24/0x70
> [    2.006677]  phy_init+0x78/0xd0
> [    2.009784]  dwc3_meson_g12a_probe+0x2c8/0x560
> [    2.014182]  platform_drv_probe+0x58/0xa8
> [    2.018149]  really_probe+0x114/0x3d8
> [    2.021770]  driver_probe_device+0x5c/0xb8
> [    2.025824]  __device_attach_driver+0x98/0xb8
> [    2.030138]  bus_for_each_drv+0x74/0xd8
> [    2.033933]  __device_attach+0xec/0x148
> [    2.037726]  device_initial_probe+0x24/0x30
> [    2.041868]  bus_probe_device+0x9c/0xa8
> [    2.045663]  deferred_probe_work_func+0x7c/0xb8
> [    2.050150]  process_one_work+0x1f0/0x4b0
> [    2.054115]  worker_thread+0x210/0x480
> [    2.057824]  kthread+0x158/0x160
> [    2.061017]  ret_from_fork+0x10/0x18
> [    2.064550] ---[ end trace 94d737efe593c6f6 ]---
> [    2.069158] phy phy-d0078000.phy.0: phy init failed --> -22
> [    2.074858] dwc3-meson-g12a: probe of d0078080.usb failed with error -22
> 
> This breaks USB on this device. reverting the change brings it back.
> 
> Looking at the reset framework code, I don't think drivers sharing the
> same reset line should mix using reset_control_reset() VS
> reset_control_assert()/reset_control_deassert()
> 

Thanks for finding this. I was only able to test on the Odroid N2

> >
> > 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);
> 
> The change introduced here is significant. If the reset is not initially
> asserted, it never will be before the life of the device.
> 
> This means that Linux will have to deal which whatever state is left by the
> bootloader. This looks sketchy ...
> 
> I think the safer way solve the problem here would be to keep using
> reset_control_reset() and introduce a new API in the reset
> framework to decrement the reset line "triggered_count"
> (reset_control_clear() ??)

I have very limited experience with reset controls, but phy_meson_gxl_usb2_init
calls reset_control_reset on a shared reset line, which should not be done
according to the reset control docs. Would removing the use of reset_control_reset,
or using reset_control_(de)assert in phy_meson_gxl_usb2_init also resolve the
issue?

> That way, if all device using the reset line go to suspend, the line will
> be "reset-able" again by the first device coming out of suspend ?
> 
> Philip, would you be ok with such new API ?
> 
> In the meantime, I think this change should be reverted. A warning on
> suspend seems less critical than a regression breaking USB completly.

The reset_control_(de)assert() calls could also be removed from the
suspend/resume calls for now.

> >  	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
> 

[-- 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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Thu 20 Aug 2020 at 20:44, Dan Robertson <dan@dlrobertson.com> wrote:

> On Wed, Aug 19, 2020 at 05:03:32PM +0200, Jerome Brunet wrote:
>> 
>> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
>> 
>> > 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:
>> 
>> And now the same type of warning maybe seen on boot. This is
>> happening for me on the libretech-cc (s905x - gxl).
>> 
>> [    1.863469] ------------[ cut here ]------------
>> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
>> [    1.876525] Modules linked in:
>> [    1.879548] CPU: 1 PID: 16 Comm: kworker/1:0 Not tainted 5.9.0-rc1-00167-ga2e4e3a34775 #64
>> [    1.887737] Hardware name: Libre Computer AML-S905X-CC V2 (DT)
>> [    1.893525] Workqueue: events deferred_probe_work_func
>> [    1.898607] pstate: 80000005 (Nzcv daif -PAN -UAO BTYPE=--)
>> [    1.904126] pc : reset_control_reset+0x130/0x150
>> [    1.908700] lr : phy_meson_gxl_usb2_init+0x24/0x70
>> [    1.913439] sp : ffff8000123ebad0
>> [    1.916717] x29: ffff8000123ebad0 x28: 0000000000000000 
>> [    1.921978] x27: ffff00007c4b1ac8 x26: ffff00007c4b1ab0 
>> [    1.927239] x25: ffff00007fc149b0 x24: ffff00007c4b1ab0 
>> [    1.932500] x23: ffff00007cd03800 x22: ffff800011fb9000 
>> [    1.937761] x21: ffff00007c60db08 x20: ffff00007c468a80 
>> [    1.943023] x19: ffff00007c466b00 x18: ffffffffffffffff 
>> [    1.948284] x17: 0000000000000000 x16: 000000000000000e 
>> [    1.953545] x15: ffff800011fb9948 x14: ffffffffffffffff 
>> [    1.958806] x13: ffffffff00000000 x12: ffffffffffffffff 
>> [    1.964068] x11: 0000000000000020 x10: 7f7f7f7f7f7f7f7f 
>> [    1.969329] x9 : 78676f2c32617274 x8 : 0000000000000000 
>> [    1.974590] x7 : ffffffffffffffff x6 : 0000000000000000 
>> [    1.979851] x5 : 0000000000000000 x4 : 0000000000000000 
>> [    1.985112] x3 : 0000000000000000 x2 : ffff8000107aa370 
>> [    1.990374] x1 : 0000000000000001 x0 : 0000000000000001 
>> [    1.995636] Call trace:
>> [    1.998054]  reset_control_reset+0x130/0x150
>> [    2.002279]  phy_meson_gxl_usb2_init+0x24/0x70
>> [    2.006677]  phy_init+0x78/0xd0
>> [    2.009784]  dwc3_meson_g12a_probe+0x2c8/0x560
>> [    2.014182]  platform_drv_probe+0x58/0xa8
>> [    2.018149]  really_probe+0x114/0x3d8
>> [    2.021770]  driver_probe_device+0x5c/0xb8
>> [    2.025824]  __device_attach_driver+0x98/0xb8
>> [    2.030138]  bus_for_each_drv+0x74/0xd8
>> [    2.033933]  __device_attach+0xec/0x148
>> [    2.037726]  device_initial_probe+0x24/0x30
>> [    2.041868]  bus_probe_device+0x9c/0xa8
>> [    2.045663]  deferred_probe_work_func+0x7c/0xb8
>> [    2.050150]  process_one_work+0x1f0/0x4b0
>> [    2.054115]  worker_thread+0x210/0x480
>> [    2.057824]  kthread+0x158/0x160
>> [    2.061017]  ret_from_fork+0x10/0x18
>> [    2.064550] ---[ end trace 94d737efe593c6f6 ]---
>> [    2.069158] phy phy-d0078000.phy.0: phy init failed --> -22
>> [    2.074858] dwc3-meson-g12a: probe of d0078080.usb failed with error -22
>> 
>> This breaks USB on this device. reverting the change brings it back.
>> 
>> Looking at the reset framework code, I don't think drivers sharing the
>> same reset line should mix using reset_control_reset() VS
>> reset_control_assert()/reset_control_deassert()
>> 
>
> Thanks for finding this. I was only able to test on the Odroid N2
>
>> >
>> > 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);
>> 
>> The change introduced here is significant. If the reset is not initially
>> asserted, it never will be before the life of the device.
>> 
>> This means that Linux will have to deal which whatever state is left by the
>> bootloader. This looks sketchy ...
>> 
>> I think the safer way solve the problem here would be to keep using
>> reset_control_reset() and introduce a new API in the reset
>> framework to decrement the reset line "triggered_count"
>> (reset_control_clear() ??)
>
> I have very limited experience with reset controls, but phy_meson_gxl_usb2_init
> calls reset_control_reset on a shared reset line, which should not be done
> according to the reset control docs. Would removing the use of reset_control_reset,
> or using reset_control_(de)assert in phy_meson_gxl_usb2_init also resolve the
> issue?

 As pointed out before, using the deassert() in probe does not provide
 any guarantee that a reset will actually be trigerred on the
 device. Reset are deasserted on boot on these chips so it actually
 likely to never be triggered.
 This defeats the purpose of using resets in our case.

>
>> That way, if all device using the reset line go to suspend, the line will
>> be "reset-able" again by the first device coming out of suspend ?
>> 
>> Philip, would you be ok with such new API ?
>> 
>> In the meantime, I think this change should be reverted. A warning on
>> suspend seems less critical than a regression breaking USB completly.
>
> The reset_control_(de)assert() calls could also be removed from the
> suspend/resume calls for now.

If it is not causing issue, why not.

>
>> >  	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
>> 


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Philipp Zabel]

Hi Jerome,

On Wed, 2020-08-19 at 17:03 +0200, Jerome Brunet wrote:
> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
> 
> > 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:
> 
> And now the same type of warning maybe seen on boot. This is
> happening for me on the libretech-cc (s905x - gxl).
> 
> [    1.863469] ------------[ cut here ]------------
> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
[...]
> This breaks USB on this device. reverting the change brings it back.
> 
> Looking at the reset framework code, I don't think drivers sharing the
> same reset line should mix using reset_control_reset() VS
> reset_control_assert()/reset_control_deassert()

That is correct, users must not mix the assert/deassert and reset calls
on shared resets:

/**
 * reset_control_reset - reset the controlled device
 * @rstc: reset controller
 *
 * On a shared reset line the actual reset pulse is only triggered once for the
 * lifetime of the reset_control instance: for all but the first caller this is
 * a no-op.
 * Consumers must not use reset_control_(de)assert on shared reset lines when
 * reset_control_reset has been used.
 *
 * If rstc is NULL it is an optional reset and the function will just
 * return 0.
 */

[...]
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);
> 
> The change introduced here is significant. If the reset is not initially
> asserted, it never will be before the life of the device.
>
> This means that Linux will have to deal which whatever state is left by the
> bootloader. This looks sketchy ...
>
> I think the safer way solve the problem here would be to keep using
> reset_control_reset() and introduce a new API in the reset
> framework to decrement the reset line "triggered_count"
> (reset_control_clear() ??)
> 
> That way, if all device using the reset line go to suspend, the line will
> be "reset-able" again by the first device coming out of suspend ?
>
> Philip, would you be ok with such new API ?

I'd like to first evaluate whether the already available APIs might be a
better fit. There is already the option of handing off exclusive control
between multiple drivers via the reset_control_acquire/release APIs on
exclusive reset controls.

If all drivers that are now sharing the reset line would switch to
requesting resets via devm_reset_control_get_exclusive_released()
and then prepend their reset handling with reset_control_acquire() (but
ignore -EBUSY) and the driver that got exclusive control releases the
reset via reset_control_release() during suspend, this should do exactly
what you want. Note that reset_control_release() must not be called on a
reset control that has not been successfully acquired by the same
driver.

Is this something that would be feasible for your combination of
drivers? Otherwise it is be unclear to me under which condition a driver
should be allowed to call the proposed reset_control_clear().

> In the meantime, I think this change should be reverted. A warning on
> suspend seems less critical than a regression breaking USB completly.

Agreed.

regards
Philipp

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Mon 24 Aug 2020 at 12:24, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> Hi Jerome,
>
> On Wed, 2020-08-19 at 17:03 +0200, Jerome Brunet wrote:
>> On Mon 13 Jul 2020 at 18:05, Dan Robertson <dan@dlrobertson.com> wrote:
>> 
>> > 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:
>> 
>> And now the same type of warning maybe seen on boot. This is
>> happening for me on the libretech-cc (s905x - gxl).
>> 
>> [    1.863469] ------------[ cut here ]------------
>> [    1.867914] WARNING: CPU: 1 PID: 16 at drivers/reset/core.c:309 reset_control_reset+0x130/0x150
> [...]
>> This breaks USB on this device. reverting the change brings it back.
>> 
>> Looking at the reset framework code, I don't think drivers sharing the
>> same reset line should mix using reset_control_reset() VS
>> reset_control_assert()/reset_control_deassert()
>
> That is correct, users must not mix the assert/deassert and reset calls
> on shared resets:
>
> /**
>  * reset_control_reset - reset the controlled device
>  * @rstc: reset controller
>  *
>  * On a shared reset line the actual reset pulse is only triggered once for the
>  * lifetime of the reset_control instance: for all but the first caller this is
>  * a no-op.
>  * Consumers must not use reset_control_(de)assert on shared reset lines when
>  * reset_control_reset has been used.
>  *
>  * If rstc is NULL it is an optional reset and the function will just
>  * return 0.
>  */
>
> [...]
> 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);
>> 
>> The change introduced here is significant. If the reset is not initially
>> asserted, it never will be before the life of the device.
>>
>> This means that Linux will have to deal which whatever state is left by the
>> bootloader. This looks sketchy ...
>>
>> I think the safer way solve the problem here would be to keep using
>> reset_control_reset() and introduce a new API in the reset
>> framework to decrement the reset line "triggered_count"
>> (reset_control_clear() ??)
>> 
>> That way, if all device using the reset line go to suspend, the line will
>> be "reset-able" again by the first device coming out of suspend ?
>>
>> Philip, would you be ok with such new API ?
>
> I'd like to first evaluate whether the already available APIs might be a
> better fit. There is already the option of handing off exclusive control
> between multiple drivers via the reset_control_acquire/release APIs on
> exclusive reset controls.
>
> If all drivers that are now sharing the reset line would switch to
> requesting resets via devm_reset_control_get_exclusive_released()
> and then prepend their reset handling with reset_control_acquire() (but
> ignore -EBUSY) and the driver that got exclusive control releases the
> reset via reset_control_release() during suspend, this should do exactly
> what you want. Note that reset_control_release() must not be called on a
> reset control that has not been successfully acquired by the same
> driver.

In practice, I think your proposition would work since the drivers
sharing this USB reset line are likely to be probed/suspended/resumed at
the same time but ...

If we imagine a situation where 2 device share a reset line, 1 go in
suspend, the other does not - if the first device as control of the
reset, it could trigger it and break the 2nd device. Same goes for
probe/remove()

I agree it could be seen as unlikely but leaving such race condition
open looks dangerous to me.

>
> Is this something that would be feasible for your combination of
> drivers? Otherwise it is be unclear to me under which condition a driver
> should be allowed to call the proposed reset_control_clear().

I was thinking of reset_control_clear() as the counter part of
reset_control_reset().

When a reset_control_reset() has been called once, "triggered_count" is
incremented which signals that the ressource under the reset is
"in_use" and the reset should not be done again. reset_control_clear()
would be the way to state that the ressource is no longer used and, that
from the caller perspective, the reset can fired again if necessary.

If we take the probe / suspend / resume example:
* 1st device using the shared will actually trigger it (as it is now)
* following device just increase triggered_count

If all devices go to suspend (calling reset_control_clear()) then
triggered_count will reach zero, allowing the first device resuming to
trigger the reset again ... this is important since it might not be the
one which would have got the exclusive control

If any device don't go to suspend, meaning the ressource under reset
keep on being used, no reset will performed. With exlusive control,
there is a risk that the resuming device resets something already in use.

Regarding the condition, on shared resets, call reset_control_reset()
should be balanced reset_control_clear() - no clear before reset.

>
>> In the meantime, I think this change should be reverted. A warning on
>> suspend seems less critical than a regression breaking USB completly.
>
> Agreed.
>
> regards
> Philipp


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Philipp Zabel]

On Mon, 2020-08-24 at 16:26 +0200, Jerome Brunet wrote:
[...]
> In practice, I think your proposition would work since the drivers
> sharing this USB reset line are likely to be probed/suspended/resumed at
> the same time but ...
> 
> If we imagine a situation where 2 device share a reset line, 1 go in
> suspend, the other does not - if the first device as control of the
> reset, it could trigger it and break the 2nd device. Same goes for
> probe/remove()
> 
> I agree it could be seen as unlikely but leaving such race condition
> open looks dangerous to me.

You are right, this is not good enough.

> > Is this something that would be feasible for your combination of
> > drivers? Otherwise it is be unclear to me under which condition a driver
> > should be allowed to call the proposed reset_control_clear().
> 
> I was thinking of reset_control_clear() as the counter part of
> reset_control_reset().

I'm not particularly fond of reset_control_clear as a name, because it
is very close to "clearing a reset bit" which usually would deassert a
reset line (or the inverse).

> When a reset_control_reset() has been called once, "triggered_count" is
> incremented which signals that the ressource under the reset is
> "in_use" and the reset should not be done again.

"triggered_count" would then have to be renamed to something like
"trigger_requested_count", or "use_count". I wonder it might be possible
to merge this with "deassert_count" as they'd share the same semantics
(while the count is > 0, the reset line must stay deasserted).

> reset_control_clear()
> would be the way to state that the ressource is no longer used and, that
> from the caller perspective, the reset can fired again if necessary.
> 
> If we take the probe / suspend / resume example:
> * 1st device using the shared will actually trigger it (as it is now)
> * following device just increase triggered_count
> 
> If all devices go to suspend (calling reset_control_clear()) then
> triggered_count will reach zero, allowing the first device resuming to
> trigger the reset again ... this is important since it might not be the
> one which would have got the exclusive control
> 
> If any device don't go to suspend, meaning the ressource under reset
> keep on being used, no reset will performed. With exlusive control,
> there is a risk that the resuming device resets something already in use.
>
> Regarding the condition, on shared resets, call reset_control_reset()
> should be balanced reset_control_clear() - no clear before reset.

Martin, is this something that would be useful for the current users of
the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
meson8b-usb2 with reset-meson)?

regards
Philipp

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Tue 25 Aug 2020 at 12:20, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> On Mon, 2020-08-24 at 16:26 +0200, Jerome Brunet wrote:
> [...]
>> In practice, I think your proposition would work since the drivers
>> sharing this USB reset line are likely to be probed/suspended/resumed at
>> the same time but ...
>> 
>> If we imagine a situation where 2 device share a reset line, 1 go in
>> suspend, the other does not - if the first device as control of the
>> reset, it could trigger it and break the 2nd device. Same goes for
>> probe/remove()
>> 
>> I agree it could be seen as unlikely but leaving such race condition
>> open looks dangerous to me.
>
> You are right, this is not good enough.
>
>> > Is this something that would be feasible for your combination of
>> > drivers? Otherwise it is be unclear to me under which condition a driver
>> > should be allowed to call the proposed reset_control_clear().
>> 
>> I was thinking of reset_control_clear() as the counter part of
>> reset_control_reset().
>
> I'm not particularly fond of reset_control_clear as a name, because it
> is very close to "clearing a reset bit" which usually would deassert a
> reset line (or the inverse).

It was merely a suggestion :) any other name you prefer is fine by me

>
>> When a reset_control_reset() has been called once, "triggered_count" is
>> incremented which signals that the ressource under the reset is
>> "in_use" and the reset should not be done again.
>
> "triggered_count" would then have to be renamed to something like
> "trigger_requested_count", or "use_count". I wonder it might be possible
> to merge this with "deassert_count" as they'd share the same semantics
> (while the count is > 0, the reset line must stay deasserted).

Sure. Could investigate this as a 2nd step ?
I'd like to bring a solution for our meson-usb use case quickly - even
with the revert suggested, we are having an ugly warning around suspend

>
>> reset_control_clear()
>> would be the way to state that the ressource is no longer used and, that
>> from the caller perspective, the reset can fired again if necessary.
>> 
>> If we take the probe / suspend / resume example:
>> * 1st device using the shared will actually trigger it (as it is now)
>> * following device just increase triggered_count
>> 
>> If all devices go to suspend (calling reset_control_clear()) then
>> triggered_count will reach zero, allowing the first device resuming to
>> trigger the reset again ... this is important since it might not be the
>> one which would have got the exclusive control
>> 
>> If any device don't go to suspend, meaning the ressource under reset
>> keep on being used, no reset will performed. With exlusive control,
>> there is a risk that the resuming device resets something already in use.
>>
>> Regarding the condition, on shared resets, call reset_control_reset()
>> should be balanced reset_control_clear() - no clear before reset.
>
> Martin, is this something that would be useful for the current users of
> the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
> meson8b-usb2 with reset-meson)?

I'm not Martin but these devices are the origin of the request/suggestion.

>
> regards
> Philipp


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Philipp Zabel]

On Tue, 2020-08-25 at 16:20 +0200, Jerome Brunet wrote:
> On Tue 25 Aug 2020 at 12:20, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> 
> > On Mon, 2020-08-24 at 16:26 +0200, Jerome Brunet wrote:
> > [...]
> > > In practice, I think your proposition would work since the drivers
> > > sharing this USB reset line are likely to be probed/suspended/resumed at
> > > the same time but ...
> > > 
> > > If we imagine a situation where 2 device share a reset line, 1 go in
> > > suspend, the other does not - if the first device as control of the
> > > reset, it could trigger it and break the 2nd device. Same goes for
> > > probe/remove()
> > > 
> > > I agree it could be seen as unlikely but leaving such race condition
> > > open looks dangerous to me.
> > 
> > You are right, this is not good enough.
> > 
> > > > Is this something that would be feasible for your combination of
> > > > drivers? Otherwise it is be unclear to me under which condition a driver
> > > > should be allowed to call the proposed reset_control_clear().
> > > 
> > > I was thinking of reset_control_clear() as the counter part of
> > > reset_control_reset().
> > 
> > I'm not particularly fond of reset_control_clear as a name, because it
> > is very close to "clearing a reset bit" which usually would deassert a
> > reset line (or the inverse).
> 
> It was merely a suggestion :) any other name you prefer is fine by me

Naming is hard. All metaphors I can think of are either a obscure or
clash with other current usage. My instinct would be to call this
"resetting the (reset) control", but _reset() is already taken, with the
opposite meaning. How about _rewind() or _rearm()? Not sure if those are
good metaphors either, but at least there is no obvious but incorrect
interpretation. I kind of wish reset_control_reset() would be called
reset_control_trigger() instead.

> > > When a reset_control_reset() has been called once, "triggered_count" is
> > > incremented which signals that the ressource under the reset is
> > > "in_use" and the reset should not be done again.
> > 
> > "triggered_count" would then have to be renamed to something like
> > "trigger_requested_count", or "use_count". I wonder it might be possible
> > to merge this with "deassert_count" as they'd share the same semantics
> > (while the count is > 0, the reset line must stay deasserted).
> 
> Sure. Could investigate this as a 2nd step ?

Yes.

> I'd like to bring a solution for our meson-usb use case quickly - even
> with the revert suggested, we are having an ugly warning around suspend

I understand. Let's still do this carefully :)

> > > reset_control_clear()
> > > would be the way to state that the ressource is no longer used and, that
> > > from the caller perspective, the reset can fired again if necessary.
> > > 
> > > If we take the probe / suspend / resume example:
> > > * 1st device using the shared will actually trigger it (as it is now)
> > > * following device just increase triggered_count
> > > 
> > > If all devices go to suspend (calling reset_control_clear()) then
> > > triggered_count will reach zero, allowing the first device resuming to
> > > trigger the reset again ... this is important since it might not be the
> > > one which would have got the exclusive control
> > > 
> > > If any device don't go to suspend, meaning the ressource under reset
> > > keep on being used, no reset will performed. With exlusive control,
> > > there is a risk that the resuming device resets something already in use.
> > > 
> > > Regarding the condition, on shared resets, call reset_control_reset()
> > > should be balanced reset_control_clear() - no clear before reset.
> > 
> > Martin, is this something that would be useful for the current users of
> > the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
> > meson8b-usb2 with reset-meson)?
> 
> I'm not Martin but these devices are the origin of the request/suggestion.

So these two phy drivers are used together with dwc3-meson-g12a?
Will you change them to use the new API as well?

regards
Philipp

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Wed 26 Aug 2020 at 10:14, Philipp Zabel <p.zabel@pengutronix.de> wrote:

> On Tue, 2020-08-25 at 16:20 +0200, Jerome Brunet wrote:
>> On Tue 25 Aug 2020 at 12:20, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> 
>> > On Mon, 2020-08-24 at 16:26 +0200, Jerome Brunet wrote:
>> > [...]
>> > > In practice, I think your proposition would work since the drivers
>> > > sharing this USB reset line are likely to be probed/suspended/resumed at
>> > > the same time but ...
>> > > 
>> > > If we imagine a situation where 2 device share a reset line, 1 go in
>> > > suspend, the other does not - if the first device as control of the
>> > > reset, it could trigger it and break the 2nd device. Same goes for
>> > > probe/remove()
>> > > 
>> > > I agree it could be seen as unlikely but leaving such race condition
>> > > open looks dangerous to me.
>> > 
>> > You are right, this is not good enough.
>> > 
>> > > > Is this something that would be feasible for your combination of
>> > > > drivers? Otherwise it is be unclear to me under which condition a driver
>> > > > should be allowed to call the proposed reset_control_clear().
>> > > 
>> > > I was thinking of reset_control_clear() as the counter part of
>> > > reset_control_reset().
>> > 
>> > I'm not particularly fond of reset_control_clear as a name, because it
>> > is very close to "clearing a reset bit" which usually would deassert a
>> > reset line (or the inverse).
>> 
>> It was merely a suggestion :) any other name you prefer is fine by me
>
> Naming is hard. All metaphors I can think of are either a obscure or
> clash with other current usage. My instinct would be to call this
> "resetting the (reset) control", but _reset() is already taken, with the
> opposite meaning. How about _rewind() or _rearm()? Not sure if those are
> good metaphors either, but at least there is no obvious but incorrect
> interpretation. I kind of wish reset_control_reset() would be called
> reset_control_trigger() instead.

We'll pick something for the v1 ... maybe the inspiration will come
later on and we'll make a v2 ;)

>
>> > > When a reset_control_reset() has been called once, "triggered_count" is
>> > > incremented which signals that the ressource under the reset is
>> > > "in_use" and the reset should not be done again.
>> > 
>> > "triggered_count" would then have to be renamed to something like
>> > "trigger_requested_count", or "use_count". I wonder it might be possible
>> > to merge this with "deassert_count" as they'd share the same semantics
>> > (while the count is > 0, the reset line must stay deasserted).
>> 
>> Sure. Could investigate this as a 2nd step ?
>
> Yes.
>
>> I'd like to bring a solution for our meson-usb use case quickly - even
>> with the revert suggested, we are having an ugly warning around suspend
>
> I understand. Let's still do this carefully :)

will do

>
>> > > reset_control_clear()
>> > > would be the way to state that the ressource is no longer used and, that
>> > > from the caller perspective, the reset can fired again if necessary.
>> > > 
>> > > If we take the probe / suspend / resume example:
>> > > * 1st device using the shared will actually trigger it (as it is now)
>> > > * following device just increase triggered_count
>> > > 
>> > > If all devices go to suspend (calling reset_control_clear()) then
>> > > triggered_count will reach zero, allowing the first device resuming to
>> > > trigger the reset again ... this is important since it might not be the
>> > > one which would have got the exclusive control
>> > > 
>> > > If any device don't go to suspend, meaning the ressource under reset
>> > > keep on being used, no reset will performed. With exlusive control,
>> > > there is a risk that the resuming device resets something already in use.
>> > > 
>> > > Regarding the condition, on shared resets, call reset_control_reset()
>> > > should be balanced reset_control_clear() - no clear before reset.
>> > 
>> > Martin, is this something that would be useful for the current users of
>> > the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
>> > meson8b-usb2 with reset-meson)?
>> 
>> I'm not Martin but these devices are the origin of the request/suggestion.
>
> So these two phy drivers are used together with dwc3-meson-g12a?

Yes, reset is shared by the different usb devices of the SoCs

> Will you change them to use the new API as well?

That's the plan, yes.

>
> regards
> Philipp


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Martin Blumenstingl]

Hi Philipp,

On Tue, Aug 25, 2020 at 12:20 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
[...]
> > reset_control_clear()
> > would be the way to state that the ressource is no longer used and, that
> > from the caller perspective, the reset can fired again if necessary.
> >
> > If we take the probe / suspend / resume example:
> > * 1st device using the shared will actually trigger it (as it is now)
> > * following device just increase triggered_count
> >
> > If all devices go to suspend (calling reset_control_clear()) then
> > triggered_count will reach zero, allowing the first device resuming to
> > trigger the reset again ... this is important since it might not be the
> > one which would have got the exclusive control
> >
> > If any device don't go to suspend, meaning the ressource under reset
> > keep on being used, no reset will performed. With exlusive control,
> > there is a risk that the resuming device resets something already in use.
> >
> > Regarding the condition, on shared resets, call reset_control_reset()
> > should be balanced reset_control_clear() - no clear before reset.
>
> Martin, is this something that would be useful for the current users of
> the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
> meson8b-usb2 with reset-meson)?
for the specific use-case (system suspend) this would currently not be
useful for the two drivers you have named. This is because the
platforms on which they are used currently don't support system
suspend.
if other drivers are going to benefit from this change then please go
ahead and add the necessary API. Then I can also use it in these
drivers. however, (as far as I understand) I won't be able to verify
the new "fixed" behavior


Best regards,
Martin

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Amjad Ouled-Ameur]

Le sam. 29 août 2020 à 17:25, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> a écrit :
>
> Hi Philipp,
>
> On Tue, Aug 25, 2020 at 12:20 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> [...]
> > > reset_control_clear()
> > > would be the way to state that the ressource is no longer used and, that
> > > from the caller perspective, the reset can fired again if necessary.
> > >
> > > If we take the probe / suspend / resume example:
> > > * 1st device using the shared will actually trigger it (as it is now)
> > > * following device just increase triggered_count
> > >
> > > If all devices go to suspend (calling reset_control_clear()) then
> > > triggered_count will reach zero, allowing the first device resuming to
> > > trigger the reset again ... this is important since it might not be the
> > > one which would have got the exclusive control
> > >
> > > If any device don't go to suspend, meaning the ressource under reset
> > > keep on being used, no reset will performed. With exlusive control,
> > > there is a risk that the resuming device resets something already in use.
> > >
> > > Regarding the condition, on shared resets, call reset_control_reset()
> > > should be balanced reset_control_clear() - no clear before reset.
> >
> > Martin, is this something that would be useful for the current users of
> > the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
> > meson8b-usb2 with reset-meson)?
> for the specific use-case (system suspend) this would currently not be
> useful for the two drivers you have named. This is because the
> platforms on which they are used currently don't support system
> suspend.
> if other drivers are going to benefit from this change then please go
> ahead and add the necessary API. Then I can also use it in these
> drivers. however, (as far as I understand) I won't be able to verify
> the new "fixed" behavior
>
>
> Best regards,
> Martin

Hi Philipp,

Regarding the naming of the new call, since reset_control_clear() is not
really representative of the intended behaviour, I have thought of some
other metaphors such as:

reset_control_resettable()    (sounds the most relevant to me)
reset_control_standby()
reset_control_unseal()
reset_control_untie()
reset_control_loosen()/loose()
reset_control_unfetter()

What do you think?

Regards,
Amjad


-- 
Amjad Ouled-Ameur
Embedded Linux Engineer - Villeneuve Loubet, FR
Engit@BayLibre - At the Heart of Embedded Linux

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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Jerome Brunet]

On Wed 02 Sep 2020 at 16:13, Amjad Ouled-Ameur <aouledameur@baylibre.com> wrote:

> Le sam. 29 août 2020 à 17:25, Martin Blumenstingl
> <martin.blumenstingl@googlemail.com> a écrit :
>>
>> Hi Philipp,
>>
>> On Tue, Aug 25, 2020 at 12:20 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> [...]
>> > > reset_control_clear()
>> > > would be the way to state that the ressource is no longer used and, that
>> > > from the caller perspective, the reset can fired again if necessary.
>> > >
>> > > If we take the probe / suspend / resume example:
>> > > * 1st device using the shared will actually trigger it (as it is now)
>> > > * following device just increase triggered_count
>> > >
>> > > If all devices go to suspend (calling reset_control_clear()) then
>> > > triggered_count will reach zero, allowing the first device resuming to
>> > > trigger the reset again ... this is important since it might not be the
>> > > one which would have got the exclusive control
>> > >
>> > > If any device don't go to suspend, meaning the ressource under reset
>> > > keep on being used, no reset will performed. With exlusive control,
>> > > there is a risk that the resuming device resets something already in use.
>> > >
>> > > Regarding the condition, on shared resets, call reset_control_reset()
>> > > should be balanced reset_control_clear() - no clear before reset.
>> >
>> > Martin, is this something that would be useful for the current users of
>> > the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
>> > meson8b-usb2 with reset-meson)?
>> for the specific use-case (system suspend) this would currently not be
>> useful for the two drivers you have named. This is because the
>> platforms on which they are used currently don't support system
>> suspend.
>> if other drivers are going to benefit from this change then please go
>> ahead and add the necessary API. Then I can also use it in these
>> drivers. however, (as far as I understand) I won't be able to verify
>> the new "fixed" behavior
>>
>>
>> Best regards,
>> Martin
>
> Hi Philipp,
>
> Regarding the naming of the new call, since reset_control_clear() is not
> really representative of the intended behaviour, I have thought of some
> other metaphors such as:
>
> reset_control_resettable()    (sounds the most relevant to me)
> reset_control_standby()
> reset_control_unseal()
> reset_control_untie()
> reset_control_loosen()/loose()
> reset_control_unfetter()
>
> What do you think?

my suggestion would be reset_control_put()

>
> Regards,
> Amjad


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

Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use

[Amjad Ouled-Ameur]

reset_control_put() is already used in the reset framework.


Le lun. 7 sept. 2020 à 10:31, Jerome Brunet <jbrunet@baylibre.com> a écrit :
>
>
> On Wed 02 Sep 2020 at 16:13, Amjad Ouled-Ameur <aouledameur@baylibre.com> wrote:
>
> > Le sam. 29 août 2020 à 17:25, Martin Blumenstingl
> > <martin.blumenstingl@googlemail.com> a écrit :
> >>
> >> Hi Philipp,
> >>
> >> On Tue, Aug 25, 2020 at 12:20 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> [...]
> >> > > reset_control_clear()
> >> > > would be the way to state that the ressource is no longer used and, that
> >> > > from the caller perspective, the reset can fired again if necessary.
> >> > >
> >> > > If we take the probe / suspend / resume example:
> >> > > * 1st device using the shared will actually trigger it (as it is now)
> >> > > * following device just increase triggered_count
> >> > >
> >> > > If all devices go to suspend (calling reset_control_clear()) then
> >> > > triggered_count will reach zero, allowing the first device resuming to
> >> > > trigger the reset again ... this is important since it might not be the
> >> > > one which would have got the exclusive control
> >> > >
> >> > > If any device don't go to suspend, meaning the ressource under reset
> >> > > keep on being used, no reset will performed. With exlusive control,
> >> > > there is a risk that the resuming device resets something already in use.
> >> > >
> >> > > Regarding the condition, on shared resets, call reset_control_reset()
> >> > > should be balanced reset_control_clear() - no clear before reset.
> >> >
> >> > Martin, is this something that would be useful for the current users of
> >> > the shared reset trigger functionality (phy-meson-gxl-usb2 and phy-
> >> > meson8b-usb2 with reset-meson)?
> >> for the specific use-case (system suspend) this would currently not be
> >> useful for the two drivers you have named. This is because the
> >> platforms on which they are used currently don't support system
> >> suspend.
> >> if other drivers are going to benefit from this change then please go
> >> ahead and add the necessary API. Then I can also use it in these
> >> drivers. however, (as far as I understand) I won't be able to verify
> >> the new "fixed" behavior
> >>
> >>
> >> Best regards,
> >> Martin
> >
> > Hi Philipp,
> >
> > Regarding the naming of the new call, since reset_control_clear() is not
> > really representative of the intended behaviour, I have thought of some
> > other metaphors such as:
> >
> > reset_control_resettable()    (sounds the most relevant to me)
> > reset_control_standby()
> > reset_control_unseal()
> > reset_control_untie()
> > reset_control_loosen()/loose()
> > reset_control_unfetter()
> >
> > What do you think?
>
> my suggestion would be reset_control_put()
>
> >
> > Regards,
> > Amjad
>


--
Amjad Ouled-Ameur
Embedded Linux Engineer - Villeneuve Loubet, FR
Engit@BayLibre - At the Heart of Embedded Linux

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

[-- 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 --]

From 841500c784ac5253e9eaf9afecd6ce125e8bf1ad Mon Sep 17 00:00:00 2001
From: Anand Moon <linux.amoon@gmail.com>
Date: Tue, 14 Jul 2020 12:13:17 +0530
Subject: [PATCH] usb: dwc3: meson-g12a: drop reset controller call from
 suspend and resume

Drop the reset controller call from suspend and resume.

Signed-off-by: Anand Moon <linux.amoon@gmail.com>
---
 drivers/usb/dwc3/dwc3-meson-g12a.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index 1f7f4d88ed9d..0c0ea55215cb 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -876,8 +876,6 @@ static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev)
 		phy_exit(priv->phys[i]);
 	}
 
-	reset_control_assert(priv->reset);
-
 	return 0;
 }
 
@@ -886,8 +884,6 @@ static int __maybe_unused dwc3_meson_g12a_resume(struct device *dev)
 	struct dwc3_meson_g12a *priv = dev_get_drvdata(dev);
 	int i, ret;
 
-	reset_control_deassert(priv->reset);
-
 	ret = priv->drvdata->usb_init(priv);
 	if (ret)
 		return ret;
-- 
2.25.1


[-- 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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson]

[-- 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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Dan Robertson]

[-- 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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Neil Armstrong]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[Anand Moon]

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

Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use

[patchwork-bot+linux-amlogic]

Hello:

This patch was applied to khilman/linux-amlogic.git (refs/heads/for-next).

On Mon, 13 Jul 2020 12:05:21 -0400 you 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
> 
> [...]


Here is a summary with links:
  - [1/1] usb: dwc3: meson-g12a: fix shared reset control use
    https://git.kernel.org/khilman/linux-amlogic/c/7a410953d1fb4dbe91ffcfdee9cbbf889d19b0d7

You are awesome, thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/pwbot

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