diff mbox series

[v2,1/4] dt-bindings: net: Add MTIP L2 switch description

Message ID 20250328133544.4149716-2-lukma@denx.de (mailing list archive)
State New
Headers show
Series net: mtip: Add support for MTIP imx287 L2 switch driver | expand

Commit Message

Lukasz Majewski March 28, 2025, 1:35 p.m. UTC
This patch provides description of the MTIP L2 switch available in some
NXP's SOCs - e.g. imx287.

Signed-off-by: Lukasz Majewski <lukma@denx.de>
---
Changes for v2:
- Rename the file to match exactly the compatible
  (nxp,imx287-mtip-switch)
---
 .../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

Comments

Krzysztof Kozlowski March 28, 2025, 2:07 p.m. UTC | #1
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@denx.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.

> ---
>  .../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@denx.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.

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

Just one -.

> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      The physical base address and size of the MTIP L2 SW module IO range

Wasn't here, drop.

> +
> +  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

> +
> +  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]


> +        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@800f0000 {
> +        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

> +
> +        ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;

Messed indentation. See example-schema or writing-schema.



Best regards,
Krzysztof
Andrew Lunn March 28, 2025, 6:23 p.m. UTC | #2
> +                ethphy0: ethernet-phy@0 {
> +                        reg = <0>;
> +                        smsc,disable-energy-detect;
> +                        /* Both PHYs (i.e. 0,1) have the same, single GPIO, */
> +                        /* line to handle both, their interrupts (AND'ed) */
> +                        interrupt-parent = <&gpio4>;
> +                        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;

Shared interrupts cannot be edge. They are level, so that either can
hold the interrupt active until it is cleared.

Also, PHY interrupts in general are level, because there are multiple
interrupt sources within the PHY, and you need to clear them all
before the interrupt is released.

	Andrew
Andrew Lunn March 28, 2025, 6:30 p.m. UTC | #3
> +                        /* Both PHYs (i.e. 0,1) have the same, single GPIO, */
> +                        /* line to handle both, their interrupts (AND'ed) */

ORed, not ANDed.

Often, the interrupt line has a weak pullup resistor, so by default it
is high. Either PHY can then pull it low, using an open collector,
which is HI-Z when not driving.

	Andrew
Rob Herring March 29, 2025, 1:37 a.m. UTC | #4
On Fri, 28 Mar 2025 14:35:41 +0100, 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@denx.de>
> ---
> Changes for v2:
> - Rename the file to match exactly the compatible
>   (nxp,imx287-mtip-switch)
> ---
>  .../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
> 

My bot found errors running 'make dt_binding_check' on your patch:

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.example.dtb: /example-0/switch@800f0000: failed to match any schema with compatible: ['nxp,imx287-mtip-switch']
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.example.dtb: ethernet-phy@0: interrupts: [[13], [2]] is too long
	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#
/builds/robherring/dt-review-ci/linux/Documentation/devicetree/bindings/net/nxp,imx287-mtip-switch.example.dtb: ethernet-phy@1: interrupts: [[13], [2]] is too long
	from schema $id: http://devicetree.org/schemas/net/ethernet-phy.yaml#

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/project/devicetree-bindings/patch/20250328133544.4149716-2-lukma@denx.de

The base for the series is generally the latest rc1. A different dependency
should be noted in *this* patch.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit after running the above command yourself. Note
that DT_SCHEMA_FILES can be set to your schema file to speed up checking
your schema. However, it must be unset to test all examples with your schema.
Rob Herring March 29, 2025, 5:09 p.m. UTC | #5
On Fri, Mar 28, 2025 at 02:35:41PM +0100, 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@denx.de>
> ---
> Changes for v2:
> - Rename the file to match exactly the compatible
>   (nxp,imx287-mtip-switch)
> ---
>  .../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@denx.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.
> +
> +properties:
> +  compatible:
> +    const: nxp,imx287-mtip--switch
> +
> +  reg:
> +    maxItems: 1
> +    description:
> +      The physical base address and size of the MTIP L2 SW module IO range
> +
> +  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
> +
> +  ethernet-ports:
> +    type: object
> +    additionalProperties: false
> +
> +    properties:
> +      '#address-cells':
> +        const: 1
> +      '#size-cells':
> +        const: 0
> +
> +    patternProperties:
> +      "^port@[0-9]+$":
> +        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@800f0000 {
> +        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";
> +
> +        ethernet-ports {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                mtip_port1: port@1 {
> +                        reg = <1>;
> +                        label = "lan0";
> +                        local-mac-address = [ 00 00 00 00 00 00 ];
> +                        phy-mode = "rmii";
> +                        phy-handle = <&ethphy0>;
> +                };
> +
> +                mtip_port2: port@2 {
> +                        reg = <2>;
> +                        label = "lan1";
> +                        local-mac-address = [ 00 00 00 00 00 00 ];
> +                        phy-mode = "rmii";
> +                        phy-handle = <&ethphy1>;
> +                };
> +        };
> +
> +        mdio_sw: mdio {
> +                #address-cells = <1>;
> +                #size-cells = <0>;
> +
> +                reset-gpios = <&gpio2 13 0>;
> +                reset-delay-us = <25000>;
> +                reset-post-delay-us = <10000>;
> +
> +                ethphy0: ethernet-phy@0 {
> +                        reg = <0>;
> +                        smsc,disable-energy-detect;

With a custom property, you should have a specific compatible.

> +                        /* Both PHYs (i.e. 0,1) have the same, single GPIO, */
> +                        /* line to handle both, their interrupts (AND'ed) */
> +                        interrupt-parent = <&gpio4>;
> +                        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;

The error report is because the examples have to guess the number of 
provider interrupt cells and only 1 guess is supported. It guessed 1 
from above.

In any case, unless the phys are built-in and fixed, they are out of 
scope of this binding. So perhaps drop the interrupts and smsc property.

Rob
Lukasz Majewski March 29, 2025, 9:16 p.m. UTC | #6
Hi Rob,

> On Fri, Mar 28, 2025 at 02:35:41PM +0100, 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@denx.de>
> > ---
> > Changes for v2:
> > - Rename the file to match exactly the compatible
> >   (nxp,imx287-mtip-switch)
> > ---
> >  .../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@denx.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.
> > +
> > +properties:
> > +  compatible:
> > +    const: nxp,imx287-mtip--switch
> > +
> > +  reg:
> > +    maxItems: 1
> > +    description:
> > +      The physical base address and size of the MTIP L2 SW module
> > IO range +
> > +  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
> > +
> > +  ethernet-ports:
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      '#address-cells':
> > +        const: 1
> > +      '#size-cells':
> > +        const: 0
> > +
> > +    patternProperties:
> > +      "^port@[0-9]+$":
> > +        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@800f0000 {
> > +        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";
> > +
> > +        ethernet-ports {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                mtip_port1: port@1 {
> > +                        reg = <1>;
> > +                        label = "lan0";
> > +                        local-mac-address = [ 00 00 00 00 00 00 ];
> > +                        phy-mode = "rmii";
> > +                        phy-handle = <&ethphy0>;
> > +                };
> > +
> > +                mtip_port2: port@2 {
> > +                        reg = <2>;
> > +                        label = "lan1";
> > +                        local-mac-address = [ 00 00 00 00 00 00 ];
> > +                        phy-mode = "rmii";
> > +                        phy-handle = <&ethphy1>;
> > +                };
> > +        };
> > +
> > +        mdio_sw: mdio {
> > +                #address-cells = <1>;
> > +                #size-cells = <0>;
> > +
> > +                reset-gpios = <&gpio2 13 0>;
> > +                reset-delay-us = <25000>;
> > +                reset-post-delay-us = <10000>;
> > +
> > +                ethphy0: ethernet-phy@0 {
> > +                        reg = <0>;
> > +                        smsc,disable-energy-detect;  
> 
> With a custom property, you should have a specific compatible.

I could add:
compatible = "ethernet-phy-id0007.c0f0","ethernet-phy-ieee802.3-c22";

but it would not make things either clearer nor simpler.

I will just remove this property.

> 
> > +                        /* Both PHYs (i.e. 0,1) have the same,
> > single GPIO, */
> > +                        /* line to handle both, their interrupts
> > (AND'ed) */
> > +                        interrupt-parent = <&gpio4>;
> > +                        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;  
> 
> The error report is because the examples have to guess the number of 
> provider interrupt cells and only 1 guess is supported. It guessed 1 
> from above.
> 
> In any case, unless the phys are built-in and fixed, they are out of 
> scope of this binding. So perhaps drop the interrupts and smsc
> property.

Ok, I will remove them.

> 
> Rob




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@denx.de
Lukasz Majewski March 29, 2025, 10:10 p.m. UTC | #7
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@denx.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@denx.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@800f0000 {
> > +        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@denx.de
Lukasz Majewski March 30, 2025, 6:36 a.m. UTC | #8
Hi Andrew,

> > +                ethphy0: ethernet-phy@0 {
> > +                        reg = <0>;
> > +                        smsc,disable-energy-detect;
> > +                        /* Both PHYs (i.e. 0,1) have the same,
> > single GPIO, */
> > +                        /* line to handle both, their interrupts
> > (AND'ed) */
> > +                        interrupt-parent = <&gpio4>;
> > +                        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;  
> 
> Shared interrupts cannot be edge. They are level, so that either can
> hold the interrupt active until it is cleared.
> 
> Also, PHY interrupts in general are level, because there are multiple
> interrupt sources within the PHY, and you need to clear them all
> before the interrupt is released.

Good point, I will update it accordingly. Thanks.

> 
> 	Andrew




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@denx.de
Lukasz Majewski March 30, 2025, 6:38 a.m. UTC | #9
Hi Andrew,

> > +                        /* Both PHYs (i.e. 0,1) have the same,
> > single GPIO, */
> > +                        /* line to handle both, their interrupts
> > (AND'ed) */  
> 
> ORed, not ANDed.
> 

Yes, correct.

> Often, the interrupt line has a weak pullup resistor, so by default it
> is high. Either PHY can then pull it low, using an open collector,
> which is HI-Z when not driving.

Yes, correct - this is how it works.

> 
> 	Andrew

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@denx.de
Krzysztof Kozlowski March 30, 2025, 9:47 a.m. UTC | #10
On 29/03/2025 23:10, Lukasz Majewski wrote:
>>> +  
>>
>> 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#

These are children, I am speaking about this device node.

> 
>>
>>> +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.

Drop description, not the reg. Reg was in the previous version. You
added random changes here, not coming from the previous review.

Best regards,
Krzysztof
Lukasz Majewski March 30, 2025, 9:04 p.m. UTC | #11
Hi Krzysztof,

> On 29/03/2025 23:10, Lukasz Majewski wrote:
> >>> +    
> >>
> >> 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#  
> 
> These are children, I am speaking about this device node.

It looks like there is no such reference.

I've checked the aforementioned ti,cpsw-switch.yaml,
microchip,lan966x-switch.yaml and renesas,r8a779f0-ether-switch.yaml.

Those only have $ref: for ethernet-port children node.

The "outer" one doesn't have it.


Or am I missing something?

> 
> >   
> >>  
> >>> +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.  
> 
> Drop description, not the reg. Reg was in the previous version. You
> added random changes here, not coming from the previous review.
> 

Ach... You mean the "description" in the:

	reg:
	  maxItems: 1
	  description:
	    XX YY

Ok, I will remove it.

> 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@denx.de
Krzysztof Kozlowski March 31, 2025, 6:30 a.m. UTC | #12
On 30/03/2025 23:04, Lukasz Majewski wrote:
> Hi Krzysztof,
> 
>> On 29/03/2025 23:10, Lukasz Majewski wrote:
>>>>> +    
>>>>
>>>> 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#  
>>
>> These are children, I am speaking about this device node.
> 
> It looks like there is no such reference.
> 
> I've checked the aforementioned ti,cpsw-switch.yaml,
> microchip,lan966x-switch.yaml and renesas,r8a779f0-ether-switch.yaml.
> 
> Those only have $ref: for ethernet-port children node.
> 
> The "outer" one doesn't have it.
> 
> 
> Or am I missing something?

There is ethernet-switch.yaml for non-DSA switches and there is DSA
(using ethernet switch, btw). I don't know why these devices do not use
it, I guess no one converted them after we split ethernet-switch out of DSA.

Best regards,
Krzysztof
Lukasz Majewski March 31, 2025, 7:19 a.m. UTC | #13
Hi Krzysztof,

> On 30/03/2025 23:04, Lukasz Majewski wrote:
> > Hi Krzysztof,
> >   
> >> On 29/03/2025 23:10, Lukasz Majewski wrote:  
> >>>>> +      
> >>>>
> >>>> 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#
> >>>  
> >>
> >> These are children, I am speaking about this device node.  
> > 
> > It looks like there is no such reference.
> > 
> > I've checked the aforementioned ti,cpsw-switch.yaml,
> > microchip,lan966x-switch.yaml and
> > renesas,r8a779f0-ether-switch.yaml.
> > 
> > Those only have $ref: for ethernet-port children node.
> > 
> > The "outer" one doesn't have it.
> > 
> > 
> > Or am I missing something?  
> 
> There is ethernet-switch.yaml for non-DSA switches and there is DSA
> (using ethernet switch, btw). I don't know why these devices do not
> use it, I guess no one converted them after we split ethernet-switch
> out of DSA.

In net next there is:
Documentation/devicetree/bindings/net/dsa/dsa.yaml
Documentation/devicetree/bindings/net/mscc,vsc7514-switch.yaml

which uses
$ref: ethernet-switch.yaml#

I will add it in a similar way as it is in dsa.yaml.

> 
> 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@denx.de
diff mbox series

Patch

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@denx.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.
+
+properties:
+  compatible:
+    const: nxp,imx287-mtip--switch
+
+  reg:
+    maxItems: 1
+    description:
+      The physical base address and size of the MTIP L2 SW module IO range
+
+  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
+
+  ethernet-ports:
+    type: object
+    additionalProperties: false
+
+    properties:
+      '#address-cells':
+        const: 1
+      '#size-cells':
+        const: 0
+
+    patternProperties:
+      "^port@[0-9]+$":
+        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@800f0000 {
+        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";
+
+        ethernet-ports {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                mtip_port1: port@1 {
+                        reg = <1>;
+                        label = "lan0";
+                        local-mac-address = [ 00 00 00 00 00 00 ];
+                        phy-mode = "rmii";
+                        phy-handle = <&ethphy0>;
+                };
+
+                mtip_port2: port@2 {
+                        reg = <2>;
+                        label = "lan1";
+                        local-mac-address = [ 00 00 00 00 00 00 ];
+                        phy-mode = "rmii";
+                        phy-handle = <&ethphy1>;
+                };
+        };
+
+        mdio_sw: mdio {
+                #address-cells = <1>;
+                #size-cells = <0>;
+
+                reset-gpios = <&gpio2 13 0>;
+                reset-delay-us = <25000>;
+                reset-post-delay-us = <10000>;
+
+                ethphy0: ethernet-phy@0 {
+                        reg = <0>;
+                        smsc,disable-energy-detect;
+                        /* Both PHYs (i.e. 0,1) have the same, single GPIO, */
+                        /* line to handle both, their interrupts (AND'ed) */
+                        interrupt-parent = <&gpio4>;
+                        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+                };
+
+                ethphy1: ethernet-phy@1 {
+                        reg = <1>;
+                        smsc,disable-energy-detect;
+                        interrupt-parent = <&gpio4>;
+                        interrupts = <13 IRQ_TYPE_EDGE_FALLING>;
+                };
+        };
+    };