Re: [coreboot] r636 - in coreboot-v3: include mainboard/artecgroup/dbe61 mainboard/artecgroup/dbe62 mainboard/pcengines/alix1c northbridge/amd/geodelx southbridge/amd/cs5536
On Fri, Mar 07, 2008 at 12:33:59AM +0100, svn@coreboot.org wrote:
Modified: coreboot-v3/mainboard/artecgroup/dbe61/initram.c
--- coreboot-v3/mainboard/artecgroup/dbe61/initram.c 2008-03-06 23:18:13 UTC (rev 635) +++ coreboot-v3/mainboard/artecgroup/dbe61/initram.c 2008-03-06 23:33:59 UTC (rev 636) @@ -97,13 +97,17 @@ /* returns 0xFF on any failures */ u8 ret = 0xff;
- printk(BIOS_DEBUG, "spd_read_byte dev %04x\n", device);
- printk(BIOS_DEBUG, "spd_read_byte dev %04x", device); if (device == DIMM0) { for (i = 0; i < ARRAY_SIZE(spd_table); i++) { if (spd_table[i].address == address) { ret = spd_table[i].data;
No break here?
} }
if (i == ARRAY_SIZE(spd_table))printk(BIOS_DEBUG, " addr %02x does not exist in SPD table",address);}
printk(BIOS_DEBUG, " addr %02x returns %02x\n", address, ret);
Modified: coreboot-v3/northbridge/amd/geodelx/raminit.c
--- coreboot-v3/northbridge/amd/geodelx/raminit.c 2008-03-06 23:18:13 UTC (rev 635) +++ coreboot-v3/northbridge/amd/geodelx/raminit.c 2008-03-06 23:33:59 UTC (rev 636) @@ -130,6 +130,7 @@ /* Module Density * Module Banks */ /* Shift to multiply by the number of DIMM banks. */ dimm_size <<= (dimm_setting >> CF07_UPPER_D0_MB_SHIFT) & 1;
- printk(BIOS_DEBUG, "DIMM size is %x\n", dimm_size); banner(BIOS_DEBUG, "BEFORT CTZ"); dimm_size = __builtin_ctz(dimm_size); banner(BIOS_DEBUG, "TEST DIMM SIZE>8");
@@ -183,6 +184,7 @@ banner(BIOS_DEBUG, "RDMSR CF07"); msr = rdmsr(MC_CF07_DATA); banner(BIOS_DEBUG, "WRMSR CF07");
- printk(BIOS_DEBUG, "CF07(%x): %08x.%08x\n", MC_CF07_DATA, msr.hi, msr.lo); if (dimm == dimm0) { msr.hi &= 0xFFFF0000; msr.hi |= dimm_setting;
@@ -223,6 +225,7 @@ /* Turn SPD ns time into MHz. Check what the asm does to this math. */ speed = 2 * ((10000 / (((spd_byte0 >> 4) * 10) + (spd_byte0 & 0x0F))));
- printk(BIOS_DEBUG, "ddr max speed is %d\n", speed);
ddr -> DDR
/* Current speed > max speed? */ if (geode_link_speed() > speed) { printk(BIOS_EMERG, "DIMM overclocked. Check GeodeLink speed\n"); @@ -266,6 +269,7 @@ msr = rdmsr(MC_CF07_DATA); msr.lo |= ((rate0 * (geode_link_speed() / 2)) / 16) << CF07_LOWER_REF_INT_SHIFT;
- printk(BIOS_DEBUG, "Refresh rate set to %x\n", rate0); wrmsr(MC_CF07_DATA, msr);
}
@@ -385,6 +389,7 @@ hlt(); }
- printk(BIOS_DEBUG, "Set cas latency to %x\n", spd_byte);
cas -> CAS
Modified: coreboot-v3/southbridge/amd/cs5536/cs5536.h
--- coreboot-v3/southbridge/amd/cs5536/cs5536.h 2008-03-06 23:18:13 UTC (rev 635) +++ coreboot-v3/southbridge/amd/cs5536/cs5536.h 2008-03-06 23:33:59 UTC (rev 636) @@ -444,6 +444,7 @@ /* Function prototypes */ void cs5536_disable_internal_uart(void); void cs5536_setup_onchipuart(void); +void cs5536_setup_onchipuart2(void);
Is there some more descriptive suffix than "2" for this function?
void cs5536_stage1(void);
#endif /* SOUTHBRIDGE_AMD_CS5536_CS5536_H */
@@ -239,6 +222,40 @@ wrmsr(MDD_UART1_CONF, msr); }
+void cs5536_setup_onchipuart2(void)
Please add a (doxygen-)comment for this function, which should also mention what is different to cs5536_setup_onchipuart() and why we need two such functions.
+{
- struct msr msr;
- /* GPIO4 - UART2_TX */
- /* Set: Output Enable (0x4) */
- outl(GPIOL_4_SET, GPIO_IO_BASE + GPIOL_OUTPUT_ENABLE);
- /* Set: OUTAUX1 Select (0x10) */
- outl(GPIOL_4_SET, GPIO_IO_BASE + GPIOL_OUT_AUX1_SELECT);
- /* GPIO4 - UART2_RX */
- /* Set: Input Enable (0x20) */
- outl(GPIOL_3_SET, GPIO_IO_BASE + GPIOL_INPUT_ENABLE);
- /* Set: INAUX1 Select (0x34) */
The (0x4), (0x10), (0x20), (0x34) etc. should be dropped, that's what the defines are used for. We don't _care_ about the actual numbers here.
Uwe.
Thanks, Uwe, for the comments, I will fix these things in the next patch, and will add Carl-Daniel's fixes too (which address some of your comments)
thanks again
ron
On 07.03.2008 01:02, Uwe Hermann wrote:
On Fri, Mar 07, 2008 at 12:33:59AM +0100, svn@coreboot.org wrote:
Modified: coreboot-v3/mainboard/artecgroup/dbe61/initram.c
--- coreboot-v3/mainboard/artecgroup/dbe61/initram.c 2008-03-06 23:18:13 UTC (rev 635) +++ coreboot-v3/mainboard/artecgroup/dbe61/initram.c 2008-03-06 23:33:59 UTC (rev 636) @@ -97,13 +97,17 @@ /* returns 0xFF on any failures */ u8 ret = 0xff;
- printk(BIOS_DEBUG, "spd_read_byte dev %04x\n", device);
- printk(BIOS_DEBUG, "spd_read_byte dev %04x", device); if (device == DIMM0) { for (i = 0; i < ARRAY_SIZE(spd_table); i++) { if (spd_table[i].address == address) { ret = spd_table[i].data;
No break here?
That break is part of the fixup patch I sent in response. It will be committed in due course.
} }
if (i == ARRAY_SIZE(spd_table))printk(BIOS_DEBUG, " addr %02x does not exist in SPD table",address);}
printk(BIOS_DEBUG, " addr %02x returns %02x\n", address, ret);
Modified: coreboot-v3/northbridge/amd/geodelx/raminit.c
--- coreboot-v3/northbridge/amd/geodelx/raminit.c 2008-03-06 23:18:13 UTC (rev 635) +++ coreboot-v3/northbridge/amd/geodelx/raminit.c 2008-03-06 23:33:59 UTC (rev 636) @@ -130,6 +130,7 @@ /* Module Density * Module Banks */ /* Shift to multiply by the number of DIMM banks. */ dimm_size <<= (dimm_setting >> CF07_UPPER_D0_MB_SHIFT) & 1;
- printk(BIOS_DEBUG, "DIMM size is %x\n", dimm_size); banner(BIOS_DEBUG, "BEFORT CTZ"); dimm_size = __builtin_ctz(dimm_size); banner(BIOS_DEBUG, "TEST DIMM SIZE>8");
@@ -183,6 +184,7 @@ banner(BIOS_DEBUG, "RDMSR CF07"); msr = rdmsr(MC_CF07_DATA); banner(BIOS_DEBUG, "WRMSR CF07");
- printk(BIOS_DEBUG, "CF07(%x): %08x.%08x\n", MC_CF07_DATA, msr.hi, msr.lo); if (dimm == dimm0) { msr.hi &= 0xFFFF0000; msr.hi |= dimm_setting;
@@ -223,6 +225,7 @@ /* Turn SPD ns time into MHz. Check what the asm does to this math. */ speed = 2 * ((10000 / (((spd_byte0 >> 4) * 10) + (spd_byte0 & 0x0F))));
- printk(BIOS_DEBUG, "ddr max speed is %d\n", speed);
ddr -> DDR
Ah, cosmetics. I almost never review patches for comment/message style. For me, code style and readability is all that matters.
/* Current speed > max speed? */ if (geode_link_speed() > speed) { printk(BIOS_EMERG, "DIMM overclocked. Check GeodeLink speed\n"); @@ -266,6 +269,7 @@ msr = rdmsr(MC_CF07_DATA); msr.lo |= ((rate0 * (geode_link_speed() / 2)) / 16) << CF07_LOWER_REF_INT_SHIFT;
- printk(BIOS_DEBUG, "Refresh rate set to %x\n", rate0); wrmsr(MC_CF07_DATA, msr);
}
@@ -385,6 +389,7 @@ hlt(); }
- printk(BIOS_DEBUG, "Set cas latency to %x\n", spd_byte);
cas -> CAS
Modified: coreboot-v3/southbridge/amd/cs5536/cs5536.h
--- coreboot-v3/southbridge/amd/cs5536/cs5536.h 2008-03-06 23:18:13 UTC (rev 635) +++ coreboot-v3/southbridge/amd/cs5536/cs5536.h 2008-03-06 23:33:59 UTC (rev 636) @@ -444,6 +444,7 @@ /* Function prototypes */ void cs5536_disable_internal_uart(void); void cs5536_setup_onchipuart(void); +void cs5536_setup_onchipuart2(void);
Is there some more descriptive suffix than "2" for this function?
See my fixup patch.
void cs5536_stage1(void);
#endif /* SOUTHBRIDGE_AMD_CS5536_CS5536_H */
@@ -239,6 +222,40 @@ wrmsr(MDD_UART1_CONF, msr); }
+void cs5536_setup_onchipuart2(void)
Please add a (doxygen-)comment for this function, which should also mention what is different to cs5536_setup_onchipuart() and why we need two such functions.
Ditto (though not completely addressed by my patch).
+{
- struct msr msr;
- /* GPIO4 - UART2_TX */
- /* Set: Output Enable (0x4) */
- outl(GPIOL_4_SET, GPIO_IO_BASE + GPIOL_OUTPUT_ENABLE);
- /* Set: OUTAUX1 Select (0x10) */
- outl(GPIOL_4_SET, GPIO_IO_BASE + GPIOL_OUT_AUX1_SELECT);
- /* GPIO4 - UART2_RX */
- /* Set: Input Enable (0x20) */
- outl(GPIOL_3_SET, GPIO_IO_BASE + GPIOL_INPUT_ENABLE);
- /* Set: INAUX1 Select (0x34) */
The (0x4), (0x10), (0x20), (0x34) etc. should be dropped, that's what the defines are used for. We don't _care_ about the actual numbers here.
Feel free to send a patch. My personal opinion is that the comments make datasheet reading easier when they include the value of the symbolic constants, but I won't force that view on you.
Regards, Carl-Daniel
-
Carl-Daniel Hailfinger -
ron minnich -
Uwe Hermann