mbertens wrote:
Here is the patch just for the pirq_routing() function. Its made specific to the CONFIG_NORTHBRIDGE_INTEL_440BX if that is to generic please replace by CONFIG_BOARD_NOKIA_IP530.
_440BX is good, but there are still a few issues.
- added the correction for the i440BX by AND the link value with 0x5F
so that always the value is kept below 0x5F. That AND value should be 0x03 i think because the link value cannot be greater than 3. But i'm not sure about that, thats why i used the current solution.
This is different from my code in your last patch. Why did it change? I'd love to know if there was a problem with using subtraction, in order to understand the work you and others are doing.
My original if(link>0x5f) link-=0x5f; is a really crude hack that makes some wild assumptions in order to translate data at hand into data that is needed. I think this must not be coded into coreboot in this particular manner.
+++ src/arch/i386/boot/pirq_routing.c (working copy)
- printk(BIOS_INFO, "Verifing copy of Interrupt Routing Table at 0x%08x... ", addr);
- /**
* fix made by Marc Bertens <mbertens@xs4all.nl>** The compiler was putting out a warning that the 'addr' value* was of the unsigned int long type but the printk() was using '%08x'*/- printk(BIOS_INFO, "Verifing copy of Interrupt Routing Table at 0x%08lx... ", addr);
These comments are not ok. Please consider what the source code would look like if every commit that touched something small like a printk argument came with 6 lines of comments.
Also, don't try to document who made changes to the code *within the code* because it will not work over time and more importantly because the source control system that we are using (subversion) keeps track of this for us! You sign-off, someone commits, the commit gets a revision number, SVN logs the commit message (which includes sign-off) and SVN can also find the revision number that is responsible for every line of source code in the project, so there is already perfect traceability both to the commiter (svn username) and to the original author(s) (Signed-off-by) without trying to store that in comments in the code.
Finally, comments about changes that have been made go into the commit message and not into a comment in the source code.
Documentation in comments is not at all forbidden, but I certainly think that it's important to not have *too many* comments in the code either. Some code certainly deserves to be commented, but not all, then it is probably so weird that it should be reworked a bit to be clear also without comments.
@@ -121,7 +147,24 @@
printk(BIOS_DEBUG, "INT: %c link: %x bitmap: %x ", 'A' + j, link, bitmap);
+#if CONFIG_NORTHBRIDGE_INTEL_I440BX == 1
/*** fix made by Marc Bertens <mbertens@xs4all.nl>** this was done for the Northbridge i440BX, due to the fact* that the values in the PIRQ table needs to be 60, 61, 62* and 63. This was passed to me by Idwer Vollering <vidwer@gmail.com>* and Peter Stuge <peter@stuge.se> helped with this* fix, so that the IRQ routing is done.*/if (link > 0x5f) {/*** as if the maximum value can be 0x5F we should* AND it instead of substracting, my opinion.*/link &= 0x5F;}+#endif
NAK for this. Even if it works, it's not the right way to solve this problem. I don't think this can be fixed without some more investigation and possibly central changes in coreboot. I don't think they will be very large changes, but still.
//Peter