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@xxxxxxxxxx>
> > ---
> > 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
--
~Vinod
