| Message ID | 20231120-j7200-usb-suspend-v2-0-038c7e4a3df4@bootlin.com |
|---|---|
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset-on-resume | expand |
On 20/11/2023 19:06, Théo Lebrun wrote: > Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will > in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME. > > Avoid this warning on resume: > > [ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit > > When used, remote wakeup is not expected to work. > > Only focus J7200 as other SoC are untested. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++-- > 1 file changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index 84f93c2fcd5c..7d56a1acbc54 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -16,6 +16,7 @@ > #include <linux/of_platform.h> > #include <linux/pm_runtime.h> > #include <linux/property.h> > +#include "core.h" > > /* USB Wrapper register offsets */ > #define USBSS_PID 0x0 > @@ -128,6 +129,7 @@ static int cdns_ti_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > struct device_node *node = pdev->dev.of_node; > + const struct of_dev_auxdata *auxdata; > struct cdns_ti *data; > unsigned long rate; > int error, i; > @@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev) > > cdns_ti_init_hw(data); > > - error = of_platform_populate(node, NULL, NULL, dev); > + auxdata = of_device_get_match_data(dev); > + error = of_platform_populate(node, NULL, auxdata, dev); > if (error) { > dev_err(dev, "failed to create children: %d\n", error); > return error; > @@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = { > > #endif /* CONFIG_PM */ > > +static struct cdns3_platform_data cdns_ti_j7200_pdata = { > + .quirks = CDNS3_RESET_ON_RESUME, > +}; We will need to introduce a new data structure "struct cdns_ti_platform_data" and add platform specific details like "reset_on_resume" to it. This is to address what Krzysztof pointed to in patch 4. > + > +static const struct of_dev_auxdata cdns_ti_j7200_auxdata[] = { > + { > + .compatible = "cdns,usb3", > + .platform_data = &cdns_ti_j7200_pdata, > + }, > + {}, > +}; > + > static const struct of_device_id cdns_ti_of_match[] = { > - { .compatible = "ti,j7200-usb", }, > + { .compatible = "ti,j7200-usb", .data = cdns_ti_j7200_auxdata, }, Here we should pass "struct cdns_ti_platform_data" > { .compatible = "ti,j721e-usb", }, > { .compatible = "ti,am64-usb", }, > {}, >
Hello, On Tue Nov 21, 2023 at 5:53 PM CET, Roger Quadros wrote: > On 20/11/2023 19:06, Théo Lebrun wrote: > > Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will > > in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME. > > > > Avoid this warning on resume: > > > > [ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit > > > > When used, remote wakeup is not expected to work. > > > > Only focus J7200 as other SoC are untested. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++-- > > 1 file changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index 84f93c2fcd5c..7d56a1acbc54 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > @@ -16,6 +16,7 @@ > > #include <linux/of_platform.h> > > #include <linux/pm_runtime.h> > > #include <linux/property.h> > > +#include "core.h" > > > > /* USB Wrapper register offsets */ > > #define USBSS_PID 0x0 > > @@ -128,6 +129,7 @@ static int cdns_ti_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct device_node *node = pdev->dev.of_node; > > + const struct of_dev_auxdata *auxdata; > > struct cdns_ti *data; > > unsigned long rate; > > int error, i; > > @@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev) > > > > cdns_ti_init_hw(data); > > > > - error = of_platform_populate(node, NULL, NULL, dev); > > + auxdata = of_device_get_match_data(dev); > > + error = of_platform_populate(node, NULL, auxdata, dev); > > if (error) { > > dev_err(dev, "failed to create children: %d\n", error); > > return error; > > @@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = { > > > > #endif /* CONFIG_PM */ > > > > +static struct cdns3_platform_data cdns_ti_j7200_pdata = { > > + .quirks = CDNS3_RESET_ON_RESUME, > > +}; > > We will need to introduce a new data structure "struct cdns_ti_platform_data" > and add platform specific details like "reset_on_resume" to it. > This is to address what Krzysztof pointed to in patch 4. Yes I've got it locally following Krzysztof's review. Below my signature is a sneak peak as I'll wait a bit more before a V3. First we implement resume behavior in the wrapper driver using match data then we add auxdata passed to the subdevices. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------
Théo Lebrun <theo.lebrun@bootlin.com> writes: > The driver does not use RPM. It enables it & gets a reference at probe. > It then undoes that on probe error or at remove. ...which is a fairly standard thing to do for a rudimentary runtime PM support on platforms that use power domains. This will likely (almost surely) break other platforms. Without a runtime PM get call, the power domain that this device is in could be powered off without this driver ever knowing about it, causing a crash as soon as the driver is used after the domain is turned off. Kevin
Théo Lebrun <theo.lebrun@bootlin.com> writes: > Hi, > > Suspend on the TI J7200 platform is broken currently. There are two > components that need to be patched so that they assume reset on > resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the > controller. > > About (1): the TI wrapper only did its hardware configuration at probe > time. Update so that it is done at resume on J7200 SoC. > > About (2): signal from cdns3-ti to cdns3 host to xHCI that the > controller resets on resume. This way we avoid the following warning: > > xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit > > Strictly speaking (2) is not required to have working suspend on J7200; > its only goal is to silence this warning. > > Those patches have been tested on the TI J7200 EVM GP. No need to > mention that other patches are required for S2R to work, but those will > be sent later down the road. Those USB patches are rather standalone. > > Thanks, > Théo > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > Changes in v2: > - Remove runtime PM from cdns3-ti; it brings nothing. That means our > cdns3-ti suspend/resume patch is simpler; there is no need to handle > runtime PM at suspend/resume. Sorry I sent comments on v2 before I noticed v3 was out, but this is not a good idea IMO. I sent more detailed comment on that specific patch. Kevin
Hi, Suspend on the TI J7200 platform is broken currently. There are two components that need to be patched so that they assume reset on resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the controller. About (1): the TI wrapper only did its hardware configuration at probe time. Update so that it is done at resume on J7200 SoC. About (2): signal from cdns3-ti to cdns3 host to xHCI that the controller resets on resume. This way we avoid the following warning: xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit Strictly speaking (2) is not required to have working suspend on J7200; its only goal is to silence this warning. Those patches have been tested on the TI J7200 EVM GP. No need to mention that other patches are required for S2R to work, but those will be sent later down the road. Those USB patches are rather standalone. Thanks, Théo -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v2: - Remove runtime PM from cdns3-ti; it brings nothing. That means our cdns3-ti suspend/resume patch is simpler; there is no need to handle runtime PM at suspend/resume. - Do not add cdns3 host role suspend/resume callbacks; they are not needed as core detects reset on resume & calls cdns_drd_host_on when needed. - cdns3-ti: Move usb2_refclk_rate_code assignment closer to the value computation. - cdns3/host.c: do not pass XHCI_SUSPEND_RESUME_CLKS quirk to xHCI; it is unneeded on our platform. - Link to v1: https://lore.kernel.org/r/20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com --- Théo Lebrun (7): dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible usb: cdns3-ti: remove runtime PM usb: cdns3-ti: move reg writes from probe into an init_hw helper usb: cdns3-ti: add suspend/resume procedures for J7200 usb: cdns3: add quirk to platform data for reset-on-resume usb: cdns3-ti: signal reset-on-resume to xHCI for J7200 platform arm64: dts: ti: k3-j7200: use J7200-specific USB compatible .../devicetree/bindings/usb/ti,j721e-usb.yaml | 4 + arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +- drivers/usb/cdns3/cdns3-ti.c | 134 +++++++++++++-------- drivers/usb/cdns3/core.h | 1 + drivers/usb/cdns3/host.c | 3 + 5 files changed, 90 insertions(+), 54 deletions(-) --- base-commit: 1d42d5c8f1ca11106579dcaadef4161fee03419e change-id: 20231113-j7200-usb-suspend-2a47f2281e04 Best regards,