| Message ID | 20210530225659.17138-1-chris.packham@alliedtelesis.co.nz |
|---|---|
| State | New |
| Headers | show |
| Series | i2c: bcm-iproc: Add i2c recovery support | expand |
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
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@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) 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
On 6/1/2021 2:44 PM, Chris Packham wrote: > > 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@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) > > 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(). > Sure I think that makes sense. >> >>> + >>> +} >>> + >>> +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. Yes agree with you that 'bcm_iproc_i2c_resume' can be resued. >>> + 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? > You may leave that (as first level recovery due to some potential internal ASIC issues). Let iproc_i2c_lockup_recover handles the bus lockup type of recovery (after confirming the bus is indeed locked up). >> >>> 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
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@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> >> --- <snip> > + >> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = { > static const struct ... The kernel test bot doesn't like the suggestion. >> drivers/i2c/busses/i2c-bcm-iproc.c:1264:26: error: assigning to 'struct i2c_bus_recovery_info *' from 'const struct i2c_bus_recovery_info *' discards qualifiers [-Werror,-Wincompatible-pointer-types-discards-qualifiers] adap->bus_recovery_info = &bcm_iproc_recovery_info; ^ ~~~~~~~~~~~~~~~~~~~~~~~~ I can see why. Are you happy for me to drop the const in v3? Or perhaps you have a better suggestion.
On 6/1/2021 7:32 PM, Chris Packham wrote: > > 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@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> >>> --- > <snip> >> + >>> +static struct i2c_bus_recovery_info bcm_iproc_recovery_info = { >> static const struct ... > > The kernel test bot doesn't like the suggestion. > > >> drivers/i2c/busses/i2c-bcm-iproc.c:1264:26: error: assigning to > 'struct i2c_bus_recovery_info *' from 'const struct > i2c_bus_recovery_info *' discards qualifiers > [-Werror,-Wincompatible-pointer-types-discards-qualifiers] > > adap->bus_recovery_info = &bcm_iproc_recovery_info; > ^ ~~~~~~~~~~~~~~~~~~~~~~~~ > > I can see why. Are you happy for me to drop the const in v3? Or perhaps > you have a better suggestion. > Yeah of course. Please drop it. Sorry I missed that while making that suggestion.
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 #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 +#define M_BB_SMBCLK_OUT_EN 30 +#define M_BB_SMBDAT_IN 29 +#define M_BB_SMBDAT_OUT_EN 28 + #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); + +} + +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); +} + +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"); +} + +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 = { + .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); 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;