On Saturday 14 March 2009, [email protected] wrote:
> From: Sneha Narnakaje <[email protected]>
>
> This patch has minor changes to the core MTD NAND
> read_page to pass the "page" argument.
This is another patch that would need more public
discussion, since it affects MTD core and all users
thereof ... so, send to linux-mtd and LKML.
Some bugs, sufficient to get a NAK from this version
of the patch: this will break the build for several
NAND drivers that already override those methods.
When you change such a programming interface, make
sure the same patch changes all of its mainline users
at the same time. (Out-of-tree code must catch up
without your help.)
> The DaVinci
> NAND driver overrides these APIs for DM355 and
> require the "page" to be passed for reading the
> whole OOB area first and then data in chunks of 512bytes.
I predict that won't be a popular argument. As in,
at least five other drivers (in mainline, ignoring any
that haven't yet merged) now override those calls, but
haven't needed to change the call syntax ... so why
should this hardware imply a change?
Maybe it should. But if so, you'll need to justify it.
I'd point out that the 4-bit ECC isn't specific to the
dm355; it's shared with other newish hardware. And that
no existing NAND_ECC_HW_SYNDROME driver seems to have
been used with more than one step per page, so finding
concept-level bugs in that code is no surprise at all.
I *think* your rationale here must be something like:
We don't want to use the "infix OOB" scheme
currently implied by the syndrome ECC mode
support, since that
- overwrites manufacturer bad-block markings,
which may lose that very important data;
- complicates the jobs of non-Linux NAND
programmers (in development, manufacturing);
- implies ECC protection for the "prepad"
bytes, so Linux raw_write ops will fail.
Accordingly, we want to ... (text above about
reading OOB before data, so ECC codes can be
used incrementally during reads).
Given that ... I wonder if a better solution wouldn't
just involve defining a new ECC mode, so the read and
write methods (from patch #3 of this set) need not be
specific to the DaVinci NAND driver...
In fact, if you pitch that question properly, I suspect
that you'll get grumbling but agreement that the current
NAND_ECC_HW_SYNDROME stuff has significant-enough issues
to merit changes. Or maybe not; it might be best to just
write a patch for a new ECC mode, and not expect much
discussion on that topic.
> Signed-off-by: Sandeep Paulraj <[email protected]>
> Signed-off-by: Sneha Narnakaje <[email protected]>
> ---
> drivers/mtd/nand/nand_base.c | 16 +++++++++-------
> include/linux/mtd/nand.h | 4 ++--
> 2 files changed, 11 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index 599185c..b3682b3 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -750,7 +750,7 @@ static int nand_wait(struct mtd_info *mtd, struct
> nand_chip *chip)
> * @buf: buffer to store read data
> */
> static int nand_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> - uint8_t *buf)
> + uint8_t *buf, int page)
> {
> chip->read_buf(mtd, buf, mtd->writesize);
> chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> @@ -764,7 +764,7 @@ static int nand_read_page_raw(struct mtd_info *mtd,
> struct nand_chip *chip,
> * @buf: buffer to store read data
> */
> static int nand_read_page_swecc(struct mtd_info *mtd, struct nand_chip *chip,
> - uint8_t *buf)
> + uint8_t *buf, int page)
> {
> int i, eccsize = chip->ecc.size;
> int eccbytes = chip->ecc.bytes;
> @@ -774,7 +774,7 @@ static int nand_read_page_swecc(struct mtd_info *mtd,
> struct nand_chip *chip,
> uint8_t *ecc_code = chip->buffers->ecccode;
> uint32_t *eccpos = chip->ecc.layout->eccpos;
>
> - chip->ecc.read_page_raw(mtd, chip, buf);
> + chip->ecc.read_page_raw(mtd, chip, buf, page);
>
> for (i = 0; eccsteps; eccsteps--, i += eccbytes, p += eccsize)
> chip->ecc.calculate(mtd, p, &ecc_calc[i]);
> @@ -887,7 +887,7 @@ static int nand_read_subpage(struct mtd_info *mtd, struct
> nand_chip *chip, uint3
> * Not for syndrome calculating ecc controllers which need a special oob
> layout
> */
> static int nand_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip,
> - uint8_t *buf)
> + uint8_t *buf, int page)
> {
> int i, eccsize = chip->ecc.size;
> int eccbytes = chip->ecc.bytes;
> @@ -932,7 +932,7 @@ static int nand_read_page_hwecc(struct mtd_info *mtd,
> struct nand_chip *chip,
> * we need a special oob layout and handling.
> */
> static int nand_read_page_syndrome(struct mtd_info *mtd, struct nand_chip
> *chip,
> - uint8_t *buf)
> + uint8_t *buf, int page)
> {
> int i, eccsize = chip->ecc.size;
> int eccbytes = chip->ecc.bytes;
> @@ -1074,11 +1074,13 @@ static int nand_do_read_ops(struct mtd_info *mtd,
> loff_t from,
>
> /* Now read the page into the buffer */
> if (unlikely(ops->mode == MTD_OOB_RAW))
> - ret = chip->ecc.read_page_raw(mtd, chip,
> bufpoi);
> + ret = chip->ecc.read_page_raw(mtd, chip,
> + bufpoi, page);
> else if (!aligned && NAND_SUBPAGE_READ(chip) && !oob)
> ret = chip->ecc.read_subpage(mtd, chip, col,
> bytes, bufpoi);
> else
> - ret = chip->ecc.read_page(mtd, chip, bufpoi);
> + ret = chip->ecc.read_page(mtd, chip, bufpoi,
> + page);
> if (ret < 0)
> break;
>
> diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> index b2bed22..63ed0eb 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -278,13 +278,13 @@ struct nand_ecc_ctrl {
> uint8_t *calc_ecc);
> int (*read_page_raw)(struct mtd_info *mtd,
> struct nand_chip *chip,
> - uint8_t *buf);
> + uint8_t *buf, int page);
> void (*write_page_raw)(struct mtd_info *mtd,
> struct nand_chip *chip,
> const uint8_t *buf);
> int (*read_page)(struct mtd_info *mtd,
> struct nand_chip *chip,
> - uint8_t *buf);
> + uint8_t *buf, int page);
> int (*read_subpage)(struct mtd_info *mtd,
> struct nand_chip *chip,
> uint32_t offs, uint32_t len,
> --
> 1.6.0.4
>
_______________________________________________
Davinci-linux-open-source mailing list
[email protected]
http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
