Patchwork Patch for Nokia-IP530, now with working PIRQ table, fix on the pirq_routing

login
register
about
Submitter marc bertens
Date 2010-05-27 09:56:03
Message ID <1274954163.28185.43.camel@andrala.reboot>
Download mbox | patch
Permalink /patch/1404/
State Accepted
Headers show

Comments

marc bertens - 2010-05-27 09:56:03
Signed-off-by: Marc Bertens <mbertens@xs4all.nl>

Hi all,

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. 

Put the following extras in the file; 
- added header accordingly "Common License Header" that was missing.
- corrected a printk() warning of the compiler.
- 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.

The other patches will follow to day.

Marc 



On Wed, 2010-05-26 at 15:14 -0600, Myles Watson wrote:
> On Wed, May 26, 2010 at 2:15 PM, mbertens <mbertens@xs4all.nl> wrote:
> > On Wed, 2010-05-26 at 11:00 -0600, Myles Watson wrote:
> >> +                     // fix made by Marc Bertens <mbertens@xs4all.nl>
> >> +                     if (link > 0x5f) {
> >> +                             // This is basically for the 440BX
> >> +                             link -= 0x5f;
> >> +                     }
> >>
> >> I'd prefer this to be guarded by
> >> #if CONFIG_NORTHBRIDGE_INTEL_440BX (or whatever the correct one is)
> > I was thinking of it to put it that way, but i'd. But i will make the
> > changes to the code.
> >>
marc bertens - 2010-05-27 10:14:37
Signed-off-by: Marc Bertens <mbertens@xs4all.nl>

Part two now the PIRQ table for the Nokia IP530.

Changes made:
- rebuild the table to get th eNICs and PCMCIA controllers get there
correct IRQs assigned. 
- Added a stub pirq_assign_irqs() due to the fact that it was missing at
compilation time. This was discussed with Rudolf Marek (ruik)
<r.marek@assembler.cz>

Marc


On Thu, 2010-05-27 at 11:56 +0200, mbertens wrote:

> Hi all,
> 
> 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. 
> 
> Put the following extras in the file; 
> - added header accordingly "Common License Header" that was missing.
> - corrected a printk() warning of the compiler.
> - 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.
> 
> The other patches will follow to day.
> 
> Marc 
> 
> 
> 
> On Wed, 2010-05-26 at 15:14 -0600, Myles Watson wrote:
> > On Wed, May 26, 2010 at 2:15 PM, mbertens <mbertens@xs4all.nl> wrote:
> > > On Wed, 2010-05-26 at 11:00 -0600, Myles Watson wrote:
> > >> +                     // fix made by Marc Bertens <mbertens@xs4all.nl>
> > >> +                     if (link > 0x5f) {
> > >> +                             // This is basically for the 440BX
> > >> +                             link -= 0x5f;
> > >> +                     }
> > >>
> > >> I'd prefer this to be guarded by
> > >> #if CONFIG_NORTHBRIDGE_INTEL_440BX (or whatever the correct one is)
> > > I was thinking of it to put it that way, but i'd. But i will make the
> > > changes to the code.
> > >>
>
Peter Stuge - 2010-05-27 18:32:05
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
marc bertens - 2010-05-27 19:59:09
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

Patch

Index: src/arch/i386/boot/pirq_routing.c
===================================================================
--- src/arch/i386/boot/pirq_routing.c	(revision 5593)
+++ src/arch/i386/boot/pirq_routing.c	(working copy)
@@ -1,3 +1,23 @@ 
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright (C) 2000-2010 Coreboot project
+ * Copyright (C) 2010 Marc Bertens <mbertens@xs4all.nl>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA  02110-1301 USA
+ */
 #include <console/console.h>
 #include <arch/pirq_routing.h>
 #include <string.h>
@@ -64,7 +84,13 @@ 

 	rt_curr = (uint8_t*)addr;
 	rt_orig = (uint8_t*)&intel_irq_routing_table;
-	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);
 	for (i = 0; i < intel_irq_routing_table.size; i++) {
 		if (*(rt_curr + i) != *(rt_orig + i)) {
 			printk(BIOS_INFO, "failed\n");
@@ -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
 			if (!bitmap|| !link || link > 4) {

 				printk(BIOS_DEBUG, "not routed\n");