| Message ID | 1366616878-29481-4-git-send-email-b32955@freescale.com |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 22, 2013 at 12:47 AM, Huang Shijie <b32955@freescale.com> wrote: > Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page > to store the ECC info. > > The onfi spec tells us that if the nand chip's recommended ECC codeword > size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then > read the Extended ECC information that is part of the extended parameter > page to retrieve the ECC requirements for this device. > > This patch implement the reading of the Extended Parameter Page, and parses > the sections for ECC type, and get the ECC info from the ECC section. > > Tested this patch with Micron MT29F64G08CBABAWP. > > Signed-off-by: Huang Shijie <b32955@freescale.com> > --- > drivers/mtd/nand/nand_base.c | 54 ++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 54 insertions(+), 0 deletions(-) > > diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c > index beff911..48ff097 100644 > --- a/drivers/mtd/nand/nand_base.c > +++ b/drivers/mtd/nand/nand_base.c > @@ -2846,6 +2846,56 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len) > return crc; > } > > +/* Parse the Extended Parameter Page. */ > +static void nand_flash_detect_ext_param_page(struct mtd_info *mtd, > + struct nand_chip *chip, struct nand_onfi_params *p, int last) > +{ I think we want a return code (int) for this function. It obviously can fail, and the caller needs to know this. The "last" parameter is not very obvious until you read the whole function, where you see that this function assumes a lot about the caller. Please address the comments below and/or fully document the parameters and calling context for this function. > + struct onfi_ext_param_page *ep; > + struct onfi_ext_section *s; > + struct onfi_ext_ecc_info *ecc; > + uint8_t *cursor; > + int len; > + int i; > + > + len = le16_to_cpu(p->ext_param_page_length) * 16; > + ep = kcalloc(1, max_t(int, len, sizeof(*p)), GFP_KERNEL); > + if (!ep) > + goto ext_out; > + > + /* > + * Skip the ONFI Parameter Pages. > + * The Change Read Columm command may does not works here. Why not? > + */ > + for (i = last + 1; i < p->num_of_param_pages; i++) > + chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p)); You never sent a command to the chip. How can you expect to read from it? It seems that you are writing this function with the assumption of a particular calling context (a context in which the last command was CMD_PARAM). IMO, it would make a lot more sense that this function actually send its own CMD_PARAM followed by either X bytes of skipped read_buf() or a change read column command. Then it doesn't need the "last" argument, and its usage makes much more sense. > + > + /* Read out the Extended Parameter Page. */ > + chip->read_buf(mtd, (uint8_t *)ep, len); > + if ((onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2) > + != le16_to_cpu(ep->crc)) || strncmp(ep->sig, "EPPS", 4)) > + goto ext_out; > + > + /* find the ECC section. */ > + cursor = (uint8_t *)(ep + 1); > + for (i = 0; i < ONFI_EXT_SECTION_MAX; i++) { > + s = ep->sections + i; > + if (s->type == ONFI_SECTION_TYPE_2) > + break; > + cursor += s->length * 16; > + } > + if (i == ONFI_EXT_SECTION_MAX) > + goto ext_out; > + > + /* get the info we want. */ > + ecc = (struct onfi_ext_ecc_info *)cursor; > + chip->ecc_strength = ecc->ecc_bits; > + chip->ecc_size = 1 << ecc->codeword_size; > + > + pr_info("ONFI extended param page detected.\n"); > +ext_out: > + kfree(ep); > +} > + > /* > * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise. > */ > @@ -2914,6 +2964,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, > if (p->ecc_bits != 0xff) { > chip->ecc_strength = p->ecc_bits; > chip->ecc_size = 512; > + } else if (chip->onfi_version >= 21 && > + (onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) { > + /* The Extended Parameter Page is supported since ONFI 2.1. */ > + nand_flash_detect_ext_param_page(mtd, chip, p, i); > } > > pr_info("ONFI flash detected\n"); > -- > 1.7.1 > >
于 2013年04月23日 05:22, Brian Norris 写道: > On Mon, Apr 22, 2013 at 12:47 AM, Huang Shijie<b32955@freescale.com> wrote: >> Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page >> to store the ECC info. >> >> The onfi spec tells us that if the nand chip's recommended ECC codeword >> size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then >> read the Extended ECC information that is part of the extended parameter >> page to retrieve the ECC requirements for this device. >> >> This patch implement the reading of the Extended Parameter Page, and parses >> the sections for ECC type, and get the ECC info from the ECC section. >> >> Tested this patch with Micron MT29F64G08CBABAWP. >> >> Signed-off-by: Huang Shijie<b32955@freescale.com> >> --- >> drivers/mtd/nand/nand_base.c | 54 ++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 54 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >> index beff911..48ff097 100644 >> --- a/drivers/mtd/nand/nand_base.c >> +++ b/drivers/mtd/nand/nand_base.c >> @@ -2846,6 +2846,56 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len) >> return crc; >> } >> >> +/* Parse the Extended Parameter Page. */ >> +static void nand_flash_detect_ext_param_page(struct mtd_info *mtd, >> + struct nand_chip *chip, struct nand_onfi_params *p, int last) >> +{ > I think we want a return code (int) for this function. It obviously > can fail, and the caller needs to know this. I not sure who will uses ecc_strength/ecc_size, except the gpmi. So i ignore the return value. If you think we should add it, i will add it. > The "last" parameter is not very obvious until you read the whole > function, where you see that this function assumes a lot about the > caller. Please address the comments below and/or fully document the > parameters and calling context for this function. > ok. I can add more comments. >> + struct onfi_ext_param_page *ep; >> + struct onfi_ext_section *s; >> + struct onfi_ext_ecc_info *ecc; >> + uint8_t *cursor; >> + int len; >> + int i; >> + >> + len = le16_to_cpu(p->ext_param_page_length) * 16; >> + ep = kcalloc(1, max_t(int, len, sizeof(*p)), GFP_KERNEL); >> + if (!ep) >> + goto ext_out; >> + >> + /* >> + * Skip the ONFI Parameter Pages. >> + * The Change Read Columm command may does not works here. > Why not? > You can give me a fix patch which bases on my patch set. I can test it. :) I tried to use the Change-read-columm command, but failed. >> + */ >> + for (i = last + 1; i< p->num_of_param_pages; i++) >> + chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p)); > You never sent a command to the chip. How can you expect to read from it? we have sent a command in the nand_flash_detect_onfi(). > It seems that you are writing this function with the assumption of a > particular calling context (a context in which the last command was > CMD_PARAM). IMO, it would make a lot more sense that this function > actually send its own CMD_PARAM followed by either X bytes of skipped > read_buf() or a change read column command. Then it doesn't need the > "last" argument, and its usage makes much more sense. > I added the "last" argument just because the Change-read-column command did not works. thanks Huang Shijie
于 2013年04月23日 05:22, Brian Norris 写道: > You never sent a command to the chip. How can you expect to read from it? > > It seems that you are writing this function with the assumption of a > particular calling context (a context in which the last command was > CMD_PARAM). IMO, it would make a lot more sense that this function > actually send its own CMD_PARAM followed by either X bytes of skipped > read_buf() or a change read column command. Then it doesn't need the > "last" argument, and its usage makes much more sense. > I finally find why i can not use the CHANGE READ COLUMN command: When we detect the ONFI nand, we actually use the nand_command() to issue the command which does not works with CHANGE READ COLUMN command. I use the nand_command_lp() to issue the CHANGE READ COLUMN command, it works fine. I can remove the "last" argument now. thanks Brian. Huang Shijie
On Mon, Apr 22, 2013 at 7:43 PM, Huang Shijie <b32955@freescale.com> wrote: > 于 2013年04月23日 05:22, Brian Norris 写道: > >> On Mon, Apr 22, 2013 at 12:47 AM, Huang Shijie<b32955@freescale.com> >> wrote: >>> >>> Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page >>> to store the ECC info. >>> >>> The onfi spec tells us that if the nand chip's recommended ECC codeword >>> size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then >>> read the Extended ECC information that is part of the extended parameter >>> page to retrieve the ECC requirements for this device. >>> >>> This patch implement the reading of the Extended Parameter Page, and >>> parses >>> the sections for ECC type, and get the ECC info from the ECC section. >>> >>> Tested this patch with Micron MT29F64G08CBABAWP. >>> >>> Signed-off-by: Huang Shijie<b32955@freescale.com> >>> --- >>> drivers/mtd/nand/nand_base.c | 54 >>> ++++++++++++++++++++++++++++++++++++++++++ >>> 1 files changed, 54 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c >>> index beff911..48ff097 100644 >>> --- a/drivers/mtd/nand/nand_base.c >>> +++ b/drivers/mtd/nand/nand_base.c >>> @@ -2846,6 +2846,56 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t >>> len) >>> return crc; >>> } >>> >>> +/* Parse the Extended Parameter Page. */ >>> +static void nand_flash_detect_ext_param_page(struct mtd_info *mtd, >>> + struct nand_chip *chip, struct nand_onfi_params *p, int >>> last) >>> +{ >> >> I think we want a return code (int) for this function. It obviously >> can fail, and the caller needs to know this. > > I not sure who will uses ecc_strength/ecc_size, except the gpmi. > So i ignore the return value. Well, that's the dangerous part of this patch series: that it is written entirely from the point of view of a specific use case (gpmi). It can be improved a little to make it more generically useful. > If you think we should add it, i will add it. Well, the code doesn't clearly show what happens to ecc_strength/ecc_size if (1) we are out of memory (but we are hosed in this case anyway...) or (2) the extended parameter page is corrupted and/or not present. In either case, the caller may want to zero out the the parameters, set them to -1, or whatever else we decide for the null case. >> The "last" parameter is not very obvious until you read the whole >> function, where you see that this function assumes a lot about the >> caller. Please address the comments below and/or fully document the >> parameters and calling context for this function. >> > ok. I can add more comments. > > >>> + struct onfi_ext_param_page *ep; >>> + struct onfi_ext_section *s; >>> + struct onfi_ext_ecc_info *ecc; >>> + uint8_t *cursor; >>> + int len; >>> + int i; >>> + >>> + len = le16_to_cpu(p->ext_param_page_length) * 16; >>> + ep = kcalloc(1, max_t(int, len, sizeof(*p)), GFP_KERNEL); Does this need to be zeroed out? We will overwrite it before using it anyway. I'd just recommend kmalloc. >>> + if (!ep) >>> + goto ext_out; >>> + >>> + /* >>> + * Skip the ONFI Parameter Pages. >>> + * The Change Read Columm command may does not works here. >> >> Why not? >> > You can give me a fix patch which bases on my patch set. > I can test it. :) > > I tried to use the Change-read-columm command, but failed. I believe the behavior here will really depend on the driver used. My driver, for instance, intercepts these commands and sends them to the controller in a different form. So my patches won't necessarily help you if your driver is broken :) >>> + */ >>> + for (i = last + 1; i< p->num_of_param_pages; i++) >>> + chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p)); >> >> You never sent a command to the chip. How can you expect to read from it? > > we have sent a command in the nand_flash_detect_onfi(). Right, I understand. But it's not very clean code if it makes this assumption w/o documenting it. >> It seems that you are writing this function with the assumption of a >> particular calling context (a context in which the last command was >> CMD_PARAM). IMO, it would make a lot more sense that this function >> actually send its own CMD_PARAM followed by either X bytes of skipped >> read_buf() or a change read column command. Then it doesn't need the >> "last" argument, and its usage makes much more sense. >> > I added the "last" argument just because the Change-read-column command did > not works. Brian
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c index beff911..48ff097 100644 --- a/drivers/mtd/nand/nand_base.c +++ b/drivers/mtd/nand/nand_base.c @@ -2846,6 +2846,56 @@ static u16 onfi_crc16(u16 crc, u8 const *p, size_t len) return crc; } +/* Parse the Extended Parameter Page. */ +static void nand_flash_detect_ext_param_page(struct mtd_info *mtd, + struct nand_chip *chip, struct nand_onfi_params *p, int last) +{ + struct onfi_ext_param_page *ep; + struct onfi_ext_section *s; + struct onfi_ext_ecc_info *ecc; + uint8_t *cursor; + int len; + int i; + + len = le16_to_cpu(p->ext_param_page_length) * 16; + ep = kcalloc(1, max_t(int, len, sizeof(*p)), GFP_KERNEL); + if (!ep) + goto ext_out; + + /* + * Skip the ONFI Parameter Pages. + * The Change Read Columm command may does not works here. + */ + for (i = last + 1; i < p->num_of_param_pages; i++) + chip->read_buf(mtd, (uint8_t *)ep, sizeof(*p)); + + /* Read out the Extended Parameter Page. */ + chip->read_buf(mtd, (uint8_t *)ep, len); + if ((onfi_crc16(ONFI_CRC_BASE, ((uint8_t *)ep) + 2, len - 2) + != le16_to_cpu(ep->crc)) || strncmp(ep->sig, "EPPS", 4)) + goto ext_out; + + /* find the ECC section. */ + cursor = (uint8_t *)(ep + 1); + for (i = 0; i < ONFI_EXT_SECTION_MAX; i++) { + s = ep->sections + i; + if (s->type == ONFI_SECTION_TYPE_2) + break; + cursor += s->length * 16; + } + if (i == ONFI_EXT_SECTION_MAX) + goto ext_out; + + /* get the info we want. */ + ecc = (struct onfi_ext_ecc_info *)cursor; + chip->ecc_strength = ecc->ecc_bits; + chip->ecc_size = 1 << ecc->codeword_size; + + pr_info("ONFI extended param page detected.\n"); +ext_out: + kfree(ep); +} + /* * Check if the NAND chip is ONFI compliant, returns 1 if it is, 0 otherwise. */ @@ -2914,6 +2964,10 @@ static int nand_flash_detect_onfi(struct mtd_info *mtd, struct nand_chip *chip, if (p->ecc_bits != 0xff) { chip->ecc_strength = p->ecc_bits; chip->ecc_size = 512; + } else if (chip->onfi_version >= 21 && + (onfi_feature(chip) & ONFI_FEATURE_EXT_PARAM_PAGE)) { + /* The Extended Parameter Page is supported since ONFI 2.1. */ + nand_flash_detect_ext_param_page(mtd, chip, p, i); } pr_info("ONFI flash detected\n");
Since the ONFI 2.1, the onfi spec adds the Extended Parameter Page to store the ECC info. The onfi spec tells us that if the nand chip's recommended ECC codeword size is not 512 bytes, then the @ecc_bits is 0xff. The host _SHOULD_ then read the Extended ECC information that is part of the extended parameter page to retrieve the ECC requirements for this device. This patch implement the reading of the Extended Parameter Page, and parses the sections for ECC type, and get the ECC info from the ECC section. Tested this patch with Micron MT29F64G08CBABAWP. Signed-off-by: Huang Shijie <b32955@freescale.com> --- drivers/mtd/nand/nand_base.c | 54 ++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 54 insertions(+), 0 deletions(-)