Re: [PATCH] microblaze: use the common infrastructure to support built-in DTB

[Michal Simek <monstr@monstr.eu>]

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