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

Groups > linux.kernel > #1303908 > unrolled thread

Re: [PATCH] mtd: Make MTD_BCM47XXSFLASH to depend on MIPS

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH] mtd: Make MTD_BCM47XXSFLASH to depend on MIPS "Maciej W. Rozycki" <macro@linux-mips.org> - 2016-01-07 22:10 +0100
    [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of  KSEG0ADDR() Brian Norris <computersforpeace@gmail.com> - 2016-01-08 00:10 +0100
      Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() Rafał Miłecki <zajec5@gmail.com> - 2016-01-08 09:00 +0100
        Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() "Maciej W. Rozycki" <macro@linux-mips.org> - 2016-01-08 15:10 +0100
          Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR() Rafał Miłecki <zajec5@gmail.com> - 2016-01-08 16:30 +0100
            Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() "Maciej W. Rozycki" <macro@linux-mips.org> - 2016-01-09 03:40 +0100
          Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() Brian Norris <computersforpeace@gmail.com> - 2016-01-08 20:00 +0100
            Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead  of KSEG0ADDR() "Maciej W. Rozycki" <macro@linux-mips.org> - 2016-01-09 03:40 +0100

#1303908 — Re: [PATCH] mtd: Make MTD_BCM47XXSFLASH to depend on MIPS

On Wed, 4 Nov 2015, Brian Norris wrote:

> > > The bcm47xxsflash driver uses the KSEG0ADDR() function to map an address
> > > to a certain kernel segment. But that is only defined if the MIPS config
> > > symbol is enabled. The driver does not have an explicit dependency on it
> > > and relies on a transitive dependency relation:
> > >
> > > MTD_BCM47XXSFLASH -> BCMA_SFLASH -> BCMA_DRIVER_MIPS -> BCMA && MIPS
> > >
> > > But BCMA_SFLASH and BCMA_DRIVER_MIPS have only runtime and not buildtime
> > > dependency with MIPS so can be changed to be built test using the config
> > > COMPILE_TEST symbol. But that would make MTD_BCM47XXSFLASH be built with
> > > MIPS not enabled and cause the following build error:
> > >
> > > drivers/mtd/devices//bcm47xxsflash.c: In function 'bcm47xxsflash_read':
> > > drivers/mtd/devices//bcm47xxsflash.c:112:2: error: implicit declaration of function 'KSEG0ADDR' [-Werror=implicit-function-declaration]
> > >   memcpy_fromio(buf, (void __iomem *)KSEG0ADDR(b47s->window + from),
> > 
> > I think we're not really supposed to use KSEG0ADDR anyway. What about
> > replacing it with ioremap_nocache?
> 
> I'm not really a MIPS expert, but isn't KSEG0 actually *cached*? (And is
> that correct, then?)
> 
> AIUI, ioremap_nocache() will actually get you a KSEG1 address here, I
> think.

 Depending on configuration `ioremap_nocache' may give you a KSEG1, an 
XKPHYS or an uncached virtual (KSEG2) address.  Drivers are not supposed 
to use KSEG0ADDR, etc. macros, these are only for low-level core platform 
code.  Besides, KSEG0ADDR is not portable to 64-bit systems -- which may 
not be an issue here, but still this means it shouldn't appear in a driver 
as this causes a portability and maintenance issue.

 Use plain `ioremap' instead for a cached MMIO mapping; of course you need 
to place it elsewhere as at the very least you need to `iounmap' the area 
mappad later on.  In most cases it makes sense to handle the mapping and 
unmapping in device initialisation and shutdown respectively, and then 
carry the pointer obtained through and use it throughout the use of the 
device.

 HTH,

  Maciej

[toc] | [next] | [standalone]

#1303988 — [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

drivers/bcma/driver_chipcommon_sflash.c already nicely sets us up a
struct resource for this window, but we just aren't using it. Use it
now!

This removes some (implicit) MIPS dependencies and makes the code more
portable, whether we need it or not :)

Signed-off-by: Brian Norris <computersforpeace@gmail.com>
---
On Thu, Jan 07, 2016 at 09:06:50PM +0000, Maciej W. Rozycki wrote:
> On Wed, 4 Nov 2015, Brian Norris wrote:
> > > I think we're not really supposed to use KSEG0ADDR anyway. What about
> > > replacing it with ioremap_nocache?

OK, done! Compile tested only.

 drivers/bcma/driver_chipcommon_sflash.c     |  1 -
 drivers/mtd/devices/bcm47xxsflash.c         | 30 +++++++++++++++++++++++------
 drivers/mtd/devices/bcm47xxsflash.h         |  3 ++-
 include/linux/bcma/bcma_driver_chipcommon.h |  1 -
 4 files changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/bcma/driver_chipcommon_sflash.c b/drivers/bcma/driver_chipcommon_sflash.c
index 7e11ef4cb7db..5534cbcc222f 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 347bb83db864..ce6b7e51569d 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,34 @@ 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 = devm_ioremap_nocache(dev, 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,11 +316,10 @@ 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;
-	bcm47xxsflash_fill_mtd(b47s, &pdev->dev);
+	bcm47xxsflash_fill_mtd(b47s, dev);
 
 	err = mtd_device_parse_register(&b47s->mtd, probes, NULL, NULL, 0);
 	if (err) {
diff --git a/drivers/mtd/devices/bcm47xxsflash.h b/drivers/mtd/devices/bcm47xxsflash.h
index fe93daf4f489..1564b62b412e 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 db51a6ffb7d6..03e6fea9e9ce 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;
-- 
2.6.0.rc2.230.g3dd15c0

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

#1304244 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On 8 January 2016 at 00:05, Brian Norris <computersforpeace@gmail.com> wrote:
> drivers/bcma/driver_chipcommon_sflash.c already nicely sets us up a
> struct resource for this window, but we just aren't using it. Use it
> now!

Works nice :)
[    1.333884] [bcm47xxsflash_bcma_probe] res->start:0x1c000000


> This removes some (implicit) MIPS dependencies and makes the code more
> portable, whether we need it or not :)

So now we have following forwardtrace:
devm_ioremap_nocache
ioremap_nocache
__ioremap_mode
__ioremap
CKSEG1ADDR

It results in different address than KSEG0ADDR:
[    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
[    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000

But it still works as expected! :)
[    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
[    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":


> Signed-off-by: Brian Norris <computersforpeace@gmail.com>

Tested-by: Rafał Miłecki <zajec5@gmail.com>

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

#1304572 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Fri, 8 Jan 2016, Rafał Miłecki wrote:

> > This removes some (implicit) MIPS dependencies and makes the code more
> > portable, whether we need it or not :)
> 
> So now we have following forwardtrace:
> devm_ioremap_nocache
> ioremap_nocache
> __ioremap_mode
> __ioremap
> CKSEG1ADDR
> 
> It results in different address than KSEG0ADDR:
> [    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
> [    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000
> 
> But it still works as expected! :)
> [    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
> [    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":

 It is a functional change though and I think the change from a cached to 
uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a 
separate patch, so that both changes can be reviewed independently.

  Maciej

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

#1304649 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On 8 January 2016 at 15:01, Maciej W. Rozycki <macro@linux-mips.org> wrote:
> On Fri, 8 Jan 2016, Rafał Miłecki wrote:
>
>> > This removes some (implicit) MIPS dependencies and makes the code more
>> > portable, whether we need it or not :)
>>
>> So now we have following forwardtrace:
>> devm_ioremap_nocache
>> ioremap_nocache
>> __ioremap_mode
>> __ioremap
>> CKSEG1ADDR
>>
>> It results in different address than KSEG0ADDR:
>> [    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
>> [    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000
>>
>> But it still works as expected! :)
>> [    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
>> [    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":
>
>  It is a functional change though and I think the change from a cached to
> uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a
> separate patch, so that both changes can be reviewed independently.

We didn't switch from 'ioremap' but from KSEG0ADDR. What exactly
should be a separated patch?

-- 
Rafał

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

#1305144 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Fri, 8 Jan 2016, Rafał Miłecki wrote:

> >> > This removes some (implicit) MIPS dependencies and makes the code more
> >> > portable, whether we need it or not :)
> >>
> >> So now we have following forwardtrace:
> >> devm_ioremap_nocache
> >> ioremap_nocache
> >> __ioremap_mode
> >> __ioremap
> >> CKSEG1ADDR
> >>
> >> It results in different address than KSEG0ADDR:
> >> [    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
> >> [    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000
> >>
> >> But it still works as expected! :)
> >> [    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
> >> [    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":
> >
> >  It is a functional change though and I think the change from a cached to
> > uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a
> > separate patch, so that both changes can be reviewed independently.
> 
> We didn't switch from 'ioremap' but from KSEG0ADDR. What exactly
> should be a separated patch?

 See my other reply -- KSEG0ADDR (cached mapping) corresponds to 
`ioremap_cache', whereas `ioremap_nocache' (or its generic `ioremap_uc' 
alias) or plain `ioremap' correspond to KSEG1ADDR (uncached mapping).  
Consequently a change that switches from KSEG0ADDR to `ioremap_nocache' or 
`ioremap' includes a functional change along a build error (portability) 
fix.  Therefore such a change has to be split into two, so that the 
functional change can be reviewed separately from the portability fix.

  Maciej

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

#1304933 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Fri, Jan 08, 2016 at 02:01:00PM +0000, Maciej W. Rozycki wrote:
> On Fri, 8 Jan 2016, Rafał Miłecki wrote:
> 
> > > This removes some (implicit) MIPS dependencies and makes the code more
> > > portable, whether we need it or not :)
> > 
> > So now we have following forwardtrace:
> > devm_ioremap_nocache
> > ioremap_nocache
> > __ioremap_mode
> > __ioremap
> > CKSEG1ADDR

I just noticed that ioremap() and ioremap_nocache() are the same on
MIPS. So I could just do devm_ioremap_resource() and save myself a few
lines...

> > It results in different address than KSEG0ADDR:
> > [    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
> > [    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000
> > 
> > But it still works as expected! :)
> > [    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
> > [    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":
> 
>  It is a functional change though and I think the change from a cached to 
> uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a 
> separate patch, so that both changes can be reviewed independently.

As I noted before sending my patch, I don't think this driver should
have been using KSEG0 anyway; it should have been KSEG1, right? I can
note that in the patch description, but I don't really see why it needs
to be a separate patch.

Brian

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

#1305145 — Re: [PATCH] mtd: bcm47xxsflash: use devm_ioremap_nocache() instead of KSEG0ADDR()

On Fri, 8 Jan 2016, Brian Norris wrote:

> > > > This removes some (implicit) MIPS dependencies and makes the code more
> > > > portable, whether we need it or not :)
> > > 
> > > So now we have following forwardtrace:
> > > devm_ioremap_nocache
> > > ioremap_nocache
> > > __ioremap_mode
> > > __ioremap
> > > CKSEG1ADDR
> 
> I just noticed that ioremap() and ioremap_nocache() are the same on
> MIPS. So I could just do devm_ioremap_resource() and save myself a few
> lines...

 My bad, I wrote from memory and didn't double check what the caching mode 
for plain `ioremap' is.  You need to use `ioremap_cache' for a cached 
mapping.  Unfortunately the MIPS port is missing this generic interface 
and defines `ioremap_cachable' instead.  I've just posted an obvious fix 
for this problem.

 Ralf, can you pick this fix for 4.5?  It qualifies as obvious I believe, 
similar to a recent `ioremap_uc' addition.

> > > It results in different address than KSEG0ADDR:
> > > [    1.339752] [bcm47xxsflash_bcma_probe] KSEG0ADDR(BCMA_SOC_FLASH2):9c000000
> > > [    1.346848] [bcm47xxsflash_bcma_probe] devm_ioremap_nocache:bc000000
> > > 
> > > But it still works as expected! :)
> > > [    1.609426] 6 bcm47xxpart partitions found on MTD device bcm47xxsflash
> > > [    1.616169] Creating 6 MTD partitions on "bcm47xxsflash":
> > 
> >  It is a functional change though and I think the change from a cached to 
> > uncached mapping (i.e. from `ioremap' to `ioremap_nocache') has to be a 
> > separate patch, so that both changes can be reviewed independently.
> 
> As I noted before sending my patch, I don't think this driver should
> have been using KSEG0 anyway; it should have been KSEG1, right? I can
> note that in the patch description, but I don't really see why it needs
> to be a separate patch.

 You did mention that, but didn't actually justify why an uncached mapping 
is required here.

 This code is in a function called `bcm47xxsflash_read' and reads from an 
MMIO region, presumably flash memory which behaves like ordinary memory on 
reads (i.e. no side effects).  Therefore using a cached mapping will in 
most cases result in much better performance as the CPU will load 
(prefetch) data in cacheline-sized quantities rather than hitting the 
external bus every time with a word-sized quantity transferred only.

 Switching to an uncached mapping would invalidate this optimisation and 
is independent from a build error fix.  Therefore it requires a separate 
patch and review.  Perhaps `memremap' should be used instead (possibly 
with the MEMREMAP_WT policy), but this is exactly why a separate review is 
required for that part.

  Maciej

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web