diff mbox series

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

Message ID 20210618141839.3777270-4-vkoul@kernel.org
State New
Headers show
Series soc: qcom: geni: add support for gpi dma | expand

Commit Message

Vinod Koul June 18, 2021, 2:18 p.m. UTC 
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(-)

-- 
2.31.1

Comments

Bjorn Andersson June 18, 2021, 5:23 p.m. UTC | #1 
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

>
Vinod Koul June 22, 2021, 12:15 p.m. UTC | #2 
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
diff mbox series

Patch

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 */