Messages in this thread | | | Subject | Re: [PATCH] regmap: move readable check before accessing regcache. | From | Srinivas Kandagatla <> | Date | Mon, 21 Jun 2021 13:43:10 +0100 |
| |
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_device_spec.pdf > >> 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
>
| |