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

Bryan O'Donoghue
2026-01-01T20:58:37Z

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