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

[Jon Hunter <jonathanh@nvidia.com>]

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@nvidia.com>
>> Signed-off-by: Rajesh Gumasta <rgumasta@nvidia.com>
>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>> ---
>>   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