[RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

This patch is to add a callback function pointer
for preparation routine of erase in nand_manufacturer_ops.

Signed-off-by: Bean Huo <beanhuo@micron.com>
---
 drivers/mtd/nand/raw/internals.h | 2 ++
 drivers/mtd/nand/raw/nand_base.c | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
index fbf6ca0..8382948 100644
--- a/drivers/mtd/nand/raw/internals.h
+++ b/drivers/mtd/nand/raw/internals.h
@@ -42,6 +42,7 @@
  *	     is here to let vendor specific code release those resources.
  * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI parameter
  *			   page. This is called after the checksum is verified.
+ * @erase_pre: preparation before actually erase a physical block.
  */
 struct nand_manufacturer_ops {
 	void (*detect)(struct nand_chip *chip);
@@ -49,6 +50,7 @@ struct nand_manufacturer_ops {
 	void (*cleanup)(struct nand_chip *chip);
 	void (*fixup_onfi_param_page)(struct nand_chip *chip,
 				      struct nand_onfi_params *p);
+	int (*erase_pre)(struct nand_chip *chip, int page);
 };
 
 /**
diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
index cca4b24..d446d1c 100644
--- a/drivers/mtd/nand/raw/nand_base.c
+++ b/drivers/mtd/nand/raw/nand_base.c
@@ -4246,6 +4246,11 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
 		    (page + pages_per_block))
 			chip->pagebuf = -1;
 
+		if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
+		    chip->manufacturer.desc->ops->erase_pre)
+			chip->manufacturer.desc->ops->erase_pre(chip,
+						page & chip->pagemask);
+
 		if (chip->legacy.erase)
 			status = chip->legacy.erase(chip,
 						    page & chip->pagemask);
-- 
2.7.4

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Boris Brezillon]

On Fri, 18 Jan 2019 22:11:34 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> This patch is to add a callback function pointer
> for preparation routine of erase in nand_manufacturer_ops.
> 
> Signed-off-by: Bean Huo <beanhuo@micron.com>
> ---
>  drivers/mtd/nand/raw/internals.h | 2 ++
>  drivers/mtd/nand/raw/nand_base.c | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> index fbf6ca0..8382948 100644
> --- a/drivers/mtd/nand/raw/internals.h
> +++ b/drivers/mtd/nand/raw/internals.h
> @@ -42,6 +42,7 @@
>   *	     is here to let vendor specific code release those resources.
>   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI parameter
>   *			   page. This is called after the checksum is verified.
> + * @erase_pre: preparation before actually erase a physical block.
>   */
>  struct nand_manufacturer_ops {
>  	void (*detect)(struct nand_chip *chip);
> @@ -49,6 +50,7 @@ struct nand_manufacturer_ops {
>  	void (*cleanup)(struct nand_chip *chip);
>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
>  				      struct nand_onfi_params *p);
> +	int (*erase_pre)(struct nand_chip *chip, int page);

Let's move this hook to nand_chip and name it ->pre_erase() or
->erase_preparation().

>  };
>  
>  /**
> diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> index cca4b24..d446d1c 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -4246,6 +4246,11 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
>  		    (page + pages_per_block))
>  			chip->pagebuf = -1;
>  
> +		if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
> +		    chip->manufacturer.desc->ops->erase_pre)
> +			chip->manufacturer.desc->ops->erase_pre(chip,
> +						page & chip->pagemask);
> +
>  		if (chip->legacy.erase)
>  			status = chip->legacy.erase(chip,
>  						    page & chip->pagemask);


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Miquel Raynal]

Hi Bean,

For the changes!

Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 18 Jan 2019
23:42:48 +0100:

> On Fri, 18 Jan 2019 22:11:34 +0000
> "Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:
> 
> > This patch is to add a callback function pointer
> > for preparation routine of erase in nand_manufacturer_ops.
> > 
> > Signed-off-by: Bean Huo <beanhuo@micron.com>
> > ---
> >  drivers/mtd/nand/raw/internals.h | 2 ++
> >  drivers/mtd/nand/raw/nand_base.c | 5 +++++
> >  2 files changed, 7 insertions(+)
> > 
> > diff --git a/drivers/mtd/nand/raw/internals.h b/drivers/mtd/nand/raw/internals.h
> > index fbf6ca0..8382948 100644
> > --- a/drivers/mtd/nand/raw/internals.h
> > +++ b/drivers/mtd/nand/raw/internals.h
> > @@ -42,6 +42,7 @@
> >   *	     is here to let vendor specific code release those resources.
> >   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI parameter
> >   *			   page. This is called after the checksum is verified.
> > + * @erase_pre: preparation before actually erase a physical block.

What about "prepare a physical erase block before erasure" ?

> >   */
> >  struct nand_manufacturer_ops {
> >  	void (*detect)(struct nand_chip *chip);
> > @@ -49,6 +50,7 @@ struct nand_manufacturer_ops {
> >  	void (*cleanup)(struct nand_chip *chip);
> >  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
> >  				      struct nand_onfi_params *p);
> > +	int (*erase_pre)(struct nand_chip *chip, int page);  
> 
> Let's move this hook to nand_chip and name it ->pre_erase() or
> ->erase_preparation().  
> 
> >  };
> >  
> >  /**
> > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c
> > index cca4b24..d446d1c 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -4246,6 +4246,11 @@ int nand_erase_nand(struct nand_chip *chip, struct erase_info *instr,
> >  		    (page + pages_per_block))
> >  			chip->pagebuf = -1;
> >  
> > +		if (chip->manufacturer.desc && chip->manufacturer.desc->ops &&
> > +		    chip->manufacturer.desc->ops->erase_pre)
> > +			chip->manufacturer.desc->ops->erase_pre(chip,
> > +						page & chip->pagemask);
> > +
> >  		if (chip->legacy.erase)
> >  			status = chip->legacy.erase(chip,
> >  						    page & chip->pagemask);  
> 

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Hi, Boris
Thanks for reviewing this patch.

>>   *	     is here to let vendor specific code release those resources.
>>   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI
>parameter
>>   *			   page. This is called after the checksum is verified.
>> + * @erase_pre: preparation before actually erase a physical block.
>>   */
>>  struct nand_manufacturer_ops {
>>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
>> nand_manufacturer_ops {
>>  	void (*cleanup)(struct nand_chip *chip);
>>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
>>  				      struct nand_onfi_params *p);
>> +	int (*erase_pre)(struct nand_chip *chip, int page);
>
>Let's move this hook to nand_chip and name it ->pre_erase() or
>->erase_preparation().
>

Can you tell us more reasons why moves this hook to nand_chip?
In my opinion, it is better to add this hook in nand_manufacturer_ops, since nand_manufacturer_ops
Already exists in nand_chip, also this function is related to specific NAND manufacturer.

//Bean Huo

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Boris Brezillon]

On Mon, 21 Jan 2019 09:25:26 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris
> Thanks for reviewing this patch.
> 
> >>   *	     is here to let vendor specific code release those resources.
> >>   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI  
> >parameter  
> >>   *			   page. This is called after the checksum is verified.
> >> + * @erase_pre: preparation before actually erase a physical block.
> >>   */
> >>  struct nand_manufacturer_ops {
> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
> >> nand_manufacturer_ops {
> >>  	void (*cleanup)(struct nand_chip *chip);
> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
> >>  				      struct nand_onfi_params *p);
> >> +	int (*erase_pre)(struct nand_chip *chip, int page);  
> >
> >Let's move this hook to nand_chip and name it ->pre_erase() or  
> >->erase_preparation().  
> >  
> 
> Can you tell us more reasons why moves this hook to nand_chip?

Because it's supposed to be a per-chip thing. I mean, not all of your
chips have this bug (at least I hope), so we want to only have a
->pre_erase() implementation when it's really needed. The manufacturer
specific ->init() hook will decide when it's appropriate to populate
this ->pre_erase pointer based on the NAND chip id (or the NAND chip
model).

> In my opinion, it is better to add this hook in nand_manufacturer_ops, since nand_manufacturer_ops
> Already exists in nand_chip, also this function is related to specific NAND manufacturer.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Thanks for reviewing, Miquel.

>> > diff --git a/drivers/mtd/nand/raw/internals.h
>> > b/drivers/mtd/nand/raw/internals.h
>> > index fbf6ca0..8382948 100644
>> > --- a/drivers/mtd/nand/raw/internals.h
>> > +++ b/drivers/mtd/nand/raw/internals.h
>> > @@ -42,6 +42,7 @@
>> >   *	     is here to let vendor specific code release those resources.
>> >   * @fixup_onfi_param_page: apply vendor specific fixups to the ONFI
>parameter
>> >   *			   page. This is called after the checksum is verified.
>> > + * @erase_pre: preparation before actually erase a physical block.
>
>What about "prepare a physical erase block before erasure" ?

I prefer your suggestion, apply it to next version patch.

//Bean
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Hi, Boris

>> >>  struct nand_manufacturer_ops {
>> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
>> >> nand_manufacturer_ops {
>> >>  	void (*cleanup)(struct nand_chip *chip);
>> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
>> >>  				      struct nand_onfi_params *p);
>> >> +	int (*erase_pre)(struct nand_chip *chip, int page);
>> >
>> >Let's move this hook to nand_chip and name it ->pre_erase() or
>> >->erase_preparation().
>> >
>>
>> Can you tell us more reasons why moves this hook to nand_chip?
>
>Because it's supposed to be a per-chip thing. I mean, not all of your chips have
>this bug (at least I hope), so we want to only have a
>->pre_erase() implementation when it's really needed. The manufacturer
>specific ->init() hook will decide when it's appropriate to populate this -
>>pre_erase pointer based on the NAND chip id (or the NAND chip model).
>

>> In my opinion, it is better to add this hook in nand_manufacturer_ops,
>> since nand_manufacturer_ops Already exists in nand_chip, also this function is
>related to specific NAND manufacturer.
I still think, keep this hook in nand_manufacturer_ops is better, otherwise, just add one
Function hook pointer in nand_chip, it is very weird.

//Bean

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Boris Brezillon]

On Thu, 24 Jan 2019 15:52:03 +0000
"Bean Huo (beanhuo)" <beanhuo@micron.com> wrote:

> Hi, Boris
> 
> >> >>  struct nand_manufacturer_ops {
> >> >>  	void (*detect)(struct nand_chip *chip); @@ -49,6 +50,7 @@ struct
> >> >> nand_manufacturer_ops {
> >> >>  	void (*cleanup)(struct nand_chip *chip);
> >> >>  	void (*fixup_onfi_param_page)(struct nand_chip *chip,
> >> >>  				      struct nand_onfi_params *p);
> >> >> +	int (*erase_pre)(struct nand_chip *chip, int page);  
> >> >
> >> >Let's move this hook to nand_chip and name it ->pre_erase() or  
> >> >->erase_preparation().  
> >> >  
> >>
> >> Can you tell us more reasons why moves this hook to nand_chip?  
> >
> >Because it's supposed to be a per-chip thing. I mean, not all of your chips have
> >this bug (at least I hope), so we want to only have a  
> >->pre_erase() implementation when it's really needed. The manufacturer  
> >specific ->init() hook will decide when it's appropriate to populate this -  
> >>pre_erase pointer based on the NAND chip id (or the NAND chip model).  
> >  
> 
> >> In my opinion, it is better to add this hook in nand_manufacturer_ops,
> >> since nand_manufacturer_ops Already exists in nand_chip, also this function is  
> >related to specific NAND manufacturer.  
> I still think, keep this hook in nand_manufacturer_ops is better, otherwise, just add one
> Function hook pointer in nand_chip, it is very weird.
> 

And I tell you it's not. What's the point of adding a hook at the
nand_manufacturer_ops level (which is the same for all NAND chips
produced by a manufacturer) if you then have to check whether a
specific chip has anything to do inside the hook itself. When we added
nand_manufacturer and the associated ops it was created to address
chip detection and initialization, nothing more. Anything that is
needed at runtime and is manufacturer specific should have a hook/flag
in nand_chip.

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

RE: [EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Bean Huo (beanhuo)]

Hi, Piotr

>> To avoid this rare occurrence, we make sure that at least
>> 15 pages have been programmed to a block before it is erased.
>> In case we find that less than 15 pages have been programmed,
>> additional pages are programmed in the block. The observation is that
>> additional pages rarely need to be written and most of the time
>> UBI/UBIFS erases blocks that contain more programmed pages.
>>
>> Signed-off-by: Bean Huo <beanhuo at micron.com>
>> Reviewed-by: ZOLTAN SZUBBOCSEV <zszubbocsev at micron.com>
>> ---
>>  drivers/mtd/nand/raw/nand_micron.c | 119
>> +++++++++++++++++++++++++++++++++++++
>>  1 file changed, 119 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/nand_micron.c
>> b/drivers/mtd/nand/raw/nand_micron.c
>> index b85e1c1..f52e072 100644
>> --- a/drivers/mtd/nand/raw/nand_micron.c
>> +++ b/drivers/mtd/nand/raw/nand_micron.c
>
>Is there a plan to merge this patch soon?
>
>
I think, it is quickly merged in.
I need to update a new version since there are some places need to change.

>______________________________________________________
>Linux MTD discussion mailing list
>http://lists.infradead.org/mailman/listinfo/linux-mtd/
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[EXT] Re: [RESEND PATCH V2 1/2] mtd: core: add erase preparation hook function pointer

[Piotr Wojtaszczyk]

> On some legacy planar 2D Micron NAND devices when a
> block erase command is issued, occasionally even
> though a block erase operation successfully completes
> and returns a pass status, the flash block may not be
> completely erased. Subsequent operations to this block
> on very rare cases can result in subtle failures or
> corruption. These extremely rare cases should nevertheless
> be considered.
>
> These rare occurrences have been observed on partially
> written blocks. Partially written blocks are not uncommon
> with UBI/UBIFS.
>
> To avoid this rare occurrence, we make sure that at least
> 15 pages have been programmed to a block before it is erased.
> In case we find that less than 15 pages have been programmed,
> additional pages are programmed in the block. The observation
> is that additional pages rarely need to be written and most of
> the time UBI/UBIFS erases blocks that contain more programmed
> pages.
>
> Signed-off-by: Bean Huo <beanhuo at micron.com>
> Reviewed-by: ZOLTAN SZUBBOCSEV <zszubbocsev at micron.com>
> ---
>  drivers/mtd/nand/raw/nand_micron.c | 119 
> +++++++++++++++++++++++++++++++++++++
>  1 file changed, 119 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_micron.c 
> b/drivers/mtd/nand/raw/nand_micron.c
> index b85e1c1..f52e072 100644
> --- a/drivers/mtd/nand/raw/nand_micron.c
> +++ b/drivers/mtd/nand/raw/nand_micron.c

Is there a plan to merge this patch soon?


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/