Messages in this thread | | | Date | Mon, 05 Sep 2011 18:14:38 +0200 | From | Lars-Peter Clausen <> | Subject | Re: [PATCH 1/6] regmap: Introduce caching support |
| |
On 09/05/2011 03:50 PM, Dimitris Papastamos wrote: > 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 <dp@opensource.wolfsonmicro.com>
I gave it a try with the rbtree caching on real hardware and noticed some smaller issues, but with those fixed it seems to work fine.
> --- > drivers/base/regmap/Makefile | 2 +- > drivers/base/regmap/internal.h | 52 +++++++ > drivers/base/regmap/regcache.c | 302 ++++++++++++++++++++++++++++++++++++++++ > include/linux/regmap.h | 24 +++- > 4 files changed, 374 insertions(+), 6 deletions(-) > create mode 100644 drivers/base/regmap/regcache.c > > diff --git a/drivers/base/regmap/Makefile b/drivers/base/regmap/Makefile > index 057c13f..2e103ea 100644 > --- a/drivers/base/regmap/Makefile > +++ b/drivers/base/regmap/Makefile > @@ -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 > diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h > index 5ab3fef..dbeccd2 100644 > --- a/drivers/base/regmap/internal.h > +++ b/drivers/base/regmap/internal.h > @@ -17,6 +17,7 @@ > #include <linux/fs.h> > > struct regmap; > +struct regcache_ops; > > struct regmap_format { > size_t buf_size; > @@ -46,6 +47,40 @@ struct regmap { > bool (*readable_reg)(struct device *dev, unsigned int reg); > bool (*volatile_reg)(struct device *dev, unsigned int reg); > bool (*precious_reg)(struct device *dev, unsigned int reg); > + > + /* regcache specific members */ > + const struct regcache_ops *cache_ops; > + enum regcache_type cache_type; > + > + /* number of bytes in cache_defaults_raw */ > + unsigned int cache_size_raw; > + /* number of bytes per word in cache_defaults_raw */ > + unsigned int cache_word_size; > + /* number of entries in cache_defaults */ > + unsigned int num_cache_defaults; > + /* number of entries in cache_defaults_raw */ > + unsigned int num_cache_defaults_raw; > + > + /* if set, only the cache is modified not the HW */ > + unsigned int cache_only:1; > + /* if set, only the HW is modified not the cache */ > + unsigned int cache_bypass:1; > + /* if set, remember to free cache_defaults_raw */ > + unsigned int cache_free:1; > + > + struct reg_default *cache_defaults; > + const void *cache_defaults_raw; > + void *cache; > +}; > + > +struct regcache_ops { > + const char *name; > + enum regcache_type type; > + int (*init)(struct regmap *map); > + int (*exit)(struct regmap *map); > + int (*read)(struct regmap *map, unsigned int reg, unsigned int *value); > + int (*write)(struct regmap *map, unsigned int reg, unsigned int value); > + int (*sync)(struct regmap *map); > }; > > bool regmap_writeable(struct regmap *map, unsigned int reg); > @@ -63,4 +98,21 @@ void regmap_debugfs_init(struct regmap *map) { } > void regmap_debugfs_exit(struct regmap *map) { } > #endif > > +/* regcache core declarations */ > +int regcache_init(struct regmap *map); > +void regcache_exit(struct regmap *map); > +int regcache_read(struct regmap *map, > + unsigned int reg, unsigned int *value); > +int regcache_write(struct regmap *map, > + unsigned int reg, unsigned int value); > +int regcache_sync(struct regmap *map); > + > +unsigned int regcache_get_val(const void *base, unsigned int idx, > + unsigned int word_size); > +bool regcache_set_val(void *base, unsigned int idx, > + unsigned int val, unsigned int word_size); > +int regcache_lookup_reg(struct regmap *map, unsigned int reg); > +int regcache_insert_reg(struct regmap *map, unsigned int reg, > + unsigned int val); > + > #endif > diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c > new file mode 100644 > index 0000000..f26df00 > --- /dev/null > +++ b/drivers/base/regmap/regcache.c > @@ -0,0 +1,302 @@ > +/* > + * Register cache access API > + * > + * Copyright 2011 Wolfson Microelectronics plc > + * > + * Author: Dimitris Papastamos <dp@opensource.wolfsonmicro.com> > + * > + * 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> > + > +#include "internal.h" > + > +static const struct regcache_ops *cache_types[] = { > +}; > + > +int regcache_init(struct regmap *map) > +{ > + int i, j; > + int count; > + int ret; > + unsigned int val; > + void *tmp_buf; > + > + 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->read || > + !map->cache_ops->write || > + !map->cache_ops->name) > + return -EINVAL; > + > + /* We still need to ensure that the cache_defaults > + * won't vanish from under us. We'll need to make > + * a copy of it. > + */ > + if (map->cache_defaults) { > + if (!map->num_cache_defaults) > + return -EINVAL; > + tmp_buf = kmemdup(map->cache_defaults, map->num_cache_defaults * > + sizeof(struct reg_default), GFP_KERNEL); > + if (!tmp_buf) > + return -ENOMEM; > + map->cache_defaults = tmp_buf; > + goto cache_init; > + }
Maybe put the cache initialization from raw cache into a separate function and don't use goto. Should keep things a bit more readable.
> + /* Some devices such as PMIC's don't have cache defaults, > + * we cope with this by reading back the HW registers and > + * crafting the cache defaults by hand. > + */ > + if (!map->cache_defaults_raw) { > + if (!map->num_cache_defaults_raw) > + return -EINVAL; > + dev_warn(map->dev, "No cache defaults, reading back from HW\n"); > + tmp_buf = kmalloc(map->cache_size_raw, GFP_KERNEL); > + if (!tmp_buf) > + return -EINVAL; > + ret = regmap_bulk_read(map, 0, tmp_buf, > + map->num_cache_defaults_raw); > + if (ret < 0) { > + kfree(tmp_buf); > + return ret; > + } > + map->cache_defaults_raw = tmp_buf; > + map->cache_free = 1; > + } > + > + /* 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++; > + } > + > +cache_init: > + 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; > +} > + > [...] > + > +/** > + * 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) > +{ > + unsigned int max_reg; > + > + if (map->cache_type == REGCACHE_NONE) > + return 0; > + > + BUG_ON(!map->cache_ops); > + > + if (!regmap_readable(map, reg)) > + return 0;
Shouldn't we return some kind of error here? Otherwise value we indicate success, but *value contain random garbage.
> + > + if (map->max_register) > + max_reg = map->max_register; > + else > + max_reg = map->num_cache_defaults_raw; > + > + > + if (reg < max_reg && !regmap_volatile(map, reg)) > + 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) > +{ > + unsigned int max_reg; > + > + if (map->cache_type == REGCACHE_NONE) > + return 0; > + > + BUG_ON(!map->cache_ops); > + > + if (!regmap_writeable(map, reg)) > + return 0;
Same here.
> + > + if (map->max_register) > + max_reg = map->max_register; > + else > + max_reg = map->num_cache_defaults_raw; > + > + if (reg < max_reg && !regmap_volatile(map, reg)) > + return map->cache_ops->write(map, reg, value); > + return 0; > +} > +EXPORT_SYMBOL_GPL(regcache_write); > + > [...] > diff --git a/include/linux/regmap.h b/include/linux/regmap.h > index 449e264..7591e01 100644 > --- a/include/linux/regmap.h > +++ b/include/linux/regmap.h > @@ -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,14 @@ 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: Power on reset values for registers (for use with > + * register cache support). > + * @num_cache_defaults: Number of elements in cache_defaults. > + * @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; > @@ -64,8 +74,12 @@ 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; > + > + enum regcache_type cache_type; > + struct reg_default *cache_defaults;
Should be made const.
> + unsigned int num_cache_defaults; > + const void *cache_defaults_raw; > + unsigned int num_cache_defaults_raw; > };
| |