Re: [PATCH v2 6/6] drivers: net: fsl_enetc: force the RGMII MAC speed/duplex instead of using in-band signaling

[Ramon Fried <rfried.dev@gmail.com>]

On Fri, Jun 18, 2021 at 1:26 AM Vladimir Oltean <vladimir.oltean@nxp.com> wrote:
>
> The RGMII spec supports optional in-band status reporting for the speed
> and duplex negotiated on the copper side, and the ENETC driver enables
> this feature by default.
>
> However, this does not work when the PHY does not implement the in-band
> reporting, or when there is a MAC-to-MAC connection described using a
> fixed-link. In that case, it would be better to disable the feature in
> the ENETC MAC and always force the speed and duplex to the values that
> were negotiated and retrieved over MDIO once the autoneg is finished.
> Since this works always, we just do it unconditionally and drop the
> in-band code.
>
> Note that because we need to wait for the autoneg to complete, we need
> to move enetc_setup_mac_iface() after phy_startup() returns, and then
> pass the phydev pointer all the way to enetc_init_rgmii().
>
> The same considerations have led to a similar Linux driver patch as well:
> https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=c76a97218dcbb2cb7cec1404ace43ef96c87d874
>
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
> v1->v2: reword commit message
>
>  drivers/net/fsl_enetc.c | 44 ++++++++++++++++++++++++++++++-----------
>  drivers/net/fsl_enetc.h |  5 +++++
>  2 files changed, 38 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/net/fsl_enetc.c b/drivers/net/fsl_enetc.c
> index 5961775024ff..de1e42f907e7 100644
> --- a/drivers/net/fsl_enetc.c
> +++ b/drivers/net/fsl_enetc.c
> @@ -178,21 +178,43 @@ static int enetc_init_sgmii(struct udevice *dev)
>  }
>
>  /* set up MAC for RGMII */
> -static int enetc_init_rgmii(struct udevice *dev)
> +static void enetc_init_rgmii(struct udevice *dev, struct phy_device *phydev)
>  {
>         struct enetc_priv *priv = dev_get_priv(dev);
> -       u32 if_mode;
> +       u32 old_val, val;
>
> -       /* enable RGMII AN */
> -       if_mode = enetc_read_port(priv, ENETC_PM_IF_MODE);
> -       if_mode |= ENETC_PM_IF_MODE_AN_ENA;
> -       enetc_write_port(priv, ENETC_PM_IF_MODE, if_mode);
> +       old_val = val = enetc_read_port(priv, ENETC_PM_IF_MODE);
>
> -       return 0;
> +       /* disable unreliable RGMII in-band signaling and force the MAC into
> +        * the speed negotiated by the PHY.
> +        */
> +       val &= ~ENETC_PM_IF_MODE_AN_ENA;
> +
> +       if (phydev->speed == SPEED_1000) {
> +               val &= ~ENETC_PM_IFM_SSP_MASK;
> +               val |= ENETC_PM_IFM_SSP_1000;
> +       } else if (phydev->speed == SPEED_100) {
> +               val &= ~ENETC_PM_IFM_SSP_MASK;
> +               val |= ENETC_PM_IFM_SSP_100;
> +       } else if (phydev->speed == SPEED_10) {
> +               val &= ~ENETC_PM_IFM_SSP_MASK;
> +               val |= ENETC_PM_IFM_SSP_10;
> +       }
> +
> +       if (phydev->duplex == DUPLEX_FULL)
> +               val |= ENETC_PM_IFM_FULL_DPX;
> +       else
> +               val &= ~ENETC_PM_IFM_FULL_DPX;
> +
> +       if (val == old_val)
> +               return;
> +
> +       enetc_write_port(priv, ENETC_PM_IF_MODE, val);
>  }
>
>  /* set up MAC configuration for the given interface type */
> -static void enetc_setup_mac_iface(struct udevice *dev)
> +static void enetc_setup_mac_iface(struct udevice *dev,
> +                                 struct phy_device *phydev)
>  {
>         struct enetc_priv *priv = dev_get_priv(dev);
>         u32 if_mode;
> @@ -202,7 +224,7 @@ static void enetc_setup_mac_iface(struct udevice *dev)
>         case PHY_INTERFACE_MODE_RGMII_ID:
>         case PHY_INTERFACE_MODE_RGMII_RXID:
>         case PHY_INTERFACE_MODE_RGMII_TXID:
> -               enetc_init_rgmii(dev);
> +               enetc_init_rgmii(dev, phydev);
>                 break;
>         case PHY_INTERFACE_MODE_XGMII:
>         case PHY_INTERFACE_MODE_USXGMII:
> @@ -546,10 +568,10 @@ static int enetc_start(struct udevice *dev)
>         enetc_setup_tx_bdr(dev);
>         enetc_setup_rx_bdr(dev);
>
> -       enetc_setup_mac_iface(dev);
> -
>         phy_startup(priv->phy);
>
> +       enetc_setup_mac_iface(dev, priv->phy);
> +
>         return 0;
>  }
>
> diff --git a/drivers/net/fsl_enetc.h b/drivers/net/fsl_enetc.h
> index 110c1d78fbc6..a4409505e043 100644
> --- a/drivers/net/fsl_enetc.h
> +++ b/drivers/net/fsl_enetc.h
> @@ -77,6 +77,11 @@ enum enetc_bdr_type {TX, RX};
>  #define ENETC_PM_IF_MODE               0x8300
>  #define  ENETC_PM_IF_MODE_RG           BIT(2)
>  #define  ENETC_PM_IF_MODE_AN_ENA       BIT(15)
> +#define  ENETC_PM_IFM_SSP_MASK         GENMASK(14, 13)
> +#define  ENETC_PM_IFM_SSP_1000         (2 << 13)
> +#define  ENETC_PM_IFM_SSP_100          (0 << 13)
> +#define  ENETC_PM_IFM_SSP_10           (1 << 13)
> +#define  ENETC_PM_IFM_FULL_DPX         BIT(12)
>  #define  ENETC_PM_IF_IFMODE_MASK       GENMASK(1, 0)
>
>  /* buffer descriptors count must be multiple of 8 and aligned to 128 bytes */
> --
> 2.25.1
>
Reviewed-by: Ramon Fried <rfried.dev@gmail.com>