On Wed, Oct 26, 2022 at 12:55 PM Vladimir Oltean vladimir.oltean@nxp.com wrote:
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.
ok, I understand now
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?
not sure honestly - it's in the original drivers/net/phy/mv88e61xxx.c driver that I based this one off of.
From the functional spec of the 88E6176:
- energy detect mode1 (what I'm doing here): only the signal detection circuitry and the serial management interface are active. If the PHY detects energy on the line, it starts to Auto-Negotiate sending FLPs for 5 seconds. If at the end of 5 seconds the Auto-Negotiation is not completed, then the PHY stops sending FLPs and goes back to monitoring receive energy. If Auto-Negotation is completed, then the PHY goes into normal 10/100/1000 MBps operation. If during normal operation the link is lost, the PHY will re-start Auto-Negotiation. If no energy is detected after 5 seconds, the PHY goes back to monitoring receive energy. - energy detect mode2 (appears to be the power-on default): The PHY sends out a single 10Mbps NLP (Normal Link Pulse) every on second. Except for this difference, Mode 2 is identical to Mode 1 operation. If the device is in Mode 1, it cannot wake up a connected device; therefore, the connected device must be transmitting NLPs, or either device must be woken up through register access. If the device is in Mode 2, then it can wake a connected device. - energy detect disabled: Normal 10/100/1000 mbps operation
You bring up a great point regarding the Marvell PHY driver not doing this... I'm happy to kill it off?
Tim
/* 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);+}