[PATCH v3 0/3] usb: meson: fix shared reset control use

[PATCH v3 0/3] usb: meson: fix shared reset control use

[Amjad Ouled-Ameur]

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").

changes since v2 [0]:
- Rebased on latest master

[0]: https://lore.kernel.org/all/20201201190100.17831-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       | 17 ++++++++++++-----
 3 files changed, 20 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH v3 1/3] phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use

[Amjad Ouled-Ameur]

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 <aouledameur@baylibre.com>
Reported-by: Jerome Brunet <jbrunet@baylibre.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 2b3c0d730f20..9a9c769ecabc 100644
--- a/drivers/phy/amlogic/phy-meson-gxl-usb2.c
+++ b/drivers/phy/amlogic/phy-meson-gxl-usb2.c
@@ -110,8 +110,10 @@ static int phy_meson_gxl_usb2_init(struct phy *phy)
 	int ret;
 
 	ret = reset_control_reset(priv->reset);
-	if (ret)
+	if (ret) {
+		reset_control_rearm(priv->reset);
 		return ret;
+	}
 
 	ret = clk_prepare_enable(priv->clk);
 	if (ret)
@@ -125,6 +127,7 @@ static int phy_meson_gxl_usb2_exit(struct phy *phy)
 	struct phy_meson_gxl_usb2_priv *priv = phy_get_drvdata(phy);
 
 	clk_disable_unprepare(priv->clk);
+	reset_control_rearm(priv->reset);
 
 	return 0;
 }
-- 
2.25.1

Re: [PATCH v3 1/3] phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use

[Martin Blumenstingl]

On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur
<aouledameur@baylibre.com> wrote:
>
> 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 <aouledameur@baylibre.com>
> Reported-by: Jerome Brunet <jbrunet@baylibre.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Re: [PATCH v3 1/3] phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use

[Philipp Zabel]

Hi Amjad,

On Fri, 2021-11-12 at 17:28 +0100, Amjad Ouled-Ameur wrote:
> 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 <aouledameur@baylibre.com>
> Reported-by: Jerome Brunet <jbrunet@baylibre.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 2b3c0d730f20..9a9c769ecabc 100644
> --- a/drivers/phy/amlogic/phy-meson-gxl-usb2.c
> +++ b/drivers/phy/amlogic/phy-meson-gxl-usb2.c
> @@ -110,8 +110,10 @@ static int phy_meson_gxl_usb2_init(struct phy *phy)
>  	int ret;
>
>  	ret = reset_control_reset(priv->reset);
> -	if (ret)
> +	if (ret) {
> +		reset_control_rearm(priv->reset);

I don't understand this. If reset_control_reset() returns an error for a
shared reset, it should have either
- returned before incrementing triggered_count, or
- incremented triggered_count, got a failed reset op, decremented
  triggered_count again

In both cases there should be no need to rearm.


regards
Philipp

Re: [PATCH v3 1/3] phy: amlogic: phy-meson-gxl-usb2: fix shared reset controller use

[Amjad Ouled-Ameur]

Hi Philipp,

Thank you for the review.

On 22/11/2021 10:03, Philipp Zabel wrote:
> Hi Amjad,
>
> On Fri, 2021-11-12 at 17:28 +0100, Amjad Ouled-Ameur wrote:
>> 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 <aouledameur@baylibre.com>
>> Reported-by: Jerome Brunet <jbrunet@baylibre.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 2b3c0d730f20..9a9c769ecabc 100644
>> --- a/drivers/phy/amlogic/phy-meson-gxl-usb2.c
>> +++ b/drivers/phy/amlogic/phy-meson-gxl-usb2.c
>> @@ -110,8 +110,10 @@ static int phy_meson_gxl_usb2_init(struct phy *phy)
>>   	int ret;
>>
>>   	ret = reset_control_reset(priv->reset);
>> -	if (ret)
>> +	if (ret) {
>> +		reset_control_rearm(priv->reset);
> I don't understand this. If reset_control_reset() returns an error for a
> shared reset, it should have either
> - returned before incrementing triggered_count, or
> - incremented triggered_count, got a failed reset op, decremented
>    triggered_count again
>
> In both cases there should be no need to rearm.
>
I have checked this out and I agree with you, will remove this in next 
version.

Thank you for spotting this.


Regards,

Amjad

> regards
> Philipp

[PATCH v3 2/3] usb: dwc3: meson-g12a: fix shared reset control use

[Amjad Ouled-Ameur]

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 <aouledameur@baylibre.com>
Reported-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/usb/dwc3/dwc3-meson-g12a.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-meson-g12a.c b/drivers/usb/dwc3/dwc3-meson-g12a.c
index d0f9b7c296b0..bd814df3bf8b 100644
--- a/drivers/usb/dwc3/dwc3-meson-g12a.c
+++ b/drivers/usb/dwc3/dwc3-meson-g12a.c
@@ -755,16 +755,16 @@ 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)
-		goto err_disable_clks;
+		goto err_rearm;
 
 	if (priv->vbus) {
 		ret = regulator_enable(priv->vbus);
 		if (ret)
-			goto err_disable_clks;
+			goto err_rearm;
 	}
 
 	/* Get dr_mode */
@@ -825,6 +825,9 @@ static int dwc3_meson_g12a_probe(struct platform_device *pdev)
 	if (priv->vbus)
 		regulator_disable(priv->vbus);
 
+err_rearm:
+	reset_control_rearm(priv->reset);
+
 err_disable_clks:
 	clk_bulk_disable_unprepare(priv->drvdata->num_clks,
 				   priv->drvdata->clks);
@@ -852,6 +855,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);
 
@@ -892,7 +897,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;
 }
@@ -902,7 +907,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.25.1

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

[Martin Blumenstingl]

Hi Amjad,

+Cc Anand who was also investigating the original issue one year ago

On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur
<aouledameur@baylibre.com> wrote:
>
>
it seems that there's an extraneous blank line here

> 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 <aouledameur@baylibre.com>
> Reported-by: Jerome Brunet <jbrunet@baylibre.com>
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

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

[Anand Moon]

Hi Amjad

On Sun, 21 Nov 2021 at 05:21, Martin Blumenstingl
<martin.blumenstingl@googlemail.com> wrote:
>
> Hi Amjad,
>
> +Cc Anand who was also investigating the original issue one year ago
>
Thanks.
> On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur
> <aouledameur@baylibre.com> wrote:
> >
> >
> it seems that there's an extraneous blank line here
>
> > 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 <aouledameur@baylibre.com>
> > Reported-by: Jerome Brunet <jbrunet@baylibre.com>
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>

Changes fix the warning messages on my odroid n2 during suspend / resume.
Please add my
Tested-by: Anand Moon <linux.amoon@gmail.com>

[PATCH v3 3/3] phy: amlogic: meson8b-usb2: fix shared reset control use

[Amjad Ouled-Ameur]

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 <aouledameur@baylibre.com>
Reported-by: Jerome Brunet <jbrunet@baylibre.com>
---
 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 cf10bed40528..a6e74288ca8f 100644
--- a/drivers/phy/amlogic/phy-meson8b-usb2.c
+++ b/drivers/phy/amlogic/phy-meson8b-usb2.c
@@ -154,6 +154,7 @@ 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;
 	}
 
@@ -161,6 +162,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 	if (ret) {
 		dev_err(&phy->dev, "Failed to enable USB DDR clock\n");
 		clk_disable_unprepare(priv->clk_usb_general);
+		reset_control_rearm(priv->reset);
 		return ret;
 	}
 
@@ -199,6 +201,7 @@ static int phy_meson8b_usb2_power_on(struct phy *phy)
 				dev_warn(&phy->dev, "USB ID detect failed!\n");
 				clk_disable_unprepare(priv->clk_usb);
 				clk_disable_unprepare(priv->clk_usb_general);
+				reset_control_rearm(priv->reset);
 				return -EINVAL;
 			}
 		}
@@ -218,6 +221,7 @@ static int phy_meson8b_usb2_power_off(struct phy *phy)
 
 	clk_disable_unprepare(priv->clk_usb);
 	clk_disable_unprepare(priv->clk_usb_general);
+	reset_control_rearm(priv->reset);
 
 	/* power off the PHY by putting it into reset mode */
 	regmap_update_bits(priv->regmap, REG_CTRL, REG_CTRL_POWER_ON_RESET,
-- 
2.25.1

Re: [PATCH v3 3/3] phy: amlogic: meson8b-usb2: fix shared reset control use

[Martin Blumenstingl]

Hi Amjad,

Thanks for working on this!

On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur
<aouledameur@baylibre.com> wrote:
[...]
> +               reset_control_rearm(priv->reset);
Using priv->reset in this driver currently requires an IS_ERR check beforehand.
When I wrote the driver originally I used the following code in
phy_meson8b_usb2_probe:
  priv->reset = ...
  if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
    return PTR_ERR(priv->reset);

That means: priv->reset can (in theory) be an error pointer at runtime.
Since your patch is valid: can you please add another one (before this
one) in the series and change the priv->reset error checking to use
something like:
  if (IS_ERR(priv->reset))
    return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset), "Failed to
get the reset line");

With such a patch you can consider this one as:
Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>


Best regards,
Martin

Re: [PATCH v3 3/3] phy: amlogic: meson8b-usb2: fix shared reset control use

[Amjad Ouled-Ameur]

Hi Martin,

Thank you for reviewing this.

On 21/11/2021 00:57, Martin Blumenstingl wrote:
> Hi Amjad,
>
> Thanks for working on this!
>
> On Fri, Nov 12, 2021 at 5:33 PM Amjad Ouled-Ameur
> <aouledameur@baylibre.com> wrote:
> [...]
>> +               reset_control_rearm(priv->reset);
> Using priv->reset in this driver currently requires an IS_ERR check beforehand.
> When I wrote the driver originally I used the following code in
> phy_meson8b_usb2_probe:
>    priv->reset = ...
>    if (PTR_ERR(priv->reset) == -EPROBE_DEFER)
>      return PTR_ERR(priv->reset);
>
> That means: priv->reset can (in theory) be an error pointer at runtime.
> Since your patch is valid: can you please add another one (before this
> one) in the series and change the priv->reset error checking to use
> something like:
>    if (IS_ERR(priv->reset))
>      return dev_err_probe(&pdev->dev, PTR_ERR(priv->reset), "Failed to
> get the reset line");

No worries, will do.


Regards,

Amjad

> With such a patch you can consider this one as:
> Reviewed-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>
>
> Best regards,
> Martin

Re: [PATCH v3 0/3] usb: meson: fix shared reset control use

[Kevin Hilman]

Amjad Ouled-Ameur <aouledameur@baylibre.com> writes:

> 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").

Tested-by: Kevin Hilman <khilman@baylibre.com>

Tested suspend/resume on Khadas VIM3 and confirmed that the reset
warnings are gone.

Kevin