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