[PATCH 0/7] Add and enable GPI DMA users

[PATCH 0/7] Add and enable GPI DMA users

[Vinod Koul]

Hello,

This series add the GPI DMA in qcom geni spi and i2c drivers. For this we
first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
headers and then add support for gpi dma in geni driver.

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

Lastly, add the GPI dma nodes and enable dma for spi found in Rb3 board.

To merge this, we could merge all thru qcom tree with ack on spi/i2c.

Vinod Koul (7):
  soc: qcom: geni: move GENI_IF_DISABLE_RO to common header
  soc: qcom: geni: move struct geni_wrapper to header
  soc: qcom: geni: Add support for gpi dma
  spi: spi-geni-qcom: Add support for GPI dma
  i2c: qcom-geni: Add support for GPI DMA
  arm64: dts: qcom: sdm845: Add gpi dma node
  arm64: dts: qcom: sdm845: enable dma for spi

 arch/arm64/boot/dts/qcom/sdm845-db845c.dts |   4 +
 arch/arm64/boot/dts/qcom/sdm845.dtsi       |  57 +++
 drivers/i2c/busses/i2c-qcom-geni.c         | 246 ++++++++++++-
 drivers/soc/qcom/qcom-geni-se.c            |  55 ++-
 drivers/spi/spi-geni-qcom.c                | 395 ++++++++++++++++++++-
 include/linux/qcom-geni-se.h               |  20 ++
 6 files changed, 747 insertions(+), 30 deletions(-)

Thanks
-- 
2.26.2

[PATCH 1/7] 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

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

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index f42954e2c98e..285ed86c2bab 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -108,7 +108,6 @@ static struct geni_wrapper *earlycon_wrapper;
 #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 ec2ad4b0fe14..e3f4b16040d9 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -65,6 +65,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
-- 
2.26.2

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

[Bjorn Andersson]

On Mon 11 Jan 09:16 CST 2021, Vinod Koul 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>

Regards,
Bjorn

> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 1 -
>  include/linux/qcom-geni-se.h    | 1 +
>  2 files changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index f42954e2c98e..285ed86c2bab 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -108,7 +108,6 @@ static struct geni_wrapper *earlycon_wrapper;
>  #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 ec2ad4b0fe14..e3f4b16040d9 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -65,6 +65,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
> -- 
> 2.26.2
> 

[PATCH 2/7] soc: qcom: geni: move struct geni_wrapper to header

[Vinod Koul]

I2C geni driver needs to access struct geni_wrapper, so move it to
header.

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

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 285ed86c2bab..a3868228ea05 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -79,21 +79,6 @@
  */
 
 #define MAX_CLK_PERF_LEVEL 32
-#define NUM_AHB_CLKS 2
-
-/**
- * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
- * @dev:		Device pointer of the QUP wrapper core
- * @base:		Base address of this instance of QUP wrapper core
- * @ahb_clks:		Handle to the primary & secondary AHB clocks
- * @to_core:		Core ICC path
- */
-struct geni_wrapper {
-	struct device *dev;
-	void __iomem *base;
-	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
-	struct geni_icc_path to_core;
-};
 
 static const char * const icc_path_names[] = {"qup-core", "qup-config",
 						"qup-memory"};
diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
index e3f4b16040d9..cb4e40908f9f 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -38,6 +38,21 @@ struct geni_icc_path {
 	unsigned int avg_bw;
 };
 
+#define NUM_AHB_CLKS 2
+
+/**
+ * @struct geni_wrapper - Data structure to represent the QUP Wrapper Core
+ * @dev:		Device pointer of the QUP wrapper core
+ * @base:		Base address of this instance of QUP wrapper core
+ * @ahb_clks:		Handle to the primary & secondary AHB clocks
+ */
+struct geni_wrapper {
+	struct device *dev;
+	void __iomem *base;
+	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
+	struct geni_icc_path to_core;
+};
+
 /**
  * struct geni_se - GENI Serial Engine
  * @base:		Base Address of the Serial Engine's register block
-- 
2.26.2

Re: [PATCH 2/7] soc: qcom: geni: move struct geni_wrapper to header

[Bjorn Andersson]

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

> I2C geni driver needs to access struct geni_wrapper, so move it to
> header.
> 

Please tell me more!

Glanced through the other patches and the only user I can find it in
patch 5 where you use this to get the struct device * of the wrapper.

At least in the DT case this would be [SE]->dev->parent, perhaps we
can't rely on this due to ACPI?

Regards,
Bjorn

> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 15 ---------------
>  include/linux/qcom-geni-se.h    | 15 +++++++++++++++
>  2 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 285ed86c2bab..a3868228ea05 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -79,21 +79,6 @@
>   */
>  
>  #define MAX_CLK_PERF_LEVEL 32
> -#define NUM_AHB_CLKS 2
> -
> -/**
> - * struct geni_wrapper - Data structure to represent the QUP Wrapper Core
> - * @dev:		Device pointer of the QUP wrapper core
> - * @base:		Base address of this instance of QUP wrapper core
> - * @ahb_clks:		Handle to the primary & secondary AHB clocks
> - * @to_core:		Core ICC path
> - */
> -struct geni_wrapper {
> -	struct device *dev;
> -	void __iomem *base;
> -	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> -	struct geni_icc_path to_core;
> -};
>  
>  static const char * const icc_path_names[] = {"qup-core", "qup-config",
>  						"qup-memory"};
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index e3f4b16040d9..cb4e40908f9f 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -38,6 +38,21 @@ struct geni_icc_path {
>  	unsigned int avg_bw;
>  };
>  
> +#define NUM_AHB_CLKS 2
> +
> +/**
> + * @struct geni_wrapper - Data structure to represent the QUP Wrapper Core
> + * @dev:		Device pointer of the QUP wrapper core
> + * @base:		Base address of this instance of QUP wrapper core
> + * @ahb_clks:		Handle to the primary & secondary AHB clocks
> + */
> +struct geni_wrapper {
> +	struct device *dev;
> +	void __iomem *base;
> +	struct clk_bulk_data ahb_clks[NUM_AHB_CLKS];
> +	struct geni_icc_path to_core;
> +};
> +
>  /**
>   * struct geni_se - GENI Serial Engine
>   * @base:		Base Address of the Serial Engine's register block
> -- 
> 2.26.2
> 

Re: [PATCH 2/7] soc: qcom: geni: move struct geni_wrapper to header

[Vinod Koul]

On 11-01-21, 09:34, Bjorn Andersson wrote:
> On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:
> 
> > I2C geni driver needs to access struct geni_wrapper, so move it to
> > header.
> > 
> 
> Please tell me more!
> 
> Glanced through the other patches and the only user I can find it in
> patch 5 where you use this to get the struct device * of the wrapper.

That is correct. The dma mapping needs to be done with SE device.

> At least in the DT case this would be [SE]->dev->parent, perhaps we
> can't rely on this due to ACPI?

I would have missed that then, but I somehow recall trying that.. Though
I have not looked into ACPI..

Given that we would need to worry about ACPI, do you recommend using
parent or keeping this

-- 
~Vinod

Re: [PATCH 2/7] soc: qcom: geni: move struct geni_wrapper to header

[Bjorn Andersson]

On Mon 11 Jan 11:43 CST 2021, Vinod Koul wrote:

> On 11-01-21, 09:34, Bjorn Andersson wrote:
> > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:
> > 
> > > I2C geni driver needs to access struct geni_wrapper, so move it to
> > > header.
> > > 
> > 
> > Please tell me more!
> > 
> > Glanced through the other patches and the only user I can find it in
> > patch 5 where you use this to get the struct device * of the wrapper.
> 
> That is correct. The dma mapping needs to be done with SE device.
> 
> > At least in the DT case this would be [SE]->dev->parent, perhaps we
> > can't rely on this due to ACPI?
> 
> I would have missed that then, but I somehow recall trying that.. Though
> I have not looked into ACPI..
> 
> Given that we would need to worry about ACPI, do you recommend using
> parent or keeping this
> 

We get the wrapper by the means of dev_drv_getdata(dev->parent),
so afaict we need to figure out how to get hold of the wrapper for ACPI
to work either way.

We then need to lug around the wrapper's device for your uses and
exposing the wrapper struct solves this for us. So I'm okay with your
proposal.

Regards,
Bjorn

[PATCH 3/7] 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

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

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index a3868228ea05..db44dc32e049 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -310,6 +310,39 @@ static void geni_se_select_dma_mode(struct geni_se *se)
 		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
 }
 
+static int geni_se_select_gpi_mode(struct geni_se *se)
+{
+	unsigned int geni_dma_mode = 0;
+	unsigned int gpi_event_en = 0;
+	unsigned int common_geni_m_irq_en = 0;
+	unsigned int common_geni_s_irq_en = 0;
+
+	common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
+	common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
+	common_geni_m_irq_en &=
+			~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
+			M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
+	common_geni_s_irq_en &= ~S_CMD_DONE_EN;
+	geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
+	gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN);
+
+	geni_dma_mode |= GENI_DMA_MODE_EN;
+	gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
+				GENI_M_EVENT_EN | GENI_S_EVENT_EN);
+
+	writel_relaxed(0, se->base + SE_IRQ_EN);
+	writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN);
+	writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN);
+	writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
+	writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR);
+	writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR);
+	writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR);
+	writel_relaxed(geni_dma_mode, se->base + SE_GENI_DMA_MODE_EN);
+	writel_relaxed(gpi_event_en, se->base + SE_GSI_EVENT_EN);
+
+	return 0;
+}
+
 /**
  * geni_se_select_mode() - Select the serial engine transfer mode
  * @se:		Pointer to the concerned serial engine.
@@ -317,7 +350,8 @@ 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:
@@ -326,6 +360,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 cb4e40908f9f..12003a6cb133 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -12,6 +12,7 @@
 enum geni_se_xfer_mode {
 	GENI_SE_INVALID,
 	GENI_SE_FIFO,
+	GENI_GPI_DMA,
 	GENI_SE_DMA,
 };
 
@@ -123,6 +124,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.26.2

Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma

[Bjorn Andersson]

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

> GPI DMA is one of the DMA modes supported on geni, this adds support to
> enable that mode
> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 39 ++++++++++++++++++++++++++++++++-
>  include/linux/qcom-geni-se.h    |  4 ++++
>  2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index a3868228ea05..db44dc32e049 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -310,6 +310,39 @@ static void geni_se_select_dma_mode(struct geni_se *se)
>  		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
>  }
>  
> +static int geni_se_select_gpi_mode(struct geni_se *se)

This doesn't return any information and the return value isn't looked
at, please make it void.

> +{
> +	unsigned int geni_dma_mode = 0;
> +	unsigned int gpi_event_en = 0;
> +	unsigned int common_geni_m_irq_en = 0;
> +	unsigned int common_geni_s_irq_en = 0;

These could certainly be given a shorter name.

None of them needs to be initialized, first access in all cases are
assignments.

> +
> +	common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
> +	common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> +	common_geni_m_irq_en &=
> +			~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> +			M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> +	common_geni_s_irq_en &= ~S_CMD_DONE_EN;
> +	geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> +	gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN);
> +
> +	geni_dma_mode |= GENI_DMA_MODE_EN;
> +	gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> +				GENI_M_EVENT_EN | GENI_S_EVENT_EN);

Please reorder these so that you do
	readl(m)
	mask out bits of m

	readl(s)
	mask out bits of s

	...

> +
> +	writel_relaxed(0, se->base + SE_IRQ_EN);
> +	writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN);
> +	writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> +	writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);

Lowercase hex digits please.

> +	writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR);
> +	writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR);
> +	writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR);
> +	writel_relaxed(geni_dma_mode, se->base + SE_GENI_DMA_MODE_EN);
> +	writel_relaxed(gpi_event_en, se->base + SE_GSI_EVENT_EN);

Why is this driver using _relaxed accessors exclusively? Why are you
using _relaxed versions?

And wouldn't it be suitable to have a wmb() before the "dma mode enable"
and "event enable" at least? (I.e. use writel() instead)

Regards,
Bjorn

> +
> +	return 0;
> +}
> +
>  /**
>   * geni_se_select_mode() - Select the serial engine transfer mode
>   * @se:		Pointer to the concerned serial engine.
> @@ -317,7 +350,8 @@ 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:
> @@ -326,6 +360,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 cb4e40908f9f..12003a6cb133 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -12,6 +12,7 @@
>  enum geni_se_xfer_mode {
>  	GENI_SE_INVALID,
>  	GENI_SE_FIFO,
> +	GENI_GPI_DMA,
>  	GENI_SE_DMA,
>  };
>  
> @@ -123,6 +124,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.26.2
> 

Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma

[Vinod Koul]

On 11-01-21, 09:40, Bjorn Andersson wrote:
> On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:
> 
> > GPI DMA is one of the DMA modes supported on geni, this adds support to
> > enable that mode
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/soc/qcom/qcom-geni-se.c | 39 ++++++++++++++++++++++++++++++++-
> >  include/linux/qcom-geni-se.h    |  4 ++++
> >  2 files changed, 42 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> > index a3868228ea05..db44dc32e049 100644
> > --- a/drivers/soc/qcom/qcom-geni-se.c
> > +++ b/drivers/soc/qcom/qcom-geni-se.c
> > @@ -310,6 +310,39 @@ static void geni_se_select_dma_mode(struct geni_se *se)
> >  		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
> >  }
> >  
> > +static int geni_se_select_gpi_mode(struct geni_se *se)
> 
> This doesn't return any information and the return value isn't looked
> at, please make it void.

Sure..

> > +{
> > +	unsigned int geni_dma_mode = 0;
> > +	unsigned int gpi_event_en = 0;
> > +	unsigned int common_geni_m_irq_en = 0;
> > +	unsigned int common_geni_s_irq_en = 0;
> 
> These could certainly be given a shorter name.

Certainly..

> None of them needs to be initialized, first access in all cases are
> assignments.

Will update

> > +
> > +	common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
> > +	common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> > +	common_geni_m_irq_en &=
> > +			~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> > +			M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> > +	common_geni_s_irq_en &= ~S_CMD_DONE_EN;
> > +	geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> > +	gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN);
> > +
> > +	geni_dma_mode |= GENI_DMA_MODE_EN;
> > +	gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> > +				GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> 
> Please reorder these so that you do
> 	readl(m)
> 	mask out bits of m
> 
> 	readl(s)
> 	mask out bits of s
> 
> 	...

okay will update

> > +
> > +	writel_relaxed(0, se->base + SE_IRQ_EN);
> > +	writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN);
> > +	writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> > +	writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
> 
> Lowercase hex digits please.

Yeah missed

> > +	writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR);
> > +	writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR);
> > +	writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR);
> > +	writel_relaxed(geni_dma_mode, se->base + SE_GENI_DMA_MODE_EN);
> > +	writel_relaxed(gpi_event_en, se->base + SE_GSI_EVENT_EN);
> 
> Why is this driver using _relaxed accessors exclusively? Why are you
> using _relaxed versions?
> 
> And wouldn't it be suitable to have a wmb() before the "dma mode enable"
> and "event enable" at least? (I.e. use writel() instead)

Yeah we invoke this to select the mode before programming DMA, so yes a
wmb() would make sense. Thanks for quick look

-- 
~Vinod

Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma

[Doug Anderson]

Hi,

On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> +static int geni_se_select_gpi_mode(struct geni_se *se)
> +{
> +       unsigned int geni_dma_mode = 0;
> +       unsigned int gpi_event_en = 0;
> +       unsigned int common_geni_m_irq_en = 0;
> +       unsigned int common_geni_s_irq_en = 0;
> +
> +       common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
> +       common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> +       common_geni_m_irq_en &=
> +                       ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> +                       M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> +       common_geni_s_irq_en &= ~S_CMD_DONE_EN;
> +       geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);

Do you really need to do a read/modify/write of SE_GENI_DMA_MODE_EN?
It's a register with a single bit in it.  Just set the bit, no?  There
are cases where read-modify-write is the correct thing to do but IMO
it shouldn't be the default way of working.  If code is initting a
register to a default state it should just be setting the register.
If some later incarnation of the hardware comes along and adds extra
bits to this register then, sure, we might have to modify the code.
However, it has the advantage where we aren't left at the whims of
whatever was programmed by whatever version of whatever firmware was
running on the device.  I've been bitten way more often by firmware
leaving registers in some random / unexpected state than by new bits
introduced by new versions of hardware.

...same for other registers in your patch that you can just init.

(this is true throughout lots of Qualcomm code, but I figure might as
well start trying to do something about it?)


> +       gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN);
> +
> +       geni_dma_mode |= GENI_DMA_MODE_EN;
> +       gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> +                               GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> +
> +       writel_relaxed(0, se->base + SE_IRQ_EN);
> +       writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN);
> +       writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN);

Last time I touched this bit of code I think folks agreed that it
would be better to move managing of the interrupt enables out of the
common code and move them to the various drivers using geni [1].  I
was hoping that someone from Qualcomm would be able to pick this up.
Managing them in the wrapper just ends up causing a whole bunch of
special cases.  Any chance you could take that on as part of your
series?

Presumably if this was mananged in individual drivers you also might
be able to do less read-modify-write type stuff, too...

[1] https://lore.kernel.org/linux-arm-msm/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/


> +       writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
> +       writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR);
> +       writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR);
> +       writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR);

This looks mostly like geni_se_irq_clear().  Should they somehow share code?


> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index cb4e40908f9f..12003a6cb133 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -12,6 +12,7 @@
>  enum geni_se_xfer_mode {
>         GENI_SE_INVALID,
>         GENI_SE_FIFO,
> +       GENI_GPI_DMA,

Add a comment like "Also known as GSI" here to help people figure out
what's going on when they're trying to parse the manual or #defines
like "SE_GSI_EVENT_EN"


> @@ -123,6 +124,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))

Maybe this define belonged in patch #1?  It doesn't seem related to this patch?

Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma

[Vinod Koul]

On 12-01-21, 16:01, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > +static int geni_se_select_gpi_mode(struct geni_se *se)
> > +{
> > +       unsigned int geni_dma_mode = 0;
> > +       unsigned int gpi_event_en = 0;
> > +       unsigned int common_geni_m_irq_en = 0;
> > +       unsigned int common_geni_s_irq_en = 0;
> > +
> > +       common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN);
> > +       common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> > +       common_geni_m_irq_en &=
> > +                       ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> > +                       M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> > +       common_geni_s_irq_en &= ~S_CMD_DONE_EN;
> > +       geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN);
> 
> Do you really need to do a read/modify/write of SE_GENI_DMA_MODE_EN?
> It's a register with a single bit in it.  Just set the bit, no?  There
> are cases where read-modify-write is the correct thing to do but IMO
> it shouldn't be the default way of working.  If code is initting a
> register to a default state it should just be setting the register.
> If some later incarnation of the hardware comes along and adds extra
> bits to this register then, sure, we might have to modify the code.
> However, it has the advantage where we aren't left at the whims of
> whatever was programmed by whatever version of whatever firmware was
> running on the device.  I've been bitten way more often by firmware
> leaving registers in some random / unexpected state than by new bits
> introduced by new versions of hardware.
> 
> ...same for other registers in your patch that you can just init.

Yes sounds right to me. I will review this and other register writes and
update the code

> (this is true throughout lots of Qualcomm code, but I figure might as
> well start trying to do something about it?)

yep, we can always start somewhere :)

> > +       gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN);
> > +
> > +       geni_dma_mode |= GENI_DMA_MODE_EN;
> > +       gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> > +                               GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> > +
> > +       writel_relaxed(0, se->base + SE_IRQ_EN);
> > +       writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN);
> > +       writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> 
> Last time I touched this bit of code I think folks agreed that it
> would be better to move managing of the interrupt enables out of the
> common code and move them to the various drivers using geni [1].  I
> was hoping that someone from Qualcomm would be able to pick this up.
> Managing them in the wrapper just ends up causing a whole bunch of
> special cases.  Any chance you could take that on as part of your
> series?
> 
> Presumably if this was mananged in individual drivers you also might
> be able to do less read-modify-write type stuff, too...
> 
> [1] https://lore.kernel.org/linux-arm-msm/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/

If it is okay, I can take this up but after this series. We can handle
the move independently as that is certainly a good thing to do.

> > +       writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
> > +       writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR);
> > +       writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR);
> > +       writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR);
> 
> This looks mostly like geni_se_irq_clear().  Should they somehow share code?

Mostly seems similar, let me move over that and verify

> > diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> > index cb4e40908f9f..12003a6cb133 100644
> > --- a/include/linux/qcom-geni-se.h
> > +++ b/include/linux/qcom-geni-se.h
> > @@ -12,6 +12,7 @@
> >  enum geni_se_xfer_mode {
> >         GENI_SE_INVALID,
> >         GENI_SE_FIFO,
> > +       GENI_GPI_DMA,
> 
> Add a comment like "Also known as GSI" here to help people figure out
> what's going on when they're trying to parse the manual or #defines
> like "SE_GSI_EVENT_EN"

Sure will add GSI.

> > @@ -123,6 +124,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))
> 
> Maybe this define belonged in patch #1?  It doesn't seem related to this patch?

Yes the bit defines should go with the register.

-- 
~Vinod

[PATCH 4/7] 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 | 395 +++++++++++++++++++++++++++++++++++-
 1 file changed, 384 insertions(+), 11 deletions(-)

diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
index 512e925d5ea4..5bb0e2192734 100644
--- a/drivers/spi/spi-geni-qcom.c
+++ b/drivers/spi/spi-geni-qcom.c
@@ -2,6 +2,8 @@
 // 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/interrupt.h>
 #include <linux/io.h>
 #include <linux/log2.h>
@@ -10,6 +12,7 @@
 #include <linux/pm_opp.h>
 #include <linux/pm_runtime.h>
 #include <linux/qcom-geni-se.h>
+#include <linux/dma/qcom-gpi-dma.h>
 #include <linux/spi/spi.h>
 #include <linux/spinlock.h>
 
@@ -63,6 +66,35 @@
 #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 desc_cb;
+};
+
+enum spi_m_cmd_opcode {
+	CMD_NONE,
+	CMD_XFER,
+	CMD_CS,
+	CMD_CANCEL,
+};
+
 struct spi_geni_master {
 	struct geni_se se;
 	struct device *dev;
@@ -79,10 +111,21 @@ struct spi_geni_master {
 	struct completion cs_done;
 	struct completion cancel_done;
 	struct completion abort_done;
+	struct completion xfer_done;
 	unsigned int oversampling;
 	spinlock_t lock;
 	int irq;
 	bool cs_flag;
+	struct spi_geni_gsi *gsi;
+	struct dma_chan *tx;
+	struct dma_chan *rx;
+	struct completion tx_cb;
+	struct completion rx_cb;
+	bool qn_err;
+	int cur_xfer_mode;
+	int num_tx_eot;
+	int num_rx_eot;
+	int num_xfers;
 };
 
 static int get_spi_clk_cfg(unsigned int speed_hz,
@@ -274,31 +317,269 @@ static int setup_fifo_params(struct spi_device *spi_slv,
 	return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz);
 }
 
+static int get_xfer_mode(struct spi_master *spi)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	struct geni_se *se = &mas->se;
+	int mode = GENI_SE_FIFO;
+	int fifo_disable;
+	bool dma_chan_valid;
+
+	fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
+	dma_chan_valid = !(IS_ERR_OR_NULL(mas->tx) || IS_ERR_OR_NULL(mas->rx));
+
+	/*
+	 * If FIFO Interface is disabled and there are no DMA channels then we
+	 * can't do this transfer.
+	 * If FIFO interface is disabled, we can do GSI only,
+	 * else pick FIFO mode.
+	 */
+	if (fifo_disable && !dma_chan_valid) {
+		dev_err(mas->dev, "Fifo and dma mode disabled!! can't xfer\n");
+		mode = -EINVAL;
+	} else if (fifo_disable) {
+		mode = GENI_GPI_DMA;
+	} else {
+		mode = GENI_SE_FIFO;
+	}
+
+	return 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) {
+		dev_err(gsi->mas->dev, "%s: DMA %s txn failed\n", __func__, tx ? "tx" : "rx");
+		return;
+	}
+
+	if (!result->residue) {
+		dev_dbg(gsi->mas->dev, "%s\n", __func__);
+		if (tx)
+			complete(&gsi->mas->tx_cb);
+		else
+			complete(&gsi->mas->rx_cb);
+	} else {
+		dev_err(gsi->mas->dev, "DMA xfer has pending: %d\n", 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)
+{
+	int ret = 0;
+	unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
+	struct spi_geni_gsi *gsi;
+	struct dma_slave_config config;
+	struct gpi_spi_config peripheral;
+
+	memset(&config, 0, sizeof(config));
+	memset(&peripheral, 0, sizeof(peripheral));
+	config.peripheral_config = &peripheral;
+	config.peripheral_size = sizeof(peripheral);
+
+	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;
+		peripheral.set_config = true;
+	}
+
+	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;
+	}
+
+	peripheral.cs = spi_slv->chip_select;
+
+	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.pack_en = true;
+	peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
+	ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
+			      &peripheral.clk_src, &peripheral.clk_div);
+	if (ret) {
+		dev_err(mas->dev, "%s:Err setting clks:%d\n", __func__, ret);
+		return ret;
+	}
+
+	if (!xfer->cs_change) {
+		if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
+			peripheral.fragmentation = FRAGMENTATION;
+	}
+
+	gsi = &mas->gsi[mas->num_xfers];
+	gsi->desc_cb.mas = mas;
+	gsi->desc_cb.xfer = xfer;
+	if (peripheral.cmd & SPI_RX) {
+		dmaengine_slave_config(mas->rx, &config);
+		gsi->rx_desc = dmaengine_prep_slave_single(mas->rx, xfer->rx_dma,
+							   xfer->len, DMA_DEV_TO_MEM, flags);
+		if (IS_ERR_OR_NULL(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->desc_cb;
+		mas->num_rx_eot++;
+	}
+
+	if (peripheral.cmd & SPI_TX_ONLY)
+		mas->num_tx_eot++;
+
+	dmaengine_slave_config(mas->tx, &config);
+	gsi->tx_desc = dmaengine_prep_slave_single(mas->tx, xfer->tx_dma,
+						   xfer->len, DMA_MEM_TO_DEV, flags);
+
+	if (IS_ERR_OR_NULL(gsi->tx_desc)) {
+		dev_err(mas->dev, "Err setting up tx desc\n");
+		return -EIO;
+	}
+	gsi->tx_desc->callback_result = spi_gsi_tx_callback_result;
+	gsi->tx_desc->callback_param = &gsi->desc_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++;
+	return ret;
+}
+
+static int spi_geni_map_buf(struct spi_geni_master *mas, struct spi_message *msg)
+{
+	struct spi_transfer *xfer;
+	struct device *gsi_dev = mas->dev->parent;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (xfer->rx_buf) {
+			xfer->rx_dma = dma_map_single(gsi_dev, xfer->rx_buf,
+						      xfer->len, DMA_FROM_DEVICE);
+			if (dma_mapping_error(mas->dev, xfer->rx_dma)) {
+				dev_err(mas->dev, "Err mapping buf\n");
+				return -ENOMEM;
+			}
+		}
+
+		if (xfer->tx_buf) {
+			xfer->tx_dma = dma_map_single(gsi_dev, (void *)xfer->tx_buf,
+						      xfer->len, DMA_TO_DEVICE);
+			if (dma_mapping_error(gsi_dev, xfer->tx_dma)) {
+				dev_err(mas->dev, "Err mapping buf\n");
+				dma_unmap_single(gsi_dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
+				return -ENOMEM;
+			}
+		}
+	};
+
+	return 0;
+}
+
+static void spi_geni_unmap_buf(struct spi_geni_master *mas, struct spi_message *msg)
+{
+	struct spi_transfer *xfer;
+	struct device *gsi_dev = mas->dev;
+
+	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
+		if (xfer->rx_buf)
+			dma_unmap_single(gsi_dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
+		if (xfer->tx_buf)
+			dma_unmap_single(gsi_dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
+	};
+}
+
 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);
+	struct geni_se *se = &mas->se;
+
+	mas->cur_xfer_mode = get_xfer_mode(spi);
+
+	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
+		geni_se_select_mode(se, GENI_SE_FIFO);
+		reinit_completion(&mas->xfer_done);
+		ret = setup_fifo_params(spi_msg->spi, spi);
+		if (ret)
+			dev_err(mas->dev, "Couldn't select mode %d\n", ret);
+
+	} else if (mas->cur_xfer_mode == GENI_GPI_DMA) {
+		mas->num_tx_eot = 0;
+		mas->num_rx_eot = 0;
+		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));
+		geni_se_select_mode(se, GENI_GPI_DMA);
+		ret = spi_geni_map_buf(mas, spi_msg);
+
+	} else {
+		dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode);
+		ret = -EINVAL;
+	}
 
-	ret = setup_fifo_params(spi_msg->spi, spi);
-	if (ret)
-		dev_err(mas->dev, "Couldn't select mode %d\n", ret);
 	return ret;
 }
 
+static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg)
+{
+	struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
+
+	mas->cur_speed_hz = 0;
+	mas->cur_bits_per_word = 0;
+	if (mas->cur_xfer_mode == GENI_GPI_DMA)
+		spi_geni_unmap_buf(mas, spi_msg);
+	return 0;
+}
+
 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;
+	size_t gsi_sz;
+	int ret = 0;
 
 	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;
+		ret = -ENXIO;
+		goto out_pm;
 	}
 	mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
 
@@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
 	spi_tx_cfg &= ~CS_TOGGLE;
 	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
 
+	mas->tx = dma_request_slave_channel(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 out_pm;
+	} else {
+		mas->rx = dma_request_slave_channel(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));
+			dma_release_channel(mas->tx);
+			ret = PTR_ERR(mas->rx);
+			goto out_pm;
+		}
+
+		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)) {
+			dma_release_channel(mas->tx);
+			dma_release_channel(mas->rx);
+			mas->tx = NULL;
+			mas->rx = NULL;
+			goto out_pm;
+		}
+	}
+
+out_pm:
 	pm_runtime_put(mas->dev);
-	return 0;
+	return ret;
 }
 
 static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
@@ -420,6 +727,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 {
 	u32 m_cmd = 0;
 	u32 len;
+	u32 m_param = 0;
 	struct geni_se *se = &mas->se;
 	int ret;
 
@@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 		len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
 	len &= TRANS_LEN_MSK;
 
+	if (!xfer->cs_change) {
+		if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
+			m_param |= FRAGMENTATION;
+	}
+
 	mas->cur_xfer = xfer;
 	if (xfer->tx_buf) {
 		m_cmd |= SPI_TX_ONLY;
@@ -475,7 +788,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
 	 * interrupt could come in at any time now.
 	 */
 	spin_lock_irq(&mas->lock);
-	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
+	geni_se_setup_m_cmd(se, m_cmd, m_param);
 
 	/*
 	 * TX_WATERMARK_REG should be set after SPI configuration and
@@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
 				struct spi_transfer *xfer)
 {
 	struct spi_geni_master *mas = spi_master_get_devdata(spi);
+	unsigned long timeout, jiffies;
+	int ret = 0i, i;
 
 	/* Terminate and return success for 0 byte length transfer */
 	if (!xfer->len)
-		return 0;
+		return ret;
+
+	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
+		setup_fifo_xfer(xfer, mas, slv->mode, spi);
+	} else {
+		setup_gsi_xfer(xfer, mas, slv, spi);
+		if (mas->num_xfers >= NUM_SPI_XFER ||
+		    (list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))) {
+			for (i = 0 ; i < mas->num_tx_eot; i++) {
+				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;
+				}
+				spi_finalize_current_transfer(spi);
+			}
+			for (i = 0 ; i < mas->num_rx_eot; i++) {
+				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
+				timeout = wait_for_completion_timeout(&mas->tx_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);
+			}
+			if (mas->qn_err) {
+				ret = -EIO;
+				mas->qn_err = false;
+				goto err_gsi_geni_transfer_one;
+			}
+		}
+	}
 
-	setup_fifo_xfer(xfer, mas, slv->mode, spi);
-	return 1;
+	return ret;
+
+err_gsi_geni_transfer_one:
+	dmaengine_terminate_all(mas->tx);
+	return ret;
 }
 
 static irqreturn_t geni_spi_isr(int irq, void *data)
@@ -595,6 +947,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);
@@ -632,15 +993,18 @@ static int spi_geni_probe(struct platform_device *pdev)
 	spi->num_chipselect = 4;
 	spi->max_speed_hz = 50000000;
 	spi->prepare_message = spi_geni_prepare_message;
+	spi->unprepare_message = spi_geni_unprepare_message;
 	spi->transfer_one = spi_geni_transfer_one;
 	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->xfer_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);
@@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
 	if (ret)
 		goto spi_geni_probe_runtime_disable;
 
+	/*
+	 * query 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
+	 */
+	mas->cur_xfer_mode = get_xfer_mode(spi);
+	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;
-- 
2.26.2

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

[Mark Brown]

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

On Mon, Jan 11, 2021 at 08:46:48PM +0530, Vinod Koul wrote:

> +static int get_xfer_mode(struct spi_master *spi)
> +{
> +	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +	struct geni_se *se = &mas->se;
> +	int mode = GENI_SE_FIFO;

Why not use the core DMA mapping support?

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

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

[Vinod Koul]

Hi Mark,

On 11-01-21, 16:35, Mark Brown wrote:
> On Mon, Jan 11, 2021 at 08:46:48PM +0530, Vinod Koul wrote:
> 
> > +static int get_xfer_mode(struct spi_master *spi)
> > +{
> > +	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> > +	struct geni_se *se = &mas->se;
> > +	int mode = GENI_SE_FIFO;
> 
> Why not use the core DMA mapping support?

Sorry I seemed to have missed replying to this one.

Looking at the code, that is ideal case. Only issue I can see is that
core DMA mapping device being used is incorrect. The core would use
ctlr->dev.parent which is the spi0 device here.

But in this case, that wont work. We have a parent qup device which is
the parent for both spi and dma device and needs to be used for
dma-mapping! 

If we allow drivers to set dma mapping device and use that, then I can
reuse the core. Let me know if that is agreeable to you and I can hack
this up. Maybe add a new member in spi_controller which is filled by
drivers in can_dma() callback?

Thanks
-- 
~Vinod

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

[Mark Brown]

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

On Wed, Jun 16, 2021 at 02:20:45PM +0530, Vinod Koul wrote:

> Looking at the code, that is ideal case. Only issue I can see is that
> core DMA mapping device being used is incorrect. The core would use
> ctlr->dev.parent which is the spi0 device here.

Why would the parent of the controller be a SPI device?

> But in this case, that wont work. We have a parent qup device which is
> the parent for both spi and dma device and needs to be used for
> dma-mapping! 

> If we allow drivers to set dma mapping device and use that, then I can
> reuse the core. Let me know if that is agreeable to you and I can hack
> this up. Maybe add a new member in spi_controller which is filled by
> drivers in can_dma() callback?

Possibly, I'd need to see the code.

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

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

[Vinod Koul]

On 16-06-21, 12:35, Mark Brown wrote:
> On Wed, Jun 16, 2021 at 02:20:45PM +0530, Vinod Koul wrote:
> 
> > Looking at the code, that is ideal case. Only issue I can see is that
> > core DMA mapping device being used is incorrect. The core would use
> > ctlr->dev.parent which is the spi0 device here.
> 
> Why would the parent of the controller be a SPI device?

Sorry my bad, I meant the core use ctlr->dev.parent which in this case is
the SPI master device, 880000.spi

> > But in this case, that wont work. We have a parent qup device which is
> > the parent for both spi and dma device and needs to be used for
> > dma-mapping! 
> 
> > If we allow drivers to set dma mapping device and use that, then I can
> > reuse the core. Let me know if that is agreeable to you and I can hack
> > this up. Maybe add a new member in spi_controller which is filled by
> > drivers in can_dma() callback?
> 
> Possibly, I'd need to see the code.

Ok, let me do a prototype and share ...

Thanks
-- 
~Vinod

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

[Vinod Koul]

On 16-06-21, 17:32, Vinod Koul wrote:
> On 16-06-21, 12:35, Mark Brown wrote:
> > On Wed, Jun 16, 2021 at 02:20:45PM +0530, Vinod Koul wrote:
> > > But in this case, that wont work. We have a parent qup device which is
> > > the parent for both spi and dma device and needs to be used for
> > > dma-mapping! 
> > 
> > > If we allow drivers to set dma mapping device and use that, then I can
> > > reuse the core. Let me know if that is agreeable to you and I can hack
> > > this up. Maybe add a new member in spi_controller which is filled by
> > > drivers in can_dma() callback?
> > 
> > Possibly, I'd need to see the code.
> 
> Ok, let me do a prototype and share ...

So setting the dma_map_dev in the can_dma() callback does not work as we
would need device before we invoke can_dma(), so modified this to be set
earlier by driver (in driver probe, set the dma_map_dev) and use in
__spi_map_msg().

With this it works for me & I was able to get rid of driver mapping code

-- >8 --

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

-- 
~Vinod

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

[Bjorn Andersson]

On Mon 11 Jan 09:16 CST 2021, Vinod Koul 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 | 395 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 384 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 512e925d5ea4..5bb0e2192734 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -2,6 +2,8 @@
>  // 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/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/log2.h>
> @@ -10,6 +12,7 @@
>  #include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/qcom-geni-se.h>
> +#include <linux/dma/qcom-gpi-dma.h>
>  #include <linux/spi/spi.h>
>  #include <linux/spinlock.h>
>  
> @@ -63,6 +66,35 @@
>  #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 desc_cb;
> +};
> +
> +enum spi_m_cmd_opcode {
> +	CMD_NONE,
> +	CMD_XFER,
> +	CMD_CS,
> +	CMD_CANCEL,
> +};
> +
>  struct spi_geni_master {
>  	struct geni_se se;
>  	struct device *dev;
> @@ -79,10 +111,21 @@ struct spi_geni_master {
>  	struct completion cs_done;
>  	struct completion cancel_done;
>  	struct completion abort_done;
> +	struct completion xfer_done;
>  	unsigned int oversampling;
>  	spinlock_t lock;
>  	int irq;
>  	bool cs_flag;
> +	struct spi_geni_gsi *gsi;
> +	struct dma_chan *tx;
> +	struct dma_chan *rx;
> +	struct completion tx_cb;
> +	struct completion rx_cb;
> +	bool qn_err;
> +	int cur_xfer_mode;
> +	int num_tx_eot;
> +	int num_rx_eot;
> +	int num_xfers;
>  };
>  
>  static int get_spi_clk_cfg(unsigned int speed_hz,
> @@ -274,31 +317,269 @@ static int setup_fifo_params(struct spi_device *spi_slv,
>  	return geni_spi_set_clock_and_bw(mas, spi_slv->max_speed_hz);
>  }
>  
> +static int get_xfer_mode(struct spi_master *spi)
> +{
> +	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +	struct geni_se *se = &mas->se;
> +	int mode = GENI_SE_FIFO;

No need to initialize mode, it's overwritten in all code paths.

> +	int fifo_disable;
> +	bool dma_chan_valid;
> +
> +	fifo_disable = readl(se->base + GENI_IF_DISABLE_RO) & FIFO_IF_DISABLE;
> +	dma_chan_valid = !(IS_ERR_OR_NULL(mas->tx) || IS_ERR_OR_NULL(mas->rx));
> +
> +	/*
> +	 * If FIFO Interface is disabled and there are no DMA channels then we
> +	 * can't do this transfer.
> +	 * If FIFO interface is disabled, we can do GSI only,
> +	 * else pick FIFO mode.
> +	 */
> +	if (fifo_disable && !dma_chan_valid) {
> +		dev_err(mas->dev, "Fifo and dma mode disabled!! can't xfer\n");
> +		mode = -EINVAL;
> +	} else if (fifo_disable) {
> +		mode = GENI_GPI_DMA;
> +	} else {
> +		mode = GENI_SE_FIFO;
> +	}
> +
> +	return mode;

Maybe make this tail:

	if (!dma_chan_valid && fifo_disable)
		return -EINVAL;

	return fifo_disable ? GENI_GPI_DMA : GENI_SE_FIFO;

At least to me that's more obvious.

> +}
> +
> +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 %s txn failed\n", __func__, tx ? "tx" : "rx");

"GSI DMA %s failed\n" is just as unique, without the need for __func__.

> +		return;
> +	}
> +
> +	if (!result->residue) {
> +		dev_dbg(gsi->mas->dev, "%s\n", __func__);

You have @tx, so how about including that to make the debug print
slightly more useful?

> +		if (tx)
> +			complete(&gsi->mas->tx_cb);
> +		else
> +			complete(&gsi->mas->rx_cb);
> +	} else {
> +		dev_err(gsi->mas->dev, "DMA xfer has pending: %d\n", 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)
> +{
> +	int ret = 0;
> +	unsigned long flags = DMA_PREP_INTERRUPT | DMA_CTRL_ACK;
> +	struct spi_geni_gsi *gsi;
> +	struct dma_slave_config config;
> +	struct gpi_spi_config peripheral;
> +
> +	memset(&config, 0, sizeof(config));

Assign {} during the declaration and you don't need to start with a
memset.

> +	memset(&peripheral, 0, sizeof(peripheral));
> +	config.peripheral_config = &peripheral;
> +	config.peripheral_size = sizeof(peripheral);
> +
> +	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;
> +		peripheral.set_config = true;
> +	}
> +
> +	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;
> +	}
> +
> +	peripheral.cs = spi_slv->chip_select;
> +
> +	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.pack_en = true;
> +	peripheral.word_len = xfer->bits_per_word - MIN_WORD_LEN;
> +	ret = get_spi_clk_cfg(mas->cur_speed_hz, mas,
> +			      &peripheral.clk_src, &peripheral.clk_div);
> +	if (ret) {
> +		dev_err(mas->dev, "%s:Err setting clks:%d\n", __func__, ret);

Please avoid the __func__.

> +		return ret;
> +	}
> +
> +	if (!xfer->cs_change) {
> +		if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> +			peripheral.fragmentation = FRAGMENTATION;
> +	}
> +
> +	gsi = &mas->gsi[mas->num_xfers];
> +	gsi->desc_cb.mas = mas;
> +	gsi->desc_cb.xfer = xfer;
> +	if (peripheral.cmd & SPI_RX) {
> +		dmaengine_slave_config(mas->rx, &config);
> +		gsi->rx_desc = dmaengine_prep_slave_single(mas->rx, xfer->rx_dma,
> +							   xfer->len, DMA_DEV_TO_MEM, flags);
> +		if (IS_ERR_OR_NULL(gsi->rx_desc)) {

Is this API really returning IS_ERR() or NULL on failure?

Looking at the GPI driver it seems to exclusively return NULL on failure
(for things that in most other subsystems would be -EINVAL).

> +			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->desc_cb;
> +		mas->num_rx_eot++;
> +	}
> +
> +	if (peripheral.cmd & SPI_TX_ONLY)
> +		mas->num_tx_eot++;
> +
> +	dmaengine_slave_config(mas->tx, &config);
> +	gsi->tx_desc = dmaengine_prep_slave_single(mas->tx, xfer->tx_dma,
> +						   xfer->len, DMA_MEM_TO_DEV, flags);
> +
> +	if (IS_ERR_OR_NULL(gsi->tx_desc)) {

Is there anything that will clean up rx_desc when this happens?

> +		dev_err(mas->dev, "Err setting up tx desc\n");
> +		return -EIO;
> +	}
> +	gsi->tx_desc->callback_result = spi_gsi_tx_callback_result;
> +	gsi->tx_desc->callback_param = &gsi->desc_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++;
> +	return ret;

ret can only be 0 here.

> +}
> +
> +static int spi_geni_map_buf(struct spi_geni_master *mas, struct spi_message *msg)
> +{
> +	struct spi_transfer *xfer;
> +	struct device *gsi_dev = mas->dev->parent;
> +
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if (xfer->rx_buf) {
> +			xfer->rx_dma = dma_map_single(gsi_dev, xfer->rx_buf,
> +						      xfer->len, DMA_FROM_DEVICE);
> +			if (dma_mapping_error(mas->dev, xfer->rx_dma)) {
> +				dev_err(mas->dev, "Err mapping buf\n");

You need to unmap rx_dma/tx_dma for all previous entries in
&msg->transfers.

> +				return -ENOMEM;
> +			}
> +		}
> +
> +		if (xfer->tx_buf) {
> +			xfer->tx_dma = dma_map_single(gsi_dev, (void *)xfer->tx_buf,
> +						      xfer->len, DMA_TO_DEVICE);
> +			if (dma_mapping_error(gsi_dev, xfer->tx_dma)) {
> +				dev_err(mas->dev, "Err mapping buf\n");
> +				dma_unmap_single(gsi_dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);

This should only be done if xfer->rx_buf != NULL, right?

> +				return -ENOMEM;
> +			}
> +		}
> +	};
> +
> +	return 0;
> +}
> +
> +static void spi_geni_unmap_buf(struct spi_geni_master *mas, struct spi_message *msg)
> +{
> +	struct spi_transfer *xfer;
> +	struct device *gsi_dev = mas->dev;
> +
> +	list_for_each_entry(xfer, &msg->transfers, transfer_list) {
> +		if (xfer->rx_buf)
> +			dma_unmap_single(gsi_dev, xfer->rx_dma, xfer->len, DMA_FROM_DEVICE);
> +		if (xfer->tx_buf)
> +			dma_unmap_single(gsi_dev, xfer->tx_dma, xfer->len, DMA_TO_DEVICE);
> +	};
> +}
> +
>  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);
> +	struct geni_se *se = &mas->se;
> +
> +	mas->cur_xfer_mode = get_xfer_mode(spi);
> +
> +	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +		geni_se_select_mode(se, GENI_SE_FIFO);
> +		reinit_completion(&mas->xfer_done);
> +		ret = setup_fifo_params(spi_msg->spi, spi);
> +		if (ret)
> +			dev_err(mas->dev, "Couldn't select mode %d\n", ret);

Afaict all error paths of setup_fifo_params() will have printed an error
message when we get here.

So

	switch (mas->cur_xfer_mode) {
	case GENI_SE_FIFO:
		...
		return setup_fifo_params();
	case GENI_GPI_DMA:
		...
		return spi_geni_map_buf();
	}

	return -EINVAL;

Seems cleaner to me.

> +
> +	} else if (mas->cur_xfer_mode == GENI_GPI_DMA) {
> +		mas->num_tx_eot = 0;
> +		mas->num_rx_eot = 0;
> +		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));
> +		geni_se_select_mode(se, GENI_GPI_DMA);
> +		ret = spi_geni_map_buf(mas, spi_msg);
> +
> +	} else {
> +		dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode);
> +		ret = -EINVAL;
> +	}
>  
> -	ret = setup_fifo_params(spi_msg->spi, spi);
> -	if (ret)
> -		dev_err(mas->dev, "Couldn't select mode %d\n", ret);
>  	return ret;
>  }
>  
> +static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg)
> +{
> +	struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> +	mas->cur_speed_hz = 0;
> +	mas->cur_bits_per_word = 0;
> +	if (mas->cur_xfer_mode == GENI_GPI_DMA)
> +		spi_geni_unmap_buf(mas, spi_msg);
> +	return 0;
> +}
> +
>  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;
> +	size_t gsi_sz;
> +	int ret = 0;
>  
>  	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;
> +		ret = -ENXIO;
> +		goto out_pm;
>  	}
>  	mas->tx_fifo_depth = geni_se_get_tx_fifo_depth(se);
>  
> @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
>  	spi_tx_cfg &= ~CS_TOGGLE;
>  	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>  
> +	mas->tx = dma_request_slave_channel(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));

I first wrote a rant about breaking backwards compatibility, but then at
last realized that this hunk doesn't actually modify "ret", so all this
error handling - and error printing - might still result in a successful
exit.

I also don't think it's accurate to have all errors treated the same
way, e.g. EPROBE_DEFER shouldn't be treated the same way as "no dma
property specified".

> +		ret = PTR_ERR(mas->tx);
> +		goto out_pm;
> +	} else {
> +		mas->rx = dma_request_slave_channel(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));
> +			dma_release_channel(mas->tx);
> +			ret = PTR_ERR(mas->rx);

Per the use of IS_ERR_OR_NULL() ret might become 0 here.

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

We got both rx and tx, but then this allocation failed, is that reason
for returning success without dma operational? (I'm not sure what the
right thing to do).

But checking for allocation failure isn't done with IS_ERR().

> +			dma_release_channel(mas->tx);

If you spin this hunk off into its own function then you can use the
idiomatic goto cleanup scheme and not have to repeat yourself here.

> +			dma_release_channel(mas->rx);
> +			mas->tx = NULL;
> +			mas->rx = NULL;
> +			goto out_pm;
> +		}
> +	}
> +
> +out_pm:
>  	pm_runtime_put(mas->dev);
> -	return 0;
> +	return ret;
>  }
>  
>  static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> @@ -420,6 +727,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>  {
>  	u32 m_cmd = 0;
>  	u32 len;
> +	u32 m_param = 0;
>  	struct geni_se *se = &mas->se;
>  	int ret;
>  
> @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>  		len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
>  	len &= TRANS_LEN_MSK;
>  
> +	if (!xfer->cs_change) {
> +		if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> +			m_param |= FRAGMENTATION;
> +	}
> +
>  	mas->cur_xfer = xfer;
>  	if (xfer->tx_buf) {
>  		m_cmd |= SPI_TX_ONLY;
> @@ -475,7 +788,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>  	 * interrupt could come in at any time now.
>  	 */
>  	spin_lock_irq(&mas->lock);
> -	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
> +	geni_se_setup_m_cmd(se, m_cmd, m_param);
>  
>  	/*
>  	 * TX_WATERMARK_REG should be set after SPI configuration and
> @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
>  				struct spi_transfer *xfer)
>  {
>  	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +	unsigned long timeout, jiffies;
> +	int ret = 0i, i;

0i? Is that a sign of you using vim?

>  
>  	/* Terminate and return success for 0 byte length transfer */
>  	if (!xfer->len)
> -		return 0;
> +		return ret;
> +
> +	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +		setup_fifo_xfer(xfer, mas, slv->mode, spi);
> +	} else {
> +		setup_gsi_xfer(xfer, mas, slv, spi);
> +		if (mas->num_xfers >= NUM_SPI_XFER ||
> +		    (list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))) {
> +			for (i = 0 ; i < mas->num_tx_eot; i++) {
> +				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;
> +				}
> +				spi_finalize_current_transfer(spi);
> +			}
> +			for (i = 0 ; i < mas->num_rx_eot; i++) {
> +				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
> +				timeout = wait_for_completion_timeout(&mas->tx_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);
> +			}
> +			if (mas->qn_err) {
> +				ret = -EIO;
> +				mas->qn_err = false;
> +				goto err_gsi_geni_transfer_one;
> +			}
> +		}
> +	}
>  
> -	setup_fifo_xfer(xfer, mas, slv->mode, spi);
> -	return 1;
> +	return ret;

All assignments to "ret" are followed by a goto
err_gsi_geni_transfer_one, so the only time you actually get here is
with ret has been untouched...in which case it's now 0, rather than the
1 it was previously.

> +
> +err_gsi_geni_transfer_one:
> +	dmaengine_terminate_all(mas->tx);
> +	return ret;
>  }
>  
>  static irqreturn_t geni_spi_isr(int irq, void *data)
> @@ -595,6 +947,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);
> @@ -632,15 +993,18 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	spi->num_chipselect = 4;
>  	spi->max_speed_hz = 50000000;
>  	spi->prepare_message = spi_geni_prepare_message;
> +	spi->unprepare_message = spi_geni_unprepare_message;
>  	spi->transfer_one = spi_geni_transfer_one;
>  	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->xfer_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);
> @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto spi_geni_probe_runtime_disable;
>  
> +	/*
> +	 * query 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
> +	 */

Is there no SE_DMA mode for SPI? (I know it's not implemented right now)
If we get there this would be cur_xfer_mode != GENI_GPI_DMA?

Regards,
Bjorn

> +	mas->cur_xfer_mode = get_xfer_mode(spi);
> +	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;
> -- 
> 2.26.2
> 

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

[Lukas Wunner]

On Mon, Jan 11, 2021 at 08:46:48PM +0530, Vinod Koul wrote:
> @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
>  	spi_tx_cfg &= ~CS_TOGGLE;
>  	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>  
> +	mas->tx = dma_request_slave_channel(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 out_pm;
> +	} else {
> +		mas->rx = dma_request_slave_channel(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));
> +			dma_release_channel(mas->tx);
> +			ret = PTR_ERR(mas->rx);
> +			goto out_pm;
> +		}

These channels need to be released in spi_geni_remove().

Also, you may want to fall back to PIO mode if channel allocation fails.

Thanks,

Lukas

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

[kernel test robot]

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

Hi Vinod,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.11-rc2]
[cannot apply to robh/for-next spi/for-next wsa/i2c/for-next linus/master v5.11-rc3 next-20210111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-and-enable-GPI-DMA-users/20210111-232052
base:    e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: mips-randconfig-s031-20210111 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-208-g46a52ca4-dirty
        # https://github.com/0day-ci/linux/commit/bcc874d4e76c4129ee33077e2828a311b2edcb96
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vinod-Koul/Add-and-enable-GPI-DMA-users/20210111-232052
        git checkout bcc874d4e76c4129ee33077e2828a311b2edcb96
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=mips 

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


"sparse warnings: (new ones prefixed by >>)"
   command-line: note: in included file:
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQUIRE redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_SEQ_CST redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQ_REL redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_RELEASE redefined
   builtin:0:0: sparse: this was the original definition
>> drivers/spi/spi-geni-qcom.c:811:19: sparse: sparse: constant 0i is not a valid number

vim +811 drivers/spi/spi-geni-qcom.c

561de45f72bd5f9b Girish Mahadevan 2018-10-03  804  
561de45f72bd5f9b Girish Mahadevan 2018-10-03  805  static int spi_geni_transfer_one(struct spi_master *spi,
561de45f72bd5f9b Girish Mahadevan 2018-10-03  806  				struct spi_device *slv,
561de45f72bd5f9b Girish Mahadevan 2018-10-03  807  				struct spi_transfer *xfer)
561de45f72bd5f9b Girish Mahadevan 2018-10-03  808  {
561de45f72bd5f9b Girish Mahadevan 2018-10-03  809  	struct spi_geni_master *mas = spi_master_get_devdata(spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  810  	unsigned long timeout, jiffies;
bcc874d4e76c4129 Vinod Koul       2021-01-11 @811  	int ret = 0i, i;
561de45f72bd5f9b Girish Mahadevan 2018-10-03  812  
561de45f72bd5f9b Girish Mahadevan 2018-10-03  813  	/* Terminate and return success for 0 byte length transfer */
561de45f72bd5f9b Girish Mahadevan 2018-10-03  814  	if (!xfer->len)
bcc874d4e76c4129 Vinod Koul       2021-01-11  815  		return ret;
561de45f72bd5f9b Girish Mahadevan 2018-10-03  816  
bcc874d4e76c4129 Vinod Koul       2021-01-11  817  	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
561de45f72bd5f9b Girish Mahadevan 2018-10-03  818  		setup_fifo_xfer(xfer, mas, slv->mode, spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  819  	} else {
bcc874d4e76c4129 Vinod Koul       2021-01-11  820  		setup_gsi_xfer(xfer, mas, slv, spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  821  		if (mas->num_xfers >= NUM_SPI_XFER ||
bcc874d4e76c4129 Vinod Koul       2021-01-11  822  		    (list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  823  			for (i = 0 ; i < mas->num_tx_eot; i++) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  824  				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
bcc874d4e76c4129 Vinod Koul       2021-01-11  825  				timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
bcc874d4e76c4129 Vinod Koul       2021-01-11  826  				if (timeout <= 0) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  827  					dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);
bcc874d4e76c4129 Vinod Koul       2021-01-11  828  					ret = -ETIMEDOUT;
bcc874d4e76c4129 Vinod Koul       2021-01-11  829  					goto err_gsi_geni_transfer_one;
bcc874d4e76c4129 Vinod Koul       2021-01-11  830  				}
bcc874d4e76c4129 Vinod Koul       2021-01-11  831  				spi_finalize_current_transfer(spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  832  			}
bcc874d4e76c4129 Vinod Koul       2021-01-11  833  			for (i = 0 ; i < mas->num_rx_eot; i++) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  834  				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
bcc874d4e76c4129 Vinod Koul       2021-01-11  835  				timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
bcc874d4e76c4129 Vinod Koul       2021-01-11  836  				if (timeout <= 0) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  837  					dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout);
bcc874d4e76c4129 Vinod Koul       2021-01-11  838  					ret = -ETIMEDOUT;
bcc874d4e76c4129 Vinod Koul       2021-01-11  839  					goto err_gsi_geni_transfer_one;
bcc874d4e76c4129 Vinod Koul       2021-01-11  840  				}
bcc874d4e76c4129 Vinod Koul       2021-01-11  841  				spi_finalize_current_transfer(spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  842  			}
bcc874d4e76c4129 Vinod Koul       2021-01-11  843  			if (mas->qn_err) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  844  				ret = -EIO;
bcc874d4e76c4129 Vinod Koul       2021-01-11  845  				mas->qn_err = false;
bcc874d4e76c4129 Vinod Koul       2021-01-11  846  				goto err_gsi_geni_transfer_one;
bcc874d4e76c4129 Vinod Koul       2021-01-11  847  			}
bcc874d4e76c4129 Vinod Koul       2021-01-11  848  		}
bcc874d4e76c4129 Vinod Koul       2021-01-11  849  	}
bcc874d4e76c4129 Vinod Koul       2021-01-11  850  
bcc874d4e76c4129 Vinod Koul       2021-01-11  851  	return ret;
bcc874d4e76c4129 Vinod Koul       2021-01-11  852  
bcc874d4e76c4129 Vinod Koul       2021-01-11  853  err_gsi_geni_transfer_one:
bcc874d4e76c4129 Vinod Koul       2021-01-11  854  	dmaengine_terminate_all(mas->tx);
bcc874d4e76c4129 Vinod Koul       2021-01-11  855  	return ret;
561de45f72bd5f9b Girish Mahadevan 2018-10-03  856  }
561de45f72bd5f9b Girish Mahadevan 2018-10-03  857  

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

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

[-- Attachment #3: Type: text/plain, Size: 149 bytes --]

_______________________________________________
kbuild mailing list -- kbuild@lists.01.org
To unsubscribe send an email to kbuild-leave@lists.01.org

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

[kernel test robot]

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

Hi Vinod,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v5.11-rc2]
[cannot apply to robh/for-next spi/for-next wsa/i2c/for-next linus/master v5.11-rc3 next-20210111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vinod-Koul/Add-and-enable-GPI-DMA-users/20210111-232052
base:    e71ba9452f0b5b2e8dc8aa5445198cd9214a6a62
config: mips-randconfig-s031-20210111 (attached as .config)
compiler: mipsel-linux-gcc (GCC) 9.3.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.3-208-g46a52ca4-dirty
        # https://github.com/0day-ci/linux/commit/bcc874d4e76c4129ee33077e2828a311b2edcb96
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vinod-Koul/Add-and-enable-GPI-DMA-users/20210111-232052
        git checkout bcc874d4e76c4129ee33077e2828a311b2edcb96
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=mips 

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


"sparse warnings: (new ones prefixed by >>)"
   command-line: note: in included file:
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQUIRE redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_SEQ_CST redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_ACQ_REL redefined
   builtin:0:0: sparse: this was the original definition
   builtin:1:9: sparse: sparse: preprocessor token __ATOMIC_RELEASE redefined
   builtin:0:0: sparse: this was the original definition
>> drivers/spi/spi-geni-qcom.c:811:19: sparse: sparse: constant 0i is not a valid number

vim +811 drivers/spi/spi-geni-qcom.c

561de45f72bd5f9b Girish Mahadevan 2018-10-03  804  
561de45f72bd5f9b Girish Mahadevan 2018-10-03  805  static int spi_geni_transfer_one(struct spi_master *spi,
561de45f72bd5f9b Girish Mahadevan 2018-10-03  806  				struct spi_device *slv,
561de45f72bd5f9b Girish Mahadevan 2018-10-03  807  				struct spi_transfer *xfer)
561de45f72bd5f9b Girish Mahadevan 2018-10-03  808  {
561de45f72bd5f9b Girish Mahadevan 2018-10-03  809  	struct spi_geni_master *mas = spi_master_get_devdata(spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  810  	unsigned long timeout, jiffies;
bcc874d4e76c4129 Vinod Koul       2021-01-11 @811  	int ret = 0i, i;
561de45f72bd5f9b Girish Mahadevan 2018-10-03  812  
561de45f72bd5f9b Girish Mahadevan 2018-10-03  813  	/* Terminate and return success for 0 byte length transfer */
561de45f72bd5f9b Girish Mahadevan 2018-10-03  814  	if (!xfer->len)
bcc874d4e76c4129 Vinod Koul       2021-01-11  815  		return ret;
561de45f72bd5f9b Girish Mahadevan 2018-10-03  816  
bcc874d4e76c4129 Vinod Koul       2021-01-11  817  	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
561de45f72bd5f9b Girish Mahadevan 2018-10-03  818  		setup_fifo_xfer(xfer, mas, slv->mode, spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  819  	} else {
bcc874d4e76c4129 Vinod Koul       2021-01-11  820  		setup_gsi_xfer(xfer, mas, slv, spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  821  		if (mas->num_xfers >= NUM_SPI_XFER ||
bcc874d4e76c4129 Vinod Koul       2021-01-11  822  		    (list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  823  			for (i = 0 ; i < mas->num_tx_eot; i++) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  824  				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
bcc874d4e76c4129 Vinod Koul       2021-01-11  825  				timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
bcc874d4e76c4129 Vinod Koul       2021-01-11  826  				if (timeout <= 0) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  827  					dev_err(mas->dev, "Tx[%d] timeout%lu\n", i, timeout);
bcc874d4e76c4129 Vinod Koul       2021-01-11  828  					ret = -ETIMEDOUT;
bcc874d4e76c4129 Vinod Koul       2021-01-11  829  					goto err_gsi_geni_transfer_one;
bcc874d4e76c4129 Vinod Koul       2021-01-11  830  				}
bcc874d4e76c4129 Vinod Koul       2021-01-11  831  				spi_finalize_current_transfer(spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  832  			}
bcc874d4e76c4129 Vinod Koul       2021-01-11  833  			for (i = 0 ; i < mas->num_rx_eot; i++) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  834  				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
bcc874d4e76c4129 Vinod Koul       2021-01-11  835  				timeout = wait_for_completion_timeout(&mas->tx_cb, jiffies);
bcc874d4e76c4129 Vinod Koul       2021-01-11  836  				if (timeout <= 0) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  837  					dev_err(mas->dev, "Rx[%d] timeout%lu\n", i, timeout);
bcc874d4e76c4129 Vinod Koul       2021-01-11  838  					ret = -ETIMEDOUT;
bcc874d4e76c4129 Vinod Koul       2021-01-11  839  					goto err_gsi_geni_transfer_one;
bcc874d4e76c4129 Vinod Koul       2021-01-11  840  				}
bcc874d4e76c4129 Vinod Koul       2021-01-11  841  				spi_finalize_current_transfer(spi);
bcc874d4e76c4129 Vinod Koul       2021-01-11  842  			}
bcc874d4e76c4129 Vinod Koul       2021-01-11  843  			if (mas->qn_err) {
bcc874d4e76c4129 Vinod Koul       2021-01-11  844  				ret = -EIO;
bcc874d4e76c4129 Vinod Koul       2021-01-11  845  				mas->qn_err = false;
bcc874d4e76c4129 Vinod Koul       2021-01-11  846  				goto err_gsi_geni_transfer_one;
bcc874d4e76c4129 Vinod Koul       2021-01-11  847  			}
bcc874d4e76c4129 Vinod Koul       2021-01-11  848  		}
bcc874d4e76c4129 Vinod Koul       2021-01-11  849  	}
bcc874d4e76c4129 Vinod Koul       2021-01-11  850  
bcc874d4e76c4129 Vinod Koul       2021-01-11  851  	return ret;
bcc874d4e76c4129 Vinod Koul       2021-01-11  852  
bcc874d4e76c4129 Vinod Koul       2021-01-11  853  err_gsi_geni_transfer_one:
bcc874d4e76c4129 Vinod Koul       2021-01-11  854  	dmaengine_terminate_all(mas->tx);
bcc874d4e76c4129 Vinod Koul       2021-01-11  855  	return ret;
561de45f72bd5f9b Girish Mahadevan 2018-10-03  856  }
561de45f72bd5f9b Girish Mahadevan 2018-10-03  857  

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

_______________________________________________
kbuild mailing list -- kbuild(a)lists.01.org
To unsubscribe send an email to kbuild-leave(a)lists.01.org

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

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

[Doug Anderson]

Hi,

On Mon, Jan 11, 2021 at 7:17 AM 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 | 395 +++++++++++++++++++++++++++++++++++-
>  1 file changed, 384 insertions(+), 11 deletions(-)

I did a somewhat cursory review, mostly focusing on making sure that
the non-GPI/GSI stuff doesn't regress.  ;-)  I think you've already
got a bunch of feedback for v2 so I'll plan to look back when I see
the v2 and maybe will find time to look at some of the GSI/GPI stuff
too...


> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 512e925d5ea4..5bb0e2192734 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c
> @@ -2,6 +2,8 @@
>  // 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/interrupt.h>
>  #include <linux/io.h>
>  #include <linux/log2.h>
> @@ -10,6 +12,7 @@
>  #include <linux/pm_opp.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/qcom-geni-se.h>
> +#include <linux/dma/qcom-gpi-dma.h>

nit: sort ordering doesn't match other includes.  It seems like
existing includes in this file are sorted ignoring subdirs.


>  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);
> +       struct geni_se *se = &mas->se;
> +
> +       mas->cur_xfer_mode = get_xfer_mode(spi);
> +
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +               geni_se_select_mode(se, GENI_SE_FIFO);

You don't need to do this over and over again.  We set up FIFO mode in
spi_geni_init() and it'll never change.


> +               reinit_completion(&mas->xfer_done);
> +               ret = setup_fifo_params(spi_msg->spi, spi);
> +               if (ret)
> +                       dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> +
> +       } else if (mas->cur_xfer_mode == GENI_GPI_DMA) {
> +               mas->num_tx_eot = 0;
> +               mas->num_rx_eot = 0;
> +               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));
> +               geni_se_select_mode(se, GENI_GPI_DMA);
> +               ret = spi_geni_map_buf(mas, spi_msg);
> +

Extra blank line?

> +       } else {
> +               dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode);

Please no __func__ in error messages unless you're doing a non-"dev"
print.  If you want to fill your log with function names you should
redefine the generic dev_xxx() functions to prefix "__func__" in your
own kernel.  You probably don't even need a printout here since
get_xfer_mode() already printed.


> +static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg)
> +{
> +       struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> +
> +       mas->cur_speed_hz = 0;
> +       mas->cur_bits_per_word = 0;

I think doing the above zeros will make the code a bunch slower for
FIFO mode.  Specifically we can avoid a whole bunch of (very slow)
interconnect code if the speed doesn't change between transfers and
the runtime PM auto power down hasn't hit.


> @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
>         spi_tx_cfg &= ~CS_TOGGLE;
>         writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>
> +       mas->tx = dma_request_slave_channel(mas->dev, "tx");
> +       if (IS_ERR_OR_NULL(mas->tx)) {

I didn't look too closely at this since I think Mark wanted you to
look into the core DMA support, but...

In general, don't you only need to do the DMA requests if you're in GPI mode?


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

No need for else since last "if" ended up with goto".


> +               mas->rx = dma_request_slave_channel(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));
> +                       dma_release_channel(mas->tx);
> +                       ret = PTR_ERR(mas->rx);
> +                       goto out_pm;
> +               }
> +
> +               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)) {

Is it ever an error?  Just check against NULL?


> +                       dma_release_channel(mas->tx);
> +                       dma_release_channel(mas->rx);
> +                       mas->tx = NULL;
> +                       mas->rx = NULL;

ret = -ENOMEM ?


>  static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>                 len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
>         len &= TRANS_LEN_MSK;
>
> +       if (!xfer->cs_change) {
> +               if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> +                       m_param |= FRAGMENTATION;
> +       }

Why are you changing this?  It's for FIFO mode which works correctly
the way it is.  We _always_ want the FRAGMENTATION bit set because we
explicitly set the CS.  I haven't tried it, but I'd imagine this
change breaks stuff?  I'd expect all changes in setup_fifo_xfer() to
be removed from your patch.  If there's some reason you need them then
post a separate patch.


> @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
>                                 struct spi_transfer *xfer)
>  {
>         struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +       unsigned long timeout, jiffies;

Doesn't this shadow the global "jiffies"?


> +       int ret = 0i, i;
>
>         /* Terminate and return success for 0 byte length transfer */
>         if (!xfer->len)
> -               return 0;
> +               return ret;

It feels more documenting to just leave this as "return 0".


> +
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +               setup_fifo_xfer(xfer, mas, slv->mode, spi);

It's super important to return "1" in this case to tell the SPI core
that you left the transfer in progress.  You don't do that anymore, so
boom.


> +       } else {
> +               setup_gsi_xfer(xfer, mas, slv, spi);

This feels very non-symmetric.  In the FIFO case you just call a
function.  in the GSI case you have a whole pile of stuff inline.  Can
all the stuff below be stuck in setup_gsi_xfer() or maybe you can add
an extra wrapper function?  That means you don't need the weird goto
flow in this function...

> @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>         if (ret)
>                 goto spi_geni_probe_runtime_disable;
>
> +       /*
> +        * query 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
> +        */
> +       mas->cur_xfer_mode = get_xfer_mode(spi);
> +       if (mas->cur_xfer_mode == GENI_SE_FIFO)

nit: check against != GPI mode?

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

[Vinod Koul]

On 12-01-21, 16:01, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 11, 2021 at 7:17 AM 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 | 395 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 384 insertions(+), 11 deletions(-)
> 
> I did a somewhat cursory review, mostly focusing on making sure that
> the non-GPI/GSI stuff doesn't regress.  ;-)  I think you've already
> got a bunch of feedback for v2 so I'll plan to look back when I see
> the v2 and maybe will find time to look at some of the GSI/GPI stuff
> too...

Thanks for the comments, I will update the comments and post v2. All the
below comments look good to me, I will respin..


> > diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> > index 512e925d5ea4..5bb0e2192734 100644
> > --- a/drivers/spi/spi-geni-qcom.c
> > +++ b/drivers/spi/spi-geni-qcom.c
> > @@ -2,6 +2,8 @@
> >  // 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/interrupt.h>
> >  #include <linux/io.h>
> >  #include <linux/log2.h>
> > @@ -10,6 +12,7 @@
> >  #include <linux/pm_opp.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/qcom-geni-se.h>
> > +#include <linux/dma/qcom-gpi-dma.h>
> 
> nit: sort ordering doesn't match other includes.  It seems like
> existing includes in this file are sorted ignoring subdirs.
> 
> 
> >  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);
> > +       struct geni_se *se = &mas->se;
> > +
> > +       mas->cur_xfer_mode = get_xfer_mode(spi);
> > +
> > +       if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> > +               geni_se_select_mode(se, GENI_SE_FIFO);
> 
> You don't need to do this over and over again.  We set up FIFO mode in
> spi_geni_init() and it'll never change.
> 
> 
> > +               reinit_completion(&mas->xfer_done);
> > +               ret = setup_fifo_params(spi_msg->spi, spi);
> > +               if (ret)
> > +                       dev_err(mas->dev, "Couldn't select mode %d\n", ret);
> > +
> > +       } else if (mas->cur_xfer_mode == GENI_GPI_DMA) {
> > +               mas->num_tx_eot = 0;
> > +               mas->num_rx_eot = 0;
> > +               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));
> > +               geni_se_select_mode(se, GENI_GPI_DMA);
> > +               ret = spi_geni_map_buf(mas, spi_msg);
> > +
> 
> Extra blank line?
> 
> > +       } else {
> > +               dev_err(mas->dev, "%s: Couldn't select mode %d", __func__, mas->cur_xfer_mode);
> 
> Please no __func__ in error messages unless you're doing a non-"dev"
> print.  If you want to fill your log with function names you should
> redefine the generic dev_xxx() functions to prefix "__func__" in your
> own kernel.  You probably don't even need a printout here since
> get_xfer_mode() already printed.
> 
> 
> > +static int spi_geni_unprepare_message(struct spi_master *spi_mas, struct spi_message *spi_msg)
> > +{
> > +       struct spi_geni_master *mas = spi_master_get_devdata(spi_mas);
> > +
> > +       mas->cur_speed_hz = 0;
> > +       mas->cur_bits_per_word = 0;
> 
> I think doing the above zeros will make the code a bunch slower for
> FIFO mode.  Specifically we can avoid a whole bunch of (very slow)
> interconnect code if the speed doesn't change between transfers and
> the runtime PM auto power down hasn't hit.
> 
> 
> > @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
> >         spi_tx_cfg &= ~CS_TOGGLE;
> >         writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
> >
> > +       mas->tx = dma_request_slave_channel(mas->dev, "tx");
> > +       if (IS_ERR_OR_NULL(mas->tx)) {
> 
> I didn't look too closely at this since I think Mark wanted you to
> look into the core DMA support, but...
> 
> In general, don't you only need to do the DMA requests if you're in GPI mode?
> 
> 
> > +               dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
> > +               ret = PTR_ERR(mas->tx);
> > +               goto out_pm;
> > +       } else {
> 
> No need for else since last "if" ended up with goto".
> 
> 
> > +               mas->rx = dma_request_slave_channel(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));
> > +                       dma_release_channel(mas->tx);
> > +                       ret = PTR_ERR(mas->rx);
> > +                       goto out_pm;
> > +               }
> > +
> > +               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)) {
> 
> Is it ever an error?  Just check against NULL?
> 
> 
> > +                       dma_release_channel(mas->tx);
> > +                       dma_release_channel(mas->rx);
> > +                       mas->tx = NULL;
> > +                       mas->rx = NULL;
> 
> ret = -ENOMEM ?
> 
> 
> >  static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> > @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
> >                 len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
> >         len &= TRANS_LEN_MSK;
> >
> > +       if (!xfer->cs_change) {
> > +               if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> > +                       m_param |= FRAGMENTATION;
> > +       }
> 
> Why are you changing this?  It's for FIFO mode which works correctly
> the way it is.  We _always_ want the FRAGMENTATION bit set because we
> explicitly set the CS.  I haven't tried it, but I'd imagine this
> change breaks stuff?  I'd expect all changes in setup_fifo_xfer() to
> be removed from your patch.  If there's some reason you need them then
> post a separate patch.
> 
> 
> > @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
> >                                 struct spi_transfer *xfer)
> >  {
> >         struct spi_geni_master *mas = spi_master_get_devdata(spi);
> > +       unsigned long timeout, jiffies;
> 
> Doesn't this shadow the global "jiffies"?
> 
> 
> > +       int ret = 0i, i;
> >
> >         /* Terminate and return success for 0 byte length transfer */
> >         if (!xfer->len)
> > -               return 0;
> > +               return ret;
> 
> It feels more documenting to just leave this as "return 0".
> 
> 
> > +
> > +       if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> > +               setup_fifo_xfer(xfer, mas, slv->mode, spi);
> 
> It's super important to return "1" in this case to tell the SPI core
> that you left the transfer in progress.  You don't do that anymore, so
> boom.
> 
> 
> > +       } else {
> > +               setup_gsi_xfer(xfer, mas, slv, spi);
> 
> This feels very non-symmetric.  In the FIFO case you just call a
> function.  in the GSI case you have a whole pile of stuff inline.  Can
> all the stuff below be stuck in setup_gsi_xfer() or maybe you can add
> an extra wrapper function?  That means you don't need the weird goto
> flow in this function...
> 
> > @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
> >         if (ret)
> >                 goto spi_geni_probe_runtime_disable;
> >
> > +       /*
> > +        * query 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
> > +        */
> > +       mas->cur_xfer_mode = get_xfer_mode(spi);
> > +       if (mas->cur_xfer_mode == GENI_SE_FIFO)
> 
> nit: check against != GPI mode?

-- 
~Vinod

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

[Dmitry Baryshkov]

On 11/01/2021 18:16, Vinod Koul 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 | 395 +++++++++++++++++++++++++++++++++++-
>   1 file changed, 384 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/spi/spi-geni-qcom.c b/drivers/spi/spi-geni-qcom.c
> index 512e925d5ea4..5bb0e2192734 100644
> --- a/drivers/spi/spi-geni-qcom.c
> +++ b/drivers/spi/spi-geni-qcom.c

[skipped]

> @@ -328,8 +609,34 @@ static int spi_geni_init(struct spi_geni_master *mas)
>   	spi_tx_cfg &= ~CS_TOGGLE;
>   	writel(spi_tx_cfg, se->base + SE_SPI_TRANS_CFG);
>   
> +	mas->tx = dma_request_slave_channel(mas->dev, "tx");
> +	if (IS_ERR_OR_NULL(mas->tx)) {

dma_request_slave_channel() is deprecated. Also it does not return an 
actualy error, it always returns NULL. So the error path here is bugged.
Judging from the rest of the driver, it might be logical to select the 
transfer mode at the probe time, then it would be possible to skip 
checking for DMA channels if FIFO mode is selected.

> +		dev_err(mas->dev, "Failed to get tx DMA ch %ld", PTR_ERR(mas->tx));
> +		ret = PTR_ERR(mas->tx);
> +		goto out_pm;
> +	} else {
> +		mas->rx = dma_request_slave_channel(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));
> +			dma_release_channel(mas->tx);
> +			ret = PTR_ERR(mas->rx);
> +			goto out_pm;
> +		}
> +
> +		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)) {
> +			dma_release_channel(mas->tx);
> +			dma_release_channel(mas->rx);
> +			mas->tx = NULL;
> +			mas->rx = NULL;
> +			goto out_pm;
> +		}
> +	}
> +
> +out_pm:
>   	pm_runtime_put(mas->dev);
> -	return 0;
> +	return ret;
>   }
>   
>   static unsigned int geni_byte_per_fifo_word(struct spi_geni_master *mas)
> @@ -420,6 +727,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>   {
>   	u32 m_cmd = 0;
>   	u32 len;
> +	u32 m_param = 0;
>   	struct geni_se *se = &mas->se;
>   	int ret;
>   
> @@ -457,6 +765,11 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>   		len = xfer->len / (mas->cur_bits_per_word / BITS_PER_BYTE + 1);
>   	len &= TRANS_LEN_MSK;
>   
> +	if (!xfer->cs_change) {
> +		if (!list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))
> +			m_param |= FRAGMENTATION;
> +	}
> +
>   	mas->cur_xfer = xfer;
>   	if (xfer->tx_buf) {
>   		m_cmd |= SPI_TX_ONLY;
> @@ -475,7 +788,7 @@ static void setup_fifo_xfer(struct spi_transfer *xfer,
>   	 * interrupt could come in at any time now.
>   	 */
>   	spin_lock_irq(&mas->lock);
> -	geni_se_setup_m_cmd(se, m_cmd, FRAGMENTATION);
> +	geni_se_setup_m_cmd(se, m_cmd, m_param);
>   
>   	/*
>   	 * TX_WATERMARK_REG should be set after SPI configuration and
> @@ -494,13 +807,52 @@ static int spi_geni_transfer_one(struct spi_master *spi,
>   				struct spi_transfer *xfer)
>   {
>   	struct spi_geni_master *mas = spi_master_get_devdata(spi);
> +	unsigned long timeout, jiffies;
> +	int ret = 0i, i;
>   
>   	/* Terminate and return success for 0 byte length transfer */
>   	if (!xfer->len)
> -		return 0;
> +		return ret;
> +
> +	if (mas->cur_xfer_mode == GENI_SE_FIFO) {
> +		setup_fifo_xfer(xfer, mas, slv->mode, spi);
> +	} else {
> +		setup_gsi_xfer(xfer, mas, slv, spi);
> +		if (mas->num_xfers >= NUM_SPI_XFER ||
> +		    (list_is_last(&xfer->transfer_list, &spi->cur_msg->transfers))) {
> +			for (i = 0 ; i < mas->num_tx_eot; i++) {
> +				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;
> +				}
> +				spi_finalize_current_transfer(spi);
> +			}
> +			for (i = 0 ; i < mas->num_rx_eot; i++) {
> +				jiffies = msecs_to_jiffies(SPI_XFER_TIMEOUT_MS);
> +				timeout = wait_for_completion_timeout(&mas->tx_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);
> +			}
> +			if (mas->qn_err) {
> +				ret = -EIO;
> +				mas->qn_err = false;
> +				goto err_gsi_geni_transfer_one;
> +			}
> +		}
> +	}
>   
> -	setup_fifo_xfer(xfer, mas, slv->mode, spi);
> -	return 1;
> +	return ret;
> +
> +err_gsi_geni_transfer_one:
> +	dmaengine_terminate_all(mas->tx);
> +	return ret;
>   }
>   
>   static irqreturn_t geni_spi_isr(int irq, void *data)
> @@ -595,6 +947,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);
> @@ -632,15 +993,18 @@ static int spi_geni_probe(struct platform_device *pdev)
>   	spi->num_chipselect = 4;
>   	spi->max_speed_hz = 50000000;
>   	spi->prepare_message = spi_geni_prepare_message;
> +	spi->unprepare_message = spi_geni_unprepare_message;
>   	spi->transfer_one = spi_geni_transfer_one;
>   	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->xfer_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);
> @@ -661,6 +1025,15 @@ static int spi_geni_probe(struct platform_device *pdev)
>   	if (ret)
>   		goto spi_geni_probe_runtime_disable;
>   
> +	/*
> +	 * query 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
> +	 */
> +	mas->cur_xfer_mode = get_xfer_mode(spi);
> +	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;
> 


-- 
With best wishes
Dmitry

[PATCH 5/7] 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 | 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;
+	struct dma_async_tx_descriptor *tx_desc;
+	struct dma_async_tx_descriptor *rx_desc;
+	enum i2c_se_mode se_mode;
+	bool cmd_done;
+	bool is_shared;
 };
 
 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));
+	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");
+		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;
+		peripheral.set_config =  true;
+	}
+
+	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;
+		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;
+			}
+			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);
+			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);
+		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);
+			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);
+			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:
+		if (gi2c->err) {
+			dmaengine_terminate_all(gi2c->tx_c);
+			gi2c->cfg_sent = 0;
+		}
+		if (msgs[i].flags & I2C_M_RD)
+			dma_unmap_single(rx_dev, gi2c->rx_ph, msgs[i].len, DMA_FROM_DEVICE);
+		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;
+	}
+
+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;
+	} else {
+		/* Don't set shared flag in non-GSI mode */
+		gi2c->is_shared = false;
+	}
+
 	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;
+
+		if (unlikely(proto != GENI_SE_I2C)) {
+			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;
+		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);
+
+			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));
+	}
+
+	if (gi2c->se_mode == FIFO_SE_DMA)
+		enable_irq(gi2c->irq);
 
-	enable_irq(gi2c->irq);
 	gi2c->suspended = 0;
 	return 0;
 }
-- 
2.26.2

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

[Bjorn Andersson]

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

> 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?

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

bool gsi_only;

> +	bool cmd_done;

Unused?

> +	bool is_shared;

Used but meaningless?

>  };
>  
>  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.

> +	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?

> +		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?

> +		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.

> +	}
> +
> +	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.

> +		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?

> +			}
> +			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?

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

> +			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)?

> +		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?

> +			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?

> +			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?

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

Is this a bool or an int?

> +		}
> +		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.

> +		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.

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

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

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

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

> +	}
> +
>  	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.

> +
> +		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()

> +			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"?

Please skip the _relaxed

> +		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.

> +
> +			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?

Regards,
Bjorn

> +	}
> +
> +	if (gi2c->se_mode == FIFO_SE_DMA)
> +		enable_irq(gi2c->irq);
>  
> -	enable_irq(gi2c->irq);
>  	gi2c->suspended = 0;
>  	return 0;
>  }
> -- 
> 2.26.2
> 

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

[Vinod Koul]

On 11-01-21, 12:14, Bjorn Andersson wrote:
> On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:
> 
> > 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

> 
> > +	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

> 
> > +	bool cmd_done;
> 
> Unused?

heh, will remove..

> > +	bool is_shared;
> 
> Used but meaningless?

Will drop

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

> 
> > +	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..

> 
> > +		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 :)

> 
> > +		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

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

> 
> > +		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

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

> 
> > +			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

> 
> > +		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?
> 
> > +			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

> 
> > +			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?
> 
> > +		if (gi2c->err) {
> > +			dmaengine_terminate_all(gi2c->tx_c);
> > +			gi2c->cfg_sent = 0;
> 
> Is this a bool or an int?
> 
> > +		}
> > +		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.
> 
> > +		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.
> 
> > +	}
> > +
> > +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();
> 
> > +	} else {
> > +		/* Don't set shared flag in non-GSI mode */
> > +		gi2c->is_shared = false;
> 
> I don't see this flag being looked at elsewhere.
> 
> > +	}
> > +
> >  	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.
> 
> > +
> > +		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()
> 
> > +			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

> 
> > +		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

> 
> > +
> > +			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

[PATCH 6/7] arm64: dts: qcom: sdm845: Add gpi dma node

[Vinod Koul]

This add the device node for gpi dma0 instances found in sdm845.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 arch/arm64/boot/dts/qcom/sdm845.dtsi | 46 ++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index bcf888381f14..c9a127bbd606 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -1114,6 +1114,29 @@ opp-128000000 {
 			};
 		};
 
+		gpi_dma0: dma-controller@800000 {
+			#dma-cells = <3>;
+			compatible = "qcom,sdm845-gpi-dma";
+			reg = <0 0x00800000 0 0x60000>;
+			interrupts = <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 246 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 254 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 255 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>;
+			dma-channels = <13>;
+			dma-channel-mask = <0xfa>;
+			iommus = <&apps_smmu 0x0016 0x0>;
+			status = "disabled";
+		};
+
 		qupv3_id_0: geniqup@8c0000 {
 			compatible = "qcom,geni-se-qup";
 			reg = <0 0x008c0000 0 0x6000>;
@@ -1533,6 +1556,29 @@ uart7: serial@89c000 {
 			};
 		};
 
+		gpi_dma1: dma-controller@0xa00000 {
+			#dma-cells = <3>;
+			compatible = "qcom,sdm845-gpi-dma";
+			reg = <0 0x00a00000 0 0x60000>;
+			interrupts = <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 293 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 294 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 295 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 296 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 297 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>,
+				     <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
+			dma-channels = <13>;
+			dma-channel-mask = <0xfa>;
+			iommus = <&apps_smmu 0x06d6 0x0>;
+			status = "disabled";
+		};
+
 		qupv3_id_1: geniqup@ac0000 {
 			compatible = "qcom,geni-se-qup";
 			reg = <0 0x00ac0000 0 0x6000>;
-- 
2.26.2

Re: [PATCH 6/7] arm64: dts: qcom: sdm845: Add gpi dma node

[Bjorn Andersson]

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

> This add the device node for gpi dma0 instances found in sdm845.

I think the 0 in "dma0" should go?

Apart from that, this looks good.

> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845.dtsi | 46 ++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index bcf888381f14..c9a127bbd606 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1114,6 +1114,29 @@ opp-128000000 {
>  			};
>  		};
>  
> +		gpi_dma0: dma-controller@800000 {
> +			#dma-cells = <3>;

Nit. I know #dma-cells are important to you ;) but I would prefer to
have the standard properties (e.g. compatible) first.

Regards,
Bjorn

> +			compatible = "qcom,sdm845-gpi-dma";
> +			reg = <0 0x00800000 0 0x60000>;
> +			interrupts = <GIC_SPI 244 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 245 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 246 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 247 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 248 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 249 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 250 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 251 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 252 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 253 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 254 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 255 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 256 IRQ_TYPE_LEVEL_HIGH>;
> +			dma-channels = <13>;
> +			dma-channel-mask = <0xfa>;
> +			iommus = <&apps_smmu 0x0016 0x0>;
> +			status = "disabled";
> +		};
> +
>  		qupv3_id_0: geniqup@8c0000 {
>  			compatible = "qcom,geni-se-qup";
>  			reg = <0 0x008c0000 0 0x6000>;
> @@ -1533,6 +1556,29 @@ uart7: serial@89c000 {
>  			};
>  		};
>  
> +		gpi_dma1: dma-controller@0xa00000 {
> +			#dma-cells = <3>;
> +			compatible = "qcom,sdm845-gpi-dma";
> +			reg = <0 0x00a00000 0 0x60000>;
> +			interrupts = <GIC_SPI 279 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 280 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 281 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 282 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 283 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 284 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 293 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 294 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 295 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 296 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 297 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 298 IRQ_TYPE_LEVEL_HIGH>,
> +				     <GIC_SPI 299 IRQ_TYPE_LEVEL_HIGH>;
> +			dma-channels = <13>;
> +			dma-channel-mask = <0xfa>;
> +			iommus = <&apps_smmu 0x06d6 0x0>;
> +			status = "disabled";
> +		};
> +
>  		qupv3_id_1: geniqup@ac0000 {
>  			compatible = "qcom,geni-se-qup";
>  			reg = <0 0x00ac0000 0 0x6000>;
> -- 
> 2.26.2
> 

Re: [PATCH 6/7] arm64: dts: qcom: sdm845: Add gpi dma node

[Vinod Koul]

On 11-01-21, 12:23, Bjorn Andersson wrote:
> On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:
> 
> > This add the device node for gpi dma0 instances found in sdm845.
> 
> I think the 0 in "dma0" should go?

oops, will update

> 
> Apart from that, this looks good.
> 
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sdm845.dtsi | 46 ++++++++++++++++++++++++++++
> >  1 file changed, 46 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > index bcf888381f14..c9a127bbd606 100644
> > --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> > @@ -1114,6 +1114,29 @@ opp-128000000 {
> >  			};
> >  		};
> >  
> > +		gpi_dma0: dma-controller@800000 {
> > +			#dma-cells = <3>;
> 
> Nit. I know #dma-cells are important to you ;) but I would prefer to
> have the standard properties (e.g. compatible) first.

Yes will reorder
-- 
~Vinod

[PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Vinod Koul]

Add dmas property for spi@880000 and pinconf setting so that we can use
dma for this spi device. Also, add iommu properties for qup and spi.

Signed-off-by: Vinod Koul <vkoul@kernel.org>
---
 arch/arm64/boot/dts/qcom/sdm845-db845c.dts |  4 ++++
 arch/arm64/boot/dts/qcom/sdm845.dtsi       | 11 +++++++++++
 2 files changed, 15 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
index 7cc236575ee2..0653468f26ce 100644
--- a/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
+++ b/arch/arm64/boot/dts/qcom/sdm845-db845c.dts
@@ -418,6 +418,10 @@ &gcc {
 			   <GCC_QSPI_CNOC_PERIPH_AHB_CLK>;
 };
 
+&gpi_dma0 {
+	status = "okay";
+};
+
 &gpu {
 	zap-shader {
 		memory-region = <&gpu_mem>;
diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
index c9a127bbd606..bd9952f54721 100644
--- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
+++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
@@ -12,6 +12,7 @@
 #include <dt-bindings/clock/qcom,lpass-sdm845.h>
 #include <dt-bindings/clock/qcom,rpmh.h>
 #include <dt-bindings/clock/qcom,videocc-sdm845.h>
+#include <dt-bindings/dma/qcom-gpi.h>
 #include <dt-bindings/interconnect/qcom,osm-l3.h>
 #include <dt-bindings/interconnect/qcom,sdm845.h>
 #include <dt-bindings/interrupt-controller/arm-gic.h>
@@ -1183,6 +1184,9 @@ spi0: spi@880000 {
 				interconnects = <&aggre1_noc MASTER_QUP_1 0 &config_noc SLAVE_BLSP_1 0>,
 						<&gladiator_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_BLSP_1 0>;
 				interconnect-names = "qup-core", "qup-config";
+				dmas = <&gpi_dma0 0 0 QCOM_GPI_SPI>,
+				       <&gpi_dma0 1 0 QCOM_GPI_SPI>;
+				dma-names = "tx", "rx";
 				status = "disabled";
 			};
 
@@ -2622,6 +2626,13 @@ pinmux {
 					       "gpio2", "gpio3";
 					function = "qup0";
 				};
+
+				config {
+					pins = "gpio0", "gpio1",
+					       "gpio2", "gpio3";
+					drive-strength = <6>;
+					bias-disable;
+				};
 			};
 
 			qup_spi1_default: qup-spi1-default {
-- 
2.26.2

Re: [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Konrad Dybcio]

Hi,

looks like sdm845-cheza also uses the spi0 bus, which as far as I understand is going to break with the GPI DMA disabled. Perhaps it should also be enabled over there?

Actually, is there a point in disabling DMA for BLSPs/QUPs in the SoC DTSI? I don't think any platform/vendor firmware disables entire hosts..


Konrad

Re: [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Vinod Koul]

Hi Konrad,

On 11-01-21, 17:04, Konrad Dybcio wrote:
> Hi,
> 
> looks like sdm845-cheza also uses the spi0 bus, which as far as I
> understand is going to break with the GPI DMA disabled. Perhaps it
> should also be enabled over there?

If it is working without GPI enabled, it would work.. GPI for QUP is
something that requires firmware and would have to be enabled by
firmware

> Actually, is there a point in disabling DMA for BLSPs/QUPs in the SoC
> DTSI? I don't think any platform/vendor firmware disables entire
> hosts..

Since DMA support may not be available on certain targets (firmware
support), so enabling per board would make sense

Thanks
-- 
~Vinod

Re: [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Konrad Dybcio]

> If it is working without GPI enabled, it would work.. GPI for QUP is
> something that requires firmware and would have to be enabled by
> firmware


I think with the new code of yours:


mas->tx = dma_request_slave_channel(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 out_pm;


it *may* fail to probe with the channels assigned in the "dmas=" property but with the engine having "status=disabled", but as I don't have any hardware to test that driver on, please confirm whether my concerns are right..


> Since DMA support may not be available on certain targets (firmware
> support), so enabling per board would make sense

Oh really, are there SDM845 boards shipping with GPI DMA *disabled to all peripherals*?


Konrad

Re: [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Vinod Koul]

On 11-01-21, 21:45, Konrad Dybcio wrote:
> 
> > If it is working without GPI enabled, it would work.. GPI for QUP is
> > something that requires firmware and would have to be enabled by
> > firmware
> 
> 
> I think with the new code of yours:
> 
> 
> mas->tx = dma_request_slave_channel(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 out_pm;

This code would be exercised *only* when DMA mode is enabled in
firmware.. Both SPI and I2C driver would probe the mode supported
(reason for exposing stuff in first two patches of this series)..

Only if the device reports we should enable it.. Also if the device is
working with FIFO mode then DMA mode wont work..

> 
> it *may* fail to probe with the channels assigned in the "dmas="
> property but with the engine having "status=disabled", but as I don't
> have any hardware to test that driver on, please confirm whether my
> concerns are right..
> 
> 
> > Since DMA support may not be available on certain targets (firmware
> > support), so enabling per board would make sense
> 
> Oh really, are there SDM845 boards shipping with GPI DMA *disabled to
> all peripherals*?

Is the peripheral working with FIFO mode, then answer is yes :)

Thanks
-- 
~Vinod

Re: [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Doug Anderson]

Hi,

On Mon, Jan 11, 2021 at 7:18 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> @@ -2622,6 +2626,13 @@ pinmux {
>                                                "gpio2", "gpio3";
>                                         function = "qup0";
>                                 };
> +
> +                               config {

Convention in Qualcomm device tree files seems to be that the node is
"pinconf", not "config".


> +                                       pins = "gpio0", "gpio1",
> +                                              "gpio2", "gpio3";
> +                                       drive-strength = <6>;
> +                                       bias-disable;
> +                               };

Pin config almost never belongs in the SoC dtsi file.  This should be
in the board .dts file.  What if pulls are needed on some pins?  What
if you need a stronger or weaker drive strength?

-Doug

Re: [PATCH 7/7] arm64: dts: qcom: sdm845: enable dma for spi

[Vinod Koul]

On 11-01-21, 08:47, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 11, 2021 at 7:18 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > @@ -2622,6 +2626,13 @@ pinmux {
> >                                                "gpio2", "gpio3";
> >                                         function = "qup0";
> >                                 };
> > +
> > +                               config {
> 
> Convention in Qualcomm device tree files seems to be that the node is
> "pinconf", not "config".

Yes missed that, thanks for pointing
> 
> 
> > +                                       pins = "gpio0", "gpio1",
> > +                                              "gpio2", "gpio3";
> > +                                       drive-strength = <6>;
> > +                                       bias-disable;
> > +                               };
> 
> Pin config almost never belongs in the SoC dtsi file.  This should be
> in the board .dts file.  What if pulls are needed on some pins?  What
> if you need a stronger or weaker drive strength?

Right I will move it to dtsi

-- 
~Vinod

Re: [PATCH 0/7] Add and enable GPI DMA users

[Doug Anderson]

Hi,

On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote:
>
> Hello,
>
> This series add the GPI DMA in qcom geni spi and i2c drivers. For this we
> first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
> headers and then add support for gpi dma in geni driver.
>
> Then we add spi and i2c geni driver changes to support this DMA.
>
> Lastly, add the GPI dma nodes and enable dma for spi found in Rb3 board.
>
> To merge this, we could merge all thru qcom tree with ack on spi/i2c.

It'd be super great if somewhere (ideally in the commit message and
maybe somewhere in the code) you could talk more about the different
modes.  Maybe something like this (if it's correct):

GPI Mode (confusingly, also known as "GSI" mode in some places): In
this mode something else running on the SoC is sharing access to the
geni instance.  This mode allows sharing the device between the Linux
kernel and other users including handling the fact that other users
might be running the geni port at a different clock rate.  GPI mode
limits what you can do with a port.  For instance, direct control of
chip select is not allowed.  NOTE: if firmware has configured a geni
instance for GPI then FIFO and SE_DMA usage is not allowed.
Conversely, if firmware has not configured a geni instance for GPI
then only FIFO and SE_DMA usage is allowed.

SE DMA Mode: Data transfers happen over DMA.

SE FIFO Mode: Data is manually transferred into the FIFO by the CPU.

Re: [PATCH 0/7] Add and enable GPI DMA users

[Vinod Koul]

Hello Doug,

On 12-01-21, 16:01, Doug Anderson wrote:
> Hi,
> 
> On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote:
> >
> > Hello,
> >
> > This series add the GPI DMA in qcom geni spi and i2c drivers. For this we
> > first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
> > headers and then add support for gpi dma in geni driver.
> >
> > Then we add spi and i2c geni driver changes to support this DMA.
> >
> > Lastly, add the GPI dma nodes and enable dma for spi found in Rb3 board.
> >
> > To merge this, we could merge all thru qcom tree with ack on spi/i2c.
> 
> It'd be super great if somewhere (ideally in the commit message and
> maybe somewhere in the code) you could talk more about the different
> modes.  Maybe something like this (if it's correct):
> 
> GPI Mode (confusingly, also known as "GSI" mode in some places): In
> this mode something else running on the SoC is sharing access to the
> geni instance.  This mode allows sharing the device between the Linux
> kernel and other users including handling the fact that other users
> might be running the geni port at a different clock rate.  GPI mode
> limits what you can do with a port.  For instance, direct control of
> chip select is not allowed.  NOTE: if firmware has configured a geni
> instance for GPI then FIFO and SE_DMA usage is not allowed.
> Conversely, if firmware has not configured a geni instance for GPI
> then only FIFO and SE_DMA usage is allowed.
> 
> SE DMA Mode: Data transfers happen over DMA.
> 
> SE FIFO Mode: Data is manually transferred into the FIFO by the CPU.

I think it is a good feedback, there is indeed bunch of confusion wrt
QUP DMA and i think we should add above to qcom geni driver and not just
in cover letter. FWIW for all practical purposes GSI and GPI can be used
interchangeably. There are some nuisances involved like firmware and a
microcontroller but for the sake of simplicity we can skip that :)

Thanks
-- 
~Vinod