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

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

[Bean Huo 霍斌斌 (beanhuo)]

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(+)

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.
  *
-- 
1.9.1

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

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@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

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

[Boris Brezillon]

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

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com