[PATCH v4 02/11] FWU: Add FWU metadata access driver for GPT partitioned block devices

[Masami Hiramatsu]

Hi Sughosh,

2022年2月10日(木) 21:25 Sughosh Ganu <sughosh.ganu at linaro.org>:

>
> hi Masami,
>
> On Thu, 10 Feb 2022 at 08:45, Masami Hiramatsu
> <masami.hiramatsu at linaro.org> wrote:
> >
> > 2022年2月10日(木) 10:43 Masami Hiramatsu <masami.hiramatsu at linaro.org>:
> > >
> > > > > > > Of course as I said in the other thread, I would like to put the
> > > > > > > dfu_alt_info like information in the devicetree too.
> > > > > > > (But it maybe different from this discussion)
> > > > > >
> > > > > > Just curious, why do you need to define a variable like dfu_alt_info
> > > > > > in a device tree. If this has already been described earlier, you can
> > > > > > point me to that discussion. Trying to understand what benefit does
> > > > > > having the variables defined in a device tree brings. Thanks.
> > > > >
> > > > > If we can consolidate the configuration information related to the
> > > > > firmware layout on the devicetree, it is very easy to understand
> > > > > and manage the firmware update only by checking the devicetree.
> > > > > Current design is very fragile from the consistency viewpoint,
> > > > > because there are 3 different information we are using for FWU,
> > > > > Kconfig, devicetree and u-boot env-variables. If one of them
> > > > > sets inconsistent value, FWU may not work as we expected.
> > > >
> > > > I get your point. But I think generating the dfu_alt_info at runtime,
> > > > like how it is done for the ST platforms is in general a better
> > > > method, as against using a static variable defined in the device tree.
> > >
> > > Yeah, the GPT based one is able to store this information on the GPT,
> > > and it must be a primary definition.
> > >
> > > > With runtime generation of the variable, the same code can be used on
> > > > multiple platforms and can be generated at runtime -- I feel that is
> > > > better than defining the variable in every platform's device tree.
> > >
> > > I don't agree this point at least for non-GPT devices, since the
> > > firmware storage layout depends on the platform hardware configuration
> > > statically in such cases.
> >
> > I changed my mind, it can be solved if we have "uuid" property for
> > each partition in the devicetree. I will explain later.
> >
> > > Of course if the device uses GPT to store the firmware, we have to
> > > follow the GPT layout and FWU Metadata to find the corresponding
> > > firmware partition.
> > >
> > > > Btw, there is also provision to define the variable(or part of it)
> > > > statically through Kconfig variables. As against your concern about
> > > > the feature using multiple methods for stating information, it is
> > > > indeed valid. But I guess we can have documentation explaining how
> > > > each of that information needs to be defined. Thanks.
> > >
> > > Yeah, even using GPT, we need to set correct UUID to the FWU metadata,
> > > and the metadata depends on Kconfig if we keep putting the #of
> > > images-per-bank and the #of banks in the Kconfig, and storage
> >
> > Sorry, I confused. there are "#of images and #of banks per image".
> >
> > > (controller) is defined in the devicetree.
> > >
> > > And I still feel this "chain of definitions" seems a bit fragile. This
> > > fragile comes from the FWU metadata is not enough self-described (it
> > > has no the #of images-per-bank and the #of banks, without this
> > > information we can not decode FWU metadata itself.)
> > > Anyway, if we can define the #of images-per-bank and the #of banks in
> > > the devicetree, we don't need to change the binary but just changing
> > > the devicetree for the different products which has different firmware
> > > layout. I think that is more flexible.
>
> Do you really feel that using config values for #banks and
> #images_per_bank is such a bad idea. With the approach that you
> suggest, we will have to use variable sized arrays, and populate the
> values in the probe function of every driver. Also, since you are
> going to use the same fwu_mdata.h header in your other project, will
> you be able to use variable sized arrays there as well. I feel that we
> are complicating things without much benefits.

Hm, indeed. It may make us to handle the metadata as a single data
structure on memory. We may need allocate several objects (and arrays)
and decode/encode the data from/to storage device. OK, then let those
parameters keep in the kconfig.

> >
> > What I would like to suggest is
> >
> > /* For GPT BLK backend */
> > fwu_mdata {
> >     compatible = "u-boot,fwu-mdata-gpt";
> >     fwu-mdata-store = <&mmc1>;
> >     /* No need to specify the mdata partition, because it finds the
> > mdata by partition type uuid. */
> >     banks = <2>;
> >     images-per-bank = <1>;
> > };
> >
> > /* For SF backend */
> > fwu_mdata {
> >     compatible = "u-boot,fwu-mdata-sf";
> >     fwu-mdata-store = <&spi-flash0>;
> >     mdata-offsets = <500000, 520000>; /* Or specified by partition label? */
> >     banks = <6>;
> >     images-per-bank = <1>;
> > };
>
> Looks good overall. Should the mdata-offsets property be instead
> mdata-parts, where we define the start offset and size of the metadata
> partitions. Or are we going to figure out the size of the metadata
> partition through some other mechanism.

mdata-parts may only need the label names, since we can find the
partition by name. The partition size can be defined by partitions
node. But do we really need the size? I'm reserving an enough
bigger area for mdata. (And if it is NAND, we need to reserve
"a page" for mdata)

> Also, do are using a different compatible string for every type of
> storage device type. Can we not do with a common string instead? I
> understand your reasoning here, of trying to identify the driver at
> runtime. Just that I wonder if we are going to build multiple drivers
> for a platform. Although I do not have a strong opinion on this.

I can't imagine the case that a platform has different mdata store
at the same time... Anyway, as you said, we need the different
compatible strings for different drivers.

> > Note that this is only for the metadata, the real firmware layout issue
> > still exists. If we can add "uuid" property for the fixed-partitions node
> > as a additional property, e.g.
> >
> > spi-flash at 0 {
> >    partitions {
> >        compatible = "fixed-partitions";
> >        ...
> >        uuid = "aaaaaaaa-bbbb-cccc-dddd-eeeeffffgggg";
> >        ...
> >        partition at 600000 {
> >            label = "Firmware-Bank0";
> >            reg = <600000, 400000>;
> >            uuid = "12345678-aaaa-bbbb-cccc-0123456789ab";
> >        };
> >        ...
> >    };
> > };
> >
> > Then we can decode the real fwu-mdata and find corresponding
> > partitions, and able to build dfu_alt_info in runtime.
>
> This is a sound idea. With this method, we can still use the GUIDs in
> the metadata structure, and map those with the partitions using the
> above nodes. Only question is, are these partition
> nodes(partition at 600000) standard, and are defined for all platforms,
> for all type of devices(like NOR, Nand). If so, this scheme will work.

Yes, as far as I know, now mtd subsystem requires to define the partitions
by the devicetree, and that is acceptable both U-Boot and Linux.
You can find the "fixed-partitions" definition in the Linux dt-bindings.

https://www.kernel.org/doc/Documentation/devicetree/bindings/mtd/partitions/fixed-partitions.yaml

I think we can add uuid because there is "additionalProperties: true".

We need the image-type-id uuid, but we can get it from fwu mdata.

Thus, the boot sequence will be
1. parse devicetree.
2. probe fwu-mdata driver and load mdata.
3. platform driver builds dfu_alt_info according to the mdata.
    a. read mdata and identify the partitions ofnode by location uuid.
    b. find partition ofnodes by image-uuid.
    c. build dfu-alt-info by the ofnodes information.
    d. build guid-array by mdata's image-type uuid.

Thank you,

--
Masami Hiramatsu