MIME-Version: 1.0
References: <20230320182813.963508-1-noltari@gmail.com> <CAOiHx=nKVWfa1-_VAf3bz+6PPz0uWMHyEtoVVOysFf0srZorBA@mail.gmail.com>
In-Reply-To: <CAOiHx=nKVWfa1-_VAf3bz+6PPz0uWMHyEtoVVOysFf0srZorBA@mail.gmail.com>
From:   =?UTF-8?B?w4FsdmFybyBGZXJuw6FuZGV6IFJvamFz?= <noltari@gmail.com>
Date:   Tue, 21 Mar 2023 19:04:09 +0100
Message-ID: <CAKR-sGdpck1GPMpqM3M-H6Bz_mp+xiV-o6ZuR6QZATNA_=Xa0A@mail.gmail.com>
Subject: Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
To:     Jonas Gorski <jonas.gorski@gmail.com>
Cc:     f.fainelli@gmail.com, andrew@lunn.ch, olteanv@gmail.com,
        davem@davemloft.net, edumazet@google.com, kuba@kernel.org,
        pabeni@redhat.com, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Jonas,

El mar, 21 mar 2023 a las 11:36, Jonas Gorski
(<jonas.gorski@gmail.com>) escribi=C3=B3:
>
> On Mon, 20 Mar 2023 at 19:28, =C3=81lvaro Fern=C3=A1ndez Rojas <noltari@g=
mail.com> wrote:
> >
> > Currently, B53 MMAP BCM63xx devices with an external switch hang when
> > performing PHY read and write operations due to invalid registers acces=
s.
> > This adds support for PHY ops by using the internal bus from mdio-mux-b=
cm6368
> > 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.3=
02162-1-noltari@gmail.com/
> > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.3=
02162-2-noltari@gmail.com/
> > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.3=
02162-3-noltari@gmail.com/
> > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.3=
02162-4-noltari@gmail.com/
> > As discussed, it was an ABI break and not the correct way of fixing the=
 issue.
> >
> > Signed-off-by: =C3=81lvaro Fern=C3=A1ndez Rojas <noltari@gmail.com>
> > ---
> >  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_m=
map.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 =3D 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-d=
sa-b53-mmap-disable-phy-read-write-patch

And this is the result:
https://gist.github.com/Noltari/cfecb29d6401d06b9cb5dd199607918b#file-net-d=
sa-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,
=C3=81lvaro