i2c-designware: Allow mixed endianness
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.
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
@@ -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_deu32 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_de47 ,/* 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_de13 ,/* 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_iu32 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(strucclk_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]
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]
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
PATCH 2/3 Check component type register
In reply to Jean-Hugues DeschenesDesignware 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
@@ -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]
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
-0u32 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]
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
PATCH 3/3 Allow mixed endianness accesses
In reply to Jean-Hugues DeschenesAllows 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
@@ -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]
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]
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]
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
Re: [PATCH 3/3] Allow mixed endianness accesses
In reply to a message not in the archiveQuoted 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]
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]
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]
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
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]
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
Baruch Siach
Acked-byHi 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 -