[PATCH v2] Add i2c recovery handling for bcm-iproc based devices.

[PATCH v2] Add i2c recovery handling for bcm-iproc based devices.

[Richard Laing]

It is possible for the i2c bus to become locked up preventing
communication with devices on the bus. This can occur when
another i2c device fails to be reset correctly. In this case
the SDA line will be held low preventing further communication
on the bus.

This situation can be corrected by sending a series of clock
pulses to allow the blocking device to complete the transaction
and release the bus.

Add the hooks required to allow the existing i2c recovery code
to be used to clear the lock up.

Before the recovery can be performed the device needs to be configured in
the bit-bang mode allow the clock line to be controlled directly by the
i2c_generic_recovery() in i2c-core.c. Access routines are provided to get
and set the clock line state and on completion the device is returned to
normal operations and initialized.

Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>
---
changes in v2:
- fix typos/spelling
- change M_BB_CTRL_DATA_SHIFT to M_BB_CTRL_DATA_IN_SHIFT
- change from int to bool for bcm_iproc_i2c_set_scl was NOT done, the type for
  the function pointer does not match and compilation therefore fails.
- updated calls to i2c_recover_bus

 drivers/i2c/busses/i2c-bcm-iproc.c | 111 +++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 4c8c3bc4669c..78393c3b5e83 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -23,6 +23,7 @@
 #define CFG_OFFSET                   0x00
 #define CFG_RESET_SHIFT              31
 #define CFG_EN_SHIFT                 30
+#define CFG_BIT_BANG_SHIFT           29
 #define CFG_M_RETRY_CNT_SHIFT        16
 #define CFG_M_RETRY_CNT_MASK         0x0f
 
@@ -78,6 +79,11 @@
 #define M_RX_DATA_SHIFT              0
 #define M_RX_DATA_MASK               0xff
 
+#define M_BB_CTRL_OFFSET             0x14
+#define M_BB_CTRL_CLK_IN_SHIFT       31
+#define M_BB_CTRL_CLK_SHIFT          30
+#define M_BB_CTRL_DATA_IN_SHIFT      29
+
 #define I2C_TIMEOUT_MSEC             50000
 #define M_TX_RX_FIFO_SIZE            64
 
@@ -208,6 +214,108 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
 	writel(val, iproc_i2c->base + CFG_OFFSET);
 }
 
+/*
+ * Switch to bit bang mode to prepare for i2c generic recovery.
+ */
+static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	dev_dbg(iproc_i2c->device, "Prepare recovery\n");
+
+	/* Disable interrupts */
+	writel(0, iproc_i2c->base + IE_OFFSET);
+	readl(iproc_i2c->base + IE_OFFSET);
+	synchronize_irq(iproc_i2c->irq);
+
+	/* Put the i2c controller into reset. */
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp |= BIT(CFG_RESET_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+	udelay(100);
+
+	/* Switch to bit-bang mode */
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp |= BIT(CFG_BIT_BANG_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+}
+
+/*
+ * Return to normal i2c operation following recovery.
+ */
+static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	/* Switch to normal mode */
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp &= ~BIT(CFG_BIT_BANG_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+	udelay(100);
+
+	bcm_iproc_i2c_init(iproc_i2c);
+	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
+
+	dev_dbg(iproc_i2c->device, "Recovery complete\n");
+}
+
+/*
+ * Return the SCL state, we must be configured in bit bang mode for this
+ * to work.
+ */
+static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+
+	return tmp & BIT(M_BB_CTRL_CLK_IN_SHIFT);
+}
+
+/*
+ * Set the SCL state, we must be configured in bit bang mode for this
+ * to work.
+ */
+static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+	if (val)
+		tmp |= BIT(M_BB_CTRL_CLK_SHIFT);
+	else
+		tmp &= ~BIT(M_BB_CTRL_CLK_SHIFT);
+
+	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
+}
+
+/*
+ * Return the SDA state, we must be configured in bit bang mode for this
+ * to work.
+ */
+static int bcm_iproc_i2c_get_sda(struct i2c_adapter *adap)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 tmp;
+
+	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
+
+	return tmp & BIT(M_BB_CTRL_DATA_IN_SHIFT);
+}
+
+static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {
+	.recover_bus = i2c_generic_scl_recovery,
+	.prepare_recovery = bcm_iproc_i2c_prepare_recovery,
+	.unprepare_recovery = bcm_iproc_i2c_unprepare_recovery,
+	.set_scl = bcm_iproc_i2c_set_scl,
+	.get_scl = bcm_iproc_i2c_get_scl,
+	.get_sda = bcm_iproc_i2c_get_sda,
+};
+
 static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 				      struct i2c_msg *msg)
 {
@@ -341,6 +449,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
 		      (1 << M_FIFO_TX_FLUSH_SHIFT);
 		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		i2c_recover_bus(&iproc_i2c->adapter);
 		return -ETIMEDOUT;
 	}
 
@@ -350,6 +459,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
 		      (1 << M_FIFO_TX_FLUSH_SHIFT);
 		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		i2c_recover_bus(&iproc_i2c->adapter);
 		return ret;
 	}
 
@@ -487,6 +597,7 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 	adap->quirks = &bcm_iproc_i2c_quirks;
 	adap->dev.parent = &pdev->dev;
 	adap->dev.of_node = pdev->dev.of_node;
+	adap->bus_recovery_info = &bcm_iproc_recovery_info;
 
 	return i2c_add_adapter(adap);
 }
-- 
2.21.0

Re: [PATCH v2] Add i2c recovery handling for bcm-iproc based devices.

[Ray Jui]

On 3/25/2019 1:59 PM, Richard Laing wrote:
> It is possible for the i2c bus to become locked up preventing
> communication with devices on the bus. This can occur when
> another i2c device fails to be reset correctly. In this case
> the SDA line will be held low preventing further communication
> on the bus.
> 
> This situation can be corrected by sending a series of clock
> pulses to allow the blocking device to complete the transaction
> and release the bus.
> 
> Add the hooks required to allow the existing i2c recovery code
> to be used to clear the lock up.
> 
> Before the recovery can be performed the device needs to be configured in
> the bit-bang mode allow the clock line to be controlled directly by the
> i2c_generic_recovery() in i2c-core.c. Access routines are provided to get
> and set the clock line state and on completion the device is returned to
> normal operations and initialized.
> 
> Signed-off-by: Richard Laing <richard.laing@alliedtelesis.co.nz>
> ---
> changes in v2:
> - fix typos/spelling
> - change M_BB_CTRL_DATA_SHIFT to M_BB_CTRL_DATA_IN_SHIFT
> - change from int to bool for bcm_iproc_i2c_set_scl was NOT done, the type for
>   the function pointer does not match and compilation therefore fails.
> - updated calls to i2c_recover_bus
> 
>  drivers/i2c/busses/i2c-bcm-iproc.c | 111 +++++++++++++++++++++++++++++
>  1 file changed, 111 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index 4c8c3bc4669c..78393c3b5e83 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c

[...]

> @@ -78,6 +79,11 @@
>  #define M_RX_DATA_SHIFT              0
>  #define M_RX_DATA_MASK               0xff
>  
> +#define M_BB_CTRL_OFFSET             0x14
> +#define M_BB_CTRL_CLK_IN_SHIFT       31
> +#define M_BB_CTRL_CLK_SHIFT          30

I think it missed this from my previous review: M_BB_CTRL_CLK_OUT_SHIFT

> +#define M_BB_CTRL_DATA_IN_SHIFT      29
> +
>  #define I2C_TIMEOUT_MSEC             50000
>  #define M_TX_RX_FIFO_SIZE            64
>  
> @@ -208,6 +214,108 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>  }
>  
> +/*
> + * Switch to bit bang mode to prepare for i2c generic recovery.
> + */
> +static void bcm_iproc_i2c_prepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
> +	u32 tmp;
> +
> +	dev_dbg(iproc_i2c->device, "Prepare recovery\n");
> +
> +	/* Disable interrupts */
> +	writel(0, iproc_i2c->base + IE_OFFSET);
> +	readl(iproc_i2c->base + IE_OFFSET);
> +	synchronize_irq(iproc_i2c->irq);
> +
> +	/* Put the i2c controller into reset. */
> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);
> +	tmp |= BIT(CFG_RESET_SHIFT);
> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);
> +	udelay(100);
> +
> +	/* Switch to bit-bang mode */
> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);
> +	tmp |= BIT(CFG_BIT_BANG_SHIFT);
> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);
> +}
> +
> +/*
> + * Return to normal i2c operation following recovery.
> + */
> +static void bcm_iproc_i2c_unprepare_recovery(struct i2c_adapter *adap)
> +{
> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
> +	u32 tmp;
> +
> +	/* Switch to normal mode */
> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);
> +	tmp &= ~BIT(CFG_BIT_BANG_SHIFT);
> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);
> +	udelay(100);
> +
> +	bcm_iproc_i2c_init(iproc_i2c);
> +	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> +
> +	dev_dbg(iproc_i2c->device, "Recovery complete\n");
> +}
> +
> +/*
> + * Return the SCL state, we must be configured in bit bang mode for this
> + * to work.
> + */
> +static int bcm_iproc_i2c_get_scl(struct i2c_adapter *adap)
> +{
> +	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
> +	u32 tmp;
> +
> +	tmp = readl(iproc_i2c->base + M_BB_CTRL_OFFSET);
> +
> +	return tmp & BIT(M_BB_CTRL_CLK_IN_SHIFT);
> +}
> +
> +/*
> + * Set the SCL state, we must be configured in bit bang mode for this
> + * to work.
> + */
> +static void bcm_iproc_i2c_set_scl(struct i2c_adapter *adap, int val)

You are missing to address or reply to another comment from me here.

I'll stop my review here. Please ensure you go through my previous
review thoroughly, and reply/address all of the comments.

In addition, note Wolfram just merged a series of iProc I2C patches into
his tree in branch 'i2c/for-next'. You may want to rebase your patch to
that for the next iteration.

Thanks,

Ray