[PATCH] powerpc: make the padding for the device tree a configurable option

[PATCH] powerpc: make the padding for the device tree a configurable option

[Timur Tabi]

Add the DTS_PADDING Kconfig option, which allows users and board defconfig
files to specify a padding value when compiling device trees.

When a device tree source (DTS) is compiled into a binary (DTB), a hard-coded
padding of 1024 bytes is used (i.e. dtc command-line parameter "-p 1024").
Although this has worked so far, it may not be sufficient in the future for
some boards.  Newer versions of U-boot perform more and more fixup on the
device tree, and eventually it will run out.

So to accommodate future boards where more padding is needed, we make the
option for the -p parameter configurable.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 arch/powerpc/boot/Makefile     |    4 ++--
 arch/powerpc/platforms/Kconfig |   13 +++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
index bb2465b..750fa6b 100644
--- a/arch/powerpc/boot/Makefile
+++ b/arch/powerpc/boot/Makefile
@@ -35,7 +35,7 @@ endif
 
 BOOTCFLAGS	+= -I$(obj) -I$(srctree)/$(obj)
 
-DTS_FLAGS	?= -p 1024
+DTS_FLAGS	?= -p ${CONFIG_DTS_PADDING}
 
 $(obj)/4xx.o: BOOTCFLAGS += -mcpu=405
 $(obj)/ebony.o: BOOTCFLAGS += -mcpu=405
@@ -331,7 +331,7 @@ $(obj)/treeImage.%: vmlinux $(obj)/%.dtb $(wrapperbits)
 # Rule to build device tree blobs
 DTC = $(objtree)/scripts/dtc/dtc
 
-$(obj)/%.dtb: $(dtstree)/%.dts
+$(obj)/%.dtb: $(dtstree)/%.dts $(srctree)/.config
 	$(DTC) -O dtb -o $(obj)/$*.dtb -b 0 $(DTS_FLAGS) $(dtstree)/$*.dts
 
 # If there isn't a platform selected then just strip the vmlinux.
diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
index d1663db..2b0a9c3 100644
--- a/arch/powerpc/platforms/Kconfig
+++ b/arch/powerpc/platforms/Kconfig
@@ -41,6 +41,19 @@ config PPC_OF_BOOT_TRAMPOLINE
 
 	  In case of doubt, say Y
 
+config DTS_PADDING
+	int "Padding for the device tree binary"
+	default 1024
+	help
+	  Specify the padding value to be used when compiling a DTS (device
+	  tree source) file into a DTB (device tree binary).  The padding is
+	  used to ensure enough space for any additional nodes and properties
+	  that the boot loader adds during fix-up.  If your boot loader
+	  complains about lack of space during fix-up, try increasing this
+	  value.
+
+	  If unsure, leave this value at the default.
+
 config UDBG_RTAS_CONSOLE
 	bool "RTAS based debug console"
 	depends on PPC_RTAS
-- 
1.6.5

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Benjamin Herrenschmidt]

On Wed, 2010-05-19 at 14:53 -0500, Timur Tabi wrote:
> Add the DTS_PADDING Kconfig option, which allows users and board defconfig
> files to specify a padding value when compiling device trees.
> 
> When a device tree source (DTS) is compiled into a binary (DTB), a hard-coded
> padding of 1024 bytes is used (i.e. dtc command-line parameter "-p 1024").
> Although this has worked so far, it may not be sufficient in the future for
> some boards.  Newer versions of U-boot perform more and more fixup on the
> device tree, and eventually it will run out.
> 
> So to accommodate future boards where more padding is needed, we make the
> option for the -p parameter configurable.

Can't u-boot just allocate more space ?

Ben.

> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
>  arch/powerpc/boot/Makefile     |    4 ++--
>  arch/powerpc/platforms/Kconfig |   13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/powerpc/boot/Makefile b/arch/powerpc/boot/Makefile
> index bb2465b..750fa6b 100644
> --- a/arch/powerpc/boot/Makefile
> +++ b/arch/powerpc/boot/Makefile
> @@ -35,7 +35,7 @@ endif
>  
>  BOOTCFLAGS	+= -I$(obj) -I$(srctree)/$(obj)
>  
> -DTS_FLAGS	?= -p 1024
> +DTS_FLAGS	?= -p ${CONFIG_DTS_PADDING}
>  
>  $(obj)/4xx.o: BOOTCFLAGS += -mcpu=405
>  $(obj)/ebony.o: BOOTCFLAGS += -mcpu=405
> @@ -331,7 +331,7 @@ $(obj)/treeImage.%: vmlinux $(obj)/%.dtb $(wrapperbits)
>  # Rule to build device tree blobs
>  DTC = $(objtree)/scripts/dtc/dtc
>  
> -$(obj)/%.dtb: $(dtstree)/%.dts
> +$(obj)/%.dtb: $(dtstree)/%.dts $(srctree)/.config
>  	$(DTC) -O dtb -o $(obj)/$*.dtb -b 0 $(DTS_FLAGS) $(dtstree)/$*.dts
>  
>  # If there isn't a platform selected then just strip the vmlinux.
> diff --git a/arch/powerpc/platforms/Kconfig b/arch/powerpc/platforms/Kconfig
> index d1663db..2b0a9c3 100644
> --- a/arch/powerpc/platforms/Kconfig
> +++ b/arch/powerpc/platforms/Kconfig
> @@ -41,6 +41,19 @@ config PPC_OF_BOOT_TRAMPOLINE
>  
>  	  In case of doubt, say Y
>  
> +config DTS_PADDING
> +	int "Padding for the device tree binary"
> +	default 1024
> +	help
> +	  Specify the padding value to be used when compiling a DTS (device
> +	  tree source) file into a DTB (device tree binary).  The padding is
> +	  used to ensure enough space for any additional nodes and properties
> +	  that the boot loader adds during fix-up.  If your boot loader
> +	  complains about lack of space during fix-up, try increasing this
> +	  value.
> +
> +	  If unsure, leave this value at the default.
> +
>  config UDBG_RTAS_CONSOLE
>  	bool "RTAS based debug console"
>  	depends on PPC_RTAS

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Timur Tabi]

Benjamin Herrenschmidt wrote:

>> So to accommodate future boards where more padding is needed, we make the
>> option for the -p parameter configurable.
> 
> Can't u-boot just allocate more space ?

Yes and no.  U-Boot has functions to increase the size of an fdt, but these
functions can't be sure that the fdt will grow beyond its allocated space.
So if U-Boot calls fdt_setprop() or fdt_add_subnode(), and there isn't
enough space in the fdt, those functions will return with an error.

The problem with growing the fdt is that the function which does this
(fdt_open_into) cannot guarantee that the fdt will grow too large and
overwrite the end of whatever allocated memory it's in.

I had a long argument with Wolfgang on this (see "libfdt: make
fdt_increase_size() available to everyone"), and he says he'll reject any
patch that can't guarantee that fdt_open_into() won't grow too large.  He'll
also reject any patch that uses a macro constant to reserve this space, even
if I use that constant to ensure that fdt_open_into() won't do anything bad.

So in other words, U-Boot could allocate more space, but Wolfgang won't let it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Benjamin Herrenschmidt]

On Wed, 2010-05-19 at 16:33 -0500, Timur Tabi wrote:
> Benjamin Herrenschmidt wrote:
> 
> >> So to accommodate future boards where more padding is needed, we make the
> >> option for the -p parameter configurable.
> > 
> > Can't u-boot just allocate more space ?
> 
> Yes and no.  U-Boot has functions to increase the size of an fdt, but these
> functions can't be sure that the fdt will grow beyond its allocated space.
> So if U-Boot calls fdt_setprop() or fdt_add_subnode(), and there isn't
> enough space in the fdt, those functions will return with an error.

 .../...

It's still not kernel business to have to deal with u-boot memory
allocation constraints. The padding in the kernel built is intended to
make space for DT changes done by the zImage wrapper.

Maybe we could add to libfdt a way to provide a realloc() callback to it
when it hits the max size, and uboot can then move things around (or
fail).

Ben.

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Timur Tabi]

On Wed, May 19, 2010 at 5:44 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:

> It's still not kernel business to have to deal with u-boot memory
> allocation constraints.

I agree, but it still makes sense to me to allow the padding to be configurable.

> The padding in the kernel built is intended to
> make space for DT changes done by the zImage wrapper.

Well, okay.  I think it would be nice if we expanded that to handle
general usage.

> Maybe we could add to libfdt a way to provide a realloc() callback to it
> when it hits the max size, and uboot can then move things around (or
> fail).

The problem is that the code which allocates a block for the fdt is
completely distinct from the code that manipulates the fdt.  We'd need
to put in either some kind of funky callback mechanism, or insist that
every fdt exist in a block of memory allocated by some specific method
(e.g. lmb).

I'm stuck between a rock and a hard place, it seems.  No one is
willing to compromise on any of my ideas.  It's hard to convince our
BSP developers that they should be pushing more code upstream when I
get so much resistance for a such a mundane change.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Benjamin Herrenschmidt]

On Wed, 2010-05-19 at 19:03 -0500, Timur Tabi wrote:
> The problem is that the code which allocates a block for the fdt is
> completely distinct from the code that manipulates the fdt.  We'd need
> to put in either some kind of funky callback mechanism, or insist that
> every fdt exist in a block of memory allocated by some specific method
> (e.g. lmb).
> 
> I'm stuck between a rock and a hard place, it seems.  No one is
> willing to compromise on any of my ideas.  It's hard to convince our
> BSP developers that they should be pushing more code upstream when I
> get so much resistance for a such a mundane change. 

I don't see why we couldn't add a callback to libfdt for allocation /
reallocation.

Cheers,
Ben.

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[M. Warner Losh]

In message: <AANLkTilefxXcMnWqKSultu88r4D9W98adHLHxvUwi113@mail.gmail.com>
            Timur Tabi <timur@freescale.com> writes:
: I'm stuck between a rock and a hard place, it seems.  No one is
: willing to compromise on any of my ideas.  It's hard to convince our
: BSP developers that they should be pushing more code upstream when I
: get so much resistance for a such a mundane change.

http://www.bikeshed.org/ seems appropriate here.

Warner

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[David Gibson]

On Wed, May 19, 2010 at 07:03:17PM -0500, Timur Tabi wrote:
> On Wed, May 19, 2010 at 5:44 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> 
> > It's still not kernel business to have to deal with u-boot memory
> > allocation constraints.
> 
> I agree, but it still makes sense to me to allow the padding to be configurable.
> 
> > The padding in the kernel built is intended to
> > make space for DT changes done by the zImage wrapper.
> 
> Well, okay.  I think it would be nice if we expanded that to handle
> general usage.
> 
> > Maybe we could add to libfdt a way to provide a realloc() callback to it
> > when it hits the max size, and uboot can then move things around (or
> > fail).
> 
> The problem is that the code which allocates a block for the fdt is
> completely distinct from the code that manipulates the fdt.  We'd need
> to put in either some kind of funky callback mechanism, or insist that
> every fdt exist in a block of memory allocated by some specific method
> (e.g. lmb).
> 
> I'm stuck between a rock and a hard place, it seems.  No one is
> willing to compromise on any of my ideas.  It's hard to convince our
> BSP developers that they should be pushing more code upstream when I
> get so much resistance for a such a mundane change.

Couldn't you use the configurable padding, but put the stuff to do it
into u-boot.  i.e. repad the dtb at u-boot build time, rather than
u-boot runtime.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Timur Tabi]

On Wed, May 19, 2010 at 8:18 PM, David Gibson
<david@gibson.dropbear.id.au> wrote:

> Couldn't you use the configurable padding, but put the stuff to do it
> into u-boot. =A0i.e. repad the dtb at u-boot build time, rather than
> u-boot runtime.

That's what I was trying to do.   Take a look at this thread:

http://lists.denx.de/pipermail/u-boot/2010-May/071520.html

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[David Gibson]

On Wed, May 19, 2010 at 08:46:19PM -0500, Timur Tabi wrote:
> On Wed, May 19, 2010 at 8:18 PM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> 
> > Couldn't you use the configurable padding, but put the stuff to do it
> > into u-boot.  i.e. repad the dtb at u-boot build time, rather than
> > u-boot runtime.
> 
> That's what I was trying to do.   Take a look at this thread:
> 
> http://lists.denx.de/pipermail/u-boot/2010-May/071520.html

Um.. what?  As far as I can tell that thread is about runtime
expanding the space given to the dtb.  I'm talking about altering the
padding on the file before giving it to u-boot in the first place.


-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Timur Tabi]

On Thu, May 20, 2010 at 1:17 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:

> Um.. what? =A0As far as I can tell that thread is about runtime
> expanding the space given to the dtb. =A0I'm talking about altering the
> padding on the file before giving it to u-boot in the first place.

Sorry, I guess I don't understand.  How do I alter the padding before
giving it to U-Boot?

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] powerpc: make the padding for the device tree a configurable option

[Scott Wood]

On 05/19/2010 08:18 PM, David Gibson wrote:
> On Wed, May 19, 2010 at 07:03:17PM -0500, Timur Tabi wrote:
>> On Wed, May 19, 2010 at 5:44 PM, Benjamin Herrenschmidt
>>> The padding in the kernel built is intended to
>>> make space for DT changes done by the zImage wrapper.
>>
>> Well, okay.  I think it would be nice if we expanded that to handle
>> general usage.

Don't we have dynamic expansion in the zImage wrapper?

>>> Maybe we could add to libfdt a way to provide a realloc() callback to it
>>> when it hits the max size, and uboot can then move things around (or
>>> fail).
>>
>> The problem is that the code which allocates a block for the fdt is
>> completely distinct from the code that manipulates the fdt.  We'd need
>> to put in either some kind of funky callback mechanism, or insist that
>> every fdt exist in a block of memory allocated by some specific method
>> (e.g. lmb).
>>
>> I'm stuck between a rock and a hard place, it seems.  No one is
>> willing to compromise on any of my ideas.  It's hard to convince our
>> BSP developers that they should be pushing more code upstream when I
>> get so much resistance for a such a mundane change.
>
> Couldn't you use the configurable padding, but put the stuff to do it
> into u-boot.  i.e. repad the dtb at u-boot build time, rather than
> u-boot runtime.

Currently generating a dtb is not done as part of the u-boot build 
process -- and apparently some people are using the Linux makefile 
target to do it.

I think the right thing is to do whatever we need to do in u-boot to get 
dynamic expansion fully working (it already does it in some 
circumstances, just not always, or necessarily safely).  Static padding 
is too fragile.

-Scott