| Message ID | 20170112034121.27697-9-f.fainelli@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> index cd91070b5467..d326fc4afad7 100644 > --- a/net/dsa/dsa2.c > +++ b/net/dsa/dsa2.c > @@ -81,17 +81,23 @@ 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); > } Does this clash with Viviens recent change to make names optional and have the kernel assign it? I suppose you could use an name of "eth%d"? Is it worth adding a comment to the platform data structure? Andrew
> static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) > { > + struct dsa_chip_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); Hi Florian Maybe it is hiding, but i don't see anywhere you check that pdata != NULL. At least for x86 platforms, i don't expect we are booting using platform data like ARM systems used to do. I think it is more likely a glue module will be loaded. It looks up the MDIO bus and appends a platform data to an MDIO device. The switch driver then needs to load and use the platform data. But if things happen in a different order, it could be the switch driver probes before the glue driver, meaning pdata is NULL. Do we even want to return -EPROBE_DEFERED? Andrew
On 01/13/2017 06:04 AM, Andrew Lunn wrote: >> index cd91070b5467..d326fc4afad7 100644 >> --- a/net/dsa/dsa2.c >> +++ b/net/dsa/dsa2.c >> @@ -81,17 +81,23 @@ 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); >> } > > Does this clash with Viviens recent change to make names optional and > have the kernel assign it? So there were two ways to look at this, one was that could check here that ds->pd is assigned and port->name is assigned, which means that platform data has to provide valid port name. We can also eliminate this check entirely because we now support NULL names just fines. > > I suppose you could use an name of "eth%d"? Is it worth adding a > comment to the platform data structure? Humm, that could be done, maybe for simplicity we can just let net/dsa/dsa2.c assign names either based on what platform data provided, or by falling back to eth%d. Thanks!
On 01/13/2017 06:11 AM, Andrew Lunn wrote: >> static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) >> { >> + struct dsa_chip_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); > Hello Andrew, > Hi Florian > > Maybe it is hiding, but i don't see anywhere you check that pdata != > NULL. You are right, there is not such a check, it should probably be added early on. > > At least for x86 platforms, i don't expect we are booting using > platform data like ARM systems used to do. I think it is more likely a > glue module will be loaded. It looks up the MDIO bus and appends a > platform data to an MDIO device. The switch driver then needs to load > and use the platform data. But if things happen in a different order, > it could be the switch driver probes before the glue driver, meaning > pdata is NULL. That's very valid, I will fix this, thanks! > > Do we even want to return -EPROBE_DEFERED? I was trying to exercise that code path a little bit, but could not quite make sense of what I was seeing, let me try again with more tracing.
On 01/13/2017 02:37 PM, Florian Fainelli wrote: > On 01/13/2017 06:04 AM, Andrew Lunn wrote: >>> index cd91070b5467..d326fc4afad7 100644 >>> --- a/net/dsa/dsa2.c >>> +++ b/net/dsa/dsa2.c >>> @@ -81,17 +81,23 @@ 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); >>> } >> >> Does this clash with Viviens recent change to make names optional and >> have the kernel assign it? > > So there were two ways to look at this, one was that could check here > that ds->pd is assigned and port->name is assigned, which means that > platform data has to provide valid port name. We can also eliminate this > check entirely because we now support NULL names just fines. Considering that the comment above struct dsa_chip_data::port_names in net/dsa/dsa.h is pretty clear about the port_names usage, I am tempted to keep the code as-is since without a name, for platform data, we would not have a way to tell if a port is disabled or not. Does that work for you?
diff --git a/include/net/dsa.h b/include/net/dsa.h index 16a502a6c26a..491008792e4d 100644 --- a/include/net/dsa.h +++ b/include/net/dsa.h @@ -42,6 +42,11 @@ struct dsa_chip_data { struct device *host_dev; int sw_addr; + /* + * Reference to network devices + */ + struct device *netdev[DSA_MAX_PORTS]; + /* set to size of eeprom if supported by the switch */ int eeprom_len; @@ -140,6 +145,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 cd91070b5467..d326fc4afad7 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -81,17 +81,23 @@ 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) { - return !!of_parse_phandle(port->dn, "link", 0); + if (port->name && !strcmp(port->name, "dsa")) + return true; + else + return !!of_parse_phandle(port->dn, "link", 0); } static bool dsa_port_is_cpu(struct dsa_port *port) { - return !!of_parse_phandle(port->dn, "ethernet", 0); + if (port->name && !strcmp(port->name, "cpu")) + return true; + else + return !!of_parse_phandle(port->dn, "ethernet", 0); } static bool dsa_ds_find_port_dn(struct dsa_switch *ds, @@ -251,10 +257,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); if (!name) name = "eth%d"; @@ -439,11 +446,15 @@ 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(ds->cd->netdev[index]); + } - ethernet_dev = of_find_net_device_by_node(ethernet); if (!ethernet_dev) return -EPROBE_DEFER; @@ -546,6 +557,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; @@ -570,6 +608,15 @@ static int dsa_parse_member_dn(struct device_node *np, u32 *tree, u32 *index) return 0; } +static int dsa_parse_member(struct dsa_chip_data *pd, u32 *tree, u32 *index) +{ + /* We do not support complex trees with dsa_chip_data */ + *tree = 0; + *index = 0; + + return 0; +} + static struct device_node *dsa_get_ports(struct dsa_switch *ds, struct device_node *np) { @@ -586,23 +633,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_chip_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, ds); + if (err) + return err; + } dst = dsa_get_dst(tree); if (!dst) { @@ -618,6 +676,7 @@ static int _dsa_register_switch(struct dsa_switch *ds, struct device *dev) ds->dst = dst; ds->index = index; + ds->cd = pdata; /* Initialize the routing table */ for (i = 0; i < DSA_MAX_SWITCHES; ++i)
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. We purposely do not support distributed configurations with platform data, so drivers should be providing a pointer to a 'struct dsa_chip_data' structure if they wish to communicate per-port layout. Multiple CPUs port could potentially be supported and dsa_chip_data is extended to receive up to one reference to an upstream network device per port described by a dsa_chip_data structure. Signed-off-by: Florian Fainelli <f.fainelli@gmail.com> --- include/net/dsa.h | 6 ++++ net/dsa/dsa2.c | 95 ++++++++++++++++++++++++++++++++++++++++++++----------- 2 files changed, 83 insertions(+), 18 deletions(-)