| Message ID | 20170110201235.21771-7-f.fainelli@gmail.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
> @@ -452,11 +455,14 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, > struct net_device *ethernet_dev; > struct device_node *ethernet; > > - ethernet = of_parse_phandle(port->dn, "ethernet", 0); > - if (!ethernet) > - return -EINVAL; > + if (port->dn) { > + ethernet = of_parse_phandle(port->dn, "ethernet", 0); > + if (!ethernet) > + return -EINVAL; > + ethernet_dev = of_find_net_device_by_node(ethernet); > + } else > + ethernet_dev = dev_to_net_device(dst->pd->netdev); Hi Florian This is not going to work with John's rework of my multi CPU ports code. I think you are going to have to modify the platform_data structure to support multi-CPU ports. I put higher priority on cleanly integrating multi-CPU ports using device tree, than supporting legacy platforms. I'm O.K. with preparatory patches, but i think we should wait for actually platform data changes until after Johns code has landed and we can design the platform_data to work with it. Andrew
On 01/10/2017 12:41 PM, Andrew Lunn wrote: >> @@ -452,11 +455,14 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, >> struct net_device *ethernet_dev; >> struct device_node *ethernet; >> >> - ethernet = of_parse_phandle(port->dn, "ethernet", 0); >> - if (!ethernet) >> - return -EINVAL; >> + if (port->dn) { >> + ethernet = of_parse_phandle(port->dn, "ethernet", 0); >> + if (!ethernet) >> + return -EINVAL; >> + ethernet_dev = of_find_net_device_by_node(ethernet); >> + } else >> + ethernet_dev = dev_to_net_device(dst->pd->netdev); Bonjour Andrew, > > Hi Florian > > This is not going to work with John's rework of my multi CPU ports > code. I think you are going to have to modify the platform_data > structure to support multi-CPU ports. Last time we discussed this, I had a super complex dsa2_platform_data that allowed you to do exactly the same thing we currently do with Device Tree, except that this was with platform_data. It took a lot of effort to get there, but I essentially had the ZII vf160 board example re-implemented and verified with a mockup driver (still have it in a branch that's not too far from net-next/master). Your reply then AFAIR was that we should aim for something simpler and here is the result, we end-up re-using the existing dsa_platform_data with its limitations. If we have legacy platforms with complex setups, I really don't think we have those in tree, we should use dsa2_platform_data (still have the patches somewhere for that) although I was hoping to not have to use it since it is way more intrusive into net/dsa/dsa2.c. All platforms that I know that will benefit from this patch series: x86 SCU from ZII (out of tree), BCM47xx, BCM63xx, Orion5x have the same properties: single switch attached to a SPI/MDIO/MMAP with built-in PHYs. If we have more complex setups than that, we should try to collect the requirements. > > I put higher priority on cleanly integrating multi-CPU ports using > device tree, than supporting legacy platforms. I'm O.K. with > preparatory patches, but i think we should wait for actually platform > data changes until after Johns code has landed and we can design the > platform_data to work with it. I would very much like to see the patches and then make a decision based on the submission rather than project a decision on code that has not been submitted yet. Do we agree that patches 1 through 5 and 7 could go in then?
> Last time we discussed this, I had a super complex dsa2_platform_data > that allowed you to do exactly the same thing we currently do with > Device Tree, except that this was with platform_data. It took a lot of > effort to get there, but I essentially had the ZII vf160 board example > re-implemented and verified with a mockup driver (still have it in a > branch that's not too far from net-next/master). One thing different this time is you have associated the platform data to an MDIO device. So the platform data represents one switch, not the whole complex. This is going to make the platform data much simpler, and allow the core to do the work of assembling the multiple platform datas into one switch complex. So basically, the platform data is dsa_chip_data. To handle multi-CPUs, we need to move the master ethernet device and put it next to the cpu port. So add a struct device *netdev[DSA_MAX_PORTS]; to dsa_chip_data. It then becomes easy to represent multiple CPU ports. > I would very much like to see the patches and then make a decision based > on the submission rather than project a decision on code that has not > been submitted yet. The first version was posted a week ago. I requested a lot of changes. So lets see what John says about when the next version will be ready. Andrew
On 01/10/2017 01:21 PM, Andrew Lunn wrote: >> Last time we discussed this, I had a super complex dsa2_platform_data >> that allowed you to do exactly the same thing we currently do with >> Device Tree, except that this was with platform_data. It took a lot of >> effort to get there, but I essentially had the ZII vf160 board example >> re-implemented and verified with a mockup driver (still have it in a >> branch that's not too far from net-next/master). > > One thing different this time is you have associated the platform data > to an MDIO device. So the platform data represents one switch, not the > whole complex. This is going to make the platform data much simpler, > and allow the core to do the work of assembling the multiple platform > datas into one switch complex. So basically, the platform data is > dsa_chip_data. > > To handle multi-CPUs, we need to move the master ethernet device and > put it next to the cpu port. So add a > > struct device *netdev[DSA_MAX_PORTS]; > > to dsa_chip_data. It then becomes easy to represent multiple CPU > ports. Alright, let me get that prepared then, thanks! > >> I would very much like to see the patches and then make a decision based >> on the submission rather than project a decision on code that has not >> been submitted yet. > > The first version was posted a week ago. I requested a lot of > changes. So lets see what John says about when the next version will > be ready. Oh that series, okay, somehow I thought you were referring to something else.
diff --git a/include/net/dsa.h b/include/net/dsa.h index b9394379affb..f00ed7549a6e 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -140,6 +140,7 @@ struct dsa_switch_tree { }; struct dsa_port { + const char *name; struct net_device *netdev; struct device_node *dn; unsigned int ageing_time; diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index ddee540d9a83..7adda4b94934 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -81,14 +81,15 @@ static void dsa_dst_del_ds(struct dsa_switch_tree *dst, static bool dsa_port_is_valid(struct dsa_port *port) { - return !!port->dn; + return !!(port->dn || port->name); } static bool dsa_port_is_dsa(struct dsa_port *port) { - const char *name; + const char *name = port->name; - name = of_get_property(port->dn, "label", NULL); + if (port->dn) + name = of_get_property(port->dn, "label", NULL); if (!name) return false; @@ -100,9 +101,10 @@ static bool dsa_port_is_dsa(struct dsa_port *port) static bool dsa_port_is_cpu(struct dsa_port *port) { - const char *name; + const char *name = port->name; - name = of_get_property(port->dn, "label", NULL); + if (port->dn) + name = of_get_property(port->dn, "label", NULL); if (!name) return false; @@ -269,10 +271,11 @@ static void dsa_cpu_port_unapply(struct dsa_port *port, u32 index, static int dsa_user_port_apply(struct dsa_port *port, u32 index, struct dsa_switch *ds) { - const char *name; + const char *name = port->name; int err; - name = of_get_property(port->dn, "label", NULL); + if (port->dn) + name = of_get_property(port->dn, "label", NULL); err = dsa_slave_create(ds, ds->dev, index, name); if (err) { @@ -452,11 +455,14 @@ static int dsa_cpu_parse(struct dsa_port *port, u32 index, struct net_device *ethernet_dev; struct device_node *ethernet; - ethernet = of_parse_phandle(port->dn, "ethernet", 0); - if (!ethernet) - return -EINVAL; + if (port->dn) { + ethernet = of_parse_phandle(port->dn, "ethernet", 0); + if (!ethernet) + return -EINVAL; + ethernet_dev = of_find_net_device_by_node(ethernet); + } else + ethernet_dev = dev_to_net_device(dst->pd->netdev); - ethernet_dev = of_find_net_device_by_node(ethernet); if (!ethernet_dev) return -EPROBE_DEFER; @@ -559,6 +565,33 @@ static int dsa_parse_ports_dn(struct device_node *ports, struct dsa_switch *ds) return 0; } +static int dsa_parse_ports(struct dsa_chip_data *cd, struct dsa_switch *ds) +{ + bool valid_name_found = false; + unsigned int i; + + for (i = 0; i < DSA_MAX_PORTS; i++) { + if (!cd->port_names[i]) + continue; + + ds->ports[i].name = cd->port_names[i]; + + /* Initialize enabled_port_mask now for drv->setup() + * to have access to a correct value, just like what + * net/dsa/dsa.c::dsa_switch_setup_one does. + */ + if (!dsa_port_is_cpu(&ds->ports[i])) + ds->enabled_port_mask |= 1 << i; + + valid_name_found= true; + } + + if (!valid_name_found && i == DSA_MAX_PORTS) + return -EINVAL; + + return 0; +} + static int dsa_parse_member_dn(struct device_node *np, u32 *tree, u32 *index) { int err; @@ -583,6 +616,17 @@ static int dsa_parse_member_dn(struct device_node *np, u32 *tree, u32 *index) return 0; } +static int dsa_parse_member(struct dsa_platform_data *pd, u32 *tree, u32 *index) +{ + *tree = *index = 0; + + /* TODO, re-design platform data? */ + if (pd->nr_chips >= DSA_MAX_SWITCHES || !pd->chip) + return -EINVAL; + + return 0; +} + static struct device_node *dsa_get_ports(struct dsa_switch *ds, struct device_node *np) { @@ -599,23 +643,34 @@ static struct device_node *dsa_get_ports(struct dsa_switch *ds, static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) { + struct dsa_platform_data *pdata = dev->platform_data; struct device_node *np = dev->of_node; struct dsa_switch_tree *dst; struct device_node *ports; u32 tree, index; int i, err; - err = dsa_parse_member_dn(np, &tree, &index); - if (err) - return err; + if (np) { + err = dsa_parse_member_dn(np, &tree, &index); + if (err) + return err; - ports = dsa_get_ports(ds, np); - if (IS_ERR(ports)) - return PTR_ERR(ports); + ports = dsa_get_ports(ds, np); + if (IS_ERR(ports)) + return PTR_ERR(ports); - err = dsa_parse_ports_dn(ports, ds); - if (err) - return err; + err = dsa_parse_ports_dn(ports, ds); + if (err) + return err; + } else { + err = dsa_parse_member(pdata, &tree, &index); + if (err) + return err; + + err = dsa_parse_ports(&pdata->chip[index], ds); + if (err) + return err; + } dst = dsa_get_dst(tree); if (!dst) { @@ -630,6 +685,7 @@ static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) } ds->dst = dst; + dst->pd = pdata; ds->index = index; /* Initialize the routing table */
Allow drivers to use the new DSA API with platform data. Most of the code in net/dsa/dsa2.c does not rely so much on device_nodes and can get the same information from platform_data instead. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- include/net/dsa.h | 1 + net/dsa/dsa2.c | 96 +++++++++++++++++++++++++++++++++++++++++++------------ 2 files changed, 77 insertions(+), 20 deletions(-)