[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