[U-Boot] [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env variable serially

[U-Boot] [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env variable serially

[Prabhakar Kushwaha]

Current implementation of MAC address fix-up of device tree uses
tailing number behind "ethernet" found in "/aliases".  It is not
necessary for trailing number of “ethernet” to be sequential. There
can be hole in between or any node disabled.

So provide support device tree fix-up of “ethernent” node with MAC
addresses fetched sequentially from environment variables.

Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
---
 README               | 10 ++++++++++
 common/fdt_support.c | 25 +++++++++++++++++++++----
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/README b/README
index 7a4f342..640b310 100644
--- a/README
+++ b/README
@@ -1603,6 +1603,16 @@ The following options need to be configured:
 
 		See doc/README.link-local for more information.
 
+ - MAC address from environment variables
+
+		FDT_SEQ_MACADDR_FROM_ENV
+
+		Fix-up device tree with MAC address fetched sequentially  from
+		environment variables. This will avoid dependency on device-tree
+		to get env variable sequence number. This config has assumption
+		of non-usable ethernet node of device-tree are either not present
+		or their status has been marked as "disabled".
+
  - CDP Options:
 		CONFIG_CDP_DEVICE_ID
 
diff --git a/common/fdt_support.c b/common/fdt_support.c
index f4f9543..43ca8db 100644
--- a/common/fdt_support.c
+++ b/common/fdt_support.c
@@ -469,12 +469,16 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
 
 void fdt_fixup_ethernet(void *fdt)
 {
-	int i, j, prop;
+	int i = 0, j, prop;
 	char *tmp, *end;
 	char mac[16];
 	const char *path;
 	unsigned char mac_addr[ARP_HLEN];
 	int offset;
+#ifdef FDT_SEQ_MACADDR_FROM_ENV
+	int nodeoff;
+	const struct fdt_property *fdt_prop;
+#endif
 
 	if (fdt_path_offset(fdt, "/aliases") < 0)
 		return;
@@ -487,7 +491,7 @@ void fdt_fixup_ethernet(void *fdt)
 		offset = fdt_first_property_offset(fdt,
 			fdt_path_offset(fdt, "/aliases"));
 		/* Select property number 'prop' */
-		for (i = 0; i < prop; i++)
+		for (j = 0; j < prop; j++)
 			offset = fdt_next_property_offset(fdt, offset);
 
 		if (offset < 0)
@@ -496,11 +500,16 @@ void fdt_fixup_ethernet(void *fdt)
 		path = fdt_getprop_by_offset(fdt, offset, &name, NULL);
 		if (!strncmp(name, "ethernet", 8)) {
 			/* Treat plain "ethernet" same as "ethernet0". */
-			if (!strcmp(name, "ethernet"))
+			if (!strcmp(name, "ethernet")
+#ifdef FDT_SEQ_MACADDR_FROM_ENV
+			 || !strcmp(name, "ethernet0")
+#endif
+			)
 				i = 0;
+#ifndef FDT_SEQ_MACADDR_FROM_ENV
 			else
 				i = trailing_strtol(name);
-
+#endif
 			if (i != -1) {
 				if (i == 0)
 					strcpy(mac, "ethaddr");
@@ -509,6 +518,14 @@ void fdt_fixup_ethernet(void *fdt)
 			} else {
 				continue;
 			}
+#ifdef FDT_SEQ_MACADDR_FROM_ENV
+			nodeoff = fdt_path_offset(fdt, path);
+			fdt_prop = fdt_get_property(fdt, nodeoff, "status",
+						    NULL);
+			if (fdt_prop && !strcmp(fdt_prop->data, "disabled"))
+				continue;
+			i++;
+#endif
 			tmp = env_get(mac);
 			if (!tmp)
 				continue;
-- 
2.7.4

[U-Boot] [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env variable serially

[York Sun]

On 11/21/2017 08:26 AM, Prabhakar Kushwaha wrote:
> Current implementation of MAC address fix-up of device tree uses
> tailing number behind "ethernet" found in "/aliases".  It is not
> necessary for trailing number of “ethernet” to be sequential. There
> can be hole in between or any node disabled.
> 
> So provide support device tree fix-up of “ethernent” node with MAC
> addresses fetched sequentially from environment variables.

My understanding is current implementation matches the trailing number
behind "ethernet" found in "/aliases" with environmental variables
ethNaddr (where N is 1, 2, 3, ... or absent). This doesn't require
"ethernet" nodes or ethNaddr variables to be consecutive.

From your change, I understand you are trying to apply consecutive
ethNaddr variables to "ethernet" nodes in the order they appear in
device tree.

You didn't explain what benefit you get from this new scheme, or what
problem you are trying to solve.

My understanding is you have device tree with some "ethernet" nodes
disabled (or with gap in the numbering). You also have a pool of MAC
addresses in the form of consecutive ethNaddr variables. You goal is to
assign these ethNaddr to "ethernet" nodes in the order they appear in
the device tree. Do I understand you correctly?

Is it possible to address this issue from another angle? How about
setting ethNaddr variables in U-Boot according to the SerDes protocol?
You may have explored this path already. Let me know why this doesn't work.

> 
> Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> ---
>  README               | 10 ++++++++++
>  common/fdt_support.c | 25 +++++++++++++++++++++----
>  2 files changed, 31 insertions(+), 4 deletions(-)
> 
> diff --git a/README b/README
> index 7a4f342..640b310 100644
> --- a/README
> +++ b/README
> @@ -1603,6 +1603,16 @@ The following options need to be configured:
>  
>  		See doc/README.link-local for more information.
>  
> + - MAC address from environment variables
> +
> +		FDT_SEQ_MACADDR_FROM_ENV

Why not create a Kconfig option?

York

[U-Boot] [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env variable serially

[Prabhakar Kushwaha]

> -----Original Message-----
> From: York Sun
> Sent: Tuesday, November 21, 2017 10:52 PM
> To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; u-
> boot at lists.denx.de
> Subject: Re: [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env
> variable serially
> 
> On 11/21/2017 08:26 AM, Prabhakar Kushwaha wrote:
> > Current implementation of MAC address fix-up of device tree uses
> > tailing number behind "ethernet" found in "/aliases".  It is not
> > necessary for trailing number of "ethernet" to be sequential. There
> > can be hole in between or any node disabled.
> >
> > So provide support device tree fix-up of "ethernent" node with MAC
> > addresses fetched sequentially from environment variables.
> 
> My understanding is current implementation matches the trailing number
> behind "ethernet" found in "/aliases" with environmental variables
> ethNaddr (where N is 1, 2, 3, ... or absent). This doesn't require
> "ethernet" nodes or ethNaddr variables to be consecutive.
> 
> From your change, I understand you are trying to apply consecutive
> ethNaddr variables to "ethernet" nodes in the order they appear in
> device tree.
> 
> You didn't explain what benefit you get from this new scheme, or what
> problem you are trying to solve.
> 
> My understanding is you have device tree with some "ethernet" nodes
> disabled (or with gap in the numbering). You also have a pool of MAC
> addresses in the form of consecutive ethNaddr variables. You goal is to
> assign these ethNaddr to "ethernet" nodes in the order they appear in
> the device tree. Do I understand you correctly?

Yes this understanding is correct. 
I am parsing "ethernet" node from device tree and fixing up ethNaddr sequentially.


> 
> Is it possible to address this issue from another angle? How about
> setting ethNaddr variables in U-Boot according to the SerDes protocol?
> You may have explored this path already. Let me know why this doesn't work.

Yes, analyzed this path and figured out following 2 changes in u-boot

A) I2C EEPROM:  Here MAC number has to be read sequentially from EEPROM and they have to save as ethNaddr. here N is MAC number as per SerDes
      MAC number will be saved in ethaddr, eth1addr, eth2addr, eth3addr,  eth10addr depending upon SerDes protocol
  
B) net/eth_legacy.c: eth_initialize()  --> eth_write_hwaddr
here each Ethernet driver read ethNaddr.  Here N represents eth_device-> index.
Please note eth_device->index in under control of Ethernet subsystem and  incremented automatically per Ethernet device.

for a Ethernet device which require eth9addr (updated as part of A). It must have 10 ethernet device before it to get correct eth9addr. 
Hence a mapping required from eth_device->index to MAC number.  I wanted to avoid "B" to make sure Ethernet device initialization path remain unchanged. 

Hence chosen to update fdt_support.c  

-pk









> 
> >
> > Signed-off-by: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>
> > ---
> >  README               | 10 ++++++++++
> >  common/fdt_support.c | 25 +++++++++++++++++++++----
> >  2 files changed, 31 insertions(+), 4 deletions(-)
> >
> > diff --git a/README b/README
> > index 7a4f342..640b310 100644
> > --- a/README
> > +++ b/README
> > @@ -1603,6 +1603,16 @@ The following options need to be configured:
> >
> >  		See doc/README.link-local for more information.
> >
> > + - MAC address from environment variables
> > +
> > +		FDT_SEQ_MACADDR_FROM_ENV
> 
> Why not create a Kconfig option?
> 
> York

[U-Boot] [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env variable serially

[York Sun]

On 11/22/2017 03:52 AM, Prabhakar Kushwaha wrote:
> 
>> -----Original Message-----
>> From: York Sun
>> Sent: Tuesday, November 21, 2017 10:52 PM
>> To: Prabhakar Kushwaha <prabhakar.kushwaha@nxp.com>; u-
>> boot at lists.denx.de
>> Subject: Re: [PATCH 1/3] common: Fix-up MAC addr in dts by fetching env
>> variable serially
>>
>> On 11/21/2017 08:26 AM, Prabhakar Kushwaha wrote:
>>> Current implementation of MAC address fix-up of device tree uses
>>> tailing number behind "ethernet" found in "/aliases".  It is not
>>> necessary for trailing number of “ethernet” to be sequential. There
>>> can be hole in between or any node disabled.
>>>
>>> So provide support device tree fix-up of “ethernent” node with MAC
>>> addresses fetched sequentially from environment variables.
>>
>> My understanding is current implementation matches the trailing number
>> behind "ethernet" found in "/aliases" with environmental variables
>> ethNaddr (where N is 1, 2, 3, ... or absent). This doesn't require
>> "ethernet" nodes or ethNaddr variables to be consecutive.
>>
>> From your change, I understand you are trying to apply consecutive
>> ethNaddr variables to "ethernet" nodes in the order they appear in
>> device tree.
>>
>> You didn't explain what benefit you get from this new scheme, or what
>> problem you are trying to solve.
>>
>> My understanding is you have device tree with some "ethernet" nodes
>> disabled (or with gap in the numbering). You also have a pool of MAC
>> addresses in the form of consecutive ethNaddr variables. You goal is to
>> assign these ethNaddr to "ethernet" nodes in the order they appear in
>> the device tree. Do I understand you correctly?
> 
> Yes this understanding is correct. 
> I am parsing "ethernet" node from device tree and fixing up ethNaddr sequentially.
> 
> 
>>
>> Is it possible to address this issue from another angle? How about
>> setting ethNaddr variables in U-Boot according to the SerDes protocol?
>> You may have explored this path already. Let me know why this doesn't work.
> 
> Yes, analyzed this path and figured out following 2 changes in u-boot
> 
> A) I2C EEPROM:  Here MAC number has to be read sequentially from EEPROM and they have to save as ethNaddr. here N is MAC number as per SerDes
>       MAC number will be saved in ethaddr, eth1addr, eth2addr, eth3addr,  eth10addr depending upon SerDes protocol
>   
> B) net/eth_legacy.c: eth_initialize()  --> eth_write_hwaddr
> here each Ethernet driver read ethNaddr.  Here N represents eth_device-> index.
> Please note eth_device->index in under control of Ethernet subsystem and  incremented automatically per Ethernet device.
> 
> for a Ethernet device which require eth9addr (updated as part of A). It must have 10 ethernet device before it to get correct eth9addr. 
> Hence a mapping required from eth_device->index to MAC number.  I wanted to avoid "B" to make sure Ethernet device initialization path remain unchanged. 
> 
> Hence chosen to update fdt_support.c  
> 

Thanks for the explanation. I will leave the decision to Simon Glass.
In the meantime, maybe you can rephrase the commit message to make it
more clear.

York