Signed-off-by: Al Viro <[email protected]>
---
include/asm-frv/libata-portmap.h | 1 +
1 files changed, 1 insertions(+), 0 deletions(-)
diff --git a/include/asm-frv/libata-portmap.h b/include/asm-frv/libata-portmap.h
new file mode 100644
index 0000000..75484ef
--- /dev/null
+++ b/include/asm-frv/libata-portmap.h
@@ -0,0 +1 @@
+#include <asm-generic/libata-portmap.h>
--
1.4.2.GIT
Al Viro <[email protected]> wrote:
> +++ b/include/asm-frv/libata-portmap.h
> @@ -0,0 +1 @@
> +#include <asm-generic/libata-portmap.h>
NAK... These settings are totally meaningless on FRV.
David
Ar Llu, 2006-09-25 am 11:44 +0100, ysgrifennodd David Howells:
> Al Viro <[email protected]> wrote:
>
> > +++ b/include/asm-frv/libata-portmap.h
> > @@ -0,0 +1 @@
> > +#include <asm-generic/libata-portmap.h>
>
> NAK... These settings are totally meaningless on FRV.
You have no PCI bus ?
On Mon, Sep 25, 2006 at 12:26:08PM +0100, Alan Cox wrote:
> Ar Llu, 2006-09-25 am 11:44 +0100, ysgrifennodd David Howells:
> > Al Viro <[email protected]> wrote:
> >
> > > +++ b/include/asm-frv/libata-portmap.h
> > > @@ -0,0 +1 @@
> > > +#include <asm-generic/libata-portmap.h>
> >
> > NAK... These settings are totally meaningless on FRV.
>
> You have no PCI bus ?
Note that if you don't provide an asm/libata-portmap.h file, you can't
build libata at the moment - linux/libata.h requires this file to be
present.
--
Russell King
Linux kernel 2.6 ARM Linux - http://www.arm.linux.org.uk/
maintainer of: 2.6 Serial core
Russell King <[email protected]> wrote:
> Note that if you don't provide an asm/libata-portmap.h file, you can't
> build libata at the moment - linux/libata.h requires this file to be
> present.
Yes, but it should be empty as there is no ISA bus.
David
Alan Cox <[email protected]> wrote:
> > NAK... These settings are totally meaningless on FRV.
>
> You have no PCI bus ?
I do, but I don't have an ISA bus, and the stuff in asm-generic is worse than
useless. #if'ing out all the legacy-handling code that uses this stuff seems
to work fine.
These are all legacy ISA settings, and not applicable to FRV:
#define ATA_PRIMARY_CMD 0x1F0
#define ATA_PRIMARY_CTL 0x3F6
#define ATA_PRIMARY_IRQ 14
#define ATA_SECONDARY_CMD 0x170
#define ATA_SECONDARY_CTL 0x376
#define ATA_SECONDARY_IRQ 15
Note that the ata_pci_init_legacy_port() explicitly states the IRQ numbers as
14 and 15 without reference to the macros and so is bad, eg:
probe_ent->irq = 14;
The attached patch makes it possible to dispense with the settings completely.
David
---
[PATCH] FRV: Make libata build on FRV
Make FRV build with libata enabled. This is done by making the legacy ISA
interface support conditional on the definition of the legacy ISA port
settings. If there's no ISA bus, we shouldn't even attempt to pretend that
there is.
Signed-Off-By: David Howells <[email protected]>
---
drivers/ata/libata-core.c | 2 ++
drivers/ata/libata-sff.c | 10 ++++++++++
include/asm-frv/libata-portmap.h | 17 ++++++++++++++++-
3 files changed, 28 insertions(+), 1 deletions(-)
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 753b015..2afa510 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -5714,6 +5714,7 @@ void ata_host_remove(struct ata_host *ho
ata_scsi_release(ap->scsi_host);
+#ifdef ATA_PRIMARY_CMD
if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
struct ata_ioports *ioaddr = &ap->ioaddr;
@@ -5723,6 +5724,7 @@ void ata_host_remove(struct ata_host *ho
else if (ioaddr->cmd_addr == ATA_SECONDARY_CMD)
release_region(ATA_SECONDARY_CMD, 8);
}
+#endif
scsi_host_put(ap->scsi_host);
}
diff --git a/drivers/ata/libata-sff.c b/drivers/ata/libata-sff.c
index 688bb55..63fdf73 100644
--- a/drivers/ata/libata-sff.c
+++ b/drivers/ata/libata-sff.c
@@ -867,6 +867,7 @@ ata_pci_init_native_mode(struct pci_dev
}
+#ifdef ATA_PRIMARY_CMD
static struct ata_probe_ent *ata_pci_init_legacy_port(struct pci_dev *pdev,
struct ata_port_info **port, int port_mask)
{
@@ -914,6 +915,7 @@ static struct ata_probe_ent *ata_pci_ini
return probe_ent;
}
+#endif
/**
@@ -993,6 +995,7 @@ int ata_pci_init_one (struct pci_dev *pd
goto err_out;
}
+#ifdef ATA_PRIMARY_CMD
if (legacy_mode) {
if (!request_region(ATA_PRIMARY_CMD, 8, "libata")) {
struct resource *conflict, res;
@@ -1032,6 +1035,7 @@ int ata_pci_init_one (struct pci_dev *pd
} else
legacy_mode |= ATA_PORT_SECONDARY;
}
+#endif
/* we have legacy mode, but all ports are unavailable */
if (legacy_mode == (1 << 3)) {
@@ -1047,14 +1051,18 @@ int ata_pci_init_one (struct pci_dev *pd
if (rc)
goto err_out_regions;
+#ifdef ATA_PRIMARY_CMD
if (legacy_mode) {
probe_ent = ata_pci_init_legacy_port(pdev, port, legacy_mode);
} else {
+#endif
if (n_ports == 2)
probe_ent = ata_pci_init_native_mode(pdev, port, ATA_PORT_PRIMARY | ATA_PORT_SECONDARY);
else
probe_ent = ata_pci_init_native_mode(pdev, port, ATA_PORT_PRIMARY);
+#ifdef ATA_PRIMARY_CMD
}
+#endif
if (!probe_ent) {
rc = -ENOMEM;
goto err_out_regions;
@@ -1070,10 +1078,12 @@ int ata_pci_init_one (struct pci_dev *pd
return 0;
err_out_regions:
+#ifdef ATA_PRIMARY_CMD
if (legacy_mode & ATA_PORT_PRIMARY)
release_region(ATA_PRIMARY_CMD, 8);
if (legacy_mode & ATA_PORT_SECONDARY)
release_region(ATA_SECONDARY_CMD, 8);
+#endif
pci_release_regions(pdev);
err_out:
if (disable_dev_on_err)
diff --git a/include/asm-frv/libata-portmap.h b/include/asm-frv/libata-portmap.h
index 75484ef..2403f0b 100644
--- a/include/asm-frv/libata-portmap.h
+++ b/include/asm-frv/libata-portmap.h
@@ -1 +1,16 @@
-#include <asm-generic/libata-portmap.h>
+/* ISA bus mappings for legacy ATA ports - which don't exist on FRV
+ *
+ * Copyright (C) 2006 Red Hat, Inc. All Rights Reserved.
+ * Written by David Howells ([email protected])
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * as published by the Free Software Foundation; either version
+ * 2 of the License, or (at your option) any later version.
+ */
+
+#ifndef _ASM_LIBATA_PORTMAP_H
+#define _ASM_LIBATA_PORTMAP_H
+
+
+#endif /* _ASM_LIBATA_PORTMAP_H */
Ar Llu, 2006-09-25 am 12:27 +0100, ysgrifennodd David Howells:
> Alan Cox <[email protected]> wrote:
> These are all legacy ISA settings, and not applicable to FRV:
>
> #define ATA_PRIMARY_CMD 0x1F0
> #define ATA_PRIMARY_CTL 0x3F6
> #define ATA_PRIMARY_IRQ 14
>
> #define ATA_SECONDARY_CMD 0x170
> #define ATA_SECONDARY_CTL 0x376
> #define ATA_SECONDARY_IRQ 15
Wrong these are PCI settings. Please read the PCI specifications. In
particular the handling of non-native mode IDE storage class devices on
a PCI bus. For the IRQ mapping of the non-native ports consult your
bridge documentation.
> Note that the ata_pci_init_legacy_port() explicitly states the IRQ numbers as
> 14 and 15 without reference to the macros and so is bad, eg:
>
> probe_ent->irq = 14;
That is indeed a bug
> Make FRV build with libata enabled. This is done by making the legacy ISA
> interface support conditional on the definition of the legacy ISA port
> settings. If there's no ISA bus, we shouldn't even attempt to pretend that
> there is.
>
> Signed-Off-By: David Howells <[email protected]>
Nacked-by: Alan Cox <[email protected]>
Alan Cox <[email protected]> wrote:
> Wrong these are PCI settings. Please read the PCI specifications. In
> particular the handling of non-native mode IDE storage class devices on
> a PCI bus. For the IRQ mapping of the non-native ports consult your
> bridge documentation.
Even if that is the case, they are all invalid/incorrect, and so Al Viro's
patch is _still_ NAK'd.
David
On Mon, Sep 25, 2006 at 01:18:04PM +0100, David Howells wrote:
> Alan Cox <[email protected]> wrote:
>
> > Wrong these are PCI settings. Please read the PCI specifications. In
> > particular the handling of non-native mode IDE storage class devices on
> > a PCI bus. For the IRQ mapping of the non-native ports consult your
> > bridge documentation.
>
> Even if that is the case, they are all invalid/incorrect, and so Al Viro's
> patch is _still_ NAK'd.
Fine by me. In that case we need to add
depends on !FRV || BROKEN
to drivers/ata/Kconfig and be done with that. BTW, empty libata-portmap.h
is equivalent to absent one - it still won't build.
Al Viro <[email protected]> wrote:
> Fine by me. In that case we need to add
> depends on !FRV || BROKEN
My point is that all the numbers are invalid or incorrect. They will cause
the arch to oops, and so need completely replacing. So that patch you added
is incorrect and should not include asm-generic/libata-portmap.h as nothing in
there is correct on this arch. So your patch should *not* be applied, but
should instead be replaced.
> to drivers/ata/Kconfig and be done with that. BTW, empty libata-portmap.h
> is equivalent to absent one - it still won't build.
Why does the arch have to supply those numbers? What's wrong with my
suggested patch? According to code in libata, these are _legacy_ access
methods, and on FRV they aren't currently required, so why can't I dispense
with them if they're not needed? Doing that not only skips legacy accesses
for ISA compatibility, it also reduces the code size by not actually emitting
the code for the accesses, thus making the kernel smaller...
David
Ar Llu, 2006-09-25 am 15:20 +0100, ysgrifennodd Al Viro:
> Fine by me. In that case we need to add
> depends on !FRV || BROKEN
> to drivers/ata/Kconfig and be done with that. BTW, empty libata-portmap.h
> is equivalent to absent one - it still won't build.
>From every public piece of info I can find and from looking at the FRV
tree your changes are correct for the ports Al. I can't find any info on
how legacy IRQ routing is done on FRV systems but if it is not then set
the IRQ values to zero and maybe Dave will stop complaining.
Alan
Ar Llu, 2006-09-25 am 15:39 +0100, ysgrifennodd David Howells:
> Why does the arch have to supply those numbers? What's wrong with my
> suggested patch? According to code in libata, these are _legacy_ access
> methods, and on FRV they aren't currently required, so why can't I dispense
"legacy, legacy, legacy" "wont wont wont"
The ports in question are PCI values. They come from the PCI
specifications and apply to any device with PCI bus, unless it has
special mappings. The same logic you are whining about is already partly
handled in the generic pci quirks code, and in time will end up with the
I/O port value fixups there anyway.
See quirk_ide_bases in drivers/pci/quirks.c
Go read the specifications and stop whining about legacy ports and ISA
bus for things that are not.
Ack Al Viro's changes but with IRQ set to zero.
Alan
Alan Cox <[email protected]> wrote:
> > Fine by me. In that case we need to add
> > depends on !FRV || BROKEN
> > to drivers/ata/Kconfig and be done with that. BTW, empty libata-portmap.h
> > is equivalent to absent one - it still won't build.
>
> From every public piece of info I can find and from looking at the FRV
> tree your changes are correct for the ports Al. I can't find any info on
> how legacy IRQ routing is done on FRV systems but if it is not then set
> the IRQ values to zero and maybe Dave will stop complaining.
Sigh.
On FRV, inX() and outX() take fully qualified memory-space addresses, exactly
as readX() and writeX() (in/out just wrap readX/writeX). This is because:
(1) The FRV has a limited number of static mappings, and these have to specify
_all_ access windows to I/O, RAM, ROM, etc. The FRV arch uses a single
mapping to handle *all* I/O (which happens to be through the region from
0xE0000000 to 0xFFFFFFFF) thus allowing it to use the remaining mappings
for other purposes.
(2) inX() and outX() would have to adjust the addresses to otherwise make
them appear PC compatible. Making in() and out() just pass the addresses
straight through means I don't have to do any calculation on the address
in order to use it.
inb(0x1F0) will, for example, oops because there's no mapping for the bottom
virtual megabyte to anywhere, otherwise NULL pointer detection would not be
possible.
Don't forget, also, that things like FRV systems generally _won't_ have
pluggable PCI buses, and so any devices attached to it will be known in
advance, and generalisations can be waived.
David
Alan Cox <[email protected]> wrote:
> Ack Al Viro's changes but with IRQ set to zero.
The PCI bus has special mappings. You may not address it with those numbers.
Any of those numbers. Al's patch is 100% incorrect. Sorry.
David
On Mon, Sep 25, 2006 at 05:04:10PM +0100, David Howells wrote:
> Alan Cox <[email protected]> wrote:
>
> > Ack Al Viro's changes but with IRQ set to zero.
>
> The PCI bus has special mappings. You may not address it with those numbers.
> Any of those numbers. Al's patch is 100% incorrect. Sorry.
Oh, for fuck sake... 2.6.18 drivers/scsi/libata-core.c:
if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
struct ata_ioports *ioaddr = &ap->ioaddr;
if (ioaddr->cmd_addr == 0x1f0)
release_region(0x1f0, 8);
else if (ioaddr->cmd_addr == 0x170)
release_region(0x170, 8);
}
current drivers/ata/libata-core.c:
if ((ap->flags & ATA_FLAG_NO_LEGACY) == 0) {
struct ata_ioports *ioaddr = &ap->ioaddr;
/* FIXME: Add -ac IDE pci mods to remove these special cases */
if (ioaddr->cmd_addr == ATA_PRIMARY_CMD)
release_region(ATA_PRIMARY_CMD, 8);
else if (ioaddr->cmd_addr == ATA_SECONDARY_CMD)
release_region(ATA_SECONDARY_CMD, 8);
}
Patch in question restores the situation prior to libata merge. That's
what FRV had been doing all along if SATA had been enabled. No more,
mo less.
Now, if you want to change that behaviour, more power to you. But that's
a separate patch, obviously, and all issues related to that exist in vanilla
2.6.18 just as in the current tree.
On Mon, 2006-09-25 at 16:46 +0100, Alan Cox wrote:
> Ar Llu, 2006-09-25 am 15:39 +0100, ysgrifennodd David Howells:
> > Why does the arch have to supply those numbers? What's wrong with my
> > suggested patch? According to code in libata, these are _legacy_ access
> > methods, and on FRV they aren't currently required, so why can't I dispense
>
> "legacy, legacy, legacy" "wont wont wont"
>
> The ports in question are PCI values. They come from the PCI
> specifications and apply to any device with PCI bus, unless it has
> special mappings. The same logic you are whining about is already partly
> handled in the generic pci quirks code, and in time will end up with the
> I/O port value fixups there anyway.
>
> See quirk_ide_bases in drivers/pci/quirks.c
If we can do that with PCI quirks, why the need to hard-code it in the
IDE driver too?
And IRQ zero isn't particularly helpful suggestion -- using an invalid
IRQ number would be better. Like NO_IRQ or IDE_NO_IRQ, which should be
-1.
Don't make me dig out the board where the PCI slots all get IRQ 0 :)
--
dwmw2
David Woodhouse wrote:
> On Mon, 2006-09-25 at 16:46 +0100, Alan Cox wrote:
>> Ar Llu, 2006-09-25 am 15:39 +0100, ysgrifennodd David Howells:
>>> Why does the arch have to supply those numbers? What's wrong with my
>>> suggested patch? According to code in libata, these are _legacy_ access
>>> methods, and on FRV they aren't currently required, so why can't I dispense
>> "legacy, legacy, legacy" "wont wont wont"
>>
>> The ports in question are PCI values. They come from the PCI
>> specifications and apply to any device with PCI bus, unless it has
>> special mappings. The same logic you are whining about is already partly
>> handled in the generic pci quirks code, and in time will end up with the
>> I/O port value fixups there anyway.
>>
>> See quirk_ide_bases in drivers/pci/quirks.c
>
> If we can do that with PCI quirks, why the need to hard-code it in the
> IDE driver too?
>
> And IRQ zero isn't particularly helpful suggestion -- using an invalid
> IRQ number would be better. Like NO_IRQ or IDE_NO_IRQ, which should be
> -1.
>
> Don't make me dig out the board where the PCI slots all get IRQ 0 :)
The irq is a special case no matter how we try to prettyify it. We need
two irqs, and PCI only gives us one per device.
Jeff
On Tue, 2006-09-26 at 04:52 -0400, Jeff Garzik wrote:
> The irq is a special case no matter how we try to prettyify it. We need
> two irqs, and PCI only gives us one per device.
That's fine -- but don't use zero to mean none. We have NO_IRQ for that,
and zero isn't an appropriate choice.
--
dwmw2
Ar Maw, 2006-09-26 am 09:56 +0100, ysgrifennodd David Woodhouse:
> On Tue, 2006-09-26 at 04:52 -0400, Jeff Garzik wrote:
> > The irq is a special case no matter how we try to prettyify it. We need
> > two irqs, and PCI only gives us one per device.
>
> That's fine -- but don't use zero to mean none. We have NO_IRQ for that,
> and zero isn't an appropriate choice.
Zero means "no IRQ". That's official kernel policy and true for both old
and new IDE. Architectures are supposed to remap any real "irq 0".
Might as well use NO_IRQ though, as its clearer.
Alan
Ar Maw, 2006-09-26 am 09:56 +0100, ysgrifennodd David Woodhouse:
> On Tue, 2006-09-26 at 04:52 -0400, Jeff Garzik wrote:
> > The irq is a special case no matter how we try to prettyify it. We need
> > two irqs, and PCI only gives us one per device.
>
> That's fine -- but don't use zero to mean none. We have NO_IRQ for that,
> and zero isn't an appropriate choice.
Let me correct that - you *had* NO_IRQ. It isn't defined for most
platforms any more in -mm it seems.
Alan
On Tue, 26 Sep 2006, David Woodhouse wrote:
>
> On Tue, 2006-09-26 at 04:52 -0400, Jeff Garzik wrote:
> > The irq is a special case no matter how we try to prettyify it. We need
> > two irqs, and PCI only gives us one per device.
>
> That's fine -- but don't use zero to mean none. We have NO_IRQ for that,
> and zero isn't an appropriate choice.
Zero _is_ an appropriate choice, dammit!
That NO_IRQ thing should be zero, and any architecture that thinks that
zero is a valid IRQ just needs to fix its own irq mapping so that the
"cookie" doesn't work.
The thing is, it's zero. Get over it. It can't be "-1" or some other
random value like people have indicated, because that thing is often read
from places where "-1" simply isn't a possible value (eg it gets its
default value initialized from a "unsigned char" in MMIO space on x86).
So instead of making everybody and their dog to silly things with some
NO_IRQ define that they haven't historically done, the rule is simple: "0"
means "no irq", so that you can test for it with obvious code like
if (!dev->irq)
..
and then, if your actual _hardware_ things that the bit-pattern with all
bits clear is a valid irq that can be used for normal devices, then what
you do is you add a irq number translation layer (WHICH WE NEED AND HAVE
_ANYWAY_) and make sure that nobody sees that on a _software_ level.
Linus
Linus Torvalds <[email protected]> wrote:
> Zero _is_ an appropriate choice, dammit!
> ...
> and then, if your actual _hardware_ things that the bit-pattern with all
> bits clear is a valid irq that can be used for normal devices,
PCI, for example. According to the spec, 0x00 is valid in the Interrupt Line
register and 0xFF indicates unconnected or unset.
> then what you do is you add a irq number translation layer (WHICH WE NEED
> AND HAVE _ANYWAY_) and make sure that nobody sees that on a _software_
> level.
So, by your argument, if you _have_ to have an IRQ translation layer anyway,
then what's the problem with having zero as a valid IRQ and using some other
value to indicate an invalid IRQ? As you say, you have a translation layer
anyway...
That would mean the arch maintainer could make the optimal choice for their
arch - perhaps picking 255 which would be consistent with PCI.
As far as FRV goes, I'm quite happy with 0 as being NO_IRQ, since the 0 bit in
the primary PIC registers is the master switch, not a per-level bit (there are
no source indicators unfortunately).
However, x86, x86_64, and others *do* treat IRQ 0 as being valid, and expose
it to userspace in various ways:
warthog>cat /proc/interrupts
CPU0
0: 287035291 IO-APIC-edge timer
...
So on *that* basis, using IRQ 0 to indicate unset/invalid/etc would seem to be
bad.
David
On Tue, 2006-09-26 at 09:15 -0700, Linus Torvalds wrote:
> That NO_IRQ thing should be zero, and any architecture that thinks that
> zero is a valid IRQ just needs to fix its own irq mapping so that the
> "cookie" doesn't work.
> The thing is, it's zero. Get over it.
Signed-off-by: David Woodhouse <[email protected]>
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 92be519..0cdf8ad 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -219,7 +219,7 @@ int setup_irq(unsigned int irq, struct i
unsigned long flags;
int shared = 0;
- if (irq >= NR_IRQS)
+ if (!irq || irq >= NR_IRQS)
return -EINVAL;
if (desc->chip == &no_irq_chip)
--
dwmw2
On Mon, 2006-09-25 at 15:39 +0100, David Howells wrote:
> My point is that all the numbers are invalid or incorrect. They will cause
> the arch to oops, and so need completely replacing.
The way we deal with code which blindly pokes at legacy I/O addresses on
PowerPC is to have a check_legacy_ioport() function which will tell you
if it's safe to poke at the address in question. Perhaps that should be
extended to other architectures (some can just #define it to 0) and used
in the IDE code.
--
dwmw2