[PATCH v2 0/3] soc: qcom: geni: add support for gpi dma

[PATCH v2 0/3] soc: qcom: geni: add support for gpi dma

[Vinod Koul]

This is version 2 of the GPI dma series [1]

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

The SPI and I2C driver changes shall follow shortly

[1]: http://lore.kernel.org/r/20210111151651.1616813-1-vkoul@kernel.org

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

Vinod Koul (3):
  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

 drivers/soc/qcom/qcom-geni-se.c | 47 ++++++++++++++++++++++-----------
 include/linux/qcom-geni-se.h    | 23 ++++++++++++++--
 2 files changed, 52 insertions(+), 18 deletions(-)

-- 
2.31.1

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

Also, add FIFO_IF_DISABLE define.

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

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

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

[Bjorn Andersson]

On Fri 18 Jun 09:18 CDT 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
> 
> Also, add FIFO_IF_DISABLE define.
> 

Afaict these registers relates to the hardware block that is primarily
owned by the individual engine-drivers, would it not make sense to move
them all to the shared header file?

(The patch itself still looks ok, but needs the consuming patch to be
merged)

Regards,
Bjorn

> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 1 -
>  include/linux/qcom-geni-se.h    | 4 ++++
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 5bdfb1565c14..fe666ea0c487 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -104,7 +104,6 @@ static const char * const icc_path_names[] = {"qup-core", "qup-config",
>  #define GENI_OUTPUT_CTRL		0x24
>  #define GENI_CGC_CTRL			0x28
>  #define GENI_CLK_CTRL_RO		0x60
> -#define GENI_IF_DISABLE_RO		0x64
>  #define GENI_FW_S_REVISION_RO		0x6c
>  #define SE_GENI_BYTE_GRAN		0x254
>  #define SE_GENI_TX_PACKING_CFG0		0x260
> diff --git a/include/linux/qcom-geni-se.h b/include/linux/qcom-geni-se.h
> index 7c811eebcaab..5114e2144b17 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -63,6 +63,7 @@ struct geni_se {
>  #define SE_GENI_STATUS			0x40
>  #define GENI_SER_M_CLK_CFG		0x48
>  #define GENI_SER_S_CLK_CFG		0x4c
> +#define GENI_IF_DISABLE_RO		0x64
>  #define GENI_FW_REVISION_RO		0x68
>  #define SE_GENI_CLK_SEL			0x7c
>  #define SE_GENI_DMA_MODE_EN		0x258
> @@ -105,6 +106,9 @@ struct geni_se {
>  #define CLK_DIV_MSK			GENMASK(15, 4)
>  #define CLK_DIV_SHFT			4
>  
> +/* GENI_IF_DISABLE_RO fields */
> +#define FIFO_IF_DISABLE			(BIT(0))
> +
>  /* GENI_FW_REVISION_RO fields */
>  #define FW_REV_PROTOCOL_MSK		GENMASK(15, 8)
>  #define FW_REV_PROTOCOL_SHFT		8
> -- 
> 2.31.1
> 

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

[Vinod Koul]

On 18-06-21, 12:03, Bjorn Andersson wrote:
> On Fri 18 Jun 09:18 CDT 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
> > 
> > Also, add FIFO_IF_DISABLE define.
> > 
> 
> Afaict these registers relates to the hardware block that is primarily
> owned by the individual engine-drivers, would it not make sense to move
> them all to the shared header file?

the GENI_IF_DISABLE_RO is used by SPI and I2C drivers, so we would create two
copies. So better to be defined in geni header
-- 
~Vinod

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

[Vinod Koul]

SPI & I2C geni driver needs to access struct geni_wrapper, so move it to
header. The drivers needs this header to find the geni device and use it
in dma mapping.

Using this method works for both DT and ACPI systems

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

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index fe666ea0c487..08d645b90ed3 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -78,20 +78,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];
-};
 
 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 5114e2144b17..5fda675c5cfe 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -38,6 +38,20 @@ 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_se - GENI Serial Engine
  * @base:		Base Address of the Serial Engine's register block
-- 
2.31.1

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

[Bjorn Andersson]

On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:

> SPI & I2C geni driver needs to access struct geni_wrapper, so move it to
> header. The drivers needs this header to find the geni device and use it
> in dma mapping.
> 

How does this differ from engine->dev->parent?

> Using this method works for both DT and ACPI systems
> 

I was under the impression that the wrapper and engines are describe
completely independently in ACPI, so we don't have a link between them.

If that's not the case, then I guess that answers the above question
about ->parent.

Regards,
Bjorn

> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 14 --------------
>  include/linux/qcom-geni-se.h    | 14 ++++++++++++++
>  2 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index fe666ea0c487..08d645b90ed3 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -78,20 +78,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];
> -};
>  
>  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 5114e2144b17..5fda675c5cfe 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -38,6 +38,20 @@ 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_se - GENI Serial Engine
>   * @base:		Base Address of the Serial Engine's register block
> -- 
> 2.31.1
> 

[PATCH v2 3/3] 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 | 32 +++++++++++++++++++++++++++++++-
 include/linux/qcom-geni-se.h    |  5 +++--
 2 files changed, 34 insertions(+), 3 deletions(-)

diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
index 08d645b90ed3..40a0a1f88070 100644
--- a/drivers/soc/qcom/qcom-geni-se.c
+++ b/drivers/soc/qcom/qcom-geni-se.c
@@ -307,6 +307,32 @@ static void geni_se_select_dma_mode(struct geni_se *se)
 		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
 }
 
+static void geni_se_select_gpi_mode(struct geni_se *se)
+{
+	unsigned int gpi_event_en;
+	unsigned int m_irq_en;
+	unsigned int s_irq_en;
+
+	geni_se_irq_clear(se);
+	writel(0, se->base + SE_IRQ_EN);
+
+	s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
+	s_irq_en &= ~S_CMD_DONE_EN;
+	writel(s_irq_en, se->base + SE_GENI_S_IRQ_EN);
+
+	m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
+	m_irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
+		      M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
+	writel(m_irq_en, se->base + SE_GENI_M_IRQ_EN);
+
+	writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
+
+	gpi_event_en = readl(se->base + SE_GSI_EVENT_EN);
+	gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
+			 GENI_M_EVENT_EN | GENI_S_EVENT_EN);
+	writel(gpi_event_en, se->base + SE_GSI_EVENT_EN);
+}
+
 /**
  * geni_se_select_mode() - Select the serial engine transfer mode
  * @se:		Pointer to the concerned serial engine.
@@ -314,7 +340,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:
@@ -323,6 +350,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 5fda675c5cfe..336b682392b1 100644
--- a/include/linux/qcom-geni-se.h
+++ b/include/linux/qcom-geni-se.h
@@ -11,8 +11,9 @@
 /* Transfer mode supported by GENI Serial Engines */
 enum geni_se_xfer_mode {
 	GENI_SE_INVALID,
-	GENI_SE_FIFO,
-	GENI_SE_DMA,
+	GENI_SE_FIFO, /* FIFO mode */
+	GENI_GPI_DMA, /* GSI aka GPI DMA mode */
+	GENI_SE_DMA,  /* SE DMA mode */
 };
 
 /* Protocols supported by GENI Serial Engines */
-- 
2.31.1

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

[Bjorn Andersson]

On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:

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

I think you're missing an opportunity to describe what GPI DMA is.
Perhaps something like:

In GPI DMA mode the serial engine uses the DMA controller found in the
associated wrapper to perform its transaction, add support for enabling
this mode in the engine.

> 
> Signed-off-by: Vinod Koul <vkoul@kernel.org>
> ---
>  drivers/soc/qcom/qcom-geni-se.c | 32 +++++++++++++++++++++++++++++++-
>  include/linux/qcom-geni-se.h    |  5 +++--
>  2 files changed, 34 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> index 08d645b90ed3..40a0a1f88070 100644
> --- a/drivers/soc/qcom/qcom-geni-se.c
> +++ b/drivers/soc/qcom/qcom-geni-se.c
> @@ -307,6 +307,32 @@ static void geni_se_select_dma_mode(struct geni_se *se)
>  		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
>  }
>  
> +static void geni_se_select_gpi_mode(struct geni_se *se)
> +{
> +	unsigned int gpi_event_en;
> +	unsigned int m_irq_en;
> +	unsigned int s_irq_en;

readl and writel operates on u32, so better to use that.

> +
> +	geni_se_irq_clear(se);
> +	writel(0, se->base + SE_IRQ_EN);
> +
> +	s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);

I don't mind this being _relaxed if done on purpose, but as it's the
only one (and there's no comment) I get the feeling that it's a mistake.

> +	s_irq_en &= ~S_CMD_DONE_EN;
> +	writel(s_irq_en, se->base + SE_GENI_S_IRQ_EN);

The use of 3 different variables (gpi_event_en, m_irq_en, s_irq_en)
forced me to really look if the three sets of operations somehow reused
previous results.

To clarify that this isn't the case I would suggest that you use a
single variable ("val"?) instead.

> +
> +	m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
> +	m_irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> +		      M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> +	writel(m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> +
> +	writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
> +
> +	gpi_event_en = readl(se->base + SE_GSI_EVENT_EN);
> +	gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> +			 GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> +	writel(gpi_event_en, se->base + SE_GSI_EVENT_EN);
> +}
> +
>  /**
>   * geni_se_select_mode() - Select the serial engine transfer mode
>   * @se:		Pointer to the concerned serial engine.
> @@ -314,7 +340,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);

This line can be left unbroken.

>  
>  	switch (mode) {
>  	case GENI_SE_FIFO:
> @@ -323,6 +350,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 5fda675c5cfe..336b682392b1 100644
> --- a/include/linux/qcom-geni-se.h
> +++ b/include/linux/qcom-geni-se.h
> @@ -11,8 +11,9 @@
>  /* Transfer mode supported by GENI Serial Engines */
>  enum geni_se_xfer_mode {
>  	GENI_SE_INVALID,
> -	GENI_SE_FIFO,
> -	GENI_SE_DMA,

Is the order significant? Is there a reason why SE_DMA ended up last?

> +	GENI_SE_FIFO, /* FIFO mode */
> +	GENI_GPI_DMA, /* GSI aka GPI DMA mode */

If the TLA soup is too soupy, then perhaps we should just document it as
"external DMA" or "Use wrapper's DMA controller"?

And perhaps use kernel-doc instead?

Regards,
Bjorn

> +	GENI_SE_DMA,  /* SE DMA mode */
>  };
>  
>  /* Protocols supported by GENI Serial Engines */
> -- 
> 2.31.1
> 

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

[Vinod Koul]

On 18-06-21, 12:23, Bjorn Andersson wrote:
> On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
> 
> > GPI DMA is one of the DMA modes supported on geni, this adds support to
> > enable that mode
> 
> I think you're missing an opportunity to describe what GPI DMA is.
> Perhaps something like:
> 
> In GPI DMA mode the serial engine uses the DMA controller found in the
> associated wrapper to perform its transaction, add support for enabling
> this mode in the engine.

Hmm looks like I did, I will update more kernel-doc style comments to
explain more details

> 
> > 
> > Signed-off-by: Vinod Koul <vkoul@kernel.org>
> > ---
> >  drivers/soc/qcom/qcom-geni-se.c | 32 +++++++++++++++++++++++++++++++-
> >  include/linux/qcom-geni-se.h    |  5 +++--
> >  2 files changed, 34 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c
> > index 08d645b90ed3..40a0a1f88070 100644
> > --- a/drivers/soc/qcom/qcom-geni-se.c
> > +++ b/drivers/soc/qcom/qcom-geni-se.c
> > @@ -307,6 +307,32 @@ static void geni_se_select_dma_mode(struct geni_se *se)
> >  		writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN);
> >  }
> >  
> > +static void geni_se_select_gpi_mode(struct geni_se *se)
> > +{
> > +	unsigned int gpi_event_en;
> > +	unsigned int m_irq_en;
> > +	unsigned int s_irq_en;
> 
> readl and writel operates on u32, so better to use that.

Yes

> 
> > +
> > +	geni_se_irq_clear(se);
> > +	writel(0, se->base + SE_IRQ_EN);
> > +
> > +	s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN);
> 
> I don't mind this being _relaxed if done on purpose, but as it's the
> only one (and there's no comment) I get the feeling that it's a mistake.

Yes it is a leftover, fixed now

> 
> > +	s_irq_en &= ~S_CMD_DONE_EN;
> > +	writel(s_irq_en, se->base + SE_GENI_S_IRQ_EN);
> 
> The use of 3 different variables (gpi_event_en, m_irq_en, s_irq_en)
> forced me to really look if the three sets of operations somehow reused
> previous results.
> 
> To clarify that this isn't the case I would suggest that you use a
> single variable ("val"?) instead.

Yes updated

> 
> > +
> > +	m_irq_en = readl(se->base + SE_GENI_M_IRQ_EN);
> > +	m_irq_en &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN |
> > +		      M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN);
> > +	writel(m_irq_en, se->base + SE_GENI_M_IRQ_EN);
> > +
> > +	writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN);
> > +
> > +	gpi_event_en = readl(se->base + SE_GSI_EVENT_EN);
> > +	gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN |
> > +			 GENI_M_EVENT_EN | GENI_S_EVENT_EN);
> > +	writel(gpi_event_en, se->base + SE_GSI_EVENT_EN);
> > +}
> > +
> >  /**
> >   * geni_se_select_mode() - Select the serial engine transfer mode
> >   * @se:		Pointer to the concerned serial engine.
> > @@ -314,7 +340,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);
> 
> This line can be left unbroken.

yaay 100 chars :-) will do

> 
> >  
> >  	switch (mode) {
> >  	case GENI_SE_FIFO:
> > @@ -323,6 +350,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 5fda675c5cfe..336b682392b1 100644
> > --- a/include/linux/qcom-geni-se.h
> > +++ b/include/linux/qcom-geni-se.h
> > @@ -11,8 +11,9 @@
> >  /* Transfer mode supported by GENI Serial Engines */
> >  enum geni_se_xfer_mode {
> >  	GENI_SE_INVALID,
> > -	GENI_SE_FIFO,
> > -	GENI_SE_DMA,
> 
> Is the order significant? Is there a reason why SE_DMA ended up last?

Nope not that I can tjing off.. I can add this as last!

-- 
~Vinod

Re: [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma

[Bjorn Andersson]

On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:

> This is version 2 of the GPI dma series [1]
> 
> This series add the GPI DMA in qcom geni drivers. For this we
> first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
> headers and then add support in the gpi dma in geni driver.
> 
> The SPI and I2C driver changes shall follow shortly
> 

Given that the continuation of this series will have build time
dependencies on these patches I think it would be good to see them all
as one chunk (and practically I can create a immutable branch of the
soc/qcom pieces and share with Wolfram and Mark).

Regards,
Bjorn

> [1]: http://lore.kernel.org/r/20210111151651.1616813-1-vkoul@kernel.org
> 
> Changes in v2:
>  - add r-b from Bjorn on patch 1
>  - add more details in log for patch 2
>  - Fix the comments from Doug and Bjorn for patch3, notable use read, modify
>    update for irq registers, use geni_se_irq_clear() for irq, dont update
>    single bit registers, add a bit more description for gpi dma etc
> 
> Vinod Koul (3):
>   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
> 
>  drivers/soc/qcom/qcom-geni-se.c | 47 ++++++++++++++++++++++-----------
>  include/linux/qcom-geni-se.h    | 23 ++++++++++++++--
>  2 files changed, 52 insertions(+), 18 deletions(-)
> 
> -- 
> 2.31.1
> 

Re: [PATCH v2 0/3] soc: qcom: geni: add support for gpi dma

[Vinod Koul]

On 18-06-21, 11:59, Bjorn Andersson wrote:
> On Fri 18 Jun 09:18 CDT 2021, Vinod Koul wrote:
> 
> > This is version 2 of the GPI dma series [1]
> > 
> > This series add the GPI DMA in qcom geni drivers. For this we
> > first need to move GENI_IF_DISABLE_RO and struct geni_wrapper to common
> > headers and then add support in the gpi dma in geni driver.
> > 
> > The SPI and I2C driver changes shall follow shortly
> > 
> 
> Given that the continuation of this series will have build time
> dependencies on these patches I think it would be good to see them all
> as one chunk (and practically I can create a immutable branch of the
> soc/qcom pieces and share with Wolfram and Mark).

Okay, let me post full series in few days

> 
> Regards,
> Bjorn
> 
> > [1]: http://lore.kernel.org/r/20210111151651.1616813-1-vkoul@kernel.org
> > 
> > Changes in v2:
> >  - add r-b from Bjorn on patch 1
> >  - add more details in log for patch 2
> >  - Fix the comments from Doug and Bjorn for patch3, notable use read, modify
> >    update for irq registers, use geni_se_irq_clear() for irq, dont update
> >    single bit registers, add a bit more description for gpi dma etc
> > 
> > Vinod Koul (3):
> >   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
> > 
> >  drivers/soc/qcom/qcom-geni-se.c | 47 ++++++++++++++++++++++-----------
> >  include/linux/qcom-geni-se.h    | 23 ++++++++++++++--
> >  2 files changed, 52 insertions(+), 18 deletions(-)
> > 
> > -- 
> > 2.31.1
> > 

-- 
~Vinod