[PATCH 3/4] regmap: Add support for register indirect addressing.

[PATCH 3/4] regmap: Add support for register indirect addressing.

[Krystian Garbaciak]

Devices, having indirectly accessed registers or register paging implemented,
can configure register mapping to map indirectly accessible registers on
virtual address range. During access to virtually mapped register, indirect
addressing is processed automatically, depending on configuration. Other
registers are accessed directly.

Range configuration should contain base, number of virtual registers,
information about addressing/paging field and translation function defined.
translate_reg function should take virtual register and return indirect
address/page number and data register address for proper read.

Nesting of indirectly accessible register ranges is supported and will be
automatically carried out. For example, addressing/paging register and data
register of one address range can be located in another virtual address range,
that yet requires paging.

Caching for virtual register range is also supported and can be defined in
regmap configuration. In order to make indirect access more efficient, register
used for indirect addressing/paging should not be declared as volatile, when
possible.

struct regmap_config is extended with the following:
	struct regmap_range_cfg	*ranges;
	unsigned int		n_ranges;

Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>
---
 drivers/base/regmap/internal.h |   17 ++++
 drivers/base/regmap/regmap.c   |  167 +++++++++++++++++++++++++++++++++++++++-
 include/linux/regmap.h         |   37 +++++++++
 3 files changed, 217 insertions(+), 4 deletions(-)

diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index fcafc5b..0220e4c 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -79,6 +79,8 @@ struct regmap {
 
 	struct reg_default *patch;
 	int patch_regs;
+
+	struct list_head range_list;
 };
 
 struct regcache_ops {
@@ -99,6 +101,21 @@ bool regmap_precious(struct regmap *map, unsigned int reg);
 int _regmap_write(struct regmap *map, unsigned int reg,
 		  unsigned int val);
 
+struct regmap_range {
+	struct list_head list;
+
+	unsigned int base_reg;
+	unsigned int max_reg;
+
+	void (*translate_reg)(struct device *dev, unsigned int virtual_reg,
+			      unsigned int *page, unsigned int *reg);
+	unsigned int page_sel_reg;
+	unsigned int page_sel_mask;
+	int page_sel_shift;
+
+	bool busy;
+};
+
 #ifdef CONFIG_DEBUG_FS
 extern void regmap_debugfs_initcall(void);
 extern void regmap_debugfs_init(struct regmap *map);
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 7c5291e..c32d1a6 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -175,6 +175,12 @@ struct regmap *regmap_init(struct device *dev,
 {
 	struct regmap *map;
 	int ret = -EINVAL;
+	unsigned int range_base;
+	unsigned int min_base;
+	const struct regmap_range_cfg *range_cfg;
+	struct regmap_range *range;
+	struct list_head *entry, *entry_tmp;
+	int n;
 
 	if (!bus || !config)
 		goto err;
@@ -283,10 +289,74 @@ struct regmap *regmap_init(struct device *dev,
 	    !(map->format.format_reg && map->format.format_val))
 		goto err_map;
 
+	/* For some formats, indirect addressing is not supported. */
+	if (map->format.format_write && config->n_ranges != 0)
+		goto err_map;
+
+	/* Partition all accessible registers on address ranges,
+	   either to be accessed directly or indirectly. Arrange range
+	   list by ascending addresses. */
+	INIT_LIST_HEAD(&map->range_list);
+	range_base = 0;
+	do {
+		range_cfg = NULL;
+		for (n = 0, min_base = UINT_MAX; n < config->n_ranges; n++)
+			if (range_base <= config->ranges[n].base_reg &&
+			    config->ranges[n].base_reg <= min_base)
+				range_cfg = &config->ranges[n];
+
+		if (!range_cfg || range_cfg->base_reg > range_base) {
+			/* Range of registers for direct access */
+			range = kzalloc(sizeof(*range), GFP_KERNEL);
+			if (range == NULL) {
+				ret = -ENOMEM;
+				goto err_range;
+			}
+			range->base_reg = range_base;
+			if (range_cfg)
+				range->max_reg = range_cfg->base_reg - 1;
+			else
+				range->max_reg = UINT_MAX;
+			list_add_tail(&range->list, &map->range_list);
+		}
+
+		if (range_cfg) {
+			unsigned int range_max_reg = range_cfg->base_reg +
+						     range_cfg->num_regs - 1;
+
+			/* Sanity check for range configuration */
+			if (range_cfg->num_regs == 0 ||
+			    range_max_reg < range_cfg->base_reg ||
+			    range_max_reg > map->max_register ||
+			    range_cfg->translate_reg == NULL ||
+			    (range_cfg->base_reg <= range_cfg->page_sel_reg &&
+			     range_cfg->page_sel_reg <= range_max_reg)) {
+				ret = -EINVAL;
+				goto err_range;
+			}
+
+			/* Range of registers for indirect access */
+			range = kzalloc(sizeof(*range), GFP_KERNEL);
+			if (range == NULL) {
+				ret = -ENOMEM;
+				goto err_range;
+			}
+			range->base_reg = range_cfg->base_reg;
+			range->max_reg = range_max_reg;
+			range->translate_reg = range_cfg->translate_reg;
+			range->page_sel_reg = range_cfg->page_sel_reg;
+			range->page_sel_mask = range_cfg->page_sel_mask;
+			range->page_sel_shift = range_cfg->page_sel_shift;
+			list_add_tail(&range->list, &map->range_list);
+
+			range_base = range->max_reg + 1;
+		}
+	} while (range_cfg != NULL);
+
 	map->work_buf = kzalloc(map->format.buf_size, GFP_KERNEL);
 	if (map->work_buf == NULL) {
 		ret = -ENOMEM;
-		goto err_map;
+		goto err_range;
 	}
 
 	regmap_debugfs_init(map);
@@ -299,6 +369,9 @@ struct regmap *regmap_init(struct device *dev,
 
 err_free_workbuf:
 	kfree(map->work_buf);
+err_range:
+	list_for_each_safe(entry, entry_tmp, &map->range_list)
+		kfree(entry_tmp);
 err_map:
 	kfree(map);
 err:
@@ -389,13 +462,98 @@ int regmap_reinit_cache(struct regmap *map, const struct regmap_config *config)
  */
 void regmap_exit(struct regmap *map)
 {
+	struct regmap_range *range, *range_tmp;
+
 	regcache_exit(map);
 	regmap_debugfs_exit(map);
 	kfree(map->work_buf);
+	list_for_each_entry_safe(range, range_tmp, &map->range_list, list)
+		kfree(range_tmp);
 	kfree(map);
 }
 EXPORT_SYMBOL_GPL(regmap_exit);
 
+static int _regmap_update_bits(struct regmap *map, unsigned int reg,
+			       unsigned int mask, unsigned int val,
+			       bool *change);
+
+static int
+_regmap_range_access(int (*regmap_bus_access)(struct regmap *map,
+					      unsigned int reg,
+					      void *val, unsigned int val_len),
+		     struct regmap *map, unsigned int reg,
+		     void *val, unsigned int val_num)
+{
+	struct regmap_range *range;
+	bool change;
+	unsigned int _page, _p;
+	unsigned int _reg, _r;
+	unsigned int _num;
+	u8 *_val = val;
+	int ret;
+
+	/* Search for range to write to. Should always find one. */
+	list_for_each_entry_reverse(range, &map->range_list, list) {
+		if (range->base_reg <= reg)
+			break;
+	}
+	BUG_ON(&range->list == &map->range_list);
+
+	/* Bulk write should not cross single range boundaries */
+	if (val_num != 0 &&
+	    reg + val_num - 1 > range->max_reg)
+		return -EINVAL;
+
+	if (range->translate_reg) {
+		/* One virtual range can be accessed through another range.
+		   Make sure, that there is no loops during indirect access. */
+		if (range->busy)
+			return -EBUSY;
+
+		range->busy = 1;
+
+		/* Split bulk write to pages */
+		range->translate_reg(map->dev, reg, &_page, &_reg);
+		while (val_num) {
+			for (_num = 1; _num < val_num; _num++) {
+				range->translate_reg(map->dev, reg + _num,
+						     &_p, &_r);
+				if (_p != _page ||
+				    _r != _reg + _num)
+					break;
+			}
+
+			/* Update page register (may use caching) */
+			ret = _regmap_update_bits(map, range->page_sel_reg,
+						range->page_sel_mask,
+						_page << range->page_sel_shift,
+						&change);
+			if (ret < 0)
+				return ret;
+
+			/* There is no point to pass cache for data
+			   registers, as they should be volatile anyway */
+			ret = _regmap_range_access(regmap_bus_access,
+						   map, _reg, _val, _num);
+			if (ret < 0)
+				return ret;
+
+			val_num -= _num;
+			_val += _num * map->format.val_bytes;
+			_page = _p;
+			_reg = _r;
+		}
+
+		range->busy = 0;
+
+		return 0;
+
+	} else {
+		return regmap_bus_access(map, reg, val,
+					 val_num * map->format.val_bytes);
+	}
+}
+
 static int _regmap_bus_write(struct regmap *map, unsigned int reg,
 			     void *val, size_t val_len)
 {
@@ -452,7 +610,6 @@ static int _regmap_bus_write(struct regmap *map, unsigned int reg,
 static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 			     const void *val, size_t val_len)
 {
-	void *_val = (void *)val;
 	int i;
 	int ret;
 
@@ -482,7 +639,8 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 		}
 	}
 
-	return _regmap_bus_write(map, reg, _val, val_len);
+	return _regmap_range_access(_regmap_bus_write, map, reg, (void *)val,
+				    val_len / map->format.val_bytes);
 }
 
 int _regmap_write(struct regmap *map, unsigned int reg,
@@ -659,7 +817,8 @@ static int _regmap_bus_read(struct regmap *map, unsigned int reg, void *val,
 static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 			    unsigned int val_len)
 {
-	return _regmap_bus_read(map, reg, val, val_len);
+	return _regmap_range_access(_regmap_bus_read, map, reg, val,
+				    val_len / map->format.val_bytes);
 }
 
 static int _regmap_read(struct regmap *map, unsigned int reg,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a90abb6..fdedf34 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -20,6 +20,7 @@ struct device;
 struct i2c_client;
 struct spi_device;
 struct regmap;
+struct regmap_range;
 
 /* An enum of all the supported cache types */
 enum regcache_type {
@@ -50,6 +51,9 @@ struct reg_default {
  * @pad_bits: Number of bits of padding between register and value.
  * @val_bits: Number of bits in a register value, mandatory.
  *
+ * @ranges: Array of virtual address range descriptors.
+ * @num_ranges: Descriptor array size.
+ *
  * @writeable_reg: Optional callback returning true if the register
  *                 can be written to.
  * @readable_reg: Optional callback returning true if the register
@@ -81,6 +85,9 @@ struct regmap_config {
 	int pad_bits;
 	int val_bits;
 
+	const struct regmap_range_cfg *ranges;
+	unsigned int n_ranges;
+
 	bool (*writeable_reg)(struct device *dev, unsigned int reg);
 	bool (*readable_reg)(struct device *dev, unsigned int reg);
 	bool (*volatile_reg)(struct device *dev, unsigned int reg);
@@ -97,6 +104,36 @@ struct regmap_config {
 	u8 write_flag_mask;
 };
 
+/**
+ * Configuration for indirect accessed register range.
+ * Indirect or paged registers, can be defined with one or more structures.
+ * Registers out of such defined ranges are accessed directly.
+ *
+ * @base_reg: Register address of first register in virtual address range.
+ * @num_regs: Number of registers asigned to this range.
+ *
+ * @translate_reg: Function should return indirect address/page number and
+ *                 register number (out of this range) matching virtual_reg.
+ *
+ * @page_sel_reg: Register with selector for indirect address/page update.
+ * @page_sel_shift: Bit mask for selector.
+ * @page_sel_mask: Bit shift for selector.
+ */
+struct regmap_range_cfg {
+	/* Registers of virtual address range */
+	unsigned int base_reg;
+	unsigned int num_regs;
+
+	/* Registers translation function handler */
+	void (*translate_reg)(struct device *dev, unsigned int virtual_reg,
+			      unsigned int *page, unsigned int *reg);
+
+	/* Description of page selector for indirect addressing */
+	unsigned int page_sel_reg;
+	unsigned int page_sel_mask;
+	int page_sel_shift;
+};
+
 typedef int (*regmap_hw_write)(struct device *dev, const void *data,
 			       size_t count);
 typedef int (*regmap_hw_gather_write)(struct device *dev,
-- 
1.7.0.4

Re: [PATCH 3/4] regmap: Add support for register indirect addressing.

[Mark Brown]

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

On Thu, May 31, 2012 at 04:47:20PM +0200, Krystian Garbaciak wrote:

Adding people who've got some chips with paging, please keep them in the
CCs on this unless they complain (though since I'm cutting context...  :/ )

> +       /* Partition all accessible registers on address ranges,
> +          either to be accessed directly or indirectly. Arrange range
> +          list by ascending addresses. */

Wouldn't something naturally sorted like a rbtree be a more direct way
of doing this?

> +		range_cfg = NULL;
> +		for (n = 0, min_base = UINT_MAX; n < config->n_ranges; n++)
> +			if (range_base <= config->ranges[n].base_reg &&
> +			    config->ranges[n].base_reg <= min_base)
> +				range_cfg = &config->ranges[n];
> +

I've stared at this for a little while and I'm really not sure what it's
supposed to do.  The whole thing with min_base is just a bit odd, we're
doing comparisons against it but we never update it so why aren't we
using a constant, and in fact the comparison is always going to be true
since we're comparing against UINT_MAX.

I suspect it's supposed to pick the range with the lowest base but I'm
not convinced it does that.

> +		if (!range_cfg || range_cfg->base_reg > range_base) {
> +			/* Range of registers for direct access */
> +			range = kzalloc(sizeof(*range), GFP_KERNEL);
> +			if (range == NULL) {
> +				ret = -ENOMEM;
> +				goto err_range;
> +			}
> +			range->base_reg = range_base;
> +			if (range_cfg)
> +				range->max_reg = range_cfg->base_reg - 1;
> +			else
> +				range->max_reg = UINT_MAX;
> +			list_add_tail(&range->list, &map->range_list);
> +		}

This is making my head hurt too, possibly because of the lack of clarity
in the above.

> +static int _regmap_update_bits(struct regmap *map, unsigned int reg,
> +			       unsigned int mask, unsigned int val,
> +			       bool *change);

Put this up at the top of the file.

> +static int
> +_regmap_range_access(int (*regmap_bus_access)(struct regmap *map,
> +					      unsigned int reg,
> +					      void *val, unsigned int val_len),

eew, typedef this!

> +       unsigned int _page, _p;
> +       unsigned int _reg, _r;
> +       unsigned int _num;

These _s aren't helping legibility here.

> +	/* Bulk write should not cross single range boundaries */
> +	if (val_num != 0 &&
> +	    reg + val_num - 1 > range->max_reg)
> +		return -EINVAL;

When would val_num ever be zero?

> +			/* Update page register (may use caching) */
> +			ret = _regmap_update_bits(map, range->page_sel_reg,
> +						range->page_sel_mask,
> +						_page << range->page_sel_shift,
> +						&change);
> +			if (ret < 0)
> +				return ret;

Why the comment about the cache - why would this go wrong?

> +			/* There is no point to pass cache for data
> +			   registers, as they should be volatile anyway */
> +			ret = _regmap_range_access(regmap_bus_access,
> +						   map, _reg, _val, _num);
> +			if (ret < 0)
> +				return ret;

That comment needs some clarification too...

> +/**
> + * Configuration for indirect accessed register range.
> + * Indirect or paged registers, can be defined with one or more structures.

No , here.

> + * @translate_reg: Function should return indirect address/page number and
> + *                 register number (out of this range) matching virtual_reg.

Why does the user need to specify this?  Shouldn't we just specify a
size for the underlying window and then have a default which does the
obvious translations?  I'd imagine an *overwhelming* proportion of users
will want to do that.  Allowing an override is fine but requiring code
seems wrong for something like this.

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

RE: [PATCH 3/4] regmap: Add support for register indirect addressing.

[Krystian Garbaciak]

> > +       /* Partition all accessible registers on address ranges,
> > +          either to be accessed directly or indirectly. Arrange range
> > +          list by ascending addresses. */
> 
> Wouldn't something naturally sorted like a rbtree be a more direct way of doing
> this?

I expect here to have one or two ranges registered. Do you think, rbtree will be more efficient?

> > +		range_cfg = NULL;
> > +		for (n = 0, min_base = UINT_MAX; n < config->n_ranges; n++)
> > +			if (range_base <= config->ranges[n].base_reg &&
> > +			    config->ranges[n].base_reg <= min_base)
> > +				range_cfg = &config->ranges[n];
> > +
> 
> I've stared at this for a little while and I'm really not sure what it's supposed to
> do.  The whole thing with min_base is just a bit odd, we're doing comparisons
> against it but we never update it so why aren't we using a constant, and in fact
> the comparison is always going to be true since we're comparing against
> UINT_MAX.
> 
> I suspect it's supposed to pick the range with the lowest base but I'm not
> convinced it does that.

I  am searching for a range configuration with the lowest address range, that was not added yet.
I use range_base as a pointer to mark the position of base address for the next range to be added.
 
> > +		if (!range_cfg || range_cfg->base_reg > range_base) {
> > +			/* Range of registers for direct access */
> > +			range = kzalloc(sizeof(*range), GFP_KERNEL);
> > +			if (range == NULL) {
> > +				ret = -ENOMEM;
> > +				goto err_range;
> > +			}
> > +			range->base_reg = range_base;
> > +			if (range_cfg)
> > +				range->max_reg = range_cfg->base_reg - 1;
> > +			else
> > +				range->max_reg = UINT_MAX;
> > +			list_add_tail(&range->list, &map->range_list);
> > +		}
> 
> This is making my head hurt too, possibly because of the lack of clarity in the
> above.

Any empty space before configured virtual range is filled with range used for direct access. Empty address space, after all defined ranges, is used for direct access too (If that makes sense?). To mark such range (translate_reg==NULL).

> > +	/* Bulk write should not cross single range boundaries */
> > +	if (val_num != 0 &&
> > +	    reg + val_num - 1 > range->max_reg)
> > +		return -EINVAL;
> 
> When would val_num ever be zero?

There is no checking against (val_num == 0)  before this point.
But after reviewing the code after this, it seems not to do any harm in such case.

> > +			/* Update page register (may use caching) */
> > +			ret = _regmap_update_bits(map, range-
> >page_sel_reg,
> > +						range->page_sel_mask,
> > +						_page << range-
> >page_sel_shift,
> > +						&change);
> > +			if (ret < 0)
> > +				return ret;
> 
> Why the comment about the cache - why would this go wrong?

Nothing. _regmap_update_bits() is used, so the cache can be hit here and speed up paging.

> > +			/* There is no point to pass cache for data
> > +			   registers, as they should be volatile anyway */
> > +			ret = _regmap_range_access(regmap_bus_access,
> > +						   map, _reg, _val, _num);
> > +			if (ret < 0)
> > +				return ret;
> 
> That comment needs some clarification too...

Here, _regmap_range_access() is called directly (not _regmap_raw_read), as I expect everybody to define data window for indirect access as volatile, readable and writeable.

> > + * @translate_reg: Function should return indirect address/page number and
> > + *                 register number (out of this range) matching virtual_reg.
> 
> Why does the user need to specify this?  Shouldn't we just specify a size for the
> underlying window and then have a default which does the obvious
> translations?  I'd imagine an *overwhelming* proportion of users will want to
> do that.  Allowing an override is fine but requiring code seems wrong for
> something like this.

If so, I can easily make translate_reg() something internal for regmap and leave the window size to be configurable.

Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, 
some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it
is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, 
copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.

Please consider the environment before printing this e-mail

Re: [PATCH 3/4] regmap: Add support for register indirect addressing.

[Mark Brown]

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

On Thu, May 31, 2012 at 06:37:00PM +0000, Krystian Garbaciak wrote:

Fix your mailer to word wrap between paragraphs, your mails are not easy
to read.

> > Wouldn't something naturally sorted like a rbtree be a more direct way of doing
> > this?

> I expect here to have one or two ranges registered. Do you think,
> rbtree will be more efficient?

It might make the code rather more obvious, right now it's not exactly
clear.

> > > +		range_cfg = NULL;
> > > +		for (n = 0, min_base = UINT_MAX; n < config->n_ranges; n++)
> > > +			if (range_base <= config->ranges[n].base_reg &&
> > > +			    config->ranges[n].base_reg <= min_base)
> > > +				range_cfg = &config->ranges[n];
> > > +

> > I've stared at this for a little while and I'm really not sure what it's supposed to
> > do.  The whole thing with min_base is just a bit odd, we're doing comparisons
> > against it but we never update it so why aren't we using a constant, and in fact
> > the comparison is always going to be true since we're comparing against
> > UINT_MAX.

> > I suspect it's supposed to pick the range with the lowest base but I'm not
> > convinced it does that.

> I  am searching for a range configuration with the lowest address
> range, that was not added yet.  I use range_base as a pointer to mark
> the position of base address for the next range to be added.

None of which really addresses what I'm saying at all - the code is very
obscure, especially whatever you're doing with min_base which works out
as an always true comparison with a constant as far as I can tell.

> > > +		if (!range_cfg || range_cfg->base_reg > range_base) {
> > > +			/* Range of registers for direct access */

> > This is making my head hurt too, possibly because of the lack of clarity in the
> > above.

> Any empty space before configured virtual range is filled with range
> used for direct access. Empty address space, after all defined ranges,
> is used for direct access too (If that makes sense?). To mark such
> range (translate_reg==NULL).

I got what it's supposed to do, it's just not at all obvious how it
accomplishes this.  Like I say the fact that the immediately preceeding
code upon which it relies is as clear as mud isn't helping here.

> > > +			/* Update page register (may use caching) */
> > > +			ret = _regmap_update_bits(map, range-
> > >page_sel_reg,
> > > +						range->page_sel_mask,
> > > +						_page << range-
> > >page_sel_shift,
> > > +						&change);
> > > +			if (ret < 0)
> > > +				return ret;

> > Why the comment about the cache - why would this go wrong?

> Nothing. _regmap_update_bits() is used, so the cache can be hit here
> and speed up paging.

So why is this so surprising that we need a comment?  The comment seems
like it's flagging something that might be broken but fortunately isn't.

> Legal Disclaimer: This e-mail communication (and any attachment/s) is confidential and contains proprietary information, 
> some or all of which may be legally privileged. It is intended solely for the use of the individual or entity to which it
> is addressed. Access to this email by anyone else is unauthorized. If you are not the intended recipient, any disclosure, 
> copying, distribution or any action taken or omitted to be taken in reliance on it, is prohibited and may be unlawful.
> 
> Please consider the environment before printing this e-mail

You might want to see about removing this...  clearly you can do so
since your patches don't have it?

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