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

Konrad Dybcio
2026-01-02T12:00:55Z

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