* [PATCH v9 0/4] AT91: replace broken TWI driver i2c-at91.c
@ 2012-03-19 15:07 ` Nikolaus Voss
0 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2012-03-19 15:07 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
Cc: nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0,
h.feurstein-Re5JQEeQqe8AvxtiuMwx3w,
rmallon-Re5JQEeQqe8AvxtiuMwx3w
The old driver has two main deficencies:
i) No repeated start (Sr) condiction is possible, this makes it unusable
e.g. for most SMBus transfers.
ii) I/O was done with polling/busy waiting what caused over-/underruns
even at light system loads and clock speeds.
The new driver overcomes these deficencies and in addition allows for
more than one TWI interface.
A remaining limitation is the fact, that only one repeated start is
possible (two concatenated messages). This limitation is imposed by
the hardware. However, this should not be a problem as all common
i2c-client communication does not rely on more than one repeated start.
Signed-off-by: Nikolaus Voss <n.voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>
Reviewed-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>
Tested-by: Hubert Feurstein <h.feurstein-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
v9: i) merge i2c-at91.c patches to single patch
ii) rewrite cwgr reg after timeout to reenable the right twi clock
v8: i) remove local include
ii) remove cpu_is_rm9200() and put twi hw bug info in platform_device
iii) use readl/writel_relaxed instead of __raw_readl/writel
v7: i) fix bug if internal address > 1 byte
ii) send stop when len == 1 (both reported by Carsten Behling)
v6: support for I2C_SMBUS_BLOCK_DATA transfers.
Better use of clk_(un)prepare().
More sensible transfer timeout.
v5: Another round of review comments from Ryan Mallon, Felipe Balbi
and Russell King: convert twi clk to use .dev_id, cleanups
v4: Integrated more review comments from Ryan Mallon and Felipe Balbi:
Moved register include file to local include, code cleanups
v3: Integrated review comments from Ryan Mallon and Felipe Balbi
v2: Fixed whitespace issue
Nikolaus Voss (4):
drivers/i2c/busses/i2c-at91.c: remove broken driver
Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
drivers/i2c/busses/i2c-at91.c: add new driver
G45 TWI: remove open drain setting for twi function gpios
arch/arm/mach-at91/at91cap9.c | 1 +
arch/arm/mach-at91/at91rm9200.c | 1 +
arch/arm/mach-at91/at91rm9200_devices.c | 9 +
arch/arm/mach-at91/at91sam9260.c | 1 +
arch/arm/mach-at91/at91sam9261.c | 1 +
arch/arm/mach-at91/at91sam9263.c | 1 +
arch/arm/mach-at91/at91sam9g45.c | 2 +
arch/arm/mach-at91/at91sam9g45_devices.c | 6 -
arch/arm/mach-at91/at91sam9rl.c | 2 +
arch/arm/mach-at91/include/mach/at91_twi.h | 68 ----
drivers/i2c/busses/Kconfig | 11 +-
drivers/i2c/busses/i2c-at91.c | 561 +++++++++++++++++-----------
12 files changed, 373 insertions(+), 291 deletions(-)
delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
--
1.7.5.4
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v9 1/4] drivers/i2c/busses/i2c-at91.c: remove broken driver
2012-03-19 15:07 ` Nikolaus Voss
@ 2011-11-08 10:49 ` Nikolaus Voss
-1 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 10:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
arch/arm/mach-at91/include/mach/at91_twi.h | 68 ------
drivers/i2c/busses/Makefile | 1 -
drivers/i2c/busses/i2c-at91.c | 314 ----------------------------
3 files changed, 0 insertions(+), 383 deletions(-)
delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
delete mode 100644 drivers/i2c/busses/i2c-at91.c
diff --git a/arch/arm/mach-at91/include/mach/at91_twi.h b/arch/arm/mach-at91/include/mach/at91_twi.h
deleted file mode 100644
index bb2880f..0000000
--- a/arch/arm/mach-at91/include/mach/at91_twi.h
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * arch/arm/mach-at91/include/mach/at91_twi.h
- *
- * Copyright (C) 2005 Ivan Kokshaysky
- * Copyright (C) SAN People
- *
- * Two-wire Interface (TWI) registers.
- * Based on AT91RM9200 datasheet revision E.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#ifndef AT91_TWI_H
-#define AT91_TWI_H
-
-#define AT91_TWI_CR 0x00 /* Control Register */
-#define AT91_TWI_START (1 << 0) /* Send a Start Condition */
-#define AT91_TWI_STOP (1 << 1) /* Send a Stop Condition */
-#define AT91_TWI_MSEN (1 << 2) /* Master Transfer Enable */
-#define AT91_TWI_MSDIS (1 << 3) /* Master Transfer Disable */
-#define AT91_TWI_SVEN (1 << 4) /* Slave Transfer Enable [SAM9260 only] */
-#define AT91_TWI_SVDIS (1 << 5) /* Slave Transfer Disable [SAM9260 only] */
-#define AT91_TWI_SWRST (1 << 7) /* Software Reset */
-
-#define AT91_TWI_MMR 0x04 /* Master Mode Register */
-#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address Size */
-#define AT91_TWI_IADRSZ_NO (0 << 8)
-#define AT91_TWI_IADRSZ_1 (1 << 8)
-#define AT91_TWI_IADRSZ_2 (2 << 8)
-#define AT91_TWI_IADRSZ_3 (3 << 8)
-#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
-#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
-
-#define AT91_TWI_SMR 0x08 /* Slave Mode Register [SAM9260 only] */
-#define AT91_TWI_SADR (0x7f << 16) /* Slave Address */
-
-#define AT91_TWI_IADR 0x0c /* Internal Address Register */
-
-#define AT91_TWI_CWGR 0x10 /* Clock Waveform Generator Register */
-#define AT91_TWI_CLDIV (0xff << 0) /* Clock Low Divisor */
-#define AT91_TWI_CHDIV (0xff << 8) /* Clock High Divisor */
-#define AT91_TWI_CKDIV (7 << 16) /* Clock Divider */
-
-#define AT91_TWI_SR 0x20 /* Status Register */
-#define AT91_TWI_TXCOMP (1 << 0) /* Transmission Complete */
-#define AT91_TWI_RXRDY (1 << 1) /* Receive Holding Register Ready */
-#define AT91_TWI_TXRDY (1 << 2) /* Transmit Holding Register Ready */
-#define AT91_TWI_SVREAD (1 << 3) /* Slave Read [SAM9260 only] */
-#define AT91_TWI_SVACC (1 << 4) /* Slave Access [SAM9260 only] */
-#define AT91_TWI_GACC (1 << 5) /* General Call Access [SAM9260 only] */
-#define AT91_TWI_OVRE (1 << 6) /* Overrun Error [AT91RM9200 only] */
-#define AT91_TWI_UNRE (1 << 7) /* Underrun Error [AT91RM9200 only] */
-#define AT91_TWI_NACK (1 << 8) /* Not Acknowledged */
-#define AT91_TWI_ARBLST (1 << 9) /* Arbitration Lost [SAM9260 only] */
-#define AT91_TWI_SCLWS (1 << 10) /* Clock Wait State [SAM9260 only] */
-#define AT91_TWI_EOSACC (1 << 11) /* End of Slave Address [SAM9260 only] */
-
-#define AT91_TWI_IER 0x24 /* Interrupt Enable Register */
-#define AT91_TWI_IDR 0x28 /* Interrupt Disable Register */
-#define AT91_TWI_IMR 0x2c /* Interrupt Mask Register */
-#define AT91_TWI_RHR 0x30 /* Receive Holding Register */
-#define AT91_TWI_THR 0x34 /* Transmit Holding Register */
-
-#endif
-
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index fba6da6..e8a1852 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -28,7 +28,6 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
-obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
deleted file mode 100644
index 1679dee..0000000
--- a/drivers/i2c/busses/i2c-at91.c
+++ /dev/null
@@ -1,314 +0,0 @@
-/*
- i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
-
- Copyright (C) 2004 Rick Bronson
- Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
-
- Borrowed heavily from original work by:
- Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
-
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
-*/
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/err.h>
-#include <linux/slab.h>
-#include <linux/types.h>
-#include <linux/delay.h>
-#include <linux/i2c.h>
-#include <linux/init.h>
-#include <linux/clk.h>
-#include <linux/platform_device.h>
-#include <linux/io.h>
-
-#include <mach/at91_twi.h>
-#include <mach/board.h>
-#include <mach/cpu.h>
-
-#define TWI_CLOCK 100000 /* Hz. max 400 Kbits/sec */
-
-
-static struct clk *twi_clk;
-static void __iomem *twi_base;
-
-#define at91_twi_read(reg) __raw_readl(twi_base + (reg))
-#define at91_twi_write(reg, val) __raw_writel((val), twi_base + (reg))
-
-
-/*
- * Initialize the TWI hardware registers.
- */
-static void __devinit at91_twi_hwinit(void)
-{
- unsigned long cdiv, ckdiv;
-
- at91_twi_write(AT91_TWI_IDR, 0xffffffff); /* Disable all interrupts */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_SWRST); /* Reset peripheral */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_MSEN); /* Set Master mode */
-
- /* Calcuate clock dividers */
- cdiv = (clk_get_rate(twi_clk) / (2 * TWI_CLOCK)) - 3;
- cdiv = cdiv + 1; /* round up */
- ckdiv = 0;
- while (cdiv > 255) {
- ckdiv++;
- cdiv = cdiv >> 1;
- }
-
- if (cpu_is_at91rm9200()) { /* AT91RM9200 Errata #22 */
- if (ckdiv > 5) {
- printk(KERN_ERR "AT91 I2C: Invalid TWI_CLOCK value!\n");
- ckdiv = 5;
- }
- }
-
- at91_twi_write(AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
-}
-
-/*
- * Poll the i2c status register until the specified bit is set.
- * Returns 0 if timed out (100 msec).
- */
-static short at91_poll_status(unsigned long bit)
-{
- int loop_cntr = 10000;
-
- do {
- udelay(10);
- } while (!(at91_twi_read(AT91_TWI_SR) & bit) && (--loop_cntr > 0));
-
- return (loop_cntr > 0);
-}
-
-static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
-{
- /* Send Start */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
-
- /* Read data */
- while (length--) {
- if (!length) /* need to send Stop before reading last byte */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
- if (!at91_poll_status(AT91_TWI_RXRDY)) {
- dev_dbg(&adap->dev, "RXRDY timeout\n");
- return -ETIMEDOUT;
- }
- *buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
- }
-
- return 0;
-}
-
-static int xfer_write(struct i2c_adapter *adap, unsigned char *buf, int length)
-{
- /* Load first byte into transmitter */
- at91_twi_write(AT91_TWI_THR, *buf++);
-
- /* Send Start */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
-
- do {
- if (!at91_poll_status(AT91_TWI_TXRDY)) {
- dev_dbg(&adap->dev, "TXRDY timeout\n");
- return -ETIMEDOUT;
- }
-
- length--; /* byte was transmitted */
-
- if (length > 0) /* more data to send? */
- at91_twi_write(AT91_TWI_THR, *buf++);
- } while (length);
-
- /* Send Stop */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
-
- return 0;
-}
-
-/*
- * Generic i2c master transfer entrypoint.
- *
- * Note: We do not use Atmel's feature of storing the "internal device address".
- * Instead the "internal device address" has to be written using a separate
- * i2c message.
- * http://lists.arm.linux.org.uk/pipermail/linux-arm-kernel/2004-September/024411.html
- */
-static int at91_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg, int num)
-{
- int i, ret;
-
- dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
-
- for (i = 0; i < num; i++) {
- dev_dbg(&adap->dev, " #%d: %sing %d byte%s %s 0x%02x\n", i,
- pmsg->flags & I2C_M_RD ? "read" : "writ",
- pmsg->len, pmsg->len > 1 ? "s" : "",
- pmsg->flags & I2C_M_RD ? "from" : "to", pmsg->addr);
-
- at91_twi_write(AT91_TWI_MMR, (pmsg->addr << 16)
- | ((pmsg->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
-
- if (pmsg->len && pmsg->buf) { /* sanity check */
- if (pmsg->flags & I2C_M_RD)
- ret = xfer_read(adap, pmsg->buf, pmsg->len);
- else
- ret = xfer_write(adap, pmsg->buf, pmsg->len);
-
- if (ret)
- return ret;
-
- /* Wait until transfer is finished */
- if (!at91_poll_status(AT91_TWI_TXCOMP)) {
- dev_dbg(&adap->dev, "TXCOMP timeout\n");
- return -ETIMEDOUT;
- }
- }
- dev_dbg(&adap->dev, "transfer complete\n");
- pmsg++; /* next message */
- }
- return i;
-}
-
-/*
- * Return list of supported functionality.
- */
-static u32 at91_func(struct i2c_adapter *adapter)
-{
- return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
-}
-
-static struct i2c_algorithm at91_algorithm = {
- .master_xfer = at91_xfer,
- .functionality = at91_func,
-};
-
-/*
- * Main initialization routine.
- */
-static int __devinit at91_i2c_probe(struct platform_device *pdev)
-{
- struct i2c_adapter *adapter;
- struct resource *res;
- int rc;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res)
- return -ENXIO;
-
- if (!request_mem_region(res->start, resource_size(res), "at91_i2c"))
- return -EBUSY;
-
- twi_base = ioremap(res->start, resource_size(res));
- if (!twi_base) {
- rc = -ENOMEM;
- goto fail0;
- }
-
- twi_clk = clk_get(NULL, "twi_clk");
- if (IS_ERR(twi_clk)) {
- dev_err(&pdev->dev, "no clock defined\n");
- rc = -ENODEV;
- goto fail1;
- }
-
- adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
- if (adapter == NULL) {
- dev_err(&pdev->dev, "can't allocate inteface!\n");
- rc = -ENOMEM;
- goto fail2;
- }
- snprintf(adapter->name, sizeof(adapter->name), "AT91");
- adapter->algo = &at91_algorithm;
- adapter->class = I2C_CLASS_HWMON;
- adapter->dev.parent = &pdev->dev;
- /* adapter->id == 0 ... only one TWI controller for now */
-
- platform_set_drvdata(pdev, adapter);
-
- clk_enable(twi_clk); /* enable peripheral clock */
- at91_twi_hwinit(); /* initialize TWI controller */
-
- rc = i2c_add_numbered_adapter(adapter);
- if (rc) {
- dev_err(&pdev->dev, "Adapter %s registration failed\n",
- adapter->name);
- goto fail3;
- }
-
- dev_info(&pdev->dev, "AT91 i2c bus driver.\n");
- return 0;
-
-fail3:
- platform_set_drvdata(pdev, NULL);
- kfree(adapter);
- clk_disable(twi_clk);
-fail2:
- clk_put(twi_clk);
-fail1:
- iounmap(twi_base);
-fail0:
- release_mem_region(res->start, resource_size(res));
-
- return rc;
-}
-
-static int __devexit at91_i2c_remove(struct platform_device *pdev)
-{
- struct i2c_adapter *adapter = platform_get_drvdata(pdev);
- struct resource *res;
- int rc;
-
- rc = i2c_del_adapter(adapter);
- platform_set_drvdata(pdev, NULL);
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- iounmap(twi_base);
- release_mem_region(res->start, resource_size(res));
-
- clk_disable(twi_clk); /* disable peripheral clock */
- clk_put(twi_clk);
-
- return rc;
-}
-
-#ifdef CONFIG_PM
-
-/* NOTE: could save a few mA by keeping clock off outside of at91_xfer... */
-
-static int at91_i2c_suspend(struct platform_device *pdev, pm_message_t mesg)
-{
- clk_disable(twi_clk);
- return 0;
-}
-
-static int at91_i2c_resume(struct platform_device *pdev)
-{
- return clk_enable(twi_clk);
-}
-
-#else
-#define at91_i2c_suspend NULL
-#define at91_i2c_resume NULL
-#endif
-
-static struct platform_driver at91_i2c_driver = {
- .probe = at91_i2c_probe,
- .remove = __devexit_p(at91_i2c_remove),
- .suspend = at91_i2c_suspend,
- .resume = at91_i2c_resume,
- .driver = {
- .name = "at91_i2c",
- .owner = THIS_MODULE,
- },
-};
-
-module_platform_driver(at91_i2c_driver);
-
-MODULE_AUTHOR("Rick Bronson");
-MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:at91_i2c");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v9 1/4] drivers/i2c/busses/i2c-at91.c: remove broken driver
@ 2011-11-08 10:49 ` Nikolaus Voss
0 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 10:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
arch/arm/mach-at91/include/mach/at91_twi.h | 68 ------
drivers/i2c/busses/Makefile | 1 -
drivers/i2c/busses/i2c-at91.c | 314 ----------------------------
3 files changed, 0 insertions(+), 383 deletions(-)
delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
delete mode 100644 drivers/i2c/busses/i2c-at91.c
diff --git a/arch/arm/mach-at91/include/mach/at91_twi.h b/arch/arm/mach-at91/include/mach/at91_twi.h
deleted file mode 100644
index bb2880f..0000000
--- a/arch/arm/mach-at91/include/mach/at91_twi.h
+++ /dev/null
@@ -1,68 +0,0 @@
-/*
- * arch/arm/mach-at91/include/mach/at91_twi.h
- *
- * Copyright (C) 2005 Ivan Kokshaysky
- * Copyright (C) SAN People
- *
- * Two-wire Interface (TWI) registers.
- * Based on AT91RM9200 datasheet revision E.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License as published by
- * the Free Software Foundation; either version 2 of the License, or
- * (at your option) any later version.
- */
-
-#ifndef AT91_TWI_H
-#define AT91_TWI_H
-
-#define AT91_TWI_CR 0x00 /* Control Register */
-#define AT91_TWI_START (1 << 0) /* Send a Start Condition */
-#define AT91_TWI_STOP (1 << 1) /* Send a Stop Condition */
-#define AT91_TWI_MSEN (1 << 2) /* Master Transfer Enable */
-#define AT91_TWI_MSDIS (1 << 3) /* Master Transfer Disable */
-#define AT91_TWI_SVEN (1 << 4) /* Slave Transfer Enable [SAM9260 only] */
-#define AT91_TWI_SVDIS (1 << 5) /* Slave Transfer Disable [SAM9260 only] */
-#define AT91_TWI_SWRST (1 << 7) /* Software Reset */
-
-#define AT91_TWI_MMR 0x04 /* Master Mode Register */
-#define AT91_TWI_IADRSZ (3 << 8) /* Internal Device Address Size */
-#define AT91_TWI_IADRSZ_NO (0 << 8)
-#define AT91_TWI_IADRSZ_1 (1 << 8)
-#define AT91_TWI_IADRSZ_2 (2 << 8)
-#define AT91_TWI_IADRSZ_3 (3 << 8)
-#define AT91_TWI_MREAD (1 << 12) /* Master Read Direction */
-#define AT91_TWI_DADR (0x7f << 16) /* Device Address */
-
-#define AT91_TWI_SMR 0x08 /* Slave Mode Register [SAM9260 only] */
-#define AT91_TWI_SADR (0x7f << 16) /* Slave Address */
-
-#define AT91_TWI_IADR 0x0c /* Internal Address Register */
-
-#define AT91_TWI_CWGR 0x10 /* Clock Waveform Generator Register */
-#define AT91_TWI_CLDIV (0xff << 0) /* Clock Low Divisor */
-#define AT91_TWI_CHDIV (0xff << 8) /* Clock High Divisor */
-#define AT91_TWI_CKDIV (7 << 16) /* Clock Divider */
-
-#define AT91_TWI_SR 0x20 /* Status Register */
-#define AT91_TWI_TXCOMP (1 << 0) /* Transmission Complete */
-#define AT91_TWI_RXRDY (1 << 1) /* Receive Holding Register Ready */
-#define AT91_TWI_TXRDY (1 << 2) /* Transmit Holding Register Ready */
-#define AT91_TWI_SVREAD (1 << 3) /* Slave Read [SAM9260 only] */
-#define AT91_TWI_SVACC (1 << 4) /* Slave Access [SAM9260 only] */
-#define AT91_TWI_GACC (1 << 5) /* General Call Access [SAM9260 only] */
-#define AT91_TWI_OVRE (1 << 6) /* Overrun Error [AT91RM9200 only] */
-#define AT91_TWI_UNRE (1 << 7) /* Underrun Error [AT91RM9200 only] */
-#define AT91_TWI_NACK (1 << 8) /* Not Acknowledged */
-#define AT91_TWI_ARBLST (1 << 9) /* Arbitration Lost [SAM9260 only] */
-#define AT91_TWI_SCLWS (1 << 10) /* Clock Wait State [SAM9260 only] */
-#define AT91_TWI_EOSACC (1 << 11) /* End of Slave Address [SAM9260 only] */
-
-#define AT91_TWI_IER 0x24 /* Interrupt Enable Register */
-#define AT91_TWI_IDR 0x28 /* Interrupt Disable Register */
-#define AT91_TWI_IMR 0x2c /* Interrupt Mask Register */
-#define AT91_TWI_RHR 0x30 /* Receive Holding Register */
-#define AT91_TWI_THR 0x34 /* Transmit Holding Register */
-
-#endif
-
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index fba6da6..e8a1852 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -28,7 +28,6 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
-obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
deleted file mode 100644
index 1679dee..0000000
--- a/drivers/i2c/busses/i2c-at91.c
+++ /dev/null
@@ -1,314 +0,0 @@
-/*
- i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
-
- Copyright (C) 2004 Rick Bronson
- Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
-
- Borrowed heavily from original work by:
- Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
-
- This program is free software; you can redistribute it and/or modify
- it under the terms of the GNU General Public License as published by
- the Free Software Foundation; either version 2 of the License, or
- (at your option) any later version.
-*/
-
-#include <linux/module.h>
-#include <linux/kernel.h>
-#include <linux/err.h>
-#include <linux/slab.h>
-#include <linux/types.h>
-#include <linux/delay.h>
-#include <linux/i2c.h>
-#include <linux/init.h>
-#include <linux/clk.h>
-#include <linux/platform_device.h>
-#include <linux/io.h>
-
-#include <mach/at91_twi.h>
-#include <mach/board.h>
-#include <mach/cpu.h>
-
-#define TWI_CLOCK 100000 /* Hz. max 400 Kbits/sec */
-
-
-static struct clk *twi_clk;
-static void __iomem *twi_base;
-
-#define at91_twi_read(reg) __raw_readl(twi_base + (reg))
-#define at91_twi_write(reg, val) __raw_writel((val), twi_base + (reg))
-
-
-/*
- * Initialize the TWI hardware registers.
- */
-static void __devinit at91_twi_hwinit(void)
-{
- unsigned long cdiv, ckdiv;
-
- at91_twi_write(AT91_TWI_IDR, 0xffffffff); /* Disable all interrupts */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_SWRST); /* Reset peripheral */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_MSEN); /* Set Master mode */
-
- /* Calcuate clock dividers */
- cdiv = (clk_get_rate(twi_clk) / (2 * TWI_CLOCK)) - 3;
- cdiv = cdiv + 1; /* round up */
- ckdiv = 0;
- while (cdiv > 255) {
- ckdiv++;
- cdiv = cdiv >> 1;
- }
-
- if (cpu_is_at91rm9200()) { /* AT91RM9200 Errata #22 */
- if (ckdiv > 5) {
- printk(KERN_ERR "AT91 I2C: Invalid TWI_CLOCK value!\n");
- ckdiv = 5;
- }
- }
-
- at91_twi_write(AT91_TWI_CWGR, (ckdiv << 16) | (cdiv << 8) | cdiv);
-}
-
-/*
- * Poll the i2c status register until the specified bit is set.
- * Returns 0 if timed out (100 msec).
- */
-static short at91_poll_status(unsigned long bit)
-{
- int loop_cntr = 10000;
-
- do {
- udelay(10);
- } while (!(at91_twi_read(AT91_TWI_SR) & bit) && (--loop_cntr > 0));
-
- return (loop_cntr > 0);
-}
-
-static int xfer_read(struct i2c_adapter *adap, unsigned char *buf, int length)
-{
- /* Send Start */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
-
- /* Read data */
- while (length--) {
- if (!length) /* need to send Stop before reading last byte */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
- if (!at91_poll_status(AT91_TWI_RXRDY)) {
- dev_dbg(&adap->dev, "RXRDY timeout\n");
- return -ETIMEDOUT;
- }
- *buf++ = (at91_twi_read(AT91_TWI_RHR) & 0xff);
- }
-
- return 0;
-}
-
-static int xfer_write(struct i2c_adapter *adap, unsigned char *buf, int length)
-{
- /* Load first byte into transmitter */
- at91_twi_write(AT91_TWI_THR, *buf++);
-
- /* Send Start */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_START);
-
- do {
- if (!at91_poll_status(AT91_TWI_TXRDY)) {
- dev_dbg(&adap->dev, "TXRDY timeout\n");
- return -ETIMEDOUT;
- }
-
- length--; /* byte was transmitted */
-
- if (length > 0) /* more data to send? */
- at91_twi_write(AT91_TWI_THR, *buf++);
- } while (length);
-
- /* Send Stop */
- at91_twi_write(AT91_TWI_CR, AT91_TWI_STOP);
-
- return 0;
-}
-
-/*
- * Generic i2c master transfer entrypoint.
- *
- * Note: We do not use Atmel's feature of storing the "internal device address".
- * Instead the "internal device address" has to be written using a separate
- * i2c message.
- * http://lists.arm.linux.org.uk/pipermail/linux-arm-kernel/2004-September/024411.html
- */
-static int at91_xfer(struct i2c_adapter *adap, struct i2c_msg *pmsg, int num)
-{
- int i, ret;
-
- dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
-
- for (i = 0; i < num; i++) {
- dev_dbg(&adap->dev, " #%d: %sing %d byte%s %s 0x%02x\n", i,
- pmsg->flags & I2C_M_RD ? "read" : "writ",
- pmsg->len, pmsg->len > 1 ? "s" : "",
- pmsg->flags & I2C_M_RD ? "from" : "to", pmsg->addr);
-
- at91_twi_write(AT91_TWI_MMR, (pmsg->addr << 16)
- | ((pmsg->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
-
- if (pmsg->len && pmsg->buf) { /* sanity check */
- if (pmsg->flags & I2C_M_RD)
- ret = xfer_read(adap, pmsg->buf, pmsg->len);
- else
- ret = xfer_write(adap, pmsg->buf, pmsg->len);
-
- if (ret)
- return ret;
-
- /* Wait until transfer is finished */
- if (!at91_poll_status(AT91_TWI_TXCOMP)) {
- dev_dbg(&adap->dev, "TXCOMP timeout\n");
- return -ETIMEDOUT;
- }
- }
- dev_dbg(&adap->dev, "transfer complete\n");
- pmsg++; /* next message */
- }
- return i;
-}
-
-/*
- * Return list of supported functionality.
- */
-static u32 at91_func(struct i2c_adapter *adapter)
-{
- return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
-}
-
-static struct i2c_algorithm at91_algorithm = {
- .master_xfer = at91_xfer,
- .functionality = at91_func,
-};
-
-/*
- * Main initialization routine.
- */
-static int __devinit at91_i2c_probe(struct platform_device *pdev)
-{
- struct i2c_adapter *adapter;
- struct resource *res;
- int rc;
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- if (!res)
- return -ENXIO;
-
- if (!request_mem_region(res->start, resource_size(res), "at91_i2c"))
- return -EBUSY;
-
- twi_base = ioremap(res->start, resource_size(res));
- if (!twi_base) {
- rc = -ENOMEM;
- goto fail0;
- }
-
- twi_clk = clk_get(NULL, "twi_clk");
- if (IS_ERR(twi_clk)) {
- dev_err(&pdev->dev, "no clock defined\n");
- rc = -ENODEV;
- goto fail1;
- }
-
- adapter = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
- if (adapter == NULL) {
- dev_err(&pdev->dev, "can't allocate inteface!\n");
- rc = -ENOMEM;
- goto fail2;
- }
- snprintf(adapter->name, sizeof(adapter->name), "AT91");
- adapter->algo = &at91_algorithm;
- adapter->class = I2C_CLASS_HWMON;
- adapter->dev.parent = &pdev->dev;
- /* adapter->id == 0 ... only one TWI controller for now */
-
- platform_set_drvdata(pdev, adapter);
-
- clk_enable(twi_clk); /* enable peripheral clock */
- at91_twi_hwinit(); /* initialize TWI controller */
-
- rc = i2c_add_numbered_adapter(adapter);
- if (rc) {
- dev_err(&pdev->dev, "Adapter %s registration failed\n",
- adapter->name);
- goto fail3;
- }
-
- dev_info(&pdev->dev, "AT91 i2c bus driver.\n");
- return 0;
-
-fail3:
- platform_set_drvdata(pdev, NULL);
- kfree(adapter);
- clk_disable(twi_clk);
-fail2:
- clk_put(twi_clk);
-fail1:
- iounmap(twi_base);
-fail0:
- release_mem_region(res->start, resource_size(res));
-
- return rc;
-}
-
-static int __devexit at91_i2c_remove(struct platform_device *pdev)
-{
- struct i2c_adapter *adapter = platform_get_drvdata(pdev);
- struct resource *res;
- int rc;
-
- rc = i2c_del_adapter(adapter);
- platform_set_drvdata(pdev, NULL);
-
- res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
- iounmap(twi_base);
- release_mem_region(res->start, resource_size(res));
-
- clk_disable(twi_clk); /* disable peripheral clock */
- clk_put(twi_clk);
-
- return rc;
-}
-
-#ifdef CONFIG_PM
-
-/* NOTE: could save a few mA by keeping clock off outside of at91_xfer... */
-
-static int at91_i2c_suspend(struct platform_device *pdev, pm_message_t mesg)
-{
- clk_disable(twi_clk);
- return 0;
-}
-
-static int at91_i2c_resume(struct platform_device *pdev)
-{
- return clk_enable(twi_clk);
-}
-
-#else
-#define at91_i2c_suspend NULL
-#define at91_i2c_resume NULL
-#endif
-
-static struct platform_driver at91_i2c_driver = {
- .probe = at91_i2c_probe,
- .remove = __devexit_p(at91_i2c_remove),
- .suspend = at91_i2c_suspend,
- .resume = at91_i2c_resume,
- .driver = {
- .name = "at91_i2c",
- .owner = THIS_MODULE,
- },
-};
-
-module_platform_driver(at91_i2c_driver);
-
-MODULE_AUTHOR("Rick Bronson");
-MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
-MODULE_LICENSE("GPL");
-MODULE_ALIAS("platform:at91_i2c");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
2012-03-19 15:07 ` Nikolaus Voss
@ 2011-11-08 10:49 ` Nikolaus Voss
-1 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 10:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
This driver has the following properties compared to the old driver:
1. Support for multiple interfaces.
2. Interrupt driven I/O as opposed to polling/busy waiting.
3. Support for _one_ repeated start (Sr) condition, which is enough
for most real-world applications including all SMBus transfer types.
(The hardware does not support issuing arbitrary Sr conditions on the
bus.)
testing: SoC: at91sam9g45
- BQ20Z80 battery SMBus client.
- on a 2.6.38 kernel with several i2c clients (temp-sensor,
audio-codec, touchscreen-controller, w1-bridge, io-expanders)
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
Reviewed-by: Felipe Balbi <balbi@ti.com>
Tested-by: Hubert Feurstein <h.feurstein@gmail.com>
---
arch/arm/mach-at91/at91rm9200_devices.c | 9 +
drivers/i2c/busses/Kconfig | 11 +-
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-at91.c | 455 +++++++++++++++++++++++++++++++
4 files changed, 469 insertions(+), 7 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-at91.c
diff --git a/arch/arm/mach-at91/at91rm9200_devices.c b/arch/arm/mach-at91/at91rm9200_devices.c
index 97676bd..0912f29 100644
--- a/arch/arm/mach-at91/at91rm9200_devices.c
+++ b/arch/arm/mach-at91/at91rm9200_devices.c
@@ -493,9 +493,18 @@ static struct resource twi_resources[] = {
},
};
+static const struct platform_device_id twi_id = {
+ /*
+ * driver_data is "1" for hardware with ckdiv upper limit == 5
+ * (AT91RM9200 erratum 22), "0" for twi modules without this bug
+ */
+ .driver_data = 1,
+};
+
static struct platform_device at91rm9200_twi_device = {
.name = "at91_i2c",
.id = -1,
+ .id_entry = &twi_id,
.resource = twi_resources,
.num_resources = ARRAY_SIZE(twi_resources),
};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 3101dd5..92158df 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -285,18 +285,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
config I2C_AT91
tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
- depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
+ depends on ARCH_AT91 && EXPERIMENTAL
help
This supports the use of the I2C interface on Atmel AT91
processors.
- This driver is BROKEN because the controller which it uses
- will easily trigger RX overrun and TX underrun errors. Using
- low I2C clock rates may partially work around those issues
- on some systems. Another serious problem is that there is no
- documented way to issue repeated START conditions, as needed
+ A serious problem is that there is no documented way to issue
+ repeated START conditions for more than two messages, as needed
to support combined I2C messages. Use the i2c-gpio driver
- unless your system can cope with those limitations.
+ unless your system can cope with this limitation.
config I2C_AU1550
tristate "Au1550/Au1200/Au1300 SMBus interface"
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e8a1852..fba6da6 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
new file mode 100644
index 0000000..40c39a2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -0,0 +1,455 @@
+/*
+ * i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
+ *
+ * Copyright (C) 2011 Weinmann Medical GmbH
+ * Author: Nikolaus Voss <n.voss@weinmann.de>
+ *
+ * Evolved from original work by:
+ * Copyright (C) 2004 Rick Bronson
+ * Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
+ *
+ * Borrowed heavily from original work by:
+ * Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define TWI_CLK_HZ 100000 /* max 400 Kbits/s */
+#define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
+
+/* AT91 TWI register definitions */
+#define AT91_TWI_CR 0x0000 /* Control Register */
+#define AT91_TWI_START 0x0001 /* Send a Start Condition */
+#define AT91_TWI_STOP 0x0002 /* Send a Stop Condition */
+#define AT91_TWI_MSEN 0x0004 /* Master Transfer Enable */
+#define AT91_TWI_SVDIS 0x0020 /* Slave Transfer Disable */
+#define AT91_TWI_SWRST 0x0080 /* Software Reset */
+
+#define AT91_TWI_MMR 0x0004 /* Master Mode Register */
+#define AT91_TWI_IADRSZ_1 0x0100 /* Internal Device Address Size */
+#define AT91_TWI_MREAD 0x1000 /* Master Read Direction */
+
+#define AT91_TWI_IADR 0x000c /* Internal Address Register */
+
+#define AT91_TWI_CWGR 0x0010 /* Clock Waveform Generator Reg */
+
+#define AT91_TWI_SR 0x0020 /* Status Register */
+#define AT91_TWI_TXCOMP 0x0001 /* Transmission Complete */
+#define AT91_TWI_RXRDY 0x0002 /* Receive Holding Register Ready */
+#define AT91_TWI_TXRDY 0x0004 /* Transmit Holding Register Ready */
+
+#define AT91_TWI_OVRE 0x0040 /* Overrun Error */
+#define AT91_TWI_UNRE 0x0080 /* Underrun Error */
+#define AT91_TWI_NACK 0x0100 /* Not Acknowledged */
+
+#define AT91_TWI_IER 0x0024 /* Interrupt Enable Register */
+#define AT91_TWI_IDR 0x0028 /* Interrupt Disable Register */
+#define AT91_TWI_IMR 0x002c /* Interrupt Mask Register */
+#define AT91_TWI_RHR 0x0030 /* Receive Holding Register */
+#define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
+
+struct at91_twi_dev {
+ struct device *dev;
+ void __iomem *base;
+ struct completion cmd_complete;
+ struct clk *clk;
+ u8 *buf;
+ size_t buf_len;
+ struct i2c_msg *msg;
+ int irq;
+ unsigned transfer_status;
+ struct i2c_adapter adapter;
+ bool has_ckdiv_limit_bug;
+ unsigned twi_cwgr_reg;
+};
+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+ return readl_relaxed(dev->base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
+{
+ writel_relaxed(val, dev->base + reg);
+}
+
+static void at91_disable_twi_interrupts(struct at91_twi_dev *dev)
+{
+ at91_twi_write(dev, AT91_TWI_IDR,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY);
+}
+
+static void at91_init_twi_bus(struct at91_twi_dev *dev)
+{
+ at91_disable_twi_interrupts(dev);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SWRST);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_MSEN);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SVDIS);
+ at91_twi_write(dev, AT91_TWI_CWGR, dev->twi_cwgr_reg);
+}
+
+/*
+ * Calculate symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_calc_twi_clock(struct at91_twi_dev *dev, int twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div >> 8);
+ const int cdiv = div >> ckdiv;
+
+ if (dev->has_ckdiv_limit_bug && (ckdiv > 5)) {
+ dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
+ ckdiv = 5;
+ }
+
+ dev->twi_cwgr_reg = (ckdiv << 16) | (cdiv << 8) | cdiv;
+}
+
+static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
+{
+ if (dev->buf_len <= 0)
+ return;
+
+ at91_twi_write(dev, AT91_TWI_THR, *dev->buf);
+
+ /* send stop when last byte has been written */
+ if (--dev->buf_len == 0)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "wrote 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
+{
+ *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
+ --dev->buf_len;
+
+ /* handle I2C_SMBUS_BLOCK_DATA */
+ if (unlikely(dev->msg->flags & I2C_M_RECV_LEN)) {
+ dev->msg->flags &= ~I2C_M_RECV_LEN;
+ dev->buf_len += *dev->buf;
+ dev->msg->len = dev->buf_len + 1;
+ dev_dbg(dev->dev, "received block length %d\n", dev->buf_len);
+ }
+
+ /* send stop if second but last byte has been read */
+ if (dev->buf_len == 1)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "read 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
+{
+ struct at91_twi_dev *dev = dev_id;
+ const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
+ const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ if (irqstatus & AT91_TWI_TXCOMP) {
+ at91_disable_twi_interrupts(dev);
+ dev->transfer_status = status;
+ complete(&dev->cmd_complete);
+ } else if (irqstatus & AT91_TWI_RXRDY) {
+ at91_twi_read_next_byte(dev);
+ } else if (irqstatus & AT91_TWI_TXRDY) {
+ at91_twi_write_next_byte(dev);
+ } else {
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int at91_do_twi_transfer(struct at91_twi_dev *dev)
+{
+ int ret;
+
+ dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
+ (dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
+
+ INIT_COMPLETION(dev->cmd_complete);
+ if (dev->msg->flags & I2C_M_RD) {
+ unsigned start_flags = AT91_TWI_START;
+
+ /* if only one byte is to be read, immediately stop transfer */
+ if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
+ start_flags |= AT91_TWI_STOP;
+ at91_twi_write(dev, AT91_TWI_CR, start_flags);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+ } else {
+ at91_twi_write_next_byte(dev);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+ }
+
+ ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ dev->adapter.timeout);
+ if (ret == 0) {
+ dev_err(dev->dev, "controller timed out\n");
+ at91_init_twi_bus(dev);
+ return -ETIMEDOUT;
+ }
+ if (dev->transfer_status & AT91_TWI_NACK) {
+ dev_dbg(dev->dev, "received nack\n");
+ return -EREMOTEIO;
+ }
+ if (dev->transfer_status & AT91_TWI_OVRE) {
+ dev_err(dev->dev, "overrun while reading\n");
+ return -EIO;
+ }
+ dev_dbg(dev->dev, "transfer complete\n");
+
+ return 0;
+}
+
+static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
+{
+ struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+ int ret;
+ unsigned int_addr_flag = 0;
+ struct i2c_msg *m_start = msg;
+
+ dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
+
+ /*
+ * The hardware can handle at most two messages concatenated by a
+ * repeated start via it's internal address feature.
+ */
+ if (num > 2) {
+ dev_err(dev->dev,
+ "cannot handle more than two concatenated messages.\n");
+ return 0;
+ } else if (num == 2) {
+ int internal_address = 0;
+ int i;
+
+ if (msg->flags & I2C_M_RD) {
+ dev_err(dev->dev, "first transfer must be write.\n");
+ return -EINVAL;
+ }
+ if (msg->len > 3) {
+ dev_err(dev->dev, "first message size must be <= 3.\n");
+ return -EINVAL;
+ }
+
+ /* 1st msg is put into the internal address, start with 2nd */
+ m_start = &msg[1];
+ for (i = 0; i < msg->len; ++i) {
+ const unsigned addr = msg->buf[msg->len - 1 - i];
+
+ internal_address |= addr << (8 * i);
+ int_addr_flag += AT91_TWI_IADRSZ_1;
+ }
+ at91_twi_write(dev, AT91_TWI_IADR, internal_address);
+ }
+
+ at91_twi_write(dev, AT91_TWI_MMR, (m_start->addr << 16) | int_addr_flag
+ | ((m_start->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
+
+ dev->buf_len = m_start->len;
+ dev->buf = m_start->buf;
+ dev->msg = m_start;
+
+ ret = at91_do_twi_transfer(dev);
+
+ return (ret < 0) ? ret : num;
+}
+
+static u32 at91_twi_func(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
+ | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+}
+
+static struct i2c_algorithm at91_twi_algorithm = {
+ .master_xfer = at91_twi_xfer,
+ .functionality = at91_twi_func,
+};
+
+static int __devinit at91_twi_probe(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev;
+ struct resource *mem, *ioarea;
+ int irq, rc;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ioarea = request_mem_region(mem->start, resource_size(mem), pdev->name);
+ if (!ioarea)
+ return -EBUSY;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ rc = -ENOMEM;
+ goto err_release_region;
+ }
+
+ if (pdev->id_entry && pdev->id_entry->driver_data == 1)
+ dev->has_ckdiv_limit_bug = 1;
+
+ init_completion(&dev->cmd_complete);
+
+ dev->dev = &pdev->dev;
+ dev->irq = irq;
+ platform_set_drvdata(pdev, dev);
+
+ dev->clk = clk_get(dev->dev, NULL);
+ if (IS_ERR(dev->clk)) {
+ dev_err(dev->dev, "no clock defined\n");
+ rc = -ENODEV;
+ goto err_free_mem;
+ }
+ clk_prepare(dev->clk);
+ clk_enable(dev->clk);
+
+ dev->base = ioremap(mem->start, resource_size(mem));
+ if (!dev->base) {
+ rc = -EBUSY;
+ goto err_mem_ioremap;
+ }
+
+ at91_calc_twi_clock(dev, TWI_CLK_HZ);
+ at91_init_twi_bus(dev);
+
+ rc = request_irq(dev->irq, atmel_twi_interrupt, 0,
+ dev_name(dev->dev), dev);
+ if (rc) {
+ dev_err(dev->dev, "Cannot get irq %d: %d\n", dev->irq, rc);
+ goto err_unuse_clocks;
+ }
+
+ snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
+ i2c_set_adapdata(&dev->adapter, dev);
+ dev->adapter.owner = THIS_MODULE;
+ dev->adapter.class = I2C_CLASS_HWMON;
+ dev->adapter.algo = &at91_twi_algorithm;
+ dev->adapter.dev.parent = dev->dev;
+ dev->adapter.nr = pdev->id;
+ dev->adapter.timeout = AT91_I2C_TIMEOUT;
+
+ rc = i2c_add_numbered_adapter(&dev->adapter);
+ if (rc) {
+ dev_err(dev->dev, "Adapter %s registration failed\n",
+ dev->adapter.name);
+ goto err_free_irq;
+ }
+
+ dev_info(dev->dev, "AT91 i2c bus driver.\n");
+ return 0;
+
+err_free_irq:
+ free_irq(dev->irq, dev);
+err_unuse_clocks:
+ iounmap(dev->base);
+err_mem_ioremap:
+ clk_disable(dev->clk);
+ clk_unprepare(dev->clk);
+ clk_put(dev->clk);
+err_free_mem:
+ kfree(dev);
+err_release_region:
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+static int __devexit at91_twi_remove(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev = platform_get_drvdata(pdev);
+ struct resource *mem;
+ int rc;
+
+ rc = i2c_del_adapter(&dev->adapter);
+ clk_disable(dev->clk);
+ clk_unprepare(dev->clk);
+ clk_put(dev->clk);
+ free_irq(dev->irq, dev);
+ iounmap(dev->base);
+ kfree(dev);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+#ifdef CONFIG_PM
+
+static int at91_twi_runtime_suspend(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ clk_disable(twi_dev->clk);
+
+ return 0;
+}
+
+static int at91_twi_runtime_resume(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ return clk_enable(twi_dev->clk);
+}
+
+static const struct dev_pm_ops at91_twi_pm = {
+ .runtime_suspend = at91_twi_runtime_suspend,
+ .runtime_resume = at91_twi_runtime_resume,
+};
+
+#define at91_twi_pm_ops (&at91_twi_pm)
+#else
+#define at91_twi_pm_ops NULL
+#endif
+
+static struct platform_driver at91_twi_driver = {
+ .probe = at91_twi_probe,
+ .remove = __devexit_p(at91_twi_remove),
+ .driver = {
+ .name = "at91_i2c",
+ .owner = THIS_MODULE,
+ .pm = at91_twi_pm_ops,
+ },
+};
+
+static int __init at91_twi_init(void)
+{
+ return platform_driver_register(&at91_twi_driver);
+}
+
+static void __exit at91_twi_exit(void)
+{
+ platform_driver_unregister(&at91_twi_driver);
+}
+
+subsys_initcall(at91_twi_init);
+module_exit(at91_twi_exit);
+
+MODULE_AUTHOR("Nikolaus Voss <n.voss@weinmann.de>");
+MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:at91_i2c");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2011-11-08 10:49 ` Nikolaus Voss
0 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 10:49 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
This driver has the following properties compared to the old driver:
1. Support for multiple interfaces.
2. Interrupt driven I/O as opposed to polling/busy waiting.
3. Support for _one_ repeated start (Sr) condition, which is enough
for most real-world applications including all SMBus transfer types.
(The hardware does not support issuing arbitrary Sr conditions on the
bus.)
testing: SoC: at91sam9g45
- BQ20Z80 battery SMBus client.
- on a 2.6.38 kernel with several i2c clients (temp-sensor,
audio-codec, touchscreen-controller, w1-bridge, io-expanders)
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
Reviewed-by: Felipe Balbi <balbi@ti.com>
Tested-by: Hubert Feurstein <h.feurstein@gmail.com>
---
arch/arm/mach-at91/at91rm9200_devices.c | 9 +
drivers/i2c/busses/Kconfig | 11 +-
drivers/i2c/busses/Makefile | 1 +
drivers/i2c/busses/i2c-at91.c | 455 +++++++++++++++++++++++++++++++
4 files changed, 469 insertions(+), 7 deletions(-)
create mode 100644 drivers/i2c/busses/i2c-at91.c
diff --git a/arch/arm/mach-at91/at91rm9200_devices.c b/arch/arm/mach-at91/at91rm9200_devices.c
index 97676bd..0912f29 100644
--- a/arch/arm/mach-at91/at91rm9200_devices.c
+++ b/arch/arm/mach-at91/at91rm9200_devices.c
@@ -493,9 +493,18 @@ static struct resource twi_resources[] = {
},
};
+static const struct platform_device_id twi_id = {
+ /*
+ * driver_data is "1" for hardware with ckdiv upper limit == 5
+ * (AT91RM9200 erratum 22), "0" for twi modules without this bug
+ */
+ .driver_data = 1,
+};
+
static struct platform_device at91rm9200_twi_device = {
.name = "at91_i2c",
.id = -1,
+ .id_entry = &twi_id,
.resource = twi_resources,
.num_resources = ARRAY_SIZE(twi_resources),
};
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 3101dd5..92158df 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -285,18 +285,15 @@ comment "I2C system bus drivers (mostly embedded / system-on-chip)"
config I2C_AT91
tristate "Atmel AT91 I2C Two-Wire interface (TWI)"
- depends on ARCH_AT91 && EXPERIMENTAL && BROKEN
+ depends on ARCH_AT91 && EXPERIMENTAL
help
This supports the use of the I2C interface on Atmel AT91
processors.
- This driver is BROKEN because the controller which it uses
- will easily trigger RX overrun and TX underrun errors. Using
- low I2C clock rates may partially work around those issues
- on some systems. Another serious problem is that there is no
- documented way to issue repeated START conditions, as needed
+ A serious problem is that there is no documented way to issue
+ repeated START conditions for more than two messages, as needed
to support combined I2C messages. Use the i2c-gpio driver
- unless your system can cope with those limitations.
+ unless your system can cope with this limitation.
config I2C_AU1550
tristate "Au1550/Au1200/Au1300 SMBus interface"
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index e8a1852..fba6da6 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_I2C_HYDRA) += i2c-hydra.o
obj-$(CONFIG_I2C_POWERMAC) += i2c-powermac.o
# Embedded system I2C/SMBus host controller drivers
+obj-$(CONFIG_I2C_AT91) += i2c-at91.o
obj-$(CONFIG_I2C_AU1550) += i2c-au1550.o
obj-$(CONFIG_I2C_BLACKFIN_TWI) += i2c-bfin-twi.o
obj-$(CONFIG_I2C_CPM) += i2c-cpm.o
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
new file mode 100644
index 0000000..40c39a2
--- /dev/null
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -0,0 +1,455 @@
+/*
+ * i2c Support for Atmel's AT91 Two-Wire Interface (TWI)
+ *
+ * Copyright (C) 2011 Weinmann Medical GmbH
+ * Author: Nikolaus Voss <n.voss@weinmann.de>
+ *
+ * Evolved from original work by:
+ * Copyright (C) 2004 Rick Bronson
+ * Converted to 2.6 by Andrew Victor <andrew@sanpeople.com>
+ *
+ * Borrowed heavily from original work by:
+ * Copyright (C) 2000 Philip Edelbrock <phil@stimpy.netroedge.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <linux/clk.h>
+#include <linux/completion.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define TWI_CLK_HZ 100000 /* max 400 Kbits/s */
+#define AT91_I2C_TIMEOUT msecs_to_jiffies(100) /* transfer timeout */
+
+/* AT91 TWI register definitions */
+#define AT91_TWI_CR 0x0000 /* Control Register */
+#define AT91_TWI_START 0x0001 /* Send a Start Condition */
+#define AT91_TWI_STOP 0x0002 /* Send a Stop Condition */
+#define AT91_TWI_MSEN 0x0004 /* Master Transfer Enable */
+#define AT91_TWI_SVDIS 0x0020 /* Slave Transfer Disable */
+#define AT91_TWI_SWRST 0x0080 /* Software Reset */
+
+#define AT91_TWI_MMR 0x0004 /* Master Mode Register */
+#define AT91_TWI_IADRSZ_1 0x0100 /* Internal Device Address Size */
+#define AT91_TWI_MREAD 0x1000 /* Master Read Direction */
+
+#define AT91_TWI_IADR 0x000c /* Internal Address Register */
+
+#define AT91_TWI_CWGR 0x0010 /* Clock Waveform Generator Reg */
+
+#define AT91_TWI_SR 0x0020 /* Status Register */
+#define AT91_TWI_TXCOMP 0x0001 /* Transmission Complete */
+#define AT91_TWI_RXRDY 0x0002 /* Receive Holding Register Ready */
+#define AT91_TWI_TXRDY 0x0004 /* Transmit Holding Register Ready */
+
+#define AT91_TWI_OVRE 0x0040 /* Overrun Error */
+#define AT91_TWI_UNRE 0x0080 /* Underrun Error */
+#define AT91_TWI_NACK 0x0100 /* Not Acknowledged */
+
+#define AT91_TWI_IER 0x0024 /* Interrupt Enable Register */
+#define AT91_TWI_IDR 0x0028 /* Interrupt Disable Register */
+#define AT91_TWI_IMR 0x002c /* Interrupt Mask Register */
+#define AT91_TWI_RHR 0x0030 /* Receive Holding Register */
+#define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
+
+struct at91_twi_dev {
+ struct device *dev;
+ void __iomem *base;
+ struct completion cmd_complete;
+ struct clk *clk;
+ u8 *buf;
+ size_t buf_len;
+ struct i2c_msg *msg;
+ int irq;
+ unsigned transfer_status;
+ struct i2c_adapter adapter;
+ bool has_ckdiv_limit_bug;
+ unsigned twi_cwgr_reg;
+};
+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+ return readl_relaxed(dev->base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
+{
+ writel_relaxed(val, dev->base + reg);
+}
+
+static void at91_disable_twi_interrupts(struct at91_twi_dev *dev)
+{
+ at91_twi_write(dev, AT91_TWI_IDR,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY);
+}
+
+static void at91_init_twi_bus(struct at91_twi_dev *dev)
+{
+ at91_disable_twi_interrupts(dev);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SWRST);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_MSEN);
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_SVDIS);
+ at91_twi_write(dev, AT91_TWI_CWGR, dev->twi_cwgr_reg);
+}
+
+/*
+ * Calculate symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_calc_twi_clock(struct at91_twi_dev *dev, int twi_clk)
+{
+ const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) - 2;
+ int ckdiv = fls(div >> 8);
+ const int cdiv = div >> ckdiv;
+
+ if (dev->has_ckdiv_limit_bug && (ckdiv > 5)) {
+ dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
+ ckdiv = 5;
+ }
+
+ dev->twi_cwgr_reg = (ckdiv << 16) | (cdiv << 8) | cdiv;
+}
+
+static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
+{
+ if (dev->buf_len <= 0)
+ return;
+
+ at91_twi_write(dev, AT91_TWI_THR, *dev->buf);
+
+ /* send stop when last byte has been written */
+ if (--dev->buf_len == 0)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "wrote 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
+{
+ *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
+ --dev->buf_len;
+
+ /* handle I2C_SMBUS_BLOCK_DATA */
+ if (unlikely(dev->msg->flags & I2C_M_RECV_LEN)) {
+ dev->msg->flags &= ~I2C_M_RECV_LEN;
+ dev->buf_len += *dev->buf;
+ dev->msg->len = dev->buf_len + 1;
+ dev_dbg(dev->dev, "received block length %d\n", dev->buf_len);
+ }
+
+ /* send stop if second but last byte has been read */
+ if (dev->buf_len == 1)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+
+ dev_dbg(dev->dev, "read 0x%x, to go %d\n", *dev->buf, dev->buf_len);
+
+ ++dev->buf;
+}
+
+static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
+{
+ struct at91_twi_dev *dev = dev_id;
+ const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
+ const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ if (irqstatus & AT91_TWI_TXCOMP) {
+ at91_disable_twi_interrupts(dev);
+ dev->transfer_status = status;
+ complete(&dev->cmd_complete);
+ } else if (irqstatus & AT91_TWI_RXRDY) {
+ at91_twi_read_next_byte(dev);
+ } else if (irqstatus & AT91_TWI_TXRDY) {
+ at91_twi_write_next_byte(dev);
+ } else {
+ return IRQ_NONE;
+ }
+
+ return IRQ_HANDLED;
+}
+
+static int at91_do_twi_transfer(struct at91_twi_dev *dev)
+{
+ int ret;
+
+ dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
+ (dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
+
+ INIT_COMPLETION(dev->cmd_complete);
+ if (dev->msg->flags & I2C_M_RD) {
+ unsigned start_flags = AT91_TWI_START;
+
+ /* if only one byte is to be read, immediately stop transfer */
+ if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
+ start_flags |= AT91_TWI_STOP;
+ at91_twi_write(dev, AT91_TWI_CR, start_flags);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+ } else {
+ at91_twi_write_next_byte(dev);
+ at91_twi_write(dev, AT91_TWI_IER,
+ AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+ }
+
+ ret = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
+ dev->adapter.timeout);
+ if (ret == 0) {
+ dev_err(dev->dev, "controller timed out\n");
+ at91_init_twi_bus(dev);
+ return -ETIMEDOUT;
+ }
+ if (dev->transfer_status & AT91_TWI_NACK) {
+ dev_dbg(dev->dev, "received nack\n");
+ return -EREMOTEIO;
+ }
+ if (dev->transfer_status & AT91_TWI_OVRE) {
+ dev_err(dev->dev, "overrun while reading\n");
+ return -EIO;
+ }
+ dev_dbg(dev->dev, "transfer complete\n");
+
+ return 0;
+}
+
+static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
+{
+ struct at91_twi_dev *dev = i2c_get_adapdata(adap);
+ int ret;
+ unsigned int_addr_flag = 0;
+ struct i2c_msg *m_start = msg;
+
+ dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
+
+ /*
+ * The hardware can handle at most two messages concatenated by a
+ * repeated start via it's internal address feature.
+ */
+ if (num > 2) {
+ dev_err(dev->dev,
+ "cannot handle more than two concatenated messages.\n");
+ return 0;
+ } else if (num == 2) {
+ int internal_address = 0;
+ int i;
+
+ if (msg->flags & I2C_M_RD) {
+ dev_err(dev->dev, "first transfer must be write.\n");
+ return -EINVAL;
+ }
+ if (msg->len > 3) {
+ dev_err(dev->dev, "first message size must be <= 3.\n");
+ return -EINVAL;
+ }
+
+ /* 1st msg is put into the internal address, start with 2nd */
+ m_start = &msg[1];
+ for (i = 0; i < msg->len; ++i) {
+ const unsigned addr = msg->buf[msg->len - 1 - i];
+
+ internal_address |= addr << (8 * i);
+ int_addr_flag += AT91_TWI_IADRSZ_1;
+ }
+ at91_twi_write(dev, AT91_TWI_IADR, internal_address);
+ }
+
+ at91_twi_write(dev, AT91_TWI_MMR, (m_start->addr << 16) | int_addr_flag
+ | ((m_start->flags & I2C_M_RD) ? AT91_TWI_MREAD : 0));
+
+ dev->buf_len = m_start->len;
+ dev->buf = m_start->buf;
+ dev->msg = m_start;
+
+ ret = at91_do_twi_transfer(dev);
+
+ return (ret < 0) ? ret : num;
+}
+
+static u32 at91_twi_func(struct i2c_adapter *adapter)
+{
+ return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL
+ | I2C_FUNC_SMBUS_READ_BLOCK_DATA;
+}
+
+static struct i2c_algorithm at91_twi_algorithm = {
+ .master_xfer = at91_twi_xfer,
+ .functionality = at91_twi_func,
+};
+
+static int __devinit at91_twi_probe(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev;
+ struct resource *mem, *ioarea;
+ int irq, rc;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem)
+ return -ENODEV;
+
+ irq = platform_get_irq(pdev, 0);
+ if (irq < 0)
+ return irq;
+
+ ioarea = request_mem_region(mem->start, resource_size(mem), pdev->name);
+ if (!ioarea)
+ return -EBUSY;
+
+ dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+ if (!dev) {
+ rc = -ENOMEM;
+ goto err_release_region;
+ }
+
+ if (pdev->id_entry && pdev->id_entry->driver_data == 1)
+ dev->has_ckdiv_limit_bug = 1;
+
+ init_completion(&dev->cmd_complete);
+
+ dev->dev = &pdev->dev;
+ dev->irq = irq;
+ platform_set_drvdata(pdev, dev);
+
+ dev->clk = clk_get(dev->dev, NULL);
+ if (IS_ERR(dev->clk)) {
+ dev_err(dev->dev, "no clock defined\n");
+ rc = -ENODEV;
+ goto err_free_mem;
+ }
+ clk_prepare(dev->clk);
+ clk_enable(dev->clk);
+
+ dev->base = ioremap(mem->start, resource_size(mem));
+ if (!dev->base) {
+ rc = -EBUSY;
+ goto err_mem_ioremap;
+ }
+
+ at91_calc_twi_clock(dev, TWI_CLK_HZ);
+ at91_init_twi_bus(dev);
+
+ rc = request_irq(dev->irq, atmel_twi_interrupt, 0,
+ dev_name(dev->dev), dev);
+ if (rc) {
+ dev_err(dev->dev, "Cannot get irq %d: %d\n", dev->irq, rc);
+ goto err_unuse_clocks;
+ }
+
+ snprintf(dev->adapter.name, sizeof(dev->adapter.name), "AT91");
+ i2c_set_adapdata(&dev->adapter, dev);
+ dev->adapter.owner = THIS_MODULE;
+ dev->adapter.class = I2C_CLASS_HWMON;
+ dev->adapter.algo = &at91_twi_algorithm;
+ dev->adapter.dev.parent = dev->dev;
+ dev->adapter.nr = pdev->id;
+ dev->adapter.timeout = AT91_I2C_TIMEOUT;
+
+ rc = i2c_add_numbered_adapter(&dev->adapter);
+ if (rc) {
+ dev_err(dev->dev, "Adapter %s registration failed\n",
+ dev->adapter.name);
+ goto err_free_irq;
+ }
+
+ dev_info(dev->dev, "AT91 i2c bus driver.\n");
+ return 0;
+
+err_free_irq:
+ free_irq(dev->irq, dev);
+err_unuse_clocks:
+ iounmap(dev->base);
+err_mem_ioremap:
+ clk_disable(dev->clk);
+ clk_unprepare(dev->clk);
+ clk_put(dev->clk);
+err_free_mem:
+ kfree(dev);
+err_release_region:
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+static int __devexit at91_twi_remove(struct platform_device *pdev)
+{
+ struct at91_twi_dev *dev = platform_get_drvdata(pdev);
+ struct resource *mem;
+ int rc;
+
+ rc = i2c_del_adapter(&dev->adapter);
+ clk_disable(dev->clk);
+ clk_unprepare(dev->clk);
+ clk_put(dev->clk);
+ free_irq(dev->irq, dev);
+ iounmap(dev->base);
+ kfree(dev);
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ release_mem_region(mem->start, resource_size(mem));
+
+ return rc;
+}
+
+#ifdef CONFIG_PM
+
+static int at91_twi_runtime_suspend(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ clk_disable(twi_dev->clk);
+
+ return 0;
+}
+
+static int at91_twi_runtime_resume(struct device *dev)
+{
+ struct at91_twi_dev *twi_dev = dev_get_drvdata(dev);
+
+ return clk_enable(twi_dev->clk);
+}
+
+static const struct dev_pm_ops at91_twi_pm = {
+ .runtime_suspend = at91_twi_runtime_suspend,
+ .runtime_resume = at91_twi_runtime_resume,
+};
+
+#define at91_twi_pm_ops (&at91_twi_pm)
+#else
+#define at91_twi_pm_ops NULL
+#endif
+
+static struct platform_driver at91_twi_driver = {
+ .probe = at91_twi_probe,
+ .remove = __devexit_p(at91_twi_remove),
+ .driver = {
+ .name = "at91_i2c",
+ .owner = THIS_MODULE,
+ .pm = at91_twi_pm_ops,
+ },
+};
+
+static int __init at91_twi_init(void)
+{
+ return platform_driver_register(&at91_twi_driver);
+}
+
+static void __exit at91_twi_exit(void)
+{
+ platform_driver_unregister(&at91_twi_driver);
+}
+
+subsys_initcall(at91_twi_init);
+module_exit(at91_twi_exit);
+
+MODULE_AUTHOR("Nikolaus Voss <n.voss@weinmann.de>");
+MODULE_DESCRIPTION("I2C (TWI) driver for Atmel AT91");
+MODULE_LICENSE("GPL");
+MODULE_ALIAS("platform:at91_i2c");
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2012-04-13 10:17 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-13 10:17 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-arm-kernel, linux-kernel, linux-i2c, nicolas.ferre,
ben-linux, balbi, rmallon
Hi Niko,
I'm using this driver since a while a now in my system
(soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
occasionally it happens that wrong data is read from my devices. I've
traced down the issue to the way how you do the interrupt handling. In
your code you do not consider that both status-flags (TXCOMP and
RXRDY) may be pending at the same time. So you handle the TXCOMP but
NOT the RXRDY which will cause a data-loss on the current transfer. As
a consequence also the next transfer will be corrupt, because you
start with old data in RHR. So I would suggest the following changes:
static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
if (irqstatus & AT91_TWI_RXRDY) {
at91_twi_read_next_byte(dev);
irqstatus &= ~AT91_TWI_RXRDY;
}
if (irqstatus & AT91_TWI_TXRDY) {
at91_twi_write_next_byte(dev);
irqstatus &= ~AT91_TWI_TXRDY;
}
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
irqstatus &= ~AT91_TWI_TXCOMP;
}
if (irqstatus) {
/* There should be no pending interrupt anymore. *)
return IRQ_NONE;
}
return IRQ_HANDLED;
}
To make sure that we do not start with old data in any case, I would
suggest to read SR and RHR before initiating a new transfer.
static int at91_do_twi_transfer(struct at91_twi_dev *dev)
{
int ret;
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
INIT_COMPLETION(dev->cmd_complete);
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
/* clear any pending data */
(void)at91_twi_read(dev, AT91_TWI_SR);
(void)at91_twi_read(dev, AT91_TWI_RHR);
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
at91_twi_write(dev, AT91_TWI_CR, start_flags);
[snip]
}
Hubert
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2012-04-13 10:17 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-13 10:17 UTC (permalink / raw)
To: linux-arm-kernel
Hi Niko,
I'm using this driver since a while a now in my system
(soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
occasionally it happens that wrong data is read from my devices. I've
traced down the issue to the way how you do the interrupt handling. In
your code you do not consider that both status-flags (TXCOMP and
RXRDY) may be pending at the same time. So you handle the TXCOMP but
NOT the RXRDY which will cause a data-loss on the current transfer. As
a consequence also the next transfer will be corrupt, because you
start with old data in RHR. So I would suggest the following changes:
static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
if (irqstatus & AT91_TWI_RXRDY) {
at91_twi_read_next_byte(dev);
irqstatus &= ~AT91_TWI_RXRDY;
}
if (irqstatus & AT91_TWI_TXRDY) {
at91_twi_write_next_byte(dev);
irqstatus &= ~AT91_TWI_TXRDY;
}
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
irqstatus &= ~AT91_TWI_TXCOMP;
}
if (irqstatus) {
/* There should be no pending interrupt anymore. *)
return IRQ_NONE;
}
return IRQ_HANDLED;
}
To make sure that we do not start with old data in any case, I would
suggest to read SR and RHR before initiating a new transfer.
static int at91_do_twi_transfer(struct at91_twi_dev *dev)
{
int ret;
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
INIT_COMPLETION(dev->cmd_complete);
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
/* clear any pending data */
(void)at91_twi_read(dev, AT91_TWI_SR);
(void)at91_twi_read(dev, AT91_TWI_RHR);
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
at91_twi_write(dev, AT91_TWI_CR, start_flags);
[snip]
}
Hubert
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2012-04-13 10:17 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-13 10:17 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0,
rmallon-Re5JQEeQqe8AvxtiuMwx3w
Hi Niko,
I'm using this driver since a while a now in my system
(soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
occasionally it happens that wrong data is read from my devices. I've
traced down the issue to the way how you do the interrupt handling. In
your code you do not consider that both status-flags (TXCOMP and
RXRDY) may be pending at the same time. So you handle the TXCOMP but
NOT the RXRDY which will cause a data-loss on the current transfer. As
a consequence also the next transfer will be corrupt, because you
start with old data in RHR. So I would suggest the following changes:
static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
if (irqstatus & AT91_TWI_RXRDY) {
at91_twi_read_next_byte(dev);
irqstatus &= ~AT91_TWI_RXRDY;
}
if (irqstatus & AT91_TWI_TXRDY) {
at91_twi_write_next_byte(dev);
irqstatus &= ~AT91_TWI_TXRDY;
}
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
irqstatus &= ~AT91_TWI_TXCOMP;
}
if (irqstatus) {
/* There should be no pending interrupt anymore. *)
return IRQ_NONE;
}
return IRQ_HANDLED;
}
To make sure that we do not start with old data in any case, I would
suggest to read SR and RHR before initiating a new transfer.
static int at91_do_twi_transfer(struct at91_twi_dev *dev)
{
int ret;
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
INIT_COMPLETION(dev->cmd_complete);
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
/* clear any pending data */
(void)at91_twi_read(dev, AT91_TWI_SR);
(void)at91_twi_read(dev, AT91_TWI_RHR);
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
at91_twi_write(dev, AT91_TWI_CR, start_flags);
[snip]
}
Hubert
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2012-04-13 10:39 ` Felipe Balbi
0 siblings, 0 replies; 39+ messages in thread
From: Felipe Balbi @ 2012-04-13 10:39 UTC (permalink / raw)
To: Hubert Feurstein
Cc: Nikolaus Voss, linux-arm-kernel, linux-kernel, linux-i2c,
nicolas.ferre, ben-linux, balbi, rmallon
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
Hi,
On Fri, Apr 13, 2012 at 12:17:59PM +0200, Hubert Feurstein wrote:
> Hi Niko,
>
> I'm using this driver since a while a now in my system
> (soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
> occasionally it happens that wrong data is read from my devices. I've
> traced down the issue to the way how you do the interrupt handling. In
> your code you do not consider that both status-flags (TXCOMP and
> RXRDY) may be pending at the same time. So you handle the TXCOMP but
> NOT the RXRDY which will cause a data-loss on the current transfer. As
> a consequence also the next transfer will be corrupt, because you
> start with old data in RHR. So I would suggest the following changes:
>
> static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>
> if (irqstatus & AT91_TWI_RXRDY) {
> at91_twi_read_next_byte(dev);
> irqstatus &= ~AT91_TWI_RXRDY;
> }
>
> if (irqstatus & AT91_TWI_TXRDY) {
> at91_twi_write_next_byte(dev);
> irqstatus &= ~AT91_TWI_TXRDY;
> }
>
> if (irqstatus & AT91_TWI_TXCOMP) {
> at91_disable_twi_interrupts(dev);
> dev->transfer_status = status;
> complete(&dev->cmd_complete);
> irqstatus &= ~AT91_TWI_TXCOMP;
> }
>
> if (irqstatus) {
> /* There should be no pending interrupt anymore. *)
> return IRQ_NONE;
> }
Dude, I remember asking the exact same thing when he first posted this
driver. That whole if...else if... else if ... else didn't look right.
Can you send this in patch format Hubert ? Take a look at
Documentation/SumittingPatches if you have never done it before.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2012-04-13 10:39 ` Felipe Balbi
0 siblings, 0 replies; 39+ messages in thread
From: Felipe Balbi @ 2012-04-13 10:39 UTC (permalink / raw)
To: linux-arm-kernel
Hi,
On Fri, Apr 13, 2012 at 12:17:59PM +0200, Hubert Feurstein wrote:
> Hi Niko,
>
> I'm using this driver since a while a now in my system
> (soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
> occasionally it happens that wrong data is read from my devices. I've
> traced down the issue to the way how you do the interrupt handling. In
> your code you do not consider that both status-flags (TXCOMP and
> RXRDY) may be pending at the same time. So you handle the TXCOMP but
> NOT the RXRDY which will cause a data-loss on the current transfer. As
> a consequence also the next transfer will be corrupt, because you
> start with old data in RHR. So I would suggest the following changes:
>
> static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>
> if (irqstatus & AT91_TWI_RXRDY) {
> at91_twi_read_next_byte(dev);
> irqstatus &= ~AT91_TWI_RXRDY;
> }
>
> if (irqstatus & AT91_TWI_TXRDY) {
> at91_twi_write_next_byte(dev);
> irqstatus &= ~AT91_TWI_TXRDY;
> }
>
> if (irqstatus & AT91_TWI_TXCOMP) {
> at91_disable_twi_interrupts(dev);
> dev->transfer_status = status;
> complete(&dev->cmd_complete);
> irqstatus &= ~AT91_TWI_TXCOMP;
> }
>
> if (irqstatus) {
> /* There should be no pending interrupt anymore. *)
> return IRQ_NONE;
> }
Dude, I remember asking the exact same thing when he first posted this
driver. That whole if...else if... else if ... else didn't look right.
Can you send this in patch format Hubert ? Take a look at
Documentation/SumittingPatches if you have never done it before.
--
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120413/dabfb87a/attachment-0001.sig>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
@ 2012-04-13 10:39 ` Felipe Balbi
0 siblings, 0 replies; 39+ messages in thread
From: Felipe Balbi @ 2012-04-13 10:39 UTC (permalink / raw)
To: Hubert Feurstein
Cc: Nikolaus Voss, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0,
rmallon-Re5JQEeQqe8AvxtiuMwx3w
[-- Attachment #1: Type: text/plain, Size: 1789 bytes --]
Hi,
On Fri, Apr 13, 2012 at 12:17:59PM +0200, Hubert Feurstein wrote:
> Hi Niko,
>
> I'm using this driver since a while a now in my system
> (soc:at91sam9m10; kernel:v3.2.14) and it works quite well. But
> occasionally it happens that wrong data is read from my devices. I've
> traced down the issue to the way how you do the interrupt handling. In
> your code you do not consider that both status-flags (TXCOMP and
> RXRDY) may be pending at the same time. So you handle the TXCOMP but
> NOT the RXRDY which will cause a data-loss on the current transfer. As
> a consequence also the next transfer will be corrupt, because you
> start with old data in RHR. So I would suggest the following changes:
>
> static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>
> if (irqstatus & AT91_TWI_RXRDY) {
> at91_twi_read_next_byte(dev);
> irqstatus &= ~AT91_TWI_RXRDY;
> }
>
> if (irqstatus & AT91_TWI_TXRDY) {
> at91_twi_write_next_byte(dev);
> irqstatus &= ~AT91_TWI_TXRDY;
> }
>
> if (irqstatus & AT91_TWI_TXCOMP) {
> at91_disable_twi_interrupts(dev);
> dev->transfer_status = status;
> complete(&dev->cmd_complete);
> irqstatus &= ~AT91_TWI_TXCOMP;
> }
>
> if (irqstatus) {
> /* There should be no pending interrupt anymore. *)
> return IRQ_NONE;
> }
Dude, I remember asking the exact same thing when he first posted this
driver. That whole if...else if... else if ... else didn't look right.
Can you send this in patch format Hubert ? Take a look at
Documentation/SumittingPatches if you have never done it before.
--
balbi
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-13 11:44 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-13 11:44 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-arm-kernel, linux-kernel, linux-i2c, nicolas.ferre,
ben-linux, balbi, rmallon, Hubert Feurstein
In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
pending at the same time. In this case TXCOMP is handled but NOT RXRDY
which causes a data-loss on the current transfer. As a consequence also the
next transfer will be corrupt, because old data is pending in RHR.
To make sure that we do not start with old data in any case, SR and RHR is
read empty before initiating a new transfer.
Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
---
This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
drivers/i2c/busses/i2c-at91.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 40c39a2..295835f 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
- const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+ unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
+ if (!irqstatus)
+ return IRQ_NONE;
+
+ if (irqstatus & AT91_TWI_RXRDY)
+ at91_twi_read_next_byte(dev);
+
+ if (irqstatus & AT91_TWI_TXRDY)
+ at91_twi_write_next_byte(dev);
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
- } else if (irqstatus & AT91_TWI_RXRDY) {
- at91_twi_read_next_byte(dev);
- } else if (irqstatus & AT91_TWI_TXRDY) {
- at91_twi_write_next_byte(dev);
- } else {
- return IRQ_NONE;
}
return IRQ_HANDLED;
@@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
+ /* clear any pending data */
+ (void)at91_twi_read(dev, AT91_TWI_SR);
+ (void)at91_twi_read(dev, AT91_TWI_RHR);
+
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
@@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
internal_address |= addr << (8 * i);
int_addr_flag += AT91_TWI_IADRSZ_1;
}
+
at91_twi_write(dev, AT91_TWI_IADR, internal_address);
}
--
1.7.8.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-13 11:44 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-13 11:44 UTC (permalink / raw)
To: linux-arm-kernel
In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
pending at the same time. In this case TXCOMP is handled but NOT RXRDY
which causes a data-loss on the current transfer. As a consequence also the
next transfer will be corrupt, because old data is pending in RHR.
To make sure that we do not start with old data in any case, SR and RHR is
read empty before initiating a new transfer.
Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
---
This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
drivers/i2c/busses/i2c-at91.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 40c39a2..295835f 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
- const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+ unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
+ if (!irqstatus)
+ return IRQ_NONE;
+
+ if (irqstatus & AT91_TWI_RXRDY)
+ at91_twi_read_next_byte(dev);
+
+ if (irqstatus & AT91_TWI_TXRDY)
+ at91_twi_write_next_byte(dev);
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
- } else if (irqstatus & AT91_TWI_RXRDY) {
- at91_twi_read_next_byte(dev);
- } else if (irqstatus & AT91_TWI_TXRDY) {
- at91_twi_write_next_byte(dev);
- } else {
- return IRQ_NONE;
}
return IRQ_HANDLED;
@@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
+ /* clear any pending data */
+ (void)at91_twi_read(dev, AT91_TWI_SR);
+ (void)at91_twi_read(dev, AT91_TWI_RHR);
+
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
@@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
internal_address |= addr << (8 * i);
int_addr_flag += AT91_TWI_IADRSZ_1;
}
+
at91_twi_write(dev, AT91_TWI_IADR, internal_address);
}
--
1.7.8.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-13 11:44 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-13 11:44 UTC (permalink / raw)
To: Nikolaus Voss
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0,
rmallon-Re5JQEeQqe8AvxtiuMwx3w, Hubert Feurstein
In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
pending at the same time. In this case TXCOMP is handled but NOT RXRDY
which causes a data-loss on the current transfer. As a consequence also the
next transfer will be corrupt, because old data is pending in RHR.
To make sure that we do not start with old data in any case, SR and RHR is
read empty before initiating a new transfer.
Signed-off-by: Hubert Feurstein <h.feurstein-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
---
This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
drivers/i2c/busses/i2c-at91.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 40c39a2..295835f 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
{
struct at91_twi_dev *dev = dev_id;
const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
- const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+ unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
+
+ irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
+ if (!irqstatus)
+ return IRQ_NONE;
+
+ if (irqstatus & AT91_TWI_RXRDY)
+ at91_twi_read_next_byte(dev);
+
+ if (irqstatus & AT91_TWI_TXRDY)
+ at91_twi_write_next_byte(dev);
if (irqstatus & AT91_TWI_TXCOMP) {
at91_disable_twi_interrupts(dev);
dev->transfer_status = status;
complete(&dev->cmd_complete);
- } else if (irqstatus & AT91_TWI_RXRDY) {
- at91_twi_read_next_byte(dev);
- } else if (irqstatus & AT91_TWI_TXRDY) {
- at91_twi_write_next_byte(dev);
- } else {
- return IRQ_NONE;
}
return IRQ_HANDLED;
@@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
if (dev->msg->flags & I2C_M_RD) {
unsigned start_flags = AT91_TWI_START;
+ /* clear any pending data */
+ (void)at91_twi_read(dev, AT91_TWI_SR);
+ (void)at91_twi_read(dev, AT91_TWI_RHR);
+
/* if only one byte is to be read, immediately stop transfer */
if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
start_flags |= AT91_TWI_STOP;
@@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
internal_address |= addr << (8 * i);
int_addr_flag += AT91_TWI_IADRSZ_1;
}
+
at91_twi_write(dev, AT91_TWI_IADR, internal_address);
}
--
1.7.8.3
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-13 22:06 ` Ryan Mallon
0 siblings, 0 replies; 39+ messages in thread
From: Ryan Mallon @ 2012-04-13 22:06 UTC (permalink / raw)
To: Hubert Feurstein
Cc: Nikolaus Voss, linux-arm-kernel, linux-kernel, linux-i2c,
nicolas.ferre, ben-linux, balbi
On 13/04/12 21:44, Hubert Feurstein wrote:
> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
> which causes a data-loss on the current transfer. As a consequence also the
> next transfer will be corrupt, because old data is pending in RHR.
>
> To make sure that we do not start with old data in any case, SR and RHR is
> read empty before initiating a new transfer.
>
> Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
Couple of very minor comments below.
~Ryan
> ---
> This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
>
> drivers/i2c/busses/i2c-at91.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 40c39a2..295835f 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
> + if (!irqstatus)
> + return IRQ_NONE;
> +
> + if (irqstatus & AT91_TWI_RXRDY)
> + at91_twi_read_next_byte(dev);
> +
> + if (irqstatus & AT91_TWI_TXRDY)
> + at91_twi_write_next_byte(dev);
>
> if (irqstatus & AT91_TWI_TXCOMP) {
> at91_disable_twi_interrupts(dev);
> dev->transfer_status = status;
> complete(&dev->cmd_complete);
> - } else if (irqstatus & AT91_TWI_RXRDY) {
> - at91_twi_read_next_byte(dev);
> - } else if (irqstatus & AT91_TWI_TXRDY) {
> - at91_twi_write_next_byte(dev);
> - } else {
> - return IRQ_NONE;
> }
>
> return IRQ_HANDLED;
> @@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> if (dev->msg->flags & I2C_M_RD) {
> unsigned start_flags = AT91_TWI_START;
>
> + /* clear any pending data */
> + (void)at91_twi_read(dev, AT91_TWI_SR);
> + (void)at91_twi_read(dev, AT91_TWI_RHR);
Drop the void cast.
> +
> /* if only one byte is to be read, immediately stop transfer */
> if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
> start_flags |= AT91_TWI_STOP;
> @@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> internal_address |= addr << (8 * i);
> int_addr_flag += AT91_TWI_IADRSZ_1;
> }
> +
Please don't make unrelated whitespace changes.
> at91_twi_write(dev, AT91_TWI_IADR, internal_address);
> }
>
With those fixed, please add:
Reviewed-by: Ryan Mallon <rmallon@gmail.com>
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-13 22:06 ` Ryan Mallon
0 siblings, 0 replies; 39+ messages in thread
From: Ryan Mallon @ 2012-04-13 22:06 UTC (permalink / raw)
To: linux-arm-kernel
On 13/04/12 21:44, Hubert Feurstein wrote:
> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
> which causes a data-loss on the current transfer. As a consequence also the
> next transfer will be corrupt, because old data is pending in RHR.
>
> To make sure that we do not start with old data in any case, SR and RHR is
> read empty before initiating a new transfer.
>
> Signed-off-by: Hubert Feurstein <h.feurstein@gmail.com>
Couple of very minor comments below.
~Ryan
> ---
> This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
>
> drivers/i2c/busses/i2c-at91.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 40c39a2..295835f 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
> + if (!irqstatus)
> + return IRQ_NONE;
> +
> + if (irqstatus & AT91_TWI_RXRDY)
> + at91_twi_read_next_byte(dev);
> +
> + if (irqstatus & AT91_TWI_TXRDY)
> + at91_twi_write_next_byte(dev);
>
> if (irqstatus & AT91_TWI_TXCOMP) {
> at91_disable_twi_interrupts(dev);
> dev->transfer_status = status;
> complete(&dev->cmd_complete);
> - } else if (irqstatus & AT91_TWI_RXRDY) {
> - at91_twi_read_next_byte(dev);
> - } else if (irqstatus & AT91_TWI_TXRDY) {
> - at91_twi_write_next_byte(dev);
> - } else {
> - return IRQ_NONE;
> }
>
> return IRQ_HANDLED;
> @@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> if (dev->msg->flags & I2C_M_RD) {
> unsigned start_flags = AT91_TWI_START;
>
> + /* clear any pending data */
> + (void)at91_twi_read(dev, AT91_TWI_SR);
> + (void)at91_twi_read(dev, AT91_TWI_RHR);
Drop the void cast.
> +
> /* if only one byte is to be read, immediately stop transfer */
> if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
> start_flags |= AT91_TWI_STOP;
> @@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> internal_address |= addr << (8 * i);
> int_addr_flag += AT91_TWI_IADRSZ_1;
> }
> +
Please don't make unrelated whitespace changes.
> at91_twi_write(dev, AT91_TWI_IADR, internal_address);
> }
>
With those fixed, please add:
Reviewed-by: Ryan Mallon <rmallon@gmail.com>
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-13 22:06 ` Ryan Mallon
0 siblings, 0 replies; 39+ messages in thread
From: Ryan Mallon @ 2012-04-13 22:06 UTC (permalink / raw)
To: Hubert Feurstein
Cc: Nikolaus Voss, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0
On 13/04/12 21:44, Hubert Feurstein wrote:
> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
> which causes a data-loss on the current transfer. As a consequence also the
> next transfer will be corrupt, because old data is pending in RHR.
>
> To make sure that we do not start with old data in any case, SR and RHR is
> read empty before initiating a new transfer.
>
> Signed-off-by: Hubert Feurstein <h.feurstein-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Couple of very minor comments below.
~Ryan
> ---
> This patch applies on top of [PATCH v9 3/4] drivers/i2c/busses/i2c-at91.c: add new driver
>
> drivers/i2c/busses/i2c-at91.c | 23 ++++++++++++++++-------
> 1 files changed, 16 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 40c39a2..295835f 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
> + if (!irqstatus)
> + return IRQ_NONE;
> +
> + if (irqstatus & AT91_TWI_RXRDY)
> + at91_twi_read_next_byte(dev);
> +
> + if (irqstatus & AT91_TWI_TXRDY)
> + at91_twi_write_next_byte(dev);
>
> if (irqstatus & AT91_TWI_TXCOMP) {
> at91_disable_twi_interrupts(dev);
> dev->transfer_status = status;
> complete(&dev->cmd_complete);
> - } else if (irqstatus & AT91_TWI_RXRDY) {
> - at91_twi_read_next_byte(dev);
> - } else if (irqstatus & AT91_TWI_TXRDY) {
> - at91_twi_write_next_byte(dev);
> - } else {
> - return IRQ_NONE;
> }
>
> return IRQ_HANDLED;
> @@ -189,6 +193,10 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> if (dev->msg->flags & I2C_M_RD) {
> unsigned start_flags = AT91_TWI_START;
>
> + /* clear any pending data */
> + (void)at91_twi_read(dev, AT91_TWI_SR);
> + (void)at91_twi_read(dev, AT91_TWI_RHR);
Drop the void cast.
> +
> /* if only one byte is to be read, immediately stop transfer */
> if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
> start_flags |= AT91_TWI_STOP;
> @@ -259,6 +267,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> internal_address |= addr << (8 * i);
> int_addr_flag += AT91_TWI_IADRSZ_1;
> }
> +
Please don't make unrelated whitespace changes.
> at91_twi_write(dev, AT91_TWI_IADR, internal_address);
> }
>
With those fixed, please add:
Reviewed-by: Ryan Mallon <rmallon-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
^ permalink raw reply [flat|nested] 39+ messages in thread
* RE: [PATCH] i2c-at91: fix data-loss issue
2012-04-13 11:44 ` Hubert Feurstein
(?)
@ 2012-04-16 7:30 ` Voss, Nikolaus
-1 siblings, 0 replies; 39+ messages in thread
From: Voss, Nikolaus @ 2012-04-16 7:30 UTC (permalink / raw)
To: 'Hubert Feurstein'
Cc: 'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'linux-i2c@vger.kernel.org',
'nicolas.ferre@atmel.com', 'ben-linux@fluff.org',
'balbi@ti.com', 'rmallon@gmail.com'
Hubert Feurstein wrote on 2012-04-13:
> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
> which causes a data-loss on the current transfer
Right, this is definitely a bug and must be corrected. Part of my
motivation for exclusively or-ing the irq bits was not reading/
writing beyond the buffer because of (still) pending bits despite
of an already finished transfer, so I gave TXCOMP the highest prio.
Because of other reasons, write_next_byte() already checks this and
does nothing if all data already has been written. My suggestion is
to have read_next_byte() do this check too, as I don't trust the
hardware to reset RXRDY _immediately_ after reading.
> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
> *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
The above line should be unnecessary as no more than those interrupts
are enabled anyway. Any special reason for this?
> + if (!irqstatus)
> + return IRQ_NONE;
> +
> + if (irqstatus & AT91_TWI_RXRDY)
> + at91_twi_read_next_byte(dev);
> +
> + if (irqstatus & AT91_TWI_TXRDY)
> + at91_twi_write_next_byte(dev);
I would like to exclusively or TXRDY and RXRDY as those really should
not be active at the same time. Keeps the decision tree lean ;-).
> @@ -189,6 +193,10 @@ static int
> at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev->msg->flags &
> I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
> + /* clear any pending data */
> + (void)at91_twi_read(dev, AT91_TWI_SR);
> + (void)at91_twi_read(dev, AT91_TWI_RHR);
I would like to modify this, as this is a partial fix for the above bug
which should already be fully fixed by the modified isr.
I fear subtle data-loss if we make (partial) tabula rasa before each
transfer. I'd rather add an assertion to check if the corresponding
irqs are active as an indication for a driver/hw-bug.
I'll repost the driver with your fix on positive feedback from you.
Thanks for tracking this down.
Ben, is there any chance to get this driver into next?
Niko
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-16 7:30 ` Voss, Nikolaus
0 siblings, 0 replies; 39+ messages in thread
From: Voss, Nikolaus @ 2012-04-16 7:30 UTC (permalink / raw)
To: linux-arm-kernel
Hubert Feurstein wrote on 2012-04-13:
> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
> which causes a data-loss on the current transfer
Right, this is definitely a bug and must be corrected. Part of my
motivation for exclusively or-ing the irq bits was not reading/
writing beyond the buffer because of (still) pending bits despite
of an already finished transfer, so I gave TXCOMP the highest prio.
Because of other reasons, write_next_byte() already checks this and
does nothing if all data already has been written. My suggestion is
to have read_next_byte() do this check too, as I don't trust the
hardware to reset RXRDY _immediately_ after reading.
> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
> *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
The above line should be unnecessary as no more than those interrupts
are enabled anyway. Any special reason for this?
> + if (!irqstatus)
> + return IRQ_NONE;
> +
> + if (irqstatus & AT91_TWI_RXRDY)
> + at91_twi_read_next_byte(dev);
> +
> + if (irqstatus & AT91_TWI_TXRDY)
> + at91_twi_write_next_byte(dev);
I would like to exclusively or TXRDY and RXRDY as those really should
not be active at the same time. Keeps the decision tree lean ;-).
> @@ -189,6 +193,10 @@ static int
> at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev->msg->flags &
> I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
> + /* clear any pending data */
> + (void)at91_twi_read(dev, AT91_TWI_SR);
> + (void)at91_twi_read(dev, AT91_TWI_RHR);
I would like to modify this, as this is a partial fix for the above bug
which should already be fully fixed by the modified isr.
I fear subtle data-loss if we make (partial) tabula rasa before each
transfer. I'd rather add an assertion to check if the corresponding
irqs are active as an indication for a driver/hw-bug.
I'll repost the driver with your fix on positive feedback from you.
Thanks for tracking this down.
Ben, is there any chance to get this driver into next?
Niko
^ permalink raw reply [flat|nested] 39+ messages in thread* RE: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-16 7:30 ` Voss, Nikolaus
0 siblings, 0 replies; 39+ messages in thread
From: Voss, Nikolaus @ 2012-04-16 7:30 UTC (permalink / raw)
To: 'Hubert Feurstein'
Cc: 'rmallon@gmail.com', 'nicolas.ferre@atmel.com',
'linux-kernel@vger.kernel.org', 'balbi@ti.com',
'linux-i2c@vger.kernel.org',
'ben-linux@fluff.org',
'linux-arm-kernel@lists.infradead.org'
Hubert Feurstein wrote on 2012-04-13:
> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
> which causes a data-loss on the current transfer
Right, this is definitely a bug and must be corrected. Part of my
motivation for exclusively or-ing the irq bits was not reading/
writing beyond the buffer because of (still) pending bits despite
of an already finished transfer, so I gave TXCOMP the highest prio.
Because of other reasons, write_next_byte() already checks this and
does nothing if all data already has been written. My suggestion is
to have read_next_byte() do this check too, as I don't trust the
hardware to reset RXRDY _immediately_ after reading.
> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
> *dev_id)
> {
> struct at91_twi_dev *dev = dev_id;
> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
> +
> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
The above line should be unnecessary as no more than those interrupts
are enabled anyway. Any special reason for this?
> + if (!irqstatus)
> + return IRQ_NONE;
> +
> + if (irqstatus & AT91_TWI_RXRDY)
> + at91_twi_read_next_byte(dev);
> +
> + if (irqstatus & AT91_TWI_TXRDY)
> + at91_twi_write_next_byte(dev);
I would like to exclusively or TXRDY and RXRDY as those really should
not be active at the same time. Keeps the decision tree lean ;-).
> @@ -189,6 +193,10 @@ static int
> at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev->msg->flags &
> I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
> + /* clear any pending data */
> + (void)at91_twi_read(dev, AT91_TWI_SR);
> + (void)at91_twi_read(dev, AT91_TWI_RHR);
I would like to modify this, as this is a partial fix for the above bug
which should already be fully fixed by the modified isr.
I fear subtle data-loss if we make (partial) tabula rasa before each
transfer. I'd rather add an assertion to check if the corresponding
irqs are active as an indication for a driver/hw-bug.
I'll repost the driver with your fix on positive feedback from you.
Thanks for tracking this down.
Ben, is there any chance to get this driver into next?
Niko
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-16 9:27 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-16 9:27 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: linux-arm-kernel, linux-kernel, linux-i2c, nicolas.ferre,
ben-linux, balbi, rmallon
Am 16. April 2012 09:30 schrieb Voss, Nikolaus <N.Voss@weinmann.de>:
> Hubert Feurstein wrote on 2012-04-13:
>> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
>> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
>> which causes a data-loss on the current transfer
>
> Right, this is definitely a bug and must be corrected. Part of my
> motivation for exclusively or-ing the irq bits was not reading/
> writing beyond the buffer because of (still) pending bits despite
> of an already finished transfer, so I gave TXCOMP the highest prio.
>
> Because of other reasons, write_next_byte() already checks this and
> does nothing if all data already has been written. My suggestion is
> to have read_next_byte() do this check too, as I don't trust the
> hardware to reset RXRDY _immediately_ after reading.
Adding a check in read_next_byte() would be good just for safety.
>
>> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
>> *dev_id)
>> {
>> struct at91_twi_dev *dev = dev_id;
>> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
>> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> +
>> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
>
> The above line should be unnecessary as no more than those interrupts
> are enabled anyway. Any special reason for this?
No special reason for this.
>
>> + if (!irqstatus)
>> + return IRQ_NONE;
>> +
>> + if (irqstatus & AT91_TWI_RXRDY)
>> + at91_twi_read_next_byte(dev);
>> +
>> + if (irqstatus & AT91_TWI_TXRDY)
>> + at91_twi_write_next_byte(dev);
>
> I would like to exclusively or TXRDY and RXRDY as those really should
> not be active at the same time. Keeps the decision tree lean ;-).
I agree, it should be save to xor at least those two.
>
>> @@ -189,6 +193,10 @@ static int
>> at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev->msg->flags &
>> I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
>> + /* clear any pending data */
>> + (void)at91_twi_read(dev, AT91_TWI_SR);
>> + (void)at91_twi_read(dev, AT91_TWI_RHR);
>
> I would like to modify this, as this is a partial fix for the above bug
> which should already be fully fixed by the modified isr.
> I fear subtle data-loss if we make (partial) tabula rasa before each
> transfer. I'd rather add an assertion to check if the corresponding
> irqs are active as an indication for a driver/hw-bug.
You also can add both, print an error/warning if the state in SR is
not as expected and then add the two recovery lines.
>
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
>
> Niko
>
>
Hubert
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-16 9:27 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-16 9:27 UTC (permalink / raw)
To: linux-arm-kernel
Am 16. April 2012 09:30 schrieb Voss, Nikolaus <N.Voss@weinmann.de>:
> Hubert Feurstein wrote on 2012-04-13:
>> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
>> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
>> which causes a data-loss on the current transfer
>
> Right, this is definitely a bug and must be corrected. Part of my
> motivation for exclusively or-ing the irq bits was not reading/
> writing beyond the buffer because of (still) pending bits despite
> of an already finished transfer, so I gave TXCOMP the highest prio.
>
> Because of other reasons, write_next_byte() already checks this and
> does nothing if all data already has been written. My suggestion is
> to have read_next_byte() do this check too, as I don't trust the
> hardware to reset RXRDY _immediately_ after reading.
Adding a check in read_next_byte() would be good just for safety.
>
>> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
>> *dev_id)
>> ?{
>> ? ? ? struct at91_twi_dev *dev = dev_id;
>> ? ? ? const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
>> - ? ? const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> + ? ? unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> +
>> + ? ? irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
>
> The above line should be unnecessary as no more than those interrupts
> are enabled anyway. Any special reason for this?
No special reason for this.
>
>> + ? ? if (!irqstatus)
>> + ? ? ? ? ? ? return IRQ_NONE;
>> +
>> + ? ? if (irqstatus & AT91_TWI_RXRDY)
>> + ? ? ? ? ? ? at91_twi_read_next_byte(dev);
>> +
>> + ? ? if (irqstatus & AT91_TWI_TXRDY)
>> + ? ? ? ? ? ? at91_twi_write_next_byte(dev);
>
> I would like to exclusively or TXRDY and RXRDY as those really should
> not be active at the same time. Keeps the decision tree lean ;-).
I agree, it should be save to xor at least those two.
>
>> @@ -189,6 +193,10 @@ static int
>> ?at91_do_twi_transfer(struct at91_twi_dev *dev) ? ? ? if (dev->msg->flags &
>> ?I2C_M_RD) { ? ? ? ? ?unsigned start_flags = AT91_TWI_START;
>> + ? ? ? ? ? ? /* clear any pending data */
>> + ? ? ? ? ? ? (void)at91_twi_read(dev, AT91_TWI_SR);
>> + ? ? ? ? ? ? (void)at91_twi_read(dev, AT91_TWI_RHR);
>
> I would like to modify this, as this is a partial fix for the above bug
> which should already be fully fixed by the modified isr.
> I fear subtle data-loss if we make (partial) tabula rasa before each
> transfer. I'd rather add an assertion to check if the corresponding
> irqs are active as an indication for a driver/hw-bug.
You also can add both, print an error/warning if the state in SR is
not as expected and then add the two recovery lines.
>
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
>
> Niko
>
>
Hubert
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-16 9:27 ` Hubert Feurstein
0 siblings, 0 replies; 39+ messages in thread
From: Hubert Feurstein @ 2012-04-16 9:27 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA,
nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0,
rmallon-Re5JQEeQqe8AvxtiuMwx3w
Am 16. April 2012 09:30 schrieb Voss, Nikolaus <N.Voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>:
> Hubert Feurstein wrote on 2012-04-13:
>> In the interrupt handler both status-flags (TXCOMP and RXRDY) might be
>> pending at the same time. In this case TXCOMP is handled but NOT RXRDY
>> which causes a data-loss on the current transfer
>
> Right, this is definitely a bug and must be corrected. Part of my
> motivation for exclusively or-ing the irq bits was not reading/
> writing beyond the buffer because of (still) pending bits despite
> of an already finished transfer, so I gave TXCOMP the highest prio.
>
> Because of other reasons, write_next_byte() already checks this and
> does nothing if all data already has been written. My suggestion is
> to have read_next_byte() do this check too, as I don't trust the
> hardware to reset RXRDY _immediately_ after reading.
Adding a check in read_next_byte() would be good just for safety.
>
>> @@ -161,18 +161,22 @@ static irqreturn_t atmel_twi_interrupt(int irq, void
>> *dev_id)
>> {
>> struct at91_twi_dev *dev = dev_id;
>> const unsigned status = at91_twi_read(dev, AT91_TWI_SR);
>> - const unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> + unsigned irqstatus = status & at91_twi_read(dev, AT91_TWI_IMR);
>> +
>> + irqstatus &= (AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_TXCOMP);
>
> The above line should be unnecessary as no more than those interrupts
> are enabled anyway. Any special reason for this?
No special reason for this.
>
>> + if (!irqstatus)
>> + return IRQ_NONE;
>> +
>> + if (irqstatus & AT91_TWI_RXRDY)
>> + at91_twi_read_next_byte(dev);
>> +
>> + if (irqstatus & AT91_TWI_TXRDY)
>> + at91_twi_write_next_byte(dev);
>
> I would like to exclusively or TXRDY and RXRDY as those really should
> not be active at the same time. Keeps the decision tree lean ;-).
I agree, it should be save to xor at least those two.
>
>> @@ -189,6 +193,10 @@ static int
>> at91_do_twi_transfer(struct at91_twi_dev *dev) if (dev->msg->flags &
>> I2C_M_RD) { unsigned start_flags = AT91_TWI_START;
>> + /* clear any pending data */
>> + (void)at91_twi_read(dev, AT91_TWI_SR);
>> + (void)at91_twi_read(dev, AT91_TWI_RHR);
>
> I would like to modify this, as this is a partial fix for the above bug
> which should already be fully fixed by the modified isr.
> I fear subtle data-loss if we make (partial) tabula rasa before each
> transfer. I'd rather add an assertion to check if the corresponding
> irqs are active as an indication for a driver/hw-bug.
You also can add both, print an error/warning if the state in SR is
not as expected and then add the two recovery lines.
>
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
>
> Niko
>
>
Hubert
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] i2c-at91: fix data-loss issue
2012-04-16 7:30 ` Voss, Nikolaus
(?)
@ 2012-04-18 14:39 ` Wolfram Sang
-1 siblings, 0 replies; 39+ messages in thread
From: Wolfram Sang @ 2012-04-18 14:39 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'Hubert Feurstein', 'rmallon@gmail.com',
'nicolas.ferre@atmel.com',
'linux-kernel@vger.kernel.org', 'balbi@ti.com',
'linux-i2c@vger.kernel.org',
'ben-linux@fluff.org',
'linux-arm-kernel@lists.infradead.org'
[-- Attachment #1: Type: text/plain, Size: 502 bytes --]
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
I think sending a new series would be good. It depends a bit on the amount
of donated tags (Tested-by, especially) if it will make it into 3.5.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-18 14:39 ` Wolfram Sang
0 siblings, 0 replies; 39+ messages in thread
From: Wolfram Sang @ 2012-04-18 14:39 UTC (permalink / raw)
To: linux-arm-kernel
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
I think sending a new series would be good. It depends a bit on the amount
of donated tags (Tested-by, especially) if it will make it into 3.5.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120418/133c3334/attachment.sig>
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-18 14:39 ` Wolfram Sang
0 siblings, 0 replies; 39+ messages in thread
From: Wolfram Sang @ 2012-04-18 14:39 UTC (permalink / raw)
To: Voss, Nikolaus
Cc: 'Hubert Feurstein', 'rmallon@gmail.com',
'nicolas.ferre@atmel.com',
'linux-kernel@vger.kernel.org', 'balbi@ti.com',
'linux-i2c@vger.kernel.org',
'ben-linux@fluff.org',
'linux-arm-kernel@lists.infradead.org'
[-- Attachment #1: Type: text/plain, Size: 502 bytes --]
> I'll repost the driver with your fix on positive feedback from you.
> Thanks for tracking this down.
>
> Ben, is there any chance to get this driver into next?
I think sending a new series would be good. It depends a bit on the amount
of donated tags (Tested-by, especially) if it will make it into 3.5.
Regards,
Wolfram
--
Pengutronix e.K. | Wolfram Sang |
Industrial Linux Solutions | http://www.pengutronix.de/ |
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply [flat|nested] 39+ messages in thread
* Re: [PATCH] i2c-at91: fix data-loss issue
2012-04-18 14:39 ` Wolfram Sang
(?)
(?)
@ 2012-04-21 19:33 ` Adrian Yanes
2012-04-23 5:39 ` Voss, Nikolaus
-1 siblings, 1 reply; 39+ messages in thread
From: Adrian Yanes @ 2012-04-21 19:33 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-i2c, linux-arm-kernel
Hello,
we performed some measurements with the new version of the patch (v10),
in order to verify that the twi clock was performing better with the new
ternary operator introduced for the calculation of CLDIV & CHDIV.
However, we noticed we were using the wrongs resistors in our setup,
changing them to the correct ones, the driver gives us a frequency rate
of ~396kHz when we tweak the TWI_CLK_HZ to 400000. Using the previous
version the driver (without the specific offset for the AT91RM9200) it
gives a frequency rate of ~392kHz. Therefore, both rates are inside of
the error margin that the oscilloscope can give, so we suggest to keep
the latest code introduced for it, mostly because it behaves as the
datasheet claims.
On the other hand we found that the Underrun Error (UNRE) is not handled
in the driver. When we send data up > 2-4 bytes (quite random to say
when it really fails) and we add some dev_dbg() to print
dev->transfer_status we get 141 (==UNRE). According with the datasheet,
after the first UNRE received "this action automatically generated a
STOP bit in Master Mode. Reset by read in TWI_SR when TXCOMP is set."
We thought that one possibility for this was that the board was too slow
to process the requests or that other interrupts were interfering.
Disabling the interrupts inside of the twi interrupt handler did not
help either.
The datasheet does not mention any method to implement some mechanism to
avoid the UNRE telling the hardware to wait a bit longer for the next
byte. Thus, one way will be to restart the transfer with the remaining
bytes (maybe only applicable to at91rm9200) or just to return some
error/message to userland informing that could not send all the data.
Adrian
^ permalink raw reply [flat|nested] 39+ messages in thread
* RE: [PATCH] i2c-at91: fix data-loss issue
2012-04-21 19:33 ` Adrian Yanes
2012-04-23 5:39 ` Voss, Nikolaus
@ 2012-04-23 5:39 ` Voss, Nikolaus
0 siblings, 0 replies; 39+ messages in thread
From: Voss, Nikolaus @ 2012-04-23 5:39 UTC (permalink / raw)
To: 'devel@ayanes.com', 'Wolfram Sang',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'linux-i2c@vger.kernel.org'
Cc: 'Hubert Feurstein', 'rmallon@gmail.com',
'nicolas.ferre@atmel.com', 'balbi@ti.com',
'ben-linux@fluff.org'
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1962 bytes --]
Adrian Yanes wrote on 2012-04-21:
> On the other hand we found that the Underrun Error (UNRE) is not handled
> in the driver. When we send data up > 2-4 bytes (quite random to say
> when it really fails) and we add some dev_dbg() to print
> dev->transfer_status we get 141 (==UNRE). According with the datasheet,
> after the first UNRE received "this action automatically generated a
> STOP bit in Master Mode. Reset by read in TWI_SR when TXCOMP is set."
>
> We thought that one possibility for this was that the board was too slow
> to process the requests or that other interrupts were interfering.
> Disabling the interrupts inside of the twi interrupt handler did not
> help either.
>
> The datasheet does not mention any method to implement some mechanism to
> avoid the UNRE telling the hardware to wait a bit longer for the next
> byte. Thus, one way will be to restart the transfer with the remaining
> bytes (maybe only applicable to at91rm9200) or just to return some
> error/message to userland informing that could not send all the data.
The latter is probably the easiest and most transparent solution.
There is no UNRE on G45, it just pauses the clock on an underrun
condition.
So in case UNRE is set, EIO should be returned similar to the already
handled OVRE:
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index a6f9e73..a84e19b 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
dev_err(dev->dev, "overrun while reading\n");
return -EIO;
}
+ if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
+ dev_err(dev->dev, "underrun while writing\n");
+ return -EIO;
+ }
+
dev_dbg(dev->dev, "transfer complete\n");
return 0;
ÿôèº{.nÇ+‰·Ÿ®‰†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a¢f£¢·hšïêÿ‘êçz_è®\x03(階ŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨èÚ&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-23 5:39 ` Voss, Nikolaus
0 siblings, 0 replies; 39+ messages in thread
From: Voss, Nikolaus @ 2012-04-23 5:39 UTC (permalink / raw)
To: linux-arm-kernel
Adrian Yanes wrote on 2012-04-21:
> On the other hand we found that the Underrun Error (UNRE) is not handled
> in the driver. When we send data up > 2-4 bytes (quite random to say
> when it really fails) and we add some dev_dbg() to print
> dev->transfer_status we get 141 (==UNRE). According with the datasheet,
> after the first UNRE received "this action automatically generated a
> STOP bit in Master Mode. Reset by read in TWI_SR when TXCOMP is set."
>
> We thought that one possibility for this was that the board was too slow
> to process the requests or that other interrupts were interfering.
> Disabling the interrupts inside of the twi interrupt handler did not
> help either.
>
> The datasheet does not mention any method to implement some mechanism to
> avoid the UNRE telling the hardware to wait a bit longer for the next
> byte. Thus, one way will be to restart the transfer with the remaining
> bytes (maybe only applicable to at91rm9200) or just to return some
> error/message to userland informing that could not send all the data.
The latter is probably the easiest and most transparent solution.
There is no UNRE on G45, it just pauses the clock on an underrun
condition.
So in case UNRE is set, EIO should be returned similar to the already
handled OVRE:
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index a6f9e73..a84e19b 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
dev_err(dev->dev, "overrun while reading\n");
return -EIO;
}
+ if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
+ dev_err(dev->dev, "underrun while writing\n");
+ return -EIO;
+ }
+
dev_dbg(dev->dev, "transfer complete\n");
return 0;
^ permalink raw reply related [flat|nested] 39+ messages in thread* RE: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-23 5:39 ` Voss, Nikolaus
0 siblings, 0 replies; 39+ messages in thread
From: Voss, Nikolaus @ 2012-04-23 5:39 UTC (permalink / raw)
To: 'devel@ayanes.com', 'Wolfram Sang',
'linux-arm-kernel@lists.infradead.org',
'linux-kernel@vger.kernel.org',
'linux-i2c@vger.kernel.org'
Cc: 'Hubert Feurstein', 'ben-linux@fluff.org',
'nicolas.ferre@atmel.com', 'rmallon@gmail.com',
'balbi@ti.com'
Adrian Yanes wrote on 2012-04-21:
> On the other hand we found that the Underrun Error (UNRE) is not handled
> in the driver. When we send data up > 2-4 bytes (quite random to say
> when it really fails) and we add some dev_dbg() to print
> dev->transfer_status we get 141 (==UNRE). According with the datasheet,
> after the first UNRE received "this action automatically generated a
> STOP bit in Master Mode. Reset by read in TWI_SR when TXCOMP is set."
>
> We thought that one possibility for this was that the board was too slow
> to process the requests or that other interrupts were interfering.
> Disabling the interrupts inside of the twi interrupt handler did not
> help either.
>
> The datasheet does not mention any method to implement some mechanism to
> avoid the UNRE telling the hardware to wait a bit longer for the next
> byte. Thus, one way will be to restart the transfer with the remaining
> bytes (maybe only applicable to at91rm9200) or just to return some
> error/message to userland informing that could not send all the data.
The latter is probably the easiest and most transparent solution.
There is no UNRE on G45, it just pauses the clock on an underrun
condition.
So in case UNRE is set, EIO should be returned similar to the already
handled OVRE:
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index a6f9e73..a84e19b 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
dev_err(dev->dev, "overrun while reading\n");
return -EIO;
}
+ if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
+ dev_err(dev->dev, "underrun while writing\n");
+ return -EIO;
+ }
+
dev_dbg(dev->dev, "transfer complete\n");
return 0;
^ permalink raw reply related [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
2012-04-23 5:39 ` Voss, Nikolaus
(?)
@ 2012-04-23 6:24 ` Adrian Yanes
-1 siblings, 0 replies; 39+ messages in thread
From: Adrian Yanes @ 2012-04-23 6:24 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, linux-i2c, linux-kernel, linux-i2c
> The latter is probably the easiest and most transparent solution.
> There is no UNRE on G45, it just pauses the clock on an underrun
> condition.
>
> So in case UNRE is set, EIO should be returned similar to the already
> handled OVRE:
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index a6f9e73..a84e19b 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> dev_err(dev->dev, "overrun while reading\n");
> return -EIO;
> }
> + if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
> + dev_err(dev->dev, "underrun while writing\n");
> + return -EIO;
> + }
> +
> dev_dbg(dev->dev, "transfer complete\n");
>
> return 0;
Indeed, this should be added in order to catch this exception for the
AT91RM9200.
However, the main issue still there: the board is not able to deliver
data up to 2 bytes size without to run in the UNRE case.
Measuring the i2c bus, we verified that the data is not even arriving to
the data bus, i.e. either the kernel driver is too slow to handle it or
just we are missing some tweak required by the hardware
(nevertheless the datasheet does not give any clue).
Anyone with a AT91RM9200 that can test the proposed driver as well? it
will discard that is our hardware/board the issue rather than the
chipset itself.
Adrian
^ permalink raw reply [flat|nested] 39+ messages in thread* [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-23 6:24 ` Adrian Yanes
0 siblings, 0 replies; 39+ messages in thread
From: Adrian Yanes @ 2012-04-23 6:24 UTC (permalink / raw)
To: linux-arm-kernel
> The latter is probably the easiest and most transparent solution.
> There is no UNRE on G45, it just pauses the clock on an underrun
> condition.
>
> So in case UNRE is set, EIO should be returned similar to the already
> handled OVRE:
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index a6f9e73..a84e19b 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> dev_err(dev->dev, "overrun while reading\n");
> return -EIO;
> }
> + if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
> + dev_err(dev->dev, "underrun while writing\n");
> + return -EIO;
> + }
> +
> dev_dbg(dev->dev, "transfer complete\n");
>
> return 0;
Indeed, this should be added in order to catch this exception for the
AT91RM9200.
However, the main issue still there: the board is not able to deliver
data up to 2 bytes size without to run in the UNRE case.
Measuring the i2c bus, we verified that the data is not even arriving to
the data bus, i.e. either the kernel driver is too slow to handle it or
just we are missing some tweak required by the hardware
(nevertheless the datasheet does not give any clue).
Anyone with a AT91RM9200 that can test the proposed driver as well? it
will discard that is our hardware/board the issue rather than the
chipset itself.
Adrian
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
@ 2012-04-23 6:24 ` Adrian Yanes
0 siblings, 0 replies; 39+ messages in thread
From: Adrian Yanes @ 2012-04-23 6:24 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, linux-i2c
> The latter is probably the easiest and most transparent solution.
> There is no UNRE on G45, it just pauses the clock on an underrun
> condition.
>
> So in case UNRE is set, EIO should be returned similar to the already
> handled OVRE:
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index a6f9e73..a84e19b 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -238,6 +238,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> dev_err(dev->dev, "overrun while reading\n");
> return -EIO;
> }
> + if (dev->transfer_status & AT91_TWI_UNRE && dev->is_rm9200) {
> + dev_err(dev->dev, "underrun while writing\n");
> + return -EIO;
> + }
> +
> dev_dbg(dev->dev, "transfer complete\n");
>
> return 0;
Indeed, this should be added in order to catch this exception for the
AT91RM9200.
However, the main issue still there: the board is not able to deliver
data up to 2 bytes size without to run in the UNRE case.
Measuring the i2c bus, we verified that the data is not even arriving to
the data bus, i.e. either the kernel driver is too slow to handle it or
just we are missing some tweak required by the hardware
(nevertheless the datasheet does not give any clue).
Anyone with a AT91RM9200 that can test the proposed driver as well? it
will discard that is our hardware/board the issue rather than the
chipset itself.
Adrian
^ permalink raw reply [flat|nested] 39+ messages in thread* Re: [PATCH] i2c-at91: fix data-loss issue
2012-04-23 6:24 ` Adrian Yanes
(?)
(?)
@ 2012-04-25 5:26 ` Adrian Yanes
-1 siblings, 0 replies; 39+ messages in thread
From: Adrian Yanes @ 2012-04-25 5:26 UTC (permalink / raw)
To: linux-kernel; +Cc: linux-arm-kernel, linux-i2c
Apparently the issue with the AT91RM9200 is the chipset itself due to a
factory design defect. Some people claim that the root cause of it is:
"Since the TWI doesn't have a DMA channel there is no way to deliver
timely data to the Tx buffer under heavy CPU load."[1]
Other threads[2] highlights same issues with the AT91RM9200 and the data
loss. Even in 2007, Ronny Nilsson tried to do some patch[3] blocking the
interrupts in order to win more feasibility, some testing was
performed[4], and up to 100kHz (exactly the same value as we started
getting errors), the driver just failed. At the end of the discussion
the conclusion was that the TWI controller of the AT91RM9200 is just
broken and the GPIO driver is the only alternative.
Therefore, your driver will work for all of those chipsets using PDC
with the TWI. However, it won't work with the AT91RM9200 (and probably
old hardware variants that keep same hardware design).
So I recommend to add a note/comment indicating such incompatibility for
the AT91RM9200, either in the source code or in the kernel config.
Thanks.
Adrian
1 - http://lists.atrpms.net/pipermail/i2c/2007-October/002026.html
2 - http://lists.lm-sensors.org/pipermail/i2c/2007-February/000773.html
3 - http://lists.lm-sensors.org/pipermail/i2c/2007-February/000833.html
4 - http://lists.atrpms.net/pipermail/i2c/2007-October/002034.html
^ permalink raw reply [flat|nested] 39+ messages in thread
* [PATCH v9 2/4] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
@ 2011-11-08 11:09 ` Nikolaus Voss
0 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 11:09 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
The old driver used con_id clock entries. Convert to use dev_id
for clock lookup via standard method.
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
arch/arm/mach-at91/at91cap9.c | 1 +
arch/arm/mach-at91/at91rm9200.c | 1 +
arch/arm/mach-at91/at91sam9260.c | 1 +
arch/arm/mach-at91/at91sam9261.c | 1 +
arch/arm/mach-at91/at91sam9263.c | 1 +
arch/arm/mach-at91/at91sam9g45.c | 2 ++
arch/arm/mach-at91/at91sam9rl.c | 2 ++
7 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-at91/at91cap9.c b/arch/arm/mach-at91/at91cap9.c
index a42edc2..676d6cf 100644
--- a/arch/arm/mach-at91/at91cap9.c
+++ b/arch/arm/mach-at91/at91cap9.c
@@ -219,6 +219,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
CLKDEV_CON_ID("pioA", &pioABCD_clk),
diff --git a/arch/arm/mach-at91/at91rm9200.c b/arch/arm/mach-at91/at91rm9200.c
index 99c3174..7c597e5 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -194,6 +194,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c
index d4036ba..e21a3dc 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -200,6 +200,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t1_clk", "atmel_tcb.1", &tc4_clk),
CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.1", &tc5_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* more usart lookup table for DT entries */
CLKDEV_CON_DEV_ID("usart", "fffff200.serial", &mck),
CLKDEV_CON_DEV_ID("usart", "fffb0000.serial", &usart0_clk),
diff --git a/arch/arm/mach-at91/at91sam9261.c b/arch/arm/mach-at91/at91sam9261.c
index 023c2ff..b84d882 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -175,6 +175,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &hck0),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
CLKDEV_CON_ID("pioB", &pioB_clk),
CLKDEV_CON_ID("pioC", &pioC_clk),
diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c
index 75e876c..e6e6a80 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -190,6 +190,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.0", &spi0_clk),
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index 1cb6a96..9e7ca32 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -220,6 +220,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb0_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.1", &tcb0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID(NULL, "atmel-trng", &trng_clk),
diff --git a/arch/arm/mach-at91/at91sam9rl.c b/arch/arm/mach-at91/at91sam9rl.c
index d2c91a8..2c88b94 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -183,6 +183,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.0", &tc2_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
CLKDEV_CON_ID("pioB", &pioB_clk),
CLKDEV_CON_ID("pioC", &pioC_clk),
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v9 2/4] Replace clk_lookup.con_id with clk_lookup.dev_id entries for twi clk
@ 2011-11-08 11:09 ` Nikolaus Voss
0 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 11:09 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
linux-i2c-u79uwXL29TY76Z2rM5mHXA
Cc: nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w,
ben-linux-elnMNo+KYs3YtjvyW6yDsg, balbi-l0cyMroinI0,
h.feurstein-Re5JQEeQqe8AvxtiuMwx3w,
rmallon-Re5JQEeQqe8AvxtiuMwx3w
The old driver used con_id clock entries. Convert to use dev_id
for clock lookup via standard method.
Signed-off-by: Nikolaus Voss <n.voss-+umVssTZoCsb1SvskN2V4Q@public.gmane.org>
---
arch/arm/mach-at91/at91cap9.c | 1 +
arch/arm/mach-at91/at91rm9200.c | 1 +
arch/arm/mach-at91/at91sam9260.c | 1 +
arch/arm/mach-at91/at91sam9261.c | 1 +
arch/arm/mach-at91/at91sam9263.c | 1 +
arch/arm/mach-at91/at91sam9g45.c | 2 ++
arch/arm/mach-at91/at91sam9rl.c | 2 ++
7 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/arch/arm/mach-at91/at91cap9.c b/arch/arm/mach-at91/at91cap9.c
index a42edc2..676d6cf 100644
--- a/arch/arm/mach-at91/at91cap9.c
+++ b/arch/arm/mach-at91/at91cap9.c
@@ -219,6 +219,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
CLKDEV_CON_ID("pioA", &pioABCD_clk),
diff --git a/arch/arm/mach-at91/at91rm9200.c b/arch/arm/mach-at91/at91rm9200.c
index 99c3174..7c597e5 100644
--- a/arch/arm/mach-at91/at91rm9200.c
+++ b/arch/arm/mach-at91/at91rm9200.c
@@ -194,6 +194,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
diff --git a/arch/arm/mach-at91/at91sam9260.c b/arch/arm/mach-at91/at91sam9260.c
index d4036ba..e21a3dc 100644
--- a/arch/arm/mach-at91/at91sam9260.c
+++ b/arch/arm/mach-at91/at91sam9260.c
@@ -200,6 +200,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t1_clk", "atmel_tcb.1", &tc4_clk),
CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.1", &tc5_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* more usart lookup table for DT entries */
CLKDEV_CON_DEV_ID("usart", "fffff200.serial", &mck),
CLKDEV_CON_DEV_ID("usart", "fffb0000.serial", &usart0_clk),
diff --git a/arch/arm/mach-at91/at91sam9261.c b/arch/arm/mach-at91/at91sam9261.c
index 023c2ff..b84d882 100644
--- a/arch/arm/mach-at91/at91sam9261.c
+++ b/arch/arm/mach-at91/at91sam9261.c
@@ -175,6 +175,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.2", &ssc2_clk),
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &hck0),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
CLKDEV_CON_ID("pioB", &pioB_clk),
CLKDEV_CON_ID("pioC", &pioC_clk),
diff --git a/arch/arm/mach-at91/at91sam9263.c b/arch/arm/mach-at91/at91sam9263.c
index 75e876c..e6e6a80 100644
--- a/arch/arm/mach-at91/at91sam9263.c
+++ b/arch/arm/mach-at91/at91sam9263.c
@@ -190,6 +190,7 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.0", &spi0_clk),
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c", &twi_clk),
/* fake hclk clock */
CLKDEV_CON_DEV_ID("hclk", "at91_ohci", &ohci_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
diff --git a/arch/arm/mach-at91/at91sam9g45.c b/arch/arm/mach-at91/at91sam9g45.c
index 1cb6a96..9e7ca32 100644
--- a/arch/arm/mach-at91/at91sam9g45.c
+++ b/arch/arm/mach-at91/at91sam9g45.c
@@ -220,6 +220,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("spi_clk", "atmel_spi.1", &spi1_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.0", &tcb0_clk),
CLKDEV_CON_DEV_ID("t0_clk", "atmel_tcb.1", &tcb0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
CLKDEV_CON_DEV_ID(NULL, "atmel-trng", &trng_clk),
diff --git a/arch/arm/mach-at91/at91sam9rl.c b/arch/arm/mach-at91/at91sam9rl.c
index d2c91a8..2c88b94 100644
--- a/arch/arm/mach-at91/at91sam9rl.c
+++ b/arch/arm/mach-at91/at91sam9rl.c
@@ -183,6 +183,8 @@ static struct clk_lookup periph_clocks_lookups[] = {
CLKDEV_CON_DEV_ID("t2_clk", "atmel_tcb.0", &tc2_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.0", &ssc0_clk),
CLKDEV_CON_DEV_ID("pclk", "ssc.1", &ssc1_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.0", &twi0_clk),
+ CLKDEV_CON_DEV_ID(NULL, "at91_i2c.1", &twi1_clk),
CLKDEV_CON_ID("pioA", &pioA_clk),
CLKDEV_CON_ID("pioB", &pioB_clk),
CLKDEV_CON_ID("pioC", &pioC_clk),
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread
* [PATCH v9 4/4] G45 TWI: remove open drain setting for twi function gpios
2012-03-19 15:07 ` Nikolaus Voss
@ 2011-11-08 11:11 ` Nikolaus Voss
-1 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 11:11 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
The G45 datasheets explicitly states that setting the open drain property
on peripheral function gpios is not allowed. (How about other A91 chips?)
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
arch/arm/mach-at91/at91sam9g45_devices.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 96e2adc..050e6e2 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -685,18 +685,12 @@ void __init at91_add_device_i2c(short i2c_id, struct i2c_board_info *devices, in
/* pins used for TWI interface */
if (i2c_id == 0) {
at91_set_A_periph(AT91_PIN_PA20, 0); /* TWD */
- at91_set_multi_drive(AT91_PIN_PA20, 1);
-
at91_set_A_periph(AT91_PIN_PA21, 0); /* TWCK */
- at91_set_multi_drive(AT91_PIN_PA21, 1);
platform_device_register(&at91sam9g45_twi0_device);
} else {
at91_set_A_periph(AT91_PIN_PB10, 0); /* TWD */
- at91_set_multi_drive(AT91_PIN_PB10, 1);
-
at91_set_A_periph(AT91_PIN_PB11, 0); /* TWCK */
- at91_set_multi_drive(AT91_PIN_PB11, 1);
platform_device_register(&at91sam9g45_twi1_device);
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread* [PATCH v9 4/4] G45 TWI: remove open drain setting for twi function gpios
@ 2011-11-08 11:11 ` Nikolaus Voss
0 siblings, 0 replies; 39+ messages in thread
From: Nikolaus Voss @ 2011-11-08 11:11 UTC (permalink / raw)
To: linux-arm-kernel, linux-kernel, linux-i2c
Cc: nicolas.ferre, ben-linux, balbi, h.feurstein, rmallon
The G45 datasheets explicitly states that setting the open drain property
on peripheral function gpios is not allowed. (How about other A91 chips?)
Signed-off-by: Nikolaus Voss <n.voss@weinmann.de>
---
arch/arm/mach-at91/at91sam9g45_devices.c | 6 ------
1 files changed, 0 insertions(+), 6 deletions(-)
diff --git a/arch/arm/mach-at91/at91sam9g45_devices.c b/arch/arm/mach-at91/at91sam9g45_devices.c
index 96e2adc..050e6e2 100644
--- a/arch/arm/mach-at91/at91sam9g45_devices.c
+++ b/arch/arm/mach-at91/at91sam9g45_devices.c
@@ -685,18 +685,12 @@ void __init at91_add_device_i2c(short i2c_id, struct i2c_board_info *devices, in
/* pins used for TWI interface */
if (i2c_id == 0) {
at91_set_A_periph(AT91_PIN_PA20, 0); /* TWD */
- at91_set_multi_drive(AT91_PIN_PA20, 1);
-
at91_set_A_periph(AT91_PIN_PA21, 0); /* TWCK */
- at91_set_multi_drive(AT91_PIN_PA21, 1);
platform_device_register(&at91sam9g45_twi0_device);
} else {
at91_set_A_periph(AT91_PIN_PB10, 0); /* TWD */
- at91_set_multi_drive(AT91_PIN_PB10, 1);
-
at91_set_A_periph(AT91_PIN_PB11, 0); /* TWCK */
- at91_set_multi_drive(AT91_PIN_PB11, 1);
platform_device_register(&at91sam9g45_twi1_device);
}
--
1.7.5.4
^ permalink raw reply related [flat|nested] 39+ messages in thread