Messages in this thread | | | Date | Sat, 9 Jan 2016 02:10:44 +0000 (GMT) | From | "Maciej W. Rozycki" <> | Subject | Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() |
| |
On Fri, 8 Jan 2016, Brian Norris wrote:
> > > > This removes some (implicit) MIPS dependencies and makes the code more > > > > portable, whether we need it or not :) > > > > > > So now we have following forwardtrace: > > > devm_ioremap_nocache > > > ioremap_nocache > > > __ioremap_mode > > > __ioremap > > > CKSEG1ADDR > > I just noticed that ioremap() and ioremap_nocache() are the same on > MIPS. So I could just do devm_ioremap_resource() and save myself a few > lines...
My bad, I wrote from memory and didn't double check what the caching mode for plain `ioremap' is. You need to use `ioremap_cache' for a cached mapping. Unfortunately the MIPS port is missing this generic interface and defines `ioremap_cachable' instead. I've just posted an obvious fix for this problem.
Ralf, can you pick this fix for 4.5? It qualifies as obvious I believe, similar to a recent `ioremap_uc' addition.
> > > It results in different address than KSEG0ADDR: > > > [ 1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000 > > > [ 1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000 > > > > > > But it still works as expected! :) > > > [ 1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash > > > [ 1.616169] Creating 6 MTD partitions on "bcm47xxsflash": > > > > It is a functional change though and I think the change from a cached to > > uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a > > separate patch, so that both changes can be reviewed independently. > > As I noted before sending my patch, I don't think this driver should > have been using KSEG0 anyway; it should have been KSEG1, right? I can > note that in the patch description, but I don't really see why it needs > to be a separate patch.
You did mention that, but didn't actually justify why an uncached mapping is required here.
This code is in a function called `bcm47xxsflash_read' and reads from an MMIO region, presumably flash memory which behaves like ordinary memory on reads (i.e. no side effects). Therefore using a cached mapping will in most cases result in much better performance as the CPU will load (prefetch) data in cacheline-sized quantities rather than hitting the external bus every time with a word-sized quantity transferred only.
Switching to an uncached mapping would invalidate this optimisation and is independent from a build error fix. Therefore it requires a separate patch and review. Perhaps `memremap' should be used instead (possibly with the MEMREMAP_WT policy), but this is exactly why a separate review is required for that part.
Maciej
| |