diff mbox

[U-Boot,2/4] arm: pxa: always init ethaddr for LP-8x4x

Message ID 1386999720-23460-3-git-send-email-ynvich@gmail.com
State Superseded
Delegated to: Marek Vasut
Headers show

Commit Message

Sergey Yanovich Dec. 14, 2013, 5:41 a.m. UTC
I always used tftp in my test, so the first dm9000 on LP-8x4x was
always properly initialized. However, if the boot doesn't include
network related commands, linux will not find a valid MAC and will
complain.

No longer.

Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
---
 board/icpdas/lp8x4x/lp8x4x.c | 8 ++++++++
 1 file changed, 8 insertions(+)

Comments

Marek Vasut Dec. 14, 2013, 12:30 p.m. UTC | #1
On Saturday, December 14, 2013 at 06:41:58 AM, Sergei Ianovich wrote:
> I always used tftp in my test, so the first dm9000 on LP-8x4x was
> always properly initialized. However, if the boot doesn't include
> network related commands, linux will not find a valid MAC and will
> complain.
> 
> No longer.
> 
> Signed-off-by: Sergei Ianovich <ynvich@gmail.com>
> ---
>  board/icpdas/lp8x4x/lp8x4x.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/board/icpdas/lp8x4x/lp8x4x.c b/board/icpdas/lp8x4x/lp8x4x.c
> index 1b68ef3..8396caa 100644
> --- a/board/icpdas/lp8x4x/lp8x4x.c
> +++ b/board/icpdas/lp8x4x/lp8x4x.c
> @@ -112,10 +112,18 @@ void usb_board_stop(void)
>  #ifdef CONFIG_DRIVER_DM9000
>  void lp8x4x_eth1_mac_init(void)
>  {
> +	u8 ethaddr[8];
>  	u8 eth1addr[8];
>  	int i;
>  	u8 reg;
> 
> +	eth_getenv_enetaddr_by_index("eth", 0, ethaddr);
> +	if (is_valid_ether_addr(ethaddr)) {
> +		for (i = 0, reg = 0x10; i < 6; i++, reg++) {
> +			writeb(reg, (u8 *)(DM9000_IO));
> +			writeb(ethaddr[i], (u8 *)(DM9000_DATA));
> +		}
> +	}
>  	eth_getenv_enetaddr_by_index("eth", 1, eth1addr);
>  	if (!is_valid_ether_addr(eth1addr))
>  		return;

Please pass the ethernet address via DT, will that not work for you ?

Best regards,
Marek Vasut
Sergey Yanovich Dec. 14, 2013, 3:39 p.m. UTC | #2
On Sat, 2013-12-14 at 13:30 +0100, Marek Vasut wrote:
> On Saturday, December 14, 2013 at 06:41:58 AM, Sergei Ianovich wrote:
> > I always used tftp in my test, so the first dm9000 on LP-8x4x was
> > always properly initialized. However, if the boot doesn't include
> > network related commands, linux will not find a valid MAC and will
> > complain.
> > 
> Please pass the ethernet address via DT, will that not work for you ?

This will require a custom dtb object for every device recompiled every
time kernel is upgraded. The current way is to set once per device in
U-Boot environment variable, which doesn't need to change if kernel
changes.

I strongly believe the current way is easier.
Marek Vasut Dec. 14, 2013, 5:12 p.m. UTC | #3
On Saturday, December 14, 2013 at 04:39:00 PM, Sergei Ianovich wrote:
> On Sat, 2013-12-14 at 13:30 +0100, Marek Vasut wrote:
> > On Saturday, December 14, 2013 at 06:41:58 AM, Sergei Ianovich wrote:
> > > I always used tftp in my test, so the first dm9000 on LP-8x4x was
> > > always properly initialized. However, if the boot doesn't include
> > > network related commands, linux will not find a valid MAC and will
> > > complain.
> > 
> > Please pass the ethernet address via DT, will that not work for you ?
> 
> This will require a custom dtb object for every device recompiled every
> time kernel is upgraded. The current way is to set once per device in
> U-Boot environment variable, which doesn't need to change if kernel
> changes.
> 
> I strongly believe the current way is easier.

I disagree :)

IF you set 'ethaddr' variable in U-Boot THEN
 U-Boot will patch this 'ethaddr' value into your DT. The /aliases/ethernet0 
 node will be augmented with a new property 'local-mac-address', which will 
 contain the MAC address from 'ethaddr' . The kernel will use this as the MAC
 address for that particular ethernet interface afterwards.

NOTE: It is very important to have the alias set, it has to point to your 
ethernet device. A good example in Linux's arch/arm/boot/dts/imx28.dtsi, which 
even has two ethernet interfaces. Notice each of them has an alias.
NOTE: If you have two interfaces, then 'eth1addr' is patches into 
/aliases/ethernet1 etc.

The only problem here is the non-DT kernel. Do you need to support that? Is 
there no other way to pass MAC address of an ethernet interface to Linux but 
programming it into the ethernet interface itself ?

Best regards,
Marek Vasut
Sergey Yanovich Dec. 14, 2013, 8:53 p.m. UTC | #4
On Sat, 2013-12-14 at 18:12 +0100, Marek Vasut wrote:
> On Saturday, December 14, 2013 at 04:39:00 PM, Sergei Ianovich wrote:
> > I strongly believe the current way is easier.
> 
> I disagree :)
> 
> IF you set 'ethaddr' variable in U-Boot THEN
>  U-Boot will patch this 'ethaddr' value into your DT. The /aliases/ethernet0 
>  node will be augmented with a new property 'local-mac-address', which will 
>  contain the MAC address from 'ethaddr' . The kernel will use this as the MAC
>  address for that particular ethernet interface afterwards.
> 
> NOTE: It is very important to have the alias set, it has to point to your 
> ethernet device. A good example in Linux's arch/arm/boot/dts/imx28.dtsi, which 
> even has two ethernet interfaces. Notice each of them has an alias.
> NOTE: If you have two interfaces, then 'eth1addr' is patches into 
> /aliases/ethernet1 etc.

Thanks for explaining. It works. This is COOL!

> The only problem here is the non-DT kernel. Do you need to support that? Is 
> there no other way to pass MAC address of an ethernet interface to Linux but 
> programming it into the ethernet interface itself ?

Hardware vendor uses U-Boot environment values to init MAC-addresses. I
think it is even worse than what I was doing in the patch.

I don't think we need to support non-DT kernel. So we drop the
patch.
Marek Vasut Dec. 14, 2013, 8:57 p.m. UTC | #5
On Saturday, December 14, 2013 at 09:53:48 PM, Sergei Ianovich wrote:
> On Sat, 2013-12-14 at 18:12 +0100, Marek Vasut wrote:
> > On Saturday, December 14, 2013 at 04:39:00 PM, Sergei Ianovich wrote:
> > > I strongly believe the current way is easier.
> > 
> > I disagree :)
> > 
> > IF you set 'ethaddr' variable in U-Boot THEN
> > 
> >  U-Boot will patch this 'ethaddr' value into your DT. The
> >  /aliases/ethernet0 node will be augmented with a new property
> >  'local-mac-address', which will contain the MAC address from 'ethaddr'
> >  . The kernel will use this as the MAC address for that particular
> >  ethernet interface afterwards.
> > 
> > NOTE: It is very important to have the alias set, it has to point to your
> > ethernet device. A good example in Linux's arch/arm/boot/dts/imx28.dtsi,
> > which even has two ethernet interfaces. Notice each of them has an
> > alias. NOTE: If you have two interfaces, then 'eth1addr' is patches into
> > /aliases/ethernet1 etc.
> 
> Thanks for explaining. It works. This is COOL!

Glad it helps :)

> > The only problem here is the non-DT kernel. Do you need to support that?
> > Is there no other way to pass MAC address of an ethernet interface to
> > Linux but programming it into the ethernet interface itself ?
> 
> Hardware vendor uses U-Boot environment values to init MAC-addresses. I
> think it is even worse than what I was doing in the patch.
> 
> I don't think we need to support non-DT kernel. So we drop the
> patch.

OK, that's nice :)

Best regards,
Marek Vasut
Marek Vasut Dec. 15, 2013, 5:17 a.m. UTC | #6
On Sunday, December 15, 2013 at 12:53:18 AM, Sergei Ianovich wrote:
> On Sat, 2013-12-14 at 21:57 +0100, Marek Vasut wrote:
> > > I don't think we need to support non-DT kernel. So we drop the
> > > patch.
> > 
> > OK, that's nice :)
> 
> It may even make sense to remove the code that initializes eth1, right?

Which code?

Best regards,
Marek Vasut
Sergey Yanovich Dec. 15, 2013, 9:44 a.m. UTC | #7
On Sun, 2013-12-15 at 06:17 +0100, Marek Vasut wrote:
> On Sunday, December 15, 2013 at 12:53:18 AM, Sergei Ianovich wrote:
> > It may even make sense to remove the code that initializes eth1, right?
> 
> Which code?

I mean:

lp8x4x_eth1_mac_init() in board/icpdas/lp8x4x/lp8x4x.c

and its invocation in board_eth_init()
Marek Vasut Dec. 15, 2013, 3:05 p.m. UTC | #8
On Sunday, December 15, 2013 at 10:44:06 AM, Sergei Ianovich wrote:
> On Sun, 2013-12-15 at 06:17 +0100, Marek Vasut wrote:
> > On Sunday, December 15, 2013 at 12:53:18 AM, Sergei Ianovich wrote:
> > > It may even make sense to remove the code that initializes eth1, right?
> > 
> > Which code?
> 
> I mean:
> 
> lp8x4x_eth1_mac_init() in board/icpdas/lp8x4x/lp8x4x.c
> 
> and its invocation in board_eth_init()

Heh, yes. The DT will solve it through the ethernet1 alias, just set the 
eth1addr and test it to make sure please.

Best regards,
Marek Vasut
Sergey Yanovich Dec. 15, 2013, 6:43 p.m. UTC | #9
On Sun, 2013-12-15 at 16:05 +0100, Marek Vasut wrote:
> Heh, yes. The DT will solve it through the ethernet1 alias, just set the 
> eth1addr and test it to make sure please.

It works. I've updated the dts in kernel for next version of patch
series.

I will update this patch for v2.
diff mbox

Patch

diff --git a/board/icpdas/lp8x4x/lp8x4x.c b/board/icpdas/lp8x4x/lp8x4x.c
index 1b68ef3..8396caa 100644
--- a/board/icpdas/lp8x4x/lp8x4x.c
+++ b/board/icpdas/lp8x4x/lp8x4x.c
@@ -112,10 +112,18 @@  void usb_board_stop(void)
 #ifdef CONFIG_DRIVER_DM9000
 void lp8x4x_eth1_mac_init(void)
 {
+	u8 ethaddr[8];
 	u8 eth1addr[8];
 	int i;
 	u8 reg;
 
+	eth_getenv_enetaddr_by_index("eth", 0, ethaddr);
+	if (is_valid_ether_addr(ethaddr)) {
+		for (i = 0, reg = 0x10; i < 6; i++, reg++) {
+			writeb(reg, (u8 *)(DM9000_IO));
+			writeb(ethaddr[i], (u8 *)(DM9000_DATA));
+		}
+	}
 	eth_getenv_enetaddr_by_index("eth", 1, eth1addr);
 	if (!is_valid_ether_addr(eth1addr))
 		return;