Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops

[Álvaro Fernández Rojas]

Hi Jonas,

El mar, 21 mar 2023 a las 11:36, Jonas Gorski
(<jonas.gorski@xxxxxxxxx>) escribió:
>
> On Mon, 20 Mar 2023 at 19:28, Álvaro Fernández Rojas <noltari@xxxxxxxxx> wrote:
> >
> > Currently, B53 MMAP BCM63xx devices with an external switch hang when
> > performing PHY read and write operations due to invalid registers access.
> > This adds support for PHY ops by using the internal bus from mdio-mux-bcm6368
> > when probed by device tree and also falls back to direct MDIO registers if not.
> >
> > This is an alternative to:
> > - https://patchwork.kernel.org/project/netdevbpf/cover/20230317113427.302162-1-noltari@xxxxxxxxx/
> > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-2-noltari@xxxxxxxxx/
> > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-3-noltari@xxxxxxxxx/
> > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-4-noltari@xxxxxxxxx/
> > As discussed, it was an ABI break and not the correct way of fixing the issue.
> >
> > Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx>
> > ---
> >  drivers/net/dsa/b53/b53_mmap.c    | 86 +++++++++++++++++++++++++++++++
> >  include/linux/platform_data/b53.h |  1 +
> >  2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/net/dsa/b53/b53_mmap.c b/drivers/net/dsa/b53/b53_mmap.c
> > index 706df04b6cee..7deca1c557c5 100644
> > --- a/drivers/net/dsa/b53/b53_mmap.c
> > +++ b/drivers/net/dsa/b53/b53_mmap.c
> > @@ -19,14 +19,25 @@
> >  #include <linux/bits.h>
> >  #include <linux/kernel.h>
> >  #include <linux/module.h>
> > +#include <linux/of_mdio.h>
> >  #include <linux/io.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/platform_data/b53.h>
> >
> >  #include "b53_priv.h"
> >
> > +#define REG_MDIOC              0xb0
> > +#define  REG_MDIOC_EXT_MASK    BIT(16)
> > +#define  REG_MDIOC_REG_SHIFT   20
> > +#define  REG_MDIOC_PHYID_SHIFT 25
> > +#define  REG_MDIOC_RD_MASK     BIT(30)
> > +#define  REG_MDIOC_WR_MASK     BIT(31)
> > +
> > +#define REG_MDIOD              0xb4
> > +
> >  struct b53_mmap_priv {
> >         void __iomem *regs;
> > +       struct mii_bus *bus;
> >  };
> >
> >  static int b53_mmap_read8(struct b53_device *dev, u8 page, u8 reg, u8 *val)
> > @@ -216,6 +227,69 @@ static int b53_mmap_write64(struct b53_device *dev, u8 page, u8 reg,
> >         return 0;
> >  }
> >
> > +static inline void b53_mmap_mdio_read(struct b53_device *dev, int phy_id,
> > +                                     int loc, u16 *val)
> > +{
> > +       uint32_t reg;
> > +
> > +       b53_mmap_write32(dev, 0, REG_MDIOC, 0);
> > +
> > +       reg = REG_MDIOC_RD_MASK |
> > +             (phy_id << REG_MDIOC_PHYID_SHIFT) |
> > +             (loc << REG_MDIOC_REG_SHIFT);
> > +
> > +       b53_mmap_write32(dev, 0, REG_MDIOC, reg);
> > +       udelay(50);
> > +       b53_mmap_read16(dev, 0, REG_MDIOD, val);
> > +}
> > +
> > +static inline int b53_mmap_mdio_write(struct b53_device *dev, int phy_id,
> > +                                     int loc, u16 val)
>
> On nitpick here: AFACT, what you are actually getting there as phy_id
> isn't the phy_id but the port_id, it just happens to be identical for
> internal ports.
>
> So in theory you would first need to convert this to the appropriate
> phy_id (+ which bus) first, else you risk reading from the wrong
> device (and/or bus).

I agree with you and your suggestion gave me an idea, what if
phy_read/phy_write wasn't set in b53 dsa_switch_ops for mmap?

So I implemented the following patch:
https://gist.github.com/Noltari/cfecb29d6401d06b9cb5dd199607918b#file-net-dsa-b53-mmap-disable-phy-read-write-patch

And this is the result:
https://gist.github.com/Noltari/cfecb29d6401d06b9cb5dd199607918b#file-net-dsa-b53-mmap-disable-phy-read-write-log

As you can see, bcm6368-mdio-mux is now used for every mii access as
it should have been from the beginning...
So I guess that the correct way of fixing the issue would be to
disable phy read/write from b53 mmap. However, I don't know if the
patch that I provided is correct, or if I should remove those from
dsa_switch_ops in any other way (I'm open to suggestions).

>
> See how the phys_mii_mask is based on the indexes of the user ports,
> not their actual phy_ids. [1] [2]
>
> [1] https://elixir.bootlin.com/linux/latest/source/net/dsa/dsa.c#L660
> [2] https://elixir.bootlin.com/linux/latest/source/include/net/dsa.h#L596
>
> Regards
> Jonas

--
Best regards,
Álvaro