| Message ID | 20240307-j7200-usb-suspend-v4-4-5ec7615431f3@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset-on-resume | expand |
On 07/03/2024 11:55, Théo Lebrun wrote: > Add match data support, with one boolean to indicate whether the > hardware resets after a system-wide suspend. If hardware resets, we > force execute ->runtime_resume() at system-wide resume to run the > hardware init sequence. > > No compatible exploits this functionality, just yet. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index 4c8a557e6a6f..f76327566798 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -57,9 +57,14 @@ struct cdns_ti { > unsigned vbus_divider:1; > struct clk *usb2_refclk; > struct clk *lpm_clk; > + const struct cdns_ti_match_data *match_data; > int usb2_refclk_rate_code; > }; > > +struct cdns_ti_match_data { > + bool reset_on_resume; > +}; > + > static const int cdns_ti_rate_table[] = { /* in KHZ */ > 9600, > 10000, > @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, data); > > data->dev = dev; > + data->match_data = device_get_match_data(dev); > > data->usbss = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(data->usbss)) { > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > return 0; > } > > +static int cdns_ti_suspend(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_suspend(dev); > + else > + return 0; > +} > + > +static int cdns_ti_resume(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + Instead of this just do Reset and re-init here on affected platforms. This is after you remove reset/re-init code from .runtime_resume(). > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_resume(dev); > + else > + 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(cdns_ti_suspend, cdns_ti_resume) > }; > > static const struct of_device_id cdns_ti_of_match[] = { >
Théo Lebrun <theo.lebrun@bootlin.com> writes: > Add match data support, with one boolean to indicate whether the > hardware resets after a system-wide suspend. If hardware resets, we > force execute ->runtime_resume() at system-wide resume to run the > hardware init sequence. Is "whether the hardware resets after a system-wide suspend" really a function of the IP itself, or rather whether the IP is in a power domain that might power down? > No compatible exploits this functionality, just yet. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > 1 file changed, 27 insertions(+) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index 4c8a557e6a6f..f76327566798 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -57,9 +57,14 @@ struct cdns_ti { > unsigned vbus_divider:1; > struct clk *usb2_refclk; > struct clk *lpm_clk; > + const struct cdns_ti_match_data *match_data; > int usb2_refclk_rate_code; > }; > > +struct cdns_ti_match_data { > + bool reset_on_resume; > +}; > + > static const int cdns_ti_rate_table[] = { /* in KHZ */ > 9600, > 10000, > @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev) > platform_set_drvdata(pdev, data); > > data->dev = dev; > + data->match_data = device_get_match_data(dev); > > data->usbss = devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(data->usbss)) { > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > return 0; > } > > +static int cdns_ti_suspend(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_suspend(dev); > + else > + return 0; > +} > + > +static int cdns_ti_resume(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (data->match_data && data->match_data->reset_on_resume) > + return pm_runtime_force_resume(dev); > + else > + return 0; > +} Conditionally forcing runtime suspend/resume based on a property of the IP doesn't feel right to me. IMO, the device should always runtime suspend/resume, and in the runtime PM hooks is where the conditional logic should be. And speaking of the conditional logic... let's go back to whether "resets_on_resume" is a property of the IP or the enclosing power domain. Instead of having an IP-specific flag, another way of approaching this when ->runtime_resume() is called every time is simply for that hook to check if a reset has happend. Sometimes you can tell this simply by reading a register that has been previously programmed by the driver but has a known reset. Simply check that regisister and you can tell whether context has been lost. Doing it this way makes the driver "smart" and then you don't have to rely on bool flag based on the IP and dependent on the DT compatible. Kevin
Hello, On Fri Mar 8, 2024 at 10:58 PM CET, Kevin Hilman wrote: > Théo Lebrun <theo.lebrun@bootlin.com> writes: > > Add match data support, with one boolean to indicate whether the > > hardware resets after a system-wide suspend. If hardware resets, we > > force execute ->runtime_resume() at system-wide resume to run the > > hardware init sequence. > > Is "whether the hardware resets after a system-wide suspend" really a > function of the IP itself, or rather whether the IP is in a power domain > that might power down? Is a compatible defining (1) the IP block involved or (2) the IP block involved and its integration? This is a rethorical question, I've asked it internally at Bootlin and we had some interesting discussions. :-) Whether compatible or some other mechanism indicate expected suspend behavior, we agreed that suspend types where not modeled properly by the kernel currently. > > No compatible exploits this functionality, just yet. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ > > 1 file changed, 27 insertions(+) > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index 4c8a557e6a6f..f76327566798 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > @@ -57,9 +57,14 @@ struct cdns_ti { > > unsigned vbus_divider:1; > > struct clk *usb2_refclk; > > struct clk *lpm_clk; > > + const struct cdns_ti_match_data *match_data; > > int usb2_refclk_rate_code; > > }; > > > > +struct cdns_ti_match_data { > > + bool reset_on_resume; > > +}; > > + > > static const int cdns_ti_rate_table[] = { /* in KHZ */ > > 9600, > > 10000, > > @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev) > > platform_set_drvdata(pdev, data); > > > > data->dev = dev; > > + data->match_data = device_get_match_data(dev); > > > > data->usbss = devm_platform_ioremap_resource(pdev, 0); > > if (IS_ERR(data->usbss)) { > > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) > > return 0; > > } > > > > +static int cdns_ti_suspend(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + > > + if (data->match_data && data->match_data->reset_on_resume) > > + return pm_runtime_force_suspend(dev); > > + else > > + return 0; > > +} > > + > > +static int cdns_ti_resume(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + > > + if (data->match_data && data->match_data->reset_on_resume) > > + return pm_runtime_force_resume(dev); > > + else > > + return 0; > > +} > > Conditionally forcing runtime suspend/resume based on a property of the > IP doesn't feel right to me. > > IMO, the device should always runtime suspend/resume, and in the > runtime PM hooks is where the conditional logic should be. > > And speaking of the conditional logic... let's go back to whether > "resets_on_resume" is a property of the IP or the enclosing power > domain. > > Instead of having an IP-specific flag, another way of approaching this > when ->runtime_resume() is called every time is simply for that hook to > check if a reset has happend. Sometimes you can tell this simply by > reading a register that has been previously programmed by the driver but > has a known reset. Simply check that regisister and you can tell > whether context has been lost. > > Doing it this way makes the driver "smart" and then you don't have to > rely on bool flag based on the IP and dependent on the DT compatible. I agree! I never digged into this for a reason: the HXCI subsystem takes a quirk flag that tells it whether it resets on resume (XHCI_RESET_ON_RESUME). My plans weren't grandiose enough to think about touching this aspect. That means we need to know this bool value at probe. About a smart ->runtime_resume() implementation: it is doable. I've experimented with that following your message. The condition is rather simple, looking like: static int cdns_ti_runtime_resume(struct device *dev) { struct cdns_ti *data = dev_get_drvdata(dev); u32 w1, mask; w1 = cdns_ti_readl(data, USBSS_W1); mask = USBSS_W1_PWRUP_RST | USBSS_W1_MODESTRAP_SEL; if ((w1 & mask) != mask) cdns_ti_reset_and_init_hw(dev, data); return 0; } Tested on J7200-EVM, works as expected. Both bits reset to zero. First is software reset; second is described as "this bit has to be always set to 1". cdns_ti_reset_and_init_hw() would also be call at probe before enabling runtime PM to ensure we always reset the IP at probe. Then ->runtime_resume() would get called once during probe without any impact as the conditional would return false. It would trigger at resume. System-wide suspend/resume hooks would become: SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume) Do you have any ideas related to the handling of XHCI_RESET_ON_RESUME? See xhci_resume(). Some notes: - XHCI is capable of detect what it calls reinit (reset after resume). The flags only shortcut the restore attempt (which is useless when the controller reset) and disables warning because of reinit. Logs contain this when we do not pass the flag: [ 18.518138] xhci-hcd xhci-hcd.11.auto: xHC error in resume, USBSTS 0x401, Reinit [ 18.525522] usb usb1: root hub lost power or was reset [ 18.530647] usb usb2: root hub lost power or was reset - Ways forward I can imagine: - A quick and dirty solution would be to grab a reference to the xhci_hcd struct pointer from TI wrapper and update quirks on the go. - Another one would be to keep as-is and let xhci-hcd warn on each resume. Mentioning this for exhaustiveness. - Introduce a new mechanism to communicate through the stack: from the TI wrapper, to cdns3 core, to the xhci-hcd device. Or use an existing mechanism if one is present. - Remove the assumption xHCI does that a suspend is broken if the controller was reset during it. That doesn't sound right to me, can you confirm? 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 4c8a557e6a6f..f76327566798 100644 --- a/drivers/usb/cdns3/cdns3-ti.c +++ b/drivers/usb/cdns3/cdns3-ti.c @@ -57,9 +57,14 @@ struct cdns_ti { unsigned vbus_divider:1; struct clk *usb2_refclk; struct clk *lpm_clk; + const struct cdns_ti_match_data *match_data; int usb2_refclk_rate_code; }; +struct cdns_ti_match_data { + bool reset_on_resume; +}; + static const int cdns_ti_rate_table[] = { /* in KHZ */ 9600, 10000, @@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev) platform_set_drvdata(pdev, data); data->dev = dev; + data->match_data = device_get_match_data(dev); data->usbss = devm_platform_ioremap_resource(pdev, 0); if (IS_ERR(data->usbss)) { @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev) return 0; } +static int cdns_ti_suspend(struct device *dev) +{ + struct cdns_ti *data = dev_get_drvdata(dev); + + if (data->match_data && data->match_data->reset_on_resume) + return pm_runtime_force_suspend(dev); + else + return 0; +} + +static int cdns_ti_resume(struct device *dev) +{ + struct cdns_ti *data = dev_get_drvdata(dev); + + if (data->match_data && data->match_data->reset_on_resume) + return pm_runtime_force_resume(dev); + else + 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(cdns_ti_suspend, cdns_ti_resume) }; static const struct of_device_id cdns_ti_of_match[] = {
Add match data support, with one boolean to indicate whether the hardware resets after a system-wide suspend. If hardware resets, we force execute ->runtime_resume() at system-wide resume to run the hardware init sequence. No compatible exploits this functionality, just yet. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+)