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?
> #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
