Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma
From: Bjorn Andersson <hidden>
Date: 2021-01-11 15:41:57
Also in:
linux-arm-msm, linux-spi, lkml
On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote:
quoted hunk ↗ jump to hunk
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 | 39 ++++++++++++++++++++++++++++++++- include/linux/qcom-geni-se.h | 4 ++++ 2 files changed, 42 insertions(+), 1 deletion(-)diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c index a3868228ea05..db44dc32e049 100644 --- a/drivers/soc/qcom/qcom-geni-se.c +++ b/drivers/soc/qcom/qcom-geni-se.c@@ -310,6 +310,39 @@ static void geni_se_select_dma_mode(struct geni_se *se) writel_relaxed(val, se->base + SE_GENI_DMA_MODE_EN); } +static int geni_se_select_gpi_mode(struct geni_se *se)
This doesn't return any information and the return value isn't looked at, please make it void.
+{
+ unsigned int geni_dma_mode = 0;
+ unsigned int gpi_event_en = 0;
+ unsigned int common_geni_m_irq_en = 0;
+ unsigned int common_geni_s_irq_en = 0;These could certainly be given a shorter name. None of them needs to be initialized, first access in all cases are assignments.
+ + common_geni_m_irq_en = readl_relaxed(se->base + SE_GENI_M_IRQ_EN); + common_geni_s_irq_en = readl_relaxed(se->base + SE_GENI_S_IRQ_EN); + common_geni_m_irq_en &= + ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); + common_geni_s_irq_en &= ~S_CMD_DONE_EN; + geni_dma_mode = readl_relaxed(se->base + SE_GENI_DMA_MODE_EN); + gpi_event_en = readl_relaxed(se->base + SE_GSI_EVENT_EN); + + geni_dma_mode |= GENI_DMA_MODE_EN; + gpi_event_en |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | + GENI_M_EVENT_EN | GENI_S_EVENT_EN);
Please reorder these so that you do readl(m) mask out bits of m readl(s) mask out bits of s ...
+ + writel_relaxed(0, se->base + SE_IRQ_EN); + writel_relaxed(common_geni_s_irq_en, se->base + SE_GENI_S_IRQ_EN); + writel_relaxed(common_geni_m_irq_en, se->base + SE_GENI_M_IRQ_EN); + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR);
Lowercase hex digits please.
+ writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_S_IRQ_CLEAR); + writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_TX_IRQ_CLR); + writel_relaxed(0xFFFFFFFF, se->base + SE_DMA_RX_IRQ_CLR); + writel_relaxed(geni_dma_mode, se->base + SE_GENI_DMA_MODE_EN); + writel_relaxed(gpi_event_en, se->base + SE_GSI_EVENT_EN);
Why is this driver using _relaxed accessors exclusively? Why are you using _relaxed versions? And wouldn't it be suitable to have a wmb() before the "dma mode enable" and "event enable" at least? (I.e. use writel() instead) Regards, Bjorn
quoted hunk ↗ jump to hunk
+ + return 0; +} + /** * geni_se_select_mode() - Select the serial engine transfer mode * @se: Pointer to the concerned serial engine.@@ -317,7 +350,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:@@ -326,6 +360,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 cb4e40908f9f..12003a6cb133 100644 --- a/include/linux/qcom-geni-se.h +++ b/include/linux/qcom-geni-se.h@@ -12,6 +12,7 @@ enum geni_se_xfer_mode { GENI_SE_INVALID, GENI_SE_FIFO, + GENI_GPI_DMA, GENI_SE_DMA, };@@ -123,6 +124,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.26.2