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

barnabas.czeman
2026-01-01T21:57:54Z

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

>
> 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