CAR size and CAR base defines are scattered all over the place. Set them centrally from Kconfig, but keep the Kconfig variables hidden. That way, they are available everywhere, you don't have to try to guess where they are set, and they come with help text if you look at arch/x86/Kconfig. No semantic changes, although some of the settings really could use an overhaul.
This also is a requirement for my printk buffer patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Index: LinuxBIOSv3-carsize_Kconfig/include/arch/x86/amd_geodelx.h =================================================================== --- LinuxBIOSv3-carsize_Kconfig/include/arch/x86/amd_geodelx.h (Revision 586) +++ LinuxBIOSv3-carsize_Kconfig/include/arch/x86/amd_geodelx.h (Arbeitskopie) @@ -565,8 +565,8 @@ #define SMM_SIZE 128 /* changed SMM_SIZE from 256 KB to 128 KB */
/* ------------------------ */ -#define DCACHE_RAM_SIZE 0x08000 -#define DCACHE_RAM_BASE 0x80000 +#define DCACHE_RAM_SIZE CONFIG_CARSIZE +#define DCACHE_RAM_BASE CONFIG_CARBASE /* This is where the DCache will be mapped and be used as stack. It would be * cool if it was the same base as coreboot normal stack. */ Index: LinuxBIOSv3-carsize_Kconfig/arch/x86/Kconfig =================================================================== --- LinuxBIOSv3-carsize_Kconfig/arch/x86/Kconfig (Revision 586) +++ LinuxBIOSv3-carsize_Kconfig/arch/x86/Kconfig (Arbeitskopie) @@ -69,4 +69,17 @@ coreboot work correctly on symmetric multi processor systems. It is usually set in mainboard/*/Kconfig. - + +config CARBASE + hex + default 0x8f000 if CPU_I586 + default 0x80000 if CPU_AMD_GEODELX + help + This option sets the base address of the area used for CAR. + +config CARSIZE + hex + default 0x1000 if CPU_I586 + default 0x8000 if CPU_AMD_GEODELX + help + This option sets the size of the area used for CAR. Index: LinuxBIOSv3-carsize_Kconfig/arch/x86/stage0_i586.S =================================================================== --- LinuxBIOSv3-carsize_Kconfig/arch/x86/stage0_i586.S (Revision 586) +++ LinuxBIOSv3-carsize_Kconfig/arch/x86/stage0_i586.S (Arbeitskopie) @@ -183,18 +183,9 @@ * the other is very similar to the AMD CAR, except remove amd specific msr */
-#ifndef CONFIG_CARSIZE -#define CacheSize 4096 -#else #define CacheSize CONFIG_CARSIZE -#endif
-/* pick a safer value for default -- i.e. not the C segment! */ -#ifndef CONFIG_CARBASE -#define CacheBase (0x90000 - CacheSize) -#else #define CacheBase CONFIG_CARBASE -#endif
#define ASSEMBLY #include "mtrr.h"
On Mon, Feb 11, 2008 at 01:57:40AM +0100, Carl-Daniel Hailfinger wrote:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Peter Stuge peter@stuge.se
Index: LinuxBIOSv3-carsize_Kconfig/include/arch/x86/amd_geodelx.h
--- LinuxBIOSv3-carsize_Kconfig/include/arch/x86/amd_geodelx.h (Revision 586) +++ LinuxBIOSv3-carsize_Kconfig/include/arch/x86/amd_geodelx.h (Arbeitskopie) @@ -565,8 +565,8 @@ #define SMM_SIZE 128 /* changed SMM_SIZE from 256 KB to 128 KB */
/* ------------------------ */ -#define DCACHE_RAM_SIZE 0x08000 -#define DCACHE_RAM_BASE 0x80000 +#define DCACHE_RAM_SIZE CONFIG_CARSIZE +#define DCACHE_RAM_BASE CONFIG_CARBASE /* This is where the DCache will be mapped and be used as stack. It would be
- cool if it was the same base as coreboot normal stack.
*/ Index: LinuxBIOSv3-carsize_Kconfig/arch/x86/Kconfig =================================================================== --- LinuxBIOSv3-carsize_Kconfig/arch/x86/Kconfig (Revision 586) +++ LinuxBIOSv3-carsize_Kconfig/arch/x86/Kconfig (Arbeitskopie) @@ -69,4 +69,17 @@ coreboot work correctly on symmetric multi processor systems. It is usually set in mainboard/*/Kconfig.
+config CARBASE
- hex
- default 0x8f000 if CPU_I586
- default 0x80000 if CPU_AMD_GEODELX
- help
This option sets the base address of the area used for CAR.+config CARSIZE
- hex
- default 0x1000 if CPU_I586
- default 0x8000 if CPU_AMD_GEODELX
- help
This option sets the size of the area used for CAR.Index: LinuxBIOSv3-carsize_Kconfig/arch/x86/stage0_i586.S
--- LinuxBIOSv3-carsize_Kconfig/arch/x86/stage0_i586.S (Revision 586) +++ LinuxBIOSv3-carsize_Kconfig/arch/x86/stage0_i586.S (Arbeitskopie) @@ -183,18 +183,9 @@
- the other is very similar to the AMD CAR, except remove amd specific msr
*/
-#ifndef CONFIG_CARSIZE -#define CacheSize 4096 -#else #define CacheSize CONFIG_CARSIZE -#endif
-/* pick a safer value for default -- i.e. not the C segment! */ -#ifndef CONFIG_CARBASE -#define CacheBase (0x90000 - CacheSize) -#else #define CacheBase CONFIG_CARBASE -#endif
#define ASSEMBLY #include "mtrr.h"
On 11.02.2008 02:01, Peter Stuge wrote:
On Mon, Feb 11, 2008 at 01:57:40AM +0100, Carl-Daniel Hailfinger wrote:
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
Acked-by: Peter Stuge peter@stuge.se
Thanks, r587.
Regards, Carl-Daniel
I don't want to join this scrum (http://www.thefreedictionary.com/scrum) but I see we have one NAK and one ACK, which I believe means no action to take yet.
I'm neutral, as it all confuses me too much.
ron
On 11.02.2008 02:34, ron minnich wrote:
I don't want to join this scrum (http://www.thefreedictionary.com/scrum) but I see we have one NAK and one ACK, which I believe means no action to take yet.
I'm neutral, as it all confuses me too much.
OK, so should I revert?
Regards, Carl-Daniel
* Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080211 01:57]:
CAR size and CAR base defines are scattered all over the place. Set them centrally from Kconfig, but keep the Kconfig variables hidden. That way, they are available everywhere, you don't have to try to guess where they are set, and they come with help text if you look at arch/x86/Kconfig. No semantic changes, although some of the settings really could use an overhaul.
This also is a requirement for my printk buffer patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
NACK. Search the archives for previous discussion on this. The config system is not there to set code internal only variables. That's what the arch/x86 cpu dependent include files are there for.
[sorry, saw your mail too late]
On 11.02.2008 02:03, Stefan Reinauer wrote:
- Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net [080211 01:57]:
CAR size and CAR base defines are scattered all over the place. Set them centrally from Kconfig, but keep the Kconfig variables hidden. That way, they are available everywhere, you don't have to try to guess where they are set, and they come with help text if you look at arch/x86/Kconfig. No semantic changes, although some of the settings really could use an overhaul.
This also is a requirement for my printk buffer patch.
Signed-off-by: Carl-Daniel Hailfinger c-d.hailfinger.devel.2006@gmx.net
NACK. Search the archives for previous discussion on this.
The archives do not turn up any relevant hits for "CAR Kconfig" or "cache Kconfig". More details about what to search for would be appreciated.
The config system is not there to set code internal only variables. That's what the arch/x86 cpu dependent include files are there for.
Actually, this is one of the good aspects of v2: setting subarch-dependant variables dependant on subarch and/or mainboard.
How do you define "code internal only variables"? I think variables with arch-dependent value (as opposed to arch-dependent existence) should be set by Kconfig. There is no reason to have stuff like LX_NUM_CACHELINES in Kconfig because it doesn't even exist on other subarches.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
The config system is not there to set code internal only variables. That's what the arch/x86 cpu dependent include files are there for.
Actually, this is one of the good aspects of v2: setting subarch-dependant variables dependant on subarch and/or mainboard.
We start reveiling too much information in the configuration process, implying this is something that could be changed on a per-board basis. In reality it is just a part of the code that we source out now. The CAR code for a given platform should know how to get CAR working on that platform.
My prognosis is we will end up with different settings per mainboard, like we had in v2. And in 6 months noone will know anymore why we have these different settings. In v2 you need a larger CAR area if you want to enable some features triggered by other CONFIG variables. So there's a dependency, but it is up to the user to manually resolve it. Just enabling those features will not work anymore, because you need to know that you also have to increase CAR.
The next step is to pack all kinds of logic into the config mechanism that does not belong there. In v2 we even calculated the offsets of normal and fallback in the config file. You enable a small piece of code on the Xth board using a given mainboard and things will fall apart.
I think CAR_BASE and CAR_SIZE should be a property encapsulated in the CAR code component. If some component needs to change those values, they should provide a "reason" rather than just values. As the complexity of v3 increases, the CAR component can then locally look at all the "reasons" influencing required CAR size and react upon that. Locally. Not globally. Kconfig stuff is global. Global is bad.
How do you define "code internal only variables"? I think variables with arch-dependent value (as opposed to arch-dependent existence) should be set by Kconfig. There is no reason to have stuff like LX_NUM_CACHELINES in Kconfig because it doesn't even exist on other subarches.
CONFIG stuff should be feature driven, not code driven. A feature is something like the "printk log buffer". If you want that, you might need to increase the CAR size. The code needs to know that, and change the CAR values accordingly.
Otherwise the fragility of the code is increased by the fragility of the Kconfig mechanism. If someone thinks it is necessary to adjust CAR_BASE and CAR_SIZE via Kconfig, the logic of the approach is broken. It means the code needs fixing. Not hotfixing.
There is indeed no reason for LX_NUM_CACHELINES in Kconfig either. Keeping it there is as broken as your last change. That's not a configuration aspekt. You can't configure your board to have more cache lines.
Packing all this stuff into Kconfig looks very developer friendly at first, while it is going to betray us in the same way it did and does in v2. It reduces the testability and reusability of the code while implicating that everything got more flexible.
See also http://en.wikipedia.org/wiki/Feature_Driven_Development
Stefan
On 11.02.2008 03:42, Stefan Reinauer wrote:
Carl-Daniel Hailfinger wrote:
The config system is not there to set code internal only variables. That's what the arch/x86 cpu dependent include files are there for.
Actually, this is one of the good aspects of v2: setting subarch-dependant variables dependant on subarch and/or mainboard.
We start reveiling too much information in the configuration process, implying this is something that could be changed on a per-board basis. In reality it is just a part of the code that we source out now. The CAR code for a given platform should know how to get CAR working on that platform.
Same could be said for serial, yet we offer configurability (in the dts) for serial iobase even on a mainboard level.
My prognosis is we will end up with different settings per mainboard, like we had in v2.
How so? The code I committed selects an invisible Kconfig variable based on the subarch alone. A mainboard porter will never see the variable nor does he need to know it exists.
And in 6 months noone will know anymore why we have these different settings. In v2 you need a larger CAR area if you want to enable some features triggered by other CONFIG variables. So there's a dependency, but it is up to the user to manually resolve it. Just enabling those features will not work anymore, because you need to know that you also have to increase CAR.
And manual resolution is prone to failure because the user needs to know the dependencies. That's why having that stuff in Kconfig in v3 makes things easier. Besides that, code which just fails to build (or even worse, fails to run) without giving a user-understandable error message is just broken. My CAR changes in v2 and v3 fixed some of the most glaring problems in that area for CAR.
The next step is to pack all kinds of logic into the config mechanism that does not belong there. In v2 we even calculated the offsets of normal and fallback in the config file. You enable a small piece of code on the Xth board using a given mainboard and things will fall apart.
I think CAR_BASE and CAR_SIZE should be a property encapsulated in the CAR code component. If some component needs to change those values, they should provide a "reason" rather than just values. As the complexity of v3 increases, the CAR component can then locally look at all the "reasons" influencing required CAR size and react upon that. Locally. Not globally. Kconfig stuff is global. Global is bad.
Oh, CARBASE must be global anyway because printk needs to know it for the buffer address calculation. That would make CARBASE a perfect fit for Kconfig.
How do you define "code internal only variables"? I think variables with arch-dependent value (as opposed to arch-dependent existence) should be set by Kconfig. There is no reason to have stuff like LX_NUM_CACHELINES in Kconfig because it doesn't even exist on other subarches.
CONFIG stuff should be feature driven, not code driven. A feature is something like the "printk log buffer". If you want that, you might need to increase the CAR size. The code needs to know that, and change the CAR values accordingly.
CONSOLE_LOGLEVEL is code driven, yet nobody is arguing for its removal.
Otherwise the fragility of the code is increased by the fragility of the Kconfig mechanism. If someone thinks it is necessary to adjust CAR_BASE and CAR_SIZE via Kconfig, the logic of the approach is broken. It means the code needs fixing. Not hotfixing.
CARBASE can not be adjusted with any config interface. It is a hidden variable. I still fail to see where the logic of the approach is broken.
There is indeed no reason for LX_NUM_CACHELINES in Kconfig either. Keeping it there is as broken as your last change. That's not a configuration aspekt. You can't configure your board to have more cache lines.
Packing all this stuff into Kconfig looks very developer friendly at first, while it is going to betray us in the same way it did and does in v2. It reduces the testability and reusability of the code while implicating that everything got more flexible.
How does it reduce testability and usability over lots of #ifdefs?
See also http://en.wikipedia.org/wiki/Feature_Driven_Development
That seems to be a bad model for us. The wikipedia page says 3% of the total effort are spent on design review. If we really did that, nobody would have had the time to question my design to have CARBASE in Kconfig.
Regards, Carl-Daniel
On Mon, Feb 11, 2008 at 02:39:15AM +0100, Carl-Daniel Hailfinger wrote:
The config system is not there to set code internal only variables.
Hmm.
That's what the arch/x86 cpu dependent include files are there for.
Which files?
How do you define "code internal only variables"? I think variables with arch-dependent value (as opposed to arch-dependent existence) should be set by Kconfig.
..also valid point. This is why I liked it - the values always need to be set. But Stefan is right in that we are overloading a configuration mechanism.
Linux solves this by using Kconfig to pick what objects to build, and two objects can provide the same functions but with different implementation. Do we just want better code abstraction? Could a #define be used to #include one .S from another .S ?
There is no reason to have stuff like LX_NUM_CACHELINES in Kconfig because it doesn't even exist on other subarches.
Yes, agreed.
//Peter
On 11.02.2008 03:53, Peter Stuge wrote:
On Mon, Feb 11, 2008 at 02:39:15AM +0100, Carl-Daniel Hailfinger wrote:
The config system is not there to set code internal only variables.
Hmm.
That's what the arch/x86 cpu dependent include files are there for.
Which files?
Stefan?
How do you define "code internal only variables"? I think variables with arch-dependent value (as opposed to arch-dependent existence) should be set by Kconfig.
..also valid point. This is why I liked it - the values always need to be set. But Stefan is right in that we are overloading a configuration mechanism.
Linux solves this by using Kconfig to pick what objects to build, and two objects can provide the same functions but with different implementation. Do we just want better code abstraction? Could a #define be used to #include one .S from another .S ?
CARBASE is like SMP (yes, it's in Kconfig) and DEFAULT_LOGLEVEL (in Kconfig as well) in that all of these variables do not select which objects to build. Should we remove all these variables from Kconfig?
There is no reason to have stuff like LX_NUM_CACHELINES in Kconfig because it doesn't even exist on other subarches.
Yes, agreed.
Of course. LX_NUM_CACHELINES is just a symbolic constant with no dependency on any configuration variable. CARBASE is a config variable depending on another configuration variable. It's that simple.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
Peter Stuge -
ron minnich -
Stefan Reinauer