Skip to contentSkip to conversation

i2c-designware: Allow mixed endianness

PATCH Superseded Jean-Hugues Deschenes 5 participants 18 messages Jan 13, 2010 – Jan 18, 2010 13 messages not in the archive I2C subsystem host drivers

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

cover i2c-designware: Allow mixed endianness

Hi Baruch and Ben,

Here is patchset for the i2c-designware driver, allowing little endian machines to use the DW IP instantiated in big endian and vice versa.

#1 lore raw

PATCH 1/3 Use local version of readl & writel

Use local versions of readl & writel, so per-access manipulations may be performed

---
 drivers/i2c/busses/i2c-designware.c |   88 ++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 39 deletions(-)
drivers/i2c/busses/i2c-designware.c +49 -39
Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================
@@ -219,6 +219,16 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 };
 
+static u32 i2c_dw_readl(struct dw_i2c_dev *dev, int addr)
+{
+	return readl(dev->base + addr);
+}
+
+static void i2c_dw_writel(struct dw_i2c_dev *dev, u32 b, int addr)
+{
+	writel(b, dev->base + addr);
+}
+
 static u32
 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {
@@ -288,7 +298,7 @@ static void i2c_dw_init(struct dw_i2c_de
 	u32 ic_con, hcnt, lcnt;
 
 	/* Disable the adapter */
-	writel(0, dev->base + DW_IC_ENABLE);
+	i2c_dw_writel(dev, 0, DW_IC_ENABLE);
 
 	/* set standard and fast speed deviders for high/low periods */
 
@@ -302,8 +312,8 @@ static void i2c_dw_init(struct dw_i2c_de
 				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);
+	i2c_dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT);
+	i2c_dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT);
 	dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
 	/* Fast-mode */
@@ -316,18 +326,18 @@ static void i2c_dw_init(struct dw_i2c_de
 				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);
+	i2c_dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT);
+	i2c_dw_writel(dev, lcnt, 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);
+	i2c_dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
+	i2c_dw_writel(dev, 0, 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;
-	writel(ic_con, dev->base + DW_IC_CON);
+	i2c_dw_writel(dev, ic_con, DW_IC_CON);
 }
 
 /*
@@ -337,7 +347,7 @@ static int i2c_dw_wait_bus_not_busy(stru
 {
 	int timeout = TIMEOUT;
 
-	while (readl(dev->base + DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
+	while (i2c_dw_readl(dev, DW_IC_STATUS) & DW_IC_STATUS_ACTIVITY) {
 		if (timeout <= 0) {
 			dev_warn(dev->dev, "timeout waiting for bus ready\n");
 			return -ETIMEDOUT;
@@ -355,24 +365,24 @@ static void i2c_dw_xfer_init(struct dw_i
 	u32 ic_con;
 
 	/* Disable the adapter */
-	writel(0, dev->base + DW_IC_ENABLE);
+	i2c_dw_writel(dev, 0, DW_IC_ENABLE);
 
 	/* set the slave (target) address */
-	writel(msgs[dev->msg_write_idx].addr, dev->base + DW_IC_TAR);
+	i2c_dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
 
 	/* if the slave address is ten bit address, enable 10BITADDR */
-	ic_con = readl(dev->base + DW_IC_CON);
+	ic_con = i2c_dw_readl(dev, 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);
+	i2c_dw_writel(dev, ic_con, DW_IC_CON);
 
 	/* Enable the adapter */
-	writel(1, dev->base + DW_IC_ENABLE);
+	i2c_dw_writel(dev, 1, DW_IC_ENABLE);
 
 	/* Enable interrupts */
-	writel(DW_IC_INTR_DEFAULT_MASK, dev->base + DW_IC_INTR_MASK);
+	i2c_dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
 }
 
 /*
@@ -419,15 +429,15 @@ 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);
+		tx_limit = dev->tx_fifo_depth - i2c_dw_readl(dev, DW_IC_TXFLR);
+		rx_limit = dev->rx_fifo_depth - i2c_dw_readl(dev, 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);
+				i2c_dw_writel(dev, 0x100, DW_IC_DATA_CMD);
 				rx_limit--;
 			} else
-				writel(*buf++, dev->base + DW_IC_DATA_CMD);
+				i2c_dw_writel(dev, *buf++, DW_IC_DATA_CMD);
 			tx_limit--; buf_len--;
 		}
 
@@ -452,7 +462,7 @@ i2c_dw_xfer_msg(struct dw_i2c_dev *dev)
 	if (dev->msg_err)
 		intr_mask = 0;
 
-	writel(intr_mask, dev->base + DW_IC_INTR_MASK);
+	i2c_dw_writel(dev, intr_mask, DW_IC_INTR_MASK);
 }
 
 static void
@@ -476,10 +486,10 @@ i2c_dw_read(struct dw_i2c_dev *dev)
 			buf = dev->rx_buf;
 		}
 
-		rx_valid = readl(dev->base + DW_IC_RXFLR);
+		rx_valid = i2c_dw_readl(dev, DW_IC_RXFLR);
 
 		for (; len > 0 && rx_valid > 0; len--, rx_valid--)
-			*buf++ = readl(dev->base + DW_IC_DATA_CMD);
+			*buf++ = i2c_dw_readl(dev, DW_IC_DATA_CMD);
 
 		if (len > 0) {
 			dev->status |= STATUS_READ_IN_PROGRESS;
@@ -562,7 +572,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, st
 	/* no error */
 	if (likely(!dev->cmd_err)) {
 		/* Disable the adapter */
-		writel(0, dev->base + DW_IC_ENABLE);
+		i2c_dw_writel(dev, 0, DW_IC_ENABLE);
 		ret = num;
 		goto done;
 	}
@@ -606,7 +616,7 @@ static u32 i2c_dw_read_clear_intrbits(st
 	 *
 	 * The raw version might be useful for debugging purposes.
 	 */
-	stat = readl(dev->base + DW_IC_INTR_STAT);
+	stat = i2c_dw_readl(dev, DW_IC_INTR_STAT);
 
 	/*
 	 * Do not use the IC_CLR_INTR register to clear interrupts, or
@@ -616,31 +626,31 @@ static u32 i2c_dw_read_clear_intrbits(st
 	 * Instead, use the separately-prepared IC_CLR_* registers.
 	 */
 	if (stat & DW_IC_INTR_RX_UNDER)
-		readl(dev->base + DW_IC_CLR_RX_UNDER);
+		i2c_dw_readl(dev, DW_IC_CLR_RX_UNDER);
 	if (stat & DW_IC_INTR_RX_OVER)
-		readl(dev->base + DW_IC_CLR_RX_OVER);
+		i2c_dw_readl(dev, DW_IC_CLR_RX_OVER);
 	if (stat & DW_IC_INTR_TX_OVER)
-		readl(dev->base + DW_IC_CLR_TX_OVER);
+		i2c_dw_readl(dev, DW_IC_CLR_TX_OVER);
 	if (stat & DW_IC_INTR_RD_REQ)
-		readl(dev->base + DW_IC_CLR_RD_REQ);
+		i2c_dw_readl(dev, DW_IC_CLR_RD_REQ);
 	if (stat & DW_IC_INTR_TX_ABRT) {
 		/*
 		 * The IC_TX_ABRT_SOURCE register is cleared whenever
 		 * the IC_CLR_TX_ABRT is read.  Preserve it beforehand.
 		 */
-		dev->abort_source = readl(dev->base + DW_IC_TX_ABRT_SOURCE);
-		readl(dev->base + DW_IC_CLR_TX_ABRT);
+		dev->abort_source = i2c_dw_readl(dev, DW_IC_TX_ABRT_SOURCE);
+		i2c_dw_readl(dev, DW_IC_CLR_TX_ABRT);
 	}
 	if (stat & DW_IC_INTR_RX_DONE)
-		readl(dev->base + DW_IC_CLR_RX_DONE);
+		i2c_dw_readl(dev, DW_IC_CLR_RX_DONE);
 	if (stat & DW_IC_INTR_ACTIVITY)
-		readl(dev->base + DW_IC_CLR_ACTIVITY);
+		i2c_dw_readl(dev, DW_IC_CLR_ACTIVITY);
 	if (stat & DW_IC_INTR_STOP_DET)
-		readl(dev->base + DW_IC_CLR_STOP_DET);
+		i2c_dw_readl(dev, DW_IC_CLR_STOP_DET);
 	if (stat & DW_IC_INTR_START_DET)
-		readl(dev->base + DW_IC_CLR_START_DET);
+		i2c_dw_readl(dev, DW_IC_CLR_START_DET);
 	if (stat & DW_IC_INTR_GEN_CALL)
-		readl(dev->base + DW_IC_CLR_GEN_CALL);
+		i2c_dw_readl(dev, DW_IC_CLR_GEN_CALL);
 
 	return stat;
 }
@@ -665,7 +675,7 @@ static irqreturn_t i2c_dw_isr(int this_i
 		 * Anytime TX_ABRT is set, the contents of the tx/rx
 		 * buffers are flushed.  Make sure to skip them.
 		 */
-		writel(0, dev->base + DW_IC_INTR_MASK);
+		i2c_dw_writel(dev, 0, DW_IC_INTR_MASK);
 		goto tx_aborted;
 	}
 
@@ -746,14 +756,14 @@ static int __devinit dw_i2c_probe(struct
 		goto err_unuse_clocks;
 	}
 	{
-		u32 param1 = readl(dev->base + DW_IC_COMP_PARAM_1);
+		u32 param1 = i2c_dw_readl(dev, DW_IC_COMP_PARAM_1);
 
 		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
 		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
 	}
 	i2c_dw_init(dev);
 
-	writel(0, dev->base + DW_IC_INTR_MASK); /* disable IRQ */
+	i2c_dw_writel(dev, 0, DW_IC_INTR_MASK); /* disable IRQ */
 	r = request_irq(dev->irq, i2c_dw_isr, IRQF_DISABLED, pdev->name, dev);
 	if (r) {
 		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
@@ -809,7 +819,7 @@ static int __devexit dw_i2c_remove(struc
 	clk_put(dev->clk);
 	dev->clk = NULL;
 
-	writel(0, dev->base + DW_IC_ENABLE);
+	i2c_dw_writel(dev, 0, DW_IC_ENABLE);
 	free_irq(dev->irq, dev);
 	kfree(dev);
 

A message that is not in the archive (sent off-list or not yet received): [email protected]

#3 lore raw

Re: [PATCH 1/3] Use local version of readl & writel

Quoted 1 line · Jean-Hugues Deschenes

Use local versions of readl & writel, so per-access manipulations may be performed

Let's put Signed-off-by: here.

Quoted 9 lines
---
 drivers/i2c/busses/i2c-designware.c |   88 ++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 39 deletions(-)

Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================

drivers/i2c/busses/i2c-designware.c struct dw_i2c_dev { · line 225

Quoted 7 lines
drivers/i2c/busses/i2c-designware.c +4 -0
@@ -219,6 +219,16 @@ struct dw_i2c_dev {
 	unsigned int		rx_fifo_depth;
 };
 
+static u32 i2c_dw_readl(struct dw_i2c_dev *dev, int addr)
+{
+	return readl(dev->base + addr);
+}

nit: As being used with dev->base, "offset", "reg" or something like that would be nice, rather than "addr".

drivers/i2c/busses/i2c-designware.c struct dw_i2c_dev { · line 234

Quoted 8 lines
drivers/i2c/busses/i2c-designware.c +5 -0
+static void i2c_dw_writel(struct dw_i2c_dev *dev, u32 b, int addr)
+{
+	writel(b, dev->base + addr);
+}
+
 static u32
 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {

Since these are I/O accessros / wrappers, and don't provide any meaningful functions, how about simply dw_readl() / dw_writel(), similarly as other {read,write}[bwl] wrappers do. I'd prefer a reasonably shorter name for this kind of accessors!

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics

A message that is not in the archive (sent off-list or not yet received): [email protected]

#5 lore raw

Re: [PATCH 1/3] Use local version of readl & writel

Quoted 4 lines · Shinya Kuribayashi
Quoted 1 line · Jean-Hugues Deschenes

Use local versions of readl & writel, so per-access manipulations may be performed

Let's put Signed-off-by: here.

My apologies; I need to figure out where in the process that was droppted...

Quoted 14 lines
Quoted 14 lines
---
 drivers/i2c/busses/i2c-designware.c |   88 
++++++++++++++++++++----------------
 1 file changed, 49 insertions(+), 39 deletions(-)

Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================

drivers/i2c/busses/i2c-designware.c +0 -0
@@ -219,6 +219,16 @@ struct dw_i2c_dev {
     unsigned int        rx_fifo_depth;
 };
 
Quoted 7 lines

drivers/i2c/busses/i2c-designware.c struct dw_i2c_dev { · line 225

Quoted 4 lines
drivers/i2c/busses/i2c-designware.c +4 -0
+static u32 i2c_dw_readl(struct dw_i2c_dev *dev, int addr)
+{
+    return readl(dev->base + addr);
+}

nit: As being used with dev->base, "offset", "reg" or something like that would be nice, rather than "addr".

Good idea; It'll be in the updated patch...

Quoted 14 lines

drivers/i2c/busses/i2c-designware.c struct dw_i2c_dev { · line 234

Quoted 9 lines
drivers/i2c/busses/i2c-designware.c +5 -0
+static void i2c_dw_writel(struct dw_i2c_dev *dev, u32 b, int addr)
+{
+    writel(b, dev->base + addr);
+}
+
 static u32
 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset)
 {

Since these are I/O accessros / wrappers, and don't provide any meaningful functions, how about simply dw_readl() / dw_writel(), similarly as other {read,write}[bwl] wrappers do. I'd prefer a reasonably shorter name for this kind of accessors!

... That should help with the code's readeability as well. Will be in the next patch

jh

#6 lore raw

PATCH 2/3 Check component type register

In reply to Jean-Hugues Deschenes

Designware component type register is checked before attaching to the device.

---
 drivers/i2c/busses/i2c-designware.c |   11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/i2c/busses/i2c-designware.c +11 -0
Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================
@@ -68,6 +68,7 @@
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
 #define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_COMP_TYPE		0xfc
 #define DW_IC_TX_ABRT_SOURCE	0x80
 
 #define DW_IC_CON_MASTER		0x1
@@ -756,6 +757,16 @@ static int __devinit dw_i2c_probe(struct
 		goto err_unuse_clocks;
 	}
 	{
+		u32 comp_type = i2c_dw_readl(dev, DW_IC_COMP_TYPE);
+
+		if (0x44570140 != comp_type) {
+			dev_err(&pdev->dev, "Unknown Synopsys component type: "
+					"0x%08x\n",	comp_type);
+			r = -ENODEV;
+			goto err_iounmap;
+		}
+	}
+	{
 		u32 param1 = i2c_dw_readl(dev, DW_IC_COMP_PARAM_1);
 
 		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw

Re: [PATCH 2/3] Check component type register

Quoted 22 lines · Jean-Hugues Deschenes

Designware component type register is checked before attaching to the device.

---
 drivers/i2c/busses/i2c-designware.c |   11 +++++++++++
 1 file changed, 11 insertions(+)

Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================

drivers/i2c/busses/i2c-designware.c +1 -0
@@ -68,6 +68,7 @@
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
 #define DW_IC_COMP_PARAM_1	0xf4
+#define DW_IC_COMP_TYPE		0xfc
 #define DW_IC_TX_ABRT_SOURCE	0x80
 
 #define DW_IC_CON_MASTER		0x1
@@ -756,6 +757,16 @@ static int __devinit dw_i2c_probe(struct
 		goto err_unuse_clocks;
 	}
 	{

drivers/i2c/busses/i2c-designware.c static int __devinit dw_i2c_probe(struct · line 769

Quoted 10 lines
drivers/i2c/busses/i2c-designware.c +10 -0
+		u32 comp_type = i2c_dw_readl(dev, DW_IC_COMP_TYPE);
+
+		if (0x44570140 != comp_type) {
+			dev_err(&pdev->dev, "Unknown Synopsys component type: "
+					"0x%08x\n",	comp_type);
+			r = -ENODEV;
+			goto err_iounmap;
+		}
+	}
+	{

The general convention in linux is to do (if x = constant).

Quoted 5 lines
drivers/i2c/busses/i2c-designware.c +0 -0
 		u32 param1 = i2c_dw_readl(dev, DW_IC_COMP_PARAM_1);
 
 		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
Signature · 1 line
-- 
Signature · 5 lines
-- 
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

A message that is not in the archive (sent off-list or not yet received): 20100114011101.GK3738-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]

#10 lore raw

Re: [PATCH 2/3] Check component type register

Quoted 13 lines

drivers/i2c/busses/i2c-designware.c static int __devinit dw_i2c_probe(struct · line 769

Quoted 10 lines
drivers/i2c/busses/i2c-designware.c +8 -0
+		if (0x44570140 != comp_type) {
+			dev_err(&pdev->dev, "Unknown Synopsys component type: "
+					"0x%08x\n",	comp_type);
+			r = -ENODEV;
+			goto err_iounmap;
+		}
+	}
+	{

The general convention in linux is to do (if x = constant).

Old coding habits are difficult to get rid of ;-). I'll change it in the updated patchset, once all comments have been collected.

thanks,
jh

#11 lore raw

PATCH 3/3 Allow mixed endianness accesses

In reply to Jean-Hugues Deschenes

Allows CPUs of a given endianness to access a dw controller of a different endianness. Endianncess difference is detected at run time through the dw component type register.

---
 drivers/i2c/busses/i2c-designware.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)
drivers/i2c/busses/i2c-designware.c +15 -2
Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================
@@ -36,6 +36,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/swab.h>
 
 /*
  * Registers offset
@@ -192,6 +193,7 @@ static char *abort_sources[] = {
  * @status: i2c master status, one of STATUS_*
  * @abort_source: copy of the TX_ABRT_SOURCE register
  * @irq: interrupt number for the i2c master
+ * @swab: true if the instantiated IP is of different endianess
  * @adapter: i2c subsystem adapter node
  * @tx_fifo_depth: depth of the hardware tx fifo
  * @rx_fifo_depth: depth of the hardware rx fifo
@@ -215,6 +217,7 @@ struct dw_i2c_dev {
 	unsigned int		status;
 	u32			abort_source;
 	int			irq;
+	int			swab;
 	struct i2c_adapter	adapter;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
@@ -222,11 +225,19 @@ struct dw_i2c_dev {
 
 static u32 i2c_dw_readl(struct dw_i2c_dev *dev, int addr)
 {
-	return readl(dev->base + addr);
+	u32 value = readl(dev->base + addr);
+
+	if (dev->swab)
+		return swab32(value);
+	else
+		return value;
 }
 
 static void i2c_dw_writel(struct dw_i2c_dev *dev, u32 b, int addr)
 {
+	if (dev->swab)
+		b = swab32(b);
+
 	writel(b, dev->base + addr);
 }
 
@@ -759,7 +770,9 @@ static int __devinit dw_i2c_probe(struct
 	{
 		u32 comp_type = i2c_dw_readl(dev, DW_IC_COMP_TYPE);
 
-		if (0x44570140 != comp_type) {
+		if (___constant_swab32(0x44570140) == comp_type)
+			dev->swab = 1;
+		else if (0x44570140 != comp_type) {
 			dev_err(&pdev->dev, "Unknown Synopsys component type: "
 					"0x%08x\n",	comp_type);
 			r = -ENODEV;

A message that is not in the archive (sent off-list or not yet received): [email protected]

#13 lore raw

Re: [PATCH 3/3] Allow mixed endianness accesses

Quoted 13 lines · Jean-Hugues Deschenes

Allows CPUs of a given endianness to access a dw controller of a different endianness. Endianncess difference is detected at run time through the dw component type register.

---
 drivers/i2c/busses/i2c-designware.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================

drivers/i2c/busses/i2c-designware.c line 42

Quoted 7 lines
drivers/i2c/busses/i2c-designware.c +1 -0
@@ -36,6 +36,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/swab.h>
 
 /*
  * Registers offset

I'm working on 2.6.27 MIPS kernel, and this <linux/swab.h> brings the following errors.

CC drivers/i2c/busses/i2c-designware.o
In file included from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:39:
/home/skuribay/git/linux/include/linux/swab.h:109:1: warning: "__swab16" redefined In file included from /home/skuribay/git/linux/include/linux/byteorder/big_endian.h:12,

                from include2/asm/byteorder.h:69,
                from include2/asm/bitops.h:21,
                from /home/skuribay/git/linux/include/linux/bitops.h:17,
                from /home/skuribay/git/linux/include/linux/kernel.h:15,
                from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:28:

/home/skuribay/git/linux/include/linux/byteorder/swab.h:144:1: warning: this is the location of the previous definition In file included from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:39:
/home/skuribay/git/linux/include/linux/swab.h:118:1: warning: "__swab32" redefined
In file included from /home/skuribay/git/linux/include/linux/byteorder/big_endian.h:12,

                from include2/asm/byteorder.h:69,
                from include2/asm/bitops.h:21,
                from /home/skuribay/git/linux/include/linux/bitops.h:17,
                from /home/skuribay/git/linux/include/linux/kernel.h:15,
                from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:28:

/home/skuribay/git/linux/include/linux/byteorder/swab.h:148:1: warning: this is the location of the previous definition In file included from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:39:
/home/skuribay/git/linux/include/linux/swab.h:127:1: warning: "__swab64" redefined
In file included from /home/skuribay/git/linux/include/linux/byteorder/big_endian.h:12,

                from include2/asm/byteorder.h:69,
                from include2/asm/bitops.h:21,
                from /home/skuribay/git/linux/include/linux/bitops.h:17,
                from /home/skuribay/git/linux/include/linux/kernel.h:15,
                from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:28:

/home/skuribay/git/linux/include/linux/byteorder/swab.h:152:1: warning: this is the location of the previous definition In file included from /home/skuribay/git/linux/drivers/i2c/busses/i2c-designware.c:40:
/home/skuribay/git/linux/include/linux/swab.h:46: error: redefinition of '___swab16' /home/skuribay/git/linux/include/linux/byteorder/swab.h:65: error: previous definition of '___swab16' was here /home/skuribay/git/linux/include/linux/swab.h:57: error: redefinition of '___swab32' /home/skuribay/git/linux/include/linux/byteorder/swab.h:69: error: previous definition of '___swab32' was here /home/skuribay/git/linux/include/linux/swab.h:68: error: redefinition of '___swab64' /home/skuribay/git/linux/include/linux/byteorder/swab.h:75: error: previous definition of '___swab64' was here /home/skuribay/git/linux/include/linux/swab.h:158: error: redefinition of '__swab16p' /home/skuribay/git/linux/include/linux/byteorder/swab.h:168: error: previous definition of '__swab16p' was here /home/skuribay/git/linux/include/linux/swab.h:171: error: redefinition of '__swab32p' /home/skuribay/git/linux/include/linux/byteorder/swab.h:181: error: previous definition of '__swab32p' was here /home/skuribay/git/linux/include/linux/swab.h:184: error: redefinition of '__swab64p' /home/skuribay/git/linux/include/linux/byteorder/swab.h:201: error: previous definition of '__swab64p' was here /home/skuribay/git/linux/include/linux/swab.h:227: error: redefinition of '__swab16s' /home/skuribay/git/linux/include/linux/byteorder/swab.h:172: error: previous definition of '__swab16s' was here /home/skuribay/git/linux/include/linux/swab.h:239: error: redefinition of '__swab32s' /home/skuribay/git/linux/include/linux/byteorder/swab.h:185: error: previous definition of '__swab32s' was here /home/skuribay/git/linux/include/linux/swab.h:252: error: redefinition of '__swab64s' /home/skuribay/git/linux/include/linux/byteorder/swab.h:205: error: previous definition of '__swab64s' was here

I don't have the latest kernel which could make i2c-designware.c driver built-in right now, so I'm not sure it works or not there. I hope this works with the latest LMO tree.

Quoted 10 lines
drivers/i2c/busses/i2c-designware.c +5 -1
@@ -222,11 +225,19 @@ struct dw_i2c_dev {
 
 static u32 i2c_dw_readl(struct dw_i2c_dev *dev, int addr)
 {
-	return readl(dev->base + addr);
+	u32 value = readl(dev->base + addr);
+
+	if (dev->swab)
+		return swab32(value);
+	else

drivers/i2c/busses/i2c-designware.c struct dw_i2c_dev { · line 242

Quoted 10 lines
drivers/i2c/busses/i2c-designware.c +4 -0
+		return value;
 }
 
 static void i2c_dw_writel(struct dw_i2c_dev *dev, u32 b, int addr)
 {
+	if (dev->swab)
+		b = swab32(b);
+
 	writel(b, dev->base + addr);
 }

Now I understand the background of the patch, so my question is, is this worth run-time probed? In this case, DW IP's endian is different from CPU endian, and which can not be resolved via io/swab settings in any way, that's fine.

Then I wonder is there any way to statically optimize them?

Note that I'm not objecting against this patch, just would like to search for a better way if available!

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics

A message that is not in the archive (sent off-list or not yet received): [email protected]

#15 lore raw

Re: [PATCH 3/3] Allow mixed endianness accesses

Quoted 19 lines · Shinya Kuribayashi
Quoted 18 lines · Jean-Hugues Deschenes

Allows CPUs of a given endianness to access a dw controller of a different endianness. Endianncess difference is detected at run time through the dw component type register.

---
 drivers/i2c/busses/i2c-designware.c |   17 +++++++++++++++--
 1 file changed, 15 insertions(+), 2 deletions(-)

Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================

drivers/i2c/busses/i2c-designware.c +1 -0
@@ -36,6 +36,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/swab.h>
 
Quoted 6 lines

drivers/i2c/busses/i2c-designware.c line 42

Quoted 2 lines
drivers/i2c/busses/i2c-designware.c +0 -0
 /*
  * Registers offset

I'm working on 2.6.27 MIPS kernel
so I'm not sure it works or not there.
I hope this works with the latest LMO tree.

I'll download a MIPS toolchain and give it a try.

Quoted 6 lines

Now I understand the background of the patch, so my question is, is this worth run-time probed? In this case, DW IP's endian is different from CPU endian, and which can not be resolved via io/swab settings in any way, that's fine.

Then I wonder is there any way to statically optimize them?

Humm... Configuration options (controlling #ifdefs) are a possibility, although I must admit I'm not so fond of this option, because it's error prone.

I believe the problem is not with the automatic detection per se (which is done at init, where performance is not so much an issue), but with the extra IFs at each memory access... There, the other option I can think of is having 2 sets of accessors, dw_readl_[bl]e()/dw_writel_[bl]e() and have them accessed via function pointers (set at init, following the auto-detection). But I'm not sure that loosing the option of inlining the accessor funtion (since they would now have to be accessed via function pointers) would really result in a performance gain...

jh

A message that is not in the archive (sent off-list or not yet received): [email protected]

#17 lore raw

Re: [PATCH 3/3] Allow mixed endianness accesses

Quoted 3 lines · Jean-Hugues Deschenes

I'll download a MIPS toolchain and give it a try.

I've built this driver with the patch as it has been submitted, for MIPS, on both the vanilla and LMO trees and there are no errors nor warnings. I noticed in your build output that the include files are taken fom an "include2" directory, rather than an "include" directory. Could that have something to do with it?

jh

#18 lore raw

Re: [PATCH 3/3] Allow mixed endianness accesses

In reply to a message not in the archive
Quoted 3 lines · Jean-Hugues Deschenes
Quoted 2 lines

so I'm not sure it works or not there.
I hope this works with the latest LMO tree.

I'll download a MIPS toolchain and give it a try.

Thanks. I've also checked it builds with the latest LMO tree.

Quoted 2 lines
Quoted 1 line

Then I wonder is there any way to statically optimize them?

Humm... Configuration options (controlling #ifdefs) are a possibility, although I must admit I'm not so fond of this option, because it's error prone.

Ok, fine with me.

Quoted 1 line

I believe the problem is not with the automatic detection per se (which is done at init, where performance is not so much an issue), but with the extra IFs at each memory access...

Exactly, that's what I'm concerned about!

Quoted 1 line

There, the other option I can think of is having 2 sets of accessors, dw_readl_[bl]e()/dw_writel_[bl]e() and have them accessed via function pointers (set at init, following the auto-detection). But I'm not sure that loosing the option of inlining the accessor funtion (since they would now have to be accessed via function pointers) would really result in a performance gain...

Looking around more, and now I'm not going to be nervous about I/O cost, as I2C is enough slow. It would be nice we could have statically optimized one of course, but is not indispensable.

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics

A message that is not in the archive (sent off-list or not yet received): [email protected]

#20 lore raw

Re: [PATCH 3/3] Allow mixed endianness accesses

Hello Shinya,

Quoted 2 lines · Shinya Kuribayashi

Looking around more, and now I'm not going to be nervous about I/O cost, as I2C is enough slow.

... That was my thinking.

thanks for your comments,

jh

A message that is not in the archive (sent off-list or not yet received): [email protected]

#22 lore raw
Quoted 3 lines · Jean-Hugues Deschenes

Here is patchset for the i2c-designware driver, allowing little endian machines to use the DW IP instantiated in big endian and vice versa.

In the commit ed5e1dd5 (i2c-designware: Consolidate to use 32-bit word accesses), I tried to sort out the I/O endian issue, but it seems doesn't work for your environment, R/W data is swapped :-(

Signature · 2 lines
Shinya Kuribayashi
NEC Electronics

A message that is not in the archive (sent off-list or not yet received): [email protected]

#24 lore raw
Quoted 8 lines · Shinya Kuribayashi
Quoted 3 lines · Jean-Hugues Deschenes

Here is patchset for the i2c-designware driver, allowing little endian machines to use the DW IP instantiated in big endian and vice versa.

In the commit ed5e1dd5 (i2c-designware: Consolidate to use 32-bit word accesses), I tried to sort out the I/O endian issue, but it seems doesn't work for your environment, R/W data is swapped :-(

I think this one of those weird cases where you have _both_ types of endianess of peripherals where anything in readl/writel isn't going to help you out.

Signature · 4 lines
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): 390831ED3DF58E41A3D2FB82591E2C36047AF627-Jh1kU4MlLDZ5rAAhGZPdxVaTQe2KTcn/@public.gmane.org

#27 lore raw
Quoted 13 lines · Jean-Hugues Deschenes
Quoted 7 lines
Quoted 3 lines

In the commit ed5e1dd5 (i2c-designware: Consolidate to use 32-bit word accesses), I tried to sort out the I/O endian issue, but it seems doesn't work for your environment, R/W data is swapped :-(

I think this one of those weird cases where you have _both_ types of endianess of peripherals where anything in readl/writel isn't going to help you out.

Actually, the specific case I'm interested in is one where I have an SOC built around an ARM core running in little-endian mode, in which the Designware IP has been instantiated and hooked up (to the internal data bus) in big endian format.

Quoted 4 lines

This patch could also come in handy, for example, if someone were to run an armeb kernel on an SOC where the DW IP has been instantiated and hooked up in little endian format (as is probably the case for most ARM-based SOCs out there).

I think this is currently an oditity, most ARM systems are still armel, especiailly since very few distributions do armeb

Signature · 4 lines
Ben

Q:      What's a light-year?
A:      One-third less calories than a regular year.

A message that is not in the archive (sent off-list or not yet received): 20100114064501.GR3738-SMNkleLxa3Z6Wcw2j4pizdi2O/[email protected]

#29 lore raw
Quoted 6 lines
Quoted 6 lines

Actually, the specific case I'm interested in is one where I have an SOC built around an ARM core running in little-endian mode, in which the Designware IP has been instantiated and hooked up (to the internal data bus) in big endian format.

Quoted 9 lines
Quoted 5 lines

This patch could also come in handy, for example, if someone were to run an armeb kernel on an SOC where the DW IP has been instantiated and hooked up in little endian format (as is probably the case for most ARM-based SOCs out there).

I think this is currently an oditity, most ARM systems are still armel, especiailly since very few distributions do armeb

For the latter case, I definitely agree with you. Still, this patchset solves a problem which is very real (for me!) where I'm running an armel build and interfacing to a big endian DW I2C device.

jh

Acked-by
#30 lore raw
In reply to a message not in the archive

Hi Jean-Hugues,

Quoted 5 lines · Jean-Hugues Deschenes

Hi Baruch and Ben,

Here is patchset for the i2c-designware driver, allowing little endian machines to use the DW IP instantiated in big endian and vice versa.

baruch

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - [email protected] - tel: +972.2.679.5364, http://www.tkos.co.il -