From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755204AbcAICa2 (ORCPT ); Fri, 8 Jan 2016 21:30:28 -0500 Received: from eddie.linux-mips.org ([148.251.95.138]:34986 "EHLO cvs.linux-mips.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754111AbcAIC3k (ORCPT ); Fri, 8 Jan 2016 21:29:40 -0500 Date: Sat, 9 Jan 2016 02:10:44 +0000 (GMT) From: "Maciej W. Rozycki" To: Brian Norris , Ralf Baechle cc: =?ISO-8859-2?Q?Rafa=B3_Mi=B3ecki?= , Javier Martinez Canillas , Linux Kernel Mailing List , Fengguang Wu , Michael Ellerman , Luis de Bethencourt , Jeremy Kerr , Neelesh Gupta , "linux-mtd@lists.infradead.org" , David Woodhouse , Cyril Bur Subject: Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() In-Reply-To: <20160108185120.GQ109450@google.com> Message-ID: References: <1444813494-14985-1-git-send-email-javier@osg.samsung.com> <20151104185341.GN7274@google.com> <20160107230513.GL109450@google.com> <20160108185120.GQ109450@google.com> User-Agent: Alpine 2.20 (LFD 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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