[coreboot] patch: my latest mcp55

[Carl-Daniel Hailfinger]

On 13.08.2008 17:54, ron minnich wrote:
> Committed revision 756.
>
> Also:
> On Wed, Aug 13, 2008 at 5:06 AM, Carl-Daniel Hailfinger 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.
>   

Thanks for rechecking my review.

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

Sure.

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

Noted.

>> 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.
>   

OK

>>> +             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 :-)
>   

OK.

>> 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.
>   

Hm. As long as the code becomes more readable, this is OK with me.

>>> +             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 :-)
>   

I really looked hard for the volatile and didn't see it. My mistake.

>> #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 :-)
>   

Agreed. HPET has been a constant source of problems in the Linux kernel,
probably due to suboptimal hardware.

>>> +     /* 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 for educating me. Bell Labs style sure looks beneficial. My only
fear is that a \ at the end may one day cause interesting effects if
someone removes the blank line after it by accident. No preference on my
side.

Regards,
Carl-Daniel

-- 
http://www.hailfinger.org/