diff mbox

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

Message ID 1460569154-25030-4-git-send-email-srinivas.kandagatla@linaro.org
State New
Headers show

Commit Message

Srinivas Kandagatla April 13, 2016, 5:39 p.m. UTC 
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(-)

-- 
2.5.0

Comments

Mark Brown April 14, 2016, 6:42 a.m. UTC | #1 
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.
Srinivas Kandagatla April 14, 2016, 12:35 p.m. UTC | #2 
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
Mark Brown April 14, 2016, 3:18 p.m. UTC | #3 
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.
Srinivas Kandagatla April 14, 2016, 4:48 p.m. UTC | #4 
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
>
diff mbox

Patch

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;