Re: [PATCH v3 1/1] mtd: spinand: Add flags for the Plane Select bit

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Cheng,

linchengming884@gmail.com wrote on Wed, 28 Aug 2024 14:21:31 +0800:

> From: Cheng Ming Lin <chengminglin@mxic.com.tw>
> 
> Add support for Macronix serial NAND flash with a two-plane structure.
> 
> Add the SPINAND_HAS_PP_PLANE_SELECT_BIT flag for serial NAND flash
> that require inserting the Plane Select bit into the column address
> during the write_to_cache operation.
> 
> Add the SPINAND_HAS_READ_PLANE_SELECT_BIT flag for serial NAND flash
> that require inserting the Plane Select bit into the column address
> during the read_from_cache operation.
> 
> Macronix serial NAND flash with a two-plane structure requires insertion
> of the Plane Select bit into the column address during the write_to_cache
> operation.
> 
> Additionally, for MX35{U,F}2G14AC and MX35LF2GE4AB, insertion of the
> Plane Select bit into the column address is required during the
> read_from_cache operation.
> 
> Signed-off-by: Cheng Ming Lin <chengminglin@mxic.com.tw>
> ---
>  drivers/mtd/nand/spi/core.c     |  6 ++++++
>  drivers/mtd/nand/spi/macronix.c | 17 ++++++++++-------
>  include/linux/mtd/spinand.h     |  2 ++
>  3 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index e0b6715e5dfe..49f2d66c3a9c 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -386,6 +386,9 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
>  	else
>  		rdesc = spinand->dirmaps[req->pos.plane].rdesc_ecc;
>  
> +	if (spinand->flags & SPINAND_HAS_READ_PLANE_SELECT_BIT)
> +		column |= req->pos.plane << fls(nanddev_page_size(nand));
> +
>  	while (nbytes) {
>  		ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf);
>  		if (ret < 0)
> @@ -460,6 +463,9 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
>  	else
>  		wdesc = spinand->dirmaps[req->pos.plane].wdesc_ecc;
>  
> +	if (spinand->flags & SPINAND_HAS_PP_PLANE_SELECT_BIT)
> +		column |= req->pos.plane << fls(nanddev_page_size(nand));

Please separate the core changes and the macronix changes.
1. Add support for the flag in the core (and the include)
2. Use the flag in Macronix driver.

> +
>  	while (nbytes) {
>  		ret = spi_mem_dirmap_write(wdesc, column, nbytes, buf);
>  		if (ret < 0)
> diff --git a/drivers/mtd/nand/spi/macronix.c b/drivers/mtd/nand/spi/macronix.c
> index 3f9e9c572854..a531cc8121ff 100644
> --- a/drivers/mtd/nand/spi/macronix.c
> +++ b/drivers/mtd/nand/spi/macronix.c
> @@ -118,7 +118,8 @@ static const struct spinand_info macronix_spinand_table[] = {
>  		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>  					      &write_cache_variants,
>  					      &update_cache_variants),
> -		     SPINAND_HAS_QE_BIT,
> +		     SPINAND_HAS_QE_BIT | SPINAND_HAS_PP_PLANE_SELECT_BIT |
> +			 SPINAND_HAS_READ_PLANE_SELECT_BIT,

Alignment:	     ^



...

> @@ -263,7 +266,7 @@ static const struct spinand_info macronix_spinand_table[] = {
>  		     SPINAND_INFO_OP_VARIANTS(&read_cache_variants,
>  					      &write_cache_variants,
>  					      &update_cache_variants),
> -		     SPINAND_HAS_QE_BIT,
> +		     SPINAND_HAS_QE_BIT | SPINAND_HAS_PP_PLANE_SELECT_BIT,
>  		     SPINAND_ECCINFO(&mx35lfxge4ab_ooblayout,
>  				     mx35lf1ge4ab_ecc_get_status)),
>  	SPINAND_INFO("MX35UF2G24AD-Z4I8",
> diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> index 5c19ead60499..cec451e7c71c 100644
> --- a/include/linux/mtd/spinand.h
> +++ b/include/linux/mtd/spinand.h
> @@ -312,6 +312,8 @@ struct spinand_ecc_info {
>  
>  #define SPINAND_HAS_QE_BIT		BIT(0)
>  #define SPINAND_HAS_CR_FEAT_BIT		BIT(1)
> +#define SPINAND_HAS_PP_PLANE_SELECT_BIT		BIT(2)
> +#define SPINAND_HAS_READ_PLANE_SELECT_BIT		BIT(3)

Do you think we can have the PP plane select bit without the read plane
select bit? I'd use a single flag for now.

>  
>  /**
>   * struct spinand_ondie_ecc_conf - private SPI-NAND on-die ECC engine structure


Thanks,
Miquèl