Re: [PATCH 4/4] arm64: dts: cix: add Sky1 USB4 and USB5 controllers

On 26-05-11 23:59:21, [email protected] 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 <[email protected]>
> 
> 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