Re: [PATCH] i2c: bcm-iproc: Add i2c recovery support

[Ray Jui]

On 5/30/2021 3:56 PM, Chris Packham wrote:
> From: Richard Laing <richard.laing@xxxxxxxxxxxxxxxxxxx>
> 
> The bcm-iproc controller can put the SDA/SCL lines into bit-bang mode,
> make use of this to support i2c bus recovery.
> 
> Signed-off-by: Richard Laing <richard.laing@xxxxxxxxxxxxxxxxxxx>
> Signed-off-by: Chris Packham <chris.packham@xxxxxxxxxxxxxxxxxxx>
> ---
> 
> Notes:
>     Richard did most of the work on this. I'm just cleaning it up to get it
>     upstream.
> 
>  drivers/i2c/busses/i2c-bcm-iproc.c | 115 +++++++++++++++++++++++++++++
>  1 file changed, 115 insertions(+)
> 
> diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
> index cceaf69279a9..d63a286c1660 100644
> --- a/drivers/i2c/busses/i2c-bcm-iproc.c
> +++ b/drivers/i2c/busses/i2c-bcm-iproc.c
> @@ -26,6 +26,7 @@
>  #define CFG_RESET_SHIFT              31
>  #define CFG_EN_SHIFT                 30
>  #define CFG_SLAVE_ADDR_0_SHIFT       28
> +#define CFG_BIT_BANG_SHIFT           29

move this up one line (to be consistent with existing bit order)

>  #define CFG_M_RETRY_CNT_SHIFT        16
>  #define CFG_M_RETRY_CNT_MASK         0x0f
>  
> @@ -66,6 +67,12 @@
>  #define S_FIFO_RX_THLD_SHIFT         8
>  #define S_FIFO_RX_THLD_MASK          0x3f
>  
> +#define M_BB_CTRL_OFFSET             0x14
> +#define M_BB_SMBCLK_IN               31

M_BB_CTRL_CLK_IN_SHIFT, ket is to have '_SHIFT' to be consistent with
existing code

> +#define M_BB_SMBCLK_OUT_EN           30

M_BB_CTRL_CLK_OUT_SHIFT

> +#define M_BB_SMBDAT_IN               29

M_BB_CTRL_DATA_IN_SHIFT

> +#define M_BB_SMBDAT_OUT_EN           28

M_BB_CTRL_DATA_OUT_SHIFT

> +
>  #define M_CMD_OFFSET                 0x30
>  #define M_CMD_START_BUSY_SHIFT       31
>  #define M_CMD_STATUS_SHIFT           25
> @@ -713,6 +720,112 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
>  	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
>  }
>  
> +static void bcm_iproc_i2c_reset(struct bcm_iproc_i2c_dev *iproc_i2c)
> +{
> +	u32 tmp;
> +
> +	tmp = readl(iproc_i2c->base + CFG_OFFSET);
> +	tmp |= BIT(CFG_RESET_SHIFT);
> +	writel(tmp, iproc_i2c->base + CFG_OFFSET);
> +	udelay(100);

This puts the controller in reset and hold it there, but never brings
the controller out of reset (bcm_iproc_i2c_init called in unprepare
brings the controller out of reset)

Calling it a "reset" function is a bit misleading to me. My expectation
of a reset function is that you generate a reset pulse, ie.g., reset ->
delay -> out of reset.

Why don't you simply put this seuquence of code in the prepare_recovery
function below, instead of calling this a reset function?

> +
> +}
> +
> +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);
> +
> +	bcm_iproc_i2c_reset(iproc_i2c);
> +
> +	/* 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);

add usleep_range(100, 200) here, required delay after switching to bit
bang based on spec.

> +}
> +
> +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);

Add sequence to re-configure to desired bus speed here after the reset
sequence (someone else in our team tested this is required to resume to
proper bus speed).

> +	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
> +
> +	dev_dbg(iproc_i2c->device, "Recovery complete\n");
> +}
> +
> +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_SMBCLK_IN));
> +}
> +
> +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_SMBCLK_OUT_EN);
> +	else
> +		tmp &= ~BIT(M_BB_SMBCLK_OUT_EN);
> +
> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
> +}
> +
> +static void bcm_iproc_i2c_set_sda(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_SMBDAT_OUT_EN);
> +	else
> +		tmp &= ~BIT(M_BB_SMBDAT_OUT_EN);
> +
> +	writel(tmp, iproc_i2c->base + M_BB_CTRL_OFFSET);
> +}
> +
> +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_SMBDAT_IN));
> +}
> +
> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = {

static const struct ...

> +	.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,
> +	.set_sda = bcm_iproc_i2c_set_sda,
> +	.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)
>  {
> @@ -839,6 +952,7 @@ static int bcm_iproc_i2c_xfer_internal(struct bcm_iproc_i2c_dev *iproc_i2c,
>  	if (!!(iproc_i2c_rd_reg(iproc_i2c,
>  				M_CMD_OFFSET) & BIT(M_CMD_START_BUSY_SHIFT))) {
>  		dev_warn(iproc_i2c->device, "bus is busy\n");
> +		i2c_recover_bus(&iproc_i2c->adapter);

'i2c_recover_bus' should not be ALWAYS called here. You don't know if
bus is actually locked up or it's other issues that caused this.

We need a logic to detect and confirm the lock up condition before
committing to recover operation:

/* Check if bus lockup occurred, and invoke recovery if so. */
static void iproc_i2c_lockup_recover(struct bcm_iproc_i2c_dev *iproc_i2c)
{
	/*
	 * assume bus lockup if SDA line is low;
	 * note that there is no need to switch to
	 * bit-bang mode for this check.
	 */
	if (!bcm_iproc_i2c_get_sda(&iproc_i2c->adapter)) {
		/* locked up - invoke i2c bus recovery. */
		int ret = i2c_recover_bus(&iproc_i2c->adapter);
			if (ret)
				dev_err(iproc_i2c->device,
					"bus recovery: error %d\n",
					ret);
	}
}

'iproc_i2c_lockup_recover' should be called in two locations in the driver:

1. After 'transaction timed out' (and after flush both TX/RX FIFOS)
2. After 'bcm_iproc_i2c_check_status' failures (and after flush both
TX/RX FIFOs).

>  		return -EBUSY;
>  	}
>  
> @@ -1111,6 +1225,7 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
>  		of_node_full_name(iproc_i2c->device->of_node));
>  	adap->algo = &bcm_iproc_algo;
>  	adap->quirks = &bcm_iproc_i2c_quirks;
> +	adap->bus_recovery_info = &bcm_iproc_recovery_info;
>  	adap->dev.parent = &pdev->dev;
>  	adap->dev.of_node = pdev->dev.of_node;
>  
> 

Thanks,

Ray

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature