| Message ID | 1274298809-12772-1-git-send-email-timur@freescale.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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
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.
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.
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.
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.
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
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.
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
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.
On Thu, May 20, 2010 at 1:17 AM, David Gibson <david@gibson.dropbear.id.au> wrote: > 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. Sorry, I guess I don't understand. How do I alter the padding before giving it to U-Boot?
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
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
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(-)