[PATCH v1 0/3] nvmem: core: fix regmap accessor usage.

[PATCH v1 0/3] nvmem: core: fix regmap accessor usage.

[Srinivas Kandagatla]

With recent patch 922a9f936e40 ("regmap: mmio: Convert to regmap_bus
and fix accessor usage") nvmem providers based on regmap-mmio stopped
working, as nvmem core was using raw accessors.

This patch series adds new regmap_can_raw_read() api to check
if the regmap supports raw access. And the follow on nvmem patch uses
this api to fix the issue with nvmem providers based on regmap mmio.

Thanks,
srini

Srinivas Kandagatla (3):
  regmap: add regmap_can_raw_read() api
  regmap: add dummy regmap_can_raw_write() to header
  nvmem: core: fix regmap accessor usage

 drivers/base/regmap/regmap.c | 13 ++++++-
 drivers/nvmem/core.c         | 90 ++++++++++++++++++++++++++++++++++++++++----
 include/linux/regmap.h       | 13 +++++++
 3 files changed, 107 insertions(+), 9 deletions(-)

-- 
2.5.0

[PATCH v1 1/3] regmap: add regmap_can_raw_read() api

[Srinivas Kandagatla]

This patch adds regmap_can_raw_read() api so that users like nvmem can
check if the regmap is capable of doing a raw accessors. This can also
be used by other infrastructures like nvmem which are based on regmap.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/base/regmap/regmap.c | 13 ++++++++++++-
 include/linux/regmap.h       |  7 +++++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/base/regmap/regmap.c b/drivers/base/regmap/regmap.c
index df2d2ef..23f17fd 100644
--- a/drivers/base/regmap/regmap.c
+++ b/drivers/base/regmap/regmap.c
@@ -1482,6 +1482,18 @@ int _regmap_raw_write(struct regmap *map, unsigned int reg,
 }
 
 /**
+ * regmap_can_raw_read - Test if regmap_raw_read() is supported
+ *
+ * @map: Map to check.
+ */
+bool regmap_can_raw_read(struct regmap *map)
+{
+	return map->bus && map->bus->read && map->format.format_val &&
+		map->format.format_reg;
+}
+EXPORT_SYMBOL_GPL(regmap_can_raw_read);
+
+/**
  * regmap_can_raw_write - Test if regmap_raw_write() is supported
  *
  * @map: Map to check.
@@ -2492,7 +2504,6 @@ int regmap_bulk_read(struct regmap *map, unsigned int reg, void *val,
 		 * them we have a series of single read operations.
 		 */
 		size_t total_size = val_bytes * val_count;
-
 		if (!map->use_single_read &&
 		    (!map->max_raw_read || map->max_raw_read > total_size)) {
 			ret = regmap_raw_read(map, reg, val,
diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3dc08ce..be3da4c 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -728,6 +728,7 @@ int regmap_get_val_bytes(struct regmap *map);
 int regmap_get_max_register(struct regmap *map);
 int regmap_get_reg_stride(struct regmap *map);
 int regmap_async_complete(struct regmap *map);
+bool regmap_can_raw_read(struct regmap *map);
 bool regmap_can_raw_write(struct regmap *map);
 size_t regmap_get_raw_read_max(struct regmap *map);
 size_t regmap_get_raw_write_max(struct regmap *map);
@@ -1045,6 +1046,12 @@ static inline void regmap_async_complete(struct regmap *map)
 	WARN_ONCE(1, "regmap API is disabled");
 }
 
+static inline bool regmap_can_raw_read(struct regmap *map)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return false;
+}
+
 static inline int regmap_register_patch(struct regmap *map,
 					const struct reg_sequence *regs,
 					int num_regs)
-- 
2.5.0

Re: [PATCH v1 1/3] regmap: add regmap_can_raw_read() api

[Mark Brown]

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

On Wed, Apr 13, 2016 at 06:39:12PM +0100, Srinivas Kandagatla wrote:
> This patch adds regmap_can_raw_read() api so that users like nvmem can
> check if the regmap is capable of doing a raw accessors. This can also
> be used by other infrastructures like nvmem which are based on regmap.

This seems tasteless.  If the driver is using raw I/O it needs to know
exactly what device it is working with, it shouldn't need to ask the
core about the properties of the device.  If it does that's a sign that
there's an abstraction problem somewhere along the line.  Most likely
the driver should be using bulk I/O instead.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v1 2/3] regmap: add dummy regmap_can_raw_write() to header

[Srinivas Kandagatla]

This patch adds dummy function for regmap_can_raw_write() in the header
file, so that the code can be atleast compiled without regmap enabled.
This kind of setup is mostly tested using randomconfig or zero
day-testing.

Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 include/linux/regmap.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index be3da4c..c060b9b 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -1052,6 +1052,12 @@ static inline bool regmap_can_raw_read(struct regmap *map)
 	return false;
 }
 
+static inline bool regmap_can_raw_write(struct regmap *map)
+{
+	WARN_ONCE(1, "regmap API is disabled");
+	return false;
+}
+
 static inline int regmap_register_patch(struct regmap *map,
 					const struct reg_sequence *regs,
 					int num_regs)
-- 
2.5.0

Re: [PATCH v1 2/3] regmap: add dummy regmap_can_raw_write() to header

[Mark Brown]

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

On Wed, Apr 13, 2016 at 06:39:13PM +0100, Srinivas Kandagatla wrote:
> This patch adds dummy function for regmap_can_raw_write() in the header
> file, so that the code can be atleast compiled without regmap enabled.
> This kind of setup is mostly tested using randomconfig or zero
> day-testing.

I'm not convinced that this isn't flagging real problems - it's hard to
see how code can safely and sensibly be written which would use this
without a dependency on regmap (and given that and the fact that we have
no external users we should probably make it internal to the API).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH v1 3/3] nvmem: core: fix regmap accessor usage

[Srinivas Kandagatla]

With the recent patch of removing the raw accessors form regmap-mmio,
broke the qfprom support. This patch attempts to fix that regression
by adding check before calling regmap raw accessors functions.

Without this patch nvmem providers based on regmap mmio would not work.

Reported-by: Rajendra Nayak <rjendra@qti.qualcomm.com>
Fixes: 922a9f936e40 ("regmap: mmio: Convert to regmap_bus and fix accessor usage")
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
---
 drivers/nvmem/core.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 82 insertions(+), 8 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 0de3d87..8da631c 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -67,6 +67,80 @@ static struct lock_class_key eeprom_lock_key;
 
 #define to_nvmem_device(d) container_of(d, struct nvmem_device, dev)
 
+static int nvmem_reg_read(struct nvmem_device *nvmem, unsigned int reg,
+			  void *val, size_t bytes)
+{
+	int i, ret = 0;
+	struct regmap *map = nvmem->regmap;
+	size_t word_count = bytes / nvmem->word_size;
+	unsigned int ival;
+	u32 *u32_buf = val;
+	u16 *u16_buf = val;
+	u8 *u8_buf = val;
+
+	if (regmap_can_raw_read(map))
+		return regmap_raw_read(map, reg, val, bytes);
+
+	for (i = 0; i < word_count; i++) {
+		ret = regmap_read(map, reg + i * nvmem->stride, &ival);
+		if (ret != 0)
+			return ret;
+
+		switch (nvmem->word_size) {
+		case 4:
+			u32_buf[i] = ival;
+			break;
+		case 2:
+			u16_buf[i] = ival;
+			break;
+		case 1:
+			u8_buf[i] = ival;
+			break;
+		default:
+			return -EINVAL;
+		}
+	}
+
+	return ret;
+}
+
+static int nvmem_reg_write(struct nvmem_device *nvmem, unsigned int reg,
+			   void *val, size_t bytes)
+{
+	int i, ret = 0;
+	struct regmap *map = nvmem->regmap;
+	size_t word_count = bytes / nvmem->word_size;
+	unsigned int ival;
+	u32 *u32_buf = val;
+	u16 *u16_buf = val;
+	u8 *u8_buf = val;
+
+	if (regmap_can_raw_write(map))
+		return regmap_raw_write(map, reg, val, bytes);
+
+	for (i = 0; i < word_count; i++) {
+		switch (nvmem->word_size) {
+		case 4:
+			ival =  u32_buf[i];
+			break;
+		case 2:
+			ival =  u16_buf[i];
+			break;
+		case 1:
+			ival =  u8_buf[i];
+			break;
+		default:
+			return -EINVAL;
+		}
+
+		ret = regmap_write(map, reg + i * nvmem->stride, ival);
+		if (ret != 0)
+			return ret;
+	}
+
+	return ret;
+}
+
 static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 				    struct bin_attribute *attr,
 				    char *buf, loff_t pos, size_t count)
@@ -93,7 +167,7 @@ static ssize_t bin_attr_nvmem_read(struct file *filp, struct kobject *kobj,
 
 	count = round_down(count, nvmem->word_size);
 
-	rc = regmap_raw_read(nvmem->regmap, pos, buf, count);
+	rc = nvmem_reg_read(nvmem, pos, buf, count);
 
 	if (IS_ERR_VALUE(rc))
 		return rc;
@@ -127,7 +201,7 @@ static ssize_t bin_attr_nvmem_write(struct file *filp, struct kobject *kobj,
 
 	count = round_down(count, nvmem->word_size);
 
-	rc = regmap_raw_write(nvmem->regmap, pos, buf, count);
+	rc = nvmem_reg_write(nvmem, pos, buf, count);
 
 	if (IS_ERR_VALUE(rc))
 		return rc;
@@ -948,7 +1022,7 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 {
 	int rc;
 
-	rc = regmap_raw_read(nvmem->regmap, cell->offset, buf, cell->bytes);
+	rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
 
 	if (IS_ERR_VALUE(rc))
 		return rc;
@@ -1014,7 +1088,7 @@ static inline void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 		*b <<= bit_offset;
 
 		/* setup the first byte with lsb bits from nvmem */
-		rc = regmap_raw_read(nvmem->regmap, cell->offset, &v, 1);
+		rc = nvmem_reg_read(nvmem, cell->offset, &v, 1);
 		*b++ |= GENMASK(bit_offset - 1, 0) & v;
 
 		/* setup rest of the byte if any */
@@ -1031,7 +1105,7 @@ static inline void *nvmem_cell_prepare_write_buffer(struct nvmem_cell *cell,
 	/* if it's not end on byte boundary */
 	if ((nbits + bit_offset) % BITS_PER_BYTE) {
 		/* setup the last byte with msb bits from nvmem */
-		rc = regmap_raw_read(nvmem->regmap,
+		rc = nvmem_reg_read(nvmem,
 				    cell->offset + cell->bytes - 1, &v, 1);
 		*p |= GENMASK(7, (nbits + bit_offset) % BITS_PER_BYTE) & v;
 
@@ -1064,7 +1138,7 @@ int nvmem_cell_write(struct nvmem_cell *cell, void *buf, size_t len)
 			return PTR_ERR(buf);
 	}
 
-	rc = regmap_raw_write(nvmem->regmap, cell->offset, buf, cell->bytes);
+	rc = nvmem_reg_write(nvmem, cell->offset, buf, cell->bytes);
 
 	/* free the tmp buffer */
 	if (cell->bit_offset || cell->nbits)
@@ -1155,7 +1229,7 @@ int nvmem_device_read(struct nvmem_device *nvmem,
 	if (!nvmem || !nvmem->regmap)
 		return -EINVAL;
 
-	rc = regmap_raw_read(nvmem->regmap, offset, buf, bytes);
+	rc = nvmem_reg_read(nvmem, offset, buf, bytes);
 
 	if (IS_ERR_VALUE(rc))
 		return rc;
@@ -1183,7 +1257,7 @@ int nvmem_device_write(struct nvmem_device *nvmem,
 	if (!nvmem || !nvmem->regmap)
 		return -EINVAL;
 
-	rc = regmap_raw_write(nvmem->regmap, offset, buf, bytes);
+	rc = nvmem_reg_write(nvmem, offset, buf, bytes);
 
 	if (IS_ERR_VALUE(rc))
 		return rc;
-- 
2.5.0

Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage

[Mark Brown]

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

On Wed, Apr 13, 2016 at 06:39:14PM +0100, Srinivas Kandagatla wrote:
> With the recent patch of removing the raw accessors form regmap-mmio,
> broke the qfprom support. This patch attempts to fix that regression
> by adding check before calling regmap raw accessors functions.
> 
> Without this patch nvmem providers based on regmap mmio would not work.

Ugh, this seems incredibly confused.  Why is this even using regmap?  It
doesn't seem to use anything from the framework - it only ever accesses
the device with raw accessors which bypass pretty much everything the
framework has to offer.  The raw accessors are there for things like
copying preformatted images to devices where we have a specific need to
do a subset of operations where the framework doesn't add anything, they
shouldn't be the only thing a driver ever does.  If they are then all
regmap is doing is adding a bunch of overhead and making the code more
complex.

Looking at the regmap_bus implementations in the subsystem is very
worrying too, there's lots of dummy functions in there which are never a
good sign and pretty much every bus looks like it really should be using
reg_read() and reg_write() operations but is contorting itself to do
bytestream access instead - things like parsing data out of the buffers
are not good signs as they indicate that the buses are hooking in at the
wrong abstraction layer.

> +	if (regmap_can_raw_read(map))
> +		return regmap_raw_read(map, reg, val, bytes);
> +
> +	for (i = 0; i < word_count; i++) {
> +		ret = regmap_read(map, reg + i * nvmem->stride, &ival);
> +		if (ret != 0)
> +			return ret;
> +
> +		switch (nvmem->word_size) {
> +		case 4:
> +			u32_buf[i] = ival;
> +			break;

This is clearly an abstraction failure and probably broken for systems
where the device endianness does not match the CPU endianness (like most
big endian systems, the device hardware normally stays in little endian
mode).

We need to figure out what this stuff is trying to do before we go any
further, I'm honestly not entirely clear.  I *think* that if regmap is a
good fit then it probably wants to use the bulk operations rather than
the raw operations (the bulk operations are AFAICT what is being open
coded above) but bulk I/O still does endianness handling and I'm not
sure if that's desired or not.  If the nvmem code really is just trying
to get bytestreams then regmap really isn't what it should be using,
it's all about dealing with registers and trying to force bytestreams
through it seems like it's just going to lead to fragility.  Either
whatever is happening should be abstracted within regmap or we shouldn't
be using regmap.

I'll try to have another look at this later.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage

[Srinivas Kandagatla]

On 14/04/16 07:42, Mark Brown wrote:
> On Wed, Apr 13, 2016 at 06:39:14PM +0100, Srinivas Kandagatla wrote:
>> With the recent patch of removing the raw accessors form regmap-mmio,
>> broke the qfprom support. This patch attempts to fix that regression
>> by adding check before calling regmap raw accessors functions.
>>
>> Without this patch nvmem providers based on regmap mmio would not work.
>
...
>> +	if (regmap_can_raw_read(map))
>> +		return regmap_raw_read(map, reg, val, bytes);
>> +
>> +	for (i = 0; i < word_count; i++) {
>> +		ret = regmap_read(map, reg + i * nvmem->stride, &ival);
>> +		if (ret != 0)
>> +			return ret;
>> +
>> +		switch (nvmem->word_size) {
>> +		case 4:
>> +			u32_buf[i] = ival;
>> +			break;
>
> This is clearly an abstraction failure and probably broken for systems
> where the device endianness does not match the CPU endianness (like most
> big endian systems, the device hardware normally stays in little endian
> mode).
>
> We need to figure out what this stuff is trying to do before we go any
> further, I'm honestly not entirely clear.  I *think* that if regmap is a
> good fit then it probably wants to use the bulk operations rather than
> the raw operations (the bulk operations are AFAICT what is being open
> coded above) but bulk I/O still does endianness handling and I'm not
> sure if that's desired or not.  If the nvmem code really is just trying
> to get bytestreams then regmap really isn't what it should be using,
> it's all about dealing with registers and trying to force bytestreams
> through it seems like it's just going to lead to fragility.  Either
> whatever is happening should be abstracted within regmap or we shouldn't
> be using regmap.
Thanks for your comments,

I totally agree that there is an abstraction failure here in both sides. 
It should be fixed. moving to using bulk apis would solve the nvmem 
problem for now. But for long term, using regmap should be totally 
removed from nvmem and directly use the reg read/write callbacks from 
nvmem providers, This would be much robust solution. This was indeed 
Maxime's first proposal. I will try to fix it up and see how it looks 
without regmap.

>
> I'll try to have another look at this later.
>

thanks,
srini

Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage

[Mark Brown]

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

On Thu, Apr 14, 2016 at 01:35:03PM +0100, Srinivas Kandagatla wrote:

> I totally agree that there is an abstraction failure here in both sides. It
> should be fixed. moving to using bulk apis would solve the nvmem problem for
> now. But for long term, using regmap should be totally removed from nvmem
> and directly use the reg read/write callbacks from nvmem providers, This
> would be much robust solution. This was indeed Maxime's first proposal. I
> will try to fix it up and see how it looks without regmap.

OK, so just replacing all the _raw_ calls with _bulk_ for now?  If
you're doing that watch out for the fact that the reads come back native
endian which might upset things.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH v1 3/3] nvmem: core: fix regmap accessor usage

[Srinivas Kandagatla]

On 14/04/16 16:18, Mark Brown wrote:
> On Thu, Apr 14, 2016 at 01:35:03PM +0100, Srinivas Kandagatla wrote:
>
>> I totally agree that there is an abstraction failure here in both sides. It
>> should be fixed. moving to using bulk apis would solve the nvmem problem for
>> now. But for long term, using regmap should be totally removed from nvmem
>> and directly use the reg read/write callbacks from nvmem providers, This
>> would be much robust solution. This was indeed Maxime's first proposal. I
>> will try to fix it up and see how it looks without regmap.
>
> OK, so just replacing all the _raw_ calls with _bulk_ for now?  If
> you're doing that watch out for the fact that the reads come back native
> endian which might upset things.
Yep, that would confuse users. I think its better I do a long term 
solution of regmap replacement with callbacks before someone else starts 
reporting issues.

--srini
>