[V5,2/3] ARM64 LPC: Add missing range exception for special ISA

This patch solves two issues:
1) parse and get the right I/O range from DTS node whose parent does not
define the corresponding ranges property;

There are some special ISA/LPC devices that work on a specific I/O range where
it is not correct to specify a ranges property in DTS parent node as cpu
addresses translated from DTS node are only for memory space on some
architectures, such as Arm64. Without the parent 'ranges' property, current
of_translate_address() return an error.
Here we add a fixup function, of_get_isa_indirect_io(). During the OF address
translation, this fixup will be called to check the 'reg' address to be
translating is for those sepcial ISA/LPC devices and get the I/O range
directly from the 'reg' property.

2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O device;

The current __of_address_to_resource() always translates the I/O range to PIO.
But this processing is not suitable for our ISA/LPC devices whose I/O range is
not cpu address(Arnd had stressed this in his comments on V2,V3 patch-set).
Here, we bypass the mapping between cpu address and PIO for the special
ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port address below
PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict risk
between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O range of [0,
IO_SPACE_LIMIT).
To avoid the I/O conflict, this patch reserve the I/O range below
PCIBIOS_MIN_IO.

Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
---
 .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++
 arch/arm64/include/asm/io.h                        |  6 +++
 arch/arm64/kernel/extio.c                          | 25 ++++++++++
 drivers/of/address.c                               | 56 +++++++++++++++++++++-
 drivers/pci/pci.c                                  |  6 +--
 include/linux/of_address.h                         | 17 +++++++
 include/linux/pci.h                                |  8 ++++
 7 files changed, 145 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt

Hi zhichang.yuan,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.9-rc4 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhichang-yuan/ARM64-LPC-legacy-ISA-I-O-support/20161108-114742
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: mips-ath25_defconfig (attached as .config)
compiler: mips-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=mips 

All error/warnings (new ones prefixed by >>):

   In file included from arch/mips/pci/pci.c:18:0:
>> include/linux/pci.h:2113:25: error: expected identifier or '(' before numeric constant
    #define PCIBIOS_MIN_IO  0
                            ^
>> arch/mips/pci/pci.c:34:15: note: in expansion of macro 'PCIBIOS_MIN_IO'
    unsigned long PCIBIOS_MIN_IO;
                  ^~~~~~~~~~~~~~
   In file included from include/linux/linkage.h:6:0,
                    from include/linux/kernel.h:6,
                    from include/asm-generic/bug.h:13,
                    from arch/mips/include/asm/bug.h:41,
                    from include/linux/bug.h:4,
                    from arch/mips/pci/pci.c:11:
>> include/linux/pci.h:2113:25: error: expected identifier or '(' before numeric constant
    #define PCIBIOS_MIN_IO  0
                            ^
   include/linux/export.h:57:21: note: in definition of macro '___EXPORT_SYMBOL'
     extern typeof(sym) sym;     \
                        ^~~
>> arch/mips/pci/pci.c:326:1: note: in expansion of macro 'EXPORT_SYMBOL'
    EXPORT_SYMBOL(PCIBIOS_MIN_IO);
    ^~~~~~~~~~~~~
   arch/mips/pci/pci.c:326:15: note: in expansion of macro 'PCIBIOS_MIN_IO'
    EXPORT_SYMBOL(PCIBIOS_MIN_IO);
                  ^~~~~~~~~~~~~~
>> include/linux/export.h:66:21: error: lvalue required as unary '&' operand
     = { (unsigned long)&sym, __kstrtab_##sym }
                        ^
>> include/linux/export.h:94:25: note: in expansion of macro '___EXPORT_SYMBOL'
    #define __EXPORT_SYMBOL ___EXPORT_SYMBOL
                            ^~~~~~~~~~~~~~~~
   include/linux/export.h:98:2: note: in expansion of macro '__EXPORT_SYMBOL'
     __EXPORT_SYMBOL(sym, "")
     ^~~~~~~~~~~~~~~
>> arch/mips/pci/pci.c:326:1: note: in expansion of macro 'EXPORT_SYMBOL'
    EXPORT_SYMBOL(PCIBIOS_MIN_IO);
    ^~~~~~~~~~~~~

vim +2113 include/linux/pci.h

  2107	
  2108	/*
  2109	 * define this macro here to refrain from compilation error for some
  2110	 * platforms. Please keep this macro at the end of this header file.
  2111	 */
  2112	#ifndef PCIBIOS_MIN_IO
> 2113	#define PCIBIOS_MIN_IO		0
  2114	#endif
  2115	
  2116	#endif /* LINUX_PCI_H */

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

Hi zhichang.yuan,

[auto build test ERROR on arm64/for-next/core]
[also build test ERROR on v4.9-rc4 next-20161028]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/zhichang-yuan/ARM64-LPC-legacy-ISA-I-O-support/20161108-114742
base:   https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: arm-sunxi_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `of_address_to_resource':
>> sunxi_sid.c:(.text+0x18af5c): undefined reference to `pcibios_min_io'
   sunxi_sid.c:(.text+0x18af60): undefined reference to `pcibios_min_io'

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> +Hisilicon Hip06 low-pin-count device
> +  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
> +  locate on LPC bus can be accessed direclty. But some SoCs have independent
> +  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
> +  Hisilicon Hip06 implements this LPC device.

s/direclty/directly/

My understanding of ISA (which may be flawed) is that it's not part of
the PCI host bridge, but rather on x86 it happens to share the IO space
with PCI.

So, how about this becomes:

  Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
  provides access to some legacy ISA devices.

I believe that we could theoretically have multiple independent LPC/ISA
busses, as is possible with PCI on !x86 systems. If the current ISA code
assumes a singleton bus, I think that's something that needs to be fixed
up more generically.

I don't see why we should need any architecture-specific code here. Why
can we not fix up the ISA bus code in drivers/of/address.c such that it
handles multiple ISA bus instances, and translates all sub-device
addresses relative to the specific bus instance?

> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"

This would be better as something like:

	"hisilicon,hip06-lpc-controller"

If it's reused in other SoCs, we can add more strings as we usually do.

> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base memory range where the register set of this device is mapped.
> +
> +Note:
> +  The node name before '@' must be "isa" to represent the binding stick to the
> +  ISA/EISA binding specification.
> +
> +Example:
> +
> +isa@a01b0000 {
> +	compatible = "hisilicom,low-pin-count";

s/hisilicom/hisilicon/

My comment above on the compatible string also applies.

> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> +
> +	ipmi0: bt@e4 {
> +		compatible = "ipmi-bt";
> +		device_type = "ipmi";
> +		reg = <0x01 0xe4 0x04>;
> +		status = "disabled";
> +	};
> +};

Please remove the status property; it's irrelevant to the example.

[...]

> +/**
> + * indirect_io_enabled - check whether indirectIO is enabled.
> + *	arm64_extio_ops will be set only when indirectIO mechanism had been
> + *	initialized.
> + *
> + * Returns true when indirectIO is enabled.
> + */
> +bool indirect_io_enabled(void)
> +{
> +	return arm64_extio_ops ? true : false;
> +}

	return !!arm64_extio_ops;

> +/**
> + * addr_is_indirect_io - check whether the input taddr is for indirectIO.
> + * @taddr: the io address to be checked.
> + *
> + * Returns 1 when taddr is in the range; otherwise return 0.
> + */
> +int addr_is_indirect_io(u64 taddr)
> +{
> +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end < taddr)
> +		return 0;
> +
> +	return 1;
> +}

Why not bool?

I don't think this is the right thing to do, regardless.

> + * of_isa_indirect_io - get the IO address from some isa reg property value.
> + *	For some isa/lpc devices, no ranges property in ancestor node.
> + *	The device addresses are described directly in their regs property.
> + *	This fixup function will be called to get the IO address of isa/lpc
> + *	devices when the normal of_translation failed.
> + *
> + * @parent:	points to the parent dts node;
> + * @bus:		points to the of_bus which can be used to parse address;
> + * @addr:	the address from reg property;
> + * @na:		the address cell counter of @addr;
> + * @presult:	store the address paresed from @addr;
> + *
> + * return 1 when successfully get the I/O address;
> + * 0 will return for some failures.
> + */
> +static int of_get_isa_indirect_io(struct device_node *parent,
> +				struct of_bus *bus, __be32 *addr,
> +				int na, u64 *presult)
> +{
> +	unsigned int flags;
> +	unsigned int rlen;
> +
> +	/* whether support indirectIO */
> +	if (!indirect_io_enabled())
> +		return 0;
> +
> +	if (!of_bus_isa_match(parent))
> +		return 0;
> +
> +	flags = bus->get_flags(addr);
> +	if (!(flags & IORESOURCE_IO))
> +		return 0;
> +
> +	/* there is ranges property, apply the normal translation directly. */
> +	if (of_get_property(parent, "ranges", &rlen))
> +		return 0;
> +
> +	*presult = of_read_number(addr + 1, na - 1);
> +	/* this fixup is only valid for specific I/O range. */
> +	return addr_is_indirect_io(*presult);
> +}
> +
>  static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct device_node *dev,
>  			result = of_read_number(addr, na);
>  			break;
>  		}
> +		/*
> +		 * For indirectIO device which has no ranges property, get
> +		 * the address from reg directly.
> +		 */
> +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> +			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
> +				of_node_full_name(dev), result);
> +			break;
> +		}

I don't believe this is the right place for this to live. This should
live in the isa of_bus, matched in the usual way with of_match_bus(),
and used in pbus->translate() in of_translate_one().

If we need to extend the prototypes of those functions, we should do so.

If we need to be able to register instance-specific translations or
indirection, we should build the infrastructure for that rather than
forcing a singleton translation in here.

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 11:49:53 AM CET Mark Rutland wrote:
> On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> > +Hisilicon Hip06 low-pin-count device
> > +  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
> > +  locate on LPC bus can be accessed direclty. But some SoCs have independent
> > +  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
> > +  Hisilicon Hip06 implements this LPC device.
> 
> s/direclty/directly/
> 
> My understanding of ISA (which may be flawed) is that it's not part of
> the PCI host bridge, but rather on x86 it happens to share the IO space
> with PCI.

On normal systems, ISA or LPC are behind a PCI bridge device, which
passes down both low addresses of I/O space and memory space.

> So, how about this becomes:
> 
>   Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
>   provides access to some legacy ISA devices.
> 
> I believe that we could theoretically have multiple independent LPC/ISA
> busses, as is possible with PCI on !x86 systems. If the current ISA code
> assumes a singleton bus, I think that's something that needs to be fixed
> up more generically.
> 
> I don't see why we should need any architecture-specific code here. Why
> can we not fix up the ISA bus code in drivers/of/address.c such that it
> handles multiple ISA bus instances, and translates all sub-device
> addresses relative to the specific bus instance?

I think it is a relatively safe assumption that there is only one
ISA bridge. A lot of old drivers hardcode PIO or memory addresses
when talking to an ISA device, so having multiple instances is
already problematic.

What is odd about ARM64 here is that the PIO space is not shared among
all ISA and PCI buses in some cases.

	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 05:19:54PM +0100, Arnd Bergmann wrote:
> On Tuesday, November 8, 2016 11:49:53 AM CET Mark Rutland wrote:
> > My understanding of ISA (which may be flawed) is that it's not part of
> > the PCI host bridge, but rather on x86 it happens to share the IO space
> > with PCI.
> 
> On normal systems, ISA or LPC are behind a PCI bridge device, which
> passes down both low addresses of I/O space and memory space.

Ok, so the use of those address spaces is an artifact of the ISA
controller being a device under the PCI host bridge.

Given we can have multiple domains, surely that implies we can have
multiple ISA controllers in general?

> > I believe that we could theoretically have multiple independent LPC/ISA
> > busses, as is possible with PCI on !x86 systems. If the current ISA code
> > assumes a singleton bus, I think that's something that needs to be fixed
> > up more generically.
> > 
> > I don't see why we should need any architecture-specific code here. Why
> > can we not fix up the ISA bus code in drivers/of/address.c such that it
> > handles multiple ISA bus instances, and translates all sub-device
> > addresses relative to the specific bus instance?
> 
> I think it is a relatively safe assumption that there is only one
> ISA bridge. A lot of old drivers hardcode PIO or memory addresses
> when talking to an ISA device, so having multiple instances is
> already problematic.

I'm worried that this might not be a safe assumption. Hardware these
days has a habit of pushing the boundaries of our expectations.

If we're going to assume that, I'd certainly want the kernel to verify
that it's true for all instanciated ISA/LPC devices. Otherwise, I can
imagine people relying on (or working around) that assumption in ACPI
tables and DTs, and that will be a nightmare (at best) to untangle in
future.

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 Tue, 2016-11-08 at 11:49 +0000, Mark Rutland wrote:
> 
> My understanding of ISA (which may be flawed) is that it's not part of
> the PCI host bridge, but rather on x86 it happens to share the IO space
> with PCI.

Sort-of. On some systems it actually goes through PCI and there's a
PCI->ISA bridge that uses substractive decoding to the legacy devices.

> So, how about this becomes:
> 
>   Hisilicon Hip06 SoCs implement a Low Pin Count (LPC) controller, which
>   provides access to some legacy ISA devices.
> 
> I believe that we could theoretically have multiple independent LPC/ISA
> busses, as is possible with PCI on !x86 systems. If the current ISA code
> assumes a singleton bus, I think that's something that needs to be fixed
> up more generically.
> 
> I don't see why we should need any architecture-specific code here. Why
> can we not fix up the ISA bus code in drivers/of/address.c such that it
> handles multiple ISA bus instances, and translates all sub-device
> addresses relative to the specific bus instance?

What in that code prevents that today ?

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 Wed, Nov 09, 2016 at 10:12:59AM +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2016-11-08 at 11:49 +0000, Mark Rutland wrote:
> > I believe that we could theoretically have multiple independent LPC/ISA
> > busses, as is possible with PCI on !x86 systems. If the current ISA code
> > assumes a singleton bus, I think that's something that needs to be fixed
> > up more generically.
> > 
> > I don't see why we should need any architecture-specific code here. Why
> > can we not fix up the ISA bus code in drivers/of/address.c such that it
> > handles multiple ISA bus instances, and translates all sub-device
> > addresses relative to the specific bus instance?
> 
> What in that code prevents that today ?

It appears I was mistaken w.r.t. the singleton comment. We can already
translate MMIO->MMIO addresses per-instance (in the presence of a ranges
property).

The big change would be to handle !MMIO translations, for which we'd
need a runtime registry of ISA bus instance to find the relevant
accessor ops and instance-specific data.

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 Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:
> This patch solves two issues:
> 1) parse and get the right I/O range from DTS node whose parent does not
> define the corresponding ranges property;
> 
> There are some special ISA/LPC devices that work on a specific I/O range where
> it is not correct to specify a ranges property in DTS parent node as cpu
> addresses translated from DTS node are only for memory space on some
> architectures, such as Arm64. Without the parent 'ranges' property, current
> of_translate_address() return an error.
> Here we add a fixup function, of_get_isa_indirect_io(). During the OF address
> translation, this fixup will be called to check the 'reg' address to be
> translating is for those sepcial ISA/LPC devices and get the I/O range
> directly from the 'reg' property.
> 
> 2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O device;
> 
> The current __of_address_to_resource() always translates the I/O range to PIO.
> But this processing is not suitable for our ISA/LPC devices whose I/O range is
> not cpu address(Arnd had stressed this in his comments on V2,V3 patch-set).
> Here, we bypass the mapping between cpu address and PIO for the special
> ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port address below
> PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict risk
> between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O range of [0,
> IO_SPACE_LIMIT).
> To avoid the I/O conflict, this patch reserve the I/O range below
> PCIBIOS_MIN_IO.
> 
> Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>
> Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>
> ---
>  .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++
>  arch/arm64/include/asm/io.h                        |  6 +++
>  arch/arm64/kernel/extio.c                          | 25 ++++++++++
>  drivers/of/address.c                               | 56 +++++++++++++++++++++-
>  drivers/pci/pci.c                                  |  6 +--
>  include/linux/of_address.h                         | 17 +++++++
>  include/linux/pci.h                                |  8 ++++
>  7 files changed, 145 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> new file mode 100644
> index 0000000..13c8ddd
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-count.txt
> @@ -0,0 +1,31 @@
> +Hisilicon Hip06 low-pin-count device
> +  Usually LPC controller is part of PCI host bridge, so the legacy ISA ports
> +  locate on LPC bus can be accessed direclty. But some SoCs have independent
> +  LPC controller, and access the legacy ports by triggering LPC I/O cycles.
> +  Hisilicon Hip06 implements this LPC device.
> +
> +Required properties:
> +- compatible: should be "hisilicon,low-pin-count"
> +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.
> +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.
> +- reg: base memory range where the register set of this device is mapped.
> +
> +Note:
> +  The node name before '@' must be "isa" to represent the binding stick to the
> +  ISA/EISA binding specification.
> +
> +Example:
> +
> +isa@a01b0000 {
> +	compatible = "hisilicom,low-pin-count";
> +	#address-cells = <2>;
> +	#size-cells = <1>;
> +	reg = <0x0 0xa01b0000 0x0 0x1000>;
> +
> +	ipmi0: bt@e4 {
> +		compatible = "ipmi-bt";
> +		device_type = "ipmi";
> +		reg = <0x01 0xe4 0x04>;
> +		status = "disabled";
> +	};
> +};


This documentation file needs to be part of the next patch. It has nothing to do with
what you are trying to fix here.


> diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h
> index 136735d..c26b7cc 100644
> --- a/arch/arm64/include/asm/io.h
> +++ b/arch/arm64/include/asm/io.h
> @@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile void __iomem *addr)
>  #define outsl outsl
>  
>  DECLARE_EXTIO(l, u32)
> +
> +#define indirect_io_enabled indirect_io_enabled
> +extern bool indirect_io_enabled(void);
> +
> +#define addr_is_indirect_io addr_is_indirect_io
> +extern int addr_is_indirect_io(u64 taddr);
>  #endif
>  
>  
> diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c
> index 647b3fa..3d45fa8 100644
> --- a/arch/arm64/kernel/extio.c
> +++ b/arch/arm64/kernel/extio.c
> @@ -19,6 +19,31 @@
>  
>  struct extio_ops *arm64_extio_ops;
>  
> +/**
> + * indirect_io_enabled - check whether indirectIO is enabled.
> + *	arm64_extio_ops will be set only when indirectIO mechanism had been
> + *	initialized.
> + *
> + * Returns true when indirectIO is enabled.
> + */
> +bool indirect_io_enabled(void)
> +{
> +	return arm64_extio_ops ? true : false;
> +}
> +
> +/**
> + * addr_is_indirect_io - check whether the input taddr is for indirectIO.
> + * @taddr: the io address to be checked.
> + *
> + * Returns 1 when taddr is in the range; otherwise return 0.
> + */
> +int addr_is_indirect_io(u64 taddr)
> +{
> +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end < taddr)

start >= taddr ?

> +		return 0;
> +
> +	return 1;
> +}
>  
>  BUILD_EXTIO(b, u8)
>  
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 02b2903..cc2a05d 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct device_node *np)
>  	return false;
>  }
>  
> +
> +/*
> + * of_isa_indirect_io - get the IO address from some isa reg property value.
> + *	For some isa/lpc devices, no ranges property in ancestor node.
> + *	The device addresses are described directly in their regs property.
> + *	This fixup function will be called to get the IO address of isa/lpc
> + *	devices when the normal of_translation failed.
> + *
> + * @parent:	points to the parent dts node;
> + * @bus:		points to the of_bus which can be used to parse address;
> + * @addr:	the address from reg property;
> + * @na:		the address cell counter of @addr;
> + * @presult:	store the address paresed from @addr;
> + *
> + * return 1 when successfully get the I/O address;
> + * 0 will return for some failures.

Bah, you are returning a signed int, why 0 for failure? Return a negative value with
error codes. Otherwise change the return value into a bool.

> + */
> +static int of_get_isa_indirect_io(struct device_node *parent,
> +				struct of_bus *bus, __be32 *addr,
> +				int na, u64 *presult)
> +{
> +	unsigned int flags;
> +	unsigned int rlen;
> +
> +	/* whether support indirectIO */
> +	if (!indirect_io_enabled())
> +		return 0;
> +
> +	if (!of_bus_isa_match(parent))
> +		return 0;
> +
> +	flags = bus->get_flags(addr);
> +	if (!(flags & IORESOURCE_IO))
> +		return 0;
> +
> +	/* there is ranges property, apply the normal translation directly. */

s/there is ranges/if we have a 'ranges'/

> +	if (of_get_property(parent, "ranges", &rlen))
> +		return 0;
> +
> +	*presult = of_read_number(addr + 1, na - 1);
> +	/* this fixup is only valid for specific I/O range. */
> +	return addr_is_indirect_io(*presult);
> +}
> +
>  static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>  			    struct of_bus *pbus, __be32 *addr,
>  			    int na, int ns, int pna, const char *rprop)
> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct device_node *dev,
>  			result = of_read_number(addr, na);
>  			break;
>  		}
> +		/*
> +		 * For indirectIO device which has no ranges property, get
> +		 * the address from reg directly.
> +		 */
> +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> +			pr_debug("isa indirectIO matched(%s)..addr = 0x%llx\n",
> +				of_node_full_name(dev), result);
> +			break;
> +		}
>  
>  		/* Get new parent bus and counts */
>  		pbus = of_match_bus(parent);
> @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct device_node *dev,
>  	if (taddr == OF_BAD_ADDR)
>  		return -EINVAL;
>  	memset(r, 0, sizeof(struct resource));
> -	if (flags & IORESOURCE_IO) {
> +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
>  		unsigned long port;
> +
>  		port = pci_address_to_pio(taddr);
>  		if (port == (unsigned long)-1)
>  			return -EINVAL;
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index ba34907..1a08511 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)
>  
>  #ifdef PCI_IOBASE
>  	struct io_range *range;
> -	resource_size_t allocated_size = 0;
> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
>  
>  	/* check if the range hasn't been previously recorded */
>  	spin_lock(&io_range_lock);
> @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
>  
>  #ifdef PCI_IOBASE
>  	struct io_range *range;
> -	resource_size_t allocated_size = 0;
> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;

Have you checked that pci_pio_to_address still returns valid values after this? I know that
you are trying to take into account PCIBIOS_MIN_IO limit when allocating reserving the IO ranges,
but the values added in the io_range_list are still starting from zero, no from PCIBIOS_MIN_IO,
so the calculation of the address in this function could return negative values casted to pci_addr_t.

Maybe you want to adjust the range->start value in pci_register_io_range() as well to have it
offset by PCIBIOS_MIN_IO as well.

Best regards,
Liviu

>  
>  	if (pio > IO_SPACE_LIMIT)
>  		return address;
> @@ -3335,7 +3335,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
>  {
>  #ifdef PCI_IOBASE
>  	struct io_range *res;
> -	resource_size_t offset = 0;
> +	resource_size_t offset = PCIBIOS_MIN_IO;
>  	unsigned long addr = -1;
>  
>  	spin_lock(&io_range_lock);
> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> index 3786473..deec469 100644
> --- a/include/linux/of_address.h
> +++ b/include/linux/of_address.h
> @@ -24,6 +24,23 @@ struct of_pci_range {
>  #define for_each_of_pci_range(parser, range) \
>  	for (; of_pci_range_parser_one(parser, range);)
>  
> +
> +#ifndef indirect_io_enabled
> +#define indirect_io_enabled indirect_io_enabled
> +static inline bool indirect_io_enabled(void)
> +{
> +	return false;
> +}
> +#endif
> +
> +#ifndef addr_is_indirect_io
> +#define addr_is_indirect_io addr_is_indirect_io
> +static inline int addr_is_indirect_io(u64 taddr)
> +{
> +	return 0;
> +}
> +#endif
> +
>  /* Translate a DMA address from device space to CPU space */
>  extern u64 of_translate_dma_address(struct device_node *dev,
>  				    const __be32 *in_addr);
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 0e49f70..7f6bbb6 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct pci_bus *bus)
>  /* provide the legacy pci_dma_* API */
>  #include <linux/pci-dma-compat.h>
>  
> +/*
> + * define this macro here to refrain from compilation error for some
> + * platforms. Please keep this macro at the end of this header file.
> + */
> +#ifndef PCIBIOS_MIN_IO
> +#define PCIBIOS_MIN_IO		0
> +#endif
> +
>  #endif /* LINUX_PCI_H */
> -- 
> 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

> I think it is a relatively safe assumption that there is only one
> ISA bridge. A lot of old drivers hardcode PIO or memory addresses

It's not a safe assumption for x86 at least. There are a few systems with
multiple ISA busses particularly older laptops with a docking station.

> when talking to an ISA device, so having multiple instances is
> already problematic.

PCMCIA devices handle it themselves so are ok. I'm not clear how the dual
PIIX4 configuration used in the older IBM laptop docks actually worked so
I assume the transaction went out of both bridges and providing one of
them responded the other kept silent as you simply stuffed the card into
the dock and it worked.

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

> -----Original Message-----
> From: One Thousand Gnomes [mailto:gnomes@lxorguk.ukuu.org.uk]
> Sent: 09 November 2016 13:55
> To: Arnd Bergmann
> Cc: Mark Rutland; Yuanzhichang; catalin.marinas@arm.com;
> will.deacon@arm.com; robh+dt@kernel.org; bhelgaas@google.com;
> olof@lixom.net; linux-arm-kernel@lists.infradead.org;
> lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;
> liviu.dudau@arm.com; zourongrong@gmail.com; John Garry; Gabriele
> Paoloni; zhichang.yuan02@gmail.com; kantyzc@163.com; xuwei (O);
> marc.zyngier@arm.com
> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> special ISA
> 
> > I think it is a relatively safe assumption that there is only one
> > ISA bridge. A lot of old drivers hardcode PIO or memory addresses
> 
> It's not a safe assumption for x86 at least. There are a few systems
> with
> multiple ISA busses particularly older laptops with a docking station.

Mmmm right...now the point is that this kind of special devices appearing
as a special ISA bus will probably never appear on x86 platforms (I guess).

So maybe it is a safe assumption because of this...?

Thanks

Gab

> 
> > when talking to an ISA device, so having multiple instances is
> > already problematic.
> 
> PCMCIA devices handle it themselves so are ok. I'm not clear how the
> dual
> PIIX4 configuration used in the older IBM laptop docks actually worked
> so
> I assume the transaction went out of both bridges and providing one of
> them responded the other kept silent as you simply stuffed the card
> into
> the dock and it worked.
> 
> Alan
--
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 Liviu

Thanks for reviewing

> -----Original Message-----

> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]

> Sent: 09 November 2016 11:40

> To: Yuanzhichang

> Cc: catalin.marinas@arm.com; will.deacon@arm.com; robh+dt@kernel.org;

> bhelgaas@google.com; mark.rutland@arm.com; olof@lixom.net;

> arnd@arndb.de; linux-arm-kernel@lists.infradead.org;

> lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;

> devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-

> serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;

> zourongrong@gmail.com; John Garry; Gabriele Paoloni;

> zhichang.yuan02@gmail.com; kantyzc@163.com; xuwei (O)

> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for

> special ISA

> 

> On Tue, Nov 08, 2016 at 11:47:08AM +0800, zhichang.yuan wrote:

> > This patch solves two issues:

> > 1) parse and get the right I/O range from DTS node whose parent does

> not

> > define the corresponding ranges property;

> >

> > There are some special ISA/LPC devices that work on a specific I/O

> range where

> > it is not correct to specify a ranges property in DTS parent node as

> cpu

> > addresses translated from DTS node are only for memory space on some

> > architectures, such as Arm64. Without the parent 'ranges' property,

> current

> > of_translate_address() return an error.

> > Here we add a fixup function, of_get_isa_indirect_io(). During the OF

> address

> > translation, this fixup will be called to check the 'reg' address to

> be

> > translating is for those sepcial ISA/LPC devices and get the I/O

> range

> > directly from the 'reg' property.

> >

> > 2) eliminate the I/O range conflict risk with PCI/PCIE leagecy I/O

> device;

> >

> > The current __of_address_to_resource() always translates the I/O

> range to PIO.

> > But this processing is not suitable for our ISA/LPC devices whose I/O

> range is

> > not cpu address(Arnd had stressed this in his comments on V2,V3

> patch-set).

> > Here, we bypass the mapping between cpu address and PIO for the

> special

> > ISA/LPC devices. But to drive these ISA/LPC devices, a I/O port

> address below

> > PCIBIOS_MIN_IO is needed by in*/out*(). Which means there is conflict

> risk

> > between I/O range of [0, PCIBIOS_MIN_IO) and PCI/PCIE legacy I/O

> range of [0,

> > IO_SPACE_LIMIT).

> > To avoid the I/O conflict, this patch reserve the I/O range below

> > PCIBIOS_MIN_IO.

> >

> > Signed-off-by: zhichang.yuan <yuanzhichang@hisilicon.com>

> > Signed-off-by: Gabriele Paoloni <gabriele.paoloni@huawei.com>

> > ---

> >  .../arm/hisilicon/hisilicon-low-pin-count.txt      | 31 ++++++++++++

> >  arch/arm64/include/asm/io.h                        |  6 +++

> >  arch/arm64/kernel/extio.c                          | 25 ++++++++++

> >  drivers/of/address.c                               | 56

> +++++++++++++++++++++-

> >  drivers/pci/pci.c                                  |  6 +--

> >  include/linux/of_address.h                         | 17 +++++++

> >  include/linux/pci.h                                |  8 ++++

> >  7 files changed, 145 insertions(+), 4 deletions(-)

> >  create mode 100644

> Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-

> count.txt

> >

> > diff --git

> a/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-pin-

> count.txt b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-

> low-pin-count.txt

> > new file mode 100644

> > index 0000000..13c8ddd

> > --- /dev/null

> > +++ b/Documentation/devicetree/bindings/arm/hisilicon/hisilicon-low-

> pin-count.txt

> > @@ -0,0 +1,31 @@

> > +Hisilicon Hip06 low-pin-count device

> > +  Usually LPC controller is part of PCI host bridge, so the legacy

> ISA ports

> > +  locate on LPC bus can be accessed direclty. But some SoCs have

> independent

> > +  LPC controller, and access the legacy ports by triggering LPC I/O

> cycles.

> > +  Hisilicon Hip06 implements this LPC device.

> > +

> > +Required properties:

> > +- compatible: should be "hisilicon,low-pin-count"

> > +- #address-cells: must be 2 which stick to the ISA/EISA binding doc.

> > +- #size-cells: must be 1 which stick to the ISA/EISA binding doc.

> > +- reg: base memory range where the register set of this device is

> mapped.

> > +

> > +Note:

> > +  The node name before '@' must be "isa" to represent the binding

> stick to the

> > +  ISA/EISA binding specification.

> > +

> > +Example:

> > +

> > +isa@a01b0000 {

> > +	compatible = "hisilicom,low-pin-count";

> > +	#address-cells = <2>;

> > +	#size-cells = <1>;

> > +	reg = <0x0 0xa01b0000 0x0 0x1000>;

> > +

> > +	ipmi0: bt@e4 {

> > +		compatible = "ipmi-bt";

> > +		device_type = "ipmi";

> > +		reg = <0x01 0xe4 0x04>;

> > +		status = "disabled";

> > +	};

> > +};

> 

> 

> This documentation file needs to be part of the next patch. It has

> nothing to do with

> what you are trying to fix here.


Yes you're right...we'll move it to next one

> 

> 

> > diff --git a/arch/arm64/include/asm/io.h

> b/arch/arm64/include/asm/io.h

> > index 136735d..c26b7cc 100644

> > --- a/arch/arm64/include/asm/io.h

> > +++ b/arch/arm64/include/asm/io.h

> > @@ -175,6 +175,12 @@ static inline u64 __raw_readq(const volatile

> void __iomem *addr)

> >  #define outsl outsl

> >

> >  DECLARE_EXTIO(l, u32)

> > +

> > +#define indirect_io_enabled indirect_io_enabled

> > +extern bool indirect_io_enabled(void);

> > +

> > +#define addr_is_indirect_io addr_is_indirect_io

> > +extern int addr_is_indirect_io(u64 taddr);

> >  #endif

> >

> >

> > diff --git a/arch/arm64/kernel/extio.c b/arch/arm64/kernel/extio.c

> > index 647b3fa..3d45fa8 100644

> > --- a/arch/arm64/kernel/extio.c

> > +++ b/arch/arm64/kernel/extio.c

> > @@ -19,6 +19,31 @@

> >

> >  struct extio_ops *arm64_extio_ops;

> >

> > +/**

> > + * indirect_io_enabled - check whether indirectIO is enabled.

> > + *	arm64_extio_ops will be set only when indirectIO mechanism had

> been

> > + *	initialized.

> > + *

> > + * Returns true when indirectIO is enabled.

> > + */

> > +bool indirect_io_enabled(void)

> > +{

> > +	return arm64_extio_ops ? true : false;

> > +}

> > +

> > +/**

> > + * addr_is_indirect_io - check whether the input taddr is for

> indirectIO.

> > + * @taddr: the io address to be checked.

> > + *

> > + * Returns 1 when taddr is in the range; otherwise return 0.

> > + */

> > +int addr_is_indirect_io(u64 taddr)

> > +{

> > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <

> taddr)

> 

> start >= taddr ?


Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
then taddr is outside the range [start; end] and will return 0; otherwise
it will return 1...

> 

> > +		return 0;

> > +

> > +	return 1;

> > +}

> >

> >  BUILD_EXTIO(b, u8)

> >

> > diff --git a/drivers/of/address.c b/drivers/of/address.c

> > index 02b2903..cc2a05d 100644

> > --- a/drivers/of/address.c

> > +++ b/drivers/of/address.c

> > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct

> device_node *np)

> >  	return false;

> >  }

> >

> > +

> > +/*

> > + * of_isa_indirect_io - get the IO address from some isa reg

> property value.

> > + *	For some isa/lpc devices, no ranges property in ancestor node.

> > + *	The device addresses are described directly in their regs

> property.

> > + *	This fixup function will be called to get the IO address of

> isa/lpc

> > + *	devices when the normal of_translation failed.

> > + *

> > + * @parent:	points to the parent dts node;

> > + * @bus:		points to the of_bus which can be used to parse

> address;

> > + * @addr:	the address from reg property;

> > + * @na:		the address cell counter of @addr;

> > + * @presult:	store the address paresed from @addr;

> > + *

> > + * return 1 when successfully get the I/O address;

> > + * 0 will return for some failures.

> 

> Bah, you are returning a signed int, why 0 for failure? Return a

> negative value with

> error codes. Otherwise change the return value into a bool.


Yes we'll move to bool

> 

> > + */

> > +static int of_get_isa_indirect_io(struct device_node *parent,

> > +				struct of_bus *bus, __be32 *addr,

> > +				int na, u64 *presult)

> > +{

> > +	unsigned int flags;

> > +	unsigned int rlen;

> > +

> > +	/* whether support indirectIO */

> > +	if (!indirect_io_enabled())

> > +		return 0;

> > +

> > +	if (!of_bus_isa_match(parent))

> > +		return 0;

> > +

> > +	flags = bus->get_flags(addr);

> > +	if (!(flags & IORESOURCE_IO))

> > +		return 0;

> > +

> > +	/* there is ranges property, apply the normal translation

> directly. */

> 

> s/there is ranges/if we have a 'ranges'/


Thanks for spotting this

> 

> > +	if (of_get_property(parent, "ranges", &rlen))

> > +		return 0;

> > +

> > +	*presult = of_read_number(addr + 1, na - 1);

> > +	/* this fixup is only valid for specific I/O range. */

> > +	return addr_is_indirect_io(*presult);

> > +}

> > +

> >  static int of_translate_one(struct device_node *parent, struct

> of_bus *bus,

> >  			    struct of_bus *pbus, __be32 *addr,

> >  			    int na, int ns, int pna, const char *rprop)

> > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct

> device_node *dev,

> >  			result = of_read_number(addr, na);

> >  			break;

> >  		}

> > +		/*

> > +		 * For indirectIO device which has no ranges property, get

> > +		 * the address from reg directly.

> > +		 */

> > +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {

> > +			pr_debug("isa indirectIO matched(%s)..addr =

> 0x%llx\n",

> > +				of_node_full_name(dev), result);

> > +			break;

> > +		}

> >

> >  		/* Get new parent bus and counts */

> >  		pbus = of_match_bus(parent);

> > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct

> device_node *dev,

> >  	if (taddr == OF_BAD_ADDR)

> >  		return -EINVAL;

> >  	memset(r, 0, sizeof(struct resource));

> > -	if (flags & IORESOURCE_IO) {

> > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {

> >  		unsigned long port;

> > +

> >  		port = pci_address_to_pio(taddr);

> >  		if (port == (unsigned long)-1)

> >  			return -EINVAL;

> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c

> > index ba34907..1a08511 100644

> > --- a/drivers/pci/pci.c

> > +++ b/drivers/pci/pci.c

> > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t

> addr, resource_size_t size)

> >

> >  #ifdef PCI_IOBASE

> >  	struct io_range *range;

> > -	resource_size_t allocated_size = 0;

> > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;

> >

> >  	/* check if the range hasn't been previously recorded */

> >  	spin_lock(&io_range_lock);

> > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long

> pio)

> >

> >  #ifdef PCI_IOBASE

> >  	struct io_range *range;

> > -	resource_size_t allocated_size = 0;

> > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;

> 

> Have you checked that pci_pio_to_address still returns valid values

> after this? I know that

> you are trying to take into account PCIBIOS_MIN_IO limit when

> allocating reserving the IO ranges,

> but the values added in the io_range_list are still starting from zero,

> no from PCIBIOS_MIN_IO,


I think you're wrong here as in pci_address_to_pio we have:
+	resource_size_t offset = PCIBIOS_MIN_IO;

This should be enough to guarantee that the PIOs start at
PCIBIOS_MIN_IO...right?


> so the calculation of the address in this function could return

> negative values casted to pci_addr_t.

> 

> Maybe you want to adjust the range->start value in

> pci_register_io_range() as well to have it

> offset by PCIBIOS_MIN_IO as well.

> 

> Best regards,

> Liviu

> 

> >

> >  	if (pio > IO_SPACE_LIMIT)

> >  		return address;

> > @@ -3335,7 +3335,7 @@ unsigned long __weak

> pci_address_to_pio(phys_addr_t address)

> >  {

> >  #ifdef PCI_IOBASE

> >  	struct io_range *res;

> > -	resource_size_t offset = 0;

> > +	resource_size_t offset = PCIBIOS_MIN_IO;

> >  	unsigned long addr = -1;

> >

> >  	spin_lock(&io_range_lock);

> > diff --git a/include/linux/of_address.h b/include/linux/of_address.h

> > index 3786473..deec469 100644

> > --- a/include/linux/of_address.h

> > +++ b/include/linux/of_address.h

> > @@ -24,6 +24,23 @@ struct of_pci_range {

> >  #define for_each_of_pci_range(parser, range) \

> >  	for (; of_pci_range_parser_one(parser, range);)

> >

> > +

> > +#ifndef indirect_io_enabled

> > +#define indirect_io_enabled indirect_io_enabled

> > +static inline bool indirect_io_enabled(void)

> > +{

> > +	return false;

> > +}

> > +#endif

> > +

> > +#ifndef addr_is_indirect_io

> > +#define addr_is_indirect_io addr_is_indirect_io

> > +static inline int addr_is_indirect_io(u64 taddr)

> > +{

> > +	return 0;

> > +}

> > +#endif

> > +

> >  /* Translate a DMA address from device space to CPU space */

> >  extern u64 of_translate_dma_address(struct device_node *dev,

> >  				    const __be32 *in_addr);

> > diff --git a/include/linux/pci.h b/include/linux/pci.h

> > index 0e49f70..7f6bbb6 100644

> > --- a/include/linux/pci.h

> > +++ b/include/linux/pci.h

> > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct

> pci_bus *bus)

> >  /* provide the legacy pci_dma_* API */

> >  #include <linux/pci-dma-compat.h>

> >

> > +/*

> > + * define this macro here to refrain from compilation error for some

> > + * platforms. Please keep this macro at the end of this header file.

> > + */

> > +#ifndef PCIBIOS_MIN_IO

> > +#define PCIBIOS_MIN_IO		0

> > +#endif

> > +

> >  #endif /* LINUX_PCI_H */

> > --

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

> 

> --

> ====================

> | I would like to |

> | fix the world,  |

> | but they're not |

> | giving me the   |

>  \ source code!  /

>   ---------------

>     ¯\_(ツ)_/¯

On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
> 
> Thanks for reviewing
> 

[removed some irrelevant part of discussion, avoid crazy formatting]

> > > +/**
> > > + * addr_is_indirect_io - check whether the input taddr is for
> > indirectIO.
> > > + * @taddr: the io address to be checked.
> > > + *
> > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > + */
> > > +int addr_is_indirect_io(u64 taddr)
> > > +{
> > > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
> > taddr)
> > 
> > start >= taddr ?
> 
> Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
> then taddr is outside the range [start; end] and will return 0; otherwise
> it will return 1...

Oops, sorry, did not pay attention to the returned value. The check is
correct as it is, no need to change then.

> 
> > 
> > > +		return 0;
> > > +
> > > +	return 1;
> > > +}
> > >
> > >  BUILD_EXTIO(b, u8)
> > >
> > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > index 02b2903..cc2a05d 100644
> > > --- a/drivers/of/address.c
> > > +++ b/drivers/of/address.c
> > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > device_node *np)
> > >  	return false;
> > >  }
> > >
> > > +
> > > +/*
> > > + * of_isa_indirect_io - get the IO address from some isa reg
> > property value.
> > > + *	For some isa/lpc devices, no ranges property in ancestor node.
> > > + *	The device addresses are described directly in their regs
> > property.
> > > + *	This fixup function will be called to get the IO address of
> > isa/lpc
> > > + *	devices when the normal of_translation failed.
> > > + *
> > > + * @parent:	points to the parent dts node;
> > > + * @bus:		points to the of_bus which can be used to parse
> > address;
> > > + * @addr:	the address from reg property;
> > > + * @na:		the address cell counter of @addr;
> > > + * @presult:	store the address paresed from @addr;
> > > + *
> > > + * return 1 when successfully get the I/O address;
> > > + * 0 will return for some failures.
> > 
> > Bah, you are returning a signed int, why 0 for failure? Return a
> > negative value with
> > error codes. Otherwise change the return value into a bool.
> 
> Yes we'll move to bool
> 
> > 
> > > + */
> > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > +				struct of_bus *bus, __be32 *addr,
> > > +				int na, u64 *presult)
> > > +{
> > > +	unsigned int flags;
> > > +	unsigned int rlen;
> > > +
> > > +	/* whether support indirectIO */
> > > +	if (!indirect_io_enabled())
> > > +		return 0;
> > > +
> > > +	if (!of_bus_isa_match(parent))
> > > +		return 0;
> > > +
> > > +	flags = bus->get_flags(addr);
> > > +	if (!(flags & IORESOURCE_IO))
> > > +		return 0;
> > > +
> > > +	/* there is ranges property, apply the normal translation
> > directly. */
> > 
> > s/there is ranges/if we have a 'ranges'/
> 
> Thanks for spotting this
> 
> > 
> > > +	if (of_get_property(parent, "ranges", &rlen))
> > > +		return 0;
> > > +
> > > +	*presult = of_read_number(addr + 1, na - 1);
> > > +	/* this fixup is only valid for specific I/O range. */
> > > +	return addr_is_indirect_io(*presult);
> > > +}
> > > +
> > >  static int of_translate_one(struct device_node *parent, struct
> > of_bus *bus,
> > >  			    struct of_bus *pbus, __be32 *addr,
> > >  			    int na, int ns, int pna, const char *rprop)
> > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > device_node *dev,
> > >  			result = of_read_number(addr, na);
> > >  			break;
> > >  		}
> > > +		/*
> > > +		 * For indirectIO device which has no ranges property, get
> > > +		 * the address from reg directly.
> > > +		 */
> > > +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
> > > +			pr_debug("isa indirectIO matched(%s)..addr =
> > 0x%llx\n",
> > > +				of_node_full_name(dev), result);
> > > +			break;
> > > +		}
> > >
> > >  		/* Get new parent bus and counts */
> > >  		pbus = of_match_bus(parent);
> > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > device_node *dev,
> > >  	if (taddr == OF_BAD_ADDR)
> > >  		return -EINVAL;
> > >  	memset(r, 0, sizeof(struct resource));
> > > -	if (flags & IORESOURCE_IO) {
> > > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > >  		unsigned long port;
> > > +
> > >  		port = pci_address_to_pio(taddr);
> > >  		if (port == (unsigned long)-1)
> > >  			return -EINVAL;
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index ba34907..1a08511 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
> > addr, resource_size_t size)
> > >
> > >  #ifdef PCI_IOBASE
> > >  	struct io_range *range;
> > > -	resource_size_t allocated_size = 0;
> > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > >
> > >  	/* check if the range hasn't been previously recorded */
> > >  	spin_lock(&io_range_lock);
> > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
> > pio)
> > >
> > >  #ifdef PCI_IOBASE
> > >  	struct io_range *range;
> > > -	resource_size_t allocated_size = 0;
> > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > 
> > Have you checked that pci_pio_to_address still returns valid values
> > after this? I know that
> > you are trying to take into account PCIBIOS_MIN_IO limit when
> > allocating reserving the IO ranges,
> > but the values added in the io_range_list are still starting from zero,
> > no from PCIBIOS_MIN_IO,
> 
> I think you're wrong here as in pci_address_to_pio we have:
> +	resource_size_t offset = PCIBIOS_MIN_IO;
> 
> This should be enough to guarantee that the PIOs start at
> PCIBIOS_MIN_IO...right?

I don't think you can guarantee that the pio value that gets passed into
pci_pio_to_address() always comes from a previously returned value by
pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()

	if (pio < PCIBIOS_MIN_IO)
		return address;

to avoid adding more checks in the list_for_each_entry() loop.

Best regards,
Liviu

> 
> 
> > so the calculation of the address in this function could return
> > negative values casted to pci_addr_t.
> > 
> > Maybe you want to adjust the range->start value in
> > pci_register_io_range() as well to have it
> > offset by PCIBIOS_MIN_IO as well.
> > 
> > Best regards,
> > Liviu
> > 
> > >
> > >  	if (pio > IO_SPACE_LIMIT)
> > >  		return address;
> > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > pci_address_to_pio(phys_addr_t address)
> > >  {
> > >  #ifdef PCI_IOBASE
> > >  	struct io_range *res;
> > > -	resource_size_t offset = 0;
> > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > >  	unsigned long addr = -1;
> > >
> > >  	spin_lock(&io_range_lock);
> > > diff --git a/include/linux/of_address.h b/include/linux/of_address.h
> > > index 3786473..deec469 100644
> > > --- a/include/linux/of_address.h
> > > +++ b/include/linux/of_address.h
> > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > >  #define for_each_of_pci_range(parser, range) \
> > >  	for (; of_pci_range_parser_one(parser, range);)
> > >
> > > +
> > > +#ifndef indirect_io_enabled
> > > +#define indirect_io_enabled indirect_io_enabled
> > > +static inline bool indirect_io_enabled(void)
> > > +{
> > > +	return false;
> > > +}
> > > +#endif
> > > +
> > > +#ifndef addr_is_indirect_io
> > > +#define addr_is_indirect_io addr_is_indirect_io
> > > +static inline int addr_is_indirect_io(u64 taddr)
> > > +{
> > > +	return 0;
> > > +}
> > > +#endif
> > > +
> > >  /* Translate a DMA address from device space to CPU space */
> > >  extern u64 of_translate_dma_address(struct device_node *dev,
> > >  				    const __be32 *in_addr);
> > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > index 0e49f70..7f6bbb6 100644
> > > --- a/include/linux/pci.h
> > > +++ b/include/linux/pci.h
> > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > pci_bus *bus)
> > >  /* provide the legacy pci_dma_* API */
> > >  #include <linux/pci-dma-compat.h>
> > >
> > > +/*
> > > + * define this macro here to refrain from compilation error for some
> > > + * platforms. Please keep this macro at the end of this header file.
> > > + */
> > > +#ifndef PCIBIOS_MIN_IO
> > > +#define PCIBIOS_MIN_IO		0
> > > +#endif
> > > +
> > >  #endif /* LINUX_PCI_H */
> > > --
> > > 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 Wednesday, November 9, 2016 1:54:53 PM CET One Thousand Gnomes wrote:
> > I think it is a relatively safe assumption that there is only one
> > ISA bridge. A lot of old drivers hardcode PIO or memory addresses
> 
> It's not a safe assumption for x86 at least. There are a few systems with
> multiple ISA busses particularly older laptops with a docking station.

But do they have multiple ISA domains? There is no real harm in supporting
it, the (small) downsides I can think of are:

- a few extra cycles for the lookup, from possibly walking a linked list
  to find the correct set of helpers and MMIO addresses
- making it too general could invite more people to design hardware
  around the infrastructure when we really want them to stop adding
  stuff like this.

	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,Liviu,

Thanks for your comments!


On 2016/11/10 0:50, liviu.dudau@arm.com wrote:
> On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
>> Hi Liviu
>>
>> Thanks for reviewing
>>
> 
> [removed some irrelevant part of discussion, avoid crazy formatting]
> 
>>>> +/**
>>>> + * addr_is_indirect_io - check whether the input taddr is for
>>> indirectIO.
>>>> + * @taddr: the io address to be checked.
>>>> + *
>>>> + * Returns 1 when taddr is in the range; otherwise return 0.
>>>> + */
>>>> +int addr_is_indirect_io(u64 taddr)
>>>> +{
>>>> +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end <
>>> taddr)
>>>
>>> start >= taddr ?
>>
>> Nope... if  (taddr < arm64_extio_ops->start || taddr > arm64_extio_ops->end)
>> then taddr is outside the range [start; end] and will return 0; otherwise
>> it will return 1...
> 
> Oops, sorry, did not pay attention to the returned value. The check is
> correct as it is, no need to change then.
> 
>>
>>>
>>>> +		return 0;
>>>> +
>>>> +	return 1;
>>>> +}
>>>>
>>>>  BUILD_EXTIO(b, u8)
>>>>
>>>> diff --git a/drivers/of/address.c b/drivers/of/address.c
>>>> index 02b2903..cc2a05d 100644
>>>> --- a/drivers/of/address.c
>>>> +++ b/drivers/of/address.c
>>>> @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
>>> device_node *np)
>>>>  	return false;
>>>>  }
>>>>
>>>> +
>>>> +/*
>>>> + * of_isa_indirect_io - get the IO address from some isa reg
>>> property value.
>>>> + *	For some isa/lpc devices, no ranges property in ancestor node.
>>>> + *	The device addresses are described directly in their regs
>>> property.
>>>> + *	This fixup function will be called to get the IO address of
>>> isa/lpc
>>>> + *	devices when the normal of_translation failed.
>>>> + *
>>>> + * @parent:	points to the parent dts node;
>>>> + * @bus:		points to the of_bus which can be used to parse
>>> address;
>>>> + * @addr:	the address from reg property;
>>>> + * @na:		the address cell counter of @addr;
>>>> + * @presult:	store the address paresed from @addr;
>>>> + *
>>>> + * return 1 when successfully get the I/O address;
>>>> + * 0 will return for some failures.
>>>
>>> Bah, you are returning a signed int, why 0 for failure? Return a
>>> negative value with
>>> error codes. Otherwise change the return value into a bool.
>>
>> Yes we'll move to bool
>>
>>>
>>>> + */
>>>> +static int of_get_isa_indirect_io(struct device_node *parent,
>>>> +				struct of_bus *bus, __be32 *addr,
>>>> +				int na, u64 *presult)
>>>> +{
>>>> +	unsigned int flags;
>>>> +	unsigned int rlen;
>>>> +
>>>> +	/* whether support indirectIO */
>>>> +	if (!indirect_io_enabled())
>>>> +		return 0;
>>>> +
>>>> +	if (!of_bus_isa_match(parent))
>>>> +		return 0;
>>>> +
>>>> +	flags = bus->get_flags(addr);
>>>> +	if (!(flags & IORESOURCE_IO))
>>>> +		return 0;
>>>> +
>>>> +	/* there is ranges property, apply the normal translation
>>> directly. */
>>>
>>> s/there is ranges/if we have a 'ranges'/
>>
>> Thanks for spotting this
>>
>>>
>>>> +	if (of_get_property(parent, "ranges", &rlen))
>>>> +		return 0;
>>>> +
>>>> +	*presult = of_read_number(addr + 1, na - 1);
>>>> +	/* this fixup is only valid for specific I/O range. */
>>>> +	return addr_is_indirect_io(*presult);
>>>> +}
>>>> +
>>>>  static int of_translate_one(struct device_node *parent, struct
>>> of_bus *bus,
>>>>  			    struct of_bus *pbus, __be32 *addr,
>>>>  			    int na, int ns, int pna, const char *rprop)
>>>> @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
>>> device_node *dev,
>>>>  			result = of_read_number(addr, na);
>>>>  			break;
>>>>  		}
>>>> +		/*
>>>> +		 * For indirectIO device which has no ranges property, get
>>>> +		 * the address from reg directly.
>>>> +		 */
>>>> +		if (of_get_isa_indirect_io(dev, bus, addr, na, &result)) {
>>>> +			pr_debug("isa indirectIO matched(%s)..addr =
>>> 0x%llx\n",
>>>> +				of_node_full_name(dev), result);
>>>> +			break;
>>>> +		}
>>>>
>>>>  		/* Get new parent bus and counts */
>>>>  		pbus = of_match_bus(parent);
>>>> @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
>>> device_node *dev,
>>>>  	if (taddr == OF_BAD_ADDR)
>>>>  		return -EINVAL;
>>>>  	memset(r, 0, sizeof(struct resource));
>>>> -	if (flags & IORESOURCE_IO) {
>>>> +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
>>>>  		unsigned long port;
>>>> +
>>>>  		port = pci_address_to_pio(taddr);
>>>>  		if (port == (unsigned long)-1)
>>>>  			return -EINVAL;
>>>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>>>> index ba34907..1a08511 100644
>>>> --- a/drivers/pci/pci.c
>>>> +++ b/drivers/pci/pci.c
>>>> @@ -3263,7 +3263,7 @@ int __weak pci_register_io_range(phys_addr_t
>>> addr, resource_size_t size)
>>>>
>>>>  #ifdef PCI_IOBASE
>>>>  	struct io_range *range;
>>>> -	resource_size_t allocated_size = 0;
>>>> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>>>
>>>>  	/* check if the range hasn't been previously recorded */
>>>>  	spin_lock(&io_range_lock);
>>>> @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned long
>>> pio)
>>>>
>>>>  #ifdef PCI_IOBASE
>>>>  	struct io_range *range;
>>>> -	resource_size_t allocated_size = 0;
>>>> +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
>>>
>>> Have you checked that pci_pio_to_address still returns valid values
>>> after this? I know that
>>> you are trying to take into account PCIBIOS_MIN_IO limit when
>>> allocating reserving the IO ranges,
>>> but the values added in the io_range_list are still starting from zero,
>>> no from PCIBIOS_MIN_IO,
>>
>> I think you're wrong here as in pci_address_to_pio we have:
>> +	resource_size_t offset = PCIBIOS_MIN_IO;
>>
>> This should be enough to guarantee that the PIOs start at
>> PCIBIOS_MIN_IO...right?
> 
> I don't think you can guarantee that the pio value that gets passed into
> pci_pio_to_address() always comes from a previously returned value by
> pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
> 
> 	if (pio < PCIBIOS_MIN_IO)
> 		return address;
> 
> to avoid adding more checks in the list_for_each_entry() loop.
> 

I will register some ranges to the list and test it later.

But from my understanding, pci_pio_to_address() should can return the right
original physical address.


According to the algorithm, the output PIO ranges are consecutive， just like this:


					input pio of pci_pio_to_address()
						|
						V
|----------------|--------------------------|------|-----------|
					    ^
					    |
					allocated_size is here


The change of this patch just make the start PIO from ZERO to PCIBIOS_MIN_IO.

in pci_pio_to_address(), for one input pio which fall into any PIO segment, the
return address will be:

address = range->start + pio - allocated_size;

Since allocated_size is the total range size of all IO ranges before the one
where pio belong, then (pio - allocated_size) is the offset to the range start,
So....


Thanks!
Zhichang

> Best regards,
> Liviu
> 
>>
>>
>>> so the calculation of the address in this function could return
>>> negative values casted to pci_addr_t.
>>>
>>> Maybe you want to adjust the range->start value in
>>> pci_register_io_range() as well to have it
>>> offset by PCIBIOS_MIN_IO as well.
>>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>>  	if (pio > IO_SPACE_LIMIT)
>>>>  		return address;
>>>> @@ -3335,7 +3335,7 @@ unsigned long __weak
>>> pci_address_to_pio(phys_addr_t address)
>>>>  {
>>>>  #ifdef PCI_IOBASE
>>>>  	struct io_range *res;
>>>> -	resource_size_t offset = 0;
>>>> +	resource_size_t offset = PCIBIOS_MIN_IO;
>>>>  	unsigned long addr = -1;
>>>>
>>>>  	spin_lock(&io_range_lock);
>>>> diff --git a/include/linux/of_address.h b/include/linux/of_address.h
>>>> index 3786473..deec469 100644
>>>> --- a/include/linux/of_address.h
>>>> +++ b/include/linux/of_address.h
>>>> @@ -24,6 +24,23 @@ struct of_pci_range {
>>>>  #define for_each_of_pci_range(parser, range) \
>>>>  	for (; of_pci_range_parser_one(parser, range);)
>>>>
>>>> +
>>>> +#ifndef indirect_io_enabled
>>>> +#define indirect_io_enabled indirect_io_enabled
>>>> +static inline bool indirect_io_enabled(void)
>>>> +{
>>>> +	return false;
>>>> +}
>>>> +#endif
>>>> +
>>>> +#ifndef addr_is_indirect_io
>>>> +#define addr_is_indirect_io addr_is_indirect_io
>>>> +static inline int addr_is_indirect_io(u64 taddr)
>>>> +{
>>>> +	return 0;
>>>> +}
>>>> +#endif
>>>> +
>>>>  /* Translate a DMA address from device space to CPU space */
>>>>  extern u64 of_translate_dma_address(struct device_node *dev,
>>>>  				    const __be32 *in_addr);
>>>> diff --git a/include/linux/pci.h b/include/linux/pci.h
>>>> index 0e49f70..7f6bbb6 100644
>>>> --- a/include/linux/pci.h
>>>> +++ b/include/linux/pci.h
>>>> @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
>>> pci_bus *bus)
>>>>  /* provide the legacy pci_dma_* API */
>>>>  #include <linux/pci-dma-compat.h>
>>>>
>>>> +/*
>>>> + * define this macro here to refrain from compilation error for some
>>>> + * platforms. Please keep this macro at the end of this header file.
>>>> + */
>>>> +#ifndef PCIBIOS_MIN_IO
>>>> +#define PCIBIOS_MIN_IO		0
>>>> +#endif
>>>> +
>>>>  #endif /* LINUX_PCI_H */
>>>> --
>>>> 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
> 

--
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, 2016-11-09 at 11:20 +0000, Mark Rutland wrote:
> The big change would be to handle !MMIO translations, for which we'd
> need a runtime registry of ISA bus instance to find the relevant
> accessor ops and instance-specific data.

Yes. We do something a bit like that on ppc, we find the PCI bus
for which the IO ports match and I have a hook to register the
special indirect ISA.

It's a bit messy, we could do something nicer generically.

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 Liviu

> -----Original Message-----

> From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]

> Sent: 09 November 2016 16:51

> To: Gabriele Paoloni

> Cc: Yuanzhichang; catalin.marinas@arm.com; will.deacon@arm.com;

> robh+dt@kernel.org; bhelgaas@google.com; mark.rutland@arm.com;

> olof@lixom.net; arnd@arndb.de; linux-arm-kernel@lists.infradead.org;

> lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;

> devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-

> serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;

> zourongrong@gmail.com; John Garry; zhichang.yuan02@gmail.com;

> kantyzc@163.com; xuwei (O)

> Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for

> special ISA

> 

> On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:

> > Hi Liviu

> >

> > Thanks for reviewing

> >

> 

> [removed some irrelevant part of discussion, avoid crazy formatting]

> 

> > > > +/**

> > > > + * addr_is_indirect_io - check whether the input taddr is for

> > > indirectIO.

> > > > + * @taddr: the io address to be checked.

> > > > + *

> > > > + * Returns 1 when taddr is in the range; otherwise return 0.

> > > > + */

> > > > +int addr_is_indirect_io(u64 taddr)

> > > > +{

> > > > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end

> <

> > > taddr)

> > >

> > > start >= taddr ?

> >

> > Nope... if  (taddr < arm64_extio_ops->start || taddr >

> arm64_extio_ops->end)

> > then taddr is outside the range [start; end] and will return 0;

> otherwise

> > it will return 1...

> 

> Oops, sorry, did not pay attention to the returned value. The check is

> correct as it is, no need to change then.

> 

> >

> > >

> > > > +		return 0;

> > > > +

> > > > +	return 1;

> > > > +}

> > > >

> > > >  BUILD_EXTIO(b, u8)

> > > >

> > > > diff --git a/drivers/of/address.c b/drivers/of/address.c

> > > > index 02b2903..cc2a05d 100644

> > > > --- a/drivers/of/address.c

> > > > +++ b/drivers/of/address.c

> > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct

> > > device_node *np)

> > > >  	return false;

> > > >  }

> > > >

> > > > +

> > > > +/*

> > > > + * of_isa_indirect_io - get the IO address from some isa reg

> > > property value.

> > > > + *	For some isa/lpc devices, no ranges property in ancestor

> node.

> > > > + *	The device addresses are described directly in their regs

> > > property.

> > > > + *	This fixup function will be called to get the IO address of

> > > isa/lpc

> > > > + *	devices when the normal of_translation failed.

> > > > + *

> > > > + * @parent:	points to the parent dts node;

> > > > + * @bus:		points to the of_bus which can be used to parse

> > > address;

> > > > + * @addr:	the address from reg property;

> > > > + * @na:		the address cell counter of @addr;

> > > > + * @presult:	store the address paresed from @addr;

> > > > + *

> > > > + * return 1 when successfully get the I/O address;

> > > > + * 0 will return for some failures.

> > >

> > > Bah, you are returning a signed int, why 0 for failure? Return a

> > > negative value with

> > > error codes. Otherwise change the return value into a bool.

> >

> > Yes we'll move to bool

> >

> > >

> > > > + */

> > > > +static int of_get_isa_indirect_io(struct device_node *parent,

> > > > +				struct of_bus *bus, __be32 *addr,

> > > > +				int na, u64 *presult)

> > > > +{

> > > > +	unsigned int flags;

> > > > +	unsigned int rlen;

> > > > +

> > > > +	/* whether support indirectIO */

> > > > +	if (!indirect_io_enabled())

> > > > +		return 0;

> > > > +

> > > > +	if (!of_bus_isa_match(parent))

> > > > +		return 0;

> > > > +

> > > > +	flags = bus->get_flags(addr);

> > > > +	if (!(flags & IORESOURCE_IO))

> > > > +		return 0;

> > > > +

> > > > +	/* there is ranges property, apply the normal translation

> > > directly. */

> > >

> > > s/there is ranges/if we have a 'ranges'/

> >

> > Thanks for spotting this

> >

> > >

> > > > +	if (of_get_property(parent, "ranges", &rlen))

> > > > +		return 0;

> > > > +

> > > > +	*presult = of_read_number(addr + 1, na - 1);

> > > > +	/* this fixup is only valid for specific I/O range. */

> > > > +	return addr_is_indirect_io(*presult);

> > > > +}

> > > > +

> > > >  static int of_translate_one(struct device_node *parent, struct

> > > of_bus *bus,

> > > >  			    struct of_bus *pbus, __be32 *addr,

> > > >  			    int na, int ns, int pna, const char *rprop)

> > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct

> > > device_node *dev,

> > > >  			result = of_read_number(addr, na);

> > > >  			break;

> > > >  		}

> > > > +		/*

> > > > +		 * For indirectIO device which has no ranges

> property, get

> > > > +		 * the address from reg directly.

> > > > +		 */

> > > > +		if (of_get_isa_indirect_io(dev, bus, addr, na,

> &result)) {

> > > > +			pr_debug("isa indirectIO matched(%s)..addr =

> > > 0x%llx\n",

> > > > +				of_node_full_name(dev), result);

> > > > +			break;

> > > > +		}

> > > >

> > > >  		/* Get new parent bus and counts */

> > > >  		pbus = of_match_bus(parent);

> > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct

> > > device_node *dev,

> > > >  	if (taddr == OF_BAD_ADDR)

> > > >  		return -EINVAL;

> > > >  	memset(r, 0, sizeof(struct resource));

> > > > -	if (flags & IORESOURCE_IO) {

> > > > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {

> > > >  		unsigned long port;

> > > > +

> > > >  		port = pci_address_to_pio(taddr);

> > > >  		if (port == (unsigned long)-1)

> > > >  			return -EINVAL;

> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c

> > > > index ba34907..1a08511 100644

> > > > --- a/drivers/pci/pci.c

> > > > +++ b/drivers/pci/pci.c

> > > > @@ -3263,7 +3263,7 @@ int __weak

> pci_register_io_range(phys_addr_t

> > > addr, resource_size_t size)

> > > >

> > > >  #ifdef PCI_IOBASE

> > > >  	struct io_range *range;

> > > > -	resource_size_t allocated_size = 0;

> > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;

> > > >

> > > >  	/* check if the range hasn't been previously recorded */

> > > >  	spin_lock(&io_range_lock);

> > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned

> long

> > > pio)

> > > >

> > > >  #ifdef PCI_IOBASE

> > > >  	struct io_range *range;

> > > > -	resource_size_t allocated_size = 0;

> > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;

> > >

> > > Have you checked that pci_pio_to_address still returns valid values

> > > after this? I know that

> > > you are trying to take into account PCIBIOS_MIN_IO limit when

> > > allocating reserving the IO ranges,

> > > but the values added in the io_range_list are still starting from

> zero,

> > > no from PCIBIOS_MIN_IO,

> >

> > I think you're wrong here as in pci_address_to_pio we have:

> > +	resource_size_t offset = PCIBIOS_MIN_IO;

> >

> > This should be enough to guarantee that the PIOs start at

> > PCIBIOS_MIN_IO...right?

> 

> I don't think you can guarantee that the pio value that gets passed

> into

> pci_pio_to_address() always comes from a previously returned value by

> pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()


Maybe I am missing something...could you make an exampleof a case
where an IO toke doesn’t come from pci_address_to_pio() ?

Thanks

Gab


> 

> 	if (pio < PCIBIOS_MIN_IO)

> 		return address;

> 

> to avoid adding more checks in the list_for_each_entry() loop.

> 

> Best regards,

> Liviu

> 

> >

> >

> > > so the calculation of the address in this function could return

> > > negative values casted to pci_addr_t.

> > >

> > > Maybe you want to adjust the range->start value in

> > > pci_register_io_range() as well to have it

> > > offset by PCIBIOS_MIN_IO as well.

> > >

> > > Best regards,

> > > Liviu

> > >

> > > >

> > > >  	if (pio > IO_SPACE_LIMIT)

> > > >  		return address;

> > > > @@ -3335,7 +3335,7 @@ unsigned long __weak

> > > pci_address_to_pio(phys_addr_t address)

> > > >  {

> > > >  #ifdef PCI_IOBASE

> > > >  	struct io_range *res;

> > > > -	resource_size_t offset = 0;

> > > > +	resource_size_t offset = PCIBIOS_MIN_IO;

> > > >  	unsigned long addr = -1;

> > > >

> > > >  	spin_lock(&io_range_lock);

> > > > diff --git a/include/linux/of_address.h

> b/include/linux/of_address.h

> > > > index 3786473..deec469 100644

> > > > --- a/include/linux/of_address.h

> > > > +++ b/include/linux/of_address.h

> > > > @@ -24,6 +24,23 @@ struct of_pci_range {

> > > >  #define for_each_of_pci_range(parser, range) \

> > > >  	for (; of_pci_range_parser_one(parser, range);)

> > > >

> > > > +

> > > > +#ifndef indirect_io_enabled

> > > > +#define indirect_io_enabled indirect_io_enabled

> > > > +static inline bool indirect_io_enabled(void)

> > > > +{

> > > > +	return false;

> > > > +}

> > > > +#endif

> > > > +

> > > > +#ifndef addr_is_indirect_io

> > > > +#define addr_is_indirect_io addr_is_indirect_io

> > > > +static inline int addr_is_indirect_io(u64 taddr)

> > > > +{

> > > > +	return 0;

> > > > +}

> > > > +#endif

> > > > +

> > > >  /* Translate a DMA address from device space to CPU space */

> > > >  extern u64 of_translate_dma_address(struct device_node *dev,

> > > >  				    const __be32 *in_addr);

> > > > diff --git a/include/linux/pci.h b/include/linux/pci.h

> > > > index 0e49f70..7f6bbb6 100644

> > > > --- a/include/linux/pci.h

> > > > +++ b/include/linux/pci.h

> > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct

> > > pci_bus *bus)

> > > >  /* provide the legacy pci_dma_* API */

> > > >  #include <linux/pci-dma-compat.h>

> > > >

> > > > +/*

> > > > + * define this macro here to refrain from compilation error for

> some

> > > > + * platforms. Please keep this macro at the end of this header

> file.

> > > > + */

> > > > +#ifndef PCIBIOS_MIN_IO

> > > > +#define PCIBIOS_MIN_IO		0

> > > > +#endif

> > > > +

> > > >  #endif /* LINUX_PCI_H */

> > > > --

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

> 

> --

> ====================

> | I would like to |

> | fix the world,  |

> | but they're not |

> | giving me the   |

>  \ source code!  /

>   ---------------

>     ¯\_(ツ)_/¯

On Thu, Nov 10, 2016 at 04:06:40PM +0000, Gabriele Paoloni wrote:
> Hi Liviu
> 
> > -----Original Message-----
> > From: liviu.dudau@arm.com [mailto:liviu.dudau@arm.com]
> > Sent: 09 November 2016 16:51
> > To: Gabriele Paoloni
> > Cc: Yuanzhichang; catalin.marinas@arm.com; will.deacon@arm.com;
> > robh+dt@kernel.org; bhelgaas@google.com; mark.rutland@arm.com;
> > olof@lixom.net; arnd@arndb.de; linux-arm-kernel@lists.infradead.org;
> > lorenzo.pieralisi@arm.com; linux-kernel@vger.kernel.org; Linuxarm;
> > devicetree@vger.kernel.org; linux-pci@vger.kernel.org; linux-
> > serial@vger.kernel.org; minyard@acm.org; benh@kernel.crashing.org;
> > zourongrong@gmail.com; John Garry; zhichang.yuan02@gmail.com;
> > kantyzc@163.com; xuwei (O)
> > Subject: Re: [PATCH V5 2/3] ARM64 LPC: Add missing range exception for
> > special ISA
> > 
> > On Wed, Nov 09, 2016 at 04:16:17PM +0000, Gabriele Paoloni wrote:
> > > Hi Liviu
> > >
> > > Thanks for reviewing
> > >
> > 
> > [removed some irrelevant part of discussion, avoid crazy formatting]
> > 
> > > > > +/**
> > > > > + * addr_is_indirect_io - check whether the input taddr is for
> > > > indirectIO.
> > > > > + * @taddr: the io address to be checked.
> > > > > + *
> > > > > + * Returns 1 when taddr is in the range; otherwise return 0.
> > > > > + */
> > > > > +int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > +	if (arm64_extio_ops->start > taddr || arm64_extio_ops->end
> > <
> > > > taddr)
> > > >
> > > > start >= taddr ?
> > >
> > > Nope... if  (taddr < arm64_extio_ops->start || taddr >
> > arm64_extio_ops->end)
> > > then taddr is outside the range [start; end] and will return 0;
> > otherwise
> > > it will return 1...
> > 
> > Oops, sorry, did not pay attention to the returned value. The check is
> > correct as it is, no need to change then.
> > 
> > >
> > > >
> > > > > +		return 0;
> > > > > +
> > > > > +	return 1;
> > > > > +}
> > > > >
> > > > >  BUILD_EXTIO(b, u8)
> > > > >
> > > > > diff --git a/drivers/of/address.c b/drivers/of/address.c
> > > > > index 02b2903..cc2a05d 100644
> > > > > --- a/drivers/of/address.c
> > > > > +++ b/drivers/of/address.c
> > > > > @@ -479,6 +479,50 @@ static int of_empty_ranges_quirk(struct
> > > > device_node *np)
> > > > >  	return false;
> > > > >  }
> > > > >
> > > > > +
> > > > > +/*
> > > > > + * of_isa_indirect_io - get the IO address from some isa reg
> > > > property value.
> > > > > + *	For some isa/lpc devices, no ranges property in ancestor
> > node.
> > > > > + *	The device addresses are described directly in their regs
> > > > property.
> > > > > + *	This fixup function will be called to get the IO address of
> > > > isa/lpc
> > > > > + *	devices when the normal of_translation failed.
> > > > > + *
> > > > > + * @parent:	points to the parent dts node;
> > > > > + * @bus:		points to the of_bus which can be used to parse
> > > > address;
> > > > > + * @addr:	the address from reg property;
> > > > > + * @na:		the address cell counter of @addr;
> > > > > + * @presult:	store the address paresed from @addr;
> > > > > + *
> > > > > + * return 1 when successfully get the I/O address;
> > > > > + * 0 will return for some failures.
> > > >
> > > > Bah, you are returning a signed int, why 0 for failure? Return a
> > > > negative value with
> > > > error codes. Otherwise change the return value into a bool.
> > >
> > > Yes we'll move to bool
> > >
> > > >
> > > > > + */
> > > > > +static int of_get_isa_indirect_io(struct device_node *parent,
> > > > > +				struct of_bus *bus, __be32 *addr,
> > > > > +				int na, u64 *presult)
> > > > > +{
> > > > > +	unsigned int flags;
> > > > > +	unsigned int rlen;
> > > > > +
> > > > > +	/* whether support indirectIO */
> > > > > +	if (!indirect_io_enabled())
> > > > > +		return 0;
> > > > > +
> > > > > +	if (!of_bus_isa_match(parent))
> > > > > +		return 0;
> > > > > +
> > > > > +	flags = bus->get_flags(addr);
> > > > > +	if (!(flags & IORESOURCE_IO))
> > > > > +		return 0;
> > > > > +
> > > > > +	/* there is ranges property, apply the normal translation
> > > > directly. */
> > > >
> > > > s/there is ranges/if we have a 'ranges'/
> > >
> > > Thanks for spotting this
> > >
> > > >
> > > > > +	if (of_get_property(parent, "ranges", &rlen))
> > > > > +		return 0;
> > > > > +
> > > > > +	*presult = of_read_number(addr + 1, na - 1);
> > > > > +	/* this fixup is only valid for specific I/O range. */
> > > > > +	return addr_is_indirect_io(*presult);
> > > > > +}
> > > > > +
> > > > >  static int of_translate_one(struct device_node *parent, struct
> > > > of_bus *bus,
> > > > >  			    struct of_bus *pbus, __be32 *addr,
> > > > >  			    int na, int ns, int pna, const char *rprop)
> > > > > @@ -595,6 +639,15 @@ static u64 __of_translate_address(struct
> > > > device_node *dev,
> > > > >  			result = of_read_number(addr, na);
> > > > >  			break;
> > > > >  		}
> > > > > +		/*
> > > > > +		 * For indirectIO device which has no ranges
> > property, get
> > > > > +		 * the address from reg directly.
> > > > > +		 */
> > > > > +		if (of_get_isa_indirect_io(dev, bus, addr, na,
> > &result)) {
> > > > > +			pr_debug("isa indirectIO matched(%s)..addr =
> > > > 0x%llx\n",
> > > > > +				of_node_full_name(dev), result);
> > > > > +			break;
> > > > > +		}
> > > > >
> > > > >  		/* Get new parent bus and counts */
> > > > >  		pbus = of_match_bus(parent);
> > > > > @@ -688,8 +741,9 @@ static int __of_address_to_resource(struct
> > > > device_node *dev,
> > > > >  	if (taddr == OF_BAD_ADDR)
> > > > >  		return -EINVAL;
> > > > >  	memset(r, 0, sizeof(struct resource));
> > > > > -	if (flags & IORESOURCE_IO) {
> > > > > +	if (flags & IORESOURCE_IO && taddr >= PCIBIOS_MIN_IO) {
> > > > >  		unsigned long port;
> > > > > +
> > > > >  		port = pci_address_to_pio(taddr);
> > > > >  		if (port == (unsigned long)-1)
> > > > >  			return -EINVAL;
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index ba34907..1a08511 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -3263,7 +3263,7 @@ int __weak
> > pci_register_io_range(phys_addr_t
> > > > addr, resource_size_t size)
> > > > >
> > > > >  #ifdef PCI_IOBASE
> > > > >  	struct io_range *range;
> > > > > -	resource_size_t allocated_size = 0;
> > > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > > >
> > > > >  	/* check if the range hasn't been previously recorded */
> > > > >  	spin_lock(&io_range_lock);
> > > > > @@ -3312,7 +3312,7 @@ phys_addr_t pci_pio_to_address(unsigned
> > long
> > > > pio)
> > > > >
> > > > >  #ifdef PCI_IOBASE
> > > > >  	struct io_range *range;
> > > > > -	resource_size_t allocated_size = 0;
> > > > > +	resource_size_t allocated_size = PCIBIOS_MIN_IO;
> > > >
> > > > Have you checked that pci_pio_to_address still returns valid values
> > > > after this? I know that
> > > > you are trying to take into account PCIBIOS_MIN_IO limit when
> > > > allocating reserving the IO ranges,
> > > > but the values added in the io_range_list are still starting from
> > zero,
> > > > no from PCIBIOS_MIN_IO,
> > >
> > > I think you're wrong here as in pci_address_to_pio we have:
> > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > >
> > > This should be enough to guarantee that the PIOs start at
> > > PCIBIOS_MIN_IO...right?
> > 
> > I don't think you can guarantee that the pio value that gets passed
> > into
> > pci_pio_to_address() always comes from a previously returned value by
> > pci_address_to_pio(). Maybe you can add a check in pci_pio_to_address()
> 
> Maybe I am missing something...could you make an exampleof a case
> where an IO toke doesn’t come from pci_address_to_pio() ?

Don't know, maybe it is coming from some DT or platform data? I was just
asking for confirmation that no one has seen issues there, I'm not saying
I've seen it happening.

Best regards,
Liviu

> 
> Thanks
> 
> Gab
> 
> 
> > 
> > 	if (pio < PCIBIOS_MIN_IO)
> > 		return address;
> > 
> > to avoid adding more checks in the list_for_each_entry() loop.
> > 
> > Best regards,
> > Liviu
> > 
> > >
> > >
> > > > so the calculation of the address in this function could return
> > > > negative values casted to pci_addr_t.
> > > >
> > > > Maybe you want to adjust the range->start value in
> > > > pci_register_io_range() as well to have it
> > > > offset by PCIBIOS_MIN_IO as well.
> > > >
> > > > Best regards,
> > > > Liviu
> > > >
> > > > >
> > > > >  	if (pio > IO_SPACE_LIMIT)
> > > > >  		return address;
> > > > > @@ -3335,7 +3335,7 @@ unsigned long __weak
> > > > pci_address_to_pio(phys_addr_t address)
> > > > >  {
> > > > >  #ifdef PCI_IOBASE
> > > > >  	struct io_range *res;
> > > > > -	resource_size_t offset = 0;
> > > > > +	resource_size_t offset = PCIBIOS_MIN_IO;
> > > > >  	unsigned long addr = -1;
> > > > >
> > > > >  	spin_lock(&io_range_lock);
> > > > > diff --git a/include/linux/of_address.h
> > b/include/linux/of_address.h
> > > > > index 3786473..deec469 100644
> > > > > --- a/include/linux/of_address.h
> > > > > +++ b/include/linux/of_address.h
> > > > > @@ -24,6 +24,23 @@ struct of_pci_range {
> > > > >  #define for_each_of_pci_range(parser, range) \
> > > > >  	for (; of_pci_range_parser_one(parser, range);)
> > > > >
> > > > > +
> > > > > +#ifndef indirect_io_enabled
> > > > > +#define indirect_io_enabled indirect_io_enabled
> > > > > +static inline bool indirect_io_enabled(void)
> > > > > +{
> > > > > +	return false;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#ifndef addr_is_indirect_io
> > > > > +#define addr_is_indirect_io addr_is_indirect_io
> > > > > +static inline int addr_is_indirect_io(u64 taddr)
> > > > > +{
> > > > > +	return 0;
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > >  /* Translate a DMA address from device space to CPU space */
> > > > >  extern u64 of_translate_dma_address(struct device_node *dev,
> > > > >  				    const __be32 *in_addr);
> > > > > diff --git a/include/linux/pci.h b/include/linux/pci.h
> > > > > index 0e49f70..7f6bbb6 100644
> > > > > --- a/include/linux/pci.h
> > > > > +++ b/include/linux/pci.h
> > > > > @@ -2130,4 +2130,12 @@ static inline bool pci_ari_enabled(struct
> > > > pci_bus *bus)
> > > > >  /* provide the legacy pci_dma_* API */
> > > > >  #include <linux/pci-dma-compat.h>
> > > > >
> > > > > +/*
> > > > > + * define this macro here to refrain from compilation error for
> > some
> > > > > + * platforms. Please keep this macro at the end of this header
> > file.
> > > > > + */
> > > > > +#ifndef PCIBIOS_MIN_IO
> > > > > +#define PCIBIOS_MIN_IO		0
> > > > > +#endif
> > > > > +
> > > > >  #endif /* LINUX_PCI_H */
> > > > > --
> > > > > 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
> > 
> > --
> > ====================
> > | I would like to |
> > | fix the world,  |
> > | but they're not |
> > | giving me the   |
> >  \ source code!  /
> >   ---------------
> >     ¯\_(ツ)_/¯

> > It's not a safe assumption for x86 at least. There are a few systems with
> > multiple ISA busses particularly older laptops with a docking station.  
> 
> But do they have multiple ISA domains? There is no real harm in supporting
> it, the (small) downsides I can think of are:

I don't believe they x86 class ones have multiple ISA domains. But as
I've said I don't know how the electronics in the older ThinkPad worked
when it used two PIIX4s with some LPC or ISA stuff on each.

It works in DOS and unmodified Linux so I'm pretty sure there are no
additional domains. Likewise the various x86 schemes that route some bits
of ISA bus off into strange places work in DOS and don't have any
overlaps.

yenta_socket handles PCI/PCMCIA bridging and routes a range of that flat
ISA space appropriately to the card.

Alan
--
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 Monday, November 14, 2016 11:11:11 AM CET One Thousand Gnomes wrote:
> > > It's not a safe assumption for x86 at least. There are a few systems with
> > > multiple ISA busses particularly older laptops with a docking station.  
> > 
> > But do they have multiple ISA domains? There is no real harm in supporting
> > it, the (small) downsides I can think of are:
> 
> I don't believe they x86 class ones have multiple ISA domains. But as
> I've said I don't know how the electronics in the older ThinkPad worked
> when it used two PIIX4s with some LPC or ISA stuff on each.
> 
> It works in DOS and unmodified Linux so I'm pretty sure there are no
> additional domains. Likewise the various x86 schemes that route some bits
> of ISA bus off into strange places work in DOS and don't have any
> overlaps.
> 
> yenta_socket handles PCI/PCMCIA bridging and routes a range of that flat
> ISA space appropriately to the card.

Right, that's what I had expected, so we still don't even
need to handle multiple ISA I/O address spaces for the
only known case of multiple ISA buses, though we may decide
to generalize the code like that anyway.

	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