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]

Date:   Sat, 13 Feb 2021 01:45:03 +0000
From:   Robert Hancock <robert.hancock@...ian.com>
To:     "hkallweit1@...il.com" <hkallweit1@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "kuba@...nel.org" <kuba@...nel.org>,
        "andrew@...n.ch" <andrew@...n.ch>
CC:     "linux@...linux.org.uk" <linux@...linux.org.uk>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bcm-kernel-feedback-list@...adcom.com" 
        <bcm-kernel-feedback-list@...adcom.com>
Subject: Re: [PATCH net-next 1/2] net: phy: broadcom: Set proper
 1000BaseX/SGMII interface mode for BCM54616S

On Fri, 2021-02-12 at 17:26 -0800, Florian Fainelli wrote:
> 
> 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://urldefense.com/v3/__https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-05-series.pdf__;!!IOGos0k!20FhZqRHEiz2-qFJ7J8XC4xX2qG-ajZ17Ma1W-VwDgwdQZeIhHEpWKlNldWW8DyFaQo$ 
> > https://urldefense.com/v3/__https://www.belfuse.com/resources/datasheets/powersolutions/ds-bps-sfp-1gbt-06-series.pdf__;!!IOGos0k!20FhZqRHEiz2-qFJ7J8XC4xX2qG-ajZ17Ma1W-VwDgwdQZeIhHEpWKlNldWW58K3fY4$ 
> > 
> > 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?

Can do.

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

Ah, indeed. I assume that is specific to the MII_BCM54XX_AUXCTL_SHDWSEL_MISC
register? I suppose bcm54xx_auxctl_write could add that automatically for
writes to that register. Not sure if that is too much magic for that function
or not..

> 
> The reset of the sequence and changes looks fine to me.
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com