This patch updates the pci_iomap() kernel-doc to make it clarify the
case when read*()/write*() can be called over ioread*/iowrite*(). When
driver writers read this documenation sometimes it is assumed you just
*need* to use ioread*()/iorwrite*(). We have an exception so lets just
clarify this is not true for the exception.
lib/iomap.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
Signed-off-by: Luis R. Rodriguez <[email protected]>
Luis
On Mon, 17 Sep 2007 16:22:07 -0400
"Luis R. Rodriguez" <[email protected]> wrote:
> This patch updates the pci_iomap() kernel-doc to make it clarify the
> case when read*()/write*() can be called over ioread*/iowrite*(). When
> driver writers read this documenation sometimes it is assumed you just
> *need* to use ioread*()/iorwrite*(). We have an exception so lets just
> clarify this is not true for the exception.
>
> lib/iomap.c | 11 ++++++++---
> 1 files changed, 8 insertions(+), 3 deletions(-)
>
> Signed-off-by: Luis R. Rodriguez <[email protected]>
Reviewed-by: Alan Cox <[email protected]>
Although I would say s/recommended/required/
Alan
On 9/18/07, Alan Cox <[email protected]> wrote:
> On Mon, 17 Sep 2007 16:22:07 -0400
> "Luis R. Rodriguez" <[email protected]> wrote:
>
> > This patch updates the pci_iomap() kernel-doc to make it clarify the
> > case when read*()/write*() can be called over ioread*/iowrite*(). When
> > driver writers read this documenation sometimes it is assumed you just
> > *need* to use ioread*()/iorwrite*(). We have an exception so lets just
> > clarify this is not true for the exception.
> >
> > lib/iomap.c | 11 ++++++++---
> > 1 files changed, 8 insertions(+), 3 deletions(-)
> >
> > Signed-off-by: Luis R. Rodriguez <[email protected]>
>
> Reviewed-by: Alan Cox <[email protected]>
>
> Although I would say s/recommended/required/
Alright, here is the same patch inline with s/recommended/required/ language:
Signed-off-by: Luis R. Rodriguez <[email protected]>
Luis
---
lib/iomap.c | 11 ++++++++---
1 files changed, 8 insertions(+), 3 deletions(-)
diff --git a/lib/iomap.c b/lib/iomap.c
index 864f2ec..33651f8 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -247,9 +247,14 @@ EXPORT_SYMBOL(ioport_unmap);
* @maxlen: length of the memory to map
*
* Using this function you will get a __iomem address to your device BAR.
- * You can access it using ioread*() and iowrite*(). These functions hide
- * the details if this is a MMIO or PIO address space and will just do what
- * you expect from them in the correct way.
+ * If you use pci_iomap() it is recommended you use ioread*() and
iowrite*(). If
+ * your device is MMIO-only and as long as future architectures don't break the
+ * assumption that pcio_iomap()-returned cookie can be used by read*()/write*()
+ * then you can use read*()/write*() instead. The ioread*() and iowrite*()
+ * functions hide the details if this is a MMIO or PIO address space and will
+ * just do what you expect from them in the correct way. Drivers for MMIO-only
+ * devices would use this only to save themselves from having to call
+ * &pci_resource_start().
*
* @maxlen specifies the maximum length to map. If you want to get access to
* the complete BAR without checking for its length first, pass %0 here.
On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
>
> Alright, here is the same patch inline with s/recommended/required/ language:
Well, the thing is, I'm not at all sure that I agree with this.
If you use ioport_map/unmap, then you really *should* access them with the
proper iomem accessors (ioread/iowrite). The fact that it may happen to
work (when using the default lib/iomap.c implementation, at least) on
some architectures and with the current implementation still doesn't mean
that you should necessarily use readb/writeb.
After all, you cannot use "inb/outb" on it, even if would happen to be an
IO address.
So what is this usage that wants to use the bogus accessor? Why not fix
that instead of adding documentation for something that is very arguably
something we want to *avoid* having people do!
Linus
On 9/18/07, Linus Torvalds <[email protected]> wrote:
>
>
> On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
> >
> > Alright, here is the same patch inline with s/recommended/required/ language:
>
> Well, the thing is, I'm not at all sure that I agree with this.
>
> If you use ioport_map/unmap, then you really *should* access them with the
> proper iomem accessors (ioread/iowrite). The fact that it may happen to
> work (when using the default lib/iomap.c implementation, at least) on
> some architectures and with the current implementation still doesn't mean
> that you should necessarily use readb/writeb.
>
> After all, you cannot use "inb/outb" on it, even if would happen to be an
> IO address.
>
> So what is this usage that wants to use the bogus accessor? Why not fix
> that instead of adding documentation for something that is very arguably
> something we want to *avoid* having people do!
ACK -- driver developers use this just to save themselves a few lines
from calling pci_resource_start() and friends. How about having an
inline which does what pci_iomap() does except it doesn't call
ioport_map() ? I am just not sure where this would go..
Luis
On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
>
> ACK -- driver developers use this just to save themselves a few lines
> from calling pci_resource_start() and friends. How about having an
> inline which does what pci_iomap() does except it doesn't call
> ioport_map() ? I am just not sure where this would go..
I'm not understanding what the problem is?
Why don't these people just use "ioread*()/iowrite*()"?
In other words, the whole point of *not* using "read*/write*()" is that
you get a whole slew of much nicer interfaces.
So can people explain this fundamental issue? Why do people insist on
using the old interfaces (and matching them with the new setup)?
Linus
On 9/18/07, Linus Torvalds <[email protected]> wrote:
>
>
> On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
> >
> > ACK -- driver developers use this just to save themselves a few lines
> > from calling pci_resource_start() and friends. How about having an
> > inline which does what pci_iomap() does except it doesn't call
> > ioport_map() ? I am just not sure where this would go..
>
> I'm not understanding what the problem is?
>
> Why don't these people just use "ioread*()/iowrite*()"?
>
> In other words, the whole point of *not* using "read*/write*()" is that
> you get a whole slew of much nicer interfaces.
>
> So can people explain this fundamental issue? Why do people insist on
> using the old interfaces (and matching them with the new setup)?
An extra branch is created on MMIO-only devices on read/writes on the
IO_COND macro using this interface -- or is this optimized out?
Luis
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index cde592f..69e5390 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -63,6 +63,7 @@ extern void ioport_unmap(void __iomem *);
/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
struct pci_dev;
extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_mmio_map(struct pci_dev *dev, int bar, unsigned long max);
extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
#endif
diff --git a/lib/iomap.c b/lib/iomap.c
index 864f2ec..c87d7f3 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -275,9 +275,32 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
return NULL;
}
+/* for people who wish to use readl/writel exclusively */
+void __iomem *pci_mmio_map(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+ unsigned long start = pci_resource_start(dev, bar);
+ unsigned long len = pci_resource_len(dev, bar);
+ unsigned long flags = pci_resource_flags(dev, bar);
+
+ if (!len || !start)
+ return NULL;
+ if (maxlen && len > maxlen)
+ len = maxlen;
+ if (flags & IORESOURCE_IO)
+ return NULL;
+ if (flags & IORESOURCE_MEM) {
+ if (flags & IORESOURCE_CACHEABLE)
+ return ioremap(start, len);
+ return ioremap_nocache(start, len);
+ }
+ /* What? */
+ return NULL;
+}
+
void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
{
IO_COND(addr, /* nothing */, iounmap(addr));
}
EXPORT_SYMBOL(pci_iomap);
+EXPORT_SYMBOL(pci_mmio_map);
EXPORT_SYMBOL(pci_iounmap);
On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
>
> An extra branch is created on MMIO-only devices on read/writes on the
> IO_COND macro using this interface -- or is this optimized out?
Umm. Does anybody actually have any performance numbers?
The thing is, those things are *cheap* compared to the IO. And any
high-performance device will be using DMA for the real IO, so we're not
generally even talking about any performance-critical stuff.
Quite frankly, if performance is a _real_ reason to avoid
ioread*/iowrite*, I'll happily accept read*/write*, but it would be needed
to be backed up by real numbers. Can you even measure it?
I would definitely *not* encourage the notion that people should use
readl/writel because of "performance reasons". That may be valid for some
fbcon driver, but those drivers go to other extremes (ie they use
"__raw_writel()" etc, and MTRR's etc).
If you don't use write-combining memory regions, the performance argument
is not really valid.
Linus
On 9/18/07, Linus Torvalds <[email protected]> wrote:
>
>
> On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
> >
> > An extra branch is created on MMIO-only devices on read/writes on the
> > IO_COND macro using this interface -- or is this optimized out?
>
> Umm. Does anybody actually have any performance numbers?
>
> The thing is, those things are *cheap* compared to the IO. And any
> high-performance device will be using DMA for the real IO, so we're not
> generally even talking about any performance-critical stuff.
ACK but do we really need to benchmark this if we *know* we are
creating unnecessary branches? How about Jeff's suggestion of
introducing pci_mmio_map() ? This way we leave the current
documenation as it is and just introduce this for MMIO-only devices.
No benchmarks are necessary then, and no need to confuse people with
the old API.
Luis
On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
>
> ACK but do we really need to benchmark this if we *know* we are
> creating unnecessary branches? How about Jeff's suggestion of
> introducing pci_mmio_map() ?
Here's the counter-argument:
- the run-time expense of this is basically *zero*
- the expense of badly written drivers is absolutely *huge*
The fewer "clever" interfaces we have for no good reason, and the less
unnecessary stuff that really doesn't matter, the more we help the
*second* case.
The fact is, for device drivers, the *true* performance issues are nowhere
*near* the actual IO accessor functions, and are all at a much higher
level for all but a very very limited set of drivers. And I think you're
making the important issues *worse*.
Really. "pci_mmio_map()" may not be a bad interface, but you're simply
working on entirely the wrong problem. The problem is not an out-of-line
IO accessor function - the problem is that you're making it even more
complicated to write drivers.
The whole point of the "iomap()" interface was *never* about making the
driver interfaces faster. It was about making them *cleaner*. And you're
screwing that up!
The old situation with SATA drivers that had
if (iomem)
writel(..)
else
outl(..)
in the cases that needed it (and used hardcoded writel/outl in the cases
that didn't) was an example of code that "in theory" is faster. But
dammit, in practice that mattered not one whit, and what iomap() tries to
do is to attack the _real_ problem we had in that area.
Which had nothing what-so-ever to do with any branches.
Linus
Linus Torvalds wrote:
> The old situation with SATA drivers that had
>
> if (iomem)
> writel(..)
> else
> outl(..)
>
> in the cases that needed it (and used hardcoded writel/outl in the cases
> that didn't) was an example of code that "in theory" is faster. But
> dammit, in practice that mattered not one whit, and what iomap() tries to
> do is to attack the _real_ problem we had in that area.
>
> Which had nothing what-so-ever to do with any branches.
And none of those issues are a factor at all in ath5k (which spawned
this sub-discussion), or indeed many other drivers. The above code you
cite is -generic-, hardware agnostic code.
Most new hardware is MMIO-only, making ioread32() only for drivers that
care about legacy IO support, something that is being slowly phased out.
e.g. legacy IDE, legacy 10/100 mbps ethernet NICs with the dual
MMIO/PIO register spaces.
Jeff
On Tue, 18 Sep 2007, Jeff Garzik wrote:
>
> Most new hardware is MMIO-only, making ioread32() only for drivers that care
> about legacy IO support, something that is being slowly phased out. e.g.
> legacy IDE, legacy 10/100 mbps ethernet NICs with the dual MMIO/PIO register
> spaces.
Hey, I think your patch was better than the one that just assumed that you
can do "read*/write*()" on an iomap(). If people really want it, I don't
care *that* much.
I just think that the arguments that have been raised so far have really
been very weak. There really is no "performance" argument without some
kinds of numbers.
I just think that proliferation of IO interfaces is a bad idea.
Linus
On Tue, 2007-09-18 at 12:03 -0700, Linus Torvalds wrote:
>
> On Tue, 18 Sep 2007, Luis R. Rodriguez wrote:
> >
> > Alright, here is the same patch inline with s/recommended/required/ language:
>
> Well, the thing is, I'm not at all sure that I agree with this.
>
> If you use ioport_map/unmap, then you really *should* access them with the
> proper iomem accessors (ioread/iowrite). The fact that it may happen to
> work (when using the default lib/iomap.c implementation, at least) on
> some architectures and with the current implementation still doesn't mean
> that you should necessarily use readb/writeb.
>
> After all, you cannot use "inb/outb" on it, even if would happen to be an
> IO address.
>
> So what is this usage that wants to use the bogus accessor? Why not fix
> that instead of adding documentation for something that is very arguably
> something we want to *avoid* having people do!
Agreed.
ioremap -> readb/writeb
ioport_map/pci_iomap -> ioread/iowrite
and nothing else should be allowed.
Cheers,
Ben.
> If you use ioport_map/unmap, then you really *should* access them with the
> proper iomem accessors (ioread/iowrite). The fact that it may happen to
> work (when using the default lib/iomap.c implementation, at least) on
> some architectures and with the current implementation still doesn't mean
> that you should necessarily use readb/writeb.
Another reason we should enforce the use of ioread/iowrite is that some
platforms do daft things like map mmio type devices through indirect
register access. Sparc PCMCIA is a classic example. The only sane way to
make these work is to require ioread/iowrite is used with iomap
On Tue, 2007-09-18 at 16:21 -0400, Jeff Garzik wrote:
> A new pci_mmio_map() helper, to be used with 100% MMIO hardware, might
> help eliminate confusion.
Maybe not the best name in theory but at least would show that it
relates to existing ioremap would be pci_ioremap()
Ben.
On Tue, 2007-09-18 at 13:30 -0700, Linus Torvalds wrote:
> Quite frankly, if performance is a _real_ reason to avoid
> ioread*/iowrite*, I'll happily accept read*/write*, but it would be
> needed
> to be backed up by real numbers. Can you even measure it?
>
Also, I've been told that modern x86 chipsets have the ability to remap
IO space in the CPU physical address space. Is that true ? That would
allow even x86 to get rid of the condition and just use some magic
offset at map time.
Ben.
On Tue, 18 Sep 2007, Linus Torvalds wrote:
>
> I just think that proliferation of IO interfaces is a bad idea.
Side note: just allowing readl/writel on the iomap'ed pointers obviously
avoids that proliferation, but iomap really is architecture-specific, so
not only will different architectures act very differently, it's not
necessarily the case that iomap() will always have the same pointer as the
old ioremap(), even though the routines in the *default* implementation in
lib/iomap.c do that.
In fact, for IO port accesses, we already do some debugging to make sure
that people don't misuse the iomap() interfaces (ie the whole "Bad IO
access.." thing). I could well see the same thing happening for MMIO under
some debugging option, explicitly using some high bits in a 64-bit address
as a cookie - and that would make using regular readl/writel on the cookie
just fail spectacularly.
That said, it does seem unlikely (ie if we added a cookie for MMIO
pointers, we'd probably do it *both* for iomap and for ioremap(), and
readl/writel would continue to work). So in practice the readl/writel
probably works.
Linus
On Wed, 19 Sep 2007, Benjamin Herrenschmidt wrote:
>
> Also, I've been told that modern x86 chipsets have the ability to remap
> IO space in the CPU physical address space. Is that true ? That would
> allow even x86 to get rid of the condition and just use some magic
> offset at map time.
I've not seen that, but I wouldn't be entirely surprised if IO
virtualization eventually causes something like this to happen:
virtualizing PIO is just damn painful right now, due to the lack of any
way to remap it.
I *think* you may be confused with the PCI config cycles, where the new
MMIO configuration was introduced (for similar virtualization reasons).
But it's also possible that this is one of those undocumented areas and
CPU's actually do have some IO remapping facility.
Linus
commit 6e09c71822f76c618353682bf295fc7588284521
Author: Jeff Garzik <[email protected]>
Date: Tue Sep 18 19:06:08 2007 -0400
Add pci_ioremap() to generic iomap lib.
(arches that don't wish to use lib/iomap.c's version may fill in their own)
Signed-off-by: Jeff Garzik <[email protected]>
include/asm-generic/iomap.h | 1 +
lib/iomap.c | 34 ++++++++++++++++++++++++++++++++++
2 files changed, 35 insertions(+)
6e09c71822f76c618353682bf295fc7588284521
diff --git a/include/asm-generic/iomap.h b/include/asm-generic/iomap.h
index cde592f..611e6cf 100644
--- a/include/asm-generic/iomap.h
+++ b/include/asm-generic/iomap.h
@@ -63,6 +63,7 @@ extern void ioport_unmap(void __iomem *);
/* Create a virtual mapping cookie for a PCI BAR (memory or IO) */
struct pci_dev;
extern void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long max);
+extern void __iomem *pci_ioremap(struct pci_dev *dev, int bar, unsigned long max);
extern void pci_iounmap(struct pci_dev *dev, void __iomem *);
#endif
diff --git a/lib/iomap.c b/lib/iomap.c
index 864f2ec..0338da0 100644
--- a/lib/iomap.c
+++ b/lib/iomap.c
@@ -275,9 +275,43 @@ void __iomem *pci_iomap(struct pci_dev *dev, int bar, unsigned long maxlen)
return NULL;
}
+/**
+ * pci_ioremap - create a virtual mapping cookie for a memory-based PCI BAR
+ * @dev: PCI device that owns the BAR
+ * @bar: BAR number
+ * @maxlen: length of MMIO memory to map
+ *
+ * Using this function you will get a __iomem address to your device BAR.
+ * You can access it using read*() and write*().
+ *
+ * @maxlen specifies the maximum length to map. If you want to get access to
+ * the complete BAR without checking for its length first, pass %0 here.
+ * */
+void __iomem *pci_ioremap(struct pci_dev *dev, int bar, unsigned long maxlen)
+{
+ unsigned long start = pci_resource_start(dev, bar);
+ unsigned long len = pci_resource_len(dev, bar);
+ unsigned long flags = pci_resource_flags(dev, bar);
+
+ if (!len || !start)
+ return NULL;
+ if (maxlen && len > maxlen)
+ len = maxlen;
+ if (flags & IORESOURCE_IO)
+ return NULL;
+ if (flags & IORESOURCE_MEM) {
+ if (flags & IORESOURCE_CACHEABLE)
+ return ioremap(start, len);
+ return ioremap_nocache(start, len);
+ }
+ /* What? */
+ return NULL;
+}
+
void pci_iounmap(struct pci_dev *dev, void __iomem * addr)
{
IO_COND(addr, /* nothing */, iounmap(addr));
}
EXPORT_SYMBOL(pci_iomap);
+EXPORT_SYMBOL(pci_ioremap);
EXPORT_SYMBOL(pci_iounmap);
On Tue, 18 Sep 2007, Jeff Garzik wrote:
>
> Easy enough... 'pcimap' branch of
> git://git.kernel.org/pub/scm/linux/kernel/git/jgarzik/misc-2.6.git
This is wrong.
You must not put it in lib/iomap.c, since that file is only compiled for
architectures that use CONFIG_GENERIC_IOMAP.
So you need to put it in some *generic* PCI place, like drivers/pci/pci.c
or similar.
Linus
2007/9/19, Alan Cox <[email protected]>:
> > If you use ioport_map/unmap, then you really *should* access them with the
> > proper iomem accessors (ioread/iowrite). The fact that it may happen to
> > work (when using the default lib/iomap.c implementation, at least) on
> > some architectures and with the current implementation still doesn't mean
> > that you should necessarily use readb/writeb.
>
> Another reason we should enforce the use of ioread/iowrite is that some
> platforms do daft things like map mmio type devices through indirect
> register access. Sparc PCMCIA is a classic example. The only sane way to
> make these work is to require ioread/iowrite is used with iomap
>
Hmm, we 've had bug reports about driver (madwifi-old-openhal) not
working on Sparc64, can someone test if this is the case (the fact
that we used readl/writel instead of ioread/iowrite on
madwifi-old-openhal) ? It was really weird because we handle
big-endian machines ok (madwifi-old-openhal on ppc worked fine).
(ath5k was based on madwifi-old-openhal)
--
GPG ID: 0xD21DB2DB
As you read this post global entropy rises. Have Fun ;-)
Nick