| Message ID | 20220610091459.17612-20-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Handled Elsewhere |
| Headers | show |
| Series | dmaengine: dw-edma: Add RP/EP local DMA controllers support | expand |
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
> -----Original Message----- > From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > Sent: Wednesday, July 6, 2022 12:43 AM > 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; dmaengine@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: [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@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://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&data=05%7C01%7CFrank.Li% > 40nxp.com%7C44fc7f7d7f844fb10b4a08da5f125456%7C686ea1d3bc2b4c6fa > 92cd99c5c301635%7C0%7C0%7C637926829585015681%7CUnknown%7CTWF > pbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV > CI6Mn0%3D%7C3000%7C%7C%7C&sdata=nwJEnsQoej4RzpY9ZTDfOwh > on%2BzjXz48Xx5Yz5WAR2w%3D&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&data=05%7C01%7CFrank.Li%40nxp.com%7C4 > 4fc7f7d7f844fb10b4a08da5f125456%7C686ea1d3bc2b4c6fa92cd99c5c301635 > %7C0%7C0%7C637926829585015681%7CUnknown%7CTWFpbGZsb3d8eyJWI > joiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3 > 000%7C%7C%7C&sdata=rNV74hncfbxxb4crPA2PGfkeIW68GBOiv58Q1yC > heUo%3D&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&data=05%7C01%7CFrank.Li%40 > nxp.com%7C44fc7f7d7f844fb10b4a08da5f125456%7C686ea1d3bc2b4c6fa92c > d99c5c301635%7C0%7C0%7C637926829585015681%7CUnknown%7CTWFpb > GZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6 > Mn0%3D%7C3000%7C%7C%7C&sdata=Q%2Bx5IxIaQyS1oVZEoNXEL2X4 > SAB0ffO2NjMrUT3MGho%3D&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&data=05%7C01%7CFrank.Li%40nxp.com%7C44fc7f7d7f844f > b10b4a08da5f125456%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7 > C637926829585015681%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw > MDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C > %7C&sdata=nGZetYm3da8Vj4adPAXZV7Rr0kXvcliW%2B0PlwOmsnsg%3 > D&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
Hi, > From: Frank Li, Sent: Wednesday, July 6, 2022 11:29 PM > > > > From: Serge Semin, Sent: Friday, June 10, 2022 6:15 PM <snip> > > > > 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. Thank you for the information! I had looked the PCI repository only. I confirmed the latest Linux-next (next-20220706 tag) code have removed the CONFIG_64BIT. Best regards, Yoshihiro Shimoda
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 @@ -8,6 +8,8 @@ #include <linux/bitfield.h> +#include <linux/io-64-nonatomic-lo-hi.h> + #include "dw-edma-core.h" #include "dw-edma-v0-core.h" #include "dw-edma-v0-regs.h" @@ -53,8 +55,6 @@ static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw) SET_32(dw, rd_##name, value); \ } while (0) -#ifdef CONFIG_64BIT - #define SET_64(dw, name, value) \ writeq(value, &(__dw_regs(dw)->name)) @@ -80,8 +80,6 @@ static inline struct dw_edma_v0_regs __iomem *__dw_regs(struct dw_edma *dw) SET_64(dw, rd_##name, value); \ } while (0) -#endif /* CONFIG_64BIT */ - #define SET_COMPAT(dw, name, value) \ writel(value, &(__dw_regs(dw)->type.unroll.name)) @@ -164,14 +162,13 @@ static inline u32 readl_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, #define SET_LL_32(ll, value) \ writel(value, ll) -#ifdef CONFIG_64BIT - static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, u64 value, void __iomem *addr) { + unsigned long flags; + if (dw->chip->mf == EDMA_MF_EDMA_LEGACY) { u32 viewport_sel; - unsigned long flags; raw_spin_lock_irqsave(&dw->lock, flags); @@ -181,22 +178,25 @@ static inline void writeq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, writel(viewport_sel, &(__dw_regs(dw)->type.legacy.viewport_sel)); + } + + if (dw->chip->flags & DW_EDMA_CHIP_32BIT_DBI) + lo_hi_writeq(value, addr); + else writeq(value, addr); + if (dw->chip->mf == EDMA_MF_EDMA_LEGACY) raw_spin_unlock_irqrestore(&dw->lock, flags); - } else { - writeq(value, addr); - } } static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, const void __iomem *addr) { - u32 value; + unsigned long flags; + u64 value; if (dw->chip->mf == EDMA_MF_EDMA_LEGACY) { u32 viewport_sel; - unsigned long flags; raw_spin_lock_irqsave(&dw->lock, flags); @@ -206,12 +206,15 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, writel(viewport_sel, &(__dw_regs(dw)->type.legacy.viewport_sel)); + } + + if (dw->chip->flags & DW_EDMA_CHIP_32BIT_DBI) + value = lo_hi_readq(addr); + else value = readq(addr); + if (dw->chip->mf == EDMA_MF_EDMA_LEGACY) raw_spin_unlock_irqrestore(&dw->lock, flags); - } else { - value = readq(addr); - } return value; } @@ -225,8 +228,6 @@ static inline u64 readq_ch(struct dw_edma *dw, enum dw_edma_dir dir, u16 ch, #define SET_LL_64(ll, value) \ writeq(value, ll) -#endif /* CONFIG_64BIT */ - /* eDMA management callbacks */ void dw_edma_v0_core_off(struct dw_edma *dw) { @@ -325,19 +326,10 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) /* Transfer size */ SET_LL_32(&lli[i].transfer_size, child->sz); /* SAR */ - #ifdef CONFIG_64BIT - SET_LL_64(&lli[i].sar.reg, child->sar); - #else /* CONFIG_64BIT */ - SET_LL_32(&lli[i].sar.lsb, lower_32_bits(child->sar)); - SET_LL_32(&lli[i].sar.msb, upper_32_bits(child->sar)); - #endif /* CONFIG_64BIT */ + SET_LL_64(&lli[i].sar.reg, child->sar); /* DAR */ - #ifdef CONFIG_64BIT - SET_LL_64(&lli[i].dar.reg, child->dar); - #else /* CONFIG_64BIT */ - SET_LL_32(&lli[i].dar.lsb, lower_32_bits(child->dar)); - SET_LL_32(&lli[i].dar.msb, upper_32_bits(child->dar)); - #endif /* CONFIG_64BIT */ + SET_LL_64(&lli[i].dar.reg, child->dar); + i++; } @@ -349,12 +341,7 @@ static void dw_edma_v0_core_write_chunk(struct dw_edma_chunk *chunk) /* Channel control */ SET_LL_32(&llp->control, control); /* Linked list */ - #ifdef CONFIG_64BIT - SET_LL_64(&llp->llp.reg, chunk->ll_region.paddr); - #else /* CONFIG_64BIT */ - SET_LL_32(&llp->llp.lsb, lower_32_bits(chunk->ll_region.paddr)); - SET_LL_32(&llp->llp.msb, upper_32_bits(chunk->ll_region.paddr)); - #endif /* CONFIG_64BIT */ + SET_LL_64(&llp->llp.reg, chunk->ll_region.paddr); } void dw_edma_v0_core_start(struct dw_edma_chunk *chunk, bool first) @@ -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) || - !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,