[PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Komal Bajaj]

This patch adds bindings for secure qfprom found in QCOM SOCs.
Secure QFPROM driver is based on simple nvmem framework.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
new file mode 100644
index 000000000000..1425ced36fdf
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc, Secure QFPROM Efuse
+
+maintainers:
+  - Komal Bajaj <quic_kbajaj@quicinc.com>
+
+description: |
+  For some of the Qualcomm SoC's, it is possible that
+  the qfprom region is protected from non-secure access.
+  In such situations, linux will have to use secure calls
+  to read the region.
+
+allOf:
+  - $ref: nvmem.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,qdu1000-sec-qfprom
+      - const: qcom,sec-qfprom
+
+  reg:
+    items:
+      - description: The secure qfprom corrected region.
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      efuse@221c8000 {
+        compatible = "qcom,qdu1000-sec-qfprom", "qcom,sec-qfprom";
+        reg = <0 0x221c8000 0 0x1000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        multi_chan_ddr: multi-chan-ddr@12b {
+          reg = <0x12b 0x1>;
+          bits = <0 2>;
+        };
+      };
+    };
+
--
2.40.1

[PATCH v5 0/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Komal Bajaj]

Changes in v5 -
 - Separating this from original series [1].
 - Added description of driver to secure qfprom binding.
 - Replaced pm_runtime_enable() withh devm_pm_runtime_enable().
 - Changed module license to GPL instead of GPL v2.

This series introduces a new driver for reading secure fuse region and adding
dt-bindings for same.

[1] https://lore.kernel.org/linux-arm-msm/20230623141806.13388-1-quic_kbajaj@quicinc.com/


Komal Bajaj (2):
  dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom
  nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

 .../bindings/nvmem/qcom,sec-qfprom.yaml       |  58 ++++++++++
 drivers/nvmem/Kconfig                         |  13 +++
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/sec-qfprom.c                    | 101 ++++++++++++++++++
 4 files changed, 174 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
 create mode 100644 drivers/nvmem/sec-qfprom.c

--
2.40.1

[PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Komal Bajaj]

This patch adds bindings for secure qfprom found in QCOM SOCs.
Secure QFPROM driver is based on simple nvmem framework.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml

diff --git a/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
new file mode 100644
index 000000000000..1425ced36fdf
--- /dev/null
+++ b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
@@ -0,0 +1,58 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm Technologies Inc, Secure QFPROM Efuse
+
+maintainers:
+  - Komal Bajaj <quic_kbajaj@quicinc.com>
+
+description: |
+  For some of the Qualcomm SoC's, it is possible that
+  the qfprom region is protected from non-secure access.
+  In such situations, linux will have to use secure calls
+  to read the region.
+
+allOf:
+  - $ref: nvmem.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,qdu1000-sec-qfprom
+      - const: qcom,sec-qfprom
+
+  reg:
+    items:
+      - description: The secure qfprom corrected region.
+
+required:
+  - compatible
+  - reg
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
+
+    soc {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      efuse@221c8000 {
+        compatible = "qcom,qdu1000-sec-qfprom", "qcom,sec-qfprom";
+        reg = <0 0x221c8000 0 0x1000>;
+        #address-cells = <1>;
+        #size-cells = <1>;
+
+        multi_chan_ddr: multi-chan-ddr@12b {
+          reg = <0x12b 0x1>;
+          bits = <0 2>;
+        };
+      };
+    };
+
--
2.40.1

[PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Komal Bajaj]

For some of the Qualcomm SoC's, it is possible that
some of the fuse regions or entire qfprom region is
protected from non-secure access. In such situations,
Linux will have to use secure calls to read the region.
With that motivation, add secure qfprom driver.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 drivers/nvmem/Kconfig      |  13 +++++
 drivers/nvmem/Makefile     |   2 +
 drivers/nvmem/sec-qfprom.c | 101 +++++++++++++++++++++++++++++++++++++
 3 files changed, 116 insertions(+)
 create mode 100644 drivers/nvmem/sec-qfprom.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index b291b27048c7..764fc5feb26c 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -216,6 +216,19 @@ config NVMEM_QCOM_QFPROM
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem_qfprom.

+config NVMEM_QCOM_SEC_QFPROM
+        tristate "QCOM SECURE QFPROM Support"
+        depends on ARCH_QCOM || COMPILE_TEST
+        depends on HAS_IOMEM
+        depends on OF
+        select QCOM_SCM
+        help
+          Say y here to enable secure QFPROM support. The secure QFPROM provides access
+          functions for QFPROM data to rest of the drivers via nvmem interface.
+
+          This driver can also be built as a module. If so, the module will be called
+          nvmem_sec_qfprom.
+
 config NVMEM_RAVE_SP_EEPROM
 	tristate "Rave SP EEPROM Support"
 	depends on RAVE_SP_CORE
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index f82431ec8aef..e248d3daadf3 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
 nvmem-nintendo-otp-y			:= nintendo-otp.o
 obj-$(CONFIG_NVMEM_QCOM_QFPROM)		+= nvmem_qfprom.o
 nvmem_qfprom-y				:= qfprom.o
+obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM)	+= nvmem_sec_qfprom.o
+nvmem_sec_qfprom-y			:= sec-qfprom.o
 obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM)	+= nvmem-rave-sp-eeprom.o
 nvmem-rave-sp-eeprom-y			:= rave-sp-eeprom.o
 obj-$(CONFIG_NVMEM_RMEM) 		+= nvmem-rmem.o
diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
new file mode 100644
index 000000000000..bc68053b7d94
--- /dev/null
+++ b/drivers/nvmem/sec-qfprom.c
@@ -0,0 +1,101 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/mod_devicetable.h>
+#include <linux/nvmem-provider.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+
+/**
+ * struct sec_qfprom - structure holding secure qfprom attributes
+ *
+ * @base: starting physical address for secure qfprom corrected address space.
+ * @dev: qfprom device structure.
+ */
+struct sec_qfprom {
+	phys_addr_t base;
+	struct device *dev;
+};
+
+static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
+{
+	struct sec_qfprom *priv = context;
+	unsigned int i;
+	u8 *val = _val;
+	u32 read_val;
+	u8 *tmp;
+
+	for (i = 0; i < bytes; i++, reg++) {
+		if (i == 0 || reg % 4 == 0) {
+			if (qcom_scm_io_readl(priv->base + (reg & ~3), &read_val)) {
+				dev_err(priv->dev, "Couldn't access fuse register\n");
+				return -EINVAL;
+			}
+			tmp = (u8 *)&read_val;
+		}
+
+		val[i] = tmp[reg & 3];
+	}
+
+	return 0;
+}
+
+static int sec_qfprom_probe(struct platform_device *pdev)
+{
+	struct nvmem_config econfig = {
+		.name = "sec-qfprom",
+		.stride = 1,
+		.word_size = 1,
+		.id = NVMEM_DEVID_AUTO,
+		.reg_read = sec_qfprom_reg_read,
+	};
+	struct device *dev = &pdev->dev;
+	struct nvmem_device *nvmem;
+	struct sec_qfprom *priv;
+	struct resource *res;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res)
+		return -EINVAL;
+
+	priv->base = res->start;
+
+	econfig.size = resource_size(res);
+	econfig.dev = dev;
+	econfig.priv = priv;
+
+	priv->dev = dev;
+
+	ret = devm_pm_runtime_enable(dev);
+	if (ret)
+		return ret;
+
+	nvmem = devm_nvmem_register(dev, &econfig);
+
+	return PTR_ERR_OR_ZERO(nvmem);
+}
+
+static const struct of_device_id sec_qfprom_of_match[] = {
+	{ .compatible = "qcom,sec-qfprom" },
+	{/* sentinel */},
+};
+MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
+
+static struct platform_driver qfprom_driver = {
+	.probe = sec_qfprom_probe,
+	.driver = {
+		.name = "qcom_sec_qfprom",
+		.of_match_table = sec_qfprom_of_match,
+	},
+};
+module_platform_driver(qfprom_driver);
+MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
+MODULE_LICENSE("GPL");
--
2.40.1

Re: [PATCH v5 0/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Komal Bajaj]

Sorry for the spam, sent the same series twice.

Thanks
Komal

On 7/24/2023 2:08 PM, Komal Bajaj wrote:
> Changes in v5 -
>   - Separating this from original series [1].
>   - Added description of driver to secure qfprom binding.
>   - Replaced pm_runtime_enable() withh devm_pm_runtime_enable().
>   - Changed module license to GPL instead of GPL v2.
>
> This series introduces a new driver for reading secure fuse region and adding
> dt-bindings for same.
>
> [1] https://lore.kernel.org/linux-arm-msm/20230623141806.13388-1-quic_kbajaj@quicinc.com/
>
>
> Komal Bajaj (2):
>    dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom
>    nvmem: sec-qfprom: Add Qualcomm secure QFPROM support
>
>   .../bindings/nvmem/qcom,sec-qfprom.yaml       |  58 ++++++++++
>   drivers/nvmem/Kconfig                         |  13 +++
>   drivers/nvmem/Makefile                        |   2 +
>   drivers/nvmem/sec-qfprom.c                    | 101 ++++++++++++++++++
>   4 files changed, 174 insertions(+)
>   create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
>   create mode 100644 drivers/nvmem/sec-qfprom.c
>
> --
> 2.40.1
>

Re: [PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Krzysztof Kozlowski]

On 24/07/2023 10:29, Komal Bajaj wrote:
> This patch adds bindings for secure qfprom found in QCOM SOCs.
> Secure QFPROM driver is based on simple nvmem framework.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> new file mode 100644
> index 000000000000..1425ced36fdf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc, Secure QFPROM Efuse
> +
> +maintainers:
> +  - Komal Bajaj <quic_kbajaj@quicinc.com>
> +
> +description: |
> +  For some of the Qualcomm SoC's, it is possible that
> +  the qfprom region is protected from non-secure access.
> +  In such situations, linux will have to use secure calls

Bindings are not for Linux, so:
s/linux/software/

> +  to read the region.
> +
> +allOf:
> +  - $ref: nvmem.yaml#
> +

With above:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [PATCH v5 0/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Conor Dooley]

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

On Mon, Jul 24, 2023 at 02:08:47PM +0530, Komal Bajaj wrote:
> Changes in v5 -
>  - Separating this from original series [1].
>  - Added description of driver to secure qfprom binding.
>  - Replaced pm_runtime_enable() withh devm_pm_runtime_enable().
>  - Changed module license to GPL instead of GPL v2.
> 
> This series introduces a new driver for reading secure fuse region and adding
> dt-bindings for same.
> 
> [1] https://lore.kernel.org/linux-arm-msm/20230623141806.13388-1-quic_kbajaj@quicinc.com/

Why does this series have two v5s?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 0/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Conor Dooley]

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

On Mon, Jul 24, 2023 at 05:53:36PM +0100, Conor Dooley wrote:
> On Mon, Jul 24, 2023 at 02:08:47PM +0530, Komal Bajaj wrote:
> > Changes in v5 -
> >  - Separating this from original series [1].
> >  - Added description of driver to secure qfprom binding.
> >  - Replaced pm_runtime_enable() withh devm_pm_runtime_enable().
> >  - Changed module license to GPL instead of GPL v2.
> > 
> > This series introduces a new driver for reading secure fuse region and adding
> > dt-bindings for same.
> > 
> > [1] https://lore.kernel.org/linux-arm-msm/20230623141806.13388-1-quic_kbajaj@quicinc.com/
> 
> Why does this series have two v5s?

Never mind, I missed the reply Komal. Apologies!

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Bjorn Andersson]

On Mon, Jul 24, 2023 at 02:08:49PM +0530, Komal Bajaj wrote:
> For some of the Qualcomm SoC's, it is possible that
> some of the fuse regions or entire qfprom region is
> protected from non-secure access. In such situations,
> Linux will have to use secure calls to read the region.
> With that motivation, add secure qfprom driver.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>

Reviewed-by: Bjorn Andersson <andersson@kernel.org>

Regards,
Bjorn

Re: [PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Rob Herring]

On Mon, Jul 24, 2023 at 02:08:48PM +0530, Komal Bajaj wrote:
> This patch adds bindings for secure qfprom found in QCOM SOCs.
> Secure QFPROM driver is based on simple nvmem framework.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> new file mode 100644
> index 000000000000..1425ced36fdf
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> @@ -0,0 +1,58 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc, Secure QFPROM Efuse
> +
> +maintainers:
> +  - Komal Bajaj <quic_kbajaj@quicinc.com>
> +
> +description: |

Don't need '|'

> +  For some of the Qualcomm SoC's, it is possible that
> +  the qfprom region is protected from non-secure access.
> +  In such situations, linux will have to use secure calls

s/linux/the OS/

> +  to read the region.

Wrap lines at 80

The wording for this is strange. Only sometimes for this binding do 
secure calls have to be used? If you are using secure calls, does that 
mean the 'reg' address is not directly accessible.

> +
> +allOf:
> +  - $ref: nvmem.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,qdu1000-sec-qfprom
> +      - const: qcom,sec-qfprom
> +
> +  reg:
> +    items:
> +      - description: The secure qfprom corrected region.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +
> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      efuse@221c8000 {
> +        compatible = "qcom,qdu1000-sec-qfprom", "qcom,sec-qfprom";
> +        reg = <0 0x221c8000 0 0x1000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        multi_chan_ddr: multi-chan-ddr@12b {
> +          reg = <0x12b 0x1>;
> +          bits = <0 2>;
> +        };
> +      };
> +    };
> +
> --
> 2.40.1
> 

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Mukesh Ojha]

Hi,

Some questions, may not need to be addressed if the reason is
known

On 7/24/2023 2:08 PM, Komal Bajaj wrote:
> For some of the Qualcomm SoC's, it is possible that
> some of the fuse regions or entire qfprom region is
> protected from non-secure access. In such situations,
> Linux will have to use secure calls to read the region.
> With that motivation, add secure qfprom driver.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>   drivers/nvmem/Kconfig      |  13 +++++
>   drivers/nvmem/Makefile     |   2 +
>   drivers/nvmem/sec-qfprom.c | 101 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 116 insertions(+)
>   create mode 100644 drivers/nvmem/sec-qfprom.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c7..764fc5feb26c 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -216,6 +216,19 @@ config NVMEM_QCOM_QFPROM
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem_qfprom.
> 
> +config NVMEM_QCOM_SEC_QFPROM
> +        tristate "QCOM SECURE QFPROM Support"
> +        depends on ARCH_QCOM || COMPILE_TEST
> +        depends on HAS_IOMEM
> +        depends on OF
> +        select QCOM_SCM
> +        help
> +          Say y here to enable secure QFPROM support. The secure QFPROM provides access
> +          functions for QFPROM data to rest of the drivers via nvmem interface.
> +
> +          This driver can also be built as a module. If so, the module will be called
> +          nvmem_sec_qfprom.
> +
>   config NVMEM_RAVE_SP_EEPROM
>   	tristate "Rave SP EEPROM Support"
>   	depends on RAVE_SP_CORE
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index f82431ec8aef..e248d3daadf3 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
>   nvmem-nintendo-otp-y			:= nintendo-otp.o
>   obj-$(CONFIG_NVMEM_QCOM_QFPROM)		+= nvmem_qfprom.o
>   nvmem_qfprom-y				:= qfprom.o
> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM)	+= nvmem_sec_qfprom.o
> +nvmem_sec_qfprom-y			:= sec-qfprom.o

Are we just doing this for just renaming the object ?

>   obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM)	+= nvmem-rave-sp-eeprom.o
>   nvmem-rave-sp-eeprom-y			:= rave-sp-eeprom.o
>   obj-$(CONFIG_NVMEM_RMEM) 		+= nvmem-rmem.o
> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
> new file mode 100644
> index 000000000000..bc68053b7d94
> --- /dev/null
> +++ b/drivers/nvmem/sec-qfprom.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +
> +/**
> + * struct sec_qfprom - structure holding secure qfprom attributes
> + *
> + * @base: starting physical address for secure qfprom corrected address space.
> + * @dev: qfprom device structure.
> + */
> +struct sec_qfprom {
> +	phys_addr_t base;
> +	struct device *dev;
> +};
> +
> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> +{
> +	struct sec_qfprom *priv = context;
> +	unsigned int i;
> +	u8 *val = _val;
> +	u32 read_val;
> +	u8 *tmp;
> +
> +	for (i = 0; i < bytes; i++, reg++) {
> +		if (i == 0 || reg % 4 == 0) {
> +			if (qcom_scm_io_readl(priv->base + (reg & ~3), &read_val)) {
> +				dev_err(priv->dev, "Couldn't access fuse register\n");
> +				return -EINVAL;
> +			}
> +			tmp = (u8 *)&read_val;
> +		}
> +
> +		val[i] = tmp[reg & 3];
> +	}

Getting secure read from fuse region is fine here, since we have to read
4 byte from trustzone, but this restriction of reading is also there
for sm8{4|5}50 soc's where byte by byte reading is protected and 
granularity set to 4 byte (qfprom_reg_read() in drivers/nvmem/qfprom.c)
is will result in abort, in  that case this function need to export this
logic.

> +
> +	return 0;
> +}
> +
> +static int sec_qfprom_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config econfig = {
> +		.name = "sec-qfprom",
> +		.stride = 1,
> +		.word_size = 1,
> +		.id = NVMEM_DEVID_AUTO,
> +		.reg_read = sec_qfprom_reg_read,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_device *nvmem;
> +	struct sec_qfprom *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	priv->base = res->start;
> +
> +	econfig.size = resource_size(res);
> +	econfig.dev = dev;
> +	econfig.priv = priv;
> +
> +	priv->dev = dev;
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;
> +
> +	nvmem = devm_nvmem_register(dev, &econfig);
> +
> +	return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id sec_qfprom_of_match[] = {
> +	{ .compatible = "qcom,sec-qfprom" },
> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
> +
> +static struct platform_driver qfprom_driver = {
> +	.probe = sec_qfprom_probe,

Why don't we have remove/remove_new callbacks?
Same comment apply for drivers/nvmem/qfprom.c

> +	.driver = {
> +		.name = "qcom_sec_qfprom",
> +		.of_match_table = sec_qfprom_of_match,
> +	},
> +};
> +module_platform_driver(qfprom_driver);
> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
> +MODULE_LICENSE("GPL");
> --
> 2.40.1
> 

-Mukesh

Re: [PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Pavan Kondeti]

On Mon, Jul 24, 2023 at 02:08:48PM +0530, Komal Bajaj wrote:
> This patch adds bindings for secure qfprom found in QCOM SOCs.
> Secure QFPROM driver is based on simple nvmem framework.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
>  1 file changed, 58 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
> 

[...]

> +$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm Technologies Inc, Secure QFPROM Efuse
> +
> +maintainers:
> +  - Komal Bajaj <quic_kbajaj@quicinc.com>
> +
> +description: |
> +  For some of the Qualcomm SoC's, it is possible that
> +  the qfprom region is protected from non-secure access.
> +  In such situations, linux will have to use secure calls
> +  to read the region.
> +
> +allOf:
> +  - $ref: nvmem.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,qdu1000-sec-qfprom
> +      - const: qcom,sec-qfprom
> +
> +  reg:
> +    items:
> +      - description: The secure qfprom corrected region.
> +
> +required:
> +  - compatible
> +  - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
> +

minor nitpick:

Since this device does not have any clocks, the above header inclusion
can be dropped.

> +    soc {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      efuse@221c8000 {
> +        compatible = "qcom,qdu1000-sec-qfprom", "qcom,sec-qfprom";
> +        reg = <0 0x221c8000 0 0x1000>;
> +        #address-cells = <1>;
> +        #size-cells = <1>;
> +
> +        multi_chan_ddr: multi-chan-ddr@12b {
> +          reg = <0x12b 0x1>;
> +          bits = <0 2>;
> +        };
> +      };
> +    };
> +

Thanks,
Pavan

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Srinivas Kandagatla]

On 24/07/2023 09:38, Komal Bajaj wrote:
> For some of the Qualcomm SoC's, it is possible that
> some of the fuse regions or entire qfprom region is
> protected from non-secure access. In such situations,
> Linux will have to use secure calls to read the region.
> With that motivation, add secure qfprom driver.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>   drivers/nvmem/Kconfig      |  13 +++++
>   drivers/nvmem/Makefile     |   2 +
>   drivers/nvmem/sec-qfprom.c | 101 +++++++++++++++++++++++++++++++++++++
>   3 files changed, 116 insertions(+)
>   create mode 100644 drivers/nvmem/sec-qfprom.c
> 

> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
> new file mode 100644
> index 000000000000..bc68053b7d94
> --- /dev/null
> +++ b/drivers/nvmem/sec-qfprom.c
> @@ -0,0 +1,101 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>

> +
> +static int sec_qfprom_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config econfig = {
> +		.name = "sec-qfprom",
> +		.stride = 1,
> +		.word_size = 1,
> +		.id = NVMEM_DEVID_AUTO,
> +		.reg_read = sec_qfprom_reg_read,
> +	};
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_device *nvmem;
> +	struct sec_qfprom *priv;
> +	struct resource *res;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -EINVAL;
> +
> +	priv->base = res->start;
> +
> +	econfig.size = resource_size(res);
> +	econfig.dev = dev;
> +	econfig.priv = priv;
> +
> +	priv->dev = dev;
> +
> +	ret = devm_pm_runtime_enable(dev);
> +	if (ret)
> +		return ret;

Any reason why we need to enable pm runtime for this driver? As Am not 
seeing any pm runtime handlers or users in this driver.


--srini
> +
> +	nvmem = devm_nvmem_register(dev, &econfig);
> +
> +	return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id sec_qfprom_of_match[] = {
> +	{ .compatible = "qcom,sec-qfprom" },
> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
> +
> +static struct platform_driver qfprom_driver = {
> +	.probe = sec_qfprom_probe,
> +	.driver = {
> +		.name = "qcom_sec_qfprom",
> +		.of_match_table = sec_qfprom_of_match,
> +	},
> +};
> +module_platform_driver(qfprom_driver);
> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
> +MODULE_LICENSE("GPL");
> --
> 2.40.1
> 

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Mukesh Ojha]

On 7/27/2023 12:09 PM, Mukesh Ojha wrote:
> Hi,
> 
> Some questions, may not need to be addressed if the reason is
> known
> 
> On 7/24/2023 2:08 PM, Komal Bajaj wrote:
>> For some of the Qualcomm SoC's, it is possible that
>> some of the fuse regions or entire qfprom region is
>> protected from non-secure access. In such situations,
>> Linux will have to use secure calls to read the region.
>> With that motivation, add secure qfprom driver.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   drivers/nvmem/Kconfig      |  13 +++++
>>   drivers/nvmem/Makefile     |   2 +
>>   drivers/nvmem/sec-qfprom.c | 101 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>   create mode 100644 drivers/nvmem/sec-qfprom.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index b291b27048c7..764fc5feb26c 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -216,6 +216,19 @@ config NVMEM_QCOM_QFPROM
>>         This driver can also be built as a module. If so, the module
>>         will be called nvmem_qfprom.
>>
>> +config NVMEM_QCOM_SEC_QFPROM
>> +        tristate "QCOM SECURE QFPROM Support"
>> +        depends on ARCH_QCOM || COMPILE_TEST
>> +        depends on HAS_IOMEM
>> +        depends on OF
>> +        select QCOM_SCM
>> +        help
>> +          Say y here to enable secure QFPROM support. The secure 
>> QFPROM provides access
>> +          functions for QFPROM data to rest of the drivers via nvmem 
>> interface.
>> +
>> +          This driver can also be built as a module. If so, the 
>> module will be called
>> +          nvmem_sec_qfprom.
>> +
>>   config NVMEM_RAVE_SP_EEPROM
>>       tristate "Rave SP EEPROM Support"
>>       depends on RAVE_SP_CORE
>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>> index f82431ec8aef..e248d3daadf3 100644
>> --- a/drivers/nvmem/Makefile
>> +++ b/drivers/nvmem/Makefile
>> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP)    += 
>> nvmem-nintendo-otp.o
>>   nvmem-nintendo-otp-y            := nintendo-otp.o
>>   obj-$(CONFIG_NVMEM_QCOM_QFPROM)        += nvmem_qfprom.o
>>   nvmem_qfprom-y                := qfprom.o
>> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM)    += nvmem_sec_qfprom.o
>> +nvmem_sec_qfprom-y            := sec-qfprom.o
> 
> Are we just doing this for just renaming the object ?
> 
>>   obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM)    += nvmem-rave-sp-eeprom.o
>>   nvmem-rave-sp-eeprom-y            := rave-sp-eeprom.o
>>   obj-$(CONFIG_NVMEM_RMEM)         += nvmem-rmem.o
>> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
>> new file mode 100644
>> index 000000000000..bc68053b7d94
>> --- /dev/null
>> +++ b/drivers/nvmem/sec-qfprom.c
>> @@ -0,0 +1,101 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include <linux/firmware/qcom/qcom_scm.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
>> +
>> +/**
>> + * struct sec_qfprom - structure holding secure qfprom attributes
>> + *
>> + * @base: starting physical address for secure qfprom corrected 
>> address space.
>> + * @dev: qfprom device structure.
>> + */
>> +struct sec_qfprom {
>> +    phys_addr_t base;
>> +    struct device *dev;
>> +};
>> +
>> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void 
>> *_val, size_t bytes)
>> +{
>> +    struct sec_qfprom *priv = context;
>> +    unsigned int i;
>> +    u8 *val = _val;
>> +    u32 read_val;
>> +    u8 *tmp;
>> +
>> +    for (i = 0; i < bytes; i++, reg++) {
>> +        if (i == 0 || reg % 4 == 0) {
>> +            if (qcom_scm_io_readl(priv->base + (reg & ~3), &read_val)) {
>> +                dev_err(priv->dev, "Couldn't access fuse register\n");
>> +                return -EINVAL;
>> +            }
>> +            tmp = (u8 *)&read_val;
>> +        }
>> +
>> +        val[i] = tmp[reg & 3];
>> +    }
> 
> Getting secure read from fuse region is fine here, since we have to read
> 4 byte from trustzone, but this restriction of reading is also there
> for sm8{4|5}50 soc's where byte by byte reading is protected and 
> granularity set to 4 byte (qfprom_reg_read() in drivers/nvmem/qfprom.c)
> is will result in abort, in  that case this function need to export this
> logic.
> 
>> +
>> +    return 0;
>> +}
>> +
>> +static int sec_qfprom_probe(struct platform_device *pdev)
>> +{
>> +    struct nvmem_config econfig = {
>> +        .name = "sec-qfprom",
>> +        .stride = 1,
>> +        .word_size = 1,
>> +        .id = NVMEM_DEVID_AUTO,
>> +        .reg_read = sec_qfprom_reg_read,
>> +    };
>> +    struct device *dev = &pdev->dev;
>> +    struct nvmem_device *nvmem;
>> +    struct sec_qfprom *priv;
>> +    struct resource *res;
>> +    int ret;
>> +
>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res)
>> +        return -EINVAL;
>> +
>> +    priv->base = res->start;
>> +
>> +    econfig.size = resource_size(res);
>> +    econfig.dev = dev;
>> +    econfig.priv = priv;
>> +
>> +    priv->dev = dev;
>> +
>> +    ret = devm_pm_runtime_enable(dev);
>> +    if (ret)
>> +        return ret;
>> +
>> +    nvmem = devm_nvmem_register(dev, &econfig);
>> +
>> +    return PTR_ERR_OR_ZERO(nvmem);
>> +}
>> +
>> +static const struct of_device_id sec_qfprom_of_match[] = {
>> +    { .compatible = "qcom,sec-qfprom" },
>> +    {/* sentinel */},
>> +};
>> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
>> +
>> +static struct platform_driver qfprom_driver = {
>> +    .probe = sec_qfprom_probe,
> 
> Why don't we have remove/remove_new callbacks?
> Same comment apply for drivers/nvmem/qfprom.c

Ignore this comment; Something new learnt with devm_* api
implementation.

-Mukesh
> 
>> +    .driver = {
>> +        .name = "qcom_sec_qfprom",
>> +        .of_match_table = sec_qfprom_of_match,
>> +    },
>> +};
>> +module_platform_driver(qfprom_driver);
>> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.40.1
>>
> 
> -Mukesh

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Mukesh Ojha]

On 7/27/2023 4:14 PM, Srinivas Kandagatla wrote:
> 
> 
> On 24/07/2023 09:38, Komal Bajaj wrote:
>> For some of the Qualcomm SoC's, it is possible that
>> some of the fuse regions or entire qfprom region is
>> protected from non-secure access. In such situations,
>> Linux will have to use secure calls to read the region.
>> With that motivation, add secure qfprom driver.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   drivers/nvmem/Kconfig      |  13 +++++
>>   drivers/nvmem/Makefile     |   2 +
>>   drivers/nvmem/sec-qfprom.c | 101 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 116 insertions(+)
>>   create mode 100644 drivers/nvmem/sec-qfprom.c
>>
> 
>> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
>> new file mode 100644
>> index 000000000000..bc68053b7d94
>> --- /dev/null
>> +++ b/drivers/nvmem/sec-qfprom.c
>> @@ -0,0 +1,101 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights 
>> reserved.
>> + */
>> +
>> +#include <linux/firmware/qcom/qcom_scm.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_runtime.h>
> 
>> +
>> +static int sec_qfprom_probe(struct platform_device *pdev)
>> +{
>> +    struct nvmem_config econfig = {
>> +        .name = "sec-qfprom",
>> +        .stride = 1,
>> +        .word_size = 1,
>> +        .id = NVMEM_DEVID_AUTO,
>> +        .reg_read = sec_qfprom_reg_read,
>> +    };
>> +    struct device *dev = &pdev->dev;
>> +    struct nvmem_device *nvmem;
>> +    struct sec_qfprom *priv;
>> +    struct resource *res;
>> +    int ret;
>> +
>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +    if (!priv)
>> +        return -ENOMEM;
>> +
>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +    if (!res)
>> +        return -EINVAL;
>> +
>> +    priv->base = res->start;
>> +
>> +    econfig.size = resource_size(res);
>> +    econfig.dev = dev;
>> +    econfig.priv = priv;
>> +
>> +    priv->dev = dev;
>> +
>> +    ret = devm_pm_runtime_enable(dev);
>> +    if (ret)
>> +        return ret;
> 
> Any reason why we need to enable pm runtime for this driver? As Am not 
> seeing any pm runtime handlers or users in this driver.

Thanks..
Yes, it is not needed as of now..
looks like, it got inherited from qfprom.c by mistake.

Same need to be corrected in Device tree, if any
unnecessary reference is there related to this..

-Mukesh
> 
> 
> --srini
>> +
>> +    nvmem = devm_nvmem_register(dev, &econfig);
>> +
>> +    return PTR_ERR_OR_ZERO(nvmem);
>> +}
>> +
>> +static const struct of_device_id sec_qfprom_of_match[] = {
>> +    { .compatible = "qcom,sec-qfprom" },
>> +    {/* sentinel */},
>> +};
>> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
>> +
>> +static struct platform_driver qfprom_driver = {
>> +    .probe = sec_qfprom_probe,
>> +    .driver = {
>> +        .name = "qcom_sec_qfprom",
>> +        .of_match_table = sec_qfprom_of_match,
>> +    },
>> +};
>> +module_platform_driver(qfprom_driver);
>> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
>> +MODULE_LICENSE("GPL");
>> -- 
>> 2.40.1
>>

Re: [PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Komal Bajaj]

On 7/26/2023 10:10 PM, Rob Herring wrote:
> On Mon, Jul 24, 2023 at 02:08:48PM +0530, Komal Bajaj wrote:
>> This patch adds bindings for secure qfprom found in QCOM SOCs.
>> Secure QFPROM driver is based on simple nvmem framework.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
>> new file mode 100644
>> index 000000000000..1425ced36fdf
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
>> @@ -0,0 +1,58 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies Inc, Secure QFPROM Efuse
>> +
>> +maintainers:
>> +  - Komal Bajaj <quic_kbajaj@quicinc.com>
>> +
>> +description: |
> Don't need '|'

Okay, will drop this.
Just a doubt here, when do we use this '|' symbol, I

  thought we will use this when
we have multi-line description/value.

>
>> +  For some of the Qualcomm SoC's, it is possible that
>> +  the qfprom region is protected from non-secure access.
>> +  In such situations, linux will have to use secure calls
> s/linux/the OS/

Will do it.

>
>> +  to read the region.
> Wrap lines at 80
>
> The wording for this is strange. Only sometimes for this binding do
> secure calls have to be used? If you are using secure calls, does that
> mean the 'reg' address is not directly accessible.

For this binding, we will always use secure calls because the 'reg' 
address is not directly
accessible to the OS.

Thanks
Komal

>
>> +
>> +allOf:
>> +  - $ref: nvmem.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - qcom,qdu1000-sec-qfprom
>> +      - const: qcom,sec-qfprom
>> +
>> +  reg:
>> +    items:
>> +      - description: The secure qfprom corrected region.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
>> +
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      efuse@221c8000 {
>> +        compatible = "qcom,qdu1000-sec-qfprom", "qcom,sec-qfprom";
>> +        reg = <0 0x221c8000 0 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        multi_chan_ddr: multi-chan-ddr@12b {
>> +          reg = <0x12b 0x1>;
>> +          bits = <0 2>;
>> +        };
>> +      };
>> +    };
>> +
>> --
>> 2.40.1
>>

Re: [PATCH v5 1/2] dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom

[Komal Bajaj]

On 7/27/2023 2:39 PM, Pavan Kondeti wrote:
> On Mon, Jul 24, 2023 at 02:08:48PM +0530, Komal Bajaj wrote:
>> This patch adds bindings for secure qfprom found in QCOM SOCs.
>> Secure QFPROM driver is based on simple nvmem framework.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   .../bindings/nvmem/qcom,sec-qfprom.yaml       | 58 +++++++++++++++++++
>>   1 file changed, 58 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
>>
> [...]
>
>> +$id: http://devicetree.org/schemas/nvmem/qcom,sec-qfprom.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm Technologies Inc, Secure QFPROM Efuse
>> +
>> +maintainers:
>> +  - Komal Bajaj <quic_kbajaj@quicinc.com>
>> +
>> +description: |
>> +  For some of the Qualcomm SoC's, it is possible that
>> +  the qfprom region is protected from non-secure access.
>> +  In such situations, linux will have to use secure calls
>> +  to read the region.
>> +
>> +allOf:
>> +  - $ref: nvmem.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - qcom,qdu1000-sec-qfprom
>> +      - const: qcom,sec-qfprom
>> +
>> +  reg:
>> +    items:
>> +      - description: The secure qfprom corrected region.
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +
>> +unevaluatedProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/qcom,gcc-sc7180.h>
>> +
> minor nitpick:
>
> Since this device does not have any clocks, the above header inclusion
> can be dropped.

Yes, it is not needed, will drop it.

Thanks
Komal

>
>> +    soc {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      efuse@221c8000 {
>> +        compatible = "qcom,qdu1000-sec-qfprom", "qcom,sec-qfprom";
>> +        reg = <0 0x221c8000 0 0x1000>;
>> +        #address-cells = <1>;
>> +        #size-cells = <1>;
>> +
>> +        multi_chan_ddr: multi-chan-ddr@12b {
>> +          reg = <0x12b 0x1>;
>> +          bits = <0 2>;
>> +        };
>> +      };
>> +    };
>> +
> Thanks,
> Pavan

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Bjorn Andersson]

On Thu, Jul 27, 2023 at 12:09:07PM +0530, Mukesh Ojha wrote:
> On 7/24/2023 2:08 PM, Komal Bajaj wrote:
[..]
> > diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> > index f82431ec8aef..e248d3daadf3 100644
> > --- a/drivers/nvmem/Makefile
> > +++ b/drivers/nvmem/Makefile
> > @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
> >   nvmem-nintendo-otp-y			:= nintendo-otp.o
> >   obj-$(CONFIG_NVMEM_QCOM_QFPROM)		+= nvmem_qfprom.o
> >   nvmem_qfprom-y				:= qfprom.o
> > +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM)	+= nvmem_sec_qfprom.o
> > +nvmem_sec_qfprom-y			:= sec-qfprom.o
> 
> Are we just doing this for just renaming the object ?
> 

Correct.

> >   obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM)	+= nvmem-rave-sp-eeprom.o
> >   nvmem-rave-sp-eeprom-y			:= rave-sp-eeprom.o
> >   obj-$(CONFIG_NVMEM_RMEM) 		+= nvmem-rmem.o
> > diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
[..]
> > +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> > +{
> > +	struct sec_qfprom *priv = context;
> > +	unsigned int i;
> > +	u8 *val = _val;
> > +	u32 read_val;
> > +	u8 *tmp;
> > +
> > +	for (i = 0; i < bytes; i++, reg++) {
> > +		if (i == 0 || reg % 4 == 0) {
> > +			if (qcom_scm_io_readl(priv->base + (reg & ~3), &read_val)) {
> > +				dev_err(priv->dev, "Couldn't access fuse register\n");
> > +				return -EINVAL;
> > +			}
> > +			tmp = (u8 *)&read_val;
> > +		}
> > +
> > +		val[i] = tmp[reg & 3];
> > +	}
> 
> Getting secure read from fuse region is fine here, since we have to read
> 4 byte from trustzone, but this restriction of reading is also there
> for sm8{4|5}50 soc's where byte by byte reading is protected and granularity
> set to 4 byte (qfprom_reg_read() in drivers/nvmem/qfprom.c)
> is will result in abort, in  that case this function need to export this
> logic.
> 

If qfprom needs similar treatment, then let's land this first and then
consider generalizing (i.e. move to some library code) this - or if
infeasible, just fix qfprom_reg_read().

Regards,
Bjorn

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Komal Bajaj]

On 7/31/2023 10:05 PM, Bjorn Andersson wrote:
> On Thu, Jul 27, 2023 at 12:09:07PM +0530, Mukesh Ojha wrote:
>> On 7/24/2023 2:08 PM, Komal Bajaj wrote:
> [..]
>>> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
>>> index f82431ec8aef..e248d3daadf3 100644
>>> --- a/drivers/nvmem/Makefile
>>> +++ b/drivers/nvmem/Makefile
>>> @@ -44,6 +44,8 @@ obj-$(CONFIG_NVMEM_NINTENDO_OTP)	+= nvmem-nintendo-otp.o
>>>    nvmem-nintendo-otp-y			:= nintendo-otp.o
>>>    obj-$(CONFIG_NVMEM_QCOM_QFPROM)		+= nvmem_qfprom.o
>>>    nvmem_qfprom-y				:= qfprom.o
>>> +obj-$(CONFIG_NVMEM_QCOM_SEC_QFPROM)	+= nvmem_sec_qfprom.o
>>> +nvmem_sec_qfprom-y			:= sec-qfprom.o
>> Are we just doing this for just renaming the object ?
>>
> Correct.
>
>>>    obj-$(CONFIG_NVMEM_RAVE_SP_EEPROM)	+= nvmem-rave-sp-eeprom.o
>>>    nvmem-rave-sp-eeprom-y			:= rave-sp-eeprom.o
>>>    obj-$(CONFIG_NVMEM_RMEM) 		+= nvmem-rmem.o
>>> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
> [..]
>>> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
>>> +{
>>> +	struct sec_qfprom *priv = context;
>>> +	unsigned int i;
>>> +	u8 *val = _val;
>>> +	u32 read_val;
>>> +	u8 *tmp;
>>> +
>>> +	for (i = 0; i < bytes; i++, reg++) {
>>> +		if (i == 0 || reg % 4 == 0) {
>>> +			if (qcom_scm_io_readl(priv->base + (reg & ~3), &read_val)) {
>>> +				dev_err(priv->dev, "Couldn't access fuse register\n");
>>> +				return -EINVAL;
>>> +			}
>>> +			tmp = (u8 *)&read_val;
>>> +		}
>>> +
>>> +		val[i] = tmp[reg & 3];
>>> +	}
>> Getting secure read from fuse region is fine here, since we have to read
>> 4 byte from trustzone, but this restriction of reading is also there
>> for sm8{4|5}50 soc's where byte by byte reading is protected and granularity
>> set to 4 byte (qfprom_reg_read() in drivers/nvmem/qfprom.c)
>> is will result in abort, in  that case this function need to export this
>> logic.
>>
> If qfprom needs similar treatment, then let's land this first and then
> consider generalizing (i.e. move to some library code) this - or if
> infeasible, just fix qfprom_reg_read().

Agree, I will implement this logic into qfprom driver (into 
qfprom_reg_read() ) in a separate patch.

Thanks
Komal
>
> Regards,
> Bjorn

Re: [PATCH v5 2/2] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support

[Komal Bajaj]

On 7/28/2023 1:55 PM, Mukesh Ojha wrote:
>
>
> On 7/27/2023 4:14 PM, Srinivas Kandagatla wrote:
>>
>>
>> On 24/07/2023 09:38, Komal Bajaj wrote:
>>> For some of the Qualcomm SoC's, it is possible that
>>> some of the fuse regions or entire qfprom region is
>>> protected from non-secure access. In such situations,
>>> Linux will have to use secure calls to read the region.
>>> With that motivation, add secure qfprom driver.
>>>
>>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>>> ---
>>>   drivers/nvmem/Kconfig      |  13 +++++
>>>   drivers/nvmem/Makefile     |   2 +
>>>   drivers/nvmem/sec-qfprom.c | 101 
>>> +++++++++++++++++++++++++++++++++++++
>>>   3 files changed, 116 insertions(+)
>>>   create mode 100644 drivers/nvmem/sec-qfprom.c
>>>
>>
>>> diff --git a/drivers/nvmem/sec-qfprom.c b/drivers/nvmem/sec-qfprom.c
>>> new file mode 100644
>>> index 000000000000..bc68053b7d94
>>> --- /dev/null
>>> +++ b/drivers/nvmem/sec-qfprom.c
>>> @@ -0,0 +1,101 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/*
>>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights 
>>> reserved.
>>> + */
>>> +
>>> +#include <linux/firmware/qcom/qcom_scm.h>
>>> +#include <linux/mod_devicetable.h>
>>> +#include <linux/nvmem-provider.h>
>>> +#include <linux/platform_device.h>
>>> +#include <linux/pm_runtime.h>
>>
>>> +
>>> +static int sec_qfprom_probe(struct platform_device *pdev)
>>> +{
>>> +    struct nvmem_config econfig = {
>>> +        .name = "sec-qfprom",
>>> +        .stride = 1,
>>> +        .word_size = 1,
>>> +        .id = NVMEM_DEVID_AUTO,
>>> +        .reg_read = sec_qfprom_reg_read,
>>> +    };
>>> +    struct device *dev = &pdev->dev;
>>> +    struct nvmem_device *nvmem;
>>> +    struct sec_qfprom *priv;
>>> +    struct resource *res;
>>> +    int ret;
>>> +
>>> +    priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>>> +    if (!priv)
>>> +        return -ENOMEM;
>>> +
>>> +    res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>>> +    if (!res)
>>> +        return -EINVAL;
>>> +
>>> +    priv->base = res->start;
>>> +
>>> +    econfig.size = resource_size(res);
>>> +    econfig.dev = dev;
>>> +    econfig.priv = priv;
>>> +
>>> +    priv->dev = dev;
>>> +
>>> +    ret = devm_pm_runtime_enable(dev);
>>> +    if (ret)
>>> +        return ret;
>>
>> Any reason why we need to enable pm runtime for this driver? As Am 
>> not seeing any pm runtime handlers or users in this driver.
>
> Thanks..
> Yes, it is not needed as of now..
> looks like, it got inherited from qfprom.c by mistake.
>
> Same need to be corrected in Device tree, if any
> unnecessary reference is there related to this..

Thanks for pointing it out.
Will drop it in the next patch series.

Thanks
Komal

>
> -Mukesh
>>
>>
>> --srini
>>> +
>>> +    nvmem = devm_nvmem_register(dev, &econfig);
>>> +
>>> +    return PTR_ERR_OR_ZERO(nvmem);
>>> +}
>>> +
>>> +static const struct of_device_id sec_qfprom_of_match[] = {
>>> +    { .compatible = "qcom,sec-qfprom" },
>>> +    {/* sentinel */},
>>> +};
>>> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
>>> +
>>> +static struct platform_driver qfprom_driver = {
>>> +    .probe = sec_qfprom_probe,
>>> +    .driver = {
>>> +        .name = "qcom_sec_qfprom",
>>> +        .of_match_table = sec_qfprom_of_match,
>>> +    },
>>> +};
>>> +module_platform_driver(qfprom_driver);
>>> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
>>> +MODULE_LICENSE("GPL");
>>> -- 
>>> 2.40.1
>>>