Patchwork Patch for Nokia-IP530, now with working PIRQ table, patch on 5591

login
register
about
Submitter marc bertens
Date 2010-05-26 15:17:32
Message ID <1274887052.28185.1.camel@andrala.reboot>
Download mbox | patch
Permalink /patch/1394/
State Accepted
Headers show

Comments

marc bertens - 2010-05-26 15:17:32
Signed-off Marc Bertens <mbertens@xs4all.nl>

In my earlier mail i forgot one file to patch. 

On Tue, 2010-05-25 at 23:22 +0200, mbertens wrote:
> Ok, new try
> 
> Please check it, if there is still something not
> correct please let me known, then ill fix the problem.
> 
> Thanks
> 
> Marc
> 
> 
> On Tue, 2010-05-25 at 23:05 +0200, mbertens wrote:
> > Still wrong, i saw of later comments in the mail that i need to fix, so
> > disregard my previous message. 
> > 
> > On Tue, 2010-05-25 at 23:02 +0200, mbertens wrote:
> > > I made a new diff, please check it, if there is still something not
> > > correct please let me known, then ill fix the problem. 
> > > 
> > > Marc  
> > > 
> > > 
> > > On Tue, 2010-05-25 at 10:34 -0600, Myles Watson wrote:
> > > > 
> > > > > -----Original Message-----
> > > > > From: mbertens [mailto:mbertens@xs4all.nl]
> > > > > Sent: Tuesday, May 25, 2010 10:21 AM
> > > > > To: Myles Watson
> > > > > Cc: Coreboot mailinglist
> > > > > Subject: Re: [coreboot] Patch for Nokia-IP530, now with working PIRQ table
> > > > > 
> > > > > Signed-off    Marc Bertens <mbertens@xs4all.nl>
> > > > 
> > > > 
> > > > +<<<<<<< .mine
> > > > +config VENDOR_NOKIA
> > > > +	bool "Nokia"
> > > > +=======
> > > >  config VENDOR_WYSE
> > > >  	bool "Wyse"
> > > > +>>>>>>> .r5583
> > > > 
> > > > This means that there was a conflict.  When your patch is ready there
> > > > shouldn't be any conflicts, and it should be obvious why you're making the
> > > > changes.  One thing that might help you would be to check out another copy
> > > > of coreboot and test patches on that unmodified repository.
> > > > 
> > > > Thanks,
> > > > Myles
> > > > 
> > > > 
> > > -- 
> > > coreboot mailing list: coreboot@coreboot.org
> > > http://www.coreboot.org/mailman/listinfo/coreboot
> -- 
> coreboot mailing list: coreboot@coreboot.org
> http://www.coreboot.org/mailman/listinfo/coreboot
Myles Watson - 2010-05-26 17:00:45
+			// 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)

It would also be nice to have an explanation.

The rest of your patch touches a lot of code with little explanation.  It
takes a lot more time to review patches like that.  For a faster review you
should split it up into pieces that add functionality.  For example, the
heap size (which seems really large) part of the patch should have an
explanation of what problem you see with a normal heap size.

Thanks,
Myles
marc bertens - 2010-05-26 20:15:15
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. 
> 
> It would also be nice to have an explanation.
And give more explaination why the change was made.
> 
> The rest of your patch touches a lot of code with little explanation.  It
> takes a lot more time to review patches like that.  For a faster review you
> should split it up into pieces that add functionality.  For example, the
I will give the patches in seperate diffs, with more explainations

> heap size (which seems really large) part of the patch should have an
> explanation of what problem you see with a normal heap size.
I was running in to problems with the heap size, therefor i increased it
to such a value that it would not bother me again :-). I will decrease
the value for it to see on which value it needs to be.

> 
> Thanks,
> Myles
> 
> 
> 
This is my first attempt to develop in an open source environment. And
i'm still learning things ie "the coding standard", i hope that i'm not
to much trouble, i will get it right one day :-)  

Marc
Myles Watson - 2010-05-26 21:14:33
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.
>>
>> It would also be nice to have an explanation.
> And give more explaination why the change was made.
>>
>> The rest of your patch touches a lot of code with little explanation.  It
>> takes a lot more time to review patches like that.  For a faster review you
>> should split it up into pieces that add functionality.  For example, the
> I will give the patches in seperate diffs, with more explainations
Great.

>> heap size (which seems really large) part of the patch should have an
>> explanation of what problem you see with a normal heap size.
> I was running in to problems with the heap size, therefor i increased it
> to such a value that it would not bother me again :-). I will decrease
> the value for it to see on which value it needs to be.

The reason why we care is that usually your device tree is wrong if
you run out of heap size.  We'd rather fix the root problem.

> This is my first attempt to develop in an open source environment. And
> i'm still learning things ie "the coding standard", i hope that i'm not
> to much trouble, i will get it right one day :-)

It's not too much trouble.  Thanks for contributing.

Thanks,
Myles
Peter Stuge - 2010-05-27 00:27:04
mbertens wrote:
> +++ src/arch/i386/boot/pirq_routing.c	(working copy)
> @@ -121,7 +121,11 @@
>  
>  			printk(BIOS_DEBUG, "INT: %c link: %x bitmap: %x  ",
>  				'A' + j, link, bitmap);
> -
> +			// fix made by Marc Bertens <mbertens@xs4all.nl>
> +			if (link > 0x5f) {
> +				// This is basically for the 440BX
> +				link -= 0x5f;
> +			}

Um, except that I gave you that code.

It is a bit of a hack. So far this file and function is actually only
called by Geode LX mainboards, and I don't quite understand why.

Maybe something equivalent of this code is invoked in other ways in
all the other boards? The gist of pirq_routing_irqs() is to call
pci_assign_irqs() and pirq_assign_irqs(). How 

There are various other solutions to calling pci_assign_irqs() on
other boards:

northbridge/via/cx700/cx700_lpc.c
northbridge/via/vx800/vx800_lpc.c
southbridge/via/vt8231/vt8231_lpc.c
southbridge/via/vt8235/vt8235_lpc.c
southbridge/via/vt8237r/vt8237r_lpc.c
mainboard/amd/serengeti/cheetah/irq_tables.c
mainboard/msi/ms7135/irq_tables.c
mainboard/olpc/btest/mainboard.c
mainboard/tyan/s2882/irq_tables.c
mainboard/iwill/dk8_htx/irq_tables.c
mainboard/emulation/qemu-x86/mainboard.c
mainboard/technologic/ts5300/mainboard.c
mainboard/digitallogic/msm586seg/mainboard.c

I have fairly strong opinion about those call sites being so
different (maybe due to ignorance) but more importantly what are the
other boards doing? There seems to be a large number of non-Geode
systems not in the list above where pci_assign_irqs() is never
called. The doxygen for the function says:

"This function should be called for each PCI slot in your system."

What gives? Can we find a generic solution to this interrupt problem?


//Peter
Peter Stuge - 2010-05-27 00:29:34
Peter Stuge wrote:
> I have fairly strong opinion about those call sites being so
> different (maybe due to ignorance)

*my* ignorance


//Peter
Peter Stuge - 2010-05-27 00:31:50
mbertens wrote:
> This is my first attempt to develop in an open source environment.

Cool! :)


> And i'm still learning things ie "the coding standard",

I guess you already saw the
http://www.coreboot.org/Development_Guidelines wiki page. It's not
neccessarily complete, so please point out things that you encounter
that seem to be missing from the page, so that they can be added.


> i hope that i'm not to much trouble, i will get it right one day :-)

I don't think it'll take very long. :)


//Peter
marc bertens - 2010-05-27 05:14:57
On Thu, 2010-05-27 at 02:27 +0200, Peter Stuge wrote:
> mbertens wrote:
> > +++ src/arch/i386/boot/pirq_routing.c	(working copy)
> > @@ -121,7 +121,11 @@
> >  
> >  			printk(BIOS_DEBUG, "INT: %c link: %x bitmap: %x  ",
> >  				'A' + j, link, bitmap);
> > -
> > +			// fix made by Marc Bertens <mbertens@xs4all.nl>
> > +			if (link > 0x5f) {
> > +				// This is basically for the 440BX
> > +				link -= 0x5f;
> > +			}
> 
> Um, except that I gave you that code.
Yes, that was already in the new comment, that it was passed to me
by you. 
> 
> It is a bit of a hack. So far this file and function is actually only
> called by Geode LX mainboards, and I don't quite understand why.
It just works fne for the i440BX, but you need a stub function
pirq_assign_irqs() in the board specific codebase (my solution).
> 
> Maybe something equivalent of this code is invoked in other ways in
> all the other boards? The gist of pirq_routing_irqs() is to call
> pci_assign_irqs() and pirq_assign_irqs(). How 
pirq_routing_irqs() calls as the last action pirq_assign_irqs()
> 
> There are various other solutions to calling pci_assign_irqs() on
> other boards:
> 
> northbridge/via/cx700/cx700_lpc.c
> northbridge/via/vx800/vx800_lpc.c
> southbridge/via/vt8231/vt8231_lpc.c
> southbridge/via/vt8235/vt8235_lpc.c
> southbridge/via/vt8237r/vt8237r_lpc.c
> mainboard/amd/serengeti/cheetah/irq_tables.c
> mainboard/msi/ms7135/irq_tables.c
> mainboard/olpc/btest/mainboard.c
> mainboard/tyan/s2882/irq_tables.c
> mainboard/iwill/dk8_htx/irq_tables.c
> mainboard/emulation/qemu-x86/mainboard.c
> mainboard/technologic/ts5300/mainboard.c
> mainboard/digitallogic/msm586seg/mainboard.c
I looked at all those you meantioned here, and soem of them are quite
similar. Maybe we can find a common solution "one size fits all", i like
sourcecode that can handle all the different board types. 
> 
> I have fairly strong opinion about those call sites being so
> different (maybe due to ignorance) but more importantly what are the
> other boards doing? There seems to be a large number of non-Geode
> systems not in the list above where pci_assign_irqs() is never
> called. The doxygen for the function says:
> 
> "This function should be called for each PCI slot in your system."
> 
> What gives? Can we find a generic solution to this interrupt problem?
First thing is that we need to identify the different board solutions
which are similar, and see what kind of solution groups we have at the
moment, and maybe its just enough to have a adjuster function for a
specific board, which is called start of the pirq_routing_irqs(), and
when no specific adjusting is required this can be left out by Kconfig
and the same applies to the pirq_assign_irqs()
> 
> 
> //Peter
> 

Marc
marc bertens - 2010-05-27 05:17:20
On Thu, 2010-05-27 at 02:31 +0200, Peter Stuge wrote:
> mbertens wrote:
> > This is my first attempt to develop in an open source environment.
> 
> Cool! :)
> 
> 
> > And i'm still learning things ie "the coding standard",
> 
> I guess you already saw the
> http://www.coreboot.org/Development_Guidelines wiki page. It's not
> neccessarily complete, so please point out things that you encounter
> that seem to be missing from the page, so that they can be added.
Yes i was reading that, i have to dislearn what i'v learned :-)
> 
> 
> > i hope that i'm not to much trouble, i will get it right one day :-)
> 
> I don't think it'll take very long. :)
> 
> 
> //Peter
> 
Marc
Peter Stuge - 2010-05-27 07:08:53
mbertens wrote:
> > It is a bit of a hack. So far this file and function is actually only
> > called by Geode LX mainboards, and I don't quite understand why.
> 
> It just works fne for the i440BX, but you need a stub function
> pirq_assign_irqs() in the board specific codebase (my solution).

Sure, but I'm talking about the nearly 200 boards in coreboot which
do not use this code but still work. I want some comments from people
who did some other board ports.

How does 945 handle IRQ assignment? The other 440BX boards? And how
about the 830 systems, or ServerWorks, or RS690/SB600?


//Peter
Stefan Reinauer - 2010-05-27 09:54:00
On 5/27/10 9:08 AM, Peter Stuge wrote:
> mbertens wrote:
>   
>>> It is a bit of a hack. So far this file and function is actually only
>>> called by Geode LX mainboards, and I don't quite understand why.
>>>       
>> It just works fne for the i440BX, but you need a stub function
>> pirq_assign_irqs() in the board specific codebase (my solution).
>>     
> Sure, but I'm talking about the nearly 200 boards in coreboot which
> do not use this code but still work. I want some comments from people
> who did some other board ports.
>
> How does 945 handle IRQ assignment? The other 440BX boards? And how
> about the 830 systems, or ServerWorks, or RS690/SB600?
>
>
> //Peter
>
>   
Mapping of 8259 interrupts:
http://tracker.coreboot.org/trac/coreboot/browser/trunk/src/mainboard/kontron/986lcd-m/devicetree.cb#L15
http://tracker.coreboot.org/trac/coreboot/browser/trunk/src/southbridge/intel/i82801gx/i82801gx_lpc.c#L99

PIRQ:
http://tracker.coreboot.org/trac/coreboot/browser/trunk/src/mainboard/kontron/986lcd-m/irq_tables.c

MP:
http://tracker.coreboot.org/trac/coreboot/browser/trunk/src/mainboard/kontron/986lcd-m/mptable.c

ACPI:
http://tracker.coreboot.org/trac/coreboot/browser/trunk/src/mainboard/kontron/986lcd-m/acpi/i945_pci_irqs.asl
http://tracker.coreboot.org/trac/coreboot/browser/trunk/src/mainboard/kontron/986lcd-m/acpi/ich7_pci_irqs.asl

Patch

Index: src/arch/i386/boot/pirq_routing.c
===================================================================
--- src/arch/i386/boot/pirq_routing.c	(revision 5591)
+++ src/arch/i386/boot/pirq_routing.c	(working copy)
@@ -121,7 +121,11 @@ 
 
 			printk(BIOS_DEBUG, "INT: %c link: %x bitmap: %x  ",
 				'A' + j, link, bitmap);
-
+			// fix made by Marc Bertens <mbertens@xs4all.nl>
+			if (link > 0x5f) {
+				// This is basically for the 440BX
+				link -= 0x5f;
+			}
 			if (!bitmap|| !link || link > 4) {
 
 				printk(BIOS_DEBUG, "not routed\n");