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

[Álvaro Fernández Rojas]

2023-03-20 19:28 GMT+01:00, Álvaro Fernández Rojas <noltari@xxxxxxxxx>:
> 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;

Can we reuse "bus" from b53_device instead of adding a new one for
b53_mmap_priv?

>  };
>
>  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);
> +	else
> +		b53_mmap_mdio_read(dev, addr, reg, value);
> +
> +	return 0;
> +}
> +
> +static int b53_mmap_phy_write16(struct b53_device *dev, int addr, int reg,
> +				u16 value)
> +{
> +	struct b53_mmap_priv *priv = dev->priv;
> +	struct mii_bus *bus = priv->bus;
> +	int ret;
> +
> +	if (bus)
> +		ret = mdiobus_write_nested(bus, addr, reg, value);
> +	else
> +		ret = b53_mmap_mdio_write(dev, addr, reg, value);
> +
> +	return ret;
> +}
> +
>  static const struct b53_io_ops b53_mmap_ops = {
>  	.read8 = b53_mmap_read8,
>  	.read16 = b53_mmap_read16,
> @@ -227,6 +301,8 @@ static const struct b53_io_ops b53_mmap_ops = {
>  	.write32 = b53_mmap_write32,
>  	.write48 = b53_mmap_write48,
>  	.write64 = b53_mmap_write64,
> +	.phy_read16 = b53_mmap_phy_read16,
> +	.phy_write16 = b53_mmap_phy_write16,
>  };
>
>  static int b53_mmap_probe_of(struct platform_device *pdev,
> @@ -234,6 +310,7 @@ static int b53_mmap_probe_of(struct platform_device
> *pdev,
>  {
>  	struct device_node *np = pdev->dev.of_node;
>  	struct device_node *of_ports, *of_port;
> +	struct device_node *mdio;
>  	struct device *dev = &pdev->dev;
>  	struct b53_platform_data *pdata;
>  	void __iomem *mem;
> @@ -251,6 +328,14 @@ static int b53_mmap_probe_of(struct platform_device
> *pdev,
>  	pdata->chip_id = (u32)device_get_match_data(dev);
>  	pdata->big_endian = of_property_read_bool(np, "big-endian");
>
> +	mdio = of_parse_phandle(np, "mii-bus", 0);

Is "mii-bus" OK or shall we use something like "brcm,mii-bus"?

> +	if (!mdio)
> +		return -EINVAL;
> +
> +	pdata->bus = of_mdio_find_bus(mdio);
> +	if (!pdata->bus)
> +		return -EPROBE_DEFER;
> +
>  	of_ports = of_get_child_by_name(np, "ports");
>  	if (!of_ports) {
>  		dev_err(dev, "no ports child node found\n");
> @@ -297,6 +382,7 @@ static int b53_mmap_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>
>  	priv->regs = pdata->regs;
> +	priv->bus = pdata->bus;
>
>  	dev = b53_switch_alloc(&pdev->dev, &b53_mmap_ops, priv);
>  	if (!dev)
> diff --git a/include/linux/platform_data/b53.h
> b/include/linux/platform_data/b53.h
> index 6f6fed2b171d..be0c5bfdedad 100644
> --- a/include/linux/platform_data/b53.h
> +++ b/include/linux/platform_data/b53.h
> @@ -32,6 +32,7 @@ struct b53_platform_data {
>  	/* only used by MMAP'd driver */
>  	unsigned big_endian:1;
>  	void __iomem *regs;
> +	struct mii_bus *bus;
>  };
>
>  #endif
> --
> 2.30.2
>
>