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

[Heiner Kallweit <hkallweit1@gmail.com>]

On 25.03.2020 23:45, Vladimir Oltean wrote:
> 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?
> 

I didn't check the datasheet, so I can't really comment on whether the
patch does what it is supposed to do. My point was that the following
pattern occurs several times in the driver:
phy_write(phydev, MII_BCM54XX_AUX_CTL, VAL_1);
phy_modify(phydev, MII_BCM54XX_AUX_CTL, VAL_2, VAL_3)
(or phy_clear_bits/phy_set_bits instead of phy_modify)

Therefore a generic helper could make sense, or it could at least be
written as such a two-liner always.

>>> +     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
> .
>