On Wednesday, September 14, 2016 10:16:28 PM CEST zhichang.yuan wrote: > > > > No need to guard includes with an #ifdef. > If remove #ifdef here, extio.h should not contain any function external declarations whose definitions are in > extio.c compiled only when CONFIG_ARM64_INDIRECT_PIO is yes. There is no problem with making declarations visible for functions that are not part of the kernel, we do that all the time. > >> +#define BUILDS_RW(bwl, type) \ > >> +static inline void reads##bwl(const volatile void __iomem *addr, \ > >> + void *buffer, unsigned int count) \ > >> +{ \ > >> + if (count) { \ > >> + type *buf = buffer; \ > >> + \ > >> + do { \ > >> + type x = __raw_read##bwl(addr); \ > >> + *buf++ = x; \ > >> + } while (--count); \ > >> + } \ > >> +} \ > >> + \ > >> +static inline void writes##bwl(volatile void __iomem *addr, \ > >> + const void *buffer, unsigned int count) \ > >> +{ \ > >> + if (count) { \ > >> + const type *buf = buffer; \ > >> + \ > >> + do { \ > >> + __raw_write##bwl(*buf++, addr); \ > >> + } while (--count); \ > >> + } \ > >> +} > >> + > >> +BUILDS_RW(b, u8) > > > > Why is this in here? > the readsb/writesb are defined in asm-generic/io.h which is included later, but the redefined insb/outsb need > to call them. Without these readsb/writesb definition before insb/outsb redefined, compile error occur. > > It seems that copy all the definitions of "asm-generic/io.h" is not a good idea, so I move the definitions of > those function needed here.... > > Ok. I think your idea below defining in(s)/out(s) in a c file can solve this issue. > > #ifdef CONFIG_ARM64_INDIRECT_PIO > #define inb inb > extern u8 inb(unsigned long addr); > > #define outb outb > extern void outb(u8 value, unsigned long addr); > > #define insb insb > extern void insb(unsigned long addr, void *buffer, unsigned int count); > > #define outsb outsb > extern void outsb(unsigned long addr, const void *buffer, unsigned int count); > #endif > > and definitions of all these functions are in extio.c : > > u8 inb(unsigned long addr) > { > if (!arm64_extio_ops || arm64_extio_ops->start > addr || > arm64_extio_ops->end < addr) > return readb(PCI_IOBASE + addr); > else > return arm64_extio_ops->pfin ? > arm64_extio_ops->pfin(arm64_extio_ops->devpara, > addr + arm64_extio_ops->ptoffset, NULL, > sizeof(u8), 1) : -1; > } > ..... Yes, sounds good. > >> @@ -149,6 +185,60 @@ static inline u64 __raw_readq(const volatile void __iomem *addr) > >> #define IO_SPACE_LIMIT (PCI_IO_SIZE - 1) > >> #define PCI_IOBASE ((void __iomem *)PCI_IO_START) > >> > >> + > >> +/* > >> + * redefine the in(s)b/out(s)b for indirect-IO. > >> + */ > >> +#define inb inb > >> +static inline u8 inb(unsigned long addr) > >> +{ > >> +#ifdef CONFIG_ARM64_INDIRECT_PIO > >> + if (arm64_extio_ops && arm64_extio_ops->start <= addr && > >> + addr <= arm64_extio_ops->end) > >> + return extio_inb(addr); > >> +#endif > >> + return readb(PCI_IOBASE + addr); > >> +} > >> + > > > > Looks ok, but you only seem to do this for the 8-bit > > accessors, when it should be done for 16-bit and 32-bit > > ones as well for consistency. > Hip06 LPC only support 8-bit I/O operations on the designated port. That is an interesting limitation. Maybe still call the extio operations and have them do WARN_ON_ONCE() instead? If you get a driver that calls inw/outw on the range that is owned by the LPC bus, you otherwise get an unhandled page fault in kernel space, which is not as nice. > >> diff --git a/drivers/bus/extio.c b/drivers/bus/extio.c > >> new file mode 100644 > >> index 0000000..1e7a9c5 > >> --- /dev/null > >> +++ b/drivers/bus/extio.c > >> @@ -0,0 +1,66 @@ > > > > This is in a globally visible directory > > > >> + > >> +struct extio_ops *arm64_extio_ops; > > > > But the identifier uses an architecture specific prefix. Either > > move the whole file into arch/arm64, or make the naming so that > > it can be used for everything. > > I perfer to move the whole file into arch/arm64, extio.h will be moved to arch/arm64/include/asm; Ok, that simplifies it a lot, you can just do everything in asm/io.h then. Arnd -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html