| Message ID | 20191029064834.23438-11-hch@lst.de |
|---|---|
| State | New |
| Headers | show |
| Series | [01/21] arm: remove ioremap_cached | expand |
On Mon, 28 Oct 2019 23:48:23 PDT (-0700), Christoph Hellwig wrote: > Whatever reason there is for the existence of ioremap_uc, and the fact > that it returns NULL by default on architectures with an MMU applies > equally to nommu architectures, so don't provide different defaults. > > In practice the difference is meaningless as the only portable driver > that uses ioremap_uc is atyfb which probably doesn't show up on nommu > devices. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > include/asm-generic/io.h | 36 ++++++++++++++++-------------------- > 1 file changed, 16 insertions(+), 20 deletions(-) > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index d02806513670..a98ed6325727 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -935,18 +935,7 @@ static inline void *phys_to_virt(unsigned long address) > * defined your own ioremap_*() variant you must then declare your own > * ioremap_*() variant as defined to itself to avoid the default NULL return. > */ > - > -#ifdef CONFIG_MMU > - > -#ifndef ioremap_uc > -#define ioremap_uc ioremap_uc > -static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) > -{ > - return NULL; > -} > -#endif > - > -#else /* !CONFIG_MMU */ > +#ifndef CONFIG_MMU > > /* > * Change "struct page" to physical address. > @@ -980,14 +969,6 @@ static inline void __iomem *ioremap_nocache(phys_addr_t offset, size_t size) > } > #endif > > -#ifndef ioremap_uc > -#define ioremap_uc ioremap_uc > -static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) > -{ > - return ioremap_nocache(offset, size); > -} > -#endif > - > #ifndef ioremap_wc > #define ioremap_wc ioremap_wc > static inline void __iomem *ioremap_wc(phys_addr_t offset, size_t size) > @@ -1004,6 +985,21 @@ static inline void __iomem *ioremap_wt(phys_addr_t offset, size_t size) > } > #endif > > +/* > + * ioremap_uc is special in that we do require an explicit architecture > + * implementation. In general you do now want to use this function in a Presumably that's supposed to be "do not want to use"? > + * 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 With the fix: Reviewed-by: Palmer Dabbelt <palmer@dabbelt.com>
On Tue, Oct 29, 2019 at 7:49 AM Christoph Hellwig <hch@lst.de> wrote: > > Whatever reason there is for the existence of ioremap_uc, and the fact > that it returns NULL by default on architectures with an MMU applies > equally to nommu architectures, so don't provide different defaults. Makes sense. > In practice the difference is meaningless as the only portable driver > that uses ioremap_uc is atyfb which probably doesn't show up on nommu > devices. > +/* > + * ioremap_uc is special in that we do require an explicit architecture > + * implementation. In general you do now 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 Maybe we could move the definition into the atyfb driver itself? As I understand it, the difference between ioremap()/ioremap_nocache() and ioremap_uc() only exists on pre-PAT x86-32 systems (i.e. 486, P5, Ppro, PII, K6, VIA C3), while on more modern systems (all non-x86, PentiumIII, Athlon, VIA C7) those three are meant to be synonyms anyway. Arnd
On Mon, Nov 11, 2019 at 11:09:05AM +0100, Arnd Bergmann wrote: > Maybe we could move the definition into the atyfb driver itself? > > As I understand it, the difference between ioremap()/ioremap_nocache() > and ioremap_uc() only exists on pre-PAT x86-32 systems (i.e. 486, P5, > Ppro, PII, K6, VIA C3), while on more modern systems (all non-x86, > PentiumIII, Athlon, VIA C7) those three are meant to be synonyms > anyway. That's not how I understood it. Based on the code and the UC- explanation ioremap_uc always overrides the MTRR, which can still be present on more modern x86 systems. In fact I remember a patch floating around very recently adding another ioremap_uc caller in some Atom platform device driver that works around buggy MTRR tables. Also this series actually adds a new override and a few callers for ia64 platform code, which works very similar to x86 based on the comments in the code. That being said I'm not sure the callers in ia64 are really required, but it was the safest thing to do as part of this cleanup.
On Mon, Nov 11, 2019 at 11:15 AM Christoph Hellwig <hch@lst.de> wrote: > > On Mon, Nov 11, 2019 at 11:09:05AM +0100, Arnd Bergmann wrote: > > Maybe we could move the definition into the atyfb driver itself? > > > > As I understand it, the difference between ioremap()/ioremap_nocache() > > and ioremap_uc() only exists on pre-PAT x86-32 systems (i.e. 486, P5, > > Ppro, PII, K6, VIA C3), while on more modern systems (all non-x86, > > PentiumIII, Athlon, VIA C7) those three are meant to be synonyms > > anyway. > > That's not how I understood it. Based on the code and the UC- > explanation ioremap_uc always overrides the MTRR, which can still > be present on more modern x86 systems. As I understand, the point is that on PAT-enabled systems, the normal ioremap() *also* overrides the MTRR, citing from Documentation/x86/pat.rst: ==== ======= === ========================= ===================== MTRR Non-PAT PAT Linux ioremap value Effective memory type ==== ======= === ========================= ===================== PAT Non-PAT | PAT |PCD | ||PWT | ||| | WC 000 WB _PAGE_CACHE_MODE_WB WC | WC WC 001 WC _PAGE_CACHE_MODE_WC WC* | WC WC 010 UC- _PAGE_CACHE_MODE_UC_MINUS WC* | UC WC 011 UC _PAGE_CACHE_MODE_UC UC | UC ==== ======= === ========================= ===================== > In fact I remember a patch > floating around very recently adding another ioremap_uc caller in > some Atom platform device driver that works around buggy MTRR > tables. Also this series actually adds a new override and a few > callers for ia64 platform code, which works very similar to x86 > based on the comments in the code. That being said I'm not sure > the callers in ia64 are really required, but it was the safest thing > to do as part of this cleanup. Ok, fair enough. Let's just go with your version for now, if only to not hold your series up more. I'd still suggest we change atyfb to only use ioremap_uc() on i386 and maybe ia64. I can send a patch for that. Arnd
On Mon, Nov 11, 2019 at 11:27:27AM +0100, Arnd Bergmann wrote: > Ok, fair enough. Let's just go with your version for now, if only to not > hold your series up more. I'd still suggest we change atyfb to only > use ioremap_uc() on i386 and maybe ia64. I can send a patch for that. I don't think we even need it on ia64. But lets kick off a dicussion with the atyfb, x86 and ia64 maintainers after this series is in. Which was kinda my plan anyway.
On Mon, Nov 11, 2019 at 11:29 AM Christoph Hellwig <hch@lst.de> wrote: > > On Mon, Nov 11, 2019 at 11:27:27AM +0100, Arnd Bergmann wrote: > > Ok, fair enough. Let's just go with your version for now, if only to not > > hold your series up more. I'd still suggest we change atyfb to only > > use ioremap_uc() on i386 and maybe ia64. I can send a patch for that. > > I don't think we even need it on ia64. But lets kick off a dicussion > with the atyfb, x86 and ia64 maintainers after this series is in. > Which was kinda my plan anyway. I missed your reply and already sent my patch now. I guess it doesn't hurt to discuss that in parallel. Anyway I think that this patch is the last one you want an Ack from me for (let me know if I missed one), so Reviewed-by: Arnd Bergmann <arnd@arndb.de>
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index d02806513670..a98ed6325727 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -935,18 +935,7 @@ static inline void *phys_to_virt(unsigned long address) * defined your own ioremap_*() variant you must then declare your own * ioremap_*() variant as defined to itself to avoid the default NULL return. */ - -#ifdef CONFIG_MMU - -#ifndef ioremap_uc -#define ioremap_uc ioremap_uc -static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) -{ - return NULL; -} -#endif - -#else /* !CONFIG_MMU */ +#ifndef CONFIG_MMU /* * Change "struct page" to physical address. @@ -980,14 +969,6 @@ static inline void __iomem *ioremap_nocache(phys_addr_t offset, size_t size) } #endif -#ifndef ioremap_uc -#define ioremap_uc ioremap_uc -static inline void __iomem *ioremap_uc(phys_addr_t offset, size_t size) -{ - return ioremap_nocache(offset, size); -} -#endif - #ifndef ioremap_wc #define ioremap_wc ioremap_wc static inline void __iomem *ioremap_wc(phys_addr_t offset, size_t size) @@ -1004,6 +985,21 @@ static inline void __iomem *ioremap_wt(phys_addr_t offset, size_t size) } #endif +/* + * ioremap_uc is special in that we do require an explicit architecture + * implementation. In general you do now 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
Whatever reason there is for the existence of ioremap_uc, and the fact that it returns NULL by default on architectures with an MMU applies equally to nommu architectures, so don't provide different defaults. In practice the difference is meaningless as the only portable driver that uses ioremap_uc is atyfb which probably doesn't show up on nommu devices. Signed-off-by: Christoph Hellwig <hch@lst.de> --- include/asm-generic/io.h | 36 ++++++++++++++++-------------------- 1 file changed, 16 insertions(+), 20 deletions(-)