| Message ID | 1413045778-5690-7-git-send-email-marex@denx.de |
|---|---|
| State | Changes Requested |
| Delegated to: | Scott Wood |
| Headers | show |
On Sat, 2014-10-11 at 18:42 +0200, Marek Vasut wrote: > + /* sometimes people do not think about using the ECC, so check > + * to see if we have an 0xff,0xff,0xff read ECC and then ignore > + * the error, on the assumption that this is an un-eccd page. > + */ Eww. I suppose I won't argue too loudly if Linux is doing the same thing, but what if it's a corrupted blank page, or the ECC just happened to turn out as all 0xff? It seems like there should at least be a warning the first time this happens, and ideally it should be configurable. > + if (read_ecc[0] == 0xff && read_ecc[1] == 0xff && read_ecc[2] == 0xff > + /*&& info->platform->ignore_unset_ecc*/) > return 0; So it looks like it is configurable in Linux, but you've commented it out here. > @@ -221,6 +298,8 @@ int board_nand_init(struct nand_chip *nand) > > nand->dev_ready = s3c24x0_dev_ready; > > + nand->chip_delay = 50; I'm not sure how this is related to the changes described in the changelog... -Scott
On Tuesday, October 28, 2014 at 11:45:08 PM, Scott Wood wrote: > On Sat, 2014-10-11 at 18:42 +0200, Marek Vasut wrote: > > + /* sometimes people do not think about using the ECC, so check > > + * to see if we have an 0xff,0xff,0xff read ECC and then ignore > > + * the error, on the assumption that this is an un-eccd page. > > + */ > > Eww. I suppose I won't argue too loudly if Linux is doing the same > thing, but what if it's a corrupted blank page, or the ECC just happened > to turn out as all 0xff? It seems like there should at least be a > warning the first time this happens, and ideally it should be > configurable. > > > + if (read_ecc[0] == 0xff && read_ecc[1] == 0xff && read_ecc[2] == 0xff > > + /*&& info->platform->ignore_unset_ecc*/) > > > > return 0; > > So it looks like it is configurable in Linux, but you've commented it > out here. > > > @@ -221,6 +298,8 @@ int board_nand_init(struct nand_chip *nand) > > > > nand->dev_ready = s3c24x0_dev_ready; > > > > + nand->chip_delay = 50; > > I'm not sure how this is related to the changes described in the > changelog... Can you collect the MTD patches which are applicable at least and drop this one? Best regards, Marek Vasut
On Thu, 2016-01-14 at 02:41 +0100, Marek Vasut wrote: > On Tuesday, October 28, 2014 at 11:45:08 PM, Scott Wood wrote: > > On Sat, 2014-10-11 at 18:42 +0200, Marek Vasut wrote: > > > + /* sometimes people do not think about using the ECC, so check > > > + * to see if we have an 0xff,0xff,0xff read ECC and then ignore > > > + * the error, on the assumption that this is an un-eccd page. > > > + */ > > > > Eww. I suppose I won't argue too loudly if Linux is doing the same > > thing, but what if it's a corrupted blank page, or the ECC just happened > > to turn out as all 0xff? It seems like there should at least be a > > warning the first time this happens, and ideally it should be > > configurable. > > > > > + if (read_ecc[0] == 0xff && read_ecc[1] == 0xff && read_ecc[2] > > > == 0xff > > > + /*&& info->platform->ignore_unset_ecc*/) > > > > > > return 0; > > > > So it looks like it is configurable in Linux, but you've commented it > > out here. > > > > > @@ -221,6 +298,8 @@ int board_nand_init(struct nand_chip *nand) > > > > > > nand->dev_ready = s3c24x0_dev_ready; > > > > > > + nand->chip_delay = 50; > > > > I'm not sure how this is related to the changes described in the > > changelog... > > Can you collect the MTD patches which are applicable at least and drop this > one? 4/10 is already merged. Which patches are you referring to that don't have comments, still apply cleanly, and are patching a NAND file? -Scott
On 02/13/2016 12:18 AM, Scott Wood wrote: > On Thu, 2016-01-14 at 02:41 +0100, Marek Vasut wrote: >> On Tuesday, October 28, 2014 at 11:45:08 PM, Scott Wood wrote: >>> On Sat, 2014-10-11 at 18:42 +0200, Marek Vasut wrote: >>>> + /* sometimes people do not think about using the ECC, so check >>>> + * to see if we have an 0xff,0xff,0xff read ECC and then ignore >>>> + * the error, on the assumption that this is an un-eccd page. >>>> + */ >>> >>> Eww. I suppose I won't argue too loudly if Linux is doing the same >>> thing, but what if it's a corrupted blank page, or the ECC just happened >>> to turn out as all 0xff? It seems like there should at least be a >>> warning the first time this happens, and ideally it should be >>> configurable. >>> >>>> + if (read_ecc[0] == 0xff && read_ecc[1] == 0xff && read_ecc[2] >>>> == 0xff >>>> + /*&& info->platform->ignore_unset_ecc*/) >>>> >>>> return 0; >>> >>> So it looks like it is configurable in Linux, but you've commented it >>> out here. >>> >>>> @@ -221,6 +298,8 @@ int board_nand_init(struct nand_chip *nand) >>>> >>>> nand->dev_ready = s3c24x0_dev_ready; >>>> >>>> + nand->chip_delay = 50; >>> >>> I'm not sure how this is related to the changes described in the >>> changelog... >> >> Can you collect the MTD patches which are applicable at least and drop this >> one? > > 4/10 is already merged. Which patches are you referring to that don't have > comments, still apply cleanly, and are patching a NAND file? Most of this patchset.
On Fri, 2016-02-19 at 18:53 +0100, Marek Vasut wrote: > On 02/13/2016 12:18 AM, Scott Wood wrote: > > On Thu, 2016-01-14 at 02:41 +0100, Marek Vasut wrote: > > > On Tuesday, October 28, 2014 at 11:45:08 PM, Scott Wood wrote: > > > > On Sat, 2014-10-11 at 18:42 +0200, Marek Vasut wrote: > > > > > + /* sometimes people do not think about using the ECC, so > > > > > check > > > > > + * to see if we have an 0xff,0xff,0xff read ECC and then > > > > > ignore > > > > > + * the error, on the assumption that this is an un-eccd > > > > > page. > > > > > + */ > > > > > > > > Eww. I suppose I won't argue too loudly if Linux is doing the same > > > > thing, but what if it's a corrupted blank page, or the ECC just > > > > happened > > > > to turn out as all 0xff? It seems like there should at least be a > > > > warning the first time this happens, and ideally it should be > > > > configurable. > > > > > > > > > + if (read_ecc[0] == 0xff && read_ecc[1] == 0xff && > > > > > read_ecc[2] > > > > > == 0xff > > > > > + /*&& info->platform->ignore_unset_ecc*/) > > > > > > > > > > return 0; > > > > > > > > So it looks like it is configurable in Linux, but you've commented it > > > > out here. > > > > > > > > > @@ -221,6 +298,8 @@ int board_nand_init(struct nand_chip *nand) > > > > > > > > > > nand->dev_ready = s3c24x0_dev_ready; > > > > > > > > > > + nand->chip_delay = 50; > > > > > > > > I'm not sure how this is related to the changes described in the > > > > changelog... > > > > > > Can you collect the MTD patches which are applicable at least and drop > > > this > > > one? > > > > 4/10 is already merged. Which patches are you referring to that don't > > have > > comments, still apply cleanly, and are patching a NAND file? > > Most of this patchset. Give me one example. I couldn't find any last time I looked. -Scott
diff --git a/drivers/mtd/nand/s3c2410_nand.c b/drivers/mtd/nand/s3c2410_nand.c index 91db6ca..f23a1c8 100644 --- a/drivers/mtd/nand/s3c2410_nand.c +++ b/drivers/mtd/nand/s3c2410_nand.c @@ -157,16 +157,93 @@ static int s3c24x0_nand_calculate_ecc(struct mtd_info *mtd, const u_char *dat, static int s3c24x0_nand_correct_data(struct mtd_info *mtd, u_char *dat, u_char *read_ecc, u_char *calc_ecc) { - if (read_ecc[0] == calc_ecc[0] && - read_ecc[1] == calc_ecc[1] && - read_ecc[2] == calc_ecc[2]) + unsigned int diff0, diff1, diff2; + unsigned int bit, byte; + + debug("%s(%p,%p,%p,%p)\n", __func__, mtd, dat, read_ecc, calc_ecc); + + diff0 = read_ecc[0] ^ calc_ecc[0]; + diff1 = read_ecc[1] ^ calc_ecc[1]; + diff2 = read_ecc[2] ^ calc_ecc[2]; + + debug("%s: rd %*phN calc %*phN diff %02x%02x%02x\n", + __func__, 3, read_ecc, 3, calc_ecc, + diff0, diff1, diff2); + + if (diff0 == 0 && diff1 == 0 && diff2 == 0) + return 0; /* ECC is ok */ + + /* sometimes people do not think about using the ECC, so check + * to see if we have an 0xff,0xff,0xff read ECC and then ignore + * the error, on the assumption that this is an un-eccd page. + */ + if (read_ecc[0] == 0xff && read_ecc[1] == 0xff && read_ecc[2] == 0xff + /*&& info->platform->ignore_unset_ecc*/) return 0; - printf("s3c24x0_nand_correct_data: not implemented\n"); + /* Can we correct this ECC (ie, one row and column change). + * Note, this is similar to the 256 error code on smartmedia */ + + if (((diff0 ^ (diff0 >> 1)) & 0x55) == 0x55 && + ((diff1 ^ (diff1 >> 1)) & 0x55) == 0x55 && + ((diff2 ^ (diff2 >> 1)) & 0x55) == 0x55) { + /* calculate the bit position of the error */ + + bit = ((diff2 >> 3) & 1) | + ((diff2 >> 4) & 2) | + ((diff2 >> 5) & 4); + + /* calculate the byte position of the error */ + + byte = ((diff2 << 7) & 0x100) | + ((diff1 << 0) & 0x80) | + ((diff1 << 1) & 0x40) | + ((diff1 << 2) & 0x20) | + ((diff1 << 3) & 0x10) | + ((diff0 >> 4) & 0x08) | + ((diff0 >> 3) & 0x04) | + ((diff0 >> 2) & 0x02) | + ((diff0 >> 1) & 0x01); + + debug("correcting error bit %d, byte %d\n", bit, byte); + + dat[byte] ^= (1 << bit); + return 1; + } + + /* if there is only one bit difference in the ECC, then + * one of only a row or column parity has changed, which + * means the error is most probably in the ECC itself */ + + diff0 |= (diff1 << 8); + diff0 |= (diff2 << 16); + + if ((diff0 & ~(1<<fls(diff0))) == 0) + return 1; + return -1; } #endif +static void s3c24x0_nand_select_chip(struct mtd_info *mtd, int chip) +{ + struct s3c24x0_nand *nand = s3c24x0_get_base_nand(); + uint32_t sel_reg, sel_bit; + +#ifdef CONFIG_S3C2410 + sel_reg = (uint32_t)&nand->nfconf; + sel_bit = S3C2410_NFCONF_nFCE; +#else + sel_reg = (uint32_t)&nand->nfcont; + sel_bit = S3C2440_NFCONT_nFCE; +#endif + + if (chip == -1) + setbits_le32(sel_reg, sel_bit); + else + clrbits_le32(sel_reg, sel_bit); +} + int board_nand_init(struct nand_chip *nand) { uint32_t cfg = 0; @@ -205,7 +282,7 @@ int board_nand_init(struct nand_chip *nand) nand->IO_ADDR_R = (void *)&nand_reg->nfdata; nand->IO_ADDR_W = (void *)&nand_reg->nfdata; - nand->select_chip = NULL; + nand->select_chip = s3c24x0_nand_select_chip; /* read_buf and write_buf are default */ /* read_byte and write_byte are default */ @@ -221,6 +298,8 @@ int board_nand_init(struct nand_chip *nand) nand->dev_ready = s3c24x0_dev_ready; + nand->chip_delay = 50; + #ifdef CONFIG_S3C2410_NAND_HWECC nand->ecc.hwctl = s3c24x0_nand_enable_hwecc; nand->ecc.calculate = s3c24x0_nand_calculate_ecc;
Implant a missing ECC correction implementation and select_chip implementation from Linux. Signed-off-by: Marek Vasut <marex@denx.de> Cc: Kyungmin Park <kyungmin.park@samsung.com> Cc: Lukasz Majewski <l.majewski@samsung.com> Cc: Minkyu Kang <mk7.kang@samsung.com> Cc: Scott Wood <scottwood@freescale.com> Cc: Vladimir Zapolskiy <vz@mleia.com> --- drivers/mtd/nand/s3c2410_nand.c | 89 ++++++++++++++++++++++++++++++++++++++--- 1 file changed, 84 insertions(+), 5 deletions(-)