[1/2] i2c: ismt: 16-byte align the DMA buffer address

Message ID 20170818160128.21228-2-radu.rendec@gmail.com
State New
Headers show

Commit Message

Radu Rendec Aug. 18, 2017, 4:01 p.m.
Use only a portion of the data buffer for DMA transfers, which is always
16-byte aligned. This makes the DMA buffer address 16-byte aligned and
compensates for spurious hardware parity errors that may appear when the
DMA buffer address is not 16-byte aligned.

The data buffer is enlarged in order to accommodate any possible 16-byte
alignment offset and changes the DMA code to only use a portion of the
data buffer, which is 16-byte aligned.

The symptom of the hardware issue is the same as the one addressed in
v3.12-rc2-5-gbf41691 and manifests by transfers failing with EIO, with
bit 9 being set in the ERRSTS register.

Signed-off-by: Radu Rendec <radu.rendec@gmail.com>
---
 drivers/i2c/busses/i2c-ismt.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

Comments

Wolfram Sang Aug. 27, 2017, 2:37 p.m. | #1
On Fri, Aug 18, 2017 at 05:01:27PM +0100, Radu Rendec wrote:
> Use only a portion of the data buffer for DMA transfers, which is always
> 16-byte aligned. This makes the DMA buffer address 16-byte aligned and
> compensates for spurious hardware parity errors that may appear when the
> DMA buffer address is not 16-byte aligned.
> 
> The data buffer is enlarged in order to accommodate any possible 16-byte
> alignment offset and changes the DMA code to only use a portion of the
> data buffer, which is 16-byte aligned.
> 
> The symptom of the hardware issue is the same as the one addressed in
> v3.12-rc2-5-gbf41691 and manifests by transfers failing with EIO, with
> bit 9 being set in the ERRSTS register.
> 
> Signed-off-by: Radu Rendec <radu.rendec@gmail.com>

Please use scripts/get_maintainers.pl as cc-cmd of git-send-email. This
would have added Seth and Neil (driver maintainers) to CC automatically.
I have done so now.

> ---
>  drivers/i2c/busses/i2c-ismt.c | 37 +++++++++++++++++++------------------
>  1 file changed, 19 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
> index e98e44e..ccce0ca 100644
> --- a/drivers/i2c/busses/i2c-ismt.c
> +++ b/drivers/i2c/busses/i2c-ismt.c
> @@ -172,7 +172,7 @@ struct ismt_priv {
>  	dma_addr_t io_rng_dma;			/* descriptor HW base addr */
>  	u8 head;				/* ring buffer head pointer */
>  	struct completion cmp;			/* interrupt completion */
> -	u8 dma_buffer[I2C_SMBUS_BLOCK_MAX + 1];	/* temp R/W data buffer */
> +	u8 buffer[I2C_SMBUS_BLOCK_MAX + 16];	/* temp R/W data buffer */

The IIO subsystems uses ____cacheline_aligned. I have never looked into
this closely, but it probably is cleaner than working with ALIGN on an
oversized buffer? Dunno why ____cacheline_aligned has four underscores
at the beginning, though...

>  };
>  
>  /**
> @@ -320,7 +320,7 @@ static int ismt_process_desc(const struct ismt_desc *desc,
>  			     struct ismt_priv *priv, int size,
>  			     char read_write)
>  {
> -	u8 *dma_buffer = priv->dma_buffer;
> +	u8 *dma_buffer = (u8*)ALIGN((unsigned long)&priv->buffer[0], 16);

The fixed value here might not work on all (future?) generations?


>  
>  	dev_dbg(&priv->pci_dev->dev, "Processing completed descriptor\n");
>  	__ismt_desc_dump(&priv->pci_dev->dev, desc);
> @@ -388,11 +388,12 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  	struct ismt_desc *desc;
>  	struct ismt_priv *priv = i2c_get_adapdata(adap);
>  	struct device *dev = &priv->pci_dev->dev;
> +	u8 *dma_buffer = (u8*)ALIGN((unsigned long)&priv->buffer[0], 16);
>  
>  	desc = &priv->hw[priv->head];
>  
>  	/* Initialize the DMA buffer */
> -	memset(priv->dma_buffer, 0, sizeof(priv->dma_buffer));
> +	memset(priv->buffer, 0, sizeof(priv->buffer));
>  
>  	/* Initialize the descriptor */
>  	memset(desc, 0, sizeof(struct ismt_desc));
> @@ -441,8 +442,8 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  			desc->wr_len_cmd = 2;
>  			dma_size = 2;
>  			dma_direction = DMA_TO_DEVICE;
> -			priv->dma_buffer[0] = command;
> -			priv->dma_buffer[1] = data->byte;
> +			dma_buffer[0] = command;
> +			dma_buffer[1] = data->byte;
>  		} else {
>  			/* Read Byte */
>  			dev_dbg(dev, "I2C_SMBUS_BYTE_DATA:  READ\n");
> @@ -461,9 +462,9 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  			desc->wr_len_cmd = 3;
>  			dma_size = 3;
>  			dma_direction = DMA_TO_DEVICE;
> -			priv->dma_buffer[0] = command;
> -			priv->dma_buffer[1] = data->word & 0xff;
> -			priv->dma_buffer[2] = data->word >> 8;
> +			dma_buffer[0] = command;
> +			dma_buffer[1] = data->word & 0xff;
> +			dma_buffer[2] = data->word >> 8;
>  		} else {
>  			/* Read Word */
>  			dev_dbg(dev, "I2C_SMBUS_WORD_DATA:  READ\n");
> @@ -481,9 +482,9 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  		desc->rd_len = 2;
>  		dma_size = 3;
>  		dma_direction = DMA_BIDIRECTIONAL;
> -		priv->dma_buffer[0] = command;
> -		priv->dma_buffer[1] = data->word & 0xff;
> -		priv->dma_buffer[2] = data->word >> 8;
> +		dma_buffer[0] = command;
> +		dma_buffer[1] = data->word & 0xff;
> +		dma_buffer[2] = data->word >> 8;
>  		break;
>  
>  	case I2C_SMBUS_BLOCK_DATA:
> @@ -494,8 +495,8 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  			dma_direction = DMA_TO_DEVICE;
>  			desc->wr_len_cmd = dma_size;
>  			desc->control |= ISMT_DESC_BLK;
> -			priv->dma_buffer[0] = command;
> -			memcpy(&priv->dma_buffer[1], &data->block[1], dma_size - 1);
> +			dma_buffer[0] = command;
> +			memcpy(&dma_buffer[1], &data->block[1], dma_size - 1);
>  		} else {
>  			/* Block Read */
>  			dev_dbg(dev, "I2C_SMBUS_BLOCK_DATA:  READ\n");
> @@ -522,8 +523,8 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  			dma_direction = DMA_TO_DEVICE;
>  			desc->wr_len_cmd = dma_size;
>  			desc->control |= ISMT_DESC_I2C;
> -			priv->dma_buffer[0] = command;
> -			memcpy(&priv->dma_buffer[1], &data->block[1], dma_size - 1);
> +			dma_buffer[0] = command;
> +			memcpy(&dma_buffer[1], &data->block[1], dma_size - 1);
>  		} else {
>  			/* i2c Block Read */
>  			dev_dbg(dev, "I2C_SMBUS_I2C_BLOCK_DATA:  READ\n");
> @@ -552,18 +553,18 @@ static int ismt_access(struct i2c_adapter *adap, u16 addr,
>  	if (dma_size != 0) {
>  		dev_dbg(dev, " dev=%p\n", dev);
>  		dev_dbg(dev, " data=%p\n", data);
> -		dev_dbg(dev, " dma_buffer=%p\n", priv->dma_buffer);
> +		dev_dbg(dev, " dma_buffer=%p\n", dma_buffer);
>  		dev_dbg(dev, " dma_size=%d\n", dma_size);
>  		dev_dbg(dev, " dma_direction=%d\n", dma_direction);
>  
>  		dma_addr = dma_map_single(dev,
> -				      priv->dma_buffer,
> +				      dma_buffer,
>  				      dma_size,
>  				      dma_direction);
>  
>  		if (dma_mapping_error(dev, dma_addr)) {
>  			dev_err(dev, "Error in mapping dma buffer %p\n",
> -				priv->dma_buffer);
> +				dma_buffer);
>  			return -EIO;
>  		}
>  
> -- 
> 2.9.5
>
Radu Rendec Aug. 29, 2017, 4:37 p.m. | #2
On Sun, 2017-08-27 at 16:37 +0200, Wolfram Sang wrote:
> Please use scripts/get_maintainers.pl as cc-cmd of git-send-email. This
> would have added Seth and Neil (driver maintainers) to CC automatically.
> I have done so now.

My bad; sorry about that and thanks for adding Seth and Neil.

> The IIO subsystems uses ____cacheline_aligned. I have never looked into
> this closely, but it probably is cleaner than working with ALIGN on an
> oversized buffer? Dunno why ____cacheline_aligned has four underscores
> at the beginning, though...

I haven't looked into that either; it seems that ____cacheline_aligned
is just __attribute__((__aligned__(SMP_CACHE_BYTES))) unless already
defined by the architecture (see include/linux/cache.h).

I believe using this on a (whole) structure only makes sense with
statically allocated structures and instructs the compiler to allocate
the structure at an aligned address. My guess is that it has no effect
on dynamically allocated structures (such as struct ismt_priv).

If you were thinking about using ____cacheline_aligned just on the
dma_buffer field, then it would align the field with respect to the
beginning of the structure (i.e. insert the required padding), but the
field would still be misaligned if the structure itself is misaligned.

> @@ -320,7 +320,7 @@ static int ismt_process_desc(const struct
> > ismt_desc *desc,
> >  			     struct ismt_priv *priv, int size,
> >  			     char read_write)
> >  {
> > -	u8 *dma_buffer = priv->dma_buffer;
> > +	u8 *dma_buffer = (u8*)ALIGN((unsigned long)&priv->buffer[0], 16);
> 
> The fixed value here might not work on all (future?) generations?

Good question. In theory this alignment should not be necessary at all.
There is no such requirement in the public datasheet, which is probably
why it was not implemented in the driver in the first place.

This is related to some hardware errata and the problem probably occurs
only with some specific hardware revisions. Hard to say what happens
with future generations. But at least it should not make any difference
to the other (existing) generations.

In any case, it's probably better to use a macro (e.g. #define
ISMT_DMA_ALIGN 16) rather than hardcoding the "16" value.

Radu

Patch

diff --git a/drivers/i2c/busses/i2c-ismt.c b/drivers/i2c/busses/i2c-ismt.c
index e98e44e..ccce0ca 100644
--- a/drivers/i2c/busses/i2c-ismt.c
+++ b/drivers/i2c/busses/i2c-ismt.c
@@ -172,7 +172,7 @@  struct ismt_priv {
 	dma_addr_t io_rng_dma;			/* descriptor HW base addr */
 	u8 head;				/* ring buffer head pointer */
 	struct completion cmp;			/* interrupt completion */
-	u8 dma_buffer[I2C_SMBUS_BLOCK_MAX + 1];	/* temp R/W data buffer */
+	u8 buffer[I2C_SMBUS_BLOCK_MAX + 16];	/* temp R/W data buffer */
 };
 
 /**
@@ -320,7 +320,7 @@  static int ismt_process_desc(const struct ismt_desc *desc,
 			     struct ismt_priv *priv, int size,
 			     char read_write)
 {
-	u8 *dma_buffer = priv->dma_buffer;
+	u8 *dma_buffer = (u8*)ALIGN((unsigned long)&priv->buffer[0], 16);
 
 	dev_dbg(&priv->pci_dev->dev, "Processing completed descriptor\n");
 	__ismt_desc_dump(&priv->pci_dev->dev, desc);
@@ -388,11 +388,12 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 	struct ismt_desc *desc;
 	struct ismt_priv *priv = i2c_get_adapdata(adap);
 	struct device *dev = &priv->pci_dev->dev;
+	u8 *dma_buffer = (u8*)ALIGN((unsigned long)&priv->buffer[0], 16);
 
 	desc = &priv->hw[priv->head];
 
 	/* Initialize the DMA buffer */
-	memset(priv->dma_buffer, 0, sizeof(priv->dma_buffer));
+	memset(priv->buffer, 0, sizeof(priv->buffer));
 
 	/* Initialize the descriptor */
 	memset(desc, 0, sizeof(struct ismt_desc));
@@ -441,8 +442,8 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 			desc->wr_len_cmd = 2;
 			dma_size = 2;
 			dma_direction = DMA_TO_DEVICE;
-			priv->dma_buffer[0] = command;
-			priv->dma_buffer[1] = data->byte;
+			dma_buffer[0] = command;
+			dma_buffer[1] = data->byte;
 		} else {
 			/* Read Byte */
 			dev_dbg(dev, "I2C_SMBUS_BYTE_DATA:  READ\n");
@@ -461,9 +462,9 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 			desc->wr_len_cmd = 3;
 			dma_size = 3;
 			dma_direction = DMA_TO_DEVICE;
-			priv->dma_buffer[0] = command;
-			priv->dma_buffer[1] = data->word & 0xff;
-			priv->dma_buffer[2] = data->word >> 8;
+			dma_buffer[0] = command;
+			dma_buffer[1] = data->word & 0xff;
+			dma_buffer[2] = data->word >> 8;
 		} else {
 			/* Read Word */
 			dev_dbg(dev, "I2C_SMBUS_WORD_DATA:  READ\n");
@@ -481,9 +482,9 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 		desc->rd_len = 2;
 		dma_size = 3;
 		dma_direction = DMA_BIDIRECTIONAL;
-		priv->dma_buffer[0] = command;
-		priv->dma_buffer[1] = data->word & 0xff;
-		priv->dma_buffer[2] = data->word >> 8;
+		dma_buffer[0] = command;
+		dma_buffer[1] = data->word & 0xff;
+		dma_buffer[2] = data->word >> 8;
 		break;
 
 	case I2C_SMBUS_BLOCK_DATA:
@@ -494,8 +495,8 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 			dma_direction = DMA_TO_DEVICE;
 			desc->wr_len_cmd = dma_size;
 			desc->control |= ISMT_DESC_BLK;
-			priv->dma_buffer[0] = command;
-			memcpy(&priv->dma_buffer[1], &data->block[1], dma_size - 1);
+			dma_buffer[0] = command;
+			memcpy(&dma_buffer[1], &data->block[1], dma_size - 1);
 		} else {
 			/* Block Read */
 			dev_dbg(dev, "I2C_SMBUS_BLOCK_DATA:  READ\n");
@@ -522,8 +523,8 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 			dma_direction = DMA_TO_DEVICE;
 			desc->wr_len_cmd = dma_size;
 			desc->control |= ISMT_DESC_I2C;
-			priv->dma_buffer[0] = command;
-			memcpy(&priv->dma_buffer[1], &data->block[1], dma_size - 1);
+			dma_buffer[0] = command;
+			memcpy(&dma_buffer[1], &data->block[1], dma_size - 1);
 		} else {
 			/* i2c Block Read */
 			dev_dbg(dev, "I2C_SMBUS_I2C_BLOCK_DATA:  READ\n");
@@ -552,18 +553,18 @@  static int ismt_access(struct i2c_adapter *adap, u16 addr,
 	if (dma_size != 0) {
 		dev_dbg(dev, " dev=%p\n", dev);
 		dev_dbg(dev, " data=%p\n", data);
-		dev_dbg(dev, " dma_buffer=%p\n", priv->dma_buffer);
+		dev_dbg(dev, " dma_buffer=%p\n", dma_buffer);
 		dev_dbg(dev, " dma_size=%d\n", dma_size);
 		dev_dbg(dev, " dma_direction=%d\n", dma_direction);
 
 		dma_addr = dma_map_single(dev,
-				      priv->dma_buffer,
+				      dma_buffer,
 				      dma_size,
 				      dma_direction);
 
 		if (dma_mapping_error(dev, dma_addr)) {
 			dev_err(dev, "Error in mapping dma buffer %p\n",
-				priv->dma_buffer);
+				dma_buffer);
 			return -EIO;
 		}