Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1311096 > unrolled thread

[PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR()

Back to article view | Back to linux.kernel

Contents

  [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR() Rafał Miłecki <zajec5@gmail.com> - 2016-01-17 01:50 +0100
    Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of  KSEG0ADDR() "Maciej W. Rozycki" <macro@imgtec.com> - 2016-01-24 21:20 +0100
      Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of  KSEG0ADDR() Brian Norris <computersforpeace@gmail.com> - 2016-01-25 05:10 +0100
        Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of  KSEG0ADDR() "Maciej W. Rozycki" <macro@imgtec.com> - 2016-01-25 20:20 +0100
          Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of  KSEG0ADDR() Brian Norris <computersforpeace@gmail.com> - 2016-01-25 20:40 +0100

#1311096 — [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR()

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

[toc] | [next] | [standalone]

#1315970 — Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR()

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

[toc] | [prev] | [next] | [standalone]

#1316194 — Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR()

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

[toc] | [prev] | [next] | [standalone]

#1317220 — Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR()

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

[toc] | [prev] | [next] | [standalone]

#1317238 — Re: [PATCH] mtd: bcm47xxsflash: use ioremap_cachable() instead of KSEG0ADDR()

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web