"ahci: drop intx manipulation on msi enable" breaks ULI M1575
Tejun,
Your patch
Quoted 11 lines
ahci: drop intx manipulation on msi enable
There's no need to turn off intx explicitly on msi enable. This is automatically handled by pci. Drop it.
This might be needed on machines if the BIOS turns intx off during boot. However, there's no evidence of such behavior for ahci and the only such case seems to be ICH5 PATA according to
ata_piix. Also, given the way ahci operates, it's highly unlikely BIOS ever disables IRQ for the controller. However, as this change has slight possibility of introducing failure, please schedule it for #upstream.
breaks SATA support on a Freescale MPC8610 HPCD (PowerPC) reference board. When I apply the patch, I get this result during boot:
ata1: SATA max UDMA/133 irq_stat 0x00400040, connection status changed irq 19
ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: qc timeout (cmd 0xec)
ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 300) ata1.00: qc timeout (cmd 0xec)
ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
ata1: limiting SATA link speed to 1.5 Gbps
ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310) ata1.00: qc timeout (cmd 0xec)
ata1.00: failed to IDENTIFY (I/O error, err_mask=0x4)
ata1: SATA link up 1.5 Gbps (SStatus 113 SControl 310)
This board uses a ULI M1575 for SATA support.
Unfortunately, I don't know enough about SATA or PCI interrupts to understand why you made the change or why it breaks on my board.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Hello,
Quoted 14 lines · Timur Tabi
Quoted 11 lines
ahci: drop intx manipulation on msi enable
There's no need to turn off intx explicitly on msi enable. This is automatically handled by pci. Drop it.
This might be needed on machines if the BIOS turns intx off during boot. However, there's no evidence of such behavior for ahci and the only such case seems to be ICH5 PATA according to
ata_piix. Also, given the way ahci operates, it's highly unlikely BIOS ever disables IRQ for the controller. However, as this change has slight possibility of introducing failure, please schedule it for #upstream.breaks SATA support on a Freescale MPC8610 HPCD (PowerPC) reference board. When I apply the patch, I get this result during boot:
...
Quoted 4 lines
This board uses a ULI M1575 for SATA support.
Unfortunately, I don't know enough about SATA or PCI interrupts to understand why you made the change or why it breaks on my board.
cc'ing Jeff.
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?
Thanks.
Signature · 1 line
tejun
Quoted 4 lines · Tejun Heo
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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 7 lines · Timur Tabi
Quoted 4 lines · Tejun Heo
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?
Quoted 7 lines
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
Signature · 8 lines
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
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Quoted 1 line · Michael Ellerman
Can you post some more details, or point us at a thread?
http://marc.info/?t=123911652100006&r=1&w=2
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 7 lines · Timur Tabi
Quoted 4 lines · Tejun Heo
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?
Quoted 7 lines
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
Signature · 8 lines
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
Quoted 9 lines · Michael Ellerman
Quoted 8 lines · Timur Tabi
Quoted 4 lines · Tejun Heo
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
Quoted 7 lines
Quoted 5 lines
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
Quoted 1 line · Michael Ellerman
Have you confirmed that INTX is disabled before that call?
How do I do that?
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 5 lines · Timur Tabi
Quoted 1 line · Michael Ellerman
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.
Signature · 1 line
tejun
Quoted 1 line · Tejun Heo
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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 7 lines · Timur Tabi
Quoted 1 line · Tejun Heo
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.
drivers/ata/ahci.c +6 -0
@@ -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 */
Signature · 1 line
tejun
Quoted 1 line · Tejun Heo
Yeah, right. The following patch should do the trick then.
Thanks, I appreciate it. I get this output:
XXX PCI_COMMAND=0x407
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 7 lines · Timur Tabi
Quoted 1 line · Tejun Heo
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.
Signature · 1 line
tejun
Quoted 3 lines · Tejun Heo
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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 7 lines · Timur Tabi
Quoted 3 lines · Tejun Heo
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
Signature · 8 lines
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
Quoted 25 lines · Michael Ellerman
Quoted 7 lines · Timur Tabi
Quoted 3 lines · Tejun Heo
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
__devinithpcd_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));
Quoted 1 line
..
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
Quoted 26 lines · Kumar Gala
Quoted 24 lines · Michael Ellerman
Quoted 8 lines · Timur Tabi
Quoted 4 lines · Tejun Heo
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
__devinithpcd_quirk_uli1575(struct pci_dev *dev){ u32 temp32; if (!machine_is(mpc86xx_hpcd)) return;
Quoted 7 lines
Quoted 4 lines
/* 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_hpcdso 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
Signature · 8 lines
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
_______________________________________________
Linuxppc-dev mailing list
[email protected]
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Quoted 32 lines · Michael Ellerman
Quoted 31 lines · Kumar Gala
Quoted 28 lines · Michael Ellerman
Quoted 8 lines · Timur Tabi
Quoted 4 lines · Tejun Heo
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
__devinithpcd_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));..
Quoted 7 lines
Quoted 2 lines
It is the odd thing is the board he's running on is a
mpc86xx_hpcdso 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
Quoted 27 lines · Michael Ellerman
Quoted 26 lines · Kumar Gala
Quoted 24 lines · Michael Ellerman
Quoted 6 lines · Timur Tabi
Quoted 3 lines · Tejun Heo
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
__devinithpcd_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));..
Quoted 6 lines
Quoted 1 line
It is the odd thing is the board he's running on is a
mpc86xx_hpcdso 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
Quoted 6 lines · Jeff Garzik
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
Quoted 6 lines
Today I am thinking we should either revert the libata commit (
a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX for us atpci_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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale
Quoted 45 lines · Jeff Garzik
Quoted 29 lines · Michael Ellerman
Quoted 28 lines · Kumar Gala
Quoted 26 lines · Michael Ellerman
Quoted 7 lines · Timur Tabi
Quoted 4 lines · Tejun Heo
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
__devinithpcd_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));..
Quoted 7 lines
Quoted 2 lines
It is the odd thing is the board he's running on is a
mpc86xx_hpcdso 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.
Quoted 7 lines
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.
Quoted 6 lines
Today I am thinking we should either revert the libata commit (
a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX for us atpci_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
Signature · 8 lines
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
Quoted 9 lines · Jeff Garzik
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 availableToday I am thinking we should either revert the libata commit (
a5bfc4714b3f01365aef89a92673f2ceb1ccf246), or poke PCI to twiddle INTX for us atpci_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.
Signature · 2 lines
Timur Tabi Linux kernel developer at Freescale