* Re: [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers
[not found] ` <20260511235922.10315C2BCB0@smtp.kernel.org>
@ 2026-05-12 3:10 ` Peter Chen
0 siblings, 0 replies; only message 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] only message in thread