[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20160512143650.686f2ab2@bbrezillon>
Date: Thu, 12 May 2016 14:36:50 +0200
From: Boris Brezillon <boris.brezillon@...e-electrons.com>
To: Richard Weinberger <richard@....at>
Cc: linux-mtd@...ts.infradead.org, linux-kernel@...r.kernel.org,
dwmw2@...radead.org, computersforpeace@...il.com
Subject: Re: [PATCH] nand: nandsim: Implement approximation mode
Hi Richard,
On Thu, 12 May 2016 13:32:45 +0200
Richard Weinberger <richard@....at> wrote:
> Sometimes all you need is a NAND with a given page, erase and chip size
> to load and inspect a certain image.
> OOB, ECC sizes and other metrics don't matter much then.
> In such a situation I find myself often fiddling around with NAND IDs
> to get what I need, especially when the given NAND ID from the datasheet
> decode to something else than the NAND advertises via ONFI.
> Using the new nandsim.approx module parameter it is possible to specify
> the page size, number of pages per block and the total number of blocks.
> Nandsim will then emulate a SLC NAND with 128 bytes OOB and the given
> sizes. Nandsim achieves this by providing ONFI parameters to NAND core.
The current nandsim implementation is indeed incomplete and does not
allow for the full NAND chain emulation.
I first considered dispatching the emulation bit in the different
components (the NAND controller, the NAND chip and the generic/standard
code), but that means introduction a new infrastructure and I'm now
unsure that this is appropriate.
The other question we should ask ourselves is whether this NAND
emulation layer belongs here. I mean, we could emulate a bunch of NAND
chips and NAND controller directly in Qemu instead of trying to teach
nandsim how to emulate advanced chips.
>
> Signed-off-by: Richard Weinberger <richard@....at>
> ---
> drivers/mtd/nand/nand_base.c | 12 +++++----
> drivers/mtd/nand/nandsim.c | 62 +++++++++++++++++++++++++++++++++++++++-----
> include/linux/mtd/nand.h | 3 +++
> 3 files changed, 66 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 557b846..25432c2 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -3208,7 +3208,7 @@ static void sanitize_string(uint8_t *s, size_t len)
> strim(s);
> }
>
> -static u16 onfi_crc16(u16 crc, u8 const *p, size_t len)
> +u16 nand_onfi_crc16(u16 crc, u8 const *p, size_t len)
> {
> int i;
> while (len--) {
> @@ -3246,7 +3246,7 @@ static int nand_flash_detect_ext_param_page(struct mtd_info *mtd,
>
> /* Read out the Extended Parameter Page. */
> chip->read_buf(mtd, (uint8_t *)ep, len);
> - if ((onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2)
> + if ((nand_onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2)
> != le16_to_cpu(ep->crc))) {
> pr_debug("fail in the CRC.\n");
> goto ext_out;
> @@ -3328,14 +3328,16 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip,
> /* Try ONFI for unknown chip or LP */
> chip->cmdfunc(mtd, NAND_CMD_READID, 0x20, -1);
> if (chip->read_byte(mtd) != 'O' || chip->read_byte(mtd) != 'N' ||
> - chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I')
> + chip->read_byte(mtd) != 'F' || chip->read_byte(mtd) != 'I') {
> +
> return 0;
> + }
Hm, this change seems unnecessary.
>
> chip->cmdfunc(mtd, NAND_CMD_PARAM, 0, -1);
> for (i = 0; i < 3; i++) {
> for (j = 0; j < sizeof(*p); j++)
> ((uint8_t *)p)[j] = chip->read_byte(mtd);
> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> + if (nand_onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 254) ==
> le16_to_cpu(p->crc)) {
> break;
> }
> @@ -3442,7 +3444,7 @@ static int nand_flash_detect_jedec(struct mtd_info *mtd, struct nand_chip *chip,
> for (j = 0; j < sizeof(*p); j++)
> ((uint8_t *)p)[j] = chip->read_byte(mtd);
>
> - if (onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> + if (nand_onfi_crc16(ONFI_CRC_BASE, (uint8_t *)p, 510) ==
> le16_to_cpu(p->crc))
> break;
> }
> diff --git a/drivers/mtd/nand/nandsim.c b/drivers/mtd/nand/nandsim.c
> index a58169a2..b5be38a 100644
> --- a/drivers/mtd/nand/nandsim.c
> +++ b/drivers/mtd/nand/nandsim.c
> @@ -114,6 +114,15 @@ static u_char id_bytes[8] = {
> [3] = CONFIG_NANDSIM_FOURTH_ID_BYTE,
> [4 ... 7] = 0xFF,
> };
> +static unsigned int approx[3];
> +
> +static struct nand_onfi_params id_onfi = {
> + .sig = {'O', 'N', 'F', 'I'},
> + .manufacturer = "nandsim ",
> + .model = "nandsim ",
> +};
> +
> +static unsigned char *id_p;
>
> module_param_array(id_bytes, byte, NULL, 0400);
> module_param_named(first_id_byte, id_bytes[0], byte, 0400);
> @@ -139,6 +148,7 @@ module_param(overridesize, uint, 0400);
> module_param(cache_file, charp, 0400);
> module_param(bbt, uint, 0400);
> module_param(bch, uint, 0400);
> +module_param_array(approx, uint, NULL, 0400);
>
> MODULE_PARM_DESC(id_bytes, "The ID bytes returned by NAND Flash 'read ID' command");
> MODULE_PARM_DESC(first_id_byte, "The first byte returned by NAND Flash 'read ID' command (manufacturer ID) (obsolete)");
> @@ -174,6 +184,10 @@ MODULE_PARM_DESC(cache_file, "File to use to cache nand pages instead of mem
> MODULE_PARM_DESC(bbt, "0 OOB, 1 BBT with marker in OOB, 2 BBT with marker in data area");
> MODULE_PARM_DESC(bch, "Enable BCH ecc and set how many bits should "
> "be correctable in 512-byte blocks");
> +MODULE_PARM_DESC(approx, "Approximation mode, simulate a good enough NAND that satisfies"
> + " page size, block size and number of blocks."
> + " e.g. 4096,128,8192 will give you a NAND with 4KiB page size, 128 pages per"
> + " block and 8192 blocks in total.");
>
> /* The largest possible page size */
> #define NS_LARGEST_PAGE_SIZE 4096
> @@ -229,6 +243,7 @@ MODULE_PARM_DESC(bch, "Enable BCH ecc and set how many bits should "
> #define STATE_CMD_RESET 0x0000000C /* reset */
> #define STATE_CMD_RNDOUT 0x0000000D /* random output command */
> #define STATE_CMD_RNDOUTSTART 0x0000000E /* random output start command */
> +#define STATE_CMD_PARAM 0x0000000F /* param output start command */
> #define STATE_CMD_MASK 0x0000000F /* command states mask */
>
> /* After an address is input, the simulator goes to one of these states */
> @@ -245,7 +260,8 @@ MODULE_PARM_DESC(bch, "Enable BCH ecc and set how many bits should "
> #define STATE_DATAOUT 0x00001000 /* waiting for page data output */
> #define STATE_DATAOUT_ID 0x00002000 /* waiting for ID bytes output */
> #define STATE_DATAOUT_STATUS 0x00003000 /* waiting for status output */
> -#define STATE_DATAOUT_MASK 0x00007000 /* data output states mask */
> +#define STATE_DATAOUT_ONFI 0x00007000 /* waiting for ONFI output */
> +#define STATE_DATAOUT_MASK 0x0000F000 /* data output states mask */
>
> /* Previous operation is done, ready to accept new requests */
> #define STATE_READY 0x00000000
> @@ -307,7 +323,6 @@ struct nandsim {
> unsigned int nbparts;
>
> uint busw; /* flash chip bus width (8 or 16) */
> - u_char ids[8]; /* chip's ID bytes */
> uint32_t options; /* chip's characteristic bits */
> uint32_t state; /* current chip state */
> uint32_t nxstate; /* next expected state */
> @@ -408,6 +423,8 @@ static struct nandsim_operations {
> {OPT_ANY, {STATE_CMD_STATUS, STATE_DATAOUT_STATUS, STATE_READY}},
> /* Read ID */
> {OPT_ANY, {STATE_CMD_READID, STATE_ADDR_ZERO, STATE_DATAOUT_ID, STATE_READY}},
> + /* Read param */
> + {OPT_ANY, {STATE_CMD_PARAM, STATE_ADDR_ZERO, STATE_DATAOUT_ONFI, STATE_READY}},
> /* Large page devices read page */
> {OPT_LARGEPAGE, {STATE_CMD_READ0, STATE_ADDR_PAGE, STATE_CMD_READSTART | ACTION_CPY,
> STATE_DATAOUT, STATE_READY}},
> @@ -1077,6 +1094,8 @@ static char *get_state_name(uint32_t state)
> return "STATE_CMD_RNDOUT";
> case STATE_CMD_RNDOUTSTART:
> return "STATE_CMD_RNDOUTSTART";
> + case STATE_CMD_PARAM:
> + return "STATE_CMD_PARAM";
> case STATE_ADDR_PAGE:
> return "STATE_ADDR_PAGE";
> case STATE_ADDR_SEC:
> @@ -1125,6 +1144,7 @@ static int check_command(int cmd)
> case NAND_CMD_RESET:
> case NAND_CMD_RNDOUT:
> case NAND_CMD_RNDOUTSTART:
> + case NAND_CMD_PARAM:
> return 0;
>
> default:
> @@ -1164,6 +1184,8 @@ static uint32_t get_state_by_command(unsigned command)
> return STATE_CMD_RNDOUT;
> case NAND_CMD_RNDOUTSTART:
> return STATE_CMD_RNDOUTSTART;
> + case NAND_CMD_PARAM:
> + return STATE_CMD_PARAM;
> }
>
> NS_ERR("get_state_by_command: unknown command, BUG\n");
> @@ -1856,9 +1878,17 @@ static void switch_state(struct nandsim *ns)
> break;
>
> case STATE_DATAOUT_ID:
> - ns->regs.num = ns->geom.idbytes;
> + if (ns->regs.row == 0x20) {
> + ns->regs.num = sizeof(id_onfi.sig);
> + id_p = (unsigned char *)id_onfi.sig;
> + } else {
> + ns->regs.num = ns->geom.idbytes;
> + id_p = (unsigned char *)id_bytes;
> + }
> + break;
> + case STATE_DATAOUT_ONFI:
> + ns->regs.num = sizeof(id_onfi);
> break;
> -
> case STATE_DATAOUT_STATUS:
> ns->regs.count = ns->regs.num = 0;
> break;
> @@ -1951,7 +1981,11 @@ static u_char ns_nand_read_byte(struct mtd_info *mtd)
> break;
> case STATE_DATAOUT_ID:
> NS_DBG("read_byte: read ID byte %d, total = %d\n", ns->regs.count, ns->regs.num);
> - outb = ns->ids[ns->regs.count];
> + outb = id_p[ns->regs.count];
> + ns->regs.count += 1;
> + break;
> + case STATE_DATAOUT_ONFI:
> + outb = ((unsigned char *)&id_onfi)[ns->regs.count];
> ns->regs.count += 1;
> break;
> default:
> @@ -2289,10 +2323,26 @@ static int __init ns_init_module(void)
> nand->geom.idbytes = 4;
> else
> nand->geom.idbytes = 2;
> +
> + if (approx[0] && approx[1] && approx[2]) {
> + id_onfi.byte_per_page = cpu_to_le32(approx[0]);
> + id_onfi.pages_per_block = cpu_to_le32(approx[1]);
> + id_onfi.blocks_per_lun = cpu_to_le32(approx[2]);
> +
> + id_onfi.spare_bytes_per_page = cpu_to_le32(128);
> + id_onfi.lun_count = cpu_to_le32(1);
> + id_onfi.bits_per_cell = cpu_to_le32(1);
> + id_onfi.revision = cpu_to_le32(1 << 5);
> + id_onfi.crc = cpu_to_le16(nand_onfi_crc16(ONFI_CRC_BASE, (uint8_t *)&id_onfi,
> + sizeof(id_onfi) - sizeof(id_onfi.crc)));
> +
> + nand->geom.idbytes = 2;
> + id_bytes[0] = id_bytes[1] = 0xFF;
> + }
> +
> nand->regs.status = NS_STATUS_OK(nand);
> nand->nxstate = STATE_UNKNOWN;
> nand->options |= OPT_PAGE512; /* temporary value */
> - memcpy(nand->ids, id_bytes, sizeof(nand->ids));
> if (bus_width == 16) {
> nand->busw = 16;
> chip->options |= NAND_BUSWIDTH_16;
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index 56574ba..82f6db2 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -50,6 +50,9 @@ extern int nand_lock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
> /* unlocks specified locked blocks */
> extern int nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len);
>
> +/* calculate ONFI CRC */
> +extern u16 nand_onfi_crc16(u16 crc, u8 const *p, size_t len);
> +
> /* The maximum number of NAND chips in an array */
> #define NAND_MAX_CHIPS 8
>
Just a general feedback on the whole approach. If what you really want
is a solution to skip the chip detection and manually set the chip
info, then you shouldn't call nand_scan_ident() and manually fill
the mtd and nand_chip fields instead.
Regards,
Boris
--
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Powered by blists - more mailing lists