| Message ID | 20231113-j7200-usb-suspend-v1-0-ad1ee714835c@bootlin.com |
|---|---|
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset on resume | expand |
Hello Théo, > On our platform, suspend-to-idle or suspend-to-RAM turn the controller > off thanks to a power-domain. This compatible triggers reset on resume > behavior to reconfigure the hardware. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > index 709081cd1e7f..581905d9199e 100644 > --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > @@ -788,7 +788,7 @@ pcie1_ep: pcie-ep@2910000 { > }; > > usbss0: cdns-usb@4104000 { > - compatible = "ti,j721e-usb"; > + compatible = "ti,j7200-usb"; What about keeping the old compatible as fallback in the unlikley case we have a new dtb with an old kernel ? Gregory > reg = <0x00 0x4104000 0x00 0x100>; > dma-coherent; > power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>; > > -- > 2.41.0 > >
Hello, On Mon Nov 13, 2023 at 4:39 PM CET, Gregory CLEMENT wrote: > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index c331bcd2faeb..50b38c4b9c87 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > @@ -197,6 +197,50 @@ static int cdns_ti_probe(struct platform_device *pdev) > > return error; > > } > > > > +#ifdef CONFIG_PM > > + > > +static int cdns_ti_suspend(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + > > + if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb")) > > + return 0; > > Just a small remark: > > What about adding a boolean in the cdns_ti struct for taking care of > it ? Then you will go through the device tree only during probe. Moreover > if this behaviour is needed for more compatible we can just add them in > the probe too. Will do. The hardest part will be to pick a good name. > Besides this you still can add my > > Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com> Thanks for the review. -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Tue Nov 14, 2023 at 11:01 AM CET, Gregory CLEMENT wrote: > > --- > > diff --git a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > > index 709081cd1e7f..581905d9199e 100644 > > --- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > > +++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi > > @@ -788,7 +788,7 @@ pcie1_ep: pcie-ep@2910000 { > > }; > > > > usbss0: cdns-usb@4104000 { > > - compatible = "ti,j721e-usb"; > > + compatible = "ti,j7200-usb"; > > What about keeping the old compatible as fallback in the unlikley case > we have a new dtb with an old kernel ? I see the usecase; that will be done in V2. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 13/11/2023 16:26, Théo Lebrun wrote: > Hardware initialisation is only done at probe. The J7200 USB controller > is reset at resume because of power-domains toggling off & on. We > therefore (1) toggle PM runtime at suspend/resume & (2) reconfigure the > hardware at resume. at probe we are doing a pm_runtime_get() and never doing a put thus preventing any runtime PM. > > Reuse the newly extracted cdns_ti_init_hw() function that contains the > register write sequence. > > We guard this behavior based on compatible to avoid modifying the > current behavior on other platforms. If the controller does not reset > we do not want to touch PM runtime & do not want to redo reg writes. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 48 +++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 47 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index c331bcd2faeb..50b38c4b9c87 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -197,6 +197,50 @@ static int cdns_ti_probe(struct platform_device *pdev) > return error; > } > > +#ifdef CONFIG_PM > + > +static int cdns_ti_suspend(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + > + if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb")) > + return 0; > + > + pm_runtime_put_sync(data->dev); > + > + return 0; You might want to check suspend/resume ops in cdns3-plat and do something similar here. > +} > + > +static int cdns_ti_resume(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + int ret; > + > + if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb")) > + return 0; > + > + ret = pm_runtime_get_sync(dev); > + if (ret < 0) { > + dev_err(dev, "pm_runtime_get_sync failed: %d\n", ret); > + goto err; > + } > + > + cdns_ti_init_hw(data); > + > + return 0; > + > +err: > + pm_runtime_put_sync(data->dev); > + pm_runtime_disable(data->dev); > + return ret; > +} > + > +static const struct dev_pm_ops cdns_ti_pm_ops = { > + SET_SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume) > +}; > + > +#endif /* CONFIG_PM */ > + > static int cdns_ti_remove_core(struct device *dev, void *c) > { > struct platform_device *pdev = to_platform_device(dev); > @@ -218,6 +262,7 @@ static void cdns_ti_remove(struct platform_device *pdev) > } > > static const struct of_device_id cdns_ti_of_match[] = { > + { .compatible = "ti,j7200-usb", }, > { .compatible = "ti,j721e-usb", }, > { .compatible = "ti,am64-usb", }, > {}, > @@ -228,8 +273,9 @@ static struct platform_driver cdns_ti_driver = { > .probe = cdns_ti_probe, > .remove_new = cdns_ti_remove, > .driver = { > - .name = "cdns3-ti", > + .name = "cdns3-ti", > .of_match_table = cdns_ti_of_match, > + .pm = pm_ptr(&cdns_ti_pm_ops), > }, > }; > >
Hello, On Wed Nov 15, 2023 at 12:33 PM CET, Roger Quadros wrote: > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c [...] > > static int cdns_ti_probe(struct platform_device *pdev) > > { > > struct device *dev = &pdev->dev; > > struct device_node *node = pdev->dev.of_node; > > struct cdns_ti *data; > > - int error; > > - u32 reg; > > - int rate_code, i; > > unsigned long rate; > > + int error, i; > > Should we leave rate_code and get rid of i? I see your point about i being usually a temp variable. Using rate_code instead of i means the for-loop becomes rather long (column 87) & should ideally be split. How about moving the data->usb2_refclk_rate_code assignment up, closer to the computation of i? That way we don't reference i three blocks down the road, seemingly out of nowhere. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------ > > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > > if (!data) > > @@ -133,8 +169,6 @@ static int cdns_ti_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - rate_code = i; > > - > > pm_runtime_enable(dev); > > error = pm_runtime_get_sync(dev)> if (error < 0) { > > @@ -142,39 +176,11 @@ static int cdns_ti_probe(struct platform_device *pdev) > > goto err; > > } > > > > - /* assert RESET */ > > - reg = cdns_ti_readl(data, USBSS_W1); > > - reg &= ~USBSS_W1_PWRUP_RST; > > - cdns_ti_writel(data, USBSS_W1, reg); > > - > > - /* set static config */ > > - reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG); > > - reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK; > > - reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT; > > - > > - reg &= ~USBSS1_STATIC_VBUS_SEL_MASK; > > data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > > - if (data->vbus_divider) > > - reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT; > > - > > - cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg); > > - reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG); > > - > > - /* set USB2_ONLY mode if requested */ > > - reg = cdns_ti_readl(data, USBSS_W1); > > data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > - if (data->usb2_only) > > - reg |= USBSS_W1_USB2_ONLY; > > - > > - /* set default modestrap */ > > - reg |= USBSS_W1_MODESTRAP_SEL; > > - reg &= ~USBSS_W1_MODESTRAP_MASK; > > - reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT; > > - cdns_ti_writel(data, USBSS_W1, reg); > > + data->usb2_refclk_rate_code = i; > > because 'i' seems temporary. > > > > > - /* de-assert RESET */ > > - reg |= USBSS_W1_PWRUP_RST; > > - cdns_ti_writel(data, USBSS_W1, reg); > > + cdns_ti_init_hw(data); > > > > error = of_platform_populate(node, NULL, NULL, dev); > > if (error) { > >
+Vibhore, On 16/11/2023 20:56, Théo Lebrun wrote: > Hello Roger, > > On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote: >> On 15/11/2023 17:02, Théo Lebrun wrote: >>> On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote: >>>> On 13/11/2023 16:26, Théo Lebrun wrote: >>>>> Hardware initialisation is only done at probe. The J7200 USB controller >>>>> is reset at resume because of power-domains toggling off & on. We >>>>> therefore (1) toggle PM runtime at suspend/resume & (2) reconfigure the >>>>> hardware at resume. >>>> >>>> at probe we are doing a pm_runtime_get() and never doing a put thus >>>> preventing any runtime PM. >>> >>> Indeed. The get() from probe/resume are in symmetry with the put() from >>> suspend. Is this wrong in some manner? >>> >>>>> index c331bcd2faeb..50b38c4b9c87 100644 >>>>> --- a/drivers/usb/cdns3/cdns3-ti.c >>>>> +++ b/drivers/usb/cdns3/cdns3-ti.c >>>>> @@ -197,6 +197,50 @@ static int cdns_ti_probe(struct platform_device *pdev) >>>>> return error; >>>>> } >>>>> >>>>> +#ifdef CONFIG_PM >>>>> + >>>>> +static int cdns_ti_suspend(struct device *dev) >>>>> +{ >>>>> + struct cdns_ti *data = dev_get_drvdata(dev); >>>>> + >>>>> + if (!of_device_is_compatible(dev_of_node(dev), "ti,j7200-usb")) >>>>> + return 0; >>>>> + >>>>> + pm_runtime_put_sync(data->dev); >>>>> + >>>>> + return 0; >>>> >>>> You might want to check suspend/resume ops in cdns3-plat and >>>> do something similar here. >>> >>> I'm unsure what you are referring to specifically in cdns3-plat? >> >> What I meant is, calling pm_runtime_get/put() from system suspend/resume >> hooks doesn't seem right. >> >> How about using something like pm_runtime_forbid(dev) on devices which >> loose USB context on runtime suspend e.g. J7200. >> So at probe we can get rid of the pm_runtime_get_sync() call. > > What is the goal of enabling PM runtime to then block (ie forbid) it in > its enabled state until system suspend? If USB controller retains context on runtime_suspend on some platforms then we don't want to forbid PM runtime. > > Thinking some more about it and having read parts of the genpd source, > it's unclear to me why there even is some PM runtime calls in this > driver. No runtime_suspend/runtime_resume callbacks are registered. > Also, power-domains work as expected without any PM runtime calls. Probably it was required when the driver was introduced. > > The power domain is turned on when attached to a device > (see genpd_dev_pm_attach). It gets turned off automatically at > suspend_noirq (taking into account the many things that make genpd > complex: multiple devices per PD, subdomains, flags to customise the > behavior, etc.). Removing calls to PM runtime at probe keeps the driver > working. > > So my new proposal would be: remove all all PM runtime calls from this > driver. Anything wrong with this approach? Nothing wrong if we don't expect runtime_pm to work with this driver. > > Only possible reason I see for having PM runtime in this wrapper driver > would be cut the full power-domain when USB isn't used, with some PM > runtime interaction with the children node. But that cannot work > currently as we don't register a runtime_resume to init the hardware, > so this cannot be the current expected behavior. > >> e.g. >> >> pm_runtime_set_active(dev); >> pm_runtime_enable(dev); >> if (cnds_ti->can_loose_context) >> pm_runtime_forbid(dev); >> >> pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY); /* could be 20ms? */ > > Why mention autosuspend in this driver? This will turn the device off in > CNDS_TI_AUTOSUSPEND_DELAY then nothing enables it back using > pm_runtime_get. We have nothing to reconfigure the device, ie no > runtime_resume, so we must not go into runtime suspend. It would be enabled/disabled based on when the child "cdns3,usb" does runtime_resume/suspend. > >> pm_runtime_mark_last_busy(dev); >> pm_runtime_use_autosuspend(dev); >> >> You will need to modify the suspend/resume handlers accordingly. >> https://docs.kernel.org/power/runtime_pm.html#runtime-pm-and-system-sleep >> >> What I'm not sure of is if there are any TI platforms that retain USB context >> on power domain off. Let me get back on this. Till then we can assume that >> all platforms loose USB context on power domain off. > > Good question indeed! Thanks for looking into it. From what I see all 5 > DT nodes which use this driver in upstream devicetrees have a > power-domain configured. So if the behavior is the same on all three TI > platforms (which would be the logical thing to assume) it would make > sense that all controllers lose power at suspend. > > Thanks, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
Hello, On Thu Nov 16, 2023 at 10:44 PM CET, Roger Quadros wrote: > On 16/11/2023 20:56, Théo Lebrun wrote: > > On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote: > >> On 15/11/2023 17:02, Théo Lebrun wrote: > >>> On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote: > >>>> You might want to check suspend/resume ops in cdns3-plat and > >>>> do something similar here. > >>> > >>> I'm unsure what you are referring to specifically in cdns3-plat? > >> > >> What I meant is, calling pm_runtime_get/put() from system suspend/resume > >> hooks doesn't seem right. > >> > >> How about using something like pm_runtime_forbid(dev) on devices which > >> loose USB context on runtime suspend e.g. J7200. > >> So at probe we can get rid of the pm_runtime_get_sync() call. > > > > What is the goal of enabling PM runtime to then block (ie forbid) it in > > its enabled state until system suspend? > > If USB controller retains context on runtime_suspend on some platforms > then we don't want to forbid PM runtime. What's the point of runtime PM if nothing is done based on it? This is the current behavior of the driver. > > Thinking some more about it and having read parts of the genpd source, > > it's unclear to me why there even is some PM runtime calls in this > > driver. No runtime_suspend/runtime_resume callbacks are registered. > > Also, power-domains work as expected without any PM runtime calls. > > Probably it was required when the driver was introduced. I'm not seeing any behavior change in cdns3-ti since its addition in Oct 2019. > > The power domain is turned on when attached to a device > > (see genpd_dev_pm_attach). It gets turned off automatically at > > suspend_noirq (taking into account the many things that make genpd > > complex: multiple devices per PD, subdomains, flags to customise the > > behavior, etc.). Removing calls to PM runtime at probe keeps the driver > > working. > > > > So my new proposal would be: remove all all PM runtime calls from this > > driver. Anything wrong with this approach? > > Nothing wrong if we don't expect runtime_pm to work with this driver. > > > > > Only possible reason I see for having PM runtime in this wrapper driver > > would be cut the full power-domain when USB isn't used, with some PM > > runtime interaction with the children node. But that cannot work > > currently as we don't register a runtime_resume to init the hardware, > > so this cannot be the current expected behavior. > > > >> e.g. > >> > >> pm_runtime_set_active(dev); > >> pm_runtime_enable(dev); > >> if (cnds_ti->can_loose_context) > >> pm_runtime_forbid(dev); > >> > >> pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY); /* could be 20ms? */ > > > > Why mention autosuspend in this driver? This will turn the device off in > > CNDS_TI_AUTOSUSPEND_DELAY then nothing enables it back using > > pm_runtime_get. We have nothing to reconfigure the device, ie no > > runtime_resume, so we must not go into runtime suspend. > > It would be enabled/disabled based on when the child "cdns3,usb" > does runtime_resume/suspend. Why care about being enabled or disabled if we don't do anything based on that? Children does do runtime PM stuff but I don't understand how that could influence us. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 17/11/2023 12:17, Théo Lebrun wrote: > Hello, > > On Thu Nov 16, 2023 at 10:44 PM CET, Roger Quadros wrote: >> On 16/11/2023 20:56, Théo Lebrun wrote: >>> On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote: >>>> On 15/11/2023 17:02, Théo Lebrun wrote: >>>>> On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote: >>>>>> You might want to check suspend/resume ops in cdns3-plat and >>>>>> do something similar here. >>>>> >>>>> I'm unsure what you are referring to specifically in cdns3-plat? >>>> >>>> What I meant is, calling pm_runtime_get/put() from system suspend/resume >>>> hooks doesn't seem right. >>>> >>>> How about using something like pm_runtime_forbid(dev) on devices which >>>> loose USB context on runtime suspend e.g. J7200. >>>> So at probe we can get rid of the pm_runtime_get_sync() call. >>> >>> What is the goal of enabling PM runtime to then block (ie forbid) it in >>> its enabled state until system suspend? >> >> If USB controller retains context on runtime_suspend on some platforms >> then we don't want to forbid PM runtime. > > What's the point of runtime PM if nothing is done based on it? This is > the current behavior of the driver. Even if driver doesn't have runtime_suspend/resume hooks, wouldn't the USB Power domain turn off if we enable runtime PM and allow runtime autosuspend and all children have runtime suspended? > >>> Thinking some more about it and having read parts of the genpd source, >>> it's unclear to me why there even is some PM runtime calls in this >>> driver. No runtime_suspend/runtime_resume callbacks are registered. >>> Also, power-domains work as expected without any PM runtime calls. >> >> Probably it was required when the driver was introduced. > > I'm not seeing any behavior change in cdns3-ti since its addition in Oct > 2019. > >>> The power domain is turned on when attached to a device >>> (see genpd_dev_pm_attach). It gets turned off automatically at >>> suspend_noirq (taking into account the many things that make genpd >>> complex: multiple devices per PD, subdomains, flags to customise the >>> behavior, etc.). Removing calls to PM runtime at probe keeps the driver >>> working. >>> >>> So my new proposal would be: remove all all PM runtime calls from this >>> driver. Anything wrong with this approach? >> >> Nothing wrong if we don't expect runtime_pm to work with this driver. >> >>> >>> Only possible reason I see for having PM runtime in this wrapper driver >>> would be cut the full power-domain when USB isn't used, with some PM >>> runtime interaction with the children node. But that cannot work >>> currently as we don't register a runtime_resume to init the hardware, >>> so this cannot be the current expected behavior. >>> >>>> e.g. >>>> >>>> pm_runtime_set_active(dev); >>>> pm_runtime_enable(dev); >>>> if (cnds_ti->can_loose_context) >>>> pm_runtime_forbid(dev); >>>> >>>> pm_runtime_set_autosuspend_delay(dev, CNDS_TI_AUTOSUSPEND_DELAY); /* could be 20ms? */ >>> >>> Why mention autosuspend in this driver? This will turn the device off in >>> CNDS_TI_AUTOSUSPEND_DELAY then nothing enables it back using >>> pm_runtime_get. We have nothing to reconfigure the device, ie no >>> runtime_resume, so we must not go into runtime suspend. >> >> It would be enabled/disabled based on when the child "cdns3,usb" >> does runtime_resume/suspend. > > Why care about being enabled or disabled if we don't do anything based > on that? Children does do runtime PM stuff but I don't understand how > that could influence us. > > Regards, > > -- > Théo Lebrun, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com >
On 17/11/2023 16:20, Théo Lebrun wrote: > Hi Roger, > > On Fri Nov 17, 2023 at 12:51 PM CET, Roger Quadros wrote: >> On 17/11/2023 12:17, Théo Lebrun wrote: >>> On Thu Nov 16, 2023 at 10:44 PM CET, Roger Quadros wrote: >>>> On 16/11/2023 20:56, Théo Lebrun wrote: >>>>> On Thu Nov 16, 2023 at 1:40 PM CET, Roger Quadros wrote: >>>>>> On 15/11/2023 17:02, Théo Lebrun wrote: >>>>>>> On Wed Nov 15, 2023 at 12:37 PM CET, Roger Quadros wrote: >>>>>>>> You might want to check suspend/resume ops in cdns3-plat and >>>>>>>> do something similar here. >>>>>>> >>>>>>> I'm unsure what you are referring to specifically in cdns3-plat? >>>>>> >>>>>> What I meant is, calling pm_runtime_get/put() from system suspend/resume >>>>>> hooks doesn't seem right. >>>>>> >>>>>> How about using something like pm_runtime_forbid(dev) on devices which >>>>>> loose USB context on runtime suspend e.g. J7200. >>>>>> So at probe we can get rid of the pm_runtime_get_sync() call. >>>>> >>>>> What is the goal of enabling PM runtime to then block (ie forbid) it in >>>>> its enabled state until system suspend? >>>> >>>> If USB controller retains context on runtime_suspend on some platforms >>>> then we don't want to forbid PM runtime. >>> >>> What's the point of runtime PM if nothing is done based on it? This is >>> the current behavior of the driver. >> >> Even if driver doesn't have runtime_suspend/resume hooks, wouldn't >> the USB Power domain turn off if we enable runtime PM and allow runtime >> autosuspend and all children have runtime suspended? > > That cannot be the currently desired behavior as it would require a > runtime_resume implementation that restores this wrapper to its desired > state. > > It could however be something that could be implemented. It would be a > matter of enabling PM runtime and that is it in the probe. No need to > even init the hardware in the probe. Then the runtime_resume > implementation would call the new cdns_ti_init_hw. > > This is what the cdns3-imx wrapper is doing in a way, though what they > need is clocks rather than some registers to be written. > > That all feels like outside the scope of the current patch series > though. > > My suggestion for V2 would still therefore be to remove all PM runtime > as it has no impact. It could be added later down the road if cutting > the power-domain is a goal of yours. OK let's do this.
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. Both only did their hardware configuration at probe time. We are talking about suspend-to-RAM but also suspend-to-idle; we have power-domains that turn off the controller in the second case which explains why s2idle doesn't work either. For cdns3-ti, we implement suspend & resume procedures only targeting our newly created compatible (ti,j7200-usb). The goal is to avoid breaking other platforms; it's unclear to me if power-domains are toggling at s2idle on those as well. About S2R I don't think it's targeted for those platforms. For the HOST role, we add a quirk flag which gets passed as auxiliary data by our wrapper TI driver. That avoids touching the behavior of other platforms; again I'm unsure what is expected and I wouldn't want to break stuff by re-initializing the role. 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> --- Théo Lebrun (6): dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible usb: cdns3-ti: move reg writes from probe into an init_hw helper usb: cdns3-ti: add suspend/resume procedures for J7200 usb: cdns3: support power-off of controller when in host role usb: cdns3-ti: notify cdns core that hardware resets across suspend on J7200 arm64: dts: ti: k3-j7200: use J7200-specific USB compatible .../devicetree/bindings/usb/ti,j721e-usb.yaml | 1 + arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +- drivers/usb/cdns3/cdns3-ti.c | 141 +++++++++++++++------ drivers/usb/cdns3/core.h | 1 + drivers/usb/cdns3/host.c | 20 +++ 5 files changed, 127 insertions(+), 38 deletions(-) --- base-commit: 1d42d5c8f1ca11106579dcaadef4161fee03419e change-id: 20231113-j7200-usb-suspend-2a47f2281e04 Best regards,