MicroBlaze is the only architecture that supports a built-in DTB in
its own way.

Other architectures (e.g., ARC, NIOS2, RISC-V, etc.) use the common
infrastructure introduced by commit aab94339cd85 ("of: Add support for
linking device tree blobs into vmlinux").

This commit migrates MicroBlaze to this common infrastructure.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

I do not know why MicroBlaze still adopts its own way.
Perhaps, because MicroBlaze supports the built-in DTB
before aab94339cd85 and nobody attempted migration.
Anyway, I only compile-tested this patch.
I hope the maintainer can do boot-testing.

 arch/microblaze/boot/Makefile          | 3 +--
 arch/microblaze/boot/dts/Makefile      | 5 +----
 arch/microblaze/boot/dts/linked_dtb.S  | 2 --
 arch/microblaze/include/asm/sections.h | 2 --
 arch/microblaze/kernel/head.S          | 2 +-
 arch/microblaze/kernel/setup.c         | 4 ++--
 arch/microblaze/kernel/vmlinux.lds.S   | 8 --------
 7 files changed, 5 insertions(+), 21 deletions(-)
 delete mode 100644 arch/microblaze/boot/dts/linked_dtb.S

diff --git a/arch/microblaze/boot/Makefile b/arch/microblaze/boot/Makefile
index 2b42c370d574..23a48e090f93 100644
--- a/arch/microblaze/boot/Makefile
+++ b/arch/microblaze/boot/Makefile
@@ -17,8 +17,7 @@ $(obj)/linux.bin.gz: $(obj)/linux.bin FORCE
 	$(call if_changed,gzip)
 
 quiet_cmd_strip = STRIP   $< $@$2
-	cmd_strip = $(STRIP) -K microblaze_start -K _end -K __log_buf \
-				-K _fdt_start $< -o $@$2
+	cmd_strip = $(STRIP) -K microblaze_start -K _end -K __log_buf $< -o $@$2
 
 UIMAGE_LOADADDR = $(CONFIG_KERNEL_BASE_ADDR)
 
diff --git a/arch/microblaze/boot/dts/Makefile b/arch/microblaze/boot/dts/Makefile
index b84e2cbb20ee..f168a127bf94 100644
--- a/arch/microblaze/boot/dts/Makefile
+++ b/arch/microblaze/boot/dts/Makefile
@@ -4,10 +4,7 @@
 dtb-y := system.dtb
 
 ifneq ($(DTB),)
-obj-y += linked_dtb.o
-
-# Ensure system.dtb exists
-$(obj)/linked_dtb.o: $(obj)/system.dtb
+obj-y += system.dtb.o
 
 # Generate system.dtb from $(DTB).dtb
 ifneq ($(DTB),system)
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/include/asm/sections.h b/arch/microblaze/include/asm/sections.h
index a9311ad84a67..6bc4855757c3 100644
--- a/arch/microblaze/include/asm/sections.h
+++ b/arch/microblaze/include/asm/sections.h
@@ -14,7 +14,5 @@
 extern char _ssbss[], _esbss[];
 extern unsigned long __ivt_start[], __ivt_end[];
 
-extern u32 _fdt_start[], _fdt_end[];
-
 # endif /* !__ASSEMBLY__ */
 #endif /* _ASM_MICROBLAZE_SECTIONS_H */
diff --git a/arch/microblaze/kernel/head.S b/arch/microblaze/kernel/head.S
index ec2fcb545e64..9727aa1934df 100644
--- a/arch/microblaze/kernel/head.S
+++ b/arch/microblaze/kernel/head.S
@@ -95,7 +95,7 @@ big_endian:
 	bnei	r11, no_fdt_arg			/* No - get out of here */
 _prepare_copy_fdt:
 	or	r11, r0, r0 /* incremment */
-	ori	r4, r0, TOPHYS(_fdt_start)
+	ori	r4, r0, TOPHYS(__dtb_start)
 	ori	r3, r0, (0x10000 - 4)
 _copy_fdt:
 	lw	r12, r7, r11 /* r12 = r7 + r11 */
diff --git a/arch/microblaze/kernel/setup.c b/arch/microblaze/kernel/setup.c
index f417333eccae..8e57b490ca9c 100644
--- a/arch/microblaze/kernel/setup.c
+++ b/arch/microblaze/kernel/setup.c
@@ -120,7 +120,7 @@ void __init machine_early_init(const char *cmdline, unsigned int ram,
 	memset(_ssbss, 0, _esbss-_ssbss);
 
 /* initialize device tree for usage in early_printk */
-	early_init_devtree(_fdt_start);
+	early_init_devtree(__dtb_start);
 
 	/* setup kernel_tlb after BSS cleaning
 	 * Maybe worth to move to asm code */
@@ -132,7 +132,7 @@ void __init machine_early_init(const char *cmdline, unsigned int ram,
 	if (fdt)
 		pr_info("FDT at 0x%08x\n", fdt);
 	else
-		pr_info("Compiled-in FDT at %p\n", _fdt_start);
+		pr_info("Compiled-in FDT at %p\n", __dtb_start);
 
 #ifdef CONFIG_MTD_UCLINUX
 	pr_info("Found romfs @ 0x%08x (0x%08x)\n",
diff --git a/arch/microblaze/kernel/vmlinux.lds.S b/arch/microblaze/kernel/vmlinux.lds.S
index ae50d3d04a7d..3d4a78aa9ab4 100644
--- a/arch/microblaze/kernel/vmlinux.lds.S
+++ b/arch/microblaze/kernel/vmlinux.lds.S
@@ -44,14 +44,6 @@ SECTIONS {
 		_etext = . ;
 	}
 
-	. = ALIGN (8) ;
-	__fdt_blob : AT(ADDR(__fdt_blob) - LOAD_OFFSET) {
-		_fdt_start = . ;		/* place for fdt blob */
-		*(__fdt_blob) ;			/* Any link-placed DTB */
-	        . = _fdt_start + 0x10000;	/* Pad up to 64kbyte */
-		_fdt_end = . ;
-	}
-
 	. = ALIGN(16);
 	RO_DATA(4096)
 
-- 
2.43.0

On 9/18/24 06:52, Masahiro Yamada wrote:
> MicroBlaze is the only architecture that supports a built-in DTB in
> its own way.
> 
> Other architectures (e.g., ARC, NIOS2, RISC-V, etc.) use the common
> infrastructure introduced by commit aab94339cd85 ("of: Add support for
> linking device tree blobs into vmlinux").
> 
> This commit migrates MicroBlaze to this common infrastructure.
> 
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
> I do not know why MicroBlaze still adopts its own way.
> Perhaps, because MicroBlaze supports the built-in DTB
> before aab94339cd85 and nobody attempted migration.
> Anyway, I only compile-tested this patch.
> I hope the maintainer can do boot-testing.


I took a look at it and it is changing current behavior.
If you look at linux.bin and there is no DT inside. But when you patch is 
applied linux.bin contains system.dtb inside which is regression.
Or is it intention of this patch?

I think there was any documentation about it's usage in past but never really 
described in upstream kernel.
But idea was that linux.bin requires DT to be passed from bootloader via R7 reg 
but simpleImage.X is linux.bin+DTB inside and can be used without bootloader.

Thanks,
Michal

On Fri, Nov 8, 2024 at 8:54 PM Michal Simek <monstr@monstr.eu> wrote:
>
>
>
> On 9/18/24 06:52, Masahiro Yamada wrote:
> > MicroBlaze is the only architecture that supports a built-in DTB in
> > its own way.
> >
> > Other architectures (e.g., ARC, NIOS2, RISC-V, etc.) use the common
> > infrastructure introduced by commit aab94339cd85 ("of: Add support for
> > linking device tree blobs into vmlinux").
> >
> > This commit migrates MicroBlaze to this common infrastructure.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> > I do not know why MicroBlaze still adopts its own way.
> > Perhaps, because MicroBlaze supports the built-in DTB
> > before aab94339cd85 and nobody attempted migration.
> > Anyway, I only compile-tested this patch.
> > I hope the maintainer can do boot-testing.
>
> I took a look at it and it is changing current behavior.
> If you look at linux.bin and there is no DT inside. But when you patch is
> applied linux.bin contains system.dtb inside which is regression.
> Or is it intention of this patch?


I do not understand your comment.

If you look at the code in arch/microblaze/Makefile,
DTB is empty unless you build simpleImage.%


My patch still keeps obj-y within the
ifneq ($(DTB),) ... endif  block.


ifneq ($(DTB),)
obj-y += system.dtb.o
  [ snip ]
endif


So, when you build linux.bin, system.dtb is not embedded in vmlinux,
is it?



> I think there was any documentation about it's usage in past but never really
> described in upstream kernel.
> But idea was that linux.bin requires DT to be passed from bootloader via R7 reg
> but simpleImage.X is linux.bin+DTB inside and can be used without bootloader.

With my patch applied, it should still work like this.


-- 
Best Regards
Masahiro Yamada

On 11/8/24 18:27, Masahiro Yamada wrote:
> On Fri, Nov 8, 2024 at 8:54 PM Michal Simek <monstr@monstr.eu> wrote:
>>
>>
>>
>> On 9/18/24 06:52, Masahiro Yamada wrote:
>>> MicroBlaze is the only architecture that supports a built-in DTB in
>>> its own way.
>>>
>>> Other architectures (e.g., ARC, NIOS2, RISC-V, etc.) use the common
>>> infrastructure introduced by commit aab94339cd85 ("of: Add support for
>>> linking device tree blobs into vmlinux").
>>>
>>> This commit migrates MicroBlaze to this common infrastructure.
>>>
>>> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
>>> ---
>>>
>>> I do not know why MicroBlaze still adopts its own way.
>>> Perhaps, because MicroBlaze supports the built-in DTB
>>> before aab94339cd85 and nobody attempted migration.
>>> Anyway, I only compile-tested this patch.
>>> I hope the maintainer can do boot-testing.
>>
>> I took a look at it and it is changing current behavior.
>> If you look at linux.bin and there is no DT inside. But when you patch is
>> applied linux.bin contains system.dtb inside which is regression.
>> Or is it intention of this patch?
> 
> 
> I do not understand your comment.
> 
> If you look at the code in arch/microblaze/Makefile,
> DTB is empty unless you build simpleImage.%
> 
> 
> My patch still keeps obj-y within the
> ifneq ($(DTB),) ... endif  block.
> 
> 
> ifneq ($(DTB),)
> obj-y += system.dtb.o
>    [ snip ]
> endif
> 
> 
> So, when you build linux.bin, system.dtb is not embedded in vmlinux,
> is it?
> 
> 
> 
>> I think there was any documentation about it's usage in past but never really
>> described in upstream kernel.
>> But idea was that linux.bin requires DT to be passed from bootloader via R7 reg
>> but simpleImage.X is linux.bin+DTB inside and can be used without bootloader.
> 
> With my patch applied, it should still work like this.

I have looked at it again and there is an issue in behavior.
drivers/of/empty_root.dts is default dts/dtb which is placed to .dtb.init.rodata 
section in linux.bin.
It means by default kernel has DT in this location all the time.

Anyway let's go back to the patch.

You are removing __fdt_blob location which has 64kB size all the time.
(. = _fdt_start + 0x10000;       /* Pad up to 64kbyte */)
When DT is passed via R7 the code below is executed.

/* r7 may point to an FDT, or there may be one linked in.
    if it's in r7, we've got to save it away ASAP.
    We ensure r7 points to a valid FDT, just in case the bootloader
    is broken or non-existent */
	beqi	r7, no_fdt_arg			/* NULL pointer?  don't copy */
/* Does r7 point to a valid FDT? Load HEADER magic number */
	/* Run time Big/Little endian platform */
	/* Save 1 as word and load byte - 0 - BIG, 1 - LITTLE */
	lbui	r11, r0, TOPHYS(endian_check)
	beqid	r11, big_endian /* DO NOT break delay stop dependency */
	lw	r11, r0, r7 /* Big endian load in delay slot */
	lwr	r11, r0, r7 /* Little endian load */
big_endian:
	rsubi	r11, r11, OF_DT_HEADER	/* Check FDT header */
	beqi	r11, _prepare_copy_fdt
	or	r7, r0, r0		/* clear R7 when not valid DTB */
	bnei	r11, no_fdt_arg			/* No - get out of here */
_prepare_copy_fdt:
	or	r11, r0, r0 /* incremment */
	ori	r4, r0, TOPHYS(_fdt_start)
	ori	r3, r0, (0x10000 - 4)
_copy_fdt:
	lw	r12, r7, r11 /* r12 = r7 + r11 */
	sw	r12, r4, r11 /* addr[r4 + r11] = r12 */
	addik	r11, r11, 4 /* increment counting */
	bgtid	r3, _copy_fdt /* loop for all entries */
	addik	r3, r3, -4 /* descrement loop */
no_fdt_arg:


It copies passed DTB to location prepared location inside the kernel with max 
value of 64kB.

When your patch is applied and fdt_blob location is replaced by dtb_start 
location there is no 64kB space because in linux.bin there is only empty_root.dtb.
It means when DT is passed kernel itself rewrite data behind allocated space. As 
is visible from System.map.

c08d5400 D __dtb_empty_root_begin
c08d5400 D __dtb_start
c08d5448 D __dtb_empty_root_end
c08d5460 D __dtb_end
c08d5460 D __irqchip_of_table
c08d5460 d __of_table_xilinx_intc_opb
c08d5524 d __of_table_xilinx_intc_xps

It means your patch works properly when kernel has dtb inside (simpleImage) but 
break cases where DTB is passed via kernel command line.

And .dtb.init.rodata has all the time DTB inside with 0xd00dfeed marker that's 
why early_init_dt_verify() pass fdt_check_header() because marker is there. And 
likely fails later anyway.
I don't think this is key issue here but code flow is a little bit different 
then was before too.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Xilinx Microblaze
Maintainer of Linux kernel - Xilinx Zynq ARM and ZynqMP/Versal ARM64 SoCs
U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal/Versal NET SoCs
TF-A maintainer - Xilinx ZynqMP/Versal/Versal NET SoCs

On Wed, Sep 18, 2024 at 1:54 PM Masahiro Yamada <masahiroy@kernel.org> wrote:
>
> MicroBlaze is the only architecture that supports a built-in DTB in
> its own way.
>
> Other architectures (e.g., ARC, NIOS2, RISC-V, etc.) use the common
> infrastructure introduced by commit aab94339cd85 ("of: Add support for
> linking device tree blobs into vmlinux").
>
> This commit migrates MicroBlaze to this common infrastructure.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---

Ping?



>
> I do not know why MicroBlaze still adopts its own way.
> Perhaps, because MicroBlaze supports the built-in DTB
> before aab94339cd85 and nobody attempted migration.
> Anyway, I only compile-tested this patch.
> I hope the maintainer can do boot-testing.
>
>  arch/microblaze/boot/Makefile          | 3 +--
>  arch/microblaze/boot/dts/Makefile      | 5 +----
>  arch/microblaze/boot/dts/linked_dtb.S  | 2 --
>  arch/microblaze/include/asm/sections.h | 2 --
>  arch/microblaze/kernel/head.S          | 2 +-
>  arch/microblaze/kernel/setup.c         | 4 ++--
>  arch/microblaze/kernel/vmlinux.lds.S   | 8 --------
>  7 files changed, 5 insertions(+), 21 deletions(-)
>  delete mode 100644 arch/microblaze/boot/dts/linked_dtb.S
>
> diff --git a/arch/microblaze/boot/Makefile b/arch/microblaze/boot/Makefile
> index 2b42c370d574..23a48e090f93 100644
> --- a/arch/microblaze/boot/Makefile
> +++ b/arch/microblaze/boot/Makefile
> @@ -17,8 +17,7 @@ $(obj)/linux.bin.gz: $(obj)/linux.bin FORCE
>         $(call if_changed,gzip)
>
>  quiet_cmd_strip = STRIP   $< $@$2
> -       cmd_strip = $(STRIP) -K microblaze_start -K _end -K __log_buf \
> -                               -K _fdt_start $< -o $@$2
> +       cmd_strip = $(STRIP) -K microblaze_start -K _end -K __log_buf $< -o $@$2
>
>  UIMAGE_LOADADDR = $(CONFIG_KERNEL_BASE_ADDR)
>
> diff --git a/arch/microblaze/boot/dts/Makefile b/arch/microblaze/boot/dts/Makefile
> index b84e2cbb20ee..f168a127bf94 100644
> --- a/arch/microblaze/boot/dts/Makefile
> +++ b/arch/microblaze/boot/dts/Makefile
> @@ -4,10 +4,7 @@
>  dtb-y := system.dtb
>
>  ifneq ($(DTB),)
> -obj-y += linked_dtb.o
> -
> -# Ensure system.dtb exists
> -$(obj)/linked_dtb.o: $(obj)/system.dtb
> +obj-y += system.dtb.o
>
>  # Generate system.dtb from $(DTB).dtb
>  ifneq ($(DTB),system)
> 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/include/asm/sections.h b/arch/microblaze/include/asm/sections.h
> index a9311ad84a67..6bc4855757c3 100644
> --- a/arch/microblaze/include/asm/sections.h
> +++ b/arch/microblaze/include/asm/sections.h
> @@ -14,7 +14,5 @@
>  extern char _ssbss[], _esbss[];
>  extern unsigned long __ivt_start[], __ivt_end[];
>
> -extern u32 _fdt_start[], _fdt_end[];
> -
>  # endif /* !__ASSEMBLY__ */
>  #endif /* _ASM_MICROBLAZE_SECTIONS_H */
> diff --git a/arch/microblaze/kernel/head.S b/arch/microblaze/kernel/head.S
> index ec2fcb545e64..9727aa1934df 100644
> --- a/arch/microblaze/kernel/head.S
> +++ b/arch/microblaze/kernel/head.S
> @@ -95,7 +95,7 @@ big_endian:
>         bnei    r11, no_fdt_arg                 /* No - get out of here */
>  _prepare_copy_fdt:
>         or      r11, r0, r0 /* incremment */
> -       ori     r4, r0, TOPHYS(_fdt_start)
> +       ori     r4, r0, TOPHYS(__dtb_start)
>         ori     r3, r0, (0x10000 - 4)
>  _copy_fdt:
>         lw      r12, r7, r11 /* r12 = r7 + r11 */
> diff --git a/arch/microblaze/kernel/setup.c b/arch/microblaze/kernel/setup.c
> index f417333eccae..8e57b490ca9c 100644
> --- a/arch/microblaze/kernel/setup.c
> +++ b/arch/microblaze/kernel/setup.c
> @@ -120,7 +120,7 @@ void __init machine_early_init(const char *cmdline, unsigned int ram,
>         memset(_ssbss, 0, _esbss-_ssbss);
>
>  /* initialize device tree for usage in early_printk */
> -       early_init_devtree(_fdt_start);
> +       early_init_devtree(__dtb_start);
>
>         /* setup kernel_tlb after BSS cleaning
>          * Maybe worth to move to asm code */
> @@ -132,7 +132,7 @@ void __init machine_early_init(const char *cmdline, unsigned int ram,
>         if (fdt)
>                 pr_info("FDT at 0x%08x\n", fdt);
>         else
> -               pr_info("Compiled-in FDT at %p\n", _fdt_start);
> +               pr_info("Compiled-in FDT at %p\n", __dtb_start);
>
>  #ifdef CONFIG_MTD_UCLINUX
>         pr_info("Found romfs @ 0x%08x (0x%08x)\n",
> diff --git a/arch/microblaze/kernel/vmlinux.lds.S b/arch/microblaze/kernel/vmlinux.lds.S
> index ae50d3d04a7d..3d4a78aa9ab4 100644
> --- a/arch/microblaze/kernel/vmlinux.lds.S
> +++ b/arch/microblaze/kernel/vmlinux.lds.S
> @@ -44,14 +44,6 @@ SECTIONS {
>                 _etext = . ;
>         }
>
> -       . = ALIGN (8) ;
> -       __fdt_blob : AT(ADDR(__fdt_blob) - LOAD_OFFSET) {
> -               _fdt_start = . ;                /* place for fdt blob */
> -               *(__fdt_blob) ;                 /* Any link-placed DTB */
> -               . = _fdt_start + 0x10000;       /* Pad up to 64kbyte */
> -               _fdt_end = . ;
> -       }
> -
>         . = ALIGN(16);
>         RO_DATA(4096)
>
> --
> 2.43.0
>


-- 
Best Regards
Masahiro Yamada