Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: Revert "dmaengine: pl330: add DMA_PAUSE feature"
To:     Frank Mori Hess <fmh6jj@gmail.com>, Vinod Koul <vkoul@kernel.org>,
        dmaengine@vger.kernel.org
Cc:     linux-kernel@vger.kernel.org,
        Dan Williams <dan.j.williams@intel.com>,
        r.baldyga@hackerion.com, Krzysztof Kozlowski <krzk@kernel.org>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>
From:   Marek Szyprowski <m.szyprowski@samsung.com>
Message-ID: <53b13d76-16a1-0e0a-09e1-c917e5d49326@samsung.com>
Date:   Tue, 8 May 2018 11:04:06 +0200
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:52.0) Gecko/20100101
        Thunderbird/52.7.0
MIME-Version: 1.0
In-Reply-To: <2484918.HKVQc3yJkt@bear>
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 201P
References: <2484918.HKVQc3yJkt@bear>
        <CGME20180508090407eucas1p13713284c2d3f5aa5c66f8d136be683c1@eucas1p1.samsung.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Frank and Vinod,

On 2018-04-28 23:50, Frank Mori Hess wrote:
> This reverts commit 88987d2c7534a0269f567fb101e6d71a08f0f01d.
>
> The pl330.c pause implementation violates the dmaengine requirement
> for no data loss, since it relies on the DMAKILL
> instruction.  However, DMAKILL discards in-flight data from the
> dma controller's fifo.  This is documented in the dma-330 manual
> and I have observed it with hardware doing device-to-memory burst
> transfers.  The discarded data may or may not show up in the
> residue count, depending on timing (resulting in data corruption
> effectively).
>
> Signed-off-by: Frank Mori Hess <fmh6jj@gmail.com>

This revert completely breaks serial driver operation on almost all Exynos
SoCs, because serial driver relies on having PAUSE feature and proper
residue reporting from dma engine. Please drop it if possible.

> ---
>   drivers/dma/pl330.c | 28 ----------------------------
>   1 file changed, 28 deletions(-)
>
> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index 6237069001c4..f802bd3b0481 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
> @@ -2290,33 +2290,6 @@ static int pl330_terminate_all(struct dma_chan *chan)
>   	return 0;
>   }
>   
> -/*
> - * We don't support DMA_RESUME command because of hardware
> - * limitations, so after pausing the channel we cannot restore
> - * it to active state. We have to terminate channel and setup
> - * DMA transfer again. This pause feature was implemented to
> - * allow safely read residue before channel termination.
> - */
> -static int pl330_pause(struct dma_chan *chan)
> -{
> -	struct dma_pl330_chan *pch = to_pchan(chan);
> -	struct pl330_dmac *pl330 = pch->dmac;
> -	unsigned long flags;
> -
> -	pm_runtime_get_sync(pl330->ddma.dev);
> -	spin_lock_irqsave(&pch->lock, flags);
> -
> -	spin_lock(&pl330->lock);
> -	_stop(pch->thread);
> -	spin_unlock(&pl330->lock);
> -
> -	spin_unlock_irqrestore(&pch->lock, flags);
> -	pm_runtime_mark_last_busy(pl330->ddma.dev);
> -	pm_runtime_put_autosuspend(pl330->ddma.dev);
> -
> -	return 0;
> -}
> -
>   static void pl330_free_chan_resources(struct dma_chan *chan)
>   {
>   	struct dma_pl330_chan *pch = to_pchan(chan);
> @@ -3027,7 +3000,6 @@ pl330_probe(struct amba_device *adev, const struct amba_id *id)
>   	pd->device_tx_status = pl330_tx_status;
>   	pd->device_prep_slave_sg = pl330_prep_slave_sg;
>   	pd->device_config = pl330_config;
> -	pd->device_pause = pl330_pause;
>   	pd->device_terminate_all = pl330_terminate_all;
>   	pd->device_issue_pending = pl330_issue_pending;
>   	pd->src_addr_widths = PL330_DMA_BUSWIDTHS;
>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland