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@xxxxxxxxxx>
> > ---
> >  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