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