linux-kernel - Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210111172200.GZ2771@vkoul-mobl>
Date:   Mon, 11 Jan 2021 22:52:00 +0530
From:   Vinod Koul <vkoul@...nel.org>
To:     Bjorn Andersson <bjorn.andersson@...aro.org>
Cc:     Mark Brown <broonie@...nel.org>, Wolfram Sang <wsa@...nel.org>,
        linux-arm-msm@...r.kernel.org, Andy Gross <agross@...nel.org>,
        Matthias Kaehlcke <mka@...omium.org>,
        Douglas Anderson <dianders@...omium.org>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Amit Pundir <amit.pundir@...aro.org>,
        linux-spi@...r.kernel.org, linux-i2c@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/7] soc: qcom: geni: Add support for gpi dma

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

-- 
~Vinod