Re: [Patch v3 2/5] mtd: nand: add NVIDIA Tegra NAND Flash controller driver

Hi Brian,

a bit of investigation later I have some coherent answers to your
requests. :)

Am Mittwoch, den 22.07.2015, 16:10 -0700 schrieb Brian Norris:

Hi,

On Wed, Jul 22, 2015 at 10:42:40PM +0200, Lucas Stach wrote:

quoted

Am Dienstag, den 21.07.2015, 14:27 -0700 schrieb Brian Norris:

quoted

On Sun, May 10, 2015 at 08:29:59PM +0200, Lucas Stach wrote:

quoted

--- /dev/null
+++ b/drivers/mtd/nand/tegra_nand.c

@@ -0,0 +1,801 @@

...

quoted

quoted

quoted

+static int tegra_nand_read_page(struct mtd_info *mtd, struct nand_chip *chip,
+				uint8_t *buf, int oob_required, int page)
+{

...

quoted

quoted

quoted

+	value = readl(nand->regs + DEC_STATUS);
+
+	if (value & DEC_STATUS_A_ECC_FAIL) {
+		/*
+		 * The ECC isn't smart enough to figure out if a page is
+		 * completely erased and flags an error in this case. So we
+		 * check the read data here to figure out if it's a legitimate
+		 * error or a false positive.
+		 */
+		int i;
+		u32 *data = (u32 *)buf;
+		for (i = 0; i < mtd->writesize / 4; i++) {
+			if (data[i] != 0xffffffff) {
+				mtd->ecc_stats.failed++;
+				return -EBADMSG;
+			}
+		}

Hmm, what about OOB? It's possible to actually write 0xff to the entire
page. This hunk means that such a data pattern would then be
unprotected. You should probably check that *both* the main and OOB data
are all 0xff; if there are non-0xff bytes, then that (probably, see the
following) means someone intentionally wrote all 0xff to the page. [1]

This check is only executed if the ECC engine flagged a non-correctable
error. If someone wrote all 0xff to the page there will be a proper ECC
checksum calculated and we won't get into this path. So to get this
check to wrongly paper over a legitimate error someone would need to
write almost all 0xff with a few bits 0, which then flip to a 1
afterwards, which is highly unlikely as far as I understand flash
technology.

On second thought, the case I mentioned is not a problem for you
currently. You'll just have more problems once you start seeing bitflips
in erased pages.

And I agree, the latter case you mention is probably pretty unlikely,
though I'm not really an EE expert to tell you cannot see such a 0->1
"stuck bit".

But anyway, why don't you check the spare area too? It turns the
"unlikely" problem into a non-issue.

If I'm using the HWECC/DMA functions I have no way to check the OOB ECC
area. The controller just skips over it and only transfers user
available OOB data into the buffer. So checking the OOB is unfortunately
not easily done.

At the same time, I see that you don't support raw page reads/writes. Is
it impossible? Or is it just an oversight?

It's possible, but I've skipped it for now, as this would increase the
amount of testing I have to do to validate each round of those patches
even more and I don't see much benefit right now. So this remains a
TODO.

[...]

quoted

quoted

quoted

+	unsigned long rate = clk_get_rate(nand->clk) / 1000000;

^^ conversion to MHz? Why? (comment) How about DIV_ROUND_UP, so you
don't overestimate the clock period (and thus underestimate the number
of clock periods needed)?

quoted

quoted

quoted

+	unsigned long period = 1000000 / rate;

And period is... in picoseconds? It took me a minute to figure that out
for sure, so IMO it deserves a comment.

quoted

quoted

quoted

+	const struct nand_sdr_timings *timings;
+	u32 val, reg = 0;
+
+	timings = onfi_async_timing_mode_to_sdr_timings(mode);
+
+	val = max3(timings->tAR_min, timings->tRR_min,
+		   timings->tRC_min) / period;

DIV_ROUND_UP()? You don't want to lop off a partial timing cycle, right?
Simliar comments apply throughout. Or please correct me if I'm wrong.

I agree with the reasoning here and made those changes for v4.

quoted

quoted

quoted

+	if (val > 2)
+		val -= 3;
+	reg |= TIMING_TCR_TAR_TRR(val);
+
+	val = max(max(timings->tCS_min, timings->tCH_min),
+		  max(timings->tALS_min, timings->tALH_min)) / period;
+	if (val > 1)
+		val -= 2;
+	reg |= TIMING_TCS(val);
+
+	val = (max(timings->tRP_min, timings->tREA_max) + 6000) / period;
+	reg |= (TIMING_TRP(val) | TIMING_TRP_RESP(val));
+
+	reg |= TIMING_TWB(timings->tWB_max / period);
+	reg |= TIMING_TWHR(timings->tWHR_min / period);
+	reg |= TIMING_TWH(timings->tWH_min / period);
+	reg |= TIMING_TWP(timings->tWP_min / period);
+	reg |= TIMING_TRH(timings->tRHW_min / period);
+
+	writel(reg, nand->regs + TIMING_1);
+
+	val = timings->tADL_min / period;
+	if (val > 2)
+		val -= 3;
+	reg = TIMING_TADL(val);
+
+	writel(reg, nand->regs + TIMING_2);
+}

Regards,
Lucas

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html