Re: [PATCH 3/9] net: dsa: mv88e6xxx: implement get_phy_address

[Lukasz Majewski <lukma@denx.de>]

[-- Attachment #1: Type: text/plain, Size: 5521 bytes --]

Hi Vladimir,

> On Tue, Nov 08, 2022 at 09:23:24AM +0100, Lukasz Majewski wrote:
> > From: Matthias Schiffer <matthias.schiffer@ew.tq-group.com>
> > 
> > Avoid the need to specify a PHY for each physical port in the
> > device tree when phy_base_addr is not 0 (6250 and 6341 families).
> > 
> > This change should be backwards-compatible with existing device
> > trees, as it only adds sensible defaults where explicit definitions
> > were required before.
> > 
> > Signed-off-by: Matthias Schiffer
> > <matthias.schiffer@ew.tq-group.com>  
> 
> Needs your Signed-off-by tag as well.

Ok.

> 
> > ---  
> 
> Would it be possible to do like armada-3720-turris-mox.dts does, and
> put the phy-handle in the device tree, avoiding the need for so many
> PHY address translation quirks?

As far as I can tell - the mv88e6xxx driver
(./drivers/net/dsa/mv88e6xxx) now uses hardcoded values for each member
of mv88e6xxx_info struct.

Those values are "port_base_addr" and "phy_base_addr". Those values
could be read from DTS description as pasted below.

> 
> If you're going to have U-Boot support for this switch as well, the
> phy-handle mechanism is the only thing that U-Boot supports, so device
> trees written in this way will work for both (and can be passed by
> U-Boot to Linux):
> 
> 	switch1@11 {
> 		compatible = "marvell,mv88e6190";
> 		reg = <0x11>;
> 		dsa,member = <0 1>;
> 		interrupt-parent = <&moxtet>;
> 		interrupts = <MOXTET_IRQ_PERIDOT(1)>;
> 		status = "disabled";
> 
> 		mdio {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			switch1phy1: switch1phy1@1 {
> 				reg = <0x1>;
> 			};
> 
> 			switch1phy2: switch1phy2@2 {
> 				reg = <0x2>;
> 			};
> 
> 			switch1phy3: switch1phy3@3 {
> 				reg = <0x3>;
> 			};
> 
> 			switch1phy4: switch1phy4@4 {
> 				reg = <0x4>;
> 			};
> 
> 			switch1phy5: switch1phy5@5 {
> 				reg = <0x5>;
> 			};
> 
> 			switch1phy6: switch1phy6@6 {
> 				reg = <0x6>;
> 			};
> 
> 			switch1phy7: switch1phy7@7 {
> 				reg = <0x7>;
> 			};
> 
> 			switch1phy8: switch1phy8@8 {
> 				reg = <0x8>;
> 			};
> 		};
> 
> 		ports {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 
> 			port@1 {
> 				reg = <0x1>;
> 				label = "lan9";
> 				phy-handle = <&switch1phy1>;
> 			};
> 
> 			port@2 {
> 				reg = <0x2>;
> 				label = "lan10";
> 				phy-handle = <&switch1phy2>;
> 			};
> 
> 			port@3 {
> 				reg = <0x3>;
> 				label = "lan11";
> 				phy-handle = <&switch1phy3>;
> 			};
> 
> 			port@4 {
> 				reg = <0x4>;
> 				label = "lan12";
> 				phy-handle = <&switch1phy4>;
> 			};
> 
> 			port@5 {
> 				reg = <0x5>;
> 				label = "lan13";
> 				phy-handle = <&switch1phy5>;
> 			};
> 
> 			port@6 {
> 				reg = <0x6>;
> 				label = "lan14";
> 				phy-handle = <&switch1phy6>;
> 			};
> 
> 			port@7 {
> 				reg = <0x7>;
> 				label = "lan15";
> 				phy-handle = <&switch1phy7>;
> 			};
> 
> 			port@8 {
> 				reg = <0x8>;
> 				label = "lan16";
> 				phy-handle = <&switch1phy8>;
> 			};
> 
> 			switch1port9: port@9 {
> 				reg = <0x9>;
> 				label = "dsa";
> 				phy-mode = "2500base-x";
> 				managed = "in-band-status";
> 				link = <&switch0port10>;
> 			};
> 
> 			switch1port10: port@a {
> 				reg = <0xa>;
> 				label = "dsa";
> 				phy-mode = "2500base-x";
> 				managed = "in-band-status";
> 				link = <&switch2port9>;
> 				status = "disabled";
> 			};
> 
> 			port-sfp@a {
> 				reg = <0xa>;
> 				label = "sfp";
> 				sfp = <&sfp>;
> 				phy-mode = "sgmii";
> 				managed = "in-band-status";
> 				status = "disabled";
> 			};
> 		};
> 	};

The u-boot mailine has basic support for mv88e6071 and mv88e6020 (and
also some 'extension' patches which are floating around [1]).

For the current code - I'm using:

 mdio {
        #address-cells = <1>;
        #size-cells = <0>;

        switch@0 {
            compatible = "marvell,mv88e6250";
            reg = <0x00>;

            interrupt-parent = <&gpio2>;
            interrupts = <20 IRQ_TYPE_LEVEL_LOW>;
            interrupt-controller;
            #interrupt-cells = <2>;

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

                port@0 {
                    reg = <0>;
                    label = "lan1";
                };

                port@1 {
                    reg = <1>;
                    label = "lan2";
                };

                port@2 {
                    reg = <2>;
                    label = "lan3";
                };

                port@5 {
                    reg = <5>;
                    label = "cpu";
                    phy-mode = "rgmii-id";
                    ethernet = <&fec1>;

                    fixed-link {
                           speed = <100>;
                           full-duplex;
                    };
                };
	};


The only "hack" which I see from time to time is the replacement of
'switch@0' with 'switch@8' to take into account the R0_LED/ADDRES4
bootstrap pin value (to shift up ports addresses).


Links:

[1] - https://lists.denx.de/pipermail/u-boot/2021-March/444827.html



Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma@denx.de

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]