Angel Pons has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/47569 )
Change subject: nb/intel/sandybridge: Clean up `dram_mr2` function ......................................................................
nb/intel/sandybridge: Clean up `dram_mr2` function
Constify variables, and also remove pointless and-masks on mr2reg.
Change-Id: I3829012ff7d41f4308ee84d6fbf3b1f2803431af Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 9 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/47569/1
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index d166292..0f1b2d8 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -767,22 +767,19 @@
static void dram_mr2(ramctr_timing *ctrl, u8 rank, int channel) { - u16 pasr, cwl, mr2reg; - odtmap odt; + const u16 pasr = 0; + const u16 cwl = ctrl->CWL - 5; + const odtmap odt = get_ODT(ctrl, channel); + int srt = 0; - - pasr = 0; - cwl = ctrl->CWL - 5; - odt = get_ODT(ctrl, channel); - if (IS_IVY_CPU(ctrl->cpu) && ctrl->tCK >= TCK_1066MHZ) srt = ctrl->extended_temperature_range && !ctrl->auto_self_refresh;
- mr2reg = 0; - mr2reg = (mr2reg & ~0x07) | pasr; - mr2reg = (mr2reg & ~0x38) | (cwl << 3); - mr2reg = (mr2reg & ~0x40) | (ctrl->auto_self_refresh << 6); - mr2reg = (mr2reg & ~0x80) | (srt << 7); + u16 mr2reg = 0; + mr2reg |= pasr; + mr2reg |= cwl << 3; + mr2reg |= ctrl->auto_self_refresh << 6; + mr2reg |= srt << 7; mr2reg |= (odt.rttwr / 60) << 9;
write_mrreg(ctrl, channel, rank, 2, mr2reg);
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47569 )
Change subject: nb/intel/sandybridge: Clean up `dram_mr2` function ......................................................................
Patch Set 2: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/47569/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47569/2/src/northbridge/intel/sandy... PS2, Line 770: const u16 pasr = 0; Is that variable needed at all? It only seems to be used once. (I have no idea what *pasr* is supposed to mean.)
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47569 )
Change subject: nb/intel/sandybridge: Clean up `dram_mr2` function ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/47569/2/src/northbridge/intel/sandy... File src/northbridge/intel/sandybridge/raminit_common.c:
https://review.coreboot.org/c/coreboot/+/47569/2/src/northbridge/intel/sandy... PS2, Line 770: const u16 pasr = 0;
Is that variable needed at all? It only seems to be used once. […]
It's Partial Array Self-Refresh. It corresponds to the three low bits of MR2. I can drop it once I switch to common MR (Mode Register) helpers
Hello build bot (Jenkins), Paul Menzel, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/47569
to look at the new patch set (#3).
Change subject: nb/intel/sandybridge: Clean up `dram_mr2` function ......................................................................
nb/intel/sandybridge: Clean up `dram_mr2` function
Constify variables, and also remove pointless and-masks on mr2reg.
Tested on Asus P8H61-M PRO, still boots.
Change-Id: I3829012ff7d41f4308ee84d6fbf3b1f2803431af Signed-off-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 9 insertions(+), 12 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/69/47569/3
Arthur Heymans has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/47569 )
Change subject: nb/intel/sandybridge: Clean up `dram_mr2` function ......................................................................
Patch Set 3: Code-Review+2
Angel Pons has submitted this change. ( https://review.coreboot.org/c/coreboot/+/47569 )
Change subject: nb/intel/sandybridge: Clean up `dram_mr2` function ......................................................................
nb/intel/sandybridge: Clean up `dram_mr2` function
Constify variables, and also remove pointless and-masks on mr2reg.
Tested on Asus P8H61-M PRO, still boots.
Change-Id: I3829012ff7d41f4308ee84d6fbf3b1f2803431af Signed-off-by: Angel Pons th3fanbus@gmail.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/47569 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Arthur Heymans arthur@aheymans.xyz --- M src/northbridge/intel/sandybridge/raminit_common.c 1 file changed, 9 insertions(+), 12 deletions(-)
Approvals: build bot (Jenkins): Verified Arthur Heymans: Looks good to me, approved
diff --git a/src/northbridge/intel/sandybridge/raminit_common.c b/src/northbridge/intel/sandybridge/raminit_common.c index 453222e..3ed1a3a 100644 --- a/src/northbridge/intel/sandybridge/raminit_common.c +++ b/src/northbridge/intel/sandybridge/raminit_common.c @@ -757,22 +757,19 @@
static void dram_mr2(ramctr_timing *ctrl, u8 rank, int channel) { - u16 pasr, cwl, mr2reg; - odtmap odt; + const u16 pasr = 0; + const u16 cwl = ctrl->CWL - 5; + const odtmap odt = get_ODT(ctrl, channel); + int srt = 0; - - pasr = 0; - cwl = ctrl->CWL - 5; - odt = get_ODT(ctrl, channel); - if (IS_IVY_CPU(ctrl->cpu) && ctrl->tCK >= TCK_1066MHZ) srt = ctrl->extended_temperature_range && !ctrl->auto_self_refresh;
- mr2reg = 0; - mr2reg = (mr2reg & ~0x07) | pasr; - mr2reg = (mr2reg & ~0x38) | (cwl << 3); - mr2reg = (mr2reg & ~0x40) | (ctrl->auto_self_refresh << 6); - mr2reg = (mr2reg & ~0x80) | (srt << 7); + u16 mr2reg = 0; + mr2reg |= pasr; + mr2reg |= cwl << 3; + mr2reg |= ctrl->auto_self_refresh << 6; + mr2reg |= srt << 7; mr2reg |= (odt.rttwr / 60) << 9;
write_mrreg(ctrl, channel, rank, 2, mr2reg);
-
Angel Pons (Code Review) -
Arthur Heymans (Code Review)
-
Paul Menzel (Code Review)