| Message ID | 1301653534-22223-1-git-send-email-viresh.kumar@st.com |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2011-04-01 at 15:55 +0530, ext Viresh Kumar wrote: > + /* > + * This is a temporary erase check. A newly erased page read > + * would result in an ecc error because the oob data is also > + * erased to FF and the calculated ecc for an FF data is not > + * FF..FF. > + * This is a workaround to skip performing correction in case > + * data is FF..FF > + */ Sorry, but still questions. The comment says this is temporary - why? When and how this going to be change to become "permanent". And you still did not add words about the problem with bit-flips.
On 4/1/2011 5:00 PM, Artem Bityutskiy wrote: > On Fri, 2011-04-01 at 15:55 +0530, ext Viresh Kumar wrote: >> + /* >> + * This is a temporary erase check. A newly erased page read >> + * would result in an ecc error because the oob data is also >> + * erased to FF and the calculated ecc for an FF data is not >> + * FF..FF. >> + * This is a workaround to skip performing correction in case >> + * data is FF..FF >> + */ > > Sorry, but still questions. > > The comment says this is temporary - why? When and how this going to be > change to become "permanent". > Temporary because the fsmc peripheral itself needs a fix for this problem > And you still did not add words about the problem with bit-flips. > OK, I would add that too
On Fri, 2011-04-01 at 17:12 +0530, ext Vipin Kumar wrote:
> Temporary because the fsmc peripheral itself needs a fix for this problem
Do you mean a hardware fix?
On 4/1/2011 5:14 PM, Artem Bityutskiy wrote: > On Fri, 2011-04-01 at 17:12 +0530, ext Vipin Kumar wrote: >> Temporary because the fsmc peripheral itself needs a fix for this problem > > Do you mean a hardware fix? > Ideally speaking, Yes Vipin
On Mon, 2011-04-04 at 11:15 +0530, ext Vipin Kumar wrote: > On 4/1/2011 5:14 PM, Artem Bityutskiy wrote: > > On Fri, 2011-04-01 at 17:12 +0530, ext Vipin Kumar wrote: > >> Temporary because the fsmc peripheral itself needs a fix for this problem > > > > Do you mean a hardware fix? > > > Ideally speaking, Yes Then the comment is not saying the truth I think - this solution is permanent.
On 4/4/2011 11:57 AM, Artem Bityutskiy wrote: > On Mon, 2011-04-04 at 11:15 +0530, ext Vipin Kumar wrote: >> On 4/1/2011 5:14 PM, Artem Bityutskiy wrote: >>> On Fri, 2011-04-01 at 17:12 +0530, ext Vipin Kumar wrote: >>>> Temporary because the fsmc peripheral itself needs a fix for this problem >>> >>> Do you mean a hardware fix? >>> >> Ideally speaking, Yes > > Then the comment is not saying the truth I think - this solution is > permanent. > That's why it is a workaround I thought "workaround" says it all Regards Vipin
On Mon, 2011-04-04 at 14:30 +0530, Vipin Kumar wrote: > On 4/4/2011 11:57 AM, Artem Bityutskiy wrote: > > On Mon, 2011-04-04 at 11:15 +0530, ext Vipin Kumar wrote: > >> On 4/1/2011 5:14 PM, Artem Bityutskiy wrote: > >>> On Fri, 2011-04-01 at 17:12 +0530, ext Vipin Kumar wrote: > >>>> Temporary because the fsmc peripheral itself needs a fix for this problem > >>> > >>> Do you mean a hardware fix? > >>> > >> Ideally speaking, Yes > > > > Then the comment is not saying the truth I think - this solution is > > permanent. > > > > That's why it is a workaround > I thought "workaround" says it all I thought we were talking about word "temporary". Temporary means "will be fixed very very soon".
On 4/4/2011 2:32 PM, Artem Bityutskiy wrote: > On Mon, 2011-04-04 at 14:30 +0530, Vipin Kumar wrote: >> On 4/4/2011 11:57 AM, Artem Bityutskiy wrote: >>> On Mon, 2011-04-04 at 11:15 +0530, ext Vipin Kumar wrote: >>>> On 4/1/2011 5:14 PM, Artem Bityutskiy wrote: >>>>> On Fri, 2011-04-01 at 17:12 +0530, ext Vipin Kumar wrote: >>>>>> Temporary because the fsmc peripheral itself needs a fix for this problem >>>>> >>>>> Do you mean a hardware fix? >>>>> >>>> Ideally speaking, Yes >>> >>> Then the comment is not saying the truth I think - this solution is >>> permanent. >>> >> >> That's why it is a workaround >> I thought "workaround" says it all > > I thought we were talking about word "temporary". Temporary means "will > be fixed very very soon". > OK. May be I misunderstood it... This patch would not be needed for devices using the fixed hardware And I thought since it is the hardware which is at fault, this patch is a workaround, but yes, it may need a longer time to get fixed Regards Vipin
diff --git a/drivers/mtd/nand/fsmc_nand.c b/drivers/mtd/nand/fsmc_nand.c index 205b10b..7df270a 100644 --- a/drivers/mtd/nand/fsmc_nand.c +++ b/drivers/mtd/nand/fsmc_nand.c @@ -420,7 +420,7 @@ static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, struct fsmc_nand_data *host = container_of(mtd, struct fsmc_nand_data, mtd); struct fsmc_eccplace *ecc_place = host->ecc_place; - int i, j, s, stat, eccsize = chip->ecc.size; + int i, j, k, s, stat, eccsize = chip->ecc.size; int eccbytes = chip->ecc.bytes; int eccsteps = chip->ecc.steps; uint8_t *p = buf; @@ -460,11 +460,27 @@ static int fsmc_read_page_hwecc(struct mtd_info *mtd, struct nand_chip *chip, memcpy(&ecc_code[i], oob, 13); chip->ecc.calculate(mtd, p, &ecc_calc[i]); - stat = chip->ecc.correct(mtd, p, &ecc_code[i], &ecc_calc[i]); - if (stat < 0) - mtd->ecc_stats.failed++; - else - mtd->ecc_stats.corrected += stat; + /* + * This is a temporary erase check. A newly erased page read + * would result in an ecc error because the oob data is also + * erased to FF and the calculated ecc for an FF data is not + * FF..FF. + * This is a workaround to skip performing correction in case + * data is FF..FF + */ + for (k = 0; k < eccsize; k++) { + if (*(p + k) != 0xff) + break; + } + + if (k < eccsize) { + stat = chip->ecc.correct(mtd, p, &ecc_code[i], + &ecc_calc[i]); + if (stat < 0) + mtd->ecc_stats.failed++; + else + mtd->ecc_stats.corrected += stat; + } } return 0;