HAOUAS Elyes has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
nb/intel/i945: Make some cosmetic changes
This will make i945GC and i945GM splitting easier.
Change-Id: I3acc1f526056248f8fbb1778a3c381d369faf020 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 26 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/39859/1
diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index 134d11f..7cf3e8c 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -74,7 +74,7 @@
PRINTK_DEBUG(" Sending RAM command 0x%08x", reg32);
- MCHBAR32(DCC) = reg32; /* This is the actual magic */ + MCHBAR32(DCC) = reg32; /* This is the actual magic */
PRINTK_DEBUG("...done\n");
@@ -447,7 +447,7 @@ sysinfo->package = SYSINFO_PACKAGE_STACKED;
if (!dimm_info.flags.bl8) - die("Only DDR-II RAM with burst length 8 is supported by this chipset.\n"); + die("Only DDR-II RAM with burst length 8 is not supported.\n");
if (dimm_info.ranksize_mb < 128) die("DDR-II rank size smaller than 128MB is not supported.\n"); @@ -495,7 +495,7 @@ die("No memory installed.\n");
if (!(dimm_mask & ((1 << DIMM_SOCKETS) - 1))) - /* Possibly does not boot in this case */ + /* FIXME: Possibly does not boot in this case */ printk(BIOS_INFO, "Channel 0 has no memory populated.\n"); }
@@ -809,9 +809,9 @@ const u8 *slew_group; /* Dual Channel needs different tables. */ if (dual_channel) - slew_group = dual_channel_slew_group_lookup; + slew_group = dual_channel_slew_group_lookup; else - slew_group = single_channel_slew_group_lookup; + slew_group = single_channel_slew_group_lookup;
switch (slew_group[index]) { case DQ2030: return dq2030; @@ -950,7 +950,7 @@ printk(BIOS_DEBUG, "Programming Dual Channel RCOMP\n"); strength_multiplier = dual_channel_strength_multiplier; dual_channel = 1; - idx = 5 * sysinfo->dimm[0] + sysinfo->dimm[2]; + idx = 5 * sysinfo->dimm[0] + sysinfo->dimm[2]; } else { printk(BIOS_DEBUG, "Programming Single Channel RCOMP\n"); strength_multiplier = single_channel_strength_multiplier; @@ -1202,7 +1202,7 @@ if (sysinfo->interleaved) tolud = (cum0 + cum1) << 1; else - tolud = (cum1 ? cum1 : cum0) << 1; + tolud = (cum1 ? cum1 : cum0) << 1;
/* The TOM register has a different format */ tom = tolud >> 3; @@ -1342,7 +1342,7 @@ reg32 |= (1 << 11); MCHBAR32(C0DRC1) = reg32;
- /* Do we have to do this if we're in Single Channel Mode? */ + /* Do we have to do this if we're in Single Channel Mode? */ reg32 = MCHBAR32(C1DRC1);
for (i = 4; i < 8; i++) { @@ -1444,12 +1444,16 @@ */ tRD_min = sysinfo->cas; switch (sysinfo->fsb_frequency) { - case 533: break; - case 667: tRD_min += 1; + case 533: break; - case 800: tRD_min += 2; + case 667: + tRD_min += 1; break; - case 1066: tRD_min += 3; + case 800: + tRD_min += 2; + break; + case 1066: + tRD_min += 3; break; }
@@ -1557,10 +1561,10 @@ printk(BIOS_DEBUG, "Setting mode of operation for memory channels...");
if (sdram_capabilities_interleave() && - ((sysinfo->banksize[0] + sysinfo->banksize[1] + - sysinfo->banksize[2] + sysinfo->banksize[3]) == - (sysinfo->banksize[4] + sysinfo->banksize[5] + - sysinfo->banksize[6] + sysinfo->banksize[7]))) { + ((sysinfo->banksize[0] + sysinfo->banksize[1] + + sysinfo->banksize[2] + sysinfo->banksize[3]) == + (sysinfo->banksize[4] + sysinfo->banksize[5] + + sysinfo->banksize[6] + sysinfo->banksize[7]))) { /* Both channels equipped with DIMMs of the same size */ sysinfo->interleaved = 1; } else { @@ -1624,9 +1628,9 @@
static void sdram_program_graphics_frequency(struct sys_info *sysinfo) { - u8 reg8; + u8 reg8; u16 reg16; - u8 freq, second_vco, voltage; + u8 freq, second_vco, voltage;
#define CRCLK_166MHz 0x00 #define CRCLK_200MHz 0x01 @@ -1699,7 +1703,7 @@
if (voltage == VOLTAGE_1_50) { second_vco = 1; - } else if ((i945_silicon_revision() > 0) && (freq == CRCLK_250MHz)) { + } else if ((i945_silicon_revision() > 0) && (freq == CRCLK_250MHz)) { u16 mem = sysinfo->memory_frequency; u16 fsb = sysinfo->fsb_frequency;
@@ -2507,7 +2511,7 @@ }
#define RTT_ODT_NONE 0 -#define RTT_ODT_50_OHM ((1 << 9) | (1 << 5)) +#define RTT_ODT_50_OHM ((1 << 9) | (1 << 5)) #define RTT_ODT_75_OHM (1 << 5) #define RTT_ODT_150_OHM (1 << 9)
@@ -2541,7 +2545,8 @@ if (sysinfo->interleaved && nonzero < 4 && i >= 4) { bankaddr = 0x40; } else { - printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero); + printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", + nonzero); bankaddr += sysinfo->banksize[nonzero] << (sysinfo->interleaved ? 26 : 25); }
Hello Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39859
to look at the new patch set (#2).
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
nb/intel/i945: Make some cosmetic changes
This will make i945GC and i945GM splitting easier.
Change-Id: I3acc1f526056248f8fbb1778a3c381d369faf020 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 26 insertions(+), 21 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/39859/2
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 2: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2544: printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero); Isn’t this exactly 96 characters?
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2557: */ While you doing cosmetic changes, fix this too?
/* … … */
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2544: printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero);
Isn’t this exactly 96 characters?
101 :)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2557: */
While you doing cosmetic changes, fix this too? […]
Ack
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39859
to look at the new patch set (#4).
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
nb/intel/i945: Make some cosmetic changes
This will make i945GC and i945GM splitting easier.
Change-Id: I3acc1f526056248f8fbb1778a3c381d369faf020 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 27 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/39859/4
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 4: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2557: */
Ack
Um, what Paul meant is this:
/* We have a bank with a non-zero size... Remember it for the next offset we have to calculate */
If that looks bad, you can also use this:
/* * We have a bank with a non-zero size... * Remember it for the next offset we have to calculate */
In any case, please use three points for the ellipsis [1]. Also, since both sentences don't fit in the same line, you can put each sentence on a different line so that they are not broken.
[1]: https://en.wikipedia.org/wiki/Ellipsis
Hello build bot (Jenkins), Angel Pons, Patrick Rudolph,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/39859
to look at the new patch set (#5).
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
nb/intel/i945: Make some cosmetic changes
This will make i945GC and i945GM splitting easier.
Change-Id: I3acc1f526056248f8fbb1778a3c381d369faf020 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr --- M src/northbridge/intel/i945/raminit.c 1 file changed, 29 insertions(+), 23 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/59/39859/5
Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 5: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2557: */
Um, what Paul meant is this: […]
Done
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2544: printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero);
101 :)
Hmm, then `echo "…" | wc -c` lied to me.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
nb/intel/i945: Make some cosmetic changes
This will make i945GC and i945GM splitting easier.
Change-Id: I3acc1f526056248f8fbb1778a3c381d369faf020 Signed-off-by: Elyes HAOUAS ehaouas@noos.fr Reviewed-on: https://review.coreboot.org/c/coreboot/+/39859 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Angel Pons th3fanbus@gmail.com --- M src/northbridge/intel/i945/raminit.c 1 file changed, 29 insertions(+), 23 deletions(-)
Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved
diff --git a/src/northbridge/intel/i945/raminit.c b/src/northbridge/intel/i945/raminit.c index 134d11f..51ee65d 100644 --- a/src/northbridge/intel/i945/raminit.c +++ b/src/northbridge/intel/i945/raminit.c @@ -74,7 +74,7 @@
PRINTK_DEBUG(" Sending RAM command 0x%08x", reg32);
- MCHBAR32(DCC) = reg32; /* This is the actual magic */ + MCHBAR32(DCC) = reg32; /* This is the actual magic */
PRINTK_DEBUG("...done\n");
@@ -205,7 +205,7 @@ return (!reg8); }
-// TODO check if we ever need this function +/* TODO check if we ever need this function */ #if 0 static int sdram_capabilities_MEM4G_disable(void) { @@ -447,7 +447,7 @@ sysinfo->package = SYSINFO_PACKAGE_STACKED;
if (!dimm_info.flags.bl8) - die("Only DDR-II RAM with burst length 8 is supported by this chipset.\n"); + die("Only DDR-II RAM with burst length 8 is supported.\n");
if (dimm_info.ranksize_mb < 128) die("DDR-II rank size smaller than 128MB is not supported.\n"); @@ -495,7 +495,7 @@ die("No memory installed.\n");
if (!(dimm_mask & ((1 << DIMM_SOCKETS) - 1))) - /* Possibly does not boot in this case */ + /* FIXME: Possibly does not boot in this case */ printk(BIOS_INFO, "Channel 0 has no memory populated.\n"); }
@@ -809,9 +809,9 @@ const u8 *slew_group; /* Dual Channel needs different tables. */ if (dual_channel) - slew_group = dual_channel_slew_group_lookup; + slew_group = dual_channel_slew_group_lookup; else - slew_group = single_channel_slew_group_lookup; + slew_group = single_channel_slew_group_lookup;
switch (slew_group[index]) { case DQ2030: return dq2030; @@ -950,7 +950,7 @@ printk(BIOS_DEBUG, "Programming Dual Channel RCOMP\n"); strength_multiplier = dual_channel_strength_multiplier; dual_channel = 1; - idx = 5 * sysinfo->dimm[0] + sysinfo->dimm[2]; + idx = 5 * sysinfo->dimm[0] + sysinfo->dimm[2]; } else { printk(BIOS_DEBUG, "Programming Single Channel RCOMP\n"); strength_multiplier = single_channel_strength_multiplier; @@ -1202,7 +1202,7 @@ if (sysinfo->interleaved) tolud = (cum0 + cum1) << 1; else - tolud = (cum1 ? cum1 : cum0) << 1; + tolud = (cum1 ? cum1 : cum0) << 1;
/* The TOM register has a different format */ tom = tolud >> 3; @@ -1342,7 +1342,7 @@ reg32 |= (1 << 11); MCHBAR32(C0DRC1) = reg32;
- /* Do we have to do this if we're in Single Channel Mode? */ + /* Do we have to do this if we're in Single Channel Mode? */ reg32 = MCHBAR32(C1DRC1);
for (i = 4; i < 8; i++) { @@ -1444,12 +1444,16 @@ */ tRD_min = sysinfo->cas; switch (sysinfo->fsb_frequency) { - case 533: break; - case 667: tRD_min += 1; + case 533: break; - case 800: tRD_min += 2; + case 667: + tRD_min += 1; break; - case 1066: tRD_min += 3; + case 800: + tRD_min += 2; + break; + case 1066: + tRD_min += 3; break; }
@@ -1557,10 +1561,10 @@ printk(BIOS_DEBUG, "Setting mode of operation for memory channels...");
if (sdram_capabilities_interleave() && - ((sysinfo->banksize[0] + sysinfo->banksize[1] + - sysinfo->banksize[2] + sysinfo->banksize[3]) == - (sysinfo->banksize[4] + sysinfo->banksize[5] + - sysinfo->banksize[6] + sysinfo->banksize[7]))) { + ((sysinfo->banksize[0] + sysinfo->banksize[1] + + sysinfo->banksize[2] + sysinfo->banksize[3]) == + (sysinfo->banksize[4] + sysinfo->banksize[5] + + sysinfo->banksize[6] + sysinfo->banksize[7]))) { /* Both channels equipped with DIMMs of the same size */ sysinfo->interleaved = 1; } else { @@ -1624,9 +1628,9 @@
static void sdram_program_graphics_frequency(struct sys_info *sysinfo) { - u8 reg8; + u8 reg8; u16 reg16; - u8 freq, second_vco, voltage; + u8 freq, second_vco, voltage;
#define CRCLK_166MHz 0x00 #define CRCLK_200MHz 0x01 @@ -1699,7 +1703,7 @@
if (voltage == VOLTAGE_1_50) { second_vco = 1; - } else if ((i945_silicon_revision() > 0) && (freq == CRCLK_250MHz)) { + } else if ((i945_silicon_revision() > 0) && (freq == CRCLK_250MHz)) { u16 mem = sysinfo->memory_frequency; u16 fsb = sysinfo->fsb_frequency;
@@ -2507,7 +2511,7 @@ }
#define RTT_ODT_NONE 0 -#define RTT_ODT_50_OHM ((1 << 9) | (1 << 5)) +#define RTT_ODT_50_OHM ((1 << 9) | (1 << 5)) #define RTT_ODT_75_OHM (1 << 5) #define RTT_ODT_150_OHM (1 << 9)
@@ -2541,13 +2545,15 @@ if (sysinfo->interleaved && nonzero < 4 && i >= 4) { bankaddr = 0x40; } else { - printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero); + printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", + nonzero); bankaddr += sysinfo->banksize[nonzero] << (sysinfo->interleaved ? 26 : 25); } }
- /* We have a bank with a non-zero size.. Remember it + /* + * We have a bank with a non-zero size... Remember it * for the next offset we have to calculate */ nonzero = i;
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/39859 )
Change subject: nb/intel/i945: Make some cosmetic changes ......................................................................
Patch Set 6:
(1 comment)
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... File src/northbridge/intel/i945/raminit.c:
https://review.coreboot.org/c/coreboot/+/39859/3/src/northbridge/intel/i945/... PS3, Line 2544: printk(BIOS_DEBUG, "bankaddr from bank size of rank %d\n", nonzero);
Hmm, then `echo "…" | wc -c` lied to me.
no, just add the tabs, you will have 101
-
Angel Pons (Code Review) -
HAOUAS Elyes (Code Review)
-
Patrick Georgi (Code Review)
-
Paul Menzel (Code Review)