[PATCH] ARM: atags: add support for Marvell's u-boot

[PATCH] ARM: atags: add support for Marvell's u-boot

[Thomas Petazzoni]

From: Willy Tarreau <w@1wt.eu>

Marvell uses a specific atag in its u-boot which includes among other
information the MAC addresses for up to 4 network interfaces. This
atag is parsed, and if found the MAC addresses are extracted there and
assigned to node aliases eth0..3 with the name "mac-address".

This allows, with the non-DT capable bootloaders currently used on
most Marvell platforms, to get a proper MAC address for the different
interfaces.

Signed-off-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/compressed/atags_to_fdt.c |  8 +++++++-
 arch/arm/include/uapi/asm/setup.h       | 18 ++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
index aabc02a..7f405ed 100644
--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -16,7 +16,7 @@ static int node_offset(void *fdt, const char *node_path)
 }
 
 static int setprop(void *fdt, const char *node_path, const char *property,
-		   uint32_t *val_array, int size)
+		   void *val_array, int size)
 {
 	int offset = node_offset(fdt, node_path);
 	if (offset < 0)
@@ -147,6 +147,12 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 					initrd_start);
 			setprop_cell(fdt, "/chosen", "linux,initrd-end",
 					initrd_start + initrd_size);
+		} else if (atag->hdr.tag == ATAG_MV_UBOOT) {
+			/* This ATAG provides up to 4 MAC addresses */
+			setprop(fdt, "eth0", "mac-address", atag->u.mv_uboot.macaddr[0], 6);
+			setprop(fdt, "eth1", "mac-address", atag->u.mv_uboot.macaddr[1], 6);
+			setprop(fdt, "eth2", "mac-address", atag->u.mv_uboot.macaddr[2], 6);
+			setprop(fdt, "eth3", "mac-address", atag->u.mv_uboot.macaddr[3], 6);
 		}
 	}
 
diff --git a/arch/arm/include/uapi/asm/setup.h b/arch/arm/include/uapi/asm/setup.h
index 979ff40..5588b1f 100644
--- a/arch/arm/include/uapi/asm/setup.h
+++ b/arch/arm/include/uapi/asm/setup.h
@@ -143,6 +143,19 @@ struct tag_memclk {
 	__u32 fmemclk;
 };
 
+/* Marvell uboot parameters */
+#define ATAG_MV_UBOOT	0x41000403
+
+struct tag_mv_uboot {
+	__u32 uboot_version;
+	__u32 tclk;
+	__u32 sysclk;
+	__u32 isusbhost;
+	__u8  macaddr[4][6];
+	__u16 mtu[4];
+	__u32 nand_ecc;
+};
+
 struct tag {
 	struct tag_header hdr;
 	union {
@@ -165,6 +178,11 @@ struct tag {
 		 * DC21285 specific
 		 */
 		struct tag_memclk	memclk;
+
+		/*
+		 * Marvell specific
+		 */
+		struct tag_mv_uboot	mv_uboot;
 	} u;
 };
 
-- 
1.8.1.2

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Mon, Jun 03, 2013 at 06:45:37PM +0200, Thomas Petazzoni wrote:
> From: Willy Tarreau <w@1wt.eu>
> 
> Marvell uses a specific atag in its u-boot which includes among other
> information the MAC addresses for up to 4 network interfaces. This
> atag is parsed, and if found the MAC addresses are extracted there and
> assigned to node aliases eth0..3 with the name "mac-address".
> 
> This allows, with the non-DT capable bootloaders currently used on
> most Marvell platforms, to get a proper MAC address for the different
> interfaces.
> 
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/compressed/atags_to_fdt.c |  8 +++++++-
>  arch/arm/include/uapi/asm/setup.h       | 18 ++++++++++++++++++
>  2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
> index aabc02a..7f405ed 100644
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -16,7 +16,7 @@ static int node_offset(void *fdt, const char *node_path)
>  }
>  
>  static int setprop(void *fdt, const char *node_path, const char *property,
> -		   uint32_t *val_array, int size)
> +		   void *val_array, int size)
>  {
>  	int offset = node_offset(fdt, node_path);
>  	if (offset < 0)
> @@ -147,6 +147,12 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  					initrd_start);
>  			setprop_cell(fdt, "/chosen", "linux,initrd-end",
>  					initrd_start + initrd_size);
> +		} else if (atag->hdr.tag == ATAG_MV_UBOOT) {
> +			/* This ATAG provides up to 4 MAC addresses */
> +			setprop(fdt, "eth0", "mac-address", atag->u.mv_uboot.macaddr[0], 6);
> +			setprop(fdt, "eth1", "mac-address", atag->u.mv_uboot.macaddr[1], 6);
> +			setprop(fdt, "eth2", "mac-address", atag->u.mv_uboot.macaddr[2], 6);
> +			setprop(fdt, "eth3", "mac-address", atag->u.mv_uboot.macaddr[3], 6);

Won't this override a user editing the 'local-mac-address' property in
the dts?  Since we're dealing with appended dtbs, there's a higher
probability of the user doing such a thing.

I'll admit it's a corner case, but I'd like to make sure we've thought
through the use cases and aren't making things more complicated than
necessary.

thx,

Jason.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Thomas Petazzoni]

Dear Jason Cooper,

On Mon, 3 Jun 2013 12:57:53 -0400, Jason Cooper wrote:

> Won't this override a user editing the 'local-mac-address' property in
> the dts?  Since we're dealing with appended dtbs, there's a higher
> probability of the user doing such a thing.
> 
> I'll admit it's a corner case, but I'd like to make sure we've thought
> through the use cases and aren't making things more complicated than
> necessary.

I don't have a strong opinion about this one, so if it's seen as being
necessary to preserve a potentially existing 'local-mac-address', then
I'll add the necessary checks to only update the property if it doesn't
already exist.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] ARM: atags: add support for Marvell's u-boot

[Russell King - ARM Linux]

On Mon, Jun 03, 2013 at 06:45:37PM +0200, Thomas Petazzoni wrote:
> From: Willy Tarreau <w@1wt.eu>
> 
> Marvell uses a specific atag in its u-boot which includes among other
> information the MAC addresses for up to 4 network interfaces. This
> atag is parsed, and if found the MAC addresses are extracted there and
> assigned to node aliases eth0..3 with the name "mac-address".
> 
> This allows, with the non-DT capable bootloaders currently used on
> most Marvell platforms, to get a proper MAC address for the different
> interfaces.
> 
> Signed-off-by: Willy Tarreau <w@1wt.eu>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

You know, my reaction to this is to nack it because:

(a) Marvell didn't talk to me about adding a new tag.
(b) There is an established precident that we do not pass MAC addresses
    to the kernel in this way (such attempts have been rejected in the
    past.)
(c) It goes completely against the design spirit of ATAGs by combining
    many different types and instances of information into one tag.
(d) It picks a tag ID without understanding how tag IDs are allocated.
    (the idea is 0x41NNNNVV where NNNN = machine ID for machine specific
    tags.)

Everyone who has gone around extending ATAG stuff has made exactly the
same mistakes time and time again - mostly stemming from the fact that
no one wants to talk to me up front.

So, this is another NACK.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Mon, Jun 03, 2013 at 07:09:54PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Mon, 3 Jun 2013 12:57:53 -0400, Jason Cooper wrote:
> 
> > Won't this override a user editing the 'local-mac-address' property in
> > the dts?  Since we're dealing with appended dtbs, there's a higher
> > probability of the user doing such a thing.
> > 
> > I'll admit it's a corner case, but I'd like to make sure we've thought
> > through the use cases and aren't making things more complicated than
> > necessary.
> 
> I don't have a strong opinion about this one, so if it's seen as being
> necessary to preserve a potentially existing 'local-mac-address', then
> I'll add the necessary checks to only update the property if it doesn't
> already exist.

hmmm, of_get_mac_address() give preferences to 'mac_address' and has the
following comment:

/**
 * Search the device tree for the best MAC address to use.
 * 'mac-address' is checked first, because that is supposed to contain
 * to "most recent" MAC address. If that isn't set, then
 * 'local-mac-address' is checked next, because that is the default
 * address.  If that isn't set, then the obsolete 'address' is checked,
 * just in case we're using an old device tree.
 *
 * Note that the 'address' property is supposed to contain a virtual
 * address of the register set, but some DTS files have redefined that
 * property to be the MAC address.
 *
 * All-zero MAC addresses are rejected, because those could be
 * properties that exist in the device tree, but were not set by U-Boot.
 * For example, the DTS could define 'mac-address' and
 * 'local-mac-address', with zero MAC addresses.  Some older U-Boots
 * only initialized 'local-mac-address'.  In this case, the real MAC is
 * in 'local-mac-address', and 'mac-address' exists but is all zeros.
*/

So the real (minor) problem is that we require 'local-mac-address' in the
binding.  And say nothing about mac-address.

u-boot, v2013.04 sets both mac-address and local-mac-address
from the environment in fdt_fixup_ethernet() and doesn't check to see if
it's already populated with a valid mac :(

barebox doesn't appear to set the mac-address, and relies on the dtb.

Since we really only have to worry about legacy u-boot with ATAGs, we
should mimic the behavior of u-boot to date.  The mac address found in
the u-boot environment is the canonical source for the mac address.
This would have the added bonus that flash-kernel (debian kernel
installer) wouldn't need to dig up the mac address and add it to the
appended dtb.

So, I'm happy with it the way it is.  If we want to dot our i's and
cross our t's, we could also set local-mac-address from the atag,
however, this isn't necessary because of the precedence in
of_get_mac_address().

thx,

Jason.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Willy Tarreau]

Hi Russell,

On Mon, Jun 03, 2013 at 06:10:18PM +0100, Russell King - ARM Linux wrote:
> You know, my reaction to this is to nack it because:
> 
> (a) Marvell didn't talk to me about adding a new tag.
> (b) There is an established precident that we do not pass MAC addresses
>     to the kernel in this way (such attempts have been rejected in the
>     past.)
> (c) It goes completely against the design spirit of ATAGs by combining
>     many different types and instances of information into one tag.
> (d) It picks a tag ID without understanding how tag IDs are allocated.
>     (the idea is 0x41NNNNVV where NNNN = machine ID for machine specific
>     tags.)
> 
> Everyone who has gone around extending ATAG stuff has made exactly the
> same mistakes time and time again - mostly stemming from the fact that
> no one wants to talk to me up front.
> 
> So, this is another NACK.

I understand your points, but then what could we do to get our devices
to have properly working ethernet interfaces ? These devices have already
been sold, and from what I've seen they've been using this ID since at
least the Kirkwood devices.

I found no other way to get the MAC address once the system is booted.
I would have no problem having some board-spec code locate the atags
and set the MAC, but it looks like the information is lost very early
and is not available anymore soon after the boot (or at least I couldn't
find it anywhere else).

It's really not with happiness that I had to add this part to the ATAGs,
but because I didn't find another solution :-(

Thanks,
Willy

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Mon, Jun 03, 2013 at 07:56:29PM +0200, Willy Tarreau wrote:
> Hi Russell,
> 
> On Mon, Jun 03, 2013 at 06:10:18PM +0100, Russell King - ARM Linux wrote:
> > You know, my reaction to this is to nack it because:
> > 
> > (a) Marvell didn't talk to me about adding a new tag.
> > (b) There is an established precident that we do not pass MAC addresses
> >     to the kernel in this way (such attempts have been rejected in the
> >     past.)
> > (c) It goes completely against the design spirit of ATAGs by combining
> >     many different types and instances of information into one tag.
> > (d) It picks a tag ID without understanding how tag IDs are allocated.
> >     (the idea is 0x41NNNNVV where NNNN = machine ID for machine specific
> >     tags.)
> > 
> > Everyone who has gone around extending ATAG stuff has made exactly the
> > same mistakes time and time again - mostly stemming from the fact that
> > no one wants to talk to me up front.
> > 
> > So, this is another NACK.
> 
> I understand your points, but then what could we do to get our devices
> to have properly working ethernet interfaces ? These devices have already
> been sold, and from what I've seen they've been using this ID since at
> least the Kirkwood devices.
> 
> I found no other way to get the MAC address once the system is booted.
> I would have no problem having some board-spec code locate the atags
> and set the MAC, but it looks like the information is lost very early
> and is not available anymore soon after the boot (or at least I couldn't
> find it anywhere else).
> 
> It's really not with happiness that I had to add this part to the ATAGs,
> but because I didn't find another solution :-(

Please take a look at Sebastian's approach, it's currently a wip:

  ARM: kirkwood: proper retain MAC address workaround on DT ethernet

The discussion following that patch should give you some good ideas.

hth,

Jason.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> On Mon, Jun 03, 2013 at 07:56:29PM +0200, Willy Tarreau wrote:
> > Hi Russell,
> > 
> > On Mon, Jun 03, 2013 at 06:10:18PM +0100, Russell King - ARM Linux wrote:
> > > You know, my reaction to this is to nack it because:
> > > 
> > > (a) Marvell didn't talk to me about adding a new tag.
> > > (b) There is an established precident that we do not pass MAC addresses
> > >     to the kernel in this way (such attempts have been rejected in the
> > >     past.)
> > > (c) It goes completely against the design spirit of ATAGs by combining
> > >     many different types and instances of information into one tag.
> > > (d) It picks a tag ID without understanding how tag IDs are allocated.
> > >     (the idea is 0x41NNNNVV where NNNN = machine ID for machine specific
> > >     tags.)
> > > 
> > > Everyone who has gone around extending ATAG stuff has made exactly the
> > > same mistakes time and time again - mostly stemming from the fact that
> > > no one wants to talk to me up front.
> > > 
> > > So, this is another NACK.
> > 
> > I understand your points, but then what could we do to get our devices
> > to have properly working ethernet interfaces ? These devices have already
> > been sold, and from what I've seen they've been using this ID since at
> > least the Kirkwood devices.
> > 
> > I found no other way to get the MAC address once the system is booted.
> > I would have no problem having some board-spec code locate the atags
> > and set the MAC, but it looks like the information is lost very early
> > and is not available anymore soon after the boot (or at least I couldn't
> > find it anywhere else).
> > 
> > It's really not with happiness that I had to add this part to the ATAGs,
> > but because I didn't find another solution :-(
> 
> Please take a look at Sebastian's approach, it's currently a wip:
> 
>   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> 
> The discussion following that patch should give you some good ideas.

Gah, here's the direct link to the patch:

https://lkml.org/lkml/2013/5/22/541

hth,

Jason.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Willy Tarreau]

Hi Jason,

On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> > I understand your points, but then what could we do to get our devices
> > to have properly working ethernet interfaces ? These devices have already
> > been sold, and from what I've seen they've been using this ID since at
> > least the Kirkwood devices.
> > 
> > I found no other way to get the MAC address once the system is booted.
> > I would have no problem having some board-spec code locate the atags
> > and set the MAC, but it looks like the information is lost very early
> > and is not available anymore soon after the boot (or at least I couldn't
> > find it anywhere else).
> > 
> > It's really not with happiness that I had to add this part to the ATAGs,
> > but because I didn't find another solution :-(
> 
> Please take a look at Sebastian's approach, it's currently a wip:
> 
>   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> 
> The discussion following that patch should give you some good ideas.

I remember this discussion, but it is different and does not apply here.
Sebastian fixed an issue with a properly configured NIC which used to
lose its MAC, so the solution consisted in saving it early in the DT.

In the mv_neta case, the NICs are not configured yet and we need to
find the MAC somewhere. I only found it in the ATAGs and nowhere else.
Well, in fact u-boot sets it on the MAC used to boot from the network
if such a boot is performed, but that's all. So in practice you boot
without the MAC address anywhere but in the ATAGs. I'd be happy to
find a way to parse non-standard atags in a board-specific file but
they're lost early it seems :-(

Thanks,
Willy

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Mon, Jun 03, 2013 at 08:30:57PM +0200, Willy Tarreau wrote:
> Hi Jason,
> 
> On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> > > I understand your points, but then what could we do to get our devices
> > > to have properly working ethernet interfaces ? These devices have already
> > > been sold, and from what I've seen they've been using this ID since at
> > > least the Kirkwood devices.
> > > 
> > > I found no other way to get the MAC address once the system is booted.
> > > I would have no problem having some board-spec code locate the atags
> > > and set the MAC, but it looks like the information is lost very early
> > > and is not available anymore soon after the boot (or at least I couldn't
> > > find it anywhere else).
> > > 
> > > It's really not with happiness that I had to add this part to the ATAGs,
> > > but because I didn't find another solution :-(
> > 
> > Please take a look at Sebastian's approach, it's currently a wip:
> > 
> >   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> > 
> > The discussion following that patch should give you some good ideas.
> 
> I remember this discussion, but it is different and does not apply here.
> Sebastian fixed an issue with a properly configured NIC which used to
> lose its MAC, so the solution consisted in saving it early in the DT.
> 
> In the mv_neta case, the NICs are not configured yet and we need to
> find the MAC somewhere. I only found it in the ATAGs and nowhere else.

ahhh, that's annoying.  :-(

> Well, in fact u-boot sets it on the MAC used to boot from the network
> if such a boot is performed, but that's all. So in practice you boot
> without the MAC address anywhere but in the ATAGs. I'd be happy to
> find a way to parse non-standard atags in a board-specific file but
> they're lost early it seems :-(

I assume they are in the u-boot environment.  As such, the only answer
may be to have user space utilities like flash-kernel parse the
environment and then customize the appended DT for the specific board.

Once the Marvell u-boot is upgraded to support fdt, they will hopefully
support editing the DT before it is passed.  All they have to do is
leave bootm alone :-/

thx,

Jason.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Nicolas Pitre]

On Mon, 3 Jun 2013, Willy Tarreau wrote:

> Hi Jason,
> 
> On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> > > I understand your points, but then what could we do to get our devices
> > > to have properly working ethernet interfaces ? These devices have already
> > > been sold, and from what I've seen they've been using this ID since at
> > > least the Kirkwood devices.
> > > 
> > > I found no other way to get the MAC address once the system is booted.
> > > I would have no problem having some board-spec code locate the atags
> > > and set the MAC, but it looks like the information is lost very early
> > > and is not available anymore soon after the boot (or at least I couldn't
> > > find it anywhere else).
> > > 
> > > It's really not with happiness that I had to add this part to the ATAGs,
> > > but because I didn't find another solution :-(
> > 
> > Please take a look at Sebastian's approach, it's currently a wip:
> > 
> >   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> > 
> > The discussion following that patch should give you some good ideas.
> 
> I remember this discussion, but it is different and does not apply here.
> Sebastian fixed an issue with a properly configured NIC which used to
> lose its MAC, so the solution consisted in saving it early in the DT.
> 
> In the mv_neta case, the NICs are not configured yet and we need to
> find the MAC somewhere. I only found it in the ATAGs and nowhere else.
> Well, in fact u-boot sets it on the MAC used to boot from the network
> if such a boot is performed, but that's all. So in practice you boot
> without the MAC address anywhere but in the ATAGs. I'd be happy to
> find a way to parse non-standard atags in a board-specific file but
> they're lost early it seems :-(

Those MAC addresses must exist somewhere before they're wrapped into 
some special ATAG.  They're certainly not hardcoded into the u-boot 
binary.


Nicolas

[PATCH] ARM: atags: add support for Marvell's u-boot

[Nicolas Pitre]

On Mon, 3 Jun 2013, Jason Cooper wrote:

> On Mon, Jun 03, 2013 at 08:30:57PM +0200, Willy Tarreau wrote:
> > Hi Jason,
> > 
> > On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> > > > I understand your points, but then what could we do to get our devices
> > > > to have properly working ethernet interfaces ? These devices have already
> > > > been sold, and from what I've seen they've been using this ID since at
> > > > least the Kirkwood devices.
> > > > 
> > > > I found no other way to get the MAC address once the system is booted.
> > > > I would have no problem having some board-spec code locate the atags
> > > > and set the MAC, but it looks like the information is lost very early
> > > > and is not available anymore soon after the boot (or at least I couldn't
> > > > find it anywhere else).
> > > > 
> > > > It's really not with happiness that I had to add this part to the ATAGs,
> > > > but because I didn't find another solution :-(
> > > 
> > > Please take a look at Sebastian's approach, it's currently a wip:
> > > 
> > >   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> > > 
> > > The discussion following that patch should give you some good ideas.
> > 
> > I remember this discussion, but it is different and does not apply here.
> > Sebastian fixed an issue with a properly configured NIC which used to
> > lose its MAC, so the solution consisted in saving it early in the DT.
> > 
> > In the mv_neta case, the NICs are not configured yet and we need to
> > find the MAC somewhere. I only found it in the ATAGs and nowhere else.
> 
> ahhh, that's annoying.  :-(
> 
> > Well, in fact u-boot sets it on the MAC used to boot from the network
> > if such a boot is performed, but that's all. So in practice you boot
> > without the MAC address anywhere but in the ATAGs. I'd be happy to
> > find a way to parse non-standard atags in a board-specific file but
> > they're lost early it seems :-(
> 
> I assume they are in the u-boot environment.  As such, the only answer
> may be to have user space utilities like flash-kernel parse the
> environment and then customize the appended DT for the specific board.

Or some board specific code could dig them out of the flash.  That at 
least would make this hack localized while keeping the common code 
clean.

Isn't upstream U-Boot supporting Marvell devices properly these days?  
If so maybe considering a U-Boot upgrade would be the best way forward?


Nicolas

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Mon, Jun 03, 2013 at 03:07:45PM -0400, Nicolas Pitre wrote:
> On Mon, 3 Jun 2013, Jason Cooper wrote:
> 
> > On Mon, Jun 03, 2013 at 08:30:57PM +0200, Willy Tarreau wrote:
> > > Hi Jason,
> > > 
> > > On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> > > > > I understand your points, but then what could we do to get our devices
> > > > > to have properly working ethernet interfaces ? These devices have already
> > > > > been sold, and from what I've seen they've been using this ID since at
> > > > > least the Kirkwood devices.
> > > > > 
> > > > > I found no other way to get the MAC address once the system is booted.
> > > > > I would have no problem having some board-spec code locate the atags
> > > > > and set the MAC, but it looks like the information is lost very early
> > > > > and is not available anymore soon after the boot (or at least I couldn't
> > > > > find it anywhere else).
> > > > > 
> > > > > It's really not with happiness that I had to add this part to the ATAGs,
> > > > > but because I didn't find another solution :-(
> > > > 
> > > > Please take a look at Sebastian's approach, it's currently a wip:
> > > > 
> > > >   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> > > > 
> > > > The discussion following that patch should give you some good ideas.
> > > 
> > > I remember this discussion, but it is different and does not apply here.
> > > Sebastian fixed an issue with a properly configured NIC which used to
> > > lose its MAC, so the solution consisted in saving it early in the DT.
> > > 
> > > In the mv_neta case, the NICs are not configured yet and we need to
> > > find the MAC somewhere. I only found it in the ATAGs and nowhere else.
> > 
> > ahhh, that's annoying.  :-(
> > 
> > > Well, in fact u-boot sets it on the MAC used to boot from the network
> > > if such a boot is performed, but that's all. So in practice you boot
> > > without the MAC address anywhere but in the ATAGs. I'd be happy to
> > > find a way to parse non-standard atags in a board-specific file but
> > > they're lost early it seems :-(
> > 
> > I assume they are in the u-boot environment.  As such, the only answer
> > may be to have user space utilities like flash-kernel parse the
> > environment and then customize the appended DT for the specific board.
> 
> Or some board specific code could dig them out of the flash.  That at 
> least would make this hack localized while keeping the common code 
> clean.

If it comes to that, I think I would rather tell users to put the mac
address on the commandline.

> Isn't upstream U-Boot supporting Marvell devices properly these days?  
> If so maybe considering a U-Boot upgrade would be the best way forward?

Yes, both u-boot and barebox are supporting Marvell devices, however, a
lot of the devices on the consumer market are not easy/failsafe to
upgrade the bootloader.  Especially for tinkerers and hobbyists.

We've been trying to avoid telling hobbyists, "In order to support
mainline kernels, you must upgrade the bootloader".

At any rate, a lot of the boards currently supported by the kernel are
not supported by u-boot or barebox.  u-boot seems to take a while to get
patches merged.  Barebox is very responsive, board patches are small,
but not many folks know about it yet...

thx,

Jason.

[PATCH] ARM: atags: add support for Marvell's u-boot

[Nicolas Pitre]

On Mon, 3 Jun 2013, Jason Cooper wrote:

> On Mon, Jun 03, 2013 at 03:07:45PM -0400, Nicolas Pitre wrote:
> > On Mon, 3 Jun 2013, Jason Cooper wrote:
> > 
> > > On Mon, Jun 03, 2013 at 08:30:57PM +0200, Willy Tarreau wrote:
> > > > Well, in fact u-boot sets it on the MAC used to boot from the network
> > > > if such a boot is performed, but that's all. So in practice you boot
> > > > without the MAC address anywhere but in the ATAGs. I'd be happy to
> > > > find a way to parse non-standard atags in a board-specific file but
> > > > they're lost early it seems :-(
> > > 
> > > I assume they are in the u-boot environment.  As such, the only answer
> > > may be to have user space utilities like flash-kernel parse the
> > > environment and then customize the appended DT for the specific board.
> > 
> > Or some board specific code could dig them out of the flash.  That at 
> > least would make this hack localized while keeping the common code 
> > clean.
> 
> If it comes to that, I think I would rather tell users to put the mac
> address on the commandline.

That is certainly a simple and viable option.  You can manage your 
U-Boot environment to construct the final command line from multiple 
variables so that some are kept static and some others are user 
editable.

But for the record, there is a precedent for digging MAC addresses out 
of flash memory -- see

arch/arm/mach-orion5x/tsx09-common.c:qnap_tsx09_find_mac_addr()


Nicolas

[PATCH] ARM: atags: add support for Marvell's u-boot

[Willy Tarreau]

On Mon, Jun 03, 2013 at 03:01:42PM -0400, Nicolas Pitre wrote:
> On Mon, 3 Jun 2013, Willy Tarreau wrote:
> 
> > Hi Jason,
> > 
> > On Mon, Jun 03, 2013 at 02:14:37PM -0400, Jason Cooper wrote:
> > > > I understand your points, but then what could we do to get our devices
> > > > to have properly working ethernet interfaces ? These devices have already
> > > > been sold, and from what I've seen they've been using this ID since at
> > > > least the Kirkwood devices.
> > > > 
> > > > I found no other way to get the MAC address once the system is booted.
> > > > I would have no problem having some board-spec code locate the atags
> > > > and set the MAC, but it looks like the information is lost very early
> > > > and is not available anymore soon after the boot (or at least I couldn't
> > > > find it anywhere else).
> > > > 
> > > > It's really not with happiness that I had to add this part to the ATAGs,
> > > > but because I didn't find another solution :-(
> > > 
> > > Please take a look at Sebastian's approach, it's currently a wip:
> > > 
> > >   ARM: kirkwood: proper retain MAC address workaround on DT ethernet
> > > 
> > > The discussion following that patch should give you some good ideas.
> > 
> > I remember this discussion, but it is different and does not apply here.
> > Sebastian fixed an issue with a properly configured NIC which used to
> > lose its MAC, so the solution consisted in saving it early in the DT.
> > 
> > In the mv_neta case, the NICs are not configured yet and we need to
> > find the MAC somewhere. I only found it in the ATAGs and nowhere else.
> > Well, in fact u-boot sets it on the MAC used to boot from the network
> > if such a boot is performed, but that's all. So in practice you boot
> > without the MAC address anywhere but in the ATAGs. I'd be happy to
> > find a way to parse non-standard atags in a board-specific file but
> > they're lost early it seems :-(
> 
> Those MAC addresses must exist somewhere before they're wrapped into 
> some special ATAG.  They're certainly not hardcoded into the u-boot 
> binary.

Indeed, they're on the u-boot environment. But I don't think that it
really helps, because the variables in RAM are most likely lost and
anyway not well structured, and the ones on the flash require a
properly working flash driver before being extracted (not to mention
that they're the stored ones, not the active ones, but that's just
nitpicking).

Best regards,
Willy

[PATCH] ARM: atags: add support for Marvell's u-boot

[Willy Tarreau]

On Mon, Jun 03, 2013 at 03:17:31PM -0400, Jason Cooper wrote:
> > Or some board specific code could dig them out of the flash.  That at 
> > least would make this hack localized while keeping the common code 
> > clean.
> 
> If it comes to that, I think I would rather tell users to put the mac
> address on the commandline.

Yes, I think that's the least annoying solution for the end user. We
could even have the driver complain and explain to the user what to
do when it has to generate a random MAC address.

Willy

[PATCH] ARM: atags: add support for Marvell's u-boot

[Thomas Petazzoni]

Willy, Jason,

On Mon, 3 Jun 2013 20:30:57 +0200, Willy Tarreau wrote:

> I remember this discussion, but it is different and does not apply here.
> Sebastian fixed an issue with a properly configured NIC which used to
> lose its MAC, so the solution consisted in saving it early in the DT.
> 
> In the mv_neta case, the NICs are not configured yet and we need to
> find the MAC somewhere. I only found it in the ATAGs and nowhere else.
> Well, in fact u-boot sets it on the MAC used to boot from the network
> if such a boot is performed, but that's all. So in practice you boot
> without the MAC address anywhere but in the ATAGs. I'd be happy to
> find a way to parse non-standard atags in a board-specific file but
> they're lost early it seems :-(

I confirm this. Only the one NIC that was used for network booting gets
its MAC address assigned in the hardware registers, the other three NIC
have their MAC address left to zero by the bootloader. And of course,
if you don't do network booting, the four NICs will have their MAC left
to zero.

So Sebastian's approach does not work for mvneta.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] ARM: atags: add support for Marvell's u-boot

[Thomas Petazzoni]

Dear Nicolas Pitre,

On Mon, 03 Jun 2013 15:07:45 -0400 (EDT), Nicolas Pitre wrote:

> Isn't upstream U-Boot supporting Marvell devices properly these days?  

No, upstream U-Boot has no support for Armada 370 and Armada XP.
Sebastian and I have started adding support for Armada 370 and Armada
XP in Barebox, but it's gonna take a while before it has enough
features to be practically useful.

And as Jason said, even though Armada 370 and Armada XP have a working
UART-based recovery method, users are also scared of doing bootloader
updates, and on some consumer products, the UART is not necessarily
easily available to the average users (may need some soldering skills,
a level-shifter to use the TTL UART of the board, etc.).

Practically, even on platforms such as Kirkwood that do have upstream
U-Boot support, I believe the vast majority of people are still running
the original U-Boot, even though they decided to use an upstream kernel
by installing for example Debian on their NAS. So the use case of a
mainline kernel on a Marvell-specific U-Boot should be supported is
pretty common.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] ARM: atags: add support for Marvell's u-boot

[Thomas Petazzoni]

Dear Jason Cooper,

On Mon, 3 Jun 2013 15:17:31 -0400, Jason Cooper wrote:

> > Isn't upstream U-Boot supporting Marvell devices properly these days?  
> > If so maybe considering a U-Boot upgrade would be the best way forward?
> 
> Yes, both u-boot and barebox are supporting Marvell devices,

Sorry Jason, but you're overstating the status of this support:

 * Barebox has only *minimal* support for Marvell SoCs, i.e only the
   serial console and timers are supported. There is no support for
   network, NAND, SPI or anything that even allows to load a kernel and
   boot it.

 * U-Boot has zero support for Armada 370 and Armada XP, and Sebastian
   has tried for several months to push support for Marvell Dove,
   without success (lack of review/interest from the U-Boot
   maintainers, if I remember correctly).

So as of today, the Marvell-specific U-Boot is the /only/ working
solution for Armada 370 and Armada XP based platforms, and it will
remain this way at least for several months, until we've pushed enough
work into Barebox. And even at that time, as you say, users may not
necessarily be willing to upgrade their bootloader.

Best regards,

Thomas
-- 
Thomas Petazzoni, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

[PATCH] ARM: atags: add support for Marvell's u-boot

[Sebastian Hesselbarth]

On 06/04/13 10:10, Thomas Petazzoni wrote:
> On Mon, 03 Jun 2013 15:07:45 -0400 (EDT), Nicolas Pitre wrote:
>> Isn't upstream U-Boot supporting Marvell devices properly these days?
 >
> And as Jason said, even though Armada 370 and Armada XP have a working
> UART-based recovery method, users are also scared of doing bootloader
> updates, and on some consumer products, the UART is not necessarily
> easily available to the average users (may need some soldering skills,
> a level-shifter to use the TTL UART of the board, etc.).

Also remember that not all Kirkwoods support UART boot mode. If your
BootROM is too old, you will have to unbrick through JTAG. I have such
an old BootROM on my Dockstar.

Sebastian

[PATCH] ARM: atags: add support for Marvell's u-boot

[Sebastian Hesselbarth]

On 06/04/13 10:13, Thomas Petazzoni wrote:
> On Mon, 3 Jun 2013 15:17:31 -0400, Jason Cooper wrote:
>>> Isn't upstream U-Boot supporting Marvell devices properly these days?
>>> If so maybe considering a U-Boot upgrade would be the best way forward?
>>
>> Yes, both u-boot and barebox are supporting Marvell devices,
>
> Sorry Jason, but you're overstating the status of this support:
>
>   * Barebox has only *minimal* support for Marvell SoCs, i.e only the
>     serial console and timers are supported. There is no support for
>     network, NAND, SPI or anything that even allows to load a kernel and
>     boot it.

That remembers me that I wanted to make it boot from DT so we don't
start adding drivers and add DT later on.

>   * U-Boot has zero support for Armada 370 and Armada XP, and Sebastian
>     has tried for several months to push support for Marvell Dove,
>     without success (lack of review/interest from the U-Boot
>     maintainers, if I remember correctly).

True. It took about 2 month for a useful review and then the discussion
was only about naming it orion-something or mv-something. Recently,
someone picked up that patches and reposted them.. but don't expect them
to get in quickly.

> So as of today, the Marvell-specific U-Boot is the /only/ working
> solution for Armada 370 and Armada XP based platforms, and it will
> remain this way at least for several months, until we've pushed enough
> work into Barebox. And even at that time, as you say, users may not
> necessarily be willing to upgrade their bootloader.

Well, there is still the possibility to have board-specific dts files.
If you are stuck for whatever reason with an old boot loader, append a
dtb with MAC addresses set for your board. IMHO, adding workarounds for
every single boot loader just because we don't want anybody to either
update or manually patch the dts is ridiculous.

Sebastian

[PATCH] ARM: atags: add support for Marvell's u-boot

[Jason Cooper]

On Tue, Jun 04, 2013 at 10:48:51AM +0200, Sebastian Hesselbarth wrote:
> On 06/04/13 10:13, Thomas Petazzoni wrote:
> >On Mon, 3 Jun 2013 15:17:31 -0400, Jason Cooper wrote:
> >>>Isn't upstream U-Boot supporting Marvell devices properly these days?
> >>>If so maybe considering a U-Boot upgrade would be the best way forward?
> >>
> >>Yes, both u-boot and barebox are supporting Marvell devices,
> >
> >Sorry Jason, but you're overstating the status of this support:

True.

> >
> >  * Barebox has only *minimal* support for Marvell SoCs, i.e only the
> >    serial console and timers are supported. There is no support for
> >    network, NAND, SPI or anything that even allows to load a kernel and
> >    boot it.
> 
> That remembers me that I wanted to make it boot from DT so we don't
> start adding drivers and add DT later on.
> 
> >  * U-Boot has zero support for Armada 370 and Armada XP, and Sebastian
> >    has tried for several months to push support for Marvell Dove,
> >    without success (lack of review/interest from the U-Boot
> >    maintainers, if I remember correctly).
> 
> True. It took about 2 month for a useful review and then the discussion
> was only about naming it orion-something or mv-something. Recently,
> someone picked up that patches and reposted them.. but don't expect them
> to get in quickly.

Sascha Silbe seems to have picked up the ball, and I could've sworn I
saw Tom Rini picked them up.  It shocked me because I thought he was
bypassing Prafulla, but I can't find the email, now.  My mind must be
playing tricks on me.

> >So as of today, the Marvell-specific U-Boot is the /only/ working
> >solution for Armada 370 and Armada XP based platforms, and it will
> >remain this way at least for several months, until we've pushed enough
> >work into Barebox. And even at that time, as you say, users may not
> >necessarily be willing to upgrade their bootloader.
> 
> Well, there is still the possibility to have board-specific dts files.
> If you are stuck for whatever reason with an old boot loader, append a
> dtb with MAC addresses set for your board. IMHO, adding workarounds for
> every single boot loader just because we don't want anybody to either
> update or manually patch the dts is ridiculous.

Agreed.

thx,

Jason.