[PATCH 0/3] i2c-designware: Allow mixed endianness

[PATCH 0/3] i2c-designware: Allow mixed endianness

[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.


-- 

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

[Jean-Hugues Deschenes]

[-- Attachment #1: use_local_readl_writel --]
[-- Type: text/plain, Size: 8328 bytes --]

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(-)

Index: linux-2.6_i2c/drivers/i2c/busses/i2c-designware.c
===================================================================
--- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
+++ 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);
 

-- 

[PATCH 2/3] Check component type register

[Jean-Hugues Deschenes]

[-- Attachment #1: check_dw_component --]
[-- Type: text/plain, Size: 1060 bytes --]

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
===================================================================
--- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
+++ 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;

-- 

[PATCH 3/3] Allow mixed endianness accesses

[Jean-Hugues Deschenes]

[-- Attachment #1: allow_mixed_endianness --]
[-- Type: text/plain, Size: 2076 bytes --]

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
===================================================================
--- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
+++ 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;

-- 

Re: [PATCH 0/3] i2c-designware: Allow mixed endianness

[Shinya Kuribayashi]

Jean-Hugues Deschenes wrote:
> 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 :-(

-- 
Shinya Kuribayashi
NEC Electronics

Re: [PATCH 0/3] i2c-designware: Allow mixed endianness

[Ben Dooks]

On Thu, Jan 14, 2010 at 09:40:53AM +0900, Shinya Kuribayashi wrote:
> Jean-Hugues Deschenes wrote:
>> 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.

-- 
Ben

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

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

[Ben Dooks]

On Wed, Jan 13, 2010 at 02:32:26PM -0500, Jean-Hugues Deschenes wrote:
> 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
> ===================================================================
> --- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
> +++ 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;
> +		}
> +	}
> +	{

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

>  		u32 param1 = i2c_dw_readl(dev, DW_IC_COMP_PARAM_1);
>  
>  		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> 
> -- 

-- 
-- 
Ben

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

Re: [PATCH 0/3] i2c-designware: Allow mixed endianness

[Ben Dooks]

On Wed, Jan 13, 2010 at 10:06:31PM -0500, Jean-Hugues Deschenes wrote:
> > > 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.
> 
> 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

-- 
Ben

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

Re: [PATCH 0/3] i2c-designware: Allow mixed endianness

[Baruch Siach]

Hi Jean-Hugues,

On Wed, Jan 13, 2010 at 02:32:24PM -0500, Jean-Hugues Deschenes wrote:
> 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.

Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

baruch

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org - tel: +972.2.679.5364, http://www.tkos.co.il -

Re: [PATCH 0/3] i2c-designware: Allow mixed endianness

[Jean-Hugues Deschenes]

>> 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.
>>
>> 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

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

[Jean-Hugues Deschenes]

>> +		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

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

[Shinya Kuribayashi]

Jean-Hugues Deschenes wrote:
> Use local versions of readl & writel, so per-access manipulations may be performed

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

> ---
>  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
> ===================================================================
> --- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
> +++ 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);
> +}

nit: As being used with dev->base, "offset", "reg" or something
like that would be nice, rather than "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)
>  {

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!

-- 
Shinya Kuribayashi
NEC Electronics

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

[Shinya Kuribayashi]

Jean-Hugues Deschenes wrote:
> 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
> ===================================================================
> --- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
> +++ 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

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.

> @@ -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);
>  }

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!

-- 
Shinya Kuribayashi
NEC Electronics

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

[Jean-Hugues Deschenes]

Shinya Kuribayashi wrote:
> Jean-Hugues Deschenes wrote:
>> 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...
>
>> ---
>>  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
>> ===================================================================
>> --- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
>> +++ 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);
>> +}
>
> 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...
>
>> +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

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

[Jean-Hugues Deschenes]

Shinya Kuribayashi wrote:
> Jean-Hugues Deschenes wrote:
>> 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
>> ===================================================================
>> --- linux-2.6_i2c.orig/drivers/i2c/busses/i2c-designware.c
>> +++ 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
>
> 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.
> 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

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

[Jean-Hugues Deschenes]

Jean-Hugues Deschenes wrote:
>
> 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

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

[Shinya Kuribayashi]

Jean-Hugues Deschenes wrote:
>> 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.

>> 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.

> 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!

> 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.

-- 
Shinya Kuribayashi
NEC Electronics

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

[Jean-Hugues Deschenes]

Hello Shinya,

Shinya Kuribayashi wrote:
> 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