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