Re: [PATCH v2 1/1] of: overlay: rename overlay source files from .dts to .dtso

[Frank Rowand <frowand.list@gmail.com>]

On 5/3/22 16:42, Rob Herring wrote:
> On Tue, May 3, 2022 at 4:20 PM <frowand.list@gmail.com> wrote:
>>
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>> In drivers/of/unittest-data/:
>>    - Rename .dts overlay source files to use .dtso suffix.
>>    - Add Makefile rule to build .dtbo.o assembly file from overlay
>>      .dtso source file.
>>    - Update Makefile to build .dtbo.o objects instead of .dtb.o from
>>      unittest overlay source files.
>>
>> Modify driver/of/unitest.c to use .dtbo.o based symbols instead of
>> .dtb.o
>>
>> Modify scripts/Makefile.lib %.dtbo rule to depend upon %.dtso instead
>> of %.dts
>>
>> Rename .dts overlay source files to use .dtso suffix in:
>>    arch/arm64/boot/dts/freescale/
>>    arch/arm64/boot/dts/xilinx/
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>
>> Testing by arm64 people would be much appreciated.
>>
>> Applies on branch dt/next, commit 4fb74186cee8 of Rob's kernel.org tree.
>> git://git.kernel.org/pub/scm/linux/kernel/git/robh/linux
>>
>> version 1 patch:
>>    https://lore.kernel.org/r/20210324223713.1334666-1-frowand.list@gmail.com
>>
>> changes from version 1:
>>    - rebase to 5.18-rc1 plus many patches already accepted by Rob
>>      Applies on branch dt/next, commit 4fb74186cee8 of Rob's kernel.org tree.
>>    - Add new overlay source files in:
>>       arch/arm64/boot/dts/freescale/
>>       arch/arm64/boot/dts/xilinx/
> 
> I can't apply these. They need to be applied separately. And probably
> at end of the merge window or right after rc1 (IOW, coordinated with
> SoC maintainers in advance). Or we support both forms for a cycle.

If applied separately then git bisect is broken.  I don't see this change
as being big enough to be considered a "flag day" change, but if I can't
get the SoC maintainers to ack you pulling these renames then I can easily
re-spin in a way to support both forms for a release cycle.

> 
> [...]
> 
>> diff --git a/drivers/of/unittest-data/Makefile b/drivers/of/unittest-data/Makefile
>> index d072f3ba3971..df2ca1820273 100644
>> --- a/drivers/of/unittest-data/Makefile
>> +++ b/drivers/of/unittest-data/Makefile
>> @@ -1,38 +1,58 @@
>>  # SPDX-License-Identifier: GPL-2.0
>> -obj-y += testcases.dtb.o
>>
>> -obj-$(CONFIG_OF_OVERLAY) += overlay.dtb.o \
>> -                           overlay_0.dtb.o \
>> -                           overlay_1.dtb.o \
>> -                           overlay_2.dtb.o \
>> -                           overlay_3.dtb.o \
>> -                           overlay_4.dtb.o \
>> -                           overlay_5.dtb.o \
>> -                           overlay_6.dtb.o \
>> -                           overlay_7.dtb.o \
>> -                           overlay_8.dtb.o \
>> -                           overlay_9.dtb.o \
>> -                           overlay_10.dtb.o \
>> -                           overlay_11.dtb.o \
>> -                           overlay_12.dtb.o \
>> -                           overlay_13.dtb.o \
>> -                           overlay_15.dtb.o \
>> -                           overlay_16.dtb.o \
>> -                           overlay_17.dtb.o \
>> -                           overlay_18.dtb.o \
>> -                           overlay_19.dtb.o \
>> -                           overlay_20.dtb.o \
>> -                           overlay_bad_add_dup_node.dtb.o \
>> -                           overlay_bad_add_dup_prop.dtb.o \
>> -                           overlay_bad_phandle.dtb.o \
>> -                           overlay_bad_symbol.dtb.o \
>> -                           overlay_base.dtb.o \
>> -                           overlay_gpio_01.dtb.o \
>> -                           overlay_gpio_02a.dtb.o \
>> -                           overlay_gpio_02b.dtb.o \
>> -                           overlay_gpio_03.dtb.o \
>> -                           overlay_gpio_04a.dtb.o \
>> -                           overlay_gpio_04b.dtb.o
>> +# Generate an assembly file to wrap the output of the device tree compiler
>> +quiet_cmd_dt_S_dtbo= DTB     $@
>> +cmd_dt_S_dtbo=\
>> +{                                                      \
>> +       echo '\#include <asm-generic/vmlinux.lds.h>';   \
>> +       echo '.section .dtb.init.rodata,"a"';           \
>> +       echo '.balign STRUCT_ALIGNMENT';                \
>> +       echo '.global __dtbo_$(subst -,_,$(*F))_begin'; \
>> +       echo '__dtbo_$(subst -,_,$(*F))_begin:';        \
>> +       echo '.incbin "$<" ';                           \
>> +       echo '__dtbo_$(subst -,_,$(*F))_end:';          \
>> +       echo '.global __dtbo_$(subst -,_,$(*F))_end';   \
>> +       echo '.balign STRUCT_ALIGNMENT';                \
>> +} > $@
>> +
>> +
>> +$(obj)/%.dtbo.S: $(obj)/%.dtbo FORCE
>> +       $(call if_changed,dt_S_dtbo)
> 
> Humm, this belongs in scripts/Makefile.lib.

I would rather keep it isolated to just the use in unittest.
We just now got rid of the final driver use of of_overlay_fdt_apply()
by the grandfathered legacy user in:

   841281fe52a7 drm: rcar-du: Drop LVDS device tree backward compatibility

That driver was the only use of %.dtb.S for an overlay.

I can't eliminate the rule for %.dtb.S because that is used in several
cases for building the system FDT into the kernel.  But having this
rule in drivers/of/unittest-data/Makefile provides consistent naming
of overlays withing unittest.  Helping to make it clear that they are
not a system FDT.

> 
> I don't think we need a different section name with __dtbo_* instead
> of __dtb_* if that simplifies the Makefile.

A different section name is not needed if the rule is moved to
scripts/Makefile.lib but even if moved there, I prefer to keep the
overlay data clearly delineated from the system FDT data.

There is also a kernel test robot email warning about randconfig
arc build errors:

>> make[4]: *** No rule to make target 'drivers/of/unittest-data/static_base_1.dtb', needed by 'drivers/of/unittest-data/static_test_1.dtb'.
>> make[4]: *** No rule to make target 'drivers/of/unittest-data/static_base_2.dtb', needed by 'drivers/of/unittest-data/static_test_2.dtb'.

They look like a pre-existing problem, not new.  I'll get their
build environment and see what's going on.

-Frank

> 
>> +
>> +obj-y += testcases.dtbo.o
>> +
>> +obj-$(CONFIG_OF_OVERLAY) += overlay.dtbo.o \
>> +                           overlay_0.dtbo.o \
>> +                           overlay_1.dtbo.o \
>> +                           overlay_2.dtbo.o \
>> +                           overlay_3.dtbo.o \
>> +                           overlay_4.dtbo.o \
>> +                           overlay_5.dtbo.o \
>> +                           overlay_6.dtbo.o \
>> +                           overlay_7.dtbo.o \
>> +                           overlay_8.dtbo.o \
>> +                           overlay_9.dtbo.o \
>> +                           overlay_10.dtbo.o \
>> +                           overlay_11.dtbo.o \
>> +                           overlay_12.dtbo.o \
>> +                           overlay_13.dtbo.o \
>> +                           overlay_15.dtbo.o \
>> +                           overlay_16.dtbo.o \
>> +                           overlay_17.dtbo.o \
>> +                           overlay_18.dtbo.o \
>> +                           overlay_19.dtbo.o \
>> +                           overlay_20.dtbo.o \
>> +                           overlay_bad_add_dup_node.dtbo.o \
>> +                           overlay_bad_add_dup_prop.dtbo.o \
>> +                           overlay_bad_phandle.dtbo.o \
>> +                           overlay_bad_symbol.dtbo.o \
>> +                           overlay_base.dtbo.o \
>> +                           overlay_gpio_01.dtbo.o \
>> +                           overlay_gpio_02a.dtbo.o \
>> +                           overlay_gpio_02b.dtbo.o \
>> +                           overlay_gpio_03.dtbo.o \
>> +                           overlay_gpio_04a.dtbo.o \
>> +                           overlay_gpio_04b.dtbo.o
>>
>>  # enable creation of __symbols__ node
>>  DTC_FLAGS_overlay += -@
> 
> 
>> diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
>> index dff55ae09d97..1d3c170d95db 100644
>> --- a/drivers/of/unittest.c
>> +++ b/drivers/of/unittest.c
>> @@ -1423,12 +1423,12 @@ static int __init unittest_data_add(void)
>>         void *unittest_data_align;
>>         struct device_node *unittest_data_node = NULL, *np;
>>         /*
>> -        * __dtb_testcases_begin[] and __dtb_testcases_end[] are magically
>> -        * created by cmd_dt_S_dtb in scripts/Makefile.lib
>> +        * __dtbo_testcases_begin[] and __dtbo_testcases_end[] are
>> +        * created by cmd_dt_S_dtbo in Makefile
>>          */
>> -       extern uint8_t __dtb_testcases_begin[];
>> -       extern uint8_t __dtb_testcases_end[];
>> -       const int size = __dtb_testcases_end - __dtb_testcases_begin;
>> +       extern uint8_t __dtbo_testcases_begin[];
>> +       extern uint8_t __dtbo_testcases_end[];
>> +       const int size = __dtbo_testcases_end - __dtbo_testcases_begin;
>>         int rc;
>>         void *ret;
>>
>> @@ -1443,7 +1443,7 @@ static int __init unittest_data_add(void)
>>                 return -ENOMEM;
>>
>>         unittest_data_align = PTR_ALIGN(unittest_data, FDT_ALIGN_SIZE);
>> -       memcpy(unittest_data_align, __dtb_testcases_begin, size);
>> +       memcpy(unittest_data_align, __dtbo_testcases_begin, size);
>>
>>         ret = of_fdt_unflatten_tree(unittest_data_align, NULL, &unittest_data_node);
>>         if (!ret) {
>> @@ -3009,24 +3009,24 @@ static inline void __init of_unittest_overlay(void) { }
>>  #ifdef CONFIG_OF_OVERLAY
>>
>>  /*
>> - * __dtb_ot_begin[] and __dtb_ot_end[] are created by cmd_dt_S_dtb
>> - * in scripts/Makefile.lib
>> + * __dtbo_##overlay_name##_begin[] and __dtbo_##overlay_name##_end[] are
>> + * created by cmd_dt_S_dtbo in Makefile
>>   */
>>
>> -#define OVERLAY_INFO_EXTERN(name) \
>> -       extern uint8_t __dtb_##name##_begin[]; \
>> -       extern uint8_t __dtb_##name##_end[]
>> +#define OVERLAY_INFO_EXTERN(overlay_name) \
>> +       extern uint8_t __dtbo_##overlay_name##_begin[]; \
>> +       extern uint8_t __dtbo_##overlay_name##_end[]
>>
>> -#define OVERLAY_INFO(overlay_name, expected)             \
>> -{      .dtb_begin       = __dtb_##overlay_name##_begin, \
>> -       .dtb_end         = __dtb_##overlay_name##_end,   \
>> -       .expected_result = expected,                     \
>> -       .name            = #overlay_name,                \
>> +#define OVERLAY_INFO(overlay_name, expected)               \
>> +{      .dtbo_begin       = __dtbo_##overlay_name##_begin, \
>> +       .dtbo_end         = __dtbo_##overlay_name##_end,   \
>> +       .expected_result = expected,                       \
>> +       .name            = #overlay_name,                  \
>>  }
>>
>>  struct overlay_info {
>> -       uint8_t         *dtb_begin;
>> -       uint8_t         *dtb_end;
>> +       uint8_t         *dtbo_begin;
>> +       uint8_t         *dtbo_end;
> 
> Is this rename really needed?
> 
>>         int             expected_result;
>>         int             ovcs_id;
>>         char            *name;
>> @@ -3100,7 +3100,7 @@ static struct overlay_info overlays[] = {
>>         OVERLAY_INFO(overlay_bad_phandle, -EINVAL),
>>         OVERLAY_INFO(overlay_bad_symbol, -EINVAL),
>>         /* end marker */
>> -       {.dtb_begin = NULL, .dtb_end = NULL, .expected_result = 0, .name = NULL}
>> +       {.dtbo_begin = NULL, .dtbo_end = NULL, .expected_result = 0, .name = NULL}
>>  };
>>
>>  static struct device_node *overlay_base_root;
>> @@ -3157,13 +3157,13 @@ void __init unittest_unflatten_overlay_base(void)
>>                 return;
>>         }
>>
>> -       data_size = info->dtb_end - info->dtb_begin;
>> +       data_size = info->dtbo_end - info->dtbo_begin;
>>         if (!data_size) {
>>                 pr_err("No dtb 'overlay_base' to attach\n");
>>                 return;
>>         }
>>
>> -       size = fdt_totalsize(info->dtb_begin);
>> +       size = fdt_totalsize(info->dtbo_begin);
>>         if (size != data_size) {
>>                 pr_err("dtb 'overlay_base' header totalsize != actual size");
>>                 return;
>> @@ -3175,7 +3175,7 @@ void __init unittest_unflatten_overlay_base(void)
>>                 return;
>>         }
>>
>> -       memcpy(new_fdt, info->dtb_begin, size);
>> +       memcpy(new_fdt, info->dtbo_begin, size);
>>
>>         __unflatten_device_tree(new_fdt, NULL, &overlay_base_root,
>>                                 dt_alloc_memory, true);
>> @@ -3210,11 +3210,11 @@ static int __init overlay_data_apply(const char *overlay_name, int *ovcs_id)
>>                 return 0;
>>         }
>>
>> -       size = info->dtb_end - info->dtb_begin;
>> +       size = info->dtbo_end - info->dtbo_begin;
>>         if (!size)
>>                 pr_err("no overlay data for %s\n", overlay_name);
>>
>> -       ret = of_overlay_fdt_apply(info->dtb_begin, size, &info->ovcs_id);
>> +       ret = of_overlay_fdt_apply(info->dtbo_begin, size, &info->ovcs_id);
>>         if (ovcs_id)
>>                 *ovcs_id = info->ovcs_id;
>>         if (ret < 0)
>> diff --git a/scripts/Makefile.lib b/scripts/Makefile.lib
>> index 9f69ecdd7977..b409bec5fa45 100644
>> --- a/scripts/Makefile.lib
>> +++ b/scripts/Makefile.lib
>> @@ -363,7 +363,7 @@ endef
>>  $(obj)/%.dtb: $(src)/%.dts $(DTC) $(DT_TMP_SCHEMA) FORCE
>>         $(call if_changed_rule,dtc)
>>
>> -$(obj)/%.dtbo: $(src)/%.dts $(DTC) FORCE
>> +$(obj)/%.dtbo: $(src)/%.dtso $(DTC) FORCE
>>         $(call if_changed_dep,dtc)
>>
>>  dtc-tmp = $(subst $(comma),_,$(dot-target).dts.tmp)
>> --
>> Frank Rowand <frank.rowand@sony.com>
>>