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.. Perhaps I should have just read the comment Konrad
