Kyösti Mälkki has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/38267 )
Change subject: intel/e7505,i82801dx: Refactor raminit ......................................................................
intel/e7505,i82801dx: Refactor raminit
Change-Id: I077e455242db9fc0f86432bd1afab75cb6fb6f4c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/northbridge/intel/e7505/raminit.c M src/northbridge/intel/e7505/raminit.h M src/northbridge/intel/e7505/romstage.c M src/southbridge/intel/i82801dx/early_smbus.c M src/southbridge/intel/i82801dx/i82801dx.h 5 files changed, 43 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/38267/1
diff --git a/src/northbridge/intel/e7505/raminit.c b/src/northbridge/intel/e7505/raminit.c index 6b7a041..9d82634 100644 --- a/src/northbridge/intel/e7505/raminit.c +++ b/src/northbridge/intel/e7505/raminit.c @@ -1759,28 +1759,38 @@ d060_control(D060_CMD_1); }
-/** - * - * - */ -void e7505_mch_init(const struct mem_controller *memctrl) -{ - timestamp_add_now(TS_BEFORE_INITRAM); - - sdram_set_registers(memctrl); - sdram_set_spd_registers(memctrl); - sdram_enable(memctrl); -} - -void e7505_mch_done(const struct mem_controller *memctrl) -{ - sdram_post_ecc(memctrl); - - timestamp_add_now(TS_AFTER_INITRAM); -} - -int e7505_mch_is_ready(void) +static int e7505_mch_is_ready(void) { uint32_t dword = pci_read_config32(MCHDEV, DRC); return !!(dword & DRC_DONE); } + +void sdram_initialize(void) +{ + static const struct mem_controller memctrl[] = { + { + .d0 = PCI_DEV(0, 0, 0), + .d0f1 = PCI_DEV(0, 0, 1), + .channel0 = { 0x50, 0x52, 0, 0 }, + .channel1 = { 0x51, 0x53, 0, 0 }, + }, + }; + + /* If this is a warm boot, some initialization can be skipped */ + if (!e7505_mch_is_ready()) { + + /* The real MCH initialisation. */ + timestamp_add_now(TS_BEFORE_INITRAM); + + sdram_set_registers(memctrl); + sdram_set_spd_registers(memctrl); + sdram_enable(memctrl); + + /* Hook for post ECC scrub settings and debug. */ + sdram_post_ecc(memctrl); + + timestamp_add_now(TS_AFTER_INITRAM); + } + + printk(BIOS_DEBUG, "SDRAM is up.\n"); +} diff --git a/src/northbridge/intel/e7505/raminit.h b/src/northbridge/intel/e7505/raminit.h index c65675f..9aa6eb4 100644 --- a/src/northbridge/intel/e7505/raminit.h +++ b/src/northbridge/intel/e7505/raminit.h @@ -30,8 +30,6 @@ uint16_t channel1[MAX_DIMM_SOCKETS_PER_CHANNEL]; };
-void e7505_mch_init(const struct mem_controller *memctrl); -void e7505_mch_done(const struct mem_controller *memctrl); -int e7505_mch_is_ready(void); +void sdram_initialize(void);
#endif /* RAMINIT_H */ diff --git a/src/northbridge/intel/e7505/romstage.c b/src/northbridge/intel/e7505/romstage.c index 6c74c1f..f506bf4 100644 --- a/src/northbridge/intel/e7505/romstage.c +++ b/src/northbridge/intel/e7505/romstage.c @@ -13,9 +13,7 @@ * GNU General Public License for more details. */
-#include <stdint.h> #include <cbmem.h> -#include <console/console.h> #include <arch/romstage.h>
#include <southbridge/intel/i82801dx/i82801dx.h> @@ -23,27 +21,12 @@
void mainboard_romstage_entry(void) { - static const struct mem_controller memctrl[] = { - { - .d0 = PCI_DEV(0, 0, 0), - .d0f1 = PCI_DEV(0, 0, 1), - .channel0 = { 0x50, 0x52, 0, 0 }, - .channel1 = { 0x51, 0x53, 0, 0 }, - }, - }; + /* Perform some early chipset initialization required + * before RAM initialization can work + */ + i82801dx_early_init();
- /* If this is a warm boot, some initialization can be skipped */ - if (!e7505_mch_is_ready()) { - enable_smbus(); - - /* The real MCH initialisation. */ - e7505_mch_init(memctrl); - - /* Hook for post ECC scrub settings and debug. */ - e7505_mch_done(memctrl); - } - - printk(BIOS_DEBUG, "SDRAM is up.\n"); + sdram_initialize();
cbmem_recovery(0); } diff --git a/src/southbridge/intel/i82801dx/early_smbus.c b/src/southbridge/intel/i82801dx/early_smbus.c index 77b0aa0..de0bc93 100644 --- a/src/southbridge/intel/i82801dx/early_smbus.c +++ b/src/southbridge/intel/i82801dx/early_smbus.c @@ -21,6 +21,11 @@
#include "i82801dx.h"
+void i82801dx_early_init(void) +{ + enable_smbus(); +} + void enable_smbus(void) { pci_devfn_t dev = PCI_DEV(0x0, 0x1f, 0x3); diff --git a/src/southbridge/intel/i82801dx/i82801dx.h b/src/southbridge/intel/i82801dx/i82801dx.h index 50122d8..8717e59 100644 --- a/src/southbridge/intel/i82801dx/i82801dx.h +++ b/src/southbridge/intel/i82801dx/i82801dx.h @@ -35,6 +35,7 @@ #include "chip.h"
void i82801dx_enable(struct device *dev); +void i82801dx_early_init(void); void enable_smbus(void); int smbus_read_byte(unsigned int device, unsigned int address); void aseg_smm_lock(void);
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38267 )
Change subject: intel/e7505,i82801dx: Refactor raminit ......................................................................
Patch Set 1:
Did I read raminit?
It would be nice to check that this still boots. I guess that's not a problem, but I'd indicate it in the commit message:
Tested on aopen/dxplplusu, still boots.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38267 )
Change subject: intel/e7505,i82801dx: Refactor raminit ......................................................................
Patch Set 1: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/38267/1/src/northbridge/intel/e7505... File src/northbridge/intel/e7505/raminit.c:
https://review.coreboot.org/c/coreboot/+/38267/1/src/northbridge/intel/e7505... PS1, Line 1782: initialisation Minor: Though both forms are correct, now the spelling of this doesn't match the comment above.
https://review.coreboot.org/c/coreboot/+/38267/1/src/southbridge/intel/i8280... File src/southbridge/intel/i82801dx/i82801dx.h:
https://review.coreboot.org/c/coreboot/+/38267/1/src/southbridge/intel/i8280... PS1, Line 38: void i82801dx_early_init(void); : void enable_smbus(void); Now, these two do the same thing. I guess this change later?
Hello Patrick Rudolph, Angel Pons, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/38267
to look at the new patch set (#2).
Change subject: intel/e7505,i82801dx: Refactor raminit ......................................................................
intel/e7505,i82801dx: Refactor raminit
Avoid direct enable_smbus() call from northbridge code.
Change-Id: I077e455242db9fc0f86432bd1afab75cb6fb6f4c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com --- M src/northbridge/intel/e7505/raminit.c M src/northbridge/intel/e7505/raminit.h M src/northbridge/intel/e7505/romstage.c M src/southbridge/intel/i82801dx/early_smbus.c M src/southbridge/intel/i82801dx/i82801dx.h 5 files changed, 43 insertions(+), 46 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/67/38267/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/38267 )
Change subject: intel/e7505,i82801dx: Refactor raminit ......................................................................
Patch Set 2: Code-Review+2
(2 comments)
https://review.coreboot.org/c/coreboot/+/38267/1/src/northbridge/intel/e7505... File src/northbridge/intel/e7505/raminit.c:
https://review.coreboot.org/c/coreboot/+/38267/1/src/northbridge/intel/e7505... PS1, Line 1782: initialisation
Minor: Though both forms are correct, now the spelling of this doesn't match the comment above.
Done
https://review.coreboot.org/c/coreboot/+/38267/1/src/southbridge/intel/i8280... File src/southbridge/intel/i82801dx/i82801dx.h:
https://review.coreboot.org/c/coreboot/+/38267/1/src/southbridge/intel/i8280... PS1, Line 38: void i82801dx_early_init(void); : void enable_smbus(void);
Now, these two do the same thing. […]
Ack
Kyösti Mälkki has submitted this change. ( https://review.coreboot.org/c/coreboot/+/38267 )
Change subject: intel/e7505,i82801dx: Refactor raminit ......................................................................
intel/e7505,i82801dx: Refactor raminit
Avoid direct enable_smbus() call from northbridge code.
Change-Id: I077e455242db9fc0f86432bd1afab75cb6fb6f4c Signed-off-by: Kyösti Mälkki kyosti.malkki@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/38267 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/e7505/raminit.c M src/northbridge/intel/e7505/raminit.h M src/northbridge/intel/e7505/romstage.c M src/southbridge/intel/i82801dx/early_smbus.c M src/southbridge/intel/i82801dx/i82801dx.h 5 files changed, 43 insertions(+), 46 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/e7505/raminit.c b/src/northbridge/intel/e7505/raminit.c index 972a2f3..7953aca 100644 --- a/src/northbridge/intel/e7505/raminit.c +++ b/src/northbridge/intel/e7505/raminit.c @@ -1740,28 +1740,38 @@ d060_control(D060_CMD_1); }
-/** - * - * - */ -void e7505_mch_init(const struct mem_controller *memctrl) -{ - timestamp_add_now(TS_BEFORE_INITRAM); - - sdram_set_registers(memctrl); - sdram_set_spd_registers(memctrl); - sdram_enable(memctrl); -} - -void e7505_mch_done(const struct mem_controller *memctrl) -{ - sdram_post_ecc(memctrl); - - timestamp_add_now(TS_AFTER_INITRAM); -} - -int e7505_mch_is_ready(void) +static int e7505_mch_is_ready(void) { uint32_t dword = pci_read_config32(MCHDEV, DRC); return !!(dword & DRC_DONE); } + +void sdram_initialize(void) +{ + static const struct mem_controller memctrl[] = { + { + .d0 = PCI_DEV(0, 0, 0), + .d0f1 = PCI_DEV(0, 0, 1), + .channel0 = { 0x50, 0x52, 0, 0 }, + .channel1 = { 0x51, 0x53, 0, 0 }, + }, + }; + + /* If this is a warm boot, some initialisation can be skipped */ + if (!e7505_mch_is_ready()) { + + /* The real MCH initialisation. */ + timestamp_add_now(TS_BEFORE_INITRAM); + + sdram_set_registers(memctrl); + sdram_set_spd_registers(memctrl); + sdram_enable(memctrl); + + /* Hook for post ECC scrub settings and debug. */ + sdram_post_ecc(memctrl); + + timestamp_add_now(TS_AFTER_INITRAM); + } + + printk(BIOS_DEBUG, "SDRAM is up.\n"); +} diff --git a/src/northbridge/intel/e7505/raminit.h b/src/northbridge/intel/e7505/raminit.h index c65675f..9aa6eb4 100644 --- a/src/northbridge/intel/e7505/raminit.h +++ b/src/northbridge/intel/e7505/raminit.h @@ -30,8 +30,6 @@ uint16_t channel1[MAX_DIMM_SOCKETS_PER_CHANNEL]; };
-void e7505_mch_init(const struct mem_controller *memctrl); -void e7505_mch_done(const struct mem_controller *memctrl); -int e7505_mch_is_ready(void); +void sdram_initialize(void);
#endif /* RAMINIT_H */ diff --git a/src/northbridge/intel/e7505/romstage.c b/src/northbridge/intel/e7505/romstage.c index 6c74c1f..f506bf4 100644 --- a/src/northbridge/intel/e7505/romstage.c +++ b/src/northbridge/intel/e7505/romstage.c @@ -13,9 +13,7 @@ * GNU General Public License for more details. */
-#include <stdint.h> #include <cbmem.h> -#include <console/console.h> #include <arch/romstage.h>
#include <southbridge/intel/i82801dx/i82801dx.h> @@ -23,27 +21,12 @@
void mainboard_romstage_entry(void) { - static const struct mem_controller memctrl[] = { - { - .d0 = PCI_DEV(0, 0, 0), - .d0f1 = PCI_DEV(0, 0, 1), - .channel0 = { 0x50, 0x52, 0, 0 }, - .channel1 = { 0x51, 0x53, 0, 0 }, - }, - }; + /* Perform some early chipset initialization required + * before RAM initialization can work + */ + i82801dx_early_init();
- /* If this is a warm boot, some initialization can be skipped */ - if (!e7505_mch_is_ready()) { - enable_smbus(); - - /* The real MCH initialisation. */ - e7505_mch_init(memctrl); - - /* Hook for post ECC scrub settings and debug. */ - e7505_mch_done(memctrl); - } - - printk(BIOS_DEBUG, "SDRAM is up.\n"); + sdram_initialize();
cbmem_recovery(0); } diff --git a/src/southbridge/intel/i82801dx/early_smbus.c b/src/southbridge/intel/i82801dx/early_smbus.c index 77b0aa0..de0bc93 100644 --- a/src/southbridge/intel/i82801dx/early_smbus.c +++ b/src/southbridge/intel/i82801dx/early_smbus.c @@ -21,6 +21,11 @@
#include "i82801dx.h"
+void i82801dx_early_init(void) +{ + enable_smbus(); +} + void enable_smbus(void) { pci_devfn_t dev = PCI_DEV(0x0, 0x1f, 0x3); diff --git a/src/southbridge/intel/i82801dx/i82801dx.h b/src/southbridge/intel/i82801dx/i82801dx.h index 50122d8..8717e59 100644 --- a/src/southbridge/intel/i82801dx/i82801dx.h +++ b/src/southbridge/intel/i82801dx/i82801dx.h @@ -35,6 +35,7 @@ #include "chip.h"
void i82801dx_enable(struct device *dev); +void i82801dx_early_init(void); void enable_smbus(void); int smbus_read_byte(unsigned int device, unsigned int address); void aseg_smm_lock(void);
-
Angel Pons (Code Review) -
Kyösti Mälkki (Code Review)