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

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

[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

[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 {

[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 
Signed-off-by: Florian Fainelli 
---
 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(>dev);
struct mv88e6xxx_priv_state *ps = ds_to_priv(ds);
 
+   dsa_unregister_switch(ds);
put_device(>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_headlist;
+
+   /* 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 ..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 
+ * Copyright (c) 2016 Andrew Lunn 
+ *
+ * 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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#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;
+
+