Introduce caching support for regmap

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.

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/Makefile   |    2 +-
 drivers/base/regmap/internal.h |   50 ++++++++
 drivers/base/regmap/regcache.c |  251 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h         |   19 +++-
 4 files changed, 316 insertions(+), 6 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 +0 -0

index 5ab3fef..b7acbeb 100644
[...]

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

new file mode 100644
index 0000000..90b7e1f
@@ -0,0 +1,251 @@
+/*
+ * Register cache access API
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Dimitris Papastamos <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as

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

+
+int regcache_init(struct regmap *map)
+{
+	int i, j;
+	int count;
+	unsigned int val;
+
+	if (map->cache_type == REGCACHE_NONE)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(cache_types); ++i)
+		if (cache_types[i]->type == map->cache_type)
+			break;
+
+	if (i == ARRAY_SIZE(cache_types)) {
+		dev_err(map->dev, "Could not match compress type: %d\n",
+			map->cache_type);
+		return -EINVAL;
+	}

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

+
+	/* calculate the size of cache_defaults */
+	for (count = 0, i = 0; i < map->num_cache_defaults_raw; i++) {
+		val = regcache_get_val(map->cache_defaults_raw,
+				       i, map->cache_word_size);
+		if (!val)
+			continue;
+		count++;
+	}
+
+	map->cache_defaults = kmalloc(count * sizeof(struct reg_default),
+				      GFP_KERNEL);
+	if (!map->cache_defaults)
+		return -ENOMEM;
+
+	/* fill the cache_defaults */
+	map->num_cache_defaults = count;
+	for (i = 0, j = 0; i < map->num_cache_defaults_raw; i++) {
+		val = regcache_get_val(map->cache_defaults_raw,
+				       i, map->cache_word_size);
+		if (!val)
+			continue;
+		map->cache_defaults[j].reg = i;
+		map->cache_defaults[j].def = val;
+		j++;
+	}
+
+	if (map->cache_ops->init) {
+		dev_dbg(map->dev, "Initializing %s cache\n",
+			map->cache_ops->name);
+		return map->cache_ops->init(map);
+	}
+	return 0;
+}
+
+void regcache_exit(struct regmap *map)
+{
+	if (map->cache_type == REGCACHE_NONE)
+		return;
+
+	BUG_ON(!map->cache_ops);
+
+	kfree(map->cache_defaults);
+	if (map->cache_ops->exit) {
+		dev_dbg(map->dev, "Destroying %s cache\n",
+			map->cache_ops->name);
+		map->cache_ops->exit(map);
+	}
+}
+
+/**
+ * regcache_read: Fetch the value of a given register from the cache.
+ *
+ * @map: map to configure.
+ * @reg: The register index.
+ * @value: The value to be returned.
+ *
+ * Return a negative value on failure, 0 on success.

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

+				return map->cache_ops->read(map, reg, value);
+		}
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(regcache_read);
+
+/**
+ * regcache_write: Set the value of a given register in the cache.
+ *
+ * @map: map to configure.
+ * @reg: The register index.
+ * @value: The new register value.
+ *
+ * Return a negative value on failure, 0 on success.
+ */
+int regcache_write(struct regmap *map,
+		   unsigned int reg, unsigned int value)
+{
+	if (map->cache_type == REGCACHE_NONE)
+		return 0;
+
+	BUG_ON(!map->cache_ops);
+

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

+
+/**
+ * regcache_sync: Sync the register cache with the hardware.
+ *
+ * @map: map to configure.
+ *
+ * Any registers that should not be synced should be marked as
+ * volatile.  In general drivers can choose not to use the provided
+ * syncing functionality if they so require.
+ *
+ * Return a negative value on failure, 0 on success.
+ */
+int regcache_sync(struct regmap *map)
+{
+	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);
+	}
+	return 0;
+}
+EXPORT_SYMBOL_GPL(regcache_sync);
+
+bool regcache_set_val(void *base, unsigned int idx,
+		      unsigned int val, unsigned int word_size)
+{
+	switch (word_size) {
+	case 1: {
+		u8 *cache = base;
+		if (cache[idx] == val)
+			return true;
+		cache[idx] = val;
+		break;
+	}
+	case 2: {
+		u16 *cache = base;
+		if (cache[idx] == val)
+			return true;
+		cache[idx] = val;
+		break;
+	}
+	default:
+		BUG();
+	}
+	/* unreachable */
+	return false;
+}
+
+unsigned int regcache_get_val(const void *base, unsigned int idx,
+			      unsigned int word_size)
+{
+	if (!base)
+		return -EINVAL;
+
+	switch (word_size) {
+	case 1: {
+		const u8 *cache = base;
+		return cache[idx];
+	}
+	case 2: {
+		const u16 *cache = base;
+		return cache[idx];
+	}

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

+
+	for (i = 0; i < map->num_cache_defaults; ++i)
+		if (map->cache_defaults[i].reg == reg)
+			return i;
+	return -1;
+}
+
+int regcache_insert_reg(struct regmap *map, unsigned int reg,
+			unsigned int val)
+{
+	void *tmp;
+
+	tmp = krealloc(map->cache_defaults,
+		       (map->num_cache_defaults + 1) * sizeof(struct reg_default),
+		       GFP_KERNEL);
+	if (!tmp)
+		return -ENOMEM;
+	map->cache_defaults = tmp;
+	++map->num_cache_defaults;
+	map->cache_defaults[map->num_cache_defaults - 1].reg = reg;
+	map->cache_defaults[map->num_cache_defaults - 1].def = val;
+	return 0;
+}

include/linux/regmap.h +10 -3

index 449e264..b81e86a 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

@@ -50,9 +55,11 @@ struct reg_default {
  *                (eg, a clear on read interrupt status register).
  *
  * @max_register: Optional, specifies the maximum valid register index.
- * @reg_defaults: Power on reset values for registers (for use with
- *                register cache support).
- * @num_reg_defaults: Number of elements in reg_defaults.
+ *
+ * @cache_type: The actual cache type.
+ * @cache_defaults_raw: Power on reset values for registers (for use with
+ *                 register cache support).
+ * @num_cache_defaults_raw: Number of elements in cache_defaults_raw.
  */
 struct regmap_config {
 	int reg_bits;

include/linux/regmap.h +4 -0

+
+	enum regcache_type cache_type;
+	const void *cache_defaults_raw;
+	unsigned num_cache_defaults_raw;
 };
 
 typedef int (*regmap_hw_write)(struct device *dev, const void *data,

drivers/base/regmap/regcache.c line 19

Quoted 2 lines · Dimitris Papastamos

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

+static const struct regcache_ops *cache_types[] = {
+};

I wonder if it makes sense to keep a list of regcache_ops this way, or whether it is not better to just pass the ops we want to use to regcache_init directly.

drivers/base/regmap/regcache.c line 48

Quoted 4 lines

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

+	if (!map->cache_defaults_raw || !map->num_cache_defaults_raw) {
+		dev_err(map->dev, "Client has not provided a defaults cache\n");
+		return -EINVAL;
+	}

It should be OK to provide no default register values, in this case regmap should assume that the default for all registers is 0.

drivers/base/regmap/regcache.c line 119

Quoted 1 line

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

+			if (value && map->cache_ops->read)

Will value or cache_ops->read ever be NULL? A register cache that either only provides read or write would be pretty useless in my opinion, and we should probably check for this at initialization time and not upon each access.

Quoted 7 lines · Lars-Peter Clausen

drivers/base/regmap/regcache.c line 19

Quoted 2 lines · Dimitris Papastamos

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

+static const struct regcache_ops *cache_types[] = {
+};

I wonder if it makes sense to keep a list of regcache_ops this way, or whether it is not better to just pass the ops we want to use to regcache_init directly.

Or have a function per cache type. I'm keen to hide the ops from users because I don't want to have to worry about them peering inside the internals.

Quoted 7 lines

drivers/base/regmap/regcache.c line 48

Quoted 4 lines

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

+	if (!map->cache_defaults_raw || !map->num_cache_defaults_raw) {
+		dev_err(map->dev, "Client has not provided a defaults cache\n");
+		return -EINVAL;
+	}

It should be OK to provide no default register values, in this case regmap should assume that the default for all registers is 0.

Yes - Dimitris, as we discussed offline it's pretty much essential for things like PMICs where the defaults aren't meaningful and may even be unknowable.

Quoted 7 lines · Mark Brown

Quoted 2 lines · Lars-Peter Clausen

It should be OK to provide no default register values, in this case regmap should assume that the default for all registers is 0.

Yes - Dimitris, as we discussed offline it's pretty much essential for things like PMICs where the defaults aren't meaningful and may even be

That's implemented in patch 8/8. I can of course squash that patch into the other patch.

Quoted 7 lines · Lars-Peter Clausen

drivers/base/regmap/regcache.c line 19

Quoted 2 lines · Dimitris Papastamos

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

+static const struct regcache_ops *cache_types[] = {
+};

I wonder if it makes sense to keep a list of regcache_ops this way, or whether it is not better to just pass the ops we want to use to regcache_init directly.

Or have a function per cache type. I'm keen to hide the ops from users because I don't want to have to worry about them peering inside the internals.

Quoted 54 lines · Dimitris Papastamos

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.

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/Makefile   |    2 +-
 drivers/base/regmap/internal.h |   50 ++++++++
 drivers/base/regmap/regcache.c |  251 ++++++++++++++++++++++++++++++++++++++++
 include/linux/regmap.h         |   19 +++-
 4 files changed, 316 insertions(+), 6 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 +0 -0

index 5ab3fef..b7acbeb 100644
[...]

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

new file mode 100644
index 0000000..90b7e1f
@@ -0,0 +1,251 @@
+/*
+ * Register cache access API
+ *
+ * Copyright 2011 Wolfson Microelectronics plc
+ *
+ * Author: Dimitris Papastamos <[email protected]>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/slab.h>
+#include <trace/events/regmap.h>

drivers/base/regmap/regcache.c line 19

Quoted 5 lines

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

+
+#include "internal.h"
+
+static const struct regcache_ops *cache_types[] = {
+};

I wonder if it makes sense to keep a list of regcache_ops this way, or whether it is not better to just pass the ops we want to use to regcache_init directly.

Quoted 24 lines

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

+
+int regcache_init(struct regmap *map)
+{
+	int i, j;
+	int count;
+	unsigned int val;
+
+	if (map->cache_type == REGCACHE_NONE)
+		return 0;
+
+	for (i = 0; i < ARRAY_SIZE(cache_types); ++i)
+		if (cache_types[i]->type == map->cache_type)
+			break;
+
+	if (i == ARRAY_SIZE(cache_types)) {
+		dev_err(map->dev, "Could not match compress type: %d\n",
+			map->cache_type);
+		return -EINVAL;
+	}
+
+	map->cache = NULL;
+	map->cache_ops = cache_types[i];
+	if (!map->cache_ops->name)
+		return -EINVAL;

drivers/base/regmap/regcache.c line 48

Quoted 5 lines

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

+
+	if (!map->cache_defaults_raw || !map->num_cache_defaults_raw) {
+		dev_err(map->dev, "Client has not provided a defaults cache\n");
+		return -EINVAL;
+	}

It should be OK to provide no default register values, in this case regmap should assume that the default for all registers is 0.

Quoted 12 lines

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

+/**
+ * regcache_read: Fetch the value of a given register from the cache.
+ *
+ * @map: map to configure.
+ * @reg: The register index.
+ * @value: The value to be returned.
+ *
+ * Return a negative value on failure, 0 on success.
+ */
+int regcache_read(struct regmap *map,
+		  unsigned int reg, unsigned int *value)
+{

drivers/base/regmap/regcache.c line 116

Quoted 6 lines

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

+	if (map->cache_type == REGCACHE_NONE)
+		return 0;
+
+	BUG_ON(!map->cache_ops);
+
+	if (reg < map->num_cache_defaults_raw) {

maybe use regmap_readable(map, reg)), but at least check against max_register and not num_cache_defaults_raw

drivers/base/regmap/regcache.c line 68

Quoted 2 lines

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

+		if (!map->volatile_reg ||
+		    (map->volatile_reg && !map->volatile_reg(map->dev, reg))) {

if (!regmap_volatile(map, reg))

drivers/base/regmap/regcache.c line 119

Quoted 1 line

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

+			if (value && map->cache_ops->read)

Will value or cache_ops->read ever be NULL? A register cache that either only provides read or write would be pretty useless in my opinion, and we should probably check for this at initialization time and not upon each access.

drivers/base/regmap/regcache.c line 228

Quoted 3 lines

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

+int regcache_lookup_reg(struct regmap *map, unsigned int reg)
+{
+	int i;

unsigned int

Quoted 19 lines

drivers/base/regmap/regcache.c new file +5 -3

 /**
  * Default value for a register.  We use an array of structs rather
  * than a simple array as many modern devices have very sparse

@@ -50,9 +55,11 @@ struct reg_default {
  *                (eg, a clear on read interrupt status register).
  *
  * @max_register: Optional, specifies the maximum valid register index.
- * @reg_defaults: Power on reset values for registers (for use with
- *                register cache support).
- * @num_reg_defaults: Number of elements in reg_defaults.
+ *
+ * @cache_type: The actual cache type.
+ * @cache_defaults_raw: Power on reset values for registers (for use with
+ *                 register cache support).
+ * @num_cache_defaults_raw: Number of elements in cache_defaults_raw.
  */
 struct regmap_config {
 	int reg_bits;

drivers/base/regmap/regcache.c struct regmap_config { · line 68

Quoted 5 lines

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

@@ -64,8 +71,10 @@ struct regmap_config {
 	bool (*precious_reg)(struct device *dev, unsigned int reg);
 
 	unsigned int max_register;
-	struct reg_default *reg_defaults;
-	int num_reg_defaults;

I guess these were removed by accident due to some merge conflict as they were added just recently. It would be good if you could re-add them and if they are set initialize cache_defaults using a memdup instead of reading cache_defaults_raw.

drivers/base/regmap/regcache.c struct regmap_config { · line 80

Quoted 7 lines

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

+
+	enum regcache_type cache_type;
+	const void *cache_defaults_raw;
+	unsigned num_cache_defaults_raw;
 };
 
 typedef int (*regmap_hw_write)(struct device *dev, const void *data,

[...]

Quoted 35 lines

Quoted 19 lines

/**
* Default value for a register. We use an array of structs rather
* than a simple array as many modern devices have very sparse

include/linux/regmap.h +5 -3

@@ -50,9 +55,11 @@ struct reg_default {
  *                (eg, a clear on read interrupt status register).
  *
  * @max_register: Optional, specifies the maximum valid register index.
- * @reg_defaults: Power on reset values for registers (for use with
- *                register cache support).
- * @num_reg_defaults: Number of elements in reg_defaults.
+ *
+ * @cache_type: The actual cache type.
+ * @cache_defaults_raw: Power on reset values for registers (for use with
+ *                 register cache support).
+ * @num_cache_defaults_raw: Number of elements in cache_defaults_raw.
  */
 struct regmap_config {
 	int reg_bits;

include/linux/regmap.h struct regmap_config { · line 68

Quoted 5 lines

include/linux/regmap.h +0 -2

@@ -64,8 +71,10 @@ struct regmap_config {
 	bool (*precious_reg)(struct device *dev, unsigned int reg);
 
 	unsigned int max_register;
-	struct reg_default *reg_defaults;
-	int num_reg_defaults;

I guess these were removed by accident due to some merge conflict as they were added just recently. It would be good if you could re-add them and if they are set initialize cache_defaults using a memdup instead of reading cache_defaults_raw.

Quoted 6 lines

include/linux/regmap.h +4 -0

+
+	enum regcache_type cache_type;
+	const void *cache_defaults_raw;
+	unsigned num_cache_defaults_raw;
 };
 

Quoted 1 line

include/linux/regmap.h struct regmap_config { · line 80

Quoted 1 line

include/linux/regmap.h +0 -0

 typedef int (*regmap_hw_write)(struct device *dev, const void *data,

I was merely renaming these.

Quoted 36 lines · Dimitris Papastamos

[...]

Quoted 35 lines

Quoted 19 lines

/**
* Default value for a register. We use an array of structs rather
* than a simple array as many modern devices have very sparse

include/linux/regmap.h +5 -3

@@ -50,9 +55,11 @@ struct reg_default {
  *                (eg, a clear on read interrupt status register).
  *
  * @max_register: Optional, specifies the maximum valid register index.
- * @reg_defaults: Power on reset values for registers (for use with
- *                register cache support).
- * @num_reg_defaults: Number of elements in reg_defaults.
+ *
+ * @cache_type: The actual cache type.
+ * @cache_defaults_raw: Power on reset values for registers (for use with
+ *                 register cache support).
+ * @num_cache_defaults_raw: Number of elements in cache_defaults_raw.
  */
 struct regmap_config {
 	int reg_bits;

include/linux/regmap.h struct regmap_config { · line 68

Quoted 5 lines

include/linux/regmap.h +0 -2

@@ -64,8 +71,10 @@ struct regmap_config {
 	bool (*precious_reg)(struct device *dev, unsigned int reg);
 
 	unsigned int max_register;
-	struct reg_default *reg_defaults;
-	int num_reg_defaults;

I guess these were removed by accident due to some merge conflict as they were added just recently. It would be good if you could re-add them and if they are set initialize cache_defaults using a memdup instead of reading cache_defaults_raw.

Quoted 6 lines

include/linux/regmap.h +4 -0

+
+	enum regcache_type cache_type;
+	const void *cache_defaults_raw;
+	unsigned num_cache_defaults_raw;
 };
 

Quoted 3 lines

Quoted 1 line

include/linux/regmap.h struct regmap_config { · line 80

Quoted 1 line

include/linux/regmap.h +0 -0

 typedef int (*regmap_hw_write)(struct device *dev, const void *data,

I was merely renaming these.

And you changed the type. We need the reg_default type register defaults for devices which have a sparse registers set or where the base register address is at a larger offset.

- Lars

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.

Signed-off-by Dimitris Papastamos <[email protected]>

---
 drivers/base/regmap/Makefile           |    2 +-
 drivers/base/regmap/internal.h         |    1 +
 drivers/base/regmap/regcache-indexed.c |   65 ++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c         |    3 +
 include/linux/regmap.h                 |    1 +
 5 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-indexed.c

[...]

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

+	if (ret < 0)
+		*value = 0;
+	else
+		*value = map->cache_defaults[ret].def;
+	return 0;
+}
+
[...]

drivers/base/regmap/regcache.c +0 -0

index 90b7e1f..dfefe40 100644

drivers/base/regmap/regcache.c +2 -0

+	&regcache_indexed_ops,
+#endif
 };
 
 int regcache_init(struct regmap *map)

include/linux/regmap.h +1 -0

index b81e86a..4d1ad09 100644
@@ -23,6 +23,7 @@ struct spi_device;
 /* An enum of all the supported cache types */
 enum regcache_type {
 	REGCACHE_NONE,
+	REGCACHE_INDEXED,
 };
 
 /**

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

Signed-off-by Dimitris Papastamos <[email protected]>

I wonder if we really still need indexed caches, because ...

Quoted 9 lines

---
 drivers/base/regmap/Makefile           |    2 +-
 drivers/base/regmap/internal.h         |    1 +
 drivers/base/regmap/regcache-indexed.c |   65 ++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c         |    3 +
 include/linux/regmap.h                 |    1 +
 5 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-indexed.c

drivers/base/regmap/regcache-indexed.c line 22

Quoted 7 lines

[...]

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

+static int regcache_indexed_read(struct regmap *map, unsigned int reg,
+				 unsigned int *value)
+{
+	int ret;
+
+	ret = regcache_lookup_reg(map, reg);

... this lookup will be slower than an rbtree lookup. And given that we use two integers per register doesn't give it much size advantage either.

I have some patches for the rbtree register cache code, which reduce some of it's complexity and also implements the adjacent node merging and allows smaller holes in the rbnodes register block if the block is smaller then an rbnode. Currently these patches are based on the ASoC register cache code, but they should be easy to rework onto of the regmap register cache code.

I did some small statistics on the number of rbnodes without and with the patches applied:

Without patches:
Nodes:

  0: 4000-4000   1
  1: 4009-400a   2
  2: 400b-400c   2
  3: 400d-4010   4
  4: 4015-4015   1
  5: 4017-4017   1
  6: 4019-401b   3
  7: 401c-401d   2
  8: 401e-4020   3
  9: 4021-4022   2
 10: 4023-4026   4
 11: 4029-402a   2
 12: 4031-4031   1
 13: 4036-4036   1
 14: 40eb-40eb   1
 15: 40f2-40f3   2
 16: 40f8-40f9   2
 17: 40fa-40fa   1

Number of nodes: 18 (504 bytes)
Number of registers: 35 (35 bytes)
Total size: 539 bytes

With patches:
Nodes:
0: 4000-4036 55
1: 40eb-40fa 16

Number of nodes: 2 (56 bytes)
Number of registers: 71 (71 bytes)
Total size: 127 bytes

In comparison a ASoC flat cache for this driver would have used 250 bytes and and regmap indexed cache would use 280 bytes. (Not that it really matters at those sizes anyway)

Quoted 15 lines

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

+	if (ret < 0)
+		*value = 0;
+	else
+		*value = map->cache_defaults[ret].def;
+	return 0;
+}
+
[...]

drivers/base/regmap/regcache.c +0 -0

index 90b7e1f..dfefe40 100644
@@ -16,6 +16,9 @@
 #include "internal.h"
 

drivers/base/regmap/regcache.c line 19

Quoted 2 lines

drivers/base/regmap/regcache.c +1 -0

 static const struct regcache_ops *cache_types[] = {
+#ifdef CONFIG_REGCACHE_INDEXED

This symbol doesn't seem to be defined anywhere.

Quoted 22 lines · Lars-Peter Clausen

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

Signed-off-by Dimitris Papastamos <[email protected]>

I wonder if we really still need indexed caches, because ...

Quoted 9 lines

---
 drivers/base/regmap/Makefile           |    2 +-
 drivers/base/regmap/internal.h         |    1 +
 drivers/base/regmap/regcache-indexed.c |   65 ++++++++++++++++++++++++++++++++
 drivers/base/regmap/regcache.c         |    3 +
 include/linux/regmap.h                 |    1 +
 5 files changed, 71 insertions(+), 1 deletions(-)
 create mode 100644 drivers/base/regmap/regcache-indexed.c

Quoted 10 lines

drivers/base/regmap/regcache-indexed.c line 22

Quoted 7 lines

[...]

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

+static int regcache_indexed_read(struct regmap *map, unsigned int reg,
+				 unsigned int *value)
+{
+	int ret;
+
+	ret = regcache_lookup_reg(map, reg);

... this lookup will be slower than an rbtree lookup. And given that we use two integers per register doesn't give it much size advantage either.

The indexed cache grew out of the flat cache somehow. Do you think the flat cache would be more appropriate than the indexed cache? One thing with the flat cache is that we need to make a flat copy of the cache_defaults (either partial or full). With the indexed cache we can use the cache_defaults directly.

I've got a patch that changes the regcache_lookup_reg() to actually use binary search. The array is kept sorted after insertions and since insertions are less common than readbacks it should improve things a bit.

In my opinion we should work towards having rbtree as the only cache type, but with optional lzo compression on the rbtree node blocks.

I have some patches for the rbtree register cache code, which reduce some of it's complexity and also implements the adjacent node merging and allows smaller holes in the rbnodes register block if the block is smaller then an rbnode. Currently these patches are based on the ASoC register cache code, but they should be easy to rework onto of the regmap register cache code.

Quoted 5 lines · Lars-Peter Clausen

I have some patches for the rbtree register cache code, which reduce some of it's complexity and also implements the adjacent node merging and allows smaller holes in the rbnodes register block if the block is smaller then an rbnode. Currently these patches are based on the ASoC register cache code, but they should be easy to rework onto of the regmap register cache code.

Dimitris just reminded me about what you said here - do you still have those patches floating around? This would really useful and would make some other optimistations like not cacheing default values much easier.

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.

Signed-off-by Dimitris Papastamos <[email protected]>

[...]

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

+
+static int regcache_rbtree_init(struct regmap *map)
+{
+	struct regcache_rbtree_ctx *rbtree_ctx;
+	int i;
+	int ret;
+
+	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
+	if (!map->cache)
+		return -ENOMEM;
+
+	rbtree_ctx = map->cache;
+	rbtree_ctx->root = RB_ROOT;
+	rbtree_ctx->cached_rbnode = NULL;
+
+	if (!map->cache_defaults)
+		return 0;
+

drivers/base/regmap/regcache.c +2 -0

+	&regcache_rbtree_ops,
+#endif
 };

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

Signed-off-by Dimitris Papastamos <[email protected]>

[...]

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

+
+static int regcache_rbtree_init(struct regmap *map)
+{
+	struct regcache_rbtree_ctx *rbtree_ctx;
+	int i;
+	int ret;
+
+	map->cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
+	if (!map->cache)
+		return -ENOMEM;
+
+	rbtree_ctx = map->cache;
+	rbtree_ctx->root = RB_ROOT;
+	rbtree_ctx->cached_rbnode = NULL;
+
+	if (!map->cache_defaults)
+		return 0;
+
+	for (i = 0; i < map->num_cache_defaults_raw; ++i) {
+		ret = regcache_lookup_reg(map, i);
+		if (ret < 0)
+			continue;

drivers/base/regmap/regcache-rbtree.c line 169

Quoted 6 lines

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

+		ret = regcache_rbtree_write(map,
+					    map->cache_defaults[ret].reg,
+					    map->cache_defaults[ret].def);
+		if (ret)
+			goto err;
+	}

You can iterate over the caches_defaults elements directly.

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.

Dimitris Papastamos <dp <at> opensource.wolfsonmicro.com> writes:

Quoted 5 lines

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.

Do you have example of such case ?

Caching 25kB or 50kB looks crazy.