Re: [PATCH 2/4] i2c: imx: only imx1 needs disable delay
On Thu, Oct 01, 2009 at 09:13:31AM +0800, Richard Zhao wrote: check cpu_is_mx1() when set disable_delay. Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 156cc95..c1e541c 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -254,14 +254,16 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, /* Write divider value to register */ writeb(i2c_clk_div[i][1], i2c_imx-base + IMX_I2C_IFDR); - /* - * There dummy delay is calculated. - * It should be about one I2C clock period long. - * This delay is used in I2C bus disable function - * to fix chip hardware bug. - */ - i2c_imx-disable_delay = (50U * i2c_clk_div[i][0] - + (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2); + if (cpu_is_mx1()) { + /* + * There dummy delay is calculated. + * It should be about one I2C clock period long. + * This delay is used in I2C bus disable function + * to fix chip hardware bug. + */ + i2c_imx-disable_delay = (50U * i2c_clk_div[i][0] + + (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2); + } I think you should put the udelay(i2c_imx-disable_delay) in cpu_is_mx1() rather than the calculation. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] i2c: imx: add macros and printk to make debug easy
On Thu, Oct 01, 2009 at 09:13:32AM +0800, Richard Zhao wrote: When CONFIG_I2C_DEBUG_BUS is enabled, it helps dump registers at operation fail condition, and print i2c_msg to xfer. Signed-off-by: Richard Zhao linux...@gmail.com Honestly I don't think we need this. It makes the driver too verbose for my taste. Sascha diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index c1e541c..87faea4 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -125,6 +125,20 @@ struct imx_i2c_struct { /** Functions for IMX I2C adapter driver *** ***/ +#ifdef CONFIG_I2C_DEBUG_BUS +#define reg_dump(i2c_imx) \ +{ \ + printk(KERN_DEBUG fun %s:%d , __func__, __LINE__); \ + printk(KERN_DEBUG IADR %02x IFDR %02x I2CR %02x I2SR %02x\n, \ + readb(i2c_imx-base + IMX_I2C_IADR), \ + readb(i2c_imx-base + IMX_I2C_IFDR), \ + readb(i2c_imx-base + IMX_I2C_I2CR), \ + readb(i2c_imx-base + IMX_I2C_I2SR)); \ +} +#else +#define reg_dump(i2c_imx) +#endif + static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) { unsigned long orig_jiffies = jiffies; @@ -146,6 +160,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) if (time_after(jiffies, orig_jiffies + HZ / 1000)) { dev_dbg(i2c_imx-adapter.dev, %s I2C bus is busy\n, __func__); + reg_dump(i2c_imx); return -EIO; } schedule(); @@ -164,9 +179,11 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx) if (unlikely(result 0)) { dev_dbg(i2c_imx-adapter.dev, %s result 0\n, __func__); + reg_dump(i2c_imx); return result; } else if (unlikely(!(i2c_imx-i2csr I2SR_IIF))) { dev_dbg(i2c_imx-adapter.dev, %s Timeout\n, __func__); + reg_dump(i2c_imx); return -ETIMEDOUT; } dev_dbg(i2c_imx-adapter.dev, %s TRX complete\n, __func__); @@ -178,6 +195,7 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) { if (readb(i2c_imx-base + IMX_I2C_I2SR) I2SR_RXAK) { dev_dbg(i2c_imx-adapter.dev, %s No ACK\n, __func__); + reg_dump(i2c_imx); return -EIO; /* No ACK */ } @@ -197,17 +215,20 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); result = i2c_imx_bus_busy(i2c_imx, 0); - if (result) + if (result) { + reg_dump(i2c_imx); return result; + } /* Start I2C transaction */ temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_MSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); result = i2c_imx_bus_busy(i2c_imx, 1); - if (result) + if (result) { + reg_dump(i2c_imx); return result; - + } temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); return result; @@ -228,7 +249,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) */ udelay(i2c_imx-disable_delay); - i2c_imx_bus_busy(i2c_imx, 0); + if (i2c_imx_bus_busy(i2c_imx, 0)) + reg_dump(i2c_imx); /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); @@ -389,7 +411,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter); dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - +#ifdef CONFIG_I2C_DEBUG_BUS + for (i = 0; i num; i++) { + printk(KERN_DEBUG msg%d addr %02x RD %d cnt %d d:, i, + msgs[i].addr, msgs[i].flags I2C_M_RD, msgs[i].len); + if (!(msgs[i].flags I2C_M_RD)) { + int j; + for (j = 0; j msgs[i].len; j++) + printk(%02x , msgs[i].buf[j]); + } + printk(\n); + } +#endif /* Start I2C transfer */ result = i2c_imx_start(i2c_imx); if (result) @@ -404,8 +437,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, temp |= I2CR_RSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); result = i2c_imx_bus_busy(i2c_imx, 1); - if (result) + if (result) { + reg_dump(i2c_imx); goto fail0; + } } dev_dbg(i2c_imx-adapter.dev, %s transfer message: %d\n, __func__, i); @@ -562,7 +597,7 @@ static int __init i2c_imx_probe(struct
Re: [PATCH 4/4] i2c: imx: disable clock when it's possible to save power.
On Thu, Oct 01, 2009 at 09:13:33AM +0800, Richard Zhao wrote: Enable clock before START, disable it after STOP. The clk_diable/enable calls in suspend/resume should be removed also. Sascha Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 87faea4..72ddea3 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -120,6 +120,7 @@ struct imx_i2c_struct { wait_queue_head_t queue; unsigned long i2csr; unsigned intdisable_delay; + unsigned intifdr; /* IMX_I2C_IFDR */ }; /** Functions for IMX I2C adapter driver *** @@ -210,6 +211,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); + clk_enable(i2c_imx-clk); + writeb(i2c_imx-ifdr, i2c_imx-base + IMX_I2C_IFDR); /* Enable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); @@ -254,6 +257,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); + clk_disable(i2c_imx-clk); } static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, @@ -273,8 +277,8 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, else for (i = 0; i2c_clk_div[i][0] div; i++); - /* Write divider value to register */ - writeb(i2c_clk_div[i][1], i2c_imx-base + IMX_I2C_IFDR); + /* Store divider value */ + i2c_imx-ifdr = i2c_clk_div[i][1]; if (cpu_is_mx1()) { /* @@ -555,7 +559,6 @@ static int __init i2c_imx_probe(struct platform_device *pdev) dev_err(pdev-dev, can't get I2C clock\n); goto fail3; } - clk_enable(i2c_imx-clk); /* Request IRQ */ ret = request_irq(i2c_imx-irq, i2c_imx_isr, 0, pdev-name, i2c_imx); @@ -604,7 +607,6 @@ static int __init i2c_imx_probe(struct platform_device *pdev) fail5: free_irq(i2c_imx-irq, i2c_imx); fail4: - clk_disable(i2c_imx-clk); clk_put(i2c_imx-clk); fail3: release_mem_region(i2c_imx-res-start, resource_size(res)); @@ -641,8 +643,6 @@ static int __exit i2c_imx_remove(struct platform_device *pdev) if (pdata pdata-exit) pdata-exit(pdev-dev); - /* Disable I2C clock */ - clk_disable(i2c_imx-clk); clk_put(i2c_imx-clk); release_mem_region(i2c_imx-res-start, resource_size(i2c_imx-res)); -- 1.6.0.4 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] i2c: imx: disable clock when it's possible to save power.
On Thu, Oct 1, 2009 at 3:34 PM, Sascha Hauer s.ha...@pengutronix.de wrote: On Thu, Oct 01, 2009 at 09:13:33AM +0800, Richard Zhao wrote: Enable clock before START, disable it after STOP. The clk_diable/enable calls in suspend/resume should be removed also. Sascha Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 87faea4..72ddea3 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -120,6 +120,7 @@ struct imx_i2c_struct { wait_queue_head_t queue; unsigned long i2csr; unsigned int disable_delay; + unsigned int ifdr; /* IMX_I2C_IFDR */ }; /** Functions for IMX I2C adapter driver *** @@ -210,6 +211,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); + clk_enable(i2c_imx-clk); + writeb(i2c_imx-ifdr, i2c_imx-base + IMX_I2C_IFDR); /* Enable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); @@ -254,6 +257,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); + clk_disable(i2c_imx-clk); } static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, @@ -273,8 +277,8 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, else for (i = 0; i2c_clk_div[i][0] div; i++); - /* Write divider value to register */ - writeb(i2c_clk_div[i][1], i2c_imx-base + IMX_I2C_IFDR); + /* Store divider value */ + i2c_imx-ifdr = i2c_clk_div[i][1]; if (cpu_is_mx1()) { /* @@ -555,7 +559,6 @@ static int __init i2c_imx_probe(struct platform_device *pdev) dev_err(pdev-dev, can't get I2C clock\n); goto fail3; } - clk_enable(i2c_imx-clk); /* Request IRQ */ ret = request_irq(i2c_imx-irq, i2c_imx_isr, 0, pdev-name, i2c_imx); @@ -604,7 +607,6 @@ static int __init i2c_imx_probe(struct platform_device *pdev) fail5: free_irq(i2c_imx-irq, i2c_imx); fail4: - clk_disable(i2c_imx-clk); clk_put(i2c_imx-clk); fail3: release_mem_region(i2c_imx-res-start, resource_size(res)); @@ -641,8 +643,6 @@ static int __exit i2c_imx_remove(struct platform_device *pdev) if (pdata pdata-exit) pdata-exit(pdev-dev); - /* Disable I2C clock */ - clk_disable(i2c_imx-clk); clk_put(i2c_imx-clk); release_mem_region(i2c_imx-res-start, resource_size(i2c_imx-res)); -- 1.6.0.4 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | I don't think we have any suspend/resume routine. And we won't need it any more. Thanks Richard -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] i2c: imx: only imx1 needs disable delay
On Thu, Oct 1, 2009 at 3:26 PM, Sascha Hauer s.ha...@pengutronix.de wrote: On Thu, Oct 01, 2009 at 09:13:31AM +0800, Richard Zhao wrote: check cpu_is_mx1() when set disable_delay. Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 156cc95..c1e541c 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -254,14 +254,16 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, /* Write divider value to register */ writeb(i2c_clk_div[i][1], i2c_imx-base + IMX_I2C_IFDR); - /* - * There dummy delay is calculated. - * It should be about one I2C clock period long. - * This delay is used in I2C bus disable function - * to fix chip hardware bug. - */ - i2c_imx-disable_delay = (50U * i2c_clk_div[i][0] - + (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2); + if (cpu_is_mx1()) { + /* + * There dummy delay is calculated. + * It should be about one I2C clock period long. + * This delay is used in I2C bus disable function + * to fix chip hardware bug. + */ + i2c_imx-disable_delay = (50U * i2c_clk_div[i][0] + + (i2c_clk_rate / 2) - 1) / (i2c_clk_rate / 2); + } I think you should put the udelay(i2c_imx-disable_delay) in cpu_is_mx1() rather than the calculation. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | Do you think udelay(0) wast any more time than if()? And Could I get all you comments in a single patch loop? Thanks Richard -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] i2c: imx: disable clock when it's possible to save power.
On Thu, Oct 01, 2009 at 03:56:29PM +0800, Richard Zhao wrote: On Thu, Oct 1, 2009 at 3:34 PM, Sascha Hauer s.ha...@pengutronix.de wrote: On Thu, Oct 01, 2009 at 09:13:33AM +0800, Richard Zhao wrote: Enable clock before START, disable it after STOP. The clk_diable/enable calls in suspend/resume should be removed also. Sascha Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 87faea4..72ddea3 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -120,6 +120,7 @@ struct imx_i2c_struct { wait_queue_head_t queue; unsigned long i2csr; unsigned int disable_delay; + unsigned int ifdr; /* IMX_I2C_IFDR */ }; /** Functions for IMX I2C adapter driver *** @@ -210,6 +211,8 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); + clk_enable(i2c_imx-clk); + writeb(i2c_imx-ifdr, i2c_imx-base + IMX_I2C_IFDR); /* Enable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); @@ -254,6 +257,7 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); + clk_disable(i2c_imx-clk); } static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, @@ -273,8 +277,8 @@ static void __init i2c_imx_set_clk(struct imx_i2c_struct *i2c_imx, else for (i = 0; i2c_clk_div[i][0] div; i++); - /* Write divider value to register */ - writeb(i2c_clk_div[i][1], i2c_imx-base + IMX_I2C_IFDR); + /* Store divider value */ + i2c_imx-ifdr = i2c_clk_div[i][1]; if (cpu_is_mx1()) { /* @@ -555,7 +559,6 @@ static int __init i2c_imx_probe(struct platform_device *pdev) dev_err(pdev-dev, can't get I2C clock\n); goto fail3; } - clk_enable(i2c_imx-clk); /* Request IRQ */ ret = request_irq(i2c_imx-irq, i2c_imx_isr, 0, pdev-name, i2c_imx); @@ -604,7 +607,6 @@ static int __init i2c_imx_probe(struct platform_device *pdev) fail5: free_irq(i2c_imx-irq, i2c_imx); fail4: - clk_disable(i2c_imx-clk); clk_put(i2c_imx-clk); fail3: release_mem_region(i2c_imx-res-start, resource_size(res)); @@ -641,8 +643,6 @@ static int __exit i2c_imx_remove(struct platform_device *pdev) if (pdata pdata-exit) pdata-exit(pdev-dev); - /* Disable I2C clock */ - clk_disable(i2c_imx-clk); clk_put(i2c_imx-clk); release_mem_region(i2c_imx-res-start, resource_size(i2c_imx-res)); -- 1.6.0.4 -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | I don't think we have any suspend/resume routine. And we won't need it any more. Ups, my bad. I have it in my local tree and didn't realize it isn't upstream. Sascha -- Pengutronix e.K. | | Industrial Linux Solutions | http://www.pengutronix.de/ | Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0| Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917- | -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao linux...@gmail.com wrote: After START/RESTART, wait for busy bit to be set and after STOP, wait for busy bit to be clear. Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 4afba3e..156cc95 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -125,14 +125,19 @@ struct imx_i2c_struct { /** Functions for IMX I2C adapter driver *** ***/ -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) { unsigned long orig_jiffies = jiffies; + unsigned int temp; dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - /* wait for bus not busy */ - while (readb(i2c_imx-base + IMX_I2C_I2SR) I2SR_IBB) { + temp = readb(i2c_imx-base + IMX_I2C_I2SR); + while (1) { + if (for_busy (temp I2SR_IBB)) + break; + if (!for_busy !(temp I2SR_IBB)) + break; if (signal_pending(current)) { dev_dbg(i2c_imx-adapter.dev, %s I2C Interrupted\n, __func__); @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) return -EIO; } schedule(); + temp = readb(i2c_imx-base + IMX_I2C_I2SR); } return 0; @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) return 0; } -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) { unsigned int temp = 0; + int result; dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); /* Enable I2C controller */ + writeb(0, i2c_imx-base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); + + result = i2c_imx_bus_busy(i2c_imx, 0); + if (result) + return result; + /* Start I2C transaction */ temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_MSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); + if (result) + return result; + temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + return result; } static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Stop I2C transaction */ dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); temp = readb(i2c_imx-base + IMX_I2C_I2CR); - temp = ~I2CR_MSTA; + temp = ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx-base + IMX_I2C_I2CR); - /* setup chip registers to defaults */ - writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); - writeb(0, i2c_imx-base + IMX_I2C_I2SR); /* * This delay caused by an i.MXL hardware bug. * If no (or too short) delay, no STOP bit will be generated. */ udelay(i2c_imx-disable_delay); + + i2c_imx_bus_busy(i2c_imx, 0); + /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); } @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) dev_dbg(i2c_imx-adapter.dev, %s clear MSTA\n, __func__); temp = readb(i2c_imx-base + IMX_I2C_I2CR); - temp = ~I2CR_MSTA; + temp = ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx-base + IMX_I2C_I2CR); } else if (i == (msgs-len - 2)) { dev_dbg(i2c_imx-adapter.dev, @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - /* Check if i2c bus is not busy */ - result = i2c_imx_bus_busy(i2c_imx); + /* Start I2C transfer */ + result = i2c_imx_start(i2c_imx); if (result) goto fail0; - /* Start I2C transfer */ - i2c_imx_start(i2c_imx); - /* read/write data */ for (i = 0; i num; i++) { if (i) { @@ -386,6 +401,9 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_RSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); + if (result) + goto fail0; }
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 01, 2009 at 04:03:20PM +0800, Richard Zhao wrote: On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao linux...@gmail.com wrote: After START/RESTART, wait for busy bit to be set and after STOP, wait for busy bit to be clear. Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 4afba3e..156cc95 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -125,14 +125,19 @@ struct imx_i2c_struct { /** Functions for IMX I2C adapter driver *** ***/ -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) { unsigned long orig_jiffies = jiffies; + unsigned int temp; dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - /* wait for bus not busy */ - while (readb(i2c_imx-base + IMX_I2C_I2SR) I2SR_IBB) { + temp = readb(i2c_imx-base + IMX_I2C_I2SR); + while (1) { + if (for_busy (temp I2SR_IBB)) + break; + if (!for_busy !(temp I2SR_IBB)) + break; if (signal_pending(current)) { dev_dbg(i2c_imx-adapter.dev, %s I2C Interrupted\n, __func__); @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) return -EIO; } schedule(); + temp = readb(i2c_imx-base + IMX_I2C_I2SR); } return 0; @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) return 0; } -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) { unsigned int temp = 0; + int result; dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); /* Enable I2C controller */ + writeb(0, i2c_imx-base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); + + result = i2c_imx_bus_busy(i2c_imx, 0); + if (result) + return result; + /* Start I2C transaction */ temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_MSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); + if (result) + return result; + temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + return result; } static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Stop I2C transaction */ dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); temp = readb(i2c_imx-base + IMX_I2C_I2CR); - temp = ~I2CR_MSTA; + temp = ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx-base + IMX_I2C_I2CR); - /* setup chip registers to defaults */ - writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); - writeb(0, i2c_imx-base + IMX_I2C_I2SR); /* * This delay caused by an i.MXL hardware bug. * If no (or too short) delay, no STOP bit will be generated. */ udelay(i2c_imx-disable_delay); + + i2c_imx_bus_busy(i2c_imx, 0); + /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); } @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) dev_dbg(i2c_imx-adapter.dev, %s clear MSTA\n, __func__); temp = readb(i2c_imx-base + IMX_I2C_I2CR); - temp = ~I2CR_MSTA; + temp = ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx-base + IMX_I2C_I2CR); } else if (i == (msgs-len - 2)) { dev_dbg(i2c_imx-adapter.dev, @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - /* Check if i2c bus is not busy */ - result = i2c_imx_bus_busy(i2c_imx); + /* Start I2C transfer */ + result = i2c_imx_start(i2c_imx); if (result) goto fail0; - /* Start I2C transfer */ - i2c_imx_start(i2c_imx); - /* read/write data */ for (i = 0; i num; i++) { if (i) { @@ -386,6 +401,9 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_RSTA; writeb(temp, i2c_imx-base +
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Thu, Oct 1, 2009 at 4:38 PM, Sascha Hauer s.ha...@pengutronix.de wrote: On Thu, Oct 01, 2009 at 04:03:20PM +0800, Richard Zhao wrote: On Thu, Oct 1, 2009 at 9:13 AM, Richard Zhao linux...@gmail.com wrote: After START/RESTART, wait for busy bit to be set and after STOP, wait for busy bit to be clear. Signed-off-by: Richard Zhao linux...@gmail.com diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index 4afba3e..156cc95 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -125,14 +125,19 @@ struct imx_i2c_struct { /** Functions for IMX I2C adapter driver *** ***/ -static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) { unsigned long orig_jiffies = jiffies; + unsigned int temp; dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - /* wait for bus not busy */ - while (readb(i2c_imx-base + IMX_I2C_I2SR) I2SR_IBB) { + temp = readb(i2c_imx-base + IMX_I2C_I2SR); + while (1) { + if (for_busy (temp I2SR_IBB)) + break; + if (!for_busy !(temp I2SR_IBB)) + break; if (signal_pending(current)) { dev_dbg(i2c_imx-adapter.dev, %s I2C Interrupted\n, __func__); @@ -144,6 +149,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx) return -EIO; } schedule(); + temp = readb(i2c_imx-base + IMX_I2C_I2SR); } return 0; @@ -179,20 +185,32 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) return 0; } -static void i2c_imx_start(struct imx_i2c_struct *i2c_imx) +static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) { unsigned int temp = 0; + int result; dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); /* Enable I2C controller */ + writeb(0, i2c_imx-base + IMX_I2C_I2SR); writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); + + result = i2c_imx_bus_busy(i2c_imx, 0); + if (result) + return result; + /* Start I2C transaction */ temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_MSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + result = i2c_imx_bus_busy(i2c_imx, 1); + if (result) + return result; + temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); + return result; } static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) @@ -202,16 +220,16 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) /* Stop I2C transaction */ dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); temp = readb(i2c_imx-base + IMX_I2C_I2CR); - temp = ~I2CR_MSTA; + temp = ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx-base + IMX_I2C_I2CR); - /* setup chip registers to defaults */ - writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); - writeb(0, i2c_imx-base + IMX_I2C_I2SR); /* * This delay caused by an i.MXL hardware bug. * If no (or too short) delay, no STOP bit will be generated. */ udelay(i2c_imx-disable_delay); + + i2c_imx_bus_busy(i2c_imx, 0); + /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); } @@ -344,7 +362,7 @@ static int i2c_imx_read(struct imx_i2c_struct *i2c_imx, struct i2c_msg *msgs) dev_dbg(i2c_imx-adapter.dev, %s clear MSTA\n, __func__); temp = readb(i2c_imx-base + IMX_I2C_I2CR); - temp = ~I2CR_MSTA; + temp = ~(I2CR_MSTA | I2CR_MTX); writeb(temp, i2c_imx-base + IMX_I2C_I2CR); } else if (i == (msgs-len - 2)) { dev_dbg(i2c_imx-adapter.dev, @@ -370,14 +388,11 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - /* Check if i2c bus is not busy */ - result = i2c_imx_bus_busy(i2c_imx); + /* Start I2C transfer */ + result = i2c_imx_start(i2c_imx); if (result) goto fail0; - /* Start I2C transfer */ - i2c_imx_start(i2c_imx); - /* read/write data */ for (i = 0; i num; i++) { if (i) { @@ -386,6 +401,9 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |=
Re: [PATCH 3/4] i2c: imx: add macros and printk to make debug easy
On Thu, Oct 1, 2009 at 4:26 PM, Sascha Hauer s.ha...@pengutronix.de wrote: On Thu, Oct 01, 2009 at 04:01:50PM +0800, Richard Zhao wrote: On Thu, Oct 1, 2009 at 3:29 PM, Sascha Hauer s.ha...@pengutronix.de wrote: On Thu, Oct 01, 2009 at 09:13:32AM +0800, Richard Zhao wrote: When CONFIG_I2C_DEBUG_BUS is enabled, it helps dump registers at operation fail condition, and print i2c_msg to xfer. Signed-off-by: Richard Zhao linux...@gmail.com Honestly I don't think we need this. It makes the driver too verbose for my taste. Sascha diff --git a/drivers/i2c/busses/i2c-imx.c b/drivers/i2c/busses/i2c-imx.c index c1e541c..87faea4 100644 --- a/drivers/i2c/busses/i2c-imx.c +++ b/drivers/i2c/busses/i2c-imx.c @@ -125,6 +125,20 @@ struct imx_i2c_struct { /** Functions for IMX I2C adapter driver *** ***/ +#ifdef CONFIG_I2C_DEBUG_BUS +#define reg_dump(i2c_imx) \ +{ \ + printk(KERN_DEBUG fun %s:%d , __func__, __LINE__); \ + printk(KERN_DEBUG IADR %02x IFDR %02x I2CR %02x I2SR %02x\n, \ + readb(i2c_imx-base + IMX_I2C_IADR), \ + readb(i2c_imx-base + IMX_I2C_IFDR), \ + readb(i2c_imx-base + IMX_I2C_I2CR), \ + readb(i2c_imx-base + IMX_I2C_I2SR)); \ +} +#else +#define reg_dump(i2c_imx) +#endif + static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) { unsigned long orig_jiffies = jiffies; @@ -146,6 +160,7 @@ static int i2c_imx_bus_busy(struct imx_i2c_struct *i2c_imx, int for_busy) if (time_after(jiffies, orig_jiffies + HZ / 1000)) { dev_dbg(i2c_imx-adapter.dev, %s I2C bus is busy\n, __func__); + reg_dump(i2c_imx); return -EIO; } schedule(); @@ -164,9 +179,11 @@ static int i2c_imx_trx_complete(struct imx_i2c_struct *i2c_imx) if (unlikely(result 0)) { dev_dbg(i2c_imx-adapter.dev, %s result 0\n, __func__); + reg_dump(i2c_imx); return result; } else if (unlikely(!(i2c_imx-i2csr I2SR_IIF))) { dev_dbg(i2c_imx-adapter.dev, %s Timeout\n, __func__); + reg_dump(i2c_imx); return -ETIMEDOUT; } dev_dbg(i2c_imx-adapter.dev, %s TRX complete\n, __func__); @@ -178,6 +195,7 @@ static int i2c_imx_acked(struct imx_i2c_struct *i2c_imx) { if (readb(i2c_imx-base + IMX_I2C_I2SR) I2SR_RXAK) { dev_dbg(i2c_imx-adapter.dev, %s No ACK\n, __func__); + reg_dump(i2c_imx); return -EIO; /* No ACK */ } @@ -197,17 +215,20 @@ static int i2c_imx_start(struct imx_i2c_struct *i2c_imx) writeb(I2CR_IEN, i2c_imx-base + IMX_I2C_I2CR); result = i2c_imx_bus_busy(i2c_imx, 0); - if (result) + if (result) { + reg_dump(i2c_imx); return result; + } /* Start I2C transaction */ temp = readb(i2c_imx-base + IMX_I2C_I2CR); temp |= I2CR_MSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); result = i2c_imx_bus_busy(i2c_imx, 1); - if (result) + if (result) { + reg_dump(i2c_imx); return result; - + } temp |= I2CR_IIEN | I2CR_MTX | I2CR_TXAK; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); return result; @@ -228,7 +249,8 @@ static void i2c_imx_stop(struct imx_i2c_struct *i2c_imx) */ udelay(i2c_imx-disable_delay); - i2c_imx_bus_busy(i2c_imx, 0); + if (i2c_imx_bus_busy(i2c_imx, 0)) + reg_dump(i2c_imx); /* Disable I2C controller */ writeb(0, i2c_imx-base + IMX_I2C_I2CR); @@ -389,7 +411,18 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, struct imx_i2c_struct *i2c_imx = i2c_get_adapdata(adapter); dev_dbg(i2c_imx-adapter.dev, %s\n, __func__); - +#ifdef CONFIG_I2C_DEBUG_BUS + for (i = 0; i num; i++) { + printk(KERN_DEBUG msg%d addr %02x RD %d cnt %d d:, i, + msgs[i].addr, msgs[i].flags I2C_M_RD, msgs[i].len); + if (!(msgs[i].flags I2C_M_RD)) { + int j; + for (j = 0; j msgs[i].len; j++) + printk(%02x , msgs[i].buf[j]); + } + printk(\n); + } +#endif /* Start I2C transfer */ result = i2c_imx_start(i2c_imx); if (result) @@ -404,8 +437,10 @@ static int i2c_imx_xfer(struct i2c_adapter *adapter, temp |= I2CR_RSTA; writeb(temp, i2c_imx-base + IMX_I2C_I2CR); result = i2c_imx_bus_busy(i2c_imx, 1); - if (result) +
Re: [PATCH 1/4] i2c: imx: check busy bit when START/STOP
On Fri, Oct 2, 2009 at 12:37 AM, Wolfram Sang w.s...@pengutronix.de wrote: Ah, so 'make the driver work on i.MX51' is a good statement which should be part of the commit message. Well, maybe I can mention it. But I think the good point is to present what you modified, not the side effect. It is not the side effect but the intention :) As no code is changed without a need, the reason really should be in the patch description. No, it's not intention. I'm just trying to make the controller work in a right way. Without this patch, maybe some other fast cpus have problem too. I just tested mx31 and mx51. I will add Without this patch, i2c on some fast SoCs (for example imx51) will not work. Is it ok for you? Yes. But I don't have multi-master system. So I can't say that. The code is just taken from Freescale latest code. Without it, It could also cause a device error. I forget the details. Anyway, it doesn't make anything wrong. Do you know where the details are explained? No, I don't. I don't have device in hand now. If you have, could you please help do a simple test? Use hw to simulate multi-master system. Before execute xfer, you first pull down SDA, then pull down SDC. It simulates a START. and execute xfer to see whether IBB is set? Thanks Richard Regards, Wolfram -- Pengutronix e.K. | Wolfram Sang | Industrial Linux Solutions | http://www.pengutronix.de/ | -BEGIN PGP SIGNATURE- Version: GnuPG v1.4.9 (GNU/Linux) iEYEARECAAYFAkrE2uEACgkQD27XaX1/VRumZQCeL4x9oaBKKjSKzJLlRrkfvvqg nlEAoLpQdpI3TeKEvK13rs46kSZRDsZU =7kM6 -END PGP SIGNATURE- -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html