[RFC v1 0/3] Add QTI QFPROM-Efuse driver support

[RFC v1 0/3] Add QTI QFPROM-Efuse driver support

[Ravi Kumar Bokka]

This patch series adds qfprom-efuse controller driver support.

This driver can access the raw qfprom regions for fuse blowing.

The current existed qfprom driver is only supports for cpufreq, thermal sensors
drivers by read out calibration data, speed bins..etc which is stored by
qfprom efuses. 

Ravi Kumar Bokka (3):
  dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse
  drivers: nvmem: Add driver for QTI qfprom-efuse support
  arm64: dts: qcom: sc7180: Add qfprom-efuse

 .../devicetree/bindings/nvmem/qfprom-efuse.yaml    |  40 ++
 arch/arm64/boot/dts/qcom/sc7180-idp.dts            |   4 +
 arch/arm64/boot/dts/qcom/sc7180.dtsi               |   9 +
 drivers/nvmem/Kconfig                              |  10 +
 drivers/nvmem/Makefile                             |   2 +
 drivers/nvmem/qfprom-efuse.c                       | 476 +++++++++++++++++++++
 6 files changed, 541 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
 create mode 100644 drivers/nvmem/qfprom-efuse.c

-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

[RFC v1 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse

[Ravi Kumar Bokka]

This patch adds dt-bindings document for qfprom-efuse controller.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
---
 .../devicetree/bindings/nvmem/qfprom-efuse.yaml    | 40 ++++++++++++++++++++++
 1 file changed, 40 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml b/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
new file mode 100644
index 0000000..d262c99
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
@@ -0,0 +1,40 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/qfprom-efuse.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc, QFPROM Efuse bindings
+
+maintainers:
+  - Ravi Kumar Bokka <rbokka@codeaurora.org>
+
+allOf:
+  - $ref: "nvmem.yaml#"
+
+properties:
+  compatible:
+    enum:
+      - qcom,sc7180-qfprom-efuse
+
+  reg:
+    maxItems: 3
+
+required:
+   - compatible
+   - reg
+   - clocks
+   - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    efuse@780000 {
+	compatible = "qcom,sc7180-qfprom-efuse";
+        reg = <0 0x00780000 0 0x100>,
+              <0 0x00780120 0 0x7a0>,
+              <0 0x00782000 0 0x100>;
+        clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+        clock-names = "secclk";
+    };
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse

[Rob Herring]

On Tue, 12 May 2020 23:47:58 +0530, Ravi Kumar Bokka wrote:
> This patch adds dt-bindings document for qfprom-efuse controller.
> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> ---
>  .../devicetree/bindings/nvmem/qfprom-efuse.yaml    | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
> 


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

Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml:  while scanning a block scalar
  in "<unicode string>", line 30, column 5
found a tab character where an indentation space is expected
  in "<unicode string>", line 34, column 1
Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/nvmem/qfprom-efuse.example.dts' failed
make[1]: *** [Documentation/devicetree/bindings/nvmem/qfprom-efuse.example.dts] Error 1
make[1]: *** Waiting for unfinished jobs....
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml: ignoring, error parsing file
warning: no schema found in file: ./Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
Makefile:1300: recipe for target 'dt_binding_check' failed
make: *** [dt_binding_check] Error 2

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

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: [RFC v1 1/3] dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse

[Doug Anderson]

Hi,

On Tue, May 12, 2020 at 11:18 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch adds dt-bindings document for qfprom-efuse controller.
>
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> ---
>  .../devicetree/bindings/nvmem/qfprom-efuse.yaml    | 40 ++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
>
> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml b/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
> new file mode 100644
> index 0000000..d262c99
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
> @@ -0,0 +1,40 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/qfprom-efuse.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc, QFPROM Efuse bindings
> +
> +maintainers:
> +  - Ravi Kumar Bokka <rbokka@codeaurora.org>
> +
> +allOf:
> +  - $ref: "nvmem.yaml#"
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sc7180-qfprom-efuse
> +
> +  reg:
> +    maxItems: 3

Instead of this, add descriptions for the 3 items.  AKA:

reg:
  items:
    - description: The base of the qfprom.
    - description: The start of the raw region.
    - description: The start of the mem region.

...but do you really need to break this down into 3 ranges?  Why can't
you just do:

reg = <0 0x00780000 0 0x2100>;

Then you really don't need any description and you'd just have:

reg:
  maxItems: 1


> +

Need something for clocks and clock-names, like:

clocks:
  maxItems: 1

clock-names:
  items:
  - const: sec


> +required:
> +   - compatible
> +   - reg
> +   - clocks
> +   - clock-names
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    efuse@780000 {
> +       compatible = "qcom,sc7180-qfprom-efuse";
> +        reg = <0 0x00780000 0 0x100>,
> +              <0 0x00780120 0 0x7a0>,
> +              <0 0x00782000 0 0x100>;
> +        clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> +        clock-names = "secclk";

nit: Folks usually don't like the names of clocks to end in "clk".  We
know it's a clock.  Just name this "sec" or even a local name like
"core".

[RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Ravi Kumar Bokka]

This patch adds new driver for QTI qfprom-efuse controller. This driver can
access the raw qfprom regions for fuse blowing.

The current existed qfprom driver is only supports for cpufreq, thermal sensors
drivers by read out calibration data, speed bins..etc which is stored
by qfprom efuses.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
---
 drivers/nvmem/Kconfig        |  10 +
 drivers/nvmem/Makefile       |   2 +
 drivers/nvmem/qfprom-efuse.c | 476 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 488 insertions(+)
 create mode 100644 drivers/nvmem/qfprom-efuse.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index d7b7f6d..c9345c5 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -121,6 +121,16 @@ config QCOM_QFPROM
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem_qfprom.
 
+config QTI_QFPROM_EFUSE
+	tristate "QTI_QFPROM_EFUSE Support"
+	depends on HAS_IOMEM
+	help
+	  Say y here to enable QFPROM-Efuse support. This driver provides access
+          QTI qfprom efuse via nvmem interface.
+
+          This driver can also be built as a module. If so, the module
+          will be called nvmem_qfprom-efuse.
+
 config NVMEM_SPMI_SDAM
 	tristate "SPMI SDAM Support"
 	depends on SPMI
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index a7c3772..1d8fe43 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -27,6 +27,8 @@ obj-$(CONFIG_MTK_EFUSE)		+= nvmem_mtk-efuse.o
 nvmem_mtk-efuse-y		:= mtk-efuse.o
 obj-$(CONFIG_QCOM_QFPROM)	+= nvmem_qfprom.o
 nvmem_qfprom-y			:= qfprom.o
+obj-$(CONFIG_QTI_QFPROM_EFUSE)	+= nvmem_qfprom-efuse.o
+nvmem_qfprom-efuse-y		:= qfprom-efuse.o
 obj-$(CONFIG_NVMEM_SPMI_SDAM)	+= nvmem_qcom-spmi-sdam.o
 nvmem_qcom-spmi-sdam-y		+= qcom-spmi-sdam.o
 obj-$(CONFIG_ROCKCHIP_EFUSE)	+= nvmem_rockchip_efuse.o
diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
new file mode 100644
index 0000000..2e3c275
--- /dev/null
+++ b/drivers/nvmem/qfprom-efuse.c
@@ -0,0 +1,476 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2020, The Linux Foundation. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+#define QFPROM_BLOW_STATUS_BUSY 0x1
+#define QFPROM_BLOW_STATUS_READY 0x0
+
+/* Blow timer clock frequency in Mhz for 10nm LPe technology */
+#define QFPROM_BLOW_TIMER_OFFSET 0x03c
+#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
+
+/* Amount of time required to hold charge to blow fuse in micro-seconds */
+#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
+#define QFPROM_BLOW_STATUS_OFFSET 0x048
+
+#define QFPROM_ACCEL_OFFSET 0x044
+
+/**
+ * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
+ * platform data
+ *
+ * @name: qfprom-efuse compatible name
+ * @fuse_blow_time_in_us: Should contain the wait time when doing the fuse blow
+ * @accel_value: Should contain qfprom accel value
+ * @accel_reset_value: The reset value of qfprom accel value
+ * @qfprom_blow_timer_value: The timer value of qfprom when doing efuse blow
+ * @qfprom_blow_reset_freq: The frequency required to set when fuse blowing
+ * is done
+ * @qfprom_blow_set_freq: The frequency required to set when we start the
+ * fuse blowing
+ * @qfprom_max_vol: max voltage required to set fuse blow
+ * @qfprom_min_vol: min voltage required to set fuse blow
+ */
+struct qfprom_efuse_platform_data {
+	const char *name;
+	u8 fuse_blow_time_in_us;
+	u32 accel_value;
+	u32 accel_reset_value;
+	u32 qfprom_blow_timer_value;
+	u32 qfprom_blow_reset_freq;
+	u32 qfprom_blow_set_freq;
+	u32 qfprom_max_vol;
+	u32 qfprom_min_vol;
+};
+
+/**
+ * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
+ *
+ * @qfpbase: iomapped memory space for qfprom base
+ * @qfpraw: iomapped memory space for qfprom raw fuse region
+ * @qfpmap: iomapped memory space for qfprom fuse blow timer
+
+ * @dev: qfprom device structure
+ * @secclk: clock supply
+ * @vcc: regulator supply
+
+ * @qfpraw_start: qfprom raw fuse start region
+ * @qfpraw_end: qfprom raw fuse end region
+ * @qfprom_efuse_platform_data: qfprom platform data
+ */
+struct qfprom_efuse_priv {
+	void __iomem *qfpbase;
+	void __iomem *qfpraw;
+	void __iomem *qfpmap;
+	struct device *dev;
+	struct clk *secclk;
+	struct regulator *vcc;
+	resource_size_t qfpraw_start;
+	resource_size_t qfpraw_end;
+	struct qfprom_efuse_platform_data efuse;
+};
+
+/*
+ * restore the gcc_sec_ctrl_clk frequency to default value(19.2 MHz)
+ */
+static int qfprom_reset_clock_settings(const struct qfprom_efuse_priv *priv)
+{
+	int ret;
+
+	ret = clk_set_rate(priv->secclk, priv->efuse.qfprom_blow_reset_freq);
+	if (ret) {
+		dev_err(priv->dev, "clk_set_rate() failed to enable secclk\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * set the gcc_sec_ctrl_clk to 4.8 MHz
+ */
+static int qfprom_set_clock_settings(const struct qfprom_efuse_priv *priv)
+{
+	int ret;
+
+	ret = clk_set_rate(priv->secclk, priv->efuse.qfprom_blow_set_freq);
+
+	if (ret) {
+		dev_err(priv->dev, "clk_set_rate() failed to enable secclk\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * set and reset the voltage for 1.8V and OFF(0V) on VDD_QFPROM (LDO11)
+ */
+static int qfprom_set_voltage_settings(const struct qfprom_efuse_priv *priv,
+				       int min_uV, int max_uV)
+{
+	int ret;
+
+	ret = regulator_set_voltage(priv->vcc, min_uV, max_uV);
+
+	if (ret) {
+		dev_err(priv->dev, "regulator_set_voltage() failed!\n");
+		return ret;
+	}
+
+	ret = regulator_enable(priv->vcc);
+	if (ret) {
+		dev_err(priv->dev, "failed to enable regulator\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * resets the value of the blow timer, accel register and the clock
+ * and voltage settings
+ */
+static int qfprom_disable_fuse_blowing(const struct qfprom_efuse_priv *priv)
+{
+	int ret;
+
+	ret = qfprom_set_voltage_settings(priv, 0, priv->efuse.qfprom_max_vol);
+	if (ret) {
+		dev_err(priv->dev, "qfprom_set_voltage_settings failed\n");
+		return ret;
+	}
+
+	ret = qfprom_reset_clock_settings(priv);
+	if (ret) {
+		dev_err(priv->dev, "qfprom_reset_clock_settings failed\n");
+		return ret;
+	}
+
+	writel(QFPROM_BLOW_TIMER_RESET_VALUE, priv->qfpmap +
+		  QFPROM_BLOW_TIMER_OFFSET);
+	writel(priv->efuse.accel_reset_value,
+	       priv->qfpmap + QFPROM_ACCEL_OFFSET);
+
+	return 0;
+}
+
+/*
+ * sets the value of the blow timer, accel register and the clock
+ * and voltage settings
+ */
+static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv *priv)
+{
+	int ret;
+
+	ret = qfprom_disable_fuse_blowing(priv);
+	if (ret) {
+		dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
+		return ret;
+	}
+
+	writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
+	       QFPROM_BLOW_TIMER_OFFSET);
+	writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
+
+	ret = qfprom_set_clock_settings(priv);
+	if (ret) {
+		dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
+		return ret;
+	}
+
+	ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
+					  priv->efuse.qfprom_max_vol);
+	if (ret) {
+		dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+/*
+ * verifying to make sure address being written or read is from qfprom
+ * raw address range
+ */
+bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
+			  size_t bytes)
+{
+	if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
+	    ((reg + bytes) <= priv->qfpraw_end)) {
+		return 1;
+	}
+
+	return 0;
+}
+
+/*
+ * API for reading from raw qfprom region
+ */
+static int qfprom_efuse_reg_read(void *context, unsigned int reg, void *_val,
+				 size_t bytes)
+{
+	struct qfprom_efuse_priv *priv = context;
+	u32 *value = _val;
+	u32 align_check;
+	int i = 0, words = bytes / 4;
+
+	dev_info(priv->dev,
+		 "reading raw qfprom region offset: 0x%08x of size: %zd\n",
+		 reg, bytes);
+
+	if (bytes % 4 != 0x00) {
+		dev_err(priv->dev,
+			"Bytes: %zd to read should be word align\n",
+			bytes);
+		return -EINVAL;
+	}
+
+	if (!addr_in_qfprom_range(priv, reg, bytes)) {
+		dev_err(priv->dev,
+			"Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
+			reg, bytes);
+		return -EINVAL;
+	}
+
+	align_check = (reg & 0xF);
+
+	if (((align_check & ~3) == align_check) && value != NULL)
+		while (words--)
+			*value++ = readl(priv->qfpbase + reg + (i++ * 4));
+
+	else
+		dev_err(priv->dev,
+			"Invalid input parameter 0x%08x fuse blow address\n",
+			reg);
+
+	return 0;
+}
+
+/*
+ * API for writing to raw qfprom region - fuse blowing
+ * returns success or failure code as per the conditions
+ */
+static int qfprom_efuse_reg_write(void *context, unsigned int reg, void *_val,
+				  size_t bytes)
+{
+	struct qfprom_efuse_priv *priv = context;
+	u32 *value = _val;
+	u32 align_check;
+	u32 blow_status = QFPROM_BLOW_STATUS_BUSY;
+	int ret;
+	int i = 0, words = bytes / 4;
+
+	dev_info(priv->dev,
+		 "writing to raw qfprom region : 0x%08x of size: %zd\n",
+		 reg, bytes);
+
+	if (bytes % 4 != 0x00) {
+		dev_err(priv->dev, "Bytes: %zd should be word align\n", bytes);
+		return -EINVAL;
+	}
+
+	if (!addr_in_qfprom_range(priv, reg, bytes)) {
+		dev_err(priv->dev,
+			"Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
+			 reg, bytes);
+		return -EINVAL;
+	}
+
+	align_check = (reg & 0xF);
+	if (value && ((align_check & ~3) == align_check)) {
+		ret = qfprom_enable_fuse_blowing(priv);
+		if (ret) {
+			dev_err(priv->dev, "qfprom_enable_fuse_blowing");
+			return ret;
+		}
+
+		ret = readl_relaxed_poll_timeout(priv->qfpmap +
+				QFPROM_BLOW_STATUS_OFFSET, blow_status,
+				(blow_status  != QFPROM_BLOW_STATUS_BUSY),
+				QFPROM_FUSE_BLOW_POLL_PERIOD,
+				priv->efuse.fuse_blow_time_in_us);
+
+		if (ret) {
+			dev_err(priv->dev, "Timeout blow status ready\n");
+			return ret;
+		}
+
+		if (blow_status == QFPROM_BLOW_STATUS_READY) {
+			while (words--)
+				writel(*value++,
+				       priv->qfpbase + reg + (i++ * 4));
+
+			ret = readl_relaxed_poll_timeout(priv->qfpmap +
+				QFPROM_BLOW_STATUS_OFFSET, blow_status,
+				(blow_status  != QFPROM_BLOW_STATUS_BUSY),
+				QFPROM_FUSE_BLOW_POLL_PERIOD,
+				priv->efuse.fuse_blow_time_in_us);
+
+			if (ret) {
+				dev_err(priv->dev, "Timeout blow-status ready");
+				return ret;
+			}
+		}
+
+		ret = qfprom_disable_fuse_blowing(priv);
+		if (ret)
+			return ret;
+	} else {
+		dev_err(priv->dev, "Invalid input parameter fuse blow address");
+		return -EINVAL;
+	}
+
+	dev_info(priv->dev, "written successfully raw qfprom region\n");
+
+	return 0;
+}
+
+static int qfprom_efuse_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct resource *qfpbase, *qfpraw, *qfpmap;
+	struct nvmem_device *nvmem;
+	struct nvmem_config *econfig;
+	struct qfprom_efuse_priv *priv;
+	const struct qfprom_efuse_platform_data *drvdata;
+	int ret;
+
+	dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
+
+	drvdata = of_device_get_match_data(&pdev->dev);
+	if (!drvdata)
+		return -EINVAL;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
+	priv->efuse.accel_value = drvdata->accel_value;
+	priv->efuse.accel_reset_value = drvdata->accel_reset_value;
+	priv->efuse.qfprom_blow_timer_value = drvdata->qfprom_blow_timer_value;
+	priv->efuse.qfprom_blow_reset_freq = drvdata->qfprom_blow_reset_freq;
+	priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
+	priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
+	priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
+	priv->dev = dev;
+
+	qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+
+	priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
+	if (IS_ERR(priv->qfpbase)) {
+		ret = PTR_ERR(priv->qfpbase);
+		goto err;
+	}
+
+	qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+
+	priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
+	if (IS_ERR(priv->qfpraw)) {
+		ret = PTR_ERR(priv->qfpraw);
+		goto err;
+	}
+
+	priv->qfpraw_start = qfpraw->start - qfpbase->start;
+	priv->qfpraw_end = qfpraw->end - qfpbase->start;
+
+	qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+
+	priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
+	if (IS_ERR(priv->qfpmap)) {
+		ret = PTR_ERR(priv->qfpmap);
+		goto err;
+	}
+
+	priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
+	if (IS_ERR(priv->vcc)) {
+		ret = PTR_ERR(priv->vcc);
+		if (ret == -ENODEV)
+			ret = -EPROBE_DEFER;
+
+		goto err;
+	}
+
+	priv->secclk = devm_clk_get(dev, "secclk");
+	if (IS_ERR(priv->secclk)) {
+		ret = PTR_ERR(priv->secclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "secclk error getting : %d\n", ret);
+		goto err;
+	}
+
+	ret = clk_prepare_enable(priv->secclk);
+	if (ret) {
+		dev_err(dev, "clk_prepare_enable() failed\n");
+		goto err;
+	}
+
+	econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
+	if (!econfig)
+		return -ENOMEM;
+
+	econfig->dev = dev;
+	econfig->name = "qfprom-efuse";
+	econfig->stride = 1;
+	econfig->word_size = 1;
+	econfig->reg_read = qfprom_efuse_reg_read;
+	econfig->reg_write = qfprom_efuse_reg_write;
+	econfig->size = resource_size(qfpraw);
+	econfig->priv = priv;
+
+	nvmem = devm_nvmem_register(dev, econfig);
+
+	return PTR_ERR_OR_ZERO(nvmem);
+
+err:
+	clk_disable_unprepare(priv->secclk);
+	return ret;
+}
+
+static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
+	.name = "sc7180-qfprom-efuse",
+	.fuse_blow_time_in_us = 10,
+	.accel_value = 0xD10,
+	.accel_reset_value = 0x800,
+	.qfprom_blow_timer_value = 25,
+	.qfprom_blow_reset_freq = 19200000,
+	.qfprom_blow_set_freq = 4800000,
+	.qfprom_max_vol = 1904000,
+	.qfprom_min_vol = 1800000,
+};
+
+static const struct of_device_id qfprom_efuse_of_match[] = {
+	{
+		.compatible = "qcom,sc7180-qfprom-efuse",
+		.data = &sc7180_qfp_efuse_data
+	},
+	{/* sentinel */},
+};
+
+MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
+
+static struct platform_driver qfprom_efuse_driver = {
+	.probe = qfprom_efuse_probe,
+	.driver = {
+		.name = "sc7180-qfprom-efuse",
+		.of_match_table = qfprom_efuse_of_match,
+	},
+};
+
+module_platform_driver(qfprom_efuse_driver);
+MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
+MODULE_LICENSE("GPL v2");
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Randy Dunlap]

On 5/12/20 11:17 AM, Ravi Kumar Bokka wrote:
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index d7b7f6d..c9345c5 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -121,6 +121,16 @@ config QCOM_QFPROM
>  	  This driver can also be built as a module. If so, the module
>  	  will be called nvmem_qfprom.
>  
> +config QTI_QFPROM_EFUSE
> +	tristate "QTI_QFPROM_EFUSE Support"
> +	depends on HAS_IOMEM
> +	help
> +	  Say y here to enable QFPROM-Efuse support. This driver provides access

	                                                                  access to

> +          QTI qfprom efuse via nvmem interface.
> +
> +          This driver can also be built as a module. If so, the module
> +          will be called nvmem_qfprom-efuse.

The last 3 non-blank lines should be indented with one tab + 2 spaces
instead of lots of spaces.

> +
>  config NVMEM_SPMI_SDAM
>  	tristate "SPMI SDAM Support"
>  	depends on SPMI


-- 
~Randy

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,


On Tue, May 12, 2020 at 11:19 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch adds new driver for QTI qfprom-efuse controller. This driver can
> access the raw qfprom regions for fuse blowing.
>
> The current existed qfprom driver is only supports for cpufreq, thermal sensors
> drivers by read out calibration data, speed bins..etc which is stored
> by qfprom efuses.
>
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> ---
>  drivers/nvmem/Kconfig        |  10 +
>  drivers/nvmem/Makefile       |   2 +
>  drivers/nvmem/qfprom-efuse.c | 476 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 488 insertions(+)
>  create mode 100644 drivers/nvmem/qfprom-efuse.c
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index d7b7f6d..c9345c5 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -121,6 +121,16 @@ config QCOM_QFPROM
>           This driver can also be built as a module. If so, the module
>           will be called nvmem_qfprom.
>
> +config QTI_QFPROM_EFUSE

nit: it's really weird that the config above has the "QCOM" prefix and
you have "QTI".  Can you just use "QCOM"?


> +       tristate "QTI_QFPROM_EFUSE Support"
> +       depends on HAS_IOMEM
> +       help
> +         Say y here to enable QFPROM-Efuse support. This driver provides access
> +          QTI qfprom efuse via nvmem interface.
> +
> +          This driver can also be built as a module. If so, the module
> +          will be called nvmem_qfprom-efuse.
> +
>  config NVMEM_SPMI_SDAM
>         tristate "SPMI SDAM Support"
>         depends on SPMI
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index a7c3772..1d8fe43 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -27,6 +27,8 @@ obj-$(CONFIG_MTK_EFUSE)               += nvmem_mtk-efuse.o
>  nvmem_mtk-efuse-y              := mtk-efuse.o
>  obj-$(CONFIG_QCOM_QFPROM)      += nvmem_qfprom.o
>  nvmem_qfprom-y                 := qfprom.o
> +obj-$(CONFIG_QTI_QFPROM_EFUSE) += nvmem_qfprom-efuse.o
> +nvmem_qfprom-efuse-y           := qfprom-efuse.o
>  obj-$(CONFIG_NVMEM_SPMI_SDAM)  += nvmem_qcom-spmi-sdam.o
>  nvmem_qcom-spmi-sdam-y         += qcom-spmi-sdam.o
>  obj-$(CONFIG_ROCKCHIP_EFUSE)   += nvmem_rockchip_efuse.o
> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
> new file mode 100644
> index 0000000..2e3c275
> --- /dev/null
> +++ b/drivers/nvmem/qfprom-efuse.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define QFPROM_BLOW_STATUS_BUSY 0x1
> +#define QFPROM_BLOW_STATUS_READY 0x0
> +
> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
> +
> +/* Amount of time required to hold charge to blow fuse in micro-seconds */
> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100

Am I reading this correctly that your timeout is 10 microseconds and
you're polling every 100 microseconds?  That seems weird.  Why not
just poll every "fuse_blow_time_in_us / 10" or something?


> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
> +
> +#define QFPROM_ACCEL_OFFSET 0x044
> +
> +/**
> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
> + * platform data
> + *
> + * @name: qfprom-efuse compatible name
> + * @fuse_blow_time_in_us: Should contain the wait time when doing the fuse blow
> + * @accel_value: Should contain qfprom accel value
> + * @accel_reset_value: The reset value of qfprom accel value
> + * @qfprom_blow_timer_value: The timer value of qfprom when doing efuse blow
> + * @qfprom_blow_reset_freq: The frequency required to set when fuse blowing
> + * is done
> + * @qfprom_blow_set_freq: The frequency required to set when we start the
> + * fuse blowing
> + * @qfprom_max_vol: max voltage required to set fuse blow
> + * @qfprom_min_vol: min voltage required to set fuse blow
> + */
> +struct qfprom_efuse_platform_data {
> +       const char *name;

You don't need the name for anything.  Remove.


> +       u8 fuse_blow_time_in_us;
> +       u32 accel_value;
> +       u32 accel_reset_value;
> +       u32 qfprom_blow_timer_value;
> +       u32 qfprom_blow_reset_freq;
> +       u32 qfprom_blow_set_freq;
> +       u32 qfprom_max_vol;
> +       u32 qfprom_min_vol;
> +};
> +
> +/**
> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
> + *
> + * @qfpbase: iomapped memory space for qfprom base
> + * @qfpraw: iomapped memory space for qfprom raw fuse region
> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
> +

Please error-check your kernel-docs with:
  scripts/kernel-doc -rst drivers/nvmem/qfprom-efuse.c > /dev/null

It reports (among other things) that you have a bad line here.


> + * @dev: qfprom device structure
> + * @secclk: clock supply
> + * @vcc: regulator supply
> +
> + * @qfpraw_start: qfprom raw fuse start region
> + * @qfpraw_end: qfprom raw fuse end region
> + * @qfprom_efuse_platform_data: qfprom platform data
> + */
> +struct qfprom_efuse_priv {
> +       void __iomem *qfpbase;
> +       void __iomem *qfpraw;
> +       void __iomem *qfpmap;
> +       struct device *dev;
> +       struct clk *secclk;
> +       struct regulator *vcc;
> +       resource_size_t qfpraw_start;
> +       resource_size_t qfpraw_end;
> +       struct qfprom_efuse_platform_data efuse;
> +};
> +
> +/*
> + * restore the gcc_sec_ctrl_clk frequency to default value(19.2 MHz)
> + */
> +static int qfprom_reset_clock_settings(const struct qfprom_efuse_priv *priv)
> +{
> +       int ret;
> +
> +       ret = clk_set_rate(priv->secclk, priv->efuse.qfprom_blow_reset_freq);
> +       if (ret) {
> +               dev_err(priv->dev, "clk_set_rate() failed to enable secclk\n");
> +               return ret;
> +       }
> +
> +       return 0;

Slightly shorter:

ret = clk_set_rate(...);
if (ret)
  dev_err(...);

return ret;

> +}
> +
> +/*
> + * set the gcc_sec_ctrl_clk to 4.8 MHz
> + */
> +static int qfprom_set_clock_settings(const struct qfprom_efuse_priv *priv)
> +{
> +       int ret;
> +
> +       ret = clk_set_rate(priv->secclk, priv->efuse.qfprom_blow_set_freq);
> +

nit: remove extra blank line.

> +       if (ret) {
> +               dev_err(priv->dev, "clk_set_rate() failed to enable secclk\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * set and reset the voltage for 1.8V and OFF(0V) on VDD_QFPROM (LDO11)
> + */
> +static int qfprom_set_voltage_settings(const struct qfprom_efuse_priv *priv,
> +                                      int min_uV, int max_uV)
> +{
> +       int ret;
> +
> +       ret = regulator_set_voltage(priv->vcc, min_uV, max_uV);
> +

nit: remove extra blank line.

> +       if (ret) {
> +               dev_err(priv->dev, "regulator_set_voltage() failed!\n");
> +               return ret;
> +       }

See above where I suggest that you shouldn't be playing with the
voltage at all.  Just enable and disable.  Then get rid of this
function and callers should just call regulator_enable() /
regulator_disable().


> +
> +       ret = regulator_enable(priv->vcc);

You have regulator_enable() in both the turn on and turn off case.
Presumably that means that every time your code is called there's
another refcount keeping this regulator on.  That's definitely a bug.


> +       if (ret) {
> +               dev_err(priv->dev, "failed to enable regulator\n");
> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * resets the value of the blow timer, accel register and the clock
> + * and voltage settings
> + */
> +static int qfprom_disable_fuse_blowing(const struct qfprom_efuse_priv *priv)
> +{
> +       int ret;
> +
> +       ret = qfprom_set_voltage_settings(priv, 0, priv->efuse.qfprom_max_vol);
> +       if (ret) {
> +               dev_err(priv->dev, "qfprom_set_voltage_settings failed\n");

qfprom_set_voltage_settings() already printed an error.  Don't need to
print all the way down.


> +               return ret;
> +       }
> +
> +       ret = qfprom_reset_clock_settings(priv);
> +       if (ret) {
> +               dev_err(priv->dev, "qfprom_reset_clock_settings failed\n");

qfprom_reset_clock_settings() already printed an error.  Don't need to
print all the way down.


> +               return ret;
> +       }
> +
> +       writel(QFPROM_BLOW_TIMER_RESET_VALUE, priv->qfpmap +
> +                 QFPROM_BLOW_TIMER_OFFSET);
> +       writel(priv->efuse.accel_reset_value,
> +              priv->qfpmap + QFPROM_ACCEL_OFFSET);
> +
> +       return 0;
> +}
> +
> +/*
> + * sets the value of the blow timer, accel register and the clock
> + * and voltage settings
> + */
> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv *priv)
> +{
> +       int ret;
> +
> +       ret = qfprom_disable_fuse_blowing(priv);
> +       if (ret) {
> +               dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
> +               return ret;
> +       }

Why is the first line of qfprom_enable_fuse_blowing() a call to
qfprom_disable_fuse_blowing()?  That doesn't make sense.  Please
remove.


> +
> +       writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
> +              QFPROM_BLOW_TIMER_OFFSET);
> +       writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
> +
> +       ret = qfprom_set_clock_settings(priv);
> +       if (ret) {
> +               dev_err(priv->dev, "qpfrom_set_clock_settings()\n");

Presumably you should undo your writel() in the error case.

qfprom_set_clock_settings() already printed an error.  Don't need to
print all the way down.


> +               return ret;
> +       }
> +
> +       ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
> +                                         priv->efuse.qfprom_max_vol);
> +       if (ret) {
> +               dev_err(priv->dev, "qfprom_set_voltage_settings()\n");

Presumably you should undo your writel() and clock settings in the error case.

qfprom_set_voltage_settings() already printed an error.  Don't need to
print all the way down.

> +               return ret;
> +       }
> +
> +       return 0;
> +}
> +
> +/*
> + * verifying to make sure address being written or read is from qfprom
> + * raw address range
> + */
> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
> +                         size_t bytes)

Function should be static.



> +{
> +       if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&

The ((reg + bytes) > reg) is a weird check.  I guess you're checking
for overflow?  If nothing else this deserves a comment.


> +           ((reg + bytes) <= priv->qfpraw_end)) {
> +               return 1;
> +       }
> +
> +       return 0;

No need to convert from bool to bool with an if statement.  Your code
reads like:

if (a)
  return true;
return false;

When instead you can just say:

return a;


...but also I think this function doesn't actually make sense at all.
You should rely on the nvmem size checking and also have the client's
concept of the register range start at 0.  See below.


> +}
> +
> +/*
> + * API for reading from raw qfprom region
> + */
> +static int qfprom_efuse_reg_read(void *context, unsigned int reg, void *_val,
> +                                size_t bytes)
> +{
> +       struct qfprom_efuse_priv *priv = context;
> +       u32 *value = _val;

Is it really legal to assume the buffer passed is word-aligned?
Someone with more nvmem subsystem experience might know better than I
do, but I wouldn't assume this.


> +       u32 align_check;
> +       int i = 0, words = bytes / 4;
> +
> +       dev_info(priv->dev,
> +                "reading raw qfprom region offset: 0x%08x of size: %zd\n",
> +                reg, bytes);
> +
> +       if (bytes % 4 != 0x00) {
> +               dev_err(priv->dev,
> +                       "Bytes: %zd to read should be word align\n",
> +                       bytes);
> +               return -EINVAL;
> +       }

You should be able to get rid of the above check if you set word_size
to 4, right?


> +
> +       if (!addr_in_qfprom_range(priv, reg, bytes)) {

It feels like you should just rely on the "nvmem->size" in the "econfig".


> +               dev_err(priv->dev,
> +                       "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
> +                       reg, bytes);

This doesn't feel like the kind of case that should be printing an
error message.  It should be up to the client to do that.  Just
silently return -EINVAL.


> +               return -EINVAL;
> +       }
> +
> +       align_check = (reg & 0xF);
> +
> +       if (((align_check & ~3) == align_check) && value != NULL)

Rather than all this align check, shouldn't you just be able to set
"stride" to 4?  As far as I understand it then the core should handle
this guarantee.  If the core isn't and it's causing you problems then
you should submit a patch to the core.  If (for some reason) you still
need an alignment check, it seems like a much less roundabout way to
do your calculations is "if (reg & 0x3)"

Also: IMO there's no need to check value against NULL.  The client is
other Linux code, not userspace.  It can be trusted not to give you a
NULL buffer.  Worst case if it does is that you get a nice clean
crash.


> +               while (words--)
> +                       *value++ = readl(priv->qfpbase + reg + (i++ * 4));

Adding "reg" to "qfpbase" really seems to violate some abstractions.
Do you have 1 memory range, or do you have 3?  Here you're taking a
register that's in the raw range but accessing it as an offset from
the "qfp_sec_base".  IMO you should just define a single register
range.  If you really need 3 register ranges then you should treat
them separately.

Note also that it feels like you're exposing the wrong number to the
client.  If a client wants to access byte 0 of your range they need to
pass 0x120, right?  It feels like the client should just be passing 0
and (if you need to) you should worry about offsetting.

Also: I think you can use ioread32_rep() to do your reading rather than
open-coding, right?


> +
> +       else
> +               dev_err(priv->dev,
> +                       "Invalid input parameter 0x%08x fuse blow address\n",
> +                       reg);
> +
> +       return 0;
> +}
> +
> +/*
> + * API for writing to raw qfprom region - fuse blowing
> + * returns success or failure code as per the conditions
> + */
> +static int qfprom_efuse_reg_write(void *context, unsigned int reg, void *_val,
> +                                 size_t bytes)
> +{
> +       struct qfprom_efuse_priv *priv = context;
> +       u32 *value = _val;

Same comments as read wondering whether it's legit to assume that the
buffer passed in is word-aligned.


> +       u32 align_check;
> +       u32 blow_status = QFPROM_BLOW_STATUS_BUSY;

Don't init values if you don't need to.


> +       int ret;
> +       int i = 0, words = bytes / 4;
> +
> +       dev_info(priv->dev,
> +                "writing to raw qfprom region : 0x%08x of size: %zd\n",
> +                reg, bytes);
> +
> +       if (bytes % 4 != 0x00) {
> +               dev_err(priv->dev, "Bytes: %zd should be word align\n", bytes);
> +               return -EINVAL;
> +       }

Same comments as in read about using word_size.


> +
> +       if (!addr_in_qfprom_range(priv, reg, bytes)) {
> +               dev_err(priv->dev,
> +                       "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
> +                        reg, bytes);
> +               return -EINVAL;
> +       }

Same comments as in read.


> +
> +       align_check = (reg & 0xF);
> +       if (value && ((align_check & ~3) == align_check)) {
> +               ret = qfprom_enable_fuse_blowing(priv);
> +               if (ret) {
> +                       dev_err(priv->dev, "qfprom_enable_fuse_blowing");
> +                       return ret;
> +               }

Same comments as in read.


> +
> +               ret = readl_relaxed_poll_timeout(priv->qfpmap +
> +                               QFPROM_BLOW_STATUS_OFFSET, blow_status,
> +                               (blow_status  != QFPROM_BLOW_STATUS_BUSY),
> +                               QFPROM_FUSE_BLOW_POLL_PERIOD,
> +                               priv->efuse.fuse_blow_time_in_us);
> +
> +               if (ret) {
> +                       dev_err(priv->dev, "Timeout blow status ready\n");
> +                       return ret;
> +               }

Need to disable fuse blowing in the error case.  Right now you leave
the regulator and clock enabled.


> +
> +               if (blow_status == QFPROM_BLOW_STATUS_READY) {

...and what happens if blow_status is no longer busy (so you didn't
timeout) but it's not QFPROM_BLOW_STATUS_READY?  Is that even
possible?  If not then remove this check.  If so then (presumably?)
you need to return an error.


> +                       while (words--)
> +                               writel(*value++,
> +                                      priv->qfpbase + reg + (i++ * 4));

Similar comments as in read that you should be able to use iowrite32_rep()


> +
> +                       ret = readl_relaxed_poll_timeout(priv->qfpmap +
> +                               QFPROM_BLOW_STATUS_OFFSET, blow_status,
> +                               (blow_status  != QFPROM_BLOW_STATUS_BUSY),
> +                               QFPROM_FUSE_BLOW_POLL_PERIOD,
> +                               priv->efuse.fuse_blow_time_in_us);
> +
> +                       if (ret) {
> +                               dev_err(priv->dev, "Timeout blow-status ready");

Need to disable fuse blowing in the error case.  Right now you leave
the regulator and clock enabled.


> +                               return ret;
> +                       }
> +               }
> +
> +               ret = qfprom_disable_fuse_blowing(priv);
> +               if (ret)
> +                       return ret;
> +       } else {
> +               dev_err(priv->dev, "Invalid input parameter fuse blow address");
> +               return -EINVAL;
> +       }
> +
> +       dev_info(priv->dev, "written successfully raw qfprom region\n");
> +
> +       return 0;
> +}
> +
> +static int qfprom_efuse_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct resource *qfpbase, *qfpraw, *qfpmap;
> +       struct nvmem_device *nvmem;
> +       struct nvmem_config *econfig;
> +       struct qfprom_efuse_priv *priv;
> +       const struct qfprom_efuse_platform_data *drvdata;
> +       int ret;
> +
> +       dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);

Please remove.  Your driver doesn't need to announce itself.


> +
> +       drvdata = of_device_get_match_data(&pdev->dev);
> +       if (!drvdata)
> +               return -EINVAL;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
> +       priv->efuse.accel_value = drvdata->accel_value;
> +       priv->efuse.accel_reset_value = drvdata->accel_reset_value;
> +       priv->efuse.qfprom_blow_timer_value = drvdata->qfprom_blow_timer_value;
> +       priv->efuse.qfprom_blow_reset_freq = drvdata->qfprom_blow_reset_freq;
> +       priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
> +       priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
> +       priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
> +       priv->dev = dev;
> +
> +       qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +       priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
> +       if (IS_ERR(priv->qfpbase)) {
> +               ret = PTR_ERR(priv->qfpbase);
> +               goto err;
> +       }
> +
> +       qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +
> +       priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
> +       if (IS_ERR(priv->qfpraw)) {
> +               ret = PTR_ERR(priv->qfpraw);
> +               goto err;
> +       }
> +
> +       priv->qfpraw_start = qfpraw->start - qfpbase->start;
> +       priv->qfpraw_end = qfpraw->end - qfpbase->start;

See above comments, but the math to calculate "qfpraw_start" and
"qfpraw_end" really seems to violate abstractions and implies that you
should just define one memory range, not 3.


> +
> +       qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +
> +       priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
> +       if (IS_ERR(priv->qfpmap)) {
> +               ret = PTR_ERR(priv->qfpmap);
> +               goto err;
> +       }
> +
> +       priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> +       if (IS_ERR(priv->vcc)) {
> +               ret = PTR_ERR(priv->vcc);
> +               if (ret == -ENODEV)
> +                       ret = -EPROBE_DEFER;
> +
> +               goto err;
> +       }
> +
> +       priv->secclk = devm_clk_get(dev, "secclk");
> +       if (IS_ERR(priv->secclk)) {
> +               ret = PTR_ERR(priv->secclk);
> +               if (ret != -EPROBE_DEFER)
> +                       dev_err(dev, "secclk error getting : %d\n", ret);
> +               goto err;
> +       }
> +
> +       ret = clk_prepare_enable(priv->secclk);

You really need your clock prepared and enabled _all the time_?  Maybe
just prepare it here and then do the enable/disable when you need it?


> +       if (ret) {
> +               dev_err(dev, "clk_prepare_enable() failed\n");
> +               goto err;
> +       }
> +
> +       econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
> +       if (!econfig)
> +               return -ENOMEM;
> +
> +       econfig->dev = dev;
> +       econfig->name = "qfprom-efuse";
> +       econfig->stride = 1;
> +       econfig->word_size = 1;

See above about suggestions, specifically about changing ".stride" and
".word_size".


> +       econfig->reg_read = qfprom_efuse_reg_read;
> +       econfig->reg_write = qfprom_efuse_reg_write;
> +       econfig->size = resource_size(qfpraw);
> +       econfig->priv = priv;
> +
> +       nvmem = devm_nvmem_register(dev, econfig);
> +
> +       return PTR_ERR_OR_ZERO(nvmem);
> +
> +err:
> +       clk_disable_unprepare(priv->secclk);
> +       return ret;
> +}
> +
> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
> +       .name = "sc7180-qfprom-efuse",
> +       .fuse_blow_time_in_us = 10,
> +       .accel_value = 0xD10,
> +       .accel_reset_value = 0x800,
> +       .qfprom_blow_timer_value = 25,
> +       .qfprom_blow_reset_freq = 19200000,
> +       .qfprom_blow_set_freq = 4800000,
> +       .qfprom_max_vol = 1904000,
> +       .qfprom_min_vol = 1800000,

Can you explain why this patch needs to specify voltage at all?  I'd
expect that the voltage constraints would be specified in the device
tree and the driver shouldn't worry about it.  The only case you
really would need to specify this would be if you're expected to be on
a rail that's shared with other components and they have a different
(but compatible) operating range to yours.  Do you actually have this?


> +};
> +
> +static const struct of_device_id qfprom_efuse_of_match[] = {
> +       {
> +               .compatible = "qcom,sc7180-qfprom-efuse",
> +               .data = &sc7180_qfp_efuse_data
> +       },
> +       {/* sentinel */},
> +};
> +
> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
> +
> +static struct platform_driver qfprom_efuse_driver = {
> +       .probe = qfprom_efuse_probe,

You need a remove to unprepare / disable your clock, right?  Unless
you change that to not leave it prepared/enabled all the time.


> +       .driver = {
> +               .name = "sc7180-qfprom-efuse",

You don't need to put "sc7180" in the driver name.  This driver will
be used for other hardware eventually too with just a different
compatible match.


> +               .of_match_table = qfprom_efuse_of_match,
> +       },
> +};
> +
> +module_platform_driver(qfprom_efuse_driver);
> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
> +MODULE_LICENSE("GPL v2");
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
>

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
> This patch adds new driver for QTI qfprom-efuse controller. This driver can
> access the raw qfprom regions for fuse blowing.

QTI?

> 
> The current existed qfprom driver is only supports for cpufreq, thermal sensors
> drivers by read out calibration data, speed bins..etc which is stored
> by qfprom efuses.

Can you explain bit more about this QFPROM instance, Is this QFPROM part 
of secure controller address space?
Is this closely tied to SoC or Secure controller version?

Any reason why this can not be integrated into qfprom driver with 
specific compatible.

> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> ---
>   drivers/nvmem/Kconfig        |  10 +
>   drivers/nvmem/Makefile       |   2 +
>   drivers/nvmem/qfprom-efuse.c | 476 +++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 488 insertions(+)
>   create mode 100644 drivers/nvmem/qfprom-efuse.c
> 
...

> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
> new file mode 100644
> index 0000000..2e3c275
> --- /dev/null
> +++ b/drivers/nvmem/qfprom-efuse.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#define QFPROM_BLOW_STATUS_BUSY 0x1
> +#define QFPROM_BLOW_STATUS_READY 0x0
> +
> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
> +
> +/* Amount of time required to hold charge to blow fuse in micro-seconds */
> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
> +
> +#define QFPROM_ACCEL_OFFSET 0x044
> +
> +/**
> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
> + * platform data
> + *
> + * @name: qfprom-efuse compatible name

??
> + * @fuse_blow_time_in_us: Should contain the wait time when doing the fuse blow
> + * @accel_value: Should contain qfprom accel value
> + * @accel_reset_value: The reset value of qfprom accel value
> + * @qfprom_blow_timer_value: The timer value of qfprom when doing efuse blow
> + * @qfprom_blow_reset_freq: The frequency required to set when fuse blowing
> + * is done
> + * @qfprom_blow_set_freq: The frequency required to set when we start the
> + * fuse blowing
> + * @qfprom_max_vol: max voltage required to set fuse blow
> + * @qfprom_min_vol: min voltage required to set fuse blow

How specific are these values per SoC?


> + */
> +struct qfprom_efuse_platform_data {
> +	const char *name;
> +	u8 fuse_blow_time_in_us;
> +	u32 accel_value;
> +	u32 accel_reset_value;
> +	u32 qfprom_blow_timer_value;
> +	u32 qfprom_blow_reset_freq;
> +	u32 qfprom_blow_set_freq;
> +	u32 qfprom_max_vol;
> +	u32 qfprom_min_vol;
> +};
> +
> +/**
> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
> + *
> + * @qfpbase: iomapped memory space for qfprom base
> + * @qfpraw: iomapped memory space for qfprom raw fuse region
> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
> +
> + * @dev: qfprom device structure
> + * @secclk: clock supply
> + * @vcc: regulator supply
> +
> + * @qfpraw_start: qfprom raw fuse start region
> + * @qfpraw_end: qfprom raw fuse end region
> + * @qfprom_efuse_platform_data: qfprom platform data
> + */
> +struct qfprom_efuse_priv {
> +	void __iomem *qfpbase;
> +	void __iomem *qfpraw;
> +	void __iomem *qfpmap;

Why are these memory regions split? Can't you just have complete qfprom 
area and add fixed offset for qfpraw within the driver?

> +	struct device *dev;
> +	struct clk *secclk;
> +	struct regulator *vcc;
> +	resource_size_t qfpraw_start;
> +	resource_size_t qfpraw_end;
Why do we need to check this range? as long as we set the nvmem_config 
with correct range then you should not need this check.


> +	struct qfprom_efuse_platform_data efuse;
A pointer here should be good enough?
> +};
> +

...

> +/*
> + * sets the value of the blow timer, accel register and the clock
> + * and voltage settings
> + */
> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv *priv)
> +{
> +	int ret;
> +
> +	ret = qfprom_disable_fuse_blowing(priv);
> +	if (ret) {
> +		dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
> +		return ret;
> +	}

Why do we need to qfprom_disable_fuse_blowing() for every call to enable it?

Or are we missing some error handling in the caller?

> +
> +	writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
> +	       QFPROM_BLOW_TIMER_OFFSET);
> +	writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
> +
> +	ret = qfprom_set_clock_settings(priv);
> +	if (ret) {
> +		dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
> +		return ret;
> +	}
> +
> +	ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
> +					  priv->efuse.qfprom_max_vol);
> +	if (ret) {
> +		dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +

<<
> +/*
> + * verifying to make sure address being written or read is from qfprom
> + * raw address range
> + */
> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
> +			  size_t bytes)
> +{
> +	if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
> +	    ((reg + bytes) <= priv->qfpraw_end)) {
> +		return 1;
> +	}
> +
> +	return 0;
> +}
 >>
Above function is totally redundant, nvmem core already has checks for this.



> +
> +/*
> + * API for reading from raw qfprom region
> + */
> +static int qfprom_efuse_reg_read(void *context, unsigned int reg, void *_val,
> +				 size_t bytes)
> +{
> +	struct qfprom_efuse_priv *priv = context;
> +	u32 *value = _val;
> +	u32 align_check;
> +	int i = 0, words = bytes / 4;
> +
> +	dev_info(priv->dev,
> +		 "reading raw qfprom region offset: 0x%08x of size: %zd\n",
> +		 reg, bytes);

In general there is lot of debug info across the code, do you really 
need all this? Consider removing these!

> +
> +	if (bytes % 4 != 0x00) {
> +		dev_err(priv->dev,
> +			"Bytes: %zd to read should be word align\n",
> +			bytes);
> +		return -EINVAL;
> +	}

This word align check is also redundant once you set nvmem_config with 
correct word_size.


> +
> +	if (!addr_in_qfprom_range(priv, reg, bytes)) {
> +		dev_err(priv->dev,
> +			"Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
> +			reg, bytes);
> +		return -EINVAL;
> +	}
> +
> +	align_check = (reg & 0xF);
> +
> +	if (((align_check & ~3) == align_check) && value != NULL)
> +		while (words--)
> +			*value++ = readl(priv->qfpbase + reg + (i++ * 4));
> +
> +	else
> +		dev_err(priv->dev,
> +			"Invalid input parameter 0x%08x fuse blow address\n",
> +			reg);
> +
> +	return 0;
> +}
...

> +
> +static int qfprom_efuse_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct resource *qfpbase, *qfpraw, *qfpmap;
> +	struct nvmem_device *nvmem;
> +	struct nvmem_config *econfig;
> +	struct qfprom_efuse_priv *priv;
> +	const struct qfprom_efuse_platform_data *drvdata;
> +	int ret;
> +
> +	dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
> +

too much debug!

> +	drvdata = of_device_get_match_data(&pdev->dev);
> +	if (!drvdata)
> +		return -EINVAL;
Unnecessary check as this driver will not be probed unless there is a 
compatible match.


> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
> +	priv->efuse.accel_value = drvdata->accel_value;
> +	priv->efuse.accel_reset_value = drvdata->accel_reset_value;
> +	priv->efuse.qfprom_blow_timer_value = drvdata->qfprom_blow_timer_value;
> +	priv->efuse.qfprom_blow_reset_freq = drvdata->qfprom_blow_reset_freq;
> +	priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
> +	priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
> +	priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
> +	priv->dev = dev;
> +
> +	qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +
> +	priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
> +	if (IS_ERR(priv->qfpbase)) {
> +		ret = PTR_ERR(priv->qfpbase);
> +		goto err;
> +	}
> +
> +	qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +
> +	priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
> +	if (IS_ERR(priv->qfpraw)) {
> +		ret = PTR_ERR(priv->qfpraw);
> +		goto err;
> +	}
> +
> +	priv->qfpraw_start = qfpraw->start - qfpbase->start;
> +	priv->qfpraw_end = qfpraw->end - qfpbase->start;
> +
> +	qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +
> +	priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
> +	if (IS_ERR(priv->qfpmap)) {
> +		ret = PTR_ERR(priv->qfpmap);
> +		goto err;
> +	}
> +
> +	priv->vcc = devm_regulator_get(&pdev->dev, "vcc");

I see no reference to this regulator in dt bindings.
> +	if (IS_ERR(priv->vcc)) {
> +		ret = PTR_ERR(priv->vcc);
> +		if (ret == -ENODEV)
> +			ret = -EPROBE_DEFER;
Can you explain what is going on here?

> +
> +		goto err;
> +	}
> +
> +	priv->secclk = devm_clk_get(dev, "secclk");
> +	if (IS_ERR(priv->secclk)) {
> +		ret = PTR_ERR(priv->secclk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "secclk error getting : %d\n", ret);
> +		goto err;
> +	}
> +
> +	ret = clk_prepare_enable(priv->secclk);
> +	if (ret) {
> +		dev_err(dev, "clk_prepare_enable() failed\n");
> +		goto err;
> +	}
> +
> +	econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
> +	if (!econfig)
Why not disabling the clk here?
> +		return -ENOMEM;

> +
> +	econfig->dev = dev;
> +	econfig->name = "qfprom-efuse";
> +	econfig->stride = 1;
> +	econfig->word_size = 1;
> +	econfig->reg_read = qfprom_efuse_reg_read;
> +	econfig->reg_write = qfprom_efuse_reg_write;
> +	econfig->size = resource_size(qfpraw);
> +	econfig->priv = priv;
> +
> +	nvmem = devm_nvmem_register(dev, econfig);
> +
> +	return PTR_ERR_OR_ZERO(nvmem);
probably you should check the nvmem here before returning to disable the 
clk properly.

> +
> +err:
> +	clk_disable_unprepare(priv->secclk);
> +	return ret;
> +}
> +
> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
> +	.name = "sc7180-qfprom-efuse",
Redundant.

> +	.fuse_blow_time_in_us = 10,
> +	.accel_value = 0xD10,
> +	.accel_reset_value = 0x800,
> +	.qfprom_blow_timer_value = 25,
> +	.qfprom_blow_reset_freq = 19200000,
> +	.qfprom_blow_set_freq = 4800000,
> +	.qfprom_max_vol = 1904000,
> +	.qfprom_min_vol = 1800000,
> +};
> +
> +static const struct of_device_id qfprom_efuse_of_match[] = {
> +	{
> +		.compatible = "qcom,sc7180-qfprom-efuse",
> +		.data = &sc7180_qfp_efuse_data
> +	},
> +	{/* sentinel */},
> +};
> +
> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
> +
> +static struct platform_driver qfprom_efuse_driver = {
> +	.probe = qfprom_efuse_probe,
> +	.driver = {
> +		.name = "sc7180-qfprom-efuse",
> +		.of_match_table = qfprom_efuse_of_match,
> +	},
> +};
> +
> +module_platform_driver(qfprom_efuse_driver);
> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
> +MODULE_LICENSE("GPL v2");
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Ravi Kumar Bokka (Temp)]

Hi Srinivas,
Thanks for your feedback by giving review comments. Please find my 
inline comments.


Regards,
Ravi Kumar.B

On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
> 
> 
> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>> This patch adds new driver for QTI qfprom-efuse controller. This 
>> driver can
>> access the raw qfprom regions for fuse blowing.
> 
> QTI?

guidance I have received from internal Legal/LOST team is that the QCOM 
prefix needs to be changed to QTI everywhere it is used

> 
>>
>> The current existed qfprom driver is only supports for cpufreq, 
>> thermal sensors
>> drivers by read out calibration data, speed bins..etc which is stored
>> by qfprom efuses.
> 
> Can you explain bit more about this QFPROM instance, Is this QFPROM part 
> of secure controller address space?
> Is this closely tied to SoC or Secure controller version?
> 
> Any reason why this can not be integrated into qfprom driver with 
> specific compatible.
> 

QFPROM driver communicates with sec_controller address space however 
scope and functionalities of this driver is different and not limited as 
existing qfprom fuse Read-Only driver for specific “fuse buckets’ like 
cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch 
requires specific sequence to write/blow fuses unlike other driver. 
Scope/functionalities are different and this is separate driver.

>>
>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>> ---
>>   drivers/nvmem/Kconfig        |  10 +
>>   drivers/nvmem/Makefile       |   2 +
>>   drivers/nvmem/qfprom-efuse.c | 476 
>> +++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 488 insertions(+)
>>   create mode 100644 drivers/nvmem/qfprom-efuse.c
>>
> ...
> 
>> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
>> new file mode 100644
>> index 0000000..2e3c275
>> --- /dev/null
>> +++ b/drivers/nvmem/qfprom-efuse.c
>> @@ -0,0 +1,476 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/of_device.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>> +#define QFPROM_BLOW_STATUS_READY 0x0
>> +
>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>> +
>> +/* Amount of time required to hold charge to blow fuse in 
>> micro-seconds */
>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>> +
>> +#define QFPROM_ACCEL_OFFSET 0x044
>> +
>> +/**
>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>> + * platform data
>> + *
>> + * @name: qfprom-efuse compatible name
> 
> ??

Thanks for your feedback. I will address this change

>> + * @fuse_blow_time_in_us: Should contain the wait time when doing the 
>> fuse blow
>> + * @accel_value: Should contain qfprom accel value
>> + * @accel_reset_value: The reset value of qfprom accel value
>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing 
>> efuse blow
>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse 
>> blowing
>> + * is done
>> + * @qfprom_blow_set_freq: The frequency required to set when we start 
>> the
>> + * fuse blowing
>> + * @qfprom_max_vol: max voltage required to set fuse blow
>> + * @qfprom_min_vol: min voltage required to set fuse blow
> 
> How specific are these values per SoC?
> 

This voltage level may change based on SoC and/or fuse-hardware 
technology, it would change for SoC with different technology, hence we 
have kept it in SOC specific settings.

> 
>> + */
>> +struct qfprom_efuse_platform_data {
>> +    const char *name;
>> +    u8 fuse_blow_time_in_us;
>> +    u32 accel_value;
>> +    u32 accel_reset_value;
>> +    u32 qfprom_blow_timer_value;
>> +    u32 qfprom_blow_reset_freq;
>> +    u32 qfprom_blow_set_freq;
>> +    u32 qfprom_max_vol;
>> +    u32 qfprom_min_vol;
>> +};
>> +
>> +/**
>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
>> + *
>> + * @qfpbase: iomapped memory space for qfprom base
>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>> +
>> + * @dev: qfprom device structure
>> + * @secclk: clock supply
>> + * @vcc: regulator supply
>> +
>> + * @qfpraw_start: qfprom raw fuse start region
>> + * @qfpraw_end: qfprom raw fuse end region
>> + * @qfprom_efuse_platform_data: qfprom platform data
>> + */
>> +struct qfprom_efuse_priv {
>> +    void __iomem *qfpbase;
>> +    void __iomem *qfpraw;
>> +    void __iomem *qfpmap;
> 
> Why are these memory regions split? Can't you just have complete qfprom 
> area and add fixed offset for qfpraw within the driver?
> 

Thanks for your feedback. I will address this change.
I have separated this memory regions because to identify raw fuse 
regions separately and compare these raw fuse regions from the user 
given input.

>> +    struct device *dev;
>> +    struct clk *secclk;
>> +    struct regulator *vcc;
>> +    resource_size_t qfpraw_start;
>> +    resource_size_t qfpraw_end;
> Why do we need to check this range? as long as we set the nvmem_config 
> with correct range then you should not need this check.
> 

There is no harm in this explicit check in QFPROM-fuse driver and based 
on internal review with our security team, this check is important to 
avoid dependency on other upper layer.


> 
>> +    struct qfprom_efuse_platform_data efuse;
> A pointer here should be good enough?
>> +};
>> +
> 

Thanks for your feedback. I will address this change

> ...
> 
>> +/*
>> + * sets the value of the blow timer, accel register and the clock
>> + * and voltage settings
>> + */
>> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv 
>> *priv)
>> +{
>> +    int ret;
>> +
>> +    ret = qfprom_disable_fuse_blowing(priv);
>> +    if (ret) {
>> +        dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>> +        return ret;
>> +    }
> 
> Why do we need to qfprom_disable_fuse_blowing() for every call to enable 
> it?
> 
> Or are we missing some error handling in the caller?
> 

We must disable/vote-off this QFPROM fuse power rail after blowing fuse, 
it is the safe and right approach as per hardware programming guide for 
fuse blowing process. Caller here is user space, can’t control 
fuse-power-rail or can’t be relied to follow the required process. There 
could also be unnecessary risk of leaving the vote/power-rail configured 
at specific level after blowing the fuse. As per hardware requirement, 
right after fuse blowing, we need to disable power rail.

>> +
>> +    writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>> +           QFPROM_BLOW_TIMER_OFFSET);
>> +    writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
>> +
>> +    ret = qfprom_set_clock_settings(priv);
>> +    if (ret) {
>> +        dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>> +        return ret;
>> +    }
>> +
>> +    ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
>> +                      priv->efuse.qfprom_max_vol);
>> +    if (ret) {
>> +        dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
> 
> <<
>> +/*
>> + * verifying to make sure address being written or read is from qfprom
>> + * raw address range
>> + */
>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
>> +              size_t bytes)
>> +{
>> +    if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>> +        ((reg + bytes) <= priv->qfpraw_end)) {
>> +        return 1;
>> +    }
>> +
>> +    return 0;
>> +}
>  >>
> Above function is totally redundant, nvmem core already has checks for 
> this.
> 

There is no harm in this explicit check in QFPROM-fuse driver and based 
on internal review with our security team, this check is important to 
avoid dependency on other upper layer.

> 
> 
>> +
>> +/*
>> + * API for reading from raw qfprom region
>> + */
>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg, 
>> void *_val,
>> +                 size_t bytes)
>> +{
>> +    struct qfprom_efuse_priv *priv = context;
>> +    u32 *value = _val;
>> +    u32 align_check;
>> +    int i = 0, words = bytes / 4;
>> +
>> +    dev_info(priv->dev,
>> +         "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>> +         reg, bytes);
> 
> In general there is lot of debug info across the code, do you really 
> need all this? Consider removing these!
> 

Thanks for your feedback. I will address this change.

>> +
>> +    if (bytes % 4 != 0x00) {
>> +        dev_err(priv->dev,
>> +            "Bytes: %zd to read should be word align\n",
>> +            bytes);
>> +        return -EINVAL;
>> +    }
> 
> This word align check is also redundant once you set nvmem_config with 
> correct word_size.
> 

I understand that there may be different approach to handle this. We 
have used this approach and tested this driver thoroughly. Unless there 
is technical limitation, changing this word_size would end up requiring 
re-writing write/read APIs and going through testing again, there is not 
much difference in either approach, we would like to keep this approach 
unless there is technical concern.

> 
>> +
>> +    if (!addr_in_qfprom_range(priv, reg, bytes)) {
>> +        dev_err(priv->dev,
>> +            "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>> +            reg, bytes);
>> +        return -EINVAL;
>> +    }
>> +
>> +    align_check = (reg & 0xF);
>> +
>> +    if (((align_check & ~3) == align_check) && value != NULL)
>> +        while (words--)
>> +            *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>> +
>> +    else
>> +        dev_err(priv->dev,
>> +            "Invalid input parameter 0x%08x fuse blow address\n",
>> +            reg);
>> +
>> +    return 0;
>> +}
> ...
> 
>> +
>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct resource *qfpbase, *qfpraw, *qfpmap;
>> +    struct nvmem_device *nvmem;
>> +    struct nvmem_config *econfig;
>> +    struct qfprom_efuse_priv *priv;
>> +    const struct qfprom_efuse_platform_data *drvdata;
>> +    int ret;
>> +
>> +    dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>> +
> 
> too much debug!
> 

Thanks for your feedback. I will address this change.

>> +    drvdata = of_device_get_match_data(&pdev->dev);
>> +    if (!drvdata)
>> +        return -EINVAL;
> Unnecessary check as this driver will not be probed unless there is a 
> compatible match.
> 

Thanks for your feedback. I will address this change.

> 
>> +
>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>> +    priv->efuse.accel_value = drvdata->accel_value;
>> +    priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>> +    priv->efuse.qfprom_blow_timer_value = 
>> drvdata->qfprom_blow_timer_value;
>> +    priv->efuse.qfprom_blow_reset_freq = 
>> drvdata->qfprom_blow_reset_freq;
>> +    priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>> +    priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>> +    priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>> +    priv->dev = dev;
>> +
>> +    qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +
>> +    priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>> +    if (IS_ERR(priv->qfpbase)) {
>> +        ret = PTR_ERR(priv->qfpbase);
>> +        goto err;
>> +    }
>> +
>> +    qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>> +
>> +    priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>> +    if (IS_ERR(priv->qfpraw)) {
>> +        ret = PTR_ERR(priv->qfpraw);
>> +        goto err;
>> +    }
>> +
>> +    priv->qfpraw_start = qfpraw->start - qfpbase->start;
>> +    priv->qfpraw_end = qfpraw->end - qfpbase->start;
>> +
>> +    qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>> +
>> +    priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>> +    if (IS_ERR(priv->qfpmap)) {
>> +        ret = PTR_ERR(priv->qfpmap);
>> +        goto err;
>> +    }
>> +
>> +    priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> 
> I see no reference to this regulator in dt bindings.

This perameter kept in board specific file i.e., sc7180-idp.dts file

>> +    if (IS_ERR(priv->vcc)) {
>> +        ret = PTR_ERR(priv->vcc);
>> +        if (ret == -ENODEV)
>> +            ret = -EPROBE_DEFER;
> Can you explain what is going on here?
> 

As i took other drivers reference, i have kept this check.

>> +
>> +        goto err;
>> +    }
>> +
>> +    priv->secclk = devm_clk_get(dev, "secclk");
>> +    if (IS_ERR(priv->secclk)) {
>> +        ret = PTR_ERR(priv->secclk);
>> +        if (ret != -EPROBE_DEFER)
>> +            dev_err(dev, "secclk error getting : %d\n", ret);
>> +        goto err;
>> +    }
>> +
>> +    ret = clk_prepare_enable(priv->secclk);
>> +    if (ret) {
>> +        dev_err(dev, "clk_prepare_enable() failed\n");
>> +        goto err;
>> +    }
>> +
>> +    econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>> +    if (!econfig)
> Why not disabling the clk here?
>> +        return -ENOMEM;
> 

Thanks for your feedback. I will address this change.

>> +
>> +    econfig->dev = dev;
>> +    econfig->name = "qfprom-efuse";
>> +    econfig->stride = 1;
>> +    econfig->word_size = 1;
>> +    econfig->reg_read = qfprom_efuse_reg_read;
>> +    econfig->reg_write = qfprom_efuse_reg_write;
>> +    econfig->size = resource_size(qfpraw);
>> +    econfig->priv = priv;
>> +
>> +    nvmem = devm_nvmem_register(dev, econfig);
>> +
>> +    return PTR_ERR_OR_ZERO(nvmem);
> probably you should check the nvmem here before returning to disable the 
> clk properly.
> 

Thanks for your feedback. I will address this change.

>> +
>> +err:
>> +    clk_disable_unprepare(priv->secclk);
>> +    return ret;
>> +}
>> +
>> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
>> +    .name = "sc7180-qfprom-efuse",
> Redundant.
> 

Thanks for your feedback. I will address this change.

>> +    .fuse_blow_time_in_us = 10,
>> +    .accel_value = 0xD10,
>> +    .accel_reset_value = 0x800,
>> +    .qfprom_blow_timer_value = 25,
>> +    .qfprom_blow_reset_freq = 19200000,
>> +    .qfprom_blow_set_freq = 4800000,
>> +    .qfprom_max_vol = 1904000,
>> +    .qfprom_min_vol = 1800000,
>> +};
>> +
>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>> +    {
>> +        .compatible = "qcom,sc7180-qfprom-efuse",
>> +        .data = &sc7180_qfp_efuse_data
>> +    },
>> +    {/* sentinel */},
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>> +
>> +static struct platform_driver qfprom_efuse_driver = {
>> +    .probe = qfprom_efuse_probe,
>> +    .driver = {
>> +        .name = "sc7180-qfprom-efuse",
>> +        .of_match_table = qfprom_efuse_of_match,
>> +    },
>> +};
>> +
>> +module_platform_driver(qfprom_efuse_driver);
>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>> +MODULE_LICENSE("GPL v2");
>>

-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

I notice that you didn't respond to any of my feedback [1], only
Srinivas's.  Any reason why?  It turns out that Srinivas had quite a
lot of the same feedback as I did, but please make sure you didn't
miss anything I suggested.  More inline below.

On Thu, May 14, 2020 at 5:26 AM Ravi Kumar Bokka (Temp)
<rbokka@codeaurora.org> wrote:
>
> Hi Srinivas,
> Thanks for your feedback by giving review comments. Please find my
> inline comments.
>
>
> Regards,
> Ravi Kumar.B
>
> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
> >> This patch adds new driver for QTI qfprom-efuse controller. This
> >> driver can
> >> access the raw qfprom regions for fuse blowing.
> >
> > QTI?
>
> guidance I have received from internal Legal/LOST team is that the QCOM
> prefix needs to be changed to QTI everywhere it is used

I'll let Srinivas comment if he cares.  I'm really not sure why a
legal team cares about the Kconfig name in a GPL-licensed Linux
kernel.


> >> The current existed qfprom driver is only supports for cpufreq,
> >> thermal sensors
> >> drivers by read out calibration data, speed bins..etc which is stored
> >> by qfprom efuses.
> >
> > Can you explain bit more about this QFPROM instance, Is this QFPROM part
> > of secure controller address space?
> > Is this closely tied to SoC or Secure controller version?
> >
> > Any reason why this can not be integrated into qfprom driver with
> > specific compatible.
> >
>
> QFPROM driver communicates with sec_controller address space however
> scope and functionalities of this driver is different and not limited as
> existing qfprom fuse Read-Only driver for specific “fuse buckets’ like
> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch
> requires specific sequence to write/blow fuses unlike other driver.
> Scope/functionalities are different and this is separate driver.

If the underlying IP blocks are the same it should be one driver and
it should just work in read-only mode for the other range of stuff.


> >> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >> ---
> >>   drivers/nvmem/Kconfig        |  10 +
> >>   drivers/nvmem/Makefile       |   2 +
> >>   drivers/nvmem/qfprom-efuse.c | 476
> >> +++++++++++++++++++++++++++++++++++++++++++
> >>   3 files changed, 488 insertions(+)
> >>   create mode 100644 drivers/nvmem/qfprom-efuse.c
> >>
> > ...
> >
> >> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
> >> new file mode 100644
> >> index 0000000..2e3c275
> >> --- /dev/null
> >> +++ b/drivers/nvmem/qfprom-efuse.c
> >> @@ -0,0 +1,476 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
> >> + */
> >> +
> >> +#include <linux/clk.h>
> >> +#include <linux/device.h>
> >> +#include <linux/io.h>
> >> +#include <linux/iopoll.h>
> >> +#include <linux/kernel.h>
> >> +#include <linux/module.h>
> >> +#include <linux/mod_devicetable.h>
> >> +#include <linux/nvmem-provider.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/platform_device.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#define QFPROM_BLOW_STATUS_BUSY 0x1
> >> +#define QFPROM_BLOW_STATUS_READY 0x0
> >> +
> >> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
> >> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> >> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
> >> +
> >> +/* Amount of time required to hold charge to blow fuse in
> >> micro-seconds */
> >> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
> >> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
> >> +
> >> +#define QFPROM_ACCEL_OFFSET 0x044
> >> +
> >> +/**
> >> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
> >> + * platform data
> >> + *
> >> + * @name: qfprom-efuse compatible name
> >
> > ??
>
> Thanks for your feedback. I will address this change
>
> >> + * @fuse_blow_time_in_us: Should contain the wait time when doing the
> >> fuse blow
> >> + * @accel_value: Should contain qfprom accel value
> >> + * @accel_reset_value: The reset value of qfprom accel value
> >> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
> >> efuse blow
> >> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
> >> blowing
> >> + * is done
> >> + * @qfprom_blow_set_freq: The frequency required to set when we start
> >> the
> >> + * fuse blowing
> >> + * @qfprom_max_vol: max voltage required to set fuse blow
> >> + * @qfprom_min_vol: min voltage required to set fuse blow
> >
> > How specific are these values per SoC?
> >
>
> This voltage level may change based on SoC and/or fuse-hardware
> technology, it would change for SoC with different technology, hence we
> have kept it in SOC specific settings.

Generally I'd expect the SoC specific settings to be in the device
tree.  Drivers don't need to specify this.  Please respond to the
comments I posed in my review.


> >> + */
> >> +struct qfprom_efuse_platform_data {
> >> +    const char *name;
> >> +    u8 fuse_blow_time_in_us;
> >> +    u32 accel_value;
> >> +    u32 accel_reset_value;
> >> +    u32 qfprom_blow_timer_value;
> >> +    u32 qfprom_blow_reset_freq;
> >> +    u32 qfprom_blow_set_freq;
> >> +    u32 qfprom_max_vol;
> >> +    u32 qfprom_min_vol;
> >> +};
> >> +
> >> +/**
> >> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
> >> + *
> >> + * @qfpbase: iomapped memory space for qfprom base
> >> + * @qfpraw: iomapped memory space for qfprom raw fuse region
> >> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
> >> +
> >> + * @dev: qfprom device structure
> >> + * @secclk: clock supply
> >> + * @vcc: regulator supply
> >> +
> >> + * @qfpraw_start: qfprom raw fuse start region
> >> + * @qfpraw_end: qfprom raw fuse end region
> >> + * @qfprom_efuse_platform_data: qfprom platform data
> >> + */
> >> +struct qfprom_efuse_priv {
> >> +    void __iomem *qfpbase;
> >> +    void __iomem *qfpraw;
> >> +    void __iomem *qfpmap;
> >
> > Why are these memory regions split? Can't you just have complete qfprom
> > area and add fixed offset for qfpraw within the driver?
> >
>
> Thanks for your feedback. I will address this change.
> I have separated this memory regions because to identify raw fuse
> regions separately and compare these raw fuse regions from the user
> given input.

How are you addressing?  Can you go back to just having one range?  If
you need to know where the raw fuse region is inside your range just
put the offset in the of_match data.


> >> +    struct device *dev;
> >> +    struct clk *secclk;
> >> +    struct regulator *vcc;
> >> +    resource_size_t qfpraw_start;
> >> +    resource_size_t qfpraw_end;
> > Why do we need to check this range? as long as we set the nvmem_config
> > with correct range then you should not need this check.
> >
>
> There is no harm in this explicit check in QFPROM-fuse driver and based
> on internal review with our security team, this check is important to
> avoid dependency on other upper layer.

There is harm: it adds extra complexity.  Please remove.

You are talking as if it was somehow important for this code to be the
same as the code on other OSes / in other contexts.  It isn't.  This
is a Linux driver and it should not be written to duplicate stuff that
Linux is already doing.


> >> +    struct qfprom_efuse_platform_data efuse;
> > A pointer here should be good enough?
> >> +};
> >> +
> >
>
> Thanks for your feedback. I will address this change
>
> > ...
> >
> >> +/*
> >> + * sets the value of the blow timer, accel register and the clock
> >> + * and voltage settings
> >> + */
> >> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv
> >> *priv)
> >> +{
> >> +    int ret;
> >> +
> >> +    ret = qfprom_disable_fuse_blowing(priv);
> >> +    if (ret) {
> >> +        dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
> >> +        return ret;
> >> +    }
> >
> > Why do we need to qfprom_disable_fuse_blowing() for every call to enable
> > it?
> >
> > Or are we missing some error handling in the caller?
> >
>
> We must disable/vote-off this QFPROM fuse power rail after blowing fuse,
> it is the safe and right approach as per hardware programming guide for
> fuse blowing process. Caller here is user space, can’t control
> fuse-power-rail or can’t be relied to follow the required process. There
> could also be unnecessary risk of leaving the vote/power-rail configured
> at specific level after blowing the fuse. As per hardware requirement,
> right after fuse blowing, we need to disable power rail.

Please remove your disable here.  Though the user initiates the call
you can still rely on Linux to make sure two users aren't trying to
blow fuses at the same time.  The Linux driver should always leave
things in a "disabled" state and you can rely on that.

...besides the only way you aren't hitting an underflow on the
regulator enable count is that you are constantly enabling over and
over again.


> >> +
> >> +    writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
> >> +           QFPROM_BLOW_TIMER_OFFSET);
> >> +    writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
> >> +
> >> +    ret = qfprom_set_clock_settings(priv);
> >> +    if (ret) {
> >> +        dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
> >> +        return ret;
> >> +    }
> >> +
> >> +    ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
> >> +                      priv->efuse.qfprom_max_vol);
> >> +    if (ret) {
> >> +        dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
> >> +        return ret;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >> +
> >
> > <<
> >> +/*
> >> + * verifying to make sure address being written or read is from qfprom
> >> + * raw address range
> >> + */
> >> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
> >> +              size_t bytes)
> >> +{
> >> +    if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
> >> +        ((reg + bytes) <= priv->qfpraw_end)) {
> >> +        return 1;
> >> +    }
> >> +
> >> +    return 0;
> >> +}
> >  >>
> > Above function is totally redundant, nvmem core already has checks for
> > this.
> >
>
> There is no harm in this explicit check in QFPROM-fuse driver and based
> on internal review with our security team, this check is important to
> avoid dependency on other upper layer.

Please remove.  You are a Linux driver.


> >> +
> >> +/*
> >> + * API for reading from raw qfprom region
> >> + */
> >> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
> >> void *_val,
> >> +                 size_t bytes)
> >> +{
> >> +    struct qfprom_efuse_priv *priv = context;
> >> +    u32 *value = _val;
> >> +    u32 align_check;
> >> +    int i = 0, words = bytes / 4;
> >> +
> >> +    dev_info(priv->dev,
> >> +         "reading raw qfprom region offset: 0x%08x of size: %zd\n",
> >> +         reg, bytes);
> >
> > In general there is lot of debug info across the code, do you really
> > need all this? Consider removing these!
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +
> >> +    if (bytes % 4 != 0x00) {
> >> +        dev_err(priv->dev,
> >> +            "Bytes: %zd to read should be word align\n",
> >> +            bytes);
> >> +        return -EINVAL;
> >> +    }
> >
> > This word align check is also redundant once you set nvmem_config with
> > correct word_size.
> >
>
> I understand that there may be different approach to handle this. We
> have used this approach and tested this driver thoroughly. Unless there
> is technical limitation, changing this word_size would end up requiring
> re-writing write/read APIs and going through testing again, there is not
> much difference in either approach, we would like to keep this approach
> unless there is technical concern.

The driver isn't done until it lands in Linux.  While it's important
to test the driver before posting upstream it is completely expected
and normal that then posting upstream you will be asked to change
things.  After you change things you will need to re-test.  The fact
that you already tested this the old way is not an excuse.  Please
fix.


> >> +
> >> +    if (!addr_in_qfprom_range(priv, reg, bytes)) {
> >> +        dev_err(priv->dev,
> >> +            "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
> >> +            reg, bytes);
> >> +        return -EINVAL;
> >> +    }
> >> +
> >> +    align_check = (reg & 0xF);
> >> +
> >> +    if (((align_check & ~3) == align_check) && value != NULL)
> >> +        while (words--)
> >> +            *value++ = readl(priv->qfpbase + reg + (i++ * 4));
> >> +
> >> +    else
> >> +        dev_err(priv->dev,
> >> +            "Invalid input parameter 0x%08x fuse blow address\n",
> >> +            reg);
> >> +
> >> +    return 0;
> >> +}
> > ...
> >
> >> +
> >> +static int qfprom_efuse_probe(struct platform_device *pdev)
> >> +{
> >> +    struct device *dev = &pdev->dev;
> >> +    struct resource *qfpbase, *qfpraw, *qfpmap;
> >> +    struct nvmem_device *nvmem;
> >> +    struct nvmem_config *econfig;
> >> +    struct qfprom_efuse_priv *priv;
> >> +    const struct qfprom_efuse_platform_data *drvdata;
> >> +    int ret;
> >> +
> >> +    dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
> >> +
> >
> > too much debug!
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +    drvdata = of_device_get_match_data(&pdev->dev);
> >> +    if (!drvdata)
> >> +        return -EINVAL;
> > Unnecessary check as this driver will not be probed unless there is a
> > compatible match.
> >
>
> Thanks for your feedback. I will address this change.
>
> >
> >> +
> >> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >> +    if (!priv)
> >> +        return -ENOMEM;
> >> +
> >> +    priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
> >> +    priv->efuse.accel_value = drvdata->accel_value;
> >> +    priv->efuse.accel_reset_value = drvdata->accel_reset_value;
> >> +    priv->efuse.qfprom_blow_timer_value =
> >> drvdata->qfprom_blow_timer_value;
> >> +    priv->efuse.qfprom_blow_reset_freq =
> >> drvdata->qfprom_blow_reset_freq;
> >> +    priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
> >> +    priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
> >> +    priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
> >> +    priv->dev = dev;
> >> +
> >> +    qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +
> >> +    priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
> >> +    if (IS_ERR(priv->qfpbase)) {
> >> +        ret = PTR_ERR(priv->qfpbase);
> >> +        goto err;
> >> +    }
> >> +
> >> +    qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> +
> >> +    priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
> >> +    if (IS_ERR(priv->qfpraw)) {
> >> +        ret = PTR_ERR(priv->qfpraw);
> >> +        goto err;
> >> +    }
> >> +
> >> +    priv->qfpraw_start = qfpraw->start - qfpbase->start;
> >> +    priv->qfpraw_end = qfpraw->end - qfpbase->start;
> >> +
> >> +    qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> >> +
> >> +    priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
> >> +    if (IS_ERR(priv->qfpmap)) {
> >> +        ret = PTR_ERR(priv->qfpmap);
> >> +        goto err;
> >> +    }
> >> +
> >> +    priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> >
> > I see no reference to this regulator in dt bindings.
>
> This perameter kept in board specific file i.e., sc7180-idp.dts file

Yes, but it still needs to be in the bindings.


> >> +    if (IS_ERR(priv->vcc)) {
> >> +        ret = PTR_ERR(priv->vcc);
> >> +        if (ret == -ENODEV)
> >> +            ret = -EPROBE_DEFER;
> > Can you explain what is going on here?
> >
>
> As i took other drivers reference, i have kept this check.

Then other drivers are wrong.  Please remove.


> >> +
> >> +        goto err;
> >> +    }
> >> +
> >> +    priv->secclk = devm_clk_get(dev, "secclk");
> >> +    if (IS_ERR(priv->secclk)) {
> >> +        ret = PTR_ERR(priv->secclk);
> >> +        if (ret != -EPROBE_DEFER)
> >> +            dev_err(dev, "secclk error getting : %d\n", ret);
> >> +        goto err;
> >> +    }
> >> +
> >> +    ret = clk_prepare_enable(priv->secclk);
> >> +    if (ret) {
> >> +        dev_err(dev, "clk_prepare_enable() failed\n");
> >> +        goto err;
> >> +    }
> >> +
> >> +    econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
> >> +    if (!econfig)
> > Why not disabling the clk here?
> >> +        return -ENOMEM;
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +
> >> +    econfig->dev = dev;
> >> +    econfig->name = "qfprom-efuse";
> >> +    econfig->stride = 1;
> >> +    econfig->word_size = 1;
> >> +    econfig->reg_read = qfprom_efuse_reg_read;
> >> +    econfig->reg_write = qfprom_efuse_reg_write;
> >> +    econfig->size = resource_size(qfpraw);
> >> +    econfig->priv = priv;
> >> +
> >> +    nvmem = devm_nvmem_register(dev, econfig);
> >> +
> >> +    return PTR_ERR_OR_ZERO(nvmem);
> > probably you should check the nvmem here before returning to disable the
> > clk properly.
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +
> >> +err:
> >> +    clk_disable_unprepare(priv->secclk);
> >> +    return ret;
> >> +}
> >> +
> >> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
> >> +    .name = "sc7180-qfprom-efuse",
> > Redundant.
> >
>
> Thanks for your feedback. I will address this change.
>
> >> +    .fuse_blow_time_in_us = 10,
> >> +    .accel_value = 0xD10,
> >> +    .accel_reset_value = 0x800,
> >> +    .qfprom_blow_timer_value = 25,
> >> +    .qfprom_blow_reset_freq = 19200000,
> >> +    .qfprom_blow_set_freq = 4800000,
> >> +    .qfprom_max_vol = 1904000,
> >> +    .qfprom_min_vol = 1800000,
> >> +};
> >> +
> >> +static const struct of_device_id qfprom_efuse_of_match[] = {
> >> +    {
> >> +        .compatible = "qcom,sc7180-qfprom-efuse",
> >> +        .data = &sc7180_qfp_efuse_data
> >> +    },
> >> +    {/* sentinel */},
> >> +};
> >> +
> >> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
> >> +
> >> +static struct platform_driver qfprom_efuse_driver = {
> >> +    .probe = qfprom_efuse_probe,
> >> +    .driver = {
> >> +        .name = "sc7180-qfprom-efuse",
> >> +        .of_match_table = qfprom_efuse_of_match,
> >> +    },
> >> +};
> >> +
> >> +module_platform_driver(qfprom_efuse_driver);
> >> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
> >> +MODULE_LICENSE("GPL v2");
> >>
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
> member of the Code Aurora Forum, hosted by the Linux Foundation.

[1] https://lore.kernel.org/r/CAD=FV=UZQRfBTbh2CLnwsRSpbXFf=8iF2MG20hdj47s42aP8HQ@mail.gmail.com

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Ravi Kumar Bokka (Temp)]

Hi Doug,
Thanks for your feedback.
I have taken care most of the comments given by you initially and send 
to you as first patch-set. I thought to address other pending comments 
in next patch-set along with upstream maintainer suggestions as well.
I will address rest of the comments and send for the review asap.


Regards,
Ravi Kumar.B

On 5/14/2020 11:51 PM, Doug Anderson wrote:
> Hi,
> 
> I notice that you didn't respond to any of my feedback [1], only
> Srinivas's.  Any reason why?  It turns out that Srinivas had quite a
> lot of the same feedback as I did, but please make sure you didn't
> miss anything I suggested.  More inline below.
> 
> On Thu, May 14, 2020 at 5:26 AM Ravi Kumar Bokka (Temp)
> <rbokka@codeaurora.org> wrote:
>>
>> Hi Srinivas,
>> Thanks for your feedback by giving review comments. Please find my
>> inline comments.
>>
>>
>> Regards,
>> Ravi Kumar.B
>>
>> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>>>> This patch adds new driver for QTI qfprom-efuse controller. This
>>>> driver can
>>>> access the raw qfprom regions for fuse blowing.
>>>
>>> QTI?
>>
>> guidance I have received from internal Legal/LOST team is that the QCOM
>> prefix needs to be changed to QTI everywhere it is used
> 
> I'll let Srinivas comment if he cares.  I'm really not sure why a
> legal team cares about the Kconfig name in a GPL-licensed Linux
> kernel.
> 
> 
>>>> The current existed qfprom driver is only supports for cpufreq,
>>>> thermal sensors
>>>> drivers by read out calibration data, speed bins..etc which is stored
>>>> by qfprom efuses.
>>>
>>> Can you explain bit more about this QFPROM instance, Is this QFPROM part
>>> of secure controller address space?
>>> Is this closely tied to SoC or Secure controller version?
>>>
>>> Any reason why this can not be integrated into qfprom driver with
>>> specific compatible.
>>>
>>
>> QFPROM driver communicates with sec_controller address space however
>> scope and functionalities of this driver is different and not limited as
>> existing qfprom fuse Read-Only driver for specific “fuse buckets’ like
>> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch
>> requires specific sequence to write/blow fuses unlike other driver.
>> Scope/functionalities are different and this is separate driver.
> 
> If the underlying IP blocks are the same it should be one driver and
> it should just work in read-only mode for the other range of stuff.
> 
> 
>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>> ---
>>>>    drivers/nvmem/Kconfig        |  10 +
>>>>    drivers/nvmem/Makefile       |   2 +
>>>>    drivers/nvmem/qfprom-efuse.c | 476
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 488 insertions(+)
>>>>    create mode 100644 drivers/nvmem/qfprom-efuse.c
>>>>
>>> ...
>>>
>>>> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
>>>> new file mode 100644
>>>> index 0000000..2e3c275
>>>> --- /dev/null
>>>> +++ b/drivers/nvmem/qfprom-efuse.c
>>>> @@ -0,0 +1,476 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/nvmem-provider.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>>>> +#define QFPROM_BLOW_STATUS_READY 0x0
>>>> +
>>>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>>>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>>>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>>>> +
>>>> +/* Amount of time required to hold charge to blow fuse in
>>>> micro-seconds */
>>>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>>>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>>>> +
>>>> +#define QFPROM_ACCEL_OFFSET 0x044
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>>>> + * platform data
>>>> + *
>>>> + * @name: qfprom-efuse compatible name
>>>
>>> ??
>>
>> Thanks for your feedback. I will address this change
>>
>>>> + * @fuse_blow_time_in_us: Should contain the wait time when doing the
>>>> fuse blow
>>>> + * @accel_value: Should contain qfprom accel value
>>>> + * @accel_reset_value: The reset value of qfprom accel value
>>>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
>>>> efuse blow
>>>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
>>>> blowing
>>>> + * is done
>>>> + * @qfprom_blow_set_freq: The frequency required to set when we start
>>>> the
>>>> + * fuse blowing
>>>> + * @qfprom_max_vol: max voltage required to set fuse blow
>>>> + * @qfprom_min_vol: min voltage required to set fuse blow
>>>
>>> How specific are these values per SoC?
>>>
>>
>> This voltage level may change based on SoC and/or fuse-hardware
>> technology, it would change for SoC with different technology, hence we
>> have kept it in SOC specific settings.
> 
> Generally I'd expect the SoC specific settings to be in the device
> tree.  Drivers don't need to specify this.  Please respond to the
> comments I posed in my review.
> 
> 
>>>> + */
>>>> +struct qfprom_efuse_platform_data {
>>>> +    const char *name;
>>>> +    u8 fuse_blow_time_in_us;
>>>> +    u32 accel_value;
>>>> +    u32 accel_reset_value;
>>>> +    u32 qfprom_blow_timer_value;
>>>> +    u32 qfprom_blow_reset_freq;
>>>> +    u32 qfprom_blow_set_freq;
>>>> +    u32 qfprom_max_vol;
>>>> +    u32 qfprom_min_vol;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
>>>> + *
>>>> + * @qfpbase: iomapped memory space for qfprom base
>>>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>>>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>>>> +
>>>> + * @dev: qfprom device structure
>>>> + * @secclk: clock supply
>>>> + * @vcc: regulator supply
>>>> +
>>>> + * @qfpraw_start: qfprom raw fuse start region
>>>> + * @qfpraw_end: qfprom raw fuse end region
>>>> + * @qfprom_efuse_platform_data: qfprom platform data
>>>> + */
>>>> +struct qfprom_efuse_priv {
>>>> +    void __iomem *qfpbase;
>>>> +    void __iomem *qfpraw;
>>>> +    void __iomem *qfpmap;
>>>
>>> Why are these memory regions split? Can't you just have complete qfprom
>>> area and add fixed offset for qfpraw within the driver?
>>>
>>
>> Thanks for your feedback. I will address this change.
>> I have separated this memory regions because to identify raw fuse
>> regions separately and compare these raw fuse regions from the user
>> given input.
> 
> How are you addressing?  Can you go back to just having one range?  If
> you need to know where the raw fuse region is inside your range just
> put the offset in the of_match data.
> 
> 
>>>> +    struct device *dev;
>>>> +    struct clk *secclk;
>>>> +    struct regulator *vcc;
>>>> +    resource_size_t qfpraw_start;
>>>> +    resource_size_t qfpraw_end;
>>> Why do we need to check this range? as long as we set the nvmem_config
>>> with correct range then you should not need this check.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and based
>> on internal review with our security team, this check is important to
>> avoid dependency on other upper layer.
> 
> There is harm: it adds extra complexity.  Please remove.
> 
> You are talking as if it was somehow important for this code to be the
> same as the code on other OSes / in other contexts.  It isn't.  This
> is a Linux driver and it should not be written to duplicate stuff that
> Linux is already doing.
> 
> 
>>>> +    struct qfprom_efuse_platform_data efuse;
>>> A pointer here should be good enough?
>>>> +};
>>>> +
>>>
>>
>> Thanks for your feedback. I will address this change
>>
>>> ...
>>>
>>>> +/*
>>>> + * sets the value of the blow timer, accel register and the clock
>>>> + * and voltage settings
>>>> + */
>>>> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv
>>>> *priv)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ret = qfprom_disable_fuse_blowing(priv);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>>>> +        return ret;
>>>> +    }
>>>
>>> Why do we need to qfprom_disable_fuse_blowing() for every call to enable
>>> it?
>>>
>>> Or are we missing some error handling in the caller?
>>>
>>
>> We must disable/vote-off this QFPROM fuse power rail after blowing fuse,
>> it is the safe and right approach as per hardware programming guide for
>> fuse blowing process. Caller here is user space, can’t control
>> fuse-power-rail or can’t be relied to follow the required process. There
>> could also be unnecessary risk of leaving the vote/power-rail configured
>> at specific level after blowing the fuse. As per hardware requirement,
>> right after fuse blowing, we need to disable power rail.
> 
> Please remove your disable here.  Though the user initiates the call
> you can still rely on Linux to make sure two users aren't trying to
> blow fuses at the same time.  The Linux driver should always leave
> things in a "disabled" state and you can rely on that.
> 
> ...besides the only way you aren't hitting an underflow on the
> regulator enable count is that you are constantly enabling over and
> over again.
> 
> 
>>>> +
>>>> +    writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>>>> +           QFPROM_BLOW_TIMER_OFFSET);
>>>> +    writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
>>>> +
>>>> +    ret = qfprom_set_clock_settings(priv);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
>>>> +                      priv->efuse.qfprom_max_vol);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>
>>> <<
>>>> +/*
>>>> + * verifying to make sure address being written or read is from qfprom
>>>> + * raw address range
>>>> + */
>>>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
>>>> +              size_t bytes)
>>>> +{
>>>> +    if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>>>> +        ((reg + bytes) <= priv->qfpraw_end)) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>   >>
>>> Above function is totally redundant, nvmem core already has checks for
>>> this.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and based
>> on internal review with our security team, this check is important to
>> avoid dependency on other upper layer.
> 
> Please remove.  You are a Linux driver.
> 
> 
>>>> +
>>>> +/*
>>>> + * API for reading from raw qfprom region
>>>> + */
>>>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
>>>> void *_val,
>>>> +                 size_t bytes)
>>>> +{
>>>> +    struct qfprom_efuse_priv *priv = context;
>>>> +    u32 *value = _val;
>>>> +    u32 align_check;
>>>> +    int i = 0, words = bytes / 4;
>>>> +
>>>> +    dev_info(priv->dev,
>>>> +         "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>>>> +         reg, bytes);
>>>
>>> In general there is lot of debug info across the code, do you really
>>> need all this? Consider removing these!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +    if (bytes % 4 != 0x00) {
>>>> +        dev_err(priv->dev,
>>>> +            "Bytes: %zd to read should be word align\n",
>>>> +            bytes);
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> This word align check is also redundant once you set nvmem_config with
>>> correct word_size.
>>>
>>
>> I understand that there may be different approach to handle this. We
>> have used this approach and tested this driver thoroughly. Unless there
>> is technical limitation, changing this word_size would end up requiring
>> re-writing write/read APIs and going through testing again, there is not
>> much difference in either approach, we would like to keep this approach
>> unless there is technical concern.
> 
> The driver isn't done until it lands in Linux.  While it's important
> to test the driver before posting upstream it is completely expected
> and normal that then posting upstream you will be asked to change
> things.  After you change things you will need to re-test.  The fact
> that you already tested this the old way is not an excuse.  Please
> fix.
> 
> 
>>>> +
>>>> +    if (!addr_in_qfprom_range(priv, reg, bytes)) {
>>>> +        dev_err(priv->dev,
>>>> +            "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>>>> +            reg, bytes);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    align_check = (reg & 0xF);
>>>> +
>>>> +    if (((align_check & ~3) == align_check) && value != NULL)
>>>> +        while (words--)
>>>> +            *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>>>> +
>>>> +    else
>>>> +        dev_err(priv->dev,
>>>> +            "Invalid input parameter 0x%08x fuse blow address\n",
>>>> +            reg);
>>>> +
>>>> +    return 0;
>>>> +}
>>> ...
>>>
>>>> +
>>>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct resource *qfpbase, *qfpraw, *qfpmap;
>>>> +    struct nvmem_device *nvmem;
>>>> +    struct nvmem_config *econfig;
>>>> +    struct qfprom_efuse_priv *priv;
>>>> +    const struct qfprom_efuse_platform_data *drvdata;
>>>> +    int ret;
>>>> +
>>>> +    dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>>>> +
>>>
>>> too much debug!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +    drvdata = of_device_get_match_data(&pdev->dev);
>>>> +    if (!drvdata)
>>>> +        return -EINVAL;
>>> Unnecessary check as this driver will not be probed unless there is a
>>> compatible match.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>
>>>> +
>>>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +    if (!priv)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>>>> +    priv->efuse.accel_value = drvdata->accel_value;
>>>> +    priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>>>> +    priv->efuse.qfprom_blow_timer_value =
>>>> drvdata->qfprom_blow_timer_value;
>>>> +    priv->efuse.qfprom_blow_reset_freq =
>>>> drvdata->qfprom_blow_reset_freq;
>>>> +    priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>>>> +    priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>>>> +    priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>>>> +    priv->dev = dev;
>>>> +
>>>> +    qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +
>>>> +    priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>>>> +    if (IS_ERR(priv->qfpbase)) {
>>>> +        ret = PTR_ERR(priv->qfpbase);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +
>>>> +    priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>>>> +    if (IS_ERR(priv->qfpraw)) {
>>>> +        ret = PTR_ERR(priv->qfpraw);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->qfpraw_start = qfpraw->start - qfpbase->start;
>>>> +    priv->qfpraw_end = qfpraw->end - qfpbase->start;
>>>> +
>>>> +    qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>>> +
>>>> +    priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>>>> +    if (IS_ERR(priv->qfpmap)) {
>>>> +        ret = PTR_ERR(priv->qfpmap);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>>
>>> I see no reference to this regulator in dt bindings.
>>
>> This perameter kept in board specific file i.e., sc7180-idp.dts file
> 
> Yes, but it still needs to be in the bindings.
> 
> 
>>>> +    if (IS_ERR(priv->vcc)) {
>>>> +        ret = PTR_ERR(priv->vcc);
>>>> +        if (ret == -ENODEV)
>>>> +            ret = -EPROBE_DEFER;
>>> Can you explain what is going on here?
>>>
>>
>> As i took other drivers reference, i have kept this check.
> 
> Then other drivers are wrong.  Please remove.
> 
> 
>>>> +
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->secclk = devm_clk_get(dev, "secclk");
>>>> +    if (IS_ERR(priv->secclk)) {
>>>> +        ret = PTR_ERR(priv->secclk);
>>>> +        if (ret != -EPROBE_DEFER)
>>>> +            dev_err(dev, "secclk error getting : %d\n", ret);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    ret = clk_prepare_enable(priv->secclk);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "clk_prepare_enable() failed\n");
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>>>> +    if (!econfig)
>>> Why not disabling the clk here?
>>>> +        return -ENOMEM;
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +    econfig->dev = dev;
>>>> +    econfig->name = "qfprom-efuse";
>>>> +    econfig->stride = 1;
>>>> +    econfig->word_size = 1;
>>>> +    econfig->reg_read = qfprom_efuse_reg_read;
>>>> +    econfig->reg_write = qfprom_efuse_reg_write;
>>>> +    econfig->size = resource_size(qfpraw);
>>>> +    econfig->priv = priv;
>>>> +
>>>> +    nvmem = devm_nvmem_register(dev, econfig);
>>>> +
>>>> +    return PTR_ERR_OR_ZERO(nvmem);
>>> probably you should check the nvmem here before returning to disable the
>>> clk properly.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +err:
>>>> +    clk_disable_unprepare(priv->secclk);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
>>>> +    .name = "sc7180-qfprom-efuse",
>>> Redundant.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +    .fuse_blow_time_in_us = 10,
>>>> +    .accel_value = 0xD10,
>>>> +    .accel_reset_value = 0x800,
>>>> +    .qfprom_blow_timer_value = 25,
>>>> +    .qfprom_blow_reset_freq = 19200000,
>>>> +    .qfprom_blow_set_freq = 4800000,
>>>> +    .qfprom_max_vol = 1904000,
>>>> +    .qfprom_min_vol = 1800000,
>>>> +};
>>>> +
>>>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>>>> +    {
>>>> +        .compatible = "qcom,sc7180-qfprom-efuse",
>>>> +        .data = &sc7180_qfp_efuse_data
>>>> +    },
>>>> +    {/* sentinel */},
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>>>> +
>>>> +static struct platform_driver qfprom_efuse_driver = {
>>>> +    .probe = qfprom_efuse_probe,
>>>> +    .driver = {
>>>> +        .name = "sc7180-qfprom-efuse",
>>>> +        .of_match_table = qfprom_efuse_of_match,
>>>> +    },
>>>> +};
>>>> +
>>>> +module_platform_driver(qfprom_efuse_driver);
>>>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>
>> --
>> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of the Code Aurora Forum, hosted by the Linux Foundation.
> 
> [1] https://lore.kernel.org/r/CAD=FV=UZQRfBTbh2CLnwsRSpbXFf=8iF2MG20hdj47s42aP8HQ@mail.gmail.com
> 

-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Ravi Kumar Bokka (Temp)]

Hi Doug,
Please find my inline comments.



On 5/14/2020 11:51 PM, Doug Anderson wrote:
> Hi,
> 
> I notice that you didn't respond to any of my feedback [1], only
> Srinivas's.  Any reason why?  It turns out that Srinivas had quite a
> lot of the same feedback as I did, but please make sure you didn't
> miss anything I suggested.  More inline below.
> 
> On Thu, May 14, 2020 at 5:26 AM Ravi Kumar Bokka (Temp)
> <rbokka@codeaurora.org> wrote:
>>
>> Hi Srinivas,
>> Thanks for your feedback by giving review comments. Please find my
>> inline comments.
>>
>>
>> Regards,
>> Ravi Kumar.B
>>
>> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>>>> This patch adds new driver for QTI qfprom-efuse controller. This
>>>> driver can
>>>> access the raw qfprom regions for fuse blowing.
>>>
>>> QTI?
>>
>> guidance I have received from internal Legal/LOST team is that the QCOM
>> prefix needs to be changed to QTI everywhere it is used
> 
> I'll let Srinivas comment if he cares.  I'm really not sure why a
> legal team cares about the Kconfig name in a GPL-licensed Linux
> kernel.
> 

I will maintain Qualcomm Technologies Inc(QTI) in the patch description 
and change Kconfig as you suggested.

>
>>>> The current existed qfprom driver is only supports for cpufreq,
>>>> thermal sensors
>>>> drivers by read out calibration data, speed bins..etc which is stored
>>>> by qfprom efuses.
>>>
>>> Can you explain bit more about this QFPROM instance, Is this QFPROM part
>>> of secure controller address space?
>>> Is this closely tied to SoC or Secure controller version?
>>>
>>> Any reason why this can not be integrated into qfprom driver with
>>> specific compatible.
>>>
>>
>> QFPROM driver communicates with sec_controller address space however
>> scope and functionalities of this driver is different and not limited as
>> existing qfprom fuse Read-Only driver for specific “fuse buckets’ like
>> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch
>> requires specific sequence to write/blow fuses unlike other driver.
>> Scope/functionalities are different and this is separate driver.
> 
> If the underlying IP blocks are the same it should be one driver and
> it should just work in read-only mode for the other range of stuff.
> 
Based on the compatible, do i need to separate probe function for 
qfprom-efuse and maintain separate nvmem object to register nvmem 
framework. Is this what you are suggesting to implementing this in to 
one existing driver?
Do I need to maintain separate efuse dt node?

Could you please suggest me to proceed further.

> 
>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>> ---
>>>>    drivers/nvmem/Kconfig        |  10 +
>>>>    drivers/nvmem/Makefile       |   2 +
>>>>    drivers/nvmem/qfprom-efuse.c | 476
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>    3 files changed, 488 insertions(+)
>>>>    create mode 100644 drivers/nvmem/qfprom-efuse.c
>>>>
>>> ...
>>>
>>>> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
>>>> new file mode 100644
>>>> index 0000000..2e3c275
>>>> --- /dev/null
>>>> +++ b/drivers/nvmem/qfprom-efuse.c
>>>> @@ -0,0 +1,476 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/nvmem-provider.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>>>> +#define QFPROM_BLOW_STATUS_READY 0x0
>>>> +
>>>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>>>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>>>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>>>> +
>>>> +/* Amount of time required to hold charge to blow fuse in
>>>> micro-seconds */
>>>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>>>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>>>> +
>>>> +#define QFPROM_ACCEL_OFFSET 0x044
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>>>> + * platform data
>>>> + *
>>>> + * @name: qfprom-efuse compatible name
>>>
>>> ??
>>
>> Thanks for your feedback. I will address this change
>>
>>>> + * @fuse_blow_time_in_us: Should contain the wait time when doing the
>>>> fuse blow
>>>> + * @accel_value: Should contain qfprom accel value
>>>> + * @accel_reset_value: The reset value of qfprom accel value
>>>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing
>>>> efuse blow
>>>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse
>>>> blowing
>>>> + * is done
>>>> + * @qfprom_blow_set_freq: The frequency required to set when we start
>>>> the
>>>> + * fuse blowing
>>>> + * @qfprom_max_vol: max voltage required to set fuse blow
>>>> + * @qfprom_min_vol: min voltage required to set fuse blow
>>>
>>> How specific are these values per SoC?
>>>
>>
>> This voltage level may change based on SoC and/or fuse-hardware
>> technology, it would change for SoC with different technology, hence we
>> have kept it in SOC specific settings.
> 
> Generally I'd expect the SoC specific settings to be in the device
> tree.  Drivers don't need to specify this.  Please respond to the
> comments I posed in my review.
> 

Thanks for your feedback. I will address this change.

> 
>>>> + */
>>>> +struct qfprom_efuse_platform_data {
>>>> +    const char *name;
>>>> +    u8 fuse_blow_time_in_us;
>>>> +    u32 accel_value;
>>>> +    u32 accel_reset_value;
>>>> +    u32 qfprom_blow_timer_value;
>>>> +    u32 qfprom_blow_reset_freq;
>>>> +    u32 qfprom_blow_set_freq;
>>>> +    u32 qfprom_max_vol;
>>>> +    u32 qfprom_min_vol;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
>>>> + *
>>>> + * @qfpbase: iomapped memory space for qfprom base
>>>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>>>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>>>> +
>>>> + * @dev: qfprom device structure
>>>> + * @secclk: clock supply
>>>> + * @vcc: regulator supply
>>>> +
>>>> + * @qfpraw_start: qfprom raw fuse start region
>>>> + * @qfpraw_end: qfprom raw fuse end region
>>>> + * @qfprom_efuse_platform_data: qfprom platform data
>>>> + */
>>>> +struct qfprom_efuse_priv {
>>>> +    void __iomem *qfpbase;
>>>> +    void __iomem *qfpraw;
>>>> +    void __iomem *qfpmap;
>>>
>>> Why are these memory regions split? Can't you just have complete qfprom
>>> area and add fixed offset for qfpraw within the driver?
>>>
>>
>> Thanks for your feedback. I will address this change.
>> I have separated this memory regions because to identify raw fuse
>> regions separately and compare these raw fuse regions from the user
>> given input.
> 
> How are you addressing?  Can you go back to just having one range?  If
> you need to know where the raw fuse region is inside your range just
> put the offset in the of_match data.
> 

I will maintain one range as suggested reg = <0 0x00780000 0 0x2100>.

> 
>>>> +    struct device *dev;
>>>> +    struct clk *secclk;
>>>> +    struct regulator *vcc;
>>>> +    resource_size_t qfpraw_start;
>>>> +    resource_size_t qfpraw_end;
>>> Why do we need to check this range? as long as we set the nvmem_config
>>> with correct range then you should not need this check.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and based
>> on internal review with our security team, this check is important to
>> avoid dependency on other upper layer.
> 
> There is harm: it adds extra complexity.  Please remove.
> 
> You are talking as if it was somehow important for this code to be the
> same as the code on other OSes / in other contexts.  It isn't.  This
> is a Linux driver and it should not be written to duplicate stuff that
> Linux is already doing.
> 

This for your feedback. I will address this change.

> 
>>>> +    struct qfprom_efuse_platform_data efuse;
>>> A pointer here should be good enough?
>>>> +};
>>>> +
>>>
>>
>> Thanks for your feedback. I will address this change
>>
>>> ...
>>>
>>>> +/*
>>>> + * sets the value of the blow timer, accel register and the clock
>>>> + * and voltage settings
>>>> + */
>>>> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv
>>>> *priv)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ret = qfprom_disable_fuse_blowing(priv);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>>>> +        return ret;
>>>> +    }
>>>
>>> Why do we need to qfprom_disable_fuse_blowing() for every call to enable
>>> it?
>>>
>>> Or are we missing some error handling in the caller?
>>>
>>
>> We must disable/vote-off this QFPROM fuse power rail after blowing fuse,
>> it is the safe and right approach as per hardware programming guide for
>> fuse blowing process. Caller here is user space, can’t control
>> fuse-power-rail or can’t be relied to follow the required process. There
>> could also be unnecessary risk of leaving the vote/power-rail configured
>> at specific level after blowing the fuse. As per hardware requirement,
>> right after fuse blowing, we need to disable power rail.
> 
> Please remove your disable here.  Though the user initiates the call
> you can still rely on Linux to make sure two users aren't trying to
> blow fuses at the same time.  The Linux driver should always leave
> things in a "disabled" state and you can rely on that.
> 
> ...besides the only way you aren't hitting an underflow on the
> regulator enable count is that you are constantly enabling over and
> over again.
> 

I will address this change.

> 
>>>> +
>>>> +    writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>>>> +           QFPROM_BLOW_TIMER_OFFSET);
>>>> +    writel(priv->efuse.accel_value, priv->qfpmap + QFPROM_ACCEL_OFFSET);
>>>> +
>>>> +    ret = qfprom_set_clock_settings(priv);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
>>>> +                      priv->efuse.qfprom_max_vol);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>
>>> <<
>>>> +/*
>>>> + * verifying to make sure address being written or read is from qfprom
>>>> + * raw address range
>>>> + */
>>>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
>>>> +              size_t bytes)
>>>> +{
>>>> +    if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>>>> +        ((reg + bytes) <= priv->qfpraw_end)) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>   >>
>>> Above function is totally redundant, nvmem core already has checks for
>>> this.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and based
>> on internal review with our security team, this check is important to
>> avoid dependency on other upper layer.
> 
> Please remove.  You are a Linux driver.
> 

I will address this change.

> 
>>>> +
>>>> +/*
>>>> + * API for reading from raw qfprom region
>>>> + */
>>>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg,
>>>> void *_val,
>>>> +                 size_t bytes)
>>>> +{
>>>> +    struct qfprom_efuse_priv *priv = context;
>>>> +    u32 *value = _val;
>>>> +    u32 align_check;
>>>> +    int i = 0, words = bytes / 4;
>>>> +
>>>> +    dev_info(priv->dev,
>>>> +         "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>>>> +         reg, bytes);
>>>
>>> In general there is lot of debug info across the code, do you really
>>> need all this? Consider removing these!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +    if (bytes % 4 != 0x00) {
>>>> +        dev_err(priv->dev,
>>>> +            "Bytes: %zd to read should be word align\n",
>>>> +            bytes);
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> This word align check is also redundant once you set nvmem_config with
>>> correct word_size.
>>>
>>
>> I understand that there may be different approach to handle this. We
>> have used this approach and tested this driver thoroughly. Unless there
>> is technical limitation, changing this word_size would end up requiring
>> re-writing write/read APIs and going through testing again, there is not
>> much difference in either approach, we would like to keep this approach
>> unless there is technical concern.
> 
> The driver isn't done until it lands in Linux.  While it's important
> to test the driver before posting upstream it is completely expected
> and normal that then posting upstream you will be asked to change
> things.  After you change things you will need to re-test.  The fact
> that you already tested this the old way is not an excuse.  Please
> fix.
> 

Thanks for your feedback, i will address this change.

> 
>>>> +
>>>> +    if (!addr_in_qfprom_range(priv, reg, bytes)) {
>>>> +        dev_err(priv->dev,
>>>> +            "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>>>> +            reg, bytes);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    align_check = (reg & 0xF);
>>>> +
>>>> +    if (((align_check & ~3) == align_check) && value != NULL)
>>>> +        while (words--)
>>>> +            *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>>>> +
>>>> +    else
>>>> +        dev_err(priv->dev,
>>>> +            "Invalid input parameter 0x%08x fuse blow address\n",
>>>> +            reg);
>>>> +
>>>> +    return 0;
>>>> +}
>>> ...
>>>
>>>> +
>>>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct resource *qfpbase, *qfpraw, *qfpmap;
>>>> +    struct nvmem_device *nvmem;
>>>> +    struct nvmem_config *econfig;
>>>> +    struct qfprom_efuse_priv *priv;
>>>> +    const struct qfprom_efuse_platform_data *drvdata;
>>>> +    int ret;
>>>> +
>>>> +    dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>>>> +
>>>
>>> too much debug!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +    drvdata = of_device_get_match_data(&pdev->dev);
>>>> +    if (!drvdata)
>>>> +        return -EINVAL;
>>> Unnecessary check as this driver will not be probed unless there is a
>>> compatible match.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>
>>>> +
>>>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +    if (!priv)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>>>> +    priv->efuse.accel_value = drvdata->accel_value;
>>>> +    priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>>>> +    priv->efuse.qfprom_blow_timer_value =
>>>> drvdata->qfprom_blow_timer_value;
>>>> +    priv->efuse.qfprom_blow_reset_freq =
>>>> drvdata->qfprom_blow_reset_freq;
>>>> +    priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>>>> +    priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>>>> +    priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>>>> +    priv->dev = dev;
>>>> +
>>>> +    qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +
>>>> +    priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>>>> +    if (IS_ERR(priv->qfpbase)) {
>>>> +        ret = PTR_ERR(priv->qfpbase);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +
>>>> +    priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>>>> +    if (IS_ERR(priv->qfpraw)) {
>>>> +        ret = PTR_ERR(priv->qfpraw);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->qfpraw_start = qfpraw->start - qfpbase->start;
>>>> +    priv->qfpraw_end = qfpraw->end - qfpbase->start;
>>>> +
>>>> +    qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>>> +
>>>> +    priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>>>> +    if (IS_ERR(priv->qfpmap)) {
>>>> +        ret = PTR_ERR(priv->qfpmap);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>>
>>> I see no reference to this regulator in dt bindings.
>>
>> This perameter kept in board specific file i.e., sc7180-idp.dts file
> 
> Yes, but it still needs to be in the bindings.
> 

Thanks for your feedback. i will address this change.

> 
>>>> +    if (IS_ERR(priv->vcc)) {
>>>> +        ret = PTR_ERR(priv->vcc);
>>>> +        if (ret == -ENODEV)
>>>> +            ret = -EPROBE_DEFER;
>>> Can you explain what is going on here?
>>>
>>
>> As i took other drivers reference, i have kept this check.
> 
> Then other drivers are wrong.  Please remove.
> 

Thanks for your feedback. I will address this change.

> 
>>>> +
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->secclk = devm_clk_get(dev, "secclk");
>>>> +    if (IS_ERR(priv->secclk)) {
>>>> +        ret = PTR_ERR(priv->secclk);
>>>> +        if (ret != -EPROBE_DEFER)
>>>> +            dev_err(dev, "secclk error getting : %d\n", ret);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    ret = clk_prepare_enable(priv->secclk);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "clk_prepare_enable() failed\n");
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>>>> +    if (!econfig)
>>> Why not disabling the clk here?
>>>> +        return -ENOMEM;
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +    econfig->dev = dev;
>>>> +    econfig->name = "qfprom-efuse";
>>>> +    econfig->stride = 1;
>>>> +    econfig->word_size = 1;
>>>> +    econfig->reg_read = qfprom_efuse_reg_read;
>>>> +    econfig->reg_write = qfprom_efuse_reg_write;
>>>> +    econfig->size = resource_size(qfpraw);
>>>> +    econfig->priv = priv;
>>>> +
>>>> +    nvmem = devm_nvmem_register(dev, econfig);
>>>> +
>>>> +    return PTR_ERR_OR_ZERO(nvmem);
>>> probably you should check the nvmem here before returning to disable the
>>> clk properly.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +err:
>>>> +    clk_disable_unprepare(priv->secclk);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
>>>> +    .name = "sc7180-qfprom-efuse",
>>> Redundant.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +    .fuse_blow_time_in_us = 10,
>>>> +    .accel_value = 0xD10,
>>>> +    .accel_reset_value = 0x800,
>>>> +    .qfprom_blow_timer_value = 25,
>>>> +    .qfprom_blow_reset_freq = 19200000,
>>>> +    .qfprom_blow_set_freq = 4800000,
>>>> +    .qfprom_max_vol = 1904000,
>>>> +    .qfprom_min_vol = 1800000,
>>>> +};
>>>> +
>>>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>>>> +    {
>>>> +        .compatible = "qcom,sc7180-qfprom-efuse",
>>>> +        .data = &sc7180_qfp_efuse_data
>>>> +    },
>>>> +    {/* sentinel */},
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>>>> +
>>>> +static struct platform_driver qfprom_efuse_driver = {
>>>> +    .probe = qfprom_efuse_probe,
>>>> +    .driver = {
>>>> +        .name = "sc7180-qfprom-efuse",
>>>> +        .of_match_table = qfprom_efuse_of_match,
>>>> +    },
>>>> +};
>>>> +
>>>> +module_platform_driver(qfprom_efuse_driver);
>>>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>
>> --
>> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a
>> member of the Code Aurora Forum, hosted by the Linux Foundation.
> 
> [1] https://lore.kernel.org/r/CAD=FV=UZQRfBTbh2CLnwsRSpbXFf=8iF2MG20hdj47s42aP8HQ@mail.gmail.com
> 


Regards,
Ravi Kumar.B

-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 14/05/2020 13:26, Ravi Kumar Bokka (Temp) wrote:
> Hi Srinivas,
> Thanks for your feedback by giving review comments. Please find my 
> inline comments.
> 
> 
> Regards,
> Ravi Kumar.B
> 
> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>>> This patch adds new driver for QTI qfprom-efuse controller. This 
>>> driver can
>>> access the raw qfprom regions for fuse blowing.
>>
>> QTI?
> 
> guidance I have received from internal Legal/LOST team is that the QCOM 
> prefix needs to be changed to QTI everywhere it is used
> 

I dont mind this in comments or patch subject line but the valid vendor 
prefix for Qualcomm is "qcom".


>>
>>>
>>> The current existed qfprom driver is only supports for cpufreq, 
>>> thermal sensors
>>> drivers by read out calibration data, speed bins..etc which is stored
>>> by qfprom efuses.
>>
>> Can you explain bit more about this QFPROM instance, Is this QFPROM 
>> part of secure controller address space?
>> Is this closely tied to SoC or Secure controller version?
>>
>> Any reason why this can not be integrated into qfprom driver with 
>> specific compatible.
>>
> 
> QFPROM driver communicates with sec_controller address space however 
> scope and functionalities of this driver is different and not limited as 
> existing qfprom fuse Read-Only driver for specific “fuse buckets’ like 
> cpufreq, thermal sensors etc. QFPROM fuse write driver in this patch 
> requires specific sequence to write/blow fuses unlike other driver. 
> Scope/functionalities are different and this is separate driver.
> 

This is another variant of qfprom, so please add this support in the 
existing qfprom driver, you could deal with the differences using 
specific compatible within the driver.

Doug already covered most of the comments, consider them as mandatory 
requirements for upstreaming!

--srini


>>>
>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>> ---
>>>   drivers/nvmem/Kconfig        |  10 +
>>>   drivers/nvmem/Makefile       |   2 +
>>>   drivers/nvmem/qfprom-efuse.c | 476 
>>> +++++++++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 488 insertions(+)
>>>   create mode 100644 drivers/nvmem/qfprom-efuse.c
>>>
>> ...
>>
>>> diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
>>> new file mode 100644
>>> index 0000000..2e3c275
>>> --- /dev/null
>>> +++ b/drivers/nvmem/qfprom-efuse.c
>>> @@ -0,0 +1,476 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>> + */
>>> +
>>> +#include <linux/clk.h>
>>> +#include <linux/device.h>
>>> +#include <linux/io.h>
>>> +#include <linux/iopoll.h>
>>> +#include <linux/kernel.h>
>>> +#include <linux/module.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/nvmem-provider.h>
>>> +#include <linux/of_device.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/regulator/consumer.h>
>>> +
>>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>>> +#define QFPROM_BLOW_STATUS_READY 0x0
>>> +
>>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>>> +
>>> +/* Amount of time required to hold charge to blow fuse in 
>>> micro-seconds */
>>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>>> +
>>> +#define QFPROM_ACCEL_OFFSET 0x044
>>> +
>>> +/**
>>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>>> + * platform data
>>> + *
>>> + * @name: qfprom-efuse compatible name
>>
>> ??
> 
> Thanks for your feedback. I will address this change
> 
>>> + * @fuse_blow_time_in_us: Should contain the wait time when doing 
>>> the fuse blow
>>> + * @accel_value: Should contain qfprom accel value
>>> + * @accel_reset_value: The reset value of qfprom accel value
>>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing 
>>> efuse blow
>>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse 
>>> blowing
>>> + * is done
>>> + * @qfprom_blow_set_freq: The frequency required to set when we 
>>> start the
>>> + * fuse blowing
>>> + * @qfprom_max_vol: max voltage required to set fuse blow
>>> + * @qfprom_min_vol: min voltage required to set fuse blow
>>
>> How specific are these values per SoC?
>>
> 
> This voltage level may change based on SoC and/or fuse-hardware 
> technology, it would change for SoC with different technology, hence we 
> have kept it in SOC specific settings.
> 
>>
>>> + */
>>> +struct qfprom_efuse_platform_data {
>>> +    const char *name;
>>> +    u8 fuse_blow_time_in_us;
>>> +    u32 accel_value;
>>> +    u32 accel_reset_value;
>>> +    u32 qfprom_blow_timer_value;
>>> +    u32 qfprom_blow_reset_freq;
>>> +    u32 qfprom_blow_set_freq;
>>> +    u32 qfprom_max_vol;
>>> +    u32 qfprom_min_vol;
>>> +};
>>> +
>>> +/**
>>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse attributes
>>> + *
>>> + * @qfpbase: iomapped memory space for qfprom base
>>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>>> +
>>> + * @dev: qfprom device structure
>>> + * @secclk: clock supply
>>> + * @vcc: regulator supply
>>> +
>>> + * @qfpraw_start: qfprom raw fuse start region
>>> + * @qfpraw_end: qfprom raw fuse end region
>>> + * @qfprom_efuse_platform_data: qfprom platform data
>>> + */
>>> +struct qfprom_efuse_priv {
>>> +    void __iomem *qfpbase;
>>> +    void __iomem *qfpraw;
>>> +    void __iomem *qfpmap;
>>
>> Why are these memory regions split? Can't you just have complete 
>> qfprom area and add fixed offset for qfpraw within the driver?
>>
> 
> Thanks for your feedback. I will address this change.
> I have separated this memory regions because to identify raw fuse 
> regions separately and compare these raw fuse regions from the user 
> given input.
> 
>>> +    struct device *dev;
>>> +    struct clk *secclk;
>>> +    struct regulator *vcc;
>>> +    resource_size_t qfpraw_start;
>>> +    resource_size_t qfpraw_end;
>> Why do we need to check this range? as long as we set the nvmem_config 
>> with correct range then you should not need this check.
>>
> 
> There is no harm in this explicit check in QFPROM-fuse driver and based 
> on internal review with our security team, this check is important to 
> avoid dependency on other upper layer.
> 
> 
>>
>>> +    struct qfprom_efuse_platform_data efuse;
>> A pointer here should be good enough?
>>> +};
>>> +
>>
> 
> Thanks for your feedback. I will address this change
> 
>> ...
>>
>>> +/*
>>> + * sets the value of the blow timer, accel register and the clock
>>> + * and voltage settings
>>> + */
>>> +static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv 
>>> *priv)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = qfprom_disable_fuse_blowing(priv);
>>> +    if (ret) {
>>> +        dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>>> +        return ret;
>>> +    }
>>
>> Why do we need to qfprom_disable_fuse_blowing() for every call to 
>> enable it?
>>
>> Or are we missing some error handling in the caller?
>>
> 
> We must disable/vote-off this QFPROM fuse power rail after blowing fuse, 
> it is the safe and right approach as per hardware programming guide for 
> fuse blowing process. Caller here is user space, can’t control 
> fuse-power-rail or can’t be relied to follow the required process. There 
> could also be unnecessary risk of leaving the vote/power-rail configured 
> at specific level after blowing the fuse. As per hardware requirement, 
> right after fuse blowing, we need to disable power rail.
> 
>>> +
>>> +    writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>>> +           QFPROM_BLOW_TIMER_OFFSET);
>>> +    writel(priv->efuse.accel_value, priv->qfpmap + 
>>> QFPROM_ACCEL_OFFSET);
>>> +
>>> +    ret = qfprom_set_clock_settings(priv);
>>> +    if (ret) {
>>> +        dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    ret = qfprom_set_voltage_settings(priv, priv->efuse.qfprom_min_vol,
>>> +                      priv->efuse.qfprom_max_vol);
>>> +    if (ret) {
>>> +        dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>>> +        return ret;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +
>>
>> <<
>>> +/*
>>> + * verifying to make sure address being written or read is from qfprom
>>> + * raw address range
>>> + */
>>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 
>>> reg,
>>> +              size_t bytes)
>>> +{
>>> +    if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>>> +        ((reg + bytes) <= priv->qfpraw_end)) {
>>> +        return 1;
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>  >>
>> Above function is totally redundant, nvmem core already has checks for 
>> this.
>>
> 
> There is no harm in this explicit check in QFPROM-fuse driver and based 
> on internal review with our security team, this check is important to 
> avoid dependency on other upper layer.
> 
>>
>>
>>> +
>>> +/*
>>> + * API for reading from raw qfprom region
>>> + */
>>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg, 
>>> void *_val,
>>> +                 size_t bytes)
>>> +{
>>> +    struct qfprom_efuse_priv *priv = context;
>>> +    u32 *value = _val;
>>> +    u32 align_check;
>>> +    int i = 0, words = bytes / 4;
>>> +
>>> +    dev_info(priv->dev,
>>> +         "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>>> +         reg, bytes);
>>
>> In general there is lot of debug info across the code, do you really 
>> need all this? Consider removing these!
>>
> 
> Thanks for your feedback. I will address this change.
> 
>>> +
>>> +    if (bytes % 4 != 0x00) {
>>> +        dev_err(priv->dev,
>>> +            "Bytes: %zd to read should be word align\n",
>>> +            bytes);
>>> +        return -EINVAL;
>>> +    }
>>
>> This word align check is also redundant once you set nvmem_config with 
>> correct word_size.
>>
> 
> I understand that there may be different approach to handle this. We 
> have used this approach and tested this driver thoroughly. Unless there 
> is technical limitation, changing this word_size would end up requiring 
> re-writing write/read APIs and going through testing again, there is not 
> much difference in either approach, we would like to keep this approach 
> unless there is technical concern.
> 
>>
>>> +
>>> +    if (!addr_in_qfprom_range(priv, reg, bytes)) {
>>> +        dev_err(priv->dev,
>>> +            "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>>> +            reg, bytes);
>>> +        return -EINVAL;
>>> +    }
>>> +
>>> +    align_check = (reg & 0xF);
>>> +
>>> +    if (((align_check & ~3) == align_check) && value != NULL)
>>> +        while (words--)
>>> +            *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>>> +
>>> +    else
>>> +        dev_err(priv->dev,
>>> +            "Invalid input parameter 0x%08x fuse blow address\n",
>>> +            reg);
>>> +
>>> +    return 0;
>>> +}
>> ...
>>
>>> +
>>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>>> +{
>>> +    struct device *dev = &pdev->dev;
>>> +    struct resource *qfpbase, *qfpraw, *qfpmap;
>>> +    struct nvmem_device *nvmem;
>>> +    struct nvmem_config *econfig;
>>> +    struct qfprom_efuse_priv *priv;
>>> +    const struct qfprom_efuse_platform_data *drvdata;
>>> +    int ret;
>>> +
>>> +    dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>>> +
>>
>> too much debug!
>>
> 
> Thanks for your feedback. I will address this change.
> 
>>> +    drvdata = of_device_get_match_data(&pdev->dev);
>>> +    if (!drvdata)
>>> +        return -EINVAL;
>> Unnecessary check as this driver will not be probed unless there is a 
>> compatible match.
>>
> 
> Thanks for your feedback. I will address this change.
> 
>>
>>> +
>>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +    if (!priv)
>>> +        return -ENOMEM;
>>> +
>>> +    priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>>> +    priv->efuse.accel_value = drvdata->accel_value;
>>> +    priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>>> +    priv->efuse.qfprom_blow_timer_value = 
>>> drvdata->qfprom_blow_timer_value;
>>> +    priv->efuse.qfprom_blow_reset_freq = 
>>> drvdata->qfprom_blow_reset_freq;
>>> +    priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>>> +    priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>>> +    priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>>> +    priv->dev = dev;
>>> +
>>> +    qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +
>>> +    priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>>> +    if (IS_ERR(priv->qfpbase)) {
>>> +        ret = PTR_ERR(priv->qfpbase);
>>> +        goto err;
>>> +    }
>>> +
>>> +    qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>> +
>>> +    priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>>> +    if (IS_ERR(priv->qfpraw)) {
>>> +        ret = PTR_ERR(priv->qfpraw);
>>> +        goto err;
>>> +    }
>>> +
>>> +    priv->qfpraw_start = qfpraw->start - qfpbase->start;
>>> +    priv->qfpraw_end = qfpraw->end - qfpbase->start;
>>> +
>>> +    qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>> +
>>> +    priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>>> +    if (IS_ERR(priv->qfpmap)) {
>>> +        ret = PTR_ERR(priv->qfpmap);
>>> +        goto err;
>>> +    }
>>> +
>>> +    priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>
>> I see no reference to this regulator in dt bindings.
> 
> This perameter kept in board specific file i.e., sc7180-idp.dts file
> 
>>> +    if (IS_ERR(priv->vcc)) {
>>> +        ret = PTR_ERR(priv->vcc);
>>> +        if (ret == -ENODEV)
>>> +            ret = -EPROBE_DEFER;
>> Can you explain what is going on here?
>>
> 
> As i took other drivers reference, i have kept this check.
> 
>>> +
>>> +        goto err;
>>> +    }
>>> +
>>> +    priv->secclk = devm_clk_get(dev, "secclk");
>>> +    if (IS_ERR(priv->secclk)) {
>>> +        ret = PTR_ERR(priv->secclk);
>>> +        if (ret != -EPROBE_DEFER)
>>> +            dev_err(dev, "secclk error getting : %d\n", ret);
>>> +        goto err;
>>> +    }
>>> +
>>> +    ret = clk_prepare_enable(priv->secclk);
>>> +    if (ret) {
>>> +        dev_err(dev, "clk_prepare_enable() failed\n");
>>> +        goto err;
>>> +    }
>>> +
>>> +    econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>>> +    if (!econfig)
>> Why not disabling the clk here?
>>> +        return -ENOMEM;
>>
> 
> Thanks for your feedback. I will address this change.
> 
>>> +
>>> +    econfig->dev = dev;
>>> +    econfig->name = "qfprom-efuse";
>>> +    econfig->stride = 1;
>>> +    econfig->word_size = 1;
>>> +    econfig->reg_read = qfprom_efuse_reg_read;
>>> +    econfig->reg_write = qfprom_efuse_reg_write;
>>> +    econfig->size = resource_size(qfpraw);
>>> +    econfig->priv = priv;
>>> +
>>> +    nvmem = devm_nvmem_register(dev, econfig);
>>> +
>>> +    return PTR_ERR_OR_ZERO(nvmem);
>> probably you should check the nvmem here before returning to disable 
>> the clk properly.
>>
> 
> Thanks for your feedback. I will address this change.
> 
>>> +
>>> +err:
>>> +    clk_disable_unprepare(priv->secclk);
>>> +    return ret;
>>> +}
>>> +
>>> +static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data 
>>> = {
>>> +    .name = "sc7180-qfprom-efuse",
>> Redundant.
>>
> 
> Thanks for your feedback. I will address this change.
> 
>>> +    .fuse_blow_time_in_us = 10,
>>> +    .accel_value = 0xD10,
>>> +    .accel_reset_value = 0x800,
>>> +    .qfprom_blow_timer_value = 25,
>>> +    .qfprom_blow_reset_freq = 19200000,
>>> +    .qfprom_blow_set_freq = 4800000,
>>> +    .qfprom_max_vol = 1904000,
>>> +    .qfprom_min_vol = 1800000,
>>> +};
>>> +
>>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>>> +    {
>>> +        .compatible = "qcom,sc7180-qfprom-efuse",
>>> +        .data = &sc7180_qfp_efuse_data
>>> +    },
>>> +    {/* sentinel */},
>>> +};
>>> +
>>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>>> +
>>> +static struct platform_driver qfprom_efuse_driver = {
>>> +    .probe = qfprom_efuse_probe,
>>> +    .driver = {
>>> +        .name = "sc7180-qfprom-efuse",
>>> +        .of_match_table = qfprom_efuse_of_match,
>>> +    },
>>> +};
>>> +
>>> +module_platform_driver(qfprom_efuse_driver);
>>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>>> +MODULE_LICENSE("GPL v2");
>>>
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Ravi Kumar Bokka (Temp)]

Hi Srinivas,
Thanks for your feedback. Please find my inline-comments.


On 5/15/2020 4:39 PM, Srinivas Kandagatla wrote:
> 
> 
> On 14/05/2020 13:26, Ravi Kumar Bokka (Temp) wrote:
>> Hi Srinivas,
>> Thanks for your feedback by giving review comments. Please find my 
>> inline comments.
>>
>>
>> Regards,
>> Ravi Kumar.B
>>
>> On 5/13/2020 6:50 PM, Srinivas Kandagatla wrote:
>>>
>>>
>>> On 12/05/2020 19:17, Ravi Kumar Bokka wrote:
>>>> This patch adds new driver for QTI qfprom-efuse controller. This 
>>>> driver can
>>>> access the raw qfprom regions for fuse blowing.
>>>
>>> QTI?
>>
>> guidance I have received from internal Legal/LOST team is that the 
>> QCOM prefix needs to be changed to QTI everywhere it is used
>>
> 
> I dont mind this in comments or patch subject line but the valid vendor 
> prefix for Qualcomm is "qcom".
> 

I will maintain Qualcomm Technologies Inc(QTI) in the patch description 
and change Kconfig as suggested.

> 
>>>
>>>>
>>>> The current existed qfprom driver is only supports for cpufreq, 
>>>> thermal sensors
>>>> drivers by read out calibration data, speed bins..etc which is stored
>>>> by qfprom efuses.
>>>
>>> Can you explain bit more about this QFPROM instance, Is this QFPROM 
>>> part of secure controller address space?
>>> Is this closely tied to SoC or Secure controller version?
>>>
>>> Any reason why this can not be integrated into qfprom driver with 
>>> specific compatible.
>>>
>>
>> QFPROM driver communicates with sec_controller address space however 
>> scope and functionalities of this driver is different and not limited 
>> as existing qfprom fuse Read-Only driver for specific “fuse buckets’ 
>> like cpufreq, thermal sensors etc. QFPROM fuse write driver in this 
>> patch requires specific sequence to write/blow fuses unlike other 
>> driver. Scope/functionalities are different and this is separate driver.
>>
> 
> This is another variant of qfprom, so please add this support in the 
> existing qfprom driver, you could deal with the differences using 
> specific compatible within the driver.
> 
> Doug already covered most of the comments, consider them as mandatory 
> requirements for upstreaming!
> 
> --srini
> 

Based on the compatible, do i need to separate probe function for 
qfprom-efuse and maintain separate nvmem object to register nvmem 
framework. Is this what you are suggesting to implementing this in to 
one existing driver?
Do I need to maintain separate efuse dt node?

Could you please suggest me to proceed further.

> 
>>>>
>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>> ---
>>>>   drivers/nvmem/Kconfig        |  10 +
>>>>   drivers/nvmem/Makefile       |   2 +
>>>>   drivers/nvmem/qfprom-efuse.c | 476 
>>>> +++++++++++++++++++++++++++++++++++++++++++
>>>>   3 files changed, 488 insertions(+)
>>>>   create mode 100644 drivers/nvmem/qfprom-efuse.c
>>>>
>>> ...
>>>
>>>> diff --git a/drivers/nvmem/qfprom-efuse.c 
>>>> b/drivers/nvmem/qfprom-efuse.c
>>>> new file mode 100644
>>>> index 0000000..2e3c275
>>>> --- /dev/null
>>>> +++ b/drivers/nvmem/qfprom-efuse.c
>>>> @@ -0,0 +1,476 @@
>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>> +/*
>>>> + * Copyright (c) 2020, The Linux Foundation. All rights reserved.
>>>> + */
>>>> +
>>>> +#include <linux/clk.h>
>>>> +#include <linux/device.h>
>>>> +#include <linux/io.h>
>>>> +#include <linux/iopoll.h>
>>>> +#include <linux/kernel.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/mod_devicetable.h>
>>>> +#include <linux/nvmem-provider.h>
>>>> +#include <linux/of_device.h>
>>>> +#include <linux/platform_device.h>
>>>> +#include <linux/regulator/consumer.h>
>>>> +
>>>> +#define QFPROM_BLOW_STATUS_BUSY 0x1
>>>> +#define QFPROM_BLOW_STATUS_READY 0x0
>>>> +
>>>> +/* Blow timer clock frequency in Mhz for 10nm LPe technology */
>>>> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
>>>> +#define QFPROM_BLOW_TIMER_RESET_VALUE 0x0
>>>> +
>>>> +/* Amount of time required to hold charge to blow fuse in 
>>>> micro-seconds */
>>>> +#define QFPROM_FUSE_BLOW_POLL_PERIOD 100
>>>> +#define QFPROM_BLOW_STATUS_OFFSET 0x048
>>>> +
>>>> +#define QFPROM_ACCEL_OFFSET 0x044
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_platform_data - structure holding qfprom-efuse
>>>> + * platform data
>>>> + *
>>>> + * @name: qfprom-efuse compatible name
>>>
>>> ??
>>
>> Thanks for your feedback. I will address this change
>>
>>>> + * @fuse_blow_time_in_us: Should contain the wait time when doing 
>>>> the fuse blow
>>>> + * @accel_value: Should contain qfprom accel value
>>>> + * @accel_reset_value: The reset value of qfprom accel value
>>>> + * @qfprom_blow_timer_value: The timer value of qfprom when doing 
>>>> efuse blow
>>>> + * @qfprom_blow_reset_freq: The frequency required to set when fuse 
>>>> blowing
>>>> + * is done
>>>> + * @qfprom_blow_set_freq: The frequency required to set when we 
>>>> start the
>>>> + * fuse blowing
>>>> + * @qfprom_max_vol: max voltage required to set fuse blow
>>>> + * @qfprom_min_vol: min voltage required to set fuse blow
>>>
>>> How specific are these values per SoC?
>>>
>>
>> This voltage level may change based on SoC and/or fuse-hardware 
>> technology, it would change for SoC with different technology, hence 
>> we have kept it in SOC specific settings.
>>
>>>
>>>> + */
>>>> +struct qfprom_efuse_platform_data {
>>>> +    const char *name;
>>>> +    u8 fuse_blow_time_in_us;
>>>> +    u32 accel_value;
>>>> +    u32 accel_reset_value;
>>>> +    u32 qfprom_blow_timer_value;
>>>> +    u32 qfprom_blow_reset_freq;
>>>> +    u32 qfprom_blow_set_freq;
>>>> +    u32 qfprom_max_vol;
>>>> +    u32 qfprom_min_vol;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct qfprom_efuse_priv - structure holding qfprom-efuse 
>>>> attributes
>>>> + *
>>>> + * @qfpbase: iomapped memory space for qfprom base
>>>> + * @qfpraw: iomapped memory space for qfprom raw fuse region
>>>> + * @qfpmap: iomapped memory space for qfprom fuse blow timer
>>>> +
>>>> + * @dev: qfprom device structure
>>>> + * @secclk: clock supply
>>>> + * @vcc: regulator supply
>>>> +
>>>> + * @qfpraw_start: qfprom raw fuse start region
>>>> + * @qfpraw_end: qfprom raw fuse end region
>>>> + * @qfprom_efuse_platform_data: qfprom platform data
>>>> + */
>>>> +struct qfprom_efuse_priv {
>>>> +    void __iomem *qfpbase;
>>>> +    void __iomem *qfpraw;
>>>> +    void __iomem *qfpmap;
>>>
>>> Why are these memory regions split? Can't you just have complete 
>>> qfprom area and add fixed offset for qfpraw within the driver?
>>>
>>
>> Thanks for your feedback. I will address this change.
>> I have separated this memory regions because to identify raw fuse 
>> regions separately and compare these raw fuse regions from the user 
>> given input.
>>
>>>> +    struct device *dev;
>>>> +    struct clk *secclk;
>>>> +    struct regulator *vcc;
>>>> +    resource_size_t qfpraw_start;
>>>> +    resource_size_t qfpraw_end;
>>> Why do we need to check this range? as long as we set the 
>>> nvmem_config with correct range then you should not need this check.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and 
>> based on internal review with our security team, this check is 
>> important to avoid dependency on other upper layer.
>>
>>
>>>
>>>> +    struct qfprom_efuse_platform_data efuse;
>>> A pointer here should be good enough?
>>>> +};
>>>> +
>>>
>>
>> Thanks for your feedback. I will address this change
>>
>>> ...
>>>
>>>> +/*
>>>> + * sets the value of the blow timer, accel register and the clock
>>>> + * and voltage settings
>>>> + */
>>>> +static int qfprom_enable_fuse_blowing(const struct 
>>>> qfprom_efuse_priv *priv)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ret = qfprom_disable_fuse_blowing(priv);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qfprom_disable_fuse_blowing()\n");
>>>> +        return ret;
>>>> +    }
>>>
>>> Why do we need to qfprom_disable_fuse_blowing() for every call to 
>>> enable it?
>>>
>>> Or are we missing some error handling in the caller?
>>>
>>
>> We must disable/vote-off this QFPROM fuse power rail after blowing 
>> fuse, it is the safe and right approach as per hardware programming 
>> guide for fuse blowing process. Caller here is user space, can’t 
>> control fuse-power-rail or can’t be relied to follow the required 
>> process. There could also be unnecessary risk of leaving the 
>> vote/power-rail configured at specific level after blowing the fuse. 
>> As per hardware requirement, right after fuse blowing, we need to 
>> disable power rail.
>>
>>>> +
>>>> +    writel(priv->efuse.qfprom_blow_timer_value, priv->qfpmap +
>>>> +           QFPROM_BLOW_TIMER_OFFSET);
>>>> +    writel(priv->efuse.accel_value, priv->qfpmap + 
>>>> QFPROM_ACCEL_OFFSET);
>>>> +
>>>> +    ret = qfprom_set_clock_settings(priv);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qpfrom_set_clock_settings()\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    ret = qfprom_set_voltage_settings(priv, 
>>>> priv->efuse.qfprom_min_vol,
>>>> +                      priv->efuse.qfprom_max_vol);
>>>> +    if (ret) {
>>>> +        dev_err(priv->dev, "qfprom_set_voltage_settings()\n");
>>>> +        return ret;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>> +
>>>
>>> <<
>>>> +/*
>>>> + * verifying to make sure address being written or read is from qfprom
>>>> + * raw address range
>>>> + */
>>>> +bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 
>>>> reg,
>>>> +              size_t bytes)
>>>> +{
>>>> +    if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
>>>> +        ((reg + bytes) <= priv->qfpraw_end)) {
>>>> +        return 1;
>>>> +    }
>>>> +
>>>> +    return 0;
>>>> +}
>>>  >>
>>> Above function is totally redundant, nvmem core already has checks 
>>> for this.
>>>
>>
>> There is no harm in this explicit check in QFPROM-fuse driver and 
>> based on internal review with our security team, this check is 
>> important to avoid dependency on other upper layer.
>>
>>>
>>>
>>>> +
>>>> +/*
>>>> + * API for reading from raw qfprom region
>>>> + */
>>>> +static int qfprom_efuse_reg_read(void *context, unsigned int reg, 
>>>> void *_val,
>>>> +                 size_t bytes)
>>>> +{
>>>> +    struct qfprom_efuse_priv *priv = context;
>>>> +    u32 *value = _val;
>>>> +    u32 align_check;
>>>> +    int i = 0, words = bytes / 4;
>>>> +
>>>> +    dev_info(priv->dev,
>>>> +         "reading raw qfprom region offset: 0x%08x of size: %zd\n",
>>>> +         reg, bytes);
>>>
>>> In general there is lot of debug info across the code, do you really 
>>> need all this? Consider removing these!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +    if (bytes % 4 != 0x00) {
>>>> +        dev_err(priv->dev,
>>>> +            "Bytes: %zd to read should be word align\n",
>>>> +            bytes);
>>>> +        return -EINVAL;
>>>> +    }
>>>
>>> This word align check is also redundant once you set nvmem_config 
>>> with correct word_size.
>>>
>>
>> I understand that there may be different approach to handle this. We 
>> have used this approach and tested this driver thoroughly. Unless 
>> there is technical limitation, changing this word_size would end up 
>> requiring re-writing write/read APIs and going through testing again, 
>> there is not much difference in either approach, we would like to keep 
>> this approach unless there is technical concern.
>>
>>>
>>>> +
>>>> +    if (!addr_in_qfprom_range(priv, reg, bytes)) {
>>>> +        dev_err(priv->dev,
>>>> +            "Invalid qfprom raw region offset 0x%08x & bytes %zd\n",
>>>> +            reg, bytes);
>>>> +        return -EINVAL;
>>>> +    }
>>>> +
>>>> +    align_check = (reg & 0xF);
>>>> +
>>>> +    if (((align_check & ~3) == align_check) && value != NULL)
>>>> +        while (words--)
>>>> +            *value++ = readl(priv->qfpbase + reg + (i++ * 4));
>>>> +
>>>> +    else
>>>> +        dev_err(priv->dev,
>>>> +            "Invalid input parameter 0x%08x fuse blow address\n",
>>>> +            reg);
>>>> +
>>>> +    return 0;
>>>> +}
>>> ...
>>>
>>>> +
>>>> +static int qfprom_efuse_probe(struct platform_device *pdev)
>>>> +{
>>>> +    struct device *dev = &pdev->dev;
>>>> +    struct resource *qfpbase, *qfpraw, *qfpmap;
>>>> +    struct nvmem_device *nvmem;
>>>> +    struct nvmem_config *econfig;
>>>> +    struct qfprom_efuse_priv *priv;
>>>> +    const struct qfprom_efuse_platform_data *drvdata;
>>>> +    int ret;
>>>> +
>>>> +    dev_info(&pdev->dev, "[%s]: Invoked\n", __func__);
>>>> +
>>>
>>> too much debug!
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +    drvdata = of_device_get_match_data(&pdev->dev);
>>>> +    if (!drvdata)
>>>> +        return -EINVAL;
>>> Unnecessary check as this driver will not be probed unless there is a 
>>> compatible match.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>
>>>> +
>>>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>>> +    if (!priv)
>>>> +        return -ENOMEM;
>>>> +
>>>> +    priv->efuse.fuse_blow_time_in_us = drvdata->fuse_blow_time_in_us;
>>>> +    priv->efuse.accel_value = drvdata->accel_value;
>>>> +    priv->efuse.accel_reset_value = drvdata->accel_reset_value;
>>>> +    priv->efuse.qfprom_blow_timer_value = 
>>>> drvdata->qfprom_blow_timer_value;
>>>> +    priv->efuse.qfprom_blow_reset_freq = 
>>>> drvdata->qfprom_blow_reset_freq;
>>>> +    priv->efuse.qfprom_blow_set_freq = drvdata->qfprom_blow_set_freq;
>>>> +    priv->efuse.qfprom_max_vol = drvdata->qfprom_max_vol;
>>>> +    priv->efuse.qfprom_min_vol = drvdata->qfprom_min_vol;
>>>> +    priv->dev = dev;
>>>> +
>>>> +    qfpbase = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>>> +
>>>> +    priv->qfpbase = devm_ioremap_resource(dev, qfpbase);
>>>> +    if (IS_ERR(priv->qfpbase)) {
>>>> +        ret = PTR_ERR(priv->qfpbase);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    qfpraw = platform_get_resource(pdev, IORESOURCE_MEM, 1);
>>>> +
>>>> +    priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
>>>> +    if (IS_ERR(priv->qfpraw)) {
>>>> +        ret = PTR_ERR(priv->qfpraw);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->qfpraw_start = qfpraw->start - qfpbase->start;
>>>> +    priv->qfpraw_end = qfpraw->end - qfpbase->start;
>>>> +
>>>> +    qfpmap = platform_get_resource(pdev, IORESOURCE_MEM, 2);
>>>> +
>>>> +    priv->qfpmap = devm_ioremap_resource(dev, qfpmap);
>>>> +    if (IS_ERR(priv->qfpmap)) {
>>>> +        ret = PTR_ERR(priv->qfpmap);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
>>>
>>> I see no reference to this regulator in dt bindings.
>>
>> This perameter kept in board specific file i.e., sc7180-idp.dts file
>>
>>>> +    if (IS_ERR(priv->vcc)) {
>>>> +        ret = PTR_ERR(priv->vcc);
>>>> +        if (ret == -ENODEV)
>>>> +            ret = -EPROBE_DEFER;
>>> Can you explain what is going on here?
>>>
>>
>> As i took other drivers reference, i have kept this check.
>>
>>>> +
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    priv->secclk = devm_clk_get(dev, "secclk");
>>>> +    if (IS_ERR(priv->secclk)) {
>>>> +        ret = PTR_ERR(priv->secclk);
>>>> +        if (ret != -EPROBE_DEFER)
>>>> +            dev_err(dev, "secclk error getting : %d\n", ret);
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    ret = clk_prepare_enable(priv->secclk);
>>>> +    if (ret) {
>>>> +        dev_err(dev, "clk_prepare_enable() failed\n");
>>>> +        goto err;
>>>> +    }
>>>> +
>>>> +    econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
>>>> +    if (!econfig)
>>> Why not disabling the clk here?
>>>> +        return -ENOMEM;
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +    econfig->dev = dev;
>>>> +    econfig->name = "qfprom-efuse";
>>>> +    econfig->stride = 1;
>>>> +    econfig->word_size = 1;
>>>> +    econfig->reg_read = qfprom_efuse_reg_read;
>>>> +    econfig->reg_write = qfprom_efuse_reg_write;
>>>> +    econfig->size = resource_size(qfpraw);
>>>> +    econfig->priv = priv;
>>>> +
>>>> +    nvmem = devm_nvmem_register(dev, econfig);
>>>> +
>>>> +    return PTR_ERR_OR_ZERO(nvmem);
>>> probably you should check the nvmem here before returning to disable 
>>> the clk properly.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +
>>>> +err:
>>>> +    clk_disable_unprepare(priv->secclk);
>>>> +    return ret;
>>>> +}
>>>> +
>>>> +static const struct qfprom_efuse_platform_data 
>>>> sc7180_qfp_efuse_data = {
>>>> +    .name = "sc7180-qfprom-efuse",
>>> Redundant.
>>>
>>
>> Thanks for your feedback. I will address this change.
>>
>>>> +    .fuse_blow_time_in_us = 10,
>>>> +    .accel_value = 0xD10,
>>>> +    .accel_reset_value = 0x800,
>>>> +    .qfprom_blow_timer_value = 25,
>>>> +    .qfprom_blow_reset_freq = 19200000,
>>>> +    .qfprom_blow_set_freq = 4800000,
>>>> +    .qfprom_max_vol = 1904000,
>>>> +    .qfprom_min_vol = 1800000,
>>>> +};
>>>> +
>>>> +static const struct of_device_id qfprom_efuse_of_match[] = {
>>>> +    {
>>>> +        .compatible = "qcom,sc7180-qfprom-efuse",
>>>> +        .data = &sc7180_qfp_efuse_data
>>>> +    },
>>>> +    {/* sentinel */},
>>>> +};
>>>> +
>>>> +MODULE_DEVICE_TABLE(of, qfprom_efuse_of_match);
>>>> +
>>>> +static struct platform_driver qfprom_efuse_driver = {
>>>> +    .probe = qfprom_efuse_probe,
>>>> +    .driver = {
>>>> +        .name = "sc7180-qfprom-efuse",
>>>> +        .of_match_table = qfprom_efuse_of_match,
>>>> +    },
>>>> +};
>>>> +
>>>> +module_platform_driver(qfprom_efuse_driver);
>>>> +MODULE_DESCRIPTION("QTI QFPROM Efuse driver");
>>>> +MODULE_LICENSE("GPL v2");
>>>>
>>


Regards,
Ravi Kumar.B

-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a 
member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 18/05/2020 11:39, Ravi Kumar Bokka (Temp) wrote:
>>
> 
> Based on the compatible, do i need to separate probe function for 
> qfprom-efuse and maintain separate nvmem object to register nvmem 
> framework. Is this what you are suggesting to implementing this in to 
> one existing driver?

Yes for same driver we should add new compatible string and add support 
to this in existing qfprom driver.
Ideally we should allocate nvmem_config object at probe with different 
parameters based on compatible string.

> Do I need to maintain separate efuse dt node?

Not sure what you mean this w.r.t driver, but based on compatible string 
the device tree bindings might vary like clocks, regulators and so on.

--srini
> 
> Could you please suggest me to proceed further.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

On Mon, May 18, 2020 at 3:45 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> On 18/05/2020 11:39, Ravi Kumar Bokka (Temp) wrote:
> >
> > Based on the compatible, do i need to separate probe function for
> > qfprom-efuse and maintain separate nvmem object to register nvmem
> > framework. Is this what you are suggesting to implementing this in to
> > one existing driver?
>
> Yes for same driver we should add new compatible string and add support
> to this in existing qfprom driver.
> Ideally we should allocate nvmem_config object at probe with different
> parameters based on compatible string.

I wish I had better documentation for exactly what was in the SoC
instead of the heavily redacted stuff Qualcomm provides.  Really the
answer here is: how do you best describe the hardware?  OK, so I just
spent the past hour or so trying to patch together all the bits and
fragments that Qualcomm provided me.  Just like a scavenger hunt!
Fun!  The best I can patch together is that there is a single QFPROM
with these ranges:

0x00780000 - 0x007800FF
QFPROM HW registers, range 1/2

0x00780120 - 0x007808FF
QFPROM "raw" space

0x00782000 - 0x007820FF
QFPROM HW registers, range 2/2

0x00784120 - 0x007848FF
QFPROM "corrected" space

0x00786000 - 0x00786FFF
QFPROM memory range that I don't really understand and maybe we don't
worry about right now?

Did I get that right?  If so, is there a prize for winning the scavenger hunt?

---

If so then, IMO, it wouldn't be insane to actually keep it as two
drivers and two device tree nodes, as you've done.  I'd defer to
Srinivas and Rob Herring, though.  The existing driver would be a
read-only driver and provide access to the "corrected" versions of all
the registers.  Its node would have "#address-cells = <1>" and
"#size-cells = <1>" because it's expected that other drivers might
need to refer to data stored here.

Your new driver would be read-write and provide access to the "raw"
values.  A read from your new driver would not necessarily equal a
read from the old driver if the FEC (forward error correction) kicked
in.  Other drivers should never refer to the non-corrected values so
you wouldn't have "#address-cells" and "#size-cells".  The only way to
really read or write it would be through sysfs.

It would be super important to document what's happening, of course.
...and ideally name them to make it clearer too.

---

Another alternative (if Srinivas and/or Rob H prefer it) would be to
deprecate the old driver and/or bindings and say that there really
should just be one node and one driver.  In that case you'd replace
the old node with:

qfprom@780000 {
  compatible = "qcom,sc7180-qfprom-efuse";
  reg = <0 0x00780000 0 0x6fff>;
  #address-cells = <1>;
  #size-cells = <1>;

  clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
  clock-names = "sec";

  qusb2p_hstx_trim: hstx-trim-primary@25b {
    reg = <0x25b 0x1>;
    bits = <1 3>;
  };
};

You'd use the of_match_table solution to figure out the relevant
offsets (0x120, 0x2000, 0x4120, 0x6000) for sc7180 and this new driver
would be responsible for being able to read the corrected values and
also for programming.  In this case I'm not sure how (assuming it's
valuable) you'd provide read access to the uncorrected data.


-Doug

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 18/05/2020 19:31, Doug Anderson wrote:
> Hi,
> 
> On Mon, May 18, 2020 at 3:45 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> On 18/05/2020 11:39, Ravi Kumar Bokka (Temp) wrote:
>>>
>>> Based on the compatible, do i need to separate probe function for
>>> qfprom-efuse and maintain separate nvmem object to register nvmem
>>> framework. Is this what you are suggesting to implementing this in to
>>> one existing driver?
>>
>> Yes for same driver we should add new compatible string and add support
>> to this in existing qfprom driver.
>> Ideally we should allocate nvmem_config object at probe with different
>> parameters based on compatible string.
> 
> I wish I had better documentation for exactly what was in the SoC
> instead of the heavily redacted stuff Qualcomm provides.  Really the
> answer here is: how do you best describe the hardware?  OK, so I just
> spent the past hour or so trying to patch together all the bits and
> fragments that Qualcomm provided me.  Just like a scavenger hunt!
> Fun!  The best I can patch together is that there is a single QFPROM
> with these ranges:
> 
> 0x00780000 - 0x007800FF
> QFPROM HW registers, range 1/2
> 
> 0x00780120 - 0x007808FF
> QFPROM "raw" space
> 

so this is the only region is the QFPROM fuses can be programmed!

> 0x00782000 - 0x007820FF
> QFPROM HW registers, range 2/2
> 
> 0x00784120 - 0x007848FF
> QFPROM "corrected" space

Is this some kind of FEC corrected regions?


> 
> 0x00786000 - 0x00786FFF
> QFPROM memory range that I don't really understand and maybe we don't
> worry about right now?

> 
> Did I get that right?  If so, is there a prize for winning the scavenger hunt?
> 
> ---
> 
> If so then, IMO, it wouldn't be insane to actually keep it as two
> drivers and two device tree nodes, as you've done.  I'd defer to
> Srinivas and Rob Herring, though.  The existing driver would be a
> read-only driver and provide access to the "corrected" versions of all
> the registers.  Its node would have "#address-cells = <1>" and
> "#size-cells = <1>" because it's expected that other drivers might
> need to refer to data stored here.
> 
> Your new driver would be read-write and provide access to the "raw"
> values.  A read from your new driver would not necessarily equal a
> read from the old driver if the FEC (forward error correction) kicked

Is this only applicable for corrected address space?

> in.  Other drivers should never refer to the non-corrected values so
> you wouldn't have "#address-cells" and "#size-cells".  The only way to
> really read or write it would be through sysfs.
> 
> It would be super important to document what's happening, of course.
> ...and ideally name them to make it clearer too.
> 
> ---
> 
> Another alternative (if Srinivas and/or Rob H prefer it) would be to
> deprecate the old driver and/or bindings and say that there really
> should just be one node and one driver.  In that case you'd replace
> the old node with:
> 
> qfprom@780000 {
>    compatible = "qcom,sc7180-qfprom-efuse";

May be "qcom,sc7180-qfprom"


>    reg = <0 0x00780000 0 0x6fff>;
>    #address-cells = <1>;
>    #size-cells = <1>;
> 
>    clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>    clock-names = "sec";
> 
>    qusb2p_hstx_trim: hstx-trim-primary@25b {
>      reg = <0x25b 0x1>;
>      bits = <1 3>;
>    };
> };
> 
> You'd use the of_match_table solution to figure out the relevant
> offsets (0x120, 0x2000, 0x4120, 0x6000) for sc7180 and this new driver
> would be responsible for being able to read the corrected values and


Encompassing these offsets in driver as part of the register defines 
itself should be a good start!

It will also be nice to understand how similar this thing is with w.rt 
other Qcom SoCs?


> also for programming.  In this case I'm not sure how (assuming it's
> valuable) you'd provide read access to the uncorrected data.
I will leave this question to the author of the driver.

--srini

> 
> 
> -Doug
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

On Wed, May 20, 2020 at 7:35 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> On 18/05/2020 19:31, Doug Anderson wrote:
> > Hi,
> >
> > On Mon, May 18, 2020 at 3:45 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >> On 18/05/2020 11:39, Ravi Kumar Bokka (Temp) wrote:
> >>>
> >>> Based on the compatible, do i need to separate probe function for
> >>> qfprom-efuse and maintain separate nvmem object to register nvmem
> >>> framework. Is this what you are suggesting to implementing this in to
> >>> one existing driver?
> >>
> >> Yes for same driver we should add new compatible string and add support
> >> to this in existing qfprom driver.
> >> Ideally we should allocate nvmem_config object at probe with different
> >> parameters based on compatible string.
> >
> > I wish I had better documentation for exactly what was in the SoC
> > instead of the heavily redacted stuff Qualcomm provides.  Really the
> > answer here is: how do you best describe the hardware?  OK, so I just
> > spent the past hour or so trying to patch together all the bits and
> > fragments that Qualcomm provided me.  Just like a scavenger hunt!
> > Fun!  The best I can patch together is that there is a single QFPROM
> > with these ranges:
> >
> > 0x00780000 - 0x007800FF
> > QFPROM HW registers, range 1/2
> >
> > 0x00780120 - 0x007808FF
> > QFPROM "raw" space
> >
>
> so this is the only region is the QFPROM fuses can be programmed!
>
> > 0x00782000 - 0x007820FF
> > QFPROM HW registers, range 2/2
> >
> > 0x00784120 - 0x007848FF
> > QFPROM "corrected" space
>
> Is this some kind of FEC corrected regions?

Yes.


> > 0x00786000 - 0x00786FFF
> > QFPROM memory range that I don't really understand and maybe we don't
> > worry about right now?
>
> >
> > Did I get that right?  If so, is there a prize for winning the scavenger hunt?
> >
> > ---
> >
> > If so then, IMO, it wouldn't be insane to actually keep it as two
> > drivers and two device tree nodes, as you've done.  I'd defer to
> > Srinivas and Rob Herring, though.  The existing driver would be a
> > read-only driver and provide access to the "corrected" versions of all
> > the registers.  Its node would have "#address-cells = <1>" and
> > "#size-cells = <1>" because it's expected that other drivers might
> > need to refer to data stored here.
> >
> > Your new driver would be read-write and provide access to the "raw"
> > values.  A read from your new driver would not necessarily equal a
> > read from the old driver if the FEC (forward error correction) kicked
>
> Is this only applicable for corrected address space?

I guess I was proposing a two dts-node / two drive approach here.

dts node #1:just covers the memory range for accessing the FEC-corrected data
driver #1: read-only and reads the FEC-corrected data

dts node #2: covers the memory range that's _not_ the FEC-corrected
memory range.
driver #2: read-write.  reading reads uncorrected data

Does that seem sane?


> > in.  Other drivers should never refer to the non-corrected values so
> > you wouldn't have "#address-cells" and "#size-cells".  The only way to
> > really read or write it would be through sysfs.
> >
> > It would be super important to document what's happening, of course.
> > ...and ideally name them to make it clearer too.
> >
> > ---
> >
> > Another alternative (if Srinivas and/or Rob H prefer it) would be to
> > deprecate the old driver and/or bindings and say that there really
> > should just be one node and one driver.  In that case you'd replace
> > the old node with:
> >
> > qfprom@780000 {
> >    compatible = "qcom,sc7180-qfprom-efuse";
>
> May be "qcom,sc7180-qfprom"
>
>
> >    reg = <0 0x00780000 0 0x6fff>;
> >    #address-cells = <1>;
> >    #size-cells = <1>;
> >
> >    clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> >    clock-names = "sec";
> >
> >    qusb2p_hstx_trim: hstx-trim-primary@25b {
> >      reg = <0x25b 0x1>;
> >      bits = <1 3>;
> >    };
> > };
> >
> > You'd use the of_match_table solution to figure out the relevant
> > offsets (0x120, 0x2000, 0x4120, 0x6000) for sc7180 and this new driver
> > would be responsible for being able to read the corrected values and
>
>
> Encompassing these offsets in driver as part of the register defines
> itself should be a good start!
>
> It will also be nice to understand how similar this thing is with w.rt
> other Qcom SoCs?

At least sdm845 is about the same.  I cross-referenced docs I had with
sc7180 and sdm845 and that's how I came up with my model for how this
works.


> > also for programming.  In this case I'm not sure how (assuming it's
> > valuable) you'd provide read access to the uncorrected data.
> I will leave this question to the author of the driver.
>
> --srini
>
> >
> >
> > -Doug
> >

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 20/05/2020 23:48, Doug Anderson wrote:
>> Is this only applicable for corrected address space?
> I guess I was proposing a two dts-node / two drive approach here.
> 
> dts node #1:just covers the memory range for accessing the FEC-corrected data
> driver #1: read-only and reads the FEC-corrected data
> 
> dts node #2: covers the memory range that's_not_  the FEC-corrected
> memory range.
> driver #2: read-write.  reading reads uncorrected data
> 
> Does that seem sane?

I see your point but it does not make sense to have two node for same thing.

Isn't the raw address space reads used to for blowing and checking the 
fuses if they are blown correctly or not and software usage of these 
fuses should only be done from correct address space?

the read interface to user should be always from corrected address space
and write interface should be to raw address space.


--srini

> 
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

On Thu, May 21, 2020 at 8:01 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> On 20/05/2020 23:48, Doug Anderson wrote:
> >> Is this only applicable for corrected address space?
> > I guess I was proposing a two dts-node / two drive approach here.
> >
> > dts node #1:just covers the memory range for accessing the FEC-corrected data
> > driver #1: read-only and reads the FEC-corrected data
> >
> > dts node #2: covers the memory range that's_not_  the FEC-corrected
> > memory range.
> > driver #2: read-write.  reading reads uncorrected data
> >
> > Does that seem sane?
>
> I see your point but it does not make sense to have two node for same thing.

OK, so that sounds as if we want to go with the proposal where we
"deprecate the old driver and/or bindings and say that there really
should just be one node and one driver".

Would this be acceptable to you?

1. Officially mark the old bindings as deprecated.

2. Leave the old driver there to support the old deprecated bindings,
at least until everyone can be transferred over.  There seem to be
quite a few existing users of "qcom,qfprom" and we're supposed to make
an attempt at keeping the old device trees working, at least for a
little while.  Once everyone is transferred over we could decide to
delete the old driver.

3. We will have a totally new driver here.

4. A given device tree will _not_ be allowed to have both
"qcom,qfprom" specified and "qcom,SOC-qfprom" specified.  ...and by
"qcom,SOC-qfprom" I mean that SOC should be replaced by the SoC name,
so "qcom,sc7180-qfprom" or "qcom,sdm845-qfprom".  So once you switch
to the new node it replaces the old node.


> Isn't the raw address space reads used to for blowing and checking the
> fuses if they are blown correctly or not and software usage of these
> fuses should only be done from correct address space?
>
> the read interface to user should be always from corrected address space
> and write interface should be to raw address space.

Great.  That sounds right to me.  Presumably the driver could add some
sort of "debugfs" access to read the raw address space if needed.

-Doug

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 21/05/2020 16:10, Doug Anderson wrote:
>> On 20/05/2020 23:48, Doug Anderson wrote:
>>>> Is this only applicable for corrected address space?
>>> I guess I was proposing a two dts-node / two drive approach here.
>>>
>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
>>> driver #1: read-only and reads the FEC-corrected data
>>>
>>> dts node #2: covers the memory range that's_not_  the FEC-corrected
>>> memory range.
>>> driver #2: read-write.  reading reads uncorrected data
>>>
>>> Does that seem sane?
>> I see your point but it does not make sense to have two node for same thing.
> OK, so that sounds as if we want to go with the proposal where we
> "deprecate the old driver and/or bindings and say that there really
> should just be one node and one driver".
> 
> Would this be acceptable to you?
> 
> 1. Officially mark the old bindings as deprecated.

Possibly Yes for some reasons below!

> 
> 2. Leave the old driver there to support the old deprecated bindings,
> at least until everyone can be transferred over.  There seem to be
> quite a few existing users of "qcom,qfprom" and we're supposed to make
> an attempt at keeping the old device trees working, at least for a
> little while.  Once everyone is transferred over we could decide to
> delete the old driver.
we could consider "qcom,qfrom" to be only passing corrected address 
space. Till we transition users to new bindings!

> 
Yes.

> 3. We will have a totally new driver here.
No, we should still be using the same driver. But the exiting driver 
seems to incorrect and is need of fixing.

Having a look at the memory map for old SoCs like msm8996 and msm8916 
shows that memory map that was passed to qfprom driver is corrected 
address space. Writes will not obviously work!

This should also be true with sdm845 or sc7180

That needs to be fixed first!

> 
> 4. A given device tree will_not_  be allowed to have both
> "qcom,qfprom" specified and "qcom,SOC-qfprom" specified.  ...and by
> "qcom,SOC-qfprom" I mean that SOC should be replaced by the SoC name,
> so "qcom,sc7180-qfprom" or "qcom,sdm845-qfprom".  So once you switch
> to the new node it replaces the old node.

Secondly, this IP is clearly an integral part of Secure Control Block, 
which clearly has versioning information.

Versioning information should be part of compatible string in msm8996 it 
should be "qcom,qfprom-5.1.0"
for msm8916 it should be "qcom,qfprom-4.0.0" this translates to 
"qcom,qfprom-<MAJOR-NUMBER>-<MINOR-NUMBER>-<STEP>"

Thirdly we should be able to have common read for all these as they tend 
to just read from corrected address space.

Offsets to corrected address space seems to always constant across SoCs too.

platform specific device tree nodes should also be able to specify 
"read-only" property to not allow writes on to this raw area.

Does this make sense ?

Thanks,
srini
> 
> 
>> Isn't the raw address space reads used to for blowing and checking the
>> fuses if they are blown correctly or not and software usage of these
>> fuses should only be done from correct address space?
>>
>> the read interface to user should be always from corrected address space
>> and write interface should be to raw address space.
> Great.  That sounds right to me.  Presumably the driver could add some
> sort of "debugfs" access to read the raw address space if needed.

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> On 21/05/2020 16:10, Doug Anderson wrote:
> >> On 20/05/2020 23:48, Doug Anderson wrote:
> >>>> Is this only applicable for corrected address space?
> >>> I guess I was proposing a two dts-node / two drive approach here.
> >>>
> >>> dts node #1:just covers the memory range for accessing the FEC-corrected data
> >>> driver #1: read-only and reads the FEC-corrected data
> >>>
> >>> dts node #2: covers the memory range that's_not_  the FEC-corrected
> >>> memory range.
> >>> driver #2: read-write.  reading reads uncorrected data
> >>>
> >>> Does that seem sane?
> >> I see your point but it does not make sense to have two node for same thing.
> > OK, so that sounds as if we want to go with the proposal where we
> > "deprecate the old driver and/or bindings and say that there really
> > should just be one node and one driver".
> >
> > Would this be acceptable to you?
> >
> > 1. Officially mark the old bindings as deprecated.
>
> Possibly Yes for some reasons below!
>
> >
> > 2. Leave the old driver there to support the old deprecated bindings,
> > at least until everyone can be transferred over.  There seem to be
> > quite a few existing users of "qcom,qfprom" and we're supposed to make
> > an attempt at keeping the old device trees working, at least for a
> > little while.  Once everyone is transferred over we could decide to
> > delete the old driver.
> we could consider "qcom,qfrom" to be only passing corrected address
> space. Till we transition users to new bindings!
>
> >
> Yes.
>
> > 3. We will have a totally new driver here.
> No, we should still be using the same driver. But the exiting driver
> seems to incorrect and is need of fixing.
>
> Having a look at the memory map for old SoCs like msm8996 and msm8916
> shows that memory map that was passed to qfprom driver is corrected
> address space. Writes will not obviously work!
>
> This should also be true with sdm845 or sc7180
>
> That needs to be fixed first!

OK, so to summarize:

1. We will have one driver: "drivers/nvmem/qfprom.c"

2. If the driver detects that its reg is pointing to the corrected
address space then it should operate in read-only mode.  Maybe it can
do this based on the compatible string being just "qcom,qfprom" or
maybe it can do this based on the size of the "reg".

3. If that driver sees a newer compatible string (one that includes
the SoC name in it) it will assume that its "reg" points to the start
of qfprom space.

4. We should post patches to transition all old dts files away from
the deprecated bindings.


> > 4. A given device tree will_not_  be allowed to have both
> > "qcom,qfprom" specified and "qcom,SOC-qfprom" specified.  ...and by
> > "qcom,SOC-qfprom" I mean that SOC should be replaced by the SoC name,
> > so "qcom,sc7180-qfprom" or "qcom,sdm845-qfprom".  So once you switch
> > to the new node it replaces the old node.
>
> Secondly, this IP is clearly an integral part of Secure Control Block,
> which clearly has versioning information.
>
> Versioning information should be part of compatible string in msm8996 it
> should be "qcom,qfprom-5.1.0"
> for msm8916 it should be "qcom,qfprom-4.0.0" this translates to
> "qcom,qfprom-<MAJOR-NUMBER>-<MINOR-NUMBER>-<STEP>"

I don't know much about this versioning info, but I'm curious: can we
read it from the chip?  If so then it actually _doesn't_ need to be in
the compatible string, I think.  Device tree shouldn't include things
that can be probed.  So if this can be probed then maybe we could have
the compatible as:

compatible = "qcom,msm8996-qfprom", "qcom,qfprom"

...where the SoC is there just in case we need it but we'd expect to
just match on "qcom,qfprom" and then probe.


If this can't be probed then having the version info is nice, so then
I guess you'd have the compatible string:

compatible = "qcom,msm8996-qfprom", "qcom,qfprom-5.1.0"

...where (again) you'd have the SoC specific string there just in case
but you'd expect that you could just use the generic string.


Does that sound right?


> Thirdly we should be able to have common read for all these as they tend
> to just read from corrected address space.
>
> Offsets to corrected address space seems to always constant across SoCs too.
>
> platform specific device tree nodes should also be able to specify
> "read-only" property to not allow writes on to this raw area.

Yeah, I was thinking we probably wanted a read-only property.  That
sounds sane to me.


-Doug

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 21/05/2020 22:28, Doug Anderson wrote:
> Hi,
> 
> On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> On 21/05/2020 16:10, Doug Anderson wrote:
>>>> On 20/05/2020 23:48, Doug Anderson wrote:
>>>>>> Is this only applicable for corrected address space?
>>>>> I guess I was proposing a two dts-node / two drive approach here.
>>>>>
>>>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
>>>>> driver #1: read-only and reads the FEC-corrected data
>>>>>
>>>>> dts node #2: covers the memory range that's_not_  the FEC-corrected
>>>>> memory range.
>>>>> driver #2: read-write.  reading reads uncorrected data
>>>>>
>>>>> Does that seem sane?
>>>> I see your point but it does not make sense to have two node for same thing.
>>> OK, so that sounds as if we want to go with the proposal where we
>>> "deprecate the old driver and/or bindings and say that there really
>>> should just be one node and one driver".
>>>
>>> Would this be acceptable to you?
>>>
>>> 1. Officially mark the old bindings as deprecated.
>>
>> Possibly Yes for some reasons below!
>>
>>>
>>> 2. Leave the old driver there to support the old deprecated bindings,
>>> at least until everyone can be transferred over.  There seem to be
>>> quite a few existing users of "qcom,qfprom" and we're supposed to make
>>> an attempt at keeping the old device trees working, at least for a
>>> little while.  Once everyone is transferred over we could decide to
>>> delete the old driver.
>> we could consider "qcom,qfrom" to be only passing corrected address
>> space. Till we transition users to new bindings!
>>
>>>
>> Yes.
>>
>>> 3. We will have a totally new driver here.
>> No, we should still be using the same driver. But the exiting driver
>> seems to incorrect and is need of fixing.
>>
>> Having a look at the memory map for old SoCs like msm8996 and msm8916
>> shows that memory map that was passed to qfprom driver is corrected
>> address space. Writes will not obviously work!
>>
>> This should also be true with sdm845 or sc7180
>>
>> That needs to be fixed first!
> 
> OK, so to summarize:
> 

> 1. We will have one driver: "drivers/nvmem/qfprom.c"

Yes, we should one driver for this because we are dealing with exactly 
same IP.

> 
> 2. If the driver detects that its reg is pointing to the corrected
> address space then it should operate in read-only mode.  Maybe it can
> do this based on the compatible string being just "qcom,qfprom" or
> maybe it can do this based on the size of the "reg".

I found out that there is a version register at offset of 0x6000 which 
can give MAJOR, MINOR and STEP numbers.

So we could still potentially continue using "qcom,qfprom"

The address space can be split into 3 resources, which is inline with 
Specs as well

1. Raw address space ("raw")
2. Configuration address space ("conf" or "core")
3. Corrected address space ("corrected")

Exiting qfprom entries or read-only qfprom  will have "corrected" 
address space which can be the only resource provided by device tree 
entries.
Other two entries("raw" and "conf") are optional.

qfprom: qfprom@780000 {
         compatible = "qcom,qfprom";
	reg = <0 0x00780000 0 0x8ff>,
		<0 0x00782000 0 0x100>,
		<0 0x00784000 0 0x8ff>;
	reg-names = "raw", "conf", "corrected";

	vcc-supply = <&vreg_xyz>;

	clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
	clock-names = "secclk";

	assigned-clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
         assigned-clock-rates = <19200000>;

	qcom,fuse-blow-frequency = <4800000>

         #address-cells = <1>;
         #size-cells = <1>;

	qusb2p_hstx_trim: hstx-trim-primary@25b {
		reg = <0x25b 0x1>;
		bits = <1 3>;
	};
};

Regarding clk rate setting, the default rate can be set using 
assigned-clock-rates property, however the blow frequency can go as new 
binding.
regarding voltage range for regulator, it should come as part of board 
specific voltage regulator node. In worst case we can discuss on adding 
new bindings for allowing specific range.

for Older SoCs: we still continue to use old style with just one 
resource corresponding to corrected by default.

qfprom: qfprom@784000 {
         compatible = "qcom,qfprom";
         reg = <0 0x00784000 0 0x8ff>;
         #address-cells = <1>;
         #size-cells = <1>;

         qusb2p_hstx_trim: hstx-trim-primary@1eb {
                 reg = <0x1eb 0x1>;
                 bits = <1 4>;
         };

         qusb2s_hstx_trim: hstx-trim-secondary@1eb {
                 reg = <0x1eb 0x2>;
                 bits = <6 4>;
         };
};


I see the patch as adding write support to qfprom, rather than adding 
new driver or new SoC support.

This in summary should give us good direction for this patch!

Correct me if I miss understood something here!


Thanks,
srini

> 
> 3. If that driver sees a newer compatible string (one that includes
> the SoC name in it) it will assume that its "reg" points to the start
> of qfprom space.
> 
> 4. We should post patches to transition all old dts files away from
> the deprecated bindings.
> 
> 
>>> 4. A given device tree will_not_  be allowed to have both
>>> "qcom,qfprom" specified and "qcom,SOC-qfprom" specified.  ...and by
>>> "qcom,SOC-qfprom" I mean that SOC should be replaced by the SoC name,
>>> so "qcom,sc7180-qfprom" or "qcom,sdm845-qfprom".  So once you switch
>>> to the new node it replaces the old node.
>>
>> Secondly, this IP is clearly an integral part of Secure Control Block,
>> which clearly has versioning information.
>>
>> Versioning information should be part of compatible string in msm8996 it
>> should be "qcom,qfprom-5.1.0"
>> for msm8916 it should be "qcom,qfprom-4.0.0" this translates to
>> "qcom,qfprom-<MAJOR-NUMBER>-<MINOR-NUMBER>-<STEP>"
> 
> I don't know much about this versioning info, but I'm curious: can we
> read it from the chip?  If so then it actually _doesn't_ need to be in
> the compatible string, I think.  Device tree shouldn't include things
> that can be probed.  So if this can be probed then maybe we could have
> the compatible as:
> 
> compatible = "qcom,msm8996-qfprom", "qcom,qfprom"
> 
> ...where the SoC is there just in case we need it but we'd expect to
> just match on "qcom,qfprom" and then probe.
> 
> 
> If this can't be probed then having the version info is nice, so then
> I guess you'd have the compatible string:
> 
> compatible = "qcom,msm8996-qfprom", "qcom,qfprom-5.1.0"
> 
> ...where (again) you'd have the SoC specific string there just in case
> but you'd expect that you could just use the generic string.
> 
> 
> Does that sound right?
> 
> 
>> Thirdly we should be able to have common read for all these as they tend
>> to just read from corrected address space.
>>
>> Offsets to corrected address space seems to always constant across SoCs too.
>>
>> platform specific device tree nodes should also be able to specify
>> "read-only" property to not allow writes on to this raw area.
> 
> Yeah, I was thinking we probably wanted a read-only property.  That
> sounds sane to me.
> 
> 
> -Doug
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

On Fri, May 22, 2020 at 4:18 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> On 21/05/2020 22:28, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >> On 21/05/2020 16:10, Doug Anderson wrote:
> >>>> On 20/05/2020 23:48, Doug Anderson wrote:
> >>>>>> Is this only applicable for corrected address space?
> >>>>> I guess I was proposing a two dts-node / two drive approach here.
> >>>>>
> >>>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
> >>>>> driver #1: read-only and reads the FEC-corrected data
> >>>>>
> >>>>> dts node #2: covers the memory range that's_not_  the FEC-corrected
> >>>>> memory range.
> >>>>> driver #2: read-write.  reading reads uncorrected data
> >>>>>
> >>>>> Does that seem sane?
> >>>> I see your point but it does not make sense to have two node for same thing.
> >>> OK, so that sounds as if we want to go with the proposal where we
> >>> "deprecate the old driver and/or bindings and say that there really
> >>> should just be one node and one driver".
> >>>
> >>> Would this be acceptable to you?
> >>>
> >>> 1. Officially mark the old bindings as deprecated.
> >>
> >> Possibly Yes for some reasons below!
> >>
> >>>
> >>> 2. Leave the old driver there to support the old deprecated bindings,
> >>> at least until everyone can be transferred over.  There seem to be
> >>> quite a few existing users of "qcom,qfprom" and we're supposed to make
> >>> an attempt at keeping the old device trees working, at least for a
> >>> little while.  Once everyone is transferred over we could decide to
> >>> delete the old driver.
> >> we could consider "qcom,qfrom" to be only passing corrected address
> >> space. Till we transition users to new bindings!
> >>
> >>>
> >> Yes.
> >>
> >>> 3. We will have a totally new driver here.
> >> No, we should still be using the same driver. But the exiting driver
> >> seems to incorrect and is need of fixing.
> >>
> >> Having a look at the memory map for old SoCs like msm8996 and msm8916
> >> shows that memory map that was passed to qfprom driver is corrected
> >> address space. Writes will not obviously work!
> >>
> >> This should also be true with sdm845 or sc7180
> >>
> >> That needs to be fixed first!
> >
> > OK, so to summarize:
> >
>
> > 1. We will have one driver: "drivers/nvmem/qfprom.c"
>
> Yes, we should one driver for this because we are dealing with exactly
> same IP.
>
> >
> > 2. If the driver detects that its reg is pointing to the corrected
> > address space then it should operate in read-only mode.  Maybe it can
> > do this based on the compatible string being just "qcom,qfprom" or
> > maybe it can do this based on the size of the "reg".
>
> I found out that there is a version register at offset of 0x6000 which
> can give MAJOR, MINOR and STEP numbers.
>
> So we could still potentially continue using "qcom,qfprom"

OK, sounds good.  I think it's still good practice to include both the
SoC specific and the generic.  Even if the driver never does anything
with the SoC-specific compatible string it doesn't hurt to have it
there.  Thus, we'd want:

compatible = "qcom,msm8996-qfprom", "qcom,qfprom"

The driver itself would never need to refer to the SoC-specific name
but that does give us more flexibility.


> The address space can be split into 3 resources, which is inline with
> Specs as well
>
> 1. Raw address space ("raw")
> 2. Configuration address space ("conf" or "core")
> 3. Corrected address space ("corrected")

Sure, this is OK with me then.  Originally Ravi had 3 ranges but then
he was (in the driver) treating it as one range.  As long as the
driver truly treats it as 3 ranges I have no problem doing it like
this.

In general, over the years, there has been a push to keep
implementation details out of the dts and rely more on the "of_match"
table to add SoC-specific details.  This becomes really important when
1 year down the road you realize that you need one more random
property or address range to fix some bug and then you need to figure
out how to try to keep old "dts" files compatible.  It's not a
hard-and-fast rule, though.


> Exiting qfprom entries or read-only qfprom  will have "corrected"
> address space which can be the only resource provided by device tree
> entries.
> Other two entries("raw" and "conf") are optional.
>
> qfprom: qfprom@780000 {
>          compatible = "qcom,qfprom";
>         reg = <0 0x00780000 0 0x8ff>,
>                 <0 0x00782000 0 0x100>,
>                 <0 0x00784000 0 0x8ff>;
>         reg-names = "raw", "conf", "corrected";
>
>         vcc-supply = <&vreg_xyz>;
>
>         clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>         clock-names = "secclk";
>
>         assigned-clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>          assigned-clock-rates = <19200000>;
>
>         qcom,fuse-blow-frequency = <4800000>
>
>          #address-cells = <1>;
>          #size-cells = <1>;
>
>         qusb2p_hstx_trim: hstx-trim-primary@25b {
>                 reg = <0x25b 0x1>;
>                 bits = <1 3>;
>         };
> };
>
> Regarding clk rate setting, the default rate can be set using
> assigned-clock-rates property, however the blow frequency can go as new
> binding.
> regarding voltage range for regulator, it should come as part of board
> specific voltage regulator node. In worst case we can discuss on adding
> new bindings for allowing specific range.

I'd up to you (and Rob H, who probably will wait for the next rev of
the binding before chiming in) but the "qcom,fuse-blow-frequency" is
the type of property that feels better in the driver and achieved from
the of_match table.  Then you don't need to worry about adding it to
the bindings.  Voltage (if needed) would be similar, but I would hope
we don't need it.


> for Older SoCs: we still continue to use old style with just one
> resource corresponding to corrected by default.
>
> qfprom: qfprom@784000 {
>          compatible = "qcom,qfprom";
>          reg = <0 0x00784000 0 0x8ff>;
>          #address-cells = <1>;
>          #size-cells = <1>;
>
>          qusb2p_hstx_trim: hstx-trim-primary@1eb {
>                  reg = <0x1eb 0x1>;
>                  bits = <1 4>;
>          };
>
>          qusb2s_hstx_trim: hstx-trim-secondary@1eb {
>                  reg = <0x1eb 0x2>;
>                  bits = <6 4>;
>          };
> };
>
>
> I see the patch as adding write support to qfprom, rather than adding
> new driver or new SoC support.
>
> This in summary should give us good direction for this patch!
>
> Correct me if I miss understood something here!

Sounds sane to me.


-Doug

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 26/05/2020 23:31, Doug Anderson wrote:
> Hi,
> 
> On Fri, May 22, 2020 at 4:18 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> On 21/05/2020 22:28, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
>>> <srinivas.kandagatla@linaro.org> wrote:
>>>>
>>>> On 21/05/2020 16:10, Doug Anderson wrote:
>>>>>> On 20/05/2020 23:48, Doug Anderson wrote:
>>>>>>>> Is this only applicable for corrected address space?
>>>>>>> I guess I was proposing a two dts-node / two drive approach here.
>>>>>>>
>>>>>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
>>>>>>> driver #1: read-only and reads the FEC-corrected data
>>>>>>>
>>>>>>> dts node #2: covers the memory range that's_not_  the FEC-corrected
>>>>>>> memory range.
>>>>>>> driver #2: read-write.  reading reads uncorrected data
>>>>>>>
>>>>>>> Does that seem sane?
>>>>>> I see your point but it does not make sense to have two node for same thing.
>>>>> OK, so that sounds as if we want to go with the proposal where we
>>>>> "deprecate the old driver and/or bindings and say that there really
>>>>> should just be one node and one driver".
>>>>>
>>>>> Would this be acceptable to you?
>>>>>
>>>>> 1. Officially mark the old bindings as deprecated.
>>>>
>>>> Possibly Yes for some reasons below!
>>>>
>>>>>
>>>>> 2. Leave the old driver there to support the old deprecated bindings,
>>>>> at least until everyone can be transferred over.  There seem to be
>>>>> quite a few existing users of "qcom,qfprom" and we're supposed to make
>>>>> an attempt at keeping the old device trees working, at least for a
>>>>> little while.  Once everyone is transferred over we could decide to
>>>>> delete the old driver.
>>>> we could consider "qcom,qfrom" to be only passing corrected address
>>>> space. Till we transition users to new bindings!
>>>>
>>>>>
>>>> Yes.
>>>>
>>>>> 3. We will have a totally new driver here.
>>>> No, we should still be using the same driver. But the exiting driver
>>>> seems to incorrect and is need of fixing.
>>>>
>>>> Having a look at the memory map for old SoCs like msm8996 and msm8916
>>>> shows that memory map that was passed to qfprom driver is corrected
>>>> address space. Writes will not obviously work!
>>>>
>>>> This should also be true with sdm845 or sc7180
>>>>
>>>> That needs to be fixed first!
>>>
>>> OK, so to summarize:
>>>
>>
>>> 1. We will have one driver: "drivers/nvmem/qfprom.c"
>>
>> Yes, we should one driver for this because we are dealing with exactly
>> same IP.
>>
>>>
>>> 2. If the driver detects that its reg is pointing to the corrected
>>> address space then it should operate in read-only mode.  Maybe it can
>>> do this based on the compatible string being just "qcom,qfprom" or
>>> maybe it can do this based on the size of the "reg".
>>
>> I found out that there is a version register at offset of 0x6000 which
>> can give MAJOR, MINOR and STEP numbers.
>>
>> So we could still potentially continue using "qcom,qfprom"
> 
> OK, sounds good.  I think it's still good practice to include both the
> SoC specific and the generic.  Even if the driver never does anything
> with the SoC-specific compatible string it doesn't hurt to have it
> there.  Thus, we'd want:
> 
> compatible = "qcom,msm8996-qfprom", "qcom,qfprom"
> 
> The driver itself would never need to refer to the SoC-specific name
> but that does give us more flexibility.
> 
> 
>> The address space can be split into 3 resources, which is inline with
>> Specs as well
>>
>> 1. Raw address space ("raw")
>> 2. Configuration address space ("conf" or "core")
>> 3. Corrected address space ("corrected")
> 
> Sure, this is OK with me then.  Originally Ravi had 3 ranges but then
> he was (in the driver) treating it as one range.  As long as the
> driver truly treats it as 3 ranges I have no problem doing it like
> this.
> 
> In general, over the years, there has been a push to keep
> implementation details out of the dts and rely more on the "of_match"
> table to add SoC-specific details.  This becomes really important when
> 1 year down the road you realize that you need one more random
> property or address range to fix some bug and then you need to figure
> out how to try to keep old "dts" files compatible.  It's not a
> hard-and-fast rule, though.

Am not 100% sure if "qcom,fuse-blow-frequency" is something integration 
specific or SoC Specific, My idea was that this will give more 
flexibility in future. As adding new SoC Support does not need driver 
changes.

Having said that, Am okay either way!
Incase we go compatible way, I would like to see compatible strings 
having proper IP versions to have ip version rather than SoC names.

Having SoC names in compatible string means both driver and bindings 
need update for every new SoC which can be overhead very soon!

Rob can help review once we have v2 bindings out!

> 
> 
>> Exiting qfprom entries or read-only qfprom  will have "corrected"
>> address space which can be the only resource provided by device tree
>> entries.
>> Other two entries("raw" and "conf") are optional.
>>
>> qfprom: qfprom@780000 {
>>           compatible = "qcom,qfprom";
>>          reg = <0 0x00780000 0 0x8ff>,
>>                  <0 0x00782000 0 0x100>,
>>                  <0 0x00784000 0 0x8ff>;
>>          reg-names = "raw", "conf", "corrected";
>>
>>          vcc-supply = <&vreg_xyz>;
>>
>>          clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>>          clock-names = "secclk";
>>
>>          assigned-clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
>>           assigned-clock-rates = <19200000>;
>>
>>          qcom,fuse-blow-frequency = <4800000>
>>
>>           #address-cells = <1>;
>>           #size-cells = <1>;
>>
>>          qusb2p_hstx_trim: hstx-trim-primary@25b {
>>                  reg = <0x25b 0x1>;
>>                  bits = <1 3>;
>>          };
>> };
>>
>> Regarding clk rate setting, the default rate can be set using
>> assigned-clock-rates property, however the blow frequency can go as new
>> binding.
>> regarding voltage range for regulator, it should come as part of board
>> specific voltage regulator node. In worst case we can discuss on adding
>> new bindings for allowing specific range.
> 
> I'd up to you (and Rob H, who probably will wait for the next rev of
> the binding before chiming in) but the "qcom,fuse-blow-frequency" is
> the type of property that feels better in the driver and achieved from
> the of_match table.  Then you don't need to worry about adding it to
> the bindings.  Voltage (if needed) would be similar, but I would hope
> we don't need it.
> 
> 
>> for Older SoCs: we still continue to use old style with just one
>> resource corresponding to corrected by default.
>>
>> qfprom: qfprom@784000 {
>>           compatible = "qcom,qfprom";
>>           reg = <0 0x00784000 0 0x8ff>;
>>           #address-cells = <1>;
>>           #size-cells = <1>;
>>
>>           qusb2p_hstx_trim: hstx-trim-primary@1eb {
>>                   reg = <0x1eb 0x1>;
>>                   bits = <1 4>;
>>           };
>>
>>           qusb2s_hstx_trim: hstx-trim-secondary@1eb {
>>                   reg = <0x1eb 0x2>;
>>                   bits = <6 4>;
>>           };
>> };
>>
>>
>> I see the patch as adding write support to qfprom, rather than adding
>> new driver or new SoC support.
>>
>> This in summary should give us good direction for this patch!
>>
>> Correct me if I miss understood something here!
> 
> Sounds sane to me.

Cool! lets see how v2 will endup like!

--srini
> 
> 
> -Doug
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Doug Anderson]

Hi,

On Mon, Jun 1, 2020 at 2:25 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 26/05/2020 23:31, Doug Anderson wrote:
> > Hi,
> >
> > On Fri, May 22, 2020 at 4:18 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >> On 21/05/2020 22:28, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Thu, May 21, 2020 at 8:56 AM Srinivas Kandagatla
> >>> <srinivas.kandagatla@linaro.org> wrote:
> >>>>
> >>>> On 21/05/2020 16:10, Doug Anderson wrote:
> >>>>>> On 20/05/2020 23:48, Doug Anderson wrote:
> >>>>>>>> Is this only applicable for corrected address space?
> >>>>>>> I guess I was proposing a two dts-node / two drive approach here.
> >>>>>>>
> >>>>>>> dts node #1:just covers the memory range for accessing the FEC-corrected data
> >>>>>>> driver #1: read-only and reads the FEC-corrected data
> >>>>>>>
> >>>>>>> dts node #2: covers the memory range that's_not_  the FEC-corrected
> >>>>>>> memory range.
> >>>>>>> driver #2: read-write.  reading reads uncorrected data
> >>>>>>>
> >>>>>>> Does that seem sane?
> >>>>>> I see your point but it does not make sense to have two node for same thing.
> >>>>> OK, so that sounds as if we want to go with the proposal where we
> >>>>> "deprecate the old driver and/or bindings and say that there really
> >>>>> should just be one node and one driver".
> >>>>>
> >>>>> Would this be acceptable to you?
> >>>>>
> >>>>> 1. Officially mark the old bindings as deprecated.
> >>>>
> >>>> Possibly Yes for some reasons below!
> >>>>
> >>>>>
> >>>>> 2. Leave the old driver there to support the old deprecated bindings,
> >>>>> at least until everyone can be transferred over.  There seem to be
> >>>>> quite a few existing users of "qcom,qfprom" and we're supposed to make
> >>>>> an attempt at keeping the old device trees working, at least for a
> >>>>> little while.  Once everyone is transferred over we could decide to
> >>>>> delete the old driver.
> >>>> we could consider "qcom,qfrom" to be only passing corrected address
> >>>> space. Till we transition users to new bindings!
> >>>>
> >>>>>
> >>>> Yes.
> >>>>
> >>>>> 3. We will have a totally new driver here.
> >>>> No, we should still be using the same driver. But the exiting driver
> >>>> seems to incorrect and is need of fixing.
> >>>>
> >>>> Having a look at the memory map for old SoCs like msm8996 and msm8916
> >>>> shows that memory map that was passed to qfprom driver is corrected
> >>>> address space. Writes will not obviously work!
> >>>>
> >>>> This should also be true with sdm845 or sc7180
> >>>>
> >>>> That needs to be fixed first!
> >>>
> >>> OK, so to summarize:
> >>>
> >>
> >>> 1. We will have one driver: "drivers/nvmem/qfprom.c"
> >>
> >> Yes, we should one driver for this because we are dealing with exactly
> >> same IP.
> >>
> >>>
> >>> 2. If the driver detects that its reg is pointing to the corrected
> >>> address space then it should operate in read-only mode.  Maybe it can
> >>> do this based on the compatible string being just "qcom,qfprom" or
> >>> maybe it can do this based on the size of the "reg".
> >>
> >> I found out that there is a version register at offset of 0x6000 which
> >> can give MAJOR, MINOR and STEP numbers.
> >>
> >> So we could still potentially continue using "qcom,qfprom"
> >
> > OK, sounds good.  I think it's still good practice to include both the
> > SoC specific and the generic.  Even if the driver never does anything
> > with the SoC-specific compatible string it doesn't hurt to have it
> > there.  Thus, we'd want:
> >
> > compatible = "qcom,msm8996-qfprom", "qcom,qfprom"
> >
> > The driver itself would never need to refer to the SoC-specific name
> > but that does give us more flexibility.
> >
> >
> >> The address space can be split into 3 resources, which is inline with
> >> Specs as well
> >>
> >> 1. Raw address space ("raw")
> >> 2. Configuration address space ("conf" or "core")
> >> 3. Corrected address space ("corrected")
> >
> > Sure, this is OK with me then.  Originally Ravi had 3 ranges but then
> > he was (in the driver) treating it as one range.  As long as the
> > driver truly treats it as 3 ranges I have no problem doing it like
> > this.
> >
> > In general, over the years, there has been a push to keep
> > implementation details out of the dts and rely more on the "of_match"
> > table to add SoC-specific details.  This becomes really important when
> > 1 year down the road you realize that you need one more random
> > property or address range to fix some bug and then you need to figure
> > out how to try to keep old "dts" files compatible.  It's not a
> > hard-and-fast rule, though.
>
> Am not 100% sure if "qcom,fuse-blow-frequency" is something integration
> specific or SoC Specific, My idea was that this will give more
> flexibility in future. As adding new SoC Support does not need driver
> changes.
>
> Having said that, Am okay either way!

Yeah, it's always a balance.  I guess the question is: why do we think
driver changes are worse than dts changes?  The value still needs to
be somewhere and having it in the driver isn't a terrible place.


> Incase we go compatible way, I would like to see compatible strings
> having proper IP versions to have ip version rather than SoC names.
>
> Having SoC names in compatible string means both driver and bindings
> need update for every new SoC which can be overhead very soon!

Almost certainly the compatible strings should have SoC names in them.
Yes it means a binding update every time a new SoC comes up but that
is just how device tree works.  Presumably there's enough chatter on
this that Rob H has totally tuned it out at this point in time, but
there are many other instances of this.

NOTE: just because we have the SoC name in the compatible string
_doesn't_ mean that the driver has to change.  You already said that
the IP version can be detected earlier in this thread, right?  You
said:

I found out that there is a version register at offset of 0x6000 which
can give MAJOR, MINOR and STEP numbers.

So how about this:

a) Compatible contains "SoC" version and the generic "qcom,qfrom", so:

compatible = "qcom,sdm845-qfprom", "qcom,qfrom"

b) Bindings will need to be updated for every new SoC, but that's
normal and should be a trivial patch to just add a new SoC to the
list.

c) If the driver can be made to make its decisions about frequencies /
timings completely by MAJOR/MINOR/STEP numbers then it can use those
in its decision and it will never need to use the SoC-specific
compatible string.  The SoC-specific compatible string will only be
present as a fallback "oops we have to workaround a bug that we didn't
know about".


> Rob can help review once we have v2 bindings out!

Sounds good.  If you're still not convinced by my arguments we can see
if we can get Rob to clarify once we have a v2.  :-)


> >> Exiting qfprom entries or read-only qfprom  will have "corrected"
> >> address space which can be the only resource provided by device tree
> >> entries.
> >> Other two entries("raw" and "conf") are optional.
> >>
> >> qfprom: qfprom@780000 {
> >>           compatible = "qcom,qfprom";
> >>          reg = <0 0x00780000 0 0x8ff>,
> >>                  <0 0x00782000 0 0x100>,
> >>                  <0 0x00784000 0 0x8ff>;
> >>          reg-names = "raw", "conf", "corrected";
> >>
> >>          vcc-supply = <&vreg_xyz>;
> >>
> >>          clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> >>          clock-names = "secclk";
> >>
> >>          assigned-clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> >>           assigned-clock-rates = <19200000>;
> >>
> >>          qcom,fuse-blow-frequency = <4800000>
> >>
> >>           #address-cells = <1>;
> >>           #size-cells = <1>;
> >>
> >>          qusb2p_hstx_trim: hstx-trim-primary@25b {
> >>                  reg = <0x25b 0x1>;
> >>                  bits = <1 3>;
> >>          };
> >> };
> >>
> >> Regarding clk rate setting, the default rate can be set using
> >> assigned-clock-rates property, however the blow frequency can go as new
> >> binding.
> >> regarding voltage range for regulator, it should come as part of board
> >> specific voltage regulator node. In worst case we can discuss on adding
> >> new bindings for allowing specific range.
> >
> > I'd up to you (and Rob H, who probably will wait for the next rev of
> > the binding before chiming in) but the "qcom,fuse-blow-frequency" is
> > the type of property that feels better in the driver and achieved from
> > the of_match table.  Then you don't need to worry about adding it to
> > the bindings.  Voltage (if needed) would be similar, but I would hope
> > we don't need it.
> >
> >
> >> for Older SoCs: we still continue to use old style with just one
> >> resource corresponding to corrected by default.
> >>
> >> qfprom: qfprom@784000 {
> >>           compatible = "qcom,qfprom";
> >>           reg = <0 0x00784000 0 0x8ff>;
> >>           #address-cells = <1>;
> >>           #size-cells = <1>;
> >>
> >>           qusb2p_hstx_trim: hstx-trim-primary@1eb {
> >>                   reg = <0x1eb 0x1>;
> >>                   bits = <1 4>;
> >>           };
> >>
> >>           qusb2s_hstx_trim: hstx-trim-secondary@1eb {
> >>                   reg = <0x1eb 0x2>;
> >>                   bits = <6 4>;
> >>           };
> >> };
> >>
> >>
> >> I see the patch as adding write support to qfprom, rather than adding
> >> new driver or new SoC support.
> >>
> >> This in summary should give us good direction for this patch!
> >>
> >> Correct me if I miss understood something here!
> >
> > Sounds sane to me.
>
> Cool! lets see how v2 will endup like!
>
> --srini
> >
> >
> > -Doug
> >

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[Srinivas Kandagatla]

On 01/06/2020 19:08, Doug Anderson wrote:
>> Am not 100% sure if "qcom,fuse-blow-frequency" is something integration
>> specific or SoC Specific, My idea was that this will give more
>> flexibility in future. As adding new SoC Support does not need driver
>> changes.
>>
>> Having said that, Am okay either way!
> Yeah, it's always a balance.  I guess the question is: why do we think
> driver changes are worse than dts changes?  The value still needs to
> be somewhere and having it in the driver isn't a terrible place.
> 

TBH, its an overkill if we are using same IP version across multiple SoCs.

> 
>> Incase we go compatible way, I would like to see compatible strings
>> having proper IP versions to have ip version rather than SoC names.
>>
>> Having SoC names in compatible string means both driver and bindings
>> need update for every new SoC which can be overhead very soon!
> Almost certainly the compatible strings should have SoC names in them.
> Yes it means a binding update every time a new SoC comes up but that
> is just how device tree works.  Presumably there's enough chatter on
> this that Rob H has totally tuned it out at this point in time, but
> there are many other instances of this.
> 
> NOTE: just because we have the SoC name in the compatible string
> _doesn't_  mean that the driver has to change.  You already said that
> the IP version can be detected earlier in this thread, right?  You
> said:
> 
> I found out that there is a version register at offset of 0x6000 which
> can give MAJOR, MINOR and STEP numbers.
> 
> So how about this:
> 
> a) Compatible contains "SoC" version and the generic "qcom,qfrom", so:
> 
> compatible = "qcom,sdm845-qfprom", "qcom,qfrom"
> 
> b) Bindings will need to be updated for every new SoC, but that's
> normal and should be a trivial patch to just add a new SoC to the
> list.
> 
> c) If the driver can be made to make its decisions about frequencies /
> timings completely by MAJOR/MINOR/STEP numbers then it can use those
> in its decision and it will never need to use the SoC-specific
> compatible string.  The SoC-specific compatible string will only be
> present as a fallback "oops we have to workaround a bug that we didn't
> know about".

This makes more sense to me, I would still stay with  MAJOR/MINOR/STEP 
numbers mostly unless we are dealing with some corner cases.


thanks,
srini
> 
> 
>> Rob can help review once we have v2 bindings out!
> Sounds good.  If you're still not convinced by my arguments we can see
> if we can get Rob to clarify once we have a v2.:-)
> 
> 

Re: [RFC v1 2/3] drivers: nvmem: Add driver for QTI qfprom-efuse support

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1314 bytes --]

Hi Ravi,

[FYI, it's a private test report for your RFC patch.]
[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v5.7-rc5 next-20200515]
[cannot apply to linux/master]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Ravi-Kumar-Bokka/Add-QTI-QFPROM-Efuse-driver-support/20200514-154913
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
reproduce:
        # apt-get install sparse
        # sparse version: v0.6.1-193-gb8fad4bc-dirty
        make ARCH=x86_64 allmodconfig
        make C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__'

If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>


sparse warnings: (new ones prefixed by >>)

>> drivers/nvmem/qfprom-efuse.c:209:6: sparse: sparse: symbol 'addr_in_qfprom_range' was not declared. Should it be static?

Please review and possibly fold the followup patch.

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[RFC PATCH] drivers: nvmem: addr_in_qfprom_range() can be static

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]


Signed-off-by: kbuild test robot <lkp@intel.com>
---
 qfprom-efuse.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/nvmem/qfprom-efuse.c b/drivers/nvmem/qfprom-efuse.c
index 2e3c275e963f9..577257d61d22d 100644
--- a/drivers/nvmem/qfprom-efuse.c
+++ b/drivers/nvmem/qfprom-efuse.c
@@ -206,8 +206,8 @@ static int qfprom_enable_fuse_blowing(const struct qfprom_efuse_priv *priv)
  * verifying to make sure address being written or read is from qfprom
  * raw address range
  */
-bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
-			  size_t bytes)
+static bool addr_in_qfprom_range(const struct qfprom_efuse_priv *priv, u32 reg,
+				 size_t bytes)
 {
 	if (((reg + bytes) > reg) && (reg >= priv->qfpraw_start) &&
 	    ((reg + bytes) <= priv->qfpraw_end)) {

[RFC v1 3/3] arm64: dts: qcom: sc7180: Add qfprom-efuse

[Ravi Kumar Bokka]

This patch adds device tree node for qfprom-efuse controller.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
---
 arch/arm64/boot/dts/qcom/sc7180-idp.dts | 4 ++++
 arch/arm64/boot/dts/qcom/sc7180.dtsi    | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index 4dd8ebc..7378c82 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -234,6 +234,10 @@
 	};
 };
 
+&qfprom_efuse {
+	vcc-supply = <&vreg_l11a_1p8>;
+};
+
 &qspi {
 	status = "okay";
 	pinctrl-names = "default";
diff --git a/arch/arm64/boot/dts/qcom/sc7180.dtsi b/arch/arm64/boot/dts/qcom/sc7180.dtsi
index 4216b57..bbd22fb 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -457,6 +457,15 @@
 			#power-domain-cells = <1>;
 		};
 
+		qfprom_efuse: efuse@780000 {
+			compatible = "qcom,sc7180-qfprom-efuse";
+			reg = <0 0x00780000 0 0x100>,
+			      <0 0x00780120 0 0x7a0>,
+			      <0 0x00782000 0 0x100>;
+			clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+			clock-names = "secclk";
+		};
+
 		qfprom@784000 {
 			compatible = "qcom,qfprom";
 			reg = <0 0x00784000 0 0x8ff>;
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

Re: [RFC v1 0/3] Add QTI QFPROM-Efuse driver support

[Doug Anderson]

Hi,

On Tue, May 12, 2020 at 11:18 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch series adds qfprom-efuse controller driver support.
>
> This driver can access the raw qfprom regions for fuse blowing.
>
> The current existed qfprom driver is only supports for cpufreq, thermal sensors
> drivers by read out calibration data, speed bins..etc which is stored by
> qfprom efuses.

I don't understand the interaction between this driver and the
existing "qcom,qfprom" driver.  Can you please explain?  Are they both
acting on the same values and this one has write access?  Are there
two instances of the same hardware block and you're managing one of
them with this new driver and one with thue old driver?  Something
else?



> Ravi Kumar Bokka (3):
>   dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse
>   drivers: nvmem: Add driver for QTI qfprom-efuse support
>   arm64: dts: qcom: sc7180: Add qfprom-efuse
>
>  .../devicetree/bindings/nvmem/qfprom-efuse.yaml    |  40 ++
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts            |   4 +
>  arch/arm64/boot/dts/qcom/sc7180.dtsi               |   9 +
>  drivers/nvmem/Kconfig                              |  10 +
>  drivers/nvmem/Makefile                             |   2 +
>  drivers/nvmem/qfprom-efuse.c                       | 476 +++++++++++++++++++++
>  6 files changed, 541 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
>  create mode 100644 drivers/nvmem/qfprom-efuse.c
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
>

Re: [RFC v1 0/3] Add QTI QFPROM-Efuse driver support

[Doug Anderson]

Hi,

On Wed, May 13, 2020 at 8:35 PM Ravi Kumar Bokka (Temp)
<rbokka@codeaurora.org> wrote:
>
> Hi doug,
>
> Thanks for your feedback. Please find below inline comments.

Probably the mailing list didn't see them.  You responded with HTML
mail.  Please be careful to only respond in plain text.


> Regards,
>
> Ravi Kumar.B
>
>
>
> On 5/13/2020 4:33 AM, Doug Anderson wrote:
>
> Hi,
>
> On Tue, May 12, 2020 at 11:18 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch series adds qfprom-efuse controller driver support.
>
> This driver can access the raw qfprom regions for fuse blowing.
>
> The current existed qfprom driver is only supports for cpufreq, thermal sensors
> drivers by read out calibration data, speed bins..etc which is stored by
> qfprom efuses.
>
> I don't understand the interaction between this driver and the
> existing "qcom,qfprom" driver.  Can you please explain?  Are they both
> acting on the same values and this one has write access?  Are there
> two instances of the same hardware block and you're managing one of
> them with this new driver and one with thue old driver?  Something
> else?
>
> [Ravi] Existing QFPROM driver in upstream kernel has limited support which is some hard coded mapping of id vs set of fuses and user can read those fuse with those id-bucket.
> That is simply reading Hw-registers and it doesn't involve any hardware programming sequence etc. Based on information given to us by QC-kernel team, existing driver was created to read calibration/sensor fuses and it is very basic/limited/fixed in functionalities and orthogonal to what we need to on Trogdor.
>
> Requirement for Trogdor fuse blow driver is different which allows to read/write almost whole fuse block and requires to follow HW programming guide. Both are completely separate and has no overlapping in terms of functionalities and capability. Please ignore the similarity of names of drivers, they are different in terms of functionalities and driver internals etc.

If they are targeting the same type of hardware IP block then, in the
very least, the bindings need to be the same.  The bindings are
supposed to be describing the hardware.

Presumably if the underlying hardware is the same you should be able
to write one driver and it can just operate in some sort of
"read-only" mode if it's running somewhere it doesn't have access
permissions to actually change the fuses.




> Ravi Kumar Bokka (3):
>   dt-bindings: nvmem: Add devicetree bindings for qfprom-efuse
>   drivers: nvmem: Add driver for QTI qfprom-efuse support
>   arm64: dts: qcom: sc7180: Add qfprom-efuse
>
>  .../devicetree/bindings/nvmem/qfprom-efuse.yaml    |  40 ++
>  arch/arm64/boot/dts/qcom/sc7180-idp.dts            |   4 +
>  arch/arm64/boot/dts/qcom/sc7180.dtsi               |   9 +
>  drivers/nvmem/Kconfig                              |  10 +
>  drivers/nvmem/Makefile                             |   2 +
>  drivers/nvmem/qfprom-efuse.c                       | 476 +++++++++++++++++++++
>  6 files changed, 541 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom-efuse.yaml
>  create mode 100644 drivers/nvmem/qfprom-efuse.c
>
> --
> Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.
>