Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

On Tue, Apr 7, 2009 at 6:57 PM, Tejun Heo <tj@kernel.org> wrote:

> Hmmm... that means intx isn't set by default. =A0I'm not sure what is
> the right thing to do here. =A0I think it's something which should be
> handled by the PCI layer. =A0Oh well, maybe we should just revert the
> change and keep setting intx?

cc'ing linuxppc-dev

I really don't know what should be done.  It seems to make sense to
have the PCI layer enable interrupts.

This seems to be a powerpc-specific bug, but I don't know enough of
the PCI subsystem.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 941 bytes --]

On Tue, 2009-04-07 at 19:36 -0500, Timur Tabi wrote:
> On Tue, Apr 7, 2009 at 6:57 PM, Tejun Heo <tj@kernel.org> wrote:
> 
> > Hmmm... that means intx isn't set by default.  I'm not sure what is
> > the right thing to do here.  I think it's something which should be
> > handled by the PCI layer.  Oh well, maybe we should just revert the
> > change and keep setting intx?
> 
> cc'ing linuxppc-dev
> 
> I really don't know what should be done.  It seems to make sense to
> have the PCI layer enable interrupts.
> 
> This seems to be a powerpc-specific bug, but I don't know enough of
> the PCI subsystem.

Can you post some more details, or point us at a thread?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

On Tue, Apr 7, 2009 at 7:52 PM, Michael Ellerman <michael@ellerman.id.au> wrote:

> Can you post some more details, or point us at a thread?

http://marc.info/?t=123911652100006&r=1&w=2

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 943 bytes --]

On Tue, 2009-04-07 at 19:36 -0500, Timur Tabi wrote:
> On Tue, Apr 7, 2009 at 6:57 PM, Tejun Heo <tj@kernel.org> wrote:
> 
> > Hmmm... that means intx isn't set by default.  I'm not sure what is
> > the right thing to do here.  I think it's something which should be
> > handled by the PCI layer.  Oh well, maybe we should just revert the
> > change and keep setting intx?
> 
> cc'ing linuxppc-dev
> 
> I really don't know what should be done.  It seems to make sense to
> have the PCI layer enable interrupts.
> 
> This seems to be a powerpc-specific bug, but I don't know enough of
> the PCI subsystem.

Have you confirmed that INTX is disabled before that call?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Jeff Garzik]

Michael Ellerman wrote:
> On Tue, 2009-04-07 at 19:36 -0500, Timur Tabi wrote:
>> On Tue, Apr 7, 2009 at 6:57 PM, Tejun Heo <tj@kernel.org> wrote:
>>
>>> Hmmm... that means intx isn't set by default.  I'm not sure what is
>>> the right thing to do here.  I think it's something which should be
>>> handled by the PCI layer.  Oh well, maybe we should just revert the
>>> change and keep setting intx?
>> cc'ing linuxppc-dev
>>
>> I really don't know what should be done.  It seems to make sense to
>> have the PCI layer enable interrupts.
>>
>> This seems to be a powerpc-specific bug, but I don't know enough of
>> the PCI subsystem.
> 
> Have you confirmed that INTX is disabled before that call?

The symptoms definitely indicate such, as well as his reversing that commit.

My guess is that this is a ULI M1575-specific bug, and the PCI layer 
needs a quirk that knows this device does -not- disable INTX, when MSI 
is enabled.

But honestly, I never saw any harm in disabling INTX manually.  Indeed, 
I wrote the code that disabled INTX out of now-substantiated paranoia 
that some PCI devices would be too dumb to follow that particular MSI 
rule...  and the cost of twiddling INTX seemed quite low in comparison 
the potential problems created by the absence of INTX twiddling.

	Jeff

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

On Tue, Apr 7, 2009 at 9:09 PM, Michael Ellerman <michael@ellerman.id.au> wrote:

> Have you confirmed that INTX is disabled before that call?

How do I do that?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Tejun Heo]

Timur Tabi wrote:
> On Tue, Apr 7, 2009 at 9:09 PM, Michael Ellerman <michael@ellerman.id.au> wrote:
> 
>> Have you confirmed that INTX is disabled before that call?
> 
> How do I do that?

Running "lspci -nnvvvxxx" before loading the driver should be enough.

-- 
tejun

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

Tejun Heo wrote:

> Running "lspci -nnvvvxxx" before loading the driver should be enough.

That might be difficult.  My root file system is on my SATA drive.
It'll be a while before I can build an NFS rootfs.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Tejun Heo]

Timur Tabi wrote:
> Tejun Heo wrote:
> 
>> Running "lspci -nnvvvxxx" before loading the driver should be enough.
> 
> That might be difficult.  My root file system is on my SATA drive.
> It'll be a while before I can build an NFS rootfs.
> 

Yeah, right.  The following patch should do the trick then.

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 788bba2..b3f4df7 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -2606,6 +2606,12 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	if (!printed_version++)
 		dev_printk(KERN_DEBUG, &pdev->dev, "version " DRV_VERSION "\n");
 
+	{
+		u16 cmd;
+		pci_read_config_word(pdev, PCI_COMMAND, &cmd);
+		printk("XXX PCI_COMMAND=0x%x\n", cmd);
+	}
+
 	/* The AHCI driver can only drive the SATA ports, the PATA driver
 	   can drive them all so if both drivers are selected make sure
 	   AHCI stays out of the way */


-- 
tejun

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

Tejun Heo wrote:

> Yeah, right.  The following patch should do the trick then.

Thanks, I appreciate it.  I get this output:

XXX PCI_COMMAND=0x407

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Tejun Heo]

Timur Tabi wrote:
> Tejun Heo wrote:
> 
>> Yeah, right.  The following patch should do the trick then.
> 
> Thanks, I appreciate it.  I get this output:
> 
> XXX PCI_COMMAND=0x407
                    ^

Yeah, that's INTX disable.  Strange that the controller has the bit
set on boot.  I'm curious where this is from, the controller reset,
firmware or the PCI init (in decreasing likelihood).  Hmmm... for now,
I think it would be best to revert the original change.  Jeff, can you
please do that?

Thanks.

-- 
tejun

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
> Hmmm... for now,
> I think it would be best to revert the original change. =A0Jeff, can you
> please do that?

Actually, give me a few days before you do that.  A colleague gave me
some suggestions to debug this.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1181 bytes --]

On Wed, 2009-04-08 at 17:15 -0500, Timur Tabi wrote:
> On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
> > Hmmm... for now,
> > I think it would be best to revert the original change.  Jeff, can you
> > please do that?
> 
> Actually, give me a few days before you do that.  A colleague gave me
> some suggestions to debug this.

What device did you say it was? A "ULI M1575" ?

Is that this one?

DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575, hpcd_quirk_uli1575);                                                  

static void __devinit hpcd_quirk_uli1575(struct pci_dev *dev)                                                                
{
        u32 temp32;

        if (!machine_is(mpc86xx_hpcd))
                return;

        /* Disable INTx */
        pci_read_config_dword(dev, 0x48, &temp32);
        pci_write_config_dword(dev, 0x48, (temp32 | 1<<26));
..


cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Kumar Gala]

On Apr 8, 2009, at 6:53 PM, Michael Ellerman wrote:

> On Wed, 2009-04-08 at 17:15 -0500, Timur Tabi wrote:
>> On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
>>> Hmmm... for now,
>>> I think it would be best to revert the original change.  Jeff, can  
>>> you
>>> please do that?
>>
>> Actually, give me a few days before you do that.  A colleague gave me
>> some suggestions to debug this.
>
> What device did you say it was? A "ULI M1575" ?
>
> Is that this one?
>
> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575,  
> hpcd_quirk_uli1575);
>
> static void __devinit hpcd_quirk_uli1575(struct pci_dev *dev)
> {
>        u32 temp32;
>
>        if (!machine_is(mpc86xx_hpcd))
>                return;
>
>        /* Disable INTx */
>        pci_read_config_dword(dev, 0x48, &temp32);
>        pci_write_config_dword(dev, 0x48, (temp32 | 1<<26));
> ..

It is the odd thing is the board he's running on is a mpc86xx_hpcd so  
he shouldn't be hitting the code that actually disables INTx.

- k

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 1546 bytes --]

On Wed, 2009-04-08 at 23:23 -0500, Kumar Gala wrote:
> On Apr 8, 2009, at 6:53 PM, Michael Ellerman wrote:
> 
> > On Wed, 2009-04-08 at 17:15 -0500, Timur Tabi wrote:
> >> On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
> >>> Hmmm... for now,
> >>> I think it would be best to revert the original change.  Jeff, can  
> >>> you
> >>> please do that?
> >>
> >> Actually, give me a few days before you do that.  A colleague gave me
> >> some suggestions to debug this.
> >
> > What device did you say it was? A "ULI M1575" ?
> >
> > Is that this one?
> >
> > DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575,  
> > hpcd_quirk_uli1575);
> >
> > static void __devinit hpcd_quirk_uli1575(struct pci_dev *dev)
> > {
> >        u32 temp32;
> >
> >        if (!machine_is(mpc86xx_hpcd))
> >                return;
> >
> >        /* Disable INTx */
> >        pci_read_config_dword(dev, 0x48, &temp32);
> >        pci_write_config_dword(dev, 0x48, (temp32 | 1<<26));
> > ..
> 
> It is the odd thing is the board he's running on is a mpc86xx_hpcd so  
> he shouldn't be hitting the code that actually disables INTx.

Sorry Kumar that's not parsing :)

He is running an mpc86xx_hpcd, so he _should_ be hitting the code that
disables INTX?

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Kumar Gala]

On Apr 8, 2009, at 11:38 PM, Michael Ellerman wrote:

> On Wed, 2009-04-08 at 23:23 -0500, Kumar Gala wrote:
>> On Apr 8, 2009, at 6:53 PM, Michael Ellerman wrote:
>>
>>> On Wed, 2009-04-08 at 17:15 -0500, Timur Tabi wrote:
>>>> On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
>>>>> Hmmm... for now,
>>>>> I think it would be best to revert the original change.  Jeff, can
>>>>> you
>>>>> please do that?
>>>>
>>>> Actually, give me a few days before you do that.  A colleague  
>>>> gave me
>>>> some suggestions to debug this.
>>>
>>> What device did you say it was? A "ULI M1575" ?
>>>
>>> Is that this one?
>>>
>>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575,
>>> hpcd_quirk_uli1575);
>>>
>>> static void __devinit hpcd_quirk_uli1575(struct pci_dev *dev)
>>> {
>>>       u32 temp32;
>>>
>>>       if (!machine_is(mpc86xx_hpcd))
>>>               return;
>>>
>>>       /* Disable INTx */
>>>       pci_read_config_dword(dev, 0x48, &temp32);
>>>       pci_write_config_dword(dev, 0x48, (temp32 | 1<<26));
>>> ..
>>
>> It is the odd thing is the board he's running on is a mpc86xx_hpcd so
>> he shouldn't be hitting the code that actually disables INTx.
>
> Sorry Kumar that's not parsing :)
>
> He is running an mpc86xx_hpcd, so he _should_ be hitting the code that
> disables INTX?


duh.. reading the !machine_is.. code incorrectly.

- k

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Jeff Garzik]

Michael Ellerman wrote:
> On Wed, 2009-04-08 at 23:23 -0500, Kumar Gala wrote:
>> On Apr 8, 2009, at 6:53 PM, Michael Ellerman wrote:
>>
>>> On Wed, 2009-04-08 at 17:15 -0500, Timur Tabi wrote:
>>>> On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
>>>>> Hmmm... for now,
>>>>> I think it would be best to revert the original change.  Jeff, can  
>>>>> you
>>>>> please do that?
>>>> Actually, give me a few days before you do that.  A colleague gave me
>>>> some suggestions to debug this.
>>> What device did you say it was? A "ULI M1575" ?
>>>
>>> Is that this one?
>>>
>>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575,  
>>> hpcd_quirk_uli1575);
>>>
>>> static void __devinit hpcd_quirk_uli1575(struct pci_dev *dev)
>>> {
>>>        u32 temp32;
>>>
>>>        if (!machine_is(mpc86xx_hpcd))
>>>                return;
>>>
>>>        /* Disable INTx */
>>>        pci_read_config_dword(dev, 0x48, &temp32);
>>>        pci_write_config_dword(dev, 0x48, (temp32 | 1<<26));
>>> ..
>> It is the odd thing is the board he's running on is a mpc86xx_hpcd so  
>> he shouldn't be hitting the code that actually disables INTx.
> 
> Sorry Kumar that's not parsing :)
> 
> He is running an mpc86xx_hpcd, so he _should_ be hitting the code that
> disables INTX?

The reversed logic of the PCI bit itself also makes for confusing 
discusion.  In an attempt to be helpful, here is a restatement of what 
is happening:

1) Old 'ahci' used to clear PCI_COMMAND_INTX_DISABLE, thus ensuring INTX 
interrupts are enabled... if and only if MSI is unavailable.

2) Current 'ahci' no longer does this

3) As a result, Timur's 'ahci' is no longer receiving interrupts. 
Presumably this means that BOTH of the following conditions are true

	a) INTX is disabled
	b) MSI is not available

Today I am thinking we should either revert the libata commit 
(a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX 
for us at pci_enable_device() time, perhaps.

I lean towards the former, but maybe the platform folks prefer a third 
solution?

	Jeff

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

On Thu, Apr 9, 2009 at 12:32 AM, Jeff Garzik <jeff@garzik.org> wrote:
> 3) As a result, Timur's 'ahci' is no longer receiving interrupts. Presuma=
bly
> this means that BOTH of the following conditions are true
>
> =A0 =A0 =A0 =A0a) INTX is disabled
> =A0 =A0 =A0 =A0b) MSI is not available
>
> Today I am thinking we should either revert the libata commit
> (a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX f=
or
> us at pci_enable_device() time, perhaps.
>
> I lean towards the former, but maybe the platform folks prefer a third
> solution?

Well, I was hoping that the latest U-Boot would fix this problem, but
it doesn't.  Earlier U-Boot couldn't find my SATA drive, so I thought
that was a clue.  The latest U-Boot does find the SATA drive, but the
Linux driver still doesn't get interrupts.

--=20
Timur Tabi
Linux kernel developer at Freescale

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Michael Ellerman]

[-- Attachment #1: Type: text/plain, Size: 2994 bytes --]

On Thu, 2009-04-09 at 01:32 -0400, Jeff Garzik wrote:
> Michael Ellerman wrote:
> > On Wed, 2009-04-08 at 23:23 -0500, Kumar Gala wrote:
> >> On Apr 8, 2009, at 6:53 PM, Michael Ellerman wrote:
> >>
> >>> On Wed, 2009-04-08 at 17:15 -0500, Timur Tabi wrote:
> >>>> On Wed, Apr 8, 2009 at 4:31 PM, Tejun Heo <tj@kernel.org> wrote:
> >>>>> Hmmm... for now,
> >>>>> I think it would be best to revert the original change.  Jeff, can  
> >>>>> you
> >>>>> please do that?
> >>>> Actually, give me a few days before you do that.  A colleague gave me
> >>>> some suggestions to debug this.
> >>> What device did you say it was? A "ULI M1575" ?
> >>>
> >>> Is that this one?
> >>>
> >>> DECLARE_PCI_FIXUP_HEADER(PCI_VENDOR_ID_AL, 0x1575,  
> >>> hpcd_quirk_uli1575);
> >>>
> >>> static void __devinit hpcd_quirk_uli1575(struct pci_dev *dev)
> >>> {
> >>>        u32 temp32;
> >>>
> >>>        if (!machine_is(mpc86xx_hpcd))
> >>>                return;
> >>>
> >>>        /* Disable INTx */
> >>>        pci_read_config_dword(dev, 0x48, &temp32);
> >>>        pci_write_config_dword(dev, 0x48, (temp32 | 1<<26));
> >>> ..
> >> It is the odd thing is the board he's running on is a mpc86xx_hpcd so  
> >> he shouldn't be hitting the code that actually disables INTx.
> > 
> > Sorry Kumar that's not parsing :)
> > 
> > He is running an mpc86xx_hpcd, so he _should_ be hitting the code that
> > disables INTX?
> 
> The reversed logic of the PCI bit itself also makes for confusing 
> discusion.  In an attempt to be helpful, here is a restatement of what 
> is happening:
> 
> 1) Old 'ahci' used to clear PCI_COMMAND_INTX_DISABLE, thus ensuring INTX 
> interrupts are enabled... if and only if MSI is unavailable.
> 
> 2) Current 'ahci' no longer does this
> 
> 3) As a result, Timur's 'ahci' is no longer receiving interrupts. 
> Presumably this means that BOTH of the following conditions are true
> 
> 	a) INTX is disabled
> 	b) MSI is not available

Agreed.

> Today I am thinking we should either revert the libata commit 
> (a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX 
> for us at pci_enable_device() time, perhaps.
> 
> I lean towards the former, but maybe the platform folks prefer a third 
> solution?

But the device should have INTX enabled, the core shouldn't need to
twiddle it. If the device comes up with INTX disabled then it needs a
quirk to turn it on.

But in this case we have a quirk to turn INTX _off_, which just seems
odd. Can anyone say _why_ we need a quirk to turn INTX off? Looking at
the git history that quirk has been there ever since the MPC8610 HPCD
support went in (0e65bfe34c1000581746b9889d095241c4cf4a5c).

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: "ahci: drop intx manipulation on msi enable" breaks ULI M1575

[Timur Tabi]

On Thu, Apr 9, 2009 at 12:32 AM, Jeff Garzik <jeff@garzik.org> wrote:

> 3) As a result, Timur's 'ahci' is no longer receiving interrupts. Presuma=
bly
> this means that BOTH of the following conditions are true
>
> =A0 =A0 =A0 =A0a) INTX is disabled
> =A0 =A0 =A0 =A0b) MSI is not available
>
> Today I am thinking we should either revert the libata commit
> (a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX f=
or
> us at pci_enable_device() time, perhaps.

We (Freescale) have discussed and debugged this issue, and I'm 99%
certain that we have a board-specific fix, so there's no need to
revert the patch.

According to the original developer, he had to disable the INTX on the
board if SATA were disabled, otherwise some other problem occurred.
He noticed that the interrupt was re-enabled (presumably by the
pre-patch code in ahci.c), so he thought it would be okay to disable
it.

I've run some tests, and so far it appears that the problem does not
occur with the latest kernel (or the latest revision of the hardware).
 I need to run some more tests to be absolutely certain.  If those
tests pass, then I will post a patch that modifies
hpcd_quirk_uli5288().

So please, don't revert any patches.  And thanks to everyone for
helping me resolve this issue.

--=20
Timur Tabi
Linux kernel developer at Freescale