| Message ID | 1452991370-20121-1-git-send-email-zajec5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
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
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
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
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
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
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
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!
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;