Re: [PATCH] microblaze: migrate to the generic rule for built-in DTB

[Michal Simek]

On 1/14/25 03:41, Masahiro Yamada wrote:
> On Wed, Jan 8, 2025 at 1:37 AM Michal Simek <michal.simek@xxxxxxx> wrote:
> > Hi Masahiro,
> >
> > On 12/22/24 10:46, Masahiro Yamada wrote:
> > > Commit 654102df2ac2 ("kbuild: add generic support for built-in boot
> > > DTBs") introduced generic support for built-in DTBs.
> > >
> > > Select GENERIC_BUILTIN_DTB to use the generic rule.
> > >
> > > MicroBlaze is the only architecture that places the built-in DTB to its
> > > own section, __fdt_blob, rather than the standard location defined by
> > > the KERNEL_DTB() macro.
> > >
> > > For backward compatibility, arch/microblaze/boot/dts/system.dtb is still
> > > placed in the __fdt_blob section, and possibly overwritten by an externel
> > > DTB passed via the r7 register.
> > >
> > > Signed-off-by: Masahiro Yamada <masahiroy@xxxxxxxxxx>
> > > ---
> > >
> > >    arch/microblaze/Kbuild                | 1 -
> > >    arch/microblaze/Kconfig               | 5 +++++
> > >    arch/microblaze/boot/dts/Makefile     | 5 -----
> > >    arch/microblaze/boot/dts/linked_dtb.S | 2 --
> > >    arch/microblaze/kernel/vmlinux.lds.S  | 2 +-
> > >    5 files changed, 6 insertions(+), 9 deletions(-)
> > >    delete mode 100644 arch/microblaze/boot/dts/linked_dtb.S
> > >
> > > diff --git a/arch/microblaze/Kbuild b/arch/microblaze/Kbuild
> > > index 077a0b8e9615..70510389eb92 100644
> > > --- a/arch/microblaze/Kbuild
> > > +++ b/arch/microblaze/Kbuild
> > > @@ -2,7 +2,6 @@
> > >    obj-y                       += kernel/
> > >    obj-y                       += mm/
> > >    obj-$(CONFIG_PCI)   += pci/
> > > -obj-y                        += boot/dts/
> > >
> > >    # for cleaning
> > >    subdir- += boot
> > > diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
> > > index f18ec02ddeb2..4ed8ca89f0c9 100644
> > > --- a/arch/microblaze/Kconfig
> > > +++ b/arch/microblaze/Kconfig
> > > @@ -10,6 +10,7 @@ config MICROBLAZE
> > >        select ARCH_MIGHT_HAVE_PC_PARPORT
> > >        select ARCH_WANT_IPC_PARSE_VERSION
> > >        select BUILDTIME_TABLE_SORT
> > > +     select GENERIC_BUILTIN_DTB
> > >        select TIMER_OF
> > >        select CLONE_BACKWARDS3
> > >        select COMMON_CLK
> > > @@ -47,6 +48,10 @@ config MICROBLAZE
> > >        select TRACE_IRQFLAGS_SUPPORT
> > >        select GENERIC_IRQ_MULTI_HANDLER
> > >
> > > +config BUILTIN_DTB_NAME
> > > +     string
> > > +     default "system"
> > > +
> > >    # Endianness selection
> > >    choice
> > >        prompt "Endianness selection"
> > > diff --git a/arch/microblaze/boot/dts/Makefile b/arch/microblaze/boot/dts/Makefile
> > > index b84e2cbb20ee..87c1d25ff096 100644
> > > --- a/arch/microblaze/boot/dts/Makefile
> > > +++ b/arch/microblaze/boot/dts/Makefile
> > > @@ -4,11 +4,6 @@
> > >    dtb-y := system.dtb
> > >
> > >    ifneq ($(DTB),)
> > > -obj-y += linked_dtb.o
> > > -
> > > -# Ensure system.dtb exists
> > > -$(obj)/linked_dtb.o: $(obj)/system.dtb
> > > -
> > >    # Generate system.dtb from $(DTB).dtb
> > >    ifneq ($(DTB),system)
> > >    $(obj)/system.dtb: $(obj)/$(DTB).dtb
> > > diff --git a/arch/microblaze/boot/dts/linked_dtb.S b/arch/microblaze/boot/dts/linked_dtb.S
> > > deleted file mode 100644
> > > index 23345af3721f..000000000000
> > > --- a/arch/microblaze/boot/dts/linked_dtb.S
> > > +++ /dev/null
> > > @@ -1,2 +0,0 @@
> > > -.section __fdt_blob,"a"
> > > -.incbin "arch/microblaze/boot/dts/system.dtb"
> > > diff --git a/arch/microblaze/kernel/vmlinux.lds.S b/arch/microblaze/kernel/vmlinux.lds.S
> > > index ae50d3d04a7d..e86f9ca8e979 100644
> > > --- a/arch/microblaze/kernel/vmlinux.lds.S
> > > +++ b/arch/microblaze/kernel/vmlinux.lds.S
> > > @@ -47,7 +47,7 @@ SECTIONS {
> > >        . = ALIGN (8) ;
> > >        __fdt_blob : AT(ADDR(__fdt_blob) - LOAD_OFFSET) {
> > >                _fdt_start = . ;                /* place for fdt blob */
> > > -             *(__fdt_blob) ;                 /* Any link-placed DTB */
> > > +             *(.dtb.init.rodata) ;           /* Any link-placed DTB */
> > >                . = _fdt_start + 0x10000;       /* Pad up to 64kbyte */
> > >                _fdt_end = . ;
> > >        }
> >
> > This patch is better then previous one but still it is changing behavior of
> > build. When this patch is applied linux.bin contains dtb which is not the same
> > behavior as before (which was empty).
> > DTB should be filled when simpleImage.<dt> is built.
>
> Why is it a problem?

Firstly that change is not described in commit message.
Second system.dtb is really old dtb more for demonstration purpose and nothing 
else and likely it is not working on any existing board.

> Microblaze always keeps 64kbyte space in case
> an external DTB is passed from a boot-loader.
>
> Even if the default system.dtb is placed there,
> nothing is wasted.

It is not about wasting of space. It is about information which none is going to 
use. That's why linux.bin has no dtb inside.


> One more thing, Microblaze is only the architecture
> that determines the built-in DTB by the command line.

yes and it has been done like this for many years without any complain about it. 
simpleImage has been taken from PPC in past.

> I do not think it is a sensible way.
>
>      $ make linux.bin simpleImage.foo
>        --> linux.bin also contains the builtin device tree
>
>      $ make simpleImage.foo simpleImage.bar
>        --> does not work

yes because foo.dtb and bar.dtb are not in the Linux tree.


> All other architectures determinantes the built-in DTB
> by CONFIG option.

Microblaze is soft core and make no sense to start to push other DTSes to the 
Linux kernel because pretty much every design is unique. That's why only 
system.dts is there for demonstration purpose.

If users have multiple boards they can just add dts to the source code and build 
kernel. And linux.bin should be used with U-Boot ITBs where dtb is passed via 
register.

Thanks,
Michal