mtd: spinand: add spi nand mtd resume handler

mtd: spinand: add spi nand mtd resume handler

[patrice.chotard]

From: Christophe Kerello <christophe.kerello@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>
---
 drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
index 17f63f95f4a2..6abaf874eb3f 100644
--- a/drivers/mtd/nand/spi/core.c
+++ b/drivers/mtd/nand/spi/core.c
@@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
 	return 0;
 }
 
+static void spinand_mtd_resume(struct mtd_info *mtd)
+{
+	struct spinand_device *spinand = mtd_to_spinand(mtd);
+	struct nand_device *nand = mtd_to_nanddev(mtd);
+	struct device *dev = &spinand->spimem->spi->dev;
+	int ret, i;
+
+	ret = spinand_reset_op(spinand);
+	if (ret)
+		return;
+
+	ret = spinand_init_quad_enable(spinand);
+	if (ret) {
+		dev_err(dev,
+			"Failed to initialize the quad part (err = %d)\n",
+			ret);
+		return;
+	}
+
+	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
+	if (ret) {
+		dev_err(dev,
+			"Failed to updtae the OTP (err = %d)\n",
+			ret);
+		return;
+	}
+
+	ret = spinand_manufacturer_init(spinand);
+	if (ret) {
+		dev_err(dev,
+			"Failed to initialize the SPI NAND chip (err = %d)\n",
+			ret);
+		return;
+	}
+
+	/* 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) {
+			dev_err(dev,
+				"Failed to select the target (err = %d)\n",
+				ret);
+			return;
+		}
+
+		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
+		if (ret) {
+			dev_err(dev,
+				"Failed to unlock block (err = %d)\n",
+				ret);
+			return;
+		}
+	}
+}
+
 static int spinand_init(struct spinand_device *spinand)
 {
 	struct device *dev = &spinand->spimem->spi->dev;
@@ -1167,6 +1222,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: mtd: spinand: add spi nand mtd resume handler

[Miquel Raynal]

Hello,

<patrice.chotard@foss.st.com> wrote on Wed, 26 May 2021 17:30:16 +0200:

> From: Christophe Kerello <christophe.kerello@foss.st.com>

Would you mind to use "add SPI-NAND MTD resume handler" as title? (with
upper case letters)

> 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.

ditto             ^^^^^^^^^^^^

> 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>
> ---
>  drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 17f63f95f4a2..6abaf874eb3f 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
>  	return 0;
>  }
>  
> +static void spinand_mtd_resume(struct mtd_info *mtd)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	struct nand_device *nand = mtd_to_nanddev(mtd);
> +	struct device *dev = &spinand->spimem->spi->dev;
> +	int ret, i;
> +
> +	ret = spinand_reset_op(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_init_quad_enable(spinand);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to initialize the quad part (err = %d)\n",

quad part? what about "Failed to resume the quad state" or something
alike?

> +			ret);
> +		return;
> +	}
> +
> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to updtae the OTP (err = %d)\n",

update

> +			ret);
> +		return;
> +	}
> +
> +	ret = spinand_manufacturer_init(spinand);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to initialize the SPI NAND chip (err = %d)\n",
> +			ret);
> +		return;
> +	}
> +
> +	/* 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) {
> +			dev_err(dev,
> +				"Failed to select the target (err = %d)\n",
> +				ret);
> +			return;
> +		}
> +
> +		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to unlock block (err = %d)\n",
> +				ret);
> +			return;
> +		}
> +	}

I bet this would deserve a helper as this is the exact same peace of
code that is being run in spinnand_init()?

At the very least I think that spinand_ecc_enable(spinand, false);
should be called.

Ideally, a resume operation should be provided by ECC engines, but that
can be added later.

> +}
> +
>  static int spinand_init(struct spinand_device *spinand)
>  {
>  	struct device *dev = &spinand->spimem->spi->dev;
> @@ -1167,6 +1222,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: mtd: spinand: add spi nand mtd resume handler

[Patrice CHOTARD]

Hi Miquel

On 5/26/21 5:42 PM, Miquel Raynal wrote:
> Hello,
> 
> <patrice.chotard@foss.st.com> wrote on Wed, 26 May 2021 17:30:16 +0200:
> 
>> From: Christophe Kerello <christophe.kerello@foss.st.com>
> 
> Would you mind to use "add SPI-NAND MTD resume handler" as title? (with
> upper case letters)

Ok

> 
>> 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.
> 
> ditto             ^^^^^^^^^^^^

Ok

> 
>> 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>
>> ---
>>  drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 17f63f95f4a2..6abaf874eb3f 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
>>  	return 0;
>>  }
>>  
>> +static void spinand_mtd_resume(struct mtd_info *mtd)
>> +{
>> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
>> +	struct nand_device *nand = mtd_to_nanddev(mtd);
>> +	struct device *dev = &spinand->spimem->spi->dev;
>> +	int ret, i;
>> +
>> +	ret = spinand_reset_op(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_init_quad_enable(spinand);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to initialize the quad part (err = %d)\n",
> 
> quad part? what about "Failed to resume the quad state" or something
> alike?

Agree, i will update this

> 
>> +			ret);
>> +		return;
>> +	}
>> +
>> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to updtae the OTP (err = %d)\n",
> 
> update

ok

> 
>> +			ret);
>> +		return;
>> +	}
>> +
>> +	ret = spinand_manufacturer_init(spinand);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to initialize the SPI NAND chip (err = %d)\n",
>> +			ret);
>> +		return;
>> +	}
>> +
>> +	/* 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) {
>> +			dev_err(dev,
>> +				"Failed to select the target (err = %d)\n",
>> +				ret);
>> +			return;
>> +		}
>> +
>> +		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
>> +		if (ret) {
>> +			dev_err(dev,
>> +				"Failed to unlock block (err = %d)\n",
>> +				ret);
>> +			return;
>> +		}
>> +	}
> 
> I bet this would deserve a helper as this is the exact same peace of
> code that is being run in spinnand_init()?

I will add a new function spinand_block_unlock(struct spinand_device *spinand)

> 
> At the very least I think that spinand_ecc_enable(spinand, false);
> should be called.
I will add it. 

> 
> Ideally, a resume operation should be provided by ECC engines, but that
> can be added later.
> 
>> +}
>> +
>>  static int spinand_init(struct spinand_device *spinand)
>>  {
>>  	struct device *dev = &spinand->spimem->spi->dev;
>> @@ -1167,6 +1222,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
> 
Thanks
Patrice

Re: [Linux-stm32] mtd: spinand: add spi nand mtd resume handler

[Patrice CHOTARD]

On 5/27/21 9:01 AM, Patrice CHOTARD wrote:
> Hi Miquel
> 
> On 5/26/21 5:42 PM, Miquel Raynal wrote:
>> Hello,
>>
>> <patrice.chotard@foss.st.com> wrote on Wed, 26 May 2021 17:30:16 +0200:
>>
>>> From: Christophe Kerello <christophe.kerello@foss.st.com>
>>
>> Would you mind to use "add SPI-NAND MTD resume handler" as title? (with
>> upper case letters)
> 
> Ok
> 
>>
>>> 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.
>>
>> ditto             ^^^^^^^^^^^^
> 
> Ok
> 
>>
>>> 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>
>>> ---
>>>  drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>>> index 17f63f95f4a2..6abaf874eb3f 100644
>>> --- a/drivers/mtd/nand/spi/core.c
>>> +++ b/drivers/mtd/nand/spi/core.c
>>> @@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
>>>  	return 0;
>>>  }
>>>  
>>> +static void spinand_mtd_resume(struct mtd_info *mtd)
>>> +{
>>> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
>>> +	struct nand_device *nand = mtd_to_nanddev(mtd);
>>> +	struct device *dev = &spinand->spimem->spi->dev;
>>> +	int ret, i;
>>> +
>>> +	ret = spinand_reset_op(spinand);
>>> +	if (ret)
>>> +		return;
>>> +
>>> +	ret = spinand_init_quad_enable(spinand);
>>> +	if (ret) {
>>> +		dev_err(dev,
>>> +			"Failed to initialize the quad part (err = %d)\n",
>>
>> quad part? what about "Failed to resume the quad state" or something
>> alike?
> 
> Agree, i will update this

I will even remove the dev_err(), as in spinand_init() 
no dev_err() was added on spinand_init_quad_enable() error.

> 
>>
>>> +			ret);
>>> +		return;
>>> +	}
>>> +
>>> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>>> +	if (ret) {
>>> +		dev_err(dev,
>>> +			"Failed to updtae the OTP (err = %d)\n",
>>
>> update
> 
> ok

ditto

> 
>>
>>> +			ret);
>>> +		return;
>>> +	}
>>> +
>>> +	ret = spinand_manufacturer_init(spinand);
>>> +	if (ret) {
>>> +		dev_err(dev,
>>> +			"Failed to initialize the SPI NAND chip (err = %d)\n",
>>> +			ret);
>>> +		return;
>>> +	}
>>> +
>>> +	/* 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) {
>>> +			dev_err(dev,
>>> +				"Failed to select the target (err = %d)\n",
>>> +				ret);

ditto

>>> +			return;
>>> +		}
>>> +
>>> +		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
>>> +		if (ret) {
>>> +			dev_err(dev,
>>> +				"Failed to unlock block (err = %d)\n",
>>> +				ret);

ditto

>>> +			return;
>>> +		}
>>> +	}
>>
>> I bet this would deserve a helper as this is the exact same peace of
>> code that is being run in spinnand_init()?
> 
> I will add a new function spinand_block_unlock(struct spinand_device *spinand)
> 
>>
>> At the very least I think that spinand_ecc_enable(spinand, false);
>> should be called.
> I will add it. 
> 
>>
>> Ideally, a resume operation should be provided by ECC engines, but that
>> can be added later.
>>
>>> +}
>>> +
>>>  static int spinand_init(struct spinand_device *spinand)
>>>  {
>>>  	struct device *dev = &spinand->spimem->spi->dev;
>>> @@ -1167,6 +1222,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
>>
> Thanks
> Patrice
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
> 

Re: mtd: spinand: add spi nand mtd resume handler

[Pratyush Yadav]

On 26/05/21 05:30PM, patrice.chotard@foss.st.com wrote:
> From: Christophe Kerello <christophe.kerello@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>
> ---
>  drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 56 insertions(+)
> 
> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> index 17f63f95f4a2..6abaf874eb3f 100644
> --- a/drivers/mtd/nand/spi/core.c
> +++ b/drivers/mtd/nand/spi/core.c
> @@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
>  	return 0;
>  }
>  
> +static void spinand_mtd_resume(struct mtd_info *mtd)
> +{
> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> +	struct nand_device *nand = mtd_to_nanddev(mtd);
> +	struct device *dev = &spinand->spimem->spi->dev;
> +	int ret, i;
> +
> +	ret = spinand_reset_op(spinand);
> +	if (ret)
> +		return;
> +
> +	ret = spinand_init_quad_enable(spinand);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to initialize the quad part (err = %d)\n",
> +			ret);
> +		return;
> +	}
> +
> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to updtae the OTP (err = %d)\n",
> +			ret);
> +		return;
> +	}

Since you have reset the flash, this cache is invalid. You should reset 
the cache and re-populate it before using it in any way.

> +
> +	ret = spinand_manufacturer_init(spinand);
> +	if (ret) {
> +		dev_err(dev,
> +			"Failed to initialize the SPI NAND chip (err = %d)\n",
> +			ret);
> +		return;
> +	}
> +
> +	/* 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) {
> +			dev_err(dev,
> +				"Failed to select the target (err = %d)\n",
> +				ret);
> +			return;
> +		}
> +
> +		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
> +		if (ret) {
> +			dev_err(dev,
> +				"Failed to unlock block (err = %d)\n",
> +				ret);
> +			return;
> +		}
> +	}
> +}
> +

Most of these seem to be copied from spinand_init(). I think it is 
better to create a common function that can be called from both 
spinand_init() and spinand_mtd_resume(). This way when someone adds 
something new to the init procedure, like support for some other modes, 
they won't have to remember to update it in two places.

>  static int spinand_init(struct spinand_device *spinand)
>  {
>  	struct device *dev = &spinand->spimem->spi->dev;
> @@ -1167,6 +1222,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;

Is it possible that the userspace can use this mtd device before the 
resume is finished? Is there a way to temporarily "pause" or unregister 
an mtd device?

>  
>  	if (nand->ecc.engine) {
>  		ret = mtd_ooblayout_count_freebytes(mtd);

-- 
Regards,
Pratyush Yadav
Texas Instruments Inc.

Re: mtd: spinand: add spi nand mtd resume handler

[Miquel Raynal]

Hi Pratyush,

Pratyush Yadav <p.yadav@ti.com> wrote on Thu, 27 May 2021 15:30:17
+0530:

> On 26/05/21 05:30PM, patrice.chotard@foss.st.com wrote:
> > From: Christophe Kerello <christophe.kerello@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>
> > ---
> >  drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
> >  1 file changed, 56 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 17f63f95f4a2..6abaf874eb3f 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
> >  	return 0;
> >  }
> >  
> > +static void spinand_mtd_resume(struct mtd_info *mtd)
> > +{
> > +	struct spinand_device *spinand = mtd_to_spinand(mtd);
> > +	struct nand_device *nand = mtd_to_nanddev(mtd);
> > +	struct device *dev = &spinand->spimem->spi->dev;
> > +	int ret, i;
> > +
> > +	ret = spinand_reset_op(spinand);
> > +	if (ret)
> > +		return;
> > +
> > +	ret = spinand_init_quad_enable(spinand);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Failed to initialize the quad part (err = %d)\n",
> > +			ret);
> > +		return;
> > +	}
> > +
> > +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Failed to updtae the OTP (err = %d)\n",
> > +			ret);
> > +		return;
> > +	}  
> 
> Since you have reset the flash, this cache is invalid. You should reset 
> the cache and re-populate it before using it in any way.
> 
> > +
> > +	ret = spinand_manufacturer_init(spinand);
> > +	if (ret) {
> > +		dev_err(dev,
> > +			"Failed to initialize the SPI NAND chip (err = %d)\n",
> > +			ret);
> > +		return;
> > +	}
> > +
> > +	/* 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) {
> > +			dev_err(dev,
> > +				"Failed to select the target (err = %d)\n",
> > +				ret);
> > +			return;
> > +		}
> > +
> > +		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
> > +		if (ret) {
> > +			dev_err(dev,
> > +				"Failed to unlock block (err = %d)\n",
> > +				ret);
> > +			return;
> > +		}
> > +	}
> > +}
> > +  
> 
> Most of these seem to be copied from spinand_init(). I think it is 
> better to create a common function that can be called from both 
> spinand_init() and spinand_mtd_resume(). This way when someone adds 
> something new to the init procedure, like support for some other modes, 
> they won't have to remember to update it in two places.

Agreed, let's write a common helper for more than just the unlocking
sequence (still in a separate patch).

> 
> >  static int spinand_init(struct spinand_device *spinand)
> >  {
> >  	struct device *dev = &spinand->spimem->spi->dev;
> > @@ -1167,6 +1222,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;  
> 
> Is it possible that the userspace can use this mtd device before the 
> resume is finished? Is there a way to temporarily "pause" or unregister 
> an mtd device?

I don't expect this to happen, I would expect the kernel to resume
entirely before giving the hand to userspace, but I am not 100% sure of
that neither.

> 
> >  
> >  	if (nand->ecc.engine) {
> >  		ret = mtd_ooblayout_count_freebytes(mtd);  
> 

Thanks,
Miquèl

Re: mtd: spinand: add spi nand mtd resume handler

[Patrice CHOTARD]

Hi Pratyush

On 5/27/21 12:00 PM, Pratyush Yadav wrote:
> On 26/05/21 05:30PM, patrice.chotard@foss.st.com wrote:
>> From: Christophe Kerello <christophe.kerello@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>
>> ---
>>  drivers/mtd/nand/spi/core.c | 56 +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 56 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
>> index 17f63f95f4a2..6abaf874eb3f 100644
>> --- a/drivers/mtd/nand/spi/core.c
>> +++ b/drivers/mtd/nand/spi/core.c
>> @@ -1074,6 +1074,61 @@ static int spinand_detect(struct spinand_device *spinand)
>>  	return 0;
>>  }
>>  
>> +static void spinand_mtd_resume(struct mtd_info *mtd)
>> +{
>> +	struct spinand_device *spinand = mtd_to_spinand(mtd);
>> +	struct nand_device *nand = mtd_to_nanddev(mtd);
>> +	struct device *dev = &spinand->spimem->spi->dev;
>> +	int ret, i;
>> +
>> +	ret = spinand_reset_op(spinand);
>> +	if (ret)
>> +		return;
>> +
>> +	ret = spinand_init_quad_enable(spinand);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to initialize the quad part (err = %d)\n",
>> +			ret);
>> +		return;
>> +	}
>> +
>> +	ret = spinand_upd_cfg(spinand, CFG_OTP_ENABLE, 0);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to updtae the OTP (err = %d)\n",
>> +			ret);
>> +		return;
>> +	}
> 
> Since you have reset the flash, this cache is invalid. You should reset 
> the cache and re-populate it before using it in any way.

I got your point, i will take it in account in v3 (v2 has already been send but need rework)

> 
>> +
>> +	ret = spinand_manufacturer_init(spinand);
>> +	if (ret) {
>> +		dev_err(dev,
>> +			"Failed to initialize the SPI NAND chip (err = %d)\n",
>> +			ret);
>> +		return;
>> +	}
>> +
>> +	/* 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) {
>> +			dev_err(dev,
>> +				"Failed to select the target (err = %d)\n",
>> +				ret);
>> +			return;
>> +		}
>> +
>> +		ret = spinand_lock_block(spinand, BL_ALL_UNLOCKED);
>> +		if (ret) {
>> +			dev_err(dev,
>> +				"Failed to unlock block (err = %d)\n",
>> +				ret);
>> +			return;
>> +		}
>> +	}
>> +}
>> +
> 
> Most of these seem to be copied from spinand_init(). I think it is 
> better to create a common function that can be called from both 
> spinand_init() and spinand_mtd_resume(). This way when someone adds 
> something new to the init procedure, like support for some other modes, 
> they won't have to remember to update it in two places.

Agree, it was already part of the v2

> 
>>  static int spinand_init(struct spinand_device *spinand)
>>  {
>>  	struct device *dev = &spinand->spimem->spi->dev;
>> @@ -1167,6 +1222,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;
> 
> Is it possible that the userspace can use this mtd device before the 
> resume is finished? Is there a way to temporarily "pause" or unregister 
> an mtd device?

Honestly, i don't know.

Thanks
Patrice

> 
>>  
>>  	if (nand->ecc.engine) {
>>  		ret = mtd_ooblayout_count_freebytes(mtd);
>