[PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Rafał Miłecki]

This registers MTD driver for serial flash platform device. Right now it
supports reading only, writing still has to be implemented.

Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
---
V2: fix mem leak (add failure path in init)
    signed-off-by
V3: drop BROKEN
    drop useless check in bcm47xxsflash_remove
    marm flash memory as ROM (until we implement writing)
---
 drivers/mtd/devices/Kconfig                 |    8 ++
 drivers/mtd/devices/Makefile                |    1 +
 drivers/mtd/devices/bcm47xxsflash.c         |  131 +++++++++++++++++++++++++++
 include/linux/bcma/bcma_driver_chipcommon.h |    2 +
 4 files changed, 142 insertions(+), 0 deletions(-)
 create mode 100644 drivers/mtd/devices/bcm47xxsflash.c

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index 6cc5a1a..ce6e3d4 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -120,6 +120,14 @@ config MTD_SST25L
 	  Set up your spi devices with the right board-specific platform data,
 	  if you want to specify device partitioning.
 
+config MTD_BCM47XXSFLASH
+	tristate "Support for serial flash on BCMA bus"
+	depends on BCMA_SFLASH
+	help
+	  BCMA bus can have various flash memories attached, they are
+	  registered by bcma as platform devices. This enables driver for
+	  serial flash memories.
+
 config MTD_SLRAM
 	tristate "Uncached system RAM"
 	help
diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
index a4dd1d8..395733a 100644
--- a/drivers/mtd/devices/Makefile
+++ b/drivers/mtd/devices/Makefile
@@ -19,5 +19,6 @@ obj-$(CONFIG_MTD_DATAFLASH)	+= mtd_dataflash.o
 obj-$(CONFIG_MTD_M25P80)	+= m25p80.o
 obj-$(CONFIG_MTD_SPEAR_SMI)	+= spear_smi.o
 obj-$(CONFIG_MTD_SST25L)	+= sst25l.o
+obj-$(CONFIG_MTD_BCM47XXSFLASH)	+= bcm47xxsflash.o
 
 CFLAGS_docg3.o			+= -I$(src)
\ No newline at end of file
diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
new file mode 100644
index 0000000..f711a51
--- /dev/null
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -0,0 +1,131 @@
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/mtd/mtd.h>
+#include <linux/platform_device.h>
+#include <linux/bcma/bcma.h>
+
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Serial flash driver for BCMA bus");
+
+static const char *probes[] = { "bcm47xxpart", NULL };
+
+static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
+			      size_t *retlen, u_char *buf)
+{
+	struct bcma_sflash *sflash = mtd->priv;
+	size_t bytes_read = 0;
+	u8 *src = (u8 *)KSEG0ADDR(sflash->window + from);
+	int i;
+	size_t unaligned_before, unaligned_after;
+
+	/* Check address range */
+	if ((from + len) > mtd->size)
+		return -EINVAL;
+
+	unaligned_before = from & 0x3;
+	unaligned_after = (from + len) & 0x3;
+
+	for (i = 0; i < unaligned_before; i++) {
+		*buf = readb(src);
+		buf++;
+		src++;
+		bytes_read++;
+	}
+	for (i = from - unaligned_before; i < from + len - unaligned_after;
+	     i += 4) {
+		*(u32 *)buf = readl((u32 *)src);
+		buf += 4;
+		src += 4;
+		bytes_read += 4;
+	}
+	for (i = 0; i < unaligned_after; i++) {
+		*buf = readb(src);
+		buf++;
+		src++;
+		bytes_read++;
+	}
+
+	*retlen = bytes_read;
+
+	return 0;
+}
+
+static void bcm47xxsflash_fill_mtd(struct bcma_sflash *sflash,
+				   struct mtd_info *mtd)
+{
+	mtd->priv = sflash;
+	mtd->name = "bcm47xxsflash";
+	mtd->owner = THIS_MODULE;
+	mtd->type = MTD_ROM;
+	mtd->size = sflash->size;
+	mtd->_read = bcm47xxsflash_read;
+
+	/* TODO: implement writing support and verify/change following code */
+	mtd->flags = MTD_CAP_ROM;
+	mtd->writebufsize = mtd->writesize = 1;
+}
+
+static int bcm47xxsflash_probe(struct platform_device *pdev)
+{
+	struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
+	int err;
+
+	sflash->mtd = kzalloc(sizeof(struct mtd_info), GFP_KERNEL);
+	if (!sflash->mtd) {
+		err = -ENOMEM;
+		goto out;
+	}
+	bcm47xxsflash_fill_mtd(sflash, sflash->mtd);
+
+	err = mtd_device_parse_register(sflash->mtd, probes, NULL, NULL, 0);
+	if (err) {
+		pr_err("Failed to register MTD device: %d\n", err);
+		goto err_dev_reg;
+	}
+
+	return 0;
+
+err_dev_reg:
+	kfree(sflash->mtd);
+out:
+	return err;
+}
+
+static int __devexit bcm47xxsflash_remove(struct platform_device *pdev)
+{
+	struct bcma_sflash *sflash = dev_get_platdata(&pdev->dev);
+
+	mtd_device_unregister(sflash->mtd);
+	kfree(sflash->mtd);
+
+	return 0;
+}
+
+static struct platform_driver bcma_sflash_driver = {
+	.remove = __devexit_p(bcm47xxsflash_remove),
+	.driver = {
+		.name = "bcma_sflash",
+		.owner = THIS_MODULE,
+	},
+};
+
+static int __init bcm47xxsflash_init(void)
+{
+	int err;
+
+	err = platform_driver_probe(&bcma_sflash_driver, bcm47xxsflash_probe);
+	if (err)
+		pr_err("Failed to register BCMA serial flash driver: %d\n",
+		       err);
+
+	return err;
+}
+
+static void __exit bcm47xxsflash_exit(void)
+{
+	platform_driver_unregister(&bcma_sflash_driver);
+}
+
+module_init(bcm47xxsflash_init);
+module_exit(bcm47xxsflash_exit);
diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h
index 6ba45d2..1cf1749 100644
--- a/include/linux/bcma/bcma_driver_chipcommon.h
+++ b/include/linux/bcma/bcma_driver_chipcommon.h
@@ -522,6 +522,8 @@ struct bcma_sflash {
 	u32 blocksize;
 	u16 numblocks;
 	u32 size;
+
+	struct mtd_info *mtd;
 };
 #endif
 
-- 
1.7.7

Re: [PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 2588 bytes --]

On Mon, 2012-09-17 at 11:50 +0200, Rafał Miłecki wrote:
> This registers MTD driver for serial flash platform device. Right now it
> supports reading only, writing still has to be implemented.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> ---
> V2: fix mem leak (add failure path in init)
>     signed-off-by
> V3: drop BROKEN
>     drop useless check in bcm47xxsflash_remove
>     marm flash memory as ROM (until we implement writing)

Aiaiai complained:

--- before_patching.log
+++ after_patching.log
@@ @@
+drivers/mtd/devices/bcm47xxsflash.c:30:30: warning: incorrect type in argument 1 (different address spaces) [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:30:30:    expected void const volatile [noderef] <asn:2>*mem [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:30:30:    got unsigned char [usertype] *src [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:37:38: warning: incorrect type in argument 1 (different address spaces) [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:37:38:    expected void const volatile [noderef] <asn:2>*mem [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:37:38:    got unsigned int [usertype] *<noident> [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:43:30: warning: incorrect type in argument 1 (different address spaces) [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:43:30:    expected void const volatile [noderef] <asn:2>*mem [sparse]
+drivers/mtd/devices/bcm47xxsflash.c:43:30:    got unsigned char [usertype] *[assigned] src [sparse]

I pushed your patch to l2-mtd.git with these changes:

diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
index f711a51..a328872 100644
--- a/drivers/mtd/devices/bcm47xxsflash.c
+++ b/drivers/mtd/devices/bcm47xxsflash.c
@@ -15,7 +15,7 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
 {
        struct bcma_sflash *sflash = mtd->priv;
        size_t bytes_read = 0;
-       u8 *src = (u8 *)KSEG0ADDR(sflash->window + from);
+       __iomem u8 *src = (__iomem u8 *)KSEG0ADDR(sflash->window + from);
        int i;
        size_t unaligned_before, unaligned_after;
 
@@ -34,7 +34,7 @@ static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
        }
        for (i = from - unaligned_before; i < from + len - unaligned_after;
             i += 4) {
-               *(u32 *)buf = readl((u32 *)src);
+               *(u32 *)buf = readl(src);
                buf += 4;
                src += 4;
                bytes_read += 4;

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Jonas Gorski]

I know I'm a bit late, but ...

On 17 September 2012 11:50, Rafał Miłecki <zajec5@gmail.com> wrote:
> (...)
> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
> index a4dd1d8..395733a 100644
> --- a/drivers/mtd/devices/Makefile
> +++ b/drivers/mtd/devices/Makefile
> @@ -19,5 +19,6 @@ obj-$(CONFIG_MTD_DATAFLASH)   += mtd_dataflash.o
>  obj-$(CONFIG_MTD_M25P80)       += m25p80.o
>  obj-$(CONFIG_MTD_SPEAR_SMI)    += spear_smi.o
>  obj-$(CONFIG_MTD_SST25L)       += sst25l.o
> +obj-$(CONFIG_MTD_BCM47XXSFLASH)        += bcm47xxsflash.o
>
>  CFLAGS_docg3.o                 += -I$(src)
> \ No newline at end of file
> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
> new file mode 100644
> index 0000000..f711a51
> --- /dev/null
> +++ b/drivers/mtd/devices/bcm47xxsflash.c
> @@ -0,0 +1,131 @@
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/slab.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/platform_device.h>
> +#include <linux/bcma/bcma.h>
> +
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Serial flash driver for BCMA bus");
> +
> +static const char *probes[] = { "bcm47xxpart", NULL };
> +
> +static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
> +                             size_t *retlen, u_char *buf)
> +{
> +       struct bcma_sflash *sflash = mtd->priv;
> +       size_t bytes_read = 0;
> +       u8 *src = (u8 *)KSEG0ADDR(sflash->window + from);
> +       int i;
> +       size_t unaligned_before, unaligned_after;
> +
> +       /* Check address range */
> +       if ((from + len) > mtd->size)
> +               return -EINVAL;
> +
> +       unaligned_before = from & 0x3;

I think this should be 4 - (from & 0x3), else you will read too much
in the unaligned_before loop, also being at an unaligned address then.

E.g. assuming I want to read four bytes from offset 3, you will read
three bytes from from, and will actually then read three bytes from 3,
landing at offset 6, and then reading another three in the
unaligned_after loop; overwriting memory with the last two.

> +       unaligned_after = (from + len) & 0x3;
> +
> +       for (i = 0; i < unaligned_before; i++) {
> +               *buf = readb(src);
> +               buf++;
> +               src++;
> +               bytes_read++;
> +       }
> +       for (i = from - unaligned_before; i < from + len - unaligned_after;

This one looks suspicious, aren't you assuming you have read
unaligned_before bytes, so shouldn't i be from /+/ unaligned_before?
Currently this will result in the example above that it will loop once
(since i = 3 - 3 = 0, and 0 < 3 + 4 - 3), resulting in additional 4
bytes read.

> +            i += 4) {
> +               *(u32 *)buf = readl((u32 *)src);
> +               buf += 4;
> +               src += 4;
> +               bytes_read += 4;
> +       }
> +       for (i = 0; i < unaligned_after; i++) {
> +               *buf = readb(src);
> +               buf++;
> +               src++;
> +               bytes_read++;
> +       }

It will also fail for unaligned reads within the same 4 byte block -
they will be read twice, once in _before, once in _after.

Also in the before and after loops, you are reading with native
endianess (since you don't swab the addresses), but in the aligned
case you are assuming little endianess. If you intend to do native
endianess reads (we only support little endian bcm47xx), you should be
fine by just replacing this whole logic with a memcpy(_fromio?), if
not you should probably swab the addresses when reading the unaligned
parts when big endian. Unfortunately I don't have any big endian
bcm47xx devices at hand to test ;-) (but I do know they exist - AFAICT
from netgear's gpl sources the bcm53xxx switch chips are big endian
bcm47xx).


Regards
Jonas

Re: [PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Rafał Miłecki]

2012/9/18 Jonas Gorski <jonas.gorski@gmail.com>:
> I know I'm a bit late, but ...
>
> On 17 September 2012 11:50, Rafał Miłecki <zajec5@gmail.com> wrote:
>> (...)
>> diff --git a/drivers/mtd/devices/Makefile b/drivers/mtd/devices/Makefile
>> index a4dd1d8..395733a 100644
>> --- a/drivers/mtd/devices/Makefile
>> +++ b/drivers/mtd/devices/Makefile
>> @@ -19,5 +19,6 @@ obj-$(CONFIG_MTD_DATAFLASH)   += mtd_dataflash.o
>>  obj-$(CONFIG_MTD_M25P80)       += m25p80.o
>>  obj-$(CONFIG_MTD_SPEAR_SMI)    += spear_smi.o
>>  obj-$(CONFIG_MTD_SST25L)       += sst25l.o
>> +obj-$(CONFIG_MTD_BCM47XXSFLASH)        += bcm47xxsflash.o
>>
>>  CFLAGS_docg3.o                 += -I$(src)
>> \ No newline at end of file
>> diff --git a/drivers/mtd/devices/bcm47xxsflash.c b/drivers/mtd/devices/bcm47xxsflash.c
>> new file mode 100644
>> index 0000000..f711a51
>> --- /dev/null
>> +++ b/drivers/mtd/devices/bcm47xxsflash.c
>> @@ -0,0 +1,131 @@
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/slab.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/bcma/bcma.h>
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_DESCRIPTION("Serial flash driver for BCMA bus");
>> +
>> +static const char *probes[] = { "bcm47xxpart", NULL };
>> +
>> +static int bcm47xxsflash_read(struct mtd_info *mtd, loff_t from, size_t len,
>> +                             size_t *retlen, u_char *buf)
>> +{
>> +       struct bcma_sflash *sflash = mtd->priv;
>> +       size_t bytes_read = 0;
>> +       u8 *src = (u8 *)KSEG0ADDR(sflash->window + from);
>> +       int i;
>> +       size_t unaligned_before, unaligned_after;
>> +
>> +       /* Check address range */
>> +       if ((from + len) > mtd->size)
>> +               return -EINVAL;
>> +
>> +       unaligned_before = from & 0x3;
>
> I think this should be 4 - (from & 0x3), else you will read too much
> in the unaligned_before loop, also being at an unaligned address then.

Right, that's wrong.


>> +       unaligned_after = (from + len) & 0x3;
>> +
>> +       for (i = 0; i < unaligned_before; i++) {
>> +               *buf = readb(src);
>> +               buf++;
>> +               src++;
>> +               bytes_read++;
>> +       }
>> +       for (i = from - unaligned_before; i < from + len - unaligned_after;
>
> This one looks suspicious, aren't you assuming you have read
> unaligned_before bytes, so shouldn't i be from /+/ unaligned_before?

Right again. I could probably drom "from" too.


>> +            i += 4) {
>> +               *(u32 *)buf = readl((u32 *)src);
>> +               buf += 4;
>> +               src += 4;
>> +               bytes_read += 4;
>> +       }
>> +       for (i = 0; i < unaligned_after; i++) {
>> +               *buf = readb(src);
>> +               buf++;
>> +               src++;
>> +               bytes_read++;
>> +       }
>
> It will also fail for unaligned reads within the same 4 byte block -
> they will be read twice, once in _before, once in _after.

Hm, I don't understand that yet. It looks fine for me :| But I'll do
some test and it should become obvious to me then :)


> Also in the before and after loops, you are reading with native
> endianess (since you don't swab the addresses), but in the aligned
> case you are assuming little endianess. If you intend to do native
> endianess reads (we only support little endian bcm47xx), you should be
> fine by just replacing this whole logic with a memcpy(_fromio?), if
> not you should probably swab the addresses when reading the unaligned
> parts when big endian. Unfortunately I don't have any big endian
> bcm47xx devices at hand to test ;-) (but I do know they exist - AFAICT
> from netgear's gpl sources the bcm53xxx switch chips are big endian
> bcm47xx).

Whoops, I didn't know readl is LE access :| I though readb/readw/readl
are native endianess. Sorry for that.


I'm not sure about that endianess, what exactly we should implement. I
wanted to get some tip from simple map (see map_funcs.c and map.h) and
it seems it is using "inline_map_copy_from" indeed.

-- 
Rafał

Re: [PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 517 bytes --]

On Mon, 2012-09-17 at 11:50 +0200, Rafał Miłecki wrote:
> This registers MTD driver for serial flash platform device. Right now it
> supports reading only, writing still has to be implemented.
> 
> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>

Can you please confirm that you tested this and the driver works in R/O
mode just fine. If yes, I would also like to add a note that this is R/O
to the Kconfig text. If it does not even work, I am willing to drop it.

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Rafał Miłecki]

2012/9/26 Artem Bityutskiy <dedekind1@gmail.com>:
> On Mon, 2012-09-17 at 11:50 +0200, Rafał Miłecki wrote:
>> This registers MTD driver for serial flash platform device. Right now it
>> supports reading only, writing still has to be implemented.
>>
>> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
>
> Can you please confirm that you tested this and the driver works in R/O
> mode just fine. If yes, I would also like to add a note that this is R/O
> to the Kconfig text. If it does not even work, I am willing to drop it.

OFC it works! :) It nicely cooperates with bcm47xxpart (partitions are
detected) and I can read mtd content (bootloader, nvram, etc.).

You're right, we miss info about R/O in Kconfig, I didn't think about this.

-- 
Rafał

Re: [PATCH V3] mtd: basic (read only) driver for BCMA serial flash

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 1713 bytes --]

On Wed, 2012-09-26 at 09:55 +0200, Rafał Miłecki wrote:
> 2012/9/26 Artem Bityutskiy <dedekind1@gmail.com>:
> > On Mon, 2012-09-17 at 11:50 +0200, Rafał Miłecki wrote:
> >> This registers MTD driver for serial flash platform device. Right now it
> >> supports reading only, writing still has to be implemented.
> >>
> >> Signed-off-by: Rafał Miłecki <zajec5@gmail.com>
> >
> > Can you please confirm that you tested this and the driver works in R/O
> > mode just fine. If yes, I would also like to add a note that this is R/O
> > to the Kconfig text. If it does not even work, I am willing to drop it.
> 
> OFC it works! :) It nicely cooperates with bcm47xxpart (partitions are
> detected) and I can read mtd content (bootloader, nvram, etc.).
> 
> You're right, we miss info about R/O in Kconfig, I didn't think about this.

I've just folded the following changes in:

diff --git a/drivers/mtd/devices/Kconfig b/drivers/mtd/devices/Kconfig
index ce6e3d4..27f80cd 100644
--- a/drivers/mtd/devices/Kconfig
+++ b/drivers/mtd/devices/Kconfig
@@ -121,12 +121,12 @@ config MTD_SST25L
          if you want to specify device partitioning.
 
 config MTD_BCM47XXSFLASH
-       tristate "Support for serial flash on BCMA bus"
+       tristate "R/O support for serial flash on BCMA bus"
        depends on BCMA_SFLASH
        help
          BCMA bus can have various flash memories attached, they are
          registered by bcma as platform devices. This enables driver for
-         serial flash memories.
+         serial flash memories (only read-only mode is implemented).
 
 config MTD_SLRAM
        tristate "Uncached system RAM"


-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]