| Message ID | 20241210120443.1813-1-jesse.vangavere@scioteq.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] dt-bindings: net: dsa: microchip,ksz: Improve example to a working one | expand |
On Tue, Dec 10, 2024 at 01:04:43PM +0100, Jesse Van Gavere wrote: > Currently the example will not work when implemented as-is as there are > some properties and nodes missing. > - Define two eth ports, one for each switch for clarity. > - Add mandatory dsa,member properties as it's a dual switch setup example. > - Add the mdio node for each switch and define the PHYs under it. > - Add a phy-mode and phy-handle to each port otherwise they won't come up. > - Add a mac-address property, without this the port does not come up, in > the example all 0 is used so the port replicates MAC from the CPU port To some extent, the example is for the properties defined in the binding. For properties defined in other bindings, you should look at the examples in other bindings, and then glue it all together in a real .dts file. I don't know if Rob will accept this patch. Andrew
Hi Andrew Op di 10 dec 2024 om 17:18 schreef Andrew Lunn <andrew@lunn.ch>: > > To some extent, the example is for the properties defined in the > binding. For properties defined in other bindings, you should look at > the examples in other bindings, and then glue it all together in a > real .dts file. > > I don't know if Rob will accept this patch. > > Andrew To some extent I understand that perspective, but in this case for example dsa-port itself has no example, I also struggled quite a bit getting the example going (admittedly a bit due to my lack of experience, figuring out the dt-schema tool really helped as well). In most cases when I look at an example I see the full scope of what it should be (or at least enough to get started from there), and this one seemed a bit off in that regard as it showcases itself as an example but is missing some critical details. The microchip,lan yaml in contrast does define these properties and I think it gives a much clearer picture of how to implement it in a device tree than expecting people to figure out all the places they need to look and how to glue that together. I don't think it's straightforward for most people getting into device trees to know how to glue that all together and knowing where to look to begin with, in that sense I figured updating this example might help others trying to get this up and running. Best regards, Jesse
On Tue, Dec 10, 2024 at 01:04:43PM +0100, Jesse Van Gavere wrote: > Currently the example will not work when implemented as-is as there are > some properties and nodes missing. > - Define two eth ports, one for each switch for clarity. For clarity of what? That's just example, not complete code. Aren't you adding unrelated pieces - ones not being part of this binding? > - Add mandatory dsa,member properties as it's a dual switch setup example. > - Add the mdio node for each switch and define the PHYs under it. > - Add a phy-mode and phy-handle to each port otherwise they won't come up. > - Add a mac-address property, without this the port does not come up, in > the example all 0 is used so the port replicates MAC from the CPU port > > Signed-off-by: Jesse Van Gavere <jesse.vangavere@scioteq.com> Mismatched SoB. Please run scripts/checkpatch.pl and fix reported warnings. Then please run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings. Some warnings can be ignored, especially from --strict run, but the code here looks like it needs a fix. Feel free to get in touch if the warning is not clear. > --- > .../bindings/net/dsa/microchip,ksz.yaml | 89 +++++++++++++++++-- > 1 file changed, 83 insertions(+), 6 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > index 62ca63e8a26f..a08ec0fd01fa 100644 > --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml > @@ -145,13 +145,19 @@ examples: > - | > #include <dt-bindings/gpio/gpio.h> > > - // Ethernet switch connected via SPI to the host, CPU port wired to eth0: > + // Ethernet switches connected via SPI to the host, CPU ports wired to eth0 and eth1: > eth0 { > fixed-link { > speed = <1000>; > full-duplex; > }; > }; > + eth1 { > + fixed-link { > + speed = <1000>; > + full-duplex; > + }; > + }; > > spi { > #address-cells = <1>; > @@ -167,28 +173,46 @@ examples: > > spi-max-frequency = <44000000>; > > + dsa,member = <0 0>; > + > ethernet-ports { > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > label = "lan1"; > + phy-mode = "internal"; > + phy-handle = <&switch0_phy0>; > + // The MAC is duplicated from the CPU port when all 0 > + mac-address = [00 00 00 00 00 00]; > }; > port@1 { > reg = <1>; > label = "lan2"; > + phy-mode = "internal"; > + phy-handle = <&switch0_phy1>; > + mac-address = [00 00 00 00 00 00]; > }; > port@2 { > reg = <2>; > label = "lan3"; > + phy-mode = "internal"; > + phy-handle = <&switch0_phy2>; > + mac-address = [00 00 00 00 00 00]; > }; > port@3 { > reg = <3>; > label = "lan4"; > + phy-mode = "internal"; > + phy-handle = <&switch0_phy3>; > + mac-address = [00 00 00 00 00 00]; > }; > port@4 { > reg = <4>; > label = "lan5"; > + phy-mode = "internal"; > + phy-handle = <&switch0_phy4>; > + mac-address = [00 00 00 00 00 00]; > }; > port@5 { > reg = <5>; > @@ -201,6 +225,27 @@ examples: > }; > }; > }; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch0_phy0: ethernet-phy@0 { > + reg = <0>; > + }; > + switch0_phy1: ethernet-phy@1 { > + reg = <1>; > + }; > + switch0_phy2: ethernet-phy@2 { > + reg = <2>; > + }; > + switch0_phy3: ethernet-phy@3 { > + reg = <3>; > + }; > + switch0_phy4: ethernet-phy@4 { > + reg = <4>; > + }; > + }; > }; > > ksz8565: switch@1 { > @@ -209,28 +254,42 @@ examples: > > spi-max-frequency = <44000000>; > > + dsa,member = <1 0>; > + > ethernet-ports { > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > - label = "lan1"; > + label = "lan6"; What's wrong with lan1? Just drop the second switch node, why do you need it in the first place? We expect only one example, unless they differ which is not the case - they are the same (difference in compatible does not count, lacking gpios is rather negative indication of needingi the example). More examples, more code to maintain, more bugs, more issues - see further comment. > + phy-mode = "internal"; > + phy-handle = <&switch1_phy0>; > + mac-address = [00 00 00 00 00 00]; > }; > port@1 { > reg = <1>; > - label = "lan2"; > + label = "lan7"; > + phy-mode = "internal"; > + phy-handle = <&switch1_phy1>; > + mac-address = [00 00 00 00 00 00]; > }; > port@2 { > reg = <2>; > - label = "lan3"; > + label = "lan8"; > + phy-mode = "internal"; > + phy-handle = <&switch1_phy2>; > + mac-address = [00 00 00 00 00 00]; > }; > port@3 { > reg = <3>; > - label = "lan4"; > + label = "lan9"; > + phy-mode = "internal"; > + phy-handle = <&switch1_phy3>; > + mac-address = [00 00 00 00 00 00]; > }; > port@6 { > reg = <6>; > - ethernet = <ð0>; > + ethernet = <ð1>; > phy-mode = "rgmii"; > > fixed-link { > @@ -239,6 +298,24 @@ examples: > }; > }; > }; > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + switch1_phy0: ethernet-phy@0 { > + reg = <0>; > + }; > + switch1_phy1: ethernet-phy@1 { > + reg = <1>; Messed alignment. Best regards, Krzysztof
On Tue, Dec 10, 2024 at 11:01:02PM +0100, Jesse Van Gavere wrote: > Hi Andrew > > Op di 10 dec 2024 om 17:18 schreef Andrew Lunn <andrew@lunn.ch>: > > > > To some extent, the example is for the properties defined in the > > binding. For properties defined in other bindings, you should look at > > the examples in other bindings, and then glue it all together in a > > real .dts file. > > > > I don't know if Rob will accept this patch. > > > > Andrew > > To some extent I understand that perspective, but in this case for > example dsa-port itself has no example, I also struggled quite a bit > getting the example going (admittedly a bit due to my lack of The point of the example is to show this device, not everything, so adding there nodes which are not covered by the binding is usually not what we expect. For example what ethernet ports are might be pretty obvious, considering they are already defined by child schema which is supposed to bring you full example and full description, thus parent schema does not have to be detailed. Best regards, Krzysztof
Hello Krystof, I can understand that from a code maintenance point of view, in that case I will not continue with this patch, thank you for the feedback. Best regards, Jesse Op vr 13 dec 2024 om 10:50 schreef Krzysztof Kozlowski <krzk@kernel.org>: > > The point of the example is to show this device, not everything, so > adding there nodes which are not covered by the binding is usually not > what we expect. > > For example what ethernet ports are might be pretty obvious, considering > they are already defined by child schema which is supposed to bring you > full example and full description, thus parent schema does not have to > be detailed. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml index 62ca63e8a26f..a08ec0fd01fa 100644 --- a/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml +++ b/Documentation/devicetree/bindings/net/dsa/microchip,ksz.yaml @@ -145,13 +145,19 @@ examples: - | #include <dt-bindings/gpio/gpio.h> - // Ethernet switch connected via SPI to the host, CPU port wired to eth0: + // Ethernet switches connected via SPI to the host, CPU ports wired to eth0 and eth1: eth0 { fixed-link { speed = <1000>; full-duplex; }; }; + eth1 { + fixed-link { + speed = <1000>; + full-duplex; + }; + }; spi { #address-cells = <1>; @@ -167,28 +173,46 @@ examples: spi-max-frequency = <44000000>; + dsa,member = <0 0>; + ethernet-ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; label = "lan1"; + phy-mode = "internal"; + phy-handle = <&switch0_phy0>; + // The MAC is duplicated from the CPU port when all 0 + mac-address = [00 00 00 00 00 00]; }; port@1 { reg = <1>; label = "lan2"; + phy-mode = "internal"; + phy-handle = <&switch0_phy1>; + mac-address = [00 00 00 00 00 00]; }; port@2 { reg = <2>; label = "lan3"; + phy-mode = "internal"; + phy-handle = <&switch0_phy2>; + mac-address = [00 00 00 00 00 00]; }; port@3 { reg = <3>; label = "lan4"; + phy-mode = "internal"; + phy-handle = <&switch0_phy3>; + mac-address = [00 00 00 00 00 00]; }; port@4 { reg = <4>; label = "lan5"; + phy-mode = "internal"; + phy-handle = <&switch0_phy4>; + mac-address = [00 00 00 00 00 00]; }; port@5 { reg = <5>; @@ -201,6 +225,27 @@ examples: }; }; }; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + switch0_phy0: ethernet-phy@0 { + reg = <0>; + }; + switch0_phy1: ethernet-phy@1 { + reg = <1>; + }; + switch0_phy2: ethernet-phy@2 { + reg = <2>; + }; + switch0_phy3: ethernet-phy@3 { + reg = <3>; + }; + switch0_phy4: ethernet-phy@4 { + reg = <4>; + }; + }; }; ksz8565: switch@1 { @@ -209,28 +254,42 @@ examples: spi-max-frequency = <44000000>; + dsa,member = <1 0>; + ethernet-ports { #address-cells = <1>; #size-cells = <0>; port@0 { reg = <0>; - label = "lan1"; + label = "lan6"; + phy-mode = "internal"; + phy-handle = <&switch1_phy0>; + mac-address = [00 00 00 00 00 00]; }; port@1 { reg = <1>; - label = "lan2"; + label = "lan7"; + phy-mode = "internal"; + phy-handle = <&switch1_phy1>; + mac-address = [00 00 00 00 00 00]; }; port@2 { reg = <2>; - label = "lan3"; + label = "lan8"; + phy-mode = "internal"; + phy-handle = <&switch1_phy2>; + mac-address = [00 00 00 00 00 00]; }; port@3 { reg = <3>; - label = "lan4"; + label = "lan9"; + phy-mode = "internal"; + phy-handle = <&switch1_phy3>; + mac-address = [00 00 00 00 00 00]; }; port@6 { reg = <6>; - ethernet = <ð0>; + ethernet = <ð1>; phy-mode = "rgmii"; fixed-link { @@ -239,6 +298,24 @@ examples: }; }; }; + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + switch1_phy0: ethernet-phy@0 { + reg = <0>; + }; + switch1_phy1: ethernet-phy@1 { + reg = <1>; + }; + switch1_phy2: ethernet-phy@2 { + reg = <2>; + }; + switch1_phy3: ethernet-phy@3 { + reg = <3>; + }; + }; }; }; ...
Currently the example will not work when implemented as-is as there are some properties and nodes missing. - Define two eth ports, one for each switch for clarity. - Add mandatory dsa,member properties as it's a dual switch setup example. - Add the mdio node for each switch and define the PHYs under it. - Add a phy-mode and phy-handle to each port otherwise they won't come up. - Add a mac-address property, without this the port does not come up, in the example all 0 is used so the port replicates MAC from the CPU port Signed-off-by: Jesse Van Gavere <jesse.vangavere@scioteq.com> --- .../bindings/net/dsa/microchip,ksz.yaml | 89 +++++++++++++++++-- 1 file changed, 83 insertions(+), 6 deletions(-)