Re: [PATCH] pata_of_platform: fix no irq handling
Anton Vorontsov wrote: When no irq specified the pata_of_platform fills the irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco [EMAIL PROTECTED] Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] applied to #tj-upstream (will soon be sent upstream) -- tejun ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Mon, 2008-10-13 at 15:56 +0900, Tejun Heo wrote: Anton Vorontsov wrote: When no irq specified the pata_of_platform fills the irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco [EMAIL PROTECTED] Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] applied to #tj-upstream (will soon be sent upstream) Hrm... I applied it to powerpc.git too :-) Hopefully the merge will sort it out ! Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Wed, Oct 08, 2008 at 11:59:59AM +0200, Geert Uytterhoeven wrote: On Wed, 8 Oct 2008, Alan Cox wrote: On Wed, 08 Oct 2008 09:40:54 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: Zero means no IRQ. Any platform with bits of code left over exposing IRQ 0 is already not supported by lots of driver code including libata. ...and must implement some kind of interrupt remapping crap just to work around this bogus design decision. I'll leave you to argue with Linus about that, but since that was the decision back in 2005 (for good C reasons) we can safely rely on it. `git grep NO_IRQ include arch/*/include' is still quite enlightening... In the case of PCI where IRQ is unsigned int, that's certainly bogus. The problem originates on platforms where a 1:1 mapping between vector and IRQ exists, where 0 is a valid value. This then needs to be remapped in to an IRQ cookie that has a non-0 value in order to be assigned to dev-irq. Despite whether this is bogus or not, there's not much to be done about it. Those of us with vectored IRQ platforms generally have enough sources that the use of IRQ-0 doesn't matter, and it's not worth the headache of setting up the remapping crap. As an example, on SH, IRQ-0 is mapped to vector 0x200. It is '0' for symbolic reasons only. It's just as easy to pad out irq_desc and treat it as an off-by-1 to work around all of code that assumes NO_IRQ == 0. There is enough code in the kernel today that makes the non-zero IRQ cookie assumption that platforms that do otherwise are simply broken. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Anton Vorontsov wrote: On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: There is a simple problem with the patch which is that an IRQ 0 can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is impossible) is more correct. This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227 Would this break any existing platforms? If so, can those be fixed together or does it become a much bigger problem that way? Thanks. -- tejun ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: Zero means no IRQ. Any platform with bits of code left over exposing IRQ 0 is already not supported by lots of driver code including libata. ...and must implement some kind of interrupt remapping crap just to work around this bogus design decision. -- David WoodhouseOpen Source Technology Centre [EMAIL PROTECTED] Intel Corporation ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Wed, 8 Oct 2008, Alan Cox wrote: On Wed, 08 Oct 2008 09:40:54 +0100 David Woodhouse [EMAIL PROTECTED] wrote: On Tue, 2008-10-07 at 10:37 +0100, Alan Cox wrote: Zero means no IRQ. Any platform with bits of code left over exposing IRQ 0 is already not supported by lots of driver code including libata. ...and must implement some kind of interrupt remapping crap just to work around this bogus design decision. I'll leave you to argue with Linus about that, but since that was the decision back in 2005 (for good C reasons) we can safely rely on it. `git grep NO_IRQ include arch/*/include' is still quite enlightening... With kind regards, Geert Uytterhoeven Software Architect Sony Techsoft Centre Europe The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium Phone:+32 (0)2 700 8453 Fax: +32 (0)2 700 8622 E-mail: [EMAIL PROTECTED] Internet: http://www.sony-europe.com/ A division of Sony Europe (Belgium) N.V. VAT BE 0413.825.160 · RPR Brussels Fortis · BIC GEBABEBB · IBAN BE41293037680010___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
I'll leave you to argue with Linus about that, but since that was the decision back in 2005 (for good C reasons) we can safely rely on it. `git grep NO_IRQ include arch/*/include' is still quite enlightening... Good guide to platform code we should delete really ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Tue, 2008-10-07 at 13:26 +0400, Anton Vorontsov wrote: On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote: Anton Vorontsov wrote: On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: There is a simple problem with the patch which is that an IRQ 0 can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is impossible) is more correct. This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227 Would this break any existing platforms? Nope. The driver is only available for PPC platforms. As time goes by one can change `depend on PPC_OF' to just `depends on OF', so that the driver will be also available for SPARC. And still it will work, because SPARC also understands VIRQ0 as invalid VIRQ. Yup, I agree. I'll pick it up in my next batch. Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Tue, Oct 07, 2008 at 10:30:59AM +0900, Tejun Heo wrote: Anton Vorontsov wrote: On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: There is a simple problem with the patch which is that an IRQ 0 can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is impossible) is more correct. This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227 Would this break any existing platforms? Nope. The driver is only available for PPC platforms. As time goes by one can change `depend on PPC_OF' to just `depends on OF', so that the driver will be also available for SPARC. And still it will work, because SPARC also understands VIRQ0 as invalid VIRQ. Thanks, -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Tejun Heo wrote: Anton Vorontsov wrote: On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: There is a simple problem with the patch which is that an IRQ 0 can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is impossible) is more correct. This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227 Would this break any existing platforms? If so, can those be fixed together or does it become a much bigger problem that way? Pata_of_platform stacks upon pata_platform. This patch fixes problem concerning definition of no irq without touch any other place. So far I can't see any new problem. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227 Would this break any existing platforms? If so, can those be fixed together or does it become a much bigger problem that way? Zero means no IRQ. Any platform with bits of code left over exposing IRQ 0 is already not supported by lots of driver code including libata. As IRQs are unsigned using -1 is asking for trouble ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
There is a simple problem with the patch which is that an IRQ 0 can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is impossible) is more correct. The problem is the check against an unsigned value for interrupts (is there any reason why you would need 4 billion interrupts possible instead of just 2 billion?) although I must say, the patch will work, and probably 99.999% of people will never see a problem with it :) -- Matt Sealey [EMAIL PROTECTED] Genesi, Manager, Developer Relations Anton Vorontsov wrote: When no irq specified the pata_of_platform fills the irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco [EMAIL PROTECTED] Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] --- Resending again... drivers/ata/pata_of_platform.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c index 408da30..1f18ad9 100644 --- a/drivers/ata/pata_of_platform.c +++ b/drivers/ata/pata_of_platform.c @@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct of_device *ofdev, ret = of_irq_to_resource(dn, 0, irq_res); if (ret == NO_IRQ) - irq_res.start = irq_res.end = -1; + irq_res.start = irq_res.end = 0; else irq_res.flags = 0; ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Mon, Oct 06, 2008 at 03:41:19PM -0500, Matt Sealey wrote: There is a simple problem with the patch which is that an IRQ 0 can and does actually exist on a bunch of platforms, at least to the best of my knowledge. Checking for -1 (which means for definite, no irq at all, because it is totally unambiguous, as a -1 IRQ numbering is impossible) is more correct. This was discussed years ago. http://lkml.org/lkml/2005/11/22/159 http://lkml.org/lkml/2005/11/22/227 -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Anton Vorontsov wrote: On Tue, Aug 12, 2008 at 06:18:42PM +0400, Sergei Shtylyov wrote: Anton Vorontsov wrote: 1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Another possibility is that you got the wrong interrupt number in the device-tree... Ben. The platform is the AMCC Sequoia board. We've built a little adapter to connect a compact flash card to the processor bus. I believe the interrupt selection in the device tree is correct, and I've checked over the u-boot settings for the IRQ line (active high, level sensitive). IDE IRQs are active-low. Only on the PCI and only in the native mode. Natively, the IDE INTRQ signal is active-high, rising edge triggering, as on ISA. You seem to have an invertor somewhere, if it's not a PCI chip... Ugh. Right you are, as always. I've just looked into mpc8349emitx schematics, there is indeed an inverter on the irq line. CF in True IDE mode is active-high, sorry. Ok, just to close this issue, my CF device is now working perfectly. Two problems, both my fault. 1) I thought the alternate registers used the same reg-shift and offset, so the alt_command and alt_status were at the wrong address, and 2) I was missing a pull-down on the IRQ line - the sequoia eval board has it in the schematic, but it is marked not populated. Thanks to all for your comments and help, Steve ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Benjamin Herrenschmidt wrote: 1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Another possibility is that you got the wrong interrupt number in the device-tree... Ben. The platform is the AMCC Sequoia board. We've built a little adapter to connect a compact flash card to the processor bus. I believe the interrupt selection in the device tree is correct, and I've checked over the u-boot settings for the IRQ line (active high, level sensitive). I've also tried edge-sensitive but it doesn't make a difference. When u-boot queries the CF card, we see the IRQ pulse as expected, but when the kernel runs, we see the IRQ go high and stay there, which the kernel naturally treats as a stuck interrupt. The other oddity is that we see a single diagnostic failure on startup: ata1.00: Drive reports diagnostics failure. This may indicate a drive ata1.00: fault or invalid emulation. Contact drive vendor for information. That is strange, because if we manually do the soft reset from u-boot, we see the ATA feature byte return 0x01, which means success. When the kernel does the soft reset, we see a 0x00, which means failure. You would think it is timing related, but a logic analyzer trace shows reasonable timing. We need to wire up a better test rig, so I don't want folks on this list to waste any time on it. I'll report back if I learn anything of general interest. With the interrupt disabled in the device tree, and ignoring the diagnostics failure, the drive actually works. I'm able to mount a filesystem, read files from it, etc. So, the drive is fully functional, just without using interrupts. Therefore, I believe most everything is correct - byte lanes, read/write signaling, timing, etc. Curious. Steve ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Tue, Aug 12, 2008 at 10:00:40AM -0400, Steven A. Falco wrote: Benjamin Herrenschmidt wrote: 1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Another possibility is that you got the wrong interrupt number in the device-tree... Ben. The platform is the AMCC Sequoia board. We've built a little adapter to connect a compact flash card to the processor bus. I believe the interrupt selection in the device tree is correct, and I've checked over the u-boot settings for the IRQ line (active high, level sensitive). IDE IRQs are active-low. -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Tuesday 12 August 2008, Anton Vorontsov wrote: Another possibility is that you got the wrong interrupt number in the device-tree... Ben. The platform is the AMCC Sequoia board. We've built a little adapter to connect a compact flash card to the processor bus. I believe the interrupt selection in the device tree is correct, and I've checked over the u-boot settings for the IRQ line (active high, level sensitive). IDE IRQs are active-low. IIRC, the CompactFlash interrupt is active-high. Best regards, Stefan ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Anton Vorontsov wrote: 1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Another possibility is that you got the wrong interrupt number in the device-tree... Ben. The platform is the AMCC Sequoia board. We've built a little adapter to connect a compact flash card to the processor bus. I believe the interrupt selection in the device tree is correct, and I've checked over the u-boot settings for the IRQ line (active high, level sensitive). IDE IRQs are active-low. Only on the PCI and only in the native mode. Natively, the IDE INTRQ signal is active-high, rising edge triggering, as on ISA. You seem to have an invertor somewhere, if it's not a PCI chip... WBR, Sergei ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Tue, Aug 12, 2008 at 06:18:42PM +0400, Sergei Shtylyov wrote: Anton Vorontsov wrote: 1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Another possibility is that you got the wrong interrupt number in the device-tree... Ben. The platform is the AMCC Sequoia board. We've built a little adapter to connect a compact flash card to the processor bus. I believe the interrupt selection in the device tree is correct, and I've checked over the u-boot settings for the IRQ line (active high, level sensitive). IDE IRQs are active-low. Only on the PCI and only in the native mode. Natively, the IDE INTRQ signal is active-high, rising edge triggering, as on ISA. You seem to have an invertor somewhere, if it's not a PCI chip... Ugh. Right you are, as always. I've just looked into mpc8349emitx schematics, there is indeed an inverter on the irq line. CF in True IDE mode is active-high, sorry. -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Anton Vorontsov wrote: When no irq specified, pata_of_platform fills irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco [EMAIL PROTECTED] Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] --- Thanks! Your fix is better - I didn't really like the -1 stuff. I found this bug because I had to disable the ATA interrupt on my system in order to get a compact-flash card to work. I am still trying to find out why the interrupt doesn't work for me. Here is part of the console log with the interrupt enabled: Uniform Multi-Platform E-IDE driver ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx Driver 'sd' needs updating - please use bus_type methods irq: irq_create_mapping(0xc0574900, 0x1b) irq: - using host @c0574900 irq: - obtained virq 32 scsi0 : pata_platform ata1: PATA max PIO4 mmio cmd 0x1c100 ctl 0x1c180 irq 32 irq 32: nobody cared (try booting with the irqpoll option) Call Trace: [cf83fcc0] [c0005a64] show_stack+0x44/0x1ac (unreliable) [cf83fd00] [c00489e4] __report_bad_irq+0x34/0xb8 [cf83fd20] [c0048cf0] note_interrupt+0x288/0x2d0 [cf83fd50] [c0049a94] handle_level_irq+0xac/0x114 [cf83fd60] [c0003df0] do_IRQ+0xa4/0xc8 [cf83fd70] [c000d60c] ret_from_except+0x0/0x18 [cf83fe30] [0020] 0x20 [cf83fe50] [c0003d48] do_softirq+0x54/0x58 [cf83fe60] [c00241c0] irq_exit+0x90/0x94 [cf83fe70] [c0003df4] do_IRQ+0xa8/0xc8 [cf83fe80] [c000d60c] ret_from_except+0x0/0x18 [cf83ff40] [c01b2cc0] ata_pio_task+0x48/0x104 [cf83ff60] [c00307a0] run_workqueue+0xb8/0x148 [cf83ff90] [c0030d54] worker_thread+0x70/0xd0 [cf83ffd0] [c0034788] kthread+0x48/0x84 [cf83fff0] [c000cd6c] kernel_thread+0x44/0x60 handlers: [c01b2d7c] (ata_sff_interrupt+0x0/0x234) Disabling IRQ #32 So it looks like the ATA handler was attached - not sure yet why I got the nobody cared message. Also, the system hangs. If I find something, I'll post it. Steve ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote:
When no irq specified, pata_of_platform fills irq_res with -1,
which is wrong to do for two reasons:
1. By definition, 'no irq' should be IRQ 0, not some negative integer;
interesting, IRQ 0 is actually valid on some ARM systems.
2. pata_platform checks for irq_res.start 0, but since irq_res.start
is unsigned type, the check will be true for `-1'.
Reported-by: Steven A. Falco [EMAIL PROTECTED]
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
---
On Mon, Aug 11, 2008 at 10:48:50AM -0400, Steven A. Falco wrote:
I think there is a bug in the communications between pata_of_platform
and pata_platform. I will refer to the master branch of the DENX git
tree, which is roughly v2.6.26.1 at this time. I am using a Sequoia
board with a PPC440EPx.
In pata_of_platform, we have:
ret = of_irq_to_resource(dn, 0, irq_res);
if (ret == NO_IRQ)
irq_res.start = irq_res.end = -1;
so if there is no interrupt defined, then start and end are -1.
However, __pata_platform_probe has:
if (irq_res irq_res-start 0) {
irq = irq_res-start;
irq_flags = irq_res-flags;
}
You might think that the (irq_res-start 0) test will fail, as it
should in this no-irq case. But, start is a u64, so the -1 actually
looks like a large positive number in the comparison. So,
__pata_platform_probe attempts to use an interrupt when there isn't one.
I think the fix would be to change __pata_platform_probe to:
if (irq_res irq_res-start != -1) {
but that might have other unintended consequences, so I'll defer to
whomever knows more about the intent of this code.
Something like this patch should work. Thanks for noticing!
drivers/ata/pata_of_platform.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct
of_device *ofdev,
ret = of_irq_to_resource(dn, 0, irq_res);
if (ret == NO_IRQ)
- irq_res.start = irq_res.end = -1;
+ irq_res.start = irq_res.end = 0;
else
irq_res.flags = 0;
--
Ben
Q: What's a light-year?
A: One-third less calories than a regular year.
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
Ben Dooks wrote:
On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote:
When no irq specified, pata_of_platform fills irq_res with -1,
which is wrong to do for two reasons:
1. By definition, 'no irq' should be IRQ 0, not some negative integer;
interesting, IRQ 0 is actually valid on some ARM systems.
It is here too, but I believe most of the code uses a virtualized irq
number, so physical IRQ 0 would presumably get mapped to a non-zero
virtual one.
2. pata_platform checks for irq_res.start 0, but since irq_res.start
is unsigned type, the check will be true for `-1'.
Reported-by: Steven A. Falco [EMAIL PROTECTED]
Signed-off-by: Anton Vorontsov [EMAIL PROTECTED]
---
On Mon, Aug 11, 2008 at 10:48:50AM -0400, Steven A. Falco wrote:
I think there is a bug in the communications between pata_of_platform
and pata_platform. I will refer to the master branch of the DENX git
tree, which is roughly v2.6.26.1 at this time. I am using a Sequoia
board with a PPC440EPx.
In pata_of_platform, we have:
ret = of_irq_to_resource(dn, 0, irq_res);
if (ret == NO_IRQ)
irq_res.start = irq_res.end = -1;
so if there is no interrupt defined, then start and end are -1.
However, __pata_platform_probe has:
if (irq_res irq_res-start 0) {
irq = irq_res-start;
irq_flags = irq_res-flags;
}
You might think that the (irq_res-start 0) test will fail, as it
should in this no-irq case. But, start is a u64, so the -1 actually
looks like a large positive number in the comparison. So,
__pata_platform_probe attempts to use an interrupt when there isn't one.
I think the fix would be to change __pata_platform_probe to:
if (irq_res irq_res-start != -1) {
but that might have other unintended consequences, so I'll defer to
whomever knows more about the intent of this code.
Something like this patch should work. Thanks for noticing!
drivers/ata/pata_of_platform.c |2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/ata/pata_of_platform.c b/drivers/ata/pata_of_platform.c
index 408da30..1f18ad9 100644
--- a/drivers/ata/pata_of_platform.c
+++ b/drivers/ata/pata_of_platform.c
@@ -52,7 +52,7 @@ static int __devinit pata_of_platform_probe(struct
of_device *ofdev,
ret = of_irq_to_resource(dn, 0, irq_res);
if (ret == NO_IRQ)
-irq_res.start = irq_res.end = -1;
+irq_res.start = irq_res.end = 0;
else
irq_res.flags = 0;
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Mon, 11 Aug 2008 17:36:48 +0100 Ben Dooks [EMAIL PROTECTED] wrote: On Mon, Aug 11, 2008 at 07:19:13PM +0400, Anton Vorontsov wrote: When no irq specified, pata_of_platform fills irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; interesting, IRQ 0 is actually valid on some ARM systems. Not as far as Linux is concerned. It's expected that any IRQ that happens to be physical IRQ 0 whatever that means is remapped by the arch code unless not visible outside the arch. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Mon, 11 Aug 2008 19:19:13 +0400 Anton Vorontsov [EMAIL PROTECTED] wrote: When no irq specified, pata_of_platform fills irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco [EMAIL PROTECTED] Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] Acked-by: Alan Cox [EMAIL PROTECTED] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
On Mon, Aug 11, 2008 at 12:23:10PM -0400, Steven A. Falco wrote: Anton Vorontsov wrote: When no irq specified, pata_of_platform fills irq_res with -1, which is wrong to do for two reasons: 1. By definition, 'no irq' should be IRQ 0, not some negative integer; 2. pata_platform checks for irq_res.start 0, but since irq_res.start is unsigned type, the check will be true for `-1'. Reported-by: Steven A. Falco [EMAIL PROTECTED] Signed-off-by: Anton Vorontsov [EMAIL PROTECTED] --- Thanks! Your fix is better - I didn't really like the -1 stuff. I found this bug because I had to disable the ATA interrupt on my system in order to get a compact-flash card to work. I am still trying to find out why the interrupt doesn't work for me. Here is part of the console log with the interrupt enabled: Uniform Multi-Platform E-IDE driver ide: Assuming 33MHz system bus speed for PIO modes; override with idebus=xx Driver 'sd' needs updating - please use bus_type methods irq: irq_create_mapping(0xc0574900, 0x1b) irq: - using host @c0574900 irq: - obtained virq 32 scsi0 : pata_platform ata1: PATA max PIO4 mmio cmd 0x1c100 ctl 0x1c180 irq 32 irq 32: nobody cared (try booting with the irqpoll option) Call Trace: [cf83fcc0] [c0005a64] show_stack+0x44/0x1ac (unreliable) [cf83fd00] [c00489e4] __report_bad_irq+0x34/0xb8 [cf83fd20] [c0048cf0] note_interrupt+0x288/0x2d0 [cf83fd50] [c0049a94] handle_level_irq+0xac/0x114 [cf83fd60] [c0003df0] do_IRQ+0xa4/0xc8 [cf83fd70] [c000d60c] ret_from_except+0x0/0x18 [cf83fe30] [0020] 0x20 [cf83fe50] [c0003d48] do_softirq+0x54/0x58 [cf83fe60] [c00241c0] irq_exit+0x90/0x94 [cf83fe70] [c0003df4] do_IRQ+0xa8/0xc8 [cf83fe80] [c000d60c] ret_from_except+0x0/0x18 [cf83ff40] [c01b2cc0] ata_pio_task+0x48/0x104 [cf83ff60] [c00307a0] run_workqueue+0xb8/0x148 [cf83ff90] [c0030d54] worker_thread+0x70/0xd0 [cf83ffd0] [c0034788] kthread+0x48/0x84 [cf83fff0] [c000cd6c] kernel_thread+0x44/0x60 handlers: [c01b2d7c] (ata_sff_interrupt+0x0/0x234) Disabling IRQ #32 So it looks like the ATA handler was attached - not sure yet why I got the nobody cared message. Nobody cared means that ata_sff_interrupt handler didn't notice any IDE events, and returned zero value. This could mean that 1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Just asking because recently Sam Sparks reported that he is observing similar issue on MPC8349E-mITX-based boards, which I can't not reproduce on my board though: http://www.nabble.com/Compact-Flash-on-8349mITX-td18754330.html http://www.nabble.com/Compact-flash-on-mpc8349eITX-td1824.html -- Anton Vorontsov email: [EMAIL PROTECTED] irc://irc.freenode.net/bd2 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH] pata_of_platform: fix no irq handling
1. IDE status read does not work. (But am I understand correctly that IDE works well if IRQ is unspecified? Then this is hardly an issue.) 2. IDE interrupt comes when it should not. I'd recommend to use oscilloscope to find out what is happening there, that is, if the drive actually deasserts its irq line after status read. If so, than this could be a PIC problem. What is the platform on which you're observing the issue, btw? Another possibility is that you got the wrong interrupt number in the device-tree... Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
