| Message ID | 20200713160522.19345-2-dan@dlrobertson.com (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | 7a410953d1fb4dbe91ffcfdee9cbbf889d19b0d7 |
| Headers | show |
| Series | usb: dwc3: meson-g12a: fix shared reset control use | expand |
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
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
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
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
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
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 >
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
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 >>
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
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
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
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
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
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
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
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
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
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
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);
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(-)