Message-ID: <bc1ce748-7620-45b0-b1ad-17d77f6d6331@axis.com>
Date: Fri, 12 Jul 2024 17:10:48 +0200
From: Kamil Horák (2N) <kamilh@...s.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: florian.fainelli@...adcom.com, bcm-kernel-feedback-list@...adcom.com,
 hkallweit1@...il.com, linux@...linux.org.uk, davem@...emloft.net,
 edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com, robh@...nel.org,
 krzk+dt@...nel.org, conor+dt@...nel.org, netdev@...r.kernel.org,
 devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v11 4/4] net: phy: bcm-phy-lib: Implement BroadR-Reach
 link modes


On 7/11/24 21:01, Andrew Lunn wrote:
>> +static int bcm5481x_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)
> bcm_phy_read_exp() could fail. So you should keep the test. Also, the
> caller of this function does look at the return value.
True - it can at least return -EOPNOTSUPP from __mdiobus_read()
Trying to handle it.

This neglect can be found elsewhere such as bcm-phy-ptp.c  and eg. 
bcm54xx_config_init()

function. I feel that at least the latest one should be fixed but it 
would be unrelated to bcm54811,

so leaving it as-is for now.

>
>> +/**
>> + * bcm5481x_read_abilities - read PHY abilities from LRESR or Clause 22
>> + * (BMSR) registers, based on whether the PHY is in BroadR-Reach or IEEE mode
>> + * @phydev: target phy_device struct
>> + *
>> + * Description: Reads the PHY's abilities and populates
>> + * phydev->supported accordingly. The register to read the abilities from is
>> + * determined by current brr mode setting of the PHY.
>> + * Note that the LRE and IEEE sets of abilities are disjunct.
>> + *
>> + * Returns: 0 on success, < 0 on failure
>> + */
>> +static int bcm5481x_read_abilities(struct phy_device *phydev)
>> +{
>> +	int i, val, err;
>> +	u8 brr_mode;
>> +
>> +	for (i = 0; i < ARRAY_SIZE(bcm54811_linkmodes); i++)
>> +		linkmode_clear_bit(bcm54811_linkmodes[i], phydev->supported);
>> +
>> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
>> +static int bcm5481x_set_brrmode(struct phy_device *phydev, bool on)
>> +{
>> +	int reg;
>> +	int err;
>> +	u16 val;
>> +
>> +	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;
>> +
>> +static int bcm54811_config_init(struct phy_device *phydev)
>> +{
>> +	struct device_node *np = phydev->mdio.dev.of_node;
>> +	bool brr = false;
>> +	int err, reg;
>> +
>>   	err = bcm54xx_config_init(phydev);
>>   
>>   	/* Enable CLK125 MUX on LED4 if ref clock is enabled. */
>> @@ -576,29 +687,80 @@ static int bcm54811_config_init(struct phy_device *phydev)
>>   			return err;
>>   	}
>>   
>> -	return err;
>> +	/* Configure BroadR-Reach function. */
>> +	brr = of_property_read_bool(np, "brr-mode");
>> +
>> +	/* With BCM54811, BroadR-Reach implies no autoneg */
>> +	if (brr)
>> +		phydev->autoneg = 0;
>> +
>> +	return bcm5481x_set_brrmode(phydev, brr);
>>   }
> The ordering seems a bit strange here.
>
> phy_probe() will call phydrv->get_features. At this point, the PHY is
> in whatever mode it resets to, or maybe what it is strapped
> to. phydev->supported could thus be set to standard IEEE modes,
> despite the board design is actually for BroadR-Reach.
>
> Sometime later, when the MAC is connected to the PHY config_init() is
> called. At that point, you poke around in DT and find how the PHY is
> connected to the cable. At that point, you set the PHY mode, and
> change phydev->supported to reflect reality.
>
> I really think that reading DT should be done much earlier, maybe in
> the driver probe function, or maybe get_features. get_features should
> always return the correct values from the board.

Also true. This is a remnant of original approach using phy-tunable 
rather than dt to select the mode.

I do not expect a hot swappable design to be possible to appear, so 
fixing the logic as suggested.

>
>> +static int bcm5481_config_aneg(struct phy_device *phydev)
>> +{
>> +	u8 brr_mode;
>> +	int ret;
>> +
>> +	ret = bcm5481x_get_brrmode(phydev, &brr_mode);
> Rather than read it from the hardware every single time, could you
> store the DT value in bcm54xx_phy_priv ?

Done. Now we rely on the DT setting and never read the PHY state. It is 
vulnerable to external manipulation

of MDIO registers and PHY reset as both hardware and software (bit 15 of 
register 0 in both

IEEE and LRE modes) reset switch to IEEE mode.

>
>> +/* Read LDS Link Partner Ability in BroadR-Reach mode */
>> +static int bcm_read_lpa(struct phy_device *phydev)
> This function seems to be missing an lds or lre prefix.
>
>> +static int bcm_read_status_fixed(struct phy_device *phydev)
> and here. Please make sure the naming is consistent. Anything which
> only accesses lre or lds registers should make that clear in its name.

I feel this requires renaming stuff like bcm_read_status_fixed to 
lre_read_status_fixed etc. in every location

only handling LRE stuff, same logic already applied to eg. lre_update_link.

>
>> +static int bcm54811_read_status(struct phy_device *phydev)
>> +{
>> +	u8 brr_mode;
>> +	int err;
>> +
>> +	err = bcm5481x_get_brrmode(phydev, &brr_mode);
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	if (brr_mode) {
>> +		/* Get the status in BroadRReach mode just like
>> +		 *   genphy_read_status does in normal mode
>> +		 */
>> +
>> +		int err, old_link = phydev->link;
>> +
>> +		/* Update the link, but return if there was an error */
>> +
>> +		err = lre_update_link(phydev);
>> +		if (err)
>> +			return err;
>> +
>> +		/* why bother the PHY if nothing can have changed */
>> +		if (phydev->autoneg ==
>> +		    AUTONEG_ENABLE && old_link && phydev->link)
>> +			return 0;
>> +
>> +		phydev->speed = SPEED_UNKNOWN;
>> +		phydev->duplex = DUPLEX_UNKNOWN;
>> +		phydev->pause = 0;
>> +		phydev->asym_pause = 0;
>> +
>> +		err = bcm_read_master_slave(phydev);
>> +		if (err < 0)
>> +			return err;
>> +
>> +		/* Read LDS Link Partner Ability */
>> +		err = bcm_read_lpa(phydev);
>> +		if (err < 0)
>> +			return err;
>> +
>> +		if (phydev->autoneg ==
>> +		    AUTONEG_ENABLE && phydev->autoneg_complete) {
>> +			phy_resolve_aneg_linkmode(phydev);
>> +		} else if (phydev->autoneg == AUTONEG_DISABLE) {
>> +			err = bcm_read_status_fixed(phydev);
>> +			if (err < 0)
>> +				return err;
>> +		}
> This would probably look better if you pulled this code out into a
> helper bcm54811_lre_read_status().
done

I've of course verified again that it works on the target device but 
unfortunately, I have no possibility

to test it on anything using BCM54811 in IEEE mode and with BCM54810 
which might be interesting for

someone using that PHY.


Kamil


>
>      Andrew
>
> ---
> pw-bot: cr

Powered by blists - more mailing lists