On Wed, Oct 26, 2022 at 12:55 PM Vladimir Oltean
<vladimir.olt...@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);
> > +}
