| Message ID | A765B125120D1346A63912DDE6D8B6310BF4CECC@NTXXIAMBX02.xacn.micron.com |
|---|---|
| State | Superseded |
| Headers | show |
Hi Bean, On Mon, 28 Sep 2015 07:02:37 +0000 Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> 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 ;-)). > > Signed-off-by: Bean Huo <beanhuo@micron.com> > --- > 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. > 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? Best Regards, Boris
> Hi Bean, > > On Mon, 28 Sep 2015 07:02:37 +0000 > Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> 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@micron.com> > > --- > > 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
Hi Bean, On Wed, 30 Sep 2015 06:05:44 +0000 Bean Huo 霍斌斌 (beanhuo) <beanhuo@micron.com> wrote: > > > 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. > Please wait for other reviews before reworking that. > > > 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. Actually that was a rhetorical question. My point was that this enum definition has nothing to do in this patch, and you're doing that (mixing unrelated changes in the same commit) a lot in your other patches. So please make sure you correctly split your changes next time you send a patch set. Best Regards, Boris
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); 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 +}; + /* * enum ubi_open_mode - UBI volume open mode constants. *
Add support for UBI bakvol in mtd layer. This solution based on MLC NAND dual plane program. so add hook in mtd layer. Signed-off-by: Bean Huo <beanhuo@micron.com> --- include/linux/mtd/mtd.h | 19 +++++++++++++++++++ include/linux/mtd/nand.h | 4 ++++ include/linux/mtd/ubi.h | 9 +++++++++ 3 files changed, 32 insertions(+)