[PATCH v7 7/8] net: add MV88E6xxx DSA driver

[Vladimir Oltean]

This patch looks much better to me. Only one comment.

On Wed, Oct 26, 2022 at 09:28:58AM -0700, Tim Harvey wrote:
> +static int mv88e6xxx_port_enable(struct udevice *dev, int port, struct phy_device *phy)
> +{
> +	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));
> +
> +	if (phy->phy_id == PHY_FIXED_ID) {
> +		/* Physical Control register: Table 62 */
> +		val = mv88e6xxx_port_read(dev, port, PORT_REG_PHYS_CTRL);
> +
> +		/* configure RGMII delays for fixed link */
> +		switch (phy->interface) {
> +		case PHY_INTERFACE_MODE_RGMII:
> +		case PHY_INTERFACE_MODE_RGMII_ID:
> +		case PHY_INTERFACE_MODE_RGMII_RXID:
> +		case PHY_INTERFACE_MODE_RGMII_TXID:
> +			dev_dbg(dev, "configure internal RGMII delays\n");
> +
> +			/* RGMII delays */
> +			val &= ~(PORT_REG_PHYS_CTRL_RGMII_DELAY_RXCLK ||
> +				 PORT_REG_PHYS_CTRL_RGMII_DELAY_TXCLK);
> +			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;
> +			break;
> +		default:
> +			break;
> +		}
> +
> +		/* 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)) {
> +			/* validate interface type */
> +			dev_dbg(dev, "validate interface type\n");
> +			val = mv88e6xxx_get_cmode(dev, port);
> +			if (val < 0)
> +				return val;
> +			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;
> +			}
> +		}
> +	} else {

What I was saying was here. The phy_id is not PHY_FIXED_ID, okay, but
that doesn't necessarily imply this is an internal PHY port, as the code
below goes on to assume. Maybe this is the case for your setup, but I
would like to see some guarding in a generic driver such that internal
PHY configuration only gets done for ports where that PHY exists.

Which actually brings me to another, more important point. This
PHY_REG_CTRL1 access is the only PHY access that the DSA driver
performs. But we also have a driver for the internal PHY. Why doesn't
the Marvell PHY driver do this write? Also, what is the utility of
enabling energy-detect sense?

> +		/* Enable energy-detect sensing on PHY */
> +		dev_dbg(dev, "enabling energy-detect sense on PHY\n");
> +		val = mv88e6xxx_phy_read(dev, port, PHY_REG_CTRL1);
> +		if (val < 0)
> +			return val;
> +		switch (priv->id) {
> +		case PORT_SWITCH_ID_6096:
> +		case PORT_SWITCH_ID_6097:
> +		case PORT_SWITCH_ID_6172:
> +		case PORT_SWITCH_ID_6176:
> +		case PORT_SWITCH_ID_6240:
> +		case PORT_SWITCH_ID_6352:
> +			val &= ~GENMASK(8, 2);
> +			val |= (PHY_REG_CTRL1_ENERGY_DET_SENSE_XMIT << 8);
> +			break;
> +		default:
> +			val &= ~GENMASK(14, 1);
> +			val |= (PHY_REG_CTRL1_ENERGY_DET_SENSE_PULSE << 14);
> +			break;
> +		}
> +		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 &= ~(PORT_REG_CTRL_PSTATE_MASK << PORT_REG_CTRL_PSTATE_SHIFT);
> +	val |= (PORT_REG_CTRL_PSTATE_FORWARD << PORT_REG_CTRL_PSTATE_SHIFT);
> +	val = mv88e6xxx_port_write(dev, port, PORT_REG_CTRL, val);
> +	if (val < 0)
> +		return val;
> +
> +	return phy_startup(phy);
> +}