[PATCH 0/5] Mechanism for platform-specific parsing of ATAGs

[PATCH 0/5] Mechanism for platform-specific parsing of ATAGs

[Thomas Petazzoni]

Russell, Nicolas,

Following your comments on the patch that was adding the parsing of a
Marvell-specific ATAG in the generic atags_to_fdt.c, here is a new
proposal that allows this parsing to be entirely contained into code
specific to the Marvell platform.

The idea, implement in 'PATCH 2/5' is that the atags_to_fdt code
copies the ATAGs contents into the /chosen/atags DT property. This
adds 6 lines of code in atags_to_fdt.c that are completely generic,
and avoid cluttering this part of the code with platform-specific
ATAGs parsing.

Once this is in place, PATCH 5/5 modifies the Armada 370/XP specific
platform code to use this /chosen/atags DT property to find the
Marvell-specific ATAG, and assign the MAC addresses properly. In order
to assign the MAC addresses, an OF helper function
of_set_mac_address() is introduced in PATCH 3/5, and the mach-mxs code
is refactored to also use it (in PATCH 4/5).

What do you think about this approach? I believe it is much more
contained in the platform-specific code, and hopefully should address
your concerns.

Thanks!

Thomas

PS: Jason, I noticed you already merged 'ARM: mvebu: set aliases for
ethernet controllers', but I kept it here to expose the full solution.

Thomas Petazzoni (4):
  arm: preserve ATAGS in /chosen/atags in the Device Tree
  of: net: introduce a of_set_mac_address() helper function
  arm: mxs: use the newly introduced of_set_mac_address() helper
  arm: mvebu: parse ATAGS to find the network interfaces MAC addresses

Willy Tarreau (1):
  ARM: mvebu: set aliases for ethernet controllers

 arch/arm/boot/compressed/atags_to_fdt.c  |  6 ++++
 arch/arm/boot/dts/armada-370-xp.dtsi     |  9 ++++--
 arch/arm/boot/dts/armada-xp-mv78460.dtsi |  3 +-
 arch/arm/boot/dts/armada-xp.dtsi         |  6 +++-
 arch/arm/mach-mvebu/armada-370-xp.c      | 52 ++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/armada-370-xp.h      | 13 ++++++++
 arch/arm/mach-mxs/mach-mxs.c             | 22 ++------------
 drivers/of/of_net.c                      | 36 ++++++++++++++++++++++
 include/linux/of_net.h                   |  6 ++++
 9 files changed, 130 insertions(+), 23 deletions(-)

-- 
1.8.1.2

[PATCH 1/5] ARM: mvebu: set aliases for ethernet controllers

[Thomas Petazzoni]

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

These aliases are used when feeding the DT from ATAGS to set the
devices MAC addresses.

Signed-off-by: Willy Tarreau <w@1wt.eu>
Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/dts/armada-370-xp.dtsi     | 9 +++++++--
 arch/arm/boot/dts/armada-xp-mv78460.dtsi | 3 ++-
 arch/arm/boot/dts/armada-xp.dtsi         | 6 +++++-
 3 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/arch/arm/boot/dts/armada-370-xp.dtsi b/arch/arm/boot/dts/armada-370-xp.dtsi
index 550eb77..8f69cf4 100644
--- a/arch/arm/boot/dts/armada-370-xp.dtsi
+++ b/arch/arm/boot/dts/armada-370-xp.dtsi
@@ -22,6 +22,11 @@
 	model = "Marvell Armada 370 and XP SoC";
 	compatible = "marvell,armada-370-xp";
 
+	aliases {
+		eth0 = &eth0;
+		eth1 = &eth1;
+	};
+
 	cpus {
 		cpu at 0 {
 			compatible = "marvell,sheeva-v7";
@@ -94,7 +99,7 @@
 				reg = <0x72004 0x4>;
 			};
 
-			ethernet at 70000 {
+			eth0: ethernet at 70000 {
 				compatible = "marvell,armada-370-neta";
 				reg = <0x70000 0x2500>;
 				interrupts = <8>;
@@ -102,7 +107,7 @@
 				status = "disabled";
 			};
 
-			ethernet at 74000 {
+			eth1: ethernet at 74000 {
 				compatible = "marvell,armada-370-neta";
 				reg = <0x74000 0x2500>;
 				interrupts = <10>;
diff --git a/arch/arm/boot/dts/armada-xp-mv78460.dtsi b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
index 6ab56bd..386f0ce 100644
--- a/arch/arm/boot/dts/armada-xp-mv78460.dtsi
+++ b/arch/arm/boot/dts/armada-xp-mv78460.dtsi
@@ -23,6 +23,7 @@
 		gpio0 = &gpio0;
 		gpio1 = &gpio1;
 		gpio2 = &gpio2;
+		eth3 = &eth3;
 	};
 
 
@@ -105,7 +106,7 @@
 				interrupts = <91>;
 			};
 
-			ethernet at 34000 {
+			eth3: ethernet at 34000 {
 				compatible = "marvell,armada-370-neta";
 				reg = <0x34000 0x2500>;
 				interrupts = <14>;
diff --git a/arch/arm/boot/dts/armada-xp.dtsi b/arch/arm/boot/dts/armada-xp.dtsi
index 5b902f9..e481d54 100644
--- a/arch/arm/boot/dts/armada-xp.dtsi
+++ b/arch/arm/boot/dts/armada-xp.dtsi
@@ -22,6 +22,10 @@
 	model = "Marvell Armada XP family SoC";
 	compatible = "marvell,armadaxp", "marvell,armada-370-xp";
 
+	aliases {
+		eth2 = &eth2;
+	};
+
 	soc {
 		internal-regs {
 			L2: l2-cache {
@@ -86,7 +90,7 @@
 				reg = <0x18200 0x500>;
 			};
 
-			ethernet at 30000 {
+			eth2: ethernet at 30000 {
 				compatible = "marvell,armada-370-neta";
 				reg = <0x30000 0x2500>;
 				interrupts = <12>;
-- 
1.8.1.2

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Thomas Petazzoni]

Some platforms have custom ATAGS that are too specific to be parsed by
generic code in atags_to_fdt.c, but that would nonetheless be
useful. For example, Marvell bootloaders pass a custom ATAG that
contain the MAC address for the various network interfaces of the
board.

This commit makes a small addition to the atags_to_fdt logic that
consists in storing the ATAGS contents in the /chosen/atags node of
the Device Tree. Some platform-specific can then later make use of
this data to parse some custom ATAGs.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/boot/compressed/atags_to_fdt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
index aabc02a..8b2ae61 100644
--- a/arch/arm/boot/compressed/atags_to_fdt.c
+++ b/arch/arm/boot/compressed/atags_to_fdt.c
@@ -98,6 +98,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 	uint32_t mem_reg_property[2 * NR_BANKS];
 	int memcount = 0;
 	int ret;
+	int atagssize = 0;
 
 	/* make sure we've got an aligned pointer */
 	if ((u32)atag_list & 0x3)
@@ -119,6 +120,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 		return ret;
 
 	for_each_tag(atag, atag_list) {
+		atagssize += atag->hdr.size * 4;
 		if (atag->hdr.tag == ATAG_CMDLINE) {
 			/* Append the ATAGS command line to the device tree
 			 * command line.
@@ -153,5 +155,9 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
 	if (memcount)
 		setprop(fdt, "/memory", "reg", mem_reg_property, 4*memcount);
 
+	if (atagssize)
+		setprop(fdt, "/chosen", "atags", atag_list,
+			atagssize);
+
 	return fdt_pack(fdt);
 }
-- 
1.8.1.2

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Jason Cooper]

On Wed, Jun 05, 2013 at 08:40:06AM +0200, Thomas Petazzoni wrote:
> Some platforms have custom ATAGS that are too specific to be parsed by
> generic code in atags_to_fdt.c, but that would nonetheless be
> useful. For example, Marvell bootloaders pass a custom ATAG that
> contain the MAC address for the various network interfaces of the
> board.
> 
> This commit makes a small addition to the atags_to_fdt logic that
> consists in storing the ATAGS contents in the /chosen/atags node of
> the Device Tree. Some platform-specific can then later make use of

                                         ^code

thx,

Jason.

> this data to parse some custom ATAGs.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/compressed/atags_to_fdt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
> index aabc02a..8b2ae61 100644
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -98,6 +98,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  	uint32_t mem_reg_property[2 * NR_BANKS];
>  	int memcount = 0;
>  	int ret;
> +	int atagssize = 0;
>  
>  	/* make sure we've got an aligned pointer */
>  	if ((u32)atag_list & 0x3)
> @@ -119,6 +120,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  		return ret;
>  
>  	for_each_tag(atag, atag_list) {
> +		atagssize += atag->hdr.size * 4;
>  		if (atag->hdr.tag == ATAG_CMDLINE) {
>  			/* Append the ATAGS command line to the device tree
>  			 * command line.
> @@ -153,5 +155,9 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  	if (memcount)
>  		setprop(fdt, "/memory", "reg", mem_reg_property, 4*memcount);
>  
> +	if (atagssize)
> +		setprop(fdt, "/chosen", "atags", atag_list,
> +			atagssize);
> +
>  	return fdt_pack(fdt);
>  }
> -- 
> 1.8.1.2
> 

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Thomas Petazzoni]

Russell, Nicolas,

I'd be interested to hear your opinion about the below proposal, that
allows platform-specific code to do its own parsing of ATAGS
information, without cluttering the generic code.

Thanks!

Thomas

On Wed,  5 Jun 2013 08:40:06 +0200, Thomas Petazzoni wrote:
> Some platforms have custom ATAGS that are too specific to be parsed by
> generic code in atags_to_fdt.c, but that would nonetheless be
> useful. For example, Marvell bootloaders pass a custom ATAG that
> contain the MAC address for the various network interfaces of the
> board.
> 
> This commit makes a small addition to the atags_to_fdt logic that
> consists in storing the ATAGS contents in the /chosen/atags node of
> the Device Tree. Some platform-specific can then later make use of
> this data to parse some custom ATAGs.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  arch/arm/boot/compressed/atags_to_fdt.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/arm/boot/compressed/atags_to_fdt.c b/arch/arm/boot/compressed/atags_to_fdt.c
> index aabc02a..8b2ae61 100644
> --- a/arch/arm/boot/compressed/atags_to_fdt.c
> +++ b/arch/arm/boot/compressed/atags_to_fdt.c
> @@ -98,6 +98,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  	uint32_t mem_reg_property[2 * NR_BANKS];
>  	int memcount = 0;
>  	int ret;
> +	int atagssize = 0;
>  
>  	/* make sure we've got an aligned pointer */
>  	if ((u32)atag_list & 0x3)
> @@ -119,6 +120,7 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  		return ret;
>  
>  	for_each_tag(atag, atag_list) {
> +		atagssize += atag->hdr.size * 4;
>  		if (atag->hdr.tag == ATAG_CMDLINE) {
>  			/* Append the ATAGS command line to the device tree
>  			 * command line.
> @@ -153,5 +155,9 @@ int atags_to_fdt(void *atag_list, void *fdt, int total_space)
>  	if (memcount)
>  		setprop(fdt, "/memory", "reg", mem_reg_property, 4*memcount);
>  
> +	if (atagssize)
> +		setprop(fdt, "/chosen", "atags", atag_list,
> +			atagssize);
> +
>  	return fdt_pack(fdt);
>  }



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

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Nicolas Pitre]

On Thu, 6 Jun 2013, Thomas Petazzoni wrote:

> Russell, Nicolas,
> 
> I'd be interested to hear your opinion about the below proposal, that
> allows platform-specific code to do its own parsing of ATAGS
> information, without cluttering the generic code.

Personally, I don't like it.

Not that the code itself is extremely ugly.  That is not the point.

This opens the door to laziness on the part of bootloader authors and 
system integrators which will simply (ab)use this mechanism as this 
allows them to sit on what they already have instead of making the extra 
mile to bring things into conformance.  And overtime this means 
maintenance nightmare for us.

I'm sorry but the only leverage we have to create some incentive for 
people to do the right thing is exactly to say no to such kind of 
accommodation.

If Marvell has done things improperly in the past, it is for them to pay 
the maintenance cost of their choices, not for mainline to carry them.

If I were you I'd try to bring the bootloader into conformance instead.  
That can be achieved in many ways:

First, you should try to add support for older platforms into mainline 
U-Boot.  That's always the preferable option.

The next best option is to get the Marvell U-Boot source code (U-Boot is 
GPL so the source must be available) and add minimal DT support to 
it.

Those two options imply a bootloader upgrade of course.  I know that 
people are scared at the prospect of updating their bootloader, however 
this can be made quite safe if the upgrade is implemented with care.

Alternately you could use a second stage bootloader (or third stage in 
some cases) that is executed from the existing U-Boot.  That second 
stage bootloader can be as simple as a little executable blob that only 
converts the legacy/proprietary ATAGs into a proper device tree and pass 
it on to the already loaded kernel in memory.  That second stage 
bootloader doesn't necessarily have to load files -- the original U-Boot 
is well capable of loading more than just a kernel + initrd into memory 
e.g. it can be scripted to load the kernel, the initrd, the desired DTB 
and this shim binary, and execute the later.  The shim and the DTB don't 
normally have to change, so a user could then enjoy upgrading the kernel 
at will and use standard booting interfaces without even being aware of 
the presence of that shim which only needs to be installed once.


Nicolas

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Thomas Petazzoni]

Dear Nicolas Pitre,

On Thu, 6 Jun 2013 13:27:38 -0400 (EDT), Nicolas Pitre wrote:

> > I'd be interested to hear your opinion about the below proposal, that
> > allows platform-specific code to do its own parsing of ATAGS
> > information, without cluttering the generic code.
> 
> Personally, I don't like it.
> 
> Not that the code itself is extremely ugly.  That is not the point.
> 
> This opens the door to laziness on the part of bootloader authors and 
> system integrators which will simply (ab)use this mechanism as this 
> allows them to sit on what they already have instead of making the extra 
> mile to bring things into conformance.  And overtime this means 
> maintenance nightmare for us.
> 
> I'm sorry but the only leverage we have to create some incentive for 
> people to do the right thing is exactly to say no to such kind of 
> accommodation.
> 
> If Marvell has done things improperly in the past, it is for them to pay 
> the maintenance cost of their choices, not for mainline to carry them.

Well, I don't think what you say here is really fair. Before the DT was
in place, unless I missed it, there was no standard way of letting the
bootloader pass MAC addresses to the kernel. And Marvell's development
on those Armada 370/XP platforms predates the introduction of the
Device Tree in the Linux kernel (the code we have originally been give
was a 2.6.3x), so it's really not their fault to not have a DT-capable
bootloader at this point.

The ARM_APPENDED_DTB and ARM_ATAG_DTB_COMPAT thing were added precisely
for this situation: the hardware platform was designed at a time where
the DT was not yet used/widespread on ARM, so the bootloaders for those
hardware platforms is not DT-capable. What I'm proposing here is a
minimally intrusive extension of this idea, in the exact same scenario.

Moving from the ATAG-based boot method to the DT-based method was a
mainline decision, and pragmatically, we should give SoC vendors enough
time to switch their entire boot infrastructure to this new boot
protocol. This is why ARM_APPENDED_DTB was created in the first place.
So please don't put the entire fault on Marvell on this one :)

Marvell will release a DT-capable U-Boot in some time, that will
properly adjust the MAC addresses in the DTB before handing it over to
the kernel. We are currently working with Marvell to define which
actions the bootloader should do on the DTB before handing it over to
the kernel. I believe this is a sufficient proof that there is no
laziness from Marvell's side and that things are progressing towards a
full DT based boot protocol.

But in the mean time, just like we have this ARM_APPENDED_DTB and
ARM_ATAG_DTB_COMPAT workarounds, I thought we could do the same to
allow platform-specific code to parse some platform-specific ATAGs.
That would be so much better for the current users of Armada 370 and
Armada XP platforms.

> If I were you I'd try to bring the bootloader into conformance instead.  
> That can be achieved in many ways:
> 
> First, you should try to add support for older platforms into mainline 
> U-Boot.  That's always the preferable option.
> 
> The next best option is to get the Marvell U-Boot source code (U-Boot is 
> GPL so the source must be available) and add minimal DT support to 
> it.

As I mentioned earlier, Marvell is going to release one relatively soon.

Thanks,

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

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Jason Cooper]

On Fri, Jun 07, 2013 at 11:21:01AM +0200, Thomas Petazzoni wrote:
> Dear Nicolas Pitre,
> 
> On Thu, 6 Jun 2013 13:27:38 -0400 (EDT), Nicolas Pitre wrote:
> 
> > > I'd be interested to hear your opinion about the below proposal, that
> > > allows platform-specific code to do its own parsing of ATAGS
> > > information, without cluttering the generic code.
> > 
> > Personally, I don't like it.
> > 
> > Not that the code itself is extremely ugly.  That is not the point.
> > 
> > This opens the door to laziness on the part of bootloader authors and 
> > system integrators which will simply (ab)use this mechanism as this 
> > allows them to sit on what they already have instead of making the extra 
> > mile to bring things into conformance.  And overtime this means 
> > maintenance nightmare for us.
> > 
> > I'm sorry but the only leverage we have to create some incentive for 
> > people to do the right thing is exactly to say no to such kind of 
> > accommodation.
> > 
> > If Marvell has done things improperly in the past, it is for them to pay 
> > the maintenance cost of their choices, not for mainline to carry them.
> 
> Well, I don't think what you say here is really fair. Before the DT was
> in place, unless I missed it, there was no standard way of letting the
> bootloader pass MAC addresses to the kernel. And Marvell's development
> on those Armada 370/XP platforms predates the introduction of the
> Device Tree in the Linux kernel (the code we have originally been give
> was a 2.6.3x), so it's really not their fault to not have a DT-capable
> bootloader at this point.

To be accurate, I think Nico is referring to the fact that Marvell
assigned their own ATAG without going through Russell.

However, just like mach-types, are we assigning any new atags?  Can we
consider them deprecated?  If so, that changes the game.  Then what
Thomas is proposing is a "legacy compatibility" patch, as opposed to a
hole vendors can use to do their own thing.

We could add Arnd's suggestion of a time bomb on the common code in
atags_to_fdt.c to prevent mis-use.

I'm not 100% convinced of this, and I actually tend to agree with Nico
here, but I'd also like to find a workable solution.

thx,

Jason.

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Thomas Petazzoni]

Dear Jason Cooper,

On Fri, 7 Jun 2013 10:32:49 -0400, Jason Cooper wrote:

> > Well, I don't think what you say here is really fair. Before the DT was
> > in place, unless I missed it, there was no standard way of letting the
> > bootloader pass MAC addresses to the kernel. And Marvell's development
> > on those Armada 370/XP platforms predates the introduction of the
> > Device Tree in the Linux kernel (the code we have originally been give
> > was a 2.6.3x), so it's really not their fault to not have a DT-capable
> > bootloader at this point.
> 
> To be accurate, I think Nico is referring to the fact that Marvell
> assigned their own ATAG without going through Russell.

Right, that's true.

> However, just like mach-types, are we assigning any new atags?  Can we
> consider them deprecated?  If so, that changes the game.  Then what
> Thomas is proposing is a "legacy compatibility" patch, as opposed to a
> hole vendors can use to do their own thing.
> 
> We could add Arnd's suggestion of a time bomb on the common code in
> atags_to_fdt.c to prevent mis-use.
> 
> I'm not 100% convinced of this, and I actually tend to agree with Nico
> here, but I'd also like to find a workable solution.

I perfectly understand Nico and Russell concerns, for sure. But I'd
also like to find a workable solution:
 
 * Passing the MAC address on the kernel command line is not something
   that the network maintainer likes. See
   http://lists.openwall.net/netdev/2011/11/17/82 and Dave Miller's
   answer http://lists.openwall.net/netdev/2011/11/17/83.

 * Parsing the U-Boot environment is really not easy. How does the
   kernel know where this environment is located? What if another
   bootloader than U-Boot is used? Reading the U-Boot environment from
   the kernel sounds clunky.

Best regards,

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

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Jason Cooper]

On Fri, Jun 07, 2013 at 07:16:51PM +0200, Thomas Petazzoni wrote:
> Dear Jason Cooper,
> 
> On Fri, 7 Jun 2013 10:32:49 -0400, Jason Cooper wrote:
> 
> > > Well, I don't think what you say here is really fair. Before the DT was
> > > in place, unless I missed it, there was no standard way of letting the
> > > bootloader pass MAC addresses to the kernel. And Marvell's development
> > > on those Armada 370/XP platforms predates the introduction of the
> > > Device Tree in the Linux kernel (the code we have originally been give
> > > was a 2.6.3x), so it's really not their fault to not have a DT-capable
> > > bootloader at this point.
> > 
> > To be accurate, I think Nico is referring to the fact that Marvell
> > assigned their own ATAG without going through Russell.
> 
> Right, that's true.
> 
> > However, just like mach-types, are we assigning any new atags?  Can we
> > consider them deprecated?  If so, that changes the game.  Then what
> > Thomas is proposing is a "legacy compatibility" patch, as opposed to a
> > hole vendors can use to do their own thing.
> > 
> > We could add Arnd's suggestion of a time bomb on the common code in
> > atags_to_fdt.c to prevent mis-use.
> > 
> > I'm not 100% convinced of this, and I actually tend to agree with Nico
> > here, but I'd also like to find a workable solution.
> 
> I perfectly understand Nico and Russell concerns, for sure. But I'd
> also like to find a workable solution:
>  
>  * Passing the MAC address on the kernel command line is not something
>    that the network maintainer likes. See
>    http://lists.openwall.net/netdev/2011/11/17/82 and Dave Miller's
>    answer http://lists.openwall.net/netdev/2011/11/17/83.
> 
>  * Parsing the U-Boot environment is really not easy. How does the
>    kernel know where this environment is located? What if another
>    bootloader than U-Boot is used? Reading the U-Boot environment from
>    the kernel sounds clunky.

I agree on both points.  Let's see what Nico and Russell think of using
Arnd's suggestion of the time bomb, but used in atags_to_fdt.c.

thx,

Jason.

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Nicolas Pitre]

On Fri, 7 Jun 2013, Thomas Petazzoni wrote:

> Dear Nicolas Pitre,
> 
> On Thu, 6 Jun 2013 13:27:38 -0400 (EDT), Nicolas Pitre wrote:
> 
> > > I'd be interested to hear your opinion about the below proposal, that
> > > allows platform-specific code to do its own parsing of ATAGS
> > > information, without cluttering the generic code.
> > 
> > Personally, I don't like it.
> > 
> > Not that the code itself is extremely ugly.  That is not the point.
> > 
> > This opens the door to laziness on the part of bootloader authors and 
> > system integrators which will simply (ab)use this mechanism as this 
> > allows them to sit on what they already have instead of making the extra 
> > mile to bring things into conformance.  And overtime this means 
> > maintenance nightmare for us.
> > 
> > I'm sorry but the only leverage we have to create some incentive for 
> > people to do the right thing is exactly to say no to such kind of 
> > accommodation.
> > 
> > If Marvell has done things improperly in the past, it is for them to pay 
> > the maintenance cost of their choices, not for mainline to carry them.
> 
> Well, I don't think what you say here is really fair. Before the DT was
> in place, unless I missed it, there was no standard way of letting the
> bootloader pass MAC addresses to the kernel. And Marvell's development
> on those Armada 370/XP platforms predates the introduction of the
> Device Tree in the Linux kernel (the code we have originally been give
> was a 2.6.3x), so it's really not their fault to not have a DT-capable
> bootloader at this point.

Well, fair enough.  In the end we don't care whose fault it is.  That's 
besides the point.

What we care about is for the upstream kernel maintenance to remain 
sane.  In that regard it is not acceptable to add new code into mainline 
in order to support some boot mechanisms that were never supported by 
the mainline kernel before, and which we know upfront are not desirable.

> The ARM_APPENDED_DTB and ARM_ATAG_DTB_COMPAT thing were added precisely
> for this situation: the hardware platform was designed at a time where
> the DT was not yet used/widespread on ARM, so the bootloaders for those
> hardware platforms is not DT-capable. 

That's different.  Those features were added, against my wish I must 
admit even if I ended up writing most of the code for them, in order not 
to break platforms that were _already_ supported into mainline while the 
kernel moved to DT.  For newly supported platforms in mainline I don't 
think we should also carry their odd ways of doing things.

> What I'm proposing here is a minimally intrusive extension of this 
> idea, in the exact same scenario.

And then someone else will come along asking for yet another minimally 
intrusive extension.  And because we would have said yes to you we'll 
have to accept that other one too.  I'm sure you can forsee the mess 
growing.

> Moving from the ATAG-based boot method to the DT-based method was a
> mainline decision, and pragmatically, we should give SoC vendors enough
> time to switch their entire boot infrastructure to this new boot
> protocol. This is why ARM_APPENDED_DTB was created in the first place.

Exact.  It was created to be pragmatic and give people a chance to 
transition from what infrastructure they had at that point.  It was not 
created to constantly be extended over time.  If anything, we should be 
thinking about phasing it out instead.

> So please don't put the entire fault on Marvell on this one :)

Please remember that I did work at Marvell for 2.5 years previously.  
So I know people over there and I know they're not evil individuals.

But Marvell's fault here is to not have taken the hint seriously enough.  
We've said more than 2 years ago that no new platforms which is not DT 
enabled will be accepted into mainline anymore.  I think that's plenty 
of time to adapt the boot infrastructure.

> Marvell will release a DT-capable U-Boot in some time, that will
> properly adjust the MAC addresses in the DTB before handing it over to
> the kernel. We are currently working with Marvell to define which
> actions the bootloader should do on the DTB before handing it over to
> the kernel. I believe this is a sufficient proof that there is no
> laziness from Marvell's side and that things are progressing towards a
> full DT based boot protocol.

Great!  Better late than never.

> But in the mean time, just like we have this ARM_APPENDED_DTB and
> ARM_ATAG_DTB_COMPAT workarounds, I thought we could do the same to
> allow platform-specific code to parse some platform-specific ATAGs.
> That would be so much better for the current users of Armada 370 and
> Armada XP platforms.

And then you asked me what I thought about it, so I told you.  

I even provided examples of ways to work around the issue _outside_ the 
kernel even without replacing currently installed bootloaders.


On Fri, 7 Jun 2013, Jason Cooper wrote:

> We could add Arnd's suggestion of a time bomb on the common code in
> atags_to_fdt.c to prevent mis-use.

That don't work.  People simply don't take that hint any more seriously 
either.  It is just too easy to postpone the trigger instead.

> I'm not 100% convinced of this, and I actually tend to agree with Nico
> here, but I'd also like to find a workable solution.


On Fri, 7 Jun 2013, Thomas Petazzoni wrote:

> I perfectly understand Nico and Russell concerns, for sure. But I'd
> also like to find a workable solution:
>  
>  * Passing the MAC address on the kernel command line is not something
>    that the network maintainer likes. See
>    http://lists.openwall.net/netdev/2011/11/17/82 and Dave Miller's
>    answer http://lists.openwall.net/netdev/2011/11/17/83.

I don't necessarily agree with Davem here but this is his prerogative.

>  * Parsing the U-Boot environment is really not easy. How does the
>    kernel know where this environment is located? What if another
>    bootloader than U-Boot is used? Reading the U-Boot environment from
>    the kernel sounds clunky.

It certainly is, no doubt about that.

So let me repeat my other suggestion again.

What you need is a third stage bootloader.  And "bootloader" is probably 
too strong a word for what this piece of code needs to be.  So let's 
call it the impedance matcher.

The impedance matcher only has to:

- Be loaded into memory by the bootloader in addition to the kernel 
  image and ramdisk if you have one.

- Be executed instead of the kernel by the bootloader when booting.  It 
  may even pretend to be a kernel image itself, albeit a very small one, 
  to fool the bootloader.

- Look at the DT image the bootloader might have loaded into memory 
  along with the other images even if the bootloader itself didn't know 
  how to deal with it, and...

- Convert the bootloader provided ATAGs into DT nodes, including 
  whatever proprietary ATAGs you might have created and which you might 
  not want to tell the world about.

- Transfer execution to the kernel zImage loaded elsewhere previously by 
  the bootloader, passing along the address of the DTB that was just 
  updated.

Does it look like ARM_ATAG_DTB_COMPAT to you?  Because it certainly 
does.  You could even copy code from the kernel to implement this if you 
wish.  The libfdt code is certainly a must, and it is highly portable to 
a simplistic runtime environment.

But this does _not_ have to be tied to the kernel.  And with the loading 
of a DTB separately you even don't need ARM_APPENDED_DTB in your kernel 
anymore !

So you write this code once, compile it once, and install it everywhere, 
and forget about it just like the bootloader itself.  Users won't need 
to think about concatenating a DTB to their zImage.  They'll be able to 
use latests mainline kernels in their pure form.

Example U-Bot operations to be scripted:

- Load initrd at 0x04000000 (nothing different here)

- Load device tree blob at 0x03000000 (just a plain binary load)

- Load zImage/uImage at 0x02000000 (zImage can be executed at any 
  address)

- Load the impedance matcher at 0x00100000 (just like a regular kernel 
  image)

- Boot the kernel (actually the impedance matcher) with the bootm 
  command.

And because the impedance matcher does not have to be generic to many 
platforms, just like the compiled bootloader is not, you may simply 
hardcode some addresses in it that only the U-Boot script needs to know 
about, such as the address of the loaded DTB and final kernel image.

That's it!  you've turned your legacy boot environment into a fully DT 
capable one without even replacing the factory bootloader.  And that 
shouldn't require a lot of extra coding besides the code you were 
already willing to add to the kernel with your suggestion.


Nicolas

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Jason Cooper]

Nicolas,

On Sat, Jun 08, 2013 at 12:50:12AM -0400, Nicolas Pitre wrote:
> On Fri, 7 Jun 2013, Thomas Petazzoni wrote:
> > On Thu, 6 Jun 2013 13:27:38 -0400 (EDT), Nicolas Pitre wrote:
...
> On Fri, 7 Jun 2013, Jason Cooper wrote:
> 
> > We could add Arnd's suggestion of a time bomb on the common code in
> > atags_to_fdt.c to prevent mis-use.
> 
> That don't work.  People simply don't take that hint any more seriously 
> either.  It is just too easy to postpone the trigger instead.

Ok, just thought I'd mention it.

> > I'm not 100% convinced of this, and I actually tend to agree with Nico
> > here, but I'd also like to find a workable solution.

Thanks for confirming my uneasiness with the idea.

> On Fri, 7 Jun 2013, Thomas Petazzoni wrote:
> 
> > I perfectly understand Nico and Russell concerns, for sure. But I'd
> > also like to find a workable solution:
...
> >  * Parsing the U-Boot environment is really not easy. How does the
> >    kernel know where this environment is located? What if another
> >    bootloader than U-Boot is used? Reading the U-Boot environment from
> >    the kernel sounds clunky.
> 
> It certainly is, no doubt about that.
> 
> So let me repeat my other suggestion again.

I somehow missed this the first time around, sorry about that.

> What you need is a third stage bootloader.  And "bootloader" is probably 
> too strong a word for what this piece of code needs to be.  So let's 
> call it the impedance matcher.

Large shim?  :-P

> The impedance matcher only has to:
> 
> - Be loaded into memory by the bootloader in addition to the kernel 
>   image and ramdisk if you have one.
> 
> - Be executed instead of the kernel by the bootloader when booting.  It 
>   may even pretend to be a kernel image itself, albeit a very small one, 
>   to fool the bootloader.
> 
> - Look at the DT image the bootloader might have loaded into memory 
>   along with the other images even if the bootloader itself didn't know 
>   how to deal with it, and...
> 
> - Convert the bootloader provided ATAGs into DT nodes, including 
>   whatever proprietary ATAGs you might have created and which you might 
>   not want to tell the world about.
> 
> - Transfer execution to the kernel zImage loaded elsewhere previously by 
>   the bootloader, passing along the address of the DTB that was just 
>   updated.
> 
> Does it look like ARM_ATAG_DTB_COMPAT to you?  Because it certainly 
> does.  You could even copy code from the kernel to implement this if you 
> wish.  The libfdt code is certainly a must, and it is highly portable to 
> a simplistic runtime environment.

barebox might be a good starting point, it's designed to be executed
from itself.

> But this does _not_ have to be tied to the kernel.  And with the loading 
> of a DTB separately you even don't need ARM_APPENDED_DTB in your kernel 
> anymore !

That would be great.

> So you write this code once, compile it once, and install it everywhere, 
> and forget about it just like the bootloader itself.  Users won't need 
> to think about concatenating a DTB to their zImage.  They'll be able to 
> use latests mainline kernels in their pure form.
> 
> Example U-Bot operations to be scripted:
> 
> - Load initrd at 0x04000000 (nothing different here)
> 
> - Load device tree blob at 0x03000000 (just a plain binary load)
> 
> - Load zImage/uImage at 0x02000000 (zImage can be executed at any 
>   address)
> 
> - Load the impedance matcher at 0x00100000 (just like a regular kernel 
>   image)
> 
> - Boot the kernel (actually the impedance matcher) with the bootm 
>   command.
> 
> And because the impedance matcher does not have to be generic to many 
> platforms, just like the compiled bootloader is not, you may simply 
> hardcode some addresses in it that only the U-Boot script needs to know 
> about, such as the address of the loaded DTB and final kernel image.

This could possibly be a separate board in barebox...  Call it
atags_to_fdt? :)

thx,

Jason.

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Russell King - ARM Linux]

On Fri, Jun 07, 2013 at 11:21:01AM +0200, Thomas Petazzoni wrote:
> Well, I don't think what you say here is really fair. Before the DT was
> in place, unless I missed it, there was no standard way of letting the
> bootloader pass MAC addresses to the kernel.

Before DT was in place, passing MAC addresses via ATAGs had been
proposed several times.  This had been rejected each time.  Hooks
into the networking code had been proposed several times in various
forms, including via a command line option, and each time it had
been rejected by the networking folk.

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Rob Herring]

On Fri, Jun 7, 2013 at 4:21 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Dear Nicolas Pitre,
>
> On Thu, 6 Jun 2013 13:27:38 -0400 (EDT), Nicolas Pitre wrote:
>
> Marvell will release a DT-capable U-Boot in some time, that will
> properly adjust the MAC addresses in the DTB before handing it over to
> the kernel. We are currently working with Marvell to define which
> actions the bootloader should do on the DTB before handing it over to
> the kernel. I believe this is a sufficient proof that there is no
> laziness from Marvell's side and that things are progressing towards a
> full DT based boot protocol.

Hopefully the lesson on custom ATAGs has been learned and we don't
repeat it with something custom and undocumented in DT. I don't want
to hear "we're already using this binding in our bootloader and can't
change it". This example doesn't apply to MAC addresses since there
already is a defined way, but I'm sure there are other examples. That
is what I've gotten from the OLPC folks before.

Rob

[PATCH 2/5] arm: preserve ATAGS in /chosen/atags in the Device Tree

[Russell King - ARM Linux]

On Sat, Jun 08, 2013 at 01:54:27PM -0500, Rob Herring wrote:
> On Fri, Jun 7, 2013 at 4:21 AM, Thomas Petazzoni
> <thomas.petazzoni@free-electrons.com> wrote:
> > Dear Nicolas Pitre,
> >
> > On Thu, 6 Jun 2013 13:27:38 -0400 (EDT), Nicolas Pitre wrote:
> >
> > Marvell will release a DT-capable U-Boot in some time, that will
> > properly adjust the MAC addresses in the DTB before handing it over to
> > the kernel. We are currently working with Marvell to define which
> > actions the bootloader should do on the DTB before handing it over to
> > the kernel. I believe this is a sufficient proof that there is no
> > laziness from Marvell's side and that things are progressing towards a
> > full DT based boot protocol.
> 
> Hopefully the lesson on custom ATAGs has been learned and we don't
> repeat it with something custom and undocumented in DT. I don't want
> to hear "we're already using this binding in our bootloader and can't
> change it". This example doesn't apply to MAC addresses since there
> already is a defined way, but I'm sure there are other examples. That
> is what I've gotten from the OLPC folks before.

That is the standard argument given when people don't talk to us first
and _try_ to present us with a fait accompli.  They hope that we'll back
down.

[PATCH 3/5] of: net: introduce a of_set_mac_address() helper function

[Thomas Petazzoni]

The ARM MXS code already has some code to allocate and setup a
property that contains the MAC address, and the Marvell Armada 370/XP
code is going to gain a similar copy of this code. Therefore, let's
add a small helper function that does that, in drivers/of/of_net.c.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 drivers/of/of_net.c    | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/of_net.h |  6 ++++++
 2 files changed, 42 insertions(+)

diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
index ffab033..02af898 100644
--- a/drivers/of/of_net.c
+++ b/drivers/of/of_net.c
@@ -92,3 +92,39 @@ const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**
+ * Set a MAC address of a given network interface device tree node, if
+ * none was already defined.
+ */
+int of_set_mac_address(struct device_node *np, const void *mac)
+{
+	struct property *pp;
+	int ret;
+
+	if (of_get_mac_address(np))
+		return 0;
+
+	pp = kzalloc(sizeof(struct property) + ETH_ALEN, GFP_KERNEL);
+	if (!pp)
+		return -ENOMEM;
+
+	pp->value = pp + 1;
+	pp->length = ETH_ALEN;
+	memcpy(pp->value, mac, ETH_ALEN);
+	pp->name = kstrdup("local-mac-address", GFP_KERNEL);
+	if (!pp->name) {
+		kfree(pp);
+		return -ENOMEM;
+	}
+
+	ret = of_update_property(np, pp);
+	if (ret < 0) {
+		kfree(pp->name);
+		kfree(pp);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL(of_set_mac_address);
diff --git a/include/linux/of_net.h b/include/linux/of_net.h
index 61bf53b..a5086d9 100644
--- a/include/linux/of_net.h
+++ b/include/linux/of_net.h
@@ -11,6 +11,7 @@
 #include <linux/of.h>
 extern const int of_get_phy_mode(struct device_node *np);
 extern const void *of_get_mac_address(struct device_node *np);
+extern int of_set_mac_address(struct device_node *np, const void *mac);
 #else
 static inline const int of_get_phy_mode(struct device_node *np)
 {
@@ -21,6 +22,11 @@ static inline const void *of_get_mac_address(struct device_node *np)
 {
 	return NULL;
 }
+
+static inline int of_set_mac_address(struct device_node *np, const void *mac)
+{
+	return -ENODEV;
+}
 #endif
 
 #endif /* __LINUX_OF_NET_H */
-- 
1.8.1.2

[PATCH 3/5] of: net: introduce a of_set_mac_address() helper function

[Andrew Lunn]

On Wed, Jun 05, 2013 at 08:40:07AM +0200, Thomas Petazzoni wrote:
> The ARM MXS code already has some code to allocate and setup a
> property that contains the MAC address, and the Marvell Armada 370/XP
> code is going to gain a similar copy of this code. Therefore, let's
> add a small helper function that does that, in drivers/of/of_net.c.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
> ---
>  drivers/of/of_net.c    | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/of_net.h |  6 ++++++
>  2 files changed, 42 insertions(+)
> 
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index ffab033..02af898 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -92,3 +92,39 @@ const void *of_get_mac_address(struct device_node *np)
>  	return NULL;
>  }
>  EXPORT_SYMBOL(of_get_mac_address);
> +
> +/**
> + * Set a MAC address of a given network interface device tree node, if
> + * none was already defined.
> + */
> +int of_set_mac_address(struct device_node *np, const void *mac)
> +{
> +	struct property *pp;
> +	int ret;
> +
> +	if (of_get_mac_address(np))
> +		return 0;

Hi Thomas

The semantics is a bit odd here for a set function. Only set if not
already set. From the function name, its not clear it has this extra
semantics. of_set_if_not_set_mac_address() seems a bit long, but makes
it clearer. Or maybe of_set_default_mac_address()?

I've no strong preference here, i just see the potential for bugs.

     Andrew

[PATCH 3/5] of: net: introduce a of_set_mac_address() helper function

[Thomas Petazzoni]

Dear Andrew Lunn,

On Wed, 5 Jun 2013 09:13:54 +0200, Andrew Lunn wrote:

> The semantics is a bit odd here for a set function. Only set if not
> already set. From the function name, its not clear it has this extra
> semantics. of_set_if_not_set_mac_address() seems a bit long, but makes
> it clearer. Or maybe of_set_default_mac_address()?
> 
> I've no strong preference here, i just see the potential for bugs.

Yes, I agree that a set function that doesn't always set is not
entirely nice, but the part I wanted to get reviewed and hopefully
accepted is PATCH 2/5. If that gets accepted, then I'm all open to
suggestions to improve this PATCH 3/5 by either renaming the function
to a more appropriate name, or change its behavior, depending on
reviewers suggestions.

Maybe of_set_mac_address() should simply be setting it, and callers are
responsible for checking with of_get_mac_address() first if no MAC
address is already set?

Best regards,

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

[PATCH 3/5] of: net: introduce a of_set_mac_address() helper function

[Grant Likely]

On Wed, Jun 5, 2013 at 7:40 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> The ARM MXS code already has some code to allocate and setup a
> property that contains the MAC address, and the Marvell Armada 370/XP
> code is going to gain a similar copy of this code. Therefore, let's
> add a small helper function that does that, in drivers/of/of_net.c.
>
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

For the vast majority of the kernel the device tree is supposed to be
a read-only structure. It's not something that should be modified at
all. If a platform has another source for obtaining a MAC address,
then it should either be injected into the DT by firmware. I'm not
saying no to this, but I am saying you need to be careful. If kexec is
ever used on this problem then the new kernel will get the updated mac
address which may or may not be what you want.

Looking at the mxs code, it would appear that the platform support
code will generate the mac address from a 32-bit value read out of
something like flash or eeprom. That in itself is fine, it is just a
question of how best to pass on that data. The other alternative is to
have an API for lookup hooks that platform code can hook into. That
could get quite a bit more complicated though.

Any DT mac address set function has a number of limits that need to be
applied. First; it needs to be called before any initcalls, and
probably before an of_platform_populate calls so that it doesn't cause
data to change under the feet of device drivers when the device is
created. If it is called too late, then it should output an error and
refuse to do anything. The mac address should not be allowed to change
under the feet of any driver using it. It would also need to be an
__init function. Finally, need to take care to make sure that the
sysfs & proc representations are correct after that function is
called. of_update_property() should take care of that, but you should
double check that it does.

Finally, if you are going to do this, then can you split the function
into two: of_set_property() and of_set_mac_address() and put all of
the memory allocation bits into the of_set_property() half. Sparc
actually already has an of_set_property() implementation, but it isn't
directly applicable due to the different back-end Sparc uses for
working with the device tree. Bonus points if you factor the common
bits out of the Sparc version at the same time. Make the Sparc version
use the prom_setprop() backend, while everyone else uses
of_update_property()

Other comments below...

> ---
>  drivers/of/of_net.c    | 36 ++++++++++++++++++++++++++++++++++++
>  include/linux/of_net.h |  6 ++++++
>  2 files changed, 42 insertions(+)
>
> diff --git a/drivers/of/of_net.c b/drivers/of/of_net.c
> index ffab033..02af898 100644
> --- a/drivers/of/of_net.c
> +++ b/drivers/of/of_net.c
> @@ -92,3 +92,39 @@ const void *of_get_mac_address(struct device_node *np)
>         return NULL;
>  }
>  EXPORT_SYMBOL(of_get_mac_address);
> +
> +/**
> + * Set a MAC address of a given network interface device tree node, if
> + * none was already defined.
> + */
> +int of_set_mac_address(struct device_node *np, const void *mac)

__init

> +{
> +       struct property *pp;
> +       int ret;
> +
> +       if (of_get_mac_address(np))
> +               return 0;
> +
> +       pp = kzalloc(sizeof(struct property) + ETH_ALEN, GFP_KERNEL);
> +       if (!pp)
> +               return -ENOMEM;
> +
> +       pp->value = pp + 1;

&pp[1].

> +       pp->length = ETH_ALEN;
> +       memcpy(pp->value, mac, ETH_ALEN);
> +       pp->name = kstrdup("local-mac-address", GFP_KERNEL);

"local-mac-address" is a static string. You're safe to directly assign
it here. I do realize that one of the property free paths does a
kfree() on the name field, but that only works on OF_DYNAMIC nodes.
You're not working with OF_DYNAMIC here. Other of_update_property()
calls already use the static name.

As an aside, it would probably be a good idea to modify the OF_DYNAMIC
path to make the name field and property structure allocated in the
same kzalloc() call, but that's a change for another patch set.

> +       if (!pp->name) {
> +               kfree(pp);
> +               return -ENOMEM;
> +       }
> +
> +       ret = of_update_property(np, pp);
> +       if (ret < 0) {
> +               kfree(pp->name);
> +               kfree(pp);
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +EXPORT_SYMBOL(of_set_mac_address);

Don't export this symbol. Make it built-in only.

> diff --git a/include/linux/of_net.h b/include/linux/of_net.h
> index 61bf53b..a5086d9 100644
> --- a/include/linux/of_net.h
> +++ b/include/linux/of_net.h
> @@ -11,6 +11,7 @@
>  #include <linux/of.h>
>  extern const int of_get_phy_mode(struct device_node *np);
>  extern const void *of_get_mac_address(struct device_node *np);
> +extern int of_set_mac_address(struct device_node *np, const void *mac);
>  #else
>  static inline const int of_get_phy_mode(struct device_node *np)
>  {
> @@ -21,6 +22,11 @@ static inline const void *of_get_mac_address(struct device_node *np)
>  {
>         return NULL;
>  }
> +
> +static inline int of_set_mac_address(struct device_node *np, const void *mac)
> +{
> +       return -ENODEV;
> +}
>  #endif
>
>  #endif /* __LINUX_OF_NET_H */
> --
> 1.8.1.2
>

[PATCH 4/5] arm: mxs: use the newly introduced of_set_mac_address() helper

[Thomas Petazzoni]

This commit refactors the existing mach-mxs code to use the newly
introduced of_set_mac_address() OF helper.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mxs/mach-mxs.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/arch/arm/mach-mxs/mach-mxs.c b/arch/arm/mach-mxs/mach-mxs.c
index 5b62b64..5cdfa55 100644
--- a/arch/arm/mach-mxs/mach-mxs.c
+++ b/arch/arm/mach-mxs/mach-mxs.c
@@ -23,6 +23,7 @@
 #include <linux/irqchip/mxs.h>
 #include <linux/micrel_phy.h>
 #include <linux/of_address.h>
+#include <linux/of_net.h>
 #include <linux/of_platform.h>
 #include <linux/phy.h>
 #include <linux/pinctrl/consumer.h>
@@ -180,9 +181,8 @@ enum mac_oui {
 static void __init update_fec_mac_prop(enum mac_oui oui)
 {
 	struct device_node *np, *from = NULL;
-	struct property *newmac;
 	const u32 *ocotp = mxs_get_ocotp();
-	u8 *macaddr;
+	u8 macaddr[ETH_ALEN];
 	u32 val;
 	int i;
 
@@ -193,26 +193,10 @@ static void __init update_fec_mac_prop(enum mac_oui oui)
 
 		from = np;
 
-		if (of_get_property(np, "local-mac-address", NULL))
-			continue;
-
-		newmac = kzalloc(sizeof(*newmac) + 6, GFP_KERNEL);
-		if (!newmac)
-			return;
-		newmac->value = newmac + 1;
-		newmac->length = 6;
-
-		newmac->name = kstrdup("local-mac-address", GFP_KERNEL);
-		if (!newmac->name) {
-			kfree(newmac);
-			return;
-		}
-
 		/*
 		 * OCOTP only stores the last 4 octets for each mac address,
 		 * so hard-code OUI here.
 		 */
-		macaddr = newmac->value;
 		switch (oui) {
 		case OUI_FSL:
 			macaddr[0] = 0x00;
@@ -235,7 +219,7 @@ static void __init update_fec_mac_prop(enum mac_oui oui)
 		macaddr[4] = (val >> 8) & 0xff;
 		macaddr[5] = (val >> 0) & 0xff;
 
-		of_update_property(np, newmac);
+		of_set_mac_address(np, macaddr);
 	}
 }
 
-- 
1.8.1.2

[PATCH 5/5] arm: mvebu: parse ATAGS to find the network interfaces MAC addresses

[Thomas Petazzoni]

Now that the atags_to_fdt code stores the ATAGS in /chosen/atags in
the Device Tree, the platform-specific code of Armada 370/XP can parse
those ATAGS, and search for the Marvell custom ATAG that contains the
MAC addresses, and use the newly introduced of_set_mac_address() to
adjust the Device Tree with those MAC addresses.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 arch/arm/mach-mvebu/armada-370-xp.c | 52 +++++++++++++++++++++++++++++++++++++
 arch/arm/mach-mvebu/armada-370-xp.h | 13 ++++++++++
 2 files changed, 65 insertions(+)

diff --git a/arch/arm/mach-mvebu/armada-370-xp.c b/arch/arm/mach-mvebu/armada-370-xp.c
index 1c48890..1ee6fa4 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.c
+++ b/arch/arm/mach-mvebu/armada-370-xp.c
@@ -14,6 +14,7 @@
 
 #include <linux/kernel.h>
 #include <linux/init.h>
+#include <linux/of_net.h>
 #include <linux/of_platform.h>
 #include <linux/io.h>
 #include <linux/time-armada-370-xp.h>
@@ -21,10 +22,13 @@
 #include <linux/dma-mapping.h>
 #include <linux/mbus.h>
 #include <linux/irqchip.h>
+#include <linux/slab.h>
+#include <linux/if_ether.h>
 #include <asm/hardware/cache-l2x0.h>
 #include <asm/mach/arch.h>
 #include <asm/mach/map.h>
 #include <asm/mach/time.h>
+#include <asm/setup.h>
 #include "armada-370-xp.h"
 #include "common.h"
 #include "coherency.h"
@@ -73,8 +77,56 @@ void __init armada_370_xp_init_early(void)
 #endif
 }
 
+/* Get MAC address for the legacy ATAGs, if available */
+static void __init armada_370_xp_get_mac_addr(void)
+{
+	const struct tag *atag, *mvatag;
+	const struct tag_mv_uboot *mvubootatag;
+	struct device_node *aliases, *chosen;
+	const void *atag_list;
+	int i;
+
+	chosen = of_find_node_by_path("/chosen");
+	if (!chosen)
+		return;
+
+	atag_list = of_get_property(chosen, "atags", NULL);
+	if (!atag_list)
+		return;
+
+	mvatag = NULL;
+	for_each_tag(atag, atag_list) {
+		if (atag->hdr.tag == ATAG_MV_UBOOT) {
+			mvatag = atag;
+			break;
+		}
+	}
+
+	if (!mvatag)
+		return;
+
+	mvubootatag = ((void *) mvatag) + sizeof(struct tag_header);
+
+	aliases = of_find_node_by_path("/aliases");
+	for (i = 0; i < 4; i++) {
+		struct device_node *ethnode;
+		const char *ethnodepath;
+		char ethname[5];
+		snprintf(ethname, sizeof(ethname), "eth%d", i);
+		if (of_property_read_string(aliases, ethname, &ethnodepath) < 0)
+			continue;
+
+		ethnode = of_find_node_by_path(ethnodepath);
+		if (!ethnode)
+			continue;
+
+		of_set_mac_address(ethnode, mvubootatag->macaddr[i]);
+	}
+}
+
 static void __init armada_370_xp_dt_init(void)
 {
+	armada_370_xp_get_mac_addr();
 	of_platform_populate(NULL, of_default_bus_match_table, NULL, NULL);
 	coherency_init();
 }
diff --git a/arch/arm/mach-mvebu/armada-370-xp.h b/arch/arm/mach-mvebu/armada-370-xp.h
index 2070e1b..329fb66 100644
--- a/arch/arm/mach-mvebu/armada-370-xp.h
+++ b/arch/arm/mach-mvebu/armada-370-xp.h
@@ -25,6 +25,19 @@
 #define ARMADA_370_XP_SDRAM_WINS_BASE   (ARMADA_370_XP_REGS_PHYS_BASE + 0x20180)
 #define ARMADA_370_XP_SDRAM_WINS_SIZE   0x20
 
+/* 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;
+};
+
 #ifdef CONFIG_SMP
 #include <linux/cpumask.h>
 
-- 
1.8.1.2