Gigabyte M57SLI-S4 Interrupt Fix

[Harald Gutmann]

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

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

[Rudolf Marek]

Hi.

Maybe you can extend the ck804 wiki page with that regs info?

And/or document it in the code?

Thanks,
Rudolf

[Luc Verhaegen]

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
>
> 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 :)
> /* 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 :)
> /* 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 */
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)?

Please keep at least some of Yinghai's comment and expand this with your
information.

This is the only question i have with this change, if i see a good answer
to this, you have:

Acked-by: Luc Verhaegen

Luc Verhaegen.

[Luc Verhaegen]

On Thu, Jun 11, 2009 at 01:06:05AM +0200, 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
> >
> > 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 :)
> > /* 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 :)
> > /* 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 */
> 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)?
>
> Please keep at least some of Yinghai's comment and expand this with your
> information.
>
> This is the only question i have with this change, if i see a good answer
> to this, you have:
>
> Acked-by: Luc Verhaegen
>
> Luc Verhaegen.

Apparently the email in the copyright statement was no longer valid.

Yinghai, maybe you should traverse over the tree and adjust it everywhere :)

Luc Verhaegen.

[Harald Gutmann]

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

Thanks for reviewing my first patch which is intended to get committed.
Harald Gutmann

[Harald Gutmann]

Hello,

here is the new version of the fix.
I cleaned out all of the stuff you mentioned.

The last point with initializing some "not existent" hardware PCI slots is as 
I initial did it.
Discussion on that point would be good.
Shall we leave it like it is in my patch, or shall we just initialize 7,8 
(physically) and 10,0 for FireWire?

From my view the patch is in a good shape and was in the mean time verified by 
Florentin Demetrescu (partially) as working and therefore it get's the:

Signed-off-by: Harald Gutmann

Kind regards,
and thanks for reviewing and discussing,
Harald Gutmann

On Thursday 11 June 2009 01:06:05 you 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
> >
> > 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 :)
> > /* 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 :)
> > /* 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 */
> 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)?
>
> Please keep at least some of Yinghai's comment and expand this with your
> information.
>
> This is the only question i have with this change, if i see a good answer
> to this, you have:
>
> Acked-by: Luc Verhaegen
>
> Luc Verhaegen.

[Carl-Daniel Hailfinger]

Hi Harald,

On 14.06.2009 16:25, Harald Gutmann wrote:
> here is the new version of the fix.
> I cleaned out all of the stuff you mentioned.
>
> The last point with initializing some "not existent" hardware PCI slots is as 
> I initial did it.
> Discussion on that point would be good.
> Shall we leave it like it is in my patch, or shall we just initialize 7,8 
> (physically) and 10,0 for FireWire?
>
> From my view the patch is in a good shape and was in the mean time verified by 
> Florentin Demetrescu (partially) as working and therefore it get's the:
>
> Signed-off-by: Harald Gutmann

Does this fix also work on version 1.x of the board? I think Ward had
problems with it.

If all revisions of the board work with this patch (or at least nothing
gets worse), the patch is
Acked-by: Carl-Daniel Hailfinger

Remember to use Luc's Ack as well if you commit.

Regards,
Carl-Daniel

[Harald Gutmann]

On Tuesday 16 June 2009 14:18:41 Carl-Daniel Hailfinger wrote:
> Hi Harald,
>
> On 14.06.2009 16:25, Harald Gutmann wrote:
> > here is the new version of the fix.
> > I cleaned out all of the stuff you mentioned.
> >
> > The last point with initializing some "not existent" hardware PCI slots
> > is as I initial did it.
> > Discussion on that point would be good.
> > Shall we leave it like it is in my patch, or shall we just initialize 7,8
> > (physically) and 10,0 for FireWire?
> >
> > From my view the patch is in a good shape and was in the mean time
> > verified by Florentin Demetrescu (partially) as working and therefore it
> > get's the:
> >
> > Signed-off-by: Harald Gutmann
>
> Does this fix also work on version 1.x of the board? I think Ward had
> problems with it.

I can't say it exactly where the problem which remains on v1 is coming from, 
but I think that this is not related to my patch as the DSDT.asl,  which is 
the base information source for this patch, from v1 and v2  is exactly the 
same (except an address base used for something else).
> If all revisions of the board work with this patch (or at least nothing
> gets worse), the patch is

All revisions work so far, except a little NMI problem on v1, but at least 
nothing gets worse with that patch.
> Acked-by: Carl-Daniel Hailfinger

Thanks, r4362.
> Remember to use Luc's Ack as well if you commit.
>
> Regards,
> Carl-Daniel

Kind regards,
Harald
>