RE: [PATCH 1/9] drivers:nand:mtd: add support for UBI bakvol in mtd layer

From: Bean Huo éææ (beanhuo)
Date: Wed Sep 30 2015 - 02:07:22 EST


> Hi Bean,
>
> On Mon, 28 Sep 2015 07:02:37 +0000
> Bean Huo éææ (beanhuo) <beanhuo@xxxxxxxxxx> wrote:
>
> > Add support for UBI bakvol in mtd layer.
> >
> > This solution based on MLC NAND dual plane program.
> > so add hook in mtd layer.
>
> I know you don't have any other choices to expose "two-plane page program"
> to the UBI layer, but I keep thinking that exposing that to the MTD users is not
> a good idea (I might be wrong ;-)).

Two-plane operation is related to NAND feature, can not involve in UBI layer.
Maybe I can firstly enable multiple plane program in MTD layer, but currently, no people
Uses multiple plane program in Linux, only in this patch.
I don't know if this is necessary.
> >
> > Signed-off-by: Bean Huo <beanhuo@xxxxxxxxxx>
> > ---
> > include/linux/mtd/mtd.h | 19 +++++++++++++++++++
> > include/linux/mtd/nand.h | 4 ++++ include/linux/mtd/ubi.h | 9
> > +++++++++
> > 3 files changed, 32 insertions(+)
> >
> > diff --git a/include/linux/mtd/mtd.h b/include/linux/mtd/mtd.h index
> > f17fa75..cfcb3a68 100644
> > --- a/include/linux/mtd/mtd.h
> > +++ b/include/linux/mtd/mtd.h
> > @@ -204,6 +204,9 @@ struct mtd_info {
> > struct mtd_oob_ops *ops);
> > int (*_write_oob) (struct mtd_info *mtd, loff_t to,
> > struct mtd_oob_ops *ops);
> > + int (*_dual_plane_write_oob) (struct mtd_info *mtd, loff_t to_plane0,
> > + struct mtd_oob_ops *ops_plane0, loff_t to_plane1,
> > + struct mtd_oob_ops *ops_plane1);
>
>
> IMHO, if we were about to allow parallel write operations this should be
> exposed as a more generic API, something like:
>
> struct mtd_write_op {
> loff_t to;
> struct mtd_oob_ops ops;
> };
>
> struct mtd_multi_write_ops {
> struct list_head writes;
> };
>
> int (*_multi_write)(struct mtd_info *mtd,
> struct mtd_multi_write_ops *ops);
>
> Then the NAND layer could optimize that if the NAND chip supports "two-plane
> page program", and if 2 pages in the write list are fulfilling the requirements.
>
Good suggestion, I can improve it for next version patch. Thanks.

> > int (*_get_fact_prot_info) (struct mtd_info *mtd, size_t len,
> > size_t *retlen, struct otp_info *buf);
> > int (*_read_fact_prot_reg) (struct mtd_info *mtd, loff_t from, @@
> > -280,6 +283,22 @@ static inline int mtd_write_oob(struct mtd_info *mtd,
> loff_t to,
> > return mtd->_write_oob(mtd, to, ops); }
> >
> > +static inline int mtd_write_dual_plane_oob(struct mtd_info *mtd,
> > + loff_t to_plane0, struct mtd_oob_ops *ops0, loff_t to_plane1,
> > + struct mtd_oob_ops *ops1)
> > +{
> > + ops0->retlen = ops0->oobretlen = 0;
> > + ops1->retlen = ops1->oobretlen = 0;
> > +
> > + if (!mtd->_dual_plane_write_oob)
> > + return -EOPNOTSUPP;
> > + if (!(mtd->flags & MTD_WRITEABLE))
> > + return -EROFS;
> > +
> > + return mtd->_dual_plane_write_oob(mtd, to_plane0, ops0,
> > + to_plane1, ops1);
> > +}
> > +
> > int mtd_get_fact_prot_info(struct mtd_info *mtd, size_t len, size_t *retlen,
> > struct otp_info *buf);
> > int mtd_read_fact_prot_reg(struct mtd_info *mtd, loff_t from, size_t
> > len, diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
> > index 272f429..4c5be01 100644
> > --- a/include/linux/mtd/nand.h
> > +++ b/include/linux/mtd/nand.h
> > @@ -77,6 +77,7 @@ extern int nand_unlock(struct mtd_info *mtd, loff_t ofs,
> uint64_t len);
> > #define NAND_CMD_READ1 1
> > #define NAND_CMD_RNDOUT 5
> > #define NAND_CMD_PAGEPROG 0x10
> > +#define NAND_CMD_MULTI_PAGEPROG 0x11
> > #define NAND_CMD_READOOB 0x50
> > #define NAND_CMD_ERASE1 0x60
> > #define NAND_CMD_STATUS 0x70
> > @@ -671,6 +672,9 @@ struct nand_chip {
> > int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > uint32_t offset, int data_len, const uint8_t *buf,
> > int oob_required, int page, int cached, int raw);
> > + int (*write_plane_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > + uint32_t offset, int data_len, const uint8_t *buf,
> > + int oob_required, int page, int plane, int raw);
> > int (*onfi_set_features)(struct mtd_info *mtd, struct nand_chip *chip,
> > int feature_addr, uint8_t *subfeature_para);
> > int (*onfi_get_features)(struct mtd_info *mtd, struct nand_chip
> > *chip, diff --git a/include/linux/mtd/ubi.h b/include/linux/mtd/ubi.h
> > index 1e271cb..1da3418 100644
> > --- a/include/linux/mtd/ubi.h
> > +++ b/include/linux/mtd/ubi.h
> > @@ -35,6 +35,15 @@
> > */
> > #define UBI_MAX_SG_COUNT 64
> >
> > +enum {
> > + UBI_BAKVOL_UNONE,
> > + UBI_BAKVOL_INIT_INFO,
> > + UBI_BAKVOL_INIT_INFO_DONE,
> > + UBI_BAKVOL_INIT_VOLUME,
> > + UBI_BAKVOL_INIT_VOLUME_DONE,
> > + UBI_BAKVOL_RUN
> > +};
> > +
>
> Are those changes related to this patch?
>

Yes, maybe can simplify more.

> Best Regards,
>
> Boris
>
> --
> Boris Brezillon, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
N‹§²æ¸›yú²X¬¶ÇvØ–)Þ{.nlj·¥Š{±‘êX§¶›¡Ü}©ž²ÆzÚj:+v‰¨¾«‘êZ+€Êzf£¢·hšˆ§~†­†Ûÿû®w¥¢¸?™¨è&¢)ßf”ùy§m…á«a¶Úÿ 0¶ìå