[PATCH 3/5] artecgroup/dbe61: Gather RAM initialization function calls to one helper function.
artecgroup/dbe61: Gather RAM initialization function calls to one helper function.
Then we can later use it for re-initializing for different SPD without code duplication.
Signed-off-by: Mart Raudsepp mart.raudsepp@artecdesign.ee --- mainboard/artecgroup/dbe61/initram.c | 27 ++++++++++++++++----------- 1 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/mainboard/artecgroup/dbe61/initram.c b/mainboard/artecgroup/dbe61/initram.c index 4a89247..d454e46 100644 --- a/mainboard/artecgroup/dbe61/initram.c +++ b/mainboard/artecgroup/dbe61/initram.c @@ -135,6 +135,21 @@ static void mb_gpio_init(void) /* Early mainboard specific GPIO setup */ }
+static void initialize_ram(u8 dimm0, u8 dimm1) +{ + cpu_reg_init(0, dimm0, dimm1, DRAM_UNTERMINATED); + printk(BIOS_DEBUG, "done cpu reg init\n"); + + sdram_set_registers(); + printk(BIOS_DEBUG, "done sdram set registers\n"); + + sdram_set_spd_registers(dimm0, dimm1); + printk(BIOS_DEBUG, "done sdram set spd registers\n"); + + sdram_enable(dimm0, dimm1); + printk(BIOS_DEBUG, "done sdram enable\n"); +} + /** * main for initram for the PC Engines Alix 1C. It might seem that you * could somehow do these functions in, e.g., the cpu code, but the @@ -155,17 +170,7 @@ int main(void) pll_reset(MANUALCONF, PLLMSRHI, PLLMSRLO); printk(BIOS_DEBUG, "done pll reset\n");
- cpu_reg_init(0, DIMM_DBE61C, DIMM_EMPTY, DRAM_UNTERMINATED); - printk(BIOS_DEBUG, "done cpu reg init\n"); - - sdram_set_registers(); - printk(BIOS_DEBUG, "done sdram set registers\n"); - - sdram_set_spd_registers(DIMM_DBE61C, DIMM_EMPTY); - printk(BIOS_DEBUG, "done sdram set spd registers\n"); - - sdram_enable(DIMM_DBE61C, DIMM_EMPTY); - printk(BIOS_DEBUG, "done sdram enable\n"); + initialize_ram(DIMM_DBE61C, DIMM_EMPTY);
/* Check low memory */ /*ram_check(0x00000000, 640*1024); */
Mart Raudsepp wrote:
artecgroup/dbe61: Gather RAM initialization function calls to one helper function.
Then we can later use it for re-initializing for different SPD without code duplication.
Signed-off-by: Mart Raudsepp mart.raudsepp@artecdesign.ee
Acked-by: Peter Stuge peter@stuge.se
On 13.11.2008 01:19, Mart Raudsepp wrote:
artecgroup/dbe61: Gather RAM initialization function calls to one helper function.
Then we can later use it for re-initializing for different SPD without code duplication.
Signed-off-by: Mart Raudsepp mart.raudsepp@artecdesign.ee
Looks fine at first glance, but we want to keep Geode targets consistent. Are the other Geode targets available for the same treatment? If yes, please post a patch converting all of them.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
we want to keep Geode targets consistent.
Says who?
If a board is unique and we can't abstract in general code then mainboard/ code is fine.
//Peter
On 13.11.2008 02:41, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
we want to keep Geode targets consistent.
Says who?
I say so and my clone agrees. ;-)
If a board is unique and we can't abstract in general code then mainboard/ code is fine.
If the abstraction works for all the other boards, there is no reason to have different codebases except to confuse people who want to port a new similar board.
We have lots of places in v2 where people made some change to one specific board, but the change would have applied to lots of boards. Later on, nobody could recall offhand why the files were different. To be honest, the amount of code duplication we have in v2 with little arbitrary changes sprinkled all over the map is one of the biggest reasons why I try to avoid v2 wherever possible. Diffing two boards will usually give you lots of differences which are in no way related to the board configuration/hardware.
Regards, Carl-Daniel
On N, 2008-11-13 at 03:11 +0100, Carl-Daniel Hailfinger wrote:
On 13.11.2008 02:41, Peter Stuge wrote:
Carl-Daniel Hailfinger wrote:
we want to keep Geode targets consistent.
Says who?
I say so and my clone agrees. ;-)
If a board is unique and we can't abstract in general code then mainboard/ code is fine.
If the abstraction works for all the other boards, there is no reason to have different codebases except to confuse people who want to port a new similar board.
We have lots of places in v2 where people made some change to one specific board, but the change would have applied to lots of boards. Later on, nobody could recall offhand why the files were different. To be honest, the amount of code duplication we have in v2 with little arbitrary changes sprinkled all over the map is one of the biggest reasons why I try to avoid v2 wherever possible. Diffing two boards will usually give you lots of differences which are in no way related to the board configuration/hardware.
The difference here is that in DBE61 I call that sequence possibly multiple times, while for all others that is not the case, and it's just an unnecessary function call.
I suppose we could add such a helper function for all boards if an inline keyword is added for it.
That said, there's actually some things to improve there regarding what is called. For instance cpu_reg_init does many things, and only one part is memory related - at least where the SPD matters. Some other bits and pieces are also re-done on second call that might not be necessary. So I'm not sure if in the long run it's suitable for all boards. Perhaps that initialize_ram deal could end up in generic code even if the order of those operations has to always be like that (does it?).
Regards, Mart Raudsepp
On 13.11.2008 04:12, Mart Raudsepp wrote:
On N, 2008-11-13 at 03:11 +0100, Carl-Daniel Hailfinger wrote:
On 13.11.2008 02:41, Peter Stuge wrote:
If a board is unique and we can't abstract in general code then mainboard/ code is fine.
If the abstraction works for all the other boards, there is no reason to have different codebases except to confuse people who want to port a new similar board.
We have lots of places in v2 where people made some change to one specific board, but the change would have applied to lots of boards. Later on, nobody could recall offhand why the files were different. To be honest, the amount of code duplication we have in v2 with little arbitrary changes sprinkled all over the map is one of the biggest reasons why I try to avoid v2 wherever possible. Diffing two boards will usually give you lots of differences which are in no way related to the board configuration/hardware.
The difference here is that in DBE61 I call that sequence possibly multiple times, while for all others that is not the case, and it's just an unnecessary function call.
I suppose we could add such a helper function for all boards if an inline keyword is added for it.
That said, there's actually some things to improve there regarding what is called. For instance cpu_reg_init does many things, and only one part is memory related - at least where the SPD matters. Some other bits and pieces are also re-done on second call that might not be necessary. So I'm not sure if in the long run it's suitable for all boards. Perhaps that initialize_ram deal could end up in generic code even if the order of those operations has to always be like that (does it?).
You're right AFAICS.
The GeodeLX RAM init code also suffers from ROMCC mentality. Stuff is recalculated instead of being passed as function parameters. A prime example is the check whether a SPD is in a given slot. Moving that check outside the functions would probably kill a dozen lines of code, if not more.
Regards, Carl-Daniel
Carl-Daniel Hailfinger wrote:
The GeodeLX RAM init code also suffers from ROMCC mentality.
Yes, this is why I think v3 is not in such a good shape. I also think that by shaping it up we could pull out several things that can be reused all over the place.
A prime example is the check whether a SPD is in a given slot. Moving that check outside the functions would probably kill a dozen lines of code, if not more.
I'd love to review and ack the patch if you feel like improving that!
//Peter
Carl-Daniel Hailfinger wrote:
we want to keep Geode targets consistent.
Says who?
I say so and my clone agrees. ;-)
Well speak for yourself. :) I sure don't want to keep Geode targets consistent at all costs. I don't think that will be useful.
If a board is unique and we can't abstract in general code then mainboard/ code is fine.
If the abstraction works for all the other boards, there is no reason to have different codebases except to confuse people who want to port a new similar board.
We have lots of places in v2 where people made some change to one specific board, but the change would have applied to lots of boards.
In that case, please apply the change to more places. Better yet, move the common code out of board specific code. I promise to review and ack swiftly.
Later on, nobody could recall offhand why the files were different.
Do you see this being a problem in this dual fake SPD and RAM chip case anytime soon?
To be honest, the amount of code duplication we have in v2 with little arbitrary changes sprinkled all over the map is one of the biggest reasons why I try to avoid v2 wherever possible.
Cool. Feel free to help improve this in v3 by sending fixes when you identify possible improvements.
//Peter
-
Carl-Daniel Hailfinger -
Mart Raudsepp -
Peter Stuge