ChangeLog
v5:
- print I2C controller version in an already existing dev_info() instead of
adding a new one.
v4:
- replace 0x%x by %#x when printing I2C controller version
- change the order of patches: the race condition bug fix becomes the first
patch so it be can more easily applied to older kernels.
v3:
- fix braces {} coding style issue
- split the alternative command patch into 2 patches: the first one fixes
a race condition whereas the second one is the actual alternative command
patch
v2:
- fix typo in comment for AT91_TWI_SVEN.
- document new device tree bindings like "atmel,fifo-size".
- explicitly set the has_alt_cmd boolean to false to already existing chip
configs.
- use the BIT() macro to define the register bits and do a little cleanup in a
dedicated patch.
- reword some comments to better explain why the TXCOMP interrupt is no longer
enabled in at91_do_twi_transfer() but later in
at91_twi_write_data_dma_callback() to avoid a race condition when DMA is used.
- remove useless TXCOMP interrupt enable line in at91_twi_write_next_byte()
since this interrupt is also enabled by at91_do_twi_transfer() for PIO
transfers.
v1:
This series of patches adds support of two new features which will be
introduced with Atmel sama5d2x SoC.
First, the alternative command mode eases the sending of STOP conditions.
Before starting an I2C transaction, the size data to be transfered is
written into the new Alternative Command Register. For each byte transferred,
the I2C controller decreases this counter and automatically sends a STOP
condition when the counter value reaches 0, that is to say when the last byte
of the transaction has been sent/received. So there is no longer need to set
the STOP bit into the Control Register.
Then the use of FIFOs allows to reduce number I/O accesses: for instance,
the TX FIFO allows to write up to 4 data in a single access to the Transmit
Holding Register. Also the RX FIFO allows to read up to 4 data in a single
access to the Receive Holding Register. Currently only DMA transfers take
advantage of FIFOs.
Cyrille Pitchen (6):
i2c: at91: fix a race condition when using the DMA controller
i2c: at91: use BIT() macro to define register bits
i2c: at91: update documentation for DT bindings
i2c: at91: add support for new alternative command mode
i2c: at91: print hardware version
i2c: at91: add support to FIFOs
Documentation/devicetree/bindings/i2c/i2c-at91.txt | 29 +-
drivers/i2c/busses/i2c-at91.c | 356 +++++++++++++++++----
2 files changed, 322 insertions(+), 63 deletions(-)
--
1.8.2.2
For TX transactions, the TXCOMP bit in the Status Register is cleared
when the first data is written into the Transmit Holding Register.
In the lines from at91_do_twi_transfer():
at91_twi_write_data_dma(dev);
at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
the TXCOMP interrupt may be enabled before the DMA controller has
actually started to write into the THR. In such a case, the TXCOMP bit
is still set into the Status Register so the interrupt is triggered
immediately. The driver understands that a transaction completion has
occurred but this transaction hasn't started yet. Hence the TXCOMP
interrupt is no longer enabled by at91_do_twi_transfer() but instead
by at91_twi_write_data_dma_callback().
Also, the TXCOMP bit in the Status Register in not a clear on read flag
but a snapshot of the transmission state at the time the Status
Register is read.
When a NACK error is dectected by the I2C controller, the TXCOMP, NACK
and TXRDY bits are set together to 1 in the SR. If enabled, the TXCOMP
interrupt is triggered at the same time. Also setting the TXRDY to 1
triggers the DMA controller to write the next data into the THR. Such
a write resets the TXCOMP bit to 0 in the SR. So depending on when the
interrupt handler reads the SR, it may fail to detect the NACK error
if it relies on the TXCOMP bit. The NACK bit and its interrupt should
be used instead.
For RX transactions, the TXCOMP bit in the Status Register is cleared
when the START bit is set into the Control Register. However to unify
the management of the TXCOMP bit when the DMA controller is used, the
TXCOMP interrupt is now enabled by the DMA callbacks for both TX and
RX transfers.
Signed-off-by: Cyrille Pitchen <[email protected]>
Cc: [email protected] #3.10 and later
---
drivers/i2c/busses/i2c-at91.c | 70 ++++++++++++++++++++++++++++++++-----------
1 file changed, 53 insertions(+), 17 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index ff23d1b..9bd10a9 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -65,6 +65,9 @@
#define AT91_TWI_UNRE 0x0080 /* Underrun Error */
#define AT91_TWI_NACK 0x0100 /* Not Acknowledged */
+#define AT91_TWI_INT_MASK \
+ (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK)
+
#define AT91_TWI_IER 0x0024 /* Interrupt Enable Register */
#define AT91_TWI_IDR 0x0028 /* Interrupt Disable Register */
#define AT91_TWI_IMR 0x002c /* Interrupt Mask Register */
@@ -119,13 +122,12 @@ static void at91_twi_write(struct at91_twi_dev *dev, unsigned reg, unsigned val)
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);
+ at91_twi_write(dev, AT91_TWI_IDR, AT91_TWI_INT_MASK);
}
static void at91_twi_irq_save(struct at91_twi_dev *dev)
{
- dev->imr = at91_twi_read(dev, AT91_TWI_IMR) & 0x7;
+ dev->imr = at91_twi_read(dev, AT91_TWI_IMR) & AT91_TWI_INT_MASK;
at91_disable_twi_interrupts(dev);
}
@@ -215,6 +217,14 @@ static void at91_twi_write_data_dma_callback(void *data)
dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg),
dev->buf_len, DMA_TO_DEVICE);
+ /*
+ * When this callback is called, THR/TX FIFO is likely not to be empty
+ * yet. So we have to wait for TXCOMP or NACK bits to be set into the
+ * Status Register to be sure that the STOP bit has been sent and the
+ * transfer is completed. The NACK interrupt has already been enabled,
+ * we just have to enable TXCOMP one.
+ */
+ at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
}
@@ -309,7 +319,7 @@ static void at91_twi_read_data_dma_callback(void *data)
/* The last two bytes have to be read without using dma */
dev->buf += dev->buf_len - 2;
dev->buf_len = 2;
- at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_RXRDY);
+ at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_RXRDY | AT91_TWI_TXCOMP);
}
static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
@@ -370,7 +380,7 @@ static irqreturn_t atmel_twi_interrupt(int irq, void *dev_id)
/* catch error flags */
dev->transfer_status |= status;
- if (irqstatus & AT91_TWI_TXCOMP) {
+ if (irqstatus & (AT91_TWI_TXCOMP | AT91_TWI_NACK)) {
at91_disable_twi_interrupts(dev);
complete(&dev->cmd_complete);
}
@@ -384,6 +394,34 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
unsigned long time_left;
bool has_unre_flag = dev->pdata->has_unre_flag;
+ /*
+ * WARNING: the TXCOMP bit in the Status Register is NOT a clear on
+ * read flag but shows the state of the transmission at the time the
+ * Status Register is read. According to the programmer datasheet,
+ * TXCOMP is set when both holding register and internal shifter are
+ * empty and STOP condition has been sent.
+ * Consequently, we should enable NACK interrupt rather than TXCOMP to
+ * detect transmission failure.
+ *
+ * Besides, the TXCOMP bit is already set before the i2c transaction
+ * has been started. For read transactions, this bit is cleared when
+ * writing the START bit into the Control Register. So the
+ * corresponding interrupt can safely be enabled just after.
+ * However for write transactions managed by the CPU, we first write
+ * into THR, so TXCOMP is cleared. Then we can safely enable TXCOMP
+ * interrupt. If TXCOMP interrupt were enabled before writing into THR,
+ * the interrupt handler would be called immediately and the i2c command
+ * would be reported as completed.
+ * Also when a write transaction is managed by the DMA controller,
+ * enabling the TXCOMP interrupt in this function may lead to a race
+ * condition since we don't know whether the TXCOMP interrupt is enabled
+ * before or after the DMA has started to write into THR. So the TXCOMP
+ * interrupt is enabled later by at91_twi_write_data_dma_callback().
+ * Immediately after in that DMA callback, we still need to send the
+ * STOP condition manually writing the corresponding bit into the
+ * Control Register.
+ */
+
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
(dev->msg->flags & I2C_M_RD) ? "read" : "write", dev->buf_len);
@@ -414,26 +452,24 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
* seems to be the best solution.
*/
if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
+ at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
at91_twi_read_data_dma(dev);
- /*
- * It is important to enable TXCOMP irq here because
- * doing it only when transferring the last two bytes
- * will mask NACK errors since TXCOMP is set when a
- * NACK occurs.
- */
- at91_twi_write(dev, AT91_TWI_IER,
- AT91_TWI_TXCOMP);
- } else
+ } else {
at91_twi_write(dev, AT91_TWI_IER,
- AT91_TWI_TXCOMP | AT91_TWI_RXRDY);
+ AT91_TWI_TXCOMP |
+ AT91_TWI_NACK |
+ AT91_TWI_RXRDY);
+ }
} else {
if (dev->use_dma && (dev->buf_len > AT91_I2C_DMA_THRESHOLD)) {
+ at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_NACK);
at91_twi_write_data_dma(dev);
- at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
} else {
at91_twi_write_next_byte(dev);
at91_twi_write(dev, AT91_TWI_IER,
- AT91_TWI_TXCOMP | AT91_TWI_TXRDY);
+ AT91_TWI_TXCOMP |
+ AT91_TWI_NACK |
+ AT91_TWI_TXRDY);
}
}
--
1.8.2.2
This patch just fixes typo before applying later patches which will use
register bits with index above 16.
Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/i2c/busses/i2c-at91.c | 29 +++++++++++++++--------------
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 9bd10a9..0e88b68 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -41,29 +41,30 @@
/* 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_QUICK 0x0040 /* SMBus quick command */
-#define AT91_TWI_SWRST 0x0080 /* Software Reset */
+#define AT91_TWI_START BIT(0) /* Send a Start Condition */
+#define AT91_TWI_STOP BIT(1) /* Send a Stop Condition */
+#define AT91_TWI_MSEN BIT(2) /* Master Transfer Enable */
+#define AT91_TWI_MSDIS BIT(3) /* Master Transfer Disable */
+#define AT91_TWI_SVEN BIT(4) /* Slave Transfer Enable */
+#define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */
+#define AT91_TWI_QUICK BIT(6) /* SMBus quick command */
+#define AT91_TWI_SWRST BIT(7) /* 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_MREAD BIT(12) /* 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_TXCOMP BIT(0) /* Transmission Complete */
+#define AT91_TWI_RXRDY BIT(1) /* Receive Holding Register Ready */
+#define AT91_TWI_TXRDY BIT(2) /* Transmit Holding Register Ready */
+#define AT91_TWI_OVRE BIT(6) /* Overrun Error */
+#define AT91_TWI_UNRE BIT(7) /* Underrun Error */
+#define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
#define AT91_TWI_INT_MASK \
(AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK)
--
1.8.2.2
add a new value "atmel,at91sama5d2-i2c" for the "compatible" property.
add a new optional property "atmel,fifo-size" to enable FIFO support when
available.
add missing optional properties "dmas" and "dma-names".
Signed-off-by: Cyrille Pitchen <[email protected]>
---
Documentation/devicetree/bindings/i2c/i2c-at91.txt | 29 ++++++++++++++++++++--
1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
index 388f0a2..7c04fd9 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
@@ -2,8 +2,8 @@ I2C for Atmel platforms
Required properties :
- compatible : Must be "atmel,at91rm9200-i2c", "atmel,at91sam9261-i2c",
- "atmel,at91sam9260-i2c", "atmel,at91sam9g20-i2c", "atmel,at91sam9g10-i2c"
- or "atmel,at91sam9x5-i2c"
+ "atmel,at91sam9260-i2c", "atmel,at91sam9g20-i2c", "atmel,at91sam9g10-i2c",
+ "atmel,at91sam9x5-i2c" or "atmel,at91sama5d2-i2c"
- reg: physical base address of the controller and length of memory mapped
region.
- interrupts: interrupt number to the cpu.
@@ -13,6 +13,9 @@ Required properties :
Optional properties:
- clock-frequency: Desired I2C bus frequency in Hz, otherwise defaults to 100000
+- dmas: A list of two dma specifiers, one for each entry in dma-names.
+- dma-names: should contain "tx" and "rx".
+- atmel,fifo-size: size of the RX and TX FIFOs, if available.
- Child nodes conforming to i2c bus binding
Examples :
@@ -32,3 +35,25 @@ i2c0: i2c@fff84000 {
pagesize = <128>;
}
}
+
+i2c0: i2c@f8034600 {
+ compatible = "atmel,at91sama5d2-i2c";
+ reg = <0xf8034600 0x100>;
+ interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
+ dmas = <&dma0
+ (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1))
+ AT91_XDMAC_DT_PERID(11)>,
+ <&dma0
+ (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1))
+ AT91_XDMAC_DT_PERID(12)>;
+ dma-names = "tx", "rx";
+ #address-cells = <1>;
+ #size-cells = <0>;
+ clocks = <&flx0>;
+ atmel,fifo-size = <32>;
+
+ wm8731: wm8731@1a {
+ compatible = "wm8731";
+ reg = <0x1a>;
+ };
+};
--
1.8.2.2
The alternative command mode was introduced to simplify the transmission
of STOP conditions and to solve timing and latency issues around them.
This mode relies on a new register, the Alternative Command Register,
which must be set at the same time as the Master Mode Register. This new
register was designed to allow simple setup of basic combined transactions
built from up to two unitary transactions.
Indeed, the ACR is split into two areas, which describe one unitary
transaction each. Each area is filled with Data Length 8bit counter, a
Direction and a PEC Request bit. The PEC bit is only used in SMBus mode
and is not supported by this driver yet. Also when using alternative
command mode, the MREAD bit from the Master Mode Register is ignored.
Instead the Direction bits from ACR are used to setup the direction, read
or write, of each unitary transaction. Finally the 8bit counters must
filled with the data length of their respective transaction. Then if only
one transaction is to be used, the data length of the second one must be
set to zero. At the moment, this driver uses only the first transaction.
In addition to MMR and ACR, the Control Register also need to be written
to enable the alternative command mode. That's the purpose of its ACMEN
bit, which stands for Alternative Command Mode Enable.
Note that the alternative command mode is compatible with the use of the
Internal Address Register. So combined transactions for eeprom read are
actually implemented with the Internal Address Register. This register is
written with up to 3 bytes, which are the internal address sent to the
slave through the first write transaction. Then the first area of the ACR
describe the write transaction to follow, which carries the data to be
read from the eeprom. The second area of the ACR is not used so its Data
Length 8bit counter is cleared.
For each byte sent or received by the device, the Data Length 8bit counter
is decremented. When it reaches 0, a STOP condition is automatically sent.
Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/i2c/busses/i2c-at91.c | 121 +++++++++++++++++++++++++++++++++++-------
1 file changed, 101 insertions(+), 20 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 0e88b68..67b4f15 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -49,6 +49,11 @@
#define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */
#define AT91_TWI_QUICK BIT(6) /* SMBus quick command */
#define AT91_TWI_SWRST BIT(7) /* Software Reset */
+#define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */
+#define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */
+#define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */
+#define AT91_TWI_RHRCLR BIT(25) /* Receive Holding Register Clear */
+#define AT91_TWI_LOCKCLR BIT(26) /* Lock Clear */
#define AT91_TWI_MMR 0x0004 /* Master Mode Register */
#define AT91_TWI_IADRSZ_1 0x0100 /* Internal Device Address Size */
@@ -65,6 +70,7 @@
#define AT91_TWI_OVRE BIT(6) /* Overrun Error */
#define AT91_TWI_UNRE BIT(7) /* Underrun Error */
#define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
+#define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */
#define AT91_TWI_INT_MASK \
(AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK)
@@ -75,10 +81,15 @@
#define AT91_TWI_RHR 0x0030 /* Receive Holding Register */
#define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
+#define AT91_TWI_ACR 0x0040 /* Alternative Command Register */
+#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
+#define AT91_TWI_ACR_DIR BIT(8)
+
struct at91_twi_pdata {
unsigned clk_max_div;
unsigned clk_offset;
bool has_unre_flag;
+ bool has_alt_cmd;
struct at_dma_slave dma_slave;
};
@@ -204,7 +215,8 @@ static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
/* send stop when last byte has been written */
if (--dev->buf_len == 0)
- at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+ if (!dev->pdata->has_alt_cmd)
+ 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);
@@ -226,7 +238,8 @@ static void at91_twi_write_data_dma_callback(void *data)
* we just have to enable TXCOMP one.
*/
at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
- at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
+ if (!dev->pdata->has_alt_cmd)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
}
static void at91_twi_write_data_dma(struct at91_twi_dev *dev)
@@ -302,7 +315,7 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
}
/* send stop if second but last byte has been read */
- if (dev->buf_len == 1)
+ if (!dev->pdata->has_alt_cmd && 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);
@@ -313,14 +326,18 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
static void at91_twi_read_data_dma_callback(void *data)
{
struct at91_twi_dev *dev = (struct at91_twi_dev *)data;
+ unsigned ier = AT91_TWI_TXCOMP;
dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg),
dev->buf_len, DMA_FROM_DEVICE);
- /* The last two bytes have to be read without using dma */
- dev->buf += dev->buf_len - 2;
- dev->buf_len = 2;
- at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_RXRDY | AT91_TWI_TXCOMP);
+ if (!dev->pdata->has_alt_cmd) {
+ /* The last two bytes have to be read without using dma */
+ dev->buf += dev->buf_len - 2;
+ dev->buf_len = 2;
+ ier |= AT91_TWI_RXRDY;
+ }
+ at91_twi_write(dev, AT91_TWI_IER, ier);
}
static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
@@ -329,13 +346,14 @@ static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
struct dma_async_tx_descriptor *rxdesc;
struct at91_twi_dma *dma = &dev->dma;
struct dma_chan *chan_rx = dma->chan_rx;
+ size_t buf_len;
+ buf_len = (dev->pdata->has_alt_cmd) ? dev->buf_len : dev->buf_len - 2;
dma->direction = DMA_FROM_DEVICE;
/* Keep in mind that we won't use dma to read the last two bytes */
at91_twi_irq_save(dev);
- dma_addr = dma_map_single(dev->dev, dev->buf, dev->buf_len - 2,
- DMA_FROM_DEVICE);
+ dma_addr = dma_map_single(dev->dev, dev->buf, buf_len, DMA_FROM_DEVICE);
if (dma_mapping_error(dev->dev, dma_addr)) {
dev_err(dev->dev, "dma map failed\n");
return;
@@ -343,7 +361,7 @@ static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
dma->buf_mapped = true;
at91_twi_irq_restore(dev);
dma->sg.dma_address = dma_addr;
- sg_dma_len(&dma->sg) = dev->buf_len - 2;
+ sg_dma_len(&dma->sg) = buf_len;
rxdesc = dmaengine_prep_slave_sg(chan_rx, &dma->sg, 1, DMA_DEV_TO_MEM,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
@@ -394,6 +412,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
int ret;
unsigned long time_left;
bool has_unre_flag = dev->pdata->has_unre_flag;
+ bool has_alt_cmd = dev->pdata->has_alt_cmd;
/*
* WARNING: the TXCOMP bit in the Status Register is NOT a clear on
@@ -403,6 +422,21 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
* empty and STOP condition has been sent.
* Consequently, we should enable NACK interrupt rather than TXCOMP to
* detect transmission failure.
+ * Indeed let's take the case of an i2c write command using DMA.
+ * Whenever the slave doesn't acknowledge a byte, the LOCK, NACK and
+ * TXCOMP bits are set together into the Status Register.
+ * LOCK is a clear on write bit, which is set to prevent the DMA
+ * controller from sending new data on the i2c bus after a NACK
+ * condition has happened. Once locked, this i2c peripheral stops
+ * triggering the DMA controller for new data but it is more than
+ * likely that a new DMA transaction is already in progress, writing
+ * into the Transmit Holding Register. Since the peripheral is locked,
+ * these new data won't be sent to the i2c bus but they will remain
+ * into the Transmit Holding Register, so TXCOMP bit is cleared.
+ * Then when the interrupt handler is called, the Status Register is
+ * read: the TXCOMP bit is clear but NACK bit is still set. The driver
+ * manage the error properly, without waiting for timeout.
+ * This case can be reproduced easyly when writing into an at24 eeprom.
*
* Besides, the TXCOMP bit is already set before the i2c transaction
* has been started. For read transactions, this bit is cleared when
@@ -418,9 +452,9 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
* condition since we don't know whether the TXCOMP interrupt is enabled
* before or after the DMA has started to write into THR. So the TXCOMP
* interrupt is enabled later by at91_twi_write_data_dma_callback().
- * Immediately after in that DMA callback, we still need to send the
- * STOP condition manually writing the corresponding bit into the
- * Control Register.
+ * Immediately after in that DMA callback, if the alternative command
+ * mode is not used, we still need to send the STOP condition manually
+ * writing the corresponding bit into the Control Register.
*/
dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
@@ -441,14 +475,16 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
}
/* if only one byte is to be read, immediately stop transfer */
- if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
+ if (!has_alt_cmd && 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);
/*
- * When using dma, the last byte has to be read manually in
- * order to not send the stop command too late and then
- * to receive extra data. In practice, there are some issues
- * if you use the dma to read n-1 bytes because of latency.
+ * When using dma without alternative command mode, the last
+ * byte has to be read manually in order to not send the stop
+ * command too late and then to receive extra data.
+ * In practice, there are some issues if you use the dma to
+ * read n-1 bytes because of latency.
* Reading n-2 bytes with dma and the two last ones manually
* seems to be the best solution.
*/
@@ -477,6 +513,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
time_left = wait_for_completion_timeout(&dev->cmd_complete,
dev->adapter.timeout);
if (time_left == 0) {
+ dev->transfer_status |= at91_twi_read(dev, AT91_TWI_SR);
dev_err(dev->dev, "controller timed out\n");
at91_init_twi_bus(dev);
ret = -ETIMEDOUT;
@@ -497,6 +534,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
ret = -EIO;
goto error;
}
+ if (has_alt_cmd && (dev->transfer_status & AT91_TWI_LOCK)) {
+ dev_err(dev->dev, "tx locked\n");
+ ret = -EIO;
+ goto error;
+ }
if (dev->recv_len_abort) {
dev_err(dev->dev, "invalid smbus block length recvd\n");
ret = -EPROTO;
@@ -508,7 +550,14 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
return 0;
error:
+ /* first stop DMA transfer if still in progress */
at91_twi_dma_cleanup(dev);
+ /* then flush THR/FIFO and unlock TX if locked */
+ if (has_alt_cmd && (dev->transfer_status & AT91_TWI_LOCK)) {
+ dev_dbg(dev->dev, "unlock tx\n");
+ at91_twi_write(dev, AT91_TWI_CR,
+ AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
+ }
return ret;
}
@@ -518,6 +567,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
int ret;
unsigned int_addr_flag = 0;
struct i2c_msg *m_start = msg;
+ bool is_read, use_alt_cmd = false;
dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
@@ -540,8 +590,23 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
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));
+ is_read = (m_start->flags & I2C_M_RD);
+ if (dev->pdata->has_alt_cmd) {
+ if (m_start->len > 0) {
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_ACMEN);
+ at91_twi_write(dev, AT91_TWI_ACR,
+ AT91_TWI_ACR_DATAL(m_start->len) |
+ ((is_read) ? AT91_TWI_ACR_DIR : 0));
+ use_alt_cmd = true;
+ } else {
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_ACMDIS);
+ }
+ }
+
+ at91_twi_write(dev, AT91_TWI_MMR,
+ (m_start->addr << 16) |
+ int_addr_flag |
+ ((!use_alt_cmd && is_read) ? AT91_TWI_MREAD : 0));
dev->buf_len = m_start->len;
dev->buf = m_start->buf;
@@ -582,30 +647,35 @@ static struct at91_twi_pdata at91rm9200_config = {
.clk_max_div = 5,
.clk_offset = 3,
.has_unre_flag = true,
+ .has_alt_cmd = false,
};
static struct at91_twi_pdata at91sam9261_config = {
.clk_max_div = 5,
.clk_offset = 4,
.has_unre_flag = false,
+ .has_alt_cmd = false,
};
static struct at91_twi_pdata at91sam9260_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
+ .has_alt_cmd = false,
};
static struct at91_twi_pdata at91sam9g20_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
+ .has_alt_cmd = false,
};
static struct at91_twi_pdata at91sam9g10_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
+ .has_alt_cmd = false,
};
static const struct platform_device_id at91_twi_devtypes[] = {
@@ -634,6 +704,14 @@ static struct at91_twi_pdata at91sam9x5_config = {
.clk_max_div = 7,
.clk_offset = 4,
.has_unre_flag = false,
+ .has_alt_cmd = false,
+};
+
+static struct at91_twi_pdata at91sama5d2_config = {
+ .clk_max_div = 7,
+ .clk_offset = 4,
+ .has_unre_flag = true,
+ .has_alt_cmd = true,
};
static const struct of_device_id atmel_twi_dt_ids[] = {
@@ -656,6 +734,9 @@ static const struct of_device_id atmel_twi_dt_ids[] = {
.compatible = "atmel,at91sam9x5-i2c",
.data = &at91sam9x5_config,
}, {
+ .compatible = "atmel,at91sama5d2-i2c",
+ .data = &at91sama5d2_config,
+ }, {
/* sentinel */
}
};
--
1.8.2.2
The probe() function now prints the hardware version of the I2C
controller.
Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/i2c/busses/i2c-at91.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 67b4f15..89a8e61 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -85,6 +85,8 @@
#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
#define AT91_TWI_ACR_DIR BIT(8)
+#define AT91_TWI_VER 0x00fc /* Version Register */
+
struct at91_twi_pdata {
unsigned clk_max_div;
unsigned clk_offset;
@@ -908,7 +910,8 @@ static int at91_twi_probe(struct platform_device *pdev)
return rc;
}
- dev_info(dev->dev, "AT91 i2c bus driver.\n");
+ dev_info(dev->dev, "AT91 i2c bus driver (version: %#x).\n",
+ at91_twi_read(dev, AT91_TWI_VER));
return 0;
}
--
1.8.2.2
When FIFOs are available and enabled, the driver now configures the Atmel
eXtended DMA Controller to perform word accesses instead of byte accesses
when possible.
The actual access width depends on the size of the buffer to transmit.
To enable FIFO support the "atmel,fifo-size" property must be set properly
in the I2C controller node of the device tree.
Signed-off-by: Cyrille Pitchen <[email protected]>
---
drivers/i2c/busses/i2c-at91.c | 147 +++++++++++++++++++++++++++++++++++++-----
1 file changed, 130 insertions(+), 17 deletions(-)
diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
index 89a8e61..c4b1671 100644
--- a/drivers/i2c/busses/i2c-at91.c
+++ b/drivers/i2c/busses/i2c-at91.c
@@ -54,6 +54,8 @@
#define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */
#define AT91_TWI_RHRCLR BIT(25) /* Receive Holding Register Clear */
#define AT91_TWI_LOCKCLR BIT(26) /* Lock Clear */
+#define AT91_TWI_FIFOEN BIT(28) /* FIFO Enable */
+#define AT91_TWI_FIFODIS BIT(29) /* FIFO Disable */
#define AT91_TWI_MMR 0x0004 /* Master Mode Register */
#define AT91_TWI_IADRSZ_1 0x0100 /* Internal Device Address Size */
@@ -85,6 +87,22 @@
#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
#define AT91_TWI_ACR_DIR BIT(8)
+#define AT91_TWI_FMR 0x0050 /* FIFO Mode Register */
+#define AT91_TWI_FMR_TXRDYM(mode) (((mode) & 0x3) << 0)
+#define AT91_TWI_FMR_TXRDYM_MASK (0x3 << 0)
+#define AT91_TWI_FMR_RXRDYM(mode) (((mode) & 0x3) << 4)
+#define AT91_TWI_FMR_RXRDYM_MASK (0x3 << 4)
+#define AT91_TWI_ONE_DATA 0x0
+#define AT91_TWI_TWO_DATA 0x1
+#define AT91_TWI_FOUR_DATA 0x2
+
+#define AT91_TWI_FLR 0x0054 /* FIFO Level Register */
+
+#define AT91_TWI_FSR 0x0060 /* FIFO Status Register */
+#define AT91_TWI_FIER 0x0064 /* FIFO Interrupt Enable Register */
+#define AT91_TWI_FIDR 0x0068 /* FIFO Interrupt Disable Register */
+#define AT91_TWI_FIMR 0x006c /* FIFO Interrupt Mask Register */
+
#define AT91_TWI_VER 0x00fc /* Version Register */
struct at91_twi_pdata {
@@ -98,7 +116,7 @@ struct at91_twi_pdata {
struct at91_twi_dma {
struct dma_chan *chan_rx;
struct dma_chan *chan_tx;
- struct scatterlist sg;
+ struct scatterlist sg[2];
struct dma_async_tx_descriptor *data_desc;
enum dma_data_direction direction;
bool buf_mapped;
@@ -121,6 +139,7 @@ struct at91_twi_dev {
struct at91_twi_pdata *pdata;
bool use_dma;
bool recv_len_abort;
+ u32 fifo_size;
struct at91_twi_dma dma;
};
@@ -154,6 +173,9 @@ 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);
+ /* FIFO should be enabled immediately after the software reset */
+ if (dev->fifo_size)
+ at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_FIFOEN);
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);
@@ -200,7 +222,7 @@ static void at91_twi_dma_cleanup(struct at91_twi_dev *dev)
dma->xfer_in_progress = false;
}
if (dma->buf_mapped) {
- dma_unmap_single(dev->dev, sg_dma_address(&dma->sg),
+ dma_unmap_single(dev->dev, sg_dma_address(&dma->sg[0]),
dev->buf_len, dma->direction);
dma->buf_mapped = false;
}
@@ -213,7 +235,8 @@ 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);
+ /* 8bit write works with and without FIFO */
+ writeb_relaxed(*dev->buf, dev->base + AT91_TWI_THR);
/* send stop when last byte has been written */
if (--dev->buf_len == 0)
@@ -229,7 +252,7 @@ static void at91_twi_write_data_dma_callback(void *data)
{
struct at91_twi_dev *dev = (struct at91_twi_dev *)data;
- dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg),
+ dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg[0]),
dev->buf_len, DMA_TO_DEVICE);
/*
@@ -250,6 +273,7 @@ static void at91_twi_write_data_dma(struct at91_twi_dev *dev)
struct dma_async_tx_descriptor *txdesc;
struct at91_twi_dma *dma = &dev->dma;
struct dma_chan *chan_tx = dma->chan_tx;
+ unsigned int sg_len = 1;
if (dev->buf_len <= 0)
return;
@@ -265,10 +289,43 @@ static void at91_twi_write_data_dma(struct at91_twi_dev *dev)
}
dma->buf_mapped = true;
at91_twi_irq_restore(dev);
- sg_dma_len(&dma->sg) = dev->buf_len;
- sg_dma_address(&dma->sg) = dma_addr;
- txdesc = dmaengine_prep_slave_sg(chan_tx, &dma->sg, 1, DMA_MEM_TO_DEV,
+ if (dev->fifo_size) {
+ size_t part1_len, part2_len;
+ struct scatterlist *sg;
+ unsigned fifo_mr;
+
+ sg_len = 0;
+
+ part1_len = dev->buf_len & ~0x3;
+ if (part1_len) {
+ sg = &dma->sg[sg_len++];
+ sg_dma_len(sg) = part1_len;
+ sg_dma_address(sg) = dma_addr;
+ }
+
+ part2_len = dev->buf_len & 0x3;
+ if (part2_len) {
+ sg = &dma->sg[sg_len++];
+ sg_dma_len(sg) = part2_len;
+ sg_dma_address(sg) = dma_addr + part1_len;
+ }
+
+ /*
+ * DMA controller is triggered when at least 4 data can be
+ * written into the TX FIFO
+ */
+ fifo_mr = at91_twi_read(dev, AT91_TWI_FMR);
+ fifo_mr &= ~AT91_TWI_FMR_TXRDYM_MASK;
+ fifo_mr |= AT91_TWI_FMR_TXRDYM(AT91_TWI_FOUR_DATA);
+ at91_twi_write(dev, AT91_TWI_FMR, fifo_mr);
+ } else {
+ sg_dma_len(&dma->sg[0]) = dev->buf_len;
+ sg_dma_address(&dma->sg[0]) = dma_addr;
+ }
+
+ txdesc = dmaengine_prep_slave_sg(chan_tx, dma->sg, sg_len,
+ DMA_MEM_TO_DEV,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
if (!txdesc) {
dev_err(dev->dev, "dma prep slave sg failed\n");
@@ -293,7 +350,8 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
if (dev->buf_len <= 0)
return;
- *dev->buf = at91_twi_read(dev, AT91_TWI_RHR) & 0xff;
+ /* 8bit read works with and without FIFO */
+ *dev->buf = readb_relaxed(dev->base + AT91_TWI_RHR);
--dev->buf_len;
/* return if aborting, we only needed to read RHR to clear RXRDY*/
@@ -330,7 +388,7 @@ static void at91_twi_read_data_dma_callback(void *data)
struct at91_twi_dev *dev = (struct at91_twi_dev *)data;
unsigned ier = AT91_TWI_TXCOMP;
- dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg),
+ dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg[0]),
dev->buf_len, DMA_FROM_DEVICE);
if (!dev->pdata->has_alt_cmd) {
@@ -362,10 +420,24 @@ static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
}
dma->buf_mapped = true;
at91_twi_irq_restore(dev);
- dma->sg.dma_address = dma_addr;
- sg_dma_len(&dma->sg) = buf_len;
- rxdesc = dmaengine_prep_slave_sg(chan_rx, &dma->sg, 1, DMA_DEV_TO_MEM,
+ if (dev->fifo_size && IS_ALIGNED(buf_len, 4)) {
+ unsigned fifo_mr;
+
+ /*
+ * DMA controller is triggered when at least 4 data can be
+ * read from the RX FIFO
+ */
+ fifo_mr = at91_twi_read(dev, AT91_TWI_FMR);
+ fifo_mr &= ~AT91_TWI_FMR_RXRDYM_MASK;
+ fifo_mr |= AT91_TWI_FMR_RXRDYM(AT91_TWI_FOUR_DATA);
+ at91_twi_write(dev, AT91_TWI_FMR, fifo_mr);
+ }
+
+ sg_dma_len(&dma->sg[0]) = buf_len;
+ sg_dma_address(&dma->sg[0]) = dma_addr;
+
+ rxdesc = dmaengine_prep_slave_sg(chan_rx, dma->sg, 1, DMA_DEV_TO_MEM,
DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
if (!rxdesc) {
dev_err(dev->dev, "dma prep slave sg failed\n");
@@ -465,6 +537,21 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
reinit_completion(&dev->cmd_complete);
dev->transfer_status = 0;
+ if (dev->fifo_size) {
+ unsigned fifo_mr = at91_twi_read(dev, AT91_TWI_FMR);
+
+ /* Reset FIFO mode register */
+ fifo_mr &= ~(AT91_TWI_FMR_TXRDYM_MASK |
+ AT91_TWI_FMR_RXRDYM_MASK);
+ fifo_mr |= AT91_TWI_FMR_TXRDYM(AT91_TWI_ONE_DATA);
+ fifo_mr |= AT91_TWI_FMR_RXRDYM(AT91_TWI_ONE_DATA);
+ at91_twi_write(dev, AT91_TWI_FMR, fifo_mr);
+
+ /* Flush FIFOs */
+ at91_twi_write(dev, AT91_TWI_CR,
+ AT91_TWI_THRCLR | AT91_TWI_RHRCLR);
+ }
+
if (!dev->buf_len) {
at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_QUICK);
at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
@@ -536,7 +623,8 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
ret = -EIO;
goto error;
}
- if (has_alt_cmd && (dev->transfer_status & AT91_TWI_LOCK)) {
+ if ((has_alt_cmd || dev->fifo_size) &&
+ (dev->transfer_status & AT91_TWI_LOCK)) {
dev_err(dev->dev, "tx locked\n");
ret = -EIO;
goto error;
@@ -555,7 +643,8 @@ error:
/* first stop DMA transfer if still in progress */
at91_twi_dma_cleanup(dev);
/* then flush THR/FIFO and unlock TX if locked */
- if (has_alt_cmd && (dev->transfer_status & AT91_TWI_LOCK)) {
+ if ((has_alt_cmd || dev->fifo_size) &&
+ (dev->transfer_status & AT91_TWI_LOCK)) {
dev_dbg(dev->dev, "unlock tx\n");
at91_twi_write(dev, AT91_TWI_CR,
AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
@@ -750,13 +839,32 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
int ret = 0;
struct dma_slave_config slave_config;
struct at91_twi_dma *dma = &dev->dma;
+ enum dma_slave_buswidth addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+
+ /*
+ * The actual width of the access will be chosen in
+ * dmaengine_prep_slave_sg():
+ * for each buffer in the scatter-gather list, if its size is aligned
+ * to addr_width then addr_width accesses will be performed to transfer
+ * the buffer. On the other hand, if the buffer size is not aligned to
+ * addr_width then the buffer is transferred using single byte accesses.
+ * Please refer to the Atmel eXtended DMA controller driver.
+ * When FIFOs are used, the TXRDYM threshold can always be set to
+ * trigger the XDMAC when at least 4 data can be written into the TX
+ * FIFO, even if single byte accesses are performed.
+ * However the RXRDYM threshold must be set to fit the access width,
+ * deduced from buffer length, so the XDMAC is triggered properly to
+ * read data from the RX FIFO.
+ */
+ if (dev->fifo_size)
+ addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
memset(&slave_config, 0, sizeof(slave_config));
slave_config.src_addr = (dma_addr_t)phy_addr + AT91_TWI_RHR;
- slave_config.src_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ slave_config.src_addr_width = addr_width;
slave_config.src_maxburst = 1;
slave_config.dst_addr = (dma_addr_t)phy_addr + AT91_TWI_THR;
- slave_config.dst_addr_width = DMA_SLAVE_BUSWIDTH_1_BYTE;
+ slave_config.dst_addr_width = addr_width;
slave_config.dst_maxburst = 1;
slave_config.device_fc = false;
@@ -788,7 +896,7 @@ static int at91_twi_configure_dma(struct at91_twi_dev *dev, u32 phy_addr)
goto error;
}
- sg_init_table(&dma->sg, 1);
+ sg_init_table(dma->sg, 2);
dma->buf_mapped = false;
dma->xfer_in_progress = false;
dev->use_dma = true;
@@ -874,6 +982,11 @@ static int at91_twi_probe(struct platform_device *pdev)
return rc;
}
+ if (!of_property_read_u32(pdev->dev.of_node, "atmel,fifo-size",
+ &dev->fifo_size)) {
+ dev_info(dev->dev, "Using FIFO (%u data)\n", dev->fifo_size);
+ }
+
rc = of_property_read_u32(dev->dev->of_node, "clock-frequency",
&bus_clk_rate);
if (rc)
--
1.8.2.2
For the whole set of patches:
Acked-by: Ludovic Desroches
Thanks Cyrille
On Wed, Jun 03, 2015 at 06:25:16PM +0200, Cyrille Pitchen wrote:
> ChangeLog
>
> v5:
> - print I2C controller version in an already existing dev_info() instead of
> adding a new one.
>
> v4:
> - replace 0x%x by %#x when printing I2C controller version
> - change the order of patches: the race condition bug fix becomes the first
> patch so it be can more easily applied to older kernels.
>
> v3:
> - fix braces {} coding style issue
> - split the alternative command patch into 2 patches: the first one fixes
> a race condition whereas the second one is the actual alternative command
> patch
>
> v2:
> - fix typo in comment for AT91_TWI_SVEN.
> - document new device tree bindings like "atmel,fifo-size".
> - explicitly set the has_alt_cmd boolean to false to already existing chip
> configs.
> - use the BIT() macro to define the register bits and do a little cleanup in a
> dedicated patch.
> - reword some comments to better explain why the TXCOMP interrupt is no longer
> enabled in at91_do_twi_transfer() but later in
> at91_twi_write_data_dma_callback() to avoid a race condition when DMA is used.
> - remove useless TXCOMP interrupt enable line in at91_twi_write_next_byte()
> since this interrupt is also enabled by at91_do_twi_transfer() for PIO
> transfers.
>
> v1:
> This series of patches adds support of two new features which will be
> introduced with Atmel sama5d2x SoC.
>
> First, the alternative command mode eases the sending of STOP conditions.
> Before starting an I2C transaction, the size data to be transfered is
> written into the new Alternative Command Register. For each byte transferred,
> the I2C controller decreases this counter and automatically sends a STOP
> condition when the counter value reaches 0, that is to say when the last byte
> of the transaction has been sent/received. So there is no longer need to set
> the STOP bit into the Control Register.
>
> Then the use of FIFOs allows to reduce number I/O accesses: for instance,
> the TX FIFO allows to write up to 4 data in a single access to the Transmit
> Holding Register. Also the RX FIFO allows to read up to 4 data in a single
> access to the Receive Holding Register. Currently only DMA transfers take
> advantage of FIFOs.
>
> Cyrille Pitchen (6):
> i2c: at91: fix a race condition when using the DMA controller
> i2c: at91: use BIT() macro to define register bits
> i2c: at91: update documentation for DT bindings
> i2c: at91: add support for new alternative command mode
> i2c: at91: print hardware version
> i2c: at91: add support to FIFOs
>
> Documentation/devicetree/bindings/i2c/i2c-at91.txt | 29 +-
> drivers/i2c/busses/i2c-at91.c | 356 +++++++++++++++++----
> 2 files changed, 322 insertions(+), 63 deletions(-)
>
> --
> 1.8.2.2
>
Le 03/06/2015 18:25, Cyrille Pitchen a ?crit :
> add a new value "atmel,at91sama5d2-i2c" for the "compatible" property.
Well, here I would like "atmel,sama5d2-i2c" like any other "SAMA5"
product compatibility string out there...
Aka NAK for me ;-)
> add a new optional property "atmel,fifo-size" to enable FIFO support when
> available.
> add missing optional properties "dmas" and "dma-names".
>
> Signed-off-by: Cyrille Pitchen <[email protected]>
> ---
> Documentation/devicetree/bindings/i2c/i2c-at91.txt | 29 ++++++++++++++++++++--
> 1 file changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-at91.txt b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> index 388f0a2..7c04fd9 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-at91.txt
> @@ -2,8 +2,8 @@ I2C for Atmel platforms
>
> Required properties :
> - compatible : Must be "atmel,at91rm9200-i2c", "atmel,at91sam9261-i2c",
> - "atmel,at91sam9260-i2c", "atmel,at91sam9g20-i2c", "atmel,at91sam9g10-i2c"
> - or "atmel,at91sam9x5-i2c"
> + "atmel,at91sam9260-i2c", "atmel,at91sam9g20-i2c", "atmel,at91sam9g10-i2c",
> + "atmel,at91sam9x5-i2c" or "atmel,at91sama5d2-i2c"
Ditto
> - reg: physical base address of the controller and length of memory mapped
> region.
> - interrupts: interrupt number to the cpu.
> @@ -13,6 +13,9 @@ Required properties :
>
> Optional properties:
> - clock-frequency: Desired I2C bus frequency in Hz, otherwise defaults to 100000
> +- dmas: A list of two dma specifiers, one for each entry in dma-names.
> +- dma-names: should contain "tx" and "rx".
> +- atmel,fifo-size: size of the RX and TX FIFOs, if available.
> - Child nodes conforming to i2c bus binding
>
> Examples :
> @@ -32,3 +35,25 @@ i2c0: i2c@fff84000 {
> pagesize = <128>;
> }
> }
> +
> +i2c0: i2c@f8034600 {
> + compatible = "atmel,at91sama5d2-i2c";
Ditto
> + reg = <0xf8034600 0x100>;
> + interrupts = <19 IRQ_TYPE_LEVEL_HIGH 7>;
> + dmas = <&dma0
> + (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1))
> + AT91_XDMAC_DT_PERID(11)>,
> + <&dma0
> + (AT91_XDMAC_DT_MEM_IF(0) | AT91_XDMAC_DT_PER_IF(1))
> + AT91_XDMAC_DT_PERID(12)>;
> + dma-names = "tx", "rx";
> + #address-cells = <1>;
> + #size-cells = <0>;
> + clocks = <&flx0>;
> + atmel,fifo-size = <32>;
> +
> + wm8731: wm8731@1a {
> + compatible = "wm8731";
> + reg = <0x1a>;
> + };
> +};
>
--
Nicolas Ferre
Le 03/06/2015 18:25, Cyrille Pitchen a ?crit :
> The alternative command mode was introduced to simplify the transmission
> of STOP conditions and to solve timing and latency issues around them.
>
> This mode relies on a new register, the Alternative Command Register,
> which must be set at the same time as the Master Mode Register. This new
> register was designed to allow simple setup of basic combined transactions
> built from up to two unitary transactions.
>
> Indeed, the ACR is split into two areas, which describe one unitary
> transaction each. Each area is filled with Data Length 8bit counter, a
> Direction and a PEC Request bit. The PEC bit is only used in SMBus mode
> and is not supported by this driver yet. Also when using alternative
> command mode, the MREAD bit from the Master Mode Register is ignored.
> Instead the Direction bits from ACR are used to setup the direction, read
> or write, of each unitary transaction. Finally the 8bit counters must
> filled with the data length of their respective transaction. Then if only
> one transaction is to be used, the data length of the second one must be
> set to zero. At the moment, this driver uses only the first transaction.
>
> In addition to MMR and ACR, the Control Register also need to be written
> to enable the alternative command mode. That's the purpose of its ACMEN
> bit, which stands for Alternative Command Mode Enable.
>
> Note that the alternative command mode is compatible with the use of the
> Internal Address Register. So combined transactions for eeprom read are
> actually implemented with the Internal Address Register. This register is
> written with up to 3 bytes, which are the internal address sent to the
> slave through the first write transaction. Then the first area of the ACR
> describe the write transaction to follow, which carries the data to be
> read from the eeprom. The second area of the ACR is not used so its Data
> Length 8bit counter is cleared.
>
> For each byte sent or received by the device, the Data Length 8bit counter
> is decremented. When it reaches 0, a STOP condition is automatically sent.
>
> Signed-off-by: Cyrille Pitchen <[email protected]>
> ---
> drivers/i2c/busses/i2c-at91.c | 121 +++++++++++++++++++++++++++++++++++-------
> 1 file changed, 101 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-at91.c b/drivers/i2c/busses/i2c-at91.c
> index 0e88b68..67b4f15 100644
> --- a/drivers/i2c/busses/i2c-at91.c
> +++ b/drivers/i2c/busses/i2c-at91.c
> @@ -49,6 +49,11 @@
> #define AT91_TWI_SVDIS BIT(5) /* Slave Transfer Disable */
> #define AT91_TWI_QUICK BIT(6) /* SMBus quick command */
> #define AT91_TWI_SWRST BIT(7) /* Software Reset */
> +#define AT91_TWI_ACMEN BIT(16) /* Alternative Command Mode Enable */
> +#define AT91_TWI_ACMDIS BIT(17) /* Alternative Command Mode Disable */
> +#define AT91_TWI_THRCLR BIT(24) /* Transmit Holding Register Clear */
> +#define AT91_TWI_RHRCLR BIT(25) /* Receive Holding Register Clear */
> +#define AT91_TWI_LOCKCLR BIT(26) /* Lock Clear */
>
> #define AT91_TWI_MMR 0x0004 /* Master Mode Register */
> #define AT91_TWI_IADRSZ_1 0x0100 /* Internal Device Address Size */
> @@ -65,6 +70,7 @@
> #define AT91_TWI_OVRE BIT(6) /* Overrun Error */
> #define AT91_TWI_UNRE BIT(7) /* Underrun Error */
> #define AT91_TWI_NACK BIT(8) /* Not Acknowledged */
> +#define AT91_TWI_LOCK BIT(23) /* TWI Lock due to Frame Errors */
>
> #define AT91_TWI_INT_MASK \
> (AT91_TWI_TXCOMP | AT91_TWI_RXRDY | AT91_TWI_TXRDY | AT91_TWI_NACK)
> @@ -75,10 +81,15 @@
> #define AT91_TWI_RHR 0x0030 /* Receive Holding Register */
> #define AT91_TWI_THR 0x0034 /* Transmit Holding Register */
>
> +#define AT91_TWI_ACR 0x0040 /* Alternative Command Register */
> +#define AT91_TWI_ACR_DATAL(len) ((len) & 0xff)
> +#define AT91_TWI_ACR_DIR BIT(8)
> +
> struct at91_twi_pdata {
> unsigned clk_max_div;
> unsigned clk_offset;
> bool has_unre_flag;
> + bool has_alt_cmd;
> struct at_dma_slave dma_slave;
> };
>
> @@ -204,7 +215,8 @@ static void at91_twi_write_next_byte(struct at91_twi_dev *dev)
>
> /* send stop when last byte has been written */
> if (--dev->buf_len == 0)
> - at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> + if (!dev->pdata->has_alt_cmd)
> + 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);
>
> @@ -226,7 +238,8 @@ static void at91_twi_write_data_dma_callback(void *data)
> * we just have to enable TXCOMP one.
> */
> at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_TXCOMP);
> - at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> + if (!dev->pdata->has_alt_cmd)
> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_STOP);
> }
>
> static void at91_twi_write_data_dma(struct at91_twi_dev *dev)
> @@ -302,7 +315,7 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
> }
>
> /* send stop if second but last byte has been read */
> - if (dev->buf_len == 1)
> + if (!dev->pdata->has_alt_cmd && 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);
> @@ -313,14 +326,18 @@ static void at91_twi_read_next_byte(struct at91_twi_dev *dev)
> static void at91_twi_read_data_dma_callback(void *data)
> {
> struct at91_twi_dev *dev = (struct at91_twi_dev *)data;
> + unsigned ier = AT91_TWI_TXCOMP;
>
> dma_unmap_single(dev->dev, sg_dma_address(&dev->dma.sg),
> dev->buf_len, DMA_FROM_DEVICE);
>
> - /* The last two bytes have to be read without using dma */
> - dev->buf += dev->buf_len - 2;
> - dev->buf_len = 2;
> - at91_twi_write(dev, AT91_TWI_IER, AT91_TWI_RXRDY | AT91_TWI_TXCOMP);
> + if (!dev->pdata->has_alt_cmd) {
> + /* The last two bytes have to be read without using dma */
> + dev->buf += dev->buf_len - 2;
> + dev->buf_len = 2;
> + ier |= AT91_TWI_RXRDY;
> + }
> + at91_twi_write(dev, AT91_TWI_IER, ier);
> }
>
> static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
> @@ -329,13 +346,14 @@ static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
> struct dma_async_tx_descriptor *rxdesc;
> struct at91_twi_dma *dma = &dev->dma;
> struct dma_chan *chan_rx = dma->chan_rx;
> + size_t buf_len;
>
> + buf_len = (dev->pdata->has_alt_cmd) ? dev->buf_len : dev->buf_len - 2;
> dma->direction = DMA_FROM_DEVICE;
>
> /* Keep in mind that we won't use dma to read the last two bytes */
> at91_twi_irq_save(dev);
> - dma_addr = dma_map_single(dev->dev, dev->buf, dev->buf_len - 2,
> - DMA_FROM_DEVICE);
> + dma_addr = dma_map_single(dev->dev, dev->buf, buf_len, DMA_FROM_DEVICE);
> if (dma_mapping_error(dev->dev, dma_addr)) {
> dev_err(dev->dev, "dma map failed\n");
> return;
> @@ -343,7 +361,7 @@ static void at91_twi_read_data_dma(struct at91_twi_dev *dev)
> dma->buf_mapped = true;
> at91_twi_irq_restore(dev);
> dma->sg.dma_address = dma_addr;
> - sg_dma_len(&dma->sg) = dev->buf_len - 2;
> + sg_dma_len(&dma->sg) = buf_len;
>
> rxdesc = dmaengine_prep_slave_sg(chan_rx, &dma->sg, 1, DMA_DEV_TO_MEM,
> DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
> @@ -394,6 +412,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> int ret;
> unsigned long time_left;
> bool has_unre_flag = dev->pdata->has_unre_flag;
> + bool has_alt_cmd = dev->pdata->has_alt_cmd;
>
> /*
> * WARNING: the TXCOMP bit in the Status Register is NOT a clear on
> @@ -403,6 +422,21 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> * empty and STOP condition has been sent.
> * Consequently, we should enable NACK interrupt rather than TXCOMP to
> * detect transmission failure.
> + * Indeed let's take the case of an i2c write command using DMA.
> + * Whenever the slave doesn't acknowledge a byte, the LOCK, NACK and
> + * TXCOMP bits are set together into the Status Register.
> + * LOCK is a clear on write bit, which is set to prevent the DMA
> + * controller from sending new data on the i2c bus after a NACK
> + * condition has happened. Once locked, this i2c peripheral stops
> + * triggering the DMA controller for new data but it is more than
> + * likely that a new DMA transaction is already in progress, writing
> + * into the Transmit Holding Register. Since the peripheral is locked,
> + * these new data won't be sent to the i2c bus but they will remain
> + * into the Transmit Holding Register, so TXCOMP bit is cleared.
> + * Then when the interrupt handler is called, the Status Register is
> + * read: the TXCOMP bit is clear but NACK bit is still set. The driver
> + * manage the error properly, without waiting for timeout.
> + * This case can be reproduced easyly when writing into an at24 eeprom.
> *
> * Besides, the TXCOMP bit is already set before the i2c transaction
> * has been started. For read transactions, this bit is cleared when
> @@ -418,9 +452,9 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> * condition since we don't know whether the TXCOMP interrupt is enabled
> * before or after the DMA has started to write into THR. So the TXCOMP
> * interrupt is enabled later by at91_twi_write_data_dma_callback().
> - * Immediately after in that DMA callback, we still need to send the
> - * STOP condition manually writing the corresponding bit into the
> - * Control Register.
> + * Immediately after in that DMA callback, if the alternative command
> + * mode is not used, we still need to send the STOP condition manually
> + * writing the corresponding bit into the Control Register.
> */
>
> dev_dbg(dev->dev, "transfer: %s %d bytes.\n",
> @@ -441,14 +475,16 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> }
>
> /* if only one byte is to be read, immediately stop transfer */
> - if (dev->buf_len <= 1 && !(dev->msg->flags & I2C_M_RECV_LEN))
> + if (!has_alt_cmd && 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);
> /*
> - * When using dma, the last byte has to be read manually in
> - * order to not send the stop command too late and then
> - * to receive extra data. In practice, there are some issues
> - * if you use the dma to read n-1 bytes because of latency.
> + * When using dma without alternative command mode, the last
> + * byte has to be read manually in order to not send the stop
> + * command too late and then to receive extra data.
> + * In practice, there are some issues if you use the dma to
> + * read n-1 bytes because of latency.
> * Reading n-2 bytes with dma and the two last ones manually
> * seems to be the best solution.
> */
> @@ -477,6 +513,7 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> time_left = wait_for_completion_timeout(&dev->cmd_complete,
> dev->adapter.timeout);
> if (time_left == 0) {
> + dev->transfer_status |= at91_twi_read(dev, AT91_TWI_SR);
> dev_err(dev->dev, "controller timed out\n");
> at91_init_twi_bus(dev);
> ret = -ETIMEDOUT;
> @@ -497,6 +534,11 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> ret = -EIO;
> goto error;
> }
> + if (has_alt_cmd && (dev->transfer_status & AT91_TWI_LOCK)) {
> + dev_err(dev->dev, "tx locked\n");
> + ret = -EIO;
> + goto error;
> + }
> if (dev->recv_len_abort) {
> dev_err(dev->dev, "invalid smbus block length recvd\n");
> ret = -EPROTO;
> @@ -508,7 +550,14 @@ static int at91_do_twi_transfer(struct at91_twi_dev *dev)
> return 0;
>
> error:
> + /* first stop DMA transfer if still in progress */
> at91_twi_dma_cleanup(dev);
> + /* then flush THR/FIFO and unlock TX if locked */
> + if (has_alt_cmd && (dev->transfer_status & AT91_TWI_LOCK)) {
> + dev_dbg(dev->dev, "unlock tx\n");
> + at91_twi_write(dev, AT91_TWI_CR,
> + AT91_TWI_THRCLR | AT91_TWI_LOCKCLR);
> + }
> return ret;
> }
>
> @@ -518,6 +567,7 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> int ret;
> unsigned int_addr_flag = 0;
> struct i2c_msg *m_start = msg;
> + bool is_read, use_alt_cmd = false;
>
> dev_dbg(&adap->dev, "at91_xfer: processing %d messages:\n", num);
>
> @@ -540,8 +590,23 @@ static int at91_twi_xfer(struct i2c_adapter *adap, struct i2c_msg *msg, int num)
> 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));
> + is_read = (m_start->flags & I2C_M_RD);
> + if (dev->pdata->has_alt_cmd) {
> + if (m_start->len > 0) {
> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_ACMEN);
> + at91_twi_write(dev, AT91_TWI_ACR,
> + AT91_TWI_ACR_DATAL(m_start->len) |
> + ((is_read) ? AT91_TWI_ACR_DIR : 0));
> + use_alt_cmd = true;
> + } else {
> + at91_twi_write(dev, AT91_TWI_CR, AT91_TWI_ACMDIS);
> + }
> + }
> +
> + at91_twi_write(dev, AT91_TWI_MMR,
> + (m_start->addr << 16) |
> + int_addr_flag |
> + ((!use_alt_cmd && is_read) ? AT91_TWI_MREAD : 0));
>
> dev->buf_len = m_start->len;
> dev->buf = m_start->buf;
> @@ -582,30 +647,35 @@ static struct at91_twi_pdata at91rm9200_config = {
> .clk_max_div = 5,
> .clk_offset = 3,
> .has_unre_flag = true,
> + .has_alt_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9261_config = {
> .clk_max_div = 5,
> .clk_offset = 4,
> .has_unre_flag = false,
> + .has_alt_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9260_config = {
> .clk_max_div = 7,
> .clk_offset = 4,
> .has_unre_flag = false,
> + .has_alt_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9g20_config = {
> .clk_max_div = 7,
> .clk_offset = 4,
> .has_unre_flag = false,
> + .has_alt_cmd = false,
> };
>
> static struct at91_twi_pdata at91sam9g10_config = {
> .clk_max_div = 7,
> .clk_offset = 4,
> .has_unre_flag = false,
> + .has_alt_cmd = false,
> };
>
> static const struct platform_device_id at91_twi_devtypes[] = {
> @@ -634,6 +704,14 @@ static struct at91_twi_pdata at91sam9x5_config = {
> .clk_max_div = 7,
> .clk_offset = 4,
> .has_unre_flag = false,
> + .has_alt_cmd = false,
> +};
> +
> +static struct at91_twi_pdata at91sama5d2_config = {
No, please name it:
"sama5d2_config"
> + .clk_max_div = 7,
> + .clk_offset = 4,
> + .has_unre_flag = true,
> + .has_alt_cmd = true,
> };
>
> static const struct of_device_id atmel_twi_dt_ids[] = {
> @@ -656,6 +734,9 @@ static const struct of_device_id atmel_twi_dt_ids[] = {
> .compatible = "atmel,at91sam9x5-i2c",
> .data = &at91sam9x5_config,
> }, {
> + .compatible = "atmel,at91sama5d2-i2c",
> + .data = &at91sama5d2_config,
Please remove the "at91" for these products and use "atmel,sama5d2-i2c"
Thanks.
> + }, {
> /* sentinel */
> }
> };
>
--
Nicolas Ferre
Nicolas,
> > +static struct at91_twi_pdata at91sama5d2_config = {
>
> No, please name it:
>
> "sama5d2_config"
Thanks for your input, but please don't quote the whole mail for those
tiny comments. Only the relevant parts, please. Otherwise it is too easy
to miss things.
Thanks,
Wolfram