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

[Jose Marinho]

Hi,

> -----Original Message-----
> From: Michal Simek <michal.simek at amd.com>
> Sent: Tuesday, June 20, 2023 3:20 PM
> To: Jassi Brar <jaswinder.singh at linaro.org>; Ilias Apalodimas
> <ilias.apalodimas at linaro.org>
> Cc: Jose Marinho <Jose.Marinho at arm.com>; u-boot at lists.denx.de;
> etienne.carriere at linaro.org; trini at konsulko.com; sjg at chromium.org;
> sughosh.ganu at linaro.org; xypron.glpk at gmx.de; takahiro.akashi at linaro.org
> Subject: Re: [PATCH v6 0/6] FWU: Add support for mtd backed feature on
> DeveloperBox
> 
> 
> 
> 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-
> jassisingh
> >>>>> brar at 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.
> 

The FW update document (DEN0118) is in the process of being released.
It defines the v2 of the metadata data-structure.
Version 2 should better fit the deployment models since it has a field for 'number of banks' and another for 'number of images per bank'.
Happy to discuss in general, and in particular if we find any gaps 😊!

Regards,
Jose

> Thanks,
> Michal
>