Mechanism for platform-specific parsing of ATAGs

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 <[email protected]>

---
 arch/arm/boot/compressed/atags_to_fdt.c | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/compressed/atags_to_fdt.c +6 -0

index aabc02a..8b2ae61 100644
@@ -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);
 }

Signature · 1 line

1.8.1.2

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 <[email protected]>

---
 arch/arm/boot/compressed/atags_to_fdt.c | 6 ++++++
 1 file changed, 6 insertions(+)

arch/arm/boot/compressed/atags_to_fdt.c +6 -0

index aabc02a..8b2ae61 100644
@@ -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);
 }

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.

Quoted 3 lines

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.

Dear Nicolas Pitre,

Quoted 14 lines · Nicolas Pitre

Quoted 3 lines

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.

Quoted 6 lines

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.

Quoted 7 lines

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.

Dear Jason Cooper,

Quoted 10 lines · Jason Cooper

Quoted 7 lines

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.

Quoted 10 lines

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.

Dear Nicolas Pitre,

Quoted 14 lines · Nicolas Pitre

Quoted 3 lines

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.

Quoted 6 lines

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.

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.

Quoted 1 line · Thomas Petazzoni

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

Quoted 3 lines

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.

Quoted 2 lines

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.

Quoted 3 lines

I perfectly understand Nico and Russell concerns, for sure. But I'd also like to find a workable solution:

Quoted 4 lines

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

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.

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.

Quoted 11 lines · Thomas Petazzoni

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.

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 <[email protected]>

---
 drivers/of/of_net.c    | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/of_net.h |  6 ++++++
 2 files changed, 42 insertions(+)

drivers/of/of_net.c +2 -0

index ffab033..02af898 100644
@@ -92,3 +92,39 @@ const void *of_get_mac_address(struct device_node *np)
 	return NULL;
 }
 EXPORT_SYMBOL(of_get_mac_address);
+
+/**

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.

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 <[email protected]>

---
 drivers/of/of_net.c    | 36 ++++++++++++++++++++++++++++++++++++
 include/linux/of_net.h |  6 ++++++
 2 files changed, 42 insertions(+)

drivers/of/of_net.c +0 -0

index ffab033..02af898 100644

drivers/of/of_net.c +5 -0

+       if (!pp->name) {
+               kfree(pp);
+               return -ENOMEM;
+       }
+

include/linux/of_net.h +6 -1

index 61bf53b..a5086d9 100644
@@ -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