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

Submitted by Andrea Adami on April 15, 2017, 8:11 p.m.

Details

Message ID 1492287078-22369-2-git-send-email-andrea.adami@gmail.com
State New
Headers show

Commit Message

Andrea Adami April 15, 2017, 8:11 p.m.
The Sharp SL Series (Zaurus) PXA handhelds have 16/64/128M of NAND flash
and share the same layout of the first 7M partition, managed by Sharp FTL.

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

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

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

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

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

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

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

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

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

Comments

Marek Vasut April 16, 2017, 4:07 p.m.
On 04/15/2017 10:11 PM, 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      |   2 +
>  drivers/mtd/sharpsl_ftl.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/mtd/sharpsl_ftl.h |  34 +++++
>  drivers/mtd/sharpslpart.c | 142 +++++++++++++++++++++
>  5 files changed, 495 insertions(+)
>  create mode 100644 drivers/mtd/sharpsl_ftl.c
>  create mode 100644 drivers/mtd/sharpsl_ftl.h
>  create mode 100644 drivers/mtd/sharpslpart.c
> 
> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
> index e83a279..5c96127 100644
> --- a/drivers/mtd/Kconfig
> +++ b/drivers/mtd/Kconfig
> @@ -155,6 +155,14 @@ config MTD_BCM47XX_PARTS
>  	  This provides partitions parser for devices based on BCM47xx
>  	  boards.
>  
> +config MTD_SHARPSL_PARTS
> +	tristate "Sharp SL Series NAND flash partition parser"
> +	depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO
> +	help
> +	  This provides the read-only FTL logic necessary to read the partition
> +	  table from the NAND flash of Sharp SL Series (Zaurus) and the MTD
> +	  partition parser using this code.
> +
>  comment "User Modules And Translation Layers"
>  
>  #
> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
> index 99bb9a1..89f707b 100644
> --- a/drivers/mtd/Makefile
> +++ b/drivers/mtd/Makefile
> @@ -13,6 +13,8 @@ obj-$(CONFIG_MTD_AFS_PARTS)	+= afs.o
>  obj-$(CONFIG_MTD_AR7_PARTS)	+= ar7part.o
>  obj-$(CONFIG_MTD_BCM63XX_PARTS)	+= bcm63xxpart.o
>  obj-$(CONFIG_MTD_BCM47XX_PARTS)	+= bcm47xxpart.o
> +obj-$(CONFIG_MTD_SHARPSL_PARTS)	+= sharpsl-part.o
> +sharpsl-part-objs := sharpsl_ftl.o sharpslpart.o

Can the FTL and partition parser be used separately ?

>  # 'Users' - code which presents functionality to userspace.
>  obj-$(CONFIG_MTD_BLKDEVS)	+= mtd_blkdevs.o
> diff --git a/drivers/mtd/sharpsl_ftl.c b/drivers/mtd/sharpsl_ftl.c
> new file mode 100644
> index 0000000..d8ebefe
> --- /dev/null
> +++ b/drivers/mtd/sharpsl_ftl.c
> @@ -0,0 +1,309 @@
> +/*
> + * MTD method for NAND accessing via logical address (SHARP FTL)
> + *
> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
> + *
> + * Based on 2.4 sources: drivers/mtd/nand/sharp_sl_logical.c
> + * Copyright (C) 2002  SHARP
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include "sharpsl_ftl.h"
> +
> +/* oob structure */
> +#define NAND_NOOB_LOGADDR_00		8
> +#define NAND_NOOB_LOGADDR_01		9
> +#define NAND_NOOB_LOGADDR_10		10
> +#define NAND_NOOB_LOGADDR_11		11
> +#define NAND_NOOB_LOGADDR_20		12
> +#define NAND_NOOB_LOGADDR_21		13
> +
> +/* Logical Table */
> +struct mtd_logical {
> +	u32 size;		/* size of the handled partition */
> +	int index;		/* mtd->index */
> +	u_int phymax;		/* physical blocks */
> +	u_int logmax;		/* logical blocks */
> +	u_int *log2phy;		/* the logical-to-physical table */
> +};
> +
> +static struct mtd_logical *sharpsl_mtd_logical;
> +
> +/* wrapper */
> +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,
> +				 size_t *retlen, uint8_t *buf)
> +{
> +	loff_t mask = mtd->writesize - 1;
> +	struct mtd_oob_ops ops;
> +	int res;
> +
> +	ops.mode = MTD_OPS_PLACE_OOB;
> +	ops.ooboffs = offs & mask;
> +	ops.ooblen = len;
> +	ops.oobbuf = buf;
> +	ops.datbuf = NULL;
> +
> +	res = mtd_read_oob(mtd, offs & ~mask, &ops);
> +	*retlen = ops.oobretlen;

You sure you want to do this assignment in all cases (also in fail case) ?

> +	return res;
> +}
> +
> +/* utility */
> +static u_int sharpsl_nand_get_logical_no(unsigned char *oob)
> +{
> +	unsigned short us, bit;
> +	int par;
> +	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]) {

Consistent indent would be nice.

> +		good0 = NAND_NOOB_LOGADDR_20;
> +		good1 = NAND_NOOB_LOGADDR_21;
> +	} else {
> +		return (u_int)-1;

What is this ?

> +	}
> +
> +	us = (((unsigned short)(oob[good0]) & 0x00ff) << 0) |
> +		(((unsigned short)(oob[good1]) & 0x00ff) << 8);

oob is unsigned char array, the cast and masking should not be needed.
btw make us into u16 type.

> +	par = 0;
> +	for (bit = 0x0001; bit != 0; bit <<= 1) {

You can use the BIT() macro here.

> +		if (us & bit)
> +			par++;
> +	}
> +	if (par & 1)
> +		return (u_int)-2;

Again, what's this return (u_int) stuff ?

> +
> +	if (us == 0xffff)
> +		return 0xffff;
> +	else
> +		return ((us & 0x07fe) >> 1);

One parenthesis set too many here.

> +}
> +
> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u_int32_t partition_size)
> +{
> +	struct mtd_logical *logical = NULL;
> +	u_int block_no;
> +	loff_t block_adr;
> +	u_int log_no;
> +	unsigned char *oob = NULL;
> +	int readretry;
> +	int ret;
> +	int i;
> +	size_t retlen;
> +
> +	/* check argument */
> +	if ((partition_size % mtd->erasesize) != 0) {

Can this even happen ?

> +		pr_err("Illegal partition size. (0x%x)\n", partition_size);
> +		return -EINVAL;
> +	}
> +
> +	/* Allocate memory for Logical Address Management */
> +	logical = kzalloc(sizeof(*logical), GFP_KERNEL);
> +	if (!logical)
> +		return -ENOMEM;
> +
> +	/* Allocate a few more bytes of memory for oob */
> +	oob = kzalloc(mtd->oobsize, GFP_KERNEL);

Can we use devm_ functions all over the place ?

Can we avoid the double-allocation, ie. by embedding the maximum oob
sized buffer into struct mtd_logical (which might need renaming btw)
or allocating it on stack (it's likely a few bytes)?

> +	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);
> +
> +	pr_debug("FTL blocks: %d physical (%d logical + %d reserved)\n",
> +		 logical->phymax, logical->logmax,
> +		 logical->phymax - logical->logmax);
> +
> +	logical->log2phy = NULL;
> +
> +	/* Allocate memory for logical->log2phy */
> +	logical->log2phy = kcalloc(logical->logmax, sizeof(u_int), GFP_KERNEL);

Another allocation which could be avoided IMO

> +	if (!logical->log2phy) {
> +		kfree(logical);
> +		kfree(oob);
> +		return -ENOMEM;
> +	}
> +
> +	/* initialize logical->log2phy */
> +	for (i = 0; i < logical->logmax; i++)
> +		logical->log2phy[i] = (u_int)-1;

You should stop using this (u_int) stuff unless there's a real good
reason for that (which I doubt).

> +	/* create physical-logical table */
> +	for (block_no = 0; block_no < logical->phymax; block_no++) {
> +		block_adr = block_no * mtd->erasesize;
> +
> +		readretry = 3;
> +read_retry:
> +		/* avoid bad blocks */
> +		if (mtd_block_isbad(mtd, block_adr)) {
> +			pr_debug("block_no=%d is marked as bad, skipping\n",
> +				 block_no);

Can we use dev_dbg() and co. all over the place ?

> +			continue;
> +		}
> +
> +		/* wrapper for mtd_read_oob() */
> +		ret = sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize,
> +					    &retlen, oob);
> +
> +		if (ret != 0) {
> +			pr_warn("Failed in read_oob, skip this block.\n");
> +			continue;
> +		}
> +
> +		if (mtd->oobsize != retlen) {
> +			pr_warn("read_oob cannot read full-size.\n");
> +			continue;
> +		}
> +
> +		/* logical address */
> +		log_no = sharpsl_nand_get_logical_no(oob);
> +
> +		if ((int)log_no < 0) {

Is the typecast needed ? Probably not ...

> +			pr_warn("Bad logical no found.\n");

Bad English is found though :-) Please fix the comment.

> +			readretry--;
> +			if (readretry > 0)
> +				goto read_retry;
> +			continue;
> +		}
> +
> +		/* reserved (FTL) */
> +		if (log_no == 0xffff) {
> +			pr_debug("block_no=%d is reserved, skipping\n",
> +				 block_no);
> +			continue;
> +		}
> +
> +		/* logical address available ? */
> +		if (log_no >= logical->logmax) {
> +			pr_warn("The logical no is too big. (%d)\n", log_no);
> +			pr_warn("***partition(%d) erasesize(%d) logmax(%d)\n",
> +				partition_size, mtd->erasesize,
> +				logical->logmax);
> +		continue;

Indent.

> +		}
> +
> +		/* duplicated or not ? */
> +		if (logical->log2phy[log_no] == (u_int)(-1)) {
> +			logical->log2phy[log_no] = block_no;
> +		} else {
> +			pr_warn("The logical no is duplicated. (%d)\n", log_no);
> +			continue;

Continue not needed.

> +		}
> +
> +		/* pr_debug("FTL map: block_no=%d block_adr=0x%x log_no=%d\n",
> +		 *		block_no, (u_int32_t)block_adr, log_no);
> +		 */

Please make up your mind with this comment.

> +	}
> +	kfree(oob);
> +	sharpsl_mtd_logical = logical;
> +
> +	return 0;
> +}
> +
> +void sharpsl_nand_cleanup_logical(void)
> +{
> +	struct mtd_logical *logical = sharpsl_mtd_logical;
> +
> +	sharpsl_mtd_logical = NULL;
> +
> +	kfree(logical->log2phy);
> +	logical->log2phy = NULL;
> +	kfree(logical);
> +	logical = NULL;
> +}
> +
> +/* MTD METHOD */
> +int sharpsl_nand_read_laddr(struct mtd_info *mtd,
> +			    loff_t from,
> +			    size_t len,
> +			    u_char *buf)
> +{
> +	struct mtd_logical *logical;
> +	u_int log_no;
> +	u_int block_no;
> +	loff_t block_adr;
> +	loff_t block_ofs;
> +	size_t retlen;
> +	int ret;
> +
> +	/* support only for one block */
> +	if (len <= 0) {
> +		pr_err("Illegal argumnent. len=0x%x\n",
> +		       len);

Indent :)

> +		return -EINVAL;
> +	}
> +
> +	if ((((u_int32_t)from) / mtd->erasesize) != ((((u_int32_t)from) +
> +		len - 1) / mtd->erasesize)) {
> +		pr_err("Illegal argument. from=0x%x len=0x%x\n",
> +		       (u_int32_t)from, len);

Typecast , indent (fix globally), the condition looks like crap ...

Use u8/u16/u32 types btw

> +		return -EINVAL;
> +	}
> +
> +	logical = sharpsl_mtd_logical;
> +	log_no = (((u_int32_t)from) / mtd->erasesize);
> +
> +	if (log_no >= logical->logmax) {
> +		pr_err("Illegal argument. from=0x%x\n",
> +		       (u_int32_t)from);
> +		return -EINVAL;
> +	}
> +
> +	/* is log_no used ? */
> +	if (logical->log2phy[log_no] == (u_int)-1) {
> +		pr_err("There is not the LOGICAL block. from=0x%x\n",
> +		       (u_int32_t)from);
> +		return -EINVAL;
> +	}
> +
> +	/* physical address */
> +	block_no = logical->log2phy[log_no];
> +	block_adr = block_no * mtd->erasesize;
> +	block_ofs = (((u_int32_t)from) % mtd->erasesize);
> +
> +	/* read */
> +	ret = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
> +	if (ret != 0) {
> +		pr_err("mtd_read failed in sharpsl_nand_read_laddr()\n");
> +		return ret;
> +	}
> +
> +	if (len != retlen) {
> +		pr_err("mtd_read cannot read full-size.\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> diff --git a/drivers/mtd/sharpsl_ftl.h b/drivers/mtd/sharpsl_ftl.h
> new file mode 100644
> index 0000000..f639646
> --- /dev/null
> +++ b/drivers/mtd/sharpsl_ftl.h
> @@ -0,0 +1,34 @@
> +/*
> + * Header file for NAND accessing via logical address (SHARP FTL)
> + *
> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
> + *
> + * Based on 2.4 sources: linux/include/asm-arm/sharp_nand_logical.h
> + * Copyright (C) 2002  SHARP
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#ifndef __SHARPSL_NAND_LOGICAL_H__
> +#define __SHARPSL_NAND_LOGICAL_H__
> +
> +#include <linux/types.h>
> +#include <linux/mtd/mtd.h>
> +
> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u_int32_t partition_size);
> +
> +void sharpsl_nand_cleanup_logical(void);
> +
> +int sharpsl_nand_read_laddr(struct mtd_info *mtd, loff_t from, size_t len,
> +			    u_char *buf);
> +
> +#endif
> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
> new file mode 100644
> index 0000000..dcd3cbe
> --- /dev/null
> +++ b/drivers/mtd/sharpslpart.c
> @@ -0,0 +1,142 @@
> +/*
> + * MTD partition parser for NAND flash on Sharp SL Series
> + *
> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/module.h>
> +#include <linux/mtd/mtd.h>
> +#include <linux/mtd/partitions.h>
> +#include "sharpsl_ftl.h"
> +
> +/* factory defaults */
> +#define SHARPSL_NAND_PARTS		3
> +#define SHARPSL_FTL_PARTITION_SIZE	(7 * 1024 * 1024)
> +#define PARAM_BLOCK_PARTITIONINFO1	0x00060000
> +#define PARAM_BLOCK_PARTITIONINFO2	0x00064000
> +
> +#define BOOT_MAGIC			be32_to_cpu(0x424f4f54)   /* BOOT */
> +#define FSRO_MAGIC			be32_to_cpu(0x4653524f)   /* FSRO */
> +#define FSRW_MAGIC			be32_to_cpu(0x46535257)   /* FSRW */
> +
> +/*
> + * Sample values read from SL-C860
> + *
> + * # cat /proc/mtd
> + * dev:    size   erasesize  name
> + * mtd0: 006d0000 00020000 "Filesystem"
> + * mtd1: 00700000 00004000 "smf"
> + * mtd2: 03500000 00004000 "root"
> + * mtd3: 04400000 00004000 "home"
> + *
> + * PARTITIONINFO1
> + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00  ......p.BOOT....
> + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00  ..p.....FSRO....
> + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00  ........FSRW....
> + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
> + */
> +
> +struct sharpsl_nand_partitioninfo {
> +	u32 boot_start;
> +	u32 boot_end;
> +	u32 boot_magic;
> +	u32 boot_reserved;

Could be reduced to

struct sharpsl_nand_partition_info {
	u32 start;
	u32 end;
	u32 magic;
	u32 reserved;
};

And then have struct sharpsl_nand_partition_info *foo; and index it with
{ 0, 1, 2 } .

> +	u32 fsro_start;
> +	u32 fsro_end;
> +	u32 fsro_magic;
> +	u32 fsro_reserved;
> +	u32 fsrw_start;
> +	u32 fsrw_end; /* ignore, was for the older models with 64M flash */
> +	u32 fsrw_magic;
> +	u32 fsrw_reserved;
> +};
> +
> +static int sharpsl_parse_mtd_partitions(struct mtd_info *master,
> +					const struct mtd_partition **pparts,
> +					struct mtd_part_parser_data *data)
> +{
> +	struct sharpsl_nand_partitioninfo buf1;
> +	struct sharpsl_nand_partitioninfo buf2;
> +	struct mtd_partition *sharpsl_nand_parts;
> +
> +	/* init logical mgmt (FTL) */
> +	if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
> +		return -EINVAL;
> +
> +	/* read the two partition tables */
> +	if (sharpsl_nand_read_laddr(master,
> +				    PARAM_BLOCK_PARTITIONINFO1, 48,
> +				    (u_char *)&buf1) ||
> +	    sharpsl_nand_read_laddr(master,
> +				    PARAM_BLOCK_PARTITIONINFO2, 48,
> +				    (u_char *)&buf2))
> +		return -EINVAL;
> +
> +	/* cleanup logical mgmt (FTL) */
> +	sharpsl_nand_cleanup_logical();
> +
> +	/* compare the two buffers */
> +	if (!memcmp(&buf1, &buf2, 48)) {
> +		pr_debug("sharpslpart: PARTITIONINFO 1/2 are in sync.\n");
> +	} else {
> +		pr_err("sharpslpart: PARTITIONINFO 1/2 differ. Quit parser.\n");
> +		return -EINVAL;
> +	}
> +
> +	/* check for magics (just in the first) */
> +	if (buf1.boot_magic == BOOT_MAGIC &&
> +	    buf1.fsro_magic == FSRO_MAGIC &&
> +	    buf1.fsrw_magic == FSRW_MAGIC) {
> +		pr_debug("sharpslpart: magic values found.\n");

Can we use dev_dbg() ?

> +	} else {
> +		pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
> +		return -EINVAL;
> +	}

Can we use devm_kzalloc() ?

> +	sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) *
> +				     SHARPSL_NAND_PARTS, GFP_KERNEL);
> +	if (!sharpsl_nand_parts)
> +		return -ENOMEM;
> +
> +	/* original names */
> +	sharpsl_nand_parts[0].name = "smf";
> +	sharpsl_nand_parts[0].offset = buf1.boot_start;
> +	sharpsl_nand_parts[0].size = buf1.boot_end - buf1.boot_start;
> +	sharpsl_nand_parts[0].mask_flags = 0;
> +
> +	sharpsl_nand_parts[1].name = "root";
> +	sharpsl_nand_parts[1].offset = buf1.fsro_start;
> +	sharpsl_nand_parts[1].size = buf1.fsro_end - buf1.fsro_start;
> +	sharpsl_nand_parts[1].mask_flags = 0;
> +
> +	sharpsl_nand_parts[2].name = "home";
> +	sharpsl_nand_parts[2].offset = buf1.fsrw_start;
> +	sharpsl_nand_parts[2].size = master->size - buf1.fsrw_start;
> +	sharpsl_nand_parts[2].mask_flags = 0;
> +
> +	*pparts = sharpsl_nand_parts;
> +	return SHARPSL_NAND_PARTS;
> +}
> +
> +static struct mtd_part_parser sharpsl_mtd_parser = {
> +	.parse_fn = sharpsl_parse_mtd_partitions,
> +	.name = "sharpslpart",
> +};
> +module_mtd_part_parser(sharpsl_mtd_parser);
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>");
> +MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series");
>
Boris Brezillon April 17, 2017, 2:44 p.m.
Marek, Andrea,

Before we even start discussing minor improvements (like coding style),
I'd like to discuss the sharp FTL and partition table format, and
decide whether we want to have such an old FTL included in the kernel.

Actually, that's the very reason I asked Andrea to post his series as
soon as possible even if some things were not perfect.

I'll try to document myself on the on-flash format of the FTL and
partition table before giving a definitive opinion, but I have the
feeling this ancient FTL is not 100% safe, and by accepting an old
(maybe unreliable?) FTL we are setting a precedent, and refusing other
(broken) proprietary/vendor FTLs will be almost impossible after that.

Maybe I'm wrong, and this FTL is really safe and can be used on other
devices, that's why I'd like to understand how it works before giving
my opinion.


On Sun, 16 Apr 2017 18:07:47 +0200
Marek Vasut <marek.vasut@gmail.com> wrote:

> On 04/15/2017 10:11 PM, 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 )


Okay, IMO that's not really a good argument to support this partition
parser. As Richard and I already mentioned several times, if your
bootloader is not capable of passing valid mtdparts= you can hardcode
it in the kernel using a default CMDLINE.

Now, I understand that you want to support multiple devices with the
same kernel, and having this partition parser would simplify your job.
I also know you are developing a 2nd stage bootloader (based on
kexec+a minimal initramfs) to address the limitations of the
non-upgradable 1st stage bootloader.

According to the rest of the description, you already have user-space
tools to manipulate the partition-table and those are aware of the FTL
layer, so I think you have all the basic blocks to get rid of this
in-kernel implementation.

All you need is a way to extract the partitions from the 2nd stage
bootloader (using some tools you have in your initramfs) and build the
according mtdparts= parameter to pass to kexec when booting the real
kernel (the one used by the system).

You tried to explain why it was not doable, but I still don't see
why.
You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS
enabled and that some people were booting distro kernels. Honestly, I
think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those
distro kernels than having your custom FTL enabled.
Then you tried to explain that with the user-space only solution you
wouldn't be able to support systems where the user had resized the
partitions with the nandlogical tool, and I still don't see why. Maybe
you can give more details to explain why this is impossible.

Just going through all these details to say that, IMO, we should only
consider inclusion of this feature if we think it's safe, because I
think all that is done here can be done from user-space.

One last thing I was wondering. You said you want to keep existing
partitioning unchanged, but I'd recommend to just drop the existing
partitioning and have a single 121MB partition with UBI on it.
This way you get support for UBI volumes, which is doing exactly what
this FTL+partition-table is doing but in a standard/safe way.
What is forcing you to keep the existing partitioning exactly?

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

I'll comment on the FTL and partition-table format later, after I had
time to review it properly.

In the meantime, I'd like other MTD maintainers to comment on my
reply. Maybe I'm the only one to think that supporting
old/unmaintainerd FTLs is a bad idea, and in this case I'll have to
accept the situation ;-).

Regards,

Boris
Andrea Adami April 18, 2017, 7:57 a.m.
On Sun, Apr 16, 2017 at 6:07 PM, Marek Vasut <marek.vasut@gmail.com> wrote:
> On 04/15/2017 10:11 PM, 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      |   2 +
>>  drivers/mtd/sharpsl_ftl.c | 309 ++++++++++++++++++++++++++++++++++++++++++++++
>>  drivers/mtd/sharpsl_ftl.h |  34 +++++
>>  drivers/mtd/sharpslpart.c | 142 +++++++++++++++++++++
>>  5 files changed, 495 insertions(+)
>>  create mode 100644 drivers/mtd/sharpsl_ftl.c
>>  create mode 100644 drivers/mtd/sharpsl_ftl.h
>>  create mode 100644 drivers/mtd/sharpslpart.c
>>
>> diff --git a/drivers/mtd/Kconfig b/drivers/mtd/Kconfig
>> index e83a279..5c96127 100644
>> --- a/drivers/mtd/Kconfig
>> +++ b/drivers/mtd/Kconfig
>> @@ -155,6 +155,14 @@ config MTD_BCM47XX_PARTS
>>         This provides partitions parser for devices based on BCM47xx
>>         boards.
>>
>> +config MTD_SHARPSL_PARTS
>> +     tristate "Sharp SL Series NAND flash partition parser"
>> +     depends on MTD_NAND_SHARPSL || MTD_NAND_TMIO
>> +     help
>> +       This provides the read-only FTL logic necessary to read the partition
>> +       table from the NAND flash of Sharp SL Series (Zaurus) and the MTD
>> +       partition parser using this code.
>> +
>>  comment "User Modules And Translation Layers"
>>
>>  #
>> diff --git a/drivers/mtd/Makefile b/drivers/mtd/Makefile
>> index 99bb9a1..89f707b 100644
>> --- a/drivers/mtd/Makefile
>> +++ b/drivers/mtd/Makefile
>> @@ -13,6 +13,8 @@ obj-$(CONFIG_MTD_AFS_PARTS) += afs.o
>>  obj-$(CONFIG_MTD_AR7_PARTS)  += ar7part.o
>>  obj-$(CONFIG_MTD_BCM63XX_PARTS)      += bcm63xxpart.o
>>  obj-$(CONFIG_MTD_BCM47XX_PARTS)      += bcm47xxpart.o
>> +obj-$(CONFIG_MTD_SHARPSL_PARTS)      += sharpsl-part.o
>> +sharpsl-part-objs := sharpsl_ftl.o sharpslpart.o
>
Hello,
thanks for the review.

I have taken in account your observations and I am applying the
suggested fixes (see comments).

> Can the FTL and partition parser be used separately ?

The parser needs the FTL to read.
The FTL could be used elsewhere but I only know about Sharp Zaurus using it.

I put the original 2.4 files here:
https://github.com/LinuxPDA/Sharp_FTL_2.4.20

>
>>  # 'Users' - code which presents functionality to userspace.
>>  obj-$(CONFIG_MTD_BLKDEVS)    += mtd_blkdevs.o
>> diff --git a/drivers/mtd/sharpsl_ftl.c b/drivers/mtd/sharpsl_ftl.c
>> new file mode 100644
>> index 0000000..d8ebefe
>> --- /dev/null
>> +++ b/drivers/mtd/sharpsl_ftl.c
>> @@ -0,0 +1,309 @@
>> +/*
>> + * MTD method for NAND accessing via logical address (SHARP FTL)
>> + *
>> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
>> + *
>> + * Based on 2.4 sources: drivers/mtd/nand/sharp_sl_logical.c
>> + * Copyright (C) 2002  SHARP
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/init.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/partitions.h>
>> +#include "sharpsl_ftl.h"
>> +
>> +/* oob structure */
>> +#define NAND_NOOB_LOGADDR_00         8
>> +#define NAND_NOOB_LOGADDR_01         9
>> +#define NAND_NOOB_LOGADDR_10         10
>> +#define NAND_NOOB_LOGADDR_11         11
>> +#define NAND_NOOB_LOGADDR_20         12
>> +#define NAND_NOOB_LOGADDR_21         13
>> +
>> +/* Logical Table */
>> +struct mtd_logical {
>> +     u32 size;               /* size of the handled partition */
>> +     int index;              /* mtd->index */
>> +     u_int phymax;           /* physical blocks */
>> +     u_int logmax;           /* logical blocks */
>> +     u_int *log2phy;         /* the logical-to-physical table */
>> +};
>> +
>> +static struct mtd_logical *sharpsl_mtd_logical;
>> +
>> +/* wrapper */
>> +static int sharpsl_nand_read_oob(struct mtd_info *mtd, loff_t offs, size_t len,
>> +                              size_t *retlen, uint8_t *buf)
>> +{
>> +     loff_t mask = mtd->writesize - 1;
>> +     struct mtd_oob_ops ops;
>> +     int res;
>> +
>> +     ops.mode = MTD_OPS_PLACE_OOB;
>> +     ops.ooboffs = offs & mask;
>> +     ops.ooblen = len;
>> +     ops.oobbuf = buf;
>> +     ops.datbuf = NULL;
>> +
>> +     res = mtd_read_oob(mtd, offs & ~mask, &ops);
>> +     *retlen = ops.oobretlen;
>
> You sure you want to do this assignment in all cases (also in fail case) ?
Well, I needed a wrapper for the old mtd->read_oob() ...
this example comes straight from other sources (ntflcore.c, inftlcore.c,).
I do a check afterwards: if ((ret != 0) || (mtd->oobsize != retlen))..

>
>> +     return res;
>> +}
>> +
>> +/* utility */
>> +static u_int sharpsl_nand_get_logical_no(unsigned char *oob)
>> +{
>> +     unsigned short us, bit;
>> +     int par;
>> +     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]) {
>
> Consistent indent would be nice.
About this, I am doing a personal battle with checkpatch.pl --strict.
I'll fix all the indentings, sorry for the eyesore.

>
>> +             good0 = NAND_NOOB_LOGADDR_20;
>> +             good1 = NAND_NOOB_LOGADDR_21;
>> +     } else {
>> +             return (u_int)-1;
>
> What is this ?
They used this casting of negative integer to signale abnormal exit.

>
>> +     }
>> +
>> +     us = (((unsigned short)(oob[good0]) & 0x00ff) << 0) |
>> +             (((unsigned short)(oob[good1]) & 0x00ff) << 8);
>
> oob is unsigned char array, the cast and masking should not be needed.
> btw make us into u16 type.
ok, I'll try

>
>> +     par = 0;
>> +     for (bit = 0x0001; bit != 0; bit <<= 1) {
>
> You can use the BIT() macro here.
Ok, I'l have to learn it

>
>> +             if (us & bit)
>> +                     par++;
>> +     }
>> +     if (par & 1)
>> +             return (u_int)-2;
>
> Again, what's this return (u_int) stuff ?
As before, replaced with UINT_MAX

>
>> +
>> +     if (us == 0xffff)
>> +             return 0xffff;
>> +     else
>> +             return ((us & 0x07fe) >> 1);
>
> One parenthesis set too many here.
Apparently not. This is necessary or you get wrong result.

>
>> +}
>> +
>> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u_int32_t partition_size)
>> +{
>> +     struct mtd_logical *logical = NULL;
>> +     u_int block_no;
>> +     loff_t block_adr;
>> +     u_int log_no;
>> +     unsigned char *oob = NULL;
>> +     int readretry;
>> +     int ret;
>> +     int i;
>> +     size_t retlen;
>> +
>> +     /* check argument */
>> +     if ((partition_size % mtd->erasesize) != 0) {
>
> Can this even happen ?
Not really. Removed.

>
>> +             pr_err("Illegal partition size. (0x%x)\n", partition_size);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* Allocate memory for Logical Address Management */
>> +     logical = kzalloc(sizeof(*logical), GFP_KERNEL);
>> +     if (!logical)
>> +             return -ENOMEM;
>> +
>> +     /* Allocate a few more bytes of memory for oob */
>> +     oob = kzalloc(mtd->oobsize, GFP_KERNEL);
>
> Can we use devm_ functions all over the place ?
Probably yes but I'll need help with the new implementation.

>
> Can we avoid the double-allocation, ie. by embedding the maximum oob
> sized buffer into struct mtd_logical (which might need renaming btw)
> or allocating it on stack (it's likely a few bytes)?

I'm open to suggestions: I started taking the old code.
Here the sources had a nice #if defined() and the oob was [16] or [64].
The right size is the oobsize.

>
>> +     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);
>> +
>> +     pr_debug("FTL blocks: %d physical (%d logical + %d reserved)\n",
>> +              logical->phymax, logical->logmax,
>> +              logical->phymax - logical->logmax);
>> +
>> +     logical->log2phy = NULL;
>> +
>> +     /* Allocate memory for logical->log2phy */
>> +     logical->log2phy = kcalloc(logical->logmax, sizeof(u_int), GFP_KERNEL);
>
> Another allocation which could be avoided IMO
Like before, this will need a redesign.

>
>> +     if (!logical->log2phy) {
>> +             kfree(logical);
>> +             kfree(oob);
>> +             return -ENOMEM;
>> +     }
>> +
>> +     /* initialize logical->log2phy */
>> +     for (i = 0; i < logical->logmax; i++)
>> +             logical->log2phy[i] = (u_int)-1;
>
> You should stop using this (u_int) stuff unless there's a real good
> reason for that (which I doubt).
I have just replaced that with UINT_MAX so one can imagine at least
the bit mask.
They mark the [i] with an out-of-range value, which will be always
bigger than the number of blocks.
>
>> +     /* create physical-logical table */
>> +     for (block_no = 0; block_no < logical->phymax; block_no++) {
>> +             block_adr = block_no * mtd->erasesize;
>> +
>> +             readretry = 3;
>> +read_retry:
>> +             /* avoid bad blocks */
>> +             if (mtd_block_isbad(mtd, block_adr)) {
>> +                     pr_debug("block_no=%d is marked as bad, skipping\n",
>> +                              block_no);
>
> Can we use dev_dbg() and co. all over the place ?
This is another big change, I see the other parsers do use pr_err et
co,. so I just did that.

>
>> +                     continue;
>> +             }
>> +
>> +             /* wrapper for mtd_read_oob() */
>> +             ret = sharpsl_nand_read_oob(mtd, block_adr, mtd->oobsize,
>> +                                         &retlen, oob);
>> +
>> +             if (ret != 0) {
>> +                     pr_warn("Failed in read_oob, skip this block.\n");
>> +                     continue;
>> +             }
>> +
>> +             if (mtd->oobsize != retlen) {
>> +                     pr_warn("read_oob cannot read full-size.\n");
>> +                     continue;
>> +             }
>> +
>> +             /* logical address */
>> +             log_no = sharpsl_nand_get_logical_no(oob);
>> +
>> +             if ((int)log_no < 0) {
>
> Is the typecast needed ? Probably not ...

Well, yes, because it is an unsigned int, set to UINT_MAX.
If you cast it to int it becomes negative again afais.

>
>> +                     pr_warn("Bad logical no found.\n");
>
> Bad English is found though :-) Please fix the comment.

I have removed many comments. This is not userspace and debug should
not be needed.

>
>> +                     readretry--;
>> +                     if (readretry > 0)
>> +                             goto read_retry;
>> +                     continue;
>> +             }
>> +
>> +             /* reserved (FTL) */
>> +             if (log_no == 0xffff) {
>> +                     pr_debug("block_no=%d is reserved, skipping\n",
>> +                              block_no);
>> +                     continue;
>> +             }
>> +
>> +             /* logical address available ? */
>> +             if (log_no >= logical->logmax) {
>> +                     pr_warn("The logical no is too big. (%d)\n", log_no);
>> +                     pr_warn("***partition(%d) erasesize(%d) logmax(%d)\n",
>> +                             partition_size, mtd->erasesize,
>> +                             logical->logmax);
>> +             continue;
>
> Indent.
Sorry again...
>
>> +             }
>> +
>> +             /* duplicated or not ? */
>> +             if (logical->log2phy[log_no] == (u_int)(-1)) {
>> +                     logical->log2phy[log_no] = block_no;
>> +             } else {
>> +                     pr_warn("The logical no is duplicated. (%d)\n", log_no);
>> +                     continue;
>
> Continue not needed.
Removed
>
>> +             }
>> +
>> +             /* pr_debug("FTL map: block_no=%d block_adr=0x%x log_no=%d\n",
>> +              *              block_no, (u_int32_t)block_adr, log_no);
>> +              */
>
> Please make up your mind with this comment.
Removed, debug not necessary anymore

>
>> +     }
>> +     kfree(oob);
>> +     sharpsl_mtd_logical = logical;
>> +
>> +     return 0;
>> +}
>> +
>> +void sharpsl_nand_cleanup_logical(void)
>> +{
>> +     struct mtd_logical *logical = sharpsl_mtd_logical;
>> +
>> +     sharpsl_mtd_logical = NULL;
>> +
>> +     kfree(logical->log2phy);
>> +     logical->log2phy = NULL;
>> +     kfree(logical);
>> +     logical = NULL;
>> +}
>> +
>> +/* MTD METHOD */
>> +int sharpsl_nand_read_laddr(struct mtd_info *mtd,
>> +                         loff_t from,
>> +                         size_t len,
>> +                         u_char *buf)
>> +{
>> +     struct mtd_logical *logical;
>> +     u_int log_no;
>> +     u_int block_no;
>> +     loff_t block_adr;
>> +     loff_t block_ofs;
>> +     size_t retlen;
>> +     int ret;
>> +
>> +     /* support only for one block */
>> +     if (len <= 0) {
>> +             pr_err("Illegal argumnent. len=0x%x\n",
>> +                    len);
>
> Indent :)
Sorry...
>
>> +             return -EINVAL;
>> +     }
>> +
>> +     if ((((u_int32_t)from) / mtd->erasesize) != ((((u_int32_t)from) +
>> +             len - 1) / mtd->erasesize)) {
>> +             pr_err("Illegal argument. from=0x%x len=0x%x\n",
>> +                    (u_int32_t)from, len);
>
> Typecast , indent (fix globally), the condition looks like crap ...
>
> Use u8/u16/u32 types btw

I'll try to rewrite it so to check block boundaries.

>
>> +             return -EINVAL;
>> +     }
>> +
>> +     logical = sharpsl_mtd_logical;
>> +     log_no = (((u_int32_t)from) / mtd->erasesize);
>> +
>> +     if (log_no >= logical->logmax) {
>> +             pr_err("Illegal argument. from=0x%x\n",
>> +                    (u_int32_t)from);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* is log_no used ? */
>> +     if (logical->log2phy[log_no] == (u_int)-1) {
>> +             pr_err("There is not the LOGICAL block. from=0x%x\n",
>> +                    (u_int32_t)from);
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* physical address */
>> +     block_no = logical->log2phy[log_no];
>> +     block_adr = block_no * mtd->erasesize;
>> +     block_ofs = (((u_int32_t)from) % mtd->erasesize);
>> +
>> +     /* read */
>> +     ret = mtd_read(mtd, block_adr + block_ofs, len, &retlen, buf);
>> +     if (ret != 0) {
>> +             pr_err("mtd_read failed in sharpsl_nand_read_laddr()\n");
>> +             return ret;
>> +     }
>> +
>> +     if (len != retlen) {
>> +             pr_err("mtd_read cannot read full-size.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     return 0;
>> +}
>> diff --git a/drivers/mtd/sharpsl_ftl.h b/drivers/mtd/sharpsl_ftl.h
>> new file mode 100644
>> index 0000000..f639646
>> --- /dev/null
>> +++ b/drivers/mtd/sharpsl_ftl.h
>> @@ -0,0 +1,34 @@
>> +/*
>> + * Header file for NAND accessing via logical address (SHARP FTL)
>> + *
>> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
>> + *
>> + * Based on 2.4 sources: linux/include/asm-arm/sharp_nand_logical.h
>> + * Copyright (C) 2002  SHARP
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#ifndef __SHARPSL_NAND_LOGICAL_H__
>> +#define __SHARPSL_NAND_LOGICAL_H__
>> +
>> +#include <linux/types.h>
>> +#include <linux/mtd/mtd.h>
>> +
>> +int sharpsl_nand_init_logical(struct mtd_info *mtd, u_int32_t partition_size);
>> +
>> +void sharpsl_nand_cleanup_logical(void);
>> +
>> +int sharpsl_nand_read_laddr(struct mtd_info *mtd, loff_t from, size_t len,
>> +                         u_char *buf);
>> +
>> +#endif
>> diff --git a/drivers/mtd/sharpslpart.c b/drivers/mtd/sharpslpart.c
>> new file mode 100644
>> index 0000000..dcd3cbe
>> --- /dev/null
>> +++ b/drivers/mtd/sharpslpart.c
>> @@ -0,0 +1,142 @@
>> +/*
>> + * MTD partition parser for NAND flash on Sharp SL Series
>> + *
>> + * Copyright (C) 2017 Andrea Adami <andrea.adami@gmail.com>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + *
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/slab.h>
>> +#include <linux/module.h>
>> +#include <linux/mtd/mtd.h>
>> +#include <linux/mtd/partitions.h>
>> +#include "sharpsl_ftl.h"
>> +
>> +/* factory defaults */
>> +#define SHARPSL_NAND_PARTS           3
>> +#define SHARPSL_FTL_PARTITION_SIZE   (7 * 1024 * 1024)
>> +#define PARAM_BLOCK_PARTITIONINFO1   0x00060000
>> +#define PARAM_BLOCK_PARTITIONINFO2   0x00064000
>> +
>> +#define BOOT_MAGIC                   be32_to_cpu(0x424f4f54)   /* BOOT */
>> +#define FSRO_MAGIC                   be32_to_cpu(0x4653524f)   /* FSRO */
>> +#define FSRW_MAGIC                   be32_to_cpu(0x46535257)   /* FSRW */
>> +
>> +/*
>> + * Sample values read from SL-C860
>> + *
>> + * # cat /proc/mtd
>> + * dev:    size   erasesize  name
>> + * mtd0: 006d0000 00020000 "Filesystem"
>> + * mtd1: 00700000 00004000 "smf"
>> + * mtd2: 03500000 00004000 "root"
>> + * mtd3: 04400000 00004000 "home"
>> + *
>> + * PARTITIONINFO1
>> + * 0x00060000: 00 00 00 00 00 00 70 00 42 4f 4f 54 00 00 00 00  ......p.BOOT....
>> + * 0x00060010: 00 00 70 00 00 00 c0 03 46 53 52 4f 00 00 00 00  ..p.....FSRO....
>> + * 0x00060020: 00 00 c0 03 00 00 00 04 46 53 52 57 00 00 00 00  ........FSRW....
>> + * 0x00060030: ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff ff  ................
>> + */
>> +
>> +struct sharpsl_nand_partitioninfo {
>> +     u32 boot_start;
>> +     u32 boot_end;
>> +     u32 boot_magic;
>> +     u32 boot_reserved;
>
> Could be reduced to
>
> struct sharpsl_nand_partition_info {
>         u32 start;
>         u32 end;
>         u32 magic;
>         u32 reserved;
> };
>
> And then have struct sharpsl_nand_partition_info *foo; and index it with
> { 0, 1, 2 } .
>

Yes, ofc, but I thought it is better to let it simple.
The extra loop and code needed will not give much shorter file.

>> +     u32 fsro_start;
>> +     u32 fsro_end;
>> +     u32 fsro_magic;
>> +     u32 fsro_reserved;
>> +     u32 fsrw_start;
>> +     u32 fsrw_end; /* ignore, was for the older models with 64M flash */
This exception would need i.e. an if ()

>> +     u32 fsrw_magic;
>> +     u32 fsrw_reserved;
>> +};
>> +
>> +static int sharpsl_parse_mtd_partitions(struct mtd_info *master,
>> +                                     const struct mtd_partition **pparts,
>> +                                     struct mtd_part_parser_data *data)
>> +{
>> +     struct sharpsl_nand_partitioninfo buf1;
>> +     struct sharpsl_nand_partitioninfo buf2;
>> +     struct mtd_partition *sharpsl_nand_parts;
>> +
>> +     /* init logical mgmt (FTL) */
>> +     if (sharpsl_nand_init_logical(master, SHARPSL_FTL_PARTITION_SIZE))
>> +             return -EINVAL;
>> +
>> +     /* read the two partition tables */
>> +     if (sharpsl_nand_read_laddr(master,
>> +                                 PARAM_BLOCK_PARTITIONINFO1, 48,
>> +                                 (u_char *)&buf1) ||
>> +         sharpsl_nand_read_laddr(master,
>> +                                 PARAM_BLOCK_PARTITIONINFO2, 48,
>> +                                 (u_char *)&buf2))
>> +             return -EINVAL;
>> +
>> +     /* cleanup logical mgmt (FTL) */
>> +     sharpsl_nand_cleanup_logical();
>> +
>> +     /* compare the two buffers */
>> +     if (!memcmp(&buf1, &buf2, 48)) {
>> +             pr_debug("sharpslpart: PARTITIONINFO 1/2 are in sync.\n");
>> +     } else {
>> +             pr_err("sharpslpart: PARTITIONINFO 1/2 differ. Quit parser.\n");
>> +             return -EINVAL;
>> +     }
>> +
>> +     /* check for magics (just in the first) */
>> +     if (buf1.boot_magic == BOOT_MAGIC &&
>> +         buf1.fsro_magic == FSRO_MAGIC &&
>> +         buf1.fsrw_magic == FSRW_MAGIC) {
>> +             pr_debug("sharpslpart: magic values found.\n");
>
> Can we use dev_dbg() ?
As said above, most parsers do use pr_err/pr_warn.
I am tempted to remove these messages as well: the parser should not need debug.

>
>> +     } else {
>> +             pr_err("sharpslpart: magic values mismatch. Quit parser.\n");
>> +             return -EINVAL;
>> +     }
>
> Can we use devm_kzalloc() ?

As above, taken straight from the other parsers.

>
>> +     sharpsl_nand_parts = kzalloc(sizeof(*sharpsl_nand_parts) *
>> +                                  SHARPSL_NAND_PARTS, GFP_KERNEL);
>> +     if (!sharpsl_nand_parts)
>> +             return -ENOMEM;
>> +
>> +     /* original names */
>> +     sharpsl_nand_parts[0].name = "smf";
>> +     sharpsl_nand_parts[0].offset = buf1.boot_start;
>> +     sharpsl_nand_parts[0].size = buf1.boot_end - buf1.boot_start;
>> +     sharpsl_nand_parts[0].mask_flags = 0;
>> +
>> +     sharpsl_nand_parts[1].name = "root";
>> +     sharpsl_nand_parts[1].offset = buf1.fsro_start;
>> +     sharpsl_nand_parts[1].size = buf1.fsro_end - buf1.fsro_start;
>> +     sharpsl_nand_parts[1].mask_flags = 0;
>> +
>> +     sharpsl_nand_parts[2].name = "home";
>> +     sharpsl_nand_parts[2].offset = buf1.fsrw_start;
>> +     sharpsl_nand_parts[2].size = master->size - buf1.fsrw_start;
>> +     sharpsl_nand_parts[2].mask_flags = 0;
>> +
>> +     *pparts = sharpsl_nand_parts;
>> +     return SHARPSL_NAND_PARTS;
>> +}
>> +
>> +static struct mtd_part_parser sharpsl_mtd_parser = {
>> +     .parse_fn = sharpsl_parse_mtd_partitions,
>> +     .name = "sharpslpart",
>> +};
>> +module_mtd_part_parser(sharpsl_mtd_parser);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Andrea Adami <andrea.adami@gmail.com>");
>> +MODULE_DESCRIPTION("MTD partitioning for NAND flash on Sharp SL Series");
>>
>
>
> --
> Best regards,
> Marek Vasut


I know there is still much work to do: I am working at a v2 of the
patch, much shorter.
https://github.com/LinuxPDA/linux/tree/sharpslpart_v2

I'll try to improve it more. Let's wait some more reviews.

Regards
Andrea.
Andrea Adami April 18, 2017, 9:08 a.m.
RESEND (sorry for the HTML)

Boris,
thanks for having read it.

On Mon, Apr 17, 2017 at 4:44 PM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Marek, Andrea,
>
> Before we even start discussing minor improvements (like coding style),
> I'd like to discuss the sharp FTL and partition table format, and
> decide whether we want to have such an old FTL included in the kernel.
>
> Actually, that's the very reason I asked Andrea to post his series as
> soon as possible even if some things were not perfect.
>
> I'll try to document myself on the on-flash format of the FTL and
> partition table before giving a definitive opinion, but I have the
> feeling this ancient FTL is not 100% safe, and by accepting an old
> (maybe unreliable?) FTL we are setting a precedent, and refusing other
> (broken) proprietary/vendor FTLs will be almost impossible after that.
>
> Maybe I'm wrong, and this FTL is really safe and can be used on other
> devices, that's why I'd like to understand how it works before giving
> my opinion.
>
I can't judge the work done by ARM/Sharp/Linaro 15 years ago...
I have only seen this on this PXA Sharp Zaurus SL series.

For the records, these are the GPL 2.4 sources
https://github.com/LinuxPDA/Sharp_FTL_2.4.20

>
> On Sun, 16 Apr 2017 18:07:47 +0200
> Marek Vasut <marek.vasut@gmail.com> wrote:
>
>> On 04/15/2017 10:11 PM, 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 )
>
>
> Okay, IMO that's not really a good argument to support this partition
> parser. As Richard and I already mentioned several times, if your
> bootloader is not capable of passing valid mtdparts= you can hardcode
> it in the kernel using a default CMDLINE.
>
We are simply trying to restore the ability for the kernel to read the mtdparts.

There are around many kernels and distros (OpenEmbedded, Arch, older 2.x stuff).
Some do require to repartition: how can I know how other people partitioned?

> Now, I understand that you want to support multiple devices with the
> same kernel, and having this partition parser would simplify your job.
> I also know you are developing a 2nd stage bootloader (based on
> kexec+a minimal initramfs) to address the limitations of the
> non-upgradable 1st stage bootloader.
>
> According to the rest of the description, you already have user-space
> tools to manipulate the partition-table and those are aware of the FTL
> layer, so I think you have all the basic blocks to get rid of this
> in-kernel implementation.
>
> All you need is a way to extract the partitions from the 2nd stage
> bootloader (using some tools you have in your initramfs) and build the
> according mtdparts= parameter to pass to kexec when booting the real
> kernel (the one used by the system).
>

We have patched kexecboot long ago to do that.
https://github.com/kexecboot/kexecboot/blob/master/machine/zaurus.c

This is done with a special kernel (linux-kexecboot) embedding the
minimal cpio and acting as 2nd stage bootloader.
It passes the mtdparts found in cmdline and does the extra trick of
reading it from mtd1 for zaurus.
You can even customize the cmdline in /boot/boot.cfg and hack the
mtdparts there.

Neverthless, what you don't seem to understand is that I cannot force
people to use kexecboot or to customize cmdline parts as I like...
I do just build kernels and images for testing...I maintain the OE
build infrastructure, not one distro.
> You tried to explain why it was not doable, but I still don't see
> why.
> You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS
> enabled and that some people were booting distro kernels. Honestly, I
> think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those
> distro kernels than having your custom FTL enabled.
> Then you tried to explain that with the user-space only solution you
> wouldn't be able to support systems where the user had resized the
> partitions with the nandlogical tool, and I still don't see why. Maybe
> you can give more details to explain why this is impossible.

I don't understand why people should get crazy with the different
mtdparts= for each model.
IMHO we are restoring a basic functionality, anything weird.

>
> Just going through all these details to say that, IMO, we should only
> consider inclusion of this feature if we think it's safe, because I
> think all that is done here can be done from user-space.
>
Read only is safe.

> One last thing I was wondering. You said you want to keep existing
> partitioning unchanged, but I'd recommend to just drop the existing
> partitioning and have a single 121MB partition with UBI on it.
> This way you get support for UBI volumes, which is doing exactly what
> this FTL+partition-table is doing but in a standard/safe way.
> What is forcing you to keep the existing partitioning exactly?
>
Nothing is forcing me anymore after this patch!
People will do what they want: one big X11 distro or two little
console images, who knows...

In OpenEmbedded we do build tar.gz, jffs2, jffs2 sum, ubifs, ubi so
people have the choice.
Checkout some pictures: https://github.com/kexecboot/kexecboot/wiki/Screenshots

>> >
>> > 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>
>
> I'll comment on the FTL and partition-table format later, after I had
> time to review it properly.
>
> In the meantime, I'd like other MTD maintainers to comment on my
> reply. Maybe I'm the only one to think that supporting
> old/unmaintainerd FTLs is a bad idea, and in this case I'll have to
> accept the situation ;-).
>
> Regards,
>
> Boris
Boris,

I am sorry you feel cornered.
I think there is a misunderstanding here: I need a partition parser, not a FTL.

Regards
Andrea
Boris Brezillon April 18, 2017, 9:35 a.m.
Hi Andrea,

On Tue, 18 Apr 2017 10:58:02 +0200
Andrea Adami <andrea.adami@gmail.com> wrote:

> Boris,
> thanks for having read it.
> 
> On Mon, Apr 17, 2017 at 4:44 PM, Boris Brezillon <
> boris.brezillon@free-electrons.com> wrote:
> > Marek, Andrea,
> >
> > Before we even start discussing minor improvements (like coding style),
> > I'd like to discuss the sharp FTL and partition table format, and
> > decide whether we want to have such an old FTL included in the kernel.
> >
> > Actually, that's the very reason I asked Andrea to post his series as
> > soon as possible even if some things were not perfect.
> >
> > I'll try to document myself on the on-flash format of the FTL and
> > partition table before giving a definitive opinion, but I have the
> > feeling this ancient FTL is not 100% safe, and by accepting an old
> > (maybe unreliable?) FTL we are setting a precedent, and refusing other
> > (broken) proprietary/vendor FTLs will be almost impossible after that.
> >
> > Maybe I'm wrong, and this FTL is really safe and can be used on other
> > devices, that's why I'd like to understand how it works before giving
> > my opinion.
> >  
> 
> I can't judge the work done by ARM/Sharp/Linaro 15 years ago...
> I have only seen this on this PXA Sharp Zaurus SL series.
> 
> For the records, these are the GPL 2.4 sources
> https://github.com/LinuxPDA/Sharp_FTL_2.4.20

That's exactly why I said someone should review it, and I'm not talking
about the code itself, but the FTL+partition-table format.

> 
> >
> > On Sun, 16 Apr 2017 18:07:47 +0200
> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >  
> >> On 04/15/2017 10:11 PM, 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 )  
> >
> >
> > Okay, IMO that's not really a good argument to support this partition
> > parser. As Richard and I already mentioned several times, if your
> > bootloader is not capable of passing valid mtdparts= you can hardcode
> > it in the kernel using a default CMDLINE.
> >  
> We are simply trying to restore the ability for the kernel to read the
> mtdparts.
> 
> There are around many kernels and distros (OpenEmbedded, Arch, older 2.x
> stuff).
> Some do require to repartition: how can I know how other people partitioned?

By using a 2nd stage bootloader, as you did.

> 
> > Now, I understand that you want to support multiple devices with the
> > same kernel, and having this partition parser would simplify your job.
> > I also know you are developing a 2nd stage bootloader (based on
> > kexec+a minimal initramfs) to address the limitations of the
> > non-upgradable 1st stage bootloader.
> >
> > According to the rest of the description, you already have user-space
> > tools to manipulate the partition-table and those are aware of the FTL
> > layer, so I think you have all the basic blocks to get rid of this
> > in-kernel implementation.
> >
> > All you need is a way to extract the partitions from the 2nd stage
> > bootloader (using some tools you have in your initramfs) and build the
> > according mtdparts= parameter to pass to kexec when booting the real
> > kernel (the one used by the system).
> >  
> 
> We have patched kexecboot long ago to do that.
> https://github.com/kexecboot/kexecboot/blob/master/machine/zaurus.c

Okay, so that's what I initially understood. You already have a fully
functional user-space solution.

> 
> This is done with a special kernel (linux-kexecboot) embedding the minimal
> cpio and acting as 2nd stage bootloader.
> It passes the mtdparts found in cmdline and does the extra trick of reading
> it from mtd1 for zaurus.
> You can even customize the cmdline in /boot/boot.cfg and hack the mtdparts
> there.
> 
> Neverthless, what you don't seem to understand is that I cannot force
> people to use kexecboot or to customize cmdline parts as I like...
> I do just build kernels and images for testing...I maintain the OE build
> infrastructure, not one distro.

Well, you can say "if you want to use a mainline kernel, stop using
this sharp FTL+partition-table and start using a 2nd stage
bootloader like kexecboot". What do they use right now to boot a new
kernel (newer than the 2.4 one)?

> 
> > You tried to explain why it was not doable, but I still don't see
> > why.
> > You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS
> > enabled and that some people were booting distro kernels. Honestly, I
> > think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those
> > distro kernels than having your custom FTL enabled.
> > Then you tried to explain that with the user-space only solution you
> > wouldn't be able to support systems where the user had resized the
> > partitions with the nandlogical tool, and I still don't see why. Maybe
> > you can give more details to explain why this is impossible.  
> 
> I don't understand why people should get crazy with the different mtdparts=
> for each model.

So you agree that passing partition info through the cmdline is a good
solution?

> IMHO we are restoring a basic functionality, anything weird.

Are you talking about sharp FTL+part-table or the cmdline mtdparts=
parameter?

> 
> >
> > Just going through all these details to say that, IMO, we should only
> > consider inclusion of this feature if we think it's safe, because I
> > think all that is done here can be done from user-space.
> >  
> Read only is safe.
> 

This is a lie. AFAIU, you have all the necessary tools to update the
partition table from user-space, so even if you only have read-only
support in the kernel, one can corrupt it from userspace, and the
kernel may not be able to recover from this corruption.

Honestly, if we want to support this FTL+partition-table-format in the
kernel, I'd recommend that we add RW support, otherwise you'll keep
having those external tools.
Andrea Adami April 18, 2017, 11:50 a.m.
On Tue, Apr 18, 2017 at 11:35 AM, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> Hi Andrea,
>
> On Tue, 18 Apr 2017 10:58:02 +0200
> Andrea Adami <andrea.adami@gmail.com> wrote:
>
>> Boris,
>> thanks for having read it.
>>
>> On Mon, Apr 17, 2017 at 4:44 PM, Boris Brezillon <
>> boris.brezillon@free-electrons.com> wrote:
>> > Marek, Andrea,
>> >
>> > Before we even start discussing minor improvements (like coding style),
>> > I'd like to discuss the sharp FTL and partition table format, and
>> > decide whether we want to have such an old FTL included in the kernel.
>> >
>> > Actually, that's the very reason I asked Andrea to post his series as
>> > soon as possible even if some things were not perfect.
>> >
>> > I'll try to document myself on the on-flash format of the FTL and
>> > partition table before giving a definitive opinion, but I have the
>> > feeling this ancient FTL is not 100% safe, and by accepting an old
>> > (maybe unreliable?) FTL we are setting a precedent, and refusing other
>> > (broken) proprietary/vendor FTLs will be almost impossible after that.
>> >
>> > Maybe I'm wrong, and this FTL is really safe and can be used on other
>> > devices, that's why I'd like to understand how it works before giving
>> > my opinion.
>> >
>>
>> I can't judge the work done by ARM/Sharp/Linaro 15 years ago...
>> I have only seen this on this PXA Sharp Zaurus SL series.
>>
>> For the records, these are the GPL 2.4 sources
>> https://github.com/LinuxPDA/Sharp_FTL_2.4.20
>
> That's exactly why I said someone should review it, and I'm not talking
> about the code itself, but the FTL+partition-table format.
>
>>
>> >
>> > On Sun, 16 Apr 2017 18:07:47 +0200
>> > Marek Vasut <marek.vasut@gmail.com> wrote:
>> >
>> >> On 04/15/2017 10:11 PM, 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 )
>> >
>> >
>> > Okay, IMO that's not really a good argument to support this partition
>> > parser. As Richard and I already mentioned several times, if your
>> > bootloader is not capable of passing valid mtdparts= you can hardcode
>> > it in the kernel using a default CMDLINE.
>> >
>> We are simply trying to restore the ability for the kernel to read the
>> mtdparts.
>>
>> There are around many kernels and distros (OpenEmbedded, Arch, older 2.x
>> stuff).
>> Some do require to repartition: how can I know how other people partitioned?
>
> By using a 2nd stage bootloader, as you did.
>
>>
>> > Now, I understand that you want to support multiple devices with the
>> > same kernel, and having this partition parser would simplify your job.
>> > I also know you are developing a 2nd stage bootloader (based on
>> > kexec+a minimal initramfs) to address the limitations of the
>> > non-upgradable 1st stage bootloader.
>> >
>> > According to the rest of the description, you already have user-space
>> > tools to manipulate the partition-table and those are aware of the FTL
>> > layer, so I think you have all the basic blocks to get rid of this
>> > in-kernel implementation.
>> >
>> > All you need is a way to extract the partitions from the 2nd stage
>> > bootloader (using some tools you have in your initramfs) and build the
>> > according mtdparts= parameter to pass to kexec when booting the real
>> > kernel (the one used by the system).
>> >
>>
>> We have patched kexecboot long ago to do that.
>> https://github.com/kexecboot/kexecboot/blob/master/machine/zaurus.c
>
> Okay, so that's what I initially understood. You already have a fully
> functional user-space solution.
>
>>
>> This is done with a special kernel (linux-kexecboot) embedding the minimal
>> cpio and acting as 2nd stage bootloader.
>> It passes the mtdparts found in cmdline and does the extra trick of reading
>> it from mtd1 for zaurus.
>> You can even customize the cmdline in /boot/boot.cfg and hack the mtdparts
>> there.
>>
>> Neverthless, what you don't seem to understand is that I cannot force
>> people to use kexecboot or to customize cmdline parts as I like...
>> I do just build kernels and images for testing...I maintain the OE build
>> infrastructure, not one distro.
>
> Well, you can say "if you want to use a mainline kernel, stop using
> this sharp FTL+partition-table and start using a 2nd stage
> bootloader like kexecboot". What do they use right now to boot a new
> kernel (newer than the 2.4 one)?
>
No, because the proper way to reflash the unit is using the provided
infrastructure provided by Sharp: an updatewr.sh, a second kernel with
its initrd, containing the tools.
If one wants, he can just flash his kernel in mtd1 without kexecboot
and save space in the mtd partition..

>>
>> > You tried to explain why it was not doable, but I still don't see
>> > why.
>> > You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS
>> > enabled and that some people were booting distro kernels. Honestly, I
>> > think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those
>> > distro kernels than having your custom FTL enabled.
>> > Then you tried to explain that with the user-space only solution you
>> > wouldn't be able to support systems where the user had resized the
>> > partitions with the nandlogical tool, and I still don't see why. Maybe
>> > you can give more details to explain why this is impossible.
>>
Th eprqactical problem we got is that for example SL-C3200 needs a
different partitioning in spitz.c in linux-kexecboot,
otherwise kexecboot cannot detect correctly the mtd partitions nor
pass the correct mtdparts=.

>> I don't understand why people should get crazy with the different mtdparts=
>> for each model.
>
> So you agree that passing partition info through the cmdline is a good
> solution?

No. What does make you think that? We didn't have alternatives so we did that.

>
>> IMHO we are restoring a basic functionality, anything weird.
>
> Are you talking about sharp FTL+part-table or the cmdline mtdparts=
> parameter?
>
Talking about the kernel knowing the mtdparts used by the (original)
bootloader and by the maintenance kernel/utils put in nand by Sharp.
CONFIG_MTD_CMDLINE_PARTS is just a workaround in this case: the real
tables are written, we just have to read these as done by Angel bl.

>>
>> >
>> > Just going through all these details to say that, IMO, we should only
>> > consider inclusion of this feature if we think it's safe, because I
>> > think all that is done here can be done from user-space.
>> >
>> Read only is safe.
>>
>
> This is a lie. AFAIU, you have all the necessary tools to update the
> partition table from user-space, so even if you only have read-only
> support in the kernel, one can corrupt it from userspace, and the
> kernel may not be able to recover from this corruption.
>

Why do we care about userland here?
These units can always be factory-restored, they just have to d/l the
full image.


> Honestly, if we want to support this FTL+partition-table-format in the
> kernel, I'd recommend that we add RW support, otherwise you'll keep
> having those external tools.

These tools are not meant for normal use.
The utility for repartitoning is since long included in the installers
so users really ignore the details.

If you think, I can try to readd the Write support but it is a bit
pointless for the purposes of the parser.

Regards
Andrea
Boris Brezillon April 18, 2017, 12:32 p.m.
Hi Andrea,

You know what, I give up, since even if I review and find a problem in
the FTL/partition-table format/approach, you'll keep arguing that it
should be supported in the kernel. I think I have enough things on my
plate to not spend extra time on this.

I'll let others review and take the final decision and won't interfere
in the process.

Regards,

Boris

On Tue, 18 Apr 2017 13:50:03 +0200
Andrea Adami <andrea.adami@gmail.com> wrote:

> On Tue, Apr 18, 2017 at 11:35 AM, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > Hi Andrea,
> >
> > On Tue, 18 Apr 2017 10:58:02 +0200
> > Andrea Adami <andrea.adami@gmail.com> wrote:
> >  
> >> Boris,
> >> thanks for having read it.
> >>
> >> On Mon, Apr 17, 2017 at 4:44 PM, Boris Brezillon <
> >> boris.brezillon@free-electrons.com> wrote:  
> >> > Marek, Andrea,
> >> >
> >> > Before we even start discussing minor improvements (like coding style),
> >> > I'd like to discuss the sharp FTL and partition table format, and
> >> > decide whether we want to have such an old FTL included in the kernel.
> >> >
> >> > Actually, that's the very reason I asked Andrea to post his series as
> >> > soon as possible even if some things were not perfect.
> >> >
> >> > I'll try to document myself on the on-flash format of the FTL and
> >> > partition table before giving a definitive opinion, but I have the
> >> > feeling this ancient FTL is not 100% safe, and by accepting an old
> >> > (maybe unreliable?) FTL we are setting a precedent, and refusing other
> >> > (broken) proprietary/vendor FTLs will be almost impossible after that.
> >> >
> >> > Maybe I'm wrong, and this FTL is really safe and can be used on other
> >> > devices, that's why I'd like to understand how it works before giving
> >> > my opinion.
> >> >  
> >>
> >> I can't judge the work done by ARM/Sharp/Linaro 15 years ago...
> >> I have only seen this on this PXA Sharp Zaurus SL series.
> >>
> >> For the records, these are the GPL 2.4 sources
> >> https://github.com/LinuxPDA/Sharp_FTL_2.4.20  
> >
> > That's exactly why I said someone should review it, and I'm not talking
> > about the code itself, but the FTL+partition-table format.
> >  
> >>  
> >> >
> >> > On Sun, 16 Apr 2017 18:07:47 +0200
> >> > Marek Vasut <marek.vasut@gmail.com> wrote:
> >> >  
> >> >> On 04/15/2017 10:11 PM, 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 )  
> >> >
> >> >
> >> > Okay, IMO that's not really a good argument to support this partition
> >> > parser. As Richard and I already mentioned several times, if your
> >> > bootloader is not capable of passing valid mtdparts= you can hardcode
> >> > it in the kernel using a default CMDLINE.
> >> >  
> >> We are simply trying to restore the ability for the kernel to read the
> >> mtdparts.
> >>
> >> There are around many kernels and distros (OpenEmbedded, Arch, older 2.x
> >> stuff).
> >> Some do require to repartition: how can I know how other people partitioned?  
> >
> > By using a 2nd stage bootloader, as you did.
> >  
> >>  
> >> > Now, I understand that you want to support multiple devices with the
> >> > same kernel, and having this partition parser would simplify your job.
> >> > I also know you are developing a 2nd stage bootloader (based on
> >> > kexec+a minimal initramfs) to address the limitations of the
> >> > non-upgradable 1st stage bootloader.
> >> >
> >> > According to the rest of the description, you already have user-space
> >> > tools to manipulate the partition-table and those are aware of the FTL
> >> > layer, so I think you have all the basic blocks to get rid of this
> >> > in-kernel implementation.
> >> >
> >> > All you need is a way to extract the partitions from the 2nd stage
> >> > bootloader (using some tools you have in your initramfs) and build the
> >> > according mtdparts= parameter to pass to kexec when booting the real
> >> > kernel (the one used by the system).
> >> >  
> >>
> >> We have patched kexecboot long ago to do that.
> >> https://github.com/kexecboot/kexecboot/blob/master/machine/zaurus.c  
> >
> > Okay, so that's what I initially understood. You already have a fully
> > functional user-space solution.
> >  
> >>
> >> This is done with a special kernel (linux-kexecboot) embedding the minimal
> >> cpio and acting as 2nd stage bootloader.
> >> It passes the mtdparts found in cmdline and does the extra trick of reading
> >> it from mtd1 for zaurus.
> >> You can even customize the cmdline in /boot/boot.cfg and hack the mtdparts
> >> there.
> >>
> >> Neverthless, what you don't seem to understand is that I cannot force
> >> people to use kexecboot or to customize cmdline parts as I like...
> >> I do just build kernels and images for testing...I maintain the OE build
> >> infrastructure, not one distro.  
> >
> > Well, you can say "if you want to use a mainline kernel, stop using
> > this sharp FTL+partition-table and start using a 2nd stage
> > bootloader like kexecboot". What do they use right now to boot a new
> > kernel (newer than the 2.4 one)?
> >  
> No, because the proper way to reflash the unit is using the provided
> infrastructure provided by Sharp: an updatewr.sh, a second kernel with
> its initrd, containing the tools.
> If one wants, he can just flash his kernel in mtd1 without kexecboot
> and save space in the mtd partition..
> 
> >>  
> >> > You tried to explain why it was not doable, but I still don't see
> >> > why.
> >> > You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS
> >> > enabled and that some people were booting distro kernels. Honestly, I
> >> > think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those
> >> > distro kernels than having your custom FTL enabled.
> >> > Then you tried to explain that with the user-space only solution you
> >> > wouldn't be able to support systems where the user had resized the
> >> > partitions with the nandlogical tool, and I still don't see why. Maybe
> >> > you can give more details to explain why this is impossible.  
> >>  
> Th eprqactical problem we got is that for example SL-C3200 needs a
> different partitioning in spitz.c in linux-kexecboot,
> otherwise kexecboot cannot detect correctly the mtd partitions nor
> pass the correct mtdparts=.
> 
> >> I don't understand why people should get crazy with the different mtdparts=
> >> for each model.  
> >
> > So you agree that passing partition info through the cmdline is a good
> > solution?  
> 
> No. What does make you think that? We didn't have alternatives so we did that.
> 
> >  
> >> IMHO we are restoring a basic functionality, anything weird.  
> >
> > Are you talking about sharp FTL+part-table or the cmdline mtdparts=
> > parameter?
> >  
> Talking about the kernel knowing the mtdparts used by the (original)
> bootloader and by the maintenance kernel/utils put in nand by Sharp.
> CONFIG_MTD_CMDLINE_PARTS is just a workaround in this case: the real
> tables are written, we just have to read these as done by Angel bl.
> 
> >>  
> >> >
> >> > Just going through all these details to say that, IMO, we should only
> >> > consider inclusion of this feature if we think it's safe, because I
> >> > think all that is done here can be done from user-space.
> >> >  
> >> Read only is safe.
> >>  
> >
> > This is a lie. AFAIU, you have all the necessary tools to update the
> > partition table from user-space, so even if you only have read-only
> > support in the kernel, one can corrupt it from userspace, and the
> > kernel may not be able to recover from this corruption.
> >  
> 
> Why do we care about userland here?
> These units can always be factory-restored, they just have to d/l the
> full image.
> 
> 
> > Honestly, if we want to support this FTL+partition-table-format in the
> > kernel, I'd recommend that we add RW support, otherwise you'll keep
> > having those external tools.  
> 
> These tools are not meant for normal use.
> The utility for repartitoning is since long included in the installers
> so users really ignore the details.
> 
> If you think, I can try to readd the Write support but it is a bit
> pointless for the purposes of the parser.
> 
> Regards
> Andrea
Dmitry Eremin-Solenikov April 18, 2017, 3:26 p.m.
2017-04-17 17:44 GMT+03:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Marek, Andrea,
>
> Before we even start discussing minor improvements (like coding style),
> I'd like to discuss the sharp FTL and partition table format, and
> decide whether we want to have such an old FTL included in the kernel.
>
> Actually, that's the very reason I asked Andrea to post his series as
> soon as possible even if some things were not perfect.
>
> I'll try to document myself on the on-flash format of the FTL and
> partition table before giving a definitive opinion, but I have the
> feeling this ancient FTL is not 100% safe, and by accepting an old
> (maybe unreliable?) FTL we are setting a precedent, and refusing other
> (broken) proprietary/vendor FTLs will be almost impossible after that.
>
> Maybe I'm wrong, and this FTL is really safe and can be used on other
> devices, that's why I'd like to understand how it works before giving
> my opinion.
>
>
> On Sun, 16 Apr 2017 18:07:47 +0200
> Marek Vasut <marek.vasut@gmail.com> wrote:
>
>> On 04/15/2017 10:11 PM, 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 )
>
>
> Okay, IMO that's not really a good argument to support this partition
> parser. As Richard and I already mentioned several times, if your
> bootloader is not capable of passing valid mtdparts= you can hardcode
> it in the kernel using a default CMDLINE.
>
> Now, I understand that you want to support multiple devices with the
> same kernel, and having this partition parser would simplify your job.
> I also know you are developing a 2nd stage bootloader (based on
> kexec+a minimal initramfs) to address the limitations of the
> non-upgradable 1st stage bootloader.
>
> According to the rest of the description, you already have user-space
> tools to manipulate the partition-table and those are aware of the FTL
> layer, so I think you have all the basic blocks to get rid of this
> in-kernel implementation.
>
> All you need is a way to extract the partitions from the 2nd stage
> bootloader (using some tools you have in your initramfs) and build the
> according mtdparts= parameter to pass to kexec when booting the real
> kernel (the one used by the system).
>
> You tried to explain why it was not doable, but I still don't see
> why.
> You first said that not all systems had CONFIG_MTD_CMDLINE_PARTS
> enabled and that some people were booting distro kernels. Honestly, I
> think you have more chances to have CONFIG_MTD_CMDLINE_PARTS in those
> distro kernels than having your custom FTL enabled.
> Then you tried to explain that with the user-space only solution you
> wouldn't be able to support systems where the user had resized the
> partitions with the nandlogical tool, and I still don't see why. Maybe
> you can give more details to explain why this is impossible.
>
> Just going through all these details to say that, IMO, we should only
> consider inclusion of this feature if we think it's safe, because I
> think all that is done here can be done from user-space.

Not everybody is using kexecboot preloader. Asking users to depend on
the exact bootloader is not viable solution in my opinion.

>
> One last thing I was wondering. You said you want to keep existing
> partitioning unchanged, but I'd recommend to just drop the existing
> partitioning and have a single 121MB partition with UBI on it.
> This way you get support for UBI volumes, which is doing exactly what
> this FTL+partition-table is doing but in a standard/safe way.
> What is forcing you to keep the existing partitioning exactly?

Mostly compatibility with pre-flashed recovery kernel and with older kernels/
setups. Yes, flash on those PDAs contains secondary kernel+rootfs which
are used to reflash the main kernel (=kexecboot in some cases) and filesystems.

Just a short notice: we are not asking to add a full support for FTL,
read/write, etc.
We are asking for a small enough support that is necessary to actually read
partition table. After that MTD partitions are accessed w/o any FTL.

And unfortunately we are stuck with old FTL/partition table format, since
nobody wants to replace existing Sharp bootloader.

>> >
>> > 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>
>
> I'll comment on the FTL and partition-table format later, after I had
> time to review it properly.
>
> In the meantime, I'd like other MTD maintainers to comment on my
> reply. Maybe I'm the only one to think that supporting
> old/unmaintainerd FTLs is a bad idea, and in this case I'll have to
> accept the situation ;-).
>
> Regards,
>
> Boris

Patch hide | download patch | download mbox

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