Hi,
Here is a new revision of the J7200 USB suspend fix. It is currently
broken on the platform, leading to a kernel panic at resume. Patches
are tested on a J7200 evaluation board, both s2idle and suspend-to-RAM.
Previous discussion showed that runtime PM was not to be deleted.
Indeed, we cannot guarantee PM domains will stay active if we don't
register to runtime PM.
We therefore rely fully on runtime PM and put our init sequence
previously done in the probe in our ->runtime_resume(). We then do the
necessary to get it called at system-wide resume for the J7200
platform.
Have a nice day,
Théo
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Signed-off-by: Théo Lebrun <[email protected]>
---
Changes in v3:
- dt-bindings: use an enum to list compatibles instead of the previous
odd construct. This is done in a separate patch from the one adding
J7200 compatible.
- dt-bindings: dropped Acked-by Conor as the changes were modified a lot.
- Add runtime PM back. Put the init sequence in ->runtime_resume(). It
gets called at probe for all compatibles and at resume for J7200.
- Introduce a cdns_ti_match_data struct rather than rely on compatible
from code.
- Reorder code changes. Add infrastructure based on match data THEN add
compatible and its match data.
- DTSI: use only J7200 compatible rather than both J7200 then J721E.
- Link to v2: https://lore.kernel.org/r/[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 (8):
dt-bindings: usb: ti,j721e-usb: drop useless compatible list
dt-bindings: usb: ti,j721e-usb: add ti,j7200-usb compatible
usb: cdns3-ti: move reg writes from probe into ->runtime_resume()
usb: cdns3-ti: support reset-on-resume behavior
usb: cdns3-ti: pass auxdata from match data to of_platform_populate()
usb: cdns3: add quirk to platform data for reset-on-resume
usb: cdns3-ti: add J7200 support with reset-on-resume behavior
arm64: dts: ti: k3-j7200: use J7200-specific USB compatible
.../devicetree/bindings/usb/ti,j721e-usb.yaml | 10 +-
arch/arm64/boot/dts/ti/k3-j7200-main.dtsi | 2 +-
drivers/usb/cdns3/cdns3-ti.c | 125 ++++++++++++++++-----
drivers/usb/cdns3/core.h | 1 +
drivers/usb/cdns3/host.c | 3 +
5 files changed, 104 insertions(+), 37 deletions(-)
---
base-commit: 1871c27e3539e5b812d50ec6ccad7567ec5414f2
change-id: 20231113-j7200-usb-suspend-2a47f2281e04
Best regards,
--
Théo Lebrun <[email protected]>
On J7200, the controller & its wrapper are reset on resume. This makes
it have a different behavior from other platforms.
Signed-off-by: Théo Lebrun <[email protected]>
---
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 949f45eb45c2..d52d5b2965aa 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -14,6 +14,7 @@ properties:
enum:
- ti,j721e-usb
- ti,am64-usb
+ - ti,j7200-usb
reg:
maxItems: 1
--
2.43.2
The hardware initialisation register write sequence is only used at
probe. Move it from being done at explicitely at probe to being done
implicitely by pm_runtime_get_sync() that calls ->runtime_resume().
Keep devicetree parsing in probe and add a new field in the private
struct to remember the USB2 refclk rate code computation result.
This opens the door to having the init sequence being executed later
down the road, at system-wide resume for example. This is NOT currently
happening because runtime PM is disabled at suspend without the
refcount being affected.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 90 +++++++++++++++++++++++++-------------------
1 file changed, 52 insertions(+), 38 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 5945c4b1e11f..4c8a557e6a6f 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -57,6 +57,7 @@ struct cdns_ti {
unsigned vbus_divider:1;
struct clk *usb2_refclk;
struct clk *lpm_clk;
+ int usb2_refclk_rate_code;
};
static const int cdns_ti_rate_table[] = { /* in KHZ */
@@ -90,10 +91,8 @@ static int cdns_ti_probe(struct platform_device *pdev)
struct device *dev = &pdev->dev;
struct device_node *node = pdev->dev.of_node;
struct cdns_ti *data;
- int error;
- u32 reg;
- int rate_code, i;
unsigned long rate;
+ int error, i;
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -133,7 +132,9 @@ static int cdns_ti_probe(struct platform_device *pdev)
return -EINVAL;
}
- rate_code = i;
+ data->usb2_refclk_rate_code = i;
+ data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
+ data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
pm_runtime_enable(dev);
error = pm_runtime_get_sync(dev);
@@ -142,40 +143,6 @@ static int cdns_ti_probe(struct platform_device *pdev)
goto err;
}
- /* assert RESET */
- reg = cdns_ti_readl(data, USBSS_W1);
- reg &= ~USBSS_W1_PWRUP_RST;
- cdns_ti_writel(data, USBSS_W1, reg);
-
- /* set static config */
- reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
- reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
- reg |= rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
-
- reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
- data->vbus_divider = device_property_read_bool(dev, "ti,vbus-divider");
- if (data->vbus_divider)
- reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
-
- cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
- reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
-
- /* set USB2_ONLY mode if requested */
- reg = cdns_ti_readl(data, USBSS_W1);
- data->usb2_only = device_property_read_bool(dev, "ti,usb2-only");
- if (data->usb2_only)
- reg |= USBSS_W1_USB2_ONLY;
-
- /* set default modestrap */
- reg |= USBSS_W1_MODESTRAP_SEL;
- reg &= ~USBSS_W1_MODESTRAP_MASK;
- reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
- cdns_ti_writel(data, USBSS_W1, reg);
-
- /* de-assert RESET */
- reg |= USBSS_W1_PWRUP_RST;
- cdns_ti_writel(data, USBSS_W1, reg);
-
error = of_platform_populate(node, NULL, NULL, dev);
if (error) {
dev_err(dev, "failed to create children: %d\n", error);
@@ -211,6 +178,52 @@ static void cdns_ti_remove(struct platform_device *pdev)
platform_set_drvdata(pdev, NULL);
}
+static int cdns_ti_runtime_resume(struct device *dev)
+{
+ struct cdns_ti *data = dev_get_drvdata(dev);
+ u32 reg;
+
+ /* assert RESET */
+ reg = cdns_ti_readl(data, USBSS_W1);
+ reg &= ~USBSS_W1_PWRUP_RST;
+ cdns_ti_writel(data, USBSS_W1, reg);
+
+ /* set static config */
+ reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+ reg &= ~USBSS1_STATIC_PLL_REF_SEL_MASK;
+ reg |= data->usb2_refclk_rate_code << USBSS1_STATIC_PLL_REF_SEL_SHIFT;
+
+ reg &= ~USBSS1_STATIC_VBUS_SEL_MASK;
+
+ if (data->vbus_divider)
+ reg |= 1 << USBSS1_STATIC_VBUS_SEL_SHIFT;
+
+ cdns_ti_writel(data, USBSS_STATIC_CONFIG, reg);
+ reg = cdns_ti_readl(data, USBSS_STATIC_CONFIG);
+
+ /* set USB2_ONLY mode if requested */
+ reg = cdns_ti_readl(data, USBSS_W1);
+
+ if (data->usb2_only)
+ reg |= USBSS_W1_USB2_ONLY;
+
+ /* set default modestrap */
+ reg |= USBSS_W1_MODESTRAP_SEL;
+ reg &= ~USBSS_W1_MODESTRAP_MASK;
+ reg |= USBSS_MODESTRAP_MODE_NONE << USBSS_W1_MODESTRAP_SHIFT;
+ cdns_ti_writel(data, USBSS_W1, reg);
+
+ /* de-assert RESET */
+ reg |= USBSS_W1_PWRUP_RST;
+ cdns_ti_writel(data, USBSS_W1, reg);
+
+ return 0;
+}
+
+static const struct dev_pm_ops cdns_ti_pm_ops = {
+ RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
+};
+
static const struct of_device_id cdns_ti_of_match[] = {
{ .compatible = "ti,j721e-usb", },
{ .compatible = "ti,am64-usb", },
@@ -224,6 +237,7 @@ static struct platform_driver cdns_ti_driver = {
.driver = {
.name = "cdns3-ti",
.of_match_table = cdns_ti_of_match,
+ .pm = pm_ptr(&cdns_ti_pm_ops),
},
};
--
2.43.2
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.
Acked-by: Peter Chen <[email protected]>
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.43.2
Add ti,j7200-usb compatible. Match data indicates the controller resets
on resume meaning:
- The cdns3-ti wrapper init sequence must be ran at resume.
- Tell the cdns3 core that we reset on resume. This silences a xHCI
warning visible in cases of unexpected resets.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 29fb24c811b3..648243a27987 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -17,6 +17,8 @@
#include <linux/pm_runtime.h>
#include <linux/property.h>
+#include "core.h"
+
/* USB Wrapper register offsets */
#define USBSS_PID 0x0
#define USBSS_W1 0x4
@@ -255,7 +257,25 @@ static const struct dev_pm_ops cdns_ti_pm_ops = {
SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume)
};
+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[] = {
+ { .compatible = "ti,j7200-usb", .data = &cdns_ti_j7200_match_data, },
{ .compatible = "ti,j721e-usb", },
{ .compatible = "ti,am64-usb", },
{},
--
2.43.2
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 45ba9c2d0344..134d6e5f3b01 100644
--- a/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-j7200-main.dtsi
@@ -793,7 +793,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.43.2
Allow compatible to pick auxdata given to child platform devices.
No compatible exploits this functionality, just yet.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index f76327566798..29fb24c811b3 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -62,7 +62,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 */
@@ -95,6 +96,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;
@@ -149,7 +151,9 @@ static int cdns_ti_probe(struct platform_device *pdev)
goto err;
}
- 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);
goto err;
--
2.43.2
Compatible can be A or B, not A or B or A+B. Remove last option.
A=ti,j721e-usb and B=ti,am64-usb.
Signed-off-by: Théo Lebrun <[email protected]>
---
Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
index 95ff9791baea..949f45eb45c2 100644
--- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
+++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
@@ -11,12 +11,9 @@ maintainers:
properties:
compatible:
- oneOf:
- - const: ti,j721e-usb
- - const: ti,am64-usb
- - items:
- - const: ti,j721e-usb
- - const: ti,am64-usb
+ enum:
+ - ti,j721e-usb
+ - ti,am64-usb
reg:
maxItems: 1
--
2.43.2
Add match data support, with one boolean to indicate whether the
hardware resets after a system-wide suspend. If hardware resets, we
force execute ->runtime_resume() at system-wide resume to run the
hardware init sequence.
No compatible exploits this functionality, just yet.
Signed-off-by: Théo Lebrun <[email protected]>
---
drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
1 file changed, 27 insertions(+)
diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
index 4c8a557e6a6f..f76327566798 100644
--- a/drivers/usb/cdns3/cdns3-ti.c
+++ b/drivers/usb/cdns3/cdns3-ti.c
@@ -57,9 +57,14 @@ struct cdns_ti {
unsigned vbus_divider:1;
struct clk *usb2_refclk;
struct clk *lpm_clk;
+ const struct cdns_ti_match_data *match_data;
int usb2_refclk_rate_code;
};
+struct cdns_ti_match_data {
+ bool reset_on_resume;
+};
+
static const int cdns_ti_rate_table[] = { /* in KHZ */
9600,
10000,
@@ -101,6 +106,7 @@ static int cdns_ti_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, data);
data->dev = dev;
+ data->match_data = device_get_match_data(dev);
data->usbss = devm_platform_ioremap_resource(pdev, 0);
if (IS_ERR(data->usbss)) {
@@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
return 0;
}
+static int cdns_ti_suspend(struct device *dev)
+{
+ struct cdns_ti *data = dev_get_drvdata(dev);
+
+ if (data->match_data && data->match_data->reset_on_resume)
+ return pm_runtime_force_suspend(dev);
+ else
+ return 0;
+}
+
+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)
+ return pm_runtime_force_resume(dev);
+ else
+ return 0;
+}
+
static const struct dev_pm_ops cdns_ti_pm_ops = {
RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
+ SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume)
};
static const struct of_device_id cdns_ti_of_match[] = {
--
2.43.2
On Fri, Feb 23, 2024 at 05:05:25PM +0100, Th?o Lebrun wrote:
> Compatible can be A or B, not A or B or A+B. Remove last option.
> A=ti,j721e-usb and B=ti,am64-usb.
>
> Signed-off-by: Th?o Lebrun <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> index 95ff9791baea..949f45eb45c2 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -11,12 +11,9 @@ maintainers:
>
> properties:
> compatible:
> - oneOf:
> - - const: ti,j721e-usb
> - - const: ti,am64-usb
> - - items:
> - - const: ti,j721e-usb
> - - const: ti,am64-usb
Correct, this makes no sense. The devices seem to be compatible though,
so I would expect this to actually be:
oneOf:
- const: ti,j721e-usb
- items:
- const: ti,am64-usb
- const: ti,j721e-usb
> + enum:
> + - ti,j721e-usb
> + - ti,am64-usb
>
> reg:
> maxItems: 1
>
> --
> 2.43.2
>
On Fri, Feb 23, 2024 at 05:05:26PM +0100, Th?o Lebrun wrote:
> On J7200, the controller & its wrapper are reset on resume. This makes
> it have a different behavior from other platforms.
>
> Signed-off-by: Th?o Lebrun <[email protected]>
> ---
> 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 949f45eb45c2..d52d5b2965aa 100644
> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> @@ -14,6 +14,7 @@ properties:
> enum:
> - ti,j721e-usb
> - ti,am64-usb
> + - ti,j7200-usb
This patch will change with what I mentioned on the other binding patch,
but the patch itself is reasonable.
Acked-by: Conor Dooley <[email protected]>
Cheers,
Conor.
>
> reg:
> maxItems: 1
>
> --
> 2.43.2
>
On 2/23/24 7:05 PM, Théo Lebrun wrote:
> Add match data support, with one boolean to indicate whether the
> hardware resets after a system-wide suspend. If hardware resets, we
> force execute ->runtime_resume() at system-wide resume to run the
> hardware init sequence.
>
> No compatible exploits this functionality, just yet.
>
> Signed-off-by: Théo Lebrun <[email protected]>
> ---
> drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> index 4c8a557e6a6f..f76327566798 100644
> --- a/drivers/usb/cdns3/cdns3-ti.c
> +++ b/drivers/usb/cdns3/cdns3-ti.c
[...]
> @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
> return 0;
> }
>
> +static int cdns_ti_suspend(struct device *dev)
> +{
> + struct cdns_ti *data = dev_get_drvdata(dev);
> +
> + if (data->match_data && data->match_data->reset_on_resume)
> + return pm_runtime_force_suspend(dev);
> + else
Pointless *else* after *return*...
> + return 0;
> +}
> +
> +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)
> + return pm_runtime_force_resume(dev);
> + else
Here as well...
> + return 0;
> +}
> +
> static const struct dev_pm_ops cdns_ti_pm_ops = {
> RUNTIME_PM_OPS(NULL, cdns_ti_runtime_resume, NULL)
> + SYSTEM_SLEEP_PM_OPS(cdns_ti_suspend, cdns_ti_resume)
> };
>
> static const struct of_device_id cdns_ti_of_match[] = {
MBR, Sergey
Hello Sergey,
On Sat Feb 24, 2024 at 10:08 AM CET, Sergei Shtylyov wrote:
> On 2/23/24 7:05 PM, Théo Lebrun wrote:
> > Add match data support, with one boolean to indicate whether the
> > hardware resets after a system-wide suspend. If hardware resets, we
> > force execute ->runtime_resume() at system-wide resume to run the
> > hardware init sequence.
> >
> > No compatible exploits this functionality, just yet.
> >
> > Signed-off-by: Théo Lebrun <[email protected]>
> > ---
> > drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
> > 1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
> > index 4c8a557e6a6f..f76327566798 100644
> > --- a/drivers/usb/cdns3/cdns3-ti.c
> > +++ b/drivers/usb/cdns3/cdns3-ti.c
> [...]
> > @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
> > return 0;
> > }
> >
> > +static int cdns_ti_suspend(struct device *dev)
> > +{
> > + struct cdns_ti *data = dev_get_drvdata(dev);
> > +
> > + if (data->match_data && data->match_data->reset_on_resume)
> > + return pm_runtime_force_suspend(dev);
> > + else
>
> Pointless *else* after *return*...
Indeed! I used this form explicitely as it reads nicely: "if reset on
reset, force suspend, else do nothing". It also prevents the error of
adding behavior below the if-statement without seeing that it won't
apply to both cases.
If you do believe it would make the code better I'll happily change it
for the next revision, I do not mind.
Thanks for the review Sergey!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Hello Conor,
On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote:
> On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote:
> > Compatible can be A or B, not A or B or A+B. Remove last option.
> > A=ti,j721e-usb and B=ti,am64-usb.
> >
> > Signed-off-by: Théo Lebrun <[email protected]>
> > ---
> > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
> > 1 file changed, 3 insertions(+), 6 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > index 95ff9791baea..949f45eb45c2 100644
> > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > @@ -11,12 +11,9 @@ maintainers:
> >
> > properties:
> > compatible:
> > - oneOf:
> > - - const: ti,j721e-usb
> > - - const: ti,am64-usb
> > - - items:
> > - - const: ti,j721e-usb
> > - - const: ti,am64-usb
>
> Correct, this makes no sense. The devices seem to be compatible though,
> so I would expect this to actually be:
> oneOf:
> - const: ti,j721e-usb
> - items:
> - const: ti,am64-usb
> - const: ti,j721e-usb
I need your help to grasp what that change is supposed to express? Would
you mind turning it into english sentences?
A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is
that a device can either be compat with A or B. But B is compatible
with A so you express it as a list of items. If B is compat with A then
A is compat with B. Does the order of items matter?
I've not applied your proposal to check for dtbs_check but I'd guess it
would throw warnings for the single existing upstream DTSI (as of
v6.8-rc6) that uses "ti,am64-usb"? See:
arch/arm64/boot/dts/ti/k3-am64-main.dtsi.
Thanks Conor!
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On Mon, Feb 26, 2024 at 11:33:06AM +0100, Th?o Lebrun wrote:
> Hello Conor,
>
> On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote:
> > On Fri, Feb 23, 2024 at 05:05:25PM +0100, Th?o Lebrun wrote:
> > > Compatible can be A or B, not A or B or A+B. Remove last option.
> > > A=ti,j721e-usb and B=ti,am64-usb.
> > >
> > > Signed-off-by: Th?o Lebrun <[email protected]>
> > > ---
> > > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
> > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > > index 95ff9791baea..949f45eb45c2 100644
> > > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > > @@ -11,12 +11,9 @@ maintainers:
> > >
> > > properties:
> > > compatible:
> > > - oneOf:
> > > - - const: ti,j721e-usb
> > > - - const: ti,am64-usb
> > > - - items:
> > > - - const: ti,j721e-usb
> > > - - const: ti,am64-usb
> >
> > Correct, this makes no sense. The devices seem to be compatible though,
> > so I would expect this to actually be:
> > oneOf:
> > - const: ti,j721e-usb
> > - items:
> > - const: ti,am64-usb
> > - const: ti,j721e-usb
>
> I need your help to grasp what that change is supposed to express? Would
> you mind turning it into english sentences?
> A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is
> that a device can either be compat with A or B. But B is compatible
> with A so you express it as a list of items. If B is compat with A then
> A is compat with B. Does the order of items matter?
The two devices are compatible with each other, based on an inspection of
the driver and the existing "A+B" setup. If this was a newly submitted
binding, "B" would not get approved because "A+B" allows support without
software changes and all that jazz.
Your patch says that allowing "A", "B" and "A+B" makes no sense and you
suggest removing "A+B". I am agreeing that it makes no sense to allow
all 3 of these situations.
What I also noticed is other problems with the binding. What should have
been "A+B" is actually documented as "B+A", but that doesn't make sense
when the originally supported device is "A".
Therefore my suggestion was to only allow "A" and "A+B", which is what
we would (hopefully) tell you to do were you submitting the am64 support
as a new patch today.
> I've not applied your proposal to check for dtbs_check but I'd guess it
> would throw warnings for the single existing upstream DTSI (as of
> v6.8-rc6) that uses "ti,am64-usb"? See:
> arch/arm64/boot/dts/ti/k3-am64-main.dtsi.
Yeah, it would but it's not as if that cannot be changed. There's no
concerns here about backwards compatibility here, right?
Hello,
On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote:
> On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote:
> > Hello Conor,
> >
> > On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote:
> > > On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote:
> > > > Compatible can be A or B, not A or B or A+B. Remove last option.
> > > > A=ti,j721e-usb and B=ti,am64-usb.
> > > >
> > > > Signed-off-by: Théo Lebrun <[email protected]>
> > > > ---
> > > > Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
> > > > 1 file changed, 3 insertions(+), 6 deletions(-)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > > > index 95ff9791baea..949f45eb45c2 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > > > +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> > > > @@ -11,12 +11,9 @@ maintainers:
> > > >
> > > > properties:
> > > > compatible:
> > > > - oneOf:
> > > > - - const: ti,j721e-usb
> > > > - - const: ti,am64-usb
> > > > - - items:
> > > > - - const: ti,j721e-usb
> > > > - - const: ti,am64-usb
> > >
> > > Correct, this makes no sense. The devices seem to be compatible though,
> > > so I would expect this to actually be:
> > > oneOf:
> > > - const: ti,j721e-usb
> > > - items:
> > > - const: ti,am64-usb
> > > - const: ti,j721e-usb
> >
> > I need your help to grasp what that change is supposed to express? Would
> > you mind turning it into english sentences?
> > A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is
> > that a device can either be compat with A or B. But B is compatible
> > with A so you express it as a list of items. If B is compat with A then
> > A is compat with B. Does the order of items matter?
>
> The two devices are compatible with each other, based on an inspection of
> the driver and the existing "A+B" setup. If this was a newly submitted
> binding, "B" would not get approved because "A+B" allows support without
> software changes and all that jazz.
>
> Your patch says that allowing "A", "B" and "A+B" makes no sense and you
> suggest removing "A+B". I am agreeing that it makes no sense to allow
> all 3 of these situations.
>
> What I also noticed is other problems with the binding. What should have
> been "A+B" is actually documented as "B+A", but that doesn't make sense
> when the originally supported device is "A".
>
> Therefore my suggestion was to only allow "A" and "A+B", which is what
> we would (hopefully) tell you to do were you submitting the am64 support
> as a new patch today.
Thank you for the in-depth explanation! It makes much more sense now,
especially the handling of historic stuff that ideally wouldn't have
been done this way but that won't be changed from now on.
> > I've not applied your proposal to check for dtbs_check but I'd guess it
> > would throw warnings for the single existing upstream DTSI (as of
> > v6.8-rc6) that uses "ti,am64-usb"? See:
> > arch/arm64/boot/dts/ti/k3-am64-main.dtsi.
>
> Yeah, it would but it's not as if that cannot be changed. There's no
> concerns here about backwards compatibility here, right?
I'm not involved in the maintenance of this platform so I do not believe
I should be answering this question. I asked the question because I
taught there always were concerns of backwards-compat when it comes to
DT and dt-bindings (in the best of all possible worlds).
K3 maintainers are already in cc.
Thanks,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
On 26/02/24 20:05, Théo Lebrun wrote:
> Hello,
>
> On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote:
>> On Mon, Feb 26, 2024 at 11:33:06AM +0100, Théo Lebrun wrote:
>>> Hello Conor,
>>>
>>> On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote:
>>>> On Fri, Feb 23, 2024 at 05:05:25PM +0100, Théo Lebrun wrote:
>>>>> Compatible can be A or B, not A or B or A+B. Remove last option.
>>>>> A=ti,j721e-usb and B=ti,am64-usb.
>>>>>
>>>>> Signed-off-by: Théo Lebrun <[email protected]>
>>>>> ---
>>>>> Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
>>>>> 1 file changed, 3 insertions(+), 6 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>>>> index 95ff9791baea..949f45eb45c2 100644
>>>>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>>>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
>>>>> @@ -11,12 +11,9 @@ maintainers:
>>>>>
>>>>> properties:
>>>>> compatible:
>>>>> - oneOf:
>>>>> - - const: ti,j721e-usb
>>>>> - - const: ti,am64-usb
>>>>> - - items:
>>>>> - - const: ti,j721e-usb
>>>>> - - const: ti,am64-usb
>>>>
>>>> Correct, this makes no sense. The devices seem to be compatible though,
>>>> so I would expect this to actually be:
>>>> oneOf:
>>>> - const: ti,j721e-usb
>>>> - items:
>>>> - const: ti,am64-usb
>>>> - const: ti,j721e-usb
>>>
>>> I need your help to grasp what that change is supposed to express? Would
>>> you mind turning it into english sentences?
>>> A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is
>>> that a device can either be compat with A or B. But B is compatible
>>> with A so you express it as a list of items. If B is compat with A then
>>> A is compat with B. Does the order of items matter?
>>
>> The two devices are compatible with each other, based on an inspection of
>> the driver and the existing "A+B" setup. If this was a newly submitted
>> binding, "B" would not get approved because "A+B" allows support without
>> software changes and all that jazz.
>>
>> Your patch says that allowing "A", "B" and "A+B" makes no sense and you
>> suggest removing "A+B". I am agreeing that it makes no sense to allow
>> all 3 of these situations.
>>
>> What I also noticed is other problems with the binding. What should have
>> been "A+B" is actually documented as "B+A", but that doesn't make sense
>> when the originally supported device is "A".
>>
>> Therefore my suggestion was to only allow "A" and "A+B", which is what
>> we would (hopefully) tell you to do were you submitting the am64 support
>> as a new patch today.
>
> Thank you for the in-depth explanation! It makes much more sense now,
> especially the handling of historic stuff that ideally wouldn't have
> been done this way but that won't be changed from now on.
>
IIRC, idea behind adding new compatible for AM64 even though register
map is very much compatible is just being future proof as AM64 and J721e
belong to different product groups and thus have differences wrt SoC
level integration etc which may need SoC specific handling later on.
I don't see any DT (now or in the past) using
compatible = B,A or compatible = A,B
So do we really need A+B to be supported by binding?
Also, note that AM64 SoC support was added long after J721e. So ideally
should be B+A if at all we need a fallback compatible.
Regards
Vignesh
--
Regards
Vignesh
On Tue, Feb 27, 2024 at 09:54:30AM +0530, Vignesh Raghavendra wrote:
> On 26/02/24 20:05, Th?o Lebrun wrote:
> > On Mon Feb 26, 2024 at 12:56 PM CET, Conor Dooley wrote:
> >> On Mon, Feb 26, 2024 at 11:33:06AM +0100, Th?o Lebrun wrote:
> >>> Hello Conor,
> >>>
> >>> On Fri Feb 23, 2024 at 7:12 PM CET, Conor Dooley wrote:
> >>>> On Fri, Feb 23, 2024 at 05:05:25PM +0100, Th?o Lebrun wrote:
> >>>>> Compatible can be A or B, not A or B or A+B. Remove last option.
> >>>>> A=ti,j721e-usb and B=ti,am64-usb.
> >>>>>
> >>>>> Signed-off-by: Th?o Lebrun <[email protected]>
> >>>>> ---
> >>>>> Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml | 9 +++------
> >>>>> 1 file changed, 3 insertions(+), 6 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>>>> index 95ff9791baea..949f45eb45c2 100644
> >>>>> --- a/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/usb/ti,j721e-usb.yaml
> >>>>> @@ -11,12 +11,9 @@ maintainers:
> >>>>>
> >>>>> properties:
> >>>>> compatible:
> >>>>> - oneOf:
> >>>>> - - const: ti,j721e-usb
> >>>>> - - const: ti,am64-usb
> >>>>> - - items:
> >>>>> - - const: ti,j721e-usb
> >>>>> - - const: ti,am64-usb
> >>>>
> >>>> Correct, this makes no sense. The devices seem to be compatible though,
> >>>> so I would expect this to actually be:
> >>>> oneOf:
> >>>> - const: ti,j721e-usb
> >>>> - items:
> >>>> - const: ti,am64-usb
> >>>> - const: ti,j721e-usb
> >>>
> >>> I need your help to grasp what that change is supposed to express? Would
> >>> you mind turning it into english sentences?
> >>> A=ti,j721e-usb and B=ti,am64-usb. My understanding of your proposal is
> >>> that a device can either be compat with A or B. But B is compatible
> >>> with A so you express it as a list of items. If B is compat with A then
> >>> A is compat with B. Does the order of items matter?
> >>
> >> The two devices are compatible with each other, based on an inspection of
> >> the driver and the existing "A+B" setup. If this was a newly submitted
> >> binding, "B" would not get approved because "A+B" allows support without
> >> software changes and all that jazz.
> >>
> >> Your patch says that allowing "A", "B" and "A+B" makes no sense and you
> >> suggest removing "A+B". I am agreeing that it makes no sense to allow
> >> all 3 of these situations.
> >>
> >> What I also noticed is other problems with the binding. What should have
> >> been "A+B" is actually documented as "B+A", but that doesn't make sense
> >> when the originally supported device is "A".
This A and B stuff confused me, I should just have used the actual
compatibles. I meant
| What should have been "B+A" is actually documented as "A+B", but that
| doesn't make sense when the originally supported device is "A"
> >>
> >> Therefore my suggestion was to only allow "A" and "A+B", which is what
> >> we would (hopefully) tell you to do were you submitting the am64 support
> >> as a new patch today.
> >
> > Thank you for the in-depth explanation! It makes much more sense now,
> > especially the handling of historic stuff that ideally wouldn't have
> > been done this way but that won't be changed from now on.
> >
>
> IIRC, idea behind adding new compatible for AM64 even though register
> map is very much compatible is just being future proof as AM64 and J721e
> belong to different product groups and thus have differences wrt SoC
> level integration etc which may need SoC specific handling later on.
That is fine, I don't think anyone here is disputing a soc-specific
compatible existing for this device.
> Also, note that AM64 SoC support was added long after J721e. So ideally
> should be B+A if at all we need a fallback compatible.
Correct, I accidentally wrote "A+B", but you can see that that
conflicted with the actual example I had given above.
> I don't see any DT (now or in the past) using
>
> compatible = B,A or compatible = A,B
>
> So do we really need A+B to be supported by binding?
Given the mistake, I am going to take this as meaning should the
fallback be supported. My take is that if we are going to remove
something, it should be "ti,am64-usb" isolation that should go.
The devicetrees can be update without concerns about compatibility.
Cheers,
Conor.
On 2/26/24 1:13 PM, Théo Lebrun wrote:
[...]
>>> Add match data support, with one boolean to indicate whether the
>>> hardware resets after a system-wide suspend. If hardware resets, we
>>> force execute ->runtime_resume() at system-wide resume to run the
>>> hardware init sequence.
>>>
>>> No compatible exploits this functionality, just yet.
>>>
>>> Signed-off-by: Théo Lebrun <[email protected]>
>>> ---
>>> drivers/usb/cdns3/cdns3-ti.c | 27 +++++++++++++++++++++++++++
>>> 1 file changed, 27 insertions(+)
>>>
>>> diff --git a/drivers/usb/cdns3/cdns3-ti.c b/drivers/usb/cdns3/cdns3-ti.c
>>> index 4c8a557e6a6f..f76327566798 100644
>>> --- a/drivers/usb/cdns3/cdns3-ti.c
>>> +++ b/drivers/usb/cdns3/cdns3-ti.c
>> [...]
>>> @@ -220,8 +226,29 @@ static int cdns_ti_runtime_resume(struct device *dev)
>>> return 0;
>>> }
>>>
>>> +static int cdns_ti_suspend(struct device *dev)
>>> +{
>>> + struct cdns_ti *data = dev_get_drvdata(dev);
>>> +
>>> + if (data->match_data && data->match_data->reset_on_resume)
>>> + return pm_runtime_force_suspend(dev);
>>> + else
>>
>> Pointless *else* after *return*...
>
> Indeed! I used this form explicitely as it reads nicely: "if reset on
> reset, force suspend, else do nothing". It also prevents the error of
s/reset/resume/ here? :-)
> adding behavior below the if-statement without seeing that it won't
> apply to both cases.
You were going to add stuff after the final *return*? :-)
> If you do believe it would make the code better I'll happily change it
> for the next revision, I do not mind.
Up to you!
This is a thing people usually complain about when reviewing
patches. I even thought checkpatch.pl would complain as well, but it
didn't... :-)
> Thanks for the review Sergey!
>
> --
> Théo Lebrun, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
MBR, Swrgey