mbox series

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

Message ID 20230531052804.21871-1-jaswinder.singh@linaro.org
Headers show
Series FWU: Add support for mtd backed feature on DeveloperBox | expand

Message

Jassi Brar May 31, 2023, 5:28 a.m. UTC
From: Jassi Brar <jaswinder.singh@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/

Changes since v5:
		* Some of the typo fixes and cosmetic changes suggested by Etienne

Changes since v4:
        * Provide default/weak implementations of fwu_plat_get_alt_num and fwu_plat_get_bootidx
        * Provide man page for mkfwumdata
        * Misc typo fixes and cosmetic changes

Changes since v3:
        * Fix and Update documentation to also build optee for FWU FIP image.
        * Fixed checkpatch warnings
        * Made local functions static.
        * Split config changes to a separate patch
        * Fix authorship of three patches.


Jassi Brar (4):
  dt: fwu: developerbox: enable fwu banks and mdata regions
  configs: move to new flash layout and boot flow
  fwu: DeveloperBox: add support for FWU
  fwu: provide default fwu_plat_get_bootidx

Masami Hiramatsu (2):
  FWU: Add FWU metadata access driver for MTD storage regions
  tools: Add mkfwumdata tool for FWU metadata image

 .../synquacer-sc2a11-developerbox-u-boot.dtsi |  49 ++-
 board/socionext/developerbox/Makefile         |   1 +
 board/socionext/developerbox/developerbox.c   |   8 +
 board/socionext/developerbox/fwu_plat.c       |  37 ++
 configs/synquacer_developerbox_defconfig      |  12 +-
 doc/board/socionext/developerbox.rst          | 154 +++++++-
 doc/mkfwumdata.1                              |  89 +++++
 drivers/fwu-mdata/Kconfig                     |  15 +
 drivers/fwu-mdata/Makefile                    |   1 +
 drivers/fwu-mdata/raw_mtd.c                   | 269 ++++++++++++++
 include/configs/synquacer.h                   |  10 +
 include/fwu.h                                 |  32 ++
 lib/fwu_updates/Makefile                      |   1 +
 lib/fwu_updates/fwu.c                         |  18 +
 lib/fwu_updates/fwu_mtd.c                     | 185 ++++++++++
 tools/Kconfig                                 |   9 +
 tools/Makefile                                |   4 +
 tools/mkfwumdata.c                            | 334 ++++++++++++++++++
 18 files changed, 1217 insertions(+), 11 deletions(-)
 create mode 100644 board/socionext/developerbox/fwu_plat.c
 create mode 100644 doc/mkfwumdata.1
 create mode 100644 drivers/fwu-mdata/raw_mtd.c
 create mode 100644 lib/fwu_updates/fwu_mtd.c
 create mode 100644 tools/mkfwumdata.c

Comments

Tom Rini June 10, 2023, 12:37 a.m. UTC | #1
On Wed, 31 May 2023 00:28:04 -0500, jaswinder.singh@linaro.org wrote:

> From: Jassi Brar <jaswinder.singh@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/
> 
> [...]

Applied to u-boot/next, thanks!
Michal Simek June 19, 2023, 3:01 p.m. UTC | #2
Hi Jassi,

On 5/31/23 07:28, jaswinder.singh@linaro.org wrote:
> From: Jassi Brar <jaswinder.singh@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/
> 
> Changes since v5:
> 		* Some of the typo fixes and cosmetic changes suggested by Etienne
> 
> Changes since v4:
>          * Provide default/weak implementations of fwu_plat_get_alt_num and fwu_plat_get_bootidx
>          * Provide man page for mkfwumdata
>          * Misc typo fixes and cosmetic changes
> 
> Changes since v3:
>          * Fix and Update documentation to also build optee for FWU FIP image.
>          * Fixed checkpatch warnings
>          * Made local functions static.
>          * Split config changes to a separate patch
>          * Fix authorship of three patches.
> 
> 
> Jassi Brar (4):
>    dt: fwu: developerbox: enable fwu banks and mdata regions
>    configs: move to new flash layout and boot flow
>    fwu: DeveloperBox: add support for FWU
>    fwu: provide default fwu_plat_get_bootidx
> 
> Masami Hiramatsu (2):
>    FWU: Add FWU metadata access driver for MTD storage regions
>    tools: Add mkfwumdata tool for FWU metadata image
> 
>   .../synquacer-sc2a11-developerbox-u-boot.dtsi |  49 ++-
>   board/socionext/developerbox/Makefile         |   1 +
>   board/socionext/developerbox/developerbox.c   |   8 +
>   board/socionext/developerbox/fwu_plat.c       |  37 ++
>   configs/synquacer_developerbox_defconfig      |  12 +-
>   doc/board/socionext/developerbox.rst          | 154 +++++++-
>   doc/mkfwumdata.1                              |  89 +++++
>   drivers/fwu-mdata/Kconfig                     |  15 +
>   drivers/fwu-mdata/Makefile                    |   1 +
>   drivers/fwu-mdata/raw_mtd.c                   | 269 ++++++++++++++
>   include/configs/synquacer.h                   |  10 +
>   include/fwu.h                                 |  32 ++
>   lib/fwu_updates/Makefile                      |   1 +
>   lib/fwu_updates/fwu.c                         |  18 +
>   lib/fwu_updates/fwu_mtd.c                     | 185 ++++++++++
>   tools/Kconfig                                 |   9 +
>   tools/Makefile                                |   4 +
>   tools/mkfwumdata.c                            | 334 ++++++++++++++++++
>   18 files changed, 1217 insertions(+), 11 deletions(-)
>   create mode 100644 board/socionext/developerbox/fwu_plat.c
>   create mode 100644 doc/mkfwumdata.1
>   create mode 100644 drivers/fwu-mdata/raw_mtd.c
>   create mode 100644 lib/fwu_updates/fwu_mtd.c
>   create mode 100644 tools/mkfwumdata.c
> 

I know that Tom has applied this already but I still think there is a work to do 
here.
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.

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?

Thanks,
Michal
Jassi Brar June 19, 2023, 4:16 p.m. UTC | #3
Hi Michal,

On Mon, 19 Jun 2023 at 10:02, Michal Simek <michal.simek@amd.com> wrote:
>
> Hi Jassi,
>
> On 5/31/23 07:28, jaswinder.singh@linaro.org wrote:
> > From: Jassi Brar <jaswinder.singh@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.

> 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.

However, we can do CRC32 calculations by varying NUM_IMAGES_PER_BANK
and NUM_BANKS and find the value pair for which the crc32 field
matches!
For limiting check-loops and finding corrupted metadata, the .config
can carry upper limits on both the options, say
MAX_NUM_IMAGES_PER_BANK=5 and MAX_NUM_BANKS=10 for the most special
scenario. If we find the approach acceptable, I can cook a patch as
proof-of-concept.

cheers.
Ilias Apalodimas June 20, 2023, 10:05 a.m. UTC | #4
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@amd.com> wrote:
> >
> > Hi Jassi,
> >
> > On 5/31/23 07:28, jaswinder.singh@linaro.org wrote:
> > > From: Jassi Brar <jaswinder.singh@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.
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?  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.
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.

>
> > 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.

>
> However, we can do CRC32 calculations by varying NUM_IMAGES_PER_BANK
> and NUM_BANKS and find the value pair for which the crc32 field
> matches!
> For limiting check-loops and finding corrupted metadata, the .config
> can carry upper limits on both the options, say
> MAX_NUM_IMAGES_PER_BANK=5 and MAX_NUM_BANKS=10 for the most special
> scenario. If we find the approach acceptable, I can cook a patch as
> proof-of-concept.
>
> cheers.

[0] https://gitlab.com/Linaro/trustedsubstrate/mbfw/uploads/3d0d7d11ca9874dc9115616b418aa330/mbfw.pdf

Thanks
/Ilias
Jassi Brar June 20, 2023, 2:14 p.m. UTC | #5
On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas
<ilias.apalodimas@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@amd.com> wrote:
> > >
> > > Hi Jassi,
> > >
> > > On 5/31/23 07:28, jaswinder.singh@linaro.org wrote:
> > > > From: Jassi Brar <jaswinder.singh@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.
Michal Simek June 20, 2023, 2:20 p.m. UTC | #6
On 6/20/23 16:14, Jassi Brar wrote:
> On Tue, 20 Jun 2023 at 05:05, Ilias Apalodimas
> <ilias.apalodimas@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@amd.com> wrote:
>>>>
>>>> Hi Jassi,
>>>>
>>>> On 5/31/23 07:28, jaswinder.singh@linaro.org wrote:
>>>>> From: Jassi Brar <jaswinder.singh@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
Jose Marinho June 21, 2023, 9:36 a.m. UTC | #7
Hi,

> -----Original Message-----
> From: Michal Simek <michal.simek@amd.com>
> Sent: Tuesday, June 20, 2023 3:20 PM
> To: Jassi Brar <jaswinder.singh@linaro.org>; Ilias Apalodimas
> <ilias.apalodimas@linaro.org>
> Cc: Jose Marinho <Jose.Marinho@arm.com>; u-boot@lists.denx.de;
> etienne.carriere@linaro.org; trini@konsulko.com; sjg@chromium.org;
> sughosh.ganu@linaro.org; xypron.glpk@gmx.de; takahiro.akashi@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@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@amd.com>
> wrote:
> >>>>
> >>>> Hi Jassi,
> >>>>
> >>>> On 5/31/23 07:28, jaswinder.singh@linaro.org wrote:
> >>>>> From: Jassi Brar <jaswinder.singh@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@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
>