Re: [PATCH v7 2/2] arm64: boot: Support Flat Image Tree

[Ahmad Fatoum]

Hello Simon,

On 29.11.23 20:02, Simon Glass wrote:
> Hi Ahmad,
> 
> On Wed, 29 Nov 2023 at 11:59, Ahmad Fatoum <a.fatoum@xxxxxxxxxxxxxx> wrote:
>>
>> Hi,
>>
>> a few more comments after decompiling the FIT image:
>>
>> On 29.11.23 18:21, Simon Glass wrote:
>>> +    with fsw.add_node('kernel'):
>>> +        fsw.property_string('description', args.name)
>>> +        fsw.property_string('type', 'kernel_noload')
>>
>> The specification only says no loading done, but doesn't explain what it
>> means for a bootloader to _not_ load an image. Looking into the U-Boot commit
>> b9b50e89d317 ("image: Implement IH_TYPE_KERNEL_NOLOAD") that introduces this,
>> apparently no loading means ignoring load and entry address?
>>
>> I presume missing load and entry is something older U-Boot versions
>> were unhappy about? Please let me know if the semantics are as I understood,
>> so I can prepare a barebox patch supporting it.
> 
> Oh, see my previous email.

Thanks.

> 
>>
>>> +        fsw.property_string('arch', args.arch)
>>> +        fsw.property_string('os', args.os)
>>> +        fsw.property_string('compression', args.compress)
>>> +        fsw.property('data', data)
>>> +        fsw.property_u32('load', 0)
>>> +        fsw.property_u32('entry', 0)
>>> +
>>> +
>>> +def finish_fit(fsw, entries):
>>> +    """Finish the FIT ready for use
>>> +
>>> +    Writes the /configurations node and subnodes
>>> +
>>> +    Args:
>>> +        fsw (libfdt.FdtSw): Object to use for writing
>>> +        entries (list of tuple): List of configurations:
>>> +            str: Description of model
>>> +            str: Compatible stringlist
>>> +    """
>>> +    fsw.end_node()
>>> +    seq = 0
>>> +    with fsw.add_node('configurations'):
>>> +        for model, compat in entries:
>>> +            seq += 1
>>> +            with fsw.add_node(f'conf-{seq}'):
>>> +                fsw.property('compatible', bytes(compat))
>>
>> The specification says that this is the root U-Boot compatible,
>> which I presume to mean the top-level compatible, which makes sense to me.
>>
>> The code here though adds all compatible strings from the device tree though,
>> is this intended?
> 
> Yes, since it saves needing to read in each DT just to get the
> compatible stringlist.

The spec reads as if only one string (root) is supposed to be in the list.
The script adds all compatibles though. This is not really useful as a bootloader
that's compatible with e.g. fsl,imx8mm would just take the first device tree
with that SoC, which is most likely to be wrong. It would be better to just
specify the top-level compatible, so the bootloader fails instead of taking
the first DT it finds.

>>> +        fsw.property_string('description', model)
>>> +        fsw.property_string('type', 'flat_dt')
>>> +        fsw.property_string('arch', arch)
>>> +        fsw.property_string('compression', compress)
>>> +        fsw.property('compatible', bytes(compat))
>>
>> I think I've never seen a compatible for a fdt node before.
>> What use does this serve?
> 
> It indicates the machine that the DT is for.

Who makes use of this information?

Thanks,
Ahmad

> 
> Regards,
> Simon
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |