Skip to contentSkip to conversation

Introduce caching support for regmap

PATCH In review Dimitris Papastamos 3 participants 19 messages Sep 19, 2011

Subsystems (3)
lore
Share & tools
lore
#0 lore raw

v5 cover Introduce caching support for regmap

This patch series introduces register caching support for regmap. I've mostly implemented all the suggestions, except the use of a dedicated init function for each of the compress types.

The only change since v4 is that I've renamed all instances of cache_defaults' to reg_defaults'.

Some things have been left out, such as support for bulk read operations, various optimizations, shared caches etc.
Most of this stuff will be implemented incrementally.

This version has only been build tested, so any testing would be highly appreciated. The changes though since v3 are trivial so it should work.

Thanks to Lars-Peter Clausen and Mark Brown for all the constructive comments.

Dimitris Papastamos (6):
regmap: Introduce caching support
regmap: Add the indexed cache support
regmap: Add the rbtree cache support
regmap: Add the LZO cache support
regmap: Add the regcache_sync trace event
regmap: Incorporate the regcache core into regmap

 drivers/base/regmap/Kconfig            |    2 +
 drivers/base/regmap/Makefile           |    2 +-
 drivers/base/regmap/internal.h         |   56 +++++
 drivers/base/regmap/regcache-indexed.c |   65 +++++
 drivers/base/regmap/regcache-lzo.c     |  361 +++++++++++++++++++++++++++++
 drivers/base/regmap/regcache-rbtree.c  |  399 ++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c         |  311 +++++++++++++++++++++++++
 drivers/base/regmap/regmap.c           |   28 +++
 include/linux/regmap.h                 |   18 ++-
 include/trace/events/regmap.h          |   24 ++
 10 files changed, 1264 insertions(+), 2 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-indexed.c
 create mode 100644 drivers/base/regmap/regcache-lzo.c
 create mode 100644 drivers/base/regmap/regcache-rbtree.c
 create mode 100644 drivers/base/regmap/regcache.c
Signature · 1 line
1.7.6.1
#1 lore raw

PATCH v5 1/6 regmap: Introduce caching support

This patch introduces caching support for regmap. The regcache API has evolved essentially out of ASoC soc-cache so most of the actual caching types (except LZO) have been tested in the past.

The purpose of regcache is to optimize in time and space the handling of register caches. Time optimization is achieved by not having to go over a slow bus like I2C to read the value of a register, instead it is cached locally in memory and can be retrieved faster. Regarding space optimization, some of the cache types are better at packing the caches, for e.g. the rbtree and the LZO caches. By doing this the sacrifice in time still wins over doing I2C transactions.

---
 drivers/base/regmap/Makefile   |    2 +-
 drivers/base/regmap/internal.h |   52 +++++++
 drivers/base/regmap/regcache.c |  302 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h         |   15 ++-
 4 files changed, 369 insertions(+), 2 deletions(-)
 create mode 100644 drivers/base/regmap/regcache.c
drivers/base/regmap/Makefile +1 -1
index 057c13f..2e103ea 100644
@@ -1,4 +1,4 @@
-obj-$(CONFIG_REGMAP) += regmap.o
+obj-$(CONFIG_REGMAP) += regmap.o regcache.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
 obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
drivers/base/regmap/internal.h +52 -0

68 lines not on this page · raw message

drivers/base/regmap/regcache.c new file +302 -0

302 lines not on this page · raw message

include/linux/regmap.h +14 -1
index 18d4afa..9d80294 100644
@@ -20,6 +20,11 @@
 struct i2c_client;
 struct spi_device;
 
+/* An enum of all the supported cache types */
+enum regcache_type {
+	REGCACHE_NONE,
+};
+
 /**
  * Default value for a register.  We use an array of structs rather
  * than a simple array as many modern devices have very sparse
@@ -59,6 +64,11 @@ struct reg_default {
  * @write_flag_mask: Mask to be set in the top byte of the register when doing
  *                   a write. If both read_flag_mask and write_flag_mask are
  *                   empty the regmap_bus default masks are used.
+ *
+ * @cache_type: The actual cache type.
+ * @reg_defaults_raw: Power on reset values for registers (for use with
+ *                    register cache support).
+ * @num_reg_defaults_raw: Number of elements in reg_defaults_raw.
  */
 struct regmap_config {
 	int reg_bits;
@@ -71,7 +81,10 @@ struct regmap_config {
 
 	unsigned int max_register;
 	struct reg_default *reg_defaults;
-	int num_reg_defaults;
+	unsigned int num_reg_defaults;
+	enum regcache_type cache_type;
+	const void *reg_defaults_raw;
+	unsigned int num_reg_defaults_raw;
 
 	u8 read_flag_mask;
 	u8 write_flag_mask;
Signature · 1 line
1.7.6.1
#2 lore raw

PATCH v5 2/6 regmap: Add the indexed cache support

In reply to Dimitris Papastamos

This is the simplest form of a cache available in regcache. Any registers whose default value is 0 are ignored. If any of those registers are modified in the future, they will be placed in the cache on demand. The cache layout is essentially using the provided register defaults by the regcache core directly and does not re-map it to another representation.

---
 drivers/base/regmap/Makefile           |    2 +-
 drivers/base/regmap/internal.h         |    1 +
 drivers/base/regmap/regcache-indexed.c |   65 ++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c         |    1 +
 include/linux/regmap.h                 |    1 +
 5 files changed, 69 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-indexed.c
drivers/base/regmap/Makefile +1 -1
index 2e103ea..418d151 100644
@@ -1,4 +1,4 @@
-obj-$(CONFIG_REGMAP) += regmap.o regcache.o
+obj-$(CONFIG_REGMAP) += regmap.o regcache.o regcache-indexed.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
 obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
drivers/base/regmap/internal.h +1 -0
index 615f558..5bd5759 100644
@@ -118,4 +118,5 @@ int regcache_lookup_reg(struct regmap *map, unsigned int reg);
 int regcache_insert_reg(struct regmap *map, unsigned int reg,
 			unsigned int val);
 
+extern struct regcache_ops regcache_indexed_ops;
 #endif
drivers/base/regmap/regcache-indexed.c new file +65 -0

65 lines not on this page · raw message

drivers/base/regmap/regcache.c +1 -0
index b33c547..6cf653d 100644
@@ -16,6 +16,7 @@
 #include "internal.h"
 
 static const struct regcache_ops *cache_types[] = {
+	&regcache_indexed_ops,
 };
 
 static int regcache_hw_init(struct regmap *map)
include/linux/regmap.h +1 -0
index 9d80294..ae6d3a4 100644
@@ -23,6 +23,7 @@ struct spi_device;
 /* An enum of all the supported cache types */
 enum regcache_type {
 	REGCACHE_NONE,
+	REGCACHE_INDEXED,
 };
 
 /**
Signature · 1 line
1.7.6.1
#3 lore raw

PATCH v5 3/6 regmap: Add the rbtree cache support

In reply to Dimitris Papastamos

This patch adds support for the rbtree cache compression type.

Each rbnode manages a variable length block of registers. There can be no two nodes with overlapping blocks. Each block has a base register and a currently top register, all the other registers, if any, lie in between these two and in ascending order.

The reasoning behind the construction of this rbtree is simple. In the snd_soc_rbtree_cache_init() function, we iterate over the register defaults provided by the regcache core. For each register value that is non-zero we insert it in the rbtree. In order to determine in which rbnode we need to add the register, we first look if there is another register already added that is adjacent to the one we are about to add. If that is the case we append it in that rbnode block, otherwise we create a new rbnode with a single register in its block and add it to the tree.

There are various optimizations across the implementation to speed up lookups by caching the most recently used rbnode.

---
 drivers/base/regmap/Makefile          |    2 +-
 drivers/base/regmap/internal.h        |    1 +
 drivers/base/regmap/regcache-rbtree.c |  399 +++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c        |    1 +
 include/linux/regmap.h                |    1 +
 5 files changed, 403 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-rbtree.c
drivers/base/regmap/Makefile +1 -1
index 418d151..20cd650 100644
@@ -1,4 +1,4 @@
-obj-$(CONFIG_REGMAP) += regmap.o regcache.o regcache-indexed.o
+obj-$(CONFIG_REGMAP) += regmap.o regcache.o regcache-indexed.o regcache-rbtree.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
 obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
drivers/base/regmap/internal.h +1 -0
index 5bd5759..7ef8afc 100644
@@ -119,4 +119,5 @@ int regcache_insert_reg(struct regmap *map, unsigned int reg,
 			unsigned int val);
 
 extern struct regcache_ops regcache_indexed_ops;
+extern struct regcache_ops regcache_rbtree_ops;
 #endif
drivers/base/regmap/regcache-rbtree.c new file +399 -0

399 lines not on this page · raw message

drivers/base/regmap/regcache.c +1 -0
index 6cf653d..cb96968 100644
@@ -17,6 +17,7 @@
 
 static const struct regcache_ops *cache_types[] = {
 	&regcache_indexed_ops,
+	&regcache_rbtree_ops,
 };
 
 static int regcache_hw_init(struct regmap *map)
include/linux/regmap.h +1 -0
index ae6d3a4..63c4a5e 100644
@@ -24,6 +24,7 @@ struct spi_device;
 enum regcache_type {
 	REGCACHE_NONE,
 	REGCACHE_INDEXED,
+	REGCACHE_RBTREE,
 };
 
 /**
Signature · 1 line
1.7.6.1
#4 lore raw

PATCH v5 4/6 regmap: Add the LZO cache support

In reply to Dimitris Papastamos

This patch adds support for LZO compression when storing the register cache.

For a typical device whose register map would normally occupy 25kB or 50kB by using the LZO compression technique, one can get down to ~5-7kB. There might be a performance penalty associated with each individual read/write due to decompressing/compressing the underlying cache, however that should not be noticeable. These memory benefits depend on whether the target architecture can get rid of the memory occupied by the original register defaults cache which is marked as __devinitconst. Nevertheless there will be some memory gain even if the target architecture can't get rid of the original register map, this should be around ~30-32kB instead of 50kB.

---
 drivers/base/regmap/Kconfig        |    2 +
 drivers/base/regmap/Makefile       |    2 +-
 drivers/base/regmap/internal.h     |    2 +
 drivers/base/regmap/regcache-lzo.c |  361 ++++++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c     |    1 +
 include/linux/regmap.h             |    1 +
 6 files changed, 368 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-lzo.c
drivers/base/regmap/Kconfig +2 -0
index fabbf6c..2fc6a66 100644
@@ -4,6 +4,8 @@
 
 config REGMAP
 	default y if (REGMAP_I2C || REGMAP_SPI)
+	select LZO_COMPRESS
+	select LZO_DECOMPRESS
 	bool
 
 config REGMAP_I2C
drivers/base/regmap/Makefile +1 -1
index 20cd650..0573c8a 100644
@@ -1,4 +1,4 @@
-obj-$(CONFIG_REGMAP) += regmap.o regcache.o regcache-indexed.o regcache-rbtree.o
+obj-$(CONFIG_REGMAP) += regmap.o regcache.o regcache-indexed.o regcache-rbtree.o regcache-lzo.o
 obj-$(CONFIG_DEBUG_FS) += regmap-debugfs.o
 obj-$(CONFIG_REGMAP_I2C) += regmap-i2c.o
 obj-$(CONFIG_REGMAP_SPI) += regmap-spi.o
drivers/base/regmap/internal.h +2 -0
index 7ef8afc..2d51b1b 100644
@@ -120,4 +120,6 @@ int regcache_insert_reg(struct regmap *map, unsigned int reg,
 
 extern struct regcache_ops regcache_indexed_ops;
 extern struct regcache_ops regcache_rbtree_ops;
+extern struct regcache_ops regcache_lzo_ops;
+
 #endif
drivers/base/regmap/regcache-lzo.c new file +361 -0

361 lines not on this page · raw message

drivers/base/regmap/regcache.c +1 -0
index cb96968..1e4849b 100644
@@ -18,6 +18,7 @@
 static const struct regcache_ops *cache_types[] = {
 	&regcache_indexed_ops,
 	&regcache_rbtree_ops,
+	&regcache_lzo_ops,
 };
 
 static int regcache_hw_init(struct regmap *map)
include/linux/regmap.h +1 -0
index 63c4a5e..cae69e6 100644
@@ -25,6 +25,7 @@ enum regcache_type {
 	REGCACHE_NONE,
 	REGCACHE_INDEXED,
 	REGCACHE_RBTREE,
+	REGCACHE_LZO
 };
 
 /**
Signature · 1 line
1.7.6.1
#5 lore raw

PATCH v5 5/6 regmap: Add the regcache_sync trace event

In reply to Dimitris Papastamos
---
 drivers/base/regmap/regcache.c |    8 +++++++-
 include/trace/events/regmap.h  |   24 ++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletions(-)
drivers/base/regmap/regcache.c +7 -1
index 1e4849b..c438498 100644
@@ -219,12 +219,18 @@ EXPORT_SYMBOL_GPL(regcache_write);
  */
 int regcache_sync(struct regmap *map)
 {
+	int ret;
+	const char *name;
+
 	BUG_ON(!map->cache_ops);
 
 	if (map->cache_ops->sync) {
 		dev_dbg(map->dev, "Syncing %s cache\n",
 			map->cache_ops->name);
-		return map->cache_ops->sync(map);
+		name = map->cache_ops->name;
+		trace_regcache_sync(map->dev, name, "start");
+		ret = map->cache_ops->sync(map);
+		trace_regcache_sync(map->dev, name, "stop");
 	}
 	return 0;
 }
include/trace/events/regmap.h +24 -0
index e35e37c..1e3193b 100644
@@ -106,6 +106,30 @@ DEFINE_EVENT(regmap_block, regmap_hw_write_done,
 	TP_ARGS(dev, reg, count)
 );
 
+TRACE_EVENT(regcache_sync,
+
+	TP_PROTO(struct device *dev, const char *type,
+		 const char *status),
+
+	TP_ARGS(dev, type, status),
+
+	TP_STRUCT__entry(
+		__string(       name,           dev_name(dev)   )
+		__string(	status,		status		)
+		__string(	type,		type		)
+		__field(	int,		type		)
+	),
+
+	TP_fast_assign(
+		__assign_str(name, dev_name(dev));
+		__assign_str(status, status);
+		__assign_str(type, type);
+	),
+
+	TP_printk("%s type=%s status=%s", __get_str(name),
+		  __get_str(type), __get_str(status))
+);
+
 #endif /* _TRACE_REGMAP_H */
 
 /* This part must be outside protection */
Signature · 1 line
1.7.6.1
#6 lore raw

PATCH v5 6/6 regmap: Incorporate the regcache core into regmap

In reply to Dimitris Papastamos

This patch incorporates the regcache core code into regmap. All previous patches have been no-ops essentially up to this point.

The bulk read operation is not supported by regcache at the moment. This will be implemented incrementally.

---
 drivers/base/regmap/regmap.c |   28 ++++++++++++++++++++++++++++
 1 files changed, 28 insertions(+), 0 deletions(-)
drivers/base/regmap/regmap.c +28 -0
index e7adfe7..602083f 100644
@@ -146,6 +146,13 @@ struct regmap *regmap_init(struct device *dev,
 	map->readable_reg = config->readable_reg;
 	map->volatile_reg = config->volatile_reg;
 	map->precious_reg = config->precious_reg;
+	map->cache_type = config->cache_type;
+	map->reg_defaults = config->reg_defaults;
+	map->num_reg_defaults = config->num_reg_defaults;
+	map->num_reg_defaults_raw = config->num_reg_defaults_raw;
+	map->reg_defaults_raw = config->reg_defaults_raw;
+	map->cache_size_raw = (config->val_bits / 8) * config->num_reg_defaults_raw;
+	map->cache_word_size = config->val_bits / 8;
 
 	if (config->read_flag_mask || config->write_flag_mask) {
 		map->read_flag_mask = config->read_flag_mask;
@@ -208,6 +215,10 @@ struct regmap *regmap_init(struct device *dev,
 		goto err_map;
 	}
 
+	ret = regcache_init(map);
+	if (ret < 0)
+		goto err_map;
+
 	regmap_debugfs_init(map);
 
 	return map;
@@ -224,6 +235,7 @@ EXPORT_SYMBOL_GPL(regmap_init);
  */
 void regmap_exit(struct regmap *map)
 {
+	regcache_exit(map);
 	regmap_debugfs_exit(map);
 	kfree(map->work_buf);
 	kfree(map);
@@ -290,6 +302,12 @@ static int _regmap_write(struct regmap *map, unsigned int reg,
 	int ret;
 	BUG_ON(!map->format.format_write && !map->format.format_val);
 
+	if (!map->cache_bypass) {
+		ret = regcache_write(map, reg, val);
+		if (!ret || map->cache_only)
+			return 0;
+	}
+
 	trace_regmap_reg_write(map->dev, reg, val);
 
 	if (map->format.format_write) {
@@ -428,6 +446,14 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 
 	mutex_lock(&map->lock);
 
+	if (!map->cache_bypass) {
+		ret = regcache_read(map, reg, val);
+		if (!ret) {
+			mutex_unlock(&map->lock);
+			return 0;
+		}
+	}
+
 	ret = _regmap_read(map, reg, val);
 
 	mutex_unlock(&map->lock);
@@ -479,6 +505,8 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 	int ret, i;
 	size_t val_bytes = map->format.val_bytes;
 
+	WARN_ON(map->cache_type != REGCACHE_NONE);
+
 	if (!map->format.parse_val)
 		return -EINVAL;
 
Signature · 1 line
1.7.6.1
#7 lore raw

Re: [PATCH 6/6 v5] regmap: Incorporate the regcache core into regmap

Quoted 8 lines · Dimitris Papastamos

This patch incorporates the regcache core code into regmap. All previous patches have been no-ops essentially up to this point.

The bulk read operation is not supported by regcache at the moment. This will be implemented incrementally.

Hi

Two issues with this version.

drivers/base/regmap/regmap.c static int _regmap_write(struct regmap *map, unsigned int reg, · line 308

Quoted 9 lines

[...]

drivers/base/regmap/regmap.c +4 -0
@@ -290,6 +302,12 @@ static int _regmap_write(struct regmap *map, unsigned int reg,
 	int ret;
 	BUG_ON(!map->format.format_write && !map->format.format_val);
 
+	if (!map->cache_bypass) {
+		ret = regcache_write(map, reg, val);
+		if (!ret || map->cache_only)
+			return 0;

The hw write shouldn't be skipped if the cache write is successful. We should only exit here if cache_only is set.

I also wonder if we should pass the return value of regcache_write on to the caller if cache_only is set.

Btw. what should happen if both cache_bypass and cache_only are set? Or is that an invalid configuration?

Quoted 6 lines
drivers/base/regmap/regmap.c +2 -0
+	}
+
 	trace_regmap_reg_write(map->dev, reg, val);
 
 	if (map->format.format_write) {

drivers/base/regmap/regmap.c int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val) · line 455

Quoted 10 lines
drivers/base/regmap/regmap.c +7 -0
@@ -428,6 +446,14 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 
 	mutex_lock(&map->lock);
 
+	if (!map->cache_bypass) {
+		ret = regcache_read(map, reg, val);
+		if (!ret) {
+			mutex_unlock(&map->lock);
+			return 0;
+		}
+	}

This should go into _regmap_read. Otherwise regmap_update_bits will always use a hw read.

Also if cache_only is set I guess we shouldn't fallback to a hw read.

Quoted 4 lines
drivers/base/regmap/regmap.c +1 -0
+
 	ret = _regmap_read(map, reg, val);
 
 	mutex_unlock(&map->lock);
#8 lore raw

Re: [PATCH 6/6 v5] regmap: Incorporate the regcache core into regmap

Quoted 9 lines · Lars-Peter Clausen

Review

Quoted 4 lines · Dimitris Papastamos
(quoted diff) +4 -0
+	if (!map->cache_bypass) {
+		ret = regcache_write(map, reg, val);
+		if (!ret || map->cache_only)
+			return 0;

The hw write shouldn't be skipped if the cache write is successful. We should only exit here if cache_only is set.

There's a couple of other issues too. I've already got the following patch for this one locally:

drivers/base/regmap/regmap.c +0 -0
@@ -304,7 +304,9 @@ static int _regmap_write(struct regmap *map,

unsigned int re

        if (!map->cache_bypass) {
                ret = regcache_write(map, reg, val);

- if (!ret || map->cache_only)
+ if (ret != 0)
+ return ret;
+ if (map->cache_only)

                        return 0;
        }
Quoted 2 lines

I also wonder if we should pass the return value of regcache_write on to the caller if cache_only is set.

Yup.

Quoted 2 lines

Btw. what should happen if both cache_bypass and cache_only are set? Or is that an invalid configuration?

That's not sensible. Probably BUG_ON().

Quoted 4 lines
Quoted 3 lines
(quoted diff) +0 -0
@@ -428,6 +446,14 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 
 	mutex_lock(&map->lock);
Quoted 10 lines

Review int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val) · line 454

Quoted 7 lines
(quoted diff) +7 -0
+	if (!map->cache_bypass) {
+		ret = regcache_read(map, reg, val);
+		if (!ret) {
+			mutex_unlock(&map->lock);
+			return 0;
+		}
+	}

This should go into _regmap_read. Otherwise regmap_update_bits will always use a hw read.

Yup, got that too.

Quoted 1 line

Also if cache_only is set I guess we shouldn't fallback to a hw read.

Probably. Cache only is *mostly* there for write side stuff, but it'd be useful to add this. I've got a patch to do this.

#9 lore raw

Re: [PATCH 6/6 v5] regmap: Incorporate the regcache core into regmap

Quoted 5 lines · Mark Brown
Quoted 2 lines · Lars-Peter Clausen

Btw. what should happen if both cache_bypass and cache_only are set? Or is that an invalid configuration?

That's not sensible. Probably BUG_ON().

Yes this used to be a BUG_ON() on some of the previous versions of the patches.

Thanks,
Dimitris

#10 lore raw

Re: [PATCH 6/6 v5] regmap: Incorporate the regcache core into regmap

In reply to Mark Brown
Quoted 21 lines · Mark Brown
Quoted 9 lines · Lars-Peter Clausen

Review

Quoted 4 lines · Dimitris Papastamos
(quoted diff) +4 -0
+	if (!map->cache_bypass) {
+		ret = regcache_write(map, reg, val);
+		if (!ret || map->cache_only)
+			return 0;

The hw write shouldn't be skipped if the cache write is successful. We should only exit here if cache_only is set.

There's a couple of other issues too. I've already got the following patch for this one locally:

drivers/base/regmap/regmap.c +0 -0
@@ -304,7 +304,9 @@ static int _regmap_write(struct regmap *map,

unsigned int re

        if (!map->cache_bypass) {
                ret = regcache_write(map, reg, val);

drivers/base/regmap/regmap.c

Quoted 3 lines
drivers/base/regmap/regmap.c +2 -1
-               if (!ret || map->cache_only)
+               if (ret != 0)
+                       return ret;

Hm... last time we said we want to fallback to hw read/write even if the cache operation has failed. The issue is that regcache_write will check for regmap_writeable, so you'll get different behaviour if caching is enabled for registers where regmap_writeable returns false.

Quoted 7 lines

+ if (map->cache_only)

                        return 0;
        }
Quoted 2 lines

I also wonder if we should pass the return value of regcache_write on to the caller if cache_only is set.

Quoted 6 lines

Yup.

Quoted 2 lines

Btw. what should happen if both cache_bypass and cache_only are set? Or is that an invalid configuration?

That's not sensible. Probably BUG_ON().

BUG_ON might be a bit to strict, if we wanted to allow cache_only to be enabled through debugfs like it is done for ASoC right now.

#11 lore raw

Re: [PATCH 6/6 v5] regmap: Incorporate the regcache core into regmap

On Mon, Sep 19, 2011 at 06:28:31PM +0200, Lars-Peter Clausen wrote:

Please delete unneeded context from mails and configure your MUA to wrap early enough to allow for at least one level of quoting.

Quoted 9 lines · Mark Brown

Review

Quoted 7 lines · Lars-Peter Clausen
        if (!map->cache_bypass) {
                ret = regcache_write(map, reg, val);
(quoted diff) +2 -1
-               if (!ret || map->cache_only)
+               if (ret != 0)
+                       return ret;
Quoted 4 lines

Hm... last time we said we want to fallback to hw read/write even if the cache operation has failed. The issue is that regcache_write will check for regmap_writeable, so you'll get different behaviour if caching is enabled for registers where regmap_writeable returns false.

I'd expect regcache_write() to silently eat such updates, though I see it doesn't at the minute.

Quoted 7 lines
Quoted 4 lines
Quoted 2 lines

Btw. what should happen if both cache_bypass and cache_only are set? Or is that an invalid configuration?

That's not sensible. Probably BUG_ON().

BUG_ON might be a bit to strict, if we wanted to allow cache_only to be enabled through debugfs like it is done for ASoC right now.

If we're going to allow those to be writable (I'm not convinced that's sane) then it seems better that the write should enforce basic sanity.

#12 lore raw
In reply to Dimitris Papastamos
Quoted 3 lines · Dimitris Papastamos

This patch series introduces register caching support for regmap. I've mostly implemented all the suggestions, except the use of a dedicated init function for each of the compress types.

So, it's really excellent to have this functionality and as it's mostly there I've applied this. As a followup to this I'll post a short series which updates things so they work for my test system plus an additional driver conversion, I'll squash some of them down into the original commits for bisection and leave others separate. Thanks for doing this work, and thanks also to Lars-Peter for his review and testing.

#13 lore raw
Quoted 6 lines · Mark Brown

So, it's really excellent to have this functionality and as it's mostly there I've applied this. As a followup to this I'll post a short series which updates things so they work for my test system plus an additional driver conversion, I'll squash some of them down into the original commits for bisection and leave others separate. Thanks for doing this work, and thanks also to Lars-Peter for his review and testing.

Oh, and I do expect there's a few other things that need to be tidied up and improved but it's easier to do so incrementally than re-review the whole series repeatedly.

#14 lore raw

PATCH 1/5 regmap: A cache type of _NONE behaves like a bypassed cache

In reply to Mark Brown

Avoid extra special casing by setting the cache_bypass flag when we're not caching.

---
 drivers/base/regmap/regcache.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)
drivers/base/regmap/regcache.c +3 -1
index c438498..e0a4a0d 100644
@@ -82,8 +82,10 @@ int regcache_init(struct regmap *map)
 	int i;
 	void *tmp_buf;
 
-	if (map->cache_type == REGCACHE_NONE)
+	if (map->cache_type == REGCACHE_NONE) {
+		map->cache_bypass = true;
 		return 0;
+	}
 
 	for (i = 0; i < ARRAY_SIZE(cache_types); i++)
 		if (cache_types[i]->type == map->cache_type)
Signature · 1 line
1.7.6.3
#15 lore raw

PATCH 2/5 regmap: Prototype regcache_sync()

---
 include/linux/regmap.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
include/linux/regmap.h +1 -0
index cae69e6..63b3075 100644
@@ -140,4 +140,5 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 int regmap_update_bits(struct regmap *map, unsigned int reg,
 		       unsigned int mask, unsigned int val);
 
+int regcache_sync(struct regmap *map);
 #endif
Signature · 1 line
1.7.6.3
#16 lore raw

PATCH 3/5 regmap: Allow drivers to control cache_only flag

In reply to Mark Brown
---
 drivers/base/regmap/regcache.c |   18 ++++++++++++++++++
 include/linux/regmap.h         |    2 ++
 2 files changed, 20 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +18 -0
index e0a4a0d..7806941 100644
@@ -238,6 +238,24 @@ int regcache_sync(struct regmap *map)
 }
 EXPORT_SYMBOL_GPL(regcache_sync);
 
+/**
+ * regcache_cache_only: Put a register map into cache only mode
+ *
+ * @map: map to configure
+ * @cache_only: flag if changes should be written to the hardware
+ *
+ * When a register map is marked as cache only writes to the register
+ * map API will only update the register cache, they will not cause
+ * any hardware changes.  This is useful for allowing portions of
+ * drivers to act as though the device were functioning as normal when
+ * it is disabled for power saving reasons.
+ */
+void regcache_cache_only(struct regmap *map, bool enable)
+{
+	map->cache_only = enable;
+}
+EXPORT_SYMBOL_GPL(regcache_cache_only);
+
 bool regcache_set_val(void *base, unsigned int idx,
 		      unsigned int val, unsigned int word_size)
 {
include/linux/regmap.h +2 -0
index 63b3075..76ac255 100644
@@ -141,4 +141,6 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
 		       unsigned int mask, unsigned int val);
 
 int regcache_sync(struct regmap *map);
+void regcache_cache_only(struct regmap *map, bool enable);
+
 #endif
Signature · 1 line
1.7.6.3
#17 lore raw

PATCH 4/5 regmap: Indentation tweaks in regcache

In reply to Mark Brown
---
 drivers/base/regmap/regcache.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
drivers/base/regmap/regcache.c +2 -0
index 7806941..e2b172b 100644
@@ -178,6 +178,7 @@ int regcache_read(struct regmap *map,
 
 	if (!regmap_volatile(map, reg))
 		return map->cache_ops->read(map, reg, value);
+
 	return -EINVAL;
 }
 EXPORT_SYMBOL_GPL(regcache_read);
@@ -204,6 +205,7 @@ int regcache_write(struct regmap *map,
 
 	if (!regmap_volatile(map, reg))
 		return map->cache_ops->write(map, reg, value);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(regcache_write);
Signature · 1 line
1.7.6.3
#18 lore raw

PATCH 5/5 regmap: Fix hookup of regcache into regmap

In reply to Mark Brown

- Only suppress physical writes if we're in cache only mode
- Use the cache for all reads, not just internal ones
- Return -EBUSY if we're in cache only mode.

---
 drivers/base/regmap/regmap.c |   21 ++++++++++++---------
 1 files changed, 12 insertions(+), 9 deletions(-)
drivers/base/regmap/regmap.c +12 -9
index 602083f..3596465 100644
@@ -304,7 +304,9 @@ static int _regmap_write(struct regmap *map, unsigned int reg,
 
 	if (!map->cache_bypass) {
 		ret = regcache_write(map, reg, val);
-		if (!ret || map->cache_only)
+		if (ret != 0)
+			return ret;
+		if (map->cache_only)
 			return 0;
 	}
 
@@ -421,6 +423,15 @@ static int _regmap_read(struct regmap *map, unsigned int reg,
 	if (!map->format.parse_val)
 		return -EINVAL;
 
+	if (!map->cache_bypass) {
+		ret = regcache_read(map, reg, val);
+		if (ret == 0)
+			return 0;
+	}
+
+	if (map->cache_only)
+		return -EBUSY;
+
 	ret = _regmap_raw_read(map, reg, map->work_buf, map->format.val_bytes);
 	if (ret == 0) {
 		*val = map->format.parse_val(map->work_buf);
@@ -446,14 +457,6 @@ int regmap_read(struct regmap *map, unsigned int reg, unsigned int *val)
 
 	mutex_lock(&map->lock);
 
-	if (!map->cache_bypass) {
-		ret = regcache_read(map, reg, val);
-		if (!ret) {
-			mutex_unlock(&map->lock);
-			return 0;
-		}
-	}
-
 	ret = _regmap_read(map, reg, val);
 
 	mutex_unlock(&map->lock);
Signature · 1 line
1.7.6.3