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

barnabas.czeman
2026-01-03T07:41:10Z

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