[RFC] regmap: allow volatile register writes with cached only read maps

[RFC] regmap: allow volatile register writes with cached only read maps

[Jorge Ramirez-Ortiz]

Regmap only allows volatile access to registers when the client
supports both reads and writes.

This commit bypasses that limitation and enables volatile writes to
selected registers while maintaining cached accesses on all reads. For
this, the client does not need to configure the reg_read callback.

Signed-off-by: Jorge Ramirez-Ortiz <jramirez@baylibre.com>
---
 drivers/base/regmap/internal.h       |  1 +
 drivers/base/regmap/regcache.c       | 10 +++++++++-
 drivers/base/regmap/regmap-debugfs.c | 17 +++++++++++++++--
 drivers/base/regmap/regmap.c         | 11 +++++++++++
 4 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index 53785e0..496c825 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -180,6 +180,7 @@ bool regmap_cached(struct regmap *map, unsigned int reg);
 bool regmap_writeable(struct regmap *map, unsigned int reg);
 bool regmap_readable(struct regmap *map, unsigned int reg);
 bool regmap_volatile(struct regmap *map, unsigned int reg);
+bool regmap_volatile_writes(struct regmap *map, unsigned int reg);
 bool regmap_precious(struct regmap *map, unsigned int reg);
 
 int _regmap_write(struct regmap *map, unsigned int reg,
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 7735603..51cceaa 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -273,8 +273,16 @@ int regcache_write(struct regmap *map,
 
 	BUG_ON(!map->cache_ops);
 
-	if (!regmap_volatile(map, reg))
+	if (!regmap_volatile(map, reg)) {
+
+		if (!regmap_readable(map, reg)) {
+			/* check if the client configured volatile writes */
+			if (regmap_volatile_writes(map, reg))
+				return 0;
+		}
+
 		return map->cache_ops->write(map, reg, value);
+	}
 
 	return 0;
 }
diff --git a/drivers/base/regmap/regmap-debugfs.c b/drivers/base/regmap/regmap-debugfs.c
index 87b562e..89fb771 100644
--- a/drivers/base/regmap/regmap-debugfs.c
+++ b/drivers/base/regmap/regmap-debugfs.c
@@ -415,20 +415,33 @@ static const struct file_operations regmap_reg_ranges_fops = {
 static int regmap_access_show(struct seq_file *s, void *ignored)
 {
 	struct regmap *map = s->private;
+	char reg_volatile;
 	int i, reg_len;
 
 	reg_len = regmap_calc_reg_len(map->max_register);
 
 	for (i = 0; i <= map->max_register; i += map->reg_stride) {
-		/* Ignore registers which are neither readable nor writable */
+		 reg_volatile = 'n';
+
+		 /* Ignore registers which are neither readable nor writable */
 		if (!regmap_readable(map, i) && !regmap_writeable(map, i))
 			continue;
 
+		if (regmap_volatile(map, i)) {
+			reg_volatile = 'y';
+			goto done;
+		}
+
+		if (!regmap_readable(map, i)) {
+			if (regmap_volatile_writes(map, i))
+				reg_volatile = 'y';
+		}
+done:
 		/* Format the register */
 		seq_printf(s, "%.*x: %c %c %c %c\n", reg_len, i,
 			   regmap_readable(map, i) ? 'y' : 'n',
 			   regmap_writeable(map, i) ? 'y' : 'n',
-			   regmap_volatile(map, i) ? 'y' : 'n',
+			   reg_volatile,
 			   regmap_precious(map, i) ? 'y' : 'n');
 	}
 
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 3bc8488..7d4b671 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -154,6 +154,17 @@ bool regmap_volatile(struct regmap *map, unsigned int reg)
 		return true;
 }
 
+bool regmap_volatile_writes(struct regmap *map, unsigned int reg)
+{
+	if (map->volatile_reg)
+		return map->volatile_reg(map->dev, reg);
+
+	if (map->volatile_table)
+		return regmap_check_range_table(map, reg, map->volatile_table);
+
+	return false;
+}
+
 bool regmap_precious(struct regmap *map, unsigned int reg)
 {
 	if (!regmap_readable(map, reg))
-- 
2.7.4

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Mark Brown]

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

On Wed, May 09, 2018 at 12:06:09AM +0200, Jorge Ramirez-Ortiz wrote:
> Regmap only allows volatile access to registers when the client
> supports both reads and writes.
> 
> This commit bypasses that limitation and enables volatile writes to
> selected registers while maintaining cached accesses on all reads. For
> this, the client does not need to configure the reg_read callback.

I don't understand what voltile access means for write only devices.
Volatile means that we don't read the cache but go direct to the
hardware so if we can't read the hardware that's pretty redundant, a
volatile read that goes to the cache is just a default read.

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

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Jorge Ramirez-Ortiz]

On 05/09/2018 10:39 AM, Mark Brown wrote:
> On Wed, May 09, 2018 at 12:06:09AM +0200, Jorge Ramirez-Ortiz wrote:
>> Regmap only allows volatile access to registers when the client
>> supports both reads and writes.
>>
>> This commit bypasses that limitation and enables volatile writes to
>> selected registers while maintaining cached accesses on all reads. For
>> this, the client does not need to configure the reg_read callback.
> I don't understand what voltile access means for write only devices.
> Volatile means that we don't read the cache but go direct to the
> hardware so if we can't read the hardware that's pretty redundant, a
> volatile read that goes to the cache is just a default read.

oops, sorry will try to be a bit more clear with an example.

This patch tries to support a map that provides:

1. only cached reads: (as a consequence every regmap write must succeed).
2. cached writes: do not access the hardware unless the value differs 
from what is in the cache already or (3) applies.
3. support for selectable volatile writes: those that will always access 
the device no matter what the cache holds.

Something like this:

static const struct regmap_config foo_regmap = {
     .reg_write        = foo_write_reg,

     .reg_bits        = 32,
     .val_bits        = 32,
     .reg_stride        = 1,

     .volatile_reg        = foo_volatile_reg,

     .max_register        = CODEC_ENABLE_DEBUG_CTRL_REG,
     .reg_defaults        = foo_reg_defaults,
     .num_reg_defaults    = ARRAY_SIZE(foo_reg_defaults),
     .cache_type        = REGCACHE_RBTREE,
};


I dont think - I could be wrong- that this is something that we can 
support today since the current code seems to require that the regmap is 
readable (ie, that it implements reg_read).
But it could also be that I am missing something in my config? This is 
why I sent an RFC instead of a PATCH, because I am not 100% sure that I 
am not missing something.

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Mark Brown]

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

On Wed, May 09, 2018 at 01:49:21PM +0200, Jorge Ramirez-Ortiz wrote:
> On 05/09/2018 10:39 AM, Mark Brown wrote:

> > I don't understand what voltile access means for write only devices.
> > Volatile means that we don't read the cache but go direct to the
> > hardware so if we can't read the hardware that's pretty redundant, a
> > volatile read that goes to the cache is just a default read.

> 1. only cached reads: (as a consequence every regmap write must succeed).
> 2. cached writes: do not access the hardware unless the value differs from
> what is in the cache already or (3) applies.
> 3. support for selectable volatile writes: those that will always access the
> device no matter what the cache holds.

We don't currently suppress writes except when regmap_update_bits()
notices that the modification was a noop.  You probably want to be using
regmap_write_bits() here instead of regmap_update_bits(), that will
always do the write.

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

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Jorge Ramirez-Ortiz]

On 05/11/2018 04:00 AM, Mark Brown wrote:
> On Wed, May 09, 2018 at 01:49:21PM +0200, Jorge Ramirez-Ortiz wrote:
>> On 05/09/2018 10:39 AM, Mark Brown wrote:
>>> I don't understand what voltile access means for write only devices.
>>> Volatile means that we don't read the cache but go direct to the
>>> hardware so if we can't read the hardware that's pretty redundant, a
>>> volatile read that goes to the cache is just a default read.
>> 1. only cached reads: (as a consequence every regmap write must succeed).
>> 2. cached writes: do not access the hardware unless the value differs from
>> what is in the cache already or (3) applies.
>> 3. support for selectable volatile writes: those that will always access the
>> device no matter what the cache holds.
> We don't currently suppress writes except when regmap_update_bits()
> notices that the modification was a noop.  You probably want to be using
> regmap_write_bits() here instead of regmap_update_bits(), that will
> always do the write.

but isnt that interface at a different level?
I am not sure if you are asking me to review my patch or just discarding 
the RFC and highlighting that I have a configuration problem.

In my use case and what triggered this RFC (config below), an 'amixer 
set' might never reach the driver's .reg_write interface even though the 
register is configured as volatile (to me this is not consistent since 
volatile_reg is being silently ignored).

So I dont see where/how your recommendation fits; maybe you could 
clarify a bit more please?

static const struct regmap_config foo_regmap = {
     .reg_write           = foo_write_reg,

     .reg_bits            = 32,
     .val_bits            = 32,
     .reg_stride          = 1,

     .volatile_reg        = foo_volatile_reg,

     .max_register        = CODEC_ENABLE_DEBUG_CTRL_REG,
     .reg_defaults        = foo_reg_defaults,
     .num_reg_defaults    = ARRAY_SIZE(foo_reg_defaults),
     .cache_type          = REGCACHE_RBTREE,
};

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Mark Brown]

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

On Fri, May 11, 2018 at 12:29:42PM +0200, Jorge Ramirez-Ortiz wrote:
> On 05/11/2018 04:00 AM, Mark Brown wrote:

> > We don't currently suppress writes except when regmap_update_bits()
> > notices that the modification was a noop.  You probably want to be using
> > regmap_write_bits() here instead of regmap_update_bits(), that will
> > always do the write.

> but isnt that interface at a different level?

It's at the level where we suppress writes - the write suppression isn't
a feature of the caching, it's something that regmap_update_bits() does
if it notices that it won't change anything.  It'll happen even if
there's no cache at all.

> I am not sure if you are asking me to review my patch or just discarding the
> RFC and highlighting that I have a configuration problem.

I don't understand your patch as-is.

> In my use case and what triggered this RFC (config below), an 'amixer set'
> might never reach the driver's .reg_write interface even though the register
> is configured as volatile (to me this is not consistent since volatile_reg
> is being silently ignored).

I'm not seeing any inconsistency there.  Volatility means the register
can't be cached as it might change underneath us, it doesn't have any
impact on writes and it's happening at a lower level.  Like I say if you
absolutely need a write to happen you should be explicitly doing a
write, though if you need a write to happen for a noop control change it
sounds like there's something weird with that control that's possibly a
problem anyway.

> So I dont see where/how your recommendation fits; maybe you could clarify a
> bit more please?

As I've been saying if you explicitly need a write to happen don't use
regmap_update_bits(), do something that guarantees you'll get a write
like regmap_write() or regmap_write_bits().

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

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Jorge Ramirez-Ortiz]

On 05/13/2018 04:22 AM, Mark Brown wrote:
> On Fri, May 11, 2018 at 12:29:42PM +0200, Jorge Ramirez-Ortiz wrote:
>> On 05/11/2018 04:00 AM, Mark Brown wrote:
>>> We don't currently suppress writes except when regmap_update_bits()
>>> notices that the modification was a noop.  You probably want to be using
>>> regmap_write_bits() here instead of regmap_update_bits(), that will
>>> always do the write.
>> but isnt that interface at a different level?
> It's at the level where we suppress writes - the write suppression isn't
> a feature of the caching, it's something that regmap_update_bits() does
> if it notices that it won't change anything.  It'll happen even if
> there's no cache at all.
>
>> I am not sure if you are asking me to review my patch or just discarding the
>> RFC and highlighting that I have a configuration problem.
> I don't understand your patch as-is.
>
>> In my use case and what triggered this RFC (config below), an 'amixer set'
>> might never reach the driver's .reg_write interface even though the register
>> is configured as volatile (to me this is not consistent since volatile_reg
>> is being silently ignored).
> I'm not seeing any inconsistency there.  Volatility means the register
> can't be cached as it might change underneath us, it doesn't have any
> impact on writes and it's happening at a lower level.  Like I say if you
> absolutely need a write to happen you should be explicitly doing a
> write, though if you need a write to happen for a noop control change it
> sounds like there's something weird with that control that's possibly a
> problem anyway.
>
>> So I dont see where/how your recommendation fits; maybe you could clarify a
>> bit more please?
> As I've been saying if you explicitly need a write to happen don't use
> regmap_update_bits(), do something that guarantees you'll get a write
> like regmap_write() or regmap_write_bits().

I do understand your point but Mark, that interface you mention sits 
above the user request (as a client the user does not call 
regmap_update_bits or regmap_write_bits or regmap_write(): none of those 
functions mean anything to the foo_regmap definition below - that is why 
we have an interface).

The client just uses this request:

static const struct regmap_config foo_regmap = {
     .reg_write           = foo_write_reg,

     .reg_bits            = 32,
     .val_bits            = 32,
     .reg_stride          = 1,

     .volatile_reg        = foo_volatile_reg,

     .max_register        = CODEC_ENABLE_DEBUG_CTRL_REG,
     .reg_defaults        = foo_reg_defaults,
     .num_reg_defaults    = ARRAY_SIZE(foo_reg_defaults),
     .cache_type          = REGCACHE_RBTREE,
};


and all this request means is that it expects foo_volatile_regs to be 
taken into consideration when accessing the reg_write callback: so 
whoever is calling the interface reg_write (be it regmap_update_bits or 
regmap_write_bits or whoever it is, I dont know) must make sure the 
volatile request applies.

the RFC patch that I submitted achieves exactly that.

does this make more sense now?

Re: [RFC] regmap: allow volatile register writes with cached only read maps

[Mark Brown]

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

On Thu, May 17, 2018 at 09:12:49AM +0200, Jorge Ramirez-Ortiz wrote:
> On 05/13/2018 04:22 AM, Mark Brown wrote:

> > > So I dont see where/how your recommendation fits; maybe you could clarify a
> > > bit more please?

> > As I've been saying if you explicitly need a write to happen don't use
> > regmap_update_bits(), do something that guarantees you'll get a write
> > like regmap_write() or regmap_write_bits().

> I do understand your point but Mark, that interface you mention sits above
> the user request (as a client the user does not call regmap_update_bits or
> regmap_write_bits or regmap_write(): none of those functions mean anything
> to the foo_regmap definition below - that is why we have an interface).

...

> and all this request means is that it expects foo_volatile_regs to be taken
> into consideration when accessing the reg_write callback: so whoever is
> calling the interface reg_write (be it regmap_update_bits or
> regmap_write_bits or whoever it is, I dont know) must make sure the volatile
> request applies.

Right, but your code can expect a lot of things and not have them happen
if they're not good expectations - that's explicitly not what we've been
meaning by volatile registers (they're just registers that can't be
cached).  I mean, you mentioned that you were doing this for an ALSA
control and since ALSA reports noop updates to userspace it'd be
entirely reasonable for an implementation change there were to suppress
the write before it gets to regmap (this is the sort of thing I was
thinking of when I said I was suspicious of what you're doing there,
this doesn't sound like a normal ALSA control).

I get what you're saying, it just doesn't feel like this is the right
place to solve whatever your end use case is, it feels like it's going
to be fragile one way or another and end up adding more complexity -
having a hand coded ALSA control for this feels more secure at that
level never mind anything that's going on in regmap. 

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