Re: [PATCH v2 net-next 02/10] net: phy: bcm7xx: Add jumbo frame configuration to PHY

[Vladimir Oltean <olteanv@gmail.com>]

On Wed, 25 Mar 2020 at 17:44, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>
> On 25.03.2020 16:22, Vladimir Oltean wrote:
> > From: Murali Krishna Policharla <murali.policharla@broadcom.com>
> >
> > Add API to configure jumbo frame settings in PHY during initial PHY
> > configuration.
> >
> > Signed-off-by: Murali Krishna Policharla <murali.policharla@broadcom.com>
> > Reviewed-by: Scott Branden <scott.branden@broadcom.com>
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/phy/bcm-phy-lib.c | 28 ++++++++++++++++++++++++++++
> >  drivers/net/phy/bcm-phy-lib.h |  1 +
> >  drivers/net/phy/bcm7xxx.c     |  4 ++++
> >  include/linux/brcmphy.h       |  1 +
> >  4 files changed, 34 insertions(+)
> >
> > diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> > index e0d3310957ff..a26c80e13b43 100644
> > --- a/drivers/net/phy/bcm-phy-lib.c
> > +++ b/drivers/net/phy/bcm-phy-lib.c
> > @@ -423,6 +423,34 @@ int bcm_phy_28nm_a0b0_afe_config_init(struct phy_device *phydev)
> >  }
> >  EXPORT_SYMBOL_GPL(bcm_phy_28nm_a0b0_afe_config_init);
> >
> > +int bcm_phy_enable_jumbo(struct phy_device *phydev)
> > +{
> > +     int val = 0, ret = 0;
> > +
> > +     ret = phy_write(phydev, MII_BCM54XX_AUX_CTL,
> > +                     MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     val = phy_read(phydev, MII_BCM54XX_AUX_CTL);
> > +
> > +     /* Enable extended length packet reception */
> > +     val |= MII_BCM54XX_AUXCTL_ACTL_EXT_PKT_LEN;
> > +     ret = phy_write(phydev, MII_BCM54XX_AUX_CTL, val);
> > +
>
> There are different helpers already in bcm-phy-lib,
> e.g. bcm54xx_auxctl_read. Also bcm_phy_write_misc()
> has has quite something in common with your new function.
> It would be good if a helper could be used here.
>

Thanks Heiner.
I'm not quite sure the operation is performed correctly though? My
books are telling me that the "Receive Extended Packet Length" field
is accessible via the Auxiliary Control Register 0x18 when the shadow
value is 000, not 111 as this patch is doing. At least for BCM54xxx in
terms of which the macros are defined. Am I wrong?

> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     val = phy_read(phydev, MII_BCM54XX_ECR);
> > +
> > +     /* Enable 10K byte packet length reception */
> > +     val |= BIT(0);
> > +     ret =  phy_write(phydev, MII_BCM54XX_ECR, val);
> > +
>
> Why not use phy_set_bits() ?
>

Well, the reason is that I didn't write the patch. I'll simplify it.

> > +     return ret;
> > +}
> > +EXPORT_SYMBOL_GPL(bcm_phy_enable_jumbo);
> > +
> >  MODULE_DESCRIPTION("Broadcom PHY Library");
> >  MODULE_LICENSE("GPL v2");
> >  MODULE_AUTHOR("Broadcom Corporation");
> > diff --git a/drivers/net/phy/bcm-phy-lib.h b/drivers/net/phy/bcm-phy-lib.h
> > index c86fb9d1240c..129df819be8c 100644
> > --- a/drivers/net/phy/bcm-phy-lib.h
> > +++ b/drivers/net/phy/bcm-phy-lib.h
> > @@ -65,5 +65,6 @@ void bcm_phy_get_stats(struct phy_device *phydev, u64 *shadow,
> >                      struct ethtool_stats *stats, u64 *data);
> >  void bcm_phy_r_rc_cal_reset(struct phy_device *phydev);
> >  int bcm_phy_28nm_a0b0_afe_config_init(struct phy_device *phydev);
> > +int bcm_phy_enable_jumbo(struct phy_device *phydev);
> >
> >  #endif /* _LINUX_BCM_PHY_LIB_H */
> > diff --git a/drivers/net/phy/bcm7xxx.c b/drivers/net/phy/bcm7xxx.c
> > index af8eabe7a6d4..692048d86ab1 100644
> > --- a/drivers/net/phy/bcm7xxx.c
> > +++ b/drivers/net/phy/bcm7xxx.c
> > @@ -178,6 +178,10 @@ static int bcm7xxx_28nm_config_init(struct phy_device *phydev)
> >               break;
> >       }
> >
> > +     if (ret)
> > +             return ret;
> > +
> > +     ret =  bcm_phy_enable_jumbo(phydev);
> >       if (ret)
> >               return ret;
> >
> > diff --git a/include/linux/brcmphy.h b/include/linux/brcmphy.h
> > index b475e7f20d28..19bd86019e93 100644
> > --- a/include/linux/brcmphy.h
> > +++ b/include/linux/brcmphy.h
> > @@ -119,6 +119,7 @@
> >  #define MII_BCM54XX_AUXCTL_SHDWSEL_AUXCTL    0x00
> >  #define MII_BCM54XX_AUXCTL_ACTL_TX_6DB               0x0400
> >  #define MII_BCM54XX_AUXCTL_ACTL_SMDSP_ENA    0x0800
> > +#define MII_BCM54XX_AUXCTL_ACTL_EXT_PKT_LEN  0x4000
> >
> >  #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC                      0x07
> >  #define MII_BCM54XX_AUXCTL_SHDWSEL_MISC_WIRESPEED_EN 0x0010
> >
>

Regards,
-Vladimir