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

From: Chris Packham
Date: Tue Jun 01 2021 - 17:47:16 EST


On 2/06/21 9:03 am, Ray Jui wrote:
>
> 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)

Ack.

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

Ack.

>> +#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?

Will do. Should I also change the udelay(100) to a usleep_range(100,
200)? Same question for the other uses of udelay().

>
>> +
>> +}
>> +
>> +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.

Ack.

>
>> +}
>> +
>> +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_resume() seems to have the exact sequence I need. If I
move it outside of CONFIG_PM_SLEEP I could just call it here.
>> + 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 ...
Ack.
>> + .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).

I see bcm_iproc_i2c_check_status() has a "recovery" case on an unknown
error. Should I remove that if I'm going to call
iproc_i2c_lockup_recover() or check for ret != -EIO?

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