[coreboot] patch: my latest mcp55
Carl-Daniel Hailfinger
c-d.hailfinger.devel.2006 at gmx.net
Wed Aug 13 19:34:06 CEST 2008
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/
More information about the coreboot
mailing list