fix ppc ioremap prototype
PATCH 1/1 fix ppc ioremap prototype
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..
arch/ppc/mm/pgtable.c +4 -4
@@ -119,9 +119,9 @@#ifndef CONFIG_44xvoid * -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); }
include/asm-ppc/io.h +1 -1
@@ -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_44xextern void *ioremap64 (unsigned long long address,unsigned long size);#endif
Quoted 2 lines · Christoph Hellwig
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.
Quoted 2 lines
/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.
Quoted 4 lines · Paul Mackerras
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.
Quoted 7 lines
Quoted 2 lines
/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_develtree. Lkml is not where most PPC-specific stuff gets discussed, it's too noisy for that.
ioremap is not ppc-specific.
Quoted 9 lines · On Thu, 4 Sep 2003 09:13:34 +0200 Christoph Hellwig <[email protected]> wrote:
Quoted 4 lines · Paul Mackerras
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_tto 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.
Quoted 9 lines · Christoph Hellwig
Quoted 4 lines · Paul Mackerras
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_tto 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?)
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 5 lines · Russell King
But
phys_addr_tin 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..
Quoted 2 lines · On Thu, 4 Sep 2003 09:38:45 +0200 Christoph Hellwig <[email protected]> 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().
Quoted 2 lines · David S. Miller
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
Quoted 4 lines · On Thu, 4 Sep 2003 01:12:28 -0700 (PDT) Linus Torvalds <[email protected]>…
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.
Quoted 9 lines · Linus Torvalds
Quoted 2 lines · David S. Miller
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..
Quoted 2 lines · David S. Miller
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?
Quoted 3 lines · Russell King
But
phys_addr_tin 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.
Quoted 1 line
(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.
Quoted 7 lines · David S. Miller
Quoted 2 lines · On Thu, 4 Sep 2003 09:38:45 +0200 Christoph Hellwig <[email protected]> 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.
arch/ppc/mm/pgtable.c +9 -13
@@ -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_44xvoid * -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)
include/asm-alpha/io.h +1 -0
@@ -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 =====
include/asm-arm/io.h +1 -0
@@ -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 =====
include/asm-arm26/io.h +1 -0
@@ -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 =====
include/asm-cris/io.h +1 -0
@@ -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 =====
include/asm-h8300/io.h +1 -0
@@ -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 =====
include/asm-i386/io.h +1 -0
@@ -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 =====
include/asm-ia64/io.h +1 -0
@@ -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 =====
include/asm-m68k/io.h +1 -0
@@ -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 =====
include/asm-m68knommu/io.h +1 -0
@@ -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 =====
include/asm-mips/io.h +2 -0
@@ -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 =====
include/asm-parisc/io.h +1 -0
@@ -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 =====
include/asm-ppc/io.h +4 -4
@@ -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 =====
include/asm-ppc64/io.h +1 -0
@@ -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 =====
include/asm-s390/io.h +1 -0
@@ -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 =====
include/asm-sh/io.h +3 -0
@@ -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 =====
include/asm-sparc/io.h +2 -1
@@ -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 =====
include/asm-sparc64/io.h +1 -0
@@ -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 =====
include/asm-v850/io.h +1 -0
@@ -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 =====
include/asm-x86_64/io.h +1 -0
@@ -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.
Quoted 4 lines · On Thu, 4 Sep 2003 19:21:34 +1000 (EST) Paul Mackerras <[email protected]> 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.
Quoted 11 lines · David S. Miller
Quoted 4 lines · On Thu, 4 Sep 2003 19:21:34 +1000 (EST) Paul Mackerras <[email protected]> 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.
Signature · 2 lines
Russell King ([email protected]) The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html
Quoted 6 lines · Russell King
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?
Quoted 8 lines · Christoph Hellwig
Quoted 2 lines · David S. Miller
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.
Quoted 5 lines
This still doesn't make the
phys_addr_ta good interims solution, though. Just useioremap_resourcefrom 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.
Quoted 11 lines · Christoph Hellwig
Quoted 7 lines · David S. Miller
Quoted 2 lines · On Thu, 4 Sep 2003 09:38:45 +0200 Christoph Hellwig <[email protected]> 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_resourceimplementation. 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.
Quoted 3 lines · David S. Miller
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)
Quoted 8 lines · On Thu, 04 Sep 2003 11:34:30 +0100 Alan Cox <[email protected]> wrote:
Quoted 3 lines · David S. Miller
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.
Quoted 3 lines · Christoph Hellwig
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.
Quoted 5 lines
This still doesn't make the
phys_addr_ta good interims solution, though. Just useioremap_resourcefrom 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.
Quoted 8 lines · Paul Mackerras
Quoted 1 line
Paul, what does actually use this higher addresses?
We have drivers for on-chip peripherals that work from a struct
ocp_deviceand call ioremap on theocp_dev->paddrvalue, which is aphys_addr_t(although some of them use__ioremapinstead). These drivers are used on 405-based systems (with 32-bitphys_addr_t) as well as on 440-based systems.
Quoted 5 lines
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 -- [email protected]
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
Quoted 4 lines · Christoph Hellwig
Yupp. This makes me question again how the
phys_addr_tthing 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.
Quoted 2 lines
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.
Quoted 1 line · Geert Uytterhoeven
`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.
Quoted 9 lines · Alan Cox
Quoted 3 lines · David S. Miller
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
Quoted 6 lines · On Thu, 4 Sep 2003 22:41:31 +1000 (EST) Paul Mackerras <[email protected]> wrote:
Quoted 1 line · Geert Uytterhoeven
`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.
Quoted 2 lines · On Thu, 04 Sep 2003 08:42:11 -0400 Jeff Garzik <[email protected]> wrote:
ioremap()has long wanted a structpci_devargument 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.
Quoted 5 lines · Paul Mackerras
Quoted 1 line · Geert Uytterhoeven
`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 -- [email protected]
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
Quoted 7 lines · Russell King
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?
Quoted 6 lines
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.
Quoted 3 lines · On Thu, 4 Sep 2003 22:59:33 +1000 (EST) Paul Mackerras <[email protected]> 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.
Quoted 7 lines · Paul Mackerras
Quoted 6 lines
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.
Quoted 6 lines
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_resourceandioport_resourceare 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...
Signature · 5 lines
Russell King ([email protected]) 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
Quoted 9 lines · David S. Miller
Quoted 3 lines · On Thu, 4 Sep 2003 22:59:33 +1000 (EST) Paul Mackerras <[email protected]> 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
Quoted 5 lines · On Thu, 4 Sep 2003 07:15:35 -0700 Matt Porter <[email protected]>…
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.
Quoted 11 lines · David S. Miller
Quoted 4 lines · On Thu, 4 Sep 2003 19:21:34 +1000 (EST) Paul Mackerras <[email protected]> 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
Quoted 2 lines · On Thu, 4 Sep 2003 07:36:50 -0700 Matt Porter <[email protected]>…
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.
Quoted 9 lines · David S. Miller
Quoted 2 lines · On Thu, 4 Sep 2003 07:36:50 -0700 Matt Porter <[email protected]>…
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 forHAVE_PCI_MMAPin 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
Quoted 5 lines · Matt Porter
My local tree has an ugly hack to
remap_page_range()(and friends) so it uses aphys_addr_tand callsfixup_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
On Sep 04 2003, at 22:59, Paul Mackerras was caught saying:
Quoted 21 lines
Quoted 7 lines · Russell King
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?
Quoted 6 lines
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.
Quoted 6 lines
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_resourceandioport_resourceare 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
Signature · 2 lines
Deepak Saxena MontaVista Software - Powering the Embedded Revolution - www.mvista.com
On Sep 04 2003, at 14:57, Geert Uytterhoeven was caught saying:
Quoted 7 lines · Paul Mackerras
Quoted 5 lines
Quoted 1 line · Geert Uytterhoeven
`ioremap is meant for PCI memory space only'
Did I say that, or someone else? :) ioremap predates PCI support by a long way IIRC...
Quoted 8 lines
inb()and friends are for ISA/PCI I/O spaceisa_readb()and friends are for ISA memory spacereadb()and friends are for PCI memory space (afterioremap())That's why other buses (e.g. SBUS and Zorro) have their own versions of
ioremap()andreadb()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
Signature · 2 lines
Deepak Saxena MontaVista Software - Powering the Embedded Revolution - www.mvista.com
Quoted 3 lines · On Thu, 4 Sep 2003 08:26:05 -0700 Matt Porter <[email protected]>…
That will work for the PCI case. It does force us to maintain a
remap_page_range64()inarch/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.
Quoted 17 lines · Deepak Saxena
On Sep 04 2003, at 14:57, Geert Uytterhoeven was caught saying:
Quoted 7 lines · Paul Mackerras
Quoted 5 lines
Quoted 1 line · Geert Uytterhoeven
`ioremap is meant for PCI memory space only'
Did I say that, or someone else? :) ioremap predates PCI support by a long way IIRC...
Quoted 8 lines
inb()and friends are for ISA/PCI I/O spaceisa_readb()and friends are for ISA memory spacereadb()and friends are for PCI memory space (afterioremap())That's why other buses (e.g. SBUS and Zorro) have their own versions of
ioremap()andreadb()etc.).Life would be much easier with bus-specific I/O ops...
Quoted 10 lines
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 -- [email protected]
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
Quoted 3 lines · On Thu, 4 Sep 2003 08:50:04 -0700 Deepak Saxena <[email protected]> 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.
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Deepak" == Deepak Saxena <[email protected]> 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
Quoted 2 lines · Geert Uytterhoeven
`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
Quoted 3 lines · On Thu, 4 Sep 2003 09:41:37 -0700 (PDT) Linus Torvalds <[email protected]>…
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.
Quoted 1 line
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?
Quoted 4 lines
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.
Quoted 5 lines · David S. Miller
Quoted 2 lines
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
Quoted 4 lines · Linus Torvalds
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
Quoted 11 lines · On Thu, 4 Sep 2003 10:06:48 -0700 (PDT) Linus Torvalds <[email protected]>…
Quoted 5 lines · David S. Miller
Quoted 2 lines
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.
Quoted 9 lines · David S. Miller
Quoted 3 lines
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
Quoted 4 lines · Jeff Garzik
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
Quoted 6 lines · Geert Uytterhoeven
inb()and friends are for ISA/PCI I/O spaceisa_readb()and friends are for ISA memory spacereadb()and friends are for PCI memory space (afterioremap())That's why other buses (e.g. SBUS and Zorro) have their own versions of
ioremap()andreadb()etc.).
I suggest you fix Documentation/zorro.txt then: 8-)
: Writing Device Drivers for Zorro Devices
: ----------------------------------------
:
: Written by Geert Uytterhoeven <[email protected]> : 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.
Quoted 7 lines · Linus Torvalds
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")
Quoted 7 lines
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
Quoted 8 lines · Paul Mackerras
Quoted 6 lines · Geert Uytterhoeven
inb()and friends are for ISA/PCI I/O spaceisa_readb()and friends are for ISA memory spacereadb()and friends are for PCI memory space (afterioremap())That's why other buses (e.g. SBUS and Zorro) have their own versions of
ioremap()andreadb()etc.).
Quoted 7 lines
I suggest you fix
Documentation/zorro.txtthen: 8-): Writing Device Drivers for Zorro Devices
: ----------------------------------------
:
: Written by Geert Uytterhoeven <[email protected]> : 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 -- [email protected]
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