On Mon, Sep 29, 2008 at 10:23:25AM -0700, Arjan van de Ven wrote:
> On Mon, 29 Sep 2008 11:10:49 -0600
> Grant Grundler <[email protected]> wrote:
>
> > And I have to agree with willy/alan, pci_iomap() is already doing
> > this.
>
> pci_iomap() does "stuff" but it assumes you're using the iomap APIs
> across the driver. MANY don't.
pci_iomap() returns a "void __iomem *".
readl/writel take "void __iomem *" as an argument.
See build_mmio_read() in include/asm-x86/io.h
I think the assumption is the other way around: use of ioread/iowrite
assumes use of io_remap(). pci_iomap is the PCI wrapper around io_remap().
You just want a simpler wrapper (and I agree, it really could without
the extra arg).
But in any case, we can document pci_iomap() to be whatever you think
we should be exporting. pci_iomap() is not currently documented in
Documentation/. Or at least grep isn't seeing it.
> And pci_iomap() takes more parameters than most driver writers want or
> need. Most of the time it's "I want the whole bar"; even if my patch
> wraps around that, making the API simpler is still worth it imo
You are right about that.
Would calling the API "pci_iomap_bar()" to keep the naming consistent help
make it more acceptable?
(And adding documentation for both would be good too...I can do
that if the new API gets accepted.)
hth,
grant
On Tue, 30 Sep 2008 23:24:50 -0600
Grant Grundler <[email protected]> wrote:
>
> > And pci_iomap() takes more parameters than most driver writers want
> > or need. Most of the time it's "I want the whole bar"; even if my
> > patch wraps around that, making the API simpler is still worth it
> > imo
>
> You are right about that.
> Would calling the API "pci_iomap_bar()" to keep the naming consistent
> help make it more acceptable?
I'm fine with pci_iomap_bar()... it meets my goals
Would be nice if I'd be allowed to make it only work on MEM bars not IO
bars (so that drivers don't accidentally end up calling this on an IO
bar and then using readl() etc)
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org
Arjan van de Ven wrote:
> On Tue, 30 Sep 2008 23:24:50 -0600
>
> Grant Grundler <[email protected]> wrote:
> > > And pci_iomap() takes more parameters than most driver writers want
> > > or need. Most of the time it's "I want the whole bar"; even if my
> > > patch wraps around that, making the API simpler is still worth it
> > > imo
> >
> > You are right about that.
> > Would calling the API "pci_iomap_bar()" to keep the naming consistent
> > help make it more acceptable?
>
> I'm fine with pci_iomap_bar()... it meets my goals
> Would be nice if I'd be allowed to make it only work on MEM bars not IO
> bars (so that drivers don't accidentally end up calling this on an IO
> bar and then using readl() etc)
IIRC pci_iomap() was documented to work on both at it does "the right thing"
automatically.
Eike
On Tue, Sep 30, 2008 at 03:30:01PM -0700, Arjan van de Ven wrote:
> On Tue, 30 Sep 2008 23:24:50 -0600
> Grant Grundler <[email protected]> wrote:
>
> >
> > > And pci_iomap() takes more parameters than most driver writers want
> > > or need. Most of the time it's "I want the whole bar"; even if my
> > > patch wraps around that, making the API simpler is still worth it
> > > imo
> >
> > You are right about that.
> > Would calling the API "pci_iomap_bar()" to keep the naming consistent
> > help make it more acceptable?
>
> I'm fine with pci_iomap_bar()... it meets my goals
> Would be nice if I'd be allowed to make it only work on MEM bars not IO
> bars (so that drivers don't accidentally end up calling this on an IO
> bar and then using readl() etc)
>
If they use the iomap interface they shouldn't be using readl at all,
they should be using ioread*... It would be a bug otherwise.
cheers, Kyle
On Wed, Oct 01, 2008 at 08:42:54AM -0400, Kyle McMartin wrote:
> On Tue, Sep 30, 2008 at 03:30:01PM -0700, Arjan van de Ven wrote:
> > I'm fine with pci_iomap_bar()... it meets my goals
> > Would be nice if I'd be allowed to make it only work on MEM bars not IO
> > bars (so that drivers don't accidentally end up calling this on an IO
> > bar and then using readl() etc)
>
> If they use the iomap interface they shouldn't be using readl at all,
> they should be using ioread*... It would be a bug otherwise.
That's a viewpoint I've heard several people espouse over the last few
days, but it's not (entirely) true. Addresses returned from calling
iomap() on a memory location must be compatible with addresses returned
from calling ioremap(), so you can use readl() on an iomap address, as
long as you know that it was a memory address that was iomapped.
if (flags & IORESOURCE_MEM) {
if (flags & IORESOURCE_CACHEABLE)
return ioremap(start, len);
return ioremap_nocache(start, len);
}
OK, not all architectures use the generic code, but I've been through
and they all do more or less the above (mn10300 and frv just return the
address, but their readl() and inl() are identical)
--
Matthew Wilcox Intel Open Source Technology Centre
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours. We can't possibly take such
a retrograde step."
On Wed, Oct 01, 2008 at 06:57:11AM -0600, Matthew Wilcox wrote:
> > If they use the iomap interface they shouldn't be using readl at all,
> > they should be using ioread*... It would be a bug otherwise.
>
> That's a viewpoint I've heard several people espouse over the last few
> days, but it's not (entirely) true. Addresses returned from calling
> iomap() on a memory location must be compatible with addresses returned
> from calling ioremap(), so you can use readl() on an iomap address, as
> long as you know that it was a memory address that was iomapped.
>
> if (flags & IORESOURCE_MEM) {
> if (flags & IORESOURCE_CACHEABLE)
> return ioremap(start, len);
> return ioremap_nocache(start, len);
> }
>
> OK, not all architectures use the generic code, but I've been through
> and they all do more or less the above (mn10300 and frv just return the
> address, but their readl() and inl() are identical)
>
I don't recall anyone ever promising that the iomap interfaces would be
usable with legacy accessors. I'd certainly prefer it if we didn't, as
well, as it makes for more explicitly written drivers...
Just because you can use them, doesn't mean you should.
regards, Kyle
On Wed, 1 Oct 2008 09:07:17 -0400
Kyle McMartin <[email protected]> wrote:
> >
> > OK, not all architectures use the generic code, but I've been
> > through and they all do more or less the above (mn10300 and frv
> > just return the address, but their readl() and inl() are identical)
> >
>
> I don't recall anyone ever promising that the iomap interfaces would
> be usable with legacy accessors. I'd certainly prefer it if we
> didn't, as well, as it makes for more explicitly written drivers...
>
> Just because you can use them, doesn't mean you should.
>
ok so now we're full circle.
I started with a real ioremap, was told "no must use iomap" despite the
same argument you make, and now we're back to square one.
What I want is an interface that can replace ioremap() for the common
"I want the bar uncached" case. Nothing more nothing less.... sigh.
--
Arjan van de Ven Intel Open Source Technology Centre
For development, discussion and tips for power savings,
visit http://www.lesswatts.org