Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper 1000BaseX/SGMII interface mode for BCM54616S

[Florian Fainelli <f.fainelli@gmail.com>]

On 2/12/2021 4:28 PM, 'Robert Hancock' via BCM-KERNEL-FEEDBACK-LIST,PDL
wrote:
> The default configuration for the BCM54616S PHY may not match the desired
> mode when using 1000BaseX or SGMII interface modes, such as when it is on
> an SFP module. Add code to explicitly set the correct mode using
> programming sequences provided by Bel-Fuse:
> 
> https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-05-series.pdf
> https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-06-series.pdf
> 
> Signed-off-by: Robert Hancock <robert.hancock@calian.com>
> ---
>  drivers/net/phy/broadcom.c | 83 ++++++++++++++++++++++++++++++++------
>  include/linux/brcmphy.h    |  4 ++
>  2 files changed, 75 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/net/phy/broadcom.c b/drivers/net/phy/broadcom.c
> index 0472b3470c59..78542580f2b2 100644
> --- a/drivers/net/phy/broadcom.c
> +++ b/drivers/net/phy/broadcom.c
> @@ -64,6 +64,63 @@ static int bcm54612e_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int bcm54616s_config_init(struct phy_device *phydev)
> +{
> +	int rc, val;
> +
> +	if (phydev->interface == PHY_INTERFACE_MODE_SGMII ||
> +	    phydev->interface == PHY_INTERFACE_MODE_1000BASEX) {

Can you reverse the condition so as to save a level of identation?

> +		/* Ensure proper interface mode is selected. */
> +		/* Disable RGMII mode */
> +		val = bcm54xx_auxctl_read(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC);
> +		if (val < 0)
> +			return val;
> +		val &= ~MII_BCM54XX_AUXCTL_SHDWSEL_MISC_RGMII_EN;
> +		rc = bcm54xx_auxctl_write(phydev, MII_BCM54XX_AUXCTL_SHDWSEL_MISC,
> +					  val);
> +		if (rc < 0)
> +			return rc;

I don't think this write is making it through since you are not setting
MII_BCM54XX_AUXCTL_MISC_WREN in val, I know this is an annoying detail,
and we could probably fold that to be within bcm54xx_auxctl_write()
directly, similarly to what bcm_phy_write_shadow() does.

The reset of the sequence and changes looks fine to me.
-- 
Florian