[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[David Wu]

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,
-- 
2.19.1

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Simon Glass]

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>

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Tom Rini]

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!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210119/1de95bf7/attachment.sig>

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210119/4ffcbc0b/attachment.sig>

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Michael Walle]

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 at 10002000";
-               eth3 = &eth_3;
-               eth5 = &eth_5;
+               ethernet0 = "/eth at 10002000";
+               ethernet3 = &eth_3;
+               ethernet5 = &eth_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

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[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 at 10002000";
> -               eth3 = &eth_3;
> -               eth5 = &eth_5;
> +               ethernet0 = "/eth at 10002000";
> +               ethernet3 = &eth_3;
> +               ethernet5 = &eth_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.

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Vladimir Oltean]

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

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Michael Walle]

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 at 10002000";
>> -               eth3 = &eth_3;
>> -               eth5 = &eth_5;
>> +               ethernet0 = "/eth at 10002000";
>> +               ethernet3 = &eth_3;
>> +               ethernet5 = &eth_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

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

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

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.

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Michael Walle]

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 at walle.cc/

Thats actually why I noticed the inconsistency and found this patch.

-michael

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Simon Glass]

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 at walle.cc/
>
> Thats actually why I noticed the inconsistency and found this patch.

If we have the choice, 'eth' is shorter  = better.

Regards,
Simon

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Michael Walle]

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 at 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

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Tom Rini]

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

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 659 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20210225/c3747b26/attachment.sig>

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Fabio Estevam]

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

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Simon Glass]

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

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Ramon Fried]

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.

[PATCH v1] net: eth-uclass: Change uclass driver name to ethernet

[Michael Walle]

[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 at 10002000";
-               eth3 = &eth_3;
-               eth5 = &eth_5;
+               ethernet0 = "/eth at 10002000";
+               ethernet3 = &eth_3;
+               ethernet5 = &eth_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