2014-04-04 03:24:23

by Yuan Yao

[permalink] [raw]
Subject: [PATCH v4 0/2] i2c: add DMA support for freescale i2c driver


Changed in v4:
- cancelled "i2c_imx->use_dma".
- changed "Dma" to "DMA".
- add timeout handling for DMA transfer complete.

Changed in v3:
- fix a bug when request the DMA faild.
- some minor fixes for coding style.
- other minor fixes.

Changed in v2:
- remove has_dma_support property
- unify i2c_imx_dma_rx and i2c_imx_dma_tx
- unify i2c_imx_dma_read and i2c_imx_pio_read
- unify i2c_imx_dma_write and i2c_imx_pio_write

Added in v1:
- Enable DMA if it's support DMA and transfer size bigger than the threshold.
- Add device tree bindings for i2c eDMA support.
- Add eDMA support for i2c driver.


2014-04-04 03:24:38

by Yuan Yao

[permalink] [raw]
Subject: [PATCH v4 2/2] Documentation:add DMA support for freescale i2c driver

Add i2c dts node properties for eDMA support, them depend on the eDMA driver.

Signed-off-by: Yuan Yao <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-imx.txt | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-imx.txt b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
index 4a8513e..52d37fd 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-imx.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-imx.txt
@@ -11,6 +11,8 @@ Required properties:
Optional properties:
- clock-frequency : Constains desired I2C/HS-I2C bus clock frequency in Hz.
The absence of the propoerty indicates the default frequency 100 kHz.
+- dmas: A list of two dma specifiers, one for each entry in dma-names.
+- dma-names: should contain "tx" and "rx".

Examples:

@@ -26,3 +28,12 @@ i2c@70038000 { /* HS-I2C on i.MX51 */
interrupts = <64>;
clock-frequency = <400000>;
};
+
+i2c0: i2c@40066000 { /* i2c0 on vf610 */
+ compatible = "fsl,vf610-i2c";
+ reg = <0x40066000 0x1000>;
+ interrupts =<0 71 0x04>;
+ dmas = <&edma0 0 50>,
+ <&edma0 0 51>;
+ dma-names = "rx","tx";
+};
--
1.8.4

2014-04-04 03:24:31

by Yuan Yao

[permalink] [raw]
Subject: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

Add dma support for i2c. This function depend on DMA driver.
You can turn on it by write both the dmas and dma-name properties in dts node.

Signed-off-by: Yuan Yao <[email protected]>
---
drivers/i2c/busses/i2c-imx.c | 372 +++++++++++++++++++++++++++++++++++++------
1 file changed, 319 insertions(+), 53 deletions(-)

diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
index db895fb..3d63b35 100644
--- a/drivers/i2c/busses/i2c-imx.c
+++ b/drivers/i2c/busses/i2c-imx.c
@@ -37,22 +37,27 @@
/** Includes *******************************************************************
*******************************************************************************/

-#include <linux/init.h>
-#include <linux/kernel.h>
-#include <linux/module.h>
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/dmaengine.h>
+#include <linux/dmapool.h>
#include <linux/errno.h>
#include <linux/err.h>
#include <linux/interrupt.h>
-#include <linux/delay.h>
#include <linux/i2c.h>
+#include <linux/init.h>
#include <linux/io.h>
-#include <linux/sched.h>
-#include <linux/platform_device.h>
-#include <linux/clk.h>
-#include <linux/slab.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
#include <linux/of.h>
#include <linux/of_device.h>
+#include <linux/of_dma.h>
#include <linux/platform_data/i2c-imx.h>
+#include <linux/platform_device.h>
+#include <linux/sched.h>
+#include <linux/slab.h>

/** Defines ********************************************************************
*******************************************************************************/
@@ -63,6 +68,10 @@
/* Default value */
#define IMX_I2C_BIT_RATE 100000 /* 100kHz */

+/* enable DMA if transfer byte size is bigger than this threshold */
+#define IMX_I2C_DMA_THRESHOLD 16
+#define IMX_I2C_DMA_TIMEOUT 1000
+
/* IMX I2C registers:
* the I2C register offset is different between SoCs,
* to provid support for all these chips, split the
@@ -88,6 +97,7 @@
#define I2SR_IBB 0x20
#define I2SR_IAAS 0x40
#define I2SR_ICF 0x80
+#define I2CR_DMAEN 0x02
#define I2CR_RSTA 0x04
#define I2CR_TXAK 0x08
#define I2CR_MTX 0x10
@@ -174,6 +184,17 @@ struct imx_i2c_hwdata {
unsigned i2cr_ien_opcode;
};

+struct imx_i2c_dma {
+ struct dma_chan *chan_tx;
+ struct dma_chan *chan_rx;
+ struct dma_chan *chan_using;
+ struct completion cmd_complete;
+ dma_addr_t dma_buf;
+ unsigned int dma_len;
+ unsigned int dma_transfer_dir;
+ unsigned int dma_data_dir;
+};
+
struct imx_i2c_struct {
struct i2c_adapter adapter;
struct clk *clk;
@@ -184,6 +205,8 @@ struct imx_i2c_struct {
int stopped;
unsigned int ifdr; /* IMX_I2C_IFDR */
const struct imx_i2c_hwdata *hwdata;
+
+ struct imx_i2c_dma *dma;
};

static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
@@ -254,6 +277,132 @@ static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
return readb(i2c_imx->base + (reg << i2c_imx->hwdata->regshift));
}

+/* Functions for DMA support */
+static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
+ dma_addr_t phy_addr)
+{
+ struct imx_i2c_dma *dma;
+ struct dma_slave_config dma_sconfig;
+ struct device *dev = &i2c_imx->adapter.dev;
+ int ret;
+
+ dma = devm_kzalloc(dev, sizeof(struct imx_i2c_dma), GFP_KERNEL);
+ if (!dma) {
+ dev_info(dev, "can't allocate DMA struct\n");
+ return -ENOMEM;
+ }
+
+ dma->chan_tx = dma_request_slave_channel(dev, "tx");
+ return 0;
+ if (!dma->chan_tx) {
+ dev_info(dev, "DMA tx channel request failed\n");
+ ret = -ENODEV;
+ goto fail_al;
+ }
+
+ dma_sconfig.dst_addr = phy_addr +
+ (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.dst_maxburst = 1;
+ dma_sconfig.direction = DMA_MEM_TO_DEV;
+ ret = dmaengine_slave_config(dma->chan_tx, &dma_sconfig);
+ if (ret < 0) {
+ dev_info(dev, "DMA slave config failed, err = %d\n", ret);
+ goto fail_tx;
+ }
+
+ dma->chan_rx = dma_request_slave_channel(dev, "rx");
+ if (!dma->chan_rx) {
+ dev_info(dev, "DMA rx channel request failed\n");
+ ret = -ENODEV;
+ goto fail_tx;
+ }
+
+ dma_sconfig.src_addr = phy_addr +
+ (IMX_I2C_I2DR << i2c_imx->hwdata->regshift);
+ dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ dma_sconfig.src_maxburst = 1;
+ dma_sconfig.direction = DMA_DEV_TO_MEM;
+ ret = dmaengine_slave_config(dma->chan_rx, &dma_sconfig);
+ if (ret < 0) {
+ dev_info(dev, "DMA slave config failed, err = %d\n", ret);
+ goto fail_rx;
+ }
+
+ i2c_imx->dma = dma;
+
+ init_completion(&dma->cmd_complete);
+
+ return 0;
+
+fail_rx:
+ dma_release_channel(dma->chan_rx);
+fail_tx:
+ dma_release_channel(dma->chan_tx);
+fail_al:
+ devm_kfree(dev, dma);
+
+ return ret;
+}
+
+static void i2c_imx_dma_callback(void *arg)
+{
+ struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
+ struct imx_i2c_dma *dma = i2c_imx->dma;
+
+ dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
+ dma->dma_len, dma->dma_data_dir);
+ complete(&dma->cmd_complete);
+}
+
+static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
+ struct i2c_msg *msgs)
+{
+ struct imx_i2c_dma *dma = i2c_imx->dma;
+ struct dma_async_tx_descriptor *txdesc;
+ struct device *dev = &i2c_imx->adapter.dev;
+
+ dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf,
+ dma->dma_len, dma->dma_data_dir);
+ if (dma_mapping_error(dma->chan_using->device->dev, dma->dma_buf)) {
+ dev_err(dev, "DMA mapping failed\n");
+ return -EINVAL;
+ }
+
+ txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf,
+ dma->dma_len, dma->dma_transfer_dir,
+ DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
+ if (!txdesc) {
+ dev_err(dev, "Not able to get desc for DMA xfer\n");
+ dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
+ dma->dma_len, dma->dma_data_dir);
+ return -EINVAL;
+ }
+
+ txdesc->callback = i2c_imx_dma_callback;
+ txdesc->callback_param = i2c_imx;
+ dmaengine_submit(txdesc);
+ dma_async_issue_pending(dma->chan_using);
+
+ return 0;
+}
+
+static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
+{
+ struct imx_i2c_dma *dma = i2c_imx->dma;
+
+ dma->dma_buf = 0;
+ dma->dma_len = 0;
+
+ dma_release_channel(dma->chan_tx);
+ dma->chan_tx = NULL;
+
+ dma_release_channel(dma->chan_rx);
+ dma->chan_rx = NULL;
+
+ dma->chan_using = NULL;
+}
+
/** Functions for IMX I2C adapter driver ***************************************
*******************************************************************************/

@@ -334,6 +483,11 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)

temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp &= ~I2CR_DMAEN;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
return result;
}

@@ -427,44 +581,101 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)

static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
{
- int i, result;
+ int i, result, timeout = IMX_I2C_DMA_TIMEOUT;
+ unsigned int temp = 0;
+ struct imx_i2c_dma *dma = i2c_imx->dma;
+ struct device *dev = &i2c_imx->adapter.dev;

- dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
+ dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
__func__, msgs->addr << 1);
+ if (dma && msgs->len >= IMX_I2C_DMA_THRESHOLD) {
+ reinit_completion(&i2c_imx->dma->cmd_complete);
+ dma->chan_using = dma->chan_tx;
+ dma->dma_transfer_dir = DMA_MEM_TO_DEV;
+ dma->dma_data_dir = DMA_TO_DEVICE;
+ dma->dma_len = msgs->len - 1;
+ result = i2c_imx_dma_xfer(i2c_imx, msgs);
+ if (result)
+ return result;

- /* write slave address */
- imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
- result = i2c_imx_trx_complete(i2c_imx);
- if (result)
- return result;
- result = i2c_imx_acked(i2c_imx);
- if (result)
- return result;
- dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp |= I2CR_DMAEN;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);

- /* write data */
- for (i = 0; i < msgs->len; i++) {
- dev_dbg(&i2c_imx->adapter.dev,
- "<%s> write byte: B%d=0x%X\n",
- __func__, i, msgs->buf[i]);
- imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
+ /* write slave address */
+ imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
+ result = wait_for_completion_interruptible_timeout(
+ &i2c_imx->dma->cmd_complete,
+ msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
+ if (result <= 0) {
+ dmaengine_terminate_all(dma->chan_using);
+ if (result)
+ return result;
+ else
+ return -ETIMEDOUT;
+ }
+
+ /* waiting for Transfer complete. */
+ while (timeout--) {
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if (temp & I2SR_ICF)
+ break;
+ udelay(10);
+ }
+
+ if (!timeout)
+ return -ETIMEDOUT;
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp &= ~I2CR_DMAEN;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+ /* write the last byte */
+ imx_i2c_write_reg(msgs->buf[msgs->len-1],
+ i2c_imx, IMX_I2C_I2DR);
result = i2c_imx_trx_complete(i2c_imx);
if (result)
return result;
+
+ result = i2c_imx_acked(i2c_imx);
+ if (result)
+ return result;
+ } else {
+ /* write slave address */
+ imx_i2c_write_reg(msgs->addr << 1, i2c_imx, IMX_I2C_I2DR);
+ result = i2c_imx_trx_complete(i2c_imx);
+ if (result)
+ return result;
+
result = i2c_imx_acked(i2c_imx);
if (result)
return result;
+
+ dev_dbg(dev, "<%s> write data\n", __func__);
+
+ /* write data */
+ for (i = 0; i < msgs->len; i++) {
+ dev_dbg(dev, "<%s> write byte: B%d=0x%X\n",
+ __func__, i, msgs->buf[i]);
+ imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
+ result = i2c_imx_trx_complete(i2c_imx);
+ if (result)
+ return result;
+ result = i2c_imx_acked(i2c_imx);
+ if (result)
+ return result;
+ }
}
return 0;
}

static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
{
- int i, result;
+ int i, result, timeout = IMX_I2C_DMA_TIMEOUT;
unsigned int temp;
+ struct imx_i2c_dma *dma = i2c_imx->dma;
+ struct device *dev = &i2c_imx->adapter.dev;

- dev_dbg(&i2c_imx->adapter.dev,
- "<%s> write slave address: addr=0x%x\n",
+ dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
__func__, (msgs->addr << 1) | 0x01);

/* write slave address */
@@ -476,7 +687,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
if (result)
return result;

- dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
+ dev_dbg(dev, "<%s> setup bus\n", __func__);

/* setup bus to read data */
temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
@@ -486,35 +697,81 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */

- dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
+ dev_dbg(dev, "<%s> read data\n", __func__);

- /* read data */
- for (i = 0; i < msgs->len; i++) {
- result = i2c_imx_trx_complete(i2c_imx);
+ if (dma && msgs->len >= IMX_I2C_DMA_THRESHOLD) {
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp |= I2CR_DMAEN;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+ reinit_completion(&i2c_imx->dma->cmd_complete);
+ dma->chan_using = dma->chan_rx;
+ dma->dma_transfer_dir = DMA_DEV_TO_MEM;
+ dma->dma_data_dir = DMA_FROM_DEVICE;
+ dma->dma_len = msgs->len - 2;
+ result = i2c_imx_dma_xfer(i2c_imx, msgs);
if (result)
return result;
- if (i == (msgs->len - 1)) {
- /* It must generate STOP before read I2DR to prevent
- controller from generating another clock cycle */
- dev_dbg(&i2c_imx->adapter.dev,
- "<%s> clear MSTA\n", __func__);
- temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
- temp &= ~(I2CR_MSTA | I2CR_MTX);
- imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
- i2c_imx_bus_busy(i2c_imx, 0);
- i2c_imx->stopped = 1;
- } else if (i == (msgs->len - 2)) {
- dev_dbg(&i2c_imx->adapter.dev,
- "<%s> set TXAK\n", __func__);
- temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
- temp |= I2CR_TXAK;
- imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+ result = wait_for_completion_interruptible_timeout(
+ &i2c_imx->dma->cmd_complete,
+ msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
+ if (result <= 0) {
+ dmaengine_terminate_all(dma->chan_using);
+ if (result)
+ return result;
+ else
+ return -ETIMEDOUT;
}
- msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
- dev_dbg(&i2c_imx->adapter.dev,
- "<%s> read byte: B%d=0x%X\n",
- __func__, i, msgs->buf[i]);
+
+ /* waiting for Transfer complete. */
+ while (timeout--) {
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
+ if (temp & I2SR_ICF)
+ break;
+ udelay(10);
+ }
+
+ if(!timeout)
+ return -ETIMEDOUT;
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp &= ~I2CR_DMAEN;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+ } else {
+ /* read data */
+ for (i = 0; i < msgs->len - 2; i++) {
+ result = i2c_imx_trx_complete(i2c_imx);
+ if (result)
+ return result;
+ msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+ dev_dbg(dev, "<%s> read byte: B%d=0x%X\n",
+ __func__, i, msgs->buf[i]);
+ }
+ result = i2c_imx_trx_complete(i2c_imx);
}
+
+ /* read n-1 byte data */
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp |= I2CR_TXAK;
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+
+ msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+ /* read n byte data */
+ result = i2c_imx_trx_complete(i2c_imx);
+ if (result)
+ return result;
+
+ /*
+ * It must generate STOP before read I2DR to prevent
+ * controller from generating another clock cycle
+ */
+ temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
+ temp &= ~(I2CR_MSTA | I2CR_MTX);
+ imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
+ i2c_imx_bus_busy(i2c_imx, 0);
+ i2c_imx->stopped = 1;
+ msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
+
return 0;
}

@@ -601,6 +858,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
void __iomem *base;
int irq, ret;
u32 bitrate;
+ u32 phy_addr;

dev_dbg(&pdev->dev, "<%s>\n", __func__);

@@ -611,6 +869,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
}

res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ phy_addr = res->start;
base = devm_ioremap_resource(&pdev->dev, res);
if (IS_ERR(base))
return PTR_ERR(base);
@@ -696,6 +955,10 @@ static int i2c_imx_probe(struct platform_device *pdev)
i2c_imx->adapter.name);
dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");

+ /* Init DMA config if support*/
+ if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr))
+ dev_warn(&pdev->dev, "can't request DMA\n");
+
return 0; /* Return OK */
}

@@ -707,6 +970,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
i2c_del_adapter(&i2c_imx->adapter);

+ if (i2c_imx->dma)
+ i2c_imx_dma_free(i2c_imx);
+
/* setup chip registers to defaults */
imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
--
1.8.4

2014-04-04 09:50:27

by Sourav Poddar

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

Hi,
On Friday 04 April 2014 08:11 AM, Yuan Yao wrote:
> Add dma support for i2c. This function depend on DMA driver.
> You can turn on it by write both the dmas and dma-name properties in dts node.
>
> Signed-off-by: Yuan Yao<[email protected]>
> ---
> drivers/i2c/busses/i2c-imx.c | 372 +++++++++++++++++++++++++++++++++++++------
> 1 file changed, 319 insertions(+), 53 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c
> index db895fb..3d63b35 100644
> --- a/drivers/i2c/busses/i2c-imx.c
> +++ b/drivers/i2c/busses/i2c-imx.c
> @@ -37,22 +37,27 @@
> /** Includes *******************************************************************
> *******************************************************************************/
>
> -#include<linux/init.h>
> -#include<linux/kernel.h>
> -#include<linux/module.h>
> +#include<linux/clk.h>
> +#include<linux/completion.h>
> +#include<linux/delay.h>
> +#include<linux/dma-mapping.h>
> +#include<linux/dmaengine.h>
> +#include<linux/dmapool.h>
> #include<linux/errno.h>
> #include<linux/err.h>
> #include<linux/interrupt.h>
> -#include<linux/delay.h>
> #include<linux/i2c.h>
> +#include<linux/init.h>
> #include<linux/io.h>
> -#include<linux/sched.h>
> -#include<linux/platform_device.h>
> -#include<linux/clk.h>
> -#include<linux/slab.h>
> +#include<linux/kernel.h>
> +#include<linux/module.h>
> #include<linux/of.h>
> #include<linux/of_device.h>
> +#include<linux/of_dma.h>
> #include<linux/platform_data/i2c-imx.h>
> +#include<linux/platform_device.h>
> +#include<linux/sched.h>
> +#include<linux/slab.h>
>
> /** Defines ********************************************************************
> *******************************************************************************/
> @@ -63,6 +68,10 @@
> /* Default value */
> #define IMX_I2C_BIT_RATE 100000 /* 100kHz */
>
> +/* enable DMA if transfer byte size is bigger than this threshold */
> +#define IMX_I2C_DMA_THRESHOLD 16
> +#define IMX_I2C_DMA_TIMEOUT 1000
> +
> /* IMX I2C registers:
> * the I2C register offset is different between SoCs,
> * to provid support for all these chips, split the
> @@ -88,6 +97,7 @@
> #define I2SR_IBB 0x20
> #define I2SR_IAAS 0x40
> #define I2SR_ICF 0x80
> +#define I2CR_DMAEN 0x02
> #define I2CR_RSTA 0x04
> #define I2CR_TXAK 0x08
> #define I2CR_MTX 0x10
> @@ -174,6 +184,17 @@ struct imx_i2c_hwdata {
> unsigned i2cr_ien_opcode;
> };
>
> +struct imx_i2c_dma {
> + struct dma_chan *chan_tx;
> + struct dma_chan *chan_rx;
> + struct dma_chan *chan_using;
> + struct completion cmd_complete;
> + dma_addr_t dma_buf;
> + unsigned int dma_len;
> + unsigned int dma_transfer_dir;
> + unsigned int dma_data_dir;
> +};
> +
> struct imx_i2c_struct {
> struct i2c_adapter adapter;
> struct clk *clk;
> @@ -184,6 +205,8 @@ struct imx_i2c_struct {
> int stopped;
> unsigned int ifdr; /* IMX_I2C_IFDR */
> const struct imx_i2c_hwdata *hwdata;
> +
> + struct imx_i2c_dma *dma;
> };
>
> static const struct imx_i2c_hwdata imx1_i2c_hwdata = {
> @@ -254,6 +277,132 @@ static inline unsigned char imx_i2c_read_reg(struct imx_i2c_struct *i2c_imx,
> return readb(i2c_imx->base + (reg<< i2c_imx->hwdata->regshift));
> }
>
> +/* Functions for DMA support */
> +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> + dma_addr_t phy_addr)
> +{
> + struct imx_i2c_dma *dma;
> + struct dma_slave_config dma_sconfig;
> + struct device *dev =&i2c_imx->adapter.dev;
> + int ret;
> +
> + dma = devm_kzalloc(dev, sizeof(struct imx_i2c_dma), GFP_KERNEL);
> + if (!dma) {
> + dev_info(dev, "can't allocate DMA struct\n");
> + return -ENOMEM;
> + }
> +
> + dma->chan_tx = dma_request_slave_channel(dev, "tx");
> + return 0;
?? Looks to be some leftover?
> + if (!dma->chan_tx) {
> + dev_info(dev, "DMA tx channel request failed\n");
> + ret = -ENODEV;
> + goto fail_al;
> + }
> +
> + dma_sconfig.dst_addr = phy_addr +
> + (IMX_I2C_I2DR<< i2c_imx->hwdata->regshift);
> + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + dma_sconfig.dst_maxburst = 1;
> + dma_sconfig.direction = DMA_MEM_TO_DEV;
> + ret = dmaengine_slave_config(dma->chan_tx,&dma_sconfig);
> + if (ret< 0) {
> + dev_info(dev, "DMA slave config failed, err = %d\n", ret);
> + goto fail_tx;
> + }
> +
> + dma->chan_rx = dma_request_slave_channel(dev, "rx");
> + if (!dma->chan_rx) {
> + dev_info(dev, "DMA rx channel request failed\n");
> + ret = -ENODEV;
> + goto fail_tx;
> + }
> +
> + dma_sconfig.src_addr = phy_addr +
> + (IMX_I2C_I2DR<< i2c_imx->hwdata->regshift);
> + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
> + dma_sconfig.src_maxburst = 1;
> + dma_sconfig.direction = DMA_DEV_TO_MEM;
> + ret = dmaengine_slave_config(dma->chan_rx,&dma_sconfig);
> + if (ret< 0) {
> + dev_info(dev, "DMA slave config failed, err = %d\n", ret);
> + goto fail_rx;
> + }
> +
> + i2c_imx->dma = dma;
> +
> + init_completion(&dma->cmd_complete);
> +
> + return 0;
> +
> +fail_rx:
> + dma_release_channel(dma->chan_rx);
> +fail_tx:
> + dma_release_channel(dma->chan_tx);
> +fail_al:
> + devm_kfree(dev, dma);
> +
> + return ret;
> +}
> +
> +static void i2c_imx_dma_callback(void *arg)
> +{
> + struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> +
> + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
> + dma->dma_len, dma->dma_data_dir);
> + complete(&dma->cmd_complete);
> +}
> +
> +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
> + struct i2c_msg *msgs)
> +{
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> + struct dma_async_tx_descriptor *txdesc;
> + struct device *dev =&i2c_imx->adapter.dev;
> +
> + dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf,
> + dma->dma_len, dma->dma_data_dir);
> + if (dma_mapping_error(dma->chan_using->device->dev, dma->dma_buf)) {
> + dev_err(dev, "DMA mapping failed\n");
> + return -EINVAL;
> + }
> +
> + txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf,
> + dma->dma_len, dma->dma_transfer_dir,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!txdesc) {
> + dev_err(dev, "Not able to get desc for DMA xfer\n");
> + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
> + dma->dma_len, dma->dma_data_dir);
> + return -EINVAL;
> + }
> +
> + txdesc->callback = i2c_imx_dma_callback;
> + txdesc->callback_param = i2c_imx;
> + dmaengine_submit(txdesc);
> + dma_async_issue_pending(dma->chan_using);
> +
> + return 0;
> +}
> +
> +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
> +{
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> +
> + dma->dma_buf = 0;
> + dma->dma_len = 0;
> +
> + dma_release_channel(dma->chan_tx);
> + dma->chan_tx = NULL;
> +
> + dma_release_channel(dma->chan_rx);
> + dma->chan_rx = NULL;
> +
> + dma->chan_using = NULL;
> +}
> +
> /** Functions for IMX I2C adapter driver ***************************************
> *******************************************************************************/
>
> @@ -334,6 +483,11 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx)
>
> temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp&= ~I2CR_DMAEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> return result;
> }
>
> @@ -427,44 +581,101 @@ static irqreturn_t i2c_imx_isr(int irq, void *dev_id)
>
> static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> {
> - int i, result;
> + int i, result, timeout = IMX_I2C_DMA_TIMEOUT;
> + unsigned int temp = 0;
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> + struct device *dev =&i2c_imx->adapter.dev;
>
> - dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address: addr=0x%x\n",
> + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
> __func__, msgs->addr<< 1);
> + if (dma&& msgs->len>= IMX_I2C_DMA_THRESHOLD) {
> + reinit_completion(&i2c_imx->dma->cmd_complete);
> + dma->chan_using = dma->chan_tx;
> + dma->dma_transfer_dir = DMA_MEM_TO_DEV;
> + dma->dma_data_dir = DMA_TO_DEVICE;
> + dma->dma_len = msgs->len - 1;
> + result = i2c_imx_dma_xfer(i2c_imx, msgs);
> + if (result)
> + return result;
>
> - /* write slave address */
> - imx_i2c_write_reg(msgs->addr<< 1, i2c_imx, IMX_I2C_I2DR);
> - result = i2c_imx_trx_complete(i2c_imx);
> - if (result)
> - return result;
> - result = i2c_imx_acked(i2c_imx);
> - if (result)
> - return result;
> - dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp |= I2CR_DMAEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>
> - /* write data */
> - for (i = 0; i< msgs->len; i++) {
> - dev_dbg(&i2c_imx->adapter.dev,
> - "<%s> write byte: B%d=0x%X\n",
> - __func__, i, msgs->buf[i]);
> - imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> + /* write slave address */
> + imx_i2c_write_reg(msgs->addr<< 1, i2c_imx, IMX_I2C_I2DR);
> + result = wait_for_completion_interruptible_timeout(
> + &i2c_imx->dma->cmd_complete,
> + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> + if (result<= 0) {
> + dmaengine_terminate_all(dma->chan_using);
> + if (result)
> + return result;
> + else
> + return -ETIMEDOUT;
> + }
> +
> + /* waiting for Transfer complete. */
> + while (timeout--) {
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if (temp& I2SR_ICF)
> + break;
> + udelay(10);
> + }
> +
> + if (!timeout)
> + return -ETIMEDOUT;
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp&= ~I2CR_DMAEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + /* write the last byte */
> + imx_i2c_write_reg(msgs->buf[msgs->len-1],
> + i2c_imx, IMX_I2C_I2DR);
> result = i2c_imx_trx_complete(i2c_imx);
> if (result)
> return result;
> +
> + result = i2c_imx_acked(i2c_imx);
> + if (result)
> + return result;
> + } else {
> + /* write slave address */
> + imx_i2c_write_reg(msgs->addr<< 1, i2c_imx, IMX_I2C_I2DR);
> + result = i2c_imx_trx_complete(i2c_imx);
> + if (result)
> + return result;
> +
> result = i2c_imx_acked(i2c_imx);
> if (result)
> return result;
> +
> + dev_dbg(dev, "<%s> write data\n", __func__);
> +
> + /* write data */
> + for (i = 0; i< msgs->len; i++) {
> + dev_dbg(dev, "<%s> write byte: B%d=0x%X\n",
> + __func__, i, msgs->buf[i]);
> + imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
> + result = i2c_imx_trx_complete(i2c_imx);
> + if (result)
> + return result;
> + result = i2c_imx_acked(i2c_imx);
> + if (result)
> + return result;
> + }
> }
> return 0;
> }
>
> static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> {
> - int i, result;
> + int i, result, timeout = IMX_I2C_DMA_TIMEOUT;
> unsigned int temp;
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> + struct device *dev =&i2c_imx->adapter.dev;
>
> - dev_dbg(&i2c_imx->adapter.dev,
> - "<%s> write slave address: addr=0x%x\n",
> + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
> __func__, (msgs->addr<< 1) | 0x01);
>
> /* write slave address */
> @@ -476,7 +687,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> if (result)
> return result;
>
> - dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
> + dev_dbg(dev, "<%s> setup bus\n", __func__);
>
> /* setup bus to read data */
> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> @@ -486,35 +697,81 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs)
> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
>
> - dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
> + dev_dbg(dev, "<%s> read data\n", __func__);
>
> - /* read data */
> - for (i = 0; i< msgs->len; i++) {
> - result = i2c_imx_trx_complete(i2c_imx);
> + if (dma&& msgs->len>= IMX_I2C_DMA_THRESHOLD) {
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp |= I2CR_DMAEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + reinit_completion(&i2c_imx->dma->cmd_complete);
> + dma->chan_using = dma->chan_rx;
> + dma->dma_transfer_dir = DMA_DEV_TO_MEM;
> + dma->dma_data_dir = DMA_FROM_DEVICE;
> + dma->dma_len = msgs->len - 2;
> + result = i2c_imx_dma_xfer(i2c_imx, msgs);
> if (result)
> return result;
> - if (i == (msgs->len - 1)) {
> - /* It must generate STOP before read I2DR to prevent
> - controller from generating another clock cycle */
> - dev_dbg(&i2c_imx->adapter.dev,
> - "<%s> clear MSTA\n", __func__);
> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> - temp&= ~(I2CR_MSTA | I2CR_MTX);
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> - i2c_imx_bus_busy(i2c_imx, 0);
> - i2c_imx->stopped = 1;
> - } else if (i == (msgs->len - 2)) {
> - dev_dbg(&i2c_imx->adapter.dev,
> - "<%s> set TXAK\n", __func__);
> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> - temp |= I2CR_TXAK;
> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + result = wait_for_completion_interruptible_timeout(
> + &i2c_imx->dma->cmd_complete,
> + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
> + if (result<= 0) {
> + dmaengine_terminate_all(dma->chan_using);
> + if (result)
> + return result;
> + else
> + return -ETIMEDOUT;
> }
> - msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> - dev_dbg(&i2c_imx->adapter.dev,
> - "<%s> read byte: B%d=0x%X\n",
> - __func__, i, msgs->buf[i]);
> +
> + /* waiting for Transfer complete. */
> + while (timeout--) {
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
> + if (temp& I2SR_ICF)
> + break;
> + udelay(10);
> + }
> +
> + if(!timeout)
> + return -ETIMEDOUT;
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp&= ~I2CR_DMAEN;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> + } else {
> + /* read data */
> + for (i = 0; i< msgs->len - 2; i++) {
> + result = i2c_imx_trx_complete(i2c_imx);
> + if (result)
> + return result;
> + msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + dev_dbg(dev, "<%s> read byte: B%d=0x%X\n",
> + __func__, i, msgs->buf[i]);
> + }
> + result = i2c_imx_trx_complete(i2c_imx);
> }
> +
> + /* read n-1 byte data */
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp |= I2CR_TXAK;
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> +
> + msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> + /* read n byte data */
> + result = i2c_imx_trx_complete(i2c_imx);
> + if (result)
> + return result;
> +
> + /*
> + * It must generate STOP before read I2DR to prevent
> + * controller from generating another clock cycle
> + */
> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
> + temp&= ~(I2CR_MSTA | I2CR_MTX);
> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
> + i2c_imx_bus_busy(i2c_imx, 0);
> + i2c_imx->stopped = 1;
> + msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
> +
> return 0;
> }
>
> @@ -601,6 +858,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> void __iomem *base;
> int irq, ret;
> u32 bitrate;
> + u32 phy_addr;
>
> dev_dbg(&pdev->dev, "<%s>\n", __func__);
>
> @@ -611,6 +869,7 @@ static int i2c_imx_probe(struct platform_device *pdev)
> }
>
> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + phy_addr = res->start;
> base = devm_ioremap_resource(&pdev->dev, res);
> if (IS_ERR(base))
> return PTR_ERR(base);
> @@ -696,6 +955,10 @@ static int i2c_imx_probe(struct platform_device *pdev)
> i2c_imx->adapter.name);
> dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>
> + /* Init DMA config if support*/
> + if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr))
> + dev_warn(&pdev->dev, "can't request DMA\n");
> +
> return 0; /* Return OK */
> }
>
> @@ -707,6 +970,9 @@ static int i2c_imx_remove(struct platform_device *pdev)
> dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
> i2c_del_adapter(&i2c_imx->adapter);
>
> + if (i2c_imx->dma)
> + i2c_imx_dma_free(i2c_imx);
> +
> /* setup chip registers to defaults */
> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);

2014-04-04 10:05:17

by Sourav Poddar

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver


>> +
>> + dma->chan_tx = dma_request_slave_channel(dev, "tx");
>> + return 0;

To be more clear,
return here looks to be some leftover.

> ?? Looks to be some leftover?
>> + if (!dma->chan_tx) {
>> + dev_info(dev, "DMA tx channel request failed\n");
>> + ret = -ENODEV;
>> + goto fail_al;
>> + }
>> +
>> + dma_sconfig.dst_addr = phy_addr +
>> + (IMX_I2C_I2DR<< i2c_imx->hwdata->regshift);
>> + dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> + dma_sconfig.dst_maxburst = 1;
>> + dma_sconfig.direction = DMA_MEM_TO_DEV;
>> + ret = dmaengine_slave_config(dma->chan_tx,&dma_sconfig);
>> + if (ret< 0) {
>> + dev_info(dev, "DMA slave config failed, err = %d\n", ret);
>> + goto fail_tx;
>> + }
>> +
>> + dma->chan_rx = dma_request_slave_channel(dev, "rx");
>> + if (!dma->chan_rx) {
>> + dev_info(dev, "DMA rx channel request failed\n");
>> + ret = -ENODEV;
>> + goto fail_tx;
>> + }
>> +
>> + dma_sconfig.src_addr = phy_addr +
>> + (IMX_I2C_I2DR<< i2c_imx->hwdata->regshift);
>> + dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
>> + dma_sconfig.src_maxburst = 1;
>> + dma_sconfig.direction = DMA_DEV_TO_MEM;
>> + ret = dmaengine_slave_config(dma->chan_rx,&dma_sconfig);
>> + if (ret< 0) {
>> + dev_info(dev, "DMA slave config failed, err = %d\n", ret);
>> + goto fail_rx;
>> + }
>> +
>> + i2c_imx->dma = dma;
>> +
>> + init_completion(&dma->cmd_complete);
>> +
>> + return 0;
>> +
>> +fail_rx:
>> + dma_release_channel(dma->chan_rx);
>> +fail_tx:
>> + dma_release_channel(dma->chan_tx);
>> +fail_al:
>> + devm_kfree(dev, dma);
>> +
>> + return ret;
>> +}
>> +
>> +static void i2c_imx_dma_callback(void *arg)
>> +{
>> + struct imx_i2c_struct *i2c_imx = (struct imx_i2c_struct *)arg;
>> + struct imx_i2c_dma *dma = i2c_imx->dma;
>> +
>> + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
>> + dma->dma_len, dma->dma_data_dir);
>> + complete(&dma->cmd_complete);
>> +}
>> +
>> +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
>> + struct i2c_msg *msgs)
>> +{
>> + struct imx_i2c_dma *dma = i2c_imx->dma;
>> + struct dma_async_tx_descriptor *txdesc;
>> + struct device *dev =&i2c_imx->adapter.dev;
>> +
>> + dma->dma_buf = dma_map_single(dma->chan_using->device->dev,
>> msgs->buf,
>> + dma->dma_len, dma->dma_data_dir);
>> + if (dma_mapping_error(dma->chan_using->device->dev,
>> dma->dma_buf)) {
>> + dev_err(dev, "DMA mapping failed\n");
>> + return -EINVAL;
>> + }
>> +
>> + txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf,
>> + dma->dma_len, dma->dma_transfer_dir,
>> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>> + if (!txdesc) {
>> + dev_err(dev, "Not able to get desc for DMA xfer\n");
>> + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
>> + dma->dma_len, dma->dma_data_dir);
>> + return -EINVAL;
>> + }
>> +
>> + txdesc->callback = i2c_imx_dma_callback;
>> + txdesc->callback_param = i2c_imx;
>> + dmaengine_submit(txdesc);
>> + dma_async_issue_pending(dma->chan_using);
>> +
>> + return 0;
>> +}
>> +
>> +static void i2c_imx_dma_free(struct imx_i2c_struct *i2c_imx)
>> +{
>> + struct imx_i2c_dma *dma = i2c_imx->dma;
>> +
>> + dma->dma_buf = 0;
>> + dma->dma_len = 0;
>> +
>> + dma_release_channel(dma->chan_tx);
>> + dma->chan_tx = NULL;
>> +
>> + dma_release_channel(dma->chan_rx);
>> + dma->chan_rx = NULL;
>> +
>> + dma->chan_using = NULL;
>> +}
>> +
>> /** Functions for IMX I2C adapter driver
>> ***************************************
>>
>> *******************************************************************************/
>>
>> @@ -334,6 +483,11 @@ static int i2c_imx_start(struct imx_i2c_struct
>> *i2c_imx)
>>
>> temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK;
>> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp&= ~I2CR_DMAEN;
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +
>> return result;
>> }
>>
>> @@ -427,44 +581,101 @@ static irqreturn_t i2c_imx_isr(int irq, void
>> *dev_id)
>>
>> static int i2c_imx_write(struct imx_i2c_struct *i2c_imx, struct
>> i2c_msg *msgs)
>> {
>> - int i, result;
>> + int i, result, timeout = IMX_I2C_DMA_TIMEOUT;
>> + unsigned int temp = 0;
>> + struct imx_i2c_dma *dma = i2c_imx->dma;
>> + struct device *dev =&i2c_imx->adapter.dev;
>>
>> - dev_dbg(&i2c_imx->adapter.dev, "<%s> write slave address:
>> addr=0x%x\n",
>> + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
>> __func__, msgs->addr<< 1);
>> + if (dma&& msgs->len>= IMX_I2C_DMA_THRESHOLD) {
>> + reinit_completion(&i2c_imx->dma->cmd_complete);
>> + dma->chan_using = dma->chan_tx;
>> + dma->dma_transfer_dir = DMA_MEM_TO_DEV;
>> + dma->dma_data_dir = DMA_TO_DEVICE;
>> + dma->dma_len = msgs->len - 1;
>> + result = i2c_imx_dma_xfer(i2c_imx, msgs);
>> + if (result)
>> + return result;
>>
>> - /* write slave address */
>> - imx_i2c_write_reg(msgs->addr<< 1, i2c_imx, IMX_I2C_I2DR);
>> - result = i2c_imx_trx_complete(i2c_imx);
>> - if (result)
>> - return result;
>> - result = i2c_imx_acked(i2c_imx);
>> - if (result)
>> - return result;
>> - dev_dbg(&i2c_imx->adapter.dev, "<%s> write data\n", __func__);
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp |= I2CR_DMAEN;
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>>
>> - /* write data */
>> - for (i = 0; i< msgs->len; i++) {
>> - dev_dbg(&i2c_imx->adapter.dev,
>> - "<%s> write byte: B%d=0x%X\n",
>> - __func__, i, msgs->buf[i]);
>> - imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
>> + /* write slave address */
>> + imx_i2c_write_reg(msgs->addr<< 1, i2c_imx, IMX_I2C_I2DR);
>> + result = wait_for_completion_interruptible_timeout(
>> + &i2c_imx->dma->cmd_complete,
>> + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
>> + if (result<= 0) {
>> + dmaengine_terminate_all(dma->chan_using);
>> + if (result)
>> + return result;
>> + else
>> + return -ETIMEDOUT;
>> + }
>> +
>> + /* waiting for Transfer complete. */
>> + while (timeout--) {
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>> + if (temp& I2SR_ICF)
>> + break;
>> + udelay(10);
>> + }
>> +
>> + if (!timeout)
>> + return -ETIMEDOUT;
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp&= ~I2CR_DMAEN;
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +
>> + /* write the last byte */
>> + imx_i2c_write_reg(msgs->buf[msgs->len-1],
>> + i2c_imx, IMX_I2C_I2DR);
>> result = i2c_imx_trx_complete(i2c_imx);
>> if (result)
>> return result;
>> +
>> + result = i2c_imx_acked(i2c_imx);
>> + if (result)
>> + return result;
>> + } else {
>> + /* write slave address */
>> + imx_i2c_write_reg(msgs->addr<< 1, i2c_imx, IMX_I2C_I2DR);
>> + result = i2c_imx_trx_complete(i2c_imx);
>> + if (result)
>> + return result;
>> +
>> result = i2c_imx_acked(i2c_imx);
>> if (result)
>> return result;
>> +
>> + dev_dbg(dev, "<%s> write data\n", __func__);
>> +
>> + /* write data */
>> + for (i = 0; i< msgs->len; i++) {
>> + dev_dbg(dev, "<%s> write byte: B%d=0x%X\n",
>> + __func__, i, msgs->buf[i]);
>> + imx_i2c_write_reg(msgs->buf[i], i2c_imx, IMX_I2C_I2DR);
>> + result = i2c_imx_trx_complete(i2c_imx);
>> + if (result)
>> + return result;
>> + result = i2c_imx_acked(i2c_imx);
>> + if (result)
>> + return result;
>> + }
>> }
>> return 0;
>> }
>>
>> static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct
>> i2c_msg *msgs)
>> {
>> - int i, result;
>> + int i, result, timeout = IMX_I2C_DMA_TIMEOUT;
>> unsigned int temp;
>> + struct imx_i2c_dma *dma = i2c_imx->dma;
>> + struct device *dev =&i2c_imx->adapter.dev;
>>
>> - dev_dbg(&i2c_imx->adapter.dev,
>> - "<%s> write slave address: addr=0x%x\n",
>> + dev_dbg(dev, "<%s> write slave address: addr=0x%x\n",
>> __func__, (msgs->addr<< 1) | 0x01);
>>
>> /* write slave address */
>> @@ -476,7 +687,7 @@ static int i2c_imx_read(struct imx_i2c_struct
>> *i2c_imx, struct i2c_msg *msgs)
>> if (result)
>> return result;
>>
>> - dev_dbg(&i2c_imx->adapter.dev, "<%s> setup bus\n", __func__);
>> + dev_dbg(dev, "<%s> setup bus\n", __func__);
>>
>> /* setup bus to read data */
>> temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> @@ -486,35 +697,81 @@ static int i2c_imx_read(struct imx_i2c_struct
>> *i2c_imx, struct i2c_msg *msgs)
>> imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR); /* dummy read */
>>
>> - dev_dbg(&i2c_imx->adapter.dev, "<%s> read data\n", __func__);
>> + dev_dbg(dev, "<%s> read data\n", __func__);
>>
>> - /* read data */
>> - for (i = 0; i< msgs->len; i++) {
>> - result = i2c_imx_trx_complete(i2c_imx);
>> + if (dma&& msgs->len>= IMX_I2C_DMA_THRESHOLD) {
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp |= I2CR_DMAEN;
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +
>> + reinit_completion(&i2c_imx->dma->cmd_complete);
>> + dma->chan_using = dma->chan_rx;
>> + dma->dma_transfer_dir = DMA_DEV_TO_MEM;
>> + dma->dma_data_dir = DMA_FROM_DEVICE;
>> + dma->dma_len = msgs->len - 2;
>> + result = i2c_imx_dma_xfer(i2c_imx, msgs);
>> if (result)
>> return result;
>> - if (i == (msgs->len - 1)) {
>> - /* It must generate STOP before read I2DR to prevent
>> - controller from generating another clock cycle */
>> - dev_dbg(&i2c_imx->adapter.dev,
>> - "<%s> clear MSTA\n", __func__);
>> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> - temp&= ~(I2CR_MSTA | I2CR_MTX);
>> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> - i2c_imx_bus_busy(i2c_imx, 0);
>> - i2c_imx->stopped = 1;
>> - } else if (i == (msgs->len - 2)) {
>> - dev_dbg(&i2c_imx->adapter.dev,
>> - "<%s> set TXAK\n", __func__);
>> - temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> - temp |= I2CR_TXAK;
>> - imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +
>> + result = wait_for_completion_interruptible_timeout(
>> + &i2c_imx->dma->cmd_complete,
>> + msecs_to_jiffies(IMX_I2C_DMA_TIMEOUT));
>> + if (result<= 0) {
>> + dmaengine_terminate_all(dma->chan_using);
>> + if (result)
>> + return result;
>> + else
>> + return -ETIMEDOUT;
>> }
>> - msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>> - dev_dbg(&i2c_imx->adapter.dev,
>> - "<%s> read byte: B%d=0x%X\n",
>> - __func__, i, msgs->buf[i]);
>> +
>> + /* waiting for Transfer complete. */
>> + while (timeout--) {
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2SR);
>> + if (temp& I2SR_ICF)
>> + break;
>> + udelay(10);
>> + }
>> +
>> + if(!timeout)
>> + return -ETIMEDOUT;
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp&= ~I2CR_DMAEN;
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> + } else {
>> + /* read data */
>> + for (i = 0; i< msgs->len - 2; i++) {
>> + result = i2c_imx_trx_complete(i2c_imx);
>> + if (result)
>> + return result;
>> + msgs->buf[i] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>> + dev_dbg(dev, "<%s> read byte: B%d=0x%X\n",
>> + __func__, i, msgs->buf[i]);
>> + }
>> + result = i2c_imx_trx_complete(i2c_imx);
>> }
>> +
>> + /* read n-1 byte data */
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp |= I2CR_TXAK;
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> +
>> + msgs->buf[msgs->len-2] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>> + /* read n byte data */
>> + result = i2c_imx_trx_complete(i2c_imx);
>> + if (result)
>> + return result;
>> +
>> + /*
>> + * It must generate STOP before read I2DR to prevent
>> + * controller from generating another clock cycle
>> + */
>> + temp = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2CR);
>> + temp&= ~(I2CR_MSTA | I2CR_MTX);
>> + imx_i2c_write_reg(temp, i2c_imx, IMX_I2C_I2CR);
>> + i2c_imx_bus_busy(i2c_imx, 0);
>> + i2c_imx->stopped = 1;
>> + msgs->buf[msgs->len-1] = imx_i2c_read_reg(i2c_imx, IMX_I2C_I2DR);
>> +
>> return 0;
>> }
>>
>> @@ -601,6 +858,7 @@ static int i2c_imx_probe(struct platform_device
>> *pdev)
>> void __iomem *base;
>> int irq, ret;
>> u32 bitrate;
>> + u32 phy_addr;
>>
>> dev_dbg(&pdev->dev, "<%s>\n", __func__);
>>
>> @@ -611,6 +869,7 @@ static int i2c_imx_probe(struct platform_device
>> *pdev)
>> }
>>
>> res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + phy_addr = res->start;
>> base = devm_ioremap_resource(&pdev->dev, res);
>> if (IS_ERR(base))
>> return PTR_ERR(base);
>> @@ -696,6 +955,10 @@ static int i2c_imx_probe(struct platform_device
>> *pdev)
>> i2c_imx->adapter.name);
>> dev_info(&i2c_imx->adapter.dev, "IMX I2C adapter registered\n");
>>
>> + /* Init DMA config if support*/
>> + if (i2c_imx_dma_request(i2c_imx, (dma_addr_t)phy_addr))
>> + dev_warn(&pdev->dev, "can't request DMA\n");
>> +
>> return 0; /* Return OK */
>> }
>>
>> @@ -707,6 +970,9 @@ static int i2c_imx_remove(struct platform_device
>> *pdev)
>> dev_dbg(&i2c_imx->adapter.dev, "adapter removed\n");
>> i2c_del_adapter(&i2c_imx->adapter);
>>
>> + if (i2c_imx->dma)
>> + i2c_imx_dma_free(i2c_imx);
>> +
>> /* setup chip registers to defaults */
>> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IADR);
>> imx_i2c_write_reg(0, i2c_imx, IMX_I2C_IFDR);
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

2014-04-04 12:43:02

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

On Friday, April 04, 2014 at 11:49:39 AM, sourav wrote:

[...]

> > +/* Functions for DMA support */
> > +static int i2c_imx_dma_request(struct imx_i2c_struct *i2c_imx,
> > + dma_addr_t phy_addr)
> > +{
> > + struct imx_i2c_dma *dma;
> > + struct dma_slave_config dma_sconfig;
> > + struct device *dev =&i2c_imx->adapter.dev;
> > + int ret;
> > +
> > + dma = devm_kzalloc(dev, sizeof(struct imx_i2c_dma), GFP_KERNEL);
> > + if (!dma) {
> > + dev_info(dev, "can't allocate DMA struct\n");
> > + return -ENOMEM;
> > + }
> > +
> > + dma->chan_tx = dma_request_slave_channel(dev, "tx");
> > + return 0;
>
> ?? Looks to be some leftover?

Nice find.

btw. you might want to trim the email only to the relevant parts when replying
so it's easier to find your commments in the entire body of text.

> > + if (!dma->chan_tx) {
> > + dev_info(dev, "DMA tx channel request failed\n");
> > + ret = -ENODEV;
> > + goto fail_al;
> > + }

[...]

Best regards,
Marek Vasut

2014-04-04 14:28:59

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

On Friday, April 04, 2014 at 04:41:11 AM, Yuan Yao wrote:
> Add dma support for i2c. This function depend on DMA driver.
> You can turn on it by write both the dmas and dma-name properties in dts
> node.
>
> Signed-off-by: Yuan Yao <[email protected]>

[...]

Since you will be fixing that superfluous return 0; (I actually wonder, did you
really test the driver at all before submitting it?) ...

> +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
> + struct i2c_msg *msgs)
> +{
> + struct imx_i2c_dma *dma = i2c_imx->dma;
> + struct dma_async_tx_descriptor *txdesc;
> + struct device *dev = &i2c_imx->adapter.dev;
> +
> + dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf,

Please fix this "noodle" here too, the chain of pointers is quite long, you can
just define a variable for that.

> + dma->dma_len, dma->dma_data_dir);
> + if (dma_mapping_error(dma->chan_using->device->dev, dma->dma_buf)) {
> + dev_err(dev, "DMA mapping failed\n");
> + return -EINVAL;
> + }
> +
> + txdesc = dmaengine_prep_slave_single(dma->chan_using, dma->dma_buf,
> + dma->dma_len, dma->dma_transfer_dir,
> + DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> + if (!txdesc) {
> + dev_err(dev, "Not able to get desc for DMA xfer\n");
> + dma_unmap_single(dma->chan_using->device->dev, dma->dma_buf,
> + dma->dma_len, dma->dma_data_dir);
> + return -EINVAL;
> + }
> +
> + txdesc->callback = i2c_imx_dma_callback;
> + txdesc->callback_param = i2c_imx;
> + dmaengine_submit(txdesc);
> + dma_async_issue_pending(dma->chan_using);
> +
> + return 0;
> +}
[...]

Other than those two things,

Reviewed-by: Marek Vasut <[email protected]>

2014-04-04 14:40:51

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

On Fri, Apr 04, 2014 at 04:28:55PM +0200, Marek Vasut wrote:
> On Friday, April 04, 2014 at 04:41:11 AM, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in dts
> > node.
> >
> > Signed-off-by: Yuan Yao <[email protected]>
>
> [...]
>
> Since you will be fixing that superfluous return 0; (I actually wonder, did you
> really test the driver at all before submitting it?) ...

Good question. @Yuan: Next time, please state what tests you did with
the new version of the driver.


Attachments:
(No filename) (590.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-04-04 15:45:34

by Yuan Yao

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

On Friday, April 04, 2014 at 22:29:32 PM, Marek Vasut wrote:
> On Friday, April 04, 2014 at 04:41:11 AM, Yuan Yao wrote:
> > Add dma support for i2c. This function depend on DMA driver.
> > You can turn on it by write both the dmas and dma-name properties in dts
> > node.
> >
> > Signed-off-by: Yuan Yao <[email protected]>

[...]

> Since you will be fixing that superfluous return 0; (I actually wonder, did you
> really test the driver at all before submitting it?) ...

I'm sorry for the leftover. It's a serious bug. The support by DMA will be disabled always.
Thanks for your consideration. But subjective assume may not suitable. Testing is a essential link .
But the bug can't be found by normal test . The testing may not be imperfect, it's my
serious fault, but please don't doubt my work attitude.
The ugly is my work or even the ability but not the attitude, I think.

At last, Thank you for giving me a lot of help, it's very important for me.

> > +static int i2c_imx_dma_xfer(struct imx_i2c_struct *i2c_imx,
> > + struct i2c_msg *msgs)
> > +{
> > + struct imx_i2c_dma *dma = i2c_imx->dma;
> > + struct dma_async_tx_descriptor *txdesc;
> > + struct device *dev = &i2c_imx->adapter.dev;
> +
> + dma->dma_buf = dma_map_single(dma->chan_using->device->dev, msgs->buf,
[...]

> Reviewed-by: Marek Vasut <[email protected]>

2014-04-04 17:40:21

by Wolfram Sang

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

> The ugly is my work or even the ability but not the attitude, I think.

Explaining which tests were done is what I wanted to request more often
from submitters anyhow. Your leftover (yes, this can happen) just
reminded me to actually do that :)


Attachments:
(No filename) (248.00 B)
signature.asc (836.00 B)
Digital signature
Download all attachments

2014-07-24 03:19:35

by Yuan Yao

[permalink] [raw]
Subject: RE: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

On Fri, May 21, 2014 at 4:01 PM +0200, Wolfram Sang wrote:
>
> Ping. Any updates? I think this was pretty close to inclusion?

Hi, Wolfram
Thanks for your concern. Sorry for my reply so late. I had on a business trip for months.
At that time I have no device to debug it. Now, I'm come back and I will try my best to finish it.
I have sent the patch for V5. Thanks for your review.

Best regards,
Yuan Yao

2014-07-24 04:14:15

by Marek Vasut

[permalink] [raw]
Subject: Re: [PATCH v4 1/2] i2c: add DMA support for freescale i2c driver

On Thursday, July 24, 2014 at 05:19:27 AM, Yao Yuan wrote:
> On Fri, May 21, 2014 at 4:01 PM +0200, Wolfram Sang wrote:
> > Ping. Any updates? I think this was pretty close to inclusion?
>
> Hi, Wolfram
> Thanks for your concern. Sorry for my reply so late. I had on a business
> trip for months. At that time I have no device to debug it. Now, I'm come
> back and I will try my best to finish it. I have sent the patch for V5.
> Thanks for your review.

So you have no means to test this ? Can this be tested on MX5 ? MX6 ?

Best regards,
Marek Vasut