[PATCH v2 0/9] cmd: fdt: Add device tree overlays support

[PATCH v2 0/9] cmd: fdt: Add device tree overlays support

[Maxime Ripard]

Hi,

The device tree overlays are a great solution to the issue raised by
the bunch expandable boards we find everywhere these days, like the
Beaglebone, Raspberry Pi or CHIP.

However, most of the time, the overlays are applied through a
mechanism involving the firmware request interface in Linux, that is
only fully functional once the userspace has been mounted and is
running.

Some expansion boards might need to be enabled before that, because
they simply need to patch the DT early on, or need to be initialized
early in order to be fully functional, or because they provide access
to the root filesystem.

In these cases, having the bootloader applying the overlay before
Linux starts seems like the easiest solution.

This implementation doesn't provide all the Linux fancyness though,
there's no transactional application, which means that if the overlay
cannot be applied for a reason while you're still halfway through the
application, you're probably screwed. It also cannot remove an
overlay, but I don't think that it is currently a use-case.

The libfdt patches has been sent upstream earlier today for review.

Let me know what you think,
Maxime

Changes from v1:
  - Moved the overlay code to libfdt
  - Added unit tests
  - Refactored the code to reduce the amount of memory allocation
  - No longer modify the overlay itself, but create a copy to operate
    on instead.
  - Removed the limitations on the fixups path, names and properties
    length
  - Fixed a few things here and there according to comments

Maxime Ripard (9):
  cmd: fdt: Narrow the check for fdt addr
  scripts: Makefile.lib: Sanitize DTB names
  vsprintf: Include stdarg for va_list
  libfdt: Add new headers and defines
  libfdt: Add iterator over properties
  libfdt: Add max phandle retrieval function
  libfdt: Add overlay application function
  cmd: fdt: add fdt overlay application subcommand
  tests: Introduce DT overlay tests

 Makefile                          |   1 +
 cmd/fdt.c                         |  22 +-
 include/libfdt.h                  |  67 ++++++
 include/libfdt_env.h              |   7 +
 include/test/overlay.h            |  16 ++
 include/test/suites.h             |   1 +
 include/vsprintf.h                |   2 +
 lib/libfdt/Makefile               |   2 +-
 lib/libfdt/fdt_overlay.c          | 414 ++++++++++++++++++++++++++++++++++++++
 lib/libfdt/fdt_ro.c               |  15 ++
 scripts/Makefile.lib              |   8 +-
 test/Kconfig                      |   1 +
 test/cmd_ut.c                     |   6 +
 test/overlay/Kconfig              |  10 +
 test/overlay/Makefile             |  15 ++
 test/overlay/cmd_ut_overlay.c     | 176 ++++++++++++++++
 test/overlay/test-fdt-base.dts    |  17 ++
 test/overlay/test-fdt-overlay.dts |  60 ++++++
 18 files changed, 834 insertions(+), 6 deletions(-)
 create mode 100644 include/test/overlay.h
 create mode 100644 lib/libfdt/fdt_overlay.c
 create mode 100644 test/overlay/Kconfig
 create mode 100644 test/overlay/Makefile
 create mode 100644 test/overlay/cmd_ut_overlay.c
 create mode 100644 test/overlay/test-fdt-base.dts
 create mode 100644 test/overlay/test-fdt-overlay.dts

-- 
2.8.2

[PATCH v2 1/9] cmd: fdt: Narrow the check for fdt addr

[Maxime Ripard]

The current code only checks if the fdt subcommand is fdt addr by checking
whether it starts with 'a'.

Since this is a pretty widely used letter, narrow down that check a bit.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 cmd/fdt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 898217ffe5f8..0f5923e75a41 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -87,7 +87,7 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	/*
 	 * Set the address of the fdt
 	 */
-	if (argv[1][0] == 'a') {
+	if (strncmp(argv[1], "ad", 2) == 0) {
 		unsigned long addr;
 		int control = 0;
 		struct fdt_header *blob;
-- 
2.8.2

Re: [PATCH v2 1/9] cmd: fdt: Narrow the check for fdt addr

[Simon Glass]

On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The current code only checks if the fdt subcommand is fdt addr by checking
> whether it starts with 'a'.
>
> Since this is a pretty widely used letter, narrow down that check a bit.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  cmd/fdt.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 1/9] cmd: fdt: Narrow the check for fdt addr

[Pantelis Antoniou]

> On Jun 10, 2016, at 03:34 , Simon Glass <sjg@chromium.org> wrote:
> 
> On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> The current code only checks if the fdt subcommand is fdt addr by checking
>> whether it starts with 'a'.
>> 
>> Since this is a pretty widely used letter, narrow down that check a bit.
>> 
>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> ---
>> cmd/fdt.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Acked-by: Simon Glass <sjg@chromium.org>

Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

[PATCH v2 2/9] scripts: Makefile.lib: Sanitize DTB names

[Maxime Ripard]

Having dashes as a separator in the DTB name is a quite common practice.

However, the current code to generate objects from DTBs assumes the
separator is an underscore, leading to a compilation error when building a
device tree with dashes.

Replace all the dashes in the DTB name to generate the symbols name, which
should solve this issue.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 scripts/Makefile.lib | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
index ad1d9b5d7dd2..df1b5a7b4150 100644
--- a/scripts/Makefile.lib
+++ b/scripts/Makefile.lib
@@ -275,11 +275,11 @@ cmd_dt_S_dtb=						\
 (							\
 	echo '.section .dtb.init.rodata,"a"';		\
 	echo '.balign 16';				\
-	echo '.global __dtb_$(*F)_begin';		\
-	echo '__dtb_$(*F)_begin:';			\
+	echo '.global __dtb_$(subst -,_,$(*F))_begin';	\
+	echo '__dtb_$(subst -,_,$(*F))_begin:';		\
 	echo '.incbin "$<" ';				\
-	echo '__dtb_$(*F)_end:';			\
-	echo '.global __dtb_$(*F)_end';			\
+	echo '__dtb_$(subst -,_,$(*F))_end:';		\
+	echo '.global __dtb_$(subst -,_,$(*F))_end';	\
 	echo '.balign 16';				\
 ) > $@
 
-- 
2.8.2

Re: [PATCH v2 2/9] scripts: Makefile.lib: Sanitize DTB names

[Simon Glass]

On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> Having dashes as a separator in the DTB name is a quite common practice.
>
> However, the current code to generate objects from DTBs assumes the
> separator is an underscore, leading to a compilation error when building a
> device tree with dashes.
>
> Replace all the dashes in the DTB name to generate the symbols name, which
> should solve this issue.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  scripts/Makefile.lib | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 2/9] scripts: Makefile.lib: Sanitize DTB names

[Pantelis Antoniou]

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> Having dashes as a separator in the DTB name is a quite common practice.
> 
> However, the current code to generate objects from DTBs assumes the
> separator is an underscore, leading to a compilation error when building a
> device tree with dashes.
> 
> Replace all the dashes in the DTB name to generate the symbols name, which
> should solve this issue.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> scripts/Makefile.lib | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
> index ad1d9b5d7dd2..df1b5a7b4150 100644
> --- a/scripts/Makefile.lib
> +++ b/scripts/Makefile.lib
> @@ -275,11 +275,11 @@ cmd_dt_S_dtb=						\
> (							\
> 	echo '.section .dtb.init.rodata,"a"';		\
> 	echo '.balign 16';				\
> -	echo '.global __dtb_$(*F)_begin';		\
> -	echo '__dtb_$(*F)_begin:';			\
> +	echo '.global __dtb_$(subst -,_,$(*F))_begin';	\
> +	echo '__dtb_$(subst -,_,$(*F))_begin:';		\
> 	echo '.incbin "$<" ';				\
> -	echo '__dtb_$(*F)_end:';			\
> -	echo '.global __dtb_$(*F)_end';			\
> +	echo '__dtb_$(subst -,_,$(*F))_end:';		\
> +	echo '.global __dtb_$(subst -,_,$(*F))_end';	\
> 	echo '.balign 16';				\
> ) > $@
> 
> -- 
> 2.8.2
> 

Acked-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

[PATCH v2 3/9] vsprintf: Include stdarg for va_list

[Maxime Ripard]

vsprintf.h doesn't include the stdarg.h file, which means that it relies on
the files that include vsprintf.h to include stdarg.h as well.

Add an explicit include to avoid build errors when simply including that
file.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 include/vsprintf.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/include/vsprintf.h b/include/vsprintf.h
index 376f5dd32499..60e91d119eee 100644
--- a/include/vsprintf.h
+++ b/include/vsprintf.h
@@ -8,6 +8,8 @@
 #ifndef __VSPRINTF_H
 #define __VSPRINTF_H
 
+#include <stdarg.h>
+
 ulong simple_strtoul(const char *cp, char **endp, unsigned int base);
 
 /**
-- 
2.8.2

Re: [PATCH v2 3/9] vsprintf: Include stdarg for va_list

[Simon Glass]

On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> vsprintf.h doesn't include the stdarg.h file, which means that it relies on
> the files that include vsprintf.h to include stdarg.h as well.
>
> Add an explicit include to avoid build errors when simply including that
> file.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  include/vsprintf.h | 2 ++
>  1 file changed, 2 insertions(+)

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 3/9] vsprintf: Include stdarg for va_list

[Pantelis Antoniou]

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> vsprintf.h doesn't include the stdarg.h file, which means that it relies on
> the files that include vsprintf.h to include stdarg.h as well.
> 
> Add an explicit include to avoid build errors when simply including that
> file.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> include/vsprintf.h | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/include/vsprintf.h b/include/vsprintf.h
> index 376f5dd32499..60e91d119eee 100644
> --- a/include/vsprintf.h
> +++ b/include/vsprintf.h
> @@ -8,6 +8,8 @@
> #ifndef __VSPRINTF_H
> #define __VSPRINTF_H
> 
> +#include <stdarg.h>
> +
> ulong simple_strtoul(const char *cp, char **endp, unsigned int base);
> 
> /**
> -- 
> 2.8.2
> 

Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

[PATCH v2 4/9] libfdt: Add new headers and defines

[Maxime Ripard]

The libfdt overlay support introduces a bunch of new includes and
functions.

Make sure we are able to build it by adding the needed glue.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 include/libfdt_env.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/include/libfdt_env.h b/include/libfdt_env.h
index 273b5d30f867..2d2196031332 100644
--- a/include/libfdt_env.h
+++ b/include/libfdt_env.h
@@ -23,6 +23,13 @@ typedef __be64 fdt64_t;
 #define fdt64_to_cpu(x)		be64_to_cpu(x)
 #define cpu_to_fdt64(x)		cpu_to_be64(x)
 
+#ifdef __UBOOT__
+#include "malloc.h"
+#include "vsprintf.h"
+
+#define strtol(cp, endp, base)	simple_strtol(cp, endp, base)
+#endif
+
 /* adding a ramdisk needs 0x44 bytes in version 2008.10 */
 #define FDT_RAMDISK_OVERHEAD	0x80
 
-- 
2.8.2

Re: [PATCH v2 4/9] libfdt: Add new headers and defines

[Simon Glass]

Hi Maxime,

On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The libfdt overlay support introduces a bunch of new includes and
> functions.
>
> Make sure we are able to build it by adding the needed glue.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  include/libfdt_env.h | 7 +++++++
>  1 file changed, 7 insertions(+)
>
> diff --git a/include/libfdt_env.h b/include/libfdt_env.h
> index 273b5d30f867..2d2196031332 100644
> --- a/include/libfdt_env.h
> +++ b/include/libfdt_env.h
> @@ -23,6 +23,13 @@ typedef __be64 fdt64_t;
>  #define fdt64_to_cpu(x)                be64_to_cpu(x)
>  #define cpu_to_fdt64(x)                cpu_to_be64(x)
>
> +#ifdef __UBOOT__
> +#include "malloc.h"
> +#include "vsprintf.h"

Can we use <malloc.h> and <vsprintf.h> instead?

> +
> +#define strtol(cp, endp, base) simple_strtol(cp, endp, base)
> +#endif
> +
>  /* adding a ramdisk needs 0x44 bytes in version 2008.10 */
>  #define FDT_RAMDISK_OVERHEAD   0x80
>
> --
> 2.8.2
>

Regards,
Simon

Re: [PATCH v2 4/9] libfdt: Add new headers and defines

[Pantelis Antoniou]

Hi Maxime,

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> The libfdt overlay support introduces a bunch of new includes and
> functions.
> 
> Make sure we are able to build it by adding the needed glue.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> include/libfdt_env.h | 7 +++++++
> 1 file changed, 7 insertions(+)
> 
> diff --git a/include/libfdt_env.h b/include/libfdt_env.h
> index 273b5d30f867..2d2196031332 100644
> --- a/include/libfdt_env.h
> +++ b/include/libfdt_env.h
> @@ -23,6 +23,13 @@ typedef __be64 fdt64_t;
> #define fdt64_to_cpu(x)		be64_to_cpu(x)
> #define cpu_to_fdt64(x)		cpu_to_be64(x)
> 
> +#ifdef __UBOOT__
> +#include "malloc.h"
> +#include "vsprintf.h"
> +
> +#define strtol(cp, endp, base)	simple_strtol(cp, endp, base)
> +#endif
> +
> /* adding a ramdisk needs 0x44 bytes in version 2008.10 */
> #define FDT_RAMDISK_OVERHEAD	0x80
> 

We need to figure out what the upstream libfdt/dtc maintainer’s take is on this is.
For u-boot we’re fine and for now it’s OK.

Acked-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

> -- 
> 2.8.2
> 

Re: [PATCH v2 4/9] libfdt: Add new headers and defines

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 1691 bytes --]

On Fri, Jun 10, 2016 at 05:03:36PM +0300, Pantelis Antoniou wrote:
> Hi Maxime,
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > The libfdt overlay support introduces a bunch of new includes and
> > functions.
> > 
> > Make sure we are able to build it by adding the needed glue.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > include/libfdt_env.h | 7 +++++++
> > 1 file changed, 7 insertions(+)
> > 
> > diff --git a/include/libfdt_env.h b/include/libfdt_env.h
> > index 273b5d30f867..2d2196031332 100644
> > --- a/include/libfdt_env.h
> > +++ b/include/libfdt_env.h
> > @@ -23,6 +23,13 @@ typedef __be64 fdt64_t;
> > #define fdt64_to_cpu(x)		be64_to_cpu(x)
> > #define cpu_to_fdt64(x)		cpu_to_be64(x)
> > 
> > +#ifdef __UBOOT__
> > +#include "malloc.h"
> > +#include "vsprintf.h"
> > +
> > +#define strtol(cp, endp, base)	simple_strtol(cp, endp, base)
> > +#endif
> > +
> > /* adding a ramdisk needs 0x44 bytes in version 2008.10 */
> > #define FDT_RAMDISK_OVERHEAD	0x80
> > 
> 
> We need to figure out what the upstream libfdt/dtc maintainer’s take is on this is.
> For u-boot we’re fine and for now it’s OK.

These were sent to the upstream dtc list as well.

The concept is fine, but there are a number of problems in the
implementation.  I sent detailed review comments on the upstream
versions, haven't seen a respin yet.
-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 4/9] libfdt: Add new headers and defines

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1926 bytes --]

Hi David,

On Sat, Jun 11, 2016 at 08:30:35PM +1000, David Gibson wrote:
> On Fri, Jun 10, 2016 at 05:03:36PM +0300, Pantelis Antoniou wrote:
> > Hi Maxime,
> > 
> > > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > > 
> > > The libfdt overlay support introduces a bunch of new includes and
> > > functions.
> > > 
> > > Make sure we are able to build it by adding the needed glue.
> > > 
> > > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > > ---
> > > include/libfdt_env.h | 7 +++++++
> > > 1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/include/libfdt_env.h b/include/libfdt_env.h
> > > index 273b5d30f867..2d2196031332 100644
> > > --- a/include/libfdt_env.h
> > > +++ b/include/libfdt_env.h
> > > @@ -23,6 +23,13 @@ typedef __be64 fdt64_t;
> > > #define fdt64_to_cpu(x)		be64_to_cpu(x)
> > > #define cpu_to_fdt64(x)		cpu_to_be64(x)
> > > 
> > > +#ifdef __UBOOT__
> > > +#include "malloc.h"
> > > +#include "vsprintf.h"
> > > +
> > > +#define strtol(cp, endp, base)	simple_strtol(cp, endp, base)
> > > +#endif
> > > +
> > > /* adding a ramdisk needs 0x44 bytes in version 2008.10 */
> > > #define FDT_RAMDISK_OVERHEAD	0x80
> > > 
> > 
> > We need to figure out what the upstream libfdt/dtc maintainer’s take is on this is.
> > For u-boot we’re fine and for now it’s OK.
> 
> These were sent to the upstream dtc list as well.
> 
> The concept is fine, but there are a number of problems in the
> implementation.  I sent detailed review comments on the upstream
> versions, haven't seen a respin yet.

Yes, thanks a lot for your comments, I'll address them and resend a
new serie.

(it might not be before a couple of weeks though).

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 4/9] libfdt: Add new headers and defines

[Simon Glass]

Hi Maxime,

On 13 June 2016 at 03:28, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi David,
>
> On Sat, Jun 11, 2016 at 08:30:35PM +1000, David Gibson wrote:
>> On Fri, Jun 10, 2016 at 05:03:36PM +0300, Pantelis Antoniou wrote:
>> > Hi Maxime,
>> >
>> > > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>> > >
>> > > The libfdt overlay support introduces a bunch of new includes and
>> > > functions.
>> > >
>> > > Make sure we are able to build it by adding the needed glue.
>> > >
>> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > > ---
>> > > include/libfdt_env.h | 7 +++++++
>> > > 1 file changed, 7 insertions(+)
>> > >
>> > > diff --git a/include/libfdt_env.h b/include/libfdt_env.h
>> > > index 273b5d30f867..2d2196031332 100644
>> > > --- a/include/libfdt_env.h
>> > > +++ b/include/libfdt_env.h
>> > > @@ -23,6 +23,13 @@ typedef __be64 fdt64_t;
>> > > #define fdt64_to_cpu(x)           be64_to_cpu(x)
>> > > #define cpu_to_fdt64(x)           cpu_to_be64(x)
>> > >
>> > > +#ifdef __UBOOT__
>> > > +#include "malloc.h"
>> > > +#include "vsprintf.h"
>> > > +
>> > > +#define strtol(cp, endp, base)   simple_strtol(cp, endp, base)
>> > > +#endif
>> > > +
>> > > /* adding a ramdisk needs 0x44 bytes in version 2008.10 */
>> > > #define FDT_RAMDISK_OVERHEAD      0x80
>> > >
>> >
>> > We need to figure out what the upstream libfdt/dtc maintainer’s take is on this is.
>> > For u-boot we’re fine and for now it’s OK.
>>
>> These were sent to the upstream dtc list as well.
>>
>> The concept is fine, but there are a number of problems in the
>> implementation.  I sent detailed review comments on the upstream
>> versions, haven't seen a respin yet.
>
> Yes, thanks a lot for your comments, I'll address them and resend a
> new serie.
>
> (it might not be before a couple of weeks though).

For U-Boot, shall we target the next merge window? Or if you would
prefer to get this in sooner, you could send an interim version with
the ntis address, and we can do a fix-up patch for U-Boot once it is
applied upstream.

>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Regards,
Simon
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[PATCH v2 5/9] libfdt: Add iterator over properties

[Maxime Ripard]

Implement a macro based on fdt_first_property_offset and
fdt_next_property_offset that provides a convenience to iterate over all
the properties of a given node.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 include/libfdt.h | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/include/libfdt.h b/include/libfdt.h
index 74b1d149c2dd..4e8eb9ede3a4 100644
--- a/include/libfdt.h
+++ b/include/libfdt.h
@@ -441,6 +441,30 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset);
 int fdt_next_property_offset(const void *fdt, int offset);
 
 /**
+ * fdt_for_each_property - iterate over all properties of a node
+ * @fdt:	FDT blob (const void *)
+ * @node:	node offset (int)
+ * @property:	property offset (int)
+ *
+ * This is actually a wrapper around a for loop and would be used like so:
+ *
+ *	fdt_for_each_property(fdt, node, property) {
+ *		...
+ *		use property
+ *		...
+ *	}
+ *
+ * Note that this is implemented as a macro and property is used as
+ * iterator in the loop. It should therefore be a locally allocated
+ * variable. The node variable on the other hand is never modified, so
+ * it can be constant or even a literal.
+ */
+#define fdt_for_each_property(fdt, node, property)		\
+	for (property = fdt_first_property_offset(fdt, node);	\
+	     property >= 0;					\
+	     property = fdt_next_property_offset(fdt, property))
+
+/**
  * fdt_get_property_by_offset - retrieve the property at a given offset
  * @fdt: pointer to the device tree blob
  * @offset: offset of the property to retrieve
-- 
2.8.2

Re: [PATCH v2 5/9] libfdt: Add iterator over properties

[Stefan Agner]

On 2016-05-27 02:13, Maxime Ripard wrote:
> Implement a macro based on fdt_first_property_offset and
> fdt_next_property_offset that provides a convenience to iterate over all
> the properties of a given node.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  include/libfdt.h | 24 ++++++++++++++++++++++++
>  1 file changed, 24 insertions(+)

Reviewed-by: Stefan Agner <stefan@agner.ch>

There would be already be several opportunities in existing code to use
that :-)

> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 74b1d149c2dd..4e8eb9ede3a4 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -441,6 +441,30 @@ int fdt_first_property_offset(const void *fdt,
> int nodeoffset);
>  int fdt_next_property_offset(const void *fdt, int offset);
>  
>  /**
> + * fdt_for_each_property - iterate over all properties of a node
> + * @fdt:	FDT blob (const void *)
> + * @node:	node offset (int)
> + * @property:	property offset (int)
> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_property(fdt, node, property) {
> + *		...
> + *		use property
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and property is used as
> + * iterator in the loop. It should therefore be a locally allocated
> + * variable. The node variable on the other hand is never modified, so
> + * it can be constant or even a literal.
> + */
> +#define fdt_for_each_property(fdt, node, property)		\
> +	for (property = fdt_first_property_offset(fdt, node);	\
> +	     property >= 0;					\
> +	     property = fdt_next_property_offset(fdt, property))
> +
> +/**
>   * fdt_get_property_by_offset - retrieve the property at a given offset
>   * @fdt: pointer to the device tree blob
>   * @offset: offset of the property to retrieve

Re: [PATCH v2 5/9] libfdt: Add iterator over properties

[Pantelis Antoniou]

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> Implement a macro based on fdt_first_property_offset and
> fdt_next_property_offset that provides a convenience to iterate over all
> the properties of a given node.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
> include/libfdt.h | 24 ++++++++++++++++++++++++
> 1 file changed, 24 insertions(+)
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 74b1d149c2dd..4e8eb9ede3a4 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -441,6 +441,30 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset);
> int fdt_next_property_offset(const void *fdt, int offset);
> 
> /**
> + * fdt_for_each_property - iterate over all properties of a node
> + * @fdt:	FDT blob (const void *)
> + * @node:	node offset (int)
> + * @property:	property offset (int)
> + *
> + * This is actually a wrapper around a for loop and would be used like so:
> + *
> + *	fdt_for_each_property(fdt, node, property) {
> + *		...
> + *		use property
> + *		...
> + *	}
> + *
> + * Note that this is implemented as a macro and property is used as
> + * iterator in the loop. It should therefore be a locally allocated
> + * variable. The node variable on the other hand is never modified, so
> + * it can be constant or even a literal.
> + */
> +#define fdt_for_each_property(fdt, node, property)		\
> +	for (property = fdt_first_property_offset(fdt, node);	\
> +	     property >= 0;					\
> +	     property = fdt_next_property_offset(fdt, property))
> +
> +/**
>  * fdt_get_property_by_offset - retrieve the property at a given offset
>  * @fdt: pointer to the device tree blob
>  * @offset: offset of the property to retrieve
> -- 
> 2.8.2
> 

Acked-by: Pantelis Antoniou <pantelis.antoniou@konsulko.com>

I’d like to see this merged in dtc upstream please too.

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

Re: [PATCH v2 5/9] libfdt: Add iterator over properties

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2404 bytes --]

Hi Pantelis,

On Fri, Jun 10, 2016 at 05:04:45PM +0300, Pantelis Antoniou wrote:
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > Implement a macro based on fdt_first_property_offset and
> > fdt_next_property_offset that provides a convenience to iterate over all
> > the properties of a given node.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > include/libfdt.h | 24 ++++++++++++++++++++++++
> > 1 file changed, 24 insertions(+)
> > 
> > diff --git a/include/libfdt.h b/include/libfdt.h
> > index 74b1d149c2dd..4e8eb9ede3a4 100644
> > --- a/include/libfdt.h
> > +++ b/include/libfdt.h
> > @@ -441,6 +441,30 @@ int fdt_first_property_offset(const void *fdt, int nodeoffset);
> > int fdt_next_property_offset(const void *fdt, int offset);
> > 
> > /**
> > + * fdt_for_each_property - iterate over all properties of a node
> > + * @fdt:	FDT blob (const void *)
> > + * @node:	node offset (int)
> > + * @property:	property offset (int)
> > + *
> > + * This is actually a wrapper around a for loop and would be used like so:
> > + *
> > + *	fdt_for_each_property(fdt, node, property) {
> > + *		...
> > + *		use property
> > + *		...
> > + *	}
> > + *
> > + * Note that this is implemented as a macro and property is used as
> > + * iterator in the loop. It should therefore be a locally allocated
> > + * variable. The node variable on the other hand is never modified, so
> > + * it can be constant or even a literal.
> > + */
> > +#define fdt_for_each_property(fdt, node, property)		\
> > +	for (property = fdt_first_property_offset(fdt, node);	\
> > +	     property >= 0;					\
> > +	     property = fdt_next_property_offset(fdt, property))
> > +
> > +/**
> >  * fdt_get_property_by_offset - retrieve the property at a given offset
> >  * @fdt: pointer to the device tree blob
> >  * @offset: offset of the property to retrieve
> > -- 
> > 2.8.2
> > 
> 
> Acked-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> 
> I’d like to see this merged in dtc upstream please too.

This has been sent. David made a couple of comments, I'll address them
and respin.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v2 6/9] libfdt: Add max phandle retrieval function

[Maxime Ripard]

Add a function to retrieve the highest phandle in a given device tree.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 include/libfdt.h    | 13 +++++++++++++
 lib/libfdt/fdt_ro.c | 15 +++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/include/libfdt.h b/include/libfdt.h
index 4e8eb9ede3a4..1e01b2c7ebdf 100644
--- a/include/libfdt.h
+++ b/include/libfdt.h
@@ -284,6 +284,19 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
 const char *fdt_string(const void *fdt, int stroffset);
 
 /**
+ * fdt_get_max_phandle - retrieves the highest phandle in a tree
+ * @fdt: pointer to the device tree blob
+ *
+ * fdt_get_max_phandle retrieves the highest phandlle in the given
+ * device tree
+ *
+ * returns:
+ *      the highest phandle on success
+ *      0, if an error occured
+ */
+uint32_t fdt_get_max_phandle(const void *fdt);
+
+/**
  * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
  * @fdt: pointer to the device tree blob
  *
diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
index 7b0777b67eb3..9130ce718965 100644
--- a/lib/libfdt/fdt_ro.c
+++ b/lib/libfdt/fdt_ro.c
@@ -47,6 +47,21 @@ static int _fdt_string_eq(const void *fdt, int stroffset,
 	return (strnlen(p, len + 1) == len) && (memcmp(p, s, len) == 0);
 }
 
+uint32_t fdt_get_max_phandle(const void *fdt)
+{
+	uint32_t max_phandle = 0, phandle;
+	int offset;
+
+	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
+	     offset = fdt_next_node(fdt, offset, NULL)) {
+		phandle = fdt_get_phandle(fdt, offset);
+		if (phandle > max_phandle)
+			max_phandle = phandle;
+	}
+
+	return max_phandle;
+}
+
 int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
 {
 	FDT_CHECK_HEADER(fdt);
-- 
2.8.2

Re: [PATCH v2 6/9] libfdt: Add max phandle retrieval function

[Stefan Agner]

On 2016-05-27 02:13, Maxime Ripard wrote:
> Add a function to retrieve the highest phandle in a given device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  include/libfdt.h    | 13 +++++++++++++
>  lib/libfdt/fdt_ro.c | 15 +++++++++++++++
>  2 files changed, 28 insertions(+)
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 4e8eb9ede3a4..1e01b2c7ebdf 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -284,6 +284,19 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
>  const char *fdt_string(const void *fdt, int stroffset);
>  
>  /**
> + * fdt_get_max_phandle - retrieves the highest phandle in a tree
> + * @fdt: pointer to the device tree blob
> + *
> + * fdt_get_max_phandle retrieves the highest phandlle in the given

Typo: phandlle

> + * device tree
> + *
> + * returns:
> + *      the highest phandle on success
> + *      0, if an error occured

Typo: occur(r)ed

Otherwise looks good to me!

Reviewed-by: Stefan Agner <stefan@agner.ch>

> + */
> +uint32_t fdt_get_max_phandle(const void *fdt);
> +
> +/**
>   * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
>   * @fdt: pointer to the device tree blob
>   *
> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
> index 7b0777b67eb3..9130ce718965 100644
> --- a/lib/libfdt/fdt_ro.c
> +++ b/lib/libfdt/fdt_ro.c
> @@ -47,6 +47,21 @@ static int _fdt_string_eq(const void *fdt, int stroffset,
>  	return (strnlen(p, len + 1) == len) && (memcmp(p, s, len) == 0);
>  }
>  
> +uint32_t fdt_get_max_phandle(const void *fdt)
> +{
> +	uint32_t max_phandle = 0, phandle;
> +	int offset;
> +
> +	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
> +	     offset = fdt_next_node(fdt, offset, NULL)) {
> +		phandle = fdt_get_phandle(fdt, offset);
> +		if (phandle > max_phandle)
> +			max_phandle = phandle;
> +	}
> +
> +	return max_phandle;
> +}
> +
>  int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
>  {
>  	FDT_CHECK_HEADER(fdt);

Re: [PATCH v2 6/9] libfdt: Add max phandle retrieval function

[Pantelis Antoniou]

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> Add a function to retrieve the highest phandle in a given device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> include/libfdt.h    | 13 +++++++++++++
> lib/libfdt/fdt_ro.c | 15 +++++++++++++++
> 2 files changed, 28 insertions(+)
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 4e8eb9ede3a4..1e01b2c7ebdf 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -284,6 +284,19 @@ int fdt_move(const void *fdt, void *buf, int bufsize);
> const char *fdt_string(const void *fdt, int stroffset);
> 
> /**
> + * fdt_get_max_phandle - retrieves the highest phandle in a tree
> + * @fdt: pointer to the device tree blob
> + *
> + * fdt_get_max_phandle retrieves the highest phandlle in the given
> + * device tree
> + *

^ typo; phandlle -> phandle
> + * returns:
> + *      the highest phandle on success
> + *      0, if an error occured
> + */
> +uint32_t fdt_get_max_phandle(const void *fdt);
> +
> +/**
>  * fdt_num_mem_rsv - retrieve the number of memory reserve map entries
>  * @fdt: pointer to the device tree blob
>  *
> diff --git a/lib/libfdt/fdt_ro.c b/lib/libfdt/fdt_ro.c
> index 7b0777b67eb3..9130ce718965 100644
> --- a/lib/libfdt/fdt_ro.c
> +++ b/lib/libfdt/fdt_ro.c
> @@ -47,6 +47,21 @@ static int _fdt_string_eq(const void *fdt, int stroffset,
> 	return (strnlen(p, len + 1) == len) && (memcmp(p, s, len) == 0);
> }
> 
> +uint32_t fdt_get_max_phandle(const void *fdt)
> +{
> +	uint32_t max_phandle = 0, phandle;
> +	int offset;
> +
> +	for (offset = fdt_next_node(fdt, -1, NULL); offset >= 0;
> +	     offset = fdt_next_node(fdt, offset, NULL)) {
> +		phandle = fdt_get_phandle(fdt, offset);
> +		if (phandle > max_phandle)
> +			max_phandle = phandle;
> +	}
> +
> +	return max_phandle;
> +}
> +
> int fdt_get_mem_rsv(const void *fdt, int n, uint64_t *address, uint64_t *size)
> {
> 	FDT_CHECK_HEADER(fdt);
> -- 
> 2.8.2
> 

Regards

— Pantelis

[PATCH v2 7/9] libfdt: Add overlay application function

[Maxime Ripard]

The device tree overlays are a good way to deal with user-modifyable
boards or boards with some kind of an expansion mechanism where we can
easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).

Add a new function to merge overlays with a base device tree.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 include/libfdt.h         |  30 ++++
 lib/libfdt/Makefile      |   2 +-
 lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 445 insertions(+), 1 deletion(-)
 create mode 100644 lib/libfdt/fdt_overlay.c

diff --git a/include/libfdt.h b/include/libfdt.h
index 1e01b2c7ebdf..783067e841a1 100644
--- a/include/libfdt.h
+++ b/include/libfdt.h
@@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
  */
 int fdt_del_node(void *fdt, int nodeoffset);
 
+/**
+ * fdt_overlay_apply - Applies a DT overlay on a base DT
+ * @fdt: pointer to the base device tree blob
+ * @fdto: pointer to the device tree overlay blob
+ *
+ * fdt_overlay_apply() will apply the given device tree overlay on the
+ * given base device tree.
+ *
+ * Expect the base device tree to be modified, even if the function
+ * returns an error.
+ *
+ * returns:
+ *	0, on success
+ *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
+ *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
+ *		properties in the base DT
+ *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
+ *		magic
+ *	-FDT_ERR_INTERNAL,
+ *	-FDT_ERR_BADLAYOUT,
+ *	-FDT_ERR_BADMAGIC,
+ *	-FDT_ERR_BADOFFSET,
+ *	-FDT_ERR_BADPATH,
+ *	-FDT_ERR_BADVERSION,
+ *	-FDT_ERR_BADSTRUCTURE,
+ *	-FDT_ERR_BADSTATE,
+ *	-FDT_ERR_TRUNCATED, standard meanings
+ */
+int fdt_overlay_apply(void *fdt, void *fdto);
+
 /**********************************************************************/
 /* Debugging / informational functions                                */
 /**********************************************************************/
diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
index 934d6142c3e9..4935bf012a75 100644
--- a/lib/libfdt/Makefile
+++ b/lib/libfdt/Makefile
@@ -6,4 +6,4 @@
 #
 
 obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
-	fdt_empty_tree.o fdt_addresses.o fdt_region.o
+	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
new file mode 100644
index 000000000000..1e68e903c734
--- /dev/null
+++ b/lib/libfdt/fdt_overlay.c
@@ -0,0 +1,414 @@
+#include "libfdt_env.h"
+
+#include <fdt.h>
+#include <libfdt.h>
+
+#include "libfdt_internal.h"
+
+static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
+{
+	const uint32_t *val;
+	int len;
+
+	val = fdt_getprop(fdto, node, "target", &len);
+	if (!val || (len != sizeof(*val)))
+		return 0;
+
+	return fdt32_to_cpu(*val);
+}
+
+static int fdt_overlay_get_target(const void *fdt, const void *fdto,
+				  int fragment)
+{
+	uint32_t phandle;
+	const char *path;
+
+	/* Try first to do a phandle based lookup */
+	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
+	if (phandle)
+		return fdt_node_offset_by_phandle(fdt, phandle);
+
+	/* And then a path based lookup */
+	path = fdt_getprop(fdto, fragment, "target-path", NULL);
+	if (!path)
+		return -FDT_ERR_NOTFOUND;
+
+	return fdt_path_offset(fdt, path);
+}
+
+static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
+					    uint32_t delta)
+{
+	int property;
+	int child;
+
+	fdt_for_each_property(fdto, node, property) {
+		const uint32_t *val;
+		const char *name;
+		uint32_t adj_val;
+		int len;
+		int ret;
+
+		val = fdt_getprop_by_offset(fdto, property,
+					    &name, &len);
+		if (!val || (len != sizeof(*val)))
+			continue;
+
+		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
+			continue;
+
+		adj_val = fdt32_to_cpu(*val);
+		adj_val += delta;
+		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(fdto, child, node)
+		fdt_overlay_adjust_node_phandles(fdto, child, delta);
+
+	return 0;
+}
+
+static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
+{
+	int root;
+
+	root = fdt_path_offset(fdto, "/");
+	if (root < 0)
+		return root;
+
+	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
+}
+
+static int _fdt_overlay_update_local_references(void *fdto,
+						int tree_node,
+						int fixup_node,
+						uint32_t delta)
+{
+	int fixup_prop;
+	int fixup_child;
+
+	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
+		const char *fixup_name, *tree_name;
+		const uint32_t *val = NULL;
+		uint32_t adj_val;
+		int fixup_len;
+		int tree_prop;
+		int tree_len;
+		int ret;
+
+		fdt_getprop_by_offset(fdto, fixup_prop,
+				      &fixup_name, &fixup_len);
+
+		if (!strcmp(fixup_name, "phandle") ||
+		    !strcmp(fixup_name, "linux,phandle"))
+			continue;
+
+		fdt_for_each_property(fdto, tree_node, tree_prop) {
+			val = fdt_getprop_by_offset(fdto, tree_prop,
+						    &tree_name, &tree_len);
+
+			if (!strcmp(tree_name, fixup_name))
+				break;
+		}
+
+		if (!val || !tree_len)
+			return -FDT_ERR_NOTFOUND;
+
+		if (!tree_name)
+			return -FDT_ERR_NOTFOUND;
+
+		if (tree_len != 4)
+			return -FDT_ERR_NOTFOUND;
+
+		adj_val = fdt32_to_cpu(*val);
+		adj_val += delta;
+		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
+					      adj_val);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
+		const char *fixup_child_name = fdt_get_name(fdto,
+							    fixup_child, NULL);
+		int tree_child;
+
+		fdt_for_each_subnode(fdto, tree_child, tree_node) {
+			const char *tree_child_name = fdt_get_name(fdto,
+								   tree_child,
+								   NULL);
+
+			if (!strcmp(fixup_child_name, tree_child_name))
+				break;
+		}
+
+		_fdt_overlay_update_local_references(fdto,
+						     tree_child,
+						     fixup_child,
+						     delta);
+	}
+
+	return 0;
+}
+
+static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
+{
+	int root, fixups;
+
+	root = fdt_path_offset(dto, "/");
+	if (root < 0)
+		return root;
+
+	fixups = fdt_path_offset(dto, "/__local_fixups__");
+	if (root < 0)
+		return root;
+
+	return _fdt_overlay_update_local_references(dto, root, fixups,
+						    delta);
+}
+
+static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
+				      int symbols_off,
+				      const char *path, const char *name,
+				      int index, const char *label)
+{
+	const uint32_t *prop_val;
+	const char *symbol_path;
+	uint32_t *fixup_val;
+	uint32_t phandle;
+	int symbol_off, fixup_off;
+	int prop_len;
+	int ret;
+
+	symbol_path = fdt_getprop(fdt, symbols_off, label,
+				  &prop_len);
+	if (!symbol_path)
+		return -FDT_ERR_NOTFOUND;
+
+	symbol_off = fdt_path_offset(fdt, symbol_path);
+	if (symbol_off < 0)
+		return symbol_off;
+
+	phandle = fdt_get_phandle(fdt, symbol_off);
+	if (!phandle)
+		return -FDT_ERR_NOTFOUND;
+
+	fixup_off = fdt_path_offset(fdto, path);
+	if (fixup_off < 0)
+		return fixup_off;
+
+	prop_val = fdt_getprop(fdto, fixup_off, name,
+			       &prop_len);
+	if (!prop_val)
+		return -FDT_ERR_NOTFOUND;
+
+	fixup_val = malloc(prop_len);
+	if (!fixup_val)
+		return -FDT_ERR_INTERNAL;
+	memcpy(fixup_val, prop_val, prop_len);
+
+	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
+		ret = -FDT_ERR_BADPHANDLE;
+		goto out;
+	}
+
+	fixup_val[index] = cpu_to_fdt32(phandle);
+	ret = fdt_setprop_inplace(fdto, fixup_off,
+				  name, fixup_val,
+				  prop_len);
+
+out:
+	free(fixup_val);
+	return ret;
+};
+
+static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
+				     int property)
+{
+	const char *value, *tmp_value;
+	const char *label;
+	int tmp_len, len, next;
+	int table = 0;
+	int i;
+
+	value = fdt_getprop_by_offset(fdto, property,
+				      &label, &len);
+	tmp_value = value;
+	tmp_len = len;
+
+	do {
+		next = strlen(tmp_value) + 1;
+		tmp_len -= next;
+		tmp_value += next;
+		table++;
+	} while (tmp_len > 0);
+
+	for (i = 0; i < table; i++) {
+		const char *prop_string = value;
+		const char *path, *name;
+		char *prop_cpy, *prop_tmp;
+		int index, stlen;
+		char *sep;
+
+		stlen = strlen(prop_string);
+		prop_cpy = malloc(stlen + 1);
+		if (!prop_cpy)
+			return -FDT_ERR_INTERNAL;
+
+		strncpy(prop_cpy, prop_string, stlen);
+		prop_cpy[stlen] = '\0';
+
+		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
+		     prop_tmp += ((sep - prop_tmp) + 1))
+			*sep = '\0';
+
+		prop_tmp = prop_cpy;
+		path = prop_tmp;
+		prop_tmp += strlen(prop_tmp) + 1;
+
+		name = prop_tmp;
+		prop_tmp += strlen(prop_tmp) + 1;
+
+		index = strtol(prop_tmp, NULL, 10);
+		prop_tmp += strlen(prop_tmp) + 1;
+
+		value += stlen + 1;
+		len -= stlen + 1;
+
+		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
+					   path, name, index, label);
+
+		free(prop_cpy);
+	}
+
+	return 0;
+}
+
+static int fdt_overlay_fixup_phandles(void *dt, void *dto)
+{
+	int fixups_off, symbols_off;
+	int property;
+
+	symbols_off = fdt_path_offset(dt, "/__symbols__");
+	fixups_off = fdt_path_offset(dto, "/__fixups__");
+
+	fdt_for_each_property(dto, fixups_off, property)
+		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
+
+	return 0;
+}
+
+static int fdt_apply_overlay_node(void *dt, void *dto,
+				  int target, int overlay)
+{
+	int property;
+	int node;
+
+	fdt_for_each_property(dto, overlay, property) {
+		const char *name;
+		const void *prop;
+		int prop_len;
+		int ret;
+
+		prop = fdt_getprop_by_offset(dto, property, &name,
+					     &prop_len);
+		if (!prop)
+			return -FDT_ERR_INTERNAL;
+
+		ret = fdt_setprop(dt, target, name, prop, prop_len);
+		if (ret)
+			return ret;
+	}
+
+	fdt_for_each_subnode(dto, node, overlay) {
+		const char *name = fdt_get_name(dto, node, NULL);
+		int nnode;
+		int ret;
+
+		nnode = fdt_add_subnode(dt, target, name);
+		if (nnode < 0)
+			return nnode;
+
+		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+static int fdt_overlay_merge(void *dt, void *dto)
+{
+	int root, fragment;
+
+	root = fdt_path_offset(dto, "/");
+	if (root < 0)
+		return root;
+
+	fdt_for_each_subnode(dto, fragment, root) {
+		const char *name = fdt_get_name(dto, fragment, NULL);
+		uint32_t target;
+		int overlay;
+		int ret;
+
+		if (strncmp(name, "fragment", 8))
+			continue;
+
+		target = fdt_overlay_get_target(dt, dto, fragment);
+		if (target < 0)
+			return target;
+
+		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
+		if (overlay < 0)
+			return overlay;
+
+		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+int fdt_overlay_apply(void *fdt, void *fdto)
+{
+	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
+	void *fdto_copy;
+	int ret;
+
+	FDT_CHECK_HEADER(fdt);
+	FDT_CHECK_HEADER(fdto);
+
+	/*
+	 * We're going to modify the overlay so that we can apply it.
+	 *
+	 * Make sure sure we don't destroy the original
+	 */
+	fdto_copy = malloc(fdt_totalsize(fdto));
+	if (!fdto_copy)
+		return -FDT_ERR_INTERNAL;
+
+	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
+	if (ret)
+		goto out;
+
+	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
+	if (ret)
+		goto out;
+
+	ret = fdt_overlay_update_local_references(fdto_copy, delta);
+	if (ret)
+		goto out;
+
+	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
+	if (ret)
+		goto out;
+
+	ret = fdt_overlay_merge(fdt, fdto_copy);
+
+out:
+	free(fdto_copy);
+	return ret;
+}
-- 
2.8.2

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Stefan Agner]

On 2016-05-27 02:13, Maxime Ripard wrote:
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> 
> Add a new function to merge overlays with a base device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  include/libfdt.h         |  30 ++++
>  lib/libfdt/Makefile      |   2 +-
>  lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 445 insertions(+), 1 deletion(-)
>  create mode 100644 lib/libfdt/fdt_overlay.c
> 

How big is the bloat? Did you thought about making it optional? e.g.
CONFIG_OF_LIBFDT_OVERLAY

What is the general opinion on that?


> diff --git a/include/libfdt.h b/include/libfdt.h
> index 1e01b2c7ebdf..783067e841a1 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int
> parentoffset, const char *name);
>   */
>  int fdt_del_node(void *fdt, int nodeoffset);
>  
> +/**
> + * fdt_overlay_apply - Applies a DT overlay on a base DT
> + * @fdt: pointer to the base device tree blob
> + * @fdto: pointer to the device tree overlay blob
> + *
> + * fdt_overlay_apply() will apply the given device tree overlay on the
> + * given base device tree.
> + *
> + * Expect the base device tree to be modified, even if the function
> + * returns an error.

Hm this sounds somewhat contradicting to a comment inside that
function...

> + *
> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
> + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
> + *		properties in the base DT
> + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
> + *		magic
> + *	-FDT_ERR_INTERNAL,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADOFFSET,
> + *	-FDT_ERR_BADPATH,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_overlay_apply(void *fdt, void *fdto);
> +
>  /**********************************************************************/
>  /* Debugging / informational functions                                */
>  /**********************************************************************/
> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> index 934d6142c3e9..4935bf012a75 100644
> --- a/lib/libfdt/Makefile
> +++ b/lib/libfdt/Makefile
> @@ -6,4 +6,4 @@
>  #
>  
>  obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
> -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
> +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> new file mode 100644
> index 000000000000..1e68e903c734
> --- /dev/null
> +++ b/lib/libfdt/fdt_overlay.c
> @@ -0,0 +1,414 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(fdto, node, "target", &len);
> +	if (!val || (len != sizeof(*val)))
> +		return 0;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> +				  int fragment)
> +{
> +	uint32_t phandle;
> +	const char *path;
> +
> +	/* Try first to do a phandle based lookup */
> +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> +	if (phandle)
> +		return fdt_node_offset_by_phandle(fdt, phandle);
> +
> +	/* And then a path based lookup */
> +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> +	if (!path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	return fdt_path_offset(fdt, path);
> +}
> +
> +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
> +					    uint32_t delta)
> +{
> +	int property;
> +	int child;
> +
> +	fdt_for_each_property(fdto, node, property) {
> +		const uint32_t *val;
> +		const char *name;
> +		uint32_t adj_val;
> +		int len;
> +		int ret;
> +
> +		val = fdt_getprop_by_offset(fdto, property,
> +					    &name, &len);
> +		if (!val || (len != sizeof(*val)))
> +			continue;
> +
> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> +			continue;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, child, node)
> +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> +{
> +	int root;
> +
> +	root = fdt_path_offset(fdto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *fdto,
> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +
> +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
> +		const char *fixup_name, *tree_name;
> +		const uint32_t *val = NULL;
> +		uint32_t adj_val;
> +		int fixup_len;
> +		int tree_prop;
> +		int tree_len;
> +		int ret;
> +
> +		fdt_getprop_by_offset(fdto, fixup_prop,
> +				      &fixup_name, &fixup_len);
> +
> +		if (!strcmp(fixup_name, "phandle") ||
> +		    !strcmp(fixup_name, "linux,phandle"))
> +			continue;
> +
> +		fdt_for_each_property(fdto, tree_node, tree_prop) {
> +			val = fdt_getprop_by_offset(fdto, tree_prop,
> +						    &tree_name, &tree_len);
> +
> +			if (!strcmp(tree_name, fixup_name))
> +				break;
> +		}
> +
> +		if (!val || !tree_len)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (!tree_name)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (tree_len != 4)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> +					      adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> +		const char *fixup_child_name = fdt_get_name(fdto,
> +							    fixup_child, NULL);
> +		int tree_child;
> +
> +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
> +			const char *tree_child_name = fdt_get_name(fdto,
> +								   tree_child,
> +								   NULL);
> +
> +			if (!strcmp(fixup_child_name, tree_child_name))
> +				break;
> +		}
> +
> +		_fdt_overlay_update_local_references(fdto,
> +						     tree_child,
> +						     fixup_child,
> +						     delta);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +	int root, fixups;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> +	if (root < 0)
> +		return root;
> +
> +	return _fdt_overlay_update_local_references(dto, root, fixups,
> +						    delta);
> +}
> +
> +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
> +				      int symbols_off,
> +				      const char *path, const char *name,
> +				      int index, const char *label)
> +{
> +	const uint32_t *prop_val;
> +	const char *symbol_path;
> +	uint32_t *fixup_val;
> +	uint32_t phandle;
> +	int symbol_off, fixup_off;
> +	int prop_len;
> +	int ret;
> +
> +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +				  &prop_len);
> +	if (!symbol_path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	symbol_off = fdt_path_offset(fdt, symbol_path);
> +	if (symbol_off < 0)
> +		return symbol_off;
> +
> +	phandle = fdt_get_phandle(fdt, symbol_off);
> +	if (!phandle)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_off = fdt_path_offset(fdto, path);
> +	if (fixup_off < 0)
> +		return fixup_off;
> +
> +	prop_val = fdt_getprop(fdto, fixup_off, name,
> +			       &prop_len);
> +	if (!prop_val)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_val = malloc(prop_len);
> +	if (!fixup_val)
> +		return -FDT_ERR_INTERNAL;
> +	memcpy(fixup_val, prop_val, prop_len);
> +
> +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
> +		ret = -FDT_ERR_BADPHANDLE;
> +		goto out;
> +	}
> +
> +	fixup_val[index] = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop_inplace(fdto, fixup_off,
> +				  name, fixup_val,
> +				  prop_len);
> +
> +out:
> +	free(fixup_val);
> +	return ret;
> +};
> +
> +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +				     int property)
> +{
> +	const char *value, *tmp_value;
> +	const char *label;
> +	int tmp_len, len, next;
> +	int table = 0;
> +	int i;
> +
> +	value = fdt_getprop_by_offset(fdto, property,
> +				      &label, &len);
> +	tmp_value = value;
> +	tmp_len = len;
> +
> +	do {
> +		next = strlen(tmp_value) + 1;
> +		tmp_len -= next;
> +		tmp_value += next;
> +		table++;
> +	} while (tmp_len > 0);
> +
> +	for (i = 0; i < table; i++) {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		char *prop_cpy, *prop_tmp;
> +		int index, stlen;
> +		char *sep;
> +
> +		stlen = strlen(prop_string);
> +		prop_cpy = malloc(stlen + 1);
> +		if (!prop_cpy)
> +			return -FDT_ERR_INTERNAL;

A malloc in a inner loop sounds somewhat scary to me, but it probably
doesn't matter that much, not sure how memory management in U-Boot
exactly works.

I tried to find a maximum property length in order to use a stack
allocated variable, but it seems that the property length is
unlimited...

What you might try is using realloc. It should only resize the
allocation if the requested size is larger.
Initialize with 255, and then realloc in here, and free at the end of
the function....



> +
> +		strncpy(prop_cpy, prop_string, stlen);
> +		prop_cpy[stlen] = '\0';
> +
> +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
> +		     prop_tmp += ((sep - prop_tmp) + 1))
> +			*sep = '\0';
> +
> +		prop_tmp = prop_cpy;
> +		path = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		name = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		index = strtol(prop_tmp, NULL, 10);
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		value += stlen + 1;
> +		len -= stlen + 1;
> +
> +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
> +					   path, name, index, label);
> +
> +		free(prop_cpy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> +	fdt_for_each_property(dto, fixups_off, property)
> +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> +
> +	return 0;
> +}
> +
> +static int fdt_apply_overlay_node(void *dt, void *dto,
> +				  int target, int overlay)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property(dto, overlay, property) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(dto, property, &name,
> +					     &prop_len);
> +		if (!prop)
> +			return -FDT_ERR_INTERNAL;
> +
> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(dto, node, overlay) {
> +		const char *name = fdt_get_name(dto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(dt, target, name);
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_merge(void *dt, void *dto)
> +{
> +	int root, fragment;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fdt_for_each_subnode(dto, fragment, root) {
> +		const char *name = fdt_get_name(dto, fragment, NULL);
> +		uint32_t target;
> +		int overlay;
> +		int ret;
> +
> +		if (strncmp(name, "fragment", 8))
> +			continue;
> +
> +		target = fdt_overlay_get_target(dt, dto, fragment);
> +		if (target < 0)
> +			return target;
> +
> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int fdt_overlay_apply(void *fdt, void *fdto)
> +{
> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> +	void *fdto_copy;
> +	int ret;
> +
> +	FDT_CHECK_HEADER(fdt);
> +	FDT_CHECK_HEADER(fdto);
> +
> +	/*
> +	 * We're going to modify the overlay so that we can apply it.
> +	 *
> +	 * Make sure sure we don't destroy the original
> +	 */

... here.

> +	fdto_copy = malloc(fdt_totalsize(fdto));
> +	if (!fdto_copy)
> +		return -FDT_ERR_INTERNAL;
> +
> +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_merge(fdt, fdto_copy);
> +
> +out:
> +	free(fdto_copy);
> +	return ret;
> +}


--
Stefan

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Pantelis Antoniou]

Hi Maxime,

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> 
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> 
> Add a new function to merge overlays with a base device tree.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> include/libfdt.h         |  30 ++++
> lib/libfdt/Makefile      |   2 +-
> lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 445 insertions(+), 1 deletion(-)
> create mode 100644 lib/libfdt/fdt_overlay.c
> 
> diff --git a/include/libfdt.h b/include/libfdt.h
> index 1e01b2c7ebdf..783067e841a1 100644
> --- a/include/libfdt.h
> +++ b/include/libfdt.h
> @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
>  */
> int fdt_del_node(void *fdt, int nodeoffset);
> 
> +/**
> + * fdt_overlay_apply - Applies a DT overlay on a base DT
> + * @fdt: pointer to the base device tree blob
> + * @fdto: pointer to the device tree overlay blob
> + *
> + * fdt_overlay_apply() will apply the given device tree overlay on the
> + * given base device tree.
> + *
> + * Expect the base device tree to be modified, even if the function
> + * returns an error.
> + *

If the base tree has been modified on an error it is unsafe to use it
for booting. A valid strategy would be to scribble over the DT magic
number so that that blob is invalidated.

What are the other people’s opinion on this?

> + * returns:
> + *	0, on success
> + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
> + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
> + *		properties in the base DT
> + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
> + *		magic
> + *	-FDT_ERR_INTERNAL,
> + *	-FDT_ERR_BADLAYOUT,
> + *	-FDT_ERR_BADMAGIC,
> + *	-FDT_ERR_BADOFFSET,
> + *	-FDT_ERR_BADPATH,
> + *	-FDT_ERR_BADVERSION,
> + *	-FDT_ERR_BADSTRUCTURE,
> + *	-FDT_ERR_BADSTATE,
> + *	-FDT_ERR_TRUNCATED, standard meanings
> + */
> +int fdt_overlay_apply(void *fdt, void *fdto);
> +
> /**********************************************************************/
> /* Debugging / informational functions                                */
> /**********************************************************************/
> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> index 934d6142c3e9..4935bf012a75 100644
> --- a/lib/libfdt/Makefile
> +++ b/lib/libfdt/Makefile
> @@ -6,4 +6,4 @@
> #
> 
> obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
> -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
> +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> new file mode 100644
> index 000000000000..1e68e903c734
> --- /dev/null
> +++ b/lib/libfdt/fdt_overlay.c
> @@ -0,0 +1,414 @@
> +#include "libfdt_env.h"
> +
> +#include <fdt.h>
> +#include <libfdt.h>
> +
> +#include "libfdt_internal.h"
> +
> +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
> +{
> +	const uint32_t *val;
> +	int len;
> +
> +	val = fdt_getprop(fdto, node, "target", &len);
> +	if (!val || (len != sizeof(*val)))
> +		return 0;
> +
> +	return fdt32_to_cpu(*val);
> +}
> +
> +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> +				  int fragment)
> +{
> +	uint32_t phandle;
> +	const char *path;
> +
> +	/* Try first to do a phandle based lookup */
> +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> +	if (phandle)
> +		return fdt_node_offset_by_phandle(fdt, phandle);
> +
> +	/* And then a path based lookup */
> +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> +	if (!path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	return fdt_path_offset(fdt, path);
> +}
> +

Those targets are fine; beware there are patches with more target options.

> +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
> +					    uint32_t delta)
> +{
> +	int property;
> +	int child;
> +
> +	fdt_for_each_property(fdto, node, property) {
> +		const uint32_t *val;
> +		const char *name;
> +		uint32_t adj_val;
> +		int len;
> +		int ret;
> +
> +		val = fdt_getprop_by_offset(fdto, property,
> +					    &name, &len);
> +		if (!val || (len != sizeof(*val)))

Superfluous parentheses.

> +			continue;
> +
> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> +			continue;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, child, node)
> +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> +{
> +	int root;
> +
> +	root = fdt_path_offset(fdto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> +}
> +
> +static int _fdt_overlay_update_local_references(void *fdto,
> +						int tree_node,
> +						int fixup_node,
> +						uint32_t delta)
> +{
> +	int fixup_prop;
> +	int fixup_child;
> +
> +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
> +		const char *fixup_name, *tree_name;
> +		const uint32_t *val = NULL;
> +		uint32_t adj_val;
> +		int fixup_len;
> +		int tree_prop;
> +		int tree_len;
> +		int ret;
> +
> +		fdt_getprop_by_offset(fdto, fixup_prop,
> +				      &fixup_name, &fixup_len);
> +
> +		if (!strcmp(fixup_name, "phandle") ||
> +		    !strcmp(fixup_name, "linux,phandle"))
> +			continue;
> +
> +		fdt_for_each_property(fdto, tree_node, tree_prop) {
> +			val = fdt_getprop_by_offset(fdto, tree_prop,
> +						    &tree_name, &tree_len);
> +
> +			if (!strcmp(tree_name, fixup_name))
> +				break;
> +		}
> +
> +		if (!val || !tree_len)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (!tree_name)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		if (tree_len != 4)
> +			return -FDT_ERR_NOTFOUND;
> +
> +		adj_val = fdt32_to_cpu(*val);
> +		adj_val += delta;
> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> +					      adj_val);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> +		const char *fixup_child_name = fdt_get_name(fdto,
> +							    fixup_child, NULL);
> +		int tree_child;
> +
> +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
> +			const char *tree_child_name = fdt_get_name(fdto,
> +								   tree_child,
> +								   NULL);
> +
> +			if (!strcmp(fixup_child_name, tree_child_name))
> +				break;
> +		}
> +
> +		_fdt_overlay_update_local_references(fdto,
> +						     tree_child,
> +						     fixup_child,
> +						     delta);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> +{
> +	int root, fixups;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> +	if (root < 0)
> +		return root;
> +
> +	return _fdt_overlay_update_local_references(dto, root, fixups,
> +						    delta);
> +}
> +
> +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
> +				      int symbols_off,
> +				      const char *path, const char *name,
> +				      int index, const char *label)
> +{
> +	const uint32_t *prop_val;
> +	const char *symbol_path;
> +	uint32_t *fixup_val;
> +	uint32_t phandle;
> +	int symbol_off, fixup_off;
> +	int prop_len;
> +	int ret;
> +
> +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> +				  &prop_len);
> +	if (!symbol_path)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	symbol_off = fdt_path_offset(fdt, symbol_path);
> +	if (symbol_off < 0)
> +		return symbol_off;
> +
> +	phandle = fdt_get_phandle(fdt, symbol_off);
> +	if (!phandle)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_off = fdt_path_offset(fdto, path);
> +	if (fixup_off < 0)
> +		return fixup_off;
> +
> +	prop_val = fdt_getprop(fdto, fixup_off, name,
> +			       &prop_len);
> +	if (!prop_val)
> +		return -FDT_ERR_NOTFOUND;
> +
> +	fixup_val = malloc(prop_len);
> +	if (!fixup_val)
> +		return -FDT_ERR_INTERNAL;
> +	memcpy(fixup_val, prop_val, prop_len);
> +
> +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
> +		ret = -FDT_ERR_BADPHANDLE;
> +		goto out;
> +	}
> +
> +	fixup_val[index] = cpu_to_fdt32(phandle);
> +	ret = fdt_setprop_inplace(fdto, fixup_off,
> +				  name, fixup_val,
> +				  prop_len);
> +
> +out:
> +	free(fixup_val);
> +	return ret;
> +};
> +
> +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> +				     int property)
> +{
> +	const char *value, *tmp_value;
> +	const char *label;
> +	int tmp_len, len, next;
> +	int table = 0;
> +	int i;
> +
> +	value = fdt_getprop_by_offset(fdto, property,
> +				      &label, &len);
> +	tmp_value = value;
> +	tmp_len = len;
> +
> +	do {
> +		next = strlen(tmp_value) + 1;
> +		tmp_len -= next;
> +		tmp_value += next;
> +		table++;
> +	} while (tmp_len > 0);
> +
> +	for (i = 0; i < table; i++) {
> +		const char *prop_string = value;
> +		const char *path, *name;
> +		char *prop_cpy, *prop_tmp;
> +		int index, stlen;
> +		char *sep;
> +
> +		stlen = strlen(prop_string);
> +		prop_cpy = malloc(stlen + 1);
> +		if (!prop_cpy)
> +			return -FDT_ERR_INTERNAL;
> +
> +		strncpy(prop_cpy, prop_string, stlen);
> +		prop_cpy[stlen] = '\0';
> +
> +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
> +		     prop_tmp += ((sep - prop_tmp) + 1))
> +			*sep = '\0';
> +
> +		prop_tmp = prop_cpy;
> +		path = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		name = prop_tmp;
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		index = strtol(prop_tmp, NULL, 10);
> +		prop_tmp += strlen(prop_tmp) + 1;
> +
> +		value += stlen + 1;
> +		len -= stlen + 1;
> +
> +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
> +					   path, name, index, label);
> +
> +		free(prop_cpy);
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> +{
> +	int fixups_off, symbols_off;
> +	int property;
> +
> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> +
> +	fdt_for_each_property(dto, fixups_off, property)
> +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> +
> +	return 0;
> +}
> +
> +static int fdt_apply_overlay_node(void *dt, void *dto,
> +				  int target, int overlay)
> +{
> +	int property;
> +	int node;
> +
> +	fdt_for_each_property(dto, overlay, property) {
> +		const char *name;
> +		const void *prop;
> +		int prop_len;
> +		int ret;
> +
> +		prop = fdt_getprop_by_offset(dto, property, &name,
> +					     &prop_len);
> +		if (!prop)
> +			return -FDT_ERR_INTERNAL;
> +
> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	fdt_for_each_subnode(dto, node, overlay) {
> +		const char *name = fdt_get_name(dto, node, NULL);
> +		int nnode;
> +		int ret;
> +
> +		nnode = fdt_add_subnode(dt, target, name);
> +		if (nnode < 0)
> +			return nnode;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int fdt_overlay_merge(void *dt, void *dto)
> +{
> +	int root, fragment;
> +
> +	root = fdt_path_offset(dto, "/");
> +	if (root < 0)
> +		return root;
> +
> +	fdt_for_each_subnode(dto, fragment, root) {
> +		const char *name = fdt_get_name(dto, fragment, NULL);
> +		uint32_t target;
> +		int overlay;
> +		int ret;
> +
> +		if (strncmp(name, "fragment", 8))
> +			continue;
> +

This is incorrect. The use of “fragment” is a convention only.
The real test whether the node is an overlay fragment is that
it contains a target property.

> +		target = fdt_overlay_get_target(dt, dto, fragment);
> +		if (target < 0)
> +			return target;
> +

So you could do ‘if (target < 0) continue;’ or handle a more complex error code.

> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> +		if (overlay < 0)
> +			return overlay;
> +
> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +int fdt_overlay_apply(void *fdt, void *fdto)
> +{
> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> +	void *fdto_copy;
> +	int ret;
> +
> +	FDT_CHECK_HEADER(fdt);
> +	FDT_CHECK_HEADER(fdto);
> +
> +	/*
> +	 * We're going to modify the overlay so that we can apply it.
> +	 *
> +	 * Make sure sure we don't destroy the original
> +	 */
> +	fdto_copy = malloc(fdt_totalsize(fdto));
> +	if (!fdto_copy)
> +		return -FDT_ERR_INTERNAL;
> +
> +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> +	if (ret)
> +		goto out;
> +
> +	ret = fdt_overlay_merge(fdt, fdto_copy);
> +
> +out:
> +	free(fdto_copy);
> +	return ret;
> +}
> -- 
> 2.8.2
> 

I would caution against the liberal use of malloc in libfdt. We’re possibly running in a constrained
environment; a custom extents based (non freeing) allocator should be better.

We need some figures about memory consumption when this is enabled, and a CONFIG option to disable it.

Regards

— Pantelis

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4029 bytes --]

Hi Pantelis,

On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> Hi Maxime,
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> > 
> > Add a new function to merge overlays with a base device tree.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > include/libfdt.h         |  30 ++++
> > lib/libfdt/Makefile      |   2 +-
> > lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 445 insertions(+), 1 deletion(-)
> > create mode 100644 lib/libfdt/fdt_overlay.c
> > 
> > diff --git a/include/libfdt.h b/include/libfdt.h
> > index 1e01b2c7ebdf..783067e841a1 100644
> > --- a/include/libfdt.h
> > +++ b/include/libfdt.h
> > @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
> >  */
> > int fdt_del_node(void *fdt, int nodeoffset);
> > 
> > +/**
> > + * fdt_overlay_apply - Applies a DT overlay on a base DT
> > + * @fdt: pointer to the base device tree blob
> > + * @fdto: pointer to the device tree overlay blob
> > + *
> > + * fdt_overlay_apply() will apply the given device tree overlay on the
> > + * given base device tree.
> > + *
> > + * Expect the base device tree to be modified, even if the function
> > + * returns an error.
> > + *
> 
> If the base tree has been modified on an error it is unsafe to use it
> for booting. A valid strategy would be to scribble over the DT magic
> number so that that blob is invalidated.
> 
> What are the other people’s opinion on this?

That would probably be safer yes, I'll change that.

> > +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> > +				  int fragment)
> > +{
> > +	uint32_t phandle;
> > +	const char *path;
> > +
> > +	/* Try first to do a phandle based lookup */
> > +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> > +	if (phandle)
> > +		return fdt_node_offset_by_phandle(fdt, phandle);
> > +
> > +	/* And then a path based lookup */
> > +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > +	if (!path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	return fdt_path_offset(fdt, path);
> > +}
> > +
> 
> Those targets are fine; beware there are patches with more target options.

Ack.

> > +static int fdt_overlay_merge(void *dt, void *dto)
> > +{
> > +	int root, fragment;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fdt_for_each_subnode(dto, fragment, root) {
> > +		const char *name = fdt_get_name(dto, fragment, NULL);
> > +		uint32_t target;
> > +		int overlay;
> > +		int ret;
> > +
> > +		if (strncmp(name, "fragment", 8))
> > +			continue;
> > +
> 
> This is incorrect. The use of “fragment” is a convention only.
> The real test whether the node is an overlay fragment is that
> it contains a target property.
> 
> > +		target = fdt_overlay_get_target(dt, dto, fragment);
> > +		if (target < 0)
> > +			return target;
> > +
> 
> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.

Ok, will change.

> I would caution against the liberal use of malloc in libfdt. We’re
> possibly running in a constrained environment; a custom extents
> based (non freeing) allocator should be better.

David had the same comments, and I will drop the mallocs entirely.

> We need some figures about memory consumption when this is enabled,
> and a CONFIG option to disable it.

The current code (before and after that patch) adds 18kB to a
sandbox_defconfig.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 15694 bytes --]

On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> Hi Maxime,
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> > 
> > Add a new function to merge overlays with a base device tree.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > include/libfdt.h         |  30 ++++
> > lib/libfdt/Makefile      |   2 +-
> > lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 445 insertions(+), 1 deletion(-)
> > create mode 100644 lib/libfdt/fdt_overlay.c
> > 
> > diff --git a/include/libfdt.h b/include/libfdt.h
> > index 1e01b2c7ebdf..783067e841a1 100644
> > --- a/include/libfdt.h
> > +++ b/include/libfdt.h
> > @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
> >  */
> > int fdt_del_node(void *fdt, int nodeoffset);
> > 
> > +/**
> > + * fdt_overlay_apply - Applies a DT overlay on a base DT
> > + * @fdt: pointer to the base device tree blob
> > + * @fdto: pointer to the device tree overlay blob
> > + *
> > + * fdt_overlay_apply() will apply the given device tree overlay on the
> > + * given base device tree.
> > + *
> > + * Expect the base device tree to be modified, even if the function
> > + * returns an error.
> > + *
> 
> If the base tree has been modified on an error it is unsafe to use it
> for booting. A valid strategy would be to scribble over the DT magic
> number so that that blob is invalidated.
> 
> What are the other people’s opinion on this?
> 
> > + * returns:
> > + *	0, on success
> > + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
> > + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
> > + *		properties in the base DT
> > + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
> > + *		magic
> > + *	-FDT_ERR_INTERNAL,
> > + *	-FDT_ERR_BADLAYOUT,
> > + *	-FDT_ERR_BADMAGIC,
> > + *	-FDT_ERR_BADOFFSET,
> > + *	-FDT_ERR_BADPATH,
> > + *	-FDT_ERR_BADVERSION,
> > + *	-FDT_ERR_BADSTRUCTURE,
> > + *	-FDT_ERR_BADSTATE,
> > + *	-FDT_ERR_TRUNCATED, standard meanings
> > + */
> > +int fdt_overlay_apply(void *fdt, void *fdto);
> > +
> > /**********************************************************************/
> > /* Debugging / informational functions                                */
> > /**********************************************************************/
> > diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
> > index 934d6142c3e9..4935bf012a75 100644
> > --- a/lib/libfdt/Makefile
> > +++ b/lib/libfdt/Makefile
> > @@ -6,4 +6,4 @@
> > #
> > 
> > obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
> > -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
> > +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
> > diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
> > new file mode 100644
> > index 000000000000..1e68e903c734
> > --- /dev/null
> > +++ b/lib/libfdt/fdt_overlay.c
> > @@ -0,0 +1,414 @@
> > +#include "libfdt_env.h"
> > +
> > +#include <fdt.h>
> > +#include <libfdt.h>
> > +
> > +#include "libfdt_internal.h"
> > +
> > +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
> > +{
> > +	const uint32_t *val;
> > +	int len;
> > +
> > +	val = fdt_getprop(fdto, node, "target", &len);
> > +	if (!val || (len != sizeof(*val)))
> > +		return 0;
> > +
> > +	return fdt32_to_cpu(*val);
> > +}
> > +
> > +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> > +				  int fragment)
> > +{
> > +	uint32_t phandle;
> > +	const char *path;
> > +
> > +	/* Try first to do a phandle based lookup */
> > +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> > +	if (phandle)
> > +		return fdt_node_offset_by_phandle(fdt, phandle);
> > +
> > +	/* And then a path based lookup */
> > +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > +	if (!path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	return fdt_path_offset(fdt, path);
> > +}
> > +
> 
> Those targets are fine; beware there are patches with more target options.
> 
> > +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
> > +					    uint32_t delta)
> > +{
> > +	int property;
> > +	int child;
> > +
> > +	fdt_for_each_property(fdto, node, property) {
> > +		const uint32_t *val;
> > +		const char *name;
> > +		uint32_t adj_val;
> > +		int len;
> > +		int ret;
> > +
> > +		val = fdt_getprop_by_offset(fdto, property,
> > +					    &name, &len);
> > +		if (!val || (len != sizeof(*val)))
> 
> Superfluous parentheses.
> 
> > +			continue;
> > +
> > +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
> > +			continue;
> > +
> > +		adj_val = fdt32_to_cpu(*val);
> > +		adj_val += delta;
> > +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	fdt_for_each_subnode(fdto, child, node)
> > +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
> > +{
> > +	int root;
> > +
> > +	root = fdt_path_offset(fdto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
> > +}
> > +
> > +static int _fdt_overlay_update_local_references(void *fdto,
> > +						int tree_node,
> > +						int fixup_node,
> > +						uint32_t delta)
> > +{
> > +	int fixup_prop;
> > +	int fixup_child;
> > +
> > +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
> > +		const char *fixup_name, *tree_name;
> > +		const uint32_t *val = NULL;
> > +		uint32_t adj_val;
> > +		int fixup_len;
> > +		int tree_prop;
> > +		int tree_len;
> > +		int ret;
> > +
> > +		fdt_getprop_by_offset(fdto, fixup_prop,
> > +				      &fixup_name, &fixup_len);
> > +
> > +		if (!strcmp(fixup_name, "phandle") ||
> > +		    !strcmp(fixup_name, "linux,phandle"))
> > +			continue;
> > +
> > +		fdt_for_each_property(fdto, tree_node, tree_prop) {
> > +			val = fdt_getprop_by_offset(fdto, tree_prop,
> > +						    &tree_name, &tree_len);
> > +
> > +			if (!strcmp(tree_name, fixup_name))
> > +				break;
> > +		}
> > +
> > +		if (!val || !tree_len)
> > +			return -FDT_ERR_NOTFOUND;
> > +
> > +		if (!tree_name)
> > +			return -FDT_ERR_NOTFOUND;
> > +
> > +		if (tree_len != 4)
> > +			return -FDT_ERR_NOTFOUND;
> > +
> > +		adj_val = fdt32_to_cpu(*val);
> > +		adj_val += delta;
> > +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
> > +					      adj_val);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
> > +		const char *fixup_child_name = fdt_get_name(fdto,
> > +							    fixup_child, NULL);
> > +		int tree_child;
> > +
> > +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
> > +			const char *tree_child_name = fdt_get_name(fdto,
> > +								   tree_child,
> > +								   NULL);
> > +
> > +			if (!strcmp(fixup_child_name, tree_child_name))
> > +				break;
> > +		}
> > +
> > +		_fdt_overlay_update_local_references(fdto,
> > +						     tree_child,
> > +						     fixup_child,
> > +						     delta);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
> > +{
> > +	int root, fixups;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fixups = fdt_path_offset(dto, "/__local_fixups__");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	return _fdt_overlay_update_local_references(dto, root, fixups,
> > +						    delta);
> > +}
> > +
> > +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
> > +				      int symbols_off,
> > +				      const char *path, const char *name,
> > +				      int index, const char *label)
> > +{
> > +	const uint32_t *prop_val;
> > +	const char *symbol_path;
> > +	uint32_t *fixup_val;
> > +	uint32_t phandle;
> > +	int symbol_off, fixup_off;
> > +	int prop_len;
> > +	int ret;
> > +
> > +	symbol_path = fdt_getprop(fdt, symbols_off, label,
> > +				  &prop_len);
> > +	if (!symbol_path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	symbol_off = fdt_path_offset(fdt, symbol_path);
> > +	if (symbol_off < 0)
> > +		return symbol_off;
> > +
> > +	phandle = fdt_get_phandle(fdt, symbol_off);
> > +	if (!phandle)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	fixup_off = fdt_path_offset(fdto, path);
> > +	if (fixup_off < 0)
> > +		return fixup_off;
> > +
> > +	prop_val = fdt_getprop(fdto, fixup_off, name,
> > +			       &prop_len);
> > +	if (!prop_val)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	fixup_val = malloc(prop_len);
> > +	if (!fixup_val)
> > +		return -FDT_ERR_INTERNAL;
> > +	memcpy(fixup_val, prop_val, prop_len);
> > +
> > +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
> > +		ret = -FDT_ERR_BADPHANDLE;
> > +		goto out;
> > +	}
> > +
> > +	fixup_val[index] = cpu_to_fdt32(phandle);
> > +	ret = fdt_setprop_inplace(fdto, fixup_off,
> > +				  name, fixup_val,
> > +				  prop_len);
> > +
> > +out:
> > +	free(fixup_val);
> > +	return ret;
> > +};
> > +
> > +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
> > +				     int property)
> > +{
> > +	const char *value, *tmp_value;
> > +	const char *label;
> > +	int tmp_len, len, next;
> > +	int table = 0;
> > +	int i;
> > +
> > +	value = fdt_getprop_by_offset(fdto, property,
> > +				      &label, &len);
> > +	tmp_value = value;
> > +	tmp_len = len;
> > +
> > +	do {
> > +		next = strlen(tmp_value) + 1;
> > +		tmp_len -= next;
> > +		tmp_value += next;
> > +		table++;
> > +	} while (tmp_len > 0);
> > +
> > +	for (i = 0; i < table; i++) {
> > +		const char *prop_string = value;
> > +		const char *path, *name;
> > +		char *prop_cpy, *prop_tmp;
> > +		int index, stlen;
> > +		char *sep;
> > +
> > +		stlen = strlen(prop_string);
> > +		prop_cpy = malloc(stlen + 1);
> > +		if (!prop_cpy)
> > +			return -FDT_ERR_INTERNAL;
> > +
> > +		strncpy(prop_cpy, prop_string, stlen);
> > +		prop_cpy[stlen] = '\0';
> > +
> > +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
> > +		     prop_tmp += ((sep - prop_tmp) + 1))
> > +			*sep = '\0';
> > +
> > +		prop_tmp = prop_cpy;
> > +		path = prop_tmp;
> > +		prop_tmp += strlen(prop_tmp) + 1;
> > +
> > +		name = prop_tmp;
> > +		prop_tmp += strlen(prop_tmp) + 1;
> > +
> > +		index = strtol(prop_tmp, NULL, 10);
> > +		prop_tmp += strlen(prop_tmp) + 1;
> > +
> > +		value += stlen + 1;
> > +		len -= stlen + 1;
> > +
> > +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
> > +					   path, name, index, label);
> > +
> > +		free(prop_cpy);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
> > +{
> > +	int fixups_off, symbols_off;
> > +	int property;
> > +
> > +	symbols_off = fdt_path_offset(dt, "/__symbols__");
> > +	fixups_off = fdt_path_offset(dto, "/__fixups__");
> > +
> > +	fdt_for_each_property(dto, fixups_off, property)
> > +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_apply_overlay_node(void *dt, void *dto,
> > +				  int target, int overlay)
> > +{
> > +	int property;
> > +	int node;
> > +
> > +	fdt_for_each_property(dto, overlay, property) {
> > +		const char *name;
> > +		const void *prop;
> > +		int prop_len;
> > +		int ret;
> > +
> > +		prop = fdt_getprop_by_offset(dto, property, &name,
> > +					     &prop_len);
> > +		if (!prop)
> > +			return -FDT_ERR_INTERNAL;
> > +
> > +		ret = fdt_setprop(dt, target, name, prop, prop_len);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	fdt_for_each_subnode(dto, node, overlay) {
> > +		const char *name = fdt_get_name(dto, node, NULL);
> > +		int nnode;
> > +		int ret;
> > +
> > +		nnode = fdt_add_subnode(dt, target, name);
> > +		if (nnode < 0)
> > +			return nnode;
> > +
> > +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int fdt_overlay_merge(void *dt, void *dto)
> > +{
> > +	int root, fragment;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fdt_for_each_subnode(dto, fragment, root) {
> > +		const char *name = fdt_get_name(dto, fragment, NULL);
> > +		uint32_t target;
> > +		int overlay;
> > +		int ret;
> > +
> > +		if (strncmp(name, "fragment", 8))
> > +			continue;
> > +
> 
> This is incorrect. The use of “fragment” is a convention only.
> The real test whether the node is an overlay fragment is that
> it contains a target property.

Hmm.. I dislike that approach.  First, it means that if new target
types are introduced in future, older code is likely to silently
ignore such fragments instead of realizing that it doesn't know how to
apply themm.  Second, it raises weird issues if some node down within
a fragment also happens to have a property called "target".


> > +		target = fdt_overlay_get_target(dt, dto, fragment);
> > +		if (target < 0)
> > +			return target;
> > +
> 
> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.
> 
> > +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
> > +		if (overlay < 0)
> > +			return overlay;
> > +
> > +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +int fdt_overlay_apply(void *fdt, void *fdto)
> > +{
> > +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
> > +	void *fdto_copy;
> > +	int ret;
> > +
> > +	FDT_CHECK_HEADER(fdt);
> > +	FDT_CHECK_HEADER(fdto);
> > +
> > +	/*
> > +	 * We're going to modify the overlay so that we can apply it.
> > +	 *
> > +	 * Make sure sure we don't destroy the original
> > +	 */
> > +	fdto_copy = malloc(fdt_totalsize(fdto));
> > +	if (!fdto_copy)
> > +		return -FDT_ERR_INTERNAL;
> > +
> > +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
> > +	if (ret)
> > +		goto out;
> > +
> > +	ret = fdt_overlay_merge(fdt, fdto_copy);
> > +
> > +out:
> > +	free(fdto_copy);
> > +	return ret;
> > +}
> 
> I would caution against the liberal use of malloc in libfdt. We’re possibly running in a constrained
> environment; a custom extents based (non freeing) allocator should be better.
> 
> We need some figures about memory consumption when this is enabled, and a CONFIG option to disable it.
> 
> Regards
> 
> — Pantelis
> 
> 

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Pantelis Antoniou]

Hi David,

> On Jun 14, 2016, at 03:25 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> 
> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
>> Hi Maxime,
>> 
>>> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
>>> 
>>> The device tree overlays are a good way to deal with user-modifyable
>>> boards or boards with some kind of an expansion mechanism where we can
>>> easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
>>> 
>>> Add a new function to merge overlays with a base device tree.
>>> 
>>> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
>>> ---
>>> include/libfdt.h         |  30 ++++
>>> lib/libfdt/Makefile      |   2 +-
>>> lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
>>> 3 files changed, 445 insertions(+), 1 deletion(-)
>>> create mode 100644 lib/libfdt/fdt_overlay.c
>>> 
>>> diff --git a/include/libfdt.h b/include/libfdt.h
>>> index 1e01b2c7ebdf..783067e841a1 100644
>>> --- a/include/libfdt.h
>>> +++ b/include/libfdt.h
>>> @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
>>> */
>>> int fdt_del_node(void *fdt, int nodeoffset);
>>> 
>>> +/**
>>> + * fdt_overlay_apply - Applies a DT overlay on a base DT
>>> + * @fdt: pointer to the base device tree blob
>>> + * @fdto: pointer to the device tree overlay blob
>>> + *
>>> + * fdt_overlay_apply() will apply the given device tree overlay on the
>>> + * given base device tree.
>>> + *
>>> + * Expect the base device tree to be modified, even if the function
>>> + * returns an error.
>>> + *
>> 
>> If the base tree has been modified on an error it is unsafe to use it
>> for booting. A valid strategy would be to scribble over the DT magic
>> number so that that blob is invalidated.
>> 
>> What are the other people’s opinion on this?
>> 
>>> + * returns:
>>> + *	0, on success
>>> + *	-FDT_ERR_NOSPACE, there's not enough space in the base device tree
>>> + *	-FDT_ERR_NOTFOUND, the overlay points to some inexistant nodes or
>>> + *		properties in the base DT
>>> + *	-FDT_ERR_BADPHANDLE, the phandles in the overlay do not have the right
>>> + *		magic
>>> + *	-FDT_ERR_INTERNAL,
>>> + *	-FDT_ERR_BADLAYOUT,
>>> + *	-FDT_ERR_BADMAGIC,
>>> + *	-FDT_ERR_BADOFFSET,
>>> + *	-FDT_ERR_BADPATH,
>>> + *	-FDT_ERR_BADVERSION,
>>> + *	-FDT_ERR_BADSTRUCTURE,
>>> + *	-FDT_ERR_BADSTATE,
>>> + *	-FDT_ERR_TRUNCATED, standard meanings
>>> + */
>>> +int fdt_overlay_apply(void *fdt, void *fdto);
>>> +
>>> /**********************************************************************/
>>> /* Debugging / informational functions                                */
>>> /**********************************************************************/
>>> diff --git a/lib/libfdt/Makefile b/lib/libfdt/Makefile
>>> index 934d6142c3e9..4935bf012a75 100644
>>> --- a/lib/libfdt/Makefile
>>> +++ b/lib/libfdt/Makefile
>>> @@ -6,4 +6,4 @@
>>> #
>>> 
>>> obj-y += fdt.o fdt_ro.o fdt_rw.o fdt_strerror.o fdt_sw.o fdt_wip.o \
>>> -	fdt_empty_tree.o fdt_addresses.o fdt_region.o
>>> +	fdt_empty_tree.o fdt_addresses.o fdt_region.o fdt_overlay.o
>>> diff --git a/lib/libfdt/fdt_overlay.c b/lib/libfdt/fdt_overlay.c
>>> new file mode 100644
>>> index 000000000000..1e68e903c734
>>> --- /dev/null
>>> +++ b/lib/libfdt/fdt_overlay.c
>>> @@ -0,0 +1,414 @@
>>> +#include "libfdt_env.h"
>>> +
>>> +#include <fdt.h>
>>> +#include <libfdt.h>
>>> +
>>> +#include "libfdt_internal.h"
>>> +
>>> +static uint32_t fdt_overlay_get_target_phandle(const void *fdto, int node)
>>> +{
>>> +	const uint32_t *val;
>>> +	int len;
>>> +
>>> +	val = fdt_getprop(fdto, node, "target", &len);
>>> +	if (!val || (len != sizeof(*val)))
>>> +		return 0;
>>> +
>>> +	return fdt32_to_cpu(*val);
>>> +}
>>> +
>>> +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
>>> +				  int fragment)
>>> +{
>>> +	uint32_t phandle;
>>> +	const char *path;
>>> +
>>> +	/* Try first to do a phandle based lookup */
>>> +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
>>> +	if (phandle)
>>> +		return fdt_node_offset_by_phandle(fdt, phandle);
>>> +
>>> +	/* And then a path based lookup */
>>> +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
>>> +	if (!path)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	return fdt_path_offset(fdt, path);
>>> +}
>>> +
>> 
>> Those targets are fine; beware there are patches with more target options.
>> 
>>> +static int fdt_overlay_adjust_node_phandles(void *fdto, int node,
>>> +					    uint32_t delta)
>>> +{
>>> +	int property;
>>> +	int child;
>>> +
>>> +	fdt_for_each_property(fdto, node, property) {
>>> +		const uint32_t *val;
>>> +		const char *name;
>>> +		uint32_t adj_val;
>>> +		int len;
>>> +		int ret;
>>> +
>>> +		val = fdt_getprop_by_offset(fdto, property,
>>> +					    &name, &len);
>>> +		if (!val || (len != sizeof(*val)))
>> 
>> Superfluous parentheses.
>> 
>>> +			continue;
>>> +
>>> +		if (strcmp(name, "phandle") && strcmp(name, "linux,phandle"))
>>> +			continue;
>>> +
>>> +		adj_val = fdt32_to_cpu(*val);
>>> +		adj_val += delta;
>>> +		ret = fdt_setprop_inplace_u32(fdto, node, name, adj_val);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	fdt_for_each_subnode(fdto, child, node)
>>> +		fdt_overlay_adjust_node_phandles(fdto, child, delta);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_adjust_local_phandles(void *fdto, uint32_t delta)
>>> +{
>>> +	int root;
>>> +
>>> +	root = fdt_path_offset(fdto, "/");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	return fdt_overlay_adjust_node_phandles(fdto, root, delta);
>>> +}
>>> +
>>> +static int _fdt_overlay_update_local_references(void *fdto,
>>> +						int tree_node,
>>> +						int fixup_node,
>>> +						uint32_t delta)
>>> +{
>>> +	int fixup_prop;
>>> +	int fixup_child;
>>> +
>>> +	fdt_for_each_property(fdto, fixup_node, fixup_prop) {
>>> +		const char *fixup_name, *tree_name;
>>> +		const uint32_t *val = NULL;
>>> +		uint32_t adj_val;
>>> +		int fixup_len;
>>> +		int tree_prop;
>>> +		int tree_len;
>>> +		int ret;
>>> +
>>> +		fdt_getprop_by_offset(fdto, fixup_prop,
>>> +				      &fixup_name, &fixup_len);
>>> +
>>> +		if (!strcmp(fixup_name, "phandle") ||
>>> +		    !strcmp(fixup_name, "linux,phandle"))
>>> +			continue;
>>> +
>>> +		fdt_for_each_property(fdto, tree_node, tree_prop) {
>>> +			val = fdt_getprop_by_offset(fdto, tree_prop,
>>> +						    &tree_name, &tree_len);
>>> +
>>> +			if (!strcmp(tree_name, fixup_name))
>>> +				break;
>>> +		}
>>> +
>>> +		if (!val || !tree_len)
>>> +			return -FDT_ERR_NOTFOUND;
>>> +
>>> +		if (!tree_name)
>>> +			return -FDT_ERR_NOTFOUND;
>>> +
>>> +		if (tree_len != 4)
>>> +			return -FDT_ERR_NOTFOUND;
>>> +
>>> +		adj_val = fdt32_to_cpu(*val);
>>> +		adj_val += delta;
>>> +		ret = fdt_setprop_inplace_u32(fdto, tree_node, fixup_name,
>>> +					      adj_val);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	fdt_for_each_subnode(fdto, fixup_child, fixup_node) {
>>> +		const char *fixup_child_name = fdt_get_name(fdto,
>>> +							    fixup_child, NULL);
>>> +		int tree_child;
>>> +
>>> +		fdt_for_each_subnode(fdto, tree_child, tree_node) {
>>> +			const char *tree_child_name = fdt_get_name(fdto,
>>> +								   tree_child,
>>> +								   NULL);
>>> +
>>> +			if (!strcmp(fixup_child_name, tree_child_name))
>>> +				break;
>>> +		}
>>> +
>>> +		_fdt_overlay_update_local_references(fdto,
>>> +						     tree_child,
>>> +						     fixup_child,
>>> +						     delta);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_update_local_references(void *dto, uint32_t delta)
>>> +{
>>> +	int root, fixups;
>>> +
>>> +	root = fdt_path_offset(dto, "/");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	fixups = fdt_path_offset(dto, "/__local_fixups__");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	return _fdt_overlay_update_local_references(dto, root, fixups,
>>> +						    delta);
>>> +}
>>> +
>>> +static int _fdt_overlay_fixup_phandle(void *fdt, void *fdto,
>>> +				      int symbols_off,
>>> +				      const char *path, const char *name,
>>> +				      int index, const char *label)
>>> +{
>>> +	const uint32_t *prop_val;
>>> +	const char *symbol_path;
>>> +	uint32_t *fixup_val;
>>> +	uint32_t phandle;
>>> +	int symbol_off, fixup_off;
>>> +	int prop_len;
>>> +	int ret;
>>> +
>>> +	symbol_path = fdt_getprop(fdt, symbols_off, label,
>>> +				  &prop_len);
>>> +	if (!symbol_path)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	symbol_off = fdt_path_offset(fdt, symbol_path);
>>> +	if (symbol_off < 0)
>>> +		return symbol_off;
>>> +
>>> +	phandle = fdt_get_phandle(fdt, symbol_off);
>>> +	if (!phandle)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	fixup_off = fdt_path_offset(fdto, path);
>>> +	if (fixup_off < 0)
>>> +		return fixup_off;
>>> +
>>> +	prop_val = fdt_getprop(fdto, fixup_off, name,
>>> +			       &prop_len);
>>> +	if (!prop_val)
>>> +		return -FDT_ERR_NOTFOUND;
>>> +
>>> +	fixup_val = malloc(prop_len);
>>> +	if (!fixup_val)
>>> +		return -FDT_ERR_INTERNAL;
>>> +	memcpy(fixup_val, prop_val, prop_len);
>>> +
>>> +	if (fdt32_to_cpu(fixup_val[index]) != 0xdeadbeef) {
>>> +		ret = -FDT_ERR_BADPHANDLE;
>>> +		goto out;
>>> +	}
>>> +
>>> +	fixup_val[index] = cpu_to_fdt32(phandle);
>>> +	ret = fdt_setprop_inplace(fdto, fixup_off,
>>> +				  name, fixup_val,
>>> +				  prop_len);
>>> +
>>> +out:
>>> +	free(fixup_val);
>>> +	return ret;
>>> +};
>>> +
>>> +static int fdt_overlay_fixup_phandle(void *fdt, void *fdto, int symbols_off,
>>> +				     int property)
>>> +{
>>> +	const char *value, *tmp_value;
>>> +	const char *label;
>>> +	int tmp_len, len, next;
>>> +	int table = 0;
>>> +	int i;
>>> +
>>> +	value = fdt_getprop_by_offset(fdto, property,
>>> +				      &label, &len);
>>> +	tmp_value = value;
>>> +	tmp_len = len;
>>> +
>>> +	do {
>>> +		next = strlen(tmp_value) + 1;
>>> +		tmp_len -= next;
>>> +		tmp_value += next;
>>> +		table++;
>>> +	} while (tmp_len > 0);
>>> +
>>> +	for (i = 0; i < table; i++) {
>>> +		const char *prop_string = value;
>>> +		const char *path, *name;
>>> +		char *prop_cpy, *prop_tmp;
>>> +		int index, stlen;
>>> +		char *sep;
>>> +
>>> +		stlen = strlen(prop_string);
>>> +		prop_cpy = malloc(stlen + 1);
>>> +		if (!prop_cpy)
>>> +			return -FDT_ERR_INTERNAL;
>>> +
>>> +		strncpy(prop_cpy, prop_string, stlen);
>>> +		prop_cpy[stlen] = '\0';
>>> +
>>> +		for (prop_tmp = prop_cpy; (sep = strchr(prop_tmp, ':'));
>>> +		     prop_tmp += ((sep - prop_tmp) + 1))
>>> +			*sep = '\0';
>>> +
>>> +		prop_tmp = prop_cpy;
>>> +		path = prop_tmp;
>>> +		prop_tmp += strlen(prop_tmp) + 1;
>>> +
>>> +		name = prop_tmp;
>>> +		prop_tmp += strlen(prop_tmp) + 1;
>>> +
>>> +		index = strtol(prop_tmp, NULL, 10);
>>> +		prop_tmp += strlen(prop_tmp) + 1;
>>> +
>>> +		value += stlen + 1;
>>> +		len -= stlen + 1;
>>> +
>>> +		_fdt_overlay_fixup_phandle(fdt, fdto, symbols_off,
>>> +					   path, name, index, label);
>>> +
>>> +		free(prop_cpy);
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_fixup_phandles(void *dt, void *dto)
>>> +{
>>> +	int fixups_off, symbols_off;
>>> +	int property;
>>> +
>>> +	symbols_off = fdt_path_offset(dt, "/__symbols__");
>>> +	fixups_off = fdt_path_offset(dto, "/__fixups__");
>>> +
>>> +	fdt_for_each_property(dto, fixups_off, property)
>>> +		fdt_overlay_fixup_phandle(dt, dto, symbols_off, property);
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_apply_overlay_node(void *dt, void *dto,
>>> +				  int target, int overlay)
>>> +{
>>> +	int property;
>>> +	int node;
>>> +
>>> +	fdt_for_each_property(dto, overlay, property) {
>>> +		const char *name;
>>> +		const void *prop;
>>> +		int prop_len;
>>> +		int ret;
>>> +
>>> +		prop = fdt_getprop_by_offset(dto, property, &name,
>>> +					     &prop_len);
>>> +		if (!prop)
>>> +			return -FDT_ERR_INTERNAL;
>>> +
>>> +		ret = fdt_setprop(dt, target, name, prop, prop_len);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	fdt_for_each_subnode(dto, node, overlay) {
>>> +		const char *name = fdt_get_name(dto, node, NULL);
>>> +		int nnode;
>>> +		int ret;
>>> +
>>> +		nnode = fdt_add_subnode(dt, target, name);
>>> +		if (nnode < 0)
>>> +			return nnode;
>>> +
>>> +		ret = fdt_apply_overlay_node(dt, dto, nnode, node);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>> +{
>>> +	int root, fragment;
>>> +
>>> +	root = fdt_path_offset(dto, "/");
>>> +	if (root < 0)
>>> +		return root;
>>> +
>>> +	fdt_for_each_subnode(dto, fragment, root) {
>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
>>> +		uint32_t target;
>>> +		int overlay;
>>> +		int ret;
>>> +
>>> +		if (strncmp(name, "fragment", 8))
>>> +			continue;
>>> +
>> 
>> This is incorrect. The use of “fragment” is a convention only.
>> The real test whether the node is an overlay fragment is that
>> it contains a target property.
> 
> Hmm.. I dislike that approach.  First, it means that if new target
> types are introduced in future, older code is likely to silently
> ignore such fragments instead of realizing that it doesn't know how to
> apply themm.  Second, it raises weird issues if some node down within
> a fragment also happens to have a property called "target”.

Not really. If new targets are introduced then the fragment will be ignored.

We can have an argument about what is better to do (report an error or 
ignore a fragment) but what it comes down to is that that applicator
does not know how to handle the new target method.

There is no issues with any target properties inside a fragment because
the check is not made recursively.
 
> 

> 
>>> +		target = fdt_overlay_get_target(dt, dto, fragment);
>>> +		if (target < 0)
>>> +			return target;
>>> +
>> 
>> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.
>> 
>>> +		overlay = fdt_subnode_offset(dto, fragment, "__overlay__");
>>> +		if (overlay < 0)
>>> +			return overlay;
>>> +
>>> +		ret = fdt_apply_overlay_node(dt, dto, target, overlay);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>> +int fdt_overlay_apply(void *fdt, void *fdto)
>>> +{
>>> +	uint32_t delta = fdt_get_max_phandle(fdt) + 1;
>>> +	void *fdto_copy;
>>> +	int ret;
>>> +
>>> +	FDT_CHECK_HEADER(fdt);
>>> +	FDT_CHECK_HEADER(fdto);
>>> +
>>> +	/*
>>> +	 * We're going to modify the overlay so that we can apply it.
>>> +	 *
>>> +	 * Make sure sure we don't destroy the original
>>> +	 */
>>> +	fdto_copy = malloc(fdt_totalsize(fdto));
>>> +	if (!fdto_copy)
>>> +		return -FDT_ERR_INTERNAL;
>>> +
>>> +	ret = fdt_move(fdto, fdto_copy, fdt_totalsize(fdto));
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_adjust_local_phandles(fdto_copy, delta);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_update_local_references(fdto_copy, delta);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_fixup_phandles(fdt, fdto_copy);
>>> +	if (ret)
>>> +		goto out;
>>> +
>>> +	ret = fdt_overlay_merge(fdt, fdto_copy);
>>> +
>>> +out:
>>> +	free(fdto_copy);
>>> +	return ret;
>>> +}
>> 
>> I would caution against the liberal use of malloc in libfdt. We’re possibly running in a constrained
>> environment; a custom extents based (non freeing) allocator should be better.
>> 
>> We need some figures about memory consumption when this is enabled, and a CONFIG option to disable it.
>> 
>> Regards
>> 
>> — Pantelis
>> 
>> 
> 

Regards

— Pantelis

> -- 
> 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 v2 7/9] libfdt: Add overlay application function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 2505 bytes --]

On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
> Hi David,
> > On Jun 14, 2016, at 03:25 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
[snip]
> >>> +static int fdt_overlay_merge(void *dt, void *dto)
> >>> +{
> >>> +	int root, fragment;
> >>> +
> >>> +	root = fdt_path_offset(dto, "/");
> >>> +	if (root < 0)
> >>> +		return root;
> >>> +
> >>> +	fdt_for_each_subnode(dto, fragment, root) {
> >>> +		const char *name = fdt_get_name(dto, fragment, NULL);
> >>> +		uint32_t target;
> >>> +		int overlay;
> >>> +		int ret;
> >>> +
> >>> +		if (strncmp(name, "fragment", 8))
> >>> +			continue;
> >>> +
> >> 
> >> This is incorrect. The use of “fragment” is a convention only.
> >> The real test whether the node is an overlay fragment is that
> >> it contains a target property.
> > 
> > Hmm.. I dislike that approach.  First, it means that if new target
> > types are introduced in future, older code is likely to silently
> > ignore such fragments instead of realizing that it doesn't know how to
> > apply themm.  Second, it raises weird issues if some node down within
> > a fragment also happens to have a property called "target”.
> 
> Not really. If new targets are introduced then the fragment will be ignored.

Yes.. and that's bad.

> We can have an argument about what is better to do (report an error or 
> ignore a fragment) but what it comes down to is that that applicator
> does not know how to handle the new target method.

Sure, let's have the argument.  The overlay is constructed on the
assumption that all the pieces will be applied, or none of them.  A
silent, partial application is an awful, awful failure mode.  We
absolutely should report an error, and in order to do so we need to
know what are applicable fragments, whether or not we understand the
target designation (or any other meta-data the fragment has).

Given what's established so far, checking the name seems the obvious
way to do that.

> There is no issues with any target properties inside a fragment because
> the check is not made recursively.

Ok, so the real test you're proposing is "at the top level AND has a
target property".

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Pantelis Antoniou]

Hi David,

> On Jun 15, 2016, at 06:14 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>>> On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> [snip]
>>>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>>>> +{
>>>>> +	int root, fragment;
>>>>> +
>>>>> +	root = fdt_path_offset(dto, "/");
>>>>> +	if (root < 0)
>>>>> +		return root;
>>>>> +
>>>>> +	fdt_for_each_subnode(dto, fragment, root) {
>>>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
>>>>> +		uint32_t target;
>>>>> +		int overlay;
>>>>> +		int ret;
>>>>> +
>>>>> +		if (strncmp(name, "fragment", 8))
>>>>> +			continue;
>>>>> +
>>>> 
>>>> This is incorrect. The use of “fragment” is a convention only.
>>>> The real test whether the node is an overlay fragment is that
>>>> it contains a target property.
>>> 
>>> Hmm.. I dislike that approach.  First, it means that if new target
>>> types are introduced in future, older code is likely to silently
>>> ignore such fragments instead of realizing that it doesn't know how to
>>> apply themm.  Second, it raises weird issues if some node down within
>>> a fragment also happens to have a property called "target”.
>> 
>> Not really. If new targets are introduced then the fragment will be ignored.
> 
> Yes.. and that's bad.
> 

That’s arguable.

>> We can have an argument about what is better to do (report an error or 
>> ignore a fragment) but what it comes down to is that that applicator
>> does not know how to handle the new target method.
> 
> Sure, let's have the argument.  The overlay is constructed on the
> assumption that all the pieces will be applied, or none of them.  A
> silent, partial application is an awful, awful failure mode.  We
> absolutely should report an error, and in order to do so we need to
> know what are applicable fragments, whether or not we understand the
> target designation (or any other meta-data the fragment has).
> 

This way also allows having nodes being something other than fragments.

So instead of being painted into a corner (all subnodes that are not
named ‘fragment@X’ are errors), we have flexibility in introducing
nodes that contain configuration data for the loader.
 
> Given what's established so far, checking the name seems the obvious
> way to do that.
> 

Again, it’s arguable. Stricter checking against future-proofing.

>> There is no issues with any target properties inside a fragment because
>> the check is not made recursively.
> 
> Ok, so the real test you're proposing is "at the top level AND has a
> target property”.

Yes

> 

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

Regards

— Pantelis

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

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 3698 bytes --]

On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote:
> Hi David,
> 
> > On Jun 15, 2016, at 06:14 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> > 
> > On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
> >> Hi David,
> >>> On Jun 14, 2016, at 03:25 , David Gibson <david-xT8FGy+AXnRB3Ne2BGzF6laj5H9X9Tb+@public.gmane.org> wrote:
> >>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> > [snip]
> >>>>> +static int fdt_overlay_merge(void *dt, void *dto)
> >>>>> +{
> >>>>> +	int root, fragment;
> >>>>> +
> >>>>> +	root = fdt_path_offset(dto, "/");
> >>>>> +	if (root < 0)
> >>>>> +		return root;
> >>>>> +
> >>>>> +	fdt_for_each_subnode(dto, fragment, root) {
> >>>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
> >>>>> +		uint32_t target;
> >>>>> +		int overlay;
> >>>>> +		int ret;
> >>>>> +
> >>>>> +		if (strncmp(name, "fragment", 8))
> >>>>> +			continue;
> >>>>> +
> >>>> 
> >>>> This is incorrect. The use of “fragment” is a convention only.
> >>>> The real test whether the node is an overlay fragment is that
> >>>> it contains a target property.
> >>> 
> >>> Hmm.. I dislike that approach.  First, it means that if new target
> >>> types are introduced in future, older code is likely to silently
> >>> ignore such fragments instead of realizing that it doesn't know how to
> >>> apply themm.  Second, it raises weird issues if some node down within
> >>> a fragment also happens to have a property called "target”.
> >> 
> >> Not really. If new targets are introduced then the fragment will be ignored.
> > 
> > Yes.. and that's bad.
> 
> That’s arguable.

!?!  No, really, silent partial application is just horrible.

> >> We can have an argument about what is better to do (report an error or 
> >> ignore a fragment) but what it comes down to is that that applicator
> >> does not know how to handle the new target method.
> > 
> > Sure, let's have the argument.  The overlay is constructed on the
> > assumption that all the pieces will be applied, or none of them.  A
> > silent, partial application is an awful, awful failure mode.  We
> > absolutely should report an error, and in order to do so we need to
> > know what are applicable fragments, whether or not we understand the
> > target designation (or any other meta-data the fragment has).
> 
> This way also allows having nodes being something other than fragments.
> 
> So instead of being painted into a corner (all subnodes that are not
> named ‘fragment@X’ are errors), we have flexibility in introducing
> nodes that contain configuration data for the loader.

There's no significant difference between the approaches from this
point of view.  Metadata nodes are certainly possible (we already have
__symbols__ and __fixups__) but calling them something other than
fragment@ is no harder than leaving off the target property.  In fact
even if it was workable, calling new metadata nodes fragment@ would be
stupidly confusing.

> > Given what's established so far, checking the name seems the obvious
> > way to do that.
> > 
> 
> Again, it’s arguable. Stricter checking against future-proofing.
> 
> >> There is no issues with any target properties inside a fragment because
> >> the check is not made recursively.
> > 
> > Ok, so the real test you're proposing is "at the top level AND has a
> > target property”.
> 
> Yes

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Pantelis Antoniou]

Hi David,

> On Jun 15, 2016, at 13:19 , David Gibson <david@gibson.dropbear.id.au> wrote:
> 
> On Wed, Jun 15, 2016 at 12:34:00PM +0300, Pantelis Antoniou wrote:
>> Hi David,
>> 
>>> On Jun 15, 2016, at 06:14 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>> 
>>> On Tue, Jun 14, 2016 at 12:22:23PM +0300, Pantelis Antoniou wrote:
>>>> Hi David,
>>>>> On Jun 14, 2016, at 03:25 , David Gibson <david@gibson.dropbear.id.au> wrote:
>>>>> On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
>>> [snip]
>>>>>>> +static int fdt_overlay_merge(void *dt, void *dto)
>>>>>>> +{
>>>>>>> +	int root, fragment;
>>>>>>> +
>>>>>>> +	root = fdt_path_offset(dto, "/");
>>>>>>> +	if (root < 0)
>>>>>>> +		return root;
>>>>>>> +
>>>>>>> +	fdt_for_each_subnode(dto, fragment, root) {
>>>>>>> +		const char *name = fdt_get_name(dto, fragment, NULL);
>>>>>>> +		uint32_t target;
>>>>>>> +		int overlay;
>>>>>>> +		int ret;
>>>>>>> +
>>>>>>> +		if (strncmp(name, "fragment", 8))
>>>>>>> +			continue;
>>>>>>> +
>>>>>> 
>>>>>> This is incorrect. The use of “fragment” is a convention only.
>>>>>> The real test whether the node is an overlay fragment is that
>>>>>> it contains a target property.
>>>>> 
>>>>> Hmm.. I dislike that approach.  First, it means that if new target
>>>>> types are introduced in future, older code is likely to silently
>>>>> ignore such fragments instead of realizing that it doesn't know how to
>>>>> apply themm.  Second, it raises weird issues if some node down within
>>>>> a fragment also happens to have a property called "target”.
>>>> 
>>>> Not really. If new targets are introduced then the fragment will be ignored.
>>> 
>>> Yes.. and that's bad.
>> 
>> That’s arguable.
> 
> !?!  No, really, silent partial application is just horrible.
> 
>>>> We can have an argument about what is better to do (report an error or 
>>>> ignore a fragment) but what it comes down to is that that applicator
>>>> does not know how to handle the new target method.
>>> 
>>> Sure, let's have the argument.  The overlay is constructed on the
>>> assumption that all the pieces will be applied, or none of them.  A
>>> silent, partial application is an awful, awful failure mode.  We
>>> absolutely should report an error, and in order to do so we need to
>>> know what are applicable fragments, whether or not we understand the
>>> target designation (or any other meta-data the fragment has).
>> 
>> This way also allows having nodes being something other than fragments.
>> 
>> So instead of being painted into a corner (all subnodes that are not
>> named ‘fragment@X’ are errors), we have flexibility in introducing
>> nodes that contain configuration data for the loader.
> 
> There's no significant difference between the approaches from this
> point of view.  Metadata nodes are certainly possible (we already have
> __symbols__ and __fixups__) but calling them something other than
> fragment@ is no harder than leaving off the target property.  In fact
> even if it was workable, calling new metadata nodes fragment@ would be
> stupidly confusing.
> 

Err, we won’t ever call metadata nodes fragment@, that would be awfully confusing.

We’re bikeshedding, it’s just a philosophical argument right now.

The correct way is to get cracking on the machine readable yaml based bindings
and enforce it through there.

>>> Given what's established so far, checking the name seems the obvious
>>> way to do that.
>>> 
>> 
>> Again, it’s arguable. Stricter checking against future-proofing.
>> 
>>>> There is no issues with any target properties inside a fragment because
>>>> the check is not made recursively.
>>> 
>>> Ok, so the real test you're proposing is "at the top level AND has a
>>> target property”.
>> 
>> Yes
> 
> -- 
> 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

Regards

— Pantelis

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

Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Warner Losh]

> On Jun 15, 2016, at 4:23 AM, Pantelis Antoniou <pantelis.antoniou@konsulko.com> wrote:

> The correct way is to get cracking on the machine readable yaml based bindings
> and enforce it through there.

Machine readable bindings with enforcement would be enabling technology to solve
a lot of different problems (both technical and non-technical). Coupled with overlays
(whatever philosophy wins and I hope one wins soon), these things will greatly improve
the flexibility of the currently somewhat rigid DTB…

Warner--
To unsubscribe from this list: send the line "unsubscribe devicetree-compiler" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 8/9] cmd: fdt: add fdt overlay application subcommand

[Maxime Ripard]

The device tree overlays are a good way to deal with user-modifyable
boards or boards with some kind of an expansion mechanism where we can
easily plug new board in (like the BBB or the raspberry pi).

However, so far, the usual mechanism to deal with it was to have in Linux
some driver detecting the expansion boards plugged in and then request
these overlays using the firmware interface.

That works in most cases, but in some cases, you might want to have the
overlays applied before the userspace comes in. Either because the new
board requires some kind of an early initialization, or because your root
filesystem is accessed through that expansion board.

The easiest solution in such a case is to simply have the component before
Linux applying that overlay, removing all these drawbacks.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 cmd/fdt.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/cmd/fdt.c b/cmd/fdt.c
index 0f5923e75a41..6cbc9e525d6c 100644
--- a/cmd/fdt.c
+++ b/cmd/fdt.c
@@ -639,6 +639,25 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 #endif
 
 	}
+	/* apply an overlay */
+	else if (strncmp(argv[1], "ap", 2) == 0) {
+		unsigned long addr;
+		struct fdt_header *blob;
+
+		if (argc != 3)
+			return CMD_RET_USAGE;
+
+		if (!working_fdt)
+			return CMD_RET_FAILURE;
+
+		addr = simple_strtoul(argv[2], NULL, 16);
+		blob = map_sysmem(addr, 0);
+		if (!fdt_valid(&blob))
+			return CMD_RET_FAILURE;
+
+		if (fdt_overlay_apply(working_fdt, blob))
+			return CMD_RET_FAILURE;
+	}
 	/* resize the fdt */
 	else if (strncmp(argv[1], "re", 2) == 0) {
 		fdt_shrink_to_minimum(working_fdt);
@@ -1025,6 +1044,7 @@ static int fdt_print(const char *pathp, char *prop, int depth)
 #ifdef CONFIG_SYS_LONGHELP
 static char fdt_help_text[] =
 	"addr [-c]  <addr> [<length>]   - Set the [control] fdt location to <addr>\n"
+	"fdt apply <addr>                    - Apply overlay to the DT\n"
 #ifdef CONFIG_OF_BOARD_SETUP
 	"fdt boardsetup                      - Do board-specific set up\n"
 #endif
-- 
2.8.2

Re: [PATCH v2 8/9] cmd: fdt: add fdt overlay application subcommand

[Simon Glass]

Hi Maxime,

On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> The device tree overlays are a good way to deal with user-modifyable

nit: modifiable

> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB or the raspberry pi).
>
> However, so far, the usual mechanism to deal with it was to have in Linux
> some driver detecting the expansion boards plugged in and then request
> these overlays using the firmware interface.
>
> That works in most cases, but in some cases, you might want to have the
> overlays applied before the userspace comes in. Either because the new
> board requires some kind of an early initialization, or because your root
> filesystem is accessed through that expansion board.
>
> The easiest solution in such a case is to simply have the component before
> Linux applying that overlay, removing all these drawbacks.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  cmd/fdt.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 8/9] cmd: fdt: add fdt overlay application subcommand

[Stefan Agner]

On 2016-05-27 02:13, Maxime Ripard wrote:
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB or the raspberry pi).
> 
> However, so far, the usual mechanism to deal with it was to have in Linux
> some driver detecting the expansion boards plugged in and then request
> these overlays using the firmware interface.
> 
> That works in most cases, but in some cases, you might want to have the
> overlays applied before the userspace comes in. Either because the new
> board requires some kind of an early initialization, or because your root
> filesystem is accessed through that expansion board.
> 
> The easiest solution in such a case is to simply have the component before
> Linux applying that overlay, removing all these drawbacks.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  cmd/fdt.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)

Reviewed-by: Stefan Agner <stefan-XLVq0VzYD2Y@public.gmane.org>

> 
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index 0f5923e75a41..6cbc9e525d6c 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -639,6 +639,25 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int
> argc, char * const argv[])
>  #endif
>  
>  	}
> +	/* apply an overlay */
> +	else if (strncmp(argv[1], "ap", 2) == 0) {
> +		unsigned long addr;
> +		struct fdt_header *blob;
> +
> +		if (argc != 3)
> +			return CMD_RET_USAGE;
> +
> +		if (!working_fdt)
> +			return CMD_RET_FAILURE;
> +
> +		addr = simple_strtoul(argv[2], NULL, 16);
> +		blob = map_sysmem(addr, 0);
> +		if (!fdt_valid(&blob))
> +			return CMD_RET_FAILURE;
> +
> +		if (fdt_overlay_apply(working_fdt, blob))
> +			return CMD_RET_FAILURE;
> +	}
>  	/* resize the fdt */
>  	else if (strncmp(argv[1], "re", 2) == 0) {
>  		fdt_shrink_to_minimum(working_fdt);
> @@ -1025,6 +1044,7 @@ static int fdt_print(const char *pathp, char
> *prop, int depth)
>  #ifdef CONFIG_SYS_LONGHELP
>  static char fdt_help_text[] =
>  	"addr [-c]  <addr> [<length>]   - Set the [control] fdt location to <addr>\n"
> +	"fdt apply <addr>                    - Apply overlay to the DT\n"
>  #ifdef CONFIG_OF_BOARD_SETUP
>  	"fdt boardsetup                      - Do board-specific set up\n"
>  #endif

Re: [PATCH v2 8/9] cmd: fdt: add fdt overlay application subcommand

[Pantelis Antoniou]

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> The device tree overlays are a good way to deal with user-modifyable
> boards or boards with some kind of an expansion mechanism where we can
> easily plug new board in (like the BBB or the raspberry pi).
> 
> However, so far, the usual mechanism to deal with it was to have in Linux
> some driver detecting the expansion boards plugged in and then request
> these overlays using the firmware interface.
> 
> That works in most cases, but in some cases, you might want to have the
> overlays applied before the userspace comes in. Either because the new
> board requires some kind of an early initialization, or because your root
> filesystem is accessed through that expansion board.
> 
> The easiest solution in such a case is to simply have the component before
> Linux applying that overlay, removing all these drawbacks.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> cmd/fdt.c | 20 ++++++++++++++++++++
> 1 file changed, 20 insertions(+)
> 
> diff --git a/cmd/fdt.c b/cmd/fdt.c
> index 0f5923e75a41..6cbc9e525d6c 100644
> --- a/cmd/fdt.c
> +++ b/cmd/fdt.c
> @@ -639,6 +639,25 @@ static int do_fdt(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> #endif
> 
> 	}
> +	/* apply an overlay */
> +	else if (strncmp(argv[1], "ap", 2) == 0) {
> +		unsigned long addr;
> +		struct fdt_header *blob;
> +
> +		if (argc != 3)
> +			return CMD_RET_USAGE;
> +
> +		if (!working_fdt)
> +			return CMD_RET_FAILURE;
> +
> +		addr = simple_strtoul(argv[2], NULL, 16);
> +		blob = map_sysmem(addr, 0);
> +		if (!fdt_valid(&blob))
> +			return CMD_RET_FAILURE;
> +
> +		if (fdt_overlay_apply(working_fdt, blob))
> +			return CMD_RET_FAILURE;
> +	}
> 	/* resize the fdt */
> 	else if (strncmp(argv[1], "re", 2) == 0) {
> 		fdt_shrink_to_minimum(working_fdt);
> @@ -1025,6 +1044,7 @@ static int fdt_print(const char *pathp, char *prop, int depth)
> #ifdef CONFIG_SYS_LONGHELP
> static char fdt_help_text[] =
> 	"addr [-c]  <addr> [<length>]   - Set the [control] fdt location to <addr>\n"
> +	"fdt apply <addr>                    - Apply overlay to the DT\n"
> #ifdef CONFIG_OF_BOARD_SETUP
> 	"fdt boardsetup                      - Do board-specific set up\n"
> #endif
> -- 
> 2.8.2
> 

Acked-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>

[PATCH v2 9/9] tests: Introduce DT overlay tests

[Maxime Ripard]

This adds a bunch of unit tests for the "fdt apply" command.

They've all been run successfully in the sandbox. However, as you still
require an out-of-tree dtc with overlay support, this is disabled by
default.

Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
---
 Makefile                          |   1 +
 include/test/overlay.h            |  16 ++++
 include/test/suites.h             |   1 +
 test/Kconfig                      |   1 +
 test/cmd_ut.c                     |   6 ++
 test/overlay/Kconfig              |  10 +++
 test/overlay/Makefile             |  15 ++++
 test/overlay/cmd_ut_overlay.c     | 176 ++++++++++++++++++++++++++++++++++++++
 test/overlay/test-fdt-base.dts    |  17 ++++
 test/overlay/test-fdt-overlay.dts |  60 +++++++++++++
 10 files changed, 303 insertions(+)
 create mode 100644 include/test/overlay.h
 create mode 100644 test/overlay/Kconfig
 create mode 100644 test/overlay/Makefile
 create mode 100644 test/overlay/cmd_ut_overlay.c
 create mode 100644 test/overlay/test-fdt-base.dts
 create mode 100644 test/overlay/test-fdt-overlay.dts

diff --git a/Makefile b/Makefile
index 954a865381af..71272501a75e 100644
--- a/Makefile
+++ b/Makefile
@@ -665,6 +665,7 @@ libs-$(CONFIG_HAS_POST) += post/
 libs-y += test/
 libs-y += test/dm/
 libs-$(CONFIG_UT_ENV) += test/env/
+libs-$(CONFIG_UT_OVERLAY) += test/overlay/
 
 libs-y += $(if $(BOARDDIR),board/$(BOARDDIR)/)
 
diff --git a/include/test/overlay.h b/include/test/overlay.h
new file mode 100644
index 000000000000..392f28ff8405
--- /dev/null
+++ b/include/test/overlay.h
@@ -0,0 +1,16 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#ifndef __TEST_OVERLAY_H__
+#define __TEST_OVERLAY_H__
+
+#include <test/test.h>
+
+/* Declare a new environment test */
+#define OVERLAY_TEST(_name, _flags)	UNIT_TEST(_name, _flags, overlay_test)
+
+#endif /* __TEST_OVERLAY_H__ */
diff --git a/include/test/suites.h b/include/test/suites.h
index f5790333ff8e..0e94feb07a79 100644
--- a/include/test/suites.h
+++ b/include/test/suites.h
@@ -10,6 +10,7 @@
 
 int do_ut_dm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_env(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
+int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 int do_ut_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
 
 #endif /* __TEST_SUITES_H__ */
diff --git a/test/Kconfig b/test/Kconfig
index d71c332eee27..3643761bc6ef 100644
--- a/test/Kconfig
+++ b/test/Kconfig
@@ -17,3 +17,4 @@ config UT_TIME
 
 source "test/dm/Kconfig"
 source "test/env/Kconfig"
+source "test/overlay/Kconfig"
diff --git a/test/cmd_ut.c b/test/cmd_ut.c
index f6e1f413db7f..14333423a178 100644
--- a/test/cmd_ut.c
+++ b/test/cmd_ut.c
@@ -19,6 +19,9 @@ static cmd_tbl_t cmd_ut_sub[] = {
 #if defined(CONFIG_UT_ENV)
 	U_BOOT_CMD_MKENT(env, CONFIG_SYS_MAXARGS, 1, do_ut_env, "", ""),
 #endif
+#ifdef CONFIG_UT_OVERLAY
+	U_BOOT_CMD_MKENT(overlay, CONFIG_SYS_MAXARGS, 1, do_ut_overlay, "", ""),
+#endif
 #ifdef CONFIG_UT_TIME
 	U_BOOT_CMD_MKENT(time, CONFIG_SYS_MAXARGS, 1, do_ut_time, "", ""),
 #endif
@@ -68,6 +71,9 @@ static char ut_help_text[] =
 #ifdef CONFIG_UT_ENV
 	"ut env [test-name]\n"
 #endif
+#ifdef CONFIG_UT_OVERLAY
+	"ut overlay [test-name]\n"
+#endif
 #ifdef CONFIG_UT_TIME
 	"ut time - Very basic test of time functions\n"
 #endif
diff --git a/test/overlay/Kconfig b/test/overlay/Kconfig
new file mode 100644
index 000000000000..b8e01a934373
--- /dev/null
+++ b/test/overlay/Kconfig
@@ -0,0 +1,10 @@
+config UT_OVERLAY
+	bool "Enable Device Tree Overlays Unit Tests"
+	depends on UNIT_TEST
+	help
+	  This enables the 'ut overlay' command which runs a series of unit
+	  tests on the fdt overlay code.
+	  If all is well then all tests pass although there will be a few
+	  messages printed along the way.
+	  Be warned that it requires an out-of-tree dtc compiler with patches
+	  to support the DT overlays, otherwise it will fail.
diff --git a/test/overlay/Makefile b/test/overlay/Makefile
new file mode 100644
index 000000000000..907f08544619
--- /dev/null
+++ b/test/overlay/Makefile
@@ -0,0 +1,15 @@
+#
+# Copyright (c) 2016 NextThing Co
+# Copyright (c) 2016 Free Electrons
+#
+# SPDX-License-Identifier:	GPL-2.0+
+#
+
+# Test files
+obj-y += cmd_ut_overlay.o
+
+DTC_FLAGS += -@
+
+# DT overlays
+obj-y += test-fdt-base.dtb.o
+obj-y += test-fdt-overlay.dtb.o
diff --git a/test/overlay/cmd_ut_overlay.c b/test/overlay/cmd_ut_overlay.c
new file mode 100644
index 000000000000..3ff1d61b7e31
--- /dev/null
+++ b/test/overlay/cmd_ut_overlay.c
@@ -0,0 +1,176 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+#include <common.h>
+#include <command.h>
+#include <errno.h>
+#include <malloc.h>
+
+#include <linux/sizes.h>
+
+#include <test/ut.h>
+#include <test/overlay.h>
+
+/* 4k ought to be enough for anybody */
+#define FDT_COPY_SIZE	(4 * SZ_1K)
+
+extern u32 __dtb_test_fdt_base_begin;
+extern u32 __dtb_test_fdt_overlay_begin;
+
+static int fdt_getprop_u32(void *fdt, const char *path, const char *name,
+			   u32 *out)
+{
+	const fdt32_t *val;
+	int node_off;
+	int len;
+
+	node_off = fdt_path_offset(fdt, path);
+	if (node_off < 0)
+		return node_off;
+
+	val = fdt_getprop(fdt, node_off, name, &len);
+	if (!val || (len != sizeof(uint32_t)))
+		return -FDT_ERR_NOTFOUND;
+
+	*out = fdt32_to_cpu(*val);
+
+	return 0;
+}
+
+static int fdt_getprop_str(void *fdt, const char *path, const char *name,
+			   const char **out)
+{
+	int node_off;
+
+	node_off = fdt_path_offset(fdt, path);
+	if (node_off < 0)
+		return node_off;
+
+	return fdt_get_string(fdt, node_off, name, out);
+}
+
+static int fdt_overlay_change_int_property(struct unit_test_state *uts)
+{
+	void *fdt = uts->priv;
+	u32 val = 0;
+
+	ut_assertok(fdt_getprop_u32(fdt, "/test-node", "test-int-property",
+				    &val));
+	ut_asserteq(43, val);
+
+	return CMD_RET_SUCCESS;
+}
+OVERLAY_TEST(fdt_overlay_change_int_property, 0);
+
+static int fdt_overlay_change_str_property(struct unit_test_state *uts)
+{
+	void *fdt = uts->priv;
+	const char *val = NULL;
+
+	ut_assertok(fdt_getprop_str(fdt, "/test-node", "test-str-property",
+				    &val));
+	ut_asserteq_str("foobar", val);
+
+	return CMD_RET_SUCCESS;
+}
+OVERLAY_TEST(fdt_overlay_change_str_property, 0);
+
+static int fdt_overlay_add_str_property(struct unit_test_state *uts)
+{
+	void *fdt = uts->priv;
+	const char *val = NULL;
+
+	ut_assertok(fdt_getprop_str(fdt, "/test-node", "test-str-property-2",
+				    &val));
+	ut_asserteq_str("foobar2", val);
+
+	return CMD_RET_SUCCESS;
+}
+OVERLAY_TEST(fdt_overlay_add_str_property, 0);
+
+static int fdt_overlay_add_node_by_phandle(struct unit_test_state *uts)
+{
+	void *fdt = uts->priv;
+	int off;
+
+	off = fdt_path_offset(fdt, "/test-node/new-node");
+	ut_assert(off >= 0);
+
+	ut_assertnonnull(fdt_getprop(fdt, off, "new-property", NULL));
+
+	return CMD_RET_SUCCESS;
+}
+OVERLAY_TEST(fdt_overlay_add_node_by_phandle, 0);
+
+static int fdt_overlay_add_node_by_path(struct unit_test_state *uts)
+{
+	void *fdt = uts->priv;
+	int off;
+
+	off = fdt_path_offset(fdt, "/new-node");
+	ut_assert(off >= 0);
+
+	ut_assertnonnull(fdt_getprop(fdt, off, "new-property", NULL));
+
+	return CMD_RET_SUCCESS;
+}
+OVERLAY_TEST(fdt_overlay_add_node_by_path, 0);
+
+int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
+{
+	struct unit_test *tests = ll_entry_start(struct unit_test,
+						 overlay_test);
+	const int n_ents = ll_entry_count(struct unit_test, overlay_test);
+	struct unit_test_state *uts;
+	struct unit_test *test;
+	void *fdt_base = &__dtb_test_fdt_base_begin;
+	void *fdt_overlay = &__dtb_test_fdt_overlay_begin;
+	void *fdt_base_copy;
+
+	ut_assertok(fdt_check_header(fdt_base));
+	ut_assertok(fdt_check_header(fdt_overlay));
+
+	uts = calloc(1, sizeof(*uts));
+	if (!uts)
+		return -ENOMEM;
+
+	fdt_base_copy = malloc(FDT_COPY_SIZE);
+	if (!fdt_base_copy)
+		return -ENOMEM;
+	uts->priv = fdt_base_copy;
+
+	/*
+	 * Resize the FDT to 4k so that we have room to operate on
+	 *
+	 * (and relocate it since the memory might be mapped
+	 * read-only)
+	 */
+	ut_assertok(fdt_open_into(fdt_base, fdt_base_copy, FDT_COPY_SIZE));
+
+	/* Apply the overlay */
+	ut_assertok(fdt_overlay_apply(fdt_base_copy, fdt_overlay));
+
+	if (argc == 1)
+		printf("Running %d environment tests\n", n_ents);
+
+	for (test = tests; test < tests + n_ents; test++) {
+		if (argc > 1 && strcmp(argv[1], test->name))
+			continue;
+		printf("Test: %s\n", test->name);
+
+		uts->start = mallinfo();
+
+		test->func(uts);
+	}
+
+	printf("Failures: %d\n", uts->fail_count);
+
+	free(fdt_base_copy);
+	free(uts);
+
+	return uts->fail_count ? CMD_RET_FAILURE : 0;
+}
diff --git a/test/overlay/test-fdt-base.dts b/test/overlay/test-fdt-base.dts
new file mode 100644
index 000000000000..e8fa9bb5be82
--- /dev/null
+++ b/test/overlay/test-fdt-base.dts
@@ -0,0 +1,17 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+
+/ {
+	test: test-node {
+		test-int-property = <42>;
+		test-str-property = "foo";
+	};
+};
+
+
diff --git a/test/overlay/test-fdt-overlay.dts b/test/overlay/test-fdt-overlay.dts
new file mode 100644
index 000000000000..ca3abcff287c
--- /dev/null
+++ b/test/overlay/test-fdt-overlay.dts
@@ -0,0 +1,60 @@
+/*
+ * Copyright (c) 2016 NextThing Co
+ * Copyright (c) 2016 Free Electrons
+ *
+ * SPDX-License-Identifier:	GPL-2.0+
+ */
+
+/dts-v1/;
+/plugin/;
+
+/ {
+	/* Test that we can change an int by another */
+	fragment@0 {
+		target = <&test>;
+
+		__overlay__ {
+			test-int-property = <43>;
+		};
+	};
+
+	/* Test that we can replace a string by a longer one */
+	fragment@1 {
+		target = <&test>;
+
+		__overlay__ {
+			test-str-property = "foobar";
+		};
+	};
+
+	/* Test that we add a new property */
+	fragment@2 {
+		target = <&test>;
+
+		__overlay__ {
+			test-str-property-2 = "foobar2";
+		};
+	};
+
+	/* Test that we add a new node (by phandle) */
+	fragment@3 {
+		target = <&test>;
+
+		__overlay__ {
+			new-node {
+				new-property;
+			};
+		};
+	};
+
+	/* Test that we add a new node (by path) */
+	fragment@4 {
+		target-path = "/";
+
+		__overlay__ {
+			new-node {
+				new-property;
+			};
+		};
+	};
+};
-- 
2.8.2

Re: [PATCH v2 9/9] tests: Introduce DT overlay tests

[Simon Glass]

On 27 May 2016 at 03:13, Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> This adds a bunch of unit tests for the "fdt apply" command.
>
> They've all been run successfully in the sandbox. However, as you still
> require an out-of-tree dtc with overlay support, this is disabled by
> default.
>
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
>  Makefile                          |   1 +
>  include/test/overlay.h            |  16 ++++
>  include/test/suites.h             |   1 +
>  test/Kconfig                      |   1 +
>  test/cmd_ut.c                     |   6 ++
>  test/overlay/Kconfig              |  10 +++
>  test/overlay/Makefile             |  15 ++++
>  test/overlay/cmd_ut_overlay.c     | 176 ++++++++++++++++++++++++++++++++++++++
>  test/overlay/test-fdt-base.dts    |  17 ++++
>  test/overlay/test-fdt-overlay.dts |  60 +++++++++++++
>  10 files changed, 303 insertions(+)
>  create mode 100644 include/test/overlay.h
>  create mode 100644 test/overlay/Kconfig
>  create mode 100644 test/overlay/Makefile
>  create mode 100644 test/overlay/cmd_ut_overlay.c
>  create mode 100644 test/overlay/test-fdt-base.dts
>  create mode 100644 test/overlay/test-fdt-overlay.dts

Acked-by: Simon Glass <sjg-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>

Re: [PATCH v2 9/9] tests: Introduce DT overlay tests

[Pantelis Antoniou]

> On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:
> 
> This adds a bunch of unit tests for the "fdt apply" command.
> 
> They've all been run successfully in the sandbox. However, as you still
> require an out-of-tree dtc with overlay support, this is disabled by
> default.
> 
> Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> ---
> Makefile                          |   1 +
> include/test/overlay.h            |  16 ++++
> include/test/suites.h             |   1 +
> test/Kconfig                      |   1 +
> test/cmd_ut.c                     |   6 ++
> test/overlay/Kconfig              |  10 +++
> test/overlay/Makefile             |  15 ++++
> test/overlay/cmd_ut_overlay.c     | 176 ++++++++++++++++++++++++++++++++++++++
> test/overlay/test-fdt-base.dts    |  17 ++++
> test/overlay/test-fdt-overlay.dts |  60 +++++++++++++
> 10 files changed, 303 insertions(+)
> create mode 100644 include/test/overlay.h
> create mode 100644 test/overlay/Kconfig
> create mode 100644 test/overlay/Makefile
> create mode 100644 test/overlay/cmd_ut_overlay.c
> create mode 100644 test/overlay/test-fdt-base.dts
> create mode 100644 test/overlay/test-fdt-overlay.dts
> 
> diff --git a/Makefile b/Makefile
> index 954a865381af..71272501a75e 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -665,6 +665,7 @@ libs-$(CONFIG_HAS_POST) += post/
> libs-y += test/
> libs-y += test/dm/
> libs-$(CONFIG_UT_ENV) += test/env/
> +libs-$(CONFIG_UT_OVERLAY) += test/overlay/
> 
> libs-y += $(if $(BOARDDIR),board/$(BOARDDIR)/)
> 
> diff --git a/include/test/overlay.h b/include/test/overlay.h
> new file mode 100644
> index 000000000000..392f28ff8405
> --- /dev/null
> +++ b/include/test/overlay.h
> @@ -0,0 +1,16 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#ifndef __TEST_OVERLAY_H__
> +#define __TEST_OVERLAY_H__
> +
> +#include <test/test.h>
> +
> +/* Declare a new environment test */
> +#define OVERLAY_TEST(_name, _flags)	UNIT_TEST(_name, _flags, overlay_test)
> +
> +#endif /* __TEST_OVERLAY_H__ */
> diff --git a/include/test/suites.h b/include/test/suites.h
> index f5790333ff8e..0e94feb07a79 100644
> --- a/include/test/suites.h
> +++ b/include/test/suites.h
> @@ -10,6 +10,7 @@
> 
> int do_ut_dm(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> int do_ut_env(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> +int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> int do_ut_time(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[]);
> 
> #endif /* __TEST_SUITES_H__ */
> diff --git a/test/Kconfig b/test/Kconfig
> index d71c332eee27..3643761bc6ef 100644
> --- a/test/Kconfig
> +++ b/test/Kconfig
> @@ -17,3 +17,4 @@ config UT_TIME
> 
> source "test/dm/Kconfig"
> source "test/env/Kconfig"
> +source "test/overlay/Kconfig"
> diff --git a/test/cmd_ut.c b/test/cmd_ut.c
> index f6e1f413db7f..14333423a178 100644
> --- a/test/cmd_ut.c
> +++ b/test/cmd_ut.c
> @@ -19,6 +19,9 @@ static cmd_tbl_t cmd_ut_sub[] = {
> #if defined(CONFIG_UT_ENV)
> 	U_BOOT_CMD_MKENT(env, CONFIG_SYS_MAXARGS, 1, do_ut_env, "", ""),
> #endif
> +#ifdef CONFIG_UT_OVERLAY
> +	U_BOOT_CMD_MKENT(overlay, CONFIG_SYS_MAXARGS, 1, do_ut_overlay, "", ""),
> +#endif
> #ifdef CONFIG_UT_TIME
> 	U_BOOT_CMD_MKENT(time, CONFIG_SYS_MAXARGS, 1, do_ut_time, "", ""),
> #endif
> @@ -68,6 +71,9 @@ static char ut_help_text[] =
> #ifdef CONFIG_UT_ENV
> 	"ut env [test-name]\n"
> #endif
> +#ifdef CONFIG_UT_OVERLAY
> +	"ut overlay [test-name]\n"
> +#endif
> #ifdef CONFIG_UT_TIME
> 	"ut time - Very basic test of time functions\n"
> #endif
> diff --git a/test/overlay/Kconfig b/test/overlay/Kconfig
> new file mode 100644
> index 000000000000..b8e01a934373
> --- /dev/null
> +++ b/test/overlay/Kconfig
> @@ -0,0 +1,10 @@
> +config UT_OVERLAY
> +	bool "Enable Device Tree Overlays Unit Tests"
> +	depends on UNIT_TEST
> +	help
> +	  This enables the 'ut overlay' command which runs a series of unit
> +	  tests on the fdt overlay code.
> +	  If all is well then all tests pass although there will be a few
> +	  messages printed along the way.
> +	  Be warned that it requires an out-of-tree dtc compiler with patches
> +	  to support the DT overlays, otherwise it will fail.
> diff --git a/test/overlay/Makefile b/test/overlay/Makefile
> new file mode 100644
> index 000000000000..907f08544619
> --- /dev/null
> +++ b/test/overlay/Makefile
> @@ -0,0 +1,15 @@
> +#
> +# Copyright (c) 2016 NextThing Co
> +# Copyright (c) 2016 Free Electrons
> +#
> +# SPDX-License-Identifier:	GPL-2.0+
> +#
> +
> +# Test files
> +obj-y += cmd_ut_overlay.o
> +
> +DTC_FLAGS += -@
> +
> +# DT overlays
> +obj-y += test-fdt-base.dtb.o
> +obj-y += test-fdt-overlay.dtb.o
> diff --git a/test/overlay/cmd_ut_overlay.c b/test/overlay/cmd_ut_overlay.c
> new file mode 100644
> index 000000000000..3ff1d61b7e31
> --- /dev/null
> +++ b/test/overlay/cmd_ut_overlay.c
> @@ -0,0 +1,176 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <errno.h>
> +#include <malloc.h>
> +
> +#include <linux/sizes.h>
> +
> +#include <test/ut.h>
> +#include <test/overlay.h>
> +
> +/* 4k ought to be enough for anybody */
> +#define FDT_COPY_SIZE	(4 * SZ_1K)
> +
> +extern u32 __dtb_test_fdt_base_begin;
> +extern u32 __dtb_test_fdt_overlay_begin;
> +
> +static int fdt_getprop_u32(void *fdt, const char *path, const char *name,
> +			   u32 *out)
> +{
> +	const fdt32_t *val;
> +	int node_off;
> +	int len;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	val = fdt_getprop(fdt, node_off, name, &len);
> +	if (!val || (len != sizeof(uint32_t)))
> +		return -FDT_ERR_NOTFOUND;
> +
> +	*out = fdt32_to_cpu(*val);
> +
> +	return 0;
> +}
> +
> +static int fdt_getprop_str(void *fdt, const char *path, const char *name,
> +			   const char **out)
> +{
> +	int node_off;
> +
> +	node_off = fdt_path_offset(fdt, path);
> +	if (node_off < 0)
> +		return node_off;
> +
> +	return fdt_get_string(fdt, node_off, name, out);
> +}
> +
> +static int fdt_overlay_change_int_property(struct unit_test_state *uts)
> +{
> +	void *fdt = uts->priv;
> +	u32 val = 0;
> +
> +	ut_assertok(fdt_getprop_u32(fdt, "/test-node", "test-int-property",
> +				    &val));
> +	ut_asserteq(43, val);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +OVERLAY_TEST(fdt_overlay_change_int_property, 0);
> +
> +static int fdt_overlay_change_str_property(struct unit_test_state *uts)
> +{
> +	void *fdt = uts->priv;
> +	const char *val = NULL;
> +
> +	ut_assertok(fdt_getprop_str(fdt, "/test-node", "test-str-property",
> +				    &val));
> +	ut_asserteq_str("foobar", val);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +OVERLAY_TEST(fdt_overlay_change_str_property, 0);
> +
> +static int fdt_overlay_add_str_property(struct unit_test_state *uts)
> +{
> +	void *fdt = uts->priv;
> +	const char *val = NULL;
> +
> +	ut_assertok(fdt_getprop_str(fdt, "/test-node", "test-str-property-2",
> +				    &val));
> +	ut_asserteq_str("foobar2", val);
> +
> +	return CMD_RET_SUCCESS;
> +}
> +OVERLAY_TEST(fdt_overlay_add_str_property, 0);
> +
> +static int fdt_overlay_add_node_by_phandle(struct unit_test_state *uts)
> +{
> +	void *fdt = uts->priv;
> +	int off;
> +
> +	off = fdt_path_offset(fdt, "/test-node/new-node");
> +	ut_assert(off >= 0);
> +
> +	ut_assertnonnull(fdt_getprop(fdt, off, "new-property", NULL));
> +
> +	return CMD_RET_SUCCESS;
> +}
> +OVERLAY_TEST(fdt_overlay_add_node_by_phandle, 0);
> +
> +static int fdt_overlay_add_node_by_path(struct unit_test_state *uts)
> +{
> +	void *fdt = uts->priv;
> +	int off;
> +
> +	off = fdt_path_offset(fdt, "/new-node");
> +	ut_assert(off >= 0);
> +
> +	ut_assertnonnull(fdt_getprop(fdt, off, "new-property", NULL));
> +
> +	return CMD_RET_SUCCESS;
> +}
> +OVERLAY_TEST(fdt_overlay_add_node_by_path, 0);
> +
> +int do_ut_overlay(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
> +{
> +	struct unit_test *tests = ll_entry_start(struct unit_test,
> +						 overlay_test);
> +	const int n_ents = ll_entry_count(struct unit_test, overlay_test);
> +	struct unit_test_state *uts;
> +	struct unit_test *test;
> +	void *fdt_base = &__dtb_test_fdt_base_begin;
> +	void *fdt_overlay = &__dtb_test_fdt_overlay_begin;
> +	void *fdt_base_copy;
> +
> +	ut_assertok(fdt_check_header(fdt_base));
> +	ut_assertok(fdt_check_header(fdt_overlay));
> +
> +	uts = calloc(1, sizeof(*uts));
> +	if (!uts)
> +		return -ENOMEM;
> +
> +	fdt_base_copy = malloc(FDT_COPY_SIZE);
> +	if (!fdt_base_copy)
> +		return -ENOMEM;
> +	uts->priv = fdt_base_copy;
> +
> +	/*
> +	 * Resize the FDT to 4k so that we have room to operate on
> +	 *
> +	 * (and relocate it since the memory might be mapped
> +	 * read-only)
> +	 */
> +	ut_assertok(fdt_open_into(fdt_base, fdt_base_copy, FDT_COPY_SIZE));
> +
> +	/* Apply the overlay */
> +	ut_assertok(fdt_overlay_apply(fdt_base_copy, fdt_overlay));
> +
> +	if (argc == 1)
> +		printf("Running %d environment tests\n", n_ents);
> +
> +	for (test = tests; test < tests + n_ents; test++) {
> +		if (argc > 1 && strcmp(argv[1], test->name))
> +			continue;
> +		printf("Test: %s\n", test->name);
> +
> +		uts->start = mallinfo();
> +
> +		test->func(uts);
> +	}
> +
> +	printf("Failures: %d\n", uts->fail_count);
> +
> +	free(fdt_base_copy);
> +	free(uts);
> +
> +	return uts->fail_count ? CMD_RET_FAILURE : 0;
> +}
> diff --git a/test/overlay/test-fdt-base.dts b/test/overlay/test-fdt-base.dts
> new file mode 100644
> index 000000000000..e8fa9bb5be82
> --- /dev/null
> +++ b/test/overlay/test-fdt-base.dts
> @@ -0,0 +1,17 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +
> +/ {
> +	test: test-node {
> +		test-int-property = <42>;
> +		test-str-property = "foo";
> +	};
> +};
> +
> +
> diff --git a/test/overlay/test-fdt-overlay.dts b/test/overlay/test-fdt-overlay.dts
> new file mode 100644
> index 000000000000..ca3abcff287c
> --- /dev/null
> +++ b/test/overlay/test-fdt-overlay.dts
> @@ -0,0 +1,60 @@
> +/*
> + * Copyright (c) 2016 NextThing Co
> + * Copyright (c) 2016 Free Electrons
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +/dts-v1/;
> +/plugin/;
> +
> +/ {
> +	/* Test that we can change an int by another */
> +	fragment@0 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-int-property = <43>;
> +		};
> +	};
> +
> +	/* Test that we can replace a string by a longer one */
> +	fragment@1 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-str-property = "foobar";
> +		};
> +	};
> +
> +	/* Test that we add a new property */
> +	fragment@2 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			test-str-property-2 = "foobar2";
> +		};
> +	};
> +
> +	/* Test that we add a new node (by phandle) */
> +	fragment@3 {
> +		target = <&test>;
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +
> +	/* Test that we add a new node (by path) */
> +	fragment@4 {
> +		target-path = "/";
> +
> +		__overlay__ {
> +			new-node {
> +				new-property;
> +			};
> +		};
> +	};
> +};
> -- 
> 2.8.2
> 

Acked-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>