arch/*/io.h: remove ioremap_uc in some architectures
Subsystems (11)
v2 cover arch/*/io.h: remove ioremap_uc in some architectures
This patchset tries to remove ioremap_uc() in the current architectures except of x86 and ia64. They will use the default ioremap_uc definition in <asm-generic/io.h> which returns NULL.
If any arch sees a breakage caused by the default ioremap_uc(), it can provide a sepcific one for its own usage.
v1->v2:
- Update log of patch 2, and document related to ioremap_uc() according to Geert's comment.
- Add Geert's Acked-by.
Baoquan He (2):
mips: add <asm-generic/io.h> including arch/*/io.h: remove ioremap_uc in some architectures
Documentation/driver-api/device-io.rst | 11 ++++-- arch/alpha/include/asm/io.h | 1 - arch/hexagon/include/asm/io.h | 3 -- arch/m68k/include/asm/kmap.h | 1 - arch/mips/include/asm/io.h | 47 +++++++++++++++++++++++++- arch/mips/include/asm/mmiowb.h | 2 -- arch/parisc/include/asm/io.h | 2 -- arch/powerpc/include/asm/io.h | 1 - arch/sh/include/asm/io.h | 2 -- arch/sparc/include/asm/io_64.h | 1 - 10 files changed, 55 insertions(+), 16 deletions(-)
Signature · 1 line
2.34.1
PATCH v2 1/2 mips: add <asm-generic/io.h> including
With the adding, some default ioremap_xx methods defined in asm-generic/io.h can be used. E.g the default ioremap_uc() returning NULL.
Here, remove the <asm/io.h> including in asm/mmiowb.h, otherwise nested including will cause compiling error.
--- arch/mips/include/asm/io.h | 46 ++++++++++++++++++++++++++++++++++ arch/mips/include/asm/mmiowb.h | 2 -- 2 files changed, 46 insertions(+), 2 deletions(-)
arch/mips/include/asm/io.h +46 -0
@@ -126,6 +126,7 @@ static inline phys_addr_t virt_to_phys(const volatile void *x)* almost all conceivable cases a device driver should not be using * this function */ +#define phys_to_virt phys_to_virtstatic inline void *phys_to_virt (unsigned long address) {return __va (address);
@@ -480,14 +481,17 @@ BUILDSTRING(l, u32) BUILDSTRING(q, u64)#endif +#define memset_io memset_iostatic inline void memset_io (volatile void __iomem *addr,unsigned char val,int count) {memset ((void __force *) addr, val, count); } +#define memcpy_fromio memcpy_fromiostatic inline void memcpy_fromio (void *dst,const volatile void __iomem *src,int count) {memcpy (dst, (void __force *) src, count); } +#define memcpy_toio memcpy_toiostatic inline void memcpy_toio (volatile void __iomem *dst,const void *src,int count) {memcpy ((void __force *) dst, src, count);
@@ -548,6 +552,46 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size);#define csr_out32 (v, a) (*(volatile u32 *)((unsigned long)(a) + __CSR_32_ADJUST) = (v))#define csr_in32 (a) (*(volatile u32 *)((unsigned long)(a) + __CSR_32_ADJUST)) + +#define inb_p inb_p +#define inw_p inw_p +#define inl_p inl_p +#define insb insb +#define insw insw +#define insl insl + +#define outb_p outb_p +#define outw_p outw_p +#define outl_p outl_p +#define outsb outsb +#define outsw outsw +#define outsl outsl + +#define readb readb +#define readw readw +#define readl readl +#define writeb writeb +#define writew writew +#define writel writel + +#define readsb readsb +#define readsw readsw +#define readsl readsl +#define readsq readsq +#define writesb writesb +#define writesw writesw +#define writesl writesl +#define writesq writesq + +#define __raw_readb __raw_readb +#define __raw_readw __raw_readw +#define __raw_readl __raw_readl +#define __raw_readq __raw_readq +#define __raw_writeb __raw_writeb +#define __raw_writew __raw_writew +#define __raw_writel __raw_writel +#define __raw_writeq __raw_writeq +/* * Convert a physical pointer to a virtual kernel pointer for /dev/mem * access
@@ -556,4 +600,6 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size);void __ioread64_copy (void *to,const void __iomem *from,size_t count); +#include <asm-generic/io.h> +#endif /* _ASM_IO_H */
arch/mips/include/asm/mmiowb.h +0 -2
@@ -2,8 +2,6 @@#ifndef _ASM_MMIOWB_H#define _ASM_MMIOWB_H -#include <asm/io.h> -#define mmiowb () iobarrier_w()#include <asm-generic/mmiowb.h>
Signature · 1 line
2.34.1
Re: [PATCH v2 1/2] mips: add <asm-generic/io.h> including
Quoted 4 lines · Baoquan He
With the adding, some default
ioremap_xxmethods defined inasm-generic/io.hcan be used. E.g the defaultioremap_uc()returning NULL.
Quoted 8 lines
Here, remove the <
asm/io.h> including inasm/mmiowb.h, otherwise nested including will cause compiling error.Signed-off-by Baoquan He <[email protected]>Cc Thomas Bogendoerfer <[email protected]>Cc Huacai Chen <[email protected]>Cc Jiaxun Yang <[email protected]>
This looks good to me,
but it obviously needs to be properly reviewed by the MIPS maintainers as well. I think others have tried to do this in the past but did not make it in.
Quoted 4 lines
arch/mips/include/asm/io.h +0
-0@@ -548,6 +552,46 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size);#define csr_out32 (v, a) (*(volatile u32 *)((unsigned long)(a) + __CSR_32_ADJUST) = (v))
arch/mips/include/asm/io.h extern void (*_dma_cache_inv)(unsigned long start, · line 564
Quoted 10 lines
arch/mips/include/asm/io.h +7
-0#define csr_in32 (a) (*(volatile u32 *)((unsigned long)(a) + __CSR_32_ADJUST)) + +#define inb_p inb_p +#define inw_p inw_p +#define inl_p inl_p +#define insb insb +#define insw insw +#define insl insl
I would prefer to put the #defines next to the function declarations, even when they come from macros.
arch/mips/include/asm/mmiowb.h line 5
Quoted 5 lines
arch/mips/include/asm/mmiowb.h +0
-2-#include <asm/io.h> -#define mmiowb () iobarrier_w()
I think this only works as long as asm/spinlock.h also includes asm/io.h, otherwise linux/spinlock.h will be missing the iobarrier_w definition.
Most likely this is implicitly included from somewhere else below linux/spinlock.h, but it would be better not to rely on that, and instead define mmiowb() to wmb() directly.
Arnd
Re: [PATCH v2 1/2] mips: add <asm-generic/io.h> including
Quoted 14 lines · Arnd Bergmann
Quoted 4 lines · Baoquan He
With the adding, some default
ioremap_xxmethods defined inasm-generic/io.hcan be used. E.g the defaultioremap_uc()returning NULL.Quoted 8 lines
Here, remove the <
asm/io.h> including inasm/mmiowb.h, otherwise nested including will cause compiling error.Signed-off-by Baoquan He <[email protected]>Cc Thomas Bogendoerfer <[email protected]>Cc Huacai Chen <[email protected]>Cc Jiaxun Yang <[email protected]>
Quoted 7 lines
This looks good to me,
Reviewed-by Arnd Bergmann <[email protected]>but it obviously needs to be properly reviewed by the MIPS maintainers as well. I think others have tried to do this in the past but did not make it in.
Thanks for reviewing. Then let's wait for MIPS people to help check this.
Quoted 8 lines
Quoted 8 lines
(quoted diff) +0
-0@@ -548,6 +552,46 @@ extern void (*_dma_cache_inv)(unsigned long start, unsigned long size); #define csr_out32(v, a) (*(volatile u32 *)((unsigned long)(a) + __CSR_32_ADJUST) = (v)) #define csr_in32(a) (*(volatile u32 *)((unsigned long)(a) + __CSR_32_ADJUST))
Quoted 10 lines
Review extern void (*_dma_cache_inv)(unsigned long start, · line 564
Quoted 7 lines
(quoted diff) +7
-0+ +#define inb_p inb_p +#define inw_p inw_p +#define inl_p inl_p +#define insb insb +#define insw insw +#define insl inslI would prefer to put the #defines next to the function declarations, even when they come from macros.
Yeah, sounds reasonable, will change.
Quoted 11 lines
Review extern void (*_dma_cache_inv)(unsigned long start,
Quoted 6 lines
(quoted diff) +0
-2-#include <asm/io.h> - #define mmiowb() iobarrier_w()I think this only works as long as
asm/spinlock.halso includesasm/io.h, otherwiselinux/spinlock.hwill be missing theiobarrier_wdefinition.
Quoted 3 lines
Most likely this is implicitly included from somewhere else below
linux/spinlock.h, but it would be better not to rely on that, and instead definemmiowb()towmb()directly.
Yeah, defining mmiowb() to wmb() directly is also good to me. I tried to comb including sequence and find where asm/io.h is included, but failed. Mainly asm/mmiowb.h including asm/io.h will cause below compiling error, the asm/io.h need see mmiowb_set_pending which is defnined in asm-generic/mmiowb.h. Moving asm-generic/mmiowb.h to above asm/io.h can also fix the compiling error.
=============
arch/mips/include/asm/mmiowb.h +1 -3
@@ -2,10 +2,8 @@#ifndef _ASM_MMIOWB_H#define _ASM_MMIOWB_H +#include <asm-generic/mmiowb.h> #include <asm/io.h> #define mmiowb () iobarrier_w() - -#include <asm-generic/mmiowb.h> -#endif /* _ASM_MMIOWB_H */
============
CC arch/mips/kernel/asm-offsets.s
In file included from ./arch/mips/include/asm/io.h:602,
from ./arch/mips/include/asm/mmiowb.h:6,
from ./include/linux/spinlock.h:65,
from ./include/linux/ipc.h:5,
from ./include/uapi/linux/sem.h:5,
from ./include/linux/sem.h:5,
from ./include/linux/compat.h:14,
from arch/mips/kernel/asm-offsets.c:12:
./include/asm-generic/io.h: In function ‘_outb’:
./include/asm-generic/io.h:46:24: error: implicit declaration of function ‘mmiowb_set_pending’ [-Werror=implicit-function-declaration]
46 | #define __io_aw() mmiowb_set_pending()
| ^~~~~~~~~~~~~~~~~~
./include/asm-generic/io.h:54:24: note: in expansion of macro ‘__io_aw’
54 | #define __io_paw() __io_aw()
| ^~~~~~~
./include/asm-generic/io.h:585:9: note: in expansion of macro ‘__io_paw’
585 | __io_paw();
| ^~~~~~~~Arnd Bergmann
maintainerRe: [PATCH v2 1/2] mips: add <asm-generic/io.h> including
Quoted 16 lines · Baoquan He
On 03/01/23 at 03:06pm, Arnd Bergmann wrote:
Yeah, defining
mmiowb()towmb()directly is also good to me. I tried to comb including sequence and find whereasm/io.his included, but failed. Mainlyasm/mmiowb.hincludingasm/io.hwill cause below compiling error, theasm/io.hneed seemmiowb_set_pendingwhich is defnined inasm-generic/mmiowb.h. Movingasm-generic/mmiowb.hto aboveasm/io.hcan also fix the compiling error.=============
arch/mips/include/asm/mmiowb.h +0
-0@@ -2,10 +2,8 @@#ifndef _ASM_MMIOWB_H
arch/mips/include/asm/mmiowb.h line 9
Quoted 10 lines
arch/mips/include/asm/mmiowb.h +1
-3#define _ASM_MMIOWB_H +#include <asm-generic/mmiowb.h> #include <asm/io.h> #define mmiowb () iobarrier_w() - -#include <asm-generic/mmiowb.h> -#endif /* _ASM_MMIOWB_H */
According to the comment in asm-generic/mmiowb.h, the intention is to have the mmiowb definition before the #include, though this would only be necessary if there was an "#ifndef mmiowb" fallback in that file. If the definition to wmb() works, I'd go for that one and leave the include order unchanged.
Arnd
Re: [PATCH v2 1/2] mips: add <asm-generic/io.h> including
Quoted 22 lines · Arnd Bergmann
Quoted 21 lines · Baoquan He
On 03/01/23 at 03:06pm, Arnd Bergmann wrote:
Yeah, defining
mmiowb()towmb()directly is also good to me. I tried to comb including sequence and find whereasm/io.his included, but failed. Mainlyasm/mmiowb.hincludingasm/io.hwill cause below compiling error, theasm/io.hneed seemmiowb_set_pendingwhich is defnined inasm-generic/mmiowb.h. Movingasm-generic/mmiowb.hto aboveasm/io.hcan also fix the compiling error.=============
arch/mips/include/asm/mmiowb.h +1
-0@@ -2,10 +2,8 @@#ifndef _ASM_MMIOWB_H#define _ASM_MMIOWB_H +#include <asm-generic/mmiowb.h> #include <asm/io.h>
Quoted 11 lines
arch/mips/include/asm/mmiowb.h line 9
Quoted 5 lines
arch/mips/include/asm/mmiowb.h +0
-3#define mmiowb () iobarrier_w() - -#include <asm-generic/mmiowb.h> -#endif /* _ASM_MMIOWB_H */ According to the comment in
asm-generic/mmiowb.h, the intention is to have the mmiowb definition before the #include, though this would only be necessary if there was an "#ifndef mmiowb" fallback in that file. If the definition towmb()works, I'd go for that one and leave the include order unchanged.
Ah, I didn't notice the comment. Then will change the definition to wmb(). Thanks.
Re: [PATCH v2 1/2] mips: add <asm-generic/io.h> including
In reply to Arnd BergmannQuoted 22 lines · Arnd Bergmann
Quoted 21 lines · Baoquan He
On 03/01/23 at 03:06pm, Arnd Bergmann wrote:
Yeah, defining
mmiowb()towmb()directly is also good to me. I tried to comb including sequence and find whereasm/io.his included, but failed. Mainlyasm/mmiowb.hincludingasm/io.hwill cause below compiling error, theasm/io.hneed seemmiowb_set_pendingwhich is defnined inasm-generic/mmiowb.h. Movingasm-generic/mmiowb.hto aboveasm/io.hcan also fix the compiling error.=============
arch/mips/include/asm/mmiowb.h +1
-0@@ -2,10 +2,8 @@#ifndef _ASM_MMIOWB_H#define _ASM_MMIOWB_H +#include <asm-generic/mmiowb.h> #include <asm/io.h>
Quoted 11 lines
arch/mips/include/asm/mmiowb.h line 9
Quoted 5 lines
arch/mips/include/asm/mmiowb.h +0
-3#define mmiowb () iobarrier_w() - -#include <asm-generic/mmiowb.h> -#endif /* _ASM_MMIOWB_H */ According to the comment in
asm-generic/mmiowb.h, the intention is to have the mmiowb definition before the #include, though this would only be necessary if there was an "#ifndef mmiowb" fallback in that file. If the definition towmb()works, I'd go for that one and leave the include order unchanged.
I didn't add definition of outb(), so it caused the compiling error at below. Adding it can fix that. I will leave the asm/mmiowb.h as is since no issue now.
In file included from ./arch/mips/include/asm/io.h:611,
from ./arch/mips/include/asm/mmiowb.h:5,
from ./include/linux/spinlock.h:65,
from ./include/linux/ipc.h:5,
from ./include/uapi/linux/sem.h:5,
from ./include/linux/sem.h:5,
from ./include/linux/compat.h:14,
from arch/mips/kernel/asm-offsets.c:12:
./include/asm-generic/io.h: In function ‘_outb’:
./include/asm-generic/io.h:46:24: error: implicit declaration of function ‘mmiowb_set_pending’ [-Werror=implicit-function-declaration]
46 | #define __io_aw() mmiowb_set_pending()
| ^~~~~~~~~~~~~~~~~~
./include/asm-generic/io.h:54:24: note: in expansion of macro ‘__io_aw’
54 | #define __io_paw() __io_aw()
| ^~~~~~~
./include/asm-generic/io.h:585:9: note: in expansion of macro ‘__io_paw’
585 | __io_paw();
| ^~~~~~~~PATCH v2 2/2 arch/*/io.h: remove ioremap_uc in some architectures
In reply to Baoquan Heioremap_uc() is only meaningful on old x86-32 systems with the PAT extension, and on ia64 with its slightly unconventional ioremap() behavior, everywhere else this is the same as ioremap() anyway.
Here, remove the ioremap_uc() definition in architecutures other than x86 and ia64. These architectures all have asm-generic/io.h included and will have the default ioremap_uc() definition which returns NULL.
Note: This changes the existing behaviour and could break code calling ioremap_uc(). If any ARCH meets this breakage and really needs a specific ioremap_uc() for its own usage, one ioremap_uc() can be added in the ARCH.
--- Documentation/driver-api/device-io.rst | 11 +++++++++-- arch/alpha/include/asm/io.h | 1 - arch/hexagon/include/asm/io.h | 3 --- arch/m68k/include/asm/kmap.h | 1 - arch/mips/include/asm/io.h | 1 - arch/parisc/include/asm/io.h | 2 -- arch/powerpc/include/asm/io.h | 1 - arch/sh/include/asm/io.h | 2 -- arch/sparc/include/asm/io_64.h | 1 - 9 files changed, 9 insertions(+), 14 deletions(-)
Documentation/driver-api/device-io.rst +9 -2
@@ -408,9 +408,16 @@ functions for details on the CPU side of things. ioremap_uc() ------------ -ioremap_uc() behaves like ioremap() except that on the x86 architecture without +ioremap_uc() behaves like ioremap() except that on x86 architecture without 'PAT' mode, it marks memory as uncached even when the MTRR has designated -it as cacheable, see Documentation/x86/pat.rst. +it as cacheable, see Documentation/x86/pat.rst, and on ia64 which checks if +attributes don't match. + + +ioremap_uc() behaves like ioremap() except that on x86 and ia64 architectures. +X86 non-PAT system marks memory as uncached even when the MTRR has designated +it as cacheable in ioremap_uc()(see Documentation/x86/pat.rst). While ia64 +system firstly checks if attributes match ioremap_uc(), otherwise fails. Portable drivers should avoid the use of ioremap_uc().
arch/alpha/include/asm/io.h +0 -1
@@ -308,7 +308,6 @@ static inline void __iomem *ioremap(unsigned long port, unsigned long size) }#define ioremap_wc ioremap -#define ioremap_uc ioremapstatic inline void iounmap (volatile void __iomem *addr) {
arch/hexagon/include/asm/io.h +0 -3
@@ -176,9 +176,6 @@ static inline void writel(u32 data, volatile void __iomem *addr)#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 static inline void memcpy_fromio(void *dst, const volatile void __iomem *src,
arch/m68k/include/asm/kmap.h +0 -1
@@ -25,7 +25,6 @@ static inline void __iomem *ioremap(unsigned long physaddr, unsigned long size)return __ioremap (physaddr, size,IOMAP_NOCACHE_SER ); } -#define ioremap_uc ioremap#define ioremap_wt ioremap_wtstatic inline void __iomem *ioremap_wt (unsigned long physaddr,unsigned long size)
arch/mips/include/asm/io.h +0 -1
@@ -167,7 +167,6 @@ void iounmap(const volatile void __iomem *addr);*/ #define ioremap (offset, size) \ ioremap_prot((offset), (size), _CACHE_UNCACHED) -#define ioremap_uc ioremap/* * ioremap_cache - map bus memory into CPU space
arch/parisc/include/asm/io.h +0 -2
@@ -132,8 +132,6 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr)#define ioremap_wc (addr, size) \ ioremap_prot((addr), (size), _PAGE_IOREMAP) -#define ioremap_uc (addr, size) \ - ioremap_prot((addr), (size), _PAGE_IOREMAP)#define pci_iounmap pci_iounmap
arch/powerpc/include/asm/io.h +0 -1
@@ -863,7 +863,6 @@ void __iomem *ioremap_wt(phys_addr_t address, unsigned long size);#endif void __iomem *ioremap_coherent (phys_addr_t address,unsigned long size); -#define ioremap_uc (addr, size) ioremap((addr), (size))#define ioremap_cache (addr, size) \ ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
arch/sh/include/asm/io.h +0 -2
@@ -278,8 +278,6 @@ unsigned long long poke_real_address_q(unsigned long long addr, ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))#endif /* CONFIG_MMU */ -#define ioremap_uc ioremap -/* * Convert a physical pointer to a virtual kernel pointer for /dev/mem * access
arch/sparc/include/asm/io_64.h +0 -1
@@ -423,7 +423,6 @@ static inline void __iomem *ioremap(unsigned long offset, unsigned long size)return (void __iomem *)offset; } -#define ioremap_uc (X ,Y ) ioremap((X),(Y))#define ioremap_wc (X ,Y ) ioremap((X),(Y))#define ioremap_wt (X ,Y ) ioremap((X),(Y))static inline void __iomem *ioremap_np (unsigned long offset,unsigned long size)
Signature · 1 line
2.34.1
Quoted 6 lines · Baoquan He
This patchset tries to remove
ioremap_uc()in the current architectures except of x86 and ia64. They will use the defaultioremap_ucdefinition in <asm-generic/io.h> which returns NULL.If any arch sees a breakage caused by the default
ioremap_uc(), it can provide a sepcific one for its own usage.
Forgot mentioning this patchset is based on below patchset.
[PATCH v5 00/17] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way
https://lore.kernel.org/all/[email protected]/T/#u
Quoted 25 lines
v1->v2:
- Update log of patch 2, and document related toioremap_uc()according to Geert's comment.
- Add Geert's Acked-by.Baoquan He (2):
mips: add <asm-generic/io.h> includingarch/*/io.h: removeioremap_ucin some architecturesDocumentation/driver-api/device-io.rst | 11 ++++-- arch/alpha/include/asm/io.h | 1 - arch/hexagon/include/asm/io.h | 3 -- arch/m68k/include/asm/kmap.h | 1 - arch/mips/include/asm/io.h | 47 +++++++++++++++++++++++++- arch/mips/include/asm/mmiowb.h | 2 -- arch/parisc/include/asm/io.h | 2 -- arch/powerpc/include/asm/io.h | 1 - arch/sh/include/asm/io.h | 2 -- arch/sparc/include/asm/io_64.h | 1 - 10 files changed, 55 insertions(+), 16 deletions(-)Signature · 1 line
2.34.1
Quoted 6 lines · Baoquan He
This patchset tries to remove
ioremap_uc()in the current architectures except of x86 and ia64. They will use the defaultioremap_ucdefinition in <asm-generic/io.h> which returns NULL.If any arch sees a breakage caused by the default
ioremap_uc(), it can provide a sepcific one for its own usage.
Feel free to add:
Luis
Quoted 11 lines · Luis Chamberlain
Quoted 6 lines · Baoquan He
This patchset tries to remove
ioremap_uc()in the current architectures except of x86 and ia64. They will use the defaultioremap_ucdefinition in <asm-generic/io.h> which returns NULL.If any arch sees a breakage caused by the default
ioremap_uc(), it can provide a sepcific one for its own usage.Feel free to add:
Reviewed-by Luis Chamberlain <[email protected]>
Thanks for check. I made some changes according to comments, will repost. Please help review.