Re: [PATCH v4 6/6] fwu: DeveloperBox: add support for FWU

[Michal Simek <michal.simek@amd.com>]

On 4/10/23 06:21, Jassi Brar wrote:
> On Wed, 29 Mar 2023 at 08:02, Michal Simek <michal.simek@amd.com> wrote:
>> On 3/27/23 23:16, jassisinghbrar@gmail.com wrote:
> 
> .....
>>> +
>>> +void fwu_plat_get_bootidx(uint *boot_idx)
>>> +{
>>> +     int ret;
>>> +     u32 active_idx;
>>> +     u32 *bootidx = boot_idx;
>>> +
>>> +     ret = fwu_get_active_index(&active_idx);
>>> +
>>
>> nit: remove this newline
>>
> ok
> 
>>> +     if (ret < 0)
>>> +             *bootidx = -1;
>>> +
>>> +     *bootidx = active_idx;
>>
>> Is this logic here right?
>> If fwu_get_active_index fails you setup bootidx to -1
>> and right after it you rewrite it to active_idx initialized in
>> fwu_get_active_index() to mdata->active_index.
>>
>> It means why to do *bootidx = -1; at all?
>>
> yes :) it's a silly remnant of history of changes.
> Actually this goes away after implementing the default/weak function.
> 
> 
>>> +}
>>> diff --git a/configs/synquacer_developerbox_defconfig b/configs/synquacer_developerbox_defconfig
>>> index 09e12b739b..d09684153a 100644
>>> --- a/configs/synquacer_developerbox_defconfig
>>> +++ b/configs/synquacer_developerbox_defconfig
>>> @@ -97,3 +97,11 @@ CONFIG_EFI_RUNTIME_UPDATE_CAPSULE=y
>>>    CONFIG_EFI_CAPSULE_ON_DISK=y
>>>    CONFIG_EFI_IGNORE_OSINDICATIONS=y
>>>    CONFIG_EFI_CAPSULE_FIRMWARE_RAW=y
>>> +CONFIG_EFI_SECURE_BOOT=y
>>> +CONFIG_FWU_MULTI_BANK_UPDATE=y
>>> +CONFIG_FWU_MDATA=y
>>> +CONFIG_FWU_MDATA_MTD=y
>>> +CONFIG_FWU_NUM_BANKS=2
>>> +CONFIG_FWU_NUM_IMAGES_PER_BANK=1
>>> +CONFIG_CMD_FWU_METADATA=y
>>> +CONFIG_TOOLS_MKFWUMDATA=y
>>
>> doesn't look like that it was created via savedefconfig.
>>
> Yes. I had some other config changes too and picked only the relevant
> ones together.

But this is defconfig not documentation.



> 
> 
>>> +And make a FIP image.::
>>> +
>>> +  cp build/synquacer/release/fip.bin SPI_NOR_NEWFIP.fd
>>> +  tools/fiptool/fiptool update --tb-fw build/synquacer/release/bl2.bin SPI_NOR_NEWFIP.fd
>>> +
>>> +UUIDs for the FWU Multi Bank Update
>>> +-----------------------------------
>>> +
>>> +FWU multi-bank update requires some UUIDs. The DeveloperBox platform uses
>>> +following UUIDs.
>>> +
>>> + - Location UUID for the FIP image: 17e86d77-41f9-4fd7-87ec-a55df9842de5
>>
>>
>> In past you have it listed at flash node in DT. I see you have removed it
>> between v3 and v4 without any note about it.
>> Is it still needed? And should it be listed in DT spec again?
>>
> After the dt change, we no longer require this. But the location_uuid
> is a standard member of an fwu_image_entry and cmd/fwu_mdata.c always
> print it. So I think this should be seen as just what a platform wants
> some unique id to be printed for the image (?).

I am fine with your explanation but documentation should make this clear that 
uuid is required by spec but not actually used by current implementation or 
description.

M