* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
@ 2010-08-25 14:23 Albert Aribaud
2010-08-25 14:23 ` [U-Boot] [PATCH 2/2] edminiv2: add I2C support using mvtwsi driver Albert Aribaud
` (3 more replies)
0 siblings, 4 replies; 13+ messages in thread
From: Albert Aribaud @ 2010-08-25 14:23 UTC (permalink / raw)
To: u-boot
This driver is for the Marvell TWSI/I2C module found in
the orion and kirkwood families among others.
Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
While the 'kirkwood_i2c' driver for the Marvell TWSI module
is already available in u-boot, this one is 25% smaller, less
complex (no state machine) and much faster (i2c probe on an
ED Mini V2 takes no noticeable time vs. half a second).
drivers/i2c/Makefile | 1 +
drivers/i2c/mvtwsi.c | 419 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 420 insertions(+), 0 deletions(-)
create mode 100644 drivers/i2c/mvtwsi.c
diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
index d2c2515..73c415d 100644
--- a/drivers/i2c/Makefile
+++ b/drivers/i2c/Makefile
@@ -30,6 +30,7 @@ COBJS-$(CONFIG_DRIVER_DAVINCI_I2C) += davinci_i2c.o
COBJS-$(CONFIG_FSL_I2C) += fsl_i2c.o
COBJS-$(CONFIG_I2C_KIRKWOOD) += kirkwood_i2c.o
COBJS-$(CONFIG_I2C_MXC) += mxc_i2c.o
+COBJS-$(CONFIG_I2C_DRIVER_MVTWSI) += mvtwsi.o
COBJS-$(CONFIG_DRIVER_OMAP1510_I2C) += omap1510_i2c.o
COBJS-$(CONFIG_DRIVER_OMAP24XX_I2C) += omap24xx_i2c.o
COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += omap24xx_i2c.o
diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
new file mode 100644
index 0000000..b591328
--- /dev/null
+++ b/drivers/i2c/mvtwsi.c
@@ -0,0 +1,419 @@
+/*
+ * Driver for the TWSI (i2c) controller on the Marvell orion5x
+ *
+ * Author: Albert Aribaud <albert.aribaud@free.fr>
+ * 2005 (c) MontaVista, Software, Inc.
+ *
+ * See file CREDITS for list of people who contributed to this
+ * project.
+ *
+ * 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.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
+ * MA 02110-1301 USA
+ */
+
+#include <common.h>
+#include <i2c.h>
+#include <asm/errno.h>
+#include <asm/io.h>
+
+/*
+ * include a file that will provide CONFIG_I2C_MVTWSI_BASE
+ * and possibly other settings
+ */
+
+#if defined(CONFIG_ORION5X)
+#include <asm/arch/orion5x.h>
+#else
+#error Driver mvtwsi not supported by SoC or board
+#endif
+
+/*
+ * TWSI register structure
+ */
+
+struct mvtwsi_registers {
+ u32 slave_address;
+ u32 data;
+ u32 control;
+ union {
+ u32 status; /* when reading */
+ u32 baudrate; /* when writing */
+ };
+ u32 extended_slave_address;
+ u32 reserved[2];
+ u32 soft_reset;
+};
+
+/*
+ * Control register fields
+ */
+
+#define MVTWSI_CONTROL_ACK 0x00000004
+#define MVTWSI_CONTROL_IFLG 0x00000008
+#define MVTWSI_CONTROL_STOP 0x00000010
+#define MVTWSI_CONTROL_START 0x00000020
+#define MVTWSI_CONTROL_TWSIEN 0x00000040
+#define MVTWSI_CONTROL_INTEN 0x00000080
+
+/*
+ * Status register values -- only those expected in normal master
+ * operation on non-10-bit-address devices; whatever status we don't
+ * expect in nominal conditions (bus errors, arbitration losses,
+ * missing ACKs...) we just pass back to the caller as an error
+ * code.
+ */
+
+#define MVTWSI_STATUS_START 0x08
+#define MVTWSI_STATUS_REPEATED_START 0x10
+#define MVTWSI_STATUS_ADDR_W_ACK 0x18
+#define MVTWSI_STATUS_DATA_W_ACK 0x28
+#define MVTWSI_STATUS_ADDR_R_ACK 0x40
+#define MVTWSI_STATUS_ADDR_R_NAK 0x48
+#define MVTWSI_STATUS_DATA_R_ACK 0x50
+#define MVTWSI_STATUS_DATA_R_NAK 0x58 /* our NAK, not the slave's */
+#define MVTWSI_STATUS_IDLE 0xF8
+
+/*
+ * The single instance of the controller we'll be dealing with
+ */
+
+static struct mvtwsi_registers *twsi =
+ (struct mvtwsi_registers *) CONFIG_I2C_MVTWSI_BASE;
+
+/*
+ * Returned statuses are 0 for success and nonzero otherwise.
+ * Currently, cmd_i2c and cmd_eeprom do not interpret an error status.
+ * Thus to ease debugging, the return status contains some debug info:
+ * - bits 31..24 are an error class: 0x01 is timeout, 0x02 is 'status mismatch'.
+ * - bits 23..16 are the last value of the control register.
+ * - bits 15..8 are the last value of the status register.
+ * - bits 7..0 are the expected value of the status register.
+ */
+
+#define MVTWSI_ERROR_WRONG_STATUS 0x01
+#define MVTWSI_ERROR_TIMEOUT 0x02
+
+#define MVTWSI_ERROR(ec,lc,ls,es) ( ( (ec << 24) & 0xFF000000) | \
+ ( (lc << 16) & 0x00FF0000) | ( (ls<<8) & 0x0000FF00) | (es & 0xFF) )
+
+/*
+ * Wait for IFLG to raise, or return 'timeout'; then if status is as expected,
+ * return 0 (ok) or return 'wrong status'.
+ */
+
+static int twsi_wait(int expected_status)
+{
+ int control,status;
+ int timeout = 1000;
+ do {
+ control = readl(&twsi->control);
+ if (control & MVTWSI_CONTROL_IFLG) {
+ status = readl(&twsi->status);
+ if (status==expected_status)
+ return 0;
+ else
+ return MVTWSI_ERROR(MVTWSI_ERROR_WRONG_STATUS,control, status,
+ expected_status);
+ }
+ udelay(10);
+ } while (timeout--);
+ status = readl(&twsi->status);
+ return MVTWSI_ERROR(MVTWSI_ERROR_TIMEOUT,control,status,expected_status);
+}
+
+/*
+ * These flags are ORed to any write to the control register
+ * They allow global setting of TWSIEN and ACK.
+ * By default none are set.
+ * twsi_start() sets TWSIEN (in case the controller was disabled)
+ * twsi_recv() sets ACK or resets it depending on expected status.
+ */
+
+static u8 twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
+
+/*
+ * Assert the START condition, either in a single I2C transaction
+ * or inside back-to-back ones (repeated starts).
+ */
+
+static int twsi_start(int expected_status)
+{
+ /* globally set TWSIEN in case it was not */
+ twsi_control_flags |= MVTWSI_CONTROL_TWSIEN;
+ /* assert START */
+ writel(twsi_control_flags | MVTWSI_CONTROL_START, &twsi->control);
+ /* wait for controller to process START */
+ return twsi_wait(expected_status);
+}
+
+/*
+ * Send a byte (i2c address or data).
+ */
+
+static int twsi_send(u8 byte, int expected_status)
+{
+ /* put byte in data register for sending */
+ writel(byte, &twsi->data);
+ /* clear any pending interrupt -- that'll cause sending */
+ writel(twsi_control_flags, &twsi->control);
+ /* wait for controller to receive byte and check ACK */
+ return twsi_wait(expected_status);
+}
+
+/*
+ * Receive a byte.
+ * Global mvtwsi_control_flags variable says if we should ack or nak.
+ */
+
+static int twsi_recv(u8 *byte)
+{
+ int expected_status, status;
+ /* compute expected status based on ACK bit in global control flags */
+ if (twsi_control_flags & MVTWSI_CONTROL_ACK)
+ expected_status = MVTWSI_STATUS_DATA_R_ACK;
+ else
+ expected_status = MVTWSI_STATUS_DATA_R_NAK;
+ /* acknowledge *previous state* and launch receive */
+ writel(twsi_control_flags, &twsi->control);
+ /* wait for controller to receive byte and assert ACK or NAK */
+ status = twsi_wait(expected_status);
+ /* if we did receive expected byte then store it */
+ if (status==0)
+ *byte = readl(&twsi->data);
+ /* return status */
+ return status;
+}
+
+/*
+ * Assert the STOP condition.
+ * This is also used to force the bus back in idle (SDA=SCL=1).
+ */
+
+static int twsi_stop(void)
+{
+ int control,status;
+ int timeout = 1000;
+ /* assert STOP */
+ control = MVTWSI_CONTROL_TWSIEN | MVTWSI_CONTROL_STOP;
+ writel(control, &twsi->control);
+ /* wait for idle condition. There won't be an IFLG so we cannot use twsi_wait(). */
+ do {
+ status = readl(&twsi->status);
+ if (status==MVTWSI_STATUS_IDLE)
+ return 0;
+ udelay(10);
+ } while (timeout--);
+ control = readl(&twsi->control);
+ return MVTWSI_ERROR(MVTWSI_ERROR_TIMEOUT,control,status,MVTWSI_STATUS_IDLE);
+}
+
+/*
+ * Ugly formula to convert m and n values to a frequency comes from
+ * TWSI specifications
+ */
+
+#define TWSI_FREQUENCY(m,n) \
+ ( (u8) (CONFIG_SYS_TCLK / (10 * (m + 1) * 2 * (1 << n))) )
+
+/*
+ * These are required to be reprogrammed before enabling the controller
+ * because a reset loses them.
+ * Default values come from the spec, but a twsi_reset will change them.
+ */
+
+static u8 twsi_baud_rate = 0x44;
+static u8 twsi_actual_speed = TWSI_FREQUENCY(4,4);
+static u8 twsi_slave_address = 0;
+
+/*
+ * Reset controller.
+ * Called@end of i2c_init unsuccessful i2c transactions.
+ * Controller reset also resets the baud rate and slave address, so
+ * re-establish them.
+ */
+
+static void twsi_reset(void)
+{
+ /* ensure controller will be enabled by any twsi*() function */
+ twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
+ /* reset controller */
+ writel(0, &twsi->soft_reset);
+ /* wait 2 ms -- this is what the Marvell LSP does */
+ udelay(20000);
+ /* set baud rate */
+ writel(twsi_baud_rate, &twsi->baudrate);
+ /* set slave address even though we don't use it */
+ writel(twsi_slave_address, &twsi->slave_address);
+ writel(0, &twsi->extended_slave_address);
+ /* assert STOP but don't care for the result */
+ (void) twsi_stop();
+}
+
+/*
+ * I2C init called by cmd_i2c when doing 'i2c reset'.
+ * Sets baud to the highest possible value not exceeding requested one.
+ */
+
+void i2c_init(int requested_speed, int slaveadd)
+{
+ int tmp_speed, highest_speed, n, m;
+ int baud = 0x44; /* value at controller reset */
+ /* use actual speed to collect progressively higher values */
+ highest_speed = 0;
+ /* compute m,n setting for highest speed not above requested speed */
+ for (n = 0; n < 8; n++) {
+ for (m = 0; m < 16; m++) {
+ tmp_speed = TWSI_FREQUENCY(m,n);
+ if (tmp_speed <= requested_speed)
+ if (tmp_speed > highest_speed) {
+ highest_speed = tmp_speed;
+ baud = (m << 3) | n;
+ }
+ }
+ }
+ /* save baud rate and slave for later calls to twsi_reset */
+ twsi_baud_rate = baud;
+ twsi_actual_speed = highest_speed;
+ twsi_slave_address = slaveadd;
+ /* reset controller */
+ twsi_reset();
+}
+
+/*
+ * Begin I2C transaction with expected start status,@given address.
+ * Common to i2c_probe, i2c_read and i2c_write.
+ * Expected address status will derive from direction bit (bit 0) in addr.
+ */
+
+static int i2c_begin(int expected_start_status, u8 addr)
+{
+ int status, expected_addr_status;
+ /* compute expected address status from direction bit in addr */
+ if (addr & 1) /* reading */
+ expected_addr_status = MVTWSI_STATUS_ADDR_R_ACK;
+ else /* writing */
+ expected_addr_status = MVTWSI_STATUS_ADDR_W_ACK;
+ /* assert START */
+ status = twsi_start(expected_start_status);
+ /* send out the address if the start went well */
+ if (status == 0)
+ status = twsi_send(addr, expected_addr_status);
+ /* return ok or status of first failure to caller */
+ return status;
+}
+
+/*
+ * End I2C transaction, of which the current status is given.
+ * Common to i2c_probe, i2c_read and i2c_write.
+ */
+
+static int i2c_end(int status)
+{
+ /* if transaction went well so far, try to assert a STOP */
+ /*if (status == 0)*/
+ twsi_stop();
+ /* if anything went wrong (including the STOP we tried), do a reset */
+ /*else
+ twsi_reset();*/
+ /* return ok or status of first failure to caller */
+ return status;
+}
+
+/*
+ * I2C probe called by cmd_i2c when doing 'i2c probe'.
+ * Begin read, nak data byte, end.
+ */
+
+int i2c_probe(uchar chip)
+{
+ u8 dummy_byte;
+ int status;
+ /* begin i2c read */
+ status = i2c_begin(MVTWSI_STATUS_START, (chip << 1) | 1 );
+ /* dummy read was accepted: receive byte but NAK it. */
+ if (status==0)
+ status = twsi_recv( &dummy_byte);
+ /* We expected either 0 (ok) or MVTWSI_STATUS_ADDR_R_NAK */
+ return i2c_end( (status==MVTWSI_STATUS_ADDR_R_NAK) ? 0: status);
+}
+
+/*
+ * I2C read called by cmd_i2c when doing 'i2c read' and by cmd_eeprom.c
+ * Begin write, send address byte(s), begin read, receive data bytes, end.
+ */
+
+int i2c_read(u8 dev, uint addr, int alen, u8 *data, int length)
+{
+ int status;
+ /* begin i2c write to send the address bytes */
+ status = i2c_begin(MVTWSI_STATUS_START, (dev << 1) );
+ /* send addr bytes */
+ while ( (status==0) && alen--)
+ status = twsi_send(addr >> (8*alen), MVTWSI_STATUS_DATA_W_ACK);
+ /* NOTE: should we send a stop before a start? That's eeprom-level stuff */
+ /* begin i2c read to receive eeprom data bytes */
+ if (status == 0)
+ status = i2c_begin(MVTWSI_STATUS_REPEATED_START, (dev << 1) | 1);
+ /* prepare ACK if@least one byte must be received */
+ if (length > 0)
+ twsi_control_flags |= MVTWSI_CONTROL_ACK;
+ /* now receive actual bytes */
+ while ( (status==0) && length-- ) {
+ /* reset NAK if we if no more to read now */
+ if (length == 0)
+ twsi_control_flags &= ~MVTWSI_CONTROL_ACK;
+ /* read current byte */
+ status = twsi_recv(data++);
+ }
+ /* end i2c transaction */
+ return i2c_end(status);
+}
+
+/*
+ * I2C write called by cmd_i2c when doing 'i2c write' and by cmd_eeprom.c
+ * Begin write, send address byte(s), send data bytes, end.
+ */
+
+int i2c_write(u8 dev, uint addr, int alen, u8 *data, int length)
+{
+ int status;
+ /* begin i2c write to send the eeprom adress bytes then data bytes */
+ status = i2c_begin(MVTWSI_STATUS_START, (dev << 1) );
+ /* send addr bytes */
+ while ( (status==0) && alen--)
+ status = twsi_send(addr >> (8*alen), MVTWSI_STATUS_DATA_W_ACK);
+ /* send data bytes */
+ while ( (status==0) && (length-->0) )
+ status = twsi_send(*(data++), MVTWSI_STATUS_DATA_W_ACK);
+ /* end i2c transaction */
+ return i2c_end(status);
+}
+
+/*
+ * Bus set/get routines: we only support bus 0.
+ */
+
+int i2c_set_bus_num(unsigned int bus)
+{
+ if (bus > 0) {
+ return -1;
+ }
+ return 0;
+}
+
+unsigned int i2c_get_bus_num(void)
+{
+ return 0;
+}
--
1.7.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 2/2] edminiv2: add I2C support using mvtwsi driver
2010-08-25 14:23 [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert Aribaud
@ 2010-08-25 14:23 ` Albert Aribaud
2010-08-25 15:28 ` [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert ARIBAUD
` (2 subsequent siblings)
3 siblings, 0 replies; 13+ messages in thread
From: Albert Aribaud @ 2010-08-25 14:23 UTC (permalink / raw)
To: u-boot
Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
include/configs/edminiv2.h | 9 +++++++++
1 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/include/configs/edminiv2.h b/include/configs/edminiv2.h
index 57dd165..36ed392 100644
--- a/include/configs/edminiv2.h
+++ b/include/configs/edminiv2.h
@@ -182,6 +182,15 @@
#endif /* CMD_IDE */
/*
+ * I2C related stuff
+ */
+#define CONFIG_I2C_DRIVER_MVTWSI
+#define CONFIG_I2C_MVTWSI_BASE ORION5X_TWSI_BASE
+#define CONFIG_SYS_I2C_SLAVE 0x0
+#define CONFIG_SYS_I2C_SPEED 100000
+#define CONFIG_CMD_I2C
+
+/*
* Environment variables configurations
*/
#define CONFIG_ENV_IS_IN_FLASH 1
--
1.7.1
^ permalink raw reply related [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-25 14:23 [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert Aribaud
2010-08-25 14:23 ` [U-Boot] [PATCH 2/2] edminiv2: add I2C support using mvtwsi driver Albert Aribaud
@ 2010-08-25 15:28 ` Albert ARIBAUD
2010-08-26 4:33 ` Prafulla Wadaskar
2010-08-26 6:30 ` Heiko Schocher
3 siblings, 0 replies; 13+ messages in thread
From: Albert ARIBAUD @ 2010-08-25 15:28 UTC (permalink / raw)
To: u-boot
Reviewing myself, as a blatant copy-paste mistake only hit my eyes right
after the patch was posted. :(
Le 25/08/2010 16:23, Albert Aribaud a ?crit :
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> @@ -0,0 +1,419 @@
> +/*
> + * Driver for the TWSI (i2c) controller on the Marvell orion5x
> + *
> + * Author: Albert Aribaud<albert.aribaud@free.fr>
> + * 2005 (c) MontaVista, Software, Inc.
This code is not (c) Montavista originally; it is a completely new
implementation, with the license notice copied from the kirkwood file.
I'll remove this line in patch V2.
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-25 14:23 [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert Aribaud
2010-08-25 14:23 ` [U-Boot] [PATCH 2/2] edminiv2: add I2C support using mvtwsi driver Albert Aribaud
2010-08-25 15:28 ` [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert ARIBAUD
@ 2010-08-26 4:33 ` Prafulla Wadaskar
2010-08-26 6:33 ` Albert ARIBAUD
2010-08-26 6:30 ` Heiko Schocher
3 siblings, 1 reply; 13+ messages in thread
From: Prafulla Wadaskar @ 2010-08-26 4:33 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: u-boot-bounces at lists.denx.de
> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
> Sent: Wednesday, August 25, 2010 7:54 PM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
>
> This driver is for the Marvell TWSI/I2C module found in
> the orion and kirkwood families among others.
>
> Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
> ---
> While the 'kirkwood_i2c' driver for the Marvell TWSI module
> is already available in u-boot, this one is 25% smaller, less
> complex (no state machine) and much faster (i2c probe on an
> ED Mini V2 takes no noticeable time vs. half a second).
Hi Albert
This will be very good enhancement indeed.
>
> drivers/i2c/Makefile | 1 +
> drivers/i2c/mvtwsi.c | 419
Can you pls follow the same strategy as we followed for mvgbe, mvsata?
Please rename and enhance current kirkwood_i2c driver support,
and then add support for Orion followed by board support for edminiv2
Regards..
Prafulla ..
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-25 14:23 [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert Aribaud
` (2 preceding siblings ...)
2010-08-26 4:33 ` Prafulla Wadaskar
@ 2010-08-26 6:30 ` Heiko Schocher
2010-08-26 6:37 ` Albert ARIBAUD
2010-08-26 22:03 ` Albert ARIBAUD
3 siblings, 2 replies; 13+ messages in thread
From: Heiko Schocher @ 2010-08-26 6:30 UTC (permalink / raw)
To: u-boot
Hello Albert,
Albert Aribaud wrote:
> This driver is for the Marvell TWSI/I2C module found in
> the orion and kirkwood families among others.
>
> Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
> ---
> While the 'kirkwood_i2c' driver for the Marvell TWSI module
> is already available in u-boot, this one is 25% smaller, less
> complex (no state machine) and much faster (i2c probe on an
> ED Mini V2 takes no noticeable time vs. half a second).
Great!
As the kirkwood_i2c driver is actually not used in any board,
we should remove it completely, can you add this for v2?
Beside of that, I have just some minor codstyling comments:
> drivers/i2c/Makefile | 1 +
> drivers/i2c/mvtwsi.c | 419 ++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 420 insertions(+), 0 deletions(-)
> create mode 100644 drivers/i2c/mvtwsi.c
>
> diff --git a/drivers/i2c/Makefile b/drivers/i2c/Makefile
> index d2c2515..73c415d 100644
> --- a/drivers/i2c/Makefile
> +++ b/drivers/i2c/Makefile
> @@ -30,6 +30,7 @@ COBJS-$(CONFIG_DRIVER_DAVINCI_I2C) += davinci_i2c.o
> COBJS-$(CONFIG_FSL_I2C) += fsl_i2c.o
> COBJS-$(CONFIG_I2C_KIRKWOOD) += kirkwood_i2c.o
> COBJS-$(CONFIG_I2C_MXC) += mxc_i2c.o
> +COBJS-$(CONFIG_I2C_DRIVER_MVTWSI) += mvtwsi.o
> COBJS-$(CONFIG_DRIVER_OMAP1510_I2C) += omap1510_i2c.o
> COBJS-$(CONFIG_DRIVER_OMAP24XX_I2C) += omap24xx_i2c.o
> COBJS-$(CONFIG_DRIVER_OMAP34XX_I2C) += omap24xx_i2c.o
> diff --git a/drivers/i2c/mvtwsi.c b/drivers/i2c/mvtwsi.c
> new file mode 100644
> index 0000000..b591328
> --- /dev/null
> +++ b/drivers/i2c/mvtwsi.c
> @@ -0,0 +1,419 @@
> +/*
> + * Driver for the TWSI (i2c) controller on the Marvell orion5x
> + *
> + * Author: Albert Aribaud <albert.aribaud@free.fr>
> + * 2005 (c) MontaVista, Software, Inc.
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * 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.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
> + * MA 02110-1301 USA
> + */
> +
> +#include <common.h>
> +#include <i2c.h>
> +#include <asm/errno.h>
> +#include <asm/io.h>
> +
> +/*
> + * include a file that will provide CONFIG_I2C_MVTWSI_BASE
> + * and possibly other settings
> + */
> +
> +#if defined(CONFIG_ORION5X)
> +#include <asm/arch/orion5x.h>
> +#else
> +#error Driver mvtwsi not supported by SoC or board
> +#endif
> +
> +/*
> + * TWSI register structure
> + */
> +
> +struct mvtwsi_registers {
> + u32 slave_address;
> + u32 data;
> + u32 control;
> + union {
> + u32 status; /* when reading */
> + u32 baudrate; /* when writing */
> + };
> + u32 extended_slave_address;
> + u32 reserved[2];
> + u32 soft_reset;
> +};
> +
> +/*
> + * Control register fields
> + */
> +
> +#define MVTWSI_CONTROL_ACK 0x00000004
> +#define MVTWSI_CONTROL_IFLG 0x00000008
> +#define MVTWSI_CONTROL_STOP 0x00000010
> +#define MVTWSI_CONTROL_START 0x00000020
> +#define MVTWSI_CONTROL_TWSIEN 0x00000040
> +#define MVTWSI_CONTROL_INTEN 0x00000080
> +
> +/*
> + * Status register values -- only those expected in normal master
> + * operation on non-10-bit-address devices; whatever status we don't
> + * expect in nominal conditions (bus errors, arbitration losses,
> + * missing ACKs...) we just pass back to the caller as an error
> + * code.
> + */
> +
> +#define MVTWSI_STATUS_START 0x08
> +#define MVTWSI_STATUS_REPEATED_START 0x10
> +#define MVTWSI_STATUS_ADDR_W_ACK 0x18
> +#define MVTWSI_STATUS_DATA_W_ACK 0x28
> +#define MVTWSI_STATUS_ADDR_R_ACK 0x40
> +#define MVTWSI_STATUS_ADDR_R_NAK 0x48
> +#define MVTWSI_STATUS_DATA_R_ACK 0x50
> +#define MVTWSI_STATUS_DATA_R_NAK 0x58 /* our NAK, not the slave's */
line too long.
> +#define MVTWSI_STATUS_IDLE 0xF8
> +
> +/*
> + * The single instance of the controller we'll be dealing with
> + */
> +
> +static struct mvtwsi_registers *twsi =
> + (struct mvtwsi_registers *) CONFIG_I2C_MVTWSI_BASE;
> +
> +/*
> + * Returned statuses are 0 for success and nonzero otherwise.
> + * Currently, cmd_i2c and cmd_eeprom do not interpret an error status.
> + * Thus to ease debugging, the return status contains some debug info:
> + * - bits 31..24 are an error class: 0x01 is timeout, 0x02 is 'status mismatch'.
> + * - bits 23..16 are the last value of the control register.
> + * - bits 15..8 are the last value of the status register.
> + * - bits 7..0 are the expected value of the status register.
> + */
> +
> +#define MVTWSI_ERROR_WRONG_STATUS 0x01
> +#define MVTWSI_ERROR_TIMEOUT 0x02
> +
> +#define MVTWSI_ERROR(ec,lc,ls,es) ( ( (ec << 24) & 0xFF000000) | \
> + ( (lc << 16) & 0x00FF0000) | ( (ls<<8) & 0x0000FF00) | (es & 0xFF) )
> +
> +/*
> + * Wait for IFLG to raise, or return 'timeout'; then if status is as expected,
> + * return 0 (ok) or return 'wrong status'.
> + */
> +
blank line not necessary.
> +static int twsi_wait(int expected_status)
> +{
> + int control,status;
> + int timeout = 1000;
please insert a blank line, check this on other places too.
> + do {
> + control = readl(&twsi->control);
> + if (control & MVTWSI_CONTROL_IFLG) {
> + status = readl(&twsi->status);
> + if (status==expected_status)
please insert a space between "=="
> + return 0;
> + else
> + return MVTWSI_ERROR(MVTWSI_ERROR_WRONG_STATUS,control, status,
line too long, also please insert a space after a ","
Please check this for the whole file.
> + expected_status);
> + }
> + udelay(10);
> + } while (timeout--);
> + status = readl(&twsi->status);
> + return MVTWSI_ERROR(MVTWSI_ERROR_TIMEOUT,control,status,expected_status);
> +}
> +
> +/*
> + * These flags are ORed to any write to the control register
> + * They allow global setting of TWSIEN and ACK.
> + * By default none are set.
> + * twsi_start() sets TWSIEN (in case the controller was disabled)
> + * twsi_recv() sets ACK or resets it depending on expected status.
> + */
> +
blank line not necessary. Please check the whole file
> +static u8 twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
> +
> +/*
> + * Assert the START condition, either in a single I2C transaction
> + * or inside back-to-back ones (repeated starts).
> + */
> +
> +static int twsi_start(int expected_status)
> +{
> + /* globally set TWSIEN in case it was not */
> + twsi_control_flags |= MVTWSI_CONTROL_TWSIEN;
> + /* assert START */
> + writel(twsi_control_flags | MVTWSI_CONTROL_START, &twsi->control);
> + /* wait for controller to process START */
> + return twsi_wait(expected_status);
> +}
> +
> +/*
> + * Send a byte (i2c address or data).
> + */
> +
> +static int twsi_send(u8 byte, int expected_status)
> +{
> + /* put byte in data register for sending */
> + writel(byte, &twsi->data);
> + /* clear any pending interrupt -- that'll cause sending */
> + writel(twsi_control_flags, &twsi->control);
> + /* wait for controller to receive byte and check ACK */
> + return twsi_wait(expected_status);
> +}
> +
> +/*
> + * Receive a byte.
> + * Global mvtwsi_control_flags variable says if we should ack or nak.
> + */
> +
> +static int twsi_recv(u8 *byte)
> +{
> + int expected_status, status;
> + /* compute expected status based on ACK bit in global control flags */
> + if (twsi_control_flags & MVTWSI_CONTROL_ACK)
> + expected_status = MVTWSI_STATUS_DATA_R_ACK;
> + else
> + expected_status = MVTWSI_STATUS_DATA_R_NAK;
> + /* acknowledge *previous state* and launch receive */
> + writel(twsi_control_flags, &twsi->control);
> + /* wait for controller to receive byte and assert ACK or NAK */
> + status = twsi_wait(expected_status);
> + /* if we did receive expected byte then store it */
> + if (status==0)
> + *byte = readl(&twsi->data);
> + /* return status */
comment not necessary.
> + return status;
> +}
> +
> +/*
> + * Assert the STOP condition.
> + * This is also used to force the bus back in idle (SDA=SCL=1).
> + */
> +
> +static int twsi_stop(void)
> +{
> + int control,status;
> + int timeout = 1000;
> + /* assert STOP */
> + control = MVTWSI_CONTROL_TWSIEN | MVTWSI_CONTROL_STOP;
> + writel(control, &twsi->control);
> + /* wait for idle condition. There won't be an IFLG so we cannot use twsi_wait(). */
> + do {
> + status = readl(&twsi->status);
> + if (status==MVTWSI_STATUS_IDLE)
> + return 0;
> + udelay(10);
> + } while (timeout--);
> + control = readl(&twsi->control);
> + return MVTWSI_ERROR(MVTWSI_ERROR_TIMEOUT,control,status,MVTWSI_STATUS_IDLE);
> +}
> +
> +/*
> + * Ugly formula to convert m and n values to a frequency comes from
> + * TWSI specifications
> + */
> +
> +#define TWSI_FREQUENCY(m,n) \
> + ( (u8) (CONFIG_SYS_TCLK / (10 * (m + 1) * 2 * (1 << n))) )
> +
> +/*
> + * These are required to be reprogrammed before enabling the controller
> + * because a reset loses them.
> + * Default values come from the spec, but a twsi_reset will change them.
> + */
> +
> +static u8 twsi_baud_rate = 0x44;
> +static u8 twsi_actual_speed = TWSI_FREQUENCY(4,4);
> +static u8 twsi_slave_address = 0;
> +
> +/*
> + * Reset controller.
> + * Called at end of i2c_init unsuccessful i2c transactions.
> + * Controller reset also resets the baud rate and slave address, so
> + * re-establish them.
> + */
> +
> +static void twsi_reset(void)
> +{
> + /* ensure controller will be enabled by any twsi*() function */
> + twsi_control_flags = MVTWSI_CONTROL_TWSIEN;
> + /* reset controller */
> + writel(0, &twsi->soft_reset);
> + /* wait 2 ms -- this is what the Marvell LSP does */
> + udelay(20000);
> + /* set baud rate */
> + writel(twsi_baud_rate, &twsi->baudrate);
> + /* set slave address even though we don't use it */
> + writel(twsi_slave_address, &twsi->slave_address);
> + writel(0, &twsi->extended_slave_address);
> + /* assert STOP but don't care for the result */
> + (void) twsi_stop();
> +}
> +
> +/*
> + * I2C init called by cmd_i2c when doing 'i2c reset'.
> + * Sets baud to the highest possible value not exceeding requested one.
> + */
> +
> +void i2c_init(int requested_speed, int slaveadd)
> +{
> + int tmp_speed, highest_speed, n, m;
> + int baud = 0x44; /* value at controller reset */
> + /* use actual speed to collect progressively higher values */
> + highest_speed = 0;
> + /* compute m,n setting for highest speed not above requested speed */
> + for (n = 0; n < 8; n++) {
> + for (m = 0; m < 16; m++) {
> + tmp_speed = TWSI_FREQUENCY(m,n);
> + if (tmp_speed <= requested_speed)
> + if (tmp_speed > highest_speed) {
> + highest_speed = tmp_speed;
> + baud = (m << 3) | n;
> + }
> + }
> + }
> + /* save baud rate and slave for later calls to twsi_reset */
> + twsi_baud_rate = baud;
> + twsi_actual_speed = highest_speed;
> + twsi_slave_address = slaveadd;
> + /* reset controller */
> + twsi_reset();
> +}
> +
> +/*
> + * Begin I2C transaction with expected start status, at given address.
> + * Common to i2c_probe, i2c_read and i2c_write.
> + * Expected address status will derive from direction bit (bit 0) in addr.
> + */
> +
> +static int i2c_begin(int expected_start_status, u8 addr)
> +{
> + int status, expected_addr_status;
> + /* compute expected address status from direction bit in addr */
> + if (addr & 1) /* reading */
> + expected_addr_status = MVTWSI_STATUS_ADDR_R_ACK;
> + else /* writing */
> + expected_addr_status = MVTWSI_STATUS_ADDR_W_ACK;
> + /* assert START */
> + status = twsi_start(expected_start_status);
> + /* send out the address if the start went well */
> + if (status == 0)
> + status = twsi_send(addr, expected_addr_status);
> + /* return ok or status of first failure to caller */
> + return status;
> +}
> +
> +/*
> + * End I2C transaction, of which the current status is given.
> + * Common to i2c_probe, i2c_read and i2c_write.
> + */
> +
> +static int i2c_end(int status)
> +{
> + /* if transaction went well so far, try to assert a STOP */
> + /*if (status == 0)*/
Don;t add dead code.
> + twsi_stop();
> + /* if anything went wrong (including the STOP we tried), do a reset */
> + /*else
> + twsi_reset();*/
here too.
> + /* return ok or status of first failure to caller */
> + return status;
> +}
> +
> +/*
> + * I2C probe called by cmd_i2c when doing 'i2c probe'.
> + * Begin read, nak data byte, end.
> + */
> +
> +int i2c_probe(uchar chip)
> +{
> + u8 dummy_byte;
> + int status;
> + /* begin i2c read */
> + status = i2c_begin(MVTWSI_STATUS_START, (chip << 1) | 1 );
> + /* dummy read was accepted: receive byte but NAK it. */
> + if (status==0)
> + status = twsi_recv( &dummy_byte);
> + /* We expected either 0 (ok) or MVTWSI_STATUS_ADDR_R_NAK */
> + return i2c_end( (status==MVTWSI_STATUS_ADDR_R_NAK) ? 0: status);
> +}
> +
> +/*
> + * I2C read called by cmd_i2c when doing 'i2c read' and by cmd_eeprom.c
> + * Begin write, send address byte(s), begin read, receive data bytes, end.
> + */
> +
> +int i2c_read(u8 dev, uint addr, int alen, u8 *data, int length)
> +{
> + int status;
> + /* begin i2c write to send the address bytes */
> + status = i2c_begin(MVTWSI_STATUS_START, (dev << 1) );
> + /* send addr bytes */
> + while ( (status==0) && alen--)
> + status = twsi_send(addr >> (8*alen), MVTWSI_STATUS_DATA_W_ACK);
> + /* NOTE: should we send a stop before a start? That's eeprom-level stuff */
> + /* begin i2c read to receive eeprom data bytes */
> + if (status == 0)
> + status = i2c_begin(MVTWSI_STATUS_REPEATED_START, (dev << 1) | 1);
> + /* prepare ACK if at least one byte must be received */
> + if (length > 0)
> + twsi_control_flags |= MVTWSI_CONTROL_ACK;
> + /* now receive actual bytes */
> + while ( (status==0) && length-- ) {
> + /* reset NAK if we if no more to read now */
> + if (length == 0)
> + twsi_control_flags &= ~MVTWSI_CONTROL_ACK;
> + /* read current byte */
> + status = twsi_recv(data++);
> + }
> + /* end i2c transaction */
> + return i2c_end(status);
> +}
> +
> +/*
> + * I2C write called by cmd_i2c when doing 'i2c write' and by cmd_eeprom.c
> + * Begin write, send address byte(s), send data bytes, end.
> + */
> +
> +int i2c_write(u8 dev, uint addr, int alen, u8 *data, int length)
> +{
> + int status;
> + /* begin i2c write to send the eeprom adress bytes then data bytes */
> + status = i2c_begin(MVTWSI_STATUS_START, (dev << 1) );
> + /* send addr bytes */
> + while ( (status==0) && alen--)
> + status = twsi_send(addr >> (8*alen), MVTWSI_STATUS_DATA_W_ACK);
> + /* send data bytes */
> + while ( (status==0) && (length-->0) )
> + status = twsi_send(*(data++), MVTWSI_STATUS_DATA_W_ACK);
> + /* end i2c transaction */
> + return i2c_end(status);
> +}
> +
> +/*
> + * Bus set/get routines: we only support bus 0.
> + */
> +
> +int i2c_set_bus_num(unsigned int bus)
> +{
> + if (bus > 0) {
> + return -1;
> + }
> + return 0;
> +}
> +
> +unsigned int i2c_get_bus_num(void)
> +{
> + return 0;
> +}
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 4:33 ` Prafulla Wadaskar
@ 2010-08-26 6:33 ` Albert ARIBAUD
2010-08-26 6:46 ` Heiko Schocher
0 siblings, 1 reply; 13+ messages in thread
From: Albert ARIBAUD @ 2010-08-26 6:33 UTC (permalink / raw)
To: u-boot
(adding Heiko, custodian of I2C/EEPROM and committer of kirkwood_i2c)
Le 26/08/2010 06:33, Prafulla Wadaskar a ?crit :
>
>
>> -----Original Message-----
>> From: u-boot-bounces at lists.denx.de
>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
>> Sent: Wednesday, August 25, 2010 7:54 PM
>> To: u-boot at lists.denx.de
>> Subject: [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
>>
>> This driver is for the Marvell TWSI/I2C module found in
>> the orion and kirkwood families among others.
>>
>> Signed-off-by: Albert Aribaud<albert.aribaud@free.fr>
>> ---
>> While the 'kirkwood_i2c' driver for the Marvell TWSI module
>> is already available in u-boot, this one is 25% smaller, less
>> complex (no state machine) and much faster (i2c probe on an
>> ED Mini V2 takes no noticeable time vs. half a second).
>
> Hi Albert
> This will be very good enhancement indeed.
>
>>
>> drivers/i2c/Makefile | 1 +
>> drivers/i2c/mvtwsi.c | 419
>
> Can you pls follow the same strategy as we followed for mvgbe, mvsata?
> Please rename and enhance current kirkwood_i2c driver support,
> and then add support for Orion followed by board support for edminiv2
>
> Regards..
> Prafulla ..
I can do this of course; however I felt that I was not fixing an
existing driver (as I did with mvgbe) or adding support (as I did with
mvsata where there was no existing driver) but introducing competition
(as kirkwood_i2c exists and is functional) and I did not want to rudely
stomp the existing driver.
Besides, as mvtwsi is new code, and even though I tested it (probe,
read, write) with the ED Mini V2 EEPROM and RTC, until we are sure that
it works we might want to keep the older kirkwood_i2c code around and be
able to switch from one to the other -- having two different drivers for
the same HW IP and selecting at config time is done in
include/configs/km_arm.h where an option can be set to use either the
soft I2C driver or the kirkwood one.
Finally, we can always remove the kirkwood_i2c driver later on if we
want, in a separate patch (which will also switch km_arm to using mvtwsi).
Anyway, this mvtwsi patch will require Heiko's ACK as well as yours;
let's hear from him (when he is back) on whether I should add mvtwsi or
replace kirkwood_i2c.
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 6:30 ` Heiko Schocher
@ 2010-08-26 6:37 ` Albert ARIBAUD
2010-08-26 22:03 ` Albert ARIBAUD
1 sibling, 0 replies; 13+ messages in thread
From: Albert ARIBAUD @ 2010-08-26 6:37 UTC (permalink / raw)
To: u-boot
Le 26/08/2010 08:30, Heiko Schocher a ?crit :
> As the kirkwood_i2c driver is actually not used in any board,
> we should remove it completely, can you add this for v2?
That answers the questions I *just* asked on the list :) -- Wilco.
> Beside of that, I have just some minor codstyling comments:
>
>[...]
Will apply these. Thanks!
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 6:33 ` Albert ARIBAUD
@ 2010-08-26 6:46 ` Heiko Schocher
2010-08-26 8:23 ` Prafulla Wadaskar
0 siblings, 1 reply; 13+ messages in thread
From: Heiko Schocher @ 2010-08-26 6:46 UTC (permalink / raw)
To: u-boot
Hello Albert,
Albert ARIBAUD wrote:
> (adding Heiko, custodian of I2C/EEPROM and committer of kirkwood_i2c)
>
> Le 26/08/2010 06:33, Prafulla Wadaskar a ?crit :
>>
>>> -----Original Message-----
>>> From: u-boot-bounces at lists.denx.de
>>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
>>> Sent: Wednesday, August 25, 2010 7:54 PM
>>> To: u-boot at lists.denx.de
>>> Subject: [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
>>>
>>> This driver is for the Marvell TWSI/I2C module found in
>>> the orion and kirkwood families among others.
>>>
>>> Signed-off-by: Albert Aribaud<albert.aribaud@free.fr>
>>> ---
>>> While the 'kirkwood_i2c' driver for the Marvell TWSI module
>>> is already available in u-boot, this one is 25% smaller, less
>>> complex (no state machine) and much faster (i2c probe on an
>>> ED Mini V2 takes no noticeable time vs. half a second).
>> Hi Albert
>> This will be very good enhancement indeed.
>>
>>> drivers/i2c/Makefile | 1 +
>>> drivers/i2c/mvtwsi.c | 419
>> Can you pls follow the same strategy as we followed for mvgbe, mvsata?
>> Please rename and enhance current kirkwood_i2c driver support,
>> and then add support for Orion followed by board support for edminiv2
>>
>> Regards..
>> Prafulla ..
>
> I can do this of course; however I felt that I was not fixing an
> existing driver (as I did with mvgbe) or adding support (as I did with
> mvsata where there was no existing driver) but introducing competition
> (as kirkwood_i2c exists and is functional) and I did not want to rudely
> stomp the existing driver.
>
> Besides, as mvtwsi is new code, and even though I tested it (probe,
> read, write) with the ED Mini V2 EEPROM and RTC, until we are sure that
> it works we might want to keep the older kirkwood_i2c code around and be
> able to switch from one to the other -- having two different drivers for
> the same HW IP and selecting at config time is done in
> include/configs/km_arm.h where an option can be set to use either the
> soft I2C driver or the kirkwood one.
We use only soft i2c on this board, so please remove the kirkwood_i2c.c
driver completely. So we have only your driver in tree, which is used and
working.
bye
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 6:46 ` Heiko Schocher
@ 2010-08-26 8:23 ` Prafulla Wadaskar
2010-08-26 9:23 ` Albert ARIBAUD
0 siblings, 1 reply; 13+ messages in thread
From: Prafulla Wadaskar @ 2010-08-26 8:23 UTC (permalink / raw)
To: u-boot
> -----Original Message-----
> From: Heiko Schocher [mailto:hs at denx.de]
> Sent: Thursday, August 26, 2010 12:17 PM
> To: Albert ARIBAUD
> Cc: Prafulla Wadaskar; u-boot at lists.denx.de; Heiko Schosher;
> Ashish Karkare; Prabhanjan Sarnaik
> Subject: Re: [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
>
> Hello Albert,
>
> Albert ARIBAUD wrote:
> > (adding Heiko, custodian of I2C/EEPROM and committer of
> kirkwood_i2c)
> >
> > Le 26/08/2010 06:33, Prafulla Wadaskar a ?crit :
> >>
> >>> -----Original Message-----
> >>> From: u-boot-bounces at lists.denx.de
> >>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
> >>> Sent: Wednesday, August 25, 2010 7:54 PM
> >>> To: u-boot at lists.denx.de
> >>> Subject: [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
> >>>
> >>> This driver is for the Marvell TWSI/I2C module found in
> >>> the orion and kirkwood families among others.
> >>>
> >>> Signed-off-by: Albert Aribaud<albert.aribaud@free.fr>
> >>> ---
> >>> While the 'kirkwood_i2c' driver for the Marvell TWSI module
> >>> is already available in u-boot, this one is 25% smaller, less
> >>> complex (no state machine) and much faster (i2c probe on an
> >>> ED Mini V2 takes no noticeable time vs. half a second).
> >> Hi Albert
> >> This will be very good enhancement indeed.
> >>
> >>> drivers/i2c/Makefile | 1 +
> >>> drivers/i2c/mvtwsi.c | 419
> >> Can you pls follow the same strategy as we followed for
> mvgbe, mvsata?
> >> Please rename and enhance current kirkwood_i2c driver support,
> >> and then add support for Orion followed by board support
> for edminiv2
> >>
> >> Regards..
> >> Prafulla ..
> >
> > I can do this of course; however I felt that I was not fixing an
> > existing driver (as I did with mvgbe) or adding support (as
> I did with
> > mvsata where there was no existing driver) but introducing
> competition
> > (as kirkwood_i2c exists and is functional) and I did not
> want to rudely
> > stomp the existing driver.
> >
> > Besides, as mvtwsi is new code, and even though I tested it (probe,
> > read, write) with the ED Mini V2 EEPROM and RTC, until we
> are sure that
> > it works we might want to keep the older kirkwood_i2c code
> around and be
> > able to switch from one to the other -- having two
> different drivers for
> > the same HW IP and selecting at config time is done in
> > include/configs/km_arm.h where an option can be set to use
> either the
> > soft I2C driver or the kirkwood one.
>
> We use only soft i2c on this board, so please remove the
> kirkwood_i2c.c
> driver completely. So we have only your driver in tree, which
> is used and
> working.
Ack.
Regards..
Prafulla . .
>
> bye
> Heiko
> --
> DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
>
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 8:23 ` Prafulla Wadaskar
@ 2010-08-26 9:23 ` Albert ARIBAUD
0 siblings, 0 replies; 13+ messages in thread
From: Albert ARIBAUD @ 2010-08-26 9:23 UTC (permalink / raw)
To: u-boot
Le 26/08/2010 10:23, Prafulla Wadaskar a ?crit :
>> We use only soft i2c on this board, so please remove the
>> kirkwood_i2c.c
>> driver completely. So we have only your driver in tree, which
>> is used and
>> working.
>
> Ack.
> Regards..
> Prafulla . .
Wilco. As a safety and cleanliness measure, I'll prepend a patch to the
set, which will remove the 'hard i2c' related options from km_arm since
they aren't used, and won't build any more once the switch is done.
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 6:30 ` Heiko Schocher
2010-08-26 6:37 ` Albert ARIBAUD
@ 2010-08-26 22:03 ` Albert ARIBAUD
2010-08-27 5:25 ` Heiko Schocher
1 sibling, 1 reply; 13+ messages in thread
From: Albert ARIBAUD @ 2010-08-26 22:03 UTC (permalink / raw)
To: u-boot
Le 26/08/2010 08:30, Heiko Schocher a ?crit :
> Beside of that, I have just some minor codstyling comments:
Meanwhile, I've run checkpatch on this file and fixed every warning and
error, thus some of your comments were taken care of in the process.
That left:
>> +#define MVTWSI_STATUS_DATA_R_NAK 0x58 /* our NAK, not the slave's */
>
> line too long.
Checkpatch did not complain on this one. Is there a specific line length
contraint beside passing checkpatch?
>> +/*
>> + * Wait for IFLG to raise, or return 'timeout'; then if status is as expected,
>> + * return 0 (ok) or return 'wrong status'.
>> + */
>> +
>
> blank line not necessary.
Which blank line exactly? Between the comment block and code? If so,
does this apply everywhere, i.e. after the topmost license comment
block, before blocks of #define, etc?
As a side note, if V2 gets Acked by Prafulla and you, can you add the
commit to your i2c tree and ask for a new pull? Anyway, Wolfgang said he
won't resume activity on u-boot before sep 5th.
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-26 22:03 ` Albert ARIBAUD
@ 2010-08-27 5:25 ` Heiko Schocher
2010-08-27 6:05 ` Albert ARIBAUD
0 siblings, 1 reply; 13+ messages in thread
From: Heiko Schocher @ 2010-08-27 5:25 UTC (permalink / raw)
To: u-boot
Hello Albert,
Albert ARIBAUD wrote:
> Le 26/08/2010 08:30, Heiko Schocher a ?crit :
>
>> Beside of that, I have just some minor codstyling comments:
>
> Meanwhile, I've run checkpatch on this file and fixed every warning and
> error, thus some of your comments were taken care of in the process.
> That left:
>
>>> +#define MVTWSI_STATUS_DATA_R_NAK 0x58 /* our NAK, not the
>>> slave's */
>>
>> line too long.
>
> Checkpatch did not complain on this one. Is there a specific line length
> contraint beside passing checkpatch?
Yep, 78 characters.
>>> +/*
>>> + * Wait for IFLG to raise, or return 'timeout'; then if status is as
>>> expected,
>>> + * return 0 (ok) or return 'wrong status'.
>>> + */
>>> +
>>
>> blank line not necessary.
>
> Which blank line exactly? Between the comment block and code? If so,
Yes.
> does this apply everywhere, i.e. after the topmost license comment
> block, before blocks of #define, etc?
No, there, it is Ok.
> As a side note, if V2 gets Acked by Prafulla and you, can you add the
> commit to your i2c tree and ask for a new pull? Anyway, Wolfgang said he
> won't resume activity on u-boot before sep 5th.
Yes, if I and Prafulla accept it, this is the usual way.
bye,
Heiko
--
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
^ permalink raw reply [flat|nested] 13+ messages in thread
* [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver
2010-08-27 5:25 ` Heiko Schocher
@ 2010-08-27 6:05 ` Albert ARIBAUD
0 siblings, 0 replies; 13+ messages in thread
From: Albert ARIBAUD @ 2010-08-27 6:05 UTC (permalink / raw)
To: u-boot
Le 27/08/2010 07:25, Heiko Schocher a ?crit :
>>>> +#define MVTWSI_STATUS_DATA_R_NAK 0x58 /* our NAK, not the
>>>> slave's */
>>>
>>> line too long.
>>
>> Checkpatch did not complain on this one. Is there a specific line length
>> contraint beside passing checkpatch?
>
> Yep, 78 characters.
Ok. I added a possibility for checkpatch to accept a custom line length;
I'll try and push that to the lkml. Meanwhile, it showed me three lines
beyond 78 characters, fixed.
>> Which blank line exactly? Between the comment block and code? If so,
>
> Yes.
>> does this apply everywhere, i.e. after the topmost license comment
>> block, before blocks of #define, etc?
>
> No, there, it is Ok.
Fixed, thanks.
>> As a side note, if V2 gets Acked by Prafulla and you, can you add the
>> commit to your i2c tree and ask for a new pull? Anyway, Wolfgang said he
>> won't resume activity on u-boot before sep 5th.
>
> Yes, if I and Prafulla accept it, this is the usual way.
Here you go, then. :)
Thanks for your review.
Amicalement,
--
Albert.
^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2010-08-27 6:05 UTC | newest]
Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-08-25 14:23 [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert Aribaud
2010-08-25 14:23 ` [U-Boot] [PATCH 2/2] edminiv2: add I2C support using mvtwsi driver Albert Aribaud
2010-08-25 15:28 ` [U-Boot] [PATCH 1/2] I2C: add Marvell TWSI simple driver Albert ARIBAUD
2010-08-26 4:33 ` Prafulla Wadaskar
2010-08-26 6:33 ` Albert ARIBAUD
2010-08-26 6:46 ` Heiko Schocher
2010-08-26 8:23 ` Prafulla Wadaskar
2010-08-26 9:23 ` Albert ARIBAUD
2010-08-26 6:30 ` Heiko Schocher
2010-08-26 6:37 ` Albert ARIBAUD
2010-08-26 22:03 ` Albert ARIBAUD
2010-08-27 5:25 ` Heiko Schocher
2010-08-27 6:05 ` Albert ARIBAUD
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.