Messages in this thread Patch in this message | | | Date | Fri, 28 Apr 2023 08:44:06 +0300 | From | Matti Vaittinen <> | Subject | [RFC PATCH] regmap: force update_bits() to write to HW when reg is volatile |
| |
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(-)
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c index d2a54eb0efd9..dd2f71b576b7 100644 --- a/drivers/base/regmap/regmap.c +++ b/drivers/base/regmap/regmap.c @@ -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; base-commit: eeac8ede17557680855031c6f305ece2378af326 -- 2.40.0
-- Matti Vaittinen, Linux device drivers ROHM Semiconductors, Finland SWDC Kiviharjunlenkki 1E 90220 OULU FINLAND
~~~ "I don't think so," said Rene Descartes. Just then he vanished ~~~ Simon says - in Latin please. ~~~ "non cogito me" dixit Rene Descarte, deinde evanescavit ~~~ Thanks to Simon Glass for the translation =] [unhandled content-type:application/pgp-signature] | |