| Message ID | 20220728131725.40492-1-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v1,1/1] net: dsa: microchip: don't try do read Gbit registers on non Gbit chips | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net |
| netdev/apply | fail | Patch does not apply to net |
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. Andrew
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? Yes. I just discovered it after adding regmap ranges validation for KSZ9893 chip. > > 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. I didn't investigated it so far. Will try to look deeper tomorrow. Regards, Oleksij
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
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: > > 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. 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
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)) + val = 0; + else + ksz_pread16(dev, addr, 0x100 + (reg << 1), &val); } *data = val;
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(-)