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

[Bean Huo éææ (beanhuo)]

> 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¶ìå