[PATCH v4 02/11] FWU: Add FWU metadata access driver for GPT partitioned block devices
Masami Hiramatsu
masami.hiramatsu at linaro.org
Thu Feb 10 02:43:50 CET 2022
Hi Sughosh,
2022年2月10日(木) 3:40 Sughosh Ganu <sughosh.ganu at linaro.org>:
>
> hi Masami,
>
> On Wed, 9 Feb 2022 at 17:18, Masami Hiramatsu
> <masami.hiramatsu at linaro.org> wrote:
> >
> > Hi Sughosh,
> >
> > 2022年2月9日(水) 18:03 Sughosh Ganu <sughosh.ganu at linaro.org>:
> > >
> > > hi Masami,
> > >
> > > On Wed, 9 Feb 2022 at 10:26, Masami Hiramatsu
> > > <masami.hiramatsu at linaro.org> wrote:
> > > >
> > > > Hi Sughosh,
> > > >
> > > > While porting mdata-sf driver, I'm confusing on the devicetree
> > > > usage.
> > > >
> > > > 2022年2月8日(火) 3:21 Sughosh Ganu <sughosh.ganu at linaro.org>:
> > > >
> > > > > +int fwu_get_mdata_device(struct udevice **mdata_dev)
> > > > > +{
> > > > > + u32 phandle;
> > > > > + int ret, size;
> > > > > + struct udevice *dev, *child;
> > > > > + ofnode fwu_mdata_node;
> > > > > + const fdt32_t *phandle_p = NULL;
> > > > > +
> > > > > + fwu_mdata_node = ofnode_path("/fwu-mdata");
> > > > > + if (!ofnode_valid(fwu_mdata_node)) {
> > > > > + log_err("fwu-node not found\n");
> > > > > + return -ENOENT;
> > > > > + }
> > > >
> > > > So this seems to get the udevice from path, but I think fwu-mdata node
> > > > has "u-boot,fwu-mdata" compatible property, in that case probe function
> > > > already gets the node. Why would you search it again by path?
> > >
> > > Okay. Is there some other api that I can use which is faster than the
> > > approach currently taken? If so, do let me know.
> >
> > I thought the ofnode which has "u-boot,fwu-mdata" compatible property
> > is passed to fwu_mdata_gpt_blk_probe(), and that is , doesn't it?
> >
> > If so, as you show in the example,
> > fwu-mdata {
> > compatible = "u-boot,fwu-mdata";
> > fwu-mdata-store = <&sdmmc1>;
> > };
> >
> > you already got the "/fwu-mdata" node. You don't need any API to find
> > that node because you already has that. That is what I meant.
>
> The probe function gets passed the corresponding udevice structure.
> This structure contains node_ member, which should correspond to the
> device node. I will try to use this member instead of searching for
> the node.
Yeah, you can use "dev_ofnode()".
> > > > > +
> > > > > + phandle_p = ofnode_get_property(fwu_mdata_node, "fwu-mdata-store",
> > > > > + &size);
> > > > > + if (!phandle_p) {
> > > > > + log_err("fwu-mdata-store property not found\n");
> > > > > + return -ENOENT;
> > > > > + }
> > > > > +
> > > > > + phandle = fdt32_to_cpu(*phandle_p);
> > > > > +
> > > > > + ret = device_get_global_by_ofnode(ofnode_get_by_phandle(phandle),
> > > > > + &dev);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + ret = -ENODEV;
> > > > > + for (device_find_first_child(dev, &child); child;
> > > > > + device_find_next_child(&child)) {
> > > > > + if (device_get_uclass_id(child) == UCLASS_BLK) {
> > > > > + *mdata_dev = child;
> > > >
> > > > I thought that the blk device node directly passed by the
> > > > "fwu-mdata-store" property. Would we need to search it from
> > > > children nodes?
> > >
> > > What the device tree is pointing to is the device like the mmc, which
> > > is described through a device tree node. The block device is the child
> > > of this device, which is not described in the device tree. The driver
> > > finally needs the block device.
> >
> > Ah, OK. Can't we specify the block device node to "fwu-mdata-store"
> > directly?
>
> I don't think so, since the fwu-mdata-store is pointing to one of the
> nodes in the device tree -- the block device does not show up as node
> on the device tree.
Ah, I got it. Indeed. In my case spi-nor device will be on the devicetree,
but mmc media is not. Sorry about that.
In that case, does fwu-mdata-store refer the controller node?
If there are several blk devices under that (I'm not sure it can be)
mdata must be in the first device (maybe decided by scanning order?).
I think this should be documented for who ports AB-update on
another platform.
> > > > BTW, if we can use the devicetree, can we define the number of banks
> > > > and the number of images can be described too?
> > >
> > > It is much easier to handle if these values are defined through
> > > Kconfig symbols. That way, this gets directly included in the config
> > > file, and can be used directly in the fwu_mdata.h metadata structure.
> >
> > Yes, it is easier to implement. And that means the firmware bank
> > configuration is embedded in the firmware binary (code) instead
> > of the configuration data (devicetree).
> > For the same reason I think dfu_alt_info (or the basement info like
> > device and offsets) should be in the devicetree. Current implementation
> > is very fragile (because dfu_alt_info can be changed by the user) or
> > fixed in the code (like stm32, it generates the dfu_alt_info by the
> > platform driver.)
> >
> > >
> > > > 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.
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
(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.
Thank you,
>
> -sughosh
>
> >
> > That is my impression felt from porting AB update on the DeveloperBox platform.
> >
> > Thank you,
> >
> > >
> > > -sughosh
> > >
> > > >
> > > > Thank you,
> > > >
> > > >
> > > > > + ret = 0;
> > > > > + }
> > > > > + }
> > > > > +
> > > > > + return ret;
> > > > > +}
> > > > > +
> > > > > +static int fwu_mdata_gpt_blk_probe(struct udevice *dev)
> > > > > +{
> > > > > + int ret;
> > > > > + struct udevice *mdata_dev = NULL;
> > > > > +
> > > > > + ret = fwu_get_mdata_device(&mdata_dev);
> > > > > + if (ret)
> > > > > + return ret;
> > > > > +
> > > > > + dev_set_priv(dev, mdata_dev);
> > > > > +
> > > > > + return 0;
> > > > > +}
> > > > > +
> > > > > +static const struct fwu_mdata_ops fwu_gpt_blk_ops = {
> > > > > + .get_image_alt_num = fwu_gpt_get_image_alt_num,
> > > > > + .mdata_check = fwu_gpt_mdata_check,
> > > > > + .get_mdata = fwu_gpt_get_mdata,
> > > > > + .update_mdata = fwu_gpt_update_mdata,
> > > > > +};
> > > > > +
> > > > > +static const struct udevice_id fwu_mdata_ids[] = {
> > > > > + { .compatible = "u-boot,fwu-mdata" },
> > > >
> > > > > + { }
> > > > > +};
> > > > > +
> > > > > +U_BOOT_DRIVER(fwu_mdata_gpt_blk) = {
> > > > > + .name = "fwu-mdata-gpt-blk",
> > > > > + .id = UCLASS_FWU_MDATA,
> > > > > + .of_match = fwu_mdata_ids,
> > > > > + .ops = &fwu_gpt_blk_ops,
> > > > > + .probe = fwu_mdata_gpt_blk_probe,
> > > > > +};
> > > > > diff --git a/include/fwu.h b/include/fwu.h
> > > > > index 5a99c579fc..2c7db2dff9 100644
> > > > > --- a/include/fwu.h
> > > > > +++ b/include/fwu.h
> > > > > @@ -43,6 +43,8 @@ int fwu_get_active_index(u32 *active_idx);
> > > > > int fwu_update_active_index(u32 active_idx);
> > > > > int fwu_get_image_alt_num(efi_guid_t image_type_id, u32 update_bank,
> > > > > int *alt_num);
> > > > > +int fwu_get_mdata_device(struct udevice **mdata_dev);
> > > > > +int fwu_verify_mdata(struct fwu_mdata *mdata, bool pri_part);
> > > > > int fwu_mdata_check(void);
> > > > > int fwu_revert_boot_index(void);
> > > > > int fwu_accept_image(efi_guid_t *img_type_id, u32 bank);
> > > > > --
> > > > > 2.17.1
> > > > >
> > > >
> > > >
> > > > --
> > > > Masami Hiramatsu
> >
> >
> >
> > --
> > Masami Hiramatsu
--
Masami Hiramatsu
More information about the U-Boot
mailing list