[LinuxBIOS] SPD Logical and Physical Banks
Uwe Hermann
uwe at hermann-uwe.de
Thu May 17 14:39:43 CEST 2007
Hi Corey,
here are a few more comments on the file/patch:
On Tue, May 15, 2007 at 04:24:26PM -0400, Corey Osgood wrote:
> Index: raminit.c
> ===================================================================
> --- raminit.c (Revision 2671)
> +++ raminit.c (Arbeitskopie)
> @@ -68,75 +69,19 @@
>
> /* Table format: register, bitmask, value. */
> static const long register_values[] = {
> - /* NBXCFG - NBX Configuration Register
> - * 0x50
> - *
> - * [31:24] SDRAM Row Without ECC
> - * 0 = ECC components are populated in this row
> - * 1 = ECC components are not populated in this row
[...]
> - * [01:00] Reserved
> - */
> - // TODO
> - NBXCFG, 0x00000000, 0xff00a00c,
Hm, please keep these values and comments here for now. They might not
be needed for every register, but I think I remember that _some_ should
be set to "sane" defaults even _during_ RAM init. So it might make sense to
set them twice (here, and later in the code).
> * [5:5] Module Mode Configuration (MMCONFIG)
> - * TODO
> + * Set by external strapping options
> + * SDRAMPWR MMCONFIG CKE Operation
> + * 0 0 3 DIMM, CKE[5:0] driven, self-refresh entry
> + * staggered. SDRAM dynamic power down available.
> + * X 1 3 DIMM, CKE0 only, self-refresh entry not
> + * staggered. SDRAM dynamic power down unavailable.
> + * 1 0 4 DIMM, GCKE only, self-refresh entry staggered.
> + * SDRAM dynamic power down unavailable.
Ack, thanks.
> @@ -189,30 +134,6 @@
> PAM5, 0x00000000, 0x00,
> PAM6, 0x00000000, 0x00,
>
> - /* DRB[0:7] - DRAM Row Boundary Registers
> - * 0x60 - 0x67
> - *
> - * An array of 8 byte registers, which hold the ending memory address
> - * assigned to each pair of DIMMs, in 8MB granularity.
> - *
> - * 0x60 DRB0 = Total memory in row0 (in 8 MB)
> - * 0x61 DRB1 = Total memory in row0+1 (in 8 MB)
> - * 0x62 DRB2 = Total memory in row0+1+2 (in 8 MB)
> - * 0x63 DRB3 = Total memory in row0+1+2+3 (in 8 MB)
> - * 0x64 DRB4 = Total memory in row0+1+2+3+4 (in 8 MB)
> - * 0x65 DRB5 = Total memory in row0+1+2+3+4+5 (in 8 MB)
> - * 0x66 DRB6 = Total memory in row0+1+2+3+4+5+6 (in 8 MB)
> - * 0x67 DRB7 = Total memory in row0+1+2+3+4+5+6+7 (in 8 MB)
> - */
> - // TODO
> - DRB0, 0x00, 0x00,
> - DRB1, 0x00, 0x00,
> - DRB2, 0x00, 0x00,
> - DRB3, 0x00, 0x00,
> - DRB4, 0x00, 0x00,
> - DRB5, 0x00, 0x00,
> - DRB6, 0x00, 0x00,
> - DRB7, 0x00, 0x00,
See above, please keep this.
> @@ -390,7 +311,7 @@
> reg = pci_read_config8(ctrl->d0, DRAMC);
>
> for (i = 0; i < DIMM_SOCKETS; i++) {
> - value = spd_read_byte(ctrl->channel0[i], SPD_REFRESH);
> + value = smbus_read_byte(ctrl->channel0[i], SPD_REFRESH);
Nope, please use spd_read_byte() everywhere.
> +static void spd_set_nbxcfg(const struct mem_controller *ctrl)
Please don't hardcode register names in function names. Use generic
function names which explain the step/work the function performs, e.g.
set_cas_latencies()
enable_timers()
set_refresh_rate()
etc.
The register might or might not have the same name in a similar chipset,
but the steps you have to perform are usually the same.
Also, generic names make the code much better readable and
understandable for new developers who try to understand it.
> + * [31:24] SDRAM Row Without ECC
> + * 0 = ECC components are populated in this row
> + * 1 = ECC components are not populated in this row */
> +
> +
> + /* Note: there's no way to tell 440zx from bx via hardware */
> + /* 440ZX doesn't do ECC, and needs bits 31-24 set to 0, so skip this */
> + #ifndef MODEL_440ZX
I'd even say you shouldn't worry about ECC all that much right now.
Just set it to off until everything else works fine. ECC is nice to
have, but non-essential.
> + for(i = 0; i < 4; i++)
4 -> DIMM_SOCKETS
Please use readable #defines instead of "magic values" whereever
possible.
> + {
> + /* Check if this dimm supports ECC */
> + spd_data = smbus_read_byte(ctrl->channel0[i], 11);
11 -> SPD_DIMM_CONFIG_TYPE
(same for all other SPD constants)
> + /* If it does *not*, or there's no dimm present, set bits for both
> + sides to disable ECC, otherwise leave them enabled */
> + if(spd_data != 2)
Even this '2' should be a #define in spd.h, maybe.
> + /* Set the DRAM frequency to the lowest of all dimms */
> + /* TODO: do this with the previous loop */
> + for(i = 0; i < 4; i++)
> + {
> + spd_data = smbus_read_byte(ctrl->channel0[i], 126);
> + /* If we find any 66MHz dimms, set the dimm speed to 66MHz and break */
> + /* Note: This depends on the dimm following the Intel SPD standard */
> + if(spd_data == 0x66)
> + {
> + nbxcfg |= 1 << 13;
> + i = 6; //just to make sure
Huh?
> + * [04:04] Reserved
> + * [03:03] USWC Write Post During I/O Bridge Access Enable (UWPIO)
> + * 1 = Enable
> + * 0 = Disable
> + * [02:02] In-Order Queue Depth (IOQD)
> + * 1 = In-order queue = maximum
> + * 0 = A7# is sampled asserted (i.e., 0)
> + * [01:00] Reserved
> + */
> + nbxcfg |= 0xc; //sets bits 3 and 2 to 1
> + PRINT_DEBUG("Value of NBXCFG calculated to 0x");
> + PRINT_DEBUG_HEX32(nbxcfg);
> + PRINT_DEBUG("/r/n");
\r\n
> + /* Count to 8 since there are 8 possible rows, but count by 2 to check by dimm */
> + /* Should 8 be replaced with DIMM_SOCKETS * 2? */
Yes, probably.
> + for(i = 0; i < 8; i = i + 2)
> + {
> + /* First check if this row is populated with SDR SDRAM */
> + if(smbus_read_byte(ctrl->channel0[i/2], 2) == 0x04)
This is very confusing. I think it would be easier to understand if you
count from 0..15 in steps of 1 and adapt the number where needed.
> + dimm_size = rows * row_size * 4;
> + print_debug("Found 0x");
> + print_debug_hex8(dimm_size);
> + print_debug("MB DIMM in slot ");
> + print_debug_hex8(i/2);
> + print_debug("\r\n");
> +
> + /* Convert dimms_size to be in 8MB chunks */
> + dimm_size = dimm_size >> 3;
dimm_size /= 8;
Please don't use bit-manipulations where you really do arithmetic
operations (just makes the code harder to understand).
(don't worry about "optimizations" here, that's the job of the compiler)
In general the patch looks quite good. Please feel free to send single
patches for functions which are finished and usable, so we can merge
them one by one (and always check if the RAM still verifies).
No need to wait until all of the functions are finished...
Uwe.
--
http://www.hermann-uwe.de | http://www.holsham-traders.de
http://www.crazy-hacks.org | http://www.unmaintained-free-software.org
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 189 bytes
Desc: Digital signature
URL: <http://www.coreboot.org/pipermail/coreboot/attachments/20070517/2d19460e/attachment.sig>
More information about the coreboot
mailing list