On 2026-01-02 10:55, Bryan O'Donoghue wrote:
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...
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
