| Message ID | 20210111151651.1616813-4-vkoul@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add and enable GPI DMA users | expand |
On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > 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 > + > + 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 >
On 11-01-21, 09:40, Bjorn Andersson wrote: > On Mon 11 Jan 09:16 CST 2021, Vinod Koul wrote: > > > 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. Sure.. > > +{ > > + 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. Certainly.. > None of them needs to be initialized, first access in all cases are > assignments. Will update > > + > > + 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 > > ... okay will update > > + > > + 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. Yeah missed > > + 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) Yeah we invoke this to select the mode before programming DMA, so yes a wmb() would make sense. Thanks for quick look
Hi, On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote: > > +static int geni_se_select_gpi_mode(struct geni_se *se) > +{ > + 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; > + > + 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); Do you really need to do a read/modify/write of SE_GENI_DMA_MODE_EN? It's a register with a single bit in it. Just set the bit, no? There are cases where read-modify-write is the correct thing to do but IMO it shouldn't be the default way of working. If code is initting a register to a default state it should just be setting the register. If some later incarnation of the hardware comes along and adds extra bits to this register then, sure, we might have to modify the code. However, it has the advantage where we aren't left at the whims of whatever was programmed by whatever version of whatever firmware was running on the device. I've been bitten way more often by firmware leaving registers in some random / unexpected state than by new bits introduced by new versions of hardware. ...same for other registers in your patch that you can just init. (this is true throughout lots of Qualcomm code, but I figure might as well start trying to do something about it?) > + 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); > + > + 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); Last time I touched this bit of code I think folks agreed that it would be better to move managing of the interrupt enables out of the common code and move them to the various drivers using geni [1]. I was hoping that someone from Qualcomm would be able to pick this up. Managing them in the wrapper just ends up causing a whole bunch of special cases. Any chance you could take that on as part of your series? Presumably if this was mananged in individual drivers you also might be able to do less read-modify-write type stuff, too... [1] https://lore.kernel.org/linux-arm-msm/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/ > + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR); > + 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); This looks mostly like geni_se_irq_clear(). Should they somehow share code? > 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, Add a comment like "Also known as GSI" here to help people figure out what's going on when they're trying to parse the manual or #defines like "SE_GSI_EVENT_EN" > @@ -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)) Maybe this define belonged in patch #1? It doesn't seem related to this patch?
On 12-01-21, 16:01, Doug Anderson wrote: > Hi, > > On Mon, Jan 11, 2021 at 7:17 AM Vinod Koul <vkoul@kernel.org> wrote: > > > > +static int geni_se_select_gpi_mode(struct geni_se *se) > > +{ > > + 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; > > + > > + 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); > > Do you really need to do a read/modify/write of SE_GENI_DMA_MODE_EN? > It's a register with a single bit in it. Just set the bit, no? There > are cases where read-modify-write is the correct thing to do but IMO > it shouldn't be the default way of working. If code is initting a > register to a default state it should just be setting the register. > If some later incarnation of the hardware comes along and adds extra > bits to this register then, sure, we might have to modify the code. > However, it has the advantage where we aren't left at the whims of > whatever was programmed by whatever version of whatever firmware was > running on the device. I've been bitten way more often by firmware > leaving registers in some random / unexpected state than by new bits > introduced by new versions of hardware. > > ...same for other registers in your patch that you can just init. Yes sounds right to me. I will review this and other register writes and update the code > (this is true throughout lots of Qualcomm code, but I figure might as > well start trying to do something about it?) yep, we can always start somewhere :) > > + 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); > > + > > + 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); > > Last time I touched this bit of code I think folks agreed that it > would be better to move managing of the interrupt enables out of the > common code and move them to the various drivers using geni [1]. I > was hoping that someone from Qualcomm would be able to pick this up. > Managing them in the wrapper just ends up causing a whole bunch of > special cases. Any chance you could take that on as part of your > series? > > Presumably if this was mananged in individual drivers you also might > be able to do less read-modify-write type stuff, too... > > [1] https://lore.kernel.org/linux-arm-msm/CAD=FV=VWPqswOXJejyXjYT_Yspdu75ELq42cffN87FrpTwPUQg@mail.gmail.com/ If it is okay, I can take this up but after this series. We can handle the move independently as that is certainly a good thing to do. > > + writel_relaxed(0xFFFFFFFF, se->base + SE_GENI_M_IRQ_CLEAR); > > + 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); > > This looks mostly like geni_se_irq_clear(). Should they somehow share code? Mostly seems similar, let me move over that and verify > > 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, > > Add a comment like "Also known as GSI" here to help people figure out > what's going on when they're trying to parse the manual or #defines > like "SE_GSI_EVENT_EN" Sure will add GSI. > > @@ -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)) > > Maybe this define belonged in patch #1? It doesn't seem related to this patch? Yes the bit defines should go with the register.
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) +{ + 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; + + 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); + + 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); + 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); + + 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
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(-)