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@xxxxxxxxxxx>
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
