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@xxxxxxxxxxxxxxxxxxxx> > > > Reviewed-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > > Tested-by: Manivannan Sadhasivam <manivannan.sadhasivam@xxxxxxxxxx> > > > Acked-by: Vinod Koul <vkoul@xxxxxxxxxx> > > > --- > > > 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
