lkml.org 
[lkml]   [2009]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH 1/3] [MTD] Flex-OneNAND support
On Tue, 03 Mar 2009 15:36:05 +0900
Rohit Hagargundgi <h.rohit@samsung.com> wrote:

> This is a repost of the Flex-OneNAND devices support.
> Changes since the last post:
> - Fix bug which caused 2X program in OneNAND to fail.
> - Fix bug with eraseregions containing odd number of blocks.
> - Add routine to check blocks are erased before changing boundary.

"changes since last post" is not interesting information in the
mainline git commit, so I habitually delete it.

After that, we end up without any useful changelog at all. Is that
really optimal?

> ---
> drivers/mtd/onenand/Kconfig | 62 +++
> drivers/mtd/onenand/onenand_base.c | 827 ++++++++++++++++++++++++++++++++----
> drivers/mtd/onenand/onenand_bbt.c | 13 +-
> drivers/mtd/onenand/onenand_sim.c | 76 +++-
> include/linux/mtd/onenand.h | 41 ++
> include/linux/mtd/onenand_regs.h | 20 +-
> 6 files changed, 950 insertions(+), 89 deletions(-)
>
> diff --git a/drivers/mtd/onenand/Kconfig b/drivers/mtd/onenand/Kconfig
> index 79fa79e..0c8fa54 100644
> --- a/drivers/mtd/onenand/Kconfig
> +++ b/drivers/mtd/onenand/Kconfig
> @@ -71,4 +71,66 @@ config MTD_ONENAND_SIM
> The simulator may simulate various OneNAND flash chips for the
> OneNAND MTD layer.
>
> +config MTD_FLEXONENAND_BOUNDARY
> + bool "Flex-OneNAND Boundary Configuration"
> + depends on MTD_ONENAND
> + default n
> + help
> + Set SLC and MLC regions of Flex-OneNAND
> +
> +config MTD_FLEXONENAND_DIE0_BOUNDARY
> + int "Last SLC Block of Flex-OneNAND (min = 0, max = 1023)"
> + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY
> + default "-1"
> + help
> + Configure Partition Information (PI) of Flex-OneNAND
> +
> + Entered value indicates index of last SLC block on Flex-OneNAND.
> + The remaining blocks are configured as MLC blocks.
> +
> + A value of -1 means that PI remains unchanged.
> +
> + This setting applies to :
> + - SDP Flex-OneNAND
> + - Die 1 of DDP Flex-OneNAND.
> +
> +config MTD_FLEXONENAND_DIE0_ISLOCKED
> + bool "Lock Boundary of Flex-OneNAND"
> + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY
> + default n
> + help
> + Configure if Flex-OneNAND boundary should be locked.
> + Once locked, the boundary cannot be changed.
> +
> + This setting applies to :
> + - SDP Flex-OneNAND
> + - Die 1 of DDP Flex-OneNAND
> +
> +config MTD_FLEXONENAND_DDP_BOUNDARY
> + bool "Flex-OneNAND DDP Boundary Configuration"
> + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY
> + default n
> + help
> + Set SLC and MLC regions of Die 2 of Flex-OneNAND DDP
> +
> +config MTD_FLEXONENAND_DIE1_BOUNDARY
> + int "Last SLC Block of Flex-OneNAND Die 2 (min = 0, max = 1023)"
> + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY
> + default "-1"
> + help
> + Configure Partition Information (PI) for Die 2 of DDP Flex-OneNAND.
> +
> + Entered value indicates index of last SLC block on Flex-OneNAND.
> + The remaining blocks are configured as MLC blocks.
> +
> + A value of -1 means that PI remains unchanged.
> +
> +config MTD_FLEXONENAND_DIE1_ISLOCKED
> + bool "Lock Boundary of Flex-OneNAND Die 2"
> + depends on MTD_ONENAND && MTD_FLEXONENAND_BOUNDARY && MTD_FLEXONENAND_DDP_BOUNDARY
> + default n
> + help
> + Configure if boundary for Die 2 of DDP Flex-OneNAND should be locked.
> + Once locked, the boundary cannot be changed.

This looks quite user-unfriendly to me. People will need to rebuild
their kernel (or request a new one from their provider) just to alter
some obscure MTD option.

It is about 100000000000 times better for people to be able to compile
or distribute a single kernel image, and for the users of those kernels
to be able to select options at boot-time or at runtime.

Can that be done here?

>
> ...
>
> +static loff_t flexonenand_get_addr(struct onenand_chip *this, int block)
> +{
> + loff_t ofs = 0;
> + int die = 0, boundary;
> +
> + if (ONENAND_IS_DDP(this) && block >= this->density_mask) {
> + block -= this->density_mask;
> + die = 1;
> + ofs = this->diesize[0];
> + }
> +
> + boundary = this->boundary[die];
> + ofs += block << (this->erase_shift - 1);
> + if (block > (boundary + 1))
> + ofs += (block - boundary - 1) << (this->erase_shift - 1);

Both `block' and `boundary' have 32-bit types. Are you sure that the
left-shift cannot overflow?

> + return ofs;
> +}
> +
> +inline loff_t onenand_get_addr(struct onenand_chip *this, int block)
> +{
> + if (!FLEXONENAND(this))
> + return block << this->erase_shift;

Ditto.

> + return flexonenand_get_addr(this, block);
> +}
> +
>
> ...
>
> +static inline int onenand_read_ecc(struct onenand_chip *this)
> +{
> + int ecc, i, result = 0;
> +
> + if (!FLEXONENAND(this))
> + return this->read_word(this->base + ONENAND_REG_ECC_STATUS);
> +
> + for (i = 0; i < 4; i++) {
> + ecc = this->read_word(this->base + ONENAND_REG_ECC_STATUS + i);
> + if (likely(!ecc))
> + continue;
> + if (ecc & FLEXONENAND_UNCORRECTABLE_ERROR)
> + return ONENAND_ECC_2BIT_ALL;
> + else
> + result = ONENAND_ECC_1BIT_ALL;
> + }
> +
> + return result;
> +}

This patch does too much manual inlining. THis function in particular
(which has two callsites) is waaaaaaaay too large to be inlined.

>
> ...
>
> +static int onenand_mlc_read_ops_nolock(struct mtd_info *mtd, loff_t from,
> + struct mtd_oob_ops *ops)
> +{
> + struct onenand_chip *this = mtd->priv;
> + struct mtd_ecc_stats stats;
> + size_t len = ops->len;
> + size_t ooblen = ops->ooblen;
> + u_char *buf = ops->datbuf;
> + u_char *oobbuf = ops->oobbuf;
> + int read = 0, column, thislen;
> + int oobread = 0, oobcolumn, thisooblen, oobsize;
> + int ret = 0;
> + int writesize = this->writesize;
> +
> + DEBUG(MTD_DEBUG_LEVEL3, "onenand_mlc_read_ops_nolock: from = 0x%08x, len = %i\n", (unsigned int) from, (int) len);
> +
> + if (ops->mode == MTD_OOB_AUTO)
> + oobsize = this->ecclayout->oobavail;
> + else
> + oobsize = mtd->oobsize;
> +
> + oobcolumn = from & (mtd->oobsize - 1);
> +
> + /* Do not allow reads past end of device */
> + if (from + len > mtd->size) {
> + printk(KERN_ERR "onenand_mlc_read_ops_nolock: Attempt read beyond end of device\n");
> + ops->retlen = 0;
> + ops->oobretlen = 0;
> + return -EINVAL;
> + }
> +
> + stats = mtd->ecc_stats;
> +
> + while (read < len) {
> + cond_resched();
> +
> + thislen = min_t(int, writesize, len - read);

Use of min_t is often a sign that the types are wrong.

`len' and `read' should have type size_t. Probably other things should
be size_t as well.

> + column = from & (writesize - 1);
> + if (column + thislen > writesize)
> + thislen = writesize - column;
> +
> + if (!onenand_check_bufferram(mtd, from)) {
> + this->command(mtd, ONENAND_CMD_READ, from, writesize);
> +
> + ret = this->wait(mtd, FL_READING);
> + if (unlikely(ret))
> + ret = onenand_recover_lsb(mtd, from, ret);
> + onenand_update_bufferram(mtd, from, !ret);
> + if (ret == -EBADMSG)
> + ret = 0;
> + }
> +
> + this->read_bufferram(mtd, ONENAND_DATARAM, buf, column, thislen);
> + if (oobbuf) {
> + thisooblen = oobsize - oobcolumn;
> + thisooblen = min_t(int, thisooblen, ooblen - oobread);
> +
> + if (ops->mode == MTD_OOB_AUTO)
> + onenand_transfer_auto_oob(mtd, oobbuf, oobcolumn, thisooblen);
> + else
> + this->read_bufferram(mtd, ONENAND_SPARERAM, oobbuf, oobcolumn, thisooblen);
> + oobread += thisooblen;
> + oobbuf += thisooblen;
> + oobcolumn = 0;
> + }
> +
> + read += thislen;
> + if (read == len)
> + break;
> +
> + from += thislen;
> + buf += thislen;
> + }
> +
> + /*
> + * Return success, if no ECC failures, else -EBADMSG
> + * fs driver will take care of that, because
> + * retlen == desired len and result == -EBADMSG
> + */
> + ops->retlen = read;
> + ops->oobretlen = oobread;
> +
> + if (ret)
> + return ret;
> +
> + if (mtd->ecc_stats.failed - stats.failed)
> + return -EBADMSG;
> +
> + return mtd->ecc_stats.corrected - stats.corrected ? -EUCLEAN : 0;
> +}

I wonder what the heck EUCLEAN was invented for and whether MTD's
extensive use of it is appropriate.

>
> ...
>
> --- a/include/linux/mtd/onenand.h
> +++ b/include/linux/mtd/onenand.h
> @@ -17,8 +17,32 @@
> #include <linux/mtd/onenand_regs.h>
> #include <linux/mtd/bbm.h>
>
> +#define MAX_DIES 2
> #define MAX_BUFFERRAM 2
>
> +#if defined CONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY

Plain old `#ifdef' is more common.

> +#define FLEXONENAND_DIE0_BOUNDARY CONFIG_MTD_FLEXONENAND_DIE0_BOUNDARY
> +#else
> +#define FLEXONENAND_DIE0_BOUNDARY -1
> +#endif
> +
> +#if defined CONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY
> +#define FLEXONENAND_DIE1_BOUNDARY CONFIG_MTD_FLEXONENAND_DIE1_BOUNDARY
> +#else
> +#define FLEXONENAND_DIE1_BOUNDARY -1
> +#endif
> +
> +#if defined CONFIG_MTD_FLEXONENAND_DIE0_ISLOCKED
> +#define FLEXONENAND_DIE0_ISLOCKED 1
> +#else
> +#define FLEXONENAND_DIE0_ISLOCKED 0
> +#endif
> +#if defined CONFIG_MTD_FLEXONENAND_DIE1_ISLOCKED
> +#define FLEXONENAND_DIE1_ISLOCKED 1
> +#else
> +#define FLEXONENAND_DIE1_ISLOCKED 0
> +#endif
> +
>
> ...
>
> +#define ONENAND_IS_MLC(this) \
> + (this->technology & ONENAND_TECHNOLOGY_IS_MLC)

hm, I wonder why all these things were implemented as macros.

>
> ...
>



\
 
 \ /
  Last update: 2009-03-03 21:53    [W:0.964 / U:0.648 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site