| Message ID | 20180709150844.32505-1-enric.balletbo@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
(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 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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) };
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(+)