[PATCH] fix ppc ioremap prototype

[PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

The 44x merge fucked this up, bring it back in linux with the other
architectures.

/me still boggles how a patch like that could have sneaked in without
a review on lkml..


--- 1.13/arch/ppc/mm/pgtable.c	Wed Sep  3 14:16:34 2003
+++ edited/arch/ppc/mm/pgtable.c	Wed Sep  3 21:10:34 2003
@@ -119,9 +119,9 @@
 
 #ifndef CONFIG_44x
 void *
-ioremap(phys_addr_t addr, unsigned long size)
+ioremap(unsigned long addr, unsigned long size)
 {
-	return __ioremap(addr, size, _PAGE_NO_CACHE);
+	return __ioremap((phys_addr_t)addr, size, _PAGE_NO_CACHE);
 }
 #else /* CONFIG_44x */
 void *
@@ -131,9 +131,9 @@
 }
 
 void *
-ioremap(phys_addr_t addr, unsigned long size)
+ioremap(unsigned long addr, unsigned long size)
 {
-	phys_addr_t addr64 = fixup_bigphys_addr(addr, size);;
+	phys_addr_t addr64 = fixup_bigphys_addr((phys_addr_t)addr, size);;
 
 	return ioremap64(addr64, size);
 }
--- 1.14/include/asm-ppc/io.h	Wed Sep  3 14:16:34 2003
+++ edited/include/asm-ppc/io.h	Wed Sep  3 21:13:40 2003
@@ -201,7 +201,7 @@
  */
 extern void *__ioremap(phys_addr_t address, unsigned long size,
 		       unsigned long flags);
-extern void *ioremap(phys_addr_t address, unsigned long size);
+extern void *ioremap(unsigned long address, unsigned long size);
 #ifdef CONFIG_44x
 extern void *ioremap64(unsigned long long address, unsigned long size);
 #endif

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Christoph Hellwig writes:

> The 44x merge fucked this up, bring it back in linux with the other
> architectures.

I don't see why this is a problem.  The change is compatible with the
existing uses.  We need to be able to map 36-bit physical addresses on
44x.  What we really need now is 64-bit start/end values in struct
resource.

> /me still boggles how a patch like that could have sneaked in without
> a review on lkml..

First, it's a PPC-only change, and secondly, it's been around for
months in the linuxppc-2.5 tree - since June last year in fact, and
even longer than that in the linuxppc_2_4_devel tree.  Lkml is not
where most PPC-specific stuff gets discussed, it's too noisy for that.

Paul.

Re: [PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

On Thu, Sep 04, 2003 at 10:33:57AM +1000, Paul Mackerras wrote:
> I don't see why this is a problem.  The change is compatible with the
> existing uses.  We need to be able to map 36-bit physical addresses on
> 44x.  What we really need now is 64-bit start/end values in struct
> resource.

Then add the phys_addr_t to all places where we deal with physical
addresses, even if it's typedef'ed to unsigned long on all other
arches and sane ppcs.

> > /me still boggles how a patch like that could have sneaked in without
> > a review on lkml..
> 
> First, it's a PPC-only change, and secondly, it's been around for
> months in the linuxppc-2.5 tree - since June last year in fact, and
> even longer than that in the linuxppc_2_4_devel tree.  Lkml is not
> where most PPC-specific stuff gets discussed, it's too noisy for that.

ioremap is not ppc-specific.

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 09:13:34 +0200
Christoph Hellwig <hch@lst.de> wrote:

> On Thu, Sep 04, 2003 at 10:33:57AM +1000, Paul Mackerras wrote:
> > I don't see why this is a problem.  The change is compatible with the
> > existing uses.  We need to be able to map 36-bit physical addresses on
> > 44x.  What we really need now is 64-bit start/end values in struct
> > resource.
> 
> Then add the phys_addr_t to all places where we deal with physical
> addresses, even if it's typedef'ed to unsigned long on all other
> arches and sane ppcs.

Or do what I do on sparc32, stick the >32bit part in the unused
bits of the resource flags.  It works perfectly fine.

Re: [PATCH] fix ppc ioremap prototype

[Russell King]

On Thu, Sep 04, 2003 at 09:13:34AM +0200, Christoph Hellwig wrote:
> On Thu, Sep 04, 2003 at 10:33:57AM +1000, Paul Mackerras wrote:
> > I don't see why this is a problem.  The change is compatible with the
> > existing uses.  We need to be able to map 36-bit physical addresses on
> > 44x.  What we really need now is 64-bit start/end values in struct
> > resource.
> 
> Then add the phys_addr_t to all places where we deal with physical
> addresses, even if it's typedef'ed to unsigned long on all other
> arches and sane ppcs.

But phys_addr_t in struct resource and being passed into ioremap is
confusing.  Apparantly, it isn't a physical address, but a platform
defined cookie which just happens to look like a physical address.

(or are we finally going to admit that it is a physical address?)

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

On Thu, Sep 04, 2003 at 08:30:07AM +0100, Russell King wrote:
> But phys_addr_t in struct resource and being passed into ioremap is
> confusing.  Apparantly, it isn't a physical address, but a platform
> defined cookie which just happens to look like a physical address.
> 
> (or are we finally going to admit that it is a physical address?)

Umm, right, so the typedef name is also completly bogus, if we're
going to go that route it needs to be something likeb iocookie_t.

See why I want stuff like this on lkml?  It has implication far beyond
ppc..

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 09:38:45 +0200
Christoph Hellwig <hch@lst.de> wrote:

> Umm, right, so the typedef name is also completly bogus, if we're
> going to go that route it needs to be something likeb iocookie_t.

My suggestion is to just pass a resource and an offset to ioremap().

Re: [PATCH] fix ppc ioremap prototype

[Linus Torvalds]

On Thu, 4 Sep 2003, David S. Miller wrote:
> 
> My suggestion is to just pass a resource and an offset to ioremap().

Actually, my suggestion right now is to ignore the issue, and let the 
current ppc440x code stand as-is. After all, it works, and it does what 
the ppc people want. We may at some point switch over _all_ ioremap users, 
but there is no real reason to do so right now.

		Linus

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 01:12:28 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> Actually, my suggestion right now is to ignore the issue, and let the 
> current ppc440x code stand as-is. After all, it works, and it does what 
> the ppc people want. We may at some point switch over _all_ ioremap users, 
> but there is no real reason to do so right now.

Ok, that's fine.

What we could do in the interim is create an ioremap_resource()
and then move things over gradually.

Re: [PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

On Thu, Sep 04, 2003 at 01:10:10AM -0700, David S. Miller wrote:
> What we could do in the interim is create an ioremap_resource()
> and then move things over gradually.

ioremap_resource() looks like a fine idea.  It's cleaner, easily
emulateable on <= 2.4 and solves the problem this hack wanted to
work around properly.

This still doesn't make the phys_addr_t a good interims solution,
though.  Just use ioremap_resource from the beginning for those
drivers that care for the bigger address space on ppc44x.

Paul, what does actually use this higher addresses?

Re: [PATCH] fix ppc ioremap prototype

[Benjamin Herrenschmidt]

On Thu, 2003-09-04 at 10:28, Christoph Hellwig wrote:
> On Thu, Sep 04, 2003 at 01:10:10AM -0700, David S. Miller wrote:
> > What we could do in the interim is create an ioremap_resource()
> > and then move things over gradually.
> 
> ioremap_resource() looks like a fine idea.  It's cleaner, easily
> emulateable on <= 2.4 and solves the problem this hack wanted to
> work around properly.
> 
> This still doesn't make the phys_addr_t a good interims solution,
> though.  Just use ioremap_resource from the beginning for those
> drivers that care for the bigger address space on ppc44x.
> 
> Paul, what does actually use this higher addresses?

The great thing with ioremap_resource() is that we can make it work
for PCI IO as well as MMIO and slowly get rid of anything tapping
ports without first ioremapp'ing them. That would help some platforms
with bazillons PCI busses as well so in the end, we don't have to
keep the whole PCI IO space of those machines ioremapped all the time,
this is especially useful on 32 bits archs where we can run short on
virtual space easily.

At first, it would just "do nothing" for PCI IO (just return the same
token), and once enough drivers have been ported, the arch can decide
to change the inx/outx implementation to rely on ioremap_resource
doing the actual mapping.

Ben.

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Christoph Hellwig writes:

> ioremap_resource() looks like a fine idea.  It's cleaner, easily
> emulateable on <= 2.4 and solves the problem this hack wanted to
> work around properly.

I agree.

> This still doesn't make the phys_addr_t a good interims solution,
> though.  Just use ioremap_resource from the beginning for those
> drivers that care for the bigger address space on ppc44x.
> 
> Paul, what does actually use this higher addresses?

We have drivers for on-chip peripherals that work from a struct
ocp_device and call ioremap on the ocp_dev->paddr value, which is a
phys_addr_t (although some of them use __ioremap instead).  These
drivers are used on 405-based systems (with 32-bit phys_addr_t) as
well as on 440-based systems.

These drivers are in the linuxppc-2.{4,5} trees but most of them
haven't made it into the official trees yet.  They could all be
audited and converted to use __ioremap, although it seems a bit
arbitrary to say that you can't use ioremap in a an ocp driver if
you're going to use it on a 440.  I wouldn't expect it to be
immediately obvious to a driver author just why you have to use
__ioremap rather than ioremap in an ocp driver.  The ioremap_resource
idea would solve that problem of course, we would put a resource in
the struct ocp_device instead of the physical address.

Paul.

Re: [PATCH] fix ppc ioremap prototype

[Geert Uytterhoeven]

On Thu, 4 Sep 2003, Paul Mackerras wrote:
> > Paul, what does actually use this higher addresses?
> 
> We have drivers for on-chip peripherals that work from a struct
> ocp_device and call ioremap on the ocp_dev->paddr value, which is a
> phys_addr_t (although some of them use __ioremap instead).  These
> drivers are used on 405-based systems (with 32-bit phys_addr_t) as
> well as on 440-based systems.
> 
> These drivers are in the linuxppc-2.{4,5} trees but most of them
> haven't made it into the official trees yet.  They could all be
> audited and converted to use __ioremap, although it seems a bit
> arbitrary to say that you can't use ioremap in a an ocp driver if
> you're going to use it on a 440.  I wouldn't expect it to be

`ioremap is meant for PCI memory space only'

Oops...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Geert Uytterhoeven writes:

> `ioremap is meant for PCI memory space only'

Did I say that, or someone else? :)  ioremap predates PCI support by a
long way IIRC...

Paul.

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 22:41:31 +1000 (EST)
Paul Mackerras <paulus@samba.org> wrote:

> Geert Uytterhoeven writes:
> 
> > `ioremap is meant for PCI memory space only'
> 
> Did I say that, or someone else? :)  ioremap predates PCI support by a
> long way IIRC...

No, it really is true.

If only your non-pci drivers need to get at the larger
physical addresses, perhaps a easier short term solution
is to just use a special ppc_foo_ioremap() for them.

Re: [PATCH] fix ppc ioremap prototype

[Geert Uytterhoeven]

On Thu, 4 Sep 2003, Paul Mackerras wrote:
> Geert Uytterhoeven writes:
> > `ioremap is meant for PCI memory space only'
> 
> Did I say that, or someone else? :)  ioremap predates PCI support by a
> long way IIRC...

inb() and friends are for ISA/PCI I/O space
isa_readb() and friends are for ISA memory space
readb() and friends are for PCI memory space (after ioremap())

That's why other buses (e.g. SBUS and Zorro) have their own versions of
ioremap() and readb() etc.).

Life would be much easier with bus-specific I/O ops...

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fix ppc ioremap prototype

[Deepak Saxena]

On Sep 04 2003, at 14:57, Geert Uytterhoeven was caught saying:
> On Thu, 4 Sep 2003, Paul Mackerras wrote:
> > Geert Uytterhoeven writes:
> > > `ioremap is meant for PCI memory space only'
> > 
> > Did I say that, or someone else? :)  ioremap predates PCI support by a
> > long way IIRC...
> 
> inb() and friends are for ISA/PCI I/O space
> isa_readb() and friends are for ISA memory space
> readb() and friends are for PCI memory space (after ioremap())
> 
> That's why other buses (e.g. SBUS and Zorro) have their own versions of
> ioremap() and readb() etc.).
> 
> Life would be much easier with bus-specific I/O ops...

What happens if I have a device that can be either ISA or connected 
directly to a local memory bus? The driver should be able to 
ioremap(some resource) and then read/write the device without
having to have ugly #ifdefs to deal with different bus types.
Example in point is the CS8900a device which is hooked up directly
to a FPGA on the local memory bus with the bytelanes backwards.
The ammount of hacking done in the driver to get around that is
ugly. It would be much nicer if the driver still just did read*/write*
and the platform level code could deal with all the translation
issues. This requires a generic API for all I/O devices.

~Deepak

-- 
Deepak Saxena
MontaVista Software - Powering the Embedded Revolution - www.mvista.com

Re: [PATCH] fix ppc ioremap prototype

[Geert Uytterhoeven]

On Thu, 4 Sep 2003, Deepak Saxena wrote:
> On Sep 04 2003, at 14:57, Geert Uytterhoeven was caught saying:
> > On Thu, 4 Sep 2003, Paul Mackerras wrote:
> > > Geert Uytterhoeven writes:
> > > > `ioremap is meant for PCI memory space only'
> > > 
> > > Did I say that, or someone else? :)  ioremap predates PCI support by a
> > > long way IIRC...
> > 
> > inb() and friends are for ISA/PCI I/O space
> > isa_readb() and friends are for ISA memory space
> > readb() and friends are for PCI memory space (after ioremap())
> > 
> > That's why other buses (e.g. SBUS and Zorro) have their own versions of
> > ioremap() and readb() etc.).
> > 
> > Life would be much easier with bus-specific I/O ops...
> 
> What happens if I have a device that can be either ISA or connected 
> directly to a local memory bus? The driver should be able to 
> ioremap(some resource) and then read/write the device without
> having to have ugly #ifdefs to deal with different bus types.
> Example in point is the CS8900a device which is hooked up directly
> to a FPGA on the local memory bus with the bytelanes backwards.
> The ammount of hacking done in the driver to get around that is
> ugly. It would be much nicer if the driver still just did read*/write*
> and the platform level code could deal with all the translation
> issues. This requires a generic API for all I/O devices.

The usual solution is to have my_read() and friends in your driver, and #define
them to what's appropriate based on your CONFIG_* settings.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fix ppc ioremap prototype

[Jes Sorensen]

>>>>> "Deepak" == Deepak Saxena <dsaxena@mvista.com> writes:

Deepak> What happens if I have a device that can be either ISA or
Deepak> connected directly to a local memory bus? The driver should be
Deepak> able to ioremap(some resource) and then read/write the device
Deepak> without having to have ugly #ifdefs to deal with different bus
Deepak> types.

You can't do that reliably, some architectures requires different
access for ISA vs PCI vs random-other busses. The driver needs to be
aware what bus it's trying to go through.

Deepak> Example in point is the CS8900a device which is hooked
Deepak> up directly to a FPGA on the local memory bus with the
Deepak> bytelanes backwards.  The ammount of hacking done in the
Deepak> driver to get around that is ugly. It would be much nicer if
Deepak> the driver still just did read*/write* and the platform level
Deepak> code could deal with all the translation issues. This requires
Deepak> a generic API for all I/O devices.

That might be nicer from the driver perspective but it will make
readb/writeb a lot more complex and inefficient for no reason on some
architectures.

The driver will have to know whether it's on a PCI, ISA or directly
attached anyway when it's probing so it's perfectly reasonable to
expect it to deal with that when it's accessing the registers as well.

Cheers,
Jes

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Geert Uytterhoeven writes:

> inb() and friends are for ISA/PCI I/O space
> isa_readb() and friends are for ISA memory space
> readb() and friends are for PCI memory space (after ioremap())
> 
> That's why other buses (e.g. SBUS and Zorro) have their own versions of
> ioremap() and readb() etc.).

I suggest you fix Documentation/zorro.txt then: 8-)

: 		  Writing Device Drivers for Zorro Devices
: 		  ----------------------------------------
: 
: Written by Geert Uytterhoeven <geert@linux-m68k.org>
: Last revised: February 27, 2000

[snip]

:   - Zorro III address space must be mapped explicitly using ioremap() first
:     before it can be accessed:
:  
: 	  virt_addr = ioremap(bus_addr, size);
: 	  ...
: 	  iounmap(virt_addr);

Regards,
Paul.

Re: [PATCH] fix ppc ioremap prototype

[Geert Uytterhoeven]

On Fri, 5 Sep 2003, Paul Mackerras wrote:
> Geert Uytterhoeven writes:
> > inb() and friends are for ISA/PCI I/O space
> > isa_readb() and friends are for ISA memory space
> > readb() and friends are for PCI memory space (after ioremap())
> > 
> > That's why other buses (e.g. SBUS and Zorro) have their own versions of
> > ioremap() and readb() etc.).
> 
> I suggest you fix Documentation/zorro.txt then: 8-)
> 
> : 		  Writing Device Drivers for Zorro Devices
> : 		  ----------------------------------------
> : 
> : Written by Geert Uytterhoeven <geert@linux-m68k.org>
> : Last revised: February 27, 2000

Thanks for reminding me I have to update this doc! Expect a patch soon ;-)

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fix ppc ioremap prototype

[Linus Torvalds]

On Thu, 4 Sep 2003, Geert Uytterhoeven wrote:
> 
> `ioremap is meant for PCI memory space only'

No, I don't agree. The "iomem_resource" is there for all IO-mapped ranges, 
and it could easily be a mixture of PCI and other buses. In fact, it 
already is on bog-standard hardware: it contains the AGP windows etc.

Whatever is visible in the physical memory window (by _some_ definition of 
"physical memory window", and that definition clearly has to be the 
biggest possible as seen by software, so it's usually a CPU-centric view 
of "any bus that is outside the CPU") should be mappable into the 
_virtual_ memory window as seen by the CPU.

So clearly ioremap() has to work for other buses too.

I think that in the 2.7.x timeframe, the right thing to do is definitely:
 - move towards using "struct resource" and "ioremap_resource()"
 - make resource sizes potentially be larger (ie use "u64" instead of 
   "unsigned long")

This is actually a potential issue already, with 64-bit PCI on regular 
PC's. We don't handle it at all right now (the PCI probing will just not 
create the resources), and nobody has complained, but clearly the 
RightThing(tm) to do eventually is to make sure this all works cleanly.

I just don't think it's worth worrying about in 2.6.x right now, since it 
doesn't matter for anybody. 

		Linus

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 09:41:37 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> No, I don't agree. The "iomem_resource" is there for all IO-mapped ranges, 
> and it could easily be a mixture of PCI and other buses. In fact, it 
> already is on bog-standard hardware: it contains the AGP windows etc.

Sure, but this model falls apart for things like I/O ports
and any other device access entity where one needs to use
special accessors to access stuff.

> So clearly ioremap() has to work for other buses too.

What if they are like I/O ports on x86 and require special
instructions to access?

> I think that in the 2.7.x timeframe, the right thing to do is definitely:
>  - move towards using "struct resource" and "ioremap_resource()"
>  - make resource sizes potentially be larger (ie use "u64" instead of 
>    "unsigned long")

Agreed.

Re: [PATCH] fix ppc ioremap prototype

[Linus Torvalds]

On Thu, 4 Sep 2003, David S. Miller wrote:
> 
> > So clearly ioremap() has to work for other buses too.
> 
> What if they are like I/O ports on x86 and require special
> instructions to access?

ioremap() is very easy to explain to a mathematician: its "domain" is
_exactly_ that which is in the "iomem_resource" tree. The "range" is a
virtual address.

In other words, something like I/O ports, that aren't in the 
iomem_resource, are not covered by ioremap(). It's that simple. 

Another way opf saying it: "iomem_resource" is one special set of
"physical resource mappings". The way you make sure they are accessible is 
"ioremap()".

This is how it has always been on x86, and it is self-consistent.

NOTE! That doesn't preclude having other resource trees, and other ways to
map those. We've never needed to have a "ioportremap()" for the
"ioport_resource" tree, because that resource is so limited that it can be
statically mapped (on x86 it's mapped by hardware, and on other
architectures it is often mapped into the virtual address space like iomem
is).

And some architectures may end up deciding that "iomem_resource" isn't 
sufficient for them, and want to maybe have _multiple_ totally independent 
address trees. Then you'd have to do something else.

		Linus

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 10:06:48 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:

> 
> On Thu, 4 Sep 2003, David S. Miller wrote:
> > 
> > > So clearly ioremap() has to work for other buses too.
> > 
> > What if they are like I/O ports on x86 and require special
> > instructions to access?
> 
> ioremap() is very easy to explain to a mathematician: its "domain" is
> _exactly_ that which is in the "iomem_resource" tree. The "range" is a
> virtual address.

A virtual address?  On x86 IOMEM resources are stored as physical
addresses and ioremap() returns a virtual mapping of that physical
address.

Maybe our wires are just crossed.

Re: [PATCH] fix ppc ioremap prototype

[Linus Torvalds]

On Thu, 4 Sep 2003, David S. Miller wrote:
> > ioremap() is very easy to explain to a mathematician: its "domain" is
> > _exactly_ that which is in the "iomem_resource" tree. The "range" is a
> > virtual address.
> 
> A virtual address?  On x86 IOMEM resources are stored as physical
> addresses and ioremap() returns a virtual mapping of that physical
> address.
> 
> Maybe our wires are just crossed.

No, I'm bad.

The "range" is not a virtual address, it's an offsettable cookie, useful
for read[bwl]() and write[bwl]() and the "memcpy_[to|from]io()" functions.

But the "domain" part was the important thing.

			Linus

Re: [PATCH] fix ppc ioremap prototype

[Jeff Garzik]

Linus Torvalds wrote:
> I think that in the 2.7.x timeframe, the right thing to do is definitely:
>  - move towards using "struct resource" and "ioremap_resource()"
>  - make resource sizes potentially be larger (ie use "u64" instead of 
>    "unsigned long")


Would still be nice to have a sysdata or struct device pointer in struct 
resource, then.  I'm not a fan of wacky 
bus-info-encoded-in-another-number schemes.

	Jeff

Re: [PATCH] fix ppc ioremap prototype

[Linus Torvalds]

On Thu, 4 Sep 2003, Jeff Garzik wrote:
> 
> Would still be nice to have a sysdata or struct device pointer in struct 
> resource, then.  I'm not a fan of wacky 
> bus-info-encoded-in-another-number schemes.

But it _isn't_ "bus info".

It's a unique number. It has no bus information embedded in it. It's a 
number that tells ioremap() what area to remap.

It's a "hardware dependent iomem address". What the address _means_ is 
entirely up to the architecture, and depends on how devices are acccessed, 
and what is most convenient to make them accessible (with the first level 
of that access translation being done "statically" by ioremap(), and the 
second level of the access translation being done by "read[bwl]()" and 
friends).

You could make "ioremap()" be a no-op and do all translation dynamically. 
Of you could make "ioremap()" be complex, and do no dynamic translation 
(x86). Or you can have a combination of the two.

The numbers don't have "meaning" per se. The inputs to "ioremap()" are 
"ranges of something". They are the exact same "somethings" as lives in 
"iomem_resource" "resources of something".

			Linus

Re: [PATCH] fix ppc ioremap prototype

[Eric W. Biederman]

Linus Torvalds <torvalds@osdl.org> writes:

> So clearly ioremap() has to work for other buses too.
> 
> I think that in the 2.7.x timeframe, the right thing to do is definitely:
>  - move towards using "struct resource" and "ioremap_resource()"
>  - make resource sizes potentially be larger (ie use "u64" instead of 
>    "unsigned long")
> 
> This is actually a potential issue already, with 64-bit PCI on regular 
> PC's. We don't handle it at all right now (the PCI probing will just not 
> create the resources), and nobody has complained, but clearly the 
> RightThing(tm) to do eventually is to make sure this all works cleanly.
> 
> I just don't think it's worth worrying about in 2.6.x right now, since it 
> doesn't matter for anybody. 

But it does.  There are at least some embedded ppc people who are actually
using 64bit physical addresses with a 32bit virtual address space.    

And this has come up in a few other times, as well.

A big question is how much of a problem this will be for later revs of
2.6.x.  I would even enable 64bit resources on an Opteron box and not
loose memory if I could be certain a 32bit kernel that people are
using temporarily would work.

So is there any reason to delay making resources a 64bit quantity?

Eric

Re: [PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

On Thu, Sep 04, 2003 at 01:12:28AM -0700, Linus Torvalds wrote:
> 
> On Thu, 4 Sep 2003, David S. Miller wrote:
> > 
> > My suggestion is to just pass a resource and an offset to ioremap().
> 
> Actually, my suggestion right now is to ignore the issue, and let the 
> current ppc440x code stand as-is. After all, it works, and it does what 
> the ppc people want. We may at some point switch over _all_ ioremap users, 
> but there is no real reason to do so right now.

So how should it work?  What basically all drivers to curretnly is
to have a unsigned long they get from pci_resource_start and pass it
to ioremap(), e.g. in tg3.c:

	unsigned long tg3reg_base, tg3reg_len;

	...

	tg3reg_base = pci_resource_start(pdev, 0);

	...

	tp->regs = (unsigned long) ioremap(tg3reg_base, tg3reg_len);

with the ppc4xx code you'd have to change the unsigned long to
a phys_addr_t to actually work with the high io addresses, which doesn't
exist on the other architectures.

Given that patch must make any sense (which I don't know as no one
even tried to explain it!) the pci code on ppc4xx doesn't actually use
the high bits of phys_addr_t.  But then this whole change to ioremap
doesn't make any sense and those arch-specific drivers should just use
a ioremap64 variant which seems to be present on ppc44x aswell..

Re: [PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

On Thu, Sep 04, 2003 at 01:09:40AM -0700, David S. Miller wrote:
> On Thu, 4 Sep 2003 09:38:45 +0200
> Christoph Hellwig <hch@lst.de> wrote:
> 
> > Umm, right, so the typedef name is also completly bogus, if we're
> > going to go that route it needs to be something likeb iocookie_t.
> 
> My suggestion is to just pass a resource and an offset to ioremap().

Here's a ioremap_resource implementation.  Tested on ppc32 with a
converted sungem driver.


--- 1.13/arch/ppc/mm/pgtable.c	Wed Sep  3 14:16:34 2003
+++ edited/arch/ppc/mm/pgtable.c	Thu Sep  4 10:53:17 2003
@@ -27,6 +27,7 @@
 #include <linux/vmalloc.h>
 #include <linux/init.h>
 #include <linux/highmem.h>
+#include <linux/ioport.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -62,6 +63,10 @@
 #define PGDIR_ORDER	0
 #endif
 
+#ifndef CONFIG_44x
+#define fixup_bigphys_addr(addr, size)	(addr)
+#endif
+
 pgd_t *pgd_alloc(struct mm_struct *mm)
 {
 	pgd_t *ret;
@@ -117,27 +122,18 @@
 	__free_page(pte);
 }
 
-#ifndef CONFIG_44x
 void *
-ioremap(phys_addr_t addr, unsigned long size)
+ioremap_resource(struct resource *res, unsigned long offset, unsigned long size)
 {
-	return __ioremap(addr, size, _PAGE_NO_CACHE);
-}
-#else /* CONFIG_44x */
-void *
-ioremap64(unsigned long long addr, unsigned long size)
-{
-	return __ioremap(addr, size, _PAGE_NO_CACHE);
+	return __ioremap(res->start + offset, size, _PAGE_NO_CACHE);
 }
 
 void *
-ioremap(phys_addr_t addr, unsigned long size)
+ioremap(unsigned long addr, unsigned long size)
 {
 	phys_addr_t addr64 = fixup_bigphys_addr(addr, size);;
-
-	return ioremap64(addr64, size);
+	return __ioremap(addr64, size, _PAGE_NO_CACHE);
 }
-#endif /* CONFIG_44x */
 
 void *
 __ioremap(phys_addr_t addr, unsigned long size, unsigned long flags)
--- 1.14/include/asm-alpha/io.h	Mon Feb 17 09:27:53 2003
+++ edited/include/asm-alpha/io.h	Thu Sep  4 10:40:25 2003
@@ -325,6 +325,7 @@
 {
 	return (void *) __ioremap(offset, size);
 } 
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 static inline void iounmap(void *addr)
 {
===== include/asm-arm/io.h 1.15 vs edited =====
--- 1.15/include/asm-arm/io.h	Sat Jan 25 19:32:47 2003
+++ edited/include/asm-arm/io.h	Thu Sep  4 10:40:34 2003
@@ -274,6 +274,7 @@
 #define ioremap_nocache(cookie,size)	__arch_ioremap((cookie),(size),0,1)
 #define iounmap(cookie)			__arch_iounmap(cookie)
 #endif
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 /*
  * can the hardware map this into one segment or not, given no other
===== include/asm-arm26/io.h 1.1 vs edited =====
--- 1.1/include/asm-arm26/io.h	Wed Jun  4 13:14:10 2003
+++ edited/include/asm-arm26/io.h	Thu Sep  4 10:40:44 2003
@@ -397,6 +397,7 @@
 #define ioremap_nocache(cookie,size)	__arch_ioremap((cookie),(size),0,1)
 #define iounmap(cookie)			__arch_iounmap(cookie)
 #endif
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 /*
  * DMA-consistent mapping functions.  These allocate/free a region of
===== include/asm-cris/io.h 1.8 vs edited =====
--- 1.8/include/asm-cris/io.h	Fri Jul  4 12:35:55 2003
+++ edited/include/asm-cris/io.h	Thu Sep  4 10:40:48 2003
@@ -24,6 +24,7 @@
 {
 	return __ioremap(offset, size, 0);
 }
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 extern void iounmap(void *addr);
 
===== include/asm-h8300/io.h 1.3 vs edited =====
--- 1.3/include/asm-h8300/io.h	Thu Aug 21 17:55:15 2003
+++ edited/include/asm-h8300/io.h	Thu Sep  4 10:40:55 2003
@@ -165,6 +165,7 @@
 {
 	return __ioremap(physaddr, size, IOMAP_FULL_CACHING);
 }
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 extern void iounmap(void *addr);
 
===== include/asm-i386/io.h 1.24 vs edited =====
--- 1.24/include/asm-i386/io.h	Mon Mar 24 18:23:37 2003
+++ edited/include/asm-i386/io.h	Thu Sep  4 10:41:01 2003
@@ -98,6 +98,7 @@
 #define page_to_phys(page)    ((dma_addr_t)page_to_pfn(page) << PAGE_SHIFT)
 
 extern void * __ioremap(unsigned long offset, unsigned long size, unsigned long flags);
+#define ioremap_resource(res, off, size)	__ioremap((res)->start + (off), (size), 0)
 
 /**
  * ioremap     -   map bus memory into CPU space
===== include/asm-ia64/io.h 1.17 vs edited =====
--- 1.17/include/asm-ia64/io.h	Wed Aug 20 08:13:39 2003
+++ edited/include/asm-ia64/io.h	Thu Sep  4 10:35:56 2003
@@ -383,6 +383,7 @@
 {
 	return (void *) (__IA64_UNCACHED_OFFSET | (offset));
 }
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 static inline void
 iounmap (void *addr)
===== include/asm-m68k/io.h 1.8 vs edited =====
--- 1.8/include/asm-m68k/io.h	Wed Mar  5 14:19:40 2003
+++ edited/include/asm-m68k/io.h	Thu Sep  4 10:36:30 2003
@@ -303,6 +303,7 @@
 {
 	return __ioremap(physaddr, size, IOMAP_FULL_CACHING);
 }
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 
 /* m68k caches aren't DMA coherent */
===== include/asm-m68knommu/io.h 1.2 vs edited =====
--- 1.2/include/asm-m68knommu/io.h	Thu Jul  3 03:18:30 2003
+++ edited/include/asm-m68knommu/io.h	Thu Sep  4 10:36:38 2003
@@ -157,6 +157,7 @@
 {
 	return __ioremap(physaddr, size, IOMAP_FULL_CACHING);
 }
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 extern void iounmap(void *addr);
 
===== include/asm-mips/io.h 1.5 vs edited =====
--- 1.5/include/asm-mips/io.h	Tue Jul 29 03:38:07 2003
+++ edited/include/asm-mips/io.h	Thu Sep  4 10:37:17 2003
@@ -190,6 +190,8 @@
  */
 #define ioremap(offset, size)						\
 	__ioremap((offset), (size), _CACHE_UNCACHED)
+#define ioremap_resource(res, off, size) \
+	__ioremap((res)->start + (off), (size), _CACHE_UNCACHED)
 
 /*
  * ioremap_nocache     -   map bus memory into CPU space
===== include/asm-parisc/io.h 1.4 vs edited =====
--- 1.4/include/asm-parisc/io.h	Sat Jan 11 17:46:56 2003
+++ edited/include/asm-parisc/io.h	Thu Sep  4 10:37:12 2003
@@ -26,6 +26,7 @@
 /* Memory mapped IO */
 
 extern void * __ioremap(unsigned long offset, unsigned long size, unsigned long flags);
+#define ioremap_resource(res, off, size)	__ioremap((res)->start + (off), (size), 0)
 
 extern inline void * ioremap(unsigned long offset, unsigned long size)
 {
===== include/asm-ppc/io.h 1.14 vs edited =====
--- 1.14/include/asm-ppc/io.h	Wed Sep  3 14:16:34 2003
+++ edited/include/asm-ppc/io.h	Thu Sep  4 10:46:59 2003
@@ -199,12 +199,12 @@
  * Map in an area of physical address space, for accessing
  * I/O devices etc.
  */
+struct resource;
 extern void *__ioremap(phys_addr_t address, unsigned long size,
 		       unsigned long flags);
-extern void *ioremap(phys_addr_t address, unsigned long size);
-#ifdef CONFIG_44x
-extern void *ioremap64(unsigned long long address, unsigned long size);
-#endif
+extern void *ioremap_resource(struct resource *res, unsigned long offset,
+			      unsigned long size);
+extern void *ioremap(unsigned long address, unsigned long size);
 #define ioremap_nocache(addr, size)	ioremap((addr), (size))
 extern void iounmap(void *addr);
 extern unsigned long iopa(unsigned long addr);
===== include/asm-ppc64/io.h 1.8 vs edited =====
--- 1.8/include/asm-ppc64/io.h	Sun Apr 27 23:32:15 2003
+++ edited/include/asm-ppc64/io.h	Thu Sep  4 10:37:44 2003
@@ -118,6 +118,7 @@
 		       unsigned long flags);
 extern void *ioremap(unsigned long address, unsigned long size);
 #define ioremap_nocache(addr, size)	ioremap((addr), (size))
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 extern void iounmap(void *addr);
 
 /*
===== include/asm-s390/io.h 1.6 vs edited =====
--- 1.6/include/asm-s390/io.h	Mon Apr 14 21:11:58 2003
+++ edited/include/asm-s390/io.h	Thu Sep  4 10:38:00 2003
@@ -53,6 +53,7 @@
 #define page_to_phys(page)	((page - mem_map) << PAGE_SHIFT)
 
 extern void * __ioremap(unsigned long offset, unsigned long size, unsigned long flags);
+#define ioremap_resource(res, off, size)	__ioremap((res)->start + (off), (size), 0)
 
 extern inline void * ioremap (unsigned long offset, unsigned long size)
 {
===== include/asm-sh/io.h 1.5 vs edited =====
--- 1.5/include/asm-sh/io.h	Sun May  4 17:30:01 2003
+++ edited/include/asm-sh/io.h	Thu Sep  4 10:39:18 2003
@@ -72,6 +72,8 @@
 # define __ioremap(a,s)	sh_mv.mv_ioremap((a), (s))
 # define __iounmap(a)	sh_mv.mv_iounmap((a))
 
+# define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
+
 # define __isa_port2addr(a)	sh_mv.mv_isa_port2addr(a)
 
 # define inb		__inb
@@ -399,6 +401,7 @@
 {
 	return __ioremap(offset, size);
 }
+#define ioremap_resource(res, off, size)	__ioremap((res)->start + (off), (size))
 
 static __inline__ void iounmap(void *addr)
 {
===== include/asm-sparc/io.h 1.5 vs edited =====
--- 1.5/include/asm-sparc/io.h	Mon Dec  2 09:23:54 2002
+++ edited/include/asm-sparc/io.h	Thu Sep  4 10:39:56 2003
@@ -176,7 +176,8 @@
  * This is why we have no bus number argument to ioremap().
  */
 extern void *ioremap(unsigned long offset, unsigned long size);
-#define ioremap_nocache(X,Y)	ioremap((X),(Y))
+#define ioremap_nocache(X,Y)	ioremap((X),(Y)) 
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 extern void iounmap(void *addr);
 
 /*
===== include/asm-sparc64/io.h 1.7 vs edited =====
--- 1.7/include/asm-sparc64/io.h	Sat Dec 21 03:02:57 2002
+++ edited/include/asm-sparc64/io.h	Thu Sep  4 10:40:03 2003
@@ -435,6 +435,7 @@
  */
 #define ioremap(__offset, __size)	((void *)(__offset))
 #define ioremap_nocache(X,Y)		ioremap((X),(Y))
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 #define iounmap(__addr)			do { (void)(__addr); } while(0)
 
 /* Similarly for SBUS. */
===== include/asm-v850/io.h 1.2 vs edited =====
--- 1.2/include/asm-v850/io.h	Tue Jun 17 07:23:12 2003
+++ edited/include/asm-v850/io.h	Thu Sep  4 10:40:13 2003
@@ -97,6 +97,7 @@
 #define ioremap_nocache(physaddr, size)		(physaddr)
 #define ioremap_writethrough(physaddr, size)	(physaddr)
 #define ioremap_fullcache(physaddr, size)	(physaddr)
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 #define page_to_phys(page)      ((page - mem_map) << PAGE_SHIFT)
 #if 0
===== include/asm-x86_64/io.h 1.9 vs edited =====
--- 1.9/include/asm-x86_64/io.h	Fri Jul 11 13:34:21 2003
+++ edited/include/asm-x86_64/io.h	Thu Sep  4 10:40:18 2003
@@ -152,6 +152,7 @@
 {
 	return __ioremap(offset, size, 0);
 }
+#define ioremap_resource(res, off, size)	ioremap((res)->start + (off), (size))
 
 /*
  * This one maps high address device memory and turns off caching for that area.

Re: [PATCH] fix ppc ioremap prototype

[Benjamin Herrenschmidt]

On Thu, 2003-09-04 at 11:29, Christoph Hellwig wrote:
> On Thu, Sep 04, 2003 at 01:09:40AM -0700, David S. Miller wrote:
> > On Thu, 4 Sep 2003 09:38:45 +0200
> > Christoph Hellwig <hch@lst.de> wrote:
> > 
> > > Umm, right, so the typedef name is also completly bogus, if we're
> > > going to go that route it needs to be something likeb iocookie_t.
> > 
> > My suggestion is to just pass a resource and an offset to ioremap().
> 
> Here's a ioremap_resource implementation.  Tested on ppc32 with a
> converted sungem driver.

Please, make so that ioremap_resource can be passed a PCI IO resource
as well and just do nothing in this case.

Ben.

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Russell King writes:

> But phys_addr_t in struct resource and being passed into ioremap is
> confusing.  Apparantly, it isn't a physical address, but a platform
> defined cookie which just happens to look like a physical address.

It happens to be a physical address on PPC.  So I don't see why so
much fuss is being made over the PPC declaration of ioremap using
phys_addr_t for it.  Other architectures might well do something
different.

> (or are we finally going to admit that it is a physical address?)

Well, it has to be globally unique across all resources of all devices
in the system.  If you can have multiple PCI domains, that means you
can't just use the PCI bus address, for instance.  And it is
preferably something that you can easily translate into a physical
address.

What I would prefer is if we passed a struct device pointer, a
resource pointer and an offset to ioremap.  Then we could just have
bus addresses in PCI device resources instead of having to translate
them into physical addresses.

Paul.

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 19:21:34 +1000 (EST)
Paul Mackerras <paulus@samba.org> wrote:

> What I would prefer is if we passed a struct device pointer, a
> resource pointer and an offset to ioremap.  Then we could just have
> bus addresses in PCI device resources instead of having to translate
> them into physical addresses.

You only need a resource in order to do this.  Then you can
stick the upper bits, controller number, whatever in the unused
resource flag bits.

Re: [PATCH] fix ppc ioremap prototype

[Russell King]

On Thu, Sep 04, 2003 at 02:36:24AM -0700, David S. Miller wrote:
> On Thu, 4 Sep 2003 19:21:34 +1000 (EST)
> Paul Mackerras <paulus@samba.org> wrote:
> 
> > What I would prefer is if we passed a struct device pointer, a
> > resource pointer and an offset to ioremap.  Then we could just have
> > bus addresses in PCI device resources instead of having to translate
> > them into physical addresses.
> 
> You only need a resource in order to do this.  Then you can
> stick the upper bits, controller number, whatever in the unused
> resource flag bits.

Using the high flag bits probably isn't a good idea for two reasons:

1. We already use bit 31 to indicate the busy status:

   #define IORESOURCE_BUSY         0x80000000      /* Driver has marked this resource busy */

   However, it looks like bits 27 to 17 are currently unused.

2. The resource tree won't know about the upper bits or whatever sitting
   in flags, and as such identical addresses on two different buses will
   clash.

Resource start,end needs to be some unique quantity no matter which (PCI)
bus you are on.

-- 
Russell King (rmk@arm.linux.org.uk)                The developer of ARM Linux
             http://www.arm.linux.org.uk/personal/aboutme.html

Re: [PATCH] fix ppc ioremap prototype

[Christoph Hellwig]

On Thu, Sep 04, 2003 at 10:48:01AM +0100, Russell King wrote:
> 2. The resource tree won't know about the upper bits or whatever sitting
>    in flags, and as such identical addresses on two different buses will
>    clash.
> 
> Resource start,end needs to be some unique quantity no matter which (PCI)
> bus you are on.

Yupp.  This makes me question again how the phys_addr_t thing is
supposed to work at all given struct resource uses unsigned long
for start and len, so the whole generic resource infrastructure
doesn't know about the higher bits...

Could someone point me a to a driver actually making use of the
extented ioremap address on ppc 44x?

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Christoph Hellwig writes:

> Yupp.  This makes me question again how the phys_addr_t thing is
> supposed to work at all given struct resource uses unsigned long
> for start and len, so the whole generic resource infrastructure
> doesn't know about the higher bits...

That's what fixup_bigphys_addr is for.  Basically, on the 440 the
first 4GB of physical address space is all RAM.  PCI memory space
occupies 2GB from 380000000 - 3ffffffff.  So if ioremap is given an
address between 2GB and 4GB, it is assumed to be from a PCI driver,
and fixup_bigphys_addr adds on the 0x300000000.

> Could someone point me a to a driver actually making use of the
> extented ioremap address on ppc 44x?

drivers/net/ibm_emac/ibm_ocp_enet.c in 2.4 does - it's an ocp driver,
not a pci driver.  Yes, we owe Jeff Garzik a 2.5 version of that
driver.

Paul.

Re: [PATCH] fix ppc ioremap prototype

[Paul Mackerras]

Russell King writes:

> Using the high flag bits probably isn't a good idea for two reasons:
> 
> 1. We already use bit 31 to indicate the busy status:
> 
>    #define IORESOURCE_BUSY         0x80000000      /* Driver has marked this resource busy */
> 
>    However, it looks like bits 27 to 17 are currently unused.

Using some flag bits would work but it seems like a bit of a kludge.
Maybe the struct resource needs to have a pointer to the struct device
which owns it?  Or maybe just a `sysdata' field?

> 2. The resource tree won't know about the upper bits or whatever sitting
>    in flags, and as such identical addresses on two different buses will
>    clash.
> 
> Resource start,end needs to be some unique quantity no matter which (PCI)
> bus you are on.

They are non-overlapping for PCI buses in the same domain.  Perhaps
the sensible thing is to have a separate resource tree for each PCI
domain (actually two trees, for I/O and memory space), and have them
contain bus addresses rather than physical addresses.  I don't know if
the generic iomem_resource and ioport_resource are still useful if we
do that.

Paul.

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 22:59:33 +1000 (EST)
Paul Mackerras <paulus@samba.org> wrote:

> Perhaps the sensible thing is to have a separate resource tree for
> each PCI domain (actually two trees, for I/O and memory space),
> and have them contain bus addresses rather than physical addresses.

I disagree, I think the current model where all resources are
in a global space makes more sense.

kernel/resource.c would get more complex with your idea.

I'd suggest making an ocp_ioremap() to solve this problem for
the time being.

Re: [PATCH] fix ppc ioremap prototype

[Matt Porter]

On Thu, Sep 04, 2003 at 06:01:39AM -0700, David S. Miller wrote:
> On Thu, 4 Sep 2003 22:59:33 +1000 (EST)
> Paul Mackerras <paulus@samba.org> wrote:
> 
> > Perhaps the sensible thing is to have a separate resource tree for
> > each PCI domain (actually two trees, for I/O and memory space),
> > and have them contain bus addresses rather than physical addresses.
> 
> I disagree, I think the current model where all resources are
> in a global space makes more sense.

The global space method doesn't allow for proper resource management
in systems with a small floating physical address window but a huge
bus address space to be accessed.  MPC8245's DAC scheme is a good
example of this as well as numerous Xscale PCI implementations.
Paul's suggestion would allow the removal of lots of ugly hacks.

-Matt

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 07:15:35 -0700
Matt Porter <mporter@kernel.crashing.org> wrote:

> The global space method doesn't allow for proper resource management
> in systems with a small floating physical address window but a huge
> bus address space to be accessed.  MPC8245's DAC scheme is a good
> example of this as well as numerous Xscale PCI implementations.
> Paul's suggestion would allow the removal of lots of ugly hacks.

There is nothing that cannot be represented with a big integer.
Encode the "window" in the upper bits, or whatever, get creative.

Re: [PATCH] fix ppc ioremap prototype

[Russell King]

On Thu, Sep 04, 2003 at 10:59:33PM +1000, Paul Mackerras wrote:
> > 2. The resource tree won't know about the upper bits or whatever sitting
> >    in flags, and as such identical addresses on two different buses will
> >    clash.
> > 
> > Resource start,end needs to be some unique quantity no matter which (PCI)
> > bus you are on.
> 
> They are non-overlapping for PCI buses in the same domain.  Perhaps
> the sensible thing is to have a separate resource tree for each PCI
> domain (actually two trees, for I/O and memory space), and have them
> contain bus addresses rather than physical addresses.  I don't know if
> the generic iomem_resource and ioport_resource are still useful if we
> do that.

I thought I pointed out that this approach would break request_region
and request_mem_region, which are the work-horses of the "this region
of space is busy".

Someone would have to (somehow) fix up all drivers which use those
functions...

-- 
Russell King (rmk@arm.linux.org.uk)	http://www.arm.linux.org.uk/personal/
Maintainer of:
  2.6 ARM Linux   - http://www.arm.linux.org.uk/
  2.6 PCMCIA      - http://pcmcia.arm.linux.org.uk/
  2.6 Serial core

Re: [PATCH] fix ppc ioremap prototype

[Deepak Saxena]

On Sep 04 2003, at 22:59, Paul Mackerras was caught saying:
> Russell King writes:
> 
> > Using the high flag bits probably isn't a good idea for two reasons:
> > 
> > 1. We already use bit 31 to indicate the busy status:
> > 
> >    #define IORESOURCE_BUSY         0x80000000      /* Driver has marked this resource busy */
> > 
> >    However, it looks like bits 27 to 17 are currently unused.
> 
> Using some flag bits would work but it seems like a bit of a kludge.
> Maybe the struct resource needs to have a pointer to the struct device
> which owns it?  Or maybe just a `sysdata' field?
> 
> > 2. The resource tree won't know about the upper bits or whatever sitting
> >    in flags, and as such identical addresses on two different buses will
> >    clash.
> > 
> > Resource start,end needs to be some unique quantity no matter which (PCI)
> > bus you are on.
> 
> They are non-overlapping for PCI buses in the same domain.  Perhaps
> the sensible thing is to have a separate resource tree for each PCI
> domain (actually two trees, for I/O and memory space), and have them
> contain bus addresses rather than physical addresses.  I don't know if
> the generic iomem_resource and ioport_resource are still useful if we
> do that.

I think we need to a have a resource tree per _bus_, not just PCI.
I have systems which have overlapping devices in multiple PCI domains
and devices on the local memory bus that also overlap. Before someone
yells at me about ioremap() only being for PCI memory resources,
it's already used throughout the kernel for locally mapped devices.

Ideally, the resource "system" as it exists would be rewritten
with something that ties into the device tree a bit by having
each resource being owned by a device.  One then calls
ioremap_resource() which on platforms that need it can look
at the resource->dev ptr to determine how to map that resource.

~Deepak

-- 
Deepak Saxena
MontaVista Software - Powering the Embedded Revolution - www.mvista.com

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 08:50:04 -0700
Deepak Saxena <dsaxena@mvista.com> wrote:

> I think we need to a have a resource tree per _bus_, not just PCI.
> I have systems which have overlapping devices in multiple PCI domains
> and devices on the local memory bus that also overlap.

The physical address ranges are unique, I really don't see
any problem.

Re: [PATCH] fix ppc ioremap prototype

[Alan Cox]

On Iau, 2003-09-04 at 10:36, David S. Miller wrote:
> You only need a resource in order to do this.  Then you can
> stick the upper bits, controller number, whatever in the unused
> resource flag bits.

If it becomes the default approach over time then we also need a 
version that allows offset/len to be included for mapping parts
of very large objects (like 256Mb frame buffers)

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 04 Sep 2003 11:34:30 +0100
Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> On Iau, 2003-09-04 at 10:36, David S. Miller wrote:
> > You only need a resource in order to do this.  Then you can
> > stick the upper bits, controller number, whatever in the unused
> > resource flag bits.
> 
> If it becomes the default approach over time then we also need a 
> version that allows offset/len to be included for mapping parts
> of very large objects (like 256Mb frame buffers)

The implication was that the args would be:

	resource, offset, len

which would give what you want.

Re: [PATCH] fix ppc ioremap prototype

[Jeff Garzik]

Alan Cox wrote:
> On Iau, 2003-09-04 at 10:36, David S. Miller wrote:
> 
>>You only need a resource in order to do this.  Then you can
>>stick the upper bits, controller number, whatever in the unused
>>resource flag bits.
> 
> 
> If it becomes the default approach over time then we also need a 
> version that allows offset/len to be included for mapping parts
> of very large objects (like 256Mb frame buffers)


ioremap() has long wanted a struct pci_dev argument too.
(or make that struct device, now)

	Jeff

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 04 Sep 2003 08:42:11 -0400
Jeff Garzik <jgarzik@pobox.com> wrote:

> ioremap() has long wanted a struct pci_dev argument too.
> (or make that struct device, now)

I majorly disagree.

If you can't do it with a resource/offset/len triple,
a struct device isn't going to help you much more.

If you need to get at the device, you can convert the
resource to the controller address range or even encode
the controller domain number in the resource somehow.

Re: [PATCH] fix ppc ioremap prototype

[Matt Porter]

On Thu, Sep 04, 2003 at 02:36:24AM -0700, David S. Miller wrote:
> On Thu, 4 Sep 2003 19:21:34 +1000 (EST)
> Paul Mackerras <paulus@samba.org> wrote:
> 
> > What I would prefer is if we passed a struct device pointer, a
> > resource pointer and an offset to ioremap.  Then we could just have
> > bus addresses in PCI device resources instead of having to translate
> > them into physical addresses.
> 
> You only need a resource in order to do this.  Then you can
> stick the upper bits, controller number, whatever in the unused
> resource flag bits.

Ok, now the other part of making PCI devices work is to support
mmap.  In most cases, that means remap_page_range() is used which
is stuck with an unsigned long physical address.  For example,
should we really have a remap_resource_range() in FB drivers to
handle mmap?  This could use arch-specific resource information
to get a 36-bit physical address (PPC44x) and maybe get rid of
some of the in-driver per-arch address munging.

My local tree has an ugly hack to remap_page_range() (and friends)
so it uses a phys_addr_t and calls fixup_bigphys_addr() to allow
use of unmodified PCI FB drivers.  I'd like to get this working
without hacks. :)

-Matt

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 07:36:50 -0700
Matt Porter <mporter@kernel.crashing.org> wrote:

> Ok, now the other part of making PCI devices work is to support
> mmap.

You get the pci device in the arch PCI mmap() routines, what
more do you need?  Grep for HAVE_PCI_MMAP in the source tree
and how sparc64 implements that.

Re: [PATCH] fix ppc ioremap prototype

[Matt Porter]

On Thu, Sep 04, 2003 at 07:30:09AM -0700, David S. Miller wrote:
> On Thu, 4 Sep 2003 07:36:50 -0700
> Matt Porter <mporter@kernel.crashing.org> wrote:
> 
> > Ok, now the other part of making PCI devices work is to support
> > mmap.
> 
> You get the pci device in the arch PCI mmap() routines, what
> more do you need?  Grep for HAVE_PCI_MMAP in the source tree
> and how sparc64 implements that.

That will work for the PCI case.  It does force us to maintain a
remap_page_range64() in arch/ppc, duplicating code, and creating a
maintainer headache.  I don't think we can hack set_pte() (so as
to call the standard remap_page_range()) to trap and add the upper
bits because there is not enough context to make the right decision.
We've got to have the remap_page_range64() for PPC4xx-specific OCP
drivers too.

A remap_page_range() that even took a paddr_t would be helpful if
not some resource based remapper.

I suppose we could even use the HAVE_PCI_MMAP approach to make
mem.c usable with a PPC44x specific hack.  Of course, a
remap_page_range() with a paddr_t arg would also help here. 

-Matt

Re: [PATCH] fix ppc ioremap prototype

[David S. Miller]

On Thu, 4 Sep 2003 08:26:05 -0700
Matt Porter <mporter@kernel.crashing.org> wrote:

> That will work for the PCI case.  It does force us to maintain a
> remap_page_range64() in arch/ppc, duplicating code, and creating a
> maintainer headache.

We've had an io_remap_page_range() with an extra "space"
argument for the >32bits of the physical address on sparc32
for ages exactly for this purpose.

Re: [PATCH] fix ppc ioremap prototype

[Linus Torvalds]

On Thu, 4 Sep 2003, Matt Porter wrote:
> 
> My local tree has an ugly hack to remap_page_range() (and friends)
> so it uses a phys_addr_t and calls fixup_bigphys_addr() to allow
> use of unmodified PCI FB drivers.  I'd like to get this working
> without hacks. :)

We could make a new remap_page_range() that takes the FPN (not the 
address, the page "number") instead. But it would need a new name, not a 
flag-day "change all users".

		Linus