[V5,1/3] ARM64 LPC: Indirect ISA port IO introduced

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.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will.deacon@arm.com>
Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 arch/arm64/Kconfig             |  6 +++
 arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
 arch/arm64/include/asm/io.h    | 29 +++++++++++++
 arch/arm64/kernel/Makefile     |  1 +
 arch/arm64/kernel/extio.c      | 27 ++++++++++++
 5 files changed, 157 insertions(+)
 create mode 100644 arch/arm64/include/asm/extio.h
 create mode 100644 arch/arm64/kernel/extio.c

On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> 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.

This has nothing to do with arm64. Hardware with this kind of indirect
bus access could be integrated with a variety of CPU architectures. It
simply hasn't been, yet.

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

As above, this has nothing to do with arm64, and as such, should live in
generic code, exactly as we would do if we had higher-level ISA
accessor ops.

Regardless, given the multi-instance case, I don't think this is
sufficient in general (and I think we need higher-level ISA accessors
to handle the indirection).

[...]

> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> new file mode 100644
> index 0000000..6ae0787
> --- /dev/null
> +++ b/arch/arm64/include/asm/extio.h

> +#ifndef __LINUX_EXTIO_H
> +#define __LINUX_EXTIO_H

This doesn't match the file naming, __ASM_EXTIO_H would be consistent
with other arm64 headers.

> +
> +struct extio_ops {
> +	unsigned long start;/* inclusive, sys io addr */
> +	unsigned long end;/* inclusive, sys io addr */

Please put whitespace before inline comments.

[...]

> +type in##bw(unsigned long addr)						\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		return read##bw(PCI_IOBASE + addr);			\
> +	return arm64_extio_ops->pfin ?					\
> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> +			addr, sizeof(type)) : -1;			\
> +}									\
> +									\
> +void out##bw(type value, unsigned long addr)				\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		write##bw(value, PCI_IOBASE + addr);			\
> +	else								\
> +		if (arm64_extio_ops->pfout)				\
> +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> +				addr, value, sizeof(type));		\
> +}									\
> +									\
> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfins)				\
> +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}									\
> +									\
> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfouts)				\
> +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}
> +

So all PCI I/O will be slowed down by irrelevant checks when this is
enabled?

[...]

> +static inline void arm64_set_extops(struct extio_ops *ops)
> +{
> +	if (ops)
> +		WRITE_ONCE(arm64_extio_ops, ops);
> +}

Why WRITE_ONCE()?

Is this not protected/propagated by some synchronisation mechanism?

WRITE_ONCE() is not sufficient to ensure that this is consistently
observed by readers, and regardless, I don't see READ_ONCE() anywhere in
this patch.

This looks very suspicious.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tuesday, November 8, 2016 12:03:23 PM CET Mark Rutland wrote:
> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> > 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.
> 
> This has nothing to do with arm64. Hardware with this kind of indirect
> bus access could be integrated with a variety of CPU architectures. It
> simply hasn't been, yet.

Actually PowerPC has a vaguely similar mechanism.

> > 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.
> 
> As above, this has nothing to do with arm64, and as such, should live in
> generic code, exactly as we would do if we had higher-level ISA
> accessor ops.
> 
> Regardless, given the multi-instance case, I don't think this is
> sufficient in general (and I think we need higher-level ISA accessors
> to handle the indirection).

I think it is rather unlikely that we have to deal with multiple
instances in the future, it's more likely that future platforms
won't have any I/O ports at all, which is why I was advocating for
simplicity here.

> > +type in##bw(unsigned long addr)						\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		return read##bw(PCI_IOBASE + addr);			\
> > +	return arm64_extio_ops->pfin ?					\
> > +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> > +			addr, sizeof(type)) : -1;			\
> > +}									\
> > +									\
> > +void out##bw(type value, unsigned long addr)				\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		write##bw(value, PCI_IOBASE + addr);			\
> > +	else								\
> > +		if (arm64_extio_ops->pfout)				\
> > +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > +				addr, value, sizeof(type));		\
> > +}									\
> > +									\
> > +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> > +	else								\
> > +		if (arm64_extio_ops->pfins)				\
> > +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> > +				addr, buffer, sizeof(type), count);	\
> > +}									\
> > +									\
> > +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> > +{									\
> > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > +			arm64_extio_ops->end < addr)			\
> > +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> > +	else								\
> > +		if (arm64_extio_ops->pfouts)				\
> > +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> > +				addr, buffer, sizeof(type), count);	\
> > +}
> > +
> 
> So all PCI I/O will be slowed down by irrelevant checks when this is
> enabled?

I don't see a better alternative. I earlier suggested having these
out of line so we don't grow the object code too much when it is
enabled.

Performance of PIO accessors is not an issue here though, any bus
access will by definition be orders of magnitude slower than the
added branches and dereferences here.

> [...]
> 
> > +static inline void arm64_set_extops(struct extio_ops *ops)
> > +{
> > +	if (ops)
> > +		WRITE_ONCE(arm64_extio_ops, ops);
> > +}
> 
> Why WRITE_ONCE()?
> 
> Is this not protected/propagated by some synchronisation mechanism?
> 
> WRITE_ONCE() is not sufficient to ensure that this is consistently
> observed by readers, and regardless, I don't see READ_ONCE() anywhere in
> this patch.
> 
> This looks very suspicious.

Agreed, this looks wrong.

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

On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> 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.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  arch/arm64/Kconfig             |  6 +++
>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>  arch/arm64/kernel/Makefile     |  1 +
>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>  5 files changed, 157 insertions(+)
>  create mode 100644 arch/arm64/include/asm/extio.h
>  create mode 100644 arch/arm64/kernel/extio.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 969ef88..b44070b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>         default 16
>  
> +config ARM64_INDIRECT_PIO
> +	bool "access peripherals with legacy I/O port"
> +	help
> +	  Support special accessors for ISA I/O devices. This is needed for
> +	  SoCs that do not support standard read/write for the ISA range.
> +
>  config NO_IOPORT_MAP
>  	def_bool y if !PCI
>  
> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> new file mode 100644
> index 0000000..6ae0787
> --- /dev/null
> +++ b/arch/arm64/include/asm/extio.h
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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
> +
> +struct extio_ops {
> +	unsigned long start;/* inclusive, sys io addr */
> +	unsigned long end;/* inclusive, sys io addr */
> +
> +	u64 (*pfin)(void *devobj, unsigned long ptaddr,	size_t dlen);
> +	void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
> +					size_t dlen);
> +	u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
> +				size_t dlen, unsigned int count);
> +	void (*pfouts)(void *devobj, unsigned long ptaddr,
> +				const void *outbuf, size_t dlen,
> +				unsigned int count);
> +	void *devpara;
> +};
> +
> +extern struct extio_ops *arm64_extio_ops;
> +
> +#define DECLARE_EXTIO(bw, type)						\
> +extern type in##bw(unsigned long addr);					\
> +extern void out##bw(type value, unsigned long addr);			\
> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
> +
> +#define BUILD_EXTIO(bw, type)						\
> +type in##bw(unsigned long addr)						\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		return read##bw(PCI_IOBASE + addr);			\
> +	return arm64_extio_ops->pfin ?					\
> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> +			addr, sizeof(type)) : -1;			\
> +}									\
> +									\
> +void out##bw(type value, unsigned long addr)				\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		write##bw(value, PCI_IOBASE + addr);			\
> +	else								\
> +		if (arm64_extio_ops->pfout)				\
> +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> +				addr, value, sizeof(type));		\
> +}									\
> +									\
> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfins)				\
> +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}									\
> +									\
> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> +{									\
> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> +			arm64_extio_ops->end < addr)			\
> +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> +	else								\
> +		if (arm64_extio_ops->pfouts)				\
> +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> +				addr, buffer, sizeof(type), count);	\
> +}
> +
> +static inline void arm64_set_extops(struct extio_ops *ops)
> +{
> +	if (ops)
> +		WRITE_ONCE(arm64_extio_ops, ops);

Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
side. Also, what if multiple drivers want to set different ops for distinct
address ranges?

> +}
> +
> +#endif /* __LINUX_EXTIO_H*/
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 0bba427..136735d 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -31,6 +31,7 @@
>  #include <asm/early_ioremap.h>
>  #include <asm/alternative.h>
>  #include <asm/cpufeature.h>
> +#include <asm/extio.h>
>  
>  #include <xen/xen.h>
>  
> @@ -149,6 +150,34 @@ 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.
> + */
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +#define inb inb
> +#define outb outb
> +#define insb insb
> +#define outsb outsb
> +/* external declaration */
> +DECLARE_EXTIO(b, u8)
> +
> +#define inw inw
> +#define outw outw
> +#define insw insw
> +#define outsw outsw
> +
> +DECLARE_EXTIO(w, u16)
> +
> +#define inl inl
> +#define outl outl
> +#define insl insl
> +#define outsl outsl
> +
> +DECLARE_EXTIO(l, u32)
> +#endif
> +
> +
>  /*
>   * String version of I/O memory access operations.
>   */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 7d66bba..60e0482 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
>  					   sys_compat.o entry32.o
>  arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>  arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO)	+= extio.o
>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
>  arm64-obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> new file mode 100644
> index 0000000..647b3fa
> --- /dev/null
> +++ b/arch/arm64/kernel/extio.c
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
> + */
> +
> +#include <linux/io.h>
> +
> +struct extio_ops *arm64_extio_ops;
> +
> +
> +BUILD_EXTIO(b, u8)
> +
> +BUILD_EXTIO(w, u16)
> +
> +BUILD_EXTIO(l, u32)

Is there no way to make this slightly more generic, so that it can be
re-used elsewhere? For example, if struct extio_ops was common, then
you could have the singleton (which maybe should be an interval tree?),
type definition, setter function and the BUILD_EXTIO invocations
somewhere generic, rather than squirelled away in the arch backend.

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

On Tuesday, November 8, 2016 5:09:59 PM CET Arnd Bergmann wrote:
> 
> I don't see a better alternative. I earlier suggested having these
> out of line so we don't grow the object code too much when it is
> enabled.
> 

On second look, I see that they are all done out of line, I would
just move around the BUILD_EXTIO macro to the file that uses it
and remove and open-code the DECLARE_EXTIO() as that makes it easier
to grep.

	Arnd

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

On 08/11/2016 16:12, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>> 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.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> ---
>>  arch/arm64/Kconfig             |  6 +++
>>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>>  arch/arm64/kernel/Makefile     |  1 +
>>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>>  5 files changed, 157 insertions(+)
>>  create mode 100644 arch/arm64/include/asm/extio.h
>>  create mode 100644 arch/arm64/kernel/extio.c
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 969ef88..b44070b 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>>         default 16
>>
>> +config ARM64_INDIRECT_PIO
>> +	bool "access peripherals with legacy I/O port"
>> +	help
>> +	  Support special accessors for ISA I/O devices. This is needed for
>> +	  SoCs that do not support standard read/write for the ISA range.
>> +
>>  config NO_IOPORT_MAP
>>  	def_bool y if !PCI
>>
>> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
>> new file mode 100644
>> index 0000000..6ae0787
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/extio.h
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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
>> +
>> +struct extio_ops {
>> +	unsigned long start;/* inclusive, sys io addr */
>> +	unsigned long end;/* inclusive, sys io addr */
>> +
>> +	u64 (*pfin)(void *devobj, unsigned long ptaddr,	size_t dlen);
>> +	void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
>> +					size_t dlen);
>> +	u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
>> +				size_t dlen, unsigned int count);
>> +	void (*pfouts)(void *devobj, unsigned long ptaddr,
>> +				const void *outbuf, size_t dlen,
>> +				unsigned int count);
>> +	void *devpara;
>> +};
>> +
>> +extern struct extio_ops *arm64_extio_ops;
>> +
>> +#define DECLARE_EXTIO(bw, type)						\
>> +extern type in##bw(unsigned long addr);					\
>> +extern void out##bw(type value, unsigned long addr);			\
>> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
>> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
>> +
>> +#define BUILD_EXTIO(bw, type)						\
>> +type in##bw(unsigned long addr)						\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		return read##bw(PCI_IOBASE + addr);			\
>> +	return arm64_extio_ops->pfin ?					\
>> +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
>> +			addr, sizeof(type)) : -1;			\
>> +}									\
>> +									\
>> +void out##bw(type value, unsigned long addr)				\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		write##bw(value, PCI_IOBASE + addr);			\
>> +	else								\
>> +		if (arm64_extio_ops->pfout)				\
>> +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>> +				addr, value, sizeof(type));		\
>> +}									\
>> +									\
>> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
>> +	else								\
>> +		if (arm64_extio_ops->pfins)				\
>> +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
>> +				addr, buffer, sizeof(type), count);	\
>> +}									\
>> +									\
>> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
>> +{									\
>> +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
>> +			arm64_extio_ops->end < addr)			\
>> +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
>> +	else								\
>> +		if (arm64_extio_ops->pfouts)				\
>> +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
>> +				addr, buffer, sizeof(type), count);	\
>> +}
>> +
>> +static inline void arm64_set_extops(struct extio_ops *ops)
>> +{
>> +	if (ops)
>> +		WRITE_ONCE(arm64_extio_ops, ops);
>
> Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
> side. Also, what if multiple drivers want to set different ops for distinct
> address ranges?

I think that the idea here is that we only have possibly one master in 
the system which offers indirectIO backend, so another one could not 
possibly re-set this value.

>
>> +}
>> +
>> +#endif /* __LINUX_EXTIO_H*/
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 0bba427..136735d 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -31,6 +31,7 @@
>>  #include <asm/early_ioremap.h>
>>  #include <asm/alternative.h>
>>  #include <asm/cpufeature.h>
>> +#include <asm/extio.h>
>>
>>  #include <xen/xen.h>
>>
>> @@ -149,6 +150,34 @@ 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.
>> + */
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +#define inb inb
>> +#define outb outb
>> +#define insb insb
>> +#define outsb outsb
>> +/* external declaration */
>> +DECLARE_EXTIO(b, u8)
>> +
>> +#define inw inw
>> +#define outw outw
>> +#define insw insw
>> +#define outsw outsw
>> +
>> +DECLARE_EXTIO(w, u16)
>> +
>> +#define inl inl
>> +#define outl outl
>> +#define insl insl
>> +#define outsl outsl
>> +
>> +DECLARE_EXTIO(l, u32)
>> +#endif
>> +
>> +
>>  /*
>>   * String version of I/O memory access operations.
>>   */
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 7d66bba..60e0482 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)		+= sys32.o kuser32.o signal32.o 	\
>>  					   sys_compat.o entry32.o
>>  arm64-obj-$(CONFIG_FUNCTION_TRACER)	+= ftrace.o entry-ftrace.o
>>  arm64-obj-$(CONFIG_MODULES)		+= arm64ksyms.o module.o
>> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO)	+= extio.o
>>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)	+= module-plts.o
>>  arm64-obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o perf_callchain.o
>>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)	+= perf_event.o
>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>> new file mode 100644
>> index 0000000..647b3fa
>> --- /dev/null
>> +++ b/arch/arm64/kernel/extio.c
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
>> + */
>> +
>> +#include <linux/io.h>
>> +
>> +struct extio_ops *arm64_extio_ops;
>> +
>> +
>> +BUILD_EXTIO(b, u8)
>> +
>> +BUILD_EXTIO(w, u16)
>> +
>> +BUILD_EXTIO(l, u32)
>
> Is there no way to make this slightly more generic, so that it can be
> re-used elsewhere? For example, if struct extio_ops was common, then
> you could have the singleton (which maybe should be an interval tree?),
> type definition, setter function and the BUILD_EXTIO invocations
> somewhere generic, rather than squirelled away in the arch backend.
>
> Will

The concern would be that some architecture which uses generic 
higher-level ISA accessor ops, but have IO space, could be affected.

John

>
> .
>


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

On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> On 08/11/2016 16:12, Will Deacon wrote:
> >On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> >>+static inline void arm64_set_extops(struct extio_ops *ops)
> >>+{
> >>+	if (ops)
> >>+		WRITE_ONCE(arm64_extio_ops, ops);
> >
> >Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
> >side. Also, what if multiple drivers want to set different ops for distinct
> >address ranges?
> 
> I think that the idea here is that we only have possibly one master in the
> system which offers indirectIO backend, so another one could not possibly
> re-set this value.

Why is that assumption valid, and why does WRITE_ONCE help there? It's not
ONCE as in WARN_ONCE, more ONCE as in exactly-once-per-invocation.

> >>diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> >>new file mode 100644
> >>index 0000000..647b3fa
> >>--- /dev/null
> >>+++ b/arch/arm64/kernel/extio.c
> >>@@ -0,0 +1,27 @@
> >>+/*
> >>+ * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> >>+ * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
> >>+ */
> >>+
> >>+#include <linux/io.h>
> >>+
> >>+struct extio_ops *arm64_extio_ops;
> >>+
> >>+
> >>+BUILD_EXTIO(b, u8)
> >>+
> >>+BUILD_EXTIO(w, u16)
> >>+
> >>+BUILD_EXTIO(l, u32)
> >
> >Is there no way to make this slightly more generic, so that it can be
> >re-used elsewhere? For example, if struct extio_ops was common, then
> >you could have the singleton (which maybe should be an interval tree?),
> >type definition, setter function and the BUILD_EXTIO invocations
> >somewhere generic, rather than squirelled away in the arch backend.
> >
> The concern would be that some architecture which uses generic higher-level
> ISA accessor ops, but have IO space, could be affected.

You're already adding a Kconfig symbol for this stuff, so you can keep
that if you don't want it on other architectures. I'm just arguing that
plumbing drivers directly into arch code via arm64_set_extops is not
something I'm particularly fond of, especially when it looks like it
could be avoided with a small amount of effort.

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

On 08/11/2016 16:49, Will Deacon wrote:
> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
>> On 08/11/2016 16:12, Will Deacon wrote:
>>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>>>> +static inline void arm64_set_extops(struct extio_ops *ops)
>>>> +{
>>>> +	if (ops)
>>>> +		WRITE_ONCE(arm64_extio_ops, ops);
>>>
>>> Why does this need to be WRITE_ONCE? You don't have READ_ONCE on the reader
>>> side. Also, what if multiple drivers want to set different ops for distinct
>>> address ranges?
>>
>> I think that the idea here is that we only have possibly one master in the
>> system which offers indirectIO backend, so another one could not possibly
>> re-set this value.
>
> Why is that assumption valid, and why does WRITE_ONCE help there? It's not
> ONCE as in WARN_ONCE, more ONCE as in exactly-once-per-invocation.

It's only valid based on the inherent assumption that all indirectIO is 
redirected to one backend master, i.e. LPC driver.

Anyway, right, I don't think that WRITE_ONCE is correct. Zhichang was 
looking for something which would only allow the pointer to be written 
once ever.

>
>>>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>>>> new file mode 100644
>>>> index 0000000..647b3fa
>>>> --- /dev/null
>>>> +++ b/arch/arm64/kernel/extio.c
>>>> @@ -0,0 +1,27 @@
>>>> +/*
>>>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>>>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
>>>> + */
>>>> +
>>>> +#include <linux/io.h>
>>>> +
>>>> +struct extio_ops *arm64_extio_ops;
>>>> +
>>>> +
>>>> +BUILD_EXTIO(b, u8)
>>>> +
>>>> +BUILD_EXTIO(w, u16)
>>>> +
>>>> +BUILD_EXTIO(l, u32)
>>>
>>> Is there no way to make this slightly more generic, so that it can be
>>> re-used elsewhere? For example, if struct extio_ops was common, then
>>> you could have the singleton (which maybe should be an interval tree?),
>>> type definition, setter function and the BUILD_EXTIO invocations
>>> somewhere generic, rather than squirelled away in the arch backend.
>>>
>> The concern would be that some architecture which uses generic higher-level
>> ISA accessor ops, but have IO space, could be affected.
>
> You're already adding a Kconfig symbol for this stuff, so you can keep
> that if you don't want it on other architectures. I'm just arguing that
> plumbing drivers directly into arch code via arm64_set_extops is not
> something I'm particularly fond of, especially when it looks like it
> could be avoided with a small amount of effort.

We'll check this.

Cheers,
John

>
> Will
>
> .
>


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

On Tuesday, November 8, 2016 4:49:49 PM CET Will Deacon wrote:
> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> > On 08/11/2016 16:12, Will Deacon wrote:
> > >On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:

> > >Is there no way to make this slightly more generic, so that it can be
> > >re-used elsewhere? For example, if struct extio_ops was common, then
> > >you could have the singleton (which maybe should be an interval tree?),
> > >type definition, setter function and the BUILD_EXTIO invocations
> > >somewhere generic, rather than squirelled away in the arch backend.
> > >
> > The concern would be that some architecture which uses generic higher-level
> > ISA accessor ops, but have IO space, could be affected.
> 
> You're already adding a Kconfig symbol for this stuff, so you can keep
> that if you don't want it on other architectures. I'm just arguing that
> plumbing drivers directly into arch code via arm64_set_extops is not
> something I'm particularly fond of, especially when it looks like it
> could be avoided with a small amount of effort.

Agreed, I initially suggested putting this into arch/arm64/, but there isn't
really a reason why it couldn't just live in lib/ with the header file
bits moved to include/asm-generic/io.h which we already use.

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

On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> > 
> > 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.
> 
> This has nothing to do with arm64. Hardware with this kind of indirect
> bus access could be integrated with a variety of CPU architectures. It
> simply hasn't been, yet.

On some ppc's we also use similar indirect access methods for IOs. We
have a generic infrastructure for re-routing some memory or IO regions
to hooks.

On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
firmware calls ;-) We use that infrastructure to plumb in the LPC bus.

> > 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.
> 
> As above, this has nothing to do with arm64, and as such, should live in
> generic code, exactly as we would do if we had higher-level ISA
> accessor ops.
> 
> Regardless, given the multi-instance case, I don't think this is
> sufficient in general (and I think we need higher-level ISA accessors
> to handle the indirection).

Multi-instance with IO is tricky to do generically because archs already
have all sort of hacks to deal with the fact that inb/outb don't require
an explicit ioremap, so an IO resource can take all sort of shape depending
on the arch.

Overall it boils down to applying some kind of per-instance "offset" to
the IO port number though.

> [...]
> 
> > 
> > diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> > new file mode 100644
> > index 0000000..6ae0787
> > --- /dev/null
> > +++ b/arch/arm64/include/asm/extio.h
> 
> > 
> > +#ifndef __LINUX_EXTIO_H
> > +#define __LINUX_EXTIO_H
> 
> This doesn't match the file naming, __ASM_EXTIO_H would be consistent
> with other arm64 headers.
> 
> > 
> > +
> > +struct extio_ops {
> > > > +	unsigned long start;/* inclusive, sys io addr */
> > > > +	unsigned long end;/* inclusive, sys io addr */
> 
> Please put whitespace before inline comments.
> 
> [...]
> 
> > 
> > > > +type in##bw(unsigned long addr)						\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		return read##bw(PCI_IOBASE + addr);			\
> > > > > > +	return arm64_extio_ops->pfin ?					\
> > > > > > +		arm64_extio_ops->pfin(arm64_extio_ops->devpara,		\
> > > > > > +			addr, sizeof(type)) : -1;			\
> > > > +}									\
> > > > +									\
> > > > +void out##bw(type value, unsigned long addr)				\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		write##bw(value, PCI_IOBASE + addr);			\
> > > > > > +	else								\
> > > > > > +		if (arm64_extio_ops->pfout)				\
> > > > +			arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> > > > > > +				addr, value, sizeof(type));		\
> > > > +}									\
> > > > +									\
> > > > +void ins##bw(unsigned long addr, void *buffer, unsigned int count)	\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		reads##bw(PCI_IOBASE + addr, buffer, count);		\
> > > > > > +	else								\
> > > > > > +		if (arm64_extio_ops->pfins)				\
> > > > +			arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> > > > > > +				addr, buffer, sizeof(type), count);	\
> > > > +}									\
> > > > +									\
> > > > +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)	\
> > > > +{									\
> > > > > > +	if (!arm64_extio_ops || arm64_extio_ops->start > addr ||	\
> > > > > > +			arm64_extio_ops->end < addr)			\
> > > > > > +		writes##bw(PCI_IOBASE + addr, buffer, count);		\
> > > > > > +	else								\
> > > > > > +		if (arm64_extio_ops->pfouts)				\
> > > > +			arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> > > > > > +				addr, buffer, sizeof(type), count);	\
> > +}
> > +
> 
> So all PCI I/O will be slowed down by irrelevant checks when this is
> enabled?
> 
> [...]
> 
> > 
> > +static inline void arm64_set_extops(struct extio_ops *ops)
> > +{
> > > > +	if (ops)
> > > > +		WRITE_ONCE(arm64_extio_ops, ops);
> > +}
> 
> Why WRITE_ONCE()?
> 
> Is this not protected/propagated by some synchronisation mechanism?
> 
> WRITE_ONCE() is not sufficient to ensure that this is consistently
> observed by readers, and regardless, I don't see READ_ONCE() anywhere in
> this patch.
> 
> This looks very suspicious.
> 
> Thanks,
> Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 08/11/2016 22:35, Arnd Bergmann wrote:
> On Tuesday, November 8, 2016 4:49:49 PM CET Will Deacon wrote:
>> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
>>> On 08/11/2016 16:12, Will Deacon wrote:
>>>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>
>>>> Is there no way to make this slightly more generic, so that it can be
>>>> re-used elsewhere? For example, if struct extio_ops was common, then
>>>> you could have the singleton (which maybe should be an interval tree?),
>>>> type definition, setter function and the BUILD_EXTIO invocations
>>>> somewhere generic, rather than squirelled away in the arch backend.
>>>>
>>> The concern would be that some architecture which uses generic higher-level
>>> ISA accessor ops, but have IO space, could be affected.
>>
>> You're already adding a Kconfig symbol for this stuff, so you can keep
>> that if you don't want it on other architectures. I'm just arguing that
>> plumbing drivers directly into arch code via arm64_set_extops is not
>> something I'm particularly fond of, especially when it looks like it
>> could be avoided with a small amount of effort.
>
> Agreed, I initially suggested putting this into arch/arm64/, but there isn't
> really a reason why it couldn't just live in lib/ with the header file
> bits moved to include/asm-generic/io.h which we already use.
>

Right, Zhichang will check the logistics of this. The generic io.h is 
quite clean, so as long as you don't mind new build switches of this 
nature being added, it should be ok; and we'll plan on moving extio.h 
into include/asm-generic as well.

Cheers,
John

> 	Arnd
>
> .
>


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

On Wednesday, November 9, 2016 11:29:46 AM CET John Garry wrote:
> On 08/11/2016 22:35, Arnd Bergmann wrote:
> > On Tuesday, November 8, 2016 4:49:49 PM CET Will Deacon wrote:
> >> On Tue, Nov 08, 2016 at 04:33:44PM +0000, John Garry wrote:
> >>> On 08/11/2016 16:12, Will Deacon wrote:
> >>>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> >
> >>>> Is there no way to make this slightly more generic, so that it can be
> >>>> re-used elsewhere? For example, if struct extio_ops was common, then
> >>>> you could have the singleton (which maybe should be an interval tree?),
> >>>> type definition, setter function and the BUILD_EXTIO invocations
> >>>> somewhere generic, rather than squirelled away in the arch backend.
> >>>>
> >>> The concern would be that some architecture which uses generic higher-level
> >>> ISA accessor ops, but have IO space, could be affected.
> >>
> >> You're already adding a Kconfig symbol for this stuff, so you can keep
> >> that if you don't want it on other architectures. I'm just arguing that
> >> plumbing drivers directly into arch code via arm64_set_extops is not
> >> something I'm particularly fond of, especially when it looks like it
> >> could be avoided with a small amount of effort.
> >
> > Agreed, I initially suggested putting this into arch/arm64/, but there isn't
> > really a reason why it couldn't just live in lib/ with the header file
> > bits moved to include/asm-generic/io.h which we already use.
> >
> 
> Right, Zhichang will check the logistics of this. The generic io.h is 
> quite clean, so as long as you don't mind new build switches of this 
> nature being added, it should be ok; and we'll plan on moving extio.h 
> into include/asm-generic as well.

I think all we need is an #ifdef CONFIG_something around the existing
defintion, with the alternative being "extern" declarations, after that
all the interesting logic can sit in a file in lib/.

	Arnd

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

Hi, Ben,


On 2016/11/9 7:16, Benjamin Herrenschmidt wrote:
> On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>>>
>>> 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.
>>
>> This has nothing to do with arm64. Hardware with this kind of indirect
>> bus access could be integrated with a variety of CPU architectures. It
>> simply hasn't been, yet.
> 
> On some ppc's we also use similar indirect access methods for IOs. We
> have a generic infrastructure for re-routing some memory or IO regions
> to hooks.
> 
I am interested on the generic infrastructure on PPC.
Could you point out where those drivers are?
want to take a look..

Thanks,
Zhichang

> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
> 
>>> 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.
>>
>> As above, this has nothing to do with arm64, and as such, should live in
>> generic code, exactly as we would do if we had higher-level ISA
>> accessor ops.
>>
>> Regardless, given the multi-instance case, I don't think this is
>> sufficient in general (and I think we need higher-level ISA accessors
>> to handle the indirection).
> 
> Multi-instance with IO is tricky to do generically because archs already
> have all sort of hacks to deal with the fact that inb/outb don't require
> an explicit ioremap, so an IO resource can take all sort of shape depending
> on the arch.
> 
> Overall it boils down to applying some kind of per-instance "offset" to
> the IO port number though.
> 

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

On Wed, Nov 09, 2016 at 10:16:42AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
> > On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
> > > 
> > > 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.
> > 
> > This has nothing to do with arm64. Hardware with this kind of indirect
> > bus access could be integrated with a variety of CPU architectures. It
> > simply hasn't been, yet.
> 
> On some ppc's we also use similar indirect access methods for IOs. We
> have a generic infrastructure for re-routing some memory or IO regions
> to hooks.
> 
> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.

Just to check, do you hook that in your inb/outb/etc?

Generally, it would seem nicer if we could have higher-level
isa_{inb,outb,whatever} accessors that we could hook separately from
other IO.

We don't necessarily have to move all ISA drivers over to that if we had
a separate symbol for that interface.

Thanks,
Mark.
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> > firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
> 
> Just to check, do you hook that in your inb/outb/etc?

Yes.

> Generally, it would seem nicer if we could have higher-level
> isa_{inb,outb,whatever} accessors that we could hook separately from
> other IO.

Maybe but generally speaking, we don't discriminate accessors per bus,
ie, readl etc... work on all memory mapped busses, inb... works on all
busses with an "IO space", at least that's been the idea. It probably
all comes from the fact that PCI IO and ISA are the same space on
x86 and most other platforms (not all).

> We don't necessarily have to move all ISA drivers over to that if we had
> a separate symbol for that interface.

What I do on ppc today is that I have a chunk of virtual address space
that is reserved for "IO space". The first 64k are "reserved" in that
they route to "the primary" ISA bus (for legacy crap that uses hard
coded addresses, though I use that for my LPC bus too). I "allocate"
space for the PCI IO spaces higher in that space. Was I to support more
LPC busses I could allocate them up there too.

The IO resource of a given device thus becomes the actual IO port plus
the offset of the base of the segment it's in.

For memory mapped IO, inb/outb will just add the virtual address of
the base of all IO space to that. The hooking mechanism will pickup
the stuff that isn't memory mapped.

It's a bit messy but then IO space performance has never been a huge
worry since IO cycles tend to be very slow to begin with.

Note: We also have the ISA memory and ISA FW spaces that we don't have
good accessors for. They somewhat exist (I think the fbdev layer uses
some for vga) but it's messy.

Cheers,
Ben.

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

Hi, Ben, Mark,

Thanks for your comments! These are helpful!


On 2016/11/11 3:32, Benjamin Herrenschmidt wrote:
> On Thu, 2016-11-10 at 11:22 +0000, Mark Rutland wrote:
>> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
>>> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
>>
>> Just to check, do you hook that in your inb/outb/etc?
> 
> Yes.
> 
>> Generally, it would seem nicer if we could have higher-level
>> isa_{inb,outb,whatever} accessors that we could hook separately from
>> other IO.
> 
> Maybe but generally speaking, we don't discriminate accessors per bus,
> ie, readl etc... work on all memory mapped busses, inb... works on all
> busses with an "IO space", at least that's been the idea. It probably
> all comes from the fact that PCI IO and ISA are the same space on
> x86 and most other platforms (not all).
> 
>> We don't necessarily have to move all ISA drivers over to that if we had
>> a separate symbol for that interface.
> 
> What I do on ppc today is that I have a chunk of virtual address space
> that is reserved for "IO space". The first 64k are "reserved" in that
> they route to "the primary" ISA bus (for legacy crap that uses hard
> coded addresses, though I use that for my LPC bus too). I "allocate"
> space for the PCI IO spaces higher in that space. Was I to support more
> LPC busses I could allocate them up there too.
>

I have similar idea as your PPC MMIO.

We notice the prototype of {in/out()} is something like that:

static inline u8 inb(unsigned long addr)
static inline void outb(u8 value, unsigned long addr)

The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
So, could you divide this 'addr' into several bit segments? The top 8 bits is
defined as bus index. For normal direct IO, the bus index is 0. For those bus
device which need indirectIO or some special I/O accessors, when these devices
are initializing, can request to allocate an unique ID to them, and register
their own accessors to the entry which is corresponding to the ID.

In this way, we can support multiple domains, I think.
But I am not sure whether it is feasible, for example, are there some
architectures/platforms had populated the top 8 bits? Do we need to request IO
region from ioport_resource for those devices?  etc...


Thanks,
Zhichang


> The IO resource of a given device thus becomes the actual IO port plus
> the offset of the base of the segment it's in.
> 
> For memory mapped IO, inb/outb will just add the virtual address of
> the base of all IO space to that. The hooking mechanism will pickup
> the stuff that isn't memory mapped.
> 
> It's a bit messy but then IO space performance has never been a huge
> worry since IO cycles tend to be very slow to begin with.
> 
> Note: We also have the ISA memory and ISA FW spaces that we don't have
> good accessors for. They somewhat exist (I think the fbdev layer uses
> some for vga) but it's messy.
> 
> Cheers,
> Ben.
> 
> 
> .
> 

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

On Friday, November 11, 2016 6:07:07 PM CET zhichang.yuan wrote:
> 
> I have similar idea as your PPC MMIO.
> 
> We notice the prototype of {in/out()} is something like that:
> 
> static inline u8 inb(unsigned long addr)
> static inline void outb(u8 value, unsigned long addr)
> 
> The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
> So, could you divide this 'addr' into several bit segments? The top 8 bits is
> defined as bus index. For normal direct IO, the bus index is 0. For those bus
> device which need indirectIO or some special I/O accessors, when these devices
> are initializing, can request to allocate an unique ID to them, and register
> their own accessors to the entry which is corresponding to the ID.

Ah, have you looked at the IA64 code? It does exactly this.
For ARM64 we decided to use the same basic approach as powerpc with
a single range of virtual memory for mapping it as that somewhat
simplified all cases we knew about at the time.

> In this way, we can support multiple domains, I think.
> But I am not sure whether it is feasible, for example, are there some
> architectures/platforms had populated the top 8 bits? Do we need to request IO
> region from ioport_resource for those devices?  etc...

On a 64-bit architecture, the top 32 bits of the port number are
definitely free to use for this, and 8 bits are probably sufficient.

Even on 32 bit architectures, I can't see why we'd ever need more than
16 bits worth of addressing within a domain, so using 8 bit domain
and 16 bit address leaves 8 or 40 unused bits.

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

Hi, Arnd,


On 2016/11/18 17:20, Arnd Bergmann wrote:
> On Friday, November 11, 2016 6:07:07 PM CET zhichang.yuan wrote:
>>
>> I have similar idea as your PPC MMIO.
>>
>> We notice the prototype of {in/out()} is something like that:
>>
>> static inline u8 inb(unsigned long addr)
>> static inline void outb(u8 value, unsigned long addr)
>>
>> The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
>> So, could you divide this 'addr' into several bit segments? The top 8 bits is
>> defined as bus index. For normal direct IO, the bus index is 0. For those bus
>> device which need indirectIO or some special I/O accessors, when these devices
>> are initializing, can request to allocate an unique ID to them, and register
>> their own accessors to the entry which is corresponding to the ID.
> 
> Ah, have you looked at the IA64 code? It does exactly this.
> For ARM64 we decided to use the same basic approach as powerpc with
> a single range of virtual memory for mapping it as that somewhat
> simplified all cases we knew about at the time.

Yes. I spent some time to trace how to work on PPC. But the code is a bit long,
I am not clear on how the indirectIO there was supported.

I noticed there are CONFIG_PPC_INDIRECT_PIO and CONFIG_PPC_INDIRECT_MMIO on PPC.
It seems that only CONFIG_PPC_INDIRECT_MMIO applied some MSB to store the bus
tokens which are used to get iowa_busses[] for specific operation helpers.
I can not find how CONFIG_PPC_INDIRECT_PIO support multiple ISA domains. It
seems only Opal-lpc.c adopt this INDIRECT_PIO method.

Although CONFIG_PPC_INDIRECT_MMIO is for MMIO, seems not suitable for ISA/LPC
I/O. But this idea is helpful.

what else did I miss??

> 
>> In this way, we can support multiple domains, I think.
>> But I am not sure whether it is feasible, for example, are there some
>> architectures/platforms had populated the top 8 bits? Do we need to request IO
>> region from ioport_resource for those devices?  etc...
> 
> On a 64-bit architecture, the top 32 bits of the port number are
> definitely free to use for this, and 8 bits are probably sufficient.
> 
> Even on 32 bit architectures, I can't see why we'd ever need more than
> 16 bits worth of addressing within a domain, so using 8 bit domain
> and 16 bit address leaves 8 or 40 unused bits.

Yes. 8 bits are enough.
But the maximal PIO on some architectures are defined as ~0 or -1. There is no
any bare space left. Probably we can not ensure the upper 8 bits available.


Thanks,
Zhichang


> 
> 	Arnd
> 
> .
> 

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

On Friday, November 18, 2016 7:12:35 PM CET zhichang.yuan wrote:
> Hi, Arnd,
> 
> 
> On 2016/11/18 17:20, Arnd Bergmann wrote:
> > On Friday, November 11, 2016 6:07:07 PM CET zhichang.yuan wrote:
> >>
> >> I have similar idea as your PPC MMIO.
> >>
> >> We notice the prototype of {in/out()} is something like that:
> >>
> >> static inline u8 inb(unsigned long addr)
> >> static inline void outb(u8 value, unsigned long addr)
> >>
> >> The type of parameter 'addr' is unsigned long. For I/O space, it is big enough.
> >> So, could you divide this 'addr' into several bit segments? The top 8 bits is
> >> defined as bus index. For normal direct IO, the bus index is 0. For those bus
> >> device which need indirectIO or some special I/O accessors, when these devices
> >> are initializing, can request to allocate an unique ID to them, and register
> >> their own accessors to the entry which is corresponding to the ID.
> > 
> > Ah, have you looked at the IA64 code? It does exactly this.
> > For ARM64 we decided to use the same basic approach as powerpc with
> > a single range of virtual memory for mapping it as that somewhat
> > simplified all cases we knew about at the time.
> 
> Yes. I spent some time to trace how to work on PPC. But the code is a bit long,
> I am not clear on how the indirectIO there was supported.
> 
> I noticed there are CONFIG_PPC_INDIRECT_PIO and CONFIG_PPC_INDIRECT_MMIO on PPC.
> It seems that only CONFIG_PPC_INDIRECT_MMIO applied some MSB to store the bus
> tokens which are used to get iowa_busses[] for specific operation helpers.
> I can not find how CONFIG_PPC_INDIRECT_PIO support multiple ISA domains. It
> seems only Opal-lpc.c adopt this INDIRECT_PIO method.
> 
> Although CONFIG_PPC_INDIRECT_MMIO is for MMIO, seems not suitable for ISA/LPC
> I/O. But this idea is helpful.
> 
> what else did I miss??

I mentioned two different things here: ia64 IIRC uses some bits of the
port number to look up the domain, while powerpc traditionally had no
support for any such lookup, it did the same thing as ARM64 with
virtual remapping of MMIO ranges into an address range starting at
a fixed virtual address.

CONFIG_PPC_INDIRECT_PIO is a fairly recent addition, I was not thinking
of that.

> >> In this way, we can support multiple domains, I think.
> >> But I am not sure whether it is feasible, for example, are there some
> >> architectures/platforms had populated the top 8 bits? Do we need to request IO
> >> region from ioport_resource for those devices?  etc...
> > 
> > On a 64-bit architecture, the top 32 bits of the port number are
> > definitely free to use for this, and 8 bits are probably sufficient.
> > 
> > Even on 32 bit architectures, I can't see why we'd ever need more than
> > 16 bits worth of addressing within a domain, so using 8 bit domain
> > and 16 bit address leaves 8 or 40 unused bits.
> 
> Yes. 8 bits are enough.
> But the maximal PIO on some architectures are defined as ~0 or -1. There is no
> any bare space left. Probably we can not ensure the upper 8 bits available.

Right, we clearly can't use it across all architectures. The trick with
architectures using ULONG_MAX as the limit for port numbers is that they
treat it as a 1:1 mapping between port numbers and virtual addresses,
which is yet another way to handle the MMIO-based devices, but that has
a number of downsides we don't need to get into now.

What I think the code should do is a generic workaround handling that
architectures can opt-in to. We'd start doing this on ARM64 only,
and can then decide whether to change ARM or PowerPC over to use
that as well.

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

On 08/11/2016 23:16, Benjamin Herrenschmidt wrote:
> On Tue, 2016-11-08 at 12:03 +0000, Mark Rutland wrote:
>> On Tue, Nov 08, 2016 at 11:47:07AM +0800, zhichang.yuan wrote:
>>>
>>> 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.
>>
>> This has nothing to do with arm64. Hardware with this kind of indirect
>> bus access could be integrated with a variety of CPU architectures. It
>> simply hasn't been, yet.
>
> On some ppc's we also use similar indirect access methods for IOs. We
> have a generic infrastructure for re-routing some memory or IO regions
> to hooks.
>
> On POWER8, our PCIe doesn't do IO at all, but we have an LPC bus behind
> firmware calls ;-) We use that infrastructure to plumb in the LPC bus.
>
Hi,

I would like to mention another topic on supporting LPC, and this is 
regard to eSPI support.
eSPI is seen as the successor for LPC, and some BMCs already support it.

I had a chat with Arnd on this, and the idea to model LPC as a SPI bus 
adpater (and also eSPI).

However it seems to me that most platforms will/should support eSPI as a 
transparent bridge, same as LPC on x86. So I don't think that this is 
much point in modelling LPC/eSPI as a bus.

So we shall continue with indriect-IO support...

Thanks,
John

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

Hi Guys,

On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> 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.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  arch/arm64/Kconfig             |  6 +++
>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>  arch/arm64/kernel/Makefile     |  1 +
>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>  5 files changed, 157 insertions(+)

When I applied these three patches against current linus tree and
enable CONFIG_HISILICON_LPC, the following build failure[1] is
triggered when running 'make modules'.


Thanks,
Ming

[1] 'make modules' failure log

  Building modules, stage 2.
  MODPOST 2260 modules
ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
ERROR: "outw" [drivers/video/vgastate.ko] undefined!
ERROR: "outb" [drivers/video/vgastate.ko] undefined!
ERROR: "inb" [drivers/video/vgastate.ko] undefined!
ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
.....
ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
scripts/Makefile.modpost:91: recipe for target '__modpost' failed
make[1]: *** [__modpost] Error 1
Makefile:1196: recipe for target 'modules' failed
make: *** [modules] Error 2


>  create mode 100644 arch/arm64/include/asm/extio.h
>  create mode 100644 arch/arm64/kernel/extio.c
>
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 969ef88..b44070b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>         default 16
>
> +config ARM64_INDIRECT_PIO
> +       bool "access peripherals with legacy I/O port"
> +       help
> +         Support special accessors for ISA I/O devices. This is needed for
> +         SoCs that do not support standard read/write for the ISA range.
> +
>  config NO_IOPORT_MAP
>         def_bool y if !PCI
>
> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
> new file mode 100644
> index 0000000..6ae0787
> --- /dev/null
> +++ b/arch/arm64/include/asm/extio.h
> @@ -0,0 +1,94 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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
> +
> +struct extio_ops {
> +       unsigned long start;/* inclusive, sys io addr */
> +       unsigned long end;/* inclusive, sys io addr */
> +
> +       u64 (*pfin)(void *devobj, unsigned long ptaddr, size_t dlen);
> +       void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
> +                                       size_t dlen);
> +       u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
> +                               size_t dlen, unsigned int count);
> +       void (*pfouts)(void *devobj, unsigned long ptaddr,
> +                               const void *outbuf, size_t dlen,
> +                               unsigned int count);
> +       void *devpara;
> +};
> +
> +extern struct extio_ops *arm64_extio_ops;
> +
> +#define DECLARE_EXTIO(bw, type)                                                \
> +extern type in##bw(unsigned long addr);                                        \
> +extern void out##bw(type value, unsigned long addr);                   \
> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
> +
> +#define BUILD_EXTIO(bw, type)                                          \
> +type in##bw(unsigned long addr)                                                \
> +{                                                                      \
> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
> +                       arm64_extio_ops->end < addr)                    \
> +               return read##bw(PCI_IOBASE + addr);                     \
> +       return arm64_extio_ops->pfin ?                                  \
> +               arm64_extio_ops->pfin(arm64_extio_ops->devpara,         \
> +                       addr, sizeof(type)) : -1;                       \
> +}                                                                      \
> +                                                                       \
> +void out##bw(type value, unsigned long addr)                           \
> +{                                                                      \
> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
> +                       arm64_extio_ops->end < addr)                    \
> +               write##bw(value, PCI_IOBASE + addr);                    \
> +       else                                                            \
> +               if (arm64_extio_ops->pfout)                             \
> +                       arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
> +                               addr, value, sizeof(type));             \
> +}                                                                      \
> +                                                                       \
> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)     \
> +{                                                                      \
> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
> +                       arm64_extio_ops->end < addr)                    \
> +               reads##bw(PCI_IOBASE + addr, buffer, count);            \
> +       else                                                            \
> +               if (arm64_extio_ops->pfins)                             \
> +                       arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
> +                               addr, buffer, sizeof(type), count);     \
> +}                                                                      \
> +                                                                       \
> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)      \
> +{                                                                      \
> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
> +                       arm64_extio_ops->end < addr)                    \
> +               writes##bw(PCI_IOBASE + addr, buffer, count);           \
> +       else                                                            \
> +               if (arm64_extio_ops->pfouts)                            \
> +                       arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
> +                               addr, buffer, sizeof(type), count);     \
> +}
> +
> +static inline void arm64_set_extops(struct extio_ops *ops)
> +{
> +       if (ops)
> +               WRITE_ONCE(arm64_extio_ops, ops);
> +}
> +
> +#endif /* __LINUX_EXTIO_H*/
> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 0bba427..136735d 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -31,6 +31,7 @@
>  #include <asm/early_ioremap.h>
>  #include <asm/alternative.h>
>  #include <asm/cpufeature.h>
> +#include <asm/extio.h>
>
>  #include <xen/xen.h>
>
> @@ -149,6 +150,34 @@ 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.
> + */
> +#ifdef CONFIG_ARM64_INDIRECT_PIO
> +#define inb inb
> +#define outb outb
> +#define insb insb
> +#define outsb outsb
> +/* external declaration */
> +DECLARE_EXTIO(b, u8)
> +
> +#define inw inw
> +#define outw outw
> +#define insw insw
> +#define outsw outsw
> +
> +DECLARE_EXTIO(w, u16)
> +
> +#define inl inl
> +#define outl outl
> +#define insl insl
> +#define outsl outsl
> +
> +DECLARE_EXTIO(l, u32)
> +#endif
> +
> +
>  /*
>   * String version of I/O memory access operations.
>   */
> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
> index 7d66bba..60e0482 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)            += sys32.o kuser32.o signal32.o         \
>                                            sys_compat.o entry32.o
>  arm64-obj-$(CONFIG_FUNCTION_TRACER)    += ftrace.o entry-ftrace.o
>  arm64-obj-$(CONFIG_MODULES)            += arm64ksyms.o module.o
> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO) += extio.o
>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)  += module-plts.o
>  arm64-obj-$(CONFIG_PERF_EVENTS)                += perf_regs.o perf_callchain.o
>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)     += perf_event.o
> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> new file mode 100644
> index 0000000..647b3fa
> --- /dev/null
> +++ b/arch/arm64/kernel/extio.c
> @@ -0,0 +1,27 @@
> +/*
> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
> + */
> +
> +#include <linux/io.h>
> +
> +struct extio_ops *arm64_extio_ops;
> +
> +
> +BUILD_EXTIO(b, u8)
> +
> +BUILD_EXTIO(w, u16)
> +
> +BUILD_EXTIO(l, u32)
> --
> 1.9.1
>

Hi，Ming,


On 2016/12/22 16:15, Ming Lei wrote:
> Hi Guys,
> 
> On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
> <yuanzhichang@hisilicon.com> wrote:
>> 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.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will.deacon@arm.com>
>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>> ---
>>  arch/arm64/Kconfig             |  6 +++
>>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>>  arch/arm64/kernel/Makefile     |  1 +
>>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>>  5 files changed, 157 insertions(+)
> 
> When I applied these three patches against current linus tree and
> enable CONFIG_HISILICON_LPC, the following build failure[1] is
> triggered when running 'make modules'.
> 

Thanks for your report!

This patch has compilation issue on some architectures, sorry for the inconvenience caused by this!
The ongoing v6 will solve these issues.
I will trace this failure and provide a fix if you can not wait for the next version.

Could you send me your .config in private? I don't want to bother all the hacker in the mail-list.


Thanks,
Zhichang

> 
> Thanks,
> Ming
> 
> [1] 'make modules' failure log
> 
>   Building modules, stage 2.
>   MODPOST 2260 modules
> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
> .....
> ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
> ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
> make[1]: *** [__modpost] Error 1
> Makefile:1196: recipe for target 'modules' failed
> make: *** [modules] Error 2
> 
> 
>>  create mode 100644 arch/arm64/include/asm/extio.h
>>  create mode 100644 arch/arm64/kernel/extio.c
>>
>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>> index 969ef88..b44070b 100644
>> --- a/arch/arm64/Kconfig
>> +++ b/arch/arm64/Kconfig
>> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>>         default 16
>>
>> +config ARM64_INDIRECT_PIO
>> +       bool "access peripherals with legacy I/O port"
>> +       help
>> +         Support special accessors for ISA I/O devices. This is needed for
>> +         SoCs that do not support standard read/write for the ISA range.
>> +
>>  config NO_IOPORT_MAP
>>         def_bool y if !PCI
>>
>> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
>> new file mode 100644
>> index 0000000..6ae0787
>> --- /dev/null
>> +++ b/arch/arm64/include/asm/extio.h
>> @@ -0,0 +1,94 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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
>> +
>> +struct extio_ops {
>> +       unsigned long start;/* inclusive, sys io addr */
>> +       unsigned long end;/* inclusive, sys io addr */
>> +
>> +       u64 (*pfin)(void *devobj, unsigned long ptaddr, size_t dlen);
>> +       void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
>> +                                       size_t dlen);
>> +       u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
>> +                               size_t dlen, unsigned int count);
>> +       void (*pfouts)(void *devobj, unsigned long ptaddr,
>> +                               const void *outbuf, size_t dlen,
>> +                               unsigned int count);
>> +       void *devpara;
>> +};
>> +
>> +extern struct extio_ops *arm64_extio_ops;
>> +
>> +#define DECLARE_EXTIO(bw, type)                                                \
>> +extern type in##bw(unsigned long addr);                                        \
>> +extern void out##bw(type value, unsigned long addr);                   \
>> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
>> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
>> +
>> +#define BUILD_EXTIO(bw, type)                                          \
>> +type in##bw(unsigned long addr)                                                \
>> +{                                                                      \
>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>> +                       arm64_extio_ops->end < addr)                    \
>> +               return read##bw(PCI_IOBASE + addr);                     \
>> +       return arm64_extio_ops->pfin ?                                  \
>> +               arm64_extio_ops->pfin(arm64_extio_ops->devpara,         \
>> +                       addr, sizeof(type)) : -1;                       \
>> +}                                                                      \
>> +                                                                       \
>> +void out##bw(type value, unsigned long addr)                           \
>> +{                                                                      \
>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>> +                       arm64_extio_ops->end < addr)                    \
>> +               write##bw(value, PCI_IOBASE + addr);                    \
>> +       else                                                            \
>> +               if (arm64_extio_ops->pfout)                             \
>> +                       arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>> +                               addr, value, sizeof(type));             \
>> +}                                                                      \
>> +                                                                       \
>> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)     \
>> +{                                                                      \
>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>> +                       arm64_extio_ops->end < addr)                    \
>> +               reads##bw(PCI_IOBASE + addr, buffer, count);            \
>> +       else                                                            \
>> +               if (arm64_extio_ops->pfins)                             \
>> +                       arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
>> +                               addr, buffer, sizeof(type), count);     \
>> +}                                                                      \
>> +                                                                       \
>> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)      \
>> +{                                                                      \
>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>> +                       arm64_extio_ops->end < addr)                    \
>> +               writes##bw(PCI_IOBASE + addr, buffer, count);           \
>> +       else                                                            \
>> +               if (arm64_extio_ops->pfouts)                            \
>> +                       arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
>> +                               addr, buffer, sizeof(type), count);     \
>> +}
>> +
>> +static inline void arm64_set_extops(struct extio_ops *ops)
>> +{
>> +       if (ops)
>> +               WRITE_ONCE(arm64_extio_ops, ops);
>> +}
>> +
>> +#endif /* __LINUX_EXTIO_H*/
>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>> index 0bba427..136735d 100644
>> --- a/arch/arm64/include/asm/io.h
>> +++ b/arch/arm64/include/asm/io.h
>> @@ -31,6 +31,7 @@
>>  #include <asm/early_ioremap.h>
>>  #include <asm/alternative.h>
>>  #include <asm/cpufeature.h>
>> +#include <asm/extio.h>
>>
>>  #include <xen/xen.h>
>>
>> @@ -149,6 +150,34 @@ 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.
>> + */
>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>> +#define inb inb
>> +#define outb outb
>> +#define insb insb
>> +#define outsb outsb
>> +/* external declaration */
>> +DECLARE_EXTIO(b, u8)
>> +
>> +#define inw inw
>> +#define outw outw
>> +#define insw insw
>> +#define outsw outsw
>> +
>> +DECLARE_EXTIO(w, u16)
>> +
>> +#define inl inl
>> +#define outl outl
>> +#define insl insl
>> +#define outsl outsl
>> +
>> +DECLARE_EXTIO(l, u32)
>> +#endif
>> +
>> +
>>  /*
>>   * String version of I/O memory access operations.
>>   */
>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>> index 7d66bba..60e0482 100644
>> --- a/arch/arm64/kernel/Makefile
>> +++ b/arch/arm64/kernel/Makefile
>> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)            += sys32.o kuser32.o signal32.o         \
>>                                            sys_compat.o entry32.o
>>  arm64-obj-$(CONFIG_FUNCTION_TRACER)    += ftrace.o entry-ftrace.o
>>  arm64-obj-$(CONFIG_MODULES)            += arm64ksyms.o module.o
>> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO) += extio.o
>>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)  += module-plts.o
>>  arm64-obj-$(CONFIG_PERF_EVENTS)                += perf_regs.o perf_callchain.o
>>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)     += perf_event.o
>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>> new file mode 100644
>> index 0000000..647b3fa
>> --- /dev/null
>> +++ b/arch/arm64/kernel/extio.c
>> @@ -0,0 +1,27 @@
>> +/*
>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
>> + */
>> +
>> +#include <linux/io.h>
>> +
>> +struct extio_ops *arm64_extio_ops;
>> +
>> +
>> +BUILD_EXTIO(b, u8)
>> +
>> +BUILD_EXTIO(w, u16)
>> +
>> +BUILD_EXTIO(l, u32)
>> --
>> 1.9.1
>>
> 
> 
> 

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

On Fri, Dec 23, 2016 at 9:43 AM, zhichang.yuan
<yuanzhichang@hisilicon.com> wrote:
> Hi，Ming,
>
>
> On 2016/12/22 16:15, Ming Lei wrote:
>> Hi Guys,
>>
>> On Tue, Nov 8, 2016 at 11:47 AM, zhichang.yuan
>> <yuanzhichang@hisilicon.com> wrote:
>>> 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.
>>>
>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>> Cc: Will Deacon <will.deacon@arm.com>
>>> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
>>> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
>>> ---
>>>  arch/arm64/Kconfig             |  6 +++
>>>  arch/arm64/include/asm/extio.h | 94 ++++++++++++++++++++++++++++++++++++++++++
>>>  arch/arm64/include/asm/io.h    | 29 +++++++++++++
>>>  arch/arm64/kernel/Makefile     |  1 +
>>>  arch/arm64/kernel/extio.c      | 27 ++++++++++++
>>>  5 files changed, 157 insertions(+)
>>
>> When I applied these three patches against current linus tree and
>> enable CONFIG_HISILICON_LPC, the following build failure[1] is
>> triggered when running 'make modules'.
>>
>
> Thanks for your report!
>
> This patch has compilation issue on some architectures, sorry for the inconvenience caused by this!
> The ongoing v6 will solve these issues.
> I will trace this failure and provide a fix if you can not wait for the next version.
>
> Could you send me your .config in private? I don't want to bother all the hacker in the mail-list.
>

Sure, please see the config in attachment.

>
> Thanks,
> Zhichang
>
>>
>> Thanks,
>> Ming
>>
>> [1] 'make modules' failure log
>>
>>   Building modules, stage 2.
>>   MODPOST 2260 modules
>> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
>> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
>> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
>> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
>> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
>> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
>> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
>> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
>> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
>> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
>> .....
>> ERROR: "inb" [drivers/ata/pata_cmd64x.ko] undefined!
>> ERROR: "inb" [drivers/ata/pata_artop.ko] undefined!
>> scripts/Makefile.modpost:91: recipe for target '__modpost' failed
>> make[1]: *** [__modpost] Error 1
>> Makefile:1196: recipe for target 'modules' failed
>> make: *** [modules] Error 2
>>
>>
>>>  create mode 100644 arch/arm64/include/asm/extio.h
>>>  create mode 100644 arch/arm64/kernel/extio.c
>>>
>>> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
>>> index 969ef88..b44070b 100644
>>> --- a/arch/arm64/Kconfig
>>> +++ b/arch/arm64/Kconfig
>>> @@ -163,6 +163,12 @@ config ARCH_MMAP_RND_COMPAT_BITS_MIN
>>>  config ARCH_MMAP_RND_COMPAT_BITS_MAX
>>>         default 16
>>>
>>> +config ARM64_INDIRECT_PIO
>>> +       bool "access peripherals with legacy I/O port"
>>> +       help
>>> +         Support special accessors for ISA I/O devices. This is needed for
>>> +         SoCs that do not support standard read/write for the ISA range.
>>> +
>>>  config NO_IOPORT_MAP
>>>         def_bool y if !PCI
>>>
>>> diff --git a/arch/arm64/include/asm/extio.h b/arch/arm64/include/asm/extio.h
>>> new file mode 100644
>>> index 0000000..6ae0787
>>> --- /dev/null
>>> +++ b/arch/arm64/include/asm/extio.h
>>> @@ -0,0 +1,94 @@
>>> +/*
>>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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
>>> +
>>> +struct extio_ops {
>>> +       unsigned long start;/* inclusive, sys io addr */
>>> +       unsigned long end;/* inclusive, sys io addr */
>>> +
>>> +       u64 (*pfin)(void *devobj, unsigned long ptaddr, size_t dlen);
>>> +       void (*pfout)(void *devobj, unsigned long ptaddr, u32 outval,
>>> +                                       size_t dlen);
>>> +       u64 (*pfins)(void *devobj, unsigned long ptaddr, void *inbuf,
>>> +                               size_t dlen, unsigned int count);
>>> +       void (*pfouts)(void *devobj, unsigned long ptaddr,
>>> +                               const void *outbuf, size_t dlen,
>>> +                               unsigned int count);
>>> +       void *devpara;
>>> +};
>>> +
>>> +extern struct extio_ops *arm64_extio_ops;
>>> +
>>> +#define DECLARE_EXTIO(bw, type)                                                \
>>> +extern type in##bw(unsigned long addr);                                        \
>>> +extern void out##bw(type value, unsigned long addr);                   \
>>> +extern void ins##bw(unsigned long addr, void *buffer, unsigned int count);\
>>> +extern void outs##bw(unsigned long addr, const void *buffer, unsigned int count);
>>> +
>>> +#define BUILD_EXTIO(bw, type)                                          \
>>> +type in##bw(unsigned long addr)                                                \
>>> +{                                                                      \
>>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>>> +                       arm64_extio_ops->end < addr)                    \
>>> +               return read##bw(PCI_IOBASE + addr);                     \
>>> +       return arm64_extio_ops->pfin ?                                  \
>>> +               arm64_extio_ops->pfin(arm64_extio_ops->devpara,         \
>>> +                       addr, sizeof(type)) : -1;                       \
>>> +}                                                                      \
>>> +                                                                       \
>>> +void out##bw(type value, unsigned long addr)                           \
>>> +{                                                                      \
>>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>>> +                       arm64_extio_ops->end < addr)                    \
>>> +               write##bw(value, PCI_IOBASE + addr);                    \
>>> +       else                                                            \
>>> +               if (arm64_extio_ops->pfout)                             \
>>> +                       arm64_extio_ops->pfout(arm64_extio_ops->devpara,\
>>> +                               addr, value, sizeof(type));             \
>>> +}                                                                      \
>>> +                                                                       \
>>> +void ins##bw(unsigned long addr, void *buffer, unsigned int count)     \
>>> +{                                                                      \
>>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>>> +                       arm64_extio_ops->end < addr)                    \
>>> +               reads##bw(PCI_IOBASE + addr, buffer, count);            \
>>> +       else                                                            \
>>> +               if (arm64_extio_ops->pfins)                             \
>>> +                       arm64_extio_ops->pfins(arm64_extio_ops->devpara,\
>>> +                               addr, buffer, sizeof(type), count);     \
>>> +}                                                                      \
>>> +                                                                       \
>>> +void outs##bw(unsigned long addr, const void *buffer, unsigned int count)      \
>>> +{                                                                      \
>>> +       if (!arm64_extio_ops || arm64_extio_ops->start > addr ||        \
>>> +                       arm64_extio_ops->end < addr)                    \
>>> +               writes##bw(PCI_IOBASE + addr, buffer, count);           \
>>> +       else                                                            \
>>> +               if (arm64_extio_ops->pfouts)                            \
>>> +                       arm64_extio_ops->pfouts(arm64_extio_ops->devpara,\
>>> +                               addr, buffer, sizeof(type), count);     \
>>> +}
>>> +
>>> +static inline void arm64_set_extops(struct extio_ops *ops)
>>> +{
>>> +       if (ops)
>>> +               WRITE_ONCE(arm64_extio_ops, ops);
>>> +}
>>> +
>>> +#endif /* __LINUX_EXTIO_H*/
>>> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
>>> index 0bba427..136735d 100644
>>> --- a/arch/arm64/include/asm/io.h
>>> +++ b/arch/arm64/include/asm/io.h
>>> @@ -31,6 +31,7 @@
>>>  #include <asm/early_ioremap.h>
>>>  #include <asm/alternative.h>
>>>  #include <asm/cpufeature.h>
>>> +#include <asm/extio.h>
>>>
>>>  #include <xen/xen.h>
>>>
>>> @@ -149,6 +150,34 @@ 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.
>>> + */
>>> +#ifdef CONFIG_ARM64_INDIRECT_PIO
>>> +#define inb inb
>>> +#define outb outb
>>> +#define insb insb
>>> +#define outsb outsb
>>> +/* external declaration */
>>> +DECLARE_EXTIO(b, u8)
>>> +
>>> +#define inw inw
>>> +#define outw outw
>>> +#define insw insw
>>> +#define outsw outsw
>>> +
>>> +DECLARE_EXTIO(w, u16)
>>> +
>>> +#define inl inl
>>> +#define outl outl
>>> +#define insl insl
>>> +#define outsl outsl
>>> +
>>> +DECLARE_EXTIO(l, u32)
>>> +#endif
>>> +
>>> +
>>>  /*
>>>   * String version of I/O memory access operations.
>>>   */
>>> diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
>>> index 7d66bba..60e0482 100644
>>> --- a/arch/arm64/kernel/Makefile
>>> +++ b/arch/arm64/kernel/Makefile
>>> @@ -31,6 +31,7 @@ arm64-obj-$(CONFIG_COMPAT)            += sys32.o kuser32.o signal32.o         \
>>>                                            sys_compat.o entry32.o
>>>  arm64-obj-$(CONFIG_FUNCTION_TRACER)    += ftrace.o entry-ftrace.o
>>>  arm64-obj-$(CONFIG_MODULES)            += arm64ksyms.o module.o
>>> +arm64-obj-$(CONFIG_ARM64_INDIRECT_PIO) += extio.o
>>>  arm64-obj-$(CONFIG_ARM64_MODULE_PLTS)  += module-plts.o
>>>  arm64-obj-$(CONFIG_PERF_EVENTS)                += perf_regs.o perf_callchain.o
>>>  arm64-obj-$(CONFIG_HW_PERF_EVENTS)     += perf_event.o
>>> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
>>> new file mode 100644
>>> index 0000000..647b3fa
>>> --- /dev/null
>>> +++ b/arch/arm64/kernel/extio.c
>>> @@ -0,0 +1,27 @@
>>> +/*
>>> + * Copyright (C) 2016 Hisilicon Limited, All Rights Reserved.
>>> + * Author: Zhichang Yuan <yuanzhichang@hisilicon.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/>.
>>> + */
>>> +
>>> +#include <linux/io.h>
>>> +
>>> +struct extio_ops *arm64_extio_ops;
>>> +
>>> +
>>> +BUILD_EXTIO(b, u8)
>>> +
>>> +BUILD_EXTIO(w, u16)
>>> +
>>> +BUILD_EXTIO(l, u32)
>>> --
>>> 1.9.1
>>>
>>
>>
>>
>



Thanks,
Ming Lei

On Thursday, December 22, 2016 4:15:57 PM CET Ming Lei wrote:
> ERROR: "inb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/wdt_pci.ko] undefined!
> ERROR: "outb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "inb" [drivers/watchdog/pcwd_pci.ko] undefined!
> ERROR: "outw" [drivers/video/vgastate.ko] undefined!
> ERROR: "outb" [drivers/video/vgastate.ko] undefined!
> ERROR: "inb" [drivers/video/vgastate.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/vt8623fb.ko] undefined!
> ERROR: "outw" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "outb" [drivers/video/fbdev/tridentfb.ko] undefined!
> ERROR: "inb" [drivers/video/fbdev/tdfxfb.ko] undefined!
> 

In case you haven't figured it out by now, the new code is simply
missing a few "EXPORT_SYMBOL" lines.

	Arnd

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