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

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);

+}