mtd: nand: mxc: Fix failed/corrected values

mtd: nand: mxc: Fix failed/corrected values

[Sascha Hauer]

The mxc_nand controller returns bogus corrected/failed values. Fix
this by adding a own read_page function. The other two patches
are cleanups based on the first one. The patches are targeted for
the v2/v3 controller types, we could do the same for v1 controllers
aswell, but I left that out because I don't have any hardware handy
at the moment.

Sascha

----------------------------------------------------------------
Sascha Hauer (3):
      mtd: nand: mxc: Fix failed/corrected values
      mtd: nand: mxc: Add own write_page
      mtd: nand: mxc: Drop now unnecessary correct_data function

 drivers/mtd/nand/mxc_nand.c | 51 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 37 insertions(+), 14 deletions(-)

[PATCH 1/3] mtd: nand: mxc: Fix failed/corrected values

[Sascha Hauer]

Currently nand_read_page_hwecc() from nand_base calls
mxc_nand_correct_data_v2_v3() for each subpage, but in this function we
return the corrected/failed results for the whole page instead
of a single subpage. On a 2k page size Nand this leads to results which
are 4 times too high.
The whole ecc.calculate/ecc.correct mechanism used by
nand_read_page_hwecc() is not suitable for devices which correct the
data in hardware, so fix this by using a driver specific read_page
function which does the right thing.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 35 +++++++++++++++++++++++++----------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index f3be0b2a8869..65d5cde4692b 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -140,6 +140,8 @@ struct mxc_nand_host;
 
 struct mxc_nand_devtype_data {
 	void (*preset)(struct mtd_info *);
+	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
+			uint8_t *buf, int oob_required, int page);
 	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_page)(struct mtd_info *, unsigned int);
@@ -617,13 +619,23 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
 static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 				 u_char *read_ecc, u_char *calc_ecc)
 {
-	struct nand_chip *nand_chip = mtd_to_nand(mtd);
-	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
+	return 0;
+}
+
+static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
+					  struct nand_chip *chip,
+					  uint8_t *buf, int oob_required,
+					  int page)
+{
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+	unsigned int max_bitflips = 0;
 	u32 ecc_stat, err;
-	int no_subpages = 1;
-	int ret = 0;
+	int no_subpages;
 	u8 ecc_bit_mask, err_limit;
 
+	chip->read_buf(mtd, buf, mtd->writesize);
+	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
+
 	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
 	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
 
@@ -634,17 +646,16 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
 	do {
 		err = ecc_stat & ecc_bit_mask;
 		if (err > err_limit) {
-			dev_dbg(host->dev, "UnCorrectable RS-ECC Error\n");
-			return -EBADMSG;
+			mtd->ecc_stats.failed++;
 		} else {
-			ret += err;
+			mtd->ecc_stats.corrected += err;
+			max_bitflips = max_t(unsigned int, max_bitflips, err);
 		}
+
 		ecc_stat >>= 4;
 	} while (--no_subpages);
 
-	dev_dbg(host->dev, "%d Symbol Correctable RS-ECC Error\n", ret);
-
-	return ret;
+	return max_bitflips;
 }
 
 static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
@@ -1444,6 +1455,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
 /* v21: i.MX25, i.MX35 */
 static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 	.preset = preset_v2,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v1_v2,
 	.send_addr = send_addr_v1_v2,
 	.send_page = send_page_v2,
@@ -1469,6 +1481,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 /* v3.2a: i.MX51 */
 static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 	.preset = preset_v3,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1494,6 +1507,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 /* v3.2b: i.MX53 */
 static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
 	.preset = preset_v3,
+	.read_page = mxc_nand_read_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1751,6 +1765,7 @@ static int mxcnd_probe(struct platform_device *pdev)
 
 	switch (this->ecc.mode) {
 	case NAND_ECC_HW:
+		this->ecc.read_page = host->devtype_data->read_page;
 		this->ecc.calculate = mxc_nand_calculate_ecc;
 		this->ecc.hwctl = mxc_nand_enable_hwecc;
 		this->ecc.correct = host->devtype_data->correct_data;
-- 
2.11.0

[PATCH 2/3] mtd: nand: mxc: Add own write_page

[Sascha Hauer]

Now that we have our own read_page function add a write_page function
for consistency aswell. This can be a lot easier than the generic
function since we do not have to iterate over subpages but can write
the whole page at once.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 65d5cde4692b..a54804f14bb1 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -142,6 +142,8 @@ struct mxc_nand_devtype_data {
 	void (*preset)(struct mtd_info *);
 	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
 			uint8_t *buf, int oob_required, int page);
+	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
+			  const uint8_t *buf, int oob_required, int page);
 	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
 	void (*send_page)(struct mtd_info *, unsigned int);
@@ -658,6 +660,17 @@ static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
 	return max_bitflips;
 }
 
+static int mxc_nand_write_page_hwecc_v2_v3(struct mtd_info *mtd,
+					   struct nand_chip *chip,
+					   const uint8_t *buf, int oob_required,
+					   int page)
+{
+	chip->write_buf(mtd, buf, mtd->writesize);
+	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
+
+	return 0;
+}
+
 static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
 				  u_char *ecc_code)
 {
@@ -1456,6 +1469,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
 static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 	.preset = preset_v2,
 	.read_page = mxc_nand_read_page_hwecc_v2_v3,
+	.write_page = mxc_nand_write_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v1_v2,
 	.send_addr = send_addr_v1_v2,
 	.send_page = send_page_v2,
@@ -1482,6 +1496,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 	.preset = preset_v3,
 	.read_page = mxc_nand_read_page_hwecc_v2_v3,
+	.write_page = mxc_nand_write_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1508,6 +1523,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
 	.preset = preset_v3,
 	.read_page = mxc_nand_read_page_hwecc_v2_v3,
+	.write_page = mxc_nand_write_page_hwecc_v2_v3,
 	.send_cmd = send_cmd_v3,
 	.send_addr = send_addr_v3,
 	.send_page = send_page_v3,
@@ -1766,6 +1782,7 @@ static int mxcnd_probe(struct platform_device *pdev)
 	switch (this->ecc.mode) {
 	case NAND_ECC_HW:
 		this->ecc.read_page = host->devtype_data->read_page;
+		this->ecc.write_page = host->devtype_data->write_page;
 		this->ecc.calculate = mxc_nand_calculate_ecc;
 		this->ecc.hwctl = mxc_nand_enable_hwecc;
 		this->ecc.correct = host->devtype_data->correct_data;
-- 
2.11.0

[PATCH 3/3] mtd: nand: mxc: Drop now unnecessary correct_data function

[Sascha Hauer]

correct_data is no longer used for v2_v3 controllers, so remove
it.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/mtd/nand/mxc_nand.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index a54804f14bb1..41838ee79bb5 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -618,12 +618,6 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
 	return 0;
 }
 
-static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
-				 u_char *read_ecc, u_char *calc_ecc)
-{
-	return 0;
-}
-
 static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
 					  struct nand_chip *chip,
 					  uint8_t *buf, int oob_required,
@@ -1480,7 +1474,6 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
 	.get_ecc_status = get_ecc_status_v2,
 	.ooblayout = &mxc_v2_ooblayout_ops,
 	.select_chip = mxc_nand_select_chip_v2,
-	.correct_data = mxc_nand_correct_data_v2_v3,
 	.setup_data_interface = mxc_nand_v2_setup_data_interface,
 	.irqpending_quirk = 0,
 	.needs_ip = 0,
@@ -1507,7 +1500,6 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
 	.get_ecc_status = get_ecc_status_v3,
 	.ooblayout = &mxc_v2_ooblayout_ops,
 	.select_chip = mxc_nand_select_chip_v1_v3,
-	.correct_data = mxc_nand_correct_data_v2_v3,
 	.irqpending_quirk = 0,
 	.needs_ip = 1,
 	.regs_offset = 0,
@@ -1534,7 +1526,6 @@ static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
 	.get_ecc_status = get_ecc_status_v3,
 	.ooblayout = &mxc_v2_ooblayout_ops,
 	.select_chip = mxc_nand_select_chip_v1_v3,
-	.correct_data = mxc_nand_correct_data_v2_v3,
 	.irqpending_quirk = 0,
 	.needs_ip = 1,
 	.regs_offset = 0,
-- 
2.11.0

Re: [PATCH 1/3] mtd: nand: mxc: Fix failed/corrected values

[Boris Brezillon]

On Fri, 15 Dec 2017 09:55:02 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Currently nand_read_page_hwecc() from nand_base calls
> mxc_nand_correct_data_v2_v3() for each subpage, but in this function we
> return the corrected/failed results for the whole page instead
> of a single subpage. On a 2k page size Nand this leads to results which
> are 4 times too high.
> The whole ecc.calculate/ecc.correct mechanism used by
> nand_read_page_hwecc() is not suitable for devices which correct the
> data in hardware, so fix this by using a driver specific read_page
> function which does the right thing.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 35 +++++++++++++++++++++++++----------
>  1 file changed, 25 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index f3be0b2a8869..65d5cde4692b 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -140,6 +140,8 @@ struct mxc_nand_host;
>  
>  struct mxc_nand_devtype_data {
>  	void (*preset)(struct mtd_info *);
> +	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> +			uint8_t *buf, int oob_required, int page);
>  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_page)(struct mtd_info *, unsigned int);
> @@ -617,13 +619,23 @@ static int mxc_nand_correct_data_v1(struct mtd_info *mtd, u_char *dat,
>  static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  				 u_char *read_ecc, u_char *calc_ecc)
>  {
> -	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> -	struct mxc_nand_host *host = nand_get_controller_data(nand_chip);
> +	return 0;
> +}
> +
> +static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> +					  struct nand_chip *chip,
> +					  uint8_t *buf, int oob_required,
> +					  int page)
> +{
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +	unsigned int max_bitflips = 0;
>  	u32 ecc_stat, err;
> -	int no_subpages = 1;
> -	int ret = 0;
> +	int no_subpages;
>  	u8 ecc_bit_mask, err_limit;

Sorry but you'll have to rebase this work on nand/next.
->read_page() hooks are now expected to send the READ0 command on
their own.

>  
> +	chip->read_buf(mtd, buf, mtd->writesize);
> +	chip->read_buf(mtd, chip->oob_poi, mtd->oobsize);
> +
>  	ecc_bit_mask = (host->eccsize == 4) ? 0x7 : 0xf;
>  	err_limit = (host->eccsize == 4) ? 0x4 : 0x8;
>  
> @@ -634,17 +646,16 @@ static int mxc_nand_correct_data_v2_v3(struct mtd_info *mtd, u_char *dat,
>  	do {
>  		err = ecc_stat & ecc_bit_mask;
>  		if (err > err_limit) {
> -			dev_dbg(host->dev, "UnCorrectable RS-ECC Error\n");
> -			return -EBADMSG;
> +			mtd->ecc_stats.failed++;
>  		} else {
> -			ret += err;
> +			mtd->ecc_stats.corrected += err;
> +			max_bitflips = max_t(unsigned int, max_bitflips, err);
>  		}
> +
>  		ecc_stat >>= 4;
>  	} while (--no_subpages);
>  
> -	dev_dbg(host->dev, "%d Symbol Correctable RS-ECC Error\n", ret);
> -
> -	return ret;
> +	return max_bitflips;
>  }
>  
>  static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> @@ -1444,6 +1455,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
>  /* v21: i.MX25, i.MX35 */
>  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  	.preset = preset_v2,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v1_v2,
>  	.send_addr = send_addr_v1_v2,
>  	.send_page = send_page_v2,
> @@ -1469,6 +1481,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  /* v3.2a: i.MX51 */
>  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  	.preset = preset_v3,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1494,6 +1507,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  /* v3.2b: i.MX53 */
>  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
>  	.preset = preset_v3,
> +	.read_page = mxc_nand_read_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1751,6 +1765,7 @@ static int mxcnd_probe(struct platform_device *pdev)
>  
>  	switch (this->ecc.mode) {
>  	case NAND_ECC_HW:
> +		this->ecc.read_page = host->devtype_data->read_page;
>  		this->ecc.calculate = mxc_nand_calculate_ecc;
>  		this->ecc.hwctl = mxc_nand_enable_hwecc;
>  		this->ecc.correct = host->devtype_data->correct_data;

Re: [PATCH 2/3] mtd: nand: mxc: Add own write_page

[Boris Brezillon]

On Fri, 15 Dec 2017 09:55:03 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Now that we have our own read_page function add a write_page function
> for consistency aswell. This can be a lot easier than the generic
> function since we do not have to iterate over subpages but can write
> the whole page at once.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 65d5cde4692b..a54804f14bb1 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -142,6 +142,8 @@ struct mxc_nand_devtype_data {
>  	void (*preset)(struct mtd_info *);
>  	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
>  			uint8_t *buf, int oob_required, int page);
> +	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
> +			  const uint8_t *buf, int oob_required, int page);
>  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_page)(struct mtd_info *, unsigned int);
> @@ -658,6 +660,17 @@ static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
>  	return max_bitflips;
>  }
>  
> +static int mxc_nand_write_page_hwecc_v2_v3(struct mtd_info *mtd,
> +					   struct nand_chip *chip,
> +					   const uint8_t *buf, int oob_required,
> +					   int page)
> +{

Same as for the ->read_page() hooks, this needs to be rebased on top of
nand/next.

Note that I've seen other drivers implementing the exact same sequence
(one example is fsl_elbc_write_subpage() [1] but AFAIR there are
others), so maybe it's worth creating a nand_write_page_raw_force_oob()
helper.

> +	chip->write_buf(mtd, buf, mtd->writesize);
> +	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +
> +	return 0;
> +}
> +

[1]http://elixir.free-electrons.com/linux/v4.15-rc3/source/drivers/mtd/nand/fsl_elbc_nand.c#L741

Re: [PATCH 2/3] mtd: nand: mxc: Add own write_page

[Boris Brezillon]

On Fri, 15 Dec 2017 09:55:03 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Now that we have our own read_page function add a write_page function
> for consistency aswell. This can be a lot easier than the generic
> function since we do not have to iterate over subpages but can write
> the whole page at once.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 65d5cde4692b..a54804f14bb1 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -142,6 +142,8 @@ struct mxc_nand_devtype_data {
>  	void (*preset)(struct mtd_info *);
>  	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
>  			uint8_t *buf, int oob_required, int page);
> +	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
> +			  const uint8_t *buf, int oob_required, int page);
>  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
>  	void (*send_page)(struct mtd_info *, unsigned int);
> @@ -658,6 +660,17 @@ static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
>  	return max_bitflips;
>  }
>  
> +static int mxc_nand_write_page_hwecc_v2_v3(struct mtd_info *mtd,
> +					   struct nand_chip *chip,
> +					   const uint8_t *buf, int oob_required,
> +					   int page)
> +{
> +	chip->write_buf(mtd, buf, mtd->writesize);
> +	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> +
> +	return 0;
> +}
> +
>  static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
>  				  u_char *ecc_code)
>  {
> @@ -1456,6 +1469,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
>  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  	.preset = preset_v2,
>  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v1_v2,
>  	.send_addr = send_addr_v1_v2,
>  	.send_page = send_page_v2,
> @@ -1482,6 +1496,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
>  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  	.preset = preset_v3,
>  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1508,6 +1523,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
>  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
>  	.preset = preset_v3,
>  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
>  	.send_cmd = send_cmd_v3,
>  	.send_addr = send_addr_v3,
>  	.send_page = send_page_v3,
> @@ -1766,6 +1782,7 @@ static int mxcnd_probe(struct platform_device *pdev)
>  	switch (this->ecc.mode) {
>  	case NAND_ECC_HW:
>  		this->ecc.read_page = host->devtype_data->read_page;
> +		this->ecc.write_page = host->devtype_data->write_page;
>  		this->ecc.calculate = mxc_nand_calculate_ecc;

I'm pretty sure you don't need this dummy calculate method now that you
provide your own ->write/read_page() implementations.

>  		this->ecc.hwctl = mxc_nand_enable_hwecc;
>  		this->ecc.correct = host->devtype_data->correct_data;

Re: [PATCH 2/3] mtd: nand: mxc: Add own write_page

[Sascha Hauer]

On Mon, Dec 18, 2017 at 06:03:08PM +0100, Boris Brezillon wrote:
> On Fri, 15 Dec 2017 09:55:03 +0100
> Sascha Hauer <s.hauer@pengutronix.de> wrote:
> 
> > Now that we have our own read_page function add a write_page function
> > for consistency aswell. This can be a lot easier than the generic
> > function since we do not have to iterate over subpages but can write
> > the whole page at once.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/mxc_nand.c | 17 +++++++++++++++++
> >  1 file changed, 17 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index 65d5cde4692b..a54804f14bb1 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -142,6 +142,8 @@ struct mxc_nand_devtype_data {
> >  	void (*preset)(struct mtd_info *);
> >  	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> >  			uint8_t *buf, int oob_required, int page);
> > +	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > +			  const uint8_t *buf, int oob_required, int page);
> >  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
> >  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
> >  	void (*send_page)(struct mtd_info *, unsigned int);
> > @@ -658,6 +660,17 @@ static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> >  	return max_bitflips;
> >  }
> >  
> > +static int mxc_nand_write_page_hwecc_v2_v3(struct mtd_info *mtd,
> > +					   struct nand_chip *chip,
> > +					   const uint8_t *buf, int oob_required,
> > +					   int page)
> > +{
> > +	chip->write_buf(mtd, buf, mtd->writesize);
> > +	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> > +
> > +	return 0;
> > +}
> > +
> >  static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> >  				  u_char *ecc_code)
> >  {
> > @@ -1456,6 +1469,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
> >  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> >  	.preset = preset_v2,
> >  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> > +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v1_v2,
> >  	.send_addr = send_addr_v1_v2,
> >  	.send_page = send_page_v2,
> > @@ -1482,6 +1496,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> >  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> >  	.preset = preset_v3,
> >  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> > +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v3,
> >  	.send_addr = send_addr_v3,
> >  	.send_page = send_page_v3,
> > @@ -1508,6 +1523,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> >  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
> >  	.preset = preset_v3,
> >  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> > +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
> >  	.send_cmd = send_cmd_v3,
> >  	.send_addr = send_addr_v3,
> >  	.send_page = send_page_v3,
> > @@ -1766,6 +1782,7 @@ static int mxcnd_probe(struct platform_device *pdev)
> >  	switch (this->ecc.mode) {
> >  	case NAND_ECC_HW:
> >  		this->ecc.read_page = host->devtype_data->read_page;
> > +		this->ecc.write_page = host->devtype_data->write_page;
> >  		this->ecc.calculate = mxc_nand_calculate_ecc;
> 
> I'm pretty sure you don't need this dummy calculate method now that you
> provide your own ->write/read_page() implementations.

True for the v2/v3 type controllers, but not for v1 which I haven't
changed in this series.

Will update the series next year.

Thanks
  Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/3] mtd: nand: mxc: Add own write_page

[Boris Brezillon]

On Mon, 18 Dec 2017 21:49:35 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> On Mon, Dec 18, 2017 at 06:03:08PM +0100, Boris Brezillon wrote:
> > On Fri, 15 Dec 2017 09:55:03 +0100
> > Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >   
> > > Now that we have our own read_page function add a write_page function
> > > for consistency aswell. This can be a lot easier than the generic
> > > function since we do not have to iterate over subpages but can write
> > > the whole page at once.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/nand/mxc_nand.c | 17 +++++++++++++++++
> > >  1 file changed, 17 insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > > index 65d5cde4692b..a54804f14bb1 100644
> > > --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -142,6 +142,8 @@ struct mxc_nand_devtype_data {
> > >  	void (*preset)(struct mtd_info *);
> > >  	int (*read_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > >  			uint8_t *buf, int oob_required, int page);
> > > +	int (*write_page)(struct mtd_info *mtd, struct nand_chip *chip,
> > > +			  const uint8_t *buf, int oob_required, int page);
> > >  	void (*send_cmd)(struct mxc_nand_host *, uint16_t, int);
> > >  	void (*send_addr)(struct mxc_nand_host *, uint16_t, int);
> > >  	void (*send_page)(struct mtd_info *, unsigned int);
> > > @@ -658,6 +660,17 @@ static int mxc_nand_read_page_hwecc_v2_v3(struct mtd_info *mtd,
> > >  	return max_bitflips;
> > >  }
> > >  
> > > +static int mxc_nand_write_page_hwecc_v2_v3(struct mtd_info *mtd,
> > > +					   struct nand_chip *chip,
> > > +					   const uint8_t *buf, int oob_required,
> > > +					   int page)
> > > +{
> > > +	chip->write_buf(mtd, buf, mtd->writesize);
> > > +	chip->write_buf(mtd, chip->oob_poi, mtd->oobsize);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
> > >  				  u_char *ecc_code)
> > >  {
> > > @@ -1456,6 +1469,7 @@ static const struct mxc_nand_devtype_data imx27_nand_devtype_data = {
> > >  static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> > >  	.preset = preset_v2,
> > >  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> > > +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
> > >  	.send_cmd = send_cmd_v1_v2,
> > >  	.send_addr = send_addr_v1_v2,
> > >  	.send_page = send_page_v2,
> > > @@ -1482,6 +1496,7 @@ static const struct mxc_nand_devtype_data imx25_nand_devtype_data = {
> > >  static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> > >  	.preset = preset_v3,
> > >  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> > > +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
> > >  	.send_cmd = send_cmd_v3,
> > >  	.send_addr = send_addr_v3,
> > >  	.send_page = send_page_v3,
> > > @@ -1508,6 +1523,7 @@ static const struct mxc_nand_devtype_data imx51_nand_devtype_data = {
> > >  static const struct mxc_nand_devtype_data imx53_nand_devtype_data = {
> > >  	.preset = preset_v3,
> > >  	.read_page = mxc_nand_read_page_hwecc_v2_v3,
> > > +	.write_page = mxc_nand_write_page_hwecc_v2_v3,
> > >  	.send_cmd = send_cmd_v3,
> > >  	.send_addr = send_addr_v3,
> > >  	.send_page = send_page_v3,
> > > @@ -1766,6 +1782,7 @@ static int mxcnd_probe(struct platform_device *pdev)
> > >  	switch (this->ecc.mode) {
> > >  	case NAND_ECC_HW:
> > >  		this->ecc.read_page = host->devtype_data->read_page;
> > > +		this->ecc.write_page = host->devtype_data->write_page;
> > >  		this->ecc.calculate = mxc_nand_calculate_ecc;  
> > 
> > I'm pretty sure you don't need this dummy calculate method now that you
> > provide your own ->write/read_page() implementations.  
> 
> True for the v2/v3 type controllers, but not for v1 which I haven't
> changed in this series.

Indeed, forgot that v1 support was not updated.

> 
> Will update the series next year.

Thanks.

> 
> Thanks
>   Sascha
>