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@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. Right. That's what is done in this patch. -Serge(y) > > Bjorn
