Re: [PATCH] regmap: allow to define reg_update_bits for no bus configuration

[Mark Brown]

On Wed, Nov 03, 2021 at 08:48:37PM +0100, Ansuel Smith wrote:
> On Wed, Nov 03, 2021 at 05:01:16PM +0000, Mark Brown wrote:
> > On Tue, Nov 02, 2021 at 10:41:38PM +0100, Ansuel Smith wrote:

> > > -	if (regmap_volatile(map, reg) && map->reg_update_bits) {
> > > +	if ((regmap_volatile(map, reg) || !map->bus) && map->reg_update_bits) {
> > >  		ret = map->reg_update_bits(map->bus_context, reg, mask, val);
> > >  		if (ret == 0 && change)
> > >  			*change = true;

> > I don't understand this change.  The point of the check for volatile
> > there is that if the register isn't volatile then we need to ensure that
> > the cache gets updated with any change that happens so we need to go
> > through paths that include cache updates.  The presence or otherwise of
> > a bus does not seem at all relevant here.

> I put the check there to not duplicate the code. The idea here is:
> if we are doing a regmap_update_bits operation on a no bus configuration
> and the function have a dedicated reg_update_bits function, use that
> instead of the normal _reg_read, check and _reg_write.

Yes, I can see that this is what the change does - the problem is that
it's buggy.

> To workaround the CACHE problem, I can add a check and detect if cache is
> disabled and only with that option permit to add a reg_update_bits
> function to the map (for no bus configuration).

That's what the volatile check that is already there does - if there is
no cache or that particular register is uncached then the register is
volatile and we don't need to worry about updating the cache.  There is
not and should not be any connection to how the device is physically
accessed, any connection there is clearly an abstraction problem.

> Again the problem here is in situation where lock is handled outside of
> the read/write and the read/modify/write operation has to be done in one
> go so splitting this operation in 2 step (like it's done for
> regmap_update_bits) would be problematic.

Unconditionally introducing a data corruption or power management bug
for any device that provides an update bits operation regardless of
their requirements to use that operation for a specific register is not
a good idea.  If an individual device can't cope with some or all
registers being cached then the driver for that device should configure
it's regmap appropriately to ensure that the registers in question won't
be cached.

> Another solution would be to expose a way to change the cache externally
> to the regmap operation so that if someone require cache operation and
> require also a dedicated reg_update_bits, he can do that by implementing
> that in his own function.

I'm struggling to see a case where this would be useful without the
register also being volatile in which case it's totally redundant.  If
the register can change underneath us then it is by definition volatile,
if the register can't be changed underneath us then with a cache there's
unlikely to be any meaningful upside to using a specific read/modify/write
operation in the first place.  You might have some case for wider
registers where you can do a smaller transfer but that's got to be rare
and I'd expect we'd have to be doing a lot of register I/O to care about
the performance diffrence.

> A third solution would be check if it's possible to cache the value
> externally to function... Something that calls the reg_update_bits
> dedicated function and then update the cache if needed.

That's exactly what the existing volatility check does, 

> But do we really need to add all this complexity when we can just deny
> an option with cache enabled and force to use the normal way (else part
> in this function)

> Hope I was able to explain better why we need this change.

We do not need this change.  The change that is being proposed will
cause bugs, my best guess is that it's trying to work around a bug in
the driver you're developing where it's enabling caching but not marking
all the volatile registers properly.  If there is any change needed in
the _update_bits() implementation then where we get a device specific
_update_bits() operation from should have no influence on our decision
to use it, doing that is a clear sign that there's an abstraction
problem.

Attachment: signature.asc
Description: PGP signature