[PATCH] imx8m: set sane default value for SPL_LOAD_FIT_ADDRESS

[Marek Vasut]

On 10/25/24 10:41 AM, Rasmus Villemoes wrote:
> On Thu, Oct 24 2024, Marek Vasut <marex at 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 at 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 at 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.

Ah, right, thanks for keeping an eye on this.

> 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.

How about introducing a default set of text bases and load addresses, 
with the right spacing, and then start removing the custom ones from 
board configs ?