All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] i2c: exynos5: rework HSI2C_MASTER_ST_LOSE state handling
       [not found] <CGME20180126121001eucas1p2931738af133ad3e5bf1cd824058ba8d0@eucas1p2.samsung.com>
@ 2018-01-26 12:09 ` Andrzej Hajda
  2018-02-01  7:51   ` Andi Shyti
  2018-02-12  8:04   ` Andrzej Hajda
  0 siblings, 2 replies; 3+ messages in thread
From: Andrzej Hajda @ 2018-01-26 12:09 UTC (permalink / raw)
  To: Wolfram Sang, Andi Shyti
  Cc: Andrzej Hajda, Bartlomiej Zolnierkiewicz, Marek Szyprowski,
	open list:I2C SUBSYSTEM,
	moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES

HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
show that hardware is usually able to recover from this state without
interrupting the transfer. Moreover documentation says that
such state can be caused by slave clock stretching, and should not be
treated as an error during transaction. The only place it indicates
an error is just before starting transaction. In such case bus recovery
procedure should be performed - master should pulse SCL line nine times
and then send STOP condition, it can be repeated until SDA goes high.
The procedure can be performed using manual commands HSI2C_CMD_READ_DATA
and HSI2C_CMD_SEND_STOP.

Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
---
Hi Wolfram,

This patch integrates and invalidates my previous patches [1][2].
I hope inline comments are sufficient.

[1]: i2c: exynos5: implement bus recovery functionality
[2]: i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant

Regards
Andrzej

 drivers/i2c/busses/i2c-exynos5.c | 67 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 61 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
index b02428498f6d..8dcc36835a1a 100644
--- a/drivers/i2c/busses/i2c-exynos5.c
+++ b/drivers/i2c/busses/i2c-exynos5.c
@@ -128,6 +128,10 @@
 #define HSI2C_TIMEOUT_EN			(1u << 31)
 #define HSI2C_TIMEOUT_MASK			0xff
 
+/* I2C_MANUAL_CMD register bits */
+#define HSI2C_CMD_READ_DATA			(1u << 4)
+#define HSI2C_CMD_SEND_STOP			(1u << 2)
+
 /* I2C_TRANS_STATUS register bits */
 #define HSI2C_MASTER_BUSY			(1u << 17)
 #define HSI2C_SLAVE_BUSY			(1u << 16)
@@ -441,12 +445,6 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
 			i2c->state = -ETIMEDOUT;
 			goto stop;
 		}
-
-		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
-		if ((trans_status & HSI2C_MASTER_ST_MASK) == HSI2C_MASTER_ST_LOSE) {
-			i2c->state = -EAGAIN;
-			goto stop;
-		}
 	} else if (int_status & HSI2C_INT_I2C) {
 		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
 		if (trans_status & HSI2C_NO_DEV_ACK) {
@@ -544,6 +542,61 @@ static int exynos5_i2c_wait_bus_idle(struct exynos5_i2c *i2c)
 	return -EBUSY;
 }
 
+static void exynos5_i2c_bus_recover(struct exynos5_i2c *i2c)
+{
+	u32 val;
+
+	dev_dbg(i2c->dev, "%s: start\n", __func__);
+
+	val = readl(i2c->regs + HSI2C_CTL) | HSI2C_RXCHON;
+	writel(val, i2c->regs + HSI2C_CTL);
+	val = readl(i2c->regs + HSI2C_CONF) & ~HSI2C_AUTO_MODE;
+	writel(val, i2c->regs + HSI2C_CONF);
+
+	/*
+	 * Specification says master should send nine clock pulses. It can be
+	 * emulated by sending manual read command (nine pulses for read eight
+	 * bits + one pulse for NACK).
+	 */
+	writel(HSI2C_CMD_READ_DATA, i2c->regs + HSI2C_MANUAL_CMD);
+	exynos5_i2c_wait_bus_idle(i2c);
+	writel(HSI2C_CMD_SEND_STOP, i2c->regs + HSI2C_MANUAL_CMD);
+	exynos5_i2c_wait_bus_idle(i2c);
+
+	val = readl(i2c->regs + HSI2C_CTL) & ~HSI2C_RXCHON;
+	writel(val, i2c->regs + HSI2C_CTL);
+	val = readl(i2c->regs + HSI2C_CONF) | HSI2C_AUTO_MODE;
+	writel(val, i2c->regs + HSI2C_CONF);
+
+	dev_dbg(i2c->dev, "%s: end\n", __func__);
+}
+
+static void exynos5_i2c_bus_check(struct exynos5_i2c *i2c)
+{
+	unsigned long timeout;
+
+	if (i2c->variant->hw != HSI2C_EXYNOS7)
+		return;
+
+	/*
+	 * HSI2C_MASTER_ST_LOSE state in EXYNOS7 variant before transaction
+	 * indicates that bus is stuck (SDA is low). In such case bus recovery
+	 * can be performed.
+	 */
+	timeout = jiffies + msecs_to_jiffies(100);
+	for (;;) {
+		u32 st = readl(i2c->regs + HSI2C_TRANS_STATUS);
+
+		if ((st & HSI2C_MASTER_ST_MASK) != HSI2C_MASTER_ST_LOSE)
+			return;
+
+		if (time_is_before_jiffies(timeout))
+			return;
+
+		exynos5_i2c_bus_recover(i2c);
+	}
+}
+
 /*
  * exynos5_i2c_message_start: Configures the bus and starts the xfer
  * i2c: struct exynos5_i2c pointer for the current bus
@@ -598,6 +651,8 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop)
 	writel(fifo_ctl, i2c->regs + HSI2C_FIFO_CTL);
 	writel(i2c_ctl, i2c->regs + HSI2C_CTL);
 
+	exynos5_i2c_bus_check(i2c);
+
 	/*
 	 * Enable interrupts before starting the transfer so that we don't
 	 * miss any INT_I2C interrupts.
-- 
2.15.1

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] i2c: exynos5: rework HSI2C_MASTER_ST_LOSE state handling
  2018-01-26 12:09 ` [PATCH] i2c: exynos5: rework HSI2C_MASTER_ST_LOSE state handling Andrzej Hajda
@ 2018-02-01  7:51   ` Andi Shyti
  2018-02-12  8:04   ` Andrzej Hajda
  1 sibling, 0 replies; 3+ messages in thread
From: Andi Shyti @ 2018-02-01  7:51 UTC (permalink / raw)
  To: Andrzej Hajda
  Cc: Wolfram Sang, Bartlomiej Zolnierkiewicz, Marek Szyprowski,
	open list:I2C SUBSYSTEM,
	moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES

Hi Andrzej,

On Fri, Jan 26, 2018 at 01:09:50PM +0100, Andrzej Hajda wrote:
> HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
> show that hardware is usually able to recover from this state without
> interrupting the transfer. Moreover documentation says that
> such state can be caused by slave clock stretching, and should not be
> treated as an error during transaction. The only place it indicates
> an error is just before starting transaction. In such case bus recovery
> procedure should be performed - master should pulse SCL line nine times
> and then send STOP condition, it can be repeated until SDA goes high.
> The procedure can be performed using manual commands HSI2C_CMD_READ_DATA
> and HSI2C_CMD_SEND_STOP.
> 
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>

Thank you, looks good to me.

Please add:

Reviewed-by: Andi Shyti <andi.shyti@samsung.com>
Tested-by: Andi Shyti <andi.shyti@samsung.com>

Thanks,
Andi

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] i2c: exynos5: rework HSI2C_MASTER_ST_LOSE state handling
  2018-01-26 12:09 ` [PATCH] i2c: exynos5: rework HSI2C_MASTER_ST_LOSE state handling Andrzej Hajda
  2018-02-01  7:51   ` Andi Shyti
@ 2018-02-12  8:04   ` Andrzej Hajda
  1 sibling, 0 replies; 3+ messages in thread
From: Andrzej Hajda @ 2018-02-12  8:04 UTC (permalink / raw)
  To: Wolfram Sang, Andi Shyti
  Cc: Bartlomiej Zolnierkiewicz, Marek Szyprowski,
	open list:I2C SUBSYSTEM,
	moderated list:ARM/SAMSUNG EXYNOS ARM ARCHITECTURES

On 26.01.2018 13:09, Andrzej Hajda wrote:
> HSI2C_MASTER_ST_LOSE state is not documented properly, extensive tests
> show that hardware is usually able to recover from this state without
> interrupting the transfer. Moreover documentation says that
> such state can be caused by slave clock stretching, and should not be
> treated as an error during transaction. The only place it indicates
> an error is just before starting transaction. In such case bus recovery
> procedure should be performed - master should pulse SCL line nine times
> and then send STOP condition, it can be repeated until SDA goes high.
> The procedure can be performed using manual commands HSI2C_CMD_READ_DATA
> and HSI2C_CMD_SEND_STOP.
>
> Signed-off-by: Andrzej Hajda <a.hajda@samsung.com>
> ---
> Hi Wolfram,
>
> This patch integrates and invalidates my previous patches [1][2].
> I hope inline comments are sufficient.

Gentle ping.

--
Regards
Andrzej

>
> [1]: i2c: exynos5: implement bus recovery functionality
> [2]: i2c: exynos5: do not check TRANS_STATUS in case of Exynos7 variant
>
> Regards
> Andrzej
>
>  drivers/i2c/busses/i2c-exynos5.c | 67 ++++++++++++++++++++++++++++++++++++----
>  1 file changed, 61 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-exynos5.c b/drivers/i2c/busses/i2c-exynos5.c
> index b02428498f6d..8dcc36835a1a 100644
> --- a/drivers/i2c/busses/i2c-exynos5.c
> +++ b/drivers/i2c/busses/i2c-exynos5.c
> @@ -128,6 +128,10 @@
>  #define HSI2C_TIMEOUT_EN			(1u << 31)
>  #define HSI2C_TIMEOUT_MASK			0xff
>  
> +/* I2C_MANUAL_CMD register bits */
> +#define HSI2C_CMD_READ_DATA			(1u << 4)
> +#define HSI2C_CMD_SEND_STOP			(1u << 2)
> +
>  /* I2C_TRANS_STATUS register bits */
>  #define HSI2C_MASTER_BUSY			(1u << 17)
>  #define HSI2C_SLAVE_BUSY			(1u << 16)
> @@ -441,12 +445,6 @@ static irqreturn_t exynos5_i2c_irq(int irqno, void *dev_id)
>  			i2c->state = -ETIMEDOUT;
>  			goto stop;
>  		}
> -
> -		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
> -		if ((trans_status & HSI2C_MASTER_ST_MASK) == HSI2C_MASTER_ST_LOSE) {
> -			i2c->state = -EAGAIN;
> -			goto stop;
> -		}
>  	} else if (int_status & HSI2C_INT_I2C) {
>  		trans_status = readl(i2c->regs + HSI2C_TRANS_STATUS);
>  		if (trans_status & HSI2C_NO_DEV_ACK) {
> @@ -544,6 +542,61 @@ static int exynos5_i2c_wait_bus_idle(struct exynos5_i2c *i2c)
>  	return -EBUSY;
>  }
>  
> +static void exynos5_i2c_bus_recover(struct exynos5_i2c *i2c)
> +{
> +	u32 val;
> +
> +	dev_dbg(i2c->dev, "%s: start\n", __func__);
> +
> +	val = readl(i2c->regs + HSI2C_CTL) | HSI2C_RXCHON;
> +	writel(val, i2c->regs + HSI2C_CTL);
> +	val = readl(i2c->regs + HSI2C_CONF) & ~HSI2C_AUTO_MODE;
> +	writel(val, i2c->regs + HSI2C_CONF);
> +
> +	/*
> +	 * Specification says master should send nine clock pulses. It can be
> +	 * emulated by sending manual read command (nine pulses for read eight
> +	 * bits + one pulse for NACK).
> +	 */
> +	writel(HSI2C_CMD_READ_DATA, i2c->regs + HSI2C_MANUAL_CMD);
> +	exynos5_i2c_wait_bus_idle(i2c);
> +	writel(HSI2C_CMD_SEND_STOP, i2c->regs + HSI2C_MANUAL_CMD);
> +	exynos5_i2c_wait_bus_idle(i2c);
> +
> +	val = readl(i2c->regs + HSI2C_CTL) & ~HSI2C_RXCHON;
> +	writel(val, i2c->regs + HSI2C_CTL);
> +	val = readl(i2c->regs + HSI2C_CONF) | HSI2C_AUTO_MODE;
> +	writel(val, i2c->regs + HSI2C_CONF);
> +
> +	dev_dbg(i2c->dev, "%s: end\n", __func__);
> +}
> +
> +static void exynos5_i2c_bus_check(struct exynos5_i2c *i2c)
> +{
> +	unsigned long timeout;
> +
> +	if (i2c->variant->hw != HSI2C_EXYNOS7)
> +		return;
> +
> +	/*
> +	 * HSI2C_MASTER_ST_LOSE state in EXYNOS7 variant before transaction
> +	 * indicates that bus is stuck (SDA is low). In such case bus recovery
> +	 * can be performed.
> +	 */
> +	timeout = jiffies + msecs_to_jiffies(100);
> +	for (;;) {
> +		u32 st = readl(i2c->regs + HSI2C_TRANS_STATUS);
> +
> +		if ((st & HSI2C_MASTER_ST_MASK) != HSI2C_MASTER_ST_LOSE)
> +			return;
> +
> +		if (time_is_before_jiffies(timeout))
> +			return;
> +
> +		exynos5_i2c_bus_recover(i2c);
> +	}
> +}
> +
>  /*
>   * exynos5_i2c_message_start: Configures the bus and starts the xfer
>   * i2c: struct exynos5_i2c pointer for the current bus
> @@ -598,6 +651,8 @@ static void exynos5_i2c_message_start(struct exynos5_i2c *i2c, int stop)
>  	writel(fifo_ctl, i2c->regs + HSI2C_FIFO_CTL);
>  	writel(i2c_ctl, i2c->regs + HSI2C_CTL);
>  
> +	exynos5_i2c_bus_check(i2c);
> +
>  	/*
>  	 * Enable interrupts before starting the transfer so that we don't
>  	 * miss any INT_I2C interrupts.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2018-02-12  8:04 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <CGME20180126121001eucas1p2931738af133ad3e5bf1cd824058ba8d0@eucas1p2.samsung.com>
2018-01-26 12:09 ` [PATCH] i2c: exynos5: rework HSI2C_MASTER_ST_LOSE state handling Andrzej Hajda
2018-02-01  7:51   ` Andi Shyti
2018-02-12  8:04   ` Andrzej Hajda

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.