Message ID | 49DD1375.8030400@kernel.org (mailing list archive) |
---|---|
State | Rejected, archived |
Headers | show |
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 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.
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.
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
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
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
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
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
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.
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
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 --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 */