| Message ID | 20230113171409.30470-20-Sergey.Semin@baikalelectronics.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | dmaengine: dw-edma: Add RP/EP local DMA controllers support | expand |
On Fri, Jan 13, 2023 at 08:14:01PM +0300, Serge Semin wrote: > Instead of splitting the 64-bits IOs up into two 32-bits ones it's > possible to use the already available 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. > > 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> > Acked-by: Vinod Koul <vkoul@kernel.org> > --- > drivers/dma/dw-edma/dw-edma-v0-core.c | 55 +++++++++------------------ > 1 file changed, 18 insertions(+), 37 deletions(-) > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > index 66f296daac5a..51a34b43434c 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 */ Great to get rid of these #ifdefs! Am I missing something? It looks like SET_64 is used only by SET_RW_64 and SET_BOTH_64, and neither of *them is used at all. Similarly for GET_64 and GET_RW_64. So maybe we could get rid of everything inside the #ifdefs as well? > #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,22 @@ 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)); > - writeq(value, addr); > + } > + > + writeq(value, addr); > > + if (dw->chip->mf == EDMA_MF_EDMA_LEGACY) > raw_spin_unlock_irqrestore(&dw->lock, flags); > - } else { > - writeq(value, addr); > - } This is basically a cosmetic change unrelated to the commit log. I don't really object to the change, although I think it's of dubious value to remove the repetition of the writeq() at the cost of adding another "if" and unlock. Lorenzo already applied this, so it's OK as-is unless you think it's worth reworking to drop the unused stuff mentioned above, in which case this rearrangement could be moved to a separate patch to make both of them more focused. Bjorn
On Fri, Jan 20, 2023 at 06:54:01PM -0600, Bjorn Helgaas wrote: > On Fri, Jan 13, 2023 at 08:14:01PM +0300, Serge Semin wrote: > > Instead of splitting the 64-bits IOs up into two 32-bits ones it's > > possible to use the already available 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. > > > > 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> > > Acked-by: Vinod Koul <vkoul@kernel.org> > > --- > > drivers/dma/dw-edma/dw-edma-v0-core.c | 55 +++++++++------------------ > > 1 file changed, 18 insertions(+), 37 deletions(-) > > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > index 66f296daac5a..51a34b43434c 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 */ > > Great to get rid of these #ifdefs! > > Am I missing something? It looks like SET_64 is used only by > SET_RW_64 and SET_BOTH_64, and neither of *them is used at all. > > Similarly for GET_64 and GET_RW_64. > > So maybe we could get rid of everything inside the #ifdefs as well? Even though these macros are indeed unused in the driver they are still a part of the DW eDMA CSRs access interface. In particular they are supposed to be used to access the 64-bit registers declared in the dw_edma_v0_regs, dw_edma_v0_unroll and dw_edma_v0_ch_regs structures. So until the interface is converted to a more preferable direct MMIO usage without any packed-structures I'd rather leave these macros be. > > > #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,22 @@ 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)); > > - writeq(value, addr); > > + } > > + > > + writeq(value, addr); > > > > + if (dw->chip->mf == EDMA_MF_EDMA_LEGACY) > > raw_spin_unlock_irqrestore(&dw->lock, flags); > > - } else { > > - writeq(value, addr); > > - } > > This is basically a cosmetic change unrelated to the commit log. I > don't really object to the change, although I think it's of dubious > value to remove the repetition of the writeq() at the cost of adding > another "if" and unlock. > The denoted change is basically a leftover of the originally more complex modification: https://lore.kernel.org/linux-pci/20220503225104.12108-21-Sergey.Semin@baikalelectronics.ru for which the update made more sense. But then the corresponding flag was dropped from the Frank' patchset and I had to reduce the patch to the code above. Though I agree with you in both aspects. Indeed the value of the change is questionable and it isn't related to the commit log. > Lorenzo already applied this, so it's OK as-is unless you think it's > worth reworking to drop the unused stuff mentioned above, in which > case this rearrangement could be moved to a separate patch to make > both of them more focused. As I said above I'd rather leave the 64-bit accessors be until the packed structure-based interface is removed from the driver. Regarding the QWORD accessors update. If @Lorenzo agrees to replace the already applied v9 patchset with a new one I can resubmit the series with no rearrangement above. -Serge(y) > > Bjorn
On Sun, Jan 22, 2023 at 02:03:59AM +0300, Serge Semin wrote: > On Fri, Jan 20, 2023 at 06:54:01PM -0600, Bjorn Helgaas wrote: > > On Fri, Jan 13, 2023 at 08:14:01PM +0300, Serge Semin wrote: > > > Instead of splitting the 64-bits IOs up into two 32-bits ones it's > > > possible to use the already available 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. > > > > > > 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> > > > Acked-by: Vinod Koul <vkoul@kernel.org> > > > --- > > > drivers/dma/dw-edma/dw-edma-v0-core.c | 55 +++++++++------------------ > > > 1 file changed, 18 insertions(+), 37 deletions(-) > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > > index 66f296daac5a..51a34b43434c 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 */ > > > > > Great to get rid of these #ifdefs! > > > > Am I missing something? It looks like SET_64 is used only by > > SET_RW_64 and SET_BOTH_64, and neither of *them is used at all. > > > > Similarly for GET_64 and GET_RW_64. > > > > So maybe we could get rid of everything inside the #ifdefs as well? > > Even though these macros are indeed unused in the driver they are > still a part of the DW eDMA CSRs access interface. In particular they > are supposed to be used to access the 64-bit registers declared in the > dw_edma_v0_regs, dw_edma_v0_unroll and dw_edma_v0_ch_regs structures. > So until the interface is converted to a more preferable direct MMIO > usage without any packed-structures I'd rather leave these macros > be. > ... > As I said above I'd rather leave the 64-bit accessors be until the > packed structure-based interface is removed from the driver. I wouldn't bother polishing something that's unused since it can't be tested, it's easy to resurrect from the history if/when it becomes necessary, and it makes it much harder to connect the commit log with the code change. But this isn't a drivers/pci change, so I'm fine with it since Vinod acked it. I guess the point is that when !CONFIG_64BIT, there was no writeq() so we used SET_LL_32 twice. linux/io-64-nonatomic-lo-hi.h provides that writeq() implementation, so we can define and use SET_LL_64 for that case. Bjorn
On Mon, Jan 23, 2023 at 10:37:09AM -0600, Bjorn Helgaas wrote: > On Sun, Jan 22, 2023 at 02:03:59AM +0300, Serge Semin wrote: > > On Fri, Jan 20, 2023 at 06:54:01PM -0600, Bjorn Helgaas wrote: > > > On Fri, Jan 13, 2023 at 08:14:01PM +0300, Serge Semin wrote: > > > > Instead of splitting the 64-bits IOs up into two 32-bits ones it's > > > > possible to use the already available 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. > > > > > > > > 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> > > > > Acked-by: Vinod Koul <vkoul@kernel.org> > > > > --- > > > > drivers/dma/dw-edma/dw-edma-v0-core.c | 55 +++++++++------------------ > > > > 1 file changed, 18 insertions(+), 37 deletions(-) > > > > > > > > diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c > > > > index 66f296daac5a..51a34b43434c 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 */ > > > > > > > > Great to get rid of these #ifdefs! > > > > > > Am I missing something? It looks like SET_64 is used only by > > > SET_RW_64 and SET_BOTH_64, and neither of *them is used at all. > > > > > > Similarly for GET_64 and GET_RW_64. > > > > > > So maybe we could get rid of everything inside the #ifdefs as well? > > > > Even though these macros are indeed unused in the driver they are > > still a part of the DW eDMA CSRs access interface. In particular they > > are supposed to be used to access the 64-bit registers declared in the > > dw_edma_v0_regs, dw_edma_v0_unroll and dw_edma_v0_ch_regs structures. > > So until the interface is converted to a more preferable direct MMIO > > usage without any packed-structures I'd rather leave these macros > > be. > > ... > > > As I said above I'd rather leave the 64-bit accessors be until the > > packed structure-based interface is removed from the driver. > > I wouldn't bother polishing something that's unused since it can't be > tested, it's easy to resurrect from the history if/when it becomes > necessary, and it makes it much harder to connect the commit log with > the code change. But this isn't a drivers/pci change, so I'm fine > with it since Vinod acked it. > > I guess the point is that when !CONFIG_64BIT, there was no writeq() so > we used SET_LL_32 twice. linux/io-64-nonatomic-lo-hi.h provides that > writeq() implementation, so we can define and use SET_LL_64 for that > case. Right. That's what is done in this patch. -Serge(y) > > Bjorn
diff --git a/drivers/dma/dw-edma/dw-edma-v0-core.c b/drivers/dma/dw-edma/dw-edma-v0-core.c index 66f296daac5a..51a34b43434c 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,22 @@ 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)); - writeq(value, addr); + } + + 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 +203,12 @@ 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)); - value = readq(addr); + } + + 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 +222,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 +320,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 +335,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)