[PATCH] mtd: nand: gpmi: Fix subpage reads

[PATCH] mtd: nand: gpmi: Fix subpage reads

[Boris Brezillon]

Commit 25f815f66a14 ("mtd: nand: force drivers to explicitly send
READ/PROG commands") added a call to nand_read_page_op() in
gpmi_ecc_read_page(), which means this function now sends a READ0
command and place the data pointer at the beginning of the page. This
logic is breaking gpmi_ecc_read_subpage() which was calling
gpmi_ecc_read_page() and expected it to only retrieve the data without
sending the READ0 command.

Create a gpmi_ecc_read_page_data() helper which only does the data
retrieval and ECC correction steps and implement gpmi_ecc_read_page()
as a wrapper that calls nand_read_page_op()+gpmi_ecc_read_page_data().

This way, gpmi_ecc_read_subpage() can call gpmi_ecc_read_page_data()
which restores the logic we had before commit 25f815f66a14 ("mtd: nand:
force drivers to explicitly send READ/PROG commands").

Fixes: 25f815f66a14 ("mtd: nand: force drivers to explicitly send READ/PROG commands")
Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
---
 drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
index 3198f5f79a82..93f212457d4d 100644
--- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
+++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
@@ -1029,11 +1029,13 @@ static void block_mark_swapping(struct gpmi_nand_data *this,
 	p[1] = (p[1] & mask) | (from_oob >> (8 - bit));
 }
 
-static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
-				uint8_t *buf, int oob_required, int page)
+static int gpmi_ecc_read_page_data(struct nand_chip *chip,
+				   uint8_t *buf, int oob_required,
+				   int page)
 {
 	struct gpmi_nand_data *this = nand_get_controller_data(chip);
 	struct bch_geometry *nfc_geo = &this->bch_geometry;
+	struct mtd_info *mtd = nand_to_mtd(chip);
 	void          *payload_virt;
 	dma_addr_t    payload_phys;
 	void          *auxiliary_virt;
@@ -1043,8 +1045,6 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	unsigned int  max_bitflips = 0;
 	int           ret;
 
-	nand_read_page_op(chip, page, 0, NULL, 0);
-
 	dev_dbg(this->dev, "page number is : %d\n", page);
 	ret = read_page_prepare(this, buf, nfc_geo->payload_size,
 					this->payload_virt, this->payload_phys,
@@ -1178,6 +1178,14 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
 	return max_bitflips;
 }
 
+static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+			      uint8_t *buf, int oob_required, int page)
+{
+	nand_read_page_op(chip, page, 0, NULL, 0);
+
+	return gpmi_ecc_read_page_data(chip, buf, oob_required, page);
+}
+
 /* Fake a virtual small page for the subpage read */
 static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 			uint32_t offs, uint32_t len, uint8_t *buf, int page)
@@ -1257,7 +1265,7 @@ static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
 
 	/* Read the subpage now */
 	this->swap_block_mark = false;
-	max_bitflips = gpmi_ecc_read_page(mtd, chip, buf, 0, page);
+	max_bitflips = gpmi_ecc_read_page_data(chip, buf, 0, page);
 
 	/* Restore */
 	writel(r1_old, bch_regs + HW_BCH_FLASH0LAYOUT0);
-- 
2.11.0

Re: [PATCH] mtd: nand: gpmi: Fix subpage reads

[Miquel Raynal]

Hello Boris,

On Tue, 23 Jan 2018 11:13:17 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Commit 25f815f66a14 ("mtd: nand: force drivers to explicitly send
> READ/PROG commands") added a call to nand_read_page_op() in
> gpmi_ecc_read_page(), which means this function now sends a READ0
> command and place the data pointer at the beginning of the page. This
> logic is breaking gpmi_ecc_read_subpage() which was calling
> gpmi_ecc_read_page() and expected it to only retrieve the data without
> sending the READ0 command.
> 
> Create a gpmi_ecc_read_page_data() helper which only does the data
> retrieval and ECC correction steps and implement gpmi_ecc_read_page()
> as a wrapper that calls nand_read_page_op()+gpmi_ecc_read_page_data().
> 
> This way, gpmi_ecc_read_subpage() can call gpmi_ecc_read_page_data()
> which restores the logic we had before commit 25f815f66a14 ("mtd: nand:
> force drivers to explicitly send READ/PROG commands").
> 
> Fixes: 25f815f66a14 ("mtd: nand: force drivers to explicitly send READ/PROG commands")
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---

Reviewed-by: Miquel Raynal <miquel.raynal@free-electrons.com>

Thanks for fixing it,
Miquèl

Re: [PATCH] mtd: nand: gpmi: Fix subpage reads

[Han Xu]

________________________________________
From: Miquel Raynal <miquel.raynal@free-electrons.com>
Sent: Wednesday, January 24, 2018 6:51 AM
To: Boris Brezillon
Cc: Richard Weinberger; linux-mtd@lists.infradead.org; David Woodhouse; Brian Norris; Marek Vasut; Cyrille Pitchen; Han Xu
Subject: Re: [PATCH] mtd: nand: gpmi: Fix subpage reads

Hello Boris,

On Tue, 23 Jan 2018 11:13:17 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Commit 25f815f66a14 ("mtd: nand: force drivers to explicitly send
> READ/PROG commands") added a call to nand_read_page_op() in
> gpmi_ecc_read_page(), which means this function now sends a READ0
> command and place the data pointer at the beginning of the page. This
> logic is breaking gpmi_ecc_read_subpage() which was calling
> gpmi_ecc_read_page() and expected it to only retrieve the data without
> sending the READ0 command.
>
> Create a gpmi_ecc_read_page_data() helper which only does the data
> retrieval and ECC correction steps and implement gpmi_ecc_read_page()
> as a wrapper that calls nand_read_page_op()+gpmi_ecc_read_page_data().
>
> This way, gpmi_ecc_read_subpage() can call gpmi_ecc_read_page_data()
> which restores the logic we had before commit 25f815f66a14 ("mtd: nand:
> force drivers to explicitly send READ/PROG commands").
>
> Fixes: 25f815f66a14 ("mtd: nand: force drivers to explicitly send READ/PROG commands")
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---

Reviewed-by: Miquel Raynal <miquel.raynal@free-electrons.com>
Acked-by: Han Xu <han.xu@nxp.com>

Thanks for fixing it,
Miquèl

Re: [PATCH] mtd: nand: gpmi: Fix subpage reads

[Boris Brezillon]

Hi Han,

On Thu, 25 Jan 2018 03:07:26 +0000
Han Xu <han.xu@nxp.com> wrote:

> Acked-by: Han Xu <han.xu@nxp.com>

Could you please fix you mailer (or use another one). Your emails are
systematically rejected by the ML, which means I don't have your Acks
in patchwork and have to add them back manually.

Thanks,

Boris

Re: [PATCH] mtd: nand: gpmi: Fix subpage reads

[Boris Brezillon]

On Tue, 23 Jan 2018 11:13:17 +0100
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> Commit 25f815f66a14 ("mtd: nand: force drivers to explicitly send
> READ/PROG commands") added a call to nand_read_page_op() in
> gpmi_ecc_read_page(), which means this function now sends a READ0
> command and place the data pointer at the beginning of the page. This
> logic is breaking gpmi_ecc_read_subpage() which was calling
> gpmi_ecc_read_page() and expected it to only retrieve the data without
> sending the READ0 command.
> 
> Create a gpmi_ecc_read_page_data() helper which only does the data
> retrieval and ECC correction steps and implement gpmi_ecc_read_page()
> as a wrapper that calls nand_read_page_op()+gpmi_ecc_read_page_data().
> 
> This way, gpmi_ecc_read_subpage() can call gpmi_ecc_read_page_data()
> which restores the logic we had before commit 25f815f66a14 ("mtd: nand:
> force drivers to explicitly send READ/PROG commands").
> 

Applied.

> Fixes: 25f815f66a14 ("mtd: nand: force drivers to explicitly send READ/PROG commands")
> Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com>
> ---
>  drivers/mtd/nand/gpmi-nand/gpmi-nand.c | 18 +++++++++++++-----
>  1 file changed, 13 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> index 3198f5f79a82..93f212457d4d 100644
> --- a/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> +++ b/drivers/mtd/nand/gpmi-nand/gpmi-nand.c
> @@ -1029,11 +1029,13 @@ static void block_mark_swapping(struct gpmi_nand_data *this,
>  	p[1] = (p[1] & mask) | (from_oob >> (8 - bit));
>  }
>  
> -static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> -				uint8_t *buf, int oob_required, int page)
> +static int gpmi_ecc_read_page_data(struct nand_chip *chip,
> +				   uint8_t *buf, int oob_required,
> +				   int page)
>  {
>  	struct gpmi_nand_data *this = nand_get_controller_data(chip);
>  	struct bch_geometry *nfc_geo = &this->bch_geometry;
> +	struct mtd_info *mtd = nand_to_mtd(chip);
>  	void          *payload_virt;
>  	dma_addr_t    payload_phys;
>  	void          *auxiliary_virt;
> @@ -1043,8 +1045,6 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	unsigned int  max_bitflips = 0;
>  	int           ret;
>  
> -	nand_read_page_op(chip, page, 0, NULL, 0);
> -
>  	dev_dbg(this->dev, "page number is : %d\n", page);
>  	ret = read_page_prepare(this, buf, nfc_geo->payload_size,
>  					this->payload_virt, this->payload_phys,
> @@ -1178,6 +1178,14 @@ static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
>  	return max_bitflips;
>  }
>  
> +static int gpmi_ecc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> +			      uint8_t *buf, int oob_required, int page)
> +{
> +	nand_read_page_op(chip, page, 0, NULL, 0);
> +
> +	return gpmi_ecc_read_page_data(chip, buf, oob_required, page);
> +}
> +
>  /* Fake a virtual small page for the subpage read */
>  static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
>  			uint32_t offs, uint32_t len, uint8_t *buf, int page)
> @@ -1257,7 +1265,7 @@ static int gpmi_ecc_read_subpage(struct mtd_info *mtd, struct nand_chip *chip,
>  
>  	/* Read the subpage now */
>  	this->swap_block_mark = false;
> -	max_bitflips = gpmi_ecc_read_page(mtd, chip, buf, 0, page);
> +	max_bitflips = gpmi_ecc_read_page_data(chip, buf, 0, page);
>  
>  	/* Restore */
>  	writel(r1_old, bch_regs + HW_BCH_FLASH0LAYOUT0);