[PATCH v6 0/6] FWU: Add support for mtd backed feature on DeveloperBox

[Michal Simek]

On 6/20/23 16:14, Jassi Brar wrote:
> On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas
> <ilias.apalodimas at linaro.org> wrote:
>>
>> Sorry for being late to the party,
>>
>> +cc Jose who maintains DEN0118
>>
>> On Mon, Jun 19, 2023 at 11:16:53AM -0500, Jassi Brar wrote:
>>> Hi Michal,
>>>
>>> On Mon, 19 Jun 2023 at 10:02, Michal Simek <michal.simek at amd.com> wrote:
>>>>
>>>> Hi Jassi,
>>>>
>>>> On 5/31/23 07:28, jaswinder.singh at linaro.org wrote:
>>>>> From: Jassi Brar <jaswinder.singh at linaro.org>
>>>>>
>>>>> Introduce support for mtd backed storage for FWU feature and enable it on
>>>>> Synquacer platform based DeveloperBox.
>>>>>
>>>>> This revision is rebased onto patchset that trims the FWU api
>>>>>    https://lore.kernel.org/u-boot/20230306231747.1888513-1-jassisinghbrar@gmail.com/
>>>>>
>>> .....
>>>
>>>> Firstly I can generate 2 images per one bank which should be pretty much regular
>>>> capsule update for 2 images. I would expect this should still work.
>>>>
>>>> And then I tried 2 banks with 2 images and fwu_gen_alt_info_from_mtd() generated
>>>> this description for DFU
>>>>
>>>> mtd nor0=bank0 raw 2320000 80000;bank1 raw 27a0000 8000&mtd nor0=bank0 raw
>>>> 23a0000 4000000;bank1 raw 2820000 4000000
>>>>
>>>> If you look at size in second entry you will see that it is 8000 instead of
>>>> 80000 because it is the same image. That's why curious if you have tested any
>>>> scenario like this.
>>>>
>>> I had, and have, strong doubts about the practicality of 2
>>> images/bank. There aren't enough specification details to explain how
>>> only 1-out-of-N images could be updated. And if we always update all
>>> images in a bank together, we might as well have them as one composite
>>> image. I never got satisfactory clarification from designers and
>>> implementers. So, sorry, I can't defend that scenario with my limited
>>> knowledge.
>>
>> This is intentionally left out, as we consider it a platform policy.
>> For example you could update a single bank and copy over the remaining
>> firmware images from a different ban. There's nothing on the spec that
>> prohibits you from doing so, but I personally think it's a bad idea.
>>
> Me too.
> 
>> Keep in mind there's a document hosted here[0] which explains the update
>> flow and documents what we have as code in U-Boot.
>>
>> As far as individual firmware components go now,  do you have any useful
>> usecase?
>>
> No.  And I don't think current A/B update implementation in u-boot
> even has helpers for platforms to implement multiple images per bank.
> Which is fine, except we pretend we do by having NUM_IMAGES_PER_BANK
> config -- which will always be set to 1 I predict ;)

Actually support is there but it requires one thing to patch. I will send that 
patch when I clean my repo.
And I have tested it on our platform.

> 
>>   The general guidance of [0] is construct a firmware
>> image, only update the components you want while leaving the rest on the
>> same revisions and update the entire firmware.  The reason is that firmware
>>
>> Updating a single image of another bank is not as easy as it sounds.
>>
> exactly.
> 
>> Firmware components nowadays, whether we like it or not, depend on each other.
>> Since firmware updates are not so often and fast,  we didn't see any gains from
>> over complicating the spec and explicitly describe that case, while dealing
>> with firmware component compatibility at the same time.
>>
> Totally.  As I said, I don't believe in the practicality of more than
> 1 image/bank.
> So we are on the same page.
> 
>>>
>>>> Next part which I want to take a look is practicality of CONFIG_FWU_NUM_BANKS
>>>> and CONFIG_FWU_NUM_IMAGES_PER_BANK because it pretty much enforcing number of
>>>> banks and images for every platform and prevent creating one u-boot which works
>>>> on different boards and just use information from mdata.
>>>> DEN0118 doesn't show any field with this information but do you think that would
>>>> be possible to extract that information from there based on for example reserved
>>>> or accepted field?
>>>>
>>> Unfortunately the DEN0118 spec doesn't leave any 'don't care' bits in
>>> 'accepted' or 'reserved' fields, all unused bits Must-Be-Zero. If we
>>> use any bit, we'll be in violation of the spec.
>>
>> Yes this is unfortunate.  We did have similar concerns on when drafting the
>> spec,  however we never had a solid usecase to justify this.  Arm and
>> Linaro and working on a v2 (which we try to make compatible with v1) which
>> addresses this shortcoming.  Maybe Jose can chime in.
>>

Another way would be to simply decode this information directly from DT 
fwu-mdata node. Because from that node it is clear how many banks and how many 
images that particular bank has.

Thanks,
Michal