Re: [U-Boot] [PATCH 3/4] i2c: i2c-cdns: Implement workaround for hold quirk of the rev 1.0

[Michal Simek]

On 27.12.2016 23:46, Moritz Fischer wrote:
> Revision 1.0 of this IP has a quirk where if during a long read transfer
> the transfer_size register will go to 0, the master will send a NACK to
> the slave prematurely.
> The way to work around this is to reprogram the transfer_size register
> mid-transfer when the only the receive fifo is known full, i.e. the I2C
> bus is known non-active.
> The workaround is based on the implementation in the linux-kernel.
> 
> Signed-off-by: Moritz Fischer 
> Cc: Heiko Schocher 
> Cc: Michal Simek 
> Cc: u-boot@lists.denx.de
> ---
>  drivers/i2c/i2c-cdns.c | 121 
> -
>  1 file changed, 89 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-cdns.c b/drivers/i2c/i2c-cdns.c
> index 9a1b520..4a46dbf 100644
> --- a/drivers/i2c/i2c-cdns.c
> +++ b/drivers/i2c/i2c-cdns.c
> @@ -17,6 +17,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  DECLARE_GLOBAL_DATA_PTR;
>  
> @@ -67,6 +68,8 @@ struct cdns_i2c_regs {
>  
>  #define CDNS_I2C_FIFO_DEPTH  16
>  #define CDNS_I2C_TRANSFER_SIZE_MAX   255 /* Controller transfer limit */
> +#define CDNS_I2C_TRANSFER_SIZE   (CDNS_I2C_TRANSFER_SIZE_MAX - 3)
> +
>  #define CDNS_I2C_BROKEN_HOLD_BIT BIT(0)
>  
>  #ifdef DEBUG
> @@ -247,15 +250,20 @@ static int cdns_i2c_write_data(struct i2c_cdns_bus 
> *i2c_bus, u32 addr, u8 *data,
>  u32 len)
>  {
>   u8 *cur_data = data;
> -
>   struct cdns_i2c_regs *regs = i2c_bus->regs;
>  
> + /* Set the controller in Master transmit mode and clear FIFO */
>   setbits_le32(>control, CDNS_I2C_CONTROL_CLR_FIFO);
> -
> -
>   clrbits_le32(>control, CDNS_I2C_CONTROL_RW);
>  
> + /* Check message size against FIFO depth, and set hold bus bit
> +  * if it is greater than FIFO depth */
> + if (len > CDNS_I2C_FIFO_DEPTH)
> + setbits_le32(>control, CDNS_I2C_CONTROL_HOLD);
> +
> + /* Clear the interrupts in status register */
>   writel(0xFF, >interrupt_status);
> +
>   writel(addr, >address);
>  
>   while (len--) {
> @@ -280,48 +288,98 @@ static int cdns_i2c_write_data(struct i2c_cdns_bus 
> *i2c_bus, u32 addr, u8 *data,
>   return 0;
>  }
>  
> +static inline bool cdns_is_hold_quirk(int hold_quirk, int curr_recv_count)
> +{
> + return hold_quirk && (curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1);
> +}
> +
>  static int cdns_i2c_read_data(struct i2c_cdns_bus *i2c_bus, u32 addr, u8 
> *data,
> -   u32 len)
> +   u32 recv_count)
>  {
> - u32 status;
> - u32 i = 0;
>   u8 *cur_data = data;
> -
> - /* TODO: Fix this */
>   struct cdns_i2c_regs *regs = i2c_bus->regs;
> + int curr_recv_count;
> + int updatetx, hold_quirk;
>  
>   /* Check the hardware can handle the requested bytes */
> - if ((len < 0))
> + if ((recv_count < 0))
>   return -EINVAL;
>  
> + curr_recv_count = recv_count;
> +
> + /* Check for the message size against the FIFO depth */
> + if (recv_count > CDNS_I2C_FIFO_DEPTH)
> + setbits_le32(>control, CDNS_I2C_CONTROL_HOLD);
> +
>   setbits_le32(>control, CDNS_I2C_CONTROL_CLR_FIFO |
>   CDNS_I2C_CONTROL_RW);
>  
> + if (recv_count > CDNS_I2C_TRANSFER_SIZE) {
> + curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
> + writel(curr_recv_count, >transfer_size);
> + } else {
> + writel(recv_count, >transfer_size);
> + }
> +
>   /* Start reading data */
>   writel(addr, >address);
> - writel(len, >transfer_size);
> -
> - /* Wait for data */
> - do {
> - status = cdns_i2c_wait(regs, CDNS_I2C_INTERRUPT_COMP |
> - CDNS_I2C_INTERRUPT_DATA);
> - if (!status) {
> - /* Release the bus */
> - clrbits_le32(>control, CDNS_I2C_CONTROL_HOLD);
> - return -ETIMEDOUT;
> +
> + updatetx = recv_count > curr_recv_count;
> +
> + hold_quirk = (i2c_bus->quirks & CDNS_I2C_BROKEN_HOLD_BIT) && updatetx;
> +
> + while (recv_count) {
> + while (readl(>status) & CDNS_I2C_STATUS_RXDV) {
> + if (recv_count < CDNS_I2C_FIFO_DEPTH &&
> + !i2c_bus->hold_flag) {
> + clrbits_le32(>control,
> +  CDNS_I2C_CONTROL_HOLD);
> + }
> + *(cur_data)++ = readl(>data);
> + recv_count--;
> + curr_recv_count--;
> +
> + if (cdns_is_hold_quirk(hold_quirk, curr_recv_count))
> + break;
>   }
> - debug("Read %d bytes\n",
> -   len - readl(>transfer_size));
> - for (; i < len - readl(>transfer_size); i++)
> -   

[U-Boot] [PATCH 3/4] i2c: i2c-cdns: Implement workaround for hold quirk of the rev 1.0

[Moritz Fischer]

Revision 1.0 of this IP has a quirk where if during a long read transfer
the transfer_size register will go to 0, the master will send a NACK to
the slave prematurely.
The way to work around this is to reprogram the transfer_size register
mid-transfer when the only the receive fifo is known full, i.e. the I2C
bus is known non-active.
The workaround is based on the implementation in the linux-kernel.

Signed-off-by: Moritz Fischer 
Cc: Heiko Schocher 
Cc: Michal Simek 
Cc: u-boot@lists.denx.de
---
 drivers/i2c/i2c-cdns.c | 121 -
 1 file changed, 89 insertions(+), 32 deletions(-)

diff --git a/drivers/i2c/i2c-cdns.c b/drivers/i2c/i2c-cdns.c
index 9a1b520..4a46dbf 100644
--- a/drivers/i2c/i2c-cdns.c
+++ b/drivers/i2c/i2c-cdns.c
@@ -17,6 +17,7 @@
 #include 
 #include 
 #include 
+#include 
 
 DECLARE_GLOBAL_DATA_PTR;
 
@@ -67,6 +68,8 @@ struct cdns_i2c_regs {
 
 #define CDNS_I2C_FIFO_DEPTH16
 #define CDNS_I2C_TRANSFER_SIZE_MAX 255 /* Controller transfer limit */
+#define CDNS_I2C_TRANSFER_SIZE (CDNS_I2C_TRANSFER_SIZE_MAX - 3)
+
 #define CDNS_I2C_BROKEN_HOLD_BIT   BIT(0)
 
 #ifdef DEBUG
@@ -247,15 +250,20 @@ static int cdns_i2c_write_data(struct i2c_cdns_bus 
*i2c_bus, u32 addr, u8 *data,
   u32 len)
 {
u8 *cur_data = data;
-
struct cdns_i2c_regs *regs = i2c_bus->regs;
 
+   /* Set the controller in Master transmit mode and clear FIFO */
setbits_le32(>control, CDNS_I2C_CONTROL_CLR_FIFO);
-
-
clrbits_le32(>control, CDNS_I2C_CONTROL_RW);
 
+   /* Check message size against FIFO depth, and set hold bus bit
+* if it is greater than FIFO depth */
+   if (len > CDNS_I2C_FIFO_DEPTH)
+   setbits_le32(>control, CDNS_I2C_CONTROL_HOLD);
+
+   /* Clear the interrupts in status register */
writel(0xFF, >interrupt_status);
+
writel(addr, >address);
 
while (len--) {
@@ -280,48 +288,98 @@ static int cdns_i2c_write_data(struct i2c_cdns_bus 
*i2c_bus, u32 addr, u8 *data,
return 0;
 }
 
+static inline bool cdns_is_hold_quirk(int hold_quirk, int curr_recv_count)
+{
+   return hold_quirk && (curr_recv_count == CDNS_I2C_FIFO_DEPTH + 1);
+}
+
 static int cdns_i2c_read_data(struct i2c_cdns_bus *i2c_bus, u32 addr, u8 *data,
- u32 len)
+ u32 recv_count)
 {
-   u32 status;
-   u32 i = 0;
u8 *cur_data = data;
-
-   /* TODO: Fix this */
struct cdns_i2c_regs *regs = i2c_bus->regs;
+   int curr_recv_count;
+   int updatetx, hold_quirk;
 
/* Check the hardware can handle the requested bytes */
-   if ((len < 0))
+   if ((recv_count < 0))
return -EINVAL;
 
+   curr_recv_count = recv_count;
+
+   /* Check for the message size against the FIFO depth */
+   if (recv_count > CDNS_I2C_FIFO_DEPTH)
+   setbits_le32(>control, CDNS_I2C_CONTROL_HOLD);
+
setbits_le32(>control, CDNS_I2C_CONTROL_CLR_FIFO |
CDNS_I2C_CONTROL_RW);
 
+   if (recv_count > CDNS_I2C_TRANSFER_SIZE) {
+   curr_recv_count = CDNS_I2C_TRANSFER_SIZE;
+   writel(curr_recv_count, >transfer_size);
+   } else {
+   writel(recv_count, >transfer_size);
+   }
+
/* Start reading data */
writel(addr, >address);
-   writel(len, >transfer_size);
-
-   /* Wait for data */
-   do {
-   status = cdns_i2c_wait(regs, CDNS_I2C_INTERRUPT_COMP |
-   CDNS_I2C_INTERRUPT_DATA);
-   if (!status) {
-   /* Release the bus */
-   clrbits_le32(>control, CDNS_I2C_CONTROL_HOLD);
-   return -ETIMEDOUT;
+
+   updatetx = recv_count > curr_recv_count;
+
+   hold_quirk = (i2c_bus->quirks & CDNS_I2C_BROKEN_HOLD_BIT) && updatetx;
+
+   while (recv_count) {
+   while (readl(>status) & CDNS_I2C_STATUS_RXDV) {
+   if (recv_count < CDNS_I2C_FIFO_DEPTH &&
+   !i2c_bus->hold_flag) {
+   clrbits_le32(>control,
+CDNS_I2C_CONTROL_HOLD);
+   }
+   *(cur_data)++ = readl(>data);
+   recv_count--;
+   curr_recv_count--;
+
+   if (cdns_is_hold_quirk(hold_quirk, curr_recv_count))
+   break;
}
-   debug("Read %d bytes\n",
- len - readl(>transfer_size));
-   for (; i < len - readl(>transfer_size); i++)
-   *(cur_data++) = readl(>data);
-   } while (readl(>transfer_size) != 0);
-   /* All done... release the bus */
-   if (!i2c_bus->hold_flag)
-