Re: [LinuxBIOS] RAM detection for Intel 430FX

[popkonserve]

> > This file is part of the LinuxBIOS project.
> > Copyright (C)
>
> Please add 2007 Holger Yourlastname

fixed. left it out as it isn't tested at all..and shouldn't be used as 
long as it isn't.
> Typo == instead of = ?

fixed, too :)
> > pci_write_config8(ctrl->d0,
> >                    ((uint8_t)e_DRAM_ROW_TYPE),
> >                    u8_DRAMType);
>
> Does this want to be a set_row_type() function?

hmm, wouldn't make much sense here i guess. the register that stores the 
type of drams used is a single register for all rows. so the only thing 
the function would do would be a single write to the same register for 
all rows. only the data would change.
> > /* Enable EDO detection mode */
>
> Maybe also candidate for a separate function?

then i would have to write a disable_edo_detection, too. and again all 
they would to is to write one special register.
both calls are alread inside a function that is called detect_type and 
that's where they belong :)
> Please change the for() into do {} while(); to improve readability.

done.
> > /* Turn off cache (L1 & L2) */
>
> Isn't there a function for this already? If not, should there be?

there should be..i didn't look. while L1 cache could be switched off on 
socket 7 processors, the L2 cache is controlled by the chipset. i don't 
know if there's a 'disable L2 cache' pin on socket 7 processors that 
would tell the chipset to ignore its L2 cache. so i used the chipset's 
function rather than leaving it as TODO.
> > /* 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?

well, there are two kind answers: yes and no. if i do not set all rows 
to zero, the writes in the type and size detection routines would have 
to be recalculated for each row. they would have to take into account 
that there already is ram in front of them and they would have to write 
to ex. OFFSET + AA0
just to avoid some more mind breaking calculation i decided to set the 
ram size to zero for all rows before the detection starts and set it 
back to zero for the current row when the detection is over.
if there's a problem with that i could rewrite the functions to use 
offsets, too.
> > /* 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.

i use the size parameter as indication if the row is populated at all 
and thus skipping the rest of the detection process (why trying to 
detect anything else if there's no ram in there?).
> Maybe a return value from detect_type() would be nicer?

yes, maybe. no problem to change it if the others agree on this one. i 
would want to reuse the code for the other socket 7 chipsets, too. even 
with a return parameter reuseability wouldn't be hurt at all.
Holger