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

[Tim Harvey]

On Thu, Jun 23, 2022 at 5:42 AM Vladimir Oltean <vladimir.oltean at nxp.com> wrote:
>
> On Tue, Jun 21, 2022 at 09:57:35AM -0700, Tim Harvey wrote:
> > > > 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);
> > > > +     }
> > > > +
> > >
> > > There's nothing too board specific about this configuration, why do you
> > > feel it does not belong to the DSA driver?
> > >
> > > Some macros hiding away magic register addresses and values would be
> > > good either way.
> > >
> >
> > I don't much care for MAC/PHY drivers configuring GPIO's and LED's due
> > to the lack of consistent dt bindings for such a thing and I wasn't
> > planning on trying to enhance the capabilities of the mv88e6xxx
> > driver.
> >
> > There are 7 functions each of the 15 GPIO's can be set to:
> > 0 - GPIO
> > 1 - PTP_TRIG - Precise Timing Protocol Trigger Generate Output
> > 2 - PTP_EVREQ - Precise Timing Protocol Event Request Input
> > 3 - PTP_EXTCLK - Precise Timing Protocol ExternalClock Input
> > 4 - SE_RCLK0 - SyncE Receive Clock 0 Output
> > 5 - SE_RCLK1 - SyncE Receive Clock 1 Output
> > 6 - Reserved
> > 7 - CLK125 - Free running 125 MHz Clock Output
> >
> > There are two LED's per port each of which can be set to 16 different
> > functions also and these functions take a lot of words to describe
> > thus probably wouldn't lend well to #define names.
> >
> > Are there dt bindings that you would suggest I look at for per-port
> > LED config on a switch, or GPIO config on a switch? If I add
> > dt-bindings I'll have to update the kernel driver as well which again,
> > was not my goal here. Maybe moving these into the mv88e6xxx driver per
> > dt bindings could be a 'todo'.
> >
> > This patch isn't making what is already in the board file more
> > obscure, it is just updating it to work with the new dsa driver. The
> > following was what this patch replaced:
> > -#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
> >
> > Best Regards,
> >
> > Tim
>
> Ok, I was thinking PHY LED configuration could be hardcoded in the
> driver itself (no DT bindings) and nobody would probably even notice.

No, it is always a bad idea to hard code a specific configuration in a
driver. Most modern PHY's have 4 LEDs with 16 configurations and board
vendors vary on what 2 LED's they use and how. I have seen this done
in PHY drivers and it makes no sense to inflict your LED policy on
other users without doing it in a configurable dt way and avoiding
changes if a configuration is not found for backwards compatibility.

> For pin configuration as RGMII 125 MHz clock or GPIO or otherwise,
> there would probably need to be a "pinctrl" DT subnode with a specific
> pinctrl driver attached. It's best if the development for that would be
> done in concert with the Linux community, perhaps even in Linux first.
>
> In any case, from my side it's ok if you leave a pinctrl sub-driver as TODO.

Can I have your Reviewed-By on this patch or do you want me to update
this with a 'TODO' comment?

Best Regards,

Tim