From: Miquel Raynal <miquel.raynal@bootlin.com>
To: shiva.linuxworks@gmail.com
Cc: Chuanhong Guo <gch981213@gmail.com>,
Vignesh Raghavendra <vigneshr@ti.com>,
Boris Brezillon <bbrezillon@kernel.org>,
Marcel Ziswiler <marcel.ziswiler@toradex.com>,
Richard Weinberger <richard@nod.at>,
linux-kernel@vger.kernel.org,
Frieder Schrempf <frieder.schrempf@kontron.de>,
liaoweixiong <liaoweixiong@allwinnertech.com>,
Marek Vasut <marek.vasut@gmail.com>,
linux-mtd@lists.infradead.org,
Jeff Kletsky <git-commits@allycomm.com>,
Brian Norris <computersforpeace@gmail.com>,
David Woodhouse <dwmw2@infradead.org>,
Shivamurthy Shastri <sshivamurthy@micron.com>
Subject: Re: [PATCH 4/8] mtd: spinand: enabled parameter page support
Date: Wed, 7 Aug 2019 11:48:55 +0200 [thread overview]
Message-ID: <20190807114855.35f26229@xps13> (raw)
In-Reply-To: <20190722055621.23526-5-sshivamurthy@micron.com>
Hi Shiva,
shiva.linuxworks@gmail.com wrote on Mon, 22 Jul 2019 07:56:17 +0200:
"mtd: spinand: enable parameter page support"
> From: Shivamurthy Shastri <sshivamurthy@micron.com>
>
> Some of the SPI NAND devices has parameter page, which is similar to
- have a
> ONFI table.
regular raw NAND ONFI tables.
>
> But, it may not be self sufficient to propagate all the required
As it may not be
> parameters. Fixup function has been added in struct manufacturer to
, a fixup is being added in the manufacturer structure
> accommodate this.
The fixup function sentence should be dropped from the commit message,
see below.
>
> Signed-off-by: Shivamurthy Shastri <sshivamurthy@micron.com>
> ---
> drivers/mtd/nand/spi/core.c | 134 ++++++++++++++++++++++++++++++++++++
> include/linux/mtd/spinand.h | 3 +
> 2 files changed, 137 insertions(+)
>
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 89f6beefb01c..7ae76dab9141 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -400,6 +400,131 @@ static int spinand_lock_block(struct spinand_device *spinand, u8 lock)
> return spinand_write_reg_op(spinand, REG_BLOCK_LOCK, lock);
> }
>
> +/**
> + * spinand_read_param_page_op - Read parameter page operation
Again, the name in the doc does not fit the function you describe
> + * @spinand: the spinand
SPI-NAND chip
Shiva, there are way too much typos and shortcuts in your series.
Please be more careful otherwise we can't focus on the technical
aspects. I am not a native English speaker at all but please, plain
English is not C code. We talk SPI-NAND and not spinand, we say
structure and not struct, acronyms are uppercase, etc.
> + * @page: page number where parameter page tables can be found
^ the
> + * @buf: buffer used to store the parameter page
> + * @len: length of the buffer
> + *
> + * Read parameter page
the
> + *
> + * Returns 0 on success, a negative error code otherwise.
> + */
> +static int spinand_parameter_page_read(struct spinand_device *spinand,
> + u8 page, void *buf, unsigned int len)
> +{
> + struct spi_mem_op pread_op = SPINAND_PAGE_READ_OP(page);
> + struct spi_mem_op pread_cache_op =
> + SPINAND_PAGE_READ_FROM_CACHE_OP(false,
> + 0,
> + 1,
> + buf,
> + len);
That's ok if you cross the 80 characters boundary here. You may put "0,
1," on the first line and "buf, len);" on the second.
> + u8 feature;
> + u8 status;
> + int ret;
> +
> + if (len && !buf)
> + return -EINVAL;
> +
> + ret = spinand_read_reg_op(spinand, REG_CFG,
> + &feature);
> + if (ret)
> + return ret;
> +
> + /* CFG_OTP_ENABLE is used to enable parameter page access */
> + feature |= CFG_OTP_ENABLE;
> +
> + spinand_write_reg_op(spinand, REG_CFG, feature);
> +
> + ret = spi_mem_exec_op(spinand->spimem, &pread_op);
> + if (ret)
> + return ret;
> +
> + ret = spinand_wait(spinand, &status);
> + if (ret < 0)
> + return ret;
> +
> + ret = spi_mem_exec_op(spinand->spimem, &pread_cache_op);
> + if (ret)
> + return ret;
> +
> + ret = spinand_read_reg_op(spinand, REG_CFG,
> + &feature);
> + if (ret)
> + return ret;
> +
> + feature &= ~CFG_OTP_ENABLE;
> +
> + spinand_write_reg_op(spinand, REG_CFG, feature);
> +
> + return 0;
> +}
> +
Add the kernel doc please
Change the below function so that it returns 1 if the page was
detected, 0 if it did not, an negative error code otherwise.
> +static int spinand_param_page_detect(struct spinand_device *spinand)
> +{
> + struct mtd_info *mtd = spinand_to_mtd(spinand);
> + struct nand_memory_organization *memorg;
> + struct nand_onfi_params *p;
> + struct nand_device *base = spinand_to_nand(spinand);
> + int i, ret;
> +
> + memorg = nanddev_get_memorg(base);
> +
> + /* Allocate buffer to hold parameter page */
> + p = kzalloc((sizeof(*p) * 3), GFP_KERNEL);
> + if (!p)
> + return -ENOMEM;
> +
> + ret = spinand_parameter_page_read(spinand, 0x01, p, sizeof(*p) * 3);
> + if (ret) {
> + ret = 0;
No, you should return the error in case of error. You will later handle
the fact that there is no parameter page.
> + goto free_param_page;
> + }
> +
> + for (i = 0; i < 3; i++) {
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)&p[i], 254) ==
^
If you force the parameter page to be 254 bytes long it means you limit
yourself to ONFI standard. That's not a problem, but then you should
mention it in the function name.
> + le16_to_cpu(p->crc)) {
> + if (i)
> + memcpy(p, &p[i], sizeof(*p));
> + break;
> + }
> + }
> +
> + if (i == 3) {
> + const void *srcbufs[3] = {p, p + 1, p + 2};
> +
> + pr_warn("Could not find a valid ONFI parameter page, trying bit-wise majority to recover it\n");
> + nand_bit_wise_majority(srcbufs, ARRAY_SIZE(srcbufs), p,
> + sizeof(*p));
> +
> + if (onfi_crc16(ONFI_CRC_BASE, (u8 *)p, 254) !=
> + le16_to_cpu(p->crc)) {
> + pr_err("ONFI parameter recovery failed, aborting\n");
> + goto free_param_page;
> + }
> + }
The whole for-loop and the if (i==3) condition is exactly the same as
for raw NANDs and must be extracted in a generic function:
1/ extract the function from nand/raw/nand_onfi.c and put it in
nand/onfi.c.
2/ then use it in this patch.
> +
> + parse_onfi_params(memorg, p);
> +
> + mtd->writesize = memorg->pagesize;
> + mtd->erasesize = memorg->pages_per_eraseblock * memorg->pagesize;
> + mtd->oobsize = memorg->oobsize;
This will be handled by nanddev_init, should be removed.
> +
> + /* Manufacturers may interpret the parameter page differently */
> + if (spinand->manufacturer->ops->fixup_param_page)
> + spinand->manufacturer->ops->fixup_param_page(spinand, p);
The whole "manufacturer fixup" should be done separately.
> +
> + /* Identification done, free the full parameter page and exit */
> + ret = 1;
> +
> +free_param_page:
> + kfree(p);
> +
> + return ret;
> +}
> +
> static int spinand_check_ecc_status(struct spinand_device *spinand, u8 status)
> {
> struct nand_device *nand = spinand_to_nand(spinand);
> @@ -911,6 +1036,15 @@ static int spinand_detect(struct spinand_device *spinand)
> return ret;
> }
>
> + if (!spinand->base.memorg.pagesize) {
> + ret = spinand_param_page_detect(spinand);
> + if (ret <= 0) {
> + dev_err(dev, "no parameter page for %*phN\n",
Not sure at this stage dev will give something meaningful. Anyway I
don't think we should scream at the user if his NAND is not an ONFI one
so please return an error only if ret < 0. If ret == 0 or ret == 1,
don't warn the user.
> + SPINAND_MAX_ID_LEN, spinand->id.data);
> + return -ENODEV;
> + }
> + }
> +
> if (nand->memorg.ntargets > 1 && !spinand->select_target) {
> dev_err(dev,
> "SPI NANDs with more than one die must implement ->select_target()\n");
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 4ea558bd3c46..fea820a20bc9 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -15,6 +15,7 @@
> #include <linux/mtd/nand.h>
> #include <linux/spi/spi.h>
> #include <linux/spi/spi-mem.h>
> +#include <linux/mtd/onfi.h>
>
> /**
> * Standard SPI NAND flash operations
> @@ -209,6 +210,8 @@ struct spinand_manufacturer_ops {
> int (*detect)(struct spinand_device *spinand);
> int (*init)(struct spinand_device *spinand);
> void (*cleanup)(struct spinand_device *spinand);
> + void (*fixup_param_page)(struct spinand_device *spinand,
> + struct nand_onfi_params *p);
Please do this in a separate patch.
> };
>
> /**
Thanks,
Miquèl
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2019-08-07 9:49 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-22 5:56 [PATCH 0/8] Introduce generic ONFI support shiva.linuxworks
2019-07-22 5:56 ` [PATCH 1/8] mtd: nand: move ONFI related functions to onfi.h shiva.linuxworks
2019-08-07 8:34 ` Miquel Raynal
2019-08-19 8:35 ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-07-22 5:56 ` [PATCH 2/8] mtd: nand: move support functions for ONFI to nand/onfi.c shiva.linuxworks
2019-08-07 9:03 ` Miquel Raynal
2019-08-19 8:36 ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-07-22 5:56 ` [PATCH 3/8] mtd: nand: create ONFI table parsing instance shiva.linuxworks
2019-08-07 9:09 ` Miquel Raynal
2019-07-22 5:56 ` [PATCH 4/8] mtd: spinand: enabled parameter page support shiva.linuxworks
2019-08-07 9:48 ` Miquel Raynal [this message]
2019-08-19 8:51 ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-08-19 9:21 ` Miquel Raynal
2019-09-30 13:23 ` Shivamurthy Shastri (sshivamurthy)
2019-10-07 7:52 ` Boris Brezillon
2019-07-22 5:56 ` [PATCH 5/8] mtd: spinand: micron: prepare for generalizing driver shiva.linuxworks
2019-08-07 9:51 ` Miquel Raynal
2019-07-22 5:56 ` [PATCH 6/8] mtd: spinand: micron: Turn driver implementation generic shiva.linuxworks
2019-08-07 10:04 ` Miquel Raynal
2019-08-19 9:03 ` [EXT] " Shivamurthy Shastri (sshivamurthy)
2019-08-19 9:19 ` Miquel Raynal
2019-09-16 10:41 ` Shivamurthy Shastri (sshivamurthy)
2019-10-07 8:13 ` Boris Brezillon
2019-10-14 12:49 ` Shivamurthy Shastri (sshivamurthy)
2019-07-22 5:56 ` [PATCH 7/8] mtd: spinand: micron: Fix read failure in Micron M70A flashes shiva.linuxworks
2019-08-07 10:05 ` Miquel Raynal
2019-07-22 5:56 ` [PATCH 8/8] mtd: spinand: micron: Enable micron flashes with multi-die shiva.linuxworks
2019-08-07 10:08 ` Miquel Raynal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190807114855.35f26229@xps13 \
--to=miquel.raynal@bootlin.com \
--cc=bbrezillon@kernel.org \
--cc=computersforpeace@gmail.com \
--cc=dwmw2@infradead.org \
--cc=frieder.schrempf@kontron.de \
--cc=gch981213@gmail.com \
--cc=git-commits@allycomm.com \
--cc=liaoweixiong@allwinnertech.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=marcel.ziswiler@toradex.com \
--cc=marek.vasut@gmail.com \
--cc=richard@nod.at \
--cc=shiva.linuxworks@gmail.com \
--cc=sshivamurthy@micron.com \
--cc=vigneshr@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).