[PATCH V3] i2c: tegra: Add Bus Clear Master Support

[PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Sowjanya Komatineni]

Bus clear feature of tegra i2c controller helps to recover from
bus hang when i2c master loses the bus arbitration due to the
slave device holding SDA LOW continuously for some unknown reasons.

Per I2C specification, the device that held the bus LOW should
release it within 9 clock pulses.

During bus clear operation, Tegra I2C controller sends 9 clock
pulses and terminates the transaction with STOP condition.
Upon successful bus clear operation, bus goes to idle state and
driver retries the transaction.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V3]: Updated comments and commit message to be clear on the change
 [V2]: Same as V1 rebased to 5.0-rc1

 drivers/i2c/busses/i2c-tegra.c | 70 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 70 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e417ebf7628c..b1b920b4a203 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -54,6 +54,7 @@
 #define I2C_FIFO_STATUS_RX_SHIFT		0
 #define I2C_INT_MASK				0x064
 #define I2C_INT_STATUS				0x068
+#define I2C_INT_BUS_CLR_DONE			BIT(11)
 #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
 #define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
 #define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
@@ -96,6 +97,15 @@
 #define I2C_HEADER_MASTER_ADDR_SHIFT		12
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
+#define I2C_BUS_CLEAR_CNFG			0x084
+#define I2C_BC_SCLK_THRESHOLD			9
+#define I2C_BC_SCLK_THRESHOLD_SHIFT		16
+#define I2C_BC_STOP_COND			BIT(2)
+#define I2C_BC_TERMINATE			BIT(1)
+#define I2C_BC_ENABLE				BIT(0)
+#define I2C_BUS_CLEAR_STATUS			0x088
+#define I2C_BC_STATUS				BIT(0)
+
 #define I2C_CONFIG_LOAD				0x08C
 #define I2C_MSTR_CONFIG_LOAD			BIT(0)
 #define I2C_SLV_CONFIG_LOAD			BIT(1)
@@ -155,6 +165,8 @@ enum msg_end_type {
  * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
  *		provides additional features and allows for longer messages to
  *		be transferred in one go.
+ * @supports_bus_clear: Bus Clear support to recover from bus hang during
+ *		SDA stuck low from device for some unknown reasons.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -167,6 +179,7 @@ struct tegra_i2c_hw_feature {
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
+	bool supports_bus_clear;
 };
 
 /**
@@ -639,6 +652,12 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
 		goto err;
 	}
+	/*
+	 * I2C transfer is terminated during the bus clear so skip
+	 * processing the other interrupts.
+	 */
+	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
+		goto err;
 
 	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
 		if (i2c_dev->msg_buf_remaining)
@@ -668,6 +687,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
 		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
 		I2C_INT_RX_FIFO_DATA_REQ);
+	if (i2c_dev->hw->supports_bus_clear)
+		tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
@@ -678,6 +699,43 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
+{
+	int err;
+	unsigned long time_left;
+	u32 reg;
+
+	if (i2c_dev->hw->supports_bus_clear) {
+		reinit_completion(&i2c_dev->msg_complete);
+		reg = (I2C_BC_SCLK_THRESHOLD << I2C_BC_SCLK_THRESHOLD_SHIFT) |
+		      I2C_BC_STOP_COND | I2C_BC_TERMINATE;
+		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+		if (i2c_dev->hw->has_config_load_reg) {
+			err = tegra_i2c_wait_for_config_load(i2c_dev);
+			if (err)
+				return err;
+		}
+		reg |= I2C_BC_ENABLE;
+		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
+
+		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
+							TEGRA_I2C_TIMEOUT);
+		if (time_left == 0) {
+			dev_err(i2c_dev->dev, "timed out for bus clear\n");
+			return -ETIMEDOUT;
+		}
+		reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
+		if (!(reg & I2C_BC_STATUS)) {
+			dev_err(i2c_dev->dev,
+				"Un-recovered arbitration lost\n");
+			return -EIO;
+		}
+	}
+
+	return -EAGAIN;
+}
+
 static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	struct i2c_msg *msg, enum msg_end_type end_state)
 {
@@ -759,6 +817,12 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		return 0;
 
 	tegra_i2c_init(i2c_dev);
+	/* start recovery upon arbitration loss in single master mode */
+	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
+		if (!i2c_dev->is_multimaster_mode)
+			return tegra_i2c_issue_bus_clear(i2c_dev);
+		return -EAGAIN;
+	}
 	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
@@ -848,6 +912,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -861,6 +926,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -874,6 +940,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -887,6 +954,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -900,6 +968,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -913,6 +982,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
+	.supports_bus_clear = true,
 };
 
 /* Match table for of_platform binding */
-- 
2.7.4

Re: [PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Dmitry Osipenko]

22.01.2019 23:02, Sowjanya Komatineni пишет:
> Bus clear feature of tegra i2c controller helps to recover from
> bus hang when i2c master loses the bus arbitration due to the
> slave device holding SDA LOW continuously for some unknown reasons.
> 
> Per I2C specification, the device that held the bus LOW should
> release it within 9 clock pulses.
> 
> During bus clear operation, Tegra I2C controller sends 9 clock
> pulses and terminates the transaction with STOP condition.
> Upon successful bus clear operation, bus goes to idle state and
> driver retries the transaction.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V3]: Updated comments and commit message to be clear on the change
>  [V2]: Same as V1 rebased to 5.0-rc1
> 
>  drivers/i2c/busses/i2c-tegra.c | 70 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 70 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index e417ebf7628c..b1b920b4a203 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -54,6 +54,7 @@
>  #define I2C_FIFO_STATUS_RX_SHIFT		0
>  #define I2C_INT_MASK				0x064
>  #define I2C_INT_STATUS				0x068
> +#define I2C_INT_BUS_CLR_DONE			BIT(11)
>  #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
>  #define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
>  #define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
> @@ -96,6 +97,15 @@
>  #define I2C_HEADER_MASTER_ADDR_SHIFT		12
>  #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
>  
> +#define I2C_BUS_CLEAR_CNFG			0x084
> +#define I2C_BC_SCLK_THRESHOLD			9
> +#define I2C_BC_SCLK_THRESHOLD_SHIFT		16
> +#define I2C_BC_STOP_COND			BIT(2)
> +#define I2C_BC_TERMINATE			BIT(1)
> +#define I2C_BC_ENABLE				BIT(0)
> +#define I2C_BUS_CLEAR_STATUS			0x088
> +#define I2C_BC_STATUS				BIT(0)
> +
>  #define I2C_CONFIG_LOAD				0x08C
>  #define I2C_MSTR_CONFIG_LOAD			BIT(0)
>  #define I2C_SLV_CONFIG_LOAD			BIT(1)
> @@ -155,6 +165,8 @@ enum msg_end_type {
>   * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
>   *		provides additional features and allows for longer messages to
>   *		be transferred in one go.
> + * @supports_bus_clear: Bus Clear support to recover from bus hang during
> + *		SDA stuck low from device for some unknown reasons.
>   */
>  struct tegra_i2c_hw_feature {
>  	bool has_continue_xfer_support;
> @@ -167,6 +179,7 @@ struct tegra_i2c_hw_feature {
>  	bool has_multi_master_mode;
>  	bool has_slcg_override_reg;
>  	bool has_mst_fifo;
> +	bool supports_bus_clear;
>  };
>  
>  /**
> @@ -639,6 +652,12 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
>  		goto err;
>  	}
> +	/*
> +	 * I2C transfer is terminated during the bus clear so skip
> +	 * processing the other interrupts.
> +	 */
> +	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
> +		goto err;
>  
>  	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
>  		if (i2c_dev->msg_buf_remaining)
> @@ -668,6 +687,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
>  		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
>  		I2C_INT_RX_FIFO_DATA_REQ);
> +	if (i2c_dev->hw->supports_bus_clear)
> +		tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>  	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
>  	if (i2c_dev->is_dvc)
>  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
> @@ -678,6 +699,43 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	return IRQ_HANDLED;
>  }
>  
> +static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
> +{
> +	int err;
> +	unsigned long time_left;
> +	u32 reg;
> +
> +	if (i2c_dev->hw->supports_bus_clear) {
> +		reinit_completion(&i2c_dev->msg_complete);
> +		reg = (I2C_BC_SCLK_THRESHOLD << I2C_BC_SCLK_THRESHOLD_SHIFT) |
> +		      I2C_BC_STOP_COND | I2C_BC_TERMINATE;
> +		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> +		if (i2c_dev->hw->has_config_load_reg) {
> +			err = tegra_i2c_wait_for_config_load(i2c_dev);
> +			if (err)
> +				return err;
> +		}
> +		reg |= I2C_BC_ENABLE;
> +		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> +		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
> +
> +		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> +							TEGRA_I2C_TIMEOUT);
> +		if (time_left == 0) {
> +			dev_err(i2c_dev->dev, "timed out for bus clear\n");
> +			return -ETIMEDOUT;
> +		}
> +		reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
> +		if (!(reg & I2C_BC_STATUS)) {
> +			dev_err(i2c_dev->dev,
> +				"Un-recovered arbitration lost\n");
> +			return -EIO;
> +		}
> +	}
> +
> +	return -EAGAIN;
> +}
> +
>  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	struct i2c_msg *msg, enum msg_end_type end_state)
>  {
> @@ -759,6 +817,12 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  		return 0;
>  
>  	tegra_i2c_init(i2c_dev);
> +	/* start recovery upon arbitration loss in single master mode */
> +	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
> +		if (!i2c_dev->is_multimaster_mode)
> +			return tegra_i2c_issue_bus_clear(i2c_dev);
> +		return -EAGAIN;

This changes the returned errno from -EIO to -EAGAIN for the supports_bus_clear=false case, is it okay and intentional?

[snip]

RE: [PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Sowjanya Komatineni]

>> Bus clear feature of tegra i2c controller helps to recover from bus 
>> hang when i2c master loses the bus arbitration due to the slave device 
>> holding SDA LOW continuously for some unknown reasons.
>> 
>> Per I2C specification, the device that held the bus LOW should release 
>> it within 9 clock pulses.
>> 
>> During bus clear operation, Tegra I2C controller sends 9 clock pulses 
>> and terminates the transaction with STOP condition.
>> Upon successful bus clear operation, bus goes to idle state and driver 
>> retries the transaction.
>> 
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  [V3]: Updated comments and commit message to be clear on the change
>>  [V2]: Same as V1 rebased to 5.0-rc1
>> 
>>  drivers/i2c/busses/i2c-tegra.c | 70 
>> ++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 70 insertions(+)
>> 
>> diff --git a/drivers/i2c/busses/i2c-tegra.c 
>> b/drivers/i2c/busses/i2c-tegra.c index e417ebf7628c..b1b920b4a203 
>> 100644
>> --- a/drivers/i2c/busses/i2c-tegra.c
>> +++ b/drivers/i2c/busses/i2c-tegra.c
>> @@ -54,6 +54,7 @@
>>  #define I2C_FIFO_STATUS_RX_SHIFT		0
>>  #define I2C_INT_MASK				0x064
>>  #define I2C_INT_STATUS				0x068
>> +#define I2C_INT_BUS_CLR_DONE			BIT(11)
>>  #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
>>  #define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
>>  #define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
>> @@ -96,6 +97,15 @@
>>  #define I2C_HEADER_MASTER_ADDR_SHIFT		12
>>  #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
>>  
>> +#define I2C_BUS_CLEAR_CNFG			0x084
>> +#define I2C_BC_SCLK_THRESHOLD			9
>> +#define I2C_BC_SCLK_THRESHOLD_SHIFT		16
>> +#define I2C_BC_STOP_COND			BIT(2)
>> +#define I2C_BC_TERMINATE			BIT(1)
>> +#define I2C_BC_ENABLE				BIT(0)
>> +#define I2C_BUS_CLEAR_STATUS			0x088
>> +#define I2C_BC_STATUS				BIT(0)
>> +
>>  #define I2C_CONFIG_LOAD				0x08C
>>  #define I2C_MSTR_CONFIG_LOAD			BIT(0)
>>  #define I2C_SLV_CONFIG_LOAD			BIT(1)
>> @@ -155,6 +165,8 @@ enum msg_end_type {
>>   * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
>>   *		provides additional features and allows for longer messages to
>>   *		be transferred in one go.
>> + * @supports_bus_clear: Bus Clear support to recover from bus hang during
>> + *		SDA stuck low from device for some unknown reasons.
>>   */
>>  struct tegra_i2c_hw_feature {
>>  	bool has_continue_xfer_support;
>> @@ -167,6 +179,7 @@ struct tegra_i2c_hw_feature {
>>  	bool has_multi_master_mode;
>>  	bool has_slcg_override_reg;
>>  	bool has_mst_fifo;
>> +	bool supports_bus_clear;
>>  };
>>  
>>  /**
>> @@ -639,6 +652,12 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>>  			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
>>  		goto err;
>>  	}
>> +	/*
>> +	 * I2C transfer is terminated during the bus clear so skip
>> +	 * processing the other interrupts.
>> +	 */
>> +	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
>> +		goto err;
>>  
>>  	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
>>  		if (i2c_dev->msg_buf_remaining)
>> @@ -668,6 +687,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>>  	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
>>  		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
>>  		I2C_INT_RX_FIFO_DATA_REQ);
>> +	if (i2c_dev->hw->supports_bus_clear)
>> +		tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>>  	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
>>  	if (i2c_dev->is_dvc)
>>  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS); @@ 
>> -678,6 +699,43 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>>  	return IRQ_HANDLED;
>>  }
>>  
>> +static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev) {
>> +	int err;
>> +	unsigned long time_left;
>> +	u32 reg;
>> +
>> +	if (i2c_dev->hw->supports_bus_clear) {
>> +		reinit_completion(&i2c_dev->msg_complete);
>> +		reg = (I2C_BC_SCLK_THRESHOLD << I2C_BC_SCLK_THRESHOLD_SHIFT) |
>> +		      I2C_BC_STOP_COND | I2C_BC_TERMINATE;
>> +		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
>> +		if (i2c_dev->hw->has_config_load_reg) {
>> +			err = tegra_i2c_wait_for_config_load(i2c_dev);
>> +			if (err)
>> +				return err;
>> +		}
>> +		reg |= I2C_BC_ENABLE;
>> +		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
>> +		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>> +
>> +		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
>> +							TEGRA_I2C_TIMEOUT);
>> +		if (time_left == 0) {
>> +			dev_err(i2c_dev->dev, "timed out for bus clear\n");
>> +			return -ETIMEDOUT;
>> +		}
>> +		reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
>> +		if (!(reg & I2C_BC_STATUS)) {
>> +			dev_err(i2c_dev->dev,
>> +				"Un-recovered arbitration lost\n");
>> +			return -EIO;
>> +		}
>> +	}
>> +
>> +	return -EAGAIN;
>> +}
>> +
>>  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>>  	struct i2c_msg *msg, enum msg_end_type end_state)  { @@ -759,6 
>> +817,12 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>>  		return 0;
>>  
>>  	tegra_i2c_init(i2c_dev);
>> +	/* start recovery upon arbitration loss in single master mode */
>> +	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
>> +		if (!i2c_dev->is_multimaster_mode)
>> +			return tegra_i2c_issue_bus_clear(i2c_dev);
>> +		return -EAGAIN;
>
>This changes the returned errno from -EIO to -EAGAIN for the supports_bus_clear=false case, is it okay and intentional?
>

Yes EAGAIN is intentional to allow for transfer retry.
During single master mode, ARBITRATION LOST notification happens when 
1. I2C Master sees the bus is occupied by some other device when a transfer is initiated 
2. I2C Master lost the bus during arbitration incase if slave device pulls SDA line low continuously for some unknown reason
If arbitration lost is due to cause 1, retry helps to continue with transfer once bus is released by the slave and it just added delay in communication due to bus release delay by slave.
In case of 2nd cause, retry never succeeds in cases where bus clear is not supported.

Thanks
Sowjanya

Re: [PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Dmitry Osipenko]

В Tue, 22 Jan 2019 22:13:53 +0000
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> >> Bus clear feature of tegra i2c controller helps to recover from
> >> bus hang when i2c master loses the bus arbitration due to the
> >> slave device holding SDA LOW continuously for some unknown reasons.
> >> 
> >> Per I2C specification, the device that held the bus LOW should
> >> release it within 9 clock pulses.
> >> 
> >> During bus clear operation, Tegra I2C controller sends 9 clock
> >> pulses and terminates the transaction with STOP condition.
> >> Upon successful bus clear operation, bus goes to idle state and
> >> driver retries the transaction.
> >> 
> >> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> >> ---
> >>  [V3]: Updated comments and commit message to be clear on the
> >> change [V2]: Same as V1 rebased to 5.0-rc1
> >> 
> >>  drivers/i2c/busses/i2c-tegra.c | 70 
> >> ++++++++++++++++++++++++++++++++++++++++++
> >>  1 file changed, 70 insertions(+)
> >> 
> >> diff --git a/drivers/i2c/busses/i2c-tegra.c 
> >> b/drivers/i2c/busses/i2c-tegra.c index e417ebf7628c..b1b920b4a203 
> >> 100644
> >> --- a/drivers/i2c/busses/i2c-tegra.c
> >> +++ b/drivers/i2c/busses/i2c-tegra.c
> >> @@ -54,6 +54,7 @@
> >>  #define I2C_FIFO_STATUS_RX_SHIFT		0
> >>  #define I2C_INT_MASK				0x064
> >>  #define I2C_INT_STATUS				0x068
> >> +#define I2C_INT_BUS_CLR_DONE			BIT(11)
> >>  #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
> >>  #define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
> >>  #define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
> >> @@ -96,6 +97,15 @@
> >>  #define I2C_HEADER_MASTER_ADDR_SHIFT		12
> >>  #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
> >>  
> >> +#define I2C_BUS_CLEAR_CNFG			0x084
> >> +#define I2C_BC_SCLK_THRESHOLD			9
> >> +#define I2C_BC_SCLK_THRESHOLD_SHIFT		16
> >> +#define I2C_BC_STOP_COND			BIT(2)
> >> +#define I2C_BC_TERMINATE			BIT(1)
> >> +#define I2C_BC_ENABLE				BIT(0)
> >> +#define I2C_BUS_CLEAR_STATUS			0x088
> >> +#define I2C_BC_STATUS				BIT(0)
> >> +
> >>  #define I2C_CONFIG_LOAD				0x08C
> >>  #define I2C_MSTR_CONFIG_LOAD			BIT(0)
> >>  #define I2C_SLV_CONFIG_LOAD			BIT(1)
> >> @@ -155,6 +165,8 @@ enum msg_end_type {
> >>   * @has_mst_fifo: The I2C controller contains the new MST FIFO
> >> interface that
> >>   *		provides additional features and allows for
> >> longer messages to
> >>   *		be transferred in one go.
> >> + * @supports_bus_clear: Bus Clear support to recover from bus
> >> hang during
> >> + *		SDA stuck low from device for some unknown
> >> reasons. */
> >>  struct tegra_i2c_hw_feature {
> >>  	bool has_continue_xfer_support;
> >> @@ -167,6 +179,7 @@ struct tegra_i2c_hw_feature {
> >>  	bool has_multi_master_mode;
> >>  	bool has_slcg_override_reg;
> >>  	bool has_mst_fifo;
> >> +	bool supports_bus_clear;
> >>  };
> >>  
> >>  /**
> >> @@ -639,6 +652,12 @@ static irqreturn_t tegra_i2c_isr(int irq,
> >> void *dev_id) i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
> >>  		goto err;
> >>  	}
> >> +	/*
> >> +	 * I2C transfer is terminated during the bus clear so skip
> >> +	 * processing the other interrupts.
> >> +	 */
> >> +	if (i2c_dev->hw->supports_bus_clear && (status &
> >> I2C_INT_BUS_CLR_DONE))
> >> +		goto err;
> >>  
> >>  	if (i2c_dev->msg_read && (status &
> >> I2C_INT_RX_FIFO_DATA_REQ)) { if (i2c_dev->msg_buf_remaining)
> >> @@ -668,6 +687,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void
> >> *dev_id) tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK |
> >> I2C_INT_ARBITRATION_LOST | I2C_INT_PACKET_XFER_COMPLETE |
> >> I2C_INT_TX_FIFO_DATA_REQ | I2C_INT_RX_FIFO_DATA_REQ);
> >> +	if (i2c_dev->hw->supports_bus_clear)
> >> +		tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
> >>  	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
> >>  	if (i2c_dev->is_dvc)
> >>  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR,
> >> DVC_STATUS); @@ -678,6 +699,43 @@ static irqreturn_t
> >> tegra_i2c_isr(int irq, void *dev_id) return IRQ_HANDLED;
> >>  }
> >>  
> >> +static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev
> >> *i2c_dev) {
> >> +	int err;
> >> +	unsigned long time_left;
> >> +	u32 reg;
> >> +
> >> +	if (i2c_dev->hw->supports_bus_clear) {
> >> +		reinit_completion(&i2c_dev->msg_complete);
> >> +		reg = (I2C_BC_SCLK_THRESHOLD <<
> >> I2C_BC_SCLK_THRESHOLD_SHIFT) |
> >> +		      I2C_BC_STOP_COND | I2C_BC_TERMINATE;
> >> +		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> >> +		if (i2c_dev->hw->has_config_load_reg) {
> >> +			err =
> >> tegra_i2c_wait_for_config_load(i2c_dev);
> >> +			if (err)
> >> +				return err;
> >> +		}
> >> +		reg |= I2C_BC_ENABLE;
> >> +		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
> >> +		tegra_i2c_unmask_irq(i2c_dev,
> >> I2C_INT_BUS_CLR_DONE); +
> >> +		time_left =
> >> wait_for_completion_timeout(&i2c_dev->msg_complete,
> >> +
> >> TEGRA_I2C_TIMEOUT);
> >> +		if (time_left == 0) {
> >> +			dev_err(i2c_dev->dev, "timed out for bus
> >> clear\n");
> >> +			return -ETIMEDOUT;
> >> +		}
> >> +		reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
> >> +		if (!(reg & I2C_BC_STATUS)) {
> >> +			dev_err(i2c_dev->dev,
> >> +				"Un-recovered arbitration
> >> lost\n");
> >> +			return -EIO;
> >> +		}
> >> +	}
> >> +
> >> +	return -EAGAIN;
> >> +}
> >> +
> >>  static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >>  	struct i2c_msg *msg, enum msg_end_type end_state)  { @@
> >> -759,6 +817,12 @@ static int tegra_i2c_xfer_msg(struct
> >> tegra_i2c_dev *i2c_dev, return 0;
> >>  
> >>  	tegra_i2c_init(i2c_dev);
> >> +	/* start recovery upon arbitration loss in single master
> >> mode */
> >> +	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
> >> +		if (!i2c_dev->is_multimaster_mode)
> >> +			return tegra_i2c_issue_bus_clear(i2c_dev);
> >> +		return -EAGAIN;  
> >
> >This changes the returned errno from -EIO to -EAGAIN for the
> >supports_bus_clear=false case, is it okay and intentional?
> >  
> 
> Yes EAGAIN is intentional to allow for transfer retry.
> During single master mode, ARBITRATION LOST notification happens when 
> 1. I2C Master sees the bus is occupied by some other device when a
> transfer is initiated 2. I2C Master lost the bus during arbitration
> incase if slave device pulls SDA line low continuously for some
> unknown reason If arbitration lost is due to cause 1, retry helps to
> continue with transfer once bus is released by the slave and it just
> added delay in communication due to bus release delay by slave. In
> case of 2nd cause, retry never succeeds in cases where bus clear is
> not supported.

It's unclear whether the "never succeeds retry" may fail with the
EAGAIN, causing an endless retry-loop. Could you please clarify this
moment?

RE: [PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Sowjanya Komatineni]

>> >> +	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
>> >> +		if (!i2c_dev->is_multimaster_mode)
>> >> +			return tegra_i2c_issue_bus_clear(i2c_dev);
>> >> +		return -EAGAIN;
>> >
>> >This changes the returned errno from -EIO to -EAGAIN for the 
>> >supports_bus_clear=false case, is it okay and intentional?
>> >  
>> 
>> Yes EAGAIN is intentional to allow for transfer retry.
>> During single master mode, ARBITRATION LOST notification happens when 
>> 1. I2C Master sees the bus is occupied by some other device when a 
>> transfer is initiated 2. I2C Master lost the bus during arbitration 
>> incase if slave device pulls SDA line low continuously for some 
>> unknown reason If arbitration lost is due to cause 1, retry helps to 
>> continue with transfer once bus is released by the slave and it just 
>> added delay in communication due to bus release delay by slave. In 
>> case of 2nd cause, retry never succeeds in cases where bus clear is 
>> not supported.
>
>It's unclear whether the "never succeeds retry" may fail with the EAGAIN, causing an endless retry-loop. Could you please clarify this moment?

during master transmit mode, on arbitration lost and if master doesn’t support bus clear to recover then transfer will return EAGAIN.
I2c core base driver performs retries if return code from i2c_transfer is EAGAIN up to specified retries in i2c adapter and returns the ret code from the last retry.
Retry is not endless as i2c core base performs retry only up to specified adapter retries.
Following return code from documentation
https://www.kernel.org/doc/Documentation/i2c/fault-codes

Re: [PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Dmitry Osipenko]

В Tue, 22 Jan 2019 23:26:08 +0000
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> >> >> +	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
> >> >> +		if (!i2c_dev->is_multimaster_mode)
> >> >> +			return
> >> >> tegra_i2c_issue_bus_clear(i2c_dev);
> >> >> +		return -EAGAIN;  
> >> >
> >> >This changes the returned errno from -EIO to -EAGAIN for the 
> >> >supports_bus_clear=false case, is it okay and intentional?
> >> >    
> >> 
> >> Yes EAGAIN is intentional to allow for transfer retry.
> >> During single master mode, ARBITRATION LOST notification happens
> >> when 1. I2C Master sees the bus is occupied by some other device
> >> when a transfer is initiated 2. I2C Master lost the bus during
> >> arbitration incase if slave device pulls SDA line low continuously
> >> for some unknown reason If arbitration lost is due to cause 1,
> >> retry helps to continue with transfer once bus is released by the
> >> slave and it just added delay in communication due to bus release
> >> delay by slave. In case of 2nd cause, retry never succeeds in
> >> cases where bus clear is not supported.  
> >
> >It's unclear whether the "never succeeds retry" may fail with the
> >EAGAIN, causing an endless retry-loop. Could you please clarify this
> >moment?  
> 
> during master transmit mode, on arbitration lost and if master
> doesn’t support bus clear to recover then transfer will return
> EAGAIN. I2c core base driver performs retries if return code from
> i2c_transfer is EAGAIN up to specified retries in i2c adapter and
> returns the ret code from the last retry. Retry is not endless as i2c
> core base performs retry only up to specified adapter retries.
> Following return code from documentation
> https://www.kernel.org/doc/Documentation/i2c/fault-codes
> 
> 

Good, thanks for the clarification.

Re: [PATCH V3] i2c: tegra: Add Bus Clear Master Support

[Dmitry Osipenko]

23.01.2019 2:26, Sowjanya Komatineni пишет:
> 
> 
>>>>> +	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
>>>>> +		if (!i2c_dev->is_multimaster_mode)
>>>>> +			return tegra_i2c_issue_bus_clear(i2c_dev);
>>>>> +		return -EAGAIN;
>>>>
>>>> This changes the returned errno from -EIO to -EAGAIN for the 
>>>> supports_bus_clear=false case, is it okay and intentional?
>>>>  
>>>
>>> Yes EAGAIN is intentional to allow for transfer retry.
>>> During single master mode, ARBITRATION LOST notification happens when 
>>> 1. I2C Master sees the bus is occupied by some other device when a 
>>> transfer is initiated 2. I2C Master lost the bus during arbitration 
>>> incase if slave device pulls SDA line low continuously for some 
>>> unknown reason If arbitration lost is due to cause 1, retry helps to 
>>> continue with transfer once bus is released by the slave and it just 
>>> added delay in communication due to bus release delay by slave. In 
>>> case of 2nd cause, retry never succeeds in cases where bus clear is 
>>> not supported.
>>
>> It's unclear whether the "never succeeds retry" may fail with the EAGAIN, causing an endless retry-loop. Could you please clarify this moment?
> 
> during master transmit mode, on arbitration lost and if master doesn’t support bus clear to recover then transfer will return EAGAIN.
> I2c core base driver performs retries if return code from i2c_transfer is EAGAIN up to specified retries in i2c adapter and returns the ret code from the last retry.
> Retry is not endless as i2c core base performs retry only up to specified adapter retries.
> Following return code from documentation
> https://www.kernel.org/doc/Documentation/i2c/fault-codes
> 
> 

Could you please point at the code that sets the number of retries for the Tegra's I2C? Looks it is always 0 and hence EAGAIN won't do anything useful.