[PATCH v4 0/9] mtd: sharpslpart partition parser

[PATCH v4 0/9] 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. The MFD maintainers
are also in cc (tmio.h change).

Changelog:
v1 initial import of 2.4 sources [1]
v2 refactor applying many suggested fixes [2]
v3 put the partition parser types in the platform data [3]
v4 refactor after ML review [4]

[1] http://support.ezaurus.com/developer/source/source_dl.asp
[2] https://github.com/LinuxPDA/linux/commits/sharpslpart_v2
[3] https://github.com/LinuxPDA/linux/commits/sharpslpart_v3
[4] https://github.com/LinuxPDA/linux/commits/sharpslpart_v4

Andrea Adami (9):
  mtd: sharpslpart: add sharpslpart MTD partition parser
  mtd: nand: sharpsl.h: support partition parser types
  mfd: tmio.h: support partition parser types
  mtd: nand: sharpsl.c: take in account partition parser types
  mtd: nand: tmio_nand.c: take in account partition parser types
  ARM: pxa: corgi.c: remove hardcoded partitioning, use sharpslpart
    parser
  ARM: pxa: tosa.c: remove hardcoded partitioning, use sharpslpart
    parser
  ARM: pxa: spitz.c: remove hardcoded partitioning, use sharpslpart
    parser
  ARM: pxa: poodle.c: remove hardcoded partitioning, use sharpslpart
    parser

 arch/arm/mach-pxa/corgi.c    |  31 +---
 arch/arm/mach-pxa/poodle.c   |  28 +---
 arch/arm/mach-pxa/spitz.c    |  34 +---
 arch/arm/mach-pxa/tosa.c     |  28 +---
 drivers/mtd/Kconfig          |   8 +
 drivers/mtd/Makefile         |   1 +
 drivers/mtd/nand/sharpsl.c   |   2 +-
 drivers/mtd/nand/tmio_nand.c |   4 +-
 drivers/mtd/sharpslpart.c    | 391 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tmio.h     |   1 +
 include/linux/mtd/sharpsl.h  |   1 +
 11 files changed, 439 insertions(+), 90 deletions(-)
 create mode 100644 drivers/mtd/sharpslpart.c

-- 
2.7.4

[PATCH v4 0/9] 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. The MFD maintainers
are also in cc (tmio.h change).

Changelog:
v1 initial import of 2.4 sources [1]
v2 refactor applying many suggested fixes [2]
v3 put the partition parser types in the platform data [3]
v4 refactor after ML review [4]

[1] http://support.ezaurus.com/developer/source/source_dl.asp
[2] https://github.com/LinuxPDA/linux/commits/sharpslpart_v2
[3] https://github.com/LinuxPDA/linux/commits/sharpslpart_v3
[4] https://github.com/LinuxPDA/linux/commits/sharpslpart_v4

Andrea Adami (9):
  mtd: sharpslpart: add sharpslpart MTD partition parser
  mtd: nand: sharpsl.h: support partition parser types
  mfd: tmio.h: support partition parser types
  mtd: nand: sharpsl.c: take in account partition parser types
  mtd: nand: tmio_nand.c: take in account partition parser types
  ARM: pxa: corgi.c: remove hardcoded partitioning, use sharpslpart
    parser
  ARM: pxa: tosa.c: remove hardcoded partitioning, use sharpslpart
    parser
  ARM: pxa: spitz.c: remove hardcoded partitioning, use sharpslpart
    parser
  ARM: pxa: poodle.c: remove hardcoded partitioning, use sharpslpart
    parser

 arch/arm/mach-pxa/corgi.c    |  31 +---
 arch/arm/mach-pxa/poodle.c   |  28 +---
 arch/arm/mach-pxa/spitz.c    |  34 +---
 arch/arm/mach-pxa/tosa.c     |  28 +---
 drivers/mtd/Kconfig          |   8 +
 drivers/mtd/Makefile         |   1 +
 drivers/mtd/nand/sharpsl.c   |   2 +-
 drivers/mtd/nand/tmio_nand.c |   4 +-
 drivers/mtd/sharpslpart.c    | 391 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/mfd/tmio.h     |   1 +
 include/linux/mtd/sharpsl.h  |   1 +
 11 files changed, 439 insertions(+), 90 deletions(-)
 create mode 100644 drivers/mtd/sharpslpart.c

-- 
2.7.4

[PATCH v4 1/9] mtd: sharpslpart: 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      |   1 +
 drivers/mtd/sharpslpart.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 400 insertions(+)
 create mode 100644 drivers/mtd/sharpslpart.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index e83a279..b196a69 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 || COMPILE_TEST
+	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..e5ef07f 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -13,6 +13,7 @@ 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)	+= sharpslpart.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
new file mode 100644
index 0000000..02b721b
--- /dev/null
+++ b/drivers/mtd/sharpslpart.c
@@ -0,0 +1,391 @@
+/*
+ * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL
+ * for logical addressing, as used on the PXA models of the SHARP SL Series.
+ *
+ * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
+ *
+ * Based on 2.4 sources:
+ *  drivers/mtd/nand/sharp_sl_logical.c
+ *  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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.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
+
+#define BLOCK_IS_RESERVED		0xffff
+#define BLOCK_UNMASK			0x07fe
+#define BLOCK_UNMASK_COMPLEMENT		1
+
+/* 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 buf_start(x)			le32_to_cpu(buf1[x].start)
+#define buf_end(x)			le32_to_cpu(buf1[x].end)
+#define buf_magic(x)			be32_to_cpu(buf1[x].magic)
+
+#define BOOT_MAGIC			0x424f4f54   /* BOOT */
+#define FSRO_MAGIC			0x4653524f   /* FSRO */
+#define FSRW_MAGIC			0x46535257   /* FSRW */
+
+/* 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 */
+};
+
+struct mtd_logical *sharpsl_mtd_logical;
+
+/*
+ * SHARP SL FTL ancillary functions
+ *
+ */
+
+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;
+}
+
+/*
+ * The logical block number assigned to a physical block is stored in the OOB
+ * of the first page, in 3 16-bit copies with the following layout:
+ *
+ * 01234567 89abcdef
+ * -------- --------
+ * ECC BB   xyxyxy
+ *
+ * When reading we check that the first two copies agree.
+ * In case of error, matching is tried using the following pairs.
+ * Reserved values 0xffff mean the block is kept for wear leveling.
+ *
+ * 01234567 89abcdef
+ * -------- --------
+ * ECC BB   xyxy    oob[8]==oob[10] && oob[9]==oob[11]   -> byte0=8   byte1=9
+ * ECC BB     xyxy  oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10  byte1=11
+ * ECC BB   xy  xy  oob[12]==oob[8] && oob[13]==oob[9]   -> byte0=12  byte1=13
+ *
+ */
+
+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 {
+		/* wrong oob fingerprint, maybe here by mistake? */
+		return UINT_MAX;
+	}
+
+	us = oob[good0] | oob[good1] << 8;
+
+	/* parity check */
+	if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)
+		return (UINT_MAX - 1);
+
+	/* reserved */
+	if (us == BLOCK_IS_RESERVED)
+		return BLOCK_IS_RESERVED;
+	else
+		return (us & BLOCK_UNMASK) >> 1;
+}
+
+int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)
+{
+	struct mtd_logical *logical;
+	u_int block_num, log_num;
+	loff_t block_adr;
+	u_char *oob;
+	int i;
+
+	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 = kmalloc_array(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;
+
+		if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
+			continue;
+
+		/* get logical block */
+		log_num = sharpsl_nand_get_logical_num(oob);
+
+		/* FTL is not used? Exit here if the oob fingerprint is wrong */
+		if (log_num == UINT_MAX) {
+			pr_info("sharpslpart: Sharp SL FTL not found. Quit parser.\n");
+			kfree(logical->log2phy);
+			kfree(logical);
+			kfree(oob);
+			return -EINVAL;
+		}
+
+		/* skip out of range and not unique values */
+		if (log_num < logical->logmax) {
+			if (logical->log2phy[log_num] == UINT_MAX)
+				logical->log2phy[log_num] = block_num;
+		}
+	}
+	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(struct mtd_logical *sharpsl_mtd_logical)
+{
+	struct mtd_logical *logical = sharpsl_mtd_logical;
+
+	sharpsl_mtd_logical = NULL;
+
+	kfree(logical->log2phy);
+	logical->log2phy = NULL;
+	kfree(logical);
+	logical = NULL;
+}
+
+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 err;
+
+	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;
+
+	err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
+	/* Ignore corrected ECC errors */
+	if (mtd_is_bitflip(err))
+		err = 0;
+	if (!err && retlen != len)
+		err = -EIO;
+	if (err)
+		pr_err("sharpslpart: error, read failed at %#llx\n",
+		       block_adr + block_ofs);
+
+	return err;
+}
+
+/*
+ * MTD Partition Parser
+ *
+ */
+
+struct sharpsl_nand_partitioninfo {
+	u32 start;
+	u32 end;
+	u32 magic;
+	u32 reserved;
+};
+
+/*
+ * 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  ................
+ *
+ */
+
+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;
+	int err;
+
+	/* init logical mgmt (FTL) */
+	if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
+		return -EINVAL;
+
+	/* read the two partition tables */
+	err = sharpsl_nand_read_laddr(master,
+				      PARAM_BLOCK_PARTITIONINFO1,
+				      sizeof(buf1), (u_char *)&buf1);
+	if (!err) {
+		sharpsl_nand_read_laddr(master,
+					PARAM_BLOCK_PARTITIONINFO2,
+					sizeof(buf2), (u_char *)&buf2);
+	} else {
+		sharpsl_nand_cleanup_logical(sharpsl_mtd_logical);
+		return err;
+	}
+
+	/* cleanup logical mgmt (FTL) */
+	sharpsl_nand_cleanup_logical(sharpsl_mtd_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 (buf_magic(0) != BOOT_MAGIC ||
+	    buf_magic(1) != FSRO_MAGIC ||
+	    buf_magic(2) != FSRW_MAGIC) {
+		pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
+		return -EINVAL;
+	}
+
+	/* fixup for hardcoded value 64 MiB (for older models) */
+	buf1[2].end = cpu_to_le32(master->size);
+
+	/* extra sanity check */
+	if (buf_end(0) <= buf_start(0) ||
+	    buf_start(1) < buf_end(0) ||
+	    buf_end(1) <= buf_start(1) ||
+	    buf_start(2) < buf_end(1) ||
+	    buf_end(2) <= buf_start(2)) {
+		pr_err("sharpslpart: partition sizes 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 = buf_start(0);
+	sharpsl_nand_parts[0].size = buf_end(0) - buf_start(0);
+
+	sharpsl_nand_parts[1].name = "root";
+	sharpsl_nand_parts[1].offset = buf_start(1);
+	sharpsl_nand_parts[1].size = buf_end(1) - buf_start(1);
+
+	sharpsl_nand_parts[2].name = "home";
+	sharpsl_nand_parts[2].offset = buf_start(2);
+	sharpsl_nand_parts[2].size = buf_end(2) - buf_start(2);
+
+	*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 v4 1/9] mtd: sharpslpart: 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      |   1 +
 drivers/mtd/sharpslpart.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 400 insertions(+)
 create mode 100644 drivers/mtd/sharpslpart.c

diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
index e83a279..b196a69 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 || COMPILE_TEST
+	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..e5ef07f 100644
--- a/drivers/mtd/Makefile
+++ b/drivers/mtd/Makefile
@@ -13,6 +13,7 @@ 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)	+= sharpslpart.o
 
 # 'Users' - code which presents functionality to userspace.
 obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
new file mode 100644
index 0000000..02b721b
--- /dev/null
+++ b/drivers/mtd/sharpslpart.c
@@ -0,0 +1,391 @@
+/*
+ * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL
+ * for logical addressing, as used on the PXA models of the SHARP SL Series.
+ *
+ * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
+ *
+ * Based on 2.4 sources:
+ *  drivers/mtd/nand/sharp_sl_logical.c
+ *  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.
+ *
+ */
+
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/mtd/mtd.h>
+#include <linux/mtd/partitions.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
+
+#define BLOCK_IS_RESERVED		0xffff
+#define BLOCK_UNMASK			0x07fe
+#define BLOCK_UNMASK_COMPLEMENT		1
+
+/* 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 buf_start(x)			le32_to_cpu(buf1[x].start)
+#define buf_end(x)			le32_to_cpu(buf1[x].end)
+#define buf_magic(x)			be32_to_cpu(buf1[x].magic)
+
+#define BOOT_MAGIC			0x424f4f54   /* BOOT */
+#define FSRO_MAGIC			0x4653524f   /* FSRO */
+#define FSRW_MAGIC			0x46535257   /* FSRW */
+
+/* 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 */
+};
+
+struct mtd_logical *sharpsl_mtd_logical;
+
+/*
+ * SHARP SL FTL ancillary functions
+ *
+ */
+
+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;
+}
+
+/*
+ * The logical block number assigned to a physical block is stored in the OOB
+ * of the first page, in 3 16-bit copies with the following layout:
+ *
+ * 01234567 89abcdef
+ * -------- --------
+ * ECC BB   xyxyxy
+ *
+ * When reading we check that the first two copies agree.
+ * In case of error, matching is tried using the following pairs.
+ * Reserved values 0xffff mean the block is kept for wear leveling.
+ *
+ * 01234567 89abcdef
+ * -------- --------
+ * ECC BB   xyxy    oob[8]==oob[10] && oob[9]==oob[11]   -> byte0=8   byte1=9
+ * ECC BB     xyxy  oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10  byte1=11
+ * ECC BB   xy  xy  oob[12]==oob[8] && oob[13]==oob[9]   -> byte0=12  byte1=13
+ *
+ */
+
+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 {
+		/* wrong oob fingerprint, maybe here by mistake? */
+		return UINT_MAX;
+	}
+
+	us = oob[good0] | oob[good1] << 8;
+
+	/* parity check */
+	if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)
+		return (UINT_MAX - 1);
+
+	/* reserved */
+	if (us == BLOCK_IS_RESERVED)
+		return BLOCK_IS_RESERVED;
+	else
+		return (us & BLOCK_UNMASK) >> 1;
+}
+
+int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)
+{
+	struct mtd_logical *logical;
+	u_int block_num, log_num;
+	loff_t block_adr;
+	u_char *oob;
+	int i;
+
+	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 = kmalloc_array(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;
+
+		if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
+			continue;
+
+		/* get logical block */
+		log_num = sharpsl_nand_get_logical_num(oob);
+
+		/* FTL is not used? Exit here if the oob fingerprint is wrong */
+		if (log_num == UINT_MAX) {
+			pr_info("sharpslpart: Sharp SL FTL not found. Quit parser.\n");
+			kfree(logical->log2phy);
+			kfree(logical);
+			kfree(oob);
+			return -EINVAL;
+		}
+
+		/* skip out of range and not unique values */
+		if (log_num < logical->logmax) {
+			if (logical->log2phy[log_num] == UINT_MAX)
+				logical->log2phy[log_num] = block_num;
+		}
+	}
+	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(struct mtd_logical *sharpsl_mtd_logical)
+{
+	struct mtd_logical *logical = sharpsl_mtd_logical;
+
+	sharpsl_mtd_logical = NULL;
+
+	kfree(logical->log2phy);
+	logical->log2phy = NULL;
+	kfree(logical);
+	logical = NULL;
+}
+
+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 err;
+
+	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;
+
+	err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
+	/* Ignore corrected ECC errors */
+	if (mtd_is_bitflip(err))
+		err = 0;
+	if (!err && retlen != len)
+		err = -EIO;
+	if (err)
+		pr_err("sharpslpart: error, read failed at %#llx\n",
+		       block_adr + block_ofs);
+
+	return err;
+}
+
+/*
+ * MTD Partition Parser
+ *
+ */
+
+struct sharpsl_nand_partitioninfo {
+	u32 start;
+	u32 end;
+	u32 magic;
+	u32 reserved;
+};
+
+/*
+ * 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  ................
+ *
+ */
+
+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;
+	int err;
+
+	/* init logical mgmt (FTL) */
+	if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
+		return -EINVAL;
+
+	/* read the two partition tables */
+	err = sharpsl_nand_read_laddr(master,
+				      PARAM_BLOCK_PARTITIONINFO1,
+				      sizeof(buf1), (u_char *)&buf1);
+	if (!err) {
+		sharpsl_nand_read_laddr(master,
+					PARAM_BLOCK_PARTITIONINFO2,
+					sizeof(buf2), (u_char *)&buf2);
+	} else {
+		sharpsl_nand_cleanup_logical(sharpsl_mtd_logical);
+		return err;
+	}
+
+	/* cleanup logical mgmt (FTL) */
+	sharpsl_nand_cleanup_logical(sharpsl_mtd_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 (buf_magic(0) != BOOT_MAGIC ||
+	    buf_magic(1) != FSRO_MAGIC ||
+	    buf_magic(2) != FSRW_MAGIC) {
+		pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
+		return -EINVAL;
+	}
+
+	/* fixup for hardcoded value 64 MiB (for older models) */
+	buf1[2].end = cpu_to_le32(master->size);
+
+	/* extra sanity check */
+	if (buf_end(0) <= buf_start(0) ||
+	    buf_start(1) < buf_end(0) ||
+	    buf_end(1) <= buf_start(1) ||
+	    buf_start(2) < buf_end(1) ||
+	    buf_end(2) <= buf_start(2)) {
+		pr_err("sharpslpart: partition sizes 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 = buf_start(0);
+	sharpsl_nand_parts[0].size = buf_end(0) - buf_start(0);
+
+	sharpsl_nand_parts[1].name = "root";
+	sharpsl_nand_parts[1].offset = buf_start(1);
+	sharpsl_nand_parts[1].size = buf_end(1) - buf_start(1);
+
+	sharpsl_nand_parts[2].name = "home";
+	sharpsl_nand_parts[2].offset = buf_start(2);
+	sharpsl_nand_parts[2].size = buf_end(2) - buf_start(2);
+
+	*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 v4 2/9] mtd: nand: sharpsl.h: support partition parser types

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 include/linux/mtd/sharpsl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mtd/sharpsl.h b/include/linux/mtd/sharpsl.h
index 65e91d0..c0e0be2 100644
--- a/include/linux/mtd/sharpsl.h
+++ b/include/linux/mtd/sharpsl.h
@@ -17,4 +17,5 @@ struct sharpsl_nand_platform_data {
 	const struct mtd_ooblayout_ops *ecc_layout;
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;
+	const char *const	*types;	/* names of parsers to use if any */
 };
-- 
2.7.4

[PATCH v4 2/9] mtd: nand: sharpsl.h: support partition parser types

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 include/linux/mtd/sharpsl.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mtd/sharpsl.h b/include/linux/mtd/sharpsl.h
index 65e91d0..c0e0be2 100644
--- a/include/linux/mtd/sharpsl.h
+++ b/include/linux/mtd/sharpsl.h
@@ -17,4 +17,5 @@ struct sharpsl_nand_platform_data {
 	const struct mtd_ooblayout_ops *ecc_layout;
 	struct mtd_partition	*partitions;
 	unsigned int		nr_partitions;
+	const char *const	*types;	/* names of parsers to use if any */
 };
-- 
2.7.4

[PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 include/linux/mfd/tmio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index a1520d8..23bb069 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -139,6 +139,7 @@ struct tmio_nand_data {
 	struct nand_bbt_descr	*badblock_pattern;
 	struct mtd_partition	*partition;
 	unsigned int		num_partitions;
+	const char *const	*types;	/* names of parsers to use if any */
 };
 
 #define FBIO_TMIO_ACC_WRITE	0x7C639300
-- 
2.7.4

[PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 include/linux/mfd/tmio.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
index a1520d8..23bb069 100644
--- a/include/linux/mfd/tmio.h
+++ b/include/linux/mfd/tmio.h
@@ -139,6 +139,7 @@ struct tmio_nand_data {
 	struct nand_bbt_descr	*badblock_pattern;
 	struct mtd_partition	*partition;
 	unsigned int		num_partitions;
+	const char *const	*types;	/* names of parsers to use if any */
 };
 
 #define FBIO_TMIO_ACC_WRITE	0x7C639300
-- 
2.7.4

[PATCH v4 4/9] mtd: nand: sharpsl.c: take in account partition parser types

[Andrea Adami]

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

diff --git a/drivers/mtd/nand/sharpsl.c b/drivers/mtd/nand/sharpsl.c
index 064ca17..87c6fc2 100644
--- a/drivers/mtd/nand/sharpsl.c
+++ b/drivers/mtd/nand/sharpsl.c
@@ -183,7 +183,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, data->types, NULL,
 					data->partitions, data->nr_partitions);
 	if (err)
 		goto err_add;
-- 
2.7.4

[PATCH v4 4/9] mtd: nand: sharpsl.c: take in account partition parser types

[Andrea Adami]

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

diff --git a/drivers/mtd/nand/sharpsl.c b/drivers/mtd/nand/sharpsl.c
index 064ca17..87c6fc2 100644
--- a/drivers/mtd/nand/sharpsl.c
+++ b/drivers/mtd/nand/sharpsl.c
@@ -183,7 +183,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, data->types, NULL,
 					data->partitions, data->nr_partitions);
 	if (err)
 		goto err_add;
-- 
2.7.4


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH v4 4/9] mtd: nand: sharpsl.c: take in account partition parser types

[Andrea Adami]

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

diff --git a/drivers/mtd/nand/sharpsl.c b/drivers/mtd/nand/sharpsl.c
index 064ca17..87c6fc2 100644
--- a/drivers/mtd/nand/sharpsl.c
+++ b/drivers/mtd/nand/sharpsl.c
@@ -183,7 +183,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, data->types, NULL,
 					data->partitions, data->nr_partitions);
 	if (err)
 		goto err_add;
-- 
2.7.4

[PATCH v4 5/9] mtd: nand: tmio_nand.c: take in account partition parser types

[Andrea Adami]

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..6a53582 100644
--- a/drivers/mtd/nand/tmio_nand.c
+++ b/drivers/mtd/nand/tmio_nand.c
@@ -440,7 +440,9 @@ 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,
+					   data ? data->types : NULL,
+					   NULL,
 					   data ? data->partition : NULL,
 					   data ? data->num_partitions : 0);
 	if (!retval)
-- 
2.7.4

[PATCH v4 5/9] mtd: nand: tmio_nand.c: take in account partition parser types

[Andrea Adami]

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..6a53582 100644
--- a/drivers/mtd/nand/tmio_nand.c
+++ b/drivers/mtd/nand/tmio_nand.c
@@ -440,7 +440,9 @@ 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,
+					   data ? data->types : NULL,
+					   NULL,
 					   data ? data->partition : NULL,
 					   data ? data->num_partitions : 0);
 	if (!retval)
-- 
2.7.4

[PATCH v4 6/9] ARM: pxa: corgi.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/corgi.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
index 7270f0d..2412add 100644
--- a/arch/arm/mach-pxa/corgi.c
+++ b/arch/arm/mach-pxa/corgi.c
@@ -606,24 +606,6 @@ static void __init corgi_init_spi(void)
 static inline void corgi_init_spi(void) {}
 #endif
 
-static struct mtd_partition sharpsl_nand_partitions[] = {
-	{
-		.name = "System Area",
-		.offset = 0,
-		.size = 7 * 1024 * 1024,
-	},
-	{
-		.name = "Root Filesystem",
-		.offset = 7 * 1024 * 1024,
-		.size = 25 * 1024 * 1024,
-	},
-	{
-		.name = "Home Filesystem",
-		.offset = MTDPART_OFS_APPEND,
-		.size = MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr sharpsl_bbt = {
@@ -633,10 +615,16 @@ static struct nand_bbt_descr sharpsl_bbt = {
 	.pattern = scan_ff_pattern
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = {
 	.badblock_pattern	= &sharpsl_bbt,
-	.partitions		= sharpsl_nand_partitions,
-	.nr_partitions		= ARRAY_SIZE(sharpsl_nand_partitions),
+	.types			= probes,
 };
 
 static struct resource sharpsl_nand_resources[] = {
@@ -750,9 +738,6 @@ static void __init corgi_init(void)
 
 	platform_scoop_config = &corgi_pcmcia_config;
 
-	if (machine_is_husky())
-		sharpsl_nand_partitions[1].size = 53 * 1024 * 1024;
-
 	platform_add_devices(devices, ARRAY_SIZE(devices));
 
 	regulator_has_full_constraints();
-- 
2.7.4

[PATCH v4 6/9] ARM: pxa: corgi.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/corgi.c | 31 ++++++++-----------------------
 1 file changed, 8 insertions(+), 23 deletions(-)

diff --git a/arch/arm/mach-pxa/corgi.c b/arch/arm/mach-pxa/corgi.c
index 7270f0d..2412add 100644
--- a/arch/arm/mach-pxa/corgi.c
+++ b/arch/arm/mach-pxa/corgi.c
@@ -606,24 +606,6 @@ static void __init corgi_init_spi(void)
 static inline void corgi_init_spi(void) {}
 #endif
 
-static struct mtd_partition sharpsl_nand_partitions[] = {
-	{
-		.name = "System Area",
-		.offset = 0,
-		.size = 7 * 1024 * 1024,
-	},
-	{
-		.name = "Root Filesystem",
-		.offset = 7 * 1024 * 1024,
-		.size = 25 * 1024 * 1024,
-	},
-	{
-		.name = "Home Filesystem",
-		.offset = MTDPART_OFS_APPEND,
-		.size = MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr sharpsl_bbt = {
@@ -633,10 +615,16 @@ static struct nand_bbt_descr sharpsl_bbt = {
 	.pattern = scan_ff_pattern
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = {
 	.badblock_pattern	= &sharpsl_bbt,
-	.partitions		= sharpsl_nand_partitions,
-	.nr_partitions		= ARRAY_SIZE(sharpsl_nand_partitions),
+	.types			= probes,
 };
 
 static struct resource sharpsl_nand_resources[] = {
@@ -750,9 +738,6 @@ static void __init corgi_init(void)
 
 	platform_scoop_config = &corgi_pcmcia_config;
 
-	if (machine_is_husky())
-		sharpsl_nand_partitions[1].size = 53 * 1024 * 1024;
-
 	platform_add_devices(devices, ARRAY_SIZE(devices));
 
 	regulator_has_full_constraints();
-- 
2.7.4

[PATCH v4 7/9] ARM: pxa: tosa.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/tosa.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-pxa/tosa.c b/arch/arm/mach-pxa/tosa.c
index 13de660..3074aae 100644
--- a/arch/arm/mach-pxa/tosa.c
+++ b/arch/arm/mach-pxa/tosa.c
@@ -673,24 +673,6 @@ static int tosa_tc6393xb_suspend(struct platform_device *dev)
 	return 0;
 }
 
-static struct mtd_partition tosa_nand_partition[] = {
-	{
-		.name	= "smf",
-		.offset	= 0,
-		.size	= 7 * 1024 * 1024,
-	},
-	{
-		.name	= "root",
-		.offset	= MTDPART_OFS_APPEND,
-		.size	= 28 * 1024 * 1024,
-	},
-	{
-		.name	= "home",
-		.offset	= MTDPART_OFS_APPEND,
-		.size	= MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr tosa_tc6393xb_nand_bbt = {
@@ -700,10 +682,16 @@ static struct nand_bbt_descr tosa_tc6393xb_nand_bbt = {
 	.pattern	= scan_ff_pattern
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct tmio_nand_data tosa_tc6393xb_nand_config = {
-	.num_partitions	= ARRAY_SIZE(tosa_nand_partition),
-	.partition	= tosa_nand_partition,
 	.badblock_pattern = &tosa_tc6393xb_nand_bbt,
+	.types		= probes,
 };
 
 static int tosa_tc6393xb_setup(struct platform_device *dev)
-- 
2.7.4

[PATCH v4 7/9] ARM: pxa: tosa.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/tosa.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-pxa/tosa.c b/arch/arm/mach-pxa/tosa.c
index 13de660..3074aae 100644
--- a/arch/arm/mach-pxa/tosa.c
+++ b/arch/arm/mach-pxa/tosa.c
@@ -673,24 +673,6 @@ static int tosa_tc6393xb_suspend(struct platform_device *dev)
 	return 0;
 }
 
-static struct mtd_partition tosa_nand_partition[] = {
-	{
-		.name	= "smf",
-		.offset	= 0,
-		.size	= 7 * 1024 * 1024,
-	},
-	{
-		.name	= "root",
-		.offset	= MTDPART_OFS_APPEND,
-		.size	= 28 * 1024 * 1024,
-	},
-	{
-		.name	= "home",
-		.offset	= MTDPART_OFS_APPEND,
-		.size	= MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr tosa_tc6393xb_nand_bbt = {
@@ -700,10 +682,16 @@ static struct nand_bbt_descr tosa_tc6393xb_nand_bbt = {
 	.pattern	= scan_ff_pattern
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct tmio_nand_data tosa_tc6393xb_nand_config = {
-	.num_partitions	= ARRAY_SIZE(tosa_nand_partition),
-	.partition	= tosa_nand_partition,
 	.badblock_pattern = &tosa_tc6393xb_nand_bbt,
+	.types		= probes,
 };
 
 static int tosa_tc6393xb_setup(struct platform_device *dev)
-- 
2.7.4

[PATCH v4 8/9] ARM: pxa: spitz.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/spitz.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 67d66c7..21a2e42 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -739,21 +739,6 @@ static inline void spitz_lcd_init(void) {}
  * NAND Flash
  ******************************************************************************/
 #if defined(CONFIG_MTD_NAND_SHARPSL) || defined(CONFIG_MTD_NAND_SHARPSL_MODULE)
-static struct mtd_partition spitz_nand_partitions[] = {
-	{
-		.name = "System Area",
-		.offset = 0,
-		.size = 7 * 1024 * 1024,
-	}, {
-		.name = "Root Filesystem",
-		.offset = 7 * 1024 * 1024,
-	}, {
-		.name = "Home Filesystem",
-		.offset = MTDPART_OFS_APPEND,
-		.size = MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr spitz_nand_bbt = {
@@ -808,10 +793,16 @@ static const struct mtd_ooblayout_ops akita_ooblayout_ops = {
 	.free = akita_ooblayout_free,
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct sharpsl_nand_platform_data spitz_nand_pdata = {
 	.badblock_pattern	= &spitz_nand_bbt,
-	.partitions		= spitz_nand_partitions,
-	.nr_partitions		= ARRAY_SIZE(spitz_nand_partitions),
+	.types			= probes,
 };
 
 static struct resource spitz_nand_resources[] = {
@@ -834,14 +825,7 @@ static struct platform_device spitz_nand_device = {
 
 static void __init spitz_nand_init(void)
 {
-	if (machine_is_spitz()) {
-		spitz_nand_partitions[1].size = 5 * 1024 * 1024;
-	} else if (machine_is_akita()) {
-		spitz_nand_partitions[1].size = 58 * 1024 * 1024;
-		spitz_nand_bbt.len = 1;
-		spitz_nand_pdata.ecc_layout = &akita_ooblayout_ops;
-	} else if (machine_is_borzoi()) {
-		spitz_nand_partitions[1].size = 32 * 1024 * 1024;
+	if (machine_is_akita() || machine_is_borzoi()) {
 		spitz_nand_bbt.len = 1;
 		spitz_nand_pdata.ecc_layout = &akita_ooblayout_ops;
 	}
-- 
2.7.4

[PATCH v4 8/9] ARM: pxa: spitz.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/spitz.c | 34 +++++++++-------------------------
 1 file changed, 9 insertions(+), 25 deletions(-)

diff --git a/arch/arm/mach-pxa/spitz.c b/arch/arm/mach-pxa/spitz.c
index 67d66c7..21a2e42 100644
--- a/arch/arm/mach-pxa/spitz.c
+++ b/arch/arm/mach-pxa/spitz.c
@@ -739,21 +739,6 @@ static inline void spitz_lcd_init(void) {}
  * NAND Flash
  ******************************************************************************/
 #if defined(CONFIG_MTD_NAND_SHARPSL) || defined(CONFIG_MTD_NAND_SHARPSL_MODULE)
-static struct mtd_partition spitz_nand_partitions[] = {
-	{
-		.name = "System Area",
-		.offset = 0,
-		.size = 7 * 1024 * 1024,
-	}, {
-		.name = "Root Filesystem",
-		.offset = 7 * 1024 * 1024,
-	}, {
-		.name = "Home Filesystem",
-		.offset = MTDPART_OFS_APPEND,
-		.size = MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr spitz_nand_bbt = {
@@ -808,10 +793,16 @@ static const struct mtd_ooblayout_ops akita_ooblayout_ops = {
 	.free = akita_ooblayout_free,
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct sharpsl_nand_platform_data spitz_nand_pdata = {
 	.badblock_pattern	= &spitz_nand_bbt,
-	.partitions		= spitz_nand_partitions,
-	.nr_partitions		= ARRAY_SIZE(spitz_nand_partitions),
+	.types			= probes,
 };
 
 static struct resource spitz_nand_resources[] = {
@@ -834,14 +825,7 @@ static struct platform_device spitz_nand_device = {
 
 static void __init spitz_nand_init(void)
 {
-	if (machine_is_spitz()) {
-		spitz_nand_partitions[1].size = 5 * 1024 * 1024;
-	} else if (machine_is_akita()) {
-		spitz_nand_partitions[1].size = 58 * 1024 * 1024;
-		spitz_nand_bbt.len = 1;
-		spitz_nand_pdata.ecc_layout = &akita_ooblayout_ops;
-	} else if (machine_is_borzoi()) {
-		spitz_nand_partitions[1].size = 32 * 1024 * 1024;
+	if (machine_is_akita() || machine_is_borzoi()) {
 		spitz_nand_bbt.len = 1;
 		spitz_nand_pdata.ecc_layout = &akita_ooblayout_ops;
 	}
-- 
2.7.4

[PATCH v4 9/9] ARM: pxa: poodle.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/poodle.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-pxa/poodle.c b/arch/arm/mach-pxa/poodle.c
index 62a1191..4881a43 100644
--- a/arch/arm/mach-pxa/poodle.c
+++ b/arch/arm/mach-pxa/poodle.c
@@ -333,24 +333,6 @@ static struct pxafb_mach_info poodle_fb_info = {
 	.lcd_conn	= LCD_COLOR_TFT_16BPP,
 };
 
-static struct mtd_partition sharpsl_nand_partitions[] = {
-	{
-		.name = "System Area",
-		.offset = 0,
-		.size = 7 * 1024 * 1024,
-	},
-	{
-		.name = "Root Filesystem",
-		.offset = 7 * 1024 * 1024,
-		.size = 22 * 1024 * 1024,
-	},
-	{
-		.name = "Home Filesystem",
-		.offset = MTDPART_OFS_APPEND,
-		.size = MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr sharpsl_bbt = {
@@ -360,10 +342,16 @@ static struct nand_bbt_descr sharpsl_bbt = {
 	.pattern = scan_ff_pattern
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = {
 	.badblock_pattern	= &sharpsl_bbt,
-	.partitions		= sharpsl_nand_partitions,
-	.nr_partitions		= ARRAY_SIZE(sharpsl_nand_partitions),
+	.types			= probes,
 };
 
 static struct resource sharpsl_nand_resources[] = {
-- 
2.7.4

[PATCH v4 9/9] ARM: pxa: poodle.c: remove hardcoded partitioning, use sharpslpart parser

[Andrea Adami]

Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
---
 arch/arm/mach-pxa/poodle.c | 28 ++++++++--------------------
 1 file changed, 8 insertions(+), 20 deletions(-)

diff --git a/arch/arm/mach-pxa/poodle.c b/arch/arm/mach-pxa/poodle.c
index 62a1191..4881a43 100644
--- a/arch/arm/mach-pxa/poodle.c
+++ b/arch/arm/mach-pxa/poodle.c
@@ -333,24 +333,6 @@ static struct pxafb_mach_info poodle_fb_info = {
 	.lcd_conn	= LCD_COLOR_TFT_16BPP,
 };
 
-static struct mtd_partition sharpsl_nand_partitions[] = {
-	{
-		.name = "System Area",
-		.offset = 0,
-		.size = 7 * 1024 * 1024,
-	},
-	{
-		.name = "Root Filesystem",
-		.offset = 7 * 1024 * 1024,
-		.size = 22 * 1024 * 1024,
-	},
-	{
-		.name = "Home Filesystem",
-		.offset = MTDPART_OFS_APPEND,
-		.size = MTDPART_SIZ_FULL,
-	},
-};
-
 static uint8_t scan_ff_pattern[] = { 0xff, 0xff };
 
 static struct nand_bbt_descr sharpsl_bbt = {
@@ -360,10 +342,16 @@ static struct nand_bbt_descr sharpsl_bbt = {
 	.pattern = scan_ff_pattern
 };
 
+static const char * const probes[] = {
+	"cmdlinepart",
+	"ofpart",
+	"sharpslpart",
+	NULL,
+};
+
 static struct sharpsl_nand_platform_data sharpsl_nand_platform_data = {
 	.badblock_pattern	= &sharpsl_bbt,
-	.partitions		= sharpsl_nand_partitions,
-	.nr_partitions		= ARRAY_SIZE(sharpsl_nand_partitions),
+	.types			= probes,
 };
 
 static struct resource sharpsl_nand_resources[] = {
-- 
2.7.4

Re: [PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Lee Jones]

Please use the $SUBJECT line expected by the subsystem.

`git log --oneline -- $SUBSYSTEM` can help with this.

You also need a commit log.

> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> ---
>  include/linux/mfd/tmio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index a1520d8..23bb069 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -139,6 +139,7 @@ struct tmio_nand_data {
>  	struct nand_bbt_descr	*badblock_pattern;
>  	struct mtd_partition	*partition;
>  	unsigned int		num_partitions;
> +	const char *const	*types;	/* names of parsers to use if any */

I'm okay with this if it's suits the MTD folk.

>  };
>  
>  #define FBIO_TMIO_ACC_WRITE	0x7C639300

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Lee Jones]

Please use the $SUBJECT line expected by the subsystem.

`git log --oneline -- $SUBSYSTEM` can help with this.

You also need a commit log.

> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> ---
>  include/linux/mfd/tmio.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> index a1520d8..23bb069 100644
> --- a/include/linux/mfd/tmio.h
> +++ b/include/linux/mfd/tmio.h
> @@ -139,6 +139,7 @@ struct tmio_nand_data {
>  	struct nand_bbt_descr	*badblock_pattern;
>  	struct mtd_partition	*partition;
>  	unsigned int		num_partitions;
> +	const char *const	*types;	/* names of parsers to use if any */

I'm okay with this if it's suits the MTD folk.

>  };
>  
>  #define FBIO_TMIO_ACC_WRITE	0x7C639300

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Andrea Adami]

On Mon, Jul 3, 2017 at 1:26 PM, Lee Jones <lee.jones@linaro.org> wrote:
> Please use the $SUBJECT line expected by the subsystem.
>
> `git log --oneline -- $SUBSYSTEM` can help with this.
>
> You also need a commit log.
>

Lee,

thanks for spotting it.
I'll fix the subject and add a little text in these patches touching headers.

I am awaiting for a new review of the big piece of the patch, the
ftl/parser, then I'll send a fixed v5.


>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>> ---
>>  include/linux/mfd/tmio.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
>> index a1520d8..23bb069 100644
>> --- a/include/linux/mfd/tmio.h
>> +++ b/include/linux/mfd/tmio.h
>> @@ -139,6 +139,7 @@ struct tmio_nand_data {
>>       struct nand_bbt_descr   *badblock_pattern;
>>       struct mtd_partition    *partition;
>>       unsigned int            num_partitions;
>> +     const char *const       *types; /* names of parsers to use if any */
>
> I'm okay with this if it's suits the MTD folk.
>

Other than kerneldoc comments there are maybe other little
discordances to settle: see above is *partition, num_partitions.
In the other header it is *partitions, nr_partitions ...I'll see if
oneday I can janiitor this.

Regards
Andrea

>>  };
>>
>>  #define FBIO_TMIO_ACC_WRITE  0x7C639300
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Andrea Adami]

On Mon, Jul 3, 2017 at 1:26 PM, Lee Jones <lee.jones@linaro.org> wrote:
> Please use the $SUBJECT line expected by the subsystem.
>
> `git log --oneline -- $SUBSYSTEM` can help with this.
>
> You also need a commit log.
>

Lee,

thanks for spotting it.
I'll fix the subject and add a little text in these patches touching headers.

I am awaiting for a new review of the big piece of the patch, the
ftl/parser, then I'll send a fixed v5.


>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>> ---
>>  include/linux/mfd/tmio.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
>> index a1520d8..23bb069 100644
>> --- a/include/linux/mfd/tmio.h
>> +++ b/include/linux/mfd/tmio.h
>> @@ -139,6 +139,7 @@ struct tmio_nand_data {
>>       struct nand_bbt_descr   *badblock_pattern;
>>       struct mtd_partition    *partition;
>>       unsigned int            num_partitions;
>> +     const char *const       *types; /* names of parsers to use if any */
>
> I'm okay with this if it's suits the MTD folk.
>

Other than kerneldoc comments there are maybe other little
discordances to settle: see above is *partition, num_partitions.
In the other header it is *partitions, nr_partitions ...I'll see if
oneday I can janiitor this.

Regards
Andrea

>>  };
>>
>>  #define FBIO_TMIO_ACC_WRITE  0x7C639300
>
> --
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org ? Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Andrea Adami]

On Mon, Jul 3, 2017 at 2:03 PM, Andrea Adami <andrea.adami@gmail.com> wrote:
> On Mon, Jul 3, 2017 at 1:26 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> Please use the $SUBJECT line expected by the subsystem.
>>
>> `git log --oneline -- $SUBSYSTEM` can help with this.
>>
>> You also need a commit log.
>>
>
> Lee,
>
> thanks for spotting it.
> I'll fix the subject and add a little text in these patches touching headers.
>
> I am awaiting for a new review of the big piece of the patch, the
> ftl/parser, then I'll send a fixed v5.
>
>

Lee,

I have looked at the history of mfd/tmio.h but couldn't find
unambiguous examples.
Do you prefer

Subject: mfd: tmio: tmio_nand: add partition parsers platform data
or
Subject: mfd: tmio: tmio-nand: add partition parsers platform data
or
Subject: mfd: tmio: tmio/nand: add partition parsers platform data

Thanks
Andrea


>>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>>> ---
>>>  include/linux/mfd/tmio.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
>>> index a1520d8..23bb069 100644
>>> --- a/include/linux/mfd/tmio.h
>>> +++ b/include/linux/mfd/tmio.h
>>> @@ -139,6 +139,7 @@ struct tmio_nand_data {
>>>       struct nand_bbt_descr   *badblock_pattern;
>>>       struct mtd_partition    *partition;
>>>       unsigned int            num_partitions;
>>> +     const char *const       *types; /* names of parsers to use if any */
>>
>> I'm okay with this if it's suits the MTD folk.
>>
>
> Other than kerneldoc comments there are maybe other little
> discordances to settle: see above is *partition, num_partitions.
> In the other header it is *partitions, nr_partitions ...I'll see if
> oneday I can janiitor this.
>
> Regards
> Andrea
>
>>>  };
>>>
>>>  #define FBIO_TMIO_ACC_WRITE  0x7C639300
>>
>> --
>> Lee Jones
>> Linaro STMicroelectronics Landing Team Lead
>> Linaro.org │ Open source software for ARM SoCs
>> Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Andrea Adami]

On Mon, Jul 3, 2017 at 2:03 PM, Andrea Adami <andrea.adami@gmail.com> wrote:
> On Mon, Jul 3, 2017 at 1:26 PM, Lee Jones <lee.jones@linaro.org> wrote:
>> Please use the $SUBJECT line expected by the subsystem.
>>
>> `git log --oneline -- $SUBSYSTEM` can help with this.
>>
>> You also need a commit log.
>>
>
> Lee,
>
> thanks for spotting it.
> I'll fix the subject and add a little text in these patches touching headers.
>
> I am awaiting for a new review of the big piece of the patch, the
> ftl/parser, then I'll send a fixed v5.
>
>

Lee,

I have looked at the history of mfd/tmio.h but couldn't find
unambiguous examples.
Do you prefer

Subject: mfd: tmio: tmio_nand: add partition parsers platform data
or
Subject: mfd: tmio: tmio-nand: add partition parsers platform data
or
Subject: mfd: tmio: tmio/nand: add partition parsers platform data

Thanks
Andrea


>>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>>> ---
>>>  include/linux/mfd/tmio.h | 1 +
>>>  1 file changed, 1 insertion(+)
>>>
>>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
>>> index a1520d8..23bb069 100644
>>> --- a/include/linux/mfd/tmio.h
>>> +++ b/include/linux/mfd/tmio.h
>>> @@ -139,6 +139,7 @@ struct tmio_nand_data {
>>>       struct nand_bbt_descr   *badblock_pattern;
>>>       struct mtd_partition    *partition;
>>>       unsigned int            num_partitions;
>>> +     const char *const       *types; /* names of parsers to use if any */
>>
>> I'm okay with this if it's suits the MTD folk.
>>
>
> Other than kerneldoc comments there are maybe other little
> discordances to settle: see above is *partition, num_partitions.
> In the other header it is *partitions, nr_partitions ...I'll see if
> oneday I can janiitor this.
>
> Regards
> Andrea
>
>>>  };
>>>
>>>  #define FBIO_TMIO_ACC_WRITE  0x7C639300
>>
>> --
>> Lee Jones
>> Linaro STMicroelectronics Landing Team Lead
>> Linaro.org ? Open source software for ARM SoCs
>> Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Lee Jones]

On Wed, 12 Jul 2017, Andrea Adami wrote:

> On Mon, Jul 3, 2017 at 2:03 PM, Andrea Adami <andrea.adami@gmail.com> wrote:
> > On Mon, Jul 3, 2017 at 1:26 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> Please use the $SUBJECT line expected by the subsystem.
> >>
> >> `git log --oneline -- $SUBSYSTEM` can help with this.
> >>
> >> You also need a commit log.
> >>
> >
> > Lee,
> >
> > thanks for spotting it.
> > I'll fix the subject and add a little text in these patches touching headers.
> >
> > I am awaiting for a new review of the big piece of the patch, the
> > ftl/parser, then I'll send a fixed v5.
> >
> >
> 
> Lee,
> 
> I have looked at the history of mfd/tmio.h but couldn't find
> unambiguous examples.
> Do you prefer
> 
> Subject: mfd: tmio: tmio_nand: add partition parsers platform data
> or
> Subject: mfd: tmio: tmio-nand: add partition parsers platform data
> or
> Subject: mfd: tmio: tmio/nand: add partition parsers platform data

Better to look at the subsystem as a whole.

  `git log --oneline -- drivers/mfd`

You will see something like:

  mfd: device-driver: Short subject starting with an uppercase char
  
> >>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> >>> ---
> >>>  include/linux/mfd/tmio.h | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> >>> index a1520d8..23bb069 100644
> >>> --- a/include/linux/mfd/tmio.h
> >>> +++ b/include/linux/mfd/tmio.h
> >>> @@ -139,6 +139,7 @@ struct tmio_nand_data {
> >>>       struct nand_bbt_descr   *badblock_pattern;
> >>>       struct mtd_partition    *partition;
> >>>       unsigned int            num_partitions;
> >>> +     const char *const       *types; /* names of parsers to use if any */
> >>
> >> I'm okay with this if it's suits the MTD folk.
> >>
> >
> > Other than kerneldoc comments there are maybe other little
> > discordances to settle: see above is *partition, num_partitions.
> > In the other header it is *partitions, nr_partitions ...I'll see if
> > oneday I can janiitor this.
> >
> > Regards
> > Andrea
> >
> >>>  };
> >>>
> >>>  #define FBIO_TMIO_ACC_WRITE  0x7C639300
> >>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH v4 3/9] mfd: tmio.h: support partition parser types

[Lee Jones]

On Wed, 12 Jul 2017, Andrea Adami wrote:

> On Mon, Jul 3, 2017 at 2:03 PM, Andrea Adami <andrea.adami@gmail.com> wrote:
> > On Mon, Jul 3, 2017 at 1:26 PM, Lee Jones <lee.jones@linaro.org> wrote:
> >> Please use the $SUBJECT line expected by the subsystem.
> >>
> >> `git log --oneline -- $SUBSYSTEM` can help with this.
> >>
> >> You also need a commit log.
> >>
> >
> > Lee,
> >
> > thanks for spotting it.
> > I'll fix the subject and add a little text in these patches touching headers.
> >
> > I am awaiting for a new review of the big piece of the patch, the
> > ftl/parser, then I'll send a fixed v5.
> >
> >
> 
> Lee,
> 
> I have looked at the history of mfd/tmio.h but couldn't find
> unambiguous examples.
> Do you prefer
> 
> Subject: mfd: tmio: tmio_nand: add partition parsers platform data
> or
> Subject: mfd: tmio: tmio-nand: add partition parsers platform data
> or
> Subject: mfd: tmio: tmio/nand: add partition parsers platform data

Better to look at the subsystem as a whole.

  `git log --oneline -- drivers/mfd`

You will see something like:

  mfd: device-driver: Short subject starting with an uppercase char
  
> >>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> >>> ---
> >>>  include/linux/mfd/tmio.h | 1 +
> >>>  1 file changed, 1 insertion(+)
> >>>
> >>> diff --git a/include/linux/mfd/tmio.h b/include/linux/mfd/tmio.h
> >>> index a1520d8..23bb069 100644
> >>> --- a/include/linux/mfd/tmio.h
> >>> +++ b/include/linux/mfd/tmio.h
> >>> @@ -139,6 +139,7 @@ struct tmio_nand_data {
> >>>       struct nand_bbt_descr   *badblock_pattern;
> >>>       struct mtd_partition    *partition;
> >>>       unsigned int            num_partitions;
> >>> +     const char *const       *types; /* names of parsers to use if any */
> >>
> >> I'm okay with this if it's suits the MTD folk.
> >>
> >
> > Other than kerneldoc comments there are maybe other little
> > discordances to settle: see above is *partition, num_partitions.
> > In the other header it is *partitions, nr_partitions ...I'll see if
> > oneday I can janiitor this.
> >
> > Regards
> > Andrea
> >
> >>>  };
> >>>
> >>>  #define FBIO_TMIO_ACC_WRITE  0x7C639300
> >>

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org ? Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v4 1/9] mtd: sharpslpart: add sharpslpart MTD partition parser

[Brian Norris]

Hi Andrea,

I'm sorry this had to wait so long, but then...you didn't actually
address several of my comments from the last time :(

This does look better, but still quite a few smaller notes. Hopefully
next one will be merge-able...

On Wed, Jun 28, 2017 at 10:30:28PM +0200, Andrea Adami wrote:
> 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      |   1 +
>  drivers/mtd/sharpslpart.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 400 insertions(+)
>  create mode 100644 drivers/mtd/sharpslpart.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..b196a69 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 || COMPILE_TEST
> +	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.
> +

This has conflicts now; probably should go in
drivers/mtd/parsers/Kconfig now. We should also probably move the other
parsers there, but that's not your problem.

>  comment "User Modules And Translation Layers"
>  
>  #
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..e5ef07f 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -13,6 +13,7 @@ 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)	+= sharpslpart.o

Also has conflicts now. Might as well move to drivers/mtd/parsers/?

>  
>  # 'Users' - code which presents functionality to userspace.
>  obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
> new file mode 100644
> index 0000000..02b721b
> --- /dev/null
> +++ b/drivers/mtd/sharpslpart.c
> @@ -0,0 +1,391 @@
> +/*
> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL
> + * for logical addressing, as used on the PXA models of the SHARP SL Series.
> + *
> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
> + *
> + * Based on 2.4 sources:
> + *  drivers/mtd/nand/sharp_sl_logical.c
> + *  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.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.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
> +
> +#define BLOCK_IS_RESERVED		0xffff
> +#define BLOCK_UNMASK			0x07fe
> +#define BLOCK_UNMASK_COMPLEMENT		1
> +
> +/* 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 buf_start(x)			le32_to_cpu(buf1[x].start)
> +#define buf_end(x)			le32_to_cpu(buf1[x].end)
> +#define buf_magic(x)			be32_to_cpu(buf1[x].magic)

Sorry, these aren't good macros; they implicitly rely on a 'buf1'
variable, without using it as an argument to the macro. That hurts
readability. If you want this macro, give it 2 args.

> +
> +#define BOOT_MAGIC			0x424f4f54   /* BOOT */
> +#define FSRO_MAGIC			0x4653524f   /* FSRO */
> +#define FSRW_MAGIC			0x46535257   /* FSRW */
> +
> +/* 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 */
> +};
> +
> +struct mtd_logical *sharpsl_mtd_logical;

I asked you not to avoid making this variable static...and I suppose
technically you did this. But global is even worse! And you really don't
need to do this; just make sharpsl_nand_init_logical() return struct
mtd_logical, and then you don't need any global/static reference at all.

> +
> +/*
> + * SHARP SL FTL ancillary functions
> + *
> + */
> +
> +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;
> +}
> +
> +/*
> + * The logical block number assigned to a physical block is stored in the OOB
> + * of the first page, in 3 16-bit copies with the following layout:
> + *
> + * 01234567 89abcdef
> + * -------- --------
> + * ECC BB   xyxyxy
> + *
> + * When reading we check that the first two copies agree.
> + * In case of error, matching is tried using the following pairs.
> + * Reserved values 0xffff mean the block is kept for wear leveling.
> + *
> + * 01234567 89abcdef
> + * -------- --------
> + * ECC BB   xyxy    oob[8]==oob[10] && oob[9]==oob[11]   -> byte0=8   byte1=9
> + * ECC BB     xyxy  oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10  byte1=11
> + * ECC BB   xy  xy  oob[12]==oob[8] && oob[13]==oob[9]   -> byte0=12  byte1=13
> + *
> + */

Thanks for this. Much nicer.

> +
> +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 {
> +		/* wrong oob fingerprint, maybe here by mistake? */
> +		return UINT_MAX;
> +	}
> +
> +	us = oob[good0] | oob[good1] << 8;
> +
> +	/* parity check */
> +	if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)
> +		return (UINT_MAX - 1);
> +
> +	/* reserved */
> +	if (us == BLOCK_IS_RESERVED)
> +		return BLOCK_IS_RESERVED;
> +	else
> +		return (us & BLOCK_UNMASK) >> 1;
> +}
> +
> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)

static?

> +{
> +	struct mtd_logical *logical;
> +	u_int block_num, log_num;
> +	loff_t block_adr;
> +	u_char *oob;
> +	int i;
> +
> +	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;

What's this for? You're re-assigning it immediately following.

> +	logical->log2phy = kmalloc_array(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;
> +
> +		if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
> +			continue;
> +
> +		/* get logical block */
> +		log_num = sharpsl_nand_get_logical_num(oob);
> +
> +		/* FTL is not used? Exit here if the oob fingerprint is wrong */
> +		if (log_num == UINT_MAX) {
> +			pr_info("sharpslpart: Sharp SL FTL not found. Quit parser.\n");
> +			kfree(logical->log2phy);
> +			kfree(logical);
> +			kfree(oob);
> +			return -EINVAL;
> +		}
> +
> +		/* skip out of range and not unique values */
> +		if (log_num < logical->logmax) {
> +			if (logical->log2phy[log_num] == UINT_MAX)
> +				logical->log2phy[log_num] = block_num;
> +		}
> +	}
> +	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(struct mtd_logical *sharpsl_mtd_logical)

static?

And why don't you just name the arg 'logical' and...

> +{
> +	struct mtd_logical *logical = sharpsl_mtd_logical;

...drop this?

> +
> +	sharpsl_mtd_logical = NULL;

If you kill the global, you don't need this.

> +
> +	kfree(logical->log2phy);
> +	logical->log2phy = NULL;

Not necessary.

> +	kfree(logical);
> +	logical = NULL;

Not necessary.

> +}
> +
> +int sharpsl_nand_read_laddr(struct mtd_info *mtd,

static?

> +			    loff_t from,
> +			    size_t len,
> +			    u_char *buf)
> +{
> +	struct mtd_logical *logical;

Make 'logical' a parameter, so you don't need the global.

> +	u_int log_num, log_new;
> +	u_int block_num;
> +	loff_t block_adr;
> +	loff_t block_ofs;
> +	size_t retlen;
> +	int err;
> +
> +	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;
> +
> +	err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
> +	/* Ignore corrected ECC errors */
> +	if (mtd_is_bitflip(err))
> +		err = 0;
> +	if (!err && retlen != len)
> +		err = -EIO;
> +	if (err)
> +		pr_err("sharpslpart: error, read failed at %#llx\n",
> +		       block_adr + block_ofs);
> +
> +	return err;
> +}
> +
> +/*
> + * MTD Partition Parser
> + *
> + */
> +
> +struct sharpsl_nand_partitioninfo {
> +	u32 start;
> +	u32 end;

Should the above 2 be '__le32'?

> +	u32 magic;

It's a little odd that the same on-flash structure would have both BE
and LE elements, but I guess judging by usage...this should be '__be32'.

> +	u32 reserved;
> +};
> +
> +/*
> + * 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  ................
> + *
> + */
> +
> +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;
> +	int err;
> +
> +	/* init logical mgmt (FTL) */
> +	if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
> +		return -EINVAL;

Please propagate the error...

> +
> +	/* read the two partition tables */
> +	err = sharpsl_nand_read_laddr(master,
> +				      PARAM_BLOCK_PARTITIONINFO1,
> +				      sizeof(buf1), (u_char *)&buf1);
> +	if (!err) {
> +		sharpsl_nand_read_laddr(master,
> +					PARAM_BLOCK_PARTITIONINFO2,
> +					sizeof(buf2), (u_char *)&buf2);

You don't really need this block under the 'if'. You can just invert
this to 'if (err)', since the second block will return early.

Brian

> +	} else {
> +		sharpsl_nand_cleanup_logical(sharpsl_mtd_logical);
> +		return err;
> +	}
> +
> +	/* cleanup logical mgmt (FTL) */
> +	sharpsl_nand_cleanup_logical(sharpsl_mtd_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 (buf_magic(0) != BOOT_MAGIC ||
> +	    buf_magic(1) != FSRO_MAGIC ||
> +	    buf_magic(2) != FSRW_MAGIC) {
> +		pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* fixup for hardcoded value 64 MiB (for older models) */
> +	buf1[2].end = cpu_to_le32(master->size);
> +
> +	/* extra sanity check */
> +	if (buf_end(0) <= buf_start(0) ||
> +	    buf_start(1) < buf_end(0) ||
> +	    buf_end(1) <= buf_start(1) ||
> +	    buf_start(2) < buf_end(1) ||
> +	    buf_end(2) <= buf_start(2)) {
> +		pr_err("sharpslpart: partition sizes 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 = buf_start(0);
> +	sharpsl_nand_parts[0].size = buf_end(0) - buf_start(0);
> +
> +	sharpsl_nand_parts[1].name = "root";
> +	sharpsl_nand_parts[1].offset = buf_start(1);
> +	sharpsl_nand_parts[1].size = buf_end(1) - buf_start(1);
> +
> +	sharpsl_nand_parts[2].name = "home";
> +	sharpsl_nand_parts[2].offset = buf_start(2);
> +	sharpsl_nand_parts[2].size = buf_end(2) - buf_start(2);
> +
> +	*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 v4 1/9] mtd: sharpslpart: add sharpslpart MTD partition parser

[Brian Norris]

Hi Andrea,

I'm sorry this had to wait so long, but then...you didn't actually
address several of my comments from the last time :(

This does look better, but still quite a few smaller notes. Hopefully
next one will be merge-able...

On Wed, Jun 28, 2017 at 10:30:28PM +0200, Andrea Adami wrote:
> 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      |   1 +
>  drivers/mtd/sharpslpart.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 400 insertions(+)
>  create mode 100644 drivers/mtd/sharpslpart.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..b196a69 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 || COMPILE_TEST
> +	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.
> +

This has conflicts now; probably should go in
drivers/mtd/parsers/Kconfig now. We should also probably move the other
parsers there, but that's not your problem.

>  comment "User Modules And Translation Layers"
>  
>  #
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..e5ef07f 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -13,6 +13,7 @@ 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)	+= sharpslpart.o

Also has conflicts now. Might as well move to drivers/mtd/parsers/?

>  
>  # 'Users' - code which presents functionality to userspace.
>  obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
> new file mode 100644
> index 0000000..02b721b
> --- /dev/null
> +++ b/drivers/mtd/sharpslpart.c
> @@ -0,0 +1,391 @@
> +/*
> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL
> + * for logical addressing, as used on the PXA models of the SHARP SL Series.
> + *
> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
> + *
> + * Based on 2.4 sources:
> + *  drivers/mtd/nand/sharp_sl_logical.c
> + *  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.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/types.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.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
> +
> +#define BLOCK_IS_RESERVED		0xffff
> +#define BLOCK_UNMASK			0x07fe
> +#define BLOCK_UNMASK_COMPLEMENT		1
> +
> +/* 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 buf_start(x)			le32_to_cpu(buf1[x].start)
> +#define buf_end(x)			le32_to_cpu(buf1[x].end)
> +#define buf_magic(x)			be32_to_cpu(buf1[x].magic)

Sorry, these aren't good macros; they implicitly rely on a 'buf1'
variable, without using it as an argument to the macro. That hurts
readability. If you want this macro, give it 2 args.

> +
> +#define BOOT_MAGIC			0x424f4f54   /* BOOT */
> +#define FSRO_MAGIC			0x4653524f   /* FSRO */
> +#define FSRW_MAGIC			0x46535257   /* FSRW */
> +
> +/* 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 */
> +};
> +
> +struct mtd_logical *sharpsl_mtd_logical;

I asked you not to avoid making this variable static...and I suppose
technically you did this. But global is even worse! And you really don't
need to do this; just make sharpsl_nand_init_logical() return struct
mtd_logical, and then you don't need any global/static reference at all.

> +
> +/*
> + * SHARP SL FTL ancillary functions
> + *
> + */
> +
> +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;
> +}
> +
> +/*
> + * The logical block number assigned to a physical block is stored in the OOB
> + * of the first page, in 3 16-bit copies with the following layout:
> + *
> + * 01234567 89abcdef
> + * -------- --------
> + * ECC BB   xyxyxy
> + *
> + * When reading we check that the first two copies agree.
> + * In case of error, matching is tried using the following pairs.
> + * Reserved values 0xffff mean the block is kept for wear leveling.
> + *
> + * 01234567 89abcdef
> + * -------- --------
> + * ECC BB   xyxy    oob[8]==oob[10] && oob[9]==oob[11]   -> byte0=8   byte1=9
> + * ECC BB     xyxy  oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10  byte1=11
> + * ECC BB   xy  xy  oob[12]==oob[8] && oob[13]==oob[9]   -> byte0=12  byte1=13
> + *
> + */

Thanks for this. Much nicer.

> +
> +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 {
> +		/* wrong oob fingerprint, maybe here by mistake? */
> +		return UINT_MAX;
> +	}
> +
> +	us = oob[good0] | oob[good1] << 8;
> +
> +	/* parity check */
> +	if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)
> +		return (UINT_MAX - 1);
> +
> +	/* reserved */
> +	if (us == BLOCK_IS_RESERVED)
> +		return BLOCK_IS_RESERVED;
> +	else
> +		return (us & BLOCK_UNMASK) >> 1;
> +}
> +
> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)

static?

> +{
> +	struct mtd_logical *logical;
> +	u_int block_num, log_num;
> +	loff_t block_adr;
> +	u_char *oob;
> +	int i;
> +
> +	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;

What's this for? You're re-assigning it immediately following.

> +	logical->log2phy = kmalloc_array(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;
> +
> +		if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
> +			continue;
> +
> +		/* get logical block */
> +		log_num = sharpsl_nand_get_logical_num(oob);
> +
> +		/* FTL is not used? Exit here if the oob fingerprint is wrong */
> +		if (log_num == UINT_MAX) {
> +			pr_info("sharpslpart: Sharp SL FTL not found. Quit parser.\n");
> +			kfree(logical->log2phy);
> +			kfree(logical);
> +			kfree(oob);
> +			return -EINVAL;
> +		}
> +
> +		/* skip out of range and not unique values */
> +		if (log_num < logical->logmax) {
> +			if (logical->log2phy[log_num] == UINT_MAX)
> +				logical->log2phy[log_num] = block_num;
> +		}
> +	}
> +	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(struct mtd_logical *sharpsl_mtd_logical)

static?

And why don't you just name the arg 'logical' and...

> +{
> +	struct mtd_logical *logical = sharpsl_mtd_logical;

...drop this?

> +
> +	sharpsl_mtd_logical = NULL;

If you kill the global, you don't need this.

> +
> +	kfree(logical->log2phy);
> +	logical->log2phy = NULL;

Not necessary.

> +	kfree(logical);
> +	logical = NULL;

Not necessary.

> +}
> +
> +int sharpsl_nand_read_laddr(struct mtd_info *mtd,

static?

> +			    loff_t from,
> +			    size_t len,
> +			    u_char *buf)
> +{
> +	struct mtd_logical *logical;

Make 'logical' a parameter, so you don't need the global.

> +	u_int log_num, log_new;
> +	u_int block_num;
> +	loff_t block_adr;
> +	loff_t block_ofs;
> +	size_t retlen;
> +	int err;
> +
> +	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;
> +
> +	err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
> +	/* Ignore corrected ECC errors */
> +	if (mtd_is_bitflip(err))
> +		err = 0;
> +	if (!err && retlen != len)
> +		err = -EIO;
> +	if (err)
> +		pr_err("sharpslpart: error, read failed at %#llx\n",
> +		       block_adr + block_ofs);
> +
> +	return err;
> +}
> +
> +/*
> + * MTD Partition Parser
> + *
> + */
> +
> +struct sharpsl_nand_partitioninfo {
> +	u32 start;
> +	u32 end;

Should the above 2 be '__le32'?

> +	u32 magic;

It's a little odd that the same on-flash structure would have both BE
and LE elements, but I guess judging by usage...this should be '__be32'.

> +	u32 reserved;
> +};
> +
> +/*
> + * 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  ................
> + *
> + */
> +
> +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;
> +	int err;
> +
> +	/* init logical mgmt (FTL) */
> +	if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
> +		return -EINVAL;

Please propagate the error...

> +
> +	/* read the two partition tables */
> +	err = sharpsl_nand_read_laddr(master,
> +				      PARAM_BLOCK_PARTITIONINFO1,
> +				      sizeof(buf1), (u_char *)&buf1);
> +	if (!err) {
> +		sharpsl_nand_read_laddr(master,
> +					PARAM_BLOCK_PARTITIONINFO2,
> +					sizeof(buf2), (u_char *)&buf2);

You don't really need this block under the 'if'. You can just invert
this to 'if (err)', since the second block will return early.

Brian

> +	} else {
> +		sharpsl_nand_cleanup_logical(sharpsl_mtd_logical);
> +		return err;
> +	}
> +
> +	/* cleanup logical mgmt (FTL) */
> +	sharpsl_nand_cleanup_logical(sharpsl_mtd_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 (buf_magic(0) != BOOT_MAGIC ||
> +	    buf_magic(1) != FSRO_MAGIC ||
> +	    buf_magic(2) != FSRW_MAGIC) {
> +		pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* fixup for hardcoded value 64 MiB (for older models) */
> +	buf1[2].end = cpu_to_le32(master->size);
> +
> +	/* extra sanity check */
> +	if (buf_end(0) <= buf_start(0) ||
> +	    buf_start(1) < buf_end(0) ||
> +	    buf_end(1) <= buf_start(1) ||
> +	    buf_start(2) < buf_end(1) ||
> +	    buf_end(2) <= buf_start(2)) {
> +		pr_err("sharpslpart: partition sizes 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 = buf_start(0);
> +	sharpsl_nand_parts[0].size = buf_end(0) - buf_start(0);
> +
> +	sharpsl_nand_parts[1].name = "root";
> +	sharpsl_nand_parts[1].offset = buf_start(1);
> +	sharpsl_nand_parts[1].size = buf_end(1) - buf_start(1);
> +
> +	sharpsl_nand_parts[2].name = "home";
> +	sharpsl_nand_parts[2].offset = buf_start(2);
> +	sharpsl_nand_parts[2].size = buf_end(2) - buf_start(2);
> +
> +	*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
> 

Re: [PATCH v4 2/9] mtd: nand: sharpsl.h: support partition parser types

[Boris Brezillon]

Le Wed, 28 Jun 2017 22:30:29 +0200,
Andrea Adami <andrea.adami@gmail.com> a écrit :

Please add a commit message explaining why you're doing that.

> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> ---
>  include/linux/mtd/sharpsl.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mtd/sharpsl.h b/include/linux/mtd/sharpsl.h
> index 65e91d0..c0e0be2 100644
> --- a/include/linux/mtd/sharpsl.h
> +++ b/include/linux/mtd/sharpsl.h
> @@ -17,4 +17,5 @@ struct sharpsl_nand_platform_data {
>  	const struct mtd_ooblayout_ops *ecc_layout;
>  	struct mtd_partition	*partitions;
>  	unsigned int		nr_partitions;
> +	const char *const	*types;	/* names of parsers to use if any */

types is not really descriptive here. How about 'part_parsers' or
something clearly reflecting the purpose of this field.

BTW, try to avoid putting comments on the same line.

>  };

[PATCH v4 2/9] mtd: nand: sharpsl.h: support partition parser types

[Boris Brezillon]

Le Wed, 28 Jun 2017 22:30:29 +0200,
Andrea Adami <andrea.adami@gmail.com> a ?crit :

Please add a commit message explaining why you're doing that.

> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
> ---
>  include/linux/mtd/sharpsl.h | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/include/linux/mtd/sharpsl.h b/include/linux/mtd/sharpsl.h
> index 65e91d0..c0e0be2 100644
> --- a/include/linux/mtd/sharpsl.h
> +++ b/include/linux/mtd/sharpsl.h
> @@ -17,4 +17,5 @@ struct sharpsl_nand_platform_data {
>  	const struct mtd_ooblayout_ops *ecc_layout;
>  	struct mtd_partition	*partitions;
>  	unsigned int		nr_partitions;
> +	const char *const	*types;	/* names of parsers to use if any */

types is not really descriptive here. How about 'part_parsers' or
something clearly reflecting the purpose of this field.

BTW, try to avoid putting comments on the same line.

>  };

Re: [PATCH v4 1/9] mtd: sharpslpart: add sharpslpart MTD partition parser

[Andrea Adami]

On Sat, Aug 12, 2017 at 11:35 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi Andrea,
>
> I'm sorry this had to wait so long, but then...you didn't actually
> address several of my comments from the last time :(
>
> This does look better, but still quite a few smaller notes. Hopefully
> next one will be merge-able...
>

Brian,

thank you again for the time spent reviewing this patch and for the guidance.

I did not dare to send all the changes together in the v4 so I kept
the management of the structure as last step (remember this code is
coming from 2.4 sources and it was supposed to keep the logical to
physical table in memory).

So I will send v5 very soon without the global structure and with
these last refinements you're suggesting.

I was really awaiting your comments about the many changes done and my
implementation (see macros...).

I'll comment briefly but substantially I agree with your observations.

> On Wed, Jun 28, 2017 at 10:30:28PM +0200, Andrea Adami wrote:
>> 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      |   1 +
>>  drivers/mtd/sharpslpart.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 400 insertions(+)
>>  create mode 100644 drivers/mtd/sharpslpart.c
>>
>> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
>> index e83a279..b196a69 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 || COMPILE_TEST
>> +     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.
>> +
>
> This has conflicts now; probably should go in
> drivers/mtd/parsers/Kconfig now. We should also probably move the other
> parsers there, but that's not your problem.
>
Ok, I'll move the parser there.

>>  comment "User Modules And Translation Layers"
>>
>>  #
>> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
>> index 99bb9a1..e5ef07f 100644
>> --- a/drivers/mtd/Makefile
>> +++ b/drivers/mtd/Makefile
>> @@ -13,6 +13,7 @@ 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)      += sharpslpart.o
>
> Also has conflicts now. Might as well move to drivers/mtd/parsers/?

Sure

>
>>
>>  # 'Users' - code which presents functionality to userspace.
>>  obj-$(CONFIG_MTD_BLKDEVS)    += mtd_blkdevs.o
>> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
>> new file mode 100644
>> index 0000000..02b721b
>> --- /dev/null
>> +++ b/drivers/mtd/sharpslpart.c
>> @@ -0,0 +1,391 @@
>> +/*
>> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL
>> + * for logical addressing, as used on the PXA models of the SHARP SL Series.
>> + *
>> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
>> + *
>> + * Based on 2.4 sources:
>> + *  drivers/mtd/nand/sharp_sl_logical.c
>> + *  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.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/types.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/partitions.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
>> +
>> +#define BLOCK_IS_RESERVED            0xffff
>> +#define BLOCK_UNMASK                 0x07fe
>> +#define BLOCK_UNMASK_COMPLEMENT              1
>> +
>> +/* 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 buf_start(x)                 le32_to_cpu(buf1[x].start)
>> +#define buf_end(x)                   le32_to_cpu(buf1[x].end)
>> +#define buf_magic(x)                 be32_to_cpu(buf1[x].magic)
>
> Sorry, these aren't good macros; they implicitly rely on a 'buf1'
> variable, without using it as an argument to the macro. That hurts
> readability. If you want this macro, give it 2 args.

Ok, I thought that was better than a bunch of le32_to_cpu() but
finally is just a preprocessor thing. Macro removed.

>
>> +
>> +#define BOOT_MAGIC                   0x424f4f54   /* BOOT */
>> +#define FSRO_MAGIC                   0x4653524f   /* FSRO */
>> +#define FSRW_MAGIC                   0x46535257   /* FSRW */
>> +
>> +/* 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 */
>> +};
>> +
>> +struct mtd_logical *sharpsl_mtd_logical;
>
> I asked you not to avoid making this variable static...and I suppose
> technically you did this. But global is even worse! And you really don't
> need to do this; just make sharpsl_nand_init_logical() return struct
> mtd_logical, and then you don't need any global/static reference at all.
>

Done, please see v5.


>> +
>> +/*
>> + * SHARP SL FTL ancillary functions
>> + *
>> + */
>> +
>> +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;
>> +}
>> +
>> +/*
>> + * The logical block number assigned to a physical block is stored in the OOB
>> + * of the first page, in 3 16-bit copies with the following layout:
>> + *
>> + * 01234567 89abcdef
>> + * -------- --------
>> + * ECC BB   xyxyxy
>> + *
>> + * When reading we check that the first two copies agree.
>> + * In case of error, matching is tried using the following pairs.
>> + * Reserved values 0xffff mean the block is kept for wear leveling.
>> + *
>> + * 01234567 89abcdef
>> + * -------- --------
>> + * ECC BB   xyxy    oob[8]==oob[10] && oob[9]==oob[11]   -> byte0=8   byte1=9
>> + * ECC BB     xyxy  oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10  byte1=11
>> + * ECC BB   xy  xy  oob[12]==oob[8] && oob[13]==oob[9]   -> byte0=12  byte1=13
>> + *
>> + */
>
> Thanks for this. Much nicer.
>
Ok, I should maybe have done it little endian ;)

>> +
>> +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 {
>> +             /* wrong oob fingerprint, maybe here by mistake? */
>> +             return UINT_MAX;
>> +     }
>> +
>> +     us = oob[good0] | oob[good1] << 8;
>> +
>> +     /* parity check */
>> +     if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)
>> +             return (UINT_MAX - 1);
>> +
>> +     /* reserved */
>> +     if (us == BLOCK_IS_RESERVED)
>> +             return BLOCK_IS_RESERVED;
>> +     else
>> +             return (us & BLOCK_UNMASK) >> 1;
>> +}
>> +
>> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)
>
> static?
>
Yes, the parser is a single file now

>> +{
>> +     struct mtd_logical *logical;
>> +     u_int block_num, log_num;
>> +     loff_t block_adr;
>> +     u_char *oob;
>> +     int i;
>> +
>> +     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;
>
> What's this for? You're re-assigning it immediately following.
>
Remnant from 2.4? Unnecessary, removed.

>> +     logical->log2phy = kmalloc_array(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;
>> +
>> +             if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
>> +                     continue;
>> +
>> +             /* get logical block */
>> +             log_num = sharpsl_nand_get_logical_num(oob);
>> +
>> +             /* FTL is not used? Exit here if the oob fingerprint is wrong */
>> +             if (log_num == UINT_MAX) {
>> +                     pr_info("sharpslpart: Sharp SL FTL not found. Quit parser.\n");
>> +                     kfree(logical->log2phy);
>> +                     kfree(logical);
>> +                     kfree(oob);
>> +                     return -EINVAL;
>> +             }
>> +
>> +             /* skip out of range and not unique values */
>> +             if (log_num < logical->logmax) {
>> +                     if (logical->log2phy[log_num] == UINT_MAX)
>> +                             logical->log2phy[log_num] = block_num;
>> +             }
>> +     }
>> +     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(struct mtd_logical *sharpsl_mtd_logical)
>
> static?
>
Yes, the parser is a single file now

> And why don't you just name the arg 'logical' and...
>

Yes, I dropped the cleanup function.

>> +{
>> +     struct mtd_logical *logical = sharpsl_mtd_logical;
>
> ...drop this?
>
>> +
>> +     sharpsl_mtd_logical = NULL;
>
> If you kill the global, you don't need this.
>
>> +
>> +     kfree(logical->log2phy);
>> +     logical->log2phy = NULL;
>
> Not necessary.
>
>> +     kfree(logical);
>> +     logical = NULL;
>
> Not necessary.
>
>> +}
>> +
>> +int sharpsl_nand_read_laddr(struct mtd_info *mtd,
>
> static?
>
Yes, the parser is a single file now

>> +                         loff_t from,
>> +                         size_t len,
>> +                         u_char *buf)
>> +{
>> +     struct mtd_logical *logical;
>
> Make 'logical' a parameter, so you don't need the global.
>
Done in v5

>> +     u_int log_num, log_new;
>> +     u_int block_num;
>> +     loff_t block_adr;
>> +     loff_t block_ofs;
>> +     size_t retlen;
>> +     int err;
>> +
>> +     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;
>> +
>> +     err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
>> +     /* Ignore corrected ECC errors */
>> +     if (mtd_is_bitflip(err))
>> +             err = 0;
>> +     if (!err && retlen != len)
>> +             err = -EIO;
>> +     if (err)
>> +             pr_err("sharpslpart: error, read failed at %#llx\n",
>> +                    block_adr + block_ofs);
>> +
>> +     return err;
>> +}
>> +
>> +/*
>> + * MTD Partition Parser
>> + *
>> + */
>> +
>> +struct sharpsl_nand_partitioninfo {
>> +     u32 start;
>> +     u32 end;
>
> Should the above 2 be '__le32'?
>
>> +     u32 magic;
>
> It's a little odd that the same on-flash structure would have both BE
> and LE elements, but I guess judging by usage...this should be '__be32'.
>

Ok, modified the struct.

>> +     u32 reserved;
>> +};
>> +
>> +/*
>> + * 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  ................
>> + *
>> + */
>> +
>> +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;
>> +     int err;
>> +
>> +     /* init logical mgmt (FTL) */
>> +     if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
>> +             return -EINVAL;
>
> Please propagate the error...
>
Ok, done.

>> +
>> +     /* read the two partition tables */
>> +     err = sharpsl_nand_read_laddr(master,
>> +                                   PARAM_BLOCK_PARTITIONINFO1,
>> +                                   sizeof(buf1), (u_char *)&buf1);
>> +     if (!err) {
>> +             sharpsl_nand_read_laddr(master,
>> +                                     PARAM_BLOCK_PARTITIONINFO2,
>> +                                     sizeof(buf2), (u_char *)&buf2);
>
> You don't really need this block under the 'if'. You can just invert
> this to 'if (err)', since the second block will return early.
>

I have removed it and added and OR for the second read, then check is
if (err) as you said.

Then I have removed the cleanup and kfree'd before returning. Please check v5.
Thanks in advance.

> Brian
>
>> +     } else {
>> +             sharpsl_nand_cleanup_logical(sharpsl_mtd_logical);
>> +             return err;
>> +     }
>> +
>> +     /* cleanup logical mgmt (FTL) */
>> +     sharpsl_nand_cleanup_logical(sharpsl_mtd_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 (buf_magic(0) != BOOT_MAGIC ||
>> +         buf_magic(1) != FSRO_MAGIC ||
>> +         buf_magic(2) != FSRW_MAGIC) {
>> +             pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* fixup for hardcoded value 64 MiB (for older models) */
>> +     buf1[2].end = cpu_to_le32(master->size);
>> +
>> +     /* extra sanity check */
>> +     if (buf_end(0) <= buf_start(0) ||
>> +         buf_start(1) < buf_end(0) ||
>> +         buf_end(1) <= buf_start(1) ||
>> +         buf_start(2) < buf_end(1) ||
>> +         buf_end(2) <= buf_start(2)) {
>> +             pr_err("sharpslpart: partition sizes 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 = buf_start(0);
>> +     sharpsl_nand_parts[0].size = buf_end(0) - buf_start(0);
>> +
>> +     sharpsl_nand_parts[1].name = "root";
>> +     sharpsl_nand_parts[1].offset = buf_start(1);
>> +     sharpsl_nand_parts[1].size = buf_end(1) - buf_start(1);
>> +
>> +     sharpsl_nand_parts[2].name = "home";
>> +     sharpsl_nand_parts[2].offset = buf_start(2);
>> +     sharpsl_nand_parts[2].size = buf_end(2) - buf_start(2);
>> +
>> +     *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 v4 1/9] mtd: sharpslpart: add sharpslpart MTD partition parser

[Andrea Adami]

On Sat, Aug 12, 2017 at 11:35 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> Hi Andrea,
>
> I'm sorry this had to wait so long, but then...you didn't actually
> address several of my comments from the last time :(
>
> This does look better, but still quite a few smaller notes. Hopefully
> next one will be merge-able...
>

Brian,

thank you again for the time spent reviewing this patch and for the guidance.

I did not dare to send all the changes together in the v4 so I kept
the management of the structure as last step (remember this code is
coming from 2.4 sources and it was supposed to keep the logical to
physical table in memory).

So I will send v5 very soon without the global structure and with
these last refinements you're suggesting.

I was really awaiting your comments about the many changes done and my
implementation (see macros...).

I'll comment briefly but substantially I agree with your observations.

> On Wed, Jun 28, 2017 at 10:30:28PM +0200, Andrea Adami wrote:
>> 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      |   1 +
>>  drivers/mtd/sharpslpart.c | 391 ++++++++++++++++++++++++++++++++++++++++++++++
>>  3 files changed, 400 insertions(+)
>>  create mode 100644 drivers/mtd/sharpslpart.c
>>
>> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
>> index e83a279..b196a69 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 || COMPILE_TEST
>> +     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.
>> +
>
> This has conflicts now; probably should go in
> drivers/mtd/parsers/Kconfig now. We should also probably move the other
> parsers there, but that's not your problem.
>
Ok, I'll move the parser there.

>>  comment "User Modules And Translation Layers"
>>
>>  #
>> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
>> index 99bb9a1..e5ef07f 100644
>> --- a/drivers/mtd/Makefile
>> +++ b/drivers/mtd/Makefile
>> @@ -13,6 +13,7 @@ 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)      += sharpslpart.o
>
> Also has conflicts now. Might as well move to drivers/mtd/parsers/?

Sure

>
>>
>>  # 'Users' - code which presents functionality to userspace.
>>  obj-$(CONFIG_MTD_BLKDEVS)    += mtd_blkdevs.o
>> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
>> new file mode 100644
>> index 0000000..02b721b
>> --- /dev/null
>> +++ b/drivers/mtd/sharpslpart.c
>> @@ -0,0 +1,391 @@
>> +/*
>> + * sharpslpart.c - MTD partition parser for NAND flash using the SHARP FTL
>> + * for logical addressing, as used on the PXA models of the SHARP SL Series.
>> + *
>> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
>> + *
>> + * Based on 2.4 sources:
>> + *  drivers/mtd/nand/sharp_sl_logical.c
>> + *  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.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/types.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/partitions.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
>> +
>> +#define BLOCK_IS_RESERVED            0xffff
>> +#define BLOCK_UNMASK                 0x07fe
>> +#define BLOCK_UNMASK_COMPLEMENT              1
>> +
>> +/* 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 buf_start(x)                 le32_to_cpu(buf1[x].start)
>> +#define buf_end(x)                   le32_to_cpu(buf1[x].end)
>> +#define buf_magic(x)                 be32_to_cpu(buf1[x].magic)
>
> Sorry, these aren't good macros; they implicitly rely on a 'buf1'
> variable, without using it as an argument to the macro. That hurts
> readability. If you want this macro, give it 2 args.

Ok, I thought that was better than a bunch of le32_to_cpu() but
finally is just a preprocessor thing. Macro removed.

>
>> +
>> +#define BOOT_MAGIC                   0x424f4f54   /* BOOT */
>> +#define FSRO_MAGIC                   0x4653524f   /* FSRO */
>> +#define FSRW_MAGIC                   0x46535257   /* FSRW */
>> +
>> +/* 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 */
>> +};
>> +
>> +struct mtd_logical *sharpsl_mtd_logical;
>
> I asked you not to avoid making this variable static...and I suppose
> technically you did this. But global is even worse! And you really don't
> need to do this; just make sharpsl_nand_init_logical() return struct
> mtd_logical, and then you don't need any global/static reference at all.
>

Done, please see v5.


>> +
>> +/*
>> + * SHARP SL FTL ancillary functions
>> + *
>> + */
>> +
>> +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;
>> +}
>> +
>> +/*
>> + * The logical block number assigned to a physical block is stored in the OOB
>> + * of the first page, in 3 16-bit copies with the following layout:
>> + *
>> + * 01234567 89abcdef
>> + * -------- --------
>> + * ECC BB   xyxyxy
>> + *
>> + * When reading we check that the first two copies agree.
>> + * In case of error, matching is tried using the following pairs.
>> + * Reserved values 0xffff mean the block is kept for wear leveling.
>> + *
>> + * 01234567 89abcdef
>> + * -------- --------
>> + * ECC BB   xyxy    oob[8]==oob[10] && oob[9]==oob[11]   -> byte0=8   byte1=9
>> + * ECC BB     xyxy  oob[10]==oob[12] && oob[11]==oob[13] -> byte0=10  byte1=11
>> + * ECC BB   xy  xy  oob[12]==oob[8] && oob[13]==oob[9]   -> byte0=12  byte1=13
>> + *
>> + */
>
> Thanks for this. Much nicer.
>
Ok, I should maybe have done it little endian ;)

>> +
>> +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 {
>> +             /* wrong oob fingerprint, maybe here by mistake? */
>> +             return UINT_MAX;
>> +     }
>> +
>> +     us = oob[good0] | oob[good1] << 8;
>> +
>> +     /* parity check */
>> +     if (hweight16(us) & BLOCK_UNMASK_COMPLEMENT)
>> +             return (UINT_MAX - 1);
>> +
>> +     /* reserved */
>> +     if (us == BLOCK_IS_RESERVED)
>> +             return BLOCK_IS_RESERVED;
>> +     else
>> +             return (us & BLOCK_UNMASK) >> 1;
>> +}
>> +
>> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u32 partition_size)
>
> static?
>
Yes, the parser is a single file now

>> +{
>> +     struct mtd_logical *logical;
>> +     u_int block_num, log_num;
>> +     loff_t block_adr;
>> +     u_char *oob;
>> +     int i;
>> +
>> +     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;
>
> What's this for? You're re-assigning it immediately following.
>
Remnant from 2.4? Unnecessary, removed.

>> +     logical->log2phy = kmalloc_array(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;
>> +
>> +             if (sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize, oob))
>> +                     continue;
>> +
>> +             /* get logical block */
>> +             log_num = sharpsl_nand_get_logical_num(oob);
>> +
>> +             /* FTL is not used? Exit here if the oob fingerprint is wrong */
>> +             if (log_num == UINT_MAX) {
>> +                     pr_info("sharpslpart: Sharp SL FTL not found. Quit parser.\n");
>> +                     kfree(logical->log2phy);
>> +                     kfree(logical);
>> +                     kfree(oob);
>> +                     return -EINVAL;
>> +             }
>> +
>> +             /* skip out of range and not unique values */
>> +             if (log_num < logical->logmax) {
>> +                     if (logical->log2phy[log_num] == UINT_MAX)
>> +                             logical->log2phy[log_num] = block_num;
>> +             }
>> +     }
>> +     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(struct mtd_logical *sharpsl_mtd_logical)
>
> static?
>
Yes, the parser is a single file now

> And why don't you just name the arg 'logical' and...
>

Yes, I dropped the cleanup function.

>> +{
>> +     struct mtd_logical *logical = sharpsl_mtd_logical;
>
> ...drop this?
>
>> +
>> +     sharpsl_mtd_logical = NULL;
>
> If you kill the global, you don't need this.
>
>> +
>> +     kfree(logical->log2phy);
>> +     logical->log2phy = NULL;
>
> Not necessary.
>
>> +     kfree(logical);
>> +     logical = NULL;
>
> Not necessary.
>
>> +}
>> +
>> +int sharpsl_nand_read_laddr(struct mtd_info *mtd,
>
> static?
>
Yes, the parser is a single file now

>> +                         loff_t from,
>> +                         size_t len,
>> +                         u_char *buf)
>> +{
>> +     struct mtd_logical *logical;
>
> Make 'logical' a parameter, so you don't need the global.
>
Done in v5

>> +     u_int log_num, log_new;
>> +     u_int block_num;
>> +     loff_t block_adr;
>> +     loff_t block_ofs;
>> +     size_t retlen;
>> +     int err;
>> +
>> +     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;
>> +
>> +     err = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
>> +     /* Ignore corrected ECC errors */
>> +     if (mtd_is_bitflip(err))
>> +             err = 0;
>> +     if (!err && retlen != len)
>> +             err = -EIO;
>> +     if (err)
>> +             pr_err("sharpslpart: error, read failed at %#llx\n",
>> +                    block_adr + block_ofs);
>> +
>> +     return err;
>> +}
>> +
>> +/*
>> + * MTD Partition Parser
>> + *
>> + */
>> +
>> +struct sharpsl_nand_partitioninfo {
>> +     u32 start;
>> +     u32 end;
>
> Should the above 2 be '__le32'?
>
>> +     u32 magic;
>
> It's a little odd that the same on-flash structure would have both BE
> and LE elements, but I guess judging by usage...this should be '__be32'.
>

Ok, modified the struct.

>> +     u32 reserved;
>> +};
>> +
>> +/*
>> + * 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  ................
>> + *
>> + */
>> +
>> +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;
>> +     int err;
>> +
>> +     /* init logical mgmt (FTL) */
>> +     if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
>> +             return -EINVAL;
>
> Please propagate the error...
>
Ok, done.

>> +
>> +     /* read the two partition tables */
>> +     err = sharpsl_nand_read_laddr(master,
>> +                                   PARAM_BLOCK_PARTITIONINFO1,
>> +                                   sizeof(buf1), (u_char *)&buf1);
>> +     if (!err) {
>> +             sharpsl_nand_read_laddr(master,
>> +                                     PARAM_BLOCK_PARTITIONINFO2,
>> +                                     sizeof(buf2), (u_char *)&buf2);
>
> You don't really need this block under the 'if'. You can just invert
> this to 'if (err)', since the second block will return early.
>

I have removed it and added and OR for the second read, then check is
if (err) as you said.

Then I have removed the cleanup and kfree'd before returning. Please check v5.
Thanks in advance.

> Brian
>
>> +     } else {
>> +             sharpsl_nand_cleanup_logical(sharpsl_mtd_logical);
>> +             return err;
>> +     }
>> +
>> +     /* cleanup logical mgmt (FTL) */
>> +     sharpsl_nand_cleanup_logical(sharpsl_mtd_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 (buf_magic(0) != BOOT_MAGIC ||
>> +         buf_magic(1) != FSRO_MAGIC ||
>> +         buf_magic(2) != FSRW_MAGIC) {
>> +             pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* fixup for hardcoded value 64 MiB (for older models) */
>> +     buf1[2].end = cpu_to_le32(master->size);
>> +
>> +     /* extra sanity check */
>> +     if (buf_end(0) <= buf_start(0) ||
>> +         buf_start(1) < buf_end(0) ||
>> +         buf_end(1) <= buf_start(1) ||
>> +         buf_start(2) < buf_end(1) ||
>> +         buf_end(2) <= buf_start(2)) {
>> +             pr_err("sharpslpart: partition sizes 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 = buf_start(0);
>> +     sharpsl_nand_parts[0].size = buf_end(0) - buf_start(0);
>> +
>> +     sharpsl_nand_parts[1].name = "root";
>> +     sharpsl_nand_parts[1].offset = buf_start(1);
>> +     sharpsl_nand_parts[1].size = buf_end(1) - buf_start(1);
>> +
>> +     sharpsl_nand_parts[2].name = "home";
>> +     sharpsl_nand_parts[2].offset = buf_start(2);
>> +     sharpsl_nand_parts[2].size = buf_end(2) - buf_start(2);
>> +
>> +     *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
>>

Re: [PATCH v4 2/9] mtd: nand: sharpsl.h: support partition parser types

[Andrea Adami]

On Sun, Aug 13, 2017 at 9:40 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Le Wed, 28 Jun 2017 22:30:29 +0200,
> Andrea Adami <andrea.adami@gmail.com> a écrit :
>
> Please add a commit message explaining why you're doing that.
>
Sure, I have added a short text.

>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>> ---
>>  include/linux/mtd/sharpsl.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/mtd/sharpsl.h b/include/linux/mtd/sharpsl.h
>> index 65e91d0..c0e0be2 100644
>> --- a/include/linux/mtd/sharpsl.h
>> +++ b/include/linux/mtd/sharpsl.h
>> @@ -17,4 +17,5 @@ struct sharpsl_nand_platform_data {
>>       const struct mtd_ooblayout_ops *ecc_layout;
>>       struct mtd_partition    *partitions;
>>       unsigned int            nr_partitions;
>> +     const char *const       *types; /* names of parsers to use if any */
>
> types is not really descriptive here. How about 'part_parsers' or
> something clearly reflecting the purpose of this field.

I took this line somewhere from a pending mtd patch.
I wasn't convinced of the name and   part_parsers   is better.

>
> BTW, try to avoid putting comments on the same line.
Yeah, we talked about these janitoring needs. Removed.

>
>>  };
>

Please review v5 (coming soon).

Thanks
Andrea

[PATCH v4 2/9] mtd: nand: sharpsl.h: support partition parser types

[Andrea Adami]

On Sun, Aug 13, 2017 at 9:40 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Le Wed, 28 Jun 2017 22:30:29 +0200,
> Andrea Adami <andrea.adami@gmail.com> a ?crit :
>
> Please add a commit message explaining why you're doing that.
>
Sure, I have added a short text.

>> Signed-off-by: Andrea Adami <andrea.adami@gmail.com>
>> ---
>>  include/linux/mtd/sharpsl.h | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/include/linux/mtd/sharpsl.h b/include/linux/mtd/sharpsl.h
>> index 65e91d0..c0e0be2 100644
>> --- a/include/linux/mtd/sharpsl.h
>> +++ b/include/linux/mtd/sharpsl.h
>> @@ -17,4 +17,5 @@ struct sharpsl_nand_platform_data {
>>       const struct mtd_ooblayout_ops *ecc_layout;
>>       struct mtd_partition    *partitions;
>>       unsigned int            nr_partitions;
>> +     const char *const       *types; /* names of parsers to use if any */
>
> types is not really descriptive here. How about 'part_parsers' or
> something clearly reflecting the purpose of this field.

I took this line somewhere from a pending mtd patch.
I wasn't convinced of the name and   part_parsers   is better.

>
> BTW, try to avoid putting comments on the same line.
Yeah, we talked about these janitoring needs. Removed.

>
>>  };
>

Please review v5 (coming soon).

Thanks
Andrea