[6/8] mtd: nand: mxc: Add own write_page

Message ID 20180109101148.13728-7-s.hauer@pengutronix.de
State New
Delegated to: Boris Brezillon
Headers show
Series
  • [1/8] mtd: nand: mxc: reorder functions to avoid forward declarations
Related show

Commit Message

Sascha Hauer Jan. 9, 2018, 10:11 a.m.
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. Also add write_page_raw and write_oob for
proper raw and oob write support.

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

Comments

Boris Brezillon Jan. 12, 2018, 3:36 p.m. | #1
On Tue,  9 Jan 2018 11:11:46 +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. Also add write_page_raw and write_oob for
> proper raw and oob write support.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 56 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 83bb01ab7079..6fbf98851a96 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -893,6 +893,59 @@ static int mxc_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
>  					     page);
>  }
>  
> +static int __mxc_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,

I'm not a big fan of those '__' prefixes. How about keeping
mxc_nand_write_page() for this one, renaming the other
mxc_nand_write_page() into mxc_nand_write_page_ecc() and keeping the
other ones unchanged.

> +				 const uint8_t *buf, int oob_required, int page)

Why don't you pass a 'bool enable_ecc' argument here so that you can
directly do

	host->devtype_data->enable_hwecc(nand_chip, enable_ecc);

from here instead of duplicating it in all wrappers?

> +{
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN, false);
> +	mxc_do_addr_cycle(mtd, 0, page);
> +
> +	memcpy32_toio(host->main_area0, buf, mtd->writesize);
> +	copy_spare(mtd, false, chip->oob_poi);
> +
> +	host->devtype_data->send_page(mtd, NFC_INPUT);
> +	host->devtype_data->send_cmd(host, NAND_CMD_PAGEPROG, true);
> +	mxc_do_addr_cycle(mtd, 0, page);
> +
> +	return 0;
> +}
> +
> +static int mxc_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> +			       const uint8_t *buf, int oob_required, int page)
> +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	host->devtype_data->enable_hwecc(nand_chip, true);
> +
> +	return __mxc_nand_write_page(mtd, chip, buf, oob_required, page);
> +}
> +
> +static int mxc_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
> +				   const uint8_t *buf, int oob_required, int page)
> +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	host->devtype_data->enable_hwecc(nand_chip, false);
> +
> +	return __mxc_nand_write_page(mtd, chip, buf, oob_required, page);
> +}
> +
> +static int mxc_nand_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> +			      int page)
> +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	memset(host->data_buf, 0xff, mtd->writesize);
> +
> +	host->devtype_data->enable_hwecc(nand_chip, false);
> +
> +	return __mxc_nand_write_page(mtd, chip, host->data_buf, 1, page);
> +}
> +
>  static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
>  				  u_char *ecc_code)
>  {
> @@ -1907,6 +1960,9 @@ static int mxcnd_probe(struct platform_device *pdev)
>  		this->ecc.read_page = mxc_nand_read_page;
>  		this->ecc.read_page_raw = mxc_nand_read_page_raw;
>  		this->ecc.read_oob = mxc_nand_read_oob;
> +		this->ecc.write_page = mxc_nand_write_page;
> +		this->ecc.write_page_raw = mxc_nand_write_page_raw;
> +		this->ecc.write_oob = mxc_nand_write_oob;
>  		this->ecc.calculate = mxc_nand_calculate_ecc;
>  		this->ecc.hwctl = mxc_nand_enable_hwecc;
>  		this->ecc.correct = host->devtype_data->correct_data;
Miquel Raynal Jan. 13, 2018, 4:25 p.m. | #2
Hi Sascha,

On Tue,  9 Jan 2018 11:11:46 +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. Also add write_page_raw and write_oob for
> proper raw and oob write support.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  drivers/mtd/nand/mxc_nand.c | 56
> +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56
> insertions(+)
> 
> diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> index 83bb01ab7079..6fbf98851a96 100644
> --- a/drivers/mtd/nand/mxc_nand.c
> +++ b/drivers/mtd/nand/mxc_nand.c
> @@ -893,6 +893,59 @@ static int mxc_nand_read_oob(struct mtd_info
> *mtd, struct nand_chip *chip, page);
>  }
>  
> +static int __mxc_nand_write_page(struct mtd_info *mtd, struct
> nand_chip *chip,
> +				 const uint8_t *buf, int
> oob_required, int page) +{
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN, false);
> +	mxc_do_addr_cycle(mtd, 0, page);
> +
> +	memcpy32_toio(host->main_area0, buf, mtd->writesize);
> +	copy_spare(mtd, false, chip->oob_poi);

If I remember correctly, there was a "spare only" mechanism to avoid
doing full page read/write when unnecessary. What about using it here
when (buf == NULL && oob_required)?

> +
> +	host->devtype_data->send_page(mtd, NFC_INPUT);
> +	host->devtype_data->send_cmd(host, NAND_CMD_PAGEPROG, true);
> +	mxc_do_addr_cycle(mtd, 0, page);
> +
> +	return 0;
> +}
> +
> +static int mxc_nand_write_page(struct mtd_info *mtd, struct
> nand_chip *chip,
> +			       const uint8_t *buf, int oob_required,
> int page) +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	host->devtype_data->enable_hwecc(nand_chip, true);
> +
> +	return __mxc_nand_write_page(mtd, chip, buf, oob_required,
> page); +}
> +
> +static int mxc_nand_write_page_raw(struct mtd_info *mtd, struct
> nand_chip *chip,
> +				   const uint8_t *buf, int
> oob_required, int page) +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	host->devtype_data->enable_hwecc(nand_chip, false);
> +
> +	return __mxc_nand_write_page(mtd, chip, buf, oob_required,
> page); +}
> +
> +static int mxc_nand_write_oob(struct mtd_info *mtd, struct nand_chip
> *chip,
> +			      int page)
> +{
> +	struct nand_chip *nand_chip = mtd_to_nand(mtd);
> +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> +
> +	memset(host->data_buf, 0xff, mtd->writesize);

If the above solution works, this won't be needed anymore.

> +
> +	host->devtype_data->enable_hwecc(nand_chip, false);
> +
> +	return __mxc_nand_write_page(mtd, chip, host->data_buf, 1,
> page);

And this could be:
    return __mxc_nand_write_page(mtd, chip, NULL, 1, page);

> +}
> +
>  static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char
> *dat, u_char *ecc_code)
>  {
> @@ -1907,6 +1960,9 @@ static int mxcnd_probe(struct platform_device
> *pdev) this->ecc.read_page = mxc_nand_read_page;
>  		this->ecc.read_page_raw = mxc_nand_read_page_raw;
>  		this->ecc.read_oob = mxc_nand_read_oob;
> +		this->ecc.write_page = mxc_nand_write_page;
> +		this->ecc.write_page_raw = mxc_nand_write_page_raw;
> +		this->ecc.write_oob = mxc_nand_write_oob;
>  		this->ecc.calculate = mxc_nand_calculate_ecc;
>  		this->ecc.hwctl = mxc_nand_enable_hwecc;
>  		this->ecc.correct = host->devtype_data->correct_data;


Thanks,
Miquèl
Sascha Hauer Jan. 17, 2018, 10:40 a.m. | #3
Hi Miquel,

On Sat, Jan 13, 2018 at 05:25:51PM +0100, Miquel Raynal wrote:
> Hi Sascha,
> 
> On Tue,  9 Jan 2018 11:11:46 +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. Also add write_page_raw and write_oob for
> > proper raw and oob write support.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > ---
> >  drivers/mtd/nand/mxc_nand.c | 56
> > +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56
> > insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
> > index 83bb01ab7079..6fbf98851a96 100644
> > --- a/drivers/mtd/nand/mxc_nand.c
> > +++ b/drivers/mtd/nand/mxc_nand.c
> > @@ -893,6 +893,59 @@ static int mxc_nand_read_oob(struct mtd_info
> > *mtd, struct nand_chip *chip, page);
> >  }
> >  
> > +static int __mxc_nand_write_page(struct mtd_info *mtd, struct
> > nand_chip *chip,
> > +				 const uint8_t *buf, int
> > oob_required, int page) +{
> > +	struct mxc_nand_host *host = nand_get_controller_data(chip);
> > +
> > +	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN, false);
> > +	mxc_do_addr_cycle(mtd, 0, page);
> > +
> > +	memcpy32_toio(host->main_area0, buf, mtd->writesize);
> > +	copy_spare(mtd, false, chip->oob_poi);
> 
> If I remember correctly, there was a "spare only" mechanism to avoid
> doing full page read/write when unnecessary. What about using it here
> when (buf == NULL && oob_required)?

You are probably referring to the SP_EN bit in the NAND_FLASH_CONFIG1
register right? This is described as:

> NAND Flash spare enable. This bit determines whether host reads/writes
> are to NAND Flash spare data only or NAND Flash main and spare data.
> This feature is supported only with memories with 1/2K page size.
> Note: There is no ECC in this mode of operation.
> 0 NAND Flash main and spare data is enabled
> 1 NAND Flash spare only data is enabled

At least the i.MX25 has the limitation of 1/2k page size for this
feature. It's not in the i.MX27 manual. I don't know if the i.MX27
documentation is inaccurate or if this limitation is there already.

Anyway, I don't think it's worth optimizing for this case given that
we still need to support a full page write for 2k pages (which should
be much more widespread nowadays than the 512b page size NANDs).

Sascha
Miquel Raynal Jan. 18, 2018, 7:16 a.m. | #4
Hi Sascha,

On Wed, 17 Jan 2018 11:40:09 +0100
Sascha Hauer <s.hauer@pengutronix.de> wrote:

> Hi Miquel,
> 
> On Sat, Jan 13, 2018 at 05:25:51PM +0100, Miquel Raynal wrote:
> > Hi Sascha,
> > 
> > On Tue,  9 Jan 2018 11:11:46 +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. Also add
> > > write_page_raw and write_oob for proper raw and oob write support.
> > > 
> > > Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> > > ---
> > >  drivers/mtd/nand/mxc_nand.c | 56
> > > +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56
> > > insertions(+)
> > > 
> > > diff --git a/drivers/mtd/nand/mxc_nand.c
> > > b/drivers/mtd/nand/mxc_nand.c index 83bb01ab7079..6fbf98851a96
> > > 100644 --- a/drivers/mtd/nand/mxc_nand.c
> > > +++ b/drivers/mtd/nand/mxc_nand.c
> > > @@ -893,6 +893,59 @@ static int mxc_nand_read_oob(struct mtd_info
> > > *mtd, struct nand_chip *chip, page);
> > >  }
> > >  
> > > +static int __mxc_nand_write_page(struct mtd_info *mtd, struct
> > > nand_chip *chip,
> > > +				 const uint8_t *buf, int
> > > oob_required, int page) +{
> > > +	struct mxc_nand_host *host =
> > > nand_get_controller_data(chip); +
> > > +	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN,
> > > false);
> > > +	mxc_do_addr_cycle(mtd, 0, page);
> > > +
> > > +	memcpy32_toio(host->main_area0, buf, mtd->writesize);
> > > +	copy_spare(mtd, false, chip->oob_poi);  
> > 
> > If I remember correctly, there was a "spare only" mechanism to avoid
> > doing full page read/write when unnecessary. What about using it
> > here when (buf == NULL && oob_required)?  
> 
> You are probably referring to the SP_EN bit in the NAND_FLASH_CONFIG1
> register right? This is described as:
> 
> > NAND Flash spare enable. This bit determines whether host
> > reads/writes are to NAND Flash spare data only or NAND Flash main
> > and spare data. This feature is supported only with memories with
> > 1/2K page size. Note: There is no ECC in this mode of operation.
> > 0 NAND Flash main and spare data is enabled
> > 1 NAND Flash spare only data is enabled  
> 
> At least the i.MX25 has the limitation of 1/2k page size for this
> feature. It's not in the i.MX27 manual. I don't know if the i.MX27
> documentation is inaccurate or if this limitation is there already.

Ok, I ignored this limitation on i.MX25. Better write generic code
then.

> 
> Anyway, I don't think it's worth optimizing for this case given that
> we still need to support a full page write for 2k pages (which should
> be much more widespread nowadays than the 512b page size NANDs).

Sure!

Thanks,
Miquèl

Patch

diff --git a/drivers/mtd/nand/mxc_nand.c b/drivers/mtd/nand/mxc_nand.c
index 83bb01ab7079..6fbf98851a96 100644
--- a/drivers/mtd/nand/mxc_nand.c
+++ b/drivers/mtd/nand/mxc_nand.c
@@ -893,6 +893,59 @@  static int mxc_nand_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
 					     page);
 }
 
+static int __mxc_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+				 const uint8_t *buf, int oob_required, int page)
+{
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	host->devtype_data->send_cmd(host, NAND_CMD_SEQIN, false);
+	mxc_do_addr_cycle(mtd, 0, page);
+
+	memcpy32_toio(host->main_area0, buf, mtd->writesize);
+	copy_spare(mtd, false, chip->oob_poi);
+
+	host->devtype_data->send_page(mtd, NFC_INPUT);
+	host->devtype_data->send_cmd(host, NAND_CMD_PAGEPROG, true);
+	mxc_do_addr_cycle(mtd, 0, page);
+
+	return 0;
+}
+
+static int mxc_nand_write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			       const uint8_t *buf, int oob_required, int page)
+{
+	struct nand_chip *nand_chip = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	host->devtype_data->enable_hwecc(nand_chip, true);
+
+	return __mxc_nand_write_page(mtd, chip, buf, oob_required, page);
+}
+
+static int mxc_nand_write_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
+				   const uint8_t *buf, int oob_required, int page)
+{
+	struct nand_chip *nand_chip = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	host->devtype_data->enable_hwecc(nand_chip, false);
+
+	return __mxc_nand_write_page(mtd, chip, buf, oob_required, page);
+}
+
+static int mxc_nand_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
+			      int page)
+{
+	struct nand_chip *nand_chip = mtd_to_nand(mtd);
+	struct mxc_nand_host *host = nand_get_controller_data(chip);
+
+	memset(host->data_buf, 0xff, mtd->writesize);
+
+	host->devtype_data->enable_hwecc(nand_chip, false);
+
+	return __mxc_nand_write_page(mtd, chip, host->data_buf, 1, page);
+}
+
 static int mxc_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat,
 				  u_char *ecc_code)
 {
@@ -1907,6 +1960,9 @@  static int mxcnd_probe(struct platform_device *pdev)
 		this->ecc.read_page = mxc_nand_read_page;
 		this->ecc.read_page_raw = mxc_nand_read_page_raw;
 		this->ecc.read_oob = mxc_nand_read_oob;
+		this->ecc.write_page = mxc_nand_write_page;
+		this->ecc.write_page_raw = mxc_nand_write_page_raw;
+		this->ecc.write_oob = mxc_nand_write_oob;
 		this->ecc.calculate = mxc_nand_calculate_ecc;
 		this->ecc.hwctl = mxc_nand_enable_hwecc;
 		this->ecc.correct = host->devtype_data->correct_data;