[PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically
@ 2019-01-31  6:16 ` Sowjanya Komatineni
  0 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

This patch sorts all the include headers alphabetically for the
I2C tegra driver

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V3/V4/V5/V7/V8] : Removed unsued headers in tegra I2C
 [V2] : Added this in V2 to sort the headers in tegra I2C

 drivers/i2c/busses/i2c-tegra.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e417ebf7628c..15806c984859 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -6,24 +6,21 @@
  * Author: Colin Cross <ccross@android.com>
  */
 
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
-#include <linux/io.h>
+#include <linux/init.h>
 #include <linux/interrupt.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
-#include <linux/of_device.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/reset.h>
+#include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
-#include <linux/iopoll.h>
-
-#include <asm/unaligned.h>
+#include <linux/reset.h>
 
 #define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
 #define BYTES_PER_FIFO_WORD 4
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically
@ 2019-01-31  6:16 ` Sowjanya Komatineni
  0 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

This patch sorts all the include headers alphabetically for the
I2C tegra driver

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V3/V4/V5/V7/V8] : Removed unsued headers in tegra I2C
 [V2] : Added this in V2 to sort the headers in tegra I2C

 drivers/i2c/busses/i2c-tegra.c | 19 ++++++++-----------
 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index e417ebf7628c..15806c984859 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -6,24 +6,21 @@
  * Author: Colin Cross <ccross@android.com>
  */
 
-#include <linux/kernel.h>
-#include <linux/init.h>
-#include <linux/platform_device.h>
 #include <linux/clk.h>
+#include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
-#include <linux/io.h>
+#include <linux/init.h>
 #include <linux/interrupt.h>
-#include <linux/delay.h>
-#include <linux/slab.h>
-#include <linux/of_device.h>
+#include <linux/io.h>
+#include <linux/iopoll.h>
+#include <linux/kernel.h>
 #include <linux/module.h>
-#include <linux/reset.h>
+#include <linux/of_device.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
-#include <linux/iopoll.h>
-
-#include <asm/unaligned.h>
+#include <linux/reset.h>
 
 #define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
 #define BYTES_PER_FIFO_WORD 4
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 2/5] i2c: tegra: Add Bus Clear Master Support
  2019-01-31  6:16 ` Sowjanya Komatineni
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  -1 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

Bus clear feature of tegra i2c controller helps to recover from
bus hang when i2c master loses the bus arbitration due to the
slave device holding SDA LOW continuously for some unknown reasons.

Per I2C specification, the device that held the bus LOW should
release it within 9 clock pulses.

During bus clear operation, Tegra I2C controller sends 9 clock
pulses and terminates the transaction with STOP condition.
Upon successful bus clear operation, bus goes to idle state and
driver retries the transaction.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V5/V6/V7/V8]: Same as V4
 [V4]: Added I2C Bus Clear support patch to this version of series.

 drivers/i2c/busses/i2c-tegra.c | 71 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 15806c984859..c4892a47a483 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -51,6 +51,7 @@
 #define I2C_FIFO_STATUS_RX_SHIFT		0
 #define I2C_INT_MASK				0x064
 #define I2C_INT_STATUS				0x068
+#define I2C_INT_BUS_CLR_DONE			BIT(11)
 #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
 #define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
 #define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
@@ -93,6 +94,15 @@
 #define I2C_HEADER_MASTER_ADDR_SHIFT		12
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
+#define I2C_BUS_CLEAR_CNFG			0x084
+#define I2C_BC_SCLK_THRESHOLD			9
+#define I2C_BC_SCLK_THRESHOLD_SHIFT		16
+#define I2C_BC_STOP_COND			BIT(2)
+#define I2C_BC_TERMINATE			BIT(1)
+#define I2C_BC_ENABLE				BIT(0)
+#define I2C_BUS_CLEAR_STATUS			0x088
+#define I2C_BC_STATUS				BIT(0)
+
 #define I2C_CONFIG_LOAD				0x08C
 #define I2C_MSTR_CONFIG_LOAD			BIT(0)
 #define I2C_SLV_CONFIG_LOAD			BIT(1)
@@ -152,6 +162,8 @@ enum msg_end_type {
  * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
  *		provides additional features and allows for longer messages to
  *		be transferred in one go.
+ * @supports_bus_clear: Bus Clear support to recover from bus hang during
+ *		SDA stuck low from device for some unknown reasons.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -164,6 +176,7 @@ struct tegra_i2c_hw_feature {
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
+	bool supports_bus_clear;
 };
 
 /**
@@ -636,6 +649,12 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
 		goto err;
 	}
+	/*
+	 * I2C transfer is terminated during the bus clear so skip
+	 * processing the other interrupts.
+	 */
+	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
+		goto err;
 
 	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
 		if (i2c_dev->msg_buf_remaining)
@@ -665,6 +684,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
 		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
 		I2C_INT_RX_FIFO_DATA_REQ);
+	if (i2c_dev->hw->supports_bus_clear)
+		tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
@@ -675,6 +696,43 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
+{
+	int err;
+	unsigned long time_left;
+	u32 reg;
+
+	if (i2c_dev->hw->supports_bus_clear) {
+		reinit_completion(&i2c_dev->msg_complete);
+		reg = (I2C_BC_SCLK_THRESHOLD << I2C_BC_SCLK_THRESHOLD_SHIFT) |
+		      I2C_BC_STOP_COND | I2C_BC_TERMINATE;
+		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+		if (i2c_dev->hw->has_config_load_reg) {
+			err = tegra_i2c_wait_for_config_load(i2c_dev);
+			if (err)
+				return err;
+		}
+		reg |= I2C_BC_ENABLE;
+		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
+
+		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
+							TEGRA_I2C_TIMEOUT);
+		if (time_left == 0) {
+			dev_err(i2c_dev->dev, "timed out for bus clear\n");
+			return -ETIMEDOUT;
+		}
+		reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
+		if (!(reg & I2C_BC_STATUS)) {
+			dev_err(i2c_dev->dev,
+				"Un-recovered arbitration lost\n");
+			return -EIO;
+		}
+	}
+
+	return -EAGAIN;
+}
+
 static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	struct i2c_msg *msg, enum msg_end_type end_state)
 {
@@ -756,6 +814,12 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		return 0;
 
 	tegra_i2c_init(i2c_dev);
+	/* start recovery upon arbitration loss in single master mode */
+	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
+		if (!i2c_dev->is_multimaster_mode)
+			return tegra_i2c_issue_bus_clear(i2c_dev);
+		return -EAGAIN;
+	}
 	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
@@ -845,6 +909,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -858,6 +923,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -871,6 +937,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -884,6 +951,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -897,6 +965,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -910,6 +979,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
+	.supports_bus_clear = true,
 };
 
 /* Match table for of_platform binding */
@@ -962,6 +1032,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->div_clk = div_clk;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
+	i2c_dev->adapter.retries = 1;
 	i2c_dev->irq = irq;
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 2/5] i2c: tegra: Add Bus Clear Master Support
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  0 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

Bus clear feature of tegra i2c controller helps to recover from
bus hang when i2c master loses the bus arbitration due to the
slave device holding SDA LOW continuously for some unknown reasons.

Per I2C specification, the device that held the bus LOW should
release it within 9 clock pulses.

During bus clear operation, Tegra I2C controller sends 9 clock
pulses and terminates the transaction with STOP condition.
Upon successful bus clear operation, bus goes to idle state and
driver retries the transaction.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V5/V6/V7/V8]: Same as V4
 [V4]: Added I2C Bus Clear support patch to this version of series.

 drivers/i2c/busses/i2c-tegra.c | 71 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 71 insertions(+)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 15806c984859..c4892a47a483 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -51,6 +51,7 @@
 #define I2C_FIFO_STATUS_RX_SHIFT		0
 #define I2C_INT_MASK				0x064
 #define I2C_INT_STATUS				0x068
+#define I2C_INT_BUS_CLR_DONE			BIT(11)
 #define I2C_INT_PACKET_XFER_COMPLETE		BIT(7)
 #define I2C_INT_ALL_PACKETS_XFER_COMPLETE	BIT(6)
 #define I2C_INT_TX_FIFO_OVERFLOW		BIT(5)
@@ -93,6 +94,15 @@
 #define I2C_HEADER_MASTER_ADDR_SHIFT		12
 #define I2C_HEADER_SLAVE_ADDR_SHIFT		1
 
+#define I2C_BUS_CLEAR_CNFG			0x084
+#define I2C_BC_SCLK_THRESHOLD			9
+#define I2C_BC_SCLK_THRESHOLD_SHIFT		16
+#define I2C_BC_STOP_COND			BIT(2)
+#define I2C_BC_TERMINATE			BIT(1)
+#define I2C_BC_ENABLE				BIT(0)
+#define I2C_BUS_CLEAR_STATUS			0x088
+#define I2C_BC_STATUS				BIT(0)
+
 #define I2C_CONFIG_LOAD				0x08C
 #define I2C_MSTR_CONFIG_LOAD			BIT(0)
 #define I2C_SLV_CONFIG_LOAD			BIT(1)
@@ -152,6 +162,8 @@ enum msg_end_type {
  * @has_mst_fifo: The I2C controller contains the new MST FIFO interface that
  *		provides additional features and allows for longer messages to
  *		be transferred in one go.
+ * @supports_bus_clear: Bus Clear support to recover from bus hang during
+ *		SDA stuck low from device for some unknown reasons.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -164,6 +176,7 @@ struct tegra_i2c_hw_feature {
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
+	bool supports_bus_clear;
 };
 
 /**
@@ -636,6 +649,12 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 			i2c_dev->msg_err |= I2C_ERR_ARBITRATION_LOST;
 		goto err;
 	}
+	/*
+	 * I2C transfer is terminated during the bus clear so skip
+	 * processing the other interrupts.
+	 */
+	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
+		goto err;
 
 	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
 		if (i2c_dev->msg_buf_remaining)
@@ -665,6 +684,8 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	tegra_i2c_mask_irq(i2c_dev, I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST |
 		I2C_INT_PACKET_XFER_COMPLETE | I2C_INT_TX_FIFO_DATA_REQ |
 		I2C_INT_RX_FIFO_DATA_REQ);
+	if (i2c_dev->hw->supports_bus_clear)
+		tegra_i2c_mask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
@@ -675,6 +696,43 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	return IRQ_HANDLED;
 }
 
+static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
+{
+	int err;
+	unsigned long time_left;
+	u32 reg;
+
+	if (i2c_dev->hw->supports_bus_clear) {
+		reinit_completion(&i2c_dev->msg_complete);
+		reg = (I2C_BC_SCLK_THRESHOLD << I2C_BC_SCLK_THRESHOLD_SHIFT) |
+		      I2C_BC_STOP_COND | I2C_BC_TERMINATE;
+		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+		if (i2c_dev->hw->has_config_load_reg) {
+			err = tegra_i2c_wait_for_config_load(i2c_dev);
+			if (err)
+				return err;
+		}
+		reg |= I2C_BC_ENABLE;
+		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
+		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
+
+		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
+							TEGRA_I2C_TIMEOUT);
+		if (time_left == 0) {
+			dev_err(i2c_dev->dev, "timed out for bus clear\n");
+			return -ETIMEDOUT;
+		}
+		reg = i2c_readl(i2c_dev, I2C_BUS_CLEAR_STATUS);
+		if (!(reg & I2C_BC_STATUS)) {
+			dev_err(i2c_dev->dev,
+				"Un-recovered arbitration lost\n");
+			return -EIO;
+		}
+	}
+
+	return -EAGAIN;
+}
+
 static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	struct i2c_msg *msg, enum msg_end_type end_state)
 {
@@ -756,6 +814,12 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		return 0;
 
 	tegra_i2c_init(i2c_dev);
+	/* start recovery upon arbitration loss in single master mode */
+	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
+		if (!i2c_dev->is_multimaster_mode)
+			return tegra_i2c_issue_bus_clear(i2c_dev);
+		return -EAGAIN;
+	}
 	if (i2c_dev->msg_err == I2C_ERR_NO_ACK) {
 		if (msg->flags & I2C_M_IGNORE_NAK)
 			return 0;
@@ -845,6 +909,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -858,6 +923,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -871,6 +937,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -884,6 +951,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -897,6 +965,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
+	.supports_bus_clear = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -910,6 +979,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
+	.supports_bus_clear = true,
 };
 
 /* Match table for of_platform binding */
@@ -962,6 +1032,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->div_clk = div_clk;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
+	i2c_dev->adapter.retries = 1;
 	i2c_dev->irq = irq;
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31  6:16 ` Sowjanya Komatineni
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  -1 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

This patch adds DMA support for Tegra I2C.

Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
transfer size of the max FIFO depth and DMA mode is used for
transfer size higher than max FIFO depth to save CPU overhead.

PIO mode needs full intervention of CPU to fill or empty FIFO's
and also need to service multiple data requests interrupt for the
same transaction. This adds delay between data bytes of the same
transfer when CPU is fully loaded and some slave devices has
internal timeout for no bus activity and stops transaction to
avoid bus hang. DMA mode is helpful in such cases.

DMA mode is also helpful for Large transfers during downloading or
uploading FW over I2C to some external devices.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V8] : Moved back dma init to i2c probe, removed ALL_PACKETS_XFER_COMPLETE
	interrupt and using PACKETS_XFER_COMPLETE interrupt only and some
	other fixes
	Updated Kconfig for APB_DMA dependency
 [V7] : Same as V6
 [V6] : Updated for proper buffer allocation/freeing, channel release.
	Updated to use exact xfer size for syncing dma buffer.
 [V5] : Same as V4
 [V4] : Updated to allocate DMA buffer only when DMA mode.
	Updated to fall back to PIO mode when DMA channel request or
	buffer allocation fails.
 [V3] : Updated without additional buffer allocation.
 [V2] : Updated based on V1 review feedback along with code cleanup for
	proper implementation of DMA.

 drivers/i2c/busses/Kconfig     |   2 +-
 drivers/i2c/busses/i2c-tegra.c | 362 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 339 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f2c681971201..046aeb92a467 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1016,7 +1016,7 @@ config I2C_SYNQUACER
 
 config I2C_TEGRA
 	tristate "NVIDIA Tegra internal I2C controller"
-	depends on ARCH_TEGRA
+	depends on (ARCH_TEGRA && TEGRA20_APB_DMA)
 	help
 	  If you say yes to this option, support will be included for the
 	  I2C controller embedded in NVIDIA Tegra SOCs
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c4892a47a483..025d63972e50 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -8,6 +8,9 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dmapool.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
@@ -44,6 +47,8 @@
 #define I2C_FIFO_CONTROL_RX_FLUSH		BIT(0)
 #define I2C_FIFO_CONTROL_TX_TRIG_SHIFT		5
 #define I2C_FIFO_CONTROL_RX_TRIG_SHIFT		2
+#define I2C_FIFO_CONTROL_TX_TRIG(x)		(((x) - 1) << 5)
+#define I2C_FIFO_CONTROL_RX_TRIG(x)		(((x) - 1) << 2)
 #define I2C_FIFO_STATUS				0x060
 #define I2C_FIFO_STATUS_TX_MASK			0xF0
 #define I2C_FIFO_STATUS_TX_SHIFT		4
@@ -125,6 +130,19 @@
 #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
 #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
 
+/* Packet header size in bytes */
+#define I2C_PACKET_HEADER_SIZE			12
+
+#define DATA_DMA_DIR_TX				(1 << 0)
+#define DATA_DMA_DIR_RX				(1 << 1)
+
+/*
+ * Upto I2C_PIO_MODE_MAX_LEN bytes, controller will use PIO mode,
+ * above this, controller will use DMA to fill FIFO.
+ * MAX PIO len is 20 bytes excluding packet header.
+ */
+#define I2C_PIO_MODE_MAX_LEN			32
+
 /*
  * msg_end_type: The bus control which need to be send at end of transfer.
  * @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -188,6 +206,7 @@ struct tegra_i2c_hw_feature {
  * @fast_clk: clock reference for fast clock of I2C controller
  * @rst: reset control for the I2C controller
  * @base: ioremapped registers cookie
+ * @base_phys: Physical base address of the I2C controller
  * @cont_id: I2C controller ID, used for packet header
  * @irq: IRQ number of transfer complete interrupt
  * @irq_disabled: used to track whether or not the interrupt is enabled
@@ -201,6 +220,14 @@ struct tegra_i2c_hw_feature {
  * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
  * @is_multimaster_mode: track if I2C controller is in multi-master mode
  * @xfer_lock: lock to serialize transfer submission and processing
+ * @has_dma: indicates if DMA can be utilized based on dma DT bindings
+ * @tx_dma_chan: DMA transmit channel
+ * @rx_dma_chan: DMA receive channel
+ * @dma_phys: handle to DMA resources
+ * @dma_buf: pointer to allocated DMA buffer
+ * @dma_buf_size: DMA buffer size
+ * @is_curr_dma_xfer: indicates active DMA transfer
+ * @dma_complete: DMA completion notifier
  */
 struct tegra_i2c_dev {
 	struct device *dev;
@@ -210,6 +237,7 @@ struct tegra_i2c_dev {
 	struct clk *fast_clk;
 	struct reset_control *rst;
 	void __iomem *base;
+	phys_addr_t base_phys;
 	int cont_id;
 	int irq;
 	bool irq_disabled;
@@ -223,6 +251,14 @@ struct tegra_i2c_dev {
 	u16 clk_divisor_non_hs_mode;
 	bool is_multimaster_mode;
 	spinlock_t xfer_lock;
+	bool has_dma;
+	struct dma_chan *tx_dma_chan;
+	struct dma_chan *rx_dma_chan;
+	dma_addr_t dma_phys;
+	u32 *dma_buf;
+	unsigned int dma_buf_size;
+	bool is_curr_dma_xfer;
+	struct completion dma_complete;
 };
 
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
@@ -291,6 +327,85 @@ static void tegra_i2c_unmask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
 	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
 }
 
+static void tegra_i2c_dma_complete(void *args)
+{
+	struct tegra_i2c_dev *i2c_dev = args;
+
+	complete(&i2c_dev->dma_complete);
+}
+
+static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len)
+{
+	struct dma_async_tx_descriptor *dma_desc;
+	enum dma_transfer_direction dir;
+	struct dma_chan *chan;
+
+	dev_dbg(i2c_dev->dev, "starting DMA for length: %zu\n", len);
+	reinit_completion(&i2c_dev->dma_complete);
+	dir = i2c_dev->msg_read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
+	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
+	dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys,
+					       len, dir, DMA_PREP_INTERRUPT |
+					       DMA_CTRL_ACK);
+	if (!dma_desc) {
+		dev_err(i2c_dev->dev, "failed to get DMA descriptor\n");
+		return -EIO;
+	}
+
+	dma_desc->callback = tegra_i2c_dma_complete;
+	dma_desc->callback_param = i2c_dev;
+	dmaengine_submit(dma_desc);
+	dma_async_issue_pending(chan);
+	return 0;
+}
+
+static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev)
+{
+	struct dma_chan *dma_chan;
+	u32 *dma_buf;
+	dma_addr_t dma_phys;
+
+	if (!i2c_dev->has_dma)
+		return -EINVAL;
+
+	if (!i2c_dev->rx_dma_chan) {
+		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "rx");
+		if (IS_ERR(dma_chan))
+			return PTR_ERR(dma_chan);
+
+		i2c_dev->rx_dma_chan = dma_chan;
+	}
+
+	if (!i2c_dev->tx_dma_chan) {
+		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "tx");
+		if (IS_ERR(dma_chan))
+			return PTR_ERR(dma_chan);
+
+		i2c_dev->tx_dma_chan = dma_chan;
+	}
+
+	if (!i2c_dev->dma_buf && i2c_dev->msg_buf_remaining) {
+		dma_buf = dma_alloc_coherent(i2c_dev->dev,
+					     i2c_dev->dma_buf_size,
+					     &dma_phys, GFP_KERNEL);
+
+		if (!dma_buf) {
+			dev_err(i2c_dev->dev,
+				"failed to allocate the DMA buffer\n");
+			dma_release_channel(i2c_dev->tx_dma_chan);
+			dma_release_channel(i2c_dev->rx_dma_chan);
+			i2c_dev->tx_dma_chan = NULL;
+			i2c_dev->rx_dma_chan = NULL;
+			return -ENOMEM;
+		}
+
+		i2c_dev->dma_buf = dma_buf;
+		i2c_dev->dma_phys = dma_phys;
+	}
+
+	return 0;
+}
+
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 {
 	unsigned long timeout = jiffies + HZ;
@@ -656,25 +771,38 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
 		goto err;
 
-	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
-		if (i2c_dev->msg_buf_remaining)
-			tegra_i2c_empty_rx_fifo(i2c_dev);
-		else
-			BUG();
-	}
+	if (!i2c_dev->is_curr_dma_xfer) {
+		if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
+			if (i2c_dev->msg_buf_remaining)
+				tegra_i2c_empty_rx_fifo(i2c_dev);
+			else
+				BUG();
+		}
 
-	if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
-		if (i2c_dev->msg_buf_remaining)
-			tegra_i2c_fill_tx_fifo(i2c_dev);
-		else
-			tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
+		if (!i2c_dev->msg_read &&
+		   (status & I2C_INT_TX_FIFO_DATA_REQ)) {
+			if (i2c_dev->msg_buf_remaining)
+				tegra_i2c_fill_tx_fifo(i2c_dev);
+			else
+				tegra_i2c_mask_irq(i2c_dev,
+						   I2C_INT_TX_FIFO_DATA_REQ);
+		}
 	}
 
 	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
 
+	/*
+	 * During message read XFER_COMPLETE interrupt is triggered prior to
+	 * DMA completion and during message write XFER_COMPLETE interrupt is
+	 * triggered after DMA completion.
+	 * PACKETS_XFER_COMPLETE indicates completion of all bytes of transfer.
+	 * so forcing msg_buf_remaining to 0 in DMA mode.
+	 */
 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
+		if (i2c_dev->is_curr_dma_xfer)
+			i2c_dev->msg_buf_remaining = 0;
 		BUG_ON(i2c_dev->msg_buf_remaining);
 		complete(&i2c_dev->msg_complete);
 	}
@@ -690,12 +818,69 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
 
+	if (i2c_dev->is_curr_dma_xfer) {
+		if (i2c_dev->msg_read)
+			dmaengine_terminate_all(i2c_dev->rx_dma_chan);
+		else
+			dmaengine_terminate_all(i2c_dev->tx_dma_chan);
+
+		complete(&i2c_dev->dma_complete);
+	}
+
 	complete(&i2c_dev->msg_complete);
 done:
 	spin_unlock(&i2c_dev->xfer_lock);
 	return IRQ_HANDLED;
 }
 
+static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
+				       size_t len, int direction)
+{
+	u32 val, reg;
+	u8 dma_burst = 0;
+	struct dma_slave_config dma_sconfig;
+	struct dma_chan *chan;
+
+	if (i2c_dev->hw->has_mst_fifo)
+		reg = I2C_MST_FIFO_CONTROL;
+	else
+		reg = I2C_FIFO_CONTROL;
+	val = i2c_readl(i2c_dev, reg);
+
+	if (len & 0xF)
+		dma_burst = 1;
+	else if (len & 0x10)
+		dma_burst = 4;
+	else
+		dma_burst = 8;
+
+	if (direction == DATA_DMA_DIR_TX) {
+		if (i2c_dev->hw->has_mst_fifo)
+			val |= I2C_MST_FIFO_CONTROL_TX_TRIG(dma_burst);
+		else
+			val |= I2C_FIFO_CONTROL_TX_TRIG(dma_burst);
+	} else {
+		if (i2c_dev->hw->has_mst_fifo)
+			val |= I2C_MST_FIFO_CONTROL_RX_TRIG(dma_burst);
+		else
+			val |= I2C_FIFO_CONTROL_RX_TRIG(dma_burst);
+	}
+	i2c_writel(i2c_dev, val, reg);
+
+	if (direction == DATA_DMA_DIR_TX) {
+		dma_sconfig.dst_addr = i2c_dev->base_phys + I2C_TX_FIFO;
+		dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dma_sconfig.dst_maxburst = dma_burst;
+	} else {
+		dma_sconfig.src_addr = i2c_dev->base_phys + I2C_RX_FIFO;
+		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dma_sconfig.src_maxburst = dma_burst;
+	}
+
+	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
+	dmaengine_slave_config(chan, &dma_sconfig);
+}
+
 static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
 {
 	int err;
@@ -740,6 +925,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	u32 int_mask;
 	unsigned long time_left;
 	unsigned long flags;
+	size_t xfer_size;
+	u32 *buffer = 0;
+	int err = 0;
+	bool dma = false;
+	struct dma_chan *chan;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -749,19 +939,69 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
 	reinit_completion(&i2c_dev->msg_complete);
 
+	if (i2c_dev->msg_read)
+		xfer_size = msg->len;
+	else
+		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
+
+	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
+	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
+	if (dma) {
+		err = tegra_i2c_init_dma_param(i2c_dev);
+		if (err < 0) {
+			dev_dbg(i2c_dev->dev, "switching to PIO transfer\n");
+			dma = false;
+		}
+	}
+
+	i2c_dev->is_curr_dma_xfer = dma;
 	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
 
 	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
 
+	if (dma) {
+		if (i2c_dev->msg_read) {
+			chan = i2c_dev->rx_dma_chan;
+			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
+						   DATA_DMA_DIR_RX);
+			dma_sync_single_for_device(i2c_dev->dev,
+						   i2c_dev->dma_phys,
+						   xfer_size,
+						   DMA_FROM_DEVICE);
+			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
+			if (err < 0) {
+				dev_err(i2c_dev->dev,
+					"starting RX DMA failed, err %d\n",
+					err);
+				goto unlock;
+			}
+		} else {
+			chan = i2c_dev->tx_dma_chan;
+			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
+						   DATA_DMA_DIR_TX);
+			dma_sync_single_for_cpu(i2c_dev->dev,
+						i2c_dev->dma_phys,
+						xfer_size,
+						DMA_TO_DEVICE);
+			buffer = i2c_dev->dma_buf;
+		}
+	}
+
 	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
 			PACKET_HEADER0_PROTOCOL_I2C |
 			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
 			(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
-	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+	if (dma && !i2c_dev->msg_read)
+		*buffer++ = packet_header;
+	else
+		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
 
 	packet_header = msg->len - 1;
-	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+	if (dma && !i2c_dev->msg_read)
+		*buffer++ = packet_header;
+	else
+		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
 
 	packet_header = I2C_HEADER_IE_ENABLE;
 	if (end_state == MSG_END_CONTINUE)
@@ -778,30 +1018,79 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		packet_header |= I2C_HEADER_CONT_ON_NAK;
 	if (msg->flags & I2C_M_RD)
 		packet_header |= I2C_HEADER_READ;
-	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
-
-	if (!(msg->flags & I2C_M_RD))
-		tegra_i2c_fill_tx_fifo(i2c_dev);
-
+	if (dma && !i2c_dev->msg_read)
+		*buffer++ = packet_header;
+	else
+		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	if (!i2c_dev->msg_read) {
+		if (dma) {
+			memcpy(buffer, msg->buf, msg->len);
+			dma_sync_single_for_device(i2c_dev->dev,
+						   i2c_dev->dma_phys,
+						   xfer_size,
+						   DMA_TO_DEVICE);
+			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
+			if (err < 0) {
+				dev_err(i2c_dev->dev,
+					"starting TX DMA failed, err %d\n",
+					err);
+				goto unlock;
+			}
+		} else {
+			tegra_i2c_fill_tx_fifo(i2c_dev);
+		}
+	}
 	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
 		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
-	if (msg->flags & I2C_M_RD)
-		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
-	else if (i2c_dev->msg_buf_remaining)
-		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+	if (!dma) {
+		if (msg->flags & I2C_M_RD)
+			int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
+		else if (i2c_dev->msg_buf_remaining)
+			int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+	}
 
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
-	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
 	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
 		i2c_readl(i2c_dev, I2C_INT_MASK));
 
+unlock:
+	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
+
+	if (dma) {
+		if (err)
+			return err;
+
+		time_left = wait_for_completion_timeout(
+						&i2c_dev->dma_complete,
+						TEGRA_I2C_TIMEOUT);
+
+		if (time_left == 0) {
+			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
+			dmaengine_terminate_all(chan);
+			tegra_i2c_init(i2c_dev);
+			return -ETIMEDOUT;
+		}
+
+		if (i2c_dev->msg_read) {
+			if (likely(i2c_dev->msg_err == I2C_ERR_NONE)) {
+				dma_sync_single_for_cpu(i2c_dev->dev,
+							i2c_dev->dma_phys,
+							xfer_size,
+							DMA_FROM_DEVICE);
+
+				memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
+					msg->len);
+			}
+		}
+	}
+
 	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
 						TEGRA_I2C_TIMEOUT);
 	tegra_i2c_mask_irq(i2c_dev, int_mask);
 
 	if (time_left == 0) {
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
-
 		tegra_i2c_init(i2c_dev);
 		return -ETIMEDOUT;
 	}
@@ -884,6 +1173,8 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 
 	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
 			"multi-master");
+
+	i2c_dev->has_dma = of_property_read_bool(np, "dmas");
 }
 
 static const struct i2c_algorithm tegra_i2c_algo = {
@@ -1002,11 +1293,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	struct clk *div_clk;
 	struct clk *fast_clk;
 	void __iomem *base;
+	phys_addr_t base_phys;
 	int irq;
 	int ret = 0;
 	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base_phys = res->start;
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -1029,6 +1322,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	i2c_dev->base = base;
+	i2c_dev->base_phys = base_phys;
 	i2c_dev->div_clk = div_clk;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
@@ -1036,6 +1330,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->irq = irq;
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
+	i2c_dev->dma_buf_size = i2c_dev->adapter.quirks->max_write_len;
 
 	i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c");
 	if (IS_ERR(i2c_dev->rst)) {
@@ -1049,6 +1344,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
 						  "nvidia,tegra20-i2c-dvc");
 	init_completion(&i2c_dev->msg_complete);
+	init_completion(&i2c_dev->dma_complete);
 	spin_lock_init(&i2c_dev->xfer_lock);
 
 	if (!i2c_dev->hw->has_single_clk_source) {
@@ -1109,6 +1405,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = tegra_i2c_init_dma_param(i2c_dev);
+	if (ret == -EPROBE_DEFER)
+		goto disable_div_clk;
+
 	ret = tegra_i2c_init(i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
@@ -1173,6 +1473,20 @@ static int tegra_i2c_remove(struct platform_device *pdev)
 	if (!i2c_dev->hw->has_single_clk_source)
 		clk_unprepare(i2c_dev->fast_clk);
 
+	if (i2c_dev->dma_buf)
+		dma_free_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
+				  i2c_dev->dma_buf, i2c_dev->dma_phys);
+
+	if (i2c_dev->tx_dma_chan) {
+		dma_release_channel(i2c_dev->tx_dma_chan);
+		i2c_dev->tx_dma_chan = NULL;
+	}
+
+	if (i2c_dev->rx_dma_chan) {
+		dma_release_channel(i2c_dev->rx_dma_chan);
+		i2c_dev->rx_dma_chan = NULL;
+	}
+
 	return 0;
 }
 
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 3/5] i2c: tegra: Add DMA Support
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  0 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

This patch adds DMA support for Tegra I2C.

Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
transfer size of the max FIFO depth and DMA mode is used for
transfer size higher than max FIFO depth to save CPU overhead.

PIO mode needs full intervention of CPU to fill or empty FIFO's
and also need to service multiple data requests interrupt for the
same transaction. This adds delay between data bytes of the same
transfer when CPU is fully loaded and some slave devices has
internal timeout for no bus activity and stops transaction to
avoid bus hang. DMA mode is helpful in such cases.

DMA mode is also helpful for Large transfers during downloading or
uploading FW over I2C to some external devices.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V8] : Moved back dma init to i2c probe, removed ALL_PACKETS_XFER_COMPLETE
	interrupt and using PACKETS_XFER_COMPLETE interrupt only and some
	other fixes
	Updated Kconfig for APB_DMA dependency
 [V7] : Same as V6
 [V6] : Updated for proper buffer allocation/freeing, channel release.
	Updated to use exact xfer size for syncing dma buffer.
 [V5] : Same as V4
 [V4] : Updated to allocate DMA buffer only when DMA mode.
	Updated to fall back to PIO mode when DMA channel request or
	buffer allocation fails.
 [V3] : Updated without additional buffer allocation.
 [V2] : Updated based on V1 review feedback along with code cleanup for
	proper implementation of DMA.

 drivers/i2c/busses/Kconfig     |   2 +-
 drivers/i2c/busses/i2c-tegra.c | 362 ++++++++++++++++++++++++++++++++++++++---
 2 files changed, 339 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index f2c681971201..046aeb92a467 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1016,7 +1016,7 @@ config I2C_SYNQUACER
 
 config I2C_TEGRA
 	tristate "NVIDIA Tegra internal I2C controller"
-	depends on ARCH_TEGRA
+	depends on (ARCH_TEGRA && TEGRA20_APB_DMA)
 	help
 	  If you say yes to this option, support will be included for the
 	  I2C controller embedded in NVIDIA Tegra SOCs
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c4892a47a483..025d63972e50 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -8,6 +8,9 @@
 
 #include <linux/clk.h>
 #include <linux/delay.h>
+#include <linux/dmaengine.h>
+#include <linux/dmapool.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/init.h>
@@ -44,6 +47,8 @@
 #define I2C_FIFO_CONTROL_RX_FLUSH		BIT(0)
 #define I2C_FIFO_CONTROL_TX_TRIG_SHIFT		5
 #define I2C_FIFO_CONTROL_RX_TRIG_SHIFT		2
+#define I2C_FIFO_CONTROL_TX_TRIG(x)		(((x) - 1) << 5)
+#define I2C_FIFO_CONTROL_RX_TRIG(x)		(((x) - 1) << 2)
 #define I2C_FIFO_STATUS				0x060
 #define I2C_FIFO_STATUS_TX_MASK			0xF0
 #define I2C_FIFO_STATUS_TX_SHIFT		4
@@ -125,6 +130,19 @@
 #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
 #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
 
+/* Packet header size in bytes */
+#define I2C_PACKET_HEADER_SIZE			12
+
+#define DATA_DMA_DIR_TX				(1 << 0)
+#define DATA_DMA_DIR_RX				(1 << 1)
+
+/*
+ * Upto I2C_PIO_MODE_MAX_LEN bytes, controller will use PIO mode,
+ * above this, controller will use DMA to fill FIFO.
+ * MAX PIO len is 20 bytes excluding packet header.
+ */
+#define I2C_PIO_MODE_MAX_LEN			32
+
 /*
  * msg_end_type: The bus control which need to be send at end of transfer.
  * @MSG_END_STOP: Send stop pulse at end of transfer.
@@ -188,6 +206,7 @@ struct tegra_i2c_hw_feature {
  * @fast_clk: clock reference for fast clock of I2C controller
  * @rst: reset control for the I2C controller
  * @base: ioremapped registers cookie
+ * @base_phys: Physical base address of the I2C controller
  * @cont_id: I2C controller ID, used for packet header
  * @irq: IRQ number of transfer complete interrupt
  * @irq_disabled: used to track whether or not the interrupt is enabled
@@ -201,6 +220,14 @@ struct tegra_i2c_hw_feature {
  * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
  * @is_multimaster_mode: track if I2C controller is in multi-master mode
  * @xfer_lock: lock to serialize transfer submission and processing
+ * @has_dma: indicates if DMA can be utilized based on dma DT bindings
+ * @tx_dma_chan: DMA transmit channel
+ * @rx_dma_chan: DMA receive channel
+ * @dma_phys: handle to DMA resources
+ * @dma_buf: pointer to allocated DMA buffer
+ * @dma_buf_size: DMA buffer size
+ * @is_curr_dma_xfer: indicates active DMA transfer
+ * @dma_complete: DMA completion notifier
  */
 struct tegra_i2c_dev {
 	struct device *dev;
@@ -210,6 +237,7 @@ struct tegra_i2c_dev {
 	struct clk *fast_clk;
 	struct reset_control *rst;
 	void __iomem *base;
+	phys_addr_t base_phys;
 	int cont_id;
 	int irq;
 	bool irq_disabled;
@@ -223,6 +251,14 @@ struct tegra_i2c_dev {
 	u16 clk_divisor_non_hs_mode;
 	bool is_multimaster_mode;
 	spinlock_t xfer_lock;
+	bool has_dma;
+	struct dma_chan *tx_dma_chan;
+	struct dma_chan *rx_dma_chan;
+	dma_addr_t dma_phys;
+	u32 *dma_buf;
+	unsigned int dma_buf_size;
+	bool is_curr_dma_xfer;
+	struct completion dma_complete;
 };
 
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
@@ -291,6 +327,85 @@ static void tegra_i2c_unmask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
 	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
 }
 
+static void tegra_i2c_dma_complete(void *args)
+{
+	struct tegra_i2c_dev *i2c_dev = args;
+
+	complete(&i2c_dev->dma_complete);
+}
+
+static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len)
+{
+	struct dma_async_tx_descriptor *dma_desc;
+	enum dma_transfer_direction dir;
+	struct dma_chan *chan;
+
+	dev_dbg(i2c_dev->dev, "starting DMA for length: %zu\n", len);
+	reinit_completion(&i2c_dev->dma_complete);
+	dir = i2c_dev->msg_read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
+	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
+	dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys,
+					       len, dir, DMA_PREP_INTERRUPT |
+					       DMA_CTRL_ACK);
+	if (!dma_desc) {
+		dev_err(i2c_dev->dev, "failed to get DMA descriptor\n");
+		return -EIO;
+	}
+
+	dma_desc->callback = tegra_i2c_dma_complete;
+	dma_desc->callback_param = i2c_dev;
+	dmaengine_submit(dma_desc);
+	dma_async_issue_pending(chan);
+	return 0;
+}
+
+static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev)
+{
+	struct dma_chan *dma_chan;
+	u32 *dma_buf;
+	dma_addr_t dma_phys;
+
+	if (!i2c_dev->has_dma)
+		return -EINVAL;
+
+	if (!i2c_dev->rx_dma_chan) {
+		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "rx");
+		if (IS_ERR(dma_chan))
+			return PTR_ERR(dma_chan);
+
+		i2c_dev->rx_dma_chan = dma_chan;
+	}
+
+	if (!i2c_dev->tx_dma_chan) {
+		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "tx");
+		if (IS_ERR(dma_chan))
+			return PTR_ERR(dma_chan);
+
+		i2c_dev->tx_dma_chan = dma_chan;
+	}
+
+	if (!i2c_dev->dma_buf && i2c_dev->msg_buf_remaining) {
+		dma_buf = dma_alloc_coherent(i2c_dev->dev,
+					     i2c_dev->dma_buf_size,
+					     &dma_phys, GFP_KERNEL);
+
+		if (!dma_buf) {
+			dev_err(i2c_dev->dev,
+				"failed to allocate the DMA buffer\n");
+			dma_release_channel(i2c_dev->tx_dma_chan);
+			dma_release_channel(i2c_dev->rx_dma_chan);
+			i2c_dev->tx_dma_chan = NULL;
+			i2c_dev->rx_dma_chan = NULL;
+			return -ENOMEM;
+		}
+
+		i2c_dev->dma_buf = dma_buf;
+		i2c_dev->dma_phys = dma_phys;
+	}
+
+	return 0;
+}
+
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 {
 	unsigned long timeout = jiffies + HZ;
@@ -656,25 +771,38 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
 		goto err;
 
-	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
-		if (i2c_dev->msg_buf_remaining)
-			tegra_i2c_empty_rx_fifo(i2c_dev);
-		else
-			BUG();
-	}
+	if (!i2c_dev->is_curr_dma_xfer) {
+		if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
+			if (i2c_dev->msg_buf_remaining)
+				tegra_i2c_empty_rx_fifo(i2c_dev);
+			else
+				BUG();
+		}
 
-	if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
-		if (i2c_dev->msg_buf_remaining)
-			tegra_i2c_fill_tx_fifo(i2c_dev);
-		else
-			tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
+		if (!i2c_dev->msg_read &&
+		   (status & I2C_INT_TX_FIFO_DATA_REQ)) {
+			if (i2c_dev->msg_buf_remaining)
+				tegra_i2c_fill_tx_fifo(i2c_dev);
+			else
+				tegra_i2c_mask_irq(i2c_dev,
+						   I2C_INT_TX_FIFO_DATA_REQ);
+		}
 	}
 
 	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
 
+	/*
+	 * During message read XFER_COMPLETE interrupt is triggered prior to
+	 * DMA completion and during message write XFER_COMPLETE interrupt is
+	 * triggered after DMA completion.
+	 * PACKETS_XFER_COMPLETE indicates completion of all bytes of transfer.
+	 * so forcing msg_buf_remaining to 0 in DMA mode.
+	 */
 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
+		if (i2c_dev->is_curr_dma_xfer)
+			i2c_dev->msg_buf_remaining = 0;
 		BUG_ON(i2c_dev->msg_buf_remaining);
 		complete(&i2c_dev->msg_complete);
 	}
@@ -690,12 +818,69 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	if (i2c_dev->is_dvc)
 		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
 
+	if (i2c_dev->is_curr_dma_xfer) {
+		if (i2c_dev->msg_read)
+			dmaengine_terminate_all(i2c_dev->rx_dma_chan);
+		else
+			dmaengine_terminate_all(i2c_dev->tx_dma_chan);
+
+		complete(&i2c_dev->dma_complete);
+	}
+
 	complete(&i2c_dev->msg_complete);
 done:
 	spin_unlock(&i2c_dev->xfer_lock);
 	return IRQ_HANDLED;
 }
 
+static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
+				       size_t len, int direction)
+{
+	u32 val, reg;
+	u8 dma_burst = 0;
+	struct dma_slave_config dma_sconfig;
+	struct dma_chan *chan;
+
+	if (i2c_dev->hw->has_mst_fifo)
+		reg = I2C_MST_FIFO_CONTROL;
+	else
+		reg = I2C_FIFO_CONTROL;
+	val = i2c_readl(i2c_dev, reg);
+
+	if (len & 0xF)
+		dma_burst = 1;
+	else if (len & 0x10)
+		dma_burst = 4;
+	else
+		dma_burst = 8;
+
+	if (direction == DATA_DMA_DIR_TX) {
+		if (i2c_dev->hw->has_mst_fifo)
+			val |= I2C_MST_FIFO_CONTROL_TX_TRIG(dma_burst);
+		else
+			val |= I2C_FIFO_CONTROL_TX_TRIG(dma_burst);
+	} else {
+		if (i2c_dev->hw->has_mst_fifo)
+			val |= I2C_MST_FIFO_CONTROL_RX_TRIG(dma_burst);
+		else
+			val |= I2C_FIFO_CONTROL_RX_TRIG(dma_burst);
+	}
+	i2c_writel(i2c_dev, val, reg);
+
+	if (direction == DATA_DMA_DIR_TX) {
+		dma_sconfig.dst_addr = i2c_dev->base_phys + I2C_TX_FIFO;
+		dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dma_sconfig.dst_maxburst = dma_burst;
+	} else {
+		dma_sconfig.src_addr = i2c_dev->base_phys + I2C_RX_FIFO;
+		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dma_sconfig.src_maxburst = dma_burst;
+	}
+
+	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
+	dmaengine_slave_config(chan, &dma_sconfig);
+}
+
 static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
 {
 	int err;
@@ -740,6 +925,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	u32 int_mask;
 	unsigned long time_left;
 	unsigned long flags;
+	size_t xfer_size;
+	u32 *buffer = 0;
+	int err = 0;
+	bool dma = false;
+	struct dma_chan *chan;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -749,19 +939,69 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
 	reinit_completion(&i2c_dev->msg_complete);
 
+	if (i2c_dev->msg_read)
+		xfer_size = msg->len;
+	else
+		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
+
+	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
+	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
+	if (dma) {
+		err = tegra_i2c_init_dma_param(i2c_dev);
+		if (err < 0) {
+			dev_dbg(i2c_dev->dev, "switching to PIO transfer\n");
+			dma = false;
+		}
+	}
+
+	i2c_dev->is_curr_dma_xfer = dma;
 	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
 
 	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
 
+	if (dma) {
+		if (i2c_dev->msg_read) {
+			chan = i2c_dev->rx_dma_chan;
+			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
+						   DATA_DMA_DIR_RX);
+			dma_sync_single_for_device(i2c_dev->dev,
+						   i2c_dev->dma_phys,
+						   xfer_size,
+						   DMA_FROM_DEVICE);
+			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
+			if (err < 0) {
+				dev_err(i2c_dev->dev,
+					"starting RX DMA failed, err %d\n",
+					err);
+				goto unlock;
+			}
+		} else {
+			chan = i2c_dev->tx_dma_chan;
+			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
+						   DATA_DMA_DIR_TX);
+			dma_sync_single_for_cpu(i2c_dev->dev,
+						i2c_dev->dma_phys,
+						xfer_size,
+						DMA_TO_DEVICE);
+			buffer = i2c_dev->dma_buf;
+		}
+	}
+
 	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
 			PACKET_HEADER0_PROTOCOL_I2C |
 			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
 			(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
-	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+	if (dma && !i2c_dev->msg_read)
+		*buffer++ = packet_header;
+	else
+		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
 
 	packet_header = msg->len - 1;
-	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+	if (dma && !i2c_dev->msg_read)
+		*buffer++ = packet_header;
+	else
+		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
 
 	packet_header = I2C_HEADER_IE_ENABLE;
 	if (end_state == MSG_END_CONTINUE)
@@ -778,30 +1018,79 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		packet_header |= I2C_HEADER_CONT_ON_NAK;
 	if (msg->flags & I2C_M_RD)
 		packet_header |= I2C_HEADER_READ;
-	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
-
-	if (!(msg->flags & I2C_M_RD))
-		tegra_i2c_fill_tx_fifo(i2c_dev);
-
+	if (dma && !i2c_dev->msg_read)
+		*buffer++ = packet_header;
+	else
+		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
+
+	if (!i2c_dev->msg_read) {
+		if (dma) {
+			memcpy(buffer, msg->buf, msg->len);
+			dma_sync_single_for_device(i2c_dev->dev,
+						   i2c_dev->dma_phys,
+						   xfer_size,
+						   DMA_TO_DEVICE);
+			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
+			if (err < 0) {
+				dev_err(i2c_dev->dev,
+					"starting TX DMA failed, err %d\n",
+					err);
+				goto unlock;
+			}
+		} else {
+			tegra_i2c_fill_tx_fifo(i2c_dev);
+		}
+	}
 	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
 		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
-	if (msg->flags & I2C_M_RD)
-		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
-	else if (i2c_dev->msg_buf_remaining)
-		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+	if (!dma) {
+		if (msg->flags & I2C_M_RD)
+			int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
+		else if (i2c_dev->msg_buf_remaining)
+			int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
+	}
 
 	tegra_i2c_unmask_irq(i2c_dev, int_mask);
-	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
 	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
 		i2c_readl(i2c_dev, I2C_INT_MASK));
 
+unlock:
+	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
+
+	if (dma) {
+		if (err)
+			return err;
+
+		time_left = wait_for_completion_timeout(
+						&i2c_dev->dma_complete,
+						TEGRA_I2C_TIMEOUT);
+
+		if (time_left == 0) {
+			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
+			dmaengine_terminate_all(chan);
+			tegra_i2c_init(i2c_dev);
+			return -ETIMEDOUT;
+		}
+
+		if (i2c_dev->msg_read) {
+			if (likely(i2c_dev->msg_err == I2C_ERR_NONE)) {
+				dma_sync_single_for_cpu(i2c_dev->dev,
+							i2c_dev->dma_phys,
+							xfer_size,
+							DMA_FROM_DEVICE);
+
+				memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
+					msg->len);
+			}
+		}
+	}
+
 	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
 						TEGRA_I2C_TIMEOUT);
 	tegra_i2c_mask_irq(i2c_dev, int_mask);
 
 	if (time_left == 0) {
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
-
 		tegra_i2c_init(i2c_dev);
 		return -ETIMEDOUT;
 	}
@@ -884,6 +1173,8 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
 
 	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
 			"multi-master");
+
+	i2c_dev->has_dma = of_property_read_bool(np, "dmas");
 }
 
 static const struct i2c_algorithm tegra_i2c_algo = {
@@ -1002,11 +1293,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	struct clk *div_clk;
 	struct clk *fast_clk;
 	void __iomem *base;
+	phys_addr_t base_phys;
 	int irq;
 	int ret = 0;
 	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	base_phys = res->start;
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -1029,6 +1322,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	i2c_dev->base = base;
+	i2c_dev->base_phys = base_phys;
 	i2c_dev->div_clk = div_clk;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
@@ -1036,6 +1330,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->irq = irq;
 	i2c_dev->cont_id = pdev->id;
 	i2c_dev->dev = &pdev->dev;
+	i2c_dev->dma_buf_size = i2c_dev->adapter.quirks->max_write_len;
 
 	i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c");
 	if (IS_ERR(i2c_dev->rst)) {
@@ -1049,6 +1344,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
 						  "nvidia,tegra20-i2c-dvc");
 	init_completion(&i2c_dev->msg_complete);
+	init_completion(&i2c_dev->dma_complete);
 	spin_lock_init(&i2c_dev->xfer_lock);
 
 	if (!i2c_dev->hw->has_single_clk_source) {
@@ -1109,6 +1405,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
+	ret = tegra_i2c_init_dma_param(i2c_dev);
+	if (ret == -EPROBE_DEFER)
+		goto disable_div_clk;
+
 	ret = tegra_i2c_init(i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
@@ -1173,6 +1473,20 @@ static int tegra_i2c_remove(struct platform_device *pdev)
 	if (!i2c_dev->hw->has_single_clk_source)
 		clk_unprepare(i2c_dev->fast_clk);
 
+	if (i2c_dev->dma_buf)
+		dma_free_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
+				  i2c_dev->dma_buf, i2c_dev->dma_phys);
+
+	if (i2c_dev->tx_dma_chan) {
+		dma_release_channel(i2c_dev->tx_dma_chan);
+		i2c_dev->tx_dma_chan = NULL;
+	}
+
+	if (i2c_dev->rx_dma_chan) {
+		dma_release_channel(i2c_dev->rx_dma_chan);
+		i2c_dev->rx_dma_chan = NULL;
+	}
+
 	return 0;
 }
 
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 4/5] i2c: tegra: Update transfer timeout
  2019-01-31  6:16 ` Sowjanya Komatineni
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  -1 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

Tegra194 allows max of 64K bytes and Tegra186 and prior allows
max of 4K bytes of transfer per packet.

one sec timeout is not enough for transfers more than 10K bytes
at STD bus rate.

This patch updates I2C transfer timeout based on the transfer size
and I2C bus rate to allow enough time during max transfer size at
lower bus speed.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V8] : Added comment with explaination of xfer time calculation
 [V5/V6/V7] : Same as V4
 [V4] : V4 series includes bus clear support and this patch is updated with
	fixed timeout of 1sec for bus clear operation.
 [V3] : Same as V2
 [V2] : Added this patch in V2 series to allow enough time for data transfer
	to happen.
	This patch has dependency with DMA patch as TEGRA_I2C_TIMEOUT define
	takes argument with this patch.

 drivers/i2c/busses/i2c-tegra.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 025d63972e50..435518cd91b6 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,7 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
-#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
+#define TEGRA_I2C_TIMEOUT(ms) (msecs_to_jiffies(ms))
 #define BYTES_PER_FIFO_WORD 4
 
 #define I2C_CNFG				0x000
@@ -901,8 +901,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
 		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
 		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 
-		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-							TEGRA_I2C_TIMEOUT);
+		time_left = wait_for_completion_timeout(
+						&i2c_dev->msg_complete,
+						TEGRA_I2C_TIMEOUT(1000));
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "timed out for bus clear\n");
 			return -ETIMEDOUT;
@@ -930,6 +931,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	int err = 0;
 	bool dma = false;
 	struct dma_chan *chan;
+	u16 xfer_time = 100;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -945,6 +947,13 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
 
 	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
+	/*
+	 * Transfer time = Total bits / transfer rate
+	 * Total bits = 9 bits per byte (including ACK bit) + Start & stop bits
+	 */
+	xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * 1000,
+					i2c_dev->bus_clk_rate);
+
 	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
 	if (dma) {
 		err = tegra_i2c_init_dma_param(i2c_dev);
@@ -1063,7 +1072,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 		time_left = wait_for_completion_timeout(
 						&i2c_dev->dma_complete,
-						TEGRA_I2C_TIMEOUT);
+						TEGRA_I2C_TIMEOUT(xfer_time));
 
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
@@ -1086,7 +1095,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	}
 
 	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-						TEGRA_I2C_TIMEOUT);
+						TEGRA_I2C_TIMEOUT(xfer_time));
 	tegra_i2c_mask_irq(i2c_dev, int_mask);
 
 	if (time_left == 0) {
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 4/5] i2c: tegra: Update transfer timeout
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  0 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

Tegra194 allows max of 64K bytes and Tegra186 and prior allows
max of 4K bytes of transfer per packet.

one sec timeout is not enough for transfers more than 10K bytes
at STD bus rate.

This patch updates I2C transfer timeout based on the transfer size
and I2C bus rate to allow enough time during max transfer size at
lower bus speed.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V8] : Added comment with explaination of xfer time calculation
 [V5/V6/V7] : Same as V4
 [V4] : V4 series includes bus clear support and this patch is updated with
	fixed timeout of 1sec for bus clear operation.
 [V3] : Same as V2
 [V2] : Added this patch in V2 series to allow enough time for data transfer
	to happen.
	This patch has dependency with DMA patch as TEGRA_I2C_TIMEOUT define
	takes argument with this patch.

 drivers/i2c/busses/i2c-tegra.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 025d63972e50..435518cd91b6 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -25,7 +25,7 @@
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
 
-#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
+#define TEGRA_I2C_TIMEOUT(ms) (msecs_to_jiffies(ms))
 #define BYTES_PER_FIFO_WORD 4
 
 #define I2C_CNFG				0x000
@@ -901,8 +901,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
 		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
 		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
 
-		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-							TEGRA_I2C_TIMEOUT);
+		time_left = wait_for_completion_timeout(
+						&i2c_dev->msg_complete,
+						TEGRA_I2C_TIMEOUT(1000));
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "timed out for bus clear\n");
 			return -ETIMEDOUT;
@@ -930,6 +931,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	int err = 0;
 	bool dma = false;
 	struct dma_chan *chan;
+	u16 xfer_time = 100;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -945,6 +947,13 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
 
 	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
+	/*
+	 * Transfer time = Total bits / transfer rate
+	 * Total bits = 9 bits per byte (including ACK bit) + Start & stop bits
+	 */
+	xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * 1000,
+					i2c_dev->bus_clk_rate);
+
 	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
 	if (dma) {
 		err = tegra_i2c_init_dma_param(i2c_dev);
@@ -1063,7 +1072,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 		time_left = wait_for_completion_timeout(
 						&i2c_dev->dma_complete,
-						TEGRA_I2C_TIMEOUT);
+						TEGRA_I2C_TIMEOUT(xfer_time));
 
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
@@ -1086,7 +1095,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	}
 
 	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
-						TEGRA_I2C_TIMEOUT);
+						TEGRA_I2C_TIMEOUT(xfer_time));
 	tegra_i2c_mask_irq(i2c_dev, int_mask);
 
 	if (time_left == 0) {
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 5/5] i2c: tegra: Add I2C interface timing support
  2019-01-31  6:16 ` Sowjanya Komatineni
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  -1 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

This patch adds I2C interface timing registers support for
proper bus rate configuration along with meeting the i2c spec
setup and hold times based on the tuning performed on Tegra210,
Tegra186 and Tegra194 platforms.

I2C_INTERFACE_TIMING_0 register contains TLOW and THIGH field
and Tegra I2C controller design uses them as a part of internal
clock divisor.

I2C_INTERFACE_TIMING_1 register contains the setup and hold times
for start and stop conditions.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V8] : Updated to handle timing implementation within tegra_i2c_init directly
 [V7] : Minor updates to timing implementation
 [V5/V6] : Added this Interface timing patch in V5 of the patch series.

 drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 158 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 435518cd91b6..fe5b89abc576 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -130,6 +130,15 @@
 #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
 #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
 
+#define I2C_INTERFACE_TIMING_0			0x94
+#define I2C_THIGH_SHIFT				8
+#define I2C_INTERFACE_TIMING_1			0x98
+
+#define I2C_STANDARD_MODE			100000
+#define I2C_FAST_MODE				400000
+#define I2C_FAST_PLUS_MODE			1000000
+#define I2C_HS_MODE				3500000
+
 /* Packet header size in bytes */
 #define I2C_PACKET_HEADER_SIZE			12
 
@@ -167,7 +176,10 @@ enum msg_end_type {
  * @has_config_load_reg: Has the config load register to load the new
  *		configuration.
  * @clk_divisor_hs_mode: Clock divisor in HS mode.
- * @clk_divisor_std_fast_mode: Clock divisor in standard/fast mode. It is
+ * @clk_divisor_std_mode: Clock divisor in standard mode. It is
+ *		applicable if there is no fast clock source i.e. single clock
+ *		source.
+ * @clk_divisor_fast_mode: Clock divisor in fast mode. It is
  *		applicable if there is no fast clock source i.e. single clock
  *		source.
  * @clk_divisor_fast_plus_mode: Clock divisor in fast mode plus. It is
@@ -182,6 +194,16 @@ enum msg_end_type {
  *		be transferred in one go.
  * @supports_bus_clear: Bus Clear support to recover from bus hang during
  *		SDA stuck low from device for some unknown reasons.
+ * @tlow_std_mode: Low period of the clock in standard mode.
+ * @thigh_std_mode: High period of the clock in standard mode.
+ * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
+ * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
+ * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
+ *		in standard mode.
+ * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
+ *		conditions in fast/fast-plus modes.
+ * @setup_hold_time_hs_mode: Setup and hold time for start and stop conditions
+ *		in HS mode.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -189,12 +211,20 @@ struct tegra_i2c_hw_feature {
 	bool has_single_clk_source;
 	bool has_config_load_reg;
 	int clk_divisor_hs_mode;
-	int clk_divisor_std_fast_mode;
+	int clk_divisor_std_mode;
+	int clk_divisor_fast_mode;
 	u16 clk_divisor_fast_plus_mode;
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
 	bool supports_bus_clear;
+	u8 tlow_std_mode;
+	u8 thigh_std_mode;
+	u8 tlow_fast_fastplus_mode;
+	u8 thigh_fast_fastplus_mode;
+	u32 setup_hold_time_std_mode;
+	u32 setup_hold_time_fast_fast_plus_mode;
+	u32 setup_hold_time_hs_mode;
 };
 
 /**
@@ -640,11 +670,13 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
-static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
 {
 	u32 val;
 	int err;
-	u32 clk_divisor;
+	u32 clk_divisor, clk_multiplier;
+	u32 tsu_thd = 0;
+	u8 tlow, thigh;
 
 	err = pm_runtime_get_sync(i2c_dev->dev);
 	if (err < 0) {
@@ -674,6 +706,36 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 					I2C_CLK_DIVISOR_STD_FAST_MODE_SHIFT;
 	i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
 
+	if ((i2c_dev->bus_clk_rate > I2C_STANDARD_MODE) &&
+	    (i2c_dev->bus_clk_rate <= I2C_FAST_PLUS_MODE)) {
+		tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
+		thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
+		tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
+	} else {
+		tlow = i2c_dev->hw->tlow_std_mode;
+		thigh = i2c_dev->hw->thigh_std_mode;
+		tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
+	}
+
+	val = (thigh << I2C_THIGH_SHIFT) | tlow;
+	if (val)
+		i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0);
+
+	if (tsu_thd)
+		i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
+
+	if (!clk_reinit) {
+		clk_multiplier = (tlow + thigh + 2);
+		clk_multiplier *= (i2c_dev->clk_divisor_non_hs_mode + 1);
+		err = clk_set_rate(i2c_dev->div_clk,
+				   i2c_dev->bus_clk_rate * clk_multiplier);
+		if (err) {
+			dev_err(i2c_dev->dev,
+				"failed changing clock rate: %d\n", err);
+			goto err;
+		}
+	}
+
 	if (!i2c_dev->is_dvc) {
 		u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
 
@@ -1077,7 +1139,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
 			dmaengine_terminate_all(chan);
-			tegra_i2c_init(i2c_dev);
+			tegra_i2c_init(i2c_dev, true);
 			return -ETIMEDOUT;
 		}
 
@@ -1100,7 +1162,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	if (time_left == 0) {
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
-		tegra_i2c_init(i2c_dev);
+		tegra_i2c_init(i2c_dev, true);
 		return -ETIMEDOUT;
 	}
 
@@ -1111,7 +1173,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
 		return 0;
 
-	tegra_i2c_init(i2c_dev);
+	tegra_i2c_init(i2c_dev, true);
 	/* start recovery upon arbitration loss in single master mode */
 	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
 		if (!i2c_dev->is_multimaster_mode)
@@ -1203,13 +1265,21 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = false,
 	.has_single_clk_source = false,
 	.clk_divisor_hs_mode = 3,
-	.clk_divisor_std_fast_mode = 0,
+	.clk_divisor_std_mode = 0,
+	.clk_divisor_fast_mode = 0,
 	.clk_divisor_fast_plus_mode = 0,
 	.has_config_load_reg = false,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
 	.supports_bus_clear = false,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1217,13 +1287,21 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = false,
 	.has_single_clk_source = false,
 	.clk_divisor_hs_mode = 3,
-	.clk_divisor_std_fast_mode = 0,
+	.clk_divisor_std_mode = 0,
+	.clk_divisor_fast_mode = 0,
 	.clk_divisor_fast_plus_mode = 0,
 	.has_config_load_reg = false,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
 	.supports_bus_clear = false,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1231,13 +1309,21 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x19,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = false,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1245,13 +1331,21 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x19,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = true,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1259,32 +1353,71 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x19,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = true,
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0,
+	.setup_hold_time_fast_fast_plus_mode = 0,
+	.setup_hold_time_hs_mode = 0,
 };
 
-static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
+static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.has_continue_xfer_support = true,
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x16,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = true,
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x3,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0,
+	.setup_hold_time_fast_fast_plus_mode = 0,
+	.setup_hold_time_hs_mode = 0,
+};
+
+static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
+	.has_continue_xfer_support = true,
+	.has_per_pkt_xfer_complete_irq = true,
+	.has_single_clk_source = true,
+	.clk_divisor_hs_mode = 1,
+	.clk_divisor_std_mode = 0x4f,
+	.clk_divisor_fast_mode = 0x3c,
+	.clk_divisor_fast_plus_mode = 0x16,
+	.has_config_load_reg = true,
+	.has_multi_master_mode = true,
+	.has_slcg_override_reg = true,
+	.has_mst_fifo = true,
+	.supports_bus_clear = true,
+	.tlow_std_mode = 0x8,
+	.thigh_std_mode = 0x7,
+	.tlow_fast_fastplus_mode = 0x2,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x08080808,
+	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
+	.setup_hold_time_hs_mode = 0x090909,
 };
 
 /* Match table for of_platform binding */
 static const struct of_device_id tegra_i2c_of_match[] = {
 	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
+	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
 	{ .compatible = "nvidia,tegra210-i2c", .data = &tegra210_i2c_hw, },
 	{ .compatible = "nvidia,tegra124-i2c", .data = &tegra124_i2c_hw, },
 	{ .compatible = "nvidia,tegra114-i2c", .data = &tegra114_i2c_hw, },
@@ -1305,7 +1438,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	phys_addr_t base_phys;
 	int irq;
 	int ret = 0;
-	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base_phys = res->start;
@@ -1375,20 +1507,17 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
-	i2c_dev->clk_divisor_non_hs_mode =
-			i2c_dev->hw->clk_divisor_std_fast_mode;
-	if (i2c_dev->hw->clk_divisor_fast_plus_mode &&
-		(i2c_dev->bus_clk_rate == 1000000))
+	if ((i2c_dev->bus_clk_rate > I2C_FAST_MODE) &&
+	    (i2c_dev->bus_clk_rate <= I2C_FAST_PLUS_MODE))
 		i2c_dev->clk_divisor_non_hs_mode =
-			i2c_dev->hw->clk_divisor_fast_plus_mode;
-
-	clk_multiplier *= (i2c_dev->clk_divisor_non_hs_mode + 1);
-	ret = clk_set_rate(i2c_dev->div_clk,
-			   i2c_dev->bus_clk_rate * clk_multiplier);
-	if (ret) {
-		dev_err(i2c_dev->dev, "Clock rate change failed %d\n", ret);
-		goto unprepare_fast_clk;
-	}
+				i2c_dev->hw->clk_divisor_fast_plus_mode;
+	else if ((i2c_dev->bus_clk_rate > I2C_STANDARD_MODE) &&
+		 (i2c_dev->bus_clk_rate <= I2C_FAST_MODE))
+		i2c_dev->clk_divisor_non_hs_mode =
+				i2c_dev->hw->clk_divisor_fast_mode;
+	else
+		i2c_dev->clk_divisor_non_hs_mode =
+				i2c_dev->hw->clk_divisor_std_mode;
 
 	ret = clk_prepare(i2c_dev->div_clk);
 	if (ret < 0) {
@@ -1418,7 +1547,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	if (ret == -EPROBE_DEFER)
 		goto disable_div_clk;
 
-	ret = tegra_i2c_init(i2c_dev);
+	ret = tegra_i2c_init(i2c_dev, false);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
 		goto disable_div_clk;
-- 
2.7.4

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* [PATCH V8 5/5] i2c: tegra: Add I2C interface timing support
@ 2019-01-31  6:16   ` Sowjanya Komatineni
  0 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31  6:16 UTC (permalink / raw)
  To: thierry.reding, jonathanh, mkarthik, smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c, Sowjanya Komatineni

This patch adds I2C interface timing registers support for
proper bus rate configuration along with meeting the i2c spec
setup and hold times based on the tuning performed on Tegra210,
Tegra186 and Tegra194 platforms.

I2C_INTERFACE_TIMING_0 register contains TLOW and THIGH field
and Tegra I2C controller design uses them as a part of internal
clock divisor.

I2C_INTERFACE_TIMING_1 register contains the setup and hold times
for start and stop conditions.

Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
---
 [V8] : Updated to handle timing implementation within tegra_i2c_init directly
 [V7] : Minor updates to timing implementation
 [V5/V6] : Added this Interface timing patch in V5 of the patch series.

 drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++++++++++++++++++++++++-------
 1 file changed, 158 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 435518cd91b6..fe5b89abc576 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -130,6 +130,15 @@
 #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
 #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
 
+#define I2C_INTERFACE_TIMING_0			0x94
+#define I2C_THIGH_SHIFT				8
+#define I2C_INTERFACE_TIMING_1			0x98
+
+#define I2C_STANDARD_MODE			100000
+#define I2C_FAST_MODE				400000
+#define I2C_FAST_PLUS_MODE			1000000
+#define I2C_HS_MODE				3500000
+
 /* Packet header size in bytes */
 #define I2C_PACKET_HEADER_SIZE			12
 
@@ -167,7 +176,10 @@ enum msg_end_type {
  * @has_config_load_reg: Has the config load register to load the new
  *		configuration.
  * @clk_divisor_hs_mode: Clock divisor in HS mode.
- * @clk_divisor_std_fast_mode: Clock divisor in standard/fast mode. It is
+ * @clk_divisor_std_mode: Clock divisor in standard mode. It is
+ *		applicable if there is no fast clock source i.e. single clock
+ *		source.
+ * @clk_divisor_fast_mode: Clock divisor in fast mode. It is
  *		applicable if there is no fast clock source i.e. single clock
  *		source.
  * @clk_divisor_fast_plus_mode: Clock divisor in fast mode plus. It is
@@ -182,6 +194,16 @@ enum msg_end_type {
  *		be transferred in one go.
  * @supports_bus_clear: Bus Clear support to recover from bus hang during
  *		SDA stuck low from device for some unknown reasons.
+ * @tlow_std_mode: Low period of the clock in standard mode.
+ * @thigh_std_mode: High period of the clock in standard mode.
+ * @tlow_fast_fastplus_mode: Low period of the clock in fast/fast-plus modes.
+ * @thigh_fast_fastplus_mode: High period of the clock in fast/fast-plus modes.
+ * @setup_hold_time_std_mode: Setup and hold time for start and stop conditions
+ *		in standard mode.
+ * @setup_hold_time_fast_fast_plus_mode: Setup and hold time for start and stop
+ *		conditions in fast/fast-plus modes.
+ * @setup_hold_time_hs_mode: Setup and hold time for start and stop conditions
+ *		in HS mode.
  */
 struct tegra_i2c_hw_feature {
 	bool has_continue_xfer_support;
@@ -189,12 +211,20 @@ struct tegra_i2c_hw_feature {
 	bool has_single_clk_source;
 	bool has_config_load_reg;
 	int clk_divisor_hs_mode;
-	int clk_divisor_std_fast_mode;
+	int clk_divisor_std_mode;
+	int clk_divisor_fast_mode;
 	u16 clk_divisor_fast_plus_mode;
 	bool has_multi_master_mode;
 	bool has_slcg_override_reg;
 	bool has_mst_fifo;
 	bool supports_bus_clear;
+	u8 tlow_std_mode;
+	u8 thigh_std_mode;
+	u8 tlow_fast_fastplus_mode;
+	u8 thigh_fast_fastplus_mode;
+	u32 setup_hold_time_std_mode;
+	u32 setup_hold_time_fast_fast_plus_mode;
+	u32 setup_hold_time_hs_mode;
 };
 
 /**
@@ -640,11 +670,13 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
-static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
+static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev, bool clk_reinit)
 {
 	u32 val;
 	int err;
-	u32 clk_divisor;
+	u32 clk_divisor, clk_multiplier;
+	u32 tsu_thd = 0;
+	u8 tlow, thigh;
 
 	err = pm_runtime_get_sync(i2c_dev->dev);
 	if (err < 0) {
@@ -674,6 +706,36 @@ static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 					I2C_CLK_DIVISOR_STD_FAST_MODE_SHIFT;
 	i2c_writel(i2c_dev, clk_divisor, I2C_CLK_DIVISOR);
 
+	if ((i2c_dev->bus_clk_rate > I2C_STANDARD_MODE) &&
+	    (i2c_dev->bus_clk_rate <= I2C_FAST_PLUS_MODE)) {
+		tlow = i2c_dev->hw->tlow_fast_fastplus_mode;
+		thigh = i2c_dev->hw->thigh_fast_fastplus_mode;
+		tsu_thd = i2c_dev->hw->setup_hold_time_fast_fast_plus_mode;
+	} else {
+		tlow = i2c_dev->hw->tlow_std_mode;
+		thigh = i2c_dev->hw->thigh_std_mode;
+		tsu_thd = i2c_dev->hw->setup_hold_time_std_mode;
+	}
+
+	val = (thigh << I2C_THIGH_SHIFT) | tlow;
+	if (val)
+		i2c_writel(i2c_dev, val, I2C_INTERFACE_TIMING_0);
+
+	if (tsu_thd)
+		i2c_writel(i2c_dev, tsu_thd, I2C_INTERFACE_TIMING_1);
+
+	if (!clk_reinit) {
+		clk_multiplier = (tlow + thigh + 2);
+		clk_multiplier *= (i2c_dev->clk_divisor_non_hs_mode + 1);
+		err = clk_set_rate(i2c_dev->div_clk,
+				   i2c_dev->bus_clk_rate * clk_multiplier);
+		if (err) {
+			dev_err(i2c_dev->dev,
+				"failed changing clock rate: %d\n", err);
+			goto err;
+		}
+	}
+
 	if (!i2c_dev->is_dvc) {
 		u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
 
@@ -1077,7 +1139,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 		if (time_left == 0) {
 			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
 			dmaengine_terminate_all(chan);
-			tegra_i2c_init(i2c_dev);
+			tegra_i2c_init(i2c_dev, true);
 			return -ETIMEDOUT;
 		}
 
@@ -1100,7 +1162,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 
 	if (time_left == 0) {
 		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
-		tegra_i2c_init(i2c_dev);
+		tegra_i2c_init(i2c_dev, true);
 		return -ETIMEDOUT;
 	}
 
@@ -1111,7 +1173,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
 		return 0;
 
-	tegra_i2c_init(i2c_dev);
+	tegra_i2c_init(i2c_dev, true);
 	/* start recovery upon arbitration loss in single master mode */
 	if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
 		if (!i2c_dev->is_multimaster_mode)
@@ -1203,13 +1265,21 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = false,
 	.has_single_clk_source = false,
 	.clk_divisor_hs_mode = 3,
-	.clk_divisor_std_fast_mode = 0,
+	.clk_divisor_std_mode = 0,
+	.clk_divisor_fast_mode = 0,
 	.clk_divisor_fast_plus_mode = 0,
 	.has_config_load_reg = false,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
 	.supports_bus_clear = false,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1217,13 +1287,21 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = false,
 	.has_single_clk_source = false,
 	.clk_divisor_hs_mode = 3,
-	.clk_divisor_std_fast_mode = 0,
+	.clk_divisor_std_mode = 0,
+	.clk_divisor_fast_mode = 0,
 	.clk_divisor_fast_plus_mode = 0,
 	.has_config_load_reg = false,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
 	.supports_bus_clear = false,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1231,13 +1309,21 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x19,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = false,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = false,
 	.has_mst_fifo = false,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1245,13 +1331,21 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x19,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = true,
 	.has_multi_master_mode = false,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x0,
+	.setup_hold_time_fast_fast_plus_mode = 0x0,
+	.setup_hold_time_hs_mode = 0x0,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1259,32 +1353,71 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x19,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = true,
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = false,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x2,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0,
+	.setup_hold_time_fast_fast_plus_mode = 0,
+	.setup_hold_time_hs_mode = 0,
 };
 
-static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
+static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.has_continue_xfer_support = true,
 	.has_per_pkt_xfer_complete_irq = true,
 	.has_single_clk_source = true,
 	.clk_divisor_hs_mode = 1,
-	.clk_divisor_std_fast_mode = 0x19,
+	.clk_divisor_std_mode = 0x16,
+	.clk_divisor_fast_mode = 0x19,
 	.clk_divisor_fast_plus_mode = 0x10,
 	.has_config_load_reg = true,
 	.has_multi_master_mode = true,
 	.has_slcg_override_reg = true,
 	.has_mst_fifo = true,
 	.supports_bus_clear = true,
+	.tlow_std_mode = 0x4,
+	.thigh_std_mode = 0x3,
+	.tlow_fast_fastplus_mode = 0x4,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0,
+	.setup_hold_time_fast_fast_plus_mode = 0,
+	.setup_hold_time_hs_mode = 0,
+};
+
+static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
+	.has_continue_xfer_support = true,
+	.has_per_pkt_xfer_complete_irq = true,
+	.has_single_clk_source = true,
+	.clk_divisor_hs_mode = 1,
+	.clk_divisor_std_mode = 0x4f,
+	.clk_divisor_fast_mode = 0x3c,
+	.clk_divisor_fast_plus_mode = 0x16,
+	.has_config_load_reg = true,
+	.has_multi_master_mode = true,
+	.has_slcg_override_reg = true,
+	.has_mst_fifo = true,
+	.supports_bus_clear = true,
+	.tlow_std_mode = 0x8,
+	.thigh_std_mode = 0x7,
+	.tlow_fast_fastplus_mode = 0x2,
+	.thigh_fast_fastplus_mode = 0x2,
+	.setup_hold_time_std_mode = 0x08080808,
+	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
+	.setup_hold_time_hs_mode = 0x090909,
 };
 
 /* Match table for of_platform binding */
 static const struct of_device_id tegra_i2c_of_match[] = {
 	{ .compatible = "nvidia,tegra194-i2c", .data = &tegra194_i2c_hw, },
+	{ .compatible = "nvidia,tegra186-i2c", .data = &tegra186_i2c_hw, },
 	{ .compatible = "nvidia,tegra210-i2c", .data = &tegra210_i2c_hw, },
 	{ .compatible = "nvidia,tegra124-i2c", .data = &tegra124_i2c_hw, },
 	{ .compatible = "nvidia,tegra114-i2c", .data = &tegra114_i2c_hw, },
@@ -1305,7 +1438,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	phys_addr_t base_phys;
 	int irq;
 	int ret = 0;
-	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	base_phys = res->start;
@@ -1375,20 +1507,17 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
-	i2c_dev->clk_divisor_non_hs_mode =
-			i2c_dev->hw->clk_divisor_std_fast_mode;
-	if (i2c_dev->hw->clk_divisor_fast_plus_mode &&
-		(i2c_dev->bus_clk_rate == 1000000))
+	if ((i2c_dev->bus_clk_rate > I2C_FAST_MODE) &&
+	    (i2c_dev->bus_clk_rate <= I2C_FAST_PLUS_MODE))
 		i2c_dev->clk_divisor_non_hs_mode =
-			i2c_dev->hw->clk_divisor_fast_plus_mode;
-
-	clk_multiplier *= (i2c_dev->clk_divisor_non_hs_mode + 1);
-	ret = clk_set_rate(i2c_dev->div_clk,
-			   i2c_dev->bus_clk_rate * clk_multiplier);
-	if (ret) {
-		dev_err(i2c_dev->dev, "Clock rate change failed %d\n", ret);
-		goto unprepare_fast_clk;
-	}
+				i2c_dev->hw->clk_divisor_fast_plus_mode;
+	else if ((i2c_dev->bus_clk_rate > I2C_STANDARD_MODE) &&
+		 (i2c_dev->bus_clk_rate <= I2C_FAST_MODE))
+		i2c_dev->clk_divisor_non_hs_mode =
+				i2c_dev->hw->clk_divisor_fast_mode;
+	else
+		i2c_dev->clk_divisor_non_hs_mode =
+				i2c_dev->hw->clk_divisor_std_mode;
 
 	ret = clk_prepare(i2c_dev->div_clk);
 	if (ret < 0) {
@@ -1418,7 +1547,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	if (ret == -EPROBE_DEFER)
 		goto disable_div_clk;
 
-	ret = tegra_i2c_init(i2c_dev);
+	ret = tegra_i2c_init(i2c_dev, false);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
 		goto disable_div_clk;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31  6:16   ` Sowjanya Komatineni
  (?)
@ 2019-01-31 12:44   ` Thierry Reding
  2019-01-31 16:41     ` Sowjanya Komatineni
  2019-02-01  0:52     ` Dmitry Osipenko
  -1 siblings, 2 replies; 32+ messages in thread
From: Thierry Reding @ 2019-01-31 12:44 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: jonathanh, mkarthik, smohammed, talho, linux-tegra, linux-kernel,
	linux-i2c

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

On Wed, Jan 30, 2019 at 10:16:25PM -0800, Sowjanya Komatineni wrote:
> This patch adds DMA support for Tegra I2C.
> 
> Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
> transfer size of the max FIFO depth and DMA mode is used for
> transfer size higher than max FIFO depth to save CPU overhead.
> 
> PIO mode needs full intervention of CPU to fill or empty FIFO's
> and also need to service multiple data requests interrupt for the
> same transaction. This adds delay between data bytes of the same
> transfer when CPU is fully loaded and some slave devices has
> internal timeout for no bus activity and stops transaction to
> avoid bus hang. DMA mode is helpful in such cases.
> 
> DMA mode is also helpful for Large transfers during downloading or
> uploading FW over I2C to some external devices.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V8] : Moved back dma init to i2c probe, removed ALL_PACKETS_XFER_COMPLETE
> 	interrupt and using PACKETS_XFER_COMPLETE interrupt only and some
> 	other fixes
> 	Updated Kconfig for APB_DMA dependency
>  [V7] : Same as V6
>  [V6] : Updated for proper buffer allocation/freeing, channel release.
> 	Updated to use exact xfer size for syncing dma buffer.
>  [V5] : Same as V4
>  [V4] : Updated to allocate DMA buffer only when DMA mode.
> 	Updated to fall back to PIO mode when DMA channel request or
> 	buffer allocation fails.
>  [V3] : Updated without additional buffer allocation.
>  [V2] : Updated based on V1 review feedback along with code cleanup for
> 	proper implementation of DMA.
> 
>  drivers/i2c/busses/Kconfig     |   2 +-
>  drivers/i2c/busses/i2c-tegra.c | 362 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 339 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> index f2c681971201..046aeb92a467 100644
> --- a/drivers/i2c/busses/Kconfig
> +++ b/drivers/i2c/busses/Kconfig
> @@ -1016,7 +1016,7 @@ config I2C_SYNQUACER
>  
>  config I2C_TEGRA
>  	tristate "NVIDIA Tegra internal I2C controller"
> -	depends on ARCH_TEGRA
> +	depends on (ARCH_TEGRA && TEGRA20_APB_DMA)

Like I said in my reply in the v7 subthread, I don't think we want this.
The dependency that we have is on the DMA engine API, not the APB DMA
driver.

Technically there could be a runtime problem if the APB DMA driver is
disabled and we list a "dmas" property. If I understand correctly, the
DMA engine API would always return -EPROBE_DEFER in that case. That's
somewhat annoying, but I think that's fine because it points at an
integration issue. It lets you know that the driver is relying on a
resources that is not showing up, which usually means that either the
provider's driver is not enabled or the provider is failing to probe.

>  	help
>  	  If you say yes to this option, support will be included for the
>  	  I2C controller embedded in NVIDIA Tegra SOCs
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index c4892a47a483..025d63972e50 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -8,6 +8,9 @@
>  
>  #include <linux/clk.h>
>  #include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dmapool.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/init.h>
> @@ -44,6 +47,8 @@
>  #define I2C_FIFO_CONTROL_RX_FLUSH		BIT(0)
>  #define I2C_FIFO_CONTROL_TX_TRIG_SHIFT		5
>  #define I2C_FIFO_CONTROL_RX_TRIG_SHIFT		2
> +#define I2C_FIFO_CONTROL_TX_TRIG(x)		(((x) - 1) << 5)
> +#define I2C_FIFO_CONTROL_RX_TRIG(x)		(((x) - 1) << 2)
>  #define I2C_FIFO_STATUS				0x060
>  #define I2C_FIFO_STATUS_TX_MASK			0xF0
>  #define I2C_FIFO_STATUS_TX_SHIFT		4
> @@ -125,6 +130,19 @@
>  #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
>  #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
>  
> +/* Packet header size in bytes */
> +#define I2C_PACKET_HEADER_SIZE			12
> +
> +#define DATA_DMA_DIR_TX				(1 << 0)
> +#define DATA_DMA_DIR_RX				(1 << 1)
> +
> +/*
> + * Upto I2C_PIO_MODE_MAX_LEN bytes, controller will use PIO mode,
> + * above this, controller will use DMA to fill FIFO.
> + * MAX PIO len is 20 bytes excluding packet header.
> + */
> +#define I2C_PIO_MODE_MAX_LEN			32
> +
>  /*
>   * msg_end_type: The bus control which need to be send at end of transfer.
>   * @MSG_END_STOP: Send stop pulse at end of transfer.
> @@ -188,6 +206,7 @@ struct tegra_i2c_hw_feature {
>   * @fast_clk: clock reference for fast clock of I2C controller
>   * @rst: reset control for the I2C controller
>   * @base: ioremapped registers cookie
> + * @base_phys: Physical base address of the I2C controller
>   * @cont_id: I2C controller ID, used for packet header
>   * @irq: IRQ number of transfer complete interrupt
>   * @irq_disabled: used to track whether or not the interrupt is enabled
> @@ -201,6 +220,14 @@ struct tegra_i2c_hw_feature {
>   * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
>   * @is_multimaster_mode: track if I2C controller is in multi-master mode
>   * @xfer_lock: lock to serialize transfer submission and processing
> + * @has_dma: indicates if DMA can be utilized based on dma DT bindings

I don't think we need this. We can just rely on the DMA engine API to
tell us if the "dmas" property isn't there.

> + * @tx_dma_chan: DMA transmit channel
> + * @rx_dma_chan: DMA receive channel
> + * @dma_phys: handle to DMA resources
> + * @dma_buf: pointer to allocated DMA buffer
> + * @dma_buf_size: DMA buffer size
> + * @is_curr_dma_xfer: indicates active DMA transfer
> + * @dma_complete: DMA completion notifier
>   */
>  struct tegra_i2c_dev {
>  	struct device *dev;
> @@ -210,6 +237,7 @@ struct tegra_i2c_dev {
>  	struct clk *fast_clk;
>  	struct reset_control *rst;
>  	void __iomem *base;
> +	phys_addr_t base_phys;
>  	int cont_id;
>  	int irq;
>  	bool irq_disabled;
> @@ -223,6 +251,14 @@ struct tegra_i2c_dev {
>  	u16 clk_divisor_non_hs_mode;
>  	bool is_multimaster_mode;
>  	spinlock_t xfer_lock;
> +	bool has_dma;
> +	struct dma_chan *tx_dma_chan;
> +	struct dma_chan *rx_dma_chan;
> +	dma_addr_t dma_phys;
> +	u32 *dma_buf;
> +	unsigned int dma_buf_size;
> +	bool is_curr_dma_xfer;
> +	struct completion dma_complete;
>  };
>  
>  static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
> @@ -291,6 +327,85 @@ static void tegra_i2c_unmask_irq(struct tegra_i2c_dev *i2c_dev, u32 mask)
>  	i2c_writel(i2c_dev, int_mask, I2C_INT_MASK);
>  }
>  
> +static void tegra_i2c_dma_complete(void *args)
> +{
> +	struct tegra_i2c_dev *i2c_dev = args;
> +
> +	complete(&i2c_dev->dma_complete);
> +}
> +
> +static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len)
> +{
> +	struct dma_async_tx_descriptor *dma_desc;
> +	enum dma_transfer_direction dir;
> +	struct dma_chan *chan;
> +
> +	dev_dbg(i2c_dev->dev, "starting DMA for length: %zu\n", len);
> +	reinit_completion(&i2c_dev->dma_complete);
> +	dir = i2c_dev->msg_read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
> +	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
> +	dma_desc = dmaengine_prep_slave_single(chan, i2c_dev->dma_phys,
> +					       len, dir, DMA_PREP_INTERRUPT |
> +					       DMA_CTRL_ACK);
> +	if (!dma_desc) {
> +		dev_err(i2c_dev->dev, "failed to get DMA descriptor\n");
> +		return -EIO;
> +	}
> +
> +	dma_desc->callback = tegra_i2c_dma_complete;
> +	dma_desc->callback_param = i2c_dev;
> +	dmaengine_submit(dma_desc);
> +	dma_async_issue_pending(chan);
> +	return 0;
> +}
> +
> +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev)
> +{
> +	struct dma_chan *dma_chan;
> +	u32 *dma_buf;
> +	dma_addr_t dma_phys;
> +
> +	if (!i2c_dev->has_dma)
> +		return -EINVAL;
> +
> +	if (!i2c_dev->rx_dma_chan) {
> +		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "rx");
> +		if (IS_ERR(dma_chan))
> +			return PTR_ERR(dma_chan);

I think we want to fallback to PIO here if dma_chan is -ENODEV.

> +
> +		i2c_dev->rx_dma_chan = dma_chan;
> +	}
> +
> +	if (!i2c_dev->tx_dma_chan) {
> +		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "tx");
> +		if (IS_ERR(dma_chan))
> +			return PTR_ERR(dma_chan);

Same here. We could use rx_dma_chan == NULL as a condition to detect
that instead of the extra has_dma.

> +		i2c_dev->tx_dma_chan = dma_chan;
> +	}

Although, I'm not exactly sure I understand what you're trying to
achieve here. Shouldn't we move the channel request parts into probe and
remove them from here? Otherwise it seems like we could get into a state
where we keep trying to get the slave channels everytime we set up a DMA
transfer, even if we already failed to do so during probe.

> +
> +	if (!i2c_dev->dma_buf && i2c_dev->msg_buf_remaining) {
> +		dma_buf = dma_alloc_coherent(i2c_dev->dev,
> +					     i2c_dev->dma_buf_size,
> +					     &dma_phys, GFP_KERNEL);
> +
> +		if (!dma_buf) {
> +			dev_err(i2c_dev->dev,
> +				"failed to allocate the DMA buffer\n");
> +			dma_release_channel(i2c_dev->tx_dma_chan);
> +			dma_release_channel(i2c_dev->rx_dma_chan);
> +			i2c_dev->tx_dma_chan = NULL;
> +			i2c_dev->rx_dma_chan = NULL;
> +			return -ENOMEM;
> +		}
> +
> +		i2c_dev->dma_buf = dma_buf;
> +		i2c_dev->dma_phys = dma_phys;
> +	}
> +
> +	return 0;
> +}
> +
>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>  {
>  	unsigned long timeout = jiffies + HZ;
> @@ -656,25 +771,38 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	if (i2c_dev->hw->supports_bus_clear && (status & I2C_INT_BUS_CLR_DONE))
>  		goto err;
>  
> -	if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
> -		if (i2c_dev->msg_buf_remaining)
> -			tegra_i2c_empty_rx_fifo(i2c_dev);
> -		else
> -			BUG();
> -	}
> +	if (!i2c_dev->is_curr_dma_xfer) {
> +		if (i2c_dev->msg_read && (status & I2C_INT_RX_FIFO_DATA_REQ)) {
> +			if (i2c_dev->msg_buf_remaining)
> +				tegra_i2c_empty_rx_fifo(i2c_dev);
> +			else
> +				BUG();
> +		}
>  
> -	if (!i2c_dev->msg_read && (status & I2C_INT_TX_FIFO_DATA_REQ)) {
> -		if (i2c_dev->msg_buf_remaining)
> -			tegra_i2c_fill_tx_fifo(i2c_dev);
> -		else
> -			tegra_i2c_mask_irq(i2c_dev, I2C_INT_TX_FIFO_DATA_REQ);
> +		if (!i2c_dev->msg_read &&
> +		   (status & I2C_INT_TX_FIFO_DATA_REQ)) {
> +			if (i2c_dev->msg_buf_remaining)
> +				tegra_i2c_fill_tx_fifo(i2c_dev);
> +			else
> +				tegra_i2c_mask_irq(i2c_dev,
> +						   I2C_INT_TX_FIFO_DATA_REQ);
> +		}
>  	}
>  
>  	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
>  	if (i2c_dev->is_dvc)
>  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
>  
> +	/*
> +	 * During message read XFER_COMPLETE interrupt is triggered prior to
> +	 * DMA completion and during message write XFER_COMPLETE interrupt is
> +	 * triggered after DMA completion.
> +	 * PACKETS_XFER_COMPLETE indicates completion of all bytes of transfer.
> +	 * so forcing msg_buf_remaining to 0 in DMA mode.
> +	 */
>  	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
> +		if (i2c_dev->is_curr_dma_xfer)
> +			i2c_dev->msg_buf_remaining = 0;
>  		BUG_ON(i2c_dev->msg_buf_remaining);
>  		complete(&i2c_dev->msg_complete);
>  	}
> @@ -690,12 +818,69 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	if (i2c_dev->is_dvc)
>  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR, DVC_STATUS);
>  
> +	if (i2c_dev->is_curr_dma_xfer) {
> +		if (i2c_dev->msg_read)
> +			dmaengine_terminate_all(i2c_dev->rx_dma_chan);
> +		else
> +			dmaengine_terminate_all(i2c_dev->tx_dma_chan);
> +
> +		complete(&i2c_dev->dma_complete);
> +	}
> +
>  	complete(&i2c_dev->msg_complete);
>  done:
>  	spin_unlock(&i2c_dev->xfer_lock);
>  	return IRQ_HANDLED;
>  }
>  
> +static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev *i2c_dev,
> +				       size_t len, int direction)
> +{
> +	u32 val, reg;
> +	u8 dma_burst = 0;
> +	struct dma_slave_config dma_sconfig;
> +	struct dma_chan *chan;
> +
> +	if (i2c_dev->hw->has_mst_fifo)
> +		reg = I2C_MST_FIFO_CONTROL;
> +	else
> +		reg = I2C_FIFO_CONTROL;
> +	val = i2c_readl(i2c_dev, reg);
> +
> +	if (len & 0xF)
> +		dma_burst = 1;
> +	else if (len & 0x10)
> +		dma_burst = 4;
> +	else
> +		dma_burst = 8;
> +
> +	if (direction == DATA_DMA_DIR_TX) {
> +		if (i2c_dev->hw->has_mst_fifo)
> +			val |= I2C_MST_FIFO_CONTROL_TX_TRIG(dma_burst);
> +		else
> +			val |= I2C_FIFO_CONTROL_TX_TRIG(dma_burst);
> +	} else {
> +		if (i2c_dev->hw->has_mst_fifo)
> +			val |= I2C_MST_FIFO_CONTROL_RX_TRIG(dma_burst);
> +		else
> +			val |= I2C_FIFO_CONTROL_RX_TRIG(dma_burst);
> +	}
> +	i2c_writel(i2c_dev, val, reg);
> +
> +	if (direction == DATA_DMA_DIR_TX) {
> +		dma_sconfig.dst_addr = i2c_dev->base_phys + I2C_TX_FIFO;
> +		dma_sconfig.dst_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		dma_sconfig.dst_maxburst = dma_burst;
> +	} else {
> +		dma_sconfig.src_addr = i2c_dev->base_phys + I2C_RX_FIFO;
> +		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		dma_sconfig.src_maxburst = dma_burst;
> +	}
> +
> +	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan : i2c_dev->tx_dma_chan;
> +	dmaengine_slave_config(chan, &dma_sconfig);
> +}
> +
>  static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
>  {
>  	int err;
> @@ -740,6 +925,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	u32 int_mask;
>  	unsigned long time_left;
>  	unsigned long flags;
> +	size_t xfer_size;
> +	u32 *buffer = 0;

Usually this should be = NULL for pointers.

> +	int err = 0;
> +	bool dma = false;
> +	struct dma_chan *chan;
>  
>  	tegra_i2c_flush_fifos(i2c_dev);
>  
> @@ -749,19 +939,69 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
>  	reinit_completion(&i2c_dev->msg_complete);
>  
> +	if (i2c_dev->msg_read)
> +		xfer_size = msg->len;
> +	else
> +		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> +
> +	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> +	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
> +	if (dma) {
> +		err = tegra_i2c_init_dma_param(i2c_dev);
> +		if (err < 0) {
> +			dev_dbg(i2c_dev->dev, "switching to PIO transfer\n");
> +			dma = false;
> +		}

If we successfully got DMA channels at probe time, doesn't this turn
into an error condition that is worth reporting? It seems to me like the
only reason it could fail is if we fail the allocation, but then again,
why don't we move the DMA buffer allocation into probe()? We already use
a fixed size for that allocation, so there's no reason it couldn't be
allocated at probe time.

Seems like maybe you just overlooked that as you were moving around the
code pieces.

> +	}
> +
> +	i2c_dev->is_curr_dma_xfer = dma;
>  	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
>  
>  	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
>  	tegra_i2c_unmask_irq(i2c_dev, int_mask);
>  
> +	if (dma) {
> +		if (i2c_dev->msg_read) {
> +			chan = i2c_dev->rx_dma_chan;
> +			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
> +						   DATA_DMA_DIR_RX);
> +			dma_sync_single_for_device(i2c_dev->dev,
> +						   i2c_dev->dma_phys,
> +						   xfer_size,
> +						   DMA_FROM_DEVICE);

Do we really need this? We're not actually passing the device any data,
so no caches to flush here. I we're cautious about flushing caches when
we do write to the buffer (and I think we do that properly already),
then there should be no need to do it here again.

> +			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> +			if (err < 0) {
> +				dev_err(i2c_dev->dev,
> +					"starting RX DMA failed, err %d\n",
> +					err);
> +				goto unlock;
> +			}
> +		} else {
> +			chan = i2c_dev->tx_dma_chan;
> +			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
> +						   DATA_DMA_DIR_TX);
> +			dma_sync_single_for_cpu(i2c_dev->dev,
> +						i2c_dev->dma_phys,
> +						xfer_size,
> +						DMA_TO_DEVICE);

This, on the other hand seems correct because we need to invalidate the
caches for this buffer to make sure the data that we put there doesn't
get overwritten.

> +			buffer = i2c_dev->dma_buf;
> +		}
> +	}
> +
>  	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
>  			PACKET_HEADER0_PROTOCOL_I2C |
>  			(i2c_dev->cont_id << PACKET_HEADER0_CONT_ID_SHIFT) |
>  			(1 << PACKET_HEADER0_PACKET_ID_SHIFT);
> -	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> +	if (dma && !i2c_dev->msg_read)
> +		*buffer++ = packet_header;
> +	else
> +		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
>  
>  	packet_header = msg->len - 1;
> -	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> +	if (dma && !i2c_dev->msg_read)
> +		*buffer++ = packet_header;
> +	else
> +		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
>  
>  	packet_header = I2C_HEADER_IE_ENABLE;
>  	if (end_state == MSG_END_CONTINUE)
> @@ -778,30 +1018,79 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  		packet_header |= I2C_HEADER_CONT_ON_NAK;
>  	if (msg->flags & I2C_M_RD)
>  		packet_header |= I2C_HEADER_READ;
> -	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> -
> -	if (!(msg->flags & I2C_M_RD))
> -		tegra_i2c_fill_tx_fifo(i2c_dev);
> -
> +	if (dma && !i2c_dev->msg_read)
> +		*buffer++ = packet_header;
> +	else
> +		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> +
> +	if (!i2c_dev->msg_read) {
> +		if (dma) {
> +			memcpy(buffer, msg->buf, msg->len);
> +			dma_sync_single_for_device(i2c_dev->dev,
> +						   i2c_dev->dma_phys,
> +						   xfer_size,
> +						   DMA_TO_DEVICE);

Again, here we properly flush the caches to make sure the data that
we've written to the DMA buffer is visible to the DMA engine.

> +			err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> +			if (err < 0) {
> +				dev_err(i2c_dev->dev,
> +					"starting TX DMA failed, err %d\n",
> +					err);
> +				goto unlock;
> +			}
> +		} else {
> +			tegra_i2c_fill_tx_fifo(i2c_dev);
> +		}
> +	}
>  	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
>  		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
> -	if (msg->flags & I2C_M_RD)
> -		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> -	else if (i2c_dev->msg_buf_remaining)
> -		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> +	if (!dma) {
> +		if (msg->flags & I2C_M_RD)
> +			int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> +		else if (i2c_dev->msg_buf_remaining)
> +			int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> +	}
>  
>  	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> -	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
>  	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
>  		i2c_readl(i2c_dev, I2C_INT_MASK));
>  
> +unlock:
> +	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
> +
> +	if (dma) {
> +		if (err)
> +			return err;
> +
> +		time_left = wait_for_completion_timeout(
> +						&i2c_dev->dma_complete,
> +						TEGRA_I2C_TIMEOUT);
> +
> +		if (time_left == 0) {
> +			dev_err(i2c_dev->dev, "DMA transfer timeout\n");
> +			dmaengine_terminate_all(chan);
> +			tegra_i2c_init(i2c_dev);
> +			return -ETIMEDOUT;
> +		}
> +
> +		if (i2c_dev->msg_read) {
> +			if (likely(i2c_dev->msg_err == I2C_ERR_NONE)) {
> +				dma_sync_single_for_cpu(i2c_dev->dev,
> +							i2c_dev->dma_phys,
> +							xfer_size,
> +							DMA_FROM_DEVICE);

Here we invalidate the caches to make sure we don't get stale data that
may be in the caches for data that we're copying out of the DMA buffer.
I think that's about all the cache maintenance that we really need.

> +
> +				memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
> +					msg->len);
> +			}
> +		}
> +	}
> +
>  	time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
>  						TEGRA_I2C_TIMEOUT);
>  	tegra_i2c_mask_irq(i2c_dev, int_mask);
>  
>  	if (time_left == 0) {
>  		dev_err(i2c_dev->dev, "i2c transfer timed out\n");
> -
>  		tegra_i2c_init(i2c_dev);
>  		return -ETIMEDOUT;
>  	}
> @@ -884,6 +1173,8 @@ static void tegra_i2c_parse_dt(struct tegra_i2c_dev *i2c_dev)
>  
>  	i2c_dev->is_multimaster_mode = of_property_read_bool(np,
>  			"multi-master");
> +
> +	i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>  }
>  
>  static const struct i2c_algorithm tegra_i2c_algo = {
> @@ -1002,11 +1293,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	struct clk *div_clk;
>  	struct clk *fast_clk;
>  	void __iomem *base;
> +	phys_addr_t base_phys;
>  	int irq;
>  	int ret = 0;
>  	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	base_phys = res->start;
>  	base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> @@ -1029,6 +1322,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	i2c_dev->base = base;
> +	i2c_dev->base_phys = base_phys;

We could probably do without the extra local variable by just moving the
assignment here. res is still valid at this point.

>  	i2c_dev->div_clk = div_clk;
>  	i2c_dev->adapter.algo = &tegra_i2c_algo;
>  	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
> @@ -1036,6 +1330,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	i2c_dev->irq = irq;
>  	i2c_dev->cont_id = pdev->id;
>  	i2c_dev->dev = &pdev->dev;
> +	i2c_dev->dma_buf_size = i2c_dev->adapter.quirks->max_write_len;
>  
>  	i2c_dev->rst = devm_reset_control_get_exclusive(&pdev->dev, "i2c");
>  	if (IS_ERR(i2c_dev->rst)) {
> @@ -1049,6 +1344,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
>  						  "nvidia,tegra20-i2c-dvc");
>  	init_completion(&i2c_dev->msg_complete);
> +	init_completion(&i2c_dev->dma_complete);
>  	spin_lock_init(&i2c_dev->xfer_lock);
>  
>  	if (!i2c_dev->hw->has_single_clk_source) {
> @@ -1109,6 +1405,10 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  		}
>  	}
>  
> +	ret = tegra_i2c_init_dma_param(i2c_dev);
> +	if (ret == -EPROBE_DEFER)
> +		goto disable_div_clk;
> +
>  	ret = tegra_i2c_init(i2c_dev);
>  	if (ret) {
>  		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
> @@ -1173,6 +1473,20 @@ static int tegra_i2c_remove(struct platform_device *pdev)
>  	if (!i2c_dev->hw->has_single_clk_source)
>  		clk_unprepare(i2c_dev->fast_clk);
>  
> +	if (i2c_dev->dma_buf)
> +		dma_free_coherent(i2c_dev->dev, i2c_dev->dma_buf_size,
> +				  i2c_dev->dma_buf, i2c_dev->dma_phys);
> +
> +	if (i2c_dev->tx_dma_chan) {
> +		dma_release_channel(i2c_dev->tx_dma_chan);
> +		i2c_dev->tx_dma_chan = NULL;
> +	}
> +
> +	if (i2c_dev->rx_dma_chan) {
> +		dma_release_channel(i2c_dev->rx_dma_chan);
> +		i2c_dev->rx_dma_chan = NULL;
> +	}

No need to explicitly set these to NULL, the memory is going to go away
in a few instructions anyway.

Thierry

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 2/5] i2c: tegra: Add Bus Clear Master Support
  2019-01-31  6:16   ` Sowjanya Komatineni
  (?)
@ 2019-01-31 12:44   ` Thierry Reding
  -1 siblings, 0 replies; 32+ messages in thread
From: Thierry Reding @ 2019-01-31 12:44 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: jonathanh, mkarthik, smohammed, talho, linux-tegra, linux-kernel,
	linux-i2c

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

On Wed, Jan 30, 2019 at 10:16:24PM -0800, Sowjanya Komatineni wrote:
> Bus clear feature of tegra i2c controller helps to recover from
> bus hang when i2c master loses the bus arbitration due to the
> slave device holding SDA LOW continuously for some unknown reasons.
> 
> Per I2C specification, the device that held the bus LOW should
> release it within 9 clock pulses.
> 
> During bus clear operation, Tegra I2C controller sends 9 clock
> pulses and terminates the transaction with STOP condition.
> Upon successful bus clear operation, bus goes to idle state and
> driver retries the transaction.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V5/V6/V7/V8]: Same as V4
>  [V4]: Added I2C Bus Clear support patch to this version of series.
> 
>  drivers/i2c/busses/i2c-tegra.c | 71 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 71 insertions(+)

See my comments on v7, with that:

Acked-by: Thierry Reding <treding@nvidia.com>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically
  2019-01-31  6:16 ` Sowjanya Komatineni
                   ` (4 preceding siblings ...)
  (?)
@ 2019-01-31 12:45 ` Thierry Reding
  -1 siblings, 0 replies; 32+ messages in thread
From: Thierry Reding @ 2019-01-31 12:45 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: jonathanh, mkarthik, smohammed, talho, linux-tegra, linux-kernel,
	linux-i2c

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

On Wed, Jan 30, 2019 at 10:16:23PM -0800, Sowjanya Komatineni wrote:
> This patch sorts all the include headers alphabetically for the
> I2C tegra driver
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V3/V4/V5/V7/V8] : Removed unsued headers in tegra I2C
>  [V2] : Added this in V2 to sort the headers in tegra I2C
> 
>  drivers/i2c/busses/i2c-tegra.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)

See my comments on v7, with those fixed:

Acked-by: Thierry Reding <treding@nvidia.com>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 4/5] i2c: tegra: Update transfer timeout
  2019-01-31  6:16   ` Sowjanya Komatineni
  (?)
@ 2019-01-31 12:48   ` Thierry Reding
  -1 siblings, 0 replies; 32+ messages in thread
From: Thierry Reding @ 2019-01-31 12:48 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: jonathanh, mkarthik, smohammed, talho, linux-tegra, linux-kernel,
	linux-i2c

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

On Wed, Jan 30, 2019 at 10:16:26PM -0800, Sowjanya Komatineni wrote:
> Tegra194 allows max of 64K bytes and Tegra186 and prior allows
> max of 4K bytes of transfer per packet.
> 
> one sec timeout is not enough for transfers more than 10K bytes
> at STD bus rate.
> 
> This patch updates I2C transfer timeout based on the transfer size
> and I2C bus rate to allow enough time during max transfer size at
> lower bus speed.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V8] : Added comment with explaination of xfer time calculation
>  [V5/V6/V7] : Same as V4
>  [V4] : V4 series includes bus clear support and this patch is updated with
> 	fixed timeout of 1sec for bus clear operation.
>  [V3] : Same as V2
>  [V2] : Added this patch in V2 series to allow enough time for data transfer
> 	to happen.
> 	This patch has dependency with DMA patch as TEGRA_I2C_TIMEOUT define
> 	takes argument with this patch.
> 
>  drivers/i2c/busses/i2c-tegra.c | 19 ++++++++++++++-----
>  1 file changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index 025d63972e50..435518cd91b6 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -25,7 +25,7 @@
>  #include <linux/pm_runtime.h>
>  #include <linux/reset.h>
>  
> -#define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
> +#define TEGRA_I2C_TIMEOUT(ms) (msecs_to_jiffies(ms))

I think I would've just gone with direct usage of msecs_to_jiffies() but
this is also fine.

>  #define BYTES_PER_FIFO_WORD 4
>  
>  #define I2C_CNFG				0x000
> @@ -901,8 +901,9 @@ static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
>  		i2c_writel(i2c_dev, reg, I2C_BUS_CLEAR_CNFG);
>  		tegra_i2c_unmask_irq(i2c_dev, I2C_INT_BUS_CLR_DONE);
>  
> -		time_left = wait_for_completion_timeout(&i2c_dev->msg_complete,
> -							TEGRA_I2C_TIMEOUT);
> +		time_left = wait_for_completion_timeout(
> +						&i2c_dev->msg_complete,
> +						TEGRA_I2C_TIMEOUT(1000));
>  		if (time_left == 0) {
>  			dev_err(i2c_dev->dev, "timed out for bus clear\n");
>  			return -ETIMEDOUT;
> @@ -930,6 +931,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	int err = 0;
>  	bool dma = false;
>  	struct dma_chan *chan;
> +	u16 xfer_time = 100;
>  
>  	tegra_i2c_flush_fifos(i2c_dev);
>  
> @@ -945,6 +947,13 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
>  
>  	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> +	/*
> +	 * Transfer time = Total bits / transfer rate
> +	 * Total bits = 9 bits per byte (including ACK bit) + Start & stop bits
> +	 */
> +	xfer_time += DIV_ROUND_CLOSEST(((xfer_size * 9) + 2) * 1000,

This doesn't really explain the factor of 1000 in there, which I assume
is just to scale this to milliseconds required by msecs_to_jiffies().
Might be worth to replace it by MSEC_PER_SEC to clarify the purpose.

With that fixed:

Acked-by: Thierry Reding <treding@nvidia.com>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 5/5] i2c: tegra: Add I2C interface timing support
  2019-01-31  6:16   ` Sowjanya Komatineni
  (?)
@ 2019-01-31 12:50   ` Thierry Reding
  -1 siblings, 0 replies; 32+ messages in thread
From: Thierry Reding @ 2019-01-31 12:50 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: jonathanh, mkarthik, smohammed, talho, linux-tegra, linux-kernel,
	linux-i2c

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

On Wed, Jan 30, 2019 at 10:16:27PM -0800, Sowjanya Komatineni wrote:
> This patch adds I2C interface timing registers support for
> proper bus rate configuration along with meeting the i2c spec
> setup and hold times based on the tuning performed on Tegra210,
> Tegra186 and Tegra194 platforms.
> 
> I2C_INTERFACE_TIMING_0 register contains TLOW and THIGH field
> and Tegra I2C controller design uses them as a part of internal
> clock divisor.
> 
> I2C_INTERFACE_TIMING_1 register contains the setup and hold times
> for start and stop conditions.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V8] : Updated to handle timing implementation within tegra_i2c_init directly
>  [V7] : Minor updates to timing implementation
>  [V5/V6] : Added this Interface timing patch in V5 of the patch series.
> 
>  drivers/i2c/busses/i2c-tegra.c | 187 ++++++++++++++++++++++++++++++++++-------
>  1 file changed, 158 insertions(+), 29 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

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

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31  6:16   ` Sowjanya Komatineni
  (?)
  (?)
@ 2019-01-31 15:12   ` Dmitry Osipenko
  2019-01-31 15:24     ` Dmitry Osipenko
  -1 siblings, 1 reply; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 15:12 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, mkarthik,
	smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 9:16, Sowjanya Komatineni пишет:
> This patch adds DMA support for Tegra I2C.
> 
> Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
> transfer size of the max FIFO depth and DMA mode is used for
> transfer size higher than max FIFO depth to save CPU overhead.
> 
> PIO mode needs full intervention of CPU to fill or empty FIFO's
> and also need to service multiple data requests interrupt for the
> same transaction. This adds delay between data bytes of the same
> transfer when CPU is fully loaded and some slave devices has
> internal timeout for no bus activity and stops transaction to
> avoid bus hang. DMA mode is helpful in such cases.
> 
> DMA mode is also helpful for Large transfers during downloading or
> uploading FW over I2C to some external devices.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V8] : Moved back dma init to i2c probe, removed ALL_PACKETS_XFER_COMPLETE
> 	interrupt and using PACKETS_XFER_COMPLETE interrupt only and some
> 	other fixes
> 	Updated Kconfig for APB_DMA dependency
>  [V7] : Same as V6
>  [V6] : Updated for proper buffer allocation/freeing, channel release.
> 	Updated to use exact xfer size for syncing dma buffer.
>  [V5] : Same as V4
>  [V4] : Updated to allocate DMA buffer only when DMA mode.
> 	Updated to fall back to PIO mode when DMA channel request or
> 	buffer allocation fails.
>  [V3] : Updated without additional buffer allocation.
>  [V2] : Updated based on V1 review feedback along with code cleanup for
> 	proper implementation of DMA.
> 
>  drivers/i2c/busses/Kconfig     |   2 +-
>  drivers/i2c/busses/i2c-tegra.c | 362 ++++++++++++++++++++++++++++++++++++++---
>  2 files changed, 339 insertions(+), 25 deletions(-)

Tegra20 crashes because of this patch:

<6>[    3.204854] pstore: Using crash dump compression: deflate
<6>[    3.306800] brcmfmac: brcmf_fw_alloc_request: using brcm/brcmfmac4329-sdio for chip BCM4329/3
<6>[    3.306898] brcmfmac: brcmf_c_process_clm_blob: no clm_blob available (err=-2), device may have limited channels available
<6>[    3.307532] brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4329/3 wl0: Sep  2 2011 14:48:19 version 4.220.48
<3>[    3.318169] brcmfmac: brcmf_setup_wiphybands: rxchain error (-52)
<7>[    3.379663] tegra-i2c 7000c000.i2c: unmasked irq: 0c
<7>[    3.379754] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<7>[    3.379763] tegra-i2c 7000c000.i2c: unmasked irq: 0d
<7>[    3.380017] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<7>[    3.380030] tegra-i2c 7000c000.i2c: unmasked irq: 0c
<7>[    3.380123] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<7>[    3.380133] tegra-i2c 7000c000.i2c: starting DMA for length: 112
<7>[    3.380144] tegra-i2c 7000c000.i2c: unmasked irq: 0c
<7>[    3.383507] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<6>[    3.383519] atmel_mxt_ts 0-004c: Family: 160 Variant: 0 Firmware V1.0.AA Objects: 18
<7>[    3.383566] tegra-i2c 7000c000.i2c: unmasked irq: 0c
<7>[    3.383660] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<7>[    3.383670] tegra-i2c 7000c000.i2c: starting DMA for length: 224
<7>[    3.383678] tegra-i2c 7000c000.i2c: unmasked irq: 0c
<7>[    3.395181] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<7>[    3.395210] tegra-i2c 7000c000.i2c: unmasked irq: 0c
<7>[    3.395345] tegra-i2c 7000c000.i2c: transfer complete: 100 0 0
<7>[    3.395354] tegra-i2c 7000c000.i2c: unmasked irq: 0d
<4>[    3.395730] wm8903 0-001a: 0-001a supply AVDD not found, using dummy regulator
<6>[    3.395801] wm8903 0-001a: Linked as a consumer to regulator.0
<4>[    3.395829] wm8903 0-001a: 0-001a supply CPVDD not found, using dummy regulator
<4>[    3.395915] ------------[ cut here ]------------
<2>[    3.395919] kernel BUG at drivers/i2c/busses/i2c-tegra.c:810!
<0>[    3.395922] Internal error: Oops - BUG: 0 [#1] PREEMPT SMP THUMB2
<4>[    3.395926] Modules linked in:
<4>[    3.395936] CPU: 0 PID: 121 Comm: kworker/0:3 Not tainted 5.0.0-rc2-next-20190121-00119-gbd91760d6769-dirty #1055
<4>[    3.395938] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
<4>[    3.395953] Workqueue: events deferred_probe_work_func
<4>[    3.395961] PC is at tegra_i2c_isr+0x26e/0x380
<4>[    3.395971] LR is at arm_heavy_mb+0x17/0x2c
<4>[    3.395975] pc : [<c05e9eb2>]    lr : [<c0112787>]    psr: 200001b3
<4>[    3.395978] sp : d66818e8  ip : c0f19274  fp : c0fd4c40
<4>[    3.395982] r10: c0fd4c54  r9 : d6c3c000  r8 : d668196c
<4>[    3.395985] r7 : 0000003e  r6 : d6476f04  r5 : 000000c2  r4 : d6476c40
<4>[    3.395989] r3 : 00000009  r2 : 00000068  r1 : 00000000  r0 : d6476f04
<4>[    3.395994] Flags: nzCv  IRQs off  FIQs on  Mode SVC_32  ISA Thumb  Segment none
<4>[    3.395998] Control: 50c5387d  Table: 0000404a  DAC: 00000051
<0>[    3.396003] Process kworker/0:3 (pid: 121, stack limit = 0x(ptrval))
<0>[    3.396007] Stack: (0xd66818e8 to 0xd6682000)
<0>[    3.396014] 18e0:                   d66819d7 ffffffff d6681928 00000010 00000006 d64dc480
<0>[    3.396022] 1900: d6dd5800 00000000 0000003e d668196c d6c3c000 c0fd4c54 c0fd4c40 c0161a1d
<0>[    3.396029] 1920: 00000006 0000000a 0000003e ffffe000 d6dd5800 c0f0a0b0 c0fd45ef 00000010
<0>[    3.396036] 1940: 00000000 d6dd5800 d6dd5800 c0f60b58 16461000 00000000 d6c3c000 c0fe5228
<0>[    3.396044] 1960: c0fe4d28 c0161bb3 d66819a0 00000000 c0c637eb d6dd5800 d6dd5864 c0161c09
<0>[    3.396051] 1980: d6dd5800 d6dd5864 c0f60b58 c01641f1 c0164181 c0ea0e28 00000000 0000003e
<0>[    3.396058] 19a0: 00000001 c016103d 0000012b c01614b7 d66819f0 c0f0a864 c0f60bf4 fe44010c
<0>[    3.396065] 19c0: fe440100 d66819f0 fe441100 c03e42ab c015f3d7 c015f3dc 60000133 ffffffff
<0>[    3.396073] 19e0: d6681a24 c0f0a0b0 d6680000 c0101a65 60000193 16461000 00000000 a0000113
<0>[    3.396080] 1a00: 00000000 00000000 00000053 00000000 c0f0a0b0 c037a95d c0fe5228 c0fe4d28
<0>[    3.396087] 1a20: 0000000a d6681a40 c015f3d7 c015f3dc 60000133 ffffffff 00000051 00000000
<0>[    3.396094] 1a40: 00000400 c01601f3 00000000 a0000113 ffffe000 00000000 c0f0a0b0 00000000
<0>[    3.396101] 1a60: 00000113 c0fe4d28 ffffe000 00000000 00000001 000000fe 00000000 60000113
<0>[    3.396108] 1a80: 00000043 c016058d c0cca160 d6681b94 c0cb3f6c 00000001 c0cca160 0000000e
<0>[    3.396115] 1aa0: d6681ad8 d66c9420 c0cb3f6c 00000004 c0cca160 d6681b94 d6681ae6 c04acb79
<0>[    3.396122] 1ac0: c0cca160 d6681b94 d6681b0c 00000072 00000000 00000000 53425553 45545359
<0>[    3.396130] 1ae0: 32693d4d 45440063 45434956 32692b3d 2d303a63 61313030 d6681b00 c0646fa9
<0>[    3.396136] 1b00: 00000000 00000000 ffff0a00 00000000 00000000 00000000 00000000 00000000
<0>[    3.396143] 1b20: 00000000 00000000 00000000 00000000 c0cb729e d6681b48 d6ce7d58 d6ce7d58
<0>[    3.396150] 1b40: d6681b7c c0887ffd 00000076 d6681b7c c0d1da18 c04204d1 c0d1da18 d6681b94
<0>[    3.396158] 1b60: d6681bac 00000000 c0d1da18 d64fc300 d66c964c 00000002 0000003e c04acc47
<0>[    3.396165] 1b80: 00000000 d6681b94 d6681bb0 c04accb3 c0cca160 c0d1dae8 d64fc300 d6681bb0
<0>[    3.396172] 1ba0: c0cb7f0c c04ace3f 00000002 d6681bc8 c0cb7f0c d6681bac ffffffed d66c9420
<0>[    3.396179] 1bc0: c0423f1d c0cb7f0c d64fc300 c0d1da18 00000001 d66c9658 00000004 d66c9420
<0>[    3.396186] 1be0: d66c964c d66c964c 00000002 c0424009 d66ee7c0 00000005 d66c9420 00000004
<0>[    3.396194] 1c00: d66c964c c0424a2f d66c9640 00000005 d66c9400 d66c9420 d66ee740 c069d7e7
<0>[    3.396200] 1c20: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 000001f4
<0>[    3.396207] 1c40: 00000000 d66c9420 c069d745 c0fac584 d66c9400 00000000 c0fac584 00000002
<0>[    3.396214] 1c60: 0000003e c05e5485 c101d344 d66c9420 c101d348 00000000 00000000 c04afc75
<0>[    3.396222] 1c80: d66c9420 c0fac584 d6681cf8 c04affad 00000000 c0f98cb8 c0cf7a6c c04afe37
<0>[    3.396229] 1ca0: c09e36e4 c05e7b0f d66c9400 c0fac584 d66c9420 00000001 00000000 d6681cf8
<0>[    3.396236] 1cc0: c04affad 00000000 c0f98cb8 c0cf7a6c 0000003e c04ae939 d6dd8a6c d645deb8
<0>[    3.396243] 1ce0: 00000000 d66c9420 d66c9454 d66c9420 00000001 c04afadd d66c9420 00000001
<0>[    3.396250] 1d00: d66c9428 d66c9428 00000000 d66c9420 c0f98cfc c04af29f d7304e64 d66c9428
<0>[    3.396258] 1d20: 00000000 d6476c88 d66c9420 c04ad0e9 d66c9420 c04b8c93 d66c94a0 d66c9420
<0>[    3.396265] 1d40: c101d320 d66c9400 d6681d88 d6476c48 00000000 d66c9404 d66c9420 c05e5d95
<0>[    3.396272] 1d60: 00000000 c0646945 c0cc9e73 d73267d0 d732681c d6476c48 d6476c88 d7326388
<0>[    3.396279] 1d80: c0cf7a90 c05e7bd9 39386d77 00003330 00000000 00000000 00000000 001a0000
<0>[    3.396286] 1da0: 00000000 00000000 d73267d0 00000000 00000000 00000000 00000000 00000000
<0>[    3.396293] 1dc0: d6476c48 00000000 d6476c88 00000000 00000000 d7326388 7000c000 c05e61bd
<0>[    3.396300] 1de0: d6e0c000 c05e6465 fffffffe d6476c40 00000000 d6e0c010 d6e0c000 00000000
<0>[    3.396307] 1e00: d7326388 c05eac11 00000000 d6dd7dc0 d6476c40 d6debc00 00000001 d6e0c010
<0>[    3.396314] 1e20: 00000000 c0f998a4 00000000 00000000 c0f998a4 00000001 ffffe000 c04b1213
<0>[    3.396321] 1e40: c04b11e1 c101d344 d6e0c010 c101d348 00000000 c04afc75 d6e0c010 c0f998a4
<0>[    3.396328] 1e60: d6681ed0 c04affad 00000000 c0fd4c90 00000000 c04afe37 00000000 c0646805
<0>[    3.396335] 1e80: d6e0c010 c0f998a4 d6e0c010 00000001 00000000 d6681ed0 c04affad 00000000
<0>[    3.396342] 1ea0: c0fd4c90 00000000 ffffe000 c04ae939 d6ce746c d6eb9ab8 00000000 d6e0c010
<0>[    3.396350] 1ec0: d6e0c044 d6e0c010 00000001 c04afadd d6e0c010 00000001 d6e0c010 d6e0c010
<0>[    3.396357] 1ee0: c0f7d200 d6e0c010 c0f7d478 c04af29f 00000000 d6e0c010 c0f7d200 c0f7d200
<0>[    3.396364] 1f00: c0f7d21c c04af5e7 c04af59d c0f7d228 d6492180 d7301e80 d7305000 c012e74d
<0>[    3.396372] 1f20: c012f266 d7301e80 d6681f40 d6492180 d6492194 d7301e80 d7301e94 c0f03d00
<0>[    3.396379] 1f40: d7301e98 d6d17ec0 ffffe000 c012f355 ffffe000 c0fd457c c0c5ff88 c0132a24
<0>[    3.396386] 1f60: d6681f78 d6e31b40 d649e7c0 c0132a24 d6680000 d6492180 c012f11d d6d17ec0
<0>[    3.396393] 1f80: d6e31b5c c0132ac5 00000000 d649e7c0 c01329c1 00000000 00000000 00000000
<0>[    3.396400] 1fa0: 00000000 00000000 00000000 c01010bd 00000000 00000000 00000000 00000000
<0>[    3.396406] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
<0>[    3.396413] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
<4>[    3.396431] [<c05e9eb2>] (tegra_i2c_isr) from [<c0161a1d>] (__handle_irq_event_percpu+0x5d/0x1dc)
<4>[    3.396442] [<c0161a1d>] (__handle_irq_event_percpu) from [<c0161bb3>] (handle_irq_event_percpu+0x17/0x40)
<4>[    3.396450] [<c0161bb3>] (handle_irq_event_percpu) from [<c0161c09>] (handle_irq_event+0x2d/0x44)
<4>[    3.396461] [<c0161c09>] (handle_irq_event) from [<c01641f1>] (handle_fasteoi_irq+0x71/0xdc)
<4>[    3.396470] [<c01641f1>] (handle_fasteoi_irq) from [<c016103d>] (generic_handle_irq+0x1d/0x28)
<4>[    3.396479] [<c016103d>] (generic_handle_irq) from [<c01614b7>] (__handle_domain_irq+0x43/0x84)
<4>[    3.396489] [<c01614b7>] (__handle_domain_irq) from [<c03e42ab>] (gic_handle_irq+0x43/0x78)
<4>[    3.396499] [<c03e42ab>] (gic_handle_irq) from [<c0101a65>] (__irq_svc+0x65/0xac)
<4>[    3.396503] Exception stack(0xd66819f0 to 0xd6681a38)
<4>[    3.396508] 19e0:                                     60000193 16461000 00000000 a0000113
<4>[    3.396516] 1a00: 00000000 00000000 00000053 00000000 c0f0a0b0 c037a95d c0fe5228 c0fe4d28
<4>[    3.396522] 1a20: 0000000a d6681a40 c015f3d7 c015f3dc 60000133 ffffffff
<4>[    3.396532] [<c0101a65>] (__irq_svc) from [<c015f3dc>] (console_unlock+0x294/0x420)
<4>[    3.396539] [<c015f3dc>] (console_unlock) from [<c016058d>] (vprintk_emit+0x115/0x17c)
<4>[    3.396547] [<c016058d>] (vprintk_emit) from [<c04acb79>] (dev_vprintk_emit+0x99/0x14c)
<4>[    3.396555] [<c04acb79>] (dev_vprintk_emit) from [<c04acc47>] (dev_printk_emit+0x1b/0x24)
<4>[    3.396562] [<c04acc47>] (dev_printk_emit) from [<c04accb3>] (__dev_printk+0x2f/0x60)
<4>[    3.396568] [<c04accb3>] (__dev_printk) from [<c04ace3f>] (_dev_warn+0x2b/0x34)
<4>[    3.396581] [<c04ace3f>] (_dev_warn) from [<c0423f1d>] (_regulator_get+0x131/0x1d4)
<4>[    3.396590] [<c0423f1d>] (_regulator_get) from [<c0424009>] (regulator_bulk_get+0x3d/0x80)
<4>[    3.396601] [<c0424009>] (regulator_bulk_get) from [<c0424a2f>] (devm_regulator_bulk_get+0x37/0x60)
<4>[    3.396617] [<c0424a2f>] (devm_regulator_bulk_get) from [<c069d7e7>] (wm8903_i2c_probe+0xa3/0x55c)
<4>[    3.396632] [<c069d7e7>] (wm8903_i2c_probe) from [<c05e5485>] (i2c_device_probe+0x17d/0x194)
<4>[    3.396642] [<c05e5485>] (i2c_device_probe) from [<c04afc75>] (really_probe+0x141/0x1e4)
<4>[    3.396651] [<c04afc75>] (really_probe) from [<c04afe37>] (driver_probe_device+0x43/0x124)
<4>[    3.396659] [<c04afe37>] (driver_probe_device) from [<c04ae939>] (bus_for_each_drv+0x41/0x60)
<4>[    3.396667] [<c04ae939>] (bus_for_each_drv) from [<c04afadd>] (__device_attach+0x75/0xc0)
<4>[    3.396675] [<c04afadd>] (__device_attach) from [<c04af29f>] (bus_probe_device+0x5b/0x60)
<4>[    3.396682] [<c04af29f>] (bus_probe_device) from [<c04ad0e9>] (device_add+0x2a1/0x44c)
<4>[    3.396690] [<c04ad0e9>] (device_add) from [<c05e5d95>] (i2c_new_device+0xe1/0x1f8)
<4>[    3.396700] [<c05e5d95>] (i2c_new_device) from [<c05e7bd9>] (of_i2c_register_devices+0x7d/0xbc)
<4>[    3.396710] [<c05e7bd9>] (of_i2c_register_devices) from [<c05e61bd>] (i2c_register_adapter+0x105/0x2f0)
<4>[    3.396718] [<c05e61bd>] (i2c_register_adapter) from [<c05eac11>] (tegra_i2c_probe+0x30d/0x440)
<4>[    3.396728] [<c05eac11>] (tegra_i2c_probe) from [<c04b1213>] (platform_drv_probe+0x33/0x68)
<4>[    3.396737] [<c04b1213>] (platform_drv_probe) from [<c04afc75>] (really_probe+0x141/0x1e4)
<4>[    3.396745] [<c04afc75>] (really_probe) from [<c04afe37>] (driver_probe_device+0x43/0x124)
<4>[    3.396753] [<c04afe37>] (driver_probe_device) from [<c04ae939>] (bus_for_each_drv+0x41/0x60)
<4>[    3.396761] [<c04ae939>] (bus_for_each_drv) from [<c04afadd>] (__device_attach+0x75/0xc0)
<4>[    3.396769] [<c04afadd>] (__device_attach) from [<c04af29f>] (bus_probe_device+0x5b/0x60)
<4>[    3.396777] [<c04af29f>] (bus_probe_device) from [<c04af5e7>] (deferred_probe_work_func+0x4b/0x6c)
<4>[    3.396789] [<c04af5e7>] (deferred_probe_work_func) from [<c012e74d>] (process_one_work+0x155/0x3c8)
<4>[    3.396798] [<c012e74d>] (process_one_work) from [<c012f355>] (worker_thread+0x239/0x3d4)
<4>[    3.396807] [<c012f355>] (worker_thread) from [<c0132ac5>] (kthread+0x105/0x110)
<4>[    3.396814] [<c0132ac5>] (kthread) from [<c01010bd>] (ret_from_fork+0x11/0x34)
<4>[    3.396818] Exception stack(0xd6681fb0 to 0xd6681ff8)
<4>[    3.396823] 1fa0:                                     00000000 00000000 00000000 00000000
<4>[    3.396830] 1fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
<4>[    3.396836] 1fe0: 00000000 00000000 00000000 00000000 00000013 00000000
<0>[    3.396844] Code: 32b4 2b00 f43f af75 (de02) f8d4 
<4>[    3.396853] ---[ end trace e64225d6f8db918a ]---
<0>[    3.405350] Kernel panic - not syncing: Fatal exception in interrupt
<2>[    3.405365] CPU1: stopping
<4>[    3.405373] CPU: 1 PID: 0 Comm: swapper/1 Tainted: G      D           5.0.0-rc2-next-20190121-00119-gbd91760d6769-dirty #1055
<4>[    3.405376] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree)
<4>[    3.405396] [<c010dbed>] (unwind_backtrace) from [<c0109f45>] (show_stack+0x11/0x14)
<4>[    3.405412] [<c0109f45>] (show_stack) from [<c088568d>] (dump_stack+0x6d/0x7c)
<4>[    3.405423] [<c088568d>] (dump_stack) from [<c010c565>] (handle_IPI+0x2e5/0x308)
<4>[    3.405432] [<c010c565>] (handle_IPI) from [<c03e42dd>] (gic_handle_irq+0x75/0x78)
<4>[    3.405441] [<c03e42dd>] (gic_handle_irq) from [<c0101a65>] (__irq_svc+0x65/0xac)
<4>[    3.405444] Exception stack(0xd6d2bee8 to 0xd6d2bf30)
<4>[    3.405452] bee0:                   00000000 c0f12350 16472000 00000050 00000000 c0f12350
<4>[    3.405459] bf00: 00000000 00000000 00000000 c0fd530c caf99410 d7312678 fa000000 d6d2bf38
<4>[    3.405464] bf20: c061f981 c061fad0 60000133 ffffffff
<4>[    3.405477] [<c0101a65>] (__irq_svc) from [<c061fad0>] (cpuidle_enter_state+0x248/0x4d0)
<4>[    3.405490] [<c061fad0>] (cpuidle_enter_state) from [<c0621641>] (cpuidle_enter_state_coupled+0x1ad/0x2d4)
<4>[    3.405499] [<c0621641>] (cpuidle_enter_state_coupled) from [<c013e823>] (do_idle+0x17b/0x1b0)
<4>[    3.405507] [<c013e823>] (do_idle) from [<c013ea49>] (cpu_startup_entry+0x19/0x1c)
<4>[    3.405515] [<c013ea49>] (cpu_startup_entry) from [<00102551>] (0x102551)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically
  2019-01-31  6:16 ` Sowjanya Komatineni
                   ` (5 preceding siblings ...)
  (?)
@ 2019-01-31 15:14 ` Dmitry Osipenko
  -1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 15:14 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, mkarthik,
	smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 9:16, Sowjanya Komatineni пишет:
> This patch sorts all the include headers alphabetically for the
> I2C tegra driver
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V3/V4/V5/V7/V8] : Removed unsued headers in tegra I2C
>  [V2] : Added this in V2 to sort the headers in tegra I2C
> 
>  drivers/i2c/busses/i2c-tegra.c | 19 ++++++++-----------
>  1 file changed, 8 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index e417ebf7628c..15806c984859 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -6,24 +6,21 @@
>   * Author: Colin Cross <ccross@android.com>
>   */
>  
> -#include <linux/kernel.h>
> -#include <linux/init.h>
> -#include <linux/platform_device.h>
>  #include <linux/clk.h>
> +#include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
> -#include <linux/io.h>
> +#include <linux/init.h>
>  #include <linux/interrupt.h>
> -#include <linux/delay.h>
> -#include <linux/slab.h>
> -#include <linux/of_device.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/kernel.h>
>  #include <linux/module.h>
> -#include <linux/reset.h>
> +#include <linux/of_device.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/platform_device.h>
>  #include <linux/pm_runtime.h>
> -#include <linux/iopoll.h>
> -
> -#include <asm/unaligned.h>
> +#include <linux/reset.h>
>  
>  #define TEGRA_I2C_TIMEOUT (msecs_to_jiffies(1000))
>  #define BYTES_PER_FIFO_WORD 4
> 

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 2/5] i2c: tegra: Add Bus Clear Master Support
  2019-01-31  6:16   ` Sowjanya Komatineni
  (?)
  (?)
@ 2019-01-31 15:16   ` Dmitry Osipenko
  -1 siblings, 0 replies; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 15:16 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, mkarthik,
	smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 9:16, Sowjanya Komatineni пишет:
> Bus clear feature of tegra i2c controller helps to recover from
> bus hang when i2c master loses the bus arbitration due to the
> slave device holding SDA LOW continuously for some unknown reasons.
> 
> Per I2C specification, the device that held the bus LOW should
> release it within 9 clock pulses.
> 
> During bus clear operation, Tegra I2C controller sends 9 clock
> pulses and terminates the transaction with STOP condition.
> Upon successful bus clear operation, bus goes to idle state and
> driver retries the transaction.
> 
> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> ---
>  [V5/V6/V7/V8]: Same as V4
>  [V4]: Added I2C Bus Clear support patch to this version of series.
> 
 
I haven't checked all of the bits in this patch, but at least -EAGAIN should work as expected on older Tegra's:

Reviewed-by: Dmitry Osipenko <digetx@gmail.com> 

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 15:12   ` Dmitry Osipenko
@ 2019-01-31 15:24     ` Dmitry Osipenko
  2019-01-31 16:56       ` Sowjanya Komatineni
  0 siblings, 1 reply; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 15:24 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, jonathanh, mkarthik,
	smohammed, talho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 18:12, Dmitry Osipenko пишет:
> 31.01.2019 9:16, Sowjanya Komatineni пишет:
>> This patch adds DMA support for Tegra I2C.
>>
>> Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
>> transfer size of the max FIFO depth and DMA mode is used for
>> transfer size higher than max FIFO depth to save CPU overhead.
>>
>> PIO mode needs full intervention of CPU to fill or empty FIFO's
>> and also need to service multiple data requests interrupt for the
>> same transaction. This adds delay between data bytes of the same
>> transfer when CPU is fully loaded and some slave devices has
>> internal timeout for no bus activity and stops transaction to
>> avoid bus hang. DMA mode is helpful in such cases.
>>
>> DMA mode is also helpful for Large transfers during downloading or
>> uploading FW over I2C to some external devices.
>>
>> Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
>> ---
>>  [V8] : Moved back dma init to i2c probe, removed ALL_PACKETS_XFER_COMPLETE
>> 	interrupt and using PACKETS_XFER_COMPLETE interrupt only and some
>> 	other fixes
>> 	Updated Kconfig for APB_DMA dependency
>>  [V7] : Same as V6
>>  [V6] : Updated for proper buffer allocation/freeing, channel release.
>> 	Updated to use exact xfer size for syncing dma buffer.
>>  [V5] : Same as V4
>>  [V4] : Updated to allocate DMA buffer only when DMA mode.
>> 	Updated to fall back to PIO mode when DMA channel request or
>> 	buffer allocation fails.
>>  [V3] : Updated without additional buffer allocation.
>>  [V2] : Updated based on V1 review feedback along with code cleanup for
>> 	proper implementation of DMA.
>>
>>  drivers/i2c/busses/Kconfig     |   2 +-
>>  drivers/i2c/busses/i2c-tegra.c | 362 ++++++++++++++++++++++++++++++++++++++---
>>  2 files changed, 339 insertions(+), 25 deletions(-)
> 
> Tegra20 crashes because of this patch:
> 
[snip]
> <4>[    3.395915] ------------[ cut here ]------------
> <2>[    3.395919] kernel BUG at drivers/i2c/busses/i2c-tegra.c:810!
The BUG line is from:

	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
		if (i2c_dev->is_curr_dma_xfer)
			i2c_dev->msg_buf_remaining = 0;
		BUG_ON(i2c_dev->msg_buf_remaining);
		complete(&i2c_dev->msg_complete);
	}

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 12:44   ` Thierry Reding
@ 2019-01-31 16:41     ` Sowjanya Komatineni
  2019-02-01  0:52     ` Dmitry Osipenko
  1 sibling, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31 16:41 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Jonathan Hunter, Mantravadi Karthik, Shardar Mohammed, Timo Alho,
	linux-tegra, linux-kernel, linux-i2c

> > +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev) {
> > +	struct dma_chan *dma_chan;
> > +	u32 *dma_buf;
> > +	dma_addr_t dma_phys;
> > +
> > +	if (!i2c_dev->has_dma)
> > +		return -EINVAL;
> > +
> > +	if (!i2c_dev->rx_dma_chan) {
> > +		dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, "rx");
> > +		if (IS_ERR(dma_chan))
> > +			return PTR_ERR(dma_chan);
>
> I think we want to fallback to PIO here if dma_chan is -ENODEV.

Checking if dmas property is in device tree before channel requests. So ENODEV will not be returned.
Incase if dmas property is not there in device tree, we fall back to PIO as init_dma_params returns invalid

> Although, I'm not exactly sure I understand what you're trying to achieve here. Shouldn't we move the channel request parts into probe and remove them from here? Otherwise it seems like we could get into a state where we keep trying to get the slave channels everytime we set up a DMA transfer, even if we already failed to do so during probe.
>

This patch tries to get channel request during probe but buffer allocation will not happen till xfer is needed in dma mode.
During xfer message, this function is called again for dma buffer allocation only for dma mode.

> > +
> > +	if (!i2c_dev->dma_buf && i2c_dev->msg_buf_remaining) {
> > +		dma_buf = dma_alloc_coherent(i2c_dev->dev,
> > +					     i2c_dev->dma_buf_size,
> > +					     &dma_phys, GFP_KERNEL);
> > +
> > +		if (!dma_buf) {
> > +			dev_err(i2c_dev->dev,
> > +				"failed to allocate the DMA buffer\n");
> > +			dma_release_channel(i2c_dev->tx_dma_chan);
> > +			dma_release_channel(i2c_dev->rx_dma_chan);
> > +			i2c_dev->tx_dma_chan = NULL;
> > +			i2c_dev->rx_dma_chan = NULL;
> > +			return -ENOMEM;
> > +		}
> > +
> > +		i2c_dev->dma_buf = dma_buf;
> > +		i2c_dev->dma_phys = dma_phys;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
>
>
> > @@ -749,19 +939,69 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
> >  	i2c_dev->msg_read = (msg->flags & I2C_M_RD);
> >  	reinit_completion(&i2c_dev->msg_complete);
> >  
> > +	if (i2c_dev->msg_read)
> > +		xfer_size = msg->len;
> > +	else
> > +		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> > +
> > +	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> > +	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
> > +	if (dma) {
> > +		err = tegra_i2c_init_dma_param(i2c_dev);
> > +		if (err < 0) {
> > +			dev_dbg(i2c_dev->dev, "switching to PIO transfer\n");
> > +			dma = false;
> > +		}
>
>
> If we successfully got DMA channels at probe time, doesn't this turn into an error condition that is worth reporting? It seems to me like the only reason it could fail is if we fail the allocation, but then again, why don't we move the DMA buffer allocation into probe()? We already use a fixed size for that allocation, so there's no reason it couldn't be allocated at probe time.
>
> Seems like maybe you just overlooked that as you were moving around the code pieces.

Checking for dmas property is inside init_dma_param and if dmas property doesn't exist init_dma_param returns err EINVAL
Also init_dma_param fails for if buffer allocation fails. In both of those cases it will switch to PIO Mode
As per review feedback, performing channel request allocation during probe and buffer allocation is postponed till there is need for DMA and buffer allocation happens during 1st DMA mode xfer if it is not already allocated

>
>
>
> >  static const struct i2c_algorithm tegra_i2c_algo = { @@ -1002,11 
> > +1293,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >  	struct clk *div_clk;
> >  	struct clk *fast_clk;
> >  	void __iomem *base;
> > +	phys_addr_t base_phys;
> >  	int irq;
> >  	int ret = 0;
> >  	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
> >  
> >  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	base_phys = res->start;
> >  	base = devm_ioremap_resource(&pdev->dev, res);
> >  	if (IS_ERR(base))
> >  		return PTR_ERR(base);
> > @@ -1029,6 +1322,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
> >  		return -ENOMEM;
> >  
> >  	i2c_dev->base = base;
> > +	i2c_dev->base_phys = base_phys;
>
> We could probably do without the extra local variable by just moving the assignment here. res is still valid at this point.
Same res is used for both IORESOURCE_MEM ad IORESOURCE_IRQ and i2c_dev allocation happens later so I had to use temp variable

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 15:24     ` Dmitry Osipenko
@ 2019-01-31 16:56       ` Sowjanya Komatineni
  2019-01-31 17:11         ` Dmitry Osipenko
  0 siblings, 1 reply; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31 16:56 UTC (permalink / raw)
  To: Dmitry Osipenko, thierry.reding, Jonathan Hunter,
	Mantravadi Karthik, Shardar Mohammed, Timo Alho
  Cc: linux-tegra, linux-kernel, linux-i2c


> >>  drivers/i2c/busses/Kconfig     |   2 +-
> >>  drivers/i2c/busses/i2c-tegra.c | 362 
> >> ++++++++++++++++++++++++++++++++++++++---
> >>  2 files changed, 339 insertions(+), 25 deletions(-)
> > 
> > Tegra20 crashes because of this patch:
> > 
> [snip]
> > <4>[    3.395915] ------------[ cut here ]------------
> > <2>[    3.395919] kernel BUG at drivers/i2c/busses/i2c-tegra.c:810!
> The BUG line is from:
>
>	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>		if (i2c_dev->is_curr_dma_xfer)
>			i2c_dev->msg_buf_remaining = 0;
>		BUG_ON(i2c_dev->msg_buf_remaining);
>		complete(&i2c_dev->msg_complete);
>	}
>
BUG_ON line is not part of this change. It was already there in existing driver.
Based on log, I see DMA transfer is done for 224 bytes followed by 1 successful PIO transfer and then on next PIO transfer it received packet xfer complete interrupt with incomplete transfer bytes and that where it hit BUG_ON condition.



^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 16:56       ` Sowjanya Komatineni
@ 2019-01-31 17:11         ` Dmitry Osipenko
  2019-01-31 17:25           ` Dmitry Osipenko
  0 siblings, 1 reply; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 17:11 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, Jonathan Hunter,
	Mantravadi Karthik, Shardar Mohammed, Timo Alho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 19:56, Sowjanya Komatineni пишет:
> 
>>>>  drivers/i2c/busses/Kconfig     |   2 +-
>>>>  drivers/i2c/busses/i2c-tegra.c | 362 
>>>> ++++++++++++++++++++++++++++++++++++++---
>>>>  2 files changed, 339 insertions(+), 25 deletions(-)
>>>
>>> Tegra20 crashes because of this patch:
>>>
>> [snip]
>>> <4>[    3.395915] ------------[ cut here ]------------
>>> <2>[    3.395919] kernel BUG at drivers/i2c/busses/i2c-tegra.c:810!
>> The BUG line is from:
>>
>> 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>> 		if (i2c_dev->is_curr_dma_xfer)
>> 			i2c_dev->msg_buf_remaining = 0;
>> 		BUG_ON(i2c_dev->msg_buf_remaining);
>> 		complete(&i2c_dev->msg_complete);
>> 	}
>>
> BUG_ON line is not part of this change. It was already there in existing driver.
> Based on log, I see DMA transfer is done for 224 bytes followed by 1 successful PIO transfer and then on next PIO transfer it received packet xfer complete interrupt with incomplete transfer bytes and that where it hit BUG_ON condition.
> 
> 

Yes, that BUG_ON is caused by the DMA transferring. Everything works fine be setting dma=false in the code, hence it's likely not a bug in the code (at least for now it looks fine), but likely that HW is not programmed correctly.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 17:11         ` Dmitry Osipenko
@ 2019-01-31 17:25           ` Dmitry Osipenko
  2019-01-31 17:38             ` Dmitry Osipenko
                               ` (2 more replies)
  0 siblings, 3 replies; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 17:25 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, Jonathan Hunter,
	Mantravadi Karthik, Shardar Mohammed, Timo Alho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 20:11, Dmitry Osipenko пишет:
> 31.01.2019 19:56, Sowjanya Komatineni пишет:
>>
>>>>>  drivers/i2c/busses/Kconfig     |   2 +-
>>>>>  drivers/i2c/busses/i2c-tegra.c | 362 
>>>>> ++++++++++++++++++++++++++++++++++++++---
>>>>>  2 files changed, 339 insertions(+), 25 deletions(-)
>>>>
>>>> Tegra20 crashes because of this patch:
>>>>
>>> [snip]
>>>> <4>[    3.395915] ------------[ cut here ]------------
>>>> <2>[    3.395919] kernel BUG at drivers/i2c/busses/i2c-tegra.c:810!
>>> The BUG line is from:
>>>
>>> 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>>> 		if (i2c_dev->is_curr_dma_xfer)
>>> 			i2c_dev->msg_buf_remaining = 0;
>>> 		BUG_ON(i2c_dev->msg_buf_remaining);
>>> 		complete(&i2c_dev->msg_complete);
>>> 	}
>>>
>> BUG_ON line is not part of this change. It was already there in existing driver.
>> Based on log, I see DMA transfer is done for 224 bytes followed by 1 successful PIO transfer and then on next PIO transfer it received packet xfer complete interrupt with incomplete transfer bytes and that where it hit BUG_ON condition.
>>
>>
> 
> Yes, that BUG_ON is caused by the DMA transferring. Everything works fine be setting dma=false in the code, hence it's likely not a bug in the code (at least for now it looks fine), but likely that HW is not programmed correctly.
> 

It works with this change:

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index fe5b89abc576..8e059e94b94e 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -1170,10 +1170,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
                time_left, completion_done(&i2c_dev->msg_complete),
                i2c_dev->msg_err);
 
+       tegra_i2c_init(i2c_dev, true);
+
        if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
                return 0;
 
-       tegra_i2c_init(i2c_dev, true);
        /* start recovery upon arbitration loss in single master mode */
        if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
                if (!i2c_dev->is_multimaster_mode)


Which means that HW state is kept dirty after DMA transfer. Please check everything carefully.

^ permalink raw reply related	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 17:25           ` Dmitry Osipenko
@ 2019-01-31 17:38             ` Dmitry Osipenko
  2019-01-31 17:38             ` Sowjanya Komatineni
  2019-01-31 21:46             ` Sowjanya Komatineni
  2 siblings, 0 replies; 32+ messages in thread
From: Dmitry Osipenko @ 2019-01-31 17:38 UTC (permalink / raw)
  To: Sowjanya Komatineni, thierry.reding, Jonathan Hunter,
	Mantravadi Karthik, Shardar Mohammed, Timo Alho
  Cc: linux-tegra, linux-kernel, linux-i2c

31.01.2019 20:25, Dmitry Osipenko пишет:
> 31.01.2019 20:11, Dmitry Osipenko пишет:
>> 31.01.2019 19:56, Sowjanya Komatineni пишет:
>>>
>>>>>>  drivers/i2c/busses/Kconfig     |   2 +-
>>>>>>  drivers/i2c/busses/i2c-tegra.c | 362 
>>>>>> ++++++++++++++++++++++++++++++++++++++---
>>>>>>  2 files changed, 339 insertions(+), 25 deletions(-)
>>>>>
>>>>> Tegra20 crashes because of this patch:
>>>>>
>>>> [snip]
>>>>> <4>[    3.395915] ------------[ cut here ]------------
>>>>> <2>[    3.395919] kernel BUG at drivers/i2c/busses/i2c-tegra.c:810!
>>>> The BUG line is from:
>>>>
>>>> 	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
>>>> 		if (i2c_dev->is_curr_dma_xfer)
>>>> 			i2c_dev->msg_buf_remaining = 0;
>>>> 		BUG_ON(i2c_dev->msg_buf_remaining);
>>>> 		complete(&i2c_dev->msg_complete);
>>>> 	}
>>>>
>>> BUG_ON line is not part of this change. It was already there in existing driver.
>>> Based on log, I see DMA transfer is done for 224 bytes followed by 1 successful PIO transfer and then on next PIO transfer it received packet xfer complete interrupt with incomplete transfer bytes and that where it hit BUG_ON condition.
>>>
>>>
>>
>> Yes, that BUG_ON is caused by the DMA transferring. Everything works fine be setting dma=false in the code, hence it's likely not a bug in the code (at least for now it looks fine), but likely that HW is not programmed correctly.
>>
> 
> It works with this change:
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index fe5b89abc576..8e059e94b94e 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1170,10 +1170,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>                 time_left, completion_done(&i2c_dev->msg_complete),
>                 i2c_dev->msg_err);
>  
> +       tegra_i2c_init(i2c_dev, true);
> +
>         if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
>                 return 0;
>  
> -       tegra_i2c_init(i2c_dev, true);
>         /* start recovery upon arbitration loss in single master mode */
>         if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
>                 if (!i2c_dev->is_multimaster_mode)
> 
> 
> Which means that HW state is kept dirty after DMA transfer. Please check everything carefully.
> 

Also, enforcing dma=true regardless of transfer size doesn't work as well.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 17:25           ` Dmitry Osipenko
  2019-01-31 17:38             ` Dmitry Osipenko
@ 2019-01-31 17:38             ` Sowjanya Komatineni
  2019-01-31 21:46             ` Sowjanya Komatineni
  2 siblings, 0 replies; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31 17:38 UTC (permalink / raw)
  To: Dmitry Osipenko, thierry.reding, Jonathan Hunter,
	Mantravadi Karthik, Shardar Mohammed, Timo Alho
  Cc: linux-tegra, linux-kernel, linux-i2c



>>>
>> BUG_ON line is not part of this change. It was already there in existing driver.
>> Based on log, I see DMA transfer is done for 224 bytes followed by 1 successful PIO transfer and then on next PIO transfer it received packet xfer complete interrupt with incomplete transfer bytes and that where it hit BUG_ON condition.
>>
>>
> 
> Yes, that BUG_ON is caused by the DMA transferring. Everything works fine be setting dma=false in the code, hence it's likely not a bug in the code (at least for now it looks fine), but likely that HW is not programmed correctly.
> 

> It works with this change:
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index fe5b89abc576..8e059e94b94e 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1170,10 +1170,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>                 time_left, completion_done(&i2c_dev->msg_complete),
>                 i2c_dev->msg_err);
>
> +       tegra_i2c_init(i2c_dev, true);
> +
>         if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
>                 return 0;
>  
> -       tegra_i2c_init(i2c_dev, true);
>         /* start recovery upon arbitration loss in single master mode */
>         if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
>                 if (!i2c_dev->is_multimaster_mode)
>
>
> Which means that HW state is kept dirty after DMA transfer. Please check everything carefully.

FIFO trigger levels are diff for PIO Vs DMA so it need to be moved to xfer function. Will fix that.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 17:25           ` Dmitry Osipenko
  2019-01-31 17:38             ` Dmitry Osipenko
  2019-01-31 17:38             ` Sowjanya Komatineni
@ 2019-01-31 21:46             ` Sowjanya Komatineni
  2019-02-01  0:54               ` Dmitry Osipenko
  2 siblings, 1 reply; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-01-31 21:46 UTC (permalink / raw)
  To: Dmitry Osipenko, thierry.reding, Jonathan Hunter,
	Mantravadi Karthik, Shardar Mohammed, Timo Alho
  Cc: linux-tegra, linux-kernel, linux-i2c

> It works with this change:
>
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c index fe5b89abc576..8e059e94b94e 100644
> --- a/drivers/i2c/busses/i2c-tegra.c
> +++ b/drivers/i2c/busses/i2c-tegra.c
> @@ -1170,10 +1170,11 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>                 time_left, completion_done(&i2c_dev->msg_complete),
>                 i2c_dev->msg_err);
>
> +       tegra_i2c_init(i2c_dev, true);
> +
>         if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
>                 return 0;
>  
> -       tegra_i2c_init(i2c_dev, true);
>         /* start recovery upon arbitration loss in single master mode */
>         if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
>                 if (!i2c_dev->is_multimaster_mode)
>
>
> Which means that HW state is kept dirty after DMA transfer. Please check everything carefully.

Identified issue, When I moved logic around for diff changes, INT enable isn’t correct for FIFO DATA REQ. Will have proper fix in next version. 


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 12:44   ` Thierry Reding
  2019-01-31 16:41     ` Sowjanya Komatineni
@ 2019-02-01  0:52     ` Dmitry Osipenko
  2019-02-01  1:11       ` Sowjanya Komatineni
  1 sibling, 1 reply; 32+ messages in thread
From: Dmitry Osipenko @ 2019-02-01  0:52 UTC (permalink / raw)
  To: Thierry Reding
  Cc: Sowjanya Komatineni, jonathanh, mkarthik, smohammed, talho,
	linux-tegra, linux-kernel, linux-i2c

В Thu, 31 Jan 2019 13:44:23 +0100
Thierry Reding <thierry.reding@gmail.com> пишет:

> On Wed, Jan 30, 2019 at 10:16:25PM -0800, Sowjanya Komatineni wrote:
> > This patch adds DMA support for Tegra I2C.
> > 
> > Tegra I2C TX and RX FIFO depth is 8 words. PIO mode is used for
> > transfer size of the max FIFO depth and DMA mode is used for
> > transfer size higher than max FIFO depth to save CPU overhead.
> > 
> > PIO mode needs full intervention of CPU to fill or empty FIFO's
> > and also need to service multiple data requests interrupt for the
> > same transaction. This adds delay between data bytes of the same
> > transfer when CPU is fully loaded and some slave devices has
> > internal timeout for no bus activity and stops transaction to
> > avoid bus hang. DMA mode is helpful in such cases.
> > 
> > DMA mode is also helpful for Large transfers during downloading or
> > uploading FW over I2C to some external devices.
> > 
> > Signed-off-by: Sowjanya Komatineni <skomatineni@nvidia.com>
> > ---
> >  [V8] : Moved back dma init to i2c probe, removed
> > ALL_PACKETS_XFER_COMPLETE interrupt and using PACKETS_XFER_COMPLETE
> > interrupt only and some other fixes
> > 	Updated Kconfig for APB_DMA dependency
> >  [V7] : Same as V6
> >  [V6] : Updated for proper buffer allocation/freeing, channel
> > release. Updated to use exact xfer size for syncing dma buffer.
> >  [V5] : Same as V4
> >  [V4] : Updated to allocate DMA buffer only when DMA mode.
> > 	Updated to fall back to PIO mode when DMA channel request or
> > 	buffer allocation fails.
> >  [V3] : Updated without additional buffer allocation.
> >  [V2] : Updated based on V1 review feedback along with code cleanup
> > for proper implementation of DMA.
> > 
> >  drivers/i2c/busses/Kconfig     |   2 +-
> >  drivers/i2c/busses/i2c-tegra.c | 362
> > ++++++++++++++++++++++++++++++++++++++--- 2 files changed, 339
> > insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
> > index f2c681971201..046aeb92a467 100644
> > --- a/drivers/i2c/busses/Kconfig
> > +++ b/drivers/i2c/busses/Kconfig
> > @@ -1016,7 +1016,7 @@ config I2C_SYNQUACER
> >  
> >  config I2C_TEGRA
> >  	tristate "NVIDIA Tegra internal I2C controller"
> > -	depends on ARCH_TEGRA
> > +	depends on (ARCH_TEGRA && TEGRA20_APB_DMA)  
> 
> Like I said in my reply in the v7 subthread, I don't think we want
> this. The dependency that we have is on the DMA engine API, not the
> APB DMA driver.
> 
> Technically there could be a runtime problem if the APB DMA driver is
> disabled and we list a "dmas" property. If I understand correctly, the
> DMA engine API would always return -EPROBE_DEFER in that case. That's
> somewhat annoying, but I think that's fine because it points at an
> integration issue. It lets you know that the driver is relying on a
> resources that is not showing up, which usually means that either the
> provider's driver is not enabled or the provider is failing to probe.
> 
> >  	help
> >  	  If you say yes to this option, support will be included
> > for the I2C controller embedded in NVIDIA Tegra SOCs
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c index c4892a47a483..025d63972e50
> > 100644 --- a/drivers/i2c/busses/i2c-tegra.c
> > +++ b/drivers/i2c/busses/i2c-tegra.c
> > @@ -8,6 +8,9 @@
> >  
> >  #include <linux/clk.h>
> >  #include <linux/delay.h>
> > +#include <linux/dmaengine.h>
> > +#include <linux/dmapool.h>
> > +#include <linux/dma-mapping.h>
> >  #include <linux/err.h>
> >  #include <linux/i2c.h>
> >  #include <linux/init.h>
> > @@ -44,6 +47,8 @@
> >  #define I2C_FIFO_CONTROL_RX_FLUSH		BIT(0)
> >  #define I2C_FIFO_CONTROL_TX_TRIG_SHIFT		5
> >  #define I2C_FIFO_CONTROL_RX_TRIG_SHIFT		2
> > +#define I2C_FIFO_CONTROL_TX_TRIG(x)		(((x) - 1) << 5)
> > +#define I2C_FIFO_CONTROL_RX_TRIG(x)		(((x) - 1) << 2)
> >  #define I2C_FIFO_STATUS				0x060
> >  #define I2C_FIFO_STATUS_TX_MASK			0xF0
> >  #define I2C_FIFO_STATUS_TX_SHIFT		4
> > @@ -125,6 +130,19 @@
> >  #define I2C_MST_FIFO_STATUS_TX_MASK		0xff0000
> >  #define I2C_MST_FIFO_STATUS_TX_SHIFT		16
> >  
> > +/* Packet header size in bytes */
> > +#define I2C_PACKET_HEADER_SIZE			12
> > +
> > +#define DATA_DMA_DIR_TX				(1 << 0)
> > +#define DATA_DMA_DIR_RX				(1 << 1)
> > +
> > +/*
> > + * Upto I2C_PIO_MODE_MAX_LEN bytes, controller will use PIO mode,
> > + * above this, controller will use DMA to fill FIFO.
> > + * MAX PIO len is 20 bytes excluding packet header.
> > + */
> > +#define I2C_PIO_MODE_MAX_LEN			32
> > +
> >  /*
> >   * msg_end_type: The bus control which need to be send at end of
> > transfer.
> >   * @MSG_END_STOP: Send stop pulse at end of transfer.
> > @@ -188,6 +206,7 @@ struct tegra_i2c_hw_feature {
> >   * @fast_clk: clock reference for fast clock of I2C controller
> >   * @rst: reset control for the I2C controller
> >   * @base: ioremapped registers cookie
> > + * @base_phys: Physical base address of the I2C controller
> >   * @cont_id: I2C controller ID, used for packet header
> >   * @irq: IRQ number of transfer complete interrupt
> >   * @irq_disabled: used to track whether or not the interrupt is
> > enabled @@ -201,6 +220,14 @@ struct tegra_i2c_hw_feature {
> >   * @clk_divisor_non_hs_mode: clock divider for non-high-speed modes
> >   * @is_multimaster_mode: track if I2C controller is in
> > multi-master mode
> >   * @xfer_lock: lock to serialize transfer submission and processing
> > + * @has_dma: indicates if DMA can be utilized based on dma DT
> > bindings  
> 
> I don't think we need this. We can just rely on the DMA engine API to
> tell us if the "dmas" property isn't there.
> 
> > + * @tx_dma_chan: DMA transmit channel
> > + * @rx_dma_chan: DMA receive channel
> > + * @dma_phys: handle to DMA resources
> > + * @dma_buf: pointer to allocated DMA buffer
> > + * @dma_buf_size: DMA buffer size
> > + * @is_curr_dma_xfer: indicates active DMA transfer
> > + * @dma_complete: DMA completion notifier
> >   */
> >  struct tegra_i2c_dev {
> >  	struct device *dev;
> > @@ -210,6 +237,7 @@ struct tegra_i2c_dev {
> >  	struct clk *fast_clk;
> >  	struct reset_control *rst;
> >  	void __iomem *base;
> > +	phys_addr_t base_phys;
> >  	int cont_id;
> >  	int irq;
> >  	bool irq_disabled;
> > @@ -223,6 +251,14 @@ struct tegra_i2c_dev {
> >  	u16 clk_divisor_non_hs_mode;
> >  	bool is_multimaster_mode;
> >  	spinlock_t xfer_lock;
> > +	bool has_dma;
> > +	struct dma_chan *tx_dma_chan;
> > +	struct dma_chan *rx_dma_chan;
> > +	dma_addr_t dma_phys;
> > +	u32 *dma_buf;
> > +	unsigned int dma_buf_size;
> > +	bool is_curr_dma_xfer;
> > +	struct completion dma_complete;
> >  };
> >  
> >  static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
> > @@ -291,6 +327,85 @@ static void tegra_i2c_unmask_irq(struct
> > tegra_i2c_dev *i2c_dev, u32 mask) i2c_writel(i2c_dev, int_mask,
> > I2C_INT_MASK); }
> >  
> > +static void tegra_i2c_dma_complete(void *args)
> > +{
> > +	struct tegra_i2c_dev *i2c_dev = args;
> > +
> > +	complete(&i2c_dev->dma_complete);
> > +}
> > +
> > +static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev,
> > size_t len) +{
> > +	struct dma_async_tx_descriptor *dma_desc;
> > +	enum dma_transfer_direction dir;
> > +	struct dma_chan *chan;
> > +
> > +	dev_dbg(i2c_dev->dev, "starting DMA for length: %zu\n",
> > len);
> > +	reinit_completion(&i2c_dev->dma_complete);
> > +	dir = i2c_dev->msg_read ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV;
> > +	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan :
> > i2c_dev->tx_dma_chan;
> > +	dma_desc = dmaengine_prep_slave_single(chan,
> > i2c_dev->dma_phys,
> > +					       len, dir,
> > DMA_PREP_INTERRUPT |
> > +					       DMA_CTRL_ACK);
> > +	if (!dma_desc) {
> > +		dev_err(i2c_dev->dev, "failed to get DMA
> > descriptor\n");
> > +		return -EIO;
> > +	}
> > +
> > +	dma_desc->callback = tegra_i2c_dma_complete;
> > +	dma_desc->callback_param = i2c_dev;
> > +	dmaengine_submit(dma_desc);
> > +	dma_async_issue_pending(chan);
> > +	return 0;
> > +}
> > +
> > +static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev)
> > +{
> > +	struct dma_chan *dma_chan;
> > +	u32 *dma_buf;
> > +	dma_addr_t dma_phys;
> > +
> > +	if (!i2c_dev->has_dma)
> > +		return -EINVAL;
> > +
> > +	if (!i2c_dev->rx_dma_chan) {
> > +		dma_chan =
> > dma_request_slave_channel_reason(i2c_dev->dev, "rx");
> > +		if (IS_ERR(dma_chan))
> > +			return PTR_ERR(dma_chan);  
> 
> I think we want to fallback to PIO here if dma_chan is -ENODEV.
> 
> > +
> > +		i2c_dev->rx_dma_chan = dma_chan;
> > +	}
> > +
> > +	if (!i2c_dev->tx_dma_chan) {
> > +		dma_chan =
> > dma_request_slave_channel_reason(i2c_dev->dev, "tx");
> > +		if (IS_ERR(dma_chan))
> > +			return PTR_ERR(dma_chan);  
> 
> Same here. We could use rx_dma_chan == NULL as a condition to detect
> that instead of the extra has_dma.
> 
> > +		i2c_dev->tx_dma_chan = dma_chan;
> > +	}  
> 
> Although, I'm not exactly sure I understand what you're trying to
> achieve here. Shouldn't we move the channel request parts into probe
> and remove them from here? Otherwise it seems like we could get into
> a state where we keep trying to get the slave channels everytime we
> set up a DMA transfer, even if we already failed to do so during
> probe.
> 
> > +
> > +	if (!i2c_dev->dma_buf && i2c_dev->msg_buf_remaining) {
> > +		dma_buf = dma_alloc_coherent(i2c_dev->dev,
> > +					     i2c_dev->dma_buf_size,
> > +					     &dma_phys,
> > GFP_KERNEL); +
> > +		if (!dma_buf) {
> > +			dev_err(i2c_dev->dev,
> > +				"failed to allocate the DMA
> > buffer\n");
> > +			dma_release_channel(i2c_dev->tx_dma_chan);
> > +			dma_release_channel(i2c_dev->rx_dma_chan);
> > +			i2c_dev->tx_dma_chan = NULL;
> > +			i2c_dev->rx_dma_chan = NULL;
> > +			return -ENOMEM;
> > +		}
> > +
> > +		i2c_dev->dma_buf = dma_buf;
> > +		i2c_dev->dma_phys = dma_phys;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
> >  {
> >  	unsigned long timeout = jiffies + HZ;
> > @@ -656,25 +771,38 @@ static irqreturn_t tegra_i2c_isr(int irq,
> > void *dev_id) if (i2c_dev->hw->supports_bus_clear && (status &
> > I2C_INT_BUS_CLR_DONE)) goto err;
> >  
> > -	if (i2c_dev->msg_read && (status &
> > I2C_INT_RX_FIFO_DATA_REQ)) {
> > -		if (i2c_dev->msg_buf_remaining)
> > -			tegra_i2c_empty_rx_fifo(i2c_dev);
> > -		else
> > -			BUG();
> > -	}
> > +	if (!i2c_dev->is_curr_dma_xfer) {
> > +		if (i2c_dev->msg_read && (status &
> > I2C_INT_RX_FIFO_DATA_REQ)) {
> > +			if (i2c_dev->msg_buf_remaining)
> > +				tegra_i2c_empty_rx_fifo(i2c_dev);
> > +			else
> > +				BUG();
> > +		}
> >  
> > -	if (!i2c_dev->msg_read && (status &
> > I2C_INT_TX_FIFO_DATA_REQ)) {
> > -		if (i2c_dev->msg_buf_remaining)
> > -			tegra_i2c_fill_tx_fifo(i2c_dev);
> > -		else
> > -			tegra_i2c_mask_irq(i2c_dev,
> > I2C_INT_TX_FIFO_DATA_REQ);
> > +		if (!i2c_dev->msg_read &&
> > +		   (status & I2C_INT_TX_FIFO_DATA_REQ)) {
> > +			if (i2c_dev->msg_buf_remaining)
> > +				tegra_i2c_fill_tx_fifo(i2c_dev);
> > +			else
> > +				tegra_i2c_mask_irq(i2c_dev,
> > +
> > I2C_INT_TX_FIFO_DATA_REQ);
> > +		}
> >  	}
> >  
> >  	i2c_writel(i2c_dev, status, I2C_INT_STATUS);
> >  	if (i2c_dev->is_dvc)
> >  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR,
> > DVC_STATUS); 
> > +	/*
> > +	 * During message read XFER_COMPLETE interrupt is
> > triggered prior to
> > +	 * DMA completion and during message write XFER_COMPLETE
> > interrupt is
> > +	 * triggered after DMA completion.
> > +	 * PACKETS_XFER_COMPLETE indicates completion of all bytes
> > of transfer.
> > +	 * so forcing msg_buf_remaining to 0 in DMA mode.
> > +	 */
> >  	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
> > +		if (i2c_dev->is_curr_dma_xfer)
> > +			i2c_dev->msg_buf_remaining = 0;
> >  		BUG_ON(i2c_dev->msg_buf_remaining);
> >  		complete(&i2c_dev->msg_complete);
> >  	}
> > @@ -690,12 +818,69 @@ static irqreturn_t tegra_i2c_isr(int irq,
> > void *dev_id) if (i2c_dev->is_dvc)
> >  		dvc_writel(i2c_dev, DVC_STATUS_I2C_DONE_INTR,
> > DVC_STATUS); 
> > +	if (i2c_dev->is_curr_dma_xfer) {
> > +		if (i2c_dev->msg_read)
> > +
> > dmaengine_terminate_all(i2c_dev->rx_dma_chan);
> > +		else
> > +
> > dmaengine_terminate_all(i2c_dev->tx_dma_chan); +
> > +		complete(&i2c_dev->dma_complete);
> > +	}
> > +
> >  	complete(&i2c_dev->msg_complete);
> >  done:
> >  	spin_unlock(&i2c_dev->xfer_lock);
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +static void tegra_i2c_config_fifo_trig(struct tegra_i2c_dev
> > *i2c_dev,
> > +				       size_t len, int direction)
> > +{
> > +	u32 val, reg;
> > +	u8 dma_burst = 0;
> > +	struct dma_slave_config dma_sconfig;
> > +	struct dma_chan *chan;
> > +
> > +	if (i2c_dev->hw->has_mst_fifo)
> > +		reg = I2C_MST_FIFO_CONTROL;
> > +	else
> > +		reg = I2C_FIFO_CONTROL;
> > +	val = i2c_readl(i2c_dev, reg);
> > +
> > +	if (len & 0xF)
> > +		dma_burst = 1;
> > +	else if (len & 0x10)
> > +		dma_burst = 4;
> > +	else
> > +		dma_burst = 8;
> > +
> > +	if (direction == DATA_DMA_DIR_TX) {
> > +		if (i2c_dev->hw->has_mst_fifo)
> > +			val |=
> > I2C_MST_FIFO_CONTROL_TX_TRIG(dma_burst);
> > +		else
> > +			val |= I2C_FIFO_CONTROL_TX_TRIG(dma_burst);
> > +	} else {
> > +		if (i2c_dev->hw->has_mst_fifo)
> > +			val |=
> > I2C_MST_FIFO_CONTROL_RX_TRIG(dma_burst);
> > +		else
> > +			val |= I2C_FIFO_CONTROL_RX_TRIG(dma_burst);
> > +	}
> > +	i2c_writel(i2c_dev, val, reg);
> > +
> > +	if (direction == DATA_DMA_DIR_TX) {
> > +		dma_sconfig.dst_addr = i2c_dev->base_phys +
> > I2C_TX_FIFO;
> > +		dma_sconfig.dst_addr_width =
> > DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +		dma_sconfig.dst_maxburst = dma_burst;
> > +	} else {
> > +		dma_sconfig.src_addr = i2c_dev->base_phys +
> > I2C_RX_FIFO;
> > +		dma_sconfig.src_addr_width =
> > DMA_SLAVE_BUSWIDTH_4_BYTES;
> > +		dma_sconfig.src_maxburst = dma_burst;
> > +	}
> > +
> > +	chan = i2c_dev->msg_read ? i2c_dev->rx_dma_chan :
> > i2c_dev->tx_dma_chan;
> > +	dmaengine_slave_config(chan, &dma_sconfig);
> > +}
> > +
> >  static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
> >  {
> >  	int err;
> > @@ -740,6 +925,11 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev, u32 int_mask;
> >  	unsigned long time_left;
> >  	unsigned long flags;
> > +	size_t xfer_size;
> > +	u32 *buffer = 0;  
> 
> Usually this should be = NULL for pointers.
> 
> > +	int err = 0;
> > +	bool dma = false;
> > +	struct dma_chan *chan;
> >  
> >  	tegra_i2c_flush_fifos(i2c_dev);
> >  
> > @@ -749,19 +939,69 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev, i2c_dev->msg_read = (msg->flags & I2C_M_RD);
> >  	reinit_completion(&i2c_dev->msg_complete);
> >  
> > +	if (i2c_dev->msg_read)
> > +		xfer_size = msg->len;
> > +	else
> > +		xfer_size = msg->len + I2C_PACKET_HEADER_SIZE;
> > +
> > +	xfer_size = ALIGN(xfer_size, BYTES_PER_FIFO_WORD);
> > +	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
> > +	if (dma) {
> > +		err = tegra_i2c_init_dma_param(i2c_dev);
> > +		if (err < 0) {
> > +			dev_dbg(i2c_dev->dev, "switching to PIO
> > transfer\n");
> > +			dma = false;
> > +		}  
> 
> If we successfully got DMA channels at probe time, doesn't this turn
> into an error condition that is worth reporting? It seems to me like
> the only reason it could fail is if we fail the allocation, but then
> again, why don't we move the DMA buffer allocation into probe()? We
> already use a fixed size for that allocation, so there's no reason it
> couldn't be allocated at probe time.
> 
> Seems like maybe you just overlooked that as you were moving around
> the code pieces.
> 
> > +	}
> > +
> > +	i2c_dev->is_curr_dma_xfer = dma;
> >  	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
> >  
> >  	int_mask = I2C_INT_NO_ACK | I2C_INT_ARBITRATION_LOST;
> >  	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> >  
> > +	if (dma) {
> > +		if (i2c_dev->msg_read) {
> > +			chan = i2c_dev->rx_dma_chan;
> > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > xfer_size,
> > +
> > DATA_DMA_DIR_RX);
> > +			dma_sync_single_for_device(i2c_dev->dev,
> > +
> > i2c_dev->dma_phys,
> > +						   xfer_size,
> > +
> > DMA_FROM_DEVICE);  
> 
> Do we really need this? We're not actually passing the device any
> data, so no caches to flush here. I we're cautious about flushing
> caches when we do write to the buffer (and I think we do that
> properly already), then there should be no need to do it here again.
> 

IIUC, DMA API has a concept of buffer handing which tells to use
dma_sync_single_for_device() before issuing hardware job that touches
the buffer and to use dma_sync_single_for_cpu() after hardware done the
execution. In fact the CPU caches are getting flushed or invalidated as
appropriate in a result.

dma_sync_single_for_device(DMA_FROM_DEVICE) invalidates buffer in the
CPU cache, probably to avoid CPU evicting data from cache to
DRAM while hardware writes to the buffer. Hence this hunk is correct.

> > +			err = tegra_i2c_dma_submit(i2c_dev,
> > xfer_size);
> > +			if (err < 0) {
> > +				dev_err(i2c_dev->dev,
> > +					"starting RX DMA failed,
> > err %d\n",
> > +					err);
> > +				goto unlock;
> > +			}
> > +		} else {
> > +			chan = i2c_dev->tx_dma_chan;
> > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > xfer_size,
> > +
> > DATA_DMA_DIR_TX);
> > +			dma_sync_single_for_cpu(i2c_dev->dev,
> > +						i2c_dev->dma_phys,
> > +						xfer_size,
> > +						DMA_TO_DEVICE);  
> 
> This, on the other hand seems correct because we need to invalidate
> the caches for this buffer to make sure the data that we put there
> doesn't get overwritten.

As I stated before in a comment to v6, this particular case of
dma_sync_single_for_cpu() usage is incorrect because CPU should take
ownership of the buffer after completion of hardwate job. But in fact
dma_sync_single_for_cpu(DMA_TO_DEVICE) is a NO-OP because CPU doesn't
need to flush or invalidate anything to take ownership of the buffer if
hardware did a read-only access.

> 
> > +			buffer = i2c_dev->dma_buf;
> > +		}
> > +	}
> > +
> >  	packet_header = (0 << PACKET_HEADER0_HEADER_SIZE_SHIFT) |
> >  			PACKET_HEADER0_PROTOCOL_I2C |
> >  			(i2c_dev->cont_id <<
> > PACKET_HEADER0_CONT_ID_SHIFT) | (1 <<
> > PACKET_HEADER0_PACKET_ID_SHIFT);
> > -	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > +	if (dma && !i2c_dev->msg_read)
> > +		*buffer++ = packet_header;
> > +	else
> > +		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> >  
> >  	packet_header = msg->len - 1;
> > -	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > +	if (dma && !i2c_dev->msg_read)
> > +		*buffer++ = packet_header;
> > +	else
> > +		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> >  
> >  	packet_header = I2C_HEADER_IE_ENABLE;
> >  	if (end_state == MSG_END_CONTINUE)
> > @@ -778,30 +1018,79 @@ static int tegra_i2c_xfer_msg(struct
> > tegra_i2c_dev *i2c_dev, packet_header |= I2C_HEADER_CONT_ON_NAK;
> >  	if (msg->flags & I2C_M_RD)
> >  		packet_header |= I2C_HEADER_READ;
> > -	i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > -
> > -	if (!(msg->flags & I2C_M_RD))
> > -		tegra_i2c_fill_tx_fifo(i2c_dev);
> > -
> > +	if (dma && !i2c_dev->msg_read)
> > +		*buffer++ = packet_header;
> > +	else
> > +		i2c_writel(i2c_dev, packet_header, I2C_TX_FIFO);
> > +
> > +	if (!i2c_dev->msg_read) {
> > +		if (dma) {
> > +			memcpy(buffer, msg->buf, msg->len);
> > +			dma_sync_single_for_device(i2c_dev->dev,
> > +
> > i2c_dev->dma_phys,
> > +						   xfer_size,
> > +
> > DMA_TO_DEVICE);  
> 
> Again, here we properly flush the caches to make sure the data that
> we've written to the DMA buffer is visible to the DMA engine.
> 

+1 this is correct

> > +			err = tegra_i2c_dma_submit(i2c_dev,
> > xfer_size);
> > +			if (err < 0) {
> > +				dev_err(i2c_dev->dev,
> > +					"starting TX DMA failed,
> > err %d\n",
> > +					err);
> > +				goto unlock;
> > +			}
> > +		} else {
> > +			tegra_i2c_fill_tx_fifo(i2c_dev);
> > +		}
> > +	}
> >  	if (i2c_dev->hw->has_per_pkt_xfer_complete_irq)
> >  		int_mask |= I2C_INT_PACKET_XFER_COMPLETE;
> > -	if (msg->flags & I2C_M_RD)
> > -		int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> > -	else if (i2c_dev->msg_buf_remaining)
> > -		int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> > +	if (!dma) {
> > +		if (msg->flags & I2C_M_RD)
> > +			int_mask |= I2C_INT_RX_FIFO_DATA_REQ;
> > +		else if (i2c_dev->msg_buf_remaining)
> > +			int_mask |= I2C_INT_TX_FIFO_DATA_REQ;
> > +	}
> >  
> >  	tegra_i2c_unmask_irq(i2c_dev, int_mask);
> > -	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
> >  	dev_dbg(i2c_dev->dev, "unmasked irq: %02x\n",
> >  		i2c_readl(i2c_dev, I2C_INT_MASK));
> >  
> > +unlock:
> > +	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
> > +
> > +	if (dma) {
> > +		if (err)
> > +			return err;
> > +
> > +		time_left = wait_for_completion_timeout(
> > +
> > &i2c_dev->dma_complete,
> > +						TEGRA_I2C_TIMEOUT);
> > +
> > +		if (time_left == 0) {
> > +			dev_err(i2c_dev->dev, "DMA transfer
> > timeout\n");
> > +			dmaengine_terminate_all(chan);
> > +			tegra_i2c_init(i2c_dev);
> > +			return -ETIMEDOUT;
> > +		}
> > +
> > +		if (i2c_dev->msg_read) {
> > +			if (likely(i2c_dev->msg_err ==
> > I2C_ERR_NONE)) {
> > +
> > dma_sync_single_for_cpu(i2c_dev->dev,
> > +
> > i2c_dev->dma_phys,
> > +							xfer_size,
> > +
> > DMA_FROM_DEVICE);  
> 
> Here we invalidate the caches to make sure we don't get stale data
> that may be in the caches for data that we're copying out of the DMA
> buffer. I think that's about all the cache maintenance that we
> real
> need.

Correct.

And technically here should be dma_sync_single_for_cpu(DMA_TO_DEVICE)
for the TX. But again, it's a NO-OP.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-01-31 21:46             ` Sowjanya Komatineni
@ 2019-02-01  0:54               ` Dmitry Osipenko
  0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Osipenko @ 2019-02-01  0:54 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: thierry.reding, Jonathan Hunter, Mantravadi Karthik,
	Shardar Mohammed, Timo Alho, linux-tegra, linux-kernel,
	linux-i2c

В Thu, 31 Jan 2019 21:46:48 +0000
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> > It works with this change:
> >
> > diff --git a/drivers/i2c/busses/i2c-tegra.c
> > b/drivers/i2c/busses/i2c-tegra.c index fe5b89abc576..8e059e94b94e
> > 100644 --- a/drivers/i2c/busses/i2c-tegra.c +++
> > b/drivers/i2c/busses/i2c-tegra.c @@ -1170,10 +1170,11 @@ static int
> > tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev, time_left,
> > completion_done(&i2c_dev->msg_complete), i2c_dev->msg_err);
> >
> > +       tegra_i2c_init(i2c_dev, true);
> > +
> >         if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
> >                 return 0;
> >  
> > -       tegra_i2c_init(i2c_dev, true);
> >         /* start recovery upon arbitration loss in single master
> > mode */ if (i2c_dev->msg_err == I2C_ERR_ARBITRATION_LOST) {
> >                 if (!i2c_dev->is_multimaster_mode)
> >
> >
> > Which means that HW state is kept dirty after DMA transfer. Please
> > check everything carefully.  
> 
> Identified issue, When I moved logic around for diff changes, INT
> enable isn’t correct for FIFO DATA REQ. Will have proper fix in next
> version. 
> 

Awesome :)

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-02-01  0:52     ` Dmitry Osipenko
@ 2019-02-01  1:11       ` Sowjanya Komatineni
  2019-02-01  3:14         ` Dmitry Osipenko
  0 siblings, 1 reply; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-02-01  1:11 UTC (permalink / raw)
  To: Dmitry Osipenko, Thierry Reding
  Cc: Jonathan Hunter, Mantravadi Karthik, Shardar Mohammed, Timo Alho,
	linux-tegra, linux-kernel, linux-i2c

> > > +	if (dma) {
> > > +		if (i2c_dev->msg_read) {
> > > +			chan = i2c_dev->rx_dma_chan;
> > > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > > xfer_size,
> > > +
> > > DATA_DMA_DIR_RX);
> > > +			dma_sync_single_for_device(i2c_dev->dev,
> > > +
> > > i2c_dev->dma_phys,
> > > +						   xfer_size,
> > > +
> > > DMA_FROM_DEVICE);
> > 
> > Do we really need this? We're not actually passing the device any 
> > data, so no caches to flush here. I we're cautious about flushing 
> > caches when we do write to the buffer (and I think we do that properly 
> > already), then there should be no need to do it here again.
> > 
>
> IIUC, DMA API has a concept of buffer handing which tells to use
dma_sync_single_for_device() before issuing hardware job that touches the buffer and to use dma_sync_single_for_cpu() after hardware done the execution. In fact the CPU caches are getting flushed or invalidated as appropriate in a result.
>
> dma_sync_single_for_device(DMA_FROM_DEVICE) invalidates buffer in the CPU cache, probably to avoid CPU evicting data from cache to DRAM while hardware writes to the buffer. Hence this hunk is correct.
>
> > > +			err = tegra_i2c_dma_submit(i2c_dev,
> > > xfer_size);
> > > +			if (err < 0) {
> > > +				dev_err(i2c_dev->dev,
> > > +					"starting RX DMA failed,
> > > err %d\n",
> > > +					err);
> > > +				goto unlock;
> > > +			}
> > > +		} else {
> > > +			chan = i2c_dev->tx_dma_chan;
> > > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > > xfer_size,
> > > +
> > > DATA_DMA_DIR_TX);
> > > +			dma_sync_single_for_cpu(i2c_dev->dev,
> > > +						i2c_dev->dma_phys,
> > > +						xfer_size,
> > > +						DMA_TO_DEVICE);
> > 
> > This, on the other hand seems correct because we need to invalidate 
> > the caches for this buffer to make sure the data that we put there 
> > doesn't get overwritten.
>
> As I stated before in a comment to v6, this particular case of
> dma_sync_single_for_cpu() usage is incorrect because CPU should take ownership of the buffer after completion of hardwate job. But in fact
> dma_sync_single_for_cpu(DMA_TO_DEVICE) is a NO-OP because CPU doesn't need to flush or invalidate anything to take ownership of the buffer if hardware did a read-only access.
>
> > 
> > > +	if (!i2c_dev->msg_read) {
> > > +		if (dma) {
> > > +			memcpy(buffer, msg->buf, msg->len);
> > > +			dma_sync_single_for_device(i2c_dev->dev,
> > > +
> > > i2c_dev->dma_phys,
> > > +						   xfer_size,
> > > +
> > > DMA_TO_DEVICE);
> > 
> > Again, here we properly flush the caches to make sure the data that 
> > we've written to the DMA buffer is visible to the DMA engine.
> > 
>
> +1 this is correct
>
>
>
> > > +
> > > +		if (i2c_dev->msg_read) {
> > > +			if (likely(i2c_dev->msg_err ==
> > > I2C_ERR_NONE)) {
> > > +
> > > dma_sync_single_for_cpu(i2c_dev->dev,
> > > +
> > > i2c_dev->dma_phys,
> > > +							xfer_size,
> > > +
> > > DMA_FROM_DEVICE);
> > 
> > Here we invalidate the caches to make sure we don't get stale data 
> > that may be in the caches for data that we're copying out of the DMA 
> > buffer. I think that's about all the cache maintenance that we real 
> > need.
>
> Correct.
>
> And technically here should be dma_sync_single_for_cpu(DMA_TO_DEVICE)
> for the TX. But again, it's a NO-OP.

Is my below understanding correct? Can you please confirm?

During Transmit to device:
- Before writing msg data into dma buf by CPU, giving DMA ownership to CPU
	dma_sync_single_for_cpu with dir DMA_TO_DEVICE

- After writing to dma buf by CPU, giving back the ownership to device to access buffer to send during DMA transmit
	dma_sync_single_for_device with dir DMA_TO_DEVICE

During Receiving from Device:
- before submitting RX DMA to give buffer access to DMAengine
	dma_sync_single_for_Device(DMA_FROM_DEVICE) 
- after DMA RX completion, giving dma ownership to CPU for reading dmabuf data written by DMA from device
	dma_sync_single_for_cpu with dir DMA_FROM_DEVICE


^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-02-01  1:11       ` Sowjanya Komatineni
@ 2019-02-01  3:14         ` Dmitry Osipenko
  2019-02-01  4:19           ` Sowjanya Komatineni
  0 siblings, 1 reply; 32+ messages in thread
From: Dmitry Osipenko @ 2019-02-01  3:14 UTC (permalink / raw)
  To: Sowjanya Komatineni
  Cc: Thierry Reding, Jonathan Hunter, Mantravadi Karthik,
	Shardar Mohammed, Timo Alho, linux-tegra, linux-kernel,
	linux-i2c

В Fri, 1 Feb 2019 01:11:06 +0000
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> > > > +	if (dma) {
> > > > +		if (i2c_dev->msg_read) {
> > > > +			chan = i2c_dev->rx_dma_chan;
> > > > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > > > xfer_size,
> > > > +
> > > > DATA_DMA_DIR_RX);
> > > > +
> > > > dma_sync_single_for_device(i2c_dev->dev, +
> > > > i2c_dev->dma_phys,
> > > > +						   xfer_size,
> > > > +
> > > > DMA_FROM_DEVICE);  
> > > 
> > > Do we really need this? We're not actually passing the device any 
> > > data, so no caches to flush here. I we're cautious about flushing 
> > > caches when we do write to the buffer (and I think we do that
> > > properly already), then there should be no need to do it here
> > > again. 
> >
> > IIUC, DMA API has a concept of buffer handing which tells to use  
> dma_sync_single_for_device() before issuing hardware job that touches
> the buffer and to use dma_sync_single_for_cpu() after hardware done
> the execution. In fact the CPU caches are getting flushed or
> invalidated as appropriate in a result.
> >
> > dma_sync_single_for_device(DMA_FROM_DEVICE) invalidates buffer in
> > the CPU cache, probably to avoid CPU evicting data from cache to
> > DRAM while hardware writes to the buffer. Hence this hunk is
> > correct. 
> > > > +			err = tegra_i2c_dma_submit(i2c_dev,
> > > > xfer_size);
> > > > +			if (err < 0) {
> > > > +				dev_err(i2c_dev->dev,
> > > > +					"starting RX DMA
> > > > failed, err %d\n",
> > > > +					err);
> > > > +				goto unlock;
> > > > +			}
> > > > +		} else {
> > > > +			chan = i2c_dev->tx_dma_chan;
> > > > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > > > xfer_size,
> > > > +
> > > > DATA_DMA_DIR_TX);
> > > > +			dma_sync_single_for_cpu(i2c_dev->dev,
> > > > +
> > > > i2c_dev->dma_phys,
> > > > +						xfer_size,
> > > > +
> > > > DMA_TO_DEVICE);  
> > > 
> > > This, on the other hand seems correct because we need to
> > > invalidate the caches for this buffer to make sure the data that
> > > we put there doesn't get overwritten.  
> >
> > As I stated before in a comment to v6, this particular case of
> > dma_sync_single_for_cpu() usage is incorrect because CPU should
> > take ownership of the buffer after completion of hardwate job. But
> > in fact dma_sync_single_for_cpu(DMA_TO_DEVICE) is a NO-OP because
> > CPU doesn't need to flush or invalidate anything to take ownership
> > of the buffer if hardware did a read-only access. 
> > >   
> > > > +	if (!i2c_dev->msg_read) {
> > > > +		if (dma) {
> > > > +			memcpy(buffer, msg->buf, msg->len);
> > > > +
> > > > dma_sync_single_for_device(i2c_dev->dev, +
> > > > i2c_dev->dma_phys,
> > > > +						   xfer_size,
> > > > +
> > > > DMA_TO_DEVICE);  
> > > 
> > > Again, here we properly flush the caches to make sure the data
> > > that we've written to the DMA buffer is visible to the DMA engine.
> > >   
> >
> > +1 this is correct
> >
> >
> >  
> > > > +
> > > > +		if (i2c_dev->msg_read) {
> > > > +			if (likely(i2c_dev->msg_err ==
> > > > I2C_ERR_NONE)) {
> > > > +
> > > > dma_sync_single_for_cpu(i2c_dev->dev,
> > > > +
> > > > i2c_dev->dma_phys,
> > > > +
> > > > xfer_size, +
> > > > DMA_FROM_DEVICE);  
> > > 
> > > Here we invalidate the caches to make sure we don't get stale
> > > data that may be in the caches for data that we're copying out of
> > > the DMA buffer. I think that's about all the cache maintenance
> > > that we real need.  
> >
> > Correct.
> >
> > And technically here should be
> > dma_sync_single_for_cpu(DMA_TO_DEVICE) for the TX. But again, it's
> > a NO-OP.  
> 
> Is my below understanding correct? Can you please confirm?
> 
> During Transmit to device:
> - Before writing msg data into dma buf by CPU, giving DMA ownership
> to CPU dma_sync_single_for_cpu with dir DMA_TO_DEVICE
> 

I tried to take a look at it again and now thinking that your variant
is more correct. Still it's a bit difficult to judge because this case
is no-op.

> - After writing to dma buf by CPU, giving back the ownership to
> device to access buffer to send during DMA transmit
> dma_sync_single_for_device with dir DMA_TO_DEVICE

Correct.

> During Receiving from Device:
> - before submitting RX DMA to give buffer access to DMAengine
> 	dma_sync_single_for_Device(DMA_FROM_DEVICE) 

Correct.

> - after DMA RX completion, giving dma ownership to CPU for reading
> dmabuf data written by DMA from device dma_sync_single_for_cpu with
> dir DMA_FROM_DEVICE
> 

Correct.

^ permalink raw reply	[flat|nested] 32+ messages in thread

* RE: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-02-01  3:14         ` Dmitry Osipenko
@ 2019-02-01  4:19           ` Sowjanya Komatineni
  2019-02-01 15:02             ` Dmitry Osipenko
  0 siblings, 1 reply; 32+ messages in thread
From: Sowjanya Komatineni @ 2019-02-01  4:19 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Thierry Reding, Jonathan Hunter, Mantravadi Karthik,
	Shardar Mohammed, Timo Alho, linux-tegra, linux-kernel,
	linux-i2c

> > > > > +	if (dma) {
> > > > > +		if (i2c_dev->msg_read) {
> > > > > +			chan = i2c_dev->rx_dma_chan;
> > > > > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > > > > xfer_size,
> > > > > +
> > > > > DATA_DMA_DIR_RX);
> > > > > +
> > > > > dma_sync_single_for_device(i2c_dev->dev, + i2c_dev->dma_phys,
> > > > > +						   xfer_size,
> > > > > +
> > > > > DMA_FROM_DEVICE);
> > > > 
> > > > Do we really need this? We're not actually passing the device any 
> > > > data, so no caches to flush here. I we're cautious about flushing 
> > > > caches when we do write to the buffer (and I think we do that 
> > > > properly already), then there should be no need to do it here 
> > > > again.
> > >
> > > IIUC, DMA API has a concept of buffer handing which tells to use
> > dma_sync_single_for_device() before issuing hardware job that touches 
> > the buffer and to use dma_sync_single_for_cpu() after hardware done 
> > the execution. In fact the CPU caches are getting flushed or 
> > invalidated as appropriate in a result.
> > >
> > > dma_sync_single_for_device(DMA_FROM_DEVICE) invalidates buffer in 
> > > the CPU cache, probably to avoid CPU evicting data from cache to 
> > > DRAM while hardware writes to the buffer. Hence this hunk is 
> > > correct.
> > > > > +			err = tegra_i2c_dma_submit(i2c_dev,
> > > > > xfer_size);
> > > > > +			if (err < 0) {
> > > > > +				dev_err(i2c_dev->dev,
> > > > > +					"starting RX DMA
> > > > > failed, err %d\n",
> > > > > +					err);
> > > > > +				goto unlock;
> > > > > +			}
> > > > > +		} else {
> > > > > +			chan = i2c_dev->tx_dma_chan;
> > > > > +			tegra_i2c_config_fifo_trig(i2c_dev,
> > > > > xfer_size,
> > > > > +
> > > > > DATA_DMA_DIR_TX);
> > > > > +			dma_sync_single_for_cpu(i2c_dev->dev,
> > > > > +
> > > > > i2c_dev->dma_phys,
> > > > > +						xfer_size,
> > > > > +
> > > > > DMA_TO_DEVICE);
> > > > 
> > > > This, on the other hand seems correct because we need to 
> > > > invalidate the caches for this buffer to make sure the data that 
> > > > we put there doesn't get overwritten.
> > >
> > > As I stated before in a comment to v6, this particular case of
> > > dma_sync_single_for_cpu() usage is incorrect because CPU should take 
> > > ownership of the buffer after completion of hardwate job. But in 
> > > fact dma_sync_single_for_cpu(DMA_TO_DEVICE) is a NO-OP because CPU 
> > > doesn't need to flush or invalidate anything to take ownership of 
> > > the buffer if hardware did a read-only access.
> > > >   
> > > > > +	if (!i2c_dev->msg_read) {
> > > > > +		if (dma) {
> > > > > +			memcpy(buffer, msg->buf, msg->len);
> > > > > +
> > > > > dma_sync_single_for_device(i2c_dev->dev, + i2c_dev->dma_phys,
> > > > > +						   xfer_size,
> > > > > +
> > > > > DMA_TO_DEVICE);
> > > > 
> > > > Again, here we properly flush the caches to make sure the data 
> > > > that we've written to the DMA buffer is visible to the DMA engine.
> > > >   
> > >
> > > +1 this is correct
> > >
> > >
> > >  
> > > > > +
> > > > > +		if (i2c_dev->msg_read) {
> > > > > +			if (likely(i2c_dev->msg_err ==
> > > > > I2C_ERR_NONE)) {
> > > > > +
> > > > > dma_sync_single_for_cpu(i2c_dev->dev,
> > > > > +
> > > > > i2c_dev->dma_phys,
> > > > > +
> > > > > xfer_size, +
> > > > > DMA_FROM_DEVICE);
> > > > 
> > > > Here we invalidate the caches to make sure we don't get stale data 
> > > > that may be in the caches for data that we're copying out of the 
> > > > DMA buffer. I think that's about all the cache maintenance that we 
> > > > real need.
> > >
> > > Correct.
> > >
> > > And technically here should be
> > > dma_sync_single_for_cpu(DMA_TO_DEVICE) for the TX. But again, it's a 
> > > NO-OP.
> > 
> > Is my below understanding correct? Can you please confirm?
> > 
> > During Transmit to device:
> > - Before writing msg data into dma buf by CPU, giving DMA ownership to 
> > CPU dma_sync_single_for_cpu with dir DMA_TO_DEVICE
> > 
>
> I tried to take a look at it again and now thinking that your variant is more correct. Still it's a bit difficult to judge because this case is no-op.
>
> > - After writing to dma buf by CPU, giving back the ownership to device 
> > to access buffer to send during DMA transmit 
> > dma_sync_single_for_device with dir DMA_TO_DEVICE
>
> Correct.
>
> > During Receiving from Device:
> > - before submitting RX DMA to give buffer access to DMAengine
> > 	dma_sync_single_for_Device(DMA_FROM_DEVICE)
>
> Correct.
>
> > - after DMA RX completion, giving dma ownership to CPU for reading 
> > dmabuf data written by DMA from device dma_sync_single_for_cpu with 
> > dir DMA_FROM_DEVICE
> > 
>
> Correct.

Then what I have is exact as mentioned above. So no changes needed related to dma_sync
Pasting again here with clear comment to explain on why I have those corresponding dma_sync

        if (i2c_dev->msg_read) {
            tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
                           DATA_DMA_DIR_RX);
            /* For Reads: giving dma buf ownership to device before submitting RX DMA */
            dma_sync_single_for_device(i2c_dev->dev,
                           i2c_dev->dma_phys,
                           xfer_size,
                           DMA_FROM_DEVICE);
            err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
            if (err < 0) {
                dev_err(i2c_dev->dev,
                    "starting RX DMA failed, err %d\n",
                    err);
                goto unlock;
            }
        } else {
            tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
                           DATA_DMA_DIR_TX);
            /* For writes: giving dma buf ownership to CPU to copy transmit data to DMA Buf */
            dma_sync_single_for_cpu(i2c_dev->dev,
                        i2c_dev->dma_phys,
                        xfer_size,
                        DMA_TO_DEVICE);
            buffer = i2c_dev->dma_buf;
        }



    if (!msg->flags & I2C_M_RD) {
        if (dma) {
            memcpy(buffer, msg->buf, msg->len);
            /* For writes: giving ownership to device after done with copying data to DMA Buf */
            dma_sync_single_for_device(i2c_dev->dev,
                           i2c_dev->dma_phys,
                           xfer_size,
                           DMA_TO_DEVICE);
            err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
            if (err < 0) {
                dev_err(i2c_dev->dev,
                    "starting TX DMA failed, err %d\n",
                    err);
                goto unlock;
            }
        } else {
            tegra_i2c_fill_tx_fifo(i2c_dev);
        }

        if (i2c_dev->msg_read) {
            if (likely(i2c_dev->msg_err == I2C_ERR_NONE)) {
            /* For Reads: giving ownership to CPU after RX DMA completion to access read data */
                dma_sync_single_for_cpu(i2c_dev->dev,
                            i2c_dev->dma_phys,
                            xfer_size,
                            DMA_FROM_DEVICE);

                memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
                    msg->len);
            }
        }




^ permalink raw reply	[flat|nested] 32+ messages in thread

* Re: [PATCH V8 3/5] i2c: tegra: Add DMA Support
  2019-02-01  4:19           ` Sowjanya Komatineni
@ 2019-02-01 15:02             ` Dmitry Osipenko
  0 siblings, 0 replies; 32+ messages in thread
From: Dmitry Osipenko @ 2019-02-01 15:02 UTC (permalink / raw)
  To: Sowjanya Komatineni, Thierry Reding
  Cc: Jonathan Hunter, Mantravadi Karthik, Shardar Mohammed, Timo Alho,
	linux-tegra, linux-kernel, linux-i2c

01.02.2019 7:19, Sowjanya Komatineni пишет:
>>>>>> +	if (dma) {
>>>>>> +		if (i2c_dev->msg_read) {
>>>>>> +			chan = i2c_dev->rx_dma_chan;
>>>>>> +			tegra_i2c_config_fifo_trig(i2c_dev,
>>>>>> xfer_size,
>>>>>> +
>>>>>> DATA_DMA_DIR_RX);
>>>>>> +
>>>>>> dma_sync_single_for_device(i2c_dev->dev, + i2c_dev->dma_phys,
>>>>>> +						   xfer_size,
>>>>>> +
>>>>>> DMA_FROM_DEVICE);
>>>>>
>>>>> Do we really need this? We're not actually passing the device any 
>>>>> data, so no caches to flush here. I we're cautious about flushing 
>>>>> caches when we do write to the buffer (and I think we do that 
>>>>> properly already), then there should be no need to do it here 
>>>>> again.
>>>>
>>>> IIUC, DMA API has a concept of buffer handing which tells to use
>>> dma_sync_single_for_device() before issuing hardware job that touches 
>>> the buffer and to use dma_sync_single_for_cpu() after hardware done 
>>> the execution. In fact the CPU caches are getting flushed or 
>>> invalidated as appropriate in a result.
>>>>
>>>> dma_sync_single_for_device(DMA_FROM_DEVICE) invalidates buffer in 
>>>> the CPU cache, probably to avoid CPU evicting data from cache to 
>>>> DRAM while hardware writes to the buffer. Hence this hunk is 
>>>> correct.
>>>>>> +			err = tegra_i2c_dma_submit(i2c_dev,
>>>>>> xfer_size);
>>>>>> +			if (err < 0) {
>>>>>> +				dev_err(i2c_dev->dev,
>>>>>> +					"starting RX DMA
>>>>>> failed, err %d\n",
>>>>>> +					err);
>>>>>> +				goto unlock;
>>>>>> +			}
>>>>>> +		} else {
>>>>>> +			chan = i2c_dev->tx_dma_chan;
>>>>>> +			tegra_i2c_config_fifo_trig(i2c_dev,
>>>>>> xfer_size,
>>>>>> +
>>>>>> DATA_DMA_DIR_TX);
>>>>>> +			dma_sync_single_for_cpu(i2c_dev->dev,
>>>>>> +
>>>>>> i2c_dev->dma_phys,
>>>>>> +						xfer_size,
>>>>>> +
>>>>>> DMA_TO_DEVICE);
>>>>>
>>>>> This, on the other hand seems correct because we need to 
>>>>> invalidate the caches for this buffer to make sure the data that 
>>>>> we put there doesn't get overwritten.
>>>>
>>>> As I stated before in a comment to v6, this particular case of
>>>> dma_sync_single_for_cpu() usage is incorrect because CPU should take 
>>>> ownership of the buffer after completion of hardwate job. But in 
>>>> fact dma_sync_single_for_cpu(DMA_TO_DEVICE) is a NO-OP because CPU 
>>>> doesn't need to flush or invalidate anything to take ownership of 
>>>> the buffer if hardware did a read-only access.
>>>>>   
>>>>>> +	if (!i2c_dev->msg_read) {
>>>>>> +		if (dma) {
>>>>>> +			memcpy(buffer, msg->buf, msg->len);
>>>>>> +
>>>>>> dma_sync_single_for_device(i2c_dev->dev, + i2c_dev->dma_phys,
>>>>>> +						   xfer_size,
>>>>>> +
>>>>>> DMA_TO_DEVICE);
>>>>>
>>>>> Again, here we properly flush the caches to make sure the data 
>>>>> that we've written to the DMA buffer is visible to the DMA engine.
>>>>>   
>>>>
>>>> +1 this is correct
>>>>
>>>>
>>>>  
>>>>>> +
>>>>>> +		if (i2c_dev->msg_read) {
>>>>>> +			if (likely(i2c_dev->msg_err ==
>>>>>> I2C_ERR_NONE)) {
>>>>>> +
>>>>>> dma_sync_single_for_cpu(i2c_dev->dev,
>>>>>> +
>>>>>> i2c_dev->dma_phys,
>>>>>> +
>>>>>> xfer_size, +
>>>>>> DMA_FROM_DEVICE);
>>>>>
>>>>> Here we invalidate the caches to make sure we don't get stale data 
>>>>> that may be in the caches for data that we're copying out of the 
>>>>> DMA buffer. I think that's about all the cache maintenance that we 
>>>>> real need.
>>>>
>>>> Correct.
>>>>
>>>> And technically here should be
>>>> dma_sync_single_for_cpu(DMA_TO_DEVICE) for the TX. But again, it's a 
>>>> NO-OP.
>>>
>>> Is my below understanding correct? Can you please confirm?
>>>
>>> During Transmit to device:
>>> - Before writing msg data into dma buf by CPU, giving DMA ownership to 
>>> CPU dma_sync_single_for_cpu with dir DMA_TO_DEVICE
>>>
>>
>> I tried to take a look at it again and now thinking that your variant is more correct. Still it's a bit difficult to judge because this case is no-op.
>>
>>> - After writing to dma buf by CPU, giving back the ownership to device 
>>> to access buffer to send during DMA transmit 
>>> dma_sync_single_for_device with dir DMA_TO_DEVICE
>>
>> Correct.
>>
>>> During Receiving from Device:
>>> - before submitting RX DMA to give buffer access to DMAengine
>>> 	dma_sync_single_for_Device(DMA_FROM_DEVICE)
>>
>> Correct.
>>
>>> - after DMA RX completion, giving dma ownership to CPU for reading 
>>> dmabuf data written by DMA from device dma_sync_single_for_cpu with 
>>> dir DMA_FROM_DEVICE
>>>
>>
>> Correct.
> 
> Then what I have is exact as mentioned above. So no changes needed related to dma_sync

Yes

> Pasting again here with clear comment to explain on why I have those corresponding dma_sync

If you're going to add these comments to the actual patch, then please add brief explanation of what actually syncing is doing.

> 
>         if (i2c_dev->msg_read) {
>             tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
>                            DATA_DMA_DIR_RX);
>             /* For Reads: giving dma buf ownership to device before submitting RX DMA */

This invalidates buffer in CPU caches to avoid buffer-data eviction from the caches to DRAM while hardware writes to the buffer.

>             dma_sync_single_for_device(i2c_dev->dev,
>                            i2c_dev->dma_phys,
>                            xfer_size,
>                            DMA_FROM_DEVICE);
>             err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
>             if (err < 0) {
>                 dev_err(i2c_dev->dev,
>                     "starting RX DMA failed, err %d\n",
>                     err);
>                 goto unlock;
>             }
>         } else {
>             tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
>                            DATA_DMA_DIR_TX);
>             /* For writes: giving dma buf ownership to CPU to copy transmit data to DMA Buf */

This is a NO-OP because there is no need to flush nor to invalidate CPU caches.

>             dma_sync_single_for_cpu(i2c_dev->dev,
>                         i2c_dev->dma_phys,
>                         xfer_size,
>                         DMA_TO_DEVICE);
>             buffer = i2c_dev->dma_buf;
>         }
> 
> 
> 
>     if (!msg->flags & I2C_M_RD) {
>         if (dma) {
>             memcpy(buffer, msg->buf, msg->len);
>             /* For writes: giving ownership to device after done with copying data to DMA Buf */

This flushes out buffer from CPU caches to DRAM.

>             dma_sync_single_for_device(i2c_dev->dev,
>                            i2c_dev->dma_phys,
>                            xfer_size,
>                            DMA_TO_DEVICE);
>             err = tegra_i2c_dma_submit(i2c_dev, xfer_size);
>             if (err < 0) {
>                 dev_err(i2c_dev->dev,
>                     "starting TX DMA failed, err %d\n",
>                     err);
>                 goto unlock;
>             }
>         } else {
>             tegra_i2c_fill_tx_fifo(i2c_dev);
>         }
> 
>         if (i2c_dev->msg_read) {
>             if (likely(i2c_dev->msg_err == I2C_ERR_NONE)) {
>             /* For Reads: giving ownership to CPU after RX DMA completion to access read data */
>                 dma_sync_single_for_cpu(i2c_dev->dev,
>                             i2c_dev->dma_phys,
>                             xfer_size,
>                             DMA_FROM_DEVICE);

This invalidates buffer in CPU caches again, even though it shall be kept invalidated after dma_sync_single_for_device(DMA_FROM_DEVICE). So it's likely to be a mostly NO-OP in hardware. 

> 
>                 memcpy(i2c_dev->msg_buf, i2c_dev->dma_buf,
>                     msg->len);
>             }
>         }
> 
> 
> 

Technically we could remove the NO-OP's, but I personally would prefer to keep them for consistency with the DMA API.

^ permalink raw reply	[flat|nested] 32+ messages in thread

end of thread, other threads:[~2019-02-01 15:02 UTC | newest]

Thread overview: 32+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-31  6:16 [PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically Sowjanya Komatineni
2019-01-31  6:16 ` Sowjanya Komatineni
2019-01-31  6:16 ` [PATCH V8 2/5] i2c: tegra: Add Bus Clear Master Support Sowjanya Komatineni
2019-01-31  6:16   ` Sowjanya Komatineni
2019-01-31 12:44   ` Thierry Reding
2019-01-31 15:16   ` Dmitry Osipenko
2019-01-31  6:16 ` [PATCH V8 3/5] i2c: tegra: Add DMA Support Sowjanya Komatineni
2019-01-31  6:16   ` Sowjanya Komatineni
2019-01-31 12:44   ` Thierry Reding
2019-01-31 16:41     ` Sowjanya Komatineni
2019-02-01  0:52     ` Dmitry Osipenko
2019-02-01  1:11       ` Sowjanya Komatineni
2019-02-01  3:14         ` Dmitry Osipenko
2019-02-01  4:19           ` Sowjanya Komatineni
2019-02-01 15:02             ` Dmitry Osipenko
2019-01-31 15:12   ` Dmitry Osipenko
2019-01-31 15:24     ` Dmitry Osipenko
2019-01-31 16:56       ` Sowjanya Komatineni
2019-01-31 17:11         ` Dmitry Osipenko
2019-01-31 17:25           ` Dmitry Osipenko
2019-01-31 17:38             ` Dmitry Osipenko
2019-01-31 17:38             ` Sowjanya Komatineni
2019-01-31 21:46             ` Sowjanya Komatineni
2019-02-01  0:54               ` Dmitry Osipenko
2019-01-31  6:16 ` [PATCH V8 4/5] i2c: tegra: Update transfer timeout Sowjanya Komatineni
2019-01-31  6:16   ` Sowjanya Komatineni
2019-01-31 12:48   ` Thierry Reding
2019-01-31  6:16 ` [PATCH V8 5/5] i2c: tegra: Add I2C interface timing support Sowjanya Komatineni
2019-01-31  6:16   ` Sowjanya Komatineni
2019-01-31 12:50   ` Thierry Reding
2019-01-31 12:45 ` [PATCH V8 1/5] i2c: tegra: Sort all the include headers alphabetically Thierry Reding
2019-01-31 15:14 ` Dmitry Osipenko

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.