[PATCH v3 0/3] mtd: spinand: add SPI-NAND MTD resume handler

[PATCH v3 0/3] mtd: spinand: add SPI-NAND MTD resume handler

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Changes in v3:
  - Add spinand_read_cfg() helper 
  - Add spinand_read_cfg() call to repopulate cache during resume
  - Split v2 patch in 3 patches

Changes in v2:
  - Add helper spinand_block_unlock().
  - Add spinand_ecc_enable() call.
  - Remove some dev_err().
  - Fix commit's title and message.

Patrice Chotard (3):
  mtd: spinand: Add spinand_block_unlock() helper
  mtd: spinand: add spinand_read_cfg() helper
  mtd: spinand: add SPI-NAND MTD resume handler

 drivers/mtd/nand/spi/core.c | 90 +++++++++++++++++++++++++++++--------
 1 file changed, 71 insertions(+), 19 deletions(-)

-- 
2.17.1

[PATCH v3 1/3] mtd: spinand: Add spinand_block_unlock() helper

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Put code responsible of block unlocking in spinand_block_unlock().
This function will be needed by the future SPI-NAND resume ops.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 drivers/mtd/nand/spi/core.c | 32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 17f63f95f4a2..095742e5bc2b 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1074,12 +1074,30 @@ static int spinand_detect(struct spinand_device *spinand)
 	return 0;
 }
 
+static int spinand_block_unlock(struct spinand_device *spinand)
+{
+	struct nand_device *nand = spinand_to_nand(spinand);
+	int ret = 0, i;
+
+	for (i = 0; i < nand->memorg.ntargets; i++) {
+		ret = spinand_select_target(spinand, i);
+		if (ret)
+			return ret;
+
+		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
+		if (ret)
+			return ret;
+	}
+
+	return ret;
+}
+
 static int spinand_init(struct spinand_device *spinand)
 {
 	struct device *dev = &spinand->spimem->spi->dev;
 	struct mtd_info *mtd = spinand_to_mtd(spinand);
 	struct nand_device *nand = mtd_to_nanddev(mtd);
-	int ret, i;
+	int ret;
 
 	/*
 	 * We need a scratch buffer because the spi_mem interface requires that
@@ -1137,15 +1155,9 @@ static int spinand_init(struct spinand_device *spinand)
 	}
 
 	/* After power up, all blocks are locked, so unlock them here. */
-	for (i = 0; i < nand->memorg.ntargets; i++) {
-		ret = spinand_select_target(spinand, i);
-		if (ret)
-			goto err_manuf_cleanup;
-
-		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
-		if (ret)
-			goto err_manuf_cleanup;
-	}
+	ret = spinand_block_unlock(spinand);
+	if (ret)
+		goto err_manuf_cleanup;
 
 	ret = nanddev_init(nand, &spinand_ops, THIS_MODULE);
 	if (ret)
-- 
2.17.1

[PATCH v3 2/3] mtd: spinand: add spinand_read_cfg() helper

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

Put REG_CFG reading code in spinand_read_cfg().
This function will be needed by the future SPI-NAND resume ops.

Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
 drivers/mtd/nand/spi/core.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 095742e5bc2b..1f699ad84f1b 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -138,20 +138,12 @@ int spinand_select_target(struct spinand_device *spinand, unsigned int target)
 	return 0;
 }
 
-static int spinand_init_cfg_cache(struct spinand_device *spinand)
+static int spinand_read_cfg(struct spinand_device *spinand)
 {
 	struct nand_device *nand = spinand_to_nand(spinand);
-	struct device *dev = &spinand->spimem->spi->dev;
 	unsigned int target;
 	int ret;
 
-	spinand->cfg_cache = devm_kcalloc(dev,
-					  nand->memorg.ntargets,
-					  sizeof(*spinand->cfg_cache),
-					  GFP_KERNEL);
-	if (!spinand->cfg_cache)
-		return -ENOMEM;
-
 	for (target = 0; target < nand->memorg.ntargets; target++) {
 		ret = spinand_select_target(spinand, target);
 		if (ret)
@@ -170,6 +162,21 @@ static int spinand_init_cfg_cache(struct spinand_device *spinand)
 	return 0;
 }
 
+static int spinand_init_cfg_cache(struct spinand_device *spinand)
+{
+	struct nand_device *nand = spinand_to_nand(spinand);
+	struct device *dev = &spinand->spimem->spi->dev;
+
+	spinand->cfg_cache = devm_kcalloc(dev,
+					  nand->memorg.ntargets,
+					  sizeof(*spinand->cfg_cache),
+					  GFP_KERNEL);
+	if (!spinand->cfg_cache)
+		return -ENOMEM;
+
+	return spinand_read_cfg(spinand);
+}
+
 static int spinand_init_quad_enable(struct spinand_device *spinand)
 {
 	bool enable = false;
-- 
2.17.1

[PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[patrice.chotard]

From: Patrice Chotard <patrice.chotard@foss.st.com>

After power up, all SPI NAND's blocks are locked. Only read operations
are allowed, write and erase operations are forbidden.
The SPI NAND framework unlocks all the blocks during its initialization.

During a standby low power, the memory is powered down, losing its
configuration.
During the resume, the QSPI driver state is restored but the SPI NAND
framework does not reconfigured the memory.

This patch adds SPI-NAND MTD PM handlers for resume ops.
SPI NAND resume op re-initializes SPI NAND flash to its probed state.

Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
---
Changes in v3:
  - Add spinand_read_cfg() call to repopulate cache

Changes in v2:
  - Add helper spinand_block_unlock().
  - Add spinand_ecc_enable() call.
  - Remove some dev_err().
  - Fix commit's title and message.

 drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 1f699ad84f1b..e3fcbcf381c3 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
 	return ret;
 }
 
+static void spinand_mtd_resume(struct mtd_info *mtd)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	int ret;
+
+	ret = spinand_reset_op(spinand);
+	if (ret)
+		return;
+
+	ret = spinand_read_cfg(spinand);
+	if (ret)
+		return;
+
+	ret = spinand_init_quad_enable(spinand);
+	if (ret)
+		return;
+
+	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
+	if (ret)
+		return;
+
+	ret = spinand_manufacturer_init(spinand);
+	if (ret)
+		return;
+
+	ret = spinand_block_unlock(spinand);
+	if (ret)
+		return;
+
+	spinand_ecc_enable(spinand, false);
+}
+
 static int spinand_init(struct spinand_device *spinand)
 {
 	struct device *dev = &spinand->spimem->spi->dev;
@@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
 	mtd->_block_isreserved = spinand_mtd_block_isreserved;
 	mtd->_erase = spinand_mtd_erase;
 	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
+	mtd->_resume = spinand_mtd_resume;
 
 	if (nand->ecc.engine) {
 		ret = mtd_ooblayout_count_freebytes(mtd);
-- 
2.17.1

Re: [PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[Miquel Raynal]

Hi Patrice,

+ Pratyush

<patrice.chotard@foss.st.com> wrote on Thu, 27 May 2021 18:12:52 +0200:

> From: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> After power up, all SPI NAND's blocks are locked. Only read operations
> are allowed, write and erase operations are forbidden.
> The SPI NAND framework unlocks all the blocks during its initialization.
> 
> During a standby low power, the memory is powered down, losing its
> configuration.
> During the resume, the QSPI driver state is restored but the SPI NAND
> framework does not reconfigured the memory.
> 
> This patch adds SPI-NAND MTD PM handlers for resume ops.
> SPI NAND resume op re-initializes SPI NAND flash to its probed state.
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> ---
> Changes in v3:
>   - Add spinand_read_cfg() call to repopulate cache
> 
> Changes in v2:
>   - Add helper spinand_block_unlock().
>   - Add spinand_ecc_enable() call.
>   - Remove some dev_err().
>   - Fix commit's title and message.
> 
>  drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 1f699ad84f1b..e3fcbcf381c3 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
>  	return ret;
>  }
>  
> +static void spinand_mtd_resume(struct mtd_info *mtd)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	int ret;
> +
> +	ret = spinand_reset_op(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_read_cfg(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_init_quad_enable(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_manufacturer_init(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_block_unlock(spinand);
> +	if (ret)
> +		return;
> +
> +	spinand_ecc_enable(spinand, false);
> +}

Sorry for not being clear, but I think what Pratyush meant was that
you could create a helper doing all the common initializations between
spinand_init() and spinand_resume() and call it from these places to
avoid code duplication. His comment somehow outclassed mine as I only
focused on the unlock part (which I think is clearer anyway, please keep
it like that).

> +
>  static int spinand_init(struct spinand_device *spinand)
>  {
>  	struct device *dev = &spinand->spimem->spi->dev;
> @@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
>  	mtd->_block_isreserved = spinand_mtd_block_isreserved;
>  	mtd->_erase = spinand_mtd_erase;
>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
> +	mtd->_resume = spinand_mtd_resume;
>  
>  	if (nand->ecc.engine) {
>  		ret = mtd_ooblayout_count_freebytes(mtd);

Thanks,
Miquèl

Re: [PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[Patrice CHOTARD]

Hi Miquel

On 5/27/21 6:23 PM, Miquel Raynal wrote:
> Hi Patrice,
> 
> + Pratyush
> 
> <patrice.chotard@foss.st.com> wrote on Thu, 27 May 2021 18:12:52 +0200:
> 
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> After power up, all SPI NAND's blocks are locked. Only read operations
>> are allowed, write and erase operations are forbidden.
>> The SPI NAND framework unlocks all the blocks during its initialization.
>>
>> During a standby low power, the memory is powered down, losing its
>> configuration.
>> During the resume, the QSPI driver state is restored but the SPI NAND
>> framework does not reconfigured the memory.
>>
>> This patch adds SPI-NAND MTD PM handlers for resume ops.
>> SPI NAND resume op re-initializes SPI NAND flash to its probed state.
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> ---
>> Changes in v3:
>>   - Add spinand_read_cfg() call to repopulate cache
>>
>> Changes in v2:
>>   - Add helper spinand_block_unlock().
>>   - Add spinand_ecc_enable() call.
>>   - Remove some dev_err().
>>   - Fix commit's title and message.
>>
>>  drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 1f699ad84f1b..e3fcbcf381c3 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
>>  	return ret;
>>  }
>>  
>> +static void spinand_mtd_resume(struct mtd_info *mtd)
>> +{
>> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
>> +	int ret;
>> +
>> +	ret = spinand_reset_op(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_read_cfg(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_init_quad_enable(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_manufacturer_init(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_block_unlock(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	spinand_ecc_enable(spinand, false);
>> +}
> 
> Sorry for not being clear, but I think what Pratyush meant was that
> you could create a helper doing all the common initializations between
> spinand_init() and spinand_resume() and call it from these places to
> avoid code duplication. His comment somehow outclassed mine as I only
> focused on the unlock part (which I think is clearer anyway, please keep
> it like that).
> 

My bad too, i read too quickly Pratyush's asnwer.
I am preparing a v4.

Thanks
Patrice


>> +
>>  static int spinand_init(struct spinand_device *spinand)
>>  {
>>  	struct device *dev = &spinand->spimem->spi->dev;
>> @@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
>>  	mtd->_block_isreserved = spinand_mtd_block_isreserved;
>>  	mtd->_erase = spinand_mtd_erase;
>>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
>> +	mtd->_resume = spinand_mtd_resume;
>>  
>>  	if (nand->ecc.engine) {
>>  		ret = mtd_ooblayout_count_freebytes(mtd);
> 
> Thanks,
> Miquèl
> 

Re: [PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[Pratyush Yadav]

On 27/05/21 06:12PM, patrice.chotard@foss.st.com wrote:
> From: Patrice Chotard <patrice.chotard@foss.st.com>
> 
> After power up, all SPI NAND's blocks are locked. Only read operations
> are allowed, write and erase operations are forbidden.
> The SPI NAND framework unlocks all the blocks during its initialization.
> 
> During a standby low power, the memory is powered down, losing its
> configuration.
> During the resume, the QSPI driver state is restored but the SPI NAND
> framework does not reconfigured the memory.
> 
> This patch adds SPI-NAND MTD PM handlers for resume ops.
> SPI NAND resume op re-initializes SPI NAND flash to its probed state.
> 
> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> ---
> Changes in v3:
>   - Add spinand_read_cfg() call to repopulate cache
> 
> Changes in v2:
>   - Add helper spinand_block_unlock().
>   - Add spinand_ecc_enable() call.
>   - Remove some dev_err().
>   - Fix commit's title and message.
> 
>  drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 1f699ad84f1b..e3fcbcf381c3 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
>  	return ret;
>  }
>  
> +static void spinand_mtd_resume(struct mtd_info *mtd)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	int ret;
> +
> +	ret = spinand_reset_op(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_read_cfg(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_init_quad_enable(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_manufacturer_init(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_block_unlock(spinand);
> +	if (ret)
> +		return;
> +
> +	spinand_ecc_enable(spinand, false);
> +}
> +

I don't think you quite get what me and Miquel are suggesting.

The helper should call all these functions like read_cfg() 
quad_enable(), etc. So it should look something like:

int spinand_init_flash()
{
	ret = spinand_read_cfg(spinand);
	if (ret)
		return;

	ret = spinand_init_quad_enable(spinand);
	if (ret)
		return;

	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
	if (ret)
		return;

	ret = spinand_manufacturer_init(spinand);
	if (ret)
		return;

	ret = spinand_block_unlock(spinand);
	if (ret)
		return;

	spinand_ecc_enable(spinand, false);
}

Then spinand_mtd_resume should look something like:

int spinand_mtd_resume()
{
	ret = spinand_reset_op(spinand);
	if (ret)
		return;

	return spinand_init_flash();
}

And spinand_init() should look something like:

int spinand_init()
{
	...
	spinand->oobbuf = ...

	spinand_init_flash();

	spinand_create_dirmaps();

	...
}


>  static int spinand_init(struct spinand_device *spinand)
>  {
>  	struct device *dev = &spinand->spimem->spi->dev;
> @@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
>  	mtd->_block_isreserved = spinand_mtd_block_isreserved;
>  	mtd->_erase = spinand_mtd_erase;
>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
> +	mtd->_resume = spinand_mtd_resume;
>  
>  	if (nand->ecc.engine) {
>  		ret = mtd_ooblayout_count_freebytes(mtd);
> -- 
> 2.17.1

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[Patrice CHOTARD]

Hi Pratyush

On 5/28/21 2:05 PM, Pratyush Yadav wrote:
> On 27/05/21 06:12PM, patrice.chotard@foss.st.com wrote:
>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>
>> After power up, all SPI NAND's blocks are locked. Only read operations
>> are allowed, write and erase operations are forbidden.
>> The SPI NAND framework unlocks all the blocks during its initialization.
>>
>> During a standby low power, the memory is powered down, losing its
>> configuration.
>> During the resume, the QSPI driver state is restored but the SPI NAND
>> framework does not reconfigured the memory.
>>
>> This patch adds SPI-NAND MTD PM handlers for resume ops.
>> SPI NAND resume op re-initializes SPI NAND flash to its probed state.
>>
>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>> ---
>> Changes in v3:
>>   - Add spinand_read_cfg() call to repopulate cache
>>
>> Changes in v2:
>>   - Add helper spinand_block_unlock().
>>   - Add spinand_ecc_enable() call.
>>   - Remove some dev_err().
>>   - Fix commit's title and message.
>>
>>  drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 1f699ad84f1b..e3fcbcf381c3 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
>>  	return ret;
>>  }
>>  
>> +static void spinand_mtd_resume(struct mtd_info *mtd)
>> +{
>> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
>> +	int ret;
>> +
>> +	ret = spinand_reset_op(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_read_cfg(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_init_quad_enable(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_manufacturer_init(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_block_unlock(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	spinand_ecc_enable(spinand, false);
>> +}
>> +
> 
> I don't think you quite get what me and Miquel are suggesting.
> 
> The helper should call all these functions like read_cfg() 
> quad_enable(), etc. So it should look something like:

Yes, this series was sent too quickly on my side, and i misunderstood 
what you suggested, sorry for that.

> 
> int spinand_init_flash()
> {
> 	ret = spinand_read_cfg(spinand);
> 	if (ret)
> 		return;
> 

The new helper spinand_read_cfg() must not be called in spinand_init_flash()
but directly in spinand_resume().
This because spinand_read_cfg() only performs a REG_CFG read without doing the
memory allocation of spinand->cfg_cache.

In spinand_init(), spinand_init_cfg_cache() must be called as previously as it does spinand->cfg_cache
memory allocation and call the new helper spinand_read_cfg(). 
Then after, spinand_init_flash() can be called.

> 	ret = spinand_init_quad_enable(spinand);
> 	if (ret)
> 		return;
> 
> 	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> 	if (ret)
> 		return;
> 
> 	ret = spinand_manufacturer_init(spinand);
> 	if (ret)
> 		return;
> 
> 	ret = spinand_block_unlock(spinand);
> 	if (ret)
> 		return;
> 
> 	spinand_ecc_enable(spinand, false);
> }
> 
> Then spinand_mtd_resume should look something like:
> 
> int spinand_mtd_resume()
> {
> 	ret = spinand_reset_op(spinand);
> 	if (ret)
> 		return;
> 
> 	return spinand_init_flash();
> }
> 
> And spinand_init() should look something like:
> 
> int spinand_init()
> {
> 	...
> 	spinand->oobbuf = ...
> 
> 	spinand_init_flash();
> 
> 	spinand_create_dirmaps();
> 
> 	...

As explained just above, spinand_init() will look like :

int spinand_init()
{
	...
	spinand->oobbuf = ...

	spinand_init_cfg_cache();     => perform cfg cache memory allocation and read the REG_CFG

	spinand_init_flash();

	spinand_create_dirmaps();

> }
> 
> 
>>  static int spinand_init(struct spinand_device *spinand)
>>  {
>>  	struct device *dev = &spinand->spimem->spi->dev;
>> @@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
>>  	mtd->_block_isreserved = spinand_mtd_block_isreserved;
>>  	mtd->_erase = spinand_mtd_erase;
>>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
>> +	mtd->_resume = spinand_mtd_resume;
>>  
>>  	if (nand->ecc.engine) {
>>  		ret = mtd_ooblayout_count_freebytes(mtd);
>> -- 
>> 2.17.1
> 

Thanks
Patrice

Re: [PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[Pratyush Yadav]

On 28/05/21 02:51PM, Patrice CHOTARD wrote:
> Hi Pratyush
> 
> On 5/28/21 2:05 PM, Pratyush Yadav wrote:
> > On 27/05/21 06:12PM, patrice.chotard@foss.st.com wrote:
> >> From: Patrice Chotard <patrice.chotard@foss.st.com>
> >>
> >> After power up, all SPI NAND's blocks are locked. Only read operations
> >> are allowed, write and erase operations are forbidden.
> >> The SPI NAND framework unlocks all the blocks during its initialization.
> >>
> >> During a standby low power, the memory is powered down, losing its
> >> configuration.
> >> During the resume, the QSPI driver state is restored but the SPI NAND
> >> framework does not reconfigured the memory.
> >>
> >> This patch adds SPI-NAND MTD PM handlers for resume ops.
> >> SPI NAND resume op re-initializes SPI NAND flash to its probed state.
> >>
> >> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
> >> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
> >> ---
> >> Changes in v3:
> >>   - Add spinand_read_cfg() call to repopulate cache
> >>
> >> Changes in v2:
> >>   - Add helper spinand_block_unlock().
> >>   - Add spinand_ecc_enable() call.
> >>   - Remove some dev_err().
> >>   - Fix commit's title and message.
> >>
> >>  drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
> >>  1 file changed, 33 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> >> index 1f699ad84f1b..e3fcbcf381c3 100644
> >> --- a/drivers/mtd/nand/spi/core.c
> >> +++ b/drivers/mtd/nand/spi/core.c
> >> @@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
> >>  	return ret;
> >>  }
> >>  
> >> +static void spinand_mtd_resume(struct mtd_info *mtd)
> >> +{
> >> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> >> +	int ret;
> >> +
> >> +	ret = spinand_reset_op(spinand);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	ret = spinand_read_cfg(spinand);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	ret = spinand_init_quad_enable(spinand);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	ret = spinand_manufacturer_init(spinand);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	ret = spinand_block_unlock(spinand);
> >> +	if (ret)
> >> +		return;
> >> +
> >> +	spinand_ecc_enable(spinand, false);
> >> +}
> >> +
> > 
> > I don't think you quite get what me and Miquel are suggesting.
> > 
> > The helper should call all these functions like read_cfg() 
> > quad_enable(), etc. So it should look something like:
> 
> Yes, this series was sent too quickly on my side, and i misunderstood 
> what you suggested, sorry for that.

Ah, I thought this was the v4. I see Miquel clarified already in reply 
to this patch.

> 
> > 
> > int spinand_init_flash()
> > {
> > 	ret = spinand_read_cfg(spinand);
> > 	if (ret)
> > 		return;
> > 
> 
> The new helper spinand_read_cfg() must not be called in spinand_init_flash()
> but directly in spinand_resume().
> This because spinand_read_cfg() only performs a REG_CFG read without doing the
> memory allocation of spinand->cfg_cache.
> 
> In spinand_init(), spinand_init_cfg_cache() must be called as previously as it does spinand->cfg_cache
> memory allocation and call the new helper spinand_read_cfg(). 
> Then after, spinand_init_flash() can be called.

I think it would be simpler for spinand_init_cfg_cache() to just 
allocate the spinand->cfg_cache array and then for spinand_init_flash() 
to call spinand_read_cfg().

> 
> > 	ret = spinand_init_quad_enable(spinand);
> > 	if (ret)
> > 		return;
> > 
> > 	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> > 	if (ret)
> > 		return;
> > 
> > 	ret = spinand_manufacturer_init(spinand);
> > 	if (ret)
> > 		return;
> > 
> > 	ret = spinand_block_unlock(spinand);
> > 	if (ret)
> > 		return;
> > 
> > 	spinand_ecc_enable(spinand, false);
> > }
> > 
> > Then spinand_mtd_resume should look something like:
> > 
> > int spinand_mtd_resume()
> > {
> > 	ret = spinand_reset_op(spinand);
> > 	if (ret)
> > 		return;
> > 
> > 	return spinand_init_flash();
> > }
> > 
> > And spinand_init() should look something like:
> > 
> > int spinand_init()
> > {
> > 	...
> > 	spinand->oobbuf = ...
> > 
> > 	spinand_init_flash();
> > 
> > 	spinand_create_dirmaps();
> > 
> > 	...
> 
> As explained just above, spinand_init() will look like :
> 
> int spinand_init()
> {
> 	...
> 	spinand->oobbuf = ...
> 
> 	spinand_init_cfg_cache();     => perform cfg cache memory allocation and read the REG_CFG
> 
> 	spinand_init_flash();
> 
> 	spinand_create_dirmaps();
> 
> > }
> > 
> > 
> >>  static int spinand_init(struct spinand_device *spinand)
> >>  {
> >>  	struct device *dev = &spinand->spimem->spi->dev;
> >> @@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
> >>  	mtd->_block_isreserved = spinand_mtd_block_isreserved;
> >>  	mtd->_erase = spinand_mtd_erase;
> >>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
> >> +	mtd->_resume = spinand_mtd_resume;
> >>  
> >>  	if (nand->ecc.engine) {
> >>  		ret = mtd_ooblayout_count_freebytes(mtd);
> >> -- 
> >> 2.17.1
> > 
> 
> Thanks
> Patrice

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: [PATCH v3 3/3] mtd: spinand: add SPI-NAND MTD resume handler

[Patrice CHOTARD]

On 5/28/21 3:17 PM, Pratyush Yadav wrote:
> On 28/05/21 02:51PM, Patrice CHOTARD wrote:
>> Hi Pratyush
>>
>> On 5/28/21 2:05 PM, Pratyush Yadav wrote:
>>> On 27/05/21 06:12PM, patrice.chotard@foss.st.com wrote:
>>>> From: Patrice Chotard <patrice.chotard@foss.st.com>
>>>>
>>>> After power up, all SPI NAND's blocks are locked. Only read operations
>>>> are allowed, write and erase operations are forbidden.
>>>> The SPI NAND framework unlocks all the blocks during its initialization.
>>>>
>>>> During a standby low power, the memory is powered down, losing its
>>>> configuration.
>>>> During the resume, the QSPI driver state is restored but the SPI NAND
>>>> framework does not reconfigured the memory.
>>>>
>>>> This patch adds SPI-NAND MTD PM handlers for resume ops.
>>>> SPI NAND resume op re-initializes SPI NAND flash to its probed state.
>>>>
>>>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com>
>>>> Signed-off-by: Patrice Chotard <patrice.chotard@foss.st.com>
>>>> ---
>>>> Changes in v3:
>>>>   - Add spinand_read_cfg() call to repopulate cache
>>>>
>>>> Changes in v2:
>>>>   - Add helper spinand_block_unlock().
>>>>   - Add spinand_ecc_enable() call.
>>>>   - Remove some dev_err().
>>>>   - Fix commit's title and message.
>>>>
>>>>  drivers/mtd/nand/spi/core.c | 33 +++++++++++++++++++++++++++++++++
>>>>  1 file changed, 33 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>>>> index 1f699ad84f1b..e3fcbcf381c3 100644
>>>> --- a/drivers/mtd/nand/spi/core.c
>>>> +++ b/drivers/mtd/nand/spi/core.c
>>>> @@ -1099,6 +1099,38 @@ static int spinand_block_unlock(struct spinand_device *spinand)
>>>>  	return ret;
>>>>  }
>>>>  
>>>> +static void spinand_mtd_resume(struct mtd_info *mtd)
>>>> +{
>>>> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
>>>> +	int ret;
>>>> +
>>>> +	ret = spinand_reset_op(spinand);
>>>> +	if (ret)
>>>> +		return;
>>>> +
>>>> +	ret = spinand_read_cfg(spinand);
>>>> +	if (ret)
>>>> +		return;
>>>> +
>>>> +	ret = spinand_init_quad_enable(spinand);
>>>> +	if (ret)
>>>> +		return;
>>>> +
>>>> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>>>> +	if (ret)
>>>> +		return;
>>>> +
>>>> +	ret = spinand_manufacturer_init(spinand);
>>>> +	if (ret)
>>>> +		return;
>>>> +
>>>> +	ret = spinand_block_unlock(spinand);
>>>> +	if (ret)
>>>> +		return;
>>>> +
>>>> +	spinand_ecc_enable(spinand, false);
>>>> +}
>>>> +
>>>
>>> I don't think you quite get what me and Miquel are suggesting.
>>>
>>> The helper should call all these functions like read_cfg() 
>>> quad_enable(), etc. So it should look something like:
>>
>> Yes, this series was sent too quickly on my side, and i misunderstood 
>> what you suggested, sorry for that.
> 
> Ah, I thought this was the v4. I see Miquel clarified already in reply 
> to this patch.
> 
>>
>>>
>>> int spinand_init_flash()
>>> {
>>> 	ret = spinand_read_cfg(spinand);
>>> 	if (ret)
>>> 		return;
>>>
>>
>> The new helper spinand_read_cfg() must not be called in spinand_init_flash()
>> but directly in spinand_resume().
>> This because spinand_read_cfg() only performs a REG_CFG read without doing the
>> memory allocation of spinand->cfg_cache.
>>
>> In spinand_init(), spinand_init_cfg_cache() must be called as previously as it does spinand->cfg_cache
>> memory allocation and call the new helper spinand_read_cfg(). 
>> Then after, spinand_init_flash() can be called.
> 
> I think it would be simpler for spinand_init_cfg_cache() to just 
> allocate the spinand->cfg_cache array and then for spinand_init_flash() 
> to call spinand_read_cfg().

Argh, i just send the v5 ;-) .... i will wait the full v5 review and will follow your proposal 
regarding spinand_init_cfg_cache().

Patrice

> 
>>
>>> 	ret = spinand_init_quad_enable(spinand);
>>> 	if (ret)
>>> 		return;
>>>
>>> 	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>>> 	if (ret)
>>> 		return;
>>>
>>> 	ret = spinand_manufacturer_init(spinand);
>>> 	if (ret)
>>> 		return;
>>>
>>> 	ret = spinand_block_unlock(spinand);
>>> 	if (ret)
>>> 		return;
>>>
>>> 	spinand_ecc_enable(spinand, false);
>>> }
>>>
>>> Then spinand_mtd_resume should look something like:
>>>
>>> int spinand_mtd_resume()
>>> {
>>> 	ret = spinand_reset_op(spinand);
>>> 	if (ret)
>>> 		return;
>>>
>>> 	return spinand_init_flash();
>>> }
>>>
>>> And spinand_init() should look something like:
>>>
>>> int spinand_init()
>>> {
>>> 	...
>>> 	spinand->oobbuf = ...
>>>
>>> 	spinand_init_flash();
>>>
>>> 	spinand_create_dirmaps();
>>>
>>> 	...
>>
>> As explained just above, spinand_init() will look like :
>>
>> int spinand_init()
>> {
>> 	...
>> 	spinand->oobbuf = ...
>>
>> 	spinand_init_cfg_cache();     => perform cfg cache memory allocation and read the REG_CFG
>>
>> 	spinand_init_flash();
>>
>> 	spinand_create_dirmaps();
>>
>>> }
>>>
>>>
>>>>  static int spinand_init(struct spinand_device *spinand)
>>>>  {
>>>>  	struct device *dev = &spinand->spimem->spi->dev;
>>>> @@ -1186,6 +1218,7 @@ static int spinand_init(struct spinand_device *spinand)
>>>>  	mtd->_block_isreserved = spinand_mtd_block_isreserved;
>>>>  	mtd->_erase = spinand_mtd_erase;
>>>>  	mtd->_max_bad_blocks = nanddev_mtd_max_bad_blocks;
>>>> +	mtd->_resume = spinand_mtd_resume;
>>>>  
>>>>  	if (nand->ecc.engine) {
>>>>  		ret = mtd_ooblayout_count_freebytes(mtd);
>>>> -- 
>>>> 2.17.1
>>>
>>
>> Thanks
>> Patrice
>