| Message ID | 20241210-s2r-cdns-v6-2-28a17f9715a2@bootlin.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix USB suspend on TI J7200 (cdns3-ti, cdns3, xhci) | expand |
On 10/12/2024 19:13, Théo Lebrun wrote: > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect > if an hardware reset occurred. If it did, run the hardware init sequence. > > This callback will be called at system-wide resume. Previously, if a > reset occurred during suspend, we would crash. The wrapper config had > not been written, leading to invalid register accesses inside cdns3. > Did I understand right that the Controller reset can happen only at system suspend and never at runtime suspend? If so do you really need the runtime suspend/resume hooks? you should have different system suspend/resume hooks than runtime suspend/resume hooks and deal with the re-initialization in system resume hook. > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev) > data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > + /* > + * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it > + * detects it as uninitialised. We want to enforce a reset at probe, > + * and so do it manually here. This means the first runtime_resume() > + * will be a no-op. > + */ Separate system sleep hooks will also prevent this kind of behavior. > cdns_ti_reset_and_init_hw(data); > > pm_runtime_enable(dev); > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev) > platform_set_drvdata(pdev, NULL); > } > > +static int cdns_ti_runtime_resume(struct device *dev) > +{ > + const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL; > + struct cdns_ti *data = dev_get_drvdata(dev); > + u32 w1; > + > + w1 = cdns_ti_readl(data, USBSS_W1); > + if ((w1 & mask) != mask) > + cdns_ti_reset_and_init_hw(data); > + > + return 0; > +} > + > +static const struct dev_pm_ops cdns_ti_pm_ops = { > + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) > + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) > +}; > + > static const struct of_device_id cdns_ti_of_match[] = { > { .compatible = "ti,j721e-usb", }, > { .compatible = "ti,am64-usb", }, > @@ -245,6 +269,7 @@ static struct platform_driver cdns_ti_driver = { > .driver = { > .name = "cdns3-ti", > .of_match_table = cdns_ti_of_match, > + .pm = pm_ptr(&cdns_ti_pm_ops), > }, > }; > >
On Thu Dec 12, 2024 at 1:18 PM CET, Roger Quadros wrote: > On 10/12/2024 19:13, Théo Lebrun wrote: > > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect > > if an hardware reset occurred. If it did, run the hardware init sequence. > > > > This callback will be called at system-wide resume. Previously, if a > > reset occurred during suspend, we would crash. The wrapper config had > > not been written, leading to invalid register accesses inside cdns3. > > > > Did I understand right that the Controller reset can happen only at > system suspend and never at runtime suspend? J7200 + upstream kernel => if the power domain is cut off (it is implicitly at runtime PM) then it resets. This is 100% board dependent. We just never let it go into runtime suspend, for the moment. > If so do you really need the runtime suspend/resume hooks? > you should have different system suspend/resume hooks than runtime suspend/resume > hooks and deal with the re-initialization in system resume hook. The patch series works in the current setup with the wrapper that is never shut off. But it would also work if someone decides to use RPM on the wrapper. Overall, the current kernel-wide strategy is to minimise suspend/resume-specific code. Having only the concept of "runtime PM" and triggering that at system-wide suspend/resume is easier to reason about. It unifies concepts and reduces the states a device can be in. We could even imagine a future where ->suspend|resume() callbacks are pm_runtime_force_suspend|resume() by default. That'd be the dream, at least. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 24-12-10 18:13:36, Théo Lebrun wrote: > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect > if an hardware reset occurred. If it did, run the hardware init sequence. > > This callback will be called at system-wide resume. Previously, if a > reset occurred during suspend, we would crash. The wrapper config had > not been written, leading to invalid register accesses inside cdns3. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++ > 1 file changed, 25 insertions(+) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev) > data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > + /* > + * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it > + * detects it as uninitialised. We want to enforce a reset at probe, > + * and so do it manually here. This means the first runtime_resume() > + * will be a no-op. > + */ > cdns_ti_reset_and_init_hw(data); > > pm_runtime_enable(dev); > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev) > platform_set_drvdata(pdev, NULL); > } > > +static int cdns_ti_runtime_resume(struct device *dev) > +{ > + const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL; > + struct cdns_ti *data = dev_get_drvdata(dev); > + u32 w1; > + > + w1 = cdns_ti_readl(data, USBSS_W1); > + if ((w1 & mask) != mask) > + cdns_ti_reset_and_init_hw(data); > + > + return 0; > +} > + > +static const struct dev_pm_ops cdns_ti_pm_ops = { > + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) Why only runtime resume, but without runtime suspend? Peter
On Sat Dec 14, 2024 at 9:49 AM CET, Peter Chen wrote: > On 24-12-10 18:13:36, Théo Lebrun wrote: > > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect > > if an hardware reset occurred. If it did, run the hardware init sequence. > > > > This callback will be called at system-wide resume. Previously, if a > > reset occurred during suspend, we would crash. The wrapper config had > > not been written, leading to invalid register accesses inside cdns3. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++ > > 1 file changed, 25 insertions(+) > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev) > > data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > > data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > > > + /* > > + * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it > > + * detects it as uninitialised. We want to enforce a reset at probe, > > + * and so do it manually here. This means the first runtime_resume() > > + * will be a no-op. > > + */ > > cdns_ti_reset_and_init_hw(data); > > > > pm_runtime_enable(dev); > > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev) > > platform_set_drvdata(pdev, NULL); > > } > > > > +static int cdns_ti_runtime_resume(struct device *dev) > > +{ > > + const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL; > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + u32 w1; > > + > > + w1 = cdns_ti_readl(data, USBSS_W1); > > + if ((w1 & mask) != mask) > > + cdns_ti_reset_and_init_hw(data); > > + > > + return 0; > > +} > > + > > +static const struct dev_pm_ops cdns_ti_pm_ops = { > > + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) > > Why only runtime resume, but without runtime suspend? I don't see any situation where we would want "runtime suspend" be equivalent to "reset the cdns3 wrapper". It implies losing child states, triggering rediscovery of all USB devices at resume. Is that a desired property of runtime suspend on any discoverable bus? Sidenote: also, in our implementation, we do the standard thing of using pm_runtime_force_suspend|resume() as suspend callback implementations. With that, if we did reset the wrapper at suspend, we would: - always have to rediscover USB devices at resume and, - break wakeup sources. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 24-12-16 15:02:43, Théo Lebrun wrote: > On Sat Dec 14, 2024 at 9:49 AM CET, Peter Chen wrote: > > On 24-12-10 18:13:36, Théo Lebrun wrote: > > > At runtime_resume(), read the W1 (Wrapper Register 1) register to detect > > > if an hardware reset occurred. If it did, run the hardware init sequence. > > > > > > This callback will be called at system-wide resume. Previously, if a > > > reset occurred during suspend, we would crash. The wrapper config had > > > not been written, leading to invalid register accesses inside cdns3. > > > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > > --- > > > drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++ > > > 1 file changed, 25 insertions(+) > > > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > > index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644 > > > --- a/drivers/usb/cdns3/cdns3-ti.c > > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > > @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev) > > > data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > > > data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > > > > > + /* > > > + * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it > > > + * detects it as uninitialised. We want to enforce a reset at probe, > > > + * and so do it manually here. This means the first runtime_resume() > > > + * will be a no-op. > > > + */ > > > cdns_ti_reset_and_init_hw(data); > > > > > > pm_runtime_enable(dev); > > > @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev) > > > platform_set_drvdata(pdev, NULL); > > > } > > > > > > +static int cdns_ti_runtime_resume(struct device *dev) > > > +{ > > > + const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL; > > > + struct cdns_ti *data = dev_get_drvdata(dev); > > > + u32 w1; > > > + > > > + w1 = cdns_ti_readl(data, USBSS_W1); > > > + if ((w1 & mask) != mask) > > > + cdns_ti_reset_and_init_hw(data); > > > + > > > + return 0; > > > +} > > > + > > > +static const struct dev_pm_ops cdns_ti_pm_ops = { > > > + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) > > > > Why only runtime resume, but without runtime suspend? > > I don't see any situation where we would want "runtime suspend" be > equivalent to "reset the cdns3 wrapper". It implies losing child > states, triggering rediscovery of all USB devices at resume. Is that a > desired property of runtime suspend on any discoverable bus? Usually, if wrapper needs to put controller to lower power state, it will do some jobs like close clock, etc. And if there is a option for wakeup enable at wrapper, you may also need to set it. Peter
On 13/12/2024 17:28, Théo Lebrun wrote: > On Thu Dec 12, 2024 at 1:18 PM CET, Roger Quadros wrote: >> On 10/12/2024 19:13, Théo Lebrun wrote: >>> At runtime_resume(), read the W1 (Wrapper Register 1) register to detect >>> if an hardware reset occurred. If it did, run the hardware init sequence. >>> >>> This callback will be called at system-wide resume. Previously, if a >>> reset occurred during suspend, we would crash. The wrapper config had >>> not been written, leading to invalid register accesses inside cdns3. >>> >> >> Did I understand right that the Controller reset can happen only at >> system suspend and never at runtime suspend? > > J7200 + upstream kernel => if the power domain is cut off (it is > implicitly at runtime PM) then it resets. This is 100% board dependent. > We just never let it go into runtime suspend, for the moment. > >> If so do you really need the runtime suspend/resume hooks? >> you should have different system suspend/resume hooks than runtime suspend/resume >> hooks and deal with the re-initialization in system resume hook. > > The patch series works in the current setup with the wrapper that is > never shut off. But it would also work if someone decides to use RPM on > the wrapper. But will it work? if we let it runtime suspend and controller looses power, even though we restore the wrapper, who is restoring XHCI controller on runtime resume? Also we would be interested in wakeup events at runtime suspend and by loosing power it doesn't look like it will ever wake up with any USB event. > > Overall, the current kernel-wide strategy is to minimise > suspend/resume-specific code. Having only the concept of "runtime PM" > and triggering that at system-wide suspend/resume is easier to reason > about. It unifies concepts and reduces the states a device can be in. or we just focus on system suspend/resume if the driver can't do meaningful runtime suspend/resume? > > We could even imagine a future where ->suspend|resume() callbacks > are pm_runtime_force_suspend|resume() by default. > That'd be the dream, at least. > > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c index d704eb39820ad08a8774be7f00aa473c3ff267c0..d35be7db7616ef5e5bed7dbd53b78a094809f7cc 100644 --- a/drivers/usb/cdns3/cdns3-ti.c +++ b/drivers/usb/cdns3/cdns3-ti.c @@ -188,6 +188,12 @@ static int cdns_ti_probe(struct platform_device *pdev) data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); + /* + * The call below to pm_runtime_get_sync() MIGHT reset hardware, if it + * detects it as uninitialised. We want to enforce a reset at probe, + * and so do it manually here. This means the first runtime_resume() + * will be a no-op. + */ cdns_ti_reset_and_init_hw(data); pm_runtime_enable(dev); @@ -232,6 +238,24 @@ static void cdns_ti_remove(struct platform_device *pdev) platform_set_drvdata(pdev, NULL); } +static int cdns_ti_runtime_resume(struct device *dev) +{ + const u32 mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL; + struct cdns_ti *data = dev_get_drvdata(dev); + u32 w1; + + w1 = cdns_ti_readl(data, USBSS_W1); + if ((w1 & mask) != mask) + cdns_ti_reset_and_init_hw(data); + + return 0; +} + +static const struct dev_pm_ops cdns_ti_pm_ops = { + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) + SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) +}; + static const struct of_device_id cdns_ti_of_match[] = { { .compatible = "ti,j721e-usb", }, { .compatible = "ti,am64-usb", }, @@ -245,6 +269,7 @@ static struct platform_driver cdns_ti_driver = { .driver = { .name = "cdns3-ti", .of_match_table = cdns_ti_of_match, + .pm = pm_ptr(&cdns_ti_pm_ops), }, };
At runtime_resume(), read the W1 (Wrapper Register 1) register to detect if an hardware reset occurred. If it did, run the hardware init sequence. This callback will be called at system-wide resume. Previously, if a reset occurred during suspend, we would crash. The wrapper config had not been written, leading to invalid register accesses inside cdns3. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/usb/cdns3/cdns3-ti.c | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+)