[PATCH 0/3] Fix Footbridge PCI I/O resources

[PATCH 0/3] Fix Footbridge PCI I/O resources

[Russell King - ARM Linux admin]

This series fixes the PCI I/O resources for the Footbridge platforms
which were broken quite some time ago by misinterpretation of what
pcibios_min_io actually means, and the introduction of bus level PCI
I/O resources.

The changes become a particular problem when a Southbridge is present
on the PCI bus, which is a PCI-to-ISA bridge, and hence is where all
the ISA resources live. It is made worse when you have an IDE
controller which operates in legacy mode, with resources at the
0x170/0x1f0/0x376/0x3f6 addresses.

Worse than that, the previous changes removed the CSR I/O resource
allocation entirely, setting the 21285 to respond to I/O transactions
at bus address 0, which may overlap ISA resources on the Southbridge.

This series fixes the resource problems by restoring the old behaviour
via a flag that omits the PCI I/O resource, enabling this flag for all
footbridge platforms, and restoring the allocation of the CSR I/O
resource. This approach offers minimal impact for other platforms.

 arch/arm/include/asm/mach/pci.h          |  1 +
 arch/arm/kernel/bios32.c                 | 37 +++++++++++++++++++-------------
 arch/arm/mach-footbridge/cats-pci.c      |  1 +
 arch/arm/mach-footbridge/dc21285.c       | 15 ++++++++++++-
 arch/arm/mach-footbridge/ebsa285-pci.c   |  1 +
 arch/arm/mach-footbridge/netwinder-pci.c |  1 +
 arch/arm/mach-footbridge/personal-pci.c  |  1 +
 7 files changed, 41 insertions(+), 16 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/3] ARM: pci: make bus I/O resources optional

[Russell King]

Adding a bus I/O resource that extends from pcibios_min_io to the top
of PCI space breaks Footbridge based platforms, which may have ISA
southbridges and IDE controllers that are in legacy mode.

The PCI I/O space on these machines really does cover port addresses
from zero upwards, even when pcibios_min_io is non-zero.

Fix this by making Rob's changes optional - Rob's change is probably
based on a misunderstanding of what pcibios_min_io is - it is the
minimum IO port address that we wish to start allocating bus resources
which may not be the same as the minimum IO port address for the bus.

Fixes: 3c5d1699887b ("ARM: move PCI i/o resource setup into common code")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/include/asm/mach/pci.h |  1 +
 arch/arm/kernel/bios32.c        | 37 ++++++++++++++++++++-------------
 2 files changed, 23 insertions(+), 15 deletions(-)

diff --git a/arch/arm/include/asm/mach/pci.h b/arch/arm/include/asm/mach/pci.h
index 83d340702680..0f96c2462ee5 100644
--- a/arch/arm/include/asm/mach/pci.h
+++ b/arch/arm/include/asm/mach/pci.h
@@ -21,6 +21,7 @@ struct hw_pci {
 	struct pci_ops	*ops;
 	int		nr_controllers;
 	unsigned int	io_optional:1;
+	unsigned int	no_bus_ioport_resource:1;
 	void		**private_data;
 	int		(*setup)(int nr, struct pci_sys_data *);
 	int		(*scan)(int nr, struct pci_host_bridge *);
diff --git a/arch/arm/kernel/bios32.c b/arch/arm/kernel/bios32.c
index ed46ca69813d..c48476193687 100644
--- a/arch/arm/kernel/bios32.c
+++ b/arch/arm/kernel/bios32.c
@@ -412,10 +412,11 @@ static int pcibios_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 }
 
 static int pcibios_init_resource(int busnr, struct pci_sys_data *sys,
-				 int io_optional)
+				 int io_optional, int bus_ioport_resource)
 {
-	int ret;
 	struct resource_entry *window;
+	struct resource *res;
+	int ret;
 
 	if (list_empty(&sys->resources)) {
 		pci_add_resource_offset(&sys->resources,
@@ -434,19 +435,25 @@ static int pcibios_init_resource(int busnr, struct pci_sys_data *sys,
 		if (resource_type(window->res) == IORESOURCE_IO)
 			return 0;
 
-	sys->io_res.start = (busnr * SZ_64K) ?  : pcibios_min_io;
-	sys->io_res.end = (busnr + 1) * SZ_64K - 1;
-	sys->io_res.flags = IORESOURCE_IO;
-	sys->io_res.name = sys->io_res_name;
-	sprintf(sys->io_res_name, "PCI%d I/O", busnr);
+	if (bus_ioport_resource) {
+		sys->io_res.start = (busnr * SZ_64K) ?  : pcibios_min_io;
+		sys->io_res.end = (busnr + 1) * SZ_64K - 1;
+		sys->io_res.flags = IORESOURCE_IO;
+		sys->io_res.name = sys->io_res_name;
+		sprintf(sys->io_res_name, "PCI%d I/O", busnr);
+
+		ret = request_resource(&ioport_resource, &sys->io_res);
+		if (ret) {
+			pr_err("PCI: unable to allocate I/O port region (%d)\n", ret);
+			return ret;
+		}
 
-	ret = request_resource(&ioport_resource, &sys->io_res);
-	if (ret) {
-		pr_err("PCI: unable to allocate I/O port region (%d)\n", ret);
-		return ret;
+		res = &sys->io_res;
+	} else {
+		res = &ioport_resource;
 	}
-	pci_add_resource_offset(&sys->resources, &sys->io_res,
-				sys->io_offset);
+
+	pci_add_resource_offset(&sys->resources, res, sys->io_offset);
 
 	return 0;
 }
@@ -478,8 +485,8 @@ static void pcibios_init_hw(struct device *parent, struct hw_pci *hw,
 		ret = hw->setup(nr, sys);
 
 		if (ret > 0) {
-
-			ret = pcibios_init_resource(nr, sys, hw->io_optional);
+			ret = pcibios_init_resource(nr, sys, hw->io_optional,
+						  !hw->no_bus_ioport_resource);
 			if (ret)  {
 				pci_free_host_bridge(bridge);
 				break;
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/3] ARM: footbridge: avoid using separate PCI I/O bus resource

[Russell King]

Disable the separate PCI I/O bus resource, since the PCI bus on these
platforms covers the whole of I/O space, even low ISA addresses are
on the PCI bus.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/mach-footbridge/cats-pci.c      | 1 +
 arch/arm/mach-footbridge/ebsa285-pci.c   | 1 +
 arch/arm/mach-footbridge/netwinder-pci.c | 1 +
 arch/arm/mach-footbridge/personal-pci.c  | 1 +
 4 files changed, 4 insertions(+)

diff --git a/arch/arm/mach-footbridge/cats-pci.c b/arch/arm/mach-footbridge/cats-pci.c
index 90b1e9be430e..ac225b295f6d 100644
--- a/arch/arm/mach-footbridge/cats-pci.c
+++ b/arch/arm/mach-footbridge/cats-pci.c
@@ -48,6 +48,7 @@ static struct hw_pci cats_pci __initdata = {
 	.swizzle		= cats_no_swizzle,
 	.map_irq		= cats_map_irq,
 	.nr_controllers		= 1,
+	.no_bus_ioport_resource	= 1,
 	.ops			= &dc21285_ops,
 	.setup			= dc21285_setup,
 	.preinit		= dc21285_preinit,
diff --git a/arch/arm/mach-footbridge/ebsa285-pci.c b/arch/arm/mach-footbridge/ebsa285-pci.c
index c3f280d08fa7..89b505bb02e4 100644
--- a/arch/arm/mach-footbridge/ebsa285-pci.c
+++ b/arch/arm/mach-footbridge/ebsa285-pci.c
@@ -32,6 +32,7 @@ static int ebsa285_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 static struct hw_pci ebsa285_pci __initdata = {
 	.map_irq		= ebsa285_map_irq,
 	.nr_controllers		= 1,
+	.no_bus_ioport_resource	= 1,
 	.ops			= &dc21285_ops,
 	.setup			= dc21285_setup,
 	.preinit		= dc21285_preinit,
diff --git a/arch/arm/mach-footbridge/netwinder-pci.c b/arch/arm/mach-footbridge/netwinder-pci.c
index e8304392074b..ea12b152bbea 100644
--- a/arch/arm/mach-footbridge/netwinder-pci.c
+++ b/arch/arm/mach-footbridge/netwinder-pci.c
@@ -46,6 +46,7 @@ static int netwinder_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 static struct hw_pci netwinder_pci __initdata = {
 	.map_irq		= netwinder_map_irq,
 	.nr_controllers		= 1,
+	.no_bus_ioport_resource	= 1,
 	.ops			= &dc21285_ops,
 	.setup			= dc21285_setup,
 	.preinit		= dc21285_preinit,
diff --git a/arch/arm/mach-footbridge/personal-pci.c b/arch/arm/mach-footbridge/personal-pci.c
index 9d19aa98a663..560ad5672783 100644
--- a/arch/arm/mach-footbridge/personal-pci.c
+++ b/arch/arm/mach-footbridge/personal-pci.c
@@ -41,6 +41,7 @@ static int personal_server_map_irq(const struct pci_dev *dev, u8 slot, u8 pin)
 static struct hw_pci personal_server_pci __initdata = {
 	.map_irq		= personal_server_map_irq,
 	.nr_controllers		= 1,
+	.no_bus_ioport_resource	= 1,
 	.ops			= &dc21285_ops,
 	.setup			= dc21285_setup,
 	.preinit		= dc21285_preinit,
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/3] ARM: footbridge: restore allocation of CSR I/O resource

[Russell King]

Commit 8ef6e6201b2 ("ARM: footbridge: use fixed PCI i/o mapping") made
two changes: it contains what it says in the summary line, but it also
removes the CSR I/O allocation, which effectively means the DC21285
responds to I/O accesses at address 0..0x7f, which overlap Southbridge
ISA resources on the same PCI bus.

This commit fixes it, but depends on the previous two commits removing
the bus level PCI I/O resource:
  ARM: footbridge: avoid using separate PCI I/O bus resource
  ARM: pci: make bus I/O resources optional

Fixes: 8ef6e6201b2 ("ARM: footbridge: use fixed PCI i/o mapping")
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 arch/arm/mach-footbridge/dc21285.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-footbridge/dc21285.c b/arch/arm/mach-footbridge/dc21285.c
index f9713dc561cf..d900651a9c8f 100644
--- a/arch/arm/mach-footbridge/dc21285.c
+++ b/arch/arm/mach-footbridge/dc21285.c
@@ -332,6 +332,19 @@ void __init dc21285_preinit(void)
 			    "PCI data parity", NULL);
 
 	if (cfn_mode) {
+		static struct resource csrio;
+
+		csrio.flags  = IORESOURCE_IO;
+		csrio.name   = "Footbridge";
+
+		/*
+		 * Put the Footbridge IO space in the top 256 bytes of IO
+		 * space, which should otherwise remain unused. This avoids
+		 * any conflict with ISA peripherals.
+		 */
+		allocate_resource(&ioport_resource, &csrio, 128,
+				  0xff00, 0xffff, 128, NULL, NULL);
+
 		/*
 		 * Map our SDRAM at a known address in PCI space, just in case
 		 * the firmware had other ideas.  Using a nonzero base is
@@ -339,7 +352,7 @@ void __init dc21285_preinit(void)
 		 * in the range 0x000a0000 to 0x000c0000. (eg, S3 cards).
 		 */
 		*CSR_PCICSRBASE       = 0xf4000000;
-		*CSR_PCICSRIOBASE     = 0;
+		*CSR_PCICSRIOBASE     = csrio.start;
 		*CSR_PCISDRAMBASE     = __virt_to_bus(PAGE_OFFSET);
 		*CSR_PCIROMBASE       = 0;
 		*CSR_PCICMD = PCI_COMMAND_MEMORY | PCI_COMMAND_MASTER |
-- 
2.20.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: pci: make bus I/O resources optional

[Arnd Bergmann]

On Fri, Mar 26, 2021 at 1:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Adding a bus I/O resource that extends from pcibios_min_io to the top
> of PCI space breaks Footbridge based platforms, which may have ISA
> southbridges and IDE controllers that are in legacy mode.
>
> The PCI I/O space on these machines really does cover port addresses
> from zero upwards, even when pcibios_min_io is non-zero.
>
> Fix this by making Rob's changes optional - Rob's change is probably
> based on a misunderstanding of what pcibios_min_io is - it is the
> minimum IO port address that we wish to start allocating bus resources
> which may not be the same as the minimum IO port address for the bus.

I think Rob's original change is needed for platforms with multiple PCI
root bridges, since they need non-overlapping I/O resources, and each
if they can't all be the root ioport_resource, they have to be children of that.

Leaving out the lower 0x1000 port numbers in turn is required so
legacy ISA drivers acquire the ports with request_region(). The reason
it broke on your machine was apparently that pci_claim_resource()
failed to assign the low port numbers to the PCI bus after that has
explicitly set the minimum.

Using ioport_resource as the PCI host bridge resource as you do
is the easiest way to make it work for both ISA drivers and PCI drivers
using low I/O ports.

> @@ -434,19 +435,25 @@ static int pcibios_init_resource(int busnr, struct pci_sys_data *sys,
>                 if (resource_type(window->res) == IORESOURCE_IO)
>                         return 0;
>
> -       sys->io_res.start = (busnr * SZ_64K) ?  : pcibios_min_io;
> -       sys->io_res.end = (busnr + 1) * SZ_64K - 1;
> -       sys->io_res.flags = IORESOURCE_IO;
> -       sys->io_res.name = sys->io_res_name;
> -       sprintf(sys->io_res_name, "PCI%d I/O", busnr);
> +       if (bus_ioport_resource) {
> +               sys->io_res.start = (busnr * SZ_64K) ?  : pcibios_min_io;
> +               sys->io_res.end = (busnr + 1) * SZ_64K - 1;
> +               sys->io_res.flags = IORESOURCE_IO;


After rereading this function, I see that the function already provides
a way to do what you want here: If the hw->setup() function inserts
ioport_resource into sys->resources along with  the memory
resources, it will skip the extra one, and you get the same effect.

This is what ixp4xx and sa1100/nanoengine apparently do. I
see that cns3xxx also adds an I/O resource, but this one seems
to get it wrong (it refers to the physical address of the I/O window,
not the port numbers), which would be an unrelated bug.

         Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] ARM: footbridge: restore allocation of CSR I/O resource

[Arnd Bergmann]

On Fri, Mar 26, 2021 at 1:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Commit 8ef6e6201b2 ("ARM: footbridge: use fixed PCI i/o mapping") made
> two changes: it contains what it says in the summary line, but it also
> removes the CSR I/O allocation, which effectively means the DC21285
> responds to I/O accesses at address 0..0x7f, which overlap Southbridge
> ISA resources on the same PCI bus.
>
> This commit fixes it, but depends on the previous two commits removing
> the bus level PCI I/O resource:
>   ARM: footbridge: avoid using separate PCI I/O bus resource
>   ARM: pci: make bus I/O resources optional
>
> Fixes: 8ef6e6201b2 ("ARM: footbridge: use fixed PCI i/o mapping")
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

Reviewed-by: Arnd Bergmann <arnd@arndb.de>

This fix is needed regardless of which way the first patch fixes the
bus resource.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: pci: make bus I/O resources optional

[Russell King - ARM Linux admin]

On Fri, Mar 26, 2021 at 02:33:15PM +0100, Arnd Bergmann wrote:
> Leaving out the lower 0x1000 port numbers in turn is required so
> legacy ISA drivers acquire the ports with request_region(). The reason
> it broke on your machine was apparently that pci_claim_resource()
> failed to assign the low port numbers to the PCI bus after that has
> explicitly set the minimum.

No, this is totally broken thinking. The 0x1000 offset is *purely* a
PCI resource *allocation* issue, nothing more nothing less. To argue
otherwise just shows a lack of understanding of what is going on here.

_All_ resources from IO port 0 to 0xffff are on the PCI bus in this
case. It makes no sense to create a PCI bus resource for 0x1000-0xffff.
That is entirely illogical and ficticious.

> Using ioport_resource as the PCI host bridge resource as you do
> is the easiest way to make it work for both ISA drivers and PCI drivers
> using low I/O ports.

The fact is, in these machines, the ISA bus is _behind_ the PCI bus,
just like it is on x86 PCs and Alpha. There's no difference here.

> > @@ -434,19 +435,25 @@ static int pcibios_init_resource(int busnr, struct pci_sys_data *sys,
> >                 if (resource_type(window->res) == IORESOURCE_IO)
> >                         return 0;
> >
> > -       sys->io_res.start = (busnr * SZ_64K) ?  : pcibios_min_io;
> > -       sys->io_res.end = (busnr + 1) * SZ_64K - 1;
> > -       sys->io_res.flags = IORESOURCE_IO;
> > -       sys->io_res.name = sys->io_res_name;
> > -       sprintf(sys->io_res_name, "PCI%d I/O", busnr);
> > +       if (bus_ioport_resource) {
> > +               sys->io_res.start = (busnr * SZ_64K) ?  : pcibios_min_io;
> > +               sys->io_res.end = (busnr + 1) * SZ_64K - 1;
> > +               sys->io_res.flags = IORESOURCE_IO;
> 
> 
> After rereading this function, I see that the function already provides
> a way to do what you want here: If the hw->setup() function inserts
> ioport_resource into sys->resources along with  the memory
> resources, it will skip the extra one, and you get the same effect.

True, but I suspect the start of the IO resource above is wrong in
any case - as I say, this is based on a misunderstanding of what
pcibios_min_io is. It is the start of the region of to be used
for resource allocation on the bus, which can be different from the
range of addresses present on the bus.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 3/3] ARM: footbridge: restore allocation of CSR I/O resource

[Russell King - ARM Linux admin]

On Fri, Mar 26, 2021 at 02:37:39PM +0100, Arnd Bergmann wrote:
> On Fri, Mar 26, 2021 at 1:18 PM Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > Commit 8ef6e6201b2 ("ARM: footbridge: use fixed PCI i/o mapping") made
> > two changes: it contains what it says in the summary line, but it also
> > removes the CSR I/O allocation, which effectively means the DC21285
> > responds to I/O accesses at address 0..0x7f, which overlap Southbridge
> > ISA resources on the same PCI bus.
> >
> > This commit fixes it, but depends on the previous two commits removing
> > the bus level PCI I/O resource:
> >   ARM: footbridge: avoid using separate PCI I/O bus resource
> >   ARM: pci: make bus I/O resources optional
> >
> > Fixes: 8ef6e6201b2 ("ARM: footbridge: use fixed PCI i/o mapping")
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> This fix is needed regardless of which way the first patch fixes the
> bus resource.

This fix is dependent on the first patch as I said above - otherwise
the allocation will fail.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/3] ARM: pci: make bus I/O resources optional

[Arnd Bergmann]

On Fri, Mar 26, 2021 at 2:39 PM Russell King - ARM Linux admin
<linux@armlinux.org.uk> wrote:
>
> On Fri, Mar 26, 2021 at 02:33:15PM +0100, Arnd Bergmann wrote:
> > Leaving out the lower 0x1000 port numbers in turn is required so
> > legacy ISA drivers acquire the ports with request_region(). The reason
> > it broke on your machine was apparently that pci_claim_resource()
> > failed to assign the low port numbers to the PCI bus after that has
> > explicitly set the minimum.
>
> No, this is totally broken thinking. The 0x1000 offset is *purely* a
> PCI resource *allocation* issue, nothing more nothing less. To argue
> otherwise just shows a lack of understanding of what is going on here.
>
> _All_ resources from IO port 0 to 0xffff are on the PCI bus in this
> case. It makes no sense to create a PCI bus resource for 0x1000-0xffff.
> That is entirely illogical and ficticious.
>
> > Using ioport_resource as the PCI host bridge resource as you do
> > is the easiest way to make it work for both ISA drivers and PCI drivers
> > using low I/O ports.
>
> The fact is, in these machines, the ISA bus is _behind_ the PCI bus,
> just like it is on x86 PCs and Alpha. There's no difference here.

The problem I'm referring to is a set conflicting requirements:

1. all ISA devices must have port resources that are direct
  children of ioport_resource. This is independent of how the
  ISA bus is physically attached.

2. If there are multiple PCI host bridges that have physically
  separate I/O port ranges.

3. Any PCI device with I/O ports must have a corresponding
  resource that is a subset of the PCI host bridge->windows
  list

4. Old style PCI IDE devices and a few others must have their
  I/O ports set to within the ISA range.

I don't think there is any good solution that covers the entire
set. In case of footbridge and old PCs, Using ioport_resource
in bridge->windows as you do works fine because there is only
a single physical port range. But because it breaks constraint
#2 in the list, it wouldn't work in general.

The current code works for constraint #2, but breaks #4, so you
can't have an PCI IDE adapter.

arch/x86/pci/broadcom_bus.c manages to address all of the
above by adding the IDE resources directly into the PCI
host bridge resources, but this is platform specific and not
needed on footbridge because you don't care about #2.

        Arnd

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel