All of lore.kernel.org
 help / color / mirror / Atom feed
* [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

* [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] 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

* 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.