Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1557098 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH net-next v2 08/10] net: dsa: Add support for platform data Florian Fainelli <f.fainelli@gmail.com> - 2017-01-12 04:50 +0100
    Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data Andrew Lunn <andrew@lunn.ch> - 2017-01-13 15:10 +0100
      Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data Florian Fainelli <f.fainelli@gmail.com> - 2017-01-13 23:40 +0100
        Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data Florian Fainelli <f.fainelli@gmail.com> - 2017-01-14 20:20 +0100
    Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data Andrew Lunn <andrew@lunn.ch> - 2017-01-13 15:20 +0100
      Re: [PATCH net-next v2 08/10] net: dsa: Add support for platform data Florian Fainelli <f.fainelli@gmail.com> - 2017-01-13 23:40 +0100

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

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(-)

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)
-- 
2.9.3

[toc] | [next] | [standalone]

#1558423

> 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

[toc] | [prev] | [next] | [standalone]

#1558784

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

[toc] | [prev] | [next] | [standalone]

#1559058

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

[toc] | [prev] | [next] | [standalone]

#1558427

>  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

[toc] | [prev] | [next] | [standalone]

#1558778

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web