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

login
register
mail settings
Submitter Deepak Saxena
Date Dec. 7, 2010, 12:56 a.m.
Message ID <4CFD863A.7070000@mentor.com>
Download mbox | patch
Permalink /patch/74482/
State Not Applicable
Delegated to: Marek Vasut
Headers show

Comments

Deepak Saxena - Dec. 7, 2010, 12:56 a.m.
commit 341764495180a712b9aaccfa0479b2ff7e44e35b
Author: Deepak Saxena <deepak_saxena@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_saxena@mentor.com>
     Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>

---

See http://lists.denx.de/pipermail/u-boot/2010-December/083057.html
for initial proposal on this.
Wolfgang Denk - Dec. 7, 2010, 6:52 a.m.
Dear Deepak Saxena,

In message <4CFD863A.7070000@mentor.com> you wrote:
> commit 341764495180a712b9aaccfa0479b2ff7e44e35b
> Author: Deepak Saxena <deepak_saxena@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_saxena@mentor.com>
>      Signed-off-by: Hollis Blanchard <hollis_blanchard@mentor.com>

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?

Best regards,

Wolfgang Denk
Deepak Saxena - Dec. 7, 2010, 6:05 p.m.
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
Hollis Blanchard - Dec. 7, 2010, 6:40 p.m.
On 12/06/2010 10:52 PM, Wolfgang Denk wrote:
> 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@0
             memory
                 reg = <0 10000000>
         partition@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?

Hollis Blanchard
Mentor Graphics, Embedded Systems Division
Wolfgang Denk - Dec. 7, 2010, 7:09 p.m.
Dear Deepak,

In message <4CFE775C.6050001@mentor.com> you wrote:
>
> 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.

I agree with you, but I think this is just one part (and eventually a
minor part) of the issue.  I would not protest if you say thatthe whol
(static, compile time) configuration of U-Boot does not scale well for
such requirements.

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.

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

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

Again, there is no conflict between your statement and what I
suggested.

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

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

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

And probaly one it still has to do, as there are good chances that you
don't know the actual memory size in advance - like on systems that
come in several configruations but use a common U-Boot image, or
systems where the user can plug in DIMMs of different size.

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

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

Again, this was NOT what I suggested.

Best regards,

Wolfgang Denk
Wolfgang Denk - Dec. 7, 2010, 7:21 p.m.
Dear Hollis,

In message <4CFE7FA8.2030701@mentor.com> you wrote:
> On 12/06/2010 10:52 PM, Wolfgang Denk wrote:
> > 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:

To be honest - I have not spent thoughts yet how this can be
implemented.  I would expect that there might be some common part, but
there will eventually also be per-core configurations.

>      cpus
>          ...
>      memory
>          reg = <0 20000000>
>      soc
>          ...
>      partitions
>          partition@0
>              memory
>                  reg = <0 10000000>
>          partition@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?

I did not think about that. I did not think about how to boot an OS
and how to provide a DT to these.

My concerns are still on a lower level. Memory initialization is a
very basic task of the boot loader. We are discussiong to move the
description of this resource out of U-Boot (where it was
traditionally statically coinfigured at compile time, with the
exception of auti-sizing). When doing so, we should not let U-Boot
live in a different world, or let it operate on a different set of
information. If the description of a resource is in the DT, then
U-Boot has to use this information (from the DT) for all operations
that deal with this resource.

Just passing such information to the OS, behind U-Boot's back and with
U-Boot having an independt (and probably different) view makes no
sense to me.

There are a number of features (persistent RAM, shared log buffer,
shared video buffers, ...) where U-Boot and Linux need to have exactly
the same understanding about available and usable memory.  I just want
to make sure that future extensions will allow to keep these features
instead of breaking them.


Best regards,

Wolfgang Denk
Scott Wood - Dec. 7, 2010, 9:22 p.m.
On Mon, 6 Dec 2010 16:56:26 -0800
Deepak Saxena <deepak_saxena@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:

	memory {
		device_type = "memory";
		reg = <0 0 0 0>;        // Filled by U-Boot
	};

-Scott
Hollis Blanchard - Dec. 8, 2010, 6:30 p.m.
On 12/07/2010 11:09 AM, Wolfgang Denk wrote:
> 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.

Hollis Blanchard
Mentor Graphics, Embedded Systems Division
Deepak Saxena - Dec. 8, 2010, 6:59 p.m.
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 are use cases where a system is designed and not changeable and
users need to be able to reconfigure system partitioning on the fly. Are
you open to the CONFIG option as a first step towards supporting the
later use case?

Thanks,
~Deepak
Deepak Saxena - Dec. 8, 2010, 6:59 p.m.
On 12/07/2010 01:22 PM, Scott Wood wrote:
> On Mon, 6 Dec 2010 16:56:26 -0800
> Deepak Saxena <deepak_saxena@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
Scott Wood - Dec. 8, 2010, 7:11 p.m.
On Wed, 8 Dec 2010 10:59:44 -0800
Deepak Saxena <deepak_saxena@mentor.com> wrote:

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

Ah, I looked for that and missed it.  But there are also some boards
that have socketed RAM, but for some reason have a real memory size in
the device tree (corresponding to what the board ships with,
probably).  I think this is something that should have to be selectable
by the board config (for image size reasons as well).

Also some nits:

> +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);

const u32 *, and eliminate the cast.

> +	u64 reg_base, reg_size;
> +	void *reg, *dtype;
> +	int len;
> +
> +	if ((nodeoffset = fdt_path_offset(blob, "/memory")) >= 0)
> +	{

Brace on same line as if

Don't put the assignment inside the if statement.

> +		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];
> +			}

This only works on big-endian.

> +			if ((reg_size) &&
> +			    (reg_base >= (u64)bd->bi_memstart) &&
> +			    ((reg_size + reg_base)
> +			     <= ((u64)bd->bi_memstart + (u64)bd->bi_memsize)))
> +				return 1;				
> +		}

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

-Scott
Dan Malek - Dec. 8, 2010, 7:22 p.m.
On Dec 8, 2010, at 11:11 AM, Scott Wood wrote:

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

Thanks.

	-- Dan
Scott Wood - Dec. 8, 2010, 7:33 p.m.
On Wed, 8 Dec 2010 11:22:59 -0800
Dan Malek <ppc6dev@digitaldans.com> wrote:

> 
> On Dec 8, 2010, at 11:11 AM, Scott Wood wrote:
> 
> > 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.

I don't think that's what this is for.  I think it's for AMP scenarios
where you want to carve up memory between guests -- in which case you
want U-Boot to not overwrite the device tree with its own idea of how
much memory is present.

This patch was trying to guess whether that's the case based on whether
the existing value looks reasonable, but I think it has to be an
explicit configuration (board config, environment variable, etc).  Once
that is done, then the test should either go away, or complain -- don't
just silently do something different if it's been told that the memory
node is supposed to be complete and correct.

As for the merits of error checking in general, obviously it's not
going to be complete.  But it can be useful to check for some common
errors, such as when a board has multiple DTSes and the user has to
pick the right one for how U-Boot has been configured.  Maybe
granting an AMP partition a memory region beyond the bounds of detected
memory is another such case.

-Scott
Wolfgang Denk - Dec. 8, 2010, 8:53 p.m.
Dear Hollis Blanchard,

In message <4CFFCEC1.6000103@mentor.com> you wrote:
> On 12/07/2010 11:09 AM, Wolfgang Denk wrote:
> > 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?

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.

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.

Best regards,

Wolfgang Denk
Wolfgang Denk - Dec. 8, 2010, 9 p.m.
Dear Deepak Saxena,

In message <4CFFD57C.1010601@mentor.com> you wrote:
>
> > 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?

See my previous message to Hollis.  If you really want U-Boot to keep
it's fingers off the /memory nodes in the DT, then simply do that. But
then please be consequent, add it for all architectures, and if
enabled, then without magic where U-Boot will sometimes to this and
other times do that.  And provide documentation to the end user.

Best regards,

Wolfgang Denk
Hollis Blanchard - Dec. 8, 2010, 9:08 p.m.
On 12/08/2010 12:53 PM, Wolfgang Denk wrote:
> Dear Hollis Blanchard,
>
> In message<4CFFCEC1.6000103@mentor.com>  you wrote:
>> On 12/07/2010 11:09 AM, Wolfgang Denk wrote:
>>> 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?
> 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.
> 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?
Yes, I do include memory initialization. In our use case, u-boot must 
know about all memory (to configure the memory controller), but each OS 
is only made aware of a piece of it. They really do have different 
information about the 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 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.
> 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.
> 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 agree that all (device tree aware) systems should behave consistently 
in this regard. Of course, in that case, we don't need weak functions?

Documenting the behavior is a very good point.

Hollis Blanchard
Mentor Graphics, Embedded Systems Division
Wolfgang Denk - Dec. 8, 2010, 9:38 p.m.
Dear Hollis,

In message <4CFFF3C4.20800@mentor.com> you wrote:
>
> 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.
> > 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.

OK - but then the user should really be requested to select the
beviour.  This should be done in an explicit and documented way, ant
not based on some magic properties of the DT.

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

I have to admit that I also dislike the bootm_low / bootm_size stuff
as it's really confusing to users, especially as the difference
between bootm_size and CONFIG_SYS_BOOTMAPSZ (and BOOTMAPSZ, which is
sometimes used instead) is not really clear.

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

Best regards,

Wolfgang Denk
Dan Malek - Dec. 8, 2010, 10:08 p.m.
Hi Wolfgang.

On Dec 8, 2010, at 1:38 PM, Wolfgang Denk wrote:

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

I just wanted to make these points, as I did try some alternatives
but never found anything acceptable.  By looking at key values
in the DT, at least it was confined to one place.

This feature is needed, so I propose we let it exist in the
form we have found useful and let it evolve over time as
others gain some experience.

Thanks.

	-- Dan
Wolfgang Denk - Dec. 8, 2010, 10:34 p.m.
Dear Dan,

In message <0DDCBDA1-188F-433D-BDCC-5FDCF709A131@digitaldans.com> you wrote:
> 
> > 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."

> 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

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

to have only the memory node updated.

Best regards,

Wolfgang Denk
Peter Tyser - Dec. 8, 2010, 11:33 p.m.
On Wed, 2010-12-08 at 23:34 +0100, Wolfgang Denk wrote:
> Dear Dan,
> 
> In message <0DDCBDA1-188F-433D-BDCC-5FDCF709A131@digitaldans.com> you wrote:
> > 
> > > 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."
> 
> > 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

I haven't followed the this thread too closely, but I was curious if you
could do manual tweaks by using the 'bootm' and 'fdt' sub-commands.
This could allow more fine grained control of the boot process and let
you manually modify the DTB before booting to Linux.  Eg make the
'bootcmd' environment variable be something like:
bootm start $loadaddr; \
bootm loados; \
bootm ramdisk; \
bootm fdt; \
fdt boardsetup; \
fdt set memory reg "<0 10000000>"; \
bootm prep; \
bootm go

Some dual core Freescale board's do somewhat similar operations for AMP
operation (see doc/README.p2020rdb), although I haven't personally tried
AMP.

Best,
Peter
Dan Malek - Dec. 8, 2010, 11:59 p.m.
On Dec 8, 2010, at 2:34 PM, Wolfgang Denk wrote:

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

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

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

Well, that is pretty cool.

> 	dt_skip=memory,mac-address

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

Thanks.

	-- Dan
Wolfgang Denk - Dec. 9, 2010, 10 a.m.
Dear Dan,

In message <750641C9-DC97-4923-B337-05A2F1BC9676@digitaldans.com> you wrote:
> 
> Yes, I'm sometimes pleased....  :-)

Good :-)

> > My current thinking is to introduce something like  .....
> 
> Well, that is pretty cool.
> 
> > 	dt_skip=memory,mac-address
> 
> Do we have to write a parser now, or is there something
> that currently exists to help out? :-)

We use the same format already to implement the hwconfig feature. It
just needs to be moved to common code.

Best regards,

Wolfgang Denk
Deepak Saxena - Dec. 10, 2010, 6:04 p.m.
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
Wolfgang Denk - Dec. 12, 2010, 9:19 p.m.
Dear Deepak Saxena,

In message <4D026BB2.6020609@mentor.com> you wrote:
> 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.

You just described what the implementation should look like.

Thanks.

Wolfgang Denk
Marek Vasut - March 31, 2012, 7:32 p.m.
Dear Deepak Saxena,

> commit 341764495180a712b9aaccfa0479b2ff7e44e35b
> Author: Deepak Saxena <deepak_saxena@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_saxena@mentor.com>
>      Signed-off-by: Hollis Blanchard <hollis_blanchard@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 *bd)
>   	base = getenv_bootm_low();
>   	size = getenv_bootm_size();
> 
> -	ft_pci_board_setup(blob);
> +	if (base != (phys_addr_t)bd->bi_memstart && size !=
> (phys_addr_t)bd->bi_memsize)
> +		fdt_fixup_memory(blob, (u64)base, (u64)size);
> 
> -	fdt_fixup_memory(blob, (u64)base, (u64)size);
> +	ft_pci_board_setup(blob);
>   }
>   #endif
> 
What was the conclusion on this patch ?

Patch

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 *bd)
  	base = getenv_bootm_low();
  	size = getenv_bootm_size();

-	ft_pci_board_setup(blob);
+	if (base != (phys_addr_t)bd->bi_memstart && size != 
(phys_addr_t)bd->bi_memsize)
+		fdt_fixup_memory(blob, (u64)base, (u64)size);

-	fdt_fixup_memory(blob, (u64)base, (u64)size);
+	ft_pci_board_setup(blob);
  }
  #endif