video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64

ioremap_uc() is only meaningful on old x86-32 systems with the PAT extension, and on ia64 with its slightly unconventional ioremap() behavior, everywhere else this is the same as ioremap() anyway.

Change the only driver that still references ioremap_uc() to only do so on x86-32/ia64 in order to allow removing that interface at some point in the future for the other architectures.

On some architectures, ioremap_uc() just returns NULL, changing the driver to call ioremap() means that they now have a chance of working correctly.

From 81243b2aa78babcc5f97b2c2a29957fcf3fd3664 Mon Sep 17 00:00:00 2001

Quoted 11 lines · Arnd Bergmann

ioremap_uc() is only meaningful on old x86-32 systems with the PAT extension, and on ia64 with its slightly unconventional ioremap() behavior, everywhere else this is the same as ioremap() anyway.

Change the only driver that still references ioremap_uc() to only do so on x86-32/ia64 in order to allow removing that interface at some point in the future for the other architectures.

On some architectures, ioremap_uc() just returns NULL, changing the driver to call ioremap() means that they now have a chance of working correctly.

So this whole area is a bit of a mess. ioremap_uc on ia64 is brand new in this cycle, for ia64-internal users that were previously using ioremap_nocache. And ioremap_nocache was identical to ioremap everywhere but ia64. So I think we can safely skip ia64 in the ifdef if we go down that route.

But also on x86 I'd actually rather prefer killing off this only mainline user of ioremap_uc. It was added by Luis in commit 3cc2dac5be3f ("drivers/video/fbdev/atyfb: Replace MTRR UC hole with strong UC", which looks like an optimization of MTRR usage. I feel like I really don't understand the point there, but it also seems a pity to keep the API around just for that.

That being said linux-next added another call to ioremap_uc in drivers/mfd/intel-lpss.c, so it looks like the API might have to stay.

So I guess modulo excluding ia64 your patch looks fine, and should go along something like the one below. I'll happily carry them in the ioremap tree with the right ACKs.

---
From 81243b2aa78babcc5f97b2c2a29957fcf3fd3664 Mon Sep 17 00:00:00 2001
From: Christoph Hellwig <[email protected]>
Date: Tue, 12 Nov 2019 11:52:25 +0100
Subject: ioremap: remove ioremap_uc except on x86 and ia64

ioremap_uc is a special API to work around caching oddities on Intel platforms. Remove it from all other architectures now that only x86 and ia64-specific callers are left.

Signed-off-by Christoph Hellwig <[email protected]>

---
 arch/alpha/include/asm/io.h    |  1 -
 arch/m68k/include/asm/kmap.h   |  1 -
 arch/mips/include/asm/io.h     |  1 -
 arch/parisc/include/asm/io.h   |  1 -
 arch/powerpc/include/asm/io.h  |  1 -
 arch/sh/include/asm/io.h       |  1 -
 arch/sparc/include/asm/io_64.h |  1 -
 include/asm-generic/io.h       | 15 ---------------
 include/linux/io.h             |  2 ++
 lib/devres.c                   |  4 ++++
 10 files changed, 6 insertions(+), 22 deletions(-)

arch/alpha/include/asm/io.h +0 -1

index 1989b946a28d..11fdcade3c5c 100644
@@ -290,7 +290,6 @@ static inline void __iomem * ioremap_nocache(unsigned long offset,
 }
 
 #define ioremap_wc ioremap_nocache
-#define ioremap_uc ioremap_nocache
 
 static inline void iounmap(volatile void __iomem *addr)
 {

arch/m68k/include/asm/kmap.h +0 -1

index 559cb91bede1..22b5ea4fc8b8 100644
@@ -28,7 +28,6 @@ static inline void __iomem *ioremap(unsigned long physaddr, unsigned long size)
 }
 
 #define ioremap_nocache ioremap
-#define ioremap_uc ioremap
 #define ioremap_wt ioremap_wt
 static inline void __iomem *ioremap_wt(unsigned long physaddr,
                                       unsigned long size)

arch/mips/include/asm/io.h +0 -1

index 3f6ce74335b4..9195ded1d6a7 100644
@@ -249,7 +249,6 @@ static inline void __iomem *ioremap_prot(phys_addr_t offset,
  */
 #define ioremap_nocache(offset, size)                                  \
        __ioremap_mode((offset), (size), _CACHE_UNCACHED)
-#define ioremap_uc ioremap_nocache
 
 /*
  * ioremap_cache -     map bus memory into CPU space

arch/parisc/include/asm/io.h +0 -1

index 46212b52c23e..0674f5cd3045 100644
@@ -130,7 +130,6 @@ static inline void gsc_writeq(unsigned long long val, unsigned long addr)
 void __iomem *ioremap(unsigned long offset, unsigned long size);
 #define ioremap_nocache(off, sz)       ioremap((off), (sz))
 #define ioremap_wc                     ioremap_nocache
-#define ioremap_uc                     ioremap_nocache
 
 extern void iounmap(const volatile void __iomem *addr);
 

arch/powerpc/include/asm/io.h +0 -1

index a63ec938636d..119bcbe3e328 100644
@@ -716,7 +716,6 @@ extern void __iomem *ioremap_wc(phys_addr_t address, unsigned long size);
 void __iomem *ioremap_wt(phys_addr_t address, unsigned long size);
 void __iomem *ioremap_coherent(phys_addr_t address, unsigned long size);
 #define ioremap_nocache(addr, size)    ioremap((addr), (size))
-#define ioremap_uc(addr, size)         ioremap((addr), (size))
 #define ioremap_cache(addr, size) \
        ioremap_prot((addr), (size), pgprot_val(PAGE_KERNEL))
 

arch/sh/include/asm/io.h +0 -1

index 1495489225ac..30bbe787f1ef 100644
@@ -368,7 +368,6 @@ static inline int iounmap_fixed(void __iomem *addr) { return -EINVAL; }
 #endif
 
 #define ioremap_nocache        ioremap
-#define ioremap_uc     ioremap
 
 /*
  * Convert a physical pointer to a virtual kernel pointer for /dev/mem

arch/sparc/include/asm/io_64.h +0 -1

index f4afa301954a..688911051b44 100644
@@ -407,7 +407,6 @@ static inline void __iomem *ioremap(unsigned long offset, unsigned long size)
 }
 
 #define ioremap_nocache(X,Y)           ioremap((X),(Y))
-#define ioremap_uc(X,Y)                        ioremap((X),(Y))
 #define ioremap_wc(X,Y)                        ioremap((X),(Y))
 #define ioremap_wt(X,Y)                        ioremap((X),(Y))
 

include/asm-generic/io.h +0 -15

index 325fc98cc9ff..357e8638040c 100644
@@ -972,21 +972,6 @@ static inline void __iomem *ioremap(phys_addr_t addr, size_t size)
 #define ioremap_wt ioremap
 #endif
 
-/*
- * ioremap_uc is special in that we do require an explicit architecture
- * implementation.  In general you do not want to use this function in a
- * driver and use plain ioremap, which is uncached by default.  Similarly
- * architectures should not implement it unless they have a very good
- * reason.
- */
-#ifndef ioremap_uc
-#define ioremap_uc ioremap_uc
-static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size)
-{
-       return NULL;
-}
-#endif
-
 #ifdef CONFIG_HAS_IOPORT_MAP
 #ifndef CONFIG_GENERIC_IOMAP
 #ifndef ioport_map

include/linux/io.h +2 -0

index a59834bc0a11..6574bb0f28e6 100644
@@ -64,8 +64,10 @@ static inline void devm_ioport_unmap(struct device *dev, void __iomem *addr)
 
 void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
                           resource_size_t size);
+#ifdef ioremap_uc
 void __iomem *devm_ioremap_uc(struct device *dev, resource_size_t offset,
                                   resource_size_t size);
+#endif
 void __iomem *devm_ioremap_nocache(struct device *dev, resource_size_t offset,
                                   resource_size_t size);
 void __iomem *devm_ioremap_wc(struct device *dev, resource_size_t offset,

lib/devres.c +4 -1

index f56070cf970b..0299279ddf02 100644
@@ -40,9 +40,11 @@ static void __iomem *__devm_ioremap(struct device *dev, resource_size_t offset,
        case DEVM_IOREMAP_NC:
                addr = ioremap_nocache(offset, size);
                break;
+#ifdef ioremap_uc
        case DEVM_IOREMAP_UC:
                addr = ioremap_uc(offset, size);
                break;
+#endif
        case DEVM_IOREMAP_WC:
                addr = ioremap_wc(offset, size);
                break;

@@ -72,6 +74,7 @@ void __iomem *devm_ioremap(struct device *dev, resource_size_t offset,
 }
 EXPORT_SYMBOL(devm_ioremap);
 
+#ifdef ioremap_uc
 /**
  * devm_ioremap_uc - Managed ioremap_uc()
  * @dev: Generic device to remap IO address for

@@ -86,6 +89,7 @@ void __iomem *devm_ioremap_uc(struct device *dev, resource_size_t offset,
        return __devm_ioremap(dev, offset, size, DEVM_IOREMAP_UC);
 }
 EXPORT_SYMBOL_GPL(devm_ioremap_uc);
+#endif /* ioremap_uc */
 
 /**
  * devm_ioremap_nocache - Managed ioremap_nocache()
--

2.20.1

Wut ... Maybe I'm missing something, but from how we use mtrr in other gpu drivers it's a) either you use MTRR because that's all you got or
b) you use pat. Mixing both sounds like a pretty bad idea, since if you need MTRR for performance (because you dont have PAT) then you can't fix the wc with the PAT-based ioremap_uc. And if you have PAT, then you don't really need an MTRR to get wc.

So I'd revert this patch from Luis and ...

... apply this one. Since the same reasoning should apply to anything that's running on any cpu with PAT.

Quoted 8 lines · Daniel Vetter

Wut ... Maybe I'm missing something, but from how we use mtrr in other gpu drivers it's a) either you use MTRR because that's all you got or
b) you use pat. Mixing both sounds like a pretty bad idea, since if you need MTRR for performance (because you dont have PAT) then you can't fix the wc with the PAT-based ioremap_uc. And if you have PAT, then you don't really need an MTRR to get wc.

So I'd revert this patch from Luis and ...

Sounds great to me..

Quoted 2 lines

... apply this one. Since the same reasoning should apply to anything that's running on any cpu with PAT.

Can you take a look at "mfd: intel-lpss: Use devm_ioremap_uc for MMIO" in linux-next, which also looks rather fishy to me? Can't we use the MTRR APIs to override the broken BIOS MTRR setup there as well?

With that we could kill ioremap_uc entirely.

Quoted 15 lines · Christoph Hellwig

Quoted 8 lines · Daniel Vetter

Wut ... Maybe I'm missing something, but from how we use mtrr in other gpu drivers it's a) either you use MTRR because that's all you got or
b) you use pat. Mixing both sounds like a pretty bad idea, since if you need MTRR for performance (because you dont have PAT) then you can't fix the wc with the PAT-based ioremap_uc. And if you have PAT, then you don't really need an MTRR to get wc.

So I'd revert this patch from Luis and ...

Sounds great to me..

Quoted 2 lines

... apply this one. Since the same reasoning should apply to anything that's running on any cpu with PAT.

Quoted 3 lines

Can you take a look at "mfd: intel-lpss: Use devm_ioremap_uc for MMIO" in linux-next, which also looks rather fishy to me? Can't we use the MTRR APIs to override the broken BIOS MTRR setup there as well?

Hm so that's way out of my knowledge, but I think mtrr_cleanup() was supposed to fix up messy/broken MTRR setups by the bios. So maybe they simply didn't enable that in their .config with CONFIG_MTRR_SANITIZER.

An explicit cleanup is currently not possible for drivers, since the only interface exported to drivers is arch_phys_wc_add/del (which short-circuits if pat works since you don't need mtrr in that case).

Adding everyone from that commit, plus Luis. Drivers really shouldn't assume/work around the bios setting up superflous/wrong MTRR.

Quoted 1 line

With that we could kill ioremap_uc entirely.

So yeah removing that seems definitely like the right thing.

I think this would be possible if we could flop ioremap_nocache() to UC instead of UC- on x86. Otherwise, I can't see how we can remove this by still not allowing direct MTRR calls.

Quoted 3 lines · Luis Chamberlain

I think this would be possible if we could flop ioremap_nocache() to UC instead of UC- on x86. Otherwise, I can't see how we can remove this by still not allowing direct MTRR calls.

If everything goes well ioremap_nocache will be gone as of 5.5.

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · Luis Chamberlain

I think this would be possible if we could flop ioremap_nocache() to UC instead of UC- on x86. Otherwise, I can't see how we can remove this by still not allowing direct MTRR calls.

Quoted 1 line

If everything goes well ioremap_nocache will be gone as of 5.5.

As ioremap_nocache() just an alias for ioremap(), I suppose the idea would then be to make x86 ioremap be UC instead of UC-, again matching what the other architectures do already.

Quoted 7 lines · Arnd Bergmann

Quoted 6 lines · Christoph Hellwig

Quoted 3 lines · Luis Chamberlain

I think this would be possible if we could flop ioremap_nocache() to UC instead of UC- on x86. Otherwise, I can't see how we can remove this by still not allowing direct MTRR calls.

Quoted 5 lines

Quoted 1 line

If everything goes well ioremap_nocache will be gone as of 5.5.

As ioremap_nocache() just an alias for ioremap(), I suppose the idea would then be to make x86 ioremap be UC instead of UC-, again matching what the other architectures do already.

I think it's right thing to do, i.e. assume that ioremap() always does strong UC independently on MTRR settings.

Quoted 3 lines · Daniel Vetter

Wut ... Maybe I'm missing something, but from how we use mtrr in other gpu drivers it's a) either you use MTRR because that's all you got or
b) you use pat. Mixing both sounds like a pretty bad idea,

Can you take a look at "mfd: intel-lpss: Use devm_ioremap_uc for MMIO" in linux-next, which also looks rather fishy to me? Can't we use the MTRR APIs to override the broken BIOS MTRR setup there as well?

With that we could kill ioremap_uc entirely.