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 > --- > 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 >