Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hi Thomas,
>
> > > >
---
> > > > static void macronix_nand_onfi_init(struct nand_chip *chip)
> > > > {
> > > > struct nand_parameters *p = &chip->parameters;
> > > > struct nand_onfi_vendor_macronix *mxic = (void
> > > > *)p->onfi->vendor;
> > >
> > > Why cast to void*, instead of casting directly to struct
> > > nand_onfi_vendor_macronix * ?
> >
> > Due to got a warning:
> >
> > warning: initialization from incompatible pointer type
> > struct nand_onfi_vendor_macronix *mxic = p->onfi->vendor;
>
> You didn't look at my code, I suggested:
>
>mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
Oops, sorry that I did not pay attention to it.
Will patch it by your comments.
static void macronix_nand_onfi_init(struct nand_chip *chip)
{
struct nand_parameters *p = &chip->parameters;
struct nand_onfi_vendor_macronix *mxic;
if (!p->onfi)
return;
mxic = (struct nand_onfi_vendor_macronix *) p->onfi->vendor;
if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
return;
chip->read_retries = MACRONIX_READ_RETRY_MODE;
chip->setup_read_retry = macronix_nand_setup_read_retry;
if (p->supports_set_get_features) {
bitmap_set(p->set_feature_list,
ONFI_FEATURE_ADDR_READ_RETRY, 1);
bitmap_set(p->get_feature_list,
ONFI_FEATURE_ADDR_READ_RETRY, 1);
}
}
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.
=
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hello,
On Tue, 14 May 2019 09:53:16 +0800
masonccy...@mxic.com.tw wrote:
> > > ---
> > > static void macronix_nand_onfi_init(struct nand_chip *chip)
> > > {
> > > struct nand_parameters *p = &chip->parameters;
> > > struct nand_onfi_vendor_macronix *mxic = (void
> > > *)p->onfi->vendor;
> >
> > Why cast to void*, instead of casting directly to struct
> > nand_onfi_vendor_macronix * ?
>
> Due to got a warning:
>
> warning: initialization from incompatible pointer type
> struct nand_onfi_vendor_macronix *mxic = p->onfi->vendor;
You didn't look at my code, I suggested:
mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
I.e, you indeed still need a cast, because p->info->vendor is a u8[].
But instead of casting to void*, and then implicitly casting to struct
nand_onfi_vendor_macronix *, I suggest to cast directly to struct
nand_onfi_vendor_macronix *.
> > > if (!p->onfi ||
> > > ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) ==
> 0))
> > > return;
> >
> > So, the code should be:
> >
> >struct nand_onfi_vendor_macronix *mxic;
> >
> >if (!p->onfi)
> > return;
> >
> >mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
> >
> >if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
> > return;
>
> Also got a warning:
>
> warning: ISO C90 forbids mixed declarations and code
> [-Wdeclaration-after-statement]
No, you don't get this warning if you use my code. You get this warning
if you declare and initialized the "mxic" variable at the same location.
> static void macronix_nand_onfi_init(struct nand_chip *chip)
> {
> struct nand_parameters *p = &chip->parameters;
> struct nand_onfi_vendor_macronix *mxic = (void *)p->onfi->vendor;
You are dereferencing p->info...
>
> if (!p->onfi)
> return;
... before you check it is NULL. This is wrong.
Please check again the code I sent in my previous e-mail:
struct nand_onfi_vendor_macronix *mxic;
if (!p->onfi)
return;
mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
return;
Best regards,
Thomas Petazzoni
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hi Miquel,
> > > > > > +
> > > > > > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
> > > > > > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
> > > > >
> > > > > Why +1 here, I am missing something?
> > > >
> > > >
> > > > Without + 1, read retry mode is up to 4 rather than 5.
> > > > But this NAND device support read retry mode up to 5.
> > > >
> > >
> > > If there are 5 modes, you need to set 5 to chip->read_retries, not
6.
> > >
> > > If only 4 modes are used, there is probably a bug in the core that
> > > must be fixed, please do not workaround it in the driver!
> >
> >
> > It seems some patches is needed in nand_base.c
> >
-
> > diff --git a/drivers/mtd/nand/raw/nand_base.c
> > b/drivers/mtd/nand/raw/nand_base.c
> > index ddd396e..56be3a9 100644
> > --- a/drivers/mtd/nand/raw/nand_base.c
> > +++ b/drivers/mtd/nand/raw/nand_base.c
> > @@ -3101,7 +3101,8 @@ static int nand_setup_read_retry(struct
nand_chip
> > *chip, int retry_mode)
> > {
> > pr_debug("setting READ RETRY mode %d\n", retry_mode);
> >
> > - if (retry_mode >= chip->read_retries)
> > + if (retry_mode > chip->read_retries)
>
> If I understand correctly, chip->read_retries is the total number of
> 'modes' so the last valid mode is indeed chip->read_retries -1.
>
> I thought the problem would come from the core but I was wrong, you
> actually need a MACRONIX_NUM_READ_RETRY_MODES set to 6. Please have two
> defines if you need both (the first one being something like
> MACRONIX_MAXIMUM_READ_RETRY_MODE set to 5).
I have checked one of Micron's datasheet and it defined read-retry number
= 8,
mode 0 ~ 7, 0 mean to disable read-retry.
Therefore, I will patch MACRONIX_NUM_READ_RETRY_MODES = 6.
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.
=
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hi Mason,
masonccy...@mxic.com.tw wrote on Mon, 13 May 2019 15:47:53 +0800:
> Hi Miquel,
>
>
> > >
> > > >
> > > > > + if (ret)
> > > > > + pr_err("set feature failed to read retry moded:%d\n",
> mode);
> > > >
> > > >"Failed to set read retry mode: %d\n"
> > > >
> > > > I think you can abort the operation with a negative return code in
> this
> > > > case.
> > > >
> > >
> > > After set feature operation, this NAND device need a get feature
> command
> >
> > You need to add a comment for this.
>
> okay, will add this comment by next version.
>
> >
> > > or
> > > SW reset command to exit read retry mode.
> > > Therefore, set features command followed by get feature command is
> needed.
> >
> > In this case you must check first that both set and get are supported.
> >
>
> okay, thanks.
>
>
> > > > > +
> > > > > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
> > > > > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
> > > >
> > > > Why +1 here, I am missing something?
> > >
> > >
> > > Without + 1, read retry mode is up to 4 rather than 5.
> > > But this NAND device support read retry mode up to 5.
> > >
> >
> > If there are 5 modes, you need to set 5 to chip->read_retries, not 6.
> >
> > If only 4 modes are used, there is probably a bug in the core that
> > must be fixed, please do not workaround it in the driver!
>
>
> It seems some patches is needed in nand_base.c
> -
> diff --git a/drivers/mtd/nand/raw/nand_base.c
> b/drivers/mtd/nand/raw/nand_base.c
> index ddd396e..56be3a9 100644
> --- a/drivers/mtd/nand/raw/nand_base.c
> +++ b/drivers/mtd/nand/raw/nand_base.c
> @@ -3101,7 +3101,8 @@ static int nand_setup_read_retry(struct nand_chip
> *chip, int retry_mode)
> {
> pr_debug("setting READ RETRY mode %d\n", retry_mode);
>
> - if (retry_mode >= chip->read_retries)
> + if (retry_mode > chip->read_retries)
If I understand correctly, chip->read_retries is the total number of
'modes' so the last valid mode is indeed chip->read_retries -1.
I thought the problem would come from the core but I was wrong, you
actually need a MACRONIX_NUM_READ_RETRY_MODES set to 6. Please have two
defines if you need both (the first one being something like
MACRONIX_MAXIMUM_READ_RETRY_MODE set to 5).
Thanks,
Miquèl
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hello,
On Mon, 13 May 2019 15:37:39 +0800
masonccy...@mxic.com.tw wrote:
> ---
> static void macronix_nand_onfi_init(struct nand_chip *chip)
> {
> struct nand_parameters *p = &chip->parameters;
> struct nand_onfi_vendor_macronix *mxic = (void
> *)p->onfi->vendor;
Why cast to void*, instead of casting directly to struct
nand_onfi_vendor_macronix * ?
Also, you are dereferencing p->info before checking whether it's NULL
or not.
> if (!p->onfi ||
> ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0))
> return;
So, the code should be:
struct nand_onfi_vendor_macronix *mxic;
if (!p->onfi)
return;
mxic = (struct nand_onfi_vendor_macronix *) p->info->vendor;
if ((mxic->reliability_func & MACRONIX_READ_RETRY_BIT) == 0)
return;
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hello,
Thomas Petazzoni wrote on Fri, 10 May
2019 15:37:04 +0200:
> Hello,
>
> Some purely cosmetic suggestions below.
>
> On Fri, 10 May 2019 15:41:02 +0800
> Mason Yang wrote:
>
> > + if (ret)
> > + pr_err("set feature failed to read retry moded:%d\n", mode);
>
> I don't know what is the policy in the MTD/NAND subsystem, but
> shouldn't you be using dev_err() instead of pr_err() here to have a
> nice prefix for the message ?
>
> dev_err(&nand_to_mtd(chip)->dev, "set feature ..", mode);
Actually, no, manufacturer initializations happens in
nand_scan_tail() which runs before mtd_device_register(). At this
point, mtd->dev is not yet populated so dev_err() cannot be used. You
should keep a pr_err().
Thanks,
Miquèl
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hi Mason,
Thomas Petazzoni wrote on Fri, 10 May
2019 15:37:04 +0200:
> Hello,
>
> Some purely cosmetic suggestions below.
>
> On Fri, 10 May 2019 15:41:02 +0800
> Mason Yang wrote:
>
> > + if (ret)
> > + pr_err("set feature failed to read retry moded:%d\n", mode);
>
> I don't know what is the policy in the MTD/NAND subsystem, but
> shouldn't you be using dev_err() instead of pr_err() here to have a
> nice prefix for the message ?
>
> dev_err(&nand_to_mtd(chip)->dev, "set feature ..", mode);
Indeed. You can even dereference an mtd_info object first, then use
mtd->dev.
>
> > +static void macronix_nand_onfi_init(struct nand_chip *chip)
> > +{
> > + struct nand_parameters *p = &chip->parameters;
> > +
> > + if (p->onfi) {
>
> Change to:
>
> if (!p->onfi)
> return;
>
> This way the rest of the function can save one level of indentation.
>
> > + struct nand_onfi_vendor_macronix *mxic =
> > + (void *)p->onfi->vendor;
> > +
> > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
>
> Change to:
>
> if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT == 0)
> return;
>
> And the rest of the function can save one level of indentation.
>
> > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
> > + chip->setup_read_retry =
> > +macronix_nand_setup_read_retry;
> > + if (p->supports_set_get_features) {
> > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
> > + p->set_feature_list);
> > + set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
> > + p->get_feature_list);
> > + }
>
> Which will require less wrapping in those lines that are already at the
> third indentation level.
>
> To me, it is also more logical: we exclude the cases we are not
> interested in and return early, and then if we are still in the case we
> are interested, we handle it.
I definitely agree with these cosmetic changes.
Thanks,
Miquèl
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hello,
Some purely cosmetic suggestions below.
On Fri, 10 May 2019 15:41:02 +0800
Mason Yang wrote:
> + if (ret)
> + pr_err("set feature failed to read retry moded:%d\n", mode);
I don't know what is the policy in the MTD/NAND subsystem, but
shouldn't you be using dev_err() instead of pr_err() here to have a
nice prefix for the message ?
dev_err(&nand_to_mtd(chip)->dev, "set feature ..", mode);
> +static void macronix_nand_onfi_init(struct nand_chip *chip)
> +{
> + struct nand_parameters *p = &chip->parameters;
> +
> + if (p->onfi) {
Change to:
if (!p->onfi)
return;
This way the rest of the function can save one level of indentation.
> + struct nand_onfi_vendor_macronix *mxic =
> + (void *)p->onfi->vendor;
> +
> + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
Change to:
if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT == 0)
return;
And the rest of the function can save one level of indentation.
> + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
> + chip->setup_read_retry =
> + macronix_nand_setup_read_retry;
> + if (p->supports_set_get_features) {
> + set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
> + p->set_feature_list);
> + set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
> + p->get_feature_list);
> + }
Which will require less wrapping in those lines that are already at the
third indentation level.
To me, it is also more logical: we exclude the cases we are not
interested in and return early, and then if we are still in the case we
are interested, we handle it.
Best regards,
Thomas
--
Thomas Petazzoni, CTO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hi Mason,
masonccy...@mxic.com.tw wrote on Fri, 10 May 2019 16:51:20 +0800:
> Hi Miquel,
>
>
> > > Add a driver for Macronix NAND read retry.
> >
> > "Add support for Macronix NAND read retry."? This is not a "new driver".
> >
> > >
> > > Macronix NAND supports specfical read for data recovery and enabled
> >
> >
> > Macronix NANDs support specific read operation for data recovery,
> > which can be enabled with a SET_FEATURE.
> >
> > > Driver check byte 167 of Vendor Blocks in ONFI parameter page table
> >
> > checks
> >
> > > to see if this high reliability function is support or not.
> >
> > high-reliability function? not sure it is English
> > anyway.
> >
> > supported
> >
>
> okay, thanks for your review, will patch it to:
>
> Add support for Macronix NAND read retry.
>
> Macronix NANDs support specific read operation for data recovery,
> which can be enabled with a SET_FEATURE.
> Driver checks byte 167 of Vendor Blocks in ONFI parameter page table
> to see if this high-reliability function is supported.
> -
Fine by me.
[...]
> > > + int ret;
> > > +
> > > + if (mode > MACRONIX_READ_RETRY_MODE)
> > > + mode = MACRONIX_READ_RETRY_MODE;
> > > +
> > > + feature[0] = mode;
> > > + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY,
> feature);
> >
> > Don't you miss to select/deselect the target?
>
> nand_select_target() and nand_deselect_target() are already in
> nand_do_read_ops().
Right
>
> >
> > > + if (ret)
> > > + pr_err("set feature failed to read retry moded:%d\n", mode);
> >
> >"Failed to set read retry mode: %d\n"
> >
> > I think you can abort the operation with a negative return code in this
> > case.
> >
>
> After set feature operation, this NAND device need a get feature command
You need to add a comment for this.
> or
> SW reset command to exit read retry mode.
> Therefore, set features command followed by get feature command is needed.
In this case you must check first that both set and get are supported.
>
> > > +
> > > + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY,
> feature);
> >
> > If the operation succeeded but the controller cannot get the feature
> > you don't want to abort the operation. You should check if get_features
> > is supported, if yes you can rely on the below test.
> >
>
> I thought it has been check in macronix_nand_onfi_init() and set by
> set_bit(ONFI_FEATURE_ADDR_READ_RETRY, p->get_feature_list);
You only checked that the operation can be done, you cannot know in
advance if it will actually succeed.
>
> right ?
>
> > > + if (ret || feature[0] != mode)
> > > + pr_err("get feature failed to read retry moded:%d(%d)\n",
> > > + mode, feature[0]);
> >
> >"Failed to verify read retry mode..."
> >
> > Also return something negative here.
> >
> > > +
> > > + return ret;
> >
> > And if all went right, return 0 at the end.
> >
> > > +}
> > > +
> > > +static void macronix_nand_onfi_init(struct nand_chip *chip)
> > > +{
> > > + struct nand_parameters *p = &chip->parameters;
> > > +
> > > + if (p->onfi) {
> > > + struct nand_onfi_vendor_macronix *mxic =
> > > +(void *)p->onfi->vendor;
> >
> > Please put everything on the same line
>
> It will over 80 char.
I know, that's fine here.
>
> >
> > > +
> > > + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
> > > + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
> >
> > Why +1 here, I am missing something?
>
>
> Without + 1, read retry mode is up to 4 rather than 5.
> But this NAND device support read retry mode up to 5.
>
If there are 5 modes, you need to set 5 to chip->read_retries, not 6.
If only 4 modes are used, there is probably a bug in the core that
must be fixed, please do not workaround it in the driver!
Thanks,
Miquèl
Re: [PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Hi Mason,
Mason Yang wrote on Fri, 10 May 2019 15:41:02
+0800:
> Add a driver for Macronix NAND read retry.
"Add support for Macronix NAND read retry."? This is not a "new driver".
>
> Macronix NAND supports specfical read for data recovery and enabled
Macronix NANDs support specific read operation for data recovery,
which can be enabled with a SET_FEATURE.
> Driver check byte 167 of Vendor Blocks in ONFI parameter page table
checks
> to see if this high reliability function is support or not.
high-reliability function? not sure it is English
anyway.
supported
>
> Signed-off-by: Mason Yang
> ---
> drivers/mtd/nand/raw/nand_macronix.c | 52
>
> 1 file changed, 52 insertions(+)
>
> diff --git a/drivers/mtd/nand/raw/nand_macronix.c
> b/drivers/mtd/nand/raw/nand_macronix.c
> index 47d8cda..5cd1e5f 100644
> --- a/drivers/mtd/nand/raw/nand_macronix.c
> +++ b/drivers/mtd/nand/raw/nand_macronix.c
> @@ -17,6 +17,57 @@
>
> #include "internals.h"
>
> +#define MACRONIX_READ_RETRY_BIT BIT(0)
> +#define MACRONIX_READ_RETRY_MODE 5
> +
> +struct nand_onfi_vendor_macronix {
> + u8 reserved[1];
> + u8 reliability_func;
> +} __packed;
> +
> +static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode)
> +{
> + u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0};
0 is not needed here
> + int ret;
> +
> + if (mode > MACRONIX_READ_RETRY_MODE)
> + mode = MACRONIX_READ_RETRY_MODE;
> +
> + feature[0] = mode;
> + ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature);
Don't you miss to select/deselect the target?
> + if (ret)
> + pr_err("set feature failed to read retry moded:%d\n", mode);
"Failed to set read retry mode: %d\n"
I think you can abort the operation with a negative return code in this
case.
> +
> + ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature);
If the operation succeeded but the controller cannot get the feature
you don't want to abort the operation. You should check if get_features
is supported, if yes you can rely on the below test.
> + if (ret || feature[0] != mode)
> + pr_err("get feature failed to read retry moded:%d(%d)\n",
> +mode, feature[0]);
"Failed to verify read retry mode..."
Also return something negative here.
> +
> + return ret;
And if all went right, return 0 at the end.
> +}
> +
> +static void macronix_nand_onfi_init(struct nand_chip *chip)
> +{
> + struct nand_parameters *p = &chip->parameters;
> +
> + if (p->onfi) {
> + struct nand_onfi_vendor_macronix *mxic =
> + (void *)p->onfi->vendor;
Please put everything on the same line
> +
> + if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
> + chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
Why +1 here, I am missing something?
> + chip->setup_read_retry =
> + macronix_nand_setup_read_retry;
> + if (p->supports_set_get_features) {
> + set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
> + p->set_feature_list);
> + set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
> + p->get_feature_list);
Please use bitmap_set()
> + }
> + }
> + }
> +}
> +
> /*
> * Macronix AC series does not support using SET/GET_FEATURES to change
> * the timings unlike what is declared in the parameter page. Unflag
> @@ -65,6 +116,7 @@ static int macronix_nand_init(struct nand_chip *chip)
> chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
>
> macronix_nand_fix_broken_get_timings(chip);
> + macronix_nand_onfi_init(chip);
>
> return 0;
> }
Thanks,
Miquèl
[PATCH v1] mtd: rawnand: Add Macronix NAND read retry support
Add a driver for Macronix NAND read retry.
Macronix NAND supports specfical read for data recovery and enabled
it by Set Feature.
Driver check byte 167 of Vendor Blocks in ONFI parameter page table
to see if this high reliability function is support or not.
Signed-off-by: Mason Yang
---
drivers/mtd/nand/raw/nand_macronix.c | 52
1 file changed, 52 insertions(+)
diff --git a/drivers/mtd/nand/raw/nand_macronix.c
b/drivers/mtd/nand/raw/nand_macronix.c
index 47d8cda..5cd1e5f 100644
--- a/drivers/mtd/nand/raw/nand_macronix.c
+++ b/drivers/mtd/nand/raw/nand_macronix.c
@@ -17,6 +17,57 @@
#include "internals.h"
+#define MACRONIX_READ_RETRY_BIT BIT(0)
+#define MACRONIX_READ_RETRY_MODE 5
+
+struct nand_onfi_vendor_macronix {
+ u8 reserved[1];
+ u8 reliability_func;
+} __packed;
+
+static int macronix_nand_setup_read_retry(struct nand_chip *chip, int mode)
+{
+ u8 feature[ONFI_SUBFEATURE_PARAM_LEN] = {0};
+ int ret;
+
+ if (mode > MACRONIX_READ_RETRY_MODE)
+ mode = MACRONIX_READ_RETRY_MODE;
+
+ feature[0] = mode;
+ ret = nand_set_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature);
+ if (ret)
+ pr_err("set feature failed to read retry moded:%d\n", mode);
+
+ ret = nand_get_features(chip, ONFI_FEATURE_ADDR_READ_RETRY, feature);
+ if (ret || feature[0] != mode)
+ pr_err("get feature failed to read retry moded:%d(%d)\n",
+ mode, feature[0]);
+
+ return ret;
+}
+
+static void macronix_nand_onfi_init(struct nand_chip *chip)
+{
+ struct nand_parameters *p = &chip->parameters;
+
+ if (p->onfi) {
+ struct nand_onfi_vendor_macronix *mxic =
+ (void *)p->onfi->vendor;
+
+ if (mxic->reliability_func & MACRONIX_READ_RETRY_BIT) {
+ chip->read_retries = MACRONIX_READ_RETRY_MODE + 1;
+ chip->setup_read_retry =
+macronix_nand_setup_read_retry;
+ if (p->supports_set_get_features) {
+ set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
+ p->set_feature_list);
+ set_bit(ONFI_FEATURE_ADDR_READ_RETRY,
+ p->get_feature_list);
+ }
+ }
+ }
+}
+
/*
* Macronix AC series does not support using SET/GET_FEATURES to change
* the timings unlike what is declared in the parameter page. Unflag
@@ -65,6 +116,7 @@ static int macronix_nand_init(struct nand_chip *chip)
chip->bbt_options |= NAND_BBT_SCAN2NDPAGE;
macronix_nand_fix_broken_get_timings(chip);
+ macronix_nand_onfi_init(chip);
return 0;
}
--
1.9.1
