[PATCH 5/6] net: dsa: felix: call phy_config at .port_probe() time

[Ramon Fried]

On Tue, Jun 29, 2021 at 8:09 PM Vladimir Oltean <olteanv at gmail.com> wrote:
>
> From: Vladimir Oltean <vladimir.oltean at 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 at 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 at gmail.com>