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

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

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