[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com --- net/eth-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/eth-uclass.c b/net/eth-uclass.c index e14695c0f1..3497a17db6 100644 --- a/net/eth-uclass.c +++ b/net/eth-uclass.c @@ -593,8 +593,8 @@ static int eth_pre_remove(struct udevice *dev) return 0; }
-UCLASS_DRIVER(eth) = { - .name = "eth", +UCLASS_DRIVER(ethernet) = { + .name = "ethernet", .id = UCLASS_ETH, .post_bind = eth_post_bind, .pre_unbind = eth_pre_unbind,
On Thu, 7 Jan 2021 at 19:53, David Wu david.wu@rock-chips.com wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com
net/eth-uclass.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
Reviewed-by: Simon Glass sjg@chromium.org
On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
On Tue, Jan 19, 2021 at 03:01:38PM -0500, Tom Rini wrote:
On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
I'm reverting this change as it breaks a number of tests that need to be updated to match on the new name.
Am 2021-01-19 21:40, schrieb Tom Rini:
On Tue, Jan 19, 2021 at 03:01:38PM -0500, Tom Rini wrote:
On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
I'm reverting this change as it breaks a number of tests that need to be updated to match on the new name.
David, are you planning to submit a new version? If I'm not mistaken, the following changes should be enought to make the tests pass again:
--- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -14,9 +14,9 @@
aliases { console = &uart0; - eth0 = "/eth@10002000"; - eth3 = ð_3; - eth5 = ð_5; + ethernet0 = "/eth@10002000"; + ethernet3 = ð_3; + ethernet5 = ð_5; gpio1 = &gpio_a; gpio2 = &gpio_b; gpio3 = &gpio_c;
In commit cc32fd911aa9 ("arm: dts: ls1028a: Add Ethernet switch node and dependencies") there was recently an additon to a board which actually uses these aliases. So this patch should change them too.
I was actually under the impression that the alias was "ethernetN" and added them to my board, see arch/arm/fsl-ls1028a-kontron*u-boot.dtsi. Seems like I wasn't the first one which was mistaken:
$ grep "ethernet. =" arch/**/*u-boot.dtsi arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet2 = &enetc2; arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet3 = &enetc6; arch/arm/dts/fsl-ls1028a-kontron-sl28-var1-u-boot.dtsi: ethernet0 = &enetc1; arch/arm/dts/fsl-ls1028a-kontron-sl28-var3-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet1 = &enetc1; arch/arm/dts/k3-am654-base-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi: ethernet1 = &ksz8851;
So at the moment I'm not sure if I should fix my dtsi files to use the ethN aliases or if I should just wait because this patch will make it into u-boot soon.
I could also pick up this patch, amend it and resubmit it myself.
-michael
On Wed, Feb 24, 2021 at 06:21:33PM +0100, Michael Walle wrote:
Am 2021-01-19 21:40, schrieb Tom Rini:
On Tue, Jan 19, 2021 at 03:01:38PM -0500, Tom Rini wrote:
On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
I'm reverting this change as it breaks a number of tests that need to be updated to match on the new name.
David, are you planning to submit a new version? If I'm not mistaken, the following changes should be enought to make the tests pass again:
--- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -14,9 +14,9 @@
aliases { console = &uart0;
eth0 = "/eth@10002000";eth3 = ð_3;eth5 = ð_5;
ethernet0 = "/eth@10002000";ethernet3 = ð_3;ethernet5 = ð_5; gpio1 = &gpio_a; gpio2 = &gpio_b; gpio3 = &gpio_c;In commit cc32fd911aa9 ("arm: dts: ls1028a: Add Ethernet switch node and dependencies") there was recently an additon to a board which actually uses these aliases. So this patch should change them too.
I was actually under the impression that the alias was "ethernetN" and added them to my board, see arch/arm/fsl-ls1028a-kontron*u-boot.dtsi. Seems like I wasn't the first one which was mistaken:
$ grep "ethernet. =" arch/**/*u-boot.dtsi arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet2 = &enetc2; arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet3 = &enetc6; arch/arm/dts/fsl-ls1028a-kontron-sl28-var1-u-boot.dtsi: ethernet0 = &enetc1; arch/arm/dts/fsl-ls1028a-kontron-sl28-var3-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet1 = &enetc1; arch/arm/dts/k3-am654-base-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi: ethernet1 = &ksz8851;
So at the moment I'm not sure if I should fix my dtsi files to use the ethN aliases or if I should just wait because this patch will make it into u-boot soon.
I could also pick up this patch, amend it and resubmit it myself.
-michael
Sorry, not sure that I understand. We are talking about the following code path, right?
device_bind_common -> dev_read_alias_seq -> of_alias_get_id -> strcmp(app->stem, stem)
Why on earth is it called "stem" if strcmp is used?
I'm not sure if there's any C standard library for string prefix comparison, but at least I know iproute2 uses a custom function (copy-pasted below):
-----------------------------[ cut here ]----------------------------- #include <stdbool.h> #include <stdio.h> #include <string.h>
/* Returns false if 'prefix' is a not empty prefix of 'string'. */ bool matches(const char *prefix, const char *string) { if (!*prefix) return true; while (*string && *prefix == *string) { prefix++; string++; }
return !!*prefix; }
int main(void) { char *str1 = "eth"; char *str2 = "ethernet";
printf("strcmp returns %d\n", strcmp(str1, str2)); printf("matches returns %d\n", matches(str2, str1)); printf("reverse matches returns %d\n", matches(str1, str2));
return 0; } -----------------------------[ cut here ]-----------------------------
strcmp returns -101 matches returns 0
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I would not, under any circumstance, break compatibility with "eth" alias names.
Am 2021-02-24 19:14, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 06:21:33PM +0100, Michael Walle wrote:
Am 2021-01-19 21:40, schrieb Tom Rini:
On Tue, Jan 19, 2021 at 03:01:38PM -0500, Tom Rini wrote:
On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
I'm reverting this change as it breaks a number of tests that need to be updated to match on the new name.
David, are you planning to submit a new version? If I'm not mistaken, the following changes should be enought to make the tests pass again:
--- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -14,9 +14,9 @@
aliases { console = &uart0;
eth0 = "/eth@10002000";eth3 = ð_3;eth5 = ð_5;
ethernet0 = "/eth@10002000";ethernet3 = ð_3;ethernet5 = ð_5; gpio1 = &gpio_a; gpio2 = &gpio_b; gpio3 = &gpio_c;In commit cc32fd911aa9 ("arm: dts: ls1028a: Add Ethernet switch node and dependencies") there was recently an additon to a board which actually uses these aliases. So this patch should change them too.
I was actually under the impression that the alias was "ethernetN" and added them to my board, see arch/arm/fsl-ls1028a-kontron*u-boot.dtsi. Seems like I wasn't the first one which was mistaken:
$ grep "ethernet. =" arch/**/*u-boot.dtsi arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet2 = &enetc2; arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet3 = &enetc6; arch/arm/dts/fsl-ls1028a-kontron-sl28-var1-u-boot.dtsi: ethernet0 = &enetc1; arch/arm/dts/fsl-ls1028a-kontron-sl28-var3-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet1 = &enetc1; arch/arm/dts/k3-am654-base-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi: ethernet1 = &ksz8851;
So at the moment I'm not sure if I should fix my dtsi files to use the ethN aliases or if I should just wait because this patch will make it into u-boot soon.
I could also pick up this patch, amend it and resubmit it myself.
-michael
Sorry, not sure that I understand. We are talking about the following code path, right?
device_bind_common -> dev_read_alias_seq -> of_alias_get_id -> strcmp(app->stem, stem)
Why on earth is it called "stem" if strcmp is used?
I'm not sure if there's any C standard library for string prefix comparison, but at least I know iproute2 uses a custom function (copy-pasted below):
I'm not sure if it should really be just a prefix match but a full string compare.
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I guess it was intended the other way around, to rename the "ethN" aliases to "ethernetN". The latter are used way more in u-boot's device trees and linux' device trees just use ethernetN aliases, though I'm not sure where they are used (can't find any of_alias_get_id("ethernet") in linux). So this makes sense, no?
I would not, under any circumstance, break compatibility with "eth" alias names.
Assuming that ethN is not used expect in u-boot device trees, how would we break backwards compatibility? Also it seems that the only users of the ethN aliases are the sandbox and u-boot's DSA.
-michael
On Wed, Feb 24, 2021 at 08:26:14PM +0100, Michael Walle wrote:
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I guess it was intended the other way around, to rename the "ethN" aliases to "ethernetN". The latter are used way more in u-boot's device trees and linux' device trees just use ethernetN aliases, though I'm not sure where they are used (can't find any of_alias_get_id("ethernet") in linux). So this makes sense, no?
Bad phrasing, I meant to ask if it was intended to _still_ match on "eth" aliases.
By the way, Linux drivers use a combination of "ethernet" and "eth": https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/samsung/s... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/s...
I would not, under any circumstance, break compatibility with "eth" alias names.
Assuming that ethN is not used expect in u-boot device trees, how would we break backwards compatibility? Also it seems that the only users of the ethN aliases are the sandbox and u-boot's DSA.
I took another look, I was under the impression that fdtdec_set_ethernet_mac_address searches by "eth" in the /aliases node of the linux device tree, but it searches by "ethernet". Fair then.
Tough luck in that case for those who got it right, ugh I mean wrong, by actually looking at the code to see what it searches for?
So yeah, this patch is waiting for someone to update the existing aliases, and you seem to be a valid candidate. Although it's low impact, and it's all in-tree so it isn't breaking any interface, it is up to you if you value consistency more than correctness. I would probably fix just the device tree you care about to use "eth" instead of "ethernet", that seems the most localized solution to a concrete problem, then you could convert the uclass name to "ethernet" and therefore fix all the other users at once (and at the same time update the kontron boards). The reason why I would leave that at the end is because when you 'fix' stuff that isn't broken, nice surprises are always bound to happen, and I don't think you want your board to still remain broken at the end of some bug-canceling-another-bug-on-board-you-don't-have debugging headaches.
Am 2021-02-24 22:34, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 08:26:14PM +0100, Michael Walle wrote:
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I guess it was intended the other way around, to rename the "ethN" aliases to "ethernetN". The latter are used way more in u-boot's device trees and linux' device trees just use ethernetN aliases, though I'm not sure where they are used (can't find any of_alias_get_id("ethernet") in linux). So this makes sense, no?
Bad phrasing, I meant to ask if it was intended to _still_ match on "eth" aliases.
By the way, Linux drivers use a combination of "ethernet" and "eth": https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/samsung/s... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/s...
I would not, under any circumstance, break compatibility with "eth" alias names.
Assuming that ethN is not used expect in u-boot device trees, how would we break backwards compatibility? Also it seems that the only users of the ethN aliases are the sandbox and u-boot's DSA.
I took another look, I was under the impression that fdtdec_set_ethernet_mac_address searches by "eth" in the /aliases node of the linux device tree, but it searches by "ethernet". Fair then.
fdtdec_set_ethernet_mac_address() seems to be "only" used by tegra?
Tough luck in that case for those who got it right, ugh I mean wrong, by actually looking at the code to see what it searches for?
So yeah, this patch is waiting for someone to update the existing aliases, and you seem to be a valid candidate. Although it's low impact, and it's all in-tree so it isn't breaking any interface, it is up to you if you value consistency more than correctness. I would probably fix just the device tree you care about to use "eth" instead of "ethernet", that seems the most localized solution to a concrete problem, then you could convert the uclass name to "ethernet" and therefore fix all the other users at once (and at the same time update the kontron boards). The reason why I would leave that at the end is because when you 'fix' stuff that isn't broken, nice surprises are always bound to happen, and I don't think you want your board to still remain broken at the end of some bug-canceling-another-bug-on-board-you-don't-have debugging headaches.
Fair enough. And I might fix em, the only user is the kontron board with the DSA support which wasn't picked up yet. But then yeah, I can't stand it that one sl28 variant has ethernetN aliases and the other as ethN aliases *g*
Btw another user might be https://patchwork.ozlabs.org/project/uboot/patch/20210224163044.17017-1-mich...
Thats actually why I noticed the inconsistency and found this patch.
-michael
Hi Michael,
On Wed, 24 Feb 2021 at 17:51, Michael Walle michael@walle.cc wrote:
Am 2021-02-24 22:34, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 08:26:14PM +0100, Michael Walle wrote:
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I guess it was intended the other way around, to rename the "ethN" aliases to "ethernetN". The latter are used way more in u-boot's device trees and linux' device trees just use ethernetN aliases, though I'm not sure where they are used (can't find any of_alias_get_id("ethernet") in linux). So this makes sense, no?
Bad phrasing, I meant to ask if it was intended to _still_ match on "eth" aliases.
By the way, Linux drivers use a combination of "ethernet" and "eth": https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/samsung/s... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/s...
I would not, under any circumstance, break compatibility with "eth" alias names.
Assuming that ethN is not used expect in u-boot device trees, how would we break backwards compatibility? Also it seems that the only users of the ethN aliases are the sandbox and u-boot's DSA.
I took another look, I was under the impression that fdtdec_set_ethernet_mac_address searches by "eth" in the /aliases node of the linux device tree, but it searches by "ethernet". Fair then.
fdtdec_set_ethernet_mac_address() seems to be "only" used by tegra?
Tough luck in that case for those who got it right, ugh I mean wrong, by actually looking at the code to see what it searches for?
So yeah, this patch is waiting for someone to update the existing aliases, and you seem to be a valid candidate. Although it's low impact, and it's all in-tree so it isn't breaking any interface, it is up to you if you value consistency more than correctness. I would probably fix just the device tree you care about to use "eth" instead of "ethernet", that seems the most localized solution to a concrete problem, then you could convert the uclass name to "ethernet" and therefore fix all the other users at once (and at the same time update the kontron boards). The reason why I would leave that at the end is because when you 'fix' stuff that isn't broken, nice surprises are always bound to happen, and I don't think you want your board to still remain broken at the end of some bug-canceling-another-bug-on-board-you-don't-have debugging headaches.
Fair enough. And I might fix em, the only user is the kontron board with the DSA support which wasn't picked up yet. But then yeah, I can't stand it that one sl28 variant has ethernetN aliases and the other as ethN aliases *g*
Btw another user might be https://patchwork.ozlabs.org/project/uboot/patch/20210224163044.17017-1-mich...
Thats actually why I noticed the inconsistency and found this patch.
If we have the choice, 'eth' is shorter = better.
Regards, Simon
Hi Simon,
Am 2021-02-25 03:31, schrieb Simon Glass:
On Wed, 24 Feb 2021 at 17:51, Michael Walle michael@walle.cc wrote:
Am 2021-02-24 22:34, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 08:26:14PM +0100, Michael Walle wrote:
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I guess it was intended the other way around, to rename the "ethN" aliases to "ethernetN". The latter are used way more in u-boot's device trees and linux' device trees just use ethernetN aliases, though I'm not sure where they are used (can't find any of_alias_get_id("ethernet") in linux). So this makes sense, no?
Bad phrasing, I meant to ask if it was intended to _still_ match on "eth" aliases.
By the way, Linux drivers use a combination of "ethernet" and "eth": https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/samsung/s... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/s...
I would not, under any circumstance, break compatibility with "eth" alias names.
Assuming that ethN is not used expect in u-boot device trees, how would we break backwards compatibility? Also it seems that the only users of the ethN aliases are the sandbox and u-boot's DSA.
I took another look, I was under the impression that fdtdec_set_ethernet_mac_address searches by "eth" in the /aliases node of the linux device tree, but it searches by "ethernet". Fair then.
fdtdec_set_ethernet_mac_address() seems to be "only" used by tegra?
Tough luck in that case for those who got it right, ugh I mean wrong, by actually looking at the code to see what it searches for?
So yeah, this patch is waiting for someone to update the existing aliases, and you seem to be a valid candidate. Although it's low impact, and it's all in-tree so it isn't breaking any interface, it is up to you if you value consistency more than correctness. I would probably fix just the device tree you care about to use "eth" instead of "ethernet", that seems the most localized solution to a concrete problem, then you could convert the uclass name to "ethernet" and therefore fix all the other users at once (and at the same time update the kontron boards). The reason why I would leave that at the end is because when you 'fix' stuff that isn't broken, nice surprises are always bound to happen, and I don't think you want your board to still remain broken at the end of some bug-canceling-another-bug-on-board-you-don't-have debugging headaches.
Fair enough. And I might fix em, the only user is the kontron board with the DSA support which wasn't picked up yet. But then yeah, I can't stand it that one sl28 variant has ethernetN aliases and the other as ethN aliases *g*
Btw another user might be https://patchwork.ozlabs.org/project/uboot/patch/20210224163044.17017-1-mich...
Thats actually why I noticed the inconsistency and found this patch.
If we have the choice, 'eth' is shorter = better.
This is how it looks in the linux tree:
$ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 633 $ grep "eth[0-9].*=.*&" arch/**/*dts{,i}|wc -l 0
This is in the u-boot tree: $ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 261 $ grep "eth[0-9].*=.*&" arch/**/*dts{,i}|wc -l 13
-michael
On Thu, Feb 25, 2021 at 08:57:37AM +0100, Michael Walle wrote:
Hi Simon,
Am 2021-02-25 03:31, schrieb Simon Glass:
On Wed, 24 Feb 2021 at 17:51, Michael Walle michael@walle.cc wrote:
Am 2021-02-24 22:34, schrieb Vladimir Oltean:
On Wed, Feb 24, 2021 at 08:26:14PM +0100, Michael Walle wrote:
Wasn't the intention of David's patch, in fact, to have the new uclass name also match on "eth" aliases? If I'm correct, doesn't this mean we'll have to replace the strcmp with an actual stem check?
I guess it was intended the other way around, to rename the "ethN" aliases to "ethernetN". The latter are used way more in u-boot's device trees and linux' device trees just use ethernetN aliases, though I'm not sure where they are used (can't find any of_alias_get_id("ethernet") in linux). So this makes sense, no?
Bad phrasing, I meant to ask if it was intended to _still_ match on "eth" aliases.
By the way, Linux drivers use a combination of "ethernet" and "eth": https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/samsung/s... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/... https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/s...
I would not, under any circumstance, break compatibility with "eth" alias names.
Assuming that ethN is not used expect in u-boot device trees, how would we break backwards compatibility? Also it seems that the only users of the ethN aliases are the sandbox and u-boot's DSA.
I took another look, I was under the impression that fdtdec_set_ethernet_mac_address searches by "eth" in the /aliases node of the linux device tree, but it searches by "ethernet". Fair then.
fdtdec_set_ethernet_mac_address() seems to be "only" used by tegra?
Tough luck in that case for those who got it right, ugh I mean wrong, by actually looking at the code to see what it searches for?
So yeah, this patch is waiting for someone to update the existing aliases, and you seem to be a valid candidate. Although it's low impact, and it's all in-tree so it isn't breaking any interface, it is up to you if you value consistency more than correctness. I would probably fix just the device tree you care about to use "eth" instead of "ethernet", that seems the most localized solution to a concrete problem, then you could convert the uclass name to "ethernet" and therefore fix all the other users at once (and at the same time update the kontron boards). The reason why I would leave that at the end is because when you 'fix' stuff that isn't broken, nice surprises are always bound to happen, and I don't think you want your board to still remain broken at the end of some bug-canceling-another-bug-on-board-you-don't-have debugging headaches.
Fair enough. And I might fix em, the only user is the kontron board with the DSA support which wasn't picked up yet. But then yeah, I can't stand it that one sl28 variant has ethernetN aliases and the other as ethN aliases *g*
Btw another user might be https://patchwork.ozlabs.org/project/uboot/patch/20210224163044.17017-1-mich...
Thats actually why I noticed the inconsistency and found this patch.
If we have the choice, 'eth' is shorter = better.
This is how it looks in the linux tree:
$ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 633 $ grep "eth[0-9].*=.*&" arch/**/*dts{,i}|wc -l 0
This is in the u-boot tree: $ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 261 $ grep "eth[0-9].*=.*&" arch/**/*dts{,i}|wc -l 13
So "ethernet" it is.
On Thu, Feb 25, 2021 at 4:58 AM Michael Walle michael@walle.cc wrote:
This is how it looks in the linux tree:
$ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 633
Yes, 'ethernet' is the recommended node name as per the Devicetree Specification doc item "2.2.2 Generic Names Recommendation".
On Thu, 25 Feb 2021 at 10:55, Fabio Estevam festevam@gmail.com wrote:
On Thu, Feb 25, 2021 at 4:58 AM Michael Walle michael@walle.cc wrote:
This is how it looks in the linux tree:
$ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 633
Yes, 'ethernet' is the recommended node name as per the Devicetree Specification doc item "2.2.2 Generic Names Recommendation".
OK.
- Simon
On Thu, Feb 25, 2021 at 9:33 PM Simon Glass sjg@chromium.org wrote:
On Thu, 25 Feb 2021 at 10:55, Fabio Estevam festevam@gmail.com wrote:
On Thu, Feb 25, 2021 at 4:58 AM Michael Walle michael@walle.cc wrote:
This is how it looks in the linux tree:
$ grep "ethernet[0-9].*=.*&" arch/**/*dts{,i}|wc -l 633
Yes, 'ethernet' is the recommended node name as per the Devicetree Specification doc item "2.2.2 Generic Names Recommendation".
OK.
- Simon
Ack.
[resend, sorry forgot to put Vladimir in CC]
Am 2021-01-19 21:40, schrieb Tom Rini:
On Tue, Jan 19, 2021 at 03:01:38PM -0500, Tom Rini wrote:
On Fri, Jan 08, 2021 at 10:53:05AM +0800, David Wu wrote:
dev_read_alias_seq() used uc_drv->name compared to alias stem string, Ethernet's alias stem uses "ethernet", which does not match the eth-uclass driver name "eth", can not get the correct index of ethernet alias namer. So it seems change uclass driver name to match the alias stem is a more reasonable way.
Signed-off-by: David Wu david.wu@rock-chips.com Reviewed-by: Simon Glass sjg@chromium.org
Applied to u-boot/master, thanks!
I'm reverting this change as it breaks a number of tests that need to be updated to match on the new name.
David, are you planning to submit a new version? If I'm not mistaken, the following changes should be enought to make the tests pass again:
--- a/arch/sandbox/dts/test.dts +++ b/arch/sandbox/dts/test.dts @@ -14,9 +14,9 @@
aliases { console = &uart0; - eth0 = "/eth@10002000"; - eth3 = ð_3; - eth5 = ð_5; + ethernet0 = "/eth@10002000"; + ethernet3 = ð_3; + ethernet5 = ð_5; gpio1 = &gpio_a; gpio2 = &gpio_b; gpio3 = &gpio_c;
In commit cc32fd911aa9 ("arm: dts: ls1028a: Add Ethernet switch node and dependencies") there was recently an additon to a board which actually uses these aliases. So this patch should change them too.
I was actually under the impression that the alias was "ethernetN" and added them to my board, see arch/arm/fsl-ls1028a-kontron*u-boot.dtsi. Seems like I wasn't the first one which was mistaken:
$ grep "ethernet. =" arch/**/*u-boot.dtsi arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet2 = &enetc2; arch/arm/dts/fsl-ls1028a-kontron-sl28-u-boot.dtsi: ethernet3 = &enetc6; arch/arm/dts/fsl-ls1028a-kontron-sl28-var1-u-boot.dtsi: ethernet0 = &enetc1; arch/arm/dts/fsl-ls1028a-kontron-sl28-var3-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet0 = &enetc0; arch/arm/dts/fsl-ls1028a-kontron-sl28-var4-u-boot.dtsi: ethernet1 = &enetc1; arch/arm/dts/k3-am654-base-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j7200-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/k3-j721e-common-proc-board-u-boot.dtsi: ethernet0 = &cpsw_port1; arch/arm/dts/stm32mp15xx-dhcom-u-boot.dtsi: ethernet1 = &ksz8851;
So at the moment I'm not sure if I should fix my dtsi files to use the ethN aliases or if I should just wait because this patch will make it into u-boot soon.
I could also pick up this patch, amend it and resubmit it myself.
-michael
participants (7)
-
David Wu -
Fabio Estevam -
Michael Walle -
Ramon Fried -
Simon Glass -
Tom Rini -
Vladimir Oltean