[PATCH net-next v2 0/5] net: dsa: misc improvements

[PATCH net-next v2 0/5] net: dsa: misc improvements

[Florian Fainelli]

Hi all,

This patch series builds on top of Andrew's "New DSA bind, switches as devices"
patch set and does the following:

- add a few helper functions/goodies for net/dsa/dsa2.c to be as close as possible
  from net/dsa/dsa.c in terms of what drivers can expect, in particular the slave
  MDIO bus and the enabled_port_mask and phy_mii_mask

- fix the CPU port ethtools ops to work in a multiple tree setup since we can
  no longer assume a single tree is supported

- make the bcm_sf2 driver register its own MDIO bus, yet assign it to
  ds->slave_mii_bus for everything to work in net/dsa/slave.c wrt. PHY probing,
  this is a tad cleaner than what we have now

Most of the previous patches have been dropped to just keep the relevant ones
now.

Florian Fainelli (5):
  net: dsa: Provide unique DSA slave MII bus names
  net: dsa: Initialize ds->enabled_port_mask and ds->phys_mii_mask
  net: dsa: Add initialization helper for CPU port ethtool_ops
  net: dsa: Initialize CPU port ethtool ops per tree
  net: dsa: bcm_sf2: Register our slave MDIO bus

 drivers/net/dsa/bcm_sf2.c | 215 +++++++++++++++++++++++++++++-----------------
 drivers/net/dsa/bcm_sf2.h |   6 ++
 net/dsa/dsa.c             |  28 ++++++
 net/dsa/dsa2.c            |  31 +++++++
 net/dsa/dsa_priv.h        |   3 +
 net/dsa/slave.c           |  25 ++----
 6 files changed, 211 insertions(+), 97 deletions(-)

-- 
2.7.4

[PATCH net-next v2 1/5] net: dsa: Provide unique DSA slave MII bus names

[Florian Fainelli]

In case we have multiples trees and switches with the same index, we
need to add another discriminating id: the switch tree.

Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/slave.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 15a492261895..a51dfedf0014 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -49,7 +49,8 @@ void dsa_slave_mii_bus_init(struct dsa_switch *ds)
 	ds->slave_mii_bus->name = "dsa slave smi";
 	ds->slave_mii_bus->read = dsa_slave_phy_read;
 	ds->slave_mii_bus->write = dsa_slave_phy_write;
-	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "dsa-%d", ds->index);
+	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "dsa-%d.%d",
+		 ds->dst->tree, ds->index);
 	ds->slave_mii_bus->parent = ds->dev;
 	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
 }
-- 
2.7.4

[PATCH net-next v2 2/5] net: dsa: Initialize ds->enabled_port_mask and ds->phys_mii_mask

[Florian Fainelli]

Some drivers rely on these two bitmasks to contain the correct values
for them to successfully probe and initialize at drv->setup() time,
calculate correct values to put in both masks as early as possible in
dsa_get_ports_dn().

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa2.c | 27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 80dfe08db825..5ae45210a936 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -283,6 +283,7 @@ static void dsa_user_port_unapply(struct device_node *port, u32 index,
 	if (ds->ports[index].netdev) {
 		dsa_slave_destroy(ds->ports[index].netdev);
 		ds->ports[index].netdev = NULL;
+		ds->enabled_port_mask &= ~(1 << index);
 	}
 }
 
@@ -292,6 +293,13 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 	u32 index;
 	int err;
 
+	/* Initialize ds->phys_mii_mask before registering the slave MDIO bus
+	 * driver and before drv->setup() has run, since the switch drivers and
+	 * the slave MDIO bus driver rely on these values for probing PHY
+	 * devices or not
+	 */
+	ds->phys_mii_mask = ds->enabled_port_mask;
+
 	err = ds->drv->setup(ds);
 	if (err < 0)
 		return err;
@@ -304,6 +312,18 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 	if (err < 0)
 		return err;
 
+	if (!ds->slave_mii_bus && ds->drv->phy_read) {
+		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
+		if (!ds->slave_mii_bus)
+			return err;
+
+		dsa_slave_mii_bus_init(ds);
+
+		err = mdiobus_register(ds->slave_mii_bus);
+		if (err < 0)
+			return err;
+	}
+
 	for (index = 0; index < DSA_MAX_PORTS; index++) {
 		port = ds->ports[index].dn;
 		if (!port)
@@ -511,6 +531,13 @@ static int dsa_parse_ports_dn(struct device_node *ports, struct dsa_switch *ds)
 			return -EINVAL;
 
 		ds->ports[reg].dn = port;
+
+		/* Initialize enabled_port_mask now for drv->setup()
+		 * to have access to a correct value, just like what
+		 * net/dsa/dsa.c::dsa_switch_setup_one does.
+		 */
+		if (!dsa_port_is_cpu(port))
+			ds->enabled_port_mask |= 1 << reg;
 	}
 
 	return 0;
-- 
2.7.4

Re: [PATCH net-next v2 2/5] net: dsa: Initialize ds->enabled_port_mask and ds->phys_mii_mask

[Andrew Lunn]

> @@ -304,6 +312,18 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
>  	if (err < 0)
>  		return err;
>  
> +	if (!ds->slave_mii_bus && ds->drv->phy_read) {
> +		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> +		if (!ds->slave_mii_bus)
> +			return err;
> +
> +		dsa_slave_mii_bus_init(ds);
> +
> +		err = mdiobus_register(ds->slave_mii_bus);
> +		if (err < 0)
> +			return err;
> +	}
> +
>  	for (index = 0; index < DSA_MAX_PORTS; index++) {
>  		port = ds->ports[index].dn;
>  		if (!port)

Hi Florian

This hunk does not seem to fit in this patch.

It is also missing the unregister in dsa_ds_unapply().

   Andrew

Re: [PATCH net-next v2 2/5] net: dsa: Initialize ds->enabled_port_mask and ds->phys_mii_mask

[Andrew Lunn]

On Tue, Jun 07, 2016 at 02:22:12AM +0200, Andrew Lunn wrote:
> > @@ -304,6 +312,18 @@ static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
> >  	if (err < 0)
> >  		return err;
> >  
> > +	if (!ds->slave_mii_bus && ds->drv->phy_read) {
> > +		ds->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
> > +		if (!ds->slave_mii_bus)
> > +			return err;
> > +
> > +		dsa_slave_mii_bus_init(ds);
> > +
> > +		err = mdiobus_register(ds->slave_mii_bus);
> > +		if (err < 0)
> > +			return err;
> > +	}
> > +
> >  	for (index = 0; index < DSA_MAX_PORTS; index++) {
> >  		port = ds->ports[index].dn;
> >  		if (!port)
> 
> Hi Florian

...
 
> It is also missing the unregister in dsa_ds_unapply().

Looking at this again...

You use devm_mdiobus_alloc(), so the memory for the bus will get freed
eventually. However, it won't get unregistered before it is freed.  So
an explicit unregister is needed. Doing that in dsa_ds_unapply() makes
sense, so we have a clear life cycle for this mdio bus.

       Andrew

[PATCH net-next v2 3/5] net: dsa: Add initialization helper for CPU port ethtool_ops

[Florian Fainelli]

Add a helper function: dsa_cpu_port_ethtool_init() which initializes a
custom ethtool_ops structure with custom DSA ethtool operations for CPU
ports. This is a preliminary change to move the initialization outside
of net/dsa/slave.c.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa_priv.h |  1 +
 net/dsa/slave.c    | 14 ++++++++------
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index b42f1a5f95f3..106a9f067f94 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -58,6 +58,7 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
+void dsa_cpu_port_ethtool_init(struct ethtool_ops *ops);
 int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		     int port, const char *name);
 void dsa_slave_destroy(struct net_device *slave_dev);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index a51dfedf0014..8d159932e082 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -865,6 +865,13 @@ static void dsa_slave_poll_controller(struct net_device *dev)
 }
 #endif
 
+void dsa_cpu_port_ethtool_init(struct ethtool_ops *ops)
+{
+	ops->get_sset_count = dsa_cpu_port_get_sset_count;
+	ops->get_ethtool_stats = dsa_cpu_port_get_ethtool_stats;
+	ops->get_strings = dsa_cpu_port_get_strings;
+}
+
 static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_settings		= dsa_slave_get_settings,
 	.set_settings		= dsa_slave_set_settings,
@@ -1124,12 +1131,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		       sizeof(struct ethtool_ops));
 		memcpy(&dsa_cpu_port_ethtool_ops, &dst->master_ethtool_ops,
 		       sizeof(struct ethtool_ops));
-		dsa_cpu_port_ethtool_ops.get_sset_count =
-					dsa_cpu_port_get_sset_count;
-		dsa_cpu_port_ethtool_ops.get_ethtool_stats =
-					dsa_cpu_port_get_ethtool_stats;
-		dsa_cpu_port_ethtool_ops.get_strings =
-					dsa_cpu_port_get_strings;
+		dsa_cpu_port_ethtool_init(&dsa_cpu_port_ethtool_ops);
 		master->ethtool_ops = &dsa_cpu_port_ethtool_ops;
 	}
 	eth_hw_addr_inherit(slave_dev, master);
-- 
2.7.4

Re: [PATCH net-next v2 3/5] net: dsa: Add initialization helper for CPU port ethtool_ops

[Vivien Didelot]

Florian Fainelli <f.fainelli@gmail.com> writes:

> Add a helper function: dsa_cpu_port_ethtool_init() which initializes a
> custom ethtool_ops structure with custom DSA ethtool operations for CPU
> ports. This is a preliminary change to move the initialization outside
> of net/dsa/slave.c.
>
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

[PATCH net-next v2 4/5] net: dsa: Initialize CPU port ethtool ops per tree

[Florian Fainelli]

Now that we can properly support multiple distinct trees in the system,
using a global variable: dsa_cpu_port_ethtool_ops is getting clobbered
as soon as the second switch tree gets probed, and we don't want that.

We need to move this to be dynamically allocated, and since we can't
really be comparing addresses anymore to determine first time
initialization versus any other times, just move this to dsa.c and
dsa2.c where the remainder of the dst/ds initialization happens.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 net/dsa/dsa.c      | 28 ++++++++++++++++++++++++++++
 net/dsa/dsa2.c     |  4 ++++
 net/dsa/dsa_priv.h |  2 ++
 net/dsa/slave.c    | 10 ----------
 4 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index ce3b942dce76..37026f04ee4d 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -266,6 +266,30 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol)
 	return ops;
 }
 
+int dsa_cpu_port_ethtool_setup(struct dsa_switch_tree *dst,
+			       struct dsa_switch *ds)
+{
+	struct net_device *master;
+	struct ethtool_ops *cpu_ops;
+
+	master = ds->dst->master_netdev;
+	if (ds->master_netdev)
+		master = ds->master_netdev;
+
+	cpu_ops = devm_kzalloc(ds->dev, sizeof(*cpu_ops), GFP_KERNEL);
+	if (!cpu_ops)
+		return -ENOMEM;
+
+	memcpy(&dst->master_ethtool_ops, master->ethtool_ops,
+	       sizeof(struct ethtool_ops));
+	memcpy(cpu_ops, &dst->master_ethtool_ops,
+	       sizeof(struct ethtool_ops));
+	dsa_cpu_port_ethtool_init(cpu_ops);
+	master->ethtool_ops = cpu_ops;
+
+	return 0;
+}
+
 static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 {
 	struct dsa_switch_driver *drv = ds->drv;
@@ -379,6 +403,10 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 		ret = 0;
 	}
 
+	ret = dsa_cpu_port_ethtool_setup(dst, ds);
+	if (ret)
+		return ret;
+
 #ifdef CONFIG_NET_DSA_HWMON
 	/* If the switch provides a temperature sensor,
 	 * register with hardware monitoring subsystem.
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index 5ae45210a936..6e912745e43d 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -391,6 +391,10 @@ static int dsa_dst_apply(struct dsa_switch_tree *dst)
 			return err;
 	}
 
+	err = dsa_cpu_port_ethtool_setup(dst, dst->ds[0]);
+	if (err)
+		return err;
+
 	/* If we use a tagging format that doesn't have an ethertype
 	 * field, make sure that all packets from this point on get
 	 * sent to the tag format's receive function.
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 106a9f067f94..3bb88b2fb580 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -54,6 +54,8 @@ int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
 		      struct device_node *port_dn, int port);
 void dsa_cpu_dsa_destroy(struct device_node *port_dn);
 const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
+int dsa_cpu_port_ethtool_setup(struct dsa_switch_tree *dst,
+			       struct dsa_switch *ds);
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 8d159932e082..7236eb26dc97 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -892,8 +892,6 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_eee		= dsa_slave_get_eee,
 };
 
-static struct ethtool_ops dsa_cpu_port_ethtool_ops;
-
 static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_open	 	= dsa_slave_open,
 	.ndo_stop		= dsa_slave_close,
@@ -1126,14 +1124,6 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 
 	slave_dev->features = master->vlan_features;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
-	if (master->ethtool_ops != &dsa_cpu_port_ethtool_ops) {
-		memcpy(&dst->master_ethtool_ops, master->ethtool_ops,
-		       sizeof(struct ethtool_ops));
-		memcpy(&dsa_cpu_port_ethtool_ops, &dst->master_ethtool_ops,
-		       sizeof(struct ethtool_ops));
-		dsa_cpu_port_ethtool_init(&dsa_cpu_port_ethtool_ops);
-		master->ethtool_ops = &dsa_cpu_port_ethtool_ops;
-	}
 	eth_hw_addr_inherit(slave_dev, master);
 	slave_dev->priv_flags |= IFF_NO_QUEUE;
 	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
-- 
2.7.4

Re: [PATCH net-next v2 4/5] net: dsa: Initialize CPU port ethtool ops per tree

[Andrew Lunn]

On Mon, Jun 06, 2016 at 04:14:54PM -0700, Florian Fainelli wrote:
> Now that we can properly support multiple distinct trees in the system,
> using a global variable: dsa_cpu_port_ethtool_ops is getting clobbered
> as soon as the second switch tree gets probed, and we don't want that.
> 
> We need to move this to be dynamically allocated, and since we can't
> really be comparing addresses anymore to determine first time
> initialization versus any other times, just move this to dsa.c and
> dsa2.c where the remainder of the dst/ds initialization happens.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
> ---
>  net/dsa/dsa.c      | 28 ++++++++++++++++++++++++++++
>  net/dsa/dsa2.c     |  4 ++++
>  net/dsa/dsa_priv.h |  2 ++
>  net/dsa/slave.c    | 10 ----------
>  4 files changed, 34 insertions(+), 10 deletions(-)
> 
> diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
> index ce3b942dce76..37026f04ee4d 100644
> --- a/net/dsa/dsa.c
> +++ b/net/dsa/dsa.c
> @@ -266,6 +266,30 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol)
>  	return ops;
>  }
>  
> +int dsa_cpu_port_ethtool_setup(struct dsa_switch_tree *dst,
> +			       struct dsa_switch *ds)
> +{
> +	struct net_device *master;
> +	struct ethtool_ops *cpu_ops;
> +
> +	master = ds->dst->master_netdev;
> +	if (ds->master_netdev)
> +		master = ds->master_netdev;
> +
> +	cpu_ops = devm_kzalloc(ds->dev, sizeof(*cpu_ops), GFP_KERNEL);
> +	if (!cpu_ops)
> +		return -ENOMEM;
> +
> +	memcpy(&dst->master_ethtool_ops, master->ethtool_ops,
> +	       sizeof(struct ethtool_ops));
> +	memcpy(cpu_ops, &dst->master_ethtool_ops,
> +	       sizeof(struct ethtool_ops));
> +	dsa_cpu_port_ethtool_init(cpu_ops);
> +	master->ethtool_ops = cpu_ops;
> +
> +	return 0;
> +}

Hi Florian

Why is there not a symmetrical dsa_cpu_port_ethertool_destroy method,
which will restore master->ethtool_ops when the switch module is
unloaded. I think at the moment, you end up with master->ethtool_ops
pointing at released memory.

	 Andrew

[PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Florian Fainelli]

Register a slave MDIO bus which allows us to divert problematic
read/writes towards conflicting pseudo-PHY address (30). Do no longer
rely on DSA's slave_mii_bus, but instead provide our own implementation
which offers more flexibility as to what to do, and when to register it.

We need to register it by the time we are able to get access to our
memory mapped registers, which is not until drv->setup() time. In order
to avoid forward declarations, we need to re-order the function bodies a
bit.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/bcm_sf2.c | 215 +++++++++++++++++++++++++++++-----------------
 drivers/net/dsa/bcm_sf2.h |   6 ++
 2 files changed, 140 insertions(+), 81 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 73df91bb0466..8026fc21c4fb 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -22,6 +22,7 @@
 #include <linux/of_irq.h>
 #include <linux/of_address.h>
 #include <linux/of_net.h>
+#include <linux/of_mdio.h>
 #include <net/dsa.h>
 #include <linux/ethtool.h>
 #include <linux/if_bridge.h>
@@ -836,6 +837,66 @@ static int bcm_sf2_sw_fdb_dump(struct dsa_switch *ds, int port,
 	return 0;
 }
 
+static int bcm_sf2_sw_indir_rw(struct bcm_sf2_priv *priv, int op, int addr,
+			       int regnum, u16 val)
+{
+	int ret = 0;
+	u32 reg;
+
+	reg = reg_readl(priv, REG_SWITCH_CNTRL);
+	reg |= MDIO_MASTER_SEL;
+	reg_writel(priv, reg, REG_SWITCH_CNTRL);
+
+	/* Page << 8 | offset */
+	reg = 0x70;
+	reg <<= 2;
+	core_writel(priv, addr, reg);
+
+	/* Page << 8 | offset */
+	reg = 0x80 << 8 | regnum << 1;
+	reg <<= 2;
+
+	if (op)
+		ret = core_readl(priv, reg);
+	else
+		core_writel(priv, val, reg);
+
+	reg = reg_readl(priv, REG_SWITCH_CNTRL);
+	reg &= ~MDIO_MASTER_SEL;
+	reg_writel(priv, reg, REG_SWITCH_CNTRL);
+
+	return ret & 0xffff;
+}
+
+static int bcm_sf2_sw_mdio_read(struct mii_bus *bus, int addr, int regnum)
+{
+	struct bcm_sf2_priv *priv = bus->priv;
+
+	/* Intercept reads from Broadcom pseudo-PHY address, else, send
+	 * them to our master MDIO bus controller
+	 */
+	if (addr == BRCM_PSEUDO_PHY_ADDR && priv->indir_phy_mask & BIT(addr))
+		return bcm_sf2_sw_indir_rw(priv, 1, addr, regnum, 0);
+	else
+		return mdiobus_read(priv->master_mii_bus, addr, regnum);
+}
+
+static int bcm_sf2_sw_mdio_write(struct mii_bus *bus, int addr, int regnum,
+				 u16 val)
+{
+	struct bcm_sf2_priv *priv = bus->priv;
+
+	/* Intercept writes to the Broadcom pseudo-PHY address, else,
+	 * send them to our master MDIO bus controller
+	 */
+	if (addr == BRCM_PSEUDO_PHY_ADDR && priv->indir_phy_mask & BIT(addr))
+		bcm_sf2_sw_indir_rw(priv, 0, addr, regnum, val);
+	else
+		mdiobus_write(priv->master_mii_bus, addr, regnum, val);
+
+	return 0;
+}
+
 static irqreturn_t bcm_sf2_switch_0_isr(int irq, void *dev_id)
 {
 	struct bcm_sf2_priv *priv = dev_id;
@@ -932,6 +993,72 @@ static void bcm_sf2_identify_ports(struct bcm_sf2_priv *priv,
 	}
 }
 
+static int bcm_sf2_mdio_register(struct dsa_switch *ds)
+{
+	struct bcm_sf2_priv *priv = ds_to_priv(ds);
+	struct device_node *dn;
+	static int index;
+	int err;
+
+	/* Find our integratd MDIO bus node */
+	dn = of_find_compatible_node(NULL, NULL, "brcm,unimac-mdio");
+	priv->master_mii_bus = of_mdio_find_bus(dn);
+	if (!priv->master_mii_bus)
+		return -EPROBE_DEFER;
+
+	get_device(&priv->master_mii_bus->dev);
+	priv->master_mii_dn = dn;
+
+	priv->slave_mii_bus = devm_mdiobus_alloc(ds->dev);
+	if (!priv->slave_mii_bus)
+		return -ENOMEM;
+
+	priv->slave_mii_bus->priv = priv;
+	priv->slave_mii_bus->name = "sf2 slave mii";
+	priv->slave_mii_bus->read = bcm_sf2_sw_mdio_read;
+	priv->slave_mii_bus->write = bcm_sf2_sw_mdio_write;
+	snprintf(priv->slave_mii_bus->id, MII_BUS_ID_SIZE, "sf2-%d",
+		 index++);
+	priv->slave_mii_bus->dev.of_node = dn;
+
+	/* Include the pseudo-PHY address to divert reads towards our
+	 * workaround. This is only required for 7445D0, since 7445E0
+	 * disconnects the internal switch pseudo-PHY such that we can use the
+	 * regular SWITCH_MDIO master controller instead.
+	 *
+	 * Here we flag the pseudo PHY as needing special treatment and would
+	 * otherwise make all other PHY read/writes go to the master MDIO bus
+	 * controller that comes with this switch backed by the "mdio-unimac"
+	 * driver.
+	 */
+	if (of_machine_is_compatible("brcm,bcm7445d0"))
+		priv->indir_phy_mask |= (1 << BRCM_PSEUDO_PHY_ADDR);
+	else
+		priv->indir_phy_mask = 0;
+
+	ds->phys_mii_mask = priv->indir_phy_mask;
+	ds->slave_mii_bus = priv->slave_mii_bus;
+	priv->slave_mii_bus->parent = ds->dev->parent;
+	priv->slave_mii_bus->phy_mask = ~priv->indir_phy_mask;
+
+	if (dn)
+		err = of_mdiobus_register(priv->slave_mii_bus, dn);
+	else
+		err = mdiobus_register(priv->slave_mii_bus);
+
+	if (err)
+		of_node_put(dn);
+
+	return err;
+}
+
+static void bcm_sf2_mdio_unregister(struct bcm_sf2_priv *priv)
+{
+	mdiobus_unregister(priv->slave_mii_bus);
+	if (priv->master_mii_dn)
+		of_node_put(priv->master_mii_dn);
+}
+
 static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 {
 	const char *reg_names[BCM_SF2_REGS_NUM] = BCM_SF2_REGS_NAME;
@@ -972,6 +1099,12 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 		goto out_unmap;
 	}
 
+	ret = bcm_sf2_mdio_register(ds);
+	if (ret) {
+		pr_err("failed to register MDIO bus\n");
+		goto out_unmap;
+	}
+
 	/* Disable all interrupts and request them */
 	bcm_sf2_intr_disable(priv);
 
@@ -1017,23 +1150,6 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
 			bcm_sf2_port_disable(ds, port, NULL);
 	}
 
-	/* Include the pseudo-PHY address and the broadcast PHY address to
-	 * divert reads towards our workaround. This is only required for
-	 * 7445D0, since 7445E0 disconnects the internal switch pseudo-PHY such
-	 * that we can use the regular SWITCH_MDIO master controller instead.
-	 *
-	 * By default, DSA initializes ds->phys_mii_mask to
-	 * ds->enabled_port_mask to have a 1:1 mapping between Port address
-	 * and PHY address in order to utilize the slave_mii_bus instance to
-	 * read from Port PHYs. This is not what we want here, so we
-	 * initialize phys_mii_mask 0 to always utilize the "master" MDIO
-	 * bus backed by the "mdio-unimac" driver.
-	 */
-	if (of_machine_is_compatible("brcm,bcm7445d0"))
-		ds->phys_mii_mask |= ((1 << BRCM_PSEUDO_PHY_ADDR) | (1 << 0));
-	else
-		ds->phys_mii_mask = 0;
-
 	rev = reg_readl(priv, REG_SWITCH_REVISION);
 	priv->hw_params.top_rev = (rev >> SWITCH_TOP_REV_SHIFT) &
 					SWITCH_TOP_REV_MASK;
@@ -1058,6 +1174,7 @@ out_unmap:
 			iounmap(*base);
 		base++;
 	}
+	bcm_sf2_mdio_unregister(priv);
 	return ret;
 }
 
@@ -1078,68 +1195,6 @@ static u32 bcm_sf2_sw_get_phy_flags(struct dsa_switch *ds, int port)
 	return priv->hw_params.gphy_rev;
 }
 
-static int bcm_sf2_sw_indir_rw(struct dsa_switch *ds, int op, int addr,
-			       int regnum, u16 val)
-{
-	struct bcm_sf2_priv *priv = ds_to_priv(ds);
-	int ret = 0;
-	u32 reg;
-
-	reg = reg_readl(priv, REG_SWITCH_CNTRL);
-	reg |= MDIO_MASTER_SEL;
-	reg_writel(priv, reg, REG_SWITCH_CNTRL);
-
-	/* Page << 8 | offset */
-	reg = 0x70;
-	reg <<= 2;
-	core_writel(priv, addr, reg);
-
-	/* Page << 8 | offset */
-	reg = 0x80 << 8 | regnum << 1;
-	reg <<= 2;
-
-	if (op)
-		ret = core_readl(priv, reg);
-	else
-		core_writel(priv, val, reg);
-
-	reg = reg_readl(priv, REG_SWITCH_CNTRL);
-	reg &= ~MDIO_MASTER_SEL;
-	reg_writel(priv, reg, REG_SWITCH_CNTRL);
-
-	return ret & 0xffff;
-}
-
-static int bcm_sf2_sw_phy_read(struct dsa_switch *ds, int addr, int regnum)
-{
-	/* Intercept reads from the MDIO broadcast address or Broadcom
-	 * pseudo-PHY address
-	 */
-	switch (addr) {
-	case 0:
-	case BRCM_PSEUDO_PHY_ADDR:
-		return bcm_sf2_sw_indir_rw(ds, 1, addr, regnum, 0);
-	default:
-		return 0xffff;
-	}
-}
-
-static int bcm_sf2_sw_phy_write(struct dsa_switch *ds, int addr, int regnum,
-				u16 val)
-{
-	/* Intercept writes to the MDIO broadcast address or Broadcom
-	 * pseudo-PHY address
-	 */
-	switch (addr) {
-	case 0:
-	case BRCM_PSEUDO_PHY_ADDR:
-		bcm_sf2_sw_indir_rw(ds, 0, addr, regnum, val);
-		break;
-	}
-
-	return 0;
-}
-
 static void bcm_sf2_sw_adjust_link(struct dsa_switch *ds, int port,
 				   struct phy_device *phydev)
 {
@@ -1376,8 +1431,6 @@ static struct dsa_switch_driver bcm_sf2_switch_driver = {
 	.setup			= bcm_sf2_sw_setup,
 	.set_addr		= bcm_sf2_sw_set_addr,
 	.get_phy_flags		= bcm_sf2_sw_get_phy_flags,
-	.phy_read		= bcm_sf2_sw_phy_read,
-	.phy_write		= bcm_sf2_sw_phy_write,
 	.get_strings		= bcm_sf2_sw_get_strings,
 	.get_ethtool_stats	= bcm_sf2_sw_get_ethtool_stats,
 	.get_sset_count		= bcm_sf2_sw_get_sset_count,
diff --git a/drivers/net/dsa/bcm_sf2.h b/drivers/net/dsa/bcm_sf2.h
index 200b1f5fdb56..bde11ebb2742 100644
--- a/drivers/net/dsa/bcm_sf2.h
+++ b/drivers/net/dsa/bcm_sf2.h
@@ -142,6 +142,12 @@ struct bcm_sf2_priv {
 
 	/* Bitmask of ports having an integrated PHY */
 	unsigned int			int_phy_mask;
+
+	/* Master and slave MDIO bus controller */
+	unsigned int			indir_phy_mask;
+	struct device_node		*master_mii_dn;
+	struct mii_bus			*slave_mii_bus;
+	struct mii_bus			*master_mii_bus;
 };
 
 struct bcm_sf2_hw_stats {
-- 
2.7.4

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Andrew Lunn]

On Mon, Jun 06, 2016 at 04:14:55PM -0700, Florian Fainelli wrote:
> Register a slave MDIO bus which allows us to divert problematic
> read/writes towards conflicting pseudo-PHY address (30). Do no longer
> rely on DSA's slave_mii_bus, but instead provide our own implementation
> which offers more flexibility as to what to do, and when to register it.
> 
> We need to register it by the time we are able to get access to our
> memory mapped registers, which is not until drv->setup() time. In order
> to avoid forward declarations, we need to re-order the function bodies a
> bit.
> 
> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

	     Andrew

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Vivien Didelot]

Florian Fainelli <f.fainelli@gmail.com> writes:

> +static int bcm_sf2_mdio_register(struct dsa_switch *ds)
> +{
> +	struct bcm_sf2_priv *priv = ds_to_priv(ds);
> +	struct device_node *dn;
> +	static int index;
> +	int err;
> +
> +	/* Find our integratd MDIO bus node */

                    integrated

(only if you need to respin a v3.)

Otherwise,

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Vivien Didelot]

Hi Florian,

Florian Fainelli <f.fainelli@gmail.com> writes:

>  static int bcm_sf2_sw_setup(struct dsa_switch *ds)
>  {
>  	const char *reg_names[BCM_SF2_REGS_NUM] = BCM_SF2_REGS_NAME;
> @@ -972,6 +1099,12 @@ static int bcm_sf2_sw_setup(struct dsa_switch *ds)
>  		goto out_unmap;
>  	}
>  
> +	ret = bcm_sf2_mdio_register(ds);
> +	if (ret) {
> +		pr_err("failed to register MDIO bus\n");
> +		goto out_unmap;
> +	}
> +
>  	/* Disable all interrupts and request them */
>  	bcm_sf2_intr_disable(priv);

Hum reviewing that again, I see that if one of the 2 subsequent calls to
request_irq fails, you end up with an unregistered MDIO bus.

We have the same issue in the mv88e6xxx legacy probe code if dsa.c fails
to allocate the dsa_switch structure. I'm moving the MDIO register code
to the setup function like you are doing here (good idea!) to fix that.

Thanks,

        Vivien

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Vivien Didelot]

Hi Florian, Andrew,

Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:

> Hum reviewing that again, I see that if one of the 2 subsequent calls to
> request_irq fails, you end up with an unregistered MDIO bus.
>
> We have the same issue in the mv88e6xxx legacy probe code if dsa.c fails
> to allocate the dsa_switch structure. I'm moving the MDIO register code
> to the setup function like you are doing here (good idea!) to fix that.

In fact it doesn't fix the issue because dsa_switch_driver doesn't
provide any remove/teardown function, in which mv88e6xxx and sf2 could
unregister their MDIO bus on switch removal.

Would it be worth it to add such optional function to DSA drivers?

Thanks,

        Vivien

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Andrew Lunn]

On Tue, Jun 07, 2016 at 12:48:37PM -0400, Vivien Didelot wrote:
> Hi Florian, Andrew,
> 
> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
> 
> > Hum reviewing that again, I see that if one of the 2 subsequent calls to
> > request_irq fails, you end up with an unregistered MDIO bus.
> >
> > We have the same issue in the mv88e6xxx legacy probe code if dsa.c fails
> > to allocate the dsa_switch structure. I'm moving the MDIO register code
> > to the setup function like you are doing here (good idea!) to fix that.
> 
> In fact it doesn't fix the issue because dsa_switch_driver doesn't
> provide any remove/teardown function, in which mv88e6xxx and sf2 could
> unregister their MDIO bus on switch removal.
> 
> Would it be worth it to add such optional function to DSA drivers?

It is not needed with DSA2 binding. The driver is always in control,
and it performs the unregister from the core. So it knows when to
unregister the mdio bus, either because probe has failed for some
reason, or it is being unloaded. That is also why i register the mdio
bus in probe, and unregister it in remove. Normal practice for a
driver.

With the legacy interface it is tricky. When would you call such a
remove/tairdown function when using the old binding?

	Andrew

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Vivien Didelot]

Andrew Lunn <andrew@lunn.ch> writes:

> On Tue, Jun 07, 2016 at 12:48:37PM -0400, Vivien Didelot wrote:
>> Hi Florian, Andrew,
>> 
>> Vivien Didelot <vivien.didelot@savoirfairelinux.com> writes:
>> 
>> > Hum reviewing that again, I see that if one of the 2 subsequent calls to
>> > request_irq fails, you end up with an unregistered MDIO bus.
>> >
>> > We have the same issue in the mv88e6xxx legacy probe code if dsa.c fails
>> > to allocate the dsa_switch structure. I'm moving the MDIO register code
>> > to the setup function like you are doing here (good idea!) to fix that.
>> 
>> In fact it doesn't fix the issue because dsa_switch_driver doesn't
>> provide any remove/teardown function, in which mv88e6xxx and sf2 could
>> unregister their MDIO bus on switch removal.
>> 
>> Would it be worth it to add such optional function to DSA drivers?
>
> It is not needed with DSA2 binding. The driver is always in control,
> and it performs the unregister from the core. So it knows when to
> unregister the mdio bus, either because probe has failed for some
> reason, or it is being unloaded. That is also why i register the mdio
> bus in probe, and unregister it in remove. Normal practice for a
> driver.

Yes I know, I was still talking about the legacy code.

> With the legacy interface it is tricky. When would you call such a
> remove/tairdown function when using the old binding?

That'd go in dsa_switch_destroy I guess, but it just covers the case
where the whole DSA code is unloaded...

        Vivien

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Andrew Lunn]

> > With the legacy interface it is tricky. When would you call such a
> > remove/tairdown function when using the old binding?
> 
> That'd go in dsa_switch_destroy I guess, but it just covers the case
> where the whole DSA code is unloaded...

I don't think that helps you. It should not be possible to unload the
DSA core while there is an active driver. The drivers needs to unload
first....

	Andrew

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>> > With the legacy interface it is tricky. When would you call such a
>> > remove/tairdown function when using the old binding?
>> 
>> That'd go in dsa_switch_destroy I guess, but it just covers the case
>> where the whole DSA code is unloaded...
>
> I don't think that helps you. It should not be possible to unload the
> DSA core while there is an active driver. The drivers needs to unload
> first....

Well, dsa_switch_destroy() is where ds->slave_mii_bus gets unregistered
(if registered by the framework), so it seems fair to do something like:

    if (ds->drv->shutdown)
        ds->drv->shutdown(ds);

But I'm still not sure if it is worth it to add a new legacy specific
function to DSA drivers, unless there is a use case for such optional
teardown callback for the new bindings too.

Thanks,

        Vivien

Re: [PATCH net-next v2 5/5] net: dsa: bcm_sf2: Register our slave MDIO bus

[Florian Fainelli]

On 06/07/2016 12:11 PM, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
>>>> With the legacy interface it is tricky. When would you call such a
>>>> remove/tairdown function when using the old binding?
>>>
>>> That'd go in dsa_switch_destroy I guess, but it just covers the case
>>> where the whole DSA code is unloaded...
>>
>> I don't think that helps you. It should not be possible to unload the
>> DSA core while there is an active driver. The drivers needs to unload
>> first....
> 
> Well, dsa_switch_destroy() is where ds->slave_mii_bus gets unregistered
> (if registered by the framework), so it seems fair to do something like:
> 
>     if (ds->drv->shutdown)
>         ds->drv->shutdown(ds);
> 
> But I'm still not sure if it is worth it to add a new legacy specific
> function to DSA drivers, unless there is a use case for such optional
> teardown callback for the new bindings too.

The new binding requires the use of dsa_unregister_switch() so this is
where all the teardown and resource freeing should occur.

I do not really think it is worth trying to fix the old binding and
support code now, unless we want to migrate it somehow to using the code
from net/dsa/dsa2.c.
-- 
Florian