Hi, Arnd

On 2016/9/14 20:24, Arnd Bergmann wrote:

On Wednesday, September 14, 2016 8:15:51 PM CEST Zhichang Yuan wrote:

quoted

From: "zhichang.yuan" <yuanzhichang-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>

For arm64, there is no I/O space as other architectural platforms, such as
X86. Most I/O accesses are achieved based on MMIO. But for some arm64 SoCs,
such as Hip06, when accessing some legacy ISA devices connected to LPC, those
known port addresses are used to control the corresponding target devices, for
example, 0x2f8 is for UART, 0xe4 is for ipmi-bt. It is different from the
normal MMIO mode in using.

To drive these devices, this patch introduces a method named indirect-IO.
In this method the in/out pair in arch/arm64/include/asm/io.h will be
redefined. When upper layer drivers call in/out with those known legacy port
addresses to access the peripherals, the hooking functions corrresponding to
those target peripherals will be called. Through this way, those upper layer
drivers which depend on in/out can run on Hip06 without any changes.

Signed-off-by: zhichang.yuan <yuanzhichang-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>

Looks ok overall, but I have a couple of comments for details.

quoted

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index bc3f00f..9579479 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig

@@ -161,6 +161,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
 config ARCH_MMAP_RND_COMPAT_BITS_MAX
        default 16
 
+config ARM64_INDIRECT_PIO
+	def_bool n

'def_bool n' is the same as the shorter and more common 'bool'.

Yes. Will modify as bool "access peripherals with legacy I/O port"

quoted

diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
index 9b6e408..d3acf1f 100644
--- a/arch/arm64/include/asm/io.h
+++ b/arch/arm64/include/asm/io.h

@@ -34,6 +34,10 @@
 
 #include <xen/xen.h>
 
+#ifdef CONFIG_ARM64_INDIRECT_PIO
+#include <linux/extio.h>
+#endif

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.


How about removing everything about the configure item "ARM64_INDIRECT_PIO"?
This will make the indirect-IO mechanism global on ARM64.
I worry about this mechanism is not so common, so using "ARM64_INDIRECT_PIO" make this feature optional.

quoted

+#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;
}
.....

quoted

@@ -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.

quoted

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

quoted

+
+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;

quoted

+u8 __weak extio_inb(unsigned long addr)
+{
+	return arm64_extio_ops->pfin ?
+		arm64_extio_ops->pfin(arm64_extio_ops->devpara,
+			addr + arm64_extio_ops->ptoffset, NULL,
+			sizeof(u8), 1) : -1;
+}

No need for the __weak attribute, just make sure that the
code is always built-in when needed.

Also, it doesn't seem necessary to have an extern function if
all it does is call the one callback that you have already 
checked earlier. Either put it all into the inline
definition in asm/io.h, or put it all into the extern
version like this.

#ifdef CONFIG_ARM64_INDIRECT_PIO /* otherwise use default from asm-generic */
#define inb inb
extern u8 inb(unsigned long addr);
#endif

Yes. This is good!
Although the in(s)/out(s) are not inline anymore.

I had applied this way above.

u8 inb(unsigned long addr)
{
	if (arm64_extio_ops && arm64_extio_ops->start <= addr &&
			addr <= arm64_extio_ops->end)
		arm64_extio_ops->pfin(arm64_extio_ops->devpara,addr + arm64_extio_ops->ptoffset, NULL,sizeof(u8), 1) : -1;
	return extio_inb(addr);
}

quoted

+#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);
+}

quoted

diff --git a/include/linux/extio.h b/include/linux/extio.h
new file mode 100644
index 0000000..08d1fca
--- /dev/null
+++ b/include/linux/extio.h

@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
+ * Author: Zhichang Yuan <yuanzhichang-C8/M+/jPZTeaMJb+Lgu22Q@public.gmane.org>
+ * Author: Zou Rongrong <@huawei.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __LINUX_EXTIO_H
+#define __LINUX_EXTIO_H
+
+
+typedef u64 (*inhook)(void *devobj, unsigned long ptaddr, void *inbuf,
+				size_t dlen, unsigned int count);
+typedef void (*outhook)(void *devobj, unsigned long ptaddr,
+				const void *outbuf, size_t dlen,
+				unsigned int count);

I would drop the typedef and just declare the types directly in the
only place that references them.

Ok. Will apply it.

Thanks!
Zhichang

	Arnd

.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html