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

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

[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] : 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

[PATCH V7 2/5] i2c: tegra: Add Bus Clear Master Support

[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]: 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

Re: [PATCH V7 2/5] i2c: tegra: Add Bus Clear Master Support

[Thierry Reding]

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

On Wed, Jan 30, 2019 at 08:01:33AM -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]: 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(+)

Almost perfect, just a minor nit:

Please be more consistent with the spelling. You've got all of "Tegra",
"tegra", "I2C" and "i2c" in the commit message. "Tegra" and "I2C" are
the correct ones.

And to clarify, it's fine to keep the lower-case spellings in the
subject because that serves as a sort of tag. In prose, always use the
correct spelling.

With that fixed:

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

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

[PATCH V7 3/5] i2c: tegra: Add DMA Support

[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>
---
 [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/i2c-tegra.c | 370 ++++++++++++++++++++++++++++++++++++++---
 1 file changed, 345 insertions(+), 25 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index c4892a47a483..b30b5da5ce6b 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
+ * @phys_addr: Physical address of I2C base address to use for DMA configuration
  * @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 phys_addr;
 	int cont_id;
 	int irq;
 	bool irq_disabled;
@@ -223,8 +251,18 @@ 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 struct dma_chan *chan;
+
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
 		       unsigned long reg)
 {
@@ -291,6 +329,82 @@ 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;
+
+	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;
+	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,
+				    bool dma_to_memory)
+{
+	struct dma_chan *dma_chan;
+	u32 *dma_buf;
+	dma_addr_t dma_phys;
+	int ret;
+	const char *chan_name = dma_to_memory ? "rx" : "tx";
+
+	if (!i2c_dev->has_dma)
+		return -EINVAL;
+
+	dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, chan_name);
+	if (IS_ERR(dma_chan)) {
+		dev_err(i2c_dev->dev, "DMA Channel not available\n");
+		return PTR_ERR(dma_chan);
+	}
+
+	if (!i2c_dev->dma_buf) {
+		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");
+			ret = -ENOMEM;
+			goto scrub;
+		}
+
+		i2c_dev->dma_buf = dma_buf;
+		i2c_dev->dma_phys = dma_phys;
+	}
+
+	if (dma_to_memory)
+		i2c_dev->rx_dma_chan = dma_chan;
+	else
+		i2c_dev->tx_dma_chan = dma_chan;
+
+	return 0;
+
+scrub:
+	dma_release_channel(dma_chan);
+	return ret;
+}
+
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
 {
 	unsigned long timeout = jiffies + HZ;
@@ -656,25 +770,45 @@ 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);
 
-	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
+	if (status & I2C_INT_ALL_PACKETS_XFER_COMPLETE) {
+	/*
+	 * During message read XFER_COMPLETE interrupt is triggered prior to
+	 * DMA complete notification and during message write XFER_COMPLETE
+	 * interrupt is triggered after DMA complete notification.
+	 * PACKETS_XFER_COMPLETE indicates completion of all bytes of transfer.
+	 * so forcing msg_buf_remaining to 0.
+	 */
+		if (i2c_dev->is_curr_dma_xfer)
+			i2c_dev->msg_buf_remaining = 0;
+		status |= I2C_INT_PACKET_XFER_COMPLETE;
+		i2c_writel(i2c_dev, status, I2C_INT_STATUS);
+		if (!i2c_dev->msg_buf_remaining)
+			complete(&i2c_dev->msg_complete);
+	} else 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);
 	}
@@ -683,19 +817,70 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
 	/* An error occurred, mask all interrupts */
 	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);
+		I2C_INT_RX_FIFO_DATA_REQ | I2C_INT_ALL_PACKETS_XFER_COMPLETE);
 	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);
 
+	if (i2c_dev->is_curr_dma_xfer) {
+		dmaengine_terminate_all(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;
+
+	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->phys_addr + 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->phys_addr + I2C_RX_FIFO;
+		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		dma_sconfig.src_maxburst = dma_burst;
+	}
+
+	dmaengine_slave_config(chan, &dma_sconfig);
+}
+
 static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
 {
 	int err;
@@ -740,6 +925,32 @@ 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 ret = 0;
+	bool dma = false;
+
+	if (msg->flags & I2C_M_RD)
+		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) {
+		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
+			ret = tegra_i2c_init_dma_param(i2c_dev, true);
+		else if (!i2c_dev->tx_dma_chan)
+			ret = tegra_i2c_init_dma_param(i2c_dev, false);
+
+		if (ret < 0) {
+			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
+			dma = false;
+			ret = 0;
+		}
+	}
+
+	i2c_dev->is_curr_dma_xfer = dma;
 
 	tegra_i2c_flush_fifos(i2c_dev);
 
@@ -754,14 +965,50 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	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);
+			/* make the dma buffer to read by dma */
+			dma_sync_single_for_device(i2c_dev->dev,
+						   i2c_dev->dma_phys,
+						   xfer_size,
+						   DMA_FROM_DEVICE);
+			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
+			if (ret < 0) {
+				dev_err(i2c_dev->dev,
+					"Starting RX DMA failed, err %d\n",
+					ret);
+				goto exit;
+			}
+		} else {
+			chan = i2c_dev->tx_dma_chan;
+			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
+						   DATA_DMA_DIR_TX);
+			/* Make the dma buffer to read by cpu */
+			dma_sync_single_for_cpu(i2c_dev->dev,
+						i2c_dev->dma_phys,
+						xfer_size,
+						DMA_TO_DEVICE);
+			buffer = (u32 *)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,29 +1025,85 @@ 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);
+			/* make the dma buffer to read by dma */
+			dma_sync_single_for_device(i2c_dev->dev,
+						   i2c_dev->dma_phys,
+						   xfer_size,
+						   DMA_TO_DEVICE);
+			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
+			if (ret < 0) {
+				dev_err(i2c_dev->dev,
+					"Starting TX DMA failed, err %d\n",
+					ret);
+				goto exit;
+			}
+		} 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) {
+		int_mask |= I2C_INT_ALL_PACKETS_XFER_COMPLETE;
+	} else {
+		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));
 
+exit:
+	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
+	if (ret)
+		return ret;
+
+	if (dma) {
+		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");
+		if (dma) {
+			dmaengine_terminate_all(chan);
+			complete(&i2c_dev->dma_complete);
+		}
 
 		tegra_i2c_init(i2c_dev);
 		return -ETIMEDOUT;
@@ -884,6 +1187,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 +1307,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	struct clk *div_clk;
 	struct clk *fast_clk;
 	void __iomem *base;
+	phys_addr_t phys_addr;
 	int irq;
 	int ret = 0;
 	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	phys_addr = res->start;
 	base = devm_ioremap_resource(&pdev->dev, res);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -1029,6 +1336,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	i2c_dev->base = base;
+	i2c_dev->phys_addr = phys_addr;
 	i2c_dev->div_clk = div_clk;
 	i2c_dev->adapter.algo = &tegra_i2c_algo;
 	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
@@ -1036,6 +1344,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 +1358,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) {
@@ -1173,6 +1483,16 @@ 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);
+
+	if (i2c_dev->rx_dma_chan)
+		dma_release_channel(i2c_dev->tx_dma_chan);
+
 	return 0;
 }
 
-- 
2.7.4

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

30.01.2019 19:01, 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>
> ---
>  [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/i2c-tegra.c | 370 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 345 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index c4892a47a483..b30b5da5ce6b 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
> + * @phys_addr: Physical address of I2C base address to use for DMA configuration
>   * @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 phys_addr;

What about to rename "phys_addr" to "base_phys"?

>  	int cont_id;
>  	int irq;
>  	bool irq_disabled;
> @@ -223,8 +251,18 @@ 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 struct dma_chan *chan;

This driver is shared among multiple I2C device instances, hence they all share the global variable. Please don't do it, you may assume that global variables are prohibited for this driver. It should be fine to move out "*chan" into "struct tegra_i2c_dev".

> +
>  static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>  		       unsigned long reg)
>  {
> @@ -291,6 +329,82 @@ 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;
> +
> +	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;
> +	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");

Most of messages in the driver start with a lowercase letter, let's try to keep them more or less consistent.

> +		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,
> +				    bool dma_to_memory)
> +{
> +	struct dma_chan *dma_chan;
> +	u32 *dma_buf;
> +	dma_addr_t dma_phys;
> +	int ret;
> +	const char *chan_name = dma_to_memory ? "rx" : "tx";

What about to move out chan_name into the function arguments?

> +
> +	if (!i2c_dev->has_dma)
> +		return -EINVAL;
> +
> +	dma_chan = dma_request_slave_channel_reason(i2c_dev->dev, chan_name);
> +	if (IS_ERR(dma_chan)) {
> +		dev_err(i2c_dev->dev, "DMA Channel not available\n");
> +		return PTR_ERR(dma_chan);
> +	}
> +
> +	if (!i2c_dev->dma_buf) {
> +		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");
> +			ret = -ENOMEM;
> +			goto scrub;
> +		}
> +
> +		i2c_dev->dma_buf = dma_buf;
> +		i2c_dev->dma_phys = dma_phys;
> +	}
> +
> +	if (dma_to_memory)
> +		i2c_dev->rx_dma_chan = dma_chan;
> +	else
> +		i2c_dev->tx_dma_chan = dma_chan;
> +
> +	return 0;
> +
> +scrub:
> +	dma_release_channel(dma_chan);
> +	return ret;
> +}
> +
>  static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
>  {
>  	unsigned long timeout = jiffies + HZ;
> @@ -656,25 +770,45 @@ 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);
>  
> -	if (status & I2C_INT_PACKET_XFER_COMPLETE) {
> +	if (status & I2C_INT_ALL_PACKETS_XFER_COMPLETE) {

I2C_INT_ALL_PACKETS_XFER_COMPLETE shall be masked for the PIO mode.

> +	/*
> +	 * During message read XFER_COMPLETE interrupt is triggered prior to
> +	 * DMA complete notification and during message write XFER_COMPLETE
> +	 * interrupt is triggered after DMA complete notification.
> +	 * PACKETS_XFER_COMPLETE indicates completion of all bytes of transfer.
> +	 * so forcing msg_buf_remaining to 0.
> +	 */
> +		if (i2c_dev->is_curr_dma_xfer)
> +			i2c_dev->msg_buf_remaining = 0;
> +		status |= I2C_INT_PACKET_XFER_COMPLETE;
> +		i2c_writel(i2c_dev, status, I2C_INT_STATUS);
> +		if (!i2c_dev->msg_buf_remaining)
> +			complete(&i2c_dev->msg_complete);
> +	} else if (status & I2C_INT_PACKET_XFER_COMPLETE) {

I2C_INT_PACKET_XFER_COMPLETE shall be masked for the DMA mode, if I'm not missing something.

> +		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);
>  	}
> @@ -683,19 +817,70 @@ static irqreturn_t tegra_i2c_isr(int irq, void *dev_id)
>  	/* An error occurred, mask all interrupts */
>  	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);
> +		I2C_INT_RX_FIFO_DATA_REQ | I2C_INT_ALL_PACKETS_XFER_COMPLETE);
>  	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);
>  
> +	if (i2c_dev->is_curr_dma_xfer) {
> +		dmaengine_terminate_all(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;
> +
> +	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->phys_addr + 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->phys_addr + I2C_RX_FIFO;
> +		dma_sconfig.src_addr_width = DMA_SLAVE_BUSWIDTH_4_BYTES;
> +		dma_sconfig.src_maxburst = dma_burst;
> +	}
> +
> +	dmaengine_slave_config(chan, &dma_sconfig);
> +}
> +
>  static int tegra_i2c_issue_bus_clear(struct tegra_i2c_dev *i2c_dev)
>  {
>  	int err;
> @@ -740,6 +925,32 @@ 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 ret = 0;
> +	bool dma = false;
> +
> +	if (msg->flags & I2C_M_RD)
> +		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) {
> +		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
> +			ret = tegra_i2c_init_dma_param(i2c_dev, true);
> +		else if (!i2c_dev->tx_dma_chan)
> +			ret = tegra_i2c_init_dma_param(i2c_dev, false);

In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.

This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.

> +		if (ret < 0) {
> +			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
> +			dma = false;
> +			ret = 0;
> +		}
> +	}
> +
> +	i2c_dev->is_curr_dma_xfer = dma;
>  
>  	tegra_i2c_flush_fifos(i2c_dev);
>  
> @@ -754,14 +965,50 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	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);
> +			/* make the dma buffer to read by dma */

I think it should be better and more correct to rephrase like this: "prepare dma buffer to be written by device".

I'd also recommend to just drop comments for dma_sync_single_*() because the function naming is quite self-explanatory.

> +			dma_sync_single_for_device(i2c_dev->dev,
> +						   i2c_dev->dma_phys,
> +						   xfer_size,
> +						   DMA_FROM_DEVICE);
> +			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> +			if (ret < 0) {
> +				dev_err(i2c_dev->dev,
> +					"Starting RX DMA failed, err %d\n",
> +					ret);
> +				goto exit;
> +			}
> +		} else {
> +			chan = i2c_dev->tx_dma_chan;
> +			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
> +						   DATA_DMA_DIR_TX);
> +			/* Make the dma buffer to read by cpu */> +			dma_sync_single_for_cpu(i2c_dev->dev,
> +						i2c_dev->dma_phys,
> +						xfer_size,
> +						DMA_TO_DEVICE);

This is not correct, you need to call dma_sync_single_for_cpu() after completion of the transfer to give back dma buffer ownership to CPU, see below. This dma_sync_single_for_cpu() invocation should be removed.

> +			buffer = (u32 *)i2c_dev->dma_buf;

The type of i2c_dev->dma_buf is u32, there is no need to cast it.

> +		}
> +	}
> +
>  	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,29 +1025,85 @@ 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);
> +			/* make the dma buffer to read by dma */
> +			dma_sync_single_for_device(i2c_dev->dev,
> +						   i2c_dev->dma_phys,
> +						   xfer_size,
> +						   DMA_TO_DEVICE);
> +			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> +			if (ret < 0) {
> +				dev_err(i2c_dev->dev,
> +					"Starting TX DMA failed, err %d\n",
> +					ret);
> +				goto exit;
> +			}
> +		} 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) {
> +		int_mask |= I2C_INT_ALL_PACKETS_XFER_COMPLETE;
> +	} else {
> +		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));
>  
> +exit:

Minor nit: usually such labels are named as "unlock:"

> +	spin_unlock_irqrestore(&i2c_dev->xfer_lock, flags);
> +	if (ret)
> +		return ret;
> +
> +	if (dma) {
> +		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;
> +		}

Handle the error-case here:

	if (i2c_dev->msg_err != I2C_ERR_NONE) {
		dev_err(i2c_dev->dev, "i2c dma transfer failed\n");
		goto i2c_recover;
	}

...
	<--- end of tegra_i2c_xfer_msg() -->

	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
		return 0;

i2c_recover:
	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;
		return -EREMOTEIO;
	}

	return -EIO;
}


> +
> +		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);
> +			}
> +		}

Here you should give back dma buffer ownership to CPU:

		else {
			dma_sync_single_for_cpu(i2c_dev->dev,
						i2c_dev->dma_phys,
						xfer_size,
						DMA_TO_DEVICE);
		}

> +	}
> +
>  	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");
> +		if (dma) {
> +			dmaengine_terminate_all(chan);
> +			complete(&i2c_dev->dma_complete);
> +		}

DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.

>  
>  		tegra_i2c_init(i2c_dev);
>  		return -ETIMEDOUT;
> @@ -884,6 +1187,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");

Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.

Hence there is a need to check for DMA driver presence in the code:

	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
		i2c_dev->has_dma = of_property_read_bool(np, "dmas");

Also Tegra I2C driver should select DMA driver in Kconfig to make DMA driver built-in when I2C driver is built-in:

config I2C_TEGRA
	tristate "NVIDIA Tegra internal I2C controller"
	depends on ARCH_TEGRA
	select TEGRA20_APB_DMA
	help
	  If you say yes to this option, support will be included for the
	  I2C controller embedded in NVIDIA Tegra SOCs


And apparently there is no APB DMA driver for T210+ in upstream yet.

>  }
>  
>  static const struct i2c_algorithm tegra_i2c_algo = {
> @@ -1002,11 +1307,13 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  	struct clk *div_clk;
>  	struct clk *fast_clk;
>  	void __iomem *base;
> +	phys_addr_t phys_addr;
>  	int irq;
>  	int ret = 0;
>  	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	phys_addr = res->start;
>  	base = devm_ioremap_resource(&pdev->dev, res);
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
> @@ -1029,6 +1336,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	i2c_dev->base = base;
> +	i2c_dev->phys_addr = phys_addr;
>  	i2c_dev->div_clk = div_clk;
>  	i2c_dev->adapter.algo = &tegra_i2c_algo;
>  	i2c_dev->adapter.quirks = &tegra_i2c_quirks;
> @@ -1036,6 +1344,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 +1358,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) {
> @@ -1173,6 +1483,16 @@ 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);
> +
> +	if (i2c_dev->rx_dma_chan)
> +		dma_release_channel(i2c_dev->tx_dma_chan);

	dma_release_channel(i2c_dev->rx_dma_chan);

> +
>  	return 0;
>  }
>  
> 

RE: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Sowjanya Komatineni]

> > +			dma_sync_single_for_device(i2c_dev->dev,
> > +						   i2c_dev->dma_phys,
> > +						   xfer_size,
> > +						   DMA_FROM_DEVICE);
> > +			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
> > +			if (ret < 0) {
> > +				dev_err(i2c_dev->dev,
> > +					"Starting RX DMA failed, err %d\n",
> > +					ret);
> > +				goto exit;
> > +			}
> > +		} else {
> > +			chan = i2c_dev->tx_dma_chan;
> > +			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
> > +						   DATA_DMA_DIR_TX);
> > +			/* Make the dma buffer to read by cpu */> +			dma_sync_single_for_cpu(i2c_dev->dev,
> > +						i2c_dev->dma_phys,
> > +						xfer_size,
> > +						DMA_TO_DEVICE);
>
> This is not correct, you need to call dma_sync_single_for_cpu() after completion of the transfer to give back dma buffer ownership to CPU, see below. This dma_sync_single_for_cpu() invocation should be removed.

Transfer is not done yet. Dma buffer ownership is given to CPU here before copying header bytes and msg bytes into dma buffer before actually transmitting

> > +
> > +		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);
> > +			}
> > +		}
>
> Here you should give back dma buffer ownership to CPU:

After transfer is done, for msg reads, dma buffer ownership is given to CPU and read data from dma buffer is copied to msg buffer
For msg writes, no need of ownership to CPU.
> >
> >		else {
> >			dma_sync_single_for_cpu(i2c_dev->dev,
> >						i2c_dev->dma_phys,
> >						xfer_size,
> >						DMA_TO_DEVICE);
> >		}
>
> >  	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");
> > +		if (dma) {
> > +			dmaengine_terminate_all(chan);
> > +			complete(&i2c_dev->dma_complete);
> > +		}
>
> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.

DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
So still need to check for msg_complete timeout. 

> > @@ -740,6 +925,32 @@ 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 ret = 0;
> > +	bool dma = false;
> > +
> > +	if (msg->flags & I2C_M_RD)
> > +		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) {
> > +		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
> > +			ret = tegra_i2c_init_dma_param(i2c_dev, true);
> > +		else if (!i2c_dev->tx_dma_chan)
> > +			ret = tegra_i2c_init_dma_param(i2c_dev, false);
>
> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
>
> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
>
> > +		if (ret < 0) {
> > +			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
> > +			dma = false;
> > +			ret = 0;
> > +		}
> > +	}
> > +
> > +	i2c_dev->is_curr_dma_xfer = dma;
>
>
Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 4:19, Sowjanya Komatineni пишет:
>>> +			dma_sync_single_for_device(i2c_dev->dev,
>>> +						   i2c_dev->dma_phys,
>>> +						   xfer_size,
>>> +						   DMA_FROM_DEVICE);
>>> +			ret = tegra_i2c_dma_submit(i2c_dev, xfer_size);
>>> +			if (ret < 0) {
>>> +				dev_err(i2c_dev->dev,
>>> +					"Starting RX DMA failed, err %d\n",
>>> +					ret);
>>> +				goto exit;
>>> +			}
>>> +		} else {
>>> +			chan = i2c_dev->tx_dma_chan;
>>> +			tegra_i2c_config_fifo_trig(i2c_dev, xfer_size,
>>> +						   DATA_DMA_DIR_TX);
>>> +			/* Make the dma buffer to read by cpu */> +			dma_sync_single_for_cpu(i2c_dev->dev,
>>> +						i2c_dev->dma_phys,
>>> +						xfer_size,
>>> +						DMA_TO_DEVICE);
>>
>> This is not correct, you need to call dma_sync_single_for_cpu() after completion of the transfer to give back dma buffer ownership to CPU, see below. This dma_sync_single_for_cpu() invocation should be removed.
> 
> Transfer is not done yet. Dma buffer ownership is given to CPU here before copying header bytes and msg bytes into dma buffer before actually transmitting
> 

In fact dma_sync_single_for_cpu(DMA_TO_DEVICE) is not needed at all because it is a NO-OP on ARM, but technically it is needed to conform to DMA API. I think it is more logical that device owns dma buffer only during of the actual DMA transfer, the rest of the time CPU should be owner of the buffer.

>>> +
>>> +		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);
>>> +			}
>>> +		}
>>
>> Here you should give back dma buffer ownership to CPU:
> 
> After transfer is done, for msg reads, dma buffer ownership is given to CPU and read data from dma buffer is copied to msg buffer
> For msg writes, no need of ownership to CPU.
>>>
>>> 		else {
>>> 			dma_sync_single_for_cpu(i2c_dev->dev,
>>> 						i2c_dev->dma_phys,
>>> 						xfer_size,
>>> 						DMA_TO_DEVICE);
>>> 		}
>>
>>>  	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");
>>> +		if (dma) {
>>> +			dmaengine_terminate_all(chan);
>>> +			complete(&i2c_dev->dma_complete);
>>> +		}
>>
>> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.
> 
> DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
> So still need to check for msg_complete timeout. 

You're waiting for DMA completion and then for the I2C message completion.

Hence your code is structured like this:

1. Issue DMA transfer
2. Wait for DMA completion
3. Wait for message completion

Why do you need to abort DMA in 3 if it was already completed in 2?

>>> @@ -740,6 +925,32 @@ 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 ret = 0;
>>> +	bool dma = false;
>>> +
>>> +	if (msg->flags & I2C_M_RD)
>>> +		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) {
>>> +		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
>>> +			ret = tegra_i2c_init_dma_param(i2c_dev, true);
>>> +		else if (!i2c_dev->tx_dma_chan)
>>> +			ret = tegra_i2c_init_dma_param(i2c_dev, false);
>>
>> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
>>
>> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
>>
>>> +		if (ret < 0) {
>>> +			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
>>> +			dma = false;
>>> +			ret = 0;
>>> +		}
>>> +	}
>>> +
>>> +	i2c_dev->is_curr_dma_xfer = dma;
>>
>>
> Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
> So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.
> 
> 

Yes, I made that comment, but then corrected myself. Seems you missed the correction: https://lkml.org/lkml/2019/1/26/217

If you're having troubles with the corporate email, maybe you could try to switch to something else like gmail.

I've tried to apply this series locally, but again it fails to apply. What's the kernel base you're using? You should make your patches on top linux-next (preferably) or mainline.

RE: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Sowjanya Komatineni]

> >>>  	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");
> >>> +		if (dma) {
> >>> +			dmaengine_terminate_all(chan);
> >>> +			complete(&i2c_dev->dma_complete);
> >>> +		}
> >>
> >> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.
> > 
> > DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
> > So still need to check for msg_complete timeout. 
>
> You're waiting for DMA completion and then for the I2C message completion.
>
> Hence your code is structured like this:
>
> 1. Issue DMA transfer
> 2. Wait for DMA completion
> 3. Wait for message completion
>
> Why do you need to abort DMA in 3 if it was already completed in 2?

Ok, thought you are referring to msg complete timeout check in dma mode. Yes no need for terminating DMA when msg timeout. Will fix it.

> >>> @@ -740,6 +925,32 @@ 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 ret = 0;
> >>> +	bool dma = false;
> >>> +
> >>> +	if (msg->flags & I2C_M_RD)
> >>> +		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) {
> >>> +		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
> >>> +			ret = tegra_i2c_init_dma_param(i2c_dev, true);
> >>> +		else if (!i2c_dev->tx_dma_chan)
> >>> +			ret = tegra_i2c_init_dma_param(i2c_dev, false);
> >>
> >> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
> >>
> >> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
> >>
> >>> +		if (ret < 0) {
> >>> +			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
> >>> +			dma = false;
> >>> +			ret = 0;
> >>> +		}
> >>> +	}
> >>> +
> >>> +	i2c_dev->is_curr_dma_xfer = dma;
> >>
> >>
> > Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
> > So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.
> > 
> > 
>
> Yes, I made that comment, but then corrected myself. Seems you missed the correction: https://lkml.org/lkml/2019/1/26/217
>
> If you're having troubles with the corporate email, maybe you could try to switch to something else like gmail.
>
> I've tried to apply this series locally, but again it fails to apply. What's the kernel base you're using? You should make your patches on top linux-next (preferably) or mainline.

Yeah somehow missed that. Will move it back to probe..
I am using 5.0-rc1

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 5:24, Sowjanya Komatineni пишет:
>>>>>  	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");
>>>>> +		if (dma) {
>>>>> +			dmaengine_terminate_all(chan);
>>>>> +			complete(&i2c_dev->dma_complete);
>>>>> +		}
>>>>
>>>> DMA transfer has been completed at this point, hence this hunk isn't needed. Please remove it.
>>>
>>> DMA complete alone doesn’t guarantee the transfer. Packets/All packets xfer interrupt from I2C confirms complete transaction along with dma complete check.
>>> So still need to check for msg_complete timeout. 
>>
>> You're waiting for DMA completion and then for the I2C message completion.
>>
>> Hence your code is structured like this:
>>
>> 1. Issue DMA transfer
>> 2. Wait for DMA completion
>> 3. Wait for message completion
>>
>> Why do you need to abort DMA in 3 if it was already completed in 2?
> 
> Ok, thought you are referring to msg complete timeout check in dma mode. Yes no need for terminating DMA when msg timeout. Will fix it.
> 
>>>>> @@ -740,6 +925,32 @@ 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 ret = 0;
>>>>> +	bool dma = false;
>>>>> +
>>>>> +	if (msg->flags & I2C_M_RD)
>>>>> +		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) {
>>>>> +		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
>>>>> +			ret = tegra_i2c_init_dma_param(i2c_dev, true);
>>>>> +		else if (!i2c_dev->tx_dma_chan)
>>>>> +			ret = tegra_i2c_init_dma_param(i2c_dev, false);
>>>>
>>>> In the comment to V3 I mentioned that it's not a good idea to request channels dynamically because suspend-resume order is based on devices registration order, in this case APB DMA must be probed before I2C. Please move channels allocation into the probe.
>>>>
>>>> This also raises the question about the need to register I2C driver from the subsys-init level because APB driver is getting registered from the module-init level and hence I2C probing will be deferred until APB DMA driver is registered. It looks to me that the subsys-init is a relict of the past and it should be fine to move I2C driver registration into the module-init level, of course it's not strictly necessary and could be done later on if desired.
>>>>
>>>>> +		if (ret < 0) {
>>>>> +			dev_dbg(i2c_dev->dev, "Switching to PIO mode\n");
>>>>> +			dma = false;
>>>>> +			ret = 0;
>>>>> +		}
>>>>> +	}
>>>>> +
>>>>> +	i2c_dev->is_curr_dma_xfer = dma;
>>>>
>>>>
>>> Since your previous feedback suggest "let's postpone channels requesting and dma_buf allocation until they are really needed", I thought it make sense to not request channels and allocate till DMA is needed.
>>> So moved from probe to xfer_msg function. By the time it gets to xfer msg function, devices registration should be done already along with apb dma probe.
>>>
>>>
>>
>> Yes, I made that comment, but then corrected myself. Seems you missed the correction: https://lkml.org/lkml/2019/1/26/217
>>
>> If you're having troubles with the corporate email, maybe you could try to switch to something else like gmail.
>>
>> I've tried to apply this series locally, but again it fails to apply. What's the kernel base you're using? You should make your patches on top linux-next (preferably) or mainline.
> 
> Yeah somehow missed that. Will move it back to probe..
> I am using 5.0-rc1
> 

Managed to apply patches this time, was my bad. Please take a look and reply to the rest of my comments before sending a new version, thanks.

RE: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Sowjanya Komatineni]

> I2C_INT_ALL_PACKETS_XFER_COMPLETE shall be masked for the PIO mode
> I2C_INT_PACKET_XFER_COMPLETE shall be masked for the DMA mode, if I'm not missing something.
>
for packets with repeated start or continue xfer, PACKET_XFER_COMPLETE will be triggers
for packets with stop bit, both PACKET_XFER_COMPLETE and ALL_PACKET_XFER_COMPLETE will be triggered.
To be align with what PIO mode is doing, will not use ALL_PACKETS_XFER_COMPLETE for DMA in next version. 

>
> Handle the error-case here:
>
>	if (i2c_dev->msg_err != I2C_ERR_NONE) {
>		dev_err(i2c_dev->dev, "i2c dma transfer failed\n");
>		goto i2c_recover;
>	}
>
>...
>	<--- end of tegra_i2c_xfer_msg() -->
>
>	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
>		return 0;
>
> i2c_recover:
>	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;
>		return -EREMOTEIO;
>	}
>
>	return -EIO;

When DMA transfer timeout, no need to check for ARB LOST or NO_ACK as if any of those interrupts trigger interrupt handler will terminate dmaengine and completes so DMA timeout will not be seen in those cases and falls thru msg_err check

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

В Thu, 31 Jan 2019 03:34:59 +0000
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> > I2C_INT_ALL_PACKETS_XFER_COMPLETE shall be masked for the PIO mode
> > I2C_INT_PACKET_XFER_COMPLETE shall be masked for the DMA mode, if
> > I'm not missing something. 
> for packets with repeated start or continue xfer,
> PACKET_XFER_COMPLETE will be triggers for packets with stop bit, both
> PACKET_XFER_COMPLETE and ALL_PACKET_XFER_COMPLETE will be triggered.
> To be align with what PIO mode is doing, will not use
> ALL_PACKETS_XFER_COMPLETE for DMA in next version. 

Sounds good.

> >
> > Handle the error-case here:
> >
> >	if (i2c_dev->msg_err != I2C_ERR_NONE) {
> >		dev_err(i2c_dev->dev, "i2c dma transfer failed\n");
> >		goto i2c_recover;
> >	}
> >
> >...
> >	<--- end of tegra_i2c_xfer_msg() -->
> >
> >	if (likely(i2c_dev->msg_err == I2C_ERR_NONE))
> >		return 0;
> >
> > i2c_recover:
> >	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;
> >		return -EREMOTEIO;
> >	}
> >
> >	return -EIO;  
> 
> When DMA transfer timeout, no need to check for ARB LOST or NO_ACK as
> if any of those interrupts trigger interrupt handler will terminate
> dmaengine and completes so DMA timeout will not be seen in those
> cases and falls thru msg_err check
> 

Okay.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Thierry Reding]

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

On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
> 30.01.2019 19:01, Sowjanya Komatineni пишет:
[...]
> > diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
[...]
> > +		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,
> > +				    bool dma_to_memory)
> > +{
> > +	struct dma_chan *dma_chan;
> > +	u32 *dma_buf;
> > +	dma_addr_t dma_phys;
> > +	int ret;
> > +	const char *chan_name = dma_to_memory ? "rx" : "tx";
> 
> What about to move out chan_name into the function arguments?

That opens up the possibility of passing dma_to_memory = true and
chan_name as "tx" and create an inconsistency.

> > @@ -884,6 +1187,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");
> 
> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
> 
> Hence there is a need to check for DMA driver presence in the code:
> 
> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");

Do we even need the ->has_dma at all? We can just go ahead and request
the channels at probe time and respond accordingly. If there's no dmas
property in DT, dma_request_slave_channel_reason() returns an error so
we can just deal with it at that time.

So if we get -EPROBE_DEFER we can propagate that, for any other errors
we can simply fallback to PIO. Or perhaps we want to restrict fallback
to PIO for -ENODEV?

I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
The purpose of these subsystems it to abstract all of that away.
Otherwise we could just as well use custom APIs, if we're tying together
drivers in this way anyway.

> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
> driver built-in when I2C driver is built-in:

I don't think there's a requirement for that. The only dependency we
really have here is the one on the DMA engine API. Since dmaengine.h
already provides dummy implementations, there's really no need for
us to have the dependency. If the DMA engine API is completely disabled,
a call to dma_request_slave_channel_reason() will return -ENODEV and we
should just deal with that the same way we would if there was no "dmas"
property present.

Thierry

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

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 15:06, Thierry Reding пишет:
> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
> [...]
>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> [...]
>>> +		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,
>>> +				    bool dma_to_memory)
>>> +{
>>> +	struct dma_chan *dma_chan;
>>> +	u32 *dma_buf;
>>> +	dma_addr_t dma_phys;
>>> +	int ret;
>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>
>> What about to move out chan_name into the function arguments?
> 
> That opens up the possibility of passing dma_to_memory = true and
> chan_name as "tx" and create an inconsistency.
> 
>>> @@ -884,6 +1187,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");
>>
>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>
>> Hence there is a need to check for DMA driver presence in the code:
>>
>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> 
> Do we even need the ->has_dma at all? We can just go ahead and request
> the channels at probe time and respond accordingly. If there's no dmas
> property in DT, dma_request_slave_channel_reason() returns an error so
> we can just deal with it at that time.
> 
> So if we get -EPROBE_DEFER we can propagate that, for any other errors
> we can simply fallback to PIO. Or perhaps we want to restrict fallback
> to PIO for -ENODEV?
> 
> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
> The purpose of these subsystems it to abstract all of that away.
> Otherwise we could just as well use custom APIs, if we're tying together
> drivers in this way anyway.

DMA API doesn't fully abstract the dependencies between drivers, hence I disagree.

>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>> driver built-in when I2C driver is built-in:
> 
> I don't think there's a requirement for that. The only dependency we
> really have here is the one on the DMA engine API. Since dmaengine.h
> already provides dummy implementations, there's really no need for
> us to have the dependency. If the DMA engine API is completely disabled,
> a call to dma_request_slave_channel_reason() will return -ENODEV and we
> should just deal with that the same way we would if there was no "dmas"
> property present.

In my opinion it is much better to avoid I2C driver probe failing with -EPROBE_DEFER if we could. It's just one line in code and one in Kconfig.. really. 

Good point about handling -ENODEV of dma_request_slave_channel_reason(), +1 for it.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Thierry Reding]

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

On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
> 31.01.2019 15:06, Thierry Reding пишет:
> > On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
> >> 30.01.2019 19:01, Sowjanya Komatineni пишет:
> > [...]
> >>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> > [...]
> >>> +		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,
> >>> +				    bool dma_to_memory)
> >>> +{
> >>> +	struct dma_chan *dma_chan;
> >>> +	u32 *dma_buf;
> >>> +	dma_addr_t dma_phys;
> >>> +	int ret;
> >>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
> >>
> >> What about to move out chan_name into the function arguments?
> > 
> > That opens up the possibility of passing dma_to_memory = true and
> > chan_name as "tx" and create an inconsistency.
> > 
> >>> @@ -884,6 +1187,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");
> >>
> >> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
> >>
> >> Hence there is a need to check for DMA driver presence in the code:
> >>
> >> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
> >> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> > 
> > Do we even need the ->has_dma at all? We can just go ahead and request
> > the channels at probe time and respond accordingly. If there's no dmas
> > property in DT, dma_request_slave_channel_reason() returns an error so
> > we can just deal with it at that time.
> > 
> > So if we get -EPROBE_DEFER we can propagate that, for any other errors
> > we can simply fallback to PIO. Or perhaps we want to restrict fallback
> > to PIO for -ENODEV?
> > 
> > I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
> > The purpose of these subsystems it to abstract all of that away.
> > Otherwise we could just as well use custom APIs, if we're tying together
> > drivers in this way anyway.
> 
> DMA API doesn't fully abstract the dependencies between drivers, hence
> I disagree.

Why not? The dependency we're talking about here is a runtime dependency
rather than a build time dependency. Kconfig is really all about build-
time dependencies.

> >> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
> >> driver built-in when I2C driver is built-in:
> > 
> > I don't think there's a requirement for that. The only dependency we
> > really have here is the one on the DMA engine API. Since dmaengine.h
> > already provides dummy implementations, there's really no need for
> > us to have the dependency. If the DMA engine API is completely disabled,
> > a call to dma_request_slave_channel_reason() will return -ENODEV and we
> > should just deal with that the same way we would if there was no "dmas"
> > property present.
> 
> In my opinion it is much better to avoid I2C driver probe failing with
> -EPROBE_DEFER if we could. It's just one line in code and one in
> Kconfig.. really. 

The problem is that from a theoretical point of view we don't know that
APB DMA is the provider for the DMA channels. This provider could be a
completely different device on a different Tegra generation (in fact,
the DMA engine on Tegra186 is a different one, so we'd have to add that
to the list of checks to make sure we don't disable DMA there). And the
fact that we're tightly integrated is really only by accident. We could
have the same situation on a SoC that incorporates IP from multiple
different sources and multiple combinations thereof as well, so how
would you want to deal with those cases?

Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
is really more of an integration problem. Ideally of course there'd be
some way for the DMA engine subsystem to know that the provider for the
given device node will never show up and give us -ENODEV instead, but,
alas, I don't even think that would be possible. That's the price to pay
for abstraction.

Thierry

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

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 17:43, Thierry Reding пишет:
> On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
>> 31.01.2019 15:06, Thierry Reding пишет:
>>> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>>>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
>>> [...]
>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>> [...]
>>>>> +		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,
>>>>> +				    bool dma_to_memory)
>>>>> +{
>>>>> +	struct dma_chan *dma_chan;
>>>>> +	u32 *dma_buf;
>>>>> +	dma_addr_t dma_phys;
>>>>> +	int ret;
>>>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>>>
>>>> What about to move out chan_name into the function arguments?
>>>
>>> That opens up the possibility of passing dma_to_memory = true and
>>> chan_name as "tx" and create an inconsistency.
>>>
>>>>> @@ -884,6 +1187,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");
>>>>
>>>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>>>
>>>> Hence there is a need to check for DMA driver presence in the code:
>>>>
>>>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>>>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>>>
>>> Do we even need the ->has_dma at all? We can just go ahead and request
>>> the channels at probe time and respond accordingly. If there's no dmas
>>> property in DT, dma_request_slave_channel_reason() returns an error so
>>> we can just deal with it at that time.
>>>
>>> So if we get -EPROBE_DEFER we can propagate that, for any other errors
>>> we can simply fallback to PIO. Or perhaps we want to restrict fallback
>>> to PIO for -ENODEV?
>>>
>>> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
>>> The purpose of these subsystems it to abstract all of that away.
>>> Otherwise we could just as well use custom APIs, if we're tying together
>>> drivers in this way anyway.
>>
>> DMA API doesn't fully abstract the dependencies between drivers, hence
>> I disagree.
> 
> Why not? The dependency we're talking about here is a runtime dependency
> rather than a build time dependency. Kconfig is really all about build-
> time dependencies.

My understanding is that Kconfig is also about runtime dependencies, do you know if it's explicitly documented anywhere?

>>>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>>>> driver built-in when I2C driver is built-in:
>>>
>>> I don't think there's a requirement for that. The only dependency we
>>> really have here is the one on the DMA engine API. Since dmaengine.h
>>> already provides dummy implementations, there's really no need for
>>> us to have the dependency. If the DMA engine API is completely disabled,
>>> a call to dma_request_slave_channel_reason() will return -ENODEV and we
>>> should just deal with that the same way we would if there was no "dmas"
>>> property present.
>>
>> In my opinion it is much better to avoid I2C driver probe failing with
>> -EPROBE_DEFER if we could. It's just one line in code and one in
>> Kconfig.. really. 
> 
> The problem is that from a theoretical point of view we don't know that
> APB DMA is the provider for the DMA channels. This provider could be a
> completely different device on a different Tegra generation (in fact,
> the DMA engine on Tegra186 is a different one, so we'd have to add that
> to the list of checks to make sure we don't disable DMA there). And the
> fact that we're tightly integrated is really only by accident. We could
> have the same situation on a SoC that incorporates IP from multiple
> different sources and multiple combinations thereof as well, so how
> would you want to deal with those cases?
> 
> Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
> is really more of an integration problem. Ideally of course there'd be
> some way for the DMA engine subsystem to know that the provider for the
> given device node will never show up and give us -ENODEV instead, but,
> alas, I don't even think that would be possible. That's the price to pay
> for abstraction.

It's not a big problem to solve for this case, there is of_machine_is_compatible(). To me it's more a question about the will to invest some extra effort to support all of possible combinations. If there is no such will, then at least those unpopular combinations shouldn't hurt and thus it should make sense to add an explicit build-dependency on the DMA drivers.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Thierry Reding]

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

On Thu, Jan 31, 2019 at 06:02:45PM +0300, Dmitry Osipenko wrote:
> 31.01.2019 17:43, Thierry Reding пишет:
> > On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
> >> 31.01.2019 15:06, Thierry Reding пишет:
> >>> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
> >>>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
> >>> [...]
> >>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> >>> [...]
> >>>>> +		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,
> >>>>> +				    bool dma_to_memory)
> >>>>> +{
> >>>>> +	struct dma_chan *dma_chan;
> >>>>> +	u32 *dma_buf;
> >>>>> +	dma_addr_t dma_phys;
> >>>>> +	int ret;
> >>>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
> >>>>
> >>>> What about to move out chan_name into the function arguments?
> >>>
> >>> That opens up the possibility of passing dma_to_memory = true and
> >>> chan_name as "tx" and create an inconsistency.
> >>>
> >>>>> @@ -884,6 +1187,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");
> >>>>
> >>>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
> >>>>
> >>>> Hence there is a need to check for DMA driver presence in the code:
> >>>>
> >>>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
> >>>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
> >>>
> >>> Do we even need the ->has_dma at all? We can just go ahead and request
> >>> the channels at probe time and respond accordingly. If there's no dmas
> >>> property in DT, dma_request_slave_channel_reason() returns an error so
> >>> we can just deal with it at that time.
> >>>
> >>> So if we get -EPROBE_DEFER we can propagate that, for any other errors
> >>> we can simply fallback to PIO. Or perhaps we want to restrict fallback
> >>> to PIO for -ENODEV?
> >>>
> >>> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
> >>> The purpose of these subsystems it to abstract all of that away.
> >>> Otherwise we could just as well use custom APIs, if we're tying together
> >>> drivers in this way anyway.
> >>
> >> DMA API doesn't fully abstract the dependencies between drivers, hence
> >> I disagree.
> > 
> > Why not? The dependency we're talking about here is a runtime dependency
> > rather than a build time dependency. Kconfig is really all about build-
> > time dependencies.
> 
> My understanding is that Kconfig is also about runtime dependencies,
> do you know if it's explicitly documented anywhere?

I don't think it's explicitly documented, just a common practice that
I've seen applied multiple times over the years. A quick grep through
the drivers/ subdirectory confirms that it's not typical to have this
sort of dependency in the code.

Similarly, Kconfig uses select primarily to pull in dependencies that
are in the form of helper libraries and such. Occasionally you'll have
some ARCH_* option select a couple of features, or even drivers, but
that is mostly a shortcut to explicitly having to list the essentials
in a defconfig.

Another reason why it's not good to model these runtime dependencies in
Kconfig is because they unnecessarily restrict the driver. For example,
if you want to build a specialized Linux binary for Tegra186, you will
certainly want to include the i2c-tegra driver. At the same time you
won't want to include the APB DMA driver because it doesn't exist on
Tegra186. Instead you'd want the (non-existent) GPC DMA driver. select
on the APB DMA driver will unconditionally pull in the driver, depends
will only allow you to build i2c-tegra if the APB DMA driver is also
enabled and the conditional in the code may lead to not using DMA
because the APB DMA driver is not available. So you'd have to modify the
i2c-tegra driver to take into account the GPC DMA driver.

> >>>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
> >>>> driver built-in when I2C driver is built-in:
> >>>
> >>> I don't think there's a requirement for that. The only dependency we
> >>> really have here is the one on the DMA engine API. Since dmaengine.h
> >>> already provides dummy implementations, there's really no need for
> >>> us to have the dependency. If the DMA engine API is completely disabled,
> >>> a call to dma_request_slave_channel_reason() will return -ENODEV and we
> >>> should just deal with that the same way we would if there was no "dmas"
> >>> property present.
> >>
> >> In my opinion it is much better to avoid I2C driver probe failing with
> >> -EPROBE_DEFER if we could. It's just one line in code and one in
> >> Kconfig.. really. 
> > 
> > The problem is that from a theoretical point of view we don't know that
> > APB DMA is the provider for the DMA channels. This provider could be a
> > completely different device on a different Tegra generation (in fact,
> > the DMA engine on Tegra186 is a different one, so we'd have to add that
> > to the list of checks to make sure we don't disable DMA there). And the
> > fact that we're tightly integrated is really only by accident. We could
> > have the same situation on a SoC that incorporates IP from multiple
> > different sources and multiple combinations thereof as well, so how
> > would you want to deal with those cases?
> > 
> > Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
> > is really more of an integration problem. Ideally of course there'd be
> > some way for the DMA engine subsystem to know that the provider for the
> > given device node will never show up and give us -ENODEV instead, but,
> > alas, I don't even think that would be possible. That's the price to pay
> > for abstraction.
> 
> It's not a big problem to solve for this case, there is
> of_machine_is_compatible(). To me it's more a question about the will
> to invest some extra effort to support all of possible combinations.
> If there is no such will, then at least those unpopular combinations
> shouldn't hurt and thus it should make sense to add an explicit
> build-dependency on the DMA drivers.

I think we're arguing about the same thing, only coming at it from
different angles. For me "all possible combinations" also includes the
case where you want to be able to run the driver with DMA if the APB DMA
is not enabled. And I similarly want to be able to run without DMA if
the APB DMA is enabled (by explicitly removing dmas from DT for
example). It just seems that we can't have it both ways.

Also the i2c-tegra driver can perfectly well function without DMA
support (it's done so ever since it was first merged). Keeping existing
functionality shouldn't require the addition of another driver.

Given the deadlock, I think I'd prefer the option of adding the
conditional in the code. I think that's the most accurate description of
the dependency, even though ideally it would be handled transparently by
the DMA engine API. Would that be an acceptable compromise?

Thierry

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

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 19:01, Thierry Reding пишет:
> On Thu, Jan 31, 2019 at 06:02:45PM +0300, Dmitry Osipenko wrote:
>> 31.01.2019 17:43, Thierry Reding пишет:
>>> On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
>>>> 31.01.2019 15:06, Thierry Reding пишет:
>>>>> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>>>>>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
>>>>> [...]
>>>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>>>> [...]
>>>>>>> +		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,
>>>>>>> +				    bool dma_to_memory)
>>>>>>> +{
>>>>>>> +	struct dma_chan *dma_chan;
>>>>>>> +	u32 *dma_buf;
>>>>>>> +	dma_addr_t dma_phys;
>>>>>>> +	int ret;
>>>>>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>>>>>
>>>>>> What about to move out chan_name into the function arguments?
>>>>>
>>>>> That opens up the possibility of passing dma_to_memory = true and
>>>>> chan_name as "tx" and create an inconsistency.
>>>>>
>>>>>>> @@ -884,6 +1187,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");
>>>>>>
>>>>>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>>>>>
>>>>>> Hence there is a need to check for DMA driver presence in the code:
>>>>>>
>>>>>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>>>>>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>>>>>
>>>>> Do we even need the ->has_dma at all? We can just go ahead and request
>>>>> the channels at probe time and respond accordingly. If there's no dmas
>>>>> property in DT, dma_request_slave_channel_reason() returns an error so
>>>>> we can just deal with it at that time.
>>>>>
>>>>> So if we get -EPROBE_DEFER we can propagate that, for any other errors
>>>>> we can simply fallback to PIO. Or perhaps we want to restrict fallback
>>>>> to PIO for -ENODEV?
>>>>>
>>>>> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
>>>>> The purpose of these subsystems it to abstract all of that away.
>>>>> Otherwise we could just as well use custom APIs, if we're tying together
>>>>> drivers in this way anyway.
>>>>
>>>> DMA API doesn't fully abstract the dependencies between drivers, hence
>>>> I disagree.
>>>
>>> Why not? The dependency we're talking about here is a runtime dependency
>>> rather than a build time dependency. Kconfig is really all about build-
>>> time dependencies.
>>
>> My understanding is that Kconfig is also about runtime dependencies,
>> do you know if it's explicitly documented anywhere?
> 
> I don't think it's explicitly documented, just a common practice that
> I've seen applied multiple times over the years. A quick grep through
> the drivers/ subdirectory confirms that it's not typical to have this
> sort of dependency in the code.
> 
> Similarly, Kconfig uses select primarily to pull in dependencies that
> are in the form of helper libraries and such. Occasionally you'll have
> some ARCH_* option select a couple of features, or even drivers, but
> that is mostly a shortcut to explicitly having to list the essentials
> in a defconfig.
> 
> Another reason why it's not good to model these runtime dependencies in
> Kconfig is because they unnecessarily restrict the driver. For example,
> if you want to build a specialized Linux binary for Tegra186, you will
> certainly want to include the i2c-tegra driver. At the same time you
> won't want to include the APB DMA driver because it doesn't exist on
> Tegra186. Instead you'd want the (non-existent) GPC DMA driver. select
> on the APB DMA driver will unconditionally pull in the driver, depends
> will only allow you to build i2c-tegra if the APB DMA driver is also
> enabled and the conditional in the code may lead to not using DMA
> because the APB DMA driver is not available. So you'd have to modify the
> i2c-tegra driver to take into account the GPC DMA driver.
> 
>>>>>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>>>>>> driver built-in when I2C driver is built-in:
>>>>>
>>>>> I don't think there's a requirement for that. The only dependency we
>>>>> really have here is the one on the DMA engine API. Since dmaengine.h
>>>>> already provides dummy implementations, there's really no need for
>>>>> us to have the dependency. If the DMA engine API is completely disabled,
>>>>> a call to dma_request_slave_channel_reason() will return -ENODEV and we
>>>>> should just deal with that the same way we would if there was no "dmas"
>>>>> property present.
>>>>
>>>> In my opinion it is much better to avoid I2C driver probe failing with
>>>> -EPROBE_DEFER if we could. It's just one line in code and one in
>>>> Kconfig.. really. 
>>>
>>> The problem is that from a theoretical point of view we don't know that
>>> APB DMA is the provider for the DMA channels. This provider could be a
>>> completely different device on a different Tegra generation (in fact,
>>> the DMA engine on Tegra186 is a different one, so we'd have to add that
>>> to the list of checks to make sure we don't disable DMA there). And the
>>> fact that we're tightly integrated is really only by accident. We could
>>> have the same situation on a SoC that incorporates IP from multiple
>>> different sources and multiple combinations thereof as well, so how
>>> would you want to deal with those cases?
>>>
>>> Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
>>> is really more of an integration problem. Ideally of course there'd be
>>> some way for the DMA engine subsystem to know that the provider for the
>>> given device node will never show up and give us -ENODEV instead, but,
>>> alas, I don't even think that would be possible. That's the price to pay
>>> for abstraction.
>>
>> It's not a big problem to solve for this case, there is
>> of_machine_is_compatible(). To me it's more a question about the will
>> to invest some extra effort to support all of possible combinations.
>> If there is no such will, then at least those unpopular combinations
>> shouldn't hurt and thus it should make sense to add an explicit
>> build-dependency on the DMA drivers.
> 
> I think we're arguing about the same thing, only coming at it from
> different angles. For me "all possible combinations" also includes the
> case where you want to be able to run the driver with DMA if the APB DMA
> is not enabled. And I similarly want to be able to run without DMA if
> the APB DMA is enabled (by explicitly removing dmas from DT for
> example). It just seems that we can't have it both ways.
> 
> Also the i2c-tegra driver can perfectly well function without DMA
> support (it's done so ever since it was first merged). Keeping existing
> functionality shouldn't require the addition of another driver.
> 
> Given the deadlock, I think I'd prefer the option of adding the
> conditional in the code. I think that's the most accurate description of
> the dependency, even though ideally it would be handled transparently by
> the DMA engine API. Would that be an acceptable compromise?

Adding conditional to the code is not enough. Tegra I2C driver could be built-in, while APB DMA driver is a loadable module, hence Tegra I2C will fail to probe with -EPROBE_DEFER. Tegra I2C must select all of the relevant DMA drivers to avoid that situation. Later on it shouldn't be a problem to add .has_gpc_dma to the tegra_i2c_hw_feature and then check in the code whether corresponding DMA driver is enabled or not in the kernel's config.

Combining the code checking with the Kconfig selection that I'm suggesting covers all of possible combinations, otherwise please give me an explicit example when it could fail.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 19:27, Dmitry Osipenko пишет:
> 31.01.2019 19:01, Thierry Reding пишет:
>> On Thu, Jan 31, 2019 at 06:02:45PM +0300, Dmitry Osipenko wrote:
>>> 31.01.2019 17:43, Thierry Reding пишет:
>>>> On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
>>>>> 31.01.2019 15:06, Thierry Reding пишет:
>>>>>> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>>>>>>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
>>>>>> [...]
>>>>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>>>>> [...]
>>>>>>>> +		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,
>>>>>>>> +				    bool dma_to_memory)
>>>>>>>> +{
>>>>>>>> +	struct dma_chan *dma_chan;
>>>>>>>> +	u32 *dma_buf;
>>>>>>>> +	dma_addr_t dma_phys;
>>>>>>>> +	int ret;
>>>>>>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>>>>>>
>>>>>>> What about to move out chan_name into the function arguments?
>>>>>>
>>>>>> That opens up the possibility of passing dma_to_memory = true and
>>>>>> chan_name as "tx" and create an inconsistency.
>>>>>>
>>>>>>>> @@ -884,6 +1187,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");
>>>>>>>
>>>>>>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>>>>>>
>>>>>>> Hence there is a need to check for DMA driver presence in the code:
>>>>>>>
>>>>>>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>>>>>>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>>>>>>
>>>>>> Do we even need the ->has_dma at all? We can just go ahead and request
>>>>>> the channels at probe time and respond accordingly. If there's no dmas
>>>>>> property in DT, dma_request_slave_channel_reason() returns an error so
>>>>>> we can just deal with it at that time.
>>>>>>
>>>>>> So if we get -EPROBE_DEFER we can propagate that, for any other errors
>>>>>> we can simply fallback to PIO. Or perhaps we want to restrict fallback
>>>>>> to PIO for -ENODEV?
>>>>>>
>>>>>> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
>>>>>> The purpose of these subsystems it to abstract all of that away.
>>>>>> Otherwise we could just as well use custom APIs, if we're tying together
>>>>>> drivers in this way anyway.
>>>>>
>>>>> DMA API doesn't fully abstract the dependencies between drivers, hence
>>>>> I disagree.
>>>>
>>>> Why not? The dependency we're talking about here is a runtime dependency
>>>> rather than a build time dependency. Kconfig is really all about build-
>>>> time dependencies.
>>>
>>> My understanding is that Kconfig is also about runtime dependencies,
>>> do you know if it's explicitly documented anywhere?
>>
>> I don't think it's explicitly documented, just a common practice that
>> I've seen applied multiple times over the years. A quick grep through
>> the drivers/ subdirectory confirms that it's not typical to have this
>> sort of dependency in the code.
>>
>> Similarly, Kconfig uses select primarily to pull in dependencies that
>> are in the form of helper libraries and such. Occasionally you'll have
>> some ARCH_* option select a couple of features, or even drivers, but
>> that is mostly a shortcut to explicitly having to list the essentials
>> in a defconfig.
>>
>> Another reason why it's not good to model these runtime dependencies in
>> Kconfig is because they unnecessarily restrict the driver. For example,
>> if you want to build a specialized Linux binary for Tegra186, you will
>> certainly want to include the i2c-tegra driver. At the same time you
>> won't want to include the APB DMA driver because it doesn't exist on
>> Tegra186. Instead you'd want the (non-existent) GPC DMA driver. select
>> on the APB DMA driver will unconditionally pull in the driver, depends
>> will only allow you to build i2c-tegra if the APB DMA driver is also
>> enabled and the conditional in the code may lead to not using DMA
>> because the APB DMA driver is not available. So you'd have to modify the
>> i2c-tegra driver to take into account the GPC DMA driver.
>>
>>>>>>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>>>>>>> driver built-in when I2C driver is built-in:
>>>>>>
>>>>>> I don't think there's a requirement for that. The only dependency we
>>>>>> really have here is the one on the DMA engine API. Since dmaengine.h
>>>>>> already provides dummy implementations, there's really no need for
>>>>>> us to have the dependency. If the DMA engine API is completely disabled,
>>>>>> a call to dma_request_slave_channel_reason() will return -ENODEV and we
>>>>>> should just deal with that the same way we would if there was no "dmas"
>>>>>> property present.
>>>>>
>>>>> In my opinion it is much better to avoid I2C driver probe failing with
>>>>> -EPROBE_DEFER if we could. It's just one line in code and one in
>>>>> Kconfig.. really. 
>>>>
>>>> The problem is that from a theoretical point of view we don't know that
>>>> APB DMA is the provider for the DMA channels. This provider could be a
>>>> completely different device on a different Tegra generation (in fact,
>>>> the DMA engine on Tegra186 is a different one, so we'd have to add that
>>>> to the list of checks to make sure we don't disable DMA there). And the
>>>> fact that we're tightly integrated is really only by accident. We could
>>>> have the same situation on a SoC that incorporates IP from multiple
>>>> different sources and multiple combinations thereof as well, so how
>>>> would you want to deal with those cases?
>>>>
>>>> Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
>>>> is really more of an integration problem. Ideally of course there'd be
>>>> some way for the DMA engine subsystem to know that the provider for the
>>>> given device node will never show up and give us -ENODEV instead, but,
>>>> alas, I don't even think that would be possible. That's the price to pay
>>>> for abstraction.
>>>
>>> It's not a big problem to solve for this case, there is
>>> of_machine_is_compatible(). To me it's more a question about the will
>>> to invest some extra effort to support all of possible combinations.
>>> If there is no such will, then at least those unpopular combinations
>>> shouldn't hurt and thus it should make sense to add an explicit
>>> build-dependency on the DMA drivers.
>>
>> I think we're arguing about the same thing, only coming at it from
>> different angles. For me "all possible combinations" also includes the
>> case where you want to be able to run the driver with DMA if the APB DMA
>> is not enabled. And I similarly want to be able to run without DMA if
>> the APB DMA is enabled (by explicitly removing dmas from DT for
>> example). It just seems that we can't have it both ways.
>>
>> Also the i2c-tegra driver can perfectly well function without DMA
>> support (it's done so ever since it was first merged). Keeping existing
>> functionality shouldn't require the addition of another driver.
>>
>> Given the deadlock, I think I'd prefer the option of adding the
>> conditional in the code. I think that's the most accurate description of
>> the dependency, even though ideally it would be handled transparently by
>> the DMA engine API. Would that be an acceptable compromise?
> 
> Adding conditional to the code is not enough. Tegra I2C driver could be built-in, while APB DMA driver is a loadable module, hence Tegra I2C will fail to probe with -EPROBE_DEFER. Tegra I2C must select all of the relevant DMA drivers to avoid that situation. Later on it shouldn't be a problem to add .has_gpc_dma to the tegra_i2c_hw_feature and then check in the code whether corresponding DMA driver is enabled or not in the kernel's config.
> 
> Combining the code checking with the Kconfig selection that I'm suggesting covers all of possible combinations, otherwise please give me an explicit example when it could fail.
> 

Actually .has_gpc_dma need to be added and handled right now to maintain backwards compatibility with the future DT updates.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 19:27, Dmitry Osipenko пишет:
> 31.01.2019 19:01, Thierry Reding пишет:
>> On Thu, Jan 31, 2019 at 06:02:45PM +0300, Dmitry Osipenko wrote:
>>> 31.01.2019 17:43, Thierry Reding пишет:
>>>> On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
>>>>> 31.01.2019 15:06, Thierry Reding пишет:
>>>>>> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>>>>>>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
>>>>>> [...]
>>>>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>>>>> [...]
>>>>>>>> +		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,
>>>>>>>> +				    bool dma_to_memory)
>>>>>>>> +{
>>>>>>>> +	struct dma_chan *dma_chan;
>>>>>>>> +	u32 *dma_buf;
>>>>>>>> +	dma_addr_t dma_phys;
>>>>>>>> +	int ret;
>>>>>>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>>>>>>
>>>>>>> What about to move out chan_name into the function arguments?
>>>>>>
>>>>>> That opens up the possibility of passing dma_to_memory = true and
>>>>>> chan_name as "tx" and create an inconsistency.
>>>>>>
>>>>>>>> @@ -884,6 +1187,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");
>>>>>>>
>>>>>>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>>>>>>
>>>>>>> Hence there is a need to check for DMA driver presence in the code:
>>>>>>>
>>>>>>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>>>>>>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>>>>>>
>>>>>> Do we even need the ->has_dma at all? We can just go ahead and request
>>>>>> the channels at probe time and respond accordingly. If there's no dmas
>>>>>> property in DT, dma_request_slave_channel_reason() returns an error so
>>>>>> we can just deal with it at that time.
>>>>>>
>>>>>> So if we get -EPROBE_DEFER we can propagate that, for any other errors
>>>>>> we can simply fallback to PIO. Or perhaps we want to restrict fallback
>>>>>> to PIO for -ENODEV?
>>>>>>
>>>>>> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
>>>>>> The purpose of these subsystems it to abstract all of that away.
>>>>>> Otherwise we could just as well use custom APIs, if we're tying together
>>>>>> drivers in this way anyway.
>>>>>
>>>>> DMA API doesn't fully abstract the dependencies between drivers, hence
>>>>> I disagree.
>>>>
>>>> Why not? The dependency we're talking about here is a runtime dependency
>>>> rather than a build time dependency. Kconfig is really all about build-
>>>> time dependencies.
>>>
>>> My understanding is that Kconfig is also about runtime dependencies,
>>> do you know if it's explicitly documented anywhere?
>>
>> I don't think it's explicitly documented, just a common practice that
>> I've seen applied multiple times over the years. A quick grep through
>> the drivers/ subdirectory confirms that it's not typical to have this
>> sort of dependency in the code.
>>
>> Similarly, Kconfig uses select primarily to pull in dependencies that
>> are in the form of helper libraries and such. Occasionally you'll have
>> some ARCH_* option select a couple of features, or even drivers, but
>> that is mostly a shortcut to explicitly having to list the essentials
>> in a defconfig.
>>
>> Another reason why it's not good to model these runtime dependencies in
>> Kconfig is because they unnecessarily restrict the driver. For example,
>> if you want to build a specialized Linux binary for Tegra186, you will
>> certainly want to include the i2c-tegra driver. At the same time you
>> won't want to include the APB DMA driver because it doesn't exist on
>> Tegra186. Instead you'd want the (non-existent) GPC DMA driver. select
>> on the APB DMA driver will unconditionally pull in the driver, depends
>> will only allow you to build i2c-tegra if the APB DMA driver is also
>> enabled and the conditional in the code may lead to not using DMA
>> because the APB DMA driver is not available. So you'd have to modify the
>> i2c-tegra driver to take into account the GPC DMA driver.
>>
>>>>>>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>>>>>>> driver built-in when I2C driver is built-in:
>>>>>>
>>>>>> I don't think there's a requirement for that. The only dependency we
>>>>>> really have here is the one on the DMA engine API. Since dmaengine.h
>>>>>> already provides dummy implementations, there's really no need for
>>>>>> us to have the dependency. If the DMA engine API is completely disabled,
>>>>>> a call to dma_request_slave_channel_reason() will return -ENODEV and we
>>>>>> should just deal with that the same way we would if there was no "dmas"
>>>>>> property present.
>>>>>
>>>>> In my opinion it is much better to avoid I2C driver probe failing with
>>>>> -EPROBE_DEFER if we could. It's just one line in code and one in
>>>>> Kconfig.. really. 
>>>>
>>>> The problem is that from a theoretical point of view we don't know that
>>>> APB DMA is the provider for the DMA channels. This provider could be a
>>>> completely different device on a different Tegra generation (in fact,
>>>> the DMA engine on Tegra186 is a different one, so we'd have to add that
>>>> to the list of checks to make sure we don't disable DMA there). And the
>>>> fact that we're tightly integrated is really only by accident. We could
>>>> have the same situation on a SoC that incorporates IP from multiple
>>>> different sources and multiple combinations thereof as well, so how
>>>> would you want to deal with those cases?
>>>>
>>>> Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
>>>> is really more of an integration problem. Ideally of course there'd be
>>>> some way for the DMA engine subsystem to know that the provider for the
>>>> given device node will never show up and give us -ENODEV instead, but,
>>>> alas, I don't even think that would be possible. That's the price to pay
>>>> for abstraction.
>>>
>>> It's not a big problem to solve for this case, there is
>>> of_machine_is_compatible(). To me it's more a question about the will
>>> to invest some extra effort to support all of possible combinations.
>>> If there is no such will, then at least those unpopular combinations
>>> shouldn't hurt and thus it should make sense to add an explicit
>>> build-dependency on the DMA drivers.
>>
>> I think we're arguing about the same thing, only coming at it from
>> different angles. For me "all possible combinations" also includes the
>> case where you want to be able to run the driver with DMA if the APB DMA
>> is not enabled. And I similarly want to be able to run without DMA if
>> the APB DMA is enabled (by explicitly removing dmas from DT for
>> example). It just seems that we can't have it both ways.
>>
>> Also the i2c-tegra driver can perfectly well function without DMA
>> support (it's done so ever since it was first merged). Keeping existing
>> functionality shouldn't require the addition of another driver.
>>
>> Given the deadlock, I think I'd prefer the option of adding the
>> conditional in the code. I think that's the most accurate description of
>> the dependency, even though ideally it would be handled transparently by
>> the DMA engine API. Would that be an acceptable compromise?
> 
> Adding conditional to the code is not enough. Tegra I2C driver could be built-in, while APB DMA driver is a loadable module, hence Tegra I2C will fail to probe with -EPROBE_DEFER. Tegra I2C must select all of the relevant DMA drivers to avoid that situation. Later on it shouldn't be a problem to add .has_gpc_dma to the tegra_i2c_hw_feature and then check in the code whether corresponding DMA driver is enabled or not in the kernel's config.
> 
> Combining the code checking with the Kconfig selection that I'm suggesting covers all of possible combinations, otherwise please give me an explicit example when it could fail.
> 

Hm, oddly TEGRA20_APB_DMA driver can't be built as a module because it is a bool in Kconfig, but the code has the remove function. Maybe someday somebody will clean out all that mess :) Hence, Thierry, your point make sense. Still could be good to make DMA support explicitly optional for the I2C driver by adding Kconfig option for it, given that it adds some extra complexity and not well tested, up to you to decide.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 18:02, Dmitry Osipenko пишет:
> 31.01.2019 17:43, Thierry Reding пишет:
>> On Thu, Jan 31, 2019 at 05:06:18PM +0300, Dmitry Osipenko wrote:
>>> 31.01.2019 15:06, Thierry Reding пишет:
>>>> On Thu, Jan 31, 2019 at 03:05:48AM +0300, Dmitry Osipenko wrote:
>>>>> 30.01.2019 19:01, Sowjanya Komatineni пишет:
>>>> [...]
>>>>>> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
>>>> [...]
>>>>>> +		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,
>>>>>> +				    bool dma_to_memory)
>>>>>> +{
>>>>>> +	struct dma_chan *dma_chan;
>>>>>> +	u32 *dma_buf;
>>>>>> +	dma_addr_t dma_phys;
>>>>>> +	int ret;
>>>>>> +	const char *chan_name = dma_to_memory ? "rx" : "tx";
>>>>>
>>>>> What about to move out chan_name into the function arguments?
>>>>
>>>> That opens up the possibility of passing dma_to_memory = true and
>>>> chan_name as "tx" and create an inconsistency.
>>>>
>>>>>> @@ -884,6 +1187,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");
>>>>>
>>>>> Not only the existence of "dmas" property defines whether DMA is available. DMA subsystem could be disabled in the kernels configuration.
>>>>>
>>>>> Hence there is a need to check for DMA driver presence in the code:
>>>>>
>>>>> 	if (IS_ENABLED(CONFIG_TEGRA20_APB_DMA))
>>>>> 		i2c_dev->has_dma = of_property_read_bool(np, "dmas");
>>>>
>>>> Do we even need the ->has_dma at all? We can just go ahead and request
>>>> the channels at probe time and respond accordingly. If there's no dmas
>>>> property in DT, dma_request_slave_channel_reason() returns an error so
>>>> we can just deal with it at that time.
>>>>
>>>> So if we get -EPROBE_DEFER we can propagate that, for any other errors
>>>> we can simply fallback to PIO. Or perhaps we want to restrict fallback
>>>> to PIO for -ENODEV?
>>>>
>>>> I wouldn't want to add an IS_ENABLED(CONFIG_TEGRA20_APB_DMA) in here.
>>>> The purpose of these subsystems it to abstract all of that away.
>>>> Otherwise we could just as well use custom APIs, if we're tying together
>>>> drivers in this way anyway.
>>>
>>> DMA API doesn't fully abstract the dependencies between drivers, hence
>>> I disagree.
>>
>> Why not? The dependency we're talking about here is a runtime dependency
>> rather than a build time dependency. Kconfig is really all about build-
>> time dependencies.
> 
> My understanding is that Kconfig is also about runtime dependencies, do you know if it's explicitly documented anywhere?
> 
>>>>> Also Tegra I2C driver should select DMA driver in Kconfig to make DMA
>>>>> driver built-in when I2C driver is built-in:
>>>>
>>>> I don't think there's a requirement for that. The only dependency we
>>>> really have here is the one on the DMA engine API. Since dmaengine.h
>>>> already provides dummy implementations, there's really no need for
>>>> us to have the dependency. If the DMA engine API is completely disabled,
>>>> a call to dma_request_slave_channel_reason() will return -ENODEV and we
>>>> should just deal with that the same way we would if there was no "dmas"
>>>> property present.
>>>
>>> In my opinion it is much better to avoid I2C driver probe failing with
>>> -EPROBE_DEFER if we could. It's just one line in code and one in
>>> Kconfig.. really. 
>>
>> The problem is that from a theoretical point of view we don't know that
>> APB DMA is the provider for the DMA channels. This provider could be a
>> completely different device on a different Tegra generation (in fact,
>> the DMA engine on Tegra186 is a different one, so we'd have to add that
>> to the list of checks to make sure we don't disable DMA there). And the
>> fact that we're tightly integrated is really only by accident. We could
>> have the same situation on a SoC that incorporates IP from multiple
>> different sources and multiple combinations thereof as well, so how
>> would you want to deal with those cases?
>>
>> Agreed, failing with -EPROBE_DEFER is suboptimal in that case, but that
>> is really more of an integration problem. Ideally of course there'd be
>> some way for the DMA engine subsystem to know that the provider for the
>> given device node will never show up and give us -ENODEV instead, but,
>> alas, I don't even think that would be possible. That's the price to pay
>> for abstraction.
> 
> It's not a big problem to solve for this case, there is of_machine_is_compatible(). To me it's more a question about the will to invest some extra effort to support all of possible combinations. If there is no such will, then at least those unpopular combinations shouldn't hurt and thus it should make sense to add an explicit build-dependency on the DMA drivers.
> 

Ah, my bad. I assumed that selecting TEGRA20_APB_DMA won't enable DMA_ENGINE.

Then what about something like this?

config I2C_TEGRA
	tristate "NVIDIA Tegra internal I2C controller"
	depends on ARCH_TEGRA
	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_2x_SOC)
	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_3x_SOC)
	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_114_SOC)
	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_124_SOC)
	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_210_SOC)
	help
	  If you say yes to this option, support will be included for the
	  I2C controller embedded in NVIDIA Tegra SOCs

config I2C_TEGRA_DMA_SUPPORT
	bool "NVIDIA Tegra internal I2C controller DMA support"
	depends on I2C_TEGRA
	help
	  If you say yes to this option, DMA engine integration support will
	  be included for the I2C controller embedded in NVIDIA Tegra SOCs


Well, since my assumption was incorrect, maybe indeed not worth to care about all cases. I'm in doubt now.

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 19:55, Dmitry Osipenko пишет:
> config I2C_TEGRA
> 	tristate "NVIDIA Tegra internal I2C controller"
> 	depends on ARCH_TEGRA
> 	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_2x_SOC)
> 	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_3x_SOC)
> 	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_114_SOC)
> 	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_124_SOC)
> 	select TEGRA20_APB_DMA if (I2C_TEGRA_DMA_SUPPORT && ARCH_TEGRA_210_SOC)
> 	help
> 	  If you say yes to this option, support will be included for the
> 	  I2C controller embedded in NVIDIA Tegra SOCs
> 
> config I2C_TEGRA_DMA_SUPPORT
> 	bool "NVIDIA Tegra internal I2C controller DMA support"
> 	depends on I2C_TEGRA
> 	help
> 	  If you say yes to this option, DMA engine integration support will
> 	  be included for the I2C controller embedded in NVIDIA Tegra SOCs

Thinking a bit more on it, perhaps this will be an ideal variant:

On top of V8:
diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 046aeb92a467..520ead24fb51 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -1016,11 +1016,23 @@ config I2C_SYNQUACER
 
 config I2C_TEGRA
 	tristate "NVIDIA Tegra internal I2C controller"
-	depends on (ARCH_TEGRA && TEGRA20_APB_DMA)
+	depends on ARCH_TEGRA
 	help
 	  If you say yes to this option, support will be included for the
 	  I2C controller embedded in NVIDIA Tegra SOCs
 
+config I2C_TEGRA_DMA_SUPPORT
+	bool "NVIDIA Tegra internal I2C controller DMA support"
+	depends on I2C_TEGRA
+	depends on TEGRA20_APB_DMA && ARCH_TEGRA_2x_SOC
+	depends on TEGRA20_APB_DMA && ARCH_TEGRA_3x_SOC
+	depends on TEGRA20_APB_DMA && ARCH_TEGRA_114_SOC
+	depends on TEGRA20_APB_DMA && ARCH_TEGRA_124_SOC
+	depends on TEGRA20_APB_DMA && ARCH_TEGRA_210_SOC
+	help
+	  If you say yes to this option, DMA engine integration support will
+	  be included for the I2C controller embedded in NVIDIA Tegra SOCs
+
 config I2C_TEGRA_BPMP
 	tristate "NVIDIA Tegra BPMP I2C controller"
 	depends on TEGRA_BPMP
diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index fe5b89abc576..bce7283b027b 100644
--- a/drivers/i2c/busses/i2c-tegra.c
+++ b/drivers/i2c/busses/i2c-tegra.c
@@ -225,6 +225,7 @@ struct tegra_i2c_hw_feature {
 	u32 setup_hold_time_std_mode;
 	u32 setup_hold_time_fast_fast_plus_mode;
 	u32 setup_hold_time_hs_mode;
+	bool has_gpc_dma;
 };
 
 /**
@@ -389,51 +390,51 @@ static int tegra_i2c_dma_submit(struct tegra_i2c_dev *i2c_dev, size_t len)
 	return 0;
 }
 
-static int tegra_i2c_init_dma_param(struct tegra_i2c_dev *i2c_dev)
+static int tegra_i2c_init_dma(struct tegra_i2c_dev *i2c_dev)
 {
-	struct dma_chan *dma_chan;
-	u32 *dma_buf;
-	dma_addr_t dma_phys;
+	struct device *dev = i2c_dev->dev;
+	int err;
 
 	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);
+		return 0;
 
-		i2c_dev->rx_dma_chan = dma_chan;
+	i2c_dev->rx_dma_chan = dma_request_slave_channel_reason(dev, "rx");
+	if (IS_ERR(i2c_dev->rx_dma_chan)) {
+		err = PTR_ERR(i2c_dev->rx_dma_chan);
+		goto err_out;
 	}
 
-	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;
+	i2c_dev->tx_dma_chan = dma_request_slave_channel_reason(dev, "tx");
+	if (IS_ERR(i2c_dev->tx_dma_chan)) {
+		err = PTR_ERR(i2c_dev->tx_dma_chan);
+		goto err_release_rx;
 	}
 
-	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);
+	i2c_dev->dma_buf = dma_alloc_coherent(dev, i2c_dev->dma_buf_size,
+					      &i2c_dev->dma_phys,
+					      GFP_KERNEL | __GFP_NOWARN);
+	if (!i2c_dev->dma_buf) {
+		dev_err(dev, "failed to allocate the DMA buffer\n");
+		err = -ENOMEM;
+		goto err_release_tx;
+	}
 
-		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;
-		}
+	return 0;
 
-		i2c_dev->dma_buf = dma_buf;
-		i2c_dev->dma_phys = dma_phys;
+err_release_tx:
+	dma_release_channel(i2c_dev->tx_dma_chan);
+err_release_rx:
+	dma_release_channel(i2c_dev->rx_dma_chan);
+err_out:
+	if (err == -ENODEV) {
+		i2c_dev->has_dma = false;
+		i2c_dev->tx_dma_chan = NULL;
+		i2c_dev->rx_dma_chan = NULL;
+		i2c_dev->dma_buf = NULL;
+		return 0;
 	}
 
-	return 0;
+	return err;
 }
 
 static int tegra_i2c_flush_fifos(struct tegra_i2c_dev *i2c_dev)
@@ -991,8 +992,8 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	size_t xfer_size;
 	u32 *buffer = 0;
 	int err = 0;
-	bool dma = false;
-	struct dma_chan *chan;
+	bool dma;
+	struct dma_chan *chan = NULL;
 	u16 xfer_time = 100;
 
 	tegra_i2c_flush_fifos(i2c_dev);
@@ -1016,14 +1017,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	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);
-		if (err < 0) {
-			dev_dbg(i2c_dev->dev, "switching to PIO transfer\n");
-			dma = false;
-		}
-	}
+	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN) && i2c_dev->has_dma;
 
 	i2c_dev->is_curr_dma_xfer = dma;
 	spin_lock_irqsave(&i2c_dev->xfer_lock, flags);
@@ -1245,7 +1239,10 @@ 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");
+	if (IS_ENABLED(CONFIG_I2C_TEGRA_DMA_SUPPORT) &&
+	    IS_ENABLED(CONFIG_TEGRA20_APB_DMA) &&
+	    !i2c_dev->hw->has_gpc_dma)
+		i2c_dev->has_dma = true;
 }
 
 static const struct i2c_algorithm tegra_i2c_algo = {
@@ -1280,6 +1277,7 @@ static const struct tegra_i2c_hw_feature tegra20_i2c_hw = {
 	.setup_hold_time_std_mode = 0x0,
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
+	.has_gpc_dma = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
@@ -1302,6 +1300,7 @@ static const struct tegra_i2c_hw_feature tegra30_i2c_hw = {
 	.setup_hold_time_std_mode = 0x0,
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
+	.has_gpc_dma = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
@@ -1324,6 +1323,7 @@ static const struct tegra_i2c_hw_feature tegra114_i2c_hw = {
 	.setup_hold_time_std_mode = 0x0,
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
+	.has_gpc_dma = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
@@ -1346,6 +1346,7 @@ static const struct tegra_i2c_hw_feature tegra124_i2c_hw = {
 	.setup_hold_time_std_mode = 0x0,
 	.setup_hold_time_fast_fast_plus_mode = 0x0,
 	.setup_hold_time_hs_mode = 0x0,
+	.has_gpc_dma = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
@@ -1368,6 +1369,7 @@ static const struct tegra_i2c_hw_feature tegra210_i2c_hw = {
 	.setup_hold_time_std_mode = 0,
 	.setup_hold_time_fast_fast_plus_mode = 0,
 	.setup_hold_time_hs_mode = 0,
+	.has_gpc_dma = false,
 };
 
 static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
@@ -1390,6 +1392,7 @@ static const struct tegra_i2c_hw_feature tegra186_i2c_hw = {
 	.setup_hold_time_std_mode = 0,
 	.setup_hold_time_fast_fast_plus_mode = 0,
 	.setup_hold_time_hs_mode = 0,
+	.has_gpc_dma = true,
 };
 
 static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
@@ -1412,6 +1415,7 @@ static const struct tegra_i2c_hw_feature tegra194_i2c_hw = {
 	.setup_hold_time_std_mode = 0x08080808,
 	.setup_hold_time_fast_fast_plus_mode = 0x02020202,
 	.setup_hold_time_hs_mode = 0x090909,
+	.has_gpc_dma = true,
 };
 
 /* Match table for of_platform binding */
@@ -1479,8 +1483,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		return PTR_ERR(i2c_dev->rst);
 	}
 
-	tegra_i2c_parse_dt(i2c_dev);
-
 	i2c_dev->hw = of_device_get_match_data(&pdev->dev);
 	i2c_dev->is_dvc = of_device_is_compatible(pdev->dev.of_node,
 						  "nvidia,tegra20-i2c-dvc");
@@ -1488,6 +1490,8 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	init_completion(&i2c_dev->dma_complete);
 	spin_lock_init(&i2c_dev->xfer_lock);
 
+	tegra_i2c_parse_dt(i2c_dev);
+
 	if (!i2c_dev->hw->has_single_clk_source) {
 		fast_clk = devm_clk_get(&pdev->dev, "fast-clk");
 		if (IS_ERR(fast_clk)) {
@@ -1543,7 +1547,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
-	ret = tegra_i2c_init_dma_param(i2c_dev);
+	ret = tegra_i2c_init_dma(i2c_dev);
 	if (ret == -EPROBE_DEFER)
 		goto disable_div_clk;
 
@@ -1611,18 +1615,11 @@ 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)
+	if (i2c_dev->has_dma) {
 		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;

Re: [PATCH V7 3/5] i2c: tegra: Add DMA Support

[Dmitry Osipenko]

31.01.2019 21:08, Dmitry Osipenko пишет:
> 31.01.2019 19:55, Dmitry Osipenko пишет:
> +config I2C_TEGRA_DMA_SUPPORT
> +	bool "NVIDIA Tegra internal I2C controller DMA support"
> +	depends on I2C_TEGRA
> +	depends on TEGRA20_APB_DMA && ARCH_TEGRA_2x_SOC
> +	depends on TEGRA20_APB_DMA && ARCH_TEGRA_3x_SOC
> +	depends on TEGRA20_APB_DMA && ARCH_TEGRA_114_SOC
> +	depends on TEGRA20_APB_DMA && ARCH_TEGRA_124_SOC
> +	depends on TEGRA20_APB_DMA && ARCH_TEGRA_210_SOC
> +	help
> +	  If you say yes to this option, DMA engine integration support will
> +	  be included for the I2C controller embedded in NVIDIA Tegra SOCs
> +

Actually, seems should be like that:

config I2C_TEGRA_DMA_SUPPORT
	bool "NVIDIA Tegra internal I2C controller DMA support"
	depends on I2C_TEGRA
	depends on (TEGRA20_APB_DMA && ARCH_TEGRA_2x_SOC) || \
		   (TEGRA20_APB_DMA && ARCH_TEGRA_3x_SOC) || \
		   (TEGRA20_APB_DMA && ARCH_TEGRA_114_SOC) || \
		   (TEGRA20_APB_DMA && ARCH_TEGRA_124_SOC) || \
		   (TEGRA20_APB_DMA && ARCH_TEGRA_210_SOC)
	help
	  If you say yes to this option, DMA engine integration support will
	  be included for the I2C controller embedded in NVIDIA Tegra SOCs 

But you got the idea.

[PATCH V7 4/5] i2c: tegra: Update transfer timeout

[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>
---
 [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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index b30b5da5ce6b..623bf4f275cd 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;
@@ -929,6 +930,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	u32 *buffer = 0;
 	int ret = 0;
 	bool dma = false;
+	u16 xfer_time = 100;
 
 	if (msg->flags & I2C_M_RD)
 		xfer_size = msg->len;
@@ -936,6 +938,9 @@ 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);
+	xfer_time += DIV_ROUND_CLOSEST((xfer_size * 9) * 1000,
+					i2c_dev->bus_clk_rate);
+
 	dma = (xfer_size > I2C_PIO_MODE_MAX_LEN);
 	if (dma) {
 		if ((msg->flags & I2C_M_RD) && !i2c_dev->rx_dma_chan)
@@ -1072,7 +1077,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
 	if (dma) {
 		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");
@@ -1095,7 +1100,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

Re: [PATCH V7 4/5] i2c: tegra: Update transfer timeout

[Dmitry Osipenko]

30.01.2019 19:01, 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>
> ---
>  [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 | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
> index b30b5da5ce6b..623bf4f275cd 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;
> @@ -929,6 +930,7 @@ static int tegra_i2c_xfer_msg(struct tegra_i2c_dev *i2c_dev,
>  	u32 *buffer = 0;
>  	int ret = 0;
>  	bool dma = false;
> +	u16 xfer_time = 100;
>  
>  	if (msg->flags & I2C_M_RD)
>  		xfer_size = msg->len;
> @@ -936,6 +938,9 @@ 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);
> +	xfer_time += DIV_ROUND_CLOSEST((xfer_size * 9) * 1000,
> +					i2c_dev->bus_clk_rate);

Could you please add a clarifying comment for the equation?

Shouldn't it be something like this?

xfer_time = (P clocks) * DIV_ROUND_UP(xfer_size, (N bytes per P clocks)) / (bus_clk_rate / 1000)

[PATCH V7 5/5] i2c: tegra: Add I2C interface timing support

[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>
---
 [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 | 192 ++++++++++++++++++++++++++++++++++++-----
 1 file changed, 169 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-tegra.c b/drivers/i2c/busses/i2c-tegra.c
index 623bf4f275cd..ad8eeac5a745 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;
 };
 
 /**
@@ -262,6 +292,7 @@ struct tegra_i2c_dev {
 };
 
 static struct dma_chan *chan;
+static bool first_init;
 
 static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
 		       unsigned long reg)
@@ -639,6 +670,49 @@ static int tegra_i2c_wait_for_config_load(struct tegra_i2c_dev *i2c_dev)
 	return 0;
 }
 
+static int tegra_i2c_set_timing(struct tegra_i2c_dev *i2c_dev)
+{
+	u32 val;
+	u32 tsu_thd = 0;
+	u8 tlow = 0;
+	u8 thigh = 0;
+	u32 clk_multiplier;
+	int err = 0;
+
+	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 (first_init) {
+		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);
+		else
+			first_init = false;
+	}
+
+	return err;
+}
+
 static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
 {
 	u32 val;
@@ -673,6 +747,10 @@ 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);
 
+	err = tegra_i2c_set_timing(i2c_dev);
+	if (err < 0)
+		goto err;
+
 	if (!i2c_dev->is_dvc) {
 		u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
 
@@ -1213,13 +1291,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 = {
@@ -1227,13 +1313,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 = {
@@ -1241,13 +1335,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 = {
@@ -1255,13 +1357,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 = {
@@ -1269,32 +1379,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, },
@@ -1315,7 +1464,6 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 	phys_addr_t phys_addr;
 	int irq;
 	int ret = 0;
-	int clk_multiplier = I2C_CLK_MULTIPLIER_STD_FAST_MODE;
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	phys_addr = res->start;
@@ -1385,20 +1533,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) {
@@ -1424,6 +1569,7 @@ static int tegra_i2c_probe(struct platform_device *pdev)
 		}
 	}
 
+	first_init = true;
 	ret = tegra_i2c_init(i2c_dev);
 	if (ret) {
 		dev_err(&pdev->dev, "Failed to initialize i2c controller\n");
-- 
2.7.4

Re: [PATCH V7 5/5] i2c: tegra: Add I2C interface timing support

[Dmitry Osipenko]

В Wed, 30 Jan 2019 08:01:36 -0800
Sowjanya Komatineni <skomatineni@nvidia.com> пишет:

> 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>
> ---
>  [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 | 192
> ++++++++++++++++++++++++++++++++++++----- 1 file changed, 169
> insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-tegra.c
> b/drivers/i2c/busses/i2c-tegra.c index 623bf4f275cd..ad8eeac5a745
> 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;
>  };
>  
>  /**
> @@ -262,6 +292,7 @@ struct tegra_i2c_dev {
>  };
>  
>  static struct dma_chan *chan;
> +static bool first_init;

Same as for the chan, please don't use global variables.

>  static void dvc_writel(struct tegra_i2c_dev *i2c_dev, u32 val,
>  		       unsigned long reg)
> @@ -639,6 +670,49 @@ static int tegra_i2c_wait_for_config_load(struct
> tegra_i2c_dev *i2c_dev) return 0;
>  }
>  
> +static int tegra_i2c_set_timing(struct tegra_i2c_dev *i2c_dev)
> +{
> +	u32 val;
> +	u32 tsu_thd = 0;
> +	u8 tlow = 0;
> +	u8 thigh = 0;
> +	u32 clk_multiplier;
> +	int err = 0;
> +
> +	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 (first_init) {

Just add first_init to the arguments of
tegra_i2c_init(i2c_dev, first_init) and pass it to here.

> +		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);
> +		else
> +			first_init = false;
> +	}
> +
> +	return err;
> +}
> +
>  static int tegra_i2c_init(struct tegra_i2c_dev *i2c_dev)
>  {
>  	u32 val;
> @@ -673,6 +747,10 @@ 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);
>  
> +	err = tegra_i2c_set_timing(i2c_dev);
> +	if (err < 0)
> +		goto err;
> +
>  	if (!i2c_dev->is_dvc) {
>  		u32 sl_cfg = i2c_readl(i2c_dev, I2C_SL_CNFG);
>  

[snip]

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

[Thierry Reding]

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

On Wed, Jan 30, 2019 at 08:01:32AM -0800, Sowjanya Komatineni wrote:
> This patch sorts all the include headers alphabetically for the
> I2C tegra driver

Nit: s/tegra/Tegra/ and place the full stop at the end of the sentence.
Otherwise:

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

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