| Message ID | 20220414122250.158113-10-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 |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 6 of 6 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | warning | WARNING: line length of 91 exceeds 80 columns |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Thu, Apr 14, 2022 at 02:22:47PM +0200, Clément Léger wrote: > Add the MII converter node which describes the MII converter that is > present on the RZ/N1 SoC. Do you have a board which actually uses this? I just noticed that renesas,miic-cfg-mode is missing, it is a required property, but maybe the board .dts file provides it? Andrew
Le Fri, 15 Apr 2022 01:22:01 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > On Thu, Apr 14, 2022 at 02:22:47PM +0200, Clément Léger wrote: > > Add the MII converter node which describes the MII converter that is > > present on the RZ/N1 SoC. > > Do you have a board which actually uses this? I just noticed that > renesas,miic-cfg-mode is missing, it is a required property, but maybe > the board .dts file provides it? > > Andrew Hi Andrew, yes, I have a board that defines and use that. The renesas,miic-cfg-mode actually configures the muxes that are present on the SoC. They allows to mux the various ethernet components (Sercos Controller, HSR Controller, Ethercat, GMAC1, RTOS-GMAC). All these muxes are actually controller by a single register CONVCTRL_MODE. You can actually see the muxes that are present in the manual [1] at Section 8 and the CONVCTRL_MODE possible values are listed on page 180. This seems to be something that is board dependent because the muxing controls the MII converter outputs which depends on the board layout. I'm open to any modification for this setup which does not really fit any abstraction that I may have seen. [1] https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-system-introduction-multiplexing-electrical-and
On Fri, Apr 15, 2022 at 10:24:53AM +0200, Clément Léger wrote: > Le Fri, 15 Apr 2022 01:22:01 +0200, > Andrew Lunn <andrew@lunn.ch> a écrit : > > > On Thu, Apr 14, 2022 at 02:22:47PM +0200, Clément Léger wrote: > > > Add the MII converter node which describes the MII converter that is > > > present on the RZ/N1 SoC. > > > > Do you have a board which actually uses this? I just noticed that > > renesas,miic-cfg-mode is missing, it is a required property, but maybe > > the board .dts file provides it? > > > > Andrew > > Hi Andrew, yes, I have a board that defines and use that. Great. Do you plan to mainline it? It is always nice to see a user. > The > renesas,miic-cfg-mode actually configures the muxes that are present on > the SoC. They allows to mux the various ethernet components (Sercos > Controller, HSR Controller, Ethercat, GMAC1, RTOS-GMAC). > All these muxes are actually controller by a single register > CONVCTRL_MODE. You can actually see the muxes that are present in the > manual [1] at Section 8 and the CONVCTRL_MODE possible values are listed > on page 180. > > This seems to be something that is board dependent because the muxing > controls the MII converter outputs which depends on the board layout. Does it also mux the MDIO lines as well? We might want to consider the name 'mux'. Linux already has the concept of a mux, e.g. an MDIO mux, and i2c mux etc. These muxes have one master device, which with the aid of the mux you can connect to multiple busses. And at runtime you flip the mux as needed to access the devices on the multiple slave busses. For MDIO you typically see this when you have multiple Ethernet switch, each has its own slave MDIO bus, and you use the mux to connect the single SOC MDIO bus master to the various slave busses as needed to perform a bus transaction. I2C is similar, you can have multiple SFPs, either with there own IC2 bus, connected via a mux to a single I2C bus controller on the SoC. I've not looked at the data sheet yet, but it sounds like it operates in a different way, so we might want to avoid 'mux'. > I'm open to any modification for this setup which does not really fit > any abstraction that I may have seen. > > [1] > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-system-introduction-multiplexing-electrical-and O.K, looking at figure 8.1. What the user wants to express is something like: Connect MI_CONV5 to SECOS PORTA Connect MI_CONV4 to ETHCAT PORTB Connect MI_CONV3 to SWITCH PORTC Connect MI_CONV2 to SWITCH PORTD plus maybe Connect SWITCH PORTIN to RTOS So i guess i would express the DT bindings like this, 5 values, and let the driver then try to figure out the value you need to put in the register, or return -EINVAL. For DT bindings we try to avoid magic values which get written into registers. We prefer a higher level description, and then let the driver figure out how to actually implement that. Andrew
Le Fri, 15 Apr 2022 16:16:46 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > On Fri, Apr 15, 2022 at 10:24:53AM +0200, Clément Léger wrote: > > Le Fri, 15 Apr 2022 01:22:01 +0200, > > Andrew Lunn <andrew@lunn.ch> a écrit : > > > > > On Thu, Apr 14, 2022 at 02:22:47PM +0200, Clément Léger wrote: > > > > Add the MII converter node which describes the MII converter that is > > > > present on the RZ/N1 SoC. > > > > > > Do you have a board which actually uses this? I just noticed that > > > renesas,miic-cfg-mode is missing, it is a required property, but maybe > > > the board .dts file provides it? > > > > > > Andrew > > > > Hi Andrew, yes, I have a board that defines and use that. > > Great. Do you plan to mainline it? It is always nice to see a user. Although we are working on a specific customer board, we will probably try to mailine this support for the RZ/N1D-DB. > > > The > > renesas,miic-cfg-mode actually configures the muxes that are present on > > the SoC. They allows to mux the various ethernet components (Sercos > > Controller, HSR Controller, Ethercat, GMAC1, RTOS-GMAC). > > All these muxes are actually controller by a single register > > CONVCTRL_MODE. You can actually see the muxes that are present in the > > manual [1] at Section 8 and the CONVCTRL_MODE possible values are listed > > on page 180. > > > > This seems to be something that is board dependent because the muxing > > controls the MII converter outputs which depends on the board layout. > > Does it also mux the MDIO lines as well? Nope, the MDIO lines are muxed using the pinctrl driver. > > We might want to consider the name 'mux'. Linux already has the > concept of a mux, e.g. an MDIO mux, and i2c mux etc. These muxes have > one master device, which with the aid of the mux you can connect to > multiple busses. And at runtime you flip the mux as needed to access > the devices on the multiple slave busses. For MDIO you typically see > this when you have multiple Ethernet switch, each has its own slave > MDIO bus, and you use the mux to connect the single SOC MDIO bus > master to the various slave busses as needed to perform a bus > transaction. I2C is similar, you can have multiple SFPs, either with > there own IC2 bus, connected via a mux to a single I2C bus controller > on the SoC. > > I've not looked at the data sheet yet, but it sounds like it operates > in a different way, so we might want to avoid 'mux'. Indeed, Let's not refer to it as mux in the code at all. If using your proposal below, I guess we could avoid that. > > > I'm open to any modification for this setup which does not really fit > > any abstraction that I may have seen. > > > > [1] > > https://www.renesas.com/us/en/document/mah/rzn1d-group-rzn1s-group-rzn1l-group-users-manual-system-introduction-multiplexing-electrical-and > > O.K, looking at figure 8.1. > > What the user wants to express is something like: > > Connect MI_CONV5 to SECOS PORTA > Connect MI_CONV4 to ETHCAT PORTB > Connect MI_CONV3 to SWITCH PORTC > Connect MI_CONV2 to SWITCH PORTD > > plus maybe > > Connect SWITCH PORTIN to RTOS Yes, that is correct. > > So i guess i would express the DT bindings like this, 5 values, and > let the driver then try to figure out the value you need to put in the > register, or return -EINVAL. For DT bindings we try to avoid magic > values which get written into registers. We prefer a higher level > description, and then let the driver figure out how to actually > implement that. Ok, looks like a more flexible way to doing it. Let's go with something like this: renesas,miic-port-connection = <PORTIN_GMAC2>, <MAC2>, <SWITCH_PORTC>, <SWITCH_PORTB>, <SWITCH_PORTA>;
> Ok, looks like a more flexible way to doing it. Let's go with something > like this: > > renesas,miic-port-connection = <PORTIN_GMAC2>, <MAC2>, <SWITCH_PORTC>, > <SWITCH_PORTB>, <SWITCH_PORTA>; Not all combinations are possible. In fact, there is a limited choice for each value. So consider getting the yaml tools to help you by listing what is valid for each setting. You might need a different format than. Also, this format it is not clear what each value refers to. renesas,miic-port-connection-mii-conv1 = <PORTIN_GMAC2>; renesas,miic-port-connection-mii-conv2 = <MAC2>; renesas,miic-port-connection-mii-conv3 = <SWITCH_PORTC>; renesas,miic-port-connection-mii-conv4 = <SWITCH_PORTB>; renesas,miic-port-connection-mii-conv5 = <SWITCH_PORTA>; is more sense documenting, and i suspect easier to make the validator work for you. Andrew
Le Fri, 15 Apr 2022 17:12:26 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > > Ok, looks like a more flexible way to doing it. Let's go with something > > like this: > > > > renesas,miic-port-connection = <PORTIN_GMAC2>, <MAC2>, <SWITCH_PORTC>, > > <SWITCH_PORTB>, <SWITCH_PORTA>; > > Not all combinations are possible. In fact, there is a limited choice > for each value. So consider getting the yaml tools to help you by > listing what is valid for each setting. You might need a different > format than. Also, this format it is not clear what each value refers > to. > > renesas,miic-port-connection-mii-conv1 = <PORTIN_GMAC2>; > renesas,miic-port-connection-mii-conv2 = <MAC2>; > renesas,miic-port-connection-mii-conv3 = <SWITCH_PORTC>; > renesas,miic-port-connection-mii-conv4 = <SWITCH_PORTB>; > renesas,miic-port-connection-mii-conv5 = <SWITCH_PORTA>; > > is more sense documenting, and i suspect easier to make the validator > work for you. While doing it, I think it probably even makes more sense to have something more structured. I currently have the following: eth-miic@44030000 { ... mii_conv0: mii-conv@0 { reg = <0>; }; mii_conv1: mii-conv@1 { reg = <1>; }; ... }; I think it would be good to modify it like this: eth-miic@44030000 { ... converters { mii_conv0: mii-conv@0 { // Even if useless, maybe keeping it for the sake of coherency renesas,miic-input = <MIIC_GMAC1>; reg = <0>; }; mii_conv1: mii-conv@1 { renesas,miic-input = <SWITCH_PORTA>; reg = <1>; }; mii_conv2: mii-conv@2 { renesas,miic-input = <SWITCH_PORTB>; reg = <2>; }; mii_conv3: mii-conv@3 { renesas,miic-input = <SWITCH_PORTC>; reg = <3>; }; mii_conv4: mii-conv@4 { renesas,miic-input = <SWITCH_PORTD>; reg = <4>; }; }; This way, it remains tied to the MII converter output port definition. I guess that the yaml definitions would still allow to restrict the values available per nodes. Validation for the final combination is probably more difficult to do using yaml. Regarding the SWITCH_PORTIN, I don't see any way to use the same definition than for the converter port and thus, a "renesas,miic-switch-portin" property seems mandatory.
> I think it would be good to modify it like this: > > eth-miic@44030000 { > ... > converters { > mii_conv0: mii-conv@0 { > // Even if useless, maybe keeping it for the sake of coherency > renesas,miic-input = <MIIC_GMAC1>; > reg = <0>; > }; This is not a 'bus', so using reg, and @0, etc is i think wrong. You just have a collection of properties. > mii_conv1: mii-conv@1 { > renesas,miic-input = <SWITCH_PORTA>; > reg = <1>; > }; > mii_conv2: mii-conv@2 { > renesas,miic-input = <SWITCH_PORTB>; > reg = <2>; > }; > mii_conv3: mii-conv@3 { > renesas,miic-input = <SWITCH_PORTC>; > reg = <3>; > }; > mii_conv4: mii-conv@4 { > renesas,miic-input = <SWITCH_PORTD>; > reg = <4>; > }; > }; > > This way, it remains tied to the MII converter output port definition. I > guess that the yaml definitions would still allow to restrict the values > available per nodes. Validation for the final combination is probably > more difficult to do using yaml. I doubt you can do full validation in YAML. But you can at least limit some of the errors. You need to do full validation in the driver anyway. Andrew
Le Fri, 15 Apr 2022 18:19:46 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > > I think it would be good to modify it like this: > > > > eth-miic@44030000 { > > ... > > converters { > > mii_conv0: mii-conv@0 { > > // Even if useless, maybe keeping it for the sake of coherency > > renesas,miic-input = <MIIC_GMAC1>; > > reg = <0>; > > }; > > This is not a 'bus', so using reg, and @0, etc is i think wrong. You > just have a collection of properties. Agreed, but this is the same thing that is done for DSA ports (at least I think). It uses reg which describe the port number, this is not a real bus per se, it only refer to port indices. But if you think this should not be done like this, what do you propose then ? These nodes are also reference from "pcs-handle" properties in switch to retrieve the PCS. Would you suggest using something like pcs-handle = <ð_miic port_index> and remove the nodes then ? Thanks,
On Fri, Apr 15, 2022 at 06:45:41PM +0200, Clément Léger wrote: > Le Fri, 15 Apr 2022 18:19:46 +0200, > Andrew Lunn <andrew@lunn.ch> a écrit : > > > > I think it would be good to modify it like this: > > > > > > eth-miic@44030000 { > > > ... > > > converters { > > > mii_conv0: mii-conv@0 { > > > // Even if useless, maybe keeping it for the sake of coherency > > > renesas,miic-input = <MIIC_GMAC1>; > > > reg = <0>; > > > }; > > > > This is not a 'bus', so using reg, and @0, etc is i think wrong. You > > just have a collection of properties. > > Agreed, but this is the same thing that is done for DSA ports (at least > I think). It uses reg which describe the port number, this is not a > real bus per se, it only refer to port indices. True. That is an old binding, before a lot of good practices were enforced. I'm not sure it would be accepted today. I suggest you make a proposal and see what the DT Maintainers say. > But if you think this should not be done like this, what do you > propose then ? These nodes are also reference from "pcs-handle" > properties in switch to retrieve the PCS. This i was not thinking about. Make this clear in the binding documentation for what you propose. Humm, this last point just gave me an idea. How are you representing the PCS in DT? Are they memory mapped? So you have a nodes something like: eth-pcs-conv1@44040100 { compatible = "acm-inc,pcs" } eth-pcs-conv2@44040200 { compatible = "acm-inc,pcs" } The MAC node than has a pcs-handle pointing to one of these nodes? You implicitly have the information you need to configure the MII muxes here. The information is a lot more distributed, but it is there. As each MAC probes, it can ask the MII MUX driver to connect its MAC to the converter pointed to by its pcs-handle. Andrew
Le Sat, 16 Apr 2022 15:48:51 +0200, Andrew Lunn <andrew@lunn.ch> a écrit : > On Fri, Apr 15, 2022 at 06:45:41PM +0200, Clément Léger wrote: > > Le Fri, 15 Apr 2022 18:19:46 +0200, > > Andrew Lunn <andrew@lunn.ch> a écrit : > > > > > > I think it would be good to modify it like this: > > > > > > > > eth-miic@44030000 { > > > > ... > > > > converters { > > > > mii_conv0: mii-conv@0 { > > > > // Even if useless, maybe keeping it for the sake of coherency > > > > renesas,miic-input = <MIIC_GMAC1>; > > > > reg = <0>; > > > > }; > > > > > > This is not a 'bus', so using reg, and @0, etc is i think wrong. You > > > just have a collection of properties. > > > > Agreed, but this is the same thing that is done for DSA ports (at least > > I think). It uses reg which describe the port number, this is not a > > real bus per se, it only refer to port indices. > > True. That is an old binding, before a lot of good practices were > enforced. I'm not sure it would be accepted today. > > I suggest you make a proposal and see what the DT Maintainers say. Acked. > > > But if you think this should not be done like this, what do you > > propose then ? These nodes are also reference from "pcs-handle" > > properties in switch to retrieve the PCS. > > This i was not thinking about. Make this clear in the binding > documentation for what you propose. > > Humm, this last point just gave me an idea. How are you representing > the PCS in DT? Are they memory mapped? So you have a nodes something > like: > > eth-pcs-conv1@44040100 { > compatible = "acm-inc,pcs" > } > > eth-pcs-conv2@44040200 { > compatible = "acm-inc,pcs" > } That is a good idea since the converter are indeed (partly) memory mapped, but the hardware guys decided that it was a good idea to share some registers. Amongst shared registers, we have the reset for each converter and the muxing control which as stated before is contained in a single register. > > The MAC node than has a pcs-handle pointing to one of these nodes? > > You implicitly have the information you need to configure the MII > muxes here. The information is a lot more distributed, but it is > there. As each MAC probes, it can ask the MII MUX driver to connect > its MAC to the converter pointed to by its pcs-handle. Hum, that could be done but since only some values/combinations are allowed, it would potentially require to validate the setting at each request, leading to potential non working devices due to invalid MUX configuration required. I think the fact that we could have everything in one single node allows to validate it at probe time. Anyway, I'll make a proposal an we'll see ! Thanks again for your feedback.
> Hum, that could be done but since only some values/combinations are > allowed, it would potentially require to validate the setting at each > request, leading to potential non working devices due to invalid MUX > configuration required. Yes, validation is messy, you have to incrementally validate as each device probes and requests its PCS. I would not only return -EINVAL, but also dump the current partial configuration to the kernel log. I guess the implementation would have a big table as shown in the datasheet. You walk the table trying to find a match for those settings you have so far, and wildcard those you don't know yet. Fun little coding problem. Andrew
On Fri, Apr 15, 2022 at 06:19:46PM +0200, Andrew Lunn wrote: > > I think it would be good to modify it like this: > > > > eth-miic@44030000 { > > ... > > converters { > > mii_conv0: mii-conv@0 { > > // Even if useless, maybe keeping it for the sake of coherency > > renesas,miic-input = <MIIC_GMAC1>; > > reg = <0>; > > }; > > This is not a 'bus', so using reg, and @0, etc is i think wrong. You > just have a collection of properties. 'reg' can be for anything you need to address, not just buses. Rob
diff --git a/arch/arm/boot/dts/r9a06g032.dtsi b/arch/arm/boot/dts/r9a06g032.dtsi index 636a6ab31c58..fd174df268e8 100644 --- a/arch/arm/boot/dts/r9a06g032.dtsi +++ b/arch/arm/boot/dts/r9a06g032.dtsi @@ -200,6 +200,39 @@ nand_controller: nand-controller@40102000 { status = "disabled"; }; + eth_miic: 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>; + clock-names = "mii_ref", "rgmii_ref", "rmii_ref", "hclk_switch_rg"; + status = "disabled"; + + 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>; + }; + }; + gic: interrupt-controller@44101000 { compatible = "arm,gic-400", "arm,cortex-a7-gic"; interrupt-controller;
Add the MII converter node which describes the MII converter that is present on the RZ/N1 SoC. Signed-off-by: Clément Léger <clement.leger@bootlin.com> --- arch/arm/boot/dts/r9a06g032.dtsi | 33 ++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)