[PATCH v8 02/13] FWU: Add FWU metadata structure and driver for accessing metadata

[Simon Glass]

Hi Sughosh,

On Fri, 19 Aug 2022 at 10:18, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
>
> hi Simon,
>
> On Fri, 19 Aug 2022 at 20:53, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Sughosh,
> >
> > On Fri, 19 Aug 2022 at 07:36, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > >
> > > hi Simon,
> > >
> > > On Thu, 18 Aug 2022 at 23:20, Simon Glass <sjg at chromium.org> wrote:
> > > >
> > > > pHi Sughosh,
> > > >
> > > > On Thu, 18 Aug 2022 at 05:03, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > >
> > > > > hi Simon,
> > > > >
> > > > > On Thu, 18 Aug 2022 at 06:43, Simon Glass <sjg at chromium.org> wrote:
> > > > > >
> > > > > > Hi Sughosh,
> > > > > >
> > > > > > On Wed, 17 Aug 2022 at 06:44, Sughosh Ganu <sughosh.ganu at linaro.org> wrote:
> > > > > > >
> > > > > > > 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.
> > > > > > >
> > > > > > > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > > > > > > Reviewed-by: Patrick Delaunay <patrick.delaunay at foss.st.com>
> > > > > > > ---
> > > > > > > Changes since V7:
> > > > > > > * Rephrased the error message in fwu_update_active_index as per
> > > > > > >   suggestion from Ilias.
> > > > > > > * Reworked the logic in fwu_get_image_alt_num() as per the suggestion
> > > > > > >   from Ilias.
> > > > > > >
> > > > > > >  drivers/Kconfig                      |   2 +
> > > > > > >  drivers/Makefile                     |   1 +
> > > > > > >  drivers/fwu-mdata/Kconfig            |   7 +
> > > > > > >  drivers/fwu-mdata/Makefile           |   6 +
> > > > > > >  drivers/fwu-mdata/fwu-mdata-uclass.c | 463 +++++++++++++++++++++++++++
> > > > > > >  include/dm/uclass-id.h               |   1 +
> > > > > > >  include/fwu.h                        |  49 +++
> > > > > > >  include/fwu_mdata.h                  |  67 ++++
> > > > > > >  8 files changed, 596 insertions(+)
> > > > > > >  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
> > > > > > >
> >
> > [..]
> >
> > > >
> > > > I don't understand your comment about NULL. free(NULL) is valid in U-Boot, if that is what you are wondering?
> > > >
> > > > >
> > > > > >
> > > > > > > +       struct fwu_image_entry *img_entry;
> > > > > > > +       const struct fwu_mdata_ops *ops = NULL;
> > > > > > > +       struct fwu_image_bank_info *img_bank_info;
> > > > > > > +
> > > > > > > +       ret = fwu_get_dev_ops(&dev, &ops);
> > > > > >
> > > > > > Rather than this, see the pattern used by other uclasses
> > > > >
> > > > > Can you point me to some example code. Also, not sure what the issue
> > > > > is with using this function.
> > > >
> > > > It is a pointless obfuscation, mainly. See for example clk_request(), but most uclasses use this pattern. I really encourage you to read more of the code before submitting a patch.
> > > >
> > > > int clk_request(struct udevice *dev, struct clk *clk)
> > > > {
> > > >    const struct clk_ops *ops;
> > > >
> > > >    ...
> > > >    ops = clk_dev_ops(dev);
> > > >
> > > > (where that is defined in the header file)
> > >
> > > If you mean clk_dev_ops, it is defined in clk-uclass.c, and IMO that
> > > makes sense, since it is being called multiple times in the driver.
> > > Similarly, the fwu_get_dev_ops is being called multiple times in the
> > > uclass driver, so I think it makes sense to have it as a function. If
> > > you want me to move it to a header file or rename it, I can do that.
> >
> > Yes, rename and move to header, and use the same function signature as
> > clk and the other uclasses (i.e. no dev parameter).
> >
> > Your uclass functions need to be defined with a device parameter as
> > the first arg. They cannot go and hunt down a uclass themselves. See
> > other uclasses for how this works.
> >
> > If you want a common function to find the first device, use
> > uclass_first_device() in the caller, not internal to the uclass!
>
> I can do that, sure. But there doesn't seem to be uniformity in these
> rules. There are a bunch of uclass drivers which seem to be doing
> exactly the same thing, including, but not limited to bootcount,
> pinctrl, regulators. These uclass drivers all have instances of common

pinctrl has pinctrl_select_state() and two GPIO functions that violate
this. It should be fixed to store the default pinctrl driver in gd,
like we do with serial_dev. Please do send a patch if you can.

regulators have some functions which act on all regulators, or at
least a selection. That is fine, I think, but let me know if you see
somethign wrong.

bootcount is not fully converted to driver model and assumes a single
bootcount. That should be fixed. in that it should work like sysreset,
loading / storing from anything that exists (e.g. the 'walk'
functions).

> API being called and the uclass driver finding the device. Please note
> that the FWU uclass is not representing a real device -- it is pretty
> similar to bootcount in that regard. Which was the primary reason I
> kept the FWU API's device agnostic.

The 'real device' thing doesn't really matter. What is a real device,
anyway? Anything we want to model with drivers, a uclass and methods
is a device. It is also the primary way we call code from the
top-level part of U-Boot.

Regards,
Simon