* [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library
2026-05-11 2:42 [PATCH 0/4] Add CIX Sky1 Cadence USB3 support Peter Chen
@ 2026-05-11 2:42 ` Peter Chen
2026-05-11 23:02 ` sashiko-bot
2026-05-11 2:42 ` [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver Peter Chen
` (2 subsequent siblings)
3 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2026-05-11 2:42 UTC (permalink / raw)
To: robh, krzk+dt, conor+dt, gregkh, pawell, rogerq
Cc: devicetree, linux-kernel, linux-usb, cix-kernel-upstream,
linux-arm-kernel, arnd, Peter Chen
Split the Cadence USB3 platform probe/remove and PM paths into
cdns3_core_probe(), cdns3_core_remove(), and exported runtime/system
sleep helpers so SoC glue (e.g. Sky1) can instantiate the core on the
same platform_device deterministically.
Add glue.h documenting struct cdns3_probe_data and the public entry points.
Signed-off-by: Peter Chen <peter.chen@cixtech.com>
---
drivers/usb/cdns3/cdns3-plat.c | 138 ++++++++++++++++++++++-----------
drivers/usb/cdns3/glue.h | 51 ++++++++++++
2 files changed, 144 insertions(+), 45 deletions(-)
create mode 100644 drivers/usb/cdns3/glue.h
diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
index 3fe3109a3688..2219cbff1c59 100644
--- a/drivers/usb/cdns3/cdns3-plat.c
+++ b/drivers/usb/cdns3/cdns3-plat.c
@@ -21,6 +21,7 @@
#include "core.h"
#include "gadget-export.h"
+#include "glue.h"
#include "host-export.h"
#include "drd.h"
@@ -59,29 +60,21 @@ static int cdns3_plat_host_init(struct cdns *cdns)
}
/**
- * cdns3_plat_probe - probe for cdns3 core device
- * @pdev: Pointer to cdns3 core platform device
+ * cdns3_core_probe - Initialize the Cadence USB3 platform core
+ * @data: Controller context and platform device supplied by the glue layer
*
* Returns 0 on success otherwise negative errno
*/
-static int cdns3_plat_probe(struct platform_device *pdev)
+int cdns3_core_probe(const struct cdns3_probe_data *data)
{
+ struct platform_device *pdev = data->pdev;
struct device *dev = &pdev->dev;
- struct resource *res;
- struct cdns *cdns;
+ struct cdns *cdns = data->cdns;
+ struct resource *res;
void __iomem *regs;
int ret;
- cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
- if (!cdns)
- return -ENOMEM;
-
- cdns->dev = dev;
- cdns->pdata = dev_get_platdata(dev);
- if (cdns->pdata && cdns->pdata->override_apb_timeout)
- cdns->override_apb_timeout = cdns->pdata->override_apb_timeout;
-
- platform_set_drvdata(pdev, cdns);
+ dev_set_drvdata(dev, cdns);
ret = platform_get_irq_byname(pdev, "host");
if (ret < 0)
@@ -195,14 +188,41 @@ static int cdns3_plat_probe(struct platform_device *pdev)
return ret;
}
+EXPORT_SYMBOL_GPL(cdns3_core_probe);
/**
- * cdns3_plat_remove() - unbind drd driver and clean up
- * @pdev: Pointer to Linux platform device
+ * cdns3_plat_probe - probe for cdns3 core device
+ * @pdev: Pointer to cdns3 core platform device
+ *
+ * Returns 0 on success otherwise negative errno
*/
-static void cdns3_plat_remove(struct platform_device *pdev)
+static int cdns3_plat_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cdns *cdns;
+ struct cdns3_probe_data probe_data;
+
+ cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
+ if (!cdns)
+ return -ENOMEM;
+
+ cdns->dev = dev;
+ cdns->pdata = dev_get_platdata(dev);
+ if (cdns->pdata && cdns->pdata->override_apb_timeout)
+ cdns->override_apb_timeout = cdns->pdata->override_apb_timeout;
+
+ probe_data.cdns = cdns;
+ probe_data.pdev = pdev;
+
+ return cdns3_core_probe(&probe_data);
+}
+
+/**
+ * cdns3_core_remove - Tear down the Cadence USB3 platform core
+ * @cdns: Controller context previously initialized by cdns3_core_probe()
+ */
+void cdns3_core_remove(struct cdns *cdns)
{
- struct cdns *cdns = platform_get_drvdata(pdev);
struct device *dev = cdns->dev;
pm_runtime_get_sync(dev);
@@ -213,24 +233,30 @@ static void cdns3_plat_remove(struct platform_device *pdev)
phy_exit(cdns->usb2_phy);
phy_exit(cdns->usb3_phy);
}
+EXPORT_SYMBOL_GPL(cdns3_core_remove);
+
+/**
+ * cdns3_plat_remove() - unbind drd driver and clean up
+ * @pdev: Pointer to Linux platform device
+ */
+static void cdns3_plat_remove(struct platform_device *pdev)
+{
+ cdns3_core_remove(platform_get_drvdata(pdev));
+}
#ifdef CONFIG_PM
-static int cdns3_set_platform_suspend(struct device *dev,
- bool suspend, bool wakeup)
+static int cdns3_set_platform_suspend(struct cdns *cdns, bool suspend, bool wakeup)
{
- struct cdns *cdns = dev_get_drvdata(dev);
- int ret = 0;
-
if (cdns->pdata && cdns->pdata->platform_suspend)
- ret = cdns->pdata->platform_suspend(dev, suspend, wakeup);
+ return cdns->pdata->platform_suspend(cdns->dev, suspend, wakeup);
- return ret;
+ return 0;
}
-static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
+static int cdns3_controller_suspend(struct cdns *cdns, pm_message_t msg)
{
- struct cdns *cdns = dev_get_drvdata(dev);
+ struct device *dev = cdns->dev;
bool wakeup;
unsigned long flags;
@@ -242,7 +268,7 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
else
wakeup = device_may_wakeup(dev);
- cdns3_set_platform_suspend(cdns->dev, true, wakeup);
+ cdns3_set_platform_suspend(cdns, true, wakeup);
set_phy_power_off(cdns);
spin_lock_irqsave(&cdns->lock, flags);
cdns->in_lpm = true;
@@ -252,9 +278,8 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
return 0;
}
-static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
+static int cdns3_controller_resume(struct cdns *cdns, pm_message_t msg)
{
- struct cdns *cdns = dev_get_drvdata(dev);
int ret;
unsigned long flags;
@@ -277,7 +302,7 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
if (ret)
return ret;
- cdns3_set_platform_suspend(cdns->dev, false, false);
+ cdns3_set_platform_suspend(cdns, false, false);
spin_lock_irqsave(&cdns->lock, flags);
cdns_resume(cdns);
@@ -293,26 +318,37 @@ static int cdns3_controller_resume(struct device *dev, pm_message_t msg)
return ret;
}
-static int cdns3_plat_runtime_suspend(struct device *dev)
+int cdns3_runtime_suspend(struct cdns *cdns)
{
- return cdns3_controller_suspend(dev, PMSG_AUTO_SUSPEND);
+ return cdns3_controller_suspend(cdns, PMSG_AUTO_SUSPEND);
}
+EXPORT_SYMBOL_GPL(cdns3_runtime_suspend);
-static int cdns3_plat_runtime_resume(struct device *dev)
+int cdns3_runtime_resume(struct cdns *cdns)
{
- return cdns3_controller_resume(dev, PMSG_AUTO_RESUME);
+ return cdns3_controller_resume(cdns, PMSG_AUTO_RESUME);
}
+EXPORT_SYMBOL_GPL(cdns3_runtime_resume);
-#ifdef CONFIG_PM_SLEEP
+static int cdns3_dev_runtime_suspend(struct device *dev)
+{
+ return cdns3_runtime_suspend(dev_get_drvdata(dev));
+}
+
+static int cdns3_dev_runtime_resume(struct device *dev)
+{
+ return cdns3_runtime_resume(dev_get_drvdata(dev));
+}
-static int cdns3_plat_suspend(struct device *dev)
+#ifdef CONFIG_PM_SLEEP
+int cdns3_pm_suspend(struct cdns *cdns)
{
- struct cdns *cdns = dev_get_drvdata(dev);
+ struct device *dev = cdns->dev;
int ret;
cdns_suspend(cdns);
- ret = cdns3_controller_suspend(dev, PMSG_SUSPEND);
+ ret = cdns3_controller_suspend(cdns, PMSG_SUSPEND);
if (ret)
return ret;
@@ -321,18 +357,30 @@ static int cdns3_plat_suspend(struct device *dev)
return ret;
}
+EXPORT_SYMBOL_GPL(cdns3_pm_suspend);
+
+int cdns3_pm_resume(struct cdns *cdns)
+{
+ return cdns3_controller_resume(cdns, PMSG_RESUME);
+}
+EXPORT_SYMBOL_GPL(cdns3_pm_resume);
+
+static int cdns3_dev_pm_suspend(struct device *dev)
+{
+ return cdns3_pm_suspend(dev_get_drvdata(dev));
+}
-static int cdns3_plat_resume(struct device *dev)
+static int cdns3_dev_pm_resume(struct device *dev)
{
- return cdns3_controller_resume(dev, PMSG_RESUME);
+ return cdns3_pm_resume(dev_get_drvdata(dev));
}
#endif /* CONFIG_PM_SLEEP */
#endif /* CONFIG_PM */
static const struct dev_pm_ops cdns3_pm_ops = {
- SET_SYSTEM_SLEEP_PM_OPS(cdns3_plat_suspend, cdns3_plat_resume)
- SET_RUNTIME_PM_OPS(cdns3_plat_runtime_suspend,
- cdns3_plat_runtime_resume, NULL)
+ SET_SYSTEM_SLEEP_PM_OPS(cdns3_dev_pm_suspend, cdns3_dev_pm_resume)
+ SET_RUNTIME_PM_OPS(cdns3_dev_runtime_suspend,
+ cdns3_dev_runtime_resume, NULL)
};
#ifdef CONFIG_OF
diff --git a/drivers/usb/cdns3/glue.h b/drivers/usb/cdns3/glue.h
new file mode 100644
index 000000000000..67cd1073b555
--- /dev/null
+++ b/drivers/usb/cdns3/glue.h
@@ -0,0 +1,51 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * glue.h - Cadence USB3 DRD glue header
+ */
+
+#ifndef __DRIVERS_USB_CDNS3_GLUE_H
+#define __DRIVERS_USB_CDNS3_GLUE_H
+
+#include <linux/types.h>
+
+#include "core.h"
+
+struct platform_device;
+
+/**
+ * struct cdns3_probe_data - Parameters passed to cdns3_core_probe()
+ * @cdns: Cadence DRD controller context (allocated by the glue driver)
+ * @pdev: Platform device for resources and IRQs
+ */
+struct cdns3_probe_data {
+ struct cdns *cdns;
+ struct platform_device *pdev;
+};
+
+/**
+ * cdns3_core_probe - Initialize the Cadence USB3 platform core
+ * @data: Controller context and platform device supplied by the glue layer
+ *
+ * Performs resource mapping, PHY setup, cdns_init(), role setup, and runtime PM
+ * configuration for the standard platform binding of the Cadence USB3/USBSSP DRD IP.
+ *
+ * Return: 0 on success, negative errno on failure
+ */
+int cdns3_core_probe(const struct cdns3_probe_data *data);
+
+/**
+ * cdns3_core_remove - Tear down the Cadence USB3 platform core
+ * @cdns: Controller context previously initialized by cdns3_core_probe()
+ */
+void cdns3_core_remove(struct cdns *cdns);
+
+/*
+ * The following callbacks are for glue drivers to invoke from their own
+ * &dev_pm_ops, so platform-specific work can wrap the shared controller logic.
+ */
+int cdns3_runtime_suspend(struct cdns *cdns);
+int cdns3_runtime_resume(struct cdns *cdns);
+int cdns3_pm_suspend(struct cdns *cdns);
+int cdns3_pm_resume(struct cdns *cdns);
+
+#endif /* __DRIVERS_USB_CDNS3_GLUE_H */
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library
2026-05-11 2:42 ` [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library Peter Chen
@ 2026-05-11 23:02 ` sashiko-bot
2026-05-12 7:03 ` Peter Chen
0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-05-11 23:02 UTC (permalink / raw)
To: Peter Chen; +Cc: robh, conor+dt, krzk+dt, devicetree
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Leaked usb2_phy initialization on usb3_phy acquisition failure.
- [High] Leaked role switch and DRD resources on core role initialization failure.
- [High] Unbalanced pm_runtime_forbid() call permanently leaks the runtime PM usage counter across bind/unbind cycles.
- [High] Hardware PHY is powered off without calling synchronize_irq() during runtime suspend.
- [Medium] Exported PM helpers in glue.h lack static inline stubs for disabled PM configurations.
--
commit a327903170c6c0082aaebdb421558ca806f906da
Author: Peter Chen <peter.chen@cixtech.com>
usb: cdns3: plat: Expose platform core driver as library
This commit splits the Cadence USB3 platform probe, remove, and PM paths into
exported core functions. It also adds a new header glue.h to document the
probe data structure and public entry points, allowing SoC glue code to
instantiate the core deterministically.
> diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> index 3fe3109a3688a..2219cbff1c592 100644
> --- a/drivers/usb/cdns3/cdns3-plat.c
> +++ b/drivers/usb/cdns3/cdns3-plat.c
[ ... ]
> @@ -59,29 +60,21 @@ static int cdns3_plat_host_init(struct cdns *cdns)
> }
>
> /**
> - * cdns3_plat_probe - probe for cdns3 core device
> - * @pdev: Pointer to cdns3 core platform device
> + * cdns3_core_probe - Initialize the Cadence USB3 platform core
> + * @data: Controller context and platform device supplied by the glue layer
> *
> * Returns 0 on success otherwise negative errno
> */
> -static int cdns3_plat_probe(struct platform_device *pdev)
> +int cdns3_core_probe(const struct cdns3_probe_data *data)
> {
> + struct platform_device *pdev = data->pdev;
> struct device *dev = &pdev->dev;
> - struct resource *res;
> - struct cdns *cdns;
> + struct cdns *cdns = data->cdns;
> + struct resource *res;
> void __iomem *regs;
> int ret;
>
> - cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> - if (!cdns)
> - return -ENOMEM;
> -
> - cdns->dev = dev;
> - cdns->pdata = dev_get_platdata(dev);
> - if (cdns->pdata && cdns->pdata->override_apb_timeout)
> - cdns->override_apb_timeout = cdns->pdata->override_apb_timeout;
> -
> - platform_set_drvdata(pdev, cdns);
> + dev_set_drvdata(dev, cdns);
>
> ret = platform_get_irq_byname(pdev, "host");
Further down in this function, if devm_phy_optional_get() for cdns->usb3_phy
fails (e.g. with -EPROBE_DEFER), it returns directly via dev_err_probe(),
bypassing the error cleanup path in err_phy3_init.
Does this leak the usb2_phy initialization since phy_exit(cdns->usb2_phy) is
never called?
If probe is deferred and retried, will this repeatedly increment the PHY's
init_count, preventing it from ever powering down?
Also in this function, after cdns_init(cdns) successfully registers resources
like a role switch, if cdns_core_init_role(cdns) fails, the error path
jumps to err_cdns_init:
err_cdns_init:
set_phy_power_off(cdns);
err_phy_power_on:
...
This error path only powers off the PHYs. Can this permanently leak the role
switch and DRD resources by omitting a call to cdns_remove(cdns) or
usb_role_switch_unregister()?
Towards the end of this function, pm_runtime_forbid(dev) is conditionally
called to increase the device's runtime PM usage count:
if (!(cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW)))
pm_runtime_forbid(dev);
Should there be a corresponding pm_runtime_allow(dev) in the remove path to
balance this count?
[ ... ]
> @@ -195,14 +188,41 @@ static int cdns3_plat_probe(struct platform_device *pdev)
[ ... ]
> +/**
> + * cdns3_core_remove - Tear down the Cadence USB3 platform core
> + * @cdns: Controller context previously initialized by cdns3_core_probe()
> + */
> +void cdns3_core_remove(struct cdns *cdns)
> {
> - struct cdns *cdns = platform_get_drvdata(pdev);
> struct device *dev = cdns->dev;
>
> pm_runtime_get_sync(dev);
> @@ -213,24 +233,30 @@ static void cdns3_plat_remove(struct platform_device *pdev)
> phy_exit(cdns->usb2_phy);
> phy_exit(cdns->usb3_phy);
> }
> +EXPORT_SYMBOL_GPL(cdns3_core_remove);
Since pm_runtime_forbid(dev) was called in the probe function, could the
absence of pm_runtime_allow(dev) here permanently leak the runtime PM usage
counter across bind/unbind cycles?
If the driver is repeatedly unbound and bound, will the usage counter
continuously increment, eventually preventing the device from entering a
runtime suspended state?
[ ... ]
> @@ -242,7 +268,7 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
> else
> wakeup = device_may_wakeup(dev);
>
> - cdns3_set_platform_suspend(cdns->dev, true, wakeup);
> + cdns3_set_platform_suspend(cdns, true, wakeup);
> set_phy_power_off(cdns);
> spin_lock_irqsave(&cdns->lock, flags);
The controller uses shared IRQs, such as IRQF_SHARED for cdns->wakeup_irq.
The PM subsystem guidelines require drivers using shared IRQs to call
synchronize_irq() before powering down hardware.
Might calling set_phy_power_off(cdns) here before synchronize_irq() risk
hardware PHY being powered off while an interrupt handler is executing
concurrently on another CPU?
[ ... ]
> diff --git a/drivers/usb/cdns3/glue.h b/drivers/usb/cdns3/glue.h
> new file mode 100644
> index 0000000000000..67cd1073b5551
> --- /dev/null
> +++ b/drivers/usb/cdns3/glue.h
> @@ -0,0 +1,51 @@
[ ... ]
> +/*
> + * The following callbacks are for glue drivers to invoke from their own
> + * &dev_pm_ops, so platform-specific work can wrap the shared controller logic.
> + */
> +int cdns3_runtime_suspend(struct cdns *cdns);
> +int cdns3_runtime_resume(struct cdns *cdns);
> +int cdns3_pm_suspend(struct cdns *cdns);
> +int cdns3_pm_resume(struct cdns *cdns);
> +
> +#endif /* __DRIVERS_USB_CDNS3_GLUE_H */
These functions are conditionally compiled in cdns3-plat.c depending on
CONFIG_PM and CONFIG_PM_SLEEP. However, they are unconditionally declared
here without empty static inline fallback stubs.
If a glue driver uses __maybe_unused annotations instead of #ifdef CONFIG_PM
for its own callbacks, will the compiler still attempt to resolve these calls
and produce undefined reference linker errors when PM is disabled?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511024244.981941-1-peter.chen@cixtech.com?part=1
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library
2026-05-11 23:02 ` sashiko-bot
@ 2026-05-12 7:03 ` Peter Chen
0 siblings, 0 replies; 11+ messages in thread
From: Peter Chen @ 2026-05-12 7:03 UTC (permalink / raw)
To: sashiko; +Cc: robh, conor+dt, krzk+dt, devicetree
On 26-05-11 23:02:32, sashiko-bot@kernel.org wrote:
> EXTERNAL EMAIL
>
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> - [High] Leaked usb2_phy initialization on usb3_phy acquisition failure.
> - [High] Leaked role switch and DRD resources on core role initialization failure.
> - [High] Unbalanced pm_runtime_forbid() call permanently leaks the runtime PM usage counter across bind/unbind cycles.
> - [High] Hardware PHY is powered off without calling synchronize_irq() during runtime suspend.
> - [Medium] Exported PM helpers in glue.h lack static inline stubs for disabled PM configurations.
> --
>
> commit a327903170c6c0082aaebdb421558ca806f906da
> Author: Peter Chen <peter.chen@cixtech.com>
>
> usb: cdns3: plat: Expose platform core driver as library
>
> This commit splits the Cadence USB3 platform probe, remove, and PM paths into
> exported core functions. It also adds a new header glue.h to document the
> probe data structure and public entry points, allowing SoC glue code to
> instantiate the core deterministically.
>
> > diff --git a/drivers/usb/cdns3/cdns3-plat.c b/drivers/usb/cdns3/cdns3-plat.c
> > index 3fe3109a3688a..2219cbff1c592 100644
> > --- a/drivers/usb/cdns3/cdns3-plat.c
> > +++ b/drivers/usb/cdns3/cdns3-plat.c
> [ ... ]
> > @@ -59,29 +60,21 @@ static int cdns3_plat_host_init(struct cdns *cdns)
> > }
> >
> > /**
> > - * cdns3_plat_probe - probe for cdns3 core device
> > - * @pdev: Pointer to cdns3 core platform device
> > + * cdns3_core_probe - Initialize the Cadence USB3 platform core
> > + * @data: Controller context and platform device supplied by the glue layer
> > *
> > * Returns 0 on success otherwise negative errno
> > */
> > -static int cdns3_plat_probe(struct platform_device *pdev)
> > +int cdns3_core_probe(const struct cdns3_probe_data *data)
> > {
> > + struct platform_device *pdev = data->pdev;
> > struct device *dev = &pdev->dev;
> > - struct resource *res;
> > - struct cdns *cdns;
> > + struct cdns *cdns = data->cdns;
> > + struct resource *res;
> > void __iomem *regs;
> > int ret;
> >
> > - cdns = devm_kzalloc(dev, sizeof(*cdns), GFP_KERNEL);
> > - if (!cdns)
> > - return -ENOMEM;
> > -
> > - cdns->dev = dev;
> > - cdns->pdata = dev_get_platdata(dev);
> > - if (cdns->pdata && cdns->pdata->override_apb_timeout)
> > - cdns->override_apb_timeout = cdns->pdata->override_apb_timeout;
> > -
> > - platform_set_drvdata(pdev, cdns);
> > + dev_set_drvdata(dev, cdns);
> >
> > ret = platform_get_irq_byname(pdev, "host");
>
> Further down in this function, if devm_phy_optional_get() for cdns->usb3_phy
> fails (e.g. with -EPROBE_DEFER), it returns directly via dev_err_probe(),
> bypassing the error cleanup path in err_phy3_init.
>
> Does this leak the usb2_phy initialization since phy_exit(cdns->usb2_phy) is
> never called?
>
> If probe is deferred and retried, will this repeatedly increment the PHY's
> init_count, preventing it from ever powering down?
>
> Also in this function, after cdns_init(cdns) successfully registers resources
> like a role switch, if cdns_core_init_role(cdns) fails, the error path
> jumps to err_cdns_init:
>
> err_cdns_init:
> set_phy_power_off(cdns);
> err_phy_power_on:
> ...
>
> This error path only powers off the PHYs. Can this permanently leak the role
> switch and DRD resources by omitting a call to cdns_remove(cdns) or
> usb_role_switch_unregister()?
Ack.
It is the issue current code exists, I will fix it first.
>
> Towards the end of this function, pm_runtime_forbid(dev) is conditionally
> called to increase the device's runtime PM usage count:
>
> if (!(cdns->pdata && (cdns->pdata->quirks & CDNS3_DEFAULT_PM_RUNTIME_ALLOW)))
> pm_runtime_forbid(dev);
>
> Should there be a corresponding pm_runtime_allow(dev) in the remove path to
> balance this count?
Ack.
>
> [ ... ]
> > @@ -195,14 +188,41 @@ static int cdns3_plat_probe(struct platform_device *pdev)
> [ ... ]
> > +/**
> > + * cdns3_core_remove - Tear down the Cadence USB3 platform core
> > + * @cdns: Controller context previously initialized by cdns3_core_probe()
> > + */
> > +void cdns3_core_remove(struct cdns *cdns)
> > {
> > - struct cdns *cdns = platform_get_drvdata(pdev);
> > struct device *dev = cdns->dev;
> >
> > pm_runtime_get_sync(dev);
> > @@ -213,24 +233,30 @@ static void cdns3_plat_remove(struct platform_device *pdev)
> > phy_exit(cdns->usb2_phy);
> > phy_exit(cdns->usb3_phy);
> > }
> > +EXPORT_SYMBOL_GPL(cdns3_core_remove);
>
> Since pm_runtime_forbid(dev) was called in the probe function, could the
> absence of pm_runtime_allow(dev) here permanently leak the runtime PM usage
> counter across bind/unbind cycles?
>
> If the driver is repeatedly unbound and bound, will the usage counter
> continuously increment, eventually preventing the device from entering a
> runtime suspended state?
The same comments for above issue.
>
> [ ... ]
> > @@ -242,7 +268,7 @@ static int cdns3_controller_suspend(struct device *dev, pm_message_t msg)
> > else
> > wakeup = device_may_wakeup(dev);
> >
> > - cdns3_set_platform_suspend(cdns->dev, true, wakeup);
> > + cdns3_set_platform_suspend(cdns, true, wakeup);
> > set_phy_power_off(cdns);
> > spin_lock_irqsave(&cdns->lock, flags);
>
> The controller uses shared IRQs, such as IRQF_SHARED for cdns->wakeup_irq.
> The PM subsystem guidelines require drivers using shared IRQs to call
> synchronize_irq() before powering down hardware.
>
> Might calling set_phy_power_off(cdns) here before synchronize_irq() risk
> hardware PHY being powered off while an interrupt handler is executing
> concurrently on another CPU?
Ack.
Will call synchronize_irq() at the beginning of function cdns3_controller_suspend.
>
> [ ... ]
> > diff --git a/drivers/usb/cdns3/glue.h b/drivers/usb/cdns3/glue.h
> > new file mode 100644
> > index 0000000000000..67cd1073b5551
> > --- /dev/null
> > +++ b/drivers/usb/cdns3/glue.h
> > @@ -0,0 +1,51 @@
> [ ... ]
> > +/*
> > + * The following callbacks are for glue drivers to invoke from their own
> > + * &dev_pm_ops, so platform-specific work can wrap the shared controller logic.
> > + */
> > +int cdns3_runtime_suspend(struct cdns *cdns);
> > +int cdns3_runtime_resume(struct cdns *cdns);
> > +int cdns3_pm_suspend(struct cdns *cdns);
> > +int cdns3_pm_resume(struct cdns *cdns);
> > +
> > +#endif /* __DRIVERS_USB_CDNS3_GLUE_H */
>
> These functions are conditionally compiled in cdns3-plat.c depending on
> CONFIG_PM and CONFIG_PM_SLEEP. However, they are unconditionally declared
> here without empty static inline fallback stubs.
>
> If a glue driver uses __maybe_unused annotations instead of #ifdef CONFIG_PM
> for its own callbacks, will the compiler still attempt to resolve these calls
> and produce undefined reference linker errors when PM is disabled?
>
Ack
The PM helper declarations in glue.h should indeed be made robust for glue
drivers using __maybe_unused PM callbacks. The current Sky1 glue driver uses
CONFIG_PM/CONFIG_PM_SLEEP guards, but the public glue header should not rely
on every user doing that. I will add static inline stubs for the disabled PM
configurations in v2.
--
Best regards,
Peter
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver
2026-05-11 2:42 [PATCH 0/4] Add CIX Sky1 Cadence USB3 support Peter Chen
2026-05-11 2:42 ` [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library Peter Chen
@ 2026-05-11 2:42 ` Peter Chen
2026-05-11 23:17 ` sashiko-bot
2026-05-11 2:42 ` [PATCH 3/4] dt-bindings: usb: add CIX Sky1 Cadence USB3 controller Peter Chen
2026-05-11 2:42 ` [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers Peter Chen
3 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2026-05-11 2:42 UTC (permalink / raw)
To: robh, krzk+dt, conor+dt, gregkh, pawell, rogerq
Cc: devicetree, linux-kernel, linux-usb, cix-kernel-upstream,
linux-arm-kernel, arnd, Peter Chen
Add a CIX sky1 platform glue driver with Kconfig and Makefile entry.
It calls APIs exported from cdns3-plat.c for probe/remote/suspend/resume
routines.
Signed-off-by: Peter Chen <peter.chen@cixtech.com>
---
drivers/usb/cdns3/Kconfig | 13 ++
drivers/usb/cdns3/Makefile | 1 +
drivers/usb/cdns3/cdnsp-sky1.c | 252 +++++++++++++++++++++++++++++++++
3 files changed, 266 insertions(+)
create mode 100644 drivers/usb/cdns3/cdnsp-sky1.c
diff --git a/drivers/usb/cdns3/Kconfig b/drivers/usb/cdns3/Kconfig
index 39ad23d1ada8..7d7c322ea865 100644
--- a/drivers/usb/cdns3/Kconfig
+++ b/drivers/usb/cdns3/Kconfig
@@ -110,6 +110,19 @@ config USB_CDNS3_STARFIVE
If you choose to build this driver as module it will
be dynamically linked and module will be called cdns3-starfive.ko
+config USB_CDNSP_SKY1
+ tristate "Cadence USB3 support on CIX Sky1 SoC platforms"
+ depends on USB_CDNS3
+ depends on ARCH_CIX || COMPILE_TEST
+ default USB_CDNS3
+ help
+ Glue driver for the Cadence USB dual-role controllers on CIX Sky1
+ (device tree compatible cix,sky1-usb3). It enables clocks and resets
+ from the SoC, then uses the shared cdns3 platform core (cdns.ko).
+
+ If built as a module, the module is named cdnsp-sky1.ko and must be
+ loaded after the cdns core module when both are loadable modules.
+
endif # USB_CDNS3
endif # USB_CDNS_SUPPORT
diff --git a/drivers/usb/cdns3/Makefile b/drivers/usb/cdns3/Makefile
index b2e4ba6a49a3..ab813aaf9940 100644
--- a/drivers/usb/cdns3/Makefile
+++ b/drivers/usb/cdns3/Makefile
@@ -28,3 +28,4 @@ obj-$(CONFIG_USB_CDNSP_PCI) += cdnsp-pci.o
obj-$(CONFIG_USB_CDNS3_TI) += cdns3-ti.o
obj-$(CONFIG_USB_CDNS3_IMX) += cdns3-imx.o
obj-$(CONFIG_USB_CDNS3_STARFIVE) += cdns3-starfive.o
+obj-$(CONFIG_USB_CDNSP_SKY1) += cdnsp-sky1.o
diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
new file mode 100644
index 000000000000..049044e3d09b
--- /dev/null
+++ b/drivers/usb/cdns3/cdnsp-sky1.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * cdnsp-sky1.c - CIX Sky1 glue for Cadence USBSSP DRD controller
+ *
+ * Copyright (C) 2026 CIX Technology Group Co., Ltd.
+ */
+
+#include <linux/bits.h>
+#include <linux/clk.h>
+#include <linux/io.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/pm.h>
+#include <linux/regmap.h>
+#include <linux/reset.h>
+
+#include "glue.h"
+
+#define AXI_SETTING_OFFSET 0x0
+/* Normal Non-cacheable Bufferable */
+#define SKY1_USB_AXI_WR_CACHE_VALUE 0x33
+
+/* USB mode strap in S5 syscon */
+#define USB_MODE_STRAP_S5_DOMAIN 0x424
+#define MODE_STRAP_OTG 0
+
+#define U3_TYPEC_DRD_ID 0
+#define U3_TYPEC_HOST0_ID 1
+#define U3_TYPEC_HOST1_ID 2
+#define U3_TYPEC_HOST2_ID 3
+#define U3_TYPEA_CTRL0_ID 4
+#define U3_TYPEA_CTRL1_ID 5
+#define U2_HOST0_ID 6
+#define U2_HOST1_ID 7
+#define U2_HOST2_ID 8
+#define U2_HOST3_ID 9
+#define SKY1_USB_S5_NUM 10
+
+#define U3_TYPEC_DRD_MODE_STRAP_BIT 12
+#define U3_TYPEC_HOST0_MODE_STRAP_BIT 14
+#define U3_TYPEC_HOST1_MODE_STRAP_BIT 16
+#define U3_TYPEC_HOST2_MODE_STRAP_BIT 18
+#define U3_TYPEA_CTRL0_MODE_STRAP_BIT 8
+#define U3_TYPEA_CTRL1_MODE_STRAP_BIT 10
+#define U2_HOST0_MODE_STRAP_BIT 0
+#define U2_HOST1_MODE_STRAP_BIT 2
+#define U2_HOST2_MODE_STRAP_BIT 4
+#define U2_HOST3_MODE_STRAP_BIT 6
+
+struct cdnsp_sky1_strap_signal {
+ unsigned int offset, bit;
+};
+
+static const struct cdnsp_sky1_strap_signal strap_signals[SKY1_USB_S5_NUM] = {
+ [U3_TYPEC_DRD_ID] = { USB_MODE_STRAP_S5_DOMAIN, U3_TYPEC_DRD_MODE_STRAP_BIT },
+ [U3_TYPEC_HOST0_ID] = { USB_MODE_STRAP_S5_DOMAIN, U3_TYPEC_HOST0_MODE_STRAP_BIT },
+ [U3_TYPEC_HOST1_ID] = { USB_MODE_STRAP_S5_DOMAIN, U3_TYPEC_HOST1_MODE_STRAP_BIT },
+ [U3_TYPEC_HOST2_ID] = { USB_MODE_STRAP_S5_DOMAIN, U3_TYPEC_HOST2_MODE_STRAP_BIT },
+ [U3_TYPEA_CTRL0_ID] = { USB_MODE_STRAP_S5_DOMAIN, U3_TYPEA_CTRL0_MODE_STRAP_BIT },
+ [U3_TYPEA_CTRL1_ID] = { USB_MODE_STRAP_S5_DOMAIN, U3_TYPEA_CTRL1_MODE_STRAP_BIT },
+ [U2_HOST0_ID] = { USB_MODE_STRAP_S5_DOMAIN, U2_HOST0_MODE_STRAP_BIT },
+ [U2_HOST1_ID] = { USB_MODE_STRAP_S5_DOMAIN, U2_HOST1_MODE_STRAP_BIT },
+ [U2_HOST2_ID] = { USB_MODE_STRAP_S5_DOMAIN, U2_HOST2_MODE_STRAP_BIT },
+ [U2_HOST3_ID] = { USB_MODE_STRAP_S5_DOMAIN, U2_HOST3_MODE_STRAP_BIT },
+};
+
+struct cdnsp_sky1 {
+ struct device *dev;
+ struct cdns cdns;
+ struct regmap *usb_syscon;
+ void __iomem *glue_base;
+ struct clk_bulk_data *clks;
+ int num_clks;
+};
+
+/**
+ * sky1_set_mode_by_id - program one USB controller mode strap
+ * @syscon: regmap for S5 syscon (from DT property cix,syscon-usb)
+ * @id: controller slot ID (U3_TYPEC_DRD_ID .. U2_HOST3_ID)
+ * @mode: MODE_STRAP_OTG, MODE_STRAP_HOST, or MODE_STRAP_DEVICE
+ */
+static int cdnsp_sky1_set_mode_by_id(struct regmap *syscon, int id, int mode)
+{
+ if (id < 0 || id >= SKY1_USB_S5_NUM)
+ return -EINVAL;
+
+ return regmap_update_bits(syscon,
+ strap_signals[id].offset,
+ GENMASK(strap_signals[id].bit + 1,
+ strap_signals[id].bit),
+ (unsigned int)mode << strap_signals[id].bit);
+}
+
+static int cdnsp_sky1_set_all_controllers_otg(struct regmap *syscon)
+{
+ int id, ret;
+
+ for (id = 0; id < SKY1_USB_S5_NUM; id++) {
+ ret = cdnsp_sky1_set_mode_by_id(syscon, id, MODE_STRAP_OTG);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static const struct clk_bulk_data cdnsp_sky1_cdns_core_clks[] = {
+ { .id = "sof" },
+ { .id = "aclk" },
+ { .id = "lpm" },
+ { .id = "pclk" },
+};
+
+static int cdnsp_sky1_probe(struct platform_device *pdev)
+{
+ struct device *dev = &pdev->dev;
+ struct cdnsp_sky1 *priv;
+ struct cdns *cdns;
+ struct cdns3_probe_data probe_data;
+ struct resource *res;
+ int ret;
+
+ priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+ if (!priv)
+ return -ENOMEM;
+
+ priv->dev = dev;
+ priv->num_clks = ARRAY_SIZE(cdnsp_sky1_cdns_core_clks);
+ priv->clks = devm_kmemdup(dev, cdnsp_sky1_cdns_core_clks,
+ sizeof(cdnsp_sky1_cdns_core_clks), GFP_KERNEL);
+ if (!priv->clks)
+ return -ENOMEM;
+
+ ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to get clocks\n");
+
+ ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
+ if (ret)
+ return dev_err_probe(dev, ret, "failed to enable clocks\n");
+
+ priv->usb_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
+ "cix,syscon-usb");
+ if (IS_ERR(priv->usb_syscon))
+ return dev_err_probe(dev, PTR_ERR(priv->usb_syscon),
+ "failed to get cix,syscon-usb regmap\n");
+
+ ret = cdnsp_sky1_set_all_controllers_otg(priv->usb_syscon);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "failed to set USB controllers to OTG strap\n");
+
+ res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "glue");
+ if (!res)
+ goto err_clk;
+
+ priv->glue_base = devm_ioremap_resource(dev, res);
+ if (IS_ERR(priv->glue_base)) {
+ ret = PTR_ERR(priv->glue_base);
+ goto err_clk;
+ }
+
+ /* Set ARCACHE and AWCACHE */
+ writel(SKY1_USB_AXI_WR_CACHE_VALUE, priv->glue_base + AXI_SETTING_OFFSET);
+
+ cdns = &priv->cdns;
+ cdns->dev = dev;
+
+ probe_data.cdns = cdns;
+ probe_data.pdev = pdev;
+
+ ret = cdns3_core_probe(&probe_data);
+ if (ret)
+ goto err_clk;
+
+ return 0;
+
+err_clk:
+ clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+
+ return ret;
+}
+
+static void cdnsp_sky1_remove(struct platform_device *pdev)
+{
+ struct cdns *cdns = platform_get_drvdata(pdev);
+ struct cdnsp_sky1 *priv;
+
+ if (!cdns)
+ return;
+
+ cdns3_core_remove(cdns);
+ priv = container_of(cdns, struct cdnsp_sky1, cdns);
+ clk_bulk_disable_unprepare(priv->num_clks, priv->clks);
+}
+
+#ifdef CONFIG_PM
+static int cdnsp_sky1_runtime_suspend(struct device *dev)
+{
+ return cdns3_runtime_suspend(dev_get_drvdata(dev));
+}
+
+static int cdnsp_sky1_runtime_resume(struct device *dev)
+{
+ return cdns3_runtime_resume(dev_get_drvdata(dev));
+}
+
+#ifdef CONFIG_PM_SLEEP
+static int cdnsp_sky1_suspend(struct device *dev)
+{
+ return cdns3_pm_suspend(dev_get_drvdata(dev));
+}
+
+static int cdnsp_sky1_resume(struct device *dev)
+{
+ return cdns3_pm_resume(dev_get_drvdata(dev));
+}
+#endif /* CONFIG_PM_SLEEP */
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops cdnsp_sky1_pm_ops = {
+ SET_SYSTEM_SLEEP_PM_OPS(cdnsp_sky1_suspend, cdnsp_sky1_resume)
+ SET_RUNTIME_PM_OPS(cdnsp_sky1_runtime_suspend,
+ cdnsp_sky1_runtime_resume, NULL)
+};
+
+static const struct of_device_id cdnsp_sky1_of_match[] = {
+ { .compatible = "cix,sky1-usb3" },
+ { }
+};
+MODULE_DEVICE_TABLE(of, cdnsp_sky1_of_match);
+
+static struct platform_driver cdnsp_sky1_driver = {
+ .probe = cdnsp_sky1_probe,
+ .remove = cdnsp_sky1_remove,
+ .driver = {
+ .name = "cdnsp-sky1",
+ .of_match_table = of_match_ptr(cdnsp_sky1_of_match),
+ .pm = &cdnsp_sky1_pm_ops,
+ },
+};
+
+module_platform_driver(cdnsp_sky1_driver);
+
+MODULE_SOFTDEP("pre: cdns");
+MODULE_ALIAS("platform:cdnsp-sky1");
+MODULE_DESCRIPTION("CIX Sky1 Cadence USBSSP DRD glue driver");
+MODULE_AUTHOR("Peter Chen <peter.chen@cixtech.com>");
+MODULE_LICENSE("GPL");
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver
2026-05-11 2:42 ` [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver Peter Chen
@ 2026-05-11 23:17 ` sashiko-bot
2026-05-12 7:43 ` Peter Chen
0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-05-11 23:17 UTC (permalink / raw)
To: Peter Chen; +Cc: devicetree, conor+dt, robh, krzk+dt
Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Probe returns success (0) when memory resource retrieval fails, leading to a NULL pointer dereference.
- [High] Probe forcefully overwrites the mode strap configuration for all USB controllers on the SoC.
- [Medium] Resource leak of clocks on early probe error paths.
- [Medium] Unconditional assignment of dev_pm_ops violates Power Management Subsystem rules.
- [Medium] Commit message and implementation mismatch regarding hardware resets.
- [Low] Using of_match_ptr() with a statically allocated array causes compiler warnings.
--
commit a626bc93f86adbca6a4dce0b669265133a32a8cb
Author: Peter Chen <peter.chen@cixtech.com>
usb: cdns3: sky1: Add cdnsp-sky1 glue driver
Add a CIX sky1 platform glue driver with Kconfig and Makefile entry.
It calls APIs exported from cdns3-plat.c for probe/remote/suspend/resume
routines.
> Glue driver for the Cadence USB dual-role controllers on CIX Sky1
> (device tree compatible cix,sky1-usb3). It enables clocks and resets
> from the SoC, then uses the shared cdns3 platform core (cdns.ko).
The commit message mentions it enables clocks and resets from the SoC, but is
there missing code to handle the resets? The implementation includes linux/reset.h
but doesn't appear to request or interact with any reset lines.
> diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> new file mode 100644
> index 0000000000000..049044e3d09bd
> --- /dev/null
> +++ b/drivers/usb/cdns3/cdnsp-sky1.c
[ ... ]
> +static int cdnsp_sky1_set_all_controllers_otg(struct regmap *syscon)
> +{
> + int id, ret;
> +
> + for (id = 0; id < SKY1_USB_S5_NUM; id++) {
> + ret = cdnsp_sky1_set_mode_by_id(syscon, id, MODE_STRAP_OTG);
> + if (ret)
> + return ret;
> + }
> +
> + return 0;
> +}
Does this code unintentionally alter the state of other independent USB
controllers? It appears to iterate through all controllers on the SoC and
updates them to MODE_STRAP_OTG, which might affect concurrent controllers
that are explicitly configured as host-only (like U2_HOST0_ID).
[ ... ]
> +static int cdnsp_sky1_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct cdnsp_sky1 *priv;
> + struct cdns *cdns;
> + struct cdns3_probe_data probe_data;
> + struct resource *res;
> + int ret;
> +
> + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> + if (!priv)
> + return -ENOMEM;
> +
> + priv->dev = dev;
> + priv->num_clks = ARRAY_SIZE(cdnsp_sky1_cdns_core_clks);
> + priv->clks = devm_kmemdup(dev, cdnsp_sky1_cdns_core_clks,
> + sizeof(cdnsp_sky1_cdns_core_clks), GFP_KERNEL);
> + if (!priv->clks)
> + return -ENOMEM;
> +
> + ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to get clocks\n");
> +
> + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> + if (ret)
> + return dev_err_probe(dev, ret, "failed to enable clocks\n");
> +
> + priv->usb_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> + "cix,syscon-usb");
> + if (IS_ERR(priv->usb_syscon))
> + return dev_err_probe(dev, PTR_ERR(priv->usb_syscon),
> + "failed to get cix,syscon-usb regmap\n");
> +
> + ret = cdnsp_sky1_set_all_controllers_otg(priv->usb_syscon);
> + if (ret)
> + return dev_err_probe(dev, ret,
> + "failed to set USB controllers to OTG strap\n");
Can these error paths cause a resource leak? The clocks were previously enabled
via clk_bulk_prepare_enable(), but these direct returns bypass the err_clk
label where clk_bulk_disable_unprepare() is called.
> +
> + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "glue");
> + if (!res)
> + goto err_clk;
If this resource retrieval fails, does the probe function return success?
The ret variable was previously set to 0 by the successful call to
cdnsp_sky1_set_all_controllers_otg(), so jumping to err_clk without updating
ret to an error code like -ENODEV would result in returning 0.
Could this lead to a NULL pointer dereference during subsequent power
management operations? Since the device would be considered bound without
calling cdns3_core_probe(), dev_get_drvdata() might return NULL in
callbacks like cdnsp_sky1_runtime_suspend().
[ ... ]
> +static const struct of_device_id cdnsp_sky1_of_match[] = {
> + { .compatible = "cix,sky1-usb3" },
> + { }
> +};
> +MODULE_DEVICE_TABLE(of, cdnsp_sky1_of_match);
> +
> +static struct platform_driver cdnsp_sky1_driver = {
> + .probe = cdnsp_sky1_probe,
> + .remove = cdnsp_sky1_remove,
> + .driver = {
> + .name = "cdnsp-sky1",
> + .of_match_table = of_match_ptr(cdnsp_sky1_of_match),
If the kernel is built with CONFIG_OF disabled, of_match_ptr() evaluates to
NULL. Since the cdnsp_sky1_of_match array is declared statically and without
__maybe_unused annotations, will this result in an unused variable compiler
warning?
> + .pm = &cdnsp_sky1_pm_ops,
Does unconditionally assigning the pm callback structure produce dead code when
CONFIG_PM is disabled? According to the power management subsystem guidelines,
this should likely be wrapped with the pm_ptr() macro so it evaluates to NULL
without PM support.
> + },
> +};
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511024244.981941-1-peter.chen@cixtech.com?part=2
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver
2026-05-11 23:17 ` sashiko-bot
@ 2026-05-12 7:43 ` Peter Chen
0 siblings, 0 replies; 11+ messages in thread
From: Peter Chen @ 2026-05-12 7:43 UTC (permalink / raw)
To: sashiko; +Cc: devicetree, conor+dt, robh, krzk+dt
On 26-05-11 23:17:34, sashiko-bot@kernel.org wrote:
> EXTERNAL EMAIL
>
> Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
> - [High] Probe returns success (0) when memory resource retrieval fails, leading to a NULL pointer dereference.
> - [High] Probe forcefully overwrites the mode strap configuration for all USB controllers on the SoC.
> - [Medium] Resource leak of clocks on early probe error paths.
> - [Medium] Unconditional assignment of dev_pm_ops violates Power Management Subsystem rules.
> - [Medium] Commit message and implementation mismatch regarding hardware resets.
> - [Low] Using of_match_ptr() with a statically allocated array causes compiler warnings.
> --
>
> commit a626bc93f86adbca6a4dce0b669265133a32a8cb
> Author: Peter Chen <peter.chen@cixtech.com>
>
> usb: cdns3: sky1: Add cdnsp-sky1 glue driver
>
> Add a CIX sky1 platform glue driver with Kconfig and Makefile entry.
> It calls APIs exported from cdns3-plat.c for probe/remote/suspend/resume
> routines.
>
> > Glue driver for the Cadence USB dual-role controllers on CIX Sky1
> > (device tree compatible cix,sky1-usb3). It enables clocks and resets
> > from the SoC, then uses the shared cdns3 platform core (cdns.ko).
>
> The commit message mentions it enables clocks and resets from the SoC, but is
> there missing code to handle the resets? The implementation includes linux/reset.h
> but doesn't appear to request or interact with any reset lines.
Sorry, I have a local version with PHY driver, it needs to have reset operation, I
forget to delete it at commit message.
>
> > diff --git a/drivers/usb/cdns3/cdnsp-sky1.c b/drivers/usb/cdns3/cdnsp-sky1.c
> > new file mode 100644
> > index 0000000000000..049044e3d09bd
> > --- /dev/null
> > +++ b/drivers/usb/cdns3/cdnsp-sky1.c
>
> [ ... ]
>
> > +static int cdnsp_sky1_set_all_controllers_otg(struct regmap *syscon)
> > +{
> > + int id, ret;
> > +
> > + for (id = 0; id < SKY1_USB_S5_NUM; id++) {
> > + ret = cdnsp_sky1_set_mode_by_id(syscon, id, MODE_STRAP_OTG);
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + return 0;
> > +}
>
> Does this code unintentionally alter the state of other independent USB
> controllers? It appears to iterate through all controllers on the SoC and
> updates them to MODE_STRAP_OTG, which might affect concurrent controllers
> that are explicitly configured as host-only (like U2_HOST0_ID).
All controllers for CIX have OTG hardware, it depends on OTG register to set
proper mode, so set all controllers DR mode as OTG by default.
>
> [ ... ]
>
> > +static int cdnsp_sky1_probe(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct cdnsp_sky1 *priv;
> > + struct cdns *cdns;
> > + struct cdns3_probe_data probe_data;
> > + struct resource *res;
> > + int ret;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + priv->dev = dev;
> > + priv->num_clks = ARRAY_SIZE(cdnsp_sky1_cdns_core_clks);
> > + priv->clks = devm_kmemdup(dev, cdnsp_sky1_cdns_core_clks,
> > + sizeof(cdnsp_sky1_cdns_core_clks), GFP_KERNEL);
> > + if (!priv->clks)
> > + return -ENOMEM;
> > +
> > + ret = devm_clk_bulk_get(dev, priv->num_clks, priv->clks);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to get clocks\n");
> > +
> > + ret = clk_bulk_prepare_enable(priv->num_clks, priv->clks);
> > + if (ret)
> > + return dev_err_probe(dev, ret, "failed to enable clocks\n");
> > +
> > + priv->usb_syscon = syscon_regmap_lookup_by_phandle(dev->of_node,
> > + "cix,syscon-usb");
> > + if (IS_ERR(priv->usb_syscon))
> > + return dev_err_probe(dev, PTR_ERR(priv->usb_syscon),
> > + "failed to get cix,syscon-usb regmap\n");
> > +
> > + ret = cdnsp_sky1_set_all_controllers_otg(priv->usb_syscon);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "failed to set USB controllers to OTG strap\n");
>
> Can these error paths cause a resource leak? The clocks were previously enabled
> via clk_bulk_prepare_enable(), but these direct returns bypass the err_clk
> label where clk_bulk_disable_unprepare() is called.
Will fix at v2.
>
> > +
> > + res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "glue");
> > + if (!res)
> > + goto err_clk;
>
> If this resource retrieval fails, does the probe function return success?
>
No, the "glue" region is mandatory.
> The ret variable was previously set to 0 by the successful call to
> cdnsp_sky1_set_all_controllers_otg(), so jumping to err_clk without updating
> ret to an error code like -ENODEV would result in returning 0.
>
> Could this lead to a NULL pointer dereference during subsequent power
> management operations? Since the device would be considered bound without
> calling cdns3_core_probe(), dev_get_drvdata() might return NULL in
> callbacks like cdnsp_sky1_runtime_suspend().
NAK. Other values except for NULL is correct for platform_get_resource_byname.
>
> [ ... ]
>
> > +static const struct of_device_id cdnsp_sky1_of_match[] = {
> > + { .compatible = "cix,sky1-usb3" },
> > + { }
> > +};
> > +MODULE_DEVICE_TABLE(of, cdnsp_sky1_of_match);
> > +
> > +static struct platform_driver cdnsp_sky1_driver = {
> > + .probe = cdnsp_sky1_probe,
> > + .remove = cdnsp_sky1_remove,
> > + .driver = {
> > + .name = "cdnsp-sky1",
> > + .of_match_table = of_match_ptr(cdnsp_sky1_of_match),
>
> If the kernel is built with CONFIG_OF disabled, of_match_ptr() evaluates to
> NULL. Since the cdnsp_sky1_of_match array is declared statically and without
> __maybe_unused annotations, will this result in an unused variable compiler
> warning?
Will fix it by adding CONFIG_OF for cdnsp_sky1_of_match.
>
> > + .pm = &cdnsp_sky1_pm_ops,
>
> Does unconditionally assigning the pm callback structure produce dead code when
> CONFIG_PM is disabled? According to the power management subsystem guidelines,
> this should likely be wrapped with the pm_ptr() macro so it evaluates to NULL
> without PM support.
Will use pm_ptr for it.
--
Best regards,
Peter
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH 3/4] dt-bindings: usb: add CIX Sky1 Cadence USB3 controller
2026-05-11 2:42 [PATCH 0/4] Add CIX Sky1 Cadence USB3 support Peter Chen
2026-05-11 2:42 ` [PATCH 1/4] usb: cdns3: plat: Expose platform core driver as library Peter Chen
2026-05-11 2:42 ` [PATCH 2/4] usb: cdns3: sky1: Add cdnsp-sky1 glue driver Peter Chen
@ 2026-05-11 2:42 ` Peter Chen
2026-05-11 2:42 ` [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers Peter Chen
3 siblings, 0 replies; 11+ messages in thread
From: Peter Chen @ 2026-05-11 2:42 UTC (permalink / raw)
To: robh, krzk+dt, conor+dt, gregkh, pawell, rogerq
Cc: devicetree, linux-kernel, linux-usb, cix-kernel-upstream,
linux-arm-kernel, arnd, Peter Chen
Add a binding for the CIX Sky1 integration of the Cadence USBSSP DRD
controller. The schema documents the glue register window, clocks,
resets, interrupts and S5 system controller phandle.
Signed-off-by: Peter Chen <peter.chen@cixtech.com>
---
.../bindings/usb/cix,sky1-cdns3.yaml | 151 ++++++++++++++++++
1 file changed, 151 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml
diff --git a/Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml b/Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml
new file mode 100644
index 000000000000..23d82d8cc9bc
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/cix,sky1-cdns3.yaml
@@ -0,0 +1,151 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/usb/cix,sky1-cdns3.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: CIX Sky1 Cadence USB3 Controller
+
+maintainers:
+ - Peter Chen <peter.chen@cixtech.com>
+
+description:
+ The CIX Sky1 USB3 controller is based on the Cadence USBSSP DRD
+ controller. The integration adds glue registers and mode strap controls
+ in the Sky1 S5 system controller.
+
+allOf:
+ - $ref: usb-drd.yaml#
+ - $ref: usb-xhci.yaml#
+
+properties:
+ compatible:
+ items:
+ - const: cix,sky1-usb3
+ - const: cix,cdns-usb3
+
+ reg:
+ items:
+ - description: OTG controller registers
+ - description: Device controller registers
+ - description: XHCI host controller registers
+ - description: Sky1 USB glue registers
+
+ reg-names:
+ items:
+ - const: otg
+ - const: dev
+ - const: xhci
+ - const: glue
+
+ interrupts:
+ items:
+ - description: XHCI host controller interrupt
+ - description: Device controller interrupt
+ - description: OTG/DRD controller interrupt
+ - description: Wakeup interrupt
+
+ interrupt-names:
+ items:
+ - const: host
+ - const: peripheral
+ - const: otg
+ - const: wakeup
+
+ clocks:
+ items:
+ - description: Start-of-frame clock
+ - description: AXI bus clock
+ - description: Low-power mode clock
+ - description: APB register interface clock
+
+ clock-names:
+ items:
+ - const: sof
+ - const: aclk
+ - const: lpm
+ - const: pclk
+
+ resets:
+ items:
+ - description: APB register reset
+ - description: Controller reset
+
+ reset-names:
+ items:
+ - const: prst
+ - const: rst
+
+ cix,syscon-usb:
+ $ref: /schemas/types.yaml#/definitions/phandle
+ description:
+ Phandle to the Sky1 S5 system controller used to program USB mode
+ strap controls.
+
+ dma-coherent: true
+
+ maximum-speed:
+ enum: [super-speed-plus, super-speed, high-speed, full-speed]
+
+ phys:
+ minItems: 1
+ maxItems: 2
+
+ phy-names:
+ minItems: 1
+ maxItems: 2
+ items:
+ anyOf:
+ - const: cdns3,usb2-phy
+ - const: cdns3,usb3-phy
+
+required:
+ - compatible
+ - reg
+ - reg-names
+ - interrupts
+ - interrupt-names
+ - clocks
+ - clock-names
+ - resets
+ - reset-names
+ - cix,syscon-usb
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ #include <dt-bindings/clock/cix,sky1.h>
+ #include <dt-bindings/interrupt-controller/arm-gic.h>
+ #include <dt-bindings/reset/cix,sky1-s5-system-control.h>
+
+ soc {
+ #address-cells = <2>;
+ #size-cells = <2>;
+
+ usb@91d0000 {
+ compatible = "cix,sky1-usb3", "cix,cdns-usb3";
+ reg = <0x00 0x91d0000 0x00 0x4000>,
+ <0x00 0x91d4000 0x00 0x4000>,
+ <0x00 0x91d8000 0x00 0x8000>,
+ <0x00 0x91c0314 0x00 0x4>;
+ reg-names = "otg", "dev", "xhci", "glue";
+ interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>,
+ <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>;
+ interrupt-names = "host", "peripheral", "otg", "wakeup";
+ clocks = <&scmi_clk CLK_TREE_USB3A_H0_CLK_SOF>,
+ <&scmi_clk CLK_TREE_USB3A_0_AXI_GATE>,
+ <&scmi_clk CLK_TREE_USB3A_H0_CLK_LPM>,
+ <&scmi_clk CLK_TREE_USB3A_0_APB_GATE>;
+ clock-names = "sof", "aclk", "lpm", "pclk";
+ resets = <&s5_syscon SKY1_USBC_SS2_PRST_N>,
+ <&s5_syscon SKY1_USBC_SS2_RST_N>;
+ reset-names = "prst", "rst";
+ cix,syscon-usb = <&s5_syscon>;
+ dma-coherent;
+ maximum-speed = "super-speed-plus";
+ dr_mode = "otg";
+ };
+ };
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers
2026-05-11 2:42 [PATCH 0/4] Add CIX Sky1 Cadence USB3 support Peter Chen
` (2 preceding siblings ...)
2026-05-11 2:42 ` [PATCH 3/4] dt-bindings: usb: add CIX Sky1 Cadence USB3 controller Peter Chen
@ 2026-05-11 2:42 ` Peter Chen
2026-05-11 23:59 ` sashiko-bot
3 siblings, 1 reply; 11+ messages in thread
From: Peter Chen @ 2026-05-11 2:42 UTC (permalink / raw)
To: robh, krzk+dt, conor+dt, gregkh, pawell, rogerq
Cc: devicetree, linux-kernel, linux-usb, cix-kernel-upstream,
linux-arm-kernel, arnd, Peter Chen
Add the Sky1 USB4 and USB5 Cadence USB3 controller nodes with their
registers, interrupts, clocks, resets and S5 syscon control. Enable both
ports on the Orion O6 board in host mode with the required VBUS pinctrl.
Signed-off-by: Peter Chen <peter.chen@cixtech.com>
---
arch/arm64/boot/dts/cix/sky1-orion-o6.dts | 30 ++++++++++
arch/arm64/boot/dts/cix/sky1.dtsi | 68 +++++++++++++++++++++++
2 files changed, 98 insertions(+)
diff --git a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
index e39c87774c12..d1e2afceea15 100644
--- a/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
+++ b/arch/arm64/boot/dts/cix/sky1-orion-o6.dts
@@ -80,6 +80,22 @@ pins {
};
};
+
+ pinctrl_usb4: usb4-power-on-cfg {
+ pins {
+ pinmux = <CIX_PAD_GPIO041_FUNC_USB_DRIVE_VBUS4>;
+ bias-pull-down;
+ drive-strength = <8>;
+ };
+ };
+
+ pinctrl_usb5: usb5-power-on-cfg {
+ pins {
+ pinmux = <CIX_PAD_GPIO042_FUNC_USB_DRIVE_VBUS5>;
+ bias-pull-down;
+ drive-strength = <8>;
+ };
+ };
};
&pcie_x8_rc {
@@ -117,3 +133,17 @@ &s5_gpio2 {
&uart2 {
status = "okay";
};
+
+&usb4 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb4>;
+ dr_mode = "host";
+ status = "okay";
+};
+
+&usb5 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&pinctrl_usb5>;
+ dr_mode = "host";
+ status = "okay";
+};
diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
index bb5cfb1f2113..9f7d9ad6586c 100644
--- a/arch/arm64/boot/dts/cix/sky1.dtsi
+++ b/arch/arm64/boot/dts/cix/sky1.dtsi
@@ -6,6 +6,8 @@
#include <dt-bindings/interrupt-controller/arm-gic.h>
#include <dt-bindings/clock/cix,sky1.h>
+#include <dt-bindings/phy/phy.h>
+#include <dt-bindings/reset/cix,sky1-s5-system-control.h>
#include "sky1-power.h"
/ {
@@ -504,6 +506,72 @@ mbox_ap2sfh: mailbox@80a0000 {
cix,mbox-dir = "tx";
};
+ usb4: usb@91d0000 {
+ compatible = "cix,sky1-usb3", "cix,cdns-usb3";
+ reg = <0x00 0x91d0000 0x00 0x4000>,
+ <0x00 0x91d4000 0x00 0x4000>,
+ <0x00 0x91d8000 0x00 0x8000>,
+ <0x00 0x91c0314 0x00 0x4>;
+ reg-names = "otg", "dev", "xhci", "glue";
+
+ interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>, /* host irq */
+ <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>, /* peripheral irq */
+ <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>, /* otgirq */
+ <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>; /* wakeup irq */
+ interrupt-names = "host",
+ "peripheral",
+ "otg",
+ "wakeup";
+
+ resets = <&s5_syscon SKY1_USBC_SS2_PRST_N>,
+ <&s5_syscon SKY1_USBC_SS2_RST_N>;
+ reset-names = "prst", "rst";
+
+ clocks = <&scmi_clk CLK_TREE_USB3A_H0_CLK_SOF>,
+ <&scmi_clk CLK_TREE_USB3A_0_AXI_GATE>,
+ <&scmi_clk CLK_TREE_USB3A_H0_CLK_LPM>,
+ <&scmi_clk CLK_TREE_USB3A_0_APB_GATE>;
+ clock-names = "sof", "aclk", "lpm", "pclk";
+
+ cix,syscon-usb = <&s5_syscon>;
+ dma-coherent;
+ maximum-speed = "super-speed-plus";
+ dr_mode = "otg";
+ };
+
+ usb5: usb@91e0000 {
+ compatible = "cix,sky1-usb3", "cix,cdns-usb3";
+ reg = <0x00 0x91e0000 0x00 0x4000>,
+ <0x00 0x91e4000 0x00 0x4000>,
+ <0x00 0x91e8000 0x00 0x8000>,
+ <0x00 0x91c0324 0x00 0x4>;
+ reg-names = "otg", "dev", "xhci", "glue";
+
+ interrupts = <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>, /* host irq */
+ <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>, /* peripheral irq */
+ <GIC_SPI 258 IRQ_TYPE_LEVEL_HIGH 0>, /* otgirq */
+ <GIC_SPI 257 IRQ_TYPE_LEVEL_HIGH 0>; /* wakeup irq */
+ interrupt-names = "host",
+ "peripheral",
+ "otg",
+ "wakeup";
+
+ resets = <&s5_syscon SKY1_USBC_SS3_PRST_N>,
+ <&s5_syscon SKY1_USBC_SS3_RST_N>;
+ reset-names = "prst", "rst";
+
+ clocks = <&scmi_clk CLK_TREE_USB3A_H1_CLK_SOF>,
+ <&scmi_clk CLK_TREE_USB3A_1_AXI_GATE>,
+ <&scmi_clk CLK_TREE_USB3A_H1_CLK_LPM>,
+ <&scmi_clk CLK_TREE_USB3A_1_APB_GATE>;
+ clock-names = "sof", "aclk", "lpm", "pclk";
+
+ cix,syscon-usb = <&s5_syscon>;
+ dma-coherent;
+ maximum-speed = "super-speed-plus";
+ dr_mode = "otg";
+ };
+
pcie_x8_rc: pcie@a010000 {
compatible = "cix,sky1-pcie-host";
reg = <0x00 0x0a010000 0x00 0x10000>,
--
2.50.1
^ permalink raw reply related [flat|nested] 11+ messages in thread* Re: [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers
2026-05-11 2:42 ` [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers Peter Chen
@ 2026-05-11 23:59 ` sashiko-bot
2026-05-12 3:10 ` Peter Chen
0 siblings, 1 reply; 11+ messages in thread
From: sashiko-bot @ 2026-05-11 23:59 UTC (permalink / raw)
To: Peter Chen; +Cc: robh, devicetree, conor+dt, krzk+dt
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Medium] Missing `status = "disabled";` for `usb4` and `usb5` device nodes in the SoC-level device tree.
- [Medium] Invalid double-vendor prefix in compatible string and squashing of SoC-specific glue layer with generic IP core.
- [Low] Unnecessary inclusion of `<dt-bindings/phy/phy.h>` in `sky1.dtsi` without associated PHY definitions.
- [Low] Redundant/duplicate interrupt mappings for the `wakeup` interrupt.
--
commit a270480764c96a27fe2e14e30bfb46b99527fcfb
Author: Peter Chen <peter.chen@cixtech.com>
arm64: dts: cix: add Sky1 USB4 and USB5 controllers
Add the Sky1 USB4 and USB5 Cadence USB3 controller nodes with their
registers, interrupts, clocks, resets and S5 syscon control. Enable both
ports on the Orion O6 board in host mode with the required VBUS pinctrl.
> diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> index bb5cfb1f21133..9f7d9ad6586c3 100644
> --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> @@ -6,6 +6,8 @@
>
> #include <dt-bindings/interrupt-controller/arm-gic.h>
> #include <dt-bindings/clock/cix,sky1.h>
> +#include <dt-bindings/phy/phy.h>
Since there are no phys properties or macros from this header used in the
patch, is this inclusion needed?
> +#include <dt-bindings/reset/cix,sky1-s5-system-control.h>
> #include "sky1-power.h"
>
> / {
[ ... ]
> @@ -504,6 +506,72 @@ mbox_ap2sfh: mailbox@80a0000 {
> cix,mbox-dir = "tx";
> };
>
> + usb4: usb@91d0000 {
> + compatible = "cix,sky1-usb3", "cix,cdns-usb3";
Does the "cix,cdns-usb3" string incorrectly combine two vendor prefixes?
Should the generic fallback be "cdns,usb3"?
> + reg = <0x00 0x91d0000 0x00 0x4000>,
> + <0x00 0x91d4000 0x00 0x4000>,
> + <0x00 0x91d8000 0x00 0x8000>,
> + <0x00 0x91c0314 0x00 0x4>;
> + reg-names = "otg", "dev", "xhci", "glue";
Does adding the "glue" register and cix,syscon-usb to the generic core
break the generic upstream schema which expects only three reg entries?
Should the SoC-specific glue logic be implemented as a parent wrapper node
with a child node for the generic Cadence USB3 core instead?
> +
> + interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>, /* host irq */
> + <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>, /* peripheral irq */
> + <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>, /* otgirq */
> + <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>; /* wakeup irq */
The wakeup interrupt uses the same GIC SPI line as the main host and
peripheral interrupts. Is this redundant, and could it lead to conflicting
interrupt registration?
[ ... ]
> + cix,syscon-usb = <&s5_syscon>;
> + dma-coherent;
> + maximum-speed = "super-speed-plus";
> + dr_mode = "otg";
> + };
Should this node include a status = "disabled" property so the USB drivers do
not probe unconditionally on all boards using this SoC?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260511024244.981941-1-peter.chen@cixtech.com?part=4
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers
2026-05-11 23:59 ` sashiko-bot
@ 2026-05-12 3:10 ` Peter Chen
0 siblings, 0 replies; 11+ messages in thread
From: Peter Chen @ 2026-05-12 3:10 UTC (permalink / raw)
To: sashiko; +Cc: robh, devicetree, conor+dt, krzk+dt
On 26-05-11 23:59:21, sashiko-bot@kernel.org wrote:
> EXTERNAL EMAIL
>
> Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
> - [Medium] Missing `status = "disabled";` for `usb4` and `usb5` device nodes in the SoC-level device tree.
Ack
> - [Medium] Invalid double-vendor prefix in compatible string and squashing of SoC-specific glue layer with generic IP core.
Nak
> - [Low] Unnecessary inclusion of `<dt-bindings/phy/phy.h>` in `sky1.dtsi` without associated PHY definitions.
Ack
> - [Low] Redundant/duplicate interrupt mappings for the `wakeup` interrupt.
Nak
> --
>
> commit a270480764c96a27fe2e14e30bfb46b99527fcfb
> Author: Peter Chen <peter.chen@cixtech.com>
>
> arm64: dts: cix: add Sky1 USB4 and USB5 controllers
>
> Add the Sky1 USB4 and USB5 Cadence USB3 controller nodes with their
> registers, interrupts, clocks, resets and S5 syscon control. Enable both
> ports on the Orion O6 board in host mode with the required VBUS pinctrl.
>
> > diff --git a/arch/arm64/boot/dts/cix/sky1.dtsi b/arch/arm64/boot/dts/cix/sky1.dtsi
> > index bb5cfb1f21133..9f7d9ad6586c3 100644
> > --- a/arch/arm64/boot/dts/cix/sky1.dtsi
> > +++ b/arch/arm64/boot/dts/cix/sky1.dtsi
> > @@ -6,6 +6,8 @@
> >
> > #include <dt-bindings/interrupt-controller/arm-gic.h>
> > #include <dt-bindings/clock/cix,sky1.h>
> > +#include <dt-bindings/phy/phy.h>
>
> Since there are no phys properties or macros from this header used in the
> patch, is this inclusion needed?
Ack, will delete.
>
> > +#include <dt-bindings/reset/cix,sky1-s5-system-control.h>
> > #include "sky1-power.h"
> >
> > / {
>
> [ ... ]
>
> > @@ -504,6 +506,72 @@ mbox_ap2sfh: mailbox@80a0000 {
> > cix,mbox-dir = "tx";
> > };
> >
> > + usb4: usb@91d0000 {
> > + compatible = "cix,sky1-usb3", "cix,cdns-usb3";
>
> Does the "cix,cdns-usb3" string incorrectly combine two vendor prefixes?
> Should the generic fallback be "cdns,usb3"?
No. sky1-usb for specific SoCs "sky1", "cdns-usb3" for general CIX SoCs using
Cadence IP.
>
> > + reg = <0x00 0x91d0000 0x00 0x4000>,
> > + <0x00 0x91d4000 0x00 0x4000>,
> > + <0x00 0x91d8000 0x00 0x8000>,
> > + <0x00 0x91c0314 0x00 0x4>;
> > + reg-names = "otg", "dev", "xhci", "glue";
>
> Does adding the "glue" register and cix,syscon-usb to the generic core
> break the generic upstream schema which expects only three reg entries?
>
> Should the SoC-specific glue logic be implemented as a parent wrapper node
> with a child node for the generic Cadence USB3 core instead?
For sky1 USB, it uses Cadence USB3 core as library, so all register regions
need pass from one parent node.
>
> > +
> > + interrupts = <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>, /* host irq */
> > + <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>, /* peripheral irq */
> > + <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH 0>, /* otgirq */
> > + <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH 0>; /* wakeup irq */
>
> The wakeup interrupt uses the same GIC SPI line as the main host and
> peripheral interrupts. Is this redundant, and could it lead to conflicting
> interrupt registration?
>
Not redundant, they are different interrupt handlers and knows itself at code.
> [ ... ]
>
> > + cix,syscon-usb = <&s5_syscon>;
> > + dma-coherent;
> > + maximum-speed = "super-speed-plus";
> > + dr_mode = "otg";
> > + };
>
> Should this node include a status = "disabled" property so the USB drivers do
> not probe unconditionally on all boards using this SoC?
Ack
--
Best regards,
Peter
^ permalink raw reply [flat|nested] 11+ messages in thread