| Message ID | 20230216123419.461016-2-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm: ioremap: Convert architectures to take GENERIC_IOREMAP way | expand |
On Thu, Feb 16, 2023, at 13:34, Baoquan He wrote: > diff --git a/arch/hexagon/include/asm/io.h > b/arch/hexagon/include/asm/io.h > index 46a099de85b7..dcd9cbbf5934 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)) I think we probably want to kill off ioremap_uc() here, and use the generic version that just returns NULL. I see that there are only two callers of {devm_,}ioremap_uc() left in the tree, so maybe we can even take that final step and remove it from the interface. Maybe we can revisit [1] as part of this series. Arnd [1] https://lore.kernel.org/all/20191111192258.2234502-1-arnd@arndb.de/
On 02/16/23 at 01:53pm, Arnd Bergmann wrote: > On Thu, Feb 16, 2023, at 13:34, Baoquan He wrote: > > diff --git a/arch/hexagon/include/asm/io.h > > b/arch/hexagon/include/asm/io.h > > index 46a099de85b7..dcd9cbbf5934 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)) > > + Thanks for reviewing. > > +#define ioremap_uc(addr, size) ioremap((addr), (size)) > > I think we probably want to kill off ioremap_uc() here, and use > the generic version that just returns NULL. -#define ioremap_uc(X, Y) ioremap((X), (Y)) +#define ioremap_uc(addr, size) ioremap((addr), (size)) Here, in fact the ioremap_uc() definition is not related. I just improve the old definition passingly. And similar for other ioremap_uc() adaptation. > > I see that there are only two callers of {devm_,}ioremap_uc() left in the > tree, so maybe we can even take that final step and remove it from > the interface. Maybe we can revisit [1] as part of this series. I see now. Christoph Hellwig ever mentioned in v2 reviewing, I didn't get why. Thanks for the details. https://lore.kernel.org/all/YwHX98KBEnZw9t6e@infradead.org/T/#u I am not sure if it's OK to do the change in this patchset, maybe another patch? Thanks Baoquan
On Thu, Feb 16, 2023, at 15:47, Baoquan He wrote: > On 02/16/23 at 01:53pm, Arnd Bergmann wrote: >> On Thu, Feb 16, 2023, at 13:34, Baoquan He wrote: >> I see that there are only two callers of {devm_,}ioremap_uc() left in the >> tree, so maybe we can even take that final step and remove it from >> the interface. Maybe we can revisit [1] as part of this series. > > I see now. Christoph Hellwig ever mentioned in v2 reviewing, I > didn't get why. Thanks for the details. > https://lore.kernel.org/all/YwHX98KBEnZw9t6e@infradead.org/T/#u > > I am not sure if it's OK to do the change in this patchset, maybe > another patch? Yes, a separate patch would be ideal. If you do the same change in more than one architecture (other than ia64 and x86), you can combine those into one patch. Arnd
On 02/16/23 at 03:51pm, Arnd Bergmann wrote: > On Thu, Feb 16, 2023, at 15:47, Baoquan He wrote: > > On 02/16/23 at 01:53pm, Arnd Bergmann wrote: > >> On Thu, Feb 16, 2023, at 13:34, Baoquan He wrote: > >> I see that there are only two callers of {devm_,}ioremap_uc() left in the > >> tree, so maybe we can even take that final step and remove it from > >> the interface. Maybe we can revisit [1] as part of this series. > > > > I see now. Christoph Hellwig ever mentioned in v2 reviewing, I > > didn't get why. Thanks for the details. > > https://lore.kernel.org/all/YwHX98KBEnZw9t6e@infradead.org/T/#u > > > > I am not sure if it's OK to do the change in this patchset, maybe > > another patch? > > Yes, a separate patch would be ideal. If you do the same change in > more than one architecture (other than ia64 and x86), you can combine > those into one patch. I am making a patch to achieve this. There's trouble in mips since it hasn't had <asm-generic/io.h> in <asm/io.h>, and adding that will cuase build error. I will post the patch for reviewing.
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 46a099de85b7..dcd9cbbf5934 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)); -}
By taking GENERIC_IOREMAP method, the generic ioremap_prot() and iounmap() are visible and available to arch. This change will simplify implementation by removing duplicated codes with generic ioremap_prot() and iounmap(), and has the equivalent functioality. For hexagon, the current ioremap() and iounmap() are the same as generic version. After taking GENERIC_IOREMAP way, the old ioremap() and iounmap() can be completely removed. 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