Patrick Rudolph has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/36650 )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
soc/intel/skylake: Improve comment for DISB
Either documentation or FSP is wrong, as the behaviour of DISB is actually reversed. Document that and warn about clearing the bit.
Change-Id: I9ca748ddb245cae632e423a417c6a25d52a15354 Signed-off-by: Patrick Rudolph patrick.rudolph@9elements.com --- M src/soc/intel/skylake/pmc.c 1 file changed, 10 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/50/36650/1
diff --git a/src/soc/intel/skylake/pmc.c b/src/soc/intel/skylake/pmc.c index ab9297f..3bc5c68 100644 --- a/src/soc/intel/skylake/pmc.c +++ b/src/soc/intel/skylake/pmc.c @@ -31,7 +31,16 @@
void pmc_set_disb(void) { - /* Set the DISB after DRAM init */ + /* + * Set the DISB after DRAM init. + * + * According to Document Number: 332691-002EN + * "Intel 100 Series and Intel C230 Series Chipset Family Platform + * Controller Hub (PCH) Vol2" this bit must be CLEARED after DRAM init. + * + * Tests showed that either documentation or FSP is wrong. Clearing this + * bit here causes full memory training on every boot. + */ u32 disb_val; #if defined(__SIMPLE_DEVICE__) pci_devfn_t dev = PCH_DEV_PMC;
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36650 )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
Patch Set 1:
(2 comments)
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c... PS1, Line 35: * Set the DISB after DRAM init. Dot at the end not needed.
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c... PS1, Line 43: */ Maybe add it as the function description, so above `pmc_set_disb()`?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36650 )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c... PS1, Line 43: */
Maybe add it as the function description, so above `pmc_set_disb()`?
maybe move to the header instead?
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36650 )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c... PS1, Line 39: this bit must be CLEARED This function is called _after_ DRAM init and it _sets_ the bit. So it is not doing what this comment says.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36650 )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c... PS1, Line 39: this bit must be CLEARED
This function is called _after_ DRAM init and it _sets_ the bit. So it is […]
I've added printk()s before/after the write below:
disb_val after FSP-M: 0xe0040200 disb_val after pmc_set_disb(): 0xe0840200
Pretty much what I would have expected: FSP-M ends with the bit cleared as the datasheet suggests. So I believe, neither the documentation nor FSP is wrong. Coreboot is. We should make sure, though, by checking the FSP code that interprets this bit.
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/36650 )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c File src/soc/intel/skylake/pmc.c:
https://review.coreboot.org/c/coreboot/+/36650/1/src/soc/intel/skylake/pmc.c... PS1, Line 39: this bit must be CLEARED
I've added printk()s before/after the write below: […]
Looks like FSP uses this bit for for its own consumption. It seems to be a scratchpad, so we don't have any need to change it in coreboot (unless we want to change things before FSP runs, which seems unlikely)
Stefan Reinauer has abandoned this change. ( https://review.coreboot.org/c/coreboot/+/36650?usp=email )
Change subject: soc/intel/skylake: Improve comment for DISB ......................................................................
Abandoned
-
Angel Pons (Code Review) -
Michael Niewöhner (Code Review)
-
Nico Huber (Code Review)
-
Patrick Rudolph (Code Review)
-
Paul Menzel (Code Review)
-
Stefan Reinauer (Code Review)