Re: [PATCH] imx8m: set sane default value for SPL_LOAD_FIT_ADDRESS

[Rasmus Villemoes <ravi@prevas.dk>]

On Thu, Oct 24 2024, Marek Vasut <marex@denx.de> wrote:

> On 10/24/24 5:20 PM, Tom Rini wrote:
>> On Thu, Oct 24, 2024 at 04:06:03PM +0200, Marek Vasut wrote:
>>> On 10/24/24 3:18 PM, Rasmus Villemoes wrote:
>>>> On Thu, Oct 24 2024, Marek Vasut <marex@denx.de> wrote:
>>>>
>>>>> On 10/24/24 12:01 PM, Rasmus Villemoes wrote:
>>>>>> I enabled IMX_HAB on an imx8mp board, but even though I knew about the
>>>>>> implementation, I forgot that I had to provide a sane value for
>>>>>> SPL_LOAD_FIT_ADDRESS. The help text for IMX_HAB doesn't mention this
>>>>>> implicit requirement, and there's no build-time warning; the default
>>>>>> 0x0 value just ends up being returned from
>>>>>> board_spl_fit_buffer_addr(), obviously resulting in a non-booting
>>>>>> board.
>>>>>> The existing imx8m* board configs that set a non-zero value
>>>>>> currently
>>>>>> all use 0x44000000. The actual value doesn't matter too much, but 0 is
>>>>>> always wrong for imx8m platforms. So just use 0x44000000 as default
>>>>>> for those platforms.
>>>>>> Signed-off-by: Rasmus Villemoes <ravi@prevas.dk>
>>>>>> ---
>>>>>>     boot/Kconfig | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>> diff --git a/boot/Kconfig b/boot/Kconfig
>>>>>> index 940389d4882..72d1f69afcd 100644
>>>>>> --- a/boot/Kconfig
>>>>>> +++ b/boot/Kconfig
>>>>>> @@ -231,6 +231,7 @@ config SPL_LOAD_FIT
>>>>>>     config SPL_LOAD_FIT_ADDRESS
>>>>>>     	hex "load address of fit image"
>>>>>>     	depends on SPL_LOAD_FIT
>>>>>> +	default 0x44000000 if ARCH_IMX8M
>>>>> This only applies to HAB , for non-HAB the fitImage can be loaded at
>>>>> arbitrary location, do you need:
>>>>>
>>>>> default 0x44000000 if ARCH_IMX8M && IMX_HAB
>>>>>
>>>>> right ?
>>>>
>>>> On IMX8 without HAB, the value is not used at all AFAICT (otherwise the
>>>> value of 0x0 would have caused trouble already). I don't see the harm of
>>>> setting some sane value that's actually within DRAM space independent of
>>>> HAB.
>>>>
>>>> Making the default depend on IMX_HAB has the UX problem that if I do
>>>> "make imx8mp_evk_defconfig", then go about tweaking stuff, then do "make
>>>> menuconfig" and enable IMX_HAB, SPL_LOAD_FIT_ADDRESS already has the
>>>> bogus 0x0 value and nothing forces or asks that to be changed. Making the
>>>> default depend only on SOC (or SOC family or whatever IMX8M is in this
>>>> context) makes that problem go away.
>>> Looking at this closely, common/spl/spl_ram.c does make use of
>>> CONFIG_SPL_LOAD_FIT_ADDRESS too, so I think yes, we should have a default
>>> for this.
>>>
>>> iMX6 should have 0x14000000
>>> iMX7 should have 0x84000000
>>> iMX8M should have 0x44000000
>> These differ, slightly, from the value used in CONFIG_SYS_LOAD_ADDR.
>> Could that not be used (and the overall option changed to default
>> SYS_LOAD_ADDR ?
> If I recall it right, no ... these addresses are where the fitImage is
> loaded and where HAB does its authentication stuff on the fitImage,
> that address has to be fixed and well aligned.

Yes. Fixed certainly; I'm not aware of any specific aligment
requirements, but I can't imagine it would need more than 0x1000
alignment, and we'd never use a fixed address less aligned than that
anyway.

> The SYS_LOAD_ADDR is
> the destination address where the u-boot-nodtb.bin is copied from the
> fitImage AFTER the fitImage was authenticated.

No, I don't think so. Isn't u-boot-nodtb.bin just copied to the value of
the load= property in the FIT, which is CONFIG_TEXT_BASE, aka 0x40200000
in the imx8mp case.

As for Tom's suggestion to make SPL_LOAD_FIT_ADDRESS simply default to
SYS_LOAD_ADDR: Perhaps, it would certainly be better than 0x0 which is
almost always bogus. I think it would work for imx8mp_evk, though
0x40200000 and 0x40480000 are only 2.5MiB apart. But I have no idea how
many boards would be affected by such a change or how many random ad hoc
other CONFIG_FOO_THIS_OR_THAT exists that might or might not clash or
overlap. IOW, I'm not signing off on such a patch... But it could be
interesting to a least let CI chew on that to see if it's a complete
non-starter.

Rasmus