From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id BB6042E62C8 for ; Mon, 16 Jun 2025 11:56:55 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750075018; cv=none; b=RiaZpl+l+F/D6ARDtB79ZOsFgOeHhIL2KMZ2P8xCWsSuyL7Y31F5mElBqWkTFQYhV49mTgHPaE5ZzGctfqm0xA0ucQZSSkE/cvKJeuzMZa/oddmJ9DzTswlEZIJgT2GqOGPjtbvYI2Lkyj9OymAdCxtx5FcVTUBrZZH39NzdVQI= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1750075018; c=relaxed/simple; bh=3KqHxu39UApIOLYF5HgyfX74IQyuDyWpyGrIlCRcXa4=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=sNPlleEorVa60jAoE0jkjFv+g3UeUKoH+ITQE3mk3Kk+FzWb/rtafcn2P8icoHP2K/Y2LvoZZpmBf4VUdttI5VGVZs/4z0gptZe/e0WHbo53zFjftMNJ/MoqOY2jkICYNEMjp8j3R/OgblJLOtqHEHIWGt+uVOe5CSaVnuqKVwI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id A661F150C; Mon, 16 Jun 2025 04:56:33 -0700 (PDT) Received: from [10.57.28.131] (unknown [10.57.28.131]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 891413F58B; Mon, 16 Jun 2025 04:56:53 -0700 (PDT) Message-ID: Date: Mon, 16 Jun 2025 12:56:51 +0100 Precedence: bulk X-Mailing-List: imx@lists.linux.dev List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v2 2/5] spi: spi-fsl-dspi: Use non-coherent memory for DMA To: James Clark , Vladimir Oltean , Mark Brown Cc: Vladimir Oltean , Arnd Bergmann , Larisa Grigore , Frank Li , linux-spi@vger.kernel.org, imx@lists.linux.dev, linux-kernel@vger.kernel.org References: <20250613-james-nxp-spi-dma-v2-0-017eecf24aab@linaro.org> <20250613-james-nxp-spi-dma-v2-2-017eecf24aab@linaro.org> From: Robin Murphy Content-Language: en-GB In-Reply-To: <20250613-james-nxp-spi-dma-v2-2-017eecf24aab@linaro.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit On 2025-06-13 10:28 am, James Clark wrote: > Using coherent memory here isn't functionally necessary. Because the > change to use non-coherent memory isn't overly complex and only a few > synchronization points are required, we might as well do it while fixing > up some other DMA issues. If it doesn't need coherent memory then does the driver really need to do its own bounce-buffering at all? Could you not simplify the whole lot even more by getting rid of {tx,rx}_dma_buf altogether and relying on the SPI core helpers to DMA-map the messages in-place? Thanks, Robin. > Suggested-by: Arnd Bergmann > Signed-off-by: James Clark > --- > drivers/spi/spi-fsl-dspi.c | 55 +++++++++++++++++++++++++++++----------------- > 1 file changed, 35 insertions(+), 20 deletions(-) > > diff --git a/drivers/spi/spi-fsl-dspi.c b/drivers/spi/spi-fsl-dspi.c > index 744dfc561db2..f19404e10c92 100644 > --- a/drivers/spi/spi-fsl-dspi.c > +++ b/drivers/spi/spi-fsl-dspi.c > @@ -379,6 +379,11 @@ static bool is_s32g_dspi(struct fsl_dspi *data) > data->devtype_data == &devtype_data[S32G_TARGET]; > } > > +static int dspi_dma_transfer_size(struct fsl_dspi *dspi) > +{ > + return dspi->words_in_flight * DMA_SLAVE_BUSWIDTH_4_BYTES; > +} > + > static void dspi_native_host_to_dev(struct fsl_dspi *dspi, u32 *txdata) > { > switch (dspi->oper_word_size) { > @@ -493,7 +498,10 @@ static void dspi_tx_dma_callback(void *arg) > { > struct fsl_dspi *dspi = arg; > struct fsl_dspi_dma *dma = dspi->dma; > + struct device *dev = &dspi->pdev->dev; > > + dma_sync_single_for_cpu(dev, dma->tx_dma_phys, > + dspi_dma_transfer_size(dspi), DMA_TO_DEVICE); > complete(&dma->cmd_tx_complete); > } > > @@ -501,9 +509,13 @@ static void dspi_rx_dma_callback(void *arg) > { > struct fsl_dspi *dspi = arg; > struct fsl_dspi_dma *dma = dspi->dma; > + struct device *dev = &dspi->pdev->dev; > int i; > > if (dspi->rx) { > + dma_sync_single_for_cpu(dev, dma->rx_dma_phys, > + dspi_dma_transfer_size(dspi), > + DMA_FROM_DEVICE); > for (i = 0; i < dspi->words_in_flight; i++) > dspi_push_rx(dspi, dspi->dma->rx_dma_buf[i]); > } > @@ -513,6 +525,7 @@ static void dspi_rx_dma_callback(void *arg) > > static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi) > { > + size_t size = dspi_dma_transfer_size(dspi); > struct device *dev = &dspi->pdev->dev; > struct fsl_dspi_dma *dma = dspi->dma; > int time_left; > @@ -521,10 +534,9 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi) > for (i = 0; i < dspi->words_in_flight; i++) > dspi->dma->tx_dma_buf[i] = dspi_pop_tx_pushr(dspi); > > + dma_sync_single_for_device(dev, dma->tx_dma_phys, size, DMA_TO_DEVICE); > dma->tx_desc = dmaengine_prep_slave_single(dma->chan_tx, > - dma->tx_dma_phys, > - dspi->words_in_flight * > - DMA_SLAVE_BUSWIDTH_4_BYTES, > + dma->tx_dma_phys, size, > DMA_MEM_TO_DEV, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > if (!dma->tx_desc) { > @@ -539,10 +551,10 @@ static int dspi_next_xfer_dma_submit(struct fsl_dspi *dspi) > return -EINVAL; > } > > + dma_sync_single_for_device(dev, dma->rx_dma_phys, size, > + DMA_FROM_DEVICE); > dma->rx_desc = dmaengine_prep_slave_single(dma->chan_rx, > - dma->rx_dma_phys, > - dspi->words_in_flight * > - DMA_SLAVE_BUSWIDTH_4_BYTES, > + dma->rx_dma_phys, size, > DMA_DEV_TO_MEM, > DMA_PREP_INTERRUPT | DMA_CTRL_ACK); > if (!dma->rx_desc) { > @@ -644,17 +656,17 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) > goto err_tx_channel; > } > > - dma->tx_dma_buf = dma_alloc_coherent(dma->chan_tx->device->dev, > - dma_bufsize, &dma->tx_dma_phys, > - GFP_KERNEL); > + dma->tx_dma_buf = dma_alloc_noncoherent(dma->chan_tx->device->dev, > + dma_bufsize, &dma->tx_dma_phys, > + DMA_TO_DEVICE, GFP_KERNEL); > if (!dma->tx_dma_buf) { > ret = -ENOMEM; > goto err_tx_dma_buf; > } > > - dma->rx_dma_buf = dma_alloc_coherent(dma->chan_rx->device->dev, > - dma_bufsize, &dma->rx_dma_phys, > - GFP_KERNEL); > + dma->rx_dma_buf = dma_alloc_noncoherent(dma->chan_rx->device->dev, > + dma_bufsize, &dma->rx_dma_phys, > + DMA_FROM_DEVICE, GFP_KERNEL); > if (!dma->rx_dma_buf) { > ret = -ENOMEM; > goto err_rx_dma_buf; > @@ -689,11 +701,12 @@ static int dspi_request_dma(struct fsl_dspi *dspi, phys_addr_t phy_addr) > return 0; > > err_slave_config: > - dma_free_coherent(dma->chan_rx->device->dev, > - dma_bufsize, dma->rx_dma_buf, dma->rx_dma_phys); > + dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize, > + dma->rx_dma_buf, dma->rx_dma_phys, > + DMA_FROM_DEVICE); > err_rx_dma_buf: > - dma_free_coherent(dma->chan_tx->device->dev, > - dma_bufsize, dma->tx_dma_buf, dma->tx_dma_phys); > + dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize, > + dma->tx_dma_buf, dma->tx_dma_phys, DMA_TO_DEVICE); > err_tx_dma_buf: > dma_release_channel(dma->chan_tx); > err_tx_channel: > @@ -714,14 +727,16 @@ static void dspi_release_dma(struct fsl_dspi *dspi) > return; > > if (dma->chan_tx) { > - dma_free_coherent(dma->chan_tx->device->dev, dma_bufsize, > - dma->tx_dma_buf, dma->tx_dma_phys); > + dma_free_noncoherent(dma->chan_tx->device->dev, dma_bufsize, > + dma->tx_dma_buf, dma->tx_dma_phys, > + DMA_TO_DEVICE); > dma_release_channel(dma->chan_tx); > } > > if (dma->chan_rx) { > - dma_free_coherent(dma->chan_rx->device->dev, dma_bufsize, > - dma->rx_dma_buf, dma->rx_dma_phys); > + dma_free_noncoherent(dma->chan_rx->device->dev, dma_bufsize, > + dma->rx_dma_buf, dma->rx_dma_phys, > + DMA_FROM_DEVICE); > dma_release_channel(dma->chan_rx); > } > } >