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

[Vladimir Oltean]

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.
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.