| Message ID | 20220207182001.31270-3-sughosh.ganu@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | FWU: Add support for FWU Multi Bank Update feature | expand |
Hi Sughosh, While porting mdata-sf driver, I'm confusing on the devicetree usage. 2022年2月8日(火) 3:21 Sughosh Ganu <sughosh.ganu@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? > + > + 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? BTW, if we can use the devicetree, can we define the number of banks and the number of images can be described too? 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) 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
hi Masami, On Wed, 9 Feb 2022 at 10:26, Masami Hiramatsu <masami.hiramatsu@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@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. > > > + > > + 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. > > 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. > 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. -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
Hi Sughosh, 2022年2月9日(水) 18:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > hi Masami, > > On Wed, 9 Feb 2022 at 10:26, Masami Hiramatsu > <masami.hiramatsu@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@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. > > > + > > > + 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? > > 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. 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
hi Masami, On Wed, 9 Feb 2022 at 17:18, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > Hi Sughosh, > > 2022年2月9日(水) 18:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > hi Masami, > > > > On Wed, 9 Feb 2022 at 10:26, Masami Hiramatsu > > <masami.hiramatsu@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@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. > > > > > + > > > > + 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. > > > > 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. 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. 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. -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
Hi Sughosh, 2022年2月10日(木) 3:40 Sughosh Ganu <sughosh.ganu@linaro.org>: > > hi Masami, > > On Wed, 9 Feb 2022 at 17:18, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > Hi Sughosh, > > > > 2022年2月9日(水) 18:03 Sughosh Ganu <sughosh.ganu@linaro.org>: > > > > > > hi Masami, > > > > > > On Wed, 9 Feb 2022 at 10:26, Masami Hiramatsu > > > <masami.hiramatsu@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@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
2022年2月10日(木) 10:43 Masami Hiramatsu <masami.hiramatsu@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
hi Masami, On Thu, 10 Feb 2022 at 08:45, Masami Hiramatsu <masami.hiramatsu@linaro.org> wrote: > > 2022年2月10日(木) 10:43 Masami Hiramatsu <masami.hiramatsu@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. > > 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. 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. > > 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. 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@600000) standard, and are defined for all platforms, for all type of devices(like NOR, Nand). If so, this scheme will work. -sughosh > > 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
Hi Sughosh, 2022年2月10日(木) 21:25 Sughosh Ganu <sughosh.ganu@linaro.org>: > > hi Masami, > > On Thu, 10 Feb 2022 at 08:45, Masami Hiramatsu > <masami.hiramatsu@linaro.org> wrote: > > > > 2022年2月10日(木) 10:43 Masami Hiramatsu <masami.hiramatsu@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@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. > > 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@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
diff --git a/drivers/fwu-mdata/Kconfig b/drivers/fwu-mdata/Kconfig index d6a21c8e19..d5edef19d6 100644 --- a/drivers/fwu-mdata/Kconfig +++ b/drivers/fwu-mdata/Kconfig @@ -5,3 +5,12 @@ config DM_FWU_MDATA Enable support for accessing FWU Metadata partitions. The FWU Metadata partitions reside on the same storage device which contains the other FWU updatable firmware images. + +config FWU_MDATA_GPT_BLK + bool "FWU Metadata access for GPT partitioned Block devices" + select PARTITION_TYPE_GUID + select PARTITION_UUIDS + depends on DM && HAVE_BLOCK_DEVICE && EFI_PARTITION + help + Enable support for accessing FWU Metadata on GPT partitioned + block devices. diff --git a/drivers/fwu-mdata/Makefile b/drivers/fwu-mdata/Makefile index 7fec7171f4..12a5b4fe04 100644 --- a/drivers/fwu-mdata/Makefile +++ b/drivers/fwu-mdata/Makefile @@ -4,3 +4,4 @@ # obj-$(CONFIG_DM_FWU_MDATA) += fwu-mdata-uclass.o +obj-$(CONFIG_FWU_MDATA_GPT_BLK) += fwu_mdata_gpt_blk.o diff --git a/drivers/fwu-mdata/fwu_mdata_gpt_blk.c b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c new file mode 100644 index 0000000000..b3e0fcafb2 --- /dev/null +++ b/drivers/fwu-mdata/fwu_mdata_gpt_blk.c @@ -0,0 +1,501 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (c) 2022, Linaro Limited + */ + +#include <blk.h> +#include <dm.h> +#include <efi_loader.h> +#include <fwu.h> +#include <fwu_mdata.h> +#include <log.h> +#include <malloc.h> +#include <memalign.h> +#include <part.h> +#include <part_efi.h> + +#include <dm/device-internal.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <u-boot/crc.h> + +#define PRIMARY_PART BIT(0) +#define SECONDARY_PART BIT(1) +#define BOTH_PARTS (PRIMARY_PART | SECONDARY_PART) + +#define MDATA_READ BIT(0) +#define MDATA_WRITE BIT(1) + +static int gpt_get_mdata_partitions(struct blk_desc *desc, + u16 *primary_mpart, + u16 *secondary_mpart) +{ + int i, ret; + u32 mdata_parts; + efi_guid_t part_type_guid; + struct disk_partition info; + const efi_guid_t fwu_mdata_guid = FWU_MDATA_GUID; + + mdata_parts = 0; + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { + if (part_get_info(desc, i, &info)) + continue; + uuid_str_to_bin(info.type_guid, part_type_guid.b, + UUID_STR_FORMAT_GUID); + + if (!guidcmp(&fwu_mdata_guid, &part_type_guid)) { + ++mdata_parts; + if (!*primary_mpart) + *primary_mpart = i; + else + *secondary_mpart = i; + } + } + + if (mdata_parts != 2) { + log_err("Expect two copies of the FWU metadata instead of %d\n", + mdata_parts); + ret = -EINVAL; + } else { + ret = 0; + } + + return ret; +} + +static int gpt_get_mdata_disk_part(struct blk_desc *desc, + struct disk_partition *info, + u32 part_num) +{ + int ret; + char *mdata_guid_str = "8a7a84a0-8387-40f6-ab41-a8b9a5a60d23"; + + ret = part_get_info(desc, part_num, info); + if (ret < 0) { + log_err("Unable to get the partition info for the FWU metadata part %d", + part_num); + return -1; + } + + /* Check that it is indeed the FWU metadata partition */ + if (!strncmp(info->type_guid, mdata_guid_str, UUID_STR_LEN)) { + /* Found the FWU metadata partition */ + return 0; + } + + return -1; +} + +static int gpt_read_write_mdata(struct blk_desc *desc, + struct fwu_mdata *mdata, + u8 access, u32 part_num) +{ + int ret; + u32 len, blk_start, blkcnt; + struct disk_partition info; + + ALLOC_CACHE_ALIGN_BUFFER_PAD(struct fwu_mdata, mdata_aligned, 1, + desc->blksz); + + ret = gpt_get_mdata_disk_part(desc, &info, part_num); + if (ret < 0) { + printf("Unable to get the FWU metadata partition\n"); + return -ENODEV; + } + + len = sizeof(*mdata); + blkcnt = BLOCK_CNT(len, desc); + if (blkcnt > info.size) { + log_err("Block count exceeds FWU metadata partition size\n"); + return -ERANGE; + } + + blk_start = info.start; + if (access == MDATA_READ) { + if (blk_dread(desc, blk_start, blkcnt, mdata_aligned) != blkcnt) { + log_err("Error reading FWU metadata from the device\n"); + return -EIO; + } + memcpy(mdata, mdata_aligned, sizeof(struct fwu_mdata)); + } else { + if (blk_dwrite(desc, blk_start, blkcnt, mdata) != blkcnt) { + log_err("Error writing FWU metadata to the device\n"); + return -EIO; + } + } + + return 0; +} + +static int gpt_read_mdata(struct blk_desc *desc, + struct fwu_mdata *mdata, u32 part_num) +{ + return gpt_read_write_mdata(desc, mdata, MDATA_READ, part_num); +} + +static int gpt_write_mdata_partition(struct blk_desc *desc, + struct fwu_mdata *mdata, + u32 part_num) +{ + return gpt_read_write_mdata(desc, mdata, MDATA_WRITE, part_num); +} + +static int fwu_gpt_update_mdata(struct udevice * dev, struct fwu_mdata *mdata) +{ + int ret; + struct blk_desc *desc; + u16 primary_mpart = 0, secondary_mpart = 0; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + /* First write the primary partition*/ + ret = gpt_write_mdata_partition(desc, mdata, primary_mpart); + if (ret < 0) { + log_err("Updating primary FWU metadata partition failed\n"); + return ret; + } + + /* And now the replica */ + ret = gpt_write_mdata_partition(desc, mdata, secondary_mpart); + if (ret < 0) { + log_err("Updating secondary FWU metadata partition failed\n"); + return ret; + } + + return 0; +} + +static int gpt_get_mdata(struct blk_desc *desc, struct fwu_mdata **mdata) +{ + int ret; + u16 primary_mpart = 0, secondary_mpart = 0; + + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + *mdata = malloc(sizeof(struct fwu_mdata)); + if (!*mdata) { + log_err("Unable to allocate memory for reading FWU metadata\n"); + return -ENOMEM; + } + + ret = gpt_read_mdata(desc, *mdata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + return -EIO; + } + + ret = fwu_verify_mdata(*mdata, 1); + if (!ret) + return 0; + + /* + * Verification of the primary FWU metadata copy failed. + * Try to read the replica. + */ + memset(*mdata, 0, sizeof(struct fwu_mdata)); + ret = gpt_read_mdata(desc, *mdata, secondary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + return -EIO; + } + + ret = fwu_verify_mdata(*mdata, 0); + if (!ret) + return 0; + + /* Both the FWU metadata copies are corrupted. */ + return -1; +} + +static int gpt_check_mdata_validity(struct udevice *dev) +{ + int ret; + struct blk_desc *desc; + struct fwu_mdata pri_mdata; + struct fwu_mdata secondary_mdata; + u16 primary_mpart = 0, secondary_mpart = 0; + u16 valid_partitions, invalid_partitions; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + /* + * Two FWU metadata partitions are expected. + * If we don't have two, user needs to create + * them first + */ + valid_partitions = 0; + ret = gpt_get_mdata_partitions(desc, &primary_mpart, + &secondary_mpart); + + if (ret < 0) { + log_err("Error getting the FWU metadata partitions\n"); + return -ENODEV; + } + + ret = gpt_read_mdata(desc, &pri_mdata, primary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + goto secondary_read; + } + + ret = fwu_verify_mdata(&pri_mdata, 1); + if (!ret) + valid_partitions |= PRIMARY_PART; + +secondary_read: + /* Now check the secondary partition */ + ret = gpt_read_mdata(desc, &secondary_mdata, secondary_mpart); + if (ret < 0) { + log_err("Failed to read the FWU metadata from the device\n"); + goto mdata_restore; + } + + ret = fwu_verify_mdata(&secondary_mdata, 0); + if (!ret) + valid_partitions |= SECONDARY_PART; + +mdata_restore: + if (valid_partitions == (PRIMARY_PART | SECONDARY_PART)) { + ret = -1; + /* + * Before returning, check that both the + * FWU metadata copies are the same. If not, + * the FWU metadata copies need to be + * re-populated. + */ + if (!memcmp(&pri_mdata, &secondary_mdata, + sizeof(struct fwu_mdata))) { + ret = 0; + } else { + log_err("Both FWU metadata copies are valid but do not match. Please check!\n"); + } + goto out; + } + + ret = -1; + if (!(valid_partitions & BOTH_PARTS)) + goto out; + + invalid_partitions = valid_partitions ^ BOTH_PARTS; + ret = gpt_write_mdata_partition(desc, + (invalid_partitions == PRIMARY_PART) ? + &secondary_mdata : &pri_mdata, + (invalid_partitions == PRIMARY_PART) ? + primary_mpart : secondary_mpart); + + if (ret < 0) + log_err("Restoring %s FWU metadata partition failed\n", + (invalid_partitions == PRIMARY_PART) ? + "primary" : "secondary"); + +out: + return ret; +} + +static int gpt_get_image_alt_num(struct udevice *dev, struct blk_desc *desc, + efi_guid_t image_type_id, u32 update_bank, + int *alt_no) +{ + int ret, i; + u32 part; + struct fwu_mdata *mdata = NULL; + struct fwu_image_entry *img_entry; + struct fwu_image_bank_info *img_bank_info; + struct disk_partition info; + efi_guid_t unique_part_guid; + efi_guid_t image_guid = NULL_GUID; + + ret = gpt_get_mdata(desc, &mdata); + if (ret < 0) { + log_err("Unable to read valid FWU metadata\n"); + goto out; + } + + /* + * The FWU metadata has been read. Now get the image_uuid for the + * image with the update_bank. + */ + for (i = 0; i < CONFIG_FWU_NUM_IMAGES_PER_BANK; i++) { + if (!guidcmp(&image_type_id, + &mdata->img_entry[i].image_type_uuid)) { + img_entry = &mdata->img_entry[i]; + img_bank_info = &img_entry->img_bank_info[update_bank]; + guidcpy(&image_guid, &img_bank_info->image_uuid); + break; + } + } + + /* + * Now read the GPT Partition Table Entries to find a matching + * partition with UniquePartitionGuid value. We need to iterate + * through all the GPT partitions since they might be in any + * order + */ + for (i = 1; i < MAX_SEARCH_PARTITIONS; i++) { + if (part_get_info(desc, i, &info)) + continue; + uuid_str_to_bin(info.uuid, unique_part_guid.b, + UUID_STR_FORMAT_GUID); + + if (!guidcmp(&unique_part_guid, &image_guid)) { + /* Found the partition */ + part = i; + *alt_no = fwu_plat_get_alt_num(dev_get_priv(dev), &part); + if (*alt_no != -1) + log_info("alt_num %d for partition %pUl\n", + *alt_no, &image_guid); + ret = 0; + break; + } + } + + if (*alt_no == -1) { + log_err("alt_num not found for partition with GUID %pUl\n", + &image_guid); + ret = -EINVAL; + } + + if (i == MAX_SEARCH_PARTITIONS) { + log_err("Partition with the image guid not found\n"); + ret = -EINVAL; + } + +out: + free(mdata); + + return ret; +} + +int fwu_gpt_get_image_alt_num(struct udevice *dev, efi_guid_t image_type_id, + u32 update_bank, int *alt_no) +{ + struct blk_desc *desc; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + return gpt_get_image_alt_num(dev, desc, image_type_id, update_bank, + alt_no); +} + +int fwu_gpt_mdata_check(struct udevice *dev) +{ + /* + * Check if both the copies of the FWU metadata are + * valid. If one has gone bad, restore it from the + * other good copy. + */ + return gpt_check_mdata_validity(dev); +} + +int fwu_gpt_get_mdata(struct udevice *dev, struct fwu_mdata **mdata) +{ + struct blk_desc *desc; + + desc = dev_get_uclass_plat(dev_get_priv(dev)); + if (!desc) { + log_err("Block device not found\n"); + return -ENODEV; + } + + return gpt_get_mdata(desc, mdata); +} + +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; + } + + 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; + 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);
In the FWU Multi Bank Update feature, the information about the updatable images is stored as part of the metadata, on a separate partition. Add a driver for reading from and writing to the metadata when the updatable images and the metadata are stored on a block device which is formated with GPT based partition scheme. Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org> --- Changes since V3: * Move the metadata access driver for GPT partitioned block devices under drivers/fwu-mdata/ directory, complying with driver model. * Move functionality to get the active index under the common function instead of the GPT block device specific driver. drivers/fwu-mdata/Kconfig | 9 + drivers/fwu-mdata/Makefile | 1 + drivers/fwu-mdata/fwu_mdata_gpt_blk.c | 501 ++++++++++++++++++++++++++ include/fwu.h | 2 + 4 files changed, 513 insertions(+) create mode 100644 drivers/fwu-mdata/fwu_mdata_gpt_blk.c