[PATCH] Register PCI host bridge resource earlier

[PATCH] Register PCI host bridge resource earlier

[Thomas Bogendoerfer]

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 <tsbogend@alpha.franken.de>
---


diff --git a/arch/mips/pci/pci.c b/arch/mips/pci/pci.c
index f901140..67e01fd 100644
--- a/arch/mips/pci/pci.c
+++ b/arch/mips/pci/pci.c
@@ -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)
diff --git a/arch/mips/kernel/i8259.c b/arch/mips/kernel/i8259.c
index 9c79703..2345160 100644
--- a/arch/mips/kernel/i8259.c
+++ b/arch/mips/kernel/i8259.c
@@ -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);
 


-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Register PCI host bridge resource earlier

[Sergei Shtylyov]

Hello.

Thomas Bogendoerfer wrote:

> 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

Re: [PATCH] Register PCI host bridge resource earlier

[Thomas Bogendoerfer]

On Sun, Apr 08, 2007 at 04:20:00PM +0400, Sergei Shtylyov wrote:
> Thomas Bogendoerfer wrote:
> 
> >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, 

oops, I meant PIC resources.

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

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Register PCI host bridge resource earlier

[Sergei Shtylyov]

Hello.

Thomas Bogendoerfer wrote:

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

> oops, I meant PIC resources.

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

> Thomas.

WBR, Sergei

Re: [PATCH] Register PCI host bridge resource earlier

[Thomas Bogendoerfer]

On Sun, Apr 08, 2007 at 05:26:45PM +0400, Sergei Shtylyov wrote:
>    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.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Register PCI host bridge resource earlier

[Sergei Shtylyov]

Hello.

Thomas Bogendoerfer wrote:

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

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

> Thomas.

WBR, Sergei

Re: [PATCH] Register PCI host bridge resource earlier

[Thomas Bogendoerfer]

On Sun, Apr 08, 2007 at 06:47:41PM +0400, Sergei Shtylyov wrote:
>    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.

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Register PCI host bridge resource earlier

[Sergei Shtylyov]

Hello.

Thomas Bogendoerfer wrote:

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

> 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 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? :-)

> Thomas.

WBR, Sergei

Re: [PATCH] Register PCI host bridge resource earlier

[Thomas Bogendoerfer]

On Sun, Apr 08, 2007 at 08:58:42PM +0400, Sergei Shtylyov wrote:
> >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.

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

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

-- 
Crap can work. Given enough thrust pigs will fly, but it's not necessary a
good idea.                                                [ RFC1925, 2.3 ]

Re: [PATCH] Register PCI host bridge resource earlier

[Sergei Shtylyov]

Hello.

Thomas Bogendoerfer wrote:

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

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

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

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

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

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

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

> Thomas.

WBR, Sergei