[PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume

Claudiu posted 4 patches 3 months ago
[PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Claudiu 3 months ago
From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
of the SoC components is turned off, including the USB blocks. On the
resume path, the reset signal must be de-asserted before applying any
settings to the USB registers. To handle this properly, call
reset_control_assert() and reset_control_deassert() during suspend and
resume, respectively.

Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
---
 drivers/usb/host/ehci-platform.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index 57d5a7ddac5f..f61f095cedab 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
 	if (pdata->power_suspend)
 		pdata->power_suspend(pdev);
 
+	ret = reset_control_assert(priv->rsts);
+	if (ret) {
+		if (pdata->power_on)
+			pdata->power_on(pdev);
+
+		ehci_resume(hcd, false);
+
+		if (priv->quirk_poll)
+			quirk_poll_init(priv);
+	}
+
 	return ret;
 }
 
@@ -464,11 +475,18 @@ static int __maybe_unused ehci_platform_resume(struct device *dev)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
 	struct device *companion_dev;
+	int err;
+
+	err = reset_control_deassert(priv->rsts);
+	if (err)
+		return err;
 
 	if (pdata->power_on) {
-		int err = pdata->power_on(pdev);
-		if (err < 0)
+		err = pdata->power_on(pdev);
+		if (err < 0) {
+			reset_control_assert(priv->rsts);
 			return err;
+		}
 	}
 
 	companion_dev = usb_of_get_companion_dev(hcd->self.controller);
-- 
2.43.0
Re: [PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Geert Uytterhoeven 3 months ago
Hi Claudiu,

On Thu, 6 Nov 2025 at 15:36, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>
> The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
> of the SoC components is turned off, including the USB blocks. On the
> resume path, the reset signal must be de-asserted before applying any
> settings to the USB registers. To handle this properly, call
> reset_control_assert() and reset_control_deassert() during suspend and
> resume, respectively.
>
> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>

Thanks for your patch!

> --- a/drivers/usb/host/ehci-platform.c
> +++ b/drivers/usb/host/ehci-platform.c
> @@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
>         if (pdata->power_suspend)
>                 pdata->power_suspend(pdev);
>
> +       ret = reset_control_assert(priv->rsts);
> +       if (ret) {
> +               if (pdata->power_on)
> +                       pdata->power_on(pdev);
> +
> +               ehci_resume(hcd, false);
> +
> +               if (priv->quirk_poll)
> +                       quirk_poll_init(priv);

I have my doubts about the effectiveness of this "reverse error
handling".  If the reset_control_assert() failed, what are the chances
that the device will actually work after trying to bring it up again?

Same comment for next patch.

> +       }
> +
>         return ret;
>  }
>

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Re: [PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Claudiu Beznea 3 months ago
Hi, Geert,

On 11/6/25 16:52, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, 6 Nov 2025 at 15:36, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>
>> The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
>> of the SoC components is turned off, including the USB blocks. On the
>> resume path, the reset signal must be de-asserted before applying any
>> settings to the USB registers. To handle this properly, call
>> reset_control_assert() and reset_control_deassert() during suspend and
>> resume, respectively.
>>
>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/usb/host/ehci-platform.c
>> +++ b/drivers/usb/host/ehci-platform.c
>> @@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
>>         if (pdata->power_suspend)
>>                 pdata->power_suspend(pdev);
>>
>> +       ret = reset_control_assert(priv->rsts);
>> +       if (ret) {
>> +               if (pdata->power_on)
>> +                       pdata->power_on(pdev);
>> +
>> +               ehci_resume(hcd, false);
>> +
>> +               if (priv->quirk_poll)
>> +                       quirk_poll_init(priv);
> 
> I have my doubts about the effectiveness of this "reverse error
> handling".  If the reset_control_assert() failed, what are the chances
> that the device will actually work after trying to bring it up again?
> 
> Same comment for next patch.

I wasn't sure if I should do this revert or not. In my mind, if the reset
assert fails, the reset signal is still de-asserted.

Thank you,
Claudiu

> 
>> +       }
>> +
>>         return ret;
>>  }
>>
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
>
Re: [PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Geert Uytterhoeven 3 months ago
Hi Claudiu,

On Thu, 6 Nov 2025 at 19:56, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 11/6/25 16:52, Geert Uytterhoeven wrote:
> > On Thu, 6 Nov 2025 at 15:36, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>
> >> The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
> >> of the SoC components is turned off, including the USB blocks. On the
> >> resume path, the reset signal must be de-asserted before applying any
> >> settings to the USB registers. To handle this properly, call
> >> reset_control_assert() and reset_control_deassert() during suspend and
> >> resume, respectively.
> >>
> >> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >
> >> --- a/drivers/usb/host/ehci-platform.c
> >> +++ b/drivers/usb/host/ehci-platform.c
> >> @@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
> >>         if (pdata->power_suspend)
> >>                 pdata->power_suspend(pdev);
> >>
> >> +       ret = reset_control_assert(priv->rsts);
> >> +       if (ret) {
> >> +               if (pdata->power_on)
> >> +                       pdata->power_on(pdev);
> >> +
> >> +               ehci_resume(hcd, false);
> >> +
> >> +               if (priv->quirk_poll)
> >> +                       quirk_poll_init(priv);
> >
> > I have my doubts about the effectiveness of this "reverse error
> > handling".  If the reset_control_assert() failed, what are the chances
> > that the device will actually work after trying to bring it up again?
> >
> > Same comment for next patch.
>
> I wasn't sure if I should do this revert or not. In my mind, if the reset
> assert fails, the reset signal is still de-asserted.

Possibly.  Most reset implementations either cannot fail, or can
fail due to a timeout.  What state the device is in in case of the latter is
hard to guess...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Re: [PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Claudiu Beznea 3 months ago
Hi, Geert,

On 11/7/25 10:01, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Thu, 6 Nov 2025 at 19:56, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
>> On 11/6/25 16:52, Geert Uytterhoeven wrote:
>>> On Thu, 6 Nov 2025 at 15:36, Claudiu <claudiu.beznea@tuxon.dev> wrote:
>>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>>
>>>> The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
>>>> of the SoC components is turned off, including the USB blocks. On the
>>>> resume path, the reset signal must be de-asserted before applying any
>>>> settings to the USB registers. To handle this properly, call
>>>> reset_control_assert() and reset_control_deassert() during suspend and
>>>> resume, respectively.
>>>>
>>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
>>>
>>>> --- a/drivers/usb/host/ehci-platform.c
>>>> +++ b/drivers/usb/host/ehci-platform.c
>>>> @@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
>>>>         if (pdata->power_suspend)
>>>>                 pdata->power_suspend(pdev);
>>>>
>>>> +       ret = reset_control_assert(priv->rsts);
>>>> +       if (ret) {
>>>> +               if (pdata->power_on)
>>>> +                       pdata->power_on(pdev);
>>>> +
>>>> +               ehci_resume(hcd, false);
>>>> +
>>>> +               if (priv->quirk_poll)
>>>> +                       quirk_poll_init(priv);
>>>
>>> I have my doubts about the effectiveness of this "reverse error
>>> handling".  If the reset_control_assert() failed, what are the chances
>>> that the device will actually work after trying to bring it up again?
>>>
>>> Same comment for next patch.
>>
>> I wasn't sure if I should do this revert or not. In my mind, if the reset
>> assert fails, the reset signal is still de-asserted.
> 
> Possibly.  Most reset implementations either cannot fail, or can
> fail due to a timeout.  What state the device is in in case of the latter is
> hard to guess...

In theory there are also failures returned by the subsystem code (e.g. if
reset is shared and its reference counts don't have the proper values, if
not shared and ops->assert is missing).

In case of this particular driver and the ochi-platform one, as the resets
request is done with devm_reset_control_array_get_optional_shared() the
priv->resets is an array and the assert/de-assert is done through
reset_control_array_assert()/reset_control_array_deassert() which, in case
of failures, reverts the assert/de-assert operations. It is true that the
effectiveness of the revert operation is unknown and depends on the HW, but
the subsystem ensures it reverts the previous state in case of failure.

For the case resets is not an array, it is true, it depends on the reset
driver implementation and hardware.

Could you please let me know how would you suggest going forward with the
implementation for the patches in this series?

Thank you,
Claudiu
Re: [PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Geert Uytterhoeven 3 months ago
Hi Claudiu,

On Fri, 7 Nov 2025 at 19:42, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> On 11/7/25 10:01, Geert Uytterhoeven wrote:
> > On Thu, 6 Nov 2025 at 19:56, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> >> On 11/6/25 16:52, Geert Uytterhoeven wrote:
> >>> On Thu, 6 Nov 2025 at 15:36, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>>
> >>>> The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
> >>>> of the SoC components is turned off, including the USB blocks. On the
> >>>> resume path, the reset signal must be de-asserted before applying any
> >>>> settings to the USB registers. To handle this properly, call
> >>>> reset_control_assert() and reset_control_deassert() during suspend and
> >>>> resume, respectively.
> >>>>
> >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> >>>
> >>>> --- a/drivers/usb/host/ehci-platform.c
> >>>> +++ b/drivers/usb/host/ehci-platform.c
> >>>> @@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
> >>>>         if (pdata->power_suspend)
> >>>>                 pdata->power_suspend(pdev);
> >>>>
> >>>> +       ret = reset_control_assert(priv->rsts);
> >>>> +       if (ret) {
> >>>> +               if (pdata->power_on)
> >>>> +                       pdata->power_on(pdev);
> >>>> +
> >>>> +               ehci_resume(hcd, false);
> >>>> +
> >>>> +               if (priv->quirk_poll)
> >>>> +                       quirk_poll_init(priv);
> >>>
> >>> I have my doubts about the effectiveness of this "reverse error
> >>> handling".  If the reset_control_assert() failed, what are the chances
> >>> that the device will actually work after trying to bring it up again?
> >>>
> >>> Same comment for next patch.
> >>
> >> I wasn't sure if I should do this revert or not. In my mind, if the reset
> >> assert fails, the reset signal is still de-asserted.
> >
> > Possibly.  Most reset implementations either cannot fail, or can
> > fail due to a timeout.  What state the device is in in case of the latter is
> > hard to guess...
>
> In theory there are also failures returned by the subsystem code (e.g. if
> reset is shared and its reference counts don't have the proper values, if
> not shared and ops->assert is missing).
>
> In case of this particular driver and the ochi-platform one, as the resets
> request is done with devm_reset_control_array_get_optional_shared() the
> priv->resets is an array and the assert/de-assert is done through
> reset_control_array_assert()/reset_control_array_deassert() which, in case
> of failures, reverts the assert/de-assert operations. It is true that the
> effectiveness of the revert operation is unknown and depends on the HW, but
> the subsystem ensures it reverts the previous state in case of failure.
>
> For the case resets is not an array, it is true, it depends on the reset
> driver implementation and hardware.
>
> Could you please let me know how would you suggest going forward with the
> implementation for the patches in this series?

Up to the USB maintainer...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
Re: [PATCH 2/4] usb: host: ehci-platform: Call reset assert/deassert on suspend/resume
Posted by Alan Stern 3 months ago
On Mon, Nov 10, 2025 at 10:29:22AM +0100, Geert Uytterhoeven wrote:
> Hi Claudiu,
> 
> On Fri, 7 Nov 2025 at 19:42, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > On 11/7/25 10:01, Geert Uytterhoeven wrote:
> > > On Thu, 6 Nov 2025 at 19:56, Claudiu Beznea <claudiu.beznea@tuxon.dev> wrote:
> > >> On 11/6/25 16:52, Geert Uytterhoeven wrote:
> > >>> On Thu, 6 Nov 2025 at 15:36, Claudiu <claudiu.beznea@tuxon.dev> wrote:
> > >>>> From: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>>
> > >>>> The Renesas RZ/G3S SoC supports a power-saving mode in which power to most
> > >>>> of the SoC components is turned off, including the USB blocks. On the
> > >>>> resume path, the reset signal must be de-asserted before applying any
> > >>>> settings to the USB registers. To handle this properly, call
> > >>>> reset_control_assert() and reset_control_deassert() during suspend and
> > >>>> resume, respectively.
> > >>>>
> > >>>> Signed-off-by: Claudiu Beznea <claudiu.beznea.uj@bp.renesas.com>
> > >>>
> > >>>> --- a/drivers/usb/host/ehci-platform.c
> > >>>> +++ b/drivers/usb/host/ehci-platform.c
> > >>>> @@ -454,6 +454,17 @@ static int __maybe_unused ehci_platform_suspend(struct device *dev)
> > >>>>         if (pdata->power_suspend)
> > >>>>                 pdata->power_suspend(pdev);
> > >>>>
> > >>>> +       ret = reset_control_assert(priv->rsts);
> > >>>> +       if (ret) {
> > >>>> +               if (pdata->power_on)
> > >>>> +                       pdata->power_on(pdev);
> > >>>> +
> > >>>> +               ehci_resume(hcd, false);
> > >>>> +
> > >>>> +               if (priv->quirk_poll)
> > >>>> +                       quirk_poll_init(priv);
> > >>>
> > >>> I have my doubts about the effectiveness of this "reverse error
> > >>> handling".  If the reset_control_assert() failed, what are the chances
> > >>> that the device will actually work after trying to bring it up again?
> > >>>
> > >>> Same comment for next patch.
> > >>
> > >> I wasn't sure if I should do this revert or not. In my mind, if the reset
> > >> assert fails, the reset signal is still de-asserted.
> > >
> > > Possibly.  Most reset implementations either cannot fail, or can
> > > fail due to a timeout.  What state the device is in in case of the latter is
> > > hard to guess...
> >
> > In theory there are also failures returned by the subsystem code (e.g. if
> > reset is shared and its reference counts don't have the proper values, if
> > not shared and ops->assert is missing).
> >
> > In case of this particular driver and the ochi-platform one, as the resets
> > request is done with devm_reset_control_array_get_optional_shared() the
> > priv->resets is an array and the assert/de-assert is done through
> > reset_control_array_assert()/reset_control_array_deassert() which, in case
> > of failures, reverts the assert/de-assert operations. It is true that the
> > effectiveness of the revert operation is unknown and depends on the HW, but
> > the subsystem ensures it reverts the previous state in case of failure.
> >
> > For the case resets is not an array, it is true, it depends on the reset
> > driver implementation and hardware.
> >
> > Could you please let me know how would you suggest going forward with the
> > implementation for the patches in this series?
> 
> Up to the USB maintainer...

If you don't have any objections, the patches to ehci-platform.c and 
ohci-platform.c are okay with me.

Acked-by: Alan Stern <stern@rowland.harvard.edu>

Alan Stern