| Message ID | 20220414122250.158113-4-clement.leger@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | add support for Renesas RZ/N1 ethernet subsystem devices | expand |
On Thu, 14 Apr 2022 14:22:41 +0200, Clément Léger wrote: > This MII converter can be found on the RZ/N1 processor family. The MII > converter ports are declared as subnodes which are then referenced by > users of the PCS driver such as the switch. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- > .../bindings/net/pcs/renesas,rzn1-miic.yaml | 95 +++++++++++++++++++ > include/dt-bindings/net/pcs-rzn1-miic.h | 19 ++++ > 2 files changed, 114 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml > create mode 100644 include/dt-bindings/net/pcs-rzn1-miic.h > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: ./Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml:18:7: [warning] wrong indentation: expected 4 but found 6 (indentation) ./Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml:95:7: [error] no new line character at the end of file (new-line-at-end-of-file) dtschema/dtc warnings/errors: doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. 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.
On Thu, Apr 14, 2022 at 02:22:41PM +0200, Clément Léger wrote: > This MII converter can be found on the RZ/N1 processor family. The MII > converter ports are declared as subnodes which are then referenced by > users of the PCS driver such as the switch. > > Signed-off-by: Clément Léger <clement.leger@bootlin.com> > --- > .../bindings/net/pcs/renesas,rzn1-miic.yaml | 95 +++++++++++++++++++ > include/dt-bindings/net/pcs-rzn1-miic.h | 19 ++++ > 2 files changed, 114 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml > create mode 100644 include/dt-bindings/net/pcs-rzn1-miic.h > > diff --git a/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml > new file mode 100644 > index 000000000000..ccb25ce6cbde > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml > @@ -0,0 +1,95 @@ > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/pcs/renesas,rzn1-miic.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Renesas RZ/N1 MII converter > + > +maintainers: > + - Clément Léger <clement.leger@bootlin.com> > + > +description: | > + This MII converter is present on the Renesas RZ/N1 SoC family. It is > + responsible to do MII passthrough or convert it to RMII/RGMII. > + > +properties: > + compatible: > + const: renesas,rzn1-miic Need SoC specific compatibles. > + > + reg: > + maxItems: 1 > + > + "#address-cells": > + const: 1 > + > + "#size-cells": > + const: 0 > + > + clocks: > + items: > + - description: MII reference clock > + - description: RGMII reference clock > + - description: RMII reference clock > + - description: AHB clock used for the MII converter register interface > + > + renesas,miic-cfg-mode: > + description: MII mux configuration mode. This value should use one of the > + value defined in dt-bindings/net/pcs-rzn1-miic.h. Describe possible values here as constraints. At present, I don't see the point of this property if there is only 1 possible value and it is required. > + $ref: /schemas/types.yaml#/definitions/uint32 > + > +patternProperties: > + "^mii-conv@[0-4]$": > + type: object additionalProperties: false > + description: MII converter port > + > + properties: > + reg: > + maxItems: 1 Why do you need sub-nodes? They don't have any properties. A simple mask property could tell you which ports are present/active/enabled if that's what you are tracking. Or the SoC specific compatibles you need to add can imply the ports if they are SoC specific. > + > +required: > + - compatible > + - reg > + - clocks > + - renesas,miic-cfg-mode > + - "#address-cells" > + - "#size-cells" > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/net/pcs-rzn1-miic.h> > + #include <dt-bindings/clock/r9a06g032-sysctrl.h> > + > + eth-miic@44030000 { > + compatible = "renesas,rzn1-miic"; > + #address-cells = <1>; > + #size-cells = <0>; > + reg = <0x44030000 0x10000>; > + clocks = <&sysctrl R9A06G032_CLK_MII_REF>, > + <&sysctrl R9A06G032_CLK_RGMII_REF>, > + <&sysctrl R9A06G032_CLK_RMII_REF>, > + <&sysctrl R9A06G032_HCLK_SWITCH_RG>; > + renesas,miic-cfg-mode = <MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA>; > + > + mii_conv0: mii-conv@0 { > + reg = <0>; > + }; > + > + mii_conv1: mii-conv@1 { > + reg = <1>; > + }; > + > + mii_conv2: mii-conv@2 { > + reg = <2>; > + }; > + > + mii_conv3: mii-conv@3 { > + reg = <3>; > + }; > + > + mii_conv4: mii-conv@4 { > + reg = <4>; > + }; > + }; > \ No newline at end of file Fix this. > diff --git a/include/dt-bindings/net/pcs-rzn1-miic.h b/include/dt-bindings/net/pcs-rzn1-miic.h > new file mode 100644 > index 000000000000..c5a0f382967b > --- /dev/null > +++ b/include/dt-bindings/net/pcs-rzn1-miic.h > @@ -0,0 +1,19 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ Dual license please. > +/* > + * Copyright (C) 2022 Schneider-Electric > + * > + * Clément Léger <clement.leger@bootlin.com> > + */ > + > +#ifndef _DT_BINDINGS_PCS_RZN1_MIIC > +#define _DT_BINDINGS_PCS_RZN1_MIIC > + > +/* > + * Reefer to the datasheet [1] section 8.2.1, Internal Connection of Ethernet > + * Ports to check the meaning of these values. > + * > + * [1] REN_r01uh0750ej0140-rzn1-introduction_MAT_20210228.pdf > + */ > +#define MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA 0x13 > + > +#endif > -- > 2.34.1 > >
Le Tue, 19 Apr 2022 08:43:47 -0500, Rob Herring <robh@kernel.org> a écrit : > > + clocks: > > + items: > > + - description: MII reference clock > > + - description: RGMII reference clock > > + - description: RMII reference clock > > + - description: AHB clock used for the MII converter register interface > > + > > + renesas,miic-cfg-mode: > > + description: MII mux configuration mode. This value should use one of the > > + value defined in dt-bindings/net/pcs-rzn1-miic.h. > > Describe possible values here as constraints. At present, I don't see > the point of this property if there is only 1 possible value and it is > required. The ethernet subsystem contains a number of internal muxes that allows to configure ethernet routing. This configuration option allows to set the register that configure these muxes. After talking with Andrew, I considered moving to something like this: eth-miic@44030000 { compatible = "renesas,rzn1-miic"; mii_conv1: mii-conv-1 { renesas,miic-input = <MIIC_GMAC1_PORT>; port = <1>; }; mii_conv2: mii-conv-2 { renesas,miic-input = <MIIC_SWITCHD_PORT>; port = <2>; }; ... }; Which would allow embedding the configuration inside the port sub-nodes. Moreover, it allows a better validation of the values using the schema validation directly since only a limited number of values are allowed for each port. > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > + > > +patternProperties: > > + "^mii-conv@[0-4]$": > > + type: object > > additionalProperties: false > > > + description: MII converter port > > + > > + properties: > > + reg: > > + maxItems: 1 > > Why do you need sub-nodes? They don't have any properties. A simple mask > property could tell you which ports are present/active/enabled if that's > what you are tracking. Or the SoC specific compatibles you need to add > can imply the ports if they are SoC specific. The MACs are using phandles to these sub-nodes to query a specific MII converter port PCS: switch@44050000 { compatible = "renesas,rzn1-a5psw"; ports { port@0 { reg = <0>; label = "lan0"; phy-handle = <&switch0phy3>; pcs-handle = <&mii_conv4>; }; }; }; According to Andrew, this is not a good idea to represent the PCS as a bus since it is indeed not a bus. I could also switch to something like pcs-handle = <ð_mii 4> but i'm not sure what you'd prefer. We could also remove this from the device-tree and consider each driver to request the MII ouput to be configured using something like this for instance: miic_request_pcs(pcs_np, miic_port_nr, MIIC_SWITCHD_PORT); But I'm not really fan of this because it requires the drivers to assume some specificities of the MII converter (port number are not in the same order of the switch for instance) and thus I would prefer this to be in the device-tree. Let me know if you can think of something that would suit you better but keep in mind that I need to correctly match a switch/MAC port with a PCS port and that I also need to configure MII internal muxes. For more information, you can look at section 8 of the manual at [1]. Thanks, [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-system-introduction-multiplexing-electrical-and
On Tue, Apr 19, 2022 at 05:00:44PM +0200, Clément Léger wrote: > Le Tue, 19 Apr 2022 08:43:47 -0500, > Rob Herring <robh@kernel.org> a écrit : > > > > + clocks: > > > + items: > > > + - description: MII reference clock > > > + - description: RGMII reference clock > > > + - description: RMII reference clock > > > + - description: AHB clock used for the MII converter register interface > > > + > > > + renesas,miic-cfg-mode: > > > + description: MII mux configuration mode. This value should use one of the > > > + value defined in dt-bindings/net/pcs-rzn1-miic.h. > > > > Describe possible values here as constraints. At present, I don't see > > the point of this property if there is only 1 possible value and it is > > required. > > The ethernet subsystem contains a number of internal muxes that allows > to configure ethernet routing. This configuration option allows to set > the register that configure these muxes. > > After talking with Andrew, I considered moving to something like this: > > eth-miic@44030000 { > compatible = "renesas,rzn1-miic"; > > mii_conv1: mii-conv-1 { > renesas,miic-input = <MIIC_GMAC1_PORT>; > port = <1>; 'port' is already used, find another name. Maybe 'reg' here. Don't know. What do 1 and 2 represent? > }; > mii_conv2: mii-conv-2 { > renesas,miic-input = <MIIC_SWITCHD_PORT>; > port = <2>; > }; > ... > }; > > Which would allow embedding the configuration inside the port > sub-nodes. Moreover, it allows a better validation of the values using > the schema validation directly since only a limited number of values > are allowed for each port. > > > > > > + $ref: /schemas/types.yaml#/definitions/uint32 > > > + > > > +patternProperties: > > > + "^mii-conv@[0-4]$": > > > + type: object > > > > additionalProperties: false > > > > > + description: MII converter port > > > + > > > + properties: > > > + reg: > > > + maxItems: 1 > > > > Why do you need sub-nodes? They don't have any properties. A simple mask > > property could tell you which ports are present/active/enabled if that's > > what you are tracking. Or the SoC specific compatibles you need to add > > can imply the ports if they are SoC specific. > > The MACs are using phandles to these sub-nodes to query a specific MII > converter port PCS: > > switch@44050000 { > compatible = "renesas,rzn1-a5psw"; > > ports { > port@0 { ethernet-ports and ethernet-port so we don't collide with the graph binding. > reg = <0>; > label = "lan0"; > phy-handle = <&switch0phy3>; > pcs-handle = <&mii_conv4>; > }; > }; > }; > > According to Andrew, this is not a good idea to represent the PCS as a > bus since it is indeed not a bus. I could also switch to something like > pcs-handle = <ð_mii 4> but i'm not sure what you'd prefer. We could > also remove this from the device-tree and consider each driver to > request the MII ouput to be configured using something like this for > instance: I'm pretty sure we already defined pcs-handle as only a phandle. If you want variable cells, then it's got to be extended like all the other properties using that pattern. > > miic_request_pcs(pcs_np, miic_port_nr, MIIC_SWITCHD_PORT); > > But I'm not really fan of this because it requires the drivers to > assume some specificities of the MII converter (port number are not in > the same order of the switch for instance) and thus I would prefer this > to be in the device-tree. > > Let me know if you can think of something that would suit you > better but keep in mind that I need to correctly match a switch/MAC > port with a PCS port and that I also need to configure MII internal > muxes. > > For more information, you can look at section 8 of the manual at [1]. I can't really. Other people want their bindings reviewed too. Rob
Le Wed, 20 Apr 2022 15:11:26 -0500, Rob Herring <robh@kernel.org> a écrit : > On Tue, Apr 19, 2022 at 05:00:44PM +0200, Clément Léger wrote: > > Le Tue, 19 Apr 2022 08:43:47 -0500, > > Rob Herring <robh@kernel.org> a écrit : > > > > > > + clocks: > > > > + items: > > > > + - description: MII reference clock > > > > + - description: RGMII reference clock > > > > + - description: RMII reference clock > > > > + - description: AHB clock used for the MII converter register interface > > > > + > > > > + renesas,miic-cfg-mode: > > > > + description: MII mux configuration mode. This value should use one of the > > > > + value defined in dt-bindings/net/pcs-rzn1-miic.h. > > > > > > Describe possible values here as constraints. At present, I don't see > > > the point of this property if there is only 1 possible value and it is > > > required. > > > > The ethernet subsystem contains a number of internal muxes that allows > > to configure ethernet routing. This configuration option allows to set > > the register that configure these muxes. > > > > After talking with Andrew, I considered moving to something like this: > > > > eth-miic@44030000 { > > compatible = "renesas,rzn1-miic"; > > > > mii_conv1: mii-conv-1 { > > renesas,miic-input = <MIIC_GMAC1_PORT>; > > port = <1>; > > 'port' is already used, find another name. Maybe 'reg' here. Don't know. > What do 1 and 2 represent? 'port' represent the port index in the MII converter IP. I went with reg first, but according to Andrew feedback, it looked like it was a bad idea since it was not really a "bus". However, this pattern is already used for dsa ports. > > > > > > Why do you need sub-nodes? They don't have any properties. A simple mask > > > property could tell you which ports are present/active/enabled if that's > > > what you are tracking. Or the SoC specific compatibles you need to add > > > can imply the ports if they are SoC specific. > > > > The MACs are using phandles to these sub-nodes to query a specific MII > > converter port PCS: > > > > switch@44050000 { > > compatible = "renesas,rzn1-a5psw"; > > > > ports { > > port@0 { > > ethernet-ports and ethernet-port so we don't collide with the graph > binding. Acked. > > > > reg = <0>; > > label = "lan0"; > > phy-handle = <&switch0phy3>; > > pcs-handle = <&mii_conv4>; > > }; > > }; > > }; > > > > According to Andrew, this is not a good idea to represent the PCS as a > > bus since it is indeed not a bus. I could also switch to something like > > pcs-handle = <ð_mii 4> but i'm not sure what you'd prefer. We could > > also remove this from the device-tree and consider each driver to > > request the MII ouput to be configured using something like this for > > instance: > > I'm pretty sure we already defined pcs-handle as only a phandle. If you > want variable cells, then it's got to be extended like all the other > properties using that pattern. Yep, it seems to be used in some other driver as a single phandle too. I'll kept that. > > > > > miic_request_pcs(pcs_np, miic_port_nr, MIIC_SWITCHD_PORT); > > > > But I'm not really fan of this because it requires the drivers to > > assume some specificities of the MII converter (port number are not in > > the same order of the switch for instance) and thus I would prefer this > > to be in the device-tree. > > > > Let me know if you can think of something that would suit you > > better but keep in mind that I need to correctly match a switch/MAC > > port with a PCS port and that I also need to configure MII internal > > muxes. > > > > For more information, you can look at section 8 of the manual at [1]. > > I can't really. Other people want their bindings reviewed too. No worries. > > Rob
diff --git a/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml new file mode 100644 index 000000000000..ccb25ce6cbde --- /dev/null +++ b/Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml @@ -0,0 +1,95 @@ +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/pcs/renesas,rzn1-miic.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Renesas RZ/N1 MII converter + +maintainers: + - Clément Léger <clement.leger@bootlin.com> + +description: | + This MII converter is present on the Renesas RZ/N1 SoC family. It is + responsible to do MII passthrough or convert it to RMII/RGMII. + +properties: + compatible: + const: renesas,rzn1-miic + + reg: + maxItems: 1 + + "#address-cells": + const: 1 + + "#size-cells": + const: 0 + + clocks: + items: + - description: MII reference clock + - description: RGMII reference clock + - description: RMII reference clock + - description: AHB clock used for the MII converter register interface + + renesas,miic-cfg-mode: + description: MII mux configuration mode. This value should use one of the + value defined in dt-bindings/net/pcs-rzn1-miic.h. + $ref: /schemas/types.yaml#/definitions/uint32 + +patternProperties: + "^mii-conv@[0-4]$": + type: object + description: MII converter port + + properties: + reg: + maxItems: 1 + +required: + - compatible + - reg + - clocks + - renesas,miic-cfg-mode + - "#address-cells" + - "#size-cells" + +additionalProperties: false + +examples: + - | + #include <dt-bindings/net/pcs-rzn1-miic.h> + #include <dt-bindings/clock/r9a06g032-sysctrl.h> + + eth-miic@44030000 { + compatible = "renesas,rzn1-miic"; + #address-cells = <1>; + #size-cells = <0>; + reg = <0x44030000 0x10000>; + clocks = <&sysctrl R9A06G032_CLK_MII_REF>, + <&sysctrl R9A06G032_CLK_RGMII_REF>, + <&sysctrl R9A06G032_CLK_RMII_REF>, + <&sysctrl R9A06G032_HCLK_SWITCH_RG>; + renesas,miic-cfg-mode = <MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA>; + + mii_conv0: mii-conv@0 { + reg = <0>; + }; + + mii_conv1: mii-conv@1 { + reg = <1>; + }; + + mii_conv2: mii-conv@2 { + reg = <2>; + }; + + mii_conv3: mii-conv@3 { + reg = <3>; + }; + + mii_conv4: mii-conv@4 { + reg = <4>; + }; + }; \ No newline at end of file diff --git a/include/dt-bindings/net/pcs-rzn1-miic.h b/include/dt-bindings/net/pcs-rzn1-miic.h new file mode 100644 index 000000000000..c5a0f382967b --- /dev/null +++ b/include/dt-bindings/net/pcs-rzn1-miic.h @@ -0,0 +1,19 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2022 Schneider-Electric + * + * Clément Léger <clement.leger@bootlin.com> + */ + +#ifndef _DT_BINDINGS_PCS_RZN1_MIIC +#define _DT_BINDINGS_PCS_RZN1_MIIC + +/* + * Reefer to the datasheet [1] section 8.2.1, Internal Connection of Ethernet + * Ports to check the meaning of these values. + * + * [1] REN_r01uh0750ej0140-rzn1-introduction_MAT_20210228.pdf + */ +#define MIIC_MUX_MAC2_MAC1_SWD_SWC_SWB_SWA 0x13 + +#endif
This MII converter can be found on the RZ/N1 processor family. The MII converter ports are declared as subnodes which are then referenced by users of the PCS driver such as the switch. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- .../bindings/net/pcs/renesas,rzn1-miic.yaml | 95 +++++++++++++++++++ include/dt-bindings/net/pcs-rzn1-miic.h | 19 ++++ 2 files changed, 114 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/pcs/renesas,rzn1-miic.yaml create mode 100644 include/dt-bindings/net/pcs-rzn1-miic.h