Re: [PATCH 1/2] dmaengine: tegra: Add terminate() for Tegra234

[Jon Hunter <jonathanh@nvidia.com>]

On 07/07/2022 11:47, Akhil R wrote:
>> On 07/07/2022 11:27, Akhil R wrote:
>>> In certain cases where the DMA client bus gets corrupted or if the end
>>> device ceases to send/receive data, DMA can wait indefinitely for the
>>> data to be received/sent. Attempting to terminate the transfer will
>>> put the DMA in pause flush mode and it remains there.
>>>
>>> The channel is irrecoverable once this pause times out in Tegra194 and
>>> earlier chips. Whereas, from Tegra234, it can be recovered by
>>> disabling the channel and reprograming it.
>>>
>>> Hence add a new terminate() function that ignores the outcome of
>>> dma_pause() and disables the channel.
>>>
>>> Signed-off-by: Akhil R <akhilrajeev@nvidia.com>
>>> ---
>>>    arch/arm64/boot/dts/nvidia/tegra234.dtsi |  5 +++--
>>>    drivers/dma/tegra186-gpc-dma.c           | 26 ++++++++++++++++++++++--
>>>    2 files changed, 27 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> index cf611eff7f6b..83d1ad7d3c8c 100644
>>> --- a/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> +++ b/arch/arm64/boot/dts/nvidia/tegra234.dtsi
>>> @@ -22,8 +22,9 @@
>>>    		ranges = <0x0 0x0 0x0 0x40000000>;
>>>
>>>    		gpcdma: dma-controller@2600000 {
>>> -			compatible = "nvidia,tegra194-gpcdma",
>>> -				      "nvidia,tegra186-gpcdma";
>>> +			compatible = "nvidia,tegra234-gpcdma",
>>> +				     "nvidia,tegra194-gpcdma",
>>> +				     "nvidia,tegra186-gpcdma";
>>>    			reg = <0x2600000 0x210000>;
>>>    			resets = <&bpmp TEGRA234_RESET_GPCDMA>;
>>>    			reset-names = "gpcdma";
>>
>> I think that this should be split into two patches.
>>
>>> diff --git a/drivers/dma/tegra186-gpc-dma.c
>>> b/drivers/dma/tegra186-gpc-dma.c index 05cd451f541d..fa9bda4a2bc6
>>> 100644
>>> --- a/drivers/dma/tegra186-gpc-dma.c
>>> +++ b/drivers/dma/tegra186-gpc-dma.c
>>> @@ -157,8 +157,8 @@
>>>     * If any burst is in flight and DMA paused then this is the time to complete
>>>     * on-flight burst and update DMA status register.
>>>     */
>>> -#define TEGRA_GPCDMA_BURST_COMPLETE_TIME	20
>>> -#define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT	100
>>> +#define TEGRA_GPCDMA_BURST_COMPLETE_TIME	10
>>> +#define TEGRA_GPCDMA_BURST_COMPLETION_TIMEOUT	5000 /* 5
>> msec */
>>>
>>>    /* Channel base address offset from GPCDMA base address */
>>>    #define TEGRA_GPCDMA_CHANNEL_BASE_ADD_OFFSET	0x20000
>>> @@ -432,6 +432,17 @@ static int tegra_dma_device_resume(struct
>> dma_chan *dc)
>>>    	return 0;
>>>    }
>>>
>>> +static inline int tegra_dma_pause_noerr(struct tegra_dma_channel
>>> +*tdc) {
>>> +	/* Return 0 irrespective of PAUSE status.
>>> +	 * This is useful to recover channels that can exit out of flush
>>> +	 * state when the channel is disabled.
>>> +	 */
>>> +
>>> +	tegra_dma_pause(tdc);
>>> +	return 0;
>>> +}
>>
>> The commit message says that "add a new terminate() function that ignores the
>> outcome of dma_pause() and disables the channel". But I only see pause being
>> done here.
> The function is set as .terminate() function in chip_data and is called during
> terminate_all(). Since this return 0, tegra_dma_terminate_all() will proceed
> and calls tegra_dma_disable() in the next step.


That's fine, but it is not clear from the commit message.

Jon

-- 
nvpublic