| Message ID | 20240307-j7200-usb-suspend-v4-3-5ec7615431f3@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | usb: cdns: fix suspend on J7200 by assuming reset-on-resume | expand |
Hi, On 07/03/2024 11:55, Théo Lebrun wrote: > The hardware initialisation register write sequence is only used at > probe. Move it from being done at explicitely at probe to being done > implicitely by pm_runtime_get_sync() that calls ->runtime_resume(). explicitly / implicitly > > Keep devicetree parsing in probe and add a new field in the private > struct to remember the USB2 refclk rate code computation result. > > This opens the door to having the init sequence being executed later > down the road, at system-wide resume for example. This is NOT currently > happening because runtime PM is disabled at suspend without the > refcount being affected. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- > drivers/usb/cdns3/cdns3-ti.c | 90 +++++++++++++++++++++++++------------------- > 1 file changed, 52 insertions(+), 38 deletions(-) > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > index 5945c4b1e11f..4c8a557e6a6f 100644 > --- a/drivers/usb/cdns3/cdns3-ti.c > +++ b/drivers/usb/cdns3/cdns3-ti.c > @@ -57,6 +57,7 @@ struct cdns_ti { > unsigned vbus_divider:1; > struct clk *usb2_refclk; > struct clk *lpm_clk; > + int usb2_refclk_rate_code; > }; > > static const int cdns_ti_rate_table[] = { /* in KHZ */ > @@ -90,10 +91,8 @@ 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; > > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > if (!data) > @@ -133,7 +132,9 @@ static int cdns_ti_probe(struct platform_device *pdev) > return -EINVAL; > } > > - rate_code = i; > + data->usb2_refclk_rate_code = i; > + data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > + data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > pm_runtime_enable(dev); > error = pm_runtime_get_sync(dev); > @@ -142,40 +143,6 @@ 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); > - > - /* de-assert RESET */ > - reg |= USBSS_W1_PWRUP_RST; > - cdns_ti_writel(data, USBSS_W1, reg); > - > error = of_platform_populate(node, NULL, NULL, dev); > if (error) { > dev_err(dev, "failed to create children: %d\n", error); > @@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev) > platform_set_drvdata(pdev, NULL); > } > > +static int cdns_ti_runtime_resume(struct device *dev) > +{ > + struct cdns_ti *data = dev_get_drvdata(dev); > + u32 reg; > + > + /* 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 |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT; > + > + reg &= ~USBSS1_STATIC_VBUS_SEL_MASK; > + > + 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); > + > + 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); > + > + /* de-assert RESET */ > + reg |= USBSS_W1_PWRUP_RST; > + cdns_ti_writel(data, USBSS_W1, reg); I don't think USB controller requires a reset and re-init between runtime suspend/resume. What you need is reset/re-init during system Resume on certain platforms. So you should move this part of code into a helper function and call it from .probe() and .system_resume() > + > + return 0; > +} > + > +static const struct dev_pm_ops cdns_ti_pm_ops = { > + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) > +}; > + > static const struct of_device_id cdns_ti_of_match[] = { > { .compatible = "ti,j721e-usb", }, > { .compatible = "ti,am64-usb", }, > @@ -224,6 +237,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), > }, > }; > >
Hello Roger, On Thu Mar 7, 2024 at 1:31 PM CET, Roger Quadros wrote: > Hi, > > On 07/03/2024 11:55, Théo Lebrun wrote: > > The hardware initialisation register write sequence is only used at > > probe. Move it from being done at explicitely at probe to being done > > implicitely by pm_runtime_get_sync() that calls ->runtime_resume(). > > explicitly / implicitly > > > > > Keep devicetree parsing in probe and add a new field in the private > > struct to remember the USB2 refclk rate code computation result. > > > > This opens the door to having the init sequence being executed later > > down the road, at system-wide resume for example. This is NOT currently > > happening because runtime PM is disabled at suspend without the > > refcount being affected. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > drivers/usb/cdns3/cdns3-ti.c | 90 +++++++++++++++++++++++++------------------- > > 1 file changed, 52 insertions(+), 38 deletions(-) > > > > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c > > index 5945c4b1e11f..4c8a557e6a6f 100644 > > --- a/drivers/usb/cdns3/cdns3-ti.c > > +++ b/drivers/usb/cdns3/cdns3-ti.c > > @@ -57,6 +57,7 @@ struct cdns_ti { > > unsigned vbus_divider:1; > > struct clk *usb2_refclk; > > struct clk *lpm_clk; > > + int usb2_refclk_rate_code; > > }; > > > > static const int cdns_ti_rate_table[] = { /* in KHZ */ > > @@ -90,10 +91,8 @@ 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; > > > > data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > > if (!data) > > @@ -133,7 +132,9 @@ static int cdns_ti_probe(struct platform_device *pdev) > > return -EINVAL; > > } > > > > - rate_code = i; > > + data->usb2_refclk_rate_code = i; > > + data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); > > + data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); > > > > pm_runtime_enable(dev); > > error = pm_runtime_get_sync(dev); > > @@ -142,40 +143,6 @@ 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); > > - > > - /* de-assert RESET */ > > - reg |= USBSS_W1_PWRUP_RST; > > - cdns_ti_writel(data, USBSS_W1, reg); > > - > > error = of_platform_populate(node, NULL, NULL, dev); > > if (error) { > > dev_err(dev, "failed to create children: %d\n", error); > > @@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev) > > platform_set_drvdata(pdev, NULL); > > } > > > > +static int cdns_ti_runtime_resume(struct device *dev) > > +{ > > + struct cdns_ti *data = dev_get_drvdata(dev); > > + u32 reg; > > + > > + /* 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 |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT; > > + > > + reg &= ~USBSS1_STATIC_VBUS_SEL_MASK; > > + > > + 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); > > + > > + 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); > > + > > + /* de-assert RESET */ > > + reg |= USBSS_W1_PWRUP_RST; > > + cdns_ti_writel(data, USBSS_W1, reg); > > I don't think USB controller requires a reset and re-init between > runtime suspend/resume. > > What you need is reset/re-init during system Resume on certain platforms. > So you should move this part of code into a helper function and call it > from .probe() and .system_resume() Runtime resume is being called at probe() and system-wide resume. See our runtime_resume() implementation as that helper function you are describing. A previous revision did what you are recommending. We leaned towards the current version. See: https://lore.kernel.org/lkml/7h34wxfmwn.fsf@baylibre.com/ Also, assuming we enable runtime PM, a reset and re-init after runtime suspend would be the right thing to do anyways. My reading of drivers/pmdomain/core.c tells me that if our device goes to runtime suspend, domains will be shut down. Our controller will be reset and we'll need to re-init it. The GENPD_FLAG_RPM_ALWAYS_ON flag is of interest to avoid the PD to be shut down during runtime PM. Regards, -- 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 5945c4b1e11f..4c8a557e6a6f 100644 --- a/drivers/usb/cdns3/cdns3-ti.c +++ b/drivers/usb/cdns3/cdns3-ti.c @@ -57,6 +57,7 @@ struct cdns_ti { unsigned vbus_divider:1; struct clk *usb2_refclk; struct clk *lpm_clk; + int usb2_refclk_rate_code; }; static const int cdns_ti_rate_table[] = { /* in KHZ */ @@ -90,10 +91,8 @@ 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; data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); if (!data) @@ -133,7 +132,9 @@ static int cdns_ti_probe(struct platform_device *pdev) return -EINVAL; } - rate_code = i; + data->usb2_refclk_rate_code = i; + data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider"); + data->usb2_only = device_property_read_bool(dev, "ti,usb2-only"); pm_runtime_enable(dev); error = pm_runtime_get_sync(dev); @@ -142,40 +143,6 @@ 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); - - /* de-assert RESET */ - reg |= USBSS_W1_PWRUP_RST; - cdns_ti_writel(data, USBSS_W1, reg); - error = of_platform_populate(node, NULL, NULL, dev); if (error) { dev_err(dev, "failed to create children: %d\n", error); @@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev) platform_set_drvdata(pdev, NULL); } +static int cdns_ti_runtime_resume(struct device *dev) +{ + struct cdns_ti *data = dev_get_drvdata(dev); + u32 reg; + + /* 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 |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT; + + reg &= ~USBSS1_STATIC_VBUS_SEL_MASK; + + 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); + + 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); + + /* de-assert RESET */ + reg |= USBSS_W1_PWRUP_RST; + cdns_ti_writel(data, USBSS_W1, reg); + + return 0; +} + +static const struct dev_pm_ops cdns_ti_pm_ops = { + RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL) +}; + static const struct of_device_id cdns_ti_of_match[] = { { .compatible = "ti,j721e-usb", }, { .compatible = "ti,am64-usb", }, @@ -224,6 +237,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), }, };
The hardware initialisation register write sequence is only used at probe. Move it from being done at explicitely at probe to being done implicitely by pm_runtime_get_sync() that calls ->runtime_resume(). Keep devicetree parsing in probe and add a new field in the private struct to remember the USB2 refclk rate code computation result. This opens the door to having the init sequence being executed later down the road, at system-wide resume for example. This is NOT currently happening because runtime PM is disabled at suspend without the refcount being affected. Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- drivers/usb/cdns3/cdns3-ti.c | 90 +++++++++++++++++++++++++------------------- 1 file changed, 52 insertions(+), 38 deletions(-)