[PATCH v5 0/8] iProc I2C slave mode and NIC mode

[PATCH v5 0/8] iProc I2C slave mode and NIC mode

[Ray Jui]

This patch series adds the following support to the iProc I2C driver:
- Increase maximum read transfer size to 255 bytes
- I2C slave mode
- Polling mode
- NIC I2C mode

This patch series is based on kernel v5.0-rc3 and available at:
https://github.com/Broadcom/arm64-linux.git
branch: i2c-slave-v5

Changes from v4:
 - Add more detailed explanations in the device tree binding document
   changes, to address Rob's review comments

Changes from v3:
 - Various minor fixes on commit messages and commits
 - Rebased to v5.0-rc3

Changes from v2:
 - Address Ray's review comments.

Changes from v1:
 - Rebased to Linux v5.0.0-rc2

Ray Jui (1):
  dt-bindings: i2c: iproc: make 'interrupts' optional

Rayagonda Kokatanur (5):
  i2c: iproc: add polling support
  i2c: iproc: use wrapper for read/write access
  dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string
  i2c: iproc: add NIC I2C support
  arm64: dts: Stingray: Add NIC i2c device node

Shreesha Rajashekar (2):
  i2c: iproc: Extend I2C read up to 255 bytes
  i2c: iproc: Add slave mode support

 .../bindings/i2c/brcm,iproc-i2c.txt           |  17 +-
 .../boot/dts/broadcom/stingray/stingray.dtsi  |  18 +
 drivers/i2c/busses/Kconfig                    |   1 +
 drivers/i2c/busses/i2c-bcm-iproc.c            | 758 +++++++++++++++---
 4 files changed, 663 insertions(+), 131 deletions(-)

-- 
2.17.1

[PATCH v5 1/8] i2c: iproc: Extend I2C read up to 255 bytes

[Ray Jui]

From: Shreesha Rajashekar <shreesha@broadcom.com>

Add support to allow I2C master read transfer up to 255 bytes.

Signed-off-by: Shreesha Rajashekar <shreesha@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 98 +++++++++++++++++++++++-------
 1 file changed, 76 insertions(+), 22 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 4c8c3bc4669c..eb7339a0280e 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -80,6 +80,10 @@
 
 #define I2C_TIMEOUT_MSEC             50000
 #define M_TX_RX_FIFO_SIZE            64
+#define M_RX_FIFO_MAX_THLD_VALUE     (M_TX_RX_FIFO_SIZE - 1)
+
+#define M_RX_MAX_READ_LEN            255
+#define M_RX_FIFO_THLD_VALUE         50
 
 enum bus_speed_index {
 	I2C_SPD_100K = 0,
@@ -102,17 +106,41 @@ struct bcm_iproc_i2c_dev {
 
 	/* bytes that have been transferred */
 	unsigned int tx_bytes;
+	/* bytes that have been read */
+	unsigned int rx_bytes;
+	unsigned int thld_bytes;
 };
 
 /*
  * Can be expanded in the future if more interrupt status bits are utilized
  */
-#define ISR_MASK (BIT(IS_M_START_BUSY_SHIFT) | BIT(IS_M_TX_UNDERRUN_SHIFT))
+#define ISR_MASK (BIT(IS_M_START_BUSY_SHIFT) | BIT(IS_M_TX_UNDERRUN_SHIFT)\
+		| BIT(IS_M_RX_THLD_SHIFT))
+
+static void bcm_iproc_i2c_read_valid_bytes(struct bcm_iproc_i2c_dev *iproc_i2c)
+{
+	struct i2c_msg *msg = iproc_i2c->msg;
+
+	/* Read valid data from RX FIFO */
+	while (iproc_i2c->rx_bytes < msg->len) {
+		if (!((readl(iproc_i2c->base +
+		M_FIFO_CTRL_OFFSET) >>
+		M_FIFO_RX_CNT_SHIFT) &
+		M_FIFO_RX_CNT_MASK))
+			break;
+
+		msg->buf[iproc_i2c->rx_bytes] =
+			(readl(iproc_i2c->base + M_RX_OFFSET) >>
+			M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
+		iproc_i2c->rx_bytes++;
+	}
+}
 
 static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = data;
 	u32 status = readl(iproc_i2c->base + IS_OFFSET);
+	u32 tmp;
 
 	status &= ISR_MASK;
 
@@ -136,8 +164,6 @@ static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 
 			/* mark the last byte */
 			if (idx == msg->len - 1) {
-				u32 tmp;
-
 				val |= BIT(M_TX_WR_STATUS_SHIFT);
 
 				/*
@@ -156,6 +182,32 @@ static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 		iproc_i2c->tx_bytes += tx_bytes;
 	}
 
+	if (status & BIT(IS_M_RX_THLD_SHIFT)) {
+		struct i2c_msg *msg = iproc_i2c->msg;
+		u32 bytes_left;
+
+		bcm_iproc_i2c_read_valid_bytes(iproc_i2c);
+		bytes_left = msg->len - iproc_i2c->rx_bytes;
+		if (bytes_left == 0) {
+			/* finished reading all data, disable rx thld event */
+			tmp = readl(iproc_i2c->base + IE_OFFSET);
+			tmp &= ~BIT(IS_M_RX_THLD_SHIFT);
+			writel(tmp, iproc_i2c->base + IE_OFFSET);
+		} else if (bytes_left < iproc_i2c->thld_bytes) {
+			/* set bytes left as threshold */
+			tmp = readl(iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+			tmp &= ~(M_FIFO_RX_THLD_MASK << M_FIFO_RX_THLD_SHIFT);
+			tmp |= (bytes_left << M_FIFO_RX_THLD_SHIFT);
+			writel(tmp, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+			iproc_i2c->thld_bytes = bytes_left;
+		}
+		/*
+		 * bytes_left >= iproc_i2c->thld_bytes,
+		 * hence no need to change the THRESHOLD SET.
+		 * It will remain as iproc_i2c->thld_bytes itself
+		 */
+	}
+
 	if (status & BIT(IS_M_START_BUSY_SHIFT)) {
 		iproc_i2c->xfer_is_done = 1;
 		complete(&iproc_i2c->done);
@@ -253,7 +305,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 {
 	int ret, i;
 	u8 addr;
-	u32 val;
+	u32 val, tmp, val_intr_en;
 	unsigned int tx_bytes;
 	unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MSEC);
 
@@ -298,7 +350,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 * transaction is done, i.e., the internal start_busy bit, transitions
 	 * from 1 to 0.
 	 */
-	val = BIT(IE_M_START_BUSY_SHIFT);
+	val_intr_en = BIT(IE_M_START_BUSY_SHIFT);
 
 	/*
 	 * If TX data size is larger than the TX FIFO, need to enable TX
@@ -307,9 +359,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 */
 	if (!(msg->flags & I2C_M_RD) &&
 	    msg->len > iproc_i2c->tx_bytes)
-		val |= BIT(IE_M_TX_UNDERRUN_SHIFT);
-
-	writel(val, iproc_i2c->base + IE_OFFSET);
+		val_intr_en |= BIT(IE_M_TX_UNDERRUN_SHIFT);
 
 	/*
 	 * Now we can activate the transfer. For a read operation, specify the
@@ -317,11 +367,27 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	 */
 	val = BIT(M_CMD_START_BUSY_SHIFT);
 	if (msg->flags & I2C_M_RD) {
+		iproc_i2c->rx_bytes = 0;
+		if (msg->len > M_RX_FIFO_MAX_THLD_VALUE)
+			iproc_i2c->thld_bytes = M_RX_FIFO_THLD_VALUE;
+		else
+			iproc_i2c->thld_bytes = msg->len;
+
+		/* set threshold value */
+		tmp = readl(iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		tmp &= ~(M_FIFO_RX_THLD_MASK << M_FIFO_RX_THLD_SHIFT);
+		tmp |= iproc_i2c->thld_bytes << M_FIFO_RX_THLD_SHIFT;
+		writel(tmp, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+
+		/* enable the RX threshold interrupt */
+		val_intr_en |= BIT(IE_M_RX_THLD_SHIFT);
+
 		val |= (M_CMD_PROTOCOL_BLK_RD << M_CMD_PROTOCOL_SHIFT) |
 		       (msg->len << M_CMD_RD_CNT_SHIFT);
 	} else {
 		val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
 	}
+	writel(val_intr_en, iproc_i2c->base + IE_OFFSET);
 	writel(val, iproc_i2c->base + M_CMD_OFFSET);
 
 	time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left);
@@ -353,17 +419,6 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 		return ret;
 	}
 
-	/*
-	 * For a read operation, we now need to load the data from FIFO
-	 * into the memory buffer
-	 */
-	if (msg->flags & I2C_M_RD) {
-		for (i = 0; i < msg->len; i++) {
-			msg->buf[i] = (readl(iproc_i2c->base + M_RX_OFFSET) >>
-				      M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
-		}
-	}
-
 	return 0;
 }
 
@@ -395,9 +450,8 @@ static const struct i2c_algorithm bcm_iproc_algo = {
 	.functionality = bcm_iproc_i2c_functionality,
 };
 
-static const struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
-	/* need to reserve one byte in the FIFO for the slave address */
-	.max_read_len = M_TX_RX_FIFO_SIZE - 1,
+static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
+	.max_read_len = M_RX_MAX_READ_LEN,
 };
 
 static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
-- 
2.17.1

[PATCH v5 2/8] i2c: iproc: Add slave mode support

[Ray Jui]

From: Shreesha Rajashekar <shreesha.rajashekar@broadcom.com>

Add slave mode support to the iProc I2C driver.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Michael Cheng <ccheng@broadcom.com>
Signed-off-by: Shreesha Rajashekar <shreesha.rajashekar@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/Kconfig         |   1 +
 drivers/i2c/busses/i2c-bcm-iproc.c | 314 ++++++++++++++++++++++++++++-
 2 files changed, 309 insertions(+), 6 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f2c681971201..e0798b82b3bc 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -424,6 +424,7 @@ config I2C_BCM_IPROC
 	tristate "Broadcom iProc I2C controller"
 	depends on ARCH_BCM_IPROC || COMPILE_TEST
 	default ARCH_BCM_IPROC
+	select I2C_SLAVE
 	help
 	  If you say yes to this option, support will be included for the
 	  Broadcom iProc I2C controller.
diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index eb7339a0280e..775b3d89a9c4 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -23,11 +23,30 @@
 #define CFG_OFFSET                   0x00
 #define CFG_RESET_SHIFT              31
 #define CFG_EN_SHIFT                 30
+#define CFG_SLAVE_ADDR_0_SHIFT       28
 #define CFG_M_RETRY_CNT_SHIFT        16
 #define CFG_M_RETRY_CNT_MASK         0x0f
 
 #define TIM_CFG_OFFSET               0x04
 #define TIM_CFG_MODE_400_SHIFT       31
+#define TIM_RAND_SLAVE_STRETCH_SHIFT      24
+#define TIM_RAND_SLAVE_STRETCH_MASK       0x7f
+#define TIM_PERIODIC_SLAVE_STRETCH_SHIFT  16
+#define TIM_PERIODIC_SLAVE_STRETCH_MASK   0x7f
+
+#define S_CFG_SMBUS_ADDR_OFFSET           0x08
+#define S_CFG_EN_NIC_SMB_ADDR3_SHIFT      31
+#define S_CFG_NIC_SMB_ADDR3_SHIFT         24
+#define S_CFG_NIC_SMB_ADDR3_MASK          0x7f
+#define S_CFG_EN_NIC_SMB_ADDR2_SHIFT      23
+#define S_CFG_NIC_SMB_ADDR2_SHIFT         16
+#define S_CFG_NIC_SMB_ADDR2_MASK          0x7f
+#define S_CFG_EN_NIC_SMB_ADDR1_SHIFT      15
+#define S_CFG_NIC_SMB_ADDR1_SHIFT         8
+#define S_CFG_NIC_SMB_ADDR1_MASK          0x7f
+#define S_CFG_EN_NIC_SMB_ADDR0_SHIFT      7
+#define S_CFG_NIC_SMB_ADDR0_SHIFT         0
+#define S_CFG_NIC_SMB_ADDR0_MASK          0x7f
 
 #define M_FIFO_CTRL_OFFSET           0x0c
 #define M_FIFO_RX_FLUSH_SHIFT        31
@@ -37,6 +56,14 @@
 #define M_FIFO_RX_THLD_SHIFT         8
 #define M_FIFO_RX_THLD_MASK          0x3f
 
+#define S_FIFO_CTRL_OFFSET           0x10
+#define S_FIFO_RX_FLUSH_SHIFT        31
+#define S_FIFO_TX_FLUSH_SHIFT        30
+#define S_FIFO_RX_CNT_SHIFT          16
+#define S_FIFO_RX_CNT_MASK           0x7f
+#define S_FIFO_RX_THLD_SHIFT         8
+#define S_FIFO_RX_THLD_MASK          0x3f
+
 #define M_CMD_OFFSET                 0x30
 #define M_CMD_START_BUSY_SHIFT       31
 #define M_CMD_STATUS_SHIFT           25
@@ -46,6 +73,8 @@
 #define M_CMD_STATUS_NACK_ADDR       0x2
 #define M_CMD_STATUS_NACK_DATA       0x3
 #define M_CMD_STATUS_TIMEOUT         0x4
+#define M_CMD_STATUS_FIFO_UNDERRUN   0x5
+#define M_CMD_STATUS_RX_FIFO_FULL    0x6
 #define M_CMD_PROTOCOL_SHIFT         9
 #define M_CMD_PROTOCOL_MASK          0xf
 #define M_CMD_PROTOCOL_BLK_WR        0x7
@@ -54,17 +83,36 @@
 #define M_CMD_RD_CNT_SHIFT           0
 #define M_CMD_RD_CNT_MASK            0xff
 
+#define S_CMD_OFFSET                 0x34
+#define S_CMD_START_BUSY_SHIFT       31
+#define S_CMD_STATUS_SHIFT           23
+#define S_CMD_STATUS_MASK            0x07
+#define S_CMD_STATUS_SUCCESS         0x0
+#define S_CMD_STATUS_TIMEOUT         0x5
+
 #define IE_OFFSET                    0x38
 #define IE_M_RX_FIFO_FULL_SHIFT      31
 #define IE_M_RX_THLD_SHIFT           30
 #define IE_M_START_BUSY_SHIFT        28
 #define IE_M_TX_UNDERRUN_SHIFT       27
+#define IE_S_RX_FIFO_FULL_SHIFT      26
+#define IE_S_RX_THLD_SHIFT           25
+#define IE_S_RX_EVENT_SHIFT          24
+#define IE_S_START_BUSY_SHIFT        23
+#define IE_S_TX_UNDERRUN_SHIFT       22
+#define IE_S_RD_EVENT_SHIFT          21
 
 #define IS_OFFSET                    0x3c
 #define IS_M_RX_FIFO_FULL_SHIFT      31
 #define IS_M_RX_THLD_SHIFT           30
 #define IS_M_START_BUSY_SHIFT        28
 #define IS_M_TX_UNDERRUN_SHIFT       27
+#define IS_S_RX_FIFO_FULL_SHIFT      26
+#define IS_S_RX_THLD_SHIFT           25
+#define IS_S_RX_EVENT_SHIFT          24
+#define IS_S_START_BUSY_SHIFT        23
+#define IS_S_TX_UNDERRUN_SHIFT       22
+#define IS_S_RD_EVENT_SHIFT          21
 
 #define M_TX_OFFSET                  0x40
 #define M_TX_WR_STATUS_SHIFT         31
@@ -78,6 +126,18 @@
 #define M_RX_DATA_SHIFT              0
 #define M_RX_DATA_MASK               0xff
 
+#define S_TX_OFFSET                  0x48
+#define S_TX_WR_STATUS_SHIFT         31
+#define S_TX_DATA_SHIFT              0
+#define S_TX_DATA_MASK               0xff
+
+#define S_RX_OFFSET                  0x4c
+#define S_RX_STATUS_SHIFT            30
+#define S_RX_STATUS_MASK             0x03
+#define S_RX_PEC_ERR_SHIFT           29
+#define S_RX_DATA_SHIFT              0
+#define S_RX_DATA_MASK               0xff
+
 #define I2C_TIMEOUT_MSEC             50000
 #define M_TX_RX_FIFO_SIZE            64
 #define M_RX_FIFO_MAX_THLD_VALUE     (M_TX_RX_FIFO_SIZE - 1)
@@ -85,6 +145,30 @@
 #define M_RX_MAX_READ_LEN            255
 #define M_RX_FIFO_THLD_VALUE         50
 
+#define IE_M_ALL_INTERRUPT_SHIFT     27
+#define IE_M_ALL_INTERRUPT_MASK      0x1e
+
+#define SLAVE_READ_WRITE_BIT_MASK    0x1
+#define SLAVE_READ_WRITE_BIT_SHIFT   0x1
+#define SLAVE_MAX_SIZE_TRANSACTION   64
+#define SLAVE_CLOCK_STRETCH_TIME     25
+
+#define IE_S_ALL_INTERRUPT_SHIFT     21
+#define IE_S_ALL_INTERRUPT_MASK      0x3f
+
+enum i2c_slave_read_status {
+	I2C_SLAVE_RX_FIFO_EMPTY = 0,
+	I2C_SLAVE_RX_START,
+	I2C_SLAVE_RX_DATA,
+	I2C_SLAVE_RX_END,
+};
+
+enum i2c_slave_xfer_dir {
+	I2C_SLAVE_DIR_READ = 0,
+	I2C_SLAVE_DIR_WRITE,
+	I2C_SLAVE_DIR_NONE,
+};
+
 enum bus_speed_index {
 	I2C_SPD_100K = 0,
 	I2C_SPD_400K,
@@ -104,6 +188,9 @@ struct bcm_iproc_i2c_dev {
 
 	struct i2c_msg *msg;
 
+	struct i2c_client *slave;
+	enum i2c_slave_xfer_dir xfer_dir;
+
 	/* bytes that have been transferred */
 	unsigned int tx_bytes;
 	/* bytes that have been read */
@@ -117,6 +204,156 @@ struct bcm_iproc_i2c_dev {
 #define ISR_MASK (BIT(IS_M_START_BUSY_SHIFT) | BIT(IS_M_TX_UNDERRUN_SHIFT)\
 		| BIT(IS_M_RX_THLD_SHIFT))
 
+#define ISR_MASK_SLAVE (BIT(IS_S_START_BUSY_SHIFT)\
+		| BIT(IS_S_RX_EVENT_SHIFT) | BIT(IS_S_RD_EVENT_SHIFT))
+
+static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave);
+static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave);
+static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
+					 bool enable);
+
+static void bcm_iproc_i2c_slave_init(
+	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
+{
+	u32 val;
+
+	if (need_reset) {
+		/* put controller in reset */
+		val = readl(iproc_i2c->base + CFG_OFFSET);
+		val |= BIT(CFG_RESET_SHIFT);
+		writel(val, iproc_i2c->base + CFG_OFFSET);
+
+		/* wait 100 usec per spec */
+		udelay(100);
+
+		/* bring controller out of reset */
+		val &= ~(BIT(CFG_RESET_SHIFT));
+		writel(val, iproc_i2c->base + CFG_OFFSET);
+	}
+
+	/* flush TX/RX FIFOs */
+	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
+	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);
+
+	/* RANDOM SLAVE STRETCH time - 20ms*/
+	val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
+	val &= ~(TIM_RAND_SLAVE_STRETCH_MASK << TIM_RAND_SLAVE_STRETCH_SHIFT);
+	val |= (SLAVE_CLOCK_STRETCH_TIME << TIM_RAND_SLAVE_STRETCH_SHIFT);
+	writel(val, iproc_i2c->base + TIM_CFG_OFFSET);
+
+	/* Configure the slave address */
+	val = readl(iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+	val |= BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
+	val &= ~(S_CFG_NIC_SMB_ADDR3_MASK << S_CFG_NIC_SMB_ADDR3_SHIFT);
+	val |= (iproc_i2c->slave->addr << S_CFG_NIC_SMB_ADDR3_SHIFT);
+	writel(val, iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+
+	/* clear all pending slave interrupts */
+	writel(ISR_MASK_SLAVE, iproc_i2c->base + IS_OFFSET);
+
+	/* Enable interrupt register for any READ event */
+	val = BIT(IE_S_RD_EVENT_SHIFT);
+	/* Enable interrupt register to indicate a valid byte in receive fifo */
+	val |= BIT(IE_S_RX_EVENT_SHIFT);
+	/* Enable interrupt register for the Slave BUSY command */
+	val |= BIT(IE_S_START_BUSY_SHIFT);
+	writel(val, iproc_i2c->base + IE_OFFSET);
+
+	iproc_i2c->xfer_dir = I2C_SLAVE_DIR_NONE;
+}
+
+static void bcm_iproc_i2c_check_slave_status(
+	struct bcm_iproc_i2c_dev *iproc_i2c)
+{
+	u32 val;
+
+	val = readl(iproc_i2c->base + S_CMD_OFFSET);
+	val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
+
+	if (val == S_CMD_STATUS_TIMEOUT) {
+		dev_err(iproc_i2c->device, "slave random stretch time timeout\n");
+
+		/* re-initialize i2c for recovery */
+		bcm_iproc_i2c_enable_disable(iproc_i2c, false);
+		bcm_iproc_i2c_slave_init(iproc_i2c, true);
+		bcm_iproc_i2c_enable_disable(iproc_i2c, true);
+	}
+}
+
+static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
+				u32 status)
+{
+	u8 value;
+	u32 val;
+	u32 rd_status;
+	u32 tmp;
+
+	/* Start of transaction. check address and populate the direction */
+	if (iproc_i2c->xfer_dir == I2C_SLAVE_DIR_NONE) {
+		tmp = readl(iproc_i2c->base + S_RX_OFFSET);
+		rd_status = (tmp >> S_RX_STATUS_SHIFT) & S_RX_STATUS_MASK;
+		/* This condition checks whether the request is a new request */
+		if (((rd_status == I2C_SLAVE_RX_START) &&
+			(status & BIT(IS_S_RX_EVENT_SHIFT))) ||
+			((rd_status == I2C_SLAVE_RX_END) &&
+			(status & BIT(IS_S_RD_EVENT_SHIFT)))) {
+
+			/* Last bit is W/R bit.
+			 * If 1 then its a read request(by master).
+			 */
+			iproc_i2c->xfer_dir = tmp & SLAVE_READ_WRITE_BIT_MASK;
+			if (iproc_i2c->xfer_dir == I2C_SLAVE_DIR_WRITE)
+				i2c_slave_event(iproc_i2c->slave,
+					I2C_SLAVE_READ_REQUESTED, &value);
+			else
+				i2c_slave_event(iproc_i2c->slave,
+					I2C_SLAVE_WRITE_REQUESTED, &value);
+		}
+	}
+
+	/* read request from master */
+	if ((status & BIT(IS_S_RD_EVENT_SHIFT)) &&
+		(iproc_i2c->xfer_dir == I2C_SLAVE_DIR_WRITE)) {
+		i2c_slave_event(iproc_i2c->slave,
+			I2C_SLAVE_READ_PROCESSED, &value);
+		writel(value, iproc_i2c->base + S_TX_OFFSET);
+
+		val = BIT(S_CMD_START_BUSY_SHIFT);
+		writel(val, iproc_i2c->base + S_CMD_OFFSET);
+	}
+
+	/* write request from master */
+	if ((status & BIT(IS_S_RX_EVENT_SHIFT)) &&
+		(iproc_i2c->xfer_dir == I2C_SLAVE_DIR_READ)) {
+		val = readl(iproc_i2c->base + S_RX_OFFSET);
+		/* Its a write request by Master to Slave.
+		 * We read data present in receive FIFO
+		 */
+		value = (u8)((val >> S_RX_DATA_SHIFT) & S_RX_DATA_MASK);
+		i2c_slave_event(iproc_i2c->slave,
+			I2C_SLAVE_WRITE_RECEIVED, &value);
+
+		/* check the status for the last byte of the transaction */
+		rd_status = (val >> S_RX_STATUS_SHIFT) & S_RX_STATUS_MASK;
+		if (rd_status == I2C_SLAVE_RX_END)
+			iproc_i2c->xfer_dir = I2C_SLAVE_DIR_NONE;
+
+		dev_dbg(iproc_i2c->device, "\nread value = 0x%x\n", value);
+	}
+
+	/* Stop */
+	if (status & BIT(IS_S_START_BUSY_SHIFT)) {
+		i2c_slave_event(iproc_i2c->slave, I2C_SLAVE_STOP, &value);
+		iproc_i2c->xfer_dir = I2C_SLAVE_DIR_NONE;
+	}
+
+	/* clear interrupt status */
+	writel(status, iproc_i2c->base + IS_OFFSET);
+
+	bcm_iproc_i2c_check_slave_status(iproc_i2c);
+	return true;
+}
+
 static void bcm_iproc_i2c_read_valid_bytes(struct bcm_iproc_i2c_dev *iproc_i2c)
 {
 	struct i2c_msg *msg = iproc_i2c->msg;
@@ -142,6 +379,18 @@ static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 	u32 status = readl(iproc_i2c->base + IS_OFFSET);
 	u32 tmp;
 
+
+	bool ret;
+	u32 sl_status = status & ISR_MASK_SLAVE;
+
+	if (sl_status) {
+		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, sl_status);
+		if (ret)
+			return IRQ_HANDLED;
+		else
+			return IRQ_NONE;
+	}
+
 	status &= ISR_MASK;
 
 	if (!status)
@@ -224,22 +473,25 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
 
 	/* put controller in reset */
 	val = readl(iproc_i2c->base + CFG_OFFSET);
-	val |= 1 << CFG_RESET_SHIFT;
-	val &= ~(1 << CFG_EN_SHIFT);
+	val |= BIT(CFG_RESET_SHIFT);
+	val &= ~(BIT(CFG_EN_SHIFT));
 	writel(val, iproc_i2c->base + CFG_OFFSET);
 
 	/* wait 100 usec per spec */
 	udelay(100);
 
 	/* bring controller out of reset */
-	val &= ~(1 << CFG_RESET_SHIFT);
+	val &= ~(BIT(CFG_RESET_SHIFT));
 	writel(val, iproc_i2c->base + CFG_OFFSET);
 
 	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
-	val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
+	val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
 	writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
 	/* disable all interrupts */
-	writel(0, iproc_i2c->base + IE_OFFSET);
+	val = readl(iproc_i2c->base + IE_OFFSET);
+	val &= ~(IE_M_ALL_INTERRUPT_MASK <<
+			IE_M_ALL_INTERRUPT_SHIFT);
+	writel(val, iproc_i2c->base + IE_OFFSET);
 
 	/* clear all pending interrupts */
 	writel(0xffffffff, iproc_i2c->base + IS_OFFSET);
@@ -288,6 +540,14 @@ static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 		dev_dbg(iproc_i2c->device, "bus timeout\n");
 		return -ETIMEDOUT;
 
+	case M_CMD_STATUS_FIFO_UNDERRUN:
+		dev_dbg(iproc_i2c->device, "FIFO under-run\n");
+		return -ENXIO;
+
+	case M_CMD_STATUS_RX_FIFO_FULL:
+		dev_dbg(iproc_i2c->device, "Master Rx FIFO full > 10ms\n");
+		return -ETIMEDOUT;
+
 	default:
 		dev_dbg(iproc_i2c->device, "unknown error code=%d\n", val);
 
@@ -442,12 +702,14 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
 
 static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
 }
 
 static const struct i2c_algorithm bcm_iproc_algo = {
 	.master_xfer = bcm_iproc_i2c_xfer,
 	.functionality = bcm_iproc_i2c_functionality,
+	.reg_slave = bcm_iproc_i2c_reg_slave,
+	.unreg_slave = bcm_iproc_i2c_unreg_slave,
 };
 
 static struct i2c_adapter_quirks bcm_iproc_i2c_quirks = {
@@ -612,6 +874,46 @@ static const struct dev_pm_ops bcm_iproc_i2c_pm_ops = {
 #define BCM_IPROC_I2C_PM_OPS NULL
 #endif /* CONFIG_PM_SLEEP */
 
+
+static int bcm_iproc_i2c_reg_slave(struct i2c_client *slave)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
+
+	if (iproc_i2c->slave)
+		return -EBUSY;
+
+	if (slave->flags & I2C_CLIENT_TEN)
+		return -EAFNOSUPPORT;
+
+	iproc_i2c->slave = slave;
+	bcm_iproc_i2c_slave_init(iproc_i2c, false);
+	return 0;
+}
+
+static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
+{
+	u32 tmp;
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(slave->adapter);
+
+	if (!iproc_i2c->slave)
+		return -EINVAL;
+
+	iproc_i2c->slave = NULL;
+
+	/* disable all slave interrupts */
+	tmp = readl(iproc_i2c->base + IE_OFFSET);
+	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
+			IE_S_ALL_INTERRUPT_SHIFT);
+	writel(tmp, iproc_i2c->base + IE_OFFSET);
+
+	/* Erase the slave address programmed */
+	tmp = readl(iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
+	writel(tmp, iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+
+	return 0;
+}
+
 static const struct of_device_id bcm_iproc_i2c_of_match[] = {
 	{ .compatible = "brcm,iproc-i2c" },
 	{ /* sentinel */ }
-- 
2.17.1

[PATCH v5 3/8] dt-bindings: i2c: iproc: make 'interrupts' optional

[Ray Jui]

Update the binding document to make the 'interrupts' property optional.
For certain revisions of the I2C controller (e.g., iProc NIC I2C), I2C
interrupt is unwired to the interrupt controller. In such case, this
'interrupts' property should be left unspecified, and driver will fall
back to polling mode

Signed-off-by: Ray Jui <ray.jui@broadcom.com>
Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
---
 .../devicetree/bindings/i2c/brcm,iproc-i2c.txt        | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt b/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
index 81f982ccca31..7a32bf81bfa9 100644
--- a/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
@@ -9,9 +9,6 @@ Required properties:
     Define the base and range of the I/O address space that contain the iProc
     I2C controller registers
 
-- interrupts:
-    Should contain the I2C interrupt
-
 - clock-frequency:
     This is the I2C bus clock. Need to be either 100000 or 400000
 
@@ -21,6 +18,14 @@ Required properties:
 - #size-cells:
     Always 0
 
+Optional properties:
+
+- interrupts:
+    Should contain the I2C interrupt. For certain revisions of the I2C
+    controller, I2C interrupt is unwired to the interrupt controller. In such
+    case, this property should be left unspecified, and driver will fall back
+    to polling mode
+
 Example:
 	i2c0: i2c@18008000 {
 		compatible = "brcm,iproc-i2c";
-- 
2.17.1

[PATCH v5 4/8] i2c: iproc: add polling support

[Ray Jui]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Add polling support to the iProc I2C driver. Polling mode is
activated when the driver fails to obtain an interrupt ID from device
tree

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 298 ++++++++++++++++++-----------
 1 file changed, 181 insertions(+), 117 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 775b3d89a9c4..073e5a8888ad 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -373,95 +373,115 @@ static void bcm_iproc_i2c_read_valid_bytes(struct bcm_iproc_i2c_dev *iproc_i2c)
 	}
 }
 
-static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
+static void bcm_iproc_i2c_send(struct bcm_iproc_i2c_dev *iproc_i2c)
 {
-	struct bcm_iproc_i2c_dev *iproc_i2c = data;
-	u32 status = readl(iproc_i2c->base + IS_OFFSET);
-	u32 tmp;
-
-
-	bool ret;
-	u32 sl_status = status & ISR_MASK_SLAVE;
-
-	if (sl_status) {
-		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, sl_status);
-		if (ret)
-			return IRQ_HANDLED;
-		else
-			return IRQ_NONE;
-	}
+	struct i2c_msg *msg = iproc_i2c->msg;
+	unsigned int tx_bytes = msg->len - iproc_i2c->tx_bytes;
+	unsigned int i;
+	u32 val;
 
-	status &= ISR_MASK;
+	/* can only fill up to the FIFO size */
+	tx_bytes = min_t(unsigned int, tx_bytes, M_TX_RX_FIFO_SIZE);
+	for (i = 0; i < tx_bytes; i++) {
+		/* start from where we left over */
+		unsigned int idx = iproc_i2c->tx_bytes + i;
 
-	if (!status)
-		return IRQ_NONE;
+		val = msg->buf[idx];
 
-	/* TX FIFO is empty and we have more data to send */
-	if (status & BIT(IS_M_TX_UNDERRUN_SHIFT)) {
-		struct i2c_msg *msg = iproc_i2c->msg;
-		unsigned int tx_bytes = msg->len - iproc_i2c->tx_bytes;
-		unsigned int i;
-		u32 val;
-
-		/* can only fill up to the FIFO size */
-		tx_bytes = min_t(unsigned int, tx_bytes, M_TX_RX_FIFO_SIZE);
-		for (i = 0; i < tx_bytes; i++) {
-			/* start from where we left over */
-			unsigned int idx = iproc_i2c->tx_bytes + i;
+		/* mark the last byte */
+		if (idx == msg->len - 1) {
+			val |= BIT(M_TX_WR_STATUS_SHIFT);
 
-			val = msg->buf[idx];
-
-			/* mark the last byte */
-			if (idx == msg->len - 1) {
-				val |= BIT(M_TX_WR_STATUS_SHIFT);
+			if (iproc_i2c->irq) {
+				u32 tmp;
 
 				/*
-				 * Since this is the last byte, we should
-				 * now disable TX FIFO underrun interrupt
+				 * Since this is the last byte, we should now
+				 * disable TX FIFO underrun interrupt
 				 */
 				tmp = readl(iproc_i2c->base + IE_OFFSET);
 				tmp &= ~BIT(IE_M_TX_UNDERRUN_SHIFT);
 				writel(tmp, iproc_i2c->base + IE_OFFSET);
 			}
-
-			/* load data into TX FIFO */
-			writel(val, iproc_i2c->base + M_TX_OFFSET);
 		}
-		/* update number of transferred bytes */
-		iproc_i2c->tx_bytes += tx_bytes;
+
+		/* load data into TX FIFO */
+		writel(val, iproc_i2c->base + M_TX_OFFSET);
 	}
 
-	if (status & BIT(IS_M_RX_THLD_SHIFT)) {
-		struct i2c_msg *msg = iproc_i2c->msg;
-		u32 bytes_left;
+	/* update number of transferred bytes */
+	iproc_i2c->tx_bytes += tx_bytes;
+}
 
-		bcm_iproc_i2c_read_valid_bytes(iproc_i2c);
-		bytes_left = msg->len - iproc_i2c->rx_bytes;
-		if (bytes_left == 0) {
+static void bcm_iproc_i2c_read(struct bcm_iproc_i2c_dev *iproc_i2c)
+{
+	struct i2c_msg *msg = iproc_i2c->msg;
+	u32 bytes_left, val;
+
+	bcm_iproc_i2c_read_valid_bytes(iproc_i2c);
+	bytes_left = msg->len - iproc_i2c->rx_bytes;
+	if (bytes_left == 0) {
+		if (iproc_i2c->irq) {
 			/* finished reading all data, disable rx thld event */
-			tmp = readl(iproc_i2c->base + IE_OFFSET);
-			tmp &= ~BIT(IS_M_RX_THLD_SHIFT);
-			writel(tmp, iproc_i2c->base + IE_OFFSET);
-		} else if (bytes_left < iproc_i2c->thld_bytes) {
-			/* set bytes left as threshold */
-			tmp = readl(iproc_i2c->base + M_FIFO_CTRL_OFFSET);
-			tmp &= ~(M_FIFO_RX_THLD_MASK << M_FIFO_RX_THLD_SHIFT);
-			tmp |= (bytes_left << M_FIFO_RX_THLD_SHIFT);
-			writel(tmp, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
-			iproc_i2c->thld_bytes = bytes_left;
+			val = readl(iproc_i2c->base + IE_OFFSET);
+			val &= ~BIT(IS_M_RX_THLD_SHIFT);
+			writel(val, iproc_i2c->base + IE_OFFSET);
 		}
-		/*
-		 * bytes_left >= iproc_i2c->thld_bytes,
-		 * hence no need to change the THRESHOLD SET.
-		 * It will remain as iproc_i2c->thld_bytes itself
-		 */
+	} else if (bytes_left < iproc_i2c->thld_bytes) {
+		/* set bytes left as threshold */
+		val = readl(iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		val &= ~(M_FIFO_RX_THLD_MASK << M_FIFO_RX_THLD_SHIFT);
+		val |= (bytes_left << M_FIFO_RX_THLD_SHIFT);
+		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		iproc_i2c->thld_bytes = bytes_left;
 	}
+	/*
+	 * bytes_left >= iproc_i2c->thld_bytes,
+	 * hence no need to change the THRESHOLD SET.
+	 * It will remain as iproc_i2c->thld_bytes itself
+	 */
+}
 
+static void bcm_iproc_i2c_process_m_event(struct bcm_iproc_i2c_dev *iproc_i2c,
+					  u32 status)
+{
+	/* TX FIFO is empty and we have more data to send */
+	if (status & BIT(IS_M_TX_UNDERRUN_SHIFT))
+		bcm_iproc_i2c_send(iproc_i2c);
+
+	/* RX FIFO threshold is reached and data needs to be read out */
+	if (status & BIT(IS_M_RX_THLD_SHIFT))
+		bcm_iproc_i2c_read(iproc_i2c);
+
+	/* transfer is done */
 	if (status & BIT(IS_M_START_BUSY_SHIFT)) {
 		iproc_i2c->xfer_is_done = 1;
-		complete(&iproc_i2c->done);
+		if (iproc_i2c->irq)
+			complete(&iproc_i2c->done);
+	}
+}
+
+static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
+{
+	struct bcm_iproc_i2c_dev *iproc_i2c = data;
+	u32 status = readl(iproc_i2c->base + IS_OFFSET);
+	bool ret;
+	u32 sl_status = status & ISR_MASK_SLAVE;
+
+	if (sl_status) {
+		ret = bcm_iproc_i2c_slave_isr(iproc_i2c, sl_status);
+		if (ret)
+			return IRQ_HANDLED;
+		else
+			return IRQ_NONE;
 	}
 
+	status &= ISR_MASK;
+	if (!status)
+		return IRQ_NONE;
+
+	/* process all master based events */
+	bcm_iproc_i2c_process_m_event(iproc_i2c, status);
 	writel(status, iproc_i2c->base + IS_OFFSET);
 
 	return IRQ_HANDLED;
@@ -560,14 +580,71 @@ static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 	}
 }
 
+static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
+				   struct i2c_msg *msg,
+				   u32 cmd)
+{
+	unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MSEC);
+	u32 val, status;
+	int ret;
+
+	writel(cmd, iproc_i2c->base + M_CMD_OFFSET);
+
+	if (iproc_i2c->irq) {
+		time_left = wait_for_completion_timeout(&iproc_i2c->done,
+							time_left);
+		/* disable all interrupts */
+		writel(0, iproc_i2c->base + IE_OFFSET);
+		/* read it back to flush the write */
+		readl(iproc_i2c->base + IE_OFFSET);
+		/* make sure the interrupt handler isn't running */
+		synchronize_irq(iproc_i2c->irq);
+
+	} else { /* polling mode */
+		unsigned long timeout = jiffies + time_left;
+
+		do {
+			status = readl(iproc_i2c->base + IS_OFFSET) & ISR_MASK;
+			bcm_iproc_i2c_process_m_event(iproc_i2c, status);
+			writel(status, iproc_i2c->base + IS_OFFSET);
+
+			if (time_after(jiffies, timeout)) {
+				time_left = 0;
+				break;
+			}
+
+			cpu_relax();
+			cond_resched();
+		} while (!iproc_i2c->xfer_is_done);
+	}
+
+	if (!time_left && !iproc_i2c->xfer_is_done) {
+		dev_err(iproc_i2c->device, "transaction timed out\n");
+
+		/* flush both TX/RX FIFOs */
+		val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
+		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		return -ETIMEDOUT;
+	}
+
+	ret = bcm_iproc_i2c_check_status(iproc_i2c, msg);
+	if (ret) {
+		/* flush both TX/RX FIFOs */
+		val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
+		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 					 struct i2c_msg *msg)
 {
-	int ret, i;
+	int i;
 	u8 addr;
 	u32 val, tmp, val_intr_en;
 	unsigned int tx_bytes;
-	unsigned long time_left = msecs_to_jiffies(I2C_TIMEOUT_MSEC);
 
 	/* check if bus is busy */
 	if (!!(readl(iproc_i2c->base + M_CMD_OFFSET) &
@@ -602,7 +679,9 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	}
 
 	/* mark as incomplete before starting the transaction */
-	reinit_completion(&iproc_i2c->done);
+	if (iproc_i2c->irq)
+		reinit_completion(&iproc_i2c->done);
+
 	iproc_i2c->xfer_is_done = 0;
 
 	/*
@@ -647,39 +726,11 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	} else {
 		val |= (M_CMD_PROTOCOL_BLK_WR << M_CMD_PROTOCOL_SHIFT);
 	}
-	writel(val_intr_en, iproc_i2c->base + IE_OFFSET);
-	writel(val, iproc_i2c->base + M_CMD_OFFSET);
 
-	time_left = wait_for_completion_timeout(&iproc_i2c->done, time_left);
+	if (iproc_i2c->irq)
+		writel(val_intr_en, iproc_i2c->base + IE_OFFSET);
 
-	/* disable all interrupts */
-	writel(0, iproc_i2c->base + IE_OFFSET);
-	/* read it back to flush the write */
-	readl(iproc_i2c->base + IE_OFFSET);
-
-	/* make sure the interrupt handler isn't running */
-	synchronize_irq(iproc_i2c->irq);
-
-	if (!time_left && !iproc_i2c->xfer_is_done) {
-		dev_err(iproc_i2c->device, "transaction timed out\n");
-
-		/* flush FIFOs */
-		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
-		      (1 << M_FIFO_TX_FLUSH_SHIFT);
-		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
-		return -ETIMEDOUT;
-	}
-
-	ret = bcm_iproc_i2c_check_status(iproc_i2c, msg);
-	if (ret) {
-		/* flush both TX/RX FIFOs */
-		val = (1 << M_FIFO_RX_FLUSH_SHIFT) |
-		      (1 << M_FIFO_TX_FLUSH_SHIFT);
-		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
-		return ret;
-	}
-
-	return 0;
+	return bcm_iproc_i2c_xfer_wait(iproc_i2c, msg, val);
 }
 
 static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
@@ -781,17 +832,20 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 		return ret;
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq <= 0) {
-		dev_err(iproc_i2c->device, "no irq resource\n");
-		return irq;
-	}
-	iproc_i2c->irq = irq;
+	if (irq > 0) {
+		ret = devm_request_irq(iproc_i2c->device, irq,
+				       bcm_iproc_i2c_isr, 0, pdev->name,
+				       iproc_i2c);
+		if (ret < 0) {
+			dev_err(iproc_i2c->device,
+				"unable to request irq %i\n", irq);
+			return ret;
+		}
 
-	ret = devm_request_irq(iproc_i2c->device, irq, bcm_iproc_i2c_isr, 0,
-			       pdev->name, iproc_i2c);
-	if (ret < 0) {
-		dev_err(iproc_i2c->device, "unable to request irq %i\n", irq);
-		return ret;
+		iproc_i2c->irq = irq;
+	} else {
+		dev_warn(iproc_i2c->device,
+			 "no irq resource, falling back to poll mode\n");
 	}
 
 	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
@@ -811,10 +865,15 @@ static int bcm_iproc_i2c_remove(struct platform_device *pdev)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = platform_get_drvdata(pdev);
 
-	/* make sure there's no pending interrupt when we remove the adapter */
-	writel(0, iproc_i2c->base + IE_OFFSET);
-	readl(iproc_i2c->base + IE_OFFSET);
-	synchronize_irq(iproc_i2c->irq);
+	if (iproc_i2c->irq) {
+		/*
+		 * Make sure there's no pending interrupt when we remove the
+		 * adapter
+		 */
+		writel(0, iproc_i2c->base + IE_OFFSET);
+		readl(iproc_i2c->base + IE_OFFSET);
+		synchronize_irq(iproc_i2c->irq);
+	}
 
 	i2c_del_adapter(&iproc_i2c->adapter);
 	bcm_iproc_i2c_enable_disable(iproc_i2c, false);
@@ -828,10 +887,15 @@ static int bcm_iproc_i2c_suspend(struct device *dev)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = dev_get_drvdata(dev);
 
-	/* make sure there's no pending interrupt when we go into suspend */
-	writel(0, iproc_i2c->base + IE_OFFSET);
-	readl(iproc_i2c->base + IE_OFFSET);
-	synchronize_irq(iproc_i2c->irq);
+	if (iproc_i2c->irq) {
+		/*
+		 * Make sure there's no pending interrupt when we go into
+		 * suspend
+		 */
+		writel(0, iproc_i2c->base + IE_OFFSET);
+		readl(iproc_i2c->base + IE_OFFSET);
+		synchronize_irq(iproc_i2c->irq);
+	}
 
 	/* now disable the controller */
 	bcm_iproc_i2c_enable_disable(iproc_i2c, false);
-- 
2.17.1

[PATCH v5 5/8] i2c: iproc: use wrapper for read/write access

[Ray Jui]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Use the following wrapper for read/write access of iProc i2c registers:
u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
                     u32 offset)
void iproc_i2c_wr_reg(struct bcm_iproc_i2c_dev *iproc_i2c, u32 offset,
                      u32 val)

This preps the driver for support of indirect register access required
by certain SoCs with this iProc I2C block integrated

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 145 ++++++++++++++++-------------
 1 file changed, 79 insertions(+), 66 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 073e5a8888ad..5b9cbd7a3776 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -212,6 +212,18 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave);
 static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
 					 bool enable);
 
+static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
+				   u32 offset)
+{
+	return readl(iproc_i2c->base + offset);
+}
+
+static inline void iproc_i2c_wr_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
+				    u32 offset, u32 val)
+{
+	writel(val, iproc_i2c->base + offset);
+}
+
 static void bcm_iproc_i2c_slave_init(
 	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
 {
@@ -219,37 +231,37 @@ static void bcm_iproc_i2c_slave_init(
 
 	if (need_reset) {
 		/* put controller in reset */
-		val = readl(iproc_i2c->base + CFG_OFFSET);
+		val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
 		val |= BIT(CFG_RESET_SHIFT);
-		writel(val, iproc_i2c->base + CFG_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 
 		/* wait 100 usec per spec */
 		udelay(100);
 
 		/* bring controller out of reset */
 		val &= ~(BIT(CFG_RESET_SHIFT));
-		writel(val, iproc_i2c->base + CFG_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 	}
 
 	/* flush TX/RX FIFOs */
 	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
-	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, S_FIFO_CTRL_OFFSET, val);
 
 	/* RANDOM SLAVE STRETCH time - 20ms*/
-	val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
 	val &= ~(TIM_RAND_SLAVE_STRETCH_MASK << TIM_RAND_SLAVE_STRETCH_SHIFT);
 	val |= (SLAVE_CLOCK_STRETCH_TIME << TIM_RAND_SLAVE_STRETCH_SHIFT);
-	writel(val, iproc_i2c->base + TIM_CFG_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);
 
 	/* Configure the slave address */
-	val = readl(iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
 	val |= BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
 	val &= ~(S_CFG_NIC_SMB_ADDR3_MASK << S_CFG_NIC_SMB_ADDR3_SHIFT);
 	val |= (iproc_i2c->slave->addr << S_CFG_NIC_SMB_ADDR3_SHIFT);
-	writel(val, iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, val);
 
 	/* clear all pending slave interrupts */
-	writel(ISR_MASK_SLAVE, iproc_i2c->base + IS_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, ISR_MASK_SLAVE);
 
 	/* Enable interrupt register for any READ event */
 	val = BIT(IE_S_RD_EVENT_SHIFT);
@@ -257,7 +269,7 @@ static void bcm_iproc_i2c_slave_init(
 	val |= BIT(IE_S_RX_EVENT_SHIFT);
 	/* Enable interrupt register for the Slave BUSY command */
 	val |= BIT(IE_S_START_BUSY_SHIFT);
-	writel(val, iproc_i2c->base + IE_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 
 	iproc_i2c->xfer_dir = I2C_SLAVE_DIR_NONE;
 }
@@ -267,7 +279,7 @@ static void bcm_iproc_i2c_check_slave_status(
 {
 	u32 val;
 
-	val = readl(iproc_i2c->base + S_CMD_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, S_CMD_OFFSET);
 	val = (val >> S_CMD_STATUS_SHIFT) & S_CMD_STATUS_MASK;
 
 	if (val == S_CMD_STATUS_TIMEOUT) {
@@ -290,7 +302,7 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 	/* Start of transaction. check address and populate the direction */
 	if (iproc_i2c->xfer_dir == I2C_SLAVE_DIR_NONE) {
-		tmp = readl(iproc_i2c->base + S_RX_OFFSET);
+		tmp = iproc_i2c_rd_reg(iproc_i2c, S_RX_OFFSET);
 		rd_status = (tmp >> S_RX_STATUS_SHIFT) & S_RX_STATUS_MASK;
 		/* This condition checks whether the request is a new request */
 		if (((rd_status == I2C_SLAVE_RX_START) &&
@@ -316,16 +328,16 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 		(iproc_i2c->xfer_dir == I2C_SLAVE_DIR_WRITE)) {
 		i2c_slave_event(iproc_i2c->slave,
 			I2C_SLAVE_READ_PROCESSED, &value);
-		writel(value, iproc_i2c->base + S_TX_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, S_TX_OFFSET, value);
 
 		val = BIT(S_CMD_START_BUSY_SHIFT);
-		writel(val, iproc_i2c->base + S_CMD_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, S_CMD_OFFSET, val);
 	}
 
 	/* write request from master */
 	if ((status & BIT(IS_S_RX_EVENT_SHIFT)) &&
 		(iproc_i2c->xfer_dir == I2C_SLAVE_DIR_READ)) {
-		val = readl(iproc_i2c->base + S_RX_OFFSET);
+		val = iproc_i2c_rd_reg(iproc_i2c, S_RX_OFFSET);
 		/* Its a write request by Master to Slave.
 		 * We read data present in receive FIFO
 		 */
@@ -348,7 +360,7 @@ static bool bcm_iproc_i2c_slave_isr(struct bcm_iproc_i2c_dev *iproc_i2c,
 	}
 
 	/* clear interrupt status */
-	writel(status, iproc_i2c->base + IS_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, status);
 
 	bcm_iproc_i2c_check_slave_status(iproc_i2c);
 	return true;
@@ -360,14 +372,13 @@ static void bcm_iproc_i2c_read_valid_bytes(struct bcm_iproc_i2c_dev *iproc_i2c)
 
 	/* Read valid data from RX FIFO */
 	while (iproc_i2c->rx_bytes < msg->len) {
-		if (!((readl(iproc_i2c->base +
-		M_FIFO_CTRL_OFFSET) >>
-		M_FIFO_RX_CNT_SHIFT) &
-		M_FIFO_RX_CNT_MASK))
+		if (!((iproc_i2c_rd_reg(iproc_i2c,
+					M_FIFO_CTRL_OFFSET) >>
+		       M_FIFO_RX_CNT_SHIFT) & M_FIFO_RX_CNT_MASK))
 			break;
 
 		msg->buf[iproc_i2c->rx_bytes] =
-			(readl(iproc_i2c->base + M_RX_OFFSET) >>
+			(iproc_i2c_rd_reg(iproc_i2c, M_RX_OFFSET) >>
 			M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
 		iproc_i2c->rx_bytes++;
 	}
@@ -399,14 +410,15 @@ static void bcm_iproc_i2c_send(struct bcm_iproc_i2c_dev *iproc_i2c)
 				 * Since this is the last byte, we should now
 				 * disable TX FIFO underrun interrupt
 				 */
-				tmp = readl(iproc_i2c->base + IE_OFFSET);
+				tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 				tmp &= ~BIT(IE_M_TX_UNDERRUN_SHIFT);
-				writel(tmp, iproc_i2c->base + IE_OFFSET);
+				iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET,
+						 tmp);
 			}
 		}
 
 		/* load data into TX FIFO */
-		writel(val, iproc_i2c->base + M_TX_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 	}
 
 	/* update number of transferred bytes */
@@ -423,16 +435,16 @@ static void bcm_iproc_i2c_read(struct bcm_iproc_i2c_dev *iproc_i2c)
 	if (bytes_left == 0) {
 		if (iproc_i2c->irq) {
 			/* finished reading all data, disable rx thld event */
-			val = readl(iproc_i2c->base + IE_OFFSET);
+			val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 			val &= ~BIT(IS_M_RX_THLD_SHIFT);
-			writel(val, iproc_i2c->base + IE_OFFSET);
+			iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 		}
 	} else if (bytes_left < iproc_i2c->thld_bytes) {
 		/* set bytes left as threshold */
-		val = readl(iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		val = iproc_i2c_rd_reg(iproc_i2c, M_FIFO_CTRL_OFFSET);
 		val &= ~(M_FIFO_RX_THLD_MASK << M_FIFO_RX_THLD_SHIFT);
 		val |= (bytes_left << M_FIFO_RX_THLD_SHIFT);
-		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
 		iproc_i2c->thld_bytes = bytes_left;
 	}
 	/*
@@ -464,7 +476,7 @@ static void bcm_iproc_i2c_process_m_event(struct bcm_iproc_i2c_dev *iproc_i2c,
 static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 {
 	struct bcm_iproc_i2c_dev *iproc_i2c = data;
-	u32 status = readl(iproc_i2c->base + IS_OFFSET);
+	u32 status = iproc_i2c_rd_reg(iproc_i2c, IS_OFFSET);
 	bool ret;
 	u32 sl_status = status & ISR_MASK_SLAVE;
 
@@ -482,7 +494,7 @@ static irqreturn_t bcm_iproc_i2c_isr(int irq, void *data)
 
 	/* process all master based events */
 	bcm_iproc_i2c_process_m_event(iproc_i2c, status);
-	writel(status, iproc_i2c->base + IS_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, status);
 
 	return IRQ_HANDLED;
 }
@@ -492,29 +504,29 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
 	u32 val;
 
 	/* put controller in reset */
-	val = readl(iproc_i2c->base + CFG_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
 	val |= BIT(CFG_RESET_SHIFT);
 	val &= ~(BIT(CFG_EN_SHIFT));
-	writel(val, iproc_i2c->base + CFG_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 
 	/* wait 100 usec per spec */
 	udelay(100);
 
 	/* bring controller out of reset */
 	val &= ~(BIT(CFG_RESET_SHIFT));
-	writel(val, iproc_i2c->base + CFG_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 
 	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
 	val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
-	writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
 	/* disable all interrupts */
-	val = readl(iproc_i2c->base + IE_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 	val &= ~(IE_M_ALL_INTERRUPT_MASK <<
 			IE_M_ALL_INTERRUPT_SHIFT);
-	writel(val, iproc_i2c->base + IE_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val);
 
 	/* clear all pending interrupts */
-	writel(0xffffffff, iproc_i2c->base + IS_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, 0xffffffff);
 
 	return 0;
 }
@@ -524,12 +536,12 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
 {
 	u32 val;
 
-	val = readl(iproc_i2c->base + CFG_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, CFG_OFFSET);
 	if (enable)
 		val |= BIT(CFG_EN_SHIFT);
 	else
 		val &= ~BIT(CFG_EN_SHIFT);
-	writel(val, iproc_i2c->base + CFG_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, CFG_OFFSET, val);
 }
 
 static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
@@ -537,7 +549,7 @@ static int bcm_iproc_i2c_check_status(struct bcm_iproc_i2c_dev *iproc_i2c,
 {
 	u32 val;
 
-	val = readl(iproc_i2c->base + M_CMD_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, M_CMD_OFFSET);
 	val = (val >> M_CMD_STATUS_SHIFT) & M_CMD_STATUS_MASK;
 
 	switch (val) {
@@ -588,15 +600,15 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
 	u32 val, status;
 	int ret;
 
-	writel(cmd, iproc_i2c->base + M_CMD_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, M_CMD_OFFSET, cmd);
 
 	if (iproc_i2c->irq) {
 		time_left = wait_for_completion_timeout(&iproc_i2c->done,
 							time_left);
 		/* disable all interrupts */
-		writel(0, iproc_i2c->base + IE_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, 0);
 		/* read it back to flush the write */
-		readl(iproc_i2c->base + IE_OFFSET);
+		iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 		/* make sure the interrupt handler isn't running */
 		synchronize_irq(iproc_i2c->irq);
 
@@ -604,9 +616,10 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
 		unsigned long timeout = jiffies + time_left;
 
 		do {
-			status = readl(iproc_i2c->base + IS_OFFSET) & ISR_MASK;
+			status = iproc_i2c_rd_reg(iproc_i2c,
+						  IS_OFFSET) & ISR_MASK;
 			bcm_iproc_i2c_process_m_event(iproc_i2c, status);
-			writel(status, iproc_i2c->base + IS_OFFSET);
+			iproc_i2c_wr_reg(iproc_i2c, IS_OFFSET, status);
 
 			if (time_after(jiffies, timeout)) {
 				time_left = 0;
@@ -623,7 +636,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);
-		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
 		return -ETIMEDOUT;
 	}
 
@@ -631,7 +644,7 @@ static int bcm_iproc_i2c_xfer_wait(struct bcm_iproc_i2c_dev *iproc_i2c,
 	if (ret) {
 		/* flush both TX/RX FIFOs */
 		val = BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT);
-		writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, val);
 		return ret;
 	}
 
@@ -647,8 +660,8 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	unsigned int tx_bytes;
 
 	/* check if bus is busy */
-	if (!!(readl(iproc_i2c->base + M_CMD_OFFSET) &
-	       BIT(M_CMD_START_BUSY_SHIFT))) {
+	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");
 		return -EBUSY;
 	}
@@ -657,7 +670,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 
 	/* format and load slave address into the TX FIFO */
 	addr = i2c_8bit_addr_from_msg(msg);
-	writel(addr, iproc_i2c->base + M_TX_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, addr);
 
 	/*
 	 * For a write transaction, load data into the TX FIFO. Only allow
@@ -673,7 +686,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 			if (i == msg->len - 1)
 				val |= 1 << M_TX_WR_STATUS_SHIFT;
 
-			writel(val, iproc_i2c->base + M_TX_OFFSET);
+			iproc_i2c_wr_reg(iproc_i2c, M_TX_OFFSET, val);
 		}
 		iproc_i2c->tx_bytes = tx_bytes;
 	}
@@ -713,10 +726,10 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 			iproc_i2c->thld_bytes = msg->len;
 
 		/* set threshold value */
-		tmp = readl(iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		tmp = iproc_i2c_rd_reg(iproc_i2c, M_FIFO_CTRL_OFFSET);
 		tmp &= ~(M_FIFO_RX_THLD_MASK << M_FIFO_RX_THLD_SHIFT);
 		tmp |= iproc_i2c->thld_bytes << M_FIFO_RX_THLD_SHIFT;
-		writel(tmp, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, M_FIFO_CTRL_OFFSET, tmp);
 
 		/* enable the RX threshold interrupt */
 		val_intr_en |= BIT(IE_M_RX_THLD_SHIFT);
@@ -728,7 +741,7 @@ static int bcm_iproc_i2c_xfer_single_msg(struct bcm_iproc_i2c_dev *iproc_i2c,
 	}
 
 	if (iproc_i2c->irq)
-		writel(val_intr_en, iproc_i2c->base + IE_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, val_intr_en);
 
 	return bcm_iproc_i2c_xfer_wait(iproc_i2c, msg, val);
 }
@@ -792,10 +805,10 @@ static int bcm_iproc_i2c_cfg_speed(struct bcm_iproc_i2c_dev *iproc_i2c)
 	}
 
 	iproc_i2c->bus_speed = bus_speed;
-	val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
 	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
 	val |= (bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
-	writel(val, iproc_i2c->base + TIM_CFG_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);
 
 	dev_info(iproc_i2c->device, "bus set to %u Hz\n", bus_speed);
 
@@ -870,8 +883,8 @@ static int bcm_iproc_i2c_remove(struct platform_device *pdev)
 		 * Make sure there's no pending interrupt when we remove the
 		 * adapter
 		 */
-		writel(0, iproc_i2c->base + IE_OFFSET);
-		readl(iproc_i2c->base + IE_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, 0);
+		iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 		synchronize_irq(iproc_i2c->irq);
 	}
 
@@ -892,8 +905,8 @@ static int bcm_iproc_i2c_suspend(struct device *dev)
 		 * Make sure there's no pending interrupt when we go into
 		 * suspend
 		 */
-		writel(0, iproc_i2c->base + IE_OFFSET);
-		readl(iproc_i2c->base + IE_OFFSET);
+		iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, 0);
+		iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 		synchronize_irq(iproc_i2c->irq);
 	}
 
@@ -918,10 +931,10 @@ static int bcm_iproc_i2c_resume(struct device *dev)
 		return ret;
 
 	/* configure to the desired bus speed */
-	val = readl(iproc_i2c->base + TIM_CFG_OFFSET);
+	val = iproc_i2c_rd_reg(iproc_i2c, TIM_CFG_OFFSET);
 	val &= ~(1 << TIM_CFG_MODE_400_SHIFT);
 	val |= (iproc_i2c->bus_speed == 400000) << TIM_CFG_MODE_400_SHIFT;
-	writel(val, iproc_i2c->base + TIM_CFG_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, TIM_CFG_OFFSET, val);
 
 	bcm_iproc_i2c_enable_disable(iproc_i2c, true);
 
@@ -965,15 +978,15 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
 	iproc_i2c->slave = NULL;
 
 	/* disable all slave interrupts */
-	tmp = readl(iproc_i2c->base + IE_OFFSET);
+	tmp = iproc_i2c_rd_reg(iproc_i2c, IE_OFFSET);
 	tmp &= ~(IE_S_ALL_INTERRUPT_MASK <<
 			IE_S_ALL_INTERRUPT_SHIFT);
-	writel(tmp, iproc_i2c->base + IE_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, IE_OFFSET, tmp);
 
 	/* Erase the slave address programmed */
-	tmp = readl(iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+	tmp = iproc_i2c_rd_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET);
 	tmp &= ~BIT(S_CFG_EN_NIC_SMB_ADDR3_SHIFT);
-	writel(tmp, iproc_i2c->base + S_CFG_SMBUS_ADDR_OFFSET);
+	iproc_i2c_wr_reg(iproc_i2c, S_CFG_SMBUS_ADDR_OFFSET, tmp);
 
 	return 0;
 }
-- 
2.17.1

[PATCH v5 6/8] dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string

[Ray Jui]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Update iProc I2C binding document to add new compatible string
"brcm,iproc-nic-i2c". Optional property "brcm,ape-hsls-addr-mask" is
also added that allows configuration of the host view into the APE's
address for "brcm,iproc-nic-i2c"

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt b/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
index 7a32bf81bfa9..d12cc33cca6c 100644
--- a/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt
@@ -3,7 +3,7 @@ Broadcom iProc I2C controller
 Required properties:
 
 - compatible:
-    Must be "brcm,iproc-i2c"
+    Must be "brcm,iproc-i2c" or "brcm,iproc-nic-i2c"
 
 - reg:
     Define the base and range of the I/O address space that contain the iProc
@@ -26,6 +26,10 @@ Optional properties:
     case, this property should be left unspecified, and driver will fall back
     to polling mode
 
+- brcm,ape-hsls-addr-mask:
+    Required for "brcm,iproc-nic-i2c". Host view of address mask into the
+    'APE' co-processor. Value must be unsigned, 32-bit
+
 Example:
 	i2c0: i2c@18008000 {
 		compatible = "brcm,iproc-i2c";
-- 
2.17.1

[PATCH v5 7/8] i2c: iproc: add NIC I2C support

[Ray Jui]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Add NIC I2C support to the iProc I2C driver. Access to the NIC I2C base
registers requires going through the IDM wrapper to map into the NIC's
address space

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 drivers/i2c/busses/i2c-bcm-iproc.c | 79 ++++++++++++++++++++++++++++--
 1 file changed, 75 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-bcm-iproc.c b/drivers/i2c/busses/i2c-bcm-iproc.c
index 5b9cbd7a3776..9598e1d0048e 100644
--- a/drivers/i2c/busses/i2c-bcm-iproc.c
+++ b/drivers/i2c/busses/i2c-bcm-iproc.c
@@ -17,9 +17,11 @@
 #include <linux/io.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 
+#define IDM_CTRL_DIRECT_OFFSET       0x00
 #define CFG_OFFSET                   0x00
 #define CFG_RESET_SHIFT              31
 #define CFG_EN_SHIFT                 30
@@ -174,11 +176,26 @@ enum bus_speed_index {
 	I2C_SPD_400K,
 };
 
+enum bcm_iproc_i2c_type {
+	IPROC_I2C,
+	IPROC_I2C_NIC
+};
+
 struct bcm_iproc_i2c_dev {
 	struct device *device;
+	enum bcm_iproc_i2c_type type;
 	int irq;
 
 	void __iomem *base;
+	void __iomem *idm_base;
+
+	u32 ape_addr_mask;
+
+	/* lock for indirect access through IDM */
+	spinlock_t idm_lock;
+
+	/* indicates no slave mode support */
+	bool no_slave;
 
 	struct i2c_adapter adapter;
 	unsigned int bus_speed;
@@ -215,13 +232,33 @@ static void bcm_iproc_i2c_enable_disable(struct bcm_iproc_i2c_dev *iproc_i2c,
 static inline u32 iproc_i2c_rd_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
 				   u32 offset)
 {
-	return readl(iproc_i2c->base + offset);
+	u32 val;
+
+	if (iproc_i2c->idm_base) {
+		spin_lock(&iproc_i2c->idm_lock);
+		writel(iproc_i2c->ape_addr_mask,
+		       iproc_i2c->idm_base + IDM_CTRL_DIRECT_OFFSET);
+		val = readl(iproc_i2c->base + offset);
+		spin_unlock(&iproc_i2c->idm_lock);
+	} else {
+		val = readl(iproc_i2c->base + offset);
+	}
+
+	return val;
 }
 
 static inline void iproc_i2c_wr_reg(struct bcm_iproc_i2c_dev *iproc_i2c,
 				    u32 offset, u32 val)
 {
-	writel(val, iproc_i2c->base + offset);
+	if (iproc_i2c->idm_base) {
+		spin_lock(&iproc_i2c->idm_lock);
+		writel(iproc_i2c->ape_addr_mask,
+		       iproc_i2c->idm_base + IDM_CTRL_DIRECT_OFFSET);
+		writel(val, iproc_i2c->base + offset);
+		spin_unlock(&iproc_i2c->idm_lock);
+	} else {
+		writel(val, iproc_i2c->base + offset);
+	}
 }
 
 static void bcm_iproc_i2c_slave_init(
@@ -766,7 +803,13 @@ static int bcm_iproc_i2c_xfer(struct i2c_adapter *adapter,
 
 static uint32_t bcm_iproc_i2c_functionality(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | I2C_FUNC_SLAVE;
+	struct bcm_iproc_i2c_dev *iproc_i2c = i2c_get_adapdata(adap);
+	u32 val = I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+
+	if (!iproc_i2c->no_slave)
+		val |= I2C_FUNC_SLAVE;
+
+	return val;
 }
 
 static const struct i2c_algorithm bcm_iproc_algo = {
@@ -829,6 +872,8 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 
 	platform_set_drvdata(pdev, iproc_i2c);
 	iproc_i2c->device = &pdev->dev;
+	iproc_i2c->type =
+		(enum bcm_iproc_i2c_type) of_device_get_match_data(&pdev->dev);
 	init_completion(&iproc_i2c->done);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -836,6 +881,26 @@ static int bcm_iproc_i2c_probe(struct platform_device *pdev)
 	if (IS_ERR(iproc_i2c->base))
 		return PTR_ERR(iproc_i2c->base);
 
+	if (iproc_i2c->type == IPROC_I2C_NIC) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+		iproc_i2c->idm_base = devm_ioremap_resource(iproc_i2c->device,
+							    res);
+		if (IS_ERR(iproc_i2c->idm_base))
+			return PTR_ERR(iproc_i2c->idm_base);
+
+		ret = of_property_read_u32(iproc_i2c->device->of_node,
+					   "brcm,ape-hsls-addr-mask",
+					   &iproc_i2c->ape_addr_mask);
+		if (ret < 0) {
+			dev_err(iproc_i2c->device,
+				"'brcm,ape-hsls-addr-mask' missing\n");
+			return -EINVAL;
+		}
+
+		spin_lock_init(&iproc_i2c->idm_lock);
+		iproc_i2c->no_slave = true;
+	}
+
 	ret = bcm_iproc_i2c_init(iproc_i2c);
 	if (ret)
 		return ret;
@@ -992,7 +1057,13 @@ static int bcm_iproc_i2c_unreg_slave(struct i2c_client *slave)
 }
 
 static const struct of_device_id bcm_iproc_i2c_of_match[] = {
-	{ .compatible = "brcm,iproc-i2c" },
+	{
+		.compatible = "brcm,iproc-i2c",
+		.data = (int *)IPROC_I2C,
+	}, {
+		.compatible = "brcm,iproc-nic-i2c",
+		.data = (int *)IPROC_I2C_NIC,
+	},
 	{ /* sentinel */ }
 };
 MODULE_DEVICE_TABLE(of, bcm_iproc_i2c_of_match);
-- 
2.17.1

[PATCH v5 8/8] arm64: dts: Stingray: Add NIC i2c device node

[Ray Jui]

From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>

Add NIC i2c device node.

Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
Signed-off-by: Ray Jui <ray.jui@broadcom.com>
---
 .../boot/dts/broadcom/stingray/stingray.dtsi   | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
index cfeaa855bd05..67e53768a84a 100644
--- a/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
+++ b/arch/arm64/boot/dts/broadcom/stingray/stingray.dtsi
@@ -612,4 +612,22 @@
 			status = "disabled";
 		};
 	};
+
+	nic-hsls {
+		compatible = "simple-bus";
+		#address-cells = <1>;
+		#size-cells = <1>;
+		ranges = <0x0 0x0  0x0 0x7fffffff>;
+
+		nic_i2c0: i2c@60826100 {
+			compatible = "brcm,iproc-nic-i2c";
+			#address-cells = <1>;
+			#size-cells = <0>;
+			reg = <0x60826100 0x100>,
+			      <0x60e00408 0x1000>;
+			brcm,ape-hsls-addr-mask = <0x03400000>;
+			clock-frequency = <100000>;
+			status = "disabled";
+		};
+	};
 };
-- 
2.17.1

Re: [PATCH v5 3/8] dt-bindings: i2c: iproc: make 'interrupts' optional

[Rob Herring]

On Thu, Feb 14, 2019 at 11:57 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
> Update the binding document to make the 'interrupts' property optional.
> For certain revisions of the I2C controller (e.g., iProc NIC I2C), I2C
> interrupt is unwired to the interrupt controller. In such case, this
> 'interrupts' property should be left unspecified, and driver will fall
> back to polling mode
>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> ---
>  .../devicetree/bindings/i2c/brcm,iproc-i2c.txt        | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 6/8] dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string

[Rob Herring]

On Thu, Feb 14, 2019 at 11:58 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
> From: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
>
> Update iProc I2C binding document to add new compatible string
> "brcm,iproc-nic-i2c". Optional property "brcm,ape-hsls-addr-mask" is
> also added that allows configuration of the host view into the APE's
> address for "brcm,iproc-nic-i2c"
>
> Signed-off-by: Rayagonda Kokatanur <rayagonda.kokatanur@broadcom.com>
> Signed-off-by: Ray Jui <ray.jui@broadcom.com>
> ---
>  Documentation/devicetree/bindings/i2c/brcm,iproc-i2c.txt | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)

Reviewed-by: Rob Herring <robh@kernel.org>

Re: [PATCH v5 0/8] iProc I2C slave mode and NIC mode

[Ray Jui]

Hi Wolfram,

Rob has reviewed all DT binding related changes from this patch series.
Could you please help to review the I2C driver related changes when you
have time?

Thanks,

Ray

On 2/14/2019 9:57 AM, Ray Jui wrote:
> This patch series adds the following support to the iProc I2C driver:
> - Increase maximum read transfer size to 255 bytes
> - I2C slave mode
> - Polling mode
> - NIC I2C mode
> 
> This patch series is based on kernel v5.0-rc3 and available at:
> https://github.com/Broadcom/arm64-linux.git
> branch: i2c-slave-v5
> 
> Changes from v4:
>  - Add more detailed explanations in the device tree binding document
>    changes, to address Rob's review comments
> 
> Changes from v3:
>  - Various minor fixes on commit messages and commits
>  - Rebased to v5.0-rc3
> 
> Changes from v2:
>  - Address Ray's review comments.
> 
> Changes from v1:
>  - Rebased to Linux v5.0.0-rc2
> 
> Ray Jui (1):
>   dt-bindings: i2c: iproc: make 'interrupts' optional
> 
> Rayagonda Kokatanur (5):
>   i2c: iproc: add polling support
>   i2c: iproc: use wrapper for read/write access
>   dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string
>   i2c: iproc: add NIC I2C support
>   arm64: dts: Stingray: Add NIC i2c device node
> 
> Shreesha Rajashekar (2):
>   i2c: iproc: Extend I2C read up to 255 bytes
>   i2c: iproc: Add slave mode support
> 
>  .../bindings/i2c/brcm,iproc-i2c.txt           |  17 +-
>  .../boot/dts/broadcom/stingray/stingray.dtsi  |  18 +
>  drivers/i2c/busses/Kconfig                    |   1 +
>  drivers/i2c/busses/i2c-bcm-iproc.c            | 758 +++++++++++++++---
>  4 files changed, 663 insertions(+), 131 deletions(-)
> 

Re: [PATCH v5 0/8] iProc I2C slave mode and NIC mode

[Florian Fainelli]

On 2/22/19 12:04 PM, Ray Jui wrote:
> Hi Wolfram,
> 
> Rob has reviewed all DT binding related changes from this patch series.
> Could you please help to review the I2C driver related changes when you
> have time?

Wolfram, did you queue those patches yet? The dt-binding patch has been
reviewed by Rob, therefore I am going to queue up patch 8 for the ARM
SoC pull request for 5.2. Let me know if that does not align with when
you plan on sending the i2c changes, thanks!

> 
> Thanks,
> 
> Ray
> 
> On 2/14/2019 9:57 AM, Ray Jui wrote:
>> This patch series adds the following support to the iProc I2C driver:
>> - Increase maximum read transfer size to 255 bytes
>> - I2C slave mode
>> - Polling mode
>> - NIC I2C mode
>>
>> This patch series is based on kernel v5.0-rc3 and available at:
>> https://github.com/Broadcom/arm64-linux.git
>> branch: i2c-slave-v5
>>
>> Changes from v4:
>>  - Add more detailed explanations in the device tree binding document
>>    changes, to address Rob's review comments
>>
>> Changes from v3:
>>  - Various minor fixes on commit messages and commits
>>  - Rebased to v5.0-rc3
>>
>> Changes from v2:
>>  - Address Ray's review comments.
>>
>> Changes from v1:
>>  - Rebased to Linux v5.0.0-rc2
>>
>> Ray Jui (1):
>>   dt-bindings: i2c: iproc: make 'interrupts' optional
>>
>> Rayagonda Kokatanur (5):
>>   i2c: iproc: add polling support
>>   i2c: iproc: use wrapper for read/write access
>>   dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string
>>   i2c: iproc: add NIC I2C support
>>   arm64: dts: Stingray: Add NIC i2c device node
>>
>> Shreesha Rajashekar (2):
>>   i2c: iproc: Extend I2C read up to 255 bytes
>>   i2c: iproc: Add slave mode support
>>
>>  .../bindings/i2c/brcm,iproc-i2c.txt           |  17 +-
>>  .../boot/dts/broadcom/stingray/stingray.dtsi  |  18 +
>>  drivers/i2c/busses/Kconfig                    |   1 +
>>  drivers/i2c/busses/i2c-bcm-iproc.c            | 758 +++++++++++++++---
>>  4 files changed, 663 insertions(+), 131 deletions(-)
>>


-- 
Florian

Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2480 bytes --]


> +static void bcm_iproc_i2c_slave_init(
> +	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
> +{
> +	u32 val;
> +
> +	if (need_reset) {
> +		/* put controller in reset */
> +		val = readl(iproc_i2c->base + CFG_OFFSET);
> +		val |= BIT(CFG_RESET_SHIFT);
> +		writel(val, iproc_i2c->base + CFG_OFFSET);
> +
> +		/* wait 100 usec per spec */
> +		udelay(100);
> +
> +		/* bring controller out of reset */
> +		val &= ~(BIT(CFG_RESET_SHIFT));
> +		writel(val, iproc_i2c->base + CFG_OFFSET);
> +	}
> +
> +	/* flush TX/RX FIFOs */
> +	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
> +	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);

Will flushing FIFOs work when a slave is register while a master
transfer is on-going at the same time?

> +
> +	/* RANDOM SLAVE STRETCH time - 20ms*/

What is a "random stretch time"? 20ms sounds like a lot. Also, missing
space before comment terminator.

> @@ -224,22 +473,25 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
>  
>  	/* put controller in reset */
>  	val = readl(iproc_i2c->base + CFG_OFFSET);
> -	val |= 1 << CFG_RESET_SHIFT;
> -	val &= ~(1 << CFG_EN_SHIFT);
> +	val |= BIT(CFG_RESET_SHIFT);
> +	val &= ~(BIT(CFG_EN_SHIFT));
>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>  
>  	/* wait 100 usec per spec */
>  	udelay(100);
>  
>  	/* bring controller out of reset */
> -	val &= ~(1 << CFG_RESET_SHIFT);
> +	val &= ~(BIT(CFG_RESET_SHIFT));
>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>  
>  	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
> -	val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
> +	val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
>  	writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
>  	/* disable all interrupts */
> -	writel(0, iproc_i2c->base + IE_OFFSET);
> +	val = readl(iproc_i2c->base + IE_OFFSET);
> +	val &= ~(IE_M_ALL_INTERRUPT_MASK <<
> +			IE_M_ALL_INTERRUPT_SHIFT);
> +	writel(val, iproc_i2c->base + IE_OFFSET);

This block looks unrelated, but I won't be too strict here...

> +	case M_CMD_STATUS_FIFO_UNDERRUN:
> +		dev_dbg(iproc_i2c->device, "FIFO under-run\n");
> +		return -ENXIO;
> +
> +	case M_CMD_STATUS_RX_FIFO_FULL:
> +		dev_dbg(iproc_i2c->device, "Master Rx FIFO full > 10ms\n");
> +		return -ETIMEDOUT;
> +

... however, this looks really unrelated to me. This is about master
transmission, or?

Rest looks OK.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/8] i2c: iproc: Extend I2C read up to 255 bytes

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 397 bytes --]


> +		if (!((readl(iproc_i2c->base +
> +		M_FIFO_CTRL_OFFSET) >>
> +		M_FIFO_RX_CNT_SHIFT) &
> +		M_FIFO_RX_CNT_MASK))

Don't be too strict with the 80 char limit. I think the above is hardly
readable...

> +			break;
> +
> +		msg->buf[iproc_i2c->rx_bytes] =
> +			(readl(iproc_i2c->base + M_RX_OFFSET) >>
> +			M_RX_DATA_SHIFT) & M_RX_DATA_MASK;

... this here is MUCH better.

Rest looks good.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 6/8] dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 468 bytes --]


> Update iProc I2C binding document to add new compatible string
> "brcm,iproc-nic-i2c". Optional property "brcm,ape-hsls-addr-mask" is
> also added that allows configuration of the host view into the APE's
> address for "brcm,iproc-nic-i2c"

I don't know the platform, but wouldn't it be more DT-like to describe
the APE in DT and derive the mask from that information? Custom bindings
with values which are directly poked into a register usually raise my
eyebrow.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 0/8] iProc I2C slave mode and NIC mode

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 490 bytes --]

On Thu, Feb 14, 2019 at 09:57:17AM -0800, Ray Jui wrote:
> This patch series adds the following support to the iProc I2C driver:
> - Increase maximum read transfer size to 255 bytes
> - I2C slave mode
> - Polling mode
> - NIC I2C mode
> 
> This patch series is based on kernel v5.0-rc3 and available at:
> https://github.com/Broadcom/arm64-linux.git

So, I had a high level look at these patches and made a few comments. For
the details, I trust you (being the maintainer :))


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 371 bytes --]


> > +	/* flush TX/RX FIFOs */
> > +	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
> > +	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);
> 
> Will flushing FIFOs work when a slave is register while a master
> transfer is on-going at the same time?

Sorry, my bad, this can't happen. Registering a slave is protected by
the adapter lock.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

[Ray Jui]

Hi Wolfram/Rayagonda,

On 3/27/2019 3:14 PM, Wolfram Sang wrote:
> 
>> +static void bcm_iproc_i2c_slave_init(
>> +	struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
>> +{
>> +	u32 val;
>> +
>> +	if (need_reset) {
>> +		/* put controller in reset */
>> +		val = readl(iproc_i2c->base + CFG_OFFSET);
>> +		val |= BIT(CFG_RESET_SHIFT);
>> +		writel(val, iproc_i2c->base + CFG_OFFSET);
>> +
>> +		/* wait 100 usec per spec */
>> +		udelay(100);
>> +
>> +		/* bring controller out of reset */
>> +		val &= ~(BIT(CFG_RESET_SHIFT));
>> +		writel(val, iproc_i2c->base + CFG_OFFSET);
>> +	}
>> +
>> +	/* flush TX/RX FIFOs */
>> +	val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
>> +	writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);
> 
> Will flushing FIFOs work when a slave is register while a master
> transfer is on-going at the same time?
> 

Okay, as you pointed out in a subsequent email, this can't happen.

>> +
>> +	/* RANDOM SLAVE STRETCH time - 20ms*/
> 
> What is a "random stretch time"? 20ms sounds like a lot. Also, missing
> space before comment terminator.
> 

Rayagonda,

Could you please help to comment on the choice of the 20 ms to allow
clock stretch from the slave? In probably all cases, the slave should
not need more than 1 ms? 20 ms does seem way too long as Wolfram pointed
out.

Will fix the missing space before comment terminator.

>> @@ -224,22 +473,25 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
>>  
>>  	/* put controller in reset */
>>  	val = readl(iproc_i2c->base + CFG_OFFSET);
>> -	val |= 1 << CFG_RESET_SHIFT;
>> -	val &= ~(1 << CFG_EN_SHIFT);
>> +	val |= BIT(CFG_RESET_SHIFT);
>> +	val &= ~(BIT(CFG_EN_SHIFT));
>>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>>  
>>  	/* wait 100 usec per spec */
>>  	udelay(100);
>>  
>>  	/* bring controller out of reset */
>> -	val &= ~(1 << CFG_RESET_SHIFT);
>> +	val &= ~(BIT(CFG_RESET_SHIFT));
>>  	writel(val, iproc_i2c->base + CFG_OFFSET);
>>  
>>  	/* flush TX/RX FIFOs and set RX FIFO threshold to zero */
>> -	val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
>> +	val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
>>  	writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
>>  	/* disable all interrupts */
>> -	writel(0, iproc_i2c->base + IE_OFFSET);
>> +	val = readl(iproc_i2c->base + IE_OFFSET);
>> +	val &= ~(IE_M_ALL_INTERRUPT_MASK <<
>> +			IE_M_ALL_INTERRUPT_SHIFT);
>> +	writel(val, iproc_i2c->base + IE_OFFSET);
> 
> This block looks unrelated, but I won't be too strict here...
> 
>> +	case M_CMD_STATUS_FIFO_UNDERRUN:
>> +		dev_dbg(iproc_i2c->device, "FIFO under-run\n");
>> +		return -ENXIO;
>> +
>> +	case M_CMD_STATUS_RX_FIFO_FULL:
>> +		dev_dbg(iproc_i2c->device, "Master Rx FIFO full > 10ms\n");
>> +		return -ETIMEDOUT;
>> +
> 
> ... however, this looks really unrelated to me. This is about master
> transmission, or?

This should be submitted in a separate commit. Will do that in the next
iteration of patch series.

> 
> Rest looks OK.
> 

Thanks,

Ray

Re: [PATCH v5 1/8] i2c: iproc: Extend I2C read up to 255 bytes

[Ray Jui]

On 3/27/2019 3:17 PM, Wolfram Sang wrote:
> 
>> +		if (!((readl(iproc_i2c->base +
>> +		M_FIFO_CTRL_OFFSET) >>
>> +		M_FIFO_RX_CNT_SHIFT) &
>> +		M_FIFO_RX_CNT_MASK))
> 
> Don't be too strict with the 80 char limit. I think the above is hardly
> readable...
> 

Right, that makes sense. Will change to make it more readable than
trying to be compliant to the < 80 chars rule.

>> +			break;
>> +
>> +		msg->buf[iproc_i2c->rx_bytes] =
>> +			(readl(iproc_i2c->base + M_RX_OFFSET) >>
>> +			M_RX_DATA_SHIFT) & M_RX_DATA_MASK;
> 
> ... this here is MUCH better.
> 
> Rest looks good.
> 

Thanks,

Ray

Re: [PATCH v5 6/8] dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string

[Ray Jui]

Hi Wolfram,

On 3/27/2019 3:24 PM, Wolfram Sang wrote:
> 
>> Update iProc I2C binding document to add new compatible string
>> "brcm,iproc-nic-i2c". Optional property "brcm,ape-hsls-addr-mask" is
>> also added that allows configuration of the host view into the APE's
>> address for "brcm,iproc-nic-i2c"
> 
> I don't know the platform, but wouldn't it be more DT-like to describe
> the APE in DT and derive the mask from that information? Custom bindings
> with values which are directly poked into a register usually raise my
> eyebrow.
> 

Note APE is a co-processor that is not visible from the Linux running
from the host processor.

"brcm,iproc-nic-i2c" here is introduced to allow the I2C port from APE
to be completely owned by the host CPU, to meet the requirement of
certain use cases. At the same time, the control of the I2C port from
APE will be disabled.

The "brcm,ape-hsls-addr-mask" defines the address translation and be
programmed into some configuration registers to allow the host to
directly access the I2C registers of APE. Note those configuration
registers are owned by the host, and that address is not APE's address
space nor the host is intending to take over the control of APE.

Therefore, I think it makes way more sense to use an address mask type
of DT property here.

Thanks,

Ray

Re: [PATCH v5 0/8] iProc I2C slave mode and NIC mode

[Ray Jui]

On 3/27/2019 3:27 PM, Wolfram Sang wrote:
> On Thu, Feb 14, 2019 at 09:57:17AM -0800, Ray Jui wrote:
>> This patch series adds the following support to the iProc I2C driver:
>> - Increase maximum read transfer size to 255 bytes
>> - I2C slave mode
>> - Polling mode
>> - NIC I2C mode
>>
>> This patch series is based on kernel v5.0-rc3 and available at:
>> https://github.com/Broadcom/arm64-linux.git
> 
> So, I had a high level look at these patches and made a few comments. For
> the details, I trust you (being the maintainer :))
> 

Thanks for the feedback, Wolfram. We'll reply and address your comments.

Ray

Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

[Rayagonda Kokatanur]

Hi Ray/Wolfram,

On Tue, Apr 2, 2019 at 3:03 AM Ray Jui <ray.jui@broadcom.com> wrote:
>
> Hi Wolfram/Rayagonda,
>
> On 3/27/2019 3:14 PM, Wolfram Sang wrote:
> >
> >> +static void bcm_iproc_i2c_slave_init(
> >> +    struct bcm_iproc_i2c_dev *iproc_i2c, bool need_reset)
> >> +{
> >> +    u32 val;
> >> +
> >> +    if (need_reset) {
> >> +            /* put controller in reset */
> >> +            val = readl(iproc_i2c->base + CFG_OFFSET);
> >> +            val |= BIT(CFG_RESET_SHIFT);
> >> +            writel(val, iproc_i2c->base + CFG_OFFSET);
> >> +
> >> +            /* wait 100 usec per spec */
> >> +            udelay(100);
> >> +
> >> +            /* bring controller out of reset */
> >> +            val &= ~(BIT(CFG_RESET_SHIFT));
> >> +            writel(val, iproc_i2c->base + CFG_OFFSET);
> >> +    }
> >> +
> >> +    /* flush TX/RX FIFOs */
> >> +    val = (BIT(S_FIFO_RX_FLUSH_SHIFT) | BIT(S_FIFO_TX_FLUSH_SHIFT));
> >> +    writel(val, iproc_i2c->base + S_FIFO_CTRL_OFFSET);
> >
> > Will flushing FIFOs work when a slave is register while a master
> > transfer is on-going at the same time?
> >
>
> Okay, as you pointed out in a subsequent email, this can't happen.
>
> >> +
> >> +    /* RANDOM SLAVE STRETCH time - 20ms*/
> >
> > What is a "random stretch time"? 20ms sounds like a lot. Also, missing
> > space before comment terminator.
> >
>
> Rayagonda,
>
> Could you please help to comment on the choice of the 20 ms to allow
> clock stretch from the slave? In probably all cases, the slave should
> not need more than 1 ms? 20 ms does seem way too long as Wolfram pointed
> out.

In fact we are programming max slave stretch time  ie 25ms, comment
should be correcting.
Its maximum time for slave to complete read/write operation, if slave
is done with read/write then clock will not be stretched further, it
will be released immediately.
Hence I feel no harm in programming max timeout value.
Also determining correct slave stretch is very subjective and depends
on slave type as well.

Best regards,
Rayagonda

>
> Will fix the missing space before comment terminator.
>
> >> @@ -224,22 +473,25 @@ static int bcm_iproc_i2c_init(struct bcm_iproc_i2c_dev *iproc_i2c)
> >>
> >>      /* put controller in reset */
> >>      val = readl(iproc_i2c->base + CFG_OFFSET);
> >> -    val |= 1 << CFG_RESET_SHIFT;
> >> -    val &= ~(1 << CFG_EN_SHIFT);
> >> +    val |= BIT(CFG_RESET_SHIFT);
> >> +    val &= ~(BIT(CFG_EN_SHIFT));
> >>      writel(val, iproc_i2c->base + CFG_OFFSET);
> >>
> >>      /* wait 100 usec per spec */
> >>      udelay(100);
> >>
> >>      /* bring controller out of reset */
> >> -    val &= ~(1 << CFG_RESET_SHIFT);
> >> +    val &= ~(BIT(CFG_RESET_SHIFT));
> >>      writel(val, iproc_i2c->base + CFG_OFFSET);
> >>
> >>      /* flush TX/RX FIFOs and set RX FIFO threshold to zero */
> >> -    val = (1 << M_FIFO_RX_FLUSH_SHIFT) | (1 << M_FIFO_TX_FLUSH_SHIFT);
> >> +    val = (BIT(M_FIFO_RX_FLUSH_SHIFT) | BIT(M_FIFO_TX_FLUSH_SHIFT));
> >>      writel(val, iproc_i2c->base + M_FIFO_CTRL_OFFSET);
> >>      /* disable all interrupts */
> >> -    writel(0, iproc_i2c->base + IE_OFFSET);
> >> +    val = readl(iproc_i2c->base + IE_OFFSET);
> >> +    val &= ~(IE_M_ALL_INTERRUPT_MASK <<
> >> +                    IE_M_ALL_INTERRUPT_SHIFT);
> >> +    writel(val, iproc_i2c->base + IE_OFFSET);
> >
> > This block looks unrelated, but I won't be too strict here...
> >
> >> +    case M_CMD_STATUS_FIFO_UNDERRUN:
> >> +            dev_dbg(iproc_i2c->device, "FIFO under-run\n");
> >> +            return -ENXIO;
> >> +
> >> +    case M_CMD_STATUS_RX_FIFO_FULL:
> >> +            dev_dbg(iproc_i2c->device, "Master Rx FIFO full > 10ms\n");
> >> +            return -ETIMEDOUT;
> >> +
> >
> > ... however, this looks really unrelated to me. This is about master
> > transmission, or?
>
> This should be submitted in a separate commit. Will do that in the next
> iteration of patch series.
>
> >
> > Rest looks OK.
> >
>
> Thanks,
>
> Ray

Re: [PATCH v5 2/8] i2c: iproc: Add slave mode support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1001 bytes --]


> > >> +    /* RANDOM SLAVE STRETCH time - 20ms*/
> > >
> > > What is a "random stretch time"? 20ms sounds like a lot. Also, missing
> > > space before comment terminator.
> > >
> >
> > Rayagonda,
> >
> > Could you please help to comment on the choice of the 20 ms to allow
> > clock stretch from the slave? In probably all cases, the slave should
> > not need more than 1 ms? 20 ms does seem way too long as Wolfram pointed
> > out.
> 
> In fact we are programming max slave stretch time  ie 25ms, comment
> should be correcting.
> Its maximum time for slave to complete read/write operation, if slave
> is done with read/write then clock will not be stretched further, it
> will be released immediately.

Ah, now I see. This is a protection against the slave stretching the
clock forever. This makes sense.

> Hence I feel no harm in programming max timeout value.

I agree. "Random stretch" is just a bit confusing as a name. "Maximum"
would have been more clear IMO.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 6/8] dt-bindings: i2c: iproc: add "brcm,iproc-nic-i2c" compatible string

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 895 bytes --]


> Note APE is a co-processor that is not visible from the Linux running
> from the host processor.
> 
> "brcm,iproc-nic-i2c" here is introduced to allow the I2C port from APE
> to be completely owned by the host CPU, to meet the requirement of
> certain use cases. At the same time, the control of the I2C port from
> APE will be disabled.
> 
> The "brcm,ape-hsls-addr-mask" defines the address translation and be
> programmed into some configuration registers to allow the host to
> directly access the I2C registers of APE. Note those configuration
> registers are owned by the host, and that address is not APE's address
> space nor the host is intending to take over the control of APE.
> 
> Therefore, I think it makes way more sense to use an address mask type
> of DT property here.

Thanks for the explanation! Well, since Rob is already OK with it, then
so am I.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 7/8] i2c: iproc: add NIC I2C support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 593 bytes --]


> +#define IDM_CTRL_DIRECT_OFFSET       0x00

And this IDM thing is also never used outside of the I2C context? In
other words, it also doesn't need a seperate DT node?


> +	/* indicates no slave mode support */
> +	bool no_slave;

I would suggest to not use a flag, but to nullify the {un}reg_slave
callbacks in probe depending on the type. That will also tell the
i2c-core that slave functionality is not supported. And you can use if
(!algo->reg_slave) as a flag, too.

> +	iproc_i2c->type =
> +		(enum bcm_iproc_i2c_type) of_device_get_match_data(&pdev->dev);

No need to cast a void*.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 7/8] i2c: iproc: add NIC I2C support

[Ray Jui]

On 4/2/2019 3:27 AM, Wolfram Sang wrote:
> 
>> +#define IDM_CTRL_DIRECT_OFFSET       0x00
> 
> And this IDM thing is also never used outside of the I2C context? In
> other words, it also doesn't need a seperate DT node?
> 
> 

That is correct. Only in the I2C context in our use case.

>> +	/* indicates no slave mode support */
>> +	bool no_slave;
> 
> I would suggest to not use a flag, but to nullify the {un}reg_slave
> callbacks in probe depending on the type. That will also tell the
> i2c-core that slave functionality is not supported. And you can use if
> (!algo->reg_slave) as a flag, too.
> 

Yes. Great idea. I'll make the change and remove reg_slave.

>> +	iproc_i2c->type =
>> +		(enum bcm_iproc_i2c_type) of_device_get_match_data(&pdev->dev);
> 
> No need to cast a void*.
> 

Yes will fix.

Thanks,

Ray

Re: [PATCH v5 7/8] i2c: iproc: add NIC I2C support

[Ray Jui]

On 4/2/2019 10:57 AM, Ray Jui wrote:
> 
> 
> On 4/2/2019 3:27 AM, Wolfram Sang wrote:

[...]

>>> +	iproc_i2c->type =
>>> +		(enum bcm_iproc_i2c_type) of_device_get_match_data(&pdev->dev);
>>
>> No need to cast a void*.
>>

In fact, we do need to type cast here since 'iproc_i2c->type' is NOT a
pointer:

drivers/i2c/busses/i2c-bcm-iproc.c:870:18: error: incompatible types
when assigning to type ‘enum bcm_iproc_i2c_type’ from type ‘const void *’
  iproc_i2c->type = of_device_get_match_data(&pdev->dev);
                  ^
> 
> Yes will fix.
> 
> Thanks,
> 
> Ray
>