[LinuxBIOS] RAM detection for Intel 430FX

[Uwe Hermann]

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>