[RFC] regmap: force update_bits() to write to HW when reg is volatile
Commit Message
In many cases the volatile registers are expected to be written to
regardless of the existing value (because, the value of a volatile
register can't be trusted to stay the same during RMW cycle). In fact, it
is questionable if the volatile registers and regmap_update_bits()
conceptually make sense without device specific map->reg_update_bits.
Yet, there are devices where some register contains a 'fixed part' and a
'volatile part'. For example the ROHM BU27008 RGBC sensor has a register
with part-ID (fixed) and a reset bit which, should be written to no matter
what the cached value is - and which is cleared by hardware. In such case
using regmap_update_bits() with this register marked volatile kind of
makes sense. Doing a RMW-cycle is Ok as the static part of the register
can be read. The other option is to hard-code the static part in driver.
This can hit to problems though. For example the fixed part-ID may
actually change, should the driver ever support a variant with different
part ID.
In such case the read + modify could be done by caller, and caller could
then use regmap_write() directly.
However, there may be some similar use-cases already in-tree - potentially
not even noticing that the write might never get in HW when using
regmap_update_bits() with volatile registers if the force_write was not
used here. Thus, set the force_write here even though it may cause some
performance penalty. Still, it's probably easier to optimize the sepcial
cases with performance critical volatile registers than spot all the
errors caused by writes to volatile registers not reaching the HW (once in
a blue moon when tmp == orig).
Force writes to hardware when regmap_update_bits() is used with volatile
registers.
Signed-off-by: Matti Vaittinen <mazziesaccount@gmail.com>
---
This change is inspired by the fact that I've now done the same mistake
_many_ times. I've defined a special register (usually a register with a
reset bit, but there were some clear-on-write registers as well) as
volatile to guarantee that writes are always going to hardware
regardless the existing register value. (To either always trigger the
operation like a reset in the hardware, or by knowing that the value
that has been previously written may be changed by the hardware). Later
I've happily used regmap_update_bits() to change the specific bit (to
clear specific bit or to trigger specific operation) forgetting the fact
that the regmap_update_bits() do not force write to hardware.
Unfortunately, these bugs can be hard to spot as it may be that it is a
rare case where read value is same than the value being written making
issues to occur only once in a blue moon. I am afraid I am not the only
one making this mistake, although I may be the only one who seems to be
unable to learn this :/
This patch intends to 'fix' this by making the volatile regs to be
always written to - even when regmap_update_bits() is used. I am not
100% sure this is "the right thing to do". (I obviously think it might
be, else I wouldn't be sending this patch). Maybe we should just
warn_once() when volatile registers are accessed using
regmap_update_bits() instead? Or perhaps some perl wizard could cook-up
a checkpatch improvement which could check this when checkpatch is ran?
In any case, it'd be great to have some way of help avoiding these
bugs...
This patch has received only very minimal testing and all further
testing would be appreciated!
---
drivers/base/regmap/regmap.c | 72 ++++++++++++++++++++++++++++--------
1 file changed, 56 insertions(+), 16 deletions(-)
base-commit: eeac8ede17557680855031c6f305ece2378af326
Comments
On Fri, Apr 28, 2023 at 08:44:06AM +0300, Matti Vaittinen wrote:
> In many cases the volatile registers are expected to be written to
> regardless of the existing value (because, the value of a volatile
> register can't be trusted to stay the same during RMW cycle). In fact, it
> is questionable if the volatile registers and regmap_update_bits()
> conceptually make sense without device specific map->reg_update_bits.
I think you're looking for regmap_write_bits() here.
On 4/28/23 14:35, Mark Brown wrote:
> On Fri, Apr 28, 2023 at 08:44:06AM +0300, Matti Vaittinen wrote:
>> In many cases the volatile registers are expected to be written to
>> regardless of the existing value (because, the value of a volatile
>> register can't be trusted to stay the same during RMW cycle). In fact, it
>> is questionable if the volatile registers and regmap_update_bits()
>> conceptually make sense without device specific map->reg_update_bits.
>
> I think you're looking for regmap_write_bits() here.
Yes. Thanks for pointing this out to me. This is the functionality I
searched for.
However, my question really was if the regmap_update_bits() (and
regmap_set_bits() / regmap_clear_bits()) should automatically work as
regmap_write_bits() for volatile registers. I'll take your answer as
"no" - and try to remember just use regmap_write_bits() with volatile
registers which may require forcing writing the value even when it is
not changed. Thanks!
Yours,
-- Matti
--
Matti Vaittinen
Linux kernel developer at ROHM Semiconductors
Oulu Finland
~~ When things go utterly wrong vim users can always type :help! ~~
On Fri, Apr 28, 2023 at 12:36:51PM +0000, Vaittinen, Matti wrote:
> On 4/28/23 14:35, Mark Brown wrote:
> > I think you're looking for regmap_write_bits() here.
> Yes. Thanks for pointing this out to me. This is the functionality I
> searched for.
> However, my question really was if the regmap_update_bits() (and
> regmap_set_bits() / regmap_clear_bits()) should automatically work as
> regmap_write_bits() for volatile registers. I'll take your answer as
> "no" - and try to remember just use regmap_write_bits() with volatile
> registers which may require forcing writing the value even when it is
> not changed. Thanks!
Yeah, like you said in your commit there are cases where suppressing the
write is still useful - you might have some bits that are volatile and
some not or you might have something like a pattern where the hardware
can set bits and the host writes 1 to clear. One common thing is to
have read only status bits mixed in with normal read/write bits.
@@ -3234,25 +3234,65 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
if (change)
*change = false;
- if (regmap_volatile(map, reg) && map->reg_update_bits) {
- reg += map->reg_base;
- reg >>= map->format.reg_downshift;
- ret = map->reg_update_bits(map->bus_context, reg, mask, val);
- if (ret == 0 && change)
- *change = true;
- } else {
- ret = _regmap_read(map, reg, &orig);
- if (ret != 0)
- return ret;
-
- tmp = orig & ~mask;
- tmp |= val & mask;
-
- if (force_write || (tmp != orig)) {
- ret = _regmap_write(map, reg, tmp);
+ if (regmap_volatile(map, reg)) {
+ if (map->reg_update_bits) {
+ reg += map->reg_base;
+ reg >>= map->format.reg_downshift;
+ ret = map->reg_update_bits(map->bus_context, reg, mask, val);
if (ret == 0 && change)
*change = true;
+
+ return ret;
}
+ /*
+ * In many cases the volatile registers are expected to be
+ * written to regardless of the existing value (because, the
+ * value of a volatile register can't be trusted to stay the
+ * same during RMW cycle). In fact, it is questionable if the
+ * volatile registers and regmap_update_bits() conceptually
+ * make sense without device specific map->reg_update_bits.
+ *
+ * Yet, there are devices where some register contains a
+ * 'fixed part' and a 'volatile part'. For example the ROHM
+ * BU27008 RGBC sensor has a register with part-ID (fixed) and
+ * a reset bit which, should be written to no matter what the
+ * cached value is - and which is cleared by hardware. In such
+ * case using regmap_update_bits() with this register marked
+ * volatile kind of makes sense. Doing a RMW-cycle is Ok as the
+ * static part of the register can be read. The other option
+ * is to hard-code the static part in driver. This can hit
+ * to problems though. For example the fixed part-ID may
+ * actually change, should the driver ever support a variant
+ * with different part ID.
+ *
+ * In such case the read + modify could be done by caller, and
+ * caller could then use regmap_write() directly.
+ *
+ * However, there may be some similar use-cases already in-tree
+ * - potentially not even noticing that the write might never
+ * get in HW when using regmap_update_bits() with volatile
+ * registers if the force_write was not used here. Thus, set
+ * the force_write here even though it may cause some
+ * performance penalty. Still, it's probably easier to optimize
+ * the sepcial cases with performance critical volatile
+ * registers than spot all the errors caused by writes to
+ * volatile registers not reaching the HW (once in a blue moon
+ * when tmp == orig).
+ */
+ force_write = true;
+ }
+
+ ret = _regmap_read(map, reg, &orig);
+ if (ret != 0)
+ return ret;
+
+ tmp = orig & ~mask;
+ tmp |= val & mask;
+
+ if (force_write || (tmp != orig)) {
+ ret = _regmap_write(map, reg, tmp);
+ if (ret == 0 && change)
+ *change = true;
}
return ret;