[PATCH v3 0/8] Update ADSP pil loader for SC7280 platform

[PATCH v3 0/8] Update ADSP pil loader for SC7280 platform

[Srinivasa Rao Mandadapu]

Update ADSP pil loader driver for SC7280 platforms.

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 (8):
  dt-bindings: remoteproc: qcom: adsp: Make ADSP pil loader as generic
  dt-bindings: remoteproc: qcom: adsp: Add required bindings for SC7280
  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

 ...m845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml} |  19 ++-
 drivers/remoteproc/qcom_q6v5_adsp.c                | 150 ++++++++++++++++++++-
 2 files changed, 158 insertions(+), 11 deletions(-)
 rename Documentation/devicetree/bindings/remoteproc/{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml} (90%)

-- 
2.7.4

[PATCH v3 1/8] dt-bindings: remoteproc: qcom: adsp: Make ADSP pil loader as generic

[Srinivasa Rao Mandadapu]

Rename sdm845 adsp pil bindings to generic name, for using same binings
file for subsequent SoCs.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
---
Changes since V2:
	-- Generate patch with -M flag.

 .../{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml}           | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
 rename Documentation/devicetree/bindings/remoteproc/{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml} (97%)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,sdm845-adsp-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml
similarity index 97%
rename from Documentation/devicetree/bindings/remoteproc/qcom,sdm845-adsp-pil.yaml
rename to Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml
index 1535bbb..9f11332 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,sdm845-adsp-pil.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml
@@ -1,10 +1,10 @@
 # SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
 %YAML 1.2
 ---
-$id: http://devicetree.org/schemas/remoteproc/qcom,sdm845-adsp-pil.yaml#
+$id: http://devicetree.org/schemas/remoteproc/qcom,lpass-adsp-pil.yaml#
 $schema: http://devicetree.org/meta-schemas/core.yaml#
 
-title: Qualcomm SDM845 ADSP Peripheral Image Loader
+title: Qualcomm LPASS ADSP Peripheral Image Loader
 
 maintainers:
   - Bjorn Andersson <bjorn.andersson@linaro.org>
-- 
2.7.4

[PATCH v3 2/8] dt-bindings: remoteproc: qcom: adsp: Add required bindings for SC7280

[Srinivasa Rao Mandadapu]

Add compatible name, clocks and update max reg items for SC7280
based platforms.
Add adsp-memory-regions property, required for memory sandboxing.

Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
Reviewed-by: Rob Herring <robh@kernel.org>
---
Changes since V2:
	-- Add clock property.
	-- Add qcom,adsp-memory-regions property.
Changes since V1:
	-- Change reg property maxItems to minItems and update description.

 .../bindings/remoteproc/qcom,lpass-adsp-pil.yaml          | 15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml b/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml
index 9f11332..45bc732 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml
@@ -17,11 +17,13 @@ properties:
   compatible:
     enum:
       - qcom,sdm845-adsp-pil
+      - qcom,sc7280-adsp-pil
 
   reg:
-    maxItems: 1
-    description:
-      The base address and size of the qdsp6ss register
+    minItems: 1
+    items:
+      - description: qdsp6ss register
+      - description: efuse q6ss register
 
   interrupts:
     items:
@@ -48,6 +50,7 @@ properties:
       - description: QDSP XO clock
       - description: Q6SP6SS SLEEP clock
       - description: Q6SP6SS CORE clock
+      - description: GCC CFG NOC clock
 
   clock-names:
     items:
@@ -58,6 +61,7 @@ properties:
       - const: qdsp6ss_xo
       - const: qdsp6ss_sleep
       - const: qdsp6ss_core
+      - const: gcc_cfg_noc
 
   power-domains:
     items:
@@ -77,6 +81,11 @@ properties:
     maxItems: 1
     description: Reference to the reserved-memory for the Hexagon core
 
+  qcom,adsp-memory-regions:
+    minItems: 1
+    items:
+      - description: List of memory regions accessed by ADSP firmware.
+
   qcom,halt-regs:
     $ref: /schemas/types.yaml#/definitions/phandle-array
     description:
-- 
2.7.4

[PATCH v3 3/8] 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 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..d18ec74 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 is_adsp_sb_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 is_adsp_sb_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->is_adsp_sb_needed = desc->is_adsp_sb_needed;
+
 	platform_set_drvdata(pdev, adsp);
 
 	if (desc->is_wpss)
-- 
2.7.4

[PATCH v3 4/8] 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 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 d18ec74..15d9834 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,
+	.is_adsp_sb_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",
@@ -741,6 +757,7 @@ static const struct of_device_id adsp_of_match[] = {
 	{ .compatible = "qcom,qcs404-cdsp-pil", .data = &cdsp_resource_init },
 	{ .compatible = "qcom,sc7280-wpss-pil", .data = &wpss_resource_init },
 	{ .compatible = "qcom,sdm845-adsp-pil", .data = &adsp_resource_init },
+	{ .compatible = "qcom,sc7280-adsp-pil", .data = &adsp_sc7280_resource_init },
 	{ },
 };
 MODULE_DEVICE_TABLE(of, adsp_of_match);
-- 
2.7.4

[PATCH v3 5/8] 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 15d9834..141fd339 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 v3 6/8] 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 141fd339..f2945bf 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 v3 7/8] 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 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 | 107 +++++++++++++++++++++++++++++++++++-
 1 file changed, 105 insertions(+), 2 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
index f2945bf..b9cafe2 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,94 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
 	return 0;
 }
 
+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");
+		return -ENOMEM;
+	}
+
+	ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
+	if (ret) {
+		dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
+		return -EBUSY;
+	}
+
+	/* 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");
+		return ret;
+	}
+
+	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);
+				return ret;
+			}
+		}
+	} else {
+		if (!rproc->table_ptr)
+			return 0;
+
+		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__);
+				return ret;
+			}
+		}
+	}
+	return 0;
+}
+
+
 static int adsp_start(struct rproc *rproc)
 {
 	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
@@ -343,9 +434,16 @@ static int adsp_start(struct rproc *rproc)
 	if (ret)
 		return ret;
 
+	if (adsp->is_adsp_sb_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 +499,11 @@ 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->is_adsp_sb_needed) {
+		iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
+		iommu_domain_free(adsp->iommu_dom);
+	}
 disable_irqs:
 	qcom_q6v5_unprepare(&adsp->q6v5);
 
-- 
2.7.4

[PATCH v3 8/8] 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>
---
Changes since V1:
	-- Fix typo error.

 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 b9cafe2..5d22cf2 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*/
@@ -473,13 +473,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 v3 2/8] dt-bindings: remoteproc: qcom: adsp: Add required bindings for SC7280

[Rob Herring]

On Wed, 10 Aug 2022 13:15:52 +0530, Srinivasa Rao Mandadapu wrote:
> Add compatible name, clocks and update max reg items for SC7280
> based platforms.
> Add adsp-memory-regions property, required for memory sandboxing.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> Reviewed-by: Rob Herring <robh@kernel.org>
> ---
> Changes since V2:
> 	-- Add clock property.
> 	-- Add qcom,adsp-memory-regions property.
> Changes since V1:
> 	-- Change reg property maxItems to minItems and update description.
> 
>  .../bindings/remoteproc/qcom,lpass-adsp-pil.yaml          | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml: properties:qcom,adsp-memory-regions: 'oneOf' conditional failed, one must be fixed:
	[{'description': 'List of memory regions accessed by ADSP firmware.'}] is too short
	False schema does not allow 1
	hint: "minItems" is only needed if less than the "items" list length
	from schema $id: http://devicetree.org/meta-schemas/items.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml: properties:qcom,adsp-memory-regions: 'oneOf' conditional failed, one must be fixed:
	'type' is a required property
		hint: A vendor boolean property can use "type: boolean"
	'description' is a required property
		hint: A vendor boolean property can use "type: boolean"
	Additional properties are not allowed ('items', 'minItems' were unexpected)
		hint: A vendor boolean property can use "type: boolean"
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml: properties:qcom,adsp-memory-regions: 'oneOf' conditional failed, one must be fixed:
		'enum' is a required property
		'const' is a required property
		hint: A vendor string property with exact values has an implicit type
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml: properties:qcom,adsp-memory-regions: 'oneOf' conditional failed, one must be fixed:
		'$ref' is a required property
		'allOf' is a required property
		hint: A vendor property needs a $ref to types.yaml
		from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
	hint: Vendor specific properties must have a type and description unless they have a defined, common suffix.
	from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml#
/builds/robherring/linux-dt-review/Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.yaml: ignoring, error in schema: properties: qcom,adsp-memory-regions
Documentation/devicetree/bindings/remoteproc/qcom,lpass-adsp-pil.example.dtb:0:0: /example-0/remoteproc@17300000: failed to match any schema with compatible: ['qcom,sdm845-adsp-pil']

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v3 4/8] remoteproc: qcom: Add compatible name for SC7280 ADSP

[Stephen Boyd]

Quoting Srinivasa Rao Mandadapu (2022-08-10 00:45:54)
> @@ -741,6 +757,7 @@ static const struct of_device_id adsp_of_match[] = {
>         { .compatible = "qcom,qcs404-cdsp-pil", .data = &cdsp_resource_init },
>         { .compatible = "qcom,sc7280-wpss-pil", .data = &wpss_resource_init },
>         { .compatible = "qcom,sdm845-adsp-pil", .data = &adsp_resource_init },
> +       { .compatible = "qcom,sc7280-adsp-pil", .data = &adsp_sc7280_resource_init },

Please keep this sorted on compatible string.

Re: [PATCH v3 3/8] remoteproc: qcom: Add flag in adsp private data structure

[Stephen Boyd]

Quoting Srinivasa Rao Mandadapu (2022-08-10 00:45:53)
> 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 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..d18ec74 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 is_adsp_sb_needed;

What does 'sb' mean? Self boot? Can you just write it out? And maybe
drop 'is_' prefix because if (is_*) and if (something_needed) reads the
same.

Re: [PATCH v3 7/8] remoteproc: qcom: Add support for memory sandbox

[Christophe JAILLET]

Le 10/08/2022 à 09:45, 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.
> 

Hi,

comments below about error handling paths that look incomplete to me.

Just my 2c.

> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> ---
> 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 | 107 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 105 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c b/drivers/remoteproc/qcom_q6v5_adsp.c
> index f2945bf..b9cafe2 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,94 @@ static int adsp_load(struct rproc *rproc, const struct firmware *fw)
>   	return 0;
>   }
>   
> +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");
> +		return -ENOMEM;
> +	}
> +
> +	ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
> +	if (ret) {
> +		dev_err(adsp->dev, "could not attach device ret = %d\n", ret);

iommu_domain_free() or error handling path (see below)?

> +		return -EBUSY;
> +	}
> +
> +	/* 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");

iommu_domain_free() or error handling path (see below)?

> +		return ret;
> +	}
> +
> +	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);
> +				return ret;

Should there be an error handling path to undo iommu_domain_alloc() and 
iommu_map() above.
Same for iommu_map() already done in the loop.

> +			}
> +		}
> +	} else {
> +		if (!rproc->table_ptr)
> +			return 0;
> +
> +		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__);
> +				return ret;

Same comment.

> +			}
> +		}
> +	}
> +	return 0;
> +}
> +
> +
>   static int adsp_start(struct rproc *rproc)
>   {
>   	struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
> @@ -343,9 +434,16 @@ static int adsp_start(struct rproc *rproc)
>   	if (ret)
>   		return ret;
>   
> +	if (adsp->is_adsp_sb_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 +499,11 @@ 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->is_adsp_sb_needed) {
> +		iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
> +		iommu_domain_free(adsp->iommu_dom);

Hi,

Do the iommu_map() in the for loops of adsp_map_smmu() also need some 
iommu_unmap() here?

Maybe introducing a adsp_unmap_smmu() would simplify the release of 
resources.

Does the same resource release makes sense in adsp_stop() or somewhere else?

CJ


> +	}
>   disable_irqs:
>   	qcom_q6v5_unprepare(&adsp->q6v5);
>   

Re: [PATCH v3 7/8] remoteproc: qcom: Add support for memory sandbox

[Srinivasa Rao Mandadapu]

On 8/12/2022 11:02 AM, Christophe JAILLET wrote:
Thanks for your time Christophe!!!
> Le 10/08/2022 à 09:45, 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.
>>
>
> Hi,
>
> comments below about error handling paths that look incomplete to me.
>
> Just my 2c.
>
>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> ---
>> 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 | 107 
>> +++++++++++++++++++++++++++++++++++-
>>   1 file changed, 105 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_adsp.c 
>> b/drivers/remoteproc/qcom_q6v5_adsp.c
>> index f2945bf..b9cafe2 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,94 @@ static int adsp_load(struct rproc *rproc, const 
>> struct firmware *fw)
>>       return 0;
>>   }
>>   +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");
>> +        return -ENOMEM;
>> +    }
>> +
>> +    ret = iommu_attach_device(adsp->iommu_dom, adsp->dev);
>> +    if (ret) {
>> +        dev_err(adsp->dev, "could not attach device ret = %d\n", ret);
>
> iommu_domain_free() or error handling path (see below)?
>
>> +        return -EBUSY;
>> +    }
>> +
>> +    /* 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");
>
> iommu_domain_free() or error handling path (see below)?
Okay. Will update accordingly!!!
>
>> +        return ret;
>> +    }
>> +
>> +    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);
>> +                return ret;
>
> Should there be an error handling path to undo iommu_domain_alloc() 
> and iommu_map() above.
> Same for iommu_map() already done in the loop.
Okay. Will update accordingly!!!
>
>> +            }
>> +        }
>> +    } else {
>> +        if (!rproc->table_ptr)
>> +            return 0;
>> +
>> +        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__);
>> +                return ret;
>
> Same comment.
Okay.
>
>> +            }
>> +        }
>> +    }
>> +    return 0;
>> +}
>> +
>> +
>>   static int adsp_start(struct rproc *rproc)
>>   {
>>       struct qcom_adsp *adsp = (struct qcom_adsp *)rproc->priv;
>> @@ -343,9 +434,16 @@ static int adsp_start(struct rproc *rproc)
>>       if (ret)
>>           return ret;
>>   +    if (adsp->is_adsp_sb_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 +499,11 @@ 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->is_adsp_sb_needed) {
>> +        iommu_unmap(adsp->iommu_dom, adsp->mem_phys, adsp->mem_size);
>> +        iommu_domain_free(adsp->iommu_dom);
>
> Hi,
>
> Do the iommu_map() in the for loops of adsp_map_smmu() also need some 
> iommu_unmap() here?
>
> Maybe introducing a adsp_unmap_smmu() would simplify the release of 
> resources.
>
> Does the same resource release makes sense in adsp_stop() or somewhere 
> else?
>
> CJ
>
Okay. Will update accordingly!!!
>
>> +    }
>>   disable_irqs:
>>       qcom_q6v5_unprepare(&adsp->q6v5);
>

Re: [PATCH v3 1/8] dt-bindings: remoteproc: qcom: adsp: Make ADSP pil loader as generic

[Rob Herring]

On Wed, 10 Aug 2022 13:15:51 +0530, Srinivasa Rao Mandadapu wrote:
> Rename sdm845 adsp pil bindings to generic name, for using same binings
> file for subsequent SoCs.
> 
> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
> ---
> Changes since V2:
> 	-- Generate patch with -M flag.
> 
>  .../{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml}           | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>  rename Documentation/devicetree/bindings/remoteproc/{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml} (97%)
> 

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v3 1/8] dt-bindings: remoteproc: qcom: adsp: Make ADSP pil loader as generic

[Srinivasa Rao Mandadapu]

On 8/15/2022 2:15 AM, Rob Herring wrote:
Thanks for Your time Rob!!!
> On Wed, 10 Aug 2022 13:15:51 +0530, Srinivasa Rao Mandadapu wrote:
>> Rename sdm845 adsp pil bindings to generic name, for using same binings
>> file for subsequent SoCs.
>>
>> Signed-off-by: Srinivasa Rao Mandadapu <quic_srivasam@quicinc.com>
>> ---
>> Changes since V2:
>> 	-- Generate patch with -M flag.
>>
>>   .../{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml}           | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>   rename Documentation/devicetree/bindings/remoteproc/{qcom,sdm845-adsp-pil.yaml => qcom,lpass-adsp-pil.yaml} (97%)
>>
> Reviewed-by: Rob Herring <robh@kernel.org>
This patch dropped in consequent series. Please ignore this patch.