[RFC PATCH 00/16] New DSA bind, switches as devices

[RFC PATCH 00/16] New DSA bind, switches as devices

[Andrew Lunn]

This is an RFC patchset and should not be accepted yet.

The interesting patches here are the last three. They implement a new
binding for DSA, which removes a few limitations of the current DSA
binding. In particular, it allows switches to be true Linux devices.
These devices can be on any type of bus, unlike the old DSA binding
which assumes MDIO. See the commit log for more details. The second to
last patch modifies an existing boards device tree to use the new
binding, giving a good example of how switches can be true MDIO
devices. The last patch documents the new binding.

I know both John Crispin and Bryan Whitehead are interesting in
implementing DSA drivers, hence i have CC: you. Comments welcome.

Thanks go to Florian and Vivien for reviewing, testing and bug fixing
these patches.

Andrew Lunn (15):
  dsa: slave: chip data is optional, don't dereference NULL
  dsa: slave: Remove MDIO address from switch MDIO bus name
  dsa: tag_{e}dsa.c: Remove dependency on platform data
  dsa: Add a ports structure and use it in the switch structure
  dsa: Move port device node into port structure
  dsa: Remove dynamic allocate of routing table
  dsa: Copy the routing table into the switch structure
  dsa: dsa: Split up creating/destroying of DSA and CPU ports
  net: dsa: mv88e6xxx: Only support EDSA tagging
  net: dsa: Refactor selection of tag ops into a function
  dsa: Make mdio bus optional
  net: dsa: mv88e6xxx: Refactor MDIO so driver registers mdio bus
  net: dsa: Add new binding implementation
  arm: dt: vf610-zii-devel-b: Make use of new DSA binding
  dsa: Document new binding

Vivien Didelot (1):
  net: dsa: mv88e6xxx: fix circular lock in PPU work

 Documentation/devicetree/bindings/net/dsa/dsa.txt | 278 ++++++++-
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts         | 328 +++++------
 drivers/net/dsa/bcm_sf2.c                         |   4 +-
 drivers/net/dsa/mv88e6xxx.c                       | 264 ++++++---
 drivers/net/dsa/mv88e6xxx.h                       |   6 +
 include/net/dsa.h                                 |  57 +-
 net/dsa/Makefile                                  |   2 +-
 net/dsa/dsa.c                                     | 210 ++++---
 net/dsa/dsa2.c                                    | 653 ++++++++++++++++++++++
 net/dsa/dsa_priv.h                                |   6 +-
 net/dsa/slave.c                                   |  57 +-
 net/dsa/tag_brcm.c                                |   4 +-
 net/dsa/tag_dsa.c                                 |  10 +-
 net/dsa/tag_edsa.c                                |  10 +-
 net/dsa/tag_trailer.c                             |   4 +-
 15 files changed, 1485 insertions(+), 408 deletions(-)
 create mode 100644 net/dsa/dsa2.c

-- 
2.8.1

[RFC PATCH 01/16] dsa: slave: chip data is optional, don't dereference NULL

[Andrew Lunn]

The new binding does not make use of dsa_chip_data, a.k.a cd.  When
retrieving the size of the EEPROM attached to a switch, don't assume
there is a cd attached to the switch structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/slave.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 152436cdab30..135a91706755 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -615,7 +615,7 @@ static int dsa_slave_get_eeprom_len(struct net_device *dev)
 	struct dsa_slave_priv *p = netdev_priv(dev);
 	struct dsa_switch *ds = p->parent;
 
-	if (ds->cd->eeprom_len)
+	if (ds->cd && ds->cd->eeprom_len)
 		return ds->cd->eeprom_len;
 
 	if (ds->drv->get_eeprom_len)
-- 
2.8.1

Re: [RFC PATCH 01/16] dsa: slave: chip data is optional, don't dereference NULL

[Vivien Didelot]

Hi Andrew, Florian,

We are inconsistent on commit titles and messages. Some of them use
"net: " prefix, some others don't, sometimes lower or upper case titles.

I'd suggest we stick with the "net: dsa: " prefix and lowercase titles.

When possible, let's try to respect the 50/72 Git rule.

So we'd have e.g.:

    net: dsa: add new bindings
    net: dsa: mv88e6xxx: add support for foo
    net: dsa: sf2: remove bar
    ...

The networking documentation doesn't seem to have opinion on the "net: "
prefix. We might drop it and keep only "dsa: " for core and drivers.

What do you think?

Thanks,

        Vivien

Re: [RFC PATCH 01/16] dsa: slave: chip data is optional, don't dereference NULL

[Florian Fainelli]

On 05/27/2016 11:45 AM, Vivien Didelot wrote:
> Hi Andrew, Florian,
> 
> We are inconsistent on commit titles and messages. Some of them use
> "net: " prefix, some others don't, sometimes lower or upper case titles.
> 
> I'd suggest we stick with the "net: dsa: " prefix and lowercase titles.
> 
> When possible, let's try to respect the 50/72 Git rule.
> 
> So we'd have e.g.:
> 
>     net: dsa: add new bindings
>     net: dsa: mv88e6xxx: add support for foo
>     net: dsa: sf2: remove bar
>     ...
> 
> The networking documentation doesn't seem to have opinion on the "net: "
> prefix. We might drop it and keep only "dsa: " for core and drivers.
> 
> What do you think?

My preference goes for "net: dsa: <blah>", but at the same time, I don't
really care as long as I can git log the file.
-- 
Florian

[RFC PATCH 02/16] net: dsa: mv88e6xxx: fix circular lock in PPU work

[Andrew Lunn]

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

Lock debugging shows that there is a possible circular lock in the PPU
work code. Switch the lock order of smi_mutex and ppu_mutex to fix this.

Here's the full trace:

    [    4.341325] ======================================================
    [    4.347519] [ INFO: possible circular locking dependency detected ]
    [    4.353800] 4.6.0 #4 Not tainted
    [    4.357039] -------------------------------------------------------
    [    4.363315] kworker/0:1/328 is trying to acquire lock:
    [    4.368463]  (&ps->smi_mutex){+.+.+.}, at: [<8049c758>] mv88e6xxx_reg_read+0x30/0x54
    [    4.376313]
    [    4.376313] but task is already holding lock:
    [    4.382160]  (&ps->ppu_mutex){+.+...}, at: [<8049cac0>] mv88e6xxx_ppu_reenable_work+0x28/0xd4
    [    4.390772]
    [    4.390772] which lock already depends on the new lock.
    [    4.390772]
    [    4.398963]
    [    4.398963] the existing dependency chain (in reverse order) is:
    [    4.406461]
    [    4.406461] -> #1 (&ps->ppu_mutex){+.+...}:
    [    4.410897]        [<806d86bc>] mutex_lock_nested+0x54/0x360
    [    4.416606]        [<8049a800>] mv88e6xxx_ppu_access_get+0x28/0x100
    [    4.422906]        [<8049b778>] mv88e6xxx_phy_read+0x90/0xdc
    [    4.428599]        [<806a4534>] dsa_slave_phy_read+0x3c/0x40
    [    4.434300]        [<804943ec>] mdiobus_read+0x68/0x80
    [    4.439481]        [<804939d4>] get_phy_device+0x58/0x1d8
    [    4.444914]        [<80493ed0>] mdiobus_scan+0x24/0xf4
    [    4.450078]        [<8049409c>] __mdiobus_register+0xfc/0x1ac
    [    4.455857]        [<806a40b0>] dsa_probe+0x860/0xca8
    [    4.460934]        [<8043246c>] platform_drv_probe+0x5c/0xc0
    [    4.466627]        [<804305a0>] driver_probe_device+0x118/0x450
    [    4.472589]        [<80430b00>] __device_attach_driver+0xac/0x128
    [    4.478724]        [<8042e350>] bus_for_each_drv+0x74/0xa8
    [    4.484235]        [<804302d8>] __device_attach+0xc4/0x154
    [    4.489755]        [<80430cec>] device_initial_probe+0x1c/0x20
    [    4.495612]        [<8042f620>] bus_probe_device+0x98/0xa0
    [    4.501123]        [<8042fbd0>] deferred_probe_work_func+0x4c/0xd4
    [    4.507328]        [<8013a794>] process_one_work+0x1a8/0x604
    [    4.513030]        [<8013ac54>] worker_thread+0x64/0x528
    [    4.518367]        [<801409e8>] kthread+0xec/0x100
    [    4.523201]        [<80108f30>] ret_from_fork+0x14/0x24
    [    4.528462]
    [    4.528462] -> #0 (&ps->smi_mutex){+.+.+.}:
    [    4.532895]        [<8015ad5c>] lock_acquire+0xb4/0x1dc
    [    4.538154]        [<806d86bc>] mutex_lock_nested+0x54/0x360
    [    4.543856]        [<8049c758>] mv88e6xxx_reg_read+0x30/0x54
    [    4.549549]        [<8049cad8>] mv88e6xxx_ppu_reenable_work+0x40/0xd4
    [    4.556022]        [<8013a794>] process_one_work+0x1a8/0x604
    [    4.561707]        [<8013ac54>] worker_thread+0x64/0x528
    [    4.567053]        [<801409e8>] kthread+0xec/0x100
    [    4.571878]        [<80108f30>] ret_from_fork+0x14/0x24
    [    4.577139]
    [    4.577139] other info that might help us debug this:
    [    4.577139]
    [    4.585159]  Possible unsafe locking scenario:
    [    4.585159]
    [    4.591093]        CPU0                    CPU1
    [    4.595631]        ----                    ----
    [    4.600169]   lock(&ps->ppu_mutex);
    [    4.603693]                                lock(&ps->smi_mutex);
    [    4.609742]                                lock(&ps->ppu_mutex);
    [    4.615790]   lock(&ps->smi_mutex);
    [    4.619314]
    [    4.619314]  *** DEADLOCK ***
    [    4.619314]
    [    4.625256] 3 locks held by kworker/0:1/328:
    [    4.629537]  #0:  ("events"){.+.+..}, at: [<8013a704>] process_one_work+0x118/0x604
    [    4.637288]  #1:  ((&ps->ppu_work)){+.+...}, at: [<8013a704>] process_one_work+0x118/0x604
    [    4.645653]  #2:  (&ps->ppu_mutex){+.+...}, at: [<8049cac0>] mv88e6xxx_ppu_reenable_work+0x28/0xd4
    [    4.654714]
    [    4.654714] stack backtrace:
    [    4.659098] CPU: 0 PID: 328 Comm: kworker/0:1 Not tainted 4.6.0 #4
    [    4.665286] Hardware name: Freescale Vybrid VF5xx/VF6xx (Device Tree)
    [    4.671748] Workqueue: events mv88e6xxx_ppu_reenable_work
    [    4.677174] Backtrace:
    [    4.679674] [<8010d354>] (dump_backtrace) from [<8010d5a0>] (show_stack+0x20/0x24)
    [    4.687252]  r6:80fb3c88 r5:80fb3c88 r4:80fb4728 r3:00000002
    [    4.693003] [<8010d580>] (show_stack) from [<803b45e8>] (dump_stack+0x24/0x28)
    [    4.700246] [<803b45c4>] (dump_stack) from [<80157398>] (print_circular_bug+0x208/0x32c)
    [    4.708361] [<80157190>] (print_circular_bug) from [<8015a630>] (__lock_acquire+0x185c/0x1b80)
    [    4.716982]  r10:9ec22a00 r9:00000060 r8:8164b6bc r7:00000040 r6:00000003 r5:8163a5b4
    [    4.724905]  r4:00000003 r3:9ec22de8
    [    4.728537] [<80158dd4>] (__lock_acquire) from [<8015ad5c>] (lock_acquire+0xb4/0x1dc)
    [    4.736378]  r10:60000013 r9:00000000 r8:00000000 r7:00000000 r6:9e5e9c50 r5:80e618e0
    [    4.744301]  r4:00000000
    [    4.746879] [<8015aca8>] (lock_acquire) from [<806d86bc>] (mutex_lock_nested+0x54/0x360)
    [    4.754976]  r10:9e5e9c1c r9:80e616c4 r8:9f685ea0 r7:0000001b r6:9ec22a00 r5:8163a5b4
    [    4.762899]  r4:9e5e9c1c
    [    4.765477] [<806d8668>] (mutex_lock_nested) from [<8049c758>] (mv88e6xxx_reg_read+0x30/0x54)
    [    4.774008]  r10:80e60c5b r9:80e616c4 r8:9f685ea0 r7:0000001b r6:00000004 r5:9e5e9c10
    [    4.781930]  r4:9e5e9c1c
    [    4.784507] [<8049c728>] (mv88e6xxx_reg_read) from [<8049cad8>] (mv88e6xxx_ppu_reenable_work+0x40/0xd4)
    [    4.793907]  r7:9ffd5400 r6:9e5e9c68 r5:9e5e9cb0 r4:9e5e9c10
    [    4.799659] [<8049ca98>] (mv88e6xxx_ppu_reenable_work) from [<8013a794>] (process_one_work+0x1a8/0x604)
    [    4.809059]  r9:80e616c4 r8:9f685ea0 r7:9ffd5400 r6:80e0a1c8 r5:9f5f2e80 r4:9e5e9cb0
    [    4.816910] [<8013a5ec>] (process_one_work) from [<8013ac54>] (worker_thread+0x64/0x528)
    [    4.825010]  r10:9f5f2e80 r9:00000008 r8:80e0dc80 r7:80e0a1fc r6:80e0a1c8 r5:9f5f2e98
    [    4.832933]  r4:80e0a1c8
    [    4.835510] [<8013abf0>] (worker_thread) from [<801409e8>] (kthread+0xec/0x100)
    [    4.842827]  r10:00000000 r9:00000000 r8:00000000 r7:8013abf0 r6:9f5f2e80 r5:9ec15740
    [    4.850749]  r4:00000000
    [    4.853327] [<801408fc>] (kthread) from [<80108f30>] (ret_from_fork+0x14/0x24)
    [    4.860557]  r7:00000000 r6:00000000 r5:801408fc r4:9ec15740

Signed-off-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>
---
 drivers/net/dsa/mv88e6xxx.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index ba9dfc9421ef..c361036e7f9c 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -288,18 +288,18 @@ static int mv88e6xxx_ppu_enable(struct mv88e6xxx_priv_state *ps)
 	int ret, err;
 	unsigned long timeout;
 
-	ret = mv88e6xxx_reg_read(ps, REG_GLOBAL, GLOBAL_CONTROL);
+	ret = _mv88e6xxx_reg_read(ps, REG_GLOBAL, GLOBAL_CONTROL);
 	if (ret < 0)
 		return ret;
 
-	err = mv88e6xxx_reg_write(ps, REG_GLOBAL, GLOBAL_CONTROL,
-				  ret | GLOBAL_CONTROL_PPU_ENABLE);
+	err = _mv88e6xxx_reg_write(ps, REG_GLOBAL, GLOBAL_CONTROL,
+				   ret | GLOBAL_CONTROL_PPU_ENABLE);
 	if (err)
 		return err;
 
 	timeout = jiffies + 1 * HZ;
 	while (time_before(jiffies, timeout)) {
-		ret = mv88e6xxx_reg_read(ps, REG_GLOBAL, GLOBAL_STATUS);
+		ret = _mv88e6xxx_reg_read(ps, REG_GLOBAL, GLOBAL_STATUS);
 		if (ret < 0)
 			return ret;
 
@@ -317,11 +317,16 @@ static void mv88e6xxx_ppu_reenable_work(struct work_struct *ugly)
 	struct mv88e6xxx_priv_state *ps;
 
 	ps = container_of(ugly, struct mv88e6xxx_priv_state, ppu_work);
+
+	mutex_lock(&ps->smi_mutex);
+
 	if (mutex_trylock(&ps->ppu_mutex)) {
 		if (mv88e6xxx_ppu_enable(ps) == 0)
 			ps->ppu_disabled = 0;
 		mutex_unlock(&ps->ppu_mutex);
 	}
+
+	mutex_unlock(&ps->smi_mutex);
 }
 
 static void mv88e6xxx_ppu_reenable_timer(unsigned long _ps)
-- 
2.8.1

[RFC PATCH 03/16] dsa: slave: Remove MDIO address from switch MDIO bus name

[Andrew Lunn]

The DSA layer should no longer assume the switch is connected to an
MDIO bus. As a result, we cannot use the address on the MDIO bus when
forming the name of the switches internal MDIO bus for its builtin and
possibly external PHYs. The switch index is sufficient to make the
name unique, so drop the MDIO address.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/slave.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 135a91706755..f640a48a6ff3 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -49,8 +49,7 @@ 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:%.2x",
-			ds->index, ds->cd->sw_addr);
+	snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "dsa-%d", ds->index);
 	ds->slave_mii_bus->parent = ds->dev;
 	ds->slave_mii_bus->phy_mask = ~ds->phys_mii_mask;
 }
-- 
2.8.1

[RFC PATCH 04/16] dsa: tag_{e}dsa.c: Remove dependency on platform data

[Andrew Lunn]

The platform data nr_chips is used when validating a received packet,
to ensure it comes from a know switch chip. The number of possible
switches is limited to DSA_MAX_SWITCHES, so use this as the first
validation step. The new binding allows holes in the dst->ds[] array,
so also ensure ensure there is a valid dsa_switch for this packet.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/tag_dsa.c  | 6 +++++-
 net/dsa/tag_edsa.c | 6 +++++-
 2 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index aa780e4ac0bd..f9832f097681 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -107,9 +107,13 @@ static int dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 	 * Check that the source device exists and that the source
 	 * port is a registered DSA port.
 	 */
-	if (source_device >= dst->pd->nr_chips)
+	if (source_device >= DSA_MAX_SWITCHES)
 		goto out_drop;
+
 	ds = dst->ds[source_device];
+	if (!ds)
+		goto out_drop;
+
 	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
 		goto out_drop;
 
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 2288c8098c42..3890aac8190f 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -120,9 +120,13 @@ static int edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 	 * Check that the source device exists and that the source
 	 * port is a registered DSA port.
 	 */
-	if (source_device >= dst->pd->nr_chips)
+	if (source_device >= DSA_MAX_SWITCHES)
 		goto out_drop;
+
 	ds = dst->ds[source_device];
+	if (!ds)
+		goto out_drop;
+
 	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
 		goto out_drop;
 
-- 
2.8.1

[RFC PATCH 05/16] dsa: Add a ports structure and use it in the switch structure

[Andrew Lunn]

There are going to be more per-port members added to the switch
structure. So add a port structure and move the netdev into it.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/bcm_sf2.c   |  4 ++--
 drivers/net/dsa/mv88e6xxx.c | 27 ++++++++++++++++-----------
 include/net/dsa.h           |  8 ++++++--
 net/dsa/dsa.c               |  8 ++++----
 net/dsa/slave.c             |  4 ++--
 net/dsa/tag_brcm.c          |  4 ++--
 net/dsa/tag_dsa.c           |  4 ++--
 net/dsa/tag_edsa.c          |  4 ++--
 net/dsa/tag_trailer.c       |  4 ++--
 9 files changed, 38 insertions(+), 29 deletions(-)

diff --git a/drivers/net/dsa/bcm_sf2.c b/drivers/net/dsa/bcm_sf2.c
index 10ddd5a5dfb6..73df91bb0466 100644
--- a/drivers/net/dsa/bcm_sf2.c
+++ b/drivers/net/dsa/bcm_sf2.c
@@ -804,7 +804,7 @@ static int bcm_sf2_sw_fdb_dump(struct dsa_switch *ds, int port,
 			       int (*cb)(struct switchdev_obj *obj))
 {
 	struct bcm_sf2_priv *priv = ds_to_priv(ds);
-	struct net_device *dev = ds->ports[port];
+	struct net_device *dev = ds->ports[port].netdev;
 	struct bcm_sf2_arl_entry results[2];
 	unsigned int count = 0;
 	int ret;
@@ -1248,7 +1248,7 @@ static void bcm_sf2_sw_fixed_link_update(struct dsa_switch *ds, int port,
 		 * state machine and make it go in PHY_FORCING state instead.
 		 */
 		if (!status->link)
-			netif_carrier_off(ds->ports[port]);
+			netif_carrier_off(ds->ports[port].netdev);
 		status->duplex = 1;
 	} else {
 		status->link = 1;
diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index c361036e7f9c..85332d9a245a 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -1327,7 +1327,7 @@ static int _mv88e6xxx_port_state(struct mv88e6xxx_priv_state *ps, int port,
 		if (ret)
 			return ret;
 
-		netdev_dbg(ds->ports[port], "PortState %s (was %s)\n",
+		netdev_dbg(ds->ports[port].netdev, "PortState %s (was %s)\n",
 			   mv88e6xxx_port_state_names[state],
 			   mv88e6xxx_port_state_names[oldstate]);
 	}
@@ -1405,7 +1405,8 @@ static void mv88e6xxx_port_stp_state_set(struct dsa_switch *ds, int port,
 	mutex_unlock(&ps->smi_mutex);
 
 	if (err)
-		netdev_err(ds->ports[port], "failed to update state to %s\n",
+		netdev_err(ds->ports[port].netdev,
+			   "failed to update state to %s\n",
 			   mv88e6xxx_port_state_names[stp_state]);
 }
 
@@ -1431,8 +1432,8 @@ static int _mv88e6xxx_port_pvid(struct mv88e6xxx_priv_state *ps, int port,
 		if (ret < 0)
 			return ret;
 
-		netdev_dbg(ds->ports[port], "DefaultVID %d (was %d)\n", *new,
-			   pvid);
+		netdev_dbg(ds->ports[port].netdev,
+			   "DefaultVID %d (was %d)\n", *new, pvid);
 	}
 
 	if (old)
@@ -1847,7 +1848,8 @@ static int _mv88e6xxx_port_fid(struct mv88e6xxx_priv_state *ps, int port,
 		if (ret < 0)
 			return ret;
 
-		netdev_dbg(ds->ports[port], "FID %d (was %d)\n", *new, fid);
+		netdev_dbg(ds->ports[port].netdev,
+			   "FID %d (was %d)\n", *new, fid);
 	}
 
 	if (old)
@@ -2028,7 +2030,7 @@ static int mv88e6xxx_port_check_hw_vlan(struct dsa_switch *ds, int port,
 			    ps->ports[port].bridge_dev)
 				break; /* same bridge, check next VLAN */
 
-			netdev_warn(ds->ports[port],
+			netdev_warn(ds->ports[port].netdev,
 				    "hardware VLAN %d already used by %s\n",
 				    vlan.vid,
 				    netdev_name(ps->ports[i].bridge_dev));
@@ -2078,7 +2080,7 @@ static int mv88e6xxx_port_vlan_filtering(struct dsa_switch *ds, int port,
 		if (ret < 0)
 			goto unlock;
 
-		netdev_dbg(ds->ports[port], "802.1Q Mode %s (was %s)\n",
+		netdev_dbg(ds->ports[port].netdev, "802.1Q Mode %s (was %s)\n",
 			   mv88e6xxx_port_8021q_mode_names[new],
 			   mv88e6xxx_port_8021q_mode_names[old]);
 	}
@@ -2147,11 +2149,12 @@ static void mv88e6xxx_port_vlan_add(struct dsa_switch *ds, int port,
 
 	for (vid = vlan->vid_begin; vid <= vlan->vid_end; ++vid)
 		if (_mv88e6xxx_port_vlan_add(ps, port, vid, untagged))
-			netdev_err(ds->ports[port], "failed to add VLAN %d%c\n",
+			netdev_err(ds->ports[port].netdev,
+				   "failed to add VLAN %d%c\n",
 				   vid, untagged ? 'u' : 't');
 
 	if (pvid && _mv88e6xxx_port_pvid_set(ps, port, vlan->vid_end))
-		netdev_err(ds->ports[port], "failed to set PVID %d\n",
+		netdev_err(ds->ports[port].netdev, "failed to set PVID %d\n",
 			   vlan->vid_end);
 
 	mutex_unlock(&ps->smi_mutex);
@@ -2336,7 +2339,8 @@ static void mv88e6xxx_port_fdb_add(struct dsa_switch *ds, int port,
 
 	mutex_lock(&ps->smi_mutex);
 	if (_mv88e6xxx_port_fdb_load(ps, port, fdb->addr, fdb->vid, state))
-		netdev_err(ds->ports[port], "failed to load MAC address\n");
+		netdev_err(ds->ports[port].netdev,
+			   "failed to load MAC address\n");
 	mutex_unlock(&ps->smi_mutex);
 }
 
@@ -2537,7 +2541,8 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
 	for (i = 0; i < ps->info->num_ports; ++i)
 		if (i == port || ps->ports[i].bridge_dev == bridge)
 			if (_mv88e6xxx_port_based_vlan_map(ps, i))
-				netdev_warn(ds->ports[i], "failed to remap\n");
+				netdev_warn(ds->ports[i].netdev,
+					    "failed to remap\n");
 
 	mutex_unlock(&ps->smi_mutex);
 }
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 17c3d37b6779..9aed8572037c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -119,6 +119,10 @@ struct dsa_switch_tree {
 	struct dsa_switch	*ds[DSA_MAX_SWITCHES];
 };
 
+struct dsa_port {
+	struct net_device	*netdev;
+};
+
 struct dsa_switch {
 	struct device *dev;
 
@@ -158,8 +162,8 @@ struct dsa_switch {
 	u32			dsa_port_mask;
 	u32			enabled_port_mask;
 	u32			phys_mii_mask;
+	struct dsa_port		ports[DSA_MAX_PORTS];
 	struct mii_bus		*slave_mii_bus;
-	struct net_device	*ports[DSA_MAX_PORTS];
 };
 
 static inline bool dsa_is_cpu_port(struct dsa_switch *ds, int p)
@@ -174,7 +178,7 @@ static inline bool dsa_is_dsa_port(struct dsa_switch *ds, int p)
 
 static inline bool dsa_is_port_initialized(struct dsa_switch *ds, int p)
 {
-	return ds->enabled_port_mask & (1 << p) && ds->ports[p];
+	return ds->enabled_port_mask & (1 << p) && ds->ports[p].netdev;
 }
 
 static inline u8 dsa_upstream_port(struct dsa_switch *ds)
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index eff5dfc2e33f..18086e0cc617 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -437,10 +437,10 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		if (!(ds->enabled_port_mask & (1 << port)))
 			continue;
 
-		if (!ds->ports[port])
+		if (!ds->ports[port].netdev)
 			continue;
 
-		dsa_slave_destroy(ds->ports[port]);
+		dsa_slave_destroy(ds->ports[port].netdev);
 	}
 
 	/* Remove any fixed link PHYs */
@@ -469,7 +469,7 @@ static int dsa_switch_suspend(struct dsa_switch *ds)
 		if (!dsa_is_port_initialized(ds, i))
 			continue;
 
-		ret = dsa_slave_suspend(ds->ports[i]);
+		ret = dsa_slave_suspend(ds->ports[i].netdev);
 		if (ret)
 			return ret;
 	}
@@ -495,7 +495,7 @@ static int dsa_switch_resume(struct dsa_switch *ds)
 		if (!dsa_is_port_initialized(ds, i))
 			continue;
 
-		ret = dsa_slave_resume(ds->ports[i]);
+		ret = dsa_slave_resume(ds->ports[i].netdev);
 		if (ret)
 			return ret;
 	}
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f640a48a6ff3..169abacbc6ce 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1183,12 +1183,12 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	p->old_link = -1;
 	p->old_duplex = -1;
 
-	ds->ports[port] = slave_dev;
+	ds->ports[port].netdev = slave_dev;
 	ret = register_netdev(slave_dev);
 	if (ret) {
 		netdev_err(master, "error %d registering interface %s\n",
 			   ret, slave_dev->name);
-		ds->ports[port] = NULL;
+		ds->ports[port].netdev = NULL;
 		free_netdev(slave_dev);
 		return ret;
 	}
diff --git a/net/dsa/tag_brcm.c b/net/dsa/tag_brcm.c
index e2aadb73111d..21bffde6e4bf 100644
--- a/net/dsa/tag_brcm.c
+++ b/net/dsa/tag_brcm.c
@@ -127,7 +127,7 @@ static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 	source_port = brcm_tag[3] & BRCM_EG_PID_MASK;
 
 	/* Validate port against switch setup, either the port is totally */
-	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
+	if (source_port >= DSA_MAX_PORTS || !ds->ports[source_port].netdev)
 		goto out_drop;
 
 	/* Remove Broadcom tag and update checksum */
@@ -140,7 +140,7 @@ static int brcm_tag_rcv(struct sk_buff *skb, struct net_device *dev,
 
 	skb_push(skb, ETH_HLEN);
 	skb->pkt_type = PACKET_HOST;
-	skb->dev = ds->ports[source_port];
+	skb->dev = ds->ports[source_port].netdev;
 	skb->protocol = eth_type_trans(skb, skb->dev);
 
 	skb->dev->stats.rx_packets++;
diff --git a/net/dsa/tag_dsa.c b/net/dsa/tag_dsa.c
index f9832f097681..bce79ffe342b 100644
--- a/net/dsa/tag_dsa.c
+++ b/net/dsa/tag_dsa.c
@@ -114,7 +114,7 @@ static int dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!ds)
 		goto out_drop;
 
-	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
+	if (source_port >= DSA_MAX_PORTS || !ds->ports[source_port].netdev)
 		goto out_drop;
 
 	/*
@@ -163,7 +163,7 @@ static int dsa_rcv(struct sk_buff *skb, struct net_device *dev,
 			2 * ETH_ALEN);
 	}
 
-	skb->dev = ds->ports[source_port];
+	skb->dev = ds->ports[source_port].netdev;
 	skb_push(skb, ETH_HLEN);
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, skb->dev);
diff --git a/net/dsa/tag_edsa.c b/net/dsa/tag_edsa.c
index 3890aac8190f..6c1720e88537 100644
--- a/net/dsa/tag_edsa.c
+++ b/net/dsa/tag_edsa.c
@@ -127,7 +127,7 @@ static int edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 	if (!ds)
 		goto out_drop;
 
-	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
+	if (source_port >= DSA_MAX_PORTS || !ds->ports[source_port].netdev)
 		goto out_drop;
 
 	/*
@@ -182,7 +182,7 @@ static int edsa_rcv(struct sk_buff *skb, struct net_device *dev,
 			2 * ETH_ALEN);
 	}
 
-	skb->dev = ds->ports[source_port];
+	skb->dev = ds->ports[source_port].netdev;
 	skb_push(skb, ETH_HLEN);
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, skb->dev);
diff --git a/net/dsa/tag_trailer.c b/net/dsa/tag_trailer.c
index b6ca0890d018..5e3903eb1afa 100644
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -82,12 +82,12 @@ static int trailer_rcv(struct sk_buff *skb, struct net_device *dev,
 		goto out_drop;
 
 	source_port = trailer[1] & 7;
-	if (source_port >= DSA_MAX_PORTS || ds->ports[source_port] == NULL)
+	if (source_port >= DSA_MAX_PORTS || !ds->ports[source_port].netdev)
 		goto out_drop;
 
 	pskb_trim_rcsum(skb, skb->len - 4);
 
-	skb->dev = ds->ports[source_port];
+	skb->dev = ds->ports[source_port].netdev;
 	skb_push(skb, ETH_HLEN);
 	skb->pkt_type = PACKET_HOST;
 	skb->protocol = eth_type_trans(skb, skb->dev);
-- 
2.8.1

[RFC PATCH 06/16] dsa: Move port device node into port structure

[Andrew Lunn]

Move the port device node structure into the port structure, from the
chip data. This information is needed in the next step of implementing
the new binding.

The chip data structure is used while parsing the whole old binding,
before the individual switch structures exist. With the new bindings,
this is reversed, the switches exist first, and the interconnections
between the switches is derived from the individual switch
bindings. Thus this chip data structure becomes unneeded.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h | 1 +
 net/dsa/dsa.c     | 8 ++++----
 net/dsa/slave.c   | 5 ++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index 9aed8572037c..8314197d028f 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -121,6 +121,7 @@ struct dsa_switch_tree {
 
 struct dsa_port {
 	struct net_device	*netdev;
+	struct device_node	*dn;
 };
 
 struct dsa_switch {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 18086e0cc617..5907f8cd13b6 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -182,7 +182,6 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
 /* basic switch operations **************************************************/
 static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
 {
-	struct dsa_chip_data *cd = ds->cd;
 	struct device_node *port_dn;
 	struct phy_device *phydev;
 	int ret, port, mode;
@@ -191,7 +190,7 @@ static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
 		if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
 			continue;
 
-		port_dn = cd->port_dn[port];
+		port_dn = ds->ports[port].dn;
 		if (of_phy_is_fixed_link(port_dn)) {
 			ret = of_phy_register_fixed_link(port_dn);
 			if (ret) {
@@ -325,6 +324,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	 * Create network devices for physical switch ports.
 	 */
 	for (i = 0; i < DSA_MAX_PORTS; i++) {
+		ds->ports[i].dn = cd->port_dn[i];
+
 		if (!(ds->enabled_port_mask & (1 << i)))
 			continue;
 
@@ -424,7 +425,6 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 {
 	struct device_node *port_dn;
 	struct phy_device *phydev;
-	struct dsa_chip_data *cd = ds->cd;
 	int port;
 
 #ifdef CONFIG_NET_DSA_HWMON
@@ -445,7 +445,7 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 
 	/* Remove any fixed link PHYs */
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
-		port_dn = cd->port_dn[port];
+		port_dn = ds->ports[port].dn;
 		if (of_phy_is_fixed_link(port_dn)) {
 			phydev = of_phy_find_device(port_dn);
 			if (phydev) {
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 169abacbc6ce..52f1183c42a0 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -998,13 +998,12 @@ static int dsa_slave_phy_setup(struct dsa_slave_priv *p,
 				struct net_device *slave_dev)
 {
 	struct dsa_switch *ds = p->parent;
-	struct dsa_chip_data *cd = ds->cd;
 	struct device_node *phy_dn, *port_dn;
 	bool phy_is_fixed = false;
 	u32 phy_flags = 0;
 	int mode, ret;
 
-	port_dn = cd->port_dn[p->port];
+	port_dn = ds->ports[p->port].dn;
 	mode = of_get_phy_mode(port_dn);
 	if (mode < 0)
 		mode = PHY_INTERFACE_MODE_NA;
@@ -1146,7 +1145,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 				 NULL);
 
 	SET_NETDEV_DEV(slave_dev, parent);
-	slave_dev->dev.of_node = ds->cd->port_dn[port];
+	slave_dev->dev.of_node = ds->ports[port].dn;
 	slave_dev->vlan_features = master->vlan_features;
 
 	p = netdev_priv(slave_dev);
-- 
2.8.1

[RFC PATCH 07/16] dsa: Remove dynamic allocate of routing table

[Andrew Lunn]

With a maximum of four switches, the size of the routing table is the
same as the pointer to it. Removing it makes the code simpler.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx.c |  3 +--
 include/net/dsa.h           | 10 +++++-----
 net/dsa/dsa.c               | 12 ------------
 3 files changed, 6 insertions(+), 19 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 85332d9a245a..d622c0fb76cc 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3024,8 +3024,7 @@ static int mv88e6xxx_setup_global(struct mv88e6xxx_priv_state *ps)
 	for (i = 0; i < 32; i++) {
 		int nexthop = 0x1f;
 
-		if (ps->ds->cd->rtable &&
-		    i != ps->ds->index && i < ps->ds->dst->pd->nr_chips)
+		if (i != ps->ds->index && i < ps->ds->dst->pd->nr_chips)
 			nexthop = ps->ds->cd->rtable[i] & 0x1f;
 
 		err = _mv88e6xxx_reg_write(
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 8314197d028f..37e8e179d85a 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -58,12 +58,12 @@ struct dsa_chip_data {
 	struct device_node *port_dn[DSA_MAX_PORTS];
 
 	/*
-	 * An array (with nr_chips elements) of which element [a]
-	 * indicates which port on this switch should be used to
-	 * send packets to that are destined for switch a.  Can be
-	 * NULL if there is only one switch chip.
+	 * An array of which element [a] indicates which port on this
+	 * switch should be used to send packets to that are destined
+	 * for switch a.  Can be NULL if there is only one switch
+	 * chip.
 	 */
-	s8		*rtable;
+	s8		rtable[DSA_MAX_SWITCHES];
 };
 
 struct dsa_platform_data {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 5907f8cd13b6..6177dd750847 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -587,17 +587,6 @@ static int dsa_of_setup_routing_table(struct dsa_platform_data *pd,
 	if (link_sw_addr >= pd->nr_chips)
 		return -EINVAL;
 
-	/* First time routing table allocation */
-	if (!cd->rtable) {
-		cd->rtable = kmalloc_array(pd->nr_chips, sizeof(s8),
-					   GFP_KERNEL);
-		if (!cd->rtable)
-			return -ENOMEM;
-
-		/* default to no valid uplink/downlink */
-		memset(cd->rtable, -1, pd->nr_chips * sizeof(s8));
-	}
-
 	cd->rtable[link_sw_addr] = port_index;
 
 	return 0;
@@ -639,7 +628,6 @@ static void dsa_of_free_platform_data(struct dsa_platform_data *pd)
 			kfree(pd->chip[i].port_names[port_index]);
 			port_index++;
 		}
-		kfree(pd->chip[i].rtable);
 
 		/* Drop our reference to the MDIO bus device */
 		if (pd->chip[i].host_dev)
-- 
2.8.1

Re: [RFC PATCH 07/16] dsa: Remove dynamic allocate of routing table

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

>  	/*
> -	 * An array (with nr_chips elements) of which element [a]
> -	 * indicates which port on this switch should be used to
> -	 * send packets to that are destined for switch a.  Can be
> -	 * NULL if there is only one switch chip.
> +	 * An array of which element [a] indicates which port on this
> +	 * switch should be used to send packets to that are destined
> +	 * for switch a.  Can be NULL if there is only one switch
> +	 * chip.
>  	 */

Please drop the double-space and reformat on 3 lines in the meantime.

Thanks,

        Vivien

[RFC PATCH 08/16] dsa: Copy the routing table into the switch structure

[Andrew Lunn]

The new binding will not have a chip data structure, it will place the
routing directly into the switch structure. To enable backwards
compatibility, copy the routing from the chip data into the switch
structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx.c |  4 ++--
 include/net/dsa.h           | 10 +++++++++-
 net/dsa/dsa.c               |  2 ++
 3 files changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index d622c0fb76cc..492801a6398c 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3024,8 +3024,8 @@ static int mv88e6xxx_setup_global(struct mv88e6xxx_priv_state *ps)
 	for (i = 0; i < 32; i++) {
 		int nexthop = 0x1f;
 
-		if (i != ps->ds->index && i < ps->ds->dst->pd->nr_chips)
-			nexthop = ps->ds->cd->rtable[i] & 0x1f;
+		if (i != ds->index && i < DSA_MAX_SWITCHES)
+			nexthop = ds->rtable[i] & 0x1f;
 
 		err = _mv88e6xxx_reg_write(
 			ps, REG_GLOBAL2,
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 37e8e179d85a..ea0e7d30342b 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -149,6 +149,14 @@ struct dsa_switch {
 	 */
 	struct dsa_switch_driver	*drv;
 
+	/*
+	 * An array of which element [a] indicates which port on this
+	 * switch should be used to send packets to that are destined
+	 * for switch a.  Can be NULL if there is only one switch
+	 * chip.
+	 */
+	s8		rtable[DSA_MAX_SWITCHES];
+
 #ifdef CONFIG_NET_DSA_HWMON
 	/*
 	 * Hardware monitoring information
@@ -195,7 +203,7 @@ static inline u8 dsa_upstream_port(struct dsa_switch *ds)
 	if (dst->cpu_switch == ds->index)
 		return dst->cpu_port;
 	else
-		return ds->cd->rtable[dst->cpu_switch];
+		return ds->rtable[dst->cpu_switch];
 }
 
 struct switchdev_trans;
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 6177dd750847..bfe1d03d4730 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -297,6 +297,8 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 		dst->tag_protocol = drv->tag_protocol;
 	}
 
+	memcpy(ds->rtable, cd->rtable, sizeof(ds->rtable));
+
 	/*
 	 * Do basic register setup.
 	 */
-- 
2.8.1

[RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Andrew Lunn]

Refactor the code to setup a single DSA/CPU port into a function of
its own, and export it, so it can be used by the new binding.

Similarly, refactor the destroy code into a function.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/dsa.c      | 86 ++++++++++++++++++++++++++++++++----------------------
 net/dsa/dsa_priv.h |  3 ++
 2 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index bfe1d03d4730..20eede3facf5 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -180,36 +180,47 @@ __ATTRIBUTE_GROUPS(dsa_hwmon);
 #endif /* CONFIG_NET_DSA_HWMON */
 
 /* basic switch operations **************************************************/
-static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
+int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
+		      struct device_node *port_dn, int port)
 {
-	struct device_node *port_dn;
 	struct phy_device *phydev;
-	int ret, port, mode;
+	int ret, mode;
+
+	if (of_phy_is_fixed_link(port_dn)) {
+		ret = of_phy_register_fixed_link(port_dn);
+		if (ret) {
+			dev_err(dev, "failed to register fixed PHY\n");
+			return ret;
+		}
+		phydev = of_phy_find_device(port_dn);
+
+		mode = of_get_phy_mode(port_dn);
+		if (mode < 0)
+			mode = PHY_INTERFACE_MODE_NA;
+		phydev->interface = mode;
+
+		genphy_config_init(phydev);
+		genphy_read_status(phydev);
+		if (ds->drv->adjust_link)
+			ds->drv->adjust_link(ds, port, phydev);
+	}
+
+	return 0;
+}
+
+static int dsa_cpu_dsa_setups(struct dsa_switch *ds, struct device *dev)
+{
+	struct device_node *port_dn;
+	int ret, port;
 
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
 		if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
 			continue;
 
 		port_dn = ds->ports[port].dn;
-		if (of_phy_is_fixed_link(port_dn)) {
-			ret = of_phy_register_fixed_link(port_dn);
-			if (ret) {
-				netdev_err(master,
-					   "failed to register fixed PHY\n");
-				return ret;
-			}
-			phydev = of_phy_find_device(port_dn);
-
-			mode = of_get_phy_mode(port_dn);
-			if (mode < 0)
-				mode = PHY_INTERFACE_MODE_NA;
-			phydev->interface = mode;
-
-			genphy_config_init(phydev);
-			genphy_read_status(phydev);
-			if (ds->drv->adjust_link)
-				ds->drv->adjust_link(ds, port, phydev);
-		}
+		ret = dsa_cpu_dsa_setup(ds, dev, port_dn, port);
+		if (ret)
+			return ret;
 	}
 	return 0;
 }
@@ -340,7 +351,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	}
 
 	/* Perform configuration of the CPU and DSA ports */
-	ret = dsa_cpu_dsa_setup(ds, dst->master_netdev);
+	ret = dsa_cpu_dsa_setups(ds, parent);
 	if (ret < 0) {
 		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
 			   index);
@@ -423,10 +434,21 @@ dsa_switch_setup(struct dsa_switch_tree *dst, int index,
 	return ds;
 }
 
-static void dsa_switch_destroy(struct dsa_switch *ds)
+void dsa_cpu_dsa_destroy(struct device_node *port_dn)
 {
-	struct device_node *port_dn;
 	struct phy_device *phydev;
+
+	if (of_phy_is_fixed_link(port_dn)) {
+		phydev = of_phy_find_device(port_dn);
+		if (phydev) {
+			phy_device_free(phydev);
+			fixed_phy_unregister(phydev);
+		}
+	}
+}
+
+static void dsa_switch_destroy(struct dsa_switch *ds)
+{
 	int port;
 
 #ifdef CONFIG_NET_DSA_HWMON
@@ -445,17 +467,11 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		dsa_slave_destroy(ds->ports[port].netdev);
 	}
 
-	/* Remove any fixed link PHYs */
+	/* Disable configuration of the CPU and DSA ports */
 	for (port = 0; port < DSA_MAX_PORTS; port++) {
-		port_dn = ds->ports[port].dn;
-		if (of_phy_is_fixed_link(port_dn)) {
-			phydev = of_phy_find_device(port_dn);
-			if (phydev) {
-				phy_device_free(phydev);
-				of_node_put(port_dn);
-				fixed_phy_unregister(phydev);
-			}
-		}
+		if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
+			continue;
+		dsa_cpu_dsa_destroy(ds->ports[port].dn);
 	}
 
 	mdiobus_unregister(ds->slave_mii_bus);
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index dfa33779d49c..dbea5d9e7f75 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -50,6 +50,9 @@ struct dsa_slave_priv {
 
 /* dsa.c */
 extern char dsa_driver_version[];
+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);
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
-- 
2.8.1

Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> -static void dsa_switch_destroy(struct dsa_switch *ds)
> +void dsa_cpu_dsa_destroy(struct device_node *port_dn)
>  {
> -	struct device_node *port_dn;
>  	struct phy_device *phydev;
> +
> +	if (of_phy_is_fixed_link(port_dn)) {
> +		phydev = of_phy_find_device(port_dn);
> +		if (phydev) {
> +			phy_device_free(phydev);
> +			fixed_phy_unregister(phydev);
> +		}
> +	}
> +}
> +
> +static void dsa_switch_destroy(struct dsa_switch *ds)
> +{
>  	int port;
>  
>  #ifdef CONFIG_NET_DSA_HWMON
> @@ -445,17 +467,11 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
>  		dsa_slave_destroy(ds->ports[port].netdev);
>  	}
>  
> -	/* Remove any fixed link PHYs */
> +	/* Disable configuration of the CPU and DSA ports */
>  	for (port = 0; port < DSA_MAX_PORTS; port++) {
> -		port_dn = ds->ports[port].dn;
> -		if (of_phy_is_fixed_link(port_dn)) {
> -			phydev = of_phy_find_device(port_dn);
> -			if (phydev) {
> -				phy_device_free(phydev);
> -				of_node_put(port_dn);

Why does dsa_cpu_dsa_destroy drop that of_node_put call?

> -				fixed_phy_unregister(phydev);
> -			}
> -		}
> +		if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
> +			continue;

Why do we skip DSA and CPU ports here? The previous code didn't.

> +		dsa_cpu_dsa_destroy(ds->ports[port].dn);

Thanks,

        Vivien

Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Andrew Lunn]

On Fri, May 27, 2016 at 10:33:49AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > -static void dsa_switch_destroy(struct dsa_switch *ds)
> > +void dsa_cpu_dsa_destroy(struct device_node *port_dn)
> >  {
> > -	struct device_node *port_dn;
> >  	struct phy_device *phydev;
> > +
> > +	if (of_phy_is_fixed_link(port_dn)) {
> > +		phydev = of_phy_find_device(port_dn);
> > +		if (phydev) {
> > +			phy_device_free(phydev);
> > +			fixed_phy_unregister(phydev);
> > +		}
> > +	}
> > +}
> > +
> > +static void dsa_switch_destroy(struct dsa_switch *ds)
> > +{
> >  	int port;
> >  
> >  #ifdef CONFIG_NET_DSA_HWMON
> > @@ -445,17 +467,11 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
> >  		dsa_slave_destroy(ds->ports[port].netdev);
> >  	}
> >  
> > -	/* Remove any fixed link PHYs */
> > +	/* Disable configuration of the CPU and DSA ports */
> >  	for (port = 0; port < DSA_MAX_PORTS; port++) {
> > -		port_dn = ds->ports[port].dn;
> > -		if (of_phy_is_fixed_link(port_dn)) {
> > -			phydev = of_phy_find_device(port_dn);
> > -			if (phydev) {
> > -				phy_device_free(phydev);
> > -				of_node_put(port_dn);
> 
> Why does dsa_cpu_dsa_destroy drop that of_node_put call?

The of node reference counting is broken. The DT maintainers actually
say not to care, the whole reference counting scheme is broken. Which
is a bit sad really. There was a discussion about this a couple of
months ago.

Anyway, the reference is taken in dsa_of_probe() as part of the
or_each_available_child_of_node(child, port). This reference has
nothing to do with the port being a fixed link or not. So freeing it
here is inappropriate. The correct place to free it would probably be
in dsa_of_remove.
 
> > -				fixed_phy_unregister(phydev);
> > -			}
> > -		}
> > +		if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
> > +			continue;
> 
> Why do we skip DSA and CPU ports here? The previous code didn't.
> 
> > +		dsa_cpu_dsa_destroy(ds->ports[port].dn);

They are now destroyed by the newly added dsa_cpu_dsa_destroy().  I'm
making the code more symmetrical and easier to re-use. The inverse of
this function is dsa_switch_setup_one() and it also uses a helper
function to setup the dsa and cpu ports, dsa_cpu_dsa_setups().

	 Andrew

Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Fri, May 27, 2016 at 10:33:49AM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> > -static void dsa_switch_destroy(struct dsa_switch *ds)
>> > +void dsa_cpu_dsa_destroy(struct device_node *port_dn)
>> >  {
>> > -	struct device_node *port_dn;
>> >  	struct phy_device *phydev;
>> > +
>> > +	if (of_phy_is_fixed_link(port_dn)) {
>> > +		phydev = of_phy_find_device(port_dn);
>> > +		if (phydev) {
>> > +			phy_device_free(phydev);
>> > +			fixed_phy_unregister(phydev);
>> > +		}
>> > +	}
>> > +}
>> > +
>> > +static void dsa_switch_destroy(struct dsa_switch *ds)
>> > +{
>> >  	int port;
>> >  
>> >  #ifdef CONFIG_NET_DSA_HWMON
>> > @@ -445,17 +467,11 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
>> >  		dsa_slave_destroy(ds->ports[port].netdev);
>> >  	}
>> >  
>> > -	/* Remove any fixed link PHYs */
>> > +	/* Disable configuration of the CPU and DSA ports */
>> >  	for (port = 0; port < DSA_MAX_PORTS; port++) {
>> > -		port_dn = ds->ports[port].dn;
>> > -		if (of_phy_is_fixed_link(port_dn)) {
>> > -			phydev = of_phy_find_device(port_dn);
>> > -			if (phydev) {
>> > -				phy_device_free(phydev);
>> > -				of_node_put(port_dn);
>> 
>> Why does dsa_cpu_dsa_destroy drop that of_node_put call?
>
> The of node reference counting is broken. The DT maintainers actually
> say not to care, the whole reference counting scheme is broken. Which
> is a bit sad really. There was a discussion about this a couple of
> months ago.
>
> Anyway, the reference is taken in dsa_of_probe() as part of the
> or_each_available_child_of_node(child, port). This reference has
> nothing to do with the port being a fixed link or not. So freeing it
> here is inappropriate. The correct place to free it would probably be
> in dsa_of_remove.

OK, good to know. Can you split that in its own patch (prefered), or at
least document that in the commit message?

>> > -				fixed_phy_unregister(phydev);
>> > -			}
>> > -		}
>> > +		if ((dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
>> > +			continue;
>> 
>> Why do we skip DSA and CPU ports here? The previous code didn't.
>> 
>> > +		dsa_cpu_dsa_destroy(ds->ports[port].dn);
>
> They are now destroyed by the newly added dsa_cpu_dsa_destroy().  I'm
> making the code more symmetrical and easier to re-use. The inverse of
> this function is dsa_switch_setup_one() and it also uses a helper
> function to setup the dsa and cpu ports, dsa_cpu_dsa_setups().

But dsa_cpu_dsa_destroy() is not called here. Shouldn't we drop (like
before) or at least invert the condition above?

Thanks,

        Vivien

Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Vivien Didelot]

Hi Andrew, Florian,

I suggest to use this RFC to agree on a consistent and robust API for
the DSA layer. Some functions have non-intuitive names or signatures.

What about:

    <namespace>_<action>[_<object>]

where <namespace> matches the first argument. So instead of
dsa_cpu_dsa_setup, we would have:

    dsa_switch_setup_cpu_dsa_port(struct dsa_switch *ds, ...)

[...]

Andrew Lunn <andrew@lunn.ch> writes:

>  /* basic switch operations **************************************************/
> -static int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct net_device *master)
> +int dsa_cpu_dsa_setup(struct dsa_switch *ds, struct device *dev,
> +		      struct device_node *port_dn, int port)
>  {

We will need the port number in the struct dsa_port at some point. The
device_node is already a member of this structure.

I suggest to add the port index before this patch and change this
function for a more logical:

    dsa_switch_setup_cpu_dsa_port(struct dsa_switch *ds, struct dsa_port *dp)

[...]

>  	/* Perform configuration of the CPU and DSA ports */
> -	ret = dsa_cpu_dsa_setup(ds, dst->master_netdev);
> +	ret = dsa_cpu_dsa_setups(ds, parent);
>  	if (ret < 0) {
>  		netdev_err(dst->master_netdev, "[%d] : can't configure CPU and DSA ports\n",
>  			   index);

dsa_cpu_dsa_setups is not really meaningful and doesn't add much
value. At least I'd call it dsa_switch_setup_cpu_dsa_ports(), or better,
move the loop directly where it is needed so we know which port sucked:

dsa_switch_setup_one:

        /* Perform configuration of the CPU and DSA ports */
        for (port = 0; port < DSA_MAX_PORTS; port++) {
                if (!(dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port)))
                        continue;

                ret = dsa_switch_setup_cpu_dsa_port(ds, dev, ds->ports[port]);
                if (ret) {
                        netdev_err(dst->master_netdev,
                                   "[%d] : can't configure port %d\n",
                                   index, ds->ports[port]->index);
                        break;
                }
        }
        
Thanks,

        Vivien

Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Andrew Lunn]

On Fri, May 27, 2016 at 03:25:47PM -0400, Vivien Didelot wrote:
> Hi Andrew, Florian,
> 
> I suggest to use this RFC to agree on a consistent and robust API for
> the DSA layer. Some functions have non-intuitive names or signatures.

Nope.

What is important is getting this patchset accepted and merged, so
people can write drivers using it.

Changes like this can come later, since hopefully all changes like
this are internal. They don't affect the drivers, or the API to the
core.

	Andrew

Re: [RFC PATCH 09/16] dsa: dsa: Split up creating/destroying of DSA and CPU ports

[Andrew Lunn]

> dsa_cpu_dsa_setups is not really meaningful and doesn't add much
> value.

I would disagree.  Quoting Documentation/CodingStyle

                Chapter 6: Functions

Functions should be short and sweet, and do just one thing.  They should
fit on one or two screenfuls of text (the ISO/ANSI screen size is 80x24,
as we all know), and do one thing and do that well.

dsa_cpu_dsa_setups iterates the ports and find the dsa and cpu ports
that need setting up.

dsa_cpu_dsa_setup() does the actual setup.

These are completely different things, and so should be in different
functions.

	Andrew

[RFC PATCH 10/16] net: dsa: mv88e6xxx: Only support EDSA tagging

[Andrew Lunn]

The merged driver no longer offers the option to use DSA tagging. So
remove the code to setup the switch to do DSA tagging and hard code
the use of EDSA.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 492801a6398c..11845eccf670 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -2725,11 +2725,8 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_priv_state *ps, int port)
 		if (mv88e6xxx_6352_family(ps) || mv88e6xxx_6351_family(ps) ||
 		    mv88e6xxx_6165_family(ps) || mv88e6xxx_6097_family(ps) ||
 		    mv88e6xxx_6320_family(ps)) {
-			if (ds->dst->tag_protocol == DSA_TAG_PROTO_EDSA)
-				reg |= PORT_CONTROL_FRAME_ETHER_TYPE_DSA;
-			else
-				reg |= PORT_CONTROL_FRAME_MODE_DSA;
-			reg |= PORT_CONTROL_FORWARD_UNKNOWN |
+			reg |= PORT_CONTROL_FRAME_ETHER_TYPE_DSA |
+				PORT_CONTROL_FORWARD_UNKNOWN |
 				PORT_CONTROL_FORWARD_UNKNOWN_MC;
 		}
 
@@ -2737,7 +2734,6 @@ static int mv88e6xxx_setup_port(struct mv88e6xxx_priv_state *ps, int port)
 		    mv88e6xxx_6165_family(ps) || mv88e6xxx_6097_family(ps) ||
 		    mv88e6xxx_6095_family(ps) || mv88e6xxx_6065_family(ps) ||
 		    mv88e6xxx_6185_family(ps) || mv88e6xxx_6320_family(ps)) {
-			if (ds->dst->tag_protocol == DSA_TAG_PROTO_EDSA)
 				reg |= PORT_CONTROL_EGRESS_ADD_TAG;
 		}
 	}
-- 
2.8.1

[RFC PATCH 11/16] net: dsa: Refactor selection of tag ops into a function

[Andrew Lunn]

Replace the two switch statements with an array lookup, and store the
result in the dsa tree structure. The drivers no longer need to know
the selected tag protocol, so remove it from the dsa switch structure.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 include/net/dsa.h  |  8 +++++-
 net/dsa/dsa.c      | 71 ++++++++++++++++++++++++++++++++++--------------------
 net/dsa/dsa_priv.h |  1 +
 net/dsa/slave.c    | 35 +--------------------------
 4 files changed, 54 insertions(+), 61 deletions(-)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index ea0e7d30342b..adb75422bc6c 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -26,6 +26,7 @@ enum dsa_tag_protocol {
 	DSA_TAG_PROTO_TRAILER,
 	DSA_TAG_PROTO_EDSA,
 	DSA_TAG_PROTO_BRCM,
+	_DSA_TAG_LAST,
 };
 
 #define DSA_MAX_SWITCHES	4
@@ -100,7 +101,6 @@ struct dsa_switch_tree {
 				       struct net_device *dev,
 				       struct packet_type *pt,
 				       struct net_device *orig_dev);
-	enum dsa_tag_protocol	tag_protocol;
 
 	/*
 	 * Original copy of the master netdev ethtool_ops
@@ -117,6 +117,12 @@ struct dsa_switch_tree {
 	 * Data for the individual switch chips.
 	 */
 	struct dsa_switch	*ds[DSA_MAX_SWITCHES];
+
+	/*
+	 * Tagging protocol operations for adding and removing an
+	 * encapsulation tag.
+	 */
+	const struct dsa_device_ops *tag_ops;
 };
 
 struct dsa_port {
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 20eede3facf5..49cdf143c822 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -29,6 +29,33 @@
 
 char dsa_driver_version[] = "0.1";
 
+static struct sk_buff *dsa_slave_notag_xmit(struct sk_buff *skb,
+					    struct net_device *dev)
+{
+	/* Just return the original SKB */
+	return skb;
+}
+
+static const struct dsa_device_ops none_ops = {
+	.xmit	= dsa_slave_notag_xmit,
+	.rcv	= NULL,
+};
+
+const struct dsa_device_ops *dsa_device_ops[_DSA_TAG_LAST] = {
+#ifdef CONFIG_NET_DSA_TAG_DSA
+	[DSA_TAG_PROTO_DSA] = &dsa_netdev_ops,
+#endif
+#ifdef CONFIG_NET_DSA_TAG_EDSA
+	[DSA_TAG_PROTO_EDSA] = &edsa_netdev_ops,
+#endif
+#ifdef CONFIG_NET_DSA_TAG_TRAILER
+	[DSA_TAG_PROTO_TRAILER] = &trailer_netdev_ops,
+#endif
+#ifdef CONFIG_NET_DSA_TAG_BRCM
+	[DSA_TAG_PROTO_BRCM] = &brcm_netdev_ops,
+#endif
+	[DSA_TAG_PROTO_NONE] = &none_ops,
+};
 
 /* switch driver registration ***********************************************/
 static DEFINE_MUTEX(dsa_switch_drivers_mutex);
@@ -225,6 +252,20 @@ static int dsa_cpu_dsa_setups(struct dsa_switch *ds, struct device *dev)
 	return 0;
 }
 
+const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol)
+{
+	const struct dsa_device_ops *ops;
+
+	if (tag_protocol >= _DSA_TAG_LAST)
+		return ERR_PTR(-EINVAL);
+	ops = dsa_device_ops[tag_protocol];
+
+	if (!ops)
+		return ERR_PTR(-ENOPROTOOPT);
+
+	return ops;
+}
+
 static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 {
 	struct dsa_switch_driver *drv = ds->drv;
@@ -277,35 +318,13 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	 * switch.
 	 */
 	if (dst->cpu_switch == index) {
-		switch (drv->tag_protocol) {
-#ifdef CONFIG_NET_DSA_TAG_DSA
-		case DSA_TAG_PROTO_DSA:
-			dst->rcv = dsa_netdev_ops.rcv;
-			break;
-#endif
-#ifdef CONFIG_NET_DSA_TAG_EDSA
-		case DSA_TAG_PROTO_EDSA:
-			dst->rcv = edsa_netdev_ops.rcv;
-			break;
-#endif
-#ifdef CONFIG_NET_DSA_TAG_TRAILER
-		case DSA_TAG_PROTO_TRAILER:
-			dst->rcv = trailer_netdev_ops.rcv;
-			break;
-#endif
-#ifdef CONFIG_NET_DSA_TAG_BRCM
-		case DSA_TAG_PROTO_BRCM:
-			dst->rcv = brcm_netdev_ops.rcv;
-			break;
-#endif
-		case DSA_TAG_PROTO_NONE:
-			break;
-		default:
-			ret = -ENOPROTOOPT;
+		dst->tag_ops = dsa_resolve_tag_protocol(drv->tag_protocol);
+		if (IS_ERR(dst->tag_ops)) {
+			ret = PTR_ERR(dst->tag_ops);
 			goto out;
 		}
 
-		dst->tag_protocol = drv->tag_protocol;
+		dst->rcv = dst->tag_ops->rcv;
 	}
 
 	memcpy(ds->rtable, cd->rtable, sizeof(ds->rtable));
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index dbea5d9e7f75..72f7b8989cfb 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -53,6 +53,7 @@ extern char dsa_driver_version[];
 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);
 
 /* slave.c */
 extern const struct dsa_device_ops notag_netdev_ops;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 52f1183c42a0..35e5f0f6688b 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -521,14 +521,6 @@ static netdev_tx_t dsa_slave_xmit(struct sk_buff *skb, struct net_device *dev)
 	return NETDEV_TX_OK;
 }
 
-static struct sk_buff *dsa_slave_notag_xmit(struct sk_buff *skb,
-					    struct net_device *dev)
-{
-	/* Just return the original SKB */
-	return skb;
-}
-
-
 /* ethtool operations *******************************************************/
 static int
 dsa_slave_get_settings(struct net_device *dev, struct ethtool_cmd *cmd)
@@ -1151,32 +1143,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 	p = netdev_priv(slave_dev);
 	p->parent = ds;
 	p->port = port;
-
-	switch (ds->dst->tag_protocol) {
-#ifdef CONFIG_NET_DSA_TAG_DSA
-	case DSA_TAG_PROTO_DSA:
-		p->xmit = dsa_netdev_ops.xmit;
-		break;
-#endif
-#ifdef CONFIG_NET_DSA_TAG_EDSA
-	case DSA_TAG_PROTO_EDSA:
-		p->xmit = edsa_netdev_ops.xmit;
-		break;
-#endif
-#ifdef CONFIG_NET_DSA_TAG_TRAILER
-	case DSA_TAG_PROTO_TRAILER:
-		p->xmit = trailer_netdev_ops.xmit;
-		break;
-#endif
-#ifdef CONFIG_NET_DSA_TAG_BRCM
-	case DSA_TAG_PROTO_BRCM:
-		p->xmit = brcm_netdev_ops.xmit;
-		break;
-#endif
-	default:
-		p->xmit	= dsa_slave_notag_xmit;
-		break;
-	}
+	p->xmit = dst->tag_ops->xmit;
 
 	p->old_pause = -1;
 	p->old_link = -1;
-- 
2.8.1

Re: [RFC PATCH 11/16] net: dsa: Refactor selection of tag ops into a function

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> @@ -26,6 +26,7 @@ enum dsa_tag_protocol {
>  	DSA_TAG_PROTO_TRAILER,
>  	DSA_TAG_PROTO_EDSA,
>  	DSA_TAG_PROTO_BRCM,
> +	_DSA_TAG_LAST,
>  };

I would avoid _ prefixed functions or symbols, we've seen in mv88e6xxx
that it doesn't make the code really readable.

There's already an implicit "DSA_TAG_PROTO" namespace, so I suggest
"DSA_MAX_TAGS" to keep it consistent with DSA_MAX_{PORTS,SWITCHES}.

[...]

> +	/*
> +	 * Tagging protocol operations for adding and removing an
> +	 * encapsulation tag.
> +	 */
> +	const struct dsa_device_ops *tag_ops;

dsa_device_ops seems too generic for the xmit/rcv tag-related pair. That
being said, I don't really have better suggestions. Any idea?

[...]

> +const struct dsa_device_ops *dsa_device_ops[_DSA_TAG_LAST] = {
> +#ifdef CONFIG_NET_DSA_TAG_DSA
> +	[DSA_TAG_PROTO_DSA] = &dsa_netdev_ops,
> +#endif
> +#ifdef CONFIG_NET_DSA_TAG_EDSA
> +	[DSA_TAG_PROTO_EDSA] = &edsa_netdev_ops,
> +#endif
> +#ifdef CONFIG_NET_DSA_TAG_TRAILER
> +	[DSA_TAG_PROTO_TRAILER] = &trailer_netdev_ops,
> +#endif
> +#ifdef CONFIG_NET_DSA_TAG_BRCM
> +	[DSA_TAG_PROTO_BRCM] = &brcm_netdev_ops,
> +#endif
> +	[DSA_TAG_PROTO_NONE] = &none_ops,
> +};
>  
>  /* switch driver registration ***********************************************/
>  static DEFINE_MUTEX(dsa_switch_drivers_mutex);
> @@ -225,6 +252,20 @@ static int dsa_cpu_dsa_setups(struct dsa_switch *ds, struct device *dev)
>  	return 0;
>  }
>  
> +const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol)
> +{
> +	const struct dsa_device_ops *ops;
> +
> +	if (tag_protocol >= _DSA_TAG_LAST)
> +		return ERR_PTR(-EINVAL);
> +	ops = dsa_device_ops[tag_protocol];
> +
> +	if (!ops)
> +		return ERR_PTR(-ENOPROTOOPT);
> +
> +	return ops;
> +}

I don't see the need to add a dsa_device_ops array. I'd keep the switch
case on tag_protocol within this new dsa_resolve_tag_protocol function,
to make it more readable (no value checking necessary).

Thanks,

        Vivien

Re: [RFC PATCH 11/16] net: dsa: Refactor selection of tag ops into a function

[Andrew Lunn]

On Fri, May 27, 2016 at 03:35:57PM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > @@ -26,6 +26,7 @@ enum dsa_tag_protocol {
> >  	DSA_TAG_PROTO_TRAILER,
> >  	DSA_TAG_PROTO_EDSA,
> >  	DSA_TAG_PROTO_BRCM,
> > +	_DSA_TAG_LAST,
> >  };
> 
> I would avoid _ prefixed functions or symbols, we've seen in mv88e6xxx
> that it doesn't make the code really readable.

The point is to make is special, so that people look at it, wonder why
it is special, and don't make the stupid mistakes of adding a new
protocol after it.

However, if you don't like the _, we can just add a comment
/* MUST BE LAST */

> I don't see the need to add a dsa_device_ops array. I'd keep the switch
> case on tag_protocol within this new dsa_resolve_tag_protocol function,
> to make it more readable (no value checking necessary).

I don't see one way being better than the other. I see it as a
personal preference. If you don't like it, i won't NACK a patch
submitted after it has been accepted which changes it to your personal
preference.

      Andrew

[RFC PATCH 12/16] dsa: Make mdio bus optional

[Andrew Lunn]

The switch may want to instantiate its own MDIO bus. Only do it
centrally if the switch has not already created one, and the read op
is implemented.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 net/dsa/dsa.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index 49cdf143c822..b1787e2f4bb3 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -340,17 +340,18 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 	if (ret < 0)
 		goto out;
 
-	ds->slave_mii_bus = devm_mdiobus_alloc(parent);
-	if (ds->slave_mii_bus == NULL) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	dsa_slave_mii_bus_init(ds);
-
-	ret = mdiobus_register(ds->slave_mii_bus);
-	if (ret < 0)
-		goto out;
+	if (!ds->slave_mii_bus && drv->phy_read) {
+		ds->slave_mii_bus = devm_mdiobus_alloc(parent);
+		if (!ds->slave_mii_bus) {
+			ret = -ENOMEM;
+			goto out;
+		}
+		dsa_slave_mii_bus_init(ds);
 
+		ret = mdiobus_register(ds->slave_mii_bus);
+		if (ret < 0)
+			goto out;
+	}
 
 	/*
 	 * Create network devices for physical switch ports.
@@ -493,7 +494,8 @@ static void dsa_switch_destroy(struct dsa_switch *ds)
 		dsa_cpu_dsa_destroy(ds->ports[port].dn);
 	}
 
-	mdiobus_unregister(ds->slave_mii_bus);
+	if (ds->slave_mii_bus && ds->drv->phy_read)
+		mdiobus_unregister(ds->slave_mii_bus);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.8.1

Re: [RFC PATCH 12/16] dsa: Make mdio bus optional

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> -	mdiobus_unregister(ds->slave_mii_bus);
> +	if (ds->slave_mii_bus && ds->drv->phy_read)
> +		mdiobus_unregister(ds->slave_mii_bus);

So if a driver registered the slave MII bus itself, it may have
unregistered it itself as well, so checking ds->slave_mii_bus is OK
(assuming the driver correctly zero'ed it).

But is it necessary to check ds->drv->phy_read?

Thanks,

        Vivien

Re: [RFC PATCH 12/16] dsa: Make mdio bus optional

[Andrew Lunn]

On Fri, May 27, 2016 at 10:55:45AM -0400, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > -	mdiobus_unregister(ds->slave_mii_bus);
> > +	if (ds->slave_mii_bus && ds->drv->phy_read)
> > +		mdiobus_unregister(ds->slave_mii_bus);
> 
> So if a driver registered the slave MII bus itself, it may have
> unregistered it itself as well, so checking ds->slave_mii_bus is OK
> (assuming the driver correctly zero'ed it).
> 
> But is it necessary to check ds->drv->phy_read?

It makes the code symmetrical to the register code, which makes the
same check. My experience is that keeping the code symmetrical results
in less surprises late on.

One such surprise could be a driver that does not zero
ds->slave_mii_bus. In fact, mv88e6xxx does not zero it. So we would
get a double unregister happening. We also don't want the core
unregistering it, since we have other cleanup work to do, we have an
of_node_put() to call.

	Andrew

Re: [RFC PATCH 12/16] dsa: Make mdio bus optional

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Fri, May 27, 2016 at 10:55:45AM -0400, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> > -	mdiobus_unregister(ds->slave_mii_bus);
>> > +	if (ds->slave_mii_bus && ds->drv->phy_read)
>> > +		mdiobus_unregister(ds->slave_mii_bus);
>> 
>> So if a driver registered the slave MII bus itself, it may have
>> unregistered it itself as well, so checking ds->slave_mii_bus is OK
>> (assuming the driver correctly zero'ed it).
>> 
>> But is it necessary to check ds->drv->phy_read?
>
> It makes the code symmetrical to the register code, which makes the
> same check. My experience is that keeping the code symmetrical results
> in less surprises late on.
>
> One such surprise could be a driver that does not zero
> ds->slave_mii_bus. In fact, mv88e6xxx does not zero it. So we would
> get a double unregister happening. We also don't want the core
> unregistering it, since we have other cleanup work to do, we have an
> of_node_put() to call.

OK good for me, as long as it is intuitive for the driver
implementation.

Thanks,

        Vivien

[RFC PATCH 13/16] net: dsa: mv88e6xxx: Refactor MDIO so driver registers mdio bus

[Andrew Lunn]

Have the switch driver register its own MDIO bus. This allows for an
mdio property in the device tree, with child nodes for phys, which
can be referenced via phandles, etc.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 drivers/net/dsa/mv88e6xxx.c | 204 ++++++++++++++++++++++++++++++--------------
 drivers/net/dsa/mv88e6xxx.h |   6 ++
 2 files changed, 144 insertions(+), 66 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 11845eccf670..8fbc771f0475 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -21,6 +21,7 @@
 #include <linux/list.h>
 #include <linux/mdio.h>
 #include <linux/module.h>
+#include <linux/of_mdio.h>
 #include <linux/netdevice.h>
 #include <linux/gpio/consumer.h>
 #include <linux/phy.h>
@@ -238,16 +239,16 @@ int mv88e6xxx_set_addr(struct dsa_switch *ds, u8 *addr)
 		return mv88e6xxx_set_addr_direct(ds, addr);
 }
 
-static int _mv88e6xxx_phy_read(struct mv88e6xxx_priv_state *ps, int addr,
-			       int regnum)
+static int _mv88e6xxx_mdio_read(struct mv88e6xxx_priv_state *ps, int addr,
+				int regnum)
 {
 	if (addr >= 0)
 		return _mv88e6xxx_reg_read(ps, addr, regnum);
 	return 0xffff;
 }
 
-static int _mv88e6xxx_phy_write(struct mv88e6xxx_priv_state *ps, int addr,
-				int regnum, u16 val)
+static int _mv88e6xxx_mdio_write(struct mv88e6xxx_priv_state *ps, int addr,
+				 int regnum, u16 val)
 {
 	if (addr >= 0)
 		return _mv88e6xxx_reg_write(ps, addr, regnum, val);
@@ -378,8 +379,8 @@ void mv88e6xxx_ppu_state_init(struct mv88e6xxx_priv_state *ps)
 	ps->ppu_timer.function = mv88e6xxx_ppu_reenable_timer;
 }
 
-static int mv88e6xxx_phy_read_ppu(struct mv88e6xxx_priv_state *ps, int addr,
-				  int regnum)
+static int mv88e6xxx_mdio_read_ppu(struct mv88e6xxx_priv_state *ps, int addr,
+				   int regnum)
 {
 	int ret;
 
@@ -392,8 +393,8 @@ static int mv88e6xxx_phy_read_ppu(struct mv88e6xxx_priv_state *ps, int addr,
 	return ret;
 }
 
-static int mv88e6xxx_phy_write_ppu(struct mv88e6xxx_priv_state *ps, int addr,
-				   int regnum, u16 val)
+static int mv88e6xxx_mdio_write_ppu(struct mv88e6xxx_priv_state *ps, int addr,
+				    int regnum, u16 val)
 {
 	int ret;
 
@@ -829,7 +830,7 @@ static int mv88e6xxx_wait(struct mv88e6xxx_priv_state *ps, int reg,
 	return ret;
 }
 
-static int _mv88e6xxx_phy_wait(struct mv88e6xxx_priv_state *ps)
+static int _mv88e6xxx_mdio_wait(struct mv88e6xxx_priv_state *ps)
 {
 	return _mv88e6xxx_wait(ps, REG_GLOBAL2, GLOBAL2_SMI_OP,
 			       GLOBAL2_SMI_OP_BUSY);
@@ -1076,8 +1077,8 @@ static int _mv88e6xxx_atu_wait(struct mv88e6xxx_priv_state *ps)
 			       GLOBAL_ATU_OP_BUSY);
 }
 
-static int _mv88e6xxx_phy_read_indirect(struct mv88e6xxx_priv_state *ps,
-					int addr, int regnum)
+static int _mv88e6xxx_mdio_read_indirect(struct mv88e6xxx_priv_state *ps,
+					 int addr, int regnum)
 {
 	int ret;
 
@@ -1087,7 +1088,7 @@ static int _mv88e6xxx_phy_read_indirect(struct mv88e6xxx_priv_state *ps,
 	if (ret < 0)
 		return ret;
 
-	ret = _mv88e6xxx_phy_wait(ps);
+	ret = _mv88e6xxx_mdio_wait(ps);
 	if (ret < 0)
 		return ret;
 
@@ -1096,8 +1097,8 @@ static int _mv88e6xxx_phy_read_indirect(struct mv88e6xxx_priv_state *ps,
 	return ret;
 }
 
-static int _mv88e6xxx_phy_write_indirect(struct mv88e6xxx_priv_state *ps,
-					 int addr, int regnum, u16 val)
+static int _mv88e6xxx_mdio_write_indirect(struct mv88e6xxx_priv_state *ps,
+					  int addr, int regnum, u16 val)
 {
 	int ret;
 
@@ -1109,7 +1110,7 @@ static int _mv88e6xxx_phy_write_indirect(struct mv88e6xxx_priv_state *ps,
 				   GLOBAL2_SMI_OP_22_WRITE | (addr << 5) |
 				   regnum);
 
-	return _mv88e6xxx_phy_wait(ps);
+	return _mv88e6xxx_mdio_wait(ps);
 }
 
 static int mv88e6xxx_get_eee(struct dsa_switch *ds, int port,
@@ -1123,7 +1124,7 @@ static int mv88e6xxx_get_eee(struct dsa_switch *ds, int port,
 
 	mutex_lock(&ps->smi_mutex);
 
-	reg = _mv88e6xxx_phy_read_indirect(ps, port, 16);
+	reg = _mv88e6xxx_mdio_read_indirect(ps, port, 16);
 	if (reg < 0)
 		goto out;
 
@@ -1154,7 +1155,7 @@ static int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
 
 	mutex_lock(&ps->smi_mutex);
 
-	ret = _mv88e6xxx_phy_read_indirect(ps, port, 16);
+	ret = _mv88e6xxx_mdio_read_indirect(ps, port, 16);
 	if (ret < 0)
 		goto out;
 
@@ -1164,7 +1165,7 @@ static int mv88e6xxx_set_eee(struct dsa_switch *ds, int port,
 	if (e->tx_lpi_enabled)
 		reg |= 0x0100;
 
-	ret = _mv88e6xxx_phy_write_indirect(ps, port, 16, reg);
+	ret = _mv88e6xxx_mdio_write_indirect(ps, port, 16, reg);
 out:
 	mutex_unlock(&ps->smi_mutex);
 
@@ -2547,34 +2548,34 @@ static void mv88e6xxx_port_bridge_leave(struct dsa_switch *ds, int port)
 	mutex_unlock(&ps->smi_mutex);
 }
 
-static int _mv88e6xxx_phy_page_write(struct mv88e6xxx_priv_state *ps,
-				     int port, int page, int reg, int val)
+static int _mv88e6xxx_mdio_page_write(struct mv88e6xxx_priv_state *ps,
+				      int port, int page, int reg, int val)
 {
 	int ret;
 
-	ret = _mv88e6xxx_phy_write_indirect(ps, port, 0x16, page);
+	ret = _mv88e6xxx_mdio_write_indirect(ps, port, 0x16, page);
 	if (ret < 0)
 		goto restore_page_0;
 
-	ret = _mv88e6xxx_phy_write_indirect(ps, port, reg, val);
+	ret = _mv88e6xxx_mdio_write_indirect(ps, port, reg, val);
 restore_page_0:
-	_mv88e6xxx_phy_write_indirect(ps, port, 0x16, 0x0);
+	_mv88e6xxx_mdio_write_indirect(ps, port, 0x16, 0x0);
 
 	return ret;
 }
 
-static int _mv88e6xxx_phy_page_read(struct mv88e6xxx_priv_state *ps,
-				    int port, int page, int reg)
+static int _mv88e6xxx_mdio_page_read(struct mv88e6xxx_priv_state *ps,
+				     int port, int page, int reg)
 {
 	int ret;
 
-	ret = _mv88e6xxx_phy_write_indirect(ps, port, 0x16, page);
+	ret = _mv88e6xxx_mdio_write_indirect(ps, port, 0x16, page);
 	if (ret < 0)
 		goto restore_page_0;
 
-	ret = _mv88e6xxx_phy_read_indirect(ps, port, reg);
+	ret = _mv88e6xxx_mdio_read_indirect(ps, port, reg);
 restore_page_0:
-	_mv88e6xxx_phy_write_indirect(ps, port, 0x16, 0x0);
+	_mv88e6xxx_mdio_write_indirect(ps, port, 0x16, 0x0);
 
 	return ret;
 }
@@ -2645,16 +2646,16 @@ static int mv88e6xxx_power_on_serdes(struct mv88e6xxx_priv_state *ps)
 {
 	int ret;
 
-	ret = _mv88e6xxx_phy_page_read(ps, REG_FIBER_SERDES, PAGE_FIBER_SERDES,
-				       MII_BMCR);
+	ret = _mv88e6xxx_mdio_page_read(ps, REG_FIBER_SERDES,
+					PAGE_FIBER_SERDES, MII_BMCR);
 	if (ret < 0)
 		return ret;
 
 	if (ret & BMCR_PDOWN) {
 		ret &= ~BMCR_PDOWN;
-		ret = _mv88e6xxx_phy_page_write(ps, REG_FIBER_SERDES,
-						PAGE_FIBER_SERDES, MII_BMCR,
-						ret);
+		ret = _mv88e6xxx_mdio_page_write(ps, REG_FIBER_SERDES,
+						 PAGE_FIBER_SERDES, MII_BMCR,
+						 ret);
 	}
 
 	return ret;
@@ -3130,13 +3131,11 @@ static int mv88e6xxx_setup(struct dsa_switch *ds)
 	int i;
 
 	ps->ds = ds;
+	ds->slave_mii_bus = ps->mdio_bus;
 
 	if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_EEPROM))
 		mutex_init(&ps->eeprom_mutex);
 
-	if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_PPU))
-		mv88e6xxx_ppu_state_init(ps);
-
 	mutex_lock(&ps->smi_mutex);
 
 	err = mv88e6xxx_switch_reset(ps);
@@ -3159,26 +3158,27 @@ unlock:
 	return err;
 }
 
-int mv88e6xxx_phy_page_read(struct dsa_switch *ds, int port, int page, int reg)
+int mv88e6xxx_mdio_page_read(struct dsa_switch *ds, int port, int page,
+			     int reg)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_phy_page_read(ps, port, page, reg);
+	ret = _mv88e6xxx_mdio_page_read(ps, port, page, reg);
 	mutex_unlock(&ps->smi_mutex);
 
 	return ret;
 }
 
-int mv88e6xxx_phy_page_write(struct dsa_switch *ds, int port, int page,
-			     int reg, int val)
+int mv88e6xxx_mdio_page_write(struct dsa_switch *ds, int port, int page,
+			      int reg, int val)
 {
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 	int ret;
 
 	mutex_lock(&ps->smi_mutex);
-	ret = _mv88e6xxx_phy_page_write(ps, port, page, reg, val);
+	ret = _mv88e6xxx_mdio_page_write(ps, port, page, reg, val);
 	mutex_unlock(&ps->smi_mutex);
 
 	return ret;
@@ -3192,9 +3192,9 @@ static int mv88e6xxx_port_to_phy_addr(struct mv88e6xxx_priv_state *ps,
 	return -EINVAL;
 }
 
-static int mv88e6xxx_phy_read(struct dsa_switch *ds, int port, int regnum)
+static int mv88e6xxx_mdio_read(struct mii_bus *bus, int port, int regnum)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_priv_state *ps = bus->priv;
 	int addr = mv88e6xxx_port_to_phy_addr(ps, port);
 	int ret;
 
@@ -3204,20 +3204,20 @@ static int mv88e6xxx_phy_read(struct dsa_switch *ds, int port, int regnum)
 	mutex_lock(&ps->smi_mutex);
 
 	if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_PPU))
-		ret = mv88e6xxx_phy_read_ppu(ps, addr, regnum);
+		ret = mv88e6xxx_mdio_read_ppu(ps, addr, regnum);
 	else if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_SMI_PHY))
-		ret = _mv88e6xxx_phy_read_indirect(ps, addr, regnum);
+		ret = _mv88e6xxx_mdio_read_indirect(ps, addr, regnum);
 	else
-		ret = _mv88e6xxx_phy_read(ps, addr, regnum);
+		ret = _mv88e6xxx_mdio_read(ps, addr, regnum);
 
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
 
-static int mv88e6xxx_phy_write(struct dsa_switch *ds, int port, int regnum,
-			       u16 val)
+static int mv88e6xxx_mdio_write(struct mii_bus *bus, int port, int regnum,
+				u16 val)
 {
-	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
+	struct mv88e6xxx_priv_state *ps = bus->priv;
 	int addr = mv88e6xxx_port_to_phy_addr(ps, port);
 	int ret;
 
@@ -3227,16 +3227,76 @@ static int mv88e6xxx_phy_write(struct dsa_switch *ds, int port, int regnum,
 	mutex_lock(&ps->smi_mutex);
 
 	if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_PPU))
-		ret = mv88e6xxx_phy_write_ppu(ps, addr, regnum, val);
+		ret = mv88e6xxx_mdio_write_ppu(ps, addr, regnum, val);
 	else if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_SMI_PHY))
-		ret = _mv88e6xxx_phy_write_indirect(ps, addr, regnum, val);
+		ret = _mv88e6xxx_mdio_write_indirect(ps, addr, regnum, val);
 	else
-		ret = _mv88e6xxx_phy_write(ps, addr, regnum, val);
+		ret = _mv88e6xxx_mdio_write(ps, addr, regnum, val);
 
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
 
+static int mv88e6xxx_mdio_register(struct mv88e6xxx_priv_state *ps,
+				   struct device_node *np)
+{
+	static int index;
+	struct mii_bus *bus;
+	int err;
+
+	if (mv88e6xxx_has(ps, MV88E6XXX_FLAG_PPU))
+		mv88e6xxx_ppu_state_init(ps);
+
+	if (np)
+		ps->mdio_np = of_get_child_by_name(np, "mdio");
+
+	bus = devm_mdiobus_alloc(ps->dev);
+	if (!bus)
+		return -ENOMEM;
+
+	bus->priv = (void *)ps;
+	if (np) {
+		bus->name = np->full_name;
+		snprintf(bus->id, MII_BUS_ID_SIZE, "%s", np->full_name);
+	} else {
+		bus->name = "mv88e6xxx SMI";
+		snprintf(bus->id, MII_BUS_ID_SIZE, "mv88e6xxx-%d", index++);
+	}
+
+	bus->read = mv88e6xxx_mdio_read;
+	bus->write = mv88e6xxx_mdio_write;
+	bus->parent = ps->dev;
+
+	if (ps->mdio_np)
+		err = of_mdiobus_register(bus, ps->mdio_np);
+	else
+		err = mdiobus_register(bus);
+	if (err) {
+		dev_err(ps->dev, "Cannot register MDIO bus (%d)\n", err);
+		goto out;
+	}
+	ps->mdio_bus = bus;
+
+	return 0;
+
+out:
+	if (ps->mdio_np)
+		of_node_put(ps->mdio_np);
+
+	return err;
+}
+
+static void mv88e6xxx_mdio_unregister(struct mv88e6xxx_priv_state *ps)
+
+{
+	struct mii_bus *bus = ps->mdio_bus;
+
+	mdiobus_unregister(bus);
+
+	if (ps->mdio_np)
+		of_node_put(ps->mdio_np);
+}
+
 #ifdef CONFIG_NET_DSA_HWMON
 
 static int mv88e61xx_get_temp(struct dsa_switch *ds, int *temp)
@@ -3249,37 +3309,37 @@ static int mv88e61xx_get_temp(struct dsa_switch *ds, int *temp)
 
 	mutex_lock(&ps->smi_mutex);
 
-	ret = _mv88e6xxx_phy_write(ps, 0x0, 0x16, 0x6);
+	ret = _mv88e6xxx_mdio_write(ps, 0x0, 0x16, 0x6);
 	if (ret < 0)
 		goto error;
 
 	/* Enable temperature sensor */
-	ret = _mv88e6xxx_phy_read(ps, 0x0, 0x1a);
+	ret = _mv88e6xxx_mdio_read(ps, 0x0, 0x1a);
 	if (ret < 0)
 		goto error;
 
-	ret = _mv88e6xxx_phy_write(ps, 0x0, 0x1a, ret | (1 << 5));
+	ret = _mv88e6xxx_mdio_write(ps, 0x0, 0x1a, ret | (1 << 5));
 	if (ret < 0)
 		goto error;
 
 	/* Wait for temperature to stabilize */
 	usleep_range(10000, 12000);
 
-	val = _mv88e6xxx_phy_read(ps, 0x0, 0x1a);
+	val = _mv88e6xxx_mdio_read(ps, 0x0, 0x1a);
 	if (val < 0) {
 		ret = val;
 		goto error;
 	}
 
 	/* Disable temperature sensor */
-	ret = _mv88e6xxx_phy_write(ps, 0x0, 0x1a, ret & ~(1 << 5));
+	ret = _mv88e6xxx_mdio_write(ps, 0x0, 0x1a, ret & ~(1 << 5));
 	if (ret < 0)
 		goto error;
 
 	*temp = ((val & 0x1f) - 5) * 5;
 
 error:
-	_mv88e6xxx_phy_write(ps, 0x0, 0x16, 0x0);
+	_mv88e6xxx_mdio_write(ps, 0x0, 0x16, 0x0);
 	mutex_unlock(&ps->smi_mutex);
 	return ret;
 }
@@ -3292,7 +3352,7 @@ static int mv88e63xx_get_temp(struct dsa_switch *ds, int *temp)
 
 	*temp = 0;
 
-	ret = mv88e6xxx_phy_page_read(ds, phy, 6, 27);
+	ret = mv88e6xxx_mdio_page_read(ds, phy, 6, 27);
 	if (ret < 0)
 		return ret;
 
@@ -3325,7 +3385,7 @@ static int mv88e6xxx_get_temp_limit(struct dsa_switch *ds, int *temp)
 
 	*temp = 0;
 
-	ret = mv88e6xxx_phy_page_read(ds, phy, 6, 26);
+	ret = mv88e6xxx_mdio_page_read(ds, phy, 6, 26);
 	if (ret < 0)
 		return ret;
 
@@ -3343,12 +3403,12 @@ static int mv88e6xxx_set_temp_limit(struct dsa_switch *ds, int temp)
 	if (!mv88e6xxx_has(ps, MV88E6XXX_FLAG_TEMP_LIMIT))
 		return -EOPNOTSUPP;
 
-	ret = mv88e6xxx_phy_page_read(ds, phy, 6, 26);
+	ret = mv88e6xxx_mdio_page_read(ds, phy, 6, 26);
 	if (ret < 0)
 		return ret;
 	temp = clamp_val(DIV_ROUND_CLOSEST(temp, 5) + 5, 0, 0x1f);
-	return mv88e6xxx_phy_page_write(ds, phy, 6, 26,
-					(ret & 0xe0ff) | (temp << 8));
+	return mv88e6xxx_mdio_page_write(ds, phy, 6, 26,
+					 (ret & 0xe0ff) | (temp << 8));
 }
 
 static int mv88e6xxx_get_temp_alarm(struct dsa_switch *ds, bool *alarm)
@@ -3362,7 +3422,7 @@ static int mv88e6xxx_get_temp_alarm(struct dsa_switch *ds, bool *alarm)
 
 	*alarm = false;
 
-	ret = mv88e6xxx_phy_page_read(ds, phy, 6, 26);
+	ret = mv88e6xxx_mdio_page_read(ds, phy, 6, 26);
 	if (ret < 0)
 		return ret;
 
@@ -3549,6 +3609,7 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 	struct mii_bus *bus;
 	const char *name;
 	int id, prod_num, rev;
+	int err;
 
 	bus = dsa_host_dev_to_mii_bus(host_dev);
 	if (!bus)
@@ -3575,8 +3636,13 @@ static const char *mv88e6xxx_drv_probe(struct device *dsa_dev,
 	ps->bus = bus;
 	ps->sw_addr = sw_addr;
 	ps->info = info;
+	ps->dev = dsa_dev;
 	mutex_init(&ps->smi_mutex);
 
+	err = mv88e6xxx_mdio_register(ps, NULL);
+	if (err)
+		return NULL;
+
 	*priv = ps;
 
 	dev_info(&ps->bus->dev, "switch 0x%x probed: %s, revision %u\n",
@@ -3590,8 +3656,6 @@ struct dsa_switch_driver mv88e6xxx_switch_driver = {
 	.probe			= mv88e6xxx_drv_probe,
 	.setup			= mv88e6xxx_setup,
 	.set_addr		= mv88e6xxx_set_addr,
-	.phy_read		= mv88e6xxx_phy_read,
-	.phy_write		= mv88e6xxx_phy_write,
 	.adjust_link		= mv88e6xxx_adjust_link,
 	.get_strings		= mv88e6xxx_get_strings,
 	.get_ethtool_stats	= mv88e6xxx_get_ethtool_stats,
@@ -3677,6 +3741,12 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 	    !of_property_read_u32(np, "eeprom-length", &eeprom_len))
 		ps->eeprom_len = eeprom_len;
 
+	err = mv88e6xxx_mdio_register(ps, mdiodev->dev.of_node);
+	if (err)
+		return err;
+
+	ds->slave_mii_bus = ps->mdio_bus;
+
 	dev_set_drvdata(dev, ds);
 
 	dev_info(dev, "switch 0x%x probed: %s, revision %u\n",
@@ -3691,6 +3761,8 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 
 	put_device(&ps->bus->dev);
+
+	mv88e6xxx_mdio_unregister(ps);
 }
 
 static const struct of_device_id mv88e6xxx_of_match[] = {
diff --git a/drivers/net/dsa/mv88e6xxx.h b/drivers/net/dsa/mv88e6xxx.h
index 36d0e1504de1..8221c3c7ec5a 100644
--- a/drivers/net/dsa/mv88e6xxx.h
+++ b/drivers/net/dsa/mv88e6xxx.h
@@ -600,6 +600,12 @@ struct mv88e6xxx_priv_state {
 
 	/* set to size of eeprom if supported by the switch */
 	int		eeprom_len;
+
+	/* Device node for the MDIO bus */
+	struct device_node *mdio_np;
+
+	/* And the MDIO bus itself */
+	struct mii_bus *mdio_bus;
 };
 
 enum stat_type {
-- 
2.8.1

Re: [RFC PATCH 13/16] net: dsa: mv88e6xxx: Refactor MDIO so driver registers mdio bus

[Vivien Didelot]

Hi Andrew,

I like the s/phy/mdio/ renaming, but please move it in its own
non-functional patch for easier reviewing.

Also, we may want to use this opportunity to get rid of some _ prefixed
functions. It is hard to following the distinction between these two
following signatures:

    int _mv88e6xxx_mdio_read(struct mv88e6xxx_priv_state *ps, int addr, int reg_num)

and

    int mv88e6xxx_mdio_read(struct mii_bus *bus, int port, int regnum)

(which take different first argument.)

It would be great to introduce here the two low-level, bus-agnostic,
SMI unlocked functions: mv88e6xxx_read() and mv88e6xxx_write().

Thanks,

        Vivien

[RFC PATCH 14/16] net: dsa: Add new binding implementation

[Andrew Lunn]

The existing DSA binding has a number of limitations and problems. The
main problem is that it cannot represent a switch as a linux device,
hanging off some bus. It is limited to one CPU port. The DSA platform
device is artificial, and does not really represent hardware.

Implement a new binding which can be embedded into any type of node on
a bus to represent one switch device, and its links to other switches.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx.c |   7 +
 include/net/dsa.h           |  20 ++
 net/dsa/Makefile            |   2 +-
 net/dsa/dsa.c               |   1 +
 net/dsa/dsa2.c              | 653 ++++++++++++++++++++++++++++++++++++++++++++
 net/dsa/dsa_priv.h          |   2 +-
 net/dsa/slave.c             |   8 +-
 7 files changed, 689 insertions(+), 4 deletions(-)
 create mode 100644 net/dsa/dsa2.c

diff --git a/drivers/net/dsa/mv88e6xxx.c b/drivers/net/dsa/mv88e6xxx.c
index 8fbc771f0475..a6ccfdfbe225 100644
--- a/drivers/net/dsa/mv88e6xxx.c
+++ b/drivers/net/dsa/mv88e6xxx.c
@@ -3749,6 +3749,12 @@ int mv88e6xxx_probe(struct mdio_device *mdiodev)
 
 	dev_set_drvdata(dev, ds);
 
+	err = dsa_register_switch(ds, mdiodev->dev.of_node);
+	if (err) {
+		mv88e6xxx_mdio_unregister(ps);
+		return err;
+	}
+
 	dev_info(dev, "switch 0x%x probed: %s, revision %u\n",
 		 prod_num, ps->info->name, rev);
 
@@ -3760,6 +3766,7 @@ static void mv88e6xxx_remove(struct mdio_device *mdiodev)
 	struct dsa_switch *ds = dev_get_drvdata(&mdiodev->dev);
 	struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 
+	dsa_unregister_switch(ds);
 	put_device(&ps->bus->dev);
 
 	mv88e6xxx_mdio_unregister(ps);
diff --git a/include/net/dsa.h b/include/net/dsa.h
index adb75422bc6c..032f6efa4b3e 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -86,6 +86,17 @@ struct dsa_platform_data {
 struct packet_type;
 
 struct dsa_switch_tree {
+	struct list_head	list;
+
+	/* Tree identifier */
+	u32 tree;
+
+	/* Number of switches attached to this tree */
+	struct kref refcount;
+
+	/* Has this tree been applied to the hardware? */
+	bool applied;
+
 	/*
 	 * Configuration data for the platform device that owns
 	 * this dsa switch tree instance.
@@ -172,9 +183,15 @@ struct dsa_switch {
 #endif
 
 	/*
+	 * The lower device this switch uses to talk to the host
+	 */
+	struct net_device *master_netdev;
+
+	/*
 	 * Slave mii_bus and devices for the individual ports.
 	 */
 	u32			dsa_port_mask;
+	u32			cpu_port_mask;
 	u32			enabled_port_mask;
 	u32			phys_mii_mask;
 	struct dsa_port		ports[DSA_MAX_PORTS];
@@ -363,4 +380,7 @@ static inline bool dsa_uses_tagged_protocol(struct dsa_switch_tree *dst)
 {
 	return dst->rcv != NULL;
 }
+
+void dsa_unregister_switch(struct dsa_switch *ds);
+int dsa_register_switch(struct dsa_switch *ds, struct device_node *np);
 #endif
diff --git a/net/dsa/Makefile b/net/dsa/Makefile
index da06ed1df620..8af4ded70f1c 100644
--- a/net/dsa/Makefile
+++ b/net/dsa/Makefile
@@ -1,6 +1,6 @@
 # the core
 obj-$(CONFIG_NET_DSA) += dsa_core.o
-dsa_core-y += dsa.o slave.o
+dsa_core-y += dsa.o slave.o dsa2.o
 
 # tagging formats
 dsa_core-$(CONFIG_NET_DSA_TAG_BRCM) += tag_brcm.o
diff --git a/net/dsa/dsa.c b/net/dsa/dsa.c
index b1787e2f4bb3..b3cada3ecab7 100644
--- a/net/dsa/dsa.c
+++ b/net/dsa/dsa.c
@@ -294,6 +294,7 @@ static int dsa_switch_setup_one(struct dsa_switch *ds, struct device *parent)
 			}
 			dst->cpu_switch = index;
 			dst->cpu_port = i;
+			ds->cpu_port_mask |= 1 << i;
 		} else if (!strcmp(name, "dsa")) {
 			ds->dsa_port_mask |= 1 << i;
 		} else {
diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
new file mode 100644
index 000000000000..4e5051bed643
--- /dev/null
+++ b/net/dsa/dsa2.c
@@ -0,0 +1,653 @@
+/*
+ * net/dsa/dsa2.c - Hardware switch handling, binding version 2
+ * Copyright (c) 2008-2009 Marvell Semiconductor
+ * Copyright (c) 2013 Florian Fainelli <florian@openwrt.org>
+ * Copyright (c) 2016 Andrew Lunn <andrew@lunn.ch>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/list.h>
+#include <linux/slab.h>
+#include <linux/rtnetlink.h>
+#include <net/dsa.h>
+#include <linux/of.h>
+#include <linux/of_net.h>
+#include "dsa_priv.h"
+
+static LIST_HEAD(dsa_switch_trees);
+static DEFINE_MUTEX(dsa2_mutex);
+
+static struct dsa_switch_tree *dsa_get_dst(u32 tree)
+{
+	struct dsa_switch_tree *dst;
+
+	list_for_each_entry(dst, &dsa_switch_trees, list)
+		if (dst->tree == tree)
+			return dst;
+	return NULL;
+}
+
+static void dsa_free_dst(struct kref *ref)
+{
+	struct dsa_switch_tree *dst = container_of(ref, struct dsa_switch_tree,
+						   refcount);
+
+	list_del(&dst->list);
+	kfree(dst);
+}
+
+static void dsa_put_dst(struct dsa_switch_tree *dst)
+{
+	kref_put(&dst->refcount, dsa_free_dst);
+}
+
+static struct dsa_switch_tree *dsa_add_dst(u32 tree)
+{
+	struct dsa_switch_tree *dst;
+
+	dst = kzalloc(sizeof(*dst), GFP_KERNEL);
+	if (!dst)
+		return NULL;
+	dst->tree = tree;
+	dst->cpu_switch = -1;
+	INIT_LIST_HEAD(&dst->list);
+	list_add_tail(&dsa_switch_trees, &dst->list);
+	kref_init(&dst->refcount);
+
+	return dst;
+}
+
+static void dsa_dst_add_ds(struct dsa_switch_tree *dst,
+			   struct dsa_switch *ds, u32 index)
+{
+	kref_get(&dst->refcount);
+	dst->ds[index] = ds;
+}
+
+static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
+			   struct dsa_switch *ds, u32 index)
+{
+	dst->ds[index] = NULL;
+	kref_put(&dst->refcount, dsa_free_dst);
+}
+
+static bool dsa_port_is_dsa(struct device_node *port)
+{
+	const char *name;
+
+	name = of_get_property(port, "label", NULL);
+	if (!name)
+		return false;
+
+	if (!strcmp(name, "dsa"))
+		return true;
+
+	return false;
+}
+
+static bool dsa_port_is_cpu(struct device_node *port)
+{
+	const char *name;
+
+	name = of_get_property(port, "label", NULL);
+	if (!name)
+		return false;
+
+	if (!strcmp(name, "cpu"))
+		return true;
+
+	return false;
+}
+
+static bool dsa_ds_find_port(struct dsa_switch *ds,
+			     struct device_node *port)
+{
+	u32 index;
+
+	for (index = 0; index < DSA_MAX_PORTS; index++)
+		if (ds->ports[index].dn == port)
+			return true;
+	return false;
+}
+
+static struct dsa_switch *dsa_dst_find_port(struct dsa_switch_tree *dst,
+					    struct device_node *port)
+{
+	struct dsa_switch *ds;
+	u32 index;
+
+	for (index = 0; index < DSA_MAX_SWITCHES; index++) {
+		ds = dst->ds[index];
+		if (!ds)
+			continue;
+
+		if (dsa_ds_find_port(ds, port))
+			return ds;
+	}
+
+	return NULL;
+}
+
+static int dsa_port_complete(struct dsa_switch_tree *dst,
+			     struct dsa_switch *src_ds,
+			     struct device_node *port,
+			     u32 src_port)
+{
+	struct device_node *link;
+	int index;
+	struct dsa_switch *dst_ds;
+
+	for (index = 0;; index++) {
+		link = of_parse_phandle(port, "link", index);
+		if (!link)
+			break;
+
+		dst_ds = dsa_dst_find_port(dst, link);
+		of_node_put(link);
+
+		if (!dst_ds)
+			return 1;
+
+		src_ds->rtable[dst_ds->index] = src_port;
+	}
+
+	return 0;
+}
+
+/* A switch is complete if all the DSA ports phandles point to ports
+ * known in the tree. A return value of 1 means the tree is not
+ * complete. This is not an error condition. A value of 0 is
+ * success.
+ */
+static int dsa_ds_complete(struct dsa_switch_tree *dst, struct dsa_switch *ds)
+{
+	struct device_node *port;
+	u32 index;
+	int err;
+
+	for (index = 0; index < DSA_MAX_PORTS; index++) {
+		port = ds->ports[index].dn;
+		if (!port)
+			continue;
+
+		if (!dsa_port_is_dsa(port))
+			continue;
+
+		ds->dsa_port_mask |= 1 << index;
+
+		err = dsa_port_complete(dst, ds, port, index);
+		if (err != 0)
+			return err;
+	}
+
+	return 0;
+}
+
+/* A tree is complete if all the DSA ports phandles point to ports
+ * known in the tree. A return value of 1 means the tree is not
+ * complete. This is not an error condition. A value of 0 is
+ * success.
+ */
+static int dsa_dst_complete(struct dsa_switch_tree *dst)
+{
+	struct dsa_switch *ds;
+	u32 index;
+	int err;
+
+	for (index = 0; index < DSA_MAX_SWITCHES; index++) {
+		ds = dst->ds[index];
+		if (!ds)
+			continue;
+
+		err = dsa_ds_complete(dst, ds);
+		if (err != 0)
+			return err;
+	}
+
+	return 0;
+}
+
+static int dsa_dsa_port_apply(struct device_node *port, u32 index,
+			      struct dsa_switch *ds)
+{
+	int err;
+
+	err = dsa_cpu_dsa_setup(ds, ds->dev, port, index);
+	if (err) {
+		dev_warn(ds->dev, "Failed to setup dsa port %d: %d\n",
+			 index, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static void dsa_dsa_port_unapply(struct device_node *port, u32 index,
+				 struct dsa_switch *ds)
+{
+	dsa_cpu_dsa_destroy(port);
+}
+
+static int dsa_cpu_port_apply(struct device_node *port, u32 index,
+			      struct dsa_switch *ds)
+{
+	int err;
+
+	err = dsa_cpu_dsa_setup(ds, ds->dev, port, index);
+	if (err) {
+		dev_warn(ds->dev, "Failed to setup cpu port %d: %d\n",
+			 index, err);
+		return err;
+	}
+
+	ds->cpu_port_mask |= 1 << index;
+
+	return 0;
+}
+
+static void dsa_cpu_port_unapply(struct device_node *port, u32 index,
+				 struct dsa_switch *ds)
+{
+	dsa_cpu_dsa_destroy(port);
+}
+
+static int dsa_user_port_apply(struct device_node *port, u32 index,
+			       struct dsa_switch *ds)
+{
+	const char *name;
+	int err;
+
+	name = of_get_property(port, "label", NULL);
+
+	err = dsa_slave_create(ds, ds->dev, index, name);
+	if (err) {
+		dev_warn(ds->dev, "Failed to create slave %d: %d\n",
+			 index, err);
+		return err;
+	}
+
+	return 0;
+}
+
+static void dsa_user_port_unapply(struct device_node *port, u32 index,
+				  struct dsa_switch *ds)
+{
+	if (ds->ports[index].netdev) {
+		dsa_slave_destroy(ds->ports[index].netdev);
+		ds->ports[index].netdev = NULL;
+	}
+}
+
+static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
+{
+	struct device_node *port;
+	u32 index;
+	int err;
+
+	err = ds->drv->setup(ds);
+	if (err < 0)
+		return err;
+
+	err = ds->drv->set_addr(ds, dst->master_netdev->dev_addr);
+	if (err < 0)
+		return err;
+
+	err = ds->drv->set_addr(ds, dst->master_netdev->dev_addr);
+	if (err < 0)
+		return err;
+
+	for (index = 0; index < DSA_MAX_PORTS; index++) {
+		port = ds->ports[index].dn;
+		if (!port)
+			continue;
+
+		if (dsa_port_is_dsa(port)) {
+			err = dsa_dsa_port_apply(port, index, ds);
+			if (err)
+				return err;
+			continue;
+		}
+
+		if (dsa_port_is_cpu(port)) {
+			err = dsa_cpu_port_apply(port, index, ds);
+			if (err)
+				return err;
+			continue;
+		}
+
+		err = dsa_user_port_apply(port, index, ds);
+		if (err)
+			continue;
+	}
+
+	return 0;
+}
+
+static void dsa_ds_unapply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
+{
+	struct device_node *port;
+	u32 index;
+
+	for (index = 0; index < DSA_MAX_PORTS; index++) {
+		port = ds->ports[index].dn;
+		if (!port)
+			continue;
+
+		if (dsa_port_is_dsa(port)) {
+			dsa_dsa_port_unapply(port, index, ds);
+			continue;
+		}
+
+		if (dsa_port_is_cpu(port)) {
+			dsa_cpu_port_unapply(port, index, ds);
+			continue;
+		}
+
+		dsa_user_port_unapply(port, index, ds);
+	}
+}
+
+static int dsa_dst_apply(struct dsa_switch_tree *dst)
+{
+	struct dsa_switch *ds;
+	u32 index;
+	int err;
+
+	for (index = 0; index < DSA_MAX_SWITCHES; index++) {
+		ds = dst->ds[index];
+		if (!ds)
+			continue;
+
+		err = dsa_ds_apply(dst, ds);
+		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.
+	 */
+	wmb();
+	dst->master_netdev->dsa_ptr = (void *)dst;
+	dst->applied = true;
+
+	return 0;
+}
+
+static void dsa_dst_unapply(struct dsa_switch_tree *dst)
+{
+	struct dsa_switch *ds;
+	u32 index;
+
+	if (!dst->applied)
+		return;
+
+	dst->master_netdev->dsa_ptr = NULL;
+
+	/* If we used a tagging format that doesn't have an ethertype
+	 * field, make sure that all packets from this point get sent
+	 * without the tag and go through the regular receive path.
+	 */
+	wmb();
+
+	for (index = 0; index < DSA_MAX_SWITCHES; index++) {
+		ds = dst->ds[index];
+		if (!ds)
+			continue;
+
+		dsa_ds_unapply(dst, ds);
+	}
+
+	pr_info("DSA: tree %d unapplied\n", dst->tree);
+	dst->applied = false;
+}
+
+static int dsa_cpu_parse(struct device_node *port, u32 index,
+			 struct dsa_switch_tree *dst,
+			 struct dsa_switch *ds)
+{
+	struct net_device *ethernet_dev;
+	struct device_node *ethernet;
+
+	ethernet = of_parse_phandle(port, "ethernet", 0);
+	if (!ethernet)
+		return -EINVAL;
+
+	ethernet_dev = of_find_net_device_by_node(ethernet);
+	if (!ethernet_dev)
+		return -EPROBE_DEFER;
+
+	if (!ds->master_netdev)
+		ds->master_netdev = ethernet_dev;
+
+	if (!dst->master_netdev)
+		dst->master_netdev = ethernet_dev;
+
+	if (dst->cpu_switch == -1) {
+		dst->cpu_switch = ds->index;
+		dst->cpu_port = index;
+	}
+
+	dst->tag_ops = dsa_resolve_tag_protocol(ds->drv->tag_protocol);
+	if (IS_ERR(dst->tag_ops)) {
+		dev_warn(ds->dev, "No tagger for this switch\n");
+		return PTR_ERR(dst->tag_ops);
+	}
+
+	dst->rcv = dst->tag_ops->rcv;
+
+	return 0;
+}
+
+static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds)
+{
+	struct device_node *port;
+	u32 index;
+	int err;
+
+	for (index = 0; index < DSA_MAX_PORTS; index++) {
+		port = ds->ports[index].dn;
+		if (!port)
+			continue;
+
+		if (dsa_port_is_cpu(port)) {
+			err = dsa_cpu_parse(port, index, dst, ds);
+			if (err)
+				return err;
+		}
+	}
+
+	pr_info("DSA: switch %d %d parsed\n", dst->tree, ds->index);
+
+	return 0;
+}
+
+static int dsa_dst_parse(struct dsa_switch_tree *dst)
+{
+	struct dsa_switch *ds;
+	u32 index;
+	int err;
+
+	for (index = 0; index < DSA_MAX_SWITCHES; index++) {
+		ds = dst->ds[index];
+		if (!ds)
+			continue;
+
+		err = dsa_ds_parse(dst, ds);
+		if (err)
+			return err;
+	}
+
+	if (!dst->master_netdev) {
+		pr_warn("Tree has no master device\n");
+		return -EINVAL;
+	}
+
+	pr_info("DSA: tree %d parsed\n", dst->tree);
+
+	return 0;
+}
+
+static int dsa_parse_ports_dn(struct device_node *ports, struct dsa_switch *ds)
+{
+	struct device_node *port;
+	int err;
+	u32 reg;
+
+	for_each_available_child_of_node(ports, port) {
+		err = of_property_read_u32(port, "reg", &reg);
+		if (err)
+			return err;
+
+		if (reg >= DSA_MAX_PORTS)
+			return -EINVAL;
+
+		ds->ports[reg].dn = port;
+	}
+
+	return 0;
+}
+
+static int dsa_parse_member(struct device_node *np, u32 *tree, u32 *index)
+{
+	int err;
+
+	*tree = *index = 0;
+
+	err = of_property_read_u32_index(np, "dsa,member", 0, tree);
+	if (err) {
+		/* Does not exist, but it is optional */
+		if (err == -EINVAL)
+			return 0;
+		return err;
+	}
+
+	err = of_property_read_u32_index(np, "dsa,member", 1, index);
+	if (err)
+		return err;
+
+	if (*index >= DSA_MAX_SWITCHES)
+		return -EINVAL;
+
+	return 0;
+}
+
+static struct device_node *dsa_get_ports(struct dsa_switch *ds,
+					 struct device_node *np)
+{
+	struct device_node *ports;
+
+	ports = of_get_child_by_name(np, "ports");
+	if (!ports) {
+		dev_err(ds->dev, "no ports child node found\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	return ports;
+}
+
+static int _dsa_register_switch(struct dsa_switch *ds, struct device_node *np)
+{
+	struct device_node *ports = dsa_get_ports(ds, np);
+	struct dsa_switch_tree *dst;
+	u32 tree, index;
+	int err;
+
+	err = dsa_parse_member(np, &tree, &index);
+	if (err)
+		return err;
+
+	if (IS_ERR(ports))
+		return PTR_ERR(ports);
+
+	err = dsa_parse_ports_dn(ports, ds);
+	if (err)
+		return err;
+
+	dst = dsa_get_dst(tree);
+	if (!dst) {
+		dst = dsa_add_dst(tree);
+		if (!dst)
+			return -ENOMEM;
+	}
+
+	if (dst->ds[index]) {
+		err = -EBUSY;
+		goto out;
+	}
+
+	ds->dst = dst;
+	ds->index = index;
+	dsa_dst_add_ds(dst, ds, index);
+
+	err = dsa_dst_complete(dst);
+	if (err < 0)
+		goto out_del_dst;
+
+	if (err == 1) {
+		/* Not all switches registered yet */
+		err = 0;
+		goto out;
+	}
+
+	if (dst->applied) {
+		pr_info("DSA: Disjoint trees?\n");
+		return -EINVAL;
+	}
+
+	err = dsa_dst_parse(dst);
+	if (err)
+		goto out_del_dst;
+
+	err = dsa_dst_apply(dst);
+	if (err) {
+		dsa_dst_unapply(dst);
+		goto out_del_dst;
+	}
+
+	dsa_put_dst(dst);
+	return 0;
+
+out_del_dst:
+	dsa_dst_del_ds(dst, ds, ds->index);
+out:
+	dsa_put_dst(dst);
+
+	return err;
+}
+
+int dsa_register_switch(struct dsa_switch *ds, struct device_node *np)
+{
+	int err;
+
+	mutex_lock(&dsa2_mutex);
+	err = _dsa_register_switch(ds, np);
+	mutex_unlock(&dsa2_mutex);
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(dsa_register_switch);
+
+void _dsa_unregister_switch(struct dsa_switch *ds)
+{
+	struct dsa_switch_tree *dst = ds->dst;
+
+	dsa_dst_unapply(dst);
+
+	dsa_dst_del_ds(dst, ds, ds->index);
+}
+
+void dsa_unregister_switch(struct dsa_switch *ds)
+{
+	mutex_lock(&dsa2_mutex);
+	_dsa_unregister_switch(ds);
+	mutex_unlock(&dsa2_mutex);
+}
+EXPORT_SYMBOL_GPL(dsa_unregister_switch);
+
diff --git a/net/dsa/dsa_priv.h b/net/dsa/dsa_priv.h
index 72f7b8989cfb..b42f1a5f95f3 100644
--- a/net/dsa/dsa_priv.h
+++ b/net/dsa/dsa_priv.h
@@ -59,7 +59,7 @@ const struct dsa_device_ops *dsa_resolve_tag_protocol(int tag_protocol);
 extern const struct dsa_device_ops notag_netdev_ops;
 void dsa_slave_mii_bus_init(struct dsa_switch *ds);
 int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
-		     int port, char *name);
+		     int port, const char *name);
 void dsa_slave_destroy(struct net_device *slave_dev);
 int dsa_slave_suspend(struct net_device *slave_dev);
 int dsa_slave_resume(struct net_device *slave_dev);
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 35e5f0f6688b..15a492261895 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -1099,14 +1099,18 @@ int dsa_slave_resume(struct net_device *slave_dev)
 }
 
 int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
-		     int port, char *name)
+		     int port, const char *name)
 {
-	struct net_device *master = ds->dst->master_netdev;
 	struct dsa_switch_tree *dst = ds->dst;
+	struct net_device *master;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
 	int ret;
 
+	master = ds->dst->master_netdev;
+	if (ds->master_netdev)
+		master = ds->master_netdev;
+
 	slave_dev = alloc_netdev(sizeof(struct dsa_slave_priv), name,
 				 NET_NAME_UNKNOWN, ether_setup);
 	if (slave_dev == NULL)
-- 
2.8.1

Re: [RFC PATCH 14/16] net: dsa: Add new binding implementation

[Vivien Didelot]

Hi Andrew, Florian,

Here again, I'd suggested an implicit namespace for functions taking a
dsa_switch_tree structure as first argument, i.e. dsa_tree_do_foo().

Since we are likely to spend some time in net/dsa, it'd be great to
introduce the new bindings and an intuitive API at the same time ;-)

Examples below.

> +void dsa_unregister_switch(struct dsa_switch *ds);
> +int dsa_register_switch(struct dsa_switch *ds, struct device_node *np);

    void dsa_switch_unregister(struct dsa_switch *ds);
    int dsa_switch_register(struct dsa_switch *ds, struct device_node *np);

> +static struct dsa_switch_tree *dsa_get_dst(u32 tree)
> +static void dsa_free_dst(struct kref *ref)

static struct dsa_switch_tree *dsa_get_tree(u32 tree)
dsa_free_tree()

> +static void dsa_put_dst(struct dsa_switch_tree *dst)

static void dsa_tree_put(struct dsa_switch_tree *dst)

> +static struct dsa_switch_tree *dsa_add_dst(u32 tree)

dsa_add_tree(u32 tree)

> +static void dsa_dst_add_ds(struct dsa_switch_tree *dst,
> +			   struct dsa_switch *ds, u32 index)

static void dsa_tree_add_switch(struct dsa_switch_tree *dst, u32 index,
			        struct dsa_switch *ds)

> +static void dsa_dst_del_ds(struct dsa_switch_tree *dst,
> +			   struct dsa_switch *ds, u32 index)

static void dsa_tree_del_switch(struct dsa_switch_tree *dst, u32 index)

> +static bool dsa_port_is_dsa(struct device_node *port)
> +static bool dsa_port_is_cpu(struct device_node *port)

It would be consistent to have public helpers like:

    bool dsa_port_is_cpu(struct dsa_port *dp);
    bool dsa_port_is_dsa(struct dsa_port *dp);

> +static bool dsa_ds_find_port(struct dsa_switch *ds,
> +			     struct device_node *port)

dsa_switch_find_port_node()

> +static struct dsa_switch *dsa_dst_find_port(struct dsa_switch_tree *dst,
> +					    struct device_node *port)

dsa_tree_find_port_node()

> +static int dsa_port_complete(struct dsa_switch_tree *dst,
> +			     struct dsa_switch *src_ds,
> +			     struct device_node *port,
> +			     u32 src_port)

> +static int dsa_ds_complete(struct dsa_switch_tree *dst, struct dsa_switch *ds)

> +static int dsa_dst_complete(struct dsa_switch_tree *dst)

static int dsa_tree_complete_port(struct dsa_switch_tree *dst,
                                  struct dsa_port *dp)

static int dsa_tree_complete_switch(struct dsa_switch_tree *dst,
                                    struct dsa_switch *ds)

static int dsa_tree_complete(struct dsa_switch_tree *dst)

> +static int dsa_dsa_port_apply(struct device_node *port, u32 index,
> +			      struct dsa_switch *ds)
> +static void dsa_dsa_port_unapply(struct device_node *port, u32 index,
> +				 struct dsa_switch *ds)
> +static int dsa_cpu_port_apply(struct device_node *port, u32 index,
> +			      struct dsa_switch *ds)
> +static void dsa_cpu_port_unapply(struct device_node *port, u32 index,
> +				 struct dsa_switch *ds)
> +static int dsa_user_port_apply(struct device_node *port, u32 index,
> +			       struct dsa_switch *ds)
> +static void dsa_user_port_unapply(struct device_node *port, u32 index,
> +				  struct dsa_switch *ds)

dsa_switch_{un,}apply_{cpu,dsa,user}_port(struct dsa_switch *ds,
                                          struct dsa_port *dp)

> +static int dsa_ds_apply(struct dsa_switch_tree *dst, struct dsa_switch *ds)
> +static void dsa_ds_unapply(struct dsa_switch_tree *dst, struct dsa_switch *ds)

dsa_tree_{un,}apply_switch(dst, ds)

> +static int dsa_dst_apply(struct dsa_switch_tree *dst)
> +static void dsa_dst_unapply(struct dsa_switch_tree *dst)

dsa_tree_{un,}apply(dst)

> +static int dsa_cpu_parse(struct device_node *port, u32 index,
> +			 struct dsa_switch_tree *dst,
> +			 struct dsa_switch *ds)
> +static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds)
> +static int dsa_dst_parse(struct dsa_switch_tree *dst)

dsa_tree_parse(dst)
dsa_tree_parse_switch(dst, ds)
dsa_tree_parse_cpu_port(dst, dp)

> +static int dsa_parse_ports_dn(struct device_node *ports, struct dsa_switch *ds)

dsa_switch_parse_port_nodes(struct dsa_switch *ds, struct device_node *ports)

> +static struct device_node *dsa_get_ports(struct dsa_switch *ds,
> +					 struct device_node *np)

dsa_switch_get_port_nodes(struct dsa_switch *ds, struct device_node *np)

> +static int _dsa_register_switch(struct dsa_switch *ds, struct device_node *np)


                                                     
I think it would be great to introduce the following in
include/net/dsa.h:

    enum dsa_port_type {
        DSA_PORT_TYPE_CPU,
        DSA_PORT_TYPE_DSA,
        DSA_PORT_TYPE_USER,
    };

    struct dsa_port {
        struct dsa_switch       *ds;
        unsigned int            index;
        enum dsa_port_type      type;
        ...
    };

and have port helpers and simpler functions for the framework like:

    bool dsa_port_is_{cpu,dsa,user}(struct dsa_port *dp);
    int dsa_switch_{apply,setup}_port(struct dsa_switch *ds,
                                      struct dsa_port *dp);

Also, since most of the content of this file is tree-wide, why not name
it net/dsa/tree.c instead of dsa2.c?

These are just suggestions. What do you think?

Thanks,

        Vivien

Re: [RFC PATCH 14/16] net: dsa: Add new binding implementation

[Andrew Lunn]

On Fri, May 27, 2016 at 04:39:05PM -0400, Vivien Didelot wrote:
> 
> Hi Andrew, Florian,
> 
> Here again, I'd suggested an implicit namespace for functions taking a
> dsa_switch_tree structure as first argument, i.e. dsa_tree_do_foo().

Using tree actually makes things worse, since tree is never used
anywhere in the current code. It is always called dst. If you do this,
you also need to replace every instance of dst with tree.

We mostly have a good convention

struct dsa_switch *ds;
dsa_switch_tree *dst;

What is not quite consistent is

struct dsa_chip_data *cd

which should really be

struct dsa_chip_data *dcd

but we are consistent with using cd everywhere.
 
> Since we are likely to spend some time in net/dsa, it'd be great to
> introduce the new bindings and an intuitive API at the same time ;-)

They are two separate things. And the binding will be set in stone,
never to be changed again in incompatible ways, where as the API we
can change as much as we like. We should be concentrate on the
binding.

    Andrew

Re: [RFC PATCH 14/16] net: dsa: Add new binding implementation

[Florian Fainelli]

On 05/27/2016 01:57 PM, Andrew Lunn wrote:
> On Fri, May 27, 2016 at 04:39:05PM -0400, Vivien Didelot wrote:
>>
>> Hi Andrew, Florian,
>>
>> Here again, I'd suggested an implicit namespace for functions taking a
>> dsa_switch_tree structure as first argument, i.e. dsa_tree_do_foo().
> 
> Using tree actually makes things worse, since tree is never used
> anywhere in the current code. It is always called dst. If you do this,
> you also need to replace every instance of dst with tree.
> 
> We mostly have a good convention
> 
> struct dsa_switch *ds;
> dsa_switch_tree *dst;
> 
> What is not quite consistent is
> 
> struct dsa_chip_data *cd
> 
> which should really be
> 
> struct dsa_chip_data *dcd
> 
> but we are consistent with using cd everywhere.
>  
>> Since we are likely to spend some time in net/dsa, it'd be great to
>> introduce the new bindings and an intuitive API at the same time ;-)
> 
> They are two separate things. And the binding will be set in stone,
> never to be changed again in incompatible ways, where as the API we
> can change as much as we like. We should be concentrate on the
> binding.

I completely agree, Andrew has been working long enough on this it needs
to be posted and merged, anything that becomes a clean up on top of that
can be done at any random time, by you, me, or any kernel janitor who
feels like it.

We need to set priorities here, and the highest priority is to get these
patches accepted to enable more people to utilize DSA, so once we have
more devices we can get to a longer term plan to get a better
abstraction model for the switch devices that are in the source tree.

Considering the fast pace nature of the net-next tree, I am sure that
any kind of cleanup on this code after the patches get merged would be
merged in a matter of weeks, but it does not strike me as being a
blocker here.

Thanks
-- 
Florian

Re: [RFC PATCH 14/16] net: dsa: Add new binding implementation

[Richard Cochran]

On Fri, May 27, 2016 at 02:29:04PM -0700, Florian Fainelli wrote:
> We need to set priorities here, and the highest priority is to get these
> patches accepted to enable more people to utilize DSA, so once we have
> more devices we can get to a longer term plan to get a better
> abstraction model for the switch devices that are in the source tree.

+1

Thanks,
Richard

[RFC PATCH 15/16] arm: dt: vf610-zii-devel-b: Make use of new DSA binding

[Andrew Lunn]

Hang the three switches of the three MDIO busses using the new DSA
binding. Also, make use of the mdio-bus and explicitly list the phys
on one device. This is not required, but good for testing.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
---
 arch/arm/boot/dts/vf610-zii-dev-rev-b.dts | 328 ++++++++++++++++--------------
 1 file changed, 170 insertions(+), 158 deletions(-)

diff --git a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
index 6c60b7f91104..5c1fcab4a6f7 100644
--- a/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
+++ b/arch/arm/boot/dts/vf610-zii-dev-rev-b.dts
@@ -85,187 +85,199 @@
 			reg = <1>;
 			#address-cells = <1>;
 			#size-cells = <0>;
+
+			switch0: switch0@0 {
+				compatible = "marvell,mv88e6085";
+				#address-cells = <1>;
+				#size-cells = <0>;
+				reg = <0>;
+				dsa,member = <0 0>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					port@0 {
+						reg = <0>;
+						label = "lan0";
+					};
+
+					port@1 {
+						reg = <1>;
+						label = "lan1";
+					};
+
+					port@2 {
+						reg = <2>;
+						label = "lan2";
+					};
+
+					switch0port5: port@5 {
+						reg = <5>;
+						label = "dsa";
+						phy-mode = "rgmii-txid";
+						link = <&switch1port6
+							&switch2port9>;
+						fixed-link {
+							speed = <1000>;
+							full-duplex;
+						};
+					};
+
+					port@6 {
+						reg = <6>;
+						label = "cpu";
+						ethernet = <&fec1>;
+						fixed-link {
+							speed = <100>;
+							full-duplex;
+						};
+					};
+				};
+			};
 		};
 
 		mdio_mux_2: mdio@2 {
 			reg = <2>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-		};
-
-		mdio_mux_4: mdio@4 {
-			reg = <4>;
-			#address-cells = <1>;
-			#size-cells = <0>;
-		};
-
-		mdio_mux_8: mdio@8 {
-			reg = <8>;
-			#address-cells = <1>;
-			#size-cells = <0>;
-		};
-	};
-
-	dsa {
-		compatible = "marvell,dsa";
-		#address-cells = <2>;
-		#size-cells = <0>;
-		dsa,ethernet = <&fec1>;
-		dsa,mii-bus = <&mdio_mux_1>;
-
-		/* 6352 - Primary - 7 ports */
-		switch0: switch@0-0 {
-			#address-cells = <1>;
-			#size-cells = <0>;
-			reg = <0x00 0>;
-			eeprom-length = <512>;
 
-			port@0 {
+			switch1: switch1@0 {
+				compatible = "marvell,mv88e6085";
+				#address-cells = <1>;
+				#size-cells = <0>;
 				reg = <0>;
-				label = "lan0";
-			};
-
-			port@1 {
-				reg = <1>;
-				label = "lan1";
-			};
-
-			port@2 {
-				reg = <2>;
-				label = "lan2";
-			};
-
-			switch0port5: port@5 {
-				reg = <5>;
-				label = "dsa";
-				phy-mode = "rgmii-txid";
-				link = <&switch1port6
-					&switch2port9>;
-
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
+				dsa,member = <0 1>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					port@0 {
+						reg = <0>;
+						label = "lan3";
+						phy-handle = <&switch1phy0>;
+					};
+
+					port@1 {
+						reg = <1>;
+						label = "lan4";
+						phy-handle = <&switch1phy1>;
+					};
+
+					port@2 {
+						reg = <2>;
+						label = "lan5";
+						phy-handle = <&switch1phy2>;
+					};
+
+					switch1port5: port@5 {
+						reg = <5>;
+						label = "dsa";
+						link = <&switch2port9>;
+						phy-mode = "rgmii-txid";
+						fixed-link {
+							speed = <1000>;
+							full-duplex;
+						};
+					};
+
+					switch1port6: port@6 {
+						reg = <6>;
+						label = "dsa";
+						phy-mode = "rgmii-txid";
+						link = <&switch0port5>;
+						fixed-link {
+							speed = <1000>;
+							full-duplex;
+						};
+					};
 				};
-			};
-
-			port@6 {
-				reg = <6>;
-				label = "cpu";
-
-				fixed-link {
-					speed = <100>;
-					full-duplex;
+				mdio {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					switch1phy0: switch1phy0@0 {
+						reg = <0>;
+					};
+					switch1phy1: switch1phy0@1 {
+						reg = <1>;
+					};
+					switch1phy2: switch1phy0@2 {
+						reg = <2>;
+					};
 				};
 			};
-
 		};
 
-		/* 6352 - Secondary - 7 ports */
-		switch1: switch@0-1 {
+		mdio_mux_4: mdio@4 {
 			#address-cells = <1>;
 			#size-cells = <0>;
-			reg = <0x00 1>;
-			eeprom-length = <512>;
-			mii-bus = <&mdio_mux_2>;
+			reg = <4>;
 
-			port@0 {
+			switch2: switch2@0 {
+				compatible = "marvell,mv88e6085";
+				#address-cells = <1>;
+				#size-cells = <0>;
 				reg = <0>;
-				label = "lan3";
-			};
-
-			port@1 {
-				reg = <1>;
-				label = "lan4";
-			};
-
-			port@2 {
-				reg = <2>;
-				label = "lan5";
-			};
-
-			switch1port5: port@5 {
-				reg = <5>;
-				label = "dsa";
-				link = <&switch2port9>;
-				phy-mode = "rgmii-txid";
-
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
-				};
-			};
-
-			switch1port6: port@6 {
-				reg = <6>;
-				label = "dsa";
-				phy-mode = "rgmii-txid";
-				link = <&switch0port5>;
-
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
+				dsa,member = <0 2>;
+
+				ports {
+					#address-cells = <1>;
+					#size-cells = <0>;
+					port@0 {
+						reg = <0>;
+						label = "lan6";
+					};
+
+					port@1 {
+						reg = <1>;
+						label = "lan7";
+					};
+
+					port@2 {
+						reg = <2>;
+						label = "lan8";
+					};
+
+					port@3 {
+						reg = <3>;
+						label = "optical3";
+						fixed-link {
+							speed = <1000>;
+							full-duplex;
+							link-gpios = <&gpio6 2
+							      GPIO_ACTIVE_HIGH>;
+						};
+					};
+
+					port@4 {
+						reg = <4>;
+						label = "optical4";
+						fixed-link {
+							speed = <1000>;
+							full-duplex;
+							link-gpios = <&gpio6 3
+							      GPIO_ACTIVE_HIGH>;
+						};
+					};
+
+					switch2port9: port@9 {
+						reg = <9>;
+						label = "dsa";
+						phy-mode = "rgmii-txid";
+						link = <&switch1port5
+							&switch0port5>;
+						fixed-link {
+							speed = <1000>;
+							full-duplex;
+						};
+					};
 				};
 			};
 		};
 
-		/* 6185 - 10 ports */
-		switch2: switch@0-2 {
+		mdio_mux_8: mdio@8 {
+			reg = <8>;
 			#address-cells = <1>;
 			#size-cells = <0>;
-			reg = <0x00 2>;
-			mii-bus = <&mdio_mux_4>;
-
-			port@0 {
-				reg = <0>;
-				label = "lan6";
-			};
-
-			port@1 {
-				reg = <1>;
-				label = "lan7";
-			};
-
-			port@2 {
-				reg = <2>;
-				label = "lan8";
-			};
-
-			port@3 {
-				reg = <3>;
-				label = "optical3";
-
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
-					link-gpios = <&gpio6 2
-						      GPIO_ACTIVE_HIGH>;
-				};
-			};
-
-			port@4 {
-				reg = <4>;
-				label = "optical4";
-
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
-					link-gpios = <&gpio6 3
-						      GPIO_ACTIVE_HIGH>;
-				};
-			};
-
-			switch2port9: port@9 {
-				reg = <9>;
-				label = "dsa";
-				phy-mode = "rgmii-txid";
-				link = <&switch1port5
-					&switch0port5>;
-
-				fixed-link {
-					speed = <1000>;
-					full-duplex;
-				};
-			};
 		};
 	};
 
-- 
2.8.1

[RFC PATCH 16/16] dsa: Document new binding

[Andrew Lunn]

Add the new binding to the documentation of the existing binding.
Mark the old binding as deprecated.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 Documentation/devicetree/bindings/net/dsa/dsa.txt | 278 +++++++++++++++++++++-
 1 file changed, 276 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/net/dsa/dsa.txt b/Documentation/devicetree/bindings/net/dsa/dsa.txt
index 9f4807f90c31..8c9e1b80cb65 100644
--- a/Documentation/devicetree/bindings/net/dsa/dsa.txt
+++ b/Documentation/devicetree/bindings/net/dsa/dsa.txt
@@ -1,5 +1,279 @@
-Marvell Distributed Switch Architecture Device Tree Bindings
-------------------------------------------------------------
+Distributed Switch Architecture Device Tree Bindings
+----------------------------------------------------
+
+Two bindings exist, one of which has been deprecated due to
+limitations.
+
+Current Binding
+---------------
+
+Switches are true Linux devices and can be probes by any means. Once
+probed, they register to the DSA framework, passing a node
+pointer. This node is expected to fulfil the following binding, and
+may contain additional properties as required by the device it is
+embedded within.
+
+Required properties:
+
+- ports		: A container for child nodes representing switch ports.
+
+Optional properties:
+
+- dsa,member	: A two element list indicates which DSA cluster, and position
+		  within the cluster a switch takes. <0 0> is cluster 0,
+		  switch 0. <0 1> is cluster 0, switch 1. <1 0> is cluster 1,
+		  switch 0. A switch not part of any cluster (single device
+		  hanging off a CPU port) must not specify this property
+
+The ports container has the following properties
+
+Required properties:
+
+- #address-cells	: Must be 1
+- #size-cells		: Must be 0
+
+Each port children node must have the following mandatory properties:
+- reg			: Describes the port address in the switch
+- label			: Describes the label associated with this port, which
+                          will become the netdev name. Special labels are
+			  "cpu" to indicate a CPU port and "dsa" to
+			  indicate an uplink/downlink port between switches in
+			  the cluster.
+
+A port labelled "dsa" has the following mandatory property:
+
+- link			: Should be a list of phandles to other switch's DSA
+  			  port. This port is used as the outgoing port
+			  towards the phandle ports. The full routing
+			  information must be given, not just the one hop
+			  routes to neighbouring switches.
+
+A port labelled "cpu" has the following mandatory property:
+
+- ethernet		: Should be a phandle to a valid Ethernet device node.
+                          This host device is what the switch port is
+			  connected to.
+
+Port child nodes may also contain the following optional standardised
+properties, described in binding documents:
+
+- phy-handle		: Phandle to a PHY on an MDIO bus. See
+			  Documentation/devicetree/bindings/net/ethernet.txt
+			  for details.
+
+- phy-mode		: See
+			  Documentation/devicetree/bindings/net/ethernet.txt
+			  for details.
+
+- fixed-link		: Fixed-link subnode describing a link to a non-MDIO
+			  managed entity. See
+			  Documentation/devicetree/bindings/net/fixed-link.txt
+			  for details.
+
+Example
+
+The following example shows three switches on three MDIO busses,
+linked into one DSA cluster.
+
+&mdio1 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	switch0: switch0@0 {
+		compatible = "marvell,mv88e6085";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0>;
+
+		dsa,member = <0 0>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				label = "lan0";
+			};
+
+			port@1 {
+				reg = <1>;
+				label = "lan1";
+			};
+
+			port@2 {
+				reg = <2>;
+				label = "lan2";
+			};
+
+			switch0port5: port@5 {
+				reg = <5>;
+				label = "dsa";
+				phy-mode = "rgmii-txid";
+				link = <&switch1port6
+					&switch2port9>;
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+
+			port@6 {
+				reg = <6>;
+				label = "cpu";
+				ethernet = <&fec1>;
+				fixed-link {
+					speed = <100>;
+					full-duplex;
+				};
+			};
+		};
+	};
+};
+
+&mdio2 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	switch1: switch1@0 {
+		compatible = "marvell,mv88e6085";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0>;
+
+		dsa,member = <0 1>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				label = "lan3";
+				phy-handle = <&switch1phy0>;
+			};
+
+			port@1 {
+				reg = <1>;
+				label = "lan4";
+				phy-handle = <&switch1phy1>;
+			};
+
+			port@2 {
+				reg = <2>;
+				label = "lan5";
+				phy-handle = <&switch1phy2>;
+			};
+
+			switch1port5: port@5 {
+				reg = <5>;
+				label = "dsa";
+				link = <&switch2port9>;
+				phy-mode = "rgmii-txid";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+
+			switch1port6: port@6 {
+				reg = <6>;
+				label = "dsa";
+				phy-mode = "rgmii-txid";
+				link = <&switch0port5>;
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+		};
+		mdio-bus {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			switch1phy0: switch1phy0@0 {
+				reg = <0>;
+			};
+			switch1phy1: switch1phy0@1 {
+				reg = <1>;
+			};
+			switch1phy2: switch1phy0@2 {
+				reg = <2>;
+			};
+		};
+	 };
+};
+
+&mdio4 {
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	switch2: switch2@0 {
+		compatible = "marvell,mv88e6085";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		reg = <0>;
+
+		dsa,member = <0 2>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				label = "lan6";
+			};
+
+			port@1 {
+				reg = <1>;
+				label = "lan7";
+			};
+
+			port@2 {
+				reg = <2>;
+				label = "lan8";
+			};
+
+			port@3 {
+				reg = <3>;
+				label = "optical3";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+					link-gpios = <&gpio6 2
+					      GPIO_ACTIVE_HIGH>;
+				};
+			};
+
+			port@4 {
+				reg = <4>;
+				label = "optical4";
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+					link-gpios = <&gpio6 3
+					      GPIO_ACTIVE_HIGH>;
+				};
+			};
+
+			switch2port9: port@9 {
+				reg = <9>;
+				label = "dsa";
+				phy-mode = "rgmii-txid";
+				link = <&switch1port5
+					&switch0port5>;
+				fixed-link {
+					speed = <1000>;
+					full-duplex;
+				};
+			};
+		};
+	};
+};
+
+Deprecated Binding
+------------------
+
+The deprecated binding makes use of a platform device to represent the
+switches. The switches themselves are not Linux devices, and make use
+of an MDIO bus for management.
 
 Required properties:
 - compatible		: Should be "marvell,dsa"
-- 
2.8.1

Re: [RFC PATCH 00/16] New DSA bind, switches as devices

[Vivien Didelot]

Hi Andrew,

I tested this RFC and it works fine for me, looks good overall.

I'll do a second pass for nit-picking.

Thanks for this work,

       Vivien