[coreboot] patch: my latest mcp55

[ron minnich]

Committed revision 756.

Also:
On Wed, Aug 13, 2008 at 5:06 AM, Carl-Daniel Hailfinger
<c-d.hailfinger.devel.2006 at gmx.net> wrote:


> That code is really buggy. I wonder how/if it ever worked in v2. If you
> address the comments below, this is
> Acked-by: Carl-Daniel Hailfinger <c-d.hailfinger.devel.2006 at gmx.net>

buggy in a style sense, but it worked. So I am only changing things
below related to style.

You're going to fix the subsys stuff :-)

I am very careful not to change functionality. This is a tricky chip.
It appears to have lots of bugs.


>
> I am now entitled to the title "professional reviewer" and need to get
> that printed on a t-shirt ;-)

we'll get that done :-)

>> +     struct ioapicreg *a = ioapicregvalues;
>>
>
> Either we use "ioapicregvalues" everywhere or we use "a" everywhere.
> Your choice.

once we get it working, we will make that change.

>> +             ioapicregvalues[0].value_high = NONE;
>> +             ioapicregvalues[0].value_low = DISABLED;
>> +     }
>> +
>> +     l = (unsigned long *) ioapic_base;
>> +
>> +     for (i = 0; i < sizeof(ioapicregvalues) / sizeof(ioapicregvalues[0]);
>>
>
> Please use the ARRAY_SIZE macro.

Next pass :-)


> What's the problem with using ioapicregvalues[i] and killing a completely?

I don't know. Again, next rev. I would actually prefer to use 'a'
everywhere, myself. Some compilers intentionally do more optimization
when given a pointer as opposed to an array index.

>
>> +             l[0] = (a->reg * 2) + 0x10;
>> +             l[4] = a->value_low;
>>
>
> These l[0] l[4] accesses need to be moved to a separate function. They
> are clearly paired accesses. One could also argue for writel() here.
>
>> +             value_low = l[4];
>>
>
> I don't get the idea here. Should this be a readback? If yes, this won't
> work. GCC is free to optimize this away. Needs to be readl().

No, it will work; l is volatile. We must not forget: this code is in
use supporting mainboards; we can not assume that things that "look
wrong" are wrong. We may be wrong :-)


>
> #ifdef CONFIG_HPET ?

I added that config variable.

Lest we forget: to be blunt, the HPET experience on almost all
platforms has not been a good one. In general, I don't see a need to
enable it in coreboot -- any reason to? Clearly, it did not work out.

I put the CONFIG_HPET around this, but left the call to it commented out :-)

>> +     /* Initialize the High Precision Event Timers */
>> +//   enable_hpet(dev);
>>
>
> #ifdef CONFIG_HPET instead of commenting it out?

I added a stronger comment to the effect of "NO!".
:-)

>> +                                     if( (base == 0x290) || (base >= 0x400)) {
>> +                                             if(var_num>=4) continue; // only 4 var ; compact them ?
>>
>
> comment doesn't match code.

YingHai, what does this comment mean?


>> -STAGE0_CHIPSET_OBJ += $(obj)/southbridge/nvidia/mcp55/stage1.o
>> +STAGE2_CHIPSET_SRC += $(src)/southbridge/nvidia/mcp55/ide.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/pci.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/lpc.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/pcie.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/sata.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/smbus.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/usb.c \
>> +                                     $(src)/southbridge/nvidia/mcp55/usb2.c \
>> +
>> +STAGE0_CHIPSET_OBJ += $(obj)/southbridge/nvidia/mcp55/stage1.o \
>>
>
> The \ at the end of the line needs to go away.
>>

style note: in the Bell Labs source, they leave the \ there. It's ok,
next line is blank. So the \ is actually common in the Land That
Invented Make. I took it out; it's not commonly done in GNU land. I
did this one out of habit; I prefer the Bell Labs style. It forces
blank lines. But I will go with the GNU style here, I'm the minority!

thanks

ron