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