This is now the third attempt to fetch the MAC addresses from the VPD
for the Kontron sl28 boards. Previous discussions can be found here:
https://lore.kernel.org/lkml/[email protected]/
NVMEM cells are typically added by board code or by the devicetree. But
as the cells get more complex, there is (valid) push back from the
devicetree maintainers to not put that handling in the devicetree.
Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
can add cells during runtime. That way it is possible to add more complex
cells than it is possible right now with the offset/length/bits
description in the device tree. For example, you can have post processing
for individual cells (think of endian swapping, or ethernet offset
handling).
The imx-ocotp driver is the only user of the global post processing hook,
convert it to nvmem layouts and drop the global post pocessing hook.
For now, the layouts are selected by the device tree. But the idea is
that also board files or other drivers could set a layout. Although no
code for that exists yet.
Thanks to Miquel, the device tree bindings are already approved and merged.
NVMEM layouts as modules?
While possible in principle, it doesn't make any sense because the NVMEM
core can't be compiled as a module. The layouts needs to be available at
probe time. (That is also the reason why they get registered with
subsys_initcall().) So if the NVMEM core would be a module, the layouts
could be modules, too.
Michael Walle (19):
net: add helper eth_addr_add()
of: base: add of_parse_phandle_with_optional_args()
of: property: make #.*-cells optional for simple props
of: property: add #nvmem-cell-cells property
nvmem: core: fix device node refcounting
nvmem: core: add an index parameter to the cell
nvmem: core: move struct nvmem_cell_info to nvmem-provider.h
nvmem: core: drop the removal of the cells in nvmem_add_cells()
nvmem: core: fix cell removal on error
nvmem: core: add nvmem_add_one_cell()
nvmem: core: use nvmem_add_one_cell() in nvmem_add_cells_from_of()
nvmem: core: introduce NVMEM layouts
nvmem: core: add per-cell post processing
nvmem: core: allow to modify a cell before adding it
nvmem: imx-ocotp: replace global post processing with layouts
nvmem: cell: drop global cell_post_process
nvmem: core: provide own priv pointer in post process callback
nvmem: layouts: add sl28vpd layout
MAINTAINERS: add myself as sl28vpd nvmem layout driver
Miquel Raynal (2):
nvmem: layouts: Add ONIE tlv layout driver
MAINTAINERS: Add myself as ONIE tlv NVMEM layout maintainer
Documentation/driver-api/nvmem.rst | 15 ++
MAINTAINERS | 12 ++
drivers/nvmem/Kconfig | 4 +
drivers/nvmem/Makefile | 1 +
drivers/nvmem/core.c | 295 +++++++++++++++++++++--------
drivers/nvmem/imx-ocotp.c | 34 ++--
drivers/nvmem/layouts/Kconfig | 23 +++
drivers/nvmem/layouts/Makefile | 7 +
drivers/nvmem/layouts/onie-tlv.c | 244 ++++++++++++++++++++++++
drivers/nvmem/layouts/sl28vpd.c | 153 +++++++++++++++
drivers/of/property.c | 6 +-
include/linux/etherdevice.h | 14 ++
include/linux/nvmem-consumer.h | 17 +-
include/linux/nvmem-provider.h | 95 +++++++++-
include/linux/of.h | 25 +++
15 files changed, 837 insertions(+), 108 deletions(-)
create mode 100644 drivers/nvmem/layouts/Kconfig
create mode 100644 drivers/nvmem/layouts/Makefile
create mode 100644 drivers/nvmem/layouts/onie-tlv.c
create mode 100644 drivers/nvmem/layouts/sl28vpd.c
--
2.30.2
Provide a way to modify a cell before it will get added. This is useful
to attach a custom post processing hook via a layout.
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- none
changes since v3:
- none
changes since v2:
- none
changes since v1:
- new patch
drivers/nvmem/core.c | 4 ++++
include/linux/nvmem-provider.h | 5 +++++
2 files changed, 9 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 5afd4818cb34..2706fc5ed494 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -694,6 +694,7 @@ static int nvmem_validate_keepouts(struct nvmem_device *nvmem)
static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
{
+ struct nvmem_layout *layout = nvmem->layout;
struct device *dev = &nvmem->dev;
struct device_node *child;
const __be32 *addr;
@@ -723,6 +724,9 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
info.np = of_node_get(child);
+ if (layout && layout->fixup_cell_info)
+ layout->fixup_cell_info(nvmem, layout, &info);
+
ret = nvmem_add_one_cell(nvmem, &info);
kfree(info.name);
if (ret) {
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 1930496d8854..bfaba5227ac9 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -157,6 +157,8 @@ struct nvmem_cell_table {
* @add_cells: Will be called if a nvmem device is found which
* has this layout. The function will add layout
* specific cells with nvmem_add_one_cell().
+ * @fixup_cell_info: Will be called before a cell is added. Can be
+ * used to modify the nvmem_cell_info.
* @owner: Pointer to struct module.
* @node: List node.
*
@@ -170,6 +172,9 @@ struct nvmem_layout {
const struct of_device_id *of_match_table;
int (*add_cells)(struct device *dev, struct nvmem_device *nvmem,
struct nvmem_layout *layout);
+ void (*fixup_cell_info)(struct nvmem_device *nvmem,
+ struct nvmem_layout *layout,
+ struct nvmem_cell_info *cell);
/* private */
struct module *owner;
--
2.30.2
From: Miquel Raynal <[email protected]>
This layout applies on top of any non volatile storage device containing
an ONIE table factory flashed. This table follows the tlv
(type-length-value) organization described in the link below. We cannot
afford using regular parsers because the content of these tables is
manufacturer specific and must be dynamically discovered.
Link: https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
Signed-off-by: Miquel Raynal <[email protected]>
---
changes since v4:
- none
changes since v3:
- don't use kfree()
- fix typo, s/no/on/
changes since v2:
- new patch
drivers/nvmem/layouts/Kconfig | 9 ++
drivers/nvmem/layouts/Makefile | 1 +
drivers/nvmem/layouts/onie-tlv.c | 244 +++++++++++++++++++++++++++++++
3 files changed, 254 insertions(+)
create mode 100644 drivers/nvmem/layouts/onie-tlv.c
diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
index 75082f6b471d..9ad50474cb77 100644
--- a/drivers/nvmem/layouts/Kconfig
+++ b/drivers/nvmem/layouts/Kconfig
@@ -11,4 +11,13 @@ config NVMEM_LAYOUT_SL28_VPD
If unsure, say N.
+config NVMEM_LAYOUT_ONIE_TLV
+ bool "ONIE tlv support"
+ select CRC32
+ help
+ Say Y here if you want to support the Open Compute Project ONIE
+ Type-Length-Value standard table.
+
+ If unsure, say N.
+
endmenu
diff --git a/drivers/nvmem/layouts/Makefile b/drivers/nvmem/layouts/Makefile
index fc617b9e87d0..2974bd7d33ed 100644
--- a/drivers/nvmem/layouts/Makefile
+++ b/drivers/nvmem/layouts/Makefile
@@ -4,3 +4,4 @@
#
obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o
+obj-$(CONFIG_NVMEM_LAYOUT_ONIE_TLV) += onie-tlv.o
diff --git a/drivers/nvmem/layouts/onie-tlv.c b/drivers/nvmem/layouts/onie-tlv.c
new file mode 100644
index 000000000000..074c7c700845
--- /dev/null
+++ b/drivers/nvmem/layouts/onie-tlv.c
@@ -0,0 +1,244 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * ONIE tlv NVMEM cells provider
+ *
+ * Copyright (C) 2022 Open Compute Group ONIE
+ * Author: Miquel Raynal <[email protected]>
+ * Based on the nvmem driver written by: Vadym Kochan <[email protected]>
+ * Inspired by the first layout written by: Rafał Miłecki <[email protected]>
+ */
+
+#include <linux/crc32.h>
+#include <linux/etherdevice.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+
+#define ONIE_TLV_MAX_LEN 2048
+#define ONIE_TLV_CRC_FIELD_SZ 6
+#define ONIE_TLV_CRC_SZ 4
+#define ONIE_TLV_HDR_ID "TlvInfo"
+
+struct onie_tlv_hdr {
+ u8 id[8];
+ u8 version;
+ __be16 data_len;
+} __packed;
+
+struct onie_tlv {
+ u8 type;
+ u8 len;
+} __packed;
+
+static const char *onie_tlv_cell_name(u8 type)
+{
+ switch (type) {
+ case 0x21:
+ return "product-name";
+ case 0x22:
+ return "part-number";
+ case 0x23:
+ return "serial-number";
+ case 0x24:
+ return "mac-address";
+ case 0x25:
+ return "manufacture-date";
+ case 0x26:
+ return "device-version";
+ case 0x27:
+ return "label-revision";
+ case 0x28:
+ return "platforn-name";
+ case 0x29:
+ return "onie-version";
+ case 0x2A:
+ return "num-macs";
+ case 0x2B:
+ return "manufacturer";
+ case 0x2C:
+ return "country-code";
+ case 0x2D:
+ return "vendor";
+ case 0x2E:
+ return "diag-version";
+ case 0x2F:
+ return "service-tag";
+ case 0xFD:
+ return "vendor-extension";
+ case 0xFE:
+ return "crc32";
+ default:
+ break;
+ }
+
+ return NULL;
+}
+
+static int onie_tlv_mac_read_cb(void *priv, const char *id, int index,
+ unsigned int offset, void *buf,
+ size_t bytes)
+{
+ eth_addr_add(buf, index);
+
+ return 0;
+}
+
+static nvmem_cell_post_process_t onie_tlv_read_cb(u8 type, u8 *buf)
+{
+ switch (type) {
+ case 0x24:
+ return &onie_tlv_mac_read_cb;
+ default:
+ break;
+ }
+
+ return NULL;
+}
+
+static int onie_tlv_add_cells(struct device *dev, struct nvmem_device *nvmem,
+ size_t data_len, u8 *data)
+{
+ struct nvmem_cell_info cell = {};
+ struct device_node *layout;
+ struct onie_tlv tlv;
+ unsigned int hdr_len = sizeof(struct onie_tlv_hdr);
+ unsigned int offset = 0;
+ int ret;
+
+ layout = of_nvmem_layout_get_container(nvmem);
+ if (!layout)
+ return -ENOENT;
+
+ while (offset < data_len) {
+ memcpy(&tlv, data + offset, sizeof(tlv));
+ if (offset + tlv.len >= data_len) {
+ dev_err(dev, "Out of bounds field (0x%x bytes at 0x%x)\n",
+ tlv.len, hdr_len + offset);
+ break;
+ }
+
+ cell.name = onie_tlv_cell_name(tlv.type);
+ if (!cell.name)
+ continue;
+
+ cell.offset = hdr_len + offset + sizeof(tlv.type) + sizeof(tlv.len);
+ cell.bytes = tlv.len;
+ cell.np = of_get_child_by_name(layout, cell.name);
+ cell.read_post_process = onie_tlv_read_cb(tlv.type, data + offset + sizeof(tlv));
+
+ ret = nvmem_add_one_cell(nvmem, &cell);
+ if (ret) {
+ of_node_put(layout);
+ return ret;
+ }
+
+ offset += sizeof(tlv) + tlv.len;
+ }
+
+ of_node_put(layout);
+
+ return 0;
+}
+
+static bool onie_tlv_hdr_is_valid(struct device *dev, struct onie_tlv_hdr *hdr)
+{
+ if (memcmp(hdr->id, ONIE_TLV_HDR_ID, sizeof(hdr->id))) {
+ dev_err(dev, "Invalid header\n");
+ return false;
+ }
+
+ if (hdr->version != 0x1) {
+ dev_err(dev, "Invalid version number\n");
+ return false;
+ }
+
+ return true;
+}
+
+static bool onie_tlv_crc_is_valid(struct device *dev, size_t table_len, u8 *table)
+{
+ struct onie_tlv crc_hdr;
+ u32 read_crc, calc_crc;
+ __be32 crc_be;
+
+ memcpy(&crc_hdr, table + table_len - ONIE_TLV_CRC_FIELD_SZ, sizeof(crc_hdr));
+ if (crc_hdr.type != 0xfe || crc_hdr.len != ONIE_TLV_CRC_SZ) {
+ dev_err(dev, "Invalid CRC field\n");
+ return false;
+ }
+
+ /* The table contains a JAMCRC, which is XOR'ed compared to the original
+ * CRC32 implementation as known in the Ethernet world.
+ */
+ memcpy(&crc_be, table + table_len - ONIE_TLV_CRC_SZ, ONIE_TLV_CRC_SZ);
+ read_crc = be32_to_cpu(crc_be);
+ calc_crc = crc32(~0, table, table_len - ONIE_TLV_CRC_SZ) ^ 0xFFFFFFFF;
+ if (read_crc != calc_crc) {
+ dev_err(dev, "Invalid CRC read: 0x%08x, expected: 0x%08x\n",
+ read_crc, calc_crc);
+ return false;
+ }
+
+ return true;
+}
+
+static int onie_tlv_parse_table(struct device *dev, struct nvmem_device *nvmem,
+ struct nvmem_layout *layout)
+{
+ struct onie_tlv_hdr hdr;
+ size_t table_len, data_len, hdr_len;
+ u8 *table, *data;
+ int ret;
+
+ ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
+ if (ret < 0)
+ return ret;
+
+ if (!onie_tlv_hdr_is_valid(dev, &hdr)) {
+ dev_err(dev, "Invalid ONIE TLV header\n");
+ return -EINVAL;
+ }
+
+ hdr_len = sizeof(hdr.id) + sizeof(hdr.version) + sizeof(hdr.data_len);
+ data_len = be16_to_cpu(hdr.data_len);
+ table_len = hdr_len + data_len;
+ if (table_len > ONIE_TLV_MAX_LEN) {
+ dev_err(dev, "Invalid ONIE TLV data length\n");
+ return -EINVAL;
+ }
+
+ table = devm_kmalloc(dev, table_len, GFP_KERNEL);
+ if (!table)
+ return -ENOMEM;
+
+ ret = nvmem_device_read(nvmem, 0, table_len, table);
+ if (ret != table_len)
+ return ret;
+
+ if (!onie_tlv_crc_is_valid(dev, table_len, table))
+ return -EINVAL;
+
+ data = table + hdr_len;
+ ret = onie_tlv_add_cells(dev, nvmem, data_len, data);
+ if (ret)
+ return ret;
+
+ return 0;
+}
+
+static const struct of_device_id onie_tlv_of_match_table[] = {
+ { .compatible = "onie,tlv-layout", },
+ {},
+};
+
+static struct nvmem_layout onie_tlv_layout = {
+ .name = "ONIE tlv layout",
+ .of_match_table = onie_tlv_of_match_table,
+ .add_cells = onie_tlv_parse_table,
+};
+
+static int __init onie_tlv_init(void)
+{
+ return nvmem_layout_register(&onie_tlv_layout);
+}
+subsys_initcall(onie_tlv_init);
--
2.30.2
There are no users anymore for the global cell_post_process callback
anymore. New users should use proper nvmem layouts.
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- none
changes since v3:
- none
changes since v2:
- none
changes since v1:
- new patch
drivers/nvmem/core.c | 9 ---------
include/linux/nvmem-provider.h | 2 --
2 files changed, 11 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 2706fc5ed494..dc329daaa350 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -38,7 +38,6 @@ struct nvmem_device {
unsigned int nkeepout;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
- nvmem_cell_post_process_t cell_post_process;
struct gpio_desc *wp_gpio;
struct nvmem_layout *layout;
void *priv;
@@ -892,7 +891,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
nvmem->type = config->type;
nvmem->reg_read = config->reg_read;
nvmem->reg_write = config->reg_write;
- nvmem->cell_post_process = config->cell_post_process;
nvmem->keepout = config->keepout;
nvmem->nkeepout = config->nkeepout;
if (config->of_node)
@@ -1562,13 +1560,6 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
return rc;
}
- if (nvmem->cell_post_process) {
- rc = nvmem->cell_post_process(nvmem->priv, id, index,
- cell->offset, buf, cell->bytes);
- if (rc)
- return rc;
- }
-
if (len)
*len = cell->bytes;
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index bfaba5227ac9..12833fe4eb4d 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -85,7 +85,6 @@ struct nvmem_cell_info {
* @no_of_node: Device should not use the parent's of_node even if it's !NULL.
* @reg_read: Callback to read data.
* @reg_write: Callback to write data.
- * @cell_post_process: Callback for vendor specific post processing of cell data
* @size: Device size.
* @word_size: Minimum read/write access granularity.
* @stride: Minimum read/write access stride.
@@ -120,7 +119,6 @@ struct nvmem_config {
bool no_of_node;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
- nvmem_cell_post_process_t cell_post_process;
int size;
int word_size;
int stride;
--
2.30.2
Add a helper to add an offset to a ethernet address. This comes in handy
if you have a base ethernet address for multiple interfaces.
Signed-off-by: Michael Walle <[email protected]>
Reviewed-by: Andrew Lunn <[email protected]>
Acked-by: Jakub Kicinski <[email protected]>
---
changes since v4:
- none
changes since v3:
- fix typo s/and/an/
changes since v2:
- none
changes since v1:
- none
include/linux/etherdevice.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index a541f0c4f146..224645f17c33 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -507,6 +507,20 @@ static inline void eth_addr_inc(u8 *addr)
u64_to_ether_addr(u, addr);
}
+/**
+ * eth_addr_add() - Add (or subtract) an offset to/from the given MAC address.
+ *
+ * @offset: Offset to add.
+ * @addr: Pointer to a six-byte array containing Ethernet address to increment.
+ */
+static inline void eth_addr_add(u8 *addr, long offset)
+{
+ u64 u = ether_addr_to_u64(addr);
+
+ u += offset;
+ u64_to_ether_addr(u, addr);
+}
+
/**
* is_etherdev_addr - Tell if given Ethernet address belongs to the device.
* @dev: Pointer to a device structure
--
2.30.2
This layout applies to the VPD of the Kontron sl28 boards. The VPD only
contains a base MAC address. Therefore, we have to add an individual
offset to it. This is done by taking the second argument of the nvmem
phandle into account. Also this let us checking the VPD version and the
checksum.
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- none
changes since v3:
- none
changes since v2:
- use of_nvmem_layout_get_container()
changes since v1:
- none
drivers/nvmem/layouts/Kconfig | 9 ++
drivers/nvmem/layouts/Makefile | 2 +
drivers/nvmem/layouts/sl28vpd.c | 153 ++++++++++++++++++++++++++++++++
3 files changed, 164 insertions(+)
create mode 100644 drivers/nvmem/layouts/sl28vpd.c
diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
index 9ad3911d1605..75082f6b471d 100644
--- a/drivers/nvmem/layouts/Kconfig
+++ b/drivers/nvmem/layouts/Kconfig
@@ -2,4 +2,13 @@
menu "Layout Types"
+config NVMEM_LAYOUT_SL28_VPD
+ bool "Kontron sl28 VPD layout support"
+ select CRC8
+ help
+ Say Y here if you want to support the VPD layout of the Kontron
+ SMARC-sAL28 boards.
+
+ If unsure, say N.
+
endmenu
diff --git a/drivers/nvmem/layouts/Makefile b/drivers/nvmem/layouts/Makefile
index 6fdb3c60a4fa..fc617b9e87d0 100644
--- a/drivers/nvmem/layouts/Makefile
+++ b/drivers/nvmem/layouts/Makefile
@@ -2,3 +2,5 @@
#
# Makefile for nvmem layouts.
#
+
+obj-$(CONFIG_NVMEM_LAYOUT_SL28_VPD) += sl28vpd.o
diff --git a/drivers/nvmem/layouts/sl28vpd.c b/drivers/nvmem/layouts/sl28vpd.c
new file mode 100644
index 000000000000..a36800f201a3
--- /dev/null
+++ b/drivers/nvmem/layouts/sl28vpd.c
@@ -0,0 +1,153 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/crc8.h>
+#include <linux/etherdevice.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <uapi/linux/if_ether.h>
+
+#define SL28VPD_MAGIC 'V'
+
+struct sl28vpd_header {
+ u8 magic;
+ u8 version;
+} __packed;
+
+struct sl28vpd_v1 {
+ struct sl28vpd_header header;
+ char serial_number[15];
+ u8 base_mac_address[ETH_ALEN];
+ u8 crc8;
+} __packed;
+
+static int sl28vpd_mac_address_pp(void *priv, const char *id, int index,
+ unsigned int offset, void *buf,
+ size_t bytes)
+{
+ if (bytes != ETH_ALEN)
+ return -EINVAL;
+
+ if (index < 0)
+ return -EINVAL;
+
+ if (!is_valid_ether_addr(buf))
+ return -EINVAL;
+
+ eth_addr_add(buf, index);
+
+ return 0;
+}
+
+static const struct nvmem_cell_info sl28vpd_v1_entries[] = {
+ {
+ .name = "serial-number",
+ .offset = offsetof(struct sl28vpd_v1, serial_number),
+ .bytes = sizeof_field(struct sl28vpd_v1, serial_number),
+ },
+ {
+ .name = "base-mac-address",
+ .offset = offsetof(struct sl28vpd_v1, base_mac_address),
+ .bytes = sizeof_field(struct sl28vpd_v1, base_mac_address),
+ .read_post_process = sl28vpd_mac_address_pp,
+ },
+};
+
+static int sl28vpd_v1_check_crc(struct device *dev, struct nvmem_device *nvmem)
+{
+ struct sl28vpd_v1 data_v1;
+ u8 table[CRC8_TABLE_SIZE];
+ int ret;
+ u8 crc;
+
+ crc8_populate_msb(table, 0x07);
+
+ ret = nvmem_device_read(nvmem, 0, sizeof(data_v1), &data_v1);
+ if (ret < 0)
+ return ret;
+ else if (ret != sizeof(data_v1))
+ return -EIO;
+
+ crc = crc8(table, (void *)&data_v1, sizeof(data_v1) - 1, 0);
+
+ if (crc != data_v1.crc8) {
+ dev_err(dev,
+ "Checksum is invalid (got %02x, expected %02x).\n",
+ crc, data_v1.crc8);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int sl28vpd_add_cells(struct device *dev, struct nvmem_device *nvmem,
+ struct nvmem_layout *layout)
+{
+ const struct nvmem_cell_info *pinfo;
+ struct nvmem_cell_info info = {0};
+ struct device_node *layout_np;
+ struct sl28vpd_header hdr;
+ int ret, i;
+
+ /* check header */
+ ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
+ if (ret < 0)
+ return ret;
+ else if (ret != sizeof(hdr))
+ return -EIO;
+
+ if (hdr.magic != SL28VPD_MAGIC) {
+ dev_err(dev, "Invalid magic value (%02x)\n", hdr.magic);
+ return -EINVAL;
+ }
+
+ if (hdr.version != 1) {
+ dev_err(dev, "Version %d is unsupported.\n", hdr.version);
+ return -EINVAL;
+ }
+
+ ret = sl28vpd_v1_check_crc(dev, nvmem);
+ if (ret)
+ return ret;
+
+ layout_np = of_nvmem_layout_get_container(nvmem);
+ if (!layout_np)
+ return -ENOENT;
+
+ for (i = 0; i < ARRAY_SIZE(sl28vpd_v1_entries); i++) {
+ pinfo = &sl28vpd_v1_entries[i];
+
+ info.name = pinfo->name;
+ info.offset = pinfo->offset;
+ info.bytes = pinfo->bytes;
+ info.read_post_process = pinfo->read_post_process;
+ info.np = of_get_child_by_name(layout_np, pinfo->name);
+
+ ret = nvmem_add_one_cell(nvmem, &info);
+ if (ret) {
+ of_node_put(layout_np);
+ return ret;
+ }
+ }
+
+ of_node_put(layout_np);
+
+ return 0;
+}
+
+static const struct of_device_id sl28vpd_of_match_table[] = {
+ { .compatible = "kontron,sl28-vpd" },
+ {},
+};
+
+struct nvmem_layout sl28vpd_layout = {
+ .name = "sl28-vpd",
+ .of_match_table = sl28vpd_of_match_table,
+ .add_cells = sl28vpd_add_cells,
+};
+
+static int __init sl28vpd_init(void)
+{
+ return nvmem_layout_register(&sl28vpd_layout);
+}
+subsys_initcall(sl28vpd_init);
--
2.30.2
It doesn't make any more sense to have a opaque pointer set up by the
nvmem device. Usually, the layout isn't associated with a particular
nvmem device. Instead, let the caller who set the post process callback
provide the priv pointer.
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- none
changes since v3:
- none
changes since v2:
- don't drop the pointer but let the user specify an opaque pointer
changes since v1:
- new patch
drivers/nvmem/core.c | 4 +++-
include/linux/nvmem-provider.h | 5 ++++-
2 files changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index dc329daaa350..2282dc80aa0b 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -53,6 +53,7 @@ struct nvmem_cell_entry {
int bit_offset;
int nbits;
nvmem_cell_post_process_t read_post_process;
+ void *priv;
struct device_node *np;
struct nvmem_device *nvmem;
struct list_head node;
@@ -470,6 +471,7 @@ static int nvmem_cell_info_to_nvmem_cell_entry_nodup(struct nvmem_device *nvmem,
cell->bytes = info->bytes;
cell->name = info->name;
cell->read_post_process = info->read_post_process;
+ cell->priv = info->priv;
cell->bit_offset = info->bit_offset;
cell->nbits = info->nbits;
@@ -1554,7 +1556,7 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
nvmem_shift_read_buffer_in_place(cell, buf);
if (cell->read_post_process) {
- rc = cell->read_post_process(nvmem->priv, id, index,
+ rc = cell->read_post_process(cell->priv, id, index,
cell->offset, buf, cell->bytes);
if (rc)
return rc;
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 12833fe4eb4d..cb0814f2ddae 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -20,7 +20,8 @@ typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
void *val, size_t bytes);
/* used for vendor specific post processing of cell data */
typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, int index,
- unsigned int offset, void *buf, size_t bytes);
+ unsigned int offset, void *buf,
+ size_t bytes);
enum nvmem_type {
NVMEM_TYPE_UNKNOWN = 0,
@@ -56,6 +57,7 @@ struct nvmem_keepout {
* @np: Optional device_node pointer.
* @read_post_process: Callback for optional post processing of cell data
* on reads.
+ * @priv: Opaque data passed to the read_post_process hook.
*/
struct nvmem_cell_info {
const char *name;
@@ -65,6 +67,7 @@ struct nvmem_cell_info {
unsigned int nbits;
struct device_node *np;
nvmem_cell_post_process_t read_post_process;
+ void *priv;
};
/**
--
2.30.2
NVMEM layouts are used to generate NVMEM cells during runtime. Think of
an EEPROM with a well-defined conent. For now, the content can be
described by a device tree or a board file. But this only works if the
offsets and lengths are static and don't change. One could also argue
that putting the layout of the EEPROM in the device tree is the wrong
place. Instead, the device tree should just have a specific compatible
string.
Right now there are two use cases:
(1) The NVMEM cell needs special processing. E.g. if it only specifies
a base MAC address offset and you need to add an offset, or it
needs to parse a MAC from ASCII format or some proprietary format.
(Post processing of cells is added in a later commit).
(2) u-boot environment parsing. The cells don't have a particular
offset but it needs parsing the content to determine the offsets
and length.
Co-developed-by: Miquel Raynal <[email protected]>
Signed-off-by: Miquel Raynal <[email protected]>
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- none
changes since v3:
- check return code of .add_cells()
changes since v2:
- look for "nvmem-layout" node and its compatible
- add of_nvmem_layout_get_container()
- special handling in of_nvmem_cell_get()
changes since v1:
- add documentation in nvmem.rst
- add nvmem_layout_unregister() + necessary module tracking
- make it possible to supply a layout via nvmem_register()
- check add_cells, before calling
Documentation/driver-api/nvmem.rst | 15 ++++
drivers/nvmem/Kconfig | 4 +
drivers/nvmem/Makefile | 1 +
drivers/nvmem/core.c | 118 +++++++++++++++++++++++++++++
drivers/nvmem/layouts/Kconfig | 5 ++
drivers/nvmem/layouts/Makefile | 4 +
include/linux/nvmem-consumer.h | 7 ++
include/linux/nvmem-provider.h | 51 +++++++++++++
8 files changed, 205 insertions(+)
create mode 100644 drivers/nvmem/layouts/Kconfig
create mode 100644 drivers/nvmem/layouts/Makefile
diff --git a/Documentation/driver-api/nvmem.rst b/Documentation/driver-api/nvmem.rst
index e3366322d46c..de221e91c8e3 100644
--- a/Documentation/driver-api/nvmem.rst
+++ b/Documentation/driver-api/nvmem.rst
@@ -185,3 +185,18 @@ ex::
=====================
See Documentation/devicetree/bindings/nvmem/nvmem.txt
+
+8. NVMEM layouts
+================
+
+NVMEM layouts are yet another mechanism to create cells. With the device
+tree binding it is possible to specify simple cells by using an offset
+and a length. Sometimes, the cells doesn't have a static offset, but
+the content is still well defined, e.g. tag-length-values. In this case,
+the NVMEM device content has to be first parsed and the cells need to
+be added accordingly. Layouts let you read the content of the NVMEM device
+and let you add cells dynamically.
+
+Another use case for layouts is the post processing of cells. With layouts,
+it is possible to associate a custom post processing hook to a cell. It
+even possible to add this hook to cells not created by the layout itself.
diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 755f551426b5..0e10b5b094b9 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -21,6 +21,10 @@ config NVMEM_SYSFS
This interface is mostly used by userspace applications to
read/write directly into nvmem.
+# Layouts
+
+source "drivers/nvmem/layouts/Kconfig"
+
# Devices
config NVMEM_APPLE_EFUSES
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index fa80fe17e567..4cf87ef6c24d 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -5,6 +5,7 @@
obj-$(CONFIG_NVMEM) += nvmem_core.o
nvmem_core-y := core.o
+obj-y += layouts/
# Devices
obj-$(CONFIG_NVMEM_APPLE_EFUSES) += nvmem-apple-efuses.o
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 0993e1ebdeaf..748a850a1960 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -40,6 +40,7 @@ struct nvmem_device {
nvmem_reg_write_t reg_write;
nvmem_cell_post_process_t cell_post_process;
struct gpio_desc *wp_gpio;
+ struct nvmem_layout *layout;
void *priv;
};
@@ -74,6 +75,9 @@ static LIST_HEAD(nvmem_lookup_list);
static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
+static DEFINE_SPINLOCK(nvmem_layout_lock);
+static LIST_HEAD(nvmem_layouts);
+
static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
void *val, size_t bytes)
{
@@ -728,6 +732,99 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
return 0;
}
+int __nvmem_layout_register(struct nvmem_layout *layout, struct module *owner)
+{
+ layout->owner = owner;
+
+ spin_lock(&nvmem_layout_lock);
+ list_add(&layout->node, &nvmem_layouts);
+ spin_unlock(&nvmem_layout_lock);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(__nvmem_layout_register);
+
+void nvmem_layout_unregister(struct nvmem_layout *layout)
+{
+ spin_lock(&nvmem_layout_lock);
+ list_del(&layout->node);
+ spin_unlock(&nvmem_layout_lock);
+}
+EXPORT_SYMBOL_GPL(nvmem_layout_unregister);
+
+static struct nvmem_layout *nvmem_layout_get(struct nvmem_device *nvmem)
+{
+ struct device_node *layout_np, *np = nvmem->dev.of_node;
+ struct nvmem_layout *l, *layout = NULL;
+
+ layout_np = of_get_child_by_name(np, "nvmem-layout");
+ if (!layout_np)
+ return NULL;
+
+ spin_lock(&nvmem_layout_lock);
+
+ list_for_each_entry(l, &nvmem_layouts, node) {
+ if (of_match_node(l->of_match_table, layout_np)) {
+ if (try_module_get(l->owner))
+ layout = l;
+
+ break;
+ }
+ }
+
+ spin_unlock(&nvmem_layout_lock);
+ of_node_put(layout_np);
+
+ return layout;
+}
+
+static void nvmem_layout_put(struct nvmem_layout *layout)
+{
+ if (layout)
+ module_put(layout->owner);
+}
+
+static int nvmem_add_cells_from_layout(struct nvmem_device *nvmem)
+{
+ struct nvmem_layout *layout = nvmem->layout;
+ int ret;
+
+ if (layout && layout->add_cells) {
+ ret = layout->add_cells(&nvmem->dev, nvmem, layout);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+#if IS_ENABLED(CONFIG_OF)
+/**
+ * of_nvmem_layout_get_container() - Get OF node to layout container.
+ *
+ * @nvmem: nvmem device.
+ *
+ * Return: a node pointer with refcount incremented or NULL if no
+ * container exists. Use of_node_put() on it when done.
+ */
+struct device_node *of_nvmem_layout_get_container(struct nvmem_device *nvmem)
+{
+ return of_get_child_by_name(nvmem->dev.of_node, "nvmem-layout");
+}
+EXPORT_SYMBOL_GPL(of_nvmem_layout_get_container);
+#endif
+
+const void *nvmem_layout_get_match_data(struct nvmem_device *nvmem,
+ struct nvmem_layout *layout)
+{
+ const struct of_device_id *match;
+
+ match = of_match_node(layout->of_match_table, nvmem->dev.of_node);
+
+ return match ? match->data : NULL;
+}
+EXPORT_SYMBOL_GPL(nvmem_layout_get_match_data);
+
/**
* nvmem_register() - Register a nvmem device for given nvmem_config.
* Also creates a binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
@@ -842,6 +939,12 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
goto err_device_del;
}
+ /*
+ * If the driver supplied a layout by config->layout, the module
+ * pointer will be NULL and nvmem_layout_put() will be a noop.
+ */
+ nvmem->layout = config->layout ?: nvmem_layout_get(nvmem);
+
if (config->cells) {
rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
if (rval)
@@ -856,6 +959,10 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
if (rval)
goto err_remove_cells;
+ rval = nvmem_add_cells_from_layout(nvmem);
+ if (rval)
+ goto err_remove_cells;
+
blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
return nvmem;
@@ -864,6 +971,7 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
nvmem_device_remove_all_cells(nvmem);
if (config->compat)
nvmem_sysfs_remove_compat(nvmem, config);
+ nvmem_layout_put(nvmem->layout);
err_device_del:
device_del(&nvmem->dev);
err_put_device:
@@ -885,6 +993,7 @@ static void nvmem_device_release(struct kref *kref)
device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
nvmem_device_remove_all_cells(nvmem);
+ nvmem_layout_put(nvmem->layout);
device_unregister(&nvmem->dev);
}
@@ -1250,6 +1359,15 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
return ERR_PTR(-EINVAL);
}
+ /* nvmem layouts produce cells within the nvmem-layout container */
+ if (of_node_name_eq(nvmem_np, "nvmem-layout")) {
+ nvmem_np = of_get_next_parent(nvmem_np);
+ if (!nvmem_np) {
+ of_node_put(cell_np);
+ return ERR_PTR(-EINVAL);
+ }
+ }
+
nvmem = __nvmem_device_get(nvmem_np, device_match_of_node);
of_node_put(nvmem_np);
if (IS_ERR(nvmem)) {
diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
new file mode 100644
index 000000000000..9ad3911d1605
--- /dev/null
+++ b/drivers/nvmem/layouts/Kconfig
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0
+
+menu "Layout Types"
+
+endmenu
diff --git a/drivers/nvmem/layouts/Makefile b/drivers/nvmem/layouts/Makefile
new file mode 100644
index 000000000000..6fdb3c60a4fa
--- /dev/null
+++ b/drivers/nvmem/layouts/Makefile
@@ -0,0 +1,4 @@
+# SPDX-License-Identifier: GPL-2.0
+#
+# Makefile for nvmem layouts.
+#
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 1f62f7ba71ca..fa030d93b768 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -239,6 +239,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
const char *id);
struct nvmem_device *of_nvmem_device_get(struct device_node *np,
const char *name);
+struct device_node *of_nvmem_layout_get_container(struct nvmem_device *nvmem);
#else
static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
const char *id)
@@ -251,6 +252,12 @@ static inline struct nvmem_device *of_nvmem_device_get(struct device_node *np,
{
return ERR_PTR(-EOPNOTSUPP);
}
+
+static inline struct device_node *
+of_nvmem_layout_get_container(struct nvmem_device *nvmem)
+{
+ return ERR_PTR(-EOPNOTSUPP);
+}
#endif /* CONFIG_NVMEM && CONFIG_OF */
#endif /* ifndef _LINUX_NVMEM_CONSUMER_H */
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 385d29168008..4185767c114f 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -89,6 +89,7 @@ struct nvmem_cell_info {
* @priv: User context passed to read/write callbacks.
* @wp-gpio: Write protect pin
* @ignore_wp: Write Protect pin is managed by the provider.
+ * @layout: Fixed layout associated with this nvmem device.
*
* Note: A default "nvmem<id>" name will be assigned to the device if
* no name is specified in its configuration. In such case "<id>" is
@@ -111,6 +112,7 @@ struct nvmem_config {
bool read_only;
bool root_only;
bool ignore_wp;
+ struct nvmem_layout *layout;
struct device_node *of_node;
bool no_of_node;
nvmem_reg_read_t reg_read;
@@ -144,6 +146,33 @@ struct nvmem_cell_table {
struct list_head node;
};
+/**
+ * struct nvmem_layout - NVMEM layout definitions
+ *
+ * @name: Layout name.
+ * @of_match_table: Open firmware match table.
+ * @add_cells: Will be called if a nvmem device is found which
+ * has this layout. The function will add layout
+ * specific cells with nvmem_add_one_cell().
+ * @owner: Pointer to struct module.
+ * @node: List node.
+ *
+ * A nvmem device can hold a well defined structure which can just be
+ * evaluated during runtime. For example a TLV list, or a list of "name=val"
+ * pairs. A nvmem layout can parse the nvmem device and add appropriate
+ * cells.
+ */
+struct nvmem_layout {
+ const char *name;
+ const struct of_device_id *of_match_table;
+ int (*add_cells)(struct device *dev, struct nvmem_device *nvmem,
+ struct nvmem_layout *layout);
+
+ /* private */
+ struct module *owner;
+ struct list_head node;
+};
+
#if IS_ENABLED(CONFIG_NVMEM)
struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
@@ -158,6 +187,14 @@ void nvmem_del_cell_table(struct nvmem_cell_table *table);
int nvmem_add_one_cell(struct nvmem_device *nvmem,
const struct nvmem_cell_info *info);
+int __nvmem_layout_register(struct nvmem_layout *layout, struct module *owner);
+#define nvmem_layout_register(layout) \
+ __nvmem_layout_register(layout, THIS_MODULE)
+void nvmem_layout_unregister(struct nvmem_layout *layout);
+
+const void *nvmem_layout_get_match_data(struct nvmem_device *nvmem,
+ struct nvmem_layout *layout);
+
#else
static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
@@ -181,5 +218,19 @@ static inline int nvmem_add_one_cell(struct nvmem_device *nvmem,
return -EOPNOTSUPP;
}
+static inline int nvmem_layout_register(struct nvmem_layout *layout)
+{
+ return -EOPNOTSUPP;
+}
+
+static inline void nvmem_layout_unregister(struct nvmem_layout *layout) {}
+
+static inline const void *
+nvmem_layout_get_match_data(struct nvmem_device *nvmem,
+ struct nvmem_layout *layout)
+{
+ return NULL;
+}
+
#endif /* CONFIG_NVMEM */
#endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
--
2.30.2
Convert nvmem_add_cells_from_of() to use the new nvmem_add_one_cell().
This will remove duplicate code and it will make it possible to add a
hook to a nvmem layout in between, which can change fields before the
cell is finally added.
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- drop now unused allocation of nvmem_cell_entrys, thanks Dan
changes since v3:
- none
changes since v2:
- none
changes since v1:
- new patch
drivers/nvmem/core.c | 45 ++++++++++++++------------------------------
1 file changed, 14 insertions(+), 31 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 5db169aa7988..0993e1ebdeaf 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -688,15 +688,14 @@ static int nvmem_validate_keepouts(struct nvmem_device *nvmem)
static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
{
- struct device_node *parent, *child;
struct device *dev = &nvmem->dev;
- struct nvmem_cell_entry *cell;
+ struct device_node *child;
const __be32 *addr;
- int len;
+ int len, ret;
- parent = dev->of_node;
+ for_each_child_of_node(dev->of_node, child) {
+ struct nvmem_cell_info info = {0};
- for_each_child_of_node(parent, child) {
addr = of_get_property(child, "reg", &len);
if (!addr)
continue;
@@ -706,40 +705,24 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
return -EINVAL;
}
- cell = kzalloc(sizeof(*cell), GFP_KERNEL);
- if (!cell) {
- of_node_put(child);
- return -ENOMEM;
- }
-
- cell->nvmem = nvmem;
- cell->offset = be32_to_cpup(addr++);
- cell->bytes = be32_to_cpup(addr);
- cell->name = kasprintf(GFP_KERNEL, "%pOFn", child);
+ info.offset = be32_to_cpup(addr++);
+ info.bytes = be32_to_cpup(addr);
+ info.name = kasprintf(GFP_KERNEL, "%pOFn", child);
addr = of_get_property(child, "bits", &len);
if (addr && len == (2 * sizeof(u32))) {
- cell->bit_offset = be32_to_cpup(addr++);
- cell->nbits = be32_to_cpup(addr);
+ info.bit_offset = be32_to_cpup(addr++);
+ info.nbits = be32_to_cpup(addr);
}
- if (cell->nbits)
- cell->bytes = DIV_ROUND_UP(
- cell->nbits + cell->bit_offset,
- BITS_PER_BYTE);
+ info.np = of_node_get(child);
- if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
- dev_err(dev, "cell %s unaligned to nvmem stride %d\n",
- cell->name, nvmem->stride);
- /* Cells already added will be freed later. */
- kfree_const(cell->name);
- kfree(cell);
+ ret = nvmem_add_one_cell(nvmem, &info);
+ kfree(info.name);
+ if (ret) {
of_node_put(child);
- return -EINVAL;
+ return ret;
}
-
- cell->np = of_node_get(child);
- nvmem_cell_entry_add(cell);
}
return 0;
--
2.30.2
Bindings describe the new '#nvmem-cell-cells' property. Now that the
arguments count property is optional, we just add this property to the
nvmem-cells.
Signed-off-by: Michael Walle <[email protected]>
Tested-by: Miquel Raynal <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
changes since v4:
- new patch
changes since v3:
- new patch
drivers/of/property.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/of/property.c b/drivers/of/property.c
index 3043ca7735db..8d9ba20a8f90 100644
--- a/drivers/of/property.c
+++ b/drivers/of/property.c
@@ -1307,7 +1307,7 @@ DEFINE_SIMPLE_PROP(dmas, "dmas", "#dma-cells")
DEFINE_SIMPLE_PROP(power_domains, "power-domains", "#power-domain-cells")
DEFINE_SIMPLE_PROP(hwlocks, "hwlocks", "#hwlock-cells")
DEFINE_SIMPLE_PROP(extcon, "extcon", NULL)
-DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", NULL)
+DEFINE_SIMPLE_PROP(nvmem_cells, "nvmem-cells", "#nvmem-cell-cells")
DEFINE_SIMPLE_PROP(phys, "phys", "#phy-cells")
DEFINE_SIMPLE_PROP(wakeup_parent, "wakeup-parent", NULL)
DEFINE_SIMPLE_PROP(pinctrl0, "pinctrl-0", NULL)
--
2.30.2
Add a new function to add exactly one cell. This will be used by the
nvmem layout drivers to add custom cells. In contrast to the
nvmem_add_cells(), this has the advantage that we don't have to assemble
a list of cells on runtime.
Signed-off-by: Michael Walle <[email protected]>
---
changes since v4:
- none
changes since v3:
- none
changes since v2:
- add EXPORT_SYMBOL_GPL()
changes since v1:
- none
drivers/nvmem/core.c | 59 ++++++++++++++++++++--------------
include/linux/nvmem-provider.h | 8 +++++
2 files changed, 43 insertions(+), 24 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 70c3d0a20d0d..5db169aa7988 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -501,6 +501,36 @@ static int nvmem_cell_info_to_nvmem_cell_entry(struct nvmem_device *nvmem,
return 0;
}
+/**
+ * nvmem_add_one_cell() - Add one cell information to an nvmem device
+ *
+ * @nvmem: nvmem device to add cells to.
+ * @info: nvmem cell info to add to the device
+ *
+ * Return: 0 or negative error code on failure.
+ */
+int nvmem_add_one_cell(struct nvmem_device *nvmem,
+ const struct nvmem_cell_info *info)
+{
+ struct nvmem_cell_entry *cell;
+ int rval;
+
+ cell = kzalloc(sizeof(*cell), GFP_KERNEL);
+ if (!cell)
+ return -ENOMEM;
+
+ rval = nvmem_cell_info_to_nvmem_cell_entry(nvmem, info, cell);
+ if (rval) {
+ kfree(cell);
+ return rval;
+ }
+
+ nvmem_cell_entry_add(cell);
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(nvmem_add_one_cell);
+
/**
* nvmem_add_cells() - Add cell information to an nvmem device
*
@@ -514,34 +544,15 @@ static int nvmem_add_cells(struct nvmem_device *nvmem,
const struct nvmem_cell_info *info,
int ncells)
{
- struct nvmem_cell_entry **cells;
- int i, rval = 0;
-
- cells = kcalloc(ncells, sizeof(*cells), GFP_KERNEL);
- if (!cells)
- return -ENOMEM;
+ int i, rval;
for (i = 0; i < ncells; i++) {
- cells[i] = kzalloc(sizeof(**cells), GFP_KERNEL);
- if (!cells[i]) {
- rval = -ENOMEM;
- goto out;
- }
-
- rval = nvmem_cell_info_to_nvmem_cell_entry(nvmem, &info[i], cells[i]);
- if (rval) {
- kfree(cells[i]);
- goto out;
- }
-
- nvmem_cell_entry_add(cells[i]);
+ rval = nvmem_add_one_cell(nvmem, &info[i]);
+ if (rval)
+ return rval;
}
-out:
- /* remove tmp array */
- kfree(cells);
-
- return rval;
+ return 0;
}
/**
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 14a32a1bc249..385d29168008 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -155,6 +155,9 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
void nvmem_add_cell_table(struct nvmem_cell_table *table);
void nvmem_del_cell_table(struct nvmem_cell_table *table);
+int nvmem_add_one_cell(struct nvmem_device *nvmem,
+ const struct nvmem_cell_info *info);
+
#else
static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
@@ -172,6 +175,11 @@ devm_nvmem_register(struct device *dev, const struct nvmem_config *c)
static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
+static inline int nvmem_add_one_cell(struct nvmem_device *nvmem,
+ const struct nvmem_cell_info *info)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_NVMEM */
#endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
--
2.30.2
Hi Srinivas,
[email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
> This is now the third attempt to fetch the MAC addresses from the VPD
> for the Kontron sl28 boards. Previous discussions can be found here:
> https://lore.kernel.org/lkml/[email protected]/
>
>
> NVMEM cells are typically added by board code or by the devicetree. But
> as the cells get more complex, there is (valid) push back from the
> devicetree maintainers to not put that handling in the devicetree.
>
> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
> can add cells during runtime. That way it is possible to add more complex
> cells than it is possible right now with the offset/length/bits
> description in the device tree. For example, you can have post processing
> for individual cells (think of endian swapping, or ethernet offset
> handling).
>
> The imx-ocotp driver is the only user of the global post processing hook,
> convert it to nvmem layouts and drop the global post pocessing hook.
>
> For now, the layouts are selected by the device tree. But the idea is
> that also board files or other drivers could set a layout. Although no
> code for that exists yet.
>
> Thanks to Miquel, the device tree bindings are already approved and merged.
>
> NVMEM layouts as modules?
> While possible in principle, it doesn't make any sense because the NVMEM
> core can't be compiled as a module. The layouts needs to be available at
> probe time. (That is also the reason why they get registered with
> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> could be modules, too.
I believe this series still applies even though -rc1 (and -rc2) are out
now, may we know if you consider merging it anytime soon or if there
are still discrepancies in the implementation you would like to
discuss? Otherwise I would really like to see this laying in -next a
few weeks before being sent out to Linus, just in case.
Thanks,
Miquèl
Hi Miquel,
On 03/01/2023 15:39, Miquel Raynal wrote:
> Hi Srinivas,
>
> [email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
>
>> This is now the third attempt to fetch the MAC addresses from the VPD
>> for the Kontron sl28 boards. Previous discussions can be found here:
>> https://lore.kernel.org/lkml/[email protected]/
>>
>>
>> NVMEM cells are typically added by board code or by the devicetree. But
>> as the cells get more complex, there is (valid) push back from the
>> devicetree maintainers to not put that handling in the devicetree.
>>
>> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
>> can add cells during runtime. That way it is possible to add more complex
>> cells than it is possible right now with the offset/length/bits
>> description in the device tree. For example, you can have post processing
>> for individual cells (think of endian swapping, or ethernet offset
>> handling).
>>
>> The imx-ocotp driver is the only user of the global post processing hook,
>> convert it to nvmem layouts and drop the global post pocessing hook.
>>
>> For now, the layouts are selected by the device tree. But the idea is
>> that also board files or other drivers could set a layout. Although no
>> code for that exists yet.
>>
>> Thanks to Miquel, the device tree bindings are already approved and merged.
>>
>> NVMEM layouts as modules?
>> While possible in principle, it doesn't make any sense because the NVMEM
>> core can't be compiled as a module. The layouts needs to be available at
>> probe time. (That is also the reason why they get registered with
>> subsys_initcall().) So if the NVMEM core would be a module, the layouts
>> could be modules, too.
>
> I believe this series still applies even though -rc1 (and -rc2) are out
> now, may we know if you consider merging it anytime soon or if there
> are still discrepancies in the implementation you would like to
> discuss? Otherwise I would really like to see this laying in -next a
> few weeks before being sent out to Linus, just in case.
Thanks for the work!
Lets get some testing in -next.
Applied now,
--srini
>
> Thanks,
> Miquèl
Hi Srinivas,
[email protected] wrote on Tue, 3 Jan 2023 15:51:31 +0000:
> Hi Miquel,
>
> On 03/01/2023 15:39, Miquel Raynal wrote:
> > Hi Srinivas,
> >
> > [email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
> >
> >> This is now the third attempt to fetch the MAC addresses from the VPD
> >> for the Kontron sl28 boards. Previous discussions can be found here:
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >>
> >> NVMEM cells are typically added by board code or by the devicetree. But
> >> as the cells get more complex, there is (valid) push back from the
> >> devicetree maintainers to not put that handling in the devicetree.
> >>
> >> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
> >> can add cells during runtime. That way it is possible to add more complex
> >> cells than it is possible right now with the offset/length/bits
> >> description in the device tree. For example, you can have post processing
> >> for individual cells (think of endian swapping, or ethernet offset
> >> handling).
> >>
> >> The imx-ocotp driver is the only user of the global post processing hook,
> >> convert it to nvmem layouts and drop the global post pocessing hook.
> >>
> >> For now, the layouts are selected by the device tree. But the idea is
> >> that also board files or other drivers could set a layout. Although no
> >> code for that exists yet.
> >>
> >> Thanks to Miquel, the device tree bindings are already approved and merged.
> >>
> >> NVMEM layouts as modules?
> >> While possible in principle, it doesn't make any sense because the NVMEM
> >> core can't be compiled as a module. The layouts needs to be available at
> >> probe time. (That is also the reason why they get registered with
> >> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> >> could be modules, too.
> >
> > I believe this series still applies even though -rc1 (and -rc2) are out
> > now, may we know if you consider merging it anytime soon or if there
> > are still discrepancies in the implementation you would like to
> > discuss? Otherwise I would really like to see this laying in -next a
> > few weeks before being sent out to Linus, just in case.
>
> Thanks for the work!
>
> Lets get some testing in -next.
>
>
> Applied now,
Excellent! Thanks a lot for the quick answer and thanks for applying,
let's see how it behaves.
Thanks,
Miquèl
Am Dienstag, 3. Januar 2023, 16:51:31 CET schrieb Srinivas Kandagatla:
> Hi Miquel,
>
> On 03/01/2023 15:39, Miquel Raynal wrote:
> > Hi Srinivas,
> >
> > [email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
> >> This is now the third attempt to fetch the MAC addresses from the VPD
> >> for the Kontron sl28 boards. Previous discussions can be found here:
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >>
> >> NVMEM cells are typically added by board code or by the devicetree. But
> >> as the cells get more complex, there is (valid) push back from the
> >> devicetree maintainers to not put that handling in the devicetree.
> >>
> >> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
> >> can add cells during runtime. That way it is possible to add more complex
> >> cells than it is possible right now with the offset/length/bits
> >> description in the device tree. For example, you can have post processing
> >> for individual cells (think of endian swapping, or ethernet offset
> >> handling).
> >>
> >> The imx-ocotp driver is the only user of the global post processing hook,
> >> convert it to nvmem layouts and drop the global post pocessing hook.
> >>
> >> For now, the layouts are selected by the device tree. But the idea is
> >> that also board files or other drivers could set a layout. Although no
> >> code for that exists yet.
> >>
> >> Thanks to Miquel, the device tree bindings are already approved and
> >> merged.
> >>
> >> NVMEM layouts as modules?
> >> While possible in principle, it doesn't make any sense because the NVMEM
> >> core can't be compiled as a module. The layouts needs to be available at
> >> probe time. (That is also the reason why they get registered with
> >> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> >> could be modules, too.
> >
> > I believe this series still applies even though -rc1 (and -rc2) are out
> > now, may we know if you consider merging it anytime soon or if there
> > are still discrepancies in the implementation you would like to
> > discuss? Otherwise I would really like to see this laying in -next a
> > few weeks before being sent out to Linus, just in case.
>
> Thanks for the work!
>
> Lets get some testing in -next.
This causes the following errors on existing boards (imx8mq-tqma8mq-
mba8mx.dtb):
root@tqma8-common:~# uname -r
6.2.0-rc2-next-20230105
> OF: /soc@0: could not get #nvmem-cell-cells for /soc@0/bus@30000000/
efuse@30350000/soc-uid@4
> OF: /soc@0/bus@30800000/ethernet@30be0000: could not get #nvmem-cell-cells
for /soc@0/bus@30000000/efuse@30350000/mac-address@90
These are caused because '#nvmem-cell-cells = <0>;' is not explicitly set in
DT.
> TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to get nvmem cell
io_impedance_ctrl
> TI DP83867: probe of 30be0000.ethernet-1:0e failed with error -22
These are caused because of_nvmem_cell_get() now returns -EINVAL instead of -
ENODEV if the requested nvmem cell is not available.
Best regards,
Alexander
Hello,
[email protected] wrote on Thu, 05 Jan 2023 12:04:52
+0100:
> Am Dienstag, 3. Januar 2023, 16:51:31 CET schrieb Srinivas Kandagatla:
> > Hi Miquel,
> >
> > On 03/01/2023 15:39, Miquel Raynal wrote:
> > > Hi Srinivas,
> > >
> > > [email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
> > >> This is now the third attempt to fetch the MAC addresses from the VPD
> > >> for the Kontron sl28 boards. Previous discussions can be found here:
> > >> https://lore.kernel.org/lkml/[email protected]/
> > >>
> > >>
> > >> NVMEM cells are typically added by board code or by the devicetree. But
> > >> as the cells get more complex, there is (valid) push back from the
> > >> devicetree maintainers to not put that handling in the devicetree.
> > >>
> > >> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
> > >> can add cells during runtime. That way it is possible to add more complex
> > >> cells than it is possible right now with the offset/length/bits
> > >> description in the device tree. For example, you can have post processing
> > >> for individual cells (think of endian swapping, or ethernet offset
> > >> handling).
> > >>
> > >> The imx-ocotp driver is the only user of the global post processing hook,
> > >> convert it to nvmem layouts and drop the global post pocessing hook.
> > >>
> > >> For now, the layouts are selected by the device tree. But the idea is
> > >> that also board files or other drivers could set a layout. Although no
> > >> code for that exists yet.
> > >>
> > >> Thanks to Miquel, the device tree bindings are already approved and
> > >> merged.
> > >>
> > >> NVMEM layouts as modules?
> > >> While possible in principle, it doesn't make any sense because the NVMEM
> > >> core can't be compiled as a module. The layouts needs to be available at
> > >> probe time. (That is also the reason why they get registered with
> > >> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> > >> could be modules, too.
> > >
> > > I believe this series still applies even though -rc1 (and -rc2) are out
> > > now, may we know if you consider merging it anytime soon or if there
> > > are still discrepancies in the implementation you would like to
> > > discuss? Otherwise I would really like to see this laying in -next a
> > > few weeks before being sent out to Linus, just in case.
> >
> > Thanks for the work!
> >
> > Lets get some testing in -next.
>
> This causes the following errors on existing boards (imx8mq-tqma8mq-
> mba8mx.dtb):
> root@tqma8-common:~# uname -r
> 6.2.0-rc2-next-20230105
>
> > OF: /soc@0: could not get #nvmem-cell-cells for /soc@0/bus@30000000/
> efuse@30350000/soc-uid@4
> > OF: /soc@0/bus@30800000/ethernet@30be0000: could not get #nvmem-cell-cells
> for /soc@0/bus@30000000/efuse@30350000/mac-address@90
>
> These are caused because '#nvmem-cell-cells = <0>;' is not explicitly set in
> DT.
>
> > TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to get nvmem cell
> io_impedance_ctrl
> > TI DP83867: probe of 30be0000.ethernet-1:0e failed with error -22
>
> These are caused because of_nvmem_cell_get() now returns -EINVAL instead of -
> ENODEV if the requested nvmem cell is not available.
Should we just assume #nvmem-cell-cells = <0> by default? I guess it's
a safe assumption.
Thanks,
Miquèl
Hi Michael,
Am Donnerstag, 5. Januar 2023, 13:11:37 CET schrieb Michael Walle:
> Hi Alexander,
>
> thanks for debugging. I'm not yet sure what is going wrong, so
> I have some more questions below.
>
> >> This causes the following errors on existing boards (imx8mq-tqma8mq-
> >> mba8mx.dtb):
> >> root@tqma8-common:~# uname -r
> >> 6.2.0-rc2-next-20230105
> >>
> >> > OF: /soc@0: could not get #nvmem-cell-cells for /soc@0/bus@30000000/
> >>
> >> efuse@30350000/soc-uid@4
> >>
> >> > OF: /soc@0/bus@30800000/ethernet@30be0000: could not get
> >> > #nvmem-cell-cells
> >>
> >> for /soc@0/bus@30000000/efuse@30350000/mac-address@90
> >>
> >> These are caused because '#nvmem-cell-cells = <0>;' is not explicitly
> >> set in
> >> DT.
> >>
> >> > TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to get nvmem
> >> > cell
> >>
> >> io_impedance_ctrl
> >>
> >> > TI DP83867: probe of 30be0000.ethernet-1:0e failed with error -22
> >>
> >> These are caused because of_nvmem_cell_get() now returns -EINVAL
> >> instead of -
> >> ENODEV if the requested nvmem cell is not available.
>
> What do you mean with not available? Not yet available because of probe
> order?
Ah, I was talking about there is no nvmem cell being used in my PHY node, e.g.
no 'nvmem-cells' nor 'nvmem-cell-names' (set to 'io_impedance_ctrl'). That's
why of_property_match_string returns -EINVAL.
> > Should we just assume #nvmem-cell-cells = <0> by default? I guess it's
> > a safe assumption.
>
> Actually, that's what patch 2/21 is for.
>
> Alexander, did you verify that the EINVAL is returned by
> of_parse_phandle_with_optional_args()?
Yep.
--8<--
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 1b61c8bf0de4..f2a85a31d039 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1339,9 +1339,11 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node
*np, const char *id)
if (id)
index = of_property_match_string(np, "nvmem-cell-names", id);
+ pr_info("%s: index: %d\n", __func__, index);
ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
"#nvmem-cell-cells",
index, &cell_spec);
+ pr_info("%s: of_parse_phandle_with_optional_args: %d\n", __func__,
ret);
if (ret)
return ERR_PTR(ret);
--8<--
Results in:
> [ 1.861896] of_nvmem_cell_get: index: -22
> [ 1.865934] of_nvmem_cell_get: of_parse_phandle_with_optional_args: -22
> [ 1.872595] TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to
get nvmem cell io_impedance_ctrl
> [ 2.402575] TI DP83867: probe of 30be0000.ethernet-1:0e failed with error
-22
So, the index is wrong in the first place, but this was no problem until now.
Best regards,
Alexander
Hi Alexander,
thanks for debugging. I'm not yet sure what is going wrong, so
I have some more questions below.
>> This causes the following errors on existing boards (imx8mq-tqma8mq-
>> mba8mx.dtb):
>> root@tqma8-common:~# uname -r
>> 6.2.0-rc2-next-20230105
>>
>> > OF: /soc@0: could not get #nvmem-cell-cells for /soc@0/bus@30000000/
>> efuse@30350000/soc-uid@4
>> > OF: /soc@0/bus@30800000/ethernet@30be0000: could not get #nvmem-cell-cells
>> for /soc@0/bus@30000000/efuse@30350000/mac-address@90
>>
>> These are caused because '#nvmem-cell-cells = <0>;' is not explicitly
>> set in
>> DT.
>>
>> > TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to get nvmem cell
>> io_impedance_ctrl
>> > TI DP83867: probe of 30be0000.ethernet-1:0e failed with error -22
>>
>> These are caused because of_nvmem_cell_get() now returns -EINVAL
>> instead of -
>> ENODEV if the requested nvmem cell is not available.
What do you mean with not available? Not yet available because of probe
order?
> Should we just assume #nvmem-cell-cells = <0> by default? I guess it's
> a safe assumption.
Actually, that's what patch 2/21 is for.
Alexander, did you verify that the EINVAL is returned by
of_parse_phandle_with_optional_args()?
-michael
Hi,
Am 2023-01-05 13:21, schrieb Alexander Stein:
> Am Donnerstag, 5. Januar 2023, 13:11:37 CET schrieb Michael Walle:
>> thanks for debugging. I'm not yet sure what is going wrong, so
>> I have some more questions below.
>>
>> >> This causes the following errors on existing boards (imx8mq-tqma8mq-
>> >> mba8mx.dtb):
>> >> root@tqma8-common:~# uname -r
>> >> 6.2.0-rc2-next-20230105
>> >>
>> >> > OF: /soc@0: could not get #nvmem-cell-cells for /soc@0/bus@30000000/
>> >>
>> >> efuse@30350000/soc-uid@4
>> >>
>> >> > OF: /soc@0/bus@30800000/ethernet@30be0000: could not get
>> >> > #nvmem-cell-cells
>> >>
>> >> for /soc@0/bus@30000000/efuse@30350000/mac-address@90
>> >>
>> >> These are caused because '#nvmem-cell-cells = <0>;' is not explicitly
>> >> set in
>> >> DT.
>> >>
>> >> > TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to get nvmem
>> >> > cell
>> >>
>> >> io_impedance_ctrl
>> >>
>> >> > TI DP83867: probe of 30be0000.ethernet-1:0e failed with error -22
>> >>
>> >> These are caused because of_nvmem_cell_get() now returns -EINVAL
>> >> instead of -
>> >> ENODEV if the requested nvmem cell is not available.
>>
>> What do you mean with not available? Not yet available because of
>> probe
>> order?
>
> Ah, I was talking about there is no nvmem cell being used in my PHY
> node, e.g.
> no 'nvmem-cells' nor 'nvmem-cell-names' (set to 'io_impedance_ctrl').
> That's
> why of_property_match_string returns -EINVAL.
Ahh I see. You mean ENOENT instead of ENODEV, right?
>> > Should we just assume #nvmem-cell-cells = <0> by default? I guess it's
>> > a safe assumption.
>>
>> Actually, that's what patch 2/21 is for.
>>
>> Alexander, did you verify that the EINVAL is returned by
>> of_parse_phandle_with_optional_args()?
>
> Yep.
>
> --8<--
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 1b61c8bf0de4..f2a85a31d039 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1339,9 +1339,11 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> device_node
> *np, const char *id)
> if (id)
> index = of_property_match_string(np,
> "nvmem-cell-names", id);
>
> + pr_info("%s: index: %d\n", __func__, index);
> ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
> "#nvmem-cell-cells",
> index, &cell_spec);
> + pr_info("%s: of_parse_phandle_with_optional_args: %d\n",
> __func__,
> ret);
> if (ret)
> return ERR_PTR(ret);
> --8<--
>
> Results in:
>> [ 1.861896] of_nvmem_cell_get: index: -22
>> [ 1.865934] of_nvmem_cell_get: of_parse_phandle_with_optional_args:
>> -22
>> [ 1.872595] TI DP83867 30be0000.ethernet-1:0e: error -EINVAL:
>> failed to
> get nvmem cell io_impedance_ctrl
>> [ 2.402575] TI DP83867: probe of 30be0000.ethernet-1:0e failed with
>> error
> -22
>
> So, the index is wrong in the first place, but this was no problem
> until now.
Thanks, could you try the following patch:
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 1b61c8bf0de4..1085abfcd9b1 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1336,8 +1336,11 @@ struct nvmem_cell *of_nvmem_cell_get(struct
device_node *np, const char *id)
int ret;
/* if cell name exists, find index to the name */
- if (id)
+ if (id) {
index = of_property_match_string(np, "nvmem-cell-names",
id);
+ if (index < 0)
+ return ERR_PTR(-ENOENT);
+ }
ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
"#nvmem-cell-cells",
Before patch 6/21, the -EINVAL was passed as index to of_parse_phandle()
which then returned NULL, which caused the nvmem core to return ENOENT.
I have a vague memory, that I made sure, that
of_parse_phandle_with_optional_args() will also propagate the
wrong index to its return code. But now, it won't be converted
to ENOENT.
-michael
Hi Michael,
Am Donnerstag, 5. Januar 2023, 13:51:53 CET schrieb Michael Walle:
> Hi,
>
> Am 2023-01-05 13:21, schrieb Alexander Stein:
> > Am Donnerstag, 5. Januar 2023, 13:11:37 CET schrieb Michael Walle:
> >> thanks for debugging. I'm not yet sure what is going wrong, so
> >> I have some more questions below.
> >>
> >> >> This causes the following errors on existing boards (imx8mq-tqma8mq-
> >> >> mba8mx.dtb):
> >> >> root@tqma8-common:~# uname -r
> >> >> 6.2.0-rc2-next-20230105
> >> >>
> >> >> > OF: /soc@0: could not get #nvmem-cell-cells for /soc@0/bus@30000000/
> >> >>
> >> >> efuse@30350000/soc-uid@4
> >> >>
> >> >> > OF: /soc@0/bus@30800000/ethernet@30be0000: could not get
> >> >> > #nvmem-cell-cells
> >> >>
> >> >> for /soc@0/bus@30000000/efuse@30350000/mac-address@90
> >> >>
> >> >> These are caused because '#nvmem-cell-cells = <0>;' is not explicitly
> >> >> set in
> >> >> DT.
> >> >>
> >> >> > TI DP83867 30be0000.ethernet-1:0e: error -EINVAL: failed to get
> >> >> > nvmem
> >> >> > cell
> >> >>
> >> >> io_impedance_ctrl
> >> >>
> >> >> > TI DP83867: probe of 30be0000.ethernet-1:0e failed with error -22
> >> >>
> >> >> These are caused because of_nvmem_cell_get() now returns -EINVAL
> >> >> instead of -
> >> >> ENODEV if the requested nvmem cell is not available.
> >>
> >> What do you mean with not available? Not yet available because of
> >> probe
> >> order?
> >
> > Ah, I was talking about there is no nvmem cell being used in my PHY
> > node, e.g.
> > no 'nvmem-cells' nor 'nvmem-cell-names' (set to 'io_impedance_ctrl').
> > That's
> > why of_property_match_string returns -EINVAL.
>
> Ahh I see. You mean ENOENT instead of ENODEV, right?
Yeah you are right here, ENOENT is the one missing.
> >> > Should we just assume #nvmem-cell-cells = <0> by default? I guess it's
> >> > a safe assumption.
> >>
> >> Actually, that's what patch 2/21 is for.
> >>
> >> Alexander, did you verify that the EINVAL is returned by
> >> of_parse_phandle_with_optional_args()?
> >
> > Yep.
> >
> > --8<--
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 1b61c8bf0de4..f2a85a31d039 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -1339,9 +1339,11 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> > device_node
> > *np, const char *id)
> >
> > if (id)
> >
> > index = of_property_match_string(np,
> >
> > "nvmem-cell-names", id);
> >
> > + pr_info("%s: index: %d\n", __func__, index);
> >
> > ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
> >
> > "#nvmem-cell-cells",
> > index, &cell_spec);
> >
> > + pr_info("%s: of_parse_phandle_with_optional_args: %d\n",
> > __func__,
> > ret);
> >
> > if (ret)
> >
> > return ERR_PTR(ret);
> >
> > --8<--
> >
> > Results in:
> >> [ 1.861896] of_nvmem_cell_get: index: -22
> >> [ 1.865934] of_nvmem_cell_get: of_parse_phandle_with_optional_args:
> >> -22
> >> [ 1.872595] TI DP83867 30be0000.ethernet-1:0e: error -EINVAL:
> >> failed to
> >
> > get nvmem cell io_impedance_ctrl
> >
> >> [ 2.402575] TI DP83867: probe of 30be0000.ethernet-1:0e failed with
> >> error
> >
> > -22
> >
> > So, the index is wrong in the first place, but this was no problem
> > until now.
>
> Thanks, could you try the following patch:
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 1b61c8bf0de4..1085abfcd9b1 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -1336,8 +1336,11 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> device_node *np, const char *id)
> int ret;
>
> /* if cell name exists, find index to the name */
> - if (id)
> + if (id) {
> index = of_property_match_string(np, "nvmem-cell-names",
> id);
> + if (index < 0)
> + return ERR_PTR(-ENOENT);
> + }
>
> ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
> "#nvmem-cell-cells",
>
> Before patch 6/21, the -EINVAL was passed as index to of_parse_phandle()
> which then returned NULL, which caused the nvmem core to return ENOENT.
> I have a vague memory, that I made sure, that
> of_parse_phandle_with_optional_args() will also propagate the
> wrong index to its return code. But now, it won't be converted
> to ENOENT.
Yes, this does the trick. Thanks
Best regards,
Alexander
Hi Michael/Miquel,
I had to revert Layout patches due to comments from Greg about Making
the layouts as built-in rather than modules, he is not ready to merge
them as it is.
His original comment,
"Why are we going back to "custom-built" kernel configurations? Why can
this not be a loadable module? Distros are now forced to enable these
layout and all kernels will have this dead code in the tree without any
choice in the matter?
That's not ok, these need to be auto-loaded based on the hardware
representation like any other kernel module. You can't force them to be
always present, sorry.
"
I have applied most of the patches except
nvmem: core: introduce NVMEM layouts
nvmem: core: add per-cell post processing
nvmem: core: allow to modify a cell before adding it
nvmem: imx-ocotp: replace global post processing with layouts
nvmem: cell: drop global cell_post_process
nvmem: core: provide own priv pointer in post process callback
nvmem: layouts: add sl28vpd layout
MAINTAINERS: add myself as sl28vpd nvmem layout driver
nvmem: layouts: Add ONIE tlv layout driver
MAINTAINERS: Add myself as ONIE tlv NVMEM layout maintainer
nvmem: core: return -ENOENT if nvmem cell is not found
nvmem: layouts: Fix spelling mistake "platforn" -> "platform"
dt-bindings: nvmem: Fix spelling mistake "platforn" -> "platform"
nvmem: core: fix nvmem_layout_get_match_data()
Please rebase your patches on top of nvmem-next once layouts are
converted to loadable modules.
thanks,
srini
On 03/01/2023 15:39, Miquel Raynal wrote:
> Hi Srinivas,
>
> [email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
>
>> This is now the third attempt to fetch the MAC addresses from the VPD
>> for the Kontron sl28 boards. Previous discussions can be found here:
>> https://lore.kernel.org/lkml/[email protected]/
>>
>>
>> NVMEM cells are typically added by board code or by the devicetree. But
>> as the cells get more complex, there is (valid) push back from the
>> devicetree maintainers to not put that handling in the devicetree.
>>
>> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
>> can add cells during runtime. That way it is possible to add more complex
>> cells than it is possible right now with the offset/length/bits
>> description in the device tree. For example, you can have post processing
>> for individual cells (think of endian swapping, or ethernet offset
>> handling).
>>
>> The imx-ocotp driver is the only user of the global post processing hook,
>> convert it to nvmem layouts and drop the global post pocessing hook.
>>
>> For now, the layouts are selected by the device tree. But the idea is
>> that also board files or other drivers could set a layout. Although no
>> code for that exists yet.
>>
>> Thanks to Miquel, the device tree bindings are already approved and merged.
>>
>> NVMEM layouts as modules?
>> While possible in principle, it doesn't make any sense because the NVMEM
>> core can't be compiled as a module. The layouts needs to be available at
>> probe time. (That is also the reason why they get registered with
>> subsys_initcall().) So if the NVMEM core would be a module, the layouts
>> could be modules, too.
>
> I believe this series still applies even though -rc1 (and -rc2) are out
> now, may we know if you consider merging it anytime soon or if there
> are still discrepancies in the implementation you would like to
> discuss? Otherwise I would really like to see this laying in -next a
> few weeks before being sent out to Linus, just in case.
>
> Thanks,
> Miquèl
Hi Srinivas,
+ Greg
[email protected] wrote on Mon, 6 Feb 2023 20:31:46 +0000:
> Hi Michael/Miquel,
>
> I had to revert Layout patches due to comments from Greg about Making the layouts as built-in rather than modules, he is not ready to merge them as it is.
Ok this is the second time I see something similar happening:
- maintainer or maintainers group doing the review/apply job and
sending to "upper" maintainer
- upper maintainer refusing for a "questionable" reason at this stage.
I am not saying the review is incorrect or anything. I'm just wondering
whether, for the second time, I am facing a fair situation, either
myself as a contributor or the intermediate maintainer who's being kind
of bypassed.
What I mean is: the review process has happened. Nothing was hidden,
this series has started leaving on the mailing lists more than two
years ago. The contribution process which has been in place for many
years asks the contributors to send new versions when the review
process leads to comments, which we did. Once the series has been
"accepted" it is expected that this series will be pulled during the
next merge window. If there is something else to fix, there are 6 to 8
long weeks where contributors' fixes are welcome. Why not letting us the
opportunity to use them? Why, for the second time, I am facing an
extremely urgent situation where I have to cancel all my commitments
just because a random comment has been made on a series which has been
standing still for months?
What I would expect instead, is a discussion on the cover letter of the
series where Michael explained why he did no choose to use modules in
the first place. If it appears that for some reason it is best to
enable NVMEM layouts as modules, we will send a timely series on top
of the current one to enable that particular case.
> >> NVMEM layouts as modules?
> >> While possible in principle, it doesn't make any sense because the NVMEM
> >> core can't be compiled as a module. The layouts needs to be available at
> >> probe time. (That is also the reason why they get registered with
> >> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> >> could be modules, too.
I know Michael is busy after the FOSDEM and so am I, so, Greg, would
you accept to take the PR as it is, participate to the discussion and
wait for an update?
Thanks,
Miquèl
> His original comment,
>
> "Why are we going back to "custom-built" kernel configurations? Why can
> this not be a loadable module? Distros are now forced to enable these
> layout and all kernels will have this dead code in the tree without any
> choice in the matter?
>
> That's not ok, these need to be auto-loaded based on the hardware
> representation like any other kernel module. You can't force them to be
> always present, sorry.
> "
>
> I have applied most of the patches except
>
> nvmem: core: introduce NVMEM layouts
> nvmem: core: add per-cell post processing
> nvmem: core: allow to modify a cell before adding it
> nvmem: imx-ocotp: replace global post processing with layouts
> nvmem: cell: drop global cell_post_process
> nvmem: core: provide own priv pointer in post process callback
> nvmem: layouts: add sl28vpd layout
> MAINTAINERS: add myself as sl28vpd nvmem layout driver
> nvmem: layouts: Add ONIE tlv layout driver
> MAINTAINERS: Add myself as ONIE tlv NVMEM layout maintainer
> nvmem: core: return -ENOENT if nvmem cell is not found
> nvmem: layouts: Fix spelling mistake "platforn" -> "platform"
> dt-bindings: nvmem: Fix spelling mistake "platforn" -> "platform"
> nvmem: core: fix nvmem_layout_get_match_data()
>
> Please rebase your patches on top of nvmem-next once layouts are converted to loadable modules.
>
> thanks,
> srini
>
>
>
> On 03/01/2023 15:39, Miquel Raynal wrote:
> > Hi Srinivas,
> >
> > [email protected] wrote on Tue, 6 Dec 2022 21:07:19 +0100:
> >
> >> This is now the third attempt to fetch the MAC addresses from the VPD
> >> for the Kontron sl28 boards. Previous discussions can be found here:
> >> https://lore.kernel.org/lkml/[email protected]/
> >>
> >>
> >> NVMEM cells are typically added by board code or by the devicetree. But
> >> as the cells get more complex, there is (valid) push back from the
> >> devicetree maintainers to not put that handling in the devicetree.
> >>
> >> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
> >> can add cells during runtime. That way it is possible to add more complex
> >> cells than it is possible right now with the offset/length/bits
> >> description in the device tree. For example, you can have post processing
> >> for individual cells (think of endian swapping, or ethernet offset
> >> handling).
> >>
> >> The imx-ocotp driver is the only user of the global post processing hook,
> >> convert it to nvmem layouts and drop the global post pocessing hook.
> >>
> >> For now, the layouts are selected by the device tree. But the idea is
> >> that also board files or other drivers could set a layout. Although no
> >> code for that exists yet.
> >>
> >> Thanks to Miquel, the device tree bindings are already approved and merged.
> >>
> >> NVMEM layouts as modules?
> >> While possible in principle, it doesn't make any sense because the NVMEM
> >> core can't be compiled as a module. The layouts needs to be available at
> >> probe time. (That is also the reason why they get registered with
> >> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> >> could be modules, too.
> >
> > I believe this series still applies even though -rc1 (and -rc2) are out
> > now, may we know if you consider merging it anytime soon or if there
> > are still discrepancies in the implementation you would like to
> > discuss? Otherwise I would really like to see this laying in -next a
> > few weeks before being sent out to Linus, just in case.
> >
> > Thanks,
> > Miquèl
On Mon, Feb 06, 2023 at 11:47:13PM +0100, Miquel Raynal wrote:
> Hi Srinivas,
>
> + Greg
>
> [email protected] wrote on Mon, 6 Feb 2023 20:31:46 +0000:
>
> > Hi Michael/Miquel,
> >
> > I had to revert Layout patches due to comments from Greg about Making the layouts as built-in rather than modules, he is not ready to merge them as it is.
>
> Ok this is the second time I see something similar happening:
> - maintainer or maintainers group doing the review/apply job and
> sending to "upper" maintainer
> - upper maintainer refusing for a "questionable" reason at this stage.
Only the second time? You've gotten lucky then :)
This happens all the time based on experience levels of reviewers and
just the very nature of how this whole process works. It's nothing
unusual and is good overall for the health of the project. In other
words, this is a a feature, not a bug.
> I am not saying the review is incorrect or anything. I'm just wondering
> whether, for the second time, I am facing a fair situation, either
> myself as a contributor or the intermediate maintainer who's being kind
> of bypassed.
>
> What I mean is: the review process has happened. Nothing was hidden,
> this series has started leaving on the mailing lists more than two
> years ago. The contribution process which has been in place for many
> years asks the contributors to send new versions when the review
> process leads to comments, which we did. Once the series has been
> "accepted" it is expected that this series will be pulled during the
> next merge window. If there is something else to fix, there are 6 to 8
> long weeks where contributors' fixes are welcome. Why not letting us the
> opportunity to use them? Why, for the second time, I am facing an
> extremely urgent situation where I have to cancel all my commitments
> just because a random comment has been made on a series which has been
> standing still for months?
There's no need to cancel anything, there are no deadlines in kernel
development and I am not asking for any sort of rush whatsoever.
So relax, take a week or two off (or month), and come back with an
updated patch series when you are ready. And feel free to cc: me on it
if you want my reviews (as I objected to these patches as-is) so that we
don't end up in the same situation (where one maintainer accepted
something, but the maintainer they sent it to rejected it.)
Again, there's no rush, and this is totally normal.
> What I would expect instead, is a discussion on the cover letter of the
> series where Michael explained why he did no choose to use modules in
> the first place. If it appears that for some reason it is best to
> enable NVMEM layouts as modules, we will send a timely series on top
> of the current one to enable that particular case.
Why not rework the existing series to handle this and not require
"fixups" at the end of the series? We don't normally create bugs and
then fix them up in the same patch set, as you know, so this shouldn't
be treated any differently.
> > >> NVMEM layouts as modules?
> > >> While possible in principle, it doesn't make any sense because the NVMEM
> > >> core can't be compiled as a module. The layouts needs to be available at
> > >> probe time. (That is also the reason why they get registered with
> > >> subsys_initcall().) So if the NVMEM core would be a module, the layouts
> > >> could be modules, too.
>
> I know Michael is busy after the FOSDEM and so am I, so, Greg, would
> you accept to take the PR as it is, participate to the discussion and
> wait for an update?
Kernel development doesn't work on "PR" :)
And no, I can't take these, as I don't agree with them, and I totally
imagine others will object for the same reason I did (and then they
would object to me, as the patches would be in my tree, as I am then
responsible for them.)
So send an updated version whenever you have the chance. Again, there's
no rush, deadline, or anything else here. Code is accepted when it is
ready and correct, not anytime earlier.
thanks,
greg k-h