| Message ID | 20220207234212.685316-7-sean.anderson@seco.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Simon Glass |
| Headers | show |
| Series | Add support for NVMEM API | expand |
Hi Sean, On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@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@700000 { > #address-cells = <1>; > #size-cells = <1>; > reg = <0x00700000 0x100000>; > > /* ... */ > > tsens_calibration: calib@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@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. Regards, Simon
On 2/26/22 1:36 PM, Simon Glass wrote: > Hi Sean, > > On Mon, 7 Feb 2022 at 16:42, Sean Anderson <sean.anderson@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@700000 { >> #address-cells = <1>; >> #size-cells = <1>; >> reg = <0x00700000 0x100000>; >> >> /* ... */ >> >> tsens_calibration: calib@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@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. Now, there are some instances where creating a new child device might be the best approach. For example, you might have some OTP memory in an unrelated device. In that case, creating a child device (of UCLASS_MISC, or UCLASS_FUSE if that gets created) is the right course of action. --Sean
Hi Sean, On Mon, 28 Feb 2022 at 09:43, Sean Anderson <sean.anderson@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@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@700000 { > >> #address-cells = <1>; > >> #size-cells = <1>; > >> reg = <0x00700000 0x100000>; > >> > >> /* ... */ > >> > >> tsens_calibration: calib@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@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? > > Now, there are some instances where creating a new child device might be the > best approach. For example, you might have some OTP memory in an unrelated > device. In that case, creating a child device (of UCLASS_MISC, or UCLASS_FUSE > if that gets created) is the right course of action. Regards, SImon
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@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@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@700000 { >> >> #address-cells = <1>; >> >> #size-cells = <1>; >> >> reg = <0x00700000 0x100000>; >> >> >> >> /* ... */ >> >> >> >> tsens_calibration: calib@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@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. --Sean
Hi Sean, On Thu, 3 Mar 2022 at 10:45, Sean Anderson <sean.anderson@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@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@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@700000 { > >> >> #address-cells = <1>; > >> >> #size-cells = <1>; > >> >> reg = <0x00700000 0x100000>; > >> >> > >> >> /* ... */ > >> >> > >> >> tsens_calibration: calib@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@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
On 3/11/22 9:25 PM, Simon Glass wrote: > Hi Sean, > > On Thu, 3 Mar 2022 at 10:45, Sean Anderson <sean.anderson@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@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@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@700000 { >> >> >> #address-cells = <1>; >> >> >> #size-cells = <1>; >> >> >> reg = <0x00700000 0x100000>; >> >> >> >> >> >> /* ... */ >> >> >> >> >> >> tsens_calibration: calib@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@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
diff --git a/MAINTAINERS b/MAINTAINERS index dcdd99e368..69de781cdc 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1007,6 +1007,13 @@ F: cmd/nvme.c F: include/nvme.h F: doc/develop/driver-model/nvme.rst +NVMEM +M: Sean Anderson <seanga2@gmail.com> +S: Maintained +F: doc/api/nvmem.rst +F: drivers/misc/nvmem.c +F: include/nvmem.h + NXP C45 TJA11XX PHY DRIVER M: Radu Pirea <radu-nicolae.pirea@oss.nxp.com> S: Maintained diff --git a/doc/api/index.rst b/doc/api/index.rst index 3f36174167..b92de3cf46 100644 --- a/doc/api/index.rst +++ b/doc/api/index.rst @@ -13,6 +13,7 @@ U-Boot API documentation linker_lists lmb logging + nvmem pinctrl rng sandbox diff --git a/doc/api/nvmem.rst b/doc/api/nvmem.rst new file mode 100644 index 0000000000..15c9b5b839 --- /dev/null +++ b/doc/api/nvmem.rst @@ -0,0 +1,7 @@ +.. SPDX-License-Identifier: GPL-2.0+ + +NVMEM API +========= + +.. kernel-doc:: include/nvmem.h + :internal: diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig index a8baaeaf5c..a89e457223 100644 --- a/drivers/misc/Kconfig +++ b/drivers/misc/Kconfig @@ -31,6 +31,22 @@ config TPL_MISC set of generic read, write and ioctl methods may be used to access the device. +config NVMEM + bool "NVMEM support" + help + This adds support for a common interface to different types of + non-volatile memory. Consumers can use nvmem-cells properties to look + up hardware configuration data such as MAC addresses and calibration + settings. + +config SPL_NVMEM + bool "NVMEM support in SPL" + help + This adds support for a common interface to different types of + non-volatile memory. Consumers can use nvmem-cells properties to look + up hardware configuration data such as MAC addresses and calibration + settings. + config ALTERA_SYSID bool "Altera Sysid support" depends on MISC diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile index f9826d2462..fd88ce4181 100644 --- a/drivers/misc/Makefile +++ b/drivers/misc/Makefile @@ -4,6 +4,7 @@ # Wolfgang Denk, DENX Software Engineering, wd@denx.de. obj-$(CONFIG_MISC) += misc-uclass.o +obj-$(CONFIG_$(SPL_TPL_)NVMEM) += nvmem.o obj-$(CONFIG_$(SPL_TPL_)CROS_EC) += cros_ec.o obj-$(CONFIG_$(SPL_TPL_)CROS_EC_SANDBOX) += cros_ec_sandbox.o diff --git a/drivers/misc/nvmem.c b/drivers/misc/nvmem.c new file mode 100644 index 0000000000..e08ae00a87 --- /dev/null +++ b/drivers/misc/nvmem.c @@ -0,0 +1,109 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2022 Sean Anderson <sean.anderson@seco.com> + */ + +#include <common.h> +#include <linker_lists.h> +#include <nvmem.h> +#include <dm/device_compat.h> +#include <dm/ofnode.h> +#include <dm/read.h> +#include <dm/uclass.h> + +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; + + return cell->read(cell->nvmem, cell->offset, buf, size); +} + +int nvmem_cell_write(struct nvmem_cell *cell, const 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; + + return cell->write(cell->nvmem, cell->offset, buf, size); +} + +/** + * nvmem_get_device() - Get an nvmem device for a cell + * @node: ofnode of the nvmem device + * @cell: Cell to look up + * + * Try to find a nvmem-compatible device by going through the nvmem interfaces. + * + * Return: + * * 0 on success + * * -ENODEV if we didn't find anything + * * A negative error if there was a problem looking up the device + */ +static int nvmem_get_device(ofnode node, struct nvmem_cell *cell) +{ + int ret; + struct nvmem_interface *iface; + + for (iface = ll_entry_start(struct nvmem_interface, nvmem_interface); + iface != ll_entry_end(struct nvmem_interface, nvmem_interface); + iface++) { + ret = uclass_get_device_by_ofnode(iface->id, node, + &cell->nvmem); + if (!ret) { + cell->read = iface->read; + cell->write = iface->write; + return 0; + } else if (ret != -ENODEV) { + return ret; + } + } + + return -ENODEV; +} + +int nvmem_cell_get_by_index(struct udevice *dev, int index, + struct nvmem_cell *cell) +{ + fdt_addr_t offset; + fdt_size_t size = FDT_SIZE_T_NONE; + int ret; + struct ofnode_phandle_args args; + + dev_dbg(dev, "%s: index=%d\n", __func__, index); + + ret = dev_read_phandle_with_args(dev, "nvmem-cells", NULL, 0, index, + &args); + if (ret) + return ret; + + ret = nvmem_get_device(ofnode_get_parent(args.node), cell); + if (ret) + return ret; + + offset = ofnode_get_addr_size_index_notrans(args.node, 0, &size); + if (offset == FDT_ADDR_T_NONE || size == FDT_SIZE_T_NONE) { + dev_dbg(cell->nvmem, "missing address or size for %s\n", + ofnode_get_name(args.node)); + return -EINVAL; + } + + cell->offset = offset; + cell->size = size; + return 0; +} + +int nvmem_cell_get_by_name(struct udevice *dev, const char *name, + struct nvmem_cell *cell) +{ + int index; + + dev_dbg(dev, "%s, name=%s\n", __func__, name); + + index = dev_read_stringlist_search(dev, "nvmem-cell-names", name); + if (index < 0) + return index; + + return nvmem_cell_get_by_index(dev, index, cell); +} diff --git a/include/nvmem.h b/include/nvmem.h new file mode 100644 index 0000000000..d9416979b4 --- /dev/null +++ b/include/nvmem.h @@ -0,0 +1,185 @@ +/* SPDX-License-Identifier: GPL-2.0+ */ +/* + * Copyright (c) 2022 Sean Anderson <sean.anderson@seco.com> + */ + +#ifndef NVMEM_H +#define NVMEM_H + +/** + * typedef nvmem_reg_read_t - Read a register from an nvmem device + * + * @dev: The device to read from + * @offset: The offset of the register from the beginning of @dev + * @buf: The buffer to read into + * @size: The size of @buf, in bytes + * + * Return: + * * 0 on success + * * A negative error on failure + */ +typedef int (*nvmem_reg_read_t)(struct udevice *dev, unsigned int offset, + void *buf, size_t size); + +/** + * typedef nvmem_reg_write_t - Write a register to an nvmem device + * @dev: The device to write + * @offset: The offset of the register from the beginning of @dev + * @buf: The buffer to write + * @size: The size of @buf, in bytes + * + * Return: + * * 0 on success + * * -ENOSYS if the device is read-only + * * A negative error on other failures + */ +typedef int (*nvmem_reg_write_t)(struct udevice *dev, unsigned int offset, + const void *buf, size_t size); + +/** + * struct nvmem_cell - One datum within non-volatile memory + * @nvmem: The backing storage device + * @read: The function used to read data + * @write: The function used to write data + * @offset: The offset of the cell from the start of @nvmem + * @size: The size of the cell, in bytes + */ +struct nvmem_cell { + struct udevice *nvmem; + nvmem_reg_read_t read; + nvmem_reg_write_t write; + unsigned int offset; + size_t size; +}; + +struct udevice; + +#if CONFIG_IS_ENABLED(NVMEM) + +/** + * nvmem_cell_read() - Read the value of an nvmem cell + * @cell: The nvmem cell to read + * @buf: The buffer to read into + * @size: The size of @buf + * + * Return: + * * 0 on success + * * -EINVAL if @buf is not the same size as @cell. + * * -ENOSYS if CONFIG_NVMEM is disabled + * * A negative error if there was a problem reading the underlying storage + */ +int nvmem_cell_read(struct nvmem_cell *cell, void *buf, size_t size); + +/** + * nvmem_cell_write() - Write a value to an nvmem cell + * @cell: The nvmem cell to write + * @buf: The buffer to write from + * @size: The size of @buf + * + * Return: + * * 0 on success + * * -EINVAL if @buf is not the same size as @cell + * * -ENOSYS if @cell is read-only, or if CONFIG_NVMEM is disabled + * * A negative error if there was a problem writing the underlying storage + */ +int nvmem_cell_write(struct nvmem_cell *cell, const void *buf, size_t size); + +/** + * nvmem_cell_get_by_index() - Get an nvmem cell from a given device and index + * @dev: The device that uses the nvmem cell + * @index: The index of the cell in nvmem-cells + * @cell: The cell to initialize + * + * Look up the nvmem cell referenced by the phandle at @index in nvmem-cells in + * @dev. + * + * Return: + * * 0 on success + * * -EINVAL if the regs property is missing, empty, or undersized + * * -ENODEV if the nvmem device is missing or unimplemented + * * -ENOSYS if CONFIG_NVMEM is disabled + * * A negative error if there was a problem reading nvmem-cells or getting the + * device + */ +int nvmem_cell_get_by_index(struct udevice *dev, int index, + struct nvmem_cell *cell); + +/** + * nvmem_cell_get_by_name() - Get an nvmem cell from a given device and name + * @dev: The device that uses the nvmem cell + * @name: The name of the nvmem cell + * @cell: The cell to initialize + * + * Look up the nvmem cell referenced by @name in the nvmem-cell-names property + * of @dev. + * + * Return: + * * 0 on success + * * -EINVAL if the regs property is missing, empty, or undersized + * * -ENODEV if the nvmem device is missing or unimplemented + * * -ENODATA if @name is not in nvmem-cell-names + * * -ENOSYS if CONFIG_NVMEM is disabled + * * A negative error if there was a problem reading nvmem-cell-names, + * nvmem-cells, or getting the device + */ +int nvmem_cell_get_by_name(struct udevice *dev, const char *name, + struct nvmem_cell *cell); + +#else /* CONFIG_NVMEM */ + +static inline int nvmem_cell_read(struct nvmem_cell *cell, void *buf, int size) +{ + return -ENOSYS; +} + +static inline int nvmem_cell_write(struct nvmem_cell *cell, const void *buf, + int size) +{ + return -ENOSYS; +} + +static inline int nvmem_cell_get_by_index(struct udevice *dev, int index, + struct nvmem_cell *cell) +{ + return -ENOSYS; +} + +static inline int nvmem_cell_get_by_name(struct udevice *dev, const char *name, + struct nvmem_cell *cell) +{ + return -ENOSYS; +} + +#endif /* CONFIG_NVMEM */ + +/** + * struct nvmem_interface - nvmem interface for a uclass + * @id: The uclass id for this interface + * @read: The function to use to read devices of uclass @id + * @write: The function to use to write devices of uclass @id + * + * This structure specifies the read and write nvmem functions for a given + * uclass. Drivers of uclasses implementing this interface will be elligible to + * have nvmem cell child nodes. Those nodes will specify the offset and size + * which will be passed to @read and @write. + * + * @read and @write *must* be implemented. If all devices in the uclass are + * read-only, then a dummy @write should be provided which just returns + * -ENOSYS. + */ +struct nvmem_interface { + enum uclass_id id; + nvmem_reg_read_t read; + nvmem_reg_write_t write; +}; + +#if CONFIG_IS_ENABLED(NVMEM) +/* Declare a new nvmem_interface */ +#define NVMEM_INTERFACE(__name) \ + ll_entry_declare(struct nvmem_interface, __name, nvmem_interface) +#else +#define NVMEM_INTERFACE(__name) \ + static __always_unused struct nvmem_interface nvmem_interface_##__name +#endif + +#endif /* NVMEM_H */
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@700000 { #address-cells = <1>; #size-cells = <1>; reg = <0x00700000 0x100000>; /* ... */ tsens_calibration: calib@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@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