Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data

[Florian Fainelli]

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?
-- 
Florian

Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data

[Florian Fainelli]

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, , );
>> -if (err)
>> -return err;
>> +if (np) {
>> +err = dsa_parse_member_dn(np, , );
>> +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, , );
> 

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.
-- 
Florian

Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data

[Florian Fainelli]

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!
-- 
Florian

Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data

[Andrew Lunn]

>  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, , );
> - if (err)
> - return err;
> + if (np) {
> + err = dsa_parse_member_dn(np, , );
> + 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, , );

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

Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data

[Andrew Lunn]

> 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

[PATCH net-next v2 08/10] net: dsa: Add support for platform data

[Florian Fainelli]

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 
---
 include/net/dsa.h |  6 
 net/dsa/dsa2.c| 95 ---
 2 files changed, 83 insertions(+), 18 deletions(-)

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 intageing_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(>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 */
+