Message-ID: <20250329231004.4432831b@wsk>
Date: Sat, 29 Mar 2025 23:10:04 +0100
From: Lukasz Majewski <lukma@...x.de>
To: Krzysztof Kozlowski <krzk@...nel.org>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, davem@...emloft.net, Eric Dumazet
 <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni
 <pabeni@...hat.com>, Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, Shawn Guo
 <shawnguo@...nel.org>, Sascha Hauer <s.hauer@...gutronix.de>, Pengutronix
 Kernel Team <kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>,
 Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
 imx@...ts.linux.dev, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2 1/4] dt-bindings: net: Add MTIP L2 switch description

Hi Krzysztof,

> On 28/03/2025 14:35, Lukasz Majewski wrote:
> > This patch provides description of the MTIP L2 switch available in
> > some NXP's SOCs - e.g. imx287.
> > 
> > Signed-off-by: Lukasz Majewski <lukma@...x.de>
> > ---
> > Changes for v2:
> > - Rename the file to match exactly the compatible
> >   (nxp,imx287-mtip-switch)  
> 
> Please implement all the changes, not only the rename. I gave several
> comments, although quick glance suggests you did implement them, so
> then changelog is just incomplete.

Those comments were IMHO addressed automatically, as this time I took
(I suppose :-) ) better file as a starting point.

To be more specific it was:
./Documentation/devicetree/bindings/net/ti,cpsw-switch.yaml

as I've been advised to use for the MTIP driver the same DTS
description as the above one has (i.e. they are conceptually similar,
so DTS description ABI can be used for both).

I've also checked the:
make CHECK_DTBS=y DT_SCHEMA_FILES=nxp,imx287-mtip-switch.yaml
nxp/mxs/imx28-xea.dtb

on Linux next and it gave no errors.

> 
> > ---
> >  .../bindings/net/nxp,imx287-mtip-switch.yaml  | 165
> > ++++++++++++++++++ 1 file changed, 165 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > 
> > diff --git
> > a/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > new file mode 100644 index 000000000000..a3e0fe7783ec --- /dev/null
> > +++
> > b/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.yaml
> > @@ -0,0 +1,165 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR
> > BSD-2-Clause) +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/nxp,imx287-mtip-switch.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: NXP SoC Ethernet Switch Controller (L2 MoreThanIP switch)
> > +
> > +maintainers:
> > +  - Lukasz Majewski <lukma@...x.de>
> > +
> > +description:
> > +  The 2-port switch ethernet subsystem provides ethernet packet
> > (L2)
> > +  communication and can be configured as an ethernet switch. It
> > provides the
> > +  reduced media independent interface (RMII), the management data
> > input
> > +  output (MDIO) for physical layer device (PHY) management.
> > +  
> 
> If this is ethernet switch, why it does not reference ethernet-switch
> schema? or dsa.yaml or dsa/ethernet-ports? I am not sure which one
> should go here, but surprising to see none.

It uses:
$ref:·ethernet-controller.yaml#

for "ports".

Other crucial node is "mdio", which references $ref: mdio.yaml#

> 
> > +properties:
> > +  compatible:
> > +    const: nxp,imx287-mtip--switch  
> 
> Just one -.
> 

Ok.

> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      The physical base address and size of the MTIP L2 SW module
> > IO range  
> 
> Wasn't here, drop.
> 

The 'reg' property (reg = <0x800f0000 0x20000>;) is defined in
imx28.dtsi, where the SoC generic properties (as suggested by Andrew -
like clocks, interrupts, clock-names) are moved.

> > +
> > +  phy-supply:
> > +    description:
> > +      Regulator that powers Ethernet PHYs.
> > +
> > +  clocks:
> > +    items:
> > +      - description: Register accessing clock
> > +      - description: Bus access clock
> > +      - description: Output clock for external device - e.g. PHY
> > source clock
> > +      - description: IEEE1588 timer clock
> > +
> > +  clock-names:
> > +    items:
> > +      - const: ipg
> > +      - const: ahb
> > +      - const: enet_out
> > +      - const: ptp
> > +
> > +  interrupts:
> > +    items:
> > +      - description: Switch interrupt
> > +      - description: ENET0 interrupt
> > +      - description: ENET1 interrupt
> > +
> > +  pinctrl-names: true  
> 
> Drop

The 'pinctrl-names = "default";' are specified.

Shouldn't it be kept?

> 
> > +
> > +  ethernet-ports:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^port@[0-9]+$":  
> 
> Keep consistent quotes, either " or '. Also [01]
> 

[12] - ports are numbered starting from 1.

> 
> > +        type: object
> > +        description: MTIP L2 switch external ports
> > +
> > +        $ref: ethernet-controller.yaml#
> > +        unevaluatedProperties: false
> > +
> > +        properties:
> > +          reg:
> > +            items:
> > +              - enum: [1, 2]
> > +            description: MTIP L2 switch port number
> > +
> > +          label:
> > +            description: Label associated with this port
> > +
> > +        required:
> > +          - reg
> > +          - label
> > +          - phy-mode
> > +          - phy-handle
> > +
> > +  mdio:
> > +    type: object
> > +    $ref: mdio.yaml#
> > +    unevaluatedProperties: false
> > +    description:
> > +      Specifies the mdio bus in the switch, used as a container
> > for phy nodes. +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - clocks
> > +  - clock-names
> > +  - interrupts
> > +  - mdio
> > +  - ethernet-ports
> > +  - '#address-cells'
> > +  - '#size-cells'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include<dt-bindings/interrupt-controller/irq.h>
> > +    switch@...f0000 {
> > +        compatible = "nxp,imx287-mtip-switch";
> > +        reg = <0x800f0000 0x20000>;
> > +        pinctrl-names = "default";
> > +        pinctrl-0 = <&mac0_pins_a>, <&mac1_pins_a>;
> > +        phy-supply = <&reg_fec_3v3>;
> > +        interrupts = <100>, <101>, <102>;
> > +        clocks = <&clks 57>, <&clks 57>, <&clks 64>, <&clks 35>;
> > +        clock-names = "ipg", "ahb", "enet_out", "ptp";
> > +        status = "okay";  
> 
> Drop

Ok.

> 
> > +
> > +        ethernet-ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;  
> 
> Messed indentation. See example-schema or writing-schema.
> 

Ok.

> 
> 
> Best regards,
> Krzysztof




Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@...x.de

Content of type "application/pgp-signature" skipped

Powered by blists - more mailing lists