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

[Jassi Brar]

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 ;)

>  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.
>
OK. If we have, say, reserved[0] as last Image and reserved[1] as last
Bank, we could get rid of the two compile-time configs.

Thanks.