diff mbox series

[v3,3/4] mtd: rawnand: Add support manufacturer specific suspend/resume operation

Message ID 1583220084-10890-4-git-send-email-masonccyang@mxic.com.tw
State Accepted
Headers show
Series mtd: rawnand: Add support Macronix Block Portection & Deep Power Down mode | expand

Commit Message

Mason Yang March 3, 2020, 7:21 a.m. UTC
Patch nand_suspend() & nand_resume() for manufacturer specific
suspend/resume operation.

Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
Reported-by: kbuild test robot <lkp@intel.com>
Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
---
 drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
 include/linux/mtd/rawnand.h      |  4 ++++
 2 files changed, 12 insertions(+), 3 deletions(-)

Comments

Miquel Raynal March 10, 2020, 6:30 p.m. UTC | #1
On Tue, 2020-03-03 at 07:21:23 UTC, Mason Yang wrote:
> Patch nand_suspend() & nand_resume() for manufacturer specific
> suspend/resume operation.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> Reported-by: kbuild test robot <lkp@intel.com>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel
Boris Brezillon March 10, 2020, 7:33 p.m. UTC | #2
On Tue,  3 Mar 2020 15:21:23 +0800
Mason Yang <masonccyang@mxic.com.tw> wrote:

> Patch nand_suspend() & nand_resume() for manufacturer specific
> suspend/resume operation.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> Reported-by: kbuild test robot <lkp@intel.com>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
>  include/linux/mtd/rawnand.h      |  4 ++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 769be81..b44e460 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4327,7 +4327,9 @@ static int nand_suspend(struct mtd_info *mtd)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  
>  	mutex_lock(&chip->lock);
> -	chip->suspended = 1;
> +	if (chip->_suspend)
> +		if (!chip->_suspend(chip))
> +			chip->suspended = 1;
>  	mutex_unlock(&chip->lock);
>  
>  	return 0;
> @@ -4342,11 +4344,14 @@ static void nand_resume(struct mtd_info *mtd)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  
>  	mutex_lock(&chip->lock);
> -	if (chip->suspended)
> +	if (chip->suspended) {
> +		if (chip->_resume)
> +			chip->_resume(chip);
>  		chip->suspended = 0;
> -	else
> +	} else {
>  		pr_err("%s called for a chip which is not in suspended state\n",
>  			__func__);
> +	}
>  	mutex_unlock(&chip->lock);
>  }
>  
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index bc2fa3c..c0055ed 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1064,6 +1064,8 @@ struct nand_legacy {
>   * @lock:		lock protecting the suspended field. Also used to
>   *			serialize accesses to the NAND device.
>   * @suspended:		set to 1 when the device is suspended, 0 when it's not.
> + * @_suspend:		[REPLACEABLE] specific NAND device suspend operation
> + * @_resume:		[REPLACEABLE] specific NAND device resume operation
>   * @bbt:		[INTERN] bad block table pointer
>   * @bbt_td:		[REPLACEABLE] bad block table descriptor for flash
>   *			lookup.
> @@ -1119,6 +1121,8 @@ struct nand_chip {
>  
>  	struct mutex lock;
>  	unsigned int suspended : 1;
> +	int (*_suspend)(struct nand_chip *chip);
> +	void (*_resume)(struct nand_chip *chip);

I thought we agreed on not prefixing new hooks with _ ?

>  
>  	uint8_t *oob_poi;
>  	struct nand_controller *controller;
Boris Brezillon March 10, 2020, 7:39 p.m. UTC | #3
On Tue,  3 Mar 2020 15:21:23 +0800
Mason Yang <masonccyang@mxic.com.tw> wrote:

> Patch nand_suspend() & nand_resume() for manufacturer specific
> suspend/resume operation.
> 
> Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> Reported-by: kbuild test robot <lkp@intel.com>
> Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> ---
>  drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
>  include/linux/mtd/rawnand.h      |  4 ++++
>  2 files changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index 769be81..b44e460 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4327,7 +4327,9 @@ static int nand_suspend(struct mtd_info *mtd)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  
>  	mutex_lock(&chip->lock);
> -	chip->suspended = 1;
> +	if (chip->_suspend)
> +		if (!chip->_suspend(chip))
> +			chip->suspended = 1;
>  	mutex_unlock(&chip->lock);
>  
>  	return 0;
> @@ -4342,11 +4344,14 @@ static void nand_resume(struct mtd_info *mtd)
>  	struct nand_chip *chip = mtd_to_nand(mtd);
>  
>  	mutex_lock(&chip->lock);
> -	if (chip->suspended)
> +	if (chip->suspended) {
> +		if (chip->_resume)
> +			chip->_resume(chip);
>  		chip->suspended = 0;
> -	else
> +	} else {
>  		pr_err("%s called for a chip which is not in suspended state\n",
>  			__func__);
> +	}
>  	mutex_unlock(&chip->lock);
>  }
>  
> diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> index bc2fa3c..c0055ed 100644
> --- a/include/linux/mtd/rawnand.h
> +++ b/include/linux/mtd/rawnand.h
> @@ -1064,6 +1064,8 @@ struct nand_legacy {
>   * @lock:		lock protecting the suspended field. Also used to
>   *			serialize accesses to the NAND device.
>   * @suspended:		set to 1 when the device is suspended, 0 when it's not.
> + * @_suspend:		[REPLACEABLE] specific NAND device suspend operation
> + * @_resume:		[REPLACEABLE] specific NAND device resume operation

Given you added 4 more methods in this series, I think now would be a
good time to introduce a nand_chip_ops struct grouping all ops together.

>   * @bbt:		[INTERN] bad block table pointer
>   * @bbt_td:		[REPLACEABLE] bad block table descriptor for flash
>   *			lookup.
> @@ -1119,6 +1121,8 @@ struct nand_chip {
>  
>  	struct mutex lock;
>  	unsigned int suspended : 1;
> +	int (*_suspend)(struct nand_chip *chip);
> +	void (*_resume)(struct nand_chip *chip);
>  
>  	uint8_t *oob_poi;
>  	struct nand_controller *controller;
Boris Brezillon March 10, 2020, 7:41 p.m. UTC | #4
On Tue, 10 Mar 2020 20:39:30 +0100
Boris Brezillon <boris.brezillon@collabora.com> wrote:

> On Tue,  3 Mar 2020 15:21:23 +0800
> Mason Yang <masonccyang@mxic.com.tw> wrote:
> 
> > Patch nand_suspend() & nand_resume() for manufacturer specific
> > suspend/resume operation.
> > 
> > Signed-off-by: Mason Yang <masonccyang@mxic.com.tw>
> > Reported-by: kbuild test robot <lkp@intel.com>
> > Reviewed-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
> >  include/linux/mtd/rawnand.h      |  4 ++++
> >  2 files changed, 12 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index 769be81..b44e460 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -4327,7 +4327,9 @@ static int nand_suspend(struct mtd_info *mtd)
> >  	struct nand_chip *chip = mtd_to_nand(mtd);
> >  
> >  	mutex_lock(&chip->lock);
> > -	chip->suspended = 1;
> > +	if (chip->_suspend)
> > +		if (!chip->_suspend(chip))
> > +			chip->suspended = 1;

Shouldn't you propagate the error to the caller if chip->_suspend()
fails?
Mason Yang March 11, 2020, 5:40 a.m. UTC | #5
Hi Boris,

> > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > index bc2fa3c..c0055ed 100644
> > --- a/include/linux/mtd/rawnand.h
> > +++ b/include/linux/mtd/rawnand.h
> > @@ -1064,6 +1064,8 @@ struct nand_legacy {
> >   * @lock:      lock protecting the suspended field. Also used to
> >   *         serialize accesses to the NAND device.
> >   * @suspended:      set to 1 when the device is suspended, 0 when 
it's not.
> > + * @_suspend:      [REPLACEABLE] specific NAND device suspend 
operation
> > + * @_resume:      [REPLACEABLE] specific NAND device resume operation
> >   * @bbt:      [INTERN] bad block table pointer
> >   * @bbt_td:      [REPLACEABLE] bad block table descriptor for flash
> >   *         lookup.
> > @@ -1119,6 +1121,8 @@ struct nand_chip {
> > 
> >     struct mutex lock;
> >     unsigned int suspended : 1;
> > +   int (*_suspend)(struct nand_chip *chip);
> > +   void (*_resume)(struct nand_chip *chip);
> 
> I thought we agreed on not prefixing new hooks with _ ?

For [PATCH v2] series, you mentioned to drop the _ prefix 
of _lock/_unlock only and we finally patched to lock_area/unlock_area.

thanks for your time & review.
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang March 11, 2020, 6:13 a.m. UTC | #6
Hi Boris,

> > > ---
> > >  drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
> > >  include/linux/mtd/rawnand.h      |  4 ++++
> > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/mtd/nand/raw/nand_base.c 
b/drivers/mtd/nand/raw/nand_base.c
> > > index 769be81..b44e460 100644
> > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > @@ -4327,7 +4327,9 @@ static int nand_suspend(struct mtd_info *mtd)
> > >     struct nand_chip *chip = mtd_to_nand(mtd);
> > > 
> > >     mutex_lock(&chip->lock);
> > > -   chip->suspended = 1;
> > > +   if (chip->_suspend)
> > > +      if (!chip->_suspend(chip))
> > > +         chip->suspended = 1;
> 
> Shouldn't you propagate the error to the caller if chip->_suspend()
> fails?

Currently, chip->suspend() just do sending command to nand chip and
I think caller could check chip->suspend = 1 or 0 to know the status
of nand chip.

thanks & best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Miquel Raynal March 11, 2020, 7:43 a.m. UTC | #7
Hi Mason,

masonccyang@mxic.com.tw wrote on Wed, 11 Mar 2020 13:40:52 +0800:

> Hi Boris,
> 
> > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > > index bc2fa3c..c0055ed 100644
> > > --- a/include/linux/mtd/rawnand.h
> > > +++ b/include/linux/mtd/rawnand.h
> > > @@ -1064,6 +1064,8 @@ struct nand_legacy {
> > >   * @lock:      lock protecting the suspended field. Also used to
> > >   *         serialize accesses to the NAND device.
> > >   * @suspended:      set to 1 when the device is suspended, 0 when   
> it's not.
> > > + * @_suspend:      [REPLACEABLE] specific NAND device suspend   
> operation
> > > + * @_resume:      [REPLACEABLE] specific NAND device resume operation
> > >   * @bbt:      [INTERN] bad block table pointer
> > >   * @bbt_td:      [REPLACEABLE] bad block table descriptor for flash
> > >   *         lookup.
> > > @@ -1119,6 +1121,8 @@ struct nand_chip {
> > > 
> > >     struct mutex lock;
> > >     unsigned int suspended : 1;
> > > +   int (*_suspend)(struct nand_chip *chip);
> > > +   void (*_resume)(struct nand_chip *chip);  
> > 
> > I thought we agreed on not prefixing new hooks with _ ?  
> 
> For [PATCH v2] series, you mentioned to drop the _ prefix 
> of _lock/_unlock only and we finally patched to lock_area/unlock_area.
> 

I missed this _, this is not something we want to add.

Also, when applying your patches I had several issues because they
where not base on the last -rc1.

Finally, I think I forgot a line when patching manually so it produces
a warning now.

I am dropping patch 3 and 4, I keep patch 1 and 2 which seem fine.

Please send a rebased and edited v4 for these, don't forget to drop the
kbuildtest robot tag and please also follow these slightly edited
commit logs:

2/4

    mtd: rawnand: Add support for manufacturer specific suspend/resume operation
    
    Patch nand_suspend() & nand_resume() to let manufacturers overwrite
    suspend/resume operations.

3/4

    mtd: rawnand: macronix: Add support for deep power down mode
    
    Macronix AD series support deep power down mode for a minimum
    power consumption state.
    
    Overlaod nand_suspend() & nand_resume() in Macronix specific code to
    support deep power down mode.

Thanks,
Miquèl
Boris Brezillon March 11, 2020, 7:56 a.m. UTC | #8
On Wed, 11 Mar 2020 08:43:04 +0100
Miquel Raynal <miquel.raynal@bootlin.com> wrote:

> Hi Mason,
> 
> masonccyang@mxic.com.tw wrote on Wed, 11 Mar 2020 13:40:52 +0800:
> 
> > Hi Boris,
> >   
> > > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
> > > > index bc2fa3c..c0055ed 100644
> > > > --- a/include/linux/mtd/rawnand.h
> > > > +++ b/include/linux/mtd/rawnand.h
> > > > @@ -1064,6 +1064,8 @@ struct nand_legacy {
> > > >   * @lock:      lock protecting the suspended field. Also used to
> > > >   *         serialize accesses to the NAND device.
> > > >   * @suspended:      set to 1 when the device is suspended, 0 when     
> > it's not.  
> > > > + * @_suspend:      [REPLACEABLE] specific NAND device suspend     
> > operation  
> > > > + * @_resume:      [REPLACEABLE] specific NAND device resume operation
> > > >   * @bbt:      [INTERN] bad block table pointer
> > > >   * @bbt_td:      [REPLACEABLE] bad block table descriptor for flash
> > > >   *         lookup.
> > > > @@ -1119,6 +1121,8 @@ struct nand_chip {
> > > > 
> > > >     struct mutex lock;
> > > >     unsigned int suspended : 1;
> > > > +   int (*_suspend)(struct nand_chip *chip);
> > > > +   void (*_resume)(struct nand_chip *chip);    
> > > 
> > > I thought we agreed on not prefixing new hooks with _ ?    
> > 
> > For [PATCH v2] series, you mentioned to drop the _ prefix 
> > of _lock/_unlock only and we finally patched to lock_area/unlock_area.
> >   
> 
> I missed this _, this is not something we want to add.
> 
> Also, when applying your patches I had several issues because they
> where not base on the last -rc1.
> 
> Finally, I think I forgot a line when patching manually so it produces
> a warning now.
> 
> I am dropping patch 3 and 4, I keep patch 1 and 2 which seem fine.
> 
> Please send a rebased and edited v4 for these, don't forget to drop the
> kbuildtest robot tag and please also follow these slightly edited
> commit logs:
> 
> 2/4
> 
>     mtd: rawnand: Add support for manufacturer specific suspend/resume operation
>     
>     Patch nand_suspend() & nand_resume() to let manufacturers overwrite
>     suspend/resume operations.
> 
> 3/4
> 
>     mtd: rawnand: macronix: Add support for deep power down mode
>     
>     Macronix AD series support deep power down mode for a minimum
>     power consumption state.
>     
>     Overlaod nand_suspend() & nand_resume() in Macronix specific code to
>     support deep power down mode.

And don't forget to propagate the ->suspend() error code to the upper
layer.
Boris Brezillon March 11, 2020, 8:01 a.m. UTC | #9
On Wed, 11 Mar 2020 14:13:57 +0800
masonccyang@mxic.com.tw wrote:

> Hi Boris,
> 
> > > > ---
> > > >  drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
> > > >  include/linux/mtd/rawnand.h      |  4 ++++
> > > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/mtd/nand/raw/nand_base.c   
> b/drivers/mtd/nand/raw/nand_base.c
> > > > index 769be81..b44e460 100644
> > > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > > @@ -4327,7 +4327,9 @@ static int nand_suspend(struct mtd_info *mtd)
> > > >     struct nand_chip *chip = mtd_to_nand(mtd);
> > > > 
> > > >     mutex_lock(&chip->lock);
> > > > -   chip->suspended = 1;
> > > > +   if (chip->_suspend)
> > > > +      if (!chip->_suspend(chip))
> > > > +         chip->suspended = 1;  
> > 
> > Shouldn't you propagate the error to the caller if chip->_suspend()
> > fails?  
> 
> Currently, chip->suspend() just do sending command to nand chip and
> I think caller could check chip->suspend = 1 or 0 to know the status
> of nand chip.

No, it can't. The caller (AKA the MTD layer) has no idea about this
chip->suspend field, actually it doesn't even know about the nand_chip
struct. The mtd->_suspend() hook is here to abstract HW details, so
it's the raw NAND framework responsibility to propagate the error code
returned by chip->suspend().
Mason Yang March 12, 2020, 1:45 a.m. UTC | #10
Hi Miquel,
 
> > > > diff --git a/include/linux/mtd/rawnand.h 
b/include/linux/mtd/rawnand.h
> > > > index bc2fa3c..c0055ed 100644
> > > > --- a/include/linux/mtd/rawnand.h
> > > > +++ b/include/linux/mtd/rawnand.h
> > > > @@ -1064,6 +1064,8 @@ struct nand_legacy {
> > > >   * @lock:      lock protecting the suspended field. Also used to
> > > >   *         serialize accesses to the NAND device.
> > > >   * @suspended:      set to 1 when the device is suspended, 0 when 
 
> > it's not.
> > > > + * @_suspend:      [REPLACEABLE] specific NAND device suspend 
> > operation
> > > > + * @_resume:      [REPLACEABLE] specific NAND device resume 
operation
> > > >   * @bbt:      [INTERN] bad block table pointer
> > > >   * @bbt_td:      [REPLACEABLE] bad block table descriptor for 
flash
> > > >   *         lookup.
> > > > @@ -1119,6 +1121,8 @@ struct nand_chip {
> > > > 
> > > >     struct mutex lock;
> > > >     unsigned int suspended : 1;
> > > > +   int (*_suspend)(struct nand_chip *chip);
> > > > +   void (*_resume)(struct nand_chip *chip); 
> > > 
> > > I thought we agreed on not prefixing new hooks with _ ? 
> > 
> > For [PATCH v2] series, you mentioned to drop the _ prefix 
> > of _lock/_unlock only and we finally patched to lock_area/unlock_area.
> > 
> 
> I missed this _, this is not something we want to add.
> 
> Also, when applying your patches I had several issues because they
> where not base on the last -rc1.
> 
> Finally, I think I forgot a line when patching manually so it produces
> a warning now.
> 
> I am dropping patch 3 and 4, I keep patch 1 and 2 which seem fine.
> 
> Please send a rebased and edited v4 for these, don't forget to drop the
> kbuildtest robot tag and please also follow these slightly edited
> commit logs:
> 
> 2/4
> 
>     mtd: rawnand: Add support for manufacturer specific suspend/resume 
operation
> 
>     Patch nand_suspend() & nand_resume() to let manufacturers overwrite
>     suspend/resume operations.
> 
> 3/4
> 
>     mtd: rawnand: macronix: Add support for deep power down mode
> 
>     Macronix AD series support deep power down mode for a minimum
>     power consumption state.
> 
>     Overlaod nand_suspend() & nand_resume() in Macronix specific code to
>     support deep power down mode.

okay, will resend [PATCH v4 xx/2] for suspend/resume operation with these 
commit logs.

> 
> Thanks,
> Miquèl

thanks for your review & comments.
Mason


CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
Mason Yang March 12, 2020, 1:48 a.m. UTC | #11
Hi Boris,

> > > > > ---
> > > > >  drivers/mtd/nand/raw/nand_base.c | 11 ++++++++---
> > > > >  include/linux/mtd/rawnand.h      |  4 ++++
> > > > >  2 files changed, 12 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c 
> > b/drivers/mtd/nand/raw/nand_base.c
> > > > > index 769be81..b44e460 100644
> > > > > --- a/drivers/mtd/nand/raw/nand_base.c
> > > > > +++ b/drivers/mtd/nand/raw/nand_base.c
> > > > > @@ -4327,7 +4327,9 @@ static int nand_suspend(struct mtd_info 
*mtd)
> > > > >     struct nand_chip *chip = mtd_to_nand(mtd);
> > > > > 
> > > > >     mutex_lock(&chip->lock);
> > > > > -   chip->suspended = 1;
> > > > > +   if (chip->_suspend)
> > > > > +      if (!chip->_suspend(chip))
> > > > > +         chip->suspended = 1; 
> > > 
> > > Shouldn't you propagate the error to the caller if chip->_suspend()
> > > fails? 
> > 
> > Currently, chip->suspend() just do sending command to nand chip and
> > I think caller could check chip->suspend = 1 or 0 to know the status
> > of nand chip.
> 
> No, it can't. The caller (AKA the MTD layer) has no idea about this
> chip->suspend field, actually it doesn't even know about the nand_chip
> struct. The mtd->_suspend() hook is here to abstract HW details, so
> it's the raw NAND framework responsibility to propagate the error code
> returned by chip->suspend().

Got it, thanks!

best regards,
Mason

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information 
and/or personal data, which is protected by applicable laws. Please be 
reminded that duplication, disclosure, distribution, or use of this e-mail 
(and/or its attachments) or any part thereof is prohibited. If you receive 
this e-mail in error, please notify us immediately and delete this mail as 
well as its attachment(s) from your system. In addition, please be 
informed that collection, processing, and/or use of personal data is 
prohibited unless expressly permitted by personal data protection laws. 
Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================



============================================================================

CONFIDENTIALITY NOTE:

This e-mail and any attachments may contain confidential information and/or personal data, which is protected by applicable laws. Please be reminded that duplication, disclosure, distribution, or use of this e-mail (and/or its attachments) or any part thereof is prohibited. If you receive this e-mail in error, please notify us immediately and delete this mail as well as its attachment(s) from your system. In addition, please be informed that collection, processing, and/or use of personal data is prohibited unless expressly permitted by personal data protection laws. Thank you for your attention and cooperation.

Macronix International Co., Ltd.

=====================================================================
diff mbox series

Patch

diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index 769be81..b44e460 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4327,7 +4327,9 @@  static int nand_suspend(struct mtd_info *mtd)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 
 	mutex_lock(&chip->lock);
-	chip->suspended = 1;
+	if (chip->_suspend)
+		if (!chip->_suspend(chip))
+			chip->suspended = 1;
 	mutex_unlock(&chip->lock);
 
 	return 0;
@@ -4342,11 +4344,14 @@  static void nand_resume(struct mtd_info *mtd)
 	struct nand_chip *chip = mtd_to_nand(mtd);
 
 	mutex_lock(&chip->lock);
-	if (chip->suspended)
+	if (chip->suspended) {
+		if (chip->_resume)
+			chip->_resume(chip);
 		chip->suspended = 0;
-	else
+	} else {
 		pr_err("%s called for a chip which is not in suspended state\n",
 			__func__);
+	}
 	mutex_unlock(&chip->lock);
 }
 
diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h
index bc2fa3c..c0055ed 100644
--- a/include/linux/mtd/rawnand.h
+++ b/include/linux/mtd/rawnand.h
@@ -1064,6 +1064,8 @@  struct nand_legacy {
  * @lock:		lock protecting the suspended field. Also used to
  *			serialize accesses to the NAND device.
  * @suspended:		set to 1 when the device is suspended, 0 when it's not.
+ * @_suspend:		[REPLACEABLE] specific NAND device suspend operation
+ * @_resume:		[REPLACEABLE] specific NAND device resume operation
  * @bbt:		[INTERN] bad block table pointer
  * @bbt_td:		[REPLACEABLE] bad block table descriptor for flash
  *			lookup.
@@ -1119,6 +1121,8 @@  struct nand_chip {
 
 	struct mutex lock;
 	unsigned int suspended : 1;
+	int (*_suspend)(struct nand_chip *chip);
+	void (*_resume)(struct nand_chip *chip);
 
 	uint8_t *oob_poi;
 	struct nand_controller *controller;