From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752199AbcAXURO (ORCPT ); Sun, 24 Jan 2016 15:17:14 -0500 Received: from mailapp01.imgtec.com ([195.59.15.196]:49258 "EHLO mailapp01.imgtec.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751582AbcAXURL (ORCPT ); Sun, 24 Jan 2016 15:17:11 -0500 Date: Sun, 24 Jan 2016 20:17:37 +0000 From: "Maciej W. Rozycki" To: =?ISO-8859-2?Q?Rafa=B3_Mi=B3ecki?= CC: Brian Norris , , Javier Martinez Canillas , Linux Kernel Mailing List , Fengguang Wu , Michael Ellerman , Luis de Bethencourt , Jeremy Kerr , Neelesh Gupta , David Woodhouse , Cyril Bur , Ralf Baechle Subject: Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() In-Reply-To: <1452991370-20121-1-git-send-email-zajec5@gmail.com> Message-ID: References: <1452991370-20121-1-git-send-email-zajec5@gmail.com> User-Agent: Alpine 2.00 (DEB 1167 2008-08-23) MIME-Version: 1.0 Content-Type: text/plain; charset="ISO-8859-2" Content-Transfer-Encoding: 8BIT X-Originating-IP: [10.100.200.15] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, 17 Jan 2016, Rafał Miłecki wrote: > Using KSEG0ADDR makes code highly MIPS dependent and not portable. > Ideally we should use ioremap_cache which is generic but right now MIPS > doesn't define it which results in kernel using ioremap instead. > Once MIPS gets ioremap_cache we will switch to it, it's going to be > trivial with this patch applied first. Agreed, I think your proposal is a move in the right direction and does not regress current code as we're using KSEG0ADDR anyway, which is MIPS-specific. > KSEG0ADDR was translating 0x1c000000 into 0x9c000000. With > ioremap_cachable we use MIPS's __ioremap (and remap_area_pages). This > results in different address (e.g. 0xc0080000) but it still should be > cached as expected and it was successfully tested with BCM47186B0. This is due to this piece: /* * Map uncached objects in the low 512mb of address space using KSEG1, * otherwise map using page tables. */ if (IS_LOW512(phys_addr) && IS_LOW512(last_addr) && flags == _CACHE_UNCACHED) return (void __iomem *) CKSEG1ADDR(phys_addr); special-casing uncached mapping only (replicated in 2 places). I think there will really be no harm from returning a KSEG0 mapping for calls requesting a caching mode equal to `_page_cachable_default', which -- depending on the cache architecture -- will have been either hardwired or prearranged via Config.K0. I think there's really no need to put pressure on the TLB, which may be small, in cases where a fixed mapping will do. We also seem to pessimise the `cpu_has_64bit_addresses' case and always return a fixed uncached mapping, regardless of the mode requested, hmm... > Moreover drivers/bcma/driver_chipcommon_sflash.c nicely sets us up a > struct resource for access window, but we just aren't using it. Use it > now and drop duplicated info. Agreed, again, except from one nit, as noted below. > diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c > index 347bb83..84f8fae 100644 > --- a/drivers/mtd/devices/bcm47xxsflash.c > +++ b/drivers/mtd/devices/bcm47xxsflash.c > @@ -275,15 +275,33 @@ static void bcm47xxsflash_bcma_cc_write(struct bcm47xxsflash *b47s, u16 offset, > > static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) > { > - struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev); > + struct device *dev = &pdev->dev; > + struct bcma_sflash *sflash = dev_get_platdata(dev); > struct bcm47xxsflash *b47s; > + struct resource *res; > int err; > > - b47s = devm_kzalloc(&pdev->dev, sizeof(*b47s), GFP_KERNEL); > + b47s = devm_kzalloc(dev, sizeof(*b47s), GFP_KERNEL); > if (!b47s) > return -ENOMEM; > sflash->priv = b47s; > > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_err(dev, "invalid resource\n"); > + return -EINVAL; > + } > + if (!devm_request_mem_region(dev, res->start, resource_size(res), > + res->name)) { > + dev_err(dev, "can't request region for resource %pR\n", res); > + return -EBUSY; > + } > + b47s->window = ioremap_cachable(res->start, resource_size(res)); > + if (!b47s->window) { > + dev_err(dev, "ioremap failed for resource %pR\n", res); You need to call `devm_release_mem_region' in this case. > + return -ENOMEM; > + } > + > b47s->bcma_cc = container_of(sflash, struct bcma_drv_cc, sflash); > b47s->cc_read = bcm47xxsflash_bcma_cc_read; > b47s->cc_write = bcm47xxsflash_bcma_cc_write; LGTM otherwise. Maciej