On Thursday 11 June 2009 01:06:05 Luc Verhaegen wrote: > On Tue, Jun 09, 2009 at 09:18:58PM +0200, Harald Gutmann wrote: > > Hello, > > > > after a few tries of me to fix the interrupt problems which occurred on > > the M57SLI here is the final patch from me. > > > > The Patch fixes the following issues on M57SLI: > > * Interrupt routing in the MPTable > > > > which affects the following hardware parts: > > * PCI-E 16x Slots (blue and black) - both are working now like a charm > > * PCI-E 1x Slots - all three should be fine, but I have no card to test. > > (untested) > > * PCI Slots - both work fine now. > > > > This is the first and (hopefully) the last patch on this issue which gets > > a > > > > Signed-off-by: Harald Gutmann <[email protected]> > > > > from me. > > > > I'm really looking forward to commit this patch (which is my first one) > > and mark the interrupt issues on the wiki page to the M57SLI as solved. > > :) > > > > > > Kind regards, > > Harald Gutmann > > > > > > > > @@ -87,19 +87,17 @@ > > if (res) { > > smp_write_ioapic(mc, apicid_mcp55, 0x11, > > res->base); > > } > > - > > - dword = 0x43c6c643; > > + dword = 0xc643c643; > > pci_write_config32(dev, 0x7c, dword); > > > > - dword = 0x81001a00; > > + dword = 0x8da01009; > > pci_write_config32(dev, 0x80, dword); > > > > - dword = 0xd0001202; > > + dword = 0x200018d2; > > pci_write_config32(dev, 0x84, dword); > > - > > } > > } > > - > > + > > Anal stuff: while you're here, do get rid of dword :) I'll do that.
>
> > /* The PCIe slots, each on its own bus */
> > - for(j=7; j>=2; j--) {
> > - if(!bus_mcp55[j]) continue;
> > - for(i=0;i<4;i++) { /* map all functions */
> > - PCI_INT(j,0,i, 16+(1+j+i)%4);
> > - }
> > - }
> > + k = 1;
> > + for(i=0; i<=3; i++){
> > + for(j=7; j>=2; j--){
> > + if(k>3) k=0;
> > + PCI_INT(j,0,i, 16+k);
> > + k++;
> > + }
> > + k--;
> > + }
>
> Anal stuff: i<4 and j>1 is easier on the eye :)
Yes, sounds good.
> > /* On bus 1: the physical PCI bus slots... */
> > - for(j=0; j<2; j++) /* on a Rev 1.x board, they are devs 7 and 8 */
> > - for(i=0;i<4;i++) { /* map all functions */
> > - PCI_INT(1,7+j,i, 16+(3+i+j)%4);
> > - }
> > - /* ... and OB FireWire */
> > - PCI_INT(1,0x0a,0, 18);
> > + k=2;
> > + for(i=0; i<=3; i++){
> > + for(j=6; j<=10; j++){
> > + if(k>3) k=0;
> > + PCI_INT(1,j,i, 16+k);
> > + k++;
> > + }
> > + }
>
> This is the only bit i have a real question about: you remove the comment:
> /* on a Rev 1.x board, they are devs 7 and 8 */
Oh, removing the comment was not my intention, it seems that i just missed
this line.
> and have gone from 7+[0,1] to [6,7,8,9,10] for the devices.
>
> While i like the for loops used this way, you are touching devices 6,9,10
> which weren't touched before. What board revision are you using? Could this
> be the difference between your board and what yinghai had? What possible
> effects are there with assigning irqs in the apic to devices which possibly
> aren't there (as i have no clue about apics)?
Okay, good that you ask this, because I'm not really sure if it is a benefit to
add the devices 6,9,10, but when Rudolf Marek figured out the wiring from the
vendors dsdt.asl file there appeared all devices from 6-10.
I haven't looked at the mainboard detail if there is something wired and used
from that devices, so i can't give a good answer to your question.
But i recognized that there is at least one line which is done doubled in the
code, and it would be for sure a good idea to drop this line:
+ PCI_INT(1,0x0a,0, 18); /* Firewire */
For the wiring which was figured out from the dstd.asl you can have a look on
http://www.coreboot.org/Nvidia_MCP55_Porting_Notes where I added the
information.
Has anyone of you comments/ideas on that?
Would it be better to ignore devices 6,9,10 and add a single initialization
for the firewire device?
Does it matter if the "unnecessary" devices get initialized?
> Please keep at least some of Yinghai's comment and expand this with your
> information.
Yes, that's a good idea, I'll keep at least a modified comment on the
initialization for Bus 1.
> This is the only question i have with this change, if i see a good answer
> to this, you have:
>
> Acked-by: Luc Verhaegen <[email protected]>
>
> Luc Verhaegen.
Thanks for reviewing my first patch which is intended to get committed.
Harald Gutmann
signature.asc
Description: This is a digitally signed message part.
-- coreboot mailing list: [email protected] http://www.coreboot.org/mailman/listinfo/coreboot
