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

[Andrew Lunn]

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

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

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

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

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

    Andrew

---
pw-bot: cr