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

[Boris Brezillon <boris.brezillon@free-electrons.com>]

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