Patchwork AMD CS5530(A) ROM enable

login
register
about
Submitter Uwe Hermann
Date 2009-10-06 14:34:31
Message ID <20091006143431.GP15511@greenwood>
Download mbox | patch
Permalink /patch/350/
State Accepted
Commit r4731
Headers show

Comments

Uwe Hermann - 2009-10-06 14:34:31
See patch.


Uwe.
Carl-Daniel Hailfinger - 2009-10-06 14:42:54
On 06.10.2009 16:34, Uwe Hermann wrote:
> +	 * Make the ROM write-protected.
>   

Does that mean we have to unprotect the ROM in flashrom?

Regards,
Carl-Daniel
Luc Verhaegen - 2009-10-06 14:49:22
On Tue, Oct 06, 2009 at 04:42:54PM +0200, Carl-Daniel Hailfinger wrote:
> On 06.10.2009 16:34, Uwe Hermann wrote:
> > +	 * Make the ROM write-protected.
> >   
> 
> Does that mean we have to unprotect the ROM in flashrom?
> 
> Regards,
> Carl-Daniel

I am happy with that if, and only if, the flashrom chipset enable takes 
care of that.

Luc Verhaegen.
Uwe Hermann - 2009-10-06 15:49:25
On Tue, Oct 06, 2009 at 04:49:22PM +0200, Luc Verhaegen wrote:
> > > +	 * Make the ROM write-protected.
> > >   
> > 
> > Does that mean we have to unprotect the ROM in flashrom?

Yes.

I think long-term we want a (runtime/CMOS) option for coreboot
to enable/disable ROM protection. For now, I guess every chipset does
a different thing and/or doesn't explicitly protect or unprotect
the ROM at all.

As a safe default I think disabling the ROM write access is the best thing
to do from a user's point of view.


> I am happy with that if, and only if, the flashrom chipset enable takes 
> care of that.

Sure, flashrom does that. And for other chipsets where we might write-protect
the ROM later we also have flashrom chipset support. I cannot think
of any chipset where we have coreboot support but no flashrom support
(even for CK804 and MCP55 we have).


Uwe.
ron minnich - 2009-10-06 17:24:49
On Tue, Oct 6, 2009 at 8:49 AM, Uwe Hermann <uwe@hermann-uwe.de> wrote:

> I think long-term we want a (runtime/CMOS) option for coreboot
> to enable/disable ROM protection. For now, I guess every chipset does
> a different thing and/or doesn't explicitly protect or unprotect
> the ROM at all.

Going back to the beginning, from the old days, we always left it
write-enabled. That was amistake on our part and it is good to see it
being fixed.

ron
Paul Menzel - 2009-10-06 18:16:17
Dear Uwe,


Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> Index: src/southbridge/amd/cs5530/cs5530.h
> ===================================================================
> --- src/southbridge/amd/cs5530/cs5530.h (Revision 4726)
> +++ src/southbridge/amd/cs5530/cs5530.h (Arbeitskopie)
> @@ -27,7 +27,13 @@

[…]

> +#define LOWER_ROM_ADDRESS_RANGE                (1 << 0)
> +#define ROM_WRITE_ENABLE               (1 << 1)
> +#define UPPER_ROM_ADDRESS_RANGE                (1 << 2)
> +#define BIOS_ROM_POSITIVE_DECODE       (1 << 5)

Is this spacing intentional?


Thanks,

Paul
Uwe Hermann - 2009-10-06 19:13:04
On Tue, Oct 06, 2009 at 08:16:17PM +0200, Paul Menzel wrote:
> Dear Uwe,
> 
> 
> Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> > Index: src/southbridge/amd/cs5530/cs5530.h
> > ===================================================================
> > --- src/southbridge/amd/cs5530/cs5530.h (Revision 4726)
> > +++ src/southbridge/amd/cs5530/cs5530.h (Arbeitskopie)
> > @@ -27,7 +27,13 @@
> 
> […]
> 
> > +#define LOWER_ROM_ADDRESS_RANGE                (1 << 0)
> > +#define ROM_WRITE_ENABLE               (1 << 1)
> > +#define UPPER_ROM_ADDRESS_RANGE                (1 << 2)
> > +#define BIOS_ROM_POSITIVE_DECODE       (1 << 5)
> 
> Is this spacing intentional?

Yes. It looks strange in the patch/email, but if you apply the patch and
look at the file in a text editor it'll be nicely aligned.


Uwe.
Marc Jones - 2009-10-06 22:14:05
On Tue, Oct 6, 2009 at 8:34 AM, Uwe Hermann <uwe@hermann-uwe.de> wrote:
> See patch.
>

Uwe,

Can the call go into a cs5530_enable_rom() call go into a the
southbridge init instead of every mainboard? Would it ever be
mainboard specific?

Marc
Patrick Georgi - 2009-10-07 14:18:06
Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> See patch.
Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>

The rom enable call can go into some common southbridge code once the
code flow allows it (currently it does not)
Uwe Hermann - 2009-10-07 14:39:21
On Wed, Oct 07, 2009 at 04:18:06PM +0200, Patrick Georgi wrote:
> Am Dienstag, den 06.10.2009, 16:34 +0200 schrieb Uwe Hermann:
> > See patch.
> Acked-by: Patrick Georgi <patrick.georgi@coresystems.de>
> 
> The rom enable call can go into some common southbridge code once the
> code flow allows it (currently it does not)

Thanks, r4731. Will take care of moving the calls later, as soon
as the infrastructure is there.


Uwe.

Patch

Enable full ROM access on AMD CS5530(A) (needed for CBFS).

Signed-off-by: Uwe Hermann <uwe@hermann-uwe.de>

Index: src/southbridge/amd/cs5530/cs5530_isa.c
===================================================================
--- src/southbridge/amd/cs5530/cs5530_isa.c	(Revision 4726)
+++ src/southbridge/amd/cs5530/cs5530_isa.c	(Arbeitskopie)
@@ -45,14 +45,6 @@ 
 
 static void isa_init(struct device *dev)
 {
-	uint8_t reg8;
-
-	// TODO: Test if needed, otherwise drop.
-
-	/* Set positive decode on ROM. */
-	reg8 = pci_read_config8(dev, DECODE_CONTROL_REG2);
-	reg8 |= BIOS_ROM_POSITIVE_DECODE;
-	pci_write_config8(dev, DECODE_CONTROL_REG2, reg8);
 }
 
 static void cs5530_pci_dev_enable_resources(device_t dev)
Index: src/southbridge/amd/cs5530/cs5530_enable_rom.c
===================================================================
--- src/southbridge/amd/cs5530/cs5530_enable_rom.c	(Revision 0)
+++ src/southbridge/amd/cs5530/cs5530_enable_rom.c	(Revision 0)
@@ -0,0 +1,47 @@ 
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2009 Uwe Hermann <uwe@hermann-uwe.de>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
+
+#include <stdint.h>
+#include "cs5530.h"
+
+static void cs5530_enable_rom(void)
+{
+	uint8_t reg8;
+
+	/* So far all CS5530(A) ISA bridges we've seen are at 00:12.0. */
+	device_t dev = PCI_DEV(0, 0x12, 0);
+
+	/*
+	 * Decode 0x000E0000-0x000FFFFF (128 KB), not just 64 KB, and
+	 * decode 0xFF000000-0xFFFFFFFF (16 MB), not just 256 KB.
+	 *
+	 * Make the ROM write-protected.
+	 */
+	reg8 = pci_read_config8(dev, ROM_AT_LOGIC_CONTROL_REG);
+	reg8 |= LOWER_ROM_ADDRESS_RANGE;
+	reg8 |= UPPER_ROM_ADDRESS_RANGE;
+	reg8 &= ~ROM_WRITE_ENABLE;
+	pci_write_config8(dev, ROM_AT_LOGIC_CONTROL_REG, reg8);
+
+	/* Set positive decode on ROM. */
+	reg8 = pci_read_config8(dev, DECODE_CONTROL_REG2);
+	reg8 |= BIOS_ROM_POSITIVE_DECODE;
+	pci_write_config8(dev, DECODE_CONTROL_REG2, reg8);
+}
Index: src/southbridge/amd/cs5530/cs5530.h
===================================================================
--- src/southbridge/amd/cs5530/cs5530.h	(Revision 4726)
+++ src/southbridge/amd/cs5530/cs5530.h	(Arbeitskopie)
@@ -27,7 +27,13 @@ 
 #endif
 
 #define DECODE_CONTROL_REG2		0x5b		/* F0 index 0x5b */
+#define ROM_AT_LOGIC_CONTROL_REG	0x52		/* F0 index 0x52 */
 
+#define LOWER_ROM_ADDRESS_RANGE		(1 << 0)
+#define ROM_WRITE_ENABLE		(1 << 1)
+#define UPPER_ROM_ADDRESS_RANGE		(1 << 2)
+#define BIOS_ROM_POSITIVE_DECODE	(1 << 5)
+
 /* Selects PCI positive decoding for accesses to the configured ROM space. */
 #define BIOS_ROM_POSITIVE_DECODE	(1 << 5)
 
Index: src/mainboard/axus/tc320/auto.c
===================================================================
--- src/mainboard/axus/tc320/auto.c	(Revision 4726)
+++ src/mainboard/axus/tc320/auto.c	(Arbeitskopie)
@@ -32,6 +32,7 @@ 
 #include "northbridge/amd/gx1/raminit.c"
 #include "superio/nsc/pc97317/pc97317_early_serial.c"
 #include "cpu/x86/bist.h"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x2e, PC97317_SP1)
 
@@ -41,6 +42,7 @@ 
 	uart_init();
 	console_init();
 	report_bist_failure(bist);
+	cs5530_enable_rom();
 	sdram_init();
 	/* ram_check(0, 640 * 1024); */
 }
Index: src/mainboard/bcom/winnet100/auto.c
===================================================================
--- src/mainboard/bcom/winnet100/auto.c	(Revision 4726)
+++ src/mainboard/bcom/winnet100/auto.c	(Arbeitskopie)
@@ -32,6 +32,7 @@ 
 #include "northbridge/amd/gx1/raminit.c"
 #include "superio/nsc/pc97317/pc97317_early_serial.c"
 #include "cpu/x86/bist.h"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x2e, PC97317_SP1)
 
@@ -45,6 +46,8 @@ 
 	/* Halt if there was a built in self test failure. */
 	report_bist_failure(bist);
 
+	cs5530_enable_rom();
+
 	/* Initialize RAM. */
 	sdram_init();
 
Index: src/mainboard/televideo/tc7020/auto.c
===================================================================
--- src/mainboard/televideo/tc7020/auto.c	(Revision 4726)
+++ src/mainboard/televideo/tc7020/auto.c	(Arbeitskopie)
@@ -32,6 +32,7 @@ 
 #include "northbridge/amd/gx1/raminit.c"
 #include "superio/nsc/pc97317/pc97317_early_serial.c"
 #include "cpu/x86/bist.h"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x2e, PC97317_SP1)
 
@@ -45,6 +46,8 @@ 
 	/* Halt if there was a built in self test failure. */
 	report_bist_failure(bist);
 
+	cs5530_enable_rom();
+
 	/* Initialize RAM. */
 	sdram_init();
 
Index: src/mainboard/iei/nova4899r/auto.c
===================================================================
--- src/mainboard/iei/nova4899r/auto.c	(Revision 4726)
+++ src/mainboard/iei/nova4899r/auto.c	(Arbeitskopie)
@@ -30,6 +30,7 @@ 
 #include "arch/i386/lib/console.c"
 #include "lib/ramtest.c"
 #include "superio/winbond/w83977tf/w83977tf_early_serial.c"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 #include "cpu/x86/bist.h"
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977TF_SP1)
@@ -46,6 +47,8 @@ 
 	/* Halt if there was a built in self test failure. */
 	report_bist_failure(bist);
 
+	cs5530_enable_rom();
+
 	/* Initialize RAM. */
 	sdram_init();
 
Index: src/mainboard/iei/juki-511p/auto.c
===================================================================
--- src/mainboard/iei/juki-511p/auto.c	(Revision 4726)
+++ src/mainboard/iei/juki-511p/auto.c	(Arbeitskopie)
@@ -30,6 +30,7 @@ 
 #include "arch/i386/lib/console.c"
 #include "lib/ramtest.c"
 #include "superio/winbond/w83977f/w83977f_early_serial.c"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 #include "cpu/x86/bist.h"
 #include "pc80/udelay_io.c"
 
@@ -51,6 +52,8 @@ 
 	inb(0x043);
 	inb(0x843);
 
+	cs5530_enable_rom();
+
 	/* Initialize RAM. */
 	sdram_init();
 
Index: src/mainboard/asi/mb_5blgp/auto.c
===================================================================
--- src/mainboard/asi/mb_5blgp/auto.c	(Revision 4726)
+++ src/mainboard/asi/mb_5blgp/auto.c	(Arbeitskopie)
@@ -31,6 +31,7 @@ 
 #include "northbridge/amd/gx1/raminit.c"
 #include "cpu/x86/bist.h"
 #include "superio/nsc/pc87351/pc87351_early_serial.c"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x2e, PC87351_SP1)
 
@@ -40,6 +41,7 @@ 
 	uart_init();
 	console_init();
 	report_bist_failure(bist);
+	cs5530_enable_rom();
 	sdram_init();
 	/* ram_check(0, 640 * 1024); */
 }
Index: src/mainboard/asi/mb_5blmp/auto.c
===================================================================
--- src/mainboard/asi/mb_5blmp/auto.c	(Revision 4726)
+++ src/mainboard/asi/mb_5blmp/auto.c	(Arbeitskopie)
@@ -32,6 +32,7 @@ 
 #include "northbridge/amd/gx1/raminit.c"
 #include "superio/nsc/pc87351/pc87351_early_serial.c"
 #include "cpu/x86/bist.h"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x2e, PC87351_SP1)
 
@@ -45,6 +46,8 @@ 
 	/* Halt if there was a built in self test failure. */
 	report_bist_failure(bist);
 
+	cs5530_enable_rom();
+
 	/* Initialize RAM. */
 	sdram_init();
 
Index: src/mainboard/advantech/pcm-5820/auto.c
===================================================================
--- src/mainboard/advantech/pcm-5820/auto.c	(Revision 4726)
+++ src/mainboard/advantech/pcm-5820/auto.c	(Arbeitskopie)
@@ -31,6 +31,7 @@ 
 #include "northbridge/amd/gx1/raminit.c"
 #include "cpu/x86/bist.h"
 #include "superio/winbond/w83977f/w83977f_early_serial.c"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x3f0, W83977F_SP1)
 
@@ -40,6 +41,7 @@ 
 	uart_init();
 	console_init();
 	report_bist_failure(bist);
+	cs5530_enable_rom();
 	sdram_init();
 	/* ram_check(0, 640 * 1024); */
 }
Index: src/mainboard/eaglelion/5bcm/auto.c
===================================================================
--- src/mainboard/eaglelion/5bcm/auto.c	(Revision 4726)
+++ src/mainboard/eaglelion/5bcm/auto.c	(Arbeitskopie)
@@ -14,6 +14,7 @@ 
 #include "superio/nsc/pc97317/pc97317_early_serial.c"
 //#include "northbridge/intel/i440bx/raminit.h"
 #include "cpu/x86/bist.h"
+#include "southbridge/amd/cs5530/cs5530_enable_rom.c"
 
 #define SERIAL_DEV PNP_DEV(0x2e, PC97317_SP1)
 
@@ -30,6 +31,8 @@ 
 	/* Halt if there was a built in self test failure */
 	report_bist_failure(bist);
 	
+	cs5530_enable_rom();
+
 	sdram_init();
 	
 	/* Check all of memory */