Re: [PATCH v4 01/11] FWU: Add FWU metadata structure and driver for accessing metadata

[Michal Simek <michal.simek@xilinx.com>]

Hi,

On 2/9/22 00:39, Masami Hiramatsu wrote:
> Hi Michal,
> 
> 2022年2月8日(火) 23:27 Michal Simek <michal.simek@xilinx.com>:
>>
>>
>>
>> On 2/8/22 15:14, Masami Hiramatsu wrote:
>>> 2022年2月8日(火) 22:45 Michal Simek <michal.simek@xilinx.com>:
>>>>
>>>>
>>>>
>>>> On 2/8/22 14:36, Masami Hiramatsu wrote:
>>>>> 2022年2月8日(火) 20:35 Sughosh Ganu <sughosh.ganu@linaro.org>:
>>>>>>
>>>>>> On Tue, 8 Feb 2022 at 16:26, Michal Simek <monstr@monstr.eu> wrote:
>>>>>>>
>>>>>>> po 7. 2. 2022 v 19:21 odesílatel Sughosh Ganu <sughosh.ganu@linaro.org> napsal:
>>>>>>>>
>>>>>>>> In the FWU Multi Bank Update feature, the information about the
>>>>>>>> updatable images is stored as part of the metadata, which is stored on
>>>>>>>> a dedicated partition. Add the metadata structure, and a driver model
>>>>>>>> uclass which provides functions to access the metadata. These are
>>>>>>>> generic API's, and implementations can be added based on parameters
>>>>>>>> like how the metadata partition is accessed and what type of storage
>>>>>>>> device houses the metadata.
>>>>>>>>
>>>>>>>> A device tree node fwu-mdata has been added, which is used for
>>>>>>>> pointing to the storage device which contains the FWU metadata. The
>>>>>>>> fwu-mdata node is u-boot specific, and can be added the platform's
>>>>>>>> u-boot dtsi file.
>>>>>>>>
>>>>>>>> Signed-off-by: Sughosh Ganu <sughosh.ganu@linaro.org>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Changes since V3:
>>>>>>>> * Move the FWU metadata access to driver model
>>>>>>>> * Get the storage device containing the metadata from a device tree
>>>>>>>>      property instead of a platform helper function
>>>>>>>>
>>>>>>>>     arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi      |   7 +
>>>>>>>>     .../firmware/fwu-mdata.txt                    |  18 +
>>>>>>>>     drivers/Kconfig                               |   2 +
>>>>>>>>     drivers/Makefile                              |   1 +
>>>>>>>>     drivers/fwu-mdata/Kconfig                     |   7 +
>>>>>>>>     drivers/fwu-mdata/Makefile                    |   6 +
>>>>>>>>     drivers/fwu-mdata/fwu-mdata-uclass.c          | 434 ++++++++++++++++++
>>>>>>>>     include/dm/uclass-id.h                        |   1 +
>>>>>>>>     include/fwu.h                                 |  51 ++
>>>>>>>>     include/fwu_mdata.h                           |  67 +++
>>>>>>>>     10 files changed, 594 insertions(+)
>>>>>>>>     create mode 100644 doc/device-tree-bindings/firmware/fwu-mdata.txt
>>>>>>>>     create mode 100644 drivers/fwu-mdata/Kconfig
>>>>>>>>     create mode 100644 drivers/fwu-mdata/Makefile
>>>>>>>>     create mode 100644 drivers/fwu-mdata/fwu-mdata-uclass.c
>>>>>>>>     create mode 100644 include/fwu.h
>>>>>>>>     create mode 100644 include/fwu_mdata.h
>>>>>>>>
>>>>>>>> diff --git a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi
>>>>>>>> index 06ef3a4095..3bec6107f7 100644
>>>>>>>> --- a/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi
>>>>>>>> +++ b/arch/arm/dts/stm32mp157c-dk2-u-boot.dtsi
>>>>>>>> @@ -4,3 +4,10 @@
>>>>>>>>      */
>>>>>>>>
>>>>>>>>     #include "stm32mp157a-dk1-u-boot.dtsi"
>>>>>>>> +
>>>>>>>> +/ {
>>>>>>>> +       fwu-mdata {
>>>>>>>> +               compatible = "u-boot,fwu-mdata";
>>>>>>>> +               fwu-mdata-store = <&sdmmc1>;
>>>>>>>> +       };
>>>>>>>> +};
>>>>>>>> diff --git a/doc/device-tree-bindings/firmware/fwu-mdata.txt b/doc/device-tree-bindings/firmware/fwu-mdata.txt
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..c766b595ef
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/doc/device-tree-bindings/firmware/fwu-mdata.txt
>>>>>>>> @@ -0,0 +1,18 @@
>>>>>>>> +FWU Metadata Access Devicetree Binding
>>>>>>>> +
>>>>>>>> +The FWU Multi Bank Update feature uses a metadata structure, stored on
>>>>>>>> +a separate partition for keeping information on the set of updatable
>>>>>>>> +images. The device tree node provides information on the storage
>>>>>>>> +device that contains the FWU metadata.
>>>>>>>> +
>>>>>>>> +Required properties :
>>>>>>>> +
>>>>>>>> +- compatible : "u-boot,fwu-mdata";
>>>>>>>> +- fwu-mdata-store : should point to the storage device which contains
>>>>>>>> +                   the FWU metadata partition.
>>>>>>>
>>>>>>> In 0/11 you are describing GPT partitions but I don't think this
>>>>>>> binding is generic enough to describe
>>>>>>> other cases. It is saying device but not where exactly it is.
>>>>>>> I didn't read the whole series yet but arm spec recommends GPT but dt
>>>>>>> binding should be generic enough to describe
>>>>>>> also other cases.
>>>>>>> We are saving this structure to qspi for example but current binding
>>>>>>> can't be used for it.
>>>>>>
>>>>>> I would wait to see Masami's implementation of this feature. If I am
>>>>>> not wrong, his platform too is enabling this feature with a spi as the
>>>>>> storage device. Maybe we can instead put a list of <device partition>
>>>>>> tuples which can then list the device and partition number of the
>>>>>> metadata partitions.
>>>>>
>>>>> Yes, I would like to define new compatible for sf backend, e.g.
>>>>> "u-boot,fwu-mdata-sf".
>>>>
>>>> backend is fine. What does this compatible string have to do with it?
>>>> I didn't see any fwu-mdata-gpt in this series.
>>>
>>> Sughosh made it "fwu-mdata" but I think it should be "fwu-mdata-gpt"
>>> if the required parameters are different.
>>>
>>>>
>>>>> It will have the fwu-mdata-store to point the SPI flash device, and
>>>>> will have a list of offset information for the metadata.
>>>>
>>>> Can you describe it?
>>>>
>>>> Something like for raw accesses?
>>>> fwu-mdta-store = <&qspi 0 XXXX>;
>>>
>>> I'm still not sure what is the best way. What I thought was,
>>>
>>> fwu-mdata-store = <&spi-flash>;
>>> fwu-mdata-offsets = <500000, 520000>;
>>
>> as I said. Before this is applied I think we should work on generic proposal how
>> to describe it. Because the same way can be used for u-boot variables and maybe
>> others.
> 
> Agreed. This also reminds me the dfu_alt_info. Currently the dfu_alt_info
> is passed via u-boot variables, but this should be a (important) part of
> firmware information. I think it should be defined in the devicetree too.
> (actually, stm32 builds dfu_alt_info from the device information already)

yes that capsule update via dfu_alt_info and upgrading all that structure should 
be also the part of it. I am not saying in this series. On the top of this one 
should be fine but it should be clear how this is supposed to work.


>> Definitely I would prefer to ask Rob for helping with this and get this to yaml
>> to be able to use the whole infrastructure to check it.
> 
> Yeah, and at first we should define what information should be in the
> devicetree.

yes.

Thanks,
Michal