| Message ID | 20210618141839.3777270-4-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: geni: add support for gpi dma | expand |
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 >
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!
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 */
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(-)