Honor /memory/reg node in DTB files

commit 341764495180a712b9aaccfa0479b2ff7e44e35b

Author: Deepak Saxena <[email protected]>
Date:   Mon Dec 6 15:52:07 2010 -0800

Honor /memory/reg node in DTB files

This patch adds code to the bootm path to check if a valid /memory/reg node exists in the DTB file and if so, it does not override it with the values in bi_memstart and bi_memsize. This is particularly useful in multi-core environments where the memory may be partitioned across a large number of nodes.

While the same can be accomplished on certain boards (p1022ds and p1_p2_rdb) by using the bootm_low and bootm_size environment variables, that solution is not universal and requires adding code ft_board_setup() for any new board that wants to support AMP operation. Also, given that the DTB is already used to partition board devices (see commit dc2e673 in the Linux kernel tree), it makes sense to allow memory to be partitioned the same way from a user configuration perspective.

This patch allows for the user to override the DTB file parameters on the p1022ds and p1_p2_rdb boards by setting the bootm_low and bootm_size to something other than bi_memstart and bi_memsize. In the long-term, those env variables should be depecrated and removed and system implementors should provide the memory partitioning information in the DTB.

Signed-off-by Deepak Saxena <[email protected]>

Signed-off-by Hollis Blanchard <[email protected]>

Dear Deepak Saxena,

I am not sure this is a good idea.

So far we have a pretty clear definition of responsibilities. U-Boot does the low level initialization, including the sizing and testing of the system memory. U-Boot then passes its results to Linux in the (modified by U-Boot) device tree.

Your patch inverts this situation, at least for the memory.

I agree that there may be situations where you want an easy way to pass such information. But then let's handle this right.

If you define that the device tree is the "master" for information about the memory layout (and potentially other hardware specifics), then you should be consequent and pass make U-Boot process this information. We've discussed before that there are a number of cases where it would be nice if U-Boot itself could be configured usign a device tree. This appears to be another one.

What do you think?

I am a big fan of having consistent and clear definitions of responsibilities; however, I think the model of having U-Boot handle the creation of memory nodes in the DTB does not scale cleanly to users configuring, deploying, and managing complicated AMP environments.

While we could provide a method to provide this information at build time to make U-Boot, this forces a static memory partitioning on the system and does not mesh well with use cases where developers may

The bootm_low and bootm_size variables are an attempt to get around this by overriding what U-Boot knows about the system but I think those also don't scale well when we start dealing with large numbers of cores (32+) with independent OS instances on them for some of the same reasons. I think it is far simpler to have some custom scripts to spit out new DTB files that uBoot is configured to load every time it boots than to have to change a bunch of environment variables on boot.

In the multi-node environments, we can't simply tell U-Boot to process the DTB to determine how much memory is in the system as there is one DTB per AMP node. One idea that comes to mind but that I think is not

the right way to go due to complexity is to have the concept of nested DTBs that can define multiple operational "machines" and U-Boot knows how to read this and send the right sub-DTB to the right kernel image.

I'm new to U-Boot development so would like to hear about the other use cases you mentioned (pointer to email threads are OK) so I can have a better understanding of the overall issues.

Thinking about this at a higher level, I think the question is where is the delineation between board bringup/configuration and run time configuration management? Scanning memory and determining how much exists and programming the memory controller is a board-bring up and configuration task that a bootloader has traditionally done.

Partitioning for AMP operation is about managing what and how is running on top of the bootloader. How much knowledge should the bootloader have about this? The approach of providing the memory partitioning in the DTB basically removes any of this knowledge from U-Boot, while the

other approaches (bootm, build-time configuration) make U-Boot very aware and tied to what might be running above and reduce flexibility to easily change that.

There are many board vendors who shipt boards with different configurations - with or without NAND flash; with or without other peripherals like CAN contollers, LCD, etc.; with different LCD sizes and types, in portrait or landscape orientation, etc. Some of these features can be determined by probing the hardware, others (like the orientation of a LCD) cannot. It is sometimes a maintenance nightmare to provide tens of different configurations of U-Boot for a single product. Being able to cinfigure available hardware through the DT, and using a single common binary image of U-Boot for such systems would be a great benefit.

Quoted 10 lines · Wolfgang Denk

There are many board vendors who shipt boards with different configurations - with or without NAND flash; with or without other peripherals like CAN contollers, LCD, etc.; with different LCD sizes and types, in portrait or landscape orientation, etc. Some of these features can be determined by probing the hardware, others (like the orientation of a LCD) cannot. It is sometimes a maintenance nightmare to provide tens of different configurations of U-Boot for a single product. Being able to cinfigure available hardware through the DT, and using a single common binary image of U-Boot for such systems would be a great benefit.

That's fine, but so far I don't see how it's related. This is information u-boot needs during its own initialization, right?

We need a way for our tools to specify information to the kernels' initialization, and still want u-boot to do all the hardware configuration it does today. It really doesn't matter to us if in the future u-boot uses device trees for that configuration: we just need a way to interact with the kernels.

Dear Hollis Blanchard,

Quoted 11 lines · In message<[email protected]> you wrote:

Quoted 10 lines · Wolfgang Denk

There are many board vendors who shipt boards with different configurations - with or without NAND flash; with or without other peripherals like CAN contollers, LCD, etc.; with different LCD sizes and types, in portrait or landscape orientation, etc. Some of these features can be determined by probing the hardware, others (like the orientation of a LCD) cannot. It is sometimes a maintenance nightmare to provide tens of different configurations of U-Boot for a single product. Being able to cinfigure available hardware through the DT, and using a single common binary image of U-Boot for such systems would be a great benefit.

Quoted 2 lines

That's fine, but so far I don't see how it's related. This is information u-boot needs during its own initialization, right?

Right.

Quoted 5 lines

We need a way for our tools to specify information to the kernels' initialization, and still want u-boot to do all the hardware configuration it does today. It really doesn't matter to us if in the future u-boot uses device trees for that configuration: we just need a way to interact with the kernels.

When U-boot is supposed to do the hardware initialization, you probably include memory initialization, right? If so, should we then not make sure that U-Boot and the OS we're booting use the same information about this resource?

If, on the other hand, you really want to make U-Boot ignore any /memory nodes in the device tree, then this should be done straight and without additional magic. In this case the DT should be responsible to provide all information the OS needs, and U-Boot should not touch this in any way. Then just do not call fdt_fixup_memory() at all for such configurations.

I dislike the idea that U-Boot will not touch the DT entry in one situation, but will do so in another, without any visibility to (and eventually without awareness by) the user.

If there is really a good reason for such magic, then this should be clearly documented (not only in some comment in some source file), and eventually fdt_fixup_memory() (or fdt_fixup_memory_banks() ?) should be made weak so it can be redefined by board-specific implementations.

In any case, any such changes should be implemented in a generic way, i. e. not only for one specific processor family.

I think the current way that u-boot updates the memory node is valuable for other use cases. In particular, it is very convenient for single-OS systems. Our goal is to avoid affecting those use cases.

Quoted 3 lines

I dislike the idea that U-Boot will not touch the DT entry in one situation, but will do so in another, without any visibility to (and eventually without awareness by) the user.

I see it as allowing the user to optionally override auto-detected information. The user has to go out of their way to request this behavior, so I think the visibility/awareness is there.

The existing bootm_low/bootm_size facility does exactly this, but I think we can improve on its design.

I agree that all (device tree aware) systems should behave consistently in this regard. Of course, in that case, we don't need weak functions?

If you want to make this switchable at runtime, then we should probably use an environment setting.

Quoted 3 lines

If you want to make this switchable at runtime, then we should probably use an environment setting.

I experimented with this, but could never determine the best way to cover all behavior. Do we have a variable that indicates "don't update DT?" If so, it means we have to place all values in the DT, when it's really nice for U-Boot to do some of that for us. In fact, we want U-Boot to update many things it discovers, just not the few we wish to actually (sometimes) define for ourselves.

The other alternative (granted, I'm not as smart as I used to be :-)) was to have an environment variable that specified which node we didn't want updated by U-Boot. For now, that seems reasonable since there is only one, but is that the general approach we want in the future? It also presents the challenge of having to update both environment and the provided DT.

Dear Dan,

Quoted 11 lines · In message <[email protected]> you wrote:

Quoted 3 lines

If you want to make this switchable at runtime, then we should probably use an environment setting.

I experimented with this, but could never determine the best way to cover all behavior. Do we have a variable that indicates "don't update DT?" If so, it means we have to place all values in the DT, when it's really nice for U-Boot to do some of that for us. In fact, we want U-Boot to update many things it discovers, just not the few we wish to actually (sometimes) define for ourselves.

"You can please all the people some of the time and some of the people all of the time but you can't please all the people all of the time."

Quoted 7 lines

The other alternative (granted, I'm not as smart as I used to be :-)) was to have an environment variable that specified which node we didn't want updated by U-Boot. For now, that seems reasonable since there is only one, but is that the general approach we want in the future? It also presents the challenge of having to update both environment and the provided DT.

I guess we can argue that the normal situation is that U-Boot will know how to update the DT such as needed to boot the OS. So what we are dealing with is a small percentage of cases where we need special behaviour, and where it may be acceptable if the solution is only semi-perfect ;-)

My current thinking is to introduce something like

dt_skip=memory,mac-address

"You can please all the people some of the time and some of the people all of the time but you can't please all the people all of the time."

My current thinking is to introduce something like .....

dt_skip=memory,mac-address

Yes, I'm sometimes pleased.... :-)

Quoted 1 line

My current thinking is to introduce something like .....

Well, that is pretty cool.

Quoted 1 line

dt_skip=memory,mac-address

Do we have to write a parser now, or is there something that currently exists to help out? :-)

I guess we can argue that the normal situation is that U-Boot will know how to update the DT such as needed to boot the OS. So what we are dealing with is a small percentage of cases where we need special behaviour, and where it may be acceptable if the solution is only semi-perfect ;-)

My current thinking is to introduce something like

dt_skip=memory,mac-address

including eventually "dt_skip=ALL". This should cover most of the current use cases.

If someone gets fancy he can add wildcard support.

And if we need even more flexibility, we can add some "dt_include" with higher priority, so one could do for example

dt_skip=ALL
dt_include=memory

Quoted 14 lines · Wolfgang Denk

I guess we can argue that the normal situation is that U-Boot will know how to update the DT such as needed to boot the OS. So what we are dealing with is a small percentage of cases where we need special behaviour, and where it may be acceptable if the solution is only semi-perfect ;-)

My current thinking is to introduce something like

dt_skip=memory,mac-address

including eventually "dt_skip=ALL". This should cover most of the current use cases.

Quoted 7 lines

If someone gets fancy he can add wildcard support.

And if we need even more flexibility, we can add some "dt_include" with higher priority, so one could do for example

dt_skip=ALL
dt_include=memory

I imagine this being rather ugly to implement and to keep the code clean and maintained. Who parses these variables? Does each and every piece of code in U-Boot that now touches a piece of the DT need to check for this variable? I could see something like this working
if there was a central DT handler that read nodes and then called platform-specific over-ride function, i.e.:

	for_each_node_in_dt() {
		if (dt_include(node->type))
			platform_of_dt_node_process(node, boot_stage);
	}

	where boot_stage tells us whether we are at early init, about to
        boot an OS image, or in some other step in the process.

This would provide a consistent method of handling that variable. Without something like this, I think and environment variable is just going to create confusion for users and developers.

So far we usually had pretty static board configurations, and a static compile time description was all we needed. Some developers consider even simple extensions like auto-sizing the available RAM as unnecessary luxury that just inreases the boot time and memory footprint.

When it comes to more complicated setups we should provide means for a more dynamic configuration - this has been discussed before, and there was a general agreement that the device tree would be a usable way to implement such a description of the hardware.

So what I'm proposing is not an opposite to what you have in mind, it just takes it one step further, and makes the whole system consistent again.

Waht I don't like is the tunneling of information through U-Boot, while U-Boot actually needs and uses this very information as well.

Quoted 3 lines

While we could provide a method to provide this information at build time to make U-Boot, this forces a static memory partitioning on the system and does not mesh well with use cases where developers may

This is NOT what I suggested.

Quoted 3 lines

In the multi-node environments, we can't simply tell U-Boot to process the DTB to determine how much memory is in the system as there is one DTB per AMP node. One idea that comes to mind but that I think is not

Please explain: you can use the DT to tell Linux (or other OS) how much memory they shoulduse, but you cannot use the same mechanism to pass the same information to U-Boot?

Quoted 4 lines

the right way to go due to complexity is to have the concept of nested DTBs that can define multiple operational "machines" and U-Boot knows how to read this and send the right sub-DTB to the right kernel image.

This is a technical details that can and should be discussed when we have an agreement about the basic mechanism.

Quoted 3 lines

I'm new to U-Boot development so would like to hear about the other use cases you mentioned (pointer to email threads are OK) so I can have a better understanding of the overall issues.

There are many board vendors who shipt boards with different configurations - with or without NAND flash; with or without other peripherals like CAN contollers, LCD, etc.; with different LCD sizes and types, in portrait or landscape orientation, etc. Some of these features can be determined by probing the hardware, others (like the orientation of a LCD) cannot. It is sometimes a maintenance nightmare to provide tens of different configurations of U-Boot for a single product. Being able to cinfigure available hardware through the DT, and using a single common binary image of U-Boot for such systems would be a great benefit.

Quoted 4 lines

Partitioning for AMP operation is about managing what and how is running on top of the bootloader. How much knowledge should the bootloader have about this? The approach of providing the memory partitioning in the DTB basically removes any of this knowledge from U-Boot, while the

I see many use cases where this is simply not possible, because you need need the help of the bootloader to determine parameters that are not known in advance, and that thus cannot be encoded in the DT. Memory size if a very typical example for such a parameter. It may be OK for the use case you have currently in mind to use a fixed size, but this covers just a few systems and is not flexible enough for general use.

Quoted 4 lines

Please explain: you can use the DT to tell Linux (or other OS) how much memory they shoulduse, but you cannot use the same mechanism to pass the same information to U-Boot?

I'm not against U-Boot using this information, I'm just not sure how to do this without adding quite a bit of complexity to the code base. We would have to have U-Boot parse the memory nodes, validate them, check for overlapping regions, check for holes, etc. I'm not arguing that it is not doable, but wondering if adding this complexity is worth if the scanning of memory and passing that information to the kernel works for the majority of use cases. What I'm trying to do is support a special use case, so what about wrapping support for simply passing the memory nodes from the DT to the kernel around a CONFIG option (CONFIG_OF_MEMORY_PASSTHROUGH?) that can be enabled by system implementers who need this and are running on fairly controlled environments while the larger issue of how to use the DT is hashed out?

If you define that the device tree is the "master" for information about the memory layout (and potentially other hardware specifics), then you should be consequent and pass make U-Boot process this information. We've discussed before that there are a number of cases where it would be nice if U-Boot itself could be configured usign a device tree. This appears to be another one.

Quoted 6 lines · Wolfgang Denk

If you define that the device tree is the "master" for information about the memory layout (and potentially other hardware specifics), then you should be consequent and pass make U-Boot process this information. We've discussed before that there are a number of cases where it would be nice if U-Boot itself could be configured usign a device tree. This appears to be another one.

I *think* what you're suggesting is basically providing u-boot with a single device tree, even when it will load multiple operating systems. The tree would then look something like this:

     cpus
         ...
     memory
         reg = <0 20000000>
     soc
         ...
     partitions
         partition at 0
             memory
                 reg = <0 10000000>
         partition at 1
             memory
                 reg = <10000000 10000000>

U-boot would then be responsible for constructing multiple device trees (one for each partition) itself, based on the additional information found in the "partitions" node.

Is that correct?

+/*
+ * Check to see if an valid memory/reg property exists
+ * in the fdt. If so, we do not overwrite it with what's
+ * been scanned.
+ *

+ * Valid mean all the following:
+ *
+ * - Memory node has a device-type of "memory"
+ * - A reg property exists which:
+ * + has exactly as many cells as #address-cells + #size-cells
+ * + provides a range that is within [bi_memstart, bi_memstart +
bi_memsize]
+ */

Quoted 5 lines · On Mon, 6 Dec 2010 16:56:26 -0800 Deepak Saxena <[email protected]>…

+/*
+ * Check to see if an valid memory/reg property exists
+ * in the fdt. If so, we do not overwrite it with what's
+ * been scanned.
+ *

Quoted 8 lines

+ * Valid mean all the following:
+ *
+ * - Memory node has a device-type of "memory"
+ * - A reg property exists which:
+ * + has exactly as many cells as #address-cells + #size-cells
+ * + provides a range that is within [bi_memstart, bi_memstart +
bi_memsize]
+ */

This will get false positives -- a lot of existing device tree templates have something like this:

Quoted 17 lines · Scott Wood

Quoted 13 lines · On Mon, 6 Dec 2010 16:56:26 -0800 Deepak Saxena <[email protected]>…

+/*
+ * Check to see if an valid memory/reg property exists
+ * in the fdt. If so, we do not overwrite it with what's
+ * been scanned.
+ *
+ * Valid mean all the following:
+ *
+ * - Memory node has a device-type of "memory"
+ * - A reg property exists which:
+ * + has exactly as many cells as #address-cells + #size-cells
+ * + provides a range that is within [bi_memstart, bi_memstart +
bi_memsize]
+ */

Quoted 2 lines

This will get false positives -- a lot of existing device tree templates have something like this:

ACK. The code in the patch actually checks for this, just didn't point it out in the comments.

Probably want to complain to the user if reg is invalid and not zero/missing.

Quoted 2 lines · Scott Wood

Probably want to complain to the user if reg is invalid and not zero/missing.

I think you guys are making this too complicated. There are many ways to pass stupid mistakes via
a device tree, don't get carried away trying to single out this one for error checking where the user is likely to really know what they are doing. This isn't a required specification to get correct, without anything u-boot will provide the proper information.

commit 341764495180a712b9aaccfa0479b2ff7e44e35b

Author: Deepak Saxena <[email protected]>
Date:   Mon Dec 6 15:52:07 2010 -0800

Honor /memory/reg node in DTB files

This patch adds code to the bootm path to check if a valid /memory/reg node exists in the DTB file and if so, it does not override it with the values in bi_memstart and bi_memsize. This is particularly useful in multi-core environments where the memory may be partitioned across a large number of nodes.

While the same can be accomplished on certain boards (p1022ds and p1_p2_rdb) by using the bootm_low and bootm_size environment variables, that solution is not universal and requires adding code ft_board_setup() for any new board that wants to support AMP operation. Also, given that the DTB is already used to partition board devices (see commit dc2e673 in the Linux kernel tree), it makes sense to allow memory to be partitioned the same way from a user configuration perspective.

This patch allows for the user to override the DTB file parameters on the p1022ds and p1_p2_rdb boards by setting the bootm_low and bootm_size to something other than bi_memstart and bi_memsize. In the long-term, those env variables should be depecrated and removed and system implementors should provide the memory partitioning information in the DTB.

Signed-off-by Deepak Saxena <[email protected]>

Signed-off-by Hollis Blanchard <[email protected]>

---

See http://lists.denx.de/pipermail/u-boot/2010-December/083057.html
for initial proposal on this.

(quoted diff) +0 -0

b/arch/powerpc/cpu/mpc85xx/fdt.c index 4540364..6d384e3 100644

arch/powerpc/cpu/mpc85xx/fdt.c +11 -0

@@ -377,6 +377,55 @@ static void ft_fixup_qe_snum(void *blob)
  }
  #endif
 
+/*
+ * Check to see if an valid memory/reg property exists
+ * in the fdt. If so, we do not overwrite it with what's
+ * been scanned.
+ *
+ * Valid mean all the following:
+ *
+ * - Memory node has a device-type of "memory"
+ * - A reg property exists which:
+ *   + has exactly as many cells as #address-cells + #size-cells
+ *   + provides a range that is within [bi_memstart, bi_memstart +

bi_memsize]
+ */

arch/powerpc/cpu/mpc85xx/fdt.c +39 -1

+static int ft_validate_memory(void *blob, bd_t *bd)
+{
+	int nodeoffset;
+	u32 *addrcell = (u32*)fdt_getprop(blob, 0, "#address-cells", NULL);
+	u32 *sizecell = (u32*)fdt_getprop(blob, 0, "#size-cells", NULL);
+	u64 reg_base, reg_size;
+	void *reg, *dtype;
+	int len;
+
+	if ((nodeoffset = fdt_path_offset(blob, "/memory")) >= 0)
+	{
+		dtype = fdt_getprop(blob, nodeoffset, "device_type", &len);
+		if (!dtype || (strcmp(dtype, "memory") != 0))
+			return 0;
+
+		reg = fdt_getprop(blob, nodeoffset, "reg", &len);
+		if (reg && len == ((*addrcell + *sizecell) * 4)) {
+			if (*addrcell == 2) {
+				reg_base = ((u64*)reg)[0];
+				reg_size = ((u64*)reg)[1];
+			} else {
+				reg_base = ((u32*)reg)[0];
+				reg_size = ((u32*)reg)[1];
+			}
+
+			if ((reg_size) &&
+			    (reg_base >= (u64)bd->bi_memstart) &&
+			    ((reg_size + reg_base)
+			     <= ((u64)bd->bi_memstart + (u64)bd->bi_memsize)))
+				return 1;
+		}
+	}
+
+	return 0;
+
+}
+
  void ft_cpu_setup(void *blob, bd_t *bd)
  {
  	int off;

@@ -434,7 +483,8 @@ void ft_cpu_setup(void *blob, bd_t *bd)
  		"clock-frequency", CONFIG_SYS_CLK_FREQ, 1);
  #endif
 
-	fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd->bi_memsize);
+	if (!ft_validate_memory(blob, bd))
+		fdt_fixup_memory(blob, (u64)bd->bi_memstart, (u64)bd-
 bi_memsize);
 
  #ifdef CONFIG_MP

  	ft_fixup_cpu(blob, (u64)bd->bi_memstart + (u64)bd->bi_memsize);

board/freescale/p1022ds/p1022ds.c +2 -1

index 5cdee9f..7378d88 100644
@@ -320,7 +320,8 @@ void ft_board_setup(void *blob, bd_t *bd)
  	base = getenv_bootm_low();
  	size = getenv_bootm_size();
 
-	fdt_fixup_memory(blob, (u64)base, (u64)size);
+	if (base != (phys_addr_t)bd->bi_memstart && size !=
 (phys_addr_t)bd->bi_memsize)
+		fdt_fixup_memory(blob, (u64)base, (u64)size);
 
  	FT_FSL_PCI_SETUP;

board/freescale/p1_p2_rdb/p1_p2_rdb.c +0 -0

index fae31f2..5e4adc6 100644
@@ -220,9 +220,10 @@ void ft_board_setup(void *blob, bd_t *bd)
  	base = getenv_bootm_low();
  	size = getenv_bootm_size();