[PATCH v2 0/2] Add Modem support on SC7180 SoCs

[PATCH v2 0/2] Add Modem support on SC7180 SoCs

[Sibi Sankar]

This patch series adds support for booting the Modem Q6 DSP found in
Qualcomm's SC7180 SoCs.

V2:
 * split the series according to SoC
 * Duplicate code across q6v5proc_reset
 * add Rob's 'Reviewed-by' tag

Sibi Sankar (2):
  dt-bindings: remoteproc: qcom: Add Q6V5 Modem PIL binding for SC7180
  remoteproc: mss: q6v5-mss: Add modem support on SC7180

 .../bindings/remoteproc/qcom,q6v5.txt         |  23 +-
 drivers/remoteproc/qcom_q6v5_mss.c            | 199 +++++++++++++++++-
 2 files changed, 219 insertions(+), 3 deletions(-)

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 1/2] dt-bindings: remoteproc: qcom: Add Q6V5 Modem PIL binding for SC7180

[Sibi Sankar]

Add a new modem compatible string for Qualcomm SC7180 SoCs and introduce
the "qcom,halt-nav-regs" bindings needed by the modem sub-system running
on SC7180 SoCs.

Reviewed-by: Rob Herring <robh@kernel.org>
Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 .../bindings/remoteproc/qcom,q6v5.txt         | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
index c416746f93cfd..88dfa3fc15f78 100644
--- a/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
+++ b/Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt
@@ -13,6 +13,7 @@ on the Qualcomm Hexagon core.
 		    "qcom,msm8974-mss-pil"
 		    "qcom,msm8996-mss-pil"
 		    "qcom,msm8998-mss-pil"
+		    "qcom,sc7180-mss-pil"
 		    "qcom,sdm845-mss-pil"
 
 - reg:
@@ -43,6 +44,7 @@ on the Qualcomm Hexagon core.
 		    must be "wdog", "fatal", "ready", "handover", "stop-ack"
 	qcom,msm8996-mss-pil:
 	qcom,msm8998-mss-pil:
+	qcom,sc7180-mss-pil:
 	qcom,sdm845-mss-pil:
 		    must be "wdog", "fatal", "ready", "handover", "stop-ack",
 		    "shutdown-ack"
@@ -75,6 +77,9 @@ on the Qualcomm Hexagon core.
 	qcom,msm8998-mss-pil:
 		    must be "iface", "bus", "mem", "xo", "gpll0_mss",
 		    "snoc_axi", "mnoc_axi", "qdss"
+	qcom,sc7180-mss-pil:
+		    must be "iface", "bus", "xo", "snoc_axi", "mnoc_axi",
+		    "mss_crypto", "mss_nav", "nav"
 	qcom,sdm845-mss-pil:
 		    must be "iface", "bus", "mem", "xo", "gpll0_mss",
 		    "snoc_axi", "mnoc_axi", "prng"
@@ -86,7 +91,7 @@ on the Qualcomm Hexagon core.
 		    reference to the list of 3 reset-controllers for the
 		    wcss sub-system
 		    reference to the list of 2 reset-controllers for the modem
-		    sub-system on SDM845 SoCs
+		    sub-system on SC7180, SDM845 SoCs
 
 - reset-names:
 	Usage: required
@@ -95,7 +100,7 @@ on the Qualcomm Hexagon core.
 		    must be "wcss_aon_reset", "wcss_reset", "wcss_q6_reset"
 		    for the wcss sub-system
 		    must be "mss_restart", "pdc_reset" for the modem
-		    sub-system on SDM845 SoCs
+		    sub-system on SC7180, SDM845 SoCs
 
 For the compatible strings below the following supplies are required:
   "qcom,q6v5-pil"
@@ -144,6 +149,7 @@ For the compatible string below the following supplies are required:
 	qcom,msm8996-mss-pil:
 	qcom,msm8998-mss-pil:
 		    must be "cx", "mx"
+	qcom,sc7180-mss-pil:
 	qcom,sdm845-mss-pil:
 		    must be "cx", "mx", "mss", "load_state"
 
@@ -165,6 +171,19 @@ For the compatible string below the following supplies are required:
 		    by the three offsets within syscon for q6, modem and nc
 		    halt registers.
 
+For the compatible strings below the following phandle references are required:
+  "qcom,sc7180-mss-pil"
+- qcom,halt-nav-regs:
+	Usage: required
+	Value type: <prop-encoded-array>
+	Definition: reference to a list of 2 phandles with one offset each for
+		    the modem sub-system running on SC7180 SoC. The first
+		    phandle reference is to the mss clock node followed by the
+		    offset within register space for nav halt register. The
+		    second phandle reference is to a syscon representing TCSR
+		    followed by the offset within syscon for conn_box_spare0
+		    register.
+
 = SUBNODES:
 The Hexagon node must contain two subnodes, named "mba" and "mpss" representing
 the memory regions used by the Hexagon firmware. Each sub-node must contain:
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH v2 2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180

[Sibi Sankar]

Add the out of reset sequence support for modem sub-system on SC7180
SoCs. It requires access to an additional halt nav register to put
the modem back into reset.

Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199 ++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 164fc2a53ef11..51f451311f5fc 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -68,6 +68,9 @@
 #define AXI_HALTREQ_REG			0x0
 #define AXI_HALTACK_REG			0x4
 #define AXI_IDLE_REG			0x8
+#define NAV_AXI_HALTREQ_BIT		BIT(0)
+#define NAV_AXI_HALTACK_BIT		BIT(1)
+#define NAV_AXI_IDLE_BIT		BIT(2)
 
 #define HALT_ACK_TIMEOUT_MS		100
 
@@ -101,9 +104,11 @@
 #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
 
 /* QDSP6v65 parameters */
+#define QDSP6SS_CORE_CBCR		0x20
 #define QDSP6SS_SLEEP                   0x3C
 #define QDSP6SS_BOOT_CORE_START         0x400
 #define QDSP6SS_BOOT_CMD                0x404
+#define QDSP6SS_BOOT_STATUS		0x408
 #define SLEEP_CHECK_MAX_LOOPS           200
 #define BOOT_FSM_TIMEOUT                10000
 
@@ -131,6 +136,7 @@ struct rproc_hexagon_res {
 	int version;
 	bool need_mem_protection;
 	bool has_alt_reset;
+	bool has_halt_nav;
 };
 
 struct q6v5 {
@@ -141,9 +147,14 @@ struct q6v5 {
 	void __iomem *rmb_base;
 
 	struct regmap *halt_map;
+	struct regmap *halt_nav_map;
+	struct regmap *conn_map;
+
 	u32 halt_q6;
 	u32 halt_modem;
 	u32 halt_nc;
+	u32 halt_nav;
+	u32 conn_box;
 
 	struct reset_control *mss_restart;
 	struct reset_control *pdc_reset;
@@ -187,6 +198,7 @@ struct q6v5 {
 	struct qcom_sysmon *sysmon;
 	bool need_mem_protection;
 	bool has_alt_reset;
+	bool has_halt_nav;
 	int mpss_perm;
 	int mba_perm;
 	const char *hexagon_mdt_image;
@@ -198,6 +210,7 @@ enum {
 	MSS_MSM8974,
 	MSS_MSM8996,
 	MSS_MSM8998,
+	MSS_SC7180,
 	MSS_SDM845,
 };
 
@@ -396,6 +409,18 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
 		reset_control_assert(qproc->pdc_reset);
 		ret = reset_control_reset(qproc->mss_restart);
 		reset_control_deassert(qproc->pdc_reset);
+	} else if (qproc->has_halt_nav) {
+		/* SWAR using CONN_BOX_SPARE_0 for pipeline glitch issue */
+		reset_control_assert(qproc->pdc_reset);
+		regmap_update_bits(qproc->conn_map, qproc->conn_box,
+				   BIT(0), BIT(0));
+		regmap_update_bits(qproc->halt_nav_map, qproc->halt_nav,
+				   NAV_AXI_HALTREQ_BIT, 0);
+		reset_control_assert(qproc->mss_restart);
+		reset_control_deassert(qproc->pdc_reset);
+		regmap_update_bits(qproc->conn_map, qproc->conn_box,
+				   BIT(0), 0);
+		ret = reset_control_deassert(qproc->mss_restart);
 	} else {
 		ret = reset_control_assert(qproc->mss_restart);
 	}
@@ -413,6 +438,8 @@ static int q6v5_reset_deassert(struct q6v5 *qproc)
 		ret = reset_control_reset(qproc->mss_restart);
 		writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
 		reset_control_deassert(qproc->pdc_reset);
+	} else if (qproc->has_halt_nav) {
+		ret = reset_control_reset(qproc->mss_restart);
 	} else {
 		ret = reset_control_deassert(qproc->mss_restart);
 	}
@@ -499,6 +526,54 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 			return ret;
 		}
 
+		goto pbl_wait;
+	} else if (qproc->version == MSS_SC7180) {
+		val = readl(qproc->reg_base + QDSP6SS_SLEEP);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_SLEEP);
+
+		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_SLEEP,
+					 val, !(val & BIT(31)), 1,
+					 SLEEP_CHECK_MAX_LOOPS);
+		if (ret) {
+			dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
+			return -ETIMEDOUT;
+		}
+
+		/* Turn on the XO clock needed for PLL setup */
+		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+
+		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
+					 val, !(val & BIT(31)), 1,
+					 SLEEP_CHECK_MAX_LOOPS);
+		if (ret) {
+			dev_err(qproc->dev, "QDSP6SS XO clock timed out\n");
+			return -ETIMEDOUT;
+		}
+
+		/* Configure Q6 core CBCR to auto-enable after reset sequence */
+		val = readl(qproc->reg_base + QDSP6SS_CORE_CBCR);
+		val |= 0x1;
+		writel(val, qproc->reg_base + QDSP6SS_CORE_CBCR);
+
+		/* De-assert the Q6 stop core signal */
+		writel(1, qproc->reg_base + QDSP6SS_BOOT_CORE_START);
+
+		/* Trigger the boot FSM to start the Q6 out-of-reset sequence */
+		writel(1, qproc->reg_base + QDSP6SS_BOOT_CMD);
+
+		/* Poll the QDSP6SS_BOOT_STATUS for FSM completion */
+		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_BOOT_STATUS,
+					 val, (val & BIT(0)) != 0, 1,
+					 SLEEP_CHECK_MAX_LOOPS);
+		if (ret) {
+			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
+			/* Reset the modem so that boot FSM is in reset state */
+			q6v5_reset_deassert(qproc);
+			return ret;
+		}
 		goto pbl_wait;
 	} else if (qproc->version == MSS_MSM8996 ||
 		   qproc->version == MSS_MSM8998) {
@@ -667,6 +742,39 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
 	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
 }
 
+static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
+				       struct regmap *halt_map,
+				       u32 offset)
+{
+	unsigned long timeout;
+	unsigned int val;
+	int ret;
+
+	/* Check if we're already idle */
+	ret = regmap_read(halt_map, offset, &val);
+	if (!ret && (val & NAV_AXI_IDLE_BIT))
+		return;
+
+	/* Assert halt request */
+	regmap_update_bits(halt_map, offset, NAV_AXI_HALTREQ_BIT,
+			   NAV_AXI_HALTREQ_BIT);
+
+	/* Wait for halt ack*/
+	timeout = jiffies + msecs_to_jiffies(HALT_ACK_TIMEOUT_MS);
+	for (;;) {
+		ret = regmap_read(halt_map, offset, &val);
+		if (ret || (val & NAV_AXI_HALTACK_BIT) ||
+		    time_after(jiffies, timeout))
+			break;
+
+		udelay(5);
+	}
+
+	ret = regmap_read(halt_map, offset, &val);
+	if (ret || !(val & NAV_AXI_IDLE_BIT))
+		dev_err(qproc->dev, "port failed halt\n");
+}
+
 static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
 {
 	unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
@@ -829,6 +937,9 @@ static int q6v5_mba_load(struct q6v5 *qproc)
 halt_axi_ports:
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
+	if (qproc->has_halt_nav)
+		q6v5proc_halt_nav_axi_port(qproc, qproc->halt_nav_map,
+					   qproc->halt_nav);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 
 reclaim_mba:
@@ -876,6 +987,9 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc)
 
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
+	if (qproc->has_halt_nav)
+		q6v5proc_halt_nav_axi_port(qproc, qproc->halt_nav_map,
+					   qproc->halt_nav);
 	q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
 	if (qproc->version == MSS_MSM8996) {
 		/*
@@ -1253,6 +1367,47 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
 	qproc->halt_modem = args.args[1];
 	qproc->halt_nc = args.args[2];
 
+	if (qproc->has_halt_nav) {
+		struct platform_device *nav_pdev;
+
+		ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
+						       "qcom,halt-nav-regs",
+						       1, 0, &args);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to parse halt-nav-regs\n");
+			return -EINVAL;
+		}
+
+		nav_pdev = of_find_device_by_node(args.np);
+		of_node_put(args.np);
+		if (!nav_pdev) {
+			dev_err(&pdev->dev, "failed to get mss clock device\n");
+			return -EPROBE_DEFER;
+		}
+
+		qproc->halt_nav_map = dev_get_regmap(&nav_pdev->dev, NULL);
+		if (!qproc->halt_nav_map) {
+			dev_err(&pdev->dev, "failed to get map from device\n");
+			return -EINVAL;
+		}
+		qproc->halt_nav = args.args[0];
+
+		ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
+						       "qcom,halt-nav-regs",
+						       1, 1, &args);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to parse halt-nav-regs\n");
+			return -EINVAL;
+		}
+
+		qproc->conn_map = syscon_node_to_regmap(args.np);
+		of_node_put(args.np);
+		if (IS_ERR(qproc->conn_map))
+			return PTR_ERR(qproc->conn_map);
+
+		qproc->conn_box = args.args[0];
+	}
+
 	return 0;
 }
 
@@ -1327,7 +1482,7 @@ static int q6v5_init_reset(struct q6v5 *qproc)
 		return PTR_ERR(qproc->mss_restart);
 	}
 
-	if (qproc->has_alt_reset) {
+	if (qproc->has_alt_reset || qproc->has_halt_nav) {
 		qproc->pdc_reset = devm_reset_control_get_exclusive(qproc->dev,
 								    "pdc_reset");
 		if (IS_ERR(qproc->pdc_reset)) {
@@ -1426,6 +1581,7 @@ static int q6v5_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, qproc);
 
+	qproc->has_halt_nav = desc->has_halt_nav;
 	ret = q6v5_init_mem(qproc, pdev);
 	if (ret)
 		goto free_rproc;
@@ -1549,6 +1705,41 @@ static int q6v5_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static const struct rproc_hexagon_res sc7180_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_clk_names = (char*[]){
+		"xo",
+		NULL
+	},
+	.reset_clk_names = (char*[]){
+		"iface",
+		"bus",
+		"snoc_axi",
+		NULL
+	},
+	.active_clk_names = (char*[]){
+		"mnoc_axi",
+		"nav",
+		"mss_nav",
+		"mss_crypto",
+		NULL
+	},
+	.active_pd_names = (char*[]){
+		"load_state",
+		NULL
+	},
+	.proxy_pd_names = (char*[]){
+		"cx",
+		"mx",
+		"mss",
+		NULL
+	},
+	.need_mem_protection = true,
+	.has_alt_reset = false,
+	.has_halt_nav = true,
+	.version = MSS_SC7180,
+};
+
 static const struct rproc_hexagon_res sdm845_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_clk_names = (char*[]){
@@ -1580,6 +1771,7 @@ static const struct rproc_hexagon_res sdm845_mss = {
 	},
 	.need_mem_protection = true,
 	.has_alt_reset = true,
+	.has_halt_nav = false,
 	.version = MSS_SDM845,
 };
 
@@ -1606,6 +1798,7 @@ static const struct rproc_hexagon_res msm8998_mss = {
 	},
 	.need_mem_protection = true,
 	.has_alt_reset = false,
+	.has_halt_nav = false,
 	.version = MSS_MSM8998,
 };
 
@@ -1635,6 +1828,7 @@ static const struct rproc_hexagon_res msm8996_mss = {
 	},
 	.need_mem_protection = true,
 	.has_alt_reset = false,
+	.has_halt_nav = false,
 	.version = MSS_MSM8996,
 };
 
@@ -1667,6 +1861,7 @@ static const struct rproc_hexagon_res msm8916_mss = {
 	},
 	.need_mem_protection = false,
 	.has_alt_reset = false,
+	.has_halt_nav = false,
 	.version = MSS_MSM8916,
 };
 
@@ -1707,6 +1902,7 @@ static const struct rproc_hexagon_res msm8974_mss = {
 	},
 	.need_mem_protection = false,
 	.has_alt_reset = false,
+	.has_halt_nav = false,
 	.version = MSS_MSM8974,
 };
 
@@ -1716,6 +1912,7 @@ static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
 	{ .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
 	{ .compatible = "qcom,msm8998-mss-pil", .data = &msm8998_mss},
+	{ .compatible = "qcom,sc7180-mss-pil", .data = &sc7180_mss},
 	{ .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
 	{ },
 };
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180

[Philipp Zabel]

On Thu, 2019-12-19 at 11:15 +0530, Sibi Sankar wrote:
> Add the out of reset sequence support for modem sub-system on SC7180
> SoCs. It requires access to an additional halt nav register to put
> the modem back into reset.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 199 ++++++++++++++++++++++++++++-
>  1 file changed, 198 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 164fc2a53ef11..51f451311f5fc 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
[...]
> @@ -396,6 +409,18 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
>  		reset_control_assert(qproc->pdc_reset);
>  		ret = reset_control_reset(qproc->mss_restart);
>  		reset_control_deassert(qproc->pdc_reset);
> +	} else if (qproc->has_halt_nav) {
> +		/* SWAR using CONN_BOX_SPARE_0 for pipeline glitch issue */
> +		reset_control_assert(qproc->pdc_reset);
> +		regmap_update_bits(qproc->conn_map, qproc->conn_box,
> +				   BIT(0), BIT(0));
> +		regmap_update_bits(qproc->halt_nav_map, qproc->halt_nav,
> +				   NAV_AXI_HALTREQ_BIT, 0);
> +		reset_control_assert(qproc->mss_restart);
> +		reset_control_deassert(qproc->pdc_reset);
> +		regmap_update_bits(qproc->conn_map, qproc->conn_box,
> +				   BIT(0), 0);
> +		ret = reset_control_deassert(qproc->mss_restart);
>  	} else {
>  		ret = reset_control_assert(qproc->mss_restart);
>  	}

Without knowing anything about the hardware this does look a bit weird,
but I assume there is a good reason for every step.

Is the function name still describing its behaviour correctly, or would
it make sense to rename q6v5_reset_assert/deassert to something else?

[...]
> @@ -667,6 +742,39 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>  }
>  
> +static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
> +				       struct regmap *halt_map,
> +				       u32 offset)
> +{
[...]
> +	/* Wait for halt ack*/
> +	timeout = jiffies + msecs_to_jiffies(HALT_ACK_TIMEOUT_MS);
> +	for (;;) {
> +		ret = regmap_read(halt_map, offset, &val);
> +		if (ret || (val & NAV_AXI_HALTACK_BIT) ||
> +		    time_after(jiffies, timeout))
> +			break;
> +
> +		udelay(5);
> +	}

This does look like a candidate for using regmap_read_poll_timeout().

regards
Philipp

Re: [PATCH v2 2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180

[Sibi Sankar]

Hey Philipp,

Thanks for taking time to review the
patch series.

On 2020-01-02 18:56, Philipp Zabel wrote:
> On Thu, 2019-12-19 at 11:15 +0530, Sibi Sankar wrote:
>> Add the out of reset sequence support for modem sub-system on SC7180
>> SoCs. It requires access to an additional halt nav register to put
>> the modem back into reset.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_q6v5_mss.c | 199 
>> ++++++++++++++++++++++++++++-
>>  1 file changed, 198 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 164fc2a53ef11..51f451311f5fc 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> [...]
>> @@ -396,6 +409,18 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
>>  		reset_control_assert(qproc->pdc_reset);
>>  		ret = reset_control_reset(qproc->mss_restart);
>>  		reset_control_deassert(qproc->pdc_reset);
>> +	} else if (qproc->has_halt_nav) {
>> +		/* SWAR using CONN_BOX_SPARE_0 for pipeline glitch issue */
>> +		reset_control_assert(qproc->pdc_reset);
>> +		regmap_update_bits(qproc->conn_map, qproc->conn_box,
>> +				   BIT(0), BIT(0));
>> +		regmap_update_bits(qproc->halt_nav_map, qproc->halt_nav,
>> +				   NAV_AXI_HALTREQ_BIT, 0);
>> +		reset_control_assert(qproc->mss_restart);
>> +		reset_control_deassert(qproc->pdc_reset);
>> +		regmap_update_bits(qproc->conn_map, qproc->conn_box,
>> +				   BIT(0), 0);
>> +		ret = reset_control_deassert(qproc->mss_restart);
>>  	} else {
>>  		ret = reset_control_assert(qproc->mss_restart);
>>  	}
> 
> Without knowing anything about the hardware this does look a bit weird,
> but I assume there is a good reason for every step.
> 
> Is the function name still describing its behaviour correctly, or would
> it make sense to rename q6v5_reset_assert/deassert to something else?

In qualcomm terminology, Q6 remote
processor requires its reset deasserted
while bringing it out of reset and reset
asserted while putting it back into reset.
However over time deassert/assert has
morphed into a sequence which we still
collectively refer to reset assert/
deassert.

I'm prefer continuing with the same name
since it helps abstract the out of reset
sequence into a few simple steps
(enable supplies->clk->pds->reset_deassert)
(disable reset_assert->clk->pds->supplies)
I'll let Bjorn decide what to do on this.

> 
> [...]
>> @@ -667,6 +742,39 @@ static void q6v5proc_halt_axi_port(struct q6v5 
>> *qproc,
>>  	regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>>  }
>> 
>> +static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
>> +				       struct regmap *halt_map,
>> +				       u32 offset)
>> +{
> [...]
>> +	/* Wait for halt ack*/
>> +	timeout = jiffies + msecs_to_jiffies(HALT_ACK_TIMEOUT_MS);
>> +	for (;;) {
>> +		ret = regmap_read(halt_map, offset, &val);
>> +		if (ret || (val & NAV_AXI_HALTACK_BIT) ||
>> +		    time_after(jiffies, timeout))
>> +			break;
>> +
>> +		udelay(5);
>> +	}
> 
> This does look like a candidate for using regmap_read_poll_timeout().

yes it does... will send a patch for
it since I think the series is already
in lnext.

> 
> regards
> Philipp

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180

[Evan Green]

On Wed, Dec 18, 2019 at 9:45 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Add the out of reset sequence support for modem sub-system on SC7180
> SoCs. It requires access to an additional halt nav register to put
> the modem back into reset.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 199 ++++++++++++++++++++++++++++-
>  1 file changed, 198 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 164fc2a53ef11..51f451311f5fc 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -68,6 +68,9 @@
>  #define AXI_HALTREQ_REG                        0x0
>  #define AXI_HALTACK_REG                        0x4
>  #define AXI_IDLE_REG                   0x8
> +#define NAV_AXI_HALTREQ_BIT            BIT(0)
> +#define NAV_AXI_HALTACK_BIT            BIT(1)
> +#define NAV_AXI_IDLE_BIT               BIT(2)
>
>  #define HALT_ACK_TIMEOUT_MS            100
>
> @@ -101,9 +104,11 @@
>  #define QDSP6SS_ACC_OVERRIDE_VAL               0x20
>
>  /* QDSP6v65 parameters */
> +#define QDSP6SS_CORE_CBCR              0x20
>  #define QDSP6SS_SLEEP                   0x3C
>  #define QDSP6SS_BOOT_CORE_START         0x400
>  #define QDSP6SS_BOOT_CMD                0x404
> +#define QDSP6SS_BOOT_STATUS            0x408
>  #define SLEEP_CHECK_MAX_LOOPS           200
>  #define BOOT_FSM_TIMEOUT                10000
>
> @@ -131,6 +136,7 @@ struct rproc_hexagon_res {
>         int version;
>         bool need_mem_protection;
>         bool has_alt_reset;
> +       bool has_halt_nav;
>  };
>
>  struct q6v5 {
> @@ -141,9 +147,14 @@ struct q6v5 {
>         void __iomem *rmb_base;
>
>         struct regmap *halt_map;
> +       struct regmap *halt_nav_map;
> +       struct regmap *conn_map;
> +
>         u32 halt_q6;
>         u32 halt_modem;
>         u32 halt_nc;
> +       u32 halt_nav;
> +       u32 conn_box;
>
>         struct reset_control *mss_restart;
>         struct reset_control *pdc_reset;
> @@ -187,6 +198,7 @@ struct q6v5 {
>         struct qcom_sysmon *sysmon;
>         bool need_mem_protection;
>         bool has_alt_reset;
> +       bool has_halt_nav;
>         int mpss_perm;
>         int mba_perm;
>         const char *hexagon_mdt_image;
> @@ -198,6 +210,7 @@ enum {
>         MSS_MSM8974,
>         MSS_MSM8996,
>         MSS_MSM8998,
> +       MSS_SC7180,
>         MSS_SDM845,
>  };
>
> @@ -396,6 +409,18 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
>                 reset_control_assert(qproc->pdc_reset);
>                 ret = reset_control_reset(qproc->mss_restart);
>                 reset_control_deassert(qproc->pdc_reset);
> +       } else if (qproc->has_halt_nav) {
> +               /* SWAR using CONN_BOX_SPARE_0 for pipeline glitch issue */

Can you elaborate more in this comment, or remove it? Right now it
doesn't help me since I don't know what SWAR is, I don't see a
reference to CONN_BOX_SPARE_0 in the code, I don't know what a
CONN_BOX is, and I don't know any details about the glitch issue :)

> +               reset_control_assert(qproc->pdc_reset);
> +               regmap_update_bits(qproc->conn_map, qproc->conn_box,
> +                                  BIT(0), BIT(0));

Make a register name #define for this bit?

> +               regmap_update_bits(qproc->halt_nav_map, qproc->halt_nav,
> +                                  NAV_AXI_HALTREQ_BIT, 0);
> +               reset_control_assert(qproc->mss_restart);
> +               reset_control_deassert(qproc->pdc_reset);
> +               regmap_update_bits(qproc->conn_map, qproc->conn_box,
> +                                  BIT(0), 0);
> +               ret = reset_control_deassert(qproc->mss_restart);
>         } else {
>                 ret = reset_control_assert(qproc->mss_restart);
>         }
> @@ -413,6 +438,8 @@ static int q6v5_reset_deassert(struct q6v5 *qproc)
>                 ret = reset_control_reset(qproc->mss_restart);
>                 writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
>                 reset_control_deassert(qproc->pdc_reset);
> +       } else if (qproc->has_halt_nav) {
> +               ret = reset_control_reset(qproc->mss_restart);
>         } else {
>                 ret = reset_control_deassert(qproc->mss_restart);
>         }
> @@ -499,6 +526,54 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>                         return ret;
>                 }
>
> +               goto pbl_wait;

Ick, this could benefit from a refactor that pulls those sorta-common
steps "Remove IO clamp" through "Start core execution" into a helper
function and then calls the new function from each case that needs it.

> +       } else if (qproc->version == MSS_SC7180) {
> +               val = readl(qproc->reg_base + QDSP6SS_SLEEP);
> +               val |= 0x1;

It'd be nice if there were defines for these magic 0x1 values in this hunk too.

> +               writel(val, qproc->reg_base + QDSP6SS_SLEEP);
> +
> +               ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_SLEEP,
> +                                        val, !(val & BIT(31)), 1,
> +                                        SLEEP_CHECK_MAX_LOOPS);
> +               if (ret) {
> +                       dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
> +                       return -ETIMEDOUT;
> +               }

This toggling of the sleep bit is the same as sdm845, it could also be
put into a helper function.

> +
> +               /* Turn on the XO clock needed for PLL setup */
> +               val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> +               val |= 0x1;
> +               writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> +
> +               ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
> +                                        val, !(val & BIT(31)), 1,
> +                                        SLEEP_CHECK_MAX_LOOPS);

Nit: SLEEP_CHECK_MAX_LOOPS isn't a loop count, it should really have
been named something like SLEEP_CHECK_TIMEOUT_US. Could be done in a
separate change.

> +               if (ret) {
> +                       dev_err(qproc->dev, "QDSP6SS XO clock timed out\n");
> +                       return -ETIMEDOUT;
> +               }
> +
> +               /* Configure Q6 core CBCR to auto-enable after reset sequence */
> +               val = readl(qproc->reg_base + QDSP6SS_CORE_CBCR);
> +               val |= 0x1;
> +               writel(val, qproc->reg_base + QDSP6SS_CORE_CBCR);
> +
> +               /* De-assert the Q6 stop core signal */
> +               writel(1, qproc->reg_base + QDSP6SS_BOOT_CORE_START);
> +
> +               /* Trigger the boot FSM to start the Q6 out-of-reset sequence */
> +               writel(1, qproc->reg_base + QDSP6SS_BOOT_CMD);
> +
> +               /* Poll the QDSP6SS_BOOT_STATUS for FSM completion */
> +               ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_BOOT_STATUS,
> +                                        val, (val & BIT(0)) != 0, 1,
> +                                        SLEEP_CHECK_MAX_LOOPS);
> +               if (ret) {
> +                       dev_err(qproc->dev, "Boot FSM failed to complete.\n");
> +                       /* Reset the modem so that boot FSM is in reset state */
> +                       q6v5_reset_deassert(qproc);
> +                       return ret;
> +               }
>                 goto pbl_wait;
>         } else if (qproc->version == MSS_MSM8996 ||
>                    qproc->version == MSS_MSM8998) {
> @@ -667,6 +742,39 @@ static void q6v5proc_halt_axi_port(struct q6v5 *qproc,
>         regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>  }
>
> +static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
> +                                      struct regmap *halt_map,
> +                                      u32 offset)
> +{
> +       unsigned long timeout;
> +       unsigned int val;
> +       int ret;
> +
> +       /* Check if we're already idle */
> +       ret = regmap_read(halt_map, offset, &val);
> +       if (!ret && (val & NAV_AXI_IDLE_BIT))
> +               return;
> +
> +       /* Assert halt request */
> +       regmap_update_bits(halt_map, offset, NAV_AXI_HALTREQ_BIT,
> +                          NAV_AXI_HALTREQ_BIT);
> +
> +       /* Wait for halt ack*/
> +       timeout = jiffies + msecs_to_jiffies(HALT_ACK_TIMEOUT_MS);
> +       for (;;) {
> +               ret = regmap_read(halt_map, offset, &val);
> +               if (ret || (val & NAV_AXI_HALTACK_BIT) ||
> +                   time_after(jiffies, timeout))
> +                       break;
> +
> +               udelay(5);
> +       }
> +
> +       ret = regmap_read(halt_map, offset, &val);
> +       if (ret || !(val & NAV_AXI_IDLE_BIT))
> +               dev_err(qproc->dev, "port failed halt\n");
> +}
> +
>  static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw)
>  {
>         unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
> @@ -829,6 +937,9 @@ static int q6v5_mba_load(struct q6v5 *qproc)
>  halt_axi_ports:
>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> +       if (qproc->has_halt_nav)
> +               q6v5proc_halt_nav_axi_port(qproc, qproc->halt_nav_map,
> +                                          qproc->halt_nav);
>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>
>  reclaim_mba:
> @@ -876,6 +987,9 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc)
>
>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_q6);
>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_modem);
> +       if (qproc->has_halt_nav)
> +               q6v5proc_halt_nav_axi_port(qproc, qproc->halt_nav_map,
> +                                          qproc->halt_nav);
>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, qproc->halt_nc);
>         if (qproc->version == MSS_MSM8996) {
>                 /*
> @@ -1253,6 +1367,47 @@ static int q6v5_init_mem(struct q6v5 *qproc, struct platform_device *pdev)
>         qproc->halt_modem = args.args[1];
>         qproc->halt_nc = args.args[2];
>
> +       if (qproc->has_halt_nav) {
> +               struct platform_device *nav_pdev;
> +
> +               ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> +                                                      "qcom,halt-nav-regs",
> +                                                      1, 0, &args);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "failed to parse halt-nav-regs\n");
> +                       return -EINVAL;
> +               }
> +
> +               nav_pdev = of_find_device_by_node(args.np);
> +               of_node_put(args.np);
> +               if (!nav_pdev) {
> +                       dev_err(&pdev->dev, "failed to get mss clock device\n");
> +                       return -EPROBE_DEFER;
> +               }
> +
> +               qproc->halt_nav_map = dev_get_regmap(&nav_pdev->dev, NULL);
> +               if (!qproc->halt_nav_map) {
> +                       dev_err(&pdev->dev, "failed to get map from device\n");
> +                       return -EINVAL;
> +               }
> +               qproc->halt_nav = args.args[0];
> +
> +               ret = of_parse_phandle_with_fixed_args(pdev->dev.of_node,
> +                                                      "qcom,halt-nav-regs",
> +                                                      1, 1, &args);
> +               if (ret < 0) {
> +                       dev_err(&pdev->dev, "failed to parse halt-nav-regs\n");
> +                       return -EINVAL;
> +               }
> +
> +               qproc->conn_map = syscon_node_to_regmap(args.np);
> +               of_node_put(args.np);
> +               if (IS_ERR(qproc->conn_map))
> +                       return PTR_ERR(qproc->conn_map);
> +
> +               qproc->conn_box = args.args[0];
> +       }
> +
>         return 0;
>  }
>
> @@ -1327,7 +1482,7 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>                 return PTR_ERR(qproc->mss_restart);
>         }
>
> -       if (qproc->has_alt_reset) {
> +       if (qproc->has_alt_reset || qproc->has_halt_nav) {
>                 qproc->pdc_reset = devm_reset_control_get_exclusive(qproc->dev,
>                                                                     "pdc_reset");
>                 if (IS_ERR(qproc->pdc_reset)) {
> @@ -1426,6 +1581,7 @@ static int q6v5_probe(struct platform_device *pdev)
>
>         platform_set_drvdata(pdev, qproc);
>
> +       qproc->has_halt_nav = desc->has_halt_nav;
>         ret = q6v5_init_mem(qproc, pdev);
>         if (ret)
>                 goto free_rproc;
> @@ -1549,6 +1705,41 @@ static int q6v5_remove(struct platform_device *pdev)
>         return 0;
>  }
>
> +static const struct rproc_hexagon_res sc7180_mss = {
> +       .hexagon_mba_image = "mba.mbn",
> +       .proxy_clk_names = (char*[]){
> +               "xo",
> +               NULL
> +       },
> +       .reset_clk_names = (char*[]){
> +               "iface",
> +               "bus",
> +               "snoc_axi",
> +               NULL
> +       },
> +       .active_clk_names = (char*[]){
> +               "mnoc_axi",
> +               "nav",
> +               "mss_nav",
> +               "mss_crypto",
> +               NULL
> +       },
> +       .active_pd_names = (char*[]){
> +               "load_state",
> +               NULL
> +       },
> +       .proxy_pd_names = (char*[]){
> +               "cx",
> +               "mx",
> +               "mss",
> +               NULL
> +       },
> +       .need_mem_protection = true,
> +       .has_alt_reset = false,
> +       .has_halt_nav = true,
> +       .version = MSS_SC7180,
> +};
> +
>  static const struct rproc_hexagon_res sdm845_mss = {
>         .hexagon_mba_image = "mba.mbn",
>         .proxy_clk_names = (char*[]){
> @@ -1580,6 +1771,7 @@ static const struct rproc_hexagon_res sdm845_mss = {
>         },
>         .need_mem_protection = true,
>         .has_alt_reset = true,
> +       .has_halt_nav = false,
>         .version = MSS_SDM845,
>  };
>
> @@ -1606,6 +1798,7 @@ static const struct rproc_hexagon_res msm8998_mss = {
>         },
>         .need_mem_protection = true,
>         .has_alt_reset = false,
> +       .has_halt_nav = false,
>         .version = MSS_MSM8998,
>  };
>
> @@ -1635,6 +1828,7 @@ static const struct rproc_hexagon_res msm8996_mss = {
>         },
>         .need_mem_protection = true,
>         .has_alt_reset = false,
> +       .has_halt_nav = false,
>         .version = MSS_MSM8996,
>  };
>
> @@ -1667,6 +1861,7 @@ static const struct rproc_hexagon_res msm8916_mss = {
>         },
>         .need_mem_protection = false,
>         .has_alt_reset = false,
> +       .has_halt_nav = false,
>         .version = MSS_MSM8916,
>  };
>
> @@ -1707,6 +1902,7 @@ static const struct rproc_hexagon_res msm8974_mss = {
>         },
>         .need_mem_protection = false,
>         .has_alt_reset = false,
> +       .has_halt_nav = false,
>         .version = MSS_MSM8974,
>  };
>
> @@ -1716,6 +1912,7 @@ static const struct of_device_id q6v5_of_match[] = {
>         { .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
>         { .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
>         { .compatible = "qcom,msm8998-mss-pil", .data = &msm8998_mss},
> +       { .compatible = "qcom,sc7180-mss-pil", .data = &sc7180_mss},
>         { .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
>         { },
>  };
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

Re: [PATCH v2 2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180

[Sibi Sankar]

Hey Evan,

Thanks for the review!
sry for the delayed response

On 2020-01-08 02:51, Evan Green wrote:
> On Wed, Dec 18, 2019 at 9:45 PM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Add the out of reset sequence support for modem sub-system on SC7180
>> SoCs. It requires access to an additional halt nav register to put
>> the modem back into reset.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_q6v5_mss.c | 199 
>> ++++++++++++++++++++++++++++-
>>  1 file changed, 198 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 164fc2a53ef11..51f451311f5fc 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -68,6 +68,9 @@
>>  #define AXI_HALTREQ_REG                        0x0
>>  #define AXI_HALTACK_REG                        0x4
>>  #define AXI_IDLE_REG                   0x8
>> +#define NAV_AXI_HALTREQ_BIT            BIT(0)
>> +#define NAV_AXI_HALTACK_BIT            BIT(1)
>> +#define NAV_AXI_IDLE_BIT               BIT(2)
>> 
>>  #define HALT_ACK_TIMEOUT_MS            100
>> 
>> @@ -101,9 +104,11 @@
>>  #define QDSP6SS_ACC_OVERRIDE_VAL               0x20
>> 
>>  /* QDSP6v65 parameters */
>> +#define QDSP6SS_CORE_CBCR              0x20
>>  #define QDSP6SS_SLEEP                   0x3C
>>  #define QDSP6SS_BOOT_CORE_START         0x400
>>  #define QDSP6SS_BOOT_CMD                0x404
>> +#define QDSP6SS_BOOT_STATUS            0x408
>>  #define SLEEP_CHECK_MAX_LOOPS           200
>>  #define BOOT_FSM_TIMEOUT                10000
>> 
>> @@ -131,6 +136,7 @@ struct rproc_hexagon_res {
>>         int version;
>>         bool need_mem_protection;
>>         bool has_alt_reset;
>> +       bool has_halt_nav;
>>  };
>> 
>>  struct q6v5 {
>> @@ -141,9 +147,14 @@ struct q6v5 {
>>         void __iomem *rmb_base;
>> 
>>         struct regmap *halt_map;
>> +       struct regmap *halt_nav_map;
>> +       struct regmap *conn_map;
>> +
>>         u32 halt_q6;
>>         u32 halt_modem;
>>         u32 halt_nc;
>> +       u32 halt_nav;
>> +       u32 conn_box;
>> 
>>         struct reset_control *mss_restart;
>>         struct reset_control *pdc_reset;
>> @@ -187,6 +198,7 @@ struct q6v5 {
>>         struct qcom_sysmon *sysmon;
>>         bool need_mem_protection;
>>         bool has_alt_reset;
>> +       bool has_halt_nav;
>>         int mpss_perm;
>>         int mba_perm;
>>         const char *hexagon_mdt_image;
>> @@ -198,6 +210,7 @@ enum {
>>         MSS_MSM8974,
>>         MSS_MSM8996,
>>         MSS_MSM8998,
>> +       MSS_SC7180,
>>         MSS_SDM845,
>>  };
>> 
>> @@ -396,6 +409,18 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
>>                 reset_control_assert(qproc->pdc_reset);
>>                 ret = reset_control_reset(qproc->mss_restart);
>>                 reset_control_deassert(qproc->pdc_reset);
>> +       } else if (qproc->has_halt_nav) {
>> +               /* SWAR using CONN_BOX_SPARE_0 for pipeline glitch 
>> issue */
> 
> Can you elaborate more in this comment, or remove it? Right now it
> doesn't help me since I don't know what SWAR is, I don't see a

SWAR -> software work around

I'll have to stop with the dumb
abbreviations

> reference to CONN_BOX_SPARE_0 in the code, I don't know what a

conn_box_spare_0 is at an offset
0xb3e4 in the conn_map which is
described in the bindings.

> CONN_BOX is, and I don't know any details about the glitch issue :)

lol, yes I get that the comment does
not give details on the glitch. It
was targeted towards explaining why
there is a deviation in the reset_
assert sequence from the other SoCs.
If you still feel the comment does
not add any value I can get it
removed.

> 
>> +               reset_control_assert(qproc->pdc_reset);
>> +               regmap_update_bits(qproc->conn_map, qproc->conn_box,
>> +                                  BIT(0), BIT(0));
> 
> Make a register name #define for this bit?

we'll have to make one up for it
since conn_box_spare_0 does not
have any predefined bits and seems
to be used to implement some missing
functionality.

> 
>> +               regmap_update_bits(qproc->halt_nav_map, 
>> qproc->halt_nav,
>> +                                  NAV_AXI_HALTREQ_BIT, 0);
>> +               reset_control_assert(qproc->mss_restart);
>> +               reset_control_deassert(qproc->pdc_reset);
>> +               regmap_update_bits(qproc->conn_map, qproc->conn_box,
>> +                                  BIT(0), 0);
>> +               ret = reset_control_deassert(qproc->mss_restart);
>>         } else {
>>                 ret = reset_control_assert(qproc->mss_restart);
>>         }
>> @@ -413,6 +438,8 @@ static int q6v5_reset_deassert(struct q6v5 *qproc)
>>                 ret = reset_control_reset(qproc->mss_restart);
>>                 writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
>>                 reset_control_deassert(qproc->pdc_reset);
>> +       } else if (qproc->has_halt_nav) {
>> +               ret = reset_control_reset(qproc->mss_restart);
>>         } else {
>>                 ret = reset_control_deassert(qproc->mss_restart);
>>         }
>> @@ -499,6 +526,54 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>                         return ret;
>>                 }
>> 
>> +               goto pbl_wait;
> 
> Ick, this could benefit from a refactor that pulls those sorta-common
> steps "Remove IO clamp" through "Start core execution" into a helper
> function and then calls the new function from each case that needs it.
> 
>> +       } else if (qproc->version == MSS_SC7180) {
>> +               val = readl(qproc->reg_base + QDSP6SS_SLEEP);
>> +               val |= 0x1;
> 
> It'd be nice if there were defines for these magic 0x1 values in this 
> hunk too.
> 
>> +               writel(val, qproc->reg_base + QDSP6SS_SLEEP);
>> +
>> +               ret = readl_poll_timeout(qproc->reg_base + 
>> QDSP6SS_SLEEP,
>> +                                        val, !(val & BIT(31)), 1,
>> +                                        SLEEP_CHECK_MAX_LOOPS);
>> +               if (ret) {
>> +                       dev_err(qproc->dev, "QDSP6SS Sleep clock timed 
>> out\n");
>> +                       return -ETIMEDOUT;
>> +               }
> 
> This toggling of the sleep bit is the same as sdm845, it could also be
> put into a helper function.

https://patchwork.kernel.org/patch/11250301/
https://patchwork.kernel.org/patch/11250317/

I did club ^^ a portion of the out of
reset sequence together reducing
code duplication initially but that
made the entire thing unreadable.

The general consensus at that time
was to introduce a patch for sc7180
that is most readable and have a
cleanup later with a lot of helper
functions and less branching like
you suggested.

> 
>> +
>> +               /* Turn on the XO clock needed for PLL setup */
>> +               val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
>> +               val |= 0x1;
>> +               writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
>> +
>> +               ret = readl_poll_timeout(qproc->reg_base + 
>> QDSP6SS_XO_CBCR,
>> +                                        val, !(val & BIT(31)), 1,
>> +                                        SLEEP_CHECK_MAX_LOOPS);
> 
> Nit: SLEEP_CHECK_MAX_LOOPS isn't a loop count, it should really have
> been named something like SLEEP_CHECK_TIMEOUT_US. Could be done in a
> separate change.

I'll skip this for now. I'll
make sure that this gets named
appropriately during the
planned refactor.

> 
>> +               if (ret) {
>> +                       dev_err(qproc->dev, "QDSP6SS XO clock timed 
>> out\n");
>> +                       return -ETIMEDOUT;
>> +               }
>> +
>> +               /* Configure Q6 core CBCR to auto-enable after reset 
>> sequence */
>> +               val = readl(qproc->reg_base + QDSP6SS_CORE_CBCR);
>> +               val |= 0x1;
>> +               writel(val, qproc->reg_base + QDSP6SS_CORE_CBCR);
>> +
>> +               /* De-assert the Q6 stop core signal */
>> +               writel(1, qproc->reg_base + QDSP6SS_BOOT_CORE_START);
>> +
>> +               /* Trigger the boot FSM to start the Q6 out-of-reset 
>> sequence */
>> +               writel(1, qproc->reg_base + QDSP6SS_BOOT_CMD);
>> +
>> +               /* Poll the QDSP6SS_BOOT_STATUS for FSM completion */
>> +               ret = readl_poll_timeout(qproc->reg_base + 
>> QDSP6SS_BOOT_STATUS,
>> +                                        val, (val & BIT(0)) != 0, 1,
>> +                                        SLEEP_CHECK_MAX_LOOPS);
>> +               if (ret) {
>> +                       dev_err(qproc->dev, "Boot FSM failed to 
>> complete.\n");
>> +                       /* Reset the modem so that boot FSM is in 
>> reset state */
>> +                       q6v5_reset_deassert(qproc);
>> +                       return ret;
>> +               }
>>                 goto pbl_wait;
>>         } else if (qproc->version == MSS_MSM8996 ||
>>                    qproc->version == MSS_MSM8998) {
>> @@ -667,6 +742,39 @@ static void q6v5proc_halt_axi_port(struct q6v5 
>> *qproc,
>>         regmap_write(halt_map, offset + AXI_HALTREQ_REG, 0);
>>  }
>> 
>> +static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
>> +                                      struct regmap *halt_map,
>> +                                      u32 offset)
>> +{
>> +       unsigned long timeout;
>> +       unsigned int val;
>> +       int ret;
>> +
>> +       /* Check if we're already idle */
>> +       ret = regmap_read(halt_map, offset, &val);
>> +       if (!ret && (val & NAV_AXI_IDLE_BIT))
>> +               return;
>> +
>> +       /* Assert halt request */
>> +       regmap_update_bits(halt_map, offset, NAV_AXI_HALTREQ_BIT,
>> +                          NAV_AXI_HALTREQ_BIT);
>> +
>> +       /* Wait for halt ack*/
>> +       timeout = jiffies + msecs_to_jiffies(HALT_ACK_TIMEOUT_MS);
>> +       for (;;) {
>> +               ret = regmap_read(halt_map, offset, &val);
>> +               if (ret || (val & NAV_AXI_HALTACK_BIT) ||
>> +                   time_after(jiffies, timeout))
>> +                       break;
>> +
>> +               udelay(5);
>> +       }
>> +
>> +       ret = regmap_read(halt_map, offset, &val);
>> +       if (ret || !(val & NAV_AXI_IDLE_BIT))
>> +               dev_err(qproc->dev, "port failed halt\n");
>> +}
>> +
>>  static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct 
>> firmware *fw)
>>  {
>>         unsigned long dma_attrs = DMA_ATTR_FORCE_CONTIGUOUS;
>> @@ -829,6 +937,9 @@ static int q6v5_mba_load(struct q6v5 *qproc)
>>  halt_axi_ports:
>>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, 
>> qproc->halt_q6);
>>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, 
>> qproc->halt_modem);
>> +       if (qproc->has_halt_nav)
>> +               q6v5proc_halt_nav_axi_port(qproc, qproc->halt_nav_map,
>> +                                          qproc->halt_nav);
>>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, 
>> qproc->halt_nc);
>> 
>>  reclaim_mba:
>> @@ -876,6 +987,9 @@ static void q6v5_mba_reclaim(struct q6v5 *qproc)
>> 
>>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, 
>> qproc->halt_q6);
>>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, 
>> qproc->halt_modem);
>> +       if (qproc->has_halt_nav)
>> +               q6v5proc_halt_nav_axi_port(qproc, qproc->halt_nav_map,
>> +                                          qproc->halt_nav);
>>         q6v5proc_halt_axi_port(qproc, qproc->halt_map, 
>> qproc->halt_nc);
>>         if (qproc->version == MSS_MSM8996) {
>>                 /*
>> @@ -1253,6 +1367,47 @@ static int q6v5_init_mem(struct q6v5 *qproc, 
>> struct platform_device *pdev)
>>         qproc->halt_modem = args.args[1];
>>         qproc->halt_nc = args.args[2];
>> 
>> +       if (qproc->has_halt_nav) {
>> +               struct platform_device *nav_pdev;
>> +
>> +               ret = 
>> of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>> +                                                      
>> "qcom,halt-nav-regs",
>> +                                                      1, 0, &args);
>> +               if (ret < 0) {
>> +                       dev_err(&pdev->dev, "failed to parse 
>> halt-nav-regs\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               nav_pdev = of_find_device_by_node(args.np);
>> +               of_node_put(args.np);
>> +               if (!nav_pdev) {
>> +                       dev_err(&pdev->dev, "failed to get mss clock 
>> device\n");
>> +                       return -EPROBE_DEFER;
>> +               }
>> +
>> +               qproc->halt_nav_map = dev_get_regmap(&nav_pdev->dev, 
>> NULL);
>> +               if (!qproc->halt_nav_map) {
>> +                       dev_err(&pdev->dev, "failed to get map from 
>> device\n");
>> +                       return -EINVAL;
>> +               }
>> +               qproc->halt_nav = args.args[0];
>> +
>> +               ret = 
>> of_parse_phandle_with_fixed_args(pdev->dev.of_node,
>> +                                                      
>> "qcom,halt-nav-regs",
>> +                                                      1, 1, &args);
>> +               if (ret < 0) {
>> +                       dev_err(&pdev->dev, "failed to parse 
>> halt-nav-regs\n");
>> +                       return -EINVAL;
>> +               }
>> +
>> +               qproc->conn_map = syscon_node_to_regmap(args.np);
>> +               of_node_put(args.np);
>> +               if (IS_ERR(qproc->conn_map))
>> +                       return PTR_ERR(qproc->conn_map);
>> +
>> +               qproc->conn_box = args.args[0];
>> +       }
>> +
>>         return 0;
>>  }
>> 
>> @@ -1327,7 +1482,7 @@ static int q6v5_init_reset(struct q6v5 *qproc)
>>                 return PTR_ERR(qproc->mss_restart);
>>         }
>> 
>> -       if (qproc->has_alt_reset) {
>> +       if (qproc->has_alt_reset || qproc->has_halt_nav) {
>>                 qproc->pdc_reset = 
>> devm_reset_control_get_exclusive(qproc->dev,
>>                                                                     
>> "pdc_reset");
>>                 if (IS_ERR(qproc->pdc_reset)) {
>> @@ -1426,6 +1581,7 @@ static int q6v5_probe(struct platform_device 
>> *pdev)
>> 
>>         platform_set_drvdata(pdev, qproc);
>> 
>> +       qproc->has_halt_nav = desc->has_halt_nav;
>>         ret = q6v5_init_mem(qproc, pdev);
>>         if (ret)
>>                 goto free_rproc;
>> @@ -1549,6 +1705,41 @@ static int q6v5_remove(struct platform_device 
>> *pdev)
>>         return 0;
>>  }
>> 
>> +static const struct rproc_hexagon_res sc7180_mss = {
>> +       .hexagon_mba_image = "mba.mbn",
>> +       .proxy_clk_names = (char*[]){
>> +               "xo",
>> +               NULL
>> +       },
>> +       .reset_clk_names = (char*[]){
>> +               "iface",
>> +               "bus",
>> +               "snoc_axi",
>> +               NULL
>> +       },
>> +       .active_clk_names = (char*[]){
>> +               "mnoc_axi",
>> +               "nav",
>> +               "mss_nav",
>> +               "mss_crypto",
>> +               NULL
>> +       },
>> +       .active_pd_names = (char*[]){
>> +               "load_state",
>> +               NULL
>> +       },
>> +       .proxy_pd_names = (char*[]){
>> +               "cx",
>> +               "mx",
>> +               "mss",
>> +               NULL
>> +       },
>> +       .need_mem_protection = true,
>> +       .has_alt_reset = false,
>> +       .has_halt_nav = true,
>> +       .version = MSS_SC7180,
>> +};
>> +
>>  static const struct rproc_hexagon_res sdm845_mss = {
>>         .hexagon_mba_image = "mba.mbn",
>>         .proxy_clk_names = (char*[]){
>> @@ -1580,6 +1771,7 @@ static const struct rproc_hexagon_res sdm845_mss 
>> = {
>>         },
>>         .need_mem_protection = true,
>>         .has_alt_reset = true,
>> +       .has_halt_nav = false,
>>         .version = MSS_SDM845,
>>  };
>> 
>> @@ -1606,6 +1798,7 @@ static const struct rproc_hexagon_res 
>> msm8998_mss = {
>>         },
>>         .need_mem_protection = true,
>>         .has_alt_reset = false,
>> +       .has_halt_nav = false,
>>         .version = MSS_MSM8998,
>>  };
>> 
>> @@ -1635,6 +1828,7 @@ static const struct rproc_hexagon_res 
>> msm8996_mss = {
>>         },
>>         .need_mem_protection = true,
>>         .has_alt_reset = false,
>> +       .has_halt_nav = false,
>>         .version = MSS_MSM8996,
>>  };
>> 
>> @@ -1667,6 +1861,7 @@ static const struct rproc_hexagon_res 
>> msm8916_mss = {
>>         },
>>         .need_mem_protection = false,
>>         .has_alt_reset = false,
>> +       .has_halt_nav = false,
>>         .version = MSS_MSM8916,
>>  };
>> 
>> @@ -1707,6 +1902,7 @@ static const struct rproc_hexagon_res 
>> msm8974_mss = {
>>         },
>>         .need_mem_protection = false,
>>         .has_alt_reset = false,
>> +       .has_halt_nav = false,
>>         .version = MSS_MSM8974,
>>  };
>> 
>> @@ -1716,6 +1912,7 @@ static const struct of_device_id q6v5_of_match[] 
>> = {
>>         { .compatible = "qcom,msm8974-mss-pil", .data = &msm8974_mss},
>>         { .compatible = "qcom,msm8996-mss-pil", .data = &msm8996_mss},
>>         { .compatible = "qcom,msm8998-mss-pil", .data = &msm8998_mss},
>> +       { .compatible = "qcom,sc7180-mss-pil", .data = &sc7180_mss},
>>         { .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
>>         { },
>>  };
>> --
>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>> Forum,
>> a Linux Foundation Collaborative Project

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.

Re: [PATCH v2 2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180

[Evan Green]

On Mon, Jan 13, 2020 at 11:06 PM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Hey Evan,
>
> Thanks for the review!
> sry for the delayed response
>
> On 2020-01-08 02:51, Evan Green wrote:
> > On Wed, Dec 18, 2019 at 9:45 PM Sibi Sankar <sibis@codeaurora.org>
> > wrote:
> >>
> >> Add the out of reset sequence support for modem sub-system on SC7180
> >> SoCs. It requires access to an additional halt nav register to put
> >> the modem back into reset.
> >>
> >> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> >> ---
> >>  drivers/remoteproc/qcom_q6v5_mss.c | 199
> >> ++++++++++++++++++++++++++++-
> >>  1 file changed, 198 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c
> >> b/drivers/remoteproc/qcom_q6v5_mss.c
> >> index 164fc2a53ef11..51f451311f5fc 100644
> >> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> >> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> >> @@ -68,6 +68,9 @@
> >>  #define AXI_HALTREQ_REG                        0x0
> >>  #define AXI_HALTACK_REG                        0x4
> >>  #define AXI_IDLE_REG                   0x8
> >> +#define NAV_AXI_HALTREQ_BIT            BIT(0)
> >> +#define NAV_AXI_HALTACK_BIT            BIT(1)
> >> +#define NAV_AXI_IDLE_BIT               BIT(2)
> >>
> >>  #define HALT_ACK_TIMEOUT_MS            100
> >>
> >> @@ -101,9 +104,11 @@
> >>  #define QDSP6SS_ACC_OVERRIDE_VAL               0x20
> >>
> >>  /* QDSP6v65 parameters */
> >> +#define QDSP6SS_CORE_CBCR              0x20
> >>  #define QDSP6SS_SLEEP                   0x3C
> >>  #define QDSP6SS_BOOT_CORE_START         0x400
> >>  #define QDSP6SS_BOOT_CMD                0x404
> >> +#define QDSP6SS_BOOT_STATUS            0x408
> >>  #define SLEEP_CHECK_MAX_LOOPS           200
> >>  #define BOOT_FSM_TIMEOUT                10000
> >>
> >> @@ -131,6 +136,7 @@ struct rproc_hexagon_res {
> >>         int version;
> >>         bool need_mem_protection;
> >>         bool has_alt_reset;
> >> +       bool has_halt_nav;
> >>  };
> >>
> >>  struct q6v5 {
> >> @@ -141,9 +147,14 @@ struct q6v5 {
> >>         void __iomem *rmb_base;
> >>
> >>         struct regmap *halt_map;
> >> +       struct regmap *halt_nav_map;
> >> +       struct regmap *conn_map;
> >> +
> >>         u32 halt_q6;
> >>         u32 halt_modem;
> >>         u32 halt_nc;
> >> +       u32 halt_nav;
> >> +       u32 conn_box;
> >>
> >>         struct reset_control *mss_restart;
> >>         struct reset_control *pdc_reset;
> >> @@ -187,6 +198,7 @@ struct q6v5 {
> >>         struct qcom_sysmon *sysmon;
> >>         bool need_mem_protection;
> >>         bool has_alt_reset;
> >> +       bool has_halt_nav;
> >>         int mpss_perm;
> >>         int mba_perm;
> >>         const char *hexagon_mdt_image;
> >> @@ -198,6 +210,7 @@ enum {
> >>         MSS_MSM8974,
> >>         MSS_MSM8996,
> >>         MSS_MSM8998,
> >> +       MSS_SC7180,
> >>         MSS_SDM845,
> >>  };
> >>
> >> @@ -396,6 +409,18 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
> >>                 reset_control_assert(qproc->pdc_reset);
> >>                 ret = reset_control_reset(qproc->mss_restart);
> >>                 reset_control_deassert(qproc->pdc_reset);
> >> +       } else if (qproc->has_halt_nav) {
> >> +               /* SWAR using CONN_BOX_SPARE_0 for pipeline glitch
> >> issue */
> >
> > Can you elaborate more in this comment, or remove it? Right now it
> > doesn't help me since I don't know what SWAR is, I don't see a
>
> SWAR -> software work around
>
> I'll have to stop with the dumb
> abbreviations

heh I've never heard that one before. SWWA?

>
> > reference to CONN_BOX_SPARE_0 in the code, I don't know what a
>
> conn_box_spare_0 is at an offset
> 0xb3e4 in the conn_map which is
> described in the bindings.
>
> > CONN_BOX is, and I don't know any details about the glitch issue :)
>
> lol, yes I get that the comment does
> not give details on the glitch. It
> was targeted towards explaining why
> there is a deviation in the reset_
> assert sequence from the other SoCs.
> If you still feel the comment does
> not add any value I can get it
> removed.

Maybe just infusing some of the clarity you described here would be enough.

>
> >
> >> +               reset_control_assert(qproc->pdc_reset);
> >> +               regmap_update_bits(qproc->conn_map, qproc->conn_box,
> >> +                                  BIT(0), BIT(0));
> >
> > Make a register name #define for this bit?
>
> we'll have to make one up for it
> since conn_box_spare_0 does not
> have any predefined bits and seems
> to be used to implement some missing
> functionality.

Hm, so its functionality is controlled by firmware? Or was just
defined super late in hardware so as to be termed spare through many
earlier spins of the hardware? Making one up sounds fine, it's just
nice for readability to get a sense of what exactly this bit is when
we toggle it.

>
> >
> >> +               regmap_update_bits(qproc->halt_nav_map,
> >> qproc->halt_nav,
> >> +                                  NAV_AXI_HALTREQ_BIT, 0);
> >> +               reset_control_assert(qproc->mss_restart);
> >> +               reset_control_deassert(qproc->pdc_reset);
> >> +               regmap_update_bits(qproc->conn_map, qproc->conn_box,
> >> +                                  BIT(0), 0);
> >> +               ret = reset_control_deassert(qproc->mss_restart);
> >>         } else {
> >>                 ret = reset_control_assert(qproc->mss_restart);
> >>         }
> >> @@ -413,6 +438,8 @@ static int q6v5_reset_deassert(struct q6v5 *qproc)
> >>                 ret = reset_control_reset(qproc->mss_restart);
> >>                 writel(0, qproc->rmb_base + RMB_MBA_ALT_RESET);
> >>                 reset_control_deassert(qproc->pdc_reset);
> >> +       } else if (qproc->has_halt_nav) {
> >> +               ret = reset_control_reset(qproc->mss_restart);
> >>         } else {
> >>                 ret = reset_control_deassert(qproc->mss_restart);
> >>         }
> >> @@ -499,6 +526,54 @@ static int q6v5proc_reset(struct q6v5 *qproc)
> >>                         return ret;
> >>                 }
> >>
> >> +               goto pbl_wait;
> >
> > Ick, this could benefit from a refactor that pulls those sorta-common
> > steps "Remove IO clamp" through "Start core execution" into a helper
> > function and then calls the new function from each case that needs it.
> >
> >> +       } else if (qproc->version == MSS_SC7180) {
> >> +               val = readl(qproc->reg_base + QDSP6SS_SLEEP);
> >> +               val |= 0x1;
> >
> > It'd be nice if there were defines for these magic 0x1 values in this
> > hunk too.
> >
> >> +               writel(val, qproc->reg_base + QDSP6SS_SLEEP);
> >> +
> >> +               ret = readl_poll_timeout(qproc->reg_base +
> >> QDSP6SS_SLEEP,
> >> +                                        val, !(val & BIT(31)), 1,
> >> +                                        SLEEP_CHECK_MAX_LOOPS);
> >> +               if (ret) {
> >> +                       dev_err(qproc->dev, "QDSP6SS Sleep clock timed
> >> out\n");
> >> +                       return -ETIMEDOUT;
> >> +               }
> >
> > This toggling of the sleep bit is the same as sdm845, it could also be
> > put into a helper function.
>
> https://patchwork.kernel.org/patch/11250301/
> https://patchwork.kernel.org/patch/11250317/
>
> I did club ^^ a portion of the out of
> reset sequence together reducing
> code duplication initially but that
> made the entire thing unreadable.
>
> The general consensus at that time
> was to introduce a patch for sc7180
> that is most readable and have a
> cleanup later with a lot of helper
> functions and less branching like
> you suggested.

Sounds good.

>
> >
> >> +
> >> +               /* Turn on the XO clock needed for PLL setup */
> >> +               val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> >> +               val |= 0x1;
> >> +               writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
> >> +
> >> +               ret = readl_poll_timeout(qproc->reg_base +
> >> QDSP6SS_XO_CBCR,
> >> +                                        val, !(val & BIT(31)), 1,
> >> +                                        SLEEP_CHECK_MAX_LOOPS);
> >
> > Nit: SLEEP_CHECK_MAX_LOOPS isn't a loop count, it should really have
> > been named something like SLEEP_CHECK_TIMEOUT_US. Could be done in a
> > separate change.
>
> I'll skip this for now. I'll
> make sure that this gets named
> appropriately during the
> planned refactor.

Thanks!