[PATCH v2 0/3] mtd: sharpslpart partition parser

[PATCH v2 0/3] mtd: sharpslpart partition parser

[Andrea Adami]

This patchset introduces a simple partition parser for the Sharp SL
Series PXA handhelds. More details in the commit text.

I have set in cc the ARM PXA maintainers because this is the MTD part of
a planned wider patchset cleaning the Zaurus board files.

Changelog:
v1 initial import of 2.4 sources [1]
v2 refactor applying many suggested fixes [2]

[1] https://github.com/LinuxPDA/Sharp_FTL_2.4.20
[2] https://github.com/LinuxPDA/linux/commits/sharpslpart_v2

Andrea Adami (3):
  mtd: sharpsl: add sharpslpart MTD partition parser
  mtd: nand: sharpsl.c: prefer sharpslpart MTD partition parser
  mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

 drivers/mtd/Kconfig          |   8 ++
 drivers/mtd/Makefile         |   2 +
 drivers/mtd/nand/sharpsl.c   |   4 +-
 drivers/mtd/nand/tmio_nand.c |   4 +-
 drivers/mtd/sharpsl_ftl.c    | 219 +++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/sharpsl_ftl.h    |  34 +++++++
 drivers/mtd/sharpslpart.c    | 132 ++++++++++++++++++++++++++
 7 files changed, 401 insertions(+), 2 deletions(-)
 create mode 100644 drivers/mtd/sharpsl_ftl.c
 create mode 100644 drivers/mtd/sharpsl_ftl.h
 create mode 100644 drivers/mtd/sharpslpart.c

-- 
2.7.4

[PATCH v2 1/3] mtd: sharpsl: add sharpslpart MTD partition parser

[Andrea Adami]

The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash
and share the same layout of the first 7M partition, managed by Sharp FTL.

The purpose of this self-contained patch is to add a common parser and
remove the hardcoded sizes in the board files (these devices are not yet
converted to devicetree).
Users will have benefits because the mtdparts= tag will not be necessary
anymore and they will be free to repartition the little sized flash.

The obsolete bootloader can not pass the partitioning info to modern
kernels anymore so it has to be read from flash at known logical addresses.
(see http://www.h5.dion.ne.jp/~rimemoon/zaurus/memo_006.htm )

In kernel, under arch/arm/mach-pxa we have already 8 machines:
MACH_POODLE, MACH_CORGI, MACH_SHEPERD, MACH_HUSKY, MACH_AKITA, MACH_SPITZ,
MACH_BORZOI, MACH_TOSA.
Lost after the 2.4 vendor kernel are MACH_BOXER and MACH_TERRIER.

Almost every model has different factory partitioning: add to this the
units can be repartitioned by users with userspace tools (nandlogical)
and installers for popular (back then) linux distributions.

The Parameter Area in the first (boot) partition extends from 0x00040000 to
0x0007bfff (176k) and contains two copies of the partition table:
...
0x00060000: Partition Info1	16k
0x00064000: Partition Info2	16k
0x00668000: Model		16k
...

The first 7M partition is managed by the Sharp FTL reserving 5% + 1 blocks
for wear-leveling: some blocks are remapped and one layer of translation
(logical to physical) is necessary.

There isn't much documentation about this FTL in the 2.4 sources, just the
MTD methods for reading and writing using logical addresses and the block
management (wear-leveling, use counter).
For the purpose of the MTD parser only the read part of the code was taken.

The NAND drivers that can use this parser are sharpsl.c and tmio_nand.c.

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 drivers/mtd/Kconfig       |   8 ++
 drivers/mtd/Makefile      |   2 +
 drivers/mtd/sharpsl_ftl.c | 219 ++++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/sharpsl_ftl.h |  34 +++++++
 drivers/mtd/sharpslpart.c | 132 ++++++++++++++++++++++++++++
 5 files changed, 395 insertions(+)
 create mode 100644 drivers/mtd/sharpsl_ftl.c
 create mode 100644 drivers/mtd/sharpsl_ftl.h
 create mode 100644 drivers/mtd/sharpslpart.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index e83a279..5c96127 100644
--- a/drivers/mtd/Kconfig
+++ b/drivers/mtd/Kconfig
@@ -155,6 +155,14 @@ config MTD_BCM47XX_PARTS
 	  This provides partitions parser for devices based on BCM47xx
 	  boards.
 
+config MTD_SHARPSL_PARTS
+	tristate "Sharp SL Series NAND flash partition parser"
+	depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO
+	help
+	  This provides the read-only FTL logic necessary to read the partition
+	  table from the NAND flash of Sharp SL Series (Zaurus) and the MTD
+	  partition parser using this code.
+
 comment "User Modules And Translation Layers"
 
 #
diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
index 99bb9a1..89f707b 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -13,6 +13,8 @@ obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
 obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
 obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
 obj-$(CONFIG_MTD_BCM47XX_PARTS)	+= bcm47xxpart.o
+obj-$(CONFIG_MTD_SHARPSL_PARTS)	+= sharpsl-part.o
+sharpsl-part-objs := sharpsl_ftl.o sharpslpart.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
diff --git a/drivers/mtd/sharpsl_ftl.c b/drivers/mtd/sharpsl_ftl.c
new file mode 100644
index 0000000..6b82144
--- /dev/null
+++ b/drivers/mtd/sharpsl_ftl.c
@@ -0,0 +1,219 @@
+/*
+ * MTD method for NAND accessing via logical address (SHARP FTL)
+ *
+ * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
+ *
+ * Based on 2.4 sources: drivers/mtd/nand/sharp_sl_logical.c
+ * Copyright (C) 2002  SHARP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include "sharpsl_ftl.h"
+
+/* oob structure */
+#define NAND_NOOB_LOGADDR_00		8
+#define NAND_NOOB_LOGADDR_01		9
+#define NAND_NOOB_LOGADDR_10		10
+#define NAND_NOOB_LOGADDR_11		11
+#define NAND_NOOB_LOGADDR_20		12
+#define NAND_NOOB_LOGADDR_21		13
+
+/* Logical Table */
+struct mtd_logical {
+	u32 size;		/* size of the handled partition */
+	int index;		/* mtd->index */
+	u_int phymax;		/* physical blocks */
+	u_int logmax;		/* logical blocks */
+	u_int *log2phy;		/* the logical-to-physical table */
+};
+
+static struct mtd_logical *sharpsl_mtd_logical;
+
+/* wrapper */
+static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,
+				 uint8_t *buf)
+{
+	loff_t mask = mtd->writesize - 1;
+	struct mtd_oob_ops ops;
+	int ret;
+
+	ops.mode = MTD_OPS_PLACE_OOB;
+	ops.ooboffs = offs & mask;
+	ops.ooblen = len;
+	ops.oobbuf = buf;
+	ops.datbuf = NULL;
+
+	ret = mtd_read_oob(mtd, offs & ~mask, &ops);
+	if (ret != 0 || len != ops.oobretlen)
+		return -1;
+
+	return 0;
+}
+
+/* utility */
+static u_int sharpsl_nand_get_logical_num(u_char *oob)
+{
+	u16 us;
+	int good0, good1;
+
+	if (oob[NAND_NOOB_LOGADDR_00] == oob[NAND_NOOB_LOGADDR_10] &&
+	    oob[NAND_NOOB_LOGADDR_01] == oob[NAND_NOOB_LOGADDR_11]) {
+		good0 = NAND_NOOB_LOGADDR_00;
+		good1 = NAND_NOOB_LOGADDR_01;
+	} else if (oob[NAND_NOOB_LOGADDR_10] == oob[NAND_NOOB_LOGADDR_20] &&
+		   oob[NAND_NOOB_LOGADDR_11] == oob[NAND_NOOB_LOGADDR_21]) {
+		good0 = NAND_NOOB_LOGADDR_10;
+		good1 = NAND_NOOB_LOGADDR_11;
+	} else if (oob[NAND_NOOB_LOGADDR_20] == oob[NAND_NOOB_LOGADDR_00] &&
+		   oob[NAND_NOOB_LOGADDR_21] == oob[NAND_NOOB_LOGADDR_01]) {
+		good0 = NAND_NOOB_LOGADDR_20;
+		good1 = NAND_NOOB_LOGADDR_21;
+	} else {
+		return UINT_MAX;
+	}
+
+	us = oob[good0] | oob[good1] << 8;
+
+	/* parity check */
+	if (hweight16(us) & 1)
+		return (UINT_MAX - 1);
+
+	/* reserved */
+	if (us == 0xffff)
+		return 0xffff;
+	else
+		return (us & 0x07fe) >> 1;
+}
+
+int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)
+{
+	struct mtd_logical *logical = NULL;
+	u_int block_num, log_num;
+	loff_t block_adr;
+	u_char *oob = NULL;
+	int i, readretry;
+
+	logical = kzalloc(sizeof(*logical), GFP_KERNEL);
+	if (!logical)
+		return -ENOMEM;
+
+	oob = kzalloc(mtd->oobsize, GFP_KERNEL);
+	if (!oob) {
+		kfree(logical);
+		return -ENOMEM;
+	}
+
+	/* initialize management structure */
+	logical->size = partition_size;
+	logical->index = mtd->index;
+	logical->phymax = (partition_size / mtd->erasesize);
+
+	/* FTL reserves 5% of the blocks + 1 spare  */
+	logical->logmax = ((logical->phymax * 95) / 100) - 1;
+
+	logical->log2phy = NULL;
+	logical->log2phy = kcalloc(logical->logmax, sizeof(u_int), GFP_KERNEL);
+	if (!logical->log2phy) {
+		kfree(logical);
+		kfree(oob);
+		return -ENOMEM;
+	}
+
+	/* initialize logical->log2phy */
+	for (i = 0; i < logical->logmax; i++)
+		logical->log2phy[i] = UINT_MAX;
+
+	/* create physical-logical table */
+	for (block_num = 0; block_num < logical->phymax; block_num++) {
+		block_adr = block_num * mtd->erasesize;
+
+		if (mtd_block_isbad(mtd, block_adr))
+			continue;
+
+		readretry = 3;
+read_retry:
+		if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
+			continue;
+
+		/* get logical block */
+		log_num = sharpsl_nand_get_logical_num(oob);
+
+		/* skip out of range and not unique values */
+		if ((int)log_num >= 0  && (log_num < logical->logmax)) {
+			if (logical->log2phy[log_num] == UINT_MAX)
+				logical->log2phy[log_num] = block_num;
+		} else {
+			readretry--;
+			if (readretry)
+				goto read_retry;
+		}
+	}
+	kfree(oob);
+	sharpsl_mtd_logical = logical;
+
+	pr_info("Sharp SL FTL: %d blocks used (%d logical, %d reserved)\n",
+		logical->phymax, logical->logmax,
+		logical->phymax - logical->logmax);
+
+	return 0;
+}
+
+void sharpsl_nand_cleanup_logical(void)
+{
+	struct mtd_logical *logical = sharpsl_mtd_logical;
+
+	sharpsl_mtd_logical = NULL;
+
+	kfree(logical->log2phy);
+	logical->log2phy = NULL;
+	kfree(logical);
+	logical = NULL;
+}
+
+/* MTD METHOD */
+int sharpsl_nand_read_laddr(struct mtd_info *mtd,
+			    loff_t from,
+			    size_t len,
+			    u_char *buf)
+{
+	struct mtd_logical *logical;
+	u_int log_num, log_new;
+	u_int block_num;
+	loff_t block_adr;
+	loff_t block_ofs;
+	size_t retlen;
+	int ret;
+
+	logical = sharpsl_mtd_logical;
+	log_num = (u32)from / mtd->erasesize;
+	log_new = ((u32)from + len - 1) / mtd->erasesize;
+
+	if (len <= 0 || log_num >= logical->logmax || log_new > log_num)
+		return -EINVAL;
+
+	block_num = logical->log2phy[log_num];
+	block_adr = block_num * mtd->erasesize;
+	block_ofs = (u32)from % mtd->erasesize;
+
+	ret = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
+	if (ret != 0 || len != retlen)
+		return -EINVAL;
+
+	return 0;
+}
diff --git a/drivers/mtd/sharpsl_ftl.h b/drivers/mtd/sharpsl_ftl.h
new file mode 100644
index 0000000..2880cbe
--- /dev/null
+++ b/drivers/mtd/sharpsl_ftl.h
@@ -0,0 +1,34 @@
+/*
+ * Header file for NAND accessing via logical address (SHARP FTL)
+ *
+ * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
+ *
+ * Based on 2.4 sources: linux/include/asm-arm/sharp_nand_logical.h
+ * Copyright (C) 2002  SHARP
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#ifndef __SHARPSL_NAND_LOGICAL_H__
+#define __SHARPSL_NAND_LOGICAL_H__
+
+#include <linux/types.h>
+#include <linux/mtd/mtd.h>
+
+int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size);
+
+void sharpsl_nand_cleanup_logical(void);
+
+int sharpsl_nand_read_laddr(struct mtd_info *mtd, loff_t from, size_t len,
+			    u_char *buf);
+
+#endif
diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
new file mode 100644
index 0000000..33d2fc0
--- /dev/null
+++ b/drivers/mtd/sharpslpart.c
@@ -0,0 +1,132 @@
+/*
+ * MTD partition parser for NAND flash on Sharp SL Series
+ *
+ * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.h>
+#include "sharpsl_ftl.h"
+
+/* factory defaults */
+#define SHARPSL_NAND_PARTS		3
+#define SHARPSL_FTL_PARTITION_SIZE	(7 * 1024 * 1024)
+#define PARAM_BLOCK_PARTITIONINFO1	0x00060000
+#define PARAM_BLOCK_PARTITIONINFO2	0x00064000
+
+#define BOOT_MAGIC			be32_to_cpu(0x424f4f54)   /* BOOT */
+#define FSRO_MAGIC			be32_to_cpu(0x4653524f)   /* FSRO */
+#define FSRW_MAGIC			be32_to_cpu(0x46535257)   /* FSRW */
+
+/*
+ * Sample values read from SL-C860
+ *
+ * # cat /proc/mtd
+ * dev:    size   erasesize  name
+ * mtd0: 006d0000 00020000 "Filesystem"
+ * mtd1: 00700000 00004000 "smf"
+ * mtd2: 03500000 00004000 "root"
+ * mtd3: 04400000 00004000 "home"
+ *
+ * PARTITIONINFO1
+ * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00  ......p.BOOT....
+ * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00  ..p.....FSRO....
+ * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00  ........FSRW....
+ * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
+ *
+ */
+
+struct sharpsl_nand_partitioninfo {
+	u32 start;
+	u32 end;
+	u32 magic;
+	u32 reserved;
+};
+
+static int sharpsl_parse_mtd_partitions(struct mtd_info *master,
+					const struct mtd_partition **pparts,
+					struct mtd_part_parser_data *data)
+{
+	struct sharpsl_nand_partitioninfo buf1[SHARPSL_NAND_PARTS];
+	struct sharpsl_nand_partitioninfo buf2[SHARPSL_NAND_PARTS];
+	struct mtd_partition *sharpsl_nand_parts;
+
+	/* init logical mgmt (FTL) */
+	if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
+		return -EINVAL;
+
+	/* read the two partition tables */
+	if (sharpsl_nand_read_laddr(master,
+				    PARAM_BLOCK_PARTITIONINFO1,
+				    sizeof(buf1), (u_char *)&buf1) ||
+	    sharpsl_nand_read_laddr(master,
+				    PARAM_BLOCK_PARTITIONINFO2,
+				    sizeof(buf2), (u_char *)&buf2))
+		return -EINVAL;
+
+	/* cleanup logical mgmt (FTL) */
+	sharpsl_nand_cleanup_logical();
+
+	/* compare the two buffers */
+	if (memcmp(&buf1, &buf2, sizeof(buf1))) {
+		pr_err("sharpslpart: PARTITIONINFO 1,2 differ. Quit parser.\n");
+		return -EINVAL;
+	}
+
+	/* check for magics (just in the first) */
+	if (buf1[0].magic != BOOT_MAGIC ||
+	    buf1[1].magic != FSRO_MAGIC ||
+	    buf1[2].magic != FSRW_MAGIC) {
+		pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
+		return -EINVAL;
+	}
+
+	sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) *
+				     SHARPSL_NAND_PARTS, GFP_KERNEL);
+	if (!sharpsl_nand_parts)
+		return -ENOMEM;
+
+	/* original names */
+	sharpsl_nand_parts[0].name = "smf";
+	sharpsl_nand_parts[0].offset = buf1[0].start;
+	sharpsl_nand_parts[0].size = buf1[0].end - buf1[0].start;
+	sharpsl_nand_parts[0].mask_flags = 0;
+
+	sharpsl_nand_parts[1].name = "root";
+	sharpsl_nand_parts[1].offset = buf1[1].start;
+	sharpsl_nand_parts[1].size = buf1[1].end - buf1[1].start;
+	sharpsl_nand_parts[1].mask_flags = 0;
+
+	sharpsl_nand_parts[2].name = "home";
+	sharpsl_nand_parts[2].offset = buf1[2].start;
+	/* discard buf1[2].end, was for older models with 64M flash */
+	sharpsl_nand_parts[2].size = master->size - buf1[2].start;
+	sharpsl_nand_parts[2].mask_flags = 0;
+
+	*pparts = sharpsl_nand_parts;
+	return SHARPSL_NAND_PARTS;
+}
+
+static struct mtd_part_parser sharpsl_mtd_parser = {
+	.parse_fn = sharpsl_parse_mtd_partitions,
+	.name = "sharpslpart",
+};
+module_mtd_part_parser(sharpsl_mtd_parser);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>");
+MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series");
-- 
2.7.4

[PATCH v2 2/3] mtd: nand: sharpsl.c: prefer sharpslpart MTD partition parser

[Andrea Adami]

This is the specific parser for Sharp SL Series (Zaurus).

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 drivers/mtd/nand/sharpsl.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/sharpsl.c b/drivers/mtd/nand/sharpsl.c
index 064ca17..2d1ba0e 100644
--- a/drivers/mtd/nand/sharpsl.c
+++ b/drivers/mtd/nand/sharpsl.c
@@ -108,6 +108,8 @@ static int sharpsl_nand_calculate_ecc(struct mtd_info *mtd, const u_char * dat,
 /*
  * Main initialization routine
  */
+static const char * const probes[] = { "sharpslpart", NULL };
+
 static int sharpsl_nand_probe(struct platform_device *pdev)
 {
 	struct nand_chip *this;
@@ -183,7 +185,7 @@ static int sharpsl_nand_probe(struct platform_device *pdev)
 	/* Register the partitions */
 	mtd->name = "sharpsl-nand";
 
-	err = mtd_device_parse_register(mtd, NULL, NULL,
+	err = mtd_device_parse_register(mtd, probes, NULL,
 					data->partitions, data->nr_partitions);
 	if (err)
 		goto err_add;
-- 
2.7.4

[PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

[Andrea Adami]

This is the specific parser for Sharp SL Series (Zaurus)

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 drivers/mtd/nand/tmio_nand.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
index fc5e773..f3612ac 100644
--- a/drivers/mtd/nand/tmio_nand.c
+++ b/drivers/mtd/nand/tmio_nand.c
@@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
 		cell->disable(dev);
 }
 
+static const char * const probes[] = { "sharpslpart", NULL };
+
 static int tmio_probe(struct platform_device *dev)
 {
 	struct tmio_nand_data *data = dev_get_platdata(&dev->dev);
@@ -440,7 +442,7 @@ static int tmio_probe(struct platform_device *dev)
 		goto err_irq;
 
 	/* Register the partitions */
-	retval = mtd_device_parse_register(mtd, NULL, NULL,
+	retval = mtd_device_parse_register(mtd, probes, NULL,
 					   data ? data->partition : NULL,
 					   data ? data->num_partitions : 0);
 	if (!retval)
-- 
2.7.4

Re: [PATCH v2 0/3] mtd: sharpslpart partition parser

[Andrea Adami]

On Sat, Apr 22, 2017 at 1:20 PM, Andrea Adami <andrea.adami@gmail.com> wrote:
> This patchset introduces a simple partition parser for the Sharp SL
> Series PXA handhelds. More details in the commit text.
>
> I have set in cc the ARM PXA maintainers because this is the MTD part of
> a planned wider patchset cleaning the Zaurus board files.
>
> Changelog:
> v1 initial import of 2.4 sources [1]
> v2 refactor applying many suggested fixes [2]
>
> [1] https://github.com/LinuxPDA/Sharp_FTL_2.4.20
> [2] https://github.com/LinuxPDA/linux/commits/sharpslpart_v2
>
> Andrea Adami (3):
>   mtd: sharpsl: add sharpslpart MTD partition parser
>   mtd: nand: sharpsl.c: prefer sharpslpart MTD partition parser
>   mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser
>
>  drivers/mtd/Kconfig          |   8 ++
>  drivers/mtd/Makefile         |   2 +
>  drivers/mtd/nand/sharpsl.c   |   4 +-
>  drivers/mtd/nand/tmio_nand.c |   4 +-
>  drivers/mtd/sharpsl_ftl.c    | 219 +++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/sharpsl_ftl.h    |  34 +++++++
>  drivers/mtd/sharpslpart.c    | 132 ++++++++++++++++++++++++++
>  7 files changed, 401 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/mtd/sharpsl_ftl.c
>  create mode 100644 drivers/mtd/sharpsl_ftl.h
>  create mode 100644 drivers/mtd/sharpslpart.c
>
> --
> 2.7.4
>

Hello,

I'd be thankful for a second review after the big changes from v1
which was just a proof of concept.
As for the memory management, it totally mimics the other parsers.

The patch has been backported to 4.4 [1] and up to 3.10 [2] with
minimal changes to module init/exit:
since linux 4.5 (commit b8f70ba mtd: kill off MTD partition parser
boilerplate) we use module_mtd_part_parser().

With this parser we have finally a single linux-as bootloader kernel
covering the 4 Zaurus pxa 27x devices (repartitioned or not).

Thanks in advance
Andrea

[1] http://cgit.openembedded.org/meta-handheld/tree/recipes-kernel/linux/linux-handheld-4.4/sharpslpart/0001-mtd-sharpsl-add-sharpslpart-MTD-partition-parser.patch
[2] https://github.com/greguu/linux-3.10.y-c3x00-f2fs-kexec-r0/commit/f087900fbd778cce642a8e24e7351a0d814bb9a7

Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

[Brian Norris]

On Sat, Apr 22, 2017 at 01:20:13PM +0200, Andrea Adami wrote:
> This is the specific parser for Sharp SL Series (Zaurus)
> 
> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> ---
>  drivers/mtd/nand/tmio_nand.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
> index fc5e773..f3612ac 100644
> --- a/drivers/mtd/nand/tmio_nand.c
> +++ b/drivers/mtd/nand/tmio_nand.c
> @@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
>  		cell->disable(dev);
>  }
>  
> +static const char * const probes[] = { "sharpslpart", NULL };

This breaks anyone who might have used (or might want to use) the ofpart
or cmdlinepart parsers. At a minimum, you need to include those in your
array here.

But really, I'd rather not add any more parser listings like this in
drivers. Parser selection should be determined by the platform, not by
the driver. See my last response to Rafal, who is trying to extend
support for device-tree based listing of parsers:

http://lists.infradead.org/pipermail/linux-mtd/2017-April/073729.html

He has some more work posted to the mailing list since then; search the
archives.

I'll take a look at the parser itself, and maybe we can merge that. But
I'm not likely to merge this patch, in any form.

Brian

> +
>  static int tmio_probe(struct platform_device *dev)
>  {
>  	struct tmio_nand_data *data = dev_get_platdata(&dev->dev);
> @@ -440,7 +442,7 @@ static int tmio_probe(struct platform_device *dev)
>  		goto err_irq;
>  
>  	/* Register the partitions */
> -	retval = mtd_device_parse_register(mtd, NULL, NULL,
> +	retval = mtd_device_parse_register(mtd, probes, NULL,
>  					   data ? data->partition : NULL,
>  					   data ? data->num_partitions : 0);
>  	if (!retval)
> -- 
> 2.7.4
> 

Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

[Andrea Adami]

On Thu, May 25, 2017 at 9:25 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Sat, Apr 22, 2017 at 01:20:13PM +0200, Andrea Adami wrote:
>> This is the specific parser for Sharp SL Series (Zaurus)
>>
>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>> ---
>>  drivers/mtd/nand/tmio_nand.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
>> index fc5e773..f3612ac 100644
>> --- a/drivers/mtd/nand/tmio_nand.c
>> +++ b/drivers/mtd/nand/tmio_nand.c
>> @@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
>>               cell->disable(dev);
>>  }
>>
>> +static const char * const probes[] = { "sharpslpart", NULL };
>
> This breaks anyone who might have used (or might want to use) the ofpart
> or cmdlinepart parsers. At a minimum, you need to include those in your
> array here.

I have been under the wrong assumption there is cmdlinepart as last
option (if compiled) so I have taken a wrong example.
Grepping in /mt for probes gives many examples: what if I change it with

static const char * const probes[] = { "sharpslpart", "cmdlinepart", NULL };

ofpart is utopic at the moment: these machines are not yet converted
to devicetree and it will take a while.

With this patchset we can move a step forward DT, removing all the
static partition definition from spitz.c, tosa.c, corgi.c and poodle.c

I don't dare adding ofpart here: this will be done once Zaurus pxa
platform is moved to devicetree.

>
> But really, I'd rather not add any more parser listings like this in
> drivers. Parser selection should be determined by the platform, not by
> the driver. See my last response to Rafal, who is trying to extend
> support for device-tree based listing of parsers:
>
> http://lists.infradead.org/pipermail/linux-mtd/2017-April/073729.html

Ok then but remember these are obsolete devices and as far as I know
these nand drivers are only used on Zaurus devices. No future use I
guess.

>
> He has some more work posted to the mailing list since then; search the
> archives.
>
> I'll take a look at the parser itself, and maybe we can merge that. But
> I'm not likely to merge this patch, in any form.
>
The little parser itself is universal for all Zaurus pxa variants.
As said above, please consider we can remove many lines of board code.

Thanks
Andrea

> Brian
>
>> +
>>  static int tmio_probe(struct platform_device *dev)
>>  {
>>       struct tmio_nand_data *data = dev_get_platdata(&dev->dev);
>> @@ -440,7 +442,7 @@ static int tmio_probe(struct platform_device *dev)
>>               goto err_irq;
>>
>>       /* Register the partitions */
>> -     retval = mtd_device_parse_register(mtd, NULL, NULL,
>> +     retval = mtd_device_parse_register(mtd, probes, NULL,
>>                                          data ? data->partition : NULL,
>>                                          data ? data->num_partitions : 0);
>>       if (!retval)
>> --
>> 2.7.4
>>

Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

[Brian Norris]

On Thu, May 25, 2017 at 10:47:37PM +0200, Andrea Adami wrote:
> On Thu, May 25, 2017 at 9:25 PM, Brian Norris
> <computersforpeace@gmail.com> wrote:
> > On Sat, Apr 22, 2017 at 01:20:13PM +0200, Andrea Adami wrote:
> >> This is the specific parser for Sharp SL Series (Zaurus)
> >>
> >> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> >> ---
> >>  drivers/mtd/nand/tmio_nand.c | 4 +++-
> >>  1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
> >> index fc5e773..f3612ac 100644
> >> --- a/drivers/mtd/nand/tmio_nand.c
> >> +++ b/drivers/mtd/nand/tmio_nand.c
> >> @@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
> >>               cell->disable(dev);
> >>  }
> >>
> >> +static const char * const probes[] = { "sharpslpart", NULL };
> >
> > This breaks anyone who might have used (or might want to use) the ofpart
> > or cmdlinepart parsers. At a minimum, you need to include those in your
> > array here.
> 
> I have been under the wrong assumption there is cmdlinepart as last
> option (if compiled) so I have taken a wrong example.
> Grepping in /mt for probes gives many examples: what if I change it with
> 
> static const char * const probes[] = { "sharpslpart", "cmdlinepart", NULL };
> 
> ofpart is utopic at the moment: these machines are not yet converted
> to devicetree and it will take a while.
> 
> With this patchset we can move a step forward DT, removing all the
> static partition definition from spitz.c, tosa.c, corgi.c and poodle.c
> 
> I don't dare adding ofpart here: this will be done once Zaurus pxa
> platform is moved to devicetree.

What's the harm in including ofpart? It will be silently skipped if you
don't have a conforming device tree.

> > But really, I'd rather not add any more parser listings like this in
> > drivers. Parser selection should be determined by the platform, not by
> > the driver. See my last response to Rafal, who is trying to extend
> > support for device-tree based listing of parsers:
> >
> > http://lists.infradead.org/pipermail/linux-mtd/2017-April/073729.html
> 
> Ok then but remember these are obsolete devices and as far as I know
> these nand drivers are only used on Zaurus devices. No future use I
> guess.

Yes, but the point is I don't want new examples of a bad pattern. And if
you ever do gain device tree support, I would then be "breaking" your
device tree if I dropped "sharpslpart" from your probe list.

> > He has some more work posted to the mailing list since then; search the
> > archives.
> >
> > I'll take a look at the parser itself, and maybe we can merge that. But
> > I'm not likely to merge this patch, in any form.
> >
> The little parser itself is universal for all Zaurus pxa variants.
> As said above, please consider we can remove many lines of board code.

Speaking of board code: since this is all initialized by board files,
why can't you put the "platform information" (i.e., the partition parser
type(s)) in the platform data? e.g, struct sharpsl_nand_platform_data or
struct tmio_nand_data. That'd resolve my concern about hardcoding lists
in the driver.

Brian

Re: [PATCH v2 3/3] mtd: nand: tmio_nand.c: prefer sharpslpart MTD partition parser

[Andrea Adami]

On Thu, May 25, 2017 at 11:10 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Thu, May 25, 2017 at 10:47:37PM +0200, Andrea Adami wrote:
>> On Thu, May 25, 2017 at 9:25 PM, Brian Norris
>> <computersforpeace@gmail.com> wrote:
>> > On Sat, Apr 22, 2017 at 01:20:13PM +0200, Andrea Adami wrote:
>> >> This is the specific parser for Sharp SL Series (Zaurus)
>> >>
>> >> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>> >> ---
>> >>  drivers/mtd/nand/tmio_nand.c | 4 +++-
>> >>  1 file changed, 3 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/drivers/mtd/nand/tmio_nand.c b/drivers/mtd/nand/tmio_nand.c
>> >> index fc5e773..f3612ac 100644
>> >> --- a/drivers/mtd/nand/tmio_nand.c
>> >> +++ b/drivers/mtd/nand/tmio_nand.c
>> >> @@ -357,6 +357,8 @@ static void tmio_hw_stop(struct platform_device *dev, struct tmio_nand *tmio)
>> >>               cell->disable(dev);
>> >>  }
>> >>
>> >> +static const char * const probes[] = { "sharpslpart", NULL };
>> >
>> > This breaks anyone who might have used (or might want to use) the ofpart
>> > or cmdlinepart parsers. At a minimum, you need to include those in your
>> > array here.
>>
>> I have been under the wrong assumption there is cmdlinepart as last
>> option (if compiled) so I have taken a wrong example.
>> Grepping in /mt for probes gives many examples: what if I change it with
>>
>> static const char * const probes[] = { "sharpslpart", "cmdlinepart", NULL };
>>
>> ofpart is utopic at the moment: these machines are not yet converted
>> to devicetree and it will take a while.
>>
>> With this patchset we can move a step forward DT, removing all the
>> static partition definition from spitz.c, tosa.c, corgi.c and poodle.c
>>
>> I don't dare adding ofpart here: this will be done once Zaurus pxa
>> platform is moved to devicetree.
>
> What's the harm in including ofpart? It will be silently skipped if you
> don't have a conforming device tree.
>
>> > But really, I'd rather not add any more parser listings like this in
>> > drivers. Parser selection should be determined by the platform, not by
>> > the driver. See my last response to Rafal, who is trying to extend
>> > support for device-tree based listing of parsers:
>> >
>> > http://lists.infradead.org/pipermail/linux-mtd/2017-April/073729.html
>>
>> Ok then but remember these are obsolete devices and as far as I know
>> these nand drivers are only used on Zaurus devices. No future use I
>> guess.
>
> Yes, but the point is I don't want new examples of a bad pattern. And if
> you ever do gain device tree support, I would then be "breaking" your
> device tree if I dropped "sharpslpart" from your probe list.
>
>> > He has some more work posted to the mailing list since then; search the
>> > archives.
>> >
>> > I'll take a look at the parser itself, and maybe we can merge that. But
>> > I'm not likely to merge this patch, in any form.
>> >
>> The little parser itself is universal for all Zaurus pxa variants.
>> As said above, please consider we can remove many lines of board code.
>
> Speaking of board code: since this is all initialized by board files,
> why can't you put the "platform information" (i.e., the partition parser
> type(s)) in the platform data? e.g, struct sharpsl_nand_platform_data or
> struct tmio_nand_data. That'd resolve my concern about hardcoding lists
> in the driver.
>
> Brian

Brian,

I now understand your objections about hardcoding parsers in the drivers.
I'll ask the maintainers in case there were any objections: the patch
will cross mach-pxa and drivers/mtd.

Thanks again

Andrea