Skip to contentSkip to conversation

Register PCI host bridge resource earlier

PATCH In review Thomas Bogendoerfer 2 participants 10 messages Apr 8, 2007 – Apr 9, 2007 MIPS

lore
Share & tools
lore

Thomas Bogendoerfer

author maintainer
#0 lore raw

PATCH 1/1 Register PCI host bridge resource earlier

Hi,

PCI based SNI RM machines have their EISA bus behind an Intel PCI/EISA bridge. So the PCI IO range must start at 0x0000. Changing that will break the PCI bus, because i8259.c already has registered it's IO addresses before the PCI bus gets initialized. Below is a patch, which will register the PCI host bridge resources inside register_pci_controller(). It also changes i8259.c to use insert_region(), because request_resource() will fail, if the IO space of the PIT hanging of the PCI host bridge (maybe passing the resource parent to init_i8259_irqs() is a cleaner fix for that).

Signed-off-by Thomas Bogendoerfer <[email protected]>

---

arch/mips/pci/pci.c +12 -13
index f901140..67e01fd 100644
@@ -77,6 +77,13 @@ pcibios_align_resource(void *data, struct resource *res,
 
 void __init register_pci_controller(struct pci_controller *hose)
 {
+	if (request_resource(&iomem_resource, hose->mem_resource) < 0)
+		goto out;
+	if (request_resource(&ioport_resource, hose->io_resource) < 0) {
+		release_resource(hose->mem_resource);
+		goto out;
+	}
+
 	*hose_tail = hose;
 	hose_tail = &hose->next;
 

@@ -87,6 +94,11 @@ void __init register_pci_controller(struct pci_controller *hose)
 		printk(KERN_WARNING
 		       "registering PCI controller with io_map_base unset\n");
 	}
+	return;
+
+out:
+	printk(KERN_WARNING
+	       "Skipping PCI bus scan due to resource conflict\n");
 }
 
 /* Most MIPS systems have straight-forward swizzling needs.  */

@@ -126,11 +138,6 @@ static int __init pcibios_init(void)
 	/* Scan all of the recorded PCI controllers.  */
 	for (next_busno = 0, hose = hose_head; hose; hose = hose->next) {
 
-		if (request_resource(&iomem_resource, hose->mem_resource) < 0)
-			goto out;
-		if (request_resource(&ioport_resource, hose->io_resource) < 0)
-			goto out_free_mem_resource;
-
 		if (!hose->iommu)
 			PCI_DMA_BUS_IS_PHYS = 1;
 

@@ -149,14 +156,6 @@ static int __init pcibios_init(void)
 				need_domain_info = 1;
 			}
 		}
-		continue;
-
-out_free_mem_resource:
-		release_resource(hose->mem_resource);
-
-out:
-		printk(KERN_WARNING
-		       "Skipping PCI bus scan due to resource conflict\n");
 	}
 
 	if (!pci_probe_only)
arch/mips/kernel/i8259.c +2 -2
index 9c79703..2345160 100644
@@ -328,8 +328,8 @@ void __init init_i8259_irqs (void)
 {
 	int i;
 
-	request_resource(&ioport_resource, &pic1_io_resource);
-	request_resource(&ioport_resource, &pic2_io_resource);
+	insert_resource(&ioport_resource, &pic1_io_resource);
+	insert_resource(&ioport_resource, &pic2_io_resource);
 
 	init_8259A(0);
Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]
#1 lore raw

Hello.

Quoted 9 lines · Thomas Bogendoerfer

PCI based SNI RM machines have their EISA bus behind an Intel PCI/EISA bridge. So the PCI IO range must start at 0x0000. Changing that will break the PCI bus, because i8259.c already has registered it's IO addresses before the PCI bus gets initialized. Below is a patch, which will register the PCI host bridge resources inside register_pci_controller(). It also changes i8259.c to use insert_region(), because request_resource() will fail, if the IO space of the PIT hanging of the PCI host bridge (maybe passing the resource parent to init_i8259_irqs() is a cleaner fix for that).

First, I don't understand how PIT and PIC resources may intersect. Then,
IIUC, using inert_resource() will cause PIT resource be the child of the PIC resource which doesn't make sense either.

WBR, Sergei

author maintainer
#2 lore raw
Quoted 12 lines · Sergei Shtylyov
Quoted 9 lines · Thomas Bogendoerfer

PCI based SNI RM machines have their EISA bus behind an Intel PCI/EISA bridge. So the PCI IO range must start at 0x0000. Changing that will break the PCI bus, because i8259.c already has registered it's IO addresses before the PCI bus gets initialized. Below is a patch, which will register the PCI host bridge resources inside register_pci_controller(). It also changes i8259.c to use insert_region(), because request_resource() will fail, if the IO space of the PIT hanging of the PCI host bridge (maybe passing the resource parent to init_i8259_irqs() is a cleaner fix for that).

Quoted 1 line

First, I don't understand how PIT and PIC resources may intersect. Then,

oops, I meant PIC resources.

Quoted 2 lines

IIUC, using inert_resource() will cause PIT resource be the child of the PIC resource which doesn't make sense either.

the insert_resource will make it child of the PCI resource, which makes perfect sense, because the ISA bus is behind the PCI host bridge.

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]
#3 lore raw

Hello.

Quoted 10 lines · Thomas Bogendoerfer
Quoted 9 lines
Quoted 9 lines

PCI based SNI RM machines have their EISA bus behind an Intel PCI/EISA bridge. So the PCI IO range must start at 0x0000. Changing that will break the PCI bus, because i8259.c already has registered it's IO addresses before the PCI bus gets initialized. Below is a patch, which will register the PCI host bridge resources inside register_pci_controller(). It also changes i8259.c to use insert_region(), because request_resource() will fail, if the IO space of the PIT hanging of the PCI host bridge (maybe passing the resource parent to init_i8259_irqs() is a cleaner fix for that).

Quoted 8 lines
Quoted 1 line

First, I don't understand how PIT and PIC resources may intersect. Then,

oops, I meant PIC resources.

Quoted 1 line

IIUC, using inert_resource() will cause PIT resource be the child of the PIC resource which doesn't make sense either.

the insert_resource will make it child of the PCI resource, which makes perfect sense, because the ISA bus is behind the PCI host bridge.

If you read the comment to insert_resource() you'll see that it works
contrarywise, i. e. the inserted resource is made parent of the conflicting ones. I.e. it should't work as you're intending it to.

Quoted 1 line

Thomas.

WBR, Sergei

author maintainer
#4 lore raw
Quoted 3 lines · Sergei Shtylyov

If you read the comment to insert_resource() you'll see that it works
contrarywise, i. e. the inserted resource is made parent of the conflicting ones. I.e. it should't work as you're intending it to.

why don't you think I haven't testet this ?

00000000-03bfffff : PCIT IO
00000000-0000001f : dma1
00000020-00000021 : pic1
00000040-0000005f : timer 00000060-0000006f : keyboard 00000080-0000008f : dma page reg 000000a0-000000a1 : pic2
000000c0-000000df : dma2
000002f8-000002ff : serial 000003c0-000003df : cirrusfb 000003f8-000003ff : serial 00000c80-00000c83 : EISA device @@@0000 00000cf8-00000cfb : PCI config addr 00000cfc-00000cff : PCI config data 00001000-000010ff : Madge Smart 16/4 EISA Ringnode 00001400-000014ff : Madge Smart 16/4 EISA Ringnode 00001800-000018ff : Madge Smart 16/4 EISA Ringnode 00001c00-00001cff : Madge Smart 16/4 EISA Ringnode

that's exactly what I wrote.

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]
#5 lore raw

Hello.

Quoted 24 lines · Thomas Bogendoerfer
Quoted 2 lines

If you read the comment to insert_resource() you'll see that it works
contrarywise, i. e. the inserted resource is made parent of the conflicting ones. I.e. it should't work as you're intending it to.

why don't you think I haven't testet this ?

00000000-03bfffff : PCIT IO
00000000-0000001f : dma1
00000020-00000021 : pic1
00000040-0000005f : timer 00000060-0000006f : keyboard 00000080-0000008f : dma page reg 000000a0-000000a1 : pic2
000000c0-000000df : dma2
000002f8-000002ff : serial 000003c0-000003df : cirrusfb 000003f8-000003ff : serial 00000c80-00000c83 : EISA device @@@0000 00000cf8-00000cfb : PCI config addr 00000cfc-00000cff : PCI config data 00001000-000010ff : Madge Smart 16/4 EISA Ringnode 00001400-000014ff : Madge Smart 16/4 EISA Ringnode 00001800-000018ff : Madge Smart 16/4 EISA Ringnode 00001c00-00001cff : Madge Smart 16/4 EISA Ringnode

Quoted 1 line

that's exactly what I wrote.

I'm just not seeing how using insert_resource() vs request_resource() for
i8259 ports can be relevant here.

Quoted 1 line

Thomas.

WBR, Sergei

author maintainer
#6 lore raw
Quoted 2 lines · Sergei Shtylyov

I'm just not seeing how using insert_resource() vs request_resource() for i8259 ports can be relevant here.

request_resource will fail, because the range is already taken by sni_io_resource, while insert_region inserts the resource into sni_io_resource. The problem is that init_i8259 doesn't have the right resource for doing the request_resource, if ioport_resource starting from 0x0000 is already taken by a PCI host bridge. I could probably write a patch, which adds a parameter to init_i8259 for the resource, where the request_resource is correct. No idea, whether this is worth the efford.

Opions ?

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]
#7 lore raw

Hello.

Quoted 5 lines · Thomas Bogendoerfer
Quoted 2 lines

I'm just not seeing how using insert_resource() vs request_resource() for i8259 ports can be relevant here.

request_resource will fail, because the range is already taken by sni_io_resource, while insert_region inserts the resource into sni_io_resource.

No, it shouldn't according to what I'm seeing in the code. Perhaps I'm
missing something and need to actually try executing alike code a see...

Quoted 3 lines

The problem is that init_i8259 doesn't have the right resource for doing the request_resource, if ioport_resource starting from 0x0000 is already taken by a PCI host bridge.

I'm not at all sure that giving out I/O addresses from 0 to PCI is a great
idea -- is it indeed necessary?

Quoted 5 lines

I could probably write a
patch, which adds a parameter to init_i8259 for the resource, where the request_resource is correct. No idea, whether this is worth the efford.

Opions ?

Did you mean options, opinions, or something else? :-)

Quoted 1 line

Thomas.

WBR, Sergei

author maintainer
#8 lore raw
Quoted 6 lines · Sergei Shtylyov
Quoted 3 lines

request_resource will fail, because the range is already taken by sni_io_resource, while insert_region inserts the resource into sni_io_resource.

No, it shouldn't according to what I'm seeing in the code. Perhaps I'm
missing something and need to actually try executing alike code a see...

after startup there is ioport_resource, which is 0x0000-IO_SPACE_LIMIT. My change in register_pci_controller will request the PCI IO space from 0x0000 to 0x3bffff (the maximum the PCI host bridge could address). request_resource from ioport_resource, which i8259.c tried to do, will fail now in that range, because it'ss taken by the PCI bridge. Therefore anybody wanting IO space in that range, must take it from the PCI IO space. So doing request_region (&sni_io_resource, &pci1_io_resource); would have worked as well. But the code right now doesn't have a handle for the parent resource. insert_region() on the other side searches for the parent resource over the whole given resource and plugs the wanted resource to the right sub resource. Fine for simple house keeping, which is IMHO ok in that place.

Quoted 6 lines
Quoted 3 lines

The problem is that init_i8259 doesn't have the right resource for doing the request_resource, if ioport_resource starting from 0x0000 is already taken by a PCI host bridge.

I'm not at all sure that giving out I/O addresses from 0 to PCI is a great idea -- is it indeed necessary?

I'm feeling like an oldtimer right now. Ever heard of ISA busses ? The address space there starts from 0x0000. There is this infamous DMA controller waiting exactly at IO address 0x0000-0x001f. Floppy DMA needs to use that for example. Of course this would work even without the silly resource stuff (inb/outb don't care), EISA code wants to see 0x0000 as base address of the PCI/EISA bridge.

Oh, and addresses for PCI devices (bridges are a different story) will start at PCIBIOS_MIN_IO. So there will be no PCI device resource starting at 0x0000.

Quoted 7 lines
Quoted 5 lines

I could probably write a
patch, which adds a parameter to init_i8259 for the resource, where the request_resource is correct. No idea, whether this is worth the efford.

Opions ?

Did you mean options, opinions, or something else? :-)

I wanted to know from someone, who knows what I talking about, if my current code is acceptable or needs more workout.

Thomas.

Signature · 2 lines
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]
#9 lore raw

Hello.

Quoted 9 lines · Thomas Bogendoerfer
Quoted 5 lines
Quoted 2 lines

request_resource will fail, because the range is already taken by sni_io_resource, while insert_region inserts the resource into sni_io_resource.

No, it shouldn't according to what I'm seeing in the code. Perhaps I'm
missing something and need to actually try executing alike code a see...

after startup there is ioport_resource, which is 0x0000-IO_SPACE_LIMIT. My change in register_pci_controller will request the PCI IO space from 0x0000 to 0x3bffff (the maximum the PCI host bridge could address).

Why do you need to claim I/O ports from 0, at the first place (especially
if nobody else does this)? Apparently in your EISA system, ports 0x0 thru 0x100 are reserved for ISA compat pereiplerals (they are behind the PCI-ISA bridge but that doesn't matter for this purpose), ports 0x100-0x3ff are not for PCI anyway (besides, that's legacy ISA card range, along with all the aliases), 0x1000-0x1cff, 0x2000-0x2cff, ... 0x8000-0x8cff are reserved for EISA slots and everything in beetween you're marking as unavailable to PCI too (besides, those ranges are ISA card alias ranges). So, what was the point of claiming all the lagacy ranges to belong to PCI and then prevent it from using them, (and break 8259 code by doing that)?

Quoted 7 lines

request_resource from ioport_resource, which i8259.c tried to do, will
fail now in that range, because it'ss taken by the PCI bridge. Therefore
anybody wanting IO space in that range, must take it from the PCI IO space. So doing request_region (&sni_io_resource, &pci1_io_resource); would have worked as well. But the code right now doesn't have a
handle for the parent resource. insert_region() on the other side
searches for the parent resource over the whole given resource and
plugs the wanted resource to the right sub resource.

OK, seeing my mistake in the code interpretation now. But I must note that
the comment to that function *is* misguiding:

Quoted 1 line

Fine for simple house keeping, which is IMHO ok in that place.

It's OK but I'm still not seeing why we need it at all.

Quoted 8 lines
Quoted 6 lines
Quoted 3 lines

The problem is that init_i8259 doesn't have the right resource for doing the request_resource, if ioport_resource starting from 0x0000 is already taken by a PCI host bridge.

I'm not at all sure that giving out I/O addresses from 0 to PCI is a great idea -- is it indeed necessary?

I'm feeling like an oldtimer right now. Ever heard of ISA busses ? The

Alas, your irony is lost on me. :-)

Quoted 3 lines

address space there starts from 0x0000. There is this infamous DMA controller waiting exactly at IO address 0x0000-0x001f. Floppy DMA needs to use that for example. Of course this would work even without

I even programmed ISA bus masters. :-)

Quoted 2 lines

the silly resource stuff (inb/outb don't care), EISA code wants to see 0x0000 as base address of the PCI/EISA bridge.

Does EISA code care about PCI bridge?
Well, looking at drivers/eisa/pci_eisa.c looks like it indeed may care.
Well, then I've probably lost the case. :-)

Quoted 10 lines
Quoted 7 lines
Quoted 5 lines

I could probably write a
patch, which adds a parameter to init_i8259 for the resource, where the request_resource is correct. No idea, whether this is worth the efford.

Opions ?

Did you mean options, opinions, or something else? :-)

I wanted to know from someone, who knows what I talking about, if my current code is acceptable or needs more workout.

You wanted it, you got it.

Quoted 1 line

Thomas.

WBR, Sergei