Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer

From: Dmitry Osipenko <digetx@gmail.com>
To: "Jon Hunter" <jonathanh@nvidia.com>,
	"Laxman Dewangan" <ldewangan@nvidia.com>,
	"Vinod Koul" <vkoul@kernel.org>,
	"Dan Williams" <dan.j.williams@intel.com>,
	"Thierry Reding" <thierry.reding@gmail.com>,
	"Michał Mirosław" <mirq-linux@rere.qmqm.pl>
Cc: dmaengine@vger.kernel.org, linux-tegra@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v6 11/16] dmaengine: tegra-apb: Keep clock enabled only during of DMA transfer
Date: Sat, 1 Feb 2020 18:13:50 +0300	[thread overview]
Message-ID: <bf459b54-fa4c-b0ff-0af8-b7cb66b0a43c@gmail.com> (raw)
In-Reply-To: <1b64a3c6-a8b9-34d7-96cc-95b93ca1a392@gmail.com>

31.01.2020 17:22, Dmitry Osipenko пишет:
> 31.01.2020 12:02, Jon Hunter пишет:
>>
>> On 30/01/2020 20:04, Dmitry Osipenko wrote:
>>
>> ...
>>
>>>>> The tegra_dma_stop() should put RPM anyways, which is missed in yours
>>>>> sample. Please see handle_continuous_head_request().
>>>>
>>>> Yes and that is deliberate. The cyclic transfers the transfers *should*
>>>> not stop until terminate_all is called. The tegra_dma_stop in
>>>> handle_continuous_head_request() is an error condition and so I am not
>>>> sure it is actually necessary to call pm_runtime_put() here.
>>>
>>> But then tegra_dma_stop() shouldn't unset the "busy" mark.
>>
>> True.
>>
>>>>> I'm also finding the explicit get/put a bit easier to follow in the
>>>>> code, don't you think so?
>>>>
>>>> I can see that, but I was thinking that in the case of cyclic transfers,
>>>> it should only really be necessary to call the get/put at the beginning
>>>> and end. So in my mind there should only be two exit points which are
>>>> the ISR handler for SG and terminate_all for SG and cyclic.
>>>
>>> Alright, I'll update this patch.
>>
>> Hmmm ... I am wondering if we should not mess with that and leave how
>> you have it.
> 
> I took another look and seems my current v6 should be more correct because:
> 
> 1. If "busy" is unset in tegra_dma_stop(), then the RPM should be put
> there since tegra_dma_terminate_all() won't put RPM in this case:
> 
> 	if (!tdc->busy)
> 		goto skip_dma_stop;
> 
> 2. We can't move the "busy" unsetting into the terminate because then
> tegra_dma_stop() will be invoked twice. Although, one option could be to
> remove the tegra_dma_stop() from the error paths of
> handle_continuous_head_request(), but I'm not sure that this is correct
> to do.

Jon, I realized that my v6 variant is wrong too because
tegra_dma_terminate_all() -> tdc->isr_handler() will put RPM, and thus,
the RPM enable-count will be wrecked in this case.

I'm now leaning to adopt yours variant and simply remove the
tegra_dma_stop() from handle_continuous_head_request() because there
shouldn't be any harm in keeping DMA active in the case of error
condition. Besides, these error conditions are very extreme cases that
should never happen in practice.

The "list_empty(&tdc->pending_sg_req)" error seems couldn't ever happen
at all, I'll remove it in v7.