[PATCH v3 0/4] nvmem: qfprom: Patches for fuse blowing on Qualcomm SoCs

[PATCH v3 0/4] nvmem: qfprom: Patches for fuse blowing on Qualcomm SoCs

[Douglas Anderson]

This series enables blowing of fuses on Qualcomm SoCs by extending the
existing qfprom driver with write support.

A few notes:
- Though I don't have any firsthand knowledge of it, it's my
  understanding that these changes could be used on any Qualcomm SoC.
  However, it's likely not very useful on most boards because the
  bootloader protects against this.  Thus the write support here is
  likely only useful with a cooperating bootloader.
- Blowing fuses is truly a one-way process.  If you mess around with
  this and do something wrong you could irreparably brick your chip.
  You have been warned.

Versions 1 and 2 of this series were posted by Ravi Kumar Bokka.  I am
posting version 3 containing my changes / fixups with his consent.  I
have left authorship as Ravi but added my own Signed-off-by.

Changes in v3:
- Split conversion to yaml into separate patch new in v3.
- Use 'const' for compatible instead of a 1-entry enum.
- Changed filename to match compatible string.
- Add #address-cells and #size-cells to list of properties.
- Fixed up example.
- Add an extra reg range (at 0x6000 offset for SoCs checked)
- Define two options for reg: 1 item or 4 items.
- No reg-names.
- Add "clocks" and "clock-names" to list of properties.
- Clock is now "sec", not "secclk".
- Add "vcc-supply" to list of properties.
- Fixed up example.
- Don't provide "reset" value for things; just save/restore.
- Use the major/minor version read from 0x6000.
- Reading should still read "corrected", not "raw".
- Added a sysfs knob to allow you to read "raw" instead of "corrected"
- Simplified the SoC data structure.
- No need for quite so many levels of abstraction for 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
- Polling every 100 us but timing out in 10 us didn't make sense; swap.
- No reason for 100 us to be SoC specific.
- No need for reg-names.
- We shouldn't be creating two separate nvmem devices.
- Name is now 'efuse' to match what schema checker wants.
- Reorganized ranges to match driver/bindings changes.
- Added 4th range as per driver/binding changes.
- No more reg-names as per driver/binding changes.
- Clock name is now just "sec" as per driver/binding changes.

Ravi Kumar Bokka (4):
  dt-bindings: nvmem: qfprom: Convert to yaml
  dt-bindings: nvmem: Add properties needed for blowing fuses
  nvmem: qfprom: Add fuse blowing support
  arm64: dts: qcom: sc7180: Add properties to qfprom for fuse blowing

 .../bindings/nvmem/qcom,qfprom.yaml           |  86 +++++
 .../devicetree/bindings/nvmem/qfprom.txt      |  35 --
 arch/arm64/boot/dts/qcom/sc7180-idp.dts       |   4 +
 arch/arm64/boot/dts/qcom/sc7180.dtsi          |  10 +-
 drivers/nvmem/qfprom.c                        | 314 +++++++++++++++++-
 5 files changed, 401 insertions(+), 48 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
 delete mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.txt

-- 
2.27.0.290.gba653c62da-goog

[PATCH v3 1/4] dt-bindings: nvmem: qfprom: Convert to yaml

[Douglas Anderson]

From: Ravi Kumar Bokka <rbokka@codeaurora.org>

This switches the bindings over from txt to yaml.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Split conversion to yaml into separate patch new in v3.
- Use 'const' for compatible instead of a 1-entry enum.
- Changed filename to match compatible string.
- Add #address-cells and #size-cells to list of properties.
- Fixed up example.

 .../bindings/nvmem/qcom,qfprom.yaml           | 45 +++++++++++++++++++
 .../devicetree/bindings/nvmem/qfprom.txt      | 35 ---------------
 2 files changed, 45 insertions(+), 35 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
 delete mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.txt

diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
new file mode 100644
index 000000000000..5efa5e7c4d81
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/qcom,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:
+    const: qcom,qfprom
+
+  reg:
+    items:
+      - description: The corrected region.
+
+  # Needed if any child nodes are present.
+  "#address-cells":
+    const: 1
+  "#size-cells":
+    const: 1
+
+required:
+   - compatible
+   - reg
+
+examples:
+  - |
+    efuse@784000 {
+      compatible = "qcom,qfprom";
+      reg = <0 0x00784000 0 0x8ff>;
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      hstx-trim-primary@1eb {
+        reg = <0x1eb 0x1>;
+        bits = <1 4>;
+      };
+    };
diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.txt b/Documentation/devicetree/bindings/nvmem/qfprom.txt
deleted file mode 100644
index 26fe878d5c86..000000000000
--- a/Documentation/devicetree/bindings/nvmem/qfprom.txt
+++ /dev/null
@@ -1,35 +0,0 @@
-= Qualcomm QFPROM device tree bindings =
-
-This binding is intended to represent QFPROM which is found in most QCOM SOCs.
-
-Required properties:
-- compatible: should be "qcom,qfprom"
-- reg: Should contain registers location and length
-
-= Data cells =
-Are child nodes of qfprom, bindings of which as described in
-bindings/nvmem/nvmem.txt
-
-Example:
-
-	qfprom: qfprom@700000 {
-		compatible 	= "qcom,qfprom";
-		reg		= <0x00700000 0x8000>;
-		...
-		/* Data cells */
-		tsens_calibration: calib@404 {
-			reg = <0x4404 0x10>;
-		};
-	};
-
-
-= Data consumers =
-Are device nodes which consume nvmem data cells.
-
-For example:
-
-	tsens {
-		...
-		nvmem-cells = <&tsens_calibration>;
-		nvmem-cell-names = "calibration";
-	};
-- 
2.27.0.290.gba653c62da-goog

[PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Douglas Anderson]

From: Ravi Kumar Bokka <rbokka@codeaurora.org>

On some systems it's possible to actually blow the fuses in the qfprom
from the kernel.  Add properties to support that.

NOTE: Whether this is possible depends on the BIOS settings and
whether the kernel has permissions here, so not all boards will be
able to blow fuses in the kernel.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Add an extra reg range (at 0x6000 offset for SoCs checked)
- Define two options for reg: 1 item or 4 items.
- No reg-names.
- Add "clocks" and "clock-names" to list of properties.
- Clock is now "sec", not "secclk".
- Add "vcc-supply" to list of properties.
- Fixed up example.

 .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
index 5efa5e7c4d81..b195212c6193 100644
--- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
+++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
@@ -17,8 +17,27 @@ properties:
     const: qcom,qfprom
 
   reg:
-    items:
-      - description: The corrected region.
+    # If the QFPROM is read-only OS image then only the corrected region
+    # needs to be provided.  If the QFPROM is writable then all 4 regions
+    # must be provided.
+    oneOf:
+      - items:
+          - description: The corrected region.
+      - items:
+          - description: The corrected region.
+          - description: The raw region.
+          - description: The config region.
+          - description: The security control region.
+
+  # Clock 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":
@@ -31,6 +50,28 @@ required:
    - reg
 
 examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    efuse@784000 {
+      compatible = "qcom,qfprom";
+      reg = <0 0x00784000 0 0x8ff>,
+            <0 0x00780000 0 0x7a0>,
+            <0 0x00782000 0 0x100>,
+            <0 0x00786000 0 0x1fff>;
+      clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+      clock-names = "sec";
+      #address-cells = <1>;
+      #size-cells = <1>;
+
+      vcc-supply = <&vreg_l11a_1p8>;
+
+      hstx-trim-primary@25b {
+        reg = <0x25b 0x1>;
+        bits = <1 3>;
+      };
+    };
+
   - |
     efuse@784000 {
       compatible = "qcom,qfprom";
-- 
2.27.0.290.gba653c62da-goog

[PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support

[Douglas Anderson]

From: Ravi Kumar Bokka <rbokka@codeaurora.org>

This patch adds support for blowing fuses to the qfprom driver if the
required properties are defined in the device tree.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
Please double-check that I got the major/minor version logic right
here.  I don't have documentation for this, but Srinivas mentioned
that it was at address 0x6000 and I happened to find an "8" and a "7"
on sc7180 so I assumed that was the major and minor version.

Changes in v3:
- Don't provide "reset" value for things; just save/restore.
- Use the major/minor version read from 0x6000.
- Reading should still read "corrected", not "raw".
- Added a sysfs knob to allow you to read "raw" instead of "corrected"
- Simplified the SoC data structure.
- No need for quite so many levels of abstraction for 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
- Polling every 100 us but timing out in 10 us didn't make sense; swap.
- No reason for 100 us to be SoC specific.
- No need for reg-names.
- We shouldn't be creating two separate nvmem devices.

 drivers/nvmem/qfprom.c | 314 +++++++++++++++++++++++++++++++++++++++--
 1 file changed, 303 insertions(+), 11 deletions(-)

diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
index 8a91717600be..486202860f84 100644
--- a/drivers/nvmem/qfprom.c
+++ b/drivers/nvmem/qfprom.c
@@ -3,57 +3,349 @@
  * 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/platform_device.h>
+#include <linux/regulator/consumer.h>
+
+/* Blow timer clock frequency in Mhz */
+#define QFPROM_BLOW_TIMER_OFFSET 0x03c
+
+/* Amount of time required to hold charge to blow fuse in micro-seconds */
+#define QFPROM_FUSE_BLOW_POLL_US	10
+#define QFPROM_FUSE_BLOW_TIMEOUT_US	100
+
+#define QFPROM_BLOW_STATUS_OFFSET	0x048
+#define QFPROM_BLOW_STATUS_BUSY		0x1
+#define QFPROM_BLOW_STATUS_READY	0x0
+
+#define QFPROM_ACCEL_OFFSET		0x044
+
+#define QFPROM_VERSION_OFFSET		0x0
+#define QFPROM_MAJOR_VERSION_SHIFT	28
+#define QFPROM_MAJOR_VERSION_MASK	0xf
+#define QFPROM_MINOR_VERSION_SHIFT	16
+#define QFPROM_MINOR_VERSION_MASK	0xf
+
+static bool read_raw_data;
+module_param(read_raw_data, bool, 0644);
+MODULE_PARM_DESC(read_raw_data, "Read raw instead of corrected data");
 
+/**
+ * struct qfprom_soc_data - config that varies from SoC to SoC.
+ *
+ * @accel_value:             Should contain qfprom accel value.
+ * @qfprom_blow_timer_value: The timer value of qfprom when doing efuse blow.
+ * @qfprom_blow_set_freq:    The frequency required to set when we start the
+ *                           fuse blowing.
+ */
+struct qfprom_soc_data {
+	u32 accel_value;
+	u32 qfprom_blow_timer_value;
+	u32 qfprom_blow_set_freq;
+};
+
+/**
+ * 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.
+ * @qfpsecurity:  iomapped memory space for qfprom security control space.
+ * @dev:          qfprom device structure.
+ * @secclk:       Clock supply.
+ * @vcc:          Regulator supply.
+ * @soc_data:     Data that for things that varies from SoC to SoC.
+ */
 struct qfprom_priv {
-	void __iomem *base;
+	void __iomem *qfpraw;
+	void __iomem *qfpconf;
+	void __iomem *qfpcorrected;
+	void __iomem *qfpsecurity;
+	struct device *dev;
+	struct clk *secclk;
+	struct regulator *vcc;
+	const struct qfprom_soc_data *soc_data;
+};
+
+/**
+ * struct qfprom_touched_values - saved values to restore after blowing
+ *
+ * @clk_rate: The rate the clock was at before blowing.
+ * @accel_val: The value of the accel reg before blowing.
+ * @timer_val: The value of the timer before blowing.
+ */
+struct qfprom_touched_values {
+	unsigned long clk_rate;
+	u32 accel_val;
+	u32 timer_val;
 };
 
+/**
+ * qfprom_disable_fuse_blowing() - Undo enabling of fuse blowing.
+ * @priv: Our driver data.
+ * @old:  The data that was stashed from before fuse blowing.
+ *
+ * Resets the value of the blow timer, accel register and the clock
+ * and voltage settings.
+ *
+ * Prints messages if there are errors but doesn't return an error code
+ * since there's not much we can do upon failure.
+ */
+static void qfprom_disable_fuse_blowing(const struct qfprom_priv *priv,
+					const struct qfprom_touched_values *old)
+{
+	int ret;
+
+	ret = regulator_disable(priv->vcc);
+	if (ret)
+		dev_warn(priv->dev, "Failed to disable regulator (ignoring)\n");
+
+	ret = clk_set_rate(priv->secclk, old->clk_rate);
+	if (ret)
+		dev_warn(priv->dev,
+			 "Failed to set clock rate for disable (ignoring)\n");
+
+	clk_disable_unprepare(priv->secclk);
+
+	writel(old->timer_val, priv->qfpconf + QFPROM_BLOW_TIMER_OFFSET);
+	writel(old->accel_val, priv->qfpconf + QFPROM_ACCEL_OFFSET);
+}
+
+/**
+ * qfprom_enable_fuse_blowing() - Enable fuse blowing.
+ * @priv: Our driver data.
+ * @old:  We'll stash stuff here to use when disabling.
+ *
+ * Sets the value of the blow timer, accel register and the clock
+ * and voltage settings.
+ *
+ * Prints messages if there are errors so caller doesn't need to.
+ *
+ * Return: 0 or -err.
+ */
+static int qfprom_enable_fuse_blowing(const struct qfprom_priv *priv,
+				      struct qfprom_touched_values *old)
+{
+	int ret;
+
+	ret = clk_prepare_enable(priv->secclk);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable clock\n");
+		return ret;
+	}
+
+	old->clk_rate = clk_get_rate(priv->secclk);
+	ret = clk_set_rate(priv->secclk, priv->soc_data->qfprom_blow_set_freq);
+	if (ret) {
+		dev_err(priv->dev, "Failed to set clock rate for enable\n");
+		goto err_clk_prepared;
+	}
+
+	ret = regulator_enable(priv->vcc);
+	if (ret) {
+		dev_err(priv->dev, "Failed to enable regulator\n");
+		goto err_clk_rate_set;
+	}
+
+	old->timer_val = readl(priv->qfpconf + QFPROM_BLOW_TIMER_OFFSET);
+	old->accel_val = readl(priv->qfpconf + QFPROM_ACCEL_OFFSET);
+	writel(priv->soc_data->qfprom_blow_timer_value,
+	       priv->qfpconf + QFPROM_BLOW_TIMER_OFFSET);
+	writel(priv->soc_data->accel_value,
+	       priv->qfpconf + QFPROM_ACCEL_OFFSET);
+
+	return 0;
+
+err_clk_rate_set:
+	clk_set_rate(priv->secclk, old->clk_rate);
+err_clk_prepared:
+	clk_disable_unprepare(priv->secclk);
+	return ret;
+}
+
+/**
+ * qfprom_efuse_reg_write() - Write to fuses.
+ * @context: Our driver data.
+ * @reg:     The offset to write at.
+ * @_val:    Pointer to data to write.
+ * @bytes:   The number of bytes to write.
+ *
+ * Writes to fuses.  WARNING: THIS IS PERMANENT.
+ *
+ * Return: 0 or -err.
+ */
+static int qfprom_reg_write(void *context, unsigned int reg, void *_val,
+			    size_t bytes)
+{
+	struct qfprom_priv *priv = context;
+	struct qfprom_touched_values old;
+	int words = bytes / 4;
+	u32 *value = _val;
+	u32 blow_status;
+	int ret;
+	int i;
+
+	dev_dbg(priv->dev,
+		"Writing to raw qfprom region : %#010x of size: %zu\n",
+		reg, bytes);
+
+	/*
+	 * The hardware only allows us to write word at a time, but we can
+	 * read byte at a time.  Until the nvmem framework allows a separate
+	 * word_size and stride for reading vs. writing, we'll enforce here.
+	 */
+	if (bytes % 4) {
+		dev_err(priv->dev,
+			"%zu is not an integral number of words\n", bytes);
+		return -EINVAL;
+	}
+	if (reg % 4) {
+		dev_err(priv->dev,
+			"Invalid offset: %#x.  Must be word aligned\n", reg);
+		return -EINVAL;
+	}
+
+	ret = qfprom_enable_fuse_blowing(priv, &old);
+	if (ret)
+		return ret;
+
+	ret = readl_relaxed_poll_timeout(
+		priv->qfpconf + QFPROM_BLOW_STATUS_OFFSET,
+		blow_status, blow_status == QFPROM_BLOW_STATUS_READY,
+		QFPROM_FUSE_BLOW_POLL_US, QFPROM_FUSE_BLOW_TIMEOUT_US);
+
+	if (ret) {
+		dev_err(priv->dev,
+			"Timeout waiting for initial ready; aborting.\n");
+		goto exit_enabled_fuse_blowing;
+	}
+
+	for (i = 0; i < words; i++)
+		writel(value[i], priv->qfpraw + reg + (i * 4));
+
+	ret = readl_relaxed_poll_timeout(
+		priv->qfpconf + QFPROM_BLOW_STATUS_OFFSET,
+		blow_status, blow_status == QFPROM_BLOW_STATUS_READY,
+		QFPROM_FUSE_BLOW_POLL_US, QFPROM_FUSE_BLOW_TIMEOUT_US);
+
+	/* Give an error, but not much we can do in this case */
+	if (ret)
+		dev_err(priv->dev, "Timeout waiting for finish.\n");
+
+exit_enabled_fuse_blowing:
+	qfprom_disable_fuse_blowing(priv, &old);
+
+	return ret;
+}
+
 static int qfprom_reg_read(void *context,
 			unsigned int reg, void *_val, size_t bytes)
 {
 	struct qfprom_priv *priv = context;
 	u8 *val = _val;
 	int i = 0, words = bytes;
+	void __iomem *base = priv->qfpcorrected;
+
+	if (read_raw_data && priv->qfpraw)
+		base = priv->qfpraw;
 
 	while (words--)
-		*val++ = readb(priv->base + reg + i++);
+		*val++ = readb(base + reg + i++);
 
 	return 0;
 }
 
-static struct nvmem_config econfig = {
-	.name = "qfprom",
-	.stride = 1,
-	.word_size = 1,
-	.reg_read = qfprom_reg_read,
+static const struct qfprom_soc_data qfprom_7_8_data = {
+	.accel_value = 0xD10,
+	.qfprom_blow_timer_value = 25,
+	.qfprom_blow_set_freq = 4800000,
 };
 
 static int qfprom_probe(struct platform_device *pdev)
 {
+	struct nvmem_config econfig = {
+		.name = "qfprom",
+		.stride = 1,
+		.word_size = 1,
+		.reg_read = qfprom_reg_read,
+	};
 	struct device *dev = &pdev->dev;
 	struct resource *res;
 	struct nvmem_device *nvmem;
 	struct qfprom_priv *priv;
+	int ret;
 
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
+	/* The corrected section is always provided */
 	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->qfpcorrected = devm_ioremap_resource(dev, res);
+	if (IS_ERR(priv->qfpcorrected))
+		return PTR_ERR(priv->qfpcorrected);
 
 	econfig.size = resource_size(res);
 	econfig.dev = dev;
 	econfig.priv = priv;
 
+	priv->dev = dev;
+
+	/*
+	 * If more than one region is provided then the OS has the ability
+	 * to write.
+	 */
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+	if (res) {
+		u32 version;
+		int major_version, minor_version;
+
+		priv->qfpraw = devm_ioremap_resource(dev, res);
+		if (IS_ERR(priv->qfpraw))
+			return PTR_ERR(priv->qfpraw);
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
+		priv->qfpconf = devm_ioremap_resource(dev, res);
+		if (IS_ERR(priv->qfpconf))
+			return PTR_ERR(priv->qfpconf);
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
+		priv->qfpsecurity = devm_ioremap_resource(dev, res);
+		if (IS_ERR(priv->qfpsecurity))
+			return PTR_ERR(priv->qfpsecurity);
+
+		version = readl(priv->qfpsecurity + QFPROM_VERSION_OFFSET);
+		major_version = (version >> QFPROM_MAJOR_VERSION_SHIFT) &
+				QFPROM_MAJOR_VERSION_MASK;
+		minor_version = (version >> QFPROM_MINOR_VERSION_SHIFT) &
+				QFPROM_MINOR_VERSION_MASK;
+
+		if (major_version == 7 && minor_version == 8)
+			priv->soc_data = &qfprom_7_8_data;
+
+		/* Only enable writing if we have SoC data. */
+		if (priv->soc_data)
+			econfig.reg_write = qfprom_reg_write;
+	}
+
+	priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
+	if (IS_ERR(priv->vcc))
+		return PTR_ERR(priv->vcc);
+
+	priv->secclk = devm_clk_get_optional(dev, "sec");
+	if (IS_ERR(priv->secclk)) {
+		ret = PTR_ERR(priv->secclk);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "sec error getting : %d\n", ret);
+		return ret;
+	}
+
 	nvmem = devm_nvmem_register(dev, &econfig);
 
 	return PTR_ERR_OR_ZERO(nvmem);
-- 
2.27.0.290.gba653c62da-goog

[PATCH v3 4/4] arm64: dts: qcom: sc7180: Add properties to qfprom for fuse blowing

[Douglas Anderson]

From: Ravi Kumar Bokka <rbokka@codeaurora.org>

This patch adds properties to the qfprom node to enable fuse blowing.

Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---

Changes in v3:
- Name is now 'efuse' to match what schema checker wants.
- Reorganized ranges to match driver/bindings changes.
- Added 4th range as per driver/binding changes.
- No more reg-names as per driver/binding changes.
- Clock name is now just "sec" as per driver/binding changes.

 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 4e9149d82d09..2a9224e2083f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-idp.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-idp.dts
@@ -287,6 +287,10 @@ vreg_bob: bob {
 	};
 };
 
+&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 31b9217bb5bf..d7f5e3d64b17 100644
--- a/arch/arm64/boot/dts/qcom/sc7180.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180.dtsi
@@ -498,9 +498,15 @@ gcc: clock-controller@100000 {
 			#power-domain-cells = <1>;
 		};
 
-		qfprom@784000 {
+		qfprom: efuse@784000 {
 			compatible = "qcom,qfprom";
-			reg = <0 0x00784000 0 0x8ff>;
+			reg = <0 0x00784000 0 0x8ff>,
+			      <0 0x00780000 0 0x7a0>,
+			      <0 0x00782000 0 0x100>,
+			      <0 0x00786000 0 0x1fff>;
+
+			clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
+			clock-names = "sec";
 			#address-cells = <1>;
 			#size-cells = <1>;
 
-- 
2.27.0.290.gba653c62da-goog

Re: [PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support

[Jeffrey Hugo]

On 6/17/2020 8:51 AM, Douglas Anderson wrote:
> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> 
> This patch adds support for blowing fuses to the qfprom driver if the
> required properties are defined in the device tree.
> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Please double-check that I got the major/minor version logic right
> here.  I don't have documentation for this, but Srinivas mentioned
> that it was at address 0x6000 and I happened to find an "8" and a "7"
> on sc7180 so I assumed that was the major and minor version.
> 
> Changes in v3:
> - Don't provide "reset" value for things; just save/restore.
> - Use the major/minor version read from 0x6000.
> - Reading should still read "corrected", not "raw".
> - Added a sysfs knob to allow you to read "raw" instead of "corrected"
> - Simplified the SoC data structure.
> - No need for quite so many levels of abstraction for 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
> - Polling every 100 us but timing out in 10 us didn't make sense; swap.
> - No reason for 100 us to be SoC specific.
> - No need for reg-names.
> - We shouldn't be creating two separate nvmem devices.
> 
>   drivers/nvmem/qfprom.c | 314 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 303 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 8a91717600be..486202860f84 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c
> @@ -3,57 +3,349 @@
>    * 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/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* Blow timer clock frequency in Mhz */
> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> +
> +/* Amount of time required to hold charge to blow fuse in micro-seconds */
> +#define QFPROM_FUSE_BLOW_POLL_US	10
> +#define QFPROM_FUSE_BLOW_TIMEOUT_US	100
> +
> +#define QFPROM_BLOW_STATUS_OFFSET	0x048
> +#define QFPROM_BLOW_STATUS_BUSY		0x1
> +#define QFPROM_BLOW_STATUS_READY	0x0
> +
> +#define QFPROM_ACCEL_OFFSET		0x044
> +
> +#define QFPROM_VERSION_OFFSET		0x0
> +#define QFPROM_MAJOR_VERSION_SHIFT	28
> +#define QFPROM_MAJOR_VERSION_MASK	0xf
> +#define QFPROM_MINOR_VERSION_SHIFT	16
> +#define QFPROM_MINOR_VERSION_MASK	0xf

Minor looks wrong.  Documentation says bits 27:16 are the minor version, 
and bits 15:0 are step.  I think your minor mask needs to be 0xfff.

-- 
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.

Re: [PATCH v3 1/4] dt-bindings: nvmem: qfprom: Convert to yaml

[Srinivas Kandagatla]

On 17/06/2020 15:51, Douglas Anderson wrote:
> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> 
> This switches the bindings over from txt to yaml.
> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Split conversion to yaml into separate patch new in v3.
> - Use 'const' for compatible instead of a 1-entry enum.
> - Changed filename to match compatible string.
> - Add #address-cells and #size-cells to list of properties.
> - Fixed up example.
> 
>   .../bindings/nvmem/qcom,qfprom.yaml           | 45 +++++++++++++++++++
>   .../devicetree/bindings/nvmem/qfprom.txt      | 35 ---------------
>   2 files changed, 45 insertions(+), 35 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>   delete mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.txt
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> new file mode 100644
> index 000000000000..5efa5e7c4d81
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> @@ -0,0 +1,45 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/qcom,qfprom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc, QFPROM Efuse bindings
> +
> +maintainers:
> +  - Ravi Kumar Bokka <rbokka@codeaurora.org>
> +

Am not sure this was intentional, but the old maintainer name is totally 
lost in this patch!

Please fix this!



> +allOf:
> +  - $ref: "nvmem.yaml#"
> +
> +properties:
> +  compatible:
> +    const: qcom,qfprom
> +
> +  reg:
> +    items:
> +      - description: The corrected region.
> +
> +  # Needed if any child nodes are present.
> +  "#address-cells":
> +    const: 1
> +  "#size-cells":
> +    const: 1
> +
> +required:
> +   - compatible
> +   - reg
> +
> +examples:
> +  - |
> +    efuse@784000 {
> +      compatible = "qcom,qfprom";
> +      reg = <0 0x00784000 0 0x8ff>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      hstx-trim-primary@1eb {
> +        reg = <0x1eb 0x1>;
> +        bits = <1 4>;
> +      };
> +    };
> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.txt b/Documentation/devicetree/bindings/nvmem/qfprom.txt
> deleted file mode 100644
> index 26fe878d5c86..000000000000
> --- a/Documentation/devicetree/bindings/nvmem/qfprom.txt
> +++ /dev/null
> @@ -1,35 +0,0 @@
> -= Qualcomm QFPROM device tree bindings =
> -
> -This binding is intended to represent QFPROM which is found in most QCOM SOCs.
> -
> -Required properties:
> -- compatible: should be "qcom,qfprom"
> -- reg: Should contain registers location and length
> -
> -= Data cells =
> -Are child nodes of qfprom, bindings of which as described in
> -bindings/nvmem/nvmem.txt
> -
> -Example:
> -
> -	qfprom: qfprom@700000 {
> -		compatible 	= "qcom,qfprom";
> -		reg		= <0x00700000 0x8000>;
> -		...
> -		/* Data cells */
> -		tsens_calibration: calib@404 {
> -			reg = <0x4404 0x10>;
> -		};
> -	};
> -
> -
> -= Data consumers =
> -Are device nodes which consume nvmem data cells.
> -
> -For example:
> -
> -	tsens {
> -		...
> -		nvmem-cells = <&tsens_calibration>;
> -		nvmem-cell-names = "calibration";
> -	};
> 

Re: [PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support

[Srinivas Kandagatla]

Thanks Doug,
Overall the patch is looking good, I have few minor comments below!

On 17/06/2020 15:51, Douglas Anderson wrote:
> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> 
> This patch adds support for blowing fuses to the qfprom driver if the
> required properties are defined in the device tree.
> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> Please double-check that I got the major/minor version logic right
> here.  I don't have documentation for this, but Srinivas mentioned
> that it was at address 0x6000 and I happened to find an "8" and a "7"
> on sc7180 so I assumed that was the major and minor version.
> 
> Changes in v3:
> - Don't provide "reset" value for things; just save/restore.
> - Use the major/minor version read from 0x6000.
> - Reading should still read "corrected", not "raw".
> - Added a sysfs knob to allow you to read "raw" instead of "corrected"
> - Simplified the SoC data structure.
> - No need for quite so many levels of abstraction for 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
> - Polling every 100 us but timing out in 10 us didn't make sense; swap.
> - No reason for 100 us to be SoC specific.
> - No need for reg-names.
> - We shouldn't be creating two separate nvmem devices.
> 
>   drivers/nvmem/qfprom.c | 314 +++++++++++++++++++++++++++++++++++++++--
>   1 file changed, 303 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> index 8a91717600be..486202860f84 100644
> --- a/drivers/nvmem/qfprom.c
> +++ b/drivers/nvmem/qfprom.c
> @@ -3,57 +3,349 @@
>    * 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/platform_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +/* Blow timer clock frequency in Mhz */
> +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> +
> +/* Amount of time required to hold charge to blow fuse in micro-seconds */
> +#define QFPROM_FUSE_BLOW_POLL_US	10
> +#define QFPROM_FUSE_BLOW_TIMEOUT_US	100
> +
> +#define QFPROM_BLOW_STATUS_OFFSET	0x048
> +#define QFPROM_BLOW_STATUS_BUSY		0x1
> +#define QFPROM_BLOW_STATUS_READY	0x0
> +
> +#define QFPROM_ACCEL_OFFSET		0x044
> +
> +#define QFPROM_VERSION_OFFSET		0x0
> +#define QFPROM_MAJOR_VERSION_SHIFT	28
> +#define QFPROM_MAJOR_VERSION_MASK	0xf
> +#define QFPROM_MINOR_VERSION_SHIFT	16
> +#define QFPROM_MINOR_VERSION_MASK	0xf

Using GENMASK here makes it much readable!

...

>   
>   static int qfprom_probe(struct platform_device *pdev)
>   {
> +	struct nvmem_config econfig = {
> +		.name = "qfprom",
> +		.stride = 1,
> +		.word_size = 1,
> +		.reg_read = qfprom_reg_read,
> +	};
>   	struct device *dev = &pdev->dev;
>   	struct resource *res;
>   	struct nvmem_device *nvmem;
>   	struct qfprom_priv *priv;
> +	int ret;
>   
>   	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>   	if (!priv)
>   		return -ENOMEM;
>   
> +	/* The corrected section is always provided */
>   	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->qfpcorrected = devm_ioremap_resource(dev, res);
> +	if (IS_ERR(priv->qfpcorrected))
> +		return PTR_ERR(priv->qfpcorrected);
>   
>   	econfig.size = resource_size(res);
>   	econfig.dev = dev;
>   	econfig.priv = priv;
>   
> +	priv->dev = dev;
> +
> +	/*
> +	 * If more than one region is provided then the OS has the ability
> +	 * to write.
> +	 */
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> +	if (res) {
> +		u32 version;
> +		int major_version, minor_version;
> +
> +		priv->qfpraw = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(priv->qfpraw))
> +			return PTR_ERR(priv->qfpraw);
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> +		priv->qfpconf = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(priv->qfpconf))
> +			return PTR_ERR(priv->qfpconf);
> +		res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> +		priv->qfpsecurity = devm_ioremap_resource(dev, res);
> +		if (IS_ERR(priv->qfpsecurity))
> +			return PTR_ERR(priv->qfpsecurity);
> +
> +		version = readl(priv->qfpsecurity + QFPROM_VERSION_OFFSET);
> +		major_version = (version >> QFPROM_MAJOR_VERSION_SHIFT) &
> +				QFPROM_MAJOR_VERSION_MASK;
> +		minor_version = (version >> QFPROM_MINOR_VERSION_SHIFT) &
> +				QFPROM_MINOR_VERSION_MASK;
> +
> +		if (major_version == 7 && minor_version == 8)
> +			priv->soc_data = &qfprom_7_8_data;
> +
> +		/* Only enable writing if we have SoC data. */
> +		if (priv->soc_data)
> +			econfig.reg_write = qfprom_reg_write;
> +	}
> +

<----------snip
> +	priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> +	if (IS_ERR(priv->vcc))
> +		return PTR_ERR(priv->vcc);
> +
> +	priv->secclk = devm_clk_get_optional(dev, "sec");
> +	if (IS_ERR(priv->secclk)) {
> +		ret = PTR_ERR(priv->secclk);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "sec error getting : %d\n", ret);
> +		return ret;
> +	}
> +
----------->
should you move both clk and regulator into the previous if (res) {} 
block? As I don't see them marked as optional for write cases.



>   	nvmem = devm_nvmem_register(dev, &econfig);
>   
>   	return PTR_ERR_OR_ZERO(nvmem);
> 

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Srinivas Kandagatla]

On 17/06/2020 15:51, Douglas Anderson wrote:
> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> 
> On some systems it's possible to actually blow the fuses in the qfprom
> from the kernel.  Add properties to support that.
> 
> NOTE: Whether this is possible depends on the BIOS settings and
> whether the kernel has permissions here, so not all boards will be
> able to blow fuses in the kernel.
> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Add an extra reg range (at 0x6000 offset for SoCs checked)
> - Define two options for reg: 1 item or 4 items.
> - No reg-names.
> - Add "clocks" and "clock-names" to list of properties.
> - Clock is now "sec", not "secclk".
> - Add "vcc-supply" to list of properties.
> - Fixed up example.
> 
>   .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
>   1 file changed, 43 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> index 5efa5e7c4d81..b195212c6193 100644
> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> @@ -17,8 +17,27 @@ properties:
>       const: qcom,qfprom
>   
>     reg:
> -    items:
> -      - description: The corrected region.
> +    # If the QFPROM is read-only OS image then only the corrected region
> +    # needs to be provided.  If the QFPROM is writable then all 4 regions
> +    # must be provided.
> +    oneOf:
> +      - items:
> +          - description: The corrected region.
> +      - items:
> +          - description: The corrected region.
> +          - description: The raw region.
> +          - description: The config region.
> +          - description: The security control region.
> +
> +  # Clock must be provided if QFPROM is writable from the OS image.
> +  clocks:
> +    maxItems: 1


> +  clock-names:
> +    const: sec

Do we need clock-names for just one clock here?

> +
> +  # 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":
> @@ -31,6 +50,28 @@ required:
>      - reg
>   
>   examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    efuse@784000 {
> +      compatible = "qcom,qfprom";
> +      reg = <0 0x00784000 0 0x8ff>,
> +            <0 0x00780000 0 0x7a0>,
> +            <0 0x00782000 0 0x100>,
> +            <0 0x00786000 0 0x1fff>;
> +      clocks = <&gcc GCC_SEC_CTRL_CLK_SRC>;
> +      clock-names = "sec";
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +
> +      vcc-supply = <&vreg_l11a_1p8>;
> +
> +      hstx-trim-primary@25b {
> +        reg = <0x25b 0x1>;
> +        bits = <1 3>;
> +      };
> +    };
> +
>     - |
>       efuse@784000 {
>         compatible = "qcom,qfprom";
> 

Re: [PATCH v3 1/4] dt-bindings: nvmem: qfprom: Convert to yaml

[Ravi Kumar Bokka (Temp)]

On 6/17/2020 8:48 PM, Srinivas Kandagatla wrote:
> 
> 
> On 17/06/2020 15:51, Douglas Anderson wrote:
>> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>
>> This switches the bindings over from txt to yaml.
>>
>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>> ---
>>
>> Changes in v3:
>> - Split conversion to yaml into separate patch new in v3.
>> - Use 'const' for compatible instead of a 1-entry enum.
>> - Changed filename to match compatible string.
>> - Add #address-cells and #size-cells to list of properties.
>> - Fixed up example.
>>
>>   .../bindings/nvmem/qcom,qfprom.yaml           | 45 +++++++++++++++++++
>>   .../devicetree/bindings/nvmem/qfprom.txt      | 35 ---------------
>>   2 files changed, 45 insertions(+), 35 deletions(-)
>>   create mode 100644 
>> Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>   delete mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.txt
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml 
>> b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>> new file mode 100644
>> index 000000000000..5efa5e7c4d81
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>> @@ -0,0 +1,45 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/nvmem/qcom,qfprom.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies Inc, QFPROM Efuse bindings
>> +
>> +maintainers:
>> +  - Ravi Kumar Bokka <rbokka@codeaurora.org>
>> +
> 
> Am not sure this was intentional, but the old maintainer name is totally 
> lost in this patch!
> 
> Please fix this!
> 

Hi Srinivas,
The existed qfprom dt-bindings in .txt format.
I will make it as it is to merge whole content in .yaml format once 
confirm all the parameters with this new driver changes.

> 
> 
>> +allOf:
>> +  - $ref: "nvmem.yaml#"
>> +
>> +properties:
>> +  compatible:
>> +    const: qcom,qfprom
>> +
>> +  reg:
>> +    items:
>> +      - description: The corrected region.
>> +
>> +  # Needed if any child nodes are present.
>> +  "#address-cells":
>> +    const: 1
>> +  "#size-cells":
>> +    const: 1
>> +
>> +required:
>> +   - compatible
>> +   - reg
>> +
>> +examples:
>> +  - |
>> +    efuse@784000 {
>> +      compatible = "qcom,qfprom";
>> +      reg = <0 0x00784000 0 0x8ff>;
>> +      #address-cells = <1>;
>> +      #size-cells = <1>;
>> +
>> +      hstx-trim-primary@1eb {
>> +        reg = <0x1eb 0x1>;
>> +        bits = <1 4>;
>> +      };
>> +    };
>> diff --git a/Documentation/devicetree/bindings/nvmem/qfprom.txt 
>> b/Documentation/devicetree/bindings/nvmem/qfprom.txt
>> deleted file mode 100644
>> index 26fe878d5c86..000000000000
>> --- a/Documentation/devicetree/bindings/nvmem/qfprom.txt
>> +++ /dev/null
>> @@ -1,35 +0,0 @@
>> -= Qualcomm QFPROM device tree bindings =
>> -
>> -This binding is intended to represent QFPROM which is found in most 
>> QCOM SOCs.
>> -
>> -Required properties:
>> -- compatible: should be "qcom,qfprom"
>> -- reg: Should contain registers location and length
>> -
>> -= Data cells =
>> -Are child nodes of qfprom, bindings of which as described in
>> -bindings/nvmem/nvmem.txt
>> -
>> -Example:
>> -
>> -    qfprom: qfprom@700000 {
>> -        compatible     = "qcom,qfprom";
>> -        reg        = <0x00700000 0x8000>;
>> -        ...
>> -        /* Data cells */
>> -        tsens_calibration: calib@404 {
>> -            reg = <0x4404 0x10>;
>> -        };
>> -    };
>> -
>> -
>> -= Data consumers =
>> -Are device nodes which consume nvmem data cells.
>> -
>> -For example:
>> -
>> -    tsens {
>> -        ...
>> -        nvmem-cells = <&tsens_calibration>;
>> -        nvmem-cell-names = "calibration";
>> -    };
>>

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

Re: [PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support

[Doug Anderson]

Hi,

On Wed, Jun 17, 2020 at 8:18 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> Thanks Doug,
> Overall the patch is looking good, I have few minor comments below!
>
> On 17/06/2020 15:51, Douglas Anderson wrote:
> > From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >
> > This patch adds support for blowing fuses to the qfprom driver if the
> > required properties are defined in the device tree.
> >
> > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> > Please double-check that I got the major/minor version logic right
> > here.  I don't have documentation for this, but Srinivas mentioned
> > that it was at address 0x6000 and I happened to find an "8" and a "7"
> > on sc7180 so I assumed that was the major and minor version.
> >
> > Changes in v3:
> > - Don't provide "reset" value for things; just save/restore.
> > - Use the major/minor version read from 0x6000.
> > - Reading should still read "corrected", not "raw".
> > - Added a sysfs knob to allow you to read "raw" instead of "corrected"
> > - Simplified the SoC data structure.
> > - No need for quite so many levels of abstraction for 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
> > - Polling every 100 us but timing out in 10 us didn't make sense; swap.
> > - No reason for 100 us to be SoC specific.
> > - No need for reg-names.
> > - We shouldn't be creating two separate nvmem devices.
> >
> >   drivers/nvmem/qfprom.c | 314 +++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 303 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/nvmem/qfprom.c b/drivers/nvmem/qfprom.c
> > index 8a91717600be..486202860f84 100644
> > --- a/drivers/nvmem/qfprom.c
> > +++ b/drivers/nvmem/qfprom.c
> > @@ -3,57 +3,349 @@
> >    * 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/platform_device.h>
> > +#include <linux/regulator/consumer.h>
> > +
> > +/* Blow timer clock frequency in Mhz */
> > +#define QFPROM_BLOW_TIMER_OFFSET 0x03c
> > +
> > +/* Amount of time required to hold charge to blow fuse in micro-seconds */
> > +#define QFPROM_FUSE_BLOW_POLL_US     10
> > +#define QFPROM_FUSE_BLOW_TIMEOUT_US  100
> > +
> > +#define QFPROM_BLOW_STATUS_OFFSET    0x048
> > +#define QFPROM_BLOW_STATUS_BUSY              0x1
> > +#define QFPROM_BLOW_STATUS_READY     0x0
> > +
> > +#define QFPROM_ACCEL_OFFSET          0x044
> > +
> > +#define QFPROM_VERSION_OFFSET                0x0
> > +#define QFPROM_MAJOR_VERSION_SHIFT   28
> > +#define QFPROM_MAJOR_VERSION_MASK    0xf
> > +#define QFPROM_MINOR_VERSION_SHIFT   16
> > +#define QFPROM_MINOR_VERSION_MASK    0xf
>
> Using GENMASK here makes it much readable!
>
> ...
>
> >
> >   static int qfprom_probe(struct platform_device *pdev)
> >   {
> > +     struct nvmem_config econfig = {
> > +             .name = "qfprom",
> > +             .stride = 1,
> > +             .word_size = 1,
> > +             .reg_read = qfprom_reg_read,
> > +     };
> >       struct device *dev = &pdev->dev;
> >       struct resource *res;
> >       struct nvmem_device *nvmem;
> >       struct qfprom_priv *priv;
> > +     int ret;
> >
> >       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> >       if (!priv)
> >               return -ENOMEM;
> >
> > +     /* The corrected section is always provided */
> >       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->qfpcorrected = devm_ioremap_resource(dev, res);
> > +     if (IS_ERR(priv->qfpcorrected))
> > +             return PTR_ERR(priv->qfpcorrected);
> >
> >       econfig.size = resource_size(res);
> >       econfig.dev = dev;
> >       econfig.priv = priv;
> >
> > +     priv->dev = dev;
> > +
> > +     /*
> > +      * If more than one region is provided then the OS has the ability
> > +      * to write.
> > +      */
> > +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> > +     if (res) {
> > +             u32 version;
> > +             int major_version, minor_version;
> > +
> > +             priv->qfpraw = devm_ioremap_resource(dev, res);
> > +             if (IS_ERR(priv->qfpraw))
> > +                     return PTR_ERR(priv->qfpraw);
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, 2);
> > +             priv->qfpconf = devm_ioremap_resource(dev, res);
> > +             if (IS_ERR(priv->qfpconf))
> > +                     return PTR_ERR(priv->qfpconf);
> > +             res = platform_get_resource(pdev, IORESOURCE_MEM, 3);
> > +             priv->qfpsecurity = devm_ioremap_resource(dev, res);
> > +             if (IS_ERR(priv->qfpsecurity))
> > +                     return PTR_ERR(priv->qfpsecurity);
> > +
> > +             version = readl(priv->qfpsecurity + QFPROM_VERSION_OFFSET);
> > +             major_version = (version >> QFPROM_MAJOR_VERSION_SHIFT) &
> > +                             QFPROM_MAJOR_VERSION_MASK;
> > +             minor_version = (version >> QFPROM_MINOR_VERSION_SHIFT) &
> > +                             QFPROM_MINOR_VERSION_MASK;
> > +
> > +             if (major_version == 7 && minor_version == 8)
> > +                     priv->soc_data = &qfprom_7_8_data;
> > +
> > +             /* Only enable writing if we have SoC data. */
> > +             if (priv->soc_data)
> > +                     econfig.reg_write = qfprom_reg_write;
> > +     }
> > +
>
> <----------snip
> > +     priv->vcc = devm_regulator_get(&pdev->dev, "vcc");
> > +     if (IS_ERR(priv->vcc))
> > +             return PTR_ERR(priv->vcc);
> > +
> > +     priv->secclk = devm_clk_get_optional(dev, "sec");
> > +     if (IS_ERR(priv->secclk)) {
> > +             ret = PTR_ERR(priv->secclk);
> > +             if (ret != -EPROBE_DEFER)
> > +                     dev_err(dev, "sec error getting : %d\n", ret);
> > +             return ret;
> > +     }
> > +
> ----------->
> should you move both clk and regulator into the previous if (res) {}
> block? As I don't see them marked as optional for write cases.

Sure, I'll move them on the next version.  I will note that the
current version actually works fine but I guess since there currently
isn't actually a user of the clock/regulator in the non-write case
then we don't need to make the calls...  Why does it work fine the way
it is too?

* regulator_get() will automatically create a "dummy" regulator if one
isn't specified in the device tree, so there's no harm in the call.
If you actually need to know if a regulator is there you need to call
regulator_get_optional()

* clk_get_optional() will return NULL if a clock isn't specified in
the device tree and clock framework is fine with you passing NULL for
enable/disable/prepare/unprepare.  If you actually need to know if a
clock is there you need to call clk_get().

Amazing but true that the "optional" variants of the regulator and
clock code are effectively opposites of each other.  :-P


-Doug

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Doug Anderson]

Hi,

On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 17/06/2020 15:51, Douglas Anderson wrote:
> > From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >
> > On some systems it's possible to actually blow the fuses in the qfprom
> > from the kernel.  Add properties to support that.
> >
> > NOTE: Whether this is possible depends on the BIOS settings and
> > whether the kernel has permissions here, so not all boards will be
> > able to blow fuses in the kernel.
> >
> > Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> > Signed-off-by: Douglas Anderson <dianders@chromium.org>
> > ---
> >
> > Changes in v3:
> > - Add an extra reg range (at 0x6000 offset for SoCs checked)
> > - Define two options for reg: 1 item or 4 items.
> > - No reg-names.
> > - Add "clocks" and "clock-names" to list of properties.
> > - Clock is now "sec", not "secclk".
> > - Add "vcc-supply" to list of properties.
> > - Fixed up example.
> >
> >   .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
> >   1 file changed, 43 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > index 5efa5e7c4d81..b195212c6193 100644
> > --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> > @@ -17,8 +17,27 @@ properties:
> >       const: qcom,qfprom
> >
> >     reg:
> > -    items:
> > -      - description: The corrected region.
> > +    # If the QFPROM is read-only OS image then only the corrected region
> > +    # needs to be provided.  If the QFPROM is writable then all 4 regions
> > +    # must be provided.
> > +    oneOf:
> > +      - items:
> > +          - description: The corrected region.
> > +      - items:
> > +          - description: The corrected region.
> > +          - description: The raw region.
> > +          - description: The config region.
> > +          - description: The security control region.
> > +
> > +  # Clock must be provided if QFPROM is writable from the OS image.
> > +  clocks:
> > +    maxItems: 1
>
>
> > +  clock-names:
> > +    const: sec
>
> Do we need clock-names for just one clock here?

I think technically you can get by without, but convention is that
clock-names are always provided for clocks.  It's talked about in the
same link I sent that talked about reg-names:

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

Specifically, Rob said:

> That probably is because the clock binding has had clock-names from
> the start (it may have been the first one). That was probably partly
> due to the clock API also was mainly by name already if we want to
> admit Linux influence on bindings

Basically the standard way for getting clocks in Linux is
clk_get(name).  With just one clock you can call clk_get(NULL) and I
believe that works, but when you add the 2nd clock then you have to
switch APIs to one of the less-commonly-used variants.

-Doug

Re: [PATCH v3 1/4] dt-bindings: nvmem: qfprom: Convert to yaml

[Doug Anderson]

Hi,

On Wed, Jun 17, 2020 at 9:26 AM Ravi Kumar Bokka (Temp)
<rbokka@codeaurora.org> wrote:
>
>
>
> On 6/17/2020 8:48 PM, Srinivas Kandagatla wrote:
> >
> >
> > On 17/06/2020 15:51, Douglas Anderson wrote:
> >> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >>
> >> This switches the bindings over from txt to yaml.
> >>
> >> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >> ---
> >>
> >> Changes in v3:
> >> - Split conversion to yaml into separate patch new in v3.
> >> - Use 'const' for compatible instead of a 1-entry enum.
> >> - Changed filename to match compatible string.
> >> - Add #address-cells and #size-cells to list of properties.
> >> - Fixed up example.
> >>
> >>   .../bindings/nvmem/qcom,qfprom.yaml           | 45 +++++++++++++++++++
> >>   .../devicetree/bindings/nvmem/qfprom.txt      | 35 ---------------
> >>   2 files changed, 45 insertions(+), 35 deletions(-)
> >>   create mode 100644
> >> Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>   delete mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >> b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >> new file mode 100644
> >> index 000000000000..5efa5e7c4d81
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >> @@ -0,0 +1,45 @@
> >> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> >> +%YAML 1.2
> >> +---
> >> +$id: http://devicetree.org/schemas/nvmem/qcom,qfprom.yaml#
> >> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> >> +
> >> +title: Qualcomm Technologies Inc, QFPROM Efuse bindings
> >> +
> >> +maintainers:
> >> +  - Ravi Kumar Bokka <rbokka@codeaurora.org>
> >> +
> >
> > Am not sure this was intentional, but the old maintainer name is totally
> > lost in this patch!
> >
> > Please fix this!
> >
>
> Hi Srinivas,
> The existed qfprom dt-bindings in .txt format.
> I will make it as it is to merge whole content in .yaml format once
> confirm all the parameters with this new driver changes.

Ah, right.  It makes more sense to set you as the maintainer since you
were listed as the maintainer for the Linux driver and also the author
of the old bindings.  I'll change it to you then.

Unless I hear otherwise, I'll plan to send out a v4 tomorrow with
feedback I've gotten today.

-Doug

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Srinivas Kandagatla]

+Adding SBoyd.

On 17/06/2020 18:22, Doug Anderson wrote:
> Hi,
> 
> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>
>> On 17/06/2020 15:51, Douglas Anderson wrote:
>>> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>
>>> On some systems it's possible to actually blow the fuses in the qfprom
>>> from the kernel.  Add properties to support that.
>>>
>>> NOTE: Whether this is possible depends on the BIOS settings and
>>> whether the kernel has permissions here, so not all boards will be
>>> able to blow fuses in the kernel.
>>>
>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>> ---
>>>
>>> Changes in v3:
>>> - Add an extra reg range (at 0x6000 offset for SoCs checked)
>>> - Define two options for reg: 1 item or 4 items.
>>> - No reg-names.
>>> - Add "clocks" and "clock-names" to list of properties.
>>> - Clock is now "sec", not "secclk".
>>> - Add "vcc-supply" to list of properties.
>>> - Fixed up example.
>>>
>>>    .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
>>>    1 file changed, 43 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>> index 5efa5e7c4d81..b195212c6193 100644
>>> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>> @@ -17,8 +17,27 @@ properties:
>>>        const: qcom,qfprom
>>>
>>>      reg:
>>> -    items:
>>> -      - description: The corrected region.
>>> +    # If the QFPROM is read-only OS image then only the corrected region
>>> +    # needs to be provided.  If the QFPROM is writable then all 4 regions
>>> +    # must be provided.
>>> +    oneOf:
>>> +      - items:
>>> +          - description: The corrected region.
>>> +      - items:
>>> +          - description: The corrected region.
>>> +          - description: The raw region.
>>> +          - description: The config region.
>>> +          - description: The security control region.
>>> +
>>> +  # Clock must be provided if QFPROM is writable from the OS image.
>>> +  clocks:
>>> +    maxItems: 1
>>
>>
>>> +  clock-names:
>>> +    const: sec
>>
>> Do we need clock-names for just one clock here?
> 
> I think technically you can get by without, but convention is that
> clock-names are always provided for clocks.  It's talked about in the
> same link I sent that talked about reg-names:
> 
> https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
> 

TBH, This is total confusion!!!

when to use "*-names" Device tree bindings is totally depended on Linux 
Subsystem interfaces!

And what is the starting point to draw this line?


> Specifically, Rob said:
> 
>> That probably is because the clock binding has had clock-names from
>> the start (it may have been the first one). That was probably partly
>> due to the clock API also was mainly by name already if we want to
>> admit Linux influence on bindings
> 
> Basically the standard way for getting clocks in Linux is
> clk_get(name).  With just one clock you can call clk_get(NULL) and I
> believe that works, but when you add the 2nd clock then you have to
> switch APIs to one of the less-commonly-used variants.

In previous NON-DT life clk_get api name argument comes from the clk 
names that clk provider registered the clocks with.

If I remember this correctly, the name that is refereed here for 
clk_get() is old clkdev api based on clk_lookups and is not the same as 
clk-names that we have in Device tree. Atleast in this case!

clk-names has two objectives in DT:
1> To find the index of the clock in the clocks DT property.

2> If actual clk name is specified then if "1" fails then name could 
potentially fallback to use old clkdev based clk_lookups.

In this specific case we have "sec" as clock-names which is totally used 
for indexing into clocks property and it can not be used for (2) as 
there is no clk named "sec" registered by any of the clk providers.

So this does not justify the reasoning why "clock-names" should be used 
while "reg-names" should not be used!. Both of them are going to be 
finally used for indexing into there respective properties.

This also brings in greater confusion for both existing and while adding 
bindings with "*-names" for new interfaces.

Rob, can you please provide some clarity and direction on when to 
use/not-use *-names properties!


Thanks,
srini
> 
> -Doug
> 

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
> +Adding SBoyd.
>
> On 17/06/2020 18:22, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >>
> >>
> >> On 17/06/2020 15:51, Douglas Anderson wrote:
> >>> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >>>
> >>> On some systems it's possible to actually blow the fuses in the qfprom
> >>> from the kernel.  Add properties to support that.
> >>>
> >>> NOTE: Whether this is possible depends on the BIOS settings and
> >>> whether the kernel has permissions here, so not all boards will be
> >>> able to blow fuses in the kernel.
> >>>
> >>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>> ---
> >>>
> >>> Changes in v3:
> >>> - Add an extra reg range (at 0x6000 offset for SoCs checked)
> >>> - Define two options for reg: 1 item or 4 items.
> >>> - No reg-names.
> >>> - Add "clocks" and "clock-names" to list of properties.
> >>> - Clock is now "sec", not "secclk".
> >>> - Add "vcc-supply" to list of properties.
> >>> - Fixed up example.
> >>>
> >>>    .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
> >>>    1 file changed, 43 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>> index 5efa5e7c4d81..b195212c6193 100644
> >>> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>> @@ -17,8 +17,27 @@ properties:
> >>>        const: qcom,qfprom
> >>>
> >>>      reg:
> >>> -    items:
> >>> -      - description: The corrected region.
> >>> +    # If the QFPROM is read-only OS image then only the corrected region
> >>> +    # needs to be provided.  If the QFPROM is writable then all 4 regions
> >>> +    # must be provided.
> >>> +    oneOf:
> >>> +      - items:
> >>> +          - description: The corrected region.
> >>> +      - items:
> >>> +          - description: The corrected region.
> >>> +          - description: The raw region.
> >>> +          - description: The config region.
> >>> +          - description: The security control region.
> >>> +
> >>> +  # Clock must be provided if QFPROM is writable from the OS image.
> >>> +  clocks:
> >>> +    maxItems: 1
> >>
> >>
> >>> +  clock-names:
> >>> +    const: sec
> >>
> >> Do we need clock-names for just one clock here?
> >
> > I think technically you can get by without, but convention is that
> > clock-names are always provided for clocks.  It's talked about in the
> > same link I sent that talked about reg-names:
> >
> > https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
> >
>
> TBH, This is total confusion!!!
>
> when to use "*-names" Device tree bindings is totally depended on Linux
> Subsystem interfaces!
>
> And what is the starting point to draw this line?

Definitely confusing and mostly because the dts stuff grew organically
for a while there.  It does feel like Rob is pretty clear on the
current state of things and the policy in the link I provided, though.


> > Specifically, Rob said:
> >
> >> That probably is because the clock binding has had clock-names from
> >> the start (it may have been the first one). That was probably partly
> >> due to the clock API also was mainly by name already if we want to
> >> admit Linux influence on bindings
> >
> > Basically the standard way for getting clocks in Linux is
> > clk_get(name).  With just one clock you can call clk_get(NULL) and I
> > believe that works, but when you add the 2nd clock then you have to
> > switch APIs to one of the less-commonly-used variants.
>
> In previous NON-DT life clk_get api name argument comes from the clk
> names that clk provider registered the clocks with.
>
> If I remember this correctly, the name that is refereed here for
> clk_get() is old clkdev api based on clk_lookups and is not the same as
> clk-names that we have in Device tree. Atleast in this case!
>
> clk-names has two objectives in DT:
> 1> To find the index of the clock in the clocks DT property.
>
> 2> If actual clk name is specified then if "1" fails then name could
> potentially fallback to use old clkdev based clk_lookups.
>
> In this specific case we have "sec" as clock-names which is totally used
> for indexing into clocks property and it can not be used for (2) as
> there is no clk named "sec" registered by any of the clk providers.
>
> So this does not justify the reasoning why "clock-names" should be used
> while "reg-names" should not be used!. Both of them are going to be
> finally used for indexing into there respective properties.

Right, you just have to accept the fact that logic doesn't come into
play here.  For clocks, always use "clk-names" but also always use a
consistent order (which is now more enforced by the schema checker).
For "reg" almost never use "reg-names".


> This also brings in greater confusion for both existing and while adding
> bindings with "*-names" for new interfaces.
>
> Rob, can you please provide some clarity and direction on when to
> use/not-use *-names properties!

If I had to guess Rob will say that we shouldn't add more places where
the convention is to have "-names".


I will put posting v4 of this patch on pause until this is resolved to
avoid fragmenting the discussion.


-Doug

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Srinivas Kandagatla]

On 18/06/2020 14:48, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>> +Adding SBoyd.
>>
>> On 17/06/2020 18:22, Doug Anderson wrote:
>>> Hi,
>>>
>>> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
>>> <srinivas.kandagatla@linaro.org> wrote:
>>>>
>>>>
>>>>
>>>> On 17/06/2020 15:51, Douglas Anderson wrote:
>>>>> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>>>
>>>>> On some systems it's possible to actually blow the fuses in the qfprom
>>>>> from the kernel.  Add properties to support that.
>>>>>
>>>>> NOTE: Whether this is possible depends on the BIOS settings and
>>>>> whether the kernel has permissions here, so not all boards will be
>>>>> able to blow fuses in the kernel.
>>>>>
>>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
>>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
>>>>> ---
>>>>>
>>>>> Changes in v3:
>>>>> - Add an extra reg range (at 0x6000 offset for SoCs checked)
>>>>> - Define two options for reg: 1 item or 4 items.
>>>>> - No reg-names.
>>>>> - Add "clocks" and "clock-names" to list of properties.
>>>>> - Clock is now "sec", not "secclk".
>>>>> - Add "vcc-supply" to list of properties.
>>>>> - Fixed up example.
>>>>>
>>>>>     .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
>>>>>     1 file changed, 43 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>>>> index 5efa5e7c4d81..b195212c6193 100644
>>>>> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>>>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>>>>> @@ -17,8 +17,27 @@ properties:
>>>>>         const: qcom,qfprom
>>>>>
>>>>>       reg:
>>>>> -    items:
>>>>> -      - description: The corrected region.
>>>>> +    # If the QFPROM is read-only OS image then only the corrected region
>>>>> +    # needs to be provided.  If the QFPROM is writable then all 4 regions
>>>>> +    # must be provided.
>>>>> +    oneOf:
>>>>> +      - items:
>>>>> +          - description: The corrected region.
>>>>> +      - items:
>>>>> +          - description: The corrected region.
>>>>> +          - description: The raw region.
>>>>> +          - description: The config region.
>>>>> +          - description: The security control region.
>>>>> +
>>>>> +  # Clock must be provided if QFPROM is writable from the OS image.
>>>>> +  clocks:
>>>>> +    maxItems: 1
>>>>
>>>>
>>>>> +  clock-names:
>>>>> +    const: sec
>>>>
>>>> Do we need clock-names for just one clock here?
>>>
>>> I think technically you can get by without, but convention is that
>>> clock-names are always provided for clocks.  It's talked about in the
>>> same link I sent that talked about reg-names:
>>>
>>> https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
>>>
>>
>> TBH, This is total confusion!!!
>>
>> when to use "*-names" Device tree bindings is totally depended on Linux
>> Subsystem interfaces!
>>
>> And what is the starting point to draw this line?
> 
> Definitely confusing and mostly because the dts stuff grew organically
> for a while there.  It does feel like Rob is pretty clear on the
> current state of things and the policy in the link I provided, though.
> 
> 
>>> Specifically, Rob said:
>>>
>>>> That probably is because the clock binding has had clock-names from
>>>> the start (it may have been the first one). That was probably partly
>>>> due to the clock API also was mainly by name already if we want to
>>>> admit Linux influence on bindings
>>>
>>> Basically the standard way for getting clocks in Linux is
>>> clk_get(name).  With just one clock you can call clk_get(NULL) and I
>>> believe that works, but when you add the 2nd clock then you have to
>>> switch APIs to one of the less-commonly-used variants.
>>
>> In previous NON-DT life clk_get api name argument comes from the clk
>> names that clk provider registered the clocks with.
>>
>> If I remember this correctly, the name that is refereed here for
>> clk_get() is old clkdev api based on clk_lookups and is not the same as
>> clk-names that we have in Device tree. Atleast in this case!
>>
>> clk-names has two objectives in DT:
>> 1> To find the index of the clock in the clocks DT property.
>>
>> 2> If actual clk name is specified then if "1" fails then name could
>> potentially fallback to use old clkdev based clk_lookups.
>>
>> In this specific case we have "sec" as clock-names which is totally used
>> for indexing into clocks property and it can not be used for (2) as
>> there is no clk named "sec" registered by any of the clk providers.
>>
>> So this does not justify the reasoning why "clock-names" should be used
>> while "reg-names" should not be used!. Both of them are going to be
>> finally used for indexing into there respective properties.
> 
> Right, you just have to accept the fact that logic doesn't come into
> play here.  For clocks, always use "clk-names" but also always use a
> consistent order (which is now more enforced by the schema checker).
> For "reg" almost never use "reg-names".
> 

On the other note:

clock-names are not mandatory according to 
Documentation/devicetree/bindings/clock/clock-bindings.txt

For this particular case where clock-names = "sec" is totally used for 
indexing and nothing else!


> 
>> This also brings in greater confusion for both existing and while adding
>> bindings with "*-names" for new interfaces.
>>
>> Rob, can you please provide some clarity and direction on when to
>> use/not-use *-names properties!
> 
> If I had to guess Rob will say that we shouldn't add more places where
> the convention is to have "-names".

Confusion is not just about new bindings, but with the existing ones! :-)


--srini
> 
> 
> I will put posting v4 of this patch on pause until this is resolved to
> avoid fragmenting the discussion.
> 
> 
> -Doug
> 

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 7:01 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> On 18/06/2020 14:48, Doug Anderson wrote:
> > Hi,
> >
> > On Thu, Jun 18, 2020 at 3:10 AM Srinivas Kandagatla
> > <srinivas.kandagatla@linaro.org> wrote:
> >>
> >> +Adding SBoyd.
> >>
> >> On 17/06/2020 18:22, Doug Anderson wrote:
> >>> Hi,
> >>>
> >>> On Wed, Jun 17, 2020 at 8:19 AM Srinivas Kandagatla
> >>> <srinivas.kandagatla@linaro.org> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 17/06/2020 15:51, Douglas Anderson wrote:
> >>>>> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >>>>>
> >>>>> On some systems it's possible to actually blow the fuses in the qfprom
> >>>>> from the kernel.  Add properties to support that.
> >>>>>
> >>>>> NOTE: Whether this is possible depends on the BIOS settings and
> >>>>> whether the kernel has permissions here, so not all boards will be
> >>>>> able to blow fuses in the kernel.
> >>>>>
> >>>>> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> >>>>> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> >>>>> ---
> >>>>>
> >>>>> Changes in v3:
> >>>>> - Add an extra reg range (at 0x6000 offset for SoCs checked)
> >>>>> - Define two options for reg: 1 item or 4 items.
> >>>>> - No reg-names.
> >>>>> - Add "clocks" and "clock-names" to list of properties.
> >>>>> - Clock is now "sec", not "secclk".
> >>>>> - Add "vcc-supply" to list of properties.
> >>>>> - Fixed up example.
> >>>>>
> >>>>>     .../bindings/nvmem/qcom,qfprom.yaml           | 45 ++++++++++++++++++-
> >>>>>     1 file changed, 43 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>>>> index 5efa5e7c4d81..b195212c6193 100644
> >>>>> --- a/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>>>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> >>>>> @@ -17,8 +17,27 @@ properties:
> >>>>>         const: qcom,qfprom
> >>>>>
> >>>>>       reg:
> >>>>> -    items:
> >>>>> -      - description: The corrected region.
> >>>>> +    # If the QFPROM is read-only OS image then only the corrected region
> >>>>> +    # needs to be provided.  If the QFPROM is writable then all 4 regions
> >>>>> +    # must be provided.
> >>>>> +    oneOf:
> >>>>> +      - items:
> >>>>> +          - description: The corrected region.
> >>>>> +      - items:
> >>>>> +          - description: The corrected region.
> >>>>> +          - description: The raw region.
> >>>>> +          - description: The config region.
> >>>>> +          - description: The security control region.
> >>>>> +
> >>>>> +  # Clock must be provided if QFPROM is writable from the OS image.
> >>>>> +  clocks:
> >>>>> +    maxItems: 1
> >>>>
> >>>>
> >>>>> +  clock-names:
> >>>>> +    const: sec
> >>>>
> >>>> Do we need clock-names for just one clock here?
> >>>
> >>> I think technically you can get by without, but convention is that
> >>> clock-names are always provided for clocks.  It's talked about in the
> >>> same link I sent that talked about reg-names:
> >>>
> >>> https://lore.kernel.org/r/CAL_Jsq+MMunmVWqeW9v2RyzsMKP+=kMzeTHNMG4JDHM7Fy0HBg@mail.gmail.com/
> >>>
> >>
> >> TBH, This is total confusion!!!
> >>
> >> when to use "*-names" Device tree bindings is totally depended on Linux
> >> Subsystem interfaces!
> >>
> >> And what is the starting point to draw this line?
> >
> > Definitely confusing and mostly because the dts stuff grew organically
> > for a while there.  It does feel like Rob is pretty clear on the
> > current state of things and the policy in the link I provided, though.
> >
> >
> >>> Specifically, Rob said:
> >>>
> >>>> That probably is because the clock binding has had clock-names from
> >>>> the start (it may have been the first one). That was probably partly
> >>>> due to the clock API also was mainly by name already if we want to
> >>>> admit Linux influence on bindings
> >>>
> >>> Basically the standard way for getting clocks in Linux is
> >>> clk_get(name).  With just one clock you can call clk_get(NULL) and I
> >>> believe that works, but when you add the 2nd clock then you have to
> >>> switch APIs to one of the less-commonly-used variants.
> >>
> >> In previous NON-DT life clk_get api name argument comes from the clk
> >> names that clk provider registered the clocks with.
> >>
> >> If I remember this correctly, the name that is refereed here for
> >> clk_get() is old clkdev api based on clk_lookups and is not the same as
> >> clk-names that we have in Device tree. Atleast in this case!
> >>
> >> clk-names has two objectives in DT:
> >> 1> To find the index of the clock in the clocks DT property.
> >>
> >> 2> If actual clk name is specified then if "1" fails then name could
> >> potentially fallback to use old clkdev based clk_lookups.
> >>
> >> In this specific case we have "sec" as clock-names which is totally used
> >> for indexing into clocks property and it can not be used for (2) as
> >> there is no clk named "sec" registered by any of the clk providers.
> >>
> >> So this does not justify the reasoning why "clock-names" should be used
> >> while "reg-names" should not be used!. Both of them are going to be
> >> finally used for indexing into there respective properties.
> >
> > Right, you just have to accept the fact that logic doesn't come into
> > play here.  For clocks, always use "clk-names" but also always use a
> > consistent order (which is now more enforced by the schema checker).
> > For "reg" almost never use "reg-names".
> >
>
> On the other note:
>
> clock-names are not mandatory according to
> Documentation/devicetree/bindings/clock/clock-bindings.txt
>
> For this particular case where clock-names = "sec" is totally used for
> indexing and nothing else!

So I guess in the one-clock case it's more optional and if you feel
strongly I'll get rid of clk-names here.  ...but if we ever need
another clock we probably will want to add it back and (I could be
corrected) I believe it's convention to specify clk-names even with
one clock.

I won't say it's impossible to get by without clock-names when you
have more than one clock, but (almost) nobody does it.  It's hard to
quickly come up with a good way to search for this, but skimming
through:

git grep -C5 'clocks.*,' -- arch/arm64/boot/dts

...you don't find too many examples of no clock-names and you find
_lots_ of examples where clock-names are specified.  One example that
_does_ have multiple clocks and doesn't specify clock-names is
"simple-framebuffer".  Looking at the Linux driver you can see they
have to use the special "of_clk_get()" variant to handle it.


-Doug

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Doug Anderson]

Hi,

On Thu, Jun 18, 2020 at 9:55 AM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Doug Anderson (2020-06-18 08:32:20)
> > Hi,
> >
> > On Thu, Jun 18, 2020 at 7:01 AM Srinivas Kandagatla
> > >
> > > On the other note:
> > >
> > > clock-names are not mandatory according to
> > > Documentation/devicetree/bindings/clock/clock-bindings.txt
> > >
> > > For this particular case where clock-names = "sec" is totally used for
> > > indexing and nothing else!
> >
> > So I guess in the one-clock case it's more optional and if you feel
> > strongly I'll get rid of clk-names here.  ...but if we ever need
> > another clock we probably will want to add it back and (I could be
> > corrected) I believe it's convention to specify clk-names even with
> > one clock.
>
> TL;DR: I suggest you call this "core" if you want to keep the
> clock-name, or just drop it if there's only one clk and move on.

Ah, true.  "core" sounds good.


> It's not required to have clock-names with one clk, and indeed it's not
> required to have clock-names at all. The multi clk scenario is a little
> more difficult to handle because historically the clk_get() API has been
> name based and not index based like platform resources. When there is
> one clk the driver can pass NULL as the 'con_id' argument to clk_get()
> and it will do the right thing. And when you have more than one clk you
> can pass NULL still and get the first clk, that should be in the same
> index, and then other clks by name.
>
> So far nobody has added clk_get_by_index() but I suppose if it was
> important the API could be added. Working with only legacy clkdev
> lookups would fail of course, but clock-names could be fully deprecated
> and kernel images may be smaller because we're not storing piles of
> strings and doing string comparisons. Given that it's been this way for
> a long time and we have DT schema checking it doesn't seem very
> important to mandate anything one way or the other though. I certainly
> don't feel good when I see of_clk_*() APIs being used by platform
> drivers, but sometimes it is required.
>
> To really put this into perspective, consider the fact that most drivers
> have code that figures out what clk names to look for and then they pile
> them into arrays and just turn them all on and off together. Providing
> fine grained clk control here is a gigantic waste of time, and requiring
> clock-names is just more hoops that driver authors feel they have to
> jump through for $reasons. We have clk_bulk_get_all() for this, but that
> doesn't solve the one rate changing clk among the sea of clk gates
> problem. In general, driver authors don't care and we should probably be
> providing a richer while simpler API to them that manages power state of
> some handful of clks, regulators, and power domains for a device while
> also letting them control various knobs like clk rate when necessary.
>
> BTW, on qcom platforms they usually name clks "core" and "iface" for the
> core clk and the interface clk used to access the registers of a device.
> Sometimes there are esoteric ones like "axi". In theory this cuts down
> on the number of strings the kernel keeps around but I like that it
> helps provide continuity across drivers and DTs for their SoCs. If you
> ask the hardware engineer what the clk name is for the hardware block
> they'll tell you the globally unique clk name like
> "gcc_qupv3_uart9_core_clk", which is the worst name to use.

OK, sounds about what I expected.  I suppose the path of least
resistance would be to just drop clock-names.  I guess I'm just
worried that down the road someone will want to specify the "iface"
clock too.  If that ever happens, we're stuck with these options:

1. Be the first ones to require adding clk_get_by_index().

2. Use the frowned upon of_clk_get() API which allows getting by index.

3. Get the first clock with clk_get(NULL) and the second clock with
clk_get("iface") and figure out how to specify this happily in the
yaml.

If we just define clock-names now then we pretty much match the
pattern of everyone else.


Srinivas: reading all that if you still want me to drop clock-names
then I will.  I'll use clk_get(NULL) to get the clock and if/when we
ever need an "iface" clock (maybe we never will?) we can figure it out
then.


-Doug

Re: [PATCH v3 1/4] dt-bindings: nvmem: qfprom: Convert to yaml

[Rob Herring]

On Wed, 17 Jun 2020 07:51:13 -0700, Douglas Anderson wrote:
> From: Ravi Kumar Bokka <rbokka@codeaurora.org>
> 
> This switches the bindings over from txt to yaml.
> 
> Signed-off-by: Ravi Kumar Bokka <rbokka@codeaurora.org>
> Signed-off-by: Douglas Anderson <dianders@chromium.org>
> ---
> 
> Changes in v3:
> - Split conversion to yaml into separate patch new in v3.
> - Use 'const' for compatible instead of a 1-entry enum.
> - Changed filename to match compatible string.
> - Add #address-cells and #size-cells to list of properties.
> - Fixed up example.
> 
>  .../bindings/nvmem/qcom,qfprom.yaml           | 45 +++++++++++++++++++
>  .../devicetree/bindings/nvmem/qfprom.txt      | 35 ---------------
>  2 files changed, 45 insertions(+), 35 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
>  delete mode 100644 Documentation/devicetree/bindings/nvmem/qfprom.txt
> 


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

/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/nvmem/qcom,qfprom.example.dt.yaml: example-0: efuse@784000:reg:0: [0, 7880704, 0, 2303] is too long


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

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

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

Please check and re-submit.

Re: [PATCH v3 2/4] dt-bindings: nvmem: Add properties needed for blowing fuses

[Srinivas Kandagatla]

On 18/06/2020 18:25, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 18, 2020 at 9:55 AM Stephen Boyd <sboyd@kernel.org> wrote:
>>
>> Quoting Doug Anderson (2020-06-18 08:32:20)
>>> Hi,
>>>
>>> On Thu, Jun 18, 2020 at 7:01 AM Srinivas Kandagatla
>>>>
>>>> On the other note:
>>>>
>>>> clock-names are not mandatory according to
>>>> Documentation/devicetree/bindings/clock/clock-bindings.txt
>>>>
>>>> For this particular case where clock-names = "sec" is totally used for
>>>> indexing and nothing else!
>>>
>>> So I guess in the one-clock case it's more optional and if you feel
>>> strongly I'll get rid of clk-names here.  ...but if we ever need
>>> another clock we probably will want to add it back and (I could be
>>> corrected) I believe it's convention to specify clk-names even with
>>> one clock.
>>
>> TL;DR: I suggest you call this "core" if you want to keep the
>> clock-name, or just drop it if there's only one clk and move on.
> 
> Ah, true.  "core" sounds good.
> 
> 
>> It's not required to have clock-names with one clk, and indeed it's not
>> required to have clock-names at all. The multi clk scenario is a little
>> more difficult to handle because historically the clk_get() API has been
>> name based and not index based like platform resources. When there is
>> one clk the driver can pass NULL as the 'con_id' argument to clk_get()
>> and it will do the right thing. And when you have more than one clk you
>> can pass NULL still and get the first clk, that should be in the same
>> index, and then other clks by name.
>>
>> So far nobody has added clk_get_by_index() but I suppose if it was
>> important the API could be added. Working with only legacy clkdev
>> lookups would fail of course, but clock-names could be fully deprecated
>> and kernel images may be smaller because we're not storing piles of
>> strings and doing string comparisons. Given that it's been this way for
>> a long time and we have DT schema checking it doesn't seem very
>> important to mandate anything one way or the other though. I certainly
>> don't feel good when I see of_clk_*() APIs being used by platform
>> drivers, but sometimes it is required.
>>
>> To really put this into perspective, consider the fact that most drivers
>> have code that figures out what clk names to look for and then they pile
>> them into arrays and just turn them all on and off together. Providing
>> fine grained clk control here is a gigantic waste of time, and requiring
>> clock-names is just more hoops that driver authors feel they have to
>> jump through for $reasons. We have clk_bulk_get_all() for this, but that
>> doesn't solve the one rate changing clk among the sea of clk gates
>> problem. In general, driver authors don't care and we should probably be
>> providing a richer while simpler API to them that manages power state of
>> some handful of clks, regulators, and power domains for a device while
>> also letting them control various knobs like clk rate when necessary.
>>
>> BTW, on qcom platforms they usually name clks "core" and "iface" for the
>> core clk and the interface clk used to access the registers of a device.
>> Sometimes there are esoteric ones like "axi". In theory this cuts down
>> on the number of strings the kernel keeps around but I like that it
>> helps provide continuity across drivers and DTs for their SoCs. If you
>> ask the hardware engineer what the clk name is for the hardware block
>> they'll tell you the globally unique clk name like
>> "gcc_qupv3_uart9_core_clk", which is the worst name to use.
> 
> OK, sounds about what I expected.  I suppose the path of least
> resistance would be to just drop clock-names.  I guess I'm just
> worried that down the road someone will want to specify the "iface"
> clock too.  If that ever happens, we're stuck with these options:
> 
> 1. Be the first ones to require adding clk_get_by_index().
> 
> 2. Use the frowned upon of_clk_get() API which allows getting by index.
> 
> 3. Get the first clock with clk_get(NULL) and the second clock with
> clk_get("iface") and figure out how to specify this happily in the
> yaml.
> 
> If we just define clock-names now then we pretty much match the
> pattern of everyone else.

Thanks to Stephen Boyd for his inputs and directions here!

I guess we have to live with "clock-names" for now for both consistency 
and reasons detailed by Boyd.

Am okay with the clock-names!

--srini
> 
> 
> Srinivas: reading all that if you still want me to drop clock-names
> then I will.  I'll use clk_get(NULL) to get the clock and if/when we
> ever need an "iface" clock (maybe we never will?) we can figure it out
> then.
> 
> 
> -Doug
> 

Re: [PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support

[Pavel Machek]

Hi!

> This patch adds support for blowing fuses to the qfprom driver if the
> required properties are defined in the device tree.

Should we have this in kernel?

If so, should we make it harder to use, like passing module parameter 
enabling this kind of support or something? Kconfig option as most users 
will not need this so this should be compiled out?

									Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

Re: [PATCH v3 3/4] nvmem: qfprom: Add fuse blowing support

[Doug Anderson]

Hi,

On Tue, Jun 23, 2020 at 6:36 AM Pavel Machek <pavel@ucw.cz> wrote:
>
> Hi!
>
> > This patch adds support for blowing fuses to the qfprom driver if the
> > required properties are defined in the device tree.
>
> Should we have this in kernel?
>
> If so, should we make it harder to use, like passing module parameter
> enabling this kind of support or something? Kconfig option as most users
> will not need this so this should be compiled out?
>
>                                                                         Pavel

It's definitely a good question.  I'm curious: who are you trying to
protect these fuses from?  A bumbling user or a malicious bit of code.

For a bumbling user we presumably just need something that makes it
not quite so easy to blow the fuses.  Passing a module parameter isn't
a bad idea.  Would the module parameter only take effect if provided
when the module was loaded, or could it be switched on later via
sysfs?

For a malicious bit of code: the only real protection is to have the
bootloader protect these, or to blow the fuses which disable future
fuses from being blown (the access permission fuses).  Otherwise
malicious code could always just code up its own driver to bypass any
protections.

NOTE: if we already have protection from malicious code by having the
bootloader configure protections, I wonder if we still need additional
protections against a bumbling user?


-Doug