On Fri, Jul 29, 2022 at 11:05:13AM +0200, Oleksij Rempel wrote:

On Thu, Jul 28, 2022 at 03:25:35PM +0200, Andrew Lunn wrote:

quoted

On Thu, Jul 28, 2022 at 03:17:25PM +0200, Oleksij Rempel wrote:

quoted

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.

I would do the first. The DSA driver is already doing some emulation
of a normal PHY, so it seems odd to push a workaround into the PHY
driver when it can be part of the emulation.

    Andrew