I2C: add Marvell TWSI simple driver

-----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 <[email protected]>

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

Quoted 12 lines

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

Quoted 6 lines

Signed-off-by Albert Aribaud<[email protected]>

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

Quoted 3 lines

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

(adding Heiko, custodian of I2C/EEPROM and committer of kirkwood_i2c)

Le 26/08/2010 06:33, Prafulla Wadaskar a ?crit :

Quoted 20 lines

Quoted 11 lines

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

Quoted 6 lines

Signed-off-by Albert Aribaud<[email protected]>

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

Quoted 8 lines

Quoted 2 lines

  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.

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

Quoted 1 line · Albert ARIBAUD

(adding Heiko, custodian of I2C/EEPROM and committer of

kirkwood_i2c)

Quoted 25 lines

Le 26/08/2010 06:33, Prafulla Wadaskar a ?crit :

Quoted 23 lines

Quoted 11 lines

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

Quoted 6 lines

Signed-off-by Albert Aribaud<[email protected]>

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

Quoted 2 lines

  drivers/i2c/Makefile |    1 +
  drivers/i2c/mvtwsi.c |  419

Can you pls follow the same strategy as we followed for

mvgbe, mvsata?

Quoted 2 lines

Quoted 2 lines

Please rename and enhance current kirkwood_i2c driver support, and then add support for Orion followed by board support

for edminiv2

Quoted 6 lines

Quoted 3 lines

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

Quoted 1 line

mvsata where there was no existing driver) but introducing

competition

Quoted 1 line

(as kirkwood_i2c exists and is functional) and I did not

want to rudely

Quoted 4 lines

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

Quoted 1 line

it works we might want to keep the older kirkwood_i2c code

around and be

Quoted 1 line

able to switch from one to the other -- having two

different drivers for

Quoted 2 lines

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

Quoted 1 line

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

Signature · 2 lines

DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Quoted 5 lines

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

This driver is for the Marvell TWSI/I2C module found in the orion and kirkwood families among others.

Signed-off-by Albert Aribaud <[email protected]>

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

drivers/i2c/Makefile +1 -0

index d2c2515..73c415d 100644
@@ -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

drivers/i2c/mvtwsi.c new file +75 -0

new file mode 100644
index 0000000..b591328
@@ -0,0 +1,419 @@
+/*
+ * Driver for the TWSI (i2c) controller on the Marvell orion5x
+ *
+ * Author: Albert Aribaud <[email protected]>
+ * 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.

drivers/i2c/mvtwsi.c new file +20 -0

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

drivers/i2c/mvtwsi.c new file +6 -0

+					expected_status);
+		}
+		udelay(10);
+	} while (timeout--);
+	status = readl(&twsi->status);
+	return MVTWSI_ERROR(MVTWSI_ERROR_TIMEOUT,control,status,expected_status);

drivers/i2c/mvtwsi.c new file +42 -0

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

drivers/i2c/mvtwsi.c new file +120 -0

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

drivers/i2c/mvtwsi.c new file +90 -0

+	/* 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;
+}

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:

[...]

Beside of that, I have just some minor codstyling comments:

Quoted 1 line

+#define MVTWSI_STATUS_DATA_R_NAK 0x58 /* our NAK, not the slave's */

line too long.

drivers/i2c/mvtwsi.c line 115

Quoted 5 lines

drivers/i2c/mvtwsi.c new file +5 -0

+/*
+ * 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.

Le 26/08/2010 08:30, Heiko Schocher a ?crit :

Quoted 1 line

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:

Quoted 4 lines

Quoted 2 lines

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

Quoted 8 lines

Quoted 6 lines

+/*
+ * 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.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

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

Quoted 1 line

Which blank line exactly? Between the comment block and code? If so,

Yes.

Quoted 2 lines

does this apply everywhere, i.e. after the topmost license comment block, before blocks of #define, etc?

No, there, it is Ok.

Quoted 3 lines

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.