Message ID | 20210629170839.2583797-6-olteanv@gmail.com |
---|---|
State | Changes Requested |
Delegated to: | Priyanka Jain |
Headers | show |
Series | Call phy_config at port probe time for the Felix DSA driver | expand |
On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv@gmail.com> wrote: > > From: Vladimir Oltean <vladimir.oltean@nxp.com> > > It is an unfortunate reality that some PHY settings done by U-Boot > persist even after the PHY is reset and taken over by Linux, and even > more unfortunate that Linux has come to depend on things being set in a > certain way. > > For example, on the NXP LS1028A-RDB, the felix switch ports are > connected to a VSC8514 QSGMII PHY. Between the switch port PCS and the > PHY, the U-Boot drivers enable in-band auto-negotiation which makes the > copper-side negotiated speed and duplex be transmitted from the PHY to > the MAC automatically. > > The PHY driver portion that does this is in vsc8514_config(): > > /* Enable Serdes Auto-negotiation */ > phy_write(phydev, MDIO_DEVAD_NONE, PHY_EXT_PAGE_ACCESS, > PHY_EXT_PAGE_ACCESS_EXTENDED3); > val = phy_read(phydev, MDIO_DEVAD_NONE, MIIM_VSC8514_MAC_SERDES_CON); > val = val | MIIM_VSC8574_MAC_SERDES_ANEG; > phy_write(phydev, MDIO_DEVAD_NONE, MIIM_VSC8514_MAC_SERDES_CON, val); > > The point is that in-band autoneg should be turned on in both the PHY > and the MAC, or off in both the PHY and the MAC, otherwise the QSGMII > link will be broken. > > And because phy_config() is currently called at .port_enable() time, the > result is that ports on which traffic has been sent in U-Boot will have > in-band autoneg enabled, and the rest won't. > > It can be argued that the Linux kernel should not assume one way or > another and just reinitialize everything according to what it expects, > and that is completely fair. In fact, I've already started an attempt to > remove this dependency, although admittedly I am making slow progress at > it: > https://patchwork.kernel.org/project/netdevbpf/cover/20210212172341.3489046-1-olteanv@gmail.com/ > > Nonetheless, the sad reality is that NXP also has, apart from kernel > drivers, some user space networking (DPDK), and for some reason, the > expectation there is that somebody else initializes the PHYs. The kernel > can't do it because the device ownership doesn't belong to the kernel, > so what remains is for the bootloader to do it (especially since other > drivers generally call phy_config() at probe time). This is a really > weak guarantee that might break at any time, but apparently that is > enough for some. > > Since initializing the ports and PHYs at probe time does not break > anything, we can just do that. > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> > --- > drivers/net/mscc_eswitch/felix_switch.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c > index 75073880cf87..c8ecf4f19442 100644 > --- a/drivers/net/mscc_eswitch/felix_switch.c > +++ b/drivers/net/mscc_eswitch/felix_switch.c > @@ -317,10 +317,23 @@ static int felix_probe(struct udevice *dev) > return 0; > } > > +static int felix_port_probe(struct udevice *dev, int port, > + struct phy_device *phy) > +{ > + int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full; > + struct felix_priv *priv = dev_get_priv(dev); > + > + phy->supported &= supported; > + phy->advertising &= supported; > + > + felix_start_pcs(dev, port, phy, &priv->imdio); > + > + return phy_config(phy); > +} > + > static int felix_port_enable(struct udevice *dev, int port, > struct phy_device *phy) > { > - int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full; > struct felix_priv *priv = dev_get_priv(dev); > void *base = priv->regs_base; > > @@ -339,12 +352,6 @@ static int felix_port_enable(struct udevice *dev, int port, > FELIX_QSYS_SYSTEM_SW_PORT_LOSSY | > FELIX_QSYS_SYSTEM_SW_PORT_SCH(1)); > > - felix_start_pcs(dev, port, phy, &priv->imdio); > - > - phy->supported &= supported; > - phy->advertising &= supported; > - phy_config(phy); > - > phy_startup(phy); > > return 0; > @@ -392,6 +399,7 @@ static int felix_rcv(struct udevice *dev, int *pidx, void *packet, int length) > } > > static const struct dsa_ops felix_dsa_ops = { > + .port_probe = felix_port_probe, > .port_enable = felix_port_enable, > .port_disable = felix_port_disable, > .xmit = felix_xmit, > -- > 2.25.1 > Interesting, My two cents is that device drivers by definition shouldn't assume anything regarding the state of the HW. Reviewed-by: Ramon Fried <rfried.dev@gmail.com>
diff --git a/drivers/net/mscc_eswitch/felix_switch.c b/drivers/net/mscc_eswitch/felix_switch.c index 75073880cf87..c8ecf4f19442 100644 --- a/drivers/net/mscc_eswitch/felix_switch.c +++ b/drivers/net/mscc_eswitch/felix_switch.c @@ -317,10 +317,23 @@ static int felix_probe(struct udevice *dev) return 0; } +static int felix_port_probe(struct udevice *dev, int port, + struct phy_device *phy) +{ + int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full; + struct felix_priv *priv = dev_get_priv(dev); + + phy->supported &= supported; + phy->advertising &= supported; + + felix_start_pcs(dev, port, phy, &priv->imdio); + + return phy_config(phy); +} + static int felix_port_enable(struct udevice *dev, int port, struct phy_device *phy) { - int supported = PHY_GBIT_FEATURES | SUPPORTED_2500baseX_Full; struct felix_priv *priv = dev_get_priv(dev); void *base = priv->regs_base; @@ -339,12 +352,6 @@ static int felix_port_enable(struct udevice *dev, int port, FELIX_QSYS_SYSTEM_SW_PORT_LOSSY | FELIX_QSYS_SYSTEM_SW_PORT_SCH(1)); - felix_start_pcs(dev, port, phy, &priv->imdio); - - phy->supported &= supported; - phy->advertising &= supported; - phy_config(phy); - phy_startup(phy); return 0; @@ -392,6 +399,7 @@ static int felix_rcv(struct udevice *dev, int *pidx, void *packet, int length) } static const struct dsa_ops felix_dsa_ops = { + .port_probe = felix_port_probe, .port_enable = felix_port_enable, .port_disable = felix_port_disable, .xmit = felix_xmit,