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 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). You can also have cells which have no static offset, like the
ones in an u-boot environment. The last patches will convert the current
u-boot environment driver to a NVMEM layout and lifting the restriction
that it only works with mtd devices. But as it will change the required
compatible strings, it is marked as RFC for now. It also needs to have
its device tree schema update which is left out here.
For now, the layouts are selected by a specifc compatible string in a
device tree. E.g. the VPD on the kontron sl28 do (within a SPI flash node):
compatible = "kontron,sl28-vpd", "user-otp";
or if you'd use the u-boot environment (within an MTD patition):
compatible = "u-boot,env", "nvmem";
The "user-otp" (or "nvmem") will lead to a NVMEM device, the
"kontron,sl28-vpd" (or "u-boot,env") will then apply the specific layout
on top of the NVMEM device.
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 (14):
net: add helper eth_addr_add()
of: base: add of_parse_phandle_with_optional_args()
nvmem: core: add an index parameter to the cell
nvmem: core: drop the removal of the cells in nvmem_add_cells()
nvmem: core: add nvmem_add_one_cell()
nvmem: core: introduce NVMEM layouts
nvmem: core: add per-cell post processing
dt-bindings: mtd: relax the nvmem compatible string
dt-bindings: nvmem: add YAML schema for the sl28 vpd layout
nvmem: layouts: add sl28vpd layout
nvmem: core: export nvmem device size
nvmem: layouts: rewrite the u-boot-env driver as a NVMEM layout
nvmem: layouts: u-boot-env: add device node
arm64: dts: ls1028a: sl28: get MAC addresses from VPD
.../devicetree/bindings/mtd/mtd.yaml | 7 +-
.../nvmem/layouts/kontron,sl28-vpd.yaml | 52 +++++
.../fsl-ls1028a-kontron-kbox-a-230-ls.dts | 8 +
.../fsl-ls1028a-kontron-sl28-var1.dts | 2 +
.../fsl-ls1028a-kontron-sl28-var2.dts | 4 +
.../fsl-ls1028a-kontron-sl28-var4.dts | 2 +
.../freescale/fsl-ls1028a-kontron-sl28.dts | 13 ++
drivers/nvmem/Kconfig | 2 +
drivers/nvmem/Makefile | 1 +
drivers/nvmem/core.c | 183 +++++++++++----
drivers/nvmem/imx-ocotp.c | 4 +-
drivers/nvmem/layouts/Kconfig | 22 ++
drivers/nvmem/layouts/Makefile | 7 +
drivers/nvmem/layouts/sl28vpd.c | 144 ++++++++++++
drivers/nvmem/layouts/u-boot-env.c | 147 ++++++++++++
drivers/nvmem/u-boot-env.c | 218 ------------------
include/linux/etherdevice.h | 14 ++
include/linux/nvmem-consumer.h | 11 +
include/linux/nvmem-provider.h | 47 +++-
include/linux/of.h | 25 ++
20 files changed, 649 insertions(+), 264 deletions(-)
create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
create mode 100644 drivers/nvmem/layouts/Kconfig
create mode 100644 drivers/nvmem/layouts/Makefile
create mode 100644 drivers/nvmem/layouts/sl28vpd.c
create mode 100644 drivers/nvmem/layouts/u-boot-env.c
delete mode 100644 drivers/nvmem/u-boot-env.c
--
2.30.2
Now that it is finally possible to get the MAC addresses from the OTP
memory, use it to set the addresses of the network devices.
There are 8 reserved MAC addresses in total per board. Distribute them
as follows:
+----------+------+------+------+------+------+
| | var1 | var2 | var3 | var4 | kbox |
+----------+------+------+------+------+------+
| enetc #0 | +0 | | | +0 | +0 |
| enetc #1 | | | +0 | +1 | +1 |
| enetc #2 | rand | rand | rand | rand | rand |
| enetc #3 | | | | | |
| felix p0 | | +0 | | | +4 |
| felix p1 | | +1 | | | +5 |
| felix p2 | | | | | +6 |
| felix p3 | | | | | +7 |
| felix p4 | | | | | |
| felix p5 | | | | | |
+----------+------+------+------+------+------+
An empty cell means, the port is not available and thus doesn't need an
ethernet address.
Signed-off-by: Michael Walle <[email protected]>
---
.../freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts | 8 ++++++++
.../dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts | 2 ++
.../dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts | 4 ++++
.../dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts | 2 ++
.../boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts | 13 +++++++++++++
5 files changed, 29 insertions(+)
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
index 6b575efd84a7..b5c874c145d3 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-kbox-a-230-ls.dts
@@ -76,6 +76,8 @@ &mscc_felix_port0 {
managed = "in-band-status";
phy-handle = <&qsgmii_phy0>;
phy-mode = "qsgmii";
+ nvmem-cells = <&base_mac_address 4>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
@@ -84,6 +86,8 @@ &mscc_felix_port1 {
managed = "in-band-status";
phy-handle = <&qsgmii_phy1>;
phy-mode = "qsgmii";
+ nvmem-cells = <&base_mac_address 5>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
@@ -92,6 +96,8 @@ &mscc_felix_port2 {
managed = "in-band-status";
phy-handle = <&qsgmii_phy2>;
phy-mode = "qsgmii";
+ nvmem-cells = <&base_mac_address 6>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
@@ -100,6 +106,8 @@ &mscc_felix_port3 {
managed = "in-band-status";
phy-handle = <&qsgmii_phy3>;
phy-mode = "qsgmii";
+ nvmem-cells = <&base_mac_address 7>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts
index 7cd29ab970d9..1f34c7553459 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var1.dts
@@ -55,5 +55,7 @@ &enetc_port0 {
&enetc_port1 {
phy-handle = <&phy0>;
phy-mode = "rgmii-id";
+ nvmem-cells = <&base_mac_address 0>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
index 330e34f933a3..0ed0d2545922 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var2.dts
@@ -48,6 +48,8 @@ &mscc_felix_port0 {
managed = "in-band-status";
phy-handle = <&phy0>;
phy-mode = "sgmii";
+ nvmem-cells = <&base_mac_address 0>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
@@ -56,6 +58,8 @@ &mscc_felix_port1 {
managed = "in-band-status";
phy-handle = <&phy1>;
phy-mode = "sgmii";
+ nvmem-cells = <&base_mac_address 1>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts
index 9b5e92fb753e..a4421db3784e 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28-var4.dts
@@ -43,5 +43,7 @@ vddh: vddh-regulator {
&enetc_port1 {
phy-handle = <&phy1>;
phy-mode = "rgmii-id";
+ nvmem-cells = <&base_mac_address 1>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
diff --git a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
index 4ab17b984b03..72429b37a8b4 100644
--- a/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
+++ b/arch/arm64/boot/dts/freescale/fsl-ls1028a-kontron-sl28.dts
@@ -92,6 +92,8 @@ &enetc_port0 {
phy-handle = <&phy0>;
phy-mode = "sgmii";
managed = "in-band-status";
+ nvmem-cells = <&base_mac_address 0>;
+ nvmem-cell-names = "mac-address";
status = "okay";
};
@@ -154,6 +156,17 @@ partition@3e0000 {
label = "bootloader environment";
};
};
+
+ otp-1 {
+ compatible = "kontron,sl28-vpd", "user-otp";
+
+ serial_number: serial-number {
+ };
+
+ base_mac_address: base-mac-address {
+ #nvmem-cell-cells = <1>;
+ };
+ };
};
};
--
2.30.2
Instead of hardcoding the underlying access method mtd_read() and
duplicating all the error handling, rewrite the driver as a nvmem
layout which just uses nvmem_device_read() and thus works with any
NVMEM device.
But because this is now not a device anymore, the compatible string
will have to be changed so the device will still be probed:
compatible = "u-boot,env";
to
compatible = "u-boot,env", "nvmem-cells";
"nvmem-cells" will tell the mtd layer to register a nvmem_device().
"u-boot,env" will tell the NVMEM that it should apply the u-boot
environment layout to the NVMEM device.
Signed-off-by: Michael Walle <[email protected]>
---
drivers/nvmem/layouts/Kconfig | 8 ++
drivers/nvmem/layouts/Makefile | 1 +
drivers/nvmem/layouts/u-boot-env.c | 143 +++++++++++++++++++
drivers/nvmem/u-boot-env.c | 218 -----------------------------
4 files changed, 152 insertions(+), 218 deletions(-)
create mode 100644 drivers/nvmem/layouts/u-boot-env.c
delete mode 100644 drivers/nvmem/u-boot-env.c
diff --git a/drivers/nvmem/layouts/Kconfig b/drivers/nvmem/layouts/Kconfig
index 75082f6b471d..3db0c139a8b7 100644
--- a/drivers/nvmem/layouts/Kconfig
+++ b/drivers/nvmem/layouts/Kconfig
@@ -11,4 +11,12 @@ config NVMEM_LAYOUT_SL28_VPD
If unsure, say N.
+config NVMEM_LAYOUT_U_BOOT_ENV
+ bool "U-Boot environment support"
+ select CRC32
+ help
+ Say Y here if you want to support u-boot's environment.
+
+ If unsure, say N.
+
endmenu
diff --git a/drivers/nvmem/layouts/Makefile b/drivers/nvmem/layouts/Makefile
index fc617b9e87d0..dae93fff2b85 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_U_BOOT_ENV) += u-boot-env.o
diff --git a/drivers/nvmem/layouts/u-boot-env.c b/drivers/nvmem/layouts/u-boot-env.c
new file mode 100644
index 000000000000..40b7e9a778c4
--- /dev/null
+++ b/drivers/nvmem/layouts/u-boot-env.c
@@ -0,0 +1,143 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2022 Rafał Miłecki <[email protected]>
+ */
+
+#include <linux/crc32.h>
+#include <linux/dev_printk.h>
+#include <linux/mod_devicetable.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/slab.h>
+
+enum u_boot_env_format {
+ U_BOOT_FORMAT_SINGLE,
+ U_BOOT_FORMAT_REDUNDANT,
+};
+
+struct u_boot_env_image_single {
+ __le32 crc32;
+ u8 data[];
+} __packed;
+
+struct u_boot_env_image_redundant {
+ __le32 crc32;
+ u8 mark;
+ u8 data[];
+} __packed;
+
+static int u_boot_env_add_cells(struct nvmem_device *nvmem, uint8_t *buf,
+ size_t data_offset, size_t data_len)
+{
+ struct nvmem_cell_info info = {0};
+ char *data = buf + data_offset;
+ char *var, *value, *eq;
+ int err;
+
+ for (var = data;
+ var < data + data_len && *var;
+ var = value + strlen(value) + 1) {
+ eq = strchr(var, '=');
+ if (!eq)
+ break;
+ *eq = '\0';
+ value = eq + 1;
+
+ info.name = var;
+ info.offset = data_offset + value - data;
+ info.bytes = strlen(value);
+
+ err = nvmem_add_one_cell(nvmem, &info);
+ if (err)
+ return err;
+ }
+
+ return 0;
+}
+
+static int u_boot_add_cells(struct device *dev, struct nvmem_device *nvmem,
+ struct nvmem_layout *layout)
+{
+ size_t size = nvmem_device_size(nvmem);
+ enum u_boot_env_format format;
+ size_t crc32_data_offset;
+ size_t crc32_data_len;
+ size_t crc32_offset;
+ size_t data_offset;
+ size_t data_len;
+ u32 crc32;
+ u32 calc;
+ u8 *buf;
+ int err;
+
+ format = (uintptr_t)nvmem_layout_get_match_data(nvmem, layout);
+
+ buf = kzalloc(size, GFP_KERNEL);
+ if (!buf) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+
+ err = nvmem_device_read(nvmem, 0, size, buf);
+ if (err < 0) {
+ dev_err(dev, "Failed to read from nvmem device (%pe)\n",
+ ERR_PTR(err));
+ goto err_kfree;
+ } else if (err != size) {
+ dev_err(dev, "Short read from nvmem device.\n");
+ err = -EIO;
+ goto err_kfree;
+ }
+
+ switch (format) {
+ case U_BOOT_FORMAT_SINGLE:
+ crc32_offset = offsetof(struct u_boot_env_image_single, crc32);
+ crc32_data_offset = offsetof(struct u_boot_env_image_single, data);
+ data_offset = offsetof(struct u_boot_env_image_single, data);
+ break;
+ case U_BOOT_FORMAT_REDUNDANT:
+ crc32_offset = offsetof(struct u_boot_env_image_redundant, crc32);
+ crc32_data_offset = offsetof(struct u_boot_env_image_redundant, mark);
+ data_offset = offsetof(struct u_boot_env_image_redundant, data);
+ break;
+ }
+ crc32 = le32_to_cpu(*(u32 *)(buf + crc32_offset));
+ crc32_data_len = size - crc32_data_offset;
+ data_len = size - data_offset;
+
+ calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
+ if (calc != crc32) {
+ dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 0x%08x)\n", calc, crc32);
+ err = -EINVAL;
+ goto err_kfree;
+ }
+
+ buf[size - 1] = '\0';
+ err = u_boot_env_add_cells(nvmem, buf, data_offset, data_len);
+ if (err)
+ dev_err(dev, "Failed to add cells: %d\n", err);
+
+err_kfree:
+ kfree(buf);
+err_out:
+ return err;
+}
+
+static const struct of_device_id u_boot_env_of_match_table[] = {
+ { .compatible = "u-boot,env", .data = (void *)U_BOOT_FORMAT_SINGLE, },
+ { .compatible = "u-boot,env-redundant-bool", .data = (void *)U_BOOT_FORMAT_REDUNDANT, },
+ { .compatible = "u-boot,env-redundant-count", .data = (void *)U_BOOT_FORMAT_REDUNDANT, },
+ {},
+};
+
+static struct nvmem_layout u_boot_env_layout = {
+ .name = "u_boot_env",
+ .of_match_table = u_boot_env_of_match_table,
+ .add_cells = u_boot_add_cells,
+};
+
+static int __init u_boot_env_init(void)
+{
+ return nvmem_register_layout(&u_boot_env_layout);
+}
+subsys_initcall(u_boot_env_init);
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
deleted file mode 100644
index 9b9abfb8f187..000000000000
--- a/drivers/nvmem/u-boot-env.c
+++ /dev/null
@@ -1,218 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0-only
-/*
- * Copyright (C) 2022 Rafał Miłecki <[email protected]>
- */
-
-#include <linux/crc32.h>
-#include <linux/mod_devicetable.h>
-#include <linux/module.h>
-#include <linux/mtd/mtd.h>
-#include <linux/nvmem-consumer.h>
-#include <linux/nvmem-provider.h>
-#include <linux/of_device.h>
-#include <linux/platform_device.h>
-#include <linux/slab.h>
-
-enum u_boot_env_format {
- U_BOOT_FORMAT_SINGLE,
- U_BOOT_FORMAT_REDUNDANT,
-};
-
-struct u_boot_env {
- struct device *dev;
- enum u_boot_env_format format;
-
- struct mtd_info *mtd;
-
- /* Cells */
- struct nvmem_cell_info *cells;
- int ncells;
-};
-
-struct u_boot_env_image_single {
- __le32 crc32;
- uint8_t data[];
-} __packed;
-
-struct u_boot_env_image_redundant {
- __le32 crc32;
- u8 mark;
- uint8_t data[];
-} __packed;
-
-static int u_boot_env_read(void *context, unsigned int offset, void *val,
- size_t bytes)
-{
- struct u_boot_env *priv = context;
- struct device *dev = priv->dev;
- size_t bytes_read;
- int err;
-
- err = mtd_read(priv->mtd, offset, bytes, &bytes_read, val);
- if (err && !mtd_is_bitflip(err)) {
- dev_err(dev, "Failed to read from mtd: %d\n", err);
- return err;
- }
-
- if (bytes_read != bytes) {
- dev_err(dev, "Failed to read %zu bytes\n", bytes);
- return -EIO;
- }
-
- return 0;
-}
-
-static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf,
- size_t data_offset, size_t data_len)
-{
- struct device *dev = priv->dev;
- char *data = buf + data_offset;
- char *var, *value, *eq;
- int idx;
-
- priv->ncells = 0;
- for (var = data; var < data + data_len && *var; var += strlen(var) + 1)
- priv->ncells++;
-
- priv->cells = devm_kcalloc(dev, priv->ncells, sizeof(*priv->cells), GFP_KERNEL);
- if (!priv->cells)
- return -ENOMEM;
-
- for (var = data, idx = 0;
- var < data + data_len && *var;
- var = value + strlen(value) + 1, idx++) {
- eq = strchr(var, '=');
- if (!eq)
- break;
- *eq = '\0';
- value = eq + 1;
-
- priv->cells[idx].name = devm_kstrdup(dev, var, GFP_KERNEL);
- if (!priv->cells[idx].name)
- return -ENOMEM;
- priv->cells[idx].offset = data_offset + value - data;
- priv->cells[idx].bytes = strlen(value);
- }
-
- if (WARN_ON(idx != priv->ncells))
- priv->ncells = idx;
-
- return 0;
-}
-
-static int u_boot_env_parse(struct u_boot_env *priv)
-{
- struct device *dev = priv->dev;
- size_t crc32_data_offset;
- size_t crc32_data_len;
- size_t crc32_offset;
- size_t data_offset;
- size_t data_len;
- uint32_t crc32;
- uint32_t calc;
- size_t bytes;
- uint8_t *buf;
- int err;
-
- buf = kcalloc(1, priv->mtd->size, GFP_KERNEL);
- if (!buf) {
- err = -ENOMEM;
- goto err_out;
- }
-
- err = mtd_read(priv->mtd, 0, priv->mtd->size, &bytes, buf);
- if ((err && !mtd_is_bitflip(err)) || bytes != priv->mtd->size) {
- dev_err(dev, "Failed to read from mtd: %d\n", err);
- goto err_kfree;
- }
-
- switch (priv->format) {
- case U_BOOT_FORMAT_SINGLE:
- crc32_offset = offsetof(struct u_boot_env_image_single, crc32);
- crc32_data_offset = offsetof(struct u_boot_env_image_single, data);
- data_offset = offsetof(struct u_boot_env_image_single, data);
- break;
- case U_BOOT_FORMAT_REDUNDANT:
- crc32_offset = offsetof(struct u_boot_env_image_redundant, crc32);
- crc32_data_offset = offsetof(struct u_boot_env_image_redundant, mark);
- data_offset = offsetof(struct u_boot_env_image_redundant, data);
- break;
- }
- crc32 = le32_to_cpu(*(uint32_t *)(buf + crc32_offset));
- crc32_data_len = priv->mtd->size - crc32_data_offset;
- data_len = priv->mtd->size - data_offset;
-
- calc = crc32(~0, buf + crc32_data_offset, crc32_data_len) ^ ~0L;
- if (calc != crc32) {
- dev_err(dev, "Invalid calculated CRC32: 0x%08x (expected: 0x%08x)\n", calc, crc32);
- err = -EINVAL;
- goto err_kfree;
- }
-
- buf[priv->mtd->size - 1] = '\0';
- err = u_boot_env_add_cells(priv, buf, data_offset, data_len);
- if (err)
- dev_err(dev, "Failed to add cells: %d\n", err);
-
-err_kfree:
- kfree(buf);
-err_out:
- return err;
-}
-
-static int u_boot_env_probe(struct platform_device *pdev)
-{
- struct nvmem_config config = {
- .name = "u-boot-env",
- .reg_read = u_boot_env_read,
- };
- struct device *dev = &pdev->dev;
- struct device_node *np = dev->of_node;
- struct u_boot_env *priv;
- int err;
-
- priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
- if (!priv)
- return -ENOMEM;
- priv->dev = dev;
-
- priv->format = (uintptr_t)of_device_get_match_data(dev);
-
- priv->mtd = of_get_mtd_device_by_node(np);
- if (IS_ERR(priv->mtd)) {
- dev_err_probe(dev, PTR_ERR(priv->mtd), "Failed to get %pOF MTD\n", np);
- return PTR_ERR(priv->mtd);
- }
-
- err = u_boot_env_parse(priv);
- if (err)
- return err;
-
- config.dev = dev;
- config.cells = priv->cells;
- config.ncells = priv->ncells;
- config.priv = priv;
- config.size = priv->mtd->size;
-
- return PTR_ERR_OR_ZERO(devm_nvmem_register(dev, &config));
-}
-
-static const struct of_device_id u_boot_env_of_match_table[] = {
- { .compatible = "u-boot,env", .data = (void *)U_BOOT_FORMAT_SINGLE, },
- { .compatible = "u-boot,env-redundant-bool", .data = (void *)U_BOOT_FORMAT_REDUNDANT, },
- { .compatible = "u-boot,env-redundant-count", .data = (void *)U_BOOT_FORMAT_REDUNDANT, },
- {},
-};
-
-static struct platform_driver u_boot_env_driver = {
- .probe = u_boot_env_probe,
- .driver = {
- .name = "u_boot_env",
- .of_match_table = u_boot_env_of_match_table,
- },
-};
-module_platform_driver(u_boot_env_driver);
-
-MODULE_AUTHOR("Rafał Miłecki");
-MODULE_LICENSE("GPL");
-MODULE_DEVICE_TABLE(of, u_boot_env_of_match_table);
--
2.30.2
The "user-otp" and "factory-otp" compatible string just depicts a
generic NVMEM device. But an actual device tree node might as well
contain a more specific compatible string. Make it possible to add
more specific binding elsewere and just match part of the compatibles
here.
Signed-off-by: Michael Walle <[email protected]>
---
Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml b/Documentation/devicetree/bindings/mtd/mtd.yaml
index 376b679cfc70..0291e439b6a6 100644
--- a/Documentation/devicetree/bindings/mtd/mtd.yaml
+++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
@@ -33,9 +33,10 @@ patternProperties:
properties:
compatible:
- enum:
- - user-otp
- - factory-otp
+ contains:
+ enum:
+ - user-otp
+ - factory-otp
required:
- compatible
--
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]>
---
drivers/nvmem/core.c | 58 ++++++++++++++++++++--------------
include/linux/nvmem-provider.h | 5 +++
2 files changed, 39 insertions(+), 24 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index be38e62fd190..3dfd149374a8 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -501,6 +501,35 @@ 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;
+}
+
/**
* nvmem_add_cells() - Add cell information to an nvmem device
*
@@ -514,34 +543,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 8f964b394292..e710404959e7 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -138,6 +138,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)
@@ -155,6 +158,8 @@ 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) {}
#endif /* CONFIG_NVMEM */
#endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
--
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]>
---
drivers/nvmem/layouts/Kconfig | 9 ++
drivers/nvmem/layouts/Makefile | 2 +
drivers/nvmem/layouts/sl28vpd.c | 144 ++++++++++++++++++++++++++++++++
3 files changed, 155 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..e6ec673aa58a
--- /dev/null
+++ b/drivers/nvmem/layouts/sl28vpd.c
@@ -0,0 +1,144 @@
+// 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),
+ .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 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;
+
+ 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.post_process = pinfo->post_process;
+ info.np = of_get_child_by_name(dev->of_node, pinfo->name);
+
+ ret = nvmem_add_one_cell(nvmem, &info);
+ if (ret)
+ return ret;
+ }
+
+ 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_register_layout(&sl28vpd_layout);
+}
+subsys_initcall(sl28vpd_init);
--
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]>
---
include/linux/etherdevice.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h
index 92b10e67d5f8..d1be5ca4d5cb 100644
--- a/include/linux/etherdevice.h
+++ b/include/linux/etherdevice.h
@@ -485,6 +485,20 @@ static inline void eth_addr_inc(u8 *addr)
u64_to_ether_addr(u, addr);
}
+/**
+ * eth_addr_add() - Add (or subtract) and 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
Am 2022-08-28 16:04, schrieb Rafał Miłecki:
> On 25.08.2022 23:44, Michael Walle wrote:
>> Instead of hardcoding the underlying access method mtd_read() and
>> duplicating all the error handling, rewrite the driver as a nvmem
>> layout which just uses nvmem_device_read() and thus works with any
>> NVMEM device.
>>
>> But because this is now not a device anymore, the compatible string
>> will have to be changed so the device will still be probed:
>> compatible = "u-boot,env";
>> to
>> compatible = "u-boot,env", "nvmem-cells";
>>
>> "nvmem-cells" will tell the mtd layer to register a nvmem_device().
>> "u-boot,env" will tell the NVMEM that it should apply the u-boot
>> environment layout to the NVMEM device.
>
> That's fishy but maybe we can ignore backward compatibility at
> point.
As mentioned in the cover letter, this is why this is an RFC. I
didn't see any users in the device tree, nor can I see how this
would have been used anyway, because you cannot find a cell by
its device tree node, because none is registered. So maybe we
can still change the compatible string.
-michael
> Still you need to update DT binding.
On 25.08.2022 23:44, Michael Walle wrote:
> Instead of hardcoding the underlying access method mtd_read() and
> duplicating all the error handling, rewrite the driver as a nvmem
> layout which just uses nvmem_device_read() and thus works with any
> NVMEM device.
>
> But because this is now not a device anymore, the compatible string
> will have to be changed so the device will still be probed:
> compatible = "u-boot,env";
> to
> compatible = "u-boot,env", "nvmem-cells";
>
> "nvmem-cells" will tell the mtd layer to register a nvmem_device().
> "u-boot,env" will tell the NVMEM that it should apply the u-boot
> environment layout to the NVMEM device.
That's fishy but maybe we can ignore backward compatibility at
point.
Still you need to update DT binding.
On 25.08.2022 23:44, Michael Walle wrote:
> 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.
I dropped the ball waiting for Rob's reponse in the
[PATCH 0/2] dt-bindings: nvmem: support describing cells
https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
Before we go any further can we have a clear answer from Rob (or
Krzysztof now too?):
Is there any point in having bindings like:
compatible = "mac-address";
for NVMEM cells nodes? So systems (Linux, U-Boot) can handle them in a
more generic way?
Or do we prefer more conditional drivers code (or layouts code as in
this Michael's proposal) that will handle cells properly based on their
names?
I'm not arguing for any solution. I just want to make sure we choose the
right way to proceed.
> Therefore, introduce NVMEM layouts. They operate on the NVMEM device and
> can add cells during runtime. That way it is possible to add 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). You can also have cells which have no static offset, like the
> ones in an u-boot environment. The last patches will convert the current
> u-boot environment driver to a NVMEM layout and lifting the restriction
> that it only works with mtd devices. But as it will change the required
> compatible strings, it is marked as RFC for now. It also needs to have
> its device tree schema update which is left out here.
So do I get it right that we want to have:
1. NVMEM drivers for providing I/O access to NVMEM devices
2. NVMEM layouts for parsing & NVMEM cells and translating their content
?
I guess it sounds good and seems to be a clean solution.
One thing I believe you need to handle is replacing "cell_post_process"
callback with your layout thing.
I find it confusing to have
1. cell_post_process() CB at NVMEM device level
2. post_process() CB at NVMEM cell level
> For now, the layouts are selected by a specifc compatible string in a
> device tree. E.g. the VPD on the kontron sl28 do (within a SPI flash node):
> compatible = "kontron,sl28-vpd", "user-otp";
> or if you'd use the u-boot environment (within an MTD patition):
> compatible = "u-boot,env", "nvmem";
>
> The "user-otp" (or "nvmem") will lead to a NVMEM device, the
> "kontron,sl28-vpd" (or "u-boot,env") will then apply the specific layout
> on top of the NVMEM device.
>
> 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 (14):
> net: add helper eth_addr_add()
> of: base: add of_parse_phandle_with_optional_args()
> nvmem: core: add an index parameter to the cell
> nvmem: core: drop the removal of the cells in nvmem_add_cells()
> nvmem: core: add nvmem_add_one_cell()
> nvmem: core: introduce NVMEM layouts
> nvmem: core: add per-cell post processing
> dt-bindings: mtd: relax the nvmem compatible string
> dt-bindings: nvmem: add YAML schema for the sl28 vpd layout
> nvmem: layouts: add sl28vpd layout
> nvmem: core: export nvmem device size
> nvmem: layouts: rewrite the u-boot-env driver as a NVMEM layout
> nvmem: layouts: u-boot-env: add device node
> arm64: dts: ls1028a: sl28: get MAC addresses from VPD
>
> .../devicetree/bindings/mtd/mtd.yaml | 7 +-
> .../nvmem/layouts/kontron,sl28-vpd.yaml | 52 +++++
> .../fsl-ls1028a-kontron-kbox-a-230-ls.dts | 8 +
> .../fsl-ls1028a-kontron-sl28-var1.dts | 2 +
> .../fsl-ls1028a-kontron-sl28-var2.dts | 4 +
> .../fsl-ls1028a-kontron-sl28-var4.dts | 2 +
> .../freescale/fsl-ls1028a-kontron-sl28.dts | 13 ++
> drivers/nvmem/Kconfig | 2 +
> drivers/nvmem/Makefile | 1 +
> drivers/nvmem/core.c | 183 +++++++++++----
> drivers/nvmem/imx-ocotp.c | 4 +-
> drivers/nvmem/layouts/Kconfig | 22 ++
> drivers/nvmem/layouts/Makefile | 7 +
> drivers/nvmem/layouts/sl28vpd.c | 144 ++++++++++++
> drivers/nvmem/layouts/u-boot-env.c | 147 ++++++++++++
> drivers/nvmem/u-boot-env.c | 218 ------------------
> include/linux/etherdevice.h | 14 ++
> include/linux/nvmem-consumer.h | 11 +
> include/linux/nvmem-provider.h | 47 +++-
> include/linux/of.h | 25 ++
> 20 files changed, 649 insertions(+), 264 deletions(-)
> create mode 100644 Documentation/devicetree/bindings/nvmem/layouts/kontron,sl28-vpd.yaml
> create mode 100644 drivers/nvmem/layouts/Kconfig
> create mode 100644 drivers/nvmem/layouts/Makefile
> create mode 100644 drivers/nvmem/layouts/sl28vpd.c
> create mode 100644 drivers/nvmem/layouts/u-boot-env.c
> delete mode 100644 drivers/nvmem/u-boot-env.c
Hi,
Am 2022-08-28 17:05, schrieb Rafał Miłecki:
> On 25.08.2022 23:44, Michael Walle wrote:
>> 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.
>
> I dropped the ball waiting for Rob's reponse in the
> [PATCH 0/2] dt-bindings: nvmem: support describing cells
> https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
>
> Before we go any further can we have a clear answer from Rob (or
> Krzysztof now too?):
>
>
> Is there any point in having bindings like:
>
> compatible = "mac-address";
>
> for NVMEM cells nodes? So systems (Linux, U-Boot) can handle them in a
> more generic way?
>
>
> Or do we prefer more conditional drivers code (or layouts code as in
> this Michael's proposal) that will handle cells properly based on their
> names?
What do you mean by "based on their names?".
> I'm not arguing for any solution. I just want to make sure we choose
> the
> right way to proceed.
With the 'compatible = "mac-address"', how would you detect what kind
of transformation you need to apply? You could guess ascii, yes. But
swapping bytes? You cannot guess that. So you'd need additional
information
coming from the device tree. But Rob was quite clear that this shouldn't
be in the device tree:
| I still don't think trying to encode transformations of data into the
DT
| is right approach.
https://lore.kernel.org/linux-devicetree/[email protected]/
He also mention that the compatible should be on the nvmem device level
and should use specific compatible strings:
https://lore.kernel.org/linux-devicetree/CAL_JsqL55mZJ6jUyQACer2pKMNDV08-FgwBREsJVgitnuF18Cg@mail.gmail.com/
And IMHO that makes sense, because it matches the hardware and not any
NVMEM cells which is the *content* of a memory device.
And as you see in the sl28vpd layout, it allows you to do much more,
like
checking for integrity, and make it future proof by supporting different
versions of this sl28vpd layout.
What if you use it with the u-boot,env? You wouldn't need it because
u-boot,env will already know how to interpret it as an ascii string
(and it also know the offset). In this sense, u-boot,env is already a
compatible string describing the content of a NVMEM device (and the
compatible string is at the device level).
>> Therefore, introduce NVMEM layouts. They operate on the NVMEM device
>> and
>> can add cells during runtime. That way it is possible to add 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). You can also have cells which have no static offset, like
>> the
>> ones in an u-boot environment. The last patches will convert the
>> current
>> u-boot environment driver to a NVMEM layout and lifting the
>> restriction
>> that it only works with mtd devices. But as it will change the
>> required
>> compatible strings, it is marked as RFC for now. It also needs to have
>> its device tree schema update which is left out here.
>
> So do I get it right that we want to have:
>
> 1. NVMEM drivers for providing I/O access to NVMEM devices
> 2. NVMEM layouts for parsing & NVMEM cells and translating their
> content
> ?
Correct.
> I guess it sounds good and seems to be a clean solution.
Good to hear :)
> One thing I believe you need to handle is replacing "cell_post_process"
> callback with your layout thing.
>
> I find it confusing to have
> 1. cell_post_process() CB at NVMEM device level
> 2. post_process() CB at NVMEM cell level
What is wrong with having a callback at both levels?
Granted, in this particular case (it is just used at one place), I still
think that it is the wrong approach to add this transformation in the
driver (in this particular case). The driver is supposed to give you
access to the SoC's fuse box, but it will magically change the content
of a cell if the nvmem consumer named this cell "mac-address" (which
you also found confusing the last time and I do too!).
The driver itself doesn't add any cells on its own, so I cannot register
a .post_process hook there. Therefore, you'd need that post_process hook
on every cell, which is equivalent to have a post_process hook at
device level.
Unless you have a better idea. I'll leave that up to NXP to fix that (or
leave it like that).
-michael
Thanks Michael for the work.
On 29/08/2022 09:22, Michael Walle wrote:
>
>> One thing I believe you need to handle is replacing "cell_post_process"
>> callback with your layout thing.
>>
>> I find it confusing to have
>> 1. cell_post_process() CB at NVMEM device level
>> 2. post_process() CB at NVMEM cell level
>
> What is wrong with having a callback at both levels?
we should converge this tbh, its more than one code paths to deal with
similar usecases.
I have put down some thoughts in "[PATCH v1 06/14] nvmem: core:
introduce NVMEM layouts" and "[PATCH v1 07/14] nvmem: core: add per-cell
post processing" review.
--srini
>
> Granted, in this particular case (it is just used at one place), I still
> think that it is the wrong approach to add this transformation in the
> driver (in this particular case). The driver is supposed to give you
> access to the SoC's fuse box, but it will magically change the content
> of a cell if the nvmem consumer named this cell "mac-address" (which
> you also found confusing the last time and I do too!).
>
> The driver itself doesn't add any cells on its own, so I cannot register
> a .post_process hook there. Therefore, you'd need that post_process hook
> on every cell, which is equivalent to have a post_process hook at
> device level.
>
> Unless you have a better idea. I'll leave that up to NXP to fix that (or
> leave it like that).
>
> -michael
On 26/08/2022 00:44, Michael Walle wrote:
> The "user-otp" and "factory-otp" compatible string just depicts a
> generic NVMEM device. But an actual device tree node might as well
> contain a more specific compatible string. Make it possible to add
> more specific binding elsewere and just match part of the compatibles
typo: elsewhere
> here.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml b/Documentation/devicetree/bindings/mtd/mtd.yaml
> index 376b679cfc70..0291e439b6a6 100644
> --- a/Documentation/devicetree/bindings/mtd/mtd.yaml
> +++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
> @@ -33,9 +33,10 @@ patternProperties:
>
> properties:
> compatible:
> - enum:
> - - user-otp
> - - factory-otp
> + contains:
> + enum:
> + - user-otp
> + - factory-otp
This does not work in the "elsewhere" place. You need to use similar
approach as we do for syscon or primecell.
>
> required:
> - compatible
Best regards,
Krzysztof
On 28/08/2022 18:05, Rafał Miłecki wrote:
> On 25.08.2022 23:44, Michael Walle wrote:
>> 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.
>
> I dropped the ball waiting for Rob's reponse in the
> [PATCH 0/2] dt-bindings: nvmem: support describing cells
> https://lore.kernel.org/linux-arm-kernel/[email protected]/T/
>
> Before we go any further can we have a clear answer from Rob (or
> Krzysztof now too?):
>
>
> Is there any point in having bindings like:
>
> compatible = "mac-address";
>
> for NVMEM cells nodes? So systems (Linux, U-Boot) can handle them in a
> more generic way?
I think Rob is already in the subject, but I wonder how would that work
for U-Boot? You might have multiple cards, so how does this matching
would work? IOW, what problem would it solve comparing to existing
solution (alias to ethernet device with mac-address field)?
Best regards,
Krzysztof
On 31/08/2022 10:48, Michael Walle wrote:
> Am 2022-08-31 09:37, schrieb Krzysztof Kozlowski:
>> On 26/08/2022 00:44, Michael Walle wrote:
>>> The "user-otp" and "factory-otp" compatible string just depicts a
>>> generic NVMEM device. But an actual device tree node might as well
>>> contain a more specific compatible string. Make it possible to add
>>> more specific binding elsewere and just match part of the compatibles
>>
>> typo: elsewhere
>>
>>> here.
>>>
>>> Signed-off-by: Michael Walle <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
>>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml
>>> b/Documentation/devicetree/bindings/mtd/mtd.yaml
>>> index 376b679cfc70..0291e439b6a6 100644
>>> --- a/Documentation/devicetree/bindings/mtd/mtd.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
>>> @@ -33,9 +33,10 @@ patternProperties:
>>>
>>> properties:
>>> compatible:
>>> - enum:
>>> - - user-otp
>>> - - factory-otp
>>> + contains:
>>> + enum:
>>> + - user-otp
>>> + - factory-otp
>>
>> This does not work in the "elsewhere" place. You need to use similar
>> approach as we do for syscon or primecell.
>
> I'm a bit confused. Looking at
> Documentation/devicetree/bindings/arm/primecell.yaml
> it is done in the same way as this binding.
Yes. primecell is like your mtd here. And how are other places with
primcell (like other places with user-otp) done?
>
> Whereas, the syscon use a "select:" on top of it. I'm
> pretty sure, I've tested it without the select and the
> validator picked up the constraints.
>
> Could you elaborate on what is wrong here? Select missing?
You got warning from Rob, so run tests. I think you will see the errors,
just like bot reported them.
Best regards,
Krzysztof
Am 2022-08-31 09:37, schrieb Krzysztof Kozlowski:
> On 26/08/2022 00:44, Michael Walle wrote:
>> The "user-otp" and "factory-otp" compatible string just depicts a
>> generic NVMEM device. But an actual device tree node might as well
>> contain a more specific compatible string. Make it possible to add
>> more specific binding elsewere and just match part of the compatibles
>
> typo: elsewhere
>
>> here.
>>
>> Signed-off-by: Michael Walle <[email protected]>
>> ---
>> Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/mtd.yaml
>> b/Documentation/devicetree/bindings/mtd/mtd.yaml
>> index 376b679cfc70..0291e439b6a6 100644
>> --- a/Documentation/devicetree/bindings/mtd/mtd.yaml
>> +++ b/Documentation/devicetree/bindings/mtd/mtd.yaml
>> @@ -33,9 +33,10 @@ patternProperties:
>>
>> properties:
>> compatible:
>> - enum:
>> - - user-otp
>> - - factory-otp
>> + contains:
>> + enum:
>> + - user-otp
>> + - factory-otp
>
> This does not work in the "elsewhere" place. You need to use similar
> approach as we do for syscon or primecell.
I'm a bit confused. Looking at
Documentation/devicetree/bindings/arm/primecell.yaml
it is done in the same way as this binding.
Whereas, the syscon use a "select:" on top of it. I'm
pretty sure, I've tested it without the select and the
validator picked up the constraints.
Could you elaborate on what is wrong here? Select missing?
-michael
On Thu, Aug 25, 2022 at 11:44:17PM +0200, Michael Walle wrote:
> The "user-otp" and "factory-otp" compatible string just depicts a
> generic NVMEM device. But an actual device tree node might as well
> contain a more specific compatible string. Make it possible to add
> more specific binding elsewere and just match part of the compatibles
> here.
>
> Signed-off-by: Michael Walle <[email protected]>
> ---
> Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
> 1 file changed, 4 insertions(+), 3 deletions(-)
In hindsight it looks like we are mixing 2 different purposes of 'which
instance is this' and 'what is this'. 'compatible' is supposed to be the
latter.
Maybe there's a better way to handle user/factory? There's a similar
need with partitions for A/B or factory/update.
Rob
Am 2022-08-31 23:48, schrieb Rob Herring:
> On Thu, Aug 25, 2022 at 11:44:17PM +0200, Michael Walle wrote:
>> The "user-otp" and "factory-otp" compatible string just depicts a
>> generic NVMEM device. But an actual device tree node might as well
>> contain a more specific compatible string. Make it possible to add
>> more specific binding elsewere and just match part of the compatibles
>> here.
>>
>> Signed-off-by: Michael Walle <[email protected]>
>> ---
>> Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
>> 1 file changed, 4 insertions(+), 3 deletions(-)
>
> In hindsight it looks like we are mixing 2 different purposes of 'which
> instance is this' and 'what is this'. 'compatible' is supposed to be
> the
> latter.
>
> Maybe there's a better way to handle user/factory? There's a similar
> need with partitions for A/B or factory/update.
I'm not sure I understand what you mean. It has nothing to with
user and factory provisionings.
SPI flashes have a user programmable and a factory programmable
area, some have just one of them. Whereas with A/B you (as in the
user or the board manufacturer) defines an area within a memory device
to be either slot A or slot B. But here the flash dictates what's
factory and what's user storage. It's in the datasheet.
HTH
-michael
Hi netdev maintainers,
Am 2022-08-25 23:44, schrieb Michael Walle:
> 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]>
Would it be possible to get an Ack for this patch, so I don't have
to repost this large (and still growing) series to netdev every time?
I guess it would be ok to have this go through another tree?
-michael
On Thu, Sep 01, 2022 at 06:26:39PM +0200, Michael Walle wrote:
> Hi netdev maintainers,
>
> Am 2022-08-25 23:44, schrieb Michael Walle:
> > 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]>
>
> Would it be possible to get an Ack for this patch, so I don't have
> to repost this large (and still growing) series to netdev every time?
Looks O.K. to me
Reviewed-by: Andrew Lunn <[email protected]>
On Thu, 01 Sep 2022 18:26:39 +0200 Michael Walle wrote:
> Am 2022-08-25 23:44, schrieb Michael Walle:
> > 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]>
>
> Would it be possible to get an Ack for this patch, so I don't have
> to repost this large (and still growing) series to netdev every time?
>
> I guess it would be ok to have this go through another tree?
Andrew's ack is strong enough, but in case there's any doubt:
Acked-by: Jakub Kicinski <[email protected]>
On Wed, Aug 31, 2022 at 5:30 PM Michael Walle <[email protected]> wrote:
>
> Am 2022-08-31 23:48, schrieb Rob Herring:
> > On Thu, Aug 25, 2022 at 11:44:17PM +0200, Michael Walle wrote:
> >> The "user-otp" and "factory-otp" compatible string just depicts a
> >> generic NVMEM device. But an actual device tree node might as well
> >> contain a more specific compatible string. Make it possible to add
> >> more specific binding elsewere and just match part of the compatibles
> >> here.
> >>
> >> Signed-off-by: Michael Walle <[email protected]>
> >> ---
> >> Documentation/devicetree/bindings/mtd/mtd.yaml | 7 ++++---
> >> 1 file changed, 4 insertions(+), 3 deletions(-)
> >
> > In hindsight it looks like we are mixing 2 different purposes of 'which
> > instance is this' and 'what is this'. 'compatible' is supposed to be
> > the
> > latter.
> >
> > Maybe there's a better way to handle user/factory? There's a similar
> > need with partitions for A/B or factory/update.
>
> I'm not sure I understand what you mean. It has nothing to with
> user and factory provisionings.
>
> SPI flashes have a user programmable and a factory programmable
> area, some have just one of them. Whereas with A/B you (as in the
> user or the board manufacturer) defines an area within a memory device
> to be either slot A or slot B. But here the flash dictates what's
> factory and what's user storage. It's in the datasheet.
Ah, right. Nevermind...
Rob