On 21/06/2021 12:27, Mark Brown wrote:
On Mon, Jun 21, 2021 at 11:30:00AM +0100, Srinivas Kandagatla wrote:
On 18/06/2021 16:48, Mark Brown wrote:
On Fri, Jun 18, 2021 at 01:29:50PM +0100, Srinivas Kandagatla wrote:
_regmap_update_bits() checks _regmap_read() return value before bailing out. In non cache path we have this regmap_readable() check however in cached patch we do not have this check, so _regmap_read() will return success in this case so regmap_update_bits() never reports any error.
driver in question does check the return value.
OK, so everything is working fine then - what's the problem? The value
How can this be working fine?
In this particular setup the register is marked as write only and is not readable. Should it really store value in cache at the first instance?
Yes, we know exactly what the value in the register is since we wrote it so there's no problem with us remembering and using that.
Also on the other note, if we mark the same regmap as uncached this usecase will fail straightaway with -EIO, so why is the behavior different in regcache path?
If the register is marked as uncachable then obviously the cache behaviour is going to be different to that for a register which we can cache for whatever reason the register was marked volatile.
Shouldn't the regcache path check if the register is readable before trying to cache the value?
Why? If we know what the value is we can cache it and then use it, meaning things like restoring the value in a cache sync and update_bits() work, this is useful especially on devices which have no read support at
Thanks for the insight, Yes that makes more sense to have cache for write-only too.
all. What would the benefit it not caching it be? >
From "APQ8016E Technical Reference Manual" https://developer.qualcomm.com/qfile/28813/lm80-p0436-7_f_410e_proc_apq8016e...
Section: 4.5.9.6.19 this register LPASS_LPAIF_IRQ_CLEARa is clearly marked with Type: W
with this description: "Writing a 1 to a bit in this register clears the latched interrupt event
So am not 100% sure if we read this we will get anything real from the register. I always get zeros if I do that.
Should this behavior treated as volatile?
Yes. This is indistingusihable from a register that is volatile because it doesn't latch written values, given that you're saying readback actually works there's an argument here that the documentation isn't accurate here. My guess is that this device doesn't have any write only registers as far as anything outside the device is concerned since the I/O hardware won't fault or anything on reads, it just has addresses where the read side isn't wired up to anything.
If we mark this register as volatile and make it readable then it would work but that just sounds like a hack to avoid cache.
Am sure other hardware platforms have similar write-only registers, how do they handle regmap_update_bits case if they have regcache enabled?
They either mark the registers as volatile or just don't do any operations that involve reading the value so it's a non-issue.
I agree, qcom lpass driver is already doing the second one here, so we should mark the register as volatile and readable to avoid the reported issue.
--srini