* [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use
@ 2020-07-13 16:05 Dan Robertson
2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson
` (2 more replies)
0 siblings, 3 replies; 31+ messages in thread
From: Dan Robertson @ 2020-07-13 16:05 UTC (permalink / raw)
To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman
Cc: linux-amlogic, linux-usb, Dan Robertson, linux-arm-kernel
When testing suspend for another driver I noticed the following warning:
WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c
Hardware name: Hardkernel ODROID-N2 (DT)
[..]
pc : reset_control_assert+0x184/0x19c
lr : dwc3_meson_g12a_suspend+0x68/0x7c
[..]
Call trace:
reset_control_assert+0x184/0x19c
dwc3_meson_g12a_suspend+0x68/0x7c
platform_pm_suspend+0x28/0x54
__device_suspend+0x590/0xabc
dpm_suspend+0x104/0x404
dpm_suspend_start+0x84/0x1bc
suspend_devices_and_enter+0xc4/0x4fc
In my limited experience and knowlege it appears that we hit this
because the reset control was switched to shared and the the use
of the reset control was not changed.
> * Calling reset_control_assert without first calling reset_control_deassert
> * is not allowed on a shared reset control. Calling reset_control_reset is
> * also not allowed on a shared reset control.
The above snippet from reset_control_get_shared() seems to indicate that
this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe()
and reset_control_deassert is not guaranteed to have been called
before dwc3_meson_g12a_suspend() and reset_control_assert().
After some basic tests with the following patch I no longer hit the
warning. Comments and critiques on the patch are welcome. If there is a
reason for the current use of the reset control, I'd love to learn why!
Like I said before, I have not really looked at this driver before and
have verify limited experience with reset controls... Was working on
another driver, hit the warning, and thought I'd take a shot at the
fix :-)
Cheers,
- Dan
Dan Robertson (1):
usb: dwc3: meson-g12a: fix shared reset control use
drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic
^ permalink raw reply [flat|nested] 31+ messages in thread
* [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson @ 2020-07-13 16:05 ` Dan Robertson 2020-07-18 8:47 ` Neil Armstrong 2020-08-19 15:03 ` Jerome Brunet 2020-07-14 6:56 ` [PATCH 0/1] " Anand Moon 2020-08-17 17:48 ` patchwork-bot+linux-amlogic 2 siblings, 2 replies; 31+ messages in thread From: Dan Robertson @ 2020-07-13 16:05 UTC (permalink / raw) To: Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, Dan Robertson, linux-arm-kernel The reset is a shared reset line, but reset_control_reset is still used and reset_control_deassert is not guaranteed to have been called before the first reset_control_assert call. When suspending the following warning may be seen: WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c Hardware name: Hardkernel ODROID-N2 (DT) [..] pc : reset_control_assert+0x184/0x19c lr : dwc3_meson_g12a_suspend+0x68/0x7c [..] Call trace: reset_control_assert+0x184/0x19c dwc3_meson_g12a_suspend+0x68/0x7c platform_pm_suspend+0x28/0x54 __device_suspend+0x590/0xabc dpm_suspend+0x104/0x404 dpm_suspend_start+0x84/0x1bc suspend_devices_and_enter+0xc4/0x4fc pm_suspend+0x198/0x2d4 Fixes: 6d9fa35a347a87 ("usb: dwc3: meson-g12a: get the reset as shared") Signed-off-by: Dan Robertson <dan@dlrobertson.com> --- drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c index 1f7f4d88ed9d..88b75b5a039c 100644 --- a/drivers/usb/dwc3/dwc3-meson-g12a.c +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c @@ -737,13 +737,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) goto err_disable_clks; } - ret = reset_control_reset(priv->reset); + ret = reset_control_deassert(priv->reset); if (ret) - goto err_disable_clks; + goto err_assert_reset; ret = dwc3_meson_g12a_get_phys(priv); if (ret) - goto err_disable_clks; + goto err_assert_reset; ret = priv->drvdata->setup_regmaps(priv, base); if (ret) @@ -752,7 +752,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) if (priv->vbus) { ret = regulator_enable(priv->vbus); if (ret) - goto err_disable_clks; + goto err_assert_reset; } /* Get dr_mode */ @@ -765,13 +765,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) ret = priv->drvdata->usb_init(priv); if (ret) - goto err_disable_clks; + goto err_assert_reset; /* Init PHYs */ for (i = 0 ; i < PHY_COUNT ; ++i) { ret = phy_init(priv->phys[i]); if (ret) - goto err_disable_clks; + goto err_assert_reset; } /* Set PHY Power */ @@ -809,6 +809,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) for (i = 0 ; i < PHY_COUNT ; ++i) phy_exit(priv->phys[i]); +err_assert_reset: + reset_control_assert(priv->reset); + err_disable_clks: clk_bulk_disable_unprepare(priv->drvdata->num_clks, priv->drvdata->clks); _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson @ 2020-07-18 8:47 ` Neil Armstrong 2020-07-18 22:57 ` Dan Robertson 2020-08-19 15:03 ` Jerome Brunet 1 sibling, 1 reply; 31+ messages in thread From: Neil Armstrong @ 2020-07-18 8:47 UTC (permalink / raw) To: Dan Robertson, Martin Blumenstingl, Kevin Hilman Cc: linux-amlogic, linux-usb, linux-arm-kernel Hi, Le 13/07/2020 à 18:05, Dan Robertson a écrit : > The reset is a shared reset line, but reset_control_reset is still used > and reset_control_deassert is not guaranteed to have been called before > the first reset_control_assert call. When suspending the following > warning may be seen: > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > Hardware name: Hardkernel ODROID-N2 (DT) > [..] > pc : reset_control_assert+0x184/0x19c > lr : dwc3_meson_g12a_suspend+0x68/0x7c > [..] > Call trace: > reset_control_assert+0x184/0x19c > dwc3_meson_g12a_suspend+0x68/0x7c > platform_pm_suspend+0x28/0x54 > __device_suspend+0x590/0xabc > dpm_suspend+0x104/0x404 > dpm_suspend_start+0x84/0x1bc > suspend_devices_and_enter+0xc4/0x4fc > pm_suspend+0x198/0x2d4 > > Fixes: 6d9fa35a347a87 ("usb: dwc3: meson-g12a: get the reset as shared") > Signed-off-by: Dan Robertson <dan@dlrobertson.com> > --- > drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------ > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c > index 1f7f4d88ed9d..88b75b5a039c 100644 > --- a/drivers/usb/dwc3/dwc3-meson-g12a.c > +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c > @@ -737,13 +737,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > goto err_disable_clks; > } > > - ret = reset_control_reset(priv->reset); > + ret = reset_control_deassert(priv->reset); > if (ret) > - goto err_disable_clks; > + goto err_assert_reset; > > ret = dwc3_meson_g12a_get_phys(priv); > if (ret) > - goto err_disable_clks; > + goto err_assert_reset; > > ret = priv->drvdata->setup_regmaps(priv, base); > if (ret) > @@ -752,7 +752,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > if (priv->vbus) { > ret = regulator_enable(priv->vbus); > if (ret) > - goto err_disable_clks; > + goto err_assert_reset; > } > > /* Get dr_mode */ > @@ -765,13 +765,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > > ret = priv->drvdata->usb_init(priv); > if (ret) > - goto err_disable_clks; > + goto err_assert_reset; > > /* Init PHYs */ > for (i = 0 ; i < PHY_COUNT ; ++i) { > ret = phy_init(priv->phys[i]); > if (ret) > - goto err_disable_clks; > + goto err_assert_reset; > } > > /* Set PHY Power */ > @@ -809,6 +809,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > for (i = 0 ; i < PHY_COUNT ; ++i) > phy_exit(priv->phys[i]); > > +err_assert_reset: > + reset_control_assert(priv->reset); > + > err_disable_clks: > clk_bulk_disable_unprepare(priv->drvdata->num_clks, > priv->drvdata->clks); > This should do the trick, I'll need to check first if it doesn't break the GXL/GXM support first. thanks for the fix Neil _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-18 8:47 ` Neil Armstrong @ 2020-07-18 22:57 ` Dan Robertson 0 siblings, 0 replies; 31+ messages in thread From: Dan Robertson @ 2020-07-18 22:57 UTC (permalink / raw) To: Neil Armstrong Cc: Martin Blumenstingl, Kevin Hilman, linux-usb, linux-arm-kernel, linux-amlogic On Sat, Jul 18, 2020 at 10:47:55AM +0200, Neil Armstrong wrote: > Hi, > > Le 13/07/2020 à 18:05, Dan Robertson a écrit : > > The reset is a shared reset line, but reset_control_reset is still used > > and reset_control_deassert is not guaranteed to have been called before > > the first reset_control_assert call. When suspending the following > > warning may be seen: > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > Hardware name: Hardkernel ODROID-N2 (DT) > > [..] > > pc : reset_control_assert+0x184/0x19c > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > [..] > > Call trace: > > reset_control_assert+0x184/0x19c > > dwc3_meson_g12a_suspend+0x68/0x7c > > platform_pm_suspend+0x28/0x54 > > __device_suspend+0x590/0xabc > > dpm_suspend+0x104/0x404 > > dpm_suspend_start+0x84/0x1bc > > suspend_devices_and_enter+0xc4/0x4fc > > pm_suspend+0x198/0x2d4 > > > > Fixes: 6d9fa35a347a87 ("usb: dwc3: meson-g12a: get the reset as shared") > > Signed-off-by: Dan Robertson <dan@dlrobertson.com> > > --- > > drivers/usb/dwc3/dwc3-meson-g12a.c | 15 +++++++++------ > > 1 file changed, 9 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c > > index 1f7f4d88ed9d..88b75b5a039c 100644 > > --- a/drivers/usb/dwc3/dwc3-meson-g12a.c > > +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c > > @@ -737,13 +737,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > > goto err_disable_clks; > > } > > > > - ret = reset_control_reset(priv->reset); > > + ret = reset_control_deassert(priv->reset); > > if (ret) > > - goto err_disable_clks; > > + goto err_assert_reset; > > > > ret = dwc3_meson_g12a_get_phys(priv); > > if (ret) > > - goto err_disable_clks; > > + goto err_assert_reset; > > > > ret = priv->drvdata->setup_regmaps(priv, base); > > if (ret) > > @@ -752,7 +752,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > > if (priv->vbus) { > > ret = regulator_enable(priv->vbus); > > if (ret) > > - goto err_disable_clks; > > + goto err_assert_reset; > > } > > > > /* Get dr_mode */ > > @@ -765,13 +765,13 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > > > > ret = priv->drvdata->usb_init(priv); > > if (ret) > > - goto err_disable_clks; > > + goto err_assert_reset; > > > > /* Init PHYs */ > > for (i = 0 ; i < PHY_COUNT ; ++i) { > > ret = phy_init(priv->phys[i]); > > if (ret) > > - goto err_disable_clks; > > + goto err_assert_reset; > > } > > > > /* Set PHY Power */ > > @@ -809,6 +809,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) > > for (i = 0 ; i < PHY_COUNT ; ++i) > > phy_exit(priv->phys[i]); > > > > +err_assert_reset: > > + reset_control_assert(priv->reset); > > + > > err_disable_clks: > > clk_bulk_disable_unprepare(priv->drvdata->num_clks, > > priv->drvdata->clks); > > > > > This should do the trick, I'll need to check first if it doesn't break the GXL/GXM > support first. Awesome. Let me know if you find anything. Cheers, - Dan _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson 2020-07-18 8:47 ` Neil Armstrong @ 2020-08-19 15:03 ` Jerome Brunet 2020-08-20 18:02 ` Kevin Hilman ` (2 more replies) 1 sibling, 3 replies; 31+ messages in thread From: Jerome Brunet @ 2020-08-19 15:03 UTC (permalink / raw) To: Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman, Philipp Zabel Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-19 15:03 ` Jerome Brunet @ 2020-08-20 18:02 ` Kevin Hilman 2020-08-20 18:27 ` Jerome Brunet 2020-08-20 18:44 ` Dan Robertson 2020-08-24 10:24 ` Philipp Zabel 2 siblings, 1 reply; 31+ messages in thread From: Kevin Hilman @ 2020-08-20 18:02 UTC (permalink / raw) To: Jerome Brunet, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Philipp Zabel Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-20 18:02 ` Kevin Hilman @ 2020-08-20 18:27 ` Jerome Brunet 2020-08-20 18:49 ` Kevin Hilman 0 siblings, 1 reply; 31+ messages in thread From: Jerome Brunet @ 2020-08-20 18:27 UTC (permalink / raw) To: Kevin Hilman, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Philipp Zabel, Felipe Balbi Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-20 18:27 ` Jerome Brunet @ 2020-08-20 18:49 ` Kevin Hilman 0 siblings, 0 replies; 31+ messages in thread From: Kevin Hilman @ 2020-08-20 18:49 UTC (permalink / raw) To: Jerome Brunet, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Philipp Zabel, Felipe Balbi Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-19 15:03 ` Jerome Brunet 2020-08-20 18:02 ` Kevin Hilman @ 2020-08-20 18:44 ` Dan Robertson 2020-08-24 8:14 ` Jerome Brunet 2020-08-24 10:24 ` Philipp Zabel 2 siblings, 1 reply; 31+ messages in thread From: Dan Robertson @ 2020-08-20 18:44 UTC (permalink / raw) To: Jerome Brunet Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-usb, aouledameur, Philipp Zabel, linux-amlogic, linux-arm-kernel [-- 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-20 18:44 ` Dan Robertson @ 2020-08-24 8:14 ` Jerome Brunet 0 siblings, 0 replies; 31+ messages in thread From: Jerome Brunet @ 2020-08-24 8:14 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-usb, aouledameur, Philipp Zabel, linux-amlogic, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-19 15:03 ` Jerome Brunet 2020-08-20 18:02 ` Kevin Hilman 2020-08-20 18:44 ` Dan Robertson @ 2020-08-24 10:24 ` Philipp Zabel 2020-08-24 14:26 ` Jerome Brunet 2 siblings, 1 reply; 31+ messages in thread From: Philipp Zabel @ 2020-08-24 10:24 UTC (permalink / raw) To: Jerome Brunet, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-24 10:24 ` Philipp Zabel @ 2020-08-24 14:26 ` Jerome Brunet 2020-08-25 10:20 ` Philipp Zabel 0 siblings, 1 reply; 31+ messages in thread From: Jerome Brunet @ 2020-08-24 14:26 UTC (permalink / raw) To: Philipp Zabel, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-24 14:26 ` Jerome Brunet @ 2020-08-25 10:20 ` Philipp Zabel 2020-08-25 14:20 ` Jerome Brunet 2020-08-29 15:25 ` Martin Blumenstingl 0 siblings, 2 replies; 31+ messages in thread From: Philipp Zabel @ 2020-08-25 10:20 UTC (permalink / raw) To: Jerome Brunet, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-25 10:20 ` Philipp Zabel @ 2020-08-25 14:20 ` Jerome Brunet 2020-08-26 8:14 ` Philipp Zabel 2020-08-29 15:25 ` Martin Blumenstingl 1 sibling, 1 reply; 31+ messages in thread From: Jerome Brunet @ 2020-08-25 14:20 UTC (permalink / raw) To: Philipp Zabel, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-25 14:20 ` Jerome Brunet @ 2020-08-26 8:14 ` Philipp Zabel 2020-08-26 8:34 ` Jerome Brunet 0 siblings, 1 reply; 31+ messages in thread From: Philipp Zabel @ 2020-08-26 8:14 UTC (permalink / raw) To: Jerome Brunet, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-26 8:14 ` Philipp Zabel @ 2020-08-26 8:34 ` Jerome Brunet 0 siblings, 0 replies; 31+ messages in thread From: Jerome Brunet @ 2020-08-26 8:34 UTC (permalink / raw) To: Philipp Zabel, Dan Robertson, Martin Blumenstingl, Neil Armstrong, Kevin Hilman Cc: linux-amlogic, linux-usb, aouledameur, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-25 10:20 ` Philipp Zabel 2020-08-25 14:20 ` Jerome Brunet @ 2020-08-29 15:25 ` Martin Blumenstingl 2020-09-02 14:13 ` Amjad Ouled-Ameur 1 sibling, 1 reply; 31+ messages in thread From: Martin Blumenstingl @ 2020-08-29 15:25 UTC (permalink / raw) To: Philipp Zabel Cc: Dan Robertson, Neil Armstrong, Kevin Hilman, linux-usb, aouledameur, linux-amlogic, linux-arm-kernel, Jerome Brunet 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-08-29 15:25 ` Martin Blumenstingl @ 2020-09-02 14:13 ` Amjad Ouled-Ameur 2020-09-07 8:31 ` Jerome Brunet 0 siblings, 1 reply; 31+ messages in thread From: Amjad Ouled-Ameur @ 2020-09-02 14:13 UTC (permalink / raw) To: Philipp Zabel Cc: Dan Robertson, Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-usb, linux-amlogic, linux-arm-kernel, Jerome Brunet 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-09-02 14:13 ` Amjad Ouled-Ameur @ 2020-09-07 8:31 ` Jerome Brunet 2020-09-07 8:33 ` Amjad Ouled-Ameur 0 siblings, 1 reply; 31+ messages in thread From: Jerome Brunet @ 2020-09-07 8:31 UTC (permalink / raw) To: Amjad Ouled-Ameur, Philipp Zabel Cc: Dan Robertson, Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-usb, linux-amlogic, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 1/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-09-07 8:31 ` Jerome Brunet @ 2020-09-07 8:33 ` Amjad Ouled-Ameur 0 siblings, 0 replies; 31+ messages in thread From: Amjad Ouled-Ameur @ 2020-09-07 8:33 UTC (permalink / raw) To: Jerome Brunet Cc: Dan Robertson, Neil Armstrong, Martin Blumenstingl, Kevin Hilman, linux-usb, Philipp Zabel, linux-amlogic, linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson 2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson @ 2020-07-14 6:56 ` Anand Moon 2020-07-14 13:30 ` Dan Robertson 2020-08-17 17:48 ` patchwork-bot+linux-amlogic 2 siblings, 1 reply; 31+ messages in thread From: Anand Moon @ 2020-07-14 6:56 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel [-- Attachment #1: Type: text/plain, Size: 2093 bytes --] hi Dan, On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > When testing suspend for another driver I noticed the following warning: > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > Hardware name: Hardkernel ODROID-N2 (DT) > [..] > pc : reset_control_assert+0x184/0x19c > lr : dwc3_meson_g12a_suspend+0x68/0x7c > [..] > Call trace: > reset_control_assert+0x184/0x19c > dwc3_meson_g12a_suspend+0x68/0x7c > platform_pm_suspend+0x28/0x54 > __device_suspend+0x590/0xabc > dpm_suspend+0x104/0x404 > dpm_suspend_start+0x84/0x1bc > suspend_devices_and_enter+0xc4/0x4fc > > In my limited experience and knowlege it appears that we hit this > because the reset control was switched to shared and the the use > of the reset control was not changed. > > > * Calling reset_control_assert without first calling reset_control_deassert > > * is not allowed on a shared reset control. Calling reset_control_reset is > > * also not allowed on a shared reset control. > > The above snippet from reset_control_get_shared() seems to indicate that > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > and reset_control_deassert is not guaranteed to have been called > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > After some basic tests with the following patch I no longer hit the > warning. Comments and critiques on the patch are welcome. If there is a > reason for the current use of the reset control, I'd love to learn why! > Like I said before, I have not really looked at this driver before and > have verify limited experience with reset controls... Was working on > another driver, hit the warning, and thought I'd take a shot at the > fix :-) > Thanks, I was also looking into this issue So best Fix to this issue is to drop the call of reset_control_assert from dwc3_meson_g12a_suspend and drop the call of reset_control_deassert from dwc3_meson_g12a_resume With these changes we do not see the warning on suspend and resume Can you try this attached patch? Best Regards -Anand [-- Attachment #2: 0001-usb-dwc3-meson-g12a-drop-reset-controller-call-from-.patch --] [-- Type: application/octet-stream, Size: 1097 bytes --] 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 ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-14 6:56 ` [PATCH 0/1] " Anand Moon @ 2020-07-14 13:30 ` Dan Robertson 2020-07-14 15:27 ` Anand Moon 0 siblings, 1 reply; 31+ messages in thread From: Dan Robertson @ 2020-07-14 13:30 UTC (permalink / raw) To: Anand Moon Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 3277 bytes --] Hi Anand! On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > hi Dan, > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > > > When testing suspend for another driver I noticed the following warning: > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > Hardware name: Hardkernel ODROID-N2 (DT) > > [..] > > pc : reset_control_assert+0x184/0x19c > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > [..] > > Call trace: > > reset_control_assert+0x184/0x19c > > dwc3_meson_g12a_suspend+0x68/0x7c > > platform_pm_suspend+0x28/0x54 > > __device_suspend+0x590/0xabc > > dpm_suspend+0x104/0x404 > > dpm_suspend_start+0x84/0x1bc > > suspend_devices_and_enter+0xc4/0x4fc > > > > In my limited experience and knowlege it appears that we hit this > > because the reset control was switched to shared and the the use > > of the reset control was not changed. > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > * also not allowed on a shared reset control. > > > > The above snippet from reset_control_get_shared() seems to indicate that > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > and reset_control_deassert is not guaranteed to have been called > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > After some basic tests with the following patch I no longer hit the > > warning. Comments and critiques on the patch are welcome. If there is a > > reason for the current use of the reset control, I'd love to learn why! > > Like I said before, I have not really looked at this driver before and > > have verify limited experience with reset controls... Was working on > > another driver, hit the warning, and thought I'd take a shot at the > > fix :-) > > > Thanks, I was also looking into this issue Awesome! > So best Fix to this issue is to drop the call of reset_control_assert > from dwc3_meson_g12a_suspend > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > With these changes we do not see the warning on suspend and resume We definitely would avoid hitting the warning without the reset_control_(de)assert calls in suspend and resume. That is a valid use of the reset control, but why would that be best? From reset_control_reset(): > * Consumers must not use reset_control_(de)assert on shared reset lines when > * reset_control_reset has been used. If we use reset_control_reset() then we can not (de)assert the reset line on suspend/resume or any other time. Wouldn't we want to be able to (de)assert the reset line? And why do we no longer want to (de)assert the reset line on suspend/resume? > > Can you try this attached patch? > > Best Regards > -Anand I think I had already tested something similar. Removing the (de)assert calls but keeping reset will definitely remove the warning, but it means we can not (de)assert the line. My guess is that this is not what we want, but I could be wrong. Thoughts, input, or references to documentation on this would be appreciated. Cheers, - Dan [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-14 13:30 ` Dan Robertson @ 2020-07-14 15:27 ` Anand Moon 2020-07-15 2:58 ` Dan Robertson 0 siblings, 1 reply; 31+ messages in thread From: Anand Moon @ 2020-07-14 15:27 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel Hi Dan, On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote: > > Hi Anand! > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > > hi Dan, > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > When testing suspend for another driver I noticed the following warning: > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > > Hardware name: Hardkernel ODROID-N2 (DT) > > > [..] > > > pc : reset_control_assert+0x184/0x19c > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > > [..] > > > Call trace: > > > reset_control_assert+0x184/0x19c > > > dwc3_meson_g12a_suspend+0x68/0x7c > > > platform_pm_suspend+0x28/0x54 > > > __device_suspend+0x590/0xabc > > > dpm_suspend+0x104/0x404 > > > dpm_suspend_start+0x84/0x1bc > > > suspend_devices_and_enter+0xc4/0x4fc > > > > > > In my limited experience and knowlege it appears that we hit this > > > because the reset control was switched to shared and the the use > > > of the reset control was not changed. > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > > * also not allowed on a shared reset control. > > > > > > The above snippet from reset_control_get_shared() seems to indicate that > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > > and reset_control_deassert is not guaranteed to have been called > > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > > > After some basic tests with the following patch I no longer hit the > > > warning. Comments and critiques on the patch are welcome. If there is a > > > reason for the current use of the reset control, I'd love to learn why! > > > Like I said before, I have not really looked at this driver before and > > > have verify limited experience with reset controls... Was working on > > > another driver, hit the warning, and thought I'd take a shot at the > > > fix :-) > > > > > Thanks, I was also looking into this issue > > Awesome! > > > So best Fix to this issue is to drop the call of reset_control_assert > > from dwc3_meson_g12a_suspend > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > > With these changes we do not see the warning on suspend and resume > > We definitely would avoid hitting the warning without the > reset_control_(de)assert calls in suspend and resume. That is a valid use of > the reset control, but why would that be best? > > From reset_control_reset(): Before entering the suspend state the code tries to do following clk_bulk_disable_unprepare regulator_disable phy_power_off phy_exit After this operation it's needless to call *reset_control_assert* I tried to move this call before all the above operations but still no success with this. Similarly on resume we should avoid calling resume *reset_control_deassert* earlier as the core is just reinitializing the devices. clk_bulk_prepare_enable usb_init phy_init phy_power_on regulator_enable > > > * Consumers must not use reset_control_(de)assert on shared reset lines when > > * reset_control_reset has been used. > > If we use reset_control_reset() then we can not (de)assert the reset line > on suspend/resume or any other time. Wouldn't we want to be able to > (de)assert the reset line? And why do we no longer want to (de)assert the > reset line on suspend/resume? > > > > Can you try this attached patch? > > > > Best Regards > > -Anand > > I think I had already tested something similar. Removing the (de)assert calls > but keeping reset will definitely remove the warning, but it means we can not > (de)assert the line. My guess is that this is not what we want, but I could be > wrong. Thoughts, input, or references to documentation on this would be > appreciated. > As per my knowledge suspend to mem will do complete power down of the devices with support suspend and resume feature sequentially and then it tries to bring the device up one by one. So it should also take care of reset lines as well. > Cheers, > > - Dan Best Regards -Anand _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-14 15:27 ` Anand Moon @ 2020-07-15 2:58 ` Dan Robertson 2020-07-15 16:23 ` Anand Moon 0 siblings, 1 reply; 31+ messages in thread From: Dan Robertson @ 2020-07-15 2:58 UTC (permalink / raw) To: Anand Moon Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel [-- Attachment #1.1: Type: text/plain, Size: 4925 bytes --] On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote: > Hi Dan, > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote: > > > > Hi Anand! > > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > > > hi Dan, > > > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > > > When testing suspend for another driver I noticed the following warning: > > > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > > > Hardware name: Hardkernel ODROID-N2 (DT) > > > > [..] > > > > pc : reset_control_assert+0x184/0x19c > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > > > [..] > > > > Call trace: > > > > reset_control_assert+0x184/0x19c > > > > dwc3_meson_g12a_suspend+0x68/0x7c > > > > platform_pm_suspend+0x28/0x54 > > > > __device_suspend+0x590/0xabc > > > > dpm_suspend+0x104/0x404 > > > > dpm_suspend_start+0x84/0x1bc > > > > suspend_devices_and_enter+0xc4/0x4fc > > > > > > > > In my limited experience and knowlege it appears that we hit this > > > > because the reset control was switched to shared and the the use > > > > of the reset control was not changed. > > > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > > > * also not allowed on a shared reset control. > > > > > > > > The above snippet from reset_control_get_shared() seems to indicate that > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > > > and reset_control_deassert is not guaranteed to have been called > > > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > > > > > After some basic tests with the following patch I no longer hit the > > > > warning. Comments and critiques on the patch are welcome. If there is a > > > > reason for the current use of the reset control, I'd love to learn why! > > > > Like I said before, I have not really looked at this driver before and > > > > have verify limited experience with reset controls... Was working on > > > > another driver, hit the warning, and thought I'd take a shot at the > > > > fix :-) > > > > > > > Thanks, I was also looking into this issue > > > > Awesome! > > > > > So best Fix to this issue is to drop the call of reset_control_assert > > > from dwc3_meson_g12a_suspend > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > > > With these changes we do not see the warning on suspend and resume > > > > We definitely would avoid hitting the warning without the > > reset_control_(de)assert calls in suspend and resume. That is a valid use of > > the reset control, but why would that be best? > > > > From reset_control_reset(): > > Before entering the suspend state the code tries to do following > clk_bulk_disable_unprepare > regulator_disable > phy_power_off > phy_exit > > After this operation it's needless to call *reset_control_assert* > I tried to move this call before all the above operations > but still no success with this. How so? Once the reset() is removed prope() and deassert() is guaranteed to have been called before suspend, like what is in the patch and similar to other uses of shared reset controllers, this is possible. > Similarly on resume we should avoid calling resume > *reset_control_deassert* earlier > as the core is just reinitializing the devices. > clk_bulk_prepare_enable > usb_init > phy_init > phy_power_on > regulator_enable > > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when > > > * reset_control_reset has been used. > > > > If we use reset_control_reset() then we can not (de)assert the reset line > > on suspend/resume or any other time. Wouldn't we want to be able to > > (de)assert the reset line? And why do we no longer want to (de)assert the > > reset line on suspend/resume? > > > > > > Can you try this attached patch? > > > > > I think I had already tested something similar. Removing the (de)assert calls > > but keeping reset will definitely remove the warning, but it means we can not > > (de)assert the line. My guess is that this is not what we want, but I could be > > wrong. Thoughts, input, or references to documentation on this would be > > appreciated. > > > > As per my knowledge suspend to mem will do complete power down of the > devices with support suspend and resume feature sequentially and then it tries > to bring the device up one by one. > So it should also take care of reset lines as well. So do we only _actually_ care about the reset line in the probe? Seems like we should remove the reset controller from the structure if that is the case. Cheers, - Dan [-- Attachment #1.2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] [-- Attachment #2: Type: text/plain, Size: 167 bytes --] _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-15 2:58 ` Dan Robertson @ 2020-07-15 16:23 ` Anand Moon 2020-07-17 9:01 ` Anand Moon 0 siblings, 1 reply; 31+ messages in thread From: Anand Moon @ 2020-07-15 16:23 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel Hi Dan, On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@dlrobertson.com> wrote: > > On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote: > > Hi Dan, > > > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > Hi Anand! > > > > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > > > > hi Dan, > > > > > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > > > > > When testing suspend for another driver I noticed the following warning: > > > > > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > > > > Hardware name: Hardkernel ODROID-N2 (DT) > > > > > [..] > > > > > pc : reset_control_assert+0x184/0x19c > > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > > > > [..] > > > > > Call trace: > > > > > reset_control_assert+0x184/0x19c > > > > > dwc3_meson_g12a_suspend+0x68/0x7c > > > > > platform_pm_suspend+0x28/0x54 > > > > > __device_suspend+0x590/0xabc > > > > > dpm_suspend+0x104/0x404 > > > > > dpm_suspend_start+0x84/0x1bc > > > > > suspend_devices_and_enter+0xc4/0x4fc > > > > > > > > > > In my limited experience and knowlege it appears that we hit this > > > > > because the reset control was switched to shared and the the use > > > > > of the reset control was not changed. > > > > > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > > > > * also not allowed on a shared reset control. > > > > > > > > > > The above snippet from reset_control_get_shared() seems to indicate that > > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > > > > and reset_control_deassert is not guaranteed to have been called > > > > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > > > > > > > After some basic tests with the following patch I no longer hit the > > > > > warning. Comments and critiques on the patch are welcome. If there is a > > > > > reason for the current use of the reset control, I'd love to learn why! > > > > > Like I said before, I have not really looked at this driver before and > > > > > have verify limited experience with reset controls... Was working on > > > > > another driver, hit the warning, and thought I'd take a shot at the > > > > > fix :-) > > > > > > > > > Thanks, I was also looking into this issue > > > > > > Awesome! > > > > > > > So best Fix to this issue is to drop the call of reset_control_assert > > > > from dwc3_meson_g12a_suspend > > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > > > > With these changes we do not see the warning on suspend and resume > > > > > > We definitely would avoid hitting the warning without the > > > reset_control_(de)assert calls in suspend and resume. That is a valid use of > > > the reset control, but why would that be best? > > > > > > From reset_control_reset(): > > > > Before entering the suspend state the code tries to do following > > clk_bulk_disable_unprepare > > regulator_disable > > phy_power_off > > phy_exit > > > > After this operation it's needless to call *reset_control_assert* > > I tried to move this call before all the above operations > > but still no success with this. > > How so? Once the reset() is removed prope() and deassert() is guaranteed > to have been called before suspend, like what is in the patch and similar > to other uses of shared reset controllers, this is possible. > > > Similarly on resume we should avoid calling resume > > *reset_control_deassert* earlier > > as the core is just reinitializing the devices. > > clk_bulk_prepare_enable > > usb_init > > phy_init > > phy_power_on > > regulator_enable > > > > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when > > > > * reset_control_reset has been used. > > > > > > If we use reset_control_reset() then we can not (de)assert the reset line > > > on suspend/resume or any other time. Wouldn't we want to be able to > > > (de)assert the reset line? And why do we no longer want to (de)assert the > > > reset line on suspend/resume? > > > > > > > > Can you try this attached patch? > > > > > > > I think I had already tested something similar. Removing the (de)assert calls > > > but keeping reset will definitely remove the warning, but it means we can not > > > (de)assert the line. My guess is that this is not what we want, but I could be > > > wrong. Thoughts, input, or references to documentation on this would be > > > appreciated. > > > > > > > As per my knowledge suspend to mem will do complete power down of the > > devices with support suspend and resume feature sequentially and then it tries > > to bring the device up one by one. > > So it should also take care of reset lines as well. > > So do we only _actually_ care about the reset line in the probe? Seems like we > should remove the reset controller from the structure if that is the case. > > Cheers, > > - Dan Sorry I am not the _expert_ in suspend/resume feature but I can debug this, Certainly we need to look into whether reset controller calls are needed to suspend or resume for this driver. First thing we need to get the RTC driver to work on Odroid N2 for suspend wakeup to work properly. For this I have created the following patches. [0] https://patchwork.kernel.org/cover/11665865/ With these patches the RTC feature for wake up is broken right now in my testing. Once I get something to work further I will update you. --Anand _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-15 16:23 ` Anand Moon @ 2020-07-17 9:01 ` Anand Moon 2020-07-17 16:38 ` Anand Moon 0 siblings, 1 reply; 31+ messages in thread From: Anand Moon @ 2020-07-17 9:01 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel Hi Dan, On Wed, 15 Jul 2020 at 21:53, Anand Moon <linux.amoon@gmail.com> wrote: > > Hi Dan, > > On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@dlrobertson.com> wrote: > > > > On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote: > > > Hi Dan, > > > > > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > > > Hi Anand! > > > > > > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > > > > > hi Dan, > > > > > > > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > > > > > > > When testing suspend for another driver I noticed the following warning: > > > > > > > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > > > > > Hardware name: Hardkernel ODROID-N2 (DT) > > > > > > [..] > > > > > > pc : reset_control_assert+0x184/0x19c > > > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > > > > > [..] > > > > > > Call trace: > > > > > > reset_control_assert+0x184/0x19c > > > > > > dwc3_meson_g12a_suspend+0x68/0x7c > > > > > > platform_pm_suspend+0x28/0x54 > > > > > > __device_suspend+0x590/0xabc > > > > > > dpm_suspend+0x104/0x404 > > > > > > dpm_suspend_start+0x84/0x1bc > > > > > > suspend_devices_and_enter+0xc4/0x4fc > > > > > > > > > > > > In my limited experience and knowlege it appears that we hit this > > > > > > because the reset control was switched to shared and the the use > > > > > > of the reset control was not changed. > > > > > > > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > > > > > * also not allowed on a shared reset control. > > > > > > > > > > > > The above snippet from reset_control_get_shared() seems to indicate that > > > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > > > > > and reset_control_deassert is not guaranteed to have been called > > > > > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > > > > > > > > > After some basic tests with the following patch I no longer hit the > > > > > > warning. Comments and critiques on the patch are welcome. If there is a > > > > > > reason for the current use of the reset control, I'd love to learn why! > > > > > > Like I said before, I have not really looked at this driver before and > > > > > > have verify limited experience with reset controls... Was working on > > > > > > another driver, hit the warning, and thought I'd take a shot at the > > > > > > fix :-) > > > > > > > > > > > Thanks, I was also looking into this issue > > > > > > > > Awesome! > > > > > > > > > So best Fix to this issue is to drop the call of reset_control_assert > > > > > from dwc3_meson_g12a_suspend > > > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > > > > > With these changes we do not see the warning on suspend and resume > > > > > > > > We definitely would avoid hitting the warning without the > > > > reset_control_(de)assert calls in suspend and resume. That is a valid use of > > > > the reset control, but why would that be best? > > > > > > > > From reset_control_reset(): > > > > > > Before entering the suspend state the code tries to do following > > > clk_bulk_disable_unprepare > > > regulator_disable > > > phy_power_off > > > phy_exit > > > > > > After this operation it's needless to call *reset_control_assert* > > > I tried to move this call before all the above operations > > > but still no success with this. > > > > How so? Once the reset() is removed prope() and deassert() is guaranteed > > to have been called before suspend, like what is in the patch and similar > > to other uses of shared reset controllers, this is possible. > > > > > Similarly on resume we should avoid calling resume > > > *reset_control_deassert* earlier > > > as the core is just reinitializing the devices. > > > clk_bulk_prepare_enable > > > usb_init > > > phy_init > > > phy_power_on > > > regulator_enable > > > > > > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when > > > > > * reset_control_reset has been used. > > > > > > > > If we use reset_control_reset() then we can not (de)assert the reset line > > > > on suspend/resume or any other time. Wouldn't we want to be able to > > > > (de)assert the reset line? And why do we no longer want to (de)assert the > > > > reset line on suspend/resume? > > > > > > > > > > Can you try this attached patch? > > > > > > > > > I think I had already tested something similar. Removing the (de)assert calls > > > > but keeping reset will definitely remove the warning, but it means we can not > > > > (de)assert the line. My guess is that this is not what we want, but I could be > > > > wrong. Thoughts, input, or references to documentation on this would be > > > > appreciated. > > > > > > > > > > As per my knowledge suspend to mem will do complete power down of the > > > devices with support suspend and resume feature sequentially and then it tries > > > to bring the device up one by one. > > > So it should also take care of reset lines as well. > > > > So do we only _actually_ care about the reset line in the probe? Seems like we > > should remove the reset controller from the structure if that is the case. > > > > Cheers, > > > > - Dan > > Sorry I am not the _expert_ in suspend/resume feature but I can debug this, > Certainly we need to look into whether reset controller calls are needed > to suspend or resume for this driver. > > First thing we need to get the RTC driver to work on Odroid N2 for > suspend wakeup > to work properly. > For this I have created the following patches. > > [0] https://patchwork.kernel.org/cover/11665865/ > > With these patches the RTC feature for wake up is broken right now in > my testing. > Once I get something to work further I will update you. > > --Anand Sorry for the _noise_ :‑( This feature seems to be working fine with VRTC drivers. I have tested this with a pre-compiled image of Archlinux distro. [root@alarm alarm]# uname -a Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux [root@alarm alarm]# rtcwake -s 30 -m mem rtcwake: assuming RTC uses UTC ... rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan 1 00:10:14 1970 [ 583.591477] PM: suspend entry (deep) [ 583.593737] Filesystems sync: 0.002 seconds [ 583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done. [ 583.825802] OOM killer disabled. [ 583.828966] Freezing remaining freezable tasks ... (elapsed 0.003 seconds) done. [ 583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache [ 584.020094] PM: suspend devices took 0.190 seconds [ 584.070586] Disabling non-boot CPUs ... [ 584.075037] CPU1: shutdown [ 584.075223] psci: CPU1 killed (polled 0 ms) [ 584.097199] CPU2: shutdown [ 584.098546] psci: CPU2 killed (polled 0 ms) [ 584.115370] CPU3: shutdown [ 584.116500] psci: CPU3 killed (polled 0 ms) [ 584.128116] CPU4: shutdown [ 584.129235] psci: CPU4 killed (polled 10 ms) [ 584.140122] CPU5: shutdown [ 584.147289] psci: CPU5 killed (polled 0 ms) bl30 get wakeup sources! process command 00000006 bl30 enter suspend! Little core clk suspend rate 1896000000 Big core clk suspend rate 24000000 store restore gp0 pll suspend_counter: 3 Enter ddr suspend ddr suspend time: 16us alarm=31S process command 00000001 GPIOA_11/13 off cec ver:2018/04/19 CEC cfg:0x0000 WAKEUP GPIO cfg:0x00000000 use vddee new table! use vddee new table! exit_reason:0x03 Enter ddr resume DMC_DRAM_STAT3: 0x544 ddr resume time: 3188us store restore gp0 pll cfg15 33b00000 cfg15 33b00000 Li[ 584.148720] Enabling non-boot CPUs ... [ 584.149124] Detected VIPT I-cache on CPU1 [ 584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034] [ 584.149594] CPU1 is up [ 584.160687] Detected VIPT I-cache on CPU2 [ 584.160730] arch_timer: CPU2: Trapping CNTVCT access [ 584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092] [ 584.161327] CPU2 is up [ 584.177645] Detected VIPT I-cache on CPU3 [ 584.177668] arch_timer: CPU3: Trapping CNTVCT access [ 584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092] [ 584.178036] CPU3 is up [ 584.195338] Detected VIPT I-cache on CPU4 [ 584.195361] arch_timer: CPU4: Trapping CNTVCT access [ 584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092] [ 584.195762] CPU4 is up [ 584.213002] Detected VIPT I-cache on CPU5 [ 584.213024] arch_timer: CPU5: Trapping CNTVCT access [ 584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092] [ 584.213450] CPU5 is up ttle core clk resume rate 1896000000 Big core clk resume rate 50000000 [ 584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety Features support found [ 584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for phy/rgmii link mode [ 584.401216] usb usb1: root hub lost power or was reset [ 584.401470] usb usb2: root hub lost power or was reset [ 584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode [ 584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 using xhci-hcd [ 584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd [ 585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3 using xhci-hcd [ 585.333303] PM: resume devices took 1.100 seconds [ 585.333507] OOM killer enabled. [ 585.335549] Restarting tasks ... done. [ 585.378044] PM: suspend exit rtcwake: read rtc alarm failed: Invalid argument [root@alarm alarm]# -Anand _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-17 9:01 ` Anand Moon @ 2020-07-17 16:38 ` Anand Moon 2020-07-18 6:31 ` Anand Moon 0 siblings, 1 reply; 31+ messages in thread From: Anand Moon @ 2020-07-17 16:38 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel Hi Dan, On Fri, 17 Jul 2020 at 14:31, Anand Moon <linux.amoon@gmail.com> wrote: > > Hi Dan, > > On Wed, 15 Jul 2020 at 21:53, Anand Moon <linux.amoon@gmail.com> wrote: > > > > Hi Dan, > > > > On Wed, 15 Jul 2020 at 08:48, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > On Tue, Jul 14, 2020 at 08:57:35PM +0530, Anand Moon wrote: > > > > Hi Dan, > > > > > > > > On Tue, 14 Jul 2020 at 19:00, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > > > > > Hi Anand! > > > > > > > > > > On Tue, Jul 14, 2020 at 12:26:57PM +0530, Anand Moon wrote: > > > > > > hi Dan, > > > > > > > > > > > > On Mon, 13 Jul 2020 at 21:37, Dan Robertson <dan@dlrobertson.com> wrote: > > > > > > > > > > > > > > When testing suspend for another driver I noticed the following warning: > > > > > > > > > > > > > > WARNING: CPU: 1 PID: 5530 at drivers/reset/core.c:355 reset_control_assert+0x184/0x19c > > > > > > > Hardware name: Hardkernel ODROID-N2 (DT) > > > > > > > [..] > > > > > > > pc : reset_control_assert+0x184/0x19c > > > > > > > lr : dwc3_meson_g12a_suspend+0x68/0x7c > > > > > > > [..] > > > > > > > Call trace: > > > > > > > reset_control_assert+0x184/0x19c > > > > > > > dwc3_meson_g12a_suspend+0x68/0x7c > > > > > > > platform_pm_suspend+0x28/0x54 > > > > > > > __device_suspend+0x590/0xabc > > > > > > > dpm_suspend+0x104/0x404 > > > > > > > dpm_suspend_start+0x84/0x1bc > > > > > > > suspend_devices_and_enter+0xc4/0x4fc > > > > > > > > > > > > > > In my limited experience and knowlege it appears that we hit this > > > > > > > because the reset control was switched to shared and the the use > > > > > > > of the reset control was not changed. > > > > > > > > > > > > > > > * Calling reset_control_assert without first calling reset_control_deassert > > > > > > > > * is not allowed on a shared reset control. Calling reset_control_reset is > > > > > > > > * also not allowed on a shared reset control. > > > > > > > > > > > > > > The above snippet from reset_control_get_shared() seems to indicate that > > > > > > > this is due to the use of reset_control_reset() in dwc3_meson_g12a_probe() > > > > > > > and reset_control_deassert is not guaranteed to have been called > > > > > > > before dwc3_meson_g12a_suspend() and reset_control_assert(). > > > > > > > > > > > > > > After some basic tests with the following patch I no longer hit the > > > > > > > warning. Comments and critiques on the patch are welcome. If there is a > > > > > > > reason for the current use of the reset control, I'd love to learn why! > > > > > > > Like I said before, I have not really looked at this driver before and > > > > > > > have verify limited experience with reset controls... Was working on > > > > > > > another driver, hit the warning, and thought I'd take a shot at the > > > > > > > fix :-) > > > > > > > > > > > > > Thanks, I was also looking into this issue > > > > > > > > > > Awesome! > > > > > > > > > > > So best Fix to this issue is to drop the call of reset_control_assert > > > > > > from dwc3_meson_g12a_suspend > > > > > > and drop the call of reset_control_deassert from dwc3_meson_g12a_resume > > > > > > With these changes we do not see the warning on suspend and resume > > > > > > > > > > We definitely would avoid hitting the warning without the > > > > > reset_control_(de)assert calls in suspend and resume. That is a valid use of > > > > > the reset control, but why would that be best? > > > > > > > > > > From reset_control_reset(): > > > > > > > > Before entering the suspend state the code tries to do following > > > > clk_bulk_disable_unprepare > > > > regulator_disable > > > > phy_power_off > > > > phy_exit > > > > > > > > After this operation it's needless to call *reset_control_assert* > > > > I tried to move this call before all the above operations > > > > but still no success with this. > > > > > > How so? Once the reset() is removed prope() and deassert() is guaranteed > > > to have been called before suspend, like what is in the patch and similar > > > to other uses of shared reset controllers, this is possible. > > > > > > > Similarly on resume we should avoid calling resume > > > > *reset_control_deassert* earlier > > > > as the core is just reinitializing the devices. > > > > clk_bulk_prepare_enable > > > > usb_init > > > > phy_init > > > > phy_power_on > > > > regulator_enable > > > > > > > > > > > * Consumers must not use reset_control_(de)assert on shared reset lines when > > > > > > * reset_control_reset has been used. > > > > > > > > > > If we use reset_control_reset() then we can not (de)assert the reset line > > > > > on suspend/resume or any other time. Wouldn't we want to be able to > > > > > (de)assert the reset line? And why do we no longer want to (de)assert the > > > > > reset line on suspend/resume? > > > > > > > > > > > > Can you try this attached patch? > > > > > > > > > > > I think I had already tested something similar. Removing the (de)assert calls > > > > > but keeping reset will definitely remove the warning, but it means we can not > > > > > (de)assert the line. My guess is that this is not what we want, but I could be > > > > > wrong. Thoughts, input, or references to documentation on this would be > > > > > appreciated. > > > > > > > > > > > > > As per my knowledge suspend to mem will do complete power down of the > > > > devices with support suspend and resume feature sequentially and then it tries > > > > to bring the device up one by one. > > > > So it should also take care of reset lines as well. > > > > > > So do we only _actually_ care about the reset line in the probe? Seems like we > > > should remove the reset controller from the structure if that is the case. > > > > > > Cheers, > > > > > > - Dan > > > > Sorry I am not the _expert_ in suspend/resume feature but I can debug this, > > Certainly we need to look into whether reset controller calls are needed > > to suspend or resume for this driver. > > > > First thing we need to get the RTC driver to work on Odroid N2 for > > suspend wakeup > > to work properly. > > For this I have created the following patches. > > > > [0] https://patchwork.kernel.org/cover/11665865/ > > > > With these patches the RTC feature for wake up is broken right now in > > my testing. > > Once I get something to work further I will update you. > > > > --Anand > > Sorry for the _noise_ :‑( > This feature seems to be working fine with VRTC drivers. > I have tested this with a pre-compiled image of Archlinux distro. > > [root@alarm alarm]# uname -a > Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux > [root@alarm alarm]# rtcwake -s 30 -m mem > rtcwake: assuming RTC uses UTC ... > rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan 1 00:10:14 1970 > [ 583.591477] PM: suspend entry (deep) > [ 583.593737] Filesystems sync: 0.002 seconds > [ 583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done. > [ 583.825802] OOM killer disabled. > [ 583.828966] Freezing remaining freezable tasks ... (elapsed 0.003 > seconds) done. > [ 583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache > [ 584.020094] PM: suspend devices took 0.190 seconds > [ 584.070586] Disabling non-boot CPUs ... > [ 584.075037] CPU1: shutdown > [ 584.075223] psci: CPU1 killed (polled 0 ms) > [ 584.097199] CPU2: shutdown > [ 584.098546] psci: CPU2 killed (polled 0 ms) > [ 584.115370] CPU3: shutdown > [ 584.116500] psci: CPU3 killed (polled 0 ms) > [ 584.128116] CPU4: shutdown > [ 584.129235] psci: CPU4 killed (polled 10 ms) > [ 584.140122] CPU5: shutdown > [ 584.147289] psci: CPU5 killed (polled 0 ms) > bl30 get wakeup sources! > process command 00000006 > bl30 enter suspend! > Little core clk suspend rate 1896000000 > Big core clk suspend rate 24000000 > store restore gp0 pll > suspend_counter: 3 > Enter ddr suspend > ddr suspend time: 16us > alarm=31S > process command 00000001 > GPIOA_11/13 off > cec ver:2018/04/19 > CEC cfg:0x0000 > WAKEUP GPIO cfg:0x00000000 > use vddee new table! > use vddee new table! > exit_reason:0x03 > Enter ddr resume > DMC_DRAM_STAT3: 0x544 > ddr resume time: 3188us > store restore gp0 pll > cfg15 33b00000 > cfg15 33b00000 > Li[ 584.148720] Enabling non-boot CPUs ... > [ 584.149124] Detected VIPT I-cache on CPU1 > [ 584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034] > [ 584.149594] CPU1 is up > [ 584.160687] Detected VIPT I-cache on CPU2 > [ 584.160730] arch_timer: CPU2: Trapping CNTVCT access > [ 584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092] > [ 584.161327] CPU2 is up > [ 584.177645] Detected VIPT I-cache on CPU3 > [ 584.177668] arch_timer: CPU3: Trapping CNTVCT access > [ 584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092] > [ 584.178036] CPU3 is up > [ 584.195338] Detected VIPT I-cache on CPU4 > [ 584.195361] arch_timer: CPU4: Trapping CNTVCT access > [ 584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092] > [ 584.195762] CPU4 is up > [ 584.213002] Detected VIPT I-cache on CPU5 > [ 584.213024] arch_timer: CPU5: Trapping CNTVCT access > [ 584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092] > [ 584.213450] CPU5 is up > ttle core clk resume rate 1896000000 > Big core clk resume rate 50000000 > [ 584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety > Features support found > [ 584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for > phy/rgmii link mode > [ 584.401216] usb usb1: root hub lost power or was reset > [ 584.401470] usb usb2: root hub lost power or was reset > [ 584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode > [ 584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 > using xhci-hcd > [ 584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd > [ 585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3 > using xhci-hcd > [ 585.333303] PM: resume devices took 1.100 seconds > [ 585.333507] OOM killer enabled. > [ 585.335549] Restarting tasks ... done. > [ 585.378044] PM: suspend exit > rtcwake: read rtc alarm failed: Invalid argument > [root@alarm alarm]# > > -Anand After confirming that the suspend resume feature is working correctly I found the solution to the reset warning on 5.8.x kernel Please can you try this following patch. $ cat resetwarn.patch diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c index 1f7f4d88ed9d..60a6f49139fd 100644 --- a/drivers/usb/dwc3/dwc3-meson-g12a.c +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev) platform_set_drvdata(pdev, priv); - priv->reset = devm_reset_control_get_shared(dev, NULL); + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); if (IS_ERR(priv->reset)) { ret = PTR_ERR(priv->reset); dev_err(dev, "failed to get device reset, err=%d\n", ret); -Anand _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-17 16:38 ` Anand Moon @ 2020-07-18 6:31 ` Anand Moon 2020-07-18 8:46 ` Neil Armstrong 0 siblings, 1 reply; 31+ messages in thread From: Anand Moon @ 2020-07-18 6:31 UTC (permalink / raw) To: Dan Robertson Cc: Neil Armstrong, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel Hi Dan, > > Sorry for the _noise_ :‑( > > This feature seems to be working fine with VRTC drivers. > > I have tested this with a pre-compiled image of Archlinux distro. > > > > [root@alarm alarm]# uname -a > > Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux > > [root@alarm alarm]# rtcwake -s 30 -m mem > > rtcwake: assuming RTC uses UTC ... > > rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan 1 00:10:14 1970 > > [ 583.591477] PM: suspend entry (deep) > > [ 583.593737] Filesystems sync: 0.002 seconds > > [ 583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done. > > [ 583.825802] OOM killer disabled. > > [ 583.828966] Freezing remaining freezable tasks ... (elapsed 0.003 > > seconds) done. > > [ 583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache > > [ 584.020094] PM: suspend devices took 0.190 seconds > > [ 584.070586] Disabling non-boot CPUs ... > > [ 584.075037] CPU1: shutdown > > [ 584.075223] psci: CPU1 killed (polled 0 ms) > > [ 584.097199] CPU2: shutdown > > [ 584.098546] psci: CPU2 killed (polled 0 ms) > > [ 584.115370] CPU3: shutdown > > [ 584.116500] psci: CPU3 killed (polled 0 ms) > > [ 584.128116] CPU4: shutdown > > [ 584.129235] psci: CPU4 killed (polled 10 ms) > > [ 584.140122] CPU5: shutdown > > [ 584.147289] psci: CPU5 killed (polled 0 ms) > > bl30 get wakeup sources! > > process command 00000006 > > bl30 enter suspend! > > Little core clk suspend rate 1896000000 > > Big core clk suspend rate 24000000 > > store restore gp0 pll > > suspend_counter: 3 > > Enter ddr suspend > > ddr suspend time: 16us > > alarm=31S > > process command 00000001 > > GPIOA_11/13 off > > cec ver:2018/04/19 > > CEC cfg:0x0000 > > WAKEUP GPIO cfg:0x00000000 > > use vddee new table! > > use vddee new table! > > exit_reason:0x03 > > Enter ddr resume > > DMC_DRAM_STAT3: 0x544 > > ddr resume time: 3188us > > store restore gp0 pll > > cfg15 33b00000 > > cfg15 33b00000 > > Li[ 584.148720] Enabling non-boot CPUs ... > > [ 584.149124] Detected VIPT I-cache on CPU1 > > [ 584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034] > > [ 584.149594] CPU1 is up > > [ 584.160687] Detected VIPT I-cache on CPU2 > > [ 584.160730] arch_timer: CPU2: Trapping CNTVCT access > > [ 584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092] > > [ 584.161327] CPU2 is up > > [ 584.177645] Detected VIPT I-cache on CPU3 > > [ 584.177668] arch_timer: CPU3: Trapping CNTVCT access > > [ 584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092] > > [ 584.178036] CPU3 is up > > [ 584.195338] Detected VIPT I-cache on CPU4 > > [ 584.195361] arch_timer: CPU4: Trapping CNTVCT access > > [ 584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092] > > [ 584.195762] CPU4 is up > > [ 584.213002] Detected VIPT I-cache on CPU5 > > [ 584.213024] arch_timer: CPU5: Trapping CNTVCT access > > [ 584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092] > > [ 584.213450] CPU5 is up > > ttle core clk resume rate 1896000000 > > Big core clk resume rate 50000000 > > [ 584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety > > Features support found > > [ 584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for > > phy/rgmii link mode > > [ 584.401216] usb usb1: root hub lost power or was reset > > [ 584.401470] usb usb2: root hub lost power or was reset > > [ 584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode > > [ 584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 > > using xhci-hcd > > [ 584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd > > [ 585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3 > > using xhci-hcd > > [ 585.333303] PM: resume devices took 1.100 seconds > > [ 585.333507] OOM killer enabled. > > [ 585.335549] Restarting tasks ... done. > > [ 585.378044] PM: suspend exit > > rtcwake: read rtc alarm failed: Invalid argument > > [root@alarm alarm]# > > > > -Anand > > After confirming that the suspend resume feature is working correctly > I found the solution to the reset warning on 5.8.x kernel > Please can you try this following patch. > > $ cat resetwarn.patch > diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c > b/drivers/usb/dwc3/dwc3-meson-g12a.c > index 1f7f4d88ed9d..60a6f49139fd 100644 > --- a/drivers/usb/dwc3/dwc3-meson-g12a.c > +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c > @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct > platform_device *pdev) > > platform_set_drvdata(pdev, priv); > > - priv->reset = devm_reset_control_get_shared(dev, NULL); > + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); > if (IS_ERR(priv->reset)) { > ret = PTR_ERR(priv->reset); > dev_err(dev, "failed to get device reset, err=%d\n", ret); > > -Anand Apologize once again above changes break the usb functionality. the correct fix along with these changes should be as below. reset controllers need *resets* and *reset-names* to work correctly. But the _reset controller_ warning continues on suspend / resume features, I am looking to find a FIX into this issue. diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi index 593a006f4b7b..6d34dfa9825c 100644 --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi @@ -2318,6 +2318,7 @@ usb: usb@ffe09000 { clocks = <&clkc CLKID_USB>; resets = <&reset RESET_USB>; + reset-names = "dwc3_meson"; dr_mode = "otg"; -Anand _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply related [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-18 6:31 ` Anand Moon @ 2020-07-18 8:46 ` Neil Armstrong 2020-07-18 9:54 ` Anand Moon 0 siblings, 1 reply; 31+ messages in thread From: Neil Armstrong @ 2020-07-18 8:46 UTC (permalink / raw) To: Anand Moon, Dan Robertson Cc: Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-arm-kernel, linux-amlogic Hi Anand, Le 18/07/2020 à 08:31, Anand Moon a écrit : > Hi Dan, > >>> Sorry for the _noise_ :‑( >>> This feature seems to be working fine with VRTC drivers. >>> I have tested this with a pre-compiled image of Archlinux distro. >>> >>> [root@alarm alarm]# uname -a >>> Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux >>> [root@alarm alarm]# rtcwake -s 30 -m mem >>> rtcwake: assuming RTC uses UTC ... >>> rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan 1 00:10:14 1970 >>> [ 583.591477] PM: suspend entry (deep) >>> [ 583.593737] Filesystems sync: 0.002 seconds >>> [ 583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done. >>> [ 583.825802] OOM killer disabled. >>> [ 583.828966] Freezing remaining freezable tasks ... (elapsed 0.003 >>> seconds) done. >>> [ 583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache >>> [ 584.020094] PM: suspend devices took 0.190 seconds >>> [ 584.070586] Disabling non-boot CPUs ... >>> [ 584.075037] CPU1: shutdown >>> [ 584.075223] psci: CPU1 killed (polled 0 ms) >>> [ 584.097199] CPU2: shutdown >>> [ 584.098546] psci: CPU2 killed (polled 0 ms) >>> [ 584.115370] CPU3: shutdown >>> [ 584.116500] psci: CPU3 killed (polled 0 ms) >>> [ 584.128116] CPU4: shutdown >>> [ 584.129235] psci: CPU4 killed (polled 10 ms) >>> [ 584.140122] CPU5: shutdown >>> [ 584.147289] psci: CPU5 killed (polled 0 ms) >>> bl30 get wakeup sources! >>> process command 00000006 >>> bl30 enter suspend! >>> Little core clk suspend rate 1896000000 >>> Big core clk suspend rate 24000000 >>> store restore gp0 pll >>> suspend_counter: 3 >>> Enter ddr suspend >>> ddr suspend time: 16us >>> alarm=31S >>> process command 00000001 >>> GPIOA_11/13 off >>> cec ver:2018/04/19 >>> CEC cfg:0x0000 >>> WAKEUP GPIO cfg:0x00000000 >>> use vddee new table! >>> use vddee new table! >>> exit_reason:0x03 >>> Enter ddr resume >>> DMC_DRAM_STAT3: 0x544 >>> ddr resume time: 3188us >>> store restore gp0 pll >>> cfg15 33b00000 >>> cfg15 33b00000 >>> Li[ 584.148720] Enabling non-boot CPUs ... >>> [ 584.149124] Detected VIPT I-cache on CPU1 >>> [ 584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034] >>> [ 584.149594] CPU1 is up >>> [ 584.160687] Detected VIPT I-cache on CPU2 >>> [ 584.160730] arch_timer: CPU2: Trapping CNTVCT access >>> [ 584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092] >>> [ 584.161327] CPU2 is up >>> [ 584.177645] Detected VIPT I-cache on CPU3 >>> [ 584.177668] arch_timer: CPU3: Trapping CNTVCT access >>> [ 584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092] >>> [ 584.178036] CPU3 is up >>> [ 584.195338] Detected VIPT I-cache on CPU4 >>> [ 584.195361] arch_timer: CPU4: Trapping CNTVCT access >>> [ 584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092] >>> [ 584.195762] CPU4 is up >>> [ 584.213002] Detected VIPT I-cache on CPU5 >>> [ 584.213024] arch_timer: CPU5: Trapping CNTVCT access >>> [ 584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092] >>> [ 584.213450] CPU5 is up >>> ttle core clk resume rate 1896000000 >>> Big core clk resume rate 50000000 >>> [ 584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety >>> Features support found >>> [ 584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for >>> phy/rgmii link mode >>> [ 584.401216] usb usb1: root hub lost power or was reset >>> [ 584.401470] usb usb2: root hub lost power or was reset >>> [ 584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode >>> [ 584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 >>> using xhci-hcd >>> [ 584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd >>> [ 585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3 >>> using xhci-hcd >>> [ 585.333303] PM: resume devices took 1.100 seconds >>> [ 585.333507] OOM killer enabled. >>> [ 585.335549] Restarting tasks ... done. >>> [ 585.378044] PM: suspend exit >>> rtcwake: read rtc alarm failed: Invalid argument >>> [root@alarm alarm]# >>> >>> -Anand >> >> After confirming that the suspend resume feature is working correctly >> I found the solution to the reset warning on 5.8.x kernel >> Please can you try this following patch. >> >> $ cat resetwarn.patch >> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c >> b/drivers/usb/dwc3/dwc3-meson-g12a.c >> index 1f7f4d88ed9d..60a6f49139fd 100644 >> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c >> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c >> @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct >> platform_device *pdev) >> >> platform_set_drvdata(pdev, priv); >> >> - priv->reset = devm_reset_control_get_shared(dev, NULL); >> + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); >> if (IS_ERR(priv->reset)) { >> ret = PTR_ERR(priv->reset); >> dev_err(dev, "failed to get device reset, err=%d\n", ret); >> >> -Anand > > Apologize once again above changes break the usb functionality. > the correct fix along with these changes should be as below. > reset controllers need *resets* and *reset-names* to work correctly. > > But the _reset controller_ warning continues on suspend / resume features, > I am looking to find a FIX into this issue. > > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > index 593a006f4b7b..6d34dfa9825c 100644 > --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > @@ -2318,6 +2318,7 @@ usb: usb@ffe09000 { > > clocks = <&clkc CLKID_USB>; > resets = <&reset RESET_USB>; > + reset-names = "dwc3_meson"; > > dr_mode = "otg"; > > -Anand > Theses 2 changes : >> - priv->reset = devm_reset_control_get_shared(dev, NULL); >> + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); and > + reset-names = "dwc3_meson"; are a no-op, since devm_reset_control_get_shared(dev, NULL) takes the first reset, with or without a reset-names. Neil _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-18 8:46 ` Neil Armstrong @ 2020-07-18 9:54 ` Anand Moon 0 siblings, 0 replies; 31+ messages in thread From: Anand Moon @ 2020-07-18 9:54 UTC (permalink / raw) To: Neil Armstrong Cc: Dan Robertson, Martin Blumenstingl, Kevin Hilman, Linux USB Mailing List, linux-amlogic, linux-arm-kernel Hi Neil, On Sat, 18 Jul 2020 at 14:16, Neil Armstrong <narmstrong@baylibre.com> wrote: > > Hi Anand, > > Le 18/07/2020 à 08:31, Anand Moon a écrit : > > Hi Dan, > > > >>> Sorry for the _noise_ :‑( > >>> This feature seems to be working fine with VRTC drivers. > >>> I have tested this with a pre-compiled image of Archlinux distro. > >>> > >>> [root@alarm alarm]# uname -a > >>> Linux alarm 5.7.8-1-ARCH #1 SMP Sun Jul 12 03:38:28 UTC 2020 aarch64 GNU/Linux > >>> [root@alarm alarm]# rtcwake -s 30 -m mem > >>> rtcwake: assuming RTC uses UTC ... > >>> rtcwake: wakeup from "mem" using /dev/rtc0 at Thu Jan 1 00:10:14 1970 > >>> [ 583.591477] PM: suspend entry (deep) > >>> [ 583.593737] Filesystems sync: 0.002 seconds > >>> [ 583.818967] Freezing user space processes ... (elapsed 0.005 seconds) done. > >>> [ 583.825802] OOM killer disabled. > >>> [ 583.828966] Freezing remaining freezable tasks ... (elapsed 0.003 > >>> seconds) done. > >>> [ 583.880280] sd 0:0:0:0: [sda] Synchronizing SCSI cache > >>> [ 584.020094] PM: suspend devices took 0.190 seconds > >>> [ 584.070586] Disabling non-boot CPUs ... > >>> [ 584.075037] CPU1: shutdown > >>> [ 584.075223] psci: CPU1 killed (polled 0 ms) > >>> [ 584.097199] CPU2: shutdown > >>> [ 584.098546] psci: CPU2 killed (polled 0 ms) > >>> [ 584.115370] CPU3: shutdown > >>> [ 584.116500] psci: CPU3 killed (polled 0 ms) > >>> [ 584.128116] CPU4: shutdown > >>> [ 584.129235] psci: CPU4 killed (polled 10 ms) > >>> [ 584.140122] CPU5: shutdown > >>> [ 584.147289] psci: CPU5 killed (polled 0 ms) > >>> bl30 get wakeup sources! > >>> process command 00000006 > >>> bl30 enter suspend! > >>> Little core clk suspend rate 1896000000 > >>> Big core clk suspend rate 24000000 > >>> store restore gp0 pll > >>> suspend_counter: 3 > >>> Enter ddr suspend > >>> ddr suspend time: 16us > >>> alarm=31S > >>> process command 00000001 > >>> GPIOA_11/13 off > >>> cec ver:2018/04/19 > >>> CEC cfg:0x0000 > >>> WAKEUP GPIO cfg:0x00000000 > >>> use vddee new table! > >>> use vddee new table! > >>> exit_reason:0x03 > >>> Enter ddr resume > >>> DMC_DRAM_STAT3: 0x544 > >>> ddr resume time: 3188us > >>> store restore gp0 pll > >>> cfg15 33b00000 > >>> cfg15 33b00000 > >>> Li[ 584.148720] Enabling non-boot CPUs ... > >>> [ 584.149124] Detected VIPT I-cache on CPU1 > >>> [ 584.149167] CPU1: Booted secondary processor 0x0000000001 [0x410fd034] > >>> [ 584.149594] CPU1 is up > >>> [ 584.160687] Detected VIPT I-cache on CPU2 > >>> [ 584.160730] arch_timer: CPU2: Trapping CNTVCT access > >>> [ 584.160741] CPU2: Booted secondary processor 0x0000000100 [0x410fd092] > >>> [ 584.161327] CPU2 is up > >>> [ 584.177645] Detected VIPT I-cache on CPU3 > >>> [ 584.177668] arch_timer: CPU3: Trapping CNTVCT access > >>> [ 584.177675] CPU3: Booted secondary processor 0x0000000101 [0x410fd092] > >>> [ 584.178036] CPU3 is up > >>> [ 584.195338] Detected VIPT I-cache on CPU4 > >>> [ 584.195361] arch_timer: CPU4: Trapping CNTVCT access > >>> [ 584.195368] CPU4: Booted secondary processor 0x0000000102 [0x410fd092] > >>> [ 584.195762] CPU4 is up > >>> [ 584.213002] Detected VIPT I-cache on CPU5 > >>> [ 584.213024] arch_timer: CPU5: Trapping CNTVCT access > >>> [ 584.213032] CPU5: Booted secondary processor 0x0000000103 [0x410fd092] > >>> [ 584.213450] CPU5 is up > >>> ttle core clk resume rate 1896000000 > >>> Big core clk resume rate 50000000 > >>> [ 584.279042] meson8b-dwmac ff3f0000.ethernet eth0: No Safety > >>> Features support found > >>> [ 584.281232] meson8b-dwmac ff3f0000.ethernet eth0: configuring for > >>> phy/rgmii link mode > >>> [ 584.401216] usb usb1: root hub lost power or was reset > >>> [ 584.401470] usb usb2: root hub lost power or was reset > >>> [ 584.655446] dwc3-meson-g12a ffe09000.usb: switching to Device Mode > >>> [ 584.801108] usb 2-1: reset SuperSpeed Gen 1 USB device number 2 > >>> using xhci-hcd > >>> [ 584.979632] usb 1-1: reset high-speed USB device number 2 using xhci-hcd > >>> [ 585.260450] usb 2-1.1: reset SuperSpeed Gen 1 USB device number 3 > >>> using xhci-hcd > >>> [ 585.333303] PM: resume devices took 1.100 seconds > >>> [ 585.333507] OOM killer enabled. > >>> [ 585.335549] Restarting tasks ... done. > >>> [ 585.378044] PM: suspend exit > >>> rtcwake: read rtc alarm failed: Invalid argument > >>> [root@alarm alarm]# > >>> > >>> -Anand > >> > >> After confirming that the suspend resume feature is working correctly > >> I found the solution to the reset warning on 5.8.x kernel > >> Please can you try this following patch. > >> > >> $ cat resetwarn.patch > >> diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c > >> b/drivers/usb/dwc3/dwc3-meson-g12a.c > >> index 1f7f4d88ed9d..60a6f49139fd 100644 > >> --- a/drivers/usb/dwc3/dwc3-meson-g12a.c > >> +++ b/drivers/usb/dwc3/dwc3-meson-g12a.c > >> @@ -730,7 +730,7 @@ static int dwc3_meson_g12a_probe(struct > >> platform_device *pdev) > >> > >> platform_set_drvdata(pdev, priv); > >> > >> - priv->reset = devm_reset_control_get_shared(dev, NULL); > >> + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); > >> if (IS_ERR(priv->reset)) { > >> ret = PTR_ERR(priv->reset); > >> dev_err(dev, "failed to get device reset, err=%d\n", ret); > >> > >> -Anand > > > > Apologize once again above changes break the usb functionality. > > the correct fix along with these changes should be as below. > > reset controllers need *resets* and *reset-names* to work correctly. > > > > But the _reset controller_ warning continues on suspend / resume features, > > I am looking to find a FIX into this issue. > > > > diff --git a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > > b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > > index 593a006f4b7b..6d34dfa9825c 100644 > > --- a/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > > +++ b/arch/arm64/boot/dts/amlogic/meson-g12-common.dtsi > > @@ -2318,6 +2318,7 @@ usb: usb@ffe09000 { > > > > clocks = <&clkc CLKID_USB>; > > resets = <&reset RESET_USB>; > > + reset-names = "dwc3_meson"; > > > > dr_mode = "otg"; > > > > -Anand > > > > > Theses 2 changes : > > >> - priv->reset = devm_reset_control_get_shared(dev, NULL); > >> + priv->reset = devm_reset_control_get_shared(dev, "dwc3_meson"); > > and > > > + reset-names = "dwc3_meson"; > > are a no-op, since devm_reset_control_get_shared(dev, NULL) takes the > first reset, with or without a reset-names. > Thanks for resolving my query. Sorry for the noise. > Neil -Anand _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic ^ permalink raw reply [flat|nested] 31+ messages in thread
* Re: [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use 2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson 2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson 2020-07-14 6:56 ` [PATCH 0/1] " Anand Moon @ 2020-08-17 17:48 ` patchwork-bot+linux-amlogic 2 siblings, 0 replies; 31+ messages in thread From: patchwork-bot+linux-amlogic @ 2020-08-17 17:48 UTC (permalink / raw) To: Dan Robertson; +Cc: linux-amlogic, khilman 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 ^ permalink raw reply [flat|nested] 31+ messages in thread
end of thread, other threads:[~2020-09-07 8:34 UTC | newest] Thread overview: 31+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-13 16:05 [PATCH 0/1] usb: dwc3: meson-g12a: fix shared reset control use Dan Robertson 2020-07-13 16:05 ` [PATCH 1/1] " Dan Robertson 2020-07-18 8:47 ` Neil Armstrong 2020-07-18 22:57 ` Dan Robertson 2020-08-19 15:03 ` Jerome Brunet 2020-08-20 18:02 ` Kevin Hilman 2020-08-20 18:27 ` Jerome Brunet 2020-08-20 18:49 ` Kevin Hilman 2020-08-20 18:44 ` Dan Robertson 2020-08-24 8:14 ` Jerome Brunet 2020-08-24 10:24 ` Philipp Zabel 2020-08-24 14:26 ` Jerome Brunet 2020-08-25 10:20 ` Philipp Zabel 2020-08-25 14:20 ` Jerome Brunet 2020-08-26 8:14 ` Philipp Zabel 2020-08-26 8:34 ` Jerome Brunet 2020-08-29 15:25 ` Martin Blumenstingl 2020-09-02 14:13 ` Amjad Ouled-Ameur 2020-09-07 8:31 ` Jerome Brunet 2020-09-07 8:33 ` Amjad Ouled-Ameur 2020-07-14 6:56 ` [PATCH 0/1] " Anand Moon 2020-07-14 13:30 ` Dan Robertson 2020-07-14 15:27 ` Anand Moon 2020-07-15 2:58 ` Dan Robertson 2020-07-15 16:23 ` Anand Moon 2020-07-17 9:01 ` Anand Moon 2020-07-17 16:38 ` Anand Moon 2020-07-18 6:31 ` Anand Moon 2020-07-18 8:46 ` Neil Armstrong 2020-07-18 9:54 ` Anand Moon 2020-08-17 17:48 ` patchwork-bot+linux-amlogic
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).