Hi,
Suspend on the TI J7200 platform is broken currently. There are two
components that need to be patched so that they assume reset on
resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the
controller.
About (1): the TI wrapper only did its hardware configuration at probe
time. Update so that it is done at resume on J7200 SoC.
About (2): signal from cdns3-ti to cdns3 host to xHCI that the
controller resets on resume. This way we avoid the following warning:
xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
Strictly speaking (2) is not required to have working suspend on J7200;
its only goal is to silence this warning.
Those patches have been tested on the TI J7200 EVM GP. No need to
mention that other patches are required for S2R to work, but those will
be sent later down the road. Those USB patches are rather standalone.
Thanks,
Théo
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Signed-off-by: Théo Lebrun <[email protected]>
---
Changes in v2:
- Remove runtime PM from cdns3-ti; it brings nothing. That means our
cdns3-ti suspend/resume patch is simpler; there is no need to handle
runtime PM at suspend/resume.
- Do not add cdns3 host role suspend/resume callbacks; they are not
needed as core detects reset on resume & calls cdns_drd_host_on when
needed.
- cdns3-ti: Move usb2_refclk_rate_code assignment closer to the value
computation.
- cdns3/host.c: do not pass XHCI_SUSPEND_RESUME_CLKS quirk to xHCI; it
is unneeded on our platform.
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Théo Lebrun (7):
dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
usb: cdns3-ti: remove runtime PM
usb: cdns3-ti: move reg writes from probe into an init_hw helper
usb: cdns3-ti: add suspend/resume procedures for J7200
usb: cdns3: add quirk to platform data for reset-on-resume
usb: cdns3-ti: signal reset-on-resume to xHCI for J7200 platform
arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
.../devicetree/bindings/usb/ti,j721e-usb.yaml | 4 +
arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +-
drivers/usb/cdns3/cdns3-ti.c | 134 +++++++++++++--------
drivers/usb/cdns3/core.h | 1 +
drivers/usb/cdns3/host.c | 3 +
5 files changed, 90 insertions(+), 54 deletions(-)
---
base-commit: 1d42d5c8f1ca11106579dcaadef4161fee03419e
change-id: 20231113-j7200-usb-suspend-2a47f2281e04
Best regards,
--
Théo Lebrun <[email protected]>
On this platform, the controller & its wrapper are reset on resume. This
makes it have a different behavior from other platforms.
We allow using the new compatible with a fallback onto the original
ti,j721e-usb compatible. We therefore allow using an older kernel with
a more recent devicetree.
Acked-by: Conor Dooley <[email protected]>
Signed-off-by: Théo Lebrun <[email protected]>
---
Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..69a222dfd9ff 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -12,11 +12,15 @@ maintainers:
properties:
compatible:
oneOf:
+ - const: ti,j7200-usb
- const: ti,j721e-usb
- const: ti,am64-usb
- items:
- const: ti,j721e-usb
- const: ti,am64-usb
+ - items:
+ - const: ti,j721e-usb
+ - const: ti,j7200-usb
reg:
maxItems: 1
--
2.42.0
The driver does not use RPM. It enables it & gets a reference at probe.
It then undoes that on probe error or at remove.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 5945c4b1e11f..dc1594acdcee 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -135,13 +135,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
rate_code = i;
- pm_runtime_enable(dev);
- error = pm_runtime_get_sync(dev);
- if (error < 0) {
- dev_err(dev, "pm_runtime_get_sync failed: %d\n", error);
- goto err;
- }
-
/* assert RESET */
reg = cdns_ti_readl(data, USBSS_W1);
reg &= ~USBSS_W1_PWRUP_RST;
@@ -179,16 +172,10 @@ static int cdns_ti_probe(struct platform_device *pdev)
error = of_platform_populate(node, NULL, NULL, dev);
if (error) {
dev_err(dev, "failed to create children: %d\n", error);
- goto err;
+ return error;
}
return 0;
-
-err:
- pm_runtime_put_sync(data->dev);
- pm_runtime_disable(data->dev);
-
- return error;
}
static int cdns_ti_remove_core(struct device *dev, void *c)
@@ -205,8 +192,6 @@ static void cdns_ti_remove(struct platform_device *pdev)
struct device *dev = &pdev->dev;
device_for_each_child(dev, NULL, cdns_ti_remove_core);
- pm_runtime_put_sync(dev);
- pm_runtime_disable(dev);
platform_set_drvdata(pdev, NULL);
}
--
2.42.0
The cdns3 host role does not care about reset-on-resume. xHCI however
reconfigures itself in silence rather than printing a warning about a
resume error. Related warning example:
[ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
Allow passing a CDNS3_RESET_ON_RESUME quirk flag from cdns3 pdata down
to xHCI pdata. The goal is to allow signaling about reset-on-resume
behavior from platform wrapper drivers.
When used, remote wakeup is not expected to work.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/core.h | 1 +
drivers/usb/cdns3/host.c | 3 +++
2 files changed, 4 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..28c4d1deb231 100644
--- a/drivers/usb/cdns3/host.c
+++ b/drivers/usb/cdns3/host.c
@@ -91,6 +91,9 @@ static int __cdns_host_init(struct cdns *cdns)
if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
+ if (cdns->pdata && (cdns->pdata->quirks & CDNS3_RESET_ON_RESUME))
+ cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME;
+
ret = platform_device_add_data(xhci, cdns->xhci_plat_data,
sizeof(struct xhci_plat_priv));
if (ret)
--
2.42.0
Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.
Avoid this warning on resume:
[ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
When used, remote wakeup is not expected to work.
Only focus J7200 as other SoC are untested.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++--
1 file changed, 17 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 84f93c2fcd5c..7d56a1acbc54 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -16,6 +16,7 @@
#include <linux/of_platform.h>
#include <linux/pm_runtime.h>
#include <linux/property.h>
+#include "core.h"
/* USB Wrapper register offsets */
#define USBSS_PID 0x0
@@ -128,6 +129,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
struct device_node *node = pdev->dev.of_node;
+ const struct of_dev_auxdata *auxdata;
struct cdns_ti *data;
unsigned long rate;
int error, i;
@@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
cdns_ti_init_hw(data);
- error = of_platform_populate(node, NULL, NULL, dev);
+ auxdata = of_device_get_match_data(dev);
+ error = of_platform_populate(node, NULL, auxdata, dev);
if (error) {
dev_err(dev, "failed to create children: %d\n", error);
return error;
@@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
#endif /* CONFIG_PM */
+static struct cdns3_platform_data cdns_ti_j7200_pdata = {
+ .quirks = CDNS3_RESET_ON_RESUME,
+};
+
+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.42.0
On our platform, suspend-to-idle or suspend-to-RAM turn the controller
off. This compatible triggers reset on resume behavior to reconfigure
the hardware.
Signed-off-by: Théo Lebrun <[email protected]>
---
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..52c4ee0fa334 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", "ti,j721e-usb";
reg = <0x00 0x4104000 0x00 0x100>;
dma-coherent;
power-domains = <&k3_pds 288 TI_SCI_PD_EXCLUSIVE>;
--
2.42.0
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 <[email protected]>
---
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 dc1594acdcee..d4232b440e4e 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,41 +169,11 @@ static int cdns_ti_probe(struct platform_device *pdev)
return -EINVAL;
}
- rate_code = i;
-
- /* 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->usb2_refclk_rate_code = i;
data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
- if (data->vbus_divider)
- reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
-
- cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
- reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-
- /* set USB2_ONLY mode if requested */
- reg = cdns_ti_readl(data, USBSS_W1);
data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
- if (data->usb2_only)
- reg |= USBSS_W1_USB2_ONLY;
-
- /* set default modestrap */
- reg |= USBSS_W1_MODESTRAP_SEL;
- reg &= ~USBSS_W1_MODESTRAP_MASK;
- reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
- cdns_ti_writel(data, USBSS_W1, reg);
- /* de-assert RESET */
- reg |= USBSS_W1_PWRUP_RST;
- cdns_ti_writel(data, USBSS_W1, reg);
+ cdns_ti_init_hw(data);
error = of_platform_populate(node, NULL, NULL, dev);
if (error) {
--
2.42.0
Hardware initialisation is only done at probe. The J7200 USB controller
is reset at resume because of power-domains toggling off & on. We
therefore reconfigure the hardware at resume.
Reuse the newly extracted cdns_ti_init_hw() function that contains the
register write sequence.
Only focus J7200 as other SoC are untested. If the controller does not
reset we do not want to redo reg writes.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 24 +++++++++++++++++++++++-
1 file changed, 23 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index d4232b440e4e..84f93c2fcd5c 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,7 @@ struct cdns_ti {
struct clk *usb2_refclk;
struct clk *lpm_clk;
int usb2_refclk_rate_code;
+ bool reset_on_resume;
};
static const int cdns_ti_rate_table[] = { /* in KHZ */
@@ -172,6 +173,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
data->usb2_refclk_rate_code = i;
data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
+ data->reset_on_resume = of_device_is_compatible(node, "ti,j7200-usb");
cdns_ti_init_hw(data);
@@ -202,7 +204,26 @@ static void cdns_ti_remove(struct platform_device *pdev)
platform_set_drvdata(pdev, NULL);
}
+#ifdef CONFIG_PM
+
+static int cdns_ti_resume(struct device *dev)
+{
+ struct cdns_ti *data = dev_get_drvdata(dev);
+
+ if (data->reset_on_resume)
+ cdns_ti_init_hw(data);
+
+ return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(NULL, cdns_ti_resume)
+};
+
+#endif /* CONFIG_PM */
+
static const struct of_device_id cdns_ti_of_match[] = {
+ { .compatible = "ti,j7200-usb", },
{ .compatible = "ti,j721e-usb", },
{ .compatible = "ti,am64-usb", },
{},
@@ -213,8 +234,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.42.0
On 20/11/2023 18:06, 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 reconfigure the hardware at resume.
>
> Reuse the newly extracted cdns_ti_init_hw() function that contains the
> register write sequence.
>
> Only focus J7200 as other SoC are untested. If the controller does not
> reset we do not want to redo reg writes.
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> drivers/usb/cdns3/cdns3-ti.c | 24 +++++++++++++++++++++++-
> 1 file changed, 23 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index d4232b440e4e..84f93c2fcd5c 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -58,6 +58,7 @@ struct cdns_ti {
> struct clk *usb2_refclk;
> struct clk *lpm_clk;
> int usb2_refclk_rate_code;
> + bool reset_on_resume;
> };
>
> static const int cdns_ti_rate_table[] = { /* in KHZ */
> @@ -172,6 +173,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
> data->usb2_refclk_rate_code = i;
> data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> + data->reset_on_resume = of_device_is_compatible(node, "ti,j7200-usb");
No, use driver data for this. Don't put compatibles in the code. It
makes it unmanageable soon.
I am repeating this issue from time to time :/
Best regards,
Krzysztof
On 20/11/2023 18:06, Théo Lebrun wrote:
> On this platform, the controller & its wrapper are reset on resume. This
> makes it have a different behavior from other platforms.
>
> We allow using the new compatible with a fallback onto the original
> ti,j721e-usb compatible. We therefore allow using an older kernel with
Where is fallback ti,j721e-usb used? Please point me to the code.
> a more recent devicetree.
>
> Acked-by: Conor Dooley <[email protected]>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> index 95ff9791baea..69a222dfd9ff 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -12,11 +12,15 @@ maintainers:
> properties:
> compatible:
> oneOf:
> + - const: ti,j7200-usb
> - const: ti,j721e-usb
> - const: ti,am64-usb
> - items:
> - const: ti,j721e-usb
> - const: ti,am64-usb
> + - items:
> + - const: ti,j721e-usb
This makes little sense. It's already on the list. Twice! Don't add it
third time.
I am sorry, but this binding makes no sense. I mean, existing binding
makes no sense, but your change is not making it anyhow better.
Best regards,
Krzysztof
On 20/11/2023 18:06, Théo Lebrun wrote:
> On our platform, suspend-to-idle or suspend-to-RAM turn the controller
> off. This compatible triggers reset on resume behavior to reconfigure
> the hardware.
>
> Signed-off-by: Théo Lebrun <[email protected]>
Fix your addressee list...
> ---
> 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..52c4ee0fa334 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", "ti,j721e-usb";
It does not match your bindings. Bindings say something entirely else.
Best regards,
Krzysztof
On 20/11/2023 18:06, Théo Lebrun wrote:
> Hi,
>
> Suspend on the TI J7200 platform is broken currently. There are two
> components that need to be patched so that they assume reset on
> resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the
> controller.
>
> About (1): the TI wrapper only did its hardware configuration at probe
> time. Update so that it is done at resume on J7200 SoC.
>
> About (2): signal from cdns3-ti to cdns3 host to xHCI that the
> controller resets on resume. This way we avoid the following warning:
>
> xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
>
> Strictly speaking (2) is not required to have working suspend on J7200;
> its only goal is to silence this warning.
>
> Those patches have been tested on the TI J7200 EVM GP. No need to
Yeah, but you forgot about dtbs_check testing. It's a step before
testing on the device. It does not make sense to test a code if it
causes build-time warnings.
Best regards,
Krzysztof
On 23-11-20 18:06:05, Th?o Lebrun wrote:
> The cdns3 host role does not care about reset-on-resume. xHCI however
> reconfigures itself in silence rather than printing a warning about a
> resume error. Related warning example:
>
> [ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
>
> Allow passing a CDNS3_RESET_ON_RESUME quirk flag from cdns3 pdata down
> to xHCI pdata. The goal is to allow signaling about reset-on-resume
> behavior from platform wrapper drivers.
>
> When used, remote wakeup is not expected to work.
>
> Signed-off-by: Th?o Lebrun <[email protected]>
Acked-by: Peter Chen <[email protected]>
> ---
> drivers/usb/cdns3/core.h | 1 +
> drivers/usb/cdns3/host.c | 3 +++
> 2 files changed, 4 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..28c4d1deb231 100644
> --- a/drivers/usb/cdns3/host.c
> +++ b/drivers/usb/cdns3/host.c
> @@ -91,6 +91,9 @@ static int __cdns_host_init(struct cdns *cdns)
> if (cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW))
> cdns->xhci_plat_data->quirks |= XHCI_DEFAULT_PM_RUNTIME_ALLOW;
>
> + if (cdns->pdata && (cdns->pdata->quirks & CDNS3_RESET_ON_RESUME))
> + cdns->xhci_plat_data->quirks |= XHCI_RESET_ON_RESUME;
> +
> ret = platform_device_add_data(xhci, cdns->xhci_plat_data,
> sizeof(struct xhci_plat_priv));
> if (ret)
>
> --
> 2.42.0
>
--
Thanks,
Peter Chen
Hello Krzysztof,
On Mon Nov 20, 2023 at 6:31 PM CET, Krzysztof Kozlowski wrote:
> On 20/11/2023 18:06, Théo Lebrun wrote:
[...]
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > static const int cdns_ti_rate_table[] = { /* in KHZ */
> > @@ -172,6 +173,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
> > data->usb2_refclk_rate_code = i;
> > data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
> > data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
> > + data->reset_on_resume = of_device_is_compatible(node, "ti,j7200-usb");
>
> No, use driver data for this. Don't put compatibles in the code. It
> makes it unmanageable soon.
Fixed for next revision. I hesitated on this patch but I'll know for
next time.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 20/11/2023 19:06, Théo Lebrun wrote:
> Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
> in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.
>
> Avoid this warning on resume:
>
> [ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
>
> When used, remote wakeup is not expected to work.
>
> Only focus J7200 as other SoC are untested.
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++--
> 1 file changed, 17 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 84f93c2fcd5c..7d56a1acbc54 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -16,6 +16,7 @@
> #include <linux/of_platform.h>
> #include <linux/pm_runtime.h>
> #include <linux/property.h>
> +#include "core.h"
>
> /* USB Wrapper register offsets */
> #define USBSS_PID 0x0
> @@ -128,6 +129,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct device_node *node = pdev->dev.of_node;
> + const struct of_dev_auxdata *auxdata;
> struct cdns_ti *data;
> unsigned long rate;
> int error, i;
> @@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
>
> cdns_ti_init_hw(data);
>
> - error = of_platform_populate(node, NULL, NULL, dev);
> + auxdata = of_device_get_match_data(dev);
> + error = of_platform_populate(node, NULL, auxdata, dev);
> if (error) {
> dev_err(dev, "failed to create children: %d\n", error);
> return error;
> @@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
>
> #endif /* CONFIG_PM */
>
> +static struct cdns3_platform_data cdns_ti_j7200_pdata = {
> + .quirks = CDNS3_RESET_ON_RESUME,
> +};
We will need to introduce a new data structure "struct cdns_ti_platform_data"
and add platform specific details like "reset_on_resume" to it.
This is to address what Krzysztof pointed to in patch 4.
> +
> +static const struct of_dev_auxdata cdns_ti_j7200_auxdata[] = {
> + {
> + .compatible = "cdns,usb3",
> + .platform_data = &cdns_ti_j7200_pdata,
> + },
> + {},
> +};
> +
> static const struct of_device_id cdns_ti_of_match[] = {
> - { .compatible = "ti,j7200-usb", },
> + { .compatible = "ti,j7200-usb", .data = cdns_ti_j7200_auxdata, },
Here we should pass "struct cdns_ti_platform_data"
> { .compatible = "ti,j721e-usb", },
> { .compatible = "ti,am64-usb", },
> {},
>
--
cheers,
-roger
Hello,
On Mon Nov 20, 2023 at 6:32 PM CET, Krzysztof Kozlowski wrote:
> On 20/11/2023 18:06, Théo Lebrun wrote:
> > On this platform, the controller & its wrapper are reset on resume. This
> > makes it have a different behavior from other platforms.
> >
> > We allow using the new compatible with a fallback onto the original
> > ti,j721e-usb compatible. We therefore allow using an older kernel with
>
> Where is fallback ti,j721e-usb used? Please point me to the code.
No fallback is implemented in code. Using a kernel that doesn't have
this patch series but a more recent devicetree: DT has both
devicetrees & the kernel will know which driver to use.
That is opposed to having only compatible = "ti,j7200-usb". If using an
old kernel, it would not know what driver to match it to.
[...]
> > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > @@ -12,11 +12,15 @@ maintainers:
> > properties:
> > compatible:
> > oneOf:
> > + - const: ti,j7200-usb
> > - const: ti,j721e-usb
> > - const: ti,am64-usb
> > - items:
> > - const: ti,j721e-usb
> > - const: ti,am64-usb
> > + - items:
> > + - const: ti,j721e-usb
>
> This makes little sense. It's already on the list. Twice! Don't add it
> third time.
>
> I am sorry, but this binding makes no sense. I mean, existing binding
> makes no sense, but your change is not making it anyhow better.
The goal of the DT schema pre-patch was to allow all three:
compatible = "ti,j721e-usb";
compatible = "ti,am64-usb";
compatible = "ti,j721e-usb", "ti,am64-usb";
I've followed the same scheme & added both of those:
compatible = "ti,j7200-usb";
compatible = "ti,j7200-usb", "ti,j721e-usb";
I messed up the ordering in the added 'items' options, but the logic
seems right to me. And dtbs_check agrees. Am I missing something?
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello,
On Tue Nov 21, 2023 at 5:53 PM CET, Roger Quadros wrote:
> On 20/11/2023 19:06, Théo Lebrun wrote:
> > Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
> > in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.
> >
> > Avoid this warning on resume:
> >
> > [ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
> >
> > When used, remote wakeup is not expected to work.
> >
> > Only focus J7200 as other SoC are untested.
> >
> > Signed-off-by: Théo Lebrun <[email protected]>
> > ---
> > drivers/usb/cdns3/cdns3-ti.c | 19 +++++++++++++++++--
> > 1 file changed, 17 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index 84f93c2fcd5c..7d56a1acbc54 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> > @@ -16,6 +16,7 @@
> > #include <linux/of_platform.h>
> > #include <linux/pm_runtime.h>
> > #include <linux/property.h>
> > +#include "core.h"
> >
> > /* USB Wrapper register offsets */
> > #define USBSS_PID 0x0
> > @@ -128,6 +129,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
> > {
> > struct device *dev = &pdev->dev;
> > struct device_node *node = pdev->dev.of_node;
> > + const struct of_dev_auxdata *auxdata;
> > struct cdns_ti *data;
> > unsigned long rate;
> > int error, i;
> > @@ -177,7 +179,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
> >
> > cdns_ti_init_hw(data);
> >
> > - error = of_platform_populate(node, NULL, NULL, dev);
> > + auxdata = of_device_get_match_data(dev);
> > + error = of_platform_populate(node, NULL, auxdata, dev);
> > if (error) {
> > dev_err(dev, "failed to create children: %d\n", error);
> > return error;
> > @@ -222,8 +225,20 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
> >
> > #endif /* CONFIG_PM */
> >
> > +static struct cdns3_platform_data cdns_ti_j7200_pdata = {
> > + .quirks = CDNS3_RESET_ON_RESUME,
> > +};
>
> We will need to introduce a new data structure "struct cdns_ti_platform_data"
> and add platform specific details like "reset_on_resume" to it.
> This is to address what Krzysztof pointed to in patch 4.
Yes I've got it locally following Krzysztof's review. Below my signature
is a sneak peak as I'll wait a bit more before a V3. First we implement
resume behavior in the wrapper driver using match data then we add
auxdata passed to the subdevices.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
------------------------------------------------------------------------
From 69a59e3408668dfa06d3790cb20948961708791d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20Lebrun?= <[email protected]>
Date: Mon, 20 Nov 2023 16:47:29 +0100
Subject: [PATCH 05/13] usb: cdns3-ti: add suspend/resume procedures for J7200
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Hardware initialisation is only done at probe. The J7200 USB controller
is reset at resume because of power-domains toggling off & on. We
therefore reconfigure the hardware at resume.
Reuse the newly extracted cdns_ti_init_hw() function that contains the
register write sequence.
Only focus J7200 as other SoC are untested. If the controller does not
reset we do not want to redo reg writes.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 32 +++++++++++++++++++++++++++++++-
1 file changed, 31 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index d4232b440e4e..7530b6b5159d 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -58,6 +58,11 @@ struct cdns_ti {
struct clk *usb2_refclk;
struct clk *lpm_clk;
int usb2_refclk_rate_code;
+ const struct cdns_ti_match_data *match_data;
+};
+
+struct cdns_ti_match_data {
+ bool reset_on_resume;
};
static const int cdns_ti_rate_table[] = { /* in KHZ */
@@ -138,6 +143,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
data->dev = dev;
+ data->match_data = of_device_get_match_data(dev);
data->usbss = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(data->usbss)) {
@@ -202,7 +208,30 @@ static void cdns_ti_remove(struct platform_device *pdev)
platform_set_drvdata(pdev, NULL);
}
+#ifdef CONFIG_PM
+
+static int cdns_ti_resume(struct device *dev)
+{
+ struct cdns_ti *data = dev_get_drvdata(dev);
+
+ if (data->match_data && data->match_data->reset_on_resume)
+ cdns_ti_init_hw(data);
+
+ return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(NULL, cdns_ti_resume)
+};
+
+#endif /* CONFIG_PM */
+
+static const struct cdns_ti_match_data cdns_ti_j7200_match_data = {
+ .reset_on_resume = true,
+};
+
static const struct of_device_id cdns_ti_of_match[] = {
+ { .compatible = "ti,j7200-usb", .data = &cdns_ti_j7200_match_data, },
{ .compatible = "ti,j721e-usb", },
{ .compatible = "ti,am64-usb", },
{},
@@ -213,8 +242,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.42.0
------------------------------------------------------------------------
From 4ff91a036da297e9e8585980c6133bee9c45d9a6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Th=C3=A9o=20Lebrun?= <[email protected]>
Date: Mon, 20 Nov 2023 17:02:44 +0100
Subject: [PATCH 07/13] usb: cdns3-ti: signal reset-on-resume to xHCI for J7200
platform
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Pass CDNS3_RESET_ON_RESUME as platform data to cdns3 host role. It will
in turn pass it down to xHCI platform data as XHCI_RESET_ON_RESUME.
Avoid this warning on resume:
[ 16.017462] xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
When used, remote wakeup is not expected to work.
Only focus J7200 as other SoC are untested.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 22 ++++++++++++++++++++--
1 file changed, 20 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 7530b6b5159d..da2648ebc179 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
@@ -62,7 +63,8 @@ struct cdns_ti {
};
struct cdns_ti_match_data {
- bool reset_on_resume;
+ bool reset_on_resume;
+ const struct of_dev_auxdata *auxdata;
};
static const int cdns_ti_rate_table[] = { /* in KHZ */
@@ -132,6 +134,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 = NULL;
struct cdns_ti *data;
unsigned long rate;
int error, i;
@@ -181,7 +184,9 @@ static int cdns_ti_probe(struct platform_device *pdev)
cdns_ti_init_hw(data);
- error = of_platform_populate(node, NULL, NULL, dev);
+ if (data->match_data)
+ auxdata = data->match_data->auxdata;
+ error = of_platform_populate(node, NULL, auxdata, dev);
if (error) {
dev_err(dev, "failed to create children: %d\n", error);
return error;
@@ -226,8 +231,21 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
#endif /* CONFIG_PM */
+static struct cdns3_platform_data cdns_ti_j7200_pdata = {
+ .quirks = CDNS3_RESET_ON_RESUME,
+};
+
+static const struct of_dev_auxdata cdns_ti_j7200_auxdata[] = {
+ {
+ .compatible = "cdns,usb3",
+ .platform_data = &cdns_ti_j7200_pdata,
+ },
+ {},
+};
+
static const struct cdns_ti_match_data cdns_ti_j7200_match_data = {
.reset_on_resume = true,
+ .auxdata = cdns_ti_j7200_auxdata,
};
static const struct of_device_id cdns_ti_of_match[] = {
--
2.42.0
On 21/11/2023 17:53, Théo Lebrun wrote:
> Hello,
>
> On Mon Nov 20, 2023 at 6:32 PM CET, Krzysztof Kozlowski wrote:
>> On 20/11/2023 18:06, Théo Lebrun wrote:
>>> On this platform, the controller & its wrapper are reset on resume. This
>>> makes it have a different behavior from other platforms.
>>>
>>> We allow using the new compatible with a fallback onto the original
>>> ti,j721e-usb compatible. We therefore allow using an older kernel with
>>
>> Where is fallback ti,j721e-usb used? Please point me to the code.
>
> No fallback is implemented in code. Using a kernel that doesn't have
> this patch series but a more recent devicetree: DT has both
> devicetrees & the kernel will know which driver to use.
I meant your bindings. You said - with fallback to ti,j721e-usb. I do
not see it. To me the commit description is not accurate.
>
> That is opposed to having only compatible = "ti,j7200-usb". If using an
> old kernel, it would not know what driver to match it to.
>
> [...]
>
>>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>> @@ -12,11 +12,15 @@ maintainers:
>>> properties:
>>> compatible:
>>> oneOf:
>>> + - const: ti,j7200-usb
>>> - const: ti,j721e-usb
>>> - const: ti,am64-usb
>>> - items:
>>> - const: ti,j721e-usb
>>> - const: ti,am64-usb
>>> + - items:
>>> + - const: ti,j721e-usb
>>
>> This makes little sense. It's already on the list. Twice! Don't add it
>> third time.
>>
>> I am sorry, but this binding makes no sense. I mean, existing binding
>> makes no sense, but your change is not making it anyhow better.
>
> The goal of the DT schema pre-patch was to allow all three:
>
> compatible = "ti,j721e-usb";
> compatible = "ti,am64-usb";
> compatible = "ti,j721e-usb", "ti,am64-usb";
Which does not make sense.
How ti,j721e-usb can be and cannot be compatible with am64 in the same time?
>
> I've followed the same scheme & added both of those:
>
> compatible = "ti,j7200-usb";
> compatible = "ti,j7200-usb", "ti,j721e-usb";
>
> I messed up the ordering in the added 'items' options, but the logic
> seems right to me. And dtbs_check agrees. Am I missing something?
>
Logic is wrong. Device either is or is not compatible with something. At
least usually. We have some exceptions like SMMU for Adreno. Is this the
case? Why the device is and is not compatible with some other variant?
Best regards,
Krzysztof
Hello,
On Tue Nov 21, 2023 at 6:11 PM CET, Krzysztof Kozlowski wrote:
> On 21/11/2023 17:53, Théo Lebrun wrote:
> > On Mon Nov 20, 2023 at 6:32 PM CET, Krzysztof Kozlowski wrote:
> >> On 20/11/2023 18:06, Théo Lebrun wrote:
> >>> On this platform, the controller & its wrapper are reset on resume. This
> >>> makes it have a different behavior from other platforms.
> >>>
> >>> We allow using the new compatible with a fallback onto the original
> >>> ti,j721e-usb compatible. We therefore allow using an older kernel with
> >>
> >> Where is fallback ti,j721e-usb used? Please point me to the code.
> >
> > No fallback is implemented in code. Using a kernel that doesn't have
> > this patch series but a more recent devicetree: DT has both
> > devicetrees & the kernel will know which driver to use.
>
> I meant your bindings. You said - with fallback to ti,j721e-usb. I do
> not see it. To me the commit description is not accurate.
I see your point, I'll remove that aspect.
> > That is opposed to having only compatible = "ti,j7200-usb". If using an
> > old kernel, it would not know what driver to match it to.
> >
> > [...]
> >
> >>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>> @@ -12,11 +12,15 @@ maintainers:
> >>> properties:
> >>> compatible:
> >>> oneOf:
> >>> + - const: ti,j7200-usb
> >>> - const: ti,j721e-usb
> >>> - const: ti,am64-usb
> >>> - items:
> >>> - const: ti,j721e-usb
> >>> - const: ti,am64-usb
> >>> + - items:
> >>> + - const: ti,j721e-usb
> >>
> >> This makes little sense. It's already on the list. Twice! Don't add it
> >> third time.
> >>
> >> I am sorry, but this binding makes no sense. I mean, existing binding
> >> makes no sense, but your change is not making it anyhow better.
> >
> > The goal of the DT schema pre-patch was to allow all three:
> >
> > compatible = "ti,j721e-usb";
> > compatible = "ti,am64-usb";
> > compatible = "ti,j721e-usb", "ti,am64-usb";
>
> Which does not make sense.
>
> How ti,j721e-usb can be and cannot be compatible with am64 in the same time?
The code tells us that there is no difference between ti,j721e-usb &
ti,am64-usb. And the commit adding the of_device_id entry agrees, see
4f30b9d2315f (usb: cdns3: Add support for TI's AM64 SoC, 2021-01-19).
Here is the entire patch because it is so small:
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 90e246601537..eccb1c766bba 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -214,6 +214,7 @@ static int cdns_ti_remove(struct platform_device *pdev)
static const struct of_device_id cdns_ti_of_match[] = {
{ .compatible = "ti,j721e-usb", },
+ { .compatible = "ti,am64-usb", },
{},
};
MODULE_DEVICE_TABLE(of, cdns_ti_of_match);
> > I've followed the same scheme & added both of those:
> >
> > compatible = "ti,j7200-usb";
> > compatible = "ti,j7200-usb", "ti,j721e-usb";
> >
> > I messed up the ordering in the added 'items' options, but the logic
> > seems right to me. And dtbs_check agrees. Am I missing something?
>
> Logic is wrong. Device either is or is not compatible with something. At
> least usually. We have some exceptions like SMMU for Adreno. Is this the
> case? Why the device is and is not compatible with some other variant?
My understanding is this: j721e & am64 are strictly equivalent. On our
j7200 we know we reset on resume. I see three ways forward:
- properties:
compatible:
oneOf:
- const: ti,j7200-usb
- const: ti,j721e-usb
- const: ti,am64-usb
We keep both ti,j721e-usb & ti,am64-usb separate even though they are
compatible. It makes for simpler changes & it avoids touching
devicetrees.
- properties:
compatible:
oneOf:
- const: ti,j7200-usb
- const: ti,j721e-usb
AM64 is a duplicate of J721E. Remove it and update upstream
devicetrees.
- properties:
compatible:
oneOf:
- const: ti,j7200-usb
- items:
- const: ti,j721e-usb
- const: ti,am64-usb
J721E & AM64 are compatible, express that & update devicetrees.
Option one is simpler & doesn't change devicetrees so I'd lean in that
direction. What's your opinion?
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 22/11/2023 11:46, Théo Lebrun wrote:
>>
>> How ti,j721e-usb can be and cannot be compatible with am64 in the same time?
>
> The code tells us that there is no difference between ti,j721e-usb &
> ti,am64-usb. And the commit adding the of_device_id entry agrees, see
> 4f30b9d2315f (usb: cdns3: Add support for TI's AM64 SoC, 2021-01-19).
> Here is the entire patch because it is so small:
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 90e246601537..eccb1c766bba 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
> @@ -214,6 +214,7 @@ static int cdns_ti_remove(struct platform_device *pdev)
>
> static const struct of_device_id cdns_ti_of_match[] = {
> { .compatible = "ti,j721e-usb", },
> + { .compatible = "ti,am64-usb", },
> {},
> };
> MODULE_DEVICE_TABLE(of, cdns_ti_of_match);
>
>>> I've followed the same scheme & added both of those:
>>>
>>> compatible = "ti,j7200-usb";
>>> compatible = "ti,j7200-usb", "ti,j721e-usb";
>>>
>>> I messed up the ordering in the added 'items' options, but the logic
>>> seems right to me. And dtbs_check agrees. Am I missing something?
>>
>> Logic is wrong. Device either is or is not compatible with something. At
>> least usually. We have some exceptions like SMMU for Adreno. Is this the
>> case? Why the device is and is not compatible with some other variant?
>
> My understanding is this: j721e & am64 are strictly equivalent. On our
Then this should be expressed in the bindings. Currently and in your
patch, you express that they are not compatible.
...
>
> - properties:
> compatible:
> oneOf:
> - const: ti,j7200-usb
> - items:
> - const: ti,j721e-usb
> - const: ti,am64-usb
>
> J721E & AM64 are compatible, express that & update devicetrees.
>
> Option one is simpler & doesn't change devicetrees so I'd lean in that
> direction. What's your opinion?
This one should be for am64.
For your j7200, it depends whether the fallback to j721e makes sense,
IOW, if the Linux can use j721e compatible solely to use j7200 device.
Best regards,
Krzysztof
Hello,
On Wed Nov 22, 2023 at 1:00 PM CET, Krzysztof Kozlowski wrote:
> On 22/11/2023 11:46, Théo Lebrun wrote:
> > - properties:
> > compatible:
> > oneOf:
> > - const: ti,j7200-usb
> > - items:
> > - const: ti,j721e-usb
> > - const: ti,am64-usb
> >
> > J721E & AM64 are compatible, express that & update devicetrees.
> >
> > Option one is simpler & doesn't change devicetrees so I'd lean in that
> > direction. What's your opinion?
>
> This one should be for am64.
>
> For your j7200, it depends whether the fallback to j721e makes sense,
> IOW, if the Linux can use j721e compatible solely to use j7200 device.
All compatibles might be equivalent if the reset-on-resume behavior is
observed on all three platforms. I don't have access to J721E or AM64
to test that.
@Roger Quadros: do you have any news if USB suspend/resume works on
J721E and/or AM64? My testing on the J7200 EVM is (1) boot, (2) put
the "USB2.0_MUX_SEL" GPIO high to have USB devices connected without
plugging anything, then (3) trigger a s2idle. I get a memory bus
exception on resume without my patches.
Regards,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Théo Lebrun <[email protected]> writes:
> Hi,
>
> Suspend on the TI J7200 platform is broken currently. There are two
> components that need to be patched so that they assume reset on
> resume: (1) the TI wrapper cdns3-ti & (2) the HOST role of the
> controller.
>
> About (1): the TI wrapper only did its hardware configuration at probe
> time. Update so that it is done at resume on J7200 SoC.
>
> About (2): signal from cdns3-ti to cdns3 host to xHCI that the
> controller resets on resume. This way we avoid the following warning:
>
> xhci-hcd xhci-hcd.1.auto: xHC error in resume, USBSTS 0x401, Reinit
>
> Strictly speaking (2) is not required to have working suspend on J7200;
> its only goal is to silence this warning.
>
> Those patches have been tested on the TI J7200 EVM GP. No need to
> mention that other patches are required for S2R to work, but those will
> be sent later down the road. Those USB patches are rather standalone.
>
> Thanks,
> Théo
>
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> Changes in v2:
> - Remove runtime PM from cdns3-ti; it brings nothing. That means our
> cdns3-ti suspend/resume patch is simpler; there is no need to handle
> runtime PM at suspend/resume.
Sorry I sent comments on v2 before I noticed v3 was out, but this is not
a good idea IMO. I sent more detailed comment on that specific patch.
Kevin
Théo Lebrun <[email protected]> writes:
> The driver does not use RPM. It enables it & gets a reference at probe.
> It then undoes that on probe error or at remove.
...which is a fairly standard thing to do for a rudimentary runtime PM
support on platforms that use power domains.
This will likely (almost surely) break other platforms.
Without a runtime PM get call, the power domain that this device is in
could be powered off without this driver ever knowing about it, causing
a crash as soon as the driver is used after the domain is turned off.
Kevin