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 <[email protected]> > + * > + * 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 <[email protected]> > + * > + * 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 > <[email protected]> > + * and Peter Stuge <[email protected]> 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 -- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
