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

[barnabas.czeman@mainlining.org]

On 2026-01-02 10:55, Bryan O'Donoghue wrote:
> On 01/01/2026 21:57, barnabas.czeman@mainlining.org wrote:
>> On 2026-01-01 21:58, 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...
>> Here is the original upstream logic
>> https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/tree/drivers/remoteproc/qcom_q6v5_mss.c?h=next-20251219#n823
>> and here is the same at downstream 3.18
>> https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/blob/LA.UM.6.6.c32-05500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L451
>> and same from downstream 4.9
>> https://git.codelinaro.org/clo/la/kernel/msm-4.9/-/blob/LA.UM.10.6.2.c26-01500-89xx.0/drivers/soc/qcom/pil-q6v5.c#L518
> 
> Plenty of downstream bugs...
Here is the commit where that line was introduced 
https://git.codelinaro.org/clo/la/kernel/msm-3.18/-/commit/ffc6ee0242ec4caa69687848ad3ac5f376b3d005
> 
> Let's assume those are posted writes i.e. the IO fabric equivalent of 
> UDP - I'm not sure I'd say the downstream code is consistent in its 
> treatement of write transactions..
> 
> But aside from just pointing at downstream - how is val |= readl() a 
> correct thing versus val = readl();
> 
> ...
> 
> I mean its just not
> 
>> 
>>> 
>>> 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