From: Richard Laing <[email protected]>
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 <[email protected]>
Signed-off-by: Chris Packham <[email protected]>
---
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
#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;
--
2.31.1
On 5/30/2021 3:56 PM, Chris Packham wrote:
> From: Richard Laing <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Chris Packham <[email protected]>
> ---
>
> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chris Packham <[email protected]>
>> ---
>>
>> 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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Chris Packham <[email protected]>
>>> ---
>>>
>>> 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 <[email protected]>
>>
>> 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 <[email protected]>
>> Signed-off-by: Chris Packham <[email protected]>
>> ---
<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 <[email protected]>
>>>
>>> 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 <[email protected]>
>>> Signed-off-by: Chris Packham <[email protected]>
>>> ---
> <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.