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

[Peter Stuge]

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

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


//Peter