[PATCH] regmap: Add support for register paging

[PATCH] regmap: Add support for register paging

[Krystian Garbaciak]

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

Devices, having register map split into pages, can initialize regmap to
switch between pages automatically. This simply makes the total number
of registers to be virtually expanded and multiplied by number of pages.

When driver defines non-zero number of bits for page selection, register
address passed for read or write should consist of total number of page
bits and register bits to form an address, where page bits are the most
significant ones.

In order to make page switching efficient, register used for page
selection should be declared as cacheable (when possible). Driver can
define pageable and non-pageable (page independent) registers, so that
page switching will be carried out for pageable registers only.

Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>
---

[-- Attachment #2: regmap_paging.patch --]
[-- Type: text/x-patch, Size: 11674 bytes --]

diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
index 0f6c7fb..590d72c 100644
--- a/drivers/base/regmap/Kconfig
+++ b/drivers/base/regmap/Kconfig
@@ -16,3 +16,6 @@ config REGMAP_SPI
 
 config REGMAP_IRQ
 	bool
+
+config REGMAP_PAGING
+	bool
diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
index fcafc5b..b7931e1 100644
--- a/drivers/base/regmap/internal.h
+++ b/drivers/base/regmap/internal.h
@@ -31,6 +31,18 @@ struct regmap_format {
 	unsigned int (*parse_val)(void *buf);
 };
 
+#ifdef CONFIG_REGMAP_PAGING
+struct regmap_paging {
+	unsigned int page_mask;
+	unsigned int reg_mask;
+	unsigned int reg_bits;
+	bool (*pageable_reg)(struct device *dev, unsigned int reg);
+	unsigned int sel_reg;
+	int sel_shift;
+	unsigned int sel_mask;
+};
+#endif
+
 struct regmap {
 	struct mutex lock;
 
@@ -79,6 +91,10 @@ struct regmap {
 
 	struct reg_default *patch;
 	int patch_regs;
+
+#ifdef CONFIG_REGMAP_PAGING
+	struct regmap_paging page;
+#endif
 };
 
 struct regcache_ops {
diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index 7a3f535..45794cc 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -80,6 +80,24 @@ static bool regmap_volatile_range(struct regmap *map, unsigned int reg,
 	return true;
 }
 
+#ifdef CONFIG_REGMAP_PAGING
+static bool regmap_pageable_range(struct regmap *map, unsigned int reg,
+				  unsigned int num)
+{
+	if (map->page.pageable_reg) {
+		unsigned int i;
+
+		for (i = 0; i < num; i++)
+			if (map->page.pageable_reg(map->dev, reg))
+				return true;
+
+		return false;
+	}
+
+	return true;
+}
+#endif
+
 static void regmap_format_2_6_write(struct regmap *map,
 				     unsigned int reg, unsigned int val)
 {
@@ -199,6 +217,17 @@ struct regmap *regmap_init(struct device *dev,
 	map->volatile_reg = config->volatile_reg;
 	map->precious_reg = config->precious_reg;
 	map->cache_type = config->cache_type;
+#ifdef CONFIG_REGMAP_PAGING
+	map->page.page_mask = ((1 << config->page_bits) - 1) << config->reg_bits;
+	map->page.reg_mask = (1 << config->reg_bits) - 1;
+	map->page.reg_bits = config->reg_bits;
+	map->page.pageable_reg = config->pageable_reg;
+	map->page.sel_reg = config->page_sel_reg;
+	map->page.sel_shift = config->page_sel_shift;
+	map->page.sel_mask = ((1 << config->page_bits) - 1) <<
+			     config->page_sel_shift;
+#endif
+
 
 	if (config->read_flag_mask || config->write_flag_mask) {
 		map->read_flag_mask = config->read_flag_mask;
@@ -396,41 +425,66 @@ void regmap_exit(struct regmap *map)
 }
 EXPORT_SYMBOL_GPL(regmap_exit);
 
-static int _regmap_raw_write(struct regmap *map, unsigned int reg,
-			     const void *val, size_t val_len)
-{
-	u8 *u8 = map->work_buf;
-	void *buf;
-	int ret = -ENOTSUPP;
-	size_t len;
-	int i;
+static int _regmap_update_bits(struct regmap *map, unsigned int reg,
+			       unsigned int mask, unsigned int val,
+			       bool *change);
 
-	/* Check for unwritable registers before we start */
-	if (map->writeable_reg)
-		for (i = 0; i < val_len / map->format.val_bytes; i++)
-			if (!map->writeable_reg(map->dev, reg + i))
-				return -EINVAL;
+#ifdef CONFIG_REGMAP_PAGING
+static int _regmap_paged_access(struct regmap *map,
+		int (*regmap_bus_access)(struct regmap *map, unsigned int reg,
+					 void *val, unsigned int val_len),
+		unsigned int reg, void *val, unsigned int val_len)
+{
+	unsigned int val_num = val_len / map->format.val_bytes;
+	u8 *_val = val;
+	unsigned int _reg = reg & map->page.reg_mask;
+	unsigned int _page = reg >> map->page.reg_bits;
+	unsigned int _num;
+	size_t _len;
+	bool change;
+	int ret;
 
-	if (!map->cache_bypass && map->format.parse_val) {
-		unsigned int ival;
-		int val_bytes = map->format.val_bytes;
-		for (i = 0; i < val_len / val_bytes; i++) {
-			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
-			ival = map->format.parse_val(map->work_buf);
-			ret = regcache_write(map, reg + i, ival);
-			if (ret) {
-				dev_err(map->dev,
-				   "Error in caching of register: %u ret: %d\n",
-					reg + i, ret);
+	/* Split write into separate blocks, one for each page */
+	while (val_num) {
+		if ((_reg + val_num  - 1) >> map->page.reg_bits != 0)
+			_num = (map->page.reg_mask - _reg + 1);
+		else
+			_num = val_num;
+
+		if (regmap_pageable_range(map, _reg, _num)) {
+			/* Update page selector, try to use cache.
+			   Page selector is in nonpageable register, so it is
+			   safe to reenter update function here. */
+			ret = _regmap_update_bits(map,
+						  map->page.sel_reg,
+						  map->page.sel_mask,
+						  _page << map->page.sel_shift,
+						  &change);
+			if (ret < 0)
 				return ret;
-			}
-		}
-		if (map->cache_only) {
-			map->cache_dirty = true;
-			return 0;
 		}
+
+		_len = _num * map->format.val_bytes;
+		ret = regmap_bus_access(map, _reg, _val, _len);
+		if (ret < 0)
+			return ret;
+
+		val_num -= _num;
+		_val += _len;
+		_reg = 0;
+		_page++;
 	}
 
+	return 0;
+}
+#endif /* CONFIG_REGMAP_PAGING */
+
+static int _regmap_bus_write(struct regmap *map, unsigned int reg,
+			     void *val, size_t val_len)
+{
+	u8 *u8 = map->work_buf;
+	int ret = -ENOTSUPP;
+
 	map->format.format_reg(map->work_buf, reg);
 
 	u8[0] |= map->write_flag_mask;
@@ -456,6 +510,9 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 
 	/* If that didn't work fall back on linearising by hand. */
 	if (ret == -ENOTSUPP) {
+		void *buf;
+		size_t len;
+
 		len = map->format.reg_bytes + map->format.pad_bytes + val_len;
 		buf = kzalloc(len, GFP_KERNEL);
 		if (!buf)
@@ -475,6 +532,52 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
 	return ret;
 }
 
+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;
+
+	/* Check for unwritable registers before we start */
+	if (map->writeable_reg)
+		for (i = 0; i < val_len / map->format.val_bytes; i++)
+			if (!map->writeable_reg(map->dev, reg + i))
+				return -EINVAL;
+
+	if (!map->cache_bypass && map->format.parse_val) {
+		unsigned int ival;
+		int val_bytes = map->format.val_bytes;
+		for (i = 0; i < val_len / map->format.val_bytes; i++) {
+			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
+			ival = map->format.parse_val(map->work_buf);
+			ret = regcache_write(map, reg + i, ival);
+			if (ret) {
+				dev_err(map->dev,
+				   "Error in caching of register: %u ret: %d\n",
+					reg + i, ret);
+				return ret;
+			}
+		}
+		if (map->cache_only) {
+			map->cache_dirty = true;
+			return 0;
+		}
+	}
+
+#ifdef CONFIG_REGMAP_PAGING
+	/* Maintain paging, if enabled and register is page dependent */
+	if (map->page.page_mask &&
+	    !(map->page.sel_reg == reg &&
+	      val_len == map->format.val_bytes)) {
+		return _regmap_paged_access(map, &_regmap_bus_write,
+					    reg, _val, val_len);
+	}
+#endif /* CONFIG_REGMAP_PAGING */
+
+	return _regmap_bus_write(map, reg, _val, val_len);
+}
+
 int _regmap_write(struct regmap *map, unsigned int reg,
 		  unsigned int val)
 {
@@ -494,6 +597,28 @@ int _regmap_write(struct regmap *map, unsigned int reg,
 	trace_regmap_reg_write(map->dev, reg, val);
 
 	if (map->format.format_write) {
+#ifdef CONFIG_REGMAP_PAGING
+		/* Maintain paging, if enabled and register is page dependent */
+		if (map->page.page_mask &&
+		    map->page.sel_reg != reg &&
+		    (!map->page.pageable_reg ||
+		     map->page.pageable_reg(map->dev, reg))) {
+			unsigned int page = reg >> map->page.reg_bits;
+			bool change;
+
+			/* Update page selector, when needed.
+			   Page selector is in nonpageable register, so it is
+			   safe to reenter for its update in here. */
+			ret = _regmap_update_bits(map,
+						  map->page.sel_reg,
+						  map->page.sel_mask,
+						  page << map->page.sel_shift,
+						  &change);
+			if (ret < 0)
+				return ret;
+		}
+#endif
+
 		map->format.format_write(map, reg, val);
 
 		trace_regmap_hw_write_start(map->dev, reg, 1);
@@ -620,7 +745,7 @@ out:
 }
 EXPORT_SYMBOL_GPL(regmap_bulk_write);
 
-static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
+static int _regmap_bus_read(struct regmap *map, unsigned int reg, void *val,
 			    unsigned int val_len)
 {
 	u8 *u8 = map->work_buf;
@@ -649,6 +774,21 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
 	return ret;
 }
 
+static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
+			    unsigned int val_len)
+{
+#ifdef CONFIG_REGMAP_PAGING
+	/* Maintain paging, if enabled and register is page dependent */
+	if (map->page.page_mask &&
+	    !(map->page.sel_reg == reg &&
+	      val_len == map->format.val_bytes))
+		return _regmap_paged_access(map, &_regmap_bus_read,
+					    reg, val, val_len);
+#endif /* CONFIG_REGMAP_PAGING */
+
+	return _regmap_bus_read(map, reg, val, val_len);
+}
+
 static int _regmap_read(struct regmap *map, unsigned int reg,
 			unsigned int *val)
 {
@@ -792,11 +932,9 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 	int ret;
 	unsigned int tmp, orig;
 
-	mutex_lock(&map->lock);
-
 	ret = _regmap_read(map, reg, &orig);
 	if (ret != 0)
-		goto out;
+		return ret;
 
 	tmp = orig & ~mask;
 	tmp |= val & mask;
@@ -808,9 +946,6 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
 		*change = false;
 	}
 
-out:
-	mutex_unlock(&map->lock);
-
 	return ret;
 }
 
@@ -828,7 +963,12 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
 		       unsigned int mask, unsigned int val)
 {
 	bool change;
-	return _regmap_update_bits(map, reg, mask, val, &change);
+	int ret;
+
+	mutex_lock(&map->lock);
+	ret = _regmap_update_bits(map, reg, mask, val, &change);
+	mutex_unlock(&map->lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(regmap_update_bits);
 
@@ -848,7 +988,12 @@ int regmap_update_bits_check(struct regmap *map, unsigned int reg,
 			     unsigned int mask, unsigned int val,
 			     bool *change)
 {
-	return _regmap_update_bits(map, reg, mask, val, change);
+	int ret;
+
+	mutex_lock(&map->lock);
+	ret = _regmap_update_bits(map, reg, mask, val, change);
+	mutex_unlock(&map->lock);
+	return ret;
 }
 EXPORT_SYMBOL_GPL(regmap_update_bits_check);
 
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a90abb6..e4f9db0 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -50,6 +50,14 @@ struct reg_default {
  * @pad_bits: Number of bits of padding between register and value.
  * @val_bits: Number of bits in a register value, mandatory.
  *
+ * @page_bits: Number of address top-most bits designated to be page number.
+ *             These bits will be written to page selector, for page switch.
+ * @page_sel_reg: Register to update selected page in (available on any page).
+ * @page_sel_shift: Bit shift for page selector inside the register.
+ * @pageable_reg: Optional callback returning true, if a register needs
+ *                page switching (ie. it is page dependent). Register
+ *                in page_sel_reg is always considered as page independent.
+ *
  * @writeable_reg: Optional callback returning true if the register
  *                 can be written to.
  * @readable_reg: Optional callback returning true if the register
@@ -81,6 +89,13 @@ struct regmap_config {
 	int pad_bits;
 	int val_bits;
 
+#ifdef CONFIG_REGMAP_PAGING
+	int page_bits;
+	unsigned int page_sel_reg;
+	int page_sel_shift;
+	bool (*pageable_reg)(struct device *dev, unsigned int reg);
+#endif
+
 	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);

Re: [PATCH] regmap: Add support for register paging

[Mark Brown]

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

On Fri, May 11, 2012 at 12:55:24PM +0200, Krystian Garbaciak wrote:

> Devices, having register map split into pages, can initialize regmap to
> switch between pages automatically. This simply makes the total number
> of registers to be virtually expanded and multiplied by number of pages.

Adding some people I believe work with devices that have paged registers
(I don't really myself).  I'm not deleting any text for their benefit.

> When driver defines non-zero number of bits for page selection, register
> address passed for read or write should consist of total number of page
> bits and register bits to form an address, where page bits are the most
> significant ones.

> In order to make page switching efficient, register used for page
> selection should be declared as cacheable (when possible). Driver can
> define pageable and non-pageable (page independent) registers, so that
> page switching will be carried out for pageable registers only.

In terms of framework I think this is broadly OK, though it'd probably
be clearer and easier to understand all round if it were unconditional;
right now the diff is pretty hard to read so I may have missed some
issues and I'm also reluctant to add any additional combinations of
build options that aren't actually needed.

The only thing that concerns me is that it flattens the register range
by muxing the paging into the register address.  This isn't how most of
the existing paged register users I've seen have handled things and it
does mean that we're running the risk of the number of pages interfering
with the register space.  On the other hand it hides the fact that we're
doing paging from the rest of the kernel which is very nice for anything
doing generic infrastructure on top of regmap.

One other fun thing which just occurred to me is that I've got a bunch
of chips which have several independantly paged regions.  I'm not sure
that's really worth handling here, though.

> Signed-off-by: Krystian Garbaciak <krystian.garbaciak@diasemi.com>
> ---
> diff --git a/drivers/base/regmap/Kconfig b/drivers/base/regmap/Kconfig
> index 0f6c7fb..590d72c 100644
> --- a/drivers/base/regmap/Kconfig
> +++ b/drivers/base/regmap/Kconfig
> @@ -16,3 +16,6 @@ config REGMAP_SPI
>  
>  config REGMAP_IRQ
>  	bool
> +
> +config REGMAP_PAGING
> +	bool

> diff --git a/drivers/base/regmap/internal.h b/drivers/base/regmap/internal.h
> index fcafc5b..b7931e1 100644
> --- a/drivers/base/regmap/internal.h
> +++ b/drivers/base/regmap/internal.h
> @@ -31,6 +31,18 @@ struct regmap_format {
>  	unsigned int (*parse_val)(void *buf);
>  };
>  
> +#ifdef CONFIG_REGMAP_PAGING
> +struct regmap_paging {
> +	unsigned int page_mask;
> +	unsigned int reg_mask;
> +	unsigned int reg_bits;
> +	bool (*pageable_reg)(struct device *dev, unsigned int reg);
> +	unsigned int sel_reg;
> +	int sel_shift;
> +	unsigned int sel_mask;
> +};
> +#endif

It'd be good to have a way of enumerating the pages for the debugfs
interface.  Should we have a max_page as well?

> +
>  struct regmap {
>  	struct mutex lock;
>  
> @@ -79,6 +91,10 @@ struct regmap {
>  
>  	struct reg_default *patch;
>  	int patch_regs;
> +
> +#ifdef CONFIG_REGMAP_PAGING
> +	struct regmap_paging page;
> +#endif
>  };
>  
>  struct regcache_ops {
> diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
> index 7a3f535..45794cc 100644
> --- a/drivers/base/regmap/regmap.c
> +++ b/drivers/base/regmap/regmap.c
> @@ -80,6 +80,24 @@ static bool regmap_volatile_range(struct regmap *map, unsigned int reg,
>  	return true;
>  }
>  
> +#ifdef CONFIG_REGMAP_PAGING
> +static bool regmap_pageable_range(struct regmap *map, unsigned int reg,
> +				  unsigned int num)
> +{
> +	if (map->page.pageable_reg) {
> +		unsigned int i;
> +
> +		for (i = 0; i < num; i++)
> +			if (map->page.pageable_reg(map->dev, reg))
> +				return true;
> +
> +		return false;

Hrm.  I see where you're going with this test ("is any register in the
range pageable") but you could also read the function name as "is this
range of registers pageable".  Can we rename the function to
"has_pageable" instead to avoid confusion.

> +	}
> +
> +	return true;
> +}
> +#endif
> +
>  static void regmap_format_2_6_write(struct regmap *map,
>  				     unsigned int reg, unsigned int val)
>  {
> @@ -199,6 +217,17 @@ struct regmap *regmap_init(struct device *dev,
>  	map->volatile_reg = config->volatile_reg;
>  	map->precious_reg = config->precious_reg;
>  	map->cache_type = config->cache_type;
> +#ifdef CONFIG_REGMAP_PAGING
> +	map->page.page_mask = ((1 << config->page_bits) - 1) << config->reg_bits;
> +	map->page.reg_mask = (1 << config->reg_bits) - 1;
> +	map->page.reg_bits = config->reg_bits;
> +	map->page.pageable_reg = config->pageable_reg;
> +	map->page.sel_reg = config->page_sel_reg;
> +	map->page.sel_shift = config->page_sel_shift;
> +	map->page.sel_mask = ((1 << config->page_bits) - 1) <<
> +			     config->page_sel_shift;
> +#endif
> +
>  
>  	if (config->read_flag_mask || config->write_flag_mask) {
>  		map->read_flag_mask = config->read_flag_mask;
> @@ -396,41 +425,66 @@ void regmap_exit(struct regmap *map)
>  }
>  EXPORT_SYMBOL_GPL(regmap_exit);
>  
> -static int _regmap_raw_write(struct regmap *map, unsigned int reg,
> -			     const void *val, size_t val_len)
> -{
> -	u8 *u8 = map->work_buf;
> -	void *buf;
> -	int ret = -ENOTSUPP;
> -	size_t len;
> -	int i;
> +static int _regmap_update_bits(struct regmap *map, unsigned int reg,
> +			       unsigned int mask, unsigned int val,
> +			       bool *change);
>  
> -	/* Check for unwritable registers before we start */
> -	if (map->writeable_reg)
> -		for (i = 0; i < val_len / map->format.val_bytes; i++)
> -			if (!map->writeable_reg(map->dev, reg + i))
> -				return -EINVAL;

Hrm.  diff has done something really odd here which makes things hard to
read...

> +#ifdef CONFIG_REGMAP_PAGING
> +static int _regmap_paged_access(struct regmap *map,
> +		int (*regmap_bus_access)(struct regmap *map, unsigned int reg,
> +					 void *val, unsigned int val_len),
> +		unsigned int reg, void *val, unsigned int val_len)
> +{
> +	unsigned int val_num = val_len / map->format.val_bytes;
> +	u8 *_val = val;
> +	unsigned int _reg = reg & map->page.reg_mask;
> +	unsigned int _page = reg >> map->page.reg_bits;
> +	unsigned int _num;
> +	size_t _len;
> +	bool change;
> +	int ret;
>  
> -	if (!map->cache_bypass && map->format.parse_val) {
> -		unsigned int ival;
> -		int val_bytes = map->format.val_bytes;
> -		for (i = 0; i < val_len / val_bytes; i++) {
> -			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
> -			ival = map->format.parse_val(map->work_buf);
> -			ret = regcache_write(map, reg + i, ival);
> -			if (ret) {
> -				dev_err(map->dev,
> -				   "Error in caching of register: %u ret: %d\n",
> -					reg + i, ret);
> +	/* Split write into separate blocks, one for each page */
> +	while (val_num) {
> +		if ((_reg + val_num  - 1) >> map->page.reg_bits != 0)
> +			_num = (map->page.reg_mask - _reg + 1);
> +		else
> +			_num = val_num;
> +
> +		if (regmap_pageable_range(map, _reg, _num)) {
> +			/* Update page selector, try to use cache.
> +			   Page selector is in nonpageable register, so it is
> +			   safe to reenter update function here. */
> +			ret = _regmap_update_bits(map,
> +						  map->page.sel_reg,
> +						  map->page.sel_mask,
> +						  _page << map->page.sel_shift,
> +						  &change);
> +			if (ret < 0)
>  				return ret;
> -			}
> -		}
> -		if (map->cache_only) {
> -			map->cache_dirty = true;
> -			return 0;
>  		}
> +
> +		_len = _num * map->format.val_bytes;
> +		ret = regmap_bus_access(map, _reg, _val, _len);
> +		if (ret < 0)
> +			return ret;
> +
> +		val_num -= _num;
> +		_val += _len;
> +		_reg = 0;
> +		_page++;
>  	}
>  
> +	return 0;
> +}
> +#endif /* CONFIG_REGMAP_PAGING */
> +
> +static int _regmap_bus_write(struct regmap *map, unsigned int reg,
> +			     void *val, size_t val_len)
> +{
> +	u8 *u8 = map->work_buf;
> +	int ret = -ENOTSUPP;
> +
>  	map->format.format_reg(map->work_buf, reg);
>  
>  	u8[0] |= map->write_flag_mask;
> @@ -456,6 +510,9 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
>  
>  	/* If that didn't work fall back on linearising by hand. */
>  	if (ret == -ENOTSUPP) {
> +		void *buf;
> +		size_t len;
> +
>  		len = map->format.reg_bytes + map->format.pad_bytes + val_len;
>  		buf = kzalloc(len, GFP_KERNEL);
>  		if (!buf)
> @@ -475,6 +532,52 @@ static int _regmap_raw_write(struct regmap *map, unsigned int reg,
>  	return ret;
>  }
>  
> +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;
> +
> +	/* Check for unwritable registers before we start */
> +	if (map->writeable_reg)
> +		for (i = 0; i < val_len / map->format.val_bytes; i++)
> +			if (!map->writeable_reg(map->dev, reg + i))
> +				return -EINVAL;
> +
> +	if (!map->cache_bypass && map->format.parse_val) {
> +		unsigned int ival;
> +		int val_bytes = map->format.val_bytes;
> +		for (i = 0; i < val_len / map->format.val_bytes; i++) {
> +			memcpy(map->work_buf, val + (i * val_bytes), val_bytes);
> +			ival = map->format.parse_val(map->work_buf);
> +			ret = regcache_write(map, reg + i, ival);
> +			if (ret) {
> +				dev_err(map->dev,
> +				   "Error in caching of register: %u ret: %d\n",
> +					reg + i, ret);
> +				return ret;
> +			}
> +		}
> +		if (map->cache_only) {
> +			map->cache_dirty = true;
> +			return 0;
> +		}
> +	}
> +
> +#ifdef CONFIG_REGMAP_PAGING
> +	/* Maintain paging, if enabled and register is page dependent */
> +	if (map->page.page_mask &&
> +	    !(map->page.sel_reg == reg &&
> +	      val_len == map->format.val_bytes)) {
> +		return _regmap_paged_access(map, &_regmap_bus_write,
> +					    reg, _val, val_len);
> +	}
> +#endif /* CONFIG_REGMAP_PAGING */
> +
> +	return _regmap_bus_write(map, reg, _val, val_len);
> +}
> +
>  int _regmap_write(struct regmap *map, unsigned int reg,
>  		  unsigned int val)
>  {
> @@ -494,6 +597,28 @@ int _regmap_write(struct regmap *map, unsigned int reg,
>  	trace_regmap_reg_write(map->dev, reg, val);
>  
>  	if (map->format.format_write) {
> +#ifdef CONFIG_REGMAP_PAGING
> +		/* Maintain paging, if enabled and register is page dependent */
> +		if (map->page.page_mask &&
> +		    map->page.sel_reg != reg &&
> +		    (!map->page.pageable_reg ||
> +		     map->page.pageable_reg(map->dev, reg))) {
> +			unsigned int page = reg >> map->page.reg_bits;
> +			bool change;
> +
> +			/* Update page selector, when needed.
> +			   Page selector is in nonpageable register, so it is
> +			   safe to reenter for its update in here. */
> +			ret = _regmap_update_bits(map,
> +						  map->page.sel_reg,
> +						  map->page.sel_mask,
> +						  page << map->page.sel_shift,
> +						  &change);
> +			if (ret < 0)
> +				return ret;
> +		}
> +#endif
> +
>  		map->format.format_write(map, reg, val);
>  
>  		trace_regmap_hw_write_start(map->dev, reg, 1);
> @@ -620,7 +745,7 @@ out:
>  }
>  EXPORT_SYMBOL_GPL(regmap_bulk_write);
>  
> -static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
> +static int _regmap_bus_read(struct regmap *map, unsigned int reg, void *val,
>  			    unsigned int val_len)
>  {
>  	u8 *u8 = map->work_buf;
> @@ -649,6 +774,21 @@ static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
>  	return ret;
>  }
>  
> +static int _regmap_raw_read(struct regmap *map, unsigned int reg, void *val,
> +			    unsigned int val_len)
> +{
> +#ifdef CONFIG_REGMAP_PAGING
> +	/* Maintain paging, if enabled and register is page dependent */
> +	if (map->page.page_mask &&
> +	    !(map->page.sel_reg == reg &&
> +	      val_len == map->format.val_bytes))
> +		return _regmap_paged_access(map, &_regmap_bus_read,
> +					    reg, val, val_len);
> +#endif /* CONFIG_REGMAP_PAGING */
> +
> +	return _regmap_bus_read(map, reg, val, val_len);
> +}
> +
>  static int _regmap_read(struct regmap *map, unsigned int reg,
>  			unsigned int *val)
>  {
> @@ -792,11 +932,9 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
>  	int ret;
>  	unsigned int tmp, orig;
>  
> -	mutex_lock(&map->lock);
> -
>  	ret = _regmap_read(map, reg, &orig);
>  	if (ret != 0)
> -		goto out;
> +		return ret;
>  
>  	tmp = orig & ~mask;
>  	tmp |= val & mask;
> @@ -808,9 +946,6 @@ static int _regmap_update_bits(struct regmap *map, unsigned int reg,
>  		*change = false;
>  	}
>  
> -out:
> -	mutex_unlock(&map->lock);
> -
>  	return ret;
>  }
>  
> @@ -828,7 +963,12 @@ int regmap_update_bits(struct regmap *map, unsigned int reg,
>  		       unsigned int mask, unsigned int val)
>  {
>  	bool change;
> -	return _regmap_update_bits(map, reg, mask, val, &change);
> +	int ret;
> +
> +	mutex_lock(&map->lock);
> +	ret = _regmap_update_bits(map, reg, mask, val, &change);
> +	mutex_unlock(&map->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(regmap_update_bits);
>  
> @@ -848,7 +988,12 @@ int regmap_update_bits_check(struct regmap *map, unsigned int reg,
>  			     unsigned int mask, unsigned int val,
>  			     bool *change)
>  {
> -	return _regmap_update_bits(map, reg, mask, val, change);
> +	int ret;
> +
> +	mutex_lock(&map->lock);
> +	ret = _regmap_update_bits(map, reg, mask, val, change);
> +	mutex_unlock(&map->lock);
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(regmap_update_bits_check);
>  
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index a90abb6..e4f9db0 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -50,6 +50,14 @@ struct reg_default {
>   * @pad_bits: Number of bits of padding between register and value.
>   * @val_bits: Number of bits in a register value, mandatory.
>   *
> + * @page_bits: Number of address top-most bits designated to be page number.
> + *             These bits will be written to page selector, for page switch.
> + * @page_sel_reg: Register to update selected page in (available on any page).
> + * @page_sel_shift: Bit shift for page selector inside the register.
> + * @pageable_reg: Optional callback returning true, if a register needs
> + *                page switching (ie. it is page dependent). Register
> + *                in page_sel_reg is always considered as page independent.
> + *
>   * @writeable_reg: Optional callback returning true if the register
>   *                 can be written to.
>   * @readable_reg: Optional callback returning true if the register
> @@ -81,6 +89,13 @@ struct regmap_config {
>  	int pad_bits;
>  	int val_bits;
>  
> +#ifdef CONFIG_REGMAP_PAGING
> +	int page_bits;
> +	unsigned int page_sel_reg;
> +	int page_sel_shift;
> +	bool (*pageable_reg)(struct device *dev, unsigned int reg);
> +#endif
> +
>  	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);


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

Re: [PATCH] regmap: Add support for register paging

[dd diasemi]

On Fri, May 11, 2012 at 1:27 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, May 11, 2012 at 12:55:24PM +0200, Krystian Garbaciak wrote:
>
> In terms of framework I think this is broadly OK, though it'd probably
> be clearer and easier to understand all round if it were unconditional;
> right now the diff is pretty hard to read so I may have missed some
> issues and I'm also reluctant to add any additional combinations of
> build options that aren't actually needed.
I did some rearrangement in the code, like splitting functions into
halves, so it is hard to read it from the patch. However, I have not
changed present functionality and if paging is not initialised, regmap
behaves the same way as before.
I will prepare patch without compilation flag, as it is not so important.

> One other fun thing which just occurred to me is that I've got a bunch
> of chips which have several independantly paged regions.  I'm not sure
> that's really worth handling here, though.
I have more advanced solution, which actually makes register mapping
split into virtual address ranges. This may solve many problems,
including giving capability to support several indirect register
ranges at once.
Let me know, what you think about this API?

Kind Regards,
Krystian
---
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index a90abb6..13f31f4 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;

+	struct regmap_range *ranges;
+	unsigned int num_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,35 @@ struct regmap_config {
 	u8 write_flag_mask;
 };

+/**
+ * Descriptors for virtual address range.
+ * Indirect or paged registers, can be defined with one or more descriptors.
+ * Registers not defined in such descriptor 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 that passes page number and physical
register number
+ *                 matching virtual_reg.
+ *
+ * @page_sel_reg: Register to update selected page in (available on any page).
+ * @page_sel_shift: Bit mask for page selector access inside the register.
+ * @page_sel_mask: Bit shift for page selector access inside the register.
+ */
+struct regmap_range {
+	/* Registers in virtual address range */
+	unsigned int base_reg;
+	unsigned int num_regs;
+
+	/* Registers translation handler */
+	int (*translate_reg)(struct device *dev, unsigned int virtual_reg,
+			     unsigned int *page, unsigned int *reg);
+
+	/* Description of page selector */
+	unsigned int page_sel_reg;
+	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,

Re: [PATCH] regmap: Add support for register paging

[Mark Brown]

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

On Fri, May 11, 2012 at 03:55:54PM +0200, dd diasemi wrote:
> On Fri, May 11, 2012 at 1:27 PM, Mark Brown

> > right now the diff is pretty hard to read so I may have missed some
> > issues and I'm also reluctant to add any additional combinations of
> > build options that aren't actually needed.

> I did some rearrangement in the code, like splitting functions into
> halves, so it is hard to read it from the patch. However, I have not
> changed present functionality and if paging is not initialised, regmap
> behaves the same way as before.

Ah, if you're going to do this sort of thing the best way to do it is to
rearrange the code first as one patch then add a second patch adding the
new functionality.  That way the code motion isn't mixed up with the new
code which makes review easier.

> I will prepare patch without compilation flag, as it is not so important.

Thanks.

> > One other fun thing which just occurred to me is that I've got a bunch
> > of chips which have several independantly paged regions.  I'm not sure
> > that's really worth handling here, though.

> I have more advanced solution, which actually makes register mapping
> split into virtual address ranges. This may solve many problems,
> including giving capability to support several indirect register
> ranges at once.
> Let me know, what you think about this API?

I think that sounds roughly like what I was thinking of so it's probably
sensible but obviously it'll depend on the implementation.

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

Re: [PATCH] regmap: Add support for register paging

[dd diasemi]

On Fri, May 11, 2012 at 3:10 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Fri, May 11, 2012 at 03:55:54PM +0200, dd diasemi wrote:
>> I have more advanced solution, which actually makes register mapping
>> split into virtual address ranges. This may solve many problems,
>> including giving capability to support several indirect register
>> ranges at once.
>> Let me know, what you think about this API?
>
> I think that sounds roughly like what I was thinking of so it's probably
> sensible but obviously it'll depend on the implementation.
I will use current work and add traversing of regmap_range table to
get proper range definition, then use it to do the indirect access,
like paging. Most of the drivers probably will just have one
regmap_range defined for paging or indirect registers to be accessed.
If an accessed register is not matched by any range from the table, it
will be accessed directly (without any paging).

If this is better alternative, I can continue with it. In other case,
I may send you the cleaned-up patch of current solution.

Best Regards,
Krystian