[1/1] usb: dwc3: meson-g12a: fix shared reset control use
diff mbox series

Message ID 20200713160522.19345-2-dan@dlrobertson.com
State Mainlined
Commit 7a410953d1fb4dbe91ffcfdee9cbbf889d19b0d7
Headers show
Series
  • usb: dwc3: meson-g12a: fix shared reset control use
Related show

Commit Message

Dan Robertson July 13, 2020, 4:05 p.m. UTC
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(-)

Comments

Neil Armstrong July 18, 2020, 8:47 a.m. UTC | #1
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
Dan Robertson July 18, 2020, 10:57 p.m. UTC | #2
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
Jerome Brunet Aug. 19, 2020, 3:03 p.m. UTC | #3
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
Kevin Hilman Aug. 20, 2020, 6:02 p.m. UTC | #4
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
Jerome Brunet Aug. 20, 2020, 6:27 p.m. UTC | #5
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
Dan Robertson Aug. 20, 2020, 6:44 p.m. UTC | #6
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
>
Kevin Hilman Aug. 20, 2020, 6:49 p.m. UTC | #7
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
Jerome Brunet Aug. 24, 2020, 8:14 a.m. UTC | #8
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
>>
Philipp Zabel Aug. 24, 2020, 10:24 a.m. UTC | #9
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
Jerome Brunet Aug. 24, 2020, 2:26 p.m. UTC | #10
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
Philipp Zabel Aug. 25, 2020, 10:20 a.m. UTC | #11
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
Jerome Brunet Aug. 25, 2020, 2:20 p.m. UTC | #12
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
Philipp Zabel Aug. 26, 2020, 8:14 a.m. UTC | #13
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
Jerome Brunet Aug. 26, 2020, 8:34 a.m. UTC | #14
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
Martin Blumenstingl Aug. 29, 2020, 3:25 p.m. UTC | #15
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
Amjad Ouled-Ameur Sept. 2, 2020, 2:13 p.m. UTC | #16
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
Jerome Brunet Sept. 7, 2020, 8:31 a.m. UTC | #17
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 Sept. 7, 2020, 8:33 a.m. UTC | #18
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

Patch
diff mbox series

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