[PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

[PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

[Enric Balletbo i Serra]

If we power off the SoC logic rail in S3, we can find that the Type-C
PHY can't initialize correctly after system resume. We need to toggle
the USB3-OTG reset before trying to initialize the PHY, or else it
times out.

    phy phy-ff800000.phy.9: phy poweron failed --> -110
    dwc3 fe900000.dwc3: failed to initialize core
    dwc3: probe of fe900000.dwc3 failed with error -110

Note that the RK3399 TRM suggests that we should keep the whole usb3
controller in reset for the duration of the Type-C PHY initialization.
However, it's hard to assert the reset in the current framework of
reset. We're still skeptical about that, and we haven't yet found a
case where this seems to have mattered. This approach is much easier, it
simply holds the USB3-OTG reset while device is supended.

The dwc3 core is going to reinitialize the controller at suspend/resume
anyway (including a "soft reset"), so it should be safe to do this.

Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
---
Dear all,

Now that the usb3-phy otg port support for rk3399 has been merged [1] we
found that suspend/resume is broken. The problem is well known for
ChromeOS kernels, they solved it in a similar way adding a reset pulse on
resume in the specific usb glue layer (dwc3-rockchip). In mainline,
though, we use the dwc3-of-simple glue layer instead of a specific layer
for rockchip. The patch is based on the Brian Norris work but slightly
different, it holds the reset while device is suspended. It was tested
on a Samsung Chromebook Plus with usbc docking station attached by doing
different suspend/resume cycles and checking no usb devices has been
lost.

I am not sure this is the better way to solve this but I did not find
any other way, and, as I am not sure this can be generic, the reset is only
done on rockchip platforms.

Best regards,
 Enric

[1] bfdca1736ea76345071bbc5607d18928e54909ac ('arm64: dts: rockchip: add
    usb3-phy otg-port support for rk3399')

 drivers/usb/dwc3/dwc3-of-simple.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index dbeff5e6ad14..1d1ece99ed94 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -201,9 +201,30 @@ static int dwc3_of_simple_runtime_resume(struct device *dev)
 
 	return 0;
 }
+
+static int dwc3_of_simple_suspend(struct device *dev)
+{
+	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
+
+	if (of_device_is_compatible(dev->of_node, "rockchip,rk3399-dwc3"))
+		reset_control_assert(simple->resets);
+
+	return 0;
+}
+
+static int dwc3_of_simple_resume(struct device *dev)
+{
+	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
+
+	if (of_device_is_compatible(dev->of_node, "rockchip,rk3399-dwc3"))
+		reset_control_deassert(simple->resets);
+
+	return 0;
+}
 #endif
 
 static const struct dev_pm_ops dwc3_of_simple_dev_pm_ops = {
+	SET_SYSTEM_SLEEP_PM_OPS(dwc3_of_simple_suspend, dwc3_of_simple_resume)
 	SET_RUNTIME_PM_OPS(dwc3_of_simple_runtime_suspend,
 			dwc3_of_simple_runtime_resume, NULL)
 };
-- 
2.18.0

Re: [PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

[Ezequiel Garcia]

(Cc devicetree folks)

Hi Enric,

Thanks for the patch.

On Mon, 2018-07-09 at 17:08 +0200, Enric Balletbo i Serra wrote:
> If we power off the SoC logic rail in S3, we can find that the Type-C
> PHY can't initialize correctly after system resume. We need to toggle
> the USB3-OTG reset before trying to initialize the PHY, or else it
> times out.
> 
>     phy phy-ff800000.phy.9: phy poweron failed --> -110
>     dwc3 fe900000.dwc3: failed to initialize core
>     dwc3: probe of fe900000.dwc3 failed with error -110
> 
> Note that the RK3399 TRM suggests that we should keep the whole usb3
> controller in reset for the duration of the Type-C PHY
> initialization.
> However, it's hard to assert the reset in the current framework of
> reset. We're still skeptical about that, and we haven't yet found a
> case where this seems to have mattered. This approach is much easier,
> it
> simply holds the USB3-OTG reset while device is supended.
> 
> The dwc3 core is going to reinitialize the controller at
> suspend/resume
> anyway (including a "soft reset"), so it should be safe to do this.
> 
> Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> ---
> Dear all,
> 
> Now that the usb3-phy otg port support for rk3399 has been merged [1]
> we
> found that suspend/resume is broken. The problem is well known for
> ChromeOS kernels, they solved it in a similar way adding a reset
> pulse on
> resume in the specific usb glue layer (dwc3-rockchip). In mainline,
> though, we use the dwc3-of-simple glue layer instead of a specific
> layer
> for rockchip. The patch is based on the Brian Norris work but
> slightly
> different, it holds the reset while device is suspended. It was
> tested
> on a Samsung Chromebook Plus with usbc docking station attached by
> doing
> different suspend/resume cycles and checking no usb devices has been
> lost.
> 
> I am not sure this is the better way to solve this but I did not find
> any other way, and, as I am not sure this can be generic, the reset
> is only
> done on rockchip platforms.
> 

I don't really understand why there are per-platform hacks in glue
drivers, instead of having per-platform glue drivers, or some other
pluggable hooks.

> Best regards,
>  Enric
> 
> [1] bfdca1736ea76345071bbc5607d18928e54909ac ('arm64: dts: rockchip:
> add
>     usb3-phy otg-port support for rk3399')
> 
>  drivers/usb/dwc3/dwc3-of-simple.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> b/drivers/usb/dwc3/dwc3-of-simple.c
> index dbeff5e6ad14..1d1ece99ed94 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -201,9 +201,30 @@ static int dwc3_of_simple_runtime_resume(struct
> device *dev)
>  
>  	return 0;
>  }
> +
> +static int dwc3_of_simple_suspend(struct device *dev)
> +{
> +	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
> +
> +	if (of_device_is_compatible(dev->of_node, "rockchip,rk3399-
> dwc3"))
> 

Instead of calling of_device_is_compatible in each suspend/resume,
which seems quite expensive, how about having a per-device boolean
'needs_reset' or something like that?

Or maybe just have a boolean devicetree property for this?

Regards,
Eze

Re: [PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

[Ezequiel Garcia]

Sigh, now actually Cc devicetree folks

On Mon, 2018-07-09 at 19:04 -0300, Ezequiel Garcia wrote:
> (Cc devicetree folks)
> 
> Hi Enric,
> 
> Thanks for the patch.
> 
> On Mon, 2018-07-09 at 17:08 +0200, Enric Balletbo i Serra wrote:
> > If we power off the SoC logic rail in S3, we can find that the
> > Type-C
> > PHY can't initialize correctly after system resume. We need to
> > toggle
> > the USB3-OTG reset before trying to initialize the PHY, or else it
> > times out.
> > 
> >     phy phy-ff800000.phy.9: phy poweron failed --> -110
> >     dwc3 fe900000.dwc3: failed to initialize core
> >     dwc3: probe of fe900000.dwc3 failed with error -110
> > 
> > Note that the RK3399 TRM suggests that we should keep the whole
> > usb3
> > controller in reset for the duration of the Type-C PHY
> > initialization.
> > However, it's hard to assert the reset in the current framework of
> > reset. We're still skeptical about that, and we haven't yet found a
> > case where this seems to have mattered. This approach is much
> > easier,
> > it
> > simply holds the USB3-OTG reset while device is supended.
> > 
> > The dwc3 core is going to reinitialize the controller at
> > suspend/resume
> > anyway (including a "soft reset"), so it should be safe to do this.
> > 
> > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
> > >
> > ---
> > Dear all,
> > 
> > Now that the usb3-phy otg port support for rk3399 has been merged
> > [1]
> > we
> > found that suspend/resume is broken. The problem is well known for
> > ChromeOS kernels, they solved it in a similar way adding a reset
> > pulse on
> > resume in the specific usb glue layer (dwc3-rockchip). In mainline,
> > though, we use the dwc3-of-simple glue layer instead of a specific
> > layer
> > for rockchip. The patch is based on the Brian Norris work but
> > slightly
> > different, it holds the reset while device is suspended. It was
> > tested
> > on a Samsung Chromebook Plus with usbc docking station attached by
> > doing
> > different suspend/resume cycles and checking no usb devices has
> > been
> > lost.
> > 
> > I am not sure this is the better way to solve this but I did not
> > find
> > any other way, and, as I am not sure this can be generic, the reset
> > is only
> > done on rockchip platforms.
> > 
> 
> I don't really understand why there are per-platform hacks in glue
> drivers, instead of having per-platform glue drivers, or some other
> pluggable hooks.
> 
> > Best regards,
> >  Enric
> > 
> > [1] bfdca1736ea76345071bbc5607d18928e54909ac ('arm64: dts:
> > rockchip:
> > add
> >     usb3-phy otg-port support for rk3399')
> > 
> >  drivers/usb/dwc3/dwc3-of-simple.c | 21 +++++++++++++++++++++
> >  1 file changed, 21 insertions(+)
> > 
> > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> > b/drivers/usb/dwc3/dwc3-of-simple.c
> > index dbeff5e6ad14..1d1ece99ed94 100644
> > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > @@ -201,9 +201,30 @@ static int
> > dwc3_of_simple_runtime_resume(struct
> > device *dev)
> >  
> >  	return 0;
> >  }
> > +
> > +static int dwc3_of_simple_suspend(struct device *dev)
> > +{
> > +	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
> > +
> > +	if (of_device_is_compatible(dev->of_node,
> > "rockchip,rk3399-
> > dwc3"))
> > 
> 
> Instead of calling of_device_is_compatible in each suspend/resume,
> which seems quite expensive, how about having a per-device boolean
> 'needs_reset' or something like that?
> 
> Or maybe just have a boolean devicetree property for this?
> 
> Regards,
> Eze
> 

Re: [PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

[Enric Balletbo Serra]

Hi,
Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dt., 10
de jul. 2018 a les 0:10:
>
> Sigh, now actually Cc devicetree folks
>
> On Mon, 2018-07-09 at 19:04 -0300, Ezequiel Garcia wrote:
> > (Cc devicetree folks)
> >
> > Hi Enric,
> >
> > Thanks for the patch.
> >
> > On Mon, 2018-07-09 at 17:08 +0200, Enric Balletbo i Serra wrote:
> > > If we power off the SoC logic rail in S3, we can find that the
> > > Type-C
> > > PHY can't initialize correctly after system resume. We need to
> > > toggle
> > > the USB3-OTG reset before trying to initialize the PHY, or else it
> > > times out.
> > >
> > >     phy phy-ff800000.phy.9: phy poweron failed --> -110
> > >     dwc3 fe900000.dwc3: failed to initialize core
> > >     dwc3: probe of fe900000.dwc3 failed with error -110
> > >
> > > Note that the RK3399 TRM suggests that we should keep the whole
> > > usb3
> > > controller in reset for the duration of the Type-C PHY
> > > initialization.
> > > However, it's hard to assert the reset in the current framework of
> > > reset. We're still skeptical about that, and we haven't yet found a
> > > case where this seems to have mattered. This approach is much
> > > easier,
> > > it
> > > simply holds the USB3-OTG reset while device is supended.
> > >
> > > The dwc3 core is going to reinitialize the controller at
> > > suspend/resume
> > > anyway (including a "soft reset"), so it should be safe to do this.
> > >
> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
> > > >
> > > ---
> > > Dear all,
> > >
> > > Now that the usb3-phy otg port support for rk3399 has been merged
> > > [1]
> > > we
> > > found that suspend/resume is broken. The problem is well known for
> > > ChromeOS kernels, they solved it in a similar way adding a reset
> > > pulse on
> > > resume in the specific usb glue layer (dwc3-rockchip). In mainline,
> > > though, we use the dwc3-of-simple glue layer instead of a specific
> > > layer
> > > for rockchip. The patch is based on the Brian Norris work but
> > > slightly
> > > different, it holds the reset while device is suspended. It was
> > > tested
> > > on a Samsung Chromebook Plus with usbc docking station attached by
> > > doing
> > > different suspend/resume cycles and checking no usb devices has
> > > been
> > > lost.
> > >
> > > I am not sure this is the better way to solve this but I did not
> > > find
> > > any other way, and, as I am not sure this can be generic, the reset
> > > is only
> > > done on rockchip platforms.
> > >
> >
> > I don't really understand why there are per-platform hacks in glue
> > drivers, instead of having per-platform glue drivers, or some other
> > pluggable hooks.
> >
> > > Best regards,
> > >  Enric
> > >
> > > [1] bfdca1736ea76345071bbc5607d18928e54909ac ('arm64: dts:
> > > rockchip:
> > > add
> > >     usb3-phy otg-port support for rk3399')
> > >
> > >  drivers/usb/dwc3/dwc3-of-simple.c | 21 +++++++++++++++++++++
> > >  1 file changed, 21 insertions(+)
> > >
> > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
> > > b/drivers/usb/dwc3/dwc3-of-simple.c
> > > index dbeff5e6ad14..1d1ece99ed94 100644
> > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
> > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> > > @@ -201,9 +201,30 @@ static int
> > > dwc3_of_simple_runtime_resume(struct
> > > device *dev)
> > >
> > >     return 0;
> > >  }
> > > +
> > > +static int dwc3_of_simple_suspend(struct device *dev)
> > > +{
> > > +   struct dwc3_of_simple *simple = dev_get_drvdata(dev);
> > > +
> > > +   if (of_device_is_compatible(dev->of_node,
> > > "rockchip,rk3399-
> > > dwc3"))
> > >
> >
> > Instead of calling of_device_is_compatible in each suspend/resume,
> > which seems quite expensive, how about having a per-device boolean
> > 'needs_reset' or something like that?
> >
Yep, not sure how much, but probably use a boolean will be faster. I
am also wondering if we can remove the of_device_is_compatible call
and do the reset on all platforms. This will need lots of tests on
different platforms, of course. i'd like to hear maintainers feedback
here.

> > Or maybe just have a boolean devicetree property for this?

I don't think moving the "hack" to the devicetree will be acceptable
for the DT maintainers, this is not really a hardware property.

Thanks,
  Enric.

> >
> > Regards,
> > Eze
> >

Re: [PATCH] usb: dwc3: of-simple: reset host controller at suspend/resume

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 4698 bytes --]


Hi,

Enric Balletbo Serra <eballetbo@gmail.com> writes:
> Hi,
> Missatge de Ezequiel Garcia <ezequiel@collabora.com> del dia dt., 10
> de jul. 2018 a les 0:10:
>>
>> Sigh, now actually Cc devicetree folks
>>
>> On Mon, 2018-07-09 at 19:04 -0300, Ezequiel Garcia wrote:
>> > (Cc devicetree folks)
>> >
>> > Hi Enric,
>> >
>> > Thanks for the patch.
>> >
>> > On Mon, 2018-07-09 at 17:08 +0200, Enric Balletbo i Serra wrote:
>> > > If we power off the SoC logic rail in S3, we can find that the
>> > > Type-C
>> > > PHY can't initialize correctly after system resume. We need to
>> > > toggle
>> > > the USB3-OTG reset before trying to initialize the PHY, or else it
>> > > times out.
>> > >
>> > >     phy phy-ff800000.phy.9: phy poweron failed --> -110
>> > >     dwc3 fe900000.dwc3: failed to initialize core
>> > >     dwc3: probe of fe900000.dwc3 failed with error -110
>> > >
>> > > Note that the RK3399 TRM suggests that we should keep the whole
>> > > usb3
>> > > controller in reset for the duration of the Type-C PHY
>> > > initialization.
>> > > However, it's hard to assert the reset in the current framework of
>> > > reset. We're still skeptical about that, and we haven't yet found a
>> > > case where this seems to have mattered. This approach is much
>> > > easier,
>> > > it
>> > > simply holds the USB3-OTG reset while device is supended.
>> > >
>> > > The dwc3 core is going to reinitialize the controller at
>> > > suspend/resume
>> > > anyway (including a "soft reset"), so it should be safe to do this.
>> > >
>> > > Signed-off-by: Enric Balletbo i Serra <enric.balletbo@collabora.com
>> > > >
>> > > ---
>> > > Dear all,
>> > >
>> > > Now that the usb3-phy otg port support for rk3399 has been merged
>> > > [1]
>> > > we
>> > > found that suspend/resume is broken. The problem is well known for
>> > > ChromeOS kernels, they solved it in a similar way adding a reset
>> > > pulse on
>> > > resume in the specific usb glue layer (dwc3-rockchip). In mainline,
>> > > though, we use the dwc3-of-simple glue layer instead of a specific
>> > > layer
>> > > for rockchip. The patch is based on the Brian Norris work but
>> > > slightly
>> > > different, it holds the reset while device is suspended. It was
>> > > tested
>> > > on a Samsung Chromebook Plus with usbc docking station attached by
>> > > doing
>> > > different suspend/resume cycles and checking no usb devices has
>> > > been
>> > > lost.
>> > >
>> > > I am not sure this is the better way to solve this but I did not
>> > > find
>> > > any other way, and, as I am not sure this can be generic, the reset
>> > > is only
>> > > done on rockchip platforms.
>> > >
>> >
>> > I don't really understand why there are per-platform hacks in glue
>> > drivers, instead of having per-platform glue drivers, or some other
>> > pluggable hooks.
>> >
>> > > Best regards,
>> > >  Enric
>> > >
>> > > [1] bfdca1736ea76345071bbc5607d18928e54909ac ('arm64: dts:
>> > > rockchip:
>> > > add
>> > >     usb3-phy otg-port support for rk3399')
>> > >
>> > >  drivers/usb/dwc3/dwc3-of-simple.c | 21 +++++++++++++++++++++
>> > >  1 file changed, 21 insertions(+)
>> > >
>> > > diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
>> > > b/drivers/usb/dwc3/dwc3-of-simple.c
>> > > index dbeff5e6ad14..1d1ece99ed94 100644
>> > > --- a/drivers/usb/dwc3/dwc3-of-simple.c
>> > > +++ b/drivers/usb/dwc3/dwc3-of-simple.c
>> > > @@ -201,9 +201,30 @@ static int
>> > > dwc3_of_simple_runtime_resume(struct
>> > > device *dev)
>> > >
>> > >     return 0;
>> > >  }
>> > > +
>> > > +static int dwc3_of_simple_suspend(struct device *dev)
>> > > +{
>> > > +   struct dwc3_of_simple *simple = dev_get_drvdata(dev);
>> > > +
>> > > +   if (of_device_is_compatible(dev->of_node,
>> > > "rockchip,rk3399-
>> > > dwc3"))
>> > >
>> >
>> > Instead of calling of_device_is_compatible in each suspend/resume,
>> > which seems quite expensive, how about having a per-device boolean
>> > 'needs_reset' or something like that?
>> >
> Yep, not sure how much, but probably use a boolean will be faster. I
> am also wondering if we can remove the of_device_is_compatible call
> and do the reset on all platforms. This will need lots of tests on
> different platforms, of course. i'd like to hear maintainers feedback
> here.

ideally, the reset would be unconditional, but in practice, probably
it's handled differently by different implementations.

I'd be fine with a single of_device_is_compatible() call during probe to
set a driver flag. Another possibility would be to use the data field of
of_device_id to pass that flag.

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]