Messages in this thread | | | From | Álvaro Fernández Rojas <> | Date | Mon, 20 Mar 2023 20:58:32 +0100 | Subject | Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops |
| |
El lun, 20 mar 2023 a las 20:06, Florian Fainelli (<f.fainelli@gmail.com>) escribió: > > On 3/20/23 11:28, Álvaro Fernández Rojas 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@gmail.com/ > > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-2-noltari@gmail.com/ > > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-3-noltari@gmail.com/ > > - https://patchwork.kernel.org/project/netdevbpf/patch/20230317113427.302162-4-noltari@gmail.com/ > > As discussed, it was an ABI break and not the correct way of fixing the issue. > > Looks good for the most part, just a few questions below. > > > > > Signed-off-by: Álvaro Fernández 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_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) > > For some reason, there was no bit introduced to tell us when a > transaction has finished, so we have to poll after a certain delay has > elapsed...
Yeah... :(
> > > + > > +#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) > > +{ > > + uint32_t reg; > > + > > + b53_mmap_write32(dev, 0, REG_MDIOC, 0); > > + > > + reg = REG_MDIOC_WR_MASK | > > + (phy_id << REG_MDIOC_PHYID_SHIFT) | > > + (loc << REG_MDIOC_REG_SHIFT) | > > + val; > > + > > + b53_mmap_write32(dev, 0, REG_MDIOC, reg); > > + udelay(50); > > + > > + return 0; > > +} > > + > > +static int b53_mmap_phy_read16(struct b53_device *dev, int addr, int reg, > > + u16 *value) > > +{ > > + struct b53_mmap_priv *priv = dev->priv; > > + struct mii_bus *bus = priv->bus; > > + > > + if (bus) > > + *value = mdiobus_read_nested(bus, addr, reg); > > Since you make the 'mii-bus' property and 'priv->bus' necessary > prerequisites for the driver to finish probing successfully, when shall > we not have valid priv->bus reference to work with? Do we end-up taking > the other path at all?
Yes, but only if the driver is probed from platform data and not from device tree.
> -- > Florian >
-- Álvaro.
| |