Patchwork Set the register based on the ROMSIZE

login
register
about
Submitter Bao, Zheng
Date 2010-12-10 12:02:05
Message ID <DD1CC71B621B004FA76856E5129D6B170459DBF0@sbjgexmb1.amd.com>
Download mbox | patch
Permalink /patch/2413/
State Rejected
Headers show

Comments

Bao, Zheng - 2010-12-10 12:02:05
Set the register based on the ROMSIZE.

Signed-off-by: Zheng Bao <zheng.bao@amd.com>

  *          PN 43366_sb7xx_bdg_pub_1.00, June 2009, section 3.1, page
14.
@@ -57,8 +57,18 @@
 	 * Enable LPC ROM range start at:
 	 * 0xfff8(0000): 512KB
 	 * 0xfff0(0000): 1MB
+	 * 0xffe0(0000): 2MB
+	 * 0xffc0(0000): 4MB
 	 */
+	#if CONFIG_COREBOOT_ROMSIZE_KB_512 ||
CONFIG_COREBOOT_ROMSIZE_KB_256 || CONFIG_COREBOOT_ROMSIZE_KB_128
+	pci_write_config16(dev, 0x6c, 0xfff8); /* 512KB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_1024
 	pci_write_config16(dev, 0x6c, 0xfff0); /* 1 MB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_2048
+	pci_write_config16(dev, 0x6c, 0xffe0); /* 2 MB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_4096
+	pci_write_config16(dev, 0x6c, 0xffc0); /* 4 MB */
+	#endif
 	/* Enable LPC ROM range end at 0xffff(ffff). */
 	pci_write_config16(dev, 0x6e, 0xffff);
 }
Set the register based on the ROMSIZE.

Signed-off-by: Zheng Bao <zheng.bao@amd.com>
Patrick Georgi - 2010-12-10 12:10:15
Am 10.12.2010 13:02, schrieb Bao, Zheng:
> Set the register based on the ROMSIZE.

> @@ -57,8 +57,18 @@
>  	 * Enable LPC ROM range start at:
>  	 * 0xfff8(0000): 512KB
>  	 * 0xfff0(0000): 1MB
> +	 * 0xffe0(0000): 2MB
> +	 * 0xffc0(0000): 4MB
>  	 */
> +	#if CONFIG_COREBOOT_ROMSIZE_KB_512 ||
> CONFIG_COREBOOT_ROMSIZE_KB_256 || CONFIG_COREBOOT_ROMSIZE_KB_128
> +	pci_write_config16(dev, 0x6c, 0xfff8); /* 512KB */
> +	#elif CONFIG_COREBOOT_ROMSIZE_KB_1024
>  	pci_write_config16(dev, 0x6c, 0xfff0); /* 1 MB */
> +	#elif CONFIG_COREBOOT_ROMSIZE_KB_2048
> +	pci_write_config16(dev, 0x6c, 0xffe0); /* 2 MB */
> +	#elif CONFIG_COREBOOT_ROMSIZE_KB_4096
> +	pci_write_config16(dev, 0x6c, 0xffc0); /* 4 MB */
> +	#endif
How about
pci_write_config16(dev, 0x6c,
0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
instead?


Patrick
Bao, Zheng - 2010-12-10 12:29:16
I am not sure.
I think we need to leave the workload to compiler, instead of to the
machine running the coreboot and to the people.



Zheng

> -----Original Message-----
> From: coreboot-bounces@coreboot.org
[mailto:coreboot-bounces@coreboot.org]
> On Behalf Of Patrick Georgi
> Sent: Friday, December 10, 2010 8:10 PM
> To: coreboot@coreboot.org
> Subject: Re: [coreboot] [PATCH] Set the register based on the ROMSIZE
> 
> Am 10.12.2010 13:02, schrieb Bao, Zheng:
> > Set the register based on the ROMSIZE.
> 
> > @@ -57,8 +57,18 @@
> >  	 * Enable LPC ROM range start at:
> >  	 * 0xfff8(0000): 512KB
> >  	 * 0xfff0(0000): 1MB
> > +	 * 0xffe0(0000): 2MB
> > +	 * 0xffc0(0000): 4MB
> >  	 */
> > +	#if CONFIG_COREBOOT_ROMSIZE_KB_512 ||
> > CONFIG_COREBOOT_ROMSIZE_KB_256 || CONFIG_COREBOOT_ROMSIZE_KB_128
> > +	pci_write_config16(dev, 0x6c, 0xfff8); /* 512KB */
> > +	#elif CONFIG_COREBOOT_ROMSIZE_KB_1024
> >  	pci_write_config16(dev, 0x6c, 0xfff0); /* 1 MB */
> > +	#elif CONFIG_COREBOOT_ROMSIZE_KB_2048
> > +	pci_write_config16(dev, 0x6c, 0xffe0); /* 2 MB */
> > +	#elif CONFIG_COREBOOT_ROMSIZE_KB_4096
> > +	pci_write_config16(dev, 0x6c, 0xffc0); /* 4 MB */
> > +	#endif
> How about
> pci_write_config16(dev, 0x6c,
> 0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
> instead?
> 
> 
> Patrick
> 
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
Patrick Georgi - 2010-12-10 12:58:12
Am 10.12.2010 13:29, schrieb Bao, Zheng:
>> How about
>> pci_write_config16(dev, 0x6c,
>> 0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
>> instead?
> I am not sure.
> I think we need to leave the workload to compiler, instead of to the
> machine running the coreboot and to the people.
Hmm.. The main problem might be that romcc is probably not capable of
resolving max(a,b) at compile time. Other than that, this is a rather
primitive compile time optimization.

Other than that, it's more a style question: lots of #ifdef/#elif
statements or a comparably opaque calculation as in the proposal?


Patrick
Bao, Zheng - 2010-12-10 13:14:14
My opinion is,
Use the calculation, and put the truth table in the comment, if it has
nothing to do with ROMCC.

Zheng

> -----Original Message-----
> From: coreboot-bounces+zheng.bao=amd.com@coreboot.org
[mailto:coreboot-
> bounces+zheng.bao=amd.com@coreboot.org] On Behalf Of Patrick Georgi
> Sent: Friday, December 10, 2010 8:58 PM
> To: coreboot@coreboot.org
> Subject: Re: [coreboot] [PATCH] Set the register based on the ROMSIZE
> 
> Am 10.12.2010 13:29, schrieb Bao, Zheng:
> >> How about
> >> pci_write_config16(dev, 0x6c,
> >> 0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
> >> instead?
> > I am not sure.
> > I think we need to leave the workload to compiler, instead of to the
> > machine running the coreboot and to the people.
> Hmm.. The main problem might be that romcc is probably not capable of
> resolving max(a,b) at compile time. Other than that, this is a rather
> primitive compile time optimization.
> 
> Other than that, it's more a style question: lots of #ifdef/#elif
> statements or a comparably opaque calculation as in the proposal?
> 
> 
> Patrick
> 
> --
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
Scott - 2010-12-10 16:30:23
> @@ -57,8 +57,18 @@
>  	 * Enable LPC ROM range start at:
>  	 * 0xfff8(0000): 512KB
>  	 * 0xfff0(0000): 1MB
> +	 * 0xffe0(0000): 2MB
> +	 * 0xffc0(0000): 4MB
>  	 */
> +	#if CONFIG_COREBOOT_ROMSIZE_KB_512 ||
> CONFIG_COREBOOT_ROMSIZE_KB_256 || CONFIG_COREBOOT_ROMSIZE_KB_128
> +	pci_write_config16(dev, 0x6c, 0xfff8); /* 512KB */
> +	#elif CONFIG_COREBOOT_ROMSIZE_KB_1024
>  	pci_write_config16(dev, 0x6c, 0xfff0); /* 1 MB */
> +	#elif CONFIG_COREBOOT_ROMSIZE_KB_2048
> +	pci_write_config16(dev, 0x6c, 0xffe0); /* 2 MB */
> +	#elif CONFIG_COREBOOT_ROMSIZE_KB_4096
> +	pci_write_config16(dev, 0x6c, 0xffc0); /* 4 MB */
> +	#endif

]
]How about
]pci_write_config16(dev, 0x6c,
]0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
]instead?
]
]Patrick


Why not... 
pci_write_config16(dev, 0x6c,0x10000-(CONFIG_COREBOOT_ROMSIZE_KB>>6));
In other words, why apply special treatment to sizes < 512KB?
In any case, I think most readers of C code would find one of
these methods easier to follow than the preprocessor method.

Thanks,
Scott
Peter Stuge - 2010-12-10 16:54:53
Patrick Georgi wrote:
> > Set the register based on the ROMSIZE.
..
> How about
> pci_write_config16(dev, 0x6c,
> 0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
> instead?

I have a greater problem with this;

It assumes that one image will always go into one and the same size
of flash chip.

We can not support growing or shrinking of CBFS if we would like to,
and hotswapping flash chips with larger size does not work.

I would personally prefer using the largest possible value on all
chipsets.


//Peter
Scott - 2010-12-10 17:13:03
Peter Stuge wrote:

]I have a greater problem with this;
]
]It assumes that one image will always go into one and the same size
]of flash chip.

I think I understand your concern. It is convenient to be able to 
use a bigger chip to test a smaller image. But limiting the decode
size will only block access to the usused portion of the chip, correct?

]We can not support growing or shrinking of CBFS if we would like to,
]and hotswapping flash chips with larger size does not work.

When hotswapping, shouldn't the flashing software set this register?

]I would personally prefer using the largest possible value on all
]chipsets.

Despite my comments above, I agree with this. Flashing software is 
likely going to expand the decode to 16MB in order to run a generic
chip detection algorithm, so it isn't safe to let coreboot or OS
use address in that range for MMIO. As long as this area is reserved,
why not just set the decode range for the whole area.

Thanks,
Scott

]
]//Peter
Stefan Reinauer - 2010-12-10 18:45:49
* Peter Stuge <peter@stuge.se> [101210 17:54]:
> Patrick Georgi wrote:
> > > Set the register based on the ROMSIZE.
> ..
> > How about
> > pci_write_config16(dev, 0x6c,
> > 0x10000-(max(512,CONFIG_COREBOOT_ROMSIZE_KB)>>6));
> > instead?
> 
> I have a greater problem with this;
> 
> It assumes that one image will always go into one and the same size
> of flash chip.
> 
> We can not support growing or shrinking of CBFS if we would like to,
> and hotswapping flash chips with larger size does not work.
> 
> I would personally prefer using the largest possible value on all
> chipsets.

Yes, I agree with Peter... Is there a reason to not enable the biggest
possible area all the time?

Stefan
Stefan Reinauer - 2010-12-10 18:48:17
* Scott Duplichan <scott@notabs.org> [101210 18:13]:
> Peter Stuge wrote:
> 
> ]I have a greater problem with this;
> ]
> ]It assumes that one image will always go into one and the same size
> ]of flash chip.
> 
> I think I understand your concern. It is convenient to be able to 
> use a bigger chip to test a smaller image. But limiting the decode
> size will only block access to the usused portion of the chip, correct?

One of the original design ideas of CBFS (or LAR in v3) was that we can
build a single image, with, say 256KB, and resize that same image later
on in case someone wants to put in fallback/normal, a linux kernel
payload or stuff like that. 
 
Stefan

Patch

Index: src/southbridge/amd/sb600/Kconfig
===================================================================
--- src/southbridge/amd/sb600/Kconfig	(revision 6159)
+++ src/southbridge/amd/sb600/Kconfig	(working copy)
@@ -23,6 +23,11 @@ 
 	select HAVE_USBDEBUG
 	select TINY_BOOTBLOCK
 
+config BOOTBLOCK_SOUTHBRIDGE_INIT
+	string
+	default "southbridge/amd/sb600/bootblock.c"
+	depends on SOUTHBRIDGE_AMD_SB600
+
 config EHCI_BAR
 	hex
 	default 0xfef00000 if SOUTHBRIDGE_AMD_SB600
Index: src/southbridge/amd/sb600/bootblock.c
===================================================================
--- src/southbridge/amd/sb600/bootblock.c	(revision 6159)
+++ src/southbridge/amd/sb600/bootblock.c	(working copy)
@@ -59,7 +59,15 @@ 
 	 * 0xffe0(0000): 2MB
 	 * 0xffc0(0000): 4MB
 	 */
+	#if CONFIG_COREBOOT_ROMSIZE_KB_512 || CONFIG_COREBOOT_ROMSIZE_KB_256 || CONFIG_COREBOOT_ROMSIZE_KB_128
+	pci_write_config16(dev, 0x6c, 0xfff8); /* 512KB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_1024
+	pci_write_config16(dev, 0x6c, 0xfff0); /* 1 MB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_2048
+	pci_write_config16(dev, 0x6c, 0xffe0); /* 2 MB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_4096
 	pci_write_config16(dev, 0x6c, 0xffc0); /* 4 MB */
+	#endif
 	/* Enable LPC ROM range end at 0xffff(ffff). */
 	pci_write_config16(dev, 0x6e, 0xffff);
 }
Index: src/southbridge/amd/sb700/Kconfig
===================================================================
--- src/southbridge/amd/sb700/Kconfig	(revision 6159)
+++ src/southbridge/amd/sb700/Kconfig	(working copy)
@@ -23,6 +23,11 @@ 
 	select HAVE_USBDEBUG
 	select TINY_BOOTBLOCK
 
+config BOOTBLOCK_SOUTHBRIDGE_INIT
+	string
+	default "southbridge/amd/sb700/bootblock.c"
+	depends on SOUTHBRIDGE_AMD_SB700
+
 config SOUTHBRIDGE_AMD_SB700_SKIP_ISA_DMA_INIT
 	bool
 	default n
Index: src/southbridge/amd/sb700/bootblock.c
===================================================================
--- src/southbridge/amd/sb700/bootblock.c	(revision 6159)
+++ src/southbridge/amd/sb700/bootblock.c	(working copy)
@@ -28,7 +28,7 @@ 
  * Hardware should enable LPC ROM by pin straps. This function does not
  * handle the theoretically possible PCI ROM, FWH, or SPI ROM configurations.
  *
- * The SB700 power-on default is to map 256K ROM space.
+ * The SB700 power-on default is to map 512K ROM space.
  *
  * Details: AMD SB700/710/750 BIOS Developer's Guide (BDG), Rev. 1.00,
  *          PN 43366_sb7xx_bdg_pub_1.00, June 2009, section 3.1, page 14.
@@ -57,8 +57,18 @@ 
 	 * Enable LPC ROM range start at:
 	 * 0xfff8(0000): 512KB
 	 * 0xfff0(0000): 1MB
+	 * 0xffe0(0000): 2MB
+	 * 0xffc0(0000): 4MB
 	 */
+	#if CONFIG_COREBOOT_ROMSIZE_KB_512 || CONFIG_COREBOOT_ROMSIZE_KB_256 || CONFIG_COREBOOT_ROMSIZE_KB_128
+	pci_write_config16(dev, 0x6c, 0xfff8); /* 512KB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_1024
 	pci_write_config16(dev, 0x6c, 0xfff0); /* 1 MB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_2048
+	pci_write_config16(dev, 0x6c, 0xffe0); /* 2 MB */
+	#elif CONFIG_COREBOOT_ROMSIZE_KB_4096
+	pci_write_config16(dev, 0x6c, 0xffc0); /* 4 MB */
+	#endif
 	/* Enable LPC ROM range end at 0xffff(ffff). */
 	pci_write_config16(dev, 0x6e, 0xffff);
 }