| Message ID | 1525350041-22995-10-git-send-email-absahu@codeaurora.org |
|---|---|
| State | Superseded |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | Update for QCOM NAND driver | expand |
Hi Abhishek, Some nitpicking below. On Thu, 3 May 2018 17:50:36 +0530, Abhishek Sahu <absahu@codeaurora.org> wrote: > QCOM NAND layout protect available OOB data bytes with ECC also so ^controller > when ecc->write_oob (qcom_nandc_write_oob) is being called then it You can just state "->write_oob()" > can't update just OOB bytes. Currently, it first reads the last > codeword which includes old OOB bytes. Then it updates the old OOB > bytes with new one and then again writes the codeword back. ones? > The reading codeword is unnecessary since all the other bytes > should be 0xff only. since the user is responsible to have these bytes cleared to 0xFF. > > This patch removes the read part and updates the oob bytes with s/oob/OOB/ > all other data bytes as 0xff. The end of the sentence is not clear for me. Do you mean that padding with 0xFF is realized before write? > > Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> > --- > * Changes from v1: > > NEW CHANGE > > drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > index 61d0e7d..f85d8ab 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd, > * implements ecc->write_oob() > * > * the NAND controller cannot write only data or only oob within a codeword, s/oob/OOB/ Remove the trailing ',' > - * since ecc is calculated for the combined codeword. we first copy the > - * entire contents for the last codeword(data + oob), replace the old oob > - * with the new one in chip->oob_poi, and then write the entire codeword. > - * this read-copy-write operation results in a slight performance loss. > + * since ecc is calculated for the combined codeword. So make all the data s/ecc/ECC/ > + * bytes as 0xff and update the oob from chip->oob_poi, and then write > + * the entire codeword again. What about "Pad the data area with OxFF before writing."? > */ > static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, > int page) > @@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, > int data_size, oob_size; > int ret; > > - host->use_ecc = true; > - > - clear_bam_transaction(nandc); > - ret = copy_last_cw(host, page); > - if (ret) > - return ret; > - > - clear_read_regs(nandc); > clear_bam_transaction(nandc); > > /* calculate the data and oob size for the last codeword/step */ > data_size = ecc->size - ((ecc->steps - 1) << 2); > oob_size = mtd->oobavail; > + host->use_ecc = true; You don't need to move this line here, do you? > > + memset(nandc->data_buffer, 0xff, host->cw_data); > /* override new oob content to last codeword */ > mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob, > 0, mtd->oobavail); Once fixed, you can add my: Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> Thanks, Miquèl
On 2018-05-22 15:32, Miquel Raynal wrote: > Hi Abhishek, > > Some nitpicking below. > > On Thu, 3 May 2018 17:50:36 +0530, Abhishek Sahu > <absahu@codeaurora.org> wrote: > >> QCOM NAND layout protect available OOB data bytes with ECC also so > > ^controller > >> when ecc->write_oob (qcom_nandc_write_oob) is being called then it > > You can just state "->write_oob()" > >> can't update just OOB bytes. Currently, it first reads the last >> codeword which includes old OOB bytes. Then it updates the old OOB >> bytes with new one and then again writes the codeword back. > > ones? > >> The reading codeword is unnecessary since all the other bytes >> should be 0xff only. > > since the user is responsible to > have these bytes cleared to 0xFF. > >> >> This patch removes the read part and updates the oob bytes with > > s/oob/OOB/ > >> all other data bytes as 0xff. > > The end of the sentence is not clear for me. Do you mean that padding > with 0xFF is realized before write? > >> >> Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> >> --- >> * Changes from v1: >> >> NEW CHANGE >> >> drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------ >> 1 file changed, 5 insertions(+), 12 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c >> b/drivers/mtd/nand/raw/qcom_nandc.c >> index 61d0e7d..f85d8ab 100644 >> --- a/drivers/mtd/nand/raw/qcom_nandc.c >> +++ b/drivers/mtd/nand/raw/qcom_nandc.c >> @@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct >> mtd_info *mtd, >> * implements ecc->write_oob() >> * >> * the NAND controller cannot write only data or only oob within a >> codeword, > > s/oob/OOB/ > > Remove the trailing ',' > >> - * since ecc is calculated for the combined codeword. we first copy >> the >> - * entire contents for the last codeword(data + oob), replace the old >> oob >> - * with the new one in chip->oob_poi, and then write the entire >> codeword. >> - * this read-copy-write operation results in a slight performance >> loss. >> + * since ecc is calculated for the combined codeword. So make all the >> data > > s/ecc/ECC/ > >> + * bytes as 0xff and update the oob from chip->oob_poi, and then >> write >> + * the entire codeword again. > > What about "Pad the data area with OxFF before writing."? > >> */ >> static int qcom_nandc_write_oob(struct mtd_info *mtd, struct >> nand_chip *chip, >> int page) >> @@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct >> mtd_info *mtd, struct nand_chip *chip, >> int data_size, oob_size; >> int ret; >> >> - host->use_ecc = true; >> - >> - clear_bam_transaction(nandc); >> - ret = copy_last_cw(host, page); >> - if (ret) >> - return ret; >> - >> - clear_read_regs(nandc); >> clear_bam_transaction(nandc); >> >> /* calculate the data and oob size for the last codeword/step */ >> data_size = ecc->size - ((ecc->steps - 1) << 2); >> oob_size = mtd->oobavail; >> + host->use_ecc = true; > > You don't need to move this line here, do you? > >> >> + memset(nandc->data_buffer, 0xff, host->cw_data); >> /* override new oob content to last codeword */ >> mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, >> oob, >> 0, mtd->oobavail); > > > Once fixed, you can add my: Thanks Miquel. I will fix all these and update the patch. Regards, Abhishek > > Acked-by: Miquel Raynal <miquel.raynal@bootlin.com> > > Thanks, > Miquèl
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index 61d0e7d..f85d8ab 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -2067,10 +2067,9 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd, * implements ecc->write_oob() * * the NAND controller cannot write only data or only oob within a codeword, - * since ecc is calculated for the combined codeword. we first copy the - * entire contents for the last codeword(data + oob), replace the old oob - * with the new one in chip->oob_poi, and then write the entire codeword. - * this read-copy-write operation results in a slight performance loss. + * since ecc is calculated for the combined codeword. So make all the data + * bytes as 0xff and update the oob from chip->oob_poi, and then write + * the entire codeword again. */ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int page) @@ -2082,20 +2081,14 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip, int data_size, oob_size; int ret; - host->use_ecc = true; - - clear_bam_transaction(nandc); - ret = copy_last_cw(host, page); - if (ret) - return ret; - - clear_read_regs(nandc); clear_bam_transaction(nandc); /* calculate the data and oob size for the last codeword/step */ data_size = ecc->size - ((ecc->steps - 1) << 2); oob_size = mtd->oobavail; + host->use_ecc = true; + memset(nandc->data_buffer, 0xff, host->cw_data); /* override new oob content to last codeword */ mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob, 0, mtd->oobavail);
QCOM NAND layout protect available OOB data bytes with ECC also so when ecc->write_oob (qcom_nandc_write_oob) is being called then it can't update just OOB bytes. Currently, it first reads the last codeword which includes old OOB bytes. Then it updates the old OOB bytes with new one and then again writes the codeword back. The reading codeword is unnecessary since all the other bytes should be 0xff only. This patch removes the read part and updates the oob bytes with all other data bytes as 0xff. Signed-off-by: Abhishek Sahu <absahu@codeaurora.org> --- * Changes from v1: NEW CHANGE drivers/mtd/nand/raw/qcom_nandc.c | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)