Re: [PATCH v5 3/4] dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00

[Green Wan]

On Wed, Nov 6, 2019 at 1:48 AM Vinod Koul <vkoul@xxxxxxxxxx> wrote:
>
> On 28-10-19, 15:56, Green Wan wrote:
> > Add PDMA driver, sf-pdma, to enable DMA engine on HiFive Unleashed
> > Rev A00 board.
> >
> >  - Implement dmaengine APIs, support MEM_TO_MEM async copy.
> >  - Tested by DMA Test client
> >  - Supports 4 channels DMA, each channel has 1 done and 1 err
> >    interrupt connected to platform-level interrupt controller (PLIC).
> >  - Depends on DMA_ENGINE and DMA_VIRTUAL_CHANNELS
> >
> > The datasheet is here:
> >
> >   https://static.dev.sifive.com/FU540-C000-v1.0.pdf
> >
> > Follow the DMAengine controller doc,
> > "./Documentation/driver-api/dmaengine/provider.rst" to implement DMA
> > engine. And use the dma test client in doc,
> > "./Documentation/driver-api/dmaengine/dmatest.rst", to test.
> >
> > Each DMA channel has separate HW regs and support done and error ISRs.
> > 4 channels share 1 done and 1 err ISRs. There's no expander/arbitrator
> > in DMA HW.
> >
> >    ------               ------
> >    |    |--< done 23 >--|ch 0|
> >    |    |--< err  24 >--|    |     (dma0chan0)
> >    |    |               ------
> >    |    |               ------
> >    |    |--< done 25 >--|ch 1|
> >    |    |--< err  26 >--|    |     (dma0chan1)
> >    |PLIC|               ------
> >    |    |               ------
> >    |    |--< done 27 >--|ch 2|
> >    |    |--< err  28 >--|    |     (dma0chan2)
> >    |    |               ------
> >    |    |               ------
> >    |    |--< done 29 >--|ch 3|
> >    |    |--< err  30 >--|    |     (dma0chan3)
> >    ------               ------
> >
> > Reviewed-by: Vinod Koul <vkoul@xxxxxxxxxx>
>
> when did i provide this?
>
> > Signed-off-by: Green Wan <green.wan@xxxxxxxxxx>
> > Reported-by: kbuild test robot <lkp@xxxxxxxxx>
> > Fixes: 31c3b98b5a01 ("dmaengine: sf-pdma: add platform DMA support for HiFive Unleashed A00")
>
> Fixes what... this is not a upstream commit?
>

Since I received a RFC patch from kbuild with the fix commit number I
thought it was about merged. RFC requested to add the "Reported-by"
and "Fixes" but looks miss the contributor so I added them as well.
I'll remove them in next submit. Sorry for causing confusion.

> > Signed-off-by: kbuild test robot <lkp@xxxxxxxxx>
> > ---
>
> Please list the changes done from prev version, here or in cover letter
>
will add change log.

> > +static struct sf_pdma_desc *sf_pdma_alloc_desc(struct sf_pdma_chan *chan)
> > +{
> > +     struct sf_pdma_desc *desc;
> > +     unsigned long flags;
> > +
> > +     spin_lock_irqsave(&chan->lock, flags);
> > +
> > +     if (chan->desc && !chan->desc->in_use) {
> > +             spin_unlock_irqrestore(&chan->lock, flags);
> > +             return chan->desc;
> > +     }
> > +
> > +     spin_unlock_irqrestore(&chan->lock, flags);
> > +
> > +     desc = kzalloc(sizeof(*desc), GFP_NOWAIT);
> > +
>
> this empty line in not required
>
> > +static struct dma_async_tx_descriptor *
> > +     sf_pdma_prep_dma_memcpy(struct dma_chan *dchan,
> > +                             dma_addr_t dest,
>
> please make it left justified

will fix.
>
> > +static int sf_pdma_slave_config(struct dma_chan *dchan,
> > +                             struct dma_slave_config *cfg)
> > +{
> > +     struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> > +
> > +     memcpy(&chan->cfg, cfg, sizeof(*cfg));
> > +     chan->dma_dir = DMA_MEM_TO_MEM;
>
> ?? looking at changelog we have only memcpy support, so this should not
> be here, pls remove this.
>
> > +static enum dma_status
> > +sf_pdma_tx_status(struct dma_chan *dchan,
> > +               dma_cookie_t cookie,
> > +               struct dma_tx_state *txstate)
> > +{
> > +     struct sf_pdma_chan *chan = to_sf_pdma_chan(dchan);
> > +     enum dma_status status;
> > +
> > +     status = dma_cookie_status(dchan, cookie, txstate);
> > +
> > +     if (txstate && status != DMA_ERROR)
> > +             dma_set_residue(txstate, sf_pdma_desc_residue(chan));
>
> which residue? the query can be for a cookie which is still in pending
> list! you need to check the cookie and only read register for cookie if
> submitted
>

WIll fix this.

> > +static int sf_pdma_remove(struct platform_device *pdev)
> > +{
> > +     struct sf_pdma *pdma = platform_get_drvdata(pdev);
> > +     struct sf_pdma_chan *ch;
> > +     int i;
> > +
> > +     for (i = 0; i < PDMA_NR_CH; i++) {
> > +             ch = &pdma->chans[i];
> > +
> > +             list_del(&ch->vchan.chan.device_node);
> > +             tasklet_kill(&ch->vchan.task);
> > +             tasklet_kill(&ch->done_tasklet);
> > +             tasklet_kill(&ch->err_tasklet);
>
> you have an isr registered which can fire and schedule tasklets..

will fix it by free irq first. Thanks for reviewing.

> --
> ~Vinod