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

From: Frank Li
Date: Wed Jul 06 2022 - 10:28:50 EST




> -----Original Message-----
> From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx>
> Sent: Wednesday, July 6, 2022 12:43 AM
> To: Serge Semin <Sergey.Semin@xxxxxxxxxxxxxxxxxxxx>
> Cc: Serge Semin <fancer.lancer@xxxxxxxxx>; Alexey Malahov
> <Alexey.Malahov@xxxxxxxxxxxxxxxxxxxx>; Pavel Parkhomenko
> <Pavel.Parkhomenko@xxxxxxxxxxxxxxxxxxxx>; Krzysztof Wilczyński
> <kw@xxxxxxxxx>; linux-pci@xxxxxxxxxxxxxxx; dmaengine@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; Gustavo Pimentel
> <gustavo.pimentel@xxxxxxxxxxxx>; Vinod Koul <vkoul@xxxxxxxxxx>; Rob
> Herring <robh@xxxxxxxxxx>; Bjorn Helgaas <bhelgaas@xxxxxxxxxx>; Lorenzo
> Pieralisi <lorenzo.pieralisi@xxxxxxx>; Jingoo Han <jingoohan1@xxxxxxxxx>;
> Frank Li <frank.li@xxxxxxx>; Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx>
> Subject: [EXT] RE: [PATCH v3 19/24] dmaengine: dw-edma: Use non-atomic
> io-64 methods
>
> Caution: EXT Email
>
> 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@xxxxxxxxxxxxxxxxxxxx>
> > Reviewed-by: Manivannan Sadhasivam
> <manivannan.sadhasivam@xxxxxxxxxx>
> > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx>
> > ---
> > 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fproject%2Flinux-pci%2Fcover%2F20220624143947.8991-
> 1-
> Sergey.Semin%40baikalelectronics.ru%2F&amp;data=05%7C01%7CFrank.Li%
> 40nxp.com%7C44fc7f7d7f844fb10b4a08da5f125456%7C686ea1d3bc2b4c6fa
> 92cd99c5c301635%7C0%7C0%7C637926829585015681%7CUnknown%7CTWF
> pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=nwJEnsQoej4RzpY9ZTDfOwh
> on%2BzjXz48Xx5Yz5WAR2w%3D&amp;reserved=0
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fproject%2Flinux-
> dmaengine%2Fcover%2F20220524152159.2370739-1-
> Frank.Li%40nxp.com%2F&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C4
> 4fc7f7d7f844fb10b4a08da5f125456%7C686ea1d3bc2b4c6fa92cd99c5c301635
> %7C0%7C0%7C637926829585015681%7CUnknown%7CTWFpbGZsb3d8eyJWI
> joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3
> 000%7C%7C%7C&amp;sdata=rNV74hncfbxxb4crPA2PGfkeIW68GBOiv58Q1yC
> heUo%3D&amp;reserved=0
>
> 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://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatch
> work.kernel.org%2Fproject%2Flinux-
> dmaengine%2Fpatch%2F20220503005801.1714345-9-
> Frank.Li%40nxp.com%2F%2324844332&amp;data=05%7C01%7CFrank.Li%40
> nxp.com%7C44fc7f7d7f844fb10b4a08da5f125456%7C686ea1d3bc2b4c6fa92c
> d99c5c301635%7C0%7C0%7C637926829585015681%7CUnknown%7CTWFpb
> GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Q%2Bx5IxIaQyS1oVZEoNXEL2X4
> SAB0ffO2NjMrUT3MGho%3D&amp;reserved=0
> [2]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgit.ker
> nel.org%2Fpub%2Fscm%2Flinux%2Fkernel%2Fgit%2Fvkoul%2Fdmaengine.git
> %2Fcommit%2F%3Fh%3Dfixes%26id%3D8fc5133d6d4da65cad6b73152fc714a
> d3d7f91c1&amp;data=05%7C01%7CFrank.Li%40nxp.com%7C44fc7f7d7f844f
> b10b4a08da5f125456%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7
> C637926829585015681%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw
> MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C
> %7C&amp;sdata=nGZetYm3da8Vj4adPAXZV7Rr0kXvcliW%2B0PlwOmsnsg%3
> D&amp;reserved=0
>
> 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 */
> -----
>

Latest Linux-next code have removed CONFIG_64BIT.

Best regards
Frank Li

> 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