diff mbox

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

Message ID 49DD1375.8030400@kernel.org (mailing list archive)
State Rejected, archived
Headers show

Commit Message

Tejun Heo April 8, 2009, 9:13 p.m. UTC 
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.

Comments

Timur Tabi April 8, 2009, 9:17 p.m. UTC | #1 
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
Tejun Heo April 8, 2009, 9:31 p.m. UTC | #2 
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.
Timur Tabi April 8, 2009, 10:15 p.m. UTC | #3 
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.
Michael Ellerman April 8, 2009, 11:53 p.m. UTC | #4 
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
Kumar Gala April 9, 2009, 4:23 a.m. UTC | #5 
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
Michael Ellerman April 9, 2009, 4:38 a.m. UTC | #6 
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
Kumar Gala April 9, 2009, 5:18 a.m. UTC | #7 
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
Jeff Garzik April 9, 2009, 5:32 a.m. UTC | #8 
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
Timur Tabi April 9, 2009, 3:19 p.m. UTC | #9 
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. 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?

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.
Michael Ellerman April 13, 2009, 11:34 a.m. UTC | #10 
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
Timur Tabi April 16, 2009, 9:27 p.m. UTC | #11 
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. 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.

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.
diff mbox

Patch

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 */