| Message ID | 20230301102208.148490-2-bhe@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] mips: add <asm-generic/io.h> including | expand |
On Wed, Mar 1, 2023, at 11:22, Baoquan He wrote: > 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. > > Signed-off-by: Baoquan He <bhe@redhat.com> > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > Cc: Huacai Chen <chenhuacai@kernel.org> > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > Cc: linux-mips@vger.kernel.org This looks good to me, Reviewed-by: Arnd Bergmann <arnd@arndb.de> 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. > @@ -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 I would prefer to put the #defines next to the function declarations, even when they come from macros. > > -#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
On 03/01/23 at 03:06pm, Arnd Bergmann wrote: > On Wed, Mar 1, 2023, at 11:22, Baoquan He wrote: > > 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. > > > > Signed-off-by: Baoquan He <bhe@redhat.com> > > Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> > > Cc: Huacai Chen <chenhuacai@kernel.org> > > Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> > > Cc: linux-mips@vger.kernel.org > > This looks good to me, > > Reviewed-by: Arnd Bergmann <arnd@arndb.de> > > 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. > > > @@ -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 > > I would prefer to put the #defines next to the function declarations, > even when they come from macros. Yeah, sounds reasonable, will change. > > > > > -#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. 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. ============= diff --git a/arch/mips/include/asm/mmiowb.h b/arch/mips/include/asm/mmiowb.h index a40824e3ef8e..cae2745935bc 100644 --- a/arch/mips/include/asm/mmiowb.h +++ b/arch/mips/include/asm/mmiowb.h @@ -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(); | ^~~~~~~~
On Thu, Mar 2, 2023, at 05:12, Baoquan He wrote: > On 03/01/23 at 03:06pm, Arnd Bergmann wrote: > > 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. > > ============= > diff --git a/arch/mips/include/asm/mmiowb.h b/arch/mips/include/asm/mmiowb.h > index a40824e3ef8e..cae2745935bc 100644 > --- a/arch/mips/include/asm/mmiowb.h > +++ b/arch/mips/include/asm/mmiowb.h > @@ -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 */ 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
On 03/02/23 at 08:12am, Arnd Bergmann wrote: > On Thu, Mar 2, 2023, at 05:12, Baoquan He wrote: > > On 03/01/23 at 03:06pm, Arnd Bergmann wrote: > > > > 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. > > > > ============= > > diff --git a/arch/mips/include/asm/mmiowb.h b/arch/mips/include/asm/mmiowb.h > > index a40824e3ef8e..cae2745935bc 100644 > > --- a/arch/mips/include/asm/mmiowb.h > > +++ b/arch/mips/include/asm/mmiowb.h > > @@ -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 */ > > 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. Ah, I didn't notice the comment. Then will change the definition to wmb(). Thanks.
On 03/02/23 at 08:12am, Arnd Bergmann wrote: > On Thu, Mar 2, 2023, at 05:12, Baoquan He wrote: > > On 03/01/23 at 03:06pm, Arnd Bergmann wrote: > > > > 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. > > > > ============= > > diff --git a/arch/mips/include/asm/mmiowb.h b/arch/mips/include/asm/mmiowb.h > > index a40824e3ef8e..cae2745935bc 100644 > > --- a/arch/mips/include/asm/mmiowb.h > > +++ b/arch/mips/include/asm/mmiowb.h > > @@ -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 */ > > 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. 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(); | ^~~~~~~~
diff --git a/arch/mips/include/asm/io.h b/arch/mips/include/asm/io.h index cec8347f0b85..3737b48f37dd 100644 --- a/arch/mips/include/asm/io.h +++ b/arch/mips/include/asm/io.h @@ -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_virt static 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_io static inline void memset_io(volatile void __iomem *addr, unsigned char val, int count) { memset((void __force *) addr, val, count); } +#define memcpy_fromio memcpy_fromio static inline void memcpy_fromio(void *dst, const volatile void __iomem *src, int count) { memcpy(dst, (void __force *) src, count); } +#define memcpy_toio memcpy_toio static 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 */ diff --git a/arch/mips/include/asm/mmiowb.h b/arch/mips/include/asm/mmiowb.h index a40824e3ef8e..007fe55bc7d1 100644 --- a/arch/mips/include/asm/mmiowb.h +++ b/arch/mips/include/asm/mmiowb.h @@ -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>
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. Signed-off-by: Baoquan He <bhe@redhat.com> Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: Huacai Chen <chenhuacai@kernel.org> Cc: Jiaxun Yang <jiaxun.yang@flygoat.com> Cc: linux-mips@vger.kernel.org --- arch/mips/include/asm/io.h | 46 ++++++++++++++++++++++++++++++++++ arch/mips/include/asm/mmiowb.h | 2 -- 2 files changed, 46 insertions(+), 2 deletions(-)