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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87f06cb4-3bee-3ccb-bb21-ce6943e75336@gmail.com>
Date:   Fri, 12 Feb 2021 17:26:02 -0800
From:   Florian Fainelli <f.fainelli@...il.com>
To:     Robert Hancock <robert.hancock@...ian.com>, andrew@...n.ch,
        hkallweit1@...il.com, davem@...emloft.net, kuba@...nel.org
Cc:     f.fainelli@...il.com, linux@...linux.org.uk,
        bcm-kernel-feedback-list@...adcom.com, netdev@...r.kernel.org
Subject: Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper
 1000BaseX/SGMII interface mode for BCM54616S



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@...ian.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