Re: [U-Boot] [PATCH] Honor /memory/reg node in DTB files

[Deepak Saxena]

On 12/08/2010 02:34 PM, Wolfgang Denk wrote:

 
 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

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.

~Deepak
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Honor /memory/reg node in DTB files

[Deepak Saxena]

On 12/07/2010 11:09 AM, Wolfgang Denk wrote:
 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.

I won't argue with you on this front. Having U-Boot determine some board
information from the DT and determine other board information at
run-time is not consistent and confusing.

 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.

Thanks for clarifying that. You had stated pass make U-Boot process
this information and I read that as in passing the information to
the build (make) process.

 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?

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?

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

I completely understand your perspective here and agree with it. DT has
made the kernel easier to maintain across multiple similar platforms so
it makes sense to leverage the same technology in U-Boot.

 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.

Again, I understand this, though I am not 100% sure there is a way to do
this in a completely generic way off the top of my head. There are use
cases where we must rely on U-Boot to scan and determine memory size and
there 

Re: [U-Boot] [PATCH] Honor /memory/reg node in DTB files

[Deepak Saxena]

On 12/07/2010 01:22 PM, Scott Wood wrote:
 On Mon, 6 Dec 2010 16:56:26 -0800
 Deepak Saxena deepak_sax...@mentor.com wrote:
 
 +/*
 + * 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]
 + */
 
 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.

~deepak
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH] Honor /memory/reg node in DTB files

[Deepak Saxena]

On 12/06/2010 10:52 PM, Wolfgang Denk wrote:
 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?

Hi Wolfgang,

Thanks for the response, I have a few different thoughts on the subject.

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
be testing different system layout options as it would require
a rebuild and reflash every time a new configuration is to be tested.
In certain environments, a developer may not be permitted to reflash the 
bootloader on a board shared by others (such as a remote HW lab).

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.

Thanks,
~Deepak
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH] Honor /memory/reg node in DTB files

[Deepak Saxena]

commit 341764495180a712b9aaccfa0479b2ff7e44e35b
Author: Deepak Saxena deepak_sax...@mentor.com
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 deepak_sax...@mentor.com
 Signed-off-by: Hollis Blanchard hollis_blanch...@mentor.com

---

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

diff --git a/arch/powerpc/cpu/mpc85xx/fdt.c b/arch/powerpc/cpu/mpc85xx/fdt.c
index 4540364..6d384e3 100644
--- a/arch/powerpc/cpu/mpc85xx/fdt.c
+++ b/arch/powerpc/cpu/mpc85xx/fdt.c
@@ -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]
+ */
+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);
diff --git a/board/freescale/p1022ds/p1022ds.c 
b/board/freescale/p1022ds/p1022ds.c
index 5cdee9f..7378d88 100644
--- a/board/freescale/p1022ds/p1022ds.c
+++ b/board/freescale/p1022ds/p1022ds.c
@@ -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;

diff --git a/board/freescale/p1_p2_rdb/p1_p2_rdb.c 
b/board/freescale/p1_p2_rdb/p1_p2_rdb.c
index fae31f2..5e4adc6 100644
--- a/board/freescale/p1_p2_rdb/p1_p2_rdb.c
+++ b/board/freescale/p1_p2_rdb/p1_p2_rdb.c
@@ -220,9 +220,10 @@ void ft_board_setup(void *blob, bd_t

[U-Boot] RFC: Not overriding device-tree memory nodes

[Deepak Saxena]

I am playing with loading of multiple Linux images on individual cores 
on a multi core system (P2020RDB for example) and would like to be able 
to provide the memory information for each node via the DTB as opposed 
to using the bootm_low and bootm_size environment variables. The env 
variables work; however, setting the environment variable for each node 
when we're already using the DTB to partition devices on the system 
seems confusing from the perspective of someone who is trying to 
configure and deploy a complex multi-core environment (8, 16, and more 
cores).

Using the DTB's memory node is easily accomplished by adding some code 
to the ft_board_setup() to check if a valid memory/reg property exists 
(valid meaning that it is within the [bd_memstart, bd_memstart + 
bd_memsize] range) and honoring that. The issue that comes up is what to 
do when there is a valid range in the DTB and the user has also set a 
bootm_range. For example the user is doing some quick testing and wants 
to change the amount of memory for a node or two just for one run via 
the variables instead of re-building the DTB. My thought is to 
prioritize the environment variable over the DTB; however calling 
getenv_bootm_low() returns a valid value even if the user has not set so 
there's no way to tell if the user set it or not. I'd like to do one of 
two following to deal with this:

1) Add a parameter to getenv_bootm_low() that returns whether the 
variable is user set or a default value,
getenv_bootm_low(user_set) for example.

2) Just make an assumption in ft_board_setup()  if (bootm_low + 
bootm_size == bd_memstart + bd_memsize),
the user did not set the value and we should honor what is in the DTB.

I'm leaning with option 2 as it is not intrusive to other callers and 
less confusing in my opinion and I'll likely wrap the code to check for 
an existing memory/reg DTB entry in a new ft_bootm_fixup() function that 
can be used by any existing and future boards that implement 
CONFIG_OF_BOARD_SETUP.

Thoughts?
~Deepak



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot