Messages in this thread Patch in this message | | | Subject | Re: [PATCH] i2c: bcm-iproc: Add i2c recovery support | From | Ray Jui <> | Date | Tue, 1 Jun 2021 14:03:06 -0700 |
| |
On 5/30/2021 3:56 PM, Chris Packham wrote: > From: Richard Laing <richard.laing@alliedtelesis.co.nz> > > 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@alliedtelesis.co.nz> > Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz> > --- > > 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 [unhandled content-type:application/pkcs7-signature]
| |