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

[Vladimir Oltean]

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?

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

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

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

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

> +		/* 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.

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

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

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