[PATCH 06/14] misc: Add support for nvmem cells

[Simon Glass]

Hi Sean,

On Thu, 3 Mar 2022 at 10:45, Sean Anderson <sean.anderson at seco.com> wrote:
>
> Hi Simon,
>
> On 3/1/22 9:58 AM, Simon Glass wrote:
> > Hi Sean,
> >
> > On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.anderson at seco.com> wrote:
> >>
> >>
> >>
> >> On 2/26/22 1:36 PM, Simon Glass wrote:
> >> > Hi Sean,
> >> >
> >> > On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson at seco.com> wrote:
> >> >>
> >> >> This adds support for "nvmem cells" as seen in Linux. The nvmem device
> >> >> class in Linux is used for various assorted ROMs and EEPROMs. In this
> >> >> sense, it is similar to UCLASS_MISC, but also includes
> >> >> UCLASS_I2C_EEPROM, UCLASS_RTC, and UCLASS_MTD. While nvmem devices can
> >> >> be accessed directly, they are most often used by reading/writing
> >> >> contiguous values called "cells". Cells typically hold information like
> >> >> calibration, versions, or configuration (such as mac addresses).
> >> >>
> >> >> nvmem devices can specify "cells" in their device tree:
> >> >>
> >> >>         qfprom: eeprom at 700000 {
> >> >>                 #address-cells = <1>;
> >> >>                 #size-cells = <1>;
> >> >>                 reg = <0x00700000 0x100000>;
> >> >>
> >> >>                 /* ... */
> >> >>
> >> >>                 tsens_calibration: calib at 404 {
> >> >>                         reg = <0x404 0x10>;
> >> >>                 };
> >> >>         };
> >> >>
> >> >> which can then be referenced like:
> >> >>
> >> >>         tsens {
> >> >>                 /* ... */
> >> >>                 nvmem-cells = <&tsens_calibration>;
> >> >>                 nvmem-cell-names = "calibration";
> >> >>         };
> >> >>
> >> >> The tsens driver could then read the calibration value like:
> >> >>
> >> >>         struct nvmem_cell cal_cell;
> >> >>         u8 cal[16];
> >> >>         nvmem_cell_get_by_name(dev, "calibration", &cal_cell);
> >> >>         nvmem_cell_read(&cal_cell, cal, sizeof(cal));
> >> >>
> >> >> Because nvmem devices are not all of the same uclass, supported uclasses
> >> >> must register a nvmem_interface struct. This allows CONFIG_NVMEM to be
> >> >> enabled without depending on specific uclasses. At the moment,
> >> >> nvmem_interface is very bare-bones, and assumes that no initialization
> >> >> is necessary. However, this could be amended in the future.
> >> >>
> >> >> Although I2C_EEPROM and MISC are quite similar (and could likely be
> >> >> unified), they present different read/write function signatures. To
> >> >> abstract over this, NVMEM uses the same read/write signature as Linux.
> >> >> In particular, short read/writes are not allowed, which is allowed by
> >> >> MISC.
> >> >>
> >> >> The functionality implemented by nvmem cells is very similar to that
> >> >> provided by i2c_eeprom_partition. "fixed-partition"s for eeproms does
> >> >> not seem to have made its way into Linux or into any device tree other
> >> >> than sandbox. It is possible that with the introduction of this API it
> >> >> would be possible to remove it.
> >> >>
> >> >> Signed-off-by: Sean Anderson <sean.anderson at seco.com>
> >> >> ---
> >> >>
> >> >>  MAINTAINERS           |   7 ++
> >> >>  doc/api/index.rst     |   1 +
> >> >>  doc/api/nvmem.rst     |   7 ++
> >> >>  drivers/misc/Kconfig  |  16 ++++
> >> >>  drivers/misc/Makefile |   1 +
> >> >>  drivers/misc/nvmem.c  | 109 +++++++++++++++++++++++++
> >> >>  include/nvmem.h       | 185 ++++++++++++++++++++++++++++++++++++++++++
> >> >>  7 files changed, 326 insertions(+)
> >> >>  create mode 100644 doc/api/nvmem.rst
> >> >>  create mode 100644 drivers/misc/nvmem.c
> >> >>  create mode 100644 include/nvmem.h
> >> >
> >> > Here I think it would be better to add a new uclass so that drivers
> >> > which support it can add a child device in that uclass. This is done
> >> > in lots of places in U-Boot.
> >>
> >> I'm not sure exactly what you have in mind. The issue is that there are at
> >> least 6 uclasses which I would like to support:
> >>
> >> - UCLASS_MISC
> >> - UCLASS_I2C_EEPROM
> >> - UCLASS_RTC
> >> - UCLASS_MTD
> >> - UCLASS_FUSE (doesn't exist yet, but probably should)
> >> - Possibly UCLASS_PMIC
> >>
> >> Most of these uclasses have existing interfaces which expose an NVMEM-like API,
> >> in addition to other uclass-specific functionality. Instead of having an
> >> additional API which drivers must implement, I would like to leverage these
> >> existing APIs to make adding NVMEM support as painless as possible. NVMEM is
> >> more of a "meta-uclass" which allows us to leverage existing read/write
> >> functions in uclasses. If any additional devices are to be created, they need
> >> to be created by the nvmem subsystem, or by the supported uclasses, rather than
> >> in drivers.
> >
> > I may be missing something as I have not looked in detail at your API changes.
> >
> > But the way to have a consistent API is to use a uclass. We do this
> > with BLK. When a PMIC have GPIOs, RTC and regulators, we add them as
> > child devices. We also have it with bootstd, where a bootdev is
> > created as a child device of a storage device. We can put the required
> > stuff in a helper function. We can even avoid any new code in the
> > drivers by using the pending event system.
> >
> > Can you first help me understand what is wrong with using a new uclass?
>
> I suppose it could be done this way.
>
> Effectively, we are "picking" out two functions from the existing API.
> NVMEM is a proper sub-uclass of every uclass added in this series except
> UCLASS_MISC (which just needs some API adjustment). In essence, we could
> actually implement something like nvmem_cell_read as
>
> int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size)
> {
>         dev_dbg(cell->nvmem, "%s: off=%u size=%zu\n", __func__, cell->offset, size);
>         if (size != cell->size)
>                 return -EINVAL;
>
>         switch (cell->nvmem->driver->id) {
>         case UCLASS_I2C_EEPROM:
>                 return i2c_eeprom_read(dev, offset, buf, size)
>         case UCLASS_RTC:
>                 return dm_rtc_read(cell->nvmem, offset, buf, size);
>         case UCLASS_MISC: {
>                 int ret = misc_read(cell->nvmem, offset, buf, size);
>
>                 if (ret < 0)
>                         return ret;
>                 if (ret != size)
>                         return -EIO;
>                 return 0;
>         /* etc */
>         }
>         }
>
>         return -ENOSYS;
> }
>
> Actually, that is probably cleaner than my current approach.

Yes but it is still not using a new uclass, right?

Regards,
Simon