dsa driver for mv88e61xx

[Tim Harvey]

On Thu, Mar 10, 2022 at 8:50 AM Vladimir Oltean <olteanv at gmail.com> wrote:
>
> Hello Tim,
>
> On Thu, Mar 10, 2022 at 08:16:13AM -0800, Tim Harvey wrote:
> > Greetings,
> >
> > I wanted to take a stab at adding dsa support for the mv88e61xx which
> > currently has a driver in drivers/net/phy [1]. The board I have
> > available to me is the gw5904 which has a mv88e6085 with the upstream
> > port connected to the IMX6 FEC MAC over RGMII. This currently works
> > with the existing mv88e61xx driver with the following defined in
> > gwventana_gw5904_defconfig:
> >
> > CONFIG_MV88E61XX_SWITCH=y
> > CONFIG_MV88E61XX_CPU_PORT=5
> > CONFIG_MV88E61XX_PHY_PORTS=0xf
> > CONFIG_MV88E61XX_FIXED_PORTS=0x0
> >
> > The device-tree is arch/arm/dts/imx6qdl-gw5904.dtsi [2] which I
> > believe is proper and works in Linux with the Linux driver in
> > drivers/net/dsa/mv88e6xxx [3].
> >
> > &fec {
> >         pinctrl-names = "default";
> >         pinctrl-0 = <&pinctrl_enet>;
> >         phy-mode = "rgmii-id";
> >         phy-reset-gpios = <&gpio1 30 GPIO_ACTIVE_LOW>;
> >         phy-reset-duration = <10>;
> >         phy-reset-post-delay = <100>;
> >         status = "okay";
> >
> >         fixed-link {
> >                 speed = <1000>;
> >                 full-duplex;
> >         };
> >
> >         mdio {
> >                 #address-cells = <1>;
> >                 #size-cells = <0>;
> >
> >                 switch at 0 {
> >                         compatible = "marvell,mv88e6085";
> >                         reg = <0>;
> >
> >                         ports {
> >                                 #address-cells = <1>;
> >                                 #size-cells = <0>;
> >
> >                                 port at 0 {
> >                                         reg = <0>;
> >                                         label = "lan4";
> >                                 };
> >
> >                                 port at 1 {
> >                                         reg = <1>;
> >                                         label = "lan3";
> >                                 };
> >
> >                                 port at 2 {
> >                                         reg = <2>;
> >                                         label = "lan2";
> >                                 };
> >
> >                                 port at 3 {
> >                                         reg = <3>;
> >                                         label = "lan1";
> >                                 };
> >
> >                                 port at 5 {
> >                                         reg = <5>;
> >                                         label = "cpu";
> >                                         ethernet = <&fec>;
> >                                 };
> >                         };
> >                 };
> >         };
> > };
> >
> > My motivation for doing this is to be able to drop
> > gwventana_gw5904_defconfig as it is the same defconfig as
> > gwventana_emmc_defconfig with the switch added and with get_phy_id
> > overridden by the current mv88e61xx driver that config won't work with
> > boards that lack the switch.
> >
> > My first approach was to just put a #if !defined(CONFIG_DM_DSA) around
> > mv88e61xx get_phy_id and add a skeleton driver with an of_match of
> > compatible = "marvell,mv88e6085" but the driver does not probe with
> > the above dt fragment.
> >
> > Any ideas why the driver won't probe and advise on how I should
> > proceed with this? I'm not clear yet if I can just modify the existing
> > driver or if I should create a new one.
> >
> > Best Regards,
> >
> > Tim
> > [1] https://elixir.bootlin.com/u-boot/latest/source/drivers/net/phy/mv88e61xx.c
> > [2] https://elixir.bootlin.com/u-boot/latest/source/arch/arm/dts/imx6qdl-gw5904.dtsi
> > [3] https://elixir.bootlin.com/linux/latest/source/drivers/net/dsa/mv88e6xxx/chip.c
>
> I spent about 3 minutes looking at mv88e61xx.c, so take what I'm about
> to say with a grain of salt.
>
> The biggest issue with reusing that code seems to be that it uses struct
> phy_device throughout. A DM_DSA driver would need to work around a
> struct udevice. I'd probably create a new driver, make it easy for
> others to use, then delete old driver. I see there are 5 occurrences of
> CONFIG_MV88E61XX_PHY_PORTS in defconfigs, all added within the past 4
> years. One is yours. Could have been a lot worse.
>
> As to why your driver is not probing. I think the fec_mxc parent MDIO
> bus must be a udevice as well, registered using DM_MDIO?

Vladimir,

Ok, I see that even though fec_mxc is DM_ETH it doesn't use DM_MDIO. I
suppose my ksz9477 dsa driver probed because that switch was hanging
off I2C and DM_I2C was in use as opposed to this time the switch is
hanging off of MDIO.

With changes to fec_mxc to use DM_MDIO/UCLASS_MDIO for the its mdio
bus, I see my UCLASS_MDIO bus getting probed and registering the bus
but the call to dm_eth_phy_connect(fec) fails.

For the situation I have where the fec_mxc provides the MDIO bus
connected to the switch as well as the MAC that's connected via rgmii
to the upstream port I believe I need a 'phy-handle' in my fec node vs
a 'fixed-link' subnode otherwise dm_eth_connect_phy_handle() won't
probe my mdio device. But then I find that dm_eth_phy_connect(fec)
calls dm_eth_connect_phy_handle() which probes my UCLASS_MDIO but the
call to dm_mdio_phy_connect() fails and my UCLASS_DSA device never
gets probed.

So I guess I'm confused if I should be using fixed-link or not and if
so, how does the UCLASS_MDIO device get probed?

&fec {
        pinctrl-names = "default"
#if 1 // required for dm_eth_connect_phy_handle to probe the UCLASS_MDIO device
        phy-handle = "switch";
#endif
        phy-mode = "rgmii-id";
        status = "okay";

#if 0 // if present phy_connect is called immediate in
dm_eth_connect_phy_handle and the UCLASS_MDIO device is not probed
        fixed-link {
                speed = <1000>;
                full-duplex;
        };
#endif

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

                switch: switch at 0 {
                        compatible = "marvell,mv88e6085";
                        reg = <0>;

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

                                port at 0 {
                                        reg = <0>;
                                        label = "lan4";
                                        phy-mode = "internal";
                                };

...
                                port at 5 {
                                        reg = <5>;
                                        label = "cpu";
                                        ethernet = <&fec>;
                                        phy-mode = "rgmii-id";

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

Best regards,

Tim