RE: [PATCH v3 19/24] dmaengine: dw-edma: Use non-atomic io-64 methods

From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: "Serge Semin" <fancer.lancer@gmail.com>,
	"Alexey Malahov" <Alexey.Malahov@baikalelectronics.ru>,
	"Pavel Parkhomenko" <Pavel.Parkhomenko@baikalelectronics.ru>,
	"Krzysztof Wilczyński" <kw@linux.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Gustavo Pimentel" <gustavo.pimentel@synopsys.com>,
	"Vinod Koul" <vkoul@kernel.org>, "Rob Herring" <robh@kernel.org>,
	"Bjorn Helgaas" <bhelgaas@google.com>,
	"Lorenzo Pieralisi" <lorenzo.pieralisi@arm.com>,
	"Jingoo Han" <jingoohan1@gmail.com>,
	"Frank Li" <Frank.Li@nxp.com>,
	"Manivannan Sadhasivam" <manivannan.sadhasivam@linaro.org>
Subject: RE: [PATCH v3 19/24] dmaengine: dw-edma: Use non-atomic io-64 methods
Date: Wed, 6 Jul 2022 05:42:31 +0000	[thread overview]
Message-ID: <TYBPR01MB53418C1BC8791CB6D068A177D8809@TYBPR01MB5341.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <20220610091459.17612-20-Sergey.Semin@baikalelectronics.ru>

Hi,

> From: Serge Semin, Sent: Friday, June 10, 2022 6:15 PM
> 
> Instead of splitting the 64-bits IOs up into two 32-bits ones it's
> possible to use an available set of the non-atomic readq/writeq methods
> implemented exactly for such cases. They are defined in the dedicated
> header files io-64-nonatomic-lo-hi.h/io-64-nonatomic-hi-lo.h. So in case
> if the 64-bits readq/writeq methods are unavailable on some platforms at
> consideration, the corresponding drivers can have any of these headers
> included and stop locally re-implementing the 64-bits IO accessors taking
> into account the non-atomic nature of the included methods. Let's do that
> in the DW eDMA driver too. Note by doing so we can discard the
> CONFIG_64BIT config ifdefs from the code. Also note that if a platform
> doesn't support 64-bit DBI IOs then the corresponding accessors will just
> directly call the lo_hi_readq()/lo_hi_writeq() methods.
> 
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/dma/dw-edma/dw-edma-v0-core.c | 71 +++++++++------------------
>  1 file changed, 24 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c
> index e6d611176891..4348d2323125 100644
> --- a/drivers/dma/dw-edma/dw-edma-v0-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-v0-core.c
<snip>
> @@ -417,18 +404,8 @@ void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first)
>  		SET_CH_32(dw, chan->dir, chan->id, ch_control1,
>  			  (DW_EDMA_V0_CCS | DW_EDMA_V0_LLE));
>  		/* Linked list */
> -		if ((chan->dw->chip->flags & DW_EDMA_CHIP_32BIT_DBI) ||

I'm trying to use this patch series, but I could not apply this patch.
I investigated why, and then IIUC the DW_EDMA_CHIP_32BIT_DBI flag doesn't
exist on the following based patches:
https://patchwork.kernel.org/project/linux-pci/cover/20220624143947.8991-1-Sergey.Semin@baikalelectronics.ru/
https://patchwork.kernel.org/project/linux-dmaengine/cover/20220524152159.2370739-1-Frank.Li@nxp.com/

According to the comment from Zhi Li [1], the flag can be skipped by the fixed patch [2].
That's why the flag doesn't exist on the based patches.

[1]
https://patchwork.kernel.org/project/linux-dmaengine/patch/20220503005801.1714345-9-Frank.Li@nxp.com/#24844332
[2]
https://git.kernel.org/pub/scm/linux/kernel/git/vkoul/dmaengine.git/commit/?h=fixes&id=8fc5133d6d4da65cad6b73152fc714ad3d7f91c1

Since both codes in #ifdef and #else are the same, we can just remove code of the #else part.
But, what do you think?
-----
                #ifdef CONFIG_64BIT
                /* llp is not aligned on 64bit -> keep 32bit accesses */
                SET_CH_32(dw, chan->dir, chan->id, llp.lsb,
                          lower_32_bits(chunk->ll_region.paddr));
                SET_CH_32(dw, chan->dir, chan->id, llp.msb,
                          upper_32_bits(chunk->ll_region.paddr));
                #else /* CONFIG_64BIT */
                SET_CH_32(dw, chan->dir, chan->id, llp.lsb,
                          lower_32_bits(chunk->ll_region.paddr));
                SET_CH_32(dw, chan->dir, chan->id, llp.msb,
                          upper_32_bits(chunk->ll_region.paddr));
                #endif /* CONFIG_64BIT */
-----

Best regards,
Yoshihiro Shimoda

> -		    !IS_ENABLED(CONFIG_64BIT)) {
> -			SET_CH_32(dw, chan->dir, chan->id, llp.lsb,
> -				  lower_32_bits(chunk->ll_region.paddr));
> -			SET_CH_32(dw, chan->dir, chan->id, llp.msb,
> -				  upper_32_bits(chunk->ll_region.paddr));
> -		} else {
> -		#ifdef CONFIG_64BIT
> -			SET_CH_64(dw, chan->dir, chan->id, llp.reg,
> -				  chunk->ll_region.paddr);
> -		#endif
> -		}
> +		SET_CH_64(dw, chan->dir, chan->id, llp.reg,
> +			  chunk->ll_region.paddr);
>  	}
>  	/* Doorbell */
>  	SET_RW_32(dw, chan->dir, doorbell,
> --
> 2.35.1