TK1 in a multi-master I2C environment

Autonomous Machines Jetson & Embedded Systems Jetson TK1
1

I am using the TK1 in a multi-master I2C environment (SMBus). The controller is set to 100kHz. According to the TRM the TK1 supports multi-master environments. But when debugging the I2C bus, I see that the TK1 starts writing to the bus independently if the bus is in use or not, causing the other transaction to fail.
Thus it looks like the TK1 is not checking if the bus is free before writing to it.

Is there a way to enable bus free checking? Anybody having experience with multi-master I2C and the TK1?

2

Hello,
Please check and try the following patch, it may help.

drivers/i2c/busses/i2c-tegra.c | 34 +++++++++++++++++++++++++++++++++-
 include/linux/i2c-tegra.h      |  1 +
 2 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index dfa55c2..e9db3db 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -133,6 +133,9 @@
 #define I2C_SLV_CONFIG_LOAD			(1 << 1)
 #define I2C_TIMEOUT_CONFIG_LOAD			(1 << 2)
 
+#define I2C_CLKEN_OVERRIDE			0x090
+#define I2C_MST_CORE_CLKEN_OVR			(1 << 0)
+
 #define SL_ADDR1(addr) (addr & 0xff)
 #define SL_ADDR2(addr) ((addr >> 8) & 0xff)
 
@@ -162,6 +165,7 @@ struct tegra_i2c_chipdata {
 	u16 clk_divisor_hs_mode;
 	int clk_multiplier_hs_mode;
 	bool has_config_load_reg;
+	bool has_clk_override_reg;
 };
 
 /**
@@ -232,6 +236,7 @@ struct tegra_i2c_dev {
 	bool bit_banging_xfer_after_shutdown;
 	bool is_shutdown;
 	struct notifier_block pm_nb;
+	bool is_multimaster_mode;
 };
 
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val, unsigned long reg)
@@ -702,6 +707,9 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 	if (tegra_i2c_flush_fifos(i2c_dev))
 		err = -ETIMEDOUT;
 
+	if (i2c_dev->is_multimaster_mode && i2c_dev->chipdata->has_clk_override_reg)
+		i2c_writel(i2c_dev, I2C_MST_CORE_CLKEN_OVR, I2C_CLKEN_OVERRIDE);
+
 	if (i2c_dev->chipdata->has_config_load_reg) {
 		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD, I2C_CONFIG_LOAD);
 		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
@@ -1364,6 +1372,12 @@ static struct tegra_i2c_platform_data *parse_i2c_tegra_dt(
 	pdata->sda_gpio = of_get_named_gpio(np, "sda-gpio", 0);
 	pdata->is_dvc = of_device_is_compatible(np, "nvidia,tegra20-i2c-dvc");
 
+	pdata->is_multimaster_mode = of_property_read_bool(np,
+			"nvidia,multimaster-mode");
+
+	if (pdata->is_multimaster_mode)
+		pdata->is_clkon_always = true;
+
 	/* Default configuration for device tree initiated driver */
 	pdata->slave_addr = 0xFC;
 	return pdata;
@@ -1380,6 +1394,7 @@ static struct tegra_i2c_chipdata tegra20_i2c_chipdata = {
 	.clk_divisor_hs_mode = 3,
 	.clk_multiplier_hs_mode = 12,
 	.has_config_load_reg = false,
+	.has_clk_override_reg = false,
 };
 
 static struct tegra_i2c_chipdata tegra30_i2c_chipdata = {
@@ -1393,6 +1408,7 @@ static struct tegra_i2c_chipdata tegra30_i2c_chipdata = {
 	.clk_divisor_hs_mode = 3,
 	.clk_multiplier_hs_mode = 12,
 	.has_config_load_reg = false,
+	.has_clk_override_reg = false,
 };
 
 static struct tegra_i2c_chipdata tegra114_i2c_chipdata = {
@@ -1407,6 +1423,7 @@ static struct tegra_i2c_chipdata tegra114_i2c_chipdata = {
 	.clk_divisor_hs_mode = 1,
 	.clk_multiplier_hs_mode = 3,
 	.has_config_load_reg = false,
+	.has_clk_override_reg = false,
 };
 
 static struct tegra_i2c_chipdata tegra148_i2c_chipdata = {
@@ -1421,9 +1438,23 @@ static struct tegra_i2c_chipdata tegra148_i2c_chipdata = {
 	.clk_divisor_hs_mode = 2,
 	.clk_multiplier_hs_mode = 13,
 	.has_config_load_reg = true,
+	.has_clk_override_reg = true,
 };
 
-#define tegra124_i2c_chipdata tegra148_i2c_chipdata
+static struct tegra_i2c_chipdata tegra124_i2c_chipdata = {
+	.timeout_irq_occurs_before_bus_inactive = false,
+	.has_xfer_complete_interrupt = true,
+	.has_continue_xfer_support = true,
+	.has_hw_arb_support = true,
+	.has_fast_clock = false,
+	.has_clk_divisor_std_fast_mode = true,
+	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_fast_plus_mode = 0x10,
+	.clk_divisor_hs_mode = 2,
+	.clk_multiplier_hs_mode = 13,
+	.has_config_load_reg = true,
+	.has_clk_override_reg = true,
+};
 
 /* Match table for of_platform binding */
 static const struct of_device_id tegra_i2c_of_match[] = {
@@ -1584,6 +1615,7 @@ skip_pinctrl:
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
 	i2c_dev->is_clkon_always = pdata->is_clkon_always;
+	i2c_dev->is_multimaster_mode = pdata->is_multimaster_mode;
 	i2c_dev->bus_clk_rate = pdata->bus_clk_rate ? pdata->bus_clk_rate: 100000;
 	i2c_dev->is_high_speed_enable = pdata->is_high_speed_enable;
 	i2c_dev->clk_divisor_non_hs_mode =
diff --git a/include/linux/i2c-tegra.h b/include/linux/i2c-tegra.h
index ea40c64..9d1e869 100644
--- a/include/linux/i2c-tegra.h
+++ b/include/linux/i2c-tegra.h
@@ -33,6 +33,7 @@ struct tegra_i2c_platform_data {
 	u16 hs_master_code;
 	bool needs_cl_dvfs_clock;
 	bool bit_banging_xfer_after_shutdown;
+	bool is_multimaster_mode;
 };
 
 struct tegra_i2c_slave_platform_data {



 drivers/i2c/busses/i2c-tegra.c | 147 +++++++++++++++++++++++------------------
 1 file changed, 83 insertions(+), 64 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e9db3db..dda35db 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -744,6 +744,32 @@ static int tegra_i2c_copy_next_to_current(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
+static int tegra_i2c_disable_packet_mode(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 cnfg;
+	unsigned long timeout;
+
+	cnfg = i2c_readl(i2c_dev, I2C_CNFG);
+	if (cnfg & I2C_CNFG_PACKET_MODE_EN)
+		i2c_writel(i2c_dev, cnfg & (~I2C_CNFG_PACKET_MODE_EN),
+				I2C_CNFG);
+
+	if (i2c_dev->chipdata->has_config_load_reg) {
+		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD,
+				I2C_CONFIG_LOAD);
+		timeout = jiffies + msecs_to_jiffies(1000);
+		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
+			if (time_after(jiffies, timeout)) {
+				dev_err(i2c_dev->dev,
+						"timeout config_load");
+				return -ETIMEDOUT;
+			}
+			udelay(2);
+		}
+	}
+	return 0;
+}
+
 static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 {
 	u32 status;
@@ -754,6 +780,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	struct tegra_i2c_dev *i2c_dev = dev_id;
 	u32 mask;
 
+	spin_lock_irqsave(&i2c_dev->fifo_lock, flags);
 	status = i2c_readl(i2c_dev, I2C_INT_STATUS);
 
 	if (status == 0) {
@@ -769,6 +796,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	}
 
 	if (unlikely(status & status_err)) {
+		tegra_i2c_disable_packet_mode(i2c_dev);
 		dev_dbg(i2c_dev->dev, "I2c error status 0x%08x\n", status);
 		if (status & I2C_INT_NO_ACK) {
 			i2c_dev->msg_err |= I2C_ERR_NO_ACK;
@@ -823,15 +851,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	}
 
 	if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
-		if (i2c_dev->msg_buf_remaining) {
-
-			spin_lock_irqsave(&i2c_dev->fifo_lock, flags);
-
+		if (i2c_dev->msg_buf_remaining)
 			tegra_i2c_fill_tx_fifo(i2c_dev);
-
-			spin_unlock_irqrestore(&i2c_dev->fifo_lock, flags);
-
-		}
 		else
 			tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
 	}
@@ -850,8 +871,7 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 		complete(&i2c_dev->msg_complete);
 	}
 
-	return IRQ_HANDLED;
-
+	goto done;
 err:
 	dev_dbg(i2c_dev->dev, "reg: 0x%08x 0x%08x 0x%08x 0x%08x\n",
 		 i2c_readl(i2c_dev, I2C_CNFG), i2c_readl(i2c_dev, I2C_STATUS),
@@ -901,6 +921,8 @@ err:
 
 	complete(&i2c_dev->msg_complete);
 
+done:
+	spin_unlock_irqrestore(&i2c_dev->fifo_lock, flags);
 	return IRQ_HANDLED;
 }
 
@@ -949,6 +971,46 @@ static int tegra_i2c_send_next_read_msg_pkt_header(struct tegra_i2c_dev *i2c_dev
 	return 0;
 }
 
+static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
+{
+	unsigned long timeout;
+
+	if (i2c_dev->chipdata->has_hw_arb_support) {
+		INIT_COMPLETION(i2c_dev->msg_complete);
+		i2c_writel(i2c_dev, I2C_BC_ENABLE
+				| I2C_BC_SCLK_THRESHOLD
+				| I2C_BC_STOP_COND
+				| I2C_BC_TERMINATE
+				, I2C_BUS_CLEAR_CNFG);
+		if (i2c_dev->chipdata->has_config_load_reg) {
+			i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD,
+					I2C_CONFIG_LOAD);
+			timeout = jiffies + msecs_to_jiffies(1000);
+			while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
+				if (time_after(jiffies, timeout)) {
+					dev_warn(i2c_dev->dev,
+						"timeout config_load");
+					return -ETIMEDOUT;
+				}
+				msleep(1);
+			}
+		}
+		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLEAR_DONE);
+
+		wait_for_completion_timeout(&i2c_dev->msg_complete,
+				TEGRA_I2C_TIMEOUT);
+		if (!(i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS) & I2C_BC_STATUS))
+			dev_warn(i2c_dev->dev, "Un-recovered Arbitration lost\n");
+	} else {
+		i2c_algo_busclear_gpio(i2c_dev->dev,
+				i2c_dev->scl_gpio, GPIOF_OPEN_DRAIN,
+				i2c_dev->sda_gpio, GPIOF_OPEN_DRAIN,
+				MAX_BUSCLEAR_CLOCK, 100000);
+	}
+
+	return 0;
+}
+
 static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	struct i2c_msg *msg, enum msg_end_type end_state,
 	struct i2c_msg *next_msg, enum msg_end_type next_msg_end_state)
@@ -991,6 +1053,9 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		}
 	}
 	i2c_writel(i2c_dev, 0, I2C_INT_MASK);
+	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST
+					| I2C_INT_TX_FIFO_OVERFLOW;
+	tegra_i2c_unmask_irq(i2c_dev, int_mask);
 
 	val = 7 << I2C_FIFO_CONTROL_TX_TRIG_SHIFT |
 		0 << I2C_FIFO_CONTROL_RX_TRIG_SHIFT;
@@ -1064,10 +1129,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 			i2c_dev->chipdata->has_xfer_complete_interrupt))
 		int_mask |= I2C_INT_ALL_PACKETS_XFER_COMPLETE;
 
-	spin_unlock_irqrestore(&i2c_dev->fifo_lock, flags);
-
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
 
+	spin_unlock_irqrestore(&i2c_dev->fifo_lock, flags);
+
 	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
 		i2c_readl(i2c_dev, I2C_INT_MASK));
 
@@ -1107,25 +1172,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (i2c_dev->is_dvc)
 		dvc_i2c_mask_irq(i2c_dev, DVC_CTRL_REG3_I2C_DONE_INTR_EN);
 
-	cnfg = i2c_readl(i2c_dev, I2C_CNFG);
-	if (cnfg & I2C_CNFG_PACKET_MODE_EN)
-		i2c_writel(i2c_dev, cnfg & (~I2C_CNFG_PACKET_MODE_EN),
-								I2C_CNFG);
-	else
-		dev_err(i2c_dev->dev, "i2c_cnfg PACKET_MODE_EN not set\n");
-
-	if (i2c_dev->chipdata->has_config_load_reg) {
-		i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD,
-					I2C_CONFIG_LOAD);
-		while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
-			if (time_after(jiffies, timeout)) {
-				dev_warn(i2c_dev->dev,
-					"timeout config_load");
-				return -ETIMEDOUT;
-			}
-			udelay(2);
-		}
-	}
+	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
+		tegra_i2c_disable_packet_mode(i2c_dev);
 
 	if (ret == 0) {
 		dev_err(i2c_dev->dev,
@@ -1189,39 +1237,10 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	/* Arbitration Lost occurs, Start recovery */
 	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
-		if (i2c_dev->chipdata->has_hw_arb_support) {
-			INIT_COMPLETION(i2c_dev->msg_complete);
-			i2c_writel(i2c_dev, I2C_BC_ENABLE
-					| I2C_BC_SCLK_THRESHOLD
-					| I2C_BC_STOP_COND
-					| I2C_BC_TERMINATE
-					, I2C_BUS_CLEAR_CNFG);
-
-			if (i2c_dev->chipdata->has_config_load_reg) {
-				i2c_writel(i2c_dev, I2C_MSTR_CONFIG_LOAD,
-							I2C_CONFIG_LOAD);
-				while (i2c_readl(i2c_dev, I2C_CONFIG_LOAD) != 0) {
-					if (time_after(jiffies, timeout)) {
-						dev_warn(i2c_dev->dev,
-							"timeout config_load");
-						return -ETIMEDOUT;
-					}
-					msleep(1);
-				}
-			}
-
-			tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLEAR_DONE);
-
-			wait_for_completion_timeout(&i2c_dev->msg_complete,
-				TEGRA_I2C_TIMEOUT);
-
-			if (!(i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS) & I2C_BC_STATUS))
-				dev_warn(i2c_dev->dev, "Un-recovered Arbitration lost\n");
-		} else {
-			i2c_algo_busclear_gpio(i2c_dev->dev,
-				i2c_dev->scl_gpio, GPIOF_OPEN_DRAIN,
-				i2c_dev->sda_gpio,GPIOF_OPEN_DRAIN,
-				MAX_BUSCLEAR_CLOCK, 100000);
+		if (!i2c_dev->is_multimaster_mode) {
+			ret = tegra_i2c_issue_bus_clear(i2c_dev);
+			if (ret)
+				return ret;
 		}
 		return -EAGAIN;
 	}

br
ChenJian

3

Hi jachen

Thank you.
I also came up with a similar set of patches, but I didn’t have the IRQ modifications and the tegra_i2c_disable_packet_mode() on errors. The change on having the clock always running and skipping bus_clear on arbitration lost certainly make a big improvement.

While I can’t fully proof it, I think also the IRQ and the tegra_i2c_disable_packet_mode() slightly improve the situation.

But I still see some bus errors. I also think they happen more with higher CPU load.

Do you have any other suggestions for improvements?

Marc

4

Hi Marc, Did this end up working?