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

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

[Ansuel Smith]

Some device requires a special handling for reg_update_bits and can't use
the normal regmap read write logic. An example is when locking is
handled by the device and rmw operations requires to do atomic operations.
Allow to declare a special function in regmap_config for reg_update_bits
as we do with the bus config and append it to the regmap_volatile logic in
_regmap_update_bits.

Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com>
---
 drivers/base/regmap/regmap.c | 3 ++-
 include/linux/regmap.h       | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 21a0c2562ec0..d6fcb45194b0 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -876,6 +876,7 @@ struct regmap *__regmap_init(struct device *dev,
 	if (!bus) {
 		map->reg_read  = config->reg_read;
 		map->reg_write = config->reg_write;
+		map->reg_update_bits = config->reg_update_bits;
 
 		map->defer_caching = false;
 		goto skip_format_initialization;
@@ -3064,7 +3065,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 	if (change)
 		*change = false;
 
-	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;
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index e3c9a25a853a..22652e5fbc38 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -290,6 +290,11 @@ typedef void (*regmap_unlock)(void *);
  *		  read operation on a bus such as SPI, I2C, etc. Most of the
  *		  devices do not need this.
  * @reg_write:	  Same as above for writing.
+ * @reg_update_bits: Optional callback that if filled will be used to perform
+ *		     all the update_bits(rmw) operation. Should only be provided
+ *		     if the function require special handling with lock and reg
+ *		     handling and the operation cannot be represented as a simple
+ *		     update_bits operation on a bus such as SPI, I2C, etc.
  * @fast_io:	  Register IO is fast. Use a spinlock instead of a mutex
  *	     	  to perform locking. This field is ignored if custom lock/unlock
  *	     	  functions are used (see fields lock/unlock of struct regmap_config).
@@ -372,6 +377,8 @@ struct regmap_config {
 
 	int (*reg_read)(void *context, unsigned int reg, unsigned int *val);
 	int (*reg_write)(void *context, unsigned int reg, unsigned int val);
+	int (*reg_update_bits)(void *context, unsigned int reg,
+			       unsigned int mask, unsigned int val);
 
 	bool fast_io;
 
-- 
2.32.0

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 796 bytes --]

On Tue, Nov 02, 2021 at 10:41:38PM +0100, Ansuel Smith wrote:

> @@ -3064,7 +3065,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
>  	if (change)
>  		*change = false;
>  
> -	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.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ansuel Smith]

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:
> 
> > @@ -3064,7 +3065,7 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
> >  	if (change)
> >  		*change = false;
> >  
> > -	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.
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).

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.

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

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.

-- 
	Ansuel

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 4323 bytes --]

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ansuel Smith]

On Wed, Nov 03, 2021 at 09:29:11PM +0000, Mark Brown wrote:
> 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.

I think I'm missing something. The user case is a driver that
have CACHE DISABLED. The !map->bus check is added just to limit this to
a no bus configuration not to permit this with CACHE enabled. The limit
I was referring was in the init function where the update_bits is
assigned to the map. I honestly didn't notice that anything with cache
disabled was flagged as volatile.

So the rest of the changes permit to declare a update_bits function
for a no bus configuration is good?

Again that was the main reason of this patch. Obviously I didn't want to
introduce bugs or an hack to bypass the cache thing. Just I notice this
was permitted with a bus configuration but the no bus configuration
lacks any way to configure a update_bits function.

-- 
	Ansuel

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1162 bytes --]

On Wed, Nov 03, 2021 at 10:53:17PM +0100, Ansuel Smith wrote:
> On Wed, Nov 03, 2021 at 09:29:11PM +0000, Mark Brown wrote:

> > > > 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 think I'm missing something. The user case is a driver that
> have CACHE DISABLED. The !map->bus check is added just to limit this to
> a no bus configuration not to permit this with CACHE enabled. The limit
> I was referring was in the init function where the update_bits is
> assigned to the map. I honestly didn't notice that anything with cache
> disabled was flagged as volatile.

In what way would the presence or absence of a bus be relevant to a
decision about being able to safely use an _update_bits() operation?

> So the rest of the changes permit to declare a update_bits function
> for a no bus configuration is good?

Probably, I'd need to look again.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

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

[Ansuel Smith]

On Thu, Nov 04, 2021 at 02:50:31PM +0000, Mark Brown wrote:
> On Wed, Nov 03, 2021 at 10:53:17PM +0100, Ansuel Smith wrote:
> > On Wed, Nov 03, 2021 at 09:29:11PM +0000, Mark Brown wrote:
> 
> > > > > 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 think I'm missing something. The user case is a driver that
> > have CACHE DISABLED. The !map->bus check is added just to limit this to
> > a no bus configuration not to permit this with CACHE enabled. The limit
> > I was referring was in the init function where the update_bits is
> > assigned to the map. I honestly didn't notice that anything with cache
> > disabled was flagged as volatile.
> 
> In what way would the presence or absence of a bus be relevant to a
> decision about being able to safely use an _update_bits() operation?
>

No reason. It was just to make changes only to a no bus configuration
and doesn't cause any problem/error to other regmap configuration.
(since currently we can declare a custom update_bits function only for
bus configuration)

> > So the rest of the changes permit to declare a update_bits function
> > for a no bus configuration is good?
> 
> Probably, I'd need to look again.

Let me send a v2 so you can check the final patch.

-- 
	Ansuel