Patchwork non static < 0x1000 IO space can be broken

login
register
about
Submitter Kevin O'Connor
Date 2009-07-19 19:41:50
Message ID <20090719194150.GA26540@morn.localdomain>
Download mbox | patch
Permalink /patch/51/
State Accepted
Headers show

Comments

Kevin O'Connor - 2009-07-19 19:41:50
On Sun, Jul 19, 2009 at 09:13:20PM +0200, Rudolf Marek wrote:
> Hi,
>
> Kevin just noticed that recent change in devices.c removed the IO alloc 
> start on IO port address 0x1000, so all code which relies on the fact 
> that 0x1000 and down can be done for static device allocation like in 
> vt8237r.h and k8t890.h will fail now. This needs to be fixed with proper 
> static resource to the read_resources. I'm trying to fix this together 
> with Kevin.
>
> Maybe other chipsets/boards will need similar fixups!

The patch Rudolf and I made is attached.  On IRC though, Rudolf noted
that we aren't explicilty allocating 0xcf8 and other crucial
resources.  Maybe we should just add a generic resource for everything
under 0x1000?

-Kevin
Rudolf Marek - 2009-07-19 19:48:49
> resources.  Maybe we should just add a generic resource for everything
> under 0x1000?

Yes maybe or we can try to fix the stuff which is non trivial :)

Here is the list of omitted resources in the patch (specifc to vt8237):

0x00 - 0xdf -> legacy resources
0x4d0/0x4d1 -> EISA level/edge stuff
0xcf8-0xcff -> PCI

also the 0x378+0x400 = 0x778 for parallel port ECP dont know if superio handles 
that.


Some other random ports on other chipsets:

the SB600 has something at 0xc00
K8M890 has something for S3 NVRAM at 0xf00

Rudolf
Myles Watson - 2009-07-20 14:06:49
> -----Original Message-----
> From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org]
> On Behalf Of Rudolf Marek
> Sent: Sunday, July 19, 2009 1:49 PM
> To: Kevin O'Connor
> Cc: Coreboot
> Subject: Re: [coreboot] non static < 0x1000 IO space can be broken
> 
> > resources.  Maybe we should just add a generic resource for everything
> > under 0x1000?

That's the way the rest of the boards did it.  I'm sorry I missed this
southbridge.  I thought I got them all.

The resource allocator will only care about the last I/O resource that you
add, because it doesn't split allocations.  So if you add 5 resources all
under 0x1000 with the last one ending at 0x1000, or 1 resource 0x0-0x1000,
it will have the same effect.

I noticed in your patch that you're setting the limit = base+size-1.  The
limit is not the same as the end of the resource.  For most I/O, the limit
should be 0xffff.

Thanks,
Myles
Rudolf Marek - 2009-07-20 14:14:59
Hi Myles,

> That's the way the rest of the boards did it.  I'm sorry I missed this
> southbridge.  I thought I got them all.

Ok. Please prepare the patch.

> The resource allocator will only care about the last I/O resource that you
> add, because it doesn't split allocations.  So if you add 5 resources all
> under 0x1000 with the last one ending at 0x1000, or 1 resource 0x0-0x1000,
> it will have the same effect.

ok.

> I noticed in your patch that you're setting the limit = base+size-1.  The
> limit is not the same as the end of the resource.  For most I/O, the limit
> should be 0xffff.

aha true. This was cut and paste from k8t890 where it was used to setup APIC 
resource. For mem resource it should be how? I don't understand the gran too.

For this SB (the VT8237S) we will definitely need to dynamically allocate a mmio 
BAR for SPI controller. I will prepare a patch, but cannot test.

I would like to ask if you care about the decoding range for flashes - if I need 
some other resource for that and also about 0xfee... for MSI.

Rudolf

Patch

Index: vt8237r_lpc.c
===================================================================
--- vt8237r_lpc.c	(revision 4441)
+++ vt8237r_lpc.c	(working copy)
@@ -421,8 +421,8 @@ 
 {
 	struct resource *res;
 
+	/* Fixed APIC resource */
 	pci_dev_read_resources(dev);
-	/* Fixed APIC resource */
 	res = new_resource(dev, 0x44);
 	res->base = VT8237R_APIC_BASE;
 	res->size = 256;
@@ -431,6 +431,50 @@ 
 	res->gran = 8;
 	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
 		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
+
+	/* Fixed SPI resource */
+	pci_dev_read_resources(dev);
+	res = new_resource(dev, 0xbe);
+	res->base = VT8237S_SPI_MEM_BASE;
+	res->size = 256;
+	res->limit = res->base + res->size - 1;
+	res->align = 8;
+	res->gran = 8;
+	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
+		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
+
+	/* Fixed HPET resource */
+	pci_dev_read_resources(dev);
+	res = new_resource(dev, 0x69);
+	res->base = VT8237R_HPET_ADDR;
+	res->size = 1024;
+	res->limit = res->base + res->size - 1;
+	res->align = 8;
+	res->gran = 8;
+	res->flags = IORESOURCE_MEM | IORESOURCE_FIXED |
+		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
+
+	/* Fixed smbus resource */
+	pci_dev_read_resources(dev);
+	res = new_resource(dev, 0xd0);
+	res->base = VT8237R_SMBUS_IO_BASE;
+	res->size = 16;
+	res->limit = res->base + res->size - 1;
+	res->align = 8;
+	res->gran = 8;
+	res->flags = IORESOURCE_IO | IORESOURCE_FIXED |
+		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
+
+	pci_dev_read_resources(dev);
+	/* Fixed pmio resource */
+	res = new_resource(dev, 0x88);
+	res->base = VT8237R_ACPI_IO_BASE;
+	res->size = 128;
+	res->limit = res->base + res->size - 1;
+	res->align = 8;
+	res->gran = 8;
+	res->flags = IORESOURCE_IO | IORESOURCE_FIXED |
+		     IORESOURCE_STORED | IORESOURCE_ASSIGNED;
 }
 
 /**