This patchset fixes a usb suspend warning seen on the libretech-cc by
using reset_control_rearm() call of the reset framework API.
This call allows a reset consummer to release the reset line even when
just triggered so that it may be triggered again by other reset
consummers.
reset_control_(de)assert() calls are called, in some meson usb drivers,
on a shared reset line when reset_control_reset has been used. This is not
allowed by the reset framework.
Finally the meson usb drivers are updated to use this new call, which
solves the suspend issue addressed by the previous reverted
commit 7a410953d1fb ("usb: dwc3: meson-g12a: fix shared reset control
use").
Important:
Please DO NOT merge before this patch [0] is merged, it adds
reset_control_rearm() call to the reset framework API.
[0] https://lore.kernel.org/lkml/20201112230043.28987-1-aouledameur@baylib
re.com/
Amjad Ouled-Ameur (3):
phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use
usb: dwc3: meson-g12a: fix shared reset control use
phy: amlogic: meson8b-usb2: fix shared reset control use
drivers/phy/amlogic/phy-meson-gxl-usb2.c | 5 ++++-
drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++
drivers/usb/dwc3/dwc3-meson-g12a.c | 19 +++++++++++++------
3 files changed, 21 insertions(+), 7 deletions(-)
--
2.17.1
Use reset_control_rearm() call if an error occurs in case
phy_meson_gxl_usb2_init() fails after reset() has been called ; or in case
phy_meson_gxl_usb2_exit() is called i.e the resource is no longer used
and the reset line may be triggered again by other devices.
reset_control_rearm() keeps use of triggered_count sane in the reset
framework. Therefore, use of reset_control_reset() on shared reset line
should be balanced with reset_control_rearm().
Signed-off-by: Amjad Ouled-Ameur <[email protected]>
Reported-by: Jerome Brunet <[email protected]>
---
Important:
Please DO NOT merge before this patch [0] is merged, it adds
reset_control_rearm() call to the reset framework API.
[0] https://lore.kernel.org/lkml/20201112230043.28987-1-aouledameur@baylib
re.com/
drivers/phy/amlogic/phy-meson-gxl-usb2.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/phy/amlogic/phy-meson-gxl-usb2.c b/drivers/phy/amlogic/phy-meson-gxl-usb2.c
index 43ec9bf24abf..e366c0b1e339 100644
--- a/drivers/phy/amlogic/phy-meson-gxl-usb2.c
+++ b/drivers/phy/amlogic/phy-meson-gxl-usb2.c
@@ -114,8 +114,10 @@ static int phy_meson_gxl_usb2_init(struct phy *phy)
return ret;
ret = clk_prepare_enable(priv->clk);
- if (ret)
+ if (ret) {
+ reset_control_rearm(priv->reset);
return ret;
+ }
return 0;
}
@@ -124,6 +126,7 @@ static int phy_meson_gxl_usb2_exit(struct phy *phy)
{
struct phy_meson_gxl_usb2_priv *priv = phy_get_drvdata(phy);
+ reset_control_rearm(priv->reset);
clk_disable_unprepare(priv->clk);
return 0;
--
2.17.1
reset_control_(de)assert() calls are called on a shared reset line when
reset_control_reset has been used. This is not allowed by the reset
framework.
Use reset_control_rearm() call in suspend() and remove() as a way to state
that the resource is no longer used, hence the shared reset line
may be triggered again by other devices. Use reset_control_rearm() also in
case probe fails after reset() has been called.
reset_control_rearm() keeps use of triggered_count sane in the reset
framework, use of reset_control_reset() on shared reset line should be
balanced with reset_control_rearm().
Signed-off-by: Amjad Ouled-Ameur <[email protected]>
Reported-by: Jerome Brunet <[email protected]>
---
Important:
Please DO NOT merge before this patch [0] is merged, it adds
reset_control_rearm() call to the reset framework API.
[0] https://lore.kernel.org/lkml/20201112230043.28987-1-aouledameur@baylib
re.com/
drivers/usb/dwc3/dwc3-meson-g12a.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index 417e05381b5d..3fec8f591c93 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -750,7 +750,7 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
ret = dwc3_meson_g12a_get_phys(priv);
if (ret)
- goto err_disable_clks;
+ goto err_rearm;
ret = priv->drvdata->setup_regmaps(priv, base);
if (ret)
@@ -759,7 +759,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_rearm;
}
/* Get dr_mode */
@@ -772,13 +772,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_rearm;
/* Init PHYs */
for (i = 0 ; i < PHY_COUNT ; ++i) {
ret = phy_init(priv->phys[i]);
if (ret)
- goto err_disable_clks;
+ goto err_rearm;
}
/* Set PHY Power */
@@ -816,6 +816,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
for (i = 0 ; i < PHY_COUNT ; ++i)
phy_exit(priv->phys[i]);
+err_rearm:
+ reset_control_rearm(priv->reset);
+
err_disable_clks:
clk_bulk_disable_unprepare(priv->drvdata->num_clks,
priv->drvdata->clks);
@@ -843,6 +846,8 @@ static int dwc3_meson_g12a_remove(struct platform_device *pdev)
pm_runtime_put_noidle(dev);
pm_runtime_set_suspended(dev);
+ reset_control_rearm(priv->reset);
+
clk_bulk_disable_unprepare(priv->drvdata->num_clks,
priv->drvdata->clks);
@@ -883,7 +888,7 @@ static int __maybe_unused dwc3_meson_g12a_suspend(struct device *dev)
phy_exit(priv->phys[i]);
}
- reset_control_assert(priv->reset);
+ reset_control_rearm(priv->reset);
return 0;
}
@@ -893,7 +898,9 @@ 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 = reset_control_reset(priv->reset);
+ if (ret)
+ return ret;
ret = priv->drvdata->usb_init(priv);
if (ret)
--
2.17.1
Use reset_control_rearm() call if an error occurs in case
phy_meson8b_usb2_power_on() fails after reset() has been called, or in
case phy_meson8b_usb2_power_off() is called i.e the resource is no longer
used and the reset line may be triggered again by other devices.
reset_control_rearm() keeps use of triggered_count sane in the reset
framework, use of reset_control_reset() on shared reset line should
be balanced with reset_control_rearm().
Signed-off-by: Amjad Ouled-Ameur <[email protected]>
Reported-by: Jerome Brunet <[email protected]>
---
Important:
Please DO NOT merge before this patch [0] is merged, it adds
reset_control_rearm() call to the reset framework API.
drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/phy/amlogic/phy-meson8b-usb2.c b/drivers/phy/amlogic/phy-meson8b-usb2.c
index 03c061dd5f0d..2a6c53f52423 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -154,12 +154,14 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
ret = clk_prepare_enable(priv->clk_usb_general);
if (ret) {
dev_err(&phy->dev, "Failed to enable USB general clock\n");
+ reset_control_rearm(priv->reset);
return ret;
}
ret = clk_prepare_enable(priv->clk_usb);
if (ret) {
dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
+ reset_control_rearm(priv->reset);
clk_disable_unprepare(priv->clk_usb_general);
return ret;
}
@@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
regmap_read(priv->regmap, REG_ADP_BC, ®);
if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
dev_warn(&phy->dev, "USB ID detect failed!\n");
+ reset_control_rearm(priv->reset);
clk_disable_unprepare(priv->clk_usb);
clk_disable_unprepare(priv->clk_usb_general);
return -EINVAL;
@@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
REG_DBG_UART_SET_IDDQ,
REG_DBG_UART_SET_IDDQ);
+ reset_control_rearm(priv->reset);
clk_disable_unprepare(priv->clk_usb);
clk_disable_unprepare(priv->clk_usb_general);
--
2.17.1
Hi Amjad,
On Fri, Nov 13, 2020 at 1:07 AM Amjad Ouled-Ameur
<[email protected]> wrote:
[...]
> ret = clk_prepare_enable(priv->clk_usb);
> if (ret) {
> dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
> + reset_control_rearm(priv->reset);
this should come after reset_control_rearm so we're cleaning up in
reverse order of initializing things
(in this case it probably makes no difference since
reset_control_rearm is not touching any registers, but I'd still have
it in the correct order to not confuse future developers)
> clk_disable_unprepare(priv->clk_usb_general);
> return ret;
> }
> @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
> regmap_read(priv->regmap, REG_ADP_BC, ®);
> if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
> dev_warn(&phy->dev, "USB ID detect failed!\n");
> + reset_control_rearm(priv->reset);
same here, reset_control_rearm should be after clk_disable_unprepare
> clk_disable_unprepare(priv->clk_usb);
> clk_disable_unprepare(priv->clk_usb_general);
> return -EINVAL;
> @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
> REG_DBG_UART_SET_IDDQ,
> REG_DBG_UART_SET_IDDQ);
>
> + reset_control_rearm(priv->reset);
same here, reset_control_rearm should be after clk_disable_unprepare
> clk_disable_unprepare(priv->clk_usb);
> clk_disable_unprepare(priv->clk_usb_general);
Best regards,
Martin
Hi Amjad,
On Fri, Nov 13, 2020 at 1:06 AM Amjad Ouled-Ameur
<[email protected]> wrote:
[...]
> @@ -124,6 +126,7 @@ static int phy_meson_gxl_usb2_exit(struct phy *phy)
> {
> struct phy_meson_gxl_usb2_priv *priv = phy_get_drvdata(phy);
>
> + reset_control_rearm(priv->reset);
please move reset_control_rearm after clk_disable_unprepare as
mentioned on the other patch to clean things up in reverse order of
initialization
> clk_disable_unprepare(priv->clk);
Thank you
Martin
Hi Martin,
Thank you for the review !
On 14/11/2020 20:11, Martin Blumenstingl wrote:
> Hi Amjad,
>
> On Fri, Nov 13, 2020 at 1:07 AM Amjad Ouled-Ameur
> <[email protected]> wrote:
> [...]
>> ret = clk_prepare_enable(priv->clk_usb);
>> if (ret) {
>> dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
>> + reset_control_rearm(priv->reset);
> this should come after reset_control_rearm so we're cleaning up in
> reverse order of initializing things
> (in this case it probably makes no difference since
> reset_control_rearm is not touching any registers, but I'd still have
> it in the correct order to not confuse future developers)
Agreed, it works in this current order since the two lines do not
interfere with each other, but it is cleaner to do it in the reverse
order of initialization. Will fix it in next change.
>> clk_disable_unprepare(priv->clk_usb_general);
>> return ret;
>> }
>> @@ -197,6 +199,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
>> regmap_read(priv->regmap, REG_ADP_BC, ®);
>> if (reg & REG_ADP_BC_ACA_PIN_FLOAT) {
>> dev_warn(&phy->dev, "USB ID detect failed!\n");
>> + reset_control_rearm(priv->reset);
> same here, reset_control_rearm should be after clk_disable_unprepare
Ditto, will fix it in next change.
>
>> clk_disable_unprepare(priv->clk_usb);
>> clk_disable_unprepare(priv->clk_usb_general);
>> return -EINVAL;
>> @@ -216,6 +219,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
>> REG_DBG_UART_SET_IDDQ,
>> REG_DBG_UART_SET_IDDQ);
>>
>> + reset_control_rearm(priv->reset);
> same here, reset_control_rearm should be after clk_disable_unprepare
Ditto, will fix it in next change.
>
>> clk_disable_unprepare(priv->clk_usb);
>> clk_disable_unprepare(priv->clk_usb_general);
>
> Best regards,
> Martin
>
Sincerely,
Amjad
Hi Martin,
On 14/11/2020 20:13, Martin Blumenstingl wrote:
> Hi Amjad,
>
> On Fri, Nov 13, 2020 at 1:06 AM Amjad Ouled-Ameur
> <[email protected]> wrote:
> [...]
>> @@ -124,6 +126,7 @@ static int phy_meson_gxl_usb2_exit(struct phy *phy)
>> {
>> struct phy_meson_gxl_usb2_priv *priv = phy_get_drvdata(phy);
>>
>> + reset_control_rearm(priv->reset);
> please move reset_control_rearm after clk_disable_unprepare as
> mentioned on the other patch to clean things up in reverse order of
> initialization
Agreed, will fix it in next change !
>
>> clk_disable_unprepare(priv->clk);
>
> Thank you
> Martin
>
Thank you for the feedback !
Best,
Amjad
Hello Felipe and Kevin,
Could you please review this patchset ?
Thank you in advance.
On 13/11/2020 01:05, Amjad Ouled-Ameur wrote:
> This patchset fixes a usb suspend warning seen on the libretech-cc by
> using reset_control_rearm() call of the reset framework API.
> This call allows a reset consummer to release the reset line even when
> just triggered so that it may be triggered again by other reset
> consummers.
>
> reset_control_(de)assert() calls are called, in some meson usb drivers,
> on a shared reset line when reset_control_reset has been used. This is not
> allowed by the reset framework.
>
> Finally the meson usb drivers are updated to use this new call, which
> solves the suspend issue addressed by the previous reverted
> commit 7a410953d1fb ("usb: dwc3: meson-g12a: fix shared reset control
> use").
>
> Important:
> Please DO NOT merge before this patch [0] is merged, it adds
> reset_control_rearm() call to the reset framework API.
>
> [0] https://lore.kernel.org/lkml/20201112230043.28987-1-aouledameur@baylib
> re.com/
>
> Amjad Ouled-Ameur (3):
> phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use
> usb: dwc3: meson-g12a: fix shared reset control use
> phy: amlogic: meson8b-usb2: fix shared reset control use
>
> drivers/phy/amlogic/phy-meson-gxl-usb2.c | 5 ++++-
> drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++
> drivers/usb/dwc3/dwc3-meson-g12a.c | 19 +++++++++++++------
> 3 files changed, 21 insertions(+), 7 deletions(-)
>
>
Sincerely,
Amjad
Amjad Ouled-Ameur <[email protected]> writes:
> Hello Felipe and Kevin,
>
>
> Could you please review this patchset ?
The changes are OK with me. Please update based on Martin's
suggestions and this can be queued up by the USB maintainers.
Kevin
> Thank you in advance.
>
> On 13/11/2020 01:05, Amjad Ouled-Ameur wrote:
>
>> This patchset fixes a usb suspend warning seen on the libretech-cc by
>> using reset_control_rearm() call of the reset framework API.
>> This call allows a reset consummer to release the reset line even when
>> just triggered so that it may be triggered again by other reset
>> consummers.
>>
>> reset_control_(de)assert() calls are called, in some meson usb drivers,
>> on a shared reset line when reset_control_reset has been used. This is not
>> allowed by the reset framework.
>>
>> Finally the meson usb drivers are updated to use this new call, which
>> solves the suspend issue addressed by the previous reverted
>> commit 7a410953d1fb ("usb: dwc3: meson-g12a: fix shared reset control
>> use").
>>
>> Important:
>> Please DO NOT merge before this patch [0] is merged, it adds
>> reset_control_rearm() call to the reset framework API.
>>
>> [0] https://lore.kernel.org/lkml/20201112230043.28987-1-aouledameur@baylib
>> re.com/
>>
>> Amjad Ouled-Ameur (3):
>> phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use
>> usb: dwc3: meson-g12a: fix shared reset control use
>> phy: amlogic: meson8b-usb2: fix shared reset control use
>>
>> drivers/phy/amlogic/phy-meson-gxl-usb2.c | 5 ++++-
>> drivers/phy/amlogic/phy-meson8b-usb2.c | 4 ++++
>> drivers/usb/dwc3/dwc3-meson-g12a.c | 19 +++++++++++++------
>> 3 files changed, 21 insertions(+), 7 deletions(-)
>>
>>
> Sincerely,
> Amjad