Message-ID: <96dc827a-9f4b-4b9a-8577-170cb335bebf@broadcom.com>
Date: Tue, 16 Apr 2024 16:48:39 -0700
From: Florian Fainelli <florian.fainelli@...adcom.com>
To: Kamil Horák - 2N <kamilh@...s.com>,
 florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
 andrew@...n.ch, hkallweit1@...il.com
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: bcm54811: add support for BroadR-Reach mode

On 4/16/24 05:38, Kamil Horák - 2N wrote:
> Enable the BroadR-Reach link modes for BCM54811 PHY by Broadcom.
> This allows for two-wire Ethernet at 10 or 100MBit using BroadR-Reach
> mode and also for other BroadR-Reach modes using more than one pair.

In terms of organization, it seems like you could do at least 3 patches:

- one patch that adds the new BroadR-Reach link modes and ethtool plumbing
- one patch that adds all of the register definitions to brcmphy.h which 
can be easily skipped
- one patch that updates broadcom.c and bcm-phy-lib.[ch] to implement 
BroadR-Reach

I am happy that you introduced an ethtool tunable, rather than key it 
off Device Tree or module parameter, so kudos for doing that!

Need to wrap my head a bit as to what to do with the overlapping 
register mapping, and whether it would not be simpler to do some sort of 
register level remapping of values, rather than provide different 
callbacks for read_status, config_aneg, etc.

> 
> Signed-off-by: Kamil Horák - 2N <kamilh@...s.com>
> ---
>   drivers/net/phy/broadcom.c   | 432 ++++++++++++++++++++++++++++++++++-
>   drivers/net/phy/phy-core.c   |  13 +-
>   include/linux/brcmphy.h      |  91 +++++++-
>   include/uapi/linux/ethtool.h |  13 +-
>   net/ethtool/common.c         |  31 +++
>   net/ethtool/ioctl.c          |   1 +
>   net/ipv4/tcp.c               |   2 +-

This file should really not be modified, you must have made this 
temporary change to build test your changes, but this is now fixed in 
net-next as of a few days ago.

>   7 files changed, 567 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 370e4ed45098..aeb49b566aca 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -553,18 +553,46 @@ static int bcm54810_write_mmd(struct phy_device *phydev, int devnum, u16 regnum,
>   	return -EOPNOTSUPP;
>   }
>   
> -static int bcm54811_config_init(struct phy_device *phydev)
> +static int bcm54811_get_brrmode(struct phy_device *phydev, u8 *data)
>   {
> -	int err, reg;
> +	int reg;
>   
> -	/* Disable BroadR-Reach function. */
>   	reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> -	reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> -	err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL,
> -				reg);
> -	if (err < 0)
> +
> +	*data = (reg & BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN) ?
> +				ETHTOOL_PHY_BRR_MODE_ON : ETHTOOL_PHY_BRR_MODE_OFF;
> +
> +	return 0;
> +}
> +
> +static int bcm54811_set_brrmode(struct phy_device *phydev, u8 on)
> +{
> +	int reg;
> +	int err;
> +
> +	reg = bcm_phy_read_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL);
> +
> +	if (on)
> +		reg |= BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> +	else
> +		reg &= ~BCM54810_EXP_BROADREACH_LRE_MISC_CTL_EN;
> +
> +	err = bcm_phy_write_exp(phydev, BCM54810_EXP_BROADREACH_LRE_MISC_CTL, reg);
> +	if (err)
>   		return err;
>   
> +	/* Ensure LRE or IEEE register set is accessed according to the brr on/off,
> +	 *  thus set the override
> +	 */
> +	return bcm_phy_write_exp(phydev, BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL,
> +		BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_EN |
> +		on ? 0 : BCM54811_EXP_BROADREACH_LRE_OVERLAY_CTL_OVERRIDE_VAL);
> +}
> +
> +static int bcm54811_config_init(struct phy_device *phydev)
> +{
> +	int err, reg;
> +
>   	err = bcm54xx_config_init(phydev);
>   
>   	/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
> @@ -576,16 +604,151 @@ static int bcm54811_config_init(struct phy_device *phydev)
>   			return err;
>   	}
>   
> -	return err;
> +	/* Configure BroadR-Reach function. */
> +	return  bcm54811_set_brrmode(phydev, ETHTOOL_PHY_BRR_MODE_OFF);
> +}
> +
> +static int bcm_setup_master_slave(struct phy_device *phydev)

The namespace in this driver is such that we prefix with bcm54xx_ or 
bcm<phy number>, this is generic so this could go to bcm-phy-lib.[ch] as 
a matter of fact.

> +{
> +	u16 ctl = 0;
> +
> +	switch (phydev->master_slave_set) {
> +	case MASTER_SLAVE_CFG_MASTER_PREFERRED:
> +	case MASTER_SLAVE_CFG_MASTER_FORCE:
> +		ctl = LRECR_MASTER;
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_PREFERRED:
> +		break;
> +	case MASTER_SLAVE_CFG_SLAVE_FORCE:
> +		break;
> +	case MASTER_SLAVE_CFG_UNKNOWN:
> +	case MASTER_SLAVE_CFG_UNSUPPORTED:
> +		return 0;
> +	default:
> +		phydev_warn(phydev, "Unsupported Master/Slave mode\n");
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return phy_modify_changed(phydev, MII_BCM54XX_LRECR, LRECR_MASTER, ctl);
> +}
> +
> +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);
> +}
> +
> +/**
> + * bcm_linkmode_adv_to_mii_adv_t
> + * @advertising: the linkmode advertisement settings
> + *
> + * A small helper function that translates linkmode advertisement
> + * settings to phy autonegotiation advertisements for the
> + * MII_BCM54XX_LREANAA register.
> + */
> +static inline u32 bcm_linkmode_adv_to_mii_adv_t(unsigned long *advertising)
> +{
> +	u32 result = 0;
> +
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1BR10_BIT, advertising))
> +		result |= LREANAA_10_1PAIR;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2BR10_BIT, advertising))
> +		result |= LREANAA_10_2PAIR;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_1BR100_BIT, advertising))
> +		result |= LREANAA_100_1PAIR;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_2BR100_BIT, advertising))
> +		result |= LREANAA_100_2PAIR;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_4BR100_BIT, advertising))
> +		result |= LREANAA_100_4PAIR;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Pause_BIT, advertising))
> +		result |= LRELPA_PAUSE;
> +	if (linkmode_test_bit(ETHTOOL_LINK_MODE_Asym_Pause_BIT, advertising))
> +		result |= LRELPA_PAUSE_ASYM;
> +
> +	return result;
> +}
> +
> +/**
> + * bcm_config_advert - sanitize and advertise auto-negotiation parameters
> + * @phydev: target phy_device struct
> + *
> + * Description: Writes MII_BCM54XX_LREANAA with the appropriate values,
> + *   after sanitizing the values to make sure we only advertise
> + *   what is supported.  Returns < 0 on error, 0 if the PHY's advertisement
> + *   hasn't changed, and > 0 if it has changed.
> + */
> +static int bcm_config_advert(struct phy_device *phydev)
> +{
> +	int err;
> +	u32 adv;
> +
> +	/* Only allow advertising what this PHY supports */
> +	linkmode_and(phydev->advertising, phydev->advertising,
> +		     phydev->supported);
> +
> +	adv = bcm_linkmode_adv_to_mii_adv_t(phydev->advertising);
> +
> +	/* Setup BroadR-Reach mode advertisement */
> +	err = phy_modify_changed(phydev, MII_BCM54XX_LREANAA,
> +				 LRE_ADVERTISE_ALL | LREANAA_PAUSE |
> +				 LREANAA_PAUSE_ASYM, adv);
> +
> +	if (err < 0)
> +		return err;
> +
> +	return err > 0 ? 1 : 0;
> +}
> +
> +static int bcm_config_aneg(struct phy_device *phydev, bool changed)
> +{
> +	int err;
> +
> +	if (genphy_config_eee_advert(phydev))
> +		changed = true;
> +
> +	err = bcm_setup_master_slave(phydev);
> +	if (err < 0)
> +		return err;
> +	else if (err)
> +		changed = true;
> +
> +	if (phydev->autoneg != AUTONEG_ENABLE)
> +		return bcm_setup_forced(phydev);
> +
> +	err = bcm_config_advert(phydev);
> +	if (err < 0) /* error */
> +		return err;
> +	else if (err)
> +		changed = true;
> +
> +	return genphy_check_and_restart_aneg(phydev, changed);
>   }
>   
>   static int bcm5481_config_aneg(struct phy_device *phydev)
>   {

I would create a bcm54811_config_aneg() function here which checks for 
the BRR mode.

>   	struct device_node *np = phydev->mdio.dev.of_node;
>   	int ret;
> +	u8 brr_mode;
>   
>   	/* Aneg firstly. */
> -	ret = genphy_config_aneg(phydev);
> +	ret = bcm54811_get_brrmode(phydev, &brr_mode);
> +	if (ret)
> +		return ret;
> +
> +	if (brr_mode == ETHTOOL_PHY_BRR_MODE_ON)
> +		ret = bcm_config_aneg(phydev, false);
> +	else
> +		ret = genphy_config_aneg(phydev);
>   
>   	/* Then we can set up the delay. */
>   	bcm54xx_config_clock_delay(phydev);
> @@ -1062,6 +1225,253 @@ static void bcm54xx_link_change_notify(struct phy_device *phydev)
>   	bcm_phy_write_exp(phydev, MII_BCM54XX_EXP_EXP08, ret);
>   }
>   
> +static int bcm54811_read_abilities(struct phy_device *phydev)
> +{
> +	int val, err;
> +	int i;
> +	static const int modes_array[] = {ETHTOOL_LINK_MODE_1BR100_BIT,
> +					  ETHTOOL_LINK_MODE_4BR100_BIT,
> +					  ETHTOOL_LINK_MODE_2BR100_BIT,
> +					  ETHTOOL_LINK_MODE_2BR10_BIT,
> +					  ETHTOOL_LINK_MODE_1BR10_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
> +					  ETHTOOL_LINK_MODE_1000baseT_Half_BIT,
> +					  ETHTOOL_LINK_MODE_100baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_100baseT_Half_BIT,
> +					  ETHTOOL_LINK_MODE_10baseT_Full_BIT,
> +					  ETHTOOL_LINK_MODE_10baseT_Half_BIT};
> +
> +	u8 brr_mode;
> +
> +	for (i = 0; i < ARRAY_SIZE(modes_array); i++)
> +		linkmode_clear_bit(modes_array[i], phydev->supported);
> +
> +	err = bcm54811_get_brrmode(phydev, &brr_mode);
> +
> +	if (!err) {

Please do an early return so you can reduce the indentation here, and 
elsewhere, too.
-- 
Florian


Download attachment "smime.p7s" of type "application/pkcs7-signature" (4221 bytes)

Powered by blists - more mailing lists