Date:   Mon, 21 Jun 2021 12:27:47 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Srinivas Kandagatla <srinivas.kandagatla@...aro.org>
Cc:     srivasam@...eaurora.org, rafael@...nel.org,
        dp@...nsource.wolfsonmicro.com, linux-kernel@...r.kernel.org,
        stable@...r.kernel.org, Marek Szyprowski <m.szyprowski@...sung.com>
Subject: Re: [PATCH] regmap: move readable check before accessing regcache.

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

Download attachment "signature.asc" of type "application/pgp-signature" (489 bytes)

Powered by blists - more mailing lists