Skip to contentSkip to conversation

Add Modem support on SC7180 SoCs

PATCH Applied Sibi Sankar 4 participants 9 messages Dec 19, 2019 – Jan 14, 2020

Subsystems (3)
Device tree bindings Arm Qualcomm mailing list Remote processor (remoteproc)
lore
Share & tools
lore

Sibi Sankar

author
#0 lore raw

v2 cover Add Modem support on SC7180 SoCs

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(-)
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#1 lore raw

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

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 <[email protected]>
Signed-off-by Sibi Sankar <[email protected]>
---
 .../bindings/remoteproc/qcom,q6v5.txt         | 23 +++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)
Documentation/devicetree/bindings/remoteproc/qcom,q6v5.txt +21 -2
index c416746f93cfd..88dfa3fc15f78 100644
@@ -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:
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
author
#2 lore raw

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

In reply to 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199 ++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +198 -1
index 164fc2a53ef11..51f451311f5fc 100644
@@ -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},
 	{ },
 };
Signature · 2 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
#3 lore raw

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

Quoted 13 lines · 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199 ++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +0 -0
index 164fc2a53ef11..51f451311f5fc 100644

[...]

Quoted 9 lines
drivers/remoteproc/qcom_q6v5_mss.c +5 -0
@@ -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));

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_reset_assert(struct q6v5 *qproc) · line 426

Quoted 10 lines
drivers/remoteproc/qcom_q6v5_mss.c +7 -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?

[...]

Quoted 8 lines
drivers/remoteproc/qcom_q6v5_mss.c +4 -0
@@ -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)
+{

[...]

drivers/remoteproc/qcom_q6v5_mss.c static void q6v5proc_halt_axi_port(struct q6v5 *qproc, · line 771

Quoted 10 lines
drivers/remoteproc/qcom_q6v5_mss.c +10 -0
+	/* 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

author
#4 lore raw

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

Hey Philipp,

Thanks for taking time to review the patch series.

Quoted 39 lines · Philipp Zabel
Quoted 14 lines · 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199 
++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +0 -0
index 164fc2a53ef11..51f451311f5fc 100644

[...]

Quoted 9 lines
drivers/remoteproc/qcom_q6v5_mss.c +5 -0
@@ -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));

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_reset_assert(struct q6v5 *qproc) · line 426

Quoted 10 lines
drivers/remoteproc/qcom_q6v5_mss.c +7 -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.

Quoted 2 lines

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.

Quoted 12 lines

[...]

Quoted 9 lines
drivers/remoteproc/qcom_q6v5_mss.c +4 -0
@@ -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)
+{

[...]

Quoted 12 lines

drivers/remoteproc/qcom_q6v5_mss.c static void q6v5proc_halt_axi_port(struct q6v5 *qproc, · line 771

Quoted 10 lines
drivers/remoteproc/qcom_q6v5_mss.c +10 -0
+	/* 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.

Quoted 3 lines

regards
Philipp

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
#5 lore raw

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

In reply to Sibi Sankar
Quoted 76 lines · 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199 ++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +13 -0
index 164fc2a53ef11..51f451311f5fc 100644
@@ -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,
 };

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_reset_assert(struct q6v5 *qproc) · line 413

Quoted 5 lines
drivers/remoteproc/qcom_q6v5_mss.c +2 -0
@@ -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 :)

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_reset_assert(struct q6v5 *qproc) · line 416

Quoted 3 lines
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+               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?

Quoted 20 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -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);
        }

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 529

Quoted 4 lines
drivers/remoteproc/qcom_q6v5_mss.c +1 -0
@@ -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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 532

Quoted 3 lines
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+       } 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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 541

Quoted 9 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -0
+               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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 550

Quoted 9 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -0
+
+               /* 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.

Quoted 247 lines
drivers/remoteproc/qcom_q6v5_mss.c +149 -2
+               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

author
#6 lore raw

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

Hey Evan,

Thanks for the review!
sry for the delayed response

Quoted 77 lines · Evan Green
Quoted 76 lines · 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199 
++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +13 -0
index 164fc2a53ef11..51f451311f5fc 100644
@@ -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,
 };
Quoted 10 lines
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_mss.c +2 -0
@@ -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

Quoted 1 line

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.

Quoted 1 line

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.

Quoted 6 lines

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5_reset_assert(struct q6v5 *qproc) · line 416

Quoted 4 lines
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+               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.

Quoted 49 lines
Quoted 22 lines

+ regmap_update_bits(qproc->halt_nav_map,
qproc->halt_nav,

drivers/remoteproc/qcom_q6v5_mss.c +8 -0
+                                  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);
        }

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 529

Quoted 4 lines
drivers/remoteproc/qcom_q6v5_mss.c +1 -0
@@ -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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 532

Quoted 3 lines
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+       } 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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 541

Quoted 11 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -0
+               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;
+               }
Quoted 2 lines

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.

Quoted 15 lines

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 550

Quoted 11 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -0
+
+               /* 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.

Quoted 282 lines
Quoted 282 lines
drivers/remoteproc/qcom_q6v5_mss.c +2 -0
+               if (ret) {
+                       dev_err(qproc->dev, "QDSP6SS XO clock timed

out\n");

drivers/remoteproc/qcom_q6v5_mss.c +4 -0
+                       return -ETIMEDOUT;
+               }
+
+               /* Configure Q6 core CBCR to auto-enable after reset

sequence */

drivers/remoteproc/qcom_q6v5_mss.c +8 -0
+               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 */

drivers/remoteproc/qcom_q6v5_mss.c +4 -0
+               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,

drivers/remoteproc/qcom_q6v5_mss.c +4 -0
+                                        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 */

drivers/remoteproc/qcom_q6v5_mss.c +36 -0
+                       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;
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
@@ -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:

drivers/remoteproc/qcom_q6v5_mss.c +3 -0
@@ -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) {
                /*
drivers/remoteproc/qcom_q6v5_mss.c +8 -0
@@ -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");

drivers/remoteproc/qcom_q6v5_mss.c +7 -0
+                       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");

drivers/remoteproc/qcom_q6v5_mss.c +4 -0
+                       return -EPROBE_DEFER;
+               }
+
+               qproc->halt_nav_map = dev_get_regmap(&nav_pdev->dev,

NULL);

drivers/remoteproc/qcom_q6v5_mss.c +2 -0
+               if (!qproc->halt_nav_map) {
+                       dev_err(&pdev->dev, "failed to get map from

device\n");

drivers/remoteproc/qcom_q6v5_mss.c +5 -0
+                       return -EINVAL;
+               }
+               qproc->halt_nav = args.args[0];
+
+               ret =

of_parse_phandle_with_fixed_args(pdev->dev.of_node, +
"qcom,halt-nav-regs",

drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+                                                      1, 1, &args);
+               if (ret < 0) {
+                       dev_err(&pdev->dev, "failed to parse

halt-nav-regs\n");

drivers/remoteproc/qcom_q6v5_mss.c +12 -1
+                       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)) {
drivers/remoteproc/qcom_q6v5_mss.c +1 -0
@@ -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;
drivers/remoteproc/qcom_q6v5_mss.c +35 -0
@@ -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*[]){
drivers/remoteproc/qcom_q6v5_mss.c +6 -0
@@ -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

Signature · 2 lines
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project.
#7 lore raw

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

Quoted 97 lines · Sibi Sankar

Hey Evan,

Thanks for the review!
sry for the delayed response

Quoted 79 lines · Evan Green
Quoted 77 lines · 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 <[email protected]>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 199
++++++++++++++++++++++++++++-
 1 file changed, 198 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +13 -0
index 164fc2a53ef11..51f451311f5fc 100644
@@ -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,
 };
Quoted 10 lines
Quoted 7 lines
drivers/remoteproc/qcom_q6v5_mss.c +2 -0
@@ -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

Quoted 4 lines

SWAR -> software work around

I'll have to stop with the dumb abbreviations

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

Quoted 9 lines
Quoted 2 lines

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.

Quoted 1 line

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

Quoted 8 lines

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.

Quoted 6 lines
Quoted 6 lines
Quoted 5 lines
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+               reset_control_assert(qproc->pdc_reset);
+               regmap_update_bits(qproc->conn_map, qproc->conn_box,
+                                  BIT(0), BIT(0));
Quoted 7 lines
Quoted 1 line

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.

Quoted 62 lines
Quoted 51 lines
Quoted 23 lines

+ regmap_update_bits(qproc->halt_nav_map,
qproc->halt_nav,

drivers/remoteproc/qcom_q6v5_mss.c +8 -0
+                                  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);
        }

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 529

Quoted 4 lines
drivers/remoteproc/qcom_q6v5_mss.c +1 -0
@@ -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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 532

Quoted 3 lines
drivers/remoteproc/qcom_q6v5_mss.c +3 -0
+       } 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.

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 541

Quoted 11 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -0
+               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;
+               }
Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 17 lines
Quoted 16 lines

drivers/remoteproc/qcom_q6v5_mss.c static int q6v5proc_reset(struct q6v5 *qproc) · line 550

Quoted 12 lines
drivers/remoteproc/qcom_q6v5_mss.c +9 -0
+
+               /* 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.

Quoted 4 lines

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

Thanks!

CI Applied
#8 lore raw
In reply to Sibi Sankar
This series was applied to andersson/remoteproc.git (refs/heads/for-next).

Hello:

This series was applied to andersson/remoteproc.git (refs/heads/for-next).

Quoted 9 lines · On Thu, 19 Dec 2019 11:15:04 +0530 you wrote:

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

[...]

Here is a summary with links:
- [v2,1/2] dt-bindings: remoteproc: qcom: Add Q6V5 Modem PIL binding for SC7180

    https://git.kernel.org/andersson/remoteproc/c/65518e9cf5992f88eb43a3183dd82a6e55ae7f11
  - [v2,2/2] remoteproc: mss: q6v5-mss: Add modem support on SC7180
    https://git.kernel.org/andersson/remoteproc/c/6439b5276b9fda037698ad2e26ad18c9528154b4

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.wiki.kernel.org/userdoc/pwbot