[PATCH v4 0/6] soc: qcom: llcc: Add support for QDU1000/QRU1000

[PATCH v4 0/6] soc: qcom: llcc: Add support for QDU1000/QRU1000

[Komal Bajaj]

From: Komal-Bajaj <quic_kbajaj@quicinc.com>

This patch series does the following -
 * Add secure qfprom driver for reading secure fuse region in qfprom driver
 * Add dt-bindings for secure qfprom
 * Refactor LLCC driver to support multiple configuration
 * Add support for multi channel DDR configuration in LLCC
 * Add LLCC support for the Qualcomm QDU1000 and QRU1000 SoCs

Changes in v4 -
 - Created a separate driver for reading from secure fuse region as suggested.
 - Added patch for dt-bindings of secure qfprom driver accordingly.
 - Added new properties in the dt-bindings for LLCC. 
 - Implemented new logic to read the nvmem cell as suggested by Bjorn.
 - Separating the DT patches from this series as per suggestion.

Changes in v3-
 - Addressed comments from Krzysztof and Mani.
 - Using qfprom to read DDR configuration from feature register.

Changes in v2:
  - Addressing comments from Konrad.

Komal Bajaj (6):
  dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom
  dt-bindings: cache: qcom,llcc: Add LLCC compatible for QDU1000/QRU1000
  nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.
  soc: qcom: llcc: Refactor llcc driver to support multiple
    configuration
  soc: qcom: Add LLCC support for multi channel DDR
  soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support

 .../devicetree/bindings/cache/qcom,llcc.yaml  |  10 +
 .../bindings/nvmem/qcom,sec-qfprom.yaml       |  58 ++++
 drivers/nvmem/Kconfig                         |  12 +
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/sec-qfprom.c                    | 116 +++++++
 drivers/soc/qcom/Kconfig                      |   2 +
 drivers/soc/qcom/llcc-qcom.c                  | 304 +++++++++++++-----
 include/linux/soc/qcom/llcc-qcom.h            |   2 +-
 8 files changed, 416 insertions(+), 90 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
 create mode 100644 drivers/nvmem/sec-qfprom.c

-- 
2.40.1

[PATCH v4 1/6] 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..675e27918c7b
--- /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>
+
+allOf:
+  - $ref: nvmem.yaml#
+
+properties:
+  compatible:
+    items:
+      - enum:
+          - qcom,qdu1000-sec-qfprom
+      - const: qcom,sec-qfprom
+
+  reg:
+    items:
+      - description: The secure qfprom corrected region.
+
+  # Needed if any child nodes are present.
+  "#address-cells":
+    const: 1
+  "#size-cells":
+    const: 1
+
+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 v4 2/6] dt-bindings: cache: qcom,llcc: Add LLCC compatible for QDU1000/QRU1000

[Komal Bajaj]

Add LLCC compatible for QDU1000/QRU1000 SoCs and add optional
nvmem-cells and nvmem-cell-names property.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
index 44892aa589fd..580f9a97ddf7 100644
--- a/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
+++ b/Documentation/devicetree/bindings/cache/qcom,llcc.yaml
@@ -20,6 +20,7 @@ description: |
 properties:
   compatible:
     enum:
+      - qcom,qdu1000-llcc
       - qcom,sc7180-llcc
       - qcom,sc7280-llcc
       - qcom,sc8180x-llcc
@@ -44,6 +45,14 @@ properties:
   interrupts:
     maxItems: 1
 
+  nvmem-cells:
+    items:
+      - description: Reference to an nvmem node for multi channel DDR
+
+  nvmem-cell-names:
+    items:
+      - const: multi-chan-ddr
+
 required:
   - compatible
   - reg
@@ -92,6 +101,7 @@ allOf:
         compatible:
           contains:
             enum:
+              - qcom,qdu1000-llcc
               - qcom,sc8180x-llcc
               - qcom,sc8280xp-llcc
     then:
-- 
2.40.1

[PATCH v4 3/6] 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. Ensuring
the address to read is word aligned since our secure I/O
only supports word size I/O.

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

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index b291b27048c7..2b0dd73d899e 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -216,6 +216,18 @@ 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
+        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..4b4bf5880488 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..47b2c71593dd
--- /dev/null
+++ b/drivers/nvmem/sec-qfprom.c
@@ -0,0 +1,116 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/mod_devicetable.h>
+#include <linux/nvmem-provider.h>
+#include <linux/platform_device.h>
+#include <linux/pm_domain.h>
+#include <linux/pm_runtime.h>
+#include <linux/property.h>
+#include <linux/regulator/consumer.h>
+
+
+/**
+ * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
+ *
+ * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
+ * @dev: qfprom device structure.
+ */
+struct sec_qfprom_priv {
+	phys_addr_t qfpseccorrected;
+	struct device *dev;
+};
+
+static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
+{
+	struct sec_qfprom_priv *priv = context;
+	u8 *val = _val;
+	u8 *tmp;
+	u32 read_val;
+	unsigned int i;
+
+	for (i = 0; i < bytes; i++, reg++) {
+		if (i == 0 || reg % 4 == 0) {
+			if (qcom_scm_io_readl(priv->qfpseccorrected + (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 void sec_qfprom_runtime_disable(void *data)
+{
+	pm_runtime_disable(data);
+}
+
+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 resource *res;
+	struct nvmem_device *nvmem;
+	struct sec_qfprom_priv *priv;
+	int ret;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	priv->qfpseccorrected = res->start;
+	if (!priv->qfpseccorrected)
+		return -ENOMEM;
+
+	econfig.size = resource_size(res);
+	econfig.dev = dev;
+	econfig.priv = priv;
+
+	priv->dev = dev;
+
+	pm_runtime_enable(dev);
+	ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, 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 v2");
-- 
2.40.1

[PATCH v4 4/6] soc: qcom: llcc: Refactor llcc driver to support multiple configuration

[Komal Bajaj]

Refactor driver to support multiple configuration for llcc on a target.

Reviewed-by: Abel Vesa <abel.vesa@linaro.org>
Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 drivers/soc/qcom/llcc-qcom.c | 210 ++++++++++++++++++++---------------
 1 file changed, 123 insertions(+), 87 deletions(-)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 67c19ed2219a..6cf373da5df9 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -423,101 +423,137 @@ static const u32 llcc_v2_1_reg_offset[] = {
 	[LLCC_COMMON_STATUS0]	= 0x0003400c,
 };
 
-static const struct qcom_llcc_config sc7180_cfg = {
-	.sct_data	= sc7180_data,
-	.size		= ARRAY_SIZE(sc7180_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sc7180_cfg[] = {
+	{
+		.sct_data	= sc7180_data,
+		.size		= ARRAY_SIZE(sc7180_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sc7280_cfg = {
-	.sct_data	= sc7280_data,
-	.size		= ARRAY_SIZE(sc7280_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sc7280_cfg[] = {
+	{
+		.sct_data	= sc7280_data,
+		.size		= ARRAY_SIZE(sc7280_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sc8180x_cfg = {
-	.sct_data	= sc8180x_data,
-	.size		= ARRAY_SIZE(sc8180x_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sc8180x_cfg[] = {
+	{
+		.sct_data	= sc8180x_data,
+		.size		= ARRAY_SIZE(sc8180x_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sc8280xp_cfg = {
-	.sct_data	= sc8280xp_data,
-	.size		= ARRAY_SIZE(sc8280xp_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sc8280xp_cfg[] = {
+	{
+		.sct_data	= sc8280xp_data,
+		.size		= ARRAY_SIZE(sc8280xp_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sdm845_cfg = {
-	.sct_data	= sdm845_data,
-	.size		= ARRAY_SIZE(sdm845_data),
-	.need_llcc_cfg	= false,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
-	.no_edac	= true,
+static const struct qcom_llcc_config sdm845_cfg[] = {
+	{
+		.sct_data	= sdm845_data,
+		.size		= ARRAY_SIZE(sdm845_data),
+		.need_llcc_cfg	= false,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+		.no_edac	= true,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm6350_cfg = {
-	.sct_data	= sm6350_data,
-	.size		= ARRAY_SIZE(sm6350_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sm6350_cfg[] = {
+	{
+		.sct_data	= sm6350_data,
+		.size		= ARRAY_SIZE(sm6350_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm7150_cfg = {
-	.sct_data       = sm7150_data,
-	.size           = ARRAY_SIZE(sm7150_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sm7150_cfg[] = {
+	{
+		.sct_data       = sm7150_data,
+		.size           = ARRAY_SIZE(sm7150_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm8150_cfg = {
-	.sct_data       = sm8150_data,
-	.size           = ARRAY_SIZE(sm8150_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sm8150_cfg[] = {
+	{
+		.sct_data       = sm8150_data,
+		.size           = ARRAY_SIZE(sm8150_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm8250_cfg = {
-	.sct_data       = sm8250_data,
-	.size           = ARRAY_SIZE(sm8250_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sm8250_cfg[] = {
+	{
+		.sct_data       = sm8250_data,
+		.size           = ARRAY_SIZE(sm8250_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm8350_cfg = {
-	.sct_data       = sm8350_data,
-	.size           = ARRAY_SIZE(sm8350_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v1_reg_offset,
-	.edac_reg_offset = &llcc_v1_edac_reg_offset,
+static const struct qcom_llcc_config sm8350_cfg[] = {
+	{
+		.sct_data       = sm8350_data,
+		.size           = ARRAY_SIZE(sm8350_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v1_reg_offset,
+		.edac_reg_offset = &llcc_v1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm8450_cfg = {
-	.sct_data       = sm8450_data,
-	.size           = ARRAY_SIZE(sm8450_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v2_1_reg_offset,
-	.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+static const struct qcom_llcc_config sm8450_cfg[] = {
+	{
+		.sct_data       = sm8450_data,
+		.size           = ARRAY_SIZE(sm8450_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+	{ },
 };
 
-static const struct qcom_llcc_config sm8550_cfg = {
-	.sct_data       = sm8550_data,
-	.size           = ARRAY_SIZE(sm8550_data),
-	.need_llcc_cfg	= true,
-	.reg_offset	= llcc_v2_1_reg_offset,
-	.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+static const struct qcom_llcc_config sm8550_cfg[] = {
+	{
+		.sct_data       = sm8550_data,
+		.size           = ARRAY_SIZE(sm8550_data),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+	{ },
 };
 
 static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
@@ -1004,8 +1040,8 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 
 	drv_data->version = version;
 
-	llcc_cfg = cfg->sct_data;
-	sz = cfg->size;
+	llcc_cfg = cfg[0]->sct_data;
+	sz = cfg[0]->size;
 
 	for (i = 0; i < sz; i++)
 		if (llcc_cfg[i].slice_id > drv_data->max_slices)
@@ -1051,18 +1087,18 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_llcc_of_match[] = {
-	{ .compatible = "qcom,sc7180-llcc", .data = &sc7180_cfg },
-	{ .compatible = "qcom,sc7280-llcc", .data = &sc7280_cfg },
-	{ .compatible = "qcom,sc8180x-llcc", .data = &sc8180x_cfg },
-	{ .compatible = "qcom,sc8280xp-llcc", .data = &sc8280xp_cfg },
-	{ .compatible = "qcom,sdm845-llcc", .data = &sdm845_cfg },
-	{ .compatible = "qcom,sm6350-llcc", .data = &sm6350_cfg },
-	{ .compatible = "qcom,sm7150-llcc", .data = &sm7150_cfg },
-	{ .compatible = "qcom,sm8150-llcc", .data = &sm8150_cfg },
-	{ .compatible = "qcom,sm8250-llcc", .data = &sm8250_cfg },
-	{ .compatible = "qcom,sm8350-llcc", .data = &sm8350_cfg },
-	{ .compatible = "qcom,sm8450-llcc", .data = &sm8450_cfg },
-	{ .compatible = "qcom,sm8550-llcc", .data = &sm8550_cfg },
+	{ .compatible = "qcom,sc7180-llcc", .data = sc7180_cfg },
+	{ .compatible = "qcom,sc7280-llcc", .data = sc7280_cfg },
+	{ .compatible = "qcom,sc8180x-llcc", .data = sc8180x_cfg },
+	{ .compatible = "qcom,sc8280xp-llcc", .data = sc8280xp_cfg },
+	{ .compatible = "qcom,sdm845-llcc", .data = sdm845_cfg },
+	{ .compatible = "qcom,sm6350-llcc", .data = sm6350_cfg },
+	{ .compatible = "qcom,sm7150-llcc", .data = sm7150_cfg },
+	{ .compatible = "qcom,sm8150-llcc", .data = sm8150_cfg },
+	{ .compatible = "qcom,sm8250-llcc", .data = sm8250_cfg },
+	{ .compatible = "qcom,sm8350-llcc", .data = sm8350_cfg },
+	{ .compatible = "qcom,sm8450-llcc", .data = sm8450_cfg },
+	{ .compatible = "qcom,sm8550-llcc", .data = sm8550_cfg },
 	{ }
 };
 MODULE_DEVICE_TABLE(of, qcom_llcc_of_match);
-- 
2.40.1

[PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Komal Bajaj]

Add LLCC support for multi channel DDR configuration
based on a feature register. Reading DDR channel
confiuration uses nvmem framework, so select the
dependency in Kconfig. Without this, there will be
errors while building the driver with COMPILE_TEST only.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 drivers/soc/qcom/Kconfig     |  2 ++
 drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
 2 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
index a491718f8064..cc9ad41c63aa 100644
--- a/drivers/soc/qcom/Kconfig
+++ b/drivers/soc/qcom/Kconfig
@@ -64,6 +64,8 @@ config QCOM_LLCC
 	tristate "Qualcomm Technologies, Inc. LLCC driver"
 	depends on ARCH_QCOM || COMPILE_TEST
 	select REGMAP_MMIO
+	select NVMEM
+	select QCOM_SCM
 	help
 	  Qualcomm Technologies, Inc. platform specific
 	  Last Level Cache Controller(LLCC) driver for platforms such as,
diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 6cf373da5df9..3c29612da1c5 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -12,6 +12,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/mutex.h>
+#include <linux/nvmem-consumer.h>
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/regmap.h>
@@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
 	return ret;
 }
 
+static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
+{
+	int ret;
+
+	ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
+	if (ret == -ENOENT) {
+		*cfg_index = 0;
+		return 0;
+	}
+
+	return ret;
+}
+
 static int qcom_llcc_remove(struct platform_device *pdev)
 {
 	/* Set the global pointer to a error code to avoid referencing it */
@@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int ret, i;
 	struct platform_device *llcc_edac;
-	const struct qcom_llcc_config *cfg;
+	const struct qcom_llcc_config *cfg, *entry;
 	const struct llcc_slice_config *llcc_cfg;
 	u32 sz;
+	u8 cfg_index;
 	u32 version;
 	struct regmap *regmap;
+	u32 num_entries = 0;
 
 	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
 	if (!drv_data) {
@@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 
 	drv_data->version = version;
 
-	llcc_cfg = cfg[0]->sct_data;
-	sz = cfg[0]->size;
+	ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
+	if (ret)
+		goto err;
+
+	for (entry = cfg; entry->sct_data; entry++, num_entries++)
+		;
+	if (cfg_index >= num_entries || cfg_index < 0) {
+		ret = -EINVAL;
+		goto err;
+	}
+
+	llcc_cfg = cfg[cfg_index].sct_data;
+	sz = cfg[cfg_index].size;
 
 	for (i = 0; i < sz; i++)
 		if (llcc_cfg[i].slice_id > drv_data->max_slices)
-- 
2.40.1

[PATCH v4 6/6] soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support

[Komal Bajaj]

Add LLCC configuration data for QDU1000 and QRU1000 SoCs
and updating macro name for LLCC_DRE to LLCC_ECC as per
the latest specification.

Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
---
 drivers/soc/qcom/llcc-qcom.c       | 65 +++++++++++++++++++++++++++++-
 include/linux/soc/qcom/llcc-qcom.h |  2 +-
 2 files changed, 65 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
index 3c29612da1c5..d2826158ae60 100644
--- a/drivers/soc/qcom/llcc-qcom.c
+++ b/drivers/soc/qcom/llcc-qcom.c
@@ -187,7 +187,7 @@ static const struct llcc_slice_config sc8280xp_data[] = {
 	{ LLCC_MMUHWT,   13, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
 	{ LLCC_DISP,     16, 6144, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
 	{ LLCC_AUDHW,    22, 2048, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
-	{ LLCC_DRE,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
+	{ LLCC_ECC,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
 	{ LLCC_CVP,      28, 512,  3, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
 	{ LLCC_APTCM,    30, 1024, 3, 1, 0x0,   0x1, 1, 0, 0, 1, 0, 0 },
 	{ LLCC_WRCACHE,  31, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
@@ -358,6 +358,36 @@ static const struct llcc_slice_config sm8550_data[] =  {
 	{LLCC_VIDVSP,   28,  256, 4, 1, 0xFFFFFF, 0x0,   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, },
 };
 
+static const struct llcc_slice_config qdu1000_data_2ch[] = {
+	{LLCC_MDMHPGRW, 7, 512, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_MODHW,    9, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_MDMPNG,  21, 256, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_ECC,     26, 512, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
+	{LLCC_MODPE,   29, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_APTCM,   30, 256, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
+	{LLCC_WRCACHE, 31, 128, 1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
+};
+
+static const struct llcc_slice_config qdu1000_data_4ch[] = {
+	{LLCC_MDMHPGRW, 7, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_MODHW,    9, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_MDMPNG,  21, 512,  0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_ECC,     26, 1024, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
+	{LLCC_MODPE,   29, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_APTCM,   30, 512,  3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
+	{LLCC_WRCACHE, 31, 256,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
+};
+
+static const struct llcc_slice_config qdu1000_data_8ch[] = {
+	{LLCC_MDMHPGRW, 7, 2048, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_MODHW,    9, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_MDMPNG,  21, 1024, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_ECC,     26, 2048, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
+	{LLCC_MODPE,   29, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
+	{LLCC_APTCM,   30, 1024, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
+	{LLCC_WRCACHE, 31, 512,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
+};
+
 static const struct llcc_edac_reg_offset llcc_v1_edac_reg_offset = {
 	.trp_ecc_error_status0 = 0x20344,
 	.trp_ecc_error_status1 = 0x20348,
@@ -557,6 +587,38 @@ static const struct qcom_llcc_config sm8550_cfg[] = {
 	{ },
 };
 
+static const struct qcom_llcc_config qdu1000_cfg[] = {
+	{
+		.sct_data       = qdu1000_data_8ch,
+		.size		= ARRAY_SIZE(qdu1000_data_8ch),
+		.need_llcc_cfg	= true,
+		.reg_offset	= llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+	{
+		.sct_data       = qdu1000_data_4ch,
+		.size           = ARRAY_SIZE(qdu1000_data_4ch),
+		.need_llcc_cfg  = true,
+		.reg_offset     = llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+	{
+		.sct_data       = qdu1000_data_4ch,
+		.size           = ARRAY_SIZE(qdu1000_data_4ch),
+		.need_llcc_cfg  = true,
+		.reg_offset     = llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+	{
+		.sct_data       = qdu1000_data_2ch,
+		.size           = ARRAY_SIZE(qdu1000_data_2ch),
+		.need_llcc_cfg  = true,
+		.reg_offset     = llcc_v2_1_reg_offset,
+		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
+	},
+	{ },
+};
+
 static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
 
 /**
@@ -1114,6 +1176,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
 }
 
 static const struct of_device_id qcom_llcc_of_match[] = {
+	{ .compatible = "qcom,qdu1000-llcc", .data = qdu1000_cfg},
 	{ .compatible = "qcom,sc7180-llcc", .data = sc7180_cfg },
 	{ .compatible = "qcom,sc7280-llcc", .data = sc7280_cfg },
 	{ .compatible = "qcom,sc8180x-llcc", .data = sc8180x_cfg },
diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
index 93417ba1ead4..1a886666bbb6 100644
--- a/include/linux/soc/qcom/llcc-qcom.h
+++ b/include/linux/soc/qcom/llcc-qcom.h
@@ -30,7 +30,7 @@
 #define LLCC_NPU         23
 #define LLCC_WLHW        24
 #define LLCC_PIMEM       25
-#define LLCC_DRE         26
+#define LLCC_ECC         26
 #define LLCC_CVP         28
 #define LLCC_MODPE       29
 #define LLCC_APTCM       30
-- 
2.40.1

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Dmitry Baryshkov]

On Fri, 23 Jun 2023 at 17:19, Komal Bajaj <quic_kbajaj@quicinc.com> wrote:
>
> Add LLCC support for multi channel DDR configuration
> based on a feature register. Reading DDR channel
> confiuration uses nvmem framework, so select the
> dependency in Kconfig. Without this, there will be
> errors while building the driver with COMPILE_TEST only.
>
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/soc/qcom/Kconfig     |  2 ++
>  drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718f8064..cc9ad41c63aa 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -64,6 +64,8 @@ config QCOM_LLCC
>         tristate "Qualcomm Technologies, Inc. LLCC driver"
>         depends on ARCH_QCOM || COMPILE_TEST
>         select REGMAP_MMIO
> +       select NVMEM

No need to select NVMEM. The used functions are stubbed if NVMEM is disabled

> +       select QCOM_SCM
>         help
>           Qualcomm Technologies, Inc. platform specific
>           Last Level Cache Controller(LLCC) driver for platforms such as,
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 6cf373da5df9..3c29612da1c5 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -12,6 +12,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>         return ret;
>  }
>
> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
> +{
> +       int ret;
> +
> +       ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
> +       if (ret == -ENOENT) {

|| ret == -EOPNOTSUPP ?

> +               *cfg_index = 0;
> +               return 0;
> +       }
> +
> +       return ret;
> +}
> +
>  static int qcom_llcc_remove(struct platform_device *pdev)
>  {
>         /* Set the global pointer to a error code to avoid referencing it */
> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>         struct device *dev = &pdev->dev;
>         int ret, i;
>         struct platform_device *llcc_edac;
> -       const struct qcom_llcc_config *cfg;
> +       const struct qcom_llcc_config *cfg, *entry;
>         const struct llcc_slice_config *llcc_cfg;
>         u32 sz;
> +       u8 cfg_index;
>         u32 version;
>         struct regmap *regmap;
> +       u32 num_entries = 0;
>
>         drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>         if (!drv_data) {
> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>
>         drv_data->version = version;
>
> -       llcc_cfg = cfg[0]->sct_data;
> -       sz = cfg[0]->size;
> +       ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
> +       if (ret)
> +               goto err;
> +
> +       for (entry = cfg; entry->sct_data; entry++, num_entries++)
> +               ;

Please add num_cfgs to the configuration data instead.

> +       if (cfg_index >= num_entries || cfg_index < 0) {

cfg_index is unsigned, so it can not be less than 0.

> +               ret = -EINVAL;
> +               goto err;
> +       }
> +
> +       llcc_cfg = cfg[cfg_index].sct_data;
> +       sz = cfg[cfg_index].size;
>
>         for (i = 0; i < sz; i++)
>                 if (llcc_cfg[i].slice_id > drv_data->max_slices)
> --
> 2.40.1
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 6/6] soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support

[Dmitry Baryshkov]

On Fri, 23 Jun 2023 at 17:19, Komal Bajaj <quic_kbajaj@quicinc.com> wrote:
>
> Add LLCC configuration data for QDU1000 and QRU1000 SoCs
> and updating macro name for LLCC_DRE to LLCC_ECC as per
> the latest specification.

Two commits please.

>
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c       | 65 +++++++++++++++++++++++++++++-
>  include/linux/soc/qcom/llcc-qcom.h |  2 +-
>  2 files changed, 65 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 3c29612da1c5..d2826158ae60 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -187,7 +187,7 @@ static const struct llcc_slice_config sc8280xp_data[] = {
>         { LLCC_MMUHWT,   13, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
>         { LLCC_DISP,     16, 6144, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>         { LLCC_AUDHW,    22, 2048, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
> -       { LLCC_DRE,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
> +       { LLCC_ECC,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>         { LLCC_CVP,      28, 512,  3, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>         { LLCC_APTCM,    30, 1024, 3, 1, 0x0,   0x1, 1, 0, 0, 1, 0, 0 },
>         { LLCC_WRCACHE,  31, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
> @@ -358,6 +358,36 @@ static const struct llcc_slice_config sm8550_data[] =  {
>         {LLCC_VIDVSP,   28,  256, 4, 1, 0xFFFFFF, 0x0,   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, },
>  };
>
> +static const struct llcc_slice_config qdu1000_data_2ch[] = {
> +       {LLCC_MDMHPGRW, 7, 512, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_MODHW,    9, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_MDMPNG,  21, 256, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_ECC,     26, 512, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +       {LLCC_MODPE,   29, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_APTCM,   30, 256, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_WRCACHE, 31, 128, 1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +};
> +
> +static const struct llcc_slice_config qdu1000_data_4ch[] = {
> +       {LLCC_MDMHPGRW, 7, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_MODHW,    9, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_MDMPNG,  21, 512,  0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_ECC,     26, 1024, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +       {LLCC_MODPE,   29, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_APTCM,   30, 512,  3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_WRCACHE, 31, 256,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +};
> +
> +static const struct llcc_slice_config qdu1000_data_8ch[] = {
> +       {LLCC_MDMHPGRW, 7, 2048, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_MODHW,    9, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_MDMPNG,  21, 1024, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_ECC,     26, 2048, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +       {LLCC_MODPE,   29, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_APTCM,   30, 1024, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
> +       {LLCC_WRCACHE, 31, 512,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +};
> +
>  static const struct llcc_edac_reg_offset llcc_v1_edac_reg_offset = {
>         .trp_ecc_error_status0 = 0x20344,
>         .trp_ecc_error_status1 = 0x20348,
> @@ -557,6 +587,38 @@ static const struct qcom_llcc_config sm8550_cfg[] = {
>         { },
>  };
>
> +static const struct qcom_llcc_config qdu1000_cfg[] = {
> +       {
> +               .sct_data       = qdu1000_data_8ch,
> +               .size           = ARRAY_SIZE(qdu1000_data_8ch),
> +               .need_llcc_cfg  = true,
> +               .reg_offset     = llcc_v2_1_reg_offset,
> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +       },
> +       {
> +               .sct_data       = qdu1000_data_4ch,
> +               .size           = ARRAY_SIZE(qdu1000_data_4ch),
> +               .need_llcc_cfg  = true,
> +               .reg_offset     = llcc_v2_1_reg_offset,
> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +       },
> +       {
> +               .sct_data       = qdu1000_data_4ch,
> +               .size           = ARRAY_SIZE(qdu1000_data_4ch),
> +               .need_llcc_cfg  = true,
> +               .reg_offset     = llcc_v2_1_reg_offset,
> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +       },
> +       {
> +               .sct_data       = qdu1000_data_2ch,
> +               .size           = ARRAY_SIZE(qdu1000_data_2ch),
> +               .need_llcc_cfg  = true,
> +               .reg_offset     = llcc_v2_1_reg_offset,
> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +       },
> +       { },
> +};
> +
>  static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>
>  /**
> @@ -1114,6 +1176,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  }
>
>  static const struct of_device_id qcom_llcc_of_match[] = {
> +       { .compatible = "qcom,qdu1000-llcc", .data = qdu1000_cfg},
>         { .compatible = "qcom,sc7180-llcc", .data = sc7180_cfg },
>         { .compatible = "qcom,sc7280-llcc", .data = sc7280_cfg },
>         { .compatible = "qcom,sc8180x-llcc", .data = sc8180x_cfg },
> diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
> index 93417ba1ead4..1a886666bbb6 100644
> --- a/include/linux/soc/qcom/llcc-qcom.h
> +++ b/include/linux/soc/qcom/llcc-qcom.h
> @@ -30,7 +30,7 @@
>  #define LLCC_NPU         23
>  #define LLCC_WLHW        24
>  #define LLCC_PIMEM       25
> -#define LLCC_DRE         26
> +#define LLCC_ECC         26
>  #define LLCC_CVP         28
>  #define LLCC_MODPE       29
>  #define LLCC_APTCM       30
> --
> 2.40.1
>


-- 
With best wishes
Dmitry

Re: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.

[Konrad Dybcio]

On 23.06.2023 16:18, 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. Ensuring
> the address to read is word aligned since our secure I/O
> only supports word size I/O.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/nvmem/Kconfig      |  12 ++++
>  drivers/nvmem/Makefile     |   2 +
>  drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 130 insertions(+)
>  create mode 100644 drivers/nvmem/sec-qfprom.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c7..2b0dd73d899e 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -216,6 +216,18 @@ 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
> +        select QCOM_SCM
You also need OF

> +        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..4b4bf5880488 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..47b2c71593dd
> --- /dev/null
> +++ b/drivers/nvmem/sec-qfprom.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk.h>
> +#include <linux/device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +
> +/**
> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
> + *
> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
> + * @dev: qfprom device structure.
> + */
> +struct sec_qfprom_priv {
> +	phys_addr_t qfpseccorrected;
> +	struct device *dev;
> +};
> +
> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> +{
> +	struct sec_qfprom_priv *priv = context;
> +	u8 *val = _val;
> +	u8 *tmp;
> +	u32 read_val;
> +	unsigned int i;
Please sort this to look like a reverse-Christmas-tree


> +
> +	for (i = 0; i < bytes; i++, reg++) {
> +		if (i == 0 || reg % 4 == 0) {
> +			if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
> +				dev_err(priv->dev, "Couldn't access fuse register\n");
> +				return -EINVAL;
> +			}
> +			tmp = (u8 *)&read_val;
> +		}
I don't understand this read-4-times dance.. qcom_scm_io_readl() reads
u32, so this should be as simple as:

val[i + 0] = FIELD_GET(GENMASK(7, 0), reg);
val[i + 1] = ..
val[i + 2] = ..
val[i + 3] = ..


> +
> +		val[i] = tmp[reg & 3];
> +	}
> +
> +	return 0;
> +}
> +
> +static void sec_qfprom_runtime_disable(void *data)
> +{
> +	pm_runtime_disable(data);
> +}
> +
> +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 resource *res;
> +	struct nvmem_device *nvmem;
> +	struct sec_qfprom_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->qfpseccorrected = res->start;
> +	if (!priv->qfpseccorrected)
> +		return -ENOMEM;
While it works all the same, I think checking if(!res) would be more
logical

Also, ENOMEM seems weird here.. Perhaps EINVAL?

> +
> +	econfig.size = resource_size(res);
> +	econfig.dev = dev;
> +	econfig.priv = priv;
> +
> +	priv->dev = dev;
> +
> +	pm_runtime_enable(dev);
> +	ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
> +	if (ret)
> +		return ret;
Wouldn't devm_pm_runtime_enable() take care of this? Or do we need
for this block to be always-powered?

> +
> +	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",},
The comma inside is unnecessary, replacing it with a space would also
make the whitespacing match

> +	{/* sentinel */},
> +};
> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
> +
> +static struct platform_driver qfprom_driver = {
> +	.probe = sec_qfprom_probe,
> +	.driver = {
> +		.name = "qcom,sec_qfprom",
Commas in driver names are rather.. rare? Let's make it qcom_

> +		.of_match_table = sec_qfprom_of_match,
> +	},
> +};
> +module_platform_driver(qfprom_driver);
> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
> +MODULE_LICENSE("GPL v2");
Please run scripts/checkpatch.pl on your patches before sending.

Konrad

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Konrad Dybcio]

On 23.06.2023 16:18, Komal Bajaj wrote:
> Add LLCC support for multi channel DDR configuration
> based on a feature register. Reading DDR channel
> confiuration uses nvmem framework, so select the
> dependency in Kconfig. Without this, there will be
> errors while building the driver with COMPILE_TEST only.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/soc/qcom/Kconfig     |  2 ++
>  drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718f8064..cc9ad41c63aa 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -64,6 +64,8 @@ config QCOM_LLCC
>  	tristate "Qualcomm Technologies, Inc. LLCC driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
>  	select REGMAP_MMIO
> +	select NVMEM
> +	select QCOM_SCM
>  	help
>  	  Qualcomm Technologies, Inc. platform specific
>  	  Last Level Cache Controller(LLCC) driver for platforms such as,
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 6cf373da5df9..3c29612da1c5 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -12,6 +12,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
> +{
> +	int ret;
> +
> +	ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
> +	if (ret == -ENOENT) {
> +		*cfg_index = 0;
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int qcom_llcc_remove(struct platform_device *pdev)
>  {
>  	/* Set the global pointer to a error code to avoid referencing it */
> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret, i;
>  	struct platform_device *llcc_edac;
> -	const struct qcom_llcc_config *cfg;
> +	const struct qcom_llcc_config *cfg, *entry;
>  	const struct llcc_slice_config *llcc_cfg;
>  	u32 sz;
> +	u8 cfg_index;
>  	u32 version;
>  	struct regmap *regmap;
> +	u32 num_entries = 0;
>  
>  	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>  	if (!drv_data) {
> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  
>  	drv_data->version = version;
>  
> -	llcc_cfg = cfg[0]->sct_data;
> -	sz = cfg[0]->size;
> +	ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
> +	if (ret)
> +		goto err;
> +


> +	for (entry = cfg; entry->sct_data; entry++, num_entries++)
> +		;
> +	if (cfg_index >= num_entries || cfg_index < 0) {
cfg_index is an unsigned variable, it can never be < 0

> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
if (cfg_index >= entry->size)? With that, you can also keep the config
entries non-0-terminated in the previous patch, saving a whole lot of RAM.

Konrad
> +	llcc_cfg = cfg[cfg_index].sct_data;
> +	sz = cfg[cfg_index].size;
>  
>  	for (i = 0; i < sz; i++)
>  		if (llcc_cfg[i].slice_id > drv_data->max_slices)

Re: [PATCH v4 6/6] soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support

[Konrad Dybcio]

On 23.06.2023 16:18, Komal Bajaj wrote:
> Add LLCC configuration data for QDU1000 and QRU1000 SoCs
> and updating macro name for LLCC_DRE to LLCC_ECC as per
> the latest specification.
> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/soc/qcom/llcc-qcom.c       | 65 +++++++++++++++++++++++++++++-
>  include/linux/soc/qcom/llcc-qcom.h |  2 +-
>  2 files changed, 65 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 3c29612da1c5..d2826158ae60 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -187,7 +187,7 @@ static const struct llcc_slice_config sc8280xp_data[] = {
>  	{ LLCC_MMUHWT,   13, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
>  	{ LLCC_DISP,     16, 6144, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>  	{ LLCC_AUDHW,    22, 2048, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
> -	{ LLCC_DRE,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
> +	{ LLCC_ECC,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>  	{ LLCC_CVP,      28, 512,  3, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>  	{ LLCC_APTCM,    30, 1024, 3, 1, 0x0,   0x1, 1, 0, 0, 1, 0, 0 },
>  	{ LLCC_WRCACHE,  31, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
> @@ -358,6 +358,36 @@ static const struct llcc_slice_config sm8550_data[] =  {
>  	{LLCC_VIDVSP,   28,  256, 4, 1, 0xFFFFFF, 0x0,   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, },
>  };
>  
> +static const struct llcc_slice_config qdu1000_data_2ch[] = {
> +	{LLCC_MDMHPGRW, 7, 512, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_MODHW,    9, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_MDMPNG,  21, 256, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_ECC,     26, 512, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +	{LLCC_MODPE,   29, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_APTCM,   30, 256, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_WRCACHE, 31, 128, 1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
Please keep the style coherent with existing entries, so:

- spacing between curly braces and data
- tabs/spaces
- hex should be lowercase

Konrad
> +};
> +
> +static const struct llcc_slice_config qdu1000_data_4ch[] = {
> +	{LLCC_MDMHPGRW, 7, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_MODHW,    9, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_MDMPNG,  21, 512,  0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_ECC,     26, 1024, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +	{LLCC_MODPE,   29, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_APTCM,   30, 512,  3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_WRCACHE, 31, 256,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +};
> +
> +static const struct llcc_slice_config qdu1000_data_8ch[] = {
> +	{LLCC_MDMHPGRW, 7, 2048, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_MODHW,    9, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_MDMPNG,  21, 1024, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_ECC,     26, 2048, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +	{LLCC_MODPE,   29, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_APTCM,   30, 1024, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
> +	{LLCC_WRCACHE, 31, 512,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
> +};
> +
>  static const struct llcc_edac_reg_offset llcc_v1_edac_reg_offset = {
>  	.trp_ecc_error_status0 = 0x20344,
>  	.trp_ecc_error_status1 = 0x20348,
> @@ -557,6 +587,38 @@ static const struct qcom_llcc_config sm8550_cfg[] = {
>  	{ },
>  };
>  
> +static const struct qcom_llcc_config qdu1000_cfg[] = {
> +	{
> +		.sct_data       = qdu1000_data_8ch,
> +		.size		= ARRAY_SIZE(qdu1000_data_8ch),
> +		.need_llcc_cfg	= true,
> +		.reg_offset	= llcc_v2_1_reg_offset,
> +		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +	},
> +	{
> +		.sct_data       = qdu1000_data_4ch,
> +		.size           = ARRAY_SIZE(qdu1000_data_4ch),
> +		.need_llcc_cfg  = true,
> +		.reg_offset     = llcc_v2_1_reg_offset,
> +		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +	},
> +	{
> +		.sct_data       = qdu1000_data_4ch,
> +		.size           = ARRAY_SIZE(qdu1000_data_4ch),
> +		.need_llcc_cfg  = true,
> +		.reg_offset     = llcc_v2_1_reg_offset,
> +		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +	},
> +	{
> +		.sct_data       = qdu1000_data_2ch,
> +		.size           = ARRAY_SIZE(qdu1000_data_2ch),
> +		.need_llcc_cfg  = true,
> +		.reg_offset     = llcc_v2_1_reg_offset,
> +		.edac_reg_offset = &llcc_v2_1_edac_reg_offset,
> +	},
> +	{ },
> +};
> +
>  static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>  
>  /**
> @@ -1114,6 +1176,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  }
>  
>  static const struct of_device_id qcom_llcc_of_match[] = {
> +	{ .compatible = "qcom,qdu1000-llcc", .data = qdu1000_cfg},
>  	{ .compatible = "qcom,sc7180-llcc", .data = sc7180_cfg },
>  	{ .compatible = "qcom,sc7280-llcc", .data = sc7280_cfg },
>  	{ .compatible = "qcom,sc8180x-llcc", .data = sc8180x_cfg },
> diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
> index 93417ba1ead4..1a886666bbb6 100644
> --- a/include/linux/soc/qcom/llcc-qcom.h
> +++ b/include/linux/soc/qcom/llcc-qcom.h
> @@ -30,7 +30,7 @@
>  #define LLCC_NPU         23
>  #define LLCC_WLHW        24
>  #define LLCC_PIMEM       25
> -#define LLCC_DRE         26
> +#define LLCC_ECC         26
>  #define LLCC_CVP         28
>  #define LLCC_MODPE       29
>  #define LLCC_APTCM       30

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

[Krzysztof Kozlowski]

On 23/06/2023 16:18, 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..675e27918c7b
> --- /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

SECURE is not acronym, so "Secure".

> +
> +maintainers:
> +  - Komal Bajaj <quic_kbajaj@quicinc.com>

Add description: with explanation what is this.  Specifically it should
be quite clear what is here different than regular QFPROM

> +
> +allOf:
> +  - $ref: nvmem.yaml#
> +
> +properties:
> +  compatible:
> +    items:
> +      - enum:
> +          - qcom,qdu1000-sec-qfprom
> +      - const: qcom,sec-qfprom
> +
> +  reg:
> +    items:
> +      - description: The secure qfprom corrected region.
> +
> +  # Needed if any child nodes are present.
> +  "#address-cells":
> +    const: 1
> +  "#size-cells":
> +    const: 1

Drop both, they are not needed.

> +
> +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 {

No underscores in node names.


Best regards,
Krzysztof

Re: [PATCH v4 2/6] dt-bindings: cache: qcom,llcc: Add LLCC compatible for QDU1000/QRU1000

[Krzysztof Kozlowski]

On 23/06/2023 16:18, Komal Bajaj wrote:
> Add LLCC compatible for QDU1000/QRU1000 SoCs and add optional
> nvmem-cells and nvmem-cell-names property.

Your commit should explain why you are doing this. I can easily see from
the diff that you add nvmem-cells. But why?

> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  Documentation/devicetree/bindings/cache/qcom,llcc.yaml | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 


Best regards,
Krzysztof

Re: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.

[kernel test robot]

Hi Komal,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linus/master v6.4-rc7 next-20230623]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Komal-Bajaj/dt-bindings-nvmem-sec-qfprom-Add-bindings-for-secure-qfprom/20230623-222054
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20230623141806.13388-4-quic_kbajaj%40quicinc.com
patch subject: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.
config: arm-allmodconfig (https://download.01.org/0day-ci/archive/20230624/202306240837.Keyvmt2I-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.3.0
reproduce: (https://download.01.org/0day-ci/archive/20230624/202306240837.Keyvmt2I-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202306240837.Keyvmt2I-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> drivers/nvmem/sec-qfprom.c:31: warning: expecting prototype for struct sec_sec_qfprom_priv. Prototype was for struct sec_qfprom_priv instead


vim +31 drivers/nvmem/sec-qfprom.c

    20	
    21	
    22	/**
    23	 * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
    24	 *
    25	 * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
    26	 * @dev: qfprom device structure.
    27	 */
    28	struct sec_qfprom_priv {
    29		phys_addr_t qfpseccorrected;
    30		struct device *dev;
  > 31	};
    32	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

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

[Komal Bajaj]

On 6/23/2023 10:06 PM, Krzysztof Kozlowski wrote:
> On 23/06/2023 16:18, 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..675e27918c7b
>> --- /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
> SECURE is not acronym, so "Secure".

Noted.

>
>> +
>> +maintainers:
>> +  - Komal Bajaj <quic_kbajaj@quicinc.com>
> Add description: with explanation what is this.  Specifically it should
> be quite clear what is here different than regular QFPROM

Sure, will add description the next patch set.

>
>> +
>> +allOf:
>> +  - $ref: nvmem.yaml#
>> +
>> +properties:
>> +  compatible:
>> +    items:
>> +      - enum:
>> +          - qcom,qdu1000-sec-qfprom
>> +      - const: qcom,sec-qfprom
>> +
>> +  reg:
>> +    items:
>> +      - description: The secure qfprom corrected region.
>> +
>> +  # Needed if any child nodes are present.
>> +  "#address-cells":
>> +    const: 1
>> +  "#size-cells":
>> +    const: 1
> Drop both, they are not needed.

I didn't get it. Can you please explain why these are not needed as this
node will have child nodes which will use single value for address and size.

>
>> +
>> +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 {
> No underscores in node names.

Noted.

>
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 26/06/2023 10:22, Komal Bajaj wrote:
>>
>>> +
>>> +allOf:
>>> +  - $ref: nvmem.yaml#
>>> +
>>> +properties:
>>> +  compatible:
>>> +    items:
>>> +      - enum:
>>> +          - qcom,qdu1000-sec-qfprom
>>> +      - const: qcom,sec-qfprom
>>> +
>>> +  reg:
>>> +    items:
>>> +      - description: The secure qfprom corrected region.
>>> +
>>> +  # Needed if any child nodes are present.
>>> +  "#address-cells":
>>> +    const: 1
>>> +  "#size-cells":
>>> +    const: 1
>> Drop both, they are not needed.
> 
> I didn't get it. Can you please explain why these are not needed as this
> node will have child nodes which will use single value for address and size.

I suspect they are already defined. Do other bindings (for cases with
children) have them? If not, why here it would be different?


Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 26/06/2023 11:02, Komal Bajaj wrote:
> 
> 
> On 6/26/2023 2:00 PM, Krzysztof Kozlowski wrote:
>> On 26/06/2023 10:22, Komal Bajaj wrote:
>>>>> +
>>>>> +allOf:
>>>>> +  - $ref: nvmem.yaml#
>>>>> +
>>>>> +properties:
>>>>> +  compatible:
>>>>> +    items:
>>>>> +      - enum:
>>>>> +          - qcom,qdu1000-sec-qfprom
>>>>> +      - const: qcom,sec-qfprom
>>>>> +
>>>>> +  reg:
>>>>> +    items:
>>>>> +      - description: The secure qfprom corrected region.
>>>>> +
>>>>> +  # Needed if any child nodes are present.
>>>>> +  "#address-cells":
>>>>> +    const: 1
>>>>> +  "#size-cells":
>>>>> +    const: 1
>>>> Drop both, they are not needed.
>>> I didn't get it. Can you please explain why these are not needed as this
>>> node will have child nodes which will use single value for address and size.
>> I suspect they are already defined. Do other bindings (for cases with
>> children) have them? If not, why here it would be different?
> 
> Yes, I see there are bindings that has these properties, listed a few of 
> them below -
> 
> [1] 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml

Please work on current development. It's a bit of waste of time to
review old code...

https://lore.kernel.org/all/20230611140330.154222-16-srinivas.kandagatla@linaro.org/

> [2] 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/firmware/arm,scmi.yaml

That's not a nvmem provider.

> [3] 
> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/timer/arm,arch_timer_mmio.yaml

That's not a nvmem provider.


Best regards,
Krzysztof

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

[Komal Bajaj]

On 6/26/2023 3:16 PM, Krzysztof Kozlowski wrote:
> On 26/06/2023 11:02, Komal Bajaj wrote:
>>
>> On 6/26/2023 2:00 PM, Krzysztof Kozlowski wrote:
>>> On 26/06/2023 10:22, Komal Bajaj wrote:
>>>>>> +
>>>>>> +allOf:
>>>>>> +  - $ref: nvmem.yaml#
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    items:
>>>>>> +      - enum:
>>>>>> +          - qcom,qdu1000-sec-qfprom
>>>>>> +      - const: qcom,sec-qfprom
>>>>>> +
>>>>>> +  reg:
>>>>>> +    items:
>>>>>> +      - description: The secure qfprom corrected region.
>>>>>> +
>>>>>> +  # Needed if any child nodes are present.
>>>>>> +  "#address-cells":
>>>>>> +    const: 1
>>>>>> +  "#size-cells":
>>>>>> +    const: 1
>>>>> Drop both, they are not needed.
>>>> I didn't get it. Can you please explain why these are not needed as this
>>>> node will have child nodes which will use single value for address and size.
>>> I suspect they are already defined. Do other bindings (for cases with
>>> children) have them? If not, why here it would be different?
>> Yes, I see there are bindings that has these properties, listed a few of
>> them below -
>>
>> [1]
>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/nvmem/qcom,qfprom.yaml
> Please work on current development. It's a bit of waste of time to
> review old code...

Okay sorry for that, will work on this.

Thanks
Komal
>
> https://lore.kernel.org/all/20230611140330.154222-16-srinivas.kandagatla@linaro.org/
>
>> [2]
>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/firmware/arm,scmi.yaml
> That's not a nvmem provider.
>
>> [3]
>> https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/timer/arm,arch_timer_mmio.yaml
> That's not a nvmem provider.
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.

[Komal Bajaj]

On 6/23/2023 8:23 PM, Konrad Dybcio wrote:
> On 23.06.2023 16:18, 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. Ensuring
>> the address to read is word aligned since our secure I/O
>> only supports word size I/O.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   drivers/nvmem/Kconfig      |  12 ++++
>>   drivers/nvmem/Makefile     |   2 +
>>   drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
>>   3 files changed, 130 insertions(+)
>>   create mode 100644 drivers/nvmem/sec-qfprom.c
>>
>> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
>> index b291b27048c7..2b0dd73d899e 100644
>> --- a/drivers/nvmem/Kconfig
>> +++ b/drivers/nvmem/Kconfig
>> @@ -216,6 +216,18 @@ 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
>> +        select QCOM_SCM
> You also need OF

Noted.

>
>> +        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..4b4bf5880488 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..47b2c71593dd
>> --- /dev/null
>> +++ b/drivers/nvmem/sec-qfprom.c
>> @@ -0,0 +1,116 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +
>> +#include <linux/clk.h>
>> +#include <linux/device.h>
>> +#include <linux/firmware/qcom/qcom_scm.h>
>> +#include <linux/io.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/mod_devicetable.h>
>> +#include <linux/nvmem-provider.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/pm_domain.h>
>> +#include <linux/pm_runtime.h>
>> +#include <linux/property.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +
>> +/**
>> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
>> + *
>> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
>> + * @dev: qfprom device structure.
>> + */
>> +struct sec_qfprom_priv {
>> +	phys_addr_t qfpseccorrected;
>> +	struct device *dev;
>> +};
>> +
>> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
>> +{
>> +	struct sec_qfprom_priv *priv = context;
>> +	u8 *val = _val;
>> +	u8 *tmp;
>> +	u32 read_val;
>> +	unsigned int i;
> Please sort this to look like a reverse-Christmas-tree

Okay, will sort it in that way.

>
>
>> +
>> +	for (i = 0; i < bytes; i++, reg++) {
>> +		if (i == 0 || reg % 4 == 0) {
>> +			if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
>> +				dev_err(priv->dev, "Couldn't access fuse register\n");
>> +				return -EINVAL;
>> +			}
>> +			tmp = (u8 *)&read_val;
>> +		}
> I don't understand this read-4-times dance.. qcom_scm_io_readl() reads
> u32, so this should be as simple as:
>
> val[i + 0] = FIELD_GET(GENMASK(7, 0), reg);
> val[i + 1] = ..
> val[i + 2] = ..
> val[i + 3] = ..

Won't it get too complex, I type-casted 32-bit read_val into u8 pointer, 
so that I can
easily use it for the byte-level access of read_val's value.

Doing the way that you mentioned would be something like below -
val[i] = FIELD_GET(GENMASK((reg&3)*8+7, (reg&3)*8), read_val);

Thanks
Komal
>
>> +
>> +		val[i] = tmp[reg & 3];
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static void sec_qfprom_runtime_disable(void *data)
>> +{
>> +	pm_runtime_disable(data);
>> +}
>> +
>> +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 resource *res;
>> +	struct nvmem_device *nvmem;
>> +	struct sec_qfprom_priv *priv;
>> +	int ret;
>> +
>> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
>> +	if (!priv)
>> +		return -ENOMEM;
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	priv->qfpseccorrected = res->start;
>> +	if (!priv->qfpseccorrected)
>> +		return -ENOMEM;
> While it works all the same, I think checking if(!res) would be more
> logical
>
> Also, ENOMEM seems weird here.. Perhaps EINVAL?
>
>> +
>> +	econfig.size = resource_size(res);
>> +	econfig.dev = dev;
>> +	econfig.priv = priv;
>> +
>> +	priv->dev = dev;
>> +
>> +	pm_runtime_enable(dev);
>> +	ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
>> +	if (ret)
>> +		return ret;
> Wouldn't devm_pm_runtime_enable() take care of this? Or do we need
> for this block to be always-powered?
>
>> +
>> +	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",},
> The comma inside is unnecessary, replacing it with a space would also
> make the whitespacing match
>
>> +	{/* sentinel */},
>> +};
>> +MODULE_DEVICE_TABLE(of, sec_qfprom_of_match);
>> +
>> +static struct platform_driver qfprom_driver = {
>> +	.probe = sec_qfprom_probe,
>> +	.driver = {
>> +		.name = "qcom,sec_qfprom",
> Commas in driver names are rather.. rare? Let's make it qcom_
>
>> +		.of_match_table = sec_qfprom_of_match,
>> +	},
>> +};
>> +module_platform_driver(qfprom_driver);
>> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
>> +MODULE_LICENSE("GPL v2");
> Please run scripts/checkpatch.pl on your patches before sending.
>
> Konrad

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Komal Bajaj]

On 6/23/2023 8:28 PM, Konrad Dybcio wrote:
> On 23.06.2023 16:18, Komal Bajaj wrote:
>> Add LLCC support for multi channel DDR configuration
>> based on a feature register. Reading DDR channel
>> confiuration uses nvmem framework, so select the
>> dependency in Kconfig. Without this, there will be
>> errors while building the driver with COMPILE_TEST only.
>>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   drivers/soc/qcom/Kconfig     |  2 ++
>>   drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>>   2 files changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>> index a491718f8064..cc9ad41c63aa 100644
>> --- a/drivers/soc/qcom/Kconfig
>> +++ b/drivers/soc/qcom/Kconfig
>> @@ -64,6 +64,8 @@ config QCOM_LLCC
>>   	tristate "Qualcomm Technologies, Inc. LLCC driver"
>>   	depends on ARCH_QCOM || COMPILE_TEST
>>   	select REGMAP_MMIO
>> +	select NVMEM
>> +	select QCOM_SCM
>>   	help
>>   	  Qualcomm Technologies, Inc. platform specific
>>   	  Last Level Cache Controller(LLCC) driver for platforms such as,
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 6cf373da5df9..3c29612da1c5 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -12,6 +12,7 @@
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>>   #include <linux/mutex.h>
>> +#include <linux/nvmem-consumer.h>
>>   #include <linux/of.h>
>>   #include <linux/of_device.h>
>>   #include <linux/regmap.h>
>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>>   	return ret;
>>   }
>>   
>> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
>> +{
>> +	int ret;
>> +
>> +	ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
>> +	if (ret == -ENOENT) {
>> +		*cfg_index = 0;
>> +		return 0;
>> +	}
>> +
>> +	return ret;
>> +}
>> +
>>   static int qcom_llcc_remove(struct platform_device *pdev)
>>   {
>>   	/* Set the global pointer to a error code to avoid referencing it */
>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>   	struct device *dev = &pdev->dev;
>>   	int ret, i;
>>   	struct platform_device *llcc_edac;
>> -	const struct qcom_llcc_config *cfg;
>> +	const struct qcom_llcc_config *cfg, *entry;
>>   	const struct llcc_slice_config *llcc_cfg;
>>   	u32 sz;
>> +	u8 cfg_index;
>>   	u32 version;
>>   	struct regmap *regmap;
>> +	u32 num_entries = 0;
>>   
>>   	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>>   	if (!drv_data) {
>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>   
>>   	drv_data->version = version;
>>   
>> -	llcc_cfg = cfg[0]->sct_data;
>> -	sz = cfg[0]->size;
>> +	ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
>> +	if (ret)
>> +		goto err;
>> +
>
>> +	for (entry = cfg; entry->sct_data; entry++, num_entries++)
>> +		;
>> +	if (cfg_index >= num_entries || cfg_index < 0) {
> cfg_index is an unsigned variable, it can never be < 0

Okay, will remove this condition.

>
>> +		ret = -EINVAL;
>> +		goto err;
>> +	}
>> +
> if (cfg_index >= entry->size)? With that, you can also keep the config
> entries non-0-terminated in the previous patch, saving a whole lot of RAM.

entry->size represents the size of sct table whereas num_entries 
represents the number
of sct tables that we can have. And by this check we are validating the 
value read from the
fuse register. Am I understanding your comment correctly?

>
> Konrad
>> +	llcc_cfg = cfg[cfg_index].sct_data;
>> +	sz = cfg[cfg_index].size;
>>   
>>   	for (i = 0; i < sz; i++)
>>   		if (llcc_cfg[i].slice_id > drv_data->max_slices)

Re: [PATCH v4 6/6] soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support

[Komal Bajaj]

On 6/23/2023 7:57 PM, Dmitry Baryshkov wrote:
> On Fri, 23 Jun 2023 at 17:19, Komal Bajaj <quic_kbajaj@quicinc.com> wrote:
>> Add LLCC configuration data for QDU1000 and QRU1000 SoCs
>> and updating macro name for LLCC_DRE to LLCC_ECC as per
>> the latest specification.
> Two commits please.

Okay, will split this into two commits.

Thanks
Komal

>
>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>> ---
>>   drivers/soc/qcom/llcc-qcom.c       | 65 +++++++++++++++++++++++++++++-
>>   include/linux/soc/qcom/llcc-qcom.h |  2 +-
>>   2 files changed, 65 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>> index 3c29612da1c5..d2826158ae60 100644
>> --- a/drivers/soc/qcom/llcc-qcom.c
>> +++ b/drivers/soc/qcom/llcc-qcom.c
>> @@ -187,7 +187,7 @@ static const struct llcc_slice_config sc8280xp_data[] = {
>>          { LLCC_MMUHWT,   13, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
>>          { LLCC_DISP,     16, 6144, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>>          { LLCC_AUDHW,    22, 2048, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>> -       { LLCC_DRE,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>> +       { LLCC_ECC,      26, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>>          { LLCC_CVP,      28, 512,  3, 1, 0xfff, 0x0, 0, 0, 0, 1, 0, 0 },
>>          { LLCC_APTCM,    30, 1024, 3, 1, 0x0,   0x1, 1, 0, 0, 1, 0, 0 },
>>          { LLCC_WRCACHE,  31, 1024, 1, 1, 0xfff, 0x0, 0, 0, 0, 0, 1, 0 },
>> @@ -358,6 +358,36 @@ static const struct llcc_slice_config sm8550_data[] =  {
>>          {LLCC_VIDVSP,   28,  256, 4, 1, 0xFFFFFF, 0x0,   0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, },
>>   };
>>
>> +static const struct llcc_slice_config qdu1000_data_2ch[] = {
>> +       {LLCC_MDMHPGRW, 7, 512, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_MODHW,    9, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_MDMPNG,  21, 256, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_ECC,     26, 512, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
>> +       {LLCC_MODPE,   29, 256, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_APTCM,   30, 256, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_WRCACHE, 31, 128, 1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
>> +};
>> +
>> +static const struct llcc_slice_config qdu1000_data_4ch[] = {
>> +       {LLCC_MDMHPGRW, 7, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_MODHW,    9, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_MDMPNG,  21, 512,  0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_ECC,     26, 1024, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
>> +       {LLCC_MODPE,   29, 512,  1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_APTCM,   30, 512,  3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_WRCACHE, 31, 256,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
>> +};
>> +
>> +static const struct llcc_slice_config qdu1000_data_8ch[] = {
>> +       {LLCC_MDMHPGRW, 7, 2048, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_MODHW,    9, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_MDMPNG,  21, 1024, 0, 1,   0x3, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_ECC,     26, 2048, 3, 1, 0xFFC, 0x0, 0, 0, 0, 0, 1, 0, 0 },
>> +       {LLCC_MODPE,   29, 1024, 1, 1, 0xFFF, 0x0, 0, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_APTCM,   30, 1024, 3, 1,   0x0, 0xC, 1, 0, 0, 1, 0, 0, 0 },
>> +       {LLCC_WRCACHE, 31, 512,  1, 1,   0x3, 0x0, 0, 0, 0, 0, 1, 0, 0 },
>> +};
>> +
>>   static const struct llcc_edac_reg_offset llcc_v1_edac_reg_offset = {
>>          .trp_ecc_error_status0 = 0x20344,
>>          .trp_ecc_error_status1 = 0x20348,
>> @@ -557,6 +587,38 @@ static const struct qcom_llcc_config sm8550_cfg[] = {
>>          { },
>>   };
>>
>> +static const struct qcom_llcc_config qdu1000_cfg[] = {
>> +       {
>> +               .sct_data       = qdu1000_data_8ch,
>> +               .size           = ARRAY_SIZE(qdu1000_data_8ch),
>> +               .need_llcc_cfg  = true,
>> +               .reg_offset     = llcc_v2_1_reg_offset,
>> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
>> +       },
>> +       {
>> +               .sct_data       = qdu1000_data_4ch,
>> +               .size           = ARRAY_SIZE(qdu1000_data_4ch),
>> +               .need_llcc_cfg  = true,
>> +               .reg_offset     = llcc_v2_1_reg_offset,
>> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
>> +       },
>> +       {
>> +               .sct_data       = qdu1000_data_4ch,
>> +               .size           = ARRAY_SIZE(qdu1000_data_4ch),
>> +               .need_llcc_cfg  = true,
>> +               .reg_offset     = llcc_v2_1_reg_offset,
>> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
>> +       },
>> +       {
>> +               .sct_data       = qdu1000_data_2ch,
>> +               .size           = ARRAY_SIZE(qdu1000_data_2ch),
>> +               .need_llcc_cfg  = true,
>> +               .reg_offset     = llcc_v2_1_reg_offset,
>> +               .edac_reg_offset = &llcc_v2_1_edac_reg_offset,
>> +       },
>> +       { },
>> +};
>> +
>>   static struct llcc_drv_data *drv_data = (void *) -EPROBE_DEFER;
>>
>>   /**
>> @@ -1114,6 +1176,7 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>   }
>>
>>   static const struct of_device_id qcom_llcc_of_match[] = {
>> +       { .compatible = "qcom,qdu1000-llcc", .data = qdu1000_cfg},
>>          { .compatible = "qcom,sc7180-llcc", .data = sc7180_cfg },
>>          { .compatible = "qcom,sc7280-llcc", .data = sc7280_cfg },
>>          { .compatible = "qcom,sc8180x-llcc", .data = sc8180x_cfg },
>> diff --git a/include/linux/soc/qcom/llcc-qcom.h b/include/linux/soc/qcom/llcc-qcom.h
>> index 93417ba1ead4..1a886666bbb6 100644
>> --- a/include/linux/soc/qcom/llcc-qcom.h
>> +++ b/include/linux/soc/qcom/llcc-qcom.h
>> @@ -30,7 +30,7 @@
>>   #define LLCC_NPU         23
>>   #define LLCC_WLHW        24
>>   #define LLCC_PIMEM       25
>> -#define LLCC_DRE         26
>> +#define LLCC_ECC         26
>>   #define LLCC_CVP         28
>>   #define LLCC_MODPE       29
>>   #define LLCC_APTCM       30
>> --
>> 2.40.1
>>
>

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Konrad Dybcio]

On 28.06.2023 10:52, Komal Bajaj wrote:
> 
> 
> On 6/23/2023 8:28 PM, Konrad Dybcio wrote:
>> On 23.06.2023 16:18, Komal Bajaj wrote:
>>> Add LLCC support for multi channel DDR configuration
>>> based on a feature register. Reading DDR channel
>>> confiuration uses nvmem framework, so select the
>>> dependency in Kconfig. Without this, there will be
>>> errors while building the driver with COMPILE_TEST only.
>>>
>>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>>> ---
>>>   drivers/soc/qcom/Kconfig     |  2 ++
>>>   drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>>>   2 files changed, 32 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a491718f8064..cc9ad41c63aa 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -64,6 +64,8 @@ config QCOM_LLCC
>>>       tristate "Qualcomm Technologies, Inc. LLCC driver"
>>>       depends on ARCH_QCOM || COMPILE_TEST
>>>       select REGMAP_MMIO
>>> +    select NVMEM
>>> +    select QCOM_SCM
>>>       help
>>>         Qualcomm Technologies, Inc. platform specific
>>>         Last Level Cache Controller(LLCC) driver for platforms such as,
>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>> index 6cf373da5df9..3c29612da1c5 100644
>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>> @@ -12,6 +12,7 @@
>>>   #include <linux/kernel.h>
>>>   #include <linux/module.h>
>>>   #include <linux/mutex.h>
>>> +#include <linux/nvmem-consumer.h>
>>>   #include <linux/of.h>
>>>   #include <linux/of_device.h>
>>>   #include <linux/regmap.h>
>>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>>>       return ret;
>>>   }
>>>   +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
>>> +    if (ret == -ENOENT) {
>>> +        *cfg_index = 0;
>>> +        return 0;
>>> +    }
>>> +
>>> +    return ret;
>>> +}
>>> +
>>>   static int qcom_llcc_remove(struct platform_device *pdev)
>>>   {
>>>       /* Set the global pointer to a error code to avoid referencing it */
>>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>       struct device *dev = &pdev->dev;
>>>       int ret, i;
>>>       struct platform_device *llcc_edac;
>>> -    const struct qcom_llcc_config *cfg;
>>> +    const struct qcom_llcc_config *cfg, *entry;
>>>       const struct llcc_slice_config *llcc_cfg;
>>>       u32 sz;
>>> +    u8 cfg_index;
>>>       u32 version;
>>>       struct regmap *regmap;
>>> +    u32 num_entries = 0;
>>>         drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>>>       if (!drv_data) {
>>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>         drv_data->version = version;
>>>   -    llcc_cfg = cfg[0]->sct_data;
>>> -    sz = cfg[0]->size;
>>> +    ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
>>> +    if (ret)
>>> +        goto err;
>>> +
>>
>>> +    for (entry = cfg; entry->sct_data; entry++, num_entries++)
>>> +        ;
>>> +    if (cfg_index >= num_entries || cfg_index < 0) {
>> cfg_index is an unsigned variable, it can never be < 0
> 
> Okay, will remove this condition.
> 
>>
>>> +        ret = -EINVAL;
>>> +        goto err;
>>> +    }
>>> +
>> if (cfg_index >= entry->size)? With that, you can also keep the config
>> entries non-0-terminated in the previous patch, saving a whole lot of RAM.
> 
> entry->size represents the size of sct table whereas num_entries represents the number
> of sct tables that we can have. And by this check we are validating the value read from the
> fuse register. Am I understanding your comment correctly?
Oh you're right.

I still see room for improvement, though.

For example, you duplicate assigning need_llcc_cfg, reg_offset
and edac_reg_offset. You can add a new struct, like "sct_config" and add
a pointer to sct_config[] & the length of this array to qcom_llcc_config.

Konrad

> 
>>
>> Konrad
>>> +    llcc_cfg = cfg[cfg_index].sct_data;
>>> +    sz = cfg[cfg_index].size;
>>>         for (i = 0; i < sz; i++)
>>>           if (llcc_cfg[i].slice_id > drv_data->max_slices)
> 

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Dmitry Baryshkov]

On 28/06/2023 11:45, Komal Bajaj wrote:

No HTML emails on public mailing lists, please.

> 
> 
> On 6/23/2023 7:56 PM, Dmitry Baryshkov wrote:
>> On Fri, 23 Jun 2023 at 17:19, Komal Bajaj<quic_kbajaj@quicinc.com>  wrote:
>>> Add LLCC support for multi channel DDR configuration
>>> based on a feature register. Reading DDR channel
>>> confiuration uses nvmem framework, so select the
>>> dependency in Kconfig. Without this, there will be
>>> errors while building the driver with COMPILE_TEST only.
>>>
>>> Signed-off-by: Komal Bajaj<quic_kbajaj@quicinc.com>
>>> ---
>>>   drivers/soc/qcom/Kconfig     |  2 ++
>>>   drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>>>   2 files changed, 32 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>> index a491718f8064..cc9ad41c63aa 100644
>>> --- a/drivers/soc/qcom/Kconfig
>>> +++ b/drivers/soc/qcom/Kconfig
>>> @@ -64,6 +64,8 @@ config QCOM_LLCC
>>>          tristate "Qualcomm Technologies, Inc. LLCC driver"
>>>          depends on ARCH_QCOM || COMPILE_TEST
>>>          select REGMAP_MMIO
>>> +       select NVMEM
>> No need to select NVMEM. The used functions are stubbed if NVMEM is disabled
> 
> With the previous patch, where this config was not selected, below error 
> was flagged by kernel test robot -
> 
>     drivers/soc/qcom/llcc-qcom.c: In function 'qcom_llcc_get_cfg_index':
>      >> drivers/soc/qcom/llcc-qcom.c:951:15: error: implicit declaration
>     of function 'nvmem_cell_read_u8'; did you mean
>     'nvmem_cell_read_u64'? [-Werror=implicit-function-declaration]
>           951 |         ret = nvmem_cell_read_u8(&pdev->dev,
>     "multi_chan_ddr", cfg_index);
>               |               ^~~~~~~~~~~~~~~~~~
>               |               nvmem_cell_read_u64
>         cc1: some warnings being treated as errors

Judging from the rest of nvmem-consumer.h, it appears that not having 
stubs for this function is an omission. Please fix the header instead.

> 
>>> +       select QCOM_SCM
>>>          help
>>>            Qualcomm Technologies, Inc. platform specific
>>>            Last Level Cache Controller(LLCC) driver for platforms such as,
>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>> index 6cf373da5df9..3c29612da1c5 100644
>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>> @@ -12,6 +12,7 @@
>>>   #include <linux/kernel.h>
>>>   #include <linux/module.h>
>>>   #include <linux/mutex.h>
>>> +#include <linux/nvmem-consumer.h>
>>>   #include <linux/of.h>
>>>   #include <linux/of_device.h>
>>>   #include <linux/regmap.h>
>>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>>>          return ret;
>>>   }
>>>
>>> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
>>> +{
>>> +       int ret;
>>> +
>>> +       ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
>>> +       if (ret == -ENOENT) {
>> || ret == -EOPNOTSUPP ?
> 
> Okay
> 
>>> +               *cfg_index = 0;
>>> +               return 0;
>>> +       }
>>> +
>>> +       return ret;
>>> +}
>>> +
>>>   static int qcom_llcc_remove(struct platform_device *pdev)
>>>   {
>>>          /* Set the global pointer to a error code to avoid referencing it */
>>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>          struct device *dev = &pdev->dev;
>>>          int ret, i;
>>>          struct platform_device *llcc_edac;
>>> -       const struct qcom_llcc_config *cfg;
>>> +       const struct qcom_llcc_config *cfg, *entry;
>>>          const struct llcc_slice_config *llcc_cfg;
>>>          u32 sz;
>>> +       u8 cfg_index;
>>>          u32 version;
>>>          struct regmap *regmap;
>>> +       u32 num_entries = 0;
>>>
>>>          drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>>>          if (!drv_data) {
>>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>
>>>          drv_data->version = version;
>>>
>>> -       llcc_cfg = cfg[0]->sct_data;
>>> -       sz = cfg[0]->size;
>>> +       ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
>>> +       if (ret)
>>> +               goto err;
>>> +
>>> +       for (entry = cfg; entry->sct_data; entry++, num_entries++)
>>> +               ;
>> Please add num_cfgs to the configuration data instead.
> 
> Shall I create a new wrapper struct having a field num_cfg and a pointer 
> to those cfgs
> because configuration data is itself an instance of "struct 
> qcom_llcc_config" and
> we can have multiple instances of it.

A wrapper struct is a better approach in my opinion.

> 
> 
>>> +       if (cfg_index >= num_entries || cfg_index < 0) {
>> cfg_index is unsigned, so it can not be less than 0.
> 
> Okay.
> 
>>> +               ret = -EINVAL;
>>> +               goto err;
>>> +       }
>>> +
>>> +       llcc_cfg = cfg[cfg_index].sct_data;
>>> +       sz = cfg[cfg_index].size;
>>>
>>>          for (i = 0; i < sz; i++)
>>>                  if (llcc_cfg[i].slice_id > drv_data->max_slices)
>>> --
>>> 2.40.1
>>>
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Komal Bajaj]

On 6/28/2023 4:43 PM, Konrad Dybcio wrote:
> On 28.06.2023 10:52, Komal Bajaj wrote:
>>
>> On 6/23/2023 8:28 PM, Konrad Dybcio wrote:
>>> On 23.06.2023 16:18, Komal Bajaj wrote:
>>>> Add LLCC support for multi channel DDR configuration
>>>> based on a feature register. Reading DDR channel
>>>> confiuration uses nvmem framework, so select the
>>>> dependency in Kconfig. Without this, there will be
>>>> errors while building the driver with COMPILE_TEST only.
>>>>
>>>> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
>>>> ---
>>>>    drivers/soc/qcom/Kconfig     |  2 ++
>>>>    drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>>>>    2 files changed, 32 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index a491718f8064..cc9ad41c63aa 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -64,6 +64,8 @@ config QCOM_LLCC
>>>>        tristate "Qualcomm Technologies, Inc. LLCC driver"
>>>>        depends on ARCH_QCOM || COMPILE_TEST
>>>>        select REGMAP_MMIO
>>>> +    select NVMEM
>>>> +    select QCOM_SCM
>>>>        help
>>>>          Qualcomm Technologies, Inc. platform specific
>>>>          Last Level Cache Controller(LLCC) driver for platforms such as,
>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
>>>> index 6cf373da5df9..3c29612da1c5 100644
>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>> @@ -12,6 +12,7 @@
>>>>    #include <linux/kernel.h>
>>>>    #include <linux/module.h>
>>>>    #include <linux/mutex.h>
>>>> +#include <linux/nvmem-consumer.h>
>>>>    #include <linux/of.h>
>>>>    #include <linux/of_device.h>
>>>>    #include <linux/regmap.h>
>>>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>>>>        return ret;
>>>>    }
>>>>    +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
>>>> +{
>>>> +    int ret;
>>>> +
>>>> +    ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
>>>> +    if (ret == -ENOENT) {
>>>> +        *cfg_index = 0;
>>>> +        return 0;
>>>> +    }
>>>> +
>>>> +    return ret;
>>>> +}
>>>> +
>>>>    static int qcom_llcc_remove(struct platform_device *pdev)
>>>>    {
>>>>        /* Set the global pointer to a error code to avoid referencing it */
>>>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>>        struct device *dev = &pdev->dev;
>>>>        int ret, i;
>>>>        struct platform_device *llcc_edac;
>>>> -    const struct qcom_llcc_config *cfg;
>>>> +    const struct qcom_llcc_config *cfg, *entry;
>>>>        const struct llcc_slice_config *llcc_cfg;
>>>>        u32 sz;
>>>> +    u8 cfg_index;
>>>>        u32 version;
>>>>        struct regmap *regmap;
>>>> +    u32 num_entries = 0;
>>>>          drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>>>>        if (!drv_data) {
>>>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>>>>          drv_data->version = version;
>>>>    -    llcc_cfg = cfg[0]->sct_data;
>>>> -    sz = cfg[0]->size;
>>>> +    ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
>>>> +    if (ret)
>>>> +        goto err;
>>>> +
>>>> +    for (entry = cfg; entry->sct_data; entry++, num_entries++)
>>>> +        ;
>>>> +    if (cfg_index >= num_entries || cfg_index < 0) {
>>> cfg_index is an unsigned variable, it can never be < 0
>> Okay, will remove this condition.
>>
>>>> +        ret = -EINVAL;
>>>> +        goto err;
>>>> +    }
>>>> +
>>> if (cfg_index >= entry->size)? With that, you can also keep the config
>>> entries non-0-terminated in the previous patch, saving a whole lot of RAM.
>> entry->size represents the size of sct table whereas num_entries represents the number
>> of sct tables that we can have. And by this check we are validating the value read from the
>> fuse register. Am I understanding your comment correctly?
> Oh you're right.
>
> I still see room for improvement, though.
>
> For example, you duplicate assigning need_llcc_cfg, reg_offset
> and edac_reg_offset. You can add a new struct, like "sct_config" and add
> a pointer to sct_config[] & the length of this array to qcom_llcc_config.
>
> Konrad

Okay, will follow this approach.

>
>>> Konrad
>>>> +    llcc_cfg = cfg[cfg_index].sct_data;
>>>> +    sz = cfg[cfg_index].size;
>>>>          for (i = 0; i < sz; i++)
>>>>            if (llcc_cfg[i].slice_id > drv_data->max_slices)

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Komal Bajaj]

On 6/28/2023 6:44 PM, Dmitry Baryshkov wrote:
> On 28/06/2023 11:45, Komal Bajaj wrote:
>
> No HTML emails on public mailing lists, please.
>
>>
>>
>> On 6/23/2023 7:56 PM, Dmitry Baryshkov wrote:
>>> On Fri, 23 Jun 2023 at 17:19, Komal Bajaj<quic_kbajaj@quicinc.com>  
>>> wrote:
>>>> Add LLCC support for multi channel DDR configuration
>>>> based on a feature register. Reading DDR channel
>>>> confiuration uses nvmem framework, so select the
>>>> dependency in Kconfig. Without this, there will be
>>>> errors while building the driver with COMPILE_TEST only.
>>>>
>>>> Signed-off-by: Komal Bajaj<quic_kbajaj@quicinc.com>
>>>> ---
>>>>   drivers/soc/qcom/Kconfig     |  2 ++
>>>>   drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>>>>   2 files changed, 32 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
>>>> index a491718f8064..cc9ad41c63aa 100644
>>>> --- a/drivers/soc/qcom/Kconfig
>>>> +++ b/drivers/soc/qcom/Kconfig
>>>> @@ -64,6 +64,8 @@ config QCOM_LLCC
>>>>          tristate "Qualcomm Technologies, Inc. LLCC driver"
>>>>          depends on ARCH_QCOM || COMPILE_TEST
>>>>          select REGMAP_MMIO
>>>> +       select NVMEM
>>> No need to select NVMEM. The used functions are stubbed if NVMEM is 
>>> disabled
>>
>> With the previous patch, where this config was not selected, below 
>> error was flagged by kernel test robot -
>>
>>     drivers/soc/qcom/llcc-qcom.c: In function 'qcom_llcc_get_cfg_index':
>>      >> drivers/soc/qcom/llcc-qcom.c:951:15: error: implicit declaration
>>     of function 'nvmem_cell_read_u8'; did you mean
>>     'nvmem_cell_read_u64'? [-Werror=implicit-function-declaration]
>>           951 |         ret = nvmem_cell_read_u8(&pdev->dev,
>>     "multi_chan_ddr", cfg_index);
>>               |               ^~~~~~~~~~~~~~~~~~
>>               |               nvmem_cell_read_u64
>>         cc1: some warnings being treated as errors
>
> Judging from the rest of nvmem-consumer.h, it appears that not having 
> stubs for this function is an omission. Please fix the header instead.

Okay, I will add the stub for this function in the header.

>
>>
>>>> +       select QCOM_SCM
>>>>          help
>>>>            Qualcomm Technologies, Inc. platform specific
>>>>            Last Level Cache Controller(LLCC) driver for platforms 
>>>> such as,
>>>> diff --git a/drivers/soc/qcom/llcc-qcom.c 
>>>> b/drivers/soc/qcom/llcc-qcom.c
>>>> index 6cf373da5df9..3c29612da1c5 100644
>>>> --- a/drivers/soc/qcom/llcc-qcom.c
>>>> +++ b/drivers/soc/qcom/llcc-qcom.c
>>>> @@ -12,6 +12,7 @@
>>>>   #include <linux/kernel.h>
>>>>   #include <linux/module.h>
>>>>   #include <linux/mutex.h>
>>>> +#include <linux/nvmem-consumer.h>
>>>>   #include <linux/of.h>
>>>>   #include <linux/of_device.h>
>>>>   #include <linux/regmap.h>
>>>> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct 
>>>> platform_device *pdev,
>>>>          return ret;
>>>>   }
>>>>
>>>> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, 
>>>> u8 *cfg_index)
>>>> +{
>>>> +       int ret;
>>>> +
>>>> +       ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", 
>>>> cfg_index);
>>>> +       if (ret == -ENOENT) {
>>> || ret == -EOPNOTSUPP ?
>>
>> Okay
>>
>>>> +               *cfg_index = 0;
>>>> +               return 0;
>>>> +       }
>>>> +
>>>> +       return ret;
>>>> +}
>>>> +
>>>>   static int qcom_llcc_remove(struct platform_device *pdev)
>>>>   {
>>>>          /* Set the global pointer to a error code to avoid 
>>>> referencing it */
>>>> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct 
>>>> platform_device *pdev)
>>>>          struct device *dev = &pdev->dev;
>>>>          int ret, i;
>>>>          struct platform_device *llcc_edac;
>>>> -       const struct qcom_llcc_config *cfg;
>>>> +       const struct qcom_llcc_config *cfg, *entry;
>>>>          const struct llcc_slice_config *llcc_cfg;
>>>>          u32 sz;
>>>> +       u8 cfg_index;
>>>>          u32 version;
>>>>          struct regmap *regmap;
>>>> +       u32 num_entries = 0;
>>>>
>>>>          drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>>>>          if (!drv_data) {
>>>> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct 
>>>> platform_device *pdev)
>>>>
>>>>          drv_data->version = version;
>>>>
>>>> -       llcc_cfg = cfg[0]->sct_data;
>>>> -       sz = cfg[0]->size;
>>>> +       ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
>>>> +       if (ret)
>>>> +               goto err;
>>>> +
>>>> +       for (entry = cfg; entry->sct_data; entry++, num_entries++)
>>>> +               ;
>>> Please add num_cfgs to the configuration data instead.
>>
>> Shall I create a new wrapper struct having a field num_cfg and a 
>> pointer to those cfgs
>> because configuration data is itself an instance of "struct 
>> qcom_llcc_config" and
>> we can have multiple instances of it.
>
> A wrapper struct is a better approach in my opinion.

Okay, will follow this approach then.

Thanks
Komal
>
>>
>>
>>>> +       if (cfg_index >= num_entries || cfg_index < 0) {
>>> cfg_index is unsigned, so it can not be less than 0.
>>
>> Okay.
>>
>>>> +               ret = -EINVAL;
>>>> +               goto err;
>>>> +       }
>>>> +
>>>> +       llcc_cfg = cfg[cfg_index].sct_data;
>>>> +       sz = cfg[cfg_index].size;
>>>>
>>>>          for (i = 0; i < sz; i++)
>>>>                  if (llcc_cfg[i].slice_id > drv_data->max_slices)
>>>> -- 
>>>> 2.40.1
>>>>
>>
>

Re: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.

[Bjorn Andersson]

On Fri, Jun 23, 2023 at 04:53:44PM +0200, Konrad Dybcio wrote:
> On 23.06.2023 16:18, Komal Bajaj wrote:
> > 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 *priv = context;
> > +	u8 *val = _val;
> > +	u8 *tmp;
> > +	u32 read_val;
> > +	unsigned int i;
> Please sort this to look like a reverse-Christmas-tree
> 
> 
> > +
> > +	for (i = 0; i < bytes; i++, reg++) {
> > +		if (i == 0 || reg % 4 == 0) {
> > +			if (qcom_scm_io_readl(priv->qfpseccorrected + (reg & ~3), &read_val)) {
> > +				dev_err(priv->dev, "Couldn't access fuse register\n");
> > +				return -EINVAL;
> > +			}
> > +			tmp = (u8 *)&read_val;
> > +		}
> I don't understand this read-4-times dance.. qcom_scm_io_readl() reads
> u32, so this should be as simple as:
> 
> val[i + 0] = FIELD_GET(GENMASK(7, 0), reg);
> val[i + 1] = ..
> val[i + 2] = ..
> val[i + 3] = ..
> 

No need for FIELD_GET() to do that, you can just do *(u32*)val =
read_val; although this comes with alignment issues.


But, that this the length of val is always divisible by 4, and I
wasn't able to convince myself that it was the case...

So this is, pretty much, what I proposed in v3:
https://lore.kernel.org/linux-arm-msm/20230527205031.iwsujvlbxazukwfy@ripper/

Regards,
Bjorn

> 
> > +
> > +		val[i] = tmp[reg & 3];
> > +	}
> > +
> > +	return 0;

Re: [PATCH v4 3/6] nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.

[Bjorn Andersson]

On Fri, Jun 23, 2023 at 07:48:03PM +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.

Linux

> With that motivation, add secure qfprom driver. Ensuring
> the address to read is word aligned since our secure I/O
> only supports word size I/O.

What do you mean with this last sentence? I don't see anything in your
patch demanding this. Perhaps just drop this sentence?

> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/nvmem/Kconfig      |  12 ++++
>  drivers/nvmem/Makefile     |   2 +
>  drivers/nvmem/sec-qfprom.c | 116 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 130 insertions(+)
>  create mode 100644 drivers/nvmem/sec-qfprom.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index b291b27048c7..2b0dd73d899e 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -216,6 +216,18 @@ 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
> +        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..4b4bf5880488 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..47b2c71593dd
> --- /dev/null
> +++ b/drivers/nvmem/sec-qfprom.c
> @@ -0,0 +1,116 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +
> +#include <linux/clk.h>

Please review your include list, this and a few others doesn't seem to
be used.

> +#include <linux/device.h>
> +#include <linux/firmware/qcom/qcom_scm.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/property.h>
> +#include <linux/regulator/consumer.h>
> +
> +
> +/**
> + * struct sec_sec_qfprom_priv - structure holding secure qfprom attributes
> + *
> + * @qfpseccorrected: iomapped memory space for secure qfprom corrected address space.
> + * @dev: qfprom device structure.
> + */
> +struct sec_qfprom_priv {

There no risk of confusion here, so please drop the "_priv" suffix.

> +	phys_addr_t qfpseccorrected;

Same thing here, so "base" or "addr" would suffice.

> +	struct device *dev;
> +};
> +
> +static int sec_qfprom_reg_read(void *context, unsigned int reg, void *_val, size_t bytes)
> +{
> +	struct sec_qfprom_priv *priv = context;
> +	u8 *val = _val;
> +	u8 *tmp;
> +	u32 read_val;
> +	unsigned int i;
> +
> +	for (i = 0; i < bytes; i++, reg++) {
> +		if (i == 0 || reg % 4 == 0) {
> +			if (qcom_scm_io_readl(priv->qfpseccorrected + (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 void sec_qfprom_runtime_disable(void *data)
> +{
> +	pm_runtime_disable(data);
> +}
> +
> +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 resource *res;
> +	struct nvmem_device *nvmem;
> +	struct sec_qfprom_priv *priv;
> +	int ret;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	priv->qfpseccorrected = res->start;
> +	if (!priv->qfpseccorrected)
> +		return -ENOMEM;
> +
> +	econfig.size = resource_size(res);
> +	econfig.dev = dev;
> +	econfig.priv = priv;
> +
> +	priv->dev = dev;
> +
> +	pm_runtime_enable(dev);
> +	ret = devm_add_action_or_reset(dev, sec_qfprom_runtime_disable, dev);
> +	if (ret)
> +		return ret;

Please use devm_pm_runtime_enable().

> +
> +	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",

No comma here please, qcom_sec_qfprom.

> +		.of_match_table = sec_qfprom_of_match,
> +	},
> +};
> +module_platform_driver(qfprom_driver);
> +MODULE_DESCRIPTION("Qualcomm Secure QFPROM driver");
> +MODULE_LICENSE("GPL v2");

Please change this to "GPL".

Regards,
Bjorn

Re: [PATCH v4 5/6] soc: qcom: Add LLCC support for multi channel DDR

[Bjorn Andersson]

On Fri, Jun 23, 2023 at 07:48:05PM +0530, Komal Bajaj wrote:
> Add LLCC support for multi channel DDR configuration
> based on a feature register. Reading DDR channel
> confiuration uses nvmem framework, so select the
> dependency in Kconfig. Without this, there will be
> errors while building the driver with COMPILE_TEST only.

You may drop the last sentence, I don't think it's entirely correct.

> 
> Signed-off-by: Komal Bajaj <quic_kbajaj@quicinc.com>
> ---
>  drivers/soc/qcom/Kconfig     |  2 ++
>  drivers/soc/qcom/llcc-qcom.c | 33 ++++++++++++++++++++++++++++++---
>  2 files changed, 32 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/Kconfig b/drivers/soc/qcom/Kconfig
> index a491718f8064..cc9ad41c63aa 100644
> --- a/drivers/soc/qcom/Kconfig
> +++ b/drivers/soc/qcom/Kconfig
> @@ -64,6 +64,8 @@ config QCOM_LLCC
>  	tristate "Qualcomm Technologies, Inc. LLCC driver"
>  	depends on ARCH_QCOM || COMPILE_TEST
>  	select REGMAP_MMIO
> +	select NVMEM
> +	select QCOM_SCM

I don't see anything your patch that warrants adding QCOM_SCM here,
is it needed, should it be a separate commit?

>  	help
>  	  Qualcomm Technologies, Inc. platform specific
>  	  Last Level Cache Controller(LLCC) driver for platforms such as,
> diff --git a/drivers/soc/qcom/llcc-qcom.c b/drivers/soc/qcom/llcc-qcom.c
> index 6cf373da5df9..3c29612da1c5 100644
> --- a/drivers/soc/qcom/llcc-qcom.c
> +++ b/drivers/soc/qcom/llcc-qcom.c
> @@ -12,6 +12,7 @@
>  #include <linux/kernel.h>
>  #include <linux/module.h>
>  #include <linux/mutex.h>
> +#include <linux/nvmem-consumer.h>
>  #include <linux/of.h>
>  #include <linux/of_device.h>
>  #include <linux/regmap.h>
> @@ -943,6 +944,19 @@ static int qcom_llcc_cfg_program(struct platform_device *pdev,
>  	return ret;
>  }
>  
> +static int qcom_llcc_get_cfg_index(struct platform_device *pdev, u8 *cfg_index)
> +{
> +	int ret;
> +
> +	ret = nvmem_cell_read_u8(&pdev->dev, "multi-chan-ddr", cfg_index);
> +	if (ret == -ENOENT) {
> +		*cfg_index = 0;
> +		return 0;
> +	}
> +
> +	return ret;
> +}
> +
>  static int qcom_llcc_remove(struct platform_device *pdev)
>  {
>  	/* Set the global pointer to a error code to avoid referencing it */
> @@ -975,11 +989,13 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  	struct device *dev = &pdev->dev;
>  	int ret, i;
>  	struct platform_device *llcc_edac;
> -	const struct qcom_llcc_config *cfg;
> +	const struct qcom_llcc_config *cfg, *entry;
>  	const struct llcc_slice_config *llcc_cfg;
>  	u32 sz;
> +	u8 cfg_index;
>  	u32 version;
>  	struct regmap *regmap;
> +	u32 num_entries = 0;
>  
>  	drv_data = devm_kzalloc(dev, sizeof(*drv_data), GFP_KERNEL);
>  	if (!drv_data) {
> @@ -1040,8 +1056,19 @@ static int qcom_llcc_probe(struct platform_device *pdev)
>  
>  	drv_data->version = version;
>  
> -	llcc_cfg = cfg[0]->sct_data;
> -	sz = cfg[0]->size;
> +	ret = qcom_llcc_get_cfg_index(pdev, &cfg_index);
> +	if (ret)
> +		goto err;
> +
> +	for (entry = cfg; entry->sct_data; entry++, num_entries++)
> +		;

This is still unnecessarily "clever":
"For each valid entry, do nothing, while incrementing num_entries".

How about just writing:
"For each valid entry, increment num_entries"

	for (entry = cfg; entry->sct_data; entry++)
		num_entries++;


> +	if (cfg_index >= num_entries || cfg_index < 0) {

cfg_index is an unsiged number, so it's unlikely to be negative.

Also, "cfg_index" and "num_entries" are values in the same domain, so
keeping their names related is beneficial - i.e. rename num_entries to
num_cfgs.

Regards,
Bjorn

> +		ret = -EINVAL;
> +		goto err;
> +	}
> +
> +	llcc_cfg = cfg[cfg_index].sct_data;
> +	sz = cfg[cfg_index].size;
>  
>  	for (i = 0; i < sz; i++)
>  		if (llcc_cfg[i].slice_id > drv_data->max_slices)
> -- 
> 2.40.1
> 

Re: [PATCH v4 0/6] soc: qcom: llcc: Add support for QDU1000/QRU1000

[Bjorn Andersson]

On Fri, Jun 23, 2023 at 07:48:00PM +0530, Komal Bajaj wrote:
> From: Komal-Bajaj <quic_kbajaj@quicinc.com>
> 

The patches in this series are going to be merged by two different
maintainers, the interface between them is an existing, clean API, so it
will be possible to merge the two halfs independently.

So please split this into one series for the addition of the nvmem
driver and one for the llcc pieces (with the nvmem interface/stub update
in the llcc one).

Thanks,
Bjorn

> This patch series does the following -
>  * Add secure qfprom driver for reading secure fuse region in qfprom driver
>  * Add dt-bindings for secure qfprom
>  * Refactor LLCC driver to support multiple configuration
>  * Add support for multi channel DDR configuration in LLCC
>  * Add LLCC support for the Qualcomm QDU1000 and QRU1000 SoCs
> 
> Changes in v4 -
>  - Created a separate driver for reading from secure fuse region as suggested.
>  - Added patch for dt-bindings of secure qfprom driver accordingly.
>  - Added new properties in the dt-bindings for LLCC. 
>  - Implemented new logic to read the nvmem cell as suggested by Bjorn.
>  - Separating the DT patches from this series as per suggestion.
> 
> Changes in v3-
>  - Addressed comments from Krzysztof and Mani.
>  - Using qfprom to read DDR configuration from feature register.
> 
> Changes in v2:
>   - Addressing comments from Konrad.
> 
> Komal Bajaj (6):
>   dt-bindings: nvmem: sec-qfprom: Add bindings for secure qfprom
>   dt-bindings: cache: qcom,llcc: Add LLCC compatible for QDU1000/QRU1000
>   nvmem: sec-qfprom: Add Qualcomm secure QFPROM support.
>   soc: qcom: llcc: Refactor llcc driver to support multiple
>     configuration
>   soc: qcom: Add LLCC support for multi channel DDR
>   soc: qcom: llcc: Add QDU1000 and QRU1000 LLCC support
> 
>  .../devicetree/bindings/cache/qcom,llcc.yaml  |  10 +
>  .../bindings/nvmem/qcom,sec-qfprom.yaml       |  58 ++++
>  drivers/nvmem/Kconfig                         |  12 +
>  drivers/nvmem/Makefile                        |   2 +
>  drivers/nvmem/sec-qfprom.c                    | 116 +++++++
>  drivers/soc/qcom/Kconfig                      |   2 +
>  drivers/soc/qcom/llcc-qcom.c                  | 304 +++++++++++++-----
>  include/linux/soc/qcom/llcc-qcom.h            |   2 +-
>  8 files changed, 416 insertions(+), 90 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/nvmem/qcom,sec-qfprom.yaml
>  create mode 100644 drivers/nvmem/sec-qfprom.c
> 
> -- 
> 2.40.1
>