Subject: Re: [PATCH v6 2/2] nvmem: imx-ocotp-ele: Support accessing
controller for i.MX9

Am Mittwoch, 22. Januar 2025, 04:36:33 CET schrieb Peng Fan:

quoted

quoted

Subject: Re: [PATCH v6 2/2] nvmem: imx-ocotp-ele: Support

accessing

quoted

quoted

controller for i.MX9

Hi,

Am Dienstag, 21. Januar 2025, 16:05:32 CET schrieb Peng Fan (OSS):

quoted

From: Peng Fan <peng.fan@nxp.com>

i.MX9 OCOTP supports a specific peripheral or function being

fused

quoted

quoted

quoted

which means disabled, so
 - Introduce ocotp_access_gates to be container of efuse gate info
 - Iterate all nodes to check accessing permission. If not
   allowed to be accessed, detach the node

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---
 drivers/nvmem/Kconfig         |   3 +
 drivers/nvmem/imx-ocotp-ele.c | 172
+++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 174 insertions(+), 1 deletion(-)

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig

index

quoted

quoted

quoted

8671b7c974b933e147154bb40b5d41b5730518d2..77cc496fd5e0e1af

quoted

quoted

d753534b56fe

quoted

1f5ef3e3ec55 100644

--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig

@@ -93,6 +93,9 @@ config NVMEM_IMX_OCOTP_ELE
 	  This is a driver for the On-Chip OTP Controller (OCOTP)
 	  available on i.MX SoCs which has ELE.

+	  If built as modules, any other driver relying on this working
+	  as access controller also needs to be a module as well.
+
 config NVMEM_IMX_OCOTP_SCU
 	tristate "i.MX8 SCU On-Chip OTP Controller support"
 	depends on IMX_SCU

diff --git a/drivers/nvmem/imx-ocotp-ele.c

b/drivers/nvmem/imx-ocotp-ele.c index

ca6dd71d8a2e29888c6e556aaea116c1a967cb5f..5ea6d959ce38760ee

quoted

quoted

ed44a989992

quoted

fb35c462c0b4 100644

--- a/drivers/nvmem/imx-ocotp-ele.c
+++ b/drivers/nvmem/imx-ocotp-ele.c

@@ -5,6 +5,8 @@
  * Copyright 2023 NXP
  */

+#include <dt-bindings/nvmem/fsl,imx93-ocotp.h>
+#include <dt-bindings/nvmem/fsl,imx95-ocotp.h>
 #include <linux/device.h>
 #include <linux/io.h>
 #include <linux/module.h>

@@ -27,6 +29,7 @@ struct ocotp_map_entry {  };

 struct ocotp_devtype_data {
+	const struct ocotp_access_gates *access_gates;
 	u32 reg_off;
 	char *name;
 	u32 size;

@@ -36,6 +39,20 @@ struct ocotp_devtype_data {
 	struct ocotp_map_entry entry[];
 };

+#define OCOTP_MAX_NUM_GATE_WORDS 4
+
+struct access_gate {
+	u32 word;
+	u32 mask;
+};
+
+struct ocotp_access_gates {
+	u32 num_words;
+	u32 words[OCOTP_MAX_NUM_GATE_WORDS];
+	u32 num_gates;
+	struct access_gate *gates;
+};
+
 struct imx_ocotp_priv {
 	struct device *dev;
 	void __iomem *base;

@@ -131,6 +148,82 @@ static void

imx_ocotp_fixup_dt_cell_info(struct nvmem_device *nvmem,

quoted

 	cell->read_post_process = imx_ocotp_cell_pp;  }

+static int imx_ele_ocotp_check_access(struct imx_ocotp_priv
+*priv,
+u32 id) {
+	const struct ocotp_access_gates *access_gates = priv->data-
access_gates;
+	void __iomem *reg = priv->base + priv->data->reg_off;
+	u32 word, mask, val;
+
+	if (id >= access_gates->num_gates) {
+		dev_err(priv->config.dev, "Index %d too large\n", id);
+		return -EACCES;
+	}
+
+	word = access_gates->gates[id].word;
+	mask = access_gates->gates[id].mask;
+
+	reg = priv->base + priv->data->reg_off + (word << 2);
+	val = readl(reg);
+
+	dev_dbg(priv->config.dev, "id:%d word:%d mask:0x%08x\n",

id, word, mask);

quoted

+	/* true means not allow access */
+	if (val & mask)
+		return -EACCES;
+
+	return 0;
+}
+
+static int imx_ele_ocotp_grant_access(struct imx_ocotp_priv
+*priv, struct device_node *parent) {
+	struct device *dev = priv->config.dev;
+
+	for_each_available_child_of_node_scoped(parent, child) {
+		struct of_phandle_args args;
+		u32 id, idx = 0;
+
+		while (!of_parse_phandle_with_args(child, "access-

controllers",

quoted

+						   "#access-

controller-cells",

quoted

+						   idx++, &args)) {
+			of_node_put(args.np);
+			if (args.np != dev->of_node)
+				continue;
+
+			/* Only support one cell */
+			if (args.args_count != 1) {
+				dev_err(dev, "wrong args count\n");
+				continue;
+			}
+
+			id = args.args[0];
+
+			dev_dbg(dev, "Checking node: %pOF

gate: %d\n", child, id);

quoted

+
+			if (imx_ele_ocotp_check_access(priv, id)) {
+				of_detach_node(child);
+				dev_info(dev, "%pOF: Not granted,

device driver will not be probed\n",

quoted

+					 child);
+			}
+		}
+
+		imx_ele_ocotp_grant_access(priv, child);
+	}
+
+	return 0;
+}
+
+static int imx_ele_ocotp_access_control(struct imx_ocotp_priv
+*priv) {
+	struct device_node *root __free(device_node) =
+of_find_node_by_path("/");
+
+	if (!priv->data->access_gates)
+		return 0;
+
+	/* This should never happen */
+	WARN_ON(!root);

Even if you warning something is wrong, aka root == NULL, you are
still using it on imx_ele_ocotp_grant_access(). Just return early.

if (WARN_ON(!))
	return -EINVAL;

Hmm, If this really happens, return early or not does not make much

sense.

quoted

Does it really matter here?

Why does it not make much sense? You already know something is
wrong, aka you have a NULL pointer, so it makes even less sense to
continue.
I've skipped through the sources and looked for
'WARN_ON(!<pointer>)', most of the times it is actually checked for
early returns.

I will include the fail return in V7. Please help check if other parts are
good for you.

Thanks,
Peng.