| Message ID | 20220820003125.353570-6-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: ioremap: Convert architectures to take GENERIC_IOREMAP way | expand |
> -----Original Message----- > From: Baoquan He <bhe@redhat.com> ... > With it, the old ioremap() and iounmap() can be perfectly removed > since they are duplicated with the standard functions. > > Signed-off-by: Baoquan He <bhe@redhat.com> > Cc: Brian Cain <bcain@quicinc.com> > Cc: linux-hexagon@vger.kernel.org > --- Acked-by: Brian Cain <bcain@quicinc.com>
On Sat, Aug 20, 2022 at 08:31:19AM +0800, Baoquan He wrote: > With it, the old ioremap() and iounmap() can be perfectly removed > since they are duplicated with the standard functions. Can you write a somewhat better commit message explaining that it switches to the generic code that has equivalent functionality? > +#define ioremap_uc(addr, size) ioremap((addr), (size)) This is wrong and hexagon should use the asm-generic version of ioremap_uc that returns NULL.
Le 20/08/2022 à 02:31, Baoquan He a écrit : > With it, the old ioremap() and iounmap() can be perfectly removed > since they are duplicated with the standard functions. I see nothing related to the preparation patches. Could this have been converted without all previous patches ? In that case I think this one should go in the begining of the series, it would help see which architectecture really needs the changes to the generic parts. > > Signed-off-by: Baoquan He <bhe@redhat.com> > Cc: Brian Cain <bcain@quicinc.com> > Cc: linux-hexagon@vger.kernel.org > --- > arch/hexagon/Kconfig | 1 + > arch/hexagon/include/asm/io.h | 9 +++++-- > arch/hexagon/mm/ioremap.c | 44 ----------------------------------- > 3 files changed, 8 insertions(+), 46 deletions(-) > delete mode 100644 arch/hexagon/mm/ioremap.c > > diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig > index 54eadf265178..17afffde1a7f 100644 > --- a/arch/hexagon/Kconfig > +++ b/arch/hexagon/Kconfig > @@ -25,6 +25,7 @@ config HEXAGON > select NEED_SG_DMA_LENGTH > select NO_IOPORT_MAP > select GENERIC_IOMAP > + select GENERIC_IOREMAP > select GENERIC_SMP_IDLE_THREAD > select STACKTRACE_SUPPORT > select GENERIC_CLOCKEVENTS_BROADCAST > diff --git a/arch/hexagon/include/asm/io.h b/arch/hexagon/include/asm/io.h > index c33241425a5c..e2d3091ec9d6 100644 > --- a/arch/hexagon/include/asm/io.h > +++ b/arch/hexagon/include/asm/io.h > @@ -170,8 +170,13 @@ static inline void writel(u32 data, volatile void __iomem *addr) > #define writew_relaxed __raw_writew > #define writel_relaxed __raw_writel > > -void __iomem *ioremap(unsigned long phys_addr, unsigned long size); > -#define ioremap_uc(X, Y) ioremap((X), (Y)) > +/* > + * I/O memory mapping functions. > + */ > +#define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_READ | _PAGE_WRITE | \ > + (__HEXAGON_C_DEV << 6)) > + > +#define ioremap_uc(addr, size) ioremap((addr), (size)) > > > #define __raw_writel writel > diff --git a/arch/hexagon/mm/ioremap.c b/arch/hexagon/mm/ioremap.c > deleted file mode 100644 > index 255c5b1ee1a7..000000000000 > --- a/arch/hexagon/mm/ioremap.c > +++ /dev/null > @@ -1,44 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0-only > -/* > - * I/O remap functions for Hexagon > - * > - * Copyright (c) 2010-2011, The Linux Foundation. All rights reserved. > - */ > - > -#include <linux/io.h> > -#include <linux/vmalloc.h> > -#include <linux/mm.h> > - > -void __iomem *ioremap(unsigned long phys_addr, unsigned long size) > -{ > - unsigned long last_addr, addr; > - unsigned long offset = phys_addr & ~PAGE_MASK; > - struct vm_struct *area; > - > - pgprot_t prot = __pgprot(_PAGE_PRESENT|_PAGE_READ|_PAGE_WRITE > - |(__HEXAGON_C_DEV << 6)); > - > - last_addr = phys_addr + size - 1; > - > - /* Wrapping not allowed */ > - if (!size || (last_addr < phys_addr)) > - return NULL; > - > - /* Rounds up to next page size, including whole-page offset */ > - size = PAGE_ALIGN(offset + size); > - > - area = get_vm_area(size, VM_IOREMAP); > - addr = (unsigned long)area->addr; > - > - if (ioremap_page_range(addr, addr+size, phys_addr, prot)) { > - vunmap((void *)addr); > - return NULL; > - } > - > - return (void __iomem *) (offset + addr); > -} > - > -void iounmap(const volatile void __iomem *addr) > -{ > - vunmap((void *) ((unsigned long) addr & PAGE_MASK)); > -}
On 08/21/22 at 12:00am, Christoph Hellwig wrote: > On Sat, Aug 20, 2022 at 08:31:19AM +0800, Baoquan He wrote: > > With it, the old ioremap() and iounmap() can be perfectly removed > > since they are duplicated with the standard functions. > > Can you write a somewhat better commit message explaining that it > switches to the generic code that has equivalent functionality? OK, I will rephrase like below. Please check if it's OK. == By taking GENERIC_IOREMAP method, the generic ioremap_prot() and iounmap() are visible and available to arch. Arch only needs to provide implementation of arch_ioremap() or arch_iounmap() if there's arch specific handling needed in its ioremap() or iounmap(). This change will simplify implementation by removing duplicated codes with generic ioremap() and iounma(), and has the equivalent functioality as before. > > > +#define ioremap_uc(addr, size) ioremap((addr), (size)) > > This is wrong and hexagon should use the asm-generic version of > ioremap_uc that returns NULL. I don't follow. Do you mean the function verion, but not a macro? Or define it like below? #define ioremap_uc(addr, size) \ ioremap_prot((addr), (size), _PAGE_IOREMAP)
On 08/22/22 at 06:38am, Christophe Leroy wrote: > > > Le 20/08/2022 à 02:31, Baoquan He a écrit : > > With it, the old ioremap() and iounmap() can be perfectly removed > > since they are duplicated with the standard functions. > > I see nothing related to the preparation patches. Could this have been > converted without all previous patches ? In that case I think this one > should go in the begining of the series, it would help see which > architectecture really needs the changes to the generic parts. You are right. Hexagon basically has the same implementation as standard ioremap and iounmap code. I will move this at the beginning of patch series in next round of post. Thanks for point this out. > > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > Cc: Brian Cain <bcain@quicinc.com> > > Cc: linux-hexagon@vger.kernel.org > > --- > > arch/hexagon/Kconfig | 1 + > > arch/hexagon/include/asm/io.h | 9 +++++-- > > arch/hexagon/mm/ioremap.c | 44 ----------------------------------- > > 3 files changed, 8 insertions(+), 46 deletions(-) > > delete mode 100644 arch/hexagon/mm/ioremap.c > > > > diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig > > index 54eadf265178..17afffde1a7f 100644 > > --- a/arch/hexagon/Kconfig > > +++ b/arch/hexagon/Kconfig > > @@ -25,6 +25,7 @@ config HEXAGON > > select NEED_SG_DMA_LENGTH > > select NO_IOPORT_MAP > > select GENERIC_IOMAP > > + select GENERIC_IOREMAP > > select GENERIC_SMP_IDLE_THREAD > > select STACKTRACE_SUPPORT > > select GENERIC_CLOCKEVENTS_BROADCAST > > diff --git a/arch/hexagon/include/asm/io.h b/arch/hexagon/include/asm/io.h > > index c33241425a5c..e2d3091ec9d6 100644 > > --- a/arch/hexagon/include/asm/io.h > > +++ b/arch/hexagon/include/asm/io.h > > @@ -170,8 +170,13 @@ static inline void writel(u32 data, volatile void __iomem *addr) > > #define writew_relaxed __raw_writew > > #define writel_relaxed __raw_writel > > > > -void __iomem *ioremap(unsigned long phys_addr, unsigned long size); > > -#define ioremap_uc(X, Y) ioremap((X), (Y)) > > +/* > > + * I/O memory mapping functions. > > + */ > > +#define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_READ | _PAGE_WRITE | \ > > + (__HEXAGON_C_DEV << 6)) > > + > > +#define ioremap_uc(addr, size) ioremap((addr), (size)) > > > > > > #define __raw_writel writel > > diff --git a/arch/hexagon/mm/ioremap.c b/arch/hexagon/mm/ioremap.c > > deleted file mode 100644 > > index 255c5b1ee1a7..000000000000 > > --- a/arch/hexagon/mm/ioremap.c > > +++ /dev/null > > @@ -1,44 +0,0 @@ > > -// SPDX-License-Identifier: GPL-2.0-only > > -/* > > - * I/O remap functions for Hexagon > > - * > > - * Copyright (c) 2010-2011, The Linux Foundation. All rights reserved. > > - */ > > - > > -#include <linux/io.h> > > -#include <linux/vmalloc.h> > > -#include <linux/mm.h> > > - > > -void __iomem *ioremap(unsigned long phys_addr, unsigned long size) > > -{ > > - unsigned long last_addr, addr; > > - unsigned long offset = phys_addr & ~PAGE_MASK; > > - struct vm_struct *area; > > - > > - pgprot_t prot = __pgprot(_PAGE_PRESENT|_PAGE_READ|_PAGE_WRITE > > - |(__HEXAGON_C_DEV << 6)); > > - > > - last_addr = phys_addr + size - 1; > > - > > - /* Wrapping not allowed */ > > - if (!size || (last_addr < phys_addr)) > > - return NULL; > > - > > - /* Rounds up to next page size, including whole-page offset */ > > - size = PAGE_ALIGN(offset + size); > > - > > - area = get_vm_area(size, VM_IOREMAP); > > - addr = (unsigned long)area->addr; > > - > > - if (ioremap_page_range(addr, addr+size, phys_addr, prot)) { > > - vunmap((void *)addr); > > - return NULL; > > - } > > - > > - return (void __iomem *) (offset + addr); > > -} > > - > > -void iounmap(const volatile void __iomem *addr) > > -{ > > - vunmap((void *) ((unsigned long) addr & PAGE_MASK)); > > -}
diff --git a/arch/hexagon/Kconfig b/arch/hexagon/Kconfig index 54eadf265178..17afffde1a7f 100644 --- a/arch/hexagon/Kconfig +++ b/arch/hexagon/Kconfig @@ -25,6 +25,7 @@ config HEXAGON select NEED_SG_DMA_LENGTH select NO_IOPORT_MAP select GENERIC_IOMAP + select GENERIC_IOREMAP select GENERIC_SMP_IDLE_THREAD select STACKTRACE_SUPPORT select GENERIC_CLOCKEVENTS_BROADCAST diff --git a/arch/hexagon/include/asm/io.h b/arch/hexagon/include/asm/io.h index c33241425a5c..e2d3091ec9d6 100644 --- a/arch/hexagon/include/asm/io.h +++ b/arch/hexagon/include/asm/io.h @@ -170,8 +170,13 @@ static inline void writel(u32 data, volatile void __iomem *addr) #define writew_relaxed __raw_writew #define writel_relaxed __raw_writel -void __iomem *ioremap(unsigned long phys_addr, unsigned long size); -#define ioremap_uc(X, Y) ioremap((X), (Y)) +/* + * I/O memory mapping functions. + */ +#define _PAGE_IOREMAP (_PAGE_PRESENT | _PAGE_READ | _PAGE_WRITE | \ + (__HEXAGON_C_DEV << 6)) + +#define ioremap_uc(addr, size) ioremap((addr), (size)) #define __raw_writel writel diff --git a/arch/hexagon/mm/ioremap.c b/arch/hexagon/mm/ioremap.c deleted file mode 100644 index 255c5b1ee1a7..000000000000 --- a/arch/hexagon/mm/ioremap.c +++ /dev/null @@ -1,44 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-only -/* - * I/O remap functions for Hexagon - * - * Copyright (c) 2010-2011, The Linux Foundation. All rights reserved. - */ - -#include <linux/io.h> -#include <linux/vmalloc.h> -#include <linux/mm.h> - -void __iomem *ioremap(unsigned long phys_addr, unsigned long size) -{ - unsigned long last_addr, addr; - unsigned long offset = phys_addr & ~PAGE_MASK; - struct vm_struct *area; - - pgprot_t prot = __pgprot(_PAGE_PRESENT|_PAGE_READ|_PAGE_WRITE - |(__HEXAGON_C_DEV << 6)); - - last_addr = phys_addr + size - 1; - - /* Wrapping not allowed */ - if (!size || (last_addr < phys_addr)) - return NULL; - - /* Rounds up to next page size, including whole-page offset */ - size = PAGE_ALIGN(offset + size); - - area = get_vm_area(size, VM_IOREMAP); - addr = (unsigned long)area->addr; - - if (ioremap_page_range(addr, addr+size, phys_addr, prot)) { - vunmap((void *)addr); - return NULL; - } - - return (void __iomem *) (offset + addr); -} - -void iounmap(const volatile void __iomem *addr) -{ - vunmap((void *) ((unsigned long) addr & PAGE_MASK)); -}
With it, the old ioremap() and iounmap() can be perfectly removed since they are duplicated with the standard functions. Signed-off-by: Baoquan He <bhe@redhat.com> Cc: Brian Cain <bcain@quicinc.com> Cc: linux-hexagon@vger.kernel.org --- arch/hexagon/Kconfig | 1 + arch/hexagon/include/asm/io.h | 9 +++++-- arch/hexagon/mm/ioremap.c | 44 ----------------------------------- 3 files changed, 8 insertions(+), 46 deletions(-) delete mode 100644 arch/hexagon/mm/ioremap.c