Re: [PATCH net-next 2/9] net: dsa: Add support for parsing the old binding

[Florian Fainelli]

On 06/05/2016 08:19 PM, Andrew Lunn wrote:
>> How much support do we want to have for the old binding for in tree
>> platforms? Is the plan to migrate them all to the new binding?
> 
> I think there are three cases to consider.
> 
> 1) There are some old boards using setup.c files which have a platform
>device, platform data, etc. I've never used DSA in this way, and it
>could be all the recent additions have broken this. We might want
>to test this, and if it is in fact broken, and has been for a
>while, it indicates nobody uses those boards any more. We might
>suggest removing them. Even if they do work, i doubt anybody is
>interested in converting them to device tree. So we might have to
>keep the platform data support around.

We had a report a while ago of breakage, which got addressed and fixed
upstream, so if it breaks again, it will get fixed again.

> 
> 2) In tree devices using the DT binding. We can update them all to the
>new binding. The kirkwood boards don't have a u-boot which is DT
>capable. Some of the armada boards do have a DT capable uboot, but
>all these boards have been added by the community, so i suspect
>they are not flashed never to be changed again.
> 
> 3) Out of tree devices using the DT binding. As far as i can see,
>there is no in three board actually using the Broadcom SF2 driver
>and its odd binding. However from talking to you, i know there are
>devices out in the wild using this binding, and their DT blob is
>fixed, never to be changed again.

The concept of an "in-tree" board does not make much sense once the
bootloader provides a blob to the kernel, and synchronizing the Device
Tree sources with what a bootloader provides is just a pain with no
reward as long as the binding remains standard and works.

> 
> It actually seems odd to me that we have a nice new binding and an
> implement which is reasonably clean, and we want to add code to
> support a legacy binding for an out of tree board.
> 
> I need to think on this for a while. However, i don't see the old code
> and binding going away anytime soon. It will take a few cycles to
> determine if the old platform device/platform data still works, and to
> remove the old boards if not. We can update the in tree devices to the
> new binding, but we should keep the old binding a while to aid the
> transition.

I do not see the need for platform data going away actually, there are
tons of devices out there that are not supported using Device Tree, yet
feature Ethernet switches that could easily be supported would we want
to add support for that, and clearly an answer along the lines of let's
add Device Tree support for these platforms is not going to fly.

> 
> I'm tempted to say you should keep using the old code to support your
> out of tree devices. You should define a new binding for SF2 which
> conforms to the device tree binding document which just got accepted,
> and add it to SF 2 alongside the legacy binding. And it would be great
> if you could go the last step and actually add a boards device tree
> file using it.

I suppose I could do that.

> 
> I'm hesitant to add legacy binding support for SF2 to the new DSA2
> code. We should try to keep it free of cruft, and set a good example
> for others to follow when they bring along there new drivers.

What if this code was moved to the bcm_sf2.c where it matters and there
is just the bottom part of dsa_register_switch() exposed instead?
-- 
Florian

Re: [PATCH net-next 2/9] net: dsa: Add support for parsing the old binding

[Andrew Lunn]

> How much support do we want to have for the old binding for in tree
> platforms? Is the plan to migrate them all to the new binding?

I think there are three cases to consider.

1) There are some old boards using setup.c files which have a platform
   device, platform data, etc. I've never used DSA in this way, and it
   could be all the recent additions have broken this. We might want
   to test this, and if it is in fact broken, and has been for a
   while, it indicates nobody uses those boards any more. We might
   suggest removing them. Even if they do work, i doubt anybody is
   interested in converting them to device tree. So we might have to
   keep the platform data support around.

2) In tree devices using the DT binding. We can update them all to the
   new binding. The kirkwood boards don't have a u-boot which is DT
   capable. Some of the armada boards do have a DT capable uboot, but
   all these boards have been added by the community, so i suspect
   they are not flashed never to be changed again.

3) Out of tree devices using the DT binding. As far as i can see,
   there is no in three board actually using the Broadcom SF2 driver
   and its odd binding. However from talking to you, i know there are
   devices out in the wild using this binding, and their DT blob is
   fixed, never to be changed again.

It actually seems odd to me that we have a nice new binding and an
implement which is reasonably clean, and we want to add code to
support a legacy binding for an out of tree board.

I need to think on this for a while. However, i don't see the old code
and binding going away anytime soon. It will take a few cycles to
determine if the old platform device/platform data still works, and to
remove the old boards if not. We can update the in tree devices to the
new binding, but we should keep the old binding a while to aid the
transition.

I'm tempted to say you should keep using the old code to support your
out of tree devices. You should define a new binding for SF2 which
conforms to the device tree binding document which just got accepted,
and add it to SF 2 alongside the legacy binding. And it would be great
if you could go the last step and actually add a boards device tree
file using it.

I'm hesitant to add legacy binding support for SF2 to the new DSA2
code. We should try to keep it free of cruft, and set a good example
for others to follow when they bring along there new drivers.

Andrew

Re: [PATCH net-next 2/9] net: dsa: Add support for parsing the old binding

[Florian Fainelli]

Le 04/06/2016 13:00, Andrew Lunn a écrit :
>> -static int dsa_cpu_parse(struct device_node *port, u32 index,
>> - struct dsa_switch_tree *dst,
>> - struct dsa_switch *ds)
>> +static int _dsa_cpu_parse(struct dsa_switch_tree *dst,
>> +   struct dsa_switch *ds,
>> +   struct net_device *ethernet_dev,
>> +   u32 index)
>>  {
>> -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;
>> -
> 
> Hi Florian
> 
> You have just removed all the actual DT parsing. So i would give this
> a different name, and avoid the _ prefix.
> 
>> +static int _dsa_register_switch_legacy(struct dsa_switch *ds, struct 
>> device_node *np)
>> +{
> 
> We might want to call this _dsa_register_switch_legacy_sf2, since the
> code only supports what is needed for your rather odd sf2 binding.

The SF2 binding just encapsulates the normal legacy DSA binding in its
simplified, one switch only configuration, that makes things much
easier, but not way off.

> It does not appear to work for the generic DSA binding.

It would if we also added support for parsing and filing in a routing
table, so yes, that is currently missing.

How about offering dsa_of_parse() (or another name) as a helper function
which fills up a dsa_switch / dsa_switch_tree structure to the best it
can, and then let a driver dealing with the old binding call into the
bottom parts of dsa_register_switch()? That could allow us to remove
some of the duplicated code from net/dsa/dsa.c by doing so

The other approach could be to have custom DT parsing in bcm_sf2.c and
also call the bottom parts of dsa_register_switch() once it has properly
initialized ds/dst.

How much support do we want to have for the old binding for in tree
platforms? Is the plan to migrate them all to the new binding?

-- 
Florian

Re: [PATCH net-next 2/9] net: dsa: Add support for parsing the old binding

[Andrew Lunn]

> -static int dsa_cpu_parse(struct device_node *port, u32 index,
> -  struct dsa_switch_tree *dst,
> -  struct dsa_switch *ds)
> +static int _dsa_cpu_parse(struct dsa_switch_tree *dst,
> +struct dsa_switch *ds,
> +struct net_device *ethernet_dev,
> +u32 index)
>  {
> - 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;
> -

Hi Florian

You have just removed all the actual DT parsing. So i would give this
a different name, and avoid the _ prefix.

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

We might want to call this _dsa_register_switch_legacy_sf2, since the
code only supports what is needed for your rather odd sf2 binding. It
does not appear to work for the generic DSA binding.

>  static int __dsa_register_switch(struct dsa_switch *ds, struct device_node 
> *np)
>  {
>   struct device_node *ports = dsa_get_ports(ds, np);
> @@ -626,8 +736,8 @@ static int _dsa_register_switch(struct dsa_switch *ds, 
> struct device_node *np)
>  {
>   struct device_node *ports = dsa_get_ports(ds, np);
>  
> - if (IS_ERR(ports))
> - return PTR_ERR(ports);
> + if (IS_ERR(ports) && PTR_ERR(ports) == -EINVAL)
> + return _dsa_register_switch_legacy(ds, np);

Maybe put this inside if config_enabled(CONFIG_NET_DSA_BCM_SF2)  {}?

  Andrew

Re: [PATCH net-next 2/9] net: dsa: Add support for parsing the old binding

[Andrew Lunn]

> @@ -626,8 +736,8 @@ static int _dsa_register_switch(struct dsa_switch *ds, 
> struct device_node *np)
>  {
>   struct device_node *ports = dsa_get_ports(ds, np);
>  
> - if (IS_ERR(ports))
> - return PTR_ERR(ports);
> + if (IS_ERR(ports) && PTR_ERR(ports) == -EINVAL)
> + return _dsa_register_switch_legacy(ds, np);
>  
>   return __dsa_register_switch(ds, np);
>  }

Hi Florian

Could you put this into dsa_register_switch() and so avoid all the __
prefixes.

Thanks
Andrew

[PATCH net-next 2/9] net: dsa: Add support for parsing the old binding

[Florian Fainelli]

Extend dsa2.c to support parsing for the old binding, which mostly means
looking for more or less the same properties in different places.

Signed-off-by: Florian Fainelli 
---
 net/dsa/dsa2.c | 142 ++---
 1 file changed, 126 insertions(+), 16 deletions(-)

diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c
index b5640d8ffbae..23273fd984a8 100644
--- a/net/dsa/dsa2.c
+++ b/net/dsa/dsa2.c
@@ -408,21 +408,11 @@ static void dsa_dst_unapply(struct dsa_switch_tree *dst)
dst->applied = false;
 }
 
-static int dsa_cpu_parse(struct device_node *port, u32 index,
-struct dsa_switch_tree *dst,
-struct dsa_switch *ds)
+static int _dsa_cpu_parse(struct dsa_switch_tree *dst,
+  struct dsa_switch *ds,
+  struct net_device *ethernet_dev,
+  u32 index)
 {
-   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;
 
@@ -445,6 +435,24 @@ static int dsa_cpu_parse(struct device_node *port, u32 
index,
return 0;
 }
 
+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;
+
+   return _dsa_cpu_parse(dst, ds, ethernet_dev, index);
+}
+
 static int dsa_ds_parse(struct dsa_switch_tree *dst, struct dsa_switch *ds)
 {
struct device_node *port;
@@ -552,6 +560,108 @@ static struct device_node *dsa_get_ports(struct 
dsa_switch *ds,
return ports;
 }
 
+static int _dsa_register_switch_legacy(struct dsa_switch *ds, struct 
device_node *np)
+{
+   const char *compat = of_get_property(np, "compatible", NULL);
+   struct device_node *dn, *ethernet;
+   struct net_device *ethernet_dev;
+   struct dsa_switch_tree *dst;
+   u32 tree = 0, index;
+   int err;
+
+   /* Tree is implied by how many devices are present in the DT with the
+* supported compatible strings from net/dsa/dsa.c
+*/
+   for_each_compatible_node(dn, NULL, compat) {
+   if (dn != np)
+   tree++;
+   }
+
+   /* index is present in the "reg" property, second cell */
+   err = of_property_read_u32_index(np->child, "reg", 1, );
+   if (err)
+   return err;
+
+   if (index >= DSA_MAX_SWITCHES)
+   return -EINVAL;
+
+   err = dsa_parse_ports_dn(np->child, 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;
+
+   if (err == 1) {
+   /* Not all switches registered yet */
+   err = 0;
+   goto out;
+   }
+
+   if (dst->applied) {
+   pr_info("DSA: Disjoint trees?\n");
+   err = -EINVAL;
+   goto out_del_dst;
+   }
+
+   ethernet = of_parse_phandle(np, "dsa,ethernet", 0);
+   if (!ethernet) {
+   err = -EINVAL;
+   goto out_del_dst;
+   }
+
+   ethernet_dev = of_find_net_device_by_node(ethernet);
+   if (!ethernet_dev) {
+   err = -EPROBE_DEFER;
+   goto out_del_dst;
+   }
+
+   err = _dsa_cpu_parse(dst, ds, ethernet_dev, index);
+   if (err)
+   goto out_del_dst;
+
+   if (!dst->master_netdev) {
+   pr_warn("Tree has no master device\n");
+   goto out_del_dst;
+   }
+
+   pr_info("DSA: tree %d parsed\n", dst->tree);
+
+   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;
+}
+
 static int __dsa_register_switch(struct dsa_switch *ds, struct device_node *np)
 {
struct device_node