AT91: replace broken TWI driver i2c-at91.c

drivers/i2c/busses/i2c-at91.c new file +4 -0

+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */

drivers/i2c/busses/i2c-at91.c

Quoted 9 lines

drivers/i2c/busses/i2c-at91.c new file +7 -0

+
+static unsigned at91_twi_read(struct at91_twi_dev *dev, unsigned reg)
+{
+	return __raw_readl(dev->base + reg);
+}
+
+static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned

val)

drivers/i2c/busses/i2c-at91.c line 57

Quoted 3 lines

drivers/i2c/busses/i2c-at91.c new file +3 -0

+{
+	__raw_writel(val, dev->base + reg);
+}

Better use readl/writel or readl_relaxed/writel_relaxed.

drivers/i2c/busses/i2c-at91.c

Quoted 6 lines

drivers/i2c/busses/i2c-at91.c new file +5 -0

+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int

twi_clk)

drivers/i2c/busses/i2c-at91.c line 86

Quoted 9 lines

drivers/i2c/busses/i2c-at91.c new file +9 -0

+{
+	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 (cpu_is_at91rm9200() && (ckdiv > 5)) {
+		dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
+		ckdiv = 5;
+	}

Don't check a global property like this locally in the driver. Instead, make it a property of the device that you check here and set it based on the the device name set by the platform, or by a device tree property.

drivers/i2c/busses/i2c-at91.h line 4

Quoted 10 lines

drivers/i2c/busses/i2c-at91.h new file +4 -0

new file mode 100644
index 0000000..a898159
@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+

No need for a header file if all the values in it are used in only one source file. Just move everything to i2c_at91.c

drivers/i2c/busses/i2c-at91.h

Quoted 8 lines

drivers/i2c/busses/i2c-at91.h new file +8 -0

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

*/

drivers/i2c/busses/i2c-at91.h line 36

Quoted 1 line

drivers/i2c/busses/i2c-at91.h new file +1 -0

+#define	AT91_TWI_DADR		(0x7f << 16)	/* Device Address */

These are harder to read than just using hexadecimal bitmasks:

#define AT91_TWI_MMR 0x00000004
#define AT91_TWI_IADRSZ 0x00000300
#define AT91_TWI_IADRSZ_NO 0x00000000
#define AT91_TWI_IADRSZ_1 0x00000100
...

Quoted 7 lines

drivers/i2c/busses/i2c-at91.c

Quoted 6 lines

drivers/i2c/busses/i2c-at91.c new file +5 -0

+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int

twi_clk)

Quoted 13 lines

drivers/i2c/busses/i2c-at91.c line 86

Quoted 9 lines

drivers/i2c/busses/i2c-at91.c new file +9 -0

+{
+   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 (cpu_is_at91rm9200() && (ckdiv > 5)) {
+           dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv = 5.\n");
+           ckdiv = 5;
+   }

Don't check a global property like this locally in the driver. Instead, make it a property of the device that you check here and set it based on the the device name set by the platform, or by a device tree property.

Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no revisions for specific at91 IP devices but the revision is implicitly defined by the cpu type and version. Changing this would need to add some arch infrastructure which I think is beyond the scope of this driver.

Quoted 25 lines

drivers/i2c/busses/i2c-at91.h line 4

Quoted 10 lines

drivers/i2c/busses/i2c-at91.h new file +4 -0

new file mode 100644
index 0000000..a898159
@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+

No need for a header file if all the values in it are used in only one source file. Just move everything to i2c_at91.c

drivers/i2c/busses/i2c-at91.h

Quoted 8 lines

drivers/i2c/busses/i2c-at91.h new file +8 -0

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

*/

drivers/i2c/busses/i2c-at91.h line 36

Quoted 1 line

drivers/i2c/busses/i2c-at91.h new file +1 -0

+#define    AT91_TWI_DADR           (0x7f << 16)    /* Device Address */

Quoted 7 lines

These are harder to read than just using hexadecimal bitmasks:

#define       AT91_TWI_MMR            0x00000004
#define       AT91_TWI_IADRSZ         0x00000300
#define       AT91_TWI_IADRSZ_NO      0x00000000
#define       AT91_TWI_IADRSZ_1       0x00000100

...

I agree, but this header file was already used by the old driver and converting would add possible errors to register definitions which are not (yet) used. That's why I've left it as is and just made it a local include.

Quoted 9 lines

Quoted 9 lines

drivers/i2c/busses/i2c-at91.c

Quoted 6 lines

drivers/i2c/busses/i2c-at91.c new file +5 -0

+/*
+ * Calculate and set symmetric clock as stated in datasheet:
+ * twi_clk = F_MAIN / (2 * cdiv * (1 << ckdiv) + 4)
+ */
+static void __devinit at91_set_twi_clock(struct at91_twi_dev *dev, int

twi_clk)

Quoted 2 lines

(quoted diff) +2 -0

+{
+   const int div = DIV_ROUND_UP(clk_get_rate(dev->clk), 2 * twi_clk) -

2;

Quoted 5 lines

Quoted 5 lines

drivers/i2c/busses/i2c-at91.c

Quoted 5 lines

drivers/i2c/busses/i2c-at91.c new file +5 -0

+   int ckdiv = fls(div >> 8);
+   const int cdiv = div >> ckdiv;
+
+   if (cpu_is_at91rm9200() && (ckdiv > 5)) {
+           dev_warn(dev->dev, "AT91RM9200 erratum 22: using ckdiv =

5.\n");

Quoted 7 lines

Quoted 6 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+           ckdiv = 5;
+   }

Don't check a global property like this locally in the driver. Instead, make it a property of the device that you check here and set it based on the the device name set by the platform, or by a device tree property.

Quoted 5 lines

Yes, this is a known problem and was discussed in
https://lkml.org/lkml/2011/11/8/217 . The main problem is that there a no revisions for specific at91 IP devices but the revision is implicitly defined by the cpu type and version. Changing this would need to add some arch infrastructure which I think is beyond the scope of this driver.

Aside from the flamewar in that thread, my impression is that in general people (me certainly) prefer to have driver-local workarounds be expressed in a driver specific way, not in a platform or architecture specific way because that makes the driver less portable.

Quoted 1 line

Quoted 1 line

drivers/i2c/busses/i2c-

Quoted 1 line

drivers/i2c/busses/i2c- renamed from drivers/i2c/busses/i2c-at91.h +0 -0

at91.h

Quoted 13 lines

Quoted 13 lines

drivers/i2c/busses/i2c-at91.h line 4

Quoted 9 lines

new file mode 100644
index 0000000..a898159

drivers/i2c/busses/i2c-at91.h new file +4 -0

@@ -0,0 +1,80 @@
+
+#ifndef AT91_TWI_H
+#define AT91_TWI_H
+

No need for a header file if all the values in it are used in only one source file. Just move everything to i2c_at91.c

Quoted 1 line

Quoted 1 line

drivers/i2c/busses/i2c-at91.h line 5

Quoted 1 line

drivers/i2c/busses/i2c-at91.h new file +1 -0

+#define    AT91_TWI_MMR            0x04            /* Master Mode

Register */

Quoted 1 line

Quoted 1 line

drivers/i2c/busses/i2c-at91.h line 6

Quoted 1 line

drivers/i2c/busses/i2c-at91.h new file +1 -0

+#define    AT91_TWI_IADRSZ         (3    <<  8)    /* Internal Device

Address

Quoted 6 lines

Quoted 6 lines

drivers/i2c/busses/i2c-at91.h

Quoted 6 lines

drivers/i2c/busses/i2c-at91.h new file +6 -0

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

Quoted 11 lines

Quoted 10 lines

*/

drivers/i2c/busses/i2c-at91.h line 36

Quoted 1 line

drivers/i2c/busses/i2c-at91.h new file +1 -0

+#define    AT91_TWI_DADR           (0x7f << 16)    /* Device Address */

These are harder to read than just using hexadecimal bitmasks:

#define       AT91_TWI_MMR            0x00000004
#define       AT91_TWI_IADRSZ         0x00000300
#define       AT91_TWI_IADRSZ_NO      0x00000000
#define       AT91_TWI_IADRSZ_1       0x00000100

...

Quoted 4 lines

I agree, but this header file was already used by the old driver and converting would add possible errors to register definitions which are not (yet) used. That's why I've left it as is and just made it a local include.

But you are presenting the driver as a new one, so you should be prepared to get review comments like any other new code.

Please at least move the data into the main driver file to get rid of the header file.

Quoted 4 lines

Aside from the flamewar in that thread, my impression is that in general people (me certainly) prefer to have driver-local workarounds be expressed in a driver specific way, not in a platform or architecture specific way because that makes the driver less portable.

I guess I see your point now. So you want something like pdev.has_bugX to be set by mach setup and later check this flag in the driver?

Quoted 14 lines

Quoted 10 lines

Quoted 5 lines

#define       AT91_TWI_MMR            0x00000004
#define       AT91_TWI_IADRSZ         0x00000300
#define       AT91_TWI_IADRSZ_NO      0x00000000
#define       AT91_TWI_IADRSZ_1       0x00000100

...

I agree, but this header file was already used by the old driver and converting would add possible errors to register definitions which are not (yet) used. That's why I've left it as is and just made it a local include.

But you are presenting the driver as a new one, so you should be prepared to get review comments like any other new code.

Quoted 2 lines

Please at least move the data into the main driver file to get rid of the header file.

I didn't want to appear ignorant about this, I actually appreciate your comment. I just wanted to point out that there might be a reason to keep the old file which you weren't aware of (because I presented this as a new driver). So, I will move the register definitions to the main driver.

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.

v7: Patch 4/5: i) fix bug if internal address > 1 byte
ii) send stop when len == 1
(both reported by Carsten Behling)
v6: Patch 5/5: 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 (5):
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 i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality

 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/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              |  529 +++++++++++++++++-----------
 drivers/i2c/busses/i2c-at91.h              |   80 +++++
 12 files changed, 408 insertions(+), 295 deletions(-)
 delete mode 100644 arch/arm/mach-at91/include/mach/at91_twi.h
 create mode 100644 drivers/i2c/busses/i2c-at91.h

Signature · 6 lines

1.7.5.4

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Quoted 27 lines

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.

v7: Patch 4/5: i) fix bug if internal address > 1 byte
ii) send stop when len == 1
(both reported by Carsten Behling)
v6: Patch 5/5: 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

Quoted 6 lines

Nikolaus Voss (5):
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 i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality

Is the original driver so broken that the two could not co-exist, or are we making so many changes that there's no point in keeping the original one?

Hi,

Ben Dooks wrote on 2011-11-24:

Quoted 35 lines · Nikolaus Voss

Quoted 27 lines

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.

v7: Patch 4/5: i) fix bug if internal address > 1 byte
ii) send stop when len == 1
(both reported by Carsten Behling)
v6: Patch 5/5: 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

Quoted 6 lines

Nikolaus Voss (5):
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 i2c-at91.c: add SMBUS_READ_BLOCK_DATA functionality

Quoted 3 lines

Is the original driver so broken that the two could not co-exist, or are we making so many changes that there's no point in keeping the original one?

The old driver was marked as broken for the above reasons and I can hardly imagine any setup in which it would be preferable to i2c-gpio. So it does not make any sense to keep the old driver alive. Though inspired by the old driver, the new one is almost a rewrite from scratch, so for better reviewing, I removed the old instead of doing a diff.

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.

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.

I've tested this driver with the pca953x GPIO expander driver with a PCA9554.

In the case of 8 GPIO pins (my case) "i2c_smbus_read_byte_data(...)"

I observed that reading out the GPIO status is one read delayed. The first read to a register from the pca953x driver does not result in a RXRDY interrupt and at91_twi_read_next_byte(...) is not called. Only the completion routine is triggered with a TXCOMP interrupt.

Additionally, I took a look at the status flags just after the control flags are set (in at91_do_twi_transfer(...), AT91_TWI_START|AT91_TWI_STOP for the one byte read). Surprisingly, the AT91_TWI_RXRDY flag is zero before the first register read and one for the following reads. According to the manual this flag should be zero after a read on AT91_TWI_RHR.

Reading the AT91_TWI_RHR before the control flags are set to be sure that the AT91_TWI_RXRDY is cleared leads to a never occurring RXRDY interrupt.