| Message ID | 20210625052213.32260-3-vkoul@kernel.org |
|---|---|
| State | Accepted |
| Commit | 0fa8266294754978da34d7ea785d621f51d939f2 |
| Headers | show |
| Series | Add and enable GPI DMA users | expand |
Hi, On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote: > > +static void geni_se_select_gpi_mode(struct geni_se *se) > +{ > + u32 val; > + > + geni_se_irq_clear(se); > + > + writel(0, se->base + SE_IRQ_EN); > + > + val = readl(se->base + SE_GENI_S_IRQ_EN); > + val &= ~S_CMD_DONE_EN; > + writel(val, se->base + SE_GENI_S_IRQ_EN); > + > + val = readl(se->base + SE_GENI_M_IRQ_EN); > + val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | > + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > + writel(val, se->base + SE_GENI_M_IRQ_EN); > + > + writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN); > + > + val = readl(se->base + SE_GSI_EVENT_EN); > + val |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | GENI_M_EVENT_EN | GENI_S_EVENT_EN); nit: the above has some extra parenthesis that aren't needed. I will continue to assert that all of the "set mode" stuff doesn't really belong here and should be managed by individual drivers [1]. I'll accept that it doesn't have to block forward progress, though I'm at least a bit disappointed that we asked Qualcomm to do this over 8 months ago and no action was taken. :( In any case, this looks OK to me: Reviewed-by: Douglas Anderson <dianders@chromium.org> [1] https://lore.kernel.org/r/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/
Hi Doug, On 28-06-21, 16:38, Doug Anderson wrote: > Hi, > > On Thu, Jun 24, 2021 at 10:22 PM Vinod Koul <vkoul@kernel.org> wrote: > > > > +static void geni_se_select_gpi_mode(struct geni_se *se) > > +{ > > + u32 val; > > + > > + geni_se_irq_clear(se); > > + > > + writel(0, se->base + SE_IRQ_EN); > > + > > + val = readl(se->base + SE_GENI_S_IRQ_EN); > > + val &= ~S_CMD_DONE_EN; > > + writel(val, se->base + SE_GENI_S_IRQ_EN); > > + > > + val = readl(se->base + SE_GENI_M_IRQ_EN); > > + val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | > > + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); > > + writel(val, se->base + SE_GENI_M_IRQ_EN); > > + > > + writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN); > > + > > + val = readl(se->base + SE_GSI_EVENT_EN); > > + val |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | GENI_M_EVENT_EN | GENI_S_EVENT_EN); > > nit: the above has some extra parenthesis that aren't needed. > > I will continue to assert that all of the "set mode" stuff doesn't > really belong here and should be managed by individual drivers [1]. > I'll accept that it doesn't have to block forward progress, though I'm > at least a bit disappointed that we asked Qualcomm to do this over 8 > months ago and no action was taken. :( > > In any case, this looks OK to me: > > Reviewed-by: Douglas Anderson <dianders@chromium.org> Thanks for the review. > > [1] https://lore.kernel.org/r/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/ I agree we should do something, will discuss with Bjorn and try to help here. Regards
diff --git a/drivers/soc/qcom/qcom-geni-se.c b/drivers/soc/qcom/qcom-geni-se.c index fe666ea0c487..7d649d2cf31e 100644 --- a/drivers/soc/qcom/qcom-geni-se.c +++ b/drivers/soc/qcom/qcom-geni-se.c @@ -321,6 +321,30 @@ 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) +{ + u32 val; + + geni_se_irq_clear(se); + + writel(0, se->base + SE_IRQ_EN); + + val = readl(se->base + SE_GENI_S_IRQ_EN); + val &= ~S_CMD_DONE_EN; + writel(val, se->base + SE_GENI_S_IRQ_EN); + + val = readl(se->base + SE_GENI_M_IRQ_EN); + val &= ~(M_CMD_DONE_EN | M_TX_FIFO_WATERMARK_EN | + M_RX_FIFO_WATERMARK_EN | M_RX_FIFO_LAST_EN); + writel(val, se->base + SE_GENI_M_IRQ_EN); + + writel(GENI_DMA_MODE_EN, se->base + SE_GENI_DMA_MODE_EN); + + val = readl(se->base + SE_GSI_EVENT_EN); + val |= (DMA_RX_EVENT_EN | DMA_TX_EVENT_EN | GENI_M_EVENT_EN | GENI_S_EVENT_EN); + writel(val, se->base + SE_GSI_EVENT_EN); +} + /** * geni_se_select_mode() - Select the serial engine transfer mode * @se: Pointer to the concerned serial engine. @@ -328,7 +352,7 @@ 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: @@ -337,6 +361,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 5114e2144b17..f5672785c0c4 100644 --- a/include/linux/qcom-geni-se.h +++ b/include/linux/qcom-geni-se.h @@ -8,11 +8,24 @@ #include <linux/interconnect.h> -/* Transfer mode supported by GENI Serial Engines */ +/** + * enum geni_se_xfer_mode: Transfer modes supported by Serial Engines + * + * @GENI_SE_INVALID: Invalid mode + * @GENI_SE_FIFO: FIFO mode. Data is transferred with SE FIFO + * by programmed IO method + * @GENI_SE_DMA: Serial Engine DMA mode. Data is transferred + * with SE by DMAengine internal to SE + * @GENI_GPI_DMA: GPI DMA mode. Data is transferred using a DMAengine + * configured by a firmware residing on a GSI engine. This DMA name is + * interchangeably used as GSI or GPI which seem to imply the same DMAengine + */ + enum geni_se_xfer_mode { GENI_SE_INVALID, GENI_SE_FIFO, GENI_SE_DMA, + GENI_GPI_DMA, }; /* Protocols supported by GENI Serial Engines */
GPI DMA is one of the DMA modes supported on geni, this adds support to enable that mode Also do better documentation of the enum geni_se_xfer_mode. Signed-off-by: Vinod Koul <vkoul@kernel.org> --- drivers/soc/qcom/qcom-geni-se.c | 29 ++++++++++++++++++++++++++++- include/linux/qcom-geni-se.h | 15 ++++++++++++++- 2 files changed, 42 insertions(+), 2 deletions(-)