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 <[email protected]>
> ---
> 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 <[email protected]>
> > ---
> > 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
--
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 |
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 <[email protected]>
> > ---
> > 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 |
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 <[email protected]>
> > > ---
> > > 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