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

[Sean Anderson]

On 3/11/22 9:25 PM, Simon Glass wrote:
> 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?

Right. But the idea is to use existing uclasses, because several of
them effectively already implement the interface. See e.g. I2C and
RTC which almost don't need a wrapper (except that they use different
types for the size field).

--Sean