Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 29 Jul 2022 11:05:13 +0200
From:   Oleksij Rempel <o.rempel@pengutronix.de>
To:     Andrew Lunn <andrew@lunn.ch>
Cc:     Woojung Huh <woojung.huh@microchip.com>,
        UNGLinuxDriver@microchip.com,
        Vivien Didelot <vivien.didelot@gmail.com>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net v1 1/1] net: dsa: microchip: don't try do read Gbit
 registers on non Gbit chips
Message-ID: <20220729090513.GA10850@pengutronix.de>
References: <20220728131725.40492-1-o.rempel@pengutronix.de>
 <YuKOTzS89D2+O8Ye@lunn.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <YuKOTzS89D2+O8Ye@lunn.ch>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On Thu, Jul 28, 2022 at 03:25:35PM +0200, Andrew Lunn wrote:
> On Thu, Jul 28, 2022 at 03:17:25PM +0200, Oleksij Rempel wrote:
> > Do not try to read not existing or wrong register on chips without
> > GBIT_SUPPORT.
> > 
> > Fixes: c2e866911e25 ("net: dsa: microchip: break KSZ9477 DSA driver into two files")
> > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de>
> > ---
> >  drivers/net/dsa/microchip/ksz9477.c | 8 +++++++-
> >  1 file changed, 7 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
> > index c73bb6d383ad..f6bbd9646c85 100644
> > --- a/drivers/net/dsa/microchip/ksz9477.c
> > +++ b/drivers/net/dsa/microchip/ksz9477.c
> > @@ -316,7 +316,13 @@ void ksz9477_r_phy(struct ksz_device *dev, u16 addr, u16 reg, u16 *data)
> >  			break;
> >  		}
> >  	} else {
> > -		ksz_pread16(dev, addr, 0x100 + (reg << 1), &val);
> > +		/* No gigabit support.  Do not read wrong registers. */
> > +		if (!(dev->features & GBIT_SUPPORT) &&
> > +		    (reg == MII_CTRL1000 || reg == MII_ESTATUS ||
> > +		     reg == MII_STAT1000))
> 
> Does this actually happen?
> 
> If i remember this code correctly, it tries to make the oddly looking
> PHY look like a normal PHY. phylib is then used to drive the PHY?
> 
> If i have that correct, why is phylib trying to read these registers?
> It should know there is no 1G support, and should skip them.

It looks like currently undocumented silicon errata. According to the
data sheet, the BMSR_ESTATEN should not be set BMSR_ERCAP, but this bits
are set.

The question is what is the proper place to implement it. There is same
PHYid for most KSZ switch PHYs, it is no possible to detect it by PHYid.
I have following options:
- add chips specific quirk in the ksz9477_r_phy(), just remove
  BMSR_ESTATEN and BMSR_ERCAP.
- notify about errata over get_phy_flags and implement get_caps quirk in
  the PHY driver.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |