From: Stephan Gerhold <stephan@gerhold.net>

Add support for MDM9607 MSS it have different ACC settings
and it needs mitigation for inrush current issue.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
[Reword the commit, add necessary flags, rework inrush current mitigation]
Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
---
 drivers/remoteproc/qcom_q6v5_mss.c | 89 ++++++++++++++++++++++++++++++++------
 1 file changed, 75 insertions(+), 14 deletions(-)

diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
index 3c404118b322..19863c576d72 100644
--- a/drivers/remoteproc/qcom_q6v5_mss.c
+++ b/drivers/remoteproc/qcom_q6v5_mss.c
@@ -124,6 +124,7 @@
 #define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
 #define QDSP6SS_XO_CBCR		0x0038
 #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
+#define QDSP6SS_ACC_OVERRIDE_VAL_9607	0x80800000
 #define QDSP6v55_BHS_EN_REST_ACK	BIT(0)
 
 /* QDSP6v65 parameters */
@@ -256,6 +257,7 @@ struct q6v5 {
 };
 
 enum {
+	MSS_MDM9607,
 	MSS_MSM8226,
 	MSS_MSM8909,
 	MSS_MSM8916,
@@ -747,15 +749,19 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 			return ret;
 		}
 		goto pbl_wait;
-	} else if (qproc->version == MSS_MSM8909 ||
+	} else if (qproc->version == MSS_MDM9607 ||
+		   qproc->version == MSS_MSM8909 ||
 		   qproc->version == MSS_MSM8953 ||
 		   qproc->version == MSS_MSM8996 ||
 		   qproc->version == MSS_MSM8998 ||
 		   qproc->version == MSS_SDM660) {
 
-		if (qproc->version != MSS_MSM8909 &&
-		    qproc->version != MSS_MSM8953)
-			/* Override the ACC value if required */
+		/* Override the ACC value if required */
+		if (qproc->version == MSS_MDM9607)
+			writel(QDSP6SS_ACC_OVERRIDE_VAL_9607,
+			       qproc->reg_base + QDSP6SS_STRAP_ACC);
+		else if (qproc->version != MSS_MSM8909 &&
+			 qproc->version != MSS_MSM8953)
 			writel(QDSP6SS_ACC_OVERRIDE_VAL,
 			       qproc->reg_base + QDSP6SS_STRAP_ACC);
 
@@ -800,7 +806,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
 
 		if (qproc->version != MSS_MSM8909) {
-			int mem_pwr_ctl;
+			int mem_pwr_ctl, reverse = 0;
 
 			/* Deassert QDSP6 compiler memory clamp */
 			val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
@@ -812,7 +818,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 			writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
 
 			/* Turn on L1, L2, ETB and JU memories 1 at a time */
-			if (qproc->version == MSS_MSM8953 ||
+			if (qproc->version == MSS_MDM9607 ||
+			    qproc->version == MSS_MSM8953 ||
 			    qproc->version == MSS_MSM8996) {
 				mem_pwr_ctl = QDSP6SS_MEM_PWR_CTL;
 				i = 19;
@@ -822,16 +829,34 @@ static int q6v5proc_reset(struct q6v5 *qproc)
 				i = 28;
 			}
 			val = readl(qproc->reg_base + mem_pwr_ctl);
-			for (; i >= 0; i--) {
-				val |= BIT(i);
-				writel(val, qproc->reg_base + mem_pwr_ctl);
+			if (qproc->version == MSS_MDM9607) {
 				/*
-				 * Read back value to ensure the write is done then
-				 * wait for 1us for both memory peripheral and data
-				 * array to turn on.
+				 * Set first 5 bits in reverse to avoid
+				 * "inrush current" issues.
 				 */
-				val |= readl(qproc->reg_base + mem_pwr_ctl);
-				udelay(1);
+				reverse = 6;
+				for (; i >= reverse; i--) {
+					val |= BIT(i);
+					writel(val, qproc->reg_base + mem_pwr_ctl);
+					udelay(1);
+				}
+				for (i = 0; i < reverse; i++) {
+					val |= BIT(i);
+					writel(val, qproc->reg_base + mem_pwr_ctl);
+					udelay(1);
+				}
+			} else {
+				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);
+				}
 			}
 		} else {
 			/* Turn on memories */
@@ -2410,6 +2435,41 @@ static const struct rproc_hexagon_res msm8996_mss = {
 	.version = MSS_MSM8996,
 };
 
+static const struct rproc_hexagon_res mdm9607_mss = {
+	.hexagon_mba_image = "mba.mbn",
+	.proxy_supply = (struct qcom_mss_reg_res[]) {
+		{
+			.supply = "pll",
+			.uA = 100000,
+		},
+		{}
+	},
+	.proxy_clk_names = (char*[]){
+		"xo",
+		NULL
+	},
+	.active_clk_names = (char*[]){
+		"iface",
+		"bus",
+		"mem",
+		NULL
+	},
+	.proxy_pd_names = (char*[]){
+		"mx",
+		"cx",
+		NULL
+	},
+	.need_mem_protection = false,
+	.has_alt_reset = false,
+	.has_mba_logs = false,
+	.has_spare_reg = false,
+	.has_qaccept_regs = false,
+	.has_ext_bhs_reg = false,
+	.has_ext_cntl_regs = false,
+	.has_vq6 = false,
+	.version = MSS_MDM9607,
+};
+
 static const struct rproc_hexagon_res msm8909_mss = {
 	.hexagon_mba_image = "mba.mbn",
 	.proxy_supply = (struct qcom_mss_reg_res[]) {
@@ -2672,6 +2732,7 @@ static const struct rproc_hexagon_res msm8926_mss = {
 
 static const struct of_device_id q6v5_of_match[] = {
 	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
+	{ .compatible = "qcom,mdm9607-mss-pil", .data = &mdm9607_mss},
 	{ .compatible = "qcom,msm8226-mss-pil", .data = &msm8226_mss},
 	{ .compatible = "qcom,msm8909-mss-pil", .data = &msm8909_mss},
 	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},

-- 
2.52.0

On 31/12/2025 16:30, Barnabás Czémán wrote:
> From: Stephan Gerhold <stephan@gerhold.net>
> 
> Add support for MDM9607 MSS it have different ACC settings
> and it needs mitigation for inrush current issue.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> [Reword the commit, add necessary flags, rework inrush current mitigation]
> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
> ---
>   drivers/remoteproc/qcom_q6v5_mss.c | 89 ++++++++++++++++++++++++++++++++------
>   1 file changed, 75 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
> index 3c404118b322..19863c576d72 100644
> --- a/drivers/remoteproc/qcom_q6v5_mss.c
> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
> @@ -124,6 +124,7 @@
>   #define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
>   #define QDSP6SS_XO_CBCR		0x0038
>   #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
> +#define QDSP6SS_ACC_OVERRIDE_VAL_9607	0x80800000
>   #define QDSP6v55_BHS_EN_REST_ACK	BIT(0)
> 
>   /* QDSP6v65 parameters */
> @@ -256,6 +257,7 @@ struct q6v5 {
>   };
> 
>   enum {
> +	MSS_MDM9607,
>   	MSS_MSM8226,
>   	MSS_MSM8909,
>   	MSS_MSM8916,
> @@ -747,15 +749,19 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>   			return ret;
>   		}
>   		goto pbl_wait;
> -	} else if (qproc->version == MSS_MSM8909 ||
> +	} else if (qproc->version == MSS_MDM9607 ||
> +		   qproc->version == MSS_MSM8909 ||
>   		   qproc->version == MSS_MSM8953 ||
>   		   qproc->version == MSS_MSM8996 ||
>   		   qproc->version == MSS_MSM8998 ||
>   		   qproc->version == MSS_SDM660) {
> 
> -		if (qproc->version != MSS_MSM8909 &&
> -		    qproc->version != MSS_MSM8953)
> -			/* Override the ACC value if required */
> +		/* Override the ACC value if required */
> +		if (qproc->version == MSS_MDM9607)
> +			writel(QDSP6SS_ACC_OVERRIDE_VAL_9607,
> +			       qproc->reg_base + QDSP6SS_STRAP_ACC);
> +		else if (qproc->version != MSS_MSM8909 &&
> +			 qproc->version != MSS_MSM8953)
>   			writel(QDSP6SS_ACC_OVERRIDE_VAL,
>   			       qproc->reg_base + QDSP6SS_STRAP_ACC);
> 
> @@ -800,7 +806,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>   		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> 
>   		if (qproc->version != MSS_MSM8909) {
> -			int mem_pwr_ctl;
> +			int mem_pwr_ctl, reverse = 0;
> 
>   			/* Deassert QDSP6 compiler memory clamp */
>   			val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> @@ -812,7 +818,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>   			writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
> 
>   			/* Turn on L1, L2, ETB and JU memories 1 at a time */
> -			if (qproc->version == MSS_MSM8953 ||
> +			if (qproc->version == MSS_MDM9607 ||
> +			    qproc->version == MSS_MSM8953 ||
>   			    qproc->version == MSS_MSM8996) {
>   				mem_pwr_ctl = QDSP6SS_MEM_PWR_CTL;
>   				i = 19;
> @@ -822,16 +829,34 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>   				i = 28;
>   			}
>   			val = readl(qproc->reg_base + mem_pwr_ctl);
> -			for (; i >= 0; i--) {
> -				val |= BIT(i);
> -				writel(val, qproc->reg_base + mem_pwr_ctl);
> +			if (qproc->version == MSS_MDM9607) {
>   				/*
> -				 * Read back value to ensure the write is done then
> -				 * wait for 1us for both memory peripheral and data
> -				 * array to turn on.
> +				 * Set first 5 bits in reverse to avoid
> +				 * "inrush current" issues.
>   				 */
> -				val |= readl(qproc->reg_base + mem_pwr_ctl);
> -				udelay(1);
> +				reverse = 6;
> +				for (; i >= reverse; i--) {
> +					val |= BIT(i);
> +					writel(val, qproc->reg_base + mem_pwr_ctl);
> +					udelay(1);
> +				}
> +				for (i = 0; i < reverse; i++) {
> +					val |= BIT(i);
> +					writel(val, qproc->reg_base + mem_pwr_ctl);
> +					udelay(1);
> +				}
> +			} else {
> +				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);

Isn't the logic here inverted ?

i.e. you've written a thing and ostensibly require a delay for that 
thing to take effect, the power to switch on in this case.

It makes more sense to write, delay and read back rather than write, 
readback and delay surely...


> +				}
>   			}
>   		} else {
>   			/* Turn on memories */
> @@ -2410,6 +2435,41 @@ static const struct rproc_hexagon_res msm8996_mss = {
>   	.version = MSS_MSM8996,
>   };
> 
> +static const struct rproc_hexagon_res mdm9607_mss = {
> +	.hexagon_mba_image = "mba.mbn",
> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
> +		{
> +			.supply = "pll",
> +			.uA = 100000,
> +		},
> +		{}
> +	},
> +	.proxy_clk_names = (char*[]){
> +		"xo",
> +		NULL
> +	},
> +	.active_clk_names = (char*[]){
> +		"iface",
> +		"bus",
> +		"mem",
> +		NULL
> +	},
> +	.proxy_pd_names = (char*[]){
> +		"mx",
> +		"cx",
> +		NULL
> +	},
> +	.need_mem_protection = false,
> +	.has_alt_reset = false,
> +	.has_mba_logs = false,
> +	.has_spare_reg = false,
> +	.has_qaccept_regs = false,
> +	.has_ext_bhs_reg = false,
> +	.has_ext_cntl_regs = false,
> +	.has_vq6 = false,
> +	.version = MSS_MDM9607,
> +};
> +
>   static const struct rproc_hexagon_res msm8909_mss = {
>   	.hexagon_mba_image = "mba.mbn",
>   	.proxy_supply = (struct qcom_mss_reg_res[]) {
> @@ -2672,6 +2732,7 @@ static const struct rproc_hexagon_res msm8926_mss = {
> 
>   static const struct of_device_id q6v5_of_match[] = {
>   	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
> +	{ .compatible = "qcom,mdm9607-mss-pil", .data = &mdm9607_mss},
>   	{ .compatible = "qcom,msm8226-mss-pil", .data = &msm8226_mss},
>   	{ .compatible = "qcom,msm8909-mss-pil", .data = &msm8909_mss},
>   	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
> 
> --
> 2.52.0
> 
> 

On 2026-01-01 14:13, Bryan O'Donoghue wrote:
> On 31/12/2025 16:30, Barnabás Czémán wrote:
>> From: Stephan Gerhold <stephan@gerhold.net>
>> 
>> Add support for MDM9607 MSS it have different ACC settings
>> and it needs mitigation for inrush current issue.
>> 
>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>> [Reword the commit, add necessary flags, rework inrush current 
>> mitigation]
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>>   drivers/remoteproc/qcom_q6v5_mss.c | 89 
>> ++++++++++++++++++++++++++++++++------
>>   1 file changed, 75 insertions(+), 14 deletions(-)
>> 
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c 
>> b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 3c404118b322..19863c576d72 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -124,6 +124,7 @@
>>   #define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
>>   #define QDSP6SS_XO_CBCR		0x0038
>>   #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
>> +#define QDSP6SS_ACC_OVERRIDE_VAL_9607	0x80800000
>>   #define QDSP6v55_BHS_EN_REST_ACK	BIT(0)
>> 
>>   /* QDSP6v65 parameters */
>> @@ -256,6 +257,7 @@ struct q6v5 {
>>   };
>> 
>>   enum {
>> +	MSS_MDM9607,
>>   	MSS_MSM8226,
>>   	MSS_MSM8909,
>>   	MSS_MSM8916,
>> @@ -747,15 +749,19 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   			return ret;
>>   		}
>>   		goto pbl_wait;
>> -	} else if (qproc->version == MSS_MSM8909 ||
>> +	} else if (qproc->version == MSS_MDM9607 ||
>> +		   qproc->version == MSS_MSM8909 ||
>>   		   qproc->version == MSS_MSM8953 ||
>>   		   qproc->version == MSS_MSM8996 ||
>>   		   qproc->version == MSS_MSM8998 ||
>>   		   qproc->version == MSS_SDM660) {
>> 
>> -		if (qproc->version != MSS_MSM8909 &&
>> -		    qproc->version != MSS_MSM8953)
>> -			/* Override the ACC value if required */
>> +		/* Override the ACC value if required */
>> +		if (qproc->version == MSS_MDM9607)
>> +			writel(QDSP6SS_ACC_OVERRIDE_VAL_9607,
>> +			       qproc->reg_base + QDSP6SS_STRAP_ACC);
>> +		else if (qproc->version != MSS_MSM8909 &&
>> +			 qproc->version != MSS_MSM8953)
>>   			writel(QDSP6SS_ACC_OVERRIDE_VAL,
>>   			       qproc->reg_base + QDSP6SS_STRAP_ACC);
>> 
>> @@ -800,7 +806,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> 
>>   		if (qproc->version != MSS_MSM8909) {
>> -			int mem_pwr_ctl;
>> +			int mem_pwr_ctl, reverse = 0;
>> 
>>   			/* Deassert QDSP6 compiler memory clamp */
>>   			val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> @@ -812,7 +818,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   			writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> 
>>   			/* Turn on L1, L2, ETB and JU memories 1 at a time */
>> -			if (qproc->version == MSS_MSM8953 ||
>> +			if (qproc->version == MSS_MDM9607 ||
>> +			    qproc->version == MSS_MSM8953 ||
>>   			    qproc->version == MSS_MSM8996) {
>>   				mem_pwr_ctl = QDSP6SS_MEM_PWR_CTL;
>>   				i = 19;
>> @@ -822,16 +829,34 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>   				i = 28;
>>   			}
>>   			val = readl(qproc->reg_base + mem_pwr_ctl);
>> -			for (; i >= 0; i--) {
>> -				val |= BIT(i);
>> -				writel(val, qproc->reg_base + mem_pwr_ctl);
>> +			if (qproc->version == MSS_MDM9607) {
>>   				/*
>> -				 * Read back value to ensure the write is done then
>> -				 * wait for 1us for both memory peripheral and data
>> -				 * array to turn on.
>> +				 * Set first 5 bits in reverse to avoid
>> +				 * "inrush current" issues.
>>   				 */
>> -				val |= readl(qproc->reg_base + mem_pwr_ctl);
>> -				udelay(1);
>> +				reverse = 6;
>> +				for (; i >= reverse; i--) {
>> +					val |= BIT(i);
>> +					writel(val, qproc->reg_base + mem_pwr_ctl);
>> +					udelay(1);
>> +				}
>> +				for (i = 0; i < reverse; i++) {
>> +					val |= BIT(i);
>> +					writel(val, qproc->reg_base + mem_pwr_ctl);
>> +					udelay(1);
>> +				}
>> +			} else {
>> +				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);
> 
> Isn't the logic here inverted ?
> 
> i.e. you've written a thing and ostensibly require a delay for that 
> thing to take effect, the power to switch on in this case.
> 
> It makes more sense to write, delay and read back rather than write, 
> readback and delay surely...
This is the original reset sequence without modification, i have just 
moved it in a else case when it is not an MDM9607, MSM8917 or MSM8937.
> 
> 
>> +				}
>>   			}
>>   		} else {
>>   			/* Turn on memories */
>> @@ -2410,6 +2435,41 @@ static const struct rproc_hexagon_res 
>> msm8996_mss = {
>>   	.version = MSS_MSM8996,
>>   };
>> 
>> +static const struct rproc_hexagon_res mdm9607_mss = {
>> +	.hexagon_mba_image = "mba.mbn",
>> +	.proxy_supply = (struct qcom_mss_reg_res[]) {
>> +		{
>> +			.supply = "pll",
>> +			.uA = 100000,
>> +		},
>> +		{}
>> +	},
>> +	.proxy_clk_names = (char*[]){
>> +		"xo",
>> +		NULL
>> +	},
>> +	.active_clk_names = (char*[]){
>> +		"iface",
>> +		"bus",
>> +		"mem",
>> +		NULL
>> +	},
>> +	.proxy_pd_names = (char*[]){
>> +		"mx",
>> +		"cx",
>> +		NULL
>> +	},
>> +	.need_mem_protection = false,
>> +	.has_alt_reset = false,
>> +	.has_mba_logs = false,
>> +	.has_spare_reg = false,
>> +	.has_qaccept_regs = false,
>> +	.has_ext_bhs_reg = false,
>> +	.has_ext_cntl_regs = false,
>> +	.has_vq6 = false,
>> +	.version = MSS_MDM9607,
>> +};
>> +
>>   static const struct rproc_hexagon_res msm8909_mss = {
>>   	.hexagon_mba_image = "mba.mbn",
>>   	.proxy_supply = (struct qcom_mss_reg_res[]) {
>> @@ -2672,6 +2732,7 @@ static const struct rproc_hexagon_res 
>> msm8926_mss = {
>> 
>>   static const struct of_device_id q6v5_of_match[] = {
>>   	{ .compatible = "qcom,q6v5-pil", .data = &msm8916_mss},
>> +	{ .compatible = "qcom,mdm9607-mss-pil", .data = &mdm9607_mss},
>>   	{ .compatible = "qcom,msm8226-mss-pil", .data = &msm8226_mss},
>>   	{ .compatible = "qcom,msm8909-mss-pil", .data = &msm8909_mss},
>>   	{ .compatible = "qcom,msm8916-mss-pil", .data = &msm8916_mss},
>> 
>> --
>> 2.52.0
>> 
>> 

On 01/01/2026 13:50, barnabas.czeman@mainlining.org wrote:
>>> +				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);
>> Isn't the logic here inverted ?
>>
>> i.e. you've written a thing and ostensibly require a delay for that
>> thing to take effect, the power to switch on in this case.
>>
>> It makes more sense to write, delay and read back rather than write,
>> readback and delay surely...
> This is the original reset sequence without modification, i have just
> moved it in a else case when it is not an MDM9607, MSM8917 or MSM8937.

Doesn't make it correct, we fix upstream logic bugs all the time...

For example a read-back to ensure write completion is only required for 
posted memory transactions.

Is this a posted write ?

Is there an io-fabric in the world which exceeds 1 microsecond to 
perform a write transaction ?

Anyway leaving that aside the bit that's really objectionable and IMO 
obvious a bug is val |= readl();

Why or the bit back in ? and then why not check the bit was set on the 
read ?

val = readl() is a lot less janky and shouldn't it matter that the bit 
we tried to set is actually reflected in the read-back ?

Failure to set the bit would certainly be a problem...

---
bod

On 1/1/26 9:58 PM, Bryan O'Donoghue wrote:
> On 01/01/2026 13:50, barnabas.czeman@mainlining.org wrote:
>>>> +                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);
>>> Isn't the logic here inverted ?
>>>
>>> i.e. you've written a thing and ostensibly require a delay for that
>>> thing to take effect, the power to switch on in this case.
>>>
>>> It makes more sense to write, delay and read back rather than write,
>>> readback and delay surely...
>> This is the original reset sequence without modification, i have just
>> moved it in a else case when it is not an MDM9607, MSM8917 or MSM8937.
> 
> Doesn't make it correct, we fix upstream logic bugs all the time...
> 
> For example a read-back to ensure write completion is only required for posted memory transactions.
> 
> Is this a posted write ?
> 
> Is there an io-fabric in the world which exceeds 1 microsecond to perform a write transaction ?

Writes on arm64 aren't usually observable from the remote endpoint when
you would expect them to, they can be buffered unless there's an explicit
readback right afterwards (which creates a dependency that the processor
will fulfill)

Now I don't like that this driver is going

val |= BIT(i);
writel(val, foo);
// val is "altered" but not really
val |= readl(foo);

I didn't notice we were just doing a readback for the sake of a readback
in the last revision. MDM9607 should most definitely have it too..
Perhaps I should have just read the comment

Konrad

On 2026-01-02 13:00, Konrad Dybcio wrote:
> On 1/1/26 9:58 PM, Bryan O'Donoghue wrote:
>> On 01/01/2026 13:50, barnabas.czeman@mainlining.org wrote:
>>>>> +                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);
>>>> Isn't the logic here inverted ?
>>>> 
>>>> i.e. you've written a thing and ostensibly require a delay for that
>>>> thing to take effect, the power to switch on in this case.
>>>> 
>>>> It makes more sense to write, delay and read back rather than write,
>>>> readback and delay surely...
>>> This is the original reset sequence without modification, i have just
>>> moved it in a else case when it is not an MDM9607, MSM8917 or 
>>> MSM8937.
>> 
>> Doesn't make it correct, we fix upstream logic bugs all the time...
>> 
>> For example a read-back to ensure write completion is only required 
>> for posted memory transactions.
>> 
>> Is this a posted write ?
>> 
>> Is there an io-fabric in the world which exceeds 1 microsecond to 
>> perform a write transaction ?
> 
> Writes on arm64 aren't usually observable from the remote endpoint when
> you would expect them to, they can be buffered unless there's an 
> explicit
> readback right afterwards (which creates a dependency that the 
> processor
> will fulfill)
> 
> Now I don't like that this driver is going
> 
> val |= BIT(i);
> writel(val, foo);
> // val is "altered" but not really
> val |= readl(foo);
> 
> I didn't notice we were just doing a readback for the sake of a 
> readback
> in the last revision. MDM9607 should most definitely have it too..
In this case I should go back to previous inrush current mitigation from 
v2.
> Perhaps I should have just read the comment
> 
> Konrad

On 02/01/2026 12:00, Konrad Dybcio wrote:
> Now I don't like that this driver is going
> 
> val |= BIT(i);
> writel(val, foo);
> // val is "altered" but not really
> val |= readl(foo);
> 
> I didn't notice we were just doing a readback for the sake of a readback
> in the last revision. MDM9607 should most definitely have it too..
> Perhaps I should have just read the comment

Yeah this just looks dodgy and inconsistent in this code.

And anyway, why OR those bits in...

---
bod

On 1/2/26 2:00 PM, Bryan O'Donoghue wrote:
> On 02/01/2026 12:00, Konrad Dybcio wrote:
>> Now I don't like that this driver is going
>>
>> val |= BIT(i);
>> writel(val, foo);
>> // val is "altered" but not really
>> val |= readl(foo);
>>
>> I didn't notice we were just doing a readback for the sake of a readback
>> in the last revision. MDM9607 should most definitely have it too..
>> Perhaps I should have just read the comment
> 
> Yeah this just looks dodgy and inconsistent in this code.
> 
> And anyway, why OR those bits in...

Changing |= to just = would make it easier to follow indeed

Konrad

On 02/01/2026 12:00, Konrad Dybcio wrote:
>> Is there an io-fabric in the world which exceeds 1 microsecond to perform a write transaction ?
> Writes on arm64 aren't usually observable from the remote endpoint when
> you would expect them to, they can be buffered unless there's an explicit
> readback right afterwards (which creates a dependency that the processor
> will fulfill)

I don't mean write-combining cache, I mean posted versus non-posted 
writes which is a feature of the front-side-bus :)

---
bod

On 1/2/26 1:59 PM, Bryan O'Donoghue wrote:
> On 02/01/2026 12:00, Konrad Dybcio wrote:
>>> Is there an io-fabric in the world which exceeds 1 microsecond to perform a write transaction ?
>> Writes on arm64 aren't usually observable from the remote endpoint when
>> you would expect them to, they can be buffered unless there's an explicit
>> readback right afterwards (which creates a dependency that the processor
>> will fulfill)
> 
> I don't mean write-combining cache, I mean posted versus non-posted writes which is a feature of the front-side-bus :)

Writes are posted (Device-nGnRE, note the E attribute is set)

https://documentation-service.arm.com/static/63a43e333f28e5456434e18b

Konrad

On 2026-01-01 21:58, Bryan O'Donoghue wrote:
> On 01/01/2026 13:50, barnabas.czeman@mainlining.org wrote:
>>>> +				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);
>>> Isn't the logic here inverted ?
>>> 
>>> i.e. you've written a thing and ostensibly require a delay for that
>>> thing to take effect, the power to switch on in this case.
>>> 
>>> It makes more sense to write, delay and read back rather than write,
>>> readback and delay surely...
>> This is the original reset sequence without modification, i have just
>> moved it in a else case when it is not an MDM9607, MSM8917 or MSM8937.
> 
> Doesn't make it correct, we fix upstream logic bugs all the time...
Here is the original upstream logic
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/qcom_q6v5_mss.c?h=next-20251219#n823
and here is the same at downstream 3.18
https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c32-05500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L451
and same from downstream 4.9
https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L518

> 
> For example a read-back to ensure write completion is only required for 
> posted memory transactions.
> 
> Is this a posted write ?
> 
> Is there an io-fabric in the world which exceeds 1 microsecond to 
> perform a write transaction ?
> 
> Anyway leaving that aside the bit that's really objectionable and IMO 
> obvious a bug is val |= readl();
> 
> Why or the bit back in ? and then why not check the bit was set on the 
> read ?
> 
> val = readl() is a lot less janky and shouldn't it matter that the bit 
> we tried to set is actually reflected in the read-back ?
> 
> Failure to set the bit would certainly be a problem...
> 
> ---
> bod

On 01/01/2026 21:57, barnabas.czeman@mainlining.org wrote:
> On 2026-01-01 21:58, Bryan O'Donoghue wrote:
>> On 01/01/2026 13:50, barnabas.czeman@mainlining.org wrote:
>>>>> +				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);
>>>> Isn't the logic here inverted ?
>>>>
>>>> i.e. you've written a thing and ostensibly require a delay for that
>>>> thing to take effect, the power to switch on in this case.
>>>>
>>>> It makes more sense to write, delay and read back rather than write,
>>>> readback and delay surely...
>>> This is the original reset sequence without modification, i have just
>>> moved it in a else case when it is not an MDM9607, MSM8917 or MSM8937.
>>
>> Doesn't make it correct, we fix upstream logic bugs all the time...
> Here is the original upstream logic
> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/qcom_q6v5_mss.c?h=next-20251219#n823
> and here is the same at downstream 3.18
> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c32-05500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L451
> and same from downstream 4.9
> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L518

Plenty of downstream bugs...

Let's assume those are posted writes i.e. the IO fabric equivalent of 
UDP - I'm not sure I'd say the downstream code is consistent in its 
treatement of write transactions..

But aside from just pointing at downstream - how is val |= readl() a 
correct thing versus val = readl();

...

I mean its just not

> 
>>
>> For example a read-back to ensure write completion is only required for
>> posted memory transactions.
>>
>> Is this a posted write ?
>>
>> Is there an io-fabric in the world which exceeds 1 microsecond to
>> perform a write transaction ?
>>
>> Anyway leaving that aside the bit that's really objectionable and IMO
>> obvious a bug is val |= readl();
>>
>> Why or the bit back in ? and then why not check the bit was set on the
>> read ?
>>
>> val = readl() is a lot less janky and shouldn't it matter that the bit
>> we tried to set is actually reflected in the read-back ?
>>
>> Failure to set the bit would certainly be a problem...
>>
>> ---
>> bod

On 2026-01-02 10:55, Bryan O'Donoghue wrote:
> On 01/01/2026 21:57, barnabas.czeman@mainlining.org wrote:
>> On 2026-01-01 21:58, Bryan O'Donoghue wrote:
>>> On 01/01/2026 13:50, barnabas.czeman@mainlining.org wrote:
>>>>>> +				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);
>>>>> Isn't the logic here inverted ?
>>>>> 
>>>>> i.e. you've written a thing and ostensibly require a delay for that
>>>>> thing to take effect, the power to switch on in this case.
>>>>> 
>>>>> It makes more sense to write, delay and read back rather than 
>>>>> write,
>>>>> readback and delay surely...
>>>> This is the original reset sequence without modification, i have 
>>>> just
>>>> moved it in a else case when it is not an MDM9607, MSM8917 or 
>>>> MSM8937.
>>> 
>>> Doesn't make it correct, we fix upstream logic bugs all the time...
>> Here is the original upstream logic
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/qcom_q6v5_mss.c?h=next-20251219#n823
>> and here is the same at downstream 3.18
>> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c32-05500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L451
>> and same from downstream 4.9
>> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L518
> 
> Plenty of downstream bugs...
Here is the commit where that line was introduced 
https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/ffc6ee0242ec4caa69687848ad3ac5f376b3d005
> 
> Let's assume those are posted writes i.e. the IO fabric equivalent of 
> UDP - I'm not sure I'd say the downstream code is consistent in its 
> treatement of write transactions..
> 
> But aside from just pointing at downstream - how is val |= readl() a 
> correct thing versus val = readl();
> 
> ...
> 
> I mean its just not
> 
>> 
>>> 
>>> For example a read-back to ensure write completion is only required 
>>> for
>>> posted memory transactions.
>>> 
>>> Is this a posted write ?
>>> 
>>> Is there an io-fabric in the world which exceeds 1 microsecond to
>>> perform a write transaction ?
>>> 
>>> Anyway leaving that aside the bit that's really objectionable and IMO
>>> obvious a bug is val |= readl();
>>> 
>>> Why or the bit back in ? and then why not check the bit was set on 
>>> the
>>> read ?
>>> 
>>> val = readl() is a lot less janky and shouldn't it matter that the 
>>> bit
>>> we tried to set is actually reflected in the read-back ?
>>> 
>>> Failure to set the bit would certainly be a problem...
>>> 
>>> ---
>>> bod

On 01/01/2026 13:13, Bryan O'Donoghue wrote:
> On 31/12/2025 16:30, Barnabás Czémán wrote:
>> From: Stephan Gerhold <stephan@gerhold.net>
>>
>> Add support for MDM9607 MSS it have different ACC settings
>> and it needs mitigation for inrush current issue.
>>
>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>> [Reword the commit, add necessary flags, rework inrush current mitigation]
>> Signed-off-by: Barnabás Czémán <barnabas.czeman@mainlining.org>
>> ---
>>    drivers/remoteproc/qcom_q6v5_mss.c | 89 ++++++++++++++++++++++++++++++++------
>>    1 file changed, 75 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/remoteproc/qcom_q6v5_mss.c b/drivers/remoteproc/qcom_q6v5_mss.c
>> index 3c404118b322..19863c576d72 100644
>> --- a/drivers/remoteproc/qcom_q6v5_mss.c
>> +++ b/drivers/remoteproc/qcom_q6v5_mss.c
>> @@ -124,6 +124,7 @@
>>    #define QDSP6v56_CLAMP_QMC_MEM		BIT(22)
>>    #define QDSP6SS_XO_CBCR		0x0038
>>    #define QDSP6SS_ACC_OVERRIDE_VAL		0x20
>> +#define QDSP6SS_ACC_OVERRIDE_VAL_9607	0x80800000
>>    #define QDSP6v55_BHS_EN_REST_ACK	BIT(0)
>>
>>    /* QDSP6v65 parameters */
>> @@ -256,6 +257,7 @@ struct q6v5 {
>>    };
>>
>>    enum {
>> +	MSS_MDM9607,
>>    	MSS_MSM8226,
>>    	MSS_MSM8909,
>>    	MSS_MSM8916,
>> @@ -747,15 +749,19 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>    			return ret;
>>    		}
>>    		goto pbl_wait;
>> -	} else if (qproc->version == MSS_MSM8909 ||
>> +	} else if (qproc->version == MSS_MDM9607 ||
>> +		   qproc->version == MSS_MSM8909 ||
>>    		   qproc->version == MSS_MSM8953 ||
>>    		   qproc->version == MSS_MSM8996 ||
>>    		   qproc->version == MSS_MSM8998 ||
>>    		   qproc->version == MSS_SDM660) {
>>
>> -		if (qproc->version != MSS_MSM8909 &&
>> -		    qproc->version != MSS_MSM8953)
>> -			/* Override the ACC value if required */
>> +		/* Override the ACC value if required */
>> +		if (qproc->version == MSS_MDM9607)
>> +			writel(QDSP6SS_ACC_OVERRIDE_VAL_9607,
>> +			       qproc->reg_base + QDSP6SS_STRAP_ACC);
>> +		else if (qproc->version != MSS_MSM8909 &&
>> +			 qproc->version != MSS_MSM8953)
>>    			writel(QDSP6SS_ACC_OVERRIDE_VAL,
>>    			       qproc->reg_base + QDSP6SS_STRAP_ACC);
>>
>> @@ -800,7 +806,7 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>    		writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>>
>>    		if (qproc->version != MSS_MSM8909) {
>> -			int mem_pwr_ctl;
>> +			int mem_pwr_ctl, reverse = 0;
>>
>>    			/* Deassert QDSP6 compiler memory clamp */
>>    			val = readl(qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>> @@ -812,7 +818,8 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>    			writel(val, qproc->reg_base + QDSP6SS_PWR_CTL_REG);
>>
>>    			/* Turn on L1, L2, ETB and JU memories 1 at a time */
>> -			if (qproc->version == MSS_MSM8953 ||
>> +			if (qproc->version == MSS_MDM9607 ||
>> +			    qproc->version == MSS_MSM8953 ||
>>    			    qproc->version == MSS_MSM8996) {
>>    				mem_pwr_ctl = QDSP6SS_MEM_PWR_CTL;
>>    				i = 19;
>> @@ -822,16 +829,34 @@ static int q6v5proc_reset(struct q6v5 *qproc)
>>    				i = 28;
>>    			}
>>    			val = readl(qproc->reg_base + mem_pwr_ctl);
>> -			for (; i >= 0; i--) {
>> -				val |= BIT(i);
>> -				writel(val, qproc->reg_base + mem_pwr_ctl);
>> +			if (qproc->version == MSS_MDM9607) {
>>    				/*
>> -				 * Read back value to ensure the write is done then
>> -				 * wait for 1us for both memory peripheral and data
>> -				 * array to turn on.
>> +				 * Set first 5 bits in reverse to avoid
>> +				 * "inrush current" issues.
>>    				 */
>> -				val |= readl(qproc->reg_base + mem_pwr_ctl);
>> -				udelay(1);
>> +				reverse = 6;
>> +				for (; i >= reverse; i--) {
>> +					val |= BIT(i);
>> +					writel(val, qproc->reg_base + mem_pwr_ctl);
>> +					udelay(1);
>> +				}
>> +				for (i = 0; i < reverse; i++) {
>> +					val |= BIT(i);
>> +					writel(val, qproc->reg_base + mem_pwr_ctl);
>> +					udelay(1);
>> +				}
>> +			} else {
>> +				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);
> 
> Isn't the logic here inverted ?
> 
> i.e. you've written a thing and ostensibly require a delay for that
> thing to take effect, the power to switch on in this case.
> 
> It makes more sense to write, delay and read back rather than write,
> readback and delay surely...
> 
Also now that I look at this, shouldn't you interrogate the bit gets set 
as per your write ?

Does this bit mean "yes I acknowledge your request" or "yes I carried 
out your request" ?

In the first case you care that the bit indicates something useful, in 
the second case it barely indicates anything at all.

---
bod

On 01/01/2026 13:19, Bryan O'Donoghue wrote:
> In the first case you care that the bit indicates something useful, in
> the second case it barely indicates anything at all.

Swap these bits, the first case is an acknowledgement of receiving a 
command the second case is a status bit from carrying out the command.

Either way it seems a bit mindless to just or back in whatever bit you 
read back here... in fact shouldn't val be _equal_ to the value - 
instead of or-ing the value in ?

I think it probably should and I also think if you are error checking 
you should check your flagged bit actually appears in the read-back dword.

---
bod