[PATCH v3 0/5] Add and enable GPI DMA users

[PATCH v3 0/5] Add and enable GPI DMA users

[Vinod Koul]

Hello,

This series adds the GPI DMA in qcom geni spi and i2c drivers.

For this we first need to move GENI_IF_DISABLE_RO to common
headers and then add support for gpi dma in geni drivers.

Also, to reuse the dma-mapping in spi, we add a new field dma_map_dev to
allow controllers to pass a specific device for dma-mapping

Then, we add spi and i2c geni driver changes to support this DMA.

Changes since v2:
- Add core spi patch for dma_map_dev addition
- rework the logic for getting and releasing channels in both driver, also
  ensure proper cleanup
- Fix the comments recieved from Doug and Bjorn
- Add kernel-doc changes for enum geni_se_xfer_mode

Changes since v1:
 - add r-b from Bjorn on patch 1
 - add more details in log for patch 2
 - Fix the comments from Doug and Bjorn for patch3, notable use read, modify
   update for irq registers, use geni_se_irq_clear() for irq, dont update
   single bit registers, add a bit more description for gpi dma etc

Vinod Koul (5):
  soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
  soc: qcom: geni: Add support for gpi dma
  spi: core: add dma_map_dev for dma device
  spi: spi-geni-qcom: Add support for GPI dma
  i2c: qcom-geni: Add support for GPI DMA

 drivers/i2c/busses/i2c-qcom-geni.c | 309 ++++++++++++++++++++++++++-
 drivers/soc/qcom/qcom-geni-se.c    |  30 ++-
 drivers/spi/spi-geni-qcom.c        | 329 +++++++++++++++++++++++++++--
 drivers/spi/spi.c                  |   4 +
 include/linux/qcom-geni-se.h       |  19 +-
 include/linux/spi/spi.h            |   1 +
 6 files changed, 667 insertions(+), 25 deletions(-)

-- 
2.31.1

[PATCH v3 1/5] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header

[Vinod Koul]

GENI_IF_DISABLE_RO is used by geni spi driver as well to check the
status if GENI, so move this to common header qcom-geni-se.h

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/soc/qcom/qcom-geni-se.c | 1 -
 include/linux/qcom-geni-se.h    | 4 ++++
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 5bdfb1565c14..fe666ea0c487 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -104,7 +104,6 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
 #define GENI_OUTPUT_CTRL		0x24
 #define GENI_CGC_CTRL			0x28
 #define GENI_CLK_CTRL_RO		0x60
-#define GENI_IF_DISABLE_RO		0x64
 #define GENI_FW_S_REVISION_RO		0x6c
 #define SE_GENI_BYTE_GRAN		0x254
 #define SE_GENI_TX_PACKING_CFG0		0x260
diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index 7c811eebcaab..5114e2144b17 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -63,6 +63,7 @@ struct geni_se {
 #define SE_GENI_STATUS			0x40
 #define GENI_SER_M_CLK_CFG		0x48
 #define GENI_SER_S_CLK_CFG		0x4c
+#define GENI_IF_DISABLE_RO		0x64
 #define GENI_FW_REVISION_RO		0x68
 #define SE_GENI_CLK_SEL			0x7c
 #define SE_GENI_DMA_MODE_EN		0x258
@@ -105,6 +106,9 @@ struct geni_se {
 #define CLK_DIV_MSK			GENMASK(15, 4)
 #define CLK_DIV_SHFT			4
 
+/* GENI_IF_DISABLE_RO fields */
+#define FIFO_IF_DISABLE			(BIT(0))
+
 /* GENI_FW_REVISION_RO fields */
 #define FW_REV_PROTOCOL_MSK		GENMASK(15, 8)
 #define FW_REV_PROTOCOL_SHFT		8
-- 
2.31.1

[PATCH v3 2/5] soc: qcom: geni: Add support for gpi dma

[Vinod Koul]

GPI DMA is one of the DMA modes supported on geni, this adds support to
enable that mode

Also do better documentation of the enum geni_se_xfer_mode.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/soc/qcom/qcom-geni-se.c | 29 ++++++++++++++++++++++++++++-
 include/linux/qcom-geni-se.h    | 15 ++++++++++++++-
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index fe666ea0c487..7d649d2cf31e 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -321,6 +321,30 @@ static void geni_se_select_dma_mode(struct geni_se *se)
 		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
 }
 
+static void geni_se_select_gpi_mode(struct geni_se *se)
+{
+	u32 val;
+
+	geni_se_irq_clear(se);
+
+	writel(0, se->base + SE_IRQ_EN);
+
+	val = readl(se->base + SE_GENI_S_IRQ_EN);
+	val &= ~S_CMD_DONE_EN;
+	writel(val, se->base + SE_GENI_S_IRQ_EN);
+
+	val = readl(se->base + SE_GENI_M_IRQ_EN);
+	val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
+		 M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
+	writel(val, se->base + SE_GENI_M_IRQ_EN);
+
+	writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
+
+	val = readl(se->base + SE_GSI_EVENT_EN);
+	val |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | GENI_M_EVENT_EN | GENI_S_EVENT_EN);
+	writel(val, se->base + SE_GSI_EVENT_EN);
+}
+
 /**
  * geni_se_select_mode() - Select the serial engine transfer mode
  * @se:		Pointer to the concerned serial engine.
@@ -328,7 +352,7 @@ static void geni_se_select_dma_mode(struct geni_se *se)
  */
 void geni_se_select_mode(struct geni_se *se, enum geni_se_xfer_mode mode)
 {
-	WARN_ON(mode != GENI_SE_FIFO && mode != GENI_SE_DMA);
+	WARN_ON(mode != GENI_SE_FIFO && mode != GENI_SE_DMA && mode != GENI_GPI_DMA);
 
 	switch (mode) {
 	case GENI_SE_FIFO:
@@ -337,6 +361,9 @@ void geni_se_select_mode(struct geni_se *se, enum geni_se_xfer_mode mode)
 	case GENI_SE_DMA:
 		geni_se_select_dma_mode(se);
 		break;
+	case GENI_GPI_DMA:
+		geni_se_select_gpi_mode(se);
+		break;
 	case GENI_SE_INVALID:
 	default:
 		break;
diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index 5114e2144b17..f5672785c0c4 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -8,11 +8,24 @@
 
 #include <linux/interconnect.h>
 
-/* Transfer mode supported by GENI Serial Engines */
+/**
+ * enum geni_se_xfer_mode: Transfer modes supported by Serial Engines
+ *
+ * @GENI_SE_INVALID: Invalid mode
+ * @GENI_SE_FIFO: FIFO mode. Data is transferred with SE FIFO
+ * by programmed IO method
+ * @GENI_SE_DMA: Serial Engine DMA mode. Data is transferred
+ * with SE by DMAengine internal to SE
+ * @GENI_GPI_DMA: GPI DMA mode. Data is transferred using a DMAengine
+ * configured by a firmware residing on a GSI engine. This DMA name is
+ * interchangeably used as GSI or GPI which seem to imply the same DMAengine
+ */
+
 enum geni_se_xfer_mode {
 	GENI_SE_INVALID,
 	GENI_SE_FIFO,
 	GENI_SE_DMA,
+	GENI_GPI_DMA,
 };
 
 /* Protocols supported by GENI Serial Engines */
-- 
2.31.1

[PATCH v3 3/5] spi: core: add dma_map_dev for dma device

[Vinod Koul]

Some controllers like qcom geni need the parent device to be used for
dma mapping, so add a dma_map_dev field and let drivers fill this to be
used as mapping device

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/spi/spi.c       | 4 ++++
 include/linux/spi/spi.h | 1 +
 2 files changed, 5 insertions(+)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index e353b7a9e54e..315f7e7545f7 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -961,11 +961,15 @@ static int __spi_map_msg(struct spi_controller *ctlr, struct spi_message *msg)
 
 	if (ctlr->dma_tx)
 		tx_dev = ctlr->dma_tx->device->dev;
+	else if (ctlr->dma_map_dev)
+		tx_dev = ctlr->dma_map_dev;
 	else
 		tx_dev = ctlr->dev.parent;
 
 	if (ctlr->dma_rx)
 		rx_dev = ctlr->dma_rx->device->dev;
+	else if (ctlr->dma_map_dev)
+		rx_dev = ctlr->dma_map_dev;
 	else
 		rx_dev = ctlr->dev.parent;
 
diff --git a/include/linux/spi/spi.h b/include/linux/spi/spi.h
index 74239d65c7fd..4d3f116f5723 100644
--- a/include/linux/spi/spi.h
+++ b/include/linux/spi/spi.h
@@ -586,6 +586,7 @@ struct spi_controller {
 	bool			(*can_dma)(struct spi_controller *ctlr,
 					   struct spi_device *spi,
 					   struct spi_transfer *xfer);
+	struct device *dma_map_dev;
 
 	/*
 	 * These hooks are for drivers that want to use the generic
-- 
2.31.1

[PATCH v3 4/5] spi: spi-geni-qcom: Add support for GPI dma

[Vinod Koul]

We can use GPI DMA for devices where it is enabled by firmware. Add
support for this mode

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 drivers/spi/spi-geni-qcom.c | 329 ++++++++++++++++++++++++++++++++++--
 1 file changed, 315 insertions(+), 14 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 3d0d8ddd5772..c64355c246be 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -2,6 +2,9 @@
 // Copyright (c) 2017-2018, The Linux foundation. All rights reserved.
 
 #include <linux/clk.h>
+#include <linux/dmaengine.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma/qcom-gpi-dma.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/log2.h>
@@ -63,6 +66,29 @@
 #define TIMESTAMP_AFTER		BIT(3)
 #define POST_CMD_DELAY		BIT(4)
 
+#define GSI_LOOPBACK_EN		(BIT(0))
+#define GSI_CS_TOGGLE		(BIT(3))
+#define GSI_CPHA		(BIT(4))
+#define GSI_CPOL		(BIT(5))
+
+#define MAX_TX_SG		(3)
+#define NUM_SPI_XFER		(8)
+#define SPI_XFER_TIMEOUT_MS	(250)
+
+struct gsi_desc_cb {
+	struct spi_geni_master *mas;
+	struct spi_transfer *xfer;
+};
+
+struct spi_geni_gsi {
+	dma_cookie_t tx_cookie;
+	dma_cookie_t rx_cookie;
+	struct dma_async_tx_descriptor *tx_desc;
+	struct dma_async_tx_descriptor *rx_desc;
+	struct gsi_desc_cb tx_cb;
+	struct gsi_desc_cb rx_cb;
+};
+
 struct spi_geni_master {
 	struct geni_se se;
 	struct device *dev;
@@ -84,6 +110,13 @@ struct spi_geni_master {
 	int irq;
 	bool cs_flag;
 	bool abort_failed;
+	struct spi_geni_gsi *gsi;
+	struct dma_chan *tx;
+	struct dma_chan *rx;
+	struct completion tx_cb;
+	struct completion rx_cb;
+	int cur_xfer_mode;
+	int num_xfers;
 };
 
 static int get_spi_clk_cfg(unsigned int speed_hz,
@@ -330,18 +363,230 @@ static int setup_fifo_params(struct spi_device *spi_slv,
 	return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz);
 }
 
+static void
+spi_gsi_callback_result(void *cb, const struct dmaengine_result *result, bool tx)
+{
+	struct gsi_desc_cb *gsi = cb;
+
+	if (result->result != DMA_TRANS_NOERROR) {
+		dev_err(gsi->mas->dev, "%s DMA txn failed\n", tx ? "TX" : "RX");
+		return;
+	}
+
+	if (!result->residue) {
+		if (tx)
+			complete(&gsi->mas->tx_cb);
+		else
+			complete(&gsi->mas->rx_cb);
+	} else {
+		dev_err(gsi->mas->dev, "%s DMA txn has pending %d data\n",
+			tx ? "TX" : "RX", result->residue);
+	}
+}
+
+static void
+spi_gsi_rx_callback_result(void *cb, const struct dmaengine_result *result)
+{
+	spi_gsi_callback_result(cb, result, false);
+}
+
+static void
+spi_gsi_tx_callback_result(void *cb, const struct dmaengine_result *result)
+{
+	spi_gsi_callback_result(cb, result, true);
+}
+
+static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas,
+			  struct spi_device *spi_slv, struct spi_master *spi)
+{
+	unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+	struct spi_geni_gsi *gsi;
+	struct dma_slave_config config = {};
+	struct gpi_spi_config peripheral = {};
+	unsigned long timeout, jiffies;
+	int ret, i;
+
+	config.peripheral_config = &peripheral;
+	config.peripheral_size = sizeof(peripheral);
+	peripheral.set_config = true;
+
+	if (xfer->bits_per_word != mas->cur_bits_per_word ||
+	    xfer->speed_hz != mas->cur_speed_hz) {
+		mas->cur_bits_per_word = xfer->bits_per_word;
+		mas->cur_speed_hz = xfer->speed_hz;
+	}
+
+	if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
+		peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word);
+	} else {
+		int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1;
+
+		peripheral.rx_len = (xfer->len / bytes_per_word);
+	}
+
+	if (xfer->tx_buf && xfer->rx_buf) {
+		peripheral.cmd = SPI_DUPLEX;
+	} else if (xfer->tx_buf) {
+		peripheral.cmd = SPI_TX;
+		peripheral.rx_len = 0;
+	} else if (xfer->rx_buf) {
+		peripheral.cmd = SPI_RX;
+	}
+
+	if (spi_slv->mode & SPI_LOOP)
+		peripheral.loopback_en = true;
+	if (spi_slv->mode & SPI_CPOL)
+		peripheral.clock_pol_high = true;
+	if (spi_slv->mode & SPI_CPHA)
+		peripheral.data_pol_high = true;
+
+	peripheral.cs = spi_slv->chip_select;
+	peripheral.pack_en = true;
+	peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
+	peripheral.fragmentation = FRAGMENTATION;
+
+	ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
+			      &peripheral.clk_src, &peripheral.clk_div);
+	if (ret) {
+		dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret);
+		return ret;
+	}
+
+	gsi = &mas->gsi[mas->num_xfers];
+	gsi->rx_cb.mas = mas;
+	gsi->rx_cb.xfer = xfer;
+
+	if (peripheral.cmd & SPI_RX) {
+		dmaengine_slave_config(mas->rx, &config);
+		gsi->rx_desc = dmaengine_prep_slave_sg(mas->rx, xfer->rx_sg.sgl, xfer->rx_sg.nents,
+						       DMA_DEV_TO_MEM, flags);
+		if (!gsi->rx_desc) {
+			dev_err(mas->dev, "Err setting up rx desc\n");
+			return -EIO;
+		}
+		gsi->rx_desc->callback_result = spi_gsi_rx_callback_result;
+		gsi->rx_desc->callback_param = &gsi->rx_cb;
+	}
+
+	dmaengine_slave_config(mas->tx, &config);
+	gsi->tx_desc = dmaengine_prep_slave_sg(mas->tx, xfer->tx_sg.sgl, xfer->tx_sg.nents,
+					       DMA_MEM_TO_DEV, flags);
+	if (!gsi->tx_desc) {
+		dev_err(mas->dev, "Err setting up tx desc\n");
+		return -EIO;
+	}
+
+	gsi->tx_cb.mas = mas;
+	gsi->tx_cb.xfer = xfer;
+	gsi->tx_desc->callback_result = spi_gsi_tx_callback_result;
+	gsi->tx_desc->callback_param = &gsi->tx_cb;
+
+	if (peripheral.cmd & SPI_RX)
+		gsi->rx_cookie = dmaengine_submit(gsi->rx_desc);
+	gsi->tx_cookie = dmaengine_submit(gsi->tx_desc);
+
+	if (peripheral.cmd & SPI_RX)
+		dma_async_issue_pending(mas->rx);
+	dma_async_issue_pending(mas->tx);
+	mas->num_xfers++;
+
+	jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
+	timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
+	if (timeout <= 0) {
+		dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);
+		ret = -ETIMEDOUT;
+		goto err_gsi_geni_transfer_one;
+	}
+
+	if (peripheral.cmd & SPI_RX) {
+		jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
+		timeout = wait_for_completion_timeout(&mas->rx_cb, jiffies);
+		if (timeout <= 0) {
+			dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout);
+			ret = -ETIMEDOUT;
+			goto err_gsi_geni_transfer_one;
+		}
+	}
+
+	spi_finalize_current_transfer(spi);
+	return 0;
+
+err_gsi_geni_transfer_one:
+	dmaengine_terminate_all(mas->tx);
+	return ret;
+}
+
+static bool geni_can_dma(struct spi_controller *ctlr,
+			 struct spi_device *slv, struct spi_transfer *xfer)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
+
+	/* check if dma is supported */
+	if (mas->cur_xfer_mode == GENI_GPI_DMA)
+		return true;
+
+	return false;
+}
+
 static int spi_geni_prepare_message(struct spi_master *spi,
 					struct spi_message *spi_msg)
 {
-	int ret;
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	int ret;
 
-	if (spi_geni_is_abort_still_pending(mas))
-		return -EBUSY;
+	switch (mas->cur_xfer_mode) {
+	case GENI_SE_FIFO:
+		if (spi_geni_is_abort_still_pending(mas))
+			return -EBUSY;
+		ret = setup_fifo_params(spi_msg->spi, spi);
+		if (ret)
+			dev_err(mas->dev, "Couldn't select mode %d\n", ret);
+		return ret;
 
-	ret = setup_fifo_params(spi_msg->spi, spi);
-	if (ret)
-		dev_err(mas->dev, "Couldn't select mode %d\n", ret);
+	case GENI_GPI_DMA:
+		mas->num_xfers = 0;
+		reinit_completion(&mas->tx_cb);
+		reinit_completion(&mas->rx_cb);
+		memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER));
+
+		return 0;
+	}
+
+	dev_err(mas->dev, "Mode not supported %d", mas->cur_xfer_mode);
+	return -EINVAL;
+}
+
+static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
+{
+	size_t gsi_sz;
+	int ret;
+
+	mas->tx = dma_request_chan(mas->dev, "tx");
+	if (IS_ERR_OR_NULL(mas->tx)) {
+		dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
+		ret = PTR_ERR(mas->tx);
+		goto err_tx;
+	}
+	mas->rx = dma_request_chan(mas->dev, "rx");
+	if (IS_ERR_OR_NULL(mas->rx)) {
+		dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx));
+		ret = PTR_ERR(mas->rx);
+		goto err_rx;
+	}
+
+	gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER;
+	mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL);
+	if (IS_ERR_OR_NULL(mas->gsi))
+		goto err_mem;
+	return 0;
+
+err_mem:
+	dma_release_channel(mas->rx);
+err_rx:
+	dma_release_channel(mas->tx);
+err_tx:
+	mas->tx = NULL;
+	mas->rx = NULL;
 	return ret;
 }
 
@@ -349,15 +594,15 @@ static int spi_geni_init(struct spi_geni_master *mas)
 {
 	struct geni_se *se = &mas->se;
 	unsigned int proto, major, minor, ver;
-	u32 spi_tx_cfg;
+	u32 spi_tx_cfg, fifo_disable;
+	int ret = -ENXIO;
 
 	pm_runtime_get_sync(mas->dev);
 
 	proto = geni_se_read_proto(se);
 	if (proto != GENI_SE_SPI) {
 		dev_err(mas->dev, "Invalid proto %d\n", proto);
-		pm_runtime_put(mas->dev);
-		return -ENXIO;
+		goto out_pm;
 	}
 	mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
 
@@ -380,15 +625,38 @@ static int spi_geni_init(struct spi_geni_master *mas)
 	else
 		mas->oversampling = 1;
 
-	geni_se_select_mode(se, GENI_SE_FIFO);
+	fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
+	switch (fifo_disable) {
+	case 1:
+		ret = spi_geni_grab_gpi_chan(mas);
+		if (!ret) { /* success case */
+			mas->cur_xfer_mode = GENI_GPI_DMA;
+			geni_se_select_mode(se, GENI_GPI_DMA);
+			dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n");
+			break;
+		}
+		/*
+		 * in case of failure to get dma channel, we can till do the
+		 * FIFO mode, so fallthrough
+		 */
+		dev_warn(mas->dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n");
+		fallthrough;
+
+	case 0:
+		mas->cur_xfer_mode = GENI_SE_FIFO;
+		geni_se_select_mode(se, GENI_SE_FIFO);
+		ret = 0;
+		break;
+	}
 
 	/* We always control CS manually */
 	spi_tx_cfg = readl(se->base + SE_SPI_TRANS_CFG);
 	spi_tx_cfg &= ~CS_TOGGLE;
 	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
 
+out_pm:
 	pm_runtime_put(mas->dev);
-	return 0;
+	return ret;
 }
 
 static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
@@ -575,8 +843,11 @@ static int spi_geni_transfer_one(struct spi_master *spi,
 	if (!xfer->len)
 		return 0;
 
-	setup_fifo_xfer(xfer, mas, slv->mode, spi);
-	return 1;
+	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
+		setup_fifo_xfer(xfer, mas, slv->mode, spi);
+		return 1;
+	}
+	return setup_gsi_xfer(xfer, mas, slv, spi);
 }
 
 static irqreturn_t geni_spi_isr(int irq, void *data)
@@ -671,6 +942,15 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (irq < 0)
 		return irq;
 
+	ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
+	if (ret) {
+		ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+		if (ret) {
+			dev_err(&pdev->dev, "could not set DMA mask\n");
+			return ret;
+		}
+	}
+
 	base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(base))
 		return PTR_ERR(base);
@@ -710,14 +990,17 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spi->max_speed_hz = 50000000;
 	spi->prepare_message = spi_geni_prepare_message;
 	spi->transfer_one = spi_geni_transfer_one;
+	spi->can_dma = geni_can_dma;
+	spi->dma_map_dev = mas->dev->parent;
 	spi->auto_runtime_pm = true;
 	spi->handle_err = handle_fifo_timeout;
-	spi->set_cs = spi_geni_set_cs;
 	spi->use_gpio_descriptors = true;
 
 	init_completion(&mas->cs_done);
 	init_completion(&mas->cancel_done);
 	init_completion(&mas->abort_done);
+	init_completion(&mas->tx_cb);
+	init_completion(&mas->rx_cb);
 	spin_lock_init(&mas->lock);
 	pm_runtime_use_autosuspend(&pdev->dev);
 	pm_runtime_set_autosuspend_delay(&pdev->dev, 250);
@@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (ret)
 		goto spi_geni_probe_runtime_disable;
 
+	/*
+	 * check the mode supported and set_cs for fifo mode only
+	 * for dma (gsi) mode, the gsi will set cs based on params passed in
+	 * TRE
+	 */
+	if (mas->cur_xfer_mode == GENI_SE_FIFO)
+		spi->set_cs = spi_geni_set_cs;
+
 	ret = request_irq(mas->irq, geni_spi_isr, 0, dev_name(dev), spi);
 	if (ret)
 		goto spi_geni_probe_runtime_disable;
@@ -754,6 +1045,14 @@ static int spi_geni_probe(struct platform_device *pdev)
 	return ret;
 }
 
+static void spi_geni_release_dma_chan(struct spi_geni_master *mas)
+{
+	if (mas->rx)
+		dma_release_channel(mas->rx);
+	if (mas->tx)
+		dma_release_channel(mas->tx);
+}
+
 static int spi_geni_remove(struct platform_device *pdev)
 {
 	struct spi_master *spi = platform_get_drvdata(pdev);
@@ -762,6 +1061,8 @@ static int spi_geni_remove(struct platform_device *pdev)
 	/* Unregister _before_ disabling pm_runtime() so we stop transfers */
 	spi_unregister_master(spi);
 
+	spi_geni_release_dma_chan(mas);
+
 	free_irq(mas->irq, spi);
 	pm_runtime_disable(&pdev->dev);
 	return 0;
-- 
2.31.1

[PATCH v3 5/5] i2c: qcom-geni: Add support for GPI DMA

[Vinod Koul]

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 | 309 ++++++++++++++++++++++++++++-
 1 file changed, 301 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/busses/i2c-qcom-geni.c b/drivers/i2c/busses/i2c-qcom-geni.c
index 07b710a774df..839802f04b75 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,17 @@ 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;
+	dma_cookie_t rx_cookie, tx_cookie;
+	dma_addr_t tx_addr;
+	dma_addr_t rx_addr;
+	bool cfg_sent;
+	struct dma_async_tx_descriptor *tx_desc;
+	struct dma_async_tx_descriptor *rx_desc;
+	void *tx_buf;
+	void *rx_buf;
+	enum i2c_se_mode se_mode;
 };
 
 struct geni_i2c_err_log {
@@ -456,6 +476,213 @@ 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)
+{
+	if (gi2c->tx_buf) {
+		dma_unmap_single(gi2c->se.dev->parent, gi2c->tx_addr, msg->len, DMA_TO_DEVICE);
+		i2c_put_dma_safe_msg_buf(gi2c->tx_buf, msg, false);
+		gi2c->tx_buf = NULL;
+	}
+
+	if (gi2c->rx_buf) {
+		dma_unmap_single(gi2c->se.dev->parent, gi2c->rx_addr, msg->len, DMA_FROM_DEVICE);
+		i2c_put_dma_safe_msg_buf(gi2c->rx_buf, msg, false);
+		gi2c->rx_buf = NULL;
+	}
+}
+
+static int geni_i2c_gpi_rx(struct geni_i2c_dev *gi2c,
+			   struct i2c_msg *msg, struct dma_slave_config *config)
+{
+	struct gpi_i2c_config *peripheral;
+	unsigned int flags;
+	void *dma_buf;
+	int ret;
+
+	peripheral = config->peripheral_config;
+
+	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+	if (!dma_buf)
+		return -ENOMEM;
+
+	gi2c->rx_addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, DMA_FROM_DEVICE);
+	if (dma_mapping_error(gi2c->se.dev->parent, gi2c->rx_addr)) {
+		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+		return -ENOMEM;
+	}
+
+	peripheral->rx_len = msg->len;
+	peripheral->op = I2C_READ;
+
+	ret = dmaengine_slave_config(gi2c->rx_c, config);
+	if (ret) {
+		dev_err(gi2c->se.dev, "rx dma config error:%d\n", ret);
+		goto err_config;
+	}
+
+	peripheral->set_config =  false;
+	peripheral->multi_msg = true;
+	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+
+	gi2c->rx_desc = dmaengine_prep_slave_single(gi2c->rx_c, gi2c->rx_addr,
+						    msg->len, DMA_DEV_TO_MEM, flags);
+	if (!gi2c->rx_desc) {
+		dev_err(gi2c->se.dev, "prep_slave_sg for rx failed\n");
+		ret = -EIO;
+		goto err_config;
+	}
+
+	gi2c->rx_desc->callback_result = i2c_gsi_cb_result;
+	gi2c->rx_desc->callback_param = gi2c;
+	gi2c->rx_buf = dma_buf;
+
+	return 0;
+
+err_config:
+	dma_unmap_single(gi2c->se.dev->parent, gi2c->rx_addr, msg->len, DMA_FROM_DEVICE);
+	i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+	return ret;
+}
+
+static int geni_i2c_gpi_tx(struct geni_i2c_dev *gi2c,
+			   struct i2c_msg *msg, struct dma_slave_config *config)
+{
+	struct gpi_i2c_config *peripheral;
+	unsigned int flags;
+	void *dma_buf;
+	int ret;
+
+	peripheral = config->peripheral_config;
+
+	dma_buf = i2c_get_dma_safe_msg_buf(msg, 1);
+	if (!dma_buf)
+		return -ENOMEM;
+
+	gi2c->tx_addr = dma_map_single(gi2c->se.dev->parent, dma_buf, msg->len, DMA_TO_DEVICE);
+	if (dma_mapping_error(gi2c->se.dev->parent, gi2c->tx_addr)) {
+		i2c_put_dma_safe_msg_buf(dma_buf, msg, false);
+		return -ENOMEM;
+	}
+
+	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);
+		goto err_config;
+	}
+
+	peripheral->set_config =  false;
+	peripheral->multi_msg = true;
+	flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+
+	gi2c->tx_desc = dmaengine_prep_slave_single(gi2c->tx_c, gi2c->tx_addr,
+						    msg->len, DMA_MEM_TO_DEV, flags);
+	if (!gi2c->tx_desc) {
+		dev_err(gi2c->se.dev, "prep_slave_sg for tx failed\n");
+		ret = -EIO;
+		goto err_config;
+	}
+
+	gi2c->tx_desc->callback_result = i2c_gsi_cb_result;
+	gi2c->tx_desc->callback_param = gi2c;
+	gi2c->tx_buf = dma_buf;
+
+	return 0;
+
+err_config:
+	dma_unmap_single(gi2c->se.dev->parent, gi2c->tx_addr, msg->len, DMA_FROM_DEVICE);
+	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;
+
+	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_rx(gi2c, &msgs[i], &config);
+			if (ret) {
+				dev_err(gi2c->se.dev, "Rx txn setting failed: %d\n", ret);
+				goto err;
+			}
+
+			/* Issue RX */
+			gi2c->rx_cookie = dmaengine_submit(gi2c->rx_desc);
+			dma_async_issue_pending(gi2c->rx_c);
+		}
+
+		ret =  geni_i2c_gpi_tx(gi2c, &msgs[i], &config);
+		if (ret) {
+			dev_err(gi2c->se.dev, "Tx txn setting failed: %d\n", ret);
+			goto err;
+		}
+
+		/* 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;
+			goto err;
+		}
+
+		geni_i2c_gpi_unmap(gi2c, &msgs[i]);
+	}
+
+	return 0;
+
+err:
+	dmaengine_terminate_sync(gi2c->rx_c);
+	dmaengine_terminate_sync(gi2c->tx_c);
+	geni_i2c_gpi_unmap(gi2c, &msgs[i]);
+	return ret;
+}
+
 static int geni_i2c_xfer(struct i2c_adapter *adap,
 			 struct i2c_msg msgs[],
 			 int num)
@@ -475,6 +702,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 +722,7 @@ static int geni_i2c_xfer(struct i2c_adapter *adap,
 		if (ret)
 			break;
 	}
+geni_i2c_txn_ret:
 	if (ret == 0)
 		ret = num;
 
@@ -517,11 +751,44 @@ 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)
+{
+	geni_se_select_mode(&gi2c->se, GENI_GPI_DMA);
+	gi2c->tx_c = dma_request_chan(gi2c->se.dev, "tx");
+	if (IS_ERR_OR_NULL(gi2c->tx_c)) {
+		dev_err(gi2c->se.dev, "TX dma_request_slave_channel fail\n");
+		return PTR_ERR(gi2c->tx_c);
+	}
+
+	gi2c->rx_c = dma_request_chan(gi2c->se.dev, "rx");
+	if (IS_ERR_OR_NULL(gi2c->rx_c)) {
+		dev_err(gi2c->se.dev, "RX dma_request_slave_channel fail\n");
+		dma_release_channel(gi2c->tx_c);
+		gi2c->tx_c = NULL;
+		return PTR_ERR(gi2c->rx_c);
+	}
+
+	dev_dbg(gi2c->se.dev, "Grabbed GPI dma channels\n");
+	return 0;
+}
+
 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,16 +868,43 @@ 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);
@@ -621,8 +915,6 @@ static int geni_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	dev_dbg(dev, "i2c fifo/se-dma mode. fifo depth:%d\n", tx_depth);
-
 	gi2c->suspended = 1;
 	pm_runtime_set_suspended(gi2c->se.dev);
 	pm_runtime_set_autosuspend_delay(gi2c->se.dev, I2C_AUTO_SUSPEND_DELAY);
@@ -645,6 +937,7 @@ 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

Re: (subset) [PATCH v3 0/5] Add and enable GPI DMA users

[Mark Brown]

On Fri, 25 Jun 2021 10:52:08 +0530, Vinod Koul wrote:
> This series adds the GPI DMA in qcom geni spi and i2c drivers.
> 
> For this we first need to move GENI_IF_DISABLE_RO to common
> headers and then add support for gpi dma in geni drivers.
> 
> Also, to reuse the dma-mapping in spi, we add a new field dma_map_dev to
> allow controllers to pass a specific device for dma-mapping
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next

Thanks!

[3/5] spi: core: add dma_map_dev for dma device
      commit: b470e10eb43f19e08245cd87dd3192a8141cfbb5

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

Re: [PATCH v3 4/5] spi: spi-geni-qcom: Add support for GPI dma

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> We can use GPI DMA for devices where it is enabled by firmware. Add
> support for this mode
>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/spi/spi-geni-qcom.c | 329 ++++++++++++++++++++++++++++++++++--
>  1 file changed, 315 insertions(+), 14 deletions(-)

Not a truly full review since I haven't looked into the whole GSI DMA
driver, but I tried to go over most of your changes.


> +#define GSI_LOOPBACK_EN                (BIT(0))
> +#define GSI_CS_TOGGLE          (BIT(3))
> +#define GSI_CPHA               (BIT(4))
> +#define GSI_CPOL               (BIT(5))
> +
> +#define MAX_TX_SG              (3)
> +#define NUM_SPI_XFER           (8)
> +#define SPI_XFER_TIMEOUT_MS    (250)

nit: don't need the extra parenthesis around everything above and it
doesn't match the existing definitions too.


> +struct gsi_desc_cb {
> +       struct spi_geni_master *mas;
> +       struct spi_transfer *xfer;
> +};
> +
> +struct spi_geni_gsi {
> +       dma_cookie_t tx_cookie;
> +       dma_cookie_t rx_cookie;
> +       struct dma_async_tx_descriptor *tx_desc;
> +       struct dma_async_tx_descriptor *rx_desc;
> +       struct gsi_desc_cb tx_cb;
> +       struct gsi_desc_cb rx_cb;
> +};

I'd be curious if others feel the same way, but to me it kinda feels
like this should be:

struct spi_geni_gsi_pipe {
  dma_cookie_t cookie;
  struct dma_async_tx_descriptor *desc;
  struct gsi_desc_cb cb;
};

struct spi_geni_gsi {
  spi_geni_gsi_pipe tx;
  spi_geni_gsi_pipe rx;
};

That would actually make spi_gsi_callback_result() more elegant
because you could pass in the correct structure and then a string "RX"
or "TX" and get rid of the bool.


> @@ -84,6 +110,13 @@ struct spi_geni_master {
>         int irq;
>         bool cs_flag;
>         bool abort_failed;
> +       struct spi_geni_gsi *gsi;

Maybe do something to make it obvious that this points to an array of
NUM_SPI_XFER transfers.

If my math is right then "gsi" is expected to be about 256 bytes big.
I guess you made this a pointer and only allocated it in GSI mode to
save those 256 bytes in non-GSI mode?


> +static void
> +spi_gsi_callback_result(void *cb, const struct dmaengine_result *result, bool tx)
> +{
> +       struct gsi_desc_cb *gsi = cb;
> +
> +       if (result->result != DMA_TRANS_NOERROR) {id'd do
> +               dev_err(gsi->mas->dev, "%s DMA txn failed\n", tx ? "TX" : "RX");
> +               return;
> +       }
> +
> +       if (!result->residue) {
> +               if (tx)
> +                       complete(&gsi->mas->tx_cb);
> +               else
> +                       complete(&gsi->mas->rx_cb);
> +       } else {
> +               dev_err(gsi->mas->dev, "%s DMA txn has pending %d data\n",
> +                       tx ? "TX" : "RX", result->residue);

I guess you just wait for the timeout in this case since you don't complete?


> +static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas,
> +                         struct spi_device *spi_slv, struct spi_master *spi)
> +{
> +       unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +       struct spi_geni_gsi *gsi;
> +       struct dma_slave_config config = {};
> +       struct gpi_spi_config peripheral = {};
> +       unsigned long timeout, jiffies;

Is it really a good idea to have a local variable called "jiffies"?
Isn't the global called "jiffies"?


> +       int ret, i;
> +
> +       config.peripheral_config = &peripheral;
> +       config.peripheral_size = sizeof(peripheral);
> +       peripheral.set_config = true;
> +
> +       if (xfer->bits_per_word != mas->cur_bits_per_word ||
> +           xfer->speed_hz != mas->cur_speed_hz) {
> +               mas->cur_bits_per_word = xfer->bits_per_word;
> +               mas->cur_speed_hz = xfer->speed_hz;
> +       }

It seems a bit pointless to copy these things into "mas" for GSI mode
and even more pointless to have the "if" test first.

In FIFO mode I believe we stored them in "mas" because we needed them
from the interrupt handler. It also lets us optimize and avoid calls
if the config didn't change. You don't need them for either of these
reasons, do you?

Actually, you might possibly want to keep "cur_speed_hz" and use it to
avoid a call to get_spi_clk_cfg() which, at least at one point in
time, was a bit slow.


> +       if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
> +               peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word);
> +       } else {
> +               int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1;
> +
> +               peripheral.rx_len = (xfer->len / bytes_per_word);
> +       }

I guess you could only do the above if "rx_buf" and then you don't
have to zero it back out below.


> +       if (xfer->tx_buf && xfer->rx_buf) {
> +               peripheral.cmd = SPI_DUPLEX;
> +       } else if (xfer->tx_buf) {
> +               peripheral.cmd = SPI_TX;
> +               peripheral.rx_len = 0;
> +       } else if (xfer->rx_buf) {
> +               peripheral.cmd = SPI_RX;
> +       }
> +
> +       if (spi_slv->mode & SPI_LOOP)
> +               peripheral.loopback_en = true;
> +       if (spi_slv->mode & SPI_CPOL)
> +               peripheral.clock_pol_high = true;
> +       if (spi_slv->mode & SPI_CPHA)
> +               peripheral.data_pol_high = true;

nit: I'd do it without the "if"s.

peripheral.loopback_en = spi_slv->mode & SPI_LOOP;

In theory much of this could also be done at function init time but I
dunno if it makes a huge difference, like:

struct gpi_spi_config peripheral = {
  .loopback_en = spi_slv->mode & SPI_LOOP,
  .clock_pol_high = spi_slv->mode & SPI_CPOL,
  ...
};


> +       peripheral.cs = spi_slv->chip_select;
> +       peripheral.pack_en = true;
> +       peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
> +       peripheral.fragmentation = FRAGMENTATION;
> +
> +       ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
> +                             &peripheral.clk_src, &peripheral.clk_div);
> +       if (ret) {
> +               dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret);
> +               return ret;
> +       }
> +
> +       gsi = &mas->gsi[mas->num_xfers];

It sure feels like you should error-check this against NUM_SPI_XFER.
Otherwise if someone does a transfer with more than 8 parts you're
totally hosed, right?

Actually, why do you even need an array here? It seems like by the
time this function exits the transfer will be all done, right? So you
can just keep using the same structure over and over again? It can
even be on the stack?


> +       gsi->rx_cb.mas = mas;
> +       gsi->rx_cb.xfer = xfer;
> +
> +       if (peripheral.cmd & SPI_RX) {
> +               dmaengine_slave_config(mas->rx, &config);
> +               gsi->rx_desc = dmaengine_prep_slave_sg(mas->rx, xfer->rx_sg.sgl, xfer->rx_sg.nents,
> +                                                      DMA_DEV_TO_MEM, flags);
> +               if (!gsi->rx_desc) {
> +                       dev_err(mas->dev, "Err setting up rx desc\n");
> +                       return -EIO;
> +               }
> +               gsi->rx_desc->callback_result = spi_gsi_rx_callback_result;
> +               gsi->rx_desc->callback_param = &gsi->rx_cb;
> +       }
> +
> +       dmaengine_slave_config(mas->tx, &config);
> +       gsi->tx_desc = dmaengine_prep_slave_sg(mas->tx, xfer->tx_sg.sgl, xfer->tx_sg.nents,
> +                                              DMA_MEM_TO_DEV, flags);
> +       if (!gsi->tx_desc) {
> +               dev_err(mas->dev, "Err setting up tx desc\n");
> +               return -EIO;
> +       }

I haven't dug deeply, but it surprises me that we do all the TX config
even if "xfer->tx_buf" is NULL.


> +       gsi->tx_cb.mas = mas;
> +       gsi->tx_cb.xfer = xfer;
> +       gsi->tx_desc->callback_result = spi_gsi_tx_callback_result;
> +       gsi->tx_desc->callback_param = &gsi->tx_cb;
> +
> +       if (peripheral.cmd & SPI_RX)
> +               gsi->rx_cookie = dmaengine_submit(gsi->rx_desc);
> +       gsi->tx_cookie = dmaengine_submit(gsi->tx_desc);
> +
> +       if (peripheral.cmd & SPI_RX)
> +               dma_async_issue_pending(mas->rx);
> +       dma_async_issue_pending(mas->tx);
> +       mas->num_xfers++;
> +
> +       jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
> +       timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);

You're waiting a hardcoded 250 ms for every transfer. That could be
too short for a long transfer. Can you use some logic like the SPI
framework?

It's probably also worth adding a comment about why you can't just
return "1" from transfer_one() for GSI mode and let the SPI framework
handle the timeout calculations. My guess is that you might need extra
time because another processor might hold the SPI bus and keep you
from starting your transfer right away and thus your timeout needs to
be somehow longer than the normal SPI framework. Is that true?

Actually, it looks like spi_transfer_wait() already has 200 ms of
slop. Is that enough for you? Can you just let the SPI framework
handle the timeout?


> +       if (timeout <= 0) {

Just "== 0". "timeout" is unsigned, right?


> +               dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);

timeout is always 0 in this printout.


> +               ret = -ETIMEDOUT;
> +               goto err_gsi_geni_transfer_one;
> +       }
> +
> +       if (peripheral.cmd & SPI_RX) {
> +               jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
> +               timeout = wait_for_completion_timeout(&mas->rx_cb, jiffies);

Why do you need to re-init the timeout? You should just wait for
however much is left from the previous wait? The TX and RX should be
happening in parallel, right? So you don't want to wait for a full
second transfer.


> +               if (timeout <= 0) {
> +                       dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout);
> +                       ret = -ETIMEDOUT;
> +                       goto err_gsi_geni_transfer_one;
> +               }
> +       }
> +
> +       spi_finalize_current_transfer(spi);
> +       return 0;
> +
> +err_gsi_geni_transfer_one:
> +       dmaengine_terminate_all(mas->tx);

I know not what I'm talking about, but the description of
dmaengine_terminate_all says "This function is DEPRECATED". It also
seems like the old "terminate_all" is async? That's bad because you
gave it stack pointers...

You almost certainly want a WARN_ON or something like that if you fail
to terminate the transfer because, presumably, the DMA engine will
keep looking at your config that's stored on the stack (that you're
freeing by ending the function). That's super bad and I'd want a
pretty serious warning in my logs if it might be happening.


> +static bool geni_can_dma(struct spi_controller *ctlr,
> +                        struct spi_device *slv, struct spi_transfer *xfer)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
> +
> +       /* check if dma is supported */
> +       if (mas->cur_xfer_mode == GENI_GPI_DMA)
> +               return true;
> +
> +       return false;
> +}

nit: might as well handle GENI_SE_DMA as well since it's just as easy
to test against GENI_SE_FIFO?

nit: no need for an if.

So just: return mas->cur_xfer_mode != GENI_SE_FIFO;


>  static int spi_geni_prepare_message(struct spi_master *spi,
>                                         struct spi_message *spi_msg)
>  {
> -       int ret;
>         struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       int ret;
>
> -       if (spi_geni_is_abort_still_pending(mas))
> -               return -EBUSY;
> +       switch (mas->cur_xfer_mode) {
> +       case GENI_SE_FIFO:
> +               if (spi_geni_is_abort_still_pending(mas))
> +                       return -EBUSY;
> +               ret = setup_fifo_params(spi_msg->spi, spi);
> +               if (ret)
> +                       dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> +               return ret;
>
> -       ret = setup_fifo_params(spi_msg->spi, spi);
> -       if (ret)
> -               dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> +       case GENI_GPI_DMA:
> +               mas->num_xfers = 0;
> +               reinit_completion(&mas->tx_cb);
> +               reinit_completion(&mas->rx_cb);
> +               memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER));

nit: make a #define for the size and use it here and in allocation?


> +static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
> +{
> +       size_t gsi_sz;
> +       int ret;
> +
> +       mas->tx = dma_request_chan(mas->dev, "tx");
> +       if (IS_ERR_OR_NULL(mas->tx)) {

I don't think dma_request_chan() can return NULL.


> +               dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
> +               ret = PTR_ERR(mas->tx);

niftier: ret = dev_err_probe(mas->dev, PTR_ERR(mas->tx), "Failed to
get tx DMA ch\n");

If you don't think that's nifty, at least you should add a "\n" to
your error message.


> +               goto err_tx;
> +       }

Personally I'm a fan of devm_add_action_or_reset() to simplify error
handling like this. Everything becomes easier to handle then.

ret = devm_add_action_or_reset(mas->dev, spi_geni_dma_release_chan, mas->tx);
if (ret)
  return ret;

You've got to make a tiny spi_geni_dma_release_chan() function that
just wraps dma_release_chan() (or cast and violate the C standard) but
then you get rid of all error handling in this function, get rid of
the code you had to add to remove and the code you forgot to add the
the probe() function (see below). :-)


> +       mas->rx = dma_request_chan(mas->dev, "rx");
> +       if (IS_ERR_OR_NULL(mas->rx)) {
> +               dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx));
> +               ret = PTR_ERR(mas->rx);
> +               goto err_rx;
> +       }
> +
> +       gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER;
> +       mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL);
> +       if (IS_ERR_OR_NULL(mas->gsi))
> +               goto err_mem;

This is not correct. kzalloc() never returns errors, just NULL. ...and
you don't set "ret".

Also: since you re-use this over and over and zero it before each
transfer, you could probably avoid the zero-allocation.


> @@ -349,15 +594,15 @@ static int spi_geni_init(struct spi_geni_master *mas)
>  {
>         struct geni_se *se = &mas->se;
>         unsigned int proto, major, minor, ver;
> -       u32 spi_tx_cfg;
> +       u32 spi_tx_cfg, fifo_disable;
> +       int ret = -ENXIO;
>
>         pm_runtime_get_sync(mas->dev);
>
>         proto = geni_se_read_proto(se);
>         if (proto != GENI_SE_SPI) {
>                 dev_err(mas->dev, "Invalid proto %d\n", proto);
> -               pm_runtime_put(mas->dev);
> -               return -ENXIO;
> +               goto out_pm;

nit: In theory this change could be relegated to a tiny cleanup patch
just to make your GPI change smaller to review, but I won't insist.


> @@ -380,15 +625,38 @@ static int spi_geni_init(struct spi_geni_master *mas)
>         else
>                 mas->oversampling = 1;
>
> -       geni_se_select_mode(se, GENI_SE_FIFO);
> +       fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +       switch (fifo_disable) {
> +       case 1:
> +               ret = spi_geni_grab_gpi_chan(mas);
> +               if (!ret) { /* success case */
> +                       mas->cur_xfer_mode = GENI_GPI_DMA;
> +                       geni_se_select_mode(se, GENI_GPI_DMA);
> +                       dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n");
> +                       break;
> +               }
> +               /*
> +                * in case of failure to get dma channel, we can till do the
> +                * FIFO mode, so fallthrough

s/till/still/

> +                */
> +               dev_warn(mas->dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n");
> +               fallthrough;

I was under the impression that if `FIFO_IF_DISABLE` was set that the
FIFO was, how shall we say it, "disabled". ;-) If you can in fact fall
back to FIFO mode then that bit seems pretty poorly named.


>  static irqreturn_t geni_spi_isr(int irq, void *data)
> @@ -671,6 +942,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>         if (irq < 0)
>                 return irq;
>
> +       ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));

nit: this function already has the line:

struct device *dev = &pdev->dev;

...so you can use "dev". :-)


> +       if (ret) {
> +               ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> +               if (ret) {
> +                       dev_err(&pdev->dev, "could not set DMA mask\n");
> +                       return ret;

niftier: return dev_err_probe(dev, ret, "could not set DMA mask\n");


> @@ -710,14 +990,17 @@ static int spi_geni_probe(struct platform_device *pdev)
>         spi->max_speed_hz = 50000000;
>         spi->prepare_message = spi_geni_prepare_message;
>         spi->transfer_one = spi_geni_transfer_one;
> +       spi->can_dma = geni_can_dma;
> +       spi->dma_map_dev = mas->dev->parent;

mas->dev->parent == dev->parent ?


> @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev)
>         if (ret)
>                 goto spi_geni_probe_runtime_disable;
>
> +       /*
> +        * check the mode supported and set_cs for fifo mode only
> +        * for dma (gsi) mode, the gsi will set cs based on params passed in
> +        * TRE
> +        */
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO)
> +               spi->set_cs = spi_geni_set_cs;

I'm curious: is there no way to get set_cs() working in GPI mode? In
an off-thread conversation Qualcomm seemed to indicate that it was
possible, but maybe they didn't quite understand what I was asking.

Without an implementation of set_cs() there will be drivers in Linux
that simply aren't compatible because they make the assumption that
they can lock the bus, set the CS, and do several transfers that are
part of some logical "transaction". I believe that both of the SPI
peripherals on boards that I work on, cros-ec and the SPI TPM make
this assumption. I don't even believe that the drivers can be "fixed"
because the requirement is more at the protocol level. The protocol
requires you to do things like:

0. Lock the bus.
1. Set the CS.
2. Transfer a few bytes, reading the response as you go.
3. Once you see the other side respond that it's ready, transfer some
more bytes.
4. Release the CS.
5. Unlock the bus.

You can't do this without a set_cs() implementation because of the
requirement to read the responses of the other side before moving on
to the next phase of the transfer.

As I understand it this is roughly the equivalent of i2c clock
stretching but much more ad-hoc and defined peripherals-by-peripheral.

In any case, I guess you must have examples of peripherals that need
GPI mode and don't need set_cs() so we shouldn't block your way
forward, but I'm just curious if you had more info on this.


>  static int spi_geni_remove(struct platform_device *pdev)
>  {
>         struct spi_master *spi = platform_get_drvdata(pdev);
> @@ -762,6 +1061,8 @@ static int spi_geni_remove(struct platform_device *pdev)
>         /* Unregister _before_ disabling pm_runtime() so we stop transfers */
>         spi_unregister_master(spi);
>
> +       spi_geni_release_dma_chan(mas);
> +

Why isn't there a call to spi_geni_release_dma_chan() in the error
paths for probe?


-Doug

Re: [PATCH v3 1/5] soc: qcom: geni: move GENI_IF_DISABLE_RO to common header

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> GENI_IF_DISABLE_RO is used by geni spi driver as well to check the
> status if GENI, so move this to common header qcom-geni-se.h
>
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 1 -
>  include/linux/qcom-geni-se.h    | 4 ++++
>  2 files changed, 4 insertions(+), 1 deletion(-)

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v3 2/5] soc: qcom: geni: Add support for gpi dma

[Doug Anderson]

Hi,

On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote:
>
> +static void geni_se_select_gpi_mode(struct geni_se *se)
> +{
> +       u32 val;
> +
> +       geni_se_irq_clear(se);
> +
> +       writel(0, se->base + SE_IRQ_EN);
> +
> +       val = readl(se->base + SE_GENI_S_IRQ_EN);
> +       val &= ~S_CMD_DONE_EN;
> +       writel(val, se->base + SE_GENI_S_IRQ_EN);
> +
> +       val = readl(se->base + SE_GENI_M_IRQ_EN);
> +       val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> +                M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> +       writel(val, se->base + SE_GENI_M_IRQ_EN);
> +
> +       writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
> +
> +       val = readl(se->base + SE_GSI_EVENT_EN);
> +       val |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | GENI_M_EVENT_EN | GENI_S_EVENT_EN);

nit: the above has some extra parenthesis that aren't needed.

I will continue to assert that all of the "set mode" stuff doesn't
really belong here and should be managed by individual drivers [1].
I'll accept that it doesn't have to block forward progress, though I'm
at least a bit disappointed that we asked Qualcomm to do this over 8
months ago and no action was taken. :(

In any case, this looks OK to me:

Reviewed-by: Douglas Anderson <dianders@chromium.org>


[1] https://lore.kernel.org/r/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/

Re: [PATCH v3 2/5] soc: qcom: geni: Add support for gpi dma

[Vinod Koul]

Hi Doug,

On 28-06-21, 16:38, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > +static void geni_se_select_gpi_mode(struct geni_se *se)
> > +{
> > +       u32 val;
> > +
> > +       geni_se_irq_clear(se);
> > +
> > +       writel(0, se->base + SE_IRQ_EN);
> > +
> > +       val = readl(se->base + SE_GENI_S_IRQ_EN);
> > +       val &= ~S_CMD_DONE_EN;
> > +       writel(val, se->base + SE_GENI_S_IRQ_EN);
> > +
> > +       val = readl(se->base + SE_GENI_M_IRQ_EN);
> > +       val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> > +                M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> > +       writel(val, se->base + SE_GENI_M_IRQ_EN);
> > +
> > +       writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
> > +
> > +       val = readl(se->base + SE_GSI_EVENT_EN);
> > +       val |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> 
> nit: the above has some extra parenthesis that aren't needed.
> 
> I will continue to assert that all of the "set mode" stuff doesn't
> really belong here and should be managed by individual drivers [1].
> I'll accept that it doesn't have to block forward progress, though I'm
> at least a bit disappointed that we asked Qualcomm to do this over 8
> months ago and no action was taken. :(
> 
> In any case, this looks OK to me:
> 
> Reviewed-by: Douglas Anderson <dianders@chromium.org>

Thanks for the review.

> 
> [1] https://lore.kernel.org/r/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/

I agree we should do something, will discuss with Bjorn and try to help
here.

Regards
-- 
~Vinod

Re: [PATCH v3 4/5] spi: spi-geni-qcom: Add support for GPI dma

[Vinod Koul]

Hi Doug,

On 28-06-21, 16:37, Doug Anderson wrote:
> Hi,
> 
> On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > We can use GPI DMA for devices where it is enabled by firmware. Add
> > support for this mode
> >
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/spi/spi-geni-qcom.c | 329 ++++++++++++++++++++++++++++++++++--
> >  1 file changed, 315 insertions(+), 14 deletions(-)
> 
> Not a truly full review since I haven't looked into the whole GSI DMA
> driver, but I tried to go over most of your changes.
> 
> 
> > +#define GSI_LOOPBACK_EN                (BIT(0))
> > +#define GSI_CS_TOGGLE          (BIT(3))
> > +#define GSI_CPHA               (BIT(4))
> > +#define GSI_CPOL               (BIT(5))
> > +
> > +#define MAX_TX_SG              (3)
> > +#define NUM_SPI_XFER           (8)
> > +#define SPI_XFER_TIMEOUT_MS    (250)
> 
> nit: don't need the extra parenthesis around everything above and it
> doesn't match the existing definitions too.

yes, fixed it

> > +struct gsi_desc_cb {
> > +       struct spi_geni_master *mas;
> > +       struct spi_transfer *xfer;
> > +};
> > +
> > +struct spi_geni_gsi {
> > +       dma_cookie_t tx_cookie;
> > +       dma_cookie_t rx_cookie;
> > +       struct dma_async_tx_descriptor *tx_desc;
> > +       struct dma_async_tx_descriptor *rx_desc;
> > +       struct gsi_desc_cb tx_cb;
> > +       struct gsi_desc_cb rx_cb;
> > +};
> 
> I'd be curious if others feel the same way, but to me it kinda feels
> like this should be:
> 
> struct spi_geni_gsi_pipe {
>   dma_cookie_t cookie;
>   struct dma_async_tx_descriptor *desc;
>   struct gsi_desc_cb cb;
> };
> 
> struct spi_geni_gsi {
>   spi_geni_gsi_pipe tx;
>   spi_geni_gsi_pipe rx;
> };
> 
> That would actually make spi_gsi_callback_result() more elegant
> because you could pass in the correct structure and then a string "RX"
> or "TX" and get rid of the bool.

So this and the below comment about using spi fwk to do that wait, made
me ponder upon this bit. So I went ahead with scissors and managed to
chop this off as well as the waiting routines below. This makes code
simpler to read as well.

So we dont need these structs here and wait code anymore.

> > @@ -84,6 +110,13 @@ struct spi_geni_master {
> >         int irq;
> >         bool cs_flag;
> >         bool abort_failed;
> > +       struct spi_geni_gsi *gsi;
> 
> Maybe do something to make it obvious that this points to an array of
> NUM_SPI_XFER transfers.

> If my math is right then "gsi" is expected to be about 256 bytes big.
> I guess you made this a pointer and only allocated it in GSI mode to
> save those 256 bytes in non-GSI mode?
> > +static void
> > +spi_gsi_callback_result(void *cb, const struct dmaengine_result *result, bool tx)
> > +{
> > +       struct gsi_desc_cb *gsi = cb;
> > +
> > +       if (result->result != DMA_TRANS_NOERROR) {id'd do
> > +               dev_err(gsi->mas->dev, "%s DMA txn failed\n", tx ? "TX" : "RX");
> > +               return;
> > +       }
> > +
> > +       if (!result->residue) {
> > +               if (tx)
> > +                       complete(&gsi->mas->tx_cb);
> > +               else
> > +                       complete(&gsi->mas->rx_cb);
> > +       } else {
> > +               dev_err(gsi->mas->dev, "%s DMA txn has pending %d data\n",
> > +                       tx ? "TX" : "RX", result->residue);
> 
> I guess you just wait for the timeout in this case since you don't complete?

> > +static int setup_gsi_xfer(struct spi_transfer *xfer, struct spi_geni_master *mas,
> > +                         struct spi_device *spi_slv, struct spi_master *spi)
> > +{
> > +       unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> > +       struct spi_geni_gsi *gsi;
> > +       struct dma_slave_config config = {};
> > +       struct gpi_spi_config peripheral = {};
> > +       unsigned long timeout, jiffies;
> 
> Is it really a good idea to have a local variable called "jiffies"?
> Isn't the global called "jiffies"?

removed now

> > +       int ret, i;
> > +
> > +       config.peripheral_config = &peripheral;
> > +       config.peripheral_size = sizeof(peripheral);
> > +       peripheral.set_config = true;
> > +
> > +       if (xfer->bits_per_word != mas->cur_bits_per_word ||
> > +           xfer->speed_hz != mas->cur_speed_hz) {
> > +               mas->cur_bits_per_word = xfer->bits_per_word;
> > +               mas->cur_speed_hz = xfer->speed_hz;
> > +       }
> 
> It seems a bit pointless to copy these things into "mas" for GSI mode
> and even more pointless to have the "if" test first.
> 
> In FIFO mode I believe we stored them in "mas" because we needed them
> from the interrupt handler. It also lets us optimize and avoid calls
> if the config didn't change. You don't need them for either of these
> reasons, do you?

agreed, cleaned that up

> Actually, you might possibly want to keep "cur_speed_hz" and use it to
> avoid a call to get_spi_clk_cfg() which, at least at one point in
> time, was a bit slow.

Okay, thanks for suggestion, I can do that.

> > +       if (!(mas->cur_bits_per_word % MIN_WORD_LEN)) {
> > +               peripheral.rx_len = ((xfer->len << 3) / mas->cur_bits_per_word);
> > +       } else {
> > +               int bytes_per_word = (mas->cur_bits_per_word / BITS_PER_BYTE) + 1;
> > +
> > +               peripheral.rx_len = (xfer->len / bytes_per_word);
> > +       }
> 
> I guess you could only do the above if "rx_buf" and then you don't
> have to zero it back out below.

yes updated

> > +       if (xfer->tx_buf && xfer->rx_buf) {
> > +               peripheral.cmd = SPI_DUPLEX;
> > +       } else if (xfer->tx_buf) {
> > +               peripheral.cmd = SPI_TX;
> > +               peripheral.rx_len = 0;
> > +       } else if (xfer->rx_buf) {
> > +               peripheral.cmd = SPI_RX;
> > +       }
> > +
> > +       if (spi_slv->mode & SPI_LOOP)
> > +               peripheral.loopback_en = true;
> > +       if (spi_slv->mode & SPI_CPOL)
> > +               peripheral.clock_pol_high = true;
> > +       if (spi_slv->mode & SPI_CPHA)
> > +               peripheral.data_pol_high = true;
> 
> nit: I'd do it without the "if"s.
> 
> peripheral.loopback_en = spi_slv->mode & SPI_LOOP;

Good suggestion, updated now.

> In theory much of this could also be done at function init time but I
> dunno if it makes a huge difference, like:
> 
> struct gpi_spi_config peripheral = {
>   .loopback_en = spi_slv->mode & SPI_LOOP,
>   .clock_pol_high = spi_slv->mode & SPI_CPOL,
>   ...
> };

Still kept it runtime, lets me know in next rev if we can do better :)

> > +       peripheral.cs = spi_slv->chip_select;
> > +       peripheral.pack_en = true;
> > +       peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
> > +       peripheral.fragmentation = FRAGMENTATION;
> > +
> > +       ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
> > +                             &peripheral.clk_src, &peripheral.clk_div);
> > +       if (ret) {
> > +               dev_err(mas->dev, "Err in get_spi_clk_cfg() :%d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       gsi = &mas->gsi[mas->num_xfers];
> 
> It sure feels like you should error-check this against NUM_SPI_XFER.
> Otherwise if someone does a transfer with more than 8 parts you're
> totally hosed, right?
> 
> Actually, why do you even need an array here? It seems like by the
> time this function exits the transfer will be all done, right? So you
> can just keep using the same structure over and over again? It can
> even be on the stack?

This was the trigger so things are on stack now and cleaned, pls see
next rev, should be out in next few days

> > +       gsi->rx_cb.mas = mas;
> > +       gsi->rx_cb.xfer = xfer;
> > +
> > +       if (peripheral.cmd & SPI_RX) {
> > +               dmaengine_slave_config(mas->rx, &config);
> > +               gsi->rx_desc = dmaengine_prep_slave_sg(mas->rx, xfer->rx_sg.sgl, xfer->rx_sg.nents,
> > +                                                      DMA_DEV_TO_MEM, flags);
> > +               if (!gsi->rx_desc) {
> > +                       dev_err(mas->dev, "Err setting up rx desc\n");
> > +                       return -EIO;
> > +               }
> > +               gsi->rx_desc->callback_result = spi_gsi_rx_callback_result;
> > +               gsi->rx_desc->callback_param = &gsi->rx_cb;
> > +       }
> > +
> > +       dmaengine_slave_config(mas->tx, &config);
> > +       gsi->tx_desc = dmaengine_prep_slave_sg(mas->tx, xfer->tx_sg.sgl, xfer->tx_sg.nents,
> > +                                              DMA_MEM_TO_DEV, flags);
> > +       if (!gsi->tx_desc) {
> > +               dev_err(mas->dev, "Err setting up tx desc\n");
> > +               return -EIO;
> > +       }
> 
> I haven't dug deeply, but it surprises me that we do all the TX config
> even if "xfer->tx_buf" is NULL.

GPI DMA :) We _always_ need TX even for RX. I am adding a note here for
that

> > +       gsi->tx_cb.mas = mas;
> > +       gsi->tx_cb.xfer = xfer;
> > +       gsi->tx_desc->callback_result = spi_gsi_tx_callback_result;
> > +       gsi->tx_desc->callback_param = &gsi->tx_cb;
> > +
> > +       if (peripheral.cmd & SPI_RX)
> > +               gsi->rx_cookie = dmaengine_submit(gsi->rx_desc);
> > +       gsi->tx_cookie = dmaengine_submit(gsi->tx_desc);
> > +
> > +       if (peripheral.cmd & SPI_RX)
> > +               dma_async_issue_pending(mas->rx);
> > +       dma_async_issue_pending(mas->tx);
> > +       mas->num_xfers++;
> > +
> > +       jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
> > +       timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
> 
> You're waiting a hardcoded 250 ms for every transfer. That could be
> too short for a long transfer. Can you use some logic like the SPI
> framework?
> 
> It's probably also worth adding a comment about why you can't just
> return "1" from transfer_one() for GSI mode and let the SPI framework
> handle the timeout calculations. My guess is that you might need extra
> time because another processor might hold the SPI bus and keep you
> from starting your transfer right away and thus your timeout needs to
> be somehow longer than the normal SPI framework. Is that true?
> 
> Actually, it looks like spi_transfer_wait() already has 200 ms of
> slop. Is that enough for you? Can you just let the SPI framework
> handle the timeout?

I have removed the code and returned 1 :)

> 
> > +       if (timeout <= 0) {
> 
> Just "== 0". "timeout" is unsigned, right?
> 
> 
> > +               dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);
> 
> timeout is always 0 in this printout.
> > +               ret = -ETIMEDOUT;
> > +               goto err_gsi_geni_transfer_one;
> > +       }
> > +
> > +       if (peripheral.cmd & SPI_RX) {
> > +               jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
> > +               timeout = wait_for_completion_timeout(&mas->rx_cb, jiffies);
> 
> Why do you need to re-init the timeout? You should just wait for
> however much is left from the previous wait? The TX and RX should be
> happening in parallel, right? So you don't want to wait for a full
> second transfer.
> 
> > +               if (timeout <= 0) {
> > +                       dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout);
> > +                       ret = -ETIMEDOUT;
> > +                       goto err_gsi_geni_transfer_one;
> > +               }
> > +       }
> > +
> > +       spi_finalize_current_transfer(spi);
> > +       return 0;
> > +
> > +err_gsi_geni_transfer_one:
> > +       dmaengine_terminate_all(mas->tx);
> 
> I know not what I'm talking about, but the description of
> dmaengine_terminate_all says "This function is DEPRECATED". It also
> seems like the old "terminate_all" is async? That's bad because you
> gave it stack pointers...
> 
> You almost certainly want a WARN_ON or something like that if you fail
> to terminate the transfer because, presumably, the DMA engine will
> keep looking at your config that's stored on the stack (that you're
> freeing by ending the function). That's super bad and I'd want a
> pretty serious warning in my logs if it might be happening.

Oops, somehow this got missed, we are supposed to use
dmaengine_terminate_sync() here... I think I have updated i2c one but
missed here

> > +static bool geni_can_dma(struct spi_controller *ctlr,
> > +                        struct spi_device *slv, struct spi_transfer *xfer)
> > +{
> > +       struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
> > +
> > +       /* check if dma is supported */
> > +       if (mas->cur_xfer_mode == GENI_GPI_DMA)
> > +               return true;
> > +
> > +       return false;
> > +}
> 
> nit: might as well handle GENI_SE_DMA as well since it's just as easy
> to test against GENI_SE_FIFO?

I think I will leave that for the person adding GENI_SE_DMA support :)

> nit: no need for an if.
> 
> So just: return mas->cur_xfer_mode != GENI_SE_FIFO;

Done

> >  static int spi_geni_prepare_message(struct spi_master *spi,
> >                                         struct spi_message *spi_msg)
> >  {
> > -       int ret;
> >         struct spi_geni_master *mas = spi_master_get_devdata(spi);
> > +       int ret;
> >
> > -       if (spi_geni_is_abort_still_pending(mas))
> > -               return -EBUSY;
> > +       switch (mas->cur_xfer_mode) {
> > +       case GENI_SE_FIFO:
> > +               if (spi_geni_is_abort_still_pending(mas))
> > +                       return -EBUSY;
> > +               ret = setup_fifo_params(spi_msg->spi, spi);
> > +               if (ret)
> > +                       dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> > +               return ret;
> >
> > -       ret = setup_fifo_params(spi_msg->spi, spi);
> > -       if (ret)
> > -               dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> > +       case GENI_GPI_DMA:
> > +               mas->num_xfers = 0;
> > +               reinit_completion(&mas->tx_cb);
> > +               reinit_completion(&mas->rx_cb);
> > +               memset(mas->gsi, 0, (sizeof(struct spi_geni_gsi) * NUM_SPI_XFER));
> 
> nit: make a #define for the size and use it here and in allocation?

Removed the struct so no longer needed

> > +static int spi_geni_grab_gpi_chan(struct spi_geni_master *mas)
> > +{
> > +       size_t gsi_sz;
> > +       int ret;
> > +
> > +       mas->tx = dma_request_chan(mas->dev, "tx");
> > +       if (IS_ERR_OR_NULL(mas->tx)) {
> 
> I don't think dma_request_chan() can return NULL.

No it cant, updated now!

> 
> 
> > +               dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
> > +               ret = PTR_ERR(mas->tx);
> 
> niftier: ret = dev_err_probe(mas->dev, PTR_ERR(mas->tx), "Failed to
> get tx DMA ch\n");
> 
> If you don't think that's nifty, at least you should add a "\n" to
> your error message.

I like dev_err_probe(), updated along with missing "\n"
> 
> 
> > +               goto err_tx;
> > +       }
> 
> Personally I'm a fan of devm_add_action_or_reset() to simplify error
> handling like this. Everything becomes easier to handle then.
> 
> ret = devm_add_action_or_reset(mas->dev, spi_geni_dma_release_chan, mas->tx);
> if (ret)
>   return ret;
> 
> You've got to make a tiny spi_geni_dma_release_chan() function that
> just wraps dma_release_chan() (or cast and violate the C standard) but
> then you get rid of all error handling in this function, get rid of
> the code you had to add to remove and the code you forgot to add the
> the probe() function (see below). :-)

So after reworking code, there is little error handling, so I am
skipping it for next rev

> > +       mas->rx = dma_request_chan(mas->dev, "rx");
> > +       if (IS_ERR_OR_NULL(mas->rx)) {
> > +               dev_err(mas->dev, "Failed to get rx DMA ch %ld", PTR_ERR(mas->rx));
> > +               ret = PTR_ERR(mas->rx);
> > +               goto err_rx;
> > +       }
> > +
> > +       gsi_sz = sizeof(struct spi_geni_gsi) * NUM_SPI_XFER;
> > +       mas->gsi = devm_kzalloc(mas->dev, gsi_sz, GFP_KERNEL);
> > +       if (IS_ERR_OR_NULL(mas->gsi))
> > +               goto err_mem;
> 
> This is not correct. kzalloc() never returns errors, just NULL. ...and
> you don't set "ret".
> 
> Also: since you re-use this over and over and zero it before each
> transfer, you could probably avoid the zero-allocation.
> 
> 
> > @@ -349,15 +594,15 @@ static int spi_geni_init(struct spi_geni_master *mas)
> >  {
> >         struct geni_se *se = &mas->se;
> >         unsigned int proto, major, minor, ver;
> > -       u32 spi_tx_cfg;
> > +       u32 spi_tx_cfg, fifo_disable;
> > +       int ret = -ENXIO;
> >
> >         pm_runtime_get_sync(mas->dev);
> >
> >         proto = geni_se_read_proto(se);
> >         if (proto != GENI_SE_SPI) {
> >                 dev_err(mas->dev, "Invalid proto %d\n", proto);
> > -               pm_runtime_put(mas->dev);
> > -               return -ENXIO;
> > +               goto out_pm;
> 
> nit: In theory this change could be relegated to a tiny cleanup patch
> just to make your GPI change smaller to review, but I won't insist.

This is looking better in next rev :)

> > @@ -380,15 +625,38 @@ static int spi_geni_init(struct spi_geni_master *mas)
> >         else
> >                 mas->oversampling = 1;
> >
> > -       geni_se_select_mode(se, GENI_SE_FIFO);
> > +       fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> > +       switch (fifo_disable) {
> > +       case 1:
> > +               ret = spi_geni_grab_gpi_chan(mas);
> > +               if (!ret) { /* success case */
> > +                       mas->cur_xfer_mode = GENI_GPI_DMA;
> > +                       geni_se_select_mode(se, GENI_GPI_DMA);
> > +                       dev_dbg(mas->dev, "Using GPI DMA mode for SPI\n");
> > +                       break;
> > +               }
> > +               /*
> > +                * in case of failure to get dma channel, we can till do the
> > +                * FIFO mode, so fallthrough
> 
> s/till/still/

ack

> 
> > +                */
> > +               dev_warn(mas->dev, "FIFO mode disabled, but couldn't get DMA, fall back to FIFO mode\n");
> > +               fallthrough;
> 
> I was under the impression that if `FIFO_IF_DISABLE` was set that the
> FIFO was, how shall we say it, "disabled". ;-) If you can in fact fall
> back to FIFO mode then that bit seems pretty poorly named.

So on few places I have seen fall back work and some it doesn't so name
seems correct. Now the question is should we _try_ to fallback on
disabled fifo or not.. :)

> >  static irqreturn_t geni_spi_isr(int irq, void *data)
> > @@ -671,6 +942,15 @@ static int spi_geni_probe(struct platform_device *pdev)
> >         if (irq < 0)
> >                 return irq;
> >
> > +       ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64));
> 
> nit: this function already has the line:
> 
> struct device *dev = &pdev->dev;
> 
> ...so you can use "dev". :-)

updated here and few more places


> 
> 
> > +       if (ret) {
> > +               ret = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
> > +               if (ret) {
> > +                       dev_err(&pdev->dev, "could not set DMA mask\n");
> > +                       return ret;
> 
> niftier: return dev_err_probe(dev, ret, "could not set DMA mask\n");
> 
> 
> > @@ -710,14 +990,17 @@ static int spi_geni_probe(struct platform_device *pdev)
> >         spi->max_speed_hz = 50000000;
> >         spi->prepare_message = spi_geni_prepare_message;
> >         spi->transfer_one = spi_geni_transfer_one;
> > +       spi->can_dma = geni_can_dma;
> > +       spi->dma_map_dev = mas->dev->parent;
> 
> mas->dev->parent == dev->parent ?
> 
> 
> > @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto spi_geni_probe_runtime_disable;
> >
> > +       /*
> > +        * check the mode supported and set_cs for fifo mode only
> > +        * for dma (gsi) mode, the gsi will set cs based on params passed in
> > +        * TRE
> > +        */
> > +       if (mas->cur_xfer_mode == GENI_SE_FIFO)
> > +               spi->set_cs = spi_geni_set_cs;
> 
> I'm curious: is there no way to get set_cs() working in GPI mode? In
> an off-thread conversation Qualcomm seemed to indicate that it was
> possible, but maybe they didn't quite understand what I was asking.
> 
> Without an implementation of set_cs() there will be drivers in Linux
> that simply aren't compatible because they make the assumption that
> they can lock the bus, set the CS, and do several transfers that are
> part of some logical "transaction". I believe that both of the SPI
> peripherals on boards that I work on, cros-ec and the SPI TPM make
> this assumption. I don't even believe that the drivers can be "fixed"
> because the requirement is more at the protocol level. The protocol
> requires you to do things like:
> 
> 0. Lock the bus.
> 1. Set the CS.
> 2. Transfer a few bytes, reading the response as you go.
> 3. Once you see the other side respond that it's ready, transfer some
> more bytes.
> 4. Release the CS.
> 5. Unlock the bus.
> 
> You can't do this without a set_cs() implementation because of the
> requirement to read the responses of the other side before moving on
> to the next phase of the transfer.
> 
> As I understand it this is roughly the equivalent of i2c clock
> stretching but much more ad-hoc and defined peripherals-by-peripheral.
> 
> In any case, I guess you must have examples of peripherals that need
> GPI mode and don't need set_cs() so we shouldn't block your way
> forward, but I'm just curious if you had more info on this.

So I have asked some qcom folks, they tell me it is _possible_ to use
the cs bit in the TRE and it can work. But TBH I am not yet convinced it
would work as advertised. So do you enable the GPI mode for chrome books
or it is SE DMA mode (i think SE DMA mode might be simpler to use for
your case)

> 
> 
> >  static int spi_geni_remove(struct platform_device *pdev)
> >  {
> >         struct spi_master *spi = platform_get_drvdata(pdev);
> > @@ -762,6 +1061,8 @@ static int spi_geni_remove(struct platform_device *pdev)
> >         /* Unregister _before_ disabling pm_runtime() so we stop transfers */
> >         spi_unregister_master(spi);
> >
> > +       spi_geni_release_dma_chan(mas);
> > +
> 
> Why isn't there a call to spi_geni_release_dma_chan() in the error
> paths for probe?

Fixed

-- 
~Vinod

Re: [PATCH v3 4/5] spi: spi-geni-qcom: Add support for GPI dma

[Doug Anderson]

Hi,


On Thu, Oct 14, 2021 at 9:04 AM Vinod Koul <vkoul@kernel.org> wrote:
> > > +static bool geni_can_dma(struct spi_controller *ctlr,
> > > +                        struct spi_device *slv, struct spi_transfer *xfer)
> > > +{
> > > +       struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
> > > +
> > > +       /* check if dma is supported */
> > > +       if (mas->cur_xfer_mode == GENI_GPI_DMA)
> > > +               return true;
> > > +
> > > +       return false;
> > > +}
> >
> > nit: might as well handle GENI_SE_DMA as well since it's just as easy
> > to test against GENI_SE_FIFO?
>
> I think I will leave that for the person adding GENI_SE_DMA support :)

I was just thinking of changing the "if" statement:

if (mas->cur_xfer_mode != GENI_SE_FIFO)

It's no skin off your teeth and would make one fewer line to change
if/when the other DMA mode is supported. In any case, I won't push it.
;-)


> > > @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev)
> > >         if (ret)
> > >                 goto spi_geni_probe_runtime_disable;
> > >
> > > +       /*
> > > +        * check the mode supported and set_cs for fifo mode only
> > > +        * for dma (gsi) mode, the gsi will set cs based on params passed in
> > > +        * TRE
> > > +        */
> > > +       if (mas->cur_xfer_mode == GENI_SE_FIFO)
> > > +               spi->set_cs = spi_geni_set_cs;
> >
> > I'm curious: is there no way to get set_cs() working in GPI mode? In
> > an off-thread conversation Qualcomm seemed to indicate that it was
> > possible, but maybe they didn't quite understand what I was asking.
> >
> > Without an implementation of set_cs() there will be drivers in Linux
> > that simply aren't compatible because they make the assumption that
> > they can lock the bus, set the CS, and do several transfers that are
> > part of some logical "transaction". I believe that both of the SPI
> > peripherals on boards that I work on, cros-ec and the SPI TPM make
> > this assumption. I don't even believe that the drivers can be "fixed"
> > because the requirement is more at the protocol level. The protocol
> > requires you to do things like:
> >
> > 0. Lock the bus.
> > 1. Set the CS.
> > 2. Transfer a few bytes, reading the response as you go.
> > 3. Once you see the other side respond that it's ready, transfer some
> > more bytes.
> > 4. Release the CS.
> > 5. Unlock the bus.
> >
> > You can't do this without a set_cs() implementation because of the
> > requirement to read the responses of the other side before moving on
> > to the next phase of the transfer.
> >
> > As I understand it this is roughly the equivalent of i2c clock
> > stretching but much more ad-hoc and defined peripherals-by-peripheral.
> >
> > In any case, I guess you must have examples of peripherals that need
> > GPI mode and don't need set_cs() so we shouldn't block your way
> > forward, but I'm just curious if you had more info on this.
>
> So I have asked some qcom folks, they tell me it is _possible_ to use
> the cs bit in the TRE and it can work. But TBH I am not yet convinced it
> would work as advertised. So do you enable the GPI mode for chrome books
> or it is SE DMA mode (i think SE DMA mode might be simpler to use for
> your case)

Sounds promising. I'm curious about why you're not convinced it would
work as advertised? Right now we have all our SPI devices running in
FIFO mode (!) and we've been trying to find a way to get them in DMA
mode. I think Qualcomm is trying to avoid supporting both SE DMA and
GPI DMA mode so they are saying that once GPI mode works then we can
just use that.

I don't really have a huge objection to that, but I also have zero
experience with GPI DMA mode. We don't have any need (at the moment)
to share our SPI bus with multiple execution environments, but it
seems like GPI mode _could_ still work as long as the chip select
problem is solved. I did manage to get some more documentation and I
do see a "LOCK" command, so maybe that combined with leaving the CS
asserted would solve the problem? Maybe Mark would allow your driver
to get called from spi_bus_lock() and spi_bus_unlock(). That seems
like it would be important to do anyway to match the Linux SPI client
model...

NOTE: in reality, we sorta paper over the "chip select" problem anyway
on Chromebooks. We just configure the chip select lines as GPIOs and
let Linux manage them. There is much less overhead in setting a GPIO
compared to messaging a QUP, so this improves performance. As long as
this continues to work, perhaps we don't care about whether we can
really tell GPI mode to leave the CS asserted.

-Doug

Re: [PATCH v3 4/5] spi: spi-geni-qcom: Add support for GPI dma

[Vinod Koul]

Hi,

On 14-10-21, 09:55, Doug Anderson wrote:
> On Thu, Oct 14, 2021 at 9:04 AM Vinod Koul <vkoul@kernel.org> wrote:
> > > > +static bool geni_can_dma(struct spi_controller *ctlr,
> > > > +                        struct spi_device *slv, struct spi_transfer *xfer)
> > > > +{
> > > > +       struct spi_geni_master *mas = spi_master_get_devdata(slv->master);
> > > > +
> > > > +       /* check if dma is supported */
> > > > +       if (mas->cur_xfer_mode == GENI_GPI_DMA)
> > > > +               return true;
> > > > +
> > > > +       return false;
> > > > +}
> > >
> > > nit: might as well handle GENI_SE_DMA as well since it's just as easy
> > > to test against GENI_SE_FIFO?
> >
> > I think I will leave that for the person adding GENI_SE_DMA support :)
> 
> I was just thinking of changing the "if" statement:
> 
> if (mas->cur_xfer_mode != GENI_SE_FIFO)
> 
> It's no skin off your teeth and would make one fewer line to change
> if/when the other DMA mode is supported. In any case, I won't push it.
> ;-)

I can do that!

> > > > @@ -738,6 +1021,14 @@ static int spi_geni_probe(struct platform_device *pdev)
> > > >         if (ret)
> > > >                 goto spi_geni_probe_runtime_disable;
> > > >
> > > > +       /*
> > > > +        * check the mode supported and set_cs for fifo mode only
> > > > +        * for dma (gsi) mode, the gsi will set cs based on params passed in
> > > > +        * TRE
> > > > +        */
> > > > +       if (mas->cur_xfer_mode == GENI_SE_FIFO)
> > > > +               spi->set_cs = spi_geni_set_cs;
> > >
> > > I'm curious: is there no way to get set_cs() working in GPI mode? In
> > > an off-thread conversation Qualcomm seemed to indicate that it was
> > > possible, but maybe they didn't quite understand what I was asking.

I dont think so, as the GPI firmware controls CS here. IIRC set_cs() was
causing issues with gpi mode

> > > Without an implementation of set_cs() there will be drivers in Linux
> > > that simply aren't compatible because they make the assumption that
> > > they can lock the bus, set the CS, and do several transfers that are
> > > part of some logical "transaction". I believe that both of the SPI
> > > peripherals on boards that I work on, cros-ec and the SPI TPM make
> > > this assumption. I don't even believe that the drivers can be "fixed"
> > > because the requirement is more at the protocol level. The protocol
> > > requires you to do things like:
> > >
> > > 0. Lock the bus.
> > > 1. Set the CS.
> > > 2. Transfer a few bytes, reading the response as you go.
> > > 3. Once you see the other side respond that it's ready, transfer some
> > > more bytes.
> > > 4. Release the CS.
> > > 5. Unlock the bus.
> > >
> > > You can't do this without a set_cs() implementation because of the
> > > requirement to read the responses of the other side before moving on
> > > to the next phase of the transfer.
> > >
> > > As I understand it this is roughly the equivalent of i2c clock
> > > stretching but much more ad-hoc and defined peripherals-by-peripheral.
> > >
> > > In any case, I guess you must have examples of peripherals that need
> > > GPI mode and don't need set_cs() so we shouldn't block your way
> > > forward, but I'm just curious if you had more info on this.

Yes, I am testing with CAN bus on Rb3.

> > So I have asked some qcom folks, they tell me it is _possible_ to use
> > the cs bit in the TRE and it can work. But TBH I am not yet convinced it
> > would work as advertised. So do you enable the GPI mode for chrome books
> > or it is SE DMA mode (i think SE DMA mode might be simpler to use for
> > your case)
> 
> Sounds promising. I'm curious about why you're not convinced it would
> work as advertised? Right now we have all our SPI devices running in
> FIFO mode (!) and we've been trying to find a way to get them in DMA
> mode. I think Qualcomm is trying to avoid supporting both SE DMA and
> GPI DMA mode so they are saying that once GPI mode works then we can
> just use that.
> 
> I don't really have a huge objection to that, but I also have zero
> experience with GPI DMA mode. We don't have any need (at the moment)
> to share our SPI bus with multiple execution environments, but it
> seems like GPI mode _could_ still work as long as the chip select
> problem is solved. I did manage to get some more documentation and I
> do see a "LOCK" command, so maybe that combined with leaving the CS
> asserted would solve the problem? Maybe Mark would allow your driver
> to get called from spi_bus_lock() and spi_bus_unlock(). That seems
> like it would be important to do anyway to match the Linux SPI client
> model...

We can add lock support, but IIUC it was to prevent other EEs from
running, it may not help with chip select. It also needs someone loading
firmware on the controller.

For us, since some boards are shipping with firmware loaded, fifo and se
dma mode will not work, only option is to get gpi mode working.

> NOTE: in reality, we sorta paper over the "chip select" problem anyway
> on Chromebooks. We just configure the chip select lines as GPIOs and
> let Linux manage them. There is much less overhead in setting a GPIO
> compared to messaging a QUP, so this improves performance. As long as
> this continues to work, perhaps we don't care about whether we can
> really tell GPI mode to leave the CS asserted.

I think it would be good experiment to get it working with chromebooks.
I am sure it wont be trivial and may not work out of box, but would be a
good experiment to do :)

-- 
~Vinod