[PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE

[PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need these tools going forward. Lets start fetching them.

Note that a copy of fdtdump.c was already copied back in the year 2012,
but was never updated or built for some reason.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2: Separate out this change from Makefile one.

This needs to be followed by invocation of the ./update-dtc-source.sh
script so the relevant files can be copied before the Makefile is
updated in the next patch.

 scripts/dtc/update-dtc-source.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
index bc704e2a6a4a..9bc4afb71415 100755
--- a/scripts/dtc/update-dtc-source.sh
+++ b/scripts/dtc/update-dtc-source.sh
@@ -31,9 +31,9 @@ set -ev
 DTC_UPSTREAM_PATH=`pwd`/../dtc
 DTC_LINUX_PATH=`pwd`/scripts/dtc
 
-DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
-		srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
-		dtc-lexer.l dtc-parser.y"
+DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \
+		fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \
+		util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y"
 LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
 		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
 		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
-- 
2.25.0.rc1.19.g042ed3e048af

[PATCH V2 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need these tools going forward. Lets start building them.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 scripts/dtc/Makefile | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
index 4852bf44e913..c607980a5c17 100644
--- a/scripts/dtc/Makefile
+++ b/scripts/dtc/Makefile
@@ -1,12 +1,18 @@
 # SPDX-License-Identifier: GPL-2.0
 # scripts/dtc makefile
 
-hostprogs-always-$(CONFIG_DTC)		+= dtc
+hostprogs-always-$(CONFIG_DTC)		+= dtc fdtdump fdtoverlay
 hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
 
 dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
 		   srcpos.o checks.o util.o
 dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
+fdtdump-objs	:= fdtdump.o util.o
+
+libfdt_dir	= libfdt
+libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
+libfdt		= $(addprefix $(libfdt_dir)/,$(libfdt-objs))
+fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
 
 # Source files need to get at the userspace version of libfdt_env.h to compile
 HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V2 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Masahiro Yamada]

On Thu, Jan 7, 2021 at 2:16 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> We will start building overlays for platforms soon in the kernel and
> would need these tools going forward. Lets start building them.


The commit log should explain how fdtdump and fdtoverlay are used
while building the kernel tree.







> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  scripts/dtc/Makefile | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index 4852bf44e913..c607980a5c17 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -1,12 +1,18 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # scripts/dtc makefile
>
> -hostprogs-always-$(CONFIG_DTC)         += dtc
> +hostprogs-always-$(CONFIG_DTC)         += dtc fdtdump fdtoverlay
>  hostprogs-always-$(CHECK_DT_BINDING)   += dtc
>
>  dtc-objs       := dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
>                    srcpos.o checks.o util.o
>  dtc-objs       += dtc-lexer.lex.o dtc-parser.tab.o
> +fdtdump-objs   := fdtdump.o util.o
> +
> +libfdt_dir     = libfdt


Adding 'libfdt_dir' is not helpful except
increasing the amount of code.

Please hard-code 'libfdt'


> +libfdt-objs    := fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
> +libfdt         = $(addprefix $(libfdt_dir)/,$(libfdt-objs))
> +fdtoverlay-objs        := $(libfdt) fdtoverlay.o util.o
>  # Source files need to get at the userspace version of libfdt_env.h to compile
>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
> --
> 2.25.0.rc1.19.g042ed3e048af
>


--
Best Regards

Masahiro Yamada

[PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Viresh Kumar]

We will start building overlays for platforms soon in the kernel and
would need these tools going forward. Lets start building them.

The fdtoverlay program applies (or merges) one ore more overlay dtb
blobs to a base dtb blob. The kernel build system would later use
fdtoverlay to generate the overlaid blobs based on platform specific
configurations.

The fdtdump program prints a readable version of a flat device-tree
file. This is a very useful tool to analyze the details of the overlay's
dtb and the final dtb produced by fdtoverlay after applying the
overlay's dtb to a base dtb.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V3:
- Updated log
- Remove libfdt_dir

 scripts/dtc/Makefile | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
index 4852bf44e913..472ab8cd590c 100644
--- a/scripts/dtc/Makefile
+++ b/scripts/dtc/Makefile
@@ -1,12 +1,17 @@
 # SPDX-License-Identifier: GPL-2.0
 # scripts/dtc makefile
 
-hostprogs-always-$(CONFIG_DTC)		+= dtc
+hostprogs-always-$(CONFIG_DTC)		+= dtc fdtdump fdtoverlay
 hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
 
 dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
 		   srcpos.o checks.o util.o
 dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
+fdtdump-objs	:= fdtdump.o util.o
+
+libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
+libfdt		= $(addprefix libfdt/,$(libfdt-objs))
+fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
 
 # Source files need to get at the userspace version of libfdt_env.h to compile
 HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Frank Rowand]

On 1/7/21 12:25 AM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need these tools going forward. Lets start building them.
> 
> The fdtoverlay program applies (or merges) one ore more overlay dtb
> blobs to a base dtb blob. The kernel build system would later use
> fdtoverlay to generate the overlaid blobs based on platform specific
> configurations.
> 
> The fdtdump program prints a readable version of a flat device-tree
> file. This is a very useful tool to analyze the details of the overlay's
> dtb and the final dtb produced by fdtoverlay after applying the
> overlay's dtb to a base dtb.

You can calso dump an FDT with:

   dtc -O dts XXX.dtb

Is this sufficient for the desired functionality, or is there something
additional in fdtdump that is needed?

If nothing additional needed, and there is no other justification for adding
another program, I would prefer to leave fdtdump out.

-Frank

> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V3:
> - Updated log
> - Remove libfdt_dir
> 
>  scripts/dtc/Makefile | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index 4852bf44e913..472ab8cd590c 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -1,12 +1,17 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # scripts/dtc makefile
>  
> -hostprogs-always-$(CONFIG_DTC)		+= dtc
> +hostprogs-always-$(CONFIG_DTC)		+= dtc fdtdump fdtoverlay
>  hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
>  
>  dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
>  		   srcpos.o checks.o util.o
>  dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
> +fdtdump-objs	:= fdtdump.o util.o
> +
> +libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
> +libfdt		= $(addprefix libfdt/,$(libfdt-objs))
> +fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
>  
>  # Source files need to get at the userspace version of libfdt_env.h to compile
>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
> 

Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Viresh Kumar]

On 11-01-21, 18:44, Frank Rowand wrote:
> On 1/7/21 12:25 AM, Viresh Kumar wrote:
> > We will start building overlays for platforms soon in the kernel and
> > would need these tools going forward. Lets start building them.
> > 
> > The fdtoverlay program applies (or merges) one ore more overlay dtb
> > blobs to a base dtb blob. The kernel build system would later use
> > fdtoverlay to generate the overlaid blobs based on platform specific
> > configurations.
> > 
> > The fdtdump program prints a readable version of a flat device-tree
> > file. This is a very useful tool to analyze the details of the overlay's
> > dtb and the final dtb produced by fdtoverlay after applying the
> > overlay's dtb to a base dtb.
> 
> You can calso dump an FDT with:
> 
>    dtc -O dts XXX.dtb
> 
> Is this sufficient for the desired functionality, or is there something
> additional in fdtdump that is needed?

Not for my usecase at least.

> If nothing additional needed, and there is no other justification for adding
> another program, I would prefer to leave fdtdump out.

Okay, then I will also remove the stale version of fdtdump which is
already there in kernel since a long time.

-- 
viresh

Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Frank Rowand]

On 1/11/21 11:08 PM, Viresh Kumar wrote:
> On 11-01-21, 18:44, Frank Rowand wrote:
>> On 1/7/21 12:25 AM, Viresh Kumar wrote:
>>> We will start building overlays for platforms soon in the kernel and
>>> would need these tools going forward. Lets start building them.
>>>
>>> The fdtoverlay program applies (or merges) one ore more overlay dtb
>>> blobs to a base dtb blob. The kernel build system would later use
>>> fdtoverlay to generate the overlaid blobs based on platform specific
>>> configurations.
>>>
>>> The fdtdump program prints a readable version of a flat device-tree
>>> file. This is a very useful tool to analyze the details of the overlay's
>>> dtb and the final dtb produced by fdtoverlay after applying the
>>> overlay's dtb to a base dtb.
>>
>> You can calso dump an FDT with:
>>
>>    dtc -O dts XXX.dtb
>>
>> Is this sufficient for the desired functionality, or is there something
>> additional in fdtdump that is needed?
> 

comment 1:

> Not for my usecase at least.

> 
>> If nothing additional needed, and there is no other justification for adding
>> another program, I would prefer to leave fdtdump out.
> 

comment 2:

> Okay, then I will also remove the stale version of fdtdump which is
> already there in kernel since a long time.
> 

I'm confused.  I read comment 1 as saying that fdtdump does provide a feature
that you need to analyze the dtb created by fdtoverlay.  But I read comment 2
as implying that you are accepting that fdtdump will not be added to the
Linux kernel source.

-Frank

Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Viresh Kumar]

On 12-01-21, 12:34, Frank Rowand wrote:
> On 1/11/21 11:08 PM, Viresh Kumar wrote:
> > On 11-01-21, 18:44, Frank Rowand wrote:
> >> On 1/7/21 12:25 AM, Viresh Kumar wrote:
> >>> We will start building overlays for platforms soon in the kernel and
> >>> would need these tools going forward. Lets start building them.
> >>>
> >>> The fdtoverlay program applies (or merges) one ore more overlay dtb
> >>> blobs to a base dtb blob. The kernel build system would later use
> >>> fdtoverlay to generate the overlaid blobs based on platform specific
> >>> configurations.
> >>>
> >>> The fdtdump program prints a readable version of a flat device-tree
> >>> file. This is a very useful tool to analyze the details of the overlay's
> >>> dtb and the final dtb produced by fdtoverlay after applying the
> >>> overlay's dtb to a base dtb.
> >>
> >> You can calso dump an FDT with:
> >>
> >>    dtc -O dts XXX.dtb
> >>
> >> Is this sufficient for the desired functionality, or is there something
> >> additional in fdtdump that is needed?
> > 
> 
> comment 1:
> 
> > Not for my usecase at least.

I answered this question here (and yes I could have been more clear):

"is there something additional in fdtdump that is needed?"

> 
> > 
> >> If nothing additional needed, and there is no other justification for adding
> >> another program, I would prefer to leave fdtdump out.
> > 
> 
> comment 2:
> 
> > Okay, then I will also remove the stale version of fdtdump which is
> > already there in kernel since a long time.
> > 
> 
> I'm confused.  I read comment 1 as saying that fdtdump does provide a feature
> that you need to analyze the dtb created by fdtoverlay.  But I read comment 2
> as implying that you are accepting that fdtdump will not be added to the
> Linux kernel source.

-- 
viresh

Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Frank Rowand]

Hi Viresh,

I may have an email hiccup, but I don't see a "[PATCH V3 1/2]" email, I
only see this "V3 2/2" email.

Please start each new version of a patch series as a stand alone email
instead of a reply to an email in a previous version of the series.
That way each patch series discussion stands out as a separate thread.

Also each version of the patch series needs to include all of the patches
in the current version, even if they are unchanged from the previous
version.

Thanks,

Frank

On 1/7/21 12:25 AM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need these tools going forward. Lets start building them.
> 
> The fdtoverlay program applies (or merges) one ore more overlay dtb
> blobs to a base dtb blob. The kernel build system would later use
> fdtoverlay to generate the overlaid blobs based on platform specific
> configurations.
> 
> The fdtdump program prints a readable version of a flat device-tree
> file. This is a very useful tool to analyze the details of the overlay's
> dtb and the final dtb produced by fdtoverlay after applying the
> overlay's dtb to a base dtb.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V3:
> - Updated log
> - Remove libfdt_dir
> 
>  scripts/dtc/Makefile | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index 4852bf44e913..472ab8cd590c 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -1,12 +1,17 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # scripts/dtc makefile
>  
> -hostprogs-always-$(CONFIG_DTC)		+= dtc
> +hostprogs-always-$(CONFIG_DTC)		+= dtc fdtdump fdtoverlay
>  hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
>  
>  dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
>  		   srcpos.o checks.o util.o
>  dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
> +fdtdump-objs	:= fdtdump.o util.o
> +
> +libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
> +libfdt		= $(addprefix libfdt/,$(libfdt-objs))
> +fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
>  
>  # Source files need to get at the userspace version of libfdt_env.h to compile
>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
> 

Re: [PATCH V3 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Viresh Kumar]

On 11-01-21, 18:55, Frank Rowand wrote:
> Hi Viresh,
> 
> I may have an email hiccup,

No you don't, I just sent the 2nd patch alone.

> but I don't see a "[PATCH V3 1/2]" email, I
> only see this "V3 2/2" email.
> 
> Please start each new version of a patch series as a stand alone email
> instead of a reply to an email in a previous version of the series.
> That way each patch series discussion stands out as a separate thread.
> 
> Also each version of the patch series needs to include all of the patches
> in the current version, even if they are unchanged from the previous
> version.

Sure, just that some people like to just bump individual patches until
the time there is an agreement and then send the whole stuff together.

I will send the whole stuff going forward whenever the versions
change.

Thanks Frank.

-- 
viresh

Re: [PATCH V2 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Frank Rowand]

On 1/6/21 11:15 PM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need these tools going forward. Lets start building them.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  scripts/dtc/Makefile | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
> index 4852bf44e913..c607980a5c17 100644
> --- a/scripts/dtc/Makefile
> +++ b/scripts/dtc/Makefile
> @@ -1,12 +1,18 @@
>  # SPDX-License-Identifier: GPL-2.0
>  # scripts/dtc makefile
>  
> -hostprogs-always-$(CONFIG_DTC)		+= dtc
> +hostprogs-always-$(CONFIG_DTC)		+= dtc fdtdump fdtoverlay
>  hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
>  
>  dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
>  		   srcpos.o checks.o util.o
>  dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
> +fdtdump-objs	:= fdtdump.o util.o
> +

# The upstream project builds libfdt as a separate library.  We are choosing to
# instead directly link the libfdt object files into fdtoverly

> +libfdt_dir	= libfdt
> +libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
> +libfdt		= $(addprefix $(libfdt_dir)/,$(libfdt-objs))
> +fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
>  
>  # Source files need to get at the userspace version of libfdt_env.h to compile
>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
> 

In general, I am a proponent of using shared libraries (which the upstream project
builds by default) because if a security bug in the library is fixed, it is fixed
for all users of the library.

In this specific case, I actually prefer the implementation that the patch provides
(directly linking the library object files into fdtoverlay, which uses the library)
because it is the only user of the library _and_ fdtoverlay will not inadvertently
use the system wide libfdt if it happens to be installed (as it is on my system).

Any thoughts on this Rob?

-Frank

Re: [PATCH V2 2/2] scripts: dtc: Build fdtoverlay and fdtdump tools

[Frank Rowand]

On 1/19/21 10:28 AM, Frank Rowand wrote:
> On 1/6/21 11:15 PM, Viresh Kumar wrote:
>> We will start building overlays for platforms soon in the kernel and
>> would need these tools going forward. Lets start building them.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>>  scripts/dtc/Makefile | 8 +++++++-
>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/dtc/Makefile b/scripts/dtc/Makefile
>> index 4852bf44e913..c607980a5c17 100644
>> --- a/scripts/dtc/Makefile
>> +++ b/scripts/dtc/Makefile
>> @@ -1,12 +1,18 @@
>>  # SPDX-License-Identifier: GPL-2.0
>>  # scripts/dtc makefile
>>  
>> -hostprogs-always-$(CONFIG_DTC)		+= dtc
>> +hostprogs-always-$(CONFIG_DTC)		+= dtc fdtdump fdtoverlay
>>  hostprogs-always-$(CHECK_DT_BINDING)	+= dtc
>>  
>>  dtc-objs	:= dtc.o flattree.o fstree.o data.o livetree.o treesource.o \
>>  		   srcpos.o checks.o util.o
>>  dtc-objs	+= dtc-lexer.lex.o dtc-parser.tab.o
>> +fdtdump-objs	:= fdtdump.o util.o
>> +
> 
> # The upstream project builds libfdt as a separate library.  We are choosing to
> # instead directly link the libfdt object files into fdtoverly
> 
>> +libfdt_dir	= libfdt
>> +libfdt-objs	:= fdt.o fdt_ro.o fdt_wip.o fdt_sw.o fdt_rw.o fdt_strerror.o fdt_empty_tree.o fdt_addresses.o fdt_overlay.o
>> +libfdt		= $(addprefix $(libfdt_dir)/,$(libfdt-objs))
>> +fdtoverlay-objs	:= $(libfdt) fdtoverlay.o util.o
>>  
>>  # Source files need to get at the userspace version of libfdt_env.h to compile
>>  HOST_EXTRACFLAGS += -I $(srctree)/$(src)/libfdt
>>
> 
> In general, I am a proponent of using shared libraries (which the upstream project
> builds by default) because if a security bug in the library is fixed, it is fixed
> for all users of the library.
> 
> In this specific case, I actually prefer the implementation that the patch provides
> (directly linking the library object files into fdtoverlay, which uses the library)
> because it is the only user of the library _and_ fdtoverlay will not inadvertently
> use the system wide libfdt if it happens to be installed (as it is on my system).
> 
> Any thoughts on this Rob?

I see that this patch series is up to v4, so I commented in the wrong place.
I will repeat this comment in the v4 series.

> 
> -Frank
> 

[PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

Now that fdtoverlay is part of the kernel build, start using it to test
the unitest overlays we have by applying them statically.

The file overlay_base.dtb have symbols of its own and we need to apply
overlay.dtb to overlay_base.dtb alone first to make it work, which gives
us intermediate-overlay.dtb file.

The intermediate-overlay.dtb file along with all other overlays is them
applied to testcases.dtb to generate the master.dtb file.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

---
Depends on:

https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@linaro.org/

I have kept the .dtb naming for overlays for now, lets see how we do it
eventually.

Rob/Frank, this doesn't work properly right now. Maybe I missed how
these overlays must be applied or there is a bug in fdtoverlay.

The master.dtb doesn't include any nodes from overlay_base.dtb or
overlay.dtb probably because 'testcase-data-2' node isn't present in
testcases.dtb and fdtoverlay doesn't allow applying new nodes to the
root node, i.e. allows new sub-nodes once it gets phandle to the parent
but nothing can be added to the root node itself. Though I get a feel
that it works while applying the nodes dynamically and it is expected to
work here as well.

(And yeah, this is my first serious attempt at updating Makefiles, I am
sure there is a scope of improvement here :))

---
 drivers/of/unittest-data/Makefile | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 009f4045c8e4..f17bce85f65f 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@
 
 # suppress warnings about intentional errors
 DTC_FLAGS_testcases += -Wno-interrupts_property
+
+# Apply overlays statically with fdtoverlay
+intermediate-overlay	:= overlay.dtb
+master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
+			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
+			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
+			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
+			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
+			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
+			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
+			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
+			   intermediate-overlay.dtb
+
+quiet_cmd_fdtoverlay = fdtoverlay $@
+      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
+
+$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
+	$(call if_changed,fdtoverlay)
+
+$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
+	$(call if_changed,fdtoverlay)
+
+always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
-- 
2.25.0.rc1.19.g042ed3e048af

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Rob Herring]

On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> Now that fdtoverlay is part of the kernel build, start using it to test
> the unitest overlays we have by applying them statically.

Nice idea.

> The file overlay_base.dtb have symbols of its own and we need to apply
> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> us intermediate-overlay.dtb file.

Okay? If restructuring things helps we should do that. Frank?

> The intermediate-overlay.dtb file along with all other overlays is them

s/them/then/

> applied to testcases.dtb to generate the master.dtb file.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> ---
> Depends on:
>
> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@linaro.org/
>
> I have kept the .dtb naming for overlays for now, lets see how we do it
> eventually.
>
> Rob/Frank, this doesn't work properly right now. Maybe I missed how
> these overlays must be applied or there is a bug in fdtoverlay.
>
> The master.dtb doesn't include any nodes from overlay_base.dtb or
> overlay.dtb probably because 'testcase-data-2' node isn't present in
> testcases.dtb and fdtoverlay doesn't allow applying new nodes to the
> root node, i.e. allows new sub-nodes once it gets phandle to the parent
> but nothing can be added to the root node itself. Though I get a feel
> that it works while applying the nodes dynamically and it is expected to
> work here as well.

Sounds like a bug in fdtoverlay to me. Though maybe you need an empty
base tree. An overlay serving as the base is a bit odd so it's
somewhat understandable fdtoverlay couldn't handle that. OTOH,
combining 2 overlays together seems like a valid use.

>
> (And yeah, this is my first serious attempt at updating Makefiles, I am
> sure there is a scope of improvement here :))

Usually I write something and Masahiro rewrites it for me. :)

Rob

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/11/21 9:46 AM, Rob Herring wrote:
> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>> Now that fdtoverlay is part of the kernel build, start using it to test
>> the unitest overlays we have by applying them statically.
> 
> Nice idea.
> 
>> The file overlay_base.dtb have symbols of its own and we need to apply
>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>> us intermediate-overlay.dtb file.
> 
> Okay? If restructuring things helps we should do that. Frank?

I'm a little slow responding to this thread.  After seeing this question, I
responded to the patch email with a NACK (with further explanation in that
response).

-Frank

> 
>> The intermediate-overlay.dtb file along with all other overlays is them
> 
> s/them/then/
> 
>> applied to testcases.dtb to generate the master.dtb file.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> ---
>> Depends on:
>>
>> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@linaro.org/
>>
>> I have kept the .dtb naming for overlays for now, lets see how we do it
>> eventually.
>>
>> Rob/Frank, this doesn't work properly right now. Maybe I missed how
>> these overlays must be applied or there is a bug in fdtoverlay.
>>
>> The master.dtb doesn't include any nodes from overlay_base.dtb or
>> overlay.dtb probably because 'testcase-data-2' node isn't present in
>> testcases.dtb and fdtoverlay doesn't allow applying new nodes to the
>> root node, i.e. allows new sub-nodes once it gets phandle to the parent
>> but nothing can be added to the root node itself. Though I get a feel
>> that it works while applying the nodes dynamically and it is expected to
>> work here as well.
> 
> Sounds like a bug in fdtoverlay to me. Though maybe you need an empty
> base tree. An overlay serving as the base is a bit odd so it's
> somewhat understandable fdtoverlay couldn't handle that. OTOH,
> combining 2 overlays together seems like a valid use.
> 
>>
>> (And yeah, this is my first serious attempt at updating Makefiles, I am
>> sure there is a scope of improvement here :))
> 
> Usually I write something and Masahiro rewrites it for me. :)
> 
> Rob
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 11-01-21, 09:46, Rob Herring wrote:
> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > Now that fdtoverlay is part of the kernel build, start using it to test
> > the unitest overlays we have by applying them statically.
> 
> Nice idea.
> 
> > The file overlay_base.dtb have symbols of its own and we need to apply
> > overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> > us intermediate-overlay.dtb file.
> 
> Okay? If restructuring things helps we should do that. Frank?

Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
and include it from testcases.dts like the other ones ?

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Rob Herring]

On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 11-01-21, 09:46, Rob Herring wrote:
> > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > Now that fdtoverlay is part of the kernel build, start using it to test
> > > the unitest overlays we have by applying them statically.
> >
> > Nice idea.
> >
> > > The file overlay_base.dtb have symbols of its own and we need to apply
> > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> > > us intermediate-overlay.dtb file.
> >
> > Okay? If restructuring things helps we should do that. Frank?
>
> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
> and include it from testcases.dts like the other ones ?

No, because overlay_base.dts is an overlay dt. I think we need an
empty, minimal base.dtb to apply overlay_base.dtbo to.

And then fdtoverlay needs a fix to apply overlays to the root node?

Rob

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

+David,

On 14-01-21, 09:01, Rob Herring wrote:
> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 11-01-21, 09:46, Rob Herring wrote:
> > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > >
> > > > Now that fdtoverlay is part of the kernel build, start using it to test
> > > > the unitest overlays we have by applying them statically.
> > >
> > > Nice idea.
> > >
> > > > The file overlay_base.dtb have symbols of its own and we need to apply
> > > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> > > > us intermediate-overlay.dtb file.
> > >
> > > Okay? If restructuring things helps we should do that. Frank?
> >
> > Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
> > and include it from testcases.dts like the other ones ?
> 
> No, because overlay_base.dts is an overlay dt.

What property of a file makes it an overlay ? Just the presence of /plugin/; ?

David, we are talking about the overlay base[1] file here. The fdtoverlay tool
fails to apply it to testcases.dts file (in the same directory) because none of
its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
skips them intentionally.

> I think we need an
> empty, minimal base.dtb to apply overlay_base.dtbo to.

One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
will make it work.

> And then fdtoverlay needs a fix to apply overlays to the root node?

It isn't just root node I think, but any node for which the __overlay__ label
isn't there.

So this can make it all work if everyone is fine with it:

diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..59172c4c9e5a 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -11,8 +11,7 @@
  * dtc will create nodes "/__symbols__" and "/__local_fixups__".
  */
 
-/ {
-       testcase-data-2 {
+       &overlay_base {
                #address-cells = <1>;
                #size-cells = <1>;
 
@@ -89,5 +88,3 @@ retail_1: vending@50000 {
                };
 
        };
-};
-
diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
index a85b5e1c381a..539dc7d9eddc 100644
--- a/drivers/of/unittest-data/testcases.dts
+++ b/drivers/of/unittest-data/testcases.dts
@@ -11,6 +11,11 @@ node-remove {
                        };
                };
        };
+
+       overlay_base: testcase-data-2 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+       };

-------------------------8<-------------------------

And then we can do this to the Makefile over my changes.

-------------------------8<-------------------------

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index 9f3eb30b78f1..8cc23311b778 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
 
 # Apply all overlays (except overlay_bad_* as they are not supposed to apply and
 # fail build) statically with fdtoverlay
-intermediate-overlay   := overlay.dtb
 master                 := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
                           overlay_3.dtb overlay_4.dtb overlay_5.dtb \
                           overlay_6.dtb overlay_7.dtb overlay_8.dtb \
@@ -50,15 +49,12 @@ master                      := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
                           overlay_gpio_01.dtb overlay_gpio_02a.dtb \
                           overlay_gpio_02b.dtb overlay_gpio_03.dtb \
                           overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
-                          intermediate-overlay.dtb
+                          overlay_base.dtb overlay.dtb
 
 quiet_cmd_fdtoverlay = fdtoverlay $@
       cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
 
-$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
-       $(call if_changed,fdtoverlay)
-
 $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
        $(call if_changed,fdtoverlay)
 
-always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
+always-$(CONFIG_OF_OVERLAY) += master.dtb

-- 
viresh

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/of/unittest-data/overlay_base.dts
[2] https://git.kernel.org/pub/scm/utils/dtc/dtc.git/tree/libfdt/fdt_overlay.c#n628

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

Hi Viresh,

On 1/14/21 11:44 PM, Viresh Kumar wrote:
> +David,
> 
> On 14-01-21, 09:01, Rob Herring wrote:
>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>
>>> On 11-01-21, 09:46, Rob Herring wrote:
>>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>>
>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>>>> the unitest overlays we have by applying them statically.
>>>>
>>>> Nice idea.
>>>>
>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>> us intermediate-overlay.dtb file.
>>>>
>>>> Okay? If restructuring things helps we should do that. Frank?
>>>
>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
>>> and include it from testcases.dts like the other ones ?

I was not able to look at this until tonight.  The unittest world is somewhat
convoluted and complex.  Not at all a normal OF environment since it is directly
using both dynamic OF code and overlay apply/remove code.  Not to mention
deliberately misformed devicetree (.dts) data.  And totally hacking the loading
of FDTs in additional ways.

It is late Sunday night here (almost 10:00pm), so I am going to look at this
first thing Monday morning.

>>
>> No, because overlay_base.dts is an overlay dt.
> 
> What property of a file makes it an overlay ? Just the presence of /plugin/; ?

The "/plugin/;" in a dts file is what tells the dtc compiler to process the source
file as an overlay instead of as a base.

> 
> David, we are talking about the overlay base[1] file here. The fdtoverlay tool
> fails to apply it to testcases.dts file (in the same directory) because none of
> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
> skips them intentionally.
> 
>> I think we need an
>> empty, minimal base.dtb to apply overlay_base.dtbo to.
> 
> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
> will make it work.
> 
>> And then fdtoverlay needs a fix to apply overlays to the root node?
> 
> It isn't just root node I think, but any node for which the __overlay__ label
> isn't there.
> 
> So this can make it all work if everyone is fine with it:

I'll look this over Monday morning to see what the side effects are in the
bizarre world of unittest.

> 
> diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
> index 99ab9d12d00b..59172c4c9e5a 100644
> --- a/drivers/of/unittest-data/overlay_base.dts
> +++ b/drivers/of/unittest-data/overlay_base.dts
> @@ -11,8 +11,7 @@
>   * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>   */
>  
> -/ {
> -       testcase-data-2 {
> +       &overlay_base {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>  
> @@ -89,5 +88,3 @@ retail_1: vending@50000 {
>                 };
>  
>         };
> -};
> -
> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
> index a85b5e1c381a..539dc7d9eddc 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -11,6 +11,11 @@ node-remove {
>                         };
>                 };
>         };
> +
> +       overlay_base: testcase-data-2 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +       };
> 
> -------------------------8<-------------------------
> 
> And then we can do this to the Makefile over my changes.
> 
> -------------------------8<-------------------------
> 
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index 9f3eb30b78f1..8cc23311b778 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
>  
>  # Apply all overlays (except overlay_bad_* as they are not supposed to apply and
>  # fail build) statically with fdtoverlay
> -intermediate-overlay   := overlay.dtb
>  master                 := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>                            overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>                            overlay_6.dtb overlay_7.dtb overlay_8.dtb \
> @@ -50,15 +49,12 @@ master                      := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>                            overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>                            overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>                            overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
> -                          intermediate-overlay.dtb
> +                          overlay_base.dtb overlay.dtb
>  
>  quiet_cmd_fdtoverlay = fdtoverlay $@
>        cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>  
> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
> -       $(call if_changed,fdtoverlay)
> -
>  $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
>         $(call if_changed,fdtoverlay)
>  
> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
> +always-$(CONFIG_OF_OVERLAY) += master.dtb
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/17/21 9:54 PM, Frank Rowand wrote:
> Hi Viresh,
> 
> On 1/14/21 11:44 PM, Viresh Kumar wrote:
>> +David,
>>
>> On 14-01-21, 09:01, Rob Herring wrote:
>>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>
>>>> On 11-01-21, 09:46, Rob Herring wrote:
>>>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>>>
>>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>>>>> the unitest overlays we have by applying them statically.
>>>>>
>>>>> Nice idea.
>>>>>
>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>>> us intermediate-overlay.dtb file.
>>>>>
>>>>> Okay? If restructuring things helps we should do that. Frank?
>>>>
>>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
>>>> and include it from testcases.dts like the other ones ?
> 
> I was not able to look at this until tonight.  The unittest world is somewhat
> convoluted and complex.  Not at all a normal OF environment since it is directly
> using both dynamic OF code and overlay apply/remove code.  Not to mention
> deliberately misformed devicetree (.dts) data.  And totally hacking the loading
> of FDTs in additional ways.
> 
> It is late Sunday night here (almost 10:00pm), so I am going to look at this
> first thing Monday morning.

I sent comments in the form of a patch to the original patch email.

-Frank

> 
>>>
>>> No, because overlay_base.dts is an overlay dt.
>>
>> What property of a file makes it an overlay ? Just the presence of /plugin/; ?
> 
> The "/plugin/;" in a dts file is what tells the dtc compiler to process the source
> file as an overlay instead of as a base.
> 
>>
>> David, we are talking about the overlay base[1] file here. The fdtoverlay tool
>> fails to apply it to testcases.dts file (in the same directory) because none of
>> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
>> skips them intentionally.
>>
>>> I think we need an
>>> empty, minimal base.dtb to apply overlay_base.dtbo to.
>>
>> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
>> will make it work.
>>
>>> And then fdtoverlay needs a fix to apply overlays to the root node?
>>
>> It isn't just root node I think, but any node for which the __overlay__ label
>> isn't there.
>>
>> So this can make it all work if everyone is fine with it:
> 
> I'll look this over Monday morning to see what the side effects are in the
> bizarre world of unittest.
> 
>>
>> diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
>> index 99ab9d12d00b..59172c4c9e5a 100644
>> --- a/drivers/of/unittest-data/overlay_base.dts
>> +++ b/drivers/of/unittest-data/overlay_base.dts
>> @@ -11,8 +11,7 @@
>>   * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>>   */
>>  
>> -/ {
>> -       testcase-data-2 {
>> +       &overlay_base {
>>                 #address-cells = <1>;
>>                 #size-cells = <1>;
>>  
>> @@ -89,5 +88,3 @@ retail_1: vending@50000 {
>>                 };
>>  
>>         };
>> -};
>> -
>> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
>> index a85b5e1c381a..539dc7d9eddc 100644
>> --- a/drivers/of/unittest-data/testcases.dts
>> +++ b/drivers/of/unittest-data/testcases.dts
>> @@ -11,6 +11,11 @@ node-remove {
>>                         };
>>                 };
>>         };
>> +
>> +       overlay_base: testcase-data-2 {
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +       };
>>
>> -------------------------8<-------------------------
>>
>> And then we can do this to the Makefile over my changes.
>>
>> -------------------------8<-------------------------
>>
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index 9f3eb30b78f1..8cc23311b778 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
>>  
>>  # Apply all overlays (except overlay_bad_* as they are not supposed to apply and
>>  # fail build) statically with fdtoverlay
>> -intermediate-overlay   := overlay.dtb
>>  master                 := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>>                            overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>>                            overlay_6.dtb overlay_7.dtb overlay_8.dtb \
>> @@ -50,15 +49,12 @@ master                      := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>>                            overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>>                            overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>>                            overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
>> -                          intermediate-overlay.dtb
>> +                          overlay_base.dtb overlay.dtb
>>  
>>  quiet_cmd_fdtoverlay = fdtoverlay $@
>>        cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>>  
>> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
>> -       $(call if_changed,fdtoverlay)
>> -
>>  $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
>>         $(call if_changed,fdtoverlay)
>>  
>> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
>> +always-$(CONFIG_OF_OVERLAY) += master.dtb
>>
> 
> .
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

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

On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote:
> +David,
> 
> On 14-01-21, 09:01, Rob Herring wrote:
> > On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 11-01-21, 09:46, Rob Herring wrote:
> > > > On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > > > >
> > > > > Now that fdtoverlay is part of the kernel build, start using it to test
> > > > > the unitest overlays we have by applying them statically.
> > > >
> > > > Nice idea.
> > > >
> > > > > The file overlay_base.dtb have symbols of its own and we need to apply
> > > > > overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> > > > > us intermediate-overlay.dtb file.
> > > >
> > > > Okay? If restructuring things helps we should do that. Frank?
> > >
> > > Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
> > > and include it from testcases.dts like the other ones ?
> > 
> > No, because overlay_base.dts is an overlay dt.

So.. I was confused for a bit here, because the overlay_base.dts
you're discussing is the one in the kernel tree, not the one in the
dtc tree.

The kernel file is confusing to me.  It has the /plugin/ tag which
should be used for overlays, but the rest of the file looks like a
base tree rather than an overlay.  There's even a comment saying "Base
device tree that overlays will be applied against".  But it goes on to
talk about __fixups__ and __local__fixups__ which will never be
generated for a based tree.

Oh.. and then there's terminology confusion.  dtc has had compile time
resolved overlays for a very long time.  Those are usually .dtsi
files, and should generally not have /plugin/.

More recent is the support for run-time overlays - .dtbo output files,
which are usually generated from a .dts with /plugin/.  They usually
should *not* have a "/ { ... } " stanza, since that indicates the base
portion of the tree.

> What property of a file makes it an overlay ? Just the presence of
> /plugin/; ?

Yes and no.  Generally that's how it should work , but it looks to me
like the presence of /plugin/ there is just wrong.  /plugin/ basically
just activates some extra dtc features, though, so it is possible to
"manually" construct a valid overlay without it.

> David, we are talking about the overlay base[1] file here. The
> fdtoverlay tool
> fails to apply it to testcases.dts file (in the same directory) because none of
> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
> skips them intentionally.

testcases.dts also has /plugin/ and again, it's not really clear if
that's right.

The point of /plugin/ is that it will automatically generate the
__overlay__ subnodes from dtc's &label { ... } or &{/path} { ... }
syntax, so you wouldn't expect to see __overlay__ in the source.

> > I think we need an
> > empty, minimal base.dtb to apply overlay_base.dtbo to.
> 
> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
> will make it work.
> 
> > And then fdtoverlay needs a fix to apply overlays to the root node?
> 
> It isn't just root node I think, but any node for which the __overlay__ label
> isn't there.
> 
> So this can make it all work if everyone is fine with it:
> 
> diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
> index 99ab9d12d00b..59172c4c9e5a 100644
> --- a/drivers/of/unittest-data/overlay_base.dts
> +++ b/drivers/of/unittest-data/overlay_base.dts
> @@ -11,8 +11,7 @@
>   * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>   */
>  
> -/ {
> -       testcase-data-2 {
> +       &overlay_base {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
>  
> @@ -89,5 +88,3 @@ retail_1: vending@50000 {
>                 };
>  
>         };
> -};
> -
> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
> index a85b5e1c381a..539dc7d9eddc 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -11,6 +11,11 @@ node-remove {
>                         };
>                 };
>         };
> +
> +       overlay_base: testcase-data-2 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +       };
> 
> -------------------------8<-------------------------
> 
> And then we can do this to the Makefile over my changes.
> 
> -------------------------8<-------------------------
> 
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index 9f3eb30b78f1..8cc23311b778 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
>  
>  # Apply all overlays (except overlay_bad_* as they are not supposed to apply and
>  # fail build) statically with fdtoverlay
> -intermediate-overlay   := overlay.dtb
>  master                 := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>                            overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>                            overlay_6.dtb overlay_7.dtb overlay_8.dtb \
> @@ -50,15 +49,12 @@ master                      := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>                            overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>                            overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>                            overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
> -                          intermediate-overlay.dtb
> +                          overlay_base.dtb overlay.dtb
>  
>  quiet_cmd_fdtoverlay = fdtoverlay $@
>        cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>  
> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
> -       $(call if_changed,fdtoverlay)
> -
>  $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
>         $(call if_changed,fdtoverlay)
>  
> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
> +always-$(CONFIG_OF_OVERLAY) += master.dtb
> 

-- 
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: 833 bytes --]

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/18/21 12:30 AM, David Gibson wrote:
> On Fri, Jan 15, 2021 at 11:14:50AM +0530, Viresh Kumar wrote:
>> +David,
>>
>> On 14-01-21, 09:01, Rob Herring wrote:
>>> On Wed, Jan 13, 2021 at 11:03 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>
>>>> On 11-01-21, 09:46, Rob Herring wrote:
>>>>> On Fri, Jan 8, 2021 at 2:41 AM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>>>>>
>>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>>>>> the unitest overlays we have by applying them statically.
>>>>>
>>>>> Nice idea.
>>>>>
>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>>> us intermediate-overlay.dtb file.
>>>>>
>>>>> Okay? If restructuring things helps we should do that. Frank?
>>>>
>>>> Frank, do we want to do something about it ? Maybe make overlay_base.dts an dtsi
>>>> and include it from testcases.dts like the other ones ?
>>>
>>> No, because overlay_base.dts is an overlay dt.
> 
> So.. I was confused for a bit here, because the overlay_base.dts
> you're discussing is the one in the kernel tree, not the one in the
> dtc tree.
> 
> The kernel file is confusing to me.  It has the /plugin/ tag which

It should be confusing to you, without having dug deeply into the
evil things that unittest does.  Unittest does a bunch of contortions
and abuse of direct access into the kernel devicetree code internals
to be able to create and test for abnormal conditions.  No other code
should emulate the bizarre things that unittest does.

-Frank

> should be used for overlays, but the rest of the file looks like a
> base tree rather than an overlay.  There's even a comment saying "Base
> device tree that overlays will be applied against".  But it goes on to
> talk about __fixups__ and __local__fixups__ which will never be
> generated for a based tree.
> 
> Oh.. and then there's terminology confusion.  dtc has had compile time
> resolved overlays for a very long time.  Those are usually .dtsi
> files, and should generally not have /plugin/.
> 
> More recent is the support for run-time overlays - .dtbo output files,
> which are usually generated from a .dts with /plugin/.  They usually
> should *not* have a "/ { ... } " stanza, since that indicates the base
> portion of the tree.
> 
>> What property of a file makes it an overlay ? Just the presence of
>> /plugin/; ?
> 
> Yes and no.  Generally that's how it should work , but it looks to me
> like the presence of /plugin/ there is just wrong.  /plugin/ basically
> just activates some extra dtc features, though, so it is possible to
> "manually" construct a valid overlay without it.
> 
>> David, we are talking about the overlay base[1] file here. The
>> fdtoverlay tool
>> fails to apply it to testcases.dts file (in the same directory) because none of
>> its nodes have the __overlay__ label and the dtc routine overlay_merge() [2]
>> skips them intentionally.
> 
> testcases.dts also has /plugin/ and again, it's not really clear if
> that's right.
> 
> The point of /plugin/ is that it will automatically generate the
> __overlay__ subnodes from dtc's &label { ... } or &{/path} { ... }
> syntax, so you wouldn't expect to see __overlay__ in the source.
> 
>>> I think we need an
>>> empty, minimal base.dtb to apply overlay_base.dtbo to.
>>
>> One way out is adding an (almost-empty) testcase-data-2 in testcases.dtb, that
>> will make it work.
>>
>>> And then fdtoverlay needs a fix to apply overlays to the root node?
>>
>> It isn't just root node I think, but any node for which the __overlay__ label
>> isn't there.
>>
>> So this can make it all work if everyone is fine with it:
>>
>> diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
>> index 99ab9d12d00b..59172c4c9e5a 100644
>> --- a/drivers/of/unittest-data/overlay_base.dts
>> +++ b/drivers/of/unittest-data/overlay_base.dts
>> @@ -11,8 +11,7 @@
>>   * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>>   */
>>  
>> -/ {
>> -       testcase-data-2 {
>> +       &overlay_base {
>>                 #address-cells = <1>;
>>                 #size-cells = <1>;
>>  
>> @@ -89,5 +88,3 @@ retail_1: vending@50000 {
>>                 };
>>  
>>         };
>> -};
>> -
>> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
>> index a85b5e1c381a..539dc7d9eddc 100644
>> --- a/drivers/of/unittest-data/testcases.dts
>> +++ b/drivers/of/unittest-data/testcases.dts
>> @@ -11,6 +11,11 @@ node-remove {
>>                         };
>>                 };
>>         };
>> +
>> +       overlay_base: testcase-data-2 {
>> +               #address-cells = <1>;
>> +               #size-cells = <1>;
>> +       };
>>
>> -------------------------8<-------------------------
>>
>> And then we can do this to the Makefile over my changes.
>>
>> -------------------------8<-------------------------
>>
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index 9f3eb30b78f1..8cc23311b778 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -41,7 +41,6 @@ DTC_FLAGS_testcases += -Wno-interrupts_property
>>  
>>  # Apply all overlays (except overlay_bad_* as they are not supposed to apply and
>>  # fail build) statically with fdtoverlay
>> -intermediate-overlay   := overlay.dtb
>>  master                 := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>>                            overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>>                            overlay_6.dtb overlay_7.dtb overlay_8.dtb \
>> @@ -50,15 +49,12 @@ master                      := overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>>                            overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>>                            overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>>                            overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
>> -                          intermediate-overlay.dtb
>> +                          overlay_base.dtb overlay.dtb
>>  
>>  quiet_cmd_fdtoverlay = fdtoverlay $@
>>        cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>>  
>> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
>> -       $(call if_changed,fdtoverlay)
>> -
>>  $(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
>>         $(call if_changed,fdtoverlay)
>>  
>> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
>> +always-$(CONFIG_OF_OVERLAY) += master.dtb
>>
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/8/21 2:41 AM, Viresh Kumar wrote:
> Now that fdtoverlay is part of the kernel build, start using it to test
> the unitest overlays we have by applying them statically.
> 
> The file overlay_base.dtb have symbols of its own and we need to apply
> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> us intermediate-overlay.dtb file.
> 
> The intermediate-overlay.dtb file along with all other overlays is them
> applied to testcases.dtb to generate the master.dtb file.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

NACK to this specific patch, in its current form.

There are restrictions on applying an overlay at runtime that do not apply
to applying an overlay to an FDT that will be loaded by the kernel during
early boot.  Thus the unittest overlays _must_ be applied using the kernel
overlay loading methods to test the kernel runtime overlay loading feature.

I agree that testing fdtoverlay is a good idea.  I have not looked at the
parent project to see how much testing of fdtoverlay occurs there, but I
would prefer that fdtoverlay tests reside in the parent project if practical
and reasonable.  If there is some reason that some fdtoverlay tests are
more practical in the Linux kernel repository then I am open to adding
them to the Linux kernel tree.

-Frank


> 
> ---
> Depends on:
> 
> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@linaro.org/
> 
> I have kept the .dtb naming for overlays for now, lets see how we do it
> eventually.
> 
> Rob/Frank, this doesn't work properly right now. Maybe I missed how
> these overlays must be applied or there is a bug in fdtoverlay.
> 
> The master.dtb doesn't include any nodes from overlay_base.dtb or
> overlay.dtb probably because 'testcase-data-2' node isn't present in
> testcases.dtb and fdtoverlay doesn't allow applying new nodes to the
> root node, i.e. allows new sub-nodes once it gets phandle to the parent
> but nothing can be added to the root node itself. Though I get a feel
> that it works while applying the nodes dynamically and it is expected to
> work here as well.
> 
> (And yeah, this is my first serious attempt at updating Makefiles, I am
> sure there is a scope of improvement here :))
> 
> ---
>  drivers/of/unittest-data/Makefile | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index 009f4045c8e4..f17bce85f65f 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@
>  
>  # suppress warnings about intentional errors
>  DTC_FLAGS_testcases += -Wno-interrupts_property
> +
> +# Apply overlays statically with fdtoverlay
> +intermediate-overlay	:= overlay.dtb
> +master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
> +			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
> +			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
> +			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
> +			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
> +			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
> +			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
> +			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
> +			   intermediate-overlay.dtb
> +
> +quiet_cmd_fdtoverlay = fdtoverlay $@
> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
> +
> +$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
> +	$(call if_changed,fdtoverlay)
> +
> +$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
> +	$(call if_changed,fdtoverlay)
> +
> +always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Bill Mills]

On 1/11/21 5:06 PM, Frank Rowand wrote:
> On 1/8/21 2:41 AM, Viresh Kumar wrote:
>> Now that fdtoverlay is part of the kernel build, start using it to test
>> the unitest overlays we have by applying them statically.
>>
>> The file overlay_base.dtb have symbols of its own and we need to apply
>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>> us intermediate-overlay.dtb file.
>>
>> The intermediate-overlay.dtb file along with all other overlays is them
>> applied to testcases.dtb to generate the master.dtb file.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> NACK to this specific patch, in its current form.
> 
> There are restrictions on applying an overlay at runtime that do not apply
> to applying an overlay to an FDT that will be loaded by the kernel during
> early boot.  Thus the unittest overlays _must_ be applied using the kernel
> overlay loading methods to test the kernel runtime overlay loading feature.
> 
> I agree that testing fdtoverlay is a good idea.  I have not looked at the
> parent project to see how much testing of fdtoverlay occurs there, but I
> would prefer that fdtoverlay tests reside in the parent project if practical
> and reasonable.  If there is some reason that some fdtoverlay tests are
> more practical in the Linux kernel repository then I am open to adding
> them to the Linux kernel tree.
> 

Frank,

I thought we were aligned that any new overlays into the kernel today 
would only be for boot loader applied case.  Applying overlays at kernel 
runtime was out of scope at your request.

Rob had requested that the overlays be test applied at build time.  I 
don't think there is any way to test the kernel runtime method at build 
time correct?

Please clarify your concern and your suggested way forward.

Thanks,
Bill

> -Frank
> 
> 
>>
>> ---
>> Depends on:
>>
>> https://lore.kernel.org/lkml/be5cb12a68d9ac2c35ad9dd50d6b168f7cad6837.1609996381.git.viresh.kumar@linaro.org/
>>
>> I have kept the .dtb naming for overlays for now, lets see how we do it
>> eventually.
>>
>> Rob/Frank, this doesn't work properly right now. Maybe I missed how
>> these overlays must be applied or there is a bug in fdtoverlay.
>>
>> The master.dtb doesn't include any nodes from overlay_base.dtb or
>> overlay.dtb probably because 'testcase-data-2' node isn't present in
>> testcases.dtb and fdtoverlay doesn't allow applying new nodes to the
>> root node, i.e. allows new sub-nodes once it gets phandle to the parent
>> but nothing can be added to the root node itself. Though I get a feel
>> that it works while applying the nodes dynamically and it is expected to
>> work here as well.
>>
>> (And yeah, this is my first serious attempt at updating Makefiles, I am
>> sure there is a scope of improvement here :))
>>
>> ---
>>   drivers/of/unittest-data/Makefile | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index 009f4045c8e4..f17bce85f65f 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@
>>   
>>   # suppress warnings about intentional errors
>>   DTC_FLAGS_testcases += -Wno-interrupts_property
>> +
>> +# Apply overlays statically with fdtoverlay
>> +intermediate-overlay	:= overlay.dtb
>> +master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>> +			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>> +			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
>> +			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
>> +			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
>> +			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>> +			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>> +			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
>> +			   intermediate-overlay.dtb
>> +
>> +quiet_cmd_fdtoverlay = fdtoverlay $@
>> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>> +
>> +$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
>> +	$(call if_changed,fdtoverlay)
>> +
>> +$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
>> +	$(call if_changed,fdtoverlay)
>> +
>> +always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
>>
> 

-- 
Bill Mills
Principal Technical Consultant, Linaro
+1-240-643-0836
TZ: US Eastern
Work Schedule:  Tues/Wed/Thur

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 11-01-21, 20:22, Bill Mills wrote:
> On 1/11/21 5:06 PM, Frank Rowand wrote:
> > NACK to this specific patch, in its current form.
> > 
> > There are restrictions on applying an overlay at runtime that do not apply
> > to applying an overlay to an FDT that will be loaded by the kernel during
> > early boot.  Thus the unittest overlays _must_ be applied using the kernel
> > overlay loading methods to test the kernel runtime overlay loading feature.
> > 
> > I agree that testing fdtoverlay is a good idea.  I have not looked at the
> > parent project to see how much testing of fdtoverlay occurs there, but I
> > would prefer that fdtoverlay tests reside in the parent project if practical
> > and reasonable.  If there is some reason that some fdtoverlay tests are
> > more practical in the Linux kernel repository then I am open to adding
> > them to the Linux kernel tree.

I wasn't looking to add any testing for fdtoverlay in the kernel, but
then I stumbled upon unit-tests here and thought it would be a good
idea to get this built using static tools as well, as we aren't
required to add any new source files for this and the existing tests
already cover a lot of nodes.

And so I am fine if we don't want to do this stuff in kernel.

> I thought we were aligned that any new overlays into the kernel today would
> only be for boot loader applied case.  Applying overlays at kernel runtime
> was out of scope at your request.
> 
> Rob had requested that the overlays be test applied at build time.  I don't
> think there is any way to test the kernel runtime method at build time
> correct?
> 
> Please clarify your concern and your suggested way forward.

The kernel does some overlay testing currently (at kernel boot only,
not later), to see if the overlays get applied correctly or not, these
are the unit tests.

What Frank is probably saying is that the unit-tests dtbs shouldn't
get used for testing fdtoverlay stuff. He isn't asking to support
runtime application of overlays, but to not do fdtoverlay testing in
the kernel.

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Bill Mills]

On 1/12/21 3:37 AM, Viresh Kumar wrote:
> On 11-01-21, 20:22, Bill Mills wrote:
>> On 1/11/21 5:06 PM, Frank Rowand wrote:
>>> NACK to this specific patch, in its current form.
>>>
>>> There are restrictions on applying an overlay at runtime that do not apply
>>> to applying an overlay to an FDT that will be loaded by the kernel during
>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
>>> overlay loading methods to test the kernel runtime overlay loading feature.
>>>
>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
>>> parent project to see how much testing of fdtoverlay occurs there, but I
>>> would prefer that fdtoverlay tests reside in the parent project if practical
>>> and reasonable.  If there is some reason that some fdtoverlay tests are
>>> more practical in the Linux kernel repository then I am open to adding
>>> them to the Linux kernel tree.
> 
> I wasn't looking to add any testing for fdtoverlay in the kernel, but
> then I stumbled upon unit-tests here and thought it would be a good
> idea to get this built using static tools as well, as we aren't
> required to add any new source files for this and the existing tests
> already cover a lot of nodes.
> 
> And so I am fine if we don't want to do this stuff in kernel.
> 
>> I thought we were aligned that any new overlays into the kernel today would
>> only be for boot loader applied case.  Applying overlays at kernel runtime
>> was out of scope at your request.
>>
>> Rob had requested that the overlays be test applied at build time.  I don't
>> think there is any way to test the kernel runtime method at build time
>> correct?
>>
>> Please clarify your concern and your suggested way forward.
> 
> The kernel does some overlay testing currently (at kernel boot only,
> not later), to see if the overlays get applied correctly or not, these
> are the unit tests.
> 
> What Frank is probably saying is that the unit-tests dtbs shouldn't
> get used for testing fdtoverlay stuff. He isn't asking to support
> runtime application of overlays, but to not do fdtoverlay testing in
> the kernel.
> 

Thanks Viresh, that makes sense.  Sorry for the confusion Frank.

-- 
Bill Mills

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/12/21 4:16 AM, Bill Mills wrote:
> 
> 
> On 1/12/21 3:37 AM, Viresh Kumar wrote:
>> On 11-01-21, 20:22, Bill Mills wrote:
>>> On 1/11/21 5:06 PM, Frank Rowand wrote:
>>>> NACK to this specific patch, in its current form.
>>>>
>>>> There are restrictions on applying an overlay at runtime that do not apply
>>>> to applying an overlay to an FDT that will be loaded by the kernel during
>>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
>>>> overlay loading methods to test the kernel runtime overlay loading feature.
>>>>
>>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
>>>> parent project to see how much testing of fdtoverlay occurs there, but I
>>>> would prefer that fdtoverlay tests reside in the parent project if practical
>>>> and reasonable.  If there is some reason that some fdtoverlay tests are
>>>> more practical in the Linux kernel repository then I am open to adding
>>>> them to the Linux kernel tree.
>>
>> I wasn't looking to add any testing for fdtoverlay in the kernel, but
>> then I stumbled upon unit-tests here and thought it would be a good
>> idea to get this built using static tools as well, as we aren't
>> required to add any new source files for this and the existing tests
>> already cover a lot of nodes.
>>
>> And so I am fine if we don't want to do this stuff in kernel.
>>
>>> I thought we were aligned that any new overlays into the kernel today would
>>> only be for boot loader applied case.  Applying overlays at kernel runtime
>>> was out of scope at your request.
>>>
>>> Rob had requested that the overlays be test applied at build time.  I don't
>>> think there is any way to test the kernel runtime method at build time
>>> correct?
>>>
>>> Please clarify your concern and your suggested way forward.
>>
>> The kernel does some overlay testing currently (at kernel boot only,
>> not later), to see if the overlays get applied correctly or not, these
>> are the unit tests.
>>
>> What Frank is probably saying is that the unit-tests dtbs shouldn't
>> get used for testing fdtoverlay stuff. He isn't asking to support
>> runtime application of overlays, but to not do fdtoverlay testing in
>> the kernel.

Yes, Viresh is understanding my point.

>>
> 
> Thanks Viresh, that makes sense.  Sorry for the confusion Frank.

No problem Bill, communication by email is hard, and we end up
spending a lot of time getting to the point of common understanding,
it is the nature of the process.

-Frank

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Rob Herring]

On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 1/8/21 2:41 AM, Viresh Kumar wrote:
> > Now that fdtoverlay is part of the kernel build, start using it to test
> > the unitest overlays we have by applying them statically.
> >
> > The file overlay_base.dtb have symbols of its own and we need to apply
> > overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> > us intermediate-overlay.dtb file.
> >
> > The intermediate-overlay.dtb file along with all other overlays is them
> > applied to testcases.dtb to generate the master.dtb file.
> >
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>
> NACK to this specific patch, in its current form.
>
> There are restrictions on applying an overlay at runtime that do not apply
> to applying an overlay to an FDT that will be loaded by the kernel during
> early boot.  Thus the unittest overlays _must_ be applied using the kernel
> overlay loading methods to test the kernel runtime overlay loading feature.

This patch doesn't take away from any of that and it completely orthogonal.

> I agree that testing fdtoverlay is a good idea.  I have not looked at the
> parent project to see how much testing of fdtoverlay occurs there, but I
> would prefer that fdtoverlay tests reside in the parent project if practical
> and reasonable.  If there is some reason that some fdtoverlay tests are
> more practical in the Linux kernel repository then I am open to adding
> them to the Linux kernel tree.

If you (or more importantly someone else sending us patches) make
changes to the overlays, you can test that they apply at build time
rather than runtime. I'll take it! So please help on fixing the issue
because I want to apply this.

And yes, dtc has fdtoverlay tests. But this patch shows there's at
least 2 issues. fdtoverlay can't apply overlays to the root and using
an overlay as the base tree in UML is odd IMO.

Rob

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/12/21 8:04 AM, Rob Herring wrote:
> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 1/8/21 2:41 AM, Viresh Kumar wrote:
>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>> the unitest overlays we have by applying them statically.
>>>
>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>> us intermediate-overlay.dtb file.
>>>
>>> The intermediate-overlay.dtb file along with all other overlays is them
>>> applied to testcases.dtb to generate the master.dtb file.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>
>> NACK to this specific patch, in its current form.
>>
>> There are restrictions on applying an overlay at runtime that do not apply
>> to applying an overlay to an FDT that will be loaded by the kernel during
>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
>> overlay loading methods to test the kernel runtime overlay loading feature.
> 
> This patch doesn't take away from any of that and it completely orthogonal.

Mea culpa.  I took the patch header comment at face value, and read more into
the header comment than what was written there.  I then skimmed the patch
instead of actually reading what it was doing.

I incorrectly _assumed_ (bad!) that the intent was to replace applying the
individual overlay dtb's with the master.dtb.  Reading more closely, I see
that the assumed final step of actually _using_ master.dtb does not exist.

So, yes, I agree that the patch as written is orthogonal to my concern.

My updated understanding is that this patch is attempting to use the existing
unittest overlay dts files as source to test fdtoverlay.  And that the resulting
dtb from fdtoverlay is not intended to be consumed by the kernel unittest.

I do not agree that this is a good approach to testing fdtoverlay.  The
unittest overlay dts files are constructed specifically to test various
parts of the kernel overlay code and dynamic OF code.  Some of the content
of the overlays is constructed to trigger error conditions in that code,
and thus will not be able to be processed without error by fdtoverlay.

Trying to use overlay dts files that are constructed to test runtime kernel
code as fdtoverlay input data mixes two different test environments and
objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
dts files should be created.

> 
>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
>> parent project to see how much testing of fdtoverlay occurs there, but I
>> would prefer that fdtoverlay tests reside in the parent project if practical
>> and reasonable.  If there is some reason that some fdtoverlay tests are
>> more practical in the Linux kernel repository then I am open to adding
>> them to the Linux kernel tree.
> 
> If you (or more importantly someone else sending us patches) make
> changes to the overlays, you can test that they apply at build time
> rather than runtime. I'll take it! So please help on fixing the issue
> because I want to apply this.

If the tests can be added to the upstream project, I would much prefer
they reside there.  If there is some reason a certain test is more
suited to be in the Linux kernel source tree then I also would like
it to be accepted here.

> 
> And yes, dtc has fdtoverlay tests. But this patch shows there's at
> least 2 issues,


> fdtoverlay can't apply overlays to the root 

A test of that definitely belongs in the upstream project.

> and using an overlay as the base tree in UML is odd IMO.

Am I still not fully understanding the patch?  I'm missing how
this patch changes what dtb is used as the base tree in UML.

> 
> Rob
> 

-Frank

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Rob Herring]

On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 1/12/21 8:04 AM, Rob Herring wrote:
> > On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 1/8/21 2:41 AM, Viresh Kumar wrote:
> >>> Now that fdtoverlay is part of the kernel build, start using it to test
> >>> the unitest overlays we have by applying them statically.
> >>>
> >>> The file overlay_base.dtb have symbols of its own and we need to apply
> >>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> >>> us intermediate-overlay.dtb file.
> >>>
> >>> The intermediate-overlay.dtb file along with all other overlays is them
> >>> applied to testcases.dtb to generate the master.dtb file.
> >>>
> >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >>
> >> NACK to this specific patch, in its current form.
> >>
> >> There are restrictions on applying an overlay at runtime that do not apply
> >> to applying an overlay to an FDT that will be loaded by the kernel during
> >> early boot.  Thus the unittest overlays _must_ be applied using the kernel
> >> overlay loading methods to test the kernel runtime overlay loading feature.
> >
> > This patch doesn't take away from any of that and it completely orthogonal.
>
> Mea culpa.  I took the patch header comment at face value, and read more into
> the header comment than what was written there.  I then skimmed the patch
> instead of actually reading what it was doing.
>
> I incorrectly _assumed_ (bad!) that the intent was to replace applying the
> individual overlay dtb's with the master.dtb.  Reading more closely, I see
> that the assumed final step of actually _using_ master.dtb does not exist.
>
> So, yes, I agree that the patch as written is orthogonal to my concern.
>
> My updated understanding is that this patch is attempting to use the existing
> unittest overlay dts files as source to test fdtoverlay.  And that the resulting
> dtb from fdtoverlay is not intended to be consumed by the kernel unittest.

The goal is not to test fdtoverlay. dtc unittests do that. The goal is
testing overlays we expect to be able to apply can actually apply and
doing this at build time. That's also the goal for all the 'real'
overlays which get added.

> I do not agree that this is a good approach to testing fdtoverlay.  The
> unittest overlay dts files are constructed specifically to test various
> parts of the kernel overlay code and dynamic OF code.  Some of the content
> of the overlays is constructed to trigger error conditions in that code,
> and thus will not be able to be processed without error by fdtoverlay.

Then those should be omitted.

> Trying to use overlay dts files that are constructed to test runtime kernel
> code as fdtoverlay input data mixes two different test environments and
> objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
> dts files should be created.
>
> >
> >> I agree that testing fdtoverlay is a good idea.  I have not looked at the
> >> parent project to see how much testing of fdtoverlay occurs there, but I
> >> would prefer that fdtoverlay tests reside in the parent project if practical
> >> and reasonable.  If there is some reason that some fdtoverlay tests are
> >> more practical in the Linux kernel repository then I am open to adding
> >> them to the Linux kernel tree.
> >
> > If you (or more importantly someone else sending us patches) make
> > changes to the overlays, you can test that they apply at build time
> > rather than runtime. I'll take it! So please help on fixing the issue
> > because I want to apply this.
>
> If the tests can be added to the upstream project, I would much prefer
> they reside there.  If there is some reason a certain test is more
> suited to be in the Linux kernel source tree then I also would like
> it to be accepted here.

Again, this is just about doing sanity checks at build time rather
than *only* rely on runtime.

> >
> > And yes, dtc has fdtoverlay tests. But this patch shows there's at
> > least 2 issues,
>
>
> > fdtoverlay can't apply overlays to the root
>
> A test of that definitely belongs in the upstream project.

Yes, agreed.

> > and using an overlay as the base tree in UML is odd IMO.
>
> Am I still not fully understanding the patch?  I'm missing how
> this patch changes what dtb is used as the base tree in UML.

This was more my theorising why Viresh is having problems in that
perhaps fdtoverlay can't take an overlay as a base DT while the kernel
can and does for UML. The fact that it works for UML seems wrong to
me.

Rob

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/12/21 1:41 PM, Rob Herring wrote:
> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 1/12/21 8:04 AM, Rob Herring wrote:
>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 1/8/21 2:41 AM, Viresh Kumar wrote:
>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>>>> the unitest overlays we have by applying them statically.
>>>>>
>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>> us intermediate-overlay.dtb file.
>>>>>
>>>>> The intermediate-overlay.dtb file along with all other overlays is them
>>>>> applied to testcases.dtb to generate the master.dtb file.
>>>>>
>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>
>>>> NACK to this specific patch, in its current form.
>>>>
>>>> There are restrictions on applying an overlay at runtime that do not apply
>>>> to applying an overlay to an FDT that will be loaded by the kernel during
>>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
>>>> overlay loading methods to test the kernel runtime overlay loading feature.
>>>
>>> This patch doesn't take away from any of that and it completely orthogonal.
>>
>> Mea culpa.  I took the patch header comment at face value, and read more into
>> the header comment than what was written there.  I then skimmed the patch
>> instead of actually reading what it was doing.
>>
>> I incorrectly _assumed_ (bad!) that the intent was to replace applying the
>> individual overlay dtb's with the master.dtb.  Reading more closely, I see
>> that the assumed final step of actually _using_ master.dtb does not exist.
>>
>> So, yes, I agree that the patch as written is orthogonal to my concern.
>>
>> My updated understanding is that this patch is attempting to use the existing
>> unittest overlay dts files as source to test fdtoverlay.  And that the resulting
>> dtb from fdtoverlay is not intended to be consumed by the kernel unittest.
> 
> The goal is not to test fdtoverlay. dtc unittests do that. The goal is
> testing overlays we expect to be able to apply can actually apply and
> doing this at build time. That's also the goal for all the 'real'
> overlays which get added.
> 
>> I do not agree that this is a good approach to testing fdtoverlay.  The
>> unittest overlay dts files are constructed specifically to test various
>> parts of the kernel overlay code and dynamic OF code.  Some of the content
>> of the overlays is constructed to trigger error conditions in that code,
>> and thus will not be able to be processed without error by fdtoverlay.
> 
> Then those should be omitted.
> 
>> Trying to use overlay dts files that are constructed to test runtime kernel
>> code as fdtoverlay input data mixes two different test environments and
>> objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
>> dts files should be created.
>>
>>>
>>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
>>>> parent project to see how much testing of fdtoverlay occurs there, but I
>>>> would prefer that fdtoverlay tests reside in the parent project if practical
>>>> and reasonable.  If there is some reason that some fdtoverlay tests are
>>>> more practical in the Linux kernel repository then I am open to adding
>>>> them to the Linux kernel tree.
>>>
>>> If you (or more importantly someone else sending us patches) make
>>> changes to the overlays, you can test that they apply at build time
>>> rather than runtime. I'll take it! So please help on fixing the issue
>>> because I want to apply this.
>>
>> If the tests can be added to the upstream project, I would much prefer
>> they reside there.  If there is some reason a certain test is more
>> suited to be in the Linux kernel source tree then I also would like
>> it to be accepted here.
> 
> Again, this is just about doing sanity checks at build time rather
> than *only* rely on runtime.

I'm fine with adding tests for applying overlays at build time (in
other words, tests of fdtoverlay).

But the constraints on applying an overlay at build time are different
than the runtime constraints.

The existing unittest overlay dts files are not designed to test applying
overlays at build time.  Tests for fdtoverlay should be designed to test
that overlays that meet the build time constraints can be applied
properly by fdtoverlay, and that overlays that fail to meet those
constraints are rejected by fdtoverlay.

Trying to use the same data (dts) files for tests that have different
constraints is likely to make both tests more fragile when a data file
is modified for one environment without careful consideration of the
other environment.

> 
>>>
>>> And yes, dtc has fdtoverlay tests. But this patch shows there's at
>>> least 2 issues,
>>
>>
>>> fdtoverlay can't apply overlays to the root
>>
>> A test of that definitely belongs in the upstream project.
> 
> Yes, agreed.
> 
>>> and using an overlay as the base tree in UML is odd IMO.
>>
>> Am I still not fully understanding the patch?  I'm missing how
>> this patch changes what dtb is used as the base tree in UML.
> 
> This was more my theorising why Viresh is having problems in that
> perhaps fdtoverlay can't take an overlay as a base DT while the kernel
> can and does for UML. The fact that it works for UML seems wrong to
> me.

I'll have to go back and look at UML.  I didn't recall that the base
FDT for UML was an overlay.

-Frank

> 
> Rob
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Rob Herring]

On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 1/12/21 1:41 PM, Rob Herring wrote:
> > On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 1/12/21 8:04 AM, Rob Herring wrote:
> >>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>
> >>>> On 1/8/21 2:41 AM, Viresh Kumar wrote:
> >>>>> Now that fdtoverlay is part of the kernel build, start using it to test
> >>>>> the unitest overlays we have by applying them statically.
> >>>>>
> >>>>> The file overlay_base.dtb have symbols of its own and we need to apply
> >>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> >>>>> us intermediate-overlay.dtb file.
> >>>>>
> >>>>> The intermediate-overlay.dtb file along with all other overlays is them
> >>>>> applied to testcases.dtb to generate the master.dtb file.
> >>>>>
> >>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >>>>
> >>>> NACK to this specific patch, in its current form.
> >>>>
> >>>> There are restrictions on applying an overlay at runtime that do not apply
> >>>> to applying an overlay to an FDT that will be loaded by the kernel during
> >>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
> >>>> overlay loading methods to test the kernel runtime overlay loading feature.
> >>>
> >>> This patch doesn't take away from any of that and it completely orthogonal.
> >>
> >> Mea culpa.  I took the patch header comment at face value, and read more into
> >> the header comment than what was written there.  I then skimmed the patch
> >> instead of actually reading what it was doing.
> >>
> >> I incorrectly _assumed_ (bad!) that the intent was to replace applying the
> >> individual overlay dtb's with the master.dtb.  Reading more closely, I see
> >> that the assumed final step of actually _using_ master.dtb does not exist.
> >>
> >> So, yes, I agree that the patch as written is orthogonal to my concern.
> >>
> >> My updated understanding is that this patch is attempting to use the existing
> >> unittest overlay dts files as source to test fdtoverlay.  And that the resulting
> >> dtb from fdtoverlay is not intended to be consumed by the kernel unittest.
> >
> > The goal is not to test fdtoverlay. dtc unittests do that. The goal is
> > testing overlays we expect to be able to apply can actually apply and
> > doing this at build time. That's also the goal for all the 'real'
> > overlays which get added.
> >
> >> I do not agree that this is a good approach to testing fdtoverlay.  The
> >> unittest overlay dts files are constructed specifically to test various
> >> parts of the kernel overlay code and dynamic OF code.  Some of the content
> >> of the overlays is constructed to trigger error conditions in that code,
> >> and thus will not be able to be processed without error by fdtoverlay.
> >
> > Then those should be omitted.
> >
> >> Trying to use overlay dts files that are constructed to test runtime kernel
> >> code as fdtoverlay input data mixes two different test environments and
> >> objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
> >> dts files should be created.
> >>
> >>>
> >>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
> >>>> parent project to see how much testing of fdtoverlay occurs there, but I
> >>>> would prefer that fdtoverlay tests reside in the parent project if practical
> >>>> and reasonable.  If there is some reason that some fdtoverlay tests are
> >>>> more practical in the Linux kernel repository then I am open to adding
> >>>> them to the Linux kernel tree.
> >>>
> >>> If you (or more importantly someone else sending us patches) make
> >>> changes to the overlays, you can test that they apply at build time
> >>> rather than runtime. I'll take it! So please help on fixing the issue
> >>> because I want to apply this.
> >>
> >> If the tests can be added to the upstream project, I would much prefer
> >> they reside there.  If there is some reason a certain test is more
> >> suited to be in the Linux kernel source tree then I also would like
> >> it to be accepted here.
> >
> > Again, this is just about doing sanity checks at build time rather
> > than *only* rely on runtime.
>
> I'm fine with adding tests for applying overlays at build time (in
> other words, tests of fdtoverlay).

Again, it's not tests of fdtoverlay. It's a test of the dts files. We
are testing that an overlay dts can apply to the base dts we claim it
applies. If the overlay dts has crap then we'll catch it.

We shouldn't accept overlays that can't apply to a base in the kernel
tree. That's either because it's broken or because the base doesn't
exist. With the exception of overlays designed to fail for tests,
unittest overlays should not be any different.

> But the constraints on applying an overlay at build time are different
> than the runtime constraints.

Like what specifically? Runtime is more constrained than build time.
Or at least it should be. It's not really and that's why we have
limited runtime applied overlay support.

> The existing unittest overlay dts files are not designed to test applying
> overlays at build time.  Tests for fdtoverlay should be designed to test
> that overlays that meet the build time constraints can be applied
> properly by fdtoverlay, and that overlays that fail to meet those
> constraints are rejected by fdtoverlay.
>
> Trying to use the same data (dts) files for tests that have different
> constraints is likely to make both tests more fragile when a data file
> is modified for one environment without careful consideration of the
> other environment.

We're not changing nor constraining the data files. Just adding
another sanity test on them.

Rob

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/12/21 2:46 PM, Rob Herring wrote:
> On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 1/12/21 1:41 PM, Rob Herring wrote:
>>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 1/12/21 8:04 AM, Rob Herring wrote:
>>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>
>>>>>> On 1/8/21 2:41 AM, Viresh Kumar wrote:
>>>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>>>>>> the unitest overlays we have by applying them statically.
>>>>>>>
>>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>>>> us intermediate-overlay.dtb file.
>>>>>>>
>>>>>>> The intermediate-overlay.dtb file along with all other overlays is them
>>>>>>> applied to testcases.dtb to generate the master.dtb file.
>>>>>>>
>>>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>
>>>>>> NACK to this specific patch, in its current form.
>>>>>>
>>>>>> There are restrictions on applying an overlay at runtime that do not apply
>>>>>> to applying an overlay to an FDT that will be loaded by the kernel during
>>>>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
>>>>>> overlay loading methods to test the kernel runtime overlay loading feature.
>>>>>
>>>>> This patch doesn't take away from any of that and it completely orthogonal.
>>>>
>>>> Mea culpa.  I took the patch header comment at face value, and read more into
>>>> the header comment than what was written there.  I then skimmed the patch
>>>> instead of actually reading what it was doing.
>>>>
>>>> I incorrectly _assumed_ (bad!) that the intent was to replace applying the
>>>> individual overlay dtb's with the master.dtb.  Reading more closely, I see
>>>> that the assumed final step of actually _using_ master.dtb does not exist.
>>>>
>>>> So, yes, I agree that the patch as written is orthogonal to my concern.
>>>>
>>>> My updated understanding is that this patch is attempting to use the existing
>>>> unittest overlay dts files as source to test fdtoverlay.  And that the resulting
>>>> dtb from fdtoverlay is not intended to be consumed by the kernel unittest.
>>>
>>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is
>>> testing overlays we expect to be able to apply can actually apply and
>>> doing this at build time. That's also the goal for all the 'real'
>>> overlays which get added.
>>>
>>>> I do not agree that this is a good approach to testing fdtoverlay.  The
>>>> unittest overlay dts files are constructed specifically to test various
>>>> parts of the kernel overlay code and dynamic OF code.  Some of the content
>>>> of the overlays is constructed to trigger error conditions in that code,
>>>> and thus will not be able to be processed without error by fdtoverlay.
>>>
>>> Then those should be omitted.
>>>
>>>> Trying to use overlay dts files that are constructed to test runtime kernel
>>>> code as fdtoverlay input data mixes two different test environments and
>>>> objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
>>>> dts files should be created.
>>>>
>>>>>
>>>>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
>>>>>> parent project to see how much testing of fdtoverlay occurs there, but I
>>>>>> would prefer that fdtoverlay tests reside in the parent project if practical
>>>>>> and reasonable.  If there is some reason that some fdtoverlay tests are
>>>>>> more practical in the Linux kernel repository then I am open to adding
>>>>>> them to the Linux kernel tree.
>>>>>
>>>>> If you (or more importantly someone else sending us patches) make
>>>>> changes to the overlays, you can test that they apply at build time
>>>>> rather than runtime. I'll take it! So please help on fixing the issue
>>>>> because I want to apply this.
>>>>
>>>> If the tests can be added to the upstream project, I would much prefer
>>>> they reside there.  If there is some reason a certain test is more
>>>> suited to be in the Linux kernel source tree then I also would like
>>>> it to be accepted here.
>>>
>>> Again, this is just about doing sanity checks at build time rather
>>> than *only* rely on runtime.
>>
>> I'm fine with adding tests for applying overlays at build time (in
>> other words, tests of fdtoverlay).
> 

> Again, it's not tests of fdtoverlay. It's a test of the dts files. We
> are testing that an overlay dts can apply to the base dts we claim it
> applies. If the overlay dts has crap then we'll catch it.
> 
> We shouldn't accept overlays that can't apply to a base in the kernel
> tree. That's either because it's broken or because the base doesn't
> exist. With the exception of overlays designed to fail for tests,
> unittest overlays should not be any different.

I understood the goal to be testing fdtoverlay.  I'll switch my mind
set to the goal being a test of dts files.

We already know that unittest overlays that are expected to be valid
can apply successfully.  The run time unittests already check for that.
I don't see any value in adding a build time test for the same thing
_for unittest overlay dts files_.  And I do see an ongoing maintenance
cost for _unittest overlay dts files_.

If you want to add build time tests for all (or some) non-unittest overlay
dts files, then I am not particularly opposed to that (but being aware that
an overlay dtb could apply on top of more than one base dtb, so there
is a possibility of an "explosion" of combinations to be maintained
in the build system).

I see value in having build time testing that overlay dtbs apply cleanly
on a base dtb.  I have heard frustration from the out of tree users of
overlays that apply the overlays via the bootloader, because if the
bootloader fails to apply an overlay it can be difficult to debug or
fix on the target computer.  Having a mechanism to specify what overlays
are intended to be applied to a base dtb, and verify that they do
apply would resolve some of those issues, assuming the boot loader
and fdtoverlay are consistent with each other.

> 
>> But the constraints on applying an overlay at build time are different
>> than the runtime constraints.
> 
> Like what specifically? Runtime is more constrained than build time.
> Or at least it should be. It's not really and that's why we have
> limited runtime applied overlay support.
> 
>> The existing unittest overlay dts files are not designed to test applying
>> overlays at build time.  Tests for fdtoverlay should be designed to test
>> that overlays that meet the build time constraints can be applied
>> properly by fdtoverlay, and that overlays that fail to meet those
>> constraints are rejected by fdtoverlay.
>>
>> Trying to use the same data (dts) files for tests that have different
>> constraints is likely to make both tests more fragile when a data file
>> is modified for one environment without careful consideration of the
>> other environment.
> 
> We're not changing nor constraining the data files. Just adding
> another sanity test on them.

For _unittest_ dts files, I see no value add.  And the cost of needing
to track _in the build system_ which unittest dts files are expected fail
to apply and which are expected to succeed.

-Frank

> 
> Rob
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Rob Herring]

On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> On 1/12/21 2:46 PM, Rob Herring wrote:
> > On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> On 1/12/21 1:41 PM, Rob Herring wrote:
> >>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>
> >>>> On 1/12/21 8:04 AM, Rob Herring wrote:
> >>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>>>>>
> >>>>>> On 1/8/21 2:41 AM, Viresh Kumar wrote:
> >>>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
> >>>>>>> the unitest overlays we have by applying them statically.
> >>>>>>>
> >>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
> >>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
> >>>>>>> us intermediate-overlay.dtb file.
> >>>>>>>
> >>>>>>> The intermediate-overlay.dtb file along with all other overlays is them
> >>>>>>> applied to testcases.dtb to generate the master.dtb file.
> >>>>>>>
> >>>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> >>>>>>
> >>>>>> NACK to this specific patch, in its current form.
> >>>>>>
> >>>>>> There are restrictions on applying an overlay at runtime that do not apply
> >>>>>> to applying an overlay to an FDT that will be loaded by the kernel during
> >>>>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
> >>>>>> overlay loading methods to test the kernel runtime overlay loading feature.
> >>>>>
> >>>>> This patch doesn't take away from any of that and it completely orthogonal.
> >>>>
> >>>> Mea culpa.  I took the patch header comment at face value, and read more into
> >>>> the header comment than what was written there.  I then skimmed the patch
> >>>> instead of actually reading what it was doing.
> >>>>
> >>>> I incorrectly _assumed_ (bad!) that the intent was to replace applying the
> >>>> individual overlay dtb's with the master.dtb.  Reading more closely, I see
> >>>> that the assumed final step of actually _using_ master.dtb does not exist.
> >>>>
> >>>> So, yes, I agree that the patch as written is orthogonal to my concern.
> >>>>
> >>>> My updated understanding is that this patch is attempting to use the existing
> >>>> unittest overlay dts files as source to test fdtoverlay.  And that the resulting
> >>>> dtb from fdtoverlay is not intended to be consumed by the kernel unittest.
> >>>
> >>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is
> >>> testing overlays we expect to be able to apply can actually apply and
> >>> doing this at build time. That's also the goal for all the 'real'
> >>> overlays which get added.
> >>>
> >>>> I do not agree that this is a good approach to testing fdtoverlay.  The
> >>>> unittest overlay dts files are constructed specifically to test various
> >>>> parts of the kernel overlay code and dynamic OF code.  Some of the content
> >>>> of the overlays is constructed to trigger error conditions in that code,
> >>>> and thus will not be able to be processed without error by fdtoverlay.
> >>>
> >>> Then those should be omitted.
> >>>
> >>>> Trying to use overlay dts files that are constructed to test runtime kernel
> >>>> code as fdtoverlay input data mixes two different test environments and
> >>>> objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
> >>>> dts files should be created.
> >>>>
> >>>>>
> >>>>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
> >>>>>> parent project to see how much testing of fdtoverlay occurs there, but I
> >>>>>> would prefer that fdtoverlay tests reside in the parent project if practical
> >>>>>> and reasonable.  If there is some reason that some fdtoverlay tests are
> >>>>>> more practical in the Linux kernel repository then I am open to adding
> >>>>>> them to the Linux kernel tree.
> >>>>>
> >>>>> If you (or more importantly someone else sending us patches) make
> >>>>> changes to the overlays, you can test that they apply at build time
> >>>>> rather than runtime. I'll take it! So please help on fixing the issue
> >>>>> because I want to apply this.
> >>>>
> >>>> If the tests can be added to the upstream project, I would much prefer
> >>>> they reside there.  If there is some reason a certain test is more
> >>>> suited to be in the Linux kernel source tree then I also would like
> >>>> it to be accepted here.
> >>>
> >>> Again, this is just about doing sanity checks at build time rather
> >>> than *only* rely on runtime.
> >>
> >> I'm fine with adding tests for applying overlays at build time (in
> >> other words, tests of fdtoverlay).
> >
>
> > Again, it's not tests of fdtoverlay. It's a test of the dts files. We
> > are testing that an overlay dts can apply to the base dts we claim it
> > applies. If the overlay dts has crap then we'll catch it.
> >
> > We shouldn't accept overlays that can't apply to a base in the kernel
> > tree. That's either because it's broken or because the base doesn't
> > exist. With the exception of overlays designed to fail for tests,
> > unittest overlays should not be any different.
>
> I understood the goal to be testing fdtoverlay.  I'll switch my mind
> set to the goal being a test of dts files.
>
> We already know that unittest overlays that are expected to be valid
> can apply successfully.  The run time unittests already check for that.

As soon as I apply a patch to one I don't know it's valid or can apply anymore.

> I don't see any value in adding a build time test for the same thing
> _for unittest overlay dts files_.  And I do see an ongoing maintenance
> cost for _unittest overlay dts files_.

0-day, kernelci, etc. will all build time test it. Actually, everyone
doing allmodconfig builds will. Runtime testing requires *my* time.
I'd like to say runtime testing is part of my highly automated
workflow and unittests get run all the time, but they don't.

> If you want to add build time tests for all (or some) non-unittest overlay
> dts files, then I am not particularly opposed to that (but being aware that
> an overlay dtb could apply on top of more than one base dtb, so there
> is a possibility of an "explosion" of combinations to be maintained
> in the build system).

Yes, that could be a problem. I think reality is most overlays we're
willing to accept will be board specific.

> I see value in having build time testing that overlay dtbs apply cleanly
> on a base dtb.  I have heard frustration from the out of tree users of
> overlays that apply the overlays via the bootloader, because if the
> bootloader fails to apply an overlay it can be difficult to debug or
> fix on the target computer.  Having a mechanism to specify what overlays
> are intended to be applied to a base dtb, and verify that they do
> apply would resolve some of those issues, assuming the boot loader
> and fdtoverlay are consistent with each other.

Yes, that's one main reason to require applying them at build time.

The other is if people want to refactor a current dtb into a base and
overlay, then we should still produce the original combined dtb.

> >> But the constraints on applying an overlay at build time are different
> >> than the runtime constraints.
> >
> > Like what specifically? Runtime is more constrained than build time.
> > Or at least it should be. It's not really and that's why we have
> > limited runtime applied overlay support.
> >
> >> The existing unittest overlay dts files are not designed to test applying
> >> overlays at build time.  Tests for fdtoverlay should be designed to test
> >> that overlays that meet the build time constraints can be applied
> >> properly by fdtoverlay, and that overlays that fail to meet those
> >> constraints are rejected by fdtoverlay.
> >>
> >> Trying to use the same data (dts) files for tests that have different
> >> constraints is likely to make both tests more fragile when a data file
> >> is modified for one environment without careful consideration of the
> >> other environment.
> >
> > We're not changing nor constraining the data files. Just adding
> > another sanity test on them.
>
> For _unittest_ dts files, I see no value add.  And the cost of needing
> to track _in the build system_ which unittest dts files are expected fail
> to apply and which are expected to succeed.

It costs nothing to add it (well, it would have been before this
thread). If it becomes problematic, then we can drop it.

Rob

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/13/21 9:05 AM, Rob Herring wrote:
> On Tue, Jan 12, 2021 at 8:20 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> On 1/12/21 2:46 PM, Rob Herring wrote:
>>> On Tue, Jan 12, 2021 at 2:05 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> On 1/12/21 1:41 PM, Rob Herring wrote:
>>>>> On Tue, Jan 12, 2021 at 1:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>
>>>>>> On 1/12/21 8:04 AM, Rob Herring wrote:
>>>>>>> On Mon, Jan 11, 2021 at 4:06 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>>>>>
>>>>>>>> On 1/8/21 2:41 AM, Viresh Kumar wrote:
>>>>>>>>> Now that fdtoverlay is part of the kernel build, start using it to test
>>>>>>>>> the unitest overlays we have by applying them statically.
>>>>>>>>>
>>>>>>>>> The file overlay_base.dtb have symbols of its own and we need to apply
>>>>>>>>> overlay.dtb to overlay_base.dtb alone first to make it work, which gives
>>>>>>>>> us intermediate-overlay.dtb file.
>>>>>>>>>
>>>>>>>>> The intermediate-overlay.dtb file along with all other overlays is them
>>>>>>>>> applied to testcases.dtb to generate the master.dtb file.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>>>>>>
>>>>>>>> NACK to this specific patch, in its current form.
>>>>>>>>
>>>>>>>> There are restrictions on applying an overlay at runtime that do not apply
>>>>>>>> to applying an overlay to an FDT that will be loaded by the kernel during
>>>>>>>> early boot.  Thus the unittest overlays _must_ be applied using the kernel
>>>>>>>> overlay loading methods to test the kernel runtime overlay loading feature.
>>>>>>>
>>>>>>> This patch doesn't take away from any of that and it completely orthogonal.
>>>>>>
>>>>>> Mea culpa.  I took the patch header comment at face value, and read more into
>>>>>> the header comment than what was written there.  I then skimmed the patch
>>>>>> instead of actually reading what it was doing.
>>>>>>
>>>>>> I incorrectly _assumed_ (bad!) that the intent was to replace applying the
>>>>>> individual overlay dtb's with the master.dtb.  Reading more closely, I see
>>>>>> that the assumed final step of actually _using_ master.dtb does not exist.
>>>>>>
>>>>>> So, yes, I agree that the patch as written is orthogonal to my concern.
>>>>>>
>>>>>> My updated understanding is that this patch is attempting to use the existing
>>>>>> unittest overlay dts files as source to test fdtoverlay.  And that the resulting
>>>>>> dtb from fdtoverlay is not intended to be consumed by the kernel unittest.
>>>>>
>>>>> The goal is not to test fdtoverlay. dtc unittests do that. The goal is
>>>>> testing overlays we expect to be able to apply can actually apply and
>>>>> doing this at build time. That's also the goal for all the 'real'
>>>>> overlays which get added.
>>>>>
>>>>>> I do not agree that this is a good approach to testing fdtoverlay.  The
>>>>>> unittest overlay dts files are constructed specifically to test various
>>>>>> parts of the kernel overlay code and dynamic OF code.  Some of the content
>>>>>> of the overlays is constructed to trigger error conditions in that code,
>>>>>> and thus will not be able to be processed without error by fdtoverlay.
>>>>>
>>>>> Then those should be omitted.
>>>>>
>>>>>> Trying to use overlay dts files that are constructed to test runtime kernel
>>>>>> code as fdtoverlay input data mixes two different test environments and
>>>>>> objectives.  If fdtoverlay test cases are desired, then fdtoverlay specific
>>>>>> dts files should be created.
>>>>>>
>>>>>>>
>>>>>>>> I agree that testing fdtoverlay is a good idea.  I have not looked at the
>>>>>>>> parent project to see how much testing of fdtoverlay occurs there, but I
>>>>>>>> would prefer that fdtoverlay tests reside in the parent project if practical
>>>>>>>> and reasonable.  If there is some reason that some fdtoverlay tests are
>>>>>>>> more practical in the Linux kernel repository then I am open to adding
>>>>>>>> them to the Linux kernel tree.
>>>>>>>
>>>>>>> If you (or more importantly someone else sending us patches) make
>>>>>>> changes to the overlays, you can test that they apply at build time
>>>>>>> rather than runtime. I'll take it! So please help on fixing the issue
>>>>>>> because I want to apply this.
>>>>>>
>>>>>> If the tests can be added to the upstream project, I would much prefer
>>>>>> they reside there.  If there is some reason a certain test is more
>>>>>> suited to be in the Linux kernel source tree then I also would like
>>>>>> it to be accepted here.
>>>>>
>>>>> Again, this is just about doing sanity checks at build time rather
>>>>> than *only* rely on runtime.
>>>>
>>>> I'm fine with adding tests for applying overlays at build time (in
>>>> other words, tests of fdtoverlay).
>>>
>>
>>> Again, it's not tests of fdtoverlay. It's a test of the dts files. We
>>> are testing that an overlay dts can apply to the base dts we claim it
>>> applies. If the overlay dts has crap then we'll catch it.
>>>
>>> We shouldn't accept overlays that can't apply to a base in the kernel
>>> tree. That's either because it's broken or because the base doesn't
>>> exist. With the exception of overlays designed to fail for tests,
>>> unittest overlays should not be any different.
>>
>> I understood the goal to be testing fdtoverlay.  I'll switch my mind
>> set to the goal being a test of dts files.
>>
>> We already know that unittest overlays that are expected to be valid
>> can apply successfully.  The run time unittests already check for that.
> 
> As soon as I apply a patch to one I don't know it's valid or can apply anymore.
> 
>> I don't see any value in adding a build time test for the same thing
>> _for unittest overlay dts files_.  And I do see an ongoing maintenance
>> cost for _unittest overlay dts files_.
> 
> 0-day, kernelci, etc. will all build time test it. Actually, everyone
> doing allmodconfig builds will. Runtime testing requires *my* time.

> I'd like to say runtime testing is part of my highly automated
> workflow and unittests get run all the time, but they don't.

I would like to think that if you apply a patch _to a unittest overlay_
that you would run the unittests afterward.  But I can also see that applying
a random patch to devicetree code could also unexpectedly result in a unittest
failing.

OK, I would say that helping your workflow is sufficient positive value to
more than offset the slightly negative that I was noting.  A simple one
line comment at the list of unittest overlays static tested saying that some
other overlays are not tested because they are designed to intentionally fail
to apply would make me happy.

And fortunately I do run the unittests, normally for each major release and
each -rc1 at a minimum so at least that much unittest coverage is present.
My big unittest gap is that I don't test all architectures.

I withdraw my NACK.

-Frank

> 
>> If you want to add build time tests for all (or some) non-unittest overlay
>> dts files, then I am not particularly opposed to that (but being aware that
>> an overlay dtb could apply on top of more than one base dtb, so there
>> is a possibility of an "explosion" of combinations to be maintained
>> in the build system).
> 
> Yes, that could be a problem. I think reality is most overlays we're
> willing to accept will be board specific.
> 
>> I see value in having build time testing that overlay dtbs apply cleanly
>> on a base dtb.  I have heard frustration from the out of tree users of
>> overlays that apply the overlays via the bootloader, because if the
>> bootloader fails to apply an overlay it can be difficult to debug or
>> fix on the target computer.  Having a mechanism to specify what overlays
>> are intended to be applied to a base dtb, and verify that they do
>> apply would resolve some of those issues, assuming the boot loader
>> and fdtoverlay are consistent with each other.
> 
> Yes, that's one main reason to require applying them at build time.
> 
> The other is if people want to refactor a current dtb into a base and
> overlay, then we should still produce the original combined dtb.
> 
>>>> But the constraints on applying an overlay at build time are different
>>>> than the runtime constraints.
>>>
>>> Like what specifically? Runtime is more constrained than build time.
>>> Or at least it should be. It's not really and that's why we have
>>> limited runtime applied overlay support.
>>>
>>>> The existing unittest overlay dts files are not designed to test applying
>>>> overlays at build time.  Tests for fdtoverlay should be designed to test
>>>> that overlays that meet the build time constraints can be applied
>>>> properly by fdtoverlay, and that overlays that fail to meet those
>>>> constraints are rejected by fdtoverlay.
>>>>
>>>> Trying to use the same data (dts) files for tests that have different
>>>> constraints is likely to make both tests more fragile when a data file
>>>> is modified for one environment without careful consideration of the
>>>> other environment.
>>>
>>> We're not changing nor constraining the data files. Just adding
>>> another sanity test on them.
>>
>> For _unittest_ dts files, I see no value add.  And the cost of needing
>> to track _in the build system_ which unittest dts files are expected fail
>> to apply and which are expected to succeed.
> 
> It costs nothing to add it (well, it would have been before this
> thread). If it becomes problematic, then we can drop it.
> 
> Rob
> 

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

Frank/Rob.

On 08-01-21, 14:11, Viresh Kumar wrote:
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index 009f4045c8e4..f17bce85f65f 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@
>  
>  # suppress warnings about intentional errors
>  DTC_FLAGS_testcases += -Wno-interrupts_property
> +
> +# Apply overlays statically with fdtoverlay

I will update this part to mention about the dtbs we are not using in the build
as they will fail (as per Frank's comment).

Is there anything else you guys want me to change before I send the next version
?

> +intermediate-overlay	:= overlay.dtb
> +master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
> +			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
> +			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
> +			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
> +			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
> +			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
> +			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
> +			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
> +			   intermediate-overlay.dtb
> +
> +quiet_cmd_fdtoverlay = fdtoverlay $@
> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
> +
> +$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
> +	$(call if_changed,fdtoverlay)
> +
> +$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
> +	$(call if_changed,fdtoverlay)
> +
> +always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/13/21 11:00 PM, Viresh Kumar wrote:
> Frank/Rob.
> 
> On 08-01-21, 14:11, Viresh Kumar wrote:
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index 009f4045c8e4..f17bce85f65f 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -38,3 +38,26 @@ DTC_FLAGS_testcases += -@
>>  
>>  # suppress warnings about intentional errors
>>  DTC_FLAGS_testcases += -Wno-interrupts_property
>> +
>> +# Apply overlays statically with fdtoverlay
> 
> I will update this part to mention about the dtbs we are not using in the build
> as they will fail (as per Frank's comment).
> 
> Is there anything else you guys want me to change before I send the next version
> ?

I sent some changes in the form of a patch, in reply to your original patch.
If you can fold the changes into your patch, and look into the comments
that I put into the patch, that would be great.

-Frank

> 
>> +intermediate-overlay	:= overlay.dtb
>> +master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>> +			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>> +			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
>> +			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
>> +			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
>> +			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>> +			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>> +			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
>> +			   intermediate-overlay.dtb
>> +
>> +quiet_cmd_fdtoverlay = fdtoverlay $@
>> +      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>> +
>> +$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
>> +	$(call if_changed,fdtoverlay)
>> +
>> +$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
>> +	$(call if_changed,fdtoverlay)
>> +
>> +always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
> 

re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

These changes apply on top of the patches in:

  [PATCH] of: unittest: Statically apply overlays using fdtoverlay
  Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@linaro.org>

There are still some issues to be cleaned up, so not ready for acceptance.

I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and
have not looked into the proper usage of it.

I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler
upstream project.  For my testing I added LD_LIBRARY_PATH to the body of
"cmd_fdtoverlay" to reference my hand built libfdt.  The kernel build
system will have to instead use a libfdt that is built in the kernel
tree.

I have not run this through checkpatch, or my checks for build warnings.
I have not run unittests on my target with these patches applied.

---
 drivers/of/unittest-data/Makefile | 67 ++++++++++++++++++++++---------
 1 file changed, 48 insertions(+), 19 deletions(-)

diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
index f17bce85f65f..28614a123d1e 100644
--- a/drivers/of/unittest-data/Makefile
+++ b/drivers/of/unittest-data/Makefile
@@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@
 # suppress warnings about intentional errors
 DTC_FLAGS_testcases += -Wno-interrupts_property
 
-# Apply overlays statically with fdtoverlay
-intermediate-overlay	:= overlay.dtb
-master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
-			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
-			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
-			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
-			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
-			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
-			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
-			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
-			   intermediate-overlay.dtb
-
-quiet_cmd_fdtoverlay = fdtoverlay $@
-      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
-
-$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
-	$(call if_changed,fdtoverlay)
+# Apply overlays statically with fdtoverlay.  This is a build time test that
+# the overlays can be applied successfully by fdtoverlay.  This does not
+# guarantee that the overlays can be applied successfully at run time by
+# unittest, but it provides a bit of build time test coverage for those
+# who do not execute unittest.
+#
+# The overlays are applied on top of testcases.dtb to create static_test.dtb
+# If fdtoverlay detects an error than the kernel build will fail.
+# static_test.dtb is not consumed by unittest.
+#
+# Some unittest overlays deliberately contain errors that unittest checks for.
+# These overlays will cause fdtoverlay to fail, and are thus not included
+# in the static test:
+#			overlay.dtb \
+#			overlay_bad_add_dup_node.dtb \
+#			overlay_bad_add_dup_prop.dtb \
+#			overlay_bad_phandle.dtb \
+#			overlay_bad_symbol.dtb \
+
+apply_static_overlay := overlay_base.dtb \
+			overlay_0.dtb \
+			overlay_1.dtb \
+			overlay_2.dtb \
+			overlay_3.dtb \
+			overlay_4.dtb \
+			overlay_5.dtb \
+			overlay_6.dtb \
+			overlay_7.dtb \
+			overlay_8.dtb \
+			overlay_9.dtb \
+			overlay_10.dtb \
+			overlay_11.dtb \
+			overlay_12.dtb \
+			overlay_13.dtb \
+			overlay_15.dtb \
+			overlay_gpio_01.dtb \
+			overlay_gpio_02a.dtb \
+			overlay_gpio_02b.dtb \
+			overlay_gpio_03.dtb \
+			overlay_gpio_04a.dtb \
+			overlay_gpio_04b.dtb \
+
+quiet_cmd_fdtoverlay = FDTOVERLAY $@
+
+## This is not correct, need to use libfdt from the kernel tree:
+cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
 
-$(obj)/master.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(master))
+$(obj)/static_test.dtb: $(obj)/testcases.dtb $(addprefix $(obj)/,$(apply_static_overlay))
 	$(call if_changed,fdtoverlay)
 
-always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
+always-$(CONFIG_OF_OVERLAY) += static_test.dtb
-- 
Frank Rowand <frank.rowand@sony.com>

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 18-01-21, 20:21, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
> These changes apply on top of the patches in:
> 
>   [PATCH] of: unittest: Statically apply overlays using fdtoverlay
>   Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@linaro.org>
> 
> There are still some issues to be cleaned up, so not ready for acceptance.

Are you talking about the missing __overlay__ thing ? (more below)

> I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and
> have not looked into the proper usage of it.

I wasn't sure either, maybe Masahiro can suggest the best fit.

> I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler
> upstream project.  For my testing I added LD_LIBRARY_PATH to the body of
> "cmd_fdtoverlay" to reference my hand built libfdt.  The kernel build
> system will have to instead use a libfdt that is built in the kernel
> tree.

I tested it with this patchset:

https://lore.kernel.org/lkml/cover.1610431620.git.viresh.kumar@linaro.org/

> I have not run this through checkpatch, or my checks for build warnings.
> I have not run unittests on my target with these patches applied.
> 
> ---
>  drivers/of/unittest-data/Makefile | 67 ++++++++++++++++++++++---------
>  1 file changed, 48 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
> index f17bce85f65f..28614a123d1e 100644
> --- a/drivers/of/unittest-data/Makefile
> +++ b/drivers/of/unittest-data/Makefile
> @@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@
>  # suppress warnings about intentional errors
>  DTC_FLAGS_testcases += -Wno-interrupts_property
>  
> -# Apply overlays statically with fdtoverlay
> -intermediate-overlay	:= overlay.dtb
> -master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
> -			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
> -			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
> -			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
> -			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
> -			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
> -			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
> -			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
> -			   intermediate-overlay.dtb
> -
> -quiet_cmd_fdtoverlay = fdtoverlay $@
> -      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
> -
> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
> -	$(call if_changed,fdtoverlay)
> +# Apply overlays statically with fdtoverlay.  This is a build time test that
> +# the overlays can be applied successfully by fdtoverlay.  This does not
> +# guarantee that the overlays can be applied successfully at run time by
> +# unittest, but it provides a bit of build time test coverage for those
> +# who do not execute unittest.
> +#
> +# The overlays are applied on top of testcases.dtb to create static_test.dtb
> +# If fdtoverlay detects an error than the kernel build will fail.
> +# static_test.dtb is not consumed by unittest.
> +#
> +# Some unittest overlays deliberately contain errors that unittest checks for.
> +# These overlays will cause fdtoverlay to fail, and are thus not included
> +# in the static test:
> +#			overlay.dtb \
> +#			overlay_bad_add_dup_node.dtb \
> +#			overlay_bad_add_dup_prop.dtb \
> +#			overlay_bad_phandle.dtb \
> +#			overlay_bad_symbol.dtb \
> +
> +apply_static_overlay := overlay_base.dtb \

This won't work because of the issues I mentioned earlier. This file
doesn't have __overlay__. One way to fix that is to do this:

diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
index 99ab9d12d00b..59172c4c9e5a 100644
--- a/drivers/of/unittest-data/overlay_base.dts
+++ b/drivers/of/unittest-data/overlay_base.dts
@@ -11,8 +11,7 @@
  * dtc will create nodes "/__symbols__" and "/__local_fixups__".
  */

-/ {
-       testcase-data-2 {
+       &overlay_base {
                #address-cells = <1>;
                #size-cells = <1>;

@@ -89,5 +88,3 @@ retail_1: vending@50000 {
                };

        };
-};
-
diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
index a85b5e1c381a..539dc7d9eddc 100644
--- a/drivers/of/unittest-data/testcases.dts
+++ b/drivers/of/unittest-data/testcases.dts
@@ -11,6 +11,11 @@ node-remove {
                        };
                };
        };
+
+       overlay_base: testcase-data-2 {
+               #address-cells = <1>;
+               #size-cells = <1>;
+       };

> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb

This is how static_test.dtb looks now with fdtdump

/dts-v1/;
// magic:		0xd00dfeed
// totalsize:		0x261b (9755)
// off_dt_struct:	0x38
// off_dt_strings:	0x22dc
// off_mem_rsvmap:	0x28
// version:		17
// last_comp_version:	16
// boot_cpuid_phys:	0x0
// size_dt_strings:	0x33f
// size_dt_struct:	0x22a4

/ {
    #address-cells = <0x00000001>;
    #size-cells = <0x00000001>;
    testcase-data {
        security-password = "password";
        duplicate-name = "duplicate";
        #address-cells = <0x00000001>;
        #size-cells = <0x00000001>;
        ranges;
        phandle = <0x0000000b>;
        changeset {
            prop-update = "hello";
            prop-remove = "world";
            node-remove {
            };
        };
        duplicate-name {
        };
        phandle-tests {
            provider0 {
                #phandle-cells = <0x00000000>;
                phandle = <0x00000002>;
            };
            provider1 {
                #phandle-cells = <0x00000001>;
                phandle = <0x00000001>;
            };
            provider2 {
                #phandle-cells = <0x00000002>;
                phandle = <0x00000004>;
            };
            provider3 {
                #phandle-cells = <0x00000003>;
                phandle = <0x00000003>;
            };
            provider4 {
                #phandle-cells = <0x00000002>;
                phandle-map = <0x00000000 0x00000001 0x00000001 0x00000003 0x00000004 0x00000000 0x00000002 0x00000010 0x00000005 0x00000003 0x00000003 0x00000005 0x00000000 0x000000c8 0x00000008 0x00000004 0x00000017 0x00000006 0x00000013 0x00000000 0x00000004 0x0000000f 0x00000000 0x00000002 0x00000003 0x00000003 0x00000002 0x00000005 0x00000003>;
                phandle-map-mask = <0x000000ff 0x0000000f>;
                phandle-map-pass-thru = <0x00000000 0x000000f0>;
                phandle = <0x00000005>;
            };
            consumer-a {
                phandle-list = <0x00000001 0x00000001 0x00000004 0x00000002 0x00000000 0x00000000 0x00000003 0x00000004 0x00000004 0x00000003 0x00000004 0x00000005 0x00000064 0x00000002 0x00000001 0x00000007>;
                phandle-list-names = "first", "second", "third";
                phandle-list-bad-phandle = <0x00bc614e 0x00000000 0x00000000>;
                phandle-list-bad-args = <0x00000004 0x00000001 0x00000000 0x00000003 0x00000000>;
                empty-property;
                string-property = "foobar";
                unterminated-string = <0x40414243>;
                unterminated-string-list = [66 69 72 73 74 00 73 65 63 6f 6e 64 00 40 41 42 43];
            };
            consumer-b {
                phandle-list = <0x00000001 0x00000001 0x00000005 0x00000002 0x00000003 0x00000000 0x00000005 0x00000004 0x00000100 0x00000005 0x00000000 0x00000061 0x00000002 0x00000005 0x00000013 0x00000020>;
                phandle-list-bad-phandle = <0x00bc614e 0x00000000 0x00000000>;
                phandle-list-bad-args = <0x00000004 0x00000001 0x00000000 0x00000005 0x00000000>;
            };
        };
        interrupts {
            #address-cells = <0x00000001>;
            #size-cells = <0x00000001>;
            intc0 {
                interrupt-controller;
                #interrupt-cells = <0x00000001>;
                phandle = <0x00000006>;
            };
            intc1 {
                interrupt-controller;
                #interrupt-cells = <0x00000003>;
                phandle = <0x00000007>;
            };
            intc2 {
                interrupt-controller;
                #interrupt-cells = <0x00000002>;
                phandle = <0x00000008>;
            };
            intmap0 {
                #interrupt-cells = <0x00000001>;
                #address-cells = <0x00000000>;
                interrupt-map = <0x00000001 0x00000006 0x00000009 0x00000002 0x00000007 0x0000000a 0x0000000b 0x0000000c 0x00000003 0x00000008 0x0000000d 0x0000000e 0x00000004 0x00000008 0x0000000f 0x00000010>;
                phandle = <0x00000009>;
            };
            intmap1 {
                #interrupt-cells = <0x00000002>;
                interrupt-map = <0x00005000 0x00000001 0x00000002 0x00000006 0x0000000f>;
                phandle = <0x0000000a>;
            };
            interrupts0 {
                interrupt-parent = <0x00000006>;
                interrupts = <0x00000001 0x00000002 0x00000003 0x00000004>;
            };
            interrupts1 {
                interrupt-parent = <0x00000009>;
                interrupts = <0x00000001 0x00000002 0x00000003 0x00000004>;
            };
            interrupts-extended0 {
                reg = <0x00005000 0x00000100>;
                interrupts-extended = <0x00000006 0x00000001 0x00000007 0x00000002 0x00000003 0x00000004 0x00000008 0x00000005 0x00000006 0x00000009 0x00000001 0x00000009 0x00000002 0x00000009 0x00000003 0x0000000a 0x00000001 0x00000002>;
            };
        };
        testcase-device1 {
            compatible = "testcase-device";
            interrupt-parent = <0x00000006>;
            interrupts = <0x00000001>;
        };
        testcase-device2 {
            compatible = "testcase-device";
            interrupt-parent = <0x00000008>;
            interrupts = <0x00000001>;
        };
        match-node {
            name0 {
            };
            name1 {
                device_type = "type1";
            };
            a {
                name2 {
                    device_type = "type1";
                };
            };
            b {
                name2 {
                };
            };
            c {
                name2 {
                    device_type = "type2";
                };
            };
            name3 {
                compatible = "compat3";
            };
            name4 {
                compatible = "compat2", "compat3";
            };
            name5 {
                compatible = "compat2", "compat3";
            };
            name6 {
                compatible = "compat1", "compat2", "compat3";
            };
            name7 {
                compatible = "compat2";
                device_type = "type1";
            };
            name8 {
                compatible = "compat2";
                device_type = "type1";
            };
            name9 {
                compatible = "compat2";
            };
        };
        address-tests {
            #address-cells = <0x00000001>;
            #size-cells = <0x00000001>;
            ranges = <0x70000000 0x70000000 0x40000000 0x00000000 0xd0000000 0x20000000>;
            dma-ranges = <0x00000000 0x20000000 0x40000000>;
            device@70000000 {
                reg = <0x70000000 0x00001000>;
            };
            bus@80000000 {
                #address-cells = <0x00000002>;
                #size-cells = <0x00000002>;
                ranges = <0x00000000 0x00000000 0x80000000 0x00000000 0x00100000>;
                dma-ranges = <0x00000001 0x00000000 0x00000000 0x00000020 0x00000000>;
                device@1000 {
                    reg = <0x00000000 0x00001000 0x00000000 0x00001000>;
                };
            };
            pci@90000000 {
                device_type = "pci";
                #address-cells = <0x00000003>;
                #size-cells = <0x00000002>;
                reg = <0x90000000 0x00001000>;
                ranges = <0x42000000 0x00000000 0x40000000 0x40000000 0x00000000 0x10000000>;
                dma-ranges = <0x42000000 0x00000000 0x80000000 0x00000000 0x00000000 0x10000000 0x42000000 0x00000000 0xc0000000 0x20000000 0x00000000 0x10000000>;
            };
        };
        platform-tests {
            #address-cells = <0x00000001>;
            #size-cells = <0x00000000>;
            test-device@0 {
                compatible = "test-device";
                reg = <0x00000000>;
                #address-cells = <0x00000001>;
                #size-cells = <0x00000000>;
                dev@100 {
                    compatible = "test-sub-device";
                    reg = <0x00000100>;
                };
            };
            test-device@1 {
                compatible = "test-device";
                reg = <0x00000001>;
                #address-cells = <0x00000001>;
                #size-cells = <0x00000000>;
                dev@100 {
                    compatible = "test-sub-device", "test-compat2", "test-compat3";
                    reg = <0x00000100>;
                };
            };
        };
        overlay-node {
            test-bus {
                compatible = "simple-bus";
                #address-cells = <0x00000001>;
                #size-cells = <0x00000000>;
                phandle = <0x0000000c>;
                gpio@4 {
                    gpio-line-names = "line-A", "line-B", "line-C", "line-D";
                    ngpios = <0x00000002>;
                    #gpio-cells = <0x00000002>;
                    gpio-controller;
                    reg = <0x00000004>;
                    compatible = "unittest-gpio";
                    line-c {
                        line-name = "line-C-input";
                        input;
                        gpios = <0x00000003 0x00000000>;
                        gpio-hog;
                    };
                };
                gpio@3 {
                    gpio-line-names = "line-A", "line-B", "line-C", "line-D";
                    ngpios = <0x00000002>;
                    #gpio-cells = <0x00000002>;
                    gpio-controller;
                    reg = <0x00000003>;
                    compatible = "unittest-gpio";
                    line-d {
                        line-name = "line-D-input";
                        input;
                        gpios = <0x00000004 0x00000000>;
                        gpio-hog;
                    };
                };
                gpio@2 {
                    gpio-line-names = "line-A", "line-B";
                    ngpios = <0x00000002>;
                    #gpio-cells = <0x00000002>;
                    gpio-controller;
                    reg = <0x00000002>;
                    compatible = "unittest-gpio";
                    line-a {
                        line-name = "line-A-input";
                        input;
                        gpios = <0x00000001 0x00000000>;
                        gpio-hog;
                    };
                };
                gpio@0 {
                    gpio-line-names = "line-A", "line-B";
                    ngpios = <0x00000002>;
                    #gpio-cells = <0x00000002>;
                    gpio-controller;
                    reg = <0x00000000>;
                    compatible = "unittest-gpio";
                    line-b {
                        line-name = "line-B-input";
                        input;
                        gpios = <0x00000002 0x00000000>;
                        gpio-hog;
                    };
                };
                test-unittest11 {
                    #size-cells = <0x00000000>;
                    #address-cells = <0x00000001>;
                    reg = <0x0000000b>;
                    status = "okay";
                    compatible = "unittest";
                    test-unittest111 {
                        reg = <0x00000001>;
                        status = "okay";
                        compatible = "unittest";
                    };
                };
                test-unittest10 {
                    #size-cells = <0x00000000>;
                    #address-cells = <0x00000001>;
                    reg = <0x0000000a>;
                    status = "okay";
                    compatible = "unittest";
                    test-unittest101 {
                        reg = <0x00000001>;
                        status = "okay";
                        compatible = "unittest";
                    };
                };
                test-unittest4 {
                    reg = <0x00000004>;
                    status = "okay";
                    compatible = "unittest";
                };
                test-unittest100 {
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000064>;
                    phandle = <0x0000000d>;
                };
                test-unittest101 {
                    compatible = "unittest";
                    status = "disabled";
                    reg = <0x00000065>;
                    phandle = <0x0000000e>;
                };
                test-unittest0 {
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000000>;
                    phandle = <0x0000000f>;
                };
                test-unittest1 {
                    compatible = "unittest";
                    status = "disabled";
                    reg = <0x00000001>;
                    phandle = <0x00000010>;
                };
                test-unittest2 {
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000002>;
                    phandle = <0x00000011>;
                };
                test-unittest3 {
                    compatible = "unittest";
                    status = "disabled";
                    reg = <0x00000003>;
                    phandle = <0x00000012>;
                };
                test-unittest5 {
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000005>;
                    phandle = <0x00000013>;
                };
                test-unittest6 {
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000006>;
                    phandle = <0x00000014>;
                };
                test-unittest7 {
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000007>;
                    phandle = <0x00000015>;
                };
                test-unittest8 {
                    property-foo = "bar";
                    compatible = "unittest";
                    status = "okay";
                    reg = <0x00000008>;
                    phandle = <0x00000016>;
                };
                i2c-test-bus {
                    compatible = "unittest-i2c-bus";
                    status = "okay";
                    reg = <0x00000032>;
                    #address-cells = <0x00000001>;
                    #size-cells = <0x00000000>;
                    phandle = <0x00000017>;
                    test-unittest15 {
                        #size-cells = <0x00000000>;
                        #address-cells = <0x00000001>;
                        status = "okay";
                        compatible = "unittest-i2c-mux";
                        reg = <0x0000000b>;
                        i2c@0 {
                            reg = <0x00000000>;
                            #size-cells = <0x00000000>;
                            #address-cells = <0x00000001>;
                            test-mux-dev@20 {
                                status = "okay";
                                compatible = "unittest-i2c-dev";
                                reg = <0x00000020>;
                            };
                        };
                    };
                    test-unittest12 {
                        reg = <0x00000008>;
                        compatible = "unittest-i2c-dev";
                        status = "okay";
                    };
                    test-unittest13 {
                        reg = <0x00000009>;
                        compatible = "unittest-i2c-dev";
                        status = "disabled";
                    };
                    test-unittest14 {
                        reg = <0x0000000a>;
                        compatible = "unittest-i2c-mux";
                        status = "okay";
                        #address-cells = <0x00000001>;
                        #size-cells = <0x00000000>;
                        i2c@0 {
                            #address-cells = <0x00000001>;
                            #size-cells = <0x00000000>;
                            reg = <0x00000000>;
                            test-mux-dev@20 {
                                reg = <0x00000020>;
                                compatible = "unittest-i2c-dev";
                                status = "okay";
                            };
                        };
                    };
                };
            };
        };
    };
    aliases {
        testcase-alias = "/testcase-data";
    };
    __symbols__ {
        testcase = "/testcase-data";
        provider0 = "/testcase-data/phandle-tests/provider0";
        provider1 = "/testcase-data/phandle-tests/provider1";
        provider2 = "/testcase-data/phandle-tests/provider2";
        provider3 = "/testcase-data/phandle-tests/provider3";
        provider4 = "/testcase-data/phandle-tests/provider4";
        test_intc0 = "/testcase-data/interrupts/intc0";
        test_intc1 = "/testcase-data/interrupts/intc1";
        test_intc2 = "/testcase-data/interrupts/intc2";
        test_intmap0 = "/testcase-data/interrupts/intmap0";
        test_intmap1 = "/testcase-data/interrupts/intmap1";
        unittest_test_bus = "/testcase-data/overlay-node/test-bus";
        unittest100 = "/testcase-data/overlay-node/test-bus/test-unittest100";
        unittest101 = "/testcase-data/overlay-node/test-bus/test-unittest101";
        unittest0 = "/testcase-data/overlay-node/test-bus/test-unittest0";
        unittest1 = "/testcase-data/overlay-node/test-bus/test-unittest1";
        unittest2 = "/testcase-data/overlay-node/test-bus/test-unittest2";
        unittest3 = "/testcase-data/overlay-node/test-bus/test-unittest3";
        unittest5 = "/testcase-data/overlay-node/test-bus/test-unittest5";
        unittest6 = "/testcase-data/overlay-node/test-bus/test-unittest6";
        unittest7 = "/testcase-data/overlay-node/test-bus/test-unittest7";
        unittest8 = "/testcase-data/overlay-node/test-bus/test-unittest8";
        unittest_i2c_test_bus = "/testcase-data/overlay-node/test-bus/i2c-test-bus";
    };
    __local_fixups__ {
        testcase-data {
            phandle-tests {
                provider4 {
                    phandle-map = <0x00000008 0x00000018 0x00000024 0x0000003c 0x00000050 0x00000064>;
                };
                consumer-a {
                    phandle-list = <0x00000000 0x00000008 0x00000018 0x00000028 0x00000034 0x00000038>;
                    phandle-list-bad-args = <0x00000000 0x0000000c>;
                };
                consumer-b {
                    phandle-list = <0x00000000 0x00000008 0x00000018 0x00000024 0x00000030 0x00000034>;
                    phandle-list-bad-args = <0x00000000 0x0000000c>;
                };
            };
            interrupts {
                intmap0 {
                    interrupt-map = <0x00000004 0x00000010 0x00000024 0x00000034>;
                };
                intmap1 {
                    interrupt-map = <0x0000000c>;
                };
                interrupts0 {
                    interrupt-parent = <0x00000000>;
                };
                interrupts1 {
                    interrupt-parent = <0x00000000>;
                };
                interrupts-extended0 {
                    interrupts-extended = <0x00000000 0x00000008 0x00000018 0x00000024 0x0000002c 0x00000034 0x0000003c>;
                };
            };
            testcase-device1 {
                interrupt-parent = <0x00000000>;
            };
            testcase-device2 {
                interrupt-parent = <0x00000000>;
            };
        };
    };
};

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

On 1/19/21 2:05 AM, Viresh Kumar wrote:
> On 18-01-21, 20:21, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> These changes apply on top of the patches in:
>>
>>   [PATCH] of: unittest: Statically apply overlays using fdtoverlay
>>   Message-Id: <1e42183ccafa1afba33b3e79a4e3efd3329fd133.1610095159.git.viresh.kumar@linaro.org>
>>
>> There are still some issues to be cleaned up, so not ready for acceptance.
> 
> Are you talking about the missing __overlay__ thing ? (more below)

No.  I am referencing my comments below (I'll copy them up here):

   I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and
   have not looked into the proper usage of it.

   [Tested using my own fdtoverlay instead of the one supplied by your patches
   that added fdtoverlay and fdtdump to the kernel tree.]

   I have not run this through checkpatch, or my checks for build warnings.
   I have not run unittests on my target with these patches applied.

I will have to get the updated patch, test it more fully, and fill in a gap
in my knowledge (use of "always-$(CONFIG_xxx)".


> 
>> I have not used the construct "always-$(CONFIG_OF_OVERLAY)" before, and
>> have not looked into the proper usage of it.
> 
> I wasn't sure either, maybe Masahiro can suggest the best fit.
> 
>> I tested this using a hand build libfdt and fdtoverlay from the dtc-compiler
>> upstream project.  For my testing I added LD_LIBRARY_PATH to the body of
>> "cmd_fdtoverlay" to reference my hand built libfdt.  The kernel build
>> system will have to instead use a libfdt that is built in the kernel
>> tree.
> 
> I tested it with this patchset:
> 
> https://lore.kernel.org/lkml/cover.1610431620.git.viresh.kumar@linaro.org/
> 
>> I have not run this through checkpatch, or my checks for build warnings.
>> I have not run unittests on my target with these patches applied.
>>
>> ---
>>  drivers/of/unittest-data/Makefile | 67 ++++++++++++++++++++++---------
>>  1 file changed, 48 insertions(+), 19 deletions(-)
>>
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index f17bce85f65f..28614a123d1e 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -39,25 +39,54 @@ DTC_FLAGS_testcases += -@
>>  # suppress warnings about intentional errors
>>  DTC_FLAGS_testcases += -Wno-interrupts_property
>>  
>> -# Apply overlays statically with fdtoverlay
>> -intermediate-overlay	:= overlay.dtb
>> -master			:= overlay_0.dtb overlay_1.dtb overlay_2.dtb \
>> -			   overlay_3.dtb overlay_4.dtb overlay_5.dtb \
>> -			   overlay_6.dtb overlay_7.dtb overlay_8.dtb \
>> -			   overlay_9.dtb overlay_10.dtb overlay_11.dtb \
>> -			   overlay_12.dtb overlay_13.dtb overlay_15.dtb \
>> -			   overlay_gpio_01.dtb overlay_gpio_02a.dtb \
>> -			   overlay_gpio_02b.dtb overlay_gpio_03.dtb \
>> -			   overlay_gpio_04a.dtb overlay_gpio_04b.dtb \
>> -			   intermediate-overlay.dtb
>> -
>> -quiet_cmd_fdtoverlay = fdtoverlay $@
>> -      cmd_fdtoverlay = $(objtree)/scripts/dtc/fdtoverlay -o $@ -i $^
>> -
>> -$(obj)/intermediate-overlay.dtb: $(obj)/overlay_base.dtb $(addprefix $(obj)/,$(intermediate-overlay))
>> -	$(call if_changed,fdtoverlay)
>> +# Apply overlays statically with fdtoverlay.  This is a build time test that
>> +# the overlays can be applied successfully by fdtoverlay.  This does not
>> +# guarantee that the overlays can be applied successfully at run time by
>> +# unittest, but it provides a bit of build time test coverage for those
>> +# who do not execute unittest.
>> +#
>> +# The overlays are applied on top of testcases.dtb to create static_test.dtb
>> +# If fdtoverlay detects an error than the kernel build will fail.
>> +# static_test.dtb is not consumed by unittest.
>> +#
>> +# Some unittest overlays deliberately contain errors that unittest checks for.
>> +# These overlays will cause fdtoverlay to fail, and are thus not included
>> +# in the static test:
>> +#			overlay.dtb \
>> +#			overlay_bad_add_dup_node.dtb \
>> +#			overlay_bad_add_dup_prop.dtb \
>> +#			overlay_bad_phandle.dtb \
>> +#			overlay_bad_symbol.dtb \
>> +
>> +apply_static_overlay := overlay_base.dtb \
> 
> This won't work because of the issues I mentioned earlier. This file
> doesn't have __overlay__. One way to fix that is to do this:
> 
> diff --git a/drivers/of/unittest-data/overlay_base.dts b/drivers/of/unittest-data/overlay_base.dts
> index 99ab9d12d00b..59172c4c9e5a 100644
> --- a/drivers/of/unittest-data/overlay_base.dts
> +++ b/drivers/of/unittest-data/overlay_base.dts
> @@ -11,8 +11,7 @@
>   * dtc will create nodes "/__symbols__" and "/__local_fixups__".
>   */
> 
> -/ {
> -       testcase-data-2 {
> +       &overlay_base {
>                 #address-cells = <1>;
>                 #size-cells = <1>;
> 
> @@ -89,5 +88,3 @@ retail_1: vending@50000 {
>                 };
> 
>         };
> -};
> -

No.  overlay_base.dts is intentionally compiled into a base FDT, not
an overlay.  Unittest intentionally unflattens this FDT in early boot,
in association with unflattening the system FDT.  One key intent
behind this is to use the same memory allocation method that is
used for the system FDT.

Do not try to convert overlay_base.dts into an overlay.


> diff --git a/drivers/of/unittest-data/testcases.dts b/drivers/of/unittest-data/testcases.dts
> index a85b5e1c381a..539dc7d9eddc 100644
> --- a/drivers/of/unittest-data/testcases.dts
> +++ b/drivers/of/unittest-data/testcases.dts
> @@ -11,6 +11,11 @@ node-remove {
>                         };
>                 };
>         };
> +
> +       overlay_base: testcase-data-2 {
> +               #address-cells = <1>;
> +               #size-cells = <1>;
> +       };
> 
>> -always-$(CONFIG_OF_OVERLAY) += intermediate-overlay.dtb master.dtb
>> +always-$(CONFIG_OF_OVERLAY) += static_test.dtb
> 
> This is how static_test.dtb looks now with fdtdump
> 

< snip >

-Frank

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 19-01-21, 09:44, Frank Rowand wrote:
> No.  overlay_base.dts is intentionally compiled into a base FDT, not
> an overlay.  Unittest intentionally unflattens this FDT in early boot,
> in association with unflattening the system FDT.  One key intent
> behind this is to use the same memory allocation method that is
> used for the system FDT.
> 
> Do not try to convert overlay_base.dts into an overlay.

Okay, but why does it have /plugin/; specified in it then ?

And shouldn't we create two separate dtb-s now, static_test.dtb and
static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with
testcase.dtb anyway.

Or maybe we can create another file static_overlay.dts (like testcases.dts)
which can include both testcases.dts and overlay_base.dts, and then we can
create static_test.dtb out of it ? That won't impact the runtime tests at all.

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 20-01-21, 10:36, Viresh Kumar wrote:
> On 19-01-21, 09:44, Frank Rowand wrote:
> > No.  overlay_base.dts is intentionally compiled into a base FDT, not
> > an overlay.  Unittest intentionally unflattens this FDT in early boot,
> > in association with unflattening the system FDT.  One key intent
> > behind this is to use the same memory allocation method that is
> > used for the system FDT.
> > 
> > Do not try to convert overlay_base.dts into an overlay.
> 
> Okay, but why does it have /plugin/; specified in it then ?
> 
> And shouldn't we create two separate dtb-s now, static_test.dtb and
> static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with
> testcase.dtb anyway.
> 
> Or maybe we can create another file static_overlay.dts (like testcases.dts)
> which can include both testcases.dts and overlay_base.dts, and then we can
> create static_test.dtb out of it ? That won't impact the runtime tests at all.

Hmm, I noticed just now that you have kept overlay.dtb out of the build,
probably we should then drop overlay_base.dtb as well ?

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Frank Rowand]

+David

so I don't have to repeat this in another thread

On 1/19/21 11:06 PM, Viresh Kumar wrote:
> On 19-01-21, 09:44, Frank Rowand wrote:
>> No.  overlay_base.dts is intentionally compiled into a base FDT, not
>> an overlay.  Unittest intentionally unflattens this FDT in early boot,
>> in association with unflattening the system FDT.  One key intent
>> behind this is to use the same memory allocation method that is
>> used for the system FDT.
>>
>> Do not try to convert overlay_base.dts into an overlay.
> 
> Okay, but why does it have /plugin/; specified in it then ?

OK, so I sortof lied about overlay_base.dts not being an overlay.  It is
a Frankenstein monster or a Schrodinger's dts/dtb.  It is not a normal
object.  Nobody who is not looking at how it is abused inside unittest.c
should be trying to touch it or understand it.

unittest.c first unflattens overlay_base.dtb during early boot.  Then later
it does some phandle resolution using the overlay metadata from overlay_base.
Then it removes the overlay metadata from the in kernel devicetree data
structure.  It is a hack, it is ugly, but it enables some overlay unit
tests.

Quit trying to change overlay_base.dts.

In my suggested changes to the base patch I put overlay_base.dtb in the
list of overlays for fdtoverlay to apply (apply_static_overlay in the
Makefile) because overlay_base.dts is compiled as an overlay into
overlay_base.dtb and it can be applied on top of the base tree
testcases.dtb.  This gives a little bit more testcase data for
fdtoverlay from an existing dtb.

If you keep trying to change overlay_base.dts I will just tell you
to remove overlay_base.dtb from apply_static_overlay, and then the
test coverage will become smaller.  I do not see that as a good change.

If you want more extensive testing of fdtoverlay, then create your
own specific test cases from scratch and submit patches for them
to the kernel or to the dtc compiler project.

> 
> And shouldn't we create two separate dtb-s now, static_test.dtb and
> static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with
> testcase.dtb anyway.
> 
> Or maybe we can create another file static_overlay.dts (like testcases.dts)
> which can include both testcases.dts and overlay_base.dts, and then we can
> create static_test.dtb out of it ? That won't impact the runtime tests at all.
> 

Stop trying to use all of the unittest .dts test data files.  It is convenient
that so many of them can be used in their current form.  That is goodness
and nice leveraging.  Just ignore the .dts test data files that are not
easily consumed by fdtoverlay.

The email threads around the various versions of this patch series show how
normal devicetree knowledgeable people look at the contents of some of the
.dts test data files and think that they are incorrect.  That is because
the way that unittest uses them is not normal.  Trying to modify one or two
of the many unittest .dts test data files so that they are usable by both
the static fdtoverlay and the run time unittest is not worth it.

-Frank

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[Viresh Kumar]

On 20-01-21, 23:00, Frank Rowand wrote:
> unittest.c first unflattens overlay_base.dtb during early boot.  Then later
> it does some phandle resolution using the overlay metadata from overlay_base.
> Then it removes the overlay metadata from the in kernel devicetree data
> structure.  It is a hack, it is ugly, but it enables some overlay unit
> tests.
> 
> Quit trying to change overlay_base.dts.

I have already done so (in the latest series I sent yesterday).

> In my suggested changes to the base patch I put overlay_base.dtb in the
> list of overlays for fdtoverlay to apply (apply_static_overlay in the
> Makefile) because overlay_base.dts is compiled as an overlay into
> overlay_base.dtb and it can be applied on top of the base tree
> testcases.dtb.  This gives a little bit more testcase data for
> fdtoverlay from an existing dtb.

Okay, but fdtoverlay tool can't apply overlay_base.dtb to
testcases.dtb as none of its node have the __overlay__ property and so
I have entirely skipped overlay_base.dtb and overlay.dtb now.

Yes this reduces the test coverage a bit as you said, but I don't see
a way to make it work right now. And I am not even sure if it is a
fdtoverlay bug, it expects the __overlay__ thing to be there for each
node, otherwise it can't figure out where this node should be applied.

-- 
viresh

Re: [PATCH] of: unittest: Statically apply overlays using fdtoverlay

[David Gibson]

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

On Wed, Jan 20, 2021 at 11:00:17PM -0600, Frank Rowand wrote:
> 
> +David
> 
> so I don't have to repeat this in another thread
> 
> On 1/19/21 11:06 PM, Viresh Kumar wrote:
> > On 19-01-21, 09:44, Frank Rowand wrote:
> >> No.  overlay_base.dts is intentionally compiled into a base FDT, not
> >> an overlay.  Unittest intentionally unflattens this FDT in early boot,
> >> in association with unflattening the system FDT.  One key intent
> >> behind this is to use the same memory allocation method that is
> >> used for the system FDT.
> >>
> >> Do not try to convert overlay_base.dts into an overlay.
> > 
> > Okay, but why does it have /plugin/; specified in it then ?
> 
> OK, so I sortof lied about overlay_base.dts not being an overlay.  It is
> a Frankenstein monster or a Schrodinger's dts/dtb.  It is not a normal
> object.  Nobody who is not looking at how it is abused inside unittest.c
> should be trying to touch it or understand it.

In that case, it absolutely should not be used as your standard base
dtb.

Note that overlays in general rely on particular details of the base
dtb they apply to - they'll need certain symbols and expect certain
paths to be there.  So applying random overlays to a "standard" base
dtb sounds destined to failure anyway.

Also, whatever they hell you're doing with testcases.dts sounds like a
terrible idea to begin with.

> unittest.c first unflattens overlay_base.dtb during early boot.  Then later
> it does some phandle resolution using the overlay metadata from overlay_base.
> Then it removes the overlay metadata from the in kernel devicetree data
> structure.  It is a hack, it is ugly, but it enables some overlay unit
> tests.
> 
> Quit trying to change overlay_base.dts.
> 
> In my suggested changes to the base patch I put overlay_base.dtb in the
> list of overlays for fdtoverlay to apply (apply_static_overlay in the
> Makefile) because overlay_base.dts is compiled as an overlay into
> overlay_base.dtb and it can be applied on top of the base tree
> testcases.dtb.  This gives a little bit more testcase data for
> fdtoverlay from an existing dtb.
> 
> If you keep trying to change overlay_base.dts I will just tell you
> to remove overlay_base.dtb from apply_static_overlay, and then the
> test coverage will become smaller.  I do not see that as a good change.
> 
> If you want more extensive testing of fdtoverlay, then create your
> own specific test cases from scratch and submit patches for them
> to the kernel or to the dtc compiler project.
> 
> > 
> > And shouldn't we create two separate dtb-s now, static_test.dtb and
> > static_overlay_test.dtb ? As fdtoverlay will not be able to merge it with
> > testcase.dtb anyway.
> > 
> > Or maybe we can create another file static_overlay.dts (like testcases.dts)
> > which can include both testcases.dts and overlay_base.dts, and then we can
> > create static_test.dtb out of it ? That won't impact the runtime tests at all.
> > 
> 
> Stop trying to use all of the unittest .dts test data files.  It is convenient
> that so many of them can be used in their current form.  That is goodness
> and nice leveraging.  Just ignore the .dts test data files that are not
> easily consumed by fdtoverlay.
> 
> The email threads around the various versions of this patch series show how
> normal devicetree knowledgeable people look at the contents of some of the
> .dts test data files and think that they are incorrect.  That is because
> the way that unittest uses them is not normal.  Trying to modify one or two
> of the many unittest .dts test data files so that they are usable by both
> the static fdtoverlay and the run time unittest is not worth it.
> 
> -Frank
> 

-- 
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: 833 bytes --]

Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE

[Frank Rowand]

Hi Viresh,

On 1/6/21 11:15 PM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need these tools going forward. Lets start fetching them.
> 
> Note that a copy of fdtdump.c was already copied back in the year 2012,
> but was never updated or built for some reason.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2: Separate out this change from Makefile one.
> 
> This needs to be followed by invocation of the ./update-dtc-source.sh
> script so the relevant files can be copied before the Makefile is
> updated in the next patch.

Just an FYI that Rob will do the ./update-dtc-source.sh step at the appropriate
time, creating a commit to be submitted in his pull request to Linus.

That way Rob will ensure that all of the updates from the parent project are
updated in a careful manner.

-Frank

> 
>  scripts/dtc/update-dtc-source.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
> index bc704e2a6a4a..9bc4afb71415 100755
> --- a/scripts/dtc/update-dtc-source.sh
> +++ b/scripts/dtc/update-dtc-source.sh
> @@ -31,9 +31,9 @@ set -ev
>  DTC_UPSTREAM_PATH=`pwd`/../dtc
>  DTC_LINUX_PATH=`pwd`/scripts/dtc
>  
> -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
> -		srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
> -		dtc-lexer.l dtc-parser.y"
> +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \
> +		fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \
> +		util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y"
>  LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
>  		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
>  		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
> 

Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE

[Viresh Kumar]

On 11-01-21, 16:13, Frank Rowand wrote:
> Hi Viresh,
> 
> On 1/6/21 11:15 PM, Viresh Kumar wrote:
> > We will start building overlays for platforms soon in the kernel and
> > would need these tools going forward. Lets start fetching them.
> > 
> > Note that a copy of fdtdump.c was already copied back in the year 2012,
> > but was never updated or built for some reason.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> > V2: Separate out this change from Makefile one.
> > 
> > This needs to be followed by invocation of the ./update-dtc-source.sh
> > script so the relevant files can be copied before the Makefile is
> > updated in the next patch.
> 
> Just an FYI that Rob will do the ./update-dtc-source.sh step at the appropriate
> time, creating a commit to be submitted in his pull request to Linus.
> 
> That way Rob will ensure that all of the updates from the parent project are
> updated in a careful manner.

Right, this is what I expected. I still wrote this in the patch
description to make sure others, who may want to try this stuff,
understand how this works.

-- 
viresh

Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE

[Frank Rowand]

On 1/6/21 11:15 PM, Viresh Kumar wrote:
> We will start building overlays for platforms soon in the kernel and
> would need these tools going forward. Lets start fetching them.
> 
> Note that a copy of fdtdump.c was already copied back in the year 2012,
> but was never updated or built for some reason.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> V2: Separate out this change from Makefile one.
> 
> This needs to be followed by invocation of the ./update-dtc-source.sh
> script so the relevant files can be copied before the Makefile is
> updated in the next patch.
> 
>  scripts/dtc/update-dtc-source.sh | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
> index bc704e2a6a4a..9bc4afb71415 100755
> --- a/scripts/dtc/update-dtc-source.sh
> +++ b/scripts/dtc/update-dtc-source.sh
> @@ -31,9 +31,9 @@ set -ev
>  DTC_UPSTREAM_PATH=`pwd`/../dtc
>  DTC_LINUX_PATH=`pwd`/scripts/dtc
>  
> -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
> -		srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
> -		dtc-lexer.l dtc-parser.y"
> +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \
> +		fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \
> +		util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y"
>  LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
>  		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
>  		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
> 

DTC_SOURCE is for the dtc program.  Please add a FDTOVERLAY_SOURCE and
related use for the fdtoverlay program.

-Frank

Re: [PATCH V2 1/2] scripts: dtc: Add fdtoverlay.c and fdtdump.c to DTC_SOURCE

[Frank Rowand]

On 1/19/21 10:21 AM, Frank Rowand wrote:
> On 1/6/21 11:15 PM, Viresh Kumar wrote:
>> We will start building overlays for platforms soon in the kernel and
>> would need these tools going forward. Lets start fetching them.
>>
>> Note that a copy of fdtdump.c was already copied back in the year 2012,
>> but was never updated or built for some reason.
>>
>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>> ---
>> V2: Separate out this change from Makefile one.
>>
>> This needs to be followed by invocation of the ./update-dtc-source.sh
>> script so the relevant files can be copied before the Makefile is
>> updated in the next patch.
>>
>>  scripts/dtc/update-dtc-source.sh | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/scripts/dtc/update-dtc-source.sh b/scripts/dtc/update-dtc-source.sh
>> index bc704e2a6a4a..9bc4afb71415 100755
>> --- a/scripts/dtc/update-dtc-source.sh
>> +++ b/scripts/dtc/update-dtc-source.sh
>> @@ -31,9 +31,9 @@ set -ev
>>  DTC_UPSTREAM_PATH=`pwd`/../dtc
>>  DTC_LINUX_PATH=`pwd`/scripts/dtc
>>  
>> -DTC_SOURCE="checks.c data.c dtc.c dtc.h flattree.c fstree.c livetree.c srcpos.c \
>> -		srcpos.h treesource.c util.c util.h version_gen.h yamltree.c \
>> -		dtc-lexer.l dtc-parser.y"
>> +DTC_SOURCE="checks.c data.c dtc.c dtc.h fdtdump.c fdtoverlay.c flattree.c \
>> +		fstree.c livetree.c srcpos.c srcpos.h treesource.c util.c \
>> +		util.h version_gen.h yamltree.c dtc-lexer.l dtc-parser.y"
>>  LIBFDT_SOURCE="fdt.c fdt.h fdt_addresses.c fdt_empty_tree.c \
>>  		fdt_overlay.c fdt_ro.c fdt_rw.c fdt_strerror.c fdt_sw.c \
>>  		fdt_wip.c libfdt.h libfdt_env.h libfdt_internal.h"
>>
> 
> DTC_SOURCE is for the dtc program.  Please add a FDTOVERLAY_SOURCE and
> related use for the fdtoverlay program.

I see that this patch series is up to v4, so I commented in the wrong place.
I will repeat this comment in the v4 series.

-Frank

> 
> -Frank
>