linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
@ 2023-03-20 18:28 Álvaro Fernández Rojas
  2023-03-20 19:06 ` Florian Fainelli
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Álvaro Fernández Rojas @ 2023-03-20 18:28 UTC (permalink / raw)
  To: f.fainelli, jonas.gorski, andrew, olteanv, davem, edumazet, kuba,
	pabeni, netdev, linux-kernel
  Cc: Álvaro Fernández Rojas

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.

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)
+
+#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);
+	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);
+	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


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
  2023-03-20 18:28 [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops Álvaro Fernández Rojas
@ 2023-03-20 19:06 ` Florian Fainelli
  2023-03-20 19:58   ` Álvaro Fernández Rojas
  2023-03-21  9:32 ` Álvaro Fernández Rojas
  2023-03-21 10:35 ` Jonas Gorski
  2 siblings, 1 reply; 6+ messages in thread
From: Florian Fainelli @ 2023-03-20 19:06 UTC (permalink / raw)
  To: Álvaro Fernández Rojas, jonas.gorski, andrew, olteanv,
	davem, edumazet, kuba, pabeni, netdev, linux-kernel

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...

> +
> +#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?
-- 
Florian


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
  2023-03-20 19:06 ` Florian Fainelli
@ 2023-03-20 19:58   ` Álvaro Fernández Rojas
  0 siblings, 0 replies; 6+ messages in thread
From: Álvaro Fernández Rojas @ 2023-03-20 19:58 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: jonas.gorski, andrew, olteanv, davem, edumazet, kuba, pabeni,
	netdev, linux-kernel

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.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
  2023-03-20 18:28 [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops Álvaro Fernández Rojas
  2023-03-20 19:06 ` Florian Fainelli
@ 2023-03-21  9:32 ` Álvaro Fernández Rojas
  2023-03-21 10:35 ` Jonas Gorski
  2 siblings, 0 replies; 6+ messages in thread
From: Álvaro Fernández Rojas @ 2023-03-21  9:32 UTC (permalink / raw)
  To: f.fainelli, jonas.gorski, andrew, olteanv, davem, edumazet, kuba,
	pabeni, netdev, linux-kernel
  Cc: Álvaro Fernández Rojas

2023-03-20 19:28 GMT+01:00, Álvaro Fernández Rojas <noltari@gmail.com>:
> 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.
>
> 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)
> +
> +#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
>
>

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
  2023-03-20 18:28 [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops Álvaro Fernández Rojas
  2023-03-20 19:06 ` Florian Fainelli
  2023-03-21  9:32 ` Álvaro Fernández Rojas
@ 2023-03-21 10:35 ` Jonas Gorski
  2023-03-21 18:04   ` Álvaro Fernández Rojas
  2 siblings, 1 reply; 6+ messages in thread
From: Jonas Gorski @ 2023-03-21 10:35 UTC (permalink / raw)
  To: Álvaro Fernández Rojas
  Cc: f.fainelli, andrew, olteanv, davem, edumazet, kuba, pabeni,
	netdev, linux-kernel

On Mon, 20 Mar 2023 at 19:28, Álvaro Fernández Rojas <noltari@gmail.com> 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.
>
> 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)
> +
> +#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).

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops
  2023-03-21 10:35 ` Jonas Gorski
@ 2023-03-21 18:04   ` Álvaro Fernández Rojas
  0 siblings, 0 replies; 6+ messages in thread
From: Álvaro Fernández Rojas @ 2023-03-21 18:04 UTC (permalink / raw)
  To: Jonas Gorski
  Cc: f.fainelli, andrew, olteanv, davem, edumazet, kuba, pabeni,
	netdev, linux-kernel

Hi Jonas,

El mar, 21 mar 2023 a las 11:36, Jonas Gorski
(<jonas.gorski@gmail.com>) escribió:
>
> On Mon, 20 Mar 2023 at 19:28, Álvaro Fernández Rojas <noltari@gmail.com> 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.
> >
> > 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)
> > +
> > +#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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-03-21 18:04 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-03-20 18:28 [RFC PATCH] drivers: net: dsa: b53: mmap: add phy ops Álvaro Fernández Rojas
2023-03-20 19:06 ` Florian Fainelli
2023-03-20 19:58   ` Álvaro Fernández Rojas
2023-03-21  9:32 ` Álvaro Fernández Rojas
2023-03-21 10:35 ` Jonas Gorski
2023-03-21 18:04   ` Álvaro Fernández Rojas

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).