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
+ // 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
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
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
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 wrote: > I have fairly strong opinion about those call sites being so > different (maybe due to ignorance) *my* ignorance //Peter
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
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
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
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
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");