[PATCH v2 0/4] nvmem: qfprom: Avoid untouchable regions

[PATCH v2 0/4] nvmem: qfprom: Avoid untouchable regions

[Evan Green]

Certain fuses are protected by the XPU such that the AP cannot
access them. Attempting to do so causes an SError. Introduce an
SoC-specific compatible string, and introduce support into the
nvmem core to avoid accessing specified regions. Then use those
new elements in the qfprom driver to avoid SErrors when usermode
accesses certain registers.

Changes in v2:
 - Add other soc compatible strings (Doug)
 - Fix compatible string definition (Doug)
 - Introduced keepout regions into the core (Srini)
 - Use new core support in qfprom (Srini)

Evan Green (4):
  dt-bindings: nvmem: Add soc qfprom compatible strings
  arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
  nvmem: core: Add support for keepout regions
  nvmem: qfprom: Don't touch certain fuses

 .../bindings/nvmem/qcom,qfprom.yaml           | 13 ++-
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |  2 +-
 drivers/nvmem/core.c                          | 95 ++++++++++++++++++-
 drivers/nvmem/qfprom.c                        | 30 ++++++
 include/linux/nvmem-provider.h                | 17 ++++
 5 files changed, 151 insertions(+), 6 deletions(-)

-- 
2.26.2

[PATCH v2 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings

[Evan Green]

Add SoC-specific compatible strings so that data can be attached
to it in the driver.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Changes in v2:
 - Add other soc compatible strings (Doug)
 - Fix compatible string definition (Doug)

 .../devicetree/bindings/nvmem/qcom,qfprom.yaml      | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
index 1a18b6bab35e7..eb1440045aff1 100644
--- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
+++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
@@ -14,7 +14,18 @@ allOf:
 
 properties:
   compatible:
-    const: qcom,qfprom
+    items:
+      - enum:
+          - qcom,apq8064-qfprom
+          - qcom,apq8084-qfprom
+          - qcom,msm8974-qfprom
+          - qcom,msm8916-qfprom
+          - qcom,msm8996-qfprom
+          - qcom,msm8998-qfprom
+          - qcom,qcs404-qfprom
+          - qcom,sc7180-qfprom
+          - qcom,sdm845-qfprom
+      - const: qcom,qfprom
 
   reg:
     # If the QFPROM is read-only OS image then only the corrected region
-- 
2.26.2

[PATCH v2 2/4] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string

[Evan Green]

Add the soc-specific compatible string so that it can be matched
more specifically now that the driver cares which SoC it's on.

Signed-off-by: Evan Green <evgreen@chromium.org>
Reviewed-by: Douglas Anderson <dianders@chromium.org>
---

(no changes since v1)

 arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 6678f1e8e3958..f1f8bbc0b37bc 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -660,7 +660,7 @@ gcc: clock-controller@100000 {
 		};
 
 		qfprom: efuse@784000 {
-			compatible = "qcom,qfprom";
+			compatible = "qcom,sc7180-qfprom", "qcom,qfprom";
 			reg = <0 0x00784000 0 0x8ff>,
 			      <0 0x00780000 0 0x7a0>,
 			      <0 0x00782000 0 0x100>,
-- 
2.26.2

[PATCH v2 3/4] nvmem: core: Add support for keepout regions

[Evan Green]

Introduce support into the nvmem core for arrays of register ranges
that should not result in actual device access. For these regions a
constant byte (repeated) is returned instead on read, and writes are
quietly ignored and returned as successful.

This is useful for instance if certain efuse regions are protected
from access by Linux because they contain secret info to another part
of the system (like an integrated modem).

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Changes in v2:
 - Introduced keepout regions into the core (Srini)

 drivers/nvmem/core.c           | 95 ++++++++++++++++++++++++++++++++--
 include/linux/nvmem-provider.h | 17 ++++++
 2 files changed, 108 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index a09ff8409f600..f7819c57f8828 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -19,6 +19,9 @@
 #include <linux/of.h>
 #include <linux/slab.h>
 
+#define MAX(a, b) ((a) > (b) ? (a) : (b))
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
+
 struct nvmem_device {
 	struct module		*owner;
 	struct device		dev;
@@ -34,6 +37,8 @@ struct nvmem_device {
 	struct bin_attribute	eeprom;
 	struct device		*base_dev;
 	struct list_head	cells;
+	const struct nvmem_keepout *keepout;
+	unsigned int		nkeepout;
 	nvmem_reg_read_t	reg_read;
 	nvmem_reg_write_t	reg_write;
 	struct gpio_desc	*wp_gpio;
@@ -66,8 +71,8 @@ static LIST_HEAD(nvmem_lookup_list);
 
 static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 
-static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
-			  void *val, size_t bytes)
+static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
+			    void *val, size_t bytes)
 {
 	if (nvmem->reg_read)
 		return nvmem->reg_read(nvmem->priv, offset, val, bytes);
@@ -75,8 +80,8 @@ static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
 	return -EINVAL;
 }
 
-static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
-			   void *val, size_t bytes)
+static int __nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
+			     void *val, size_t bytes)
 {
 	int ret;
 
@@ -90,6 +95,86 @@ static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
 	return -EINVAL;
 }
 
+static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
+				      unsigned int offset, void *val,
+				      size_t bytes, int write)
+{
+
+	unsigned int end = offset + bytes;
+	unsigned int kend, ksize;
+	const struct nvmem_keepout *keepout = nvmem->keepout;
+	const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
+	int rc;
+
+	/* Skip everything before the range being accessed */
+	while ((keepout < keepoutend) && (keepout->end <= offset))
+		keepout++;
+
+	while ((offset < end) && (keepout < keepoutend)) {
+
+		/* Access the valid portion before the keepout. */
+		if (offset < keepout->start) {
+			kend = MIN(end, keepout->start);
+			ksize = kend - offset;
+			if (write)
+				rc = __nvmem_reg_write(nvmem, offset, val, ksize);
+			else
+				rc = __nvmem_reg_read(nvmem, offset, val, ksize);
+
+			if (rc)
+				return rc;
+
+			offset += ksize;
+			val += ksize;
+		}
+
+		/*
+		 * Now we're aligned to the start of this keepout zone. Go
+		 * through it.
+		 */
+		kend = MIN(end, keepout->end);
+		ksize = kend - offset;
+		if (!write)
+			memset(val, keepout->value, ksize);
+
+		val += ksize;
+		offset += ksize;
+		keepout++;
+	}
+
+	/*
+	 * If we ran out of keepouts but there's still stuff to do, send it
+	 * down directly
+	 */
+	if (offset < end) {
+		ksize = end - offset;
+		if (write)
+			return __nvmem_reg_write(nvmem, offset, val, ksize);
+		else
+			return __nvmem_reg_read(nvmem, offset, val, ksize);
+	}
+
+	return 0;
+}
+
+static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
+			  void *val, size_t bytes)
+{
+	if (!nvmem->nkeepout)
+		return __nvmem_reg_read(nvmem, offset, val, bytes);
+
+	return nvmem_access_with_keepouts(nvmem, offset, val, bytes, false);
+}
+
+static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int offset,
+			   void *val, size_t bytes)
+{
+	if (!nvmem->nkeepout)
+		return __nvmem_reg_write(nvmem, offset, val, bytes);
+
+	return nvmem_access_with_keepouts(nvmem, offset, val, bytes, true);
+}
+
 #ifdef CONFIG_NVMEM_SYSFS
 static const char * const nvmem_type_str[] = {
 	[NVMEM_TYPE_UNKNOWN] = "Unknown",
@@ -647,6 +732,8 @@ 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->keepout = config->keepout;
+	nvmem->nkeepout = config->nkeepout;
 	if (!config->no_of_node)
 		nvmem->dev.of_node = config->dev->of_node;
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 06409a6c40bcb..e162b757b6d54 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -30,6 +30,19 @@ enum nvmem_type {
 #define NVMEM_DEVID_NONE	(-1)
 #define NVMEM_DEVID_AUTO	(-2)
 
+/**
+ * struct nvmem_keepout - NVMEM register keepout range.
+ *
+ * @start:	The first byte offset to avoid.
+ * @end:	One beyond the last byte offset to avoid.
+ * @value:	The byte to fill reads with for this region.
+ */
+struct nvmem_keepout {
+	unsigned int start;
+	unsigned int end;
+	unsigned char value;
+};
+
 /**
  * struct nvmem_config - NVMEM device configuration
  *
@@ -39,6 +52,8 @@ enum nvmem_type {
  * @owner:	Pointer to exporter module. Used for refcounting.
  * @cells:	Optional array of pre-defined NVMEM cells.
  * @ncells:	Number of elements in cells.
+ * @keepout:	Optional array of keepout ranges (sorted ascending by start).
+ * @nkeepout:	Number of elements in the keepout array.
  * @type:	Type of the nvmem storage
  * @read_only:	Device is read-only.
  * @root_only:	Device is accessibly to root only.
@@ -66,6 +81,8 @@ struct nvmem_config {
 	struct gpio_desc	*wp_gpio;
 	const struct nvmem_cell_info	*cells;
 	int			ncells;
+	const struct nvmem_keepout *keepout;
+	unsigned int		nkeepout;
 	enum nvmem_type		type;
 	bool			read_only;
 	bool			root_only;
-- 
2.26.2

[PATCH v2 4/4] nvmem: qfprom: Don't touch certain fuses

[Evan Green]

Some fuse ranges are protected by the XPU such that the AP cannot
access them. Attempting to do so causes an SError. Use the newly
introduced per-soc compatible string, and the newly introduced
nvmem keepout support to attach the set of regions
we should not access.

Signed-off-by: Evan Green <evgreen@chromium.org>
---

Changes in v2:
 - Use new core support in qfprom (Srini)

 drivers/nvmem/qfprom.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 5e9e60e2e591d..6cace24dfbf73 100644
--- a/drivers/nvmem/qfprom.c
+++ b/drivers/nvmem/qfprom.c
@@ -12,6 +12,7 @@
 #include <linux/mod_devicetable.h>
 #include <linux/nvmem-provider.h>
 #include <linux/platform_device.h>
+#include <linux/property.h>
 #include <linux/regulator/consumer.h>
 
 /* Blow timer clock frequency in Mhz */
@@ -88,6 +89,28 @@ struct qfprom_touched_values {
 	u32 timer_val;
 };
 
+/**
+ * struct qfprom_soc_compatible_data - Data matched against the SoC
+ * compatible string.
+ *
+ * @keepout: Array of keepout regions for this SoC.
+ * @nkeepout: Number of elements in the keepout array.
+ */
+struct qfprom_soc_compatible_data {
+	const struct nvmem_keepout *keepout;
+	unsigned int nkeepout;
+};
+
+static const struct nvmem_keepout sc7180_qfprom_keepout[] = {
+	{.start = 0x128, .end = 0x148},
+	{.start = 0x220, .end = 0x228}
+};
+
+static const struct qfprom_soc_compatible_data sc7180_qfprom = {
+	.keepout = sc7180_qfprom_keepout,
+	.nkeepout = ARRAY_SIZE(sc7180_qfprom_keepout)
+};
+
 /**
  * qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
  * @priv: Our driver data.
@@ -281,6 +304,7 @@ static int qfprom_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct nvmem_device *nvmem;
+	const struct qfprom_soc_compatible_data *soc_data;
 	struct qfprom_priv *priv;
 	int ret;
 
@@ -299,6 +323,11 @@ static int qfprom_probe(struct platform_device *pdev)
 	econfig.priv = priv;
 
 	priv->dev = dev;
+	soc_data = device_get_match_data(dev);
+	if (soc_data) {
+		econfig.keepout = soc_data->keepout;
+		econfig.nkeepout = soc_data->nkeepout;
+	}
 
 	/*
 	 * If more than one region is provided then the OS has the ability
@@ -354,6 +383,7 @@ static int qfprom_probe(struct platform_device *pdev)
 
 static const struct of_device_id qfprom_of_match[] = {
 	{ .compatible = "qcom,qfprom",},
+	{ .compatible = "qcom,sc7180-qfprom", .data = &sc7180_qfprom},
 	{/* sentinel */},
 };
 MODULE_DEVICE_TABLE(of, qfprom_of_match);
-- 
2.26.2

Re: [PATCH v2 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings

[Rob Herring]

On Fri, 16 Oct 2020 12:26:51 -0700, Evan Green wrote:
> Add SoC-specific compatible strings so that data can be attached
> to it in the driver.
> 
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> 
> Changes in v2:
>  - Add other soc compatible strings (Doug)
>  - Fix compatible string definition (Doug)
> 
>  .../devicetree/bindings/nvmem/qcom,qfprom.yaml      | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 


My bot found errors running 'make dt_binding_check' on your patch:

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.example.dt.yaml: efuse@784000: compatible:0: 'qcom,qfprom' is not one of ['qcom,apq8064-qfprom', 'qcom,apq8084-qfprom', 'qcom,msm8974-qfprom', 'qcom,msm8916-qfprom', 'qcom,msm8996-qfprom', 'qcom,msm8998-qfprom', 'qcom,qcs404-qfprom', 'qcom,sc7180-qfprom', 'qcom,sdm845-qfprom']
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.example.dt.yaml: efuse@784000: compatible: ['qcom,qfprom'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.example.dt.yaml: efuse@784000: compatible:0: 'qcom,qfprom' is not one of ['qcom,apq8064-qfprom', 'qcom,apq8084-qfprom', 'qcom,msm8974-qfprom', 'qcom,msm8916-qfprom', 'qcom,msm8996-qfprom', 'qcom,msm8998-qfprom', 'qcom,qcs404-qfprom', 'qcom,sc7180-qfprom', 'qcom,sdm845-qfprom']
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.example.dt.yaml: efuse@784000: compatible: ['qcom,qfprom'] is too short
	From schema: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml


See https://patchwork.ozlabs.org/patch/1383520

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure dt-schema is up to date:

pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade

Please check and re-submit.

Re: [PATCH v2 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings

[Doug Anderson]

Hi,

On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgreen@chromium.org> wrote:
>
> Add SoC-specific compatible strings so that data can be attached
> to it in the driver.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
>  - Add other soc compatible strings (Doug)
>  - Fix compatible string definition (Doug)
>
>  .../devicetree/bindings/nvmem/qcom,qfprom.yaml      | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> index 1a18b6bab35e7..eb1440045aff1 100644
> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> @@ -14,7 +14,18 @@ allOf:
>
>  properties:
>    compatible:
> -    const: qcom,qfprom
> +    items:
> +      - enum:
> +          - qcom,apq8064-qfprom
> +          - qcom,apq8084-qfprom
> +          - qcom,msm8974-qfprom
> +          - qcom,msm8916-qfprom
> +          - qcom,msm8996-qfprom
> +          - qcom,msm8998-qfprom
> +          - qcom,qcs404-qfprom
> +          - qcom,sc7180-qfprom
> +          - qcom,sdm845-qfprom
> +      - const: qcom,qfprom
>
>    reg:
>      # If the QFPROM is read-only OS image then only the corrected region

As Rob's bot found, your example no longer matches your requirements.
It needs an SoC-specific string plus the "qcom,qfprom".  It's always
good to try running "make dt_binding_check" to catch these sorts of
things.


-Doug

Re: [PATCH v2 3/4] nvmem: core: Add support for keepout regions

[Doug Anderson]

Hi,

On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgreen@chromium.org> wrote:
>
> Introduce support into the nvmem core for arrays of register ranges
> that should not result in actual device access. For these regions a
> constant byte (repeated) is returned instead on read, and writes are
> quietly ignored and returned as successful.
>
> This is useful for instance if certain efuse regions are protected
> from access by Linux because they contain secret info to another part
> of the system (like an integrated modem).
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
>  - Introduced keepout regions into the core (Srini)
>
>  drivers/nvmem/core.c           | 95 ++++++++++++++++++++++++++++++++--
>  include/linux/nvmem-provider.h | 17 ++++++
>  2 files changed, 108 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index a09ff8409f600..f7819c57f8828 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -19,6 +19,9 @@
>  #include <linux/of.h>
>  #include <linux/slab.h>
>
> +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> +#define MIN(a, b) ((a) < (b) ? (a) : (b))

Why not use min() / max() macros from include/linux/kernel.h?


> +static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
> +                                     unsigned int offset, void *val,
> +                                     size_t bytes, int write)
> +{
> +
> +       unsigned int end = offset + bytes;
> +       unsigned int kend, ksize;
> +       const struct nvmem_keepout *keepout = nvmem->keepout;
> +       const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
> +       int rc;
> +
> +       /* Skip everything before the range being accessed */

nit: "Skip everything" => "Skip all keepouts"

...might not hurt to remind here that keepouts are sorted?


> +       while ((keepout < keepoutend) && (keepout->end <= offset))
> +               keepout++;
> +
> +       while ((offset < end) && (keepout < keepoutend)) {
> +

nit: remove blank line?


> @@ -647,6 +732,8 @@ 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->keepout = config->keepout;
> +       nvmem->nkeepout = config->nkeepout;

It seems like it might be worth adding something to validate that the
ranges are sorted and return an error if they're not.

Maybe worth validating (and documenting) that the keepouts won't cause
us to violate "stride" and/or "word_size" ?


Everything above is just nits and other than them this looks like a
nice change.  BTW: this is the kind of thing that screams for unit
testing, though that might be a bit too much of a yak to shave here?

-Doug

Re: [PATCH v2 4/4] nvmem: qfprom: Don't touch certain fuses

[Doug Anderson]

Hi,

On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgreen@chromium.org> wrote:
>
> Some fuse ranges are protected by the XPU such that the AP cannot
> access them. Attempting to do so causes an SError. Use the newly
> introduced per-soc compatible string, and the newly introduced
> nvmem keepout support to attach the set of regions
> we should not access.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
> Changes in v2:
>  - Use new core support in qfprom (Srini)
>
>  drivers/nvmem/qfprom.c | 30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 1/4] dt-bindings: nvmem: Add soc qfprom compatible strings

[Evan Green]

On Wed, Oct 21, 2020 at 2:41 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgreen@chromium.org> wrote:
> >
> > Add SoC-specific compatible strings so that data can be attached
> > to it in the driver.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> > Changes in v2:
> >  - Add other soc compatible strings (Doug)
> >  - Fix compatible string definition (Doug)
> >
> >  .../devicetree/bindings/nvmem/qcom,qfprom.yaml      | 13 ++++++++++++-
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > index 1a18b6bab35e7..eb1440045aff1 100644
> > --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > @@ -14,7 +14,18 @@ allOf:
> >
> >  properties:
> >    compatible:
> > -    const: qcom,qfprom
> > +    items:
> > +      - enum:
> > +          - qcom,apq8064-qfprom
> > +          - qcom,apq8084-qfprom
> > +          - qcom,msm8974-qfprom
> > +          - qcom,msm8916-qfprom
> > +          - qcom,msm8996-qfprom
> > +          - qcom,msm8998-qfprom
> > +          - qcom,qcs404-qfprom
> > +          - qcom,sc7180-qfprom
> > +          - qcom,sdm845-qfprom
> > +      - const: qcom,qfprom
> >
> >    reg:
> >      # If the QFPROM is read-only OS image then only the corrected region
>
> As Rob's bot found, your example no longer matches your requirements.
> It needs an SoC-specific string plus the "qcom,qfprom".  It's always
> good to try running "make dt_binding_check" to catch these sorts of
> things.

Thanks Doug, will do for the next spin!

-Evan

Re: [PATCH v2 3/4] nvmem: core: Add support for keepout regions

[Evan Green]

On Wed, Oct 21, 2020 at 2:41 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Oct 16, 2020 at 12:27 PM Evan Green <evgreen@chromium.org> wrote:
> >
> > Introduce support into the nvmem core for arrays of register ranges
> > that should not result in actual device access. For these regions a
> > constant byte (repeated) is returned instead on read, and writes are
> > quietly ignored and returned as successful.
> >
> > This is useful for instance if certain efuse regions are protected
> > from access by Linux because they contain secret info to another part
> > of the system (like an integrated modem).
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> > Changes in v2:
> >  - Introduced keepout regions into the core (Srini)
> >
> >  drivers/nvmem/core.c           | 95 ++++++++++++++++++++++++++++++++--
> >  include/linux/nvmem-provider.h | 17 ++++++
> >  2 files changed, 108 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index a09ff8409f600..f7819c57f8828 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -19,6 +19,9 @@
> >  #include <linux/of.h>
> >  #include <linux/slab.h>
> >
> > +#define MAX(a, b) ((a) > (b) ? (a) : (b))
> > +#define MIN(a, b) ((a) < (b) ? (a) : (b))
>
> Why not use min() / max() macros from include/linux/kernel.h?
>

Done

>
> > +static int nvmem_access_with_keepouts(struct nvmem_device *nvmem,
> > +                                     unsigned int offset, void *val,
> > +                                     size_t bytes, int write)
> > +{
> > +
> > +       unsigned int end = offset + bytes;
> > +       unsigned int kend, ksize;
> > +       const struct nvmem_keepout *keepout = nvmem->keepout;
> > +       const struct nvmem_keepout *keepoutend = keepout + nvmem->nkeepout;
> > +       int rc;
> > +
> > +       /* Skip everything before the range being accessed */
>
> nit: "Skip everything" => "Skip all keepouts"
>
> ...might not hurt to remind here that keepouts are sorted?

Done

>
>
> > +       while ((keepout < keepoutend) && (keepout->end <= offset))
> > +               keepout++;
> > +
> > +       while ((offset < end) && (keepout < keepoutend)) {
> > +
>
> nit: remove blank line?

Done

>
>
> > @@ -647,6 +732,8 @@ 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->keepout = config->keepout;
> > +       nvmem->nkeepout = config->nkeepout;
>
> It seems like it might be worth adding something to validate that the
> ranges are sorted and return an error if they're not.
>
> Maybe worth validating (and documenting) that the keepouts won't cause
> us to violate "stride" and/or "word_size" ?

Done

>
>
> Everything above is just nits and other than them this looks like a
> nice change.  BTW: this is the kind of thing that screams for unit
> testing, though that might be a bit too much of a yak to shave here?

It would be cool, but I'll leave that for another time. Thanks for the
review, Doug!

>
> -Doug