Re: [coreboot] [commit] r5359 - in trunk/src: . cpu/amd/model_10xxxcpu/amd/model_fxx cpu/amd/model_lx cpu/x86/mtrrinclude/cpu/x86 mainboard/amd/mahogany_fam10mainboard/amd/serengeti_cheetah mainboard/asus/a8n_...
wasn't meant to be private only.
On 4/7/10 12:24 AM, Stefan Reinauer wrote:
On 4/7/10 12:06 AM, Myles Watson wrote:
-----Original Message----- From: coreboot-bounces@coreboot.org [mailto:coreboot-bounces@coreboot.org] On Behalf Of repository service Sent: Tuesday, April 06, 2010 3:50 PM To: coreboot@coreboot.org Subject: [coreboot] [commit] r5359 - in trunk/src: . cpu/amd/model_10xxxcpu/amd/model_fxx cpu/amd/model_lx cpu/x86/mtrrinclude/cpu/x86 mainboard/amd/mahogany_fam10mainboard/amd/serengeti_cheetah mainboard/asus/a8n_...
Author: stepan Date: Tue Apr 6 21:50:21 2010 New Revision: 5359 URL: https://tracker.coreboot.org/trac/coreboot/changeset/5359
Log: No warnings day, next round.
Thanks.
trunk/src/cpu/x86/mtrr/earlymtrr.c
trunk/src/include/cpu/x86/mtrr.h
Modified: trunk/src/cpu/x86/mtrr/earlymtrr.c
==== --- trunk/src/cpu/x86/mtrr/earlymtrr.c Tue Apr 6 21:49:31 2010 (r5358) +++ trunk/src/cpu/x86/mtrr/earlymtrr.c Tue Apr 6 21:50:21 2010 (r5359) @@ -4,37 +4,7 @@ #include <cpu/x86/mtrr.h> #include <cpu/x86/msr.h>
-/* Validate CONFIG_XIP_ROM_SIZE and CONFIG_XIP_ROM_BASE */ -#if defined(CONFIG_XIP_ROM_SIZE) && !defined(CONFIG_XIP_ROM_BASE) -# error "CONFIG_XIP_ROM_SIZE without CONFIG_XIP_ROM_BASE"
... I'm surprised you wanted to move this. It made sense to me to check it when compiling CAR.
The idea was to be able to have the check, no matter if only cpu/x86/earlymtrr.c or only cpu/amd/amd_earlymtrr.c is included.
It seems that in the long term most of the earlymtrr code can completely go away though, so I was not sure where to put it.
Modified: trunk/src/include/cpu/x86/mtrr.h
==== --- trunk/src/include/cpu/x86/mtrr.h Tue Apr 6 21:49:31 2010 (r5358) +++ trunk/src/include/cpu/x86/mtrr.h Tue Apr 6 21:50:21 2010 (r5359)
...
+#if !defined(CONFIG_RAMTOP) +# error "CONFIG_RAMTOP not defined" +#endif
Now that we have Kconfig, I think this check can disappear. Is there a way to un-define CONFIG_RAMTOP?
In theory, yes. If you say this in Kconfig:
config FOO bool depends on SOME_FEATURE
config BAR bool depends on FOO
You might end up with BAR not being there even though we modified Kconfig to always set 0 bools to 0 instead of omitting them. That's because Kconfig completely skips BAR if FOO is not set.
With RAMTOP being
config RAMTOP hex default 0x200000
it can't happen.
But I didn't want to change the rough logical flow with self-acked code, so I thought I better leave an error check too many in there ;-)
If you feel it should go, you have my Acked-by: Stefan Reinauer stepan@coresystems.de
Now that we have Kconfig, I think this check can disappear. Is there a
way
to un-define CONFIG_RAMTOP?
With RAMTOP being
config RAMTOP hex default 0x200000
it can't happen.
But I didn't want to change the rough logical flow with self-acked code, so I thought I better leave an error check too many in there ;-)
I agree that it's good to be careful.
I think it clutters this file to have so many checks. I wonder if there should be some central file for checking Kconfig variables.
Thanks, Myles
-
Myles Watson -
Stefan Reinauer