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

Subsystems: i2c subsystem, i2c subsystem host drivers, the rest

STALE1923d

6 messages, 2 authors, 2021-06-03 · open the first message on its own page

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

From: Chris Packham <chris.packham@alliedtelesis.co.nz>
Date: 2021-05-30 23:00:07 

From: Richard Laing <redacted>

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

From: Ray Jui <hidden>
Date: 2021-06-01 21:03:20 


On 5/30/2021 3:56 PM, Chris Packham wrote:
quoted hunk
From: Richard Laing <redacted>

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 <redacted>
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)
quoted hunk
 #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
quoted hunk
+
 #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 ...
quoted hunk
+	.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).
quoted hunk
 		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

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

From: Chris Packham <Chris.Packham@alliedtelesis.co.nz>
Date: 2021-06-01 21:45:09 

On 2/06/21 9:03 am, Ray Jui wrote:
On 5/30/2021 3:56 PM, Chris Packham wrote:
quoted
From: Richard Laing <redacted>

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 <redacted>
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.
quoted
  #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.
quoted
+#define M_BB_SMBCLK_OUT_EN           30
M_BB_CTRL_CLK_OUT_SHIFT
quoted
+#define M_BB_SMBDAT_IN               29
M_BB_CTRL_DATA_IN_SHIFT
quoted
+#define M_BB_SMBDAT_OUT_EN           28
M_BB_CTRL_DATA_OUT_SHIFT
quoted
+
  #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().

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

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

quoted
  		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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel





  

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

    

      From: Ray Jui <hidden>

      Date: 2021-06-01 21:51:52 
    

  

  

On 6/1/2021 2:44 PM, Chris Packham wrote:
On 2/06/21 9:03 am, Ray Jui wrote:
quoted
On 5/30/2021 3:56 PM, Chris Packham wrote:
quoted
From: Richard Laing <redacted>

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

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

quoted
quoted
+#define M_BB_SMBCLK_OUT_EN           30
M_BB_CTRL_CLK_OUT_SHIFT

quoted
+#define M_BB_SMBDAT_IN               29
M_BB_CTRL_DATA_IN_SHIFT

quoted
+#define M_BB_SMBDAT_OUT_EN           28
M_BB_CTRL_DATA_OUT_SHIFT

quoted
+
  #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.

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

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

quoted
quoted
+	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.
quoted
quoted
+	.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).

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





  

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

    

      From: Chris Packham <Chris.Packham@alliedtelesis.co.nz>

      Date: 2021-06-02 02:32:24 
    

  

  
On 2/06/21 9:03 am, Ray Jui wrote:
On 5/30/2021 3:56 PM, Chris Packham wrote:
quoted
From: Richard Laing <redacted>

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 <redacted>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
<snip>
+
quoted
+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.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel





  

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

    

      From: Ray Jui <hidden>

      Date: 2021-06-03 17:02:13 
    

  

  

On 6/1/2021 7:32 PM, Chris Packham wrote:
On 2/06/21 9:03 am, Ray Jui wrote:
quoted
On 5/30/2021 3:56 PM, Chris Packham wrote:
quoted
From: Richard Laing <redacted>

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 <redacted>
Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
<snip>
quoted
+
quoted
+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.




    
  

  
    

      

        Keyboard shortcuts
      

      

        
          
hback out one level

          
jnext message in thread

          
kprevious message in thread

          
ldrill in

          
Escclose help / fold thread tree

          
?toggle this help