On Thu, 2010-05-27 at 20:32 +0200, Peter Stuge wrote:
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.
I changed it because i saw some implementations that have even higher values in the PIRQ table (above 0x63) and i think that the link value can never be above 3 anyway (0..3), so for that the the AND value should even 0x03, to enforce that.
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.
Correct but for now until a better solution is there i think we should put it in, other i440BX users should have the same type of problem that i've had.
+++ 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.
Ok i did'nt know that, i was doing this kind of comments all the time in my own projects and i was just trying to explain why i did the fix. there.
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.
For understanding the code i think its quiet nice to have a lot of comments, what the programmer is doing. I find it conviniant to put in information about specicifations and that kind of thing. But if you think its better to keep the comments short no problem i will take it out.
@@ -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.
Even so that this is not the way to do it correctly, i like to put it in for now. By doing it this way basically nobody should have a problem with this addition. And i like to solve it the correct way, but i started two months ago with de coreboot project (and i like it alot) and overhauling the code for the i440BX is just a little to abitieus at the moment. Certainly due to that there are many boards with this chipset.
//Peter
And finally i like to keep working on this and fixing this at a later time so that it fits all. I dont think its handy solution that i need to keep a patch local to put this every time in when the code of the pirq_routing object changes. And others cannot benefit from this and they need to keep this patch locally too.
Let me know what you think, then i'll make a new patch with less comments. And the part of adjusting the link value for the i440BX will be a TODO comment.
Marc