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, [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...
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
