Re: [PATCH v3 8/8] board: gw_ventana: enable MV88E61XX DSA support

[Tim Harvey <tharvey@gateworks.com>]

On Fri, Jun 24, 2022 at 3:25 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> On Mon, May 23, 2022 at 11:25:49AM -0700, Tim Harvey wrote:
> > Add MV88E61XX DSA support:
> >  - update dt: U-Boot dsa driver requires different device-tree syntax
> >    than the linux driver in order to link the dsa ports to the mdio bus.
> >  - update defconfig
> >  - replace mv88e61xx_hw_reset weak override with board_phy_config support
> >    for mv88e61xx configuration that is outside the scope of the DSA driver
> >
> > Signed-off-by: Tim Harvey <tharvey@gateworks.com>
> > ---
> > v3:
> >  - move mdio's mdio@0 subnode
> > v2: no changes
> > ---
> >  arch/arm/dts/imx6qdl-gw5904.dtsi        | 41 ++++++++++++++++++++
> >  board/gateworks/gw_ventana/gw_ventana.c | 50 +++++++++----------------
> >  configs/gwventana_gw5904_defconfig      |  7 ++--
> >  3 files changed, 62 insertions(+), 36 deletions(-)
> >
> > diff --git a/arch/arm/dts/imx6qdl-gw5904.dtsi b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > index 286c7a9924c2..1b2f70d1ccb2 100644
> > --- a/arch/arm/dts/imx6qdl-gw5904.dtsi
> > +++ b/arch/arm/dts/imx6qdl-gw5904.dtsi
> > @@ -219,6 +219,33 @@
> >                       compatible = "marvell,mv88e6085";
> >                       reg = <0>;
> >
> > +                     mdios {
> > +                             #address-cells = <1>;
> > +                             #size-cells = <0>;
> > +
> > +                             mdio@0 {
>
> If you are going to follow this new model with a dedicated "mdios" subnode,
> I've consulted with Andrew Lunn and Florian Fainelli and there shouldn't
> be a problem to later make Linux accept this alternate binding format.
> But in that case, please match this OF node by a dedicated compatible
> string, like "marvell,mv88e6xxx-mdio-internal", so that there will be a
> way to differentiate this from the existing "marvell,mv88e6xxx-mdio-external"
> when support for that is added in U-Boot.
>
> Alternatively, to repeat myself, you can always follow the de-facto
> bindings for Linux mv88e6xxx which have:
>
>                 switch0: switch0@0 {
>                         compatible = "marvell,mv88e6190";
>
>                         ports {
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>
>                                 ...
>                         };
>
>                         mdio { // internal
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>
>                                 ...
>                         };
>
>                         mdio1 {
>                                 compatible = "marvell,mv88e6xxx-mdio-external";
>                                 #address-cells = <1>;
>                                 #size-cells = <0>;
>
>                                 ...
>                         };
>                 };
>

Documentation/devicetree/bindings/net/dsa/marvell.txt shows en example
with just one child node under the internal mdio node:

                        mdio {
                                #address-cells = <1>;
                                #size-cells = <0>;
                                switch1phy0: switch1phy0@0 {
                                        reg = <0>;
                                        interrupt-parent = <&switch0>;
                                        interrupts = <0 IRQ_TYPE_LEVEL_HIGH>;
                                };
                        };

Am I to assume I can add additional nodes there for the other ports
such as the following?

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

                                switch1phy0: switch1phy0@0 {
                                        reg = <0>;
                                };

                                switch1phy1: switch1phy1@1 {
                                        reg = <1>;
                                };

                                switch1phy2: switch1phy2@2 {
                                        reg = <2>;
                                };

                                switch1phy3: switch1phy3@3 {
                                        reg = <3>;
                                };

                                ...
                    };

Best Regards,

Tim


> and the associated parsing code:
>
> static int mv88e6xxx_mdios_register(struct mv88e6xxx_chip *chip,
>                                     struct device_node *np)
> {
>         struct device_node *child;
>         int err;
>
>         /* Always register one mdio bus for the internal/default mdio
>          * bus. This maybe represented in the device tree, but is
>          * optional.
>          */
>         child = of_get_child_by_name(np, "mdio");
>         err = mv88e6xxx_mdio_register(chip, child, false);
>         of_node_put(child);
>         if (err)
>                 return err;
>
>         /* Walk the device tree, and see if there are any other nodes
>          * which say they are compatible with the external mdio
>          * bus.
>          */
>         for_each_available_child_of_node(np, child) {
>                 if (of_device_is_compatible(
>                             child, "marvell,mv88e6xxx-mdio-external")) {
>                         err = mv88e6xxx_mdio_register(chip, child, true);
>                         if (err) {
>                                 mv88e6xxx_mdios_unregister(chip);
>                                 of_node_put(child);
>                                 return err;
>                         }
>                 }
>         }
>
>         return 0;
> }
>
> Personally I believe that if you have limited amount of time to spend on
> U-Boot DM_DSA and DT bindings in general, you should just follow the
> format already accepted by Linux ("mdio" node is for internal MDIO,
> doesn't have compatible string, is placed directly under "switch" node",
> while external MDIO is matched by compatible string and its node can
> have any name).
>
> What we should try to accomplish is that the DT blob that U-Boot creates
> for itself here to be coherent enough that Linux is able to understand
> and use it, in case we decide to pass it to the operating system. With
> your approach you'd have work to do in Linux as well to accept the
> "mdios" subnode and parse controllers by compatible string inside, and
> I'm not exactly sure you're willing to do that.
>
> > +                                     reg = <0>;
> > +                                     #address-cells = <1>;
> > +                                     #size-cells = <0>;
> > +
> > +                                     sw_phy0: ethernet-phy@0 {
> > +                                             reg = <0x0>;
> > +                                     };
> > +
> > +                                     sw_phy1: ethernet-phy@1 {
> > +                                             reg = <0x1>;
> > +                                     };
> > +
> > +                                     sw_phy2: ethernet-phy@2 {
> > +                                             reg = <0x2>;
> > +                                     };
> > +
> > +                                     sw_phy3: ethernet-phy@3 {
> > +                                             reg = <0x3>;
> > +                                     };
> > +                             };
> > +                     };
> > +
> >                       ports {
> >                               #address-cells = <1>;
> >                               #size-cells = <0>;
> > @@ -226,27 +253,41 @@
> >                               port@0 {
> >                                       reg = <0>;
> >                                       label = "lan4";
> > +                                     phy-mode = "internal";
> > +                                     phy-handle = <&sw_phy0>;
> >                               };
> >
> >                               port@1 {
> >                                       reg = <1>;
> >                                       label = "lan3";
> > +                                     phy-mode = "internal";
> > +                                     phy-handle = <&sw_phy1>;
> >                               };
> >
> >                               port@2 {
> >                                       reg = <2>;
> >                                       label = "lan2";
> > +                                     phy-mode = "internal";
> > +                                     phy-handle = <&sw_phy2>;
> >                               };
> >
> >                               port@3 {
> >                                       reg = <3>;
> >                                       label = "lan1";
> > +                                     phy-mode = "internal";
> > +                                     phy-handle = <&sw_phy3>;
> >                               };
> >
> >                               port@5 {
> >                                       reg = <5>;
> >                                       label = "cpu";
> >                                       ethernet = <&fec>;
> > +                                     phy-mode = "rgmii-id";
> > +
> > +                                     fixed-link {
> > +                                             speed = <1000>;
> > +                                             full-duplex;
> > +                                     };
> >                               };
> >                       };
> >               };
> > diff --git a/board/gateworks/gw_ventana/gw_ventana.c b/board/gateworks/gw_ventana/gw_ventana.c
> > index c06630a66b66..bef3f7ef0d2b 100644
> > --- a/board/gateworks/gw_ventana/gw_ventana.c
> > +++ b/board/gateworks/gw_ventana/gw_ventana.c
> > @@ -68,44 +68,30 @@ int board_phy_config(struct phy_device *phydev)
> >               phy_write(phydev, MDIO_DEVAD_NONE, 14, val);
> >       }
> >
> > +     /* Fixed PHY: for GW5904/GW5909 this is Marvell 88E6176 GbE Switch */
> > +     else if (phydev->phy_id == 0xa5a55a5a &&
> > +              ((board_type == GW5904) || (board_type == GW5909))) {
> > +             struct mii_dev *bus = miiphy_get_dev_by_name("mdio");
> > +
> > +             puts("MV88E61XX ");
> > +             /* GPIO[0] output CLK125 for RGMII_REFCLK */
> > +             bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x62 << 8) | 0xfe);
> > +             bus->write(bus, 0x1c, 0, 0x1a, (1 << 15) | (0x68 << 8) | 7);
> > +
> > +             /* Port 0-3 LED configuration: Table 80/82 */
> > +             /* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
> > +             bus->write(bus, 0x10, 0, 0x16, 0x8088);
> > +             bus->write(bus, 0x11, 0, 0x16, 0x8088);
> > +             bus->write(bus, 0x12, 0, 0x16, 0x8088);
> > +             bus->write(bus, 0x13, 0, 0x16, 0x8088);
> > +     }
> > +
> >       if (phydev->drv->config)
> >               phydev->drv->config(phydev);
> >
> >       return 0;
> >  }
> >
> > -#ifdef CONFIG_MV88E61XX_SWITCH
> > -int mv88e61xx_hw_reset(struct phy_device *phydev)
> > -{
> > -     struct mii_dev *bus = phydev->bus;
> > -
> > -     /* GPIO[0] output, CLK125 */
> > -     debug("enabling RGMII_REFCLK\n");
> > -     bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
> > -                0x1a /*MV_SCRATCH_MISC*/,
> > -                (1 << 15) | (0x62 /*MV_GPIO_DIR*/ << 8) | 0xfe);
> > -     bus->write(bus, 0x1c /*MV_GLOBAL2*/, 0,
> > -                0x1a /*MV_SCRATCH_MISC*/,
> > -                (1 << 15) | (0x68 /*MV_GPIO01_CNTL*/ << 8) | 7);
> > -
> > -     /* RGMII delay - Physical Control register bit[15:14] */
> > -     debug("setting port%d RGMII rx/tx delay\n", CONFIG_MV88E61XX_CPU_PORT);
> > -     /* forced 1000mbps full-duplex link */
> > -     bus->write(bus, 0x10 + CONFIG_MV88E61XX_CPU_PORT, 0, 1, 0xc0fe);
> > -     phydev->autoneg = AUTONEG_DISABLE;
> > -     phydev->speed = SPEED_1000;
> > -     phydev->duplex = DUPLEX_FULL;
> > -
> > -     /* LED configuration: 7:4-green (8=Activity)  3:0 amber (8=Link) */
> > -     bus->write(bus, 0x10, 0, 0x16, 0x8088);
> > -     bus->write(bus, 0x11, 0, 0x16, 0x8088);
> > -     bus->write(bus, 0x12, 0, 0x16, 0x8088);
> > -     bus->write(bus, 0x13, 0, 0x16, 0x8088);
> > -
> > -     return 0;
> > -}
> > -#endif // CONFIG_MV88E61XX_SWITCH
> > -
> >  #if defined(CONFIG_VIDEO_IPUV3)
> >  static void enable_hdmi(struct display_info_t const *dev)
> >  {
> > diff --git a/configs/gwventana_gw5904_defconfig b/configs/gwventana_gw5904_defconfig
> > index d25a8324b1df..9809bc691508 100644
> > --- a/configs/gwventana_gw5904_defconfig
> > +++ b/configs/gwventana_gw5904_defconfig
> > @@ -103,12 +103,11 @@ CONFIG_SUPPORT_EMMC_BOOT=y
> >  CONFIG_FSL_USDHC=y
> >  CONFIG_MTD=y
> >  CONFIG_PHYLIB=y
> > -CONFIG_MV88E61XX_SWITCH=y
> > -CONFIG_MV88E61XX_CPU_PORT=5
> > -CONFIG_MV88E61XX_PHY_PORTS=0xf
> > -CONFIG_MV88E61XX_FIXED_PORTS=0x0
> > +CONFIG_MV88E61XX=y
> > +CONFIG_PHY_FIXED=y
> >  CONFIG_DM_ETH=y
> >  CONFIG_DM_MDIO=y
> > +CONFIG_DM_DSA=y
> >  CONFIG_E1000=y
> >  CONFIG_FEC_MXC=y
> >  CONFIG_MII=y
> > --
> > 2.17.1
> >