linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/3] nvmem: qfprom: Avoid untouchable regions
@ 2020-09-29 20:58 Evan Green
  2020-09-29 20:58 ` [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string Evan Green
                   ` (2 more replies)
  0 siblings, 3 replies; 13+ messages in thread
From: Evan Green @ 2020-09-29 20:58 UTC (permalink / raw)
  To: Rob Herring, Bjorn Andersson, Srinivas Kandagatla
  Cc: Douglas Anderson, Stephen Boyd, Evan Green, Andy Gross,
	devicetree, linux-arm-msm, linux-kernel

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 then use that to determine
which fuse regions to present as Read as Zero / Write Ignore.


Evan Green (3):
  dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
  arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
  nvmem: qfprom: Don't touch certain fuses

 .../bindings/nvmem/qcom,qfprom.yaml           |  4 +-
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |  2 +-
 drivers/nvmem/qfprom.c                        | 55 +++++++++++++++++--
 3 files changed, 55 insertions(+), 6 deletions(-)

-- 
2.26.2


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
  2020-09-29 20:58 [PATCH 0/3] nvmem: qfprom: Avoid untouchable regions Evan Green
@ 2020-09-29 20:58 ` Evan Green
  2020-10-02 22:20   ` Doug Anderson
  2020-09-29 20:58 ` [PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string Evan Green
  2020-09-29 20:58 ` [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses Evan Green
  2 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-09-29 20:58 UTC (permalink / raw)
  To: Rob Herring, Bjorn Andersson, Srinivas Kandagatla
  Cc: Douglas Anderson, Stephen Boyd, Evan Green, Andy Gross,
	devicetree, linux-arm-msm, linux-kernel

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

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

 Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
index 59aca6d22ff9b..b16c8e6a8c23d 100644
--- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
+++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
@@ -14,7 +14,9 @@ allOf:
 
 properties:
   compatible:
-    const: qcom,qfprom
+    enum:
+      - qcom,qfprom
+      - qcom,sc7180-qfprom
 
   reg:
     # If the QFPROM is read-only OS image then only the corrected region
-- 
2.26.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
  2020-09-29 20:58 [PATCH 0/3] nvmem: qfprom: Avoid untouchable regions Evan Green
  2020-09-29 20:58 ` [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string Evan Green
@ 2020-09-29 20:58 ` Evan Green
  2020-10-02 22:24   ` Doug Anderson
  2020-09-29 20:58 ` [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses Evan Green
  2 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-09-29 20:58 UTC (permalink / raw)
  To: Rob Herring, Bjorn Andersson, Srinivas Kandagatla
  Cc: Douglas Anderson, Stephen Boyd, Evan Green, Andy Gross,
	devicetree, linux-arm-msm, linux-kernel

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>
---

 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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
  2020-09-29 20:58 [PATCH 0/3] nvmem: qfprom: Avoid untouchable regions Evan Green
  2020-09-29 20:58 ` [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string Evan Green
  2020-09-29 20:58 ` [PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string Evan Green
@ 2020-09-29 20:58 ` Evan Green
  2020-10-01 14:17   ` Srinivas Kandagatla
  2 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-09-29 20:58 UTC (permalink / raw)
  To: Rob Herring, Bjorn Andersson, Srinivas Kandagatla
  Cc: Douglas Anderson, Stephen Boyd, Evan Green, linux-kernel

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 to attach the set of regions
we should not access. Then tiptoe around those regions.

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

 drivers/nvmem/qfprom.c | 55 +++++++++++++++++++++++++++++++++++++++---
 1 file changed, 51 insertions(+), 4 deletions(-)

diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 5e9e60e2e591d..feea39ae52164 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 */
@@ -51,6 +52,17 @@ struct qfprom_soc_data {
 	u32 qfprom_blow_set_freq;
 };
 
+/**
+ * struct qfprom_keepout_region - registers to avoid touching.
+ *
+ * @start: The first byte offset to avoid.
+ * @end: One after the last byte offset to avoid.
+ */
+struct qfprom_keepout_region {
+	u32 start;
+	u32 end;
+};
+
 /**
  * struct qfprom_priv - structure holding qfprom attributes
  *
@@ -63,6 +75,7 @@ struct qfprom_soc_data {
  * @secclk:       Clock supply.
  * @vcc:          Regulator supply.
  * @soc_data:     Data that for things that varies from SoC to SoC.
+ * @keepout:      Fuse regions not to access, as they may cause SErrors.
  */
 struct qfprom_priv {
 	void __iomem *qfpraw;
@@ -73,6 +86,7 @@ struct qfprom_priv {
 	struct clk *secclk;
 	struct regulator *vcc;
 	const struct qfprom_soc_data *soc_data;
+	const struct qfprom_keepout_region *keepout;
 };
 
 /**
@@ -88,6 +102,12 @@ struct qfprom_touched_values {
 	u32 timer_val;
 };
 
+const struct qfprom_keepout_region sc7180_qfprom[] = {
+	{.start = 0x128, .end = 0x148},
+	{.start = 0x220, .end = 0x228},
+	{} /* Sentinal where start == end. */
+};
+
 /**
  * qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
  * @priv: Our driver data.
@@ -171,6 +191,23 @@ static int qfprom_enable_fuse_blowing(const struct qfprom_priv *priv,
 	return ret;
 }
 
+static int qfprom_check_reg(struct qfprom_priv *priv, unsigned int reg)
+{
+	const struct qfprom_keepout_region *keepout = priv->keepout;
+
+	if (!keepout)
+		return 1;
+
+	while (keepout->start != keepout->end) {
+		if ((reg >= keepout->start) && (reg < keepout->end))
+			return 0;
+
+		keepout++;
+	}
+
+	return 1;
+}
+
 /**
  * qfprom_efuse_reg_write() - Write to fuses.
  * @context: Our driver data.
@@ -228,8 +265,10 @@ static int qfprom_reg_write(void *context, unsigned int reg, void *_val,
 		goto exit_enabled_fuse_blowing;
 	}
 
-	for (i = 0; i < words; i++)
-		writel(value[i], priv->qfpraw + reg + (i * 4));
+	for (i = 0; i < words; i++) {
+		if (qfprom_check_reg(priv, reg + (i * 4)))
+			writel(value[i], priv->qfpraw + reg + (i * 4));
+	}
 
 	ret = readl_relaxed_poll_timeout(
 		priv->qfpconf + QFPROM_BLOW_STATUS_OFFSET,
@@ -257,8 +296,14 @@ static int qfprom_reg_read(void *context,
 	if (read_raw_data && priv->qfpraw)
 		base = priv->qfpraw;
 
-	while (words--)
-		*val++ = readb(base + reg + i++);
+	while (words--) {
+		if (qfprom_check_reg(priv, reg + i))
+			*val++ = readb(base + reg + i);
+		else
+			*val++ = 0;
+
+		i++;
+	}
 
 	return 0;
 }
@@ -299,6 +344,7 @@ static int qfprom_probe(struct platform_device *pdev)
 	econfig.priv = priv;
 
 	priv->dev = dev;
+	priv->keepout = device_get_match_data(dev);
 
 	/*
 	 * If more than one region is provided then the OS has the ability
@@ -354,6 +400,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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
  2020-09-29 20:58 ` [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses Evan Green
@ 2020-10-01 14:17   ` Srinivas Kandagatla
  2020-10-01 16:27     ` Evan Green
  0 siblings, 1 reply; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-10-01 14:17 UTC (permalink / raw)
  To: Evan Green, Rob Herring, Bjorn Andersson
  Cc: Douglas Anderson, Stephen Boyd, linux-kernel

Hi Evan,

On 29/09/2020 21:58, Evan Green 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 to attach the set of regions
> we should not access. Then tiptoe around those regions.
> 

This is a generic feature that can be used by any nvmem provider, can 
you move this logic to nvmem core instead of having it in qfprom!


thanks,
srini

> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
> 
>   drivers/nvmem/qfprom.c | 55 +++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 51 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 5e9e60e2e591d..feea39ae52164 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 */
> @@ -51,6 +52,17 @@ struct qfprom_soc_data {
>   	u32 qfprom_blow_set_freq;
>   };
>   
> +/**
> + * struct qfprom_keepout_region - registers to avoid touching.
> + *
> + * @start: The first byte offset to avoid.
> + * @end: One after the last byte offset to avoid.
> + */
> +struct qfprom_keepout_region {
> +	u32 start;
> +	u32 end;
> +};
> +
>   /**
>    * struct qfprom_priv - structure holding qfprom attributes
>    *
> @@ -63,6 +75,7 @@ struct qfprom_soc_data {
>    * @secclk:       Clock supply.
>    * @vcc:          Regulator supply.
>    * @soc_data:     Data that for things that varies from SoC to SoC.
> + * @keepout:      Fuse regions not to access, as they may cause SErrors.
>    */
>   struct qfprom_priv {
>   	void __iomem *qfpraw;
> @@ -73,6 +86,7 @@ struct qfprom_priv {
>   	struct clk *secclk;
>   	struct regulator *vcc;
>   	const struct qfprom_soc_data *soc_data;
> +	const struct qfprom_keepout_region *keepout;
>   };
>   
>   /**
> @@ -88,6 +102,12 @@ struct qfprom_touched_values {
>   	u32 timer_val;
>   };
>   
> +const struct qfprom_keepout_region sc7180_qfprom[] = {
> +	{.start = 0x128, .end = 0x148},
> +	{.start = 0x220, .end = 0x228},
> +	{} /* Sentinal where start == end. */
> +};
> +
>   /**
>    * qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
>    * @priv: Our driver data.
> @@ -171,6 +191,23 @@ static int qfprom_enable_fuse_blowing(const struct qfprom_priv *priv,
>   	return ret;
>   }
>   
> +static int qfprom_check_reg(struct qfprom_priv *priv, unsigned int reg)
> +{
> +	const struct qfprom_keepout_region *keepout = priv->keepout;
> +
> +	if (!keepout)
> +		return 1;
> +
> +	while (keepout->start != keepout->end) {
> +		if ((reg >= keepout->start) && (reg < keepout->end))
> +			return 0;
> +
> +		keepout++;
> +	}
> +
> +	return 1;
> +}
> +
>   /**
>    * qfprom_efuse_reg_write() - Write to fuses.
>    * @context: Our driver data.
> @@ -228,8 +265,10 @@ static int qfprom_reg_write(void *context, unsigned int reg, void *_val,
>   		goto exit_enabled_fuse_blowing;
>   	}
>   
> -	for (i = 0; i < words; i++)
> -		writel(value[i], priv->qfpraw + reg + (i * 4));
> +	for (i = 0; i < words; i++) {
> +		if (qfprom_check_reg(priv, reg + (i * 4)))
> +			writel(value[i], priv->qfpraw + reg + (i * 4));
> +	}
>   
>   	ret = readl_relaxed_poll_timeout(
>   		priv->qfpconf + QFPROM_BLOW_STATUS_OFFSET,
> @@ -257,8 +296,14 @@ static int qfprom_reg_read(void *context,
>   	if (read_raw_data && priv->qfpraw)
>   		base = priv->qfpraw;
>   
> -	while (words--)
> -		*val++ = readb(base + reg + i++);
> +	while (words--) {
> +		if (qfprom_check_reg(priv, reg + i))
> +			*val++ = readb(base + reg + i);
> +		else
> +			*val++ = 0;
> +
> +		i++;
> +	}
>   
>   	return 0;
>   }
> @@ -299,6 +344,7 @@ static int qfprom_probe(struct platform_device *pdev)
>   	econfig.priv = priv;
>   
>   	priv->dev = dev;
> +	priv->keepout = device_get_match_data(dev);
>   
>   	/*
>   	 * If more than one region is provided then the OS has the ability
> @@ -354,6 +400,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);
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
  2020-10-01 14:17   ` Srinivas Kandagatla
@ 2020-10-01 16:27     ` Evan Green
  2020-10-01 16:30       ` Srinivas Kandagatla
  0 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-10-01 16:27 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Rob Herring, Bjorn Andersson, Douglas Anderson, Stephen Boyd, LKML

On Thu, Oct 1, 2020 at 7:17 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Hi Evan,
>
> On 29/09/2020 21:58, Evan Green 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 to attach the set of regions
> > we should not access. Then tiptoe around those regions.
> >
>
> This is a generic feature that can be used by any nvmem provider, can
> you move this logic to nvmem core instead of having it in qfprom!

Sure! I'd prefer to keep this data in the driver for now rather than
trying to define DT bindings for the keepout zones. So then I'll pass
in my keepout array via struct nvmem_config at registration time, and
then the core can handle the keepout logic instead of qfprom.c.

-Evan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
  2020-10-01 16:27     ` Evan Green
@ 2020-10-01 16:30       ` Srinivas Kandagatla
  2020-10-01 21:49         ` Evan Green
  0 siblings, 1 reply; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-10-01 16:30 UTC (permalink / raw)
  To: Evan Green
  Cc: Rob Herring, Bjorn Andersson, Douglas Anderson, Stephen Boyd, LKML



On 01/10/2020 17:27, Evan Green wrote:
> On Thu, Oct 1, 2020 at 7:17 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> Hi Evan,
>>
>> On 29/09/2020 21:58, Evan Green 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 to attach the set of regions
>>> we should not access. Then tiptoe around those regions.
>>>
>>
>> This is a generic feature that can be used by any nvmem provider, can
>> you move this logic to nvmem core instead of having it in qfprom!
> 
> Sure! I'd prefer to keep this data in the driver for now rather than
Ofcourse these can come from driver directly based on compatible!

> trying to define DT bindings for the keepout zones. So then I'll pass
> in my keepout array via struct nvmem_config at registration time, and
> then the core can handle the keepout logic instead of qfprom.c.
> 

Yes, that is inline with what am thinking of as well!


00srini
> -Evan
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
  2020-10-01 16:30       ` Srinivas Kandagatla
@ 2020-10-01 21:49         ` Evan Green
  2020-10-05  9:12           ` Srinivas Kandagatla
  0 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-10-01 21:49 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Rob Herring, Bjorn Andersson, Douglas Anderson, Stephen Boyd, LKML

On Thu, Oct 1, 2020 at 9:30 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 01/10/2020 17:27, Evan Green wrote:
> > On Thu, Oct 1, 2020 at 7:17 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >> Hi Evan,
> >>
> >> On 29/09/2020 21:58, Evan Green 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 to attach the set of regions
> >>> we should not access. Then tiptoe around those regions.
> >>>
> >>
> >> This is a generic feature that can be used by any nvmem provider, can
> >> you move this logic to nvmem core instead of having it in qfprom!
> >
> > Sure! I'd prefer to keep this data in the driver for now rather than
> Ofcourse these can come from driver directly based on compatible!
>
> > trying to define DT bindings for the keepout zones. So then I'll pass
> > in my keepout array via struct nvmem_config at registration time, and
> > then the core can handle the keepout logic instead of qfprom.c.
> >
>
> Yes, that is inline with what am thinking of as well!

Oh no, I realized this isn't nearly as beautiful when I try to move it
into the core. The low level read/write interface between the nvmem
core and the driver is a range. So to move this into the core I'd have
to implement all the overlap computation logic to potentially break up
a read into several small reads in cases where there are many little
keepout ranges. It was much simpler when I could just check each byte
offset individually, and because I was doing it in this one
rarely-used driver I could make that performance tradeoff without much
penalty.

I could do all range/overlap handling if you want, but it'll be a
bigger change, and I worry my driver would be the only one to end up
using it. What do you think?
-Evan

>
>
> 00srini
> > -Evan
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
  2020-09-29 20:58 ` [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string Evan Green
@ 2020-10-02 22:20   ` Doug Anderson
  2020-10-02 23:14     ` Evan Green
  0 siblings, 1 reply; 13+ messages in thread
From: Doug Anderson @ 2020-10-02 22:20 UTC (permalink / raw)
  To: Evan Green
  Cc: Rob Herring, Bjorn Andersson, Srinivas Kandagatla, Stephen Boyd,
	Andy Gross,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-arm-msm, LKML

Hi,

On Tue, Sep 29, 2020 at 1:58 PM Evan Green <evgreen@chromium.org> wrote:
>
> Add an SoC-specific compatible string so that data can be attached
> to it in the driver.
>
> Signed-off-by: Evan Green <evgreen@chromium.org>
> ---
>
>  Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> index 59aca6d22ff9b..b16c8e6a8c23d 100644
> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> @@ -14,7 +14,9 @@ allOf:
>
>  properties:
>    compatible:
> -    const: qcom,qfprom
> +    enum:
> +      - qcom,qfprom
> +      - qcom,sc7180-qfprom

You don't want either/or.  You want both.  At the time Srinivas didn't
see the point of having the SoC-specific compatible string here, but
now that we have a reason for it maybe he'll be convinced?  IMO you
essentially want:

    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

For some context:
<https://lore.kernel.org/r/CAD=FV=WjvAWVmq3fTh=_f2p1Dv+sXg1RV-CqZr8KRgHe8_wT0w@mail.gmail.com/>

-Doug


>
>    reg:
>      # If the QFPROM is read-only OS image then only the corrected region
> --
> 2.26.2
>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string
  2020-09-29 20:58 ` [PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string Evan Green
@ 2020-10-02 22:24   ` Doug Anderson
  0 siblings, 0 replies; 13+ messages in thread
From: Doug Anderson @ 2020-10-02 22:24 UTC (permalink / raw)
  To: Evan Green
  Cc: Rob Herring, Bjorn Andersson, Srinivas Kandagatla, Stephen Boyd,
	Andy Gross,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-arm-msm, LKML

Hi,

On Tue, Sep 29, 2020 at 1:58 PM Evan Green <evgreen@chromium.org> wrote:
>
> 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>
> ---
>
>  arch/arm64/boot/dts/qcom/sc7180.dtsi | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
  2020-10-02 22:20   ` Doug Anderson
@ 2020-10-02 23:14     ` Evan Green
  2020-10-05  9:15       ` Srinivas Kandagatla
  0 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-10-02 23:14 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Rob Herring, Bjorn Andersson, Srinivas Kandagatla, Stephen Boyd,
	Andy Gross,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-arm-msm, LKML

On Fri, Oct 2, 2020 at 3:20 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Tue, Sep 29, 2020 at 1:58 PM Evan Green <evgreen@chromium.org> wrote:
> >
> > Add an SoC-specific compatible string so that data can be attached
> > to it in the driver.
> >
> > Signed-off-by: Evan Green <evgreen@chromium.org>
> > ---
> >
> >  Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > index 59aca6d22ff9b..b16c8e6a8c23d 100644
> > --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > @@ -14,7 +14,9 @@ allOf:
> >
> >  properties:
> >    compatible:
> > -    const: qcom,qfprom
> > +    enum:
> > +      - qcom,qfprom
> > +      - qcom,sc7180-qfprom
>
> You don't want either/or.  You want both.  At the time Srinivas didn't
> see the point of having the SoC-specific compatible string here, but
> now that we have a reason for it maybe he'll be convinced?  IMO you
> essentially want:
>
>     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
>
> For some context:
> <https://lore.kernel.org/r/CAD=FV=WjvAWVmq3fTh=_f2p1Dv+sXg1RV-CqZr8KRgHe8_wT0w@mail.gmail.com/>

That makes sense, thanks Doug.

Srini, do you want me to go fix up all the various device trees to add
the soc-compatible string, or just sc7180? (Also, don't forget about
my other question about whether you still want the keepout stuff in
the core at the cost of added complexity).

-Evan

>
> -Doug
>
>
> >
> >    reg:
> >      # If the QFPROM is read-only OS image then only the corrected region
> > --
> > 2.26.2
> >

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses
  2020-10-01 21:49         ` Evan Green
@ 2020-10-05  9:12           ` Srinivas Kandagatla
  0 siblings, 0 replies; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-10-05  9:12 UTC (permalink / raw)
  To: Evan Green
  Cc: Rob Herring, Bjorn Andersson, Douglas Anderson, Stephen Boyd, LKML



On 01/10/2020 22:49, Evan Green wrote:
> Oh no, I realized this isn't nearly as beautiful when I try to move it
> into the core. The low level read/write interface between the nvmem
> core and the driver is a range. So to move this into the core I'd have
> to implement all the overlap computation logic to potentially break up
> a read into several small reads in cases where there are many little
> keepout ranges. It was much simpler when I could just check each byte
> offset individually, and because I was doing it in this one
> rarely-used driver I could make that performance tradeoff without much
> penalty.
> 
> I could do all range/overlap handling if you want, but it'll be a
> bigger change, and I worry my driver would be the only one to end up
> using it. What do you think?

I still think this should go in to core. core should be able to avoid 
checking by the presence of any restricted range, so it should not be a 
overhead for other non-users. regmap has similar feature we can take 
some inspiration from that code!!


--srini


> -Evan

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string
  2020-10-02 23:14     ` Evan Green
@ 2020-10-05  9:15       ` Srinivas Kandagatla
  0 siblings, 0 replies; 13+ messages in thread
From: Srinivas Kandagatla @ 2020-10-05  9:15 UTC (permalink / raw)
  To: Evan Green, Doug Anderson
  Cc: Rob Herring, Bjorn Andersson, Stephen Boyd, Andy Gross,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	linux-arm-msm, LKML



On 03/10/2020 00:14, Evan Green wrote:
>> You don't want either/or.  You want both.  At the time Srinivas didn't
>> see the point of having the SoC-specific compatible string here, but
>> now that we have a reason for it maybe he'll be convinced?  IMO you
>> essentially want:
>>
>>      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
>>
>> For some context:
>> <https://lore.kernel.org/r/CAD=FV=WjvAWVmq3fTh=_f2p1Dv+sXg1RV-CqZr8KRgHe8_wT0w@mail.gmail.com/>
> That makes sense, thanks Doug.
> 
> Srini, do you want me to go fix up all the various device trees to add
> the soc-compatible string, or just sc7180? (Also, don't forget about
> my other question about whether you still want the keepout stuff in
> the core at the cost of added complexity).

Lets keep the existing users as it for now! and we can still list 
compatibles in the bindings so that any new users (including sc7180) can 
start using them when needed!

thanks,
srini


> 
> -Evan
> 

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2020-10-05  9:15 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-29 20:58 [PATCH 0/3] nvmem: qfprom: Avoid untouchable regions Evan Green
2020-09-29 20:58 ` [PATCH 1/3] dt-bindings: nvmem: Add qcom,sc7180-qfprom compatible string Evan Green
2020-10-02 22:20   ` Doug Anderson
2020-10-02 23:14     ` Evan Green
2020-10-05  9:15       ` Srinivas Kandagatla
2020-09-29 20:58 ` [PATCH 2/3] arm64: dts: qcom: sc7180: Add soc-specific qfprom compat string Evan Green
2020-10-02 22:24   ` Doug Anderson
2020-09-29 20:58 ` [PATCH 3/3] nvmem: qfprom: Don't touch certain fuses Evan Green
2020-10-01 14:17   ` Srinivas Kandagatla
2020-10-01 16:27     ` Evan Green
2020-10-01 16:30       ` Srinivas Kandagatla
2020-10-01 21:49         ` Evan Green
2020-10-05  9:12           ` Srinivas Kandagatla

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).