Re: [PATCH v3 2/4] dmaengine: tegra: Add tegra gpcdma driver

[Jon Hunter]

On 01/09/2021 21:56, Jon Hunter wrote:
> On 27/08/2021 07:04, Akhil R wrote:
> > Adding GPC DMA controller driver for Tegra186 and Tegra194. The driver
> > supports dma transfers between memory to memory, IO peripheral to memory
> > and memory to IO peripheral.
> >
> > Signed-off-by: Pavan Kunapuli <pkunapuli@xxxxxxxxxx>
> > Signed-off-by: Rajesh Gumasta <rgumasta@xxxxxxxxxx>
> > Signed-off-by: Akhil R <akhilrajeev@xxxxxxxxxx>
> > ---
> >   drivers/dma/Kconfig         |   12 +
> >   drivers/dma/Makefile        |    1 +
> >   drivers/dma/tegra-gpc-dma.c | 1343 
> > +++++++++++++++++++++++++++++++++++++++++++
> >   3 files changed, 1356 insertions(+)
> >   create mode 100644 drivers/dma/tegra-gpc-dma.c

...

> > +static int tegra_dma_terminate_all(struct dma_chan *dc)
> > +{
> > +    struct tegra_dma_channel *tdc = to_tegra_dma_chan(dc);
> > +    unsigned long wcount = 0;
> > +    unsigned long status;
> > +    unsigned long flags;
> > +    bool was_busy;
> > +    int err;
> > +
> > +    raw_spin_lock_irqsave(&tdc->lock, flags);
> > +
> > +    if (!tdc->dma_desc) {
> > +        raw_spin_unlock_irqrestore(&tdc->lock, flags);
> > +        return 0;
> > +    }
> > +
> > +    if (!tdc->busy)
> > +        goto skip_dma_stop;
> > +
> > +    if (tdc->tdma->chip_data->hw_support_pause) {
> > +        err = tegra_dma_pause(tdc);
> > +        if (err) {
> > +            raw_spin_unlock_irqrestore(&tdc->lock, flags);
> > +            return err;
> > +        }
> > +    } else {
> > +        /* Before Reading DMA status to figure out number
> > +         * of bytes transferred by DMA channel:
> > +         * Change the client associated with the DMA channel
> > +         * to stop DMA engine from starting any more bursts for
> > +         * the given client and wait for in flight bursts to complete
> > +         */
> > +        tegra_dma_reset_client(tdc);
> > +
> > +        /* Wait for in flight data transfer to finish */
> > +        udelay(TEGRA_GPCDMA_BURST_COMPLETE_TIME);
> > +
> > +        /* If TX/RX path is still active wait till it becomes
> > +         * inactive
> > +         */
> > +
> > +        if (readl_relaxed_poll_timeout_atomic(tdc->tdma->base_addr +
> > +                tdc->chan_base_offset +
> > +                TEGRA_GPCDMA_CHAN_STATUS,
> > +                status,
> > +                !(status & (TEGRA_GPCDMA_STATUS_CHANNEL_TX |
> > +                TEGRA_GPCDMA_STATUS_CHANNEL_RX)),
> > +                5,
> > +                TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT)) {
> > +            dev_dbg(tdc2dev(tdc), "Timeout waiting for DMA burst 
> > completion!\n");
> > +            tegra_dma_dump_chan_regs(tdc);
> > +        }
>
> I would be tempted to make the code in the 'else' clause 
> tegra_dma_sw_pause(). Then you could have tegra_dma_hw_pause() and 
> tegra_dma_sw_pause().


Thinking some more tegra_dma_hw_pause() and tegra_dma_sw_pause() it not 
very clear/accurate. I would be tempted to call these tegra_dma_pause() 
and tegra_dma_stop_client() or tegra_dma_stop_transactions(), because 
without having a proper hardware pause, you are simply ignoring the 
client sync events.

Jon
--
nvpublic