Re: [LinuxBIOS] RAM detection for Intel 430FX

[Peter Stuge]

On Fri, Aug 10, 2007 at 12:09:36AM +0200, popkonserve wrote:
> /*
> This file is part of the LinuxBIOS project.
> Copyright (C)

Please add 2007 Holger Yourlastname
> static void initialize_row(const struct mem_controller* ctrl,
>                            const uint8_t u8_row) {
>
> uint8_t u8_DRAMSize = 0;
>   uint8_t u8_DRAMType = 1;
>
> /* Initialize the current row to EDO and 32MB (maximum size) */
>   u8_DRAMType <<= u8_row;
>   u8_DRAMSize == SIMM_ROW_SIZE_MAX / SIMM_GRANULARITY;

Typo == instead of = ?
> pci_write_config8(ctrl->d0,
>                     ((uint8_t)e_DRAM_ROW_BOUNDARY_BASE) + u8_row,
>                     u8_DRAMSize);

..will always write 0 otherwise.
> pci_write_config8(ctrl->d0,
>                     ((uint8_t)e_DRAM_ROW_TYPE),
>                     u8_DRAMType);

Does this want to be a set_row_type() function?
> /* Enable EDO detection mode */
>   u8_DRAMC = pci_read_config8(ctrl->d0,
>                               ((uint8_t)e_DRAM_CONTROL));
>
> u8_DRAMC |= ((uint8_t)e_DRAMC_EDO_DETECT);
>   pci_write_config8(ctrl->d0,
>                     ((uint8_t)e_DRAM_CONTROL),
>                     u8_DRAMC);

Maybe also candidate for a separate function?
> static void detect_size(const struct mem_controller* ctrl,

[..]
> for(;;) {

[..]
> /* Quit if the size of the current row was detected or an error occured */
> if((u8_size_detected != 0) ||
>     au8_DRAMSize[u8_row] == ((uint8_t)e_EMPTY)){
>   break;
> }
>
> }

Please change the for() into do {} while(); to improve readability.
> /* Turn off cache (L1 & L2) */

Isn't there a function for this already? If not, should there be?
> /* Set size of DRAM to 0 for all rows */
>   for(u8_row = 0; u8_row < SIMM_SOCKETS; u8_row++) {
>     au8_DRAMSize[u8_row] = 0;
>     pci_write_config8(ctrl->d0,
>                       ((uint8_t)e_DRAM_ROW_BOUNDARY_BASE) + u8_row,
>                       au8_DRAMSize);
>   }
>
> /* Detect RAM presence and type in all rows */
>   for(u8_row = 0; u8_row < SIMM_SOCKETS; u8_row++) {

All rows need to be zeroed first to make this reliable?
> /* Detect type of ram in the current row */
> detect_type(ctrl,
>             au8_DRAMSize,
>             &u8_DRAMType,
>             u8_row);
>
> /* Check if any DRAM was found in this row */
> if(au8_DRAMSize[u8_row] == 0) {
>   /* No DRAM found. Skip the rest. */
>   continue;
> }

A bit unexpected to have detect_type() return meaningful data in a
Size parameter.

Maybe a return value from detect_type() would be nicer?

Looks good! :)

//Peter