[PATCH 0/6] usb: cdns: fix suspend on J7200 by assuming reset on resume

[PATCH 0/6] usb: cdns: fix suspend on J7200 by assuming reset on resume

[Théo Lebrun]

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,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>

[PATCH 0/6] usb: cdns: fix suspend on J7200 by assuming reset on resume

[Théo Lebrun]

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,
-- 
Théo Lebrun <theo.lebrun@bootlin.com>


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

On this platform, the controller is reset on resume. This makes it have
a different behavior from other platforms.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..77e0c0499936 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -12,6 +12,7 @@ maintainers:
 properties:
   compatible:
     oneOf:
+      - const: ti,j7200-usb
       - const: ti,j721e-usb
       - const: ti,am64-usb
       - items:

-- 
2.41.0

[PATCH 1/6] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Théo Lebrun]

On this platform, the controller is reset on resume. This makes it have
a different behavior from other platforms.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..77e0c0499936 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -12,6 +12,7 @@ maintainers:
 properties:
   compatible:
     oneOf:
+      - const: ti,j7200-usb
       - const: ti,j721e-usb
       - const: ti,am64-usb
       - items:

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Théo Lebrun]

The hardware initialisation register write sequence is only used at
probe. To support suspend/resume with a controller losing power, we
must redo this sequence of writes.

Extract the register write sequence to a new cdns_ti_init_hw function to
reuse it later down the road, at resume.

We keep the devicetree-parsing aspect of the sequence in probe & add a
new field in the private struct to remember the USB2 refclk rate code
computation result.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 76 ++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 5945c4b1e11f..c331bcd2faeb 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 */
@@ -85,15 +86,50 @@ static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
 	writel(value, data->usbss + offset);
 }
 
+static void cdns_ti_init_hw(struct cdns_ti *data)
+{
+	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);
+}
+
 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,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;
 
-	/* 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) {

-- 
2.41.0

[PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Théo Lebrun]

The hardware initialisation register write sequence is only used at
probe. To support suspend/resume with a controller losing power, we
must redo this sequence of writes.

Extract the register write sequence to a new cdns_ti_init_hw function to
reuse it later down the road, at resume.

We keep the devicetree-parsing aspect of the sequence in probe & add a
new field in the private struct to remember the USB2 refclk rate code
computation result.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/cdns3-ti.c | 76 ++++++++++++++++++++++++--------------------
 1 file changed, 41 insertions(+), 35 deletions(-)

diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 5945c4b1e11f..c331bcd2faeb 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 */
@@ -85,15 +86,50 @@ static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
 	writel(value, data->usbss + offset);
 }
 
+static void cdns_ti_init_hw(struct cdns_ti *data)
+{
+	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);
+}
+
 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,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;
 
-	/* 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) {

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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.

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;
+}
+
+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),
 	},
 };
 

-- 
2.41.0

[PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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.

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;
+}
+
+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),
 	},
 };
 

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Théo Lebrun]

The controller is not being reconfigured at resume. Change resume to
redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.

Platform data comes from the parent driver (eg cdns3-ti).

The quirk should be passed if the platform driver knows that the
controller might be in reset state at resume. We do NOT reconfigure the
hardware without this quirk to avoid losing state if we did a suspend
without reset.

If the quirk is on, we notify the xHCI subsystem that:

1. We reset on resume. It will therefore redo the xHC init & trigger
   such message as "root hub lost power or was reset" in dmesg.

2. It should disable/enable clocks on suspend/resume. This does not
   matter on our platform as xhci-plat does not get access to any clock
   but it would be the right thing to do if we indeed had such clocks.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/core.h |  1 +
 drivers/usb/cdns3/host.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 81a9c9d6be08..7487067ba23f 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -44,6 +44,7 @@ struct cdns3_platform_data {
 			bool suspend, bool wakeup);
 	unsigned long quirks;
 #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
+#define CDNS3_RESET_ON_RESUME		BIT(1)
 };
 
 /**
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 6164fc4c96a4..a81019a7c8cc 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -88,6 +88,9 @@ static int __cdns_host_init(struct cdns *cdns)
 		goto err1;
 	}
 
+	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
+		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
+
 	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
 		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
 
@@ -124,6 +127,18 @@ static void cdns_host_exit(struct cdns *cdns)
 	cdns_drd_host_off(cdns);
 }
 
+static int cdns_host_suspend(struct cdns *cdns, bool do_wakeup)
+{
+	if (!do_wakeup)
+		cdns_drd_host_off(cdns);
+	return 0;
+}
+
+static int cdns_host_resume(struct cdns *cdns, bool hibernated)
+{
+	return cdns_drd_host_on(cdns);
+}
+
 int cdns_host_init(struct cdns *cdns)
 {
 	struct cdns_role_driver *rdrv;
@@ -137,6 +152,11 @@ int cdns_host_init(struct cdns *cdns)
 	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
 	rdrv->name	= "host";
 
+	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME) {
+		rdrv->suspend = cdns_host_suspend;
+		rdrv->resume = cdns_host_resume;
+	}
+
 	cdns->roles[USB_ROLE_HOST] = rdrv;
 
 	return 0;

-- 
2.41.0

[PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Théo Lebrun]

The controller is not being reconfigured at resume. Change resume to
redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.

Platform data comes from the parent driver (eg cdns3-ti).

The quirk should be passed if the platform driver knows that the
controller might be in reset state at resume. We do NOT reconfigure the
hardware without this quirk to avoid losing state if we did a suspend
without reset.

If the quirk is on, we notify the xHCI subsystem that:

1. We reset on resume. It will therefore redo the xHC init & trigger
   such message as "root hub lost power or was reset" in dmesg.

2. It should disable/enable clocks on suspend/resume. This does not
   matter on our platform as xhci-plat does not get access to any clock
   but it would be the right thing to do if we indeed had such clocks.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/usb/cdns3/core.h |  1 +
 drivers/usb/cdns3/host.c | 20 ++++++++++++++++++++
 2 files changed, 21 insertions(+)

diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
index 81a9c9d6be08..7487067ba23f 100644
--- a/drivers/usb/cdns3/core.h
+++ b/drivers/usb/cdns3/core.h
@@ -44,6 +44,7 @@ struct cdns3_platform_data {
 			bool suspend, bool wakeup);
 	unsigned long quirks;
 #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
+#define CDNS3_RESET_ON_RESUME		BIT(1)
 };
 
 /**
diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
index 6164fc4c96a4..a81019a7c8cc 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -88,6 +88,9 @@ static int __cdns_host_init(struct cdns *cdns)
 		goto err1;
 	}
 
+	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
+		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
+
 	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
 		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
 
@@ -124,6 +127,18 @@ static void cdns_host_exit(struct cdns *cdns)
 	cdns_drd_host_off(cdns);
 }
 
+static int cdns_host_suspend(struct cdns *cdns, bool do_wakeup)
+{
+	if (!do_wakeup)
+		cdns_drd_host_off(cdns);
+	return 0;
+}
+
+static int cdns_host_resume(struct cdns *cdns, bool hibernated)
+{
+	return cdns_drd_host_on(cdns);
+}
+
 int cdns_host_init(struct cdns *cdns)
 {
 	struct cdns_role_driver *rdrv;
@@ -137,6 +152,11 @@ int cdns_host_init(struct cdns *cdns)
 	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
 	rdrv->name	= "host";
 
+	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME) {
+		rdrv->suspend = cdns_host_suspend;
+		rdrv->resume = cdns_host_resume;
+	}
+
 	cdns->roles[USB_ROLE_HOST] = rdrv;
 
 	return 0;

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] usb: cdns3-ti: notify cdns core that hardware resets across suspend on J7200

[Théo Lebrun]

Use the CDNS3_RESET_ON_RESUME quirk flag to inform the cdns3 core that
our J7200 USB controller will lose power during suspend. It therefore
must be reconfigured when we resume.

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 50b38c4b9c87..c65714c783fb 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
@@ -127,6 +128,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;
@@ -182,7 +184,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);
 		goto err;
@@ -261,8 +264,20 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 
+static struct cdns3_platform_data cdns_ti_j7200_pdata = {
+	.quirks = CDNS3_RESET_ON_RESUME,
+};
+
+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, },
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
 	{},

-- 
2.41.0

[PATCH 5/6] usb: cdns3-ti: notify cdns core that hardware resets across suspend on J7200

[Théo Lebrun]

Use the CDNS3_RESET_ON_RESUME quirk flag to inform the cdns3 core that
our J7200 USB controller will lose power during suspend. It therefore
must be reconfigured when we resume.

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 50b38c4b9c87..c65714c783fb 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
@@ -127,6 +128,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;
@@ -182,7 +184,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);
 		goto err;
@@ -261,8 +264,20 @@ static void cdns_ti_remove(struct platform_device *pdev)
 	platform_set_drvdata(pdev, NULL);
 }
 
+static struct cdns3_platform_data cdns_ti_j7200_pdata = {
+	.quirks = CDNS3_RESET_ON_RESUME,
+};
+
+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, },
 	{ .compatible = "ti,j721e-usb", },
 	{ .compatible = "ti,am64-usb", },
 	{},

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Théo Lebrun]

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";
 		reg = <0x00 0x4104000 0x00 0x100>;
 		dma-coherent;
 		power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;

-- 
2.41.0

[PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Théo Lebrun]

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";
 		reg = <0x00 0x4104000 0x00 0x100>;
 		dma-coherent;
 		power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;

-- 
2.41.0


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Gregory CLEMENT]

Hello Théo,

> 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.
>
> 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;

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.

Besides this you still can add my

Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory

> +
> +	pm_runtime_put_sync(data->dev);
> +
> +	return 0;
> +}
> +
> +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),
>  	},
>  };
>  
>
> -- 
> 2.41.0
>
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Gregory CLEMENT]

Hello Théo,

> 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.
>
> 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;

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.

Besides this you still can add my

Reviewed-by: Gregory CLEMENT <gregory.clement@bootlin.com>

Thanks,

Gregory

> +
> +	pm_runtime_put_sync(data->dev);
> +
> +	return 0;
> +}
> +
> +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),
>  	},
>  };
>  
>
> -- 
> 2.41.0
>
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 956 bytes --]

On Mon, Nov 13, 2023 at 03:26:56PM +0100, Théo Lebrun wrote:
> On this platform, the controller is reset on resume. This makes it have
> a different behavior from other platforms.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> index 95ff9791baea..77e0c0499936 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -12,6 +12,7 @@ maintainers:
>  properties:
>    compatible:
>      oneOf:
> +      - const: ti,j7200-usb
>        - const: ti,j721e-usb
>        - const: ti,am64-usb
>        - items:
> 
> -- 
> 2.41.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/6] dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible

[Conor Dooley]

[-- Attachment #1.1: Type: text/plain, Size: 956 bytes --]

On Mon, Nov 13, 2023 at 03:26:56PM +0100, Théo Lebrun wrote:
> On this platform, the controller is reset on resume. This makes it have
> a different behavior from other platforms.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

Cheers,
Conor.

> ---
>  Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> index 95ff9791baea..77e0c0499936 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -12,6 +12,7 @@ maintainers:
>  properties:
>    compatible:
>      oneOf:
> +      - const: ti,j7200-usb
>        - const: ti,j721e-usb
>        - const: ti,am64-usb
>        - items:
> 
> -- 
> 2.41.0
> 

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Peter Chen]

On 23-11-13 15:26:59, Théo Lebrun wrote:
> The controller is not being reconfigured at resume. Change resume to
> redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.

Current logic has power off judgement, see cdns3_controller_resume for
detail.

> 
> Platform data comes from the parent driver (eg cdns3-ti).
> 
> The quirk should be passed if the platform driver knows that the
> controller might be in reset state at resume. We do NOT reconfigure the
> hardware without this quirk to avoid losing state if we did a suspend
> without reset.
> 
> If the quirk is on, we notify the xHCI subsystem that:
> 
> 1. We reset on resume. It will therefore redo the xHC init & trigger
>    such message as "root hub lost power or was reset" in dmesg.
> 
> 2. It should disable/enable clocks on suspend/resume. This does not
>    matter on our platform as xhci-plat does not get access to any clock
>    but it would be the right thing to do if we indeed had such clocks.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/core.h |  1 +
>  drivers/usb/cdns3/host.c | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index 81a9c9d6be08..7487067ba23f 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -44,6 +44,7 @@ struct cdns3_platform_data {
>  			bool suspend, bool wakeup);
>  	unsigned long quirks;
>  #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
> +#define CDNS3_RESET_ON_RESUME		BIT(1)
>  };
>  
>  /**
> diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
> index 6164fc4c96a4..a81019a7c8cc 100644
> --- a/drivers/usb/cdns3/host.c
> +++ b/drivers/usb/cdns3/host.c
> @@ -88,6 +88,9 @@ static int __cdns_host_init(struct cdns *cdns)
>  		goto err1;
>  	}
>  
> +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> +

If you set this flag, how could you support the USB remote wakeup
request? In that case, the USB bus does not expect re-enumeration.

>  	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
>  		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
>  
> @@ -124,6 +127,18 @@ static void cdns_host_exit(struct cdns *cdns)
>  	cdns_drd_host_off(cdns);
>  }
>  
> +static int cdns_host_suspend(struct cdns *cdns, bool do_wakeup)
> +{
> +	if (!do_wakeup)
> +		cdns_drd_host_off(cdns);
> +	return 0;
> +}
> +
> +static int cdns_host_resume(struct cdns *cdns, bool hibernated)
> +{
> +	return cdns_drd_host_on(cdns);

This one will redo if controller's power is off, please consider both
on and power situation.

> +}
> +
>  int cdns_host_init(struct cdns *cdns)
>  {
>  	struct cdns_role_driver *rdrv;
> @@ -137,6 +152,11 @@ int cdns_host_init(struct cdns *cdns)
>  	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
>  	rdrv->name	= "host";
>  
> +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME) {
> +		rdrv->suspend = cdns_host_suspend;
> +		rdrv->resume = cdns_host_resume;
> +	}
> +
>  	cdns->roles[USB_ROLE_HOST] = rdrv;
>  
>  	return 0;
> 
> -- 
> 2.41.0
> 

-- 

Thanks,
Peter Chen

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Peter Chen]

On 23-11-13 15:26:59, Théo Lebrun wrote:
> The controller is not being reconfigured at resume. Change resume to
> redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.

Current logic has power off judgement, see cdns3_controller_resume for
detail.

> 
> Platform data comes from the parent driver (eg cdns3-ti).
> 
> The quirk should be passed if the platform driver knows that the
> controller might be in reset state at resume. We do NOT reconfigure the
> hardware without this quirk to avoid losing state if we did a suspend
> without reset.
> 
> If the quirk is on, we notify the xHCI subsystem that:
> 
> 1. We reset on resume. It will therefore redo the xHC init & trigger
>    such message as "root hub lost power or was reset" in dmesg.
> 
> 2. It should disable/enable clocks on suspend/resume. This does not
>    matter on our platform as xhci-plat does not get access to any clock
>    but it would be the right thing to do if we indeed had such clocks.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/core.h |  1 +
>  drivers/usb/cdns3/host.c | 20 ++++++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/drivers/usb/cdns3/core.h b/drivers/usb/cdns3/core.h
> index 81a9c9d6be08..7487067ba23f 100644
> --- a/drivers/usb/cdns3/core.h
> +++ b/drivers/usb/cdns3/core.h
> @@ -44,6 +44,7 @@ struct cdns3_platform_data {
>  			bool suspend, bool wakeup);
>  	unsigned long quirks;
>  #define CDNS3_DEFAULT_PM_RUNTIME_ALLOW	BIT(0)
> +#define CDNS3_RESET_ON_RESUME		BIT(1)
>  };
>  
>  /**
> diff --git a/drivers/usb/cdns3/host.c b/drivers/usb/cdns3/host.c
> index 6164fc4c96a4..a81019a7c8cc 100644
> --- a/drivers/usb/cdns3/host.c
> +++ b/drivers/usb/cdns3/host.c
> @@ -88,6 +88,9 @@ static int __cdns_host_init(struct cdns *cdns)
>  		goto err1;
>  	}
>  
> +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> +

If you set this flag, how could you support the USB remote wakeup
request? In that case, the USB bus does not expect re-enumeration.

>  	if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
>  		cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
>  
> @@ -124,6 +127,18 @@ static void cdns_host_exit(struct cdns *cdns)
>  	cdns_drd_host_off(cdns);
>  }
>  
> +static int cdns_host_suspend(struct cdns *cdns, bool do_wakeup)
> +{
> +	if (!do_wakeup)
> +		cdns_drd_host_off(cdns);
> +	return 0;
> +}
> +
> +static int cdns_host_resume(struct cdns *cdns, bool hibernated)
> +{
> +	return cdns_drd_host_on(cdns);

This one will redo if controller's power is off, please consider both
on and power situation.

> +}
> +
>  int cdns_host_init(struct cdns *cdns)
>  {
>  	struct cdns_role_driver *rdrv;
> @@ -137,6 +152,11 @@ int cdns_host_init(struct cdns *cdns)
>  	rdrv->state	= CDNS_ROLE_STATE_INACTIVE;
>  	rdrv->name	= "host";
>  
> +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME) {
> +		rdrv->suspend = cdns_host_suspend;
> +		rdrv->resume = cdns_host_resume;
> +	}
> +
>  	cdns->roles[USB_ROLE_HOST] = rdrv;
>  
>  	return 0;
> 
> -- 
> 2.41.0
> 

-- 

Thanks,
Peter Chen

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Gregory CLEMENT]

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
>
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

Re: [PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Gregory CLEMENT]

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
>
>

-- 
Gregory Clement, Bootlin
Embedded Linux and Kernel engineering
http://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Théo Lebrun]

Hello,

On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> On 23-11-13 15:26:59, Théo Lebrun wrote:
> > The controller is not being reconfigured at resume. Change resume to
> > redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.
>
> Current logic has power off judgement, see cdns3_controller_resume for
> detail.

Indeed! Thanks for the pointer. I had not noticed that, those patches
come from an older kernel which didn't have it. That'll make for less
changes; patches 4 & 5 can go away.

> > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > +
>
> If you set this flag, how could you support the USB remote wakeup
> request? In that case, the USB bus does not expect re-enumeration.

We didn't support remote USB wakeup. Only S2R mattered in our case and
USB remote wakeup wasn't a possibility.

> > +static int cdns_host_resume(struct cdns *cdns, bool hibernated)
> > +{
> > +	return cdns_drd_host_on(cdns);
>
> This one will redo if controller's power is off, please consider both
> on and power situation.

Clearly. Can see that at runtime.

Thanks for the review!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Théo Lebrun]

Hello,

On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> On 23-11-13 15:26:59, Théo Lebrun wrote:
> > The controller is not being reconfigured at resume. Change resume to
> > redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.
>
> Current logic has power off judgement, see cdns3_controller_resume for
> detail.

Indeed! Thanks for the pointer. I had not noticed that, those patches
come from an older kernel which didn't have it. That'll make for less
changes; patches 4 & 5 can go away.

> > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > +
>
> If you set this flag, how could you support the USB remote wakeup
> request? In that case, the USB bus does not expect re-enumeration.

We didn't support remote USB wakeup. Only S2R mattered in our case and
USB remote wakeup wasn't a possibility.

> > +static int cdns_host_resume(struct cdns *cdns, bool hibernated)
> > +{
> > +	return cdns_drd_host_on(cdns);
>
> This one will redo if controller's power is off, please consider both
> on and power situation.

Clearly. Can see that at runtime.

Thanks for the review!

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Théo Lebrun]

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

Re: [PATCH 6/6] arm64: dts: ti: k3-j7200: use J7200-specific USB compatible

[Théo Lebrun]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Roger Quadros]

Hi Théo,

On 13/11/2023 16:26, Théo Lebrun wrote:
> The hardware initialisation register write sequence is only used at
> probe. To support suspend/resume with a controller losing power, we
> must redo this sequence of writes.
> 
> Extract the register write sequence to a new cdns_ti_init_hw function to
> reuse it later down the road, at resume.
> 
> We keep the devicetree-parsing aspect of the sequence in probe & add a
> new field in the private struct to remember the USB2 refclk rate code
> computation result.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 76 ++++++++++++++++++++++++--------------------
>  1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 5945c4b1e11f..c331bcd2faeb 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 */
> @@ -85,15 +86,50 @@ static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
>  	writel(value, data->usbss + offset);
>  }
>  
> +static void cdns_ti_init_hw(struct cdns_ti *data)
> +{
> +	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);
> +}
> +
>  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?

>  
>  	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) {
> 

-- 
cheers,
-roger

Re: [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Roger Quadros]

Hi Théo,

On 13/11/2023 16:26, Théo Lebrun wrote:
> The hardware initialisation register write sequence is only used at
> probe. To support suspend/resume with a controller losing power, we
> must redo this sequence of writes.
> 
> Extract the register write sequence to a new cdns_ti_init_hw function to
> reuse it later down the road, at resume.
> 
> We keep the devicetree-parsing aspect of the sequence in probe & add a
> new field in the private struct to remember the USB2 refclk rate code
> computation result.
> 
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> ---
>  drivers/usb/cdns3/cdns3-ti.c | 76 ++++++++++++++++++++++++--------------------
>  1 file changed, 41 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 5945c4b1e11f..c331bcd2faeb 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 */
> @@ -85,15 +86,50 @@ static inline void cdns_ti_writel(struct cdns_ti *data, u32 offset, u32 value)
>  	writel(value, data->usbss + offset);
>  }
>  
> +static void cdns_ti_init_hw(struct cdns_ti *data)
> +{
> +	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);
> +}
> +
>  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?

>  
>  	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) {
> 

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

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),
>  	},
>  };
>  
> 

-- 
cheers,
-roger

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

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),
>  	},
>  };
>  
> 

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Théo Lebrun]

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) {
> > 

Re: [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Théo Lebrun]

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) {
> > 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hi Roger,

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?

 - Using pm_runtime_status_suspended() to get the current PM runtime
   state & act on it? I don't see why because we know the pm_runtime
   state is a single put() at probe.

 - Having a `in_lpm` flag to track low-power mode state? I wouldn't see
   why we'd want that as we don't register runtime_suspend &
   runtime_resume callbacks and system syspend/resume can be assumed to
   be called in the right order.

 - Checking the `device_may_wakeup()`? That doesn't apply to this driver
   which cannot be a wakeup source.

Thanks for your review!
Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

------------------------------------------------------------------------

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hi Roger,

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?

 - Using pm_runtime_status_suspended() to get the current PM runtime
   state & act on it? I don't see why because we know the pm_runtime
   state is a single put() at probe.

 - Having a `in_lpm` flag to track low-power mode state? I wouldn't see
   why we'd want that as we don't register runtime_suspend &
   runtime_resume callbacks and system syspend/resume can be assumed to
   be called in the right order.

 - Checking the `device_may_wakeup()`? That doesn't apply to this driver
   which cannot be a wakeup source.

Thanks for your review!
Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

------------------------------------------------------------------------


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Roger Quadros]

On 15/11/2023 16:23, Théo Lebrun wrote:
> 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.

That is much better. Thanks!

-- 
cheers,
-roger

Re: [PATCH 2/6] usb: cdns3-ti: move reg writes from probe into an init_hw helper

[Roger Quadros]

On 15/11/2023 16:23, Théo Lebrun wrote:
> 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.

That is much better. Thanks!

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

On 15/11/2023 17:02, Théo Lebrun wrote:
> Hi Roger,
> 
> 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.
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? */
        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.

One comment below.

> +	return ret;
> +}


> 
>  - Using pm_runtime_status_suspended() to get the current PM runtime
>    state & act on it? I don't see why because we know the pm_runtime
>    state is a single put() at probe.
> 
>  - Having a `in_lpm` flag to track low-power mode state? I wouldn't see
>    why we'd want that as we don't register runtime_suspend &
>    runtime_resume callbacks and system syspend/resume can be assumed to
>    be called in the right order.
> 
>  - Checking the `device_may_wakeup()`? That doesn't apply to this driver
>    which cannot be a wakeup source.
> 
> Thanks for your review!
> Regards,
> 
> --> +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);

Why do you do a pm_runtime_disable() here?

> +	return ret;
> +}


> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> ------------------------------------------------------------------------
> 
> 

-- 
cheers,
-roger

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

On 15/11/2023 17:02, Théo Lebrun wrote:
> Hi Roger,
> 
> 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.
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? */
        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.

One comment below.

> +	return ret;
> +}


> 
>  - Using pm_runtime_status_suspended() to get the current PM runtime
>    state & act on it? I don't see why because we know the pm_runtime
>    state is a single put() at probe.
> 
>  - Having a `in_lpm` flag to track low-power mode state? I wouldn't see
>    why we'd want that as we don't register runtime_suspend &
>    runtime_resume callbacks and system syspend/resume can be assumed to
>    be called in the right order.
> 
>  - Checking the `device_may_wakeup()`? That doesn't apply to this driver
>    which cannot be a wakeup source.
> 
> Thanks for your review!
> Regards,
> 
> --> +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);

Why do you do a pm_runtime_disable() here?

> +	return ret;
> +}


> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
> 
> ------------------------------------------------------------------------
> 
> 

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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?

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.

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?

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.

>         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

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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?

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.

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?

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.

>         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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

+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

-- 
cheers,
-roger

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

+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

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Peter Chen]

On 23-11-14 12:10:18, Théo Lebrun wrote:
> Hello,
> 
> On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > On 23-11-13 15:26:59, Théo Lebrun wrote:
> > > The controller is not being reconfigured at resume. Change resume to
> > > redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.
> >
> > Current logic has power off judgement, see cdns3_controller_resume for
> > detail.
> 
> Indeed! Thanks for the pointer. I had not noticed that, those patches
> come from an older kernel which didn't have it. That'll make for less
> changes; patches 4 & 5 can go away.
> 
> > > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > +
> >
> > If you set this flag, how could you support the USB remote wakeup
> > request? In that case, the USB bus does not expect re-enumeration.
> 
> We didn't support remote USB wakeup. Only S2R mattered in our case and
> USB remote wakeup wasn't a possibility.

Without this patch, will below be hit for your platform:


	/* re-initialize the HC on Restore Error, or Host Controller Error */
	if (temp & (STS_SRE | STS_HCE)) {
		reinit_xhc = true;
		if (!xhci->broken_suspend)
			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
	}


-- 

Thanks,
Peter Chen

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Peter Chen]

On 23-11-14 12:10:18, Théo Lebrun wrote:
> Hello,
> 
> On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > On 23-11-13 15:26:59, Théo Lebrun wrote:
> > > The controller is not being reconfigured at resume. Change resume to
> > > redo hardware config if quirk CDNS3_RESET_ON_RESUME is active.
> >
> > Current logic has power off judgement, see cdns3_controller_resume for
> > detail.
> 
> Indeed! Thanks for the pointer. I had not noticed that, those patches
> come from an older kernel which didn't have it. That'll make for less
> changes; patches 4 & 5 can go away.
> 
> > > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > +
> >
> > If you set this flag, how could you support the USB remote wakeup
> > request? In that case, the USB bus does not expect re-enumeration.
> 
> We didn't support remote USB wakeup. Only S2R mattered in our case and
> USB remote wakeup wasn't a possibility.

Without this patch, will below be hit for your platform:


	/* re-initialize the HC on Restore Error, or Host Controller Error */
	if (temp & (STS_SRE | STS_HCE)) {
		reinit_xhc = true;
		if (!xhci->broken_suspend)
			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
	}


-- 

Thanks,
Peter Chen

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Théo Lebrun]

Hello,

On Fri Nov 17, 2023 at 4:38 AM CET, Peter Chen wrote:
> On 23-11-14 12:10:18, Théo Lebrun wrote:
> > Hello,
> > 
> > On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > > > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > > +
> > >
> > > If you set this flag, how could you support the USB remote wakeup
> > > request? In that case, the USB bus does not expect re-enumeration.
> > 
> > We didn't support remote USB wakeup. Only S2R mattered in our case and
> > USB remote wakeup wasn't a possibility.
>
> Without this patch, will below be hit for your platform:
>
> 	/* re-initialize the HC on Restore Error, or Host Controller Error */
> 	if (temp & (STS_SRE | STS_HCE)) {
> 		reinit_xhc = true;
> 		if (!xhci->broken_suspend)
> 			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> 	}

Yes it hits. The warning as well. How big of an issue is that?

My understanding is that this is the expected behavior with reset on
resume if we don't explicitely pass the flag XHCI_RESET_ON_RESUME. I
don't think we should be having the broken_suspend bit set as its
mentioning some specific quirk on AMD hardware.

Is the only expected difference inbetween having CDNS3_RESET_ON_RESUME &
not having it is resume time? For reference, the status read is 0x411
ie STS_HALT | STS_PCD | STS_SRE. xhc_state is zero.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Théo Lebrun]

Hello,

On Fri Nov 17, 2023 at 4:38 AM CET, Peter Chen wrote:
> On 23-11-14 12:10:18, Théo Lebrun wrote:
> > Hello,
> > 
> > On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > > > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > > +
> > >
> > > If you set this flag, how could you support the USB remote wakeup
> > > request? In that case, the USB bus does not expect re-enumeration.
> > 
> > We didn't support remote USB wakeup. Only S2R mattered in our case and
> > USB remote wakeup wasn't a possibility.
>
> Without this patch, will below be hit for your platform:
>
> 	/* re-initialize the HC on Restore Error, or Host Controller Error */
> 	if (temp & (STS_SRE | STS_HCE)) {
> 		reinit_xhc = true;
> 		if (!xhci->broken_suspend)
> 			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> 	}

Yes it hits. The warning as well. How big of an issue is that?

My understanding is that this is the expected behavior with reset on
resume if we don't explicitely pass the flag XHCI_RESET_ON_RESUME. I
don't think we should be having the broken_suspend bit set as its
mentioning some specific quirk on AMD hardware.

Is the only expected difference inbetween having CDNS3_RESET_ON_RESUME &
not having it is resume time? For reference, the status read is 0x411
ie STS_HALT | STS_PCD | STS_SRE. xhc_state is zero.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

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
> 

-- 
cheers,
-roger

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

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
> 

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

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.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

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.

-- 
cheers,
-roger

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Roger Quadros]

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.

-- 
cheers,
-roger

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Peter Chen]

On 23-11-17 10:58:12, Théo Lebrun wrote:
> Hello,
> 
> On Fri Nov 17, 2023 at 4:38 AM CET, Peter Chen wrote:
> > On 23-11-14 12:10:18, Théo Lebrun wrote:
> > > Hello,
> > > 
> > > On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > > > > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > > > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > > > +
> > > >
> > > > If you set this flag, how could you support the USB remote wakeup
> > > > request? In that case, the USB bus does not expect re-enumeration.
> > > 
> > > We didn't support remote USB wakeup. Only S2R mattered in our case and
> > > USB remote wakeup wasn't a possibility.
> >
> > Without this patch, will below be hit for your platform:
> >
> > 	/* re-initialize the HC on Restore Error, or Host Controller Error */
> > 	if (temp & (STS_SRE | STS_HCE)) {
> > 		reinit_xhc = true;
> > 		if (!xhci->broken_suspend)
> > 			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> > 	}
> 
> Yes it hits. The warning as well. How big of an issue is that?
> 
> My understanding is that this is the expected behavior with reset on
> resume if we don't explicitely pass the flag XHCI_RESET_ON_RESUME. I
> don't think we should be having the broken_suspend bit set as its
> mentioning some specific quirk on AMD hardware.
> 
> Is the only expected difference inbetween having CDNS3_RESET_ON_RESUME &
> not having it is resume time? For reference, the status read is 0x411
> ie STS_HALT | STS_PCD | STS_SRE. xhc_state is zero.
> 

Yes. I know some xHCI controllers have powered off during suspend will
hit it, just would like to confirm if it is the same with yours. If you
don't want remote wakeup, it is most probably the same with quirks you set.

-- 

Thanks,
Peter Chen

Re: [PATCH 4/6] usb: cdns3: support power-off of controller when in host role

[Peter Chen]

On 23-11-17 10:58:12, Théo Lebrun wrote:
> Hello,
> 
> On Fri Nov 17, 2023 at 4:38 AM CET, Peter Chen wrote:
> > On 23-11-14 12:10:18, Théo Lebrun wrote:
> > > Hello,
> > > 
> > > On Tue Nov 14, 2023 at 9:38 AM CET, Peter Chen wrote:
> > > > > +	if (cdns->pdata && cdns->pdata->quirks & CDNS3_RESET_ON_RESUME)
> > > > > +		cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME | XHCI_SUSPEND_RESUME_CLKS;
> > > > > +
> > > >
> > > > If you set this flag, how could you support the USB remote wakeup
> > > > request? In that case, the USB bus does not expect re-enumeration.
> > > 
> > > We didn't support remote USB wakeup. Only S2R mattered in our case and
> > > USB remote wakeup wasn't a possibility.
> >
> > Without this patch, will below be hit for your platform:
> >
> > 	/* re-initialize the HC on Restore Error, or Host Controller Error */
> > 	if (temp & (STS_SRE | STS_HCE)) {
> > 		reinit_xhc = true;
> > 		if (!xhci->broken_suspend)
> > 			xhci_warn(xhci, "xHC error in resume, USBSTS 0x%x, Reinit\n", temp);
> > 	}
> 
> Yes it hits. The warning as well. How big of an issue is that?
> 
> My understanding is that this is the expected behavior with reset on
> resume if we don't explicitely pass the flag XHCI_RESET_ON_RESUME. I
> don't think we should be having the broken_suspend bit set as its
> mentioning some specific quirk on AMD hardware.
> 
> Is the only expected difference inbetween having CDNS3_RESET_ON_RESUME &
> not having it is resume time? For reference, the status read is 0x411
> ie STS_HALT | STS_PCD | STS_SRE. xhc_state is zero.
> 

Yes. I know some xHCI controllers have powered off during suspend will
hit it, just would like to confirm if it is the same with yours. If you
don't want remote wakeup, it is most probably the same with quirks you set.

-- 

Thanks,
Peter Chen

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> 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.

The point is to signal to the power domain the device is in that it can
power on/off.  These IP blocks are (re)used on many different SoCs, so
the driver should not make any assumptions about what power domain it is
in (if any.)

>> 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.

Or, for this USB IP block to be in a power domain that has memory
retention, in which case the power domain can still go off to save
power, but not lose context.

> 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 the way.

> 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.

It may have no impact on the platform you are on, but it's very likely
to have an impact on other platforms with this same IP. 

Kevin

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> 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.

The point is to signal to the power domain the device is in that it can
power on/off.  These IP blocks are (re)used on many different SoCs, so
the driver should not make any assumptions about what power domain it is
in (if any.)

>> 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.

Or, for this USB IP block to be in a power domain that has memory
retention, in which case the power domain can still go off to save
power, but not lose context.

> 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 the way.

> 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.

It may have no impact on the platform you are on, but it's very likely
to have an impact on other platforms with this same IP. 

Kevin



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hello,

On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > 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.
>
> The point is to signal to the power domain the device is in that it can
> power on/off.  These IP blocks are (re)used on many different SoCs, so
> the driver should not make any assumptions about what power domain it is
> in (if any.)

On my platform, when the device is attached to the PD it gets turned on.
That feels logical to me: if a driver is not RPM aware it "just works".
Are there platforms where RPM must get enabled for the attached
power-domains to be turned on?

The call chain that attaches & enables PD is platform_probe ->
dev_pm_domain_attach. That function takes a bool power_on which is
always true. In the genpd case, genpd_dev_pm_attach
calls __genpd_dev_pm_attach which does a genpd_power_on.

Things I've not accounted for:

 - ACPI looks like it does the same but I've not checked. It gets passed
   the power_on bool argument.

 - genpd_dev_pm_attach early returns with no error if there are multiple
   PM domains attached to the device. There are many platforms in the
   case according to some devicetree grepping. I can imagine a behavior
   difference where dev_pm_domain callpaths handle that differently in
   the RPM case. Is that what we are discussing?

> >> 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.
>
> Or, for this USB IP block to be in a power domain that has memory
> retention, in which case the power domain can still go off to save
> power, but not lose context.
>
> > 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 the way.

I agree & I have a prototype, but that requires some work on the child
devices as from what I remember they are not eager to go to runtime
suspend (ie a driver might be holding a reference).

I feel this leans outside the scope of this patch series though.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hello,

On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > 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.
>
> The point is to signal to the power domain the device is in that it can
> power on/off.  These IP blocks are (re)used on many different SoCs, so
> the driver should not make any assumptions about what power domain it is
> in (if any.)

On my platform, when the device is attached to the PD it gets turned on.
That feels logical to me: if a driver is not RPM aware it "just works".
Are there platforms where RPM must get enabled for the attached
power-domains to be turned on?

The call chain that attaches & enables PD is platform_probe ->
dev_pm_domain_attach. That function takes a bool power_on which is
always true. In the genpd case, genpd_dev_pm_attach
calls __genpd_dev_pm_attach which does a genpd_power_on.

Things I've not accounted for:

 - ACPI looks like it does the same but I've not checked. It gets passed
   the power_on bool argument.

 - genpd_dev_pm_attach early returns with no error if there are multiple
   PM domains attached to the device. There are many platforms in the
   case according to some devicetree grepping. I can imagine a behavior
   difference where dev_pm_domain callpaths handle that differently in
   the RPM case. Is that what we are discussing?

> >> 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.
>
> Or, for this USB IP block to be in a power domain that has memory
> retention, in which case the power domain can still go off to save
> power, but not lose context.
>
> > 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 the way.

I agree & I have a prototype, but that requires some work on the child
devices as from what I remember they are not eager to go to runtime
suspend (ie a driver might be holding a reference).

I feel this leans outside the scope of this patch series though.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hello,
>
> On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
>> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> > 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.
>>
>> The point is to signal to the power domain the device is in that it can
>> power on/off.  These IP blocks are (re)used on many different SoCs, so
>> the driver should not make any assumptions about what power domain it is
>> in (if any.)
>
> On my platform, when the device is attached to the PD it gets turned on.
> That feels logical to me: if a driver is not RPM aware it "just works".

It "just works"... until the domain gets turned off.

> Are there platforms where RPM must get enabled for the attached
> power-domains to be turned on?

Yes, but but more importantly, there are platforms where RPM must get
enabled for the power domain to *stay* on.  For example, the power
domain might get turned on due to devices probing etc, but as soon as
all the RPM-enabled drivers drop their refcount, the domain will turn
off.  If there is a device in that domain with a non-RPM enabled driver,
that device will be powered off anc cause a crash.

> The call chain that attaches & enables PD is platform_probe ->
> dev_pm_domain_attach. That function takes a bool power_on which is
> always true. In the genpd case, genpd_dev_pm_attach
> calls __genpd_dev_pm_attach which does a genpd_power_on.
>
> Things I've not accounted for:
>
>  - ACPI looks like it does the same but I've not checked. It gets passed
>    the power_on bool argument.
>
>  - genpd_dev_pm_attach early returns with no error if there are multiple
>    PM domains attached to the device. There are many platforms in the
>    case according to some devicetree grepping. I can imagine a behavior
>    difference where dev_pm_domain callpaths handle that differently in
>    the RPM case. Is that what we are discussing?

You're only looking at the attach, power-on phase.  You need to think
about when the domain powers off and powers back on.

Kevin

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hello,
>
> On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
>> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> > 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.
>>
>> The point is to signal to the power domain the device is in that it can
>> power on/off.  These IP blocks are (re)used on many different SoCs, so
>> the driver should not make any assumptions about what power domain it is
>> in (if any.)
>
> On my platform, when the device is attached to the PD it gets turned on.
> That feels logical to me: if a driver is not RPM aware it "just works".

It "just works"... until the domain gets turned off.

> Are there platforms where RPM must get enabled for the attached
> power-domains to be turned on?

Yes, but but more importantly, there are platforms where RPM must get
enabled for the power domain to *stay* on.  For example, the power
domain might get turned on due to devices probing etc, but as soon as
all the RPM-enabled drivers drop their refcount, the domain will turn
off.  If there is a device in that domain with a non-RPM enabled driver,
that device will be powered off anc cause a crash.

> The call chain that attaches & enables PD is platform_probe ->
> dev_pm_domain_attach. That function takes a bool power_on which is
> always true. In the genpd case, genpd_dev_pm_attach
> calls __genpd_dev_pm_attach which does a genpd_power_on.
>
> Things I've not accounted for:
>
>  - ACPI looks like it does the same but I've not checked. It gets passed
>    the power_on bool argument.
>
>  - genpd_dev_pm_attach early returns with no error if there are multiple
>    PM domains attached to the device. There are many platforms in the
>    case according to some devicetree grepping. I can imagine a behavior
>    difference where dev_pm_domain callpaths handle that differently in
>    the RPM case. Is that what we are discussing?

You're only looking at the attach, power-on phase.  You need to think
about when the domain powers off and powers back on.

Kevin


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hello,

On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> The point is to signal to the power domain the device is in that it can
> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
> >> the driver should not make any assumptions about what power domain it is
> >> in (if any.)
> >
> > On my platform, when the device is attached to the PD it gets turned on.
> > That feels logical to me: if a driver is not RPM aware it "just works".
>
> It "just works"... until the domain gets turned off.
>
> > Are there platforms where RPM must get enabled for the attached
> > power-domains to be turned on?
>
> Yes, but but more importantly, there are platforms where RPM must get
> enabled for the power domain to *stay* on.  For example, the power
> domain might get turned on due to devices probing etc, but as soon as
> all the RPM-enabled drivers drop their refcount, the domain will turn
> off.  If there is a device in that domain with a non-RPM enabled driver,
> that device will be powered off anc cause a crash.

OK, that makes sense, thanks for taking the time to explain. This topic
makes me see two things that I feel are close to being bugs. I'd be
curious to get your view on both.

 - If a device does not use RPM but its children do, it might get its
   associated power-domain turned off. That forces every single driver
   that want to stay alive to enable & increment RPM.

   What I naively expect: a genpd with a device attached to it that is
   not using RPM should mean that it should not be powered off at
   runtime_suspend. Benefit: no RPM calls in drivers that do not use
   it, and the behavior is that the genpd associated stays alive "as
   expected".

 - If a device uses RPM & has a refcount strictly positive, its
   associated power-domain gets turned off either way at suspend_noirq.
   That feels non-intuitive as well.

   What I naively expect: check for RPM refcounts of attached devices
   when doing suspend_noirq of power-domains. Benefit: control of what
   power-domains do from attached devices is done through the RPM API.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Théo Lebrun]

Hello,

On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> The point is to signal to the power domain the device is in that it can
> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
> >> the driver should not make any assumptions about what power domain it is
> >> in (if any.)
> >
> > On my platform, when the device is attached to the PD it gets turned on.
> > That feels logical to me: if a driver is not RPM aware it "just works".
>
> It "just works"... until the domain gets turned off.
>
> > Are there platforms where RPM must get enabled for the attached
> > power-domains to be turned on?
>
> Yes, but but more importantly, there are platforms where RPM must get
> enabled for the power domain to *stay* on.  For example, the power
> domain might get turned on due to devices probing etc, but as soon as
> all the RPM-enabled drivers drop their refcount, the domain will turn
> off.  If there is a device in that domain with a non-RPM enabled driver,
> that device will be powered off anc cause a crash.

OK, that makes sense, thanks for taking the time to explain. This topic
makes me see two things that I feel are close to being bugs. I'd be
curious to get your view on both.

 - If a device does not use RPM but its children do, it might get its
   associated power-domain turned off. That forces every single driver
   that want to stay alive to enable & increment RPM.

   What I naively expect: a genpd with a device attached to it that is
   not using RPM should mean that it should not be powered off at
   runtime_suspend. Benefit: no RPM calls in drivers that do not use
   it, and the behavior is that the genpd associated stays alive "as
   expected".

 - If a device uses RPM & has a refcount strictly positive, its
   associated power-domain gets turned off either way at suspend_noirq.
   That feels non-intuitive as well.

   What I naively expect: check for RPM refcounts of attached devices
   when doing suspend_noirq of power-domains. Benefit: control of what
   power-domains do from attached devices is done through the RPM API.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hello,
>
> On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
>> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
>> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> >> The point is to signal to the power domain the device is in that it can
>> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
>> >> the driver should not make any assumptions about what power domain it is
>> >> in (if any.)
>> >
>> > On my platform, when the device is attached to the PD it gets turned on.
>> > That feels logical to me: if a driver is not RPM aware it "just works".
>>
>> It "just works"... until the domain gets turned off.
>>
>> > Are there platforms where RPM must get enabled for the attached
>> > power-domains to be turned on?
>>
>> Yes, but but more importantly, there are platforms where RPM must get
>> enabled for the power domain to *stay* on.  For example, the power
>> domain might get turned on due to devices probing etc, but as soon as
>> all the RPM-enabled drivers drop their refcount, the domain will turn
>> off.  If there is a device in that domain with a non-RPM enabled driver,
>> that device will be powered off anc cause a crash.
>
> OK, that makes sense, thanks for taking the time to explain. This topic
> makes me see two things that I feel are close to being bugs. I'd be
> curious to get your view on both.

TL;DR; they are features, not bugs.  ;)

>  - If a device does not use RPM but its children do, it might get its
>    associated power-domain turned off. That forces every single driver
>    that want to stay alive to enable & increment RPM.
>
>    What I naively expect: a genpd with a device attached to it that is
>    not using RPM should mean that it should not be powered off at
>    runtime_suspend. Benefit: no RPM calls in drivers that do not use
>    it, and the behavior is that the genpd associated stays alive "as
>    expected".

Your expectation makes sense, but unfortunately, that's not how RPM was
designed.

Also remember that we don't really want specific device drivers to know
which PM domain they are in, or whether they are in a PM domain at
all. The same IP block can be integrated in different ways across
different SoCs, even within the same SoC family, and we want the device
driver to just work.  

For that to work well, any driver that might be in any PM domain should
add RPM calls.

>  - If a device uses RPM & has a refcount strictly positive, its
>    associated power-domain gets turned off either way at suspend_noirq.
>    That feels non-intuitive as well.
>
>    What I naively expect: check for RPM refcounts of attached devices
>    when doing suspend_noirq of power-domains. Benefit: control of what
>    power-domains do from attached devices is done through the RPM API.

I agree that this is non-intuitive from an RPM PoV, but remember that
RPM was added on top of existing system-wide suspend support.  And from
a system-wide suspend PoV, it might be non-intuitive that a driver
thinks it should be active (non-zero refcount) when user just requested
a system-wide suspend.  Traditionally, when a user requests a
system-wide suspend, they expect the whole system to shut down.

On real SoCs in real products, power management is not so black and
white, and I fully understand that, and personally, I'm definitely open
to not forcing RPM-active devices off in suspend, but that would require
changes to core code, and remove some assumptions of core code that
would need to be validated/tested.

Kevin

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Kevin Hilman]

Théo Lebrun <theo.lebrun@bootlin.com> writes:

> Hello,
>
> On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
>> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
>> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
>> >> The point is to signal to the power domain the device is in that it can
>> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
>> >> the driver should not make any assumptions about what power domain it is
>> >> in (if any.)
>> >
>> > On my platform, when the device is attached to the PD it gets turned on.
>> > That feels logical to me: if a driver is not RPM aware it "just works".
>>
>> It "just works"... until the domain gets turned off.
>>
>> > Are there platforms where RPM must get enabled for the attached
>> > power-domains to be turned on?
>>
>> Yes, but but more importantly, there are platforms where RPM must get
>> enabled for the power domain to *stay* on.  For example, the power
>> domain might get turned on due to devices probing etc, but as soon as
>> all the RPM-enabled drivers drop their refcount, the domain will turn
>> off.  If there is a device in that domain with a non-RPM enabled driver,
>> that device will be powered off anc cause a crash.
>
> OK, that makes sense, thanks for taking the time to explain. This topic
> makes me see two things that I feel are close to being bugs. I'd be
> curious to get your view on both.

TL;DR; they are features, not bugs.  ;)

>  - If a device does not use RPM but its children do, it might get its
>    associated power-domain turned off. That forces every single driver
>    that want to stay alive to enable & increment RPM.
>
>    What I naively expect: a genpd with a device attached to it that is
>    not using RPM should mean that it should not be powered off at
>    runtime_suspend. Benefit: no RPM calls in drivers that do not use
>    it, and the behavior is that the genpd associated stays alive "as
>    expected".

Your expectation makes sense, but unfortunately, that's not how RPM was
designed.

Also remember that we don't really want specific device drivers to know
which PM domain they are in, or whether they are in a PM domain at
all. The same IP block can be integrated in different ways across
different SoCs, even within the same SoC family, and we want the device
driver to just work.  

For that to work well, any driver that might be in any PM domain should
add RPM calls.

>  - If a device uses RPM & has a refcount strictly positive, its
>    associated power-domain gets turned off either way at suspend_noirq.
>    That feels non-intuitive as well.
>
>    What I naively expect: check for RPM refcounts of attached devices
>    when doing suspend_noirq of power-domains. Benefit: control of what
>    power-domains do from attached devices is done through the RPM API.

I agree that this is non-intuitive from an RPM PoV, but remember that
RPM was added on top of existing system-wide suspend support.  And from
a system-wide suspend PoV, it might be non-intuitive that a driver
thinks it should be active (non-zero refcount) when user just requested
a system-wide suspend.  Traditionally, when a user requests a
system-wide suspend, they expect the whole system to shut down.

On real SoCs in real products, power management is not so black and
white, and I fully understand that, and personally, I'm definitely open
to not forcing RPM-active devices off in suspend, but that would require
changes to core code, and remove some assumptions of core code that
would need to be validated/tested.

Kevin

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Alan Stern]

On Tue, Dec 12, 2023 at 10:26:05AM -0800, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:

> 
> > Hello,
> >
> > On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> >> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> >> The point is to signal to the power domain the device is in that it can
> >> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
> >> >> the driver should not make any assumptions about what power domain it is
> >> >> in (if any.)
> >> >
> >> > On my platform, when the device is attached to the PD it gets turned on.
> >> > That feels logical to me: if a driver is not RPM aware it "just works".
> >>
> >> It "just works"... until the domain gets turned off.
> >>
> >> > Are there platforms where RPM must get enabled for the attached
> >> > power-domains to be turned on?
> >>
> >> Yes, but but more importantly, there are platforms where RPM must get
> >> enabled for the power domain to *stay* on.  For example, the power
> >> domain might get turned on due to devices probing etc, but as soon as
> >> all the RPM-enabled drivers drop their refcount, the domain will turn
> >> off.  If there is a device in that domain with a non-RPM enabled driver,
> >> that device will be powered off anc cause a crash.
> >
> > OK, that makes sense, thanks for taking the time to explain. This topic
> > makes me see two things that I feel are close to being bugs. I'd be
> > curious to get your view on both.
> 
> TL;DR; they are features, not bugs.  ;)
> 
> >  - If a device does not use RPM but its children do, it might get its
> >    associated power-domain turned off. That forces every single driver
> >    that want to stay alive to enable & increment RPM.
> >
> >    What I naively expect: a genpd with a device attached to it that is
> >    not using RPM should mean that it should not be powered off at
> >    runtime_suspend. Benefit: no RPM calls in drivers that do not use
> >    it, and the behavior is that the genpd associated stays alive "as
> >    expected".
> 
> Your expectation makes sense, but unfortunately, that's not how RPM was
> designed.

I'm not sure how runtime PM is meant to work with power domains.  

However, from the very beginning of runtime PM the intention was that 
device drivers and subsystems could safely ignore it.  Their devices 
would have a permanently nonzero disable_depth (permanent because the 
driver/subsystem would not know to change it) and therefore would always 
remain in the active state (see rpm_check_suspend_allowed()).

It would be very surprising if runtime PM for power domains was written 
in a way that would subvert this intention.

Alan Stern

Re: [PATCH 3/6] usb: cdns3-ti: add suspend/resume procedures for J7200

[Alan Stern]

On Tue, Dec 12, 2023 at 10:26:05AM -0800, Kevin Hilman wrote:
> Théo Lebrun <theo.lebrun@bootlin.com> writes:

> 
> > Hello,
> >
> > On Sun Nov 26, 2023 at 11:36 PM CET, Kevin Hilman wrote:
> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> > On Wed Nov 22, 2023 at 11:23 PM CET, Kevin Hilman wrote:
> >> >> Théo Lebrun <theo.lebrun@bootlin.com> writes:
> >> >> The point is to signal to the power domain the device is in that it can
> >> >> power on/off.  These IP blocks are (re)used on many different SoCs, so
> >> >> the driver should not make any assumptions about what power domain it is
> >> >> in (if any.)
> >> >
> >> > On my platform, when the device is attached to the PD it gets turned on.
> >> > That feels logical to me: if a driver is not RPM aware it "just works".
> >>
> >> It "just works"... until the domain gets turned off.
> >>
> >> > Are there platforms where RPM must get enabled for the attached
> >> > power-domains to be turned on?
> >>
> >> Yes, but but more importantly, there are platforms where RPM must get
> >> enabled for the power domain to *stay* on.  For example, the power
> >> domain might get turned on due to devices probing etc, but as soon as
> >> all the RPM-enabled drivers drop their refcount, the domain will turn
> >> off.  If there is a device in that domain with a non-RPM enabled driver,
> >> that device will be powered off anc cause a crash.
> >
> > OK, that makes sense, thanks for taking the time to explain. This topic
> > makes me see two things that I feel are close to being bugs. I'd be
> > curious to get your view on both.
> 
> TL;DR; they are features, not bugs.  ;)
> 
> >  - If a device does not use RPM but its children do, it might get its
> >    associated power-domain turned off. That forces every single driver
> >    that want to stay alive to enable & increment RPM.
> >
> >    What I naively expect: a genpd with a device attached to it that is
> >    not using RPM should mean that it should not be powered off at
> >    runtime_suspend. Benefit: no RPM calls in drivers that do not use
> >    it, and the behavior is that the genpd associated stays alive "as
> >    expected".
> 
> Your expectation makes sense, but unfortunately, that's not how RPM was
> designed.

I'm not sure how runtime PM is meant to work with power domains.  

However, from the very beginning of runtime PM the intention was that 
device drivers and subsystems could safely ignore it.  Their devices 
would have a permanently nonzero disable_depth (permanent because the 
driver/subsystem would not know to change it) and therefore would always 
remain in the active state (see rpm_check_suspend_allowed()).

It would be very surprising if runtime PM for power domains was written 
in a way that would subvert this intention.

Alan Stern

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel