fdt: Rewrite the logic in fdt_fixup_ethernet()

PATCH New Bin Meng 4 participants 11 messages Oct 27, 2015 – Oct 30, 2015

Share & tools

Bin Meng

author

2015-10-27 16:10 UTC #0 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:

    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };

"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.

Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.

Signed-off-by Bin Meng <[email protected]>

---

 common/fdt_support.c | 53 ++++++++++++++++++++++++++++++++--------------------
 1 file changed, 33 insertions(+), 20 deletions(-)

common/fdt_support.c +33 ~~-20~~

index f86365e..5cdf185 100644
@@ -476,10 +476,11 @@ int fdt_fixup_memory(void *blob, u64 start, u64 size)
 void fdt_fixup_ethernet(void *fdt)
 {
 	int node, i, j;
-	char enet[16], *tmp, *end;
+	char *tmp, *end;
 	char mac[16];
 	const char *path;
 	unsigned char mac_addr[6];
+	int offset;
 
 	node = fdt_path_offset(fdt, "/aliases");
 	if (node < 0)

@@ -496,27 +497,39 @@ void fdt_fixup_ethernet(void *fdt)
 		strcpy(mac, "ethaddr");
 	}
 
-	i = 0;
-	while ((tmp = getenv(mac)) != NULL) {
-		sprintf(enet, "ethernet%d", i);
-		path = fdt_getprop(fdt, node, enet, NULL);
-		if (!path) {
-			debug("No alias for %s\n", enet);
-			sprintf(mac, "eth%daddr", ++i);
-			continue;
-		}
+	for (offset = fdt_first_property_offset(fdt, node);
+	     offset > 0;
+	     offset = fdt_next_property_offset(fdt, offset)) {
+		const char *name;
+		int len = strlen("ethernet");
+
+		path = fdt_getprop_by_offset(fdt, offset, &name, NULL);
+		if (!strncmp(name, "ethernet", len)) {
+			i = trailing_strtol(name);
+			if (i != -1) {
+				if (i == 0)
+					strcpy(mac, "ethaddr");
+				else
+					sprintf(mac, "eth%daddr", i);
+			} else {
+				continue;
+			}
+			tmp = getenv(mac);
+			if (!tmp)
+				continue;
+
+			for (j = 0; j < 6; j++) {
+				mac_addr[j] = tmp ?
+					      simple_strtoul(tmp, &end, 16) : 0;
+				if (tmp)
+					tmp = (*end) ? end + 1 : end;
+			}
 
-		for (j = 0; j < 6; j++) {
-			mac_addr[j] = tmp ? simple_strtoul(tmp, &end, 16) : 0;
-			if (tmp)
-				tmp = (*end) ? end+1 : end;
+			do_fixup_by_path(fdt, path, "mac-address",
+					 &mac_addr, 6, 0);
+			do_fixup_by_path(fdt, path, "local-mac-address",
+					 &mac_addr, 6, 1);
 		}
-
-		do_fixup_by_path(fdt, path, "mac-address", &mac_addr, 6, 0);
-		do_fixup_by_path(fdt, path, "local-mac-address",
-				&mac_addr, 6, 1);
-
-		sprintf(mac, "eth%daddr", ++i);
 	}
 }

Signature · 1 line

1.8.2.1

Joe Hershberger

2015-10-28 20:52 UTC #1 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Hi Bin,

Quoted 15 lines · Bin Meng

Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.

Quoted 6 lines

Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>

Acked-by Joe Hershberger <[email protected]>

Bin Meng

author

2015-10-29 01:40 UTC #2 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Hi Joe,

Quoted 18 lines · Joe Hershberger

Hi Bin,
Quoted 15 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.

Quoted 8 lines

Quoted 6 lines
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>
Acked-by Joe Hershberger <[email protected]>

Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.

[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html

Regards,
Bin

Tom Rini

2015-10-29 01:50 UTC #3 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Quoted 27 lines · Bin Meng

Hi Joe,
Quoted 23 lines · Joe Hershberger
Hi Bin,
Quoted 20 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.

Quoted 8 lines

Quoted 3 lines
Quoted 1 line
Signed-off-by Bin Meng <[email protected]>
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.

I need to think about that post still, sorry, but we can't remove it, it would break various systems that have on-board USB eth (pandaboard, others).

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151028/f5777ced/attachment.sig>

Simon Glass

2015-10-29 01:53 UTC #4 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

In reply to Bin Meng

Hi,

Quoted 29 lines · Bin Meng

Hi Joe,
Quoted 25 lines · Joe Hershberger
Hi Bin,
Quoted 21 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>

Quoted 8 lines

Quoted 1 line
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.
[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html

My understanding is that we were going to drop the usbethaddr variables and just use ethaddr.

Regards,
Simon

Bin Meng

author

2015-10-29 02:00 UTC #5 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Hi Simon,

Quoted 39 lines · Simon Glass

Hi,
Quoted 36 lines · Bin Meng
Hi Joe,
Quoted 18 lines · Joe Hershberger
Hi Bin,
Quoted 15 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Quoted 8 lines
Quoted 6 lines
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.

Quoted 5 lines

Quoted 1 line
[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html
My understanding is that we were going to drop the usbethaddr variables and just use ethaddr.

Yep, I agree, and driver model eth device only uses "ethaddr" as the MAC address source. I guess when we introduced "usbethaddr" to U-Boot at the beginning we didn't think about MAC address fix up. Later commit b1f49ab was added to only handle "usbethaddr" assuming that there is only one usb ethernet on the board, which is not necessarily the case. Using two sources ("usbethaddr" and "ethaddr") just causes trouble in the logic in fdt_fixup_ethernet().

Regards,
Bin

Joe Hershberger

2015-10-29 19:38 UTC #6 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

In reply to Bin Meng

Hi Bin,

Quoted 29 lines · Bin Meng

Hi Joe,
Quoted 25 lines · Joe Hershberger
Hi Bin,
Quoted 21 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>

Quoted 8 lines

Quoted 1 line
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.
[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html

I'm in agreement with you. See here:
http://lists.denx.de/pipermail/u-boot/2015-July/218601.html

It would be great if you completely remove it.

Cheers,
-Joe

Bin Meng

author

2015-10-30 02:16 UTC #7 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Hi Joe,

Quoted 39 lines · Joe Hershberger

Hi Bin,
Quoted 36 lines · Bin Meng
Hi Joe,
Quoted 18 lines · Joe Hershberger
Hi Bin,
Quoted 15 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Quoted 8 lines
Quoted 6 lines
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.

Quoted 7 lines

Quoted 1 line
[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html
I'm in agreement with you. See here:
http://lists.denx.de/pipermail/u-boot/2015-July/218601.html
It would be great if you completely remove it.

Good. I will prepare a patch to remove "usbethaddr" in fdt_fixup_ethernet().

Regards,
Bin

Tom Rini

2015-10-30 02:25 UTC #8 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Quoted 45 lines · Bin Meng

Hi Joe,
Quoted 41 lines · Joe Hershberger
Hi Bin,
Quoted 37 lines · Bin Meng
Hi Joe,
Quoted 18 lines · Joe Hershberger
Hi Bin,
Quoted 15 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Quoted 8 lines
Quoted 6 lines
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.
[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html

Quoted 7 lines

Quoted 5 lines
I'm in agreement with you. See here:
http://lists.denx.de/pipermail/u-boot/2015-July/218601.html
It would be great if you completely remove it.
Good. I will prepare a patch to remove "usbethaddr" in fdt_fixup_ethernet().

So long as you don't break the boards that need it :)

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151029/8038dc11/attachment.sig>

Bin Meng

author

2015-10-30 02:34 UTC #9 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

Hi Tom,

Quoted 49 lines · Tom Rini

Quoted 48 lines · Bin Meng
Hi Joe,
Quoted 44 lines · Joe Hershberger
Hi Bin,
Quoted 29 lines · Bin Meng
Hi Joe,
Quoted 25 lines · Joe Hershberger
Hi Bin,
Quoted 21 lines · Bin Meng
Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.
Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>
Quoted 8 lines
Quoted 1 line
Acked-by Joe Hershberger <[email protected]>
Thanks! Do you have any comments regarding to "usbethaddr" env that I raised here [1]? I originally wanted to remove "usbethaddr" handling completely in fdt_fixup_ethernet(), but did not do that when I submitted this patch.
[1] http://lists.denx.de/pipermail/u-boot/2015-October/231791.html
I'm in agreement with you. See here:
http://lists.denx.de/pipermail/u-boot/2015-July/218601.html

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
It would be great if you completely remove it.
Good. I will prepare a patch to remove "usbethaddr" in fdt_fixup_ethernet().
So long as you don't break the boards that need it :)

I will only remove the MAC address fix up in fdt_fixup_ethernet(). This needs these affected boards to configure "ethaddr" in their environment. For other instances of "usbethaddr" in U-Boot, they should be removed when these drivers/boards have been converted to driver model.

Regards,
Bin

Tom Rini

2015-10-29 01:43 UTC #10 lore raw

PATCH 1/1 fdt: Rewrite the logic in fdt_fixup_ethernet()

In reply to Bin Meng

Quoted 15 lines · Bin Meng

Currently in fdt_fixup_ethernet() the MAC address fix up is handled in a loop of which the exit condition is to test the "eth%daddr" env is not NULL. However this creates unnecessary constrains that those "eth%daddr" env variables must be sequential even if "ethernet%d" does not start from 0 in the "/aliases" node. For example, with "/aliases" node below:
    aliases {
        ethernet3 = &enet3;
        ethernet4 = &enet4;
    };
"ethaddr", "eth1addr", "eth2addr" must exist in order to fix up ethernet3's MAC address successfully.

Quoted 6 lines

Now we change the loop logic to iterate the properties in the "/aliases" node. For each property, test if it is in a format of "ethernet%d", then get its MAC address from corresponding "eth%daddr" env and fix it up in the dtb.
Signed-off-by Bin Meng <[email protected]>

Reviewed-by Tom Rini <[email protected]>

Signature · 8 lines

Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20151028/3a02adb3/attachment.sig>