Re: [PATCH v5 3/3] net: phy: bcm-phy-lib: Implement BroadR-Reach link modes

From: Andrew Lunn
Date: Wed Jun 05 2024 - 22:10:55 EST


On Wed, Jun 05, 2024 at 11:56:46AM +0200, Kamil Horák - 2N wrote:
> Implement single-pair BroadR-Reach modes on bcm5481x PHY by Broadcom.
> Create set of functions alternative to IEEE 802.3 to handle configuration
> of these modes on compatible Broadcom PHYs.
>
> Change-Id: I592d261bc0d60aaa78fc1717a315b0b1c1449c81
> Signed-off-by: Kamil Horák - 2N <kamilh@xxxxxxxx>
> ---
> drivers/net/phy/bcm-phy-lib.c | 123 ++++++++++++
> drivers/net/phy/bcm-phy-lib.h | 4 +
> drivers/net/phy/broadcom.c | 368 ++++++++++++++++++++++++++++++++--
> drivers/net/phy/phy-core.c | 2 +-
> include/linux/brcmphy.h | 9 +
> 5 files changed, 488 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/phy/bcm-phy-lib.c b/drivers/net/phy/bcm-phy-lib.c
> index 876f28fd8256..cc1b5e5a958c 100644
> --- a/drivers/net/phy/bcm-phy-lib.c
> +++ b/drivers/net/phy/bcm-phy-lib.c
> @@ -794,6 +794,47 @@ static int _bcm_phy_cable_test_get_status(struct phy_device *phydev,
> return ret;
> }
>
> +static int bcm_setup_forced(struct phy_device *phydev)
> +{
> + u16 ctl = 0;
> +
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + if (phydev->speed == SPEED_100)
> + ctl |= LRECR_SPEED100;
> +
> + if (phydev->duplex != DUPLEX_FULL)
> + return -EOPNOTSUPP;
> +
> + return phy_modify(phydev, MII_BCM54XX_LRECR, LRECR_SPEED100, ctl);

We should consider naming here. I assume this will not work for IEEE
forced mode? So maybe this should have _lre_ or _brr_ in the name to
make it clear what it actually does.

> +}



> +
> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + * @return: LDS Auto-Negotiation Advertised Ability register value
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register of Broadcom PHYs capable of LDS
> + */
> +static u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
> +{
> + u32 result = 0;

Make here, and maybe lre_advertising?

Please go through all the functions and think about naming.

Andrew

---
pw-bot: cr