[PATCH 1/3] regmap: flat: use the cache_present bitmap

[PATCH 1/3] regmap: flat: use the cache_present bitmap

[Ionut Nicu]

As opposed to the other regmap cache implementations,
regcache_flat didn't use the cache_present bitmap for
figuring out whether a register was cached or not, nor
did it mark a register as present in the cache when
regcache_flat_write() was called.

This caused incorrect behaviour, such as returning
value 0 for non-volatile registers without first reading
their value from the device and storing it in the cache.

Signed-off-by: Ionut Nicu <ioan.nicu.ext@nsn.com>
---
 drivers/base/regmap/regcache-flat.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/base/regmap/regcache-flat.c b/drivers/base/regmap/regcache-flat.c
index d9762e4..f47278e 100644
--- a/drivers/base/regmap/regcache-flat.c
+++ b/drivers/base/regmap/regcache-flat.c
@@ -16,9 +16,13 @@
 
 #include "internal.h"
 
+static int regcache_flat_write(struct regmap *map, unsigned int reg,
+			       unsigned int value);
+static int regcache_flat_exit(struct regmap *map);
+
 static int regcache_flat_init(struct regmap *map)
 {
-	int i;
+	int i, ret;
 	unsigned int *cache;
 
 	map->cache = kzalloc(sizeof(unsigned int) * (map->max_register + 1),
@@ -28,8 +32,15 @@ static int regcache_flat_init(struct regmap *map)
 
 	cache = map->cache;
 
-	for (i = 0; i < map->num_reg_defaults; i++)
-		cache[map->reg_defaults[i].reg] = map->reg_defaults[i].def;
+	for (i = 0; i < map->num_reg_defaults; i++) {
+		ret = regcache_flat_write(map,
+				map->reg_defaults[i].reg,
+				map->reg_defaults[i].def);
+		if (ret) {
+			regcache_flat_exit(map);
+			return ret;
+		}
+	}
 
 	return 0;
 }
@@ -47,6 +58,9 @@ static int regcache_flat_read(struct regmap *map,
 {
 	unsigned int *cache = map->cache;
 
+	if (!regcache_reg_present(map, reg))
+		return -ENOENT;
+
 	*value = cache[reg];
 
 	return 0;
@@ -56,6 +70,11 @@ static int regcache_flat_write(struct regmap *map, unsigned int reg,
 			       unsigned int value)
 {
 	unsigned int *cache = map->cache;
+	int ret;
+
+	ret = regcache_set_reg_present(map, reg);
+	if (ret < 0)
+		return ret;
 
 	cache[reg] = value;
 
-- 
1.7.1

Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap

[Mark Brown]

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

On Fri, Aug 09, 2013 at 12:09:11PM +0200, Ionut Nicu wrote:
> As opposed to the other regmap cache implementations,
> regcache_flat didn't use the cache_present bitmap for
> figuring out whether a register was cached or not, nor
> did it mark a register as present in the cache when
> regcache_flat_write() was called.

> This caused incorrect behaviour, such as returning
> value 0 for non-volatile registers without first reading
> their value from the device and storing it in the cache.

The goal with the flat cache is to do as little work as possible for
things like memory mapped devices where the cache operations are
actually noticable in comparison with the I/O costs.  I would therefore
exapect that anything using the flat cache would want to ensure that the
cache is fully populated at init time, reading back from the device if
nothing else (by setting num_reg_defaults_raw but not providing values),
rather than do additional operations in the data path.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap

[Ionut Nicu]

On 09.08.2013 13:44, ext Mark Brown wrote:
> On Fri, Aug 09, 2013 at 12:09:11PM +0200, Ionut Nicu wrote:
>> As opposed to the other regmap cache implementations,
>> regcache_flat didn't use the cache_present bitmap for
>> figuring out whether a register was cached or not, nor
>> did it mark a register as present in the cache when
>> regcache_flat_write() was called.
> 
>> This caused incorrect behaviour, such as returning
>> value 0 for non-volatile registers without first reading
>> their value from the device and storing it in the cache.
> 
> The goal with the flat cache is to do as little work as possible for
> things like memory mapped devices where the cache operations are
> actually noticable in comparison with the I/O costs.  I would therefore
> exapect that anything using the flat cache would want to ensure that the
> cache is fully populated at init time, reading back from the device if
> nothing else (by setting num_reg_defaults_raw but not providing values),
> rather than do additional operations in the data path.
> 

Ok, I get your point. I've tried using this approach, but the thing is
that I have a device which supports only single rw operations. The
regcache_hw_init() function calls regmap_raw_read which in turn calls
_regmap_raw_read() when cache_bypass is enabled. But _regmap_raw_read()
doesn't take the use_single_rw flag into account and tries to read
num_reg_defaults_raw registers in a single bus transfer. Unfortunately
this doesn't work with my device. 

I thought about changing regcache_hw_init() to check for this flag in
the same way regmap_bulk_read() does, but I think this is pretty ugly.

What do you think about moving the check to use_single_rw inside
_regmap_raw_read(), right where the bus accesses are done?

Something like in the following patch:


diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 42b45ac..de7f353 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1486,14 +1486,16 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 {
 	struct regmap_range_node *range;
 	u8 *u8 = map->work_buf;
-	int ret;
+	size_t val_bytes = map->format.val_bytes;
+	size_t reg_size = map->format.reg_bytes + map->format.pad_bytes;
+	int ret, i;
 
 	WARN_ON(!map->bus);
 
 	range = _regmap_range_lookup(map, reg);
 	if (range) {
 		ret = _regmap_select_page(map, &reg, range,
-					  val_len / map->format.val_bytes);
+					  val_len / val_bytes);
 		if (ret != 0)
 			return ret;
 	}
@@ -1508,15 +1510,41 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 	 */
 	u8[0] |= map->read_flag_mask;
 
-	trace_regmap_hw_read_start(map->dev, reg,
-				   val_len / map->format.val_bytes);
+	/*
+	 * Some devices does not support bulk read, for
+	 * them we have a series of single read operations.
+	 */
+	if (map->use_single_rw) {
+		for (i = 0; i < val_len; i++) {
+			trace_regmap_hw_read_start(map->dev,
+					reg + (i * map->reg_stride), 1);
+
+			map->format.format_reg(map->work_buf,
+					reg + (i *  map->reg_stride),
+					map->reg_shift);
+			u8[0] |= map->read_flag_mask;
+
+			ret = map->bus->read(map->bus_context,
+					map->work_buf,
+					reg_size,
+					val + (i * val_bytes),
+					val_bytes);
+			if (ret != 0)
+				return ret;
 
-	ret = map->bus->read(map->bus_context, map->work_buf,
-			     map->format.reg_bytes + map->format.pad_bytes,
-			     val, val_len);
+			trace_regmap_hw_read_done(map->dev,
+					reg + (i * map->reg_stride), 1);
+		}
+	} else {
+		trace_regmap_hw_read_start(map->dev, reg, val_len / val_bytes);
 
-	trace_regmap_hw_read_done(map->dev, reg,
-				  val_len / map->format.val_bytes);
+		ret = map->bus->read(map->bus_context, map->work_buf,
+				reg_size, val, val_len);
+		if (ret != 0)
+			return ret;
+
+		trace_regmap_hw_read_done(map->dev, reg, val_len / val_bytes);
+	}
 
 	return ret;
 }
@@ -1702,25 +1730,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 		return -EINVAL;
 
 	if (vol || map->cache_type == REGCACHE_NONE) {
-		/*
-		 * Some devices does not support bulk read, for
-		 * them we have a series of single read operations.
-		 */
-		if (map->use_single_rw) {
-			for (i = 0; i < val_count; i++) {
-				ret = regmap_raw_read(map,
-						reg + (i * map->reg_stride),
-						val + (i * val_bytes),
-						val_bytes);
-				if (ret != 0)
-					return ret;
-			}
-		} else {
-			ret = regmap_raw_read(map, reg, val,
-					      val_bytes * val_count);
-			if (ret != 0)
-				return ret;
-		}
+		ret = regmap_raw_read(map, reg, val, val_bytes * val_count);
+		if (ret != 0)
+			return ret;
 
 		for (i = 0; i < val_count * val_bytes; i += val_bytes)
 			map->format.parse_inplace(val + i);

Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap

[Ionut Nicu]

Hi Mark,

On 09.08.2013 19:16, Ionut Nicu wrote:
> On 09.08.2013 13:44, ext Mark Brown wrote:
>> On Fri, Aug 09, 2013 at 12:09:11PM +0200, Ionut Nicu wrote:
>>> As opposed to the other regmap cache implementations,
>>> regcache_flat didn't use the cache_present bitmap for
>>> figuring out whether a register was cached or not, nor
>>> did it mark a register as present in the cache when
>>> regcache_flat_write() was called.
>>
>>> This caused incorrect behaviour, such as returning
>>> value 0 for non-volatile registers without first reading
>>> their value from the device and storing it in the cache.
>>
>> The goal with the flat cache is to do as little work as possible for
>> things like memory mapped devices where the cache operations are
>> actually noticable in comparison with the I/O costs.  I would therefore
>> exapect that anything using the flat cache would want to ensure that the
>> cache is fully populated at init time, reading back from the device if
>> nothing else (by setting num_reg_defaults_raw but not providing values),
>> rather than do additional operations in the data path.
>>
> 
> Ok, I get your point. I've tried using this approach, but the thing is
> that I have a device which supports only single rw operations. The
> regcache_hw_init() function calls regmap_raw_read which in turn calls
> _regmap_raw_read() when cache_bypass is enabled. But _regmap_raw_read()
> doesn't take the use_single_rw flag into account and tries to read
> num_reg_defaults_raw registers in a single bus transfer. Unfortunately
> this doesn't work with my device. 
> 
> I thought about changing regcache_hw_init() to check for this flag in
> the same way regmap_bulk_read() does, but I think this is pretty ugly.
> 
> What do you think about moving the check to use_single_rw inside
> _regmap_raw_read(), right where the bus accesses are done?
> 
> Something like in the following patch:
> 
> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 42b45ac..de7f353 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1486,14 +1486,16 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
>  {
>  	struct regmap_range_node *range;
>  	u8 *u8 = map->work_buf;
> -	int ret;
> +	size_t val_bytes = map->format.val_bytes;
> +	size_t reg_size = map->format.reg_bytes + map->format.pad_bytes;
> +	int ret, i;
>  
>  	WARN_ON(!map->bus);
>  
>  	range = _regmap_range_lookup(map, reg);
>  	if (range) {
>  		ret = _regmap_select_page(map, &reg, range,
> -					  val_len / map->format.val_bytes);
> +					  val_len / val_bytes);
>  		if (ret != 0)
>  			return ret;
>  	}
> @@ -1508,15 +1510,41 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
>  	 */
>  	u8[0] |= map->read_flag_mask;
>  
> -	trace_regmap_hw_read_start(map->dev, reg,
> -				   val_len / map->format.val_bytes);
> +	/*
> +	 * Some devices does not support bulk read, for
> +	 * them we have a series of single read operations.
> +	 */
> +	if (map->use_single_rw) {
> +		for (i = 0; i < val_len; i++) {
> +			trace_regmap_hw_read_start(map->dev,
> +					reg + (i * map->reg_stride), 1);
> +
> +			map->format.format_reg(map->work_buf,
> +					reg + (i *  map->reg_stride),
> +					map->reg_shift);
> +			u8[0] |= map->read_flag_mask;
> +
> +			ret = map->bus->read(map->bus_context,
> +					map->work_buf,
> +					reg_size,
> +					val + (i * val_bytes),
> +					val_bytes);
> +			if (ret != 0)
> +				return ret;
>  
> -	ret = map->bus->read(map->bus_context, map->work_buf,
> -			     map->format.reg_bytes + map->format.pad_bytes,
> -			     val, val_len);
> +			trace_regmap_hw_read_done(map->dev,
> +					reg + (i * map->reg_stride), 1);
> +		}
> +	} else {
> +		trace_regmap_hw_read_start(map->dev, reg, val_len / val_bytes);
>  
> -	trace_regmap_hw_read_done(map->dev, reg,
> -				  val_len / map->format.val_bytes);
> +		ret = map->bus->read(map->bus_context, map->work_buf,
> +				reg_size, val, val_len);
> +		if (ret != 0)
> +			return ret;
> +
> +		trace_regmap_hw_read_done(map->dev, reg, val_len / val_bytes);
> +	}
>  
>  	return ret;
>  }
> @@ -1702,25 +1730,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
>  		return -EINVAL;
>  
>  	if (vol || map->cache_type == REGCACHE_NONE) {
> -		/*
> -		 * Some devices does not support bulk read, for
> -		 * them we have a series of single read operations.
> -		 */
> -		if (map->use_single_rw) {
> -			for (i = 0; i < val_count; i++) {
> -				ret = regmap_raw_read(map,
> -						reg + (i * map->reg_stride),
> -						val + (i * val_bytes),
> -						val_bytes);
> -				if (ret != 0)
> -					return ret;
> -			}
> -		} else {
> -			ret = regmap_raw_read(map, reg, val,
> -					      val_bytes * val_count);
> -			if (ret != 0)
> -				return ret;
> -		}
> +		ret = regmap_raw_read(map, reg, val, val_bytes * val_count);
> +		if (ret != 0)
> +			return ret;
>  
>  		for (i = 0; i < val_count * val_bytes; i += val_bytes)
>  			map->format.parse_inplace(val + i);
> 

Do you have any feedback for this patch? 	

If you think the change is too complex, I have a simpler solution that could make
things work with devices that set use_single_rw. Instead of enabling cache_bypass
in regcache_hw_init(), we could temporarily set map->cache_type to REGCACHE_NONE
and then call regmap_bulk_read() instead of regmap_raw_read().

Please let me know your opinion on any of the two solutions.

Best regards,
Ionut Nicu.

Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap

[Mark Brown]

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

On Tue, Aug 13, 2013 at 09:17:12AM +0200, Ionut Nicu wrote:

> Do you have any feedback for this patch? 	

Not yet, please allow some time for response especially over weekends.

> If you think the change is too complex, I have a simpler solution that could make
> things work with devices that set use_single_rw. Instead of enabling cache_bypass
> in regcache_hw_init(), we could temporarily set map->cache_type to REGCACHE_NONE
> and then call regmap_bulk_read() instead of regmap_raw_read().

Well, we probably don't want to be doing raw reads on random devices
though - they should really only be being done if the device can
actuallly support them.  Too much room for confusion otherwise.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: Fwd: Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap

[Alexander Sverdlin]

Hello!

On 08/19/2013 10:25 AM, Ionut Nicu wrote:
> -------- Original Message --------
> Subject: Re: [PATCH 1/3] regmap: flat: use the cache_present bitmap
> Date: Fri, 09 Aug 2013 19:16:49 +0200
> From: Ionut Nicu <ioan.nicu.ext@nsn.com>
> To: ext Mark Brown <broonie@kernel.org>
> CC: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,  linux-kernel@vger.kernel.org
> 
> On 09.08.2013 13:44, ext Mark Brown wrote:
>> On Fri, Aug 09, 2013 at 12:09:11PM +0200, Ionut Nicu wrote:
>>> As opposed to the other regmap cache implementations,
>>> regcache_flat didn't use the cache_present bitmap for
>>> figuring out whether a register was cached or not, nor
>>> did it mark a register as present in the cache when
>>> regcache_flat_write() was called.
>>
>>> This caused incorrect behaviour, such as returning
>>> value 0 for non-volatile registers without first reading
>>> their value from the device and storing it in the cache.
>>
>> The goal with the flat cache is to do as little work as possible for
>> things like memory mapped devices where the cache operations are
>> actually noticable in comparison with the I/O costs.  I would therefore
>> exapect that anything using the flat cache would want to ensure that the
>> cache is fully populated at init time, reading back from the device if
>> nothing else (by setting num_reg_defaults_raw but not providing values),
>> rather than do additional operations in the data path.
>>
> 
> Ok, I get your point. I've tried using this approach, but the thing is
> that I have a device which supports only single rw operations. The
> regcache_hw_init() function calls regmap_raw_read which in turn calls
> _regmap_raw_read() when cache_bypass is enabled. But _regmap_raw_read()
> doesn't take the use_single_rw flag into account and tries to read
> num_reg_defaults_raw registers in a single bus transfer. Unfortunately
> this doesn't work with my device. 
> 
> I thought about changing regcache_hw_init() to check for this flag in
> the same way regmap_bulk_read() does, but I think this is pretty ugly.
> 
> What do you think about moving the check to use_single_rw inside
> _regmap_raw_read(), right where the bus accesses are done?
> 
> Something like in the following patch:

Acked-by: Alexander Sverdlin <alexander.sverdlin@nsn.com>

> 
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 42b45ac..de7f353 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -1486,14 +1486,16 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
>  {
>  	struct regmap_range_node *range;
>  	u8 *u8 = map->work_buf;
> -	int ret;
> +	size_t val_bytes = map->format.val_bytes;
> +	size_t reg_size = map->format.reg_bytes + map->format.pad_bytes;
> +	int ret, i;
>  
>  	WARN_ON(!map->bus);
>  
>  	range = _regmap_range_lookup(map, reg);
>  	if (range) {
>  		ret = _regmap_select_page(map, &reg, range,
> -					  val_len / map->format.val_bytes);
> +					  val_len / val_bytes);
>  		if (ret != 0)
>  			return ret;
>  	}
> @@ -1508,15 +1510,41 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
>  	 */
>  	u8[0] |= map->read_flag_mask;
>  
> -	trace_regmap_hw_read_start(map->dev, reg,
> -				   val_len / map->format.val_bytes);
> +	/*
> +	 * Some devices does not support bulk read, for
> +	 * them we have a series of single read operations.
> +	 */
> +	if (map->use_single_rw) {
> +		for (i = 0; i < val_len; i++) {
> +			trace_regmap_hw_read_start(map->dev,
> +					reg + (i * map->reg_stride), 1);
> +
> +			map->format.format_reg(map->work_buf,
> +					reg + (i *  map->reg_stride),
> +					map->reg_shift);
> +			u8[0] |= map->read_flag_mask;
> +
> +			ret = map->bus->read(map->bus_context,
> +					map->work_buf,
> +					reg_size,
> +					val + (i * val_bytes),
> +					val_bytes);
> +			if (ret != 0)
> +				return ret;
>  
> -	ret = map->bus->read(map->bus_context, map->work_buf,
> -			     map->format.reg_bytes + map->format.pad_bytes,
> -			     val, val_len);
> +			trace_regmap_hw_read_done(map->dev,
> +					reg + (i * map->reg_stride), 1);
> +		}
> +	} else {
> +		trace_regmap_hw_read_start(map->dev, reg, val_len / val_bytes);
>  
> -	trace_regmap_hw_read_done(map->dev, reg,
> -				  val_len / map->format.val_bytes);
> +		ret = map->bus->read(map->bus_context, map->work_buf,
> +				reg_size, val, val_len);
> +		if (ret != 0)
> +			return ret;
> +
> +		trace_regmap_hw_read_done(map->dev, reg, val_len / val_bytes);
> +	}
>  
>  	return ret;
>  }
> @@ -1702,25 +1730,9 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
>  		return -EINVAL;
>  
>  	if (vol || map->cache_type == REGCACHE_NONE) {
> -		/*
> -		 * Some devices does not support bulk read, for
> -		 * them we have a series of single read operations.
> -		 */
> -		if (map->use_single_rw) {
> -			for (i = 0; i < val_count; i++) {
> -				ret = regmap_raw_read(map,
> -						reg + (i * map->reg_stride),
> -						val + (i * val_bytes),
> -						val_bytes);
> -				if (ret != 0)
> -					return ret;
> -			}
> -		} else {
> -			ret = regmap_raw_read(map, reg, val,
> -					      val_bytes * val_count);
> -			if (ret != 0)
> -				return ret;
> -		}
> +		ret = regmap_raw_read(map, reg, val, val_bytes * val_count);
> +		if (ret != 0)
> +			return ret;
>  
>  		for (i = 0; i < val_count * val_bytes; i += val_bytes)
>  			map->format.parse_inplace(val + i);
> 
> 
> 
> 
> 

-- 
Best regards,
Alexander Sverdlin.