On 01/01/2026 21:57, [email protected] wrote:
On 2026-01-01 21:58, Bryan O'Donoghue wrote:
On 01/01/2026 13:50, [email protected] 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...
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
