[PATCH 0/4] Add new driver for WCSS secure PIL loading

[PATCH 0/4] Add new driver for WCSS secure PIL loading

[Gokul Sriram Palanisamy]

This series depends on q6 clock removal series [1].

- Secure PIL is signed, split firmware images which only TrustZone (TZ) can
  authenticate and load. Linux kernel will send a request to TZ to
  authenticate and load the PIL images.

- When secure PIL support was added to the existing wcss PIL driver
  earlier in [2], Bjorn suggested not to overload the existing WCSS
  rproc driver [2], instead post a new driver for secure PIL alone.
  This series adds a new secure PIL driver for the same. 

[1] https://patchwork.kernel.org/project/linux-arm-msm/cover/20240820055618.267554-1-quic_gokulsri@quicinc.com/

[2] https://patchwork.kernel.org/project/linux-arm-msm/patch/1611984013-10201-3-git-send-email-gokulsri@codeaurora.org/

Manikanta Mylavarapu (3):
  dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL
  arm64: dts: qcom: ipq5332: add nodes to bringup q6
  arm64: dts: qcom: ipq9574: add nodes to bring up q6

Vignesh Viswanathan (1):
  remoteproc: qcom: add hexagon based WCSS secure PIL driver

 .../remoteproc/qcom,wcss-sec-pil.yaml         | 125 ++++++
 arch/arm64/boot/dts/qcom/ipq5332.dtsi         |  62 +++
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         |  58 +++
 drivers/remoteproc/Kconfig                    |  22 ++
 drivers/remoteproc/Makefile                   |   1 +
 drivers/remoteproc/qcom_q6v5_wcss_sec.c       | 360 ++++++++++++++++++
 6 files changed, 628 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
 create mode 100644 drivers/remoteproc/qcom_q6v5_wcss_sec.c

-- 
2.34.1

[PATCH 1/2] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

[Gokul Sriram Palanisamy]

From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>

Add new binding document for hexagon based WCSS secure PIL remoteproc.
IPQ5332, IPQ9574 follows secure PIL remoteproc.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 .../remoteproc/qcom,wcss-sec-pil.yaml         | 125 ++++++++++++++++++
 1 file changed, 125 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
new file mode 100644
index 000000000000..c69401b6cec1
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
@@ -0,0 +1,125 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,wcss-sec-pil.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm WCSS Secure Peripheral Image Loader
+
+maintainers:
+  - Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
+
+description:
+  WCSS Secure Peripheral Image Loader loads firmware and power up QDSP6
+  remoteproc's on the Qualcomm IPQ9574, IPQ5332 SoC.
+
+properties:
+  compatible:
+    enum:
+      - qcom,ipq5332-wcss-sec-pil
+      - qcom,ipq9574-wcss-sec-pil
+
+  reg:
+    maxItems: 1
+
+  firmware-name:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: Firmware name for the Hexagon core
+
+  interrupts:
+    items:
+      - description: Watchdog interrupt
+      - description: Fatal interrupt
+      - description: Ready interrupt
+      - description: Handover interrupt
+      - description: Stop acknowledge interrupt
+
+  interrupt-names:
+    items:
+      - const: wdog
+      - const: fatal
+      - const: ready
+      - const: handover
+      - const: stop-ack
+
+  clocks:
+    items:
+      - description: IM SLEEP clock
+
+  clock-names:
+    items:
+      - const: im_sleep
+
+  qcom,smem-states:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: States used by the AP to signal the remote processor
+    items:
+      - description: Shutdown Q6
+      - description: Stop Q6
+
+  qcom,smem-state-names:
+    description:
+      Names of the states used by the AP to signal the remote processor
+    items:
+      - const: shutdown
+      - const: stop
+
+  memory-region:
+    items:
+      - description: Q6 reserved region
+
+  glink-edge:
+    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
+    description:
+      Qualcomm G-Link subnode which represents communication edge, channels
+      and devices related to the Modem.
+    unevaluatedProperties: false
+
+required:
+  - compatible
+  - firmware-name
+  - reg
+  - interrupts
+  - interrupt-names
+  - qcom,smem-states
+  - qcom,smem-state-names
+  - memory-region
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
+    q6v5_wcss: remoteproc@d100000 {
+      compatible = "qcom,ipq5332-wcss-sec-pil";
+      reg = <0xd100000 0x4040>;
+      firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
+      interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
+                            <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
+                            <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,
+                            <&wcss_smp2p_in 2 IRQ_TYPE_NONE>,
+                            <&wcss_smp2p_in 3 IRQ_TYPE_NONE>;
+      interrupt-names = "wdog",
+                        "fatal",
+                        "ready",
+                        "handover",
+                        "stop-ack";
+
+      clocks = <&gcc GCC_IM_SLEEP_CLK>;
+      clock-names = "im_sleep";
+
+      qcom,smem-states = <&wcss_smp2p_out 0>,
+                         <&wcss_smp2p_out 1>;
+      qcom,smem-state-names = "shutdown",
+                              "stop";
+
+      memory-region = <&q6_region>;
+
+      glink-edge {
+        interrupts = <GIC_SPI 417 IRQ_TYPE_EDGE_RISING>;
+        label = "rtr";
+        qcom,remote-pid = <1>;
+        mboxes = <&apcs_glb 8>;
+      };
+    };
-- 
2.34.1

[PATCH 2/2] remoteproc: qcom: add hexagon based WCSS secure PIL driver

[Gokul Sriram Palanisamy]

From: Vignesh Viswanathan <quic_viswanat@quicinc.com>

Add support to bring up hexagon based WCSS secure PIL remoteproc.
IPQ5332, IPQ9574 supports secure PIL remoteproc.

Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 drivers/remoteproc/Kconfig              |  22 ++
 drivers/remoteproc/Makefile             |   1 +
 drivers/remoteproc/qcom_q6v5_wcss_sec.c | 363 ++++++++++++++++++++++++
 3 files changed, 386 insertions(+)
 create mode 100644 drivers/remoteproc/qcom_q6v5_wcss_sec.c

diff --git a/drivers/remoteproc/Kconfig b/drivers/remoteproc/Kconfig
index dda2ada215b7..368405825d83 100644
--- a/drivers/remoteproc/Kconfig
+++ b/drivers/remoteproc/Kconfig
@@ -255,6 +255,28 @@ config QCOM_Q6V5_WCSS
 	  Hexagon V5 based WCSS remote processors on e.g. IPQ8074.  This is
 	  a non-TrustZone wireless subsystem.
 
+config QCOM_Q6V5_WCSS_SEC
+	tristate "Qualcomm Hexagon based WCSS Secure Peripheral Image Loader"
+	depends on OF && ARCH_QCOM
+	depends on QCOM_SMEM
+	depends on RPMSG_QCOM_SMD || RPMSG_QCOM_SMD=n
+	depends on RPMSG_QCOM_GLINK_SMEM || RPMSG_QCOM_GLINK_SMEM=n
+	depends on QCOM_SYSMON || QCOM_SYSMON=n
+	depends on RPMSG_QCOM_GLINK || RPMSG_QCOM_GLINK=n
+	depends on QCOM_AOSS_QMP || QCOM_AOSS_QMP=n
+	select QCOM_MDT_LOADER
+	select QCOM_PIL_INFO
+	select QCOM_Q6V5_COMMON
+	select QCOM_RPROC_COMMON
+	select QCOM_SCM
+	help
+	  Say y here to support the Qualcomm Secure Peripheral Image Loader
+	  for the Hexagon based remote processors on e.g. IPQ5332.
+
+	  This is TrustZone wireless subsystem. The firmware is
+	  verified and booted with the help of the Peripheral Authentication
+	  System (PAS) in TrustZone.
+
 config QCOM_SYSMON
 	tristate "Qualcomm sysmon driver"
 	depends on RPMSG
diff --git a/drivers/remoteproc/Makefile b/drivers/remoteproc/Makefile
index 91314a9b43ce..54530d7fa510 100644
--- a/drivers/remoteproc/Makefile
+++ b/drivers/remoteproc/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_QCOM_Q6V5_ADSP)		+= qcom_q6v5_adsp.o
 obj-$(CONFIG_QCOM_Q6V5_MSS)		+= qcom_q6v5_mss.o
 obj-$(CONFIG_QCOM_Q6V5_PAS)		+= qcom_q6v5_pas.o
 obj-$(CONFIG_QCOM_Q6V5_WCSS)		+= qcom_q6v5_wcss.o
+obj-$(CONFIG_QCOM_Q6V5_WCSS_SEC)	+= qcom_q6v5_wcss_sec.o
 obj-$(CONFIG_QCOM_SYSMON)		+= qcom_sysmon.o
 obj-$(CONFIG_QCOM_WCNSS_PIL)		+= qcom_wcnss_pil.o
 qcom_wcnss_pil-y			+= qcom_wcnss.o
diff --git a/drivers/remoteproc/qcom_q6v5_wcss_sec.c b/drivers/remoteproc/qcom_q6v5_wcss_sec.c
new file mode 100644
index 000000000000..8c47cf35a00b
--- /dev/null
+++ b/drivers/remoteproc/qcom_q6v5_wcss_sec.c
@@ -0,0 +1,363 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2016-2018 Linaro Ltd.
+ * Copyright (C) 2014 Sony Mobile Communications AB
+ * Copyright (c) 2012-2018, 2024 The Linux Foundation. All rights reserved.
+ */
+#include <linux/clk.h>
+#include <linux/delay.h>
+#include <linux/firmware/qcom/qcom_scm.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/of_reserved_mem.h>
+#include <linux/platform_device.h>
+#include <linux/reset.h>
+#include <linux/soc/qcom/mdt_loader.h>
+#include <linux/soc/qcom/smem.h>
+#include <linux/soc/qcom/smem_state.h>
+#include "qcom_common.h"
+#include "qcom_q6v5.h"
+
+#include "remoteproc_internal.h"
+
+#define WCSS_CRASH_REASON		421
+
+#define WCSS_PAS_ID			0x6
+#define MPD_WCSS_PAS_ID			0xD
+
+struct wcss_sec {
+	struct device *dev;
+	struct qcom_rproc_glink glink_subdev;
+	struct qcom_rproc_ssr ssr_subdev;
+	struct qcom_q6v5 q6;
+	phys_addr_t mem_phys;
+	phys_addr_t mem_reloc;
+	void *mem_region;
+	size_t mem_size;
+	const struct wcss_data *desc;
+	const char *fw_name;
+
+	struct clk *im_sleep;
+};
+
+struct wcss_data {
+	u32 pasid;
+	const struct rproc_ops *ops;
+	bool need_auto_boot;
+	int (*init_clk)(struct wcss_sec *wcss);
+};
+
+static int wcss_sec_start(struct rproc *rproc)
+{
+	struct wcss_sec *wcss = rproc->priv;
+	struct device *dev = wcss->dev;
+	const struct wcss_data *desc = of_device_get_match_data(dev);
+	int ret;
+
+	qcom_q6v5_prepare(&wcss->q6);
+
+	ret = qcom_scm_pas_auth_and_reset(desc->pasid);
+	if (ret) {
+		dev_err(dev, "wcss_reset failed\n");
+		return ret;
+	}
+
+	ret = qcom_q6v5_wait_for_start(&wcss->q6, 5 * HZ);
+	if (ret == -ETIMEDOUT)
+		dev_err(dev, "start timed out\n");
+
+	return ret;
+}
+
+static int wcss_sec_stop(struct rproc *rproc)
+{
+	struct wcss_sec *wcss = rproc->priv;
+	struct device *dev = wcss->dev;
+	const struct wcss_data *desc = of_device_get_match_data(dev);
+	int ret;
+
+	ret = qcom_scm_pas_shutdown(desc->pasid);
+	if (ret) {
+		dev_err(dev, "not able to shutdown\n");
+		return ret;
+	}
+	qcom_q6v5_unprepare(&wcss->q6);
+
+	return 0;
+}
+
+static void *wcss_sec_da_to_va(struct rproc *rproc, u64 da, size_t len,
+			       bool *is_iomem)
+{
+	struct wcss_sec *wcss = rproc->priv;
+	int offset;
+
+	offset = da - wcss->mem_reloc;
+	if (offset < 0 || offset + len > wcss->mem_size)
+		return NULL;
+
+	return wcss->mem_region + offset;
+}
+
+static int wcss_sec_load(struct rproc *rproc, const struct firmware *fw)
+{
+	struct wcss_sec *wcss = rproc->priv;
+	struct device *dev = wcss->dev;
+	const struct wcss_data *desc = of_device_get_match_data(dev);
+
+	return qcom_mdt_load(dev, fw, rproc->firmware, desc->pasid,
+			     wcss->mem_region, wcss->mem_phys, wcss->mem_size,
+			     &wcss->mem_reloc);
+}
+
+static unsigned long wcss_sec_panic(struct rproc *rproc)
+{
+	struct wcss_sec *wcss = rproc->priv;
+
+	return qcom_q6v5_panic(&wcss->q6);
+}
+
+static void wcss_sec_copy_segment(struct rproc *rproc,
+				  struct rproc_dump_segment *segment,
+				  void *dest, size_t offset, size_t size)
+{
+	struct wcss_sec *wcss = rproc->priv;
+	struct device *dev = wcss->dev;
+	void *ptr;
+
+	ptr = devm_ioremap_wc(dev, segment->da, segment->size);
+	if (!ptr) {
+		dev_err(dev, "Failed to ioremap segment %pad size %zx\n",
+			&segment->da, segment->size);
+		return;
+	}
+
+	if (size <= segment->size - offset)
+		memcpy(dest, ptr + offset, size);
+	else
+		dev_err(dev, "Copy size greater than segment size. Skipping\n");
+	devm_iounmap(dev, ptr);
+}
+
+static int wcss_sec_dump_segments(struct rproc *rproc,
+				  const struct firmware *fw)
+{
+	struct device *dev = rproc->dev.parent;
+	struct reserved_mem *rmem = NULL;
+	struct device_node *node;
+	int num_segs, index = 0;
+	int ret;
+
+	/* Parse through additional reserved memory regions for the rproc
+	 * and add them to the coredump segments
+	 */
+	num_segs = of_count_phandle_with_args(dev->of_node,
+					      "memory-region", NULL);
+	while (index < num_segs) {
+		node = of_parse_phandle(dev->of_node,
+					"memory-region", index);
+		if (!node)
+			return -EINVAL;
+
+		rmem = of_reserved_mem_lookup(node);
+		if (!rmem) {
+			dev_err(dev, "unable to acquire memory-region index %d num_segs %d\n",
+				index, num_segs);
+			return -EINVAL;
+		}
+
+		of_node_put(node);
+
+		dev_dbg(dev, "Adding segment 0x%pa size 0x%pa",
+			&rmem->base, &rmem->size);
+		ret = rproc_coredump_add_custom_segment(rproc,
+							rmem->base,
+							rmem->size,
+							wcss_sec_copy_segment,
+							NULL);
+		if (ret)
+			return ret;
+
+		index++;
+	}
+
+	return 0;
+}
+
+static const struct rproc_ops wcss_sec_ops = {
+	.start = wcss_sec_start,
+	.stop = wcss_sec_stop,
+	.da_to_va = wcss_sec_da_to_va,
+	.load = wcss_sec_load,
+	.get_boot_addr = rproc_elf_get_boot_addr,
+	.panic = wcss_sec_panic,
+	.parse_fw = wcss_sec_dump_segments,
+};
+
+static int wcss_sec_alloc_memory_region(struct wcss_sec *wcss)
+{
+	struct reserved_mem *rmem = NULL;
+	struct device_node *node;
+	struct device *dev = wcss->dev;
+
+	node = of_parse_phandle(dev->of_node, "memory-region", 0);
+	if (node) {
+		rmem = of_reserved_mem_lookup(node);
+	} else {
+		dev_err(dev, "can't find phandle memory-region\n");
+		return -EINVAL;
+	}
+
+	of_node_put(node);
+
+	if (!rmem) {
+		dev_err(dev, "unable to acquire memory-region\n");
+		return -EINVAL;
+	}
+
+	wcss->mem_phys = rmem->base;
+	wcss->mem_reloc = rmem->base;
+	wcss->mem_size = rmem->size;
+	wcss->mem_region = devm_ioremap_wc(dev, wcss->mem_phys, wcss->mem_size);
+	if (!wcss->mem_region) {
+		dev_err(dev, "unable to map memory region: %pa+%pa\n",
+			&rmem->base, &rmem->size);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+static int wcss_sec_probe(struct platform_device *pdev)
+{
+	struct wcss_sec *wcss;
+	struct rproc *rproc;
+	const char *fw_name = NULL;
+	const struct wcss_data *desc = of_device_get_match_data(&pdev->dev);
+	int ret;
+
+	if (!desc)
+		return -EINVAL;
+
+	ret = of_property_read_string(pdev->dev.of_node, "firmware-name",
+				      &fw_name);
+	if (ret < 0)
+		return ret;
+
+	rproc = rproc_alloc(&pdev->dev, pdev->name, desc->ops, fw_name,
+			    sizeof(*wcss));
+	if (!rproc) {
+		dev_err(&pdev->dev, "failed to allocate rproc\n");
+		return -ENOMEM;
+	}
+
+	wcss = rproc->priv;
+	wcss->dev = &pdev->dev;
+	wcss->desc = desc;
+	wcss->fw_name = fw_name;
+
+	ret = wcss_sec_alloc_memory_region(wcss);
+	if (ret)
+		goto free_rproc;
+
+	if (desc->init_clk) {
+		ret = desc->init_clk(wcss);
+		if (ret)
+			goto free_rproc;
+	}
+
+	ret = qcom_q6v5_init(&wcss->q6, pdev, rproc,
+			     WCSS_CRASH_REASON, NULL, NULL);
+	if (ret)
+		goto free_rproc;
+
+	qcom_add_glink_subdev(rproc, &wcss->glink_subdev, "q6wcss");
+	qcom_add_ssr_subdev(rproc, &wcss->ssr_subdev, pdev->name);
+
+	rproc->auto_boot = desc->need_auto_boot;
+	rproc->dump_conf = RPROC_COREDUMP_INLINE;
+	rproc_coredump_set_elf_info(rproc, ELFCLASS32, EM_NONE);
+
+	ret = rproc_add(rproc);
+	if (ret)
+		goto free_rproc;
+
+	platform_set_drvdata(pdev, rproc);
+
+	return 0;
+
+free_rproc:
+	rproc_free(rproc);
+
+	return ret;
+}
+
+static void wcss_sec_remove(struct platform_device *pdev)
+{
+	struct rproc *rproc = platform_get_drvdata(pdev);
+	struct wcss_sec *wcss = rproc->priv;
+
+	qcom_q6v5_deinit(&wcss->q6);
+
+	rproc_del(rproc);
+	rproc_free(rproc);
+}
+
+static int wcss_sec_ipq5332_init_clk(struct wcss_sec *wcss)
+{
+	int ret;
+	struct device *dev = wcss->dev;
+
+	wcss->im_sleep = devm_clk_get(wcss->dev, "im_sleep");
+	if (IS_ERR(wcss->im_sleep)) {
+		ret = PTR_ERR(wcss->im_sleep);
+		if (ret != -EPROBE_DEFER)
+			dev_err(dev, "failed to get im_sleep clock");
+		return ret;
+	}
+
+	ret = clk_prepare_enable(wcss->im_sleep);
+	if (ret) {
+		dev_err(dev, "could not enable im_sleep clk\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct wcss_data wcss_sec_ipq5332_res_init = {
+	.pasid = MPD_WCSS_PAS_ID,
+	.ops = &wcss_sec_ops,
+	.bootargs_version = 2,
+	.init_clk = wcss_sec_ipq5332_init_clk,
+};
+
+static const struct wcss_data wcss_sec_ipq9574_res_init = {
+	.pasid = WCSS_PAS_ID,
+	.ops = &wcss_sec_ops,
+};
+
+static const struct of_device_id wcss_sec_of_match[] = {
+	{ .compatible = "qcom,ipq5332-wcss-sec-pil", .data = &wcss_sec_ipq5332_res_init },
+	{ .compatible = "qcom,ipq9574-wcss-sec-pil", .data = &wcss_sec_ipq9574_res_init },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, wcss_sec_of_match);
+
+static struct platform_driver wcss_sec_driver = {
+	.probe = wcss_sec_probe,
+	.remove = wcss_sec_remove,
+	.driver = {
+		.name = "qcom-wcss-secure-pil",
+		.of_match_table = wcss_sec_of_match,
+	},
+};
+module_platform_driver(wcss_sec_driver);
+
+MODULE_DESCRIPTION("Hexagon WCSS Secure Peripheral Image Loader");
+MODULE_LICENSE("GPL");
-- 
2.34.1

[PATCH 3/4] arm64: dts: qcom: ipq5332: add nodes to bringup q6

[Gokul Sriram Palanisamy]

From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>

Enable nodes required for q6 remoteproc bring up.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq5332.dtsi | 62 +++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
index 0a74ed4f72cc..ec93e7b64b9e 100644
--- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
@@ -145,6 +145,11 @@ smem@4a800000 {
 
 			hwlocks = <&tcsr_mutex 3>;
 		};
+
+		q6_region: wcnss@4a900000 {
+			reg = <0x0 0x4a900000 0x0 0x2b00000>;
+			no-map;
+		};
 	};
 
 	soc@0 {
@@ -476,6 +481,39 @@ frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		q6v5_wcss: remoteproc@d100000 {
+			compatible = "qcom,ipq5332-wcss-sec-pil";
+			reg = <0xd100000 0x4040>;
+			firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
+			interrupts-extended = <&intc GIC_SPI 421 IRQ_TYPE_EDGE_RISING>,
+					      <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
+					      <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,
+					      <&wcss_smp2p_in 2 IRQ_TYPE_NONE>,
+					      <&wcss_smp2p_in 3 IRQ_TYPE_NONE>;
+			interrupt-names = "wdog",
+					  "fatal",
+					  "ready",
+					  "handover",
+					  "stop-ack";
+
+			clocks = <&gcc GCC_IM_SLEEP_CLK>;
+			clock-names = "im_sleep";
+
+			qcom,smem-states = <&wcss_smp2p_out 0>,
+					   <&wcss_smp2p_out 1>;
+			qcom,smem-state-names = "shutdown",
+						"stop";
+
+			memory-region = <&q6_region>;
+
+			glink-edge {
+				interrupts = <GIC_SPI 417 IRQ_TYPE_EDGE_RISING>;
+				label = "rtr";
+				qcom,remote-pid = <1>;
+				mboxes = <&apcs_glb 8>;
+			};
+		};
 	};
 
 	timer {
@@ -485,4 +523,28 @@ timer {
 			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
 	};
+
+	wcss: wcss-smp2p {
+		compatible = "qcom,smp2p";
+		qcom,smem = <435>, <428>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <GIC_SPI 418 IRQ_TYPE_EDGE_RISING>;
+
+		mboxes = <&apcs_glb 9>;
+
+		qcom,local-pid = <0>;
+		qcom,remote-pid = <1>;
+
+		wcss_smp2p_out: master-kernel {
+			qcom,entry-name = "master-kernel";
+			#qcom,smem-state-cells = <1>;
+		};
+
+		wcss_smp2p_in: slave-kernel {
+			qcom,entry-name = "slave-kernel";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
 };
-- 
2.34.1

[PATCH 4/4] arm64: dts: qcom: ipq9574: add nodes to bring up q6

[Gokul Sriram Palanisamy]

From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>

Enable nodes required for q6 remoteproc bring up.

Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
---
 arch/arm64/boot/dts/qcom/ipq9574.dtsi | 58 +++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/ipq9574.dtsi b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
index 48dfafea46a7..bca3a50d21d7 100644
--- a/arch/arm64/boot/dts/qcom/ipq9574.dtsi
+++ b/arch/arm64/boot/dts/qcom/ipq9574.dtsi
@@ -213,6 +213,11 @@ smem@4aa00000 {
 			hwlocks = <&tcsr_mutex 3>;
 			no-map;
 		};
+
+		q6_region: wcnss@4ab00000 {
+			reg = <0x0 0x4ab00000 0x0 0x2b00000>;
+			no-map;
+		};
 	};
 
 	soc: soc@0 {
@@ -756,6 +761,35 @@ frame@b128000 {
 				status = "disabled";
 			};
 		};
+
+		q6v5_wcss: remoteproc@cd00000 {
+			compatible = "qcom,ipq9574-wcss-sec-pil";
+			reg = <0x0cd00000 0x4040>;
+			firmware-name = "ath11k/IPQ9574/hw1.0/q6_fw.mdt";
+			interrupts-extended = <&intc GIC_SPI 325 IRQ_TYPE_EDGE_RISING>,
+					      <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
+					      <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,
+					      <&wcss_smp2p_in 2 IRQ_TYPE_NONE>,
+					      <&wcss_smp2p_in 3 IRQ_TYPE_NONE>;
+			interrupt-names = "wdog",
+					  "fatal",
+					  "ready",
+					  "handover",
+					  "stop-ack";
+
+			qcom,smem-states = <&wcss_smp2p_out 0>,
+					   <&wcss_smp2p_out 1>;
+			qcom,smem-state-names = "shutdown",
+						"stop";
+			memory-region = <&q6_region>;
+
+			glink-edge {
+				interrupts = <GIC_SPI 321 IRQ_TYPE_EDGE_RISING>;
+				label = "rtr";
+				qcom,remote-pid = <1>;
+				mboxes = <&apcs_glb 8>;
+			};
+		};
 	};
 
 	thermal-zones {
@@ -987,4 +1021,28 @@ timer {
 			     <GIC_PPI 4 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>,
 			     <GIC_PPI 1 (GIC_CPU_MASK_SIMPLE(4) | IRQ_TYPE_LEVEL_LOW)>;
 	};
+
+	wcss: wcss-smp2p {
+		compatible = "qcom,smp2p";
+		qcom,smem = <435>, <428>;
+
+		interrupt-parent = <&intc>;
+		interrupts = <GIC_SPI 322 IRQ_TYPE_EDGE_RISING>;
+
+		mboxes = <&apcs_glb 9>;
+
+		qcom,local-pid = <0>;
+		qcom,remote-pid = <1>;
+
+		wcss_smp2p_out: master-kernel {
+			qcom,entry-name = "master-kernel";
+			#qcom,smem-state-cells = <1>;
+		};
+
+		wcss_smp2p_in: slave-kernel {
+			qcom,entry-name = "slave-kernel";
+			interrupt-controller;
+			#interrupt-cells = <2>;
+		};
+	};
 };
-- 
2.34.1

Re: [PATCH 0/4] Add new driver for WCSS secure PIL loading

[Krzysztof Kozlowski]

On 20/08/2024 10:55, Gokul Sriram Palanisamy wrote:
> This series depends on q6 clock removal series [1].

How? So this cannot be tested and merged?

That's second patchset to day with some totally bogus dependencies.
People, stop it.


Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

[Krzysztof Kozlowski]

On Tue, Aug 20, 2024 at 02:25:14PM +0530, Gokul Sriram Palanisamy wrote:
> From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> 
> Add new binding document for hexagon based WCSS secure PIL remoteproc.
> IPQ5332, IPQ9574 follows secure PIL remoteproc.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> ---
>  .../remoteproc/qcom,wcss-sec-pil.yaml         | 125 ++++++++++++++++++
>  1 file changed, 125 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> new file mode 100644
> index 000000000000..c69401b6cec1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
> @@ -0,0 +1,125 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,wcss-sec-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm WCSS Secure Peripheral Image Loader

...

> +
> +maintainers:
> +  - Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> +
> +description:
> +  WCSS Secure Peripheral Image Loader loads firmware and power up QDSP6

What is WCSS? Don't add random acronyms without any explanation.

> +  remoteproc's on the Qualcomm IPQ9574, IPQ5332 SoC.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,ipq5332-wcss-sec-pil
> +      - qcom,ipq9574-wcss-sec-pil
> +
> +  reg:
> +    maxItems: 1
> +
> +  firmware-name:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: Firmware name for the Hexagon core

No, look how other bindings are doing it.

It looks like you develop patches on some old kernel, so please start
from scratch on newest kernel.

> +
> +  interrupts:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +
> +  clocks:
> +    items:
> +      - description: IM SLEEP clock

What is IM? Explain all acronyms.

What is SLEEP?

> +
> +  clock-names:
> +    items:
> +      - const: im_sleep

sleep? Are there different sleep clocks here?

> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the remote processor
> +    items:
> +      - description: Shutdown Q6
> +      - description: Stop Q6
> +

Do not introduce other order. First stop, then shutdown.

> +  qcom,smem-state-names:
> +    description:
> +      Names of the states used by the AP to signal the remote processor
> +    items:
> +      - const: shutdown
> +      - const: stop

The same.

> +
> +  memory-region:
> +    items:
> +      - description: Q6 reserved region
> +
> +  glink-edge:
> +    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
> +    description:
> +      Qualcomm G-Link subnode which represents communication edge, channels
> +      and devices related to the Modem.
> +    unevaluatedProperties: false
> +
> +required:
> +  - compatible
> +  - firmware-name
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - qcom,smem-states
> +  - qcom,smem-state-names
> +  - memory-region

Keep the same order as in properties.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
> +    q6v5_wcss: remoteproc@d100000 {

Drop unused label.

> +      compatible = "qcom,ipq5332-wcss-sec-pil";
> +      reg = <0xd100000 0x4040>;
> +      firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
> +      interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
> +                            <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
> +                            <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,

Best regards,
Krzysztof

Re: [PATCH 3/4] arm64: dts: qcom: ipq5332: add nodes to bringup q6

[Krzysztof Kozlowski]

On Tue, Aug 20, 2024 at 02:25:16PM +0530, Gokul Sriram Palanisamy wrote:
> From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> 
> Enable nodes required for q6 remoteproc bring up.
> 
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/ipq5332.dtsi | 62 +++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> index 0a74ed4f72cc..ec93e7b64b9e 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
> @@ -145,6 +145,11 @@ smem@4a800000 {
>  
>  			hwlocks = <&tcsr_mutex 3>;
>  		};
> +
> +		q6_region: wcnss@4a900000 {

Why here it is wcnss...

> +			reg = <0x0 0x4a900000 0x0 0x2b00000>;
> +			no-map;
> +		};
>  	};
>  
>  	soc@0 {
> @@ -476,6 +481,39 @@ frame@b128000 {
>  				status = "disabled";
>  			};
>  		};
> +
> +		q6v5_wcss: remoteproc@d100000 {

but everywhere else is wcss?

> +			compatible = "qcom,ipq5332-wcss-sec-pil";
> +			reg = <0xd100000 0x4040>;
> +			firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";

It's one firmware independent of board?

Best regards,
Krzysztof

Re: [PATCH 2/2] remoteproc: qcom: add hexagon based WCSS secure PIL driver

[Krzysztof Kozlowski]

On Tue, Aug 20, 2024 at 02:25:15PM +0530, Gokul Sriram Palanisamy wrote:
> From: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> 
> Add support to bring up hexagon based WCSS secure PIL remoteproc.
> IPQ5332, IPQ9574 supports secure PIL remoteproc.
> 
> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
> +static int wcss_sec_dump_segments(struct rproc *rproc,
> +				  const struct firmware *fw)
> +{
> +	struct device *dev = rproc->dev.parent;
> +	struct reserved_mem *rmem = NULL;
> +	struct device_node *node;
> +	int num_segs, index = 0;
> +	int ret;
> +
> +	/* Parse through additional reserved memory regions for the rproc
> +	 * and add them to the coredump segments
> +	 */
> +	num_segs = of_count_phandle_with_args(dev->of_node,
> +					      "memory-region", NULL);
> +	while (index < num_segs) {
> +		node = of_parse_phandle(dev->of_node,
> +					"memory-region", index);
> +		if (!node)
> +			return -EINVAL;
> +
> +		rmem = of_reserved_mem_lookup(node);
> +		if (!rmem) {
> +			dev_err(dev, "unable to acquire memory-region index %d num_segs %d\n",
> +				index, num_segs);

Leaking refcnt.

> +			return -EINVAL;
> +		}
> +
> +		of_node_put(node);
> +
> +		dev_dbg(dev, "Adding segment 0x%pa size 0x%pa",
> +			&rmem->base, &rmem->size);
> +		ret = rproc_coredump_add_custom_segment(rproc,
> +							rmem->base,
> +							rmem->size,
> +							wcss_sec_copy_segment,
> +							NULL);
> +		if (ret)
> +			return ret;
> +
> +		index++;
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct rproc_ops wcss_sec_ops = {
> +	.start = wcss_sec_start,
> +	.stop = wcss_sec_stop,
> +	.da_to_va = wcss_sec_da_to_va,
> +	.load = wcss_sec_load,
> +	.get_boot_addr = rproc_elf_get_boot_addr,
> +	.panic = wcss_sec_panic,
> +	.parse_fw = wcss_sec_dump_segments,
> +};
> +
> +static int wcss_sec_alloc_memory_region(struct wcss_sec *wcss)
> +{
> +	struct reserved_mem *rmem = NULL;
> +	struct device_node *node;
> +	struct device *dev = wcss->dev;
> +
> +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
> +	if (node) {
> +		rmem = of_reserved_mem_lookup(node);
> +	} else {

No, that's over complicated.

Just if (!node) { error handling }.

> +		dev_err(dev, "can't find phandle memory-region\n");
> +		return -EINVAL;
> +	}
> +
> +	of_node_put(node);
> +
> +	if (!rmem) {
> +		dev_err(dev, "unable to acquire memory-region\n");
> +		return -EINVAL;
> +	}
> +
> +	wcss->mem_phys = rmem->base;
> +	wcss->mem_reloc = rmem->base;
> +	wcss->mem_size = rmem->size;
> +	wcss->mem_region = devm_ioremap_wc(dev, wcss->mem_phys, wcss->mem_size);
> +	if (!wcss->mem_region) {
> +		dev_err(dev, "unable to map memory region: %pa+%pa\n",
> +			&rmem->base, &rmem->size);
> +		return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +

...

> +static int wcss_sec_ipq5332_init_clk(struct wcss_sec *wcss)
> +{
> +	int ret;
> +	struct device *dev = wcss->dev;
> +
> +	wcss->im_sleep = devm_clk_get(wcss->dev, "im_sleep");
> +	if (IS_ERR(wcss->im_sleep)) {
> +		ret = PTR_ERR(wcss->im_sleep);
> +		if (ret != -EPROBE_DEFER)
> +			dev_err(dev, "failed to get im_sleep clock");

Syntax is return dev_err_probe.

> +		return ret;
> +	}
> +
> +	ret = clk_prepare_enable(wcss->im_sleep);
> +	if (ret) {
> +		dev_err(dev, "could not enable im_sleep clk\n");
> +		return ret;

Just use devm_clk_get_enabled.

> +	}
> +
> +	return 0;

Best regards,
Krzysztof

Re: [PATCH 2/2] remoteproc: qcom: add hexagon based WCSS secure PIL driver

[Gokul Sriram P]

On 8/20/2024 4:55 PM, Krzysztof Kozlowski wrote:
> On Tue, Aug 20, 2024 at 02:25:15PM +0530, Gokul Sriram Palanisamy wrote:
>> From: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>>
>> Add support to bring up hexagon based WCSS secure PIL remoteproc.
>> IPQ5332, IPQ9574 supports secure PIL remoteproc.
>>
>> Signed-off-by: Vignesh Viswanathan <quic_viswanat@quicinc.com>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
>> +static int wcss_sec_dump_segments(struct rproc *rproc,
>> +				  const struct firmware *fw)
>> +{
>> +	struct device *dev = rproc->dev.parent;
>> +	struct reserved_mem *rmem = NULL;
>> +	struct device_node *node;
>> +	int num_segs, index = 0;
>> +	int ret;
>> +
>> +	/* Parse through additional reserved memory regions for the rproc
>> +	 * and add them to the coredump segments
>> +	 */
>> +	num_segs = of_count_phandle_with_args(dev->of_node,
>> +					      "memory-region", NULL);
>> +	while (index < num_segs) {
>> +		node = of_parse_phandle(dev->of_node,
>> +					"memory-region", index);
>> +		if (!node)
>> +			return -EINVAL;
>> +
>> +		rmem = of_reserved_mem_lookup(node);
>> +		if (!rmem) {
>> +			dev_err(dev, "unable to acquire memory-region index %d num_segs %d\n",
>> +				index, num_segs);
> Leaking refcnt.
Got it. Will update. Thank you.
>> +			return -EINVAL;
>> +		}
>> +
>> +		of_node_put(node);
>> +
>> +		dev_dbg(dev, "Adding segment 0x%pa size 0x%pa",
>> +			&rmem->base, &rmem->size);
>> +		ret = rproc_coredump_add_custom_segment(rproc,
>> +							rmem->base,
>> +							rmem->size,
>> +							wcss_sec_copy_segment,
>> +							NULL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		index++;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct rproc_ops wcss_sec_ops = {
>> +	.start = wcss_sec_start,
>> +	.stop = wcss_sec_stop,
>> +	.da_to_va = wcss_sec_da_to_va,
>> +	.load = wcss_sec_load,
>> +	.get_boot_addr = rproc_elf_get_boot_addr,
>> +	.panic = wcss_sec_panic,
>> +	.parse_fw = wcss_sec_dump_segments,
>> +};
>> +
>> +static int wcss_sec_alloc_memory_region(struct wcss_sec *wcss)
>> +{
>> +	struct reserved_mem *rmem = NULL;
>> +	struct device_node *node;
>> +	struct device *dev = wcss->dev;
>> +
>> +	node = of_parse_phandle(dev->of_node, "memory-region", 0);
>> +	if (node) {
>> +		rmem = of_reserved_mem_lookup(node);
>> +	} else {
> No, that's over complicated.
>
> Just if (!node) { error handling }.
Ok. Will update.
>> +		dev_err(dev, "can't find phandle memory-region\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	of_node_put(node);
>> +
>> +	if (!rmem) {
>> +		dev_err(dev, "unable to acquire memory-region\n");
>> +		return -EINVAL;
>> +	}
>> +
>> +	wcss->mem_phys = rmem->base;
>> +	wcss->mem_reloc = rmem->base;
>> +	wcss->mem_size = rmem->size;
>> +	wcss->mem_region = devm_ioremap_wc(dev, wcss->mem_phys, wcss->mem_size);
>> +	if (!wcss->mem_region) {
>> +		dev_err(dev, "unable to map memory region: %pa+%pa\n",
>> +			&rmem->base, &rmem->size);
>> +		return -ENOMEM;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
> ...
>
>> +static int wcss_sec_ipq5332_init_clk(struct wcss_sec *wcss)
>> +{
>> +	int ret;
>> +	struct device *dev = wcss->dev;
>> +
>> +	wcss->im_sleep = devm_clk_get(wcss->dev, "im_sleep");
>> +	if (IS_ERR(wcss->im_sleep)) {
>> +		ret = PTR_ERR(wcss->im_sleep);
>> +		if (ret != -EPROBE_DEFER)
>> +			dev_err(dev, "failed to get im_sleep clock");
> Syntax is return dev_err_probe.
Thanks. Will update.
>> +		return ret;
>> +	}
>> +
>> +	ret = clk_prepare_enable(wcss->im_sleep);
>> +	if (ret) {
>> +		dev_err(dev, "could not enable im_sleep clk\n");
>> +		return ret;
> Just use devm_clk_get_enabled.
Will update.
>> +	}
>> +
>> +	return 0;
> Best regards,
> Krzysztof
>

Re: [PATCH 3/4] arm64: dts: qcom: ipq5332: add nodes to bringup q6

[Gokul Sriram P]

On 8/20/2024 4:51 PM, Krzysztof Kozlowski wrote:
> On Tue, Aug 20, 2024 at 02:25:16PM +0530, Gokul Sriram Palanisamy wrote:
>> From: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>>
>> Enable nodes required for q6 remoteproc bring up.
>>
>> Signed-off-by: Manikanta Mylavarapu <quic_mmanikan@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy <quic_gokulsri@quicinc.com>
>> ---
>>   arch/arm64/boot/dts/qcom/ipq5332.dtsi | 62 +++++++++++++++++++++++++++
>>   1 file changed, 62 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/ipq5332.dtsi b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> index 0a74ed4f72cc..ec93e7b64b9e 100644
>> --- a/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/ipq5332.dtsi
>> @@ -145,6 +145,11 @@ smem@4a800000 {
>>   
>>   			hwlocks = <&tcsr_mutex 3>;
>>   		};
>> +
>> +		q6_region: wcnss@4a900000 {
> Why here it is wcnss...
will change it to wcss.
>> +			reg = <0x0 0x4a900000 0x0 0x2b00000>;
>> +			no-map;
>> +		};
>>   	};
>>   
>>   	soc@0 {
>> @@ -476,6 +481,39 @@ frame@b128000 {
>>   				status = "disabled";
>>   			};
>>   		};
>> +
>> +		q6v5_wcss: remoteproc@d100000 {
> but everywhere else is wcss?
yes, will stick to wcss everywhere.
>
>> +			compatible = "qcom,ipq5332-wcss-sec-pil";
>> +			reg = <0xd100000 0x4040>;
>> +			firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
> It's one firmware independent of board?

Yes, we have only one firmware across all our boards.

Regards,

Gokul

>
> Best regards,
> Krzysztof
>

Re: [PATCH 0/4] Add new driver for WCSS secure PIL loading

[Gokul Sriram P]

On 8/20/2024 4:42 PM, Krzysztof Kozlowski wrote:
> On 20/08/2024 10:55, Gokul Sriram Palanisamy wrote:
>> This series depends on q6 clock removal series [1].
> How? So this cannot be tested and merged?

Yes. Though TrustZone enables these clocks, since Linux Kernel will 
consider these clock as unused.
These clock will be disabled so we cannot bring Q6 out of reset. So we 
have the dependency set.
I posted this as a separate series because [1] 'remove unnecessary q6 
clocks' series was already reviewed for some
versions.

[1] 
https://patchwork.kernel.org/project/linux-arm-msm/cover/20240820055618.267554-1-quic_gokulsri@quicinc.com/

Regards,

Gokul

> That's second patchset to day with some totally bogus dependencies.
> People, stop it.
>
>
> Best regards,
> Krzysztof
>

Re: [PATCH 1/2] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

[Gokul Sriram P]

On 8/20/2024 4:50 PM, Krzysztof Kozlowski wrote:
> On Tue, Aug 20, 2024 at 02:25:14PM +0530, Gokul Sriram Palanisamy wrote:
>> From: Manikanta Mylavarapu<quic_mmanikan@quicinc.com>
>>
>> Add new binding document for hexagon based WCSS secure PIL remoteproc.
>> IPQ5332, IPQ9574 follows secure PIL remoteproc.
>>
>> Signed-off-by: Manikanta Mylavarapu<quic_mmanikan@quicinc.com>
>> Signed-off-by: Gokul Sriram Palanisamy<quic_gokulsri@quicinc.com>
>> ---
>>   .../remoteproc/qcom,wcss-sec-pil.yaml         | 125 ++++++++++++++++++
>>   1 file changed, 125 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
>> new file mode 100644
>> index 000000000000..c69401b6cec1
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,wcss-sec-pil.yaml
>> @@ -0,0 +1,125 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id:http://devicetree.org/schemas/remoteproc/qcom,wcss-sec-pil.yaml#
>> +$schema:http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm WCSS Secure Peripheral Image Loader
> ...
>
>> +
>> +maintainers:
>> +  - Manikanta Mylavarapu<quic_mmanikan@quicinc.com>
>> +
>> +description:
>> +  WCSS Secure Peripheral Image Loader loads firmware and power up QDSP6
> What is WCSS? Don't add random acronyms without any explanation.

WCSS/WCNSS - Wireless Connectivity subsystem.

will update.

>> +  remoteproc's on the Qualcomm IPQ9574, IPQ5332 SoC.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,ipq5332-wcss-sec-pil
>> +      - qcom,ipq9574-wcss-sec-pil
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  firmware-name:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: Firmware name for the Hexagon core
> No, look how other bindings are doing it.
>
> It looks like you develop patches on some old kernel, so please start
> from scratch on newest kernel.
will update.
>> +
>> +  interrupts:
>> +    items:
>> +      - description: Watchdog interrupt
>> +      - description: Fatal interrupt
>> +      - description: Ready interrupt
>> +      - description: Handover interrupt
>> +      - description: Stop acknowledge interrupt
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: wdog
>> +      - const: fatal
>> +      - const: ready
>> +      - const: handover
>> +      - const: stop-ack
>> +
>> +  clocks:
>> +    items:
>> +      - description: IM SLEEP clock
> What is IM? Explain all acronyms.
>
> What is SLEEP?

IM_SLEEP_CLK - Internal Module sleep clock needed for Q6 reset.

SLEEP is not an acronym here.

>> +
>> +  clock-names:
>> +    items:
>> +      - const: im_sleep
> sleep? Are there different sleep clocks here?

We have different branches of sleep clk each enabled separately.

im_sleep is one of those branches that q6 uses.

>> +
>> +  qcom,smem-states:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: States used by the AP to signal the remote processor
>> +    items:
>> +      - description: Shutdown Q6
>> +      - description: Stop Q6
>> +
> Do not introduce other order. First stop, then shutdown.
will update
>> +  qcom,smem-state-names:
>> +    description:
>> +      Names of the states used by the AP to signal the remote processor
>> +    items:
>> +      - const: shutdown
>> +      - const: stop
> The same.
will update.
>> +
>> +  memory-region:
>> +    items:
>> +      - description: Q6 reserved region
>> +
>> +  glink-edge:
>> +    $ref: /schemas/remoteproc/qcom,glink-edge.yaml#
>> +    description:
>> +      Qualcomm G-Link subnode which represents communication edge, channels
>> +      and devices related to the Modem.
>> +    unevaluatedProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - firmware-name
>> +  - reg
>> +  - interrupts
>> +  - interrupt-names
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +  - memory-region
> Keep the same order as in properties.
ok.
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,ipq5332-gcc.h>
>> +    q6v5_wcss: remoteproc@d100000 {
> Drop unused label.

ok.

Regards,

Gokul

>> +      compatible = "qcom,ipq5332-wcss-sec-pil";
>> +      reg = <0xd100000 0x4040>;
>> +      firmware-name = "ath12k/IPQ5332/hw1.0/q6_fw0.mdt";
>> +      interrupts-extended = <&intc GIC_SPI 291 IRQ_TYPE_EDGE_RISING>,
>> +                            <&wcss_smp2p_in 0 IRQ_TYPE_NONE>,
>> +                            <&wcss_smp2p_in 1 IRQ_TYPE_NONE>,
> Best regards,
> Krzysztof
>

Re: [PATCH 0/4] Add new driver for WCSS secure PIL loading

[Krzysztof Kozlowski]

On 22/08/2024 12:43, Gokul Sriram P wrote:
> 
> On 8/20/2024 4:42 PM, Krzysztof Kozlowski wrote:
>> On 20/08/2024 10:55, Gokul Sriram Palanisamy wrote:
>>> This series depends on q6 clock removal series [1].
>> How? So this cannot be tested and merged?
> 
> Yes. Though TrustZone enables these clocks, since Linux Kernel will 
> consider these clock as unused.
> These clock will be disabled so we cannot bring Q6 out of reset. So we 
> have the dependency set.
> I posted this as a separate series because [1] 'remove unnecessary q6 
> clocks' series was already reviewed for some
> versions.

This is not a dependency in the kernel workflow. Nothing gets broken,
nothing stops this patch from merging. Your remark is confusing and will
either start questions or prevent applying the patchset.

Best regards,
Krzysztof

Re: [PATCH 1/2] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

[Krzysztof Kozlowski]

On 22/08/2024 12:47, Gokul Sriram P wrote:
>>> +
>>> +  interrupts:
>>> +    items:
>>> +      - description: Watchdog interrupt
>>> +      - description: Fatal interrupt
>>> +      - description: Ready interrupt
>>> +      - description: Handover interrupt
>>> +      - description: Stop acknowledge interrupt
>>> +
>>> +  interrupt-names:
>>> +    items:
>>> +      - const: wdog
>>> +      - const: fatal
>>> +      - const: ready
>>> +      - const: handover
>>> +      - const: stop-ack
>>> +
>>> +  clocks:
>>> +    items:
>>> +      - description: IM SLEEP clock
>> What is IM? Explain all acronyms.
>>
>> What is SLEEP?
> 
> IM_SLEEP_CLK - Internal Module sleep clock needed for Q6 reset.
> 
> SLEEP is not an acronym here.

Then probably you mean "Internal sleep", although "internal" is also
confusing. Devices do not receive as input something which is internal
to them.

> 
>>> +
>>> +  clock-names:
>>> +    items:
>>> +      - const: im_sleep
>> sleep? Are there different sleep clocks here?
> 
> We have different branches of sleep clk each enabled separately.
> 
> im_sleep is one of those branches that q6 uses.

So this device misses other branches? Then provide them. Otherwise it is
just "sleep".



Best regards,
Krzysztof

Re: [PATCH 0/4] Add new driver for WCSS secure PIL loading

[Gokul Sriram P]

On 8/22/2024 4:58 PM, Krzysztof Kozlowski wrote:
> On 22/08/2024 12:43, Gokul Sriram P wrote:
>> On 8/20/2024 4:42 PM, Krzysztof Kozlowski wrote:
>>> On 20/08/2024 10:55, Gokul Sriram Palanisamy wrote:
>>>> This series depends on q6 clock removal series [1].
>>> How? So this cannot be tested and merged?
>> Yes. Though TrustZone enables these clocks, since Linux Kernel will
>> consider these clock as unused.
>> These clock will be disabled so we cannot bring Q6 out of reset. So we
>> have the dependency set.
>> I posted this as a separate series because [1] 'remove unnecessary q6
>> clocks' series was already reviewed for some
>> versions.
> This is not a dependency in the kernel workflow. Nothing gets broken,
> nothing stops this patch from merging. Your remark is confusing and will
> either start questions or prevent applying the patchset.
>
Ok, got it. It will not break anything. Will post this series as 
independent series in v2.

thanks,

Gokul

Re: [PATCH 1/2] dt-bindings: remoteproc: qcom: document hexagon based WCSS secure PIL

[Krzysztof Kozlowski]

On 23/08/2024 11:47, Gokul Sriram P wrote:
> 
> On 8/22/2024 5:00 PM, Krzysztof Kozlowski wrote:
>>> IM_SLEEP_CLK - Internal Module sleep clock needed for Q6 reset.
>>>
>>> SLEEP is not an acronym here.
>> Then probably you mean "Internal sleep", although "internal" is also
>> confusing. Devices do not receive as input something which is internal
>> to them.
>>
>>>>> +
>>>>> +  clock-names:
>>>>> +    items:
>>>>> +      - const: im_sleep
>>>> sleep? Are there different sleep clocks here?
>>> We have different branches of sleep clk each enabled separately.
>>>
>>> im_sleep is one of those branches that q6 uses.
>> So this device misses other branches? Then provide them. Otherwise it is
>> just "sleep".
> 
> ok, I shall keep it as simply "sleep" and move on? Other branches of 
> sleep clk are irrelevant to remoteproc.

Then it is "sleep". The name here describes the clock input in this
device, not the clock in other places.

Best regards,
Krzysztof