Re: [PATCH] regmap: move readable check before accessing regcache.

[Srinivas Kandagatla]

Thanks Mark for your inputs,

On 18/06/2021 16:48, Mark Brown wrote:
> On Fri, Jun 18, 2021 at 01:29:50PM +0100, Srinivas Kandagatla wrote:
> > On 18/06/2021 12:51, Mark Brown wrote:
>
> > > Why will use of regmap_update_bits() mean that a driver will never
> > > notice a write failure?  Shouldn't remgap_update_bits() be fixed to
> > > report any errors it isn't reporting, or the driver fixed to check
>
> > usecase is performing regmap_update_bits() on a *write-only* registers.
>
> > _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?

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?

Shouldn't the regcache path check if the register is readable before 
trying to cache the value?

> in the register is cached, the read returns that cached value and then
> the relevant bits are updated and the new value written out to both the
> cache and the hardware.  No part of that sounds like a problem to me.
>
> > > error codes?  I really don't understand the issue you're trying to
> > > report - what is "the right thing" and what makes you believe that a
> > > driver can't do an _update_bits() on a write only but cached register?
> > > Can you specify in concrete terms what the problem is.
>
> > So one of recent patch ("ASoC: qcom: Fix for DMA interrupt clear reg
> > overwriting) https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=next-20210618&id=da0363f7bfd3c32f8d5918e40bfddb9905c86ee1
> >
> > broke audio on DragonBoard 410c.
>
> > This patch simply converts writes to regmap_update_bits for that particular
> > dma channel. The register that its updating is IRQ_CLEAR register which is
> > software "WRITE-ONLY" and Hardware read-only register.
>
> So the driver is buggy then, the register is clearly volatile and can't
> be cached, and it sounds like it's unsafe to use _update_bits() on it.
> The register is clearly not write only since it can be read and it's
> volatile since the readback value does not reflect what was written (and
> presumably can change),.  Even without that it's buggy to use
> _update_bits() here since the device needs the write to happen
> regardless of the value that is read back, with the register marked as
> volatile that will still potentially fail if the readback value happens
> to be the same as whatever bits the driver is trying to set.

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?

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?

thanks,
srini