Re: [PATCH v3 3/9] remoteproc: qcom_q6v5_mss: Add MDM9607

[Bryan O'Donoghue <bod@kernel.org>]

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