diff mbox series

[3/4] remoteproc: qcom: q6v5-mss: Add support for SDM630/636/660 MSS remoteproc

Message ID 20220514000108.3070363-4-dmitry.baryshkov@linaro.org (mailing list archive)
State New
Headers show
Series remoteproc: qcom: Add support for CDSP and MSS on SDM630/660 | expand

Commit Message

Dmitry Baryshkov May 14, 2022, 12:01 a.m. UTC
From: Konrad Dybcio <konradybcio@gmail.com>

This adds support for sdm630/636/660 modem subsystem
remote processor.

Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
[DB: fixed commit message, removed note about modem restarting regularly]
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 111 +++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

Comments

Stephan Gerhold July 3, 2022, 10:13 a.m. UTC | #1
On Sat, May 14, 2022 at 03:01:07AM +0300, Dmitry Baryshkov wrote:
> From: Konrad Dybcio <konradybcio@gmail.com>
> 
> This adds support for sdm630/636/660 modem subsystem
> remote processor.
> 
> Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
> [DB: fixed commit message, removed note about modem restarting regularly]
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

I was only looking at this by coincidence recently but since it doesn't
seem to be applied yet(?) some comments below.

> ---
>  drivers/remoteproc/qcom_q6v5_mss.c | 111 +++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index af217de75e4d..7a4cca30db8a 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
[...]
> @@ -754,6 +759,79 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>  			val |= readl(qproc->reg_base + mem_pwr_ctl);
>  			udelay(1);
>  		}
> +		/* Remove word line clamp */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_WL;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +	} else if (qproc->version == MSS_SDM660) {
> +		int mem_pwr_ctl;
> +
> +		/* Override the ACC value if required */
> +		writel(QDSP6SS_ACC_OVERRIDE_VAL,
> +		       qproc->reg_base + QDSP6SS_STRAP_ACC);
> +
> +		/* Assert resets, stop core */
> +		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
> +		val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
> +		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
> +
> +		/* BHS require xo cbcr to be enabled */
> +		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
> +		val |= 1;

val |= Q6SS_CBCR_CLKEN; (like in the existing 8996 code)

> +		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);

I would also expect this to wait for !Q6SS_CBCR_CLKOFF like on all the
other SoCs. Pretty sure downstream does that for all of them in
q6v55_branch_clk_enable().

> +
> +		/* Enable power block headswitch and wait for it to stabilize */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val |= QDSP6v56_BHS_ON;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		mb();
> +		udelay(1);
> +
> +		for (i = BHS_CHECK_MAX_LOOPS; i > 0; i--) {
> +			if (readl_relaxed(qproc->reg_base + QDSP6V62SS_BHS_STATUS)
> +			    & QDSP6v55_BHS_EN_REST_ACK)
> +				break;
> +			udelay(1);
> +		}

This looks like readl_poll_timeout().

> +		if (!i) {
> +			pr_err("%s: BHS_EN_REST_ACK not set!\n", __func__);

dev_err()

> +			return -ETIMEDOUT;
> +		}
> +
> +		/* Put LDO in bypass mode */
> +		val |= QDSP6v56_LDO_BYP;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +		/* Remove QMC_MEM clamp */
> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +		/* Deassert QDSP6 compiler memory clamp */
> +		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +		val &= ~QDSP6v56_CLAMP_QMC_MEM;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +

Why does this clear QDSP6v56_CLAMP_QMC_MEM twice?

> +		/* Deassert memory peripheral sleep and L2 memory standby */
> +		val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
> +		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> +
> +		/* Turn on L1, L2, ETB and JU memories 1 at a time */
> +		mem_pwr_ctl = QDSP6V6SS_MEM_PWR_CTL;
> +		i = 29;
> +
> +		val = readl(qproc->reg_base + mem_pwr_ctl);
> +		for (; i >= 0; i--) {
> +			val |= BIT(i);
> +			writel(val, qproc->reg_base + mem_pwr_ctl);
> +			/*
> +			 * Read back value to ensure the write is done then
> +			 * wait for 1us for both memory peripheral and data
> +			 * array to turn on.
> +			 */
> +			val |= readl(qproc->reg_base + mem_pwr_ctl);
> +			udelay(1);
> +		}
> +
>  		/* Remove word line clamp */
>  		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>  		val &= ~QDSP6v56_CLAMP_WL;

All in all this looks almost exactly like the existing code for
MSS_MSM8996/8. Wouldn't it be cleaner to just add an if statement for
the QDSP6V62SS_BHS_STATUS readl_poll_timeout() to the existing code?

Thanks,
Stephan
Dmitry Baryshkov July 3, 2022, 11 a.m. UTC | #2
On 3 July 2022 13:13:51 GMT+03:00, Stephan Gerhold <stephan@gerhold.net> wrote:
>On Sat, May 14, 2022 at 03:01:07AM +0300, Dmitry Baryshkov wrote:
>> From: Konrad Dybcio <konradybcio@gmail.com>
>> 
>> This adds support for sdm630/636/660 modem subsystem
>> remote processor.
>> 
>> Signed-off-by: Konrad Dybcio <konradybcio@gmail.com>
>> [DB: fixed commit message, removed note about modem restarting regularly]
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>
>I was only looking at this by coincidence recently but since it doesn't
>seem to be applied yet(?) some comments below.

Thanks for your comments, I will take a look next week.

>
>> ---
>>  drivers/remoteproc/qcom_q6v5_mss.c | 111 +++++++++++++++++++++++++++++
>>  1 file changed, 111 insertions(+)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index af217de75e4d..7a4cca30db8a 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>[...]
>> +		}
>> +
>>  		/* Remove word line clamp */
>>  		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>>  		val &= ~QDSP6v56_CLAMP_WL;
>
>All in all this looks almost exactly like the existing code for
>MSS_MSM8996/8. Wouldn't it be cleaner to just add an if statement for
>the QDSP6V62SS_BHS_STATUS readl_poll_timeout() to the existing code?

For quite some time I was unhappy with this part of the mss driver. Maybe it's time to review platform-specific code in attempt to generalise it.
diff mbox series

Patch

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index af217de75e4d..7a4cca30db8a 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -68,6 +68,7 @@ 
 #define QDSP6SS_MEM_PWR_CTL		0x0B0
 #define QDSP6V6SS_MEM_PWR_CTL		0x034
 #define QDSP6SS_STRAP_ACC		0x110
+#define QDSP6V62SS_BHS_STATUS		0x0C4
 
 /* AXI Halt Register Offsets */
 #define AXI_HALTREQ_REG			0x0
@@ -117,6 +118,9 @@ 
 #define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
 #define QDSP6SS_XO_CBCR		0x0038
 #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
+#define QDSP6v55_BHS_EN_REST_ACK        BIT(0)
+
+#define BHS_CHECK_MAX_LOOPS             (200)
 
 /* QDSP6v65 parameters */
 #define QDSP6SS_CORE_CBCR		0x20
@@ -239,6 +243,7 @@  enum {
 	MSS_MSM8998,
 	MSS_SC7180,
 	MSS_SC7280,
+	MSS_SDM660,
 	MSS_SDM845,
 };
 
@@ -754,6 +759,79 @@  static int q6v5proc_reset(struct q6v5 *qproc)
 			val |= readl(qproc->reg_base + mem_pwr_ctl);
 			udelay(1);
 		}
+		/* Remove word line clamp */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_WL;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+	} else if (qproc->version == MSS_SDM660) {
+		int mem_pwr_ctl;
+
+		/* Override the ACC value if required */
+		writel(QDSP6SS_ACC_OVERRIDE_VAL,
+		       qproc->reg_base + QDSP6SS_STRAP_ACC);
+
+		/* Assert resets, stop core */
+		val = readl(qproc->reg_base + QDSP6SS_RESET_REG);
+		val |= Q6SS_CORE_ARES | Q6SS_BUS_ARES_ENABLE | Q6SS_STOP_CORE;
+		writel(val, qproc->reg_base + QDSP6SS_RESET_REG);
+
+		/* BHS require xo cbcr to be enabled */
+		val = readl(qproc->reg_base + QDSP6SS_XO_CBCR);
+		val |= 1;
+		writel(val, qproc->reg_base + QDSP6SS_XO_CBCR);
+
+		/* Enable power block headswitch and wait for it to stabilize */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val |= QDSP6v56_BHS_ON;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		mb();
+		udelay(1);
+
+		for (i = BHS_CHECK_MAX_LOOPS; i > 0; i--) {
+			if (readl_relaxed(qproc->reg_base + QDSP6V62SS_BHS_STATUS)
+			    & QDSP6v55_BHS_EN_REST_ACK)
+				break;
+			udelay(1);
+		}
+		if (!i) {
+			pr_err("%s: BHS_EN_REST_ACK not set!\n", __func__);
+			return -ETIMEDOUT;
+		}
+
+		/* Put LDO in bypass mode */
+		val |= QDSP6v56_LDO_BYP;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Remove QMC_MEM clamp */
+		val &= ~QDSP6v56_CLAMP_QMC_MEM;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Deassert QDSP6 compiler memory clamp */
+		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+		val &= ~QDSP6v56_CLAMP_QMC_MEM;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Deassert memory peripheral sleep and L2 memory standby */
+		val |= Q6SS_L2DATA_STBY_N | Q6SS_SLP_RET_N;
+		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
+
+		/* Turn on L1, L2, ETB and JU memories 1 at a time */
+		mem_pwr_ctl = QDSP6V6SS_MEM_PWR_CTL;
+		i = 29;
+
+		val = readl(qproc->reg_base + mem_pwr_ctl);
+		for (; i >= 0; i--) {
+			val |= BIT(i);
+			writel(val, qproc->reg_base + mem_pwr_ctl);
+			/*
+			 * Read back value to ensure the write is done then
+			 * wait for 1us for both memory peripheral and data
+			 * array to turn on.
+			 */
+			val |= readl(qproc->reg_base + mem_pwr_ctl);
+			udelay(1);
+		}
+
 		/* Remove word line clamp */
 		val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
 		val &= ~QDSP6v56_CLAMP_WL;
@@ -785,6 +863,7 @@  static int q6v5proc_reset(struct q6v5 *qproc)
 		val |= Q6SS_L2DATA_SLP_NRET_N_0;
 		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
 	}
+
 	/* Remove IO clamp */
 	val &= ~Q6SS_CLAMP_IO;
 	writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -2098,6 +2177,37 @@  static const struct rproc_hexagon_res sc7280_mss = {
 	.version = MSS_SC7280,
 };
 
+static const struct rproc_hexagon_res sdm660_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_clk_names = (char*[]){
+			"xo",
+			"qdss",
+			NULL
+	},
+	.active_clk_names = (char*[]){
+			"iface",
+			"bus",
+			"gpll0_mss",
+			"mnoc_axi",
+			"snoc_axi",
+			"mem",
+			NULL
+	},
+	.proxy_pd_names = (char*[]){
+			"cx",
+			"mx",
+			NULL
+	},
+	.need_mem_protection = true,
+	.has_alt_reset = false,
+	.has_mba_logs = false,
+	.has_spare_reg = false,
+	.has_qaccept_regs = false,
+	.has_ext_cntl_regs = false,
+	.has_vq6 = false,
+	.version = MSS_SDM660,
+};
+
 static const struct rproc_hexagon_res sdm845_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_clk_names = (char*[]){
@@ -2304,6 +2414,7 @@  static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,msm8998-mss-pil", .data = &msm8998_mss},
 	{ .compatible = "qcom,sc7180-mss-pil", .data = &sc7180_mss},
 	{ .compatible = "qcom,sc7280-mss-pil", .data = &sc7280_mss},
+	{ .compatible = "qcom,sdm660-mss-pil", .data = &sdm660_mss},
 	{ .compatible = "qcom,sdm845-mss-pil", .data = &sdm845_mss},
 	{ },
 };