LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 0/4] Improve general readability of MSS on SC7180
@ 2020-01-17 13:51 Sibi Sankar
  2020-01-17 13:51 ` [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout Sibi Sankar
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Sibi Sankar @ 2020-01-17 13:51 UTC (permalink / raw)
  To: bjorn.andersson, evgreen, p.zabel
  Cc: ohad, linux-arm-msm, linux-remoteproc, linux-kernel, agross, Sibi Sankar

This series aims to improve the general readability of the mss reset
sequence on SC7180 SoCs. No functional change intended.

Sibi Sankar (4):
  remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout
  remoteproc: qcom: q6v5-mss: Improve readability across clk handling
  remoteproc: qcom: q6v5-mss: Rename boot status timeout
  remoteproc: qcom: q6v5-mss: Improve readability of reset_assert

 drivers/remoteproc/qcom_q6v5_mss.c | 69 +++++++++++++++++-------------
 1 file changed, 40 insertions(+), 29 deletions(-)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout
  2020-01-17 13:51 [PATCH 0/4] Improve general readability of MSS on SC7180 Sibi Sankar
@ 2020-01-17 13:51 ` Sibi Sankar
  2020-01-17 14:19   ` Philipp Zabel
  2020-01-20 19:24   ` Bjorn Andersson
  2020-01-17 13:51 ` [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling Sibi Sankar
                   ` (3 subsequent siblings)
  4 siblings, 2 replies; 13+ messages in thread
From: Sibi Sankar @ 2020-01-17 13:51 UTC (permalink / raw)
  To: bjorn.andersson, evgreen, p.zabel
  Cc: ohad, linux-arm-msm, linux-remoteproc, linux-kernel, agross, Sibi Sankar

Replace the loop for HALT_ACK detection with regmap_read_poll_timeout.

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

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 51f451311f5fc..f20b39c6ff0ed 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -73,6 +73,7 @@
 #define NAV_AXI_IDLE_BIT		BIT(2)
 
 #define HALT_ACK_TIMEOUT_MS		100
+#define NAV_HALT_ACK_TIMEOUT_US		200
 
 /* QDSP6SS_RESET */
 #define Q6SS_STOP_CORE			BIT(0)
@@ -746,7 +747,6 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
 				       struct regmap *halt_map,
 				       u32 offset)
 {
-	unsigned long timeout;
 	unsigned int val;
 	int ret;
 
@@ -760,15 +760,11 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
 			   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_poll_timeout(halt_map, offset, val,
+				       (val & NAV_AXI_HALTACK_BIT),
+				       5, NAV_HALT_ACK_TIMEOUT_US);
+	if (ret)
+		dev_err(qproc->dev, "nav halt ack timeout\n");
 
 	ret = regmap_read(halt_map, offset, &val);
 	if (ret || !(val & NAV_AXI_IDLE_BIT))
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling
  2020-01-17 13:51 [PATCH 0/4] Improve general readability of MSS on SC7180 Sibi Sankar
  2020-01-17 13:51 ` [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout Sibi Sankar
@ 2020-01-17 13:51 ` Sibi Sankar
  2020-01-21 19:22   ` Evan Green
  2020-01-17 13:51 ` [PATCH 3/4] remoteproc: qcom: q6v5-mss: Rename boot status timeout Sibi Sankar
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Sibi Sankar @ 2020-01-17 13:51 UTC (permalink / raw)
  To: bjorn.andersson, evgreen, p.zabel
  Cc: ohad, linux-arm-msm, linux-remoteproc, linux-kernel, agross, Sibi Sankar

Define CLKEN and CLKOFF for improving readability of Q6SS clock
handling.

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

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index f20b39c6ff0ed..e1784446d3da3 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -80,6 +80,11 @@
 #define Q6SS_CORE_ARES			BIT(1)
 #define Q6SS_BUS_ARES_ENABLE		BIT(2)
 
+/* QDSP6SS CBCR */
+#define Q6SS_CBCR_CLKEN			BIT(0)
+#define Q6SS_CBCR_CLKOFF		BIT(31)
+#define Q6SS_CBCR_TIMEOUT_US		200
+
 /* QDSP6SS_GFMUX_CTL */
 #define Q6SS_CLK_ENABLE			BIT(1)
 
@@ -100,7 +105,6 @@
 #define QDSP6v56_BHS_ON		BIT(24)
 #define QDSP6v56_CLAMP_WL		BIT(21)
 #define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
-#define HALT_CHECK_MAX_LOOPS		200
 #define QDSP6SS_XO_CBCR		0x0038
 #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
 
@@ -502,12 +506,12 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 
 	if (qproc->version == MSS_SDM845) {
 		val = readl(qproc->reg_base + QDSP6SS_SLEEP);
-		val |= 0x1;
+		val |= Q6SS_CBCR_CLKEN;
 		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);
+					 val, !(val & Q6SS_CBCR_CLKOFF), 1,
+					 Q6SS_CBCR_TIMEOUT_US);
 		if (ret) {
 			dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
 			return -ETIMEDOUT;
@@ -530,12 +534,12 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 		goto pbl_wait;
 	} else if (qproc->version == MSS_SC7180) {
 		val = readl(qproc->reg_base + QDSP6SS_SLEEP);
-		val |= 0x1;
+		val |= Q6SS_CBCR_CLKEN;
 		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);
+					 val, !(val & Q6SS_CBCR_CLKOFF), 1,
+					 Q6SS_CBCR_TIMEOUT_US);
 		if (ret) {
 			dev_err(qproc->dev, "QDSP6SS Sleep clock timed out\n");
 			return -ETIMEDOUT;
@@ -543,12 +547,12 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 
 		/* Turn on the XO clock needed for PLL setup */
 		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
-		val |= 0x1;
+		val |= Q6SS_CBCR_CLKEN;
 		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);
+					 val, !(val & Q6SS_CBCR_CLKOFF), 1,
+					 Q6SS_CBCR_TIMEOUT_US);
 		if (ret) {
 			dev_err(qproc->dev, "QDSP6SS XO clock timed out\n");
 			return -ETIMEDOUT;
@@ -556,7 +560,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 
 		/* Configure Q6 core CBCR to auto-enable after reset sequence */
 		val = readl(qproc->reg_base + QDSP6SS_CORE_CBCR);
-		val |= 0x1;
+		val |= Q6SS_CBCR_CLKEN;
 		writel(val, qproc->reg_base + QDSP6SS_CORE_CBCR);
 
 		/* De-assert the Q6 stop core signal */
@@ -591,13 +595,13 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 
 		/* BHS require xo cbcr to be enabled */
 		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
-		val |= 0x1;
+		val |= Q6SS_CBCR_CLKEN;
 		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
 
 		/* Read CLKOFF bit to go low indicating CLK is enabled */
 		ret = readl_poll_timeout(qproc->reg_base + QDSP6SS_XO_CBCR,
-					 val, !(val & BIT(31)), 1,
-					 HALT_CHECK_MAX_LOOPS);
+					 val, !(val & Q6SS_CBCR_CLKOFF), 1,
+					 Q6SS_CBCR_TIMEOUT_US);
 		if (ret) {
 			dev_err(qproc->dev,
 				"xo cbcr enabling timed out (rc:%d)\n", ret);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 3/4] remoteproc: qcom: q6v5-mss: Rename boot status timeout
  2020-01-17 13:51 [PATCH 0/4] Improve general readability of MSS on SC7180 Sibi Sankar
  2020-01-17 13:51 ` [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout Sibi Sankar
  2020-01-17 13:51 ` [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling Sibi Sankar
@ 2020-01-17 13:51 ` Sibi Sankar
  2020-01-21 19:22   ` Evan Green
  2020-01-17 13:51 ` [PATCH 4/4] remoteproc: qcom: q6v5-mss: Improve readability of reset_assert Sibi Sankar
  2020-01-20 19:37 ` [PATCH 0/4] Improve general readability of MSS on SC7180 Bjorn Andersson
  4 siblings, 1 reply; 13+ messages in thread
From: Sibi Sankar @ 2020-01-17 13:51 UTC (permalink / raw)
  To: bjorn.andersson, evgreen, p.zabel
  Cc: ohad, linux-arm-msm, linux-remoteproc, linux-kernel, agross, Sibi Sankar

Rename the FSM timeout on SC7180 to BOOT_STATUS_TIMEOUT_US.

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

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index e1784446d3da3..6a98e9029c70b 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -114,7 +114,7 @@
 #define QDSP6SS_BOOT_CORE_START         0x400
 #define QDSP6SS_BOOT_CMD                0x404
 #define QDSP6SS_BOOT_STATUS		0x408
-#define SLEEP_CHECK_MAX_LOOPS           200
+#define BOOT_STATUS_TIMEOUT_US		200
 #define BOOT_FSM_TIMEOUT                10000
 
 struct reg_info {
@@ -572,7 +572,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 		/* 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);
+					 BOOT_STATUS_TIMEOUT_US);
 		if (ret) {
 			dev_err(qproc->dev, "Boot FSM failed to complete.\n");
 			/* Reset the modem so that boot FSM is in reset state */
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 4/4] remoteproc: qcom: q6v5-mss: Improve readability of reset_assert
  2020-01-17 13:51 [PATCH 0/4] Improve general readability of MSS on SC7180 Sibi Sankar
                   ` (2 preceding siblings ...)
  2020-01-17 13:51 ` [PATCH 3/4] remoteproc: qcom: q6v5-mss: Rename boot status timeout Sibi Sankar
@ 2020-01-17 13:51 ` Sibi Sankar
  2020-01-20 19:36   ` Bjorn Andersson
  2020-01-20 19:37 ` [PATCH 0/4] Improve general readability of MSS on SC7180 Bjorn Andersson
  4 siblings, 1 reply; 13+ messages in thread
From: Sibi Sankar @ 2020-01-17 13:51 UTC (permalink / raw)
  To: bjorn.andersson, evgreen, p.zabel
  Cc: ohad, linux-arm-msm, linux-remoteproc, linux-kernel, agross, Sibi Sankar

Define CONN_BOX_SPARE_0_EN and fixup comments to improve readability of
Q6 modem reset_assert sequence on SC7180 SoCs.

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

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 6a98e9029c70b..8c9cfc213d5ff 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -71,6 +71,7 @@
 #define NAV_AXI_HALTREQ_BIT		BIT(0)
 #define NAV_AXI_HALTACK_BIT		BIT(1)
 #define NAV_AXI_IDLE_BIT		BIT(2)
+#define CONN_BOX_SPARE_0_EN		BIT(0)
 
 #define HALT_ACK_TIMEOUT_MS		100
 #define NAV_HALT_ACK_TIMEOUT_US		200
@@ -415,16 +416,26 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
 		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 */
+		/*
+		 * SWWA for the pipeline glitch issue seen while
+		 * putting the Q6 modem on SC7180 into reset:
+		 * 1 - Assert PDC reset
+		 * 2 - Set CONN_BOX_SPARE_0_EN
+		 * 3 - Withdraw the halt requests
+		 * 4 - Assert MSS reset
+		 * 5 - Deassert PDC reset
+		 * 6 - Clear CONN_BOX_SPARE_0_EN
+		 * 7 - Deassert MSS reset
+		 */
 		reset_control_assert(qproc->pdc_reset);
 		regmap_update_bits(qproc->conn_map, qproc->conn_box,
-				   BIT(0), BIT(0));
+				   CONN_BOX_SPARE_0_EN, 1);
 		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);
+				   CONN_BOX_SPARE_0_EN, 0);
 		ret = reset_control_deassert(qproc->mss_restart);
 	} else {
 		ret = reset_control_assert(qproc->mss_restart);
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout
  2020-01-17 13:51 ` [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout Sibi Sankar
@ 2020-01-17 14:19   ` Philipp Zabel
  2020-01-20 19:24   ` Bjorn Andersson
  1 sibling, 0 replies; 13+ messages in thread
From: Philipp Zabel @ 2020-01-17 14:19 UTC (permalink / raw)
  To: Sibi Sankar, bjorn.andersson, evgreen
  Cc: ohad, linux-arm-msm, linux-remoteproc, linux-kernel, agross

On Fri, 2020-01-17 at 19:21 +0530, Sibi Sankar wrote:
> Replace the loop for HALT_ACK detection with regmap_read_poll_timeout.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 51f451311f5fc..f20b39c6ff0ed 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -73,6 +73,7 @@
>  #define NAV_AXI_IDLE_BIT		BIT(2)
>  
>  #define HALT_ACK_TIMEOUT_MS		100
> +#define NAV_HALT_ACK_TIMEOUT_US		200
>  
>  /* QDSP6SS_RESET */
>  #define Q6SS_STOP_CORE			BIT(0)
> @@ -746,7 +747,6 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
>  				       struct regmap *halt_map,
>  				       u32 offset)
>  {
> -	unsigned long timeout;
>  	unsigned int val;
>  	int ret;
>  
> @@ -760,15 +760,11 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
>  			   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_poll_timeout(halt_map, offset, val,
> +				       (val & NAV_AXI_HALTACK_BIT),
> +				       5, NAV_HALT_ACK_TIMEOUT_US);
> +	if (ret)
> +		dev_err(qproc->dev, "nav halt ack timeout\n");
>  
>  	ret = regmap_read(halt_map, offset, &val);
>  	if (ret || !(val & NAV_AXI_IDLE_BIT))

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout
  2020-01-17 13:51 ` [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout Sibi Sankar
  2020-01-17 14:19   ` Philipp Zabel
@ 2020-01-20 19:24   ` Bjorn Andersson
  2020-01-22  6:18     ` Sibi Sankar
  1 sibling, 1 reply; 13+ messages in thread
From: Bjorn Andersson @ 2020-01-20 19:24 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: evgreen, p.zabel, ohad, linux-arm-msm, linux-remoteproc,
	linux-kernel, agross

On Fri 17 Jan 05:51 PST 2020, Sibi Sankar wrote:

> Replace the loop for HALT_ACK detection with regmap_read_poll_timeout.
> 

Nice, but we should be able to do the same in q6v5proc_halt_axi_port()?

> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 16 ++++++----------
>  1 file changed, 6 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 51f451311f5fc..f20b39c6ff0ed 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -73,6 +73,7 @@
>  #define NAV_AXI_IDLE_BIT		BIT(2)
>  
>  #define HALT_ACK_TIMEOUT_MS		100
> +#define NAV_HALT_ACK_TIMEOUT_US		200
>  
>  /* QDSP6SS_RESET */
>  #define Q6SS_STOP_CORE			BIT(0)
> @@ -746,7 +747,6 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
>  				       struct regmap *halt_map,
>  				       u32 offset)
>  {
> -	unsigned long timeout;
>  	unsigned int val;
>  	int ret;
>  
> @@ -760,15 +760,11 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 *qproc,
>  			   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_poll_timeout(halt_map, offset, val,
> +				       (val & NAV_AXI_HALTACK_BIT),
> +				       5, NAV_HALT_ACK_TIMEOUT_US);
> +	if (ret)
> +		dev_err(qproc->dev, "nav halt ack timeout\n");

Is there a case where this new print adds value beyond the printout we
already have for the case of IDLE_BIT not going high? Can we simply
ignore the return value and skip the print?

Regards,
Bjorn

>  
>  	ret = regmap_read(halt_map, offset, &val);
>  	if (ret || !(val & NAV_AXI_IDLE_BIT))
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] remoteproc: qcom: q6v5-mss: Improve readability of reset_assert
  2020-01-17 13:51 ` [PATCH 4/4] remoteproc: qcom: q6v5-mss: Improve readability of reset_assert Sibi Sankar
@ 2020-01-20 19:36   ` Bjorn Andersson
  0 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2020-01-20 19:36 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: evgreen, p.zabel, ohad, linux-arm-msm, linux-remoteproc,
	linux-kernel, agross

On Fri 17 Jan 05:51 PST 2020, Sibi Sankar wrote:

> Define CONN_BOX_SPARE_0_EN and fixup comments to improve readability of
> Q6 modem reset_assert sequence on SC7180 SoCs.
> 
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 6a98e9029c70b..8c9cfc213d5ff 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -71,6 +71,7 @@
>  #define NAV_AXI_HALTREQ_BIT		BIT(0)
>  #define NAV_AXI_HALTACK_BIT		BIT(1)
>  #define NAV_AXI_IDLE_BIT		BIT(2)
> +#define CONN_BOX_SPARE_0_EN		BIT(0)
>  
>  #define HALT_ACK_TIMEOUT_MS		100
>  #define NAV_HALT_ACK_TIMEOUT_US		200
> @@ -415,16 +416,26 @@ static int q6v5_reset_assert(struct q6v5 *qproc)
>  		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 */
> +		/*
> +		 * SWWA for the pipeline glitch issue seen while

Is SWWA an abbreviation for SoftWare WorkAround?

> +		 * putting the Q6 modem on SC7180 into reset:
> +		 * 1 - Assert PDC reset
> +		 * 2 - Set CONN_BOX_SPARE_0_EN
> +		 * 3 - Withdraw the halt requests
> +		 * 4 - Assert MSS reset
> +		 * 5 - Deassert PDC reset
> +		 * 6 - Clear CONN_BOX_SPARE_0_EN
> +		 * 7 - Deassert MSS reset

This pretty much outlines what's written below. How about making this
something like:

/* 
 * Work around a pipeline glitch seen when putting the Q6 modem in
 * SC7180 into reset by also toggling CONN_BOX_SPARE_0_EN, while holding
 * the PDC reset.
 */


Although, it would be even better if it indicated what you mean with
"pipeline glitch"...

Regards,
Bjorn

> +		 */
>  		reset_control_assert(qproc->pdc_reset);
>  		regmap_update_bits(qproc->conn_map, qproc->conn_box,
> -				   BIT(0), BIT(0));
> +				   CONN_BOX_SPARE_0_EN, 1);
>  		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);
> +				   CONN_BOX_SPARE_0_EN, 0);
>  		ret = reset_control_deassert(qproc->mss_restart);
>  	} else {
>  		ret = reset_control_assert(qproc->mss_restart);
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 0/4] Improve general readability of MSS on SC7180
  2020-01-17 13:51 [PATCH 0/4] Improve general readability of MSS on SC7180 Sibi Sankar
                   ` (3 preceding siblings ...)
  2020-01-17 13:51 ` [PATCH 4/4] remoteproc: qcom: q6v5-mss: Improve readability of reset_assert Sibi Sankar
@ 2020-01-20 19:37 ` Bjorn Andersson
  4 siblings, 0 replies; 13+ messages in thread
From: Bjorn Andersson @ 2020-01-20 19:37 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: evgreen, p.zabel, ohad, linux-arm-msm, linux-remoteproc,
	linux-kernel, agross

On Fri 17 Jan 05:51 PST 2020, Sibi Sankar wrote:

> This series aims to improve the general readability of the mss reset
> sequence on SC7180 SoCs. No functional change intended.
> 

Thanks Sibi, I picked up patch 2 and 3 for now.

Regards,
Bjorn

> Sibi Sankar (4):
>   remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout
>   remoteproc: qcom: q6v5-mss: Improve readability across clk handling
>   remoteproc: qcom: q6v5-mss: Rename boot status timeout
>   remoteproc: qcom: q6v5-mss: Improve readability of reset_assert
> 
>  drivers/remoteproc/qcom_q6v5_mss.c | 69 +++++++++++++++++-------------
>  1 file changed, 40 insertions(+), 29 deletions(-)
> 
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling
  2020-01-17 13:51 ` [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling Sibi Sankar
@ 2020-01-21 19:22   ` Evan Green
  2020-01-22  6:38     ` Sibi Sankar
  0 siblings, 1 reply; 13+ messages in thread
From: Evan Green @ 2020-01-21 19:22 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Philipp Zabel, Ohad Ben Cohen, linux-arm-msm,
	linux-remoteproc, LKML, Andy Gross

On Fri, Jan 17, 2020 at 5:51 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Define CLKEN and CLKOFF for improving readability of Q6SS clock
> handling.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

It took me awhile to wrap my head around how this new define,
Q6SS_CBCR_TIMEOUT_US, sometimes replaces HALT_CHECK_MAX_LOOPS and
sometimes replaces SLEEP_CHECK_MAX_LOOPS. I guess they're conceptually
different but set to the same value for now? And you've fixed up a
place where the wrong one was used? If you thought the distinction was
meaningless I'd also be fine merging these two defines into one.
Either way, assuming the above is intentional, this looks ok to me.
Thanks for renaming that define.

Reviewed-by: Evan Green <evgreen@chromium.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 3/4] remoteproc: qcom: q6v5-mss: Rename boot status timeout
  2020-01-17 13:51 ` [PATCH 3/4] remoteproc: qcom: q6v5-mss: Rename boot status timeout Sibi Sankar
@ 2020-01-21 19:22   ` Evan Green
  0 siblings, 0 replies; 13+ messages in thread
From: Evan Green @ 2020-01-21 19:22 UTC (permalink / raw)
  To: Sibi Sankar
  Cc: Bjorn Andersson, Philipp Zabel, Ohad Ben Cohen, linux-arm-msm,
	linux-remoteproc, LKML, Andy Gross

On Fri, Jan 17, 2020 at 5:51 AM Sibi Sankar <sibis@codeaurora.org> wrote:
>
> Rename the FSM timeout on SC7180 to BOOT_STATUS_TIMEOUT_US.
>
> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>

Reviewed-by: Evan Green <evgreen@chromium.org>

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout
  2020-01-20 19:24   ` Bjorn Andersson
@ 2020-01-22  6:18     ` Sibi Sankar
  0 siblings, 0 replies; 13+ messages in thread
From: Sibi Sankar @ 2020-01-22  6:18 UTC (permalink / raw)
  To: Bjorn Andersson
  Cc: evgreen, p.zabel, ohad, linux-arm-msm, linux-remoteproc,
	linux-kernel, agross, linux-arm-msm-owner

Hey Bjorn,

Thanks for the review!

On 2020-01-21 00:54, Bjorn Andersson wrote:
> On Fri 17 Jan 05:51 PST 2020, Sibi Sankar wrote:
> 
>> Replace the loop for HALT_ACK detection with regmap_read_poll_timeout.

sry missed it, will include it
in the next re-spin

>> 
> 
> Nice, but we should be able to do the same in q6v5proc_halt_axi_port()?
> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
>> ---
>>  drivers/remoteproc/qcom_q6v5_mss.c | 16 ++++++----------
>>  1 file changed, 6 insertions(+), 10 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 51f451311f5fc..f20b39c6ff0ed 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -73,6 +73,7 @@
>>  #define NAV_AXI_IDLE_BIT		BIT(2)
>> 
>>  #define HALT_ACK_TIMEOUT_MS		100
>> +#define NAV_HALT_ACK_TIMEOUT_US		200
>> 
>>  /* QDSP6SS_RESET */
>>  #define Q6SS_STOP_CORE			BIT(0)
>> @@ -746,7 +747,6 @@ static void q6v5proc_halt_nav_axi_port(struct q6v5 
>> *qproc,
>>  				       struct regmap *halt_map,
>>  				       u32 offset)
>>  {
>> -	unsigned long timeout;
>>  	unsigned int val;
>>  	int ret;
>> 
>> @@ -760,15 +760,11 @@ static void q6v5proc_halt_nav_axi_port(struct 
>> q6v5 *qproc,
>>  			   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_poll_timeout(halt_map, offset, val,
>> +				       (val & NAV_AXI_HALTACK_BIT),
>> +				       5, NAV_HALT_ACK_TIMEOUT_US);
>> +	if (ret)
>> +		dev_err(qproc->dev, "nav halt ack timeout\n");
> 
> Is there a case where this new print adds value beyond the printout we
> already have for the case of IDLE_BIT not going high? Can we simply
> ignore the return value and skip the print?

yes we can skip the print

> 
> Regards,
> Bjorn
> 
>> 
>>  	ret = regmap_read(halt_map, offset, &val);
>>  	if (ret || !(val & NAV_AXI_IDLE_BIT))
>> --
>> 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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling
  2020-01-21 19:22   ` Evan Green
@ 2020-01-22  6:38     ` Sibi Sankar
  0 siblings, 0 replies; 13+ messages in thread
From: Sibi Sankar @ 2020-01-22  6:38 UTC (permalink / raw)
  To: Evan Green
  Cc: Bjorn Andersson, Philipp Zabel, Ohad Ben Cohen, linux-arm-msm,
	linux-remoteproc, LKML, Andy Gross, linux-remoteproc-owner

Hey Evan,

Thanks for the review!

On 2020-01-22 00:52, Evan Green wrote:
> On Fri, Jan 17, 2020 at 5:51 AM Sibi Sankar <sibis@codeaurora.org> 
> wrote:
>> 
>> Define CLKEN and CLKOFF for improving readability of Q6SS clock
>> handling.
>> 
>> Signed-off-by: Sibi Sankar <sibis@codeaurora.org>
> 
> It took me awhile to wrap my head around how this new define,
> Q6SS_CBCR_TIMEOUT_US, sometimes replaces HALT_CHECK_MAX_LOOPS and
> sometimes replaces SLEEP_CHECK_MAX_LOOPS. I guess they're conceptually
> different but set to the same value for now? And you've fixed up a
> place where the wrong one was used? If you thought the distinction was
> meaningless I'd also be fine merging these two defines into one.

They really aren't that different
both are Clks with the same timeout
the previous naming was just plain
bad.

SLEEP_CHECK_MAX_LOOPS was used
probably because it was referring
to QDSP6SS_SLEEP CBCRs timeout.
HALT_CHECK_MAX_LOOOPS seems to
taken directly from CAF code. So
we should be fine with merging
the two defines into one.

> Either way, assuming the above is intentional, this looks ok to me.
> Thanks for renaming that define.
> 
> Reviewed-by: Evan Green <evgreen@chromium.org>

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-17 13:51 [PATCH 0/4] Improve general readability of MSS on SC7180 Sibi Sankar
2020-01-17 13:51 ` [PATCH 1/4] remoteproc: qcom: q6v5-mss: Use regmap_read_poll_timeout Sibi Sankar
2020-01-17 14:19   ` Philipp Zabel
2020-01-20 19:24   ` Bjorn Andersson
2020-01-22  6:18     ` Sibi Sankar
2020-01-17 13:51 ` [PATCH 2/4] remoteproc: qcom: q6v5-mss: Improve readability across clk handling Sibi Sankar
2020-01-21 19:22   ` Evan Green
2020-01-22  6:38     ` Sibi Sankar
2020-01-17 13:51 ` [PATCH 3/4] remoteproc: qcom: q6v5-mss: Rename boot status timeout Sibi Sankar
2020-01-21 19:22   ` Evan Green
2020-01-17 13:51 ` [PATCH 4/4] remoteproc: qcom: q6v5-mss: Improve readability of reset_assert Sibi Sankar
2020-01-20 19:36   ` Bjorn Andersson
2020-01-20 19:37 ` [PATCH 0/4] Improve general readability of MSS on SC7180 Bjorn Andersson

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lore.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lore.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lore.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lore.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lore.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lore.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lore.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lore.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lore.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git