* [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() @ 2016-01-17 0:42 Rafał Miłecki 2016-01-24 20:17 ` Maciej W. Rozycki 2016-02-26 10:50 ` [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() " Rafał Miłecki 0 siblings, 2 replies; 12+ messages in thread From: Rafał Miłecki @ 2016-01-17 0:42 UTC (permalink / raw) To: Brian Norris, linux-mtd Cc: Maciej W. Rozycki, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle, Rafał Miłecki From: Brian Norris <computersforpeace@gmail.com> 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. 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. 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. Signed-off-by: Brian Norris <computersforpeace@gmail.com> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- drivers/bcma/driver_chipcommon_sflash.c | 1 - drivers/mtd/devices/bcm47xxsflash.c | 28 +++++++++++++++++++++++----- drivers/mtd/devices/bcm47xxsflash.h | 3 ++- include/linux/bcma/bcma_driver_chipcommon.h | 1 - 4 files changed, 25 insertions(+), 8 deletions(-) diff --git a/drivers/bcma/driver_chipcommon_sflash.c b/drivers/bcma/driver_chipcommon_sflash.c index 7e11ef4..5534cbc 100644 --- a/drivers/bcma/driver_chipcommon_sflash.c +++ b/drivers/bcma/driver_chipcommon_sflash.c @@ -145,7 +145,6 @@ int bcma_sflash_init(struct bcma_drv_cc *cc) return -ENOTSUPP; } - sflash->window = BCMA_SOC_FLASH2; sflash->blocksize = e->blocksize; sflash->numblocks = e->numblocks; sflash->size = sflash->blocksize * sflash->numblocks; 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 @@ -2,6 +2,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/delay.h> +#include <linux/ioport.h> #include <linux/mtd/mtd.h> #include <linux/platform_device.h> #include <linux/bcma/bcma.h> @@ -109,8 +110,7 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len, if ((from + len) > mtd->size) return -EINVAL; - memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from), - len); + memcpy_fromio(buf, b47s->window + from, len); *retlen = len; return len; @@ -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); + 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; @@ -297,7 +315,6 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) break; } - b47s->window = sflash->window; b47s->blocksize = sflash->blocksize; b47s->numblocks = sflash->numblocks; b47s->size = sflash->size; @@ -321,6 +338,7 @@ static int bcm47xxsflash_bcma_remove(struct platform_device *pdev) struct bcm47xxsflash *b47s = sflash->priv; mtd_device_unregister(&b47s->mtd); + iounmap(b47s->window); return 0; } diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h index fe93daf..1564b62 100644 --- a/drivers/mtd/devices/bcm47xxsflash.h +++ b/drivers/mtd/devices/bcm47xxsflash.h @@ -65,7 +65,8 @@ struct bcm47xxsflash { enum bcm47xxsflash_type type; - u32 window; + void __iomem *window; + u32 blocksize; u16 numblocks; u32 size; diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h index db51a6f..03e6fea 100644 --- a/include/linux/bcma/bcma_driver_chipcommon.h +++ b/include/linux/bcma/bcma_driver_chipcommon.h @@ -583,7 +583,6 @@ struct mtd_info; struct bcma_sflash { bool present; - u32 window; u32 blocksize; u16 numblocks; u32 size; -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-01-17 0:42 [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() Rafał Miłecki @ 2016-01-24 20:17 ` Maciej W. Rozycki 2016-01-25 4:04 ` Brian Norris 2016-02-26 10:41 ` Rafał Miłecki 2016-02-26 10:50 ` [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() " Rafał Miłecki 1 sibling, 2 replies; 12+ messages in thread From: Maciej W. Rozycki @ 2016-01-24 20:17 UTC (permalink / raw) To: Rafał Miłecki Cc: Brian Norris, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-01-24 20:17 ` Maciej W. Rozycki @ 2016-01-25 4:04 ` Brian Norris 2016-01-25 19:15 ` Maciej W. Rozycki 2016-02-26 10:41 ` Rafał Miłecki 1 sibling, 1 reply; 12+ messages in thread From: Brian Norris @ 2016-01-25 4:04 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Rafał Miłecki, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle On Sun, Jan 24, 2016 at 08:17:37PM +0000, Maciej W. Rozycki wrote: > On Sun, 17 Jan 2016, Rafał Miłecki wrote: [...] > > 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. No he doesn't. devm_* functions automatically release their resources when either the device is removed, or the probe() fails. So the whole point is that we don't have to explicitly manage the error case. But...since he's not using a devm_* version of ioremap (there isn't one for ioremap_cachable()), we actually need to add an iounmap() for the case where mtd_device_parse_register() fails. If we fix that one, I can apply this. > > + 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. Thanks for the review! Brian ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-01-25 4:04 ` Brian Norris @ 2016-01-25 19:15 ` Maciej W. Rozycki 2016-01-25 19:30 ` Brian Norris 0 siblings, 1 reply; 12+ messages in thread From: Maciej W. Rozycki @ 2016-01-25 19:15 UTC (permalink / raw) To: Brian Norris Cc: Rafał Miłecki, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle On Mon, 25 Jan 2016, Brian Norris wrote: > > > + 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. > > No he doesn't. devm_* functions automatically release their resources > when either the device is removed, or the probe() fails. So the whole > point is that we don't have to explicitly manage the error case. Why does `devm_ioremap_resource' (in lib/devres.c) do that manually then? dest_ptr = devm_ioremap(dev, res->start, size); if (!dest_ptr) { dev_err(dev, "ioremap failed for resource %pR\n", res); devm_release_mem_region(dev, res->start, size); dest_ptr = IOMEM_ERR_PTR(-ENOMEM); } Is this an oversight or was it a deliberate design decision? > But...since he's not using a devm_* version of ioremap (there isn't one > for ioremap_cachable()), we actually need to add an iounmap() for the > case where mtd_device_parse_register() fails. If we fix that one, I can > apply this. As from 4.5-rc1 we now have `ioremap_cache' available for MIPS as well (thanks, Ralf, for a quick action on that!), so you can use that instead to make your code generic. > Thanks for the review! You're welcome! :) Maciej ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-01-25 19:15 ` Maciej W. Rozycki @ 2016-01-25 19:30 ` Brian Norris 0 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-01-25 19:30 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Rafał Miłecki, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle On Mon, Jan 25, 2016 at 07:15:49PM +0000, Maciej W. Rozycki wrote: > On Mon, 25 Jan 2016, Brian Norris wrote: > > > > > + 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. > > > > No he doesn't. devm_* functions automatically release their resources > > when either the device is removed, or the probe() fails. So the whole > > point is that we don't have to explicitly manage the error case. > > Why does `devm_ioremap_resource' (in lib/devres.c) do that manually then? > > dest_ptr = devm_ioremap(dev, res->start, size); > if (!dest_ptr) { > dev_err(dev, "ioremap failed for resource %pR\n", res); > devm_release_mem_region(dev, res->start, size); > dest_ptr = IOMEM_ERR_PTR(-ENOMEM); > } > > Is this an oversight or was it a deliberate design decision? I didn't design it, but I suspect it's because the API for devm_ioremap_resource() doesn't assume that a failed devm_ioremap_resource() means probe() will exit. Perhaps the driver will have some alternative action instead. It's still good to release the resources implicitly requested by devm_ioremap_resource(). The difference for us is that we *know* we're exiting the probe(), so we can rely on the automatic cleanup. > > But...since he's not using a devm_* version of ioremap (there isn't one > > for ioremap_cachable()), we actually need to add an iounmap() for the > > case where mtd_device_parse_register() fails. If we fix that one, I can > > apply this. > > As from 4.5-rc1 we now have `ioremap_cache' available for MIPS as well > (thanks, Ralf, for a quick action on that!), so you can use that instead > to make your code generic. OK, but there's still not a devm_* version of ioremap_cache(), so we'd still need to do the iounmap() ourselves. Also, it looks like kernel/memremap.c is suggesting we use memremap() instead of ioremap_cache()... But I'm not sure if that's really what we want. Brian ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-01-24 20:17 ` Maciej W. Rozycki 2016-01-25 4:04 ` Brian Norris @ 2016-02-26 10:41 ` Rafał Miłecki 2016-02-26 14:18 ` Maciej W. Rozycki 1 sibling, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2016-02-26 10:41 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Brian Norris, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle On 24 January 2016 at 21:17, Maciej W. Rozycki <macro@imgtec.com> wrote: > On Sun, 17 Jan 2016, Rafał Miłecki wrote: > >> 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. No, it isn't hitting condition you pointed. We call ioremap_cachable which uses _page_cachable_default as a flag. This flag (_page_cachable_default) isn't equal to the _CACHE_UNCACHED. Moreover code you pointed uses CKSEG1ADDR which would result in setting bit KSEG1 (0xa0000000). As I pointed in the commit message address it ORed with KSEG2 (0xc0000000). So what really happens is what my commit message says: ioremap_cachable -> __ioremap -> remap_area_pages ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-02-26 10:41 ` Rafał Miłecki @ 2016-02-26 14:18 ` Maciej W. Rozycki 2016-02-26 16:31 ` Rafał Miłecki 0 siblings, 1 reply; 12+ messages in thread From: Maciej W. Rozycki @ 2016-02-26 14:18 UTC (permalink / raw) To: Rafał Miłecki Cc: Brian Norris, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle On Fri, 26 Feb 2016, Rafał Miłecki wrote: > >> 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. > > No, it isn't hitting condition you pointed. We call ioremap_cachable > which uses _page_cachable_default as a flag. This flag > (_page_cachable_default) isn't equal to the _CACHE_UNCACHED. That's exactly what I wrote: code I quoted is "special-casing uncached mapping only" -- which as you have correctly observed does not apply after your change anymore. Which is why previously you got an address in the unmapped KSEG1 segment and now you get an address in the mapped KSEG2 rather than the unmapped KSEG0 segment. > Moreover code you pointed uses CKSEG1ADDR which would result in > setting bit KSEG1 (0xa0000000). As I pointed in the commit message > address it ORed with KSEG2 (0xc0000000). It's not merely ORed, it's actually mapped via the TLB. I hope this makes things clear. Maciej ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() 2016-02-26 14:18 ` Maciej W. Rozycki @ 2016-02-26 16:31 ` Rafał Miłecki 0 siblings, 0 replies; 12+ messages in thread From: Rafał Miłecki @ 2016-02-26 16:31 UTC (permalink / raw) To: Maciej W. Rozycki Cc: Brian Norris, linux-mtd, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle On 26 February 2016 at 15:18, Maciej W. Rozycki <macro@imgtec.com> wrote: > On Fri, 26 Feb 2016, Rafał Miłecki wrote: > >> >> 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. >> >> No, it isn't hitting condition you pointed. We call ioremap_cachable >> which uses _page_cachable_default as a flag. This flag >> (_page_cachable_default) isn't equal to the _CACHE_UNCACHED. > > That's exactly what I wrote: code I quoted is "special-casing uncached > mapping only" -- which as you have correctly observed does not apply after > your change anymore. Which is why previously you got an address in the > unmapped KSEG1 segment and now you get an address in the mapped KSEG2 > rather than the unmapped KSEG0 segment. > >> Moreover code you pointed uses CKSEG1ADDR which would result in >> setting bit KSEG1 (0xa0000000). As I pointed in the commit message >> address it ORed with KSEG2 (0xc0000000). > > It's not merely ORed, it's actually mapped via the TLB. > > I hope this makes things clear. Ah, sorry, I missed the point of your explanation. Now it's clear, thanks! -- Rafał ^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() instead of KSEG0ADDR() 2016-01-17 0:42 [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() Rafał Miłecki 2016-01-24 20:17 ` Maciej W. Rozycki @ 2016-02-26 10:50 ` Rafał Miłecki 2016-03-05 0:54 ` Brian Norris 1 sibling, 1 reply; 12+ messages in thread From: Rafał Miłecki @ 2016-02-26 10:50 UTC (permalink / raw) To: Brian Norris, linux-mtd Cc: Maciej W. Rozycki, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle, Hauke Mehrtens, Rafał Miłecki From: Brian Norris <computersforpeace@gmail.com> Using KSEG0ADDR makes code highly MIPS dependent and not portable. Thanks to the fix a68f376 ("MIPS: io.h: Define `ioremap_cache'") we can use ioremap_cache which is generic and supported on MIPS as well now. KSEG0ADDR was translating 0x1c000000 into 0x9c000000. With ioremap_cache we use MIPS's __ioremap (and then 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. Other than that drivers/bcma/driver_chipcommon_sflash.c nicely setups a struct resource for access window, but we wren't using it. Use it now and drop duplicated info. Signed-off-by: Brian Norris <computersforpeace@gmail.com> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> --- V2: 1) Use ioremap_cache as we already have commit a68f376 in l2-mtd 2) Add iounmap to the error path 3) Update commit message --- drivers/bcma/driver_chipcommon_sflash.c | 1 - drivers/mtd/devices/bcm47xxsflash.c | 29 ++++++++++++++++++++++++----- drivers/mtd/devices/bcm47xxsflash.h | 3 ++- include/linux/bcma/bcma_driver_chipcommon.h | 1 - 4 files changed, 26 insertions(+), 8 deletions(-) diff --git a/drivers/bcma/driver_chipcommon_sflash.c b/drivers/bcma/driver_chipcommon_sflash.c index 7e11ef4..5534cbc 100644 --- a/drivers/bcma/driver_chipcommon_sflash.c +++ b/drivers/bcma/driver_chipcommon_sflash.c @@ -145,7 +145,6 @@ int bcma_sflash_init(struct bcma_drv_cc *cc) return -ENOTSUPP; } - sflash->window = BCMA_SOC_FLASH2; sflash->blocksize = e->blocksize; sflash->numblocks = e->numblocks; sflash->size = sflash->blocksize * sflash->numblocks; diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c index 347bb83..1c65c15 100644 --- a/drivers/mtd/devices/bcm47xxsflash.c +++ b/drivers/mtd/devices/bcm47xxsflash.c @@ -2,6 +2,7 @@ #include <linux/module.h> #include <linux/slab.h> #include <linux/delay.h> +#include <linux/ioport.h> #include <linux/mtd/mtd.h> #include <linux/platform_device.h> #include <linux/bcma/bcma.h> @@ -109,8 +110,7 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len, if ((from + len) > mtd->size) return -EINVAL; - memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from), - len); + memcpy_fromio(buf, b47s->window + from, len); *retlen = len; return len; @@ -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_cache(res->start, resource_size(res)); + if (!b47s->window) { + dev_err(dev, "ioremap failed for resource %pR\n", res); + 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; @@ -297,7 +315,6 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) break; } - b47s->window = sflash->window; b47s->blocksize = sflash->blocksize; b47s->numblocks = sflash->numblocks; b47s->size = sflash->size; @@ -306,6 +323,7 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) err = mtd_device_parse_register(&b47s->mtd, probes, NULL, NULL, 0); if (err) { pr_err("Failed to register MTD device: %d\n", err); + iounmap(b47s->window); return err; } @@ -321,6 +339,7 @@ static int bcm47xxsflash_bcma_remove(struct platform_device *pdev) struct bcm47xxsflash *b47s = sflash->priv; mtd_device_unregister(&b47s->mtd); + iounmap(b47s->window); return 0; } diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h index fe93daf..1564b62 100644 --- a/drivers/mtd/devices/bcm47xxsflash.h +++ b/drivers/mtd/devices/bcm47xxsflash.h @@ -65,7 +65,8 @@ struct bcm47xxsflash { enum bcm47xxsflash_type type; - u32 window; + void __iomem *window; + u32 blocksize; u16 numblocks; u32 size; diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h index db51a6f..03e6fea 100644 --- a/include/linux/bcma/bcma_driver_chipcommon.h +++ b/include/linux/bcma/bcma_driver_chipcommon.h @@ -583,7 +583,6 @@ struct mtd_info; struct bcma_sflash { bool present; - u32 window; u32 blocksize; u16 numblocks; u32 size; -- 1.8.4.5 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() instead of KSEG0ADDR() 2016-02-26 10:50 ` [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() " Rafał Miłecki @ 2016-03-05 0:54 ` Brian Norris 2016-03-18 11:22 ` Ard Biesheuvel 0 siblings, 1 reply; 12+ messages in thread From: Brian Norris @ 2016-03-05 0:54 UTC (permalink / raw) To: Rafał Miłecki Cc: linux-mtd, Maciej W. Rozycki, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle, Hauke Mehrtens, Ard Biesheuvel, Dan Williams + others On Fri, Feb 26, 2016 at 11:50:28AM +0100, Rafał Miłecki wrote: > From: Brian Norris <computersforpeace@gmail.com> > > Using KSEG0ADDR makes code highly MIPS dependent and not portable. > Thanks to the fix a68f376 ("MIPS: io.h: Define `ioremap_cache'") we can > use ioremap_cache which is generic and supported on MIPS as well now. > > KSEG0ADDR was translating 0x1c000000 into 0x9c000000. With ioremap_cache > we use MIPS's __ioremap (and then 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. > > Other than that drivers/bcma/driver_chipcommon_sflash.c nicely setups a > struct resource for access window, but we wren't using it. Use it now > and drop duplicated info. > > Signed-off-by: Brian Norris <computersforpeace@gmail.com> > Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > --- > V2: > 1) Use ioremap_cache as we already have commit a68f376 in l2-mtd > 2) Add iounmap to the error path > 3) Update commit message Hmm, I'm a bit out of the loop on some things here, but it looks like ioremap_cache() is going away. See kernel/memremap.c: /* temporary while we convert existing ioremap_cache users to memremap */ __weak void __iomem *ioremap_cache(resource_size_t offset, unsigned long size) Are we supposed to move to using memremap() here? (If so, devm_memremap() can help us.) Brian > drivers/bcma/driver_chipcommon_sflash.c | 1 - > drivers/mtd/devices/bcm47xxsflash.c | 29 ++++++++++++++++++++++++----- > drivers/mtd/devices/bcm47xxsflash.h | 3 ++- > include/linux/bcma/bcma_driver_chipcommon.h | 1 - > 4 files changed, 26 insertions(+), 8 deletions(-) > > diff --git a/drivers/bcma/driver_chipcommon_sflash.c b/drivers/bcma/driver_chipcommon_sflash.c > index 7e11ef4..5534cbc 100644 > --- a/drivers/bcma/driver_chipcommon_sflash.c > +++ b/drivers/bcma/driver_chipcommon_sflash.c > @@ -145,7 +145,6 @@ int bcma_sflash_init(struct bcma_drv_cc *cc) > return -ENOTSUPP; > } > > - sflash->window = BCMA_SOC_FLASH2; > sflash->blocksize = e->blocksize; > sflash->numblocks = e->numblocks; > sflash->size = sflash->blocksize * sflash->numblocks; > diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c > index 347bb83..1c65c15 100644 > --- a/drivers/mtd/devices/bcm47xxsflash.c > +++ b/drivers/mtd/devices/bcm47xxsflash.c > @@ -2,6 +2,7 @@ > #include <linux/module.h> > #include <linux/slab.h> > #include <linux/delay.h> > +#include <linux/ioport.h> > #include <linux/mtd/mtd.h> > #include <linux/platform_device.h> > #include <linux/bcma/bcma.h> > @@ -109,8 +110,7 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len, > if ((from + len) > mtd->size) > return -EINVAL; > > - memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from), > - len); > + memcpy_fromio(buf, b47s->window + from, len); > *retlen = len; > > return len; > @@ -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_cache(res->start, resource_size(res)); > + if (!b47s->window) { > + dev_err(dev, "ioremap failed for resource %pR\n", res); > + 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; > @@ -297,7 +315,6 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) > break; > } > > - b47s->window = sflash->window; > b47s->blocksize = sflash->blocksize; > b47s->numblocks = sflash->numblocks; > b47s->size = sflash->size; > @@ -306,6 +323,7 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) > err = mtd_device_parse_register(&b47s->mtd, probes, NULL, NULL, 0); > if (err) { > pr_err("Failed to register MTD device: %d\n", err); > + iounmap(b47s->window); > return err; > } > > @@ -321,6 +339,7 @@ static int bcm47xxsflash_bcma_remove(struct platform_device *pdev) > struct bcm47xxsflash *b47s = sflash->priv; > > mtd_device_unregister(&b47s->mtd); > + iounmap(b47s->window); > > return 0; > } > diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h > index fe93daf..1564b62 100644 > --- a/drivers/mtd/devices/bcm47xxsflash.h > +++ b/drivers/mtd/devices/bcm47xxsflash.h > @@ -65,7 +65,8 @@ struct bcm47xxsflash { > > enum bcm47xxsflash_type type; > > - u32 window; > + void __iomem *window; > + > u32 blocksize; > u16 numblocks; > u32 size; > diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h > index db51a6f..03e6fea 100644 > --- a/include/linux/bcma/bcma_driver_chipcommon.h > +++ b/include/linux/bcma/bcma_driver_chipcommon.h > @@ -583,7 +583,6 @@ struct mtd_info; > > struct bcma_sflash { > bool present; > - u32 window; > u32 blocksize; > u16 numblocks; > u32 size; > -- > 1.8.4.5 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() instead of KSEG0ADDR() 2016-03-05 0:54 ` Brian Norris @ 2016-03-18 11:22 ` Ard Biesheuvel 2016-04-04 7:09 ` Brian Norris 0 siblings, 1 reply; 12+ messages in thread From: Ard Biesheuvel @ 2016-03-18 11:22 UTC (permalink / raw) To: Brian Norris Cc: Rafał Miłecki, linux-mtd, Maciej W. Rozycki, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle, Hauke Mehrtens, Dan Williams On 5 March 2016 at 01:54, Brian Norris <computersforpeace@gmail.com> wrote: > + others > > On Fri, Feb 26, 2016 at 11:50:28AM +0100, Rafał Miłecki wrote: >> From: Brian Norris <computersforpeace@gmail.com> >> >> Using KSEG0ADDR makes code highly MIPS dependent and not portable. >> Thanks to the fix a68f376 ("MIPS: io.h: Define `ioremap_cache'") we can >> use ioremap_cache which is generic and supported on MIPS as well now. >> >> KSEG0ADDR was translating 0x1c000000 into 0x9c000000. With ioremap_cache >> we use MIPS's __ioremap (and then 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. >> >> Other than that drivers/bcma/driver_chipcommon_sflash.c nicely setups a >> struct resource for access window, but we wren't using it. Use it now >> and drop duplicated info. >> >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> >> --- >> V2: >> 1) Use ioremap_cache as we already have commit a68f376 in l2-mtd >> 2) Add iounmap to the error path >> 3) Update commit message > > Hmm, I'm a bit out of the loop on some things here, but it looks like > ioremap_cache() is going away. See kernel/memremap.c: > > /* temporary while we convert existing ioremap_cache users to memremap */ > __weak void __iomem *ioremap_cache(resource_size_t offset, unsigned long size) > > Are we supposed to move to using memremap() here? (If so, > devm_memremap() can help us.) > I don't think that ioremap_cache() necessarily has to go away, but what we do need to address is its abuse to map things like firmware tables that reside in normal memory that may not be owned/tracked by the kernel. So for that case, we now have memremap() which should just give you a mapping of the requested region, either via the linear mapping if it happens to be mapped already, or via the vmalloc region if it is highmem or otherwise unavailable directly. However, memory mapped NOR flash remains a special case, since the __iomem annotation may or may not be appropriate depending on context (i.e., reads and writes may both have either memory or strongly ordered semantics) >> drivers/bcma/driver_chipcommon_sflash.c | 1 - >> drivers/mtd/devices/bcm47xxsflash.c | 29 ++++++++++++++++++++++++----- >> drivers/mtd/devices/bcm47xxsflash.h | 3 ++- >> include/linux/bcma/bcma_driver_chipcommon.h | 1 - >> 4 files changed, 26 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/bcma/driver_chipcommon_sflash.c b/drivers/bcma/driver_chipcommon_sflash.c >> index 7e11ef4..5534cbc 100644 >> --- a/drivers/bcma/driver_chipcommon_sflash.c >> +++ b/drivers/bcma/driver_chipcommon_sflash.c >> @@ -145,7 +145,6 @@ int bcma_sflash_init(struct bcma_drv_cc *cc) >> return -ENOTSUPP; >> } >> >> - sflash->window = BCMA_SOC_FLASH2; >> sflash->blocksize = e->blocksize; >> sflash->numblocks = e->numblocks; >> sflash->size = sflash->blocksize * sflash->numblocks; >> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c >> index 347bb83..1c65c15 100644 >> --- a/drivers/mtd/devices/bcm47xxsflash.c >> +++ b/drivers/mtd/devices/bcm47xxsflash.c >> @@ -2,6 +2,7 @@ >> #include <linux/module.h> >> #include <linux/slab.h> >> #include <linux/delay.h> >> +#include <linux/ioport.h> >> #include <linux/mtd/mtd.h> >> #include <linux/platform_device.h> >> #include <linux/bcma/bcma.h> >> @@ -109,8 +110,7 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len, >> if ((from + len) > mtd->size) >> return -EINVAL; >> >> - memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from), >> - len); >> + memcpy_fromio(buf, b47s->window + from, len); >> *retlen = len; >> >> return len; >> @@ -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_cache(res->start, resource_size(res)); >> + if (!b47s->window) { >> + dev_err(dev, "ioremap failed for resource %pR\n", res); >> + 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; >> @@ -297,7 +315,6 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) >> break; >> } >> >> - b47s->window = sflash->window; >> b47s->blocksize = sflash->blocksize; >> b47s->numblocks = sflash->numblocks; >> b47s->size = sflash->size; >> @@ -306,6 +323,7 @@ static int bcm47xxsflash_bcma_probe(struct platform_device *pdev) >> err = mtd_device_parse_register(&b47s->mtd, probes, NULL, NULL, 0); >> if (err) { >> pr_err("Failed to register MTD device: %d\n", err); >> + iounmap(b47s->window); >> return err; >> } >> >> @@ -321,6 +339,7 @@ static int bcm47xxsflash_bcma_remove(struct platform_device *pdev) >> struct bcm47xxsflash *b47s = sflash->priv; >> >> mtd_device_unregister(&b47s->mtd); >> + iounmap(b47s->window); >> >> return 0; >> } >> diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h >> index fe93daf..1564b62 100644 >> --- a/drivers/mtd/devices/bcm47xxsflash.h >> +++ b/drivers/mtd/devices/bcm47xxsflash.h >> @@ -65,7 +65,8 @@ struct bcm47xxsflash { >> >> enum bcm47xxsflash_type type; >> >> - u32 window; >> + void __iomem *window; >> + >> u32 blocksize; >> u16 numblocks; >> u32 size; >> diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h >> index db51a6f..03e6fea 100644 >> --- a/include/linux/bcma/bcma_driver_chipcommon.h >> +++ b/include/linux/bcma/bcma_driver_chipcommon.h >> @@ -583,7 +583,6 @@ struct mtd_info; >> >> struct bcma_sflash { >> bool present; >> - u32 window; >> u32 blocksize; >> u16 numblocks; >> u32 size; >> -- >> 1.8.4.5 >> ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() instead of KSEG0ADDR() 2016-03-18 11:22 ` Ard Biesheuvel @ 2016-04-04 7:09 ` Brian Norris 0 siblings, 0 replies; 12+ messages in thread From: Brian Norris @ 2016-04-04 7:09 UTC (permalink / raw) To: Ard Biesheuvel Cc: Rafał Miłecki, linux-mtd, Maciej W. Rozycki, Javier Martinez Canillas, Linux Kernel Mailing List, Fengguang Wu, Michael Ellerman, Luis de Bethencourt, Jeremy Kerr, Neelesh Gupta, David Woodhouse, Cyril Bur, Ralf Baechle, Hauke Mehrtens, Dan Williams On Fri, Mar 18, 2016 at 12:22:05PM +0100, Ard Biesheuvel wrote: > On 5 March 2016 at 01:54, Brian Norris <computersforpeace@gmail.com> wrote: > > + others > > > > On Fri, Feb 26, 2016 at 11:50:28AM +0100, Rafał Miłecki wrote: > >> From: Brian Norris <computersforpeace@gmail.com> > >> > >> Using KSEG0ADDR makes code highly MIPS dependent and not portable. > >> Thanks to the fix a68f376 ("MIPS: io.h: Define `ioremap_cache'") we can > >> use ioremap_cache which is generic and supported on MIPS as well now. > >> > >> KSEG0ADDR was translating 0x1c000000 into 0x9c000000. With ioremap_cache > >> we use MIPS's __ioremap (and then 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. > >> > >> Other than that drivers/bcma/driver_chipcommon_sflash.c nicely setups a > >> struct resource for access window, but we wren't using it. Use it now > >> and drop duplicated info. > >> > >> Signed-off-by: Brian Norris <computersforpeace@gmail.com> > >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com> > >> --- > >> V2: > >> 1) Use ioremap_cache as we already have commit a68f376 in l2-mtd > >> 2) Add iounmap to the error path > >> 3) Update commit message > > > > Hmm, I'm a bit out of the loop on some things here, but it looks like > > ioremap_cache() is going away. See kernel/memremap.c: > > > > /* temporary while we convert existing ioremap_cache users to memremap */ > > __weak void __iomem *ioremap_cache(resource_size_t offset, unsigned long size) > > > > Are we supposed to move to using memremap() here? (If so, > > devm_memremap() can help us.) > > > > I don't think that ioremap_cache() necessarily has to go away, but > what we do need to address is its abuse to map things like firmware > tables that reside in normal memory that may not be owned/tracked by > the kernel. So for that case, we now have memremap() which should just > give you a mapping of the requested region, either via the linear > mapping if it happens to be mapped already, or via the vmalloc region > if it is highmem or otherwise unavailable directly. > > However, memory mapped NOR flash remains a special case, since the > __iomem annotation may or may not be appropriate depending on context > (i.e., reads and writes may both have either memory or strongly > ordered semantics) OK, pushed to l2-mtd.git. ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2016-04-04 7:10 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-01-17 0:42 [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() Rafał Miłecki 2016-01-24 20:17 ` Maciej W. Rozycki 2016-01-25 4:04 ` Brian Norris 2016-01-25 19:15 ` Maciej W. Rozycki 2016-01-25 19:30 ` Brian Norris 2016-02-26 10:41 ` Rafał Miłecki 2016-02-26 14:18 ` Maciej W. Rozycki 2016-02-26 16:31 ` Rafał Miłecki 2016-02-26 10:50 ` [PATCH V2] mtd: bcm47xxsflash: use ioremap_cache() " Rafał Miłecki 2016-03-05 0:54 ` Brian Norris 2016-03-18 11:22 ` Ard Biesheuvel 2016-04-04 7:09 ` Brian Norris
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.