| Message ID | 20210108025305.18664-1-david.wu@rock-chips.com |
|---|---|
| State | Accepted |
| Commit | 1231184caacad32c180d7e2338a645f7dfe9571a |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v1] net: eth-uclass: Change uclass driver name to ethernet | expand |
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
[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
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.
On Wed, Feb 24, 2021 at 08:14:17PM +0200, Vladimir Oltean wrote: > strcmp returns -101 > matches returns 0 Damn, I posted the wrong output. Of course the program returns: strcmp returns -101 matches returns 1 reverse matches returns 0
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/sxgbe/sxgbe_platform.c#L43 https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/genet/bcmmii.c#L441 https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#L436 > > 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/sxgbe/sxgbe_platform.c#L43 > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/genet/bcmmii.c#L441 > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#L436 > >> > 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-michael@walle.cc/ 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/sxgbe/sxgbe_platform.c#L43 > > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/genet/bcmmii.c#L441 > > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#L436 > > > >> > 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-michael@walle.cc/ > > 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/sxgbe/sxgbe_platform.c#L43 >> > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/genet/bcmmii.c#L441 >> > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#L436 >> > >> >> > 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-michael@walle.cc/ >> >> 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/sxgbe/sxgbe_platform.c#L43 > > > > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/broadcom/genet/bcmmii.c#L441 > > > > https://github.com/torvalds/linux/blob/master/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c#L436 > > > > > > > >> > 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-michael@walle.cc/ > > > > > > 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.
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,
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(-)