regmap: flat: use the cache_present bitmap

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.

Quoted 9 lines · 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.

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.

Quoted 11 lines · ext Mark Brown

Quoted 9 lines · 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.

Quoted 8 lines

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:

drivers/base/regmap/regmap.c +37 -24

index 42b45ac..de7f353 100644
@@ -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,

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().

-------- 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 <[email protected]>
To: ext Mark Brown <[email protected]>
CC: Greg Kroah-Hartman <[email protected]>, [email protected]

Quoted 11 lines · ext Mark Brown

Quoted 9 lines · 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.

Quoted 8 lines

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:

drivers/base/regmap/regmap.c +40 -28

index 42b45ac..de7f353 100644
@@ -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);