[PATCH v4 0/7] Update ADSP pil loader for SC7280 platform

[PATCH v4 0/7] Update ADSP pil loader for SC7280 platform

[Srinivasa Rao Mandadapu]

Update ADSP pil loader driver for SC7280 platforms.

Changes since V3:
	-- Rename is_adsp_sb_needed to adsp_sandbox_needed.
	-- Update sc7280 compatible name entry in sorted order.
	-- Add smmu unmapping in error case and in adsp stop.
	-- Revert converting sdm845 dt bindings to generic and 
	   create new dt bindings for sc7280.
Changes since V2:
	-- Generated patch with -M flag.
	-- Add Clock property in dt bindings.
	-- Add qcom,adsp-memory-regions property.
	-- Add is_adsp_sb_needed flag instead of is_wpss.
	-- Initialize is_adsp_sb_needed flag.
	-- Remove empty proxy pds array.
	-- Replace platform_bus_type with adsp->dev->bus.
	-- Use API of_parse_phandle_with_args() instead of 
	    of_parse_phandle_with_fixed_args().
	-- Replace adsp->is_wpss with adsp->is_adsp.
	-- Update error handling in adsp_start().
Changes since V1:
	-- Change reg property maxItems to minItems and update description.
	-- Fix typo errors.
Srinivasa Rao Mandadapu (7):
  dt-bindings: remoteproc: qcom: Add SC7280 ADSP support
  remoteproc: qcom: Add flag in adsp private data structure
  remoteproc: qcom: Add compatible name for SC7280 ADSP
  remoteproc: qcom: Replace hard coded values with macros
  remoteproc: qcom: Add efuse evb selection control
  remoteproc: qcom: Add support for memory sandbox
  remoteproc: qcom: Update QDSP6 out-of-reset timeout value

 .../bindings/remoteproc/qcom,sc7280-adsp-pil.yaml  | 189 ++++++++++++++++++
 drivers/remoteproc/qcom_q6v5_adsp.c                | 215 ++++++++++++++++++++-
 2 files changed, 398 insertions(+), 6 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml

-- 
2.7.4

[PATCH v4 1/7] dt-bindings: remoteproc: qcom: Add SC7280 ADSP support

[Srinivasa Rao Mandadapu]

Add ADSP PIL loading support for SC7280 SoCs.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
 .../bindings/remoteproc/qcom,sc7280-adsp-pil.yaml  | 189 +++++++++++++++++++++
 1 file changed, 189 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
new file mode 100644
index 0000000..e656cc8
--- /dev/null
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
@@ -0,0 +1,189 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-adsp-pil.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Qualcomm SC7280 ADSP Peripheral Image Loader
+
+maintainers:
+  - Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
+
+description:
+  This document defines the binding for a component that loads and boots firmware
+  on the Qualcomm Technology Inc. ADSP.
+
+properties:
+  compatible:
+    enum:
+      - qcom,sc7280-adsp-pil
+
+  reg:
+    minItems: 1
+    items:
+      - description: qdsp6ss register
+      - description: efuse q6ss register
+
+  interrupts:
+    items:
+      - description: Watchdog interrupt
+      - description: Fatal interrupt
+      - description: Ready interrupt
+      - description: Handover interrupt
+      - description: Stop acknowledge interrupt
+      - description: Shutdown acknowledge interrupt
+
+  interrupt-names:
+    items:
+      - const: wdog
+      - const: fatal
+      - const: ready
+      - const: handover
+      - const: stop-ack
+      - const: shutdown-ack
+
+  clocks:
+    items:
+      - description: XO clock
+      - description: GCC CFG NOC LPASS clock
+      - description: LPASS AHBS AON clock
+      - description: LPASS AHBM AON clock
+      - description: QDSP XO clock
+      - description: Q6SP6SS SLEEP clock
+      - description: Q6SP6SS CORE clock
+
+  clock-names:
+    items:
+      - const: xo
+      - const: gcc_cfg_noc_lpass
+      - const: lpass_ahbs_aon_cbcr
+      - const: lpass_ahbm_aon_cbcr
+      - const: qdsp6ss_xo
+      - const: qdsp6ss_sleep
+      - const: qdsp6ss_core
+
+  power-domains:
+    items:
+      - description: LCX power domain
+
+  resets:
+    items:
+      - description: PDC AUDIO SYNC RESET
+      - description: CC LPASS restart
+
+  reset-names:
+    items:
+      - const: pdc_sync
+      - const: cc_lpass
+
+  memory-region:
+    maxItems: 1
+    description: Reference to the reserved-memory for the Hexagon core
+
+  qcom,adsp-memory-regions:
+    $ref: /schemas/types.yaml#/definitions/uint32-matrix
+    description:
+      Each entry consists of 4 integers and represents the
+      list of memory regions accessed by ADSP firmware.
+    items:
+      items:
+        - description: |
+            "iova reg" indicates the address of virtual memory region.
+        - description: |
+            "physical reg" indicates the address of phyical memory region.
+        - description: |
+            "size" indicates the offset memory region.
+        - description: |
+            "access level" indicates the read, read and write access levels.
+          minimum: 0
+          maximum: 1
+
+  qcom,halt-regs:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description:
+      Phandle reference to a syscon representing TCSR followed by the
+      three offsets within syscon for q6, modem and nc halt registers.
+
+  qcom,smem-states:
+    $ref: /schemas/types.yaml#/definitions/phandle-array
+    description: States used by the AP to signal the Hexagon core
+    items:
+      - description: Stop the modem
+
+  qcom,smem-state-names:
+    $ref: /schemas/types.yaml#/definitions/string
+    description: The names of the state bits used for SMP2P output
+    items:
+      - const: stop
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - interrupt-names
+  - clocks
+  - clock-names
+  - power-domains
+  - resets
+  - reset-names
+  - qcom,halt-regs
+  - memory-region
+  - qcom,smem-states
+  - qcom,smem-state-names
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/clock/qcom,rpmh.h>
+    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
+    #include <dt-bindings/clock/qcom,lpass-sc7280.h>
+    #include <dt-bindings/reset/qcom,sdm845-aoss.h>
+    #include <dt-bindings/reset/qcom,sdm845-pdc.h>
+    #include <dt-bindings/power/qcom-rpmpd.h>
+
+    remoteproc@3000000 {
+        compatible = "qcom,sc7280-adsp-pil";
+        reg = <0x03000000 0x5000>,
+              <0x355B000 0x10>;
+
+        interrupts-extended = <&pdc 162 IRQ_TYPE_EDGE_RISING>,
+                <&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
+                <&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
+                <&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
+                <&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
+                <&adsp_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
+
+        interrupt-names = "wdog", "fatal", "ready",
+                "handover", "stop-ack", "shutdown-ack";
+
+        clocks = <&rpmhcc RPMH_CXO_CLK>,
+                 <&gcc GCC_CFG_NOC_LPASS_CLK>,
+                 <&lpasscc LPASS_Q6SS_AHBM_CLK>,
+                 <&lpasscc LPASS_Q6SS_AHBS_CLK>,
+                 <&lpasscc LPASS_QDSP6SS_XO_CLK>,
+                 <&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
+                 <&lpasscc LPASS_QDSP6SS_CORE_CLK>;
+        clock-names = "xo", "gcc_cfg_noc_lpass",
+                "lpass_ahbs_aon_cbcr",
+                "lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
+                "qdsp6ss_sleep", "qdsp6ss_core";
+
+        power-domains = <&rpmhpd SC7280_LCX>;
+
+        resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
+                 <&aoss_reset AOSS_CC_LPASS_RESTART>;
+        reset-names = "pdc_sync", "cc_lpass";
+
+        qcom,halt-regs = <&tcsr_mutex 0x23000 0x25000 0x28000 0x33000>;
+
+        memory-region = <&adsp_mem>;
+
+        qcom,smem-states = <&adsp_smp2p_out 0>;
+        qcom,smem-state-names = "stop";
+
+        qcom,adsp-memory-regions = <0x00100000 0x00100000 0x4000 0>,
+                                   <0x00113000 0x00113000 0x1000 0>,
+                                   <0x00117000 0x00117000 0x2000 1>;
+    };
-- 
2.7.4

[PATCH v4 2/7] remoteproc: qcom: Add flag in adsp private data structure

[Srinivasa Rao Mandadapu]

Add flag in qcom_adsp private data structure and initialize
it to distinguish ADSP and WPSS modules.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
Changes since V3:
	-- Rename is_adsp_sb_needed to adsp_sandbox_needed.
Changes since V2:
	-- Add is_adsp_sb_needed flag instead of is_wpss.

 drivers/remoteproc/qcom_q6v5_adsp.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 2f3b9f5..d0b767f 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -62,6 +62,7 @@ struct adsp_pil_data {
 	const char *sysmon_name;
 	int ssctl_id;
 	bool is_wpss;
+	bool adsp_sandbox_needed;
 	bool auto_boot;
 
 	const char **clk_ids;
@@ -99,6 +100,7 @@ struct qcom_adsp {
 	phys_addr_t mem_reloc;
 	void *mem_region;
 	size_t mem_size;
+	bool adsp_sandbox_needed;
 
 	struct device *proxy_pds[QCOM_Q6V5_RPROC_PROXY_PD_MAX];
 	size_t proxy_pd_count;
@@ -602,6 +604,8 @@ static int adsp_probe(struct platform_device *pdev)
 	adsp->dev = &pdev->dev;
 	adsp->rproc = rproc;
 	adsp->info_name = desc->sysmon_name;
+	adsp->adsp_sandbox_needed = desc->adsp_sandbox_needed;
+
 	platform_set_drvdata(pdev, adsp);
 
 	if (desc->is_wpss)
-- 
2.7.4

[PATCH v4 3/7] remoteproc: qcom: Add compatible name for SC7280 ADSP

[Srinivasa Rao Mandadapu]

Update adsp pil data and compatible name for loading ADSP
binary on SC7280 based platforms.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
Changes since V3:
	-- Rename is_adsp_sb_needed to adsp_sandbox_needed.
	-- Update sc7280 compatible name entry in sorted order.
Changes since V2:
	-- Initialize is_adsp_sb_needed flag.
	-- Remove empty proxy pds array.

 drivers/remoteproc/qcom_q6v5_adsp.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index d0b767f..6d409ca 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -701,6 +701,22 @@ static const struct adsp_pil_data adsp_resource_init = {
 	},
 };
 
+static const struct adsp_pil_data adsp_sc7280_resource_init = {
+	.crash_reason_smem = 423,
+	.firmware_name = "adsp.mbn",
+	.load_state = "adsp",
+	.ssr_name = "lpass",
+	.sysmon_name = "adsp",
+	.ssctl_id = 0x14,
+	.is_wpss = false,
+	.adsp_sandbox_needed = true,
+	.auto_boot = true,
+	.clk_ids = (const char*[]) {
+		"gcc_cfg_noc_lpass", NULL
+	},
+	.num_clks = 1,
+};
+
 static const struct adsp_pil_data cdsp_resource_init = {
 	.crash_reason_smem = 601,
 	.firmware_name = "cdsp.mdt",
@@ -739,6 +755,7 @@ static const struct adsp_pil_data wpss_resource_init = {
 
 static const struct of_device_id adsp_of_match[] = {
 	{ .compatible = "qcom,qcs404-cdsp-pil", .data = &cdsp_resource_init },
+	{ .compatible = "qcom,sc7280-adsp-pil", .data = &adsp_sc7280_resource_init },
 	{ .compatible = "qcom,sc7280-wpss-pil", .data = &wpss_resource_init },
 	{ .compatible = "qcom,sdm845-adsp-pil", .data = &adsp_resource_init },
 	{ },
-- 
2.7.4

[PATCH v4 4/7] remoteproc: qcom: Replace hard coded values with macros

[Srinivasa Rao Mandadapu]

Replace hard coded values of QDSP6 boot control reg params
with appropriate macro names.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/remoteproc/qcom_q6v5_adsp.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 6d409ca..701a615 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -54,6 +54,9 @@
 
 #define QCOM_Q6V5_RPROC_PROXY_PD_MAX	3
 
+#define LPASS_BOOT_CORE_START	BIT(0)
+#define LPASS_BOOT_CMD_START	BIT(0)
+
 struct adsp_pil_data {
 	int crash_reason_smem;
 	const char *firmware_name;
@@ -366,10 +369,10 @@ static int adsp_start(struct rproc *rproc)
 	writel(adsp->mem_phys >> 4, adsp->qdsp6ss_base + RST_EVB_REG);
 
 	/* De-assert QDSP6 stop core. QDSP6 will execute after out of reset */
-	writel(0x1, adsp->qdsp6ss_base + CORE_START_REG);
+	writel(LPASS_BOOT_CORE_START, adsp->qdsp6ss_base + CORE_START_REG);
 
 	/* Trigger boot FSM to start QDSP6 */
-	writel(0x1, adsp->qdsp6ss_base + BOOT_CMD_REG);
+	writel(LPASS_BOOT_CMD_START, adsp->qdsp6ss_base + BOOT_CMD_REG);
 
 	/* Wait for core to come out of reset */
 	ret = readl_poll_timeout(adsp->qdsp6ss_base + BOOT_STATUS_REG,
-- 
2.7.4

[PATCH v4 5/7] remoteproc: qcom: Add efuse evb selection control

[Srinivasa Rao Mandadapu]

Add efuse evb selection control and enable it for starting ADSP.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_adsp.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index 701a615..b0a63a0 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -56,6 +56,7 @@
 
 #define LPASS_BOOT_CORE_START	BIT(0)
 #define LPASS_BOOT_CMD_START	BIT(0)
+#define LPASS_EFUSE_Q6SS_EVB_SEL 0x0
 
 struct adsp_pil_data {
 	int crash_reason_smem;
@@ -86,6 +87,7 @@ struct qcom_adsp {
 	struct clk_bulk_data *clks;
 
 	void __iomem *qdsp6ss_base;
+	void __iomem *lpass_efuse;
 
 	struct reset_control *pdc_sync_reset;
 	struct reset_control *restart;
@@ -368,6 +370,9 @@ static int adsp_start(struct rproc *rproc)
 	/* Program boot address */
 	writel(adsp->mem_phys >> 4, adsp->qdsp6ss_base + RST_EVB_REG);
 
+	if (adsp->lpass_efuse)
+		writel(LPASS_EFUSE_Q6SS_EVB_SEL, adsp->lpass_efuse);
+
 	/* De-assert QDSP6 stop core. QDSP6 will execute after out of reset */
 	writel(LPASS_BOOT_CORE_START, adsp->qdsp6ss_base + CORE_START_REG);
 
@@ -522,6 +527,11 @@ static int adsp_init_mmio(struct qcom_adsp *adsp,
 		return PTR_ERR(adsp->qdsp6ss_base);
 	}
 
+	adsp->lpass_efuse =  devm_platform_ioremap_resource_byname(pdev, "lpass_efuse");
+	if (IS_ERR(adsp->lpass_efuse)) {
+		adsp->lpass_efuse = NULL;
+		dev_dbg(adsp->dev, "failed to map LPASS efuse registers\n");
+	}
 	syscon = of_parse_phandle(pdev->dev.of_node, "qcom,halt-regs", 0);
 	if (!syscon) {
 		dev_err(&pdev->dev, "failed to parse qcom,halt-regs\n");
-- 
2.7.4

[PATCH v4 6/7] remoteproc: qcom: Add support for memory sandbox

[Srinivasa Rao Mandadapu]

Update pil driver with SMMU mapping for allowing authorised
memory access to ADSP firmware, by reading required memory
regions either from device tree file or from resource table
embedded in ADSP binary header.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
Changes since V3:
	-- Rename is_adsp_sb_needed to adsp_sandbox_needed.
	-- Add smmu unmapping in error case and in adsp stop.
Changes since V2:
	-- Replace platform_bus_type with adsp->dev->bus.
	-- Use API of_parse_phandle_with_args() instead of of_parse_phandle_with_fixed_args().
	-- Replace adsp->is_wpss with adsp->is_adsp.
	-- Update error handling in adsp_start().

 drivers/remoteproc/qcom_q6v5_adsp.c | 172 +++++++++++++++++++++++++++++++++++-
 1 file changed, 170 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index b0a63a0..ca45d2c 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -9,6 +9,7 @@
 #include <linux/firmware.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
+#include <linux/iommu.h>
 #include <linux/iopoll.h>
 #include <linux/kernel.h>
 #include <linux/mfd/syscon.h>
@@ -48,6 +49,8 @@
 #define LPASS_PWR_ON_REG		0x10
 #define LPASS_HALTREQ_REG		0x0
 
+#define SID_MASK_DEFAULT        0xF
+
 #define QDSP6SS_XO_CBCR		0x38
 #define QDSP6SS_CORE_CBCR	0x20
 #define QDSP6SS_SLEEP_CBCR	0x3c
@@ -78,7 +81,7 @@ struct adsp_pil_data {
 struct qcom_adsp {
 	struct device *dev;
 	struct rproc *rproc;
-
+	struct iommu_domain *iommu_dom;
 	struct qcom_q6v5 q6v5;
 
 	struct clk *xo;
@@ -333,6 +336,155 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
+{
+	unsigned long mem_phys;
+	unsigned long iova;
+	unsigned int mem_size;
+	int access_level;
+	int i;
+
+	for (i = 0; i < len; i++) {
+		iova = be32_to_cpu(prop[i++]);
+		mem_phys = be32_to_cpu(prop[i++]);
+		mem_size = be32_to_cpu(prop[i++]);
+		access_level = be32_to_cpu(prop[i]);
+		iommu_unmap(iommu_dom, iova, mem_size);
+	}
+}
+
+static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
+{
+	struct fw_rsc_devmem *rsc_fw;
+	struct fw_rsc_hdr *hdr;
+	int offset;
+	int i;
+
+	for (i = 0; i < len; i++) {
+		offset = rproc->table_ptr->offset[i];
+		hdr = (void *)rproc->table_ptr + offset;
+		rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
+
+		iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
+	}
+}
+
+static void adsp_unmap_smmu(struct rproc *rproc)
+{
+	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
+	const __be32 *prop;
+	unsigned int len;
+
+	prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
+	if (prop) {
+		adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
+	} else {
+		if (rproc->table_ptr)
+			adsp_rproc_unmap_smmu(rproc, rproc->table_ptr->num);
+	}
+}
+
+static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
+{
+	struct of_phandle_args args;
+	struct fw_rsc_devmem *rsc_fw;
+	struct fw_rsc_hdr *hdr;
+	const __be32 *prop;
+	long long sid;
+	unsigned long mem_phys;
+	unsigned long iova;
+	unsigned int mem_size;
+	unsigned int flag;
+	unsigned int len;
+	int access_level;
+	int offset;
+	int ret;
+	int rc;
+	int i;
+
+	rc = of_parse_phandle_with_args(adsp->dev->of_node, "iommus", "#iommu-cells", 0, &args);
+	if (rc < 0)
+		sid = -1;
+	else
+		sid = args.args[0] & SID_MASK_DEFAULT;
+
+	adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
+	if (!adsp->iommu_dom) {
+		dev_err(adsp->dev, "failed to allocate iommu domain\n");
+		ret = -ENOMEM;
+		goto domain_free;
+	}
+
+	ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
+	if (ret) {
+		dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
+		ret = -EBUSY;
+		goto detach_device;
+	}
+
+	/* Add SID configuration for ADSP Firmware to SMMU */
+	adsp->mem_phys =  adsp->mem_phys | (sid << 32);
+
+	ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, adsp->mem_phys,
+			adsp->mem_size,	IOMMU_READ | IOMMU_WRITE);
+	if (ret) {
+		dev_err(adsp->dev, "Unable to map ADSP Physical Memory\n");
+		goto sid_unmap;
+	}
+
+	prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
+	if (prop) {
+		len /= sizeof(__be32);
+		for (i = 0; i < len; i++) {
+			iova = be32_to_cpu(prop[i++]);
+			mem_phys = be32_to_cpu(prop[i++]);
+			mem_size = be32_to_cpu(prop[i++]);
+			access_level = be32_to_cpu(prop[i]);
+
+			if (access_level)
+				flag = IOMMU_READ | IOMMU_WRITE;
+			else
+				flag = IOMMU_READ;
+
+			ret = iommu_map(adsp->iommu_dom, iova, mem_phys, mem_size, flag);
+			if (ret) {
+				dev_err(adsp->dev, "failed to map addr = %p mem_size = %x\n",
+						&(mem_phys), mem_size);
+				goto smmu_unmap;
+			}
+		}
+	} else {
+		if (!rproc->table_ptr)
+			goto sid_unmap;
+
+		for (i = 0; i < rproc->table_ptr->num; i++) {
+			offset = rproc->table_ptr->offset[i];
+			hdr = (void *)rproc->table_ptr + offset;
+			rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
+
+			ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
+						rsc_fw->len, rsc_fw->flags);
+			if (ret) {
+				pr_err("%s; unable to map adsp memory address\n", __func__);
+				goto rproc_smmu_unmap;
+			}
+		}
+	}
+	return 0;
+rproc_smmu_unmap:
+	adsp_rproc_unmap_smmu(rproc, i);
+	goto sid_unmap;
+smmu_unmap:
+	adsp_of_unmap_smmu(adsp->iommu_dom, prop, i);
+sid_unmap:
+	iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
+detach_device:
+	iommu_domain_free(adsp->iommu_dom);
+domain_free:
+	return ret;
+}
+
+
 static int adsp_start(struct rproc *rproc)
 {
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
@@ -343,9 +495,16 @@ static int adsp_start(struct rproc *rproc)
 	if (ret)
 		return ret;
 
+	if (adsp->adsp_sandbox_needed) {
+		ret = adsp_map_smmu(adsp, rproc);
+		if (ret) {
+			dev_err(adsp->dev, "ADSP smmu mapping failed\n");
+			goto disable_irqs;
+		}
+	}
 	ret = clk_prepare_enable(adsp->xo);
 	if (ret)
-		goto disable_irqs;
+		goto adsp_smmu_unmap;
 
 	ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
 				    adsp->proxy_pd_count);
@@ -401,6 +560,12 @@ static int adsp_start(struct rproc *rproc)
 	qcom_rproc_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
 disable_xo_clk:
 	clk_disable_unprepare(adsp->xo);
+adsp_smmu_unmap:
+	if (adsp->adsp_sandbox_needed) {
+		iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
+		adsp_unmap_smmu(rproc);
+		iommu_domain_free(adsp->iommu_dom);
+	}
 disable_irqs:
 	qcom_q6v5_unprepare(&adsp->q6v5);
 
@@ -429,6 +594,9 @@ static int adsp_stop(struct rproc *rproc)
 	if (ret)
 		dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
 
+	if (adsp->adsp_sandbox_needed)
+		adsp_unmap_smmu(rproc);
+
 	handover = qcom_q6v5_unprepare(&adsp->q6v5);
 	if (handover)
 		qcom_adsp_pil_handover(&adsp->q6v5);
-- 
2.7.4

[PATCH v4 7/7] remoteproc: qcom: Update QDSP6 out-of-reset timeout value

[Srinivasa Rao Mandadapu]

Update QDSP6 out-of-reset timeout value to 1 second, as sometimes
ADSP boot failing on SC7280 based platforms with existing value.
Also add few micro seconds sleep after enabling boot core
start register.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
 drivers/remoteproc/qcom_q6v5_adsp.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index ca45d2c..2b5b172 100644
--- a/drivers/remoteproc/qcom_q6v5_adsp.c
+++ b/drivers/remoteproc/qcom_q6v5_adsp.c
@@ -34,7 +34,7 @@
 /* time out value */
 #define ACK_TIMEOUT			1000
 #define ACK_TIMEOUT_US			1000000
-#define BOOT_FSM_TIMEOUT		10000
+#define BOOT_FSM_TIMEOUT		1000000
 /* mask values */
 #define EVB_MASK			GENMASK(27, 4)
 /*QDSP6SS register offsets*/
@@ -534,13 +534,14 @@ static int adsp_start(struct rproc *rproc)
 
 	/* De-assert QDSP6 stop core. QDSP6 will execute after out of reset */
 	writel(LPASS_BOOT_CORE_START, adsp->qdsp6ss_base + CORE_START_REG);
+	usleep_range(100, 110);
 
 	/* Trigger boot FSM to start QDSP6 */
 	writel(LPASS_BOOT_CMD_START, adsp->qdsp6ss_base + BOOT_CMD_REG);
 
 	/* Wait for core to come out of reset */
 	ret = readl_poll_timeout(adsp->qdsp6ss_base + BOOT_STATUS_REG,
-			val, (val & BIT(0)) != 0, 10, BOOT_FSM_TIMEOUT);
+			val, (val & BIT(0)) != 0, 100, BOOT_FSM_TIMEOUT);
 	if (ret) {
 		dev_err(adsp->dev, "failed to bootup adsp\n");
 		goto disable_adsp_clks;
-- 
2.7.4

Re: [PATCH v4 6/7] remoteproc: qcom: Add support for memory sandbox

[kernel test robot]

Hi Srinivasa,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on remoteproc/rproc-next]
[also build test WARNING on linus/master v5.19 next-20220812]
[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/Srinivasa-Rao-Mandadapu/Update-ADSP-pil-loader-for-SC7280-platform/20220812-205239
base:   git://git.kernel.org/pub/scm/linux/kernel/git/remoteproc/linux.git rproc-next
config: arm-allyesconfig (https://download.01.org/0day-ci/archive/20220813/202208131734.HEn8peGd-lkp@intel.com/config)
compiler: arm-linux-gnueabi-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/intel-lab-lkp/linux/commit/1d330f9a7446932416d55d93ebba00e3d16bbef9
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Srinivasa-Rao-Mandadapu/Update-ADSP-pil-loader-for-SC7280-platform/20220812-205239
        git checkout 1d330f9a7446932416d55d93ebba00e3d16bbef9
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=arm SHELL=/bin/bash drivers/remoteproc/

If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/remoteproc/qcom_q6v5_adsp.c: In function 'adsp_of_unmap_smmu':
>> drivers/remoteproc/qcom_q6v5_adsp.c:343:13: warning: variable 'access_level' set but not used [-Wunused-but-set-variable]
     343 |         int access_level;
         |             ^~~~~~~~~~~~
>> drivers/remoteproc/qcom_q6v5_adsp.c:340:23: warning: variable 'mem_phys' set but not used [-Wunused-but-set-variable]
     340 |         unsigned long mem_phys;
         |                       ^~~~~~~~


vim +/access_level +343 drivers/remoteproc/qcom_q6v5_adsp.c

   337	
   338	static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
   339	{
 > 340		unsigned long mem_phys;
   341		unsigned long iova;
   342		unsigned int mem_size;
 > 343		int access_level;
   344		int i;
   345	
   346		for (i = 0; i < len; i++) {
   347			iova = be32_to_cpu(prop[i++]);
   348			mem_phys = be32_to_cpu(prop[i++]);
   349			mem_size = be32_to_cpu(prop[i++]);
   350			access_level = be32_to_cpu(prop[i]);
   351			iommu_unmap(iommu_dom, iova, mem_size);
   352		}
   353	}
   354	

-- 
0-DAY CI Kernel Test Service
https://01.org/lkp

Re: [PATCH v4 1/7] dt-bindings: remoteproc: qcom: Add SC7280 ADSP support

[Rob Herring]

On Fri, Aug 12, 2022 at 06:17:40PM +0530, Srinivasa Rao Mandadapu wrote:
> Add ADSP PIL loading support for SC7280 SoCs.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> ---
>  .../bindings/remoteproc/qcom,sc7280-adsp-pil.yaml  | 189 +++++++++++++++++++++
>  1 file changed, 189 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
> 
> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
> new file mode 100644
> index 0000000..e656cc8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
> @@ -0,0 +1,189 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-adsp-pil.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Qualcomm SC7280 ADSP Peripheral Image Loader
> +
> +maintainers:
> +  - Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> +
> +description:
> +  This document defines the binding for a component that loads and boots firmware
> +  on the Qualcomm Technology Inc. ADSP.
> +
> +properties:
> +  compatible:
> +    enum:
> +      - qcom,sc7280-adsp-pil
> +
> +  reg:
> +    minItems: 1
> +    items:
> +      - description: qdsp6ss register
> +      - description: efuse q6ss register
> +
> +  interrupts:
> +    items:
> +      - description: Watchdog interrupt
> +      - description: Fatal interrupt
> +      - description: Ready interrupt
> +      - description: Handover interrupt
> +      - description: Stop acknowledge interrupt
> +      - description: Shutdown acknowledge interrupt
> +
> +  interrupt-names:
> +    items:
> +      - const: wdog
> +      - const: fatal
> +      - const: ready
> +      - const: handover
> +      - const: stop-ack
> +      - const: shutdown-ack
> +
> +  clocks:
> +    items:
> +      - description: XO clock
> +      - description: GCC CFG NOC LPASS clock
> +      - description: LPASS AHBS AON clock
> +      - description: LPASS AHBM AON clock
> +      - description: QDSP XO clock
> +      - description: Q6SP6SS SLEEP clock
> +      - description: Q6SP6SS CORE clock
> +
> +  clock-names:
> +    items:
> +      - const: xo
> +      - const: gcc_cfg_noc_lpass
> +      - const: lpass_ahbs_aon_cbcr
> +      - const: lpass_ahbm_aon_cbcr
> +      - const: qdsp6ss_xo
> +      - const: qdsp6ss_sleep
> +      - const: qdsp6ss_core
> +
> +  power-domains:
> +    items:
> +      - description: LCX power domain
> +
> +  resets:
> +    items:
> +      - description: PDC AUDIO SYNC RESET
> +      - description: CC LPASS restart
> +
> +  reset-names:
> +    items:
> +      - const: pdc_sync
> +      - const: cc_lpass
> +
> +  memory-region:
> +    maxItems: 1
> +    description: Reference to the reserved-memory for the Hexagon core
> +
> +  qcom,adsp-memory-regions:
> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    description:
> +      Each entry consists of 4 integers and represents the
> +      list of memory regions accessed by ADSP firmware.
> +    items:
> +      items:
> +        - description: |
> +            "iova reg" indicates the address of virtual memory region.
> +        - description: |
> +            "physical reg" indicates the address of phyical memory region.
> +        - description: |
> +            "size" indicates the offset memory region.
> +        - description: |
> +            "access level" indicates the read, read and write access levels.
> +          minimum: 0
> +          maximum: 1
> +
> +  qcom,halt-regs:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description:
> +      Phandle reference to a syscon representing TCSR followed by the
> +      three offsets within syscon for q6, modem and nc halt registers.

items: 
  - items:
      - description: phandle to TCSR
      - description: offset to q6 halt registers
      - ...

> +
> +  qcom,smem-states:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array
> +    description: States used by the AP to signal the Hexagon core
> +    items:
> +      - description: Stop the modem
> +
> +  qcom,smem-state-names:
> +    $ref: /schemas/types.yaml#/definitions/string
> +    description: The names of the state bits used for SMP2P output
> +    items:
> +      - const: stop
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - interrupt-names
> +  - clocks
> +  - clock-names
> +  - power-domains
> +  - resets
> +  - reset-names
> +  - qcom,halt-regs
> +  - memory-region
> +  - qcom,smem-states
> +  - qcom,smem-state-names
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
> +    #include <dt-bindings/clock/qcom,lpass-sc7280.h>
> +    #include <dt-bindings/reset/qcom,sdm845-aoss.h>
> +    #include <dt-bindings/reset/qcom,sdm845-pdc.h>
> +    #include <dt-bindings/power/qcom-rpmpd.h>
> +
> +    remoteproc@3000000 {
> +        compatible = "qcom,sc7280-adsp-pil";
> +        reg = <0x03000000 0x5000>,
> +              <0x355B000 0x10>;
> +
> +        interrupts-extended = <&pdc 162 IRQ_TYPE_EDGE_RISING>,
> +                <&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
> +                <&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
> +                <&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
> +                <&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
> +                <&adsp_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
> +
> +        interrupt-names = "wdog", "fatal", "ready",
> +                "handover", "stop-ack", "shutdown-ack";
> +
> +        clocks = <&rpmhcc RPMH_CXO_CLK>,
> +                 <&gcc GCC_CFG_NOC_LPASS_CLK>,
> +                 <&lpasscc LPASS_Q6SS_AHBM_CLK>,
> +                 <&lpasscc LPASS_Q6SS_AHBS_CLK>,
> +                 <&lpasscc LPASS_QDSP6SS_XO_CLK>,
> +                 <&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
> +                 <&lpasscc LPASS_QDSP6SS_CORE_CLK>;
> +        clock-names = "xo", "gcc_cfg_noc_lpass",
> +                "lpass_ahbs_aon_cbcr",
> +                "lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
> +                "qdsp6ss_sleep", "qdsp6ss_core";
> +
> +        power-domains = <&rpmhpd SC7280_LCX>;
> +
> +        resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
> +                 <&aoss_reset AOSS_CC_LPASS_RESTART>;
> +        reset-names = "pdc_sync", "cc_lpass";
> +
> +        qcom,halt-regs = <&tcsr_mutex 0x23000 0x25000 0x28000 0x33000>;
> +
> +        memory-region = <&adsp_mem>;
> +
> +        qcom,smem-states = <&adsp_smp2p_out 0>;
> +        qcom,smem-state-names = "stop";
> +
> +        qcom,adsp-memory-regions = <0x00100000 0x00100000 0x4000 0>,
> +                                   <0x00113000 0x00113000 0x1000 0>,
> +                                   <0x00117000 0x00117000 0x2000 1>;
> +    };
> -- 
> 2.7.4
> 
> 

Re: [PATCH v4 6/7] remoteproc: qcom: Add support for memory sandbox

[Christophe JAILLET]

Le 12/08/2022 à 14:47, Srinivasa Rao Mandadapu a écrit :
> Update pil driver with SMMU mapping for allowing authorised
> memory access to ADSP firmware, by reading required memory
> regions either from device tree file or from resource table
> embedded in ADSP binary header.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
> ---
> Changes since V3:
> 	-- Rename is_adsp_sb_needed to adsp_sandbox_needed.
> 	-- Add smmu unmapping in error case and in adsp stop.
> Changes since V2:
> 	-- Replace platform_bus_type with adsp->dev->bus.
> 	-- Use API of_parse_phandle_with_args() instead of of_parse_phandle_with_fixed_args().
> 	-- Replace adsp->is_wpss with adsp->is_adsp.
> 	-- Update error handling in adsp_start().
> 
>   drivers/remoteproc/qcom_q6v5_adsp.c | 172 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 170 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index b0a63a0..ca45d2c 100644
> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
> @@ -9,6 +9,7 @@
>   #include <linux/firmware.h>
>   #include <linux/interrupt.h>
>   #include <linux/io.h>
> +#include <linux/iommu.h>
>   #include <linux/iopoll.h>
>   #include <linux/kernel.h>
>   #include <linux/mfd/syscon.h>
> @@ -48,6 +49,8 @@
>   #define LPASS_PWR_ON_REG		0x10
>   #define LPASS_HALTREQ_REG		0x0
>   
> +#define SID_MASK_DEFAULT        0xF
> +
>   #define QDSP6SS_XO_CBCR		0x38
>   #define QDSP6SS_CORE_CBCR	0x20
>   #define QDSP6SS_SLEEP_CBCR	0x3c
> @@ -78,7 +81,7 @@ struct adsp_pil_data {
>   struct qcom_adsp {
>   	struct device *dev;
>   	struct rproc *rproc;
> -
> +	struct iommu_domain *iommu_dom;
>   	struct qcom_q6v5 q6v5;
>   
>   	struct clk *xo;
> @@ -333,6 +336,155 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>   	return 0;
>   }
>   
> +static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, const __be32 *prop, int len)
> +{
> +	unsigned long mem_phys;
> +	unsigned long iova;
> +	unsigned int mem_size;
> +	int access_level;
> +	int i;
> +
> +	for (i = 0; i < len; i++) {
> +		iova = be32_to_cpu(prop[i++]);
> +		mem_phys = be32_to_cpu(prop[i++]);
> +		mem_size = be32_to_cpu(prop[i++]);
> +		access_level = be32_to_cpu(prop[i]);
> +		iommu_unmap(iommu_dom, iova, mem_size);
> +	}
> +}
> +
> +static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
> +{
> +	struct fw_rsc_devmem *rsc_fw;
> +	struct fw_rsc_hdr *hdr;
> +	int offset;
> +	int i;
> +
> +	for (i = 0; i < len; i++) {
> +		offset = rproc->table_ptr->offset[i];
> +		hdr = (void *)rproc->table_ptr + offset;
> +		rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
> +
> +		iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
> +	}
> +}
> +
> +static void adsp_unmap_smmu(struct rproc *rproc)
> +{

When I proposed a adsp_unmap_smmu() function, the idea was to undo 
everything that is donne by adsp_map_smmu().
iommu_domain_alloc() and iommu_map(adsp->iommu_dom, ..) are not undone here.

If this make sense, it would improve the semantic, simplify the 
'adsp_smmu_unmap' label in adsp_start() and avoid what looks like a leak 
to me in adsp_stop().


> +	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> +	const __be32 *prop;
> +	unsigned int len;
> +
> +	prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
> +	if (prop) {

In the allocation path, you have a "len /= sizeof(__be32);" which is not 
here. Is it needed?

You call adsp_unmap_smmu() from the error handling path of 
adsp_map_smmu(). If needed, maybe it should be part of adsp_of_unmap_smmu()?

> +		adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
> +	} else {
> +		if (rproc->table_ptr)
> +			adsp_rproc_unmap_smmu(rproc, rproc->table_ptr->num);
> +	}
> +}
> +
> +static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
> +{
> +	struct of_phandle_args args;
> +	struct fw_rsc_devmem *rsc_fw;
> +	struct fw_rsc_hdr *hdr;
> +	const __be32 *prop;
> +	long long sid;
> +	unsigned long mem_phys;
> +	unsigned long iova;
> +	unsigned int mem_size;
> +	unsigned int flag;
> +	unsigned int len;
> +	int access_level;
> +	int offset;
> +	int ret;
> +	int rc;

Are ret and rc both needed?

> +	int i;
> +
> +	rc = of_parse_phandle_with_args(adsp->dev->of_node, "iommus", "#iommu-cells", 0, &args);
> +	if (rc < 0)
> +		sid = -1;
> +	else
> +		sid = args.args[0] & SID_MASK_DEFAULT;
> +
> +	adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
> +	if (!adsp->iommu_dom) {
> +		dev_err(adsp->dev, "failed to allocate iommu domain\n");
> +		ret = -ENOMEM;
> +		goto domain_free;
> +	}
> +
> +	ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
> +	if (ret) {
> +		dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
> +		ret = -EBUSY;
> +		goto detach_device;
> +	}
> +
> +	/* Add SID configuration for ADSP Firmware to SMMU */
> +	adsp->mem_phys =  adsp->mem_phys | (sid << 32);
> +
> +	ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, adsp->mem_phys,
> +			adsp->mem_size,	IOMMU_READ | IOMMU_WRITE);
> +	if (ret) {
> +		dev_err(adsp->dev, "Unable to map ADSP Physical Memory\n");
> +		goto sid_unmap;
> +	}
> +
> +	prop = of_get_property(adsp->dev->of_node, "qcom,adsp-memory-regions", &len);
> +	if (prop) {
> +		len /= sizeof(__be32);
> +		for (i = 0; i < len; i++) {
> +			iova = be32_to_cpu(prop[i++]);
> +			mem_phys = be32_to_cpu(prop[i++]);
> +			mem_size = be32_to_cpu(prop[i++]);
> +			access_level = be32_to_cpu(prop[i]);
> +
> +			if (access_level)
> +				flag = IOMMU_READ | IOMMU_WRITE;
> +			else
> +				flag = IOMMU_READ;
> +
> +			ret = iommu_map(adsp->iommu_dom, iova, mem_phys, mem_size, flag);
> +			if (ret) {
> +				dev_err(adsp->dev, "failed to map addr = %p mem_size = %x\n",
> +						&(mem_phys), mem_size);
> +				goto smmu_unmap;
> +			}
> +		}
> +	} else {
> +		if (!rproc->table_ptr)
> +			goto sid_unmap;
> +
> +		for (i = 0; i < rproc->table_ptr->num; i++) {
> +			offset = rproc->table_ptr->offset[i];
> +			hdr = (void *)rproc->table_ptr + offset;
> +			rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
> +
> +			ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
> +						rsc_fw->len, rsc_fw->flags);
> +			if (ret) {
> +				pr_err("%s; unable to map adsp memory address\n", __func__);
> +				goto rproc_smmu_unmap;
> +			}
> +		}
> +	}

If you introduce a adsp_of_unmap_smmu() and adsp_rproc_unmap_smmu(), 
would it make things more readable to have the same kind of functions 
when allocating the resources?

Symmetry often helps.

> +	return 0;

Add an empty new line here?

> +rproc_smmu_unmap:
> +	adsp_rproc_unmap_smmu(rproc, i);
> +	goto sid_unmap;
> +smmu_unmap:
> +	adsp_of_unmap_smmu(adsp->iommu_dom, prop, i);
> +sid_unmap:
> +	iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
> +detach_device:
> +	iommu_domain_free(adsp->iommu_dom);
> +domain_free:
> +	return ret;
> +}
> +
> +
>   static int adsp_start(struct rproc *rproc)
>   {
>   	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> @@ -343,9 +495,16 @@ static int adsp_start(struct rproc *rproc)
>   	if (ret)
>   		return ret;
>   
> +	if (adsp->adsp_sandbox_needed) {
> +		ret = adsp_map_smmu(adsp, rproc);
> +		if (ret) {
> +			dev_err(adsp->dev, "ADSP smmu mapping failed\n");
> +			goto disable_irqs;
> +		}
> +	}
>   	ret = clk_prepare_enable(adsp->xo);
>   	if (ret)
> -		goto disable_irqs;
> +		goto adsp_smmu_unmap;
>   
>   	ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
>   				    adsp->proxy_pd_count);
> @@ -401,6 +560,12 @@ static int adsp_start(struct rproc *rproc)
>   	qcom_rproc_pds_disable(adsp, adsp->proxy_pds, adsp->proxy_pd_count);
>   disable_xo_clk:
>   	clk_disable_unprepare(adsp->xo);
> +adsp_smmu_unmap:
> +	if (adsp->adsp_sandbox_needed) {
> +		iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
> +		adsp_unmap_smmu(rproc);
> +		iommu_domain_free(adsp->iommu_dom);
> +	}
>   disable_irqs:
>   	qcom_q6v5_unprepare(&adsp->q6v5);
>   
> @@ -429,6 +594,9 @@ static int adsp_stop(struct rproc *rproc)
>   	if (ret)
>   		dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
>   
> +	if (adsp->adsp_sandbox_needed)
> +		adsp_unmap_smmu(rproc);

No need to call iommu_unmap() and iommu_domain_free() here?
(this is the same comment as the one in adsp_rproc_unmap_smmu(). This is 
just a blind guess based on symmetry of the code.)

> +
>   	handover = qcom_q6v5_unprepare(&adsp->q6v5);
>   	if (handover)
>   		qcom_adsp_pil_handover(&adsp->q6v5);

Re: [PATCH v4 1/7] dt-bindings: remoteproc: qcom: Add SC7280 ADSP support

[Srinivasa Rao Mandadapu]

On 8/15/2022 2:05 AM, Rob Herring wrote:
Thanks for Your time Rob!!!
> On Fri, Aug 12, 2022 at 06:17:40PM +0530, Srinivasa Rao Mandadapu wrote:
>> Add ADSP PIL loading support for SC7280 SoCs.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> ---
>>   .../bindings/remoteproc/qcom,sc7280-adsp-pil.yaml  | 189 +++++++++++++++++++++
>>   1 file changed, 189 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
>> new file mode 100644
>> index 0000000..e656cc8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/remoteproc/qcom,sc7280-adsp-pil.yaml
>> @@ -0,0 +1,189 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/remoteproc/qcom,sc7280-adsp-pil.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Qualcomm SC7280 ADSP Peripheral Image Loader
>> +
>> +maintainers:
>> +  - Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> +
>> +description:
>> +  This document defines the binding for a component that loads and boots firmware
>> +  on the Qualcomm Technology Inc. ADSP.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sc7280-adsp-pil
>> +
>> +  reg:
>> +    minItems: 1
>> +    items:
>> +      - description: qdsp6ss register
>> +      - description: efuse q6ss register
>> +
>> +  interrupts:
>> +    items:
>> +      - description: Watchdog interrupt
>> +      - description: Fatal interrupt
>> +      - description: Ready interrupt
>> +      - description: Handover interrupt
>> +      - description: Stop acknowledge interrupt
>> +      - description: Shutdown acknowledge interrupt
>> +
>> +  interrupt-names:
>> +    items:
>> +      - const: wdog
>> +      - const: fatal
>> +      - const: ready
>> +      - const: handover
>> +      - const: stop-ack
>> +      - const: shutdown-ack
>> +
>> +  clocks:
>> +    items:
>> +      - description: XO clock
>> +      - description: GCC CFG NOC LPASS clock
>> +      - description: LPASS AHBS AON clock
>> +      - description: LPASS AHBM AON clock
>> +      - description: QDSP XO clock
>> +      - description: Q6SP6SS SLEEP clock
>> +      - description: Q6SP6SS CORE clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: xo
>> +      - const: gcc_cfg_noc_lpass
>> +      - const: lpass_ahbs_aon_cbcr
>> +      - const: lpass_ahbm_aon_cbcr
>> +      - const: qdsp6ss_xo
>> +      - const: qdsp6ss_sleep
>> +      - const: qdsp6ss_core
>> +
>> +  power-domains:
>> +    items:
>> +      - description: LCX power domain
>> +
>> +  resets:
>> +    items:
>> +      - description: PDC AUDIO SYNC RESET
>> +      - description: CC LPASS restart
>> +
>> +  reset-names:
>> +    items:
>> +      - const: pdc_sync
>> +      - const: cc_lpass
>> +
>> +  memory-region:
>> +    maxItems: 1
>> +    description: Reference to the reserved-memory for the Hexagon core
>> +
>> +  qcom,adsp-memory-regions:
>> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
>> +    description:
>> +      Each entry consists of 4 integers and represents the
>> +      list of memory regions accessed by ADSP firmware.
>> +    items:
>> +      items:
>> +        - description: |
>> +            "iova reg" indicates the address of virtual memory region.
>> +        - description: |
>> +            "physical reg" indicates the address of phyical memory region.
>> +        - description: |
>> +            "size" indicates the offset memory region.
>> +        - description: |
>> +            "access level" indicates the read, read and write access levels.
>> +          minimum: 0
>> +          maximum: 1
>> +
>> +  qcom,halt-regs:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description:
>> +      Phandle reference to a syscon representing TCSR followed by the
>> +      three offsets within syscon for q6, modem and nc halt registers.
> items:
>    - items:
>        - description: phandle to TCSR
>        - description: offset to q6 halt registers
>        - ...
Okay. Will update accordingly and re post it.
>
>> +
>> +  qcom,smem-states:
>> +    $ref: /schemas/types.yaml#/definitions/phandle-array
>> +    description: States used by the AP to signal the Hexagon core
>> +    items:
>> +      - description: Stop the modem
>> +
>> +  qcom,smem-state-names:
>> +    $ref: /schemas/types.yaml#/definitions/string
>> +    description: The names of the state bits used for SMP2P output
>> +    items:
>> +      - const: stop
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - interrupt-names
>> +  - clocks
>> +  - clock-names
>> +  - power-domains
>> +  - resets
>> +  - reset-names
>> +  - qcom,halt-regs
>> +  - memory-region
>> +  - qcom,smem-states
>> +  - qcom,smem-state-names
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,rpmh.h>
>> +    #include <dt-bindings/clock/qcom,gcc-sc7280.h>
>> +    #include <dt-bindings/clock/qcom,lpass-sc7280.h>
>> +    #include <dt-bindings/reset/qcom,sdm845-aoss.h>
>> +    #include <dt-bindings/reset/qcom,sdm845-pdc.h>
>> +    #include <dt-bindings/power/qcom-rpmpd.h>
>> +
>> +    remoteproc@3000000 {
>> +        compatible = "qcom,sc7280-adsp-pil";
>> +        reg = <0x03000000 0x5000>,
>> +              <0x355B000 0x10>;
>> +
>> +        interrupts-extended = <&pdc 162 IRQ_TYPE_EDGE_RISING>,
>> +                <&adsp_smp2p_in 0 IRQ_TYPE_EDGE_RISING>,
>> +                <&adsp_smp2p_in 1 IRQ_TYPE_EDGE_RISING>,
>> +                <&adsp_smp2p_in 2 IRQ_TYPE_EDGE_RISING>,
>> +                <&adsp_smp2p_in 3 IRQ_TYPE_EDGE_RISING>,
>> +                <&adsp_smp2p_in 7 IRQ_TYPE_EDGE_RISING>;
>> +
>> +        interrupt-names = "wdog", "fatal", "ready",
>> +                "handover", "stop-ack", "shutdown-ack";
>> +
>> +        clocks = <&rpmhcc RPMH_CXO_CLK>,
>> +                 <&gcc GCC_CFG_NOC_LPASS_CLK>,
>> +                 <&lpasscc LPASS_Q6SS_AHBM_CLK>,
>> +                 <&lpasscc LPASS_Q6SS_AHBS_CLK>,
>> +                 <&lpasscc LPASS_QDSP6SS_XO_CLK>,
>> +                 <&lpasscc LPASS_QDSP6SS_SLEEP_CLK>,
>> +                 <&lpasscc LPASS_QDSP6SS_CORE_CLK>;
>> +        clock-names = "xo", "gcc_cfg_noc_lpass",
>> +                "lpass_ahbs_aon_cbcr",
>> +                "lpass_ahbm_aon_cbcr", "qdsp6ss_xo",
>> +                "qdsp6ss_sleep", "qdsp6ss_core";
>> +
>> +        power-domains = <&rpmhpd SC7280_LCX>;
>> +
>> +        resets = <&pdc_reset PDC_AUDIO_SYNC_RESET>,
>> +                 <&aoss_reset AOSS_CC_LPASS_RESTART>;
>> +        reset-names = "pdc_sync", "cc_lpass";
>> +
>> +        qcom,halt-regs = <&tcsr_mutex 0x23000 0x25000 0x28000 0x33000>;
>> +
>> +        memory-region = <&adsp_mem>;
>> +
>> +        qcom,smem-states = <&adsp_smp2p_out 0>;
>> +        qcom,smem-state-names = "stop";
>> +
>> +        qcom,adsp-memory-regions = <0x00100000 0x00100000 0x4000 0>,
>> +                                   <0x00113000 0x00113000 0x1000 0>,
>> +                                   <0x00117000 0x00117000 0x2000 1>;
>> +    };
>> -- 
>> 2.7.4
>>
>>

Re: [PATCH v4 6/7] remoteproc: qcom: Add support for memory sandbox

[Srinivasa Rao Mandadapu]

On 8/15/2022 12:07 PM, Christophe JAILLET wrote:
Thanks for Your time and valuable insights CJ !!!
> Le 12/08/2022 à 14:47, Srinivasa Rao Mandadapu a écrit :
>> Update pil driver with SMMU mapping for allowing authorised
>> memory access to ADSP firmware, by reading required memory
>> regions either from device tree file or from resource table
>> embedded in ADSP binary header.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu 
>> <quic_srivasam-jfJNa2p1gH1BDgjK7y7TUQ@public.gmane.org>
>> ---
>> Changes since V3:
>>     -- Rename is_adsp_sb_needed to adsp_sandbox_needed.
>>     -- Add smmu unmapping in error case and in adsp stop.
>> Changes since V2:
>>     -- Replace platform_bus_type with adsp->dev->bus.
>>     -- Use API of_parse_phandle_with_args() instead of 
>> of_parse_phandle_with_fixed_args().
>>     -- Replace adsp->is_wpss with adsp->is_adsp.
>>     -- Update error handling in adsp_start().
>>
>>   drivers/remoteproc/qcom_q6v5_adsp.c | 172 
>> +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 170 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
>> b/drivers/remoteproc/qcom_q6v5_adsp.c
>> index b0a63a0..ca45d2c 100644
>> --- a/drivers/remoteproc/qcom_q6v5_adsp.c
>> +++ b/drivers/remoteproc/qcom_q6v5_adsp.c
>> @@ -9,6 +9,7 @@
>>   #include <linux/firmware.h>
>>   #include <linux/interrupt.h>
>>   #include <linux/io.h>
>> +#include <linux/iommu.h>
>>   #include <linux/iopoll.h>
>>   #include <linux/kernel.h>
>>   #include <linux/mfd/syscon.h>
>> @@ -48,6 +49,8 @@
>>   #define LPASS_PWR_ON_REG        0x10
>>   #define LPASS_HALTREQ_REG        0x0
>>   +#define SID_MASK_DEFAULT        0xF
>> +
>>   #define QDSP6SS_XO_CBCR        0x38
>>   #define QDSP6SS_CORE_CBCR    0x20
>>   #define QDSP6SS_SLEEP_CBCR    0x3c
>> @@ -78,7 +81,7 @@ struct adsp_pil_data {
>>   struct qcom_adsp {
>>       struct device *dev;
>>       struct rproc *rproc;
>> -
>> +    struct iommu_domain *iommu_dom;
>>       struct qcom_q6v5 q6v5;
>>         struct clk *xo;
>> @@ -333,6 +336,155 @@ static int adsp_load(struct rproc *rproc, const 
>> struct firmware *fw)
>>       return 0;
>>   }
>>   +static void adsp_of_unmap_smmu(struct iommu_domain *iommu_dom, 
>> const __be32 *prop, int len)
>> +{
>> +    unsigned long mem_phys;
>> +    unsigned long iova;
>> +    unsigned int mem_size;
>> +    int access_level;
>> +    int i;
>> +
>> +    for (i = 0; i < len; i++) {
>> +        iova = be32_to_cpu(prop[i++]);
>> +        mem_phys = be32_to_cpu(prop[i++]);
>> +        mem_size = be32_to_cpu(prop[i++]);
>> +        access_level = be32_to_cpu(prop[i]);
>> +        iommu_unmap(iommu_dom, iova, mem_size);
>> +    }
>> +}
>> +
>> +static void adsp_rproc_unmap_smmu(struct rproc *rproc, int len)
>> +{
>> +    struct fw_rsc_devmem *rsc_fw;
>> +    struct fw_rsc_hdr *hdr;
>> +    int offset;
>> +    int i;
>> +
>> +    for (i = 0; i < len; i++) {
>> +        offset = rproc->table_ptr->offset[i];
>> +        hdr = (void *)rproc->table_ptr + offset;
>> +        rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
>> +
>> +        iommu_unmap(rproc->domain, rsc_fw->da, rsc_fw->len);
>> +    }
>> +}
>> +
>> +static void adsp_unmap_smmu(struct rproc *rproc)
>> +{
>
> When I proposed a adsp_unmap_smmu() function, the idea was to undo 
> everything that is donne by adsp_map_smmu().
> iommu_domain_alloc() and iommu_map(adsp->iommu_dom, ..) are not undone 
> here.
>
> If this make sense, it would improve the semantic, simplify the 
> 'adsp_smmu_unmap' label in adsp_start() and avoid what looks like a 
> leak to me in adsp_stop().
Okay. Will modify accordingly and re post it.
>
>
>> +    struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> +    const __be32 *prop;
>> +    unsigned int len;
>> +
>> +    prop = of_get_property(adsp->dev->of_node, 
>> "qcom,adsp-memory-regions", &len);
>> +    if (prop) {
>
> In the allocation path, you have a "len /= sizeof(__be32);" which is 
> not here. Is it needed?
Yes It's missing. will ad it.
>
> You call adsp_unmap_smmu() from the error handling path of 
> adsp_map_smmu(). If needed, maybe it should be part of 
> adsp_of_unmap_smmu()?
Okay. Will modify accordingly and re post it.
>
>> + adsp_of_unmap_smmu(adsp->iommu_dom, prop, len);
>> +    } else {
>> +        if (rproc->table_ptr)
>> +            adsp_rproc_unmap_smmu(rproc, rproc->table_ptr->num);
>> +    }
>> +}
>> +
>> +static int adsp_map_smmu(struct qcom_adsp *adsp, struct rproc *rproc)
>> +{
>> +    struct of_phandle_args args;
>> +    struct fw_rsc_devmem *rsc_fw;
>> +    struct fw_rsc_hdr *hdr;
>> +    const __be32 *prop;
>> +    long long sid;
>> +    unsigned long mem_phys;
>> +    unsigned long iova;
>> +    unsigned int mem_size;
>> +    unsigned int flag;
>> +    unsigned int len;
>> +    int access_level;
>> +    int offset;
>> +    int ret;
>> +    int rc;
>
> Are ret and rc both needed?
Yes it's redundant. Will replace it with ret.
>
>> +    int i;
>> +
>> +    rc = of_parse_phandle_with_args(adsp->dev->of_node, "iommus", 
>> "#iommu-cells", 0, &args);
>> +    if (rc < 0)
>> +        sid = -1;
>> +    else
>> +        sid = args.args[0] & SID_MASK_DEFAULT;
>> +
>> +    adsp->iommu_dom = iommu_domain_alloc(adsp->dev->bus);
>> +    if (!adsp->iommu_dom) {
>> +        dev_err(adsp->dev, "failed to allocate iommu domain\n");
>> +        ret = -ENOMEM;
>> +        goto domain_free;
>> +    }
>> +
>> +    ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
>> +    if (ret) {
>> +        dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
>> +        ret = -EBUSY;
>> +        goto detach_device;
>> +    }
>> +
>> +    /* Add SID configuration for ADSP Firmware to SMMU */
>> +    adsp->mem_phys =  adsp->mem_phys | (sid << 32);
>> +
>> +    ret = iommu_map(adsp->iommu_dom, adsp->mem_phys, adsp->mem_phys,
>> +            adsp->mem_size,    IOMMU_READ | IOMMU_WRITE);
>> +    if (ret) {
>> +        dev_err(adsp->dev, "Unable to map ADSP Physical Memory\n");
>> +        goto sid_unmap;
>> +    }
>> +
>> +    prop = of_get_property(adsp->dev->of_node, 
>> "qcom,adsp-memory-regions", &len);
>> +    if (prop) {
>> +        len /= sizeof(__be32);
>> +        for (i = 0; i < len; i++) {
>> +            iova = be32_to_cpu(prop[i++]);
>> +            mem_phys = be32_to_cpu(prop[i++]);
>> +            mem_size = be32_to_cpu(prop[i++]);
>> +            access_level = be32_to_cpu(prop[i]);
>> +
>> +            if (access_level)
>> +                flag = IOMMU_READ | IOMMU_WRITE;
>> +            else
>> +                flag = IOMMU_READ;
>> +
>> +            ret = iommu_map(adsp->iommu_dom, iova, mem_phys, 
>> mem_size, flag);
>> +            if (ret) {
>> +                dev_err(adsp->dev, "failed to map addr = %p mem_size 
>> = %x\n",
>> +                        &(mem_phys), mem_size);
>> +                goto smmu_unmap;
>> +            }
>> +        }
>> +    } else {
>> +        if (!rproc->table_ptr)
>> +            goto sid_unmap;
>> +
>> +        for (i = 0; i < rproc->table_ptr->num; i++) {
>> +            offset = rproc->table_ptr->offset[i];
>> +            hdr = (void *)rproc->table_ptr + offset;
>> +            rsc_fw = (struct fw_rsc_devmem *)hdr + sizeof(*hdr);
>> +
>> +            ret = iommu_map(rproc->domain, rsc_fw->da, rsc_fw->pa,
>> +                        rsc_fw->len, rsc_fw->flags);
>> +            if (ret) {
>> +                pr_err("%s; unable to map adsp memory address\n", 
>> __func__);
>> +                goto rproc_smmu_unmap;
>> +            }
>> +        }
>> +    }
>
> If you introduce a adsp_of_unmap_smmu() and adsp_rproc_unmap_smmu(), 
> would it make things more readable to have the same kind of functions 
> when allocating the resources?
>
> Symmetry often helps.
Yes, Agree.  Will update accordingly.
>
>> +    return 0;
>
> Add an empty new line here?
>
>> +rproc_smmu_unmap:
>> +    adsp_rproc_unmap_smmu(rproc, i);
>> +    goto sid_unmap;
>> +smmu_unmap:
>> +    adsp_of_unmap_smmu(adsp->iommu_dom, prop, i);
>> +sid_unmap:
>> +    iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>> +detach_device:
>> +    iommu_domain_free(adsp->iommu_dom);
>> +domain_free:
>> +    return ret;
>> +}
>> +
>> +
>>   static int adsp_start(struct rproc *rproc)
>>   {
>>       struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> @@ -343,9 +495,16 @@ static int adsp_start(struct rproc *rproc)
>>       if (ret)
>>           return ret;
>>   +    if (adsp->adsp_sandbox_needed) {
>> +        ret = adsp_map_smmu(adsp, rproc);
>> +        if (ret) {
>> +            dev_err(adsp->dev, "ADSP smmu mapping failed\n");
>> +            goto disable_irqs;
>> +        }
>> +    }
>>       ret = clk_prepare_enable(adsp->xo);
>>       if (ret)
>> -        goto disable_irqs;
>> +        goto adsp_smmu_unmap;
>>         ret = qcom_rproc_pds_enable(adsp, adsp->proxy_pds,
>>                       adsp->proxy_pd_count);
>> @@ -401,6 +560,12 @@ static int adsp_start(struct rproc *rproc)
>>       qcom_rproc_pds_disable(adsp, adsp->proxy_pds, 
>> adsp->proxy_pd_count);
>>   disable_xo_clk:
>>       clk_disable_unprepare(adsp->xo);
>> +adsp_smmu_unmap:
>> +    if (adsp->adsp_sandbox_needed) {
>> +        iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>> +        adsp_unmap_smmu(rproc);
>> +        iommu_domain_free(adsp->iommu_dom);
>> +    }
>>   disable_irqs:
>>       qcom_q6v5_unprepare(&adsp->q6v5);
>>   @@ -429,6 +594,9 @@ static int adsp_stop(struct rproc *rproc)
>>       if (ret)
>>           dev_err(adsp->dev, "failed to shutdown: %d\n", ret);
>>   +    if (adsp->adsp_sandbox_needed)
>> +        adsp_unmap_smmu(rproc);
>
> No need to call iommu_unmap() and iommu_domain_free() here?
> (this is the same comment as the one in adsp_rproc_unmap_smmu(). This 
> is just a blind guess based on symmetry of the code.)
Okay.
>
>> +
>>       handover = qcom_q6v5_unprepare(&adsp->q6v5);
>>       if (handover)
>>           qcom_adsp_pil_handover(&adsp->q6v5);
>