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

[Tim Harvey]

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