[LinuxBIOS] RAM detection for Intel 430FX
Uwe Hermann
uwe at hermann-uwe.de
Fri Aug 10 15:58:59 CEST 2007
Hi,
On Fri, Aug 10, 2007 at 12:09:36AM +0200, popkonserve wrote:
> the routine does the following:
> 1. disable all caches
> 2. set ram size to 0
> 3. take one row and set its ram size to the maximum value and its type to
> edo
> 4. write some data in the row
> 5. enable edo detection
> 6. read the data back
> 7. if the data read is the data written it is edo
> 8. if the data read is NOT the data written, write some more data
> 9. read the data back (without edo detection)
> 10. if the data read is the data written it is fpm
> 11. if the data read is NOT the data written, the row is empty
> 12. detect the real size of the row and the dram technology (i'll write
> some more about that tomorrow, sorry, it's late already :))
> 13. clear all settings for the current row
> 14. restart at 3. as long as not all rows have been processed
> 15. write the size and type (edo/fpm) of all rows
> 16. turn the caches back on ;)
Please add this documentation to the source code as a (Doygen) comment.
> /*-----------------------------------------------------------------------------
> EDO/FP/SDRAM detection routine.
> ----------------------------------------------------------------------------*/
>
> /**
> * Initializes a row before it can be checked for DRAM
> */
> static void initialize_row(const struct mem_controller* ctrl,
> const uint8_t u8_row) {
Please don't encode type information in variable names. "row" is fine.
> uint8_t u8_DRAMSize = 0;
> uint8_t u8_DRAMType = 1;
Use all-lowercase variable and function names, please. There are many
other coding style issues in the code, please read
http://linuxbios.org/Development_Guidelines#Coding_Guidelines
http://lxr.linux.no/source/Documentation/CodingStyle
and fix the code to use the Linux kernel coding style.
> /* Initialize the current row to EDO and 32MB (maximum size) */
> u8_DRAMType <<= u8_row;
> u8_DRAMSize == SIMM_ROW_SIZE_MAX / SIMM_GRANULARITY;
>
> pci_write_config8(ctrl->d0,
> ((uint8_t)e_DRAM_ROW_BOUNDARY_BASE) + u8_row,
Is the (uint8_t) cast really needed? Otherwise please drop it.
> u8_DRAMSize);
>
> pci_write_config8(ctrl->d0,
> ((uint8_t)e_DRAM_ROW_TYPE),
> u8_DRAMType);
> }
>
> /**
> * Clears a row after it has been checked for DRAM
> */
> static void clear_row(const struct mem_controller* ctrl,
> const uint8_t u8_row) {
I think the 'const' for u8_row is not needed (not needed for integer
types in general, I think).
> /**
> * Detects DRAM row size
> */
> static void detect_size(const struct mem_controller* ctrl,
> uint8_t* au8_DRAMSize,
> const uint8_t u8_row)
> {
> uint32_t u32_AA0 = 0x0;
> uint32_t u32_AA8 = 1;
> uint32_t u32_DD1 = 0x55555555UL;
> uint32_t u32_DD2 = 0xAAAAAAAAUL;
> uint32_t u32_DD3 = 0xFEDCBA98UL;
> uint32_t u32_DD4 = 0;
> uint8_t u8_size_detected = 0;
> uint8_t u8_type = 0;
>
> for(;;) {
> /* Write some data */
> dimm_write32(u32_AA0, u32_DD1);
>
> switch (au8_DRAMSize[u8_row]) {
> case e_4MB : {
[...]
> case e_8MB : {
[...]
> case e_16MB : {
The code here looks pretty similar, can it be generalized in a
common function?
> static struct S_DRAMTechnology s_DRAMT32MB[] = {
You can make this
static const struct S_DRAMTechnology s_DRAMT32MB[]
for structs with purely constant content.
> {24, 22},
> {23, 24}
> };
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/20070810/60cec694/attachment.sig>
More information about the coreboot
mailing list