On 11-01-21, 12:14, Bjorn Andersson wrote:

On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:

quoted

This adds capability to use GSI DMA for I2C transfers

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/i2c/busses/i2c-qcom-geni.c | 246 ++++++++++++++++++++++++++++-
 1 file changed, 244 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 046d241183c5..6978480fb4d1 100644
--- a/drivers/i2c/busses/i2c-qcom-geni.c
+++ b/drivers/i2c/busses/i2c-qcom-geni.c

@@ -12,7 +12,9 @@
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
+#include <linux/dmaengine.h>
 #include <linux/qcom-geni-se.h>
+#include <linux/dma/qcom-gpi-dma.h>
 #include <linux/spinlock.h>
 
 #define SE_I2C_TX_TRANS_LEN		0x26c

@@ -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,12 @@ enum geni_i2c_err_code {
 #define XFER_TIMEOUT		HZ
 #define RST_TIMEOUT		HZ
 
+enum i2c_se_mode {
+	UNINITIALIZED,
+	FIFO_SE_DMA,
+	GSI_ONLY,
+};
+
 struct geni_i2c_dev {
 	struct geni_se se;
 	u32 tx_wm;

@@ -86,6 +96,17 @@ struct geni_i2c_dev {
 	u32 clk_freq_out;
 	const struct geni_i2c_clk_fld *clk_fld;
 	int suspended;
+	struct dma_chan *tx_c;
+	struct dma_chan *rx_c;
+	dma_cookie_t rx_cookie, tx_cookie;
+	dma_addr_t tx_ph;
+	dma_addr_t rx_ph;
+	int cfg_sent;

bool?

ok

quoted

+	struct dma_async_tx_descriptor *tx_desc;
+	struct dma_async_tx_descriptor *rx_desc;
+	enum i2c_se_mode se_mode;

bool gsi_only;

I think fifo_mode would be more apt... since we check for other modes in
the code

quoted

+	bool cmd_done;

Unused?

heh, will remove..

quoted

+	bool is_shared;

Used but meaningless?

Will drop

quoted

 };
 
 struct geni_i2c_err_log {

@@ -429,6 +450,183 @@ 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 int geni_i2c_gsi_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[],
+			     int num)
+{
+	struct geni_i2c_dev *gi2c = i2c_get_adapdata(adap);
+	struct dma_slave_config config;
+	struct gpi_i2c_config peripheral;
+	int i, ret = 0, timeout = 0;
+
+	memset(&config, 0, sizeof(config));

Assign {} to config during declaration.

ok

quoted

+	memset(&peripheral, 0, sizeof(peripheral));
+	config.peripheral_config = &peripheral;
+	config.peripheral_size = sizeof(peripheral);
+
+	if (!gi2c->tx_c) {
+		gi2c->tx_c = dma_request_slave_channel(gi2c->se.dev, "tx");

So object is reused for all future transfers as well?
Seems reasonable, but it should be released on driver removal?

Could it be requested at probe time instead?

yes it can be done, i would move it..

quoted

+		if (!gi2c->tx_c) {
+			dev_err(gi2c->se.dev, "tx dma_request_slave_channel fail\n");
+			ret = -EIO;
+			goto geni_i2c_gsi_xfer_out;
+		}
+	}
+
+	if (!gi2c->rx_c) {
+		gi2c->rx_c = dma_request_slave_channel(gi2c->se.dev, "rx");
+		if (!gi2c->rx_c) {
+			dev_err(gi2c->se.dev, "rx dma_request_slave_channel fail\n");
+			ret = -EIO;
+			goto geni_i2c_gsi_xfer_out;
+		}
+	}
+
+	if (!gi2c->cfg_sent) {
+		const struct geni_i2c_clk_fld *itr = gi2c->clk_fld;
+
+		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;
+		gi2c->cfg_sent = true;

Is this a bool or an int?

Now would be a bool :)

quoted

+		peripheral.set_config =  true;

I find this somewhat ugly, you will always
dmaengine_slave_config(&config), but in the case of cfg_sent this will
point to an all-zero peripheral and hence will have set_config = false,
which will cause the skipping of setting up a configuration TRE.

I would prefer that the value of peripheral.set_config related to
cfg_sent in a more explicit fashion.

Sure, i think I can use a single value to do this, will update this

quoted

+	}
+
+	peripheral.multi_msg = false;
+	for (i = 0; i < num; i++) {
+		struct device *rx_dev = gi2c->se.wrapper->dev;
+		struct device *tx_dev = gi2c->se.wrapper->dev;
+		int stretch = (i < (num - 1));
+		u8 *dma_buf = NULL;

No need to initialize this, first use is an assignment.

ok

quoted

+		unsigned int flags;
+
+		gi2c->cur = &msgs[i];
+
+		peripheral.addr = msgs[i].addr;
+		peripheral.stretch = stretch;
+		if (msgs[i].flags & I2C_M_RD)
+			peripheral.op = I2C_READ;
+		else
+			peripheral.op = I2C_WRITE;
+
+		dma_buf = i2c_get_dma_safe_msg_buf(&msgs[i], 1);
+		if (!dma_buf) {
+			ret = -ENOMEM;
+			goto geni_i2c_gsi_xfer_out;
+		}
+
+		if (msgs[i].flags & I2C_M_RD) {
+			gi2c->rx_ph = dma_map_single(rx_dev, dma_buf,
+						     msgs[i].len, DMA_FROM_DEVICE);
+			if (dma_mapping_error(rx_dev, gi2c->rx_ph)) {
+				dev_err(gi2c->se.dev, "dma_map_single for rx failed :%d\n", ret);
+				i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false);
+				goto geni_i2c_gsi_xfer_out;
+			}
+
+			peripheral.op = I2C_READ;
+			peripheral.stretch = stretch;
+			ret = dmaengine_slave_config(gi2c->rx_c, &config);
+			if (ret) {
+				dev_err(gi2c->se.dev, "rx dma config error:%d\n", ret);
+				goto geni_i2c_gsi_xfer_out;

Need to unmap rx_ph?

yes will update

quoted

+			}
+			peripheral.set_config =  false;
+			peripheral.multi_msg = true;
+			peripheral.rx_len = msgs[i].len;
+
+			flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+			gi2c->rx_desc = dmaengine_prep_slave_single(gi2c->rx_c, gi2c->rx_ph,
+								    msgs[i].len,
+								    DMA_DEV_TO_MEM, flags);

Is the rx_desc freed by the dmaengine core when
dma_async_issue_pending() finishes it's job?

Yes

If so, why do you need to keep this pointer in gi2c? Wouldn't a local
variable suffice?

Yes local should suffice, will update

quoted

+			if (!gi2c->rx_desc) {
+				dev_err(gi2c->se.dev, "prep_slave_sg for rx failed\n");
+				gi2c->err = -EIO;
+				goto geni_i2c_err_prep_sg;
+			}
+
+			gi2c->rx_desc->callback_result = i2c_gsi_cb_result;
+			gi2c->rx_desc->callback_param = gi2c;
+
+			/* Issue RX */
+			gi2c->rx_cookie = dmaengine_submit(gi2c->rx_desc);
+			dma_async_issue_pending(gi2c->rx_c);
+		}
+
+		dev_dbg(gi2c->se.dev, "msg[%d].len:%d W\n", i, gi2c->cur->len);
+		gi2c->tx_ph = dma_map_single(tx_dev, dma_buf, msgs[i].len, DMA_TO_DEVICE);

Maybe I've forgotten something important about I2C, but why do we always
TX (even if it's a RX transfer)?

I think we need to send the device address for i2c, so even if we want
to do RX, that will always involve a TX txn as well

quoted

+		if (dma_mapping_error(tx_dev, gi2c->tx_ph)) {
+			dev_err(gi2c->se.dev, "dma_map_single for tx failed :%d\n", ret);
+			i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], false);

Need to unmap rx_ph?

quoted

+			goto geni_i2c_gsi_xfer_out;
+		}
+
+		peripheral.stretch = stretch;
+		peripheral.op = I2C_WRITE;
+		ret = dmaengine_slave_config(gi2c->tx_c, &config);
+		if (ret) {
+			dev_err(gi2c->se.dev, "tx dma config error:%d\n", ret);

Need to unmap rx_ph and tx_ph?

Yeah looks like I missed unrolling, will check and update all these

quoted

+			goto geni_i2c_gsi_xfer_out;
+		}
+		peripheral.set_config =  false;
+		peripheral.multi_msg = true;
+		gi2c->tx_desc = dmaengine_prep_slave_single(gi2c->tx_c, gi2c->tx_ph, msgs[i].len,
+							    DMA_MEM_TO_DEV,
+							    (DMA_PREP_INTERRUPT |  DMA_CTRL_ACK));
+		if (!gi2c->tx_desc) {
+			dev_err(gi2c->se.dev, "prep_slave_sg for tx failed\n");
+			gi2c->err = -ENOMEM;
+			goto geni_i2c_err_prep_sg;
+		}
+		gi2c->tx_desc->callback_result = i2c_gsi_cb_result;
+		gi2c->tx_desc->callback_param = gi2c;
+
+		/* Issue TX */
+		gi2c->tx_cookie = dmaengine_submit(gi2c->tx_desc);
+		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;
+		}
+geni_i2c_err_prep_sg:

Perhaps you can break the body of this loop out to a separate function
and thereby avoid the goto within the block?

quoted

+		if (gi2c->err) {
+			dmaengine_terminate_all(gi2c->tx_c);
+			gi2c->cfg_sent = 0;

Is this a bool or an int?

quoted

+		}
+		if (msgs[i].flags & I2C_M_RD)
+			dma_unmap_single(rx_dev, gi2c->rx_ph, msgs[i].len, DMA_FROM_DEVICE);

You unconditionally map tx_ph, but you only unmap it on ~I2C_M_RD. This
fits better with my expectation, but would mean that the whole tx block
above should be in an else.

quoted

+		else
+			dma_unmap_single(tx_dev, gi2c->tx_ph, msgs[i].len, DMA_TO_DEVICE);
+		i2c_put_dma_safe_msg_buf(dma_buf, &msgs[i], !gi2c->err);
+		if (gi2c->err)
+			goto geni_i2c_gsi_xfer_out;

This goto is just a "break" in disguise.

quoted

+	}
+
+geni_i2c_gsi_xfer_out:
+	if (!ret && gi2c->err)
+		ret = gi2c->err;
+	return ret;
+}
+
 static int geni_i2c_xfer(struct i2c_adapter *adap,
 			 struct i2c_msg msgs[],
 			 int num)

@@ -448,6 +646,15 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 	}
 
 	qcom_geni_i2c_conf(gi2c);
+
+	if (gi2c->se_mode == GSI_ONLY) {
+		ret = geni_i2c_gsi_xfer(adap, msgs, num);
+		goto geni_i2c_txn_ret;

Rather than goto skip_non_gsi_code; I think you should move the non-gsi
part of this function into a separate fifo function and make this

Okay let me take a relook at this whole blob and refactor it..

if (GSI_ONLY)
	ret = geni_i2c_gsi_xfer();
else
	ret = geni_i2c_fifo_xfer();

quoted

+	} else {
+		/* Don't set shared flag in non-GSI mode */
+		gi2c->is_shared = false;

I don't see this flag being looked at elsewhere.

quoted

+	}
+
 	for (i = 0; i < num; i++) {
 		u32 m_param = i < (num - 1) ? STOP_STRETCH : 0;
 

@@ -462,6 +669,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 		if (ret)
 			break;
 	}
+geni_i2c_txn_ret:
 	if (ret == 0)
 		ret = num;
 

@@ -628,7 +836,8 @@ static int __maybe_unused geni_i2c_runtime_suspend(struct device *dev)
 	int ret;
 	struct geni_i2c_dev *gi2c = dev_get_drvdata(dev);
 
-	disable_irq(gi2c->irq);
+	if (gi2c->se_mode == FIFO_SE_DMA)
+		disable_irq(gi2c->irq);
 	ret = geni_se_resources_off(&gi2c->se);
 	if (ret) {
 		enable_irq(gi2c->irq);

@@ -653,8 +862,41 @@ static int __maybe_unused geni_i2c_runtime_resume(struct device *dev)
 	ret = geni_se_resources_on(&gi2c->se);
 	if (ret)
 		return ret;
+	if (gi2c->se_mode == UNINITIALIZED) {
+		int proto = geni_se_read_proto(&gi2c->se);
+		u32 se_mode;

Please declare your variables at the top of the function.

quoted

+
+		if (unlikely(proto != GENI_SE_I2C)) {

If this was the case at probe time the driver would never have probed,
why has it changed?

This is not a fastpath, so skip the unlikely()

quoted

+			dev_err(gi2c->se.dev, "Invalid proto %d\n", proto);
+			geni_se_resources_off(&gi2c->se);
+			return -ENXIO;
+		}
+
+		se_mode = readl_relaxed(gi2c->se.base + GENI_IF_DISABLE_RO) &
+				FIFO_IF_DISABLE;

se_mode would better be called "fifo_disabled" or perhaps logically
suited "gsi_only"?

O think fifo_mode or just mode might be apt

Please skip the _relaxed

yes

quoted

+		if (se_mode) {
+			gi2c->se_mode = GSI_ONLY;
+			geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);
+			dev_dbg(gi2c->se.dev, "i2c GSI mode\n");
+		} else {
+			int gi2c_tx_depth = geni_se_get_tx_fifo_depth(&gi2c->se);

This variable has an unnecessarily long name.

will shorten

quoted

+
+			gi2c->se_mode = FIFO_SE_DMA;
+			gi2c->tx_wm = gi2c_tx_depth - 1;
+			geni_se_init(&gi2c->se, gi2c->tx_wm, gi2c_tx_depth);
+			geni_se_config_packing(&gi2c->se, BITS_PER_BYTE,
+					       PACKING_BYTES_PW, true, true, true);
+			qcom_geni_i2c_conf(gi2c);
+			dev_dbg(gi2c->se.dev,
+				"i2c fifo/se-dma mode. fifo depth:%d\n", gi2c_tx_depth);
+		}
+		dev_dbg(gi2c->se.dev, "i2c-%d: %s\n",
+			gi2c->adap.nr, dev_name(gi2c->se.dev));

dev_dbg() already provides dev_name. What information does this debug
print actually try to communicate?

not much am afraid, will update

-- 
~Vinod