[coreboot] Patch for Nokia-IP530, now with working PIRQ table, fix on the pirq_routing

[mbertens]

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 at 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 at 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 at gmail.com>
> > +			  * and Peter Stuge <peter at 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