Re: [RFC] regmap: allow volatile register writes with cached only read maps
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. signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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. signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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
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
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(). signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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(). signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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
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
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. signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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. signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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
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
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. signature.asc Description: PGP signature
Re: [RFC] regmap: allow volatile register writes with cached only read maps
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. signature.asc Description: PGP signature
