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

[barnabas.czeman@mainlining.org]

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