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

[RFC v2 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 QTI qfprom-efuse support
  arm64: dts: qcom: sc7180: Add qfprom-efuse

 .../devicetree/bindings/nvmem/qfprom.yaml          |  52 +++
 arch/arm64/boot/dts/qcom/sc7180-idp.dts            |   4 +
 arch/arm64/boot/dts/qcom/sc7180.dtsi               |  10 +-
 drivers/nvmem/Kconfig                              |   1 +
 drivers/nvmem/qfprom.c                             | 405 +++++++++++++++++++--
 5 files changed, 449 insertions(+), 23 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml

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

[RFC v2 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.yaml          | 52 ++++++++++++++++++++++
 1 file changed, 52 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
new file mode 100644
index 0000000..7c8fc31
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
@@ -0,0 +1,52 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/qfprom.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,qfprom
+
+  reg:
+    maxItems: 3
+
+required:
+   - compatible
+   - reg
+   - reg-names
+   - clocks
+   - clock-names
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    qfprom@780000 {
+	compatible = "qcom,qfprom";
+	reg = <0 0x00780000 0 0x7a0>,
+	      <0 0x00782000 0 0x100>,
+	      <0 0x00784000 0 0x8ff>;
+	reg-names = "raw", "conf", "corrected";
+	clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+	clock-names = "secclk";
+
+	qusb2p_hstx_trim: hstx-trim-primary@25b {
+		reg = <0x25b 0x1>;
+		bits = <1 3>;
+	};
+    };
+
+    &qfprom {
+        vcc-supply = <&vreg_l11a_1p8>;
+    };
+
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

[RFC v2 2/3] drivers: nvmem: Add QTI qfprom-efuse support

[Ravi Kumar Bokka]

This patch adds support 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  |   1 +
 drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++---
 2 files changed, 385 insertions(+), 21 deletions(-)

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index d7b7f6d..623d59e 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -117,6 +117,7 @@ config QCOM_QFPROM
 	help
 	  Say y here to enable QFPROM support. The QFPROM provides access
 	  functions for QFPROM data to rest of the drivers via nvmem interface.
+	  And 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.
diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 8a91717..312318c 100644
--- a/drivers/nvmem/qfprom.c
+++ b/drivers/nvmem/qfprom.c
@@ -3,17 +3,266 @@
  * Copyright (C) 2015 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
  */
 
+#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/io.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_priv - structure holding qfprom attributes
+ *
+ * @qfpraw: iomapped memory space for qfprom-efuse raw address space
+ * @qfpconf: iomapped memory space for qfprom-efuse configuration address space
+ * @qfpcorrected: iomapped memory space for qfprom corrected address space
+
+ * @dev: qfprom device structure
+ * @secclk: clock supply
+ * @vcc: regulator supply
 
+ * @qfprom_efuse_platform_data: qfprom platform data
+ */
 struct qfprom_priv {
-	void __iomem *base;
+	void __iomem *qfpraw;
+	void __iomem *qfpconf;
+	void __iomem *qfpcorrected;
+	struct device *dev;
+	struct clk *secclk;
+	struct regulator *vcc;
+	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_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_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_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_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->qfpconf +
+			QFPROM_BLOW_TIMER_OFFSET);
+	writel(priv->efuse.accel_reset_value,
+		priv->qfpconf + 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_priv *priv)
+{
+	int ret;
+
+	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;
+	}
+
+	writel(priv->efuse.qfprom_blow_timer_value, priv->qfpconf +
+	       QFPROM_BLOW_TIMER_OFFSET);
+	writel(priv->efuse.accel_value, priv->qfpconf + QFPROM_ACCEL_OFFSET);
+
+	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_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;
+	}
+
+	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\n");
+			return ret;
+		}
+
+		ret = readl_relaxed_poll_timeout(priv->qfpconf +
+				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->qfpraw + reg + (i++ * 4));
+
+			ret = readl_relaxed_poll_timeout(priv->qfpconf +
+				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;
+			}
+		}
+
+		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_reg_read(void *context,
 			unsigned int reg, void *_val, size_t bytes)
 {
@@ -22,47 +271,161 @@ static int qfprom_reg_read(void *context,
 	int i = 0, words = bytes;
 
 	while (words--)
-		*val++ = readb(priv->base + reg + i++);
+		*val++ = readb(priv->qfpraw + reg + i++);
 
 	return 0;
 }
 
-static struct nvmem_config econfig = {
-	.name = "qfprom",
-	.stride = 1,
-	.word_size = 1,
-	.reg_read = qfprom_reg_read,
-};
+static int qfprom_reg_write(void *context,
+			 unsigned int reg, void *_val, size_t bytes)
+{
+	struct qfprom_priv *priv = context;
+	u8 *val = _val;
+	int i = 0, words = bytes;
+
+	while (words--)
+		writeb(*val++, priv->qfpraw + reg + i++);
+
+	return 0;
+}
 
 static int qfprom_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
-	struct resource *res;
-	struct nvmem_device *nvmem;
+	struct resource *qfpraw, *qfpconf, *qfpcorrected;
+	struct nvmem_device *nvmem, *nvmem_efuse;
+	struct nvmem_config *econfig, *econfig_efuse;
 	struct qfprom_priv *priv;
+	const struct qfprom_efuse_platform_data *drvdata;
+	int ret;
+
+	drvdata = of_device_get_match_data(&pdev->dev);
+	if (!drvdata)
+		return -EINVAL;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	priv->base = devm_ioremap_resource(dev, res);
-	if (IS_ERR(priv->base))
-		return PTR_ERR(priv->base);
+	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;
+
+	qfpraw = platform_get_resource_byname(pdev, IORESOURCE_MEM, "raw");
+
+	priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
+	if (IS_ERR(priv->qfpraw)) {
+		ret = PTR_ERR(priv->qfpraw);
+		goto err;
+	}
+
+	qfpconf = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf");
+
+	priv->qfpconf = devm_ioremap_resource(dev, qfpconf);
+	if (IS_ERR(priv->qfpconf)) {
+		ret = PTR_ERR(priv->qfpconf);
+		goto err;
+	}
+
+	qfpcorrected = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						    "corrected");
+
+	priv->qfpcorrected = devm_ioremap_resource(dev, qfpcorrected);
+	if (IS_ERR(priv->qfpcorrected)) {
+		ret = PTR_ERR(priv->qfpcorrected);
+		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;
+	}
 
-	econfig.size = resource_size(res);
-	econfig.dev = dev;
-	econfig.priv = priv;
+	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;
+	}
 
-	nvmem = devm_nvmem_register(dev, &econfig);
+	ret = clk_prepare_enable(priv->secclk);
+	if (ret) {
+		dev_err(dev, "clk_prepare_enable() failed\n");
+		goto err;
+	}
 
-	return PTR_ERR_OR_ZERO(nvmem);
+	econfig_efuse = devm_kzalloc(dev, sizeof(*econfig_efuse), GFP_KERNEL);
+	if (!econfig_efuse)
+		return -ENOMEM;
+
+	econfig_efuse->dev = dev;
+	econfig_efuse->name = "qfprom-efuse";
+	econfig_efuse->stride = 1;
+	econfig_efuse->word_size = 1;
+	econfig_efuse->reg_read = qfprom_reg_read;
+	econfig_efuse->reg_write = qfprom_efuse_reg_write;
+	econfig_efuse->size = resource_size(qfpraw);
+	econfig_efuse->priv = priv;
+
+	nvmem_efuse = devm_nvmem_register(dev, econfig_efuse);
+	if (IS_ERR(nvmem_efuse)) {
+		dev_err(&pdev->dev, "failed to register nvmem config\n");
+		return PTR_ERR(nvmem_efuse);
+	}
+
+	econfig = devm_kzalloc(dev, sizeof(*econfig), GFP_KERNEL);
+	if (!econfig)
+		return -ENOMEM;
+
+	econfig->name = "qfprom",
+	econfig->stride = 1,
+	econfig->word_size = 1,
+	econfig->reg_read = qfprom_reg_read,
+	econfig->reg_write = qfprom_reg_write,
+	econfig->size = resource_size(qfpcorrected);
+	econfig->dev = dev;
+	econfig->priv = priv;
+
+	nvmem = devm_nvmem_register(dev, econfig);
+	if (IS_ERR(nvmem)) {
+		dev_err(&pdev->dev, "failed to register nvmem config\n");
+		return PTR_ERR(nvmem);
+	}
+
+err:
+	clk_disable_unprepare(priv->secclk);
+	return ret;
 }
 
+static const struct qfprom_efuse_platform_data sc7180_qfp_efuse_data = {
+	.name = "qfprom",
+	.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_of_match[] = {
-	{ .compatible = "qcom,qfprom",},
+	{ .compatible = "qcom,qfprom",
+	  .data = &sc7180_qfp_efuse_data
+	},
 	{/* sentinel */},
 };
+
 MODULE_DEVICE_TABLE(of, qfprom_of_match);
 
 static struct platform_driver qfprom_driver = {
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

[RFC v2 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    | 10 ++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
index 4e9149d..2a9224e 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -287,6 +287,10 @@
 	};
 };
 
+&qfprom {
+	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 31b9217..20f3480 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -498,9 +498,15 @@
 			#power-domain-cells = <1>;
 		};
 
-		qfprom@784000 {
+		qfprom: qfprom@780000 {
 			compatible = "qcom,qfprom";
-			reg = <0 0x00784000 0 0x8ff>;
+			reg = <0 0x00780000 0 0x7a0>,
+			      <0 0x00782000 0 0x100>,
+			      <0 0x00784000 0 0x8ff>;
+			reg-names = "raw", "conf", "corrected";
+			
+			clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+			clock-names = "secclk";
 			#address-cells = <1>;
 			#size-cells = <1>;
 
-- 
Qualcomm INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, hosted by the Linux Foundation.

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

[Rajendra Nayak]

On 6/11/2020 3:18 PM, Ravi Kumar Bokka wrote:
> This patch adds dt-bindings document for qfprom-efuse controller.

there is an existing bindings doc (in .txt) which you should convert to yaml,
and then add another patch to extend it.

> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> ---
>   .../devicetree/bindings/nvmem/qfprom.yaml          | 52 ++++++++++++++++++++++
>   1 file changed, 52 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> new file mode 100644
> index 0000000..7c8fc31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/qfprom.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,qfprom
> +
> +  reg:
> +    maxItems: 3
> +
> +required:
> +   - compatible
> +   - reg
> +   - reg-names
> +   - clocks
> +   - clock-names
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    qfprom@780000 {
> +	compatible = "qcom,qfprom";
> +	reg = <0 0x00780000 0 0x7a0>,
> +	      <0 0x00782000 0 0x100>,
> +	      <0 0x00784000 0 0x8ff>;
> +	reg-names = "raw", "conf", "corrected";
> +	clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> +	clock-names = "secclk";
> +
> +	qusb2p_hstx_trim: hstx-trim-primary@25b {
> +		reg = <0x25b 0x1>;
> +		bits = <1 3>;
> +	};
> +    };
> +
> +    &qfprom {
> +        vcc-supply = <&vreg_l11a_1p8>;
> +    };
> +
> 

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

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

[Rob Herring]

On Thu, 11 Jun 2020 15:18:00 +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.yaml          | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
> 


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

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


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

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

[Doug Anderson]

Hi,

On Thu, Jun 11, 2020 at 2:49 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.yaml          | 52 ++++++++++++++++++++++
>  1 file changed, 52 insertions(+)

Overall comment: I reviewed your v1 series and so I'm obviously
interested in your series.  Please CC me on future versions.

I would also note that, since this is relevant to Qualcomm SoCs that
you probably should be CCing "linux-arm-msm@vger.kernel.org" on your
series.


>  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
>
> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> new file mode 100644
> index 0000000..7c8fc31
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/qfprom.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,qfprom

As per discussion in patch #1, I believe SoC compatible should be here
too in case it is ever needed.  This is standard practice for dts
files for IP blocks embedded in an SoC.  AKA, this should be:

    items:
      - enum:
          - qcom,apq8064-qfprom
          - qcom,apq8084-qfprom
          - qcom,msm8974-qfprom
          - qcom,msm8916-qfprom
          - qcom,msm8996-qfprom
          - qcom,msm8998-qfprom
          - qcom,qcs404-qfprom
          - qcom,sc7180-qfprom
          - qcom,sdm845-qfprom
      - const: qcom,qfprom

NOTE: old SoCs won't have both of these and thus they will get flagged
with "dtbs_check", but I believe that's fine (Rob can correct me if
I'm wrong).  The code should still work OK if the SoC isn't there but
it would be good to fix old dts files to have the SoC specific string
too.


> +
> +  reg:
> +    maxItems: 3

Please address feedback feedback on v1.  If you disagree with my
feedback it's OK to say so (I make no claims of being always right),
but silently ignoring my feedback and sending the next version doesn't
make me feel like it's a good use of my time to keep reviewing your
series.  Specifically I suggested that you actually add descriptions
rather than just putting "maxItems: 3".

With all that has been discussed, I think the current best thing to
put there is:

    # If the QFPROM is read-only OS image then only the corrected region
    # needs to be provided.  If the QFPROM is writable then all 3 regions
    # must be provided.
    oneOf:
      - items:
          - description: The start of the corrected region.
      - items:
          - description: The start of the raw region.
          - description: The start of the config region.
          - description: The start of the corrected region.

> +

You missed a bunch of things that you should document:

  # Clocks must be provided if QFPROM is writable from the OS image.
  clocks:
    maxItems: 1
  clock-names:
    const: sec

  # Supply reference must be provided if QFPROM is writable from the OS image.
  vcc-supply:
    description: Our power supply.

  # Needed if any child nodes are present.
  "#address-cells":
    const: 1
  "#size-cells":
    const: 1

> +required:
> +   - compatible
> +   - reg
> +   - reg-names

reg-names is discouraged.  Please remove.  I always point people here
as a reference:

https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/

You can just figure out whether there are 3 register fields or 1 register field.


> +   - clocks
> +   - clock-names

You can't retroactively make things required.  In read-only mode I
believe we don't require clocks/clock-names.  Presumably the clock is
only needed if we're writing?


> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    qfprom@780000 {
> +       compatible = "qcom,qfprom";

As pointed out by Rob H in v1, you have a whole bunch of tabs in here
that "dt_binding_check" yells about.  Please fix and confirm that you
ran "dt_binding_check" on your bindings and they passed with no
errors.  This is another case where someone took the time to respond
to your v1 but you didn't address their comments in sending v2.


> +       reg = <0 0x00780000 0 0x7a0>,
> +             <0 0x00782000 0 0x100>,
> +             <0 0x00784000 0 0x8ff>;
> +       reg-names = "raw", "conf", "corrected";

You are missing #address-cells and #size-cells, which are required
because you have a sub-node.


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

As pointed out in v1, people don't like clock names to end in "clk".
Just call this "sec".


> +       qusb2p_hstx_trim: hstx-trim-primary@25b {
> +               reg = <0x25b 0x1>;
> +               bits = <1 3>;
> +       };
> +    };
> +
> +    &qfprom {
> +        vcc-supply = <&vreg_l11a_1p8>;
> +    };

Just fold the vcc-supply into the above node.  Note that your current
example refers to a phandle that doesn't exist.


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

For your edification, I have placed a patch that fixes all my own
review feedback (and passes dt_binding_check) at:

https://crrev.com/c/2243853

Please either fold this into your next patch or provide comments about
why you aren't taking one of my suggestions.

-Doug

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

[Doug Anderson]

Hi,

On Fri, Jun 12, 2020 at 2:59 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Thu, Jun 11, 2020 at 2:49 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.yaml          | 52 ++++++++++++++++++++++
> >  1 file changed, 52 insertions(+)
>
> Overall comment: I reviewed your v1 series and so I'm obviously
> interested in your series.  Please CC me on future versions.
>
> I would also note that, since this is relevant to Qualcomm SoCs that
> you probably should be CCing "linux-arm-msm@vger.kernel.org" on your
> series.
>
>
> >  create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> > new file mode 100644
> > index 0000000..7c8fc31
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> > @@ -0,0 +1,52 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/nvmem/qfprom.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,qfprom
>
> As per discussion in patch #1, I believe SoC compatible should be here
> too in case it is ever needed.  This is standard practice for dts
> files for IP blocks embedded in an SoC.  AKA, this should be:
>
>     items:
>       - enum:
>           - qcom,apq8064-qfprom
>           - qcom,apq8084-qfprom
>           - qcom,msm8974-qfprom
>           - qcom,msm8916-qfprom
>           - qcom,msm8996-qfprom
>           - qcom,msm8998-qfprom
>           - qcom,qcs404-qfprom
>           - qcom,sc7180-qfprom
>           - qcom,sdm845-qfprom
>       - const: qcom,qfprom
>
> NOTE: old SoCs won't have both of these and thus they will get flagged
> with "dtbs_check", but I believe that's fine (Rob can correct me if
> I'm wrong).  The code should still work OK if the SoC isn't there but
> it would be good to fix old dts files to have the SoC specific string
> too.
>
>
> > +
> > +  reg:
> > +    maxItems: 3
>
> Please address feedback feedback on v1.  If you disagree with my
> feedback it's OK to say so (I make no claims of being always right),
> but silently ignoring my feedback and sending the next version doesn't
> make me feel like it's a good use of my time to keep reviewing your
> series.  Specifically I suggested that you actually add descriptions
> rather than just putting "maxItems: 3".
>
> With all that has been discussed, I think the current best thing to
> put there is:
>
>     # If the QFPROM is read-only OS image then only the corrected region
>     # needs to be provided.  If the QFPROM is writable then all 3 regions
>     # must be provided.
>     oneOf:
>       - items:
>           - description: The start of the corrected region.
>       - items:
>           - description: The start of the raw region.
>           - description: The start of the config region.
>           - description: The start of the corrected region.

To address some of Srinivas's comments, I think you should actually
add the 4th range in here too:

- description: The start of the security control region.

That allows you to read 0x6000 and get the major/minor/step properly.


I will also note that as I've started reviewing the driver code it'll
probably make everyone's life easiest if you keep the "corrected"
region first, even if it's not numerically first.


I've updated my FIXUP patch again.  I might notice more comments as I
review the driver more, but we'll see...

-Doug

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

[Doug Anderson]

Hi,

On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> 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    | 10 ++++++++--
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-idp.dts b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> index 4e9149d..2a9224e 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
> @@ -287,6 +287,10 @@
>         };
>  };
>
> +&qfprom {
> +       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 31b9217..20f3480 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
> @@ -498,9 +498,15 @@
>                         #power-domain-cells = <1>;
>                 };
>
> -               qfprom@784000 {
> +               qfprom: qfprom@780000 {

The dt schema checker claims that your node should actually be called:

eeprom|efuse|nvram

So you probably want the above to actually be:

qfprom: efuse@780000

The label to the side doesn't matter and so it can stay qfprom--just
the node name itself is what the checker cares about.


>                         compatible = "qcom,qfprom";

As per my response in the bindings, this should be:

  "qcom,sc7180-qfprom", "qcom,qfprom"

...even if the driver only ever makes use of "qcom,qfprom" this
future-proofs us a bit.


> -                       reg = <0 0x00784000 0 0x8ff>;
> +                       reg = <0 0x00780000 0 0x7a0>,
> +                             <0 0x00782000 0 0x100>,
> +                             <0 0x00784000 0 0x8ff>;
> +                       reg-names = "raw", "conf", "corrected";

As per my response in the bindings, reg-names is discouraged so you
should remove and make it so that the driver doesn't need.


> +

It's hard to tell in email, but checkpatch yells above the above line:

    ERROR: trailing whitespace
    #53: FILE: arch/arm64/boot/dts/qcom/sc7180.dtsi:512:
    +^I^I^I$



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

As per the binding spec, clock name shouldn't end in "clk".

For your edification, I provided a patch to fix all my review feedback at:

https://crrev.com/c/2244933

Feel free to squash it into your next version.


-Doug

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

[Doug Anderson]

Hi,

On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>
> This patch adds support 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>
> ---

It makes all your code reviewers much happier (and much more likely to
take the time to review your patches) if you include a changelog with
what changed from one version to the next.  If you would like some
help maintaining such a thing, might I suggest "patman":

https://gitlab.denx.de/u-boot/u-boot/-/blob/master/tools/patman/README

...pay no mind that it's hosted in the U-Boot repo--it's really quite
a great tool.


>  drivers/nvmem/Kconfig  |   1 +
>  drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 385 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index d7b7f6d..623d59e 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -117,6 +117,7 @@ config QCOM_QFPROM
>         help
>           Say y here to enable QFPROM support. The QFPROM provides access
>           functions for QFPROM data to rest of the drivers via nvmem interface.
> +         And this driver provides access QTI qfprom efuse via nvmem interface.

I'm not sure it was necessary to add that line, but I won't object if
you/others really like it.


>           This driver can also be built as a module. If so, the module
>           will be called nvmem_qfprom.
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 8a91717..312318c 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c

You've still mostly not addressed most of the review feedback I've now
given you 3 times.  Rather than repeating comments, I have simply
provided a patch that makes the driver into a state that I'm happy
with:

https://crrev.com/c/2244932

Rough summary:

* There should be no reason to provide "reset" values for things.  For
anything that you change for fuse blowing, just save and restore
after.

* Use the major/minor version read from 0x6000 to pick the parameters
to use.  Please double-check that I got this right.

* Reading should still read "corrected", not "raw".  Added a sysfs
knob to allow you to read "raw", though.

* Simplified the SoC data structure.

* No need for quite so many levels of abstraction for setting clocks /
regulator.

* Don't set regulator voltage.  Rely on device tree to make sure it's right.

* Properly undo things in the case of failure.

* Don't just keep enabling the regulator over and over again.

* Enable / disable the clock each time; now we don't need a .remove
function and yet we still don't leave the clock enabled/prepared.

* Polling every 100 us but timing out in 10 us didn't make sense.
Swap those.  Also no reason for 100 us to be SoC specific.

* No need for reg-names.

* We shouldn't be creating two separate nvmem devices.


In general I'm happy to post my series to the list myself to get
review feedback.  For now I'm expecting that you can squash my changes
in and send the next version.


-Doug

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

[Srinivas Kandagatla]

I think Doug already covered most of the comments and his fixes seems be 
in right direction.

On 11/06/2020 10:48, Ravi Kumar Bokka wrote:
> +	qfpraw = platform_get_resource_byname(pdev, IORESOURCE_MEM, "raw");
> +
> +	priv->qfpraw = devm_ioremap_resource(dev, qfpraw);
> +	if (IS_ERR(priv->qfpraw)) {

General comment for up-streaming is that your patch should not break 
whats in mainline, Your patch is totally ignoring!! Please be mindful 
while doing changes to drivers which are used by other platforms.

--srini
> +		ret = PTR_ERR(priv->qfpraw);
> +		goto err;
> +	}
> +
> +	qfpconf = platform_get_resource_byname(pdev, IORESOURCE_MEM, "conf");
> +
> +	priv->qfpconf = devm_ioremap_resource(dev, qfpconf);
> +	if (IS_ERR(priv->qfpconf)) {
> +		ret = PTR_ERR(priv->qfpconf);
> +		goto err;
> +	}
> +
> +	qfpcorrected = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						    "corrected");
> +
> +	priv->qfpcorrected = devm_ioremap_resource(dev, qfpcorrected);
> +	if (IS_ERR(priv->qfpcorrected)) {
> +		ret = PTR_ERR(priv->qfpcorrected);
> +		goto err;
> +	}

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

[Srinivas Kandagatla]

On 12/06/2020 22:59, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 11, 2020 at 2:49 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.yaml          | 52 ++++++++++++++++++++++
>>   1 file changed, 52 insertions(+)
> 
> Overall comment: I reviewed your v1 series and so I'm obviously
> interested in your series.  Please CC me on future versions.
> 
> I would also note that, since this is relevant to Qualcomm SoCs that
> you probably should be CCing "linux-arm-msm@vger.kernel.org" on your
> series.
> 
> 
>>   create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
>> new file mode 100644
>> index 0000000..7c8fc31
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
>> @@ -0,0 +1,52 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/nvmem/qfprom.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,qfprom
> 
> As per discussion in patch #1, I believe SoC compatible should be here
> too in case it is ever needed.  This is standard practice for dts
> files for IP blocks embedded in an SoC.  AKA, this should be:
> 
>      items:
>        - enum:
>            - qcom,apq8064-qfprom
>            - qcom,apq8084-qfprom
>            - qcom,msm8974-qfprom
>            - qcom,msm8916-qfprom
>            - qcom,msm8996-qfprom
>            - qcom,msm8998-qfprom
>            - qcom,qcs404-qfprom
>            - qcom,sc7180-qfprom
>            - qcom,sdm845-qfprom


Above is not required for now in this patchset, as we can attach data at 
runtime specific to version of the qfprom.

This can be added when required!

>        - const: qcom,qfprom
> 
> NOTE: old SoCs won't have both of these and thus they will get flagged
> with "dtbs_check", but I believe that's fine (Rob can correct me if
> I'm wrong).  The code should still work OK if the SoC isn't there but
> it would be good to fix old dts files to have the SoC specific string
> too.
> 
> 
>> +
>> +  reg:
>> +    maxItems: 3
> 
> Please address feedback feedback on v1.  If you disagree with my
> feedback it's OK to say so (I make no claims of being always right),
> but silently ignoring my feedback and sending the next version doesn't
> make me feel like it's a good use of my time to keep reviewing your
> series.  Specifically I suggested that you actually add descriptions
> rather than just putting "maxItems: 3".
> 
> With all that has been discussed, I think the current best thing to
> put there is:
> 
>      # If the QFPROM is read-only OS image then only the corrected region
>      # needs to be provided.  If the QFPROM is writable then all 3 regions
>      # must be provided.
>      oneOf:
>        - items:
>            - description: The start of the corrected region.
>        - items:
>            - description: The start of the raw region.
>            - description: The start of the config region.
>            - description: The start of the corrected region.
> 
>> +
> 
> You missed a bunch of things that you should document:
> 
>    # Clocks must be provided if QFPROM is writable from the OS image.
>    clocks:
>      maxItems: 1
>    clock-names:
>      const: sec
> 
>    # Supply reference must be provided if QFPROM is writable from the OS image.
>    vcc-supply:
>      description: Our power supply.
> 
>    # Needed if any child nodes are present.
>    "#address-cells":
>      const: 1
>    "#size-cells":
>      const: 1
> 
>> +required:
>> +   - compatible
>> +   - reg
>> +   - reg-names
> 
> reg-names is discouraged.  Please remove.  I always point people here
> as a reference:
> 
> https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
> 
> You can just figure out whether there are 3 register fields or 1 register field.

Am not sure if I understand this correctly, reg-names are very useful in 
this particular case as we are dealing with multiple memory ranges with 
holes. I agree with not having this for cases where we have only one 
resource.

But having the ordering in DT without proper names associated with it 
seems fragile to me! And it makes very difficult to debug issues with 
wrong resource ordering in DT.

Rob, Is this the guidance for new bindings?

I have not seen any strong suggestion or guidance either in
 
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/resource-names.txt?h=v5.8-rc1 
  Or in ./drivers/of/address.c

Am I missing anything here?

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

[Srinivas Kandagatla]

On 13/06/2020 21:33, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 11, 2020 at 2:49 AM Ravi Kumar Bokka <rbokka@codeaurora.org> wrote:
>>
>> This patch adds support 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>
>> ---
> 
> It makes all your code reviewers much happier (and much more likely to
> take the time to review your patches) if you include a changelog with
> what changed from one version to the next.  If you would like some
> help maintaining such a thing, might I suggest "patman":
> 
> https://gitlab.denx.de/u-boot/u-boot/-/blob/master/tools/patman/README
> 
> ...pay no mind that it's hosted in the U-Boot repo--it's really quite
> a great tool.
> 
> 
>>   drivers/nvmem/Kconfig  |   1 +
>>   drivers/nvmem/qfprom.c | 405 ++++++++++++++++++++++++++++++++++++++++++++++---
>>   2 files changed, 385 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index d7b7f6d..623d59e 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -117,6 +117,7 @@ config QCOM_QFPROM
>>          help
>>            Say y here to enable QFPROM support. The QFPROM provides access
>>            functions for QFPROM data to rest of the drivers via nvmem interface.
>> +         And this driver provides access QTI qfprom efuse via nvmem interface.
> 
> I'm not sure it was necessary to add that line, but I won't object if
> you/others really like it.
> 

NAK from my side!

> 
>>            This driver can also be built as a module. If so, the module
>>            will be called nvmem_qfprom.
>> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
>> index 8a91717..312318c 100644
>> --- a/drivers/nvmem/qfprom.c
>> +++ b/drivers/nvmem/qfprom.c
> 
> You've still mostly not addressed most of the review feedback I've now
> given you 3 times.  Rather than repeating comments, I have simply
> provided a patch that makes the driver into a state that I'm happy
> with:
> 
> https://crrev.com/c/2244932
> 
> Rough summary:
> 
> * There should be no reason to provide "reset" values for things.  For
> anything that you change for fuse blowing, just save and restore
> after.
> 
> * Use the major/minor version read from 0x6000 to pick the parameters
> to use.  Please double-check that I got this right.
> 
> * Reading should still read "corrected", not "raw".  Added a sysfs
> knob to allow you to read "raw", though.

We could create an additional nvmem read-only provider in future if 
required to read raw!.

> 
> * Simplified the SoC data structure.
> 
> * No need for quite so many levels of abstraction for setting clocks /
> regulator.
> 
> * Don't set regulator voltage.  Rely on device tree to make sure it's right.
> 
> * Properly undo things in the case of failure.
> 
> * Don't just keep enabling the regulator over and over again.
> 
> * Enable / disable the clock each time; now we don't need a .remove
> function and yet we still don't leave the clock enabled/prepared.
> 
> * Polling every 100 us but timing out in 10 us didn't make sense.
> Swap those.  Also no reason for 100 us to be SoC specific.
> 
> * No need for reg-names.
> 
> * We shouldn't be creating two separate nvmem devices.
> 
> 
> In general I'm happy to post my series to the list myself to get
> review feedback.  For now I'm expecting that you can squash my changes
> in and send the next version.
> 
> 
> -Doug
> 

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

[Rob Herring]

On Mon, Jun 15, 2020 at 4:44 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 12/06/2020 22:59, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 11, 2020 at 2:49 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.yaml          | 52 ++++++++++++++++++++++
> >>   1 file changed, 52 insertions(+)
> >
> > Overall comment: I reviewed your v1 series and so I'm obviously
> > interested in your series.  Please CC me on future versions.
> >
> > I would also note that, since this is relevant to Qualcomm SoCs that
> > you probably should be CCing "linux-arm-msm@vger.kernel.org" on your
> > series.
> >
> >
> >>   create mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.yaml
> >>
> >> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> >> new file mode 100644
> >> index 0000000..7c8fc31
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/nvmem/qfprom.yaml
> >> @@ -0,0 +1,52 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/nvmem/qfprom.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,qfprom
> >
> > As per discussion in patch #1, I believe SoC compatible should be here
> > too in case it is ever needed.  This is standard practice for dts
> > files for IP blocks embedded in an SoC.  AKA, this should be:
> >
> >      items:
> >        - enum:
> >            - qcom,apq8064-qfprom
> >            - qcom,apq8084-qfprom
> >            - qcom,msm8974-qfprom
> >            - qcom,msm8916-qfprom
> >            - qcom,msm8996-qfprom
> >            - qcom,msm8998-qfprom
> >            - qcom,qcs404-qfprom
> >            - qcom,sc7180-qfprom
> >            - qcom,sdm845-qfprom
>
>
> Above is not required for now in this patchset, as we can attach data at
> runtime specific to version of the qfprom.
>
> This can be added when required!
>
> >        - const: qcom,qfprom
> >
> > NOTE: old SoCs won't have both of these and thus they will get flagged
> > with "dtbs_check", but I believe that's fine (Rob can correct me if
> > I'm wrong).  The code should still work OK if the SoC isn't there but
> > it would be good to fix old dts files to have the SoC specific string
> > too.
> >
> >
> >> +
> >> +  reg:
> >> +    maxItems: 3
> >
> > Please address feedback feedback on v1.  If you disagree with my
> > feedback it's OK to say so (I make no claims of being always right),
> > but silently ignoring my feedback and sending the next version doesn't
> > make me feel like it's a good use of my time to keep reviewing your
> > series.  Specifically I suggested that you actually add descriptions
> > rather than just putting "maxItems: 3".
> >
> > With all that has been discussed, I think the current best thing to
> > put there is:
> >
> >      # If the QFPROM is read-only OS image then only the corrected region
> >      # needs to be provided.  If the QFPROM is writable then all 3 regions
> >      # must be provided.
> >      oneOf:
> >        - items:
> >            - description: The start of the corrected region.
> >        - items:
> >            - description: The start of the raw region.
> >            - description: The start of the config region.
> >            - description: The start of the corrected region.
> >
> >> +
> >
> > You missed a bunch of things that you should document:
> >
> >    # Clocks must be provided if QFPROM is writable from the OS image.
> >    clocks:
> >      maxItems: 1
> >    clock-names:
> >      const: sec
> >
> >    # Supply reference must be provided if QFPROM is writable from the OS image.
> >    vcc-supply:
> >      description: Our power supply.
> >
> >    # Needed if any child nodes are present.
> >    "#address-cells":
> >      const: 1
> >    "#size-cells":
> >      const: 1
> >
> >> +required:
> >> +   - compatible
> >> +   - reg
> >> +   - reg-names
> >
> > reg-names is discouraged.  Please remove.  I always point people here
> > as a reference:
> >
> > https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
> >
> > You can just figure out whether there are 3 register fields or 1 register field.
>
> Am not sure if I understand this correctly, reg-names are very useful in
> this particular case as we are dealing with multiple memory ranges with
> holes. I agree with not having this for cases where we have only one
> resource.

1 or 3 doesn't sound like a case with holes.

> But having the ordering in DT without proper names associated with it
> seems fragile to me! And it makes very difficult to debug issues with
> wrong resource ordering in DT.
>
> Rob, Is this the guidance for new bindings?

This has *always* been the guidance since *-names were added.

> I have not seen any strong suggestion or guidance either in
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/resource-names.txt?h=v5.8-rc1

The key word is 'supplemental'. Perhaps that could be clearer, but DT
always required a defined order and the supplemental information
doesn't throw that out.

>   Or in ./drivers/of/address.c

How could it? Order is defined by the specific binding.

>
> Am I missing anything here?

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

[Doug Anderson]

Hi,

On Mon, Jun 15, 2020 at 3:44 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> >> +properties:
> >> +  compatible:
> >> +    enum:
> >> +      - qcom,qfprom
> >
> > As per discussion in patch #1, I believe SoC compatible should be here
> > too in case it is ever needed.  This is standard practice for dts
> > files for IP blocks embedded in an SoC.  AKA, this should be:
> >
> >      items:
> >        - enum:
> >            - qcom,apq8064-qfprom
> >            - qcom,apq8084-qfprom
> >            - qcom,msm8974-qfprom
> >            - qcom,msm8916-qfprom
> >            - qcom,msm8996-qfprom
> >            - qcom,msm8998-qfprom
> >            - qcom,qcs404-qfprom
> >            - qcom,sc7180-qfprom
> >            - qcom,sdm845-qfprom
>
>
> Above is not required for now in this patchset, as we can attach data at
> runtime specific to version of the qfprom.
>
> This can be added when required!

I'm OK with leaving off for this patchset.  After we get everything
landed for blowing fuses then I can send out a separate patch where we
can debate the merits of adding the SoC-specific compatible strings.
:-)  Sound good?

-Doug

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

[Srinivas Kandagatla]

On 15/06/2020 15:27, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jun 15, 2020 at 3:44 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>>> +properties:
>>>> +  compatible:
>>>> +    enum:
>>>> +      - qcom,qfprom
>>>
>>> As per discussion in patch #1, I believe SoC compatible should be here
>>> too in case it is ever needed.  This is standard practice for dts
>>> files for IP blocks embedded in an SoC.  AKA, this should be:
>>>
>>>       items:
>>>         - enum:
>>>             - qcom,apq8064-qfprom
>>>             - qcom,apq8084-qfprom
>>>             - qcom,msm8974-qfprom
>>>             - qcom,msm8916-qfprom
>>>             - qcom,msm8996-qfprom
>>>             - qcom,msm8998-qfprom
>>>             - qcom,qcs404-qfprom
>>>             - qcom,sc7180-qfprom
>>>             - qcom,sdm845-qfprom
>>
>>
>> Above is not required for now in this patchset, as we can attach data at
>> runtime specific to version of the qfprom.
>>
>> This can be added when required!
> 
> I'm OK with leaving off for this patchset.  After we get everything
> landed for blowing fuses then I can send out a separate patch where we
> can debate the merits of adding the SoC-specific compatible strings.
> :-)  Sound good?

Sounds good!

--srini

> 
> -Doug
> 

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

[Doug Anderson]

Hi,

On Mon, Jun 15, 2020 at 3:44 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> > * Reading should still read "corrected", not "raw".  Added a sysfs
> > knob to allow you to read "raw", though.
>
> We could create an additional nvmem read-only provider in future if
> required to read raw!.

Makes sense.  For now module parameter (which can be tweaked from
sysfs) seemed like at least an easy to get access to the raw values
for testing.  Other than for checking that the driver works OK,
though, I'm under the impression that you should never read the raw
values back.

-Doug

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

[Doug Anderson]

Hi,

On Sat, Jun 13, 2020 at 1:33 PM Doug Anderson <dianders@chromium.org> wrote:
>
> In general I'm happy to post my series to the list myself to get
> review feedback.  For now I'm expecting that you can squash my changes
> in and send the next version.

I talked with Ravi offline and he suggested that I send out the v3, so
it's been posted.  For the cover letter, see:

https://lore.kernel.org/r/20200617145116.247432-1-dianders@chromium.org

-Doug