All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
@ 2021-10-19  6:01 Vinod Koul
  2021-11-01 16:23 ` Bjorn Andersson
  2021-11-03  2:13 ` Alexey Minnekhanov
  0 siblings, 2 replies; 8+ messages in thread
From: Vinod Koul @ 2021-10-19  6:01 UTC (permalink / raw)
  To: Bjorn Andersson, Wolfram Sang
  Cc: linux-arm-msm, Vinod Koul, Andy Gross, Sumit Semwal,
	Douglas Anderson, Matthias Kaehlcke, linux-i2c, linux-kernel

This adds capability to use GSI DMA for I2C transfers

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
Changes since v3:
 - remove separate tx and rx function for gsi dma and make a common one
 - remove global structs and use local variables instead

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 6d635a7c104c..b783d85559f5 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c
@@ -3,7 +3,9 @@
 
 #include <linux/acpi.h>
 #include <linux/clk.h>
+#include <linux/dmaengine.h>
 #include <linux/dma-mapping.h>
+#include <linux/dma/qcom-gpi-dma.h>
 #include <linux/err.h>
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
@@ -48,6 +50,8 @@
 #define LOW_COUNTER_SHFT	10
 #define CYCLE_COUNTER_MSK	GENMASK(9, 0)
 
+#define I2C_PACK_EN		(BIT(0) | BIT(1))
+
 enum geni_i2c_err_code {
 	GP_IRQ0,
 	NACK,
@@ -72,6 +76,11 @@ enum geni_i2c_err_code {
 #define XFER_TIMEOUT		HZ
 #define RST_TIMEOUT		HZ
 
+enum i2c_se_mode {
+	I2C_FIFO_SE_DMA,
+	I2C_GPI_DMA,
+};
+
 struct geni_i2c_dev {
 	struct geni_se se;
 	u32 tx_wm;
@@ -89,6 +98,10 @@ struct geni_i2c_dev {
 	void *dma_buf;
 	size_t xfer_len;
 	dma_addr_t dma_addr;
+	struct dma_chan *tx_c;
+	struct dma_chan *rx_c;
+	bool cfg_sent;
+	enum i2c_se_mode se_mode;
 };
 
 struct geni_i2c_err_log {
@@ -456,6 +469,171 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
 	return gi2c->err;
 }
 
+static void i2c_gsi_cb_result(void *cb, const struct dmaengine_result *result)
+{
+	struct geni_i2c_dev *gi2c = cb;
+
+	if (result->result != DMA_TRANS_NOERROR) {
+		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
+		return;
+	}
+
+	if (result->residue)
+		dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue);
+
+	complete(&gi2c->done);
+}
+
+static void geni_i2c_gpi_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
+			       void *tx_buf, dma_addr_t tx_addr,
+			       void *rx_buf, dma_addr_t rx_addr)
+{
+	if (tx_buf) {
+		dma_unmap_single(gi2c->se.dev->parent, tx_addr, msg->len, DMA_TO_DEVICE);
+		i2c_put_dma_safe_msg_buf(tx_buf, msg, false);
+	}
+
+	if (rx_buf) {
+		dma_unmap_single(gi2c->se.dev->parent, rx_addr, msg->len, DMA_FROM_DEVICE);
+		i2c_put_dma_safe_msg_buf(rx_buf, msg, false);
+	}
+}
+
+static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
+			   struct dma_slave_config *config, dma_addr_t *dma_addr_p,
+			   void **buf, unsigned int op, struct dma_chan *dma_chan)
+{
+	struct gpi_i2c_config *peripheral;
+	unsigned int flags;
+	void *dma_buf = &buf;
+	dma_addr_t addr;
+	enum dma_data_direction map_dirn;
+	enum dma_transfer_direction dma_dirn;
+	struct dma_async_tx_descriptor *desc;
+	int ret;
+
+	peripheral = config->peripheral_config;
+
+	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+	if (!dma_buf)
+		return -ENOMEM;
+
+	if (op == I2C_WRITE)
+		map_dirn = DMA_TO_DEVICE;
+	else
+		map_dirn = DMA_FROM_DEVICE;
+
+	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
+	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
+		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+		return -ENOMEM;
+	}
+
+	peripheral->rx_len = msg->len;
+	peripheral->op = op;
+
+	ret = dmaengine_slave_config(dma_chan, config);
+	if (ret) {
+		dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
+		goto err_config;
+	}
+
+	peripheral->set_config =  false;
+	peripheral->multi_msg = true;
+	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+
+	if (op == I2C_WRITE)
+		dma_dirn = DMA_MEM_TO_DEV;
+	else
+		dma_dirn = DMA_DEV_TO_MEM;
+
+	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
+	if (!desc) {
+		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
+		ret = -EIO;
+		goto err_config;
+	}
+
+	desc->callback_result = i2c_gsi_cb_result;
+	desc->callback_param = gi2c;
+
+	dmaengine_submit(desc);
+	*dma_addr_p = addr;
+
+	return 0;
+
+err_config:
+	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
+	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+	return ret;
+}
+
+static int geni_i2c_gsi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int num)
+{
+	struct dma_slave_config config = {};
+	struct gpi_i2c_config peripheral = {};
+	int i, ret = 0, timeout, stretch;
+	dma_addr_t tx_addr, rx_addr;
+	void *tx_buf = NULL, *rx_buf = NULL;
+
+	config.peripheral_config = &peripheral;
+	config.peripheral_size = sizeof(peripheral);
+
+	if (!gi2c->cfg_sent) {
+		const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
+
+		gi2c->cfg_sent = true;
+		peripheral.pack_enable = I2C_PACK_EN;
+		peripheral.cycle_count = itr->t_cycle_cnt;
+		peripheral.high_count = itr->t_high_cnt;
+		peripheral.low_count = itr->t_low_cnt;
+		peripheral.clk_div = itr->clk_div;
+		peripheral.set_config =  true;
+	}
+	peripheral.multi_msg = false;
+
+	for (i = 0; i < num; i++) {
+		gi2c->cur = &msgs[i];
+		dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
+
+		stretch = (i < (num - 1));
+		peripheral.addr = msgs[i].addr;
+		peripheral.stretch = stretch;
+
+		if (msgs[i].flags & I2C_M_RD) {
+			ret =  geni_i2c_gpi(gi2c, &msgs[i], &config, &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
+			if (ret)
+				goto err;
+		}
+
+		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config, &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);
+		if (ret)
+			goto err;
+
+		if (msgs[i].flags & I2C_M_RD)
+			dma_async_issue_pending(gi2c->rx_c);
+		dma_async_issue_pending(gi2c->tx_c);
+
+		timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);
+		if (!timeout) {
+			dev_err(gi2c->se.dev, "I2C timeout gsi flags:%d addr:0x%x\n",
+				gi2c->cur->flags, gi2c->cur->addr);
+			gi2c->err = -ETIMEDOUT;
+			goto err;
+		}
+
+		geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
+	}
+
+	return 0;
+
+err:
+	dmaengine_terminate_sync(gi2c->rx_c);
+	dmaengine_terminate_sync(gi2c->tx_c);
+	geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
+	return ret;
+}
+
 static int geni_i2c_xfer(struct i2c_adapter *adap,
 			 struct i2c_msg msgs[],
 			 int num)
@@ -475,6 +653,12 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 	}
 
 	qcom_geni_i2c_conf(gi2c);
+
+	if (gi2c->se_mode == I2C_GPI_DMA) {
+		ret = geni_i2c_gsi_xfer(gi2c, msgs, num);
+		goto geni_i2c_txn_ret;
+	}
+
 	for (i = 0; i < num; i++) {
 		u32 m_param = i < (num - 1) ? STOP_STRETCH : 0;
 
@@ -489,6 +673,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 		if (ret)
 			break;
 	}
+geni_i2c_txn_ret:
 	if (ret == 0)
 		ret = num;
 
@@ -517,11 +702,49 @@ static const struct acpi_device_id geni_i2c_acpi_match[] = {
 MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
 #endif
 
+static void release_gpi_dma(struct geni_i2c_dev *gi2c)
+{
+	if (gi2c->rx_c) {
+		dma_release_channel(gi2c->rx_c);
+		gi2c->rx_c = NULL;
+	}
+	if (gi2c->tx_c) {
+		dma_release_channel(gi2c->tx_c);
+		gi2c->tx_c = NULL;
+	}
+}
+
+static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
+{
+	int ret;
+
+	geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);
+	gi2c->tx_c = dma_request_chan(gi2c->se.dev, "tx");
+	ret = dev_err_probe(gi2c->se.dev, IS_ERR(gi2c->tx_c), "Failed to get tx DMA ch\n");
+	if (ret < 0)
+		goto err_tx;
+
+	gi2c->rx_c = dma_request_chan(gi2c->se.dev, "rx");
+	ret = dev_err_probe(gi2c->se.dev, IS_ERR(gi2c->rx_c), "Failed to get rx DMA ch\n");
+	if (ret < 0)
+		goto err_rx;
+
+	dev_dbg(gi2c->se.dev, "Grabbed GPI dma channels\n");
+	return 0;
+
+err_rx:
+	dma_release_channel(gi2c->tx_c);
+	gi2c->tx_c = NULL;
+err_tx:
+	gi2c->rx_c = NULL;
+	return ret;
+}
+
 static int geni_i2c_probe(struct platform_device *pdev)
 {
 	struct geni_i2c_dev *gi2c;
 	struct resource *res;
-	u32 proto, tx_depth;
+	u32 proto, tx_depth, fifo_disable;
 	int ret;
 	struct device *dev = &pdev->dev;
 
@@ -601,27 +824,52 @@ static int geni_i2c_probe(struct platform_device *pdev)
 		return ret;
 	}
 	proto = geni_se_read_proto(&gi2c->se);
-	tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
 	if (proto != GENI_SE_I2C) {
 		dev_err(dev, "Invalid proto %d\n", proto);
 		geni_se_resources_off(&gi2c->se);
 		return -ENXIO;
 	}
-	gi2c->tx_wm = tx_depth - 1;
-	geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
-	geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, PACKING_BYTES_PW,
-							true, true, true);
+
+	fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
+
+	switch (fifo_disable) {
+	case 1:
+		ret = setup_gpi_dma(gi2c);
+		if (!ret) { /* success case */
+			gi2c->se_mode = I2C_GPI_DMA;
+			geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);
+			dev_dbg(dev, "Using GPI DMA mode for I2C\n");
+			break;
+		}
+		/*
+		 * in case of failure to get dma channel, we can till do the
+		 * FIFO mode, so fallthrough
+		 */
+		dev_warn(dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n");
+		fallthrough;
+
+	case 0:
+		gi2c->se_mode = I2C_FIFO_SE_DMA;
+		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
+		gi2c->tx_wm = tx_depth - 1;
+		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
+		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
+				       PACKING_BYTES_PW, true, true, true);
+
+		dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
+
+		break;
+	}
+
 	ret = geni_se_resources_off(&gi2c->se);
 	if (ret) {
 		dev_err(dev, "Error turning off resources %d\n", ret);
-		return ret;
+		goto err_dma;
 	}
 
 	ret = geni_icc_disable(&gi2c->se);
 	if (ret)
-		return ret;
-
-	dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
+		goto err_dma;
 
 	gi2c->suspended = 1;
 	pm_runtime_set_suspended(gi2c->se.dev);
@@ -633,18 +881,23 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (ret) {
 		dev_err(dev, "Error adding i2c adapter %d\n", ret);
 		pm_runtime_disable(gi2c->se.dev);
-		return ret;
+		goto err_dma;
 	}
 
 	dev_dbg(dev, "Geni-I2C adaptor successfully added\n");
 
 	return 0;
+
+err_dma:
+	release_gpi_dma(gi2c);
+	return ret;
 }
 
 static int geni_i2c_remove(struct platform_device *pdev)
 {
 	struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
 
+	release_gpi_dma(gi2c);
 	i2c_del_adapter(&gi2c->adap);
 	pm_runtime_disable(gi2c->se.dev);
 	return 0;
-- 
2.31.1


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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
  2021-10-19  6:01 [PATCH v4] i2c: qcom-geni: Add support for GPI DMA Vinod Koul
@ 2021-11-01 16:23 ` Bjorn Andersson
  2021-11-03  2:13 ` Alexey Minnekhanov
  1 sibling, 0 replies; 8+ messages in thread
From: Bjorn Andersson @ 2021-11-01 16:23 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Wolfram Sang, linux-arm-msm, Andy Gross, Sumit Semwal,
	Douglas Anderson, Matthias Kaehlcke, linux-i2c, linux-kernel

On Mon 18 Oct 23:01 PDT 2021, Vinod Koul wrote:

> This adds capability to use GSI DMA for I2C transfers

How about expanding this a little bit? Mention that the driver already
support using the serial engine's own DMA mode and that the patch
extends this to use the wrapper's GSI DMA controller - and what benefit
that brings.

Don't assume that readers of this commit message has read the message in
the wrapper, GPI driver or SPI patches.

> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> Changes since v3:
>  - remove separate tx and rx function for gsi dma and make a common one
>  - remove global structs and use local variables instead
> 
> diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
> index 6d635a7c104c..b783d85559f5 100644
> --- a/drivers/i2c/busses/i2c-qcom-geni.c
> +++ b/drivers/i2c/busses/i2c-qcom-geni.c
> @@ -3,7 +3,9 @@
>  
>  #include <linux/acpi.h>
>  #include <linux/clk.h>
> +#include <linux/dmaengine.h>
>  #include <linux/dma-mapping.h>
> +#include <linux/dma/qcom-gpi-dma.h>
>  #include <linux/err.h>
>  #include <linux/i2c.h>
>  #include <linux/interrupt.h>
> @@ -48,6 +50,8 @@
>  #define LOW_COUNTER_SHFT	10
>  #define CYCLE_COUNTER_MSK	GENMASK(9, 0)
>  
> +#define I2C_PACK_EN		(BIT(0) | BIT(1))

How about I2C_PACK_TX and I2C_PACK_RX to document the two bits? Also
this seems to be better placed in qcom-gpi-dma.h, next to struct
qpi_i2c_config.

> +
>  enum geni_i2c_err_code {
>  	GP_IRQ0,
>  	NACK,
> @@ -72,6 +76,11 @@ enum geni_i2c_err_code {
>  #define XFER_TIMEOUT		HZ
>  #define RST_TIMEOUT		HZ
>  
> +enum i2c_se_mode {
> +	I2C_FIFO_SE_DMA,
> +	I2C_GPI_DMA,

These names are rather generic and if they only denote fifo/se-dma vs
gpi, perhaps a "bool gpi_mode" would be cleaner?

> +};
> +
>  struct geni_i2c_dev {
>  	struct geni_se se;
>  	u32 tx_wm;
> @@ -89,6 +98,10 @@ struct geni_i2c_dev {
>  	void *dma_buf;
>  	size_t xfer_len;
>  	dma_addr_t dma_addr;
> +	struct dma_chan *tx_c;
> +	struct dma_chan *rx_c;
> +	bool cfg_sent;

This is only used in gpi mode, so it would be nice to clarify this by
naming it "gpi_cfg_sent".

> +	enum i2c_se_mode se_mode;
>  };
>  
>  struct geni_i2c_err_log {
> @@ -456,6 +469,171 @@ static int geni_i2c_tx_one_msg(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
>  	return gi2c->err;
>  }
>  
> +static void i2c_gsi_cb_result(void *cb, const struct dmaengine_result *result)
> +{
> +	struct geni_i2c_dev *gi2c = cb;
> +
> +	if (result->result != DMA_TRANS_NOERROR) {
> +		dev_err(gi2c->se.dev, "DMA txn failed:%d\n", result->result);
> +		return;
> +	}
> +
> +	if (result->residue)
> +		dev_dbg(gi2c->se.dev, "DMA xfer has pending: %d\n", result->residue);
> +
> +	complete(&gi2c->done);
> +}
> +
> +static void geni_i2c_gpi_unmap(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> +			       void *tx_buf, dma_addr_t tx_addr,
> +			       void *rx_buf, dma_addr_t rx_addr)
> +{
> +	if (tx_buf) {
> +		dma_unmap_single(gi2c->se.dev->parent, tx_addr, msg->len, DMA_TO_DEVICE);
> +		i2c_put_dma_safe_msg_buf(tx_buf, msg, false);
> +	}
> +
> +	if (rx_buf) {
> +		dma_unmap_single(gi2c->se.dev->parent, rx_addr, msg->len, DMA_FROM_DEVICE);
> +		i2c_put_dma_safe_msg_buf(rx_buf, msg, false);
> +	}
> +}
> +
> +static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
> +			   struct dma_slave_config *config, dma_addr_t *dma_addr_p,
> +			   void **buf, unsigned int op, struct dma_chan *dma_chan)
> +{
> +	struct gpi_i2c_config *peripheral;
> +	unsigned int flags;
> +	void *dma_buf = &buf;
> +	dma_addr_t addr;
> +	enum dma_data_direction map_dirn;
> +	enum dma_transfer_direction dma_dirn;
> +	struct dma_async_tx_descriptor *desc;
> +	int ret;
> +
> +	peripheral = config->peripheral_config;
> +
> +	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
> +	if (!dma_buf)
> +		return -ENOMEM;
> +
> +	if (op == I2C_WRITE)
> +		map_dirn = DMA_TO_DEVICE;
> +	else
> +		map_dirn = DMA_FROM_DEVICE;
> +
> +	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
> +	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
> +		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> +		return -ENOMEM;
> +	}
> +
> +	peripheral->rx_len = msg->len;

Why is this member named rx_len when it might contain the length of the
tx transaction? Is the name or usage incorrect?

> +	peripheral->op = op;
> +
> +	ret = dmaengine_slave_config(dma_chan, config);
> +	if (ret) {
> +		dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
> +		goto err_config;
> +	}
> +
> +	peripheral->set_config =  false;
> +	peripheral->multi_msg = true;

I think the split of updates to "peripheral" between geni_i2c_gpi() and
geni_i2c_gsi_xfer() makes the two parts harder to reason about.


Kept in the other function this would simply be "commit the config,
set up the buffers and the dmaengine".

That said, I do recognize the slight duplication that needs to happen
wrt set_config and multi_msg in the case of I2C_READ...


And set_config and multi_msg are u8, so please use 0 and 1.

> +	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +
> +	if (op == I2C_WRITE)
> +		dma_dirn = DMA_MEM_TO_DEV;
> +	else
> +		dma_dirn = DMA_DEV_TO_MEM;
> +
> +	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
> +	if (!desc) {
> +		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
> +		ret = -EIO;
> +		goto err_config;
> +	}
> +
> +	desc->callback_result = i2c_gsi_cb_result;
> +	desc->callback_param = gi2c;
> +
> +	dmaengine_submit(desc);
> +	*dma_addr_p = addr;
> +
> +	return 0;
> +
> +err_config:
> +	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
> +	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
> +	return ret;
> +}
> +
> +static int geni_i2c_gsi_xfer(struct geni_i2c_dev *gi2c, struct i2c_msg msgs[], int num)
> +{
> +	struct dma_slave_config config = {};
> +	struct gpi_i2c_config peripheral = {};
> +	int i, ret = 0, timeout, stretch;
> +	dma_addr_t tx_addr, rx_addr;
> +	void *tx_buf = NULL, *rx_buf = NULL;
> +
> +	config.peripheral_config = &peripheral;
> +	config.peripheral_size = sizeof(peripheral);
> +
> +	if (!gi2c->cfg_sent) {

For non-GPI mode we do this for every transaction, but you do it once
per controller. Is this information retained over a power collapse?

Should cfg_sent be reset in the suspend path?

> +		const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
> +
> +		gi2c->cfg_sent = true;
> +		peripheral.pack_enable = I2C_PACK_EN;
> +		peripheral.cycle_count = itr->t_cycle_cnt;
> +		peripheral.high_count = itr->t_high_cnt;
> +		peripheral.low_count = itr->t_low_cnt;
> +		peripheral.clk_div = itr->clk_div;
> +		peripheral.set_config =  true;

set_config is a u8, so perhaps better to just say 1 here?

> +	}
> +	peripheral.multi_msg = false;
> +
> +	for (i = 0; i < num; i++) {
> +		gi2c->cur = &msgs[i];
> +		dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
> +
> +		stretch = (i < (num - 1));

No need for the parenthesis here, nor for a local variable.

Given that peripheral.stretch is zero initialized I think it would be
even clearer to just use a explicit if stmt.

> +		peripheral.addr = msgs[i].addr;
> +		peripheral.stretch = stretch;
> +

		if (i < num - 1)
			peripheral.stretch = 1;

> +		if (msgs[i].flags & I2C_M_RD) {
> +			ret =  geni_i2c_gpi(gi2c, &msgs[i], &config, &rx_addr, &rx_buf, I2C_READ, gi2c->rx_c);
> +			if (ret)
> +				goto err;
> +		}
> +
> +		ret =  geni_i2c_gpi(gi2c, &msgs[i], &config, &tx_addr, &tx_buf, I2C_WRITE, gi2c->tx_c);

I have a feeling that I asked this previously, but I can find the
conversation at this point.

If the client does:

struct i2c_msg msg[2] = {
	{
		.addr = 42,
		.len = 1,
		.buf = &some_register,
	},
	{
		.addr = 42,
		.flags = I2C_M_RD,
		.len = 2,
		.buf = &buf,
	},
};
i2c_transfer(geni, msg, 2);


The first iteration through this loop would copy 1 byte of some_register
into tx_buf and kick off a transaction. Once the reply comes the second
iteration will allocate a 2 byte rx_buf and 2 bytes of tx_buf.

Why do we need a tx_buf and tx transfer of the same length as the
amount of data we intend to read?


If this is some magical requirement of the hardware, I think this
warrants a comment.

> +		if (ret)
> +			goto err;
> +
> +		if (msgs[i].flags & I2C_M_RD)
> +			dma_async_issue_pending(gi2c->rx_c);
> +		dma_async_issue_pending(gi2c->tx_c);
> +
> +		timeout = wait_for_completion_timeout(&gi2c->done, XFER_TIMEOUT);

Why are you waiting for done of each message? Can't you queue up all the
transfer requests and let the DMA engine tell you when they are all
done?

If not, then why is this function "replacing" geni_i2c_xfer() and not
geni_i2c_[rt]x_one_msg()?

> +		if (!timeout) {
> +			dev_err(gi2c->se.dev, "I2C timeout gsi flags:%d addr:0x%x\n",
> +				gi2c->cur->flags, gi2c->cur->addr);
> +			gi2c->err = -ETIMEDOUT;

I think you mean ret = -ETIMEDOUT here?

> +			goto err;
> +		}
> +
> +		geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
> +	}
> +
> +	return 0;
> +
> +err:
> +	dmaengine_terminate_sync(gi2c->rx_c);
> +	dmaengine_terminate_sync(gi2c->tx_c);
> +	geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
> +	return ret;
> +}
> +
>  static int geni_i2c_xfer(struct i2c_adapter *adap,
>  			 struct i2c_msg msgs[],
>  			 int num)
> @@ -475,6 +653,12 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  	}
>  
>  	qcom_geni_i2c_conf(gi2c);
> +
> +	if (gi2c->se_mode == I2C_GPI_DMA) {
> +		ret = geni_i2c_gsi_xfer(gi2c, msgs, num);
> +		goto geni_i2c_txn_ret;

I don't think this is a good use of a goto.

If you need to keep the dma transfers doing one message at a time, then
I think you should move this "override" into the loop and have a
geni_i2c_gpi_one_msg() function.


If it's possible to queue up the entire msgs at once I think you should
break out the loop below to a separate function, so that you can simply
do:

if (gpi_mode)
  ret = do_gpi();
else
  ret = do_fifo();

In this case you could replace the break in the loop below and just
return ret; and end the function with return num instead of the ret == 0
check...

> +	}
> +
>  	for (i = 0; i < num; i++) {
>  		u32 m_param = i < (num - 1) ? STOP_STRETCH : 0;
>  
> @@ -489,6 +673,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
>  		if (ret)
>  			break;
>  	}
> +geni_i2c_txn_ret:
>  	if (ret == 0)
>  		ret = num;
>  
> @@ -517,11 +702,49 @@ static const struct acpi_device_id geni_i2c_acpi_match[] = {
>  MODULE_DEVICE_TABLE(acpi, geni_i2c_acpi_match);
>  #endif
>  
> +static void release_gpi_dma(struct geni_i2c_dev *gi2c)
> +{
> +	if (gi2c->rx_c) {
> +		dma_release_channel(gi2c->rx_c);
> +		gi2c->rx_c = NULL;

What purpose does this assignment have?

> +	}
> +	if (gi2c->tx_c) {
> +		dma_release_channel(gi2c->tx_c);
> +		gi2c->tx_c = NULL;
> +	}
> +}
> +
> +static int setup_gpi_dma(struct geni_i2c_dev *gi2c)
> +{
> +	int ret;
> +
> +	geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);
> +	gi2c->tx_c = dma_request_chan(gi2c->se.dev, "tx");
> +	ret = dev_err_probe(gi2c->se.dev, IS_ERR(gi2c->tx_c), "Failed to get tx DMA ch\n");

Invoking dev_err_probe(, 0, ); will still result in a dev_dbg() so I
don't think you should do this outside a conditional.

> +	if (ret < 0)
> +		goto err_tx;
> +
> +	gi2c->rx_c = dma_request_chan(gi2c->se.dev, "rx");
> +	ret = dev_err_probe(gi2c->se.dev, IS_ERR(gi2c->rx_c), "Failed to get rx DMA ch\n");
> +	if (ret < 0)
> +		goto err_rx;
> +
> +	dev_dbg(gi2c->se.dev, "Grabbed GPI dma channels\n");
> +	return 0;
> +
> +err_rx:
> +	dma_release_channel(gi2c->tx_c);

Is there a reason why there's no devm_dma_request_chan()?

> +	gi2c->tx_c = NULL;

Afaict the only reason for clearing these is so that you can check for
NULL in release_gpi_dma(). Seems this need would go away if you had a
devm helper for dma_request_chan().

That said, release_gpi_dma() is called either:

1) on probe failure, in which case I think you should make sure to
properly handle the IS_ERR() case and not call release_gpi_dma().

2) on release, in which case I think you should either have left probe
with valid pointers or you're not in the GPI case and it would have been
better if you didn't touch the members in gi2c. I.e. use local variables
here.

> +err_tx:
> +	gi2c->rx_c = NULL;
> +	return ret;
> +}
> +
>  static int geni_i2c_probe(struct platform_device *pdev)
>  {
>  	struct geni_i2c_dev *gi2c;
>  	struct resource *res;
> -	u32 proto, tx_depth;
> +	u32 proto, tx_depth, fifo_disable;

Given that you mask before the assignment fifo_disable could be bool
instead.

>  	int ret;
>  	struct device *dev = &pdev->dev;
>  
> @@ -601,27 +824,52 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  		return ret;
>  	}
>  	proto = geni_se_read_proto(&gi2c->se);
> -	tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
>  	if (proto != GENI_SE_I2C) {
>  		dev_err(dev, "Invalid proto %d\n", proto);
>  		geni_se_resources_off(&gi2c->se);
>  		return -ENXIO;
>  	}
> -	gi2c->tx_wm = tx_depth - 1;
> -	geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> -	geni_se_config_packing(&gi2c->se, BITS_PER_BYTE, PACKING_BYTES_PW,
> -							true, true, true);
> +
> +	fifo_disable = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +
> +	switch (fifo_disable) {

The use of a switch statement here looks fishy, but the suspicion made
me stare at this long enough to figure out what you're actually doing
(fallback to FIFO_SE_DMA if setup_gpi_dma() fails).

Wouldn't it be cleaner to write this:

if (fifo_disable && !setup_gpio_dma(gi2c)) {
	se_mode = I2C_GPI_DMA;
	...;
} else {
	se_mode = I2C_FIFO_SE_DMA;
	...
}


That said, I don't see that you (or my proposal) account for
EPROBE_DEFER being returned from dma_request_chan()..

> +	case 1:
> +		ret = setup_gpi_dma(gi2c);
> +		if (!ret) { /* success case */
> +			gi2c->se_mode = I2C_GPI_DMA;
> +			geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);

Isn't this done in setup_gpi_dma() as well?

> +			dev_dbg(dev, "Using GPI DMA mode for I2C\n");
> +			break;
> +		}
> +		/*
> +		 * in case of failure to get dma channel, we can till do the
> +		 * FIFO mode, so fallthrough

Can we really do FIFO if FIFO_IF_DISABLE?

> +		 */
> +		dev_warn(dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n");
> +		fallthrough;
> +
> +	case 0:
> +		gi2c->se_mode = I2C_FIFO_SE_DMA;
> +		tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);
> +		gi2c->tx_wm = tx_depth - 1;
> +		geni_se_init(&gi2c->se, gi2c->tx_wm, tx_depth);
> +		geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
> +				       PACKING_BYTES_PW, true, true, true);
> +
> +		dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> +
> +		break;
> +	}
> +
>  	ret = geni_se_resources_off(&gi2c->se);
>  	if (ret) {
>  		dev_err(dev, "Error turning off resources %d\n", ret);
> -		return ret;
> +		goto err_dma;
>  	}
>  
>  	ret = geni_icc_disable(&gi2c->se);
>  	if (ret)
> -		return ret;
> -
> -	dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
> +		goto err_dma;
>  
>  	gi2c->suspended = 1;
>  	pm_runtime_set_suspended(gi2c->se.dev);
> @@ -633,18 +881,23 @@ static int geni_i2c_probe(struct platform_device *pdev)
>  	if (ret) {
>  		dev_err(dev, "Error adding i2c adapter %d\n", ret);
>  		pm_runtime_disable(gi2c->se.dev);
> -		return ret;
> +		goto err_dma;
>  	}
>  
>  	dev_dbg(dev, "Geni-I2C adaptor successfully added\n");
>  
>  	return 0;
> +
> +err_dma:
> +	release_gpi_dma(gi2c);
> +	return ret;

A devres dma_request_chan() seems like a really nice thing?

Regards,
Bjorn

>  }
>  
>  static int geni_i2c_remove(struct platform_device *pdev)
>  {
>  	struct geni_i2c_dev *gi2c = platform_get_drvdata(pdev);
>  
> +	release_gpi_dma(gi2c);
>  	i2c_del_adapter(&gi2c->adap);
>  	pm_runtime_disable(gi2c->se.dev);
>  	return 0;
> -- 
> 2.31.1
> 

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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
  2021-10-19  6:01 [PATCH v4] i2c: qcom-geni: Add support for GPI DMA Vinod Koul
  2021-11-01 16:23 ` Bjorn Andersson
@ 2021-11-03  2:13 ` Alexey Minnekhanov
  2021-11-03  5:21   ` Vinod Koul
  1 sibling, 1 reply; 8+ messages in thread
From: Alexey Minnekhanov @ 2021-11-03  2:13 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Andersson, Wolfram Sang
  Cc: linux-arm-msm, Andy Gross, Sumit Semwal, Douglas Anderson,
	Matthias Kaehlcke, linux-i2c

19.10.2021 09:01, Vinod Koul wrote:
> This adds capability to use GSI DMA for I2C transfers
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
> Changes since v3:
>   - remove separate tx and rx function for gsi dma and make a common one
>   - remove global structs and use local variables instead
> 

Hi, I've tried this patch on sm8150 board and I'm seeing weird things:

[    0.428829] gpi c00000.dma-controller: Adding to iommu group 0
[    0.480453] geni_se_qup cc0000.geniqup: Adding to iommu group 1
[    0.484019] geni_i2c c80000.i2c: Bus frequency not specified, default 
to 100kHz.
[    0.487172] geni_i2c c80000.i2c: error 0000000000000000: Failed to 
get tx DMA ch
[    0.495499] geni_i2c c80000.i2c: error 0000000000000000: Failed to 
get rx DMA ch
[    0.499842] geni_i2c c80000.i2c: Grabbed GPI dma channels
[    0.504784] geni_i2c c80000.i2c: Using GPI DMA mode for I2C
[    0.510812] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added

It seems weird to me that it "failed to get DMA channels", but then says 
anyway "Grabbed GPI DMA channels" and "Using GPI DMA mode for I2C".

What I did in dts is basically include sm8150.dtsi and do

&gpi_dma2 {
	status = "okay";
};

&i2c17 {
	status = "okay";
	dmas = <&gpi_dma2 0 0 QCOM_GPI_I2C>,
		<&gpi_dma2 1 0 QCOM_GPI_I2C>;
	dma-names = "tx", "rx";
	/* empty - no clients */
}

Another observation, even if I comment out "dmas" and "dma-names" in 
i2c17 node, I get

[    0.487037] geni_i2c c80000.i2c: error (____ptrval____): Failed to 
get tx DMA ch
[    0.490077] geni_i2c c80000.i2c: error (____ptrval____): Failed to 
get rx DMA ch
[    0.493077] geni_i2c c80000.i2c: Grabbed GPI dma channels
[    0.496061] geni_i2c c80000.i2c: Using GPI DMA mode for I2C
[    0.500155] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added

-- 
Regards
Alexey Minnekhanov

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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
  2021-11-03  2:13 ` Alexey Minnekhanov
@ 2021-11-03  5:21   ` Vinod Koul
  2021-11-04  2:44     ` Alexey Minnekhanov
  0 siblings, 1 reply; 8+ messages in thread
From: Vinod Koul @ 2021-11-03  5:21 UTC (permalink / raw)
  To: Alexey Minnekhanov
  Cc: Bjorn Andersson, Wolfram Sang, linux-arm-msm, Andy Gross,
	Sumit Semwal, Douglas Anderson, Matthias Kaehlcke, linux-i2c

Hello Alexey,

On 03-11-21, 05:13, Alexey Minnekhanov wrote:
> 19.10.2021 09:01, Vinod Koul wrote:
> > This adds capability to use GSI DMA for I2C transfers
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> > Changes since v3:
> >   - remove separate tx and rx function for gsi dma and make a common one
> >   - remove global structs and use local variables instead
> > 
> 
> Hi, I've tried this patch on sm8150 board and I'm seeing weird things:
> 
> [    0.428829] gpi c00000.dma-controller: Adding to iommu group 0
> [    0.480453] geni_se_qup cc0000.geniqup: Adding to iommu group 1
> [    0.484019] geni_i2c c80000.i2c: Bus frequency not specified, default to
> 100kHz.
> [    0.487172] geni_i2c c80000.i2c: error 0000000000000000: Failed to get tx
> DMA ch
> [    0.495499] geni_i2c c80000.i2c: error 0000000000000000: Failed to get rx
> DMA ch
> [    0.499842] geni_i2c c80000.i2c: Grabbed GPI dma channels
> [    0.504784] geni_i2c c80000.i2c: Using GPI DMA mode for I2C
> [    0.510812] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added
> 
> It seems weird to me that it "failed to get DMA channels", but then says
> anyway "Grabbed GPI DMA channels" and "Using GPI DMA mode for I2C".
> 
> What I did in dts is basically include sm8150.dtsi and do
> 
> &gpi_dma2 {
> 	status = "okay";
> };
> 
> &i2c17 {
> 	status = "okay";
> 	dmas = <&gpi_dma2 0 0 QCOM_GPI_I2C>,
> 		<&gpi_dma2 1 0 QCOM_GPI_I2C>;
> 	dma-names = "tx", "rx";
> 	/* empty - no clients */
> }
> 
> Another observation, even if I comment out "dmas" and "dma-names" in i2c17
> node, I get
> 
> [    0.487037] geni_i2c c80000.i2c: error (____ptrval____): Failed to get tx
> DMA ch
> [    0.490077] geni_i2c c80000.i2c: error (____ptrval____): Failed to get rx
> DMA ch
> [    0.493077] geni_i2c c80000.i2c: Grabbed GPI dma channels
> [    0.496061] geni_i2c c80000.i2c: Using GPI DMA mode for I2C
> [    0.500155] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added

This does not look correct. In absence of dmas property, the channel
allocation should not have gone ahead and you should not see " Grabbed
GPI dma channels" message!

Btw what is the output of below when dmas was commented out:
cat /sys/kernel/debug/dmaengine/summary

This would show the channels allocated to each controller

-- 
~Vinod

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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
  2021-11-03  5:21   ` Vinod Koul
@ 2021-11-04  2:44     ` Alexey Minnekhanov
  0 siblings, 0 replies; 8+ messages in thread
From: Alexey Minnekhanov @ 2021-11-04  2:44 UTC (permalink / raw)
  To: Vinod Koul
  Cc: Bjorn Andersson, Wolfram Sang, linux-arm-msm, Andy Gross,
	Sumit Semwal, Douglas Anderson, Matthias Kaehlcke, linux-i2c

03.11.2021 08:21, Vinod Koul пишет:
> Hello Alexey,
> 
> On 03-11-21, 05:13, Alexey Minnekhanov wrote:
>> 19.10.2021 09:01, Vinod Koul wrote:
>>> This adds capability to use GSI DMA for I2C transfers
>>>
>>> Signed-off-by: Vinod Koul <vkoul@kernel.org>
>>> ---
>>> Changes since v3:
>>>    - remove separate tx and rx function for gsi dma and make a common one
>>>    - remove global structs and use local variables instead
>>>
>>
>> Hi, I've tried this patch on sm8150 board and I'm seeing weird things:
>>
>> [    0.428829] gpi c00000.dma-controller: Adding to iommu group 0
>> [    0.480453] geni_se_qup cc0000.geniqup: Adding to iommu group 1
>> [    0.484019] geni_i2c c80000.i2c: Bus frequency not specified, default to
>> 100kHz.
>> [    0.487172] geni_i2c c80000.i2c: error 0000000000000000: Failed to get tx
>> DMA ch
>> [    0.495499] geni_i2c c80000.i2c: error 0000000000000000: Failed to get rx
>> DMA ch
>> [    0.499842] geni_i2c c80000.i2c: Grabbed GPI dma channels
>> [    0.504784] geni_i2c c80000.i2c: Using GPI DMA mode for I2C
>> [    0.510812] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added
>>
>> It seems weird to me that it "failed to get DMA channels", but then says
>> anyway "Grabbed GPI DMA channels" and "Using GPI DMA mode for I2C".
>>
>> What I did in dts is basically include sm8150.dtsi and do
>>
>> &gpi_dma2 {
>> 	status = "okay";
>> };
>>
>> &i2c17 {
>> 	status = "okay";
>> 	dmas = <&gpi_dma2 0 0 QCOM_GPI_I2C>,
>> 		<&gpi_dma2 1 0 QCOM_GPI_I2C>;
>> 	dma-names = "tx", "rx";
>> 	/* empty - no clients */
>> }
>>
>> Another observation, even if I comment out "dmas" and "dma-names" in i2c17
>> node, I get
>>
>> [    0.487037] geni_i2c c80000.i2c: error (____ptrval____): Failed to get tx
>> DMA ch
>> [    0.490077] geni_i2c c80000.i2c: error (____ptrval____): Failed to get rx
>> DMA ch
>> [    0.493077] geni_i2c c80000.i2c: Grabbed GPI dma channels
>> [    0.496061] geni_i2c c80000.i2c: Using GPI DMA mode for I2C
>> [    0.500155] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added
> 
> This does not look correct. In absence of dmas property, the channel
> allocation should not have gone ahead and you should not see " Grabbed
> GPI dma channels" message!
> 

If I change these lines in setup_gpi_dma()

  ret = dev_err_probe(gi2c->se.dev, IS_ERR(gi2c->tx_c), "Failed to get 
tx DMA ch\n");
  ...
  ret = dev_err_probe(gi2c->se.dev, IS_ERR(gi2c->rx_c), "Failed to get 
rx DMA ch\n");

to

  ret = dev_err_probe(gi2c->se.dev, PTR_ERR_OR_ZERO(gi2c->tx_c), "Failed 
to get tx DMA ch\n");
  ...
  ret = dev_err_probe(gi2c->se.dev, PTR_ERR_OR_ZERO(gi2c->rx_c), "Failed 
to get rx DMA ch\n");

Then dmesg looks as expected:

[    0.401471] geni_i2c c80000.i2c: Bus frequency not specified, default 
to 100kHz.
[    0.404609] geni_i2c c80000.i2c: error -ENODEV: Failed to get tx DMA ch
[    0.407711] geni_i2c c80000.i2c: FIFO mode disabled, but couldn't get 
DMA, fall back to FIFO mode
[    0.410542] geni_i2c c80000.i2c: i2c fifo/se-dma mode. fifo depth:16
[    0.414814] geni_i2c c80000.i2c: Geni-I2C adaptor successfully added

I think maybe IS_ERR is a mistake here, and PTR_ERR_OR_ZERO should be used?

-- 
Regards
Alexey Minnekhanov
postmarketOS developer
https://www.postmarketos.org

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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
  2021-10-29 15:31 kernel test robot
@ 2021-10-31 14:05   ` kernel test robot
  0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-10-31 14:05 UTC (permalink / raw)
  To: Vinod Koul, Bjorn Andersson, Wolfram Sang
  Cc: llvm, kbuild-all, linux-arm-msm, Vinod Koul, Andy Gross,
	Sumit Semwal, Douglas Anderson, Matthias Kaehlcke, linux-i2c,
	Linux Kernel Mailing List

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

Hi Vinod,

Thanks for your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/i2c-qcom-geni-Add-support-for-GPI-DMA/20211019-140347
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: riscv-randconfig-c006-20211028 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # install riscv cross compiling tool for clang build
         # apt-get install binutils-riscv64-linux-gnu
         # https://github.com/0day-ci/linux/commit/8244d69587ec2d94d34a75f5a3865d250e6880c7
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review Vinod-Koul/i2c-qcom-geni-Add-support-for-GPI-DMA/20211019-140347
         git checkout 8244d69587ec2d94d34a75f5a3865d250e6880c7
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv clang-analyzer

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


clang-analyzer warnings: (new ones prefixed by >>)

 >> drivers/i2c/busses/i2c-qcom-geni.c:508:8: warning: Value stored to 'dma_buf' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
            void *dma_buf = &buf;
                  ^~~~~~~   ~~~~

vim +/dma_buf +508 drivers/i2c/busses/i2c-qcom-geni.c

8244d69587ec2d9 Vinod Koul 2021-10-19  501
8244d69587ec2d9 Vinod Koul 2021-10-19  502  static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
8244d69587ec2d9 Vinod Koul 2021-10-19  503  			   struct dma_slave_config *config, dma_addr_t *dma_addr_p,
8244d69587ec2d9 Vinod Koul 2021-10-19  504  			   void **buf, unsigned int op, struct dma_chan *dma_chan)
8244d69587ec2d9 Vinod Koul 2021-10-19  505  {
8244d69587ec2d9 Vinod Koul 2021-10-19  506  	struct gpi_i2c_config *peripheral;
8244d69587ec2d9 Vinod Koul 2021-10-19  507  	unsigned int flags;
8244d69587ec2d9 Vinod Koul 2021-10-19 @508  	void *dma_buf = &buf;
8244d69587ec2d9 Vinod Koul 2021-10-19  509  	dma_addr_t addr;
8244d69587ec2d9 Vinod Koul 2021-10-19  510  	enum dma_data_direction map_dirn;
8244d69587ec2d9 Vinod Koul 2021-10-19  511  	enum dma_transfer_direction dma_dirn;
8244d69587ec2d9 Vinod Koul 2021-10-19  512  	struct dma_async_tx_descriptor *desc;
8244d69587ec2d9 Vinod Koul 2021-10-19  513  	int ret;
8244d69587ec2d9 Vinod Koul 2021-10-19  514
8244d69587ec2d9 Vinod Koul 2021-10-19  515  	peripheral = config->peripheral_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  516
8244d69587ec2d9 Vinod Koul 2021-10-19  517  	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
8244d69587ec2d9 Vinod Koul 2021-10-19  518  	if (!dma_buf)
8244d69587ec2d9 Vinod Koul 2021-10-19  519  		return -ENOMEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  520
8244d69587ec2d9 Vinod Koul 2021-10-19  521  	if (op == I2C_WRITE)
8244d69587ec2d9 Vinod Koul 2021-10-19  522  		map_dirn = DMA_TO_DEVICE;
8244d69587ec2d9 Vinod Koul 2021-10-19  523  	else
8244d69587ec2d9 Vinod Koul 2021-10-19  524  		map_dirn = DMA_FROM_DEVICE;
8244d69587ec2d9 Vinod Koul 2021-10-19  525
8244d69587ec2d9 Vinod Koul 2021-10-19  526  	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
8244d69587ec2d9 Vinod Koul 2021-10-19  527  	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
8244d69587ec2d9 Vinod Koul 2021-10-19  528  		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
8244d69587ec2d9 Vinod Koul 2021-10-19  529  		return -ENOMEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  530  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  531
8244d69587ec2d9 Vinod Koul 2021-10-19  532  	peripheral->rx_len = msg->len;
8244d69587ec2d9 Vinod Koul 2021-10-19  533  	peripheral->op = op;
8244d69587ec2d9 Vinod Koul 2021-10-19  534
8244d69587ec2d9 Vinod Koul 2021-10-19  535  	ret = dmaengine_slave_config(dma_chan, config);
8244d69587ec2d9 Vinod Koul 2021-10-19  536  	if (ret) {
8244d69587ec2d9 Vinod Koul 2021-10-19  537  		dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
8244d69587ec2d9 Vinod Koul 2021-10-19  538  		goto err_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  539  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  540
8244d69587ec2d9 Vinod Koul 2021-10-19  541  	peripheral->set_config =  false;
8244d69587ec2d9 Vinod Koul 2021-10-19  542  	peripheral->multi_msg = true;
8244d69587ec2d9 Vinod Koul 2021-10-19  543  	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
8244d69587ec2d9 Vinod Koul 2021-10-19  544
8244d69587ec2d9 Vinod Koul 2021-10-19  545  	if (op == I2C_WRITE)
8244d69587ec2d9 Vinod Koul 2021-10-19  546  		dma_dirn = DMA_MEM_TO_DEV;
8244d69587ec2d9 Vinod Koul 2021-10-19  547  	else
8244d69587ec2d9 Vinod Koul 2021-10-19  548  		dma_dirn = DMA_DEV_TO_MEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  549
8244d69587ec2d9 Vinod Koul 2021-10-19  550  	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
8244d69587ec2d9 Vinod Koul 2021-10-19  551  	if (!desc) {
8244d69587ec2d9 Vinod Koul 2021-10-19  552  		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
8244d69587ec2d9 Vinod Koul 2021-10-19  553  		ret = -EIO;
8244d69587ec2d9 Vinod Koul 2021-10-19  554  		goto err_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  555  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  556
8244d69587ec2d9 Vinod Koul 2021-10-19  557  	desc->callback_result = i2c_gsi_cb_result;
8244d69587ec2d9 Vinod Koul 2021-10-19  558  	desc->callback_param = gi2c;
8244d69587ec2d9 Vinod Koul 2021-10-19  559
8244d69587ec2d9 Vinod Koul 2021-10-19  560  	dmaengine_submit(desc);
8244d69587ec2d9 Vinod Koul 2021-10-19  561  	*dma_addr_p = addr;
8244d69587ec2d9 Vinod Koul 2021-10-19  562
8244d69587ec2d9 Vinod Koul 2021-10-19  563  	return 0;
8244d69587ec2d9 Vinod Koul 2021-10-19  564
8244d69587ec2d9 Vinod Koul 2021-10-19  565  err_config:
8244d69587ec2d9 Vinod Koul 2021-10-19  566  	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
8244d69587ec2d9 Vinod Koul 2021-10-19  567  	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
8244d69587ec2d9 Vinod Koul 2021-10-19  568  	return ret;
8244d69587ec2d9 Vinod Koul 2021-10-19  569  }
8244d69587ec2d9 Vinod Koul 2021-10-19  570

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

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 36143 bytes --]

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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
@ 2021-10-31 14:05   ` kernel test robot
  0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-10-31 14:05 UTC (permalink / raw)
  To: kbuild-all

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

Hi Vinod,

Thanks for your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/i2c-qcom-geni-Add-support-for-GPI-DMA/20211019-140347
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
config: riscv-randconfig-c006-20211028 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
         chmod +x ~/bin/make.cross
         # install riscv cross compiling tool for clang build
         # apt-get install binutils-riscv64-linux-gnu
         # https://github.com/0day-ci/linux/commit/8244d69587ec2d94d34a75f5a3865d250e6880c7
         git remote add linux-review https://github.com/0day-ci/linux
         git fetch --no-tags linux-review Vinod-Koul/i2c-qcom-geni-Add-support-for-GPI-DMA/20211019-140347
         git checkout 8244d69587ec2d94d34a75f5a3865d250e6880c7
         # save the attached .config to linux build tree
         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv clang-analyzer

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


clang-analyzer warnings: (new ones prefixed by >>)

 >> drivers/i2c/busses/i2c-qcom-geni.c:508:8: warning: Value stored to 'dma_buf' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
            void *dma_buf = &buf;
                  ^~~~~~~   ~~~~

vim +/dma_buf +508 drivers/i2c/busses/i2c-qcom-geni.c

8244d69587ec2d9 Vinod Koul 2021-10-19  501
8244d69587ec2d9 Vinod Koul 2021-10-19  502  static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
8244d69587ec2d9 Vinod Koul 2021-10-19  503  			   struct dma_slave_config *config, dma_addr_t *dma_addr_p,
8244d69587ec2d9 Vinod Koul 2021-10-19  504  			   void **buf, unsigned int op, struct dma_chan *dma_chan)
8244d69587ec2d9 Vinod Koul 2021-10-19  505  {
8244d69587ec2d9 Vinod Koul 2021-10-19  506  	struct gpi_i2c_config *peripheral;
8244d69587ec2d9 Vinod Koul 2021-10-19  507  	unsigned int flags;
8244d69587ec2d9 Vinod Koul 2021-10-19 @508  	void *dma_buf = &buf;
8244d69587ec2d9 Vinod Koul 2021-10-19  509  	dma_addr_t addr;
8244d69587ec2d9 Vinod Koul 2021-10-19  510  	enum dma_data_direction map_dirn;
8244d69587ec2d9 Vinod Koul 2021-10-19  511  	enum dma_transfer_direction dma_dirn;
8244d69587ec2d9 Vinod Koul 2021-10-19  512  	struct dma_async_tx_descriptor *desc;
8244d69587ec2d9 Vinod Koul 2021-10-19  513  	int ret;
8244d69587ec2d9 Vinod Koul 2021-10-19  514
8244d69587ec2d9 Vinod Koul 2021-10-19  515  	peripheral = config->peripheral_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  516
8244d69587ec2d9 Vinod Koul 2021-10-19  517  	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
8244d69587ec2d9 Vinod Koul 2021-10-19  518  	if (!dma_buf)
8244d69587ec2d9 Vinod Koul 2021-10-19  519  		return -ENOMEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  520
8244d69587ec2d9 Vinod Koul 2021-10-19  521  	if (op == I2C_WRITE)
8244d69587ec2d9 Vinod Koul 2021-10-19  522  		map_dirn = DMA_TO_DEVICE;
8244d69587ec2d9 Vinod Koul 2021-10-19  523  	else
8244d69587ec2d9 Vinod Koul 2021-10-19  524  		map_dirn = DMA_FROM_DEVICE;
8244d69587ec2d9 Vinod Koul 2021-10-19  525
8244d69587ec2d9 Vinod Koul 2021-10-19  526  	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
8244d69587ec2d9 Vinod Koul 2021-10-19  527  	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
8244d69587ec2d9 Vinod Koul 2021-10-19  528  		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
8244d69587ec2d9 Vinod Koul 2021-10-19  529  		return -ENOMEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  530  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  531
8244d69587ec2d9 Vinod Koul 2021-10-19  532  	peripheral->rx_len = msg->len;
8244d69587ec2d9 Vinod Koul 2021-10-19  533  	peripheral->op = op;
8244d69587ec2d9 Vinod Koul 2021-10-19  534
8244d69587ec2d9 Vinod Koul 2021-10-19  535  	ret = dmaengine_slave_config(dma_chan, config);
8244d69587ec2d9 Vinod Koul 2021-10-19  536  	if (ret) {
8244d69587ec2d9 Vinod Koul 2021-10-19  537  		dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
8244d69587ec2d9 Vinod Koul 2021-10-19  538  		goto err_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  539  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  540
8244d69587ec2d9 Vinod Koul 2021-10-19  541  	peripheral->set_config =  false;
8244d69587ec2d9 Vinod Koul 2021-10-19  542  	peripheral->multi_msg = true;
8244d69587ec2d9 Vinod Koul 2021-10-19  543  	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
8244d69587ec2d9 Vinod Koul 2021-10-19  544
8244d69587ec2d9 Vinod Koul 2021-10-19  545  	if (op == I2C_WRITE)
8244d69587ec2d9 Vinod Koul 2021-10-19  546  		dma_dirn = DMA_MEM_TO_DEV;
8244d69587ec2d9 Vinod Koul 2021-10-19  547  	else
8244d69587ec2d9 Vinod Koul 2021-10-19  548  		dma_dirn = DMA_DEV_TO_MEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  549
8244d69587ec2d9 Vinod Koul 2021-10-19  550  	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
8244d69587ec2d9 Vinod Koul 2021-10-19  551  	if (!desc) {
8244d69587ec2d9 Vinod Koul 2021-10-19  552  		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
8244d69587ec2d9 Vinod Koul 2021-10-19  553  		ret = -EIO;
8244d69587ec2d9 Vinod Koul 2021-10-19  554  		goto err_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  555  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  556
8244d69587ec2d9 Vinod Koul 2021-10-19  557  	desc->callback_result = i2c_gsi_cb_result;
8244d69587ec2d9 Vinod Koul 2021-10-19  558  	desc->callback_param = gi2c;
8244d69587ec2d9 Vinod Koul 2021-10-19  559
8244d69587ec2d9 Vinod Koul 2021-10-19  560  	dmaengine_submit(desc);
8244d69587ec2d9 Vinod Koul 2021-10-19  561  	*dma_addr_p = addr;
8244d69587ec2d9 Vinod Koul 2021-10-19  562
8244d69587ec2d9 Vinod Koul 2021-10-19  563  	return 0;
8244d69587ec2d9 Vinod Koul 2021-10-19  564
8244d69587ec2d9 Vinod Koul 2021-10-19  565  err_config:
8244d69587ec2d9 Vinod Koul 2021-10-19  566  	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
8244d69587ec2d9 Vinod Koul 2021-10-19  567  	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
8244d69587ec2d9 Vinod Koul 2021-10-19  568  	return ret;
8244d69587ec2d9 Vinod Koul 2021-10-19  569  }
8244d69587ec2d9 Vinod Koul 2021-10-19  570

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36143 bytes --]

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

* Re: [PATCH v4] i2c: qcom-geni: Add support for GPI DMA
@ 2021-10-29 15:31 kernel test robot
  2021-10-31 14:05   ` kernel test robot
  0 siblings, 1 reply; 8+ messages in thread
From: kernel test robot @ 2021-10-29 15:31 UTC (permalink / raw)
  To: kbuild

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

CC: llvm(a)lists.linux.dev
CC: kbuild-all(a)lists.01.org
In-Reply-To: <20211019060158.1482722-1-vkoul@kernel.org>
References: <20211019060158.1482722-1-vkoul@kernel.org>
TO: Vinod Koul <vkoul@kernel.org>
TO: Bjorn Andersson <bjorn.andersson@linaro.org>
TO: Wolfram Sang <wsa-dev@sang-engineering.com>
CC: linux-arm-msm(a)vger.kernel.org
CC: Vinod Koul <vkoul@kernel.org>
CC: Andy Gross <agross@kernel.org>
CC: Sumit Semwal <sumit.semwal@linaro.org>
CC: Douglas Anderson <dianders@chromium.org>
CC: Matthias Kaehlcke <mka@chromium.org>
CC: linux-i2c(a)vger.kernel.org
CC: linux-kernel(a)vger.kernel.org

Hi Vinod,

I love your patch! Perhaps something to improve:

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

url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/i2c-qcom-geni-Add-support-for-GPI-DMA/20211019-140347
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
:::::: branch date: 10 days ago
:::::: commit date: 10 days ago
config: riscv-randconfig-c006-20211028 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 5db7568a6a1fcb408eb8988abdaff2a225a8eb72)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install riscv cross compiling tool for clang build
        # apt-get install binutils-riscv64-linux-gnu
        # https://github.com/0day-ci/linux/commit/8244d69587ec2d94d34a75f5a3865d250e6880c7
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vinod-Koul/i2c-qcom-geni-Add-support-for-GPI-DMA/20211019-140347
        git checkout 8244d69587ec2d94d34a75f5a3865d250e6880c7
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=riscv clang-analyzer 

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


clang-analyzer warnings: (new ones prefixed by >>)
           ^
   drivers/scsi/ufs/ufshcd.c:5223:35: note: Access to field 'cmd_active' results in a dereference of a null pointer (loaded from field 'active_uic_cmd')
                   hba->active_uic_cmd->cmd_active = 0;
                        ~~~~~~~~~~~~~~             ^
   Suppressed 19 warnings (18 in non-user code, 1 with check filters).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   drivers/extcon/extcon-max77843.c:837:2: warning: Value stored to 'ret' is never read [clang-analyzer-deadcode.DeadStores]
           ret = regmap_update_bits(max77843->regmap_muic,
           ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/extcon/extcon-max77843.c:837:2: note: Value stored to 'ret' is never read
           ret = regmap_update_bits(max77843->regmap_muic,
           ^     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   6 warnings generated.
   Suppressed 6 warnings (6 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   4 warnings generated.
   Suppressed 4 warnings (4 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   7 warnings generated.
   drivers/i2c/busses/i2c-pxa.c:1334:32: warning: Value stored to 'bri' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
           struct i2c_bus_recovery_info *bri = &i2c->recovery;
                                         ^~~   ~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-pxa.c:1334:32: note: Value stored to 'bri' during its initialization is never read
           struct i2c_bus_recovery_info *bri = &i2c->recovery;
                                         ^~~   ~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-pxa.c:1335:17: warning: Value stored to 'dev' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
           struct device *dev = i2c->adap.dev.parent;
                          ^~~   ~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-pxa.c:1335:17: note: Value stored to 'dev' during its initialization is never read
           struct device *dev = i2c->adap.dev.parent;
                          ^~~   ~~~~~~~~~~~~~~~~~~~~
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   5 warnings generated.
   Suppressed 5 warnings (5 in non-user code).
   Use -header-filter=.* to display errors from all non-system headers. Use -system-headers to display errors from system headers as well.
   9 warnings generated.
>> drivers/i2c/busses/i2c-qcom-geni.c:508:8: warning: Value stored to 'dma_buf' during its initialization is never read [clang-analyzer-deadcode.DeadStores]
           void *dma_buf = &buf;
                 ^~~~~~~   ~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:508:8: note: Value stored to 'dma_buf' during its initialization is never read
           void *dma_buf = &buf;
                 ^~~~~~~   ~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:625:3: warning: 6th function call argument is an uninitialized value [clang-analyzer-core.CallAndMessage]
                   geni_i2c_gpi_unmap(gi2c, &msgs[i], tx_buf, tx_addr, rx_buf, rx_addr);
                   ^
   drivers/i2c/busses/i2c-qcom-geni.c:647:6: note: 'ret' is >= 0
           if (ret < 0) {
               ^~~
   drivers/i2c/busses/i2c-qcom-geni.c:647:2: note: Taking false branch
           if (ret < 0) {
           ^
   drivers/i2c/busses/i2c-qcom-geni.c:657:6: note: Assuming field 'se_mode' is equal to I2C_GPI_DMA
           if (gi2c->se_mode == I2C_GPI_DMA) {
               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:657:2: note: Taking true branch
           if (gi2c->se_mode == I2C_GPI_DMA) {
           ^
   drivers/i2c/busses/i2c-qcom-geni.c:658:9: note: Calling 'geni_i2c_gsi_xfer'
                   ret = geni_i2c_gsi_xfer(gi2c, msgs, num);
                         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:576:22: note: 'rx_addr' declared without an initial value
           dma_addr_t tx_addr, rx_addr;
                               ^~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:582:6: note: Assuming field 'cfg_sent' is true
           if (!gi2c->cfg_sent) {
               ^~~~~~~~~~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:582:2: note: Taking false branch
           if (!gi2c->cfg_sent) {
           ^
   drivers/i2c/busses/i2c-qcom-geni.c:595:14: note: Assuming 'i' is < 'num'
           for (i = 0; i < num; i++) {
                       ^~~~~~~
   drivers/i2c/busses/i2c-qcom-geni.c:595:2: note: Loop condition is true.  Entering loop body
           for (i = 0; i < num; i++) {
           ^
   drivers/i2c/busses/i2c-qcom-geni.c:597:3: note: Left side of '&&' is true
                   dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
                   ^
   include/linux/dev_printk.h:158:2: note: expanded from macro 'dev_dbg'
           dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
           ^
   include/linux/dev_printk.h:128:3: note: expanded from macro 'dev_printk'
                   dev_printk_index_emit(level, fmt);                      \
                   ^
   include/linux/dev_printk.h:105:2: note: expanded from macro 'dev_printk_index_emit'
           printk_index_subsys_emit("%s %s: ", level, fmt)
           ^
   include/linux/printk.h:413:2: note: expanded from macro 'printk_index_subsys_emit'
           __printk_index_emit(fmt, level, subsys_fmt_prefix)
           ^
   include/linux/printk.h:370:7: note: expanded from macro '__printk_index_emit'
                   if (__builtin_constant_p(_fmt) && __builtin_constant_p(_level)) { \
                       ^
   drivers/i2c/busses/i2c-qcom-geni.c:597:3: note: Taking true branch
                   dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
                   ^
   include/linux/dev_printk.h:158:2: note: expanded from macro 'dev_dbg'
           dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
           ^
   include/linux/dev_printk.h:128:3: note: expanded from macro 'dev_printk'
                   dev_printk_index_emit(level, fmt);                      \
                   ^
   include/linux/dev_printk.h:105:2: note: expanded from macro 'dev_printk_index_emit'
           printk_index_subsys_emit("%s %s: ", level, fmt)
           ^
   include/linux/printk.h:413:2: note: expanded from macro 'printk_index_subsys_emit'
           __printk_index_emit(fmt, level, subsys_fmt_prefix)
           ^
   include/linux/printk.h:370:3: note: expanded from macro '__printk_index_emit'
                   if (__builtin_constant_p(_fmt) && __builtin_constant_p(_level)) { \
                   ^
   drivers/i2c/busses/i2c-qcom-geni.c:597:3: note: '?' condition is true
                   dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
                   ^
   include/linux/dev_printk.h:158:2: note: expanded from macro 'dev_dbg'
           dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
           ^
   include/linux/dev_printk.h:128:3: note: expanded from macro 'dev_printk'
                   dev_printk_index_emit(level, fmt);                      \
                   ^
   include/linux/dev_printk.h:105:2: note: expanded from macro 'dev_printk_index_emit'
           printk_index_subsys_emit("%s %s: ", level, fmt)
           ^
   include/linux/printk.h:413:2: note: expanded from macro 'printk_index_subsys_emit'
           __printk_index_emit(fmt, level, subsys_fmt_prefix)
           ^
   include/linux/printk.h:379:12: note: expanded from macro '__printk_index_emit'
                                   .fmt = __builtin_constant_p(_fmt) ? (_fmt) : NULL, \
                                          ^
   drivers/i2c/busses/i2c-qcom-geni.c:597:3: note: '?' condition is true
                   dev_dbg(gi2c->se.dev, "msg[%d].len:%d\n", i, gi2c->cur->len);
                   ^
   include/linux/dev_printk.h:158:2: note: expanded from macro 'dev_dbg'
           dev_printk(KERN_DEBUG, dev, dev_fmt(fmt), ##__VA_ARGS__)
           ^
   include/linux/dev_printk.h:128:3: note: expanded from macro 'dev_printk'
                   dev_printk_index_emit(level, fmt);                      \

vim +/dma_buf +508 drivers/i2c/busses/i2c-qcom-geni.c

8244d69587ec2d9 Vinod Koul 2021-10-19  501  
8244d69587ec2d9 Vinod Koul 2021-10-19  502  static int geni_i2c_gpi(struct geni_i2c_dev *gi2c, struct i2c_msg *msg,
8244d69587ec2d9 Vinod Koul 2021-10-19  503  			   struct dma_slave_config *config, dma_addr_t *dma_addr_p,
8244d69587ec2d9 Vinod Koul 2021-10-19  504  			   void **buf, unsigned int op, struct dma_chan *dma_chan)
8244d69587ec2d9 Vinod Koul 2021-10-19  505  {
8244d69587ec2d9 Vinod Koul 2021-10-19  506  	struct gpi_i2c_config *peripheral;
8244d69587ec2d9 Vinod Koul 2021-10-19  507  	unsigned int flags;
8244d69587ec2d9 Vinod Koul 2021-10-19 @508  	void *dma_buf = &buf;
8244d69587ec2d9 Vinod Koul 2021-10-19  509  	dma_addr_t addr;
8244d69587ec2d9 Vinod Koul 2021-10-19  510  	enum dma_data_direction map_dirn;
8244d69587ec2d9 Vinod Koul 2021-10-19  511  	enum dma_transfer_direction dma_dirn;
8244d69587ec2d9 Vinod Koul 2021-10-19  512  	struct dma_async_tx_descriptor *desc;
8244d69587ec2d9 Vinod Koul 2021-10-19  513  	int ret;
8244d69587ec2d9 Vinod Koul 2021-10-19  514  
8244d69587ec2d9 Vinod Koul 2021-10-19  515  	peripheral = config->peripheral_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  516  
8244d69587ec2d9 Vinod Koul 2021-10-19  517  	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
8244d69587ec2d9 Vinod Koul 2021-10-19  518  	if (!dma_buf)
8244d69587ec2d9 Vinod Koul 2021-10-19  519  		return -ENOMEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  520  
8244d69587ec2d9 Vinod Koul 2021-10-19  521  	if (op == I2C_WRITE)
8244d69587ec2d9 Vinod Koul 2021-10-19  522  		map_dirn = DMA_TO_DEVICE;
8244d69587ec2d9 Vinod Koul 2021-10-19  523  	else
8244d69587ec2d9 Vinod Koul 2021-10-19  524  		map_dirn = DMA_FROM_DEVICE;
8244d69587ec2d9 Vinod Koul 2021-10-19  525  
8244d69587ec2d9 Vinod Koul 2021-10-19  526  	addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, map_dirn);
8244d69587ec2d9 Vinod Koul 2021-10-19  527  	if (dma_mapping_error(gi2c->se.dev->parent, addr)) {
8244d69587ec2d9 Vinod Koul 2021-10-19  528  		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
8244d69587ec2d9 Vinod Koul 2021-10-19  529  		return -ENOMEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  530  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  531  
8244d69587ec2d9 Vinod Koul 2021-10-19  532  	peripheral->rx_len = msg->len;
8244d69587ec2d9 Vinod Koul 2021-10-19  533  	peripheral->op = op;
8244d69587ec2d9 Vinod Koul 2021-10-19  534  
8244d69587ec2d9 Vinod Koul 2021-10-19  535  	ret = dmaengine_slave_config(dma_chan, config);
8244d69587ec2d9 Vinod Koul 2021-10-19  536  	if (ret) {
8244d69587ec2d9 Vinod Koul 2021-10-19  537  		dev_err(gi2c->se.dev, "dma config error: %d for op:%d\n", ret, op);
8244d69587ec2d9 Vinod Koul 2021-10-19  538  		goto err_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  539  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  540  
8244d69587ec2d9 Vinod Koul 2021-10-19  541  	peripheral->set_config =  false;
8244d69587ec2d9 Vinod Koul 2021-10-19  542  	peripheral->multi_msg = true;
8244d69587ec2d9 Vinod Koul 2021-10-19  543  	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
8244d69587ec2d9 Vinod Koul 2021-10-19  544  
8244d69587ec2d9 Vinod Koul 2021-10-19  545  	if (op == I2C_WRITE)
8244d69587ec2d9 Vinod Koul 2021-10-19  546  		dma_dirn = DMA_MEM_TO_DEV;
8244d69587ec2d9 Vinod Koul 2021-10-19  547  	else
8244d69587ec2d9 Vinod Koul 2021-10-19  548  		dma_dirn = DMA_DEV_TO_MEM;
8244d69587ec2d9 Vinod Koul 2021-10-19  549  
8244d69587ec2d9 Vinod Koul 2021-10-19  550  	desc = dmaengine_prep_slave_single(dma_chan, addr, msg->len, dma_dirn, flags);
8244d69587ec2d9 Vinod Koul 2021-10-19  551  	if (!desc) {
8244d69587ec2d9 Vinod Koul 2021-10-19  552  		dev_err(gi2c->se.dev, "prep_slave_sg failed\n");
8244d69587ec2d9 Vinod Koul 2021-10-19  553  		ret = -EIO;
8244d69587ec2d9 Vinod Koul 2021-10-19  554  		goto err_config;
8244d69587ec2d9 Vinod Koul 2021-10-19  555  	}
8244d69587ec2d9 Vinod Koul 2021-10-19  556  
8244d69587ec2d9 Vinod Koul 2021-10-19  557  	desc->callback_result = i2c_gsi_cb_result;
8244d69587ec2d9 Vinod Koul 2021-10-19  558  	desc->callback_param = gi2c;
8244d69587ec2d9 Vinod Koul 2021-10-19  559  
8244d69587ec2d9 Vinod Koul 2021-10-19  560  	dmaengine_submit(desc);
8244d69587ec2d9 Vinod Koul 2021-10-19  561  	*dma_addr_p = addr;
8244d69587ec2d9 Vinod Koul 2021-10-19  562  
8244d69587ec2d9 Vinod Koul 2021-10-19  563  	return 0;
8244d69587ec2d9 Vinod Koul 2021-10-19  564  
8244d69587ec2d9 Vinod Koul 2021-10-19  565  err_config:
8244d69587ec2d9 Vinod Koul 2021-10-19  566  	dma_unmap_single(gi2c->se.dev->parent, addr, msg->len, map_dirn);
8244d69587ec2d9 Vinod Koul 2021-10-19  567  	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
8244d69587ec2d9 Vinod Koul 2021-10-19  568  	return ret;
8244d69587ec2d9 Vinod Koul 2021-10-19  569  }
8244d69587ec2d9 Vinod Koul 2021-10-19  570  

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

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 36143 bytes --]

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

end of thread, other threads:[~2021-11-04  2:44 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-19  6:01 [PATCH v4] i2c: qcom-geni: Add support for GPI DMA Vinod Koul
2021-11-01 16:23 ` Bjorn Andersson
2021-11-03  2:13 ` Alexey Minnekhanov
2021-11-03  5:21   ` Vinod Koul
2021-11-04  2:44     ` Alexey Minnekhanov
2021-10-29 15:31 kernel test robot
2021-10-31 14:05 ` kernel test robot
2021-10-31 14:05   ` kernel test robot

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