Re: [PATCH net-next 1/2] net: phy: phy-c45: Auto-negotiaion changes for T1 phy in phy library

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Mon, Dec 09, 2024 at 09:44:26PM +0530, Tarun Alle wrote:
> Below auto-negotiation library changes required for T1 phys:
> - Lower byte advertisement register need to read after higher byte as
>   per 802.3-2022 : Section 45.2.7.22.

In my copy of 802.3, this refers to the link partner base page
ability register, which is not the same as the advertisement
register.

The advertisement registers are covered by the preceeding section,
45.2.7.21. This says:

"The Base Page value is transferred to mr_adv_ability when register
7.514 is written. Therefore, registers 7.515 and 7.516 should be
written before 7.514."

which I think is what's pertinent to your commit.

> - Link status need to be get from control T1 registers for T1 phys.

This statement appears to be inaccurate - more below against the
actual code change.

> 
> Signed-off-by: Tarun Alle <Tarun.Alle@microchip.com>
> ---
>  drivers/net/phy/phy-c45.c | 36 ++++++++++++++++++++++++++----------
>  1 file changed, 26 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 0dac08e85304..85d8a9b9c3f6 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -234,15 +234,11 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
>  		return -EOPNOTSUPP;
>  	}
>  
> -	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
> -
> -	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
> -				     adv_l_mask, adv_l);
> -	if (ret < 0)
> -		return ret;
> -	if (ret > 0)
> -		changed = 1;
> -
> +	/* Ref. 802.3-2022 : Section 45.2.7.22
> +	 * The Base Page value is transferred to mr_adv_ability when register
> +	 * 7.514 is written.
> +	 * Therefore, registers 7.515 and 7.516 should be written before 7.514.
> +	 */
>  	adv_m |= linkmode_adv_to_mii_t1_adv_m_t(phydev->advertising);
>  
>  	ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_M,
> @@ -252,6 +248,23 @@ static int genphy_c45_baset1_an_config_aneg(struct phy_device *phydev)
>  	if (ret > 0)
>  		changed = 1;
>  
> +	adv_l |= linkmode_adv_to_mii_t1_adv_l_t(phydev->advertising);
> +
> +	if (changed) {
> +		ret = phy_write_mmd(phydev, MDIO_MMD_AN, MDIO_AN_T1_ADV_L,
> +				    adv_l);
> +		if (ret < 0)
> +			return ret;
> +	} else {
> +		ret = phy_modify_mmd_changed(phydev, MDIO_MMD_AN,
> +					     MDIO_AN_T1_ADV_L,
> +					     adv_l_mask, adv_l);

Why do you write the complete register if changed is true, but only
modify bits 12, 11 and 10 if changed is false?

>  int genphy_c45_read_link(struct phy_device *phydev)
>  {
>  	u32 mmd_mask = MDIO_DEVS_PMAPMD;
> +	u16 reg = MDIO_CTRL1;
>  	int val, devad;
>  	bool link = true;
>  
>  	if (phydev->c45_ids.mmds_present & MDIO_DEVS_AN) {
> -		val = phy_read_mmd(phydev, MDIO_MMD_AN, MDIO_CTRL1);
> +		if (genphy_c45_baset1_able(phydev))
> +			reg = MDIO_AN_T1_CTRL;
> +		val = phy_read_mmd(phydev, MDIO_MMD_AN, reg);
>  		if (val < 0)
>  			return val;

This is not checking link status as you mention in your commit
message, it is checking whether the PHY is in the process of
restarting autoneg.

Thanks.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!