Skip to contentSkip to conversation

i2c-designware patches

RFC New Shinya Kuribayashi 6 participants 31 messages Oct 13, 2009 – Oct 19, 2009

Subsystems (2)
MIPS I2C subsystem host drivers
lore
Share & tools
lore

Shinya Kuribayashi

author
#0 lore raw

Hi Baruch and lists,

Here're various improvements / bug-fixing patches for DW I2C driver. I'm working with v2.6.27-based kernel, but they must work fine with the latest mainline kernel.

It's stil in RFC, and I'm working with it for some time. Any comments or suggestions are highly appreciated. Then I'll respin and give it a test, thanks.

Baruch, I'd say the base driver is in good shape enogh, so I'm having a fun with modifing the driver. Thanks for the initial work.

P.S. ARM and MIPS lists are Cc:ed as I believe there must be potential users of this driver.

Shinya Kuribayashi (16):
i2c-designware: Consolidate to use 32-bit word accesses
i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts
i2c-designware: Use platform_get_irq helper
i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer
i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer
i2c-designware: Remove an useless local variable "num"
i2c-designware: Set a clock name to DesignWare I2C clock source
i2c-designware: Improve _HCNT/_LCNT calculation
i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug
i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler
i2c-designware: Set Tx/Rx FIFO threshold levels
i2c-designware: Divide i2c_dw_xfer_msg into two functions
i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer
i2c-designware: Deferred FIFO-data-counting variables initialization
i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error
i2c-designware: Add I2C_FUNC_SMBUS_* bits

 drivers/i2c/busses/i2c-designware.c |  367 +++++++++++++++++++++++++----------
 1 files changed, 264 insertions(+), 103 deletions(-)
Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
author
#1 lore raw

PATCH 1/16 i2c-designware: Consolidate to use 32-bit word accesses

This driver looks originally meant for armel machines where readw()/ writew() works perfectly fine. But that doens't work for big-endian systems.

This patch converts all 8/16-bit-aware usages to 32-bit variants, so that the driver works for MIPS big-endian machines, too.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   76 +++++++++++++++++-----------------
 1 files changed, 38 insertions(+), 38 deletions(-)
drivers/i2c/busses/i2c-designware.c +38 -38
index b444762..a4f928e 100644
@@ -162,14 +162,14 @@ struct dw_i2c_dev {
 	struct i2c_msg		*msgs;
 	int			msgs_num;
 	int			msg_write_idx;
-	u16			tx_buf_len;
+	u32			tx_buf_len;
 	u8			*tx_buf;
 	int			msg_read_idx;
-	u16			rx_buf_len;
+	u32			rx_buf_len;
 	u8			*rx_buf;
 	int			msg_err;
 	unsigned int		status;
-	u16			abort_source;
+	u32			abort_source;
 	int			irq;
 	struct i2c_adapter	adapter;
 	unsigned int		tx_fifo_depth;

@@ -187,25 +187,25 @@ struct dw_i2c_dev {
 static void i2c_dw_init(struct dw_i2c_dev *dev)
 {
 	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
-	u16 ic_con;
+	u32 ic_con;
 
 	/* Disable the adapter */
-	writeb(0, dev->base + DW_IC_ENABLE);
+	writel(0, dev->base + DW_IC_ENABLE);
 
 	/* set standard and fast speed deviders for high/low periods */
-	writew((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
+	writel((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
 			dev->base + DW_IC_SS_SCL_HCNT);
-	writew((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
+	writel((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
 			dev->base + DW_IC_SS_SCL_LCNT);
-	writew((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
+	writel((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
 			dev->base + DW_IC_FS_SCL_HCNT);
-	writew((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
+	writel((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
 			dev->base + DW_IC_FS_SCL_LCNT);
 
 	/* configure the i2c master */
 	ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;
-	writew(ic_con, dev->base + DW_IC_CON);
+	writel(ic_con, dev->base + DW_IC_CON);
 }
 
 /*

@@ -215,7 +215,7 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 {
 	int timeout = TIMEOUT;
 
-	while (readb(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+	while (readl(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
 		if (timeout <= 0) {
 			dev_warn(dev->dev, "timeout waiting for bus ready\n");
 			return -ETIMEDOUT;

@@ -239,29 +239,29 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
-	u16 ic_con, intr_mask;
-	int tx_limit = dev->tx_fifo_depth - readb(dev->base + DW_IC_TXFLR);
-	int rx_limit = dev->rx_fifo_depth - readb(dev->base + DW_IC_RXFLR);
-	u16 addr = msgs[dev->msg_write_idx].addr;
-	u16 buf_len = dev->tx_buf_len;
+	u32 ic_con, intr_mask;
+	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
+	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
+	u32 addr = msgs[dev->msg_write_idx].addr;
+	u32 buf_len = dev->tx_buf_len;
 
 	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 		/* Disable the adapter */
-		writeb(0, dev->base + DW_IC_ENABLE);
+		writel(0, dev->base + DW_IC_ENABLE);
 
 		/* set the slave (target) address */
-		writew(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
+		writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
 
 		/* if the slave address is ten bit address, enable 10BITADDR */
-		ic_con = readw(dev->base + DW_IC_CON);
+		ic_con = readl(dev->base + DW_IC_CON);
 		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
 			ic_con |= DW_IC_CON_10BITADDR_MASTER;
 		else
 			ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
-		writew(ic_con, dev->base + DW_IC_CON);
+		writel(ic_con, dev->base + DW_IC_CON);
 
 		/* Enable the adapter */
-		writeb(1, dev->base + DW_IC_ENABLE);
+		writel(1, dev->base + DW_IC_ENABLE);
 	}
 
 	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {

@@ -287,10 +287,10 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 
 		while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
-				writew(0x100, dev->base + DW_IC_DATA_CMD);
+				writel(0x100, dev->base + DW_IC_DATA_CMD);
 				rx_limit--;
 			} else
-				writew(*(dev->tx_buf++),
+				writel(*(dev->tx_buf++),
 						dev->base + DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}

@@ -302,7 +302,7 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 		dev->status |= STATUS_WRITE_IN_PROGRESS;
 	} else
 		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
-	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
+	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
 
 	dev->tx_buf_len = buf_len;
 }

@@ -313,11 +313,11 @@ i2c_dw_read(struct i2c_adapter *adap)
 	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
-	u16 addr = msgs[dev->msg_read_idx].addr;
-	int rx_valid = readw(dev->base + DW_IC_RXFLR);
+	u32 addr = msgs[dev->msg_read_idx].addr;
+	int rx_valid = readl(dev->base + DW_IC_RXFLR);
 
 	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
-		u16 len;
+		u32 len;
 		u8 *buf;
 
 		if (!(msgs[dev->msg_read_idx].flags & I2C_M_RD))

@@ -336,7 +336,7 @@ i2c_dw_read(struct i2c_adapter *adap)
 		}
 
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
-			*buf++ = readb(dev->base + DW_IC_DATA_CMD);
+			*buf++ = readl(dev->base + DW_IC_DATA_CMD);
 
 		if (len > 0) {
 			dev->status |= STATUS_READ_IN_PROGRESS;

@@ -398,7 +398,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		do {
 			i2c_dw_read(adap);
 		} while (dev->status & STATUS_READ_IN_PROGRESS);
-		writeb(0, dev->base + DW_IC_ENABLE);
+		writel(0, dev->base + DW_IC_ENABLE);
 		ret = num;
 		goto done;
 	}

@@ -428,7 +428,7 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
 static void dw_i2c_pump_msg(unsigned long data)
 {
 	struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
-	u16 intr_mask;
+	u32 intr_mask;
 
 	i2c_dw_read(&dev->adapter);
 	i2c_dw_xfer_msg(&dev->adapter);

@@ -436,7 +436,7 @@ static void dw_i2c_pump_msg(unsigned long data)
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
 	if (dev->status & STATUS_WRITE_IN_PROGRESS)
 		intr_mask |= DW_IC_INTR_TX_EMPTY;
-	writew(intr_mask, dev->base + DW_IC_INTR_MASK);
+	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
 }
 
 /*

@@ -446,19 +446,19 @@ static void dw_i2c_pump_msg(unsigned long data)
 static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 {
 	struct dw_i2c_dev *dev = dev_id;
-	u16 stat;
+	u32 stat;
 
-	stat = readw(dev->base + DW_IC_INTR_STAT);
+	stat = readl(dev->base + DW_IC_INTR_STAT);
 	dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
 	if (stat & DW_IC_INTR_TX_ABRT) {
-		dev->abort_source = readw(dev->base + DW_IC_TX_ABRT_SOURCE);
+		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
 	} else if (stat & DW_IC_INTR_TX_EMPTY)
 		tasklet_schedule(&dev->pump_msg);
 
-	readb(dev->base + DW_IC_CLR_INTR);	/* clear interrupts */
-	writew(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
+	readl(dev->base + DW_IC_CLR_INTR);	/* clear interrupts */
+	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
 	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
 		complete(&dev->cmd_complete);
 

@@ -531,7 +531,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	}
 	i2c_dw_init(dev);
 
-	writew(0, dev->base + DW_IC_INTR_MASK); /* disable IRQ */
+	writel(0, dev->base + DW_IC_INTR_MASK); /* disable IRQ */
 	r = request_irq(dev->irq, i2c_dw_isr, 0, pdev->name, dev);
 	if (r) {
 		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);

@@ -587,7 +587,7 @@ static int __devexit dw_i2c_remove(struct platform_device *pdev)
 	clk_put(dev->clk);
 	dev->clk = NULL;
 
-	writeb(0, dev->base + DW_IC_ENABLE);
+	writel(0, dev->base + DW_IC_ENABLE);
 	free_irq(dev->irq, dev);
 	kfree(dev);
Signature · 1 line
1.6.5
author
#2 lore raw

PATCH 2/16 i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts

In reply to Shinya Kuribayashi

We're strongly discouraged from using the IC_CLR_INTR register because it clears all software-clearable interrupts asserted at the moment.

stat = read(IC_INTR_STAT);
:
: <=== Interrupts asserted during this period will be lost :
read(IC_CLR_INTR);

Use the separately-prepared IC_CLR_* registers, instead.

At the same time, this patch adds all remaining interrupt definitions available in the DesignWare I2C hardware.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   77 +++++++++++++++++++++++++++++++++--
 1 files changed, 73 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware.c +73 -4
index a4f928e..efddae1 100644
@@ -49,7 +49,18 @@
 #define DW_IC_FS_SCL_LCNT	0x20
 #define DW_IC_INTR_STAT		0x2c
 #define DW_IC_INTR_MASK		0x30
+#define DW_IC_RAW_INTR_STAT	0x34
 #define DW_IC_CLR_INTR		0x40
+#define DW_IC_CLR_RX_UNDER	0x44
+#define DW_IC_CLR_RX_OVER	0x48
+#define DW_IC_CLR_TX_OVER	0x4c
+#define DW_IC_CLR_RD_REQ	0x50
+#define DW_IC_CLR_TX_ABRT	0x54
+#define DW_IC_CLR_RX_DONE	0x58
+#define DW_IC_CLR_ACTIVITY	0x5c
+#define DW_IC_CLR_STOP_DET	0x60
+#define DW_IC_CLR_START_DET	0x64
+#define DW_IC_CLR_GEN_CALL	0x68
 #define DW_IC_ENABLE		0x6c
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74

@@ -64,9 +75,18 @@
 #define DW_IC_CON_RESTART_EN		0x20
 #define DW_IC_CON_SLAVE_DISABLE		0x40
 
-#define DW_IC_INTR_TX_EMPTY	0x10
-#define DW_IC_INTR_TX_ABRT	0x40
+#define DW_IC_INTR_RX_UNDER	0x001
+#define DW_IC_INTR_RX_OVER	0x002
+#define DW_IC_INTR_RX_FULL	0x004
+#define DW_IC_INTR_TX_OVER	0x008
+#define DW_IC_INTR_TX_EMPTY	0x010
+#define DW_IC_INTR_RD_REQ	0x020
+#define DW_IC_INTR_TX_ABRT	0x040
+#define DW_IC_INTR_RX_DONE	0x080
+#define DW_IC_INTR_ACTIVITY	0x100
 #define DW_IC_INTR_STOP_DET	0x200
+#define DW_IC_INTR_START_DET	0x400
+#define DW_IC_INTR_GEN_CALL	0x800
 
 #define DW_IC_STATUS_ACTIVITY	0x1
 

@@ -439,6 +459,55 @@ static void dw_i2c_pump_msg(unsigned long data)
 	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
 }
 
+static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
+{
+	u32 stat;
+
+	/*
+	 * The IC_INTR_STAT register just indicates "enabled" interrupts.
+	 * Ths unmasked raw version of interrupt status bits are available
+	 * in the IC_RAW_INTR_STAT register.
+	 *
+	 * That is,
+	 *   stat = readl(IC_INTR_STAT);
+	 * equals to,
+	 *   stat = readl(IC_RAW_INTR_STAT) & readl(IC_INTR_MASK);
+	 *
+	 * The raw version might be useful for debugging purposes.
+	 */
+	stat = readl(dev->base + DW_IC_INTR_STAT);
+
+	/*
+	 * Do not use the IC_CLR_INTR register to clear interrupts, or
+	 * you'll miss some interrupts, triggered during the period from
+	 * reading IC_INTR_STAT to reading IC_CLR_INTR.
+	 *
+	 * Use the separately-prepared IC_CLR_* registers instead.
+	 */
+	if (stat & DW_IC_INTR_RX_UNDER)
+		readl(dev->base + DW_IC_CLR_RX_UNDER);
+	if (stat & DW_IC_INTR_RX_OVER)
+		readl(dev->base + DW_IC_CLR_RX_OVER);
+	if (stat & DW_IC_INTR_TX_OVER)
+		readl(dev->base + DW_IC_CLR_TX_OVER);
+	if (stat & DW_IC_INTR_RD_REQ)
+		readl(dev->base + DW_IC_CLR_RD_REQ);
+	if (stat & DW_IC_INTR_TX_ABRT)
+		readl(dev->base + DW_IC_CLR_TX_ABRT);
+	if (stat & DW_IC_INTR_RX_DONE)
+		readl(dev->base + DW_IC_CLR_RX_DONE);
+	if (stat & DW_IC_INTR_ACTIVITY)
+		readl(dev->base + DW_IC_CLR_ACTIVITY);
+	if (stat & DW_IC_INTR_STOP_DET)
+		readl(dev->base + DW_IC_CLR_STOP_DET);
+	if (stat & DW_IC_INTR_START_DET)
+		readl(dev->base + DW_IC_CLR_START_DET);
+	if (stat & DW_IC_INTR_GEN_CALL)
+		readl(dev->base + DW_IC_CLR_GEN_CALL);
+
+	return stat;
+}
+
 /*
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.

@@ -448,8 +517,9 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	struct dw_i2c_dev *dev = dev_id;
 	u32 stat;
 
-	stat = readl(dev->base + DW_IC_INTR_STAT);
+	stat = i2c_dw_read_clear_intrbits(dev);
 	dev_dbg(dev->dev, "%s: stat=0x%x\n", __func__, stat);
+
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;

@@ -457,7 +527,6 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 	} else if (stat & DW_IC_INTR_TX_EMPTY)
 		tasklet_schedule(&dev->pump_msg);
 
-	readl(dev->base + DW_IC_CLR_INTR);	/* clear interrupts */
 	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
 	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
 		complete(&dev->cmd_complete);
Signature · 1 line
1.6.5
author
#3 lore raw

PATCH 3/16 i2c-designware: Use platform_get_irq helper

In reply to Shinya Kuribayashi
Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-designware.c +6 -6
index efddae1..0164092 100644
@@ -543,8 +543,8 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 {
 	struct dw_i2c_dev *dev;
 	struct i2c_adapter *adap;
-	struct resource *mem, *irq, *ioarea;
-	int r;
+	struct resource *mem, *ioarea;
+	int irq, r;
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);

@@ -553,10 +553,10 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 		return -EINVAL;
 	}
 
-	irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
-	if (!irq) {
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
 		dev_err(&pdev->dev, "no irq resource?\n");
-		return -EINVAL;
+		return irq; /* -ENXIO */
 	}
 
 	ioarea = request_mem_region(mem->start, resource_size(mem),

@@ -576,7 +576,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	tasklet_init(&dev->pump_msg, dw_i2c_pump_msg, (unsigned long) dev);
 	mutex_init(&dev->lock);
 	dev->dev = get_device(&pdev->dev);
-	dev->irq = irq->start;
+	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
 	dev->clk = clk_get(&pdev->dev, NULL);
Signature · 1 line
1.6.5
author
#4 lore raw

PATCH 4/16 i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer

In reply to Shinya Kuribayashi

There's no need to interface using with "struct i2c_adapter" pointer. Let's use a local "struct dw_i2c_dev" pointer, instead.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware.c +3 -4
index 0164092..c6a35bf 100644
@@ -328,9 +328,8 @@ i2c_dw_xfer_msg(struct i2c_adapter *adap)
 }
 
 static void
-i2c_dw_read(struct i2c_adapter *adap)
+i2c_dw_read(struct dw_i2c_dev *dev)
 {
-	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
 	u32 addr = msgs[dev->msg_read_idx].addr;

@@ -416,7 +415,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	if (likely(!dev->cmd_err)) {
 		/* read rx fifo, and disable the adapter */
 		do {
-			i2c_dw_read(adap);
+			i2c_dw_read(dev);
 		} while (dev->status & STATUS_READ_IN_PROGRESS);
 		writel(0, dev->base + DW_IC_ENABLE);
 		ret = num;

@@ -450,7 +449,7 @@ static void dw_i2c_pump_msg(unsigned long data)
 	struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
 	u32 intr_mask;
 
-	i2c_dw_read(&dev->adapter);
+	i2c_dw_read(dev);
 	i2c_dw_xfer_msg(&dev->adapter);
 
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
Signature · 1 line
1.6.5
author
#5 lore raw

PATCH 5/16 i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer

In reply to Shinya Kuribayashi

There's no need to interface using with "struct i2c_adapter" pointer. Let's use a local "struct dw_i2c_dev" pointer, instead.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |    7 +++----
 1 files changed, 3 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware.c +3 -4
index c6a35bf..205f691 100644
@@ -254,9 +254,8 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
  * that is longer than the size of the TX FIFO.
  */
 static void
-i2c_dw_xfer_msg(struct i2c_adapter *adap)
+i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
-	struct dw_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct i2c_msg *msgs = dev->msgs;
 	int num = dev->msgs_num;
 	u32 ic_con, intr_mask;

@@ -394,7 +393,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto done;
 
 	/* start the transfers */
-	i2c_dw_xfer_msg(adap);
+	i2c_dw_xfer_msg(dev);
 
 	/* wait for tx to complete */
 	ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete, HZ);

@@ -450,7 +449,7 @@ static void dw_i2c_pump_msg(unsigned long data)
 	u32 intr_mask;
 
 	i2c_dw_read(dev);
-	i2c_dw_xfer_msg(&dev->adapter);
+	i2c_dw_xfer_msg(dev);
 
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
 	if (dev->status & STATUS_WRITE_IN_PROGRESS)
Signature · 1 line
1.6.5
author
#6 lore raw

PATCH 6/16 i2c-designware: Remove an useless local variable "num"

In reply to Shinya Kuribayashi

We couldn't know the original intent for this variable, but at this point it's useless.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |    6 ++----
 1 files changed, 2 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware.c +2 -4
index 205f691..9d81775 100644
@@ -257,7 +257,6 @@ static void
 i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	int num = dev->msgs_num;
 	u32 ic_con, intr_mask;
 	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
 	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);

@@ -283,7 +282,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		writel(1, dev->base + DW_IC_ENABLE);
 	}
 
-	for (; dev->msg_write_idx < num; dev->msg_write_idx++) {
+	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		/* if target address has changed, we need to
 		 * reprogram the target address in the i2c
 		 * adapter when we are done with this transfer

@@ -330,11 +329,10 @@ static void
 i2c_dw_read(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	int num = dev->msgs_num;
 	u32 addr = msgs[dev->msg_read_idx].addr;
 	int rx_valid = readl(dev->base + DW_IC_RXFLR);
 
-	for (; dev->msg_read_idx < num; dev->msg_read_idx++) {
+	for (; dev->msg_read_idx < dev->msgs_num; dev->msg_read_idx++) {
 		u32 len;
 		u8 *buf;
Signature · 1 line
1.6.5
author
#7 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

In reply to Shinya Kuribayashi

This driver is originally prepared for the ARM kernel where rich and well-maintained "clkdev" clock framework is available, and clock name might not be strictly required. ARM's clkdev does slightly fuzzy matching where it basically gives preference to "struct device" mathing over "clock id". As long as used for ARM machines, there's no problem.

However, all users of this driver necessarily don't have the same clk framework with ARM's, as the clk I/F implementation varies depending on ARCHs and machines.

This patch adds a clock name so that other users with simple/minimum/ limited clk support could make use of the driver.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-designware.c +1 -1
index 9d81775..0f8bd4c 100644
@@ -575,7 +575,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
-	dev->clk = clk_get(&pdev->dev, NULL);
+	dev->clk = clk_get(&pdev->dev, "dw_i2c_clk");
 	if (IS_ERR(dev->clk)) {
 		r = -ENODEV;
 		goto err_free_mem;
Signature · 1 line
1.6.5
#8 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

Quoted 6 lines · Shinya Kuribayashi

This driver is originally prepared for the ARM kernel where rich and well-maintained "clkdev" clock framework is available, and clock name might not be strictly required. ARM's clkdev does slightly fuzzy matching where it basically gives preference to "struct device" mathing over "clock id". As long as used for ARM machines, there's no problem.

Quoted 6 lines

However, all users of this driver necessarily don't have the same clk framework with ARM's, as the clk I/F implementation varies depending on ARCHs and machines.

This patch adds a clock name so that other users with simple/minimum/ limited clk support could make use of the driver.

This seems like something that it'd be better to fix in the relevant clock frameworks in order to try to keep the API consistently usable between platforms. The clkdev matching library that most of the ARM platforms use should be generic enough to be usable elsewhere, there is regular discussion of moving it somewhere more generic.

author
#9 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

Quoted 11 lines · Mark Brown
Quoted 9 lines · Shinya Kuribayashi

This driver is originally prepared for the ARM kernel where rich and well-maintained "clkdev" clock framework is available, and clock name might not be strictly required. ARM's clkdev does slightly fuzzy matching where it basically gives preference to "struct device" mathing over "clock id". As long as used for ARM machines, there's no problem.

However, all users of this driver necessarily don't have the same clk framework with ARM's, as the clk I/F implementation varies depending on ARCHs and machines.

Quoted 8 lines
Quoted 2 lines

This patch adds a clock name so that other users with simple/minimum/ limited clk support could make use of the driver.

This seems like something that it'd be better to fix in the relevant clock frameworks in order to try to keep the API consistently usable between platforms. The clkdev matching library that most of the ARM platforms use should be generic enough to be usable elsewhere, there is regular discussion of moving it somewhere more generic.

Before fixing the driver, I've checked various discussion archives on clock framework, git log histories, and source codes of almost all ARM common/mach/plat and SH variants, so I think I know enough backgrounds on this.

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
#10 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

In reply to Shinya Kuribayashi
Quoted 6 lines · Shinya Kuribayashi

This driver is originally prepared for the ARM kernel where rich and well-maintained "clkdev" clock framework is available, and clock name might not be strictly required. ARM's clkdev does slightly fuzzy matching where it basically gives preference to "struct device" mathing over "clock id". As long as used for ARM machines, there's no problem.

Quoted 6 lines

However, all users of this driver necessarily don't have the same clk framework with ARM's, as the clk I/F implementation varies depending on ARCHs and machines.

This patch adds a clock name so that other users with simple/minimum/ limited clk support could make use of the driver.

You're meant to match with both the device and name, I belive the clkdev specification has always said this. I'm of the opinion if the clkdev implementation isn't getting this right, then it is the clkdev that should be fixed, not this driver.

Quoted 18 lines
Signed-off-by Shinya Kuribayashi <[email protected]>

---
drivers/i2c/busses/i2c-designware.c | 2 +-

1 files changed, 1 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-designware.c +1 -1
index 9d81775..0f8bd4c 100644
@@ -575,7 +575,7 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	dev->irq = irq;
 	platform_set_drvdata(pdev, dev);
 
-	dev->clk = clk_get(&pdev->dev, NULL);
+	dev->clk = clk_get(&pdev->dev, "dw_i2c_clk");
 	if (IS_ERR(dev->clk)) {
 		r = -ENODEV;
 		goto err_free_mem;
Signature · 3 lines
Ben (ben at fluff.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'
author
#11 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

Quoted 11 lines · Ben Dooks
Quoted 10 lines · Shinya Kuribayashi

This driver is originally prepared for the ARM kernel where rich and well-maintained "clkdev" clock framework is available, and clock name might not be strictly required. ARM's clkdev does slightly fuzzy matching where it basically gives preference to "struct device" mathing over "clock id". As long as used for ARM machines, there's no problem.

However, all users of this driver necessarily don't have the same clk framework with ARM's, as the clk I/F implementation varies depending on ARCHs and machines.

Quoted 7 lines
Quoted 2 lines

This patch adds a clock name so that other users with simple/minimum/ limited clk support could make use of the driver.

You're meant to match with both the device and name, I belive the clkdev specification has always said this. I'm of the opinion if the clkdev implementation isn't getting this right, then it is the clkdev that should be fixed, not this driver.

Thanks both, Mark and Ben. I do see your point, so would like to fix my clk (not clkdev :-D) implementation accordingly, or keep the fix local.

# As you can imagine, my local clk implementation is based on
"clk_id" name matching, which looks simple for me.

---

By the way, my original intention of the patch was slightly different; I didn't intend to modify the driver for my clk framework. Let me explain a bit here.

The point is, we need to consider clk_get() implementation and clk_get() user, separately.

* clk_get() implementation varies depending on ARCHs and machines. For whatever historical reasons, this is the present situation.

* And clk_get() is expected to pick up a clock source with:

1) dev_id + clk_id ... strict matching condition (default)
2) dev_id only ... fuzzy extension1 (clk_id can be regarded as wildcard)
3) clk_id only ... fuzzy extension2 (dev_id can be regarded as wildcard)

2) and 3) might be sort of ARM's clkdev extensions (I think it's useful), but that's not my point. I'll leave that alone for now.

My point is that clk framework basically requires case 1).

* Then the driver is expected to supply necessary information to clk_get(), regardless of clk_get() implementation, or should I say, the driver should always supply both "dev_id" and "clk_id" whether they are used or not.

This makes drivers much independent from the clk framework, IMHO.

* From the point of drivers, we're never interested in clk_get() implementation, and which matching pattern 1) - 3) is taken.

Supplying only "dev_id" could be regarded as misuse of clk_get(), and will increase dependence on clk framework implementation.

* All other clk_get() users with one ID supplied, should be fixed as well.

I intended only to fix the driver in the right direction, so would like to avoid discussion on clk framework implementation here.

Anyway, I'm going to drop this fix next time. Thanks!

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
#12 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

Quoted 9 lines · Shinya Kuribayashi

* And clk_get() is expected to pick up a clock source with:

1) dev_id + clk_id ... strict matching condition (default)
2) dev_id only ... fuzzy extension1 (clk_id can be regarded as wildcard)
3) clk_id only ... fuzzy extension2 (dev_id can be regarded as wildcard)

2) and 3) might be sort of ARM's clkdev extensions (I think it's useful), but that's not my point. I'll leave that alone for now.

Part of the reason I was querying this is that you were using the clock name "dw_i2c_clk" which looks like a globally namespaced name which would be used with option 3 above. Several clock frameworks just completely ignore dev_id which creates a lot of problems down the line. If it had just been i2c_clk I'd have been less likely to follow up.

Option 3 has never been the intended use of the clock API and is basically a bug in any clock implementation that requires it for most drivers but it does need to be used in a few limited cases where the relevant kernel subsystem doesn't provide a struct device (cpufreq is the major offender here).

Quoted 4 lines

* From the point of drivers, we're never interested in clk_get() implementation, and which matching pattern 1) - 3) is taken.

Supplying only "dev_id" could be regarded as misuse of clk_get(),

As far as I can tell from what rmk is saying it seems that the use of a NULL string is actually an intended usage for devices where the clocks aren't otherwise named. I had the concerns when I first saw this usage but it does seem to be an expected and intended use.

Quoted 1 line

and will increase dependence on clk framework implementation.

To a certain extent this is good in that it applies pressure to improve the clock API implementations concerned.

#13 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

In reply to Shinya Kuribayashi
Quoted 2 lines · Shinya Kuribayashi

# As you can imagine, my local clk implementation is based on
"clk_id" name matching, which looks simple for me.

... but totally wrong, and will eventually bite you. Please use clkdev and use the device names for the primary matching. clkdev is what I see as the de-facto implementation that should eventually become totally arch independent (it is today, but lives in arch/arm for no real reason other than my lazyness.)

Quoted 11 lines

* And clk_get() is expected to pick up a clock source with:

1) dev_id + clk_id ... strict matching condition (default)
2) dev_id only ... fuzzy extension1 (clk_id can be regarded as wildcard)
3) clk_id only ... fuzzy extension2 (dev_id can be regarded as wildcard)

2) and 3) might be sort of ARM's clkdev extensions (I think it's useful), but that's not my point. I'll leave that alone for now.

My point is that clk framework basically requires case 1).

Incorrect. There is no requirement for strict matching. Think about the situation where you have multiple different devices, and the SoC designer has decided to tie all those clock signals to one clock source, and this is the only clock in the system.

You might decide that your clk_get() implementation can just return that single clock irrespective of the parameters passed. That's a totally legal clk API implementation.

Also, I refer you to the description of clk_get():

* clk_get - lookup and obtain a reference to a clock producer.
* @dev: device for clock "consumer"
* @id: clock comsumer ID
*
* Returns a struct clk corresponding to the clock producer, or
* valid IS_ERR() condition containing errno. The implementation
* uses @dev and @id to determine the clock consumer, and thereby
* the clock producer. (IOW, @id may be identical strings, but
* clk_get may return different clock producers depending on @dev.)

Note the comment about identical IDs returning different producers.

Quoted 4 lines

* Then the driver is expected to supply necessary information to clk_get(), regardless of clk_get() implementation, or should I say, the driver should always supply both "dev_id" and "clk_id" whether they are used or not.

Nope. On several devices which only consume one clock signal, we explicitly pass a NULL ID to absolutely prevent people going down the road of matching only by "clk id" - it's been shown to be the wrong approach, and leads people right down the path of passing clock strings and pointers into drivers.

That's precisely what the clk API was designed to prevent - and becomes all together unnecessary when the clk API is implemented as I designed it - to match primerily by device ID.

Quoted 2 lines

I intended only to fix the driver in the right direction, so would like to avoid discussion on clk framework implementation here.

I would strongly advise you to ensure that your implementation conforms to the intentions of the clk API as I outline above to avoid problems in the future.

author
#14 lore raw

PATCH 7/16 i2c-designware: Set a clock name to DesignWare I2C clock source

Ben, Mark and Russell,

Russell King - ARM Linux wrote:
[ big snip ]

Quoted 2 lines

I would strongly advise you to ensure that your implementation conforms to the intentions of the clk API

Thanks for kind explanations. I'd like to give the clkdev a try.

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
author
#15 lore raw

PATCH 8/16 i2c-designware: Improve _HCNT/_LCNT calculation

In reply to Shinya Kuribayashi

* Calculate with accurate conditional expressions from DW manuals.
* Round ic_clk by adding 0.5 as it's important at high ic_clk rate.
* Take into account "tHD;STA" issue for _HCNT calculation.
* Take into account "tf" for _LCNT calculation.
* Add "cond" and "offset" fot further correction requirements.

For _HCNT calculation, there's one issue needs to be carefully considered; DesignWare I2C core doesn't seem to have solid strategy to meet the tHD;STA timing spec. If you configure _HCNT based on the tHIGH timing spec, it easily results in violation of the tHD;STA spec.

After many trials, we came to the conclustion that the tHD;STA period is proportional to (_HCNT + 3), and this should be selected by default.

As for _LCNT calculation, DW I2C core has one characteristic behavior; he starts counting the SCL CNTs for the LOW period of the SCL clock (tLOW) as soon as it pulls the SCL line. At that time, he doesn't take into account the fall time of SCL signal (tf), IOW, he starts counting CNTs without confirming the SCL input voltage has dropped to below VIL.

This characteristics becomes a problem on some platforms where tf is considerably long, and results in violation of the tLOW timing spec.

To make the driver configurable as much as possible for various cases, we'd have separated arguments "tf" and "offset", and for safety default values should be 0.3 us and 0, respectively.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   92 +++++++++++++++++++++++++++++++---
 1 files changed, 83 insertions(+), 9 deletions(-)
drivers/i2c/busses/i2c-designware.c +83 -9
index 0f8bd4c..26efd3a 100644
@@ -196,6 +196,60 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 };
 
+static u32
+i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
+{
+	/*
+	 * DesignWare I2C core doesn't seem to have solid strategy to meet
+	 * the tHD;STA timing spec.  Configuring _HCNT based on tHIGH spec
+	 * will result in violation of the tHD;STA spec.
+	 */
+	if (cond)
+		/*
+		 * Conditional expression:
+		 *
+		 *   IC_[FS]S_SCL_HCNT + (1+4+3) >= IC_CLK * tHIGH
+		 *
+		 * This is based on the DW manuals, and representing an
+		 * ideal configuration.  The resulting I2C bus speed will
+		 * be faster than any of the others.
+		 *
+		 * If your hardware is free from tHD;STA issue, try this one.
+		 */
+		return (ic_clk * tSYMBOL + 5000) / 10000 - 8 + offset;
+	else
+		/*
+		 * Conditional expression:
+		 *
+		 *   IC_[FS]S_SCL_HCNT + 3 >= IC_CLK * (tHD;STA + tf)
+		 *
+		 * This is just experimental rule; the tHD;STA period turned
+		 * out to be proportinal to (_HCNT + 3).  With this setting,
+		 * we could meet both tHIGH and tHD;STA timing specs.
+		 *
+		 * If unsure, you'd better to take this alternative.
+		 *
+		 * The reason why we need to take into account "tf" here,
+		 * is the same as described in i2c_dw_scl_lcnt().
+		 */
+		return (ic_clk * (tSYMBOL + tf) + 5000) / 10000 - 3 + offset;
+}
+
+static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
+{
+	/*
+	 * Conditional expression:
+	 *
+	 *   IC_[FS]S_SCL_LCNT + 1 >= IC_CLK * (tLOW + tf)
+	 *
+	 * DW I2C core starts counting the SCL CNTs for the LOW period
+	 * of the SCL clock (tLOW) as soon as it pulls the SCL line.
+	 * We need to take into account tf to meet the tLOW timing spec.
+	 * Default tf value should be 0.3 us, for safety.
+	 */
+	return ((ic_clk * (tLOW + tf) + 5000) / 10000) - 1 + offset;
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data

@@ -207,20 +261,40 @@ struct dw_i2c_dev {
 static void i2c_dw_init(struct dw_i2c_dev *dev)
 {
 	u32 input_clock_khz = clk_get_rate(dev->clk) / 1000;
-	u32 ic_con;
+	u32 ic_con, hcnt, lcnt;
 
 	/* Disable the adapter */
 	writel(0, dev->base + DW_IC_ENABLE);
 
 	/* set standard and fast speed deviders for high/low periods */
-	writel((input_clock_khz * 40 / 10000)+1, /* std speed high, 4us */
-			dev->base + DW_IC_SS_SCL_HCNT);
-	writel((input_clock_khz * 47 / 10000)+1, /* std speed low, 4.7us */
-			dev->base + DW_IC_SS_SCL_LCNT);
-	writel((input_clock_khz *  6 / 10000)+1, /* fast speed high, 0.6us */
-			dev->base + DW_IC_FS_SCL_HCNT);
-	writel((input_clock_khz * 13 / 10000)+1, /* fast speed low, 1.3us */
-			dev->base + DW_IC_FS_SCL_LCNT);
+
+	/* Standard-mode */
+	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+				40,	/* tHD;STA = tHIGH = 4.0 us */
+				3,	/* tf = 0.3 us */
+				0,	/* 0: DW default, 1: Ideal */
+				0);	/* No offset */
+	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+				47,	/* tLOW = 4.7 us */
+				3,	/* tf = 0.3 us */
+				0);	/* No offset */
+	writel(hcnt, dev->base + DW_IC_SS_SCL_HCNT);
+	writel(lcnt, dev->base + DW_IC_SS_SCL_LCNT);
+	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
+
+	/* Fast-mode */
+	hcnt = i2c_dw_scl_hcnt(input_clock_khz,
+				6,	/* tHD;STA = tHIGH = 0.6 us */
+				3,	/* tf = 0.3 us */
+				0,	/* 0: DW default, 1: Ideal */
+				0);	/* No offset */
+	lcnt = i2c_dw_scl_lcnt(input_clock_khz,
+				13,	/* tLOW = 1.3 us */
+				3,	/* tf = 0.3 us */
+				0);	/* No offset */
+	writel(hcnt, dev->base + DW_IC_FS_SCL_HCNT);
+	writel(lcnt, dev->base + DW_IC_FS_SCL_LCNT);
+	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
 	/* configure the i2c master */
 	ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
Signature · 1 line
1.6.5
author
#16 lore raw

PATCH 9/16 i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug

In reply to Shinya Kuribayashi

In case an i2c_msg, which is currently work-in-progress, has more bytes to be written, we need to set STATUS_WRITE_IN_PROGRESS _and_ exit from the msg_write_idx-search loop. Otherwise, we'll overtake the current index without waiting for transmission completed.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   17 +++++++++++------
 1 files changed, 11 insertions(+), 6 deletions(-)
drivers/i2c/busses/i2c-designware.c +11 -6
index 26efd3a..6bfdc5f 100644
@@ -386,17 +386,22 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 						dev->base + DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}
+
+		dev->tx_buf_len = buf_len;
+
+		/* more bytes to be written? */
+		if (buf_len > 0) {
+			dev->status |= STATUS_WRITE_IN_PROGRESS;
+			break;
+		} else {
+			dev->status &= ~STATUS_WRITE_IN_PROGRESS;
+		}
 	}
 
 	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
-	if (buf_len > 0) { /* more bytes to be written */
+	if (buf_len > 0)
 		intr_mask |= DW_IC_INTR_TX_EMPTY;
-		dev->status |= STATUS_WRITE_IN_PROGRESS;
-	} else
-		dev->status &= ~STATUS_WRITE_IN_PROGRESS;
 	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
-
-	dev->tx_buf_len = buf_len;
 }
 
 static void
Signature · 1 line
1.6.5
author
#17 lore raw

PATCH 10/16 i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler

In reply to Shinya Kuribayashi

Symptom:
--------
When we're going to send/receive the data with bigger size than the Tx FIFO length, the I2C transaction will be divided into several separated transactions, limited by the Tx FIFO length.

Details:
--------
As a hardware feature, DesignWare I2C core emits a STOP condition whenever Tx FIFO becomes empty (strictly speaking, whenever the last byte in the Tx FIFO has been sent out), even if we have more bytes to be written. Then, once a new transmit data is written to the Tx FIFO, DW I2C core will initiate a new I2C transaction, which leads to another START condition.

This explains how the transaction in question goes, and implies that current tasklet-based dw_i2c_pump_msg() couldn't meet the timing constraint required for avoiding the Tx FIFO underrun.

To avoid this scenario, we must keep providing new transmit data out within a given time period. In case of Fast-mode + 32-byte Tx FIFO, for instance, it takes about 22.5[us] to process one byte, and 720[us] in total.

This patch removes the existing tasklet-based "pump" system, then put its jobs into the interrupt handler.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   42 ++++++++++++++--------------------
 1 files changed, 17 insertions(+), 25 deletions(-)
drivers/i2c/busses/i2c-designware.c +17 -25
index 6bfdc5f..52a69a2 100644
@@ -149,7 +149,6 @@ static char *abort_sources[] = {
  * @dev: driver model device node
  * @base: IO registers pointer
  * @cmd_complete: tx completion indicator
- * @pump_msg: continue in progress transfers
  * @lock: protect this struct and IO registers
  * @clk: input reference clock
  * @cmd_err: run time hadware error code

@@ -175,7 +174,6 @@ struct dw_i2c_dev {
 	struct device		*dev;
 	void __iomem		*base;
 	struct completion	cmd_complete;
-	struct tasklet_struct	pump_msg;
 	struct mutex		lock;
 	struct clk		*clk;
 	int			cmd_err;

@@ -324,7 +322,7 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 /*
  * Initiate low level master read/write transaction.
  * This function is called from i2c_dw_xfer when starting a transfer.
- * This function is also called from dw_i2c_pump_msg to continue a transfer
+ * This function is also called from i2c_dw_isr to continue a transfer
  * that is longer than the size of the TX FIFO.
  */
 static void

@@ -489,10 +487,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	/* no error */
 	if (likely(!dev->cmd_err)) {
-		/* read rx fifo, and disable the adapter */
-		do {
-			i2c_dw_read(dev);
-		} while (dev->status & STATUS_READ_IN_PROGRESS);
+		/* Disable the adapter */
 		writel(0, dev->base + DW_IC_ENABLE);
 		ret = num;
 		goto done;

@@ -520,20 +515,6 @@ static u32 i2c_dw_func(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
 }
 
-static void dw_i2c_pump_msg(unsigned long data)
-{
-	struct dw_i2c_dev *dev = (struct dw_i2c_dev *) data;
-	u32 intr_mask;
-
-	i2c_dw_read(dev);
-	i2c_dw_xfer_msg(dev);
-
-	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
-	if (dev->status & STATUS_WRITE_IN_PROGRESS)
-		intr_mask |= DW_IC_INTR_TX_EMPTY;
-	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
-}
-
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
 {
 	u32 stat;

@@ -599,10 +580,22 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
 		dev->cmd_err |= DW_IC_ERR_TX_ABRT;
 		dev->status = STATUS_IDLE;
-	} else if (stat & DW_IC_INTR_TX_EMPTY)
-		tasklet_schedule(&dev->pump_msg);
+	}
+
+	if (stat & DW_IC_INTR_TX_EMPTY) {
+		/* Allocate room for data reception prior to xfer_msg() */
+		i2c_dw_read(dev);
+		i2c_dw_xfer_msg(dev);
+	}
+
+	/*
+	 * No need to modify or disable the interrupt mask here, as
+	 * i2c_dw_xfer_msg() above will do that job according to the
+	 * state machine.
+	 *
+	 * This comment can be removed once approved by I2C list.
+	 */
 
-	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */
 	if (stat & (DW_IC_INTR_TX_ABRT | DW_IC_INTR_STOP_DET))
 		complete(&dev->cmd_complete);
 

@@ -648,7 +641,6 @@ static int __devinit dw_i2c_probe(struct platform_device *pdev)
 	}
 
 	init_completion(&dev->cmd_complete);
-	tasklet_init(&dev->pump_msg, dw_i2c_pump_msg, (unsigned long) dev);
 	mutex_init(&dev->lock);
 	dev->dev = get_device(&pdev->dev);
 	dev->irq = irq;
Signature · 1 line
1.6.5
author
#18 lore raw

PATCH 11/16 i2c-designware: Set Tx/Rx FIFO threshold levels

In reply to Shinya Kuribayashi

As a hardware feature, DesignWare I2C core emits a STOP condition whenever Tx FIFO becomes empty (strictly speaking, whenever the last byte in the Tx FIFO has been sent out), even if we have more bytes to be written.

In other words, we must never make "Tx FIFO underrun" happen during a transaction, except for the last byte. For the safety's sake, we'd make TX_EMPTY interrupt get triggered every time one byte is processed.

Rx FIFO threshold needs to be set as well according to such tx policy. In addition, this patch also modifies the interrupt settings properly (enable RX_FULL mask, and hook it in the handler)

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)
drivers/i2c/busses/i2c-designware.c +11 -4
index 52a69a2..d9b5790 100644
@@ -50,6 +50,8 @@
 #define DW_IC_INTR_STAT		0x2c
 #define DW_IC_INTR_MASK		0x30
 #define DW_IC_RAW_INTR_STAT	0x34
+#define DW_IC_RX_TL		0x38
+#define DW_IC_TX_TL		0x3c
 #define DW_IC_CLR_INTR		0x40
 #define DW_IC_CLR_RX_UNDER	0x44
 #define DW_IC_CLR_RX_OVER	0x48

@@ -294,6 +296,10 @@ static void i2c_dw_init(struct dw_i2c_dev *dev)
 	writel(lcnt, dev->base + DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure Tx/Rx FIFO threshold levels */
+	writel(dev->tx_fifo_depth - 1, dev->base + DW_IC_TX_TL);
+	writel(0, dev->base + DW_IC_RX_TL);
+
 	/* configure the i2c master */
 	ic_con = DW_IC_CON_MASTER | DW_IC_CON_SLAVE_DISABLE |
 		DW_IC_CON_RESTART_EN | DW_IC_CON_SPEED_FAST;

@@ -396,7 +402,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		}
 	}
 
-	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT;
+	intr_mask = DW_IC_INTR_STOP_DET | DW_IC_INTR_TX_ABRT | DW_IC_INTR_RX_FULL;
 	if (buf_len > 0)
 		intr_mask |= DW_IC_INTR_TX_EMPTY;
 	writel(intr_mask, dev->base + DW_IC_INTR_MASK);

@@ -582,11 +588,12 @@ static irqreturn_t i2c_dw_isr(int this_irq, void *dev_id)
 		dev->status = STATUS_IDLE;
 	}
 
-	if (stat & DW_IC_INTR_TX_EMPTY) {
-		/* Allocate room for data reception prior to xfer_msg() */
+	/* Allocate room for data reception prior to i2c_dw_xfer_msg(). */
+	if (stat & DW_IC_INTR_RX_FULL)
 		i2c_dw_read(dev);
+
+	if (stat & DW_IC_INTR_TX_EMPTY)
 		i2c_dw_xfer_msg(dev);
-	}
 
 	/*
 	 * No need to modify or disable the interrupt mask here, as
Signature · 1 line
1.6.5
author
#19 lore raw

PATCH 12/16 i2c-designware: Divide i2c_dw_xfer_msg into two functions

In reply to Shinya Kuribayashi

We have some steps at the top of i2c_dw_xfer_msg() to set up the slave address and enable the DW I2C core. It's executed only when we don't have STATUS_WRITE_IN_PROGRESS.

Then we need to make sure that STATUS_WRITE_IN_PROGRESS only indicates that we have a pending i2c_msg to process. In other words, even if STATUS_WRITE_IN_PROGRESS is not set, that doesn't mean we're at initial state in the I2C transaction.

Since i2c_dw_xfer_msg() will be invoked again and again during the transaction, those init steps have a possibility to be re-processed needlessly. For example, this issue easily takes place when processing a combined transaction with a certain condition (the number of tx bytes in the first i2c_msg == Tx FIFO depth).

Consequently we should not use STATUS_WRITE_IN_PROGRESS to determine where we're at in the I2C transaction, then let's separate those initialization steps from i2c_dw_xfer_msg().

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   45 +++++++++++++++++++---------------
 1 files changed, 25 insertions(+), 20 deletions(-)
drivers/i2c/busses/i2c-designware.c +25 -20
index d9b5790..de006f0 100644
@@ -325,6 +325,29 @@ static int i2c_dw_wait_bus_not_busy(struct dw_i2c_dev *dev)
 	return 0;
 }
 
+static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
+{
+	struct i2c_msg *msgs = dev->msgs;
+	u32 ic_con;
+
+	/* Disable the adapter */
+	writel(0, dev->base + DW_IC_ENABLE);
+
+	/* set the slave (target) address */
+	writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
+
+	/* if the slave address is ten bit address, enable 10BITADDR */
+	ic_con = readl(dev->base + DW_IC_CON);
+	if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
+		ic_con |= DW_IC_CON_10BITADDR_MASTER;
+	else
+		ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
+	writel(ic_con, dev->base + DW_IC_CON);
+
+	/* Enable the adapter */
+	writel(1, dev->base + DW_IC_ENABLE);
+}
+
 /*
  * Initiate low level master read/write transaction.
  * This function is called from i2c_dw_xfer when starting a transfer.

@@ -335,31 +358,12 @@ static void
 i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
-	u32 ic_con, intr_mask;
+	u32 intr_mask;
 	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
 	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 
-	if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
-		/* Disable the adapter */
-		writel(0, dev->base + DW_IC_ENABLE);
-
-		/* set the slave (target) address */
-		writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
-
-		/* if the slave address is ten bit address, enable 10BITADDR */
-		ic_con = readl(dev->base + DW_IC_CON);
-		if (msgs[dev->msg_write_idx].flags & I2C_M_TEN)
-			ic_con |= DW_IC_CON_10BITADDR_MASTER;
-		else
-			ic_con &= ~DW_IC_CON_10BITADDR_MASTER;
-		writel(ic_con, dev->base + DW_IC_CON);
-
-		/* Enable the adapter */
-		writel(1, dev->base + DW_IC_ENABLE);
-	}
-
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		/* if target address has changed, we need to
 		 * reprogram the target address in the i2c

@@ -474,6 +478,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		goto done;
 
 	/* start the transfers */
+	i2c_dw_xfer_init(dev);
 	i2c_dw_xfer_msg(dev);
 
 	/* wait for tx to complete */
Signature · 1 line
1.6.5
author
#20 lore raw

PATCH 13/16 i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer

In reply to Shinya Kuribayashi

While we have "buf_len" local variable for dev->tx_buf_len, we don't have such local variable for dev->tx_buf pointer. While "buf_len" is restored at first then updated when we're going to process a new i2c_msg (in WRITE_IN_PROGRESS case), ->tx_buf is never done so.

Such inconsistency makes the code slightly hard to follow.

Signed-off-by Shinya Kuribayashi <[email protected]>

---

Furthermore, even with this change, i2c_dw_xfer_msg() is still inconsistent with i2c_dw_read(). I don't have preference around here, but would like to sort out. Any suggestions are welcome.

 drivers/i2c/busses/i2c-designware.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-designware.c +4 -3
index de006f0..b3152c2 100644
@@ -363,6 +363,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
+	u8 *buf = dev->tx_buf;;
 
 	for (; dev->msg_write_idx < dev->msgs_num; dev->msg_write_idx++) {
 		/* if target address has changed, we need to

@@ -381,7 +382,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 
 		if (!(dev->status & STATUS_WRITE_IN_PROGRESS)) {
 			/* new i2c_msg */
-			dev->tx_buf = msgs[dev->msg_write_idx].buf;
+			buf	= msgs[dev->msg_write_idx].buf;
 			buf_len = msgs[dev->msg_write_idx].len;
 		}
 

@@ -390,12 +391,12 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 				writel(0x100, dev->base + DW_IC_DATA_CMD);
 				rx_limit--;
 			} else
-				writel(*(dev->tx_buf++),
-						dev->base + DW_IC_DATA_CMD);
+				writel(*buf++, dev->base + DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}
 
 		dev->tx_buf_len = buf_len;
+		dev->tx_buf = buf;
 
 		/* more bytes to be written? */
 		if (buf_len > 0) {
Signature · 1 line
1.6.5
author
#21 lore raw

PATCH 14/16 i2c-designware: Deferred FIFO-data-counting variables initialization

In reply to Shinya Kuribayashi

As the driver and hardware always process the given data in parallel, it would be better to initialize those {tx,rx}_limit or rx_valid variables just prior to be used.

This will help to send/receive data as much as possible.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)
drivers/i2c/busses/i2c-designware.c +7 -3
index b3152c2..f7ea032 100644
@@ -359,8 +359,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 intr_mask;
-	int tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
-	int rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
+	int tx_limit, rx_limit;
 	u32 addr = msgs[dev->msg_write_idx].addr;
 	u32 buf_len = dev->tx_buf_len;
 	u8 *buf = dev->tx_buf;;

@@ -386,6 +385,9 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			buf_len = msgs[dev->msg_write_idx].len;
 		}
 
+		tx_limit = dev->tx_fifo_depth - readl(dev->base + DW_IC_TXFLR);
+		rx_limit = dev->rx_fifo_depth - readl(dev->base + DW_IC_RXFLR);
+
 		while (buf_len > 0 && tx_limit > 0 && rx_limit > 0) {
 			if (msgs[dev->msg_write_idx].flags & I2C_M_RD) {
 				writel(0x100, dev->base + DW_IC_DATA_CMD);

@@ -418,7 +420,7 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 {
 	struct i2c_msg *msgs = dev->msgs;
 	u32 addr = msgs[dev->msg_read_idx].addr;
-	int rx_valid = readl(dev->base + DW_IC_RXFLR);
+	int rx_valid;
 
 	for (; dev->msg_read_idx < dev->msgs_num; dev->msg_read_idx++) {
 		u32 len;

@@ -439,6 +441,8 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			buf = dev->rx_buf;
 		}
 
+		rx_valid = readl(dev->base + DW_IC_RXFLR);
+
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
 			*buf++ = readl(dev->base + DW_IC_DATA_CMD);
Signature · 1 line
1.6.5
author
#22 lore raw

PATCH 15/16 i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error

In reply to Shinya Kuribayashi

As wait_for_completion_interruptible_timeout() will be invoked after the first call to i2c_dw_xfer_msg() is made whether or not an error is detected in it, we need to mark ->cmd_complete as completed to avoid a needless HZ timeout.

Signed-off-by Shinya Kuribayashi <[email protected]>

---

Or we could change the i2c_dw_xfer_msg() prototype from "void" to "int". Which is preffered?

 drivers/i2c/busses/i2c-designware.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
drivers/i2c/busses/i2c-designware.c +1 -0
index f7ea032..6f85e28 100644
@@ -376,6 +376,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			dev_err(dev->dev,
 				"%s: invalid message length\n", __func__);
 			dev->msg_err = -EINVAL;
+			complete(&dev->cmd_complete);
 			return;
 		}
Signature · 1 line
1.6.5
author
#23 lore raw

PATCH 15/16 i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error

Quoted 10 lines · Shinya Kuribayashi

As wait_for_completion_interruptible_timeout() will be invoked after the first call to i2c_dw_xfer_msg() is made whether or not an error is detected in it, we need to mark ->cmd_complete as completed to avoid a needless HZ timeout.

drivers/i2c/busses/i2c-designware.c +0 -0
index f7ea032..6f85e28 100644

drivers/i2c/busses/i2c-designware.c i2c_dw_xfer_msg(struct dw_i2c_dev *dev) · line 381

Quoted 6 lines
drivers/i2c/busses/i2c-designware.c +1 -0
@@ -376,6 +376,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 			dev_err(dev->dev,
 				"%s: invalid message length\n", __func__);
 			dev->msg_err = -EINVAL;
+			complete(&dev->cmd_complete);
 			return;
 		}

It turned out to be incomplete, I confimred with I2C_FUNC_SMBUS_QUICK flag for testing.

In [PATCH 10/16] (Do dw_i2c_pump_msg's jobs in the interrutp handler), I deleted the following line from the interrupt handler,

drivers/i2c/busses/i2c-designware.c i2c_dw_xfer_msg(struct dw_i2c_dev *dev) · line 381

Quoted 1 line
drivers/i2c/busses/i2c-designware.c +0 -1
-	writel(0, dev->base + DW_IC_INTR_MASK);	/* disable interrupts */

so some interrupt bits, namely RX_FULL and somtimes TX_EMPTY, are now kept _opened_ by default. In error handling procedures, we need to disable them or shutdown DW I2C core properly, otherwise those interrupts continue to be asserted.

In addition, I noticed that we need similar treatments in target addr inconsistency checking path.

Here's problematic parts. I'll sort out these bits, later.

---
 drivers/i2c/busses/i2c-designware.c |   18 ++++++++++++++++--
 1 files changed, 16 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-designware.c +16 -2
index 060f2dd..70aaaec 100644
@@ -370,13 +370,21 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 		 * reprogram the target address in the i2c
 		 * adapter when we are done with this transfer
 		 */
-		if (msgs[dev->msg_write_idx].addr != addr)
+		if (msgs[dev->msg_write_idx].addr != addr) {
+			dev_err(dev->dev,
+				"%s: invalid message address\n", __func__);
+			dev->msg_err = -EINVAL;
+			writel(0, dev->base + DW_IC_ENABLE);
+			complete(&dev->cmd_complete);
 			return;
+		}
 
 		if (msgs[dev->msg_write_idx].len == 0) {
 			dev_err(dev->dev,
 				"%s: invalid message length\n", __func__);
 			dev->msg_err = -EINVAL;
+			writel(0, dev->base + DW_IC_ENABLE);
+			complete(&dev->cmd_complete);
 			return;
 		}
 

@@ -430,8 +438,14 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			continue;
 
 		/* different i2c client, reprogram the i2c adapter */
-		if (msgs[dev->msg_read_idx].addr != addr)
+		if (msgs[dev->msg_read_idx].addr != addr) {
+			dev_err(dev->dev,
+				"%s: invalid message address\n", __func__);
+			dev->msg_err = -EINVAL;
+			writel(0, dev->base + DW_IC_ENABLE);
+			complete(&dev->cmd_complete);
 			return;
+		}
 
 		if (!(dev->status & STATUS_READ_IN_PROGRESS)) {
 			len = msgs[dev->msg_read_idx].len;
Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
author
#24 lore raw

PATCH 16/16 i2c-designware: Add I2C_FUNC_SMBUS_* bits

In reply to Shinya Kuribayashi

This will ease our testing a bit with i2c-tools. Note that DW I2C core doesn't support I2C_FUNC_SMBUS_QUICK, as it's not capable of slave- addressing-only I2C transactions.

Signed-off-by Shinya Kuribayashi <[email protected]>
---
 drivers/i2c/busses/i2c-designware.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-designware.c +8 -1
index 6f85e28..80c8b8a 100644
@@ -529,7 +529,14 @@ done:
 
 static u32 i2c_dw_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
+	return	I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK |
+		I2C_FUNC_SMBUS_I2C_BLOCK_2;
 }
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
Signature · 1 line
1.6.5
#25 lore raw

PATCH 16/16 i2c-designware: Add I2C_FUNC_SMBUS_* bits

Hi Shinya,

Quoted 3 lines · Shinya Kuribayashi

This will ease our testing a bit with i2c-tools. Note that DW I2C core doesn't support I2C_FUNC_SMBUS_QUICK, as it's not capable of slave- addressing-only I2C transactions.

Is this supposed to be applied to mainline?

baruch

Quoted 29 lines
Signed-off-by Shinya Kuribayashi <[email protected]>

---
drivers/i2c/busses/i2c-designware.c | 9 ++++++++-

1 files changed, 8 insertions(+), 1 deletions(-)
drivers/i2c/busses/i2c-designware.c +8 -1
index 6f85e28..80c8b8a 100644
@@ -529,7 +529,14 @@ done:
 
 static u32 i2c_dw_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
+	return	I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK |
+		I2C_FUNC_SMBUS_I2C_BLOCK_2;
 }
 
 static u32 i2c_dw_read_clear_intrbits(struct dw_i2c_dev *dev)
Signature · 1 line
1.6.5
Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
author
#26 lore raw

PATCH 16/16 i2c-designware: Add I2C_FUNC_SMBUS_* bits

Hi Baruch,

Quoted 6 lines · Baruch Siach
Quoted 3 lines · Shinya Kuribayashi

This will ease our testing a bit with i2c-tools. Note that DW I2C core doesn't support I2C_FUNC_SMBUS_QUICK, as it's not capable of slave- addressing-only I2C transactions.

Is this supposed to be applied to mainline?

Yes, I hope so. But I have to admit I blindly added several flags for my testing, and should have audited them before submitting patches.

Quoted 14 lines
Quoted 4 lines
drivers/i2c/busses/i2c-designware.c +0 -0
@@ -529,7 +529,14 @@ done:
 
 static u32 i2c_dw_func(struct i2c_adapter *adap)
 {

drivers/i2c/busses/i2c-designware.c done: · line 540

Quoted 10 lines
drivers/i2c/busses/i2c-designware.c +8 -1
-	return I2C_FUNC_I2C | I2C_FUNC_10BIT_ADDR;
+	return	I2C_FUNC_I2C |
+		I2C_FUNC_10BIT_ADDR |
+		I2C_FUNC_SMBUS_BYTE |
+		I2C_FUNC_SMBUS_BYTE_DATA |
+		I2C_FUNC_SMBUS_WORD_DATA |
+		I2C_FUNC_SMBUS_BLOCK_DATA |
+		I2C_FUNC_SMBUS_I2C_BLOCK |
+		I2C_FUNC_SMBUS_I2C_BLOCK_2;
 }

As far as I confirmed the requirements for having I2C_FUNC_SMBUS_* from drivers/i2c/,

Quoted 3 lines

drivers/i2c/busses/i2c-designware.c done:

Quoted 3 lines
drivers/i2c/busses/i2c-designware.c +2 -0
+		I2C_FUNC_SMBUS_BLOCK_DATA |
 
+		I2C_FUNC_SMBUS_I2C_BLOCK_2;

should be removed. About the former, we have not implemented proper I2C_M_RECV_LEN handling yet [ I'm not sure what it's for ... ], and the latter doesn't seem to be used anywhere in the kernel. As for the rest, BYTE/WORD/I2C_BLOCK transaction works for me.

So the resulting func() would be,

static u32 i2c_dw_func(struct i2c_adapter *adap)

{
	return	I2C_FUNC_I2C |
		I2C_FUNC_10BIT_ADDR |
		I2C_FUNC_SMBUS_BYTE |
		I2C_FUNC_SMBUS_BYTE_DATA |
		I2C_FUNC_SMBUS_WORD_DATA |
		I2C_FUNC_SMBUS_I2C_BLOCK;
}

and will be fixed up in the next patchset.

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
#27 lore raw
In reply to Shinya Kuribayashi

Hi Shinya,

Quoted 10 lines · Shinya Kuribayashi

Here're various improvements / bug-fixing patches for DW I2C driver. I'm working with v2.6.27-based kernel, but they must work fine with the latest mainline kernel.

It's stil in RFC, and I'm working with it for some time. Any comments or suggestions are highly appreciated. Then I'll respin and give it a test, thanks.

Baruch, I'd say the base driver is in good shape enogh, so I'm having a fun with modifing the driver. Thanks for the initial work.

Thanks. It is good to see that my work is actually useful for someone. I'll go over this series in the coming days. Unfortunately I don't have the hardware for testing anymore, but I'll try at least to compile test these patches.

By the way, what is the chip you work with?

baruch

Quoted 28 lines

P.S. ARM and MIPS lists are Cc:ed as I believe there must be potential users of this driver.

Shinya Kuribayashi (16):
i2c-designware: Consolidate to use 32-bit word accesses
i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts
i2c-designware: Use platform_get_irq helper
i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer
i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer
i2c-designware: Remove an useless local variable "num"
i2c-designware: Set a clock name to DesignWare I2C clock source
i2c-designware: Improve _HCNT/_LCNT calculation
i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug
i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler
i2c-designware: Set Tx/Rx FIFO threshold levels
i2c-designware: Divide i2c_dw_xfer_msg into two functions
i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer
i2c-designware: Deferred FIFO-data-counting variables initialization
i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error
i2c-designware: Add I2C_FUNC_SMBUS_* bits

drivers/i2c/busses/i2c-designware.c | 367 +++++++++++++++++++++++++----------

1 files changed, 264 insertions(+), 103 deletions(-)
Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
author
#28 lore raw
Quoted 9 lines · Baruch Siach
Quoted 8 lines · Shinya Kuribayashi

Here're various improvements / bug-fixing patches for DW I2C driver. I'm working with v2.6.27-based kernel, but they must work fine with the latest mainline kernel.

It's stil in RFC, and I'm working with it for some time. Any comments or suggestions are highly appreciated. Then I'll respin and give it a test, thanks.

Quoted 6 lines
Quoted 2 lines

Baruch, I'd say the base driver is in good shape enogh, so I'm having a fun with modifing the driver. Thanks for the initial work.

Thanks. It is good to see that my work is actually useful for someone. I'll go over this series in the coming days. Unfortunately I don't have the hardware for testing anymore, but I'll try at least to compile test these patches.

No problem, I could do real tests on real hardwares, so just give me a feedback. Grammar corrections or rewording is also appriciated.

Quoted 1 line

By the way, what is the chip you work with?

I'd like to avoid specific comment on this. I'm working with our big- endian MIPS SoCs which are not in mainline.

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics
Acked-by
#29 lore raw
In reply to Shinya Kuribayashi

Hi Shinya,

Quoted 24 lines · Shinya Kuribayashi

Here're various improvements / bug-fixing patches for DW I2C driver. I'm working with v2.6.27-based kernel, but they must work fine with the latest mainline kernel.

It's stil in RFC, and I'm working with it for some time. Any comments or suggestions are highly appreciated. Then I'll respin and give it a test, thanks.

Baruch, I'd say the base driver is in good shape enogh, so I'm having a fun with modifing the driver. Thanks for the initial work.

P.S. ARM and MIPS lists are Cc:ed as I believe there must be potential users of this driver.

Shinya Kuribayashi (16):
i2c-designware: Consolidate to use 32-bit word accesses
i2c-designware: Don't use the IC_CLR_INTR register to clear interrupts
i2c-designware: Use platform_get_irq helper
i2c-designware: i2c_dw_read: Take "struct dw_i2c_dev" pointer
i2c-designware: i2c_dw_xfer_msg: Take "struct dw_i2c_dev" pointer
i2c-designware: Remove an useless local variable "num"
i2c-designware: Set a clock name to DesignWare I2C clock source
i2c-designware: Improve _HCNT/_LCNT calculation

Quoted 8 lines

i2c-designware: i2c_dw_xfer_msg: Fix an i2c_msg search bug
i2c-designware: Do dw_i2c_pump_msg's jobs in the interrutp handler
i2c-designware: Set Tx/Rx FIFO threshold levels
i2c-designware: Divide i2c_dw_xfer_msg into two functions
i2c-designware: i2c_dw_xfer_msg: Introduce a local "buf" pointer
i2c-designware: Deferred FIFO-data-counting variables initialization
i2c-designware: i2c_dw_xfer_msg: Mark as completed on an error
i2c-designware: Add I2C_FUNC_SMBUS_* bits

Great job.

Your contribution to the implementation of transactions that are longer than the hardware buffers is very important. I wrote this code as a theoretical exercise, as I never had access to an i2c slave which requires long transactions. So I'm not surprised to find out that this code was broken.

For all these patches except no. 7 (Set a clock name to DesignWare I2C clock source) and no. 16 (Add I2C_FUNC_SMBUS_* bits) you have my

Acked-by Baruch Siach <[email protected]>

Thanks,
baruch

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
#30 lore raw

Hi Baruch,

Quoted 4 lines · Baruch Siach

Your contribution to the implementation of transactions that are longer than the hardware buffers is very important. I wrote this code as a theoretical exercise, as I never had access to an i2c slave which requires long transactions. So I'm not surprised to find out that this code was broken.

Aa far as looking through the source code, I know that it's broken without giving it a try with real DW I2C hardware ;-), but that' not a big problem.

As said in the covermail, your initial work, namely i2c_dw_xfer(), i2c_dw_xfer_msg(), and i2c_dw_read() is well-designed, this _is_ very important. That makes further enhancements of DW I2C transactions much easier, and makes me decide to post the patches.

Quoted 4 lines

For all these patches except no. 7 (Set a clock name to DesignWare I2C clock source) and no. 16 (Add I2C_FUNC_SMBUS_* bits) you have my

Acked-by Baruch Siach <[email protected]>

Thanks, will do.

I added more 4-5 patches fixing transmit abort handling, and misc updates all over the patchset (typos, comments, better logics). They will come up, hopefully, next week.

Shinya