netdev - Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s max-speed

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 for Android: free password hash cracker in your pocket
[<prev] [next>] [day] [month] [year] [list]
Message-ID: <20190920134215.GM25745@shell.armlinux.org.uk>
Date:   Fri, 20 Sep 2019 14:42:15 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     tinywrkb <tinywrkb@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>
Cc:     Mark Rutland <mark.rutland@....com>, Andrew Lunn <andrew@...n.ch>,
        Baruch Siach <baruch@...s.co.il>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, Shawn Guo <shawnguo@...nel.org>,
        Sascha Hauer <s.hauer@...gutronix.de>,
        open list <linux-kernel@...r.kernel.org>,
        Rob Herring <robh+dt@...nel.org>,
        NXP Linux Team <linux-imx@....com>,
        Pengutronix Kernel Team <kernel@...gutronix.de>,
        Fabio Estevam <festevam@...il.com>,
        linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org
Subject: Re: [PATCH] ARM: dts: imx6dl: SolidRun: add phy node with 100Mb/s
 max-speed

On Tue, Sep 17, 2019 at 10:42:01PM +0100, Russell King - ARM Linux admin wrote:
> On Tue, Sep 17, 2019 at 06:19:13PM +0100, Russell King - ARM Linux admin wrote:
> > whether you can get the link to come up at all.  You might need to see
> > whether wiggling the RJ45 helps (I've had that sort of thing with some
> > cables.)
> > 
> > You might also need "ethtool -s eth0 advertise ffcf" after trying that
> > if it doesn't work to take the gigabit speeds out of the advertisement.
> > 
> > Thanks.
> > 
> >  drivers/net/phy/at803x.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> > index b3893347804d..85cf4a4a5e81 100644
> > --- a/drivers/net/phy/at803x.c
> > +++ b/drivers/net/phy/at803x.c
> > @@ -296,6 +296,11 @@ static int at803x_config_init(struct phy_device *phydev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > +	/* Disable smartspeed */
> > +	ret = phy_modify(phydev, 0x14, BIT(5), 0);
> > +	if (ret < 0)
> > +		return ret;
> > +
> >  	/* The RX and TX delay default is:
> >  	 *   after HW reset: RX delay enabled and TX delay disabled
> >  	 *   after SW reset: RX delay enabled, while TX delay retains the
> 
> Hi,
> 
> Could you try this patch instead - it seems that the PHY needs to be
> soft-reset for the write to take effect, and _even_ for the clearance
> of the bit to become visible in the register.
> 
> I'm not expecting this on its own to solve anything, but it should at
> least mean that the at803x doesn't modify the advertisement registers
> itself.  It may mean that the link doesn't even come up without forcing
> the advertisement via the ethtool command I mentioned before.
> 
> Thanks.
> 
>  drivers/net/phy/at803x.c | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index b3893347804d..69a58c0e6b42 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -296,6 +296,16 @@ static int at803x_config_init(struct phy_device *phydev)
>  	if (ret < 0)
>  		return ret;
>  
> +	/* Disable smartspeed */
> +	ret = phy_modify(phydev, 0x14, BIT(5), 0);
> +	if (ret < 0)
> +		return ret;
> +
> +	/* Must soft-reset the PHY for smartspeed disable to take effect */
> +	ret = genphy_soft_reset(phydev);
> +	if (ret < 0)
> +		return ret;
> +
>  	/* The RX and TX delay default is:
>  	 *   after HW reset: RX delay enabled and TX delay disabled
>  	 *   after SW reset: RX delay enabled, while TX delay retains the

Bad news I'm afraid.  It looks like the AR8035 has a bug in it.
Disabling the SmartSpeed feature appears to make register 9, the
1000BASET control register, read-only.

For example:

Reading 0x0009=0x0200
Writing 0x0014=0x082c	<= smartspeed enabled
Writing 0x0000=0xb100	<= soft reset
Writing 0x0009=0x0600
Reading 0x0009=0x0600	<= it took the value

Reading 0x0009=0x0600
Writing 0x0014=0x080c	<= smartspeed disabled
Writing 0x0000=0xb100	<= soft reset
Writing 0x0009=0x0200
Reading 0x0009=0x0600	<= it ignored the write

Reading 0x0009=0x0600
Writing 0x0014=0x082c	<= smartspeed enabled
Writing 0x0000=0xb100	<= soft reset
Writing 0x0009=0x0200
Reading 0x0009=0x0200	<= it took the value

If it's going to make register 9 read-only when smartspeed is disabled,
then that's another failure mode and autonegotiation cockup just
waiting to happen - which I spotted when trying to configure the
advertisement using ethtool, and finding that it was impossible to stop
1000baseT/Full being advertised.

I think the only sane approach - at least until we have something more
reasonable in place - is to base the negotiation result off what is
actually stored in the PHY registers at the time the link comes up, and
not on the cached versions of what we should be advertising.

5502b218e001 has caused this regression, and where we are now after
more than a week of trying to come up with some fix for this
regression, the only solution that seems to work without introducing
more failures is to revert that commit.

Adding Heiner (original commit author), Florian, David and netdev.

Thoughts?

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up