Re: [PATCH 3/4] dt-bindings: usb: add CIX Sky1 Cadence USB3 controller

[Peter Chen <peter.chen@cixtech.com>]

On 26-05-15 09:54:10, Krzysztof Kozlowski wrote:
> EXTERNAL EMAIL
> 
> On Mon, May 11, 2026 at 10:42:43AM +0800, Peter Chen wrote:
> > 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 ++++++++++++++++++
> 
> Why are you mixing USB patches with DTS in one patchset? Don't.

In this series, the 1st patch is the IP core driver changes (export APIs for glue layer
use), and the second glue layer patch is the user for new adding APIs.
Normally, we combine dt-binding, driver (glue layer) and DTS changes at one patch series.

It is much like below submission:

https://lore.kernel.org/all/20250318-dwc3-refactor-v5-0-90ea6e5b3ba4@oss.qualcomm.com/

> 
> >  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
> 
> Complete mess of filename. There is no such compatible.

Will rename to cix,sky1-usb3.yaml matching the compatible in v2.
 
> 
> > +
> > +properties:
> > +  compatible:
> > +    items:
> > +      - const: cix,sky1-usb3
> > +      - const: cix,cdns-usb3
> 
> I don't understand the fallback compatible. You claim this device is
> called EXACTLY like vendor cdns? Nope, you SoC specific compatibles.

The intent was to express "this is a Cadence USB3 IP integrated on a CIX
SoC" but "cix,cdns-usb3" is wrong. Will drop the fallback compatible in
v2, and there is only one compatible here.
> 
> 
> > +
> > +  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
> 
> Wrong order, look at cdns,usb3 schema.

Right, will reorder to match cdns,usb3: otg, xhci, dev in v2.

> 
> > +      - 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
> 
> apb
> 
> > +      - const: rst
> 
> controller or core

Will rename to: prst -> apb, rst -> core.

> 
> > +
> > +  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]
> 
> Why isn't this deducible from the compatible?

It is the common compatible (Documentation/devicetree/bindings/usb/usb.yaml)
I will delete it, and add $ref: usb.yaml#.

> 
> > +
> > +  phys:
> > +    minItems: 1
> > +    maxItems: 2
> 
> No, this is not flexible.

At Sky1 SoC, some USB controllers connect to both USB3 and USB2 PHYs, the others
only connect USB2 PHY for dedicated USB2-only port.

> 
> > +
> > +  phy-names:
> > +    minItems: 1
> > +    maxItems: 2
> > +    items:
> > +      anyOf:
> > +        - const: cdns3,usb2-phy
> > +        - const: cdns3,usb3-phy
> 
> Drop all this and define standard names.

Will use standard names: usb2-phy, usb3-phy in v2.

> 
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - interrupts
> > +  - interrupt-names
> > +  - clocks
> > +  - clock-names
> > +  - resets
> > +  - reset-names
> > +  - cix,syscon-usb
> 
> phys should be required, no?

Yes, will add phys and phy-names to required in v2.

-- 

Best regards,
Peter