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

Message ID 20210601230710.6861-1-chris.packham@alliedtelesis.co.nz
State Superseded
Headers show
Series
  • [v2] i2c: bcm-iproc: Add i2c recovery support
Related show

Commit Message

Chris Packham June 1, 2021, 11:07 p.m.
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>
---
Richard did most of the work on this. I'm just cleaning it up to get it
upstream.

Changes in v2:
- Incorporate feedback from Ray Jui
- Move bcm_iproc_i2c_resume so it can be re-used to return the i2c bus
  to normal operation at the correct speed after a recovery.
- Add iproc_i2c_lockup_recover() helper to only trigger recovery if sda
  is actually stuck
- Use usleep_range() instead of udelay()
- Cosmetic changes to register bit definitions

 drivers/i2c/busses/i2c-bcm-iproc.c | 176 +++++++++++++++++++++++++----
 1 file changed, 151 insertions(+), 25 deletions(-)

Comments

kernel test robot June 2, 2021, 2:11 a.m. | #1
Hi Chris,

Thank you for the patch! Yet something to improve:

[auto build test ERROR on wsa/i2c/for-next]
[also build test ERROR on v5.13-rc4 next-20210601]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Chris-Packham/i2c-bcm-iproc-Add-i2c-recovery-support/20210602-070900
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: arm64-randconfig-r003-20210601 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project db26cd30b6dd65e88d786e97a1e453af5cd48966)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install arm64 cross compiling tool for clang build
        # apt-get install binutils-aarch64-linux-gnu
        # https://github.com/0day-ci/linux/commit/9671d44b08834b225120a1288271bb38c01c1f33
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Chris-Packham/i2c-bcm-iproc-Add-i2c-recovery-support/20210602-070900
        git checkout 9671d44b08834b225120a1288271bb38c01c1f33
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/i2c/busses/i2c-bcm-iproc.c:1199:3: warning: cast to smaller integer type 'enum bcm_iproc_i2c_type' from 'const void *' [-Wvoid-pointer-to-enum-cast]
                   (enum bcm_iproc_i2c_type)of_device_get_match_data(&pdev->dev);
                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> 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;
                                   ^ ~~~~~~~~~~~~~~~~~~~~~~~~
   1 warning and 1 error generated.


vim +1264 drivers/i2c/busses/i2c-bcm-iproc.c

  1183	
  1184	static int bcm_iproc_i2c_probe(struct platform_device *pdev)
  1185	{
  1186		int irq, ret = 0;
  1187		struct bcm_iproc_i2c_dev *iproc_i2c;
  1188		struct i2c_adapter *adap;
  1189		struct resource *res;
  1190	
  1191		iproc_i2c = devm_kzalloc(&pdev->dev, sizeof(*iproc_i2c),
  1192					 GFP_KERNEL);
  1193		if (!iproc_i2c)
  1194			return -ENOMEM;
  1195	
  1196		platform_set_drvdata(pdev, iproc_i2c);
  1197		iproc_i2c->device = &pdev->dev;
  1198		iproc_i2c->type =
  1199			(enum bcm_iproc_i2c_type)of_device_get_match_data(&pdev->dev);
  1200		init_completion(&iproc_i2c->done);
  1201	
  1202		res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
  1203		iproc_i2c->base = devm_ioremap_resource(iproc_i2c->device, res);
  1204		if (IS_ERR(iproc_i2c->base))
  1205			return PTR_ERR(iproc_i2c->base);
  1206	
  1207		if (iproc_i2c->type == IPROC_I2C_NIC) {
  1208			res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
  1209			iproc_i2c->idm_base = devm_ioremap_resource(iproc_i2c->device,
  1210								    res);
  1211			if (IS_ERR(iproc_i2c->idm_base))
  1212				return PTR_ERR(iproc_i2c->idm_base);
  1213	
  1214			ret = of_property_read_u32(iproc_i2c->device->of_node,
  1215						   "brcm,ape-hsls-addr-mask",
  1216						   &iproc_i2c->ape_addr_mask);
  1217			if (ret < 0) {
  1218				dev_err(iproc_i2c->device,
  1219					"'brcm,ape-hsls-addr-mask' missing\n");
  1220				return -EINVAL;
  1221			}
  1222	
  1223			spin_lock_init(&iproc_i2c->idm_lock);
  1224	
  1225			/* no slave support */
  1226			bcm_iproc_algo.reg_slave = NULL;
  1227			bcm_iproc_algo.unreg_slave = NULL;
  1228		}
  1229	
  1230		ret = bcm_iproc_i2c_init(iproc_i2c);
  1231		if (ret)
  1232			return ret;
  1233	
  1234		ret = bcm_iproc_i2c_cfg_speed(iproc_i2c);
  1235		if (ret)
  1236			return ret;
  1237	
  1238		irq = platform_get_irq(pdev, 0);
  1239		if (irq > 0) {
  1240			ret = devm_request_irq(iproc_i2c->device, irq,
  1241					       bcm_iproc_i2c_isr, 0, pdev->name,
  1242					       iproc_i2c);
  1243			if (ret < 0) {
  1244				dev_err(iproc_i2c->device,
  1245					"unable to request irq %i\n", irq);
  1246				return ret;
  1247			}
  1248	
  1249			iproc_i2c->irq = irq;
  1250		} else {
  1251			dev_warn(iproc_i2c->device,
  1252				 "no irq resource, falling back to poll mode\n");
  1253		}
  1254	
  1255		bcm_iproc_i2c_enable_disable(iproc_i2c, true);
  1256	
  1257		adap = &iproc_i2c->adapter;
  1258		i2c_set_adapdata(adap, iproc_i2c);
  1259		snprintf(adap->name, sizeof(adap->name),
  1260			"Broadcom iProc (%s)",
  1261			of_node_full_name(iproc_i2c->device->of_node));
  1262		adap->algo = &bcm_iproc_algo;
  1263		adap->quirks = &bcm_iproc_i2c_quirks;
> 1264		adap->bus_recovery_info = &bcm_iproc_recovery_info;

  1265		adap->dev.parent = &pdev->dev;
  1266		adap->dev.of_node = pdev->dev.of_node;
  1267	
  1268		return i2c_add_adapter(adap);
  1269	}
  1270	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

Patch

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index cceaf69279a9..417eb8d69d54 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -25,6 +25,7 @@ 
 #define CFG_OFFSET                   0x00
 #define CFG_RESET_SHIFT              31
 #define CFG_EN_SHIFT                 30
+#define CFG_BIT_BANG_SHIFT           29
 #define CFG_SLAVE_ADDR_0_SHIFT       28
 #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_SHIFT         31
+#define M_BB_SMBCLK_OUT_EN_SHIFT     30
+#define M_BB_SMBDAT_IN_SHIFT         29
+#define M_BB_SMBDAT_OUT_EN_SHIFT     28
+
 #define M_CMD_OFFSET                 0x30
 #define M_CMD_START_BUSY_SHIFT       31
 #define M_CMD_STATUS_SHIFT           25
@@ -713,6 +720,147 @@  static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
 	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 }
 
+static int bcm_iproc_i2c_resume(struct device *dev)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = dev_get_drvdata(dev);
+	int ret;
+	u32 val;
+
+	/*
+	 * Power domain could have been shut off completely in system deep
+	 * sleep, so re-initialize the block here
+	 */
+	ret = bcm_iproc_i2c_init(iproc_i2c);
+	if (ret)
+		return ret;
+
+	/* configure to the desired bus speed */
+	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
+	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
+	val |= (iproc_i2c->bus_speed == I2C_MAX_FAST_MODE_FREQ) << TIM_CFG_MODE_400_SHIFT;
+	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);
+
+	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
+
+	return 0;
+}
+
+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);
+
+	/* Place controller in reset */
+	tmp = readl(iproc_i2c->base + CFG_OFFSET);
+	tmp |= BIT(CFG_RESET_SHIFT);
+	writel(tmp, iproc_i2c->base + CFG_OFFSET);
+	usleep_range(100, 200);
+
+	/* 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);
+	usleep_range(100, 200);
+}
+
+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);
+	usleep_range(100, 200);
+
+	bcm_iproc_i2c_resume(iproc_i2c->device);
+
+	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_SHIFT));
+}
+
+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_SHIFT);
+	else
+		tmp &= ~BIT(M_BB_SMBCLK_OUT_EN_SHIFT);
+
+	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_SHIFT);
+	else
+		tmp &= ~BIT(M_BB_SMBDAT_OUT_EN_SHIFT);
+
+	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_SHIFT));
+}
+
+/* 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);
+	}
+}
+
+static const 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)
 {
@@ -806,6 +954,7 @@  static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
 		/* flush both TX/RX FIFOs */
 		val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
+		iproc_i2c_lockup_recover(iproc_i2c);
 		return -ETIMEDOUT;
 	}
 
@@ -814,6 +963,7 @@  static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
 		/* flush both TX/RX FIFOs */
 		val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
 		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
+		iproc_i2c_lockup_recover(iproc_i2c);
 		return ret;
 	}
 
@@ -1111,6 +1261,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;
 
@@ -1159,31 +1310,6 @@  static int bcm_iproc_i2c_suspend(struct device *dev)
 	return 0;
 }
 
-static int bcm_iproc_i2c_resume(struct device *dev)
-{
-	struct bcm_iproc_i2c_dev *iproc_i2c = dev_get_drvdata(dev);
-	int ret;
-	u32 val;
-
-	/*
-	 * Power domain could have been shut off completely in system deep
-	 * sleep, so re-initialize the block here
-	 */
-	ret = bcm_iproc_i2c_init(iproc_i2c);
-	if (ret)
-		return ret;
-
-	/* configure to the desired bus speed */
-	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
-	val &= ~BIT(TIM_CFG_MODE_400_SHIFT);
-	val |= (iproc_i2c->bus_speed == I2C_MAX_FAST_MODE_FREQ) << TIM_CFG_MODE_400_SHIFT;
-	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);
-
-	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
-
-	return 0;
-}
-
 static const struct dev_pm_ops bcm_iproc_i2c_pm_ops = {
 	.suspend_late = &bcm_iproc_i2c_suspend,
 	.resume_early = &bcm_iproc_i2c_resume