On Thu, Jan 10, 2019 at 07:36:46PM -0800, Sowjanya Komatineni wrote:
> 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.
>
> Signed-off-by: Sowjanya Komatineni <skomatineni@xxxxxxxxxx>
> ---
> drivers/i2c/busses/i2c-tegra.c | 66 ++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 66 insertions(+)
Can you extend the commit message with some of the information that you
had provided as a reply to my question on v1? There was some really good
clarifying information in that reply, which I think is really good
commit message material.
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index e417ebf7628c..11bc43ed08e9 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;
> };
>
> /**
> @@ -640,6 +653,9 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
> goto err;
> }
>
> + if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
> + goto err;
Maybe also add a clarifying comment here as to why we're done processing
interrupts here.
> +
> if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
> if (i2c_dev->msg_buf_remaining)
> tegra_i2c_empty_rx_fifo(i2c_dev);
> @@ -668,6 +684,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 +696,42 @@ 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 Arb lost\n");
s/Arb/arbitration/
> + 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 +813,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 */
I think this should either say "arbitration loss" or "Arbitration Loss"
but not mixed case. Same for "Single-Master Mode". I'd personally make
all of them lower case. But it's fine with me as long as it is
consistent.
Thierry
> + 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 +908,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 +922,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 +936,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 +950,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 +964,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 +978,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
>
Attachment:
signature.asc
Description: PGP signature
