| Message ID | BYAPR08MB453384AEAB702223F460039EDB9C0@BYAPR08MB4533.namprd08.prod.outlook.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | mtd: core: NAND erase preparation | expand |
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);
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
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
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.
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
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
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.
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);
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(+)