[PATCH v6 7/8] net: add MV88E61xx DSA driver

Tim Harvey tharvey at gateworks.com
Thu Oct 13 20:09:32 CEST 2022


On Wed, Oct 12, 2022 at 12:50 PM Vladimir Oltean
<vladimir.oltean at nxp.com> wrote:
>
> On Mon, Oct 10, 2022 at 09:39:43AM -0700, Tim Harvey wrote:
> > Add a DSA driver for the MV88E6xxx compatible Ethernet switches.
> >
> > Cc: Marek BehĂșn <marek.behun at nic.cz>
> > Cc: Vladimir Oltean <vladimir.oltean at nxp.com>
> > Signed-off-by: Tim Harvey <tharvey at gateworks.com>
> > Reviewed-by: Vladimir Oltean <vladimir.oltean at nxp.com>
> > Reviewed-by: Fabio Estevam <festevam at denx.de>
> > ---
> > v6:
> >  - update commit msg (s/MV88E61xx/MV88E6xxx)
>
> really?

will fix in v7 - I updated the long msg and missed updating the short msg

>
> >  - remove GbE from commit msg and Kconfig desc
> >  - squash accidental change from patch 7 into patch 6
> >  - added error print on failure to read switch id
> >  - mv88e6xxx_probe:
> >   - check for switch enabled
> >   - remove unused variable enabled
> >   - remove unnecessary call to dsa_set_tagging
> >  - port_probe:
> >   - new function to configure phy features by calling phy_config
> >  - port_enable:
> >   - only enable energy-detect sensing on phy ports
> >   - add phy/cmode verification mistmatch warning
> >   - remove mv88e6xxx_fixed_port_setup()
> >   - always force link up for fixed ports
> >   - always set SERDES mode regardless of cpu port
>
> really?

The SERDES configuration on the 88E6352/88E6240/88E6176/88E6172 (which
I have documentation for) is not in port specific registers so I think
I should move that to the probe function for the switch.

>
> > +struct mv88e6xxx_priv {
> > +     int smi_addr;
> > +     int id;
> > +     int port_count;         /* Number of switch ports */
> > +     int port_reg_base;      /* Base of the switch port registers */
> > +     u8 global1;     /* Offset of Switch Global 1 registers */
> > +     u8 global2;     /* Offset of Switch Global 2 registers */
> > +     u8 phy_ctrl1_en_det_shift; /* 'EDet' bit field offset */
> > +     u8 phy_ctrl1_en_det_width; /* Width of 'EDet' bit field */
> > +     u8 phy_ctrl1_en_det_ctrl;  /* 'EDet' control value */
> > +};
> > +
> > +static inline int smi_cmd(int cmd, int addr, int reg)
>
> If the convention in U-Boot is the same as in Linux (which it well may be,
> except for the fact that more things escape review), then you should
> avoid manually defining functions as inline in C files. The compiler
> will decide whether to inline or not regardless of what you specify here.
>
> Although I wonder if these couldn't be in fact expressed more clearly
> using plain, boring old logical | and << operators as part of a macro.

The bitfield_replace came from the original
drivers/net/phy/mv88e61xx.c but I also find it easier to read logical
| and << operators so I'll change to that throughout for v7

>
> > +{
> > +     cmd = bitfield_replace(cmd, SMI_CMD_ADDR_SHIFT, SMI_CMD_ADDR_WIDTH,
> > +                            addr);
> > +     cmd = bitfield_replace(cmd, SMI_CMD_REG_SHIFT, SMI_CMD_REG_WIDTH, reg);
> > +     return cmd;
> > +}
> > +
> > +static inline int smi_cmd_read(int addr, int reg)
> > +{
> > +     return smi_cmd(SMI_CMD_READ, addr, reg);
> > +}
> > +
> > +static inline int smi_cmd_write(int addr, int reg)
> > +{
> > +     return smi_cmd(SMI_CMD_WRITE, addr, reg);
> > +}
> > +
> > +/* Wait for the current SMI indirect command to complete */
> > +static int mv88e6xxx_smi_wait(struct udevice *dev, int smi_addr)
> > +{
> > +     int val;
> > +     u32 timeout = 100;
> > +
> > +     do {
> > +             val = dm_mdio_read(dev->parent, smi_addr, MDIO_DEVAD_NONE, SMI_CMD_REG);
> > +             if (val >= 0 && (val & SMI_BUSY) == 0)
> > +                     return 0;
> > +
> > +             mdelay(1);
> > +     } while (--timeout);
> > +
> > +     dev_err(dev, "SMI busy timeout\n");
> > +     return -ETIMEDOUT;
> > +}
> > +
> > +static int mv88e6xxx_serdes_init(struct udevice *dev)
> > +{
> > +     int val;
> > +
> > +     val = mv88e6xxx_set_page(dev, DEVADDR_SERDES, PHY_PAGE_SERDES);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     /* Power up serdes module */
> > +     val = mv88e6xxx_phy_read(dev, DEVADDR_SERDES, MII_BMCR);
> > +     if (val < 0)
> > +             return val;
> > +     val &= ~(BMCR_PDOWN);
> > +     val = mv88e6xxx_phy_write(dev, DEVADDR_SERDES, MII_BMCR, val);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     return 0;
> > +}
> > +
> > +static int mv88e6xxx_port_enable(struct udevice *dev, int port, struct phy_device *phy)
> > +{
> > +     struct dsa_pdata *dsa_pdata = dev_get_uclass_plat(dev);
> > +     struct mv88e6xxx_priv *priv = dev_get_priv(dev);
> > +     int val, ret;
> > +
> > +     dev_dbg(dev, "%s P%d phy:0x%08x %s\n", __func__, port,
> > +             phy->phy_id, phy_string_for_interface(phy->interface));
> > +
> > +     /*
> > +      * Enable energy-detect sensing on PHY, used to determine when a PHY
> > +      * port is physically connected
> > +      */
> > +     if (port != dsa_pdata->cpu_port) {
>
> Can you test based on phy->interface == PHY_INTERFACE_MODE_INTERNAL?
> Support may come later for cascade ports, and those are ports without an
> internal PHY which are not CPU ports.

In my config there are no ports with interface ==
PHY_INTERFACE_MODE_INTERNAL so I'm not sure what you mean here. What
situations does that come up in?

>
> > +             val = mv88e6xxx_phy_read(dev, port, PHY_REG_CTRL1);
> > +             if (val < 0)
> > +                     return val;
> > +             val = bitfield_replace(val, priv->phy_ctrl1_en_det_shift,
> > +                                    priv->phy_ctrl1_en_det_width,
> > +                                    priv->phy_ctrl1_en_det_ctrl);
> > +             val = mv88e6xxx_phy_write(dev, port, PHY_REG_CTRL1, val);
> > +             if (val < 0)
> > +                     return val;
> > +     }
> > +
> > +     /* enable port */
> > +     val = mv88e6xxx_port_read(dev, port, PORT_REG_CTRL);
> > +     if (val < 0)
> > +             return val;
> > +     val = bitfield_replace(val, PORT_REG_CTRL_PSTATE_SHIFT,
> > +                            PORT_REG_CTRL_PSTATE_WIDTH,
> > +                            PORT_REG_CTRL_PSTATE_FORWARD);
> > +     val = mv88e6xxx_port_write(dev, port, PORT_REG_CTRL, val);
> > +     if (val < 0)
> > +             return val;
> > +
> > +     /* configure RGMII delays for fixed link */
> > +     if (phy->phy_id == PHY_FIXED_ID) {
>
> I believe this code should really be executed only for RGMII ports. In
> other words, move it inside a function that gets called inside the
> switch (phy->interface) block you added below.

It makes sense to only do this for RGMII modes but I'll need to take
it out of the if (mv88e61xx_6352_famiily()) as it looks like other
switches that could be supported by this driver support RGMII
interfaces.

>
> > +             /* Physical Control register: Table 62 */
> > +             val = mv88e6xxx_port_read(dev, port, PORT_REG_PHYS_CTRL);
> > +
> > +             /* RGMII delays */
> > +             val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK ||
> > +                      PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK);
>
> I find the style a bit chaotic when in other places you use bitfield_replace()
> but not everywhere.

I'll get rid of bitfield_replace and leave these as is

>
> > +             if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +                 phy->interface == PHY_INTERFACE_MODE_RGMII_RXID)
> > +                     val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK;
> > +             if (phy->interface == PHY_INTERFACE_MODE_RGMII_ID ||
> > +                 phy->interface == PHY_INTERFACE_MODE_RGMII_TXID)
> > +                     val |= PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK;
> > +             /* Force Link */
> > +             val |= PORT_REG_PHYS_CTRL_LINK_VALUE |
> > +                    PORT_REG_PHYS_CTRL_LINK_FORCE;
> > +
> > +             ret = mv88e6xxx_port_write(dev, port, PORT_REG_PHYS_CTRL, val);
> > +             if (ret < 0)
> > +                     return ret;
> > +     }
> > +
> > +     if (mv88e6xxx_6352_family(dev)) {
> > +             val = mv88e6xxx_get_cmode(dev, dsa_pdata->cpu_port);
>
> What gets handled by dsa_ops :: port_enable() should be a function of
> "int port", whereas dsa_pdata->cpu_port is an invariant of that.
>
> Otherwise said, you check for the cmode of the CPU port for as many
> times as ports there are, and never for the other ports. Probably not
> what you intend.

The original mv88e61xx driver initializes SERDES in
mv88e61xx_set_cpu_port() called from mv88e61xx_switch_init() called
from mv88e61xx_phy_config() for the cpu port PHY, only does this for
the 6172/6176/6240/6352, and the registers are not port specific:

         /* If CPU is connected to serdes, initialize serdes */
        if (mv88e61xx_6352_family(phydev)) {
                val = mv88e61xx_get_cmode(phydev, CONFIG_MV88E61XX_CPU_PORT);
                if (val < 0)
                        return val;
                if (val == PORT_REG_STATUS_CMODE_100BASE_X ||
                    val == PORT_REG_STATUS_CMODE_1000BASE_X ||
                    val == PORT_REG_STATUS_CMODE_SGMII) {
                        val = mv88e61xx_serdes_init(phydev);
                        if (val < 0)
                                return val;
                }
        }

static int mv88e61xx_serdes_init(struct phy_device *phydev)
{
        int val;

        val = mv88e61xx_set_page(phydev, DEVADDR_SERDES, PHY_PAGE_SERDES);
        if (val < 0)
                return val;

        /* Power up serdes module */
        val = mv88e61xx_phy_read(phydev, DEVADDR_SERDES, MII_BMCR);
        if (val < 0)
                return val;
        val &= ~(BMCR_PDOWN);
        val = mv88e61xx_phy_write(phydev, DEVADDR_SERDES, MII_BMCR, val);
        if (val < 0)
                return val;

        return 0;
}

So I think the proper thing to do is move this SERDES init into my
mv88e6xxx_probe function.

>
> > +             if (val < 0)
> > +                     return val;
> > +             /* initialize serdes */
> > +             if (val == PORT_REG_STATUS_CMODE_100BASE_X ||
> > +                 val == PORT_REG_STATUS_CMODE_1000BASE_X ||
> > +                 val == PORT_REG_STATUS_CMODE_SGMII) {
> > +                     ret = mv88e6xxx_serdes_init(dev);
>
> Again, you call mv88e6xxx_serdes_init() 10 times on the CPU port.
> The point of getting the C_Mode per port was to avoid directly checking
> for dsa_pdata->cpu_port.
>
> > +                     if (ret < 0)
> > +                             return ret;
> > +             }
> > +             /* validate interface type */
> > +             if (phy->phy_id == PHY_FIXED_ID) {
>
> I think you mean here to validate the C_Mode only for ports not
> connected to internal PHYs. But SERDES/RGMII ports may also have PHYs,
> albeit external. You want to validate the C_Mode for them too. So this
> check must go. Please check for PHY_INTERFACE_MODE_INTERNAL, and avoid
> comparing it to the C_Mode in that case.

I think my flaw here was that I checked c_mode for the cpu port above
as you pointed out:

val = mv88e6xxx_get_cmode(dev, dsa_pdata->cpu_port);

Changing that to mv88e6xxx_get_cmode(dev, port) should resolve that
but again I'm not sure where I would be seeing an interface mode of
PHY_INTERFACE_MODE_INTERNAL

>
> > +                     switch (phy->interface) {
> > +                     case PHY_INTERFACE_MODE_RGMII:
> > +                     case PHY_INTERFACE_MODE_RGMII_RXID:
> > +                     case PHY_INTERFACE_MODE_RGMII_TXID:
> > +                     case PHY_INTERFACE_MODE_RGMII_ID:
> > +                             if (val != PORT_REG_STATUS_CMODE_RGMII)
> > +                                     goto mismatch;
> > +                             break;
> > +                     case PHY_INTERFACE_MODE_1000BASEX:
> > +                             if (val != PORT_REG_STATUS_CMODE_1000BASE_X)
> > +                                     goto mismatch;
> > +                             break;
> > +mismatch:
> > +                     default:
> > +                             dev_err(dev, "Mismatched PHY mode %s on port %d!\n",
> > +                                     phy_string_for_interface(phy->interface), port);
> > +                             break;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return phy_startup(phy);
> > +}
>
> Overall it looks better than v5, but I believe that it could use a bit
> more attention to detail.
>
> Also, I don't work with Marvell hardware, so I'm not all that familiar
> with it. I understand this only supports the SERDES as seen on the 6176
> (where there is a single SERDES port), but I would appreciate if someone
> could take a second look at the hardware configuration in the physical
> interface area.

The only hardware I have access to for testing has a MV88E6176 and the
only register documentionat I have is from the
'88E6352/88E6240/88E6176/88E6172 Functional Specification' which is
what the driver considers the '6352 family' of Gigabit Ethernet
switches. I did manage to find a Marvell 2020 Product Selector Guide
[1] that does give an outline of the various switches between pages 15
and 17 but without register definitions for the other switches all I
have to go off of is the original u-boot driver and perhaps the Linux
driver.

Best Regards,

Tim
[1] https://jp.marvell.com/content/dam/marvell/en/psg/marvell_psg.pdf


More information about the U-Boot mailing list