[PATCH] mtd: rawnand: micron: Adapt the PAGE READ flow to constraint controllers

[PATCH] mtd: rawnand: micron: Adapt the PAGE READ flow to constraint controllers

[Miquel Raynal]

There are controllers not able to just read data cycles on the
bus. There are controllers not able to do a change column.

If we want to support both, we need to check which operation is
supported first. This is the exact same mechanism that is in use for
parameter page reads (ONFI/JEDEC) as the same problem occurs.

Speed testing does not show any throughput penalty so we do not
optimize more than that. However it is likely that, in the future, a
more robust and exhaustive test will run at boot time to avoid
re-checking what is supported and what is not at every call.

Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_micron.c | 44 ++++++++++++++++++++++++------
 1 file changed, 35 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
index b2b047b245f4..bbd0ffbae19a 100644
--- a/drivers/mtd/nand/raw/nand_micron.c
+++ b/drivers/mtd/nand/raw/nand_micron.c
@@ -192,6 +192,7 @@ static int micron_nand_on_die_ecc_status_4(struct nand_chip *chip, u8 status,
 	struct micron_nand *micron = nand_get_manufacturer_data(chip);
 	struct mtd_info *mtd = nand_to_mtd(chip);
 	unsigned int step, max_bitflips = 0;
+	bool use_datain = false;
 	int ret;
 
 	if (!(status & NAND_ECC_STATUS_WRITE_RECOMMENDED)) {
@@ -211,8 +212,18 @@ static int micron_nand_on_die_ecc_status_4(struct nand_chip *chip, u8 status,
 	 * in non-raw mode, even if the user did not request those bytes.
 	 */
 	if (!oob_required) {
-		ret = nand_read_data_op(chip, chip->oob_poi, mtd->oobsize,
-					false, false);
+		if (!nand_has_exec_op(chip) ||
+		    !nand_read_data_op(chip, chip->oob_poi, mtd->oobsize, false,
+				       true))
+			use_datain = true;
+
+		if (use_datain)
+			ret = nand_read_data_op(chip, chip->oob_poi,
+						mtd->oobsize, false, false);
+		else
+			ret = nand_change_read_column_op(chip, mtd->writesize,
+							 chip->oob_poi,
+							 mtd->oobsize, false);
 		if (ret)
 			return ret;
 	}
@@ -285,6 +296,7 @@ micron_nand_read_page_on_die_ecc(struct nand_chip *chip, uint8_t *buf,
 				 int oob_required, int page)
 {
 	struct mtd_info *mtd = nand_to_mtd(chip);
+	bool use_datain = false;
 	u8 status;
 	int ret, max_bitflips = 0;
 
@@ -300,14 +312,28 @@ micron_nand_read_page_on_die_ecc(struct nand_chip *chip, uint8_t *buf,
 	if (ret)
 		goto out;
 
-	ret = nand_exit_status_op(chip);
-	if (ret)
-		goto out;
+	if (!nand_has_exec_op(chip) ||
+	    !nand_read_data_op(chip, buf, mtd->writesize, false, true))
+		use_datain = true;
 
-	ret = nand_read_data_op(chip, buf, mtd->writesize, false, false);
-	if (!ret && oob_required)
-		ret = nand_read_data_op(chip, chip->oob_poi, mtd->oobsize,
-					false, false);
+	if (use_datain) {
+		ret = nand_exit_status_op(chip);
+		if (ret)
+			goto out;
+
+		ret = nand_read_data_op(chip, buf, mtd->writesize, false,
+					false);
+		if (!ret && oob_required)
+			ret = nand_read_data_op(chip, chip->oob_poi,
+						mtd->oobsize, false, false);
+	} else {
+		ret = nand_change_read_column_op(chip, 0, buf, mtd->writesize,
+						 false);
+		if (!ret && oob_required)
+			ret = nand_change_read_column_op(chip, mtd->writesize,
+							 chip->oob_poi,
+							 mtd->oobsize, false);
+	}
 
 	if (chip->ecc.strength == 4)
 		max_bitflips = micron_nand_on_die_ecc_status_4(chip, status,
-- 
2.20.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: micron: Adapt the PAGE READ flow to constraint controllers

[Boris Brezillon]

On Tue, 19 May 2020 09:46:39 +0200
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> There are controllers not able to just read data cycles on the
> bus. There are controllers not able to do a change column.
> 
> If we want to support both, we need to check which operation is
> supported first. This is the exact same mechanism that is in use for
> parameter page reads (ONFI/JEDEC) as the same problem occurs.
> 
> Speed testing does not show any throughput penalty so we do not
> optimize more than that. However it is likely that, in the future, a
> more robust and exhaustive test will run at boot time to avoid
> re-checking what is supported and what is not at every call.
> 
> Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>

This looks good to me, just one thing to address below and you can add

Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

> ---
>  drivers/mtd/nand/raw/nand_micron.c | 44 ++++++++++++++++++++++++------
>  1 file changed, 35 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_micron.c b/drivers/mtd/nand/raw/nand_micron.c
> index b2b047b245f4..bbd0ffbae19a 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c
> @@ -192,6 +192,7 @@ static int micron_nand_on_die_ecc_status_4(struct nand_chip *chip, u8 status,
>  	struct micron_nand *micron = nand_get_manufacturer_data(chip);
>  	struct mtd_info *mtd = nand_to_mtd(chip);
>  	unsigned int step, max_bitflips = 0;
> +	bool use_datain = false;
>  	int ret;
>  
>  	if (!(status & NAND_ECC_STATUS_WRITE_RECOMMENDED)) {
> @@ -211,8 +212,18 @@ static int micron_nand_on_die_ecc_status_4(struct nand_chip *chip, u8 status,
>  	 * in non-raw mode, even if the user did not request those bytes.
>  	 */
>  	if (!oob_required) {
> -		ret = nand_read_data_op(chip, chip->oob_poi, mtd->oobsize,
> -					false, false);

Can we add a comment explaining what we're doing here (basically a
condensed version of the commit message).

> +		if (!nand_has_exec_op(chip) ||
> +		    !nand_read_data_op(chip, chip->oob_poi, mtd->oobsize, false,
> +				       true))
> +			use_datain = true;
> +
> +		if (use_datain)
> +			ret = nand_read_data_op(chip, chip->oob_poi,
> +						mtd->oobsize, false, false);
> +		else
> +			ret = nand_change_read_column_op(chip, mtd->writesize,
> +							 chip->oob_poi,
> +							 mtd->oobsize, false);
>  		if (ret)
>  			return ret;
>  	}
> @@ -285,6 +296,7 @@ micron_nand_read_page_on_die_ecc(struct nand_chip *chip, uint8_t *buf,
>  				 int oob_required, int page)
>  {
>  	struct mtd_info *mtd = nand_to_mtd(chip);
> +	bool use_datain = false;
>  	u8 status;
>  	int ret, max_bitflips = 0;
>  
> @@ -300,14 +312,28 @@ micron_nand_read_page_on_die_ecc(struct nand_chip *chip, uint8_t *buf,
>  	if (ret)
>  		goto out;
>  
> -	ret = nand_exit_status_op(chip);
> -	if (ret)
> -		goto out;

Same here.

> +	if (!nand_has_exec_op(chip) ||
> +	    !nand_read_data_op(chip, buf, mtd->writesize, false, true))
> +		use_datain = true;
>  
> -	ret = nand_read_data_op(chip, buf, mtd->writesize, false, false);
> -	if (!ret && oob_required)
> -		ret = nand_read_data_op(chip, chip->oob_poi, mtd->oobsize,
> -					false, false);
> +	if (use_datain) {
> +		ret = nand_exit_status_op(chip);
> +		if (ret)
> +			goto out;
> +
> +		ret = nand_read_data_op(chip, buf, mtd->writesize, false,
> +					false);
> +		if (!ret && oob_required)
> +			ret = nand_read_data_op(chip, chip->oob_poi,
> +						mtd->oobsize, false, false);
> +	} else {
> +		ret = nand_change_read_column_op(chip, 0, buf, mtd->writesize,
> +						 false);
> +		if (!ret && oob_required)
> +			ret = nand_change_read_column_op(chip, mtd->writesize,
> +							 chip->oob_poi,
> +							 mtd->oobsize, false);
> +	}
>  
>  	if (chip->ecc.strength == 4)
>  		max_bitflips = micron_nand_on_die_ecc_status_4(chip, status,


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH] mtd: rawnand: micron: Adapt the PAGE READ flow to constraint controllers

[Miquel Raynal]

Hi Boris,

Boris Brezillon <boris.brezillon@collabora.com> wrote on Tue, 19 May
2020 10:39:19 +0200:

> On Tue, 19 May 2020 09:46:39 +0200
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > There are controllers not able to just read data cycles on the
> > bus. There are controllers not able to do a change column.
> > 
> > If we want to support both, we need to check which operation is
> > supported first. This is the exact same mechanism that is in use for
> > parameter page reads (ONFI/JEDEC) as the same problem occurs.
> > 
> > Speed testing does not show any throughput penalty so we do not
> > optimize more than that. However it is likely that, in the future, a
> > more robust and exhaustive test will run at boot time to avoid
> > re-checking what is supported and what is not at every call.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>  
> 
> This looks good to me, just one thing to address below and you can add
> 
> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com>

Yes sure.


Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/