Re: [PATCH v2 01/10] xilinx: Kconfig: add XILINX_OF_BOARD_DTB_ADDR default value for microblaze

From: Ovidiu Panait <ovidiu.panait@windriver.com>
To: Michal Simek <monstr@monstr.eu>,
	u-boot@lists.denx.de, Michal Simek <michal.simek@xilinx.com>
Cc: Masahiro Yamada <yamada.masahiro@socionext.com>
Subject: Re: [PATCH v2 01/10] xilinx: Kconfig: add XILINX_OF_BOARD_DTB_ADDR default value for microblaze
Date: Wed, 1 Dec 2021 19:49:06 +0200	[thread overview]
Message-ID: <10b8f5b8-903a-0977-8d9b-6cdf3c2ba018@windriver.com> (raw)
In-Reply-To: <b58abec4-40bb-c51f-b413-cfc71fa07930@monstr.eu>

Hi Michal,

On 12/1/21 12:10 PM, Michal Simek wrote:
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 11/30/21 17:33, Ovidiu Panait wrote:
>> Xilinx board_fdt_blob_setup() implementation makes use of
>> XILINX_OF_BOARD_DTB_ADDR Kconfig option, but no default value is 
>> currently
>> defined for microblaze. Add one so that microblaze could also be 
>> configured
>> with CONFIG_OF_SEPARATE.
>>
>> Signed-off-by: Ovidiu Panait <ovidiu.panait@windriver.com>
>> ---
>>
>> (no changes since v1)
>>
>>   board/xilinx/Kconfig | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/board/xilinx/Kconfig b/board/xilinx/Kconfig
>> index 64507b5d84..9e69166903 100644
>> --- a/board/xilinx/Kconfig
>> +++ b/board/xilinx/Kconfig
>> @@ -43,7 +43,7 @@ endif
>>   config XILINX_OF_BOARD_DTB_ADDR
>>       hex "Default DTB pickup address"
>>       default 0x1000 if ARCH_VERSAL
>> -     default 0x100000 if ARCH_ZYNQ || ARCH_ZYNQMP
>> +     default 0x100000 if ARCH_ZYNQ || ARCH_ZYNQMP || MICROBLAZE
>>       depends on OF_BOARD || OF_SEPARATE
>>       help
>>         Offset in the memory where the board configuration DTB is 
>> placed.
>>
>
> First of all I applied patches 2-10.
>
> And let's have short discussion about this default address. 1MB is quite
> high for DTB. In standard system when you have brams you likely don't
> have more 1MB. That's why this default is not the best.
> I didn't use microblaze spl for a while but from build it has ~30kB. Not
> sure where I did setup stack, early malloc area but I think having place
> around 32kB/64kB would be more reasonable default because you will have
> brams there.
> And having dtb in bram is likely better because it can be the part of
> bitstream.
> Can you please elaborate a little bit why you choose 1M for microblaze?
>
You're right, 1MB is not appropriate for microblaze. I can respin this 
patch
and change it to 32kB.

My initial intention with patches 1/10 and 2/10 was to switch microblaze 
from
CONFIG_OF_EMBEDDED -> CONFIG_OF_SEPARATE, but then I realized that the
updatemem utility used to bundle spl and bitstream together can only 
operate on
ELF files. Since for CONFIG_OF_SEPARATE the generated u-boot-spl ELF 
does not
contain the dtb, I abandoned the idea, but I still included the patches 
to at
least fix the compilation errors. So I haven't really put much thought into
what default address would be appropriate for microblaze.

Ovidiu

> 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 ARM64 SoCs
> U-Boot custodian - Xilinx Microblaze/Zynq/ZynqMP/Versal SoCs
>