2022年2月10日(木) 10:43 Masami Hiramatsu <masami.hirama...@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. 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>; }; 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@0 { partitions { compatible = "fixed-partitions"; ... uuid = "aaaaaaaa-bbbb-cccc-dddd-eeeeffffgggg"; ... partition@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. What would you think? Thank you, > > 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 -- Masami Hiramatsu