[PATCH] regmap: support regmap_field_write() on non-readable fields

[PATCH] regmap: support regmap_field_write() on non-readable fields

[Krzysztof Kozlowski]

Current implementation of regmap_field_write() performs an update of
register (read+write), therefore it ignores regmap read-restrictions and
is not suitable for write-only registers (e.g. interrupt clearing).

Extend regmap_field_write() and regmap_field_force_write() to check if
register is readable and only then perform an update.  In the other
case, it is expected that mask of field covers entire register thus a
full write is allowed.

Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

---

Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
---
 drivers/base/regmap/regmap.c | 50 ++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h       | 15 ++---------
 2 files changed, 52 insertions(+), 13 deletions(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 0caa5690c560..4d18a34f7b2c 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -2192,6 +2192,56 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg,
 }
 EXPORT_SYMBOL_GPL(regmap_noinc_write);
 
+static int _regmap_field_write_or_update(struct regmap_field *field,
+					 unsigned int val, bool *change,
+					 bool async, bool force)
+{
+	unsigned int mask = (~0 << field->shift) & field->mask;
+	unsigned int map_val_mask, map_val_mask_h;
+	int ret;
+
+	if (regmap_readable(field->regmap, field->reg))
+		return regmap_update_bits_base(field->regmap, field->reg,
+					       mask, val << field->shift,
+					       change, async, force);
+
+	map_val_mask_h = field->regmap->format.val_bytes * 8 - 1;
+	map_val_mask = GENMASK(map_val_mask_h, 0);
+
+	/* Writes of parts of register are not allowed for sanity */
+	if (field->shift)
+		return -EINVAL;
+
+	/* Mask of field must cover entire register */
+	if (field->mask != map_val_mask)
+		return -EINVAL;
+
+	if (change)
+		*change = false;
+
+	if (async)
+		ret = regmap_write(field->regmap, field->reg, val);
+	else
+		ret = regmap_write_async(field->regmap, field->reg, val);
+
+	if (ret == 0 && change)
+		*change = true;
+
+	return ret;
+}
+
+int regmap_field_write(struct regmap_field *field, unsigned int val)
+{
+	return _regmap_field_write_or_update(field, val, NULL, false, false);
+}
+EXPORT_SYMBOL_GPL(regmap_field_write);
+
+int regmap_field_force_write(struct regmap_field *field,  unsigned int val)
+{
+	return _regmap_field_write_or_update(field, val, NULL, false, true);
+}
+EXPORT_SYMBOL_GPL(regmap_field_force_write);
+
 /**
  * regmap_field_update_bits_base() - Perform a read/modify/write cycle a
  *                                   register field.
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 7cf2157134ac..08507e764dfa 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1307,6 +1307,8 @@ void devm_regmap_field_bulk_free(struct device *dev,
 				 struct regmap_field *field);
 
 int regmap_field_read(struct regmap_field *field, unsigned int *val);
+int regmap_field_write(struct regmap_field *field, unsigned int val);
+int regmap_field_force_write(struct regmap_field *field,  unsigned int val);
 int regmap_field_update_bits_base(struct regmap_field *field,
 				  unsigned int mask, unsigned int val,
 				  bool *change, bool async, bool force);
@@ -1316,19 +1318,6 @@ int regmap_fields_update_bits_base(struct regmap_field *field,  unsigned int id,
 				   unsigned int mask, unsigned int val,
 				   bool *change, bool async, bool force);
 
-static inline int regmap_field_write(struct regmap_field *field,
-				     unsigned int val)
-{
-	return regmap_field_update_bits_base(field, ~0, val,
-					     NULL, false, false);
-}
-
-static inline int regmap_field_force_write(struct regmap_field *field,
-					   unsigned int val)
-{
-	return regmap_field_update_bits_base(field, ~0, val, NULL, false, true);
-}
-
 static inline int regmap_field_update_bits(struct regmap_field *field,
 					   unsigned int mask, unsigned int val)
 {
-- 
2.34.1

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Mark Brown]

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

On Tue, Jul 19, 2022 at 02:14:46PM +0200, Krzysztof Kozlowski wrote:

> Extend regmap_field_write() and regmap_field_force_write() to check if
> register is readable and only then perform an update.  In the other
> case, it is expected that mask of field covers entire register thus a
> full write is allowed.

The other possible assumption there would be that the other bits are
write as zero - that's not 100% safe but does make sense if for example
the fields are being used to capture acknowledgement flags moving
around.  That's not incompatible with doing this of course, we can
always relax things later.

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

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Charles Keepax]

On Tue, Jul 19, 2022 at 02:14:46PM +0200, Krzysztof Kozlowski wrote:
> Current implementation of regmap_field_write() performs an update of
> register (read+write), therefore it ignores regmap read-restrictions and
> is not suitable for write-only registers (e.g. interrupt clearing).
> 
> Extend regmap_field_write() and regmap_field_force_write() to check if
> register is readable and only then perform an update.  In the other
> case, it is expected that mask of field covers entire register thus a
> full write is allowed.
> 
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> 
> ---
> 
> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
> ---
>  drivers/base/regmap/regmap.c | 50 ++++++++++++++++++++++++++++++++++++
>  include/linux/regmap.h       | 15 ++---------
>  2 files changed, 52 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 0caa5690c560..4d18a34f7b2c 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -2192,6 +2192,56 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg,
>  }
>  EXPORT_SYMBOL_GPL(regmap_noinc_write);
>  
> +static int _regmap_field_write_or_update(struct regmap_field *field,
> +					 unsigned int val, bool *change,
> +					 bool async, bool force)
> +{
> +	unsigned int mask = (~0 << field->shift) & field->mask;
> +	unsigned int map_val_mask, map_val_mask_h;
> +	int ret;
> +
> +	if (regmap_readable(field->regmap, field->reg))
> +		return regmap_update_bits_base(field->regmap, field->reg,
> +					       mask, val << field->shift,
> +					       change, async, force);
> +

I think this will break other valid use-cases, regmap_readable (I
believe) returns if the register is physically readable, however
it should still be possible to use update bits if the register is
in the cache even if it can't physically be read. So really you
need to fall into this path if it is readable or in the cache.

Which does I guess also raise the question if your problem would
be better solved with caching the register?

Thanks,
Charles

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Mark Brown]

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

On Tue, Jul 19, 2022 at 12:54:02PM +0000, Charles Keepax wrote:
> On Tue, Jul 19, 2022 at 02:14:46PM +0200, Krzysztof Kozlowski wrote:

> > +	if (regmap_readable(field->regmap, field->reg))
> > +		return regmap_update_bits_base(field->regmap, field->reg,
> > +					       mask, val << field->shift,
> > +					       change, async, force);

> I think this will break other valid use-cases, regmap_readable (I
> believe) returns if the register is physically readable, however
> it should still be possible to use update bits if the register is
> in the cache even if it can't physically be read. So really you
> need to fall into this path if it is readable or in the cache.

This is true, we don't currently have a readable_or_cached() check -
this is implemented as trying the cache and only considering actually
reading if there's a read, effectively what we want to do here is force
a cache only read.

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

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Krzysztof Kozlowski]

On 19/07/2022 14:54, Charles Keepax wrote:
> On Tue, Jul 19, 2022 at 02:14:46PM +0200, Krzysztof Kozlowski wrote:
>> Current implementation of regmap_field_write() performs an update of
>> register (read+write), therefore it ignores regmap read-restrictions and
>> is not suitable for write-only registers (e.g. interrupt clearing).
>>
>> Extend regmap_field_write() and regmap_field_force_write() to check if
>> register is readable and only then perform an update.  In the other
>> case, it is expected that mask of field covers entire register thus a
>> full write is allowed.
>>
>> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
>>
>> ---
>>
>> Cc: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>> Cc: Charles Keepax <ckeepax@opensource.cirrus.com>
>> Cc: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
>> Cc: Bjorn Andersson <bjorn.andersson@linaro.org>
>> ---
>>  drivers/base/regmap/regmap.c | 50 ++++++++++++++++++++++++++++++++++++
>>  include/linux/regmap.h       | 15 ++---------
>>  2 files changed, 52 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
>> index 0caa5690c560..4d18a34f7b2c 100644
>> --- a/drivers/base/regmap/regmap.c
>> +++ b/drivers/base/regmap/regmap.c
>> @@ -2192,6 +2192,56 @@ int regmap_noinc_write(struct regmap *map, unsigned int reg,
>>  }
>>  EXPORT_SYMBOL_GPL(regmap_noinc_write);
>>  
>> +static int _regmap_field_write_or_update(struct regmap_field *field,
>> +					 unsigned int val, bool *change,
>> +					 bool async, bool force)
>> +{
>> +	unsigned int mask = (~0 << field->shift) & field->mask;
>> +	unsigned int map_val_mask, map_val_mask_h;
>> +	int ret;
>> +
>> +	if (regmap_readable(field->regmap, field->reg))
>> +		return regmap_update_bits_base(field->regmap, field->reg,
>> +					       mask, val << field->shift,
>> +					       change, async, force);
>> +
> 
> I think this will break other valid use-cases, regmap_readable (I
> believe) returns if the register is physically readable, however
> it should still be possible to use update bits if the register is
> in the cache even if it can't physically be read. So really you
> need to fall into this path if it is readable or in the cache.

But what type of real use case this would be trying to solve? Either
register is readable or not. The presence of cache is just optimization
and does not change the fact that we cannot read from register thus no
need to go via updates.

> 
> Which does I guess also raise the question if your problem would
> be better solved with caching the register?

And how the value would appear in the cache? Since register cannot be
read, I expect the cache to be filled on first update. First update
would be read+write, so we are stuck again.


Best regards,
Krzysztof

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Mark Brown]

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

On Tue, Jul 19, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> On 19/07/2022 14:54, Charles Keepax wrote:

> > I think this will break other valid use-cases, regmap_readable (I
> > believe) returns if the register is physically readable, however
> > it should still be possible to use update bits if the register is
> > in the cache even if it can't physically be read. So really you
> > need to fall into this path if it is readable or in the cache.

> But what type of real use case this would be trying to solve? Either
> register is readable or not. The presence of cache is just optimization
> and does not change the fact that we cannot read from register thus no
> need to go via updates.

The original reason for creating the cache code was to simulate
readability on devices that have no read support at all (think 7x9
format I2C devices) so we can have things like helpers to map bitfields
directly to subsystems (like ASoC uses extensively).  The fact that it
also improves performance when the hardware does support reads is nice
too of course.

> > Which does I guess also raise the question if your problem would
> > be better solved with caching the register?

> And how the value would appear in the cache? Since register cannot be
> read, I expect the cache to be filled on first update. First update
> would be read+write, so we are stuck again.

This is one reason we allow cache defaults to be specified (it was the
original reason, we later started using them to optimise out I/O during
resyncs).

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

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Krzysztof Kozlowski]

On 19/07/2022 15:41, Mark Brown wrote:
> On Tue, Jul 19, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>> On 19/07/2022 14:54, Charles Keepax wrote:
> 
>>> I think this will break other valid use-cases, regmap_readable (I
>>> believe) returns if the register is physically readable, however
>>> it should still be possible to use update bits if the register is
>>> in the cache even if it can't physically be read. So really you
>>> need to fall into this path if it is readable or in the cache.
> 
>> But what type of real use case this would be trying to solve? Either
>> register is readable or not. The presence of cache is just optimization
>> and does not change the fact that we cannot read from register thus no
>> need to go via updates.
> 
> The original reason for creating the cache code was to simulate
> readability on devices that have no read support at all (think 7x9
> format I2C devices) so we can have things like helpers to map bitfields
> directly to subsystems (like ASoC uses extensively).  The fact that it
> also improves performance when the hardware does support reads is nice
> too of course.
> 
>>> Which does I guess also raise the question if your problem would
>>> be better solved with caching the register?
> 
>> And how the value would appear in the cache? Since register cannot be
>> read, I expect the cache to be filled on first update. First update
>> would be read+write, so we are stuck again.
> 
> This is one reason we allow cache defaults to be specified (it was the
> original reason, we later started using them to optimise out I/O during
> resyncs).

Thanks Mark and Charles. Let me try the cache.

Best regards,
Krzysztof

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Krzysztof Kozlowski]

On 19/07/2022 16:30, Krzysztof Kozlowski wrote:
> On 19/07/2022 15:41, Mark Brown wrote:
>> On Tue, Jul 19, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
>>> On 19/07/2022 14:54, Charles Keepax wrote:
>>
>>>> I think this will break other valid use-cases, regmap_readable (I
>>>> believe) returns if the register is physically readable, however
>>>> it should still be possible to use update bits if the register is
>>>> in the cache even if it can't physically be read. So really you
>>>> need to fall into this path if it is readable or in the cache.
>>
>>> But what type of real use case this would be trying to solve? Either
>>> register is readable or not. The presence of cache is just optimization
>>> and does not change the fact that we cannot read from register thus no
>>> need to go via updates.
>>
>> The original reason for creating the cache code was to simulate
>> readability on devices that have no read support at all (think 7x9
>> format I2C devices) so we can have things like helpers to map bitfields
>> directly to subsystems (like ASoC uses extensively).  The fact that it
>> also improves performance when the hardware does support reads is nice
>> too of course.
>>
>>>> Which does I guess also raise the question if your problem would
>>>> be better solved with caching the register?
>>
>>> And how the value would appear in the cache? Since register cannot be
>>> read, I expect the cache to be filled on first update. First update
>>> would be read+write, so we are stuck again.
>>
>> This is one reason we allow cache defaults to be specified (it was the
>> original reason, we later started using them to optimise out I/O during
>> resyncs).
> 
> Thanks Mark and Charles. Let me try the cache.

cache + forced write works for me, so I guess this patch is not really
necessary.

Best regards,
Krzysztof

Re: [PATCH] regmap: support regmap_field_write() on non-readable fields

[Charles Keepax]

On Tue, Jul 19, 2022 at 03:13:11PM +0200, Krzysztof Kozlowski wrote:
> On 19/07/2022 14:54, Charles Keepax wrote:
> > On Tue, Jul 19, 2022 at 02:14:46PM +0200, Krzysztof Kozlowski wrote:
> >> Current implementation of regmap_field_write() performs an update of
> >> register (read+write), therefore it ignores regmap read-restrictions and
> >> is not suitable for write-only registers (e.g. interrupt clearing).
> >>
> >> Extend regmap_field_write() and regmap_field_force_write() to check if
> >> register is readable and only then perform an update.  In the other
> >> case, it is expected that mask of field covers entire register thus a
> >> full write is allowed.
> >>
> >> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> >>
> >> ---
> >> +	if (regmap_readable(field->regmap, field->reg))
> >> +		return regmap_update_bits_base(field->regmap, field->reg,
> >> +					       mask, val << field->shift,
> >> +					       change, async, force);
> >> +
> > 
> > I think this will break other valid use-cases, regmap_readable (I
> > believe) returns if the register is physically readable, however
> > it should still be possible to use update bits if the register is
> > in the cache even if it can't physically be read. So really you
> > need to fall into this path if it is readable or in the cache.
> 
> But what type of real use case this would be trying to solve? Either
> register is readable or not. The presence of cache is just optimization
> and does not change the fact that we cannot read from register thus no
> need to go via updates.
> 
> > 
> > Which does I guess also raise the question if your problem would
> > be better solved with caching the register?
> 
> And how the value would appear in the cache? Since register cannot be
> read, I expect the cache to be filled on first update. First update
> would be read+write, so we are stuck again.
> 

The cache is initialised from the defaults table, so normal
proceedure for unreadable registers is you start out with the
hardware default and since you know when you wrote it the value
stays in sync.

Thanks,
Charles