On Thu, 03 Jul 2014, Gupta, Pekon wrote:

quoted

From: Brian Norris [mailto:computersforpeace-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org]
On Wed, May 28, 2014 at 10:20:05AM +0100, Lee Jones wrote:

[...]

quoted

quoted

+/*
+ * NANDi Interrupts (shared by Hamming and BCH controllers)
+ */
+static irqreturn_t nandi_irq_handler(int irq, void *dev)
+{
+	struct nandi_controller *nandi = dev;
+	unsigned int status;
+
+	status = readl(nandi->base + NANDBCH_INT_STA);
+
+	if (status & NANDBCH_INT_SEQNODESOVER) {
+		/* BCH */
+		writel(NANDBCH_INT_CLR_SEQNODESOVER,
+		       nandi->base + NANDBCH_INT_CLR);
+		complete(&nandi->seq_completed);
+	}
+	if (status & NAND_INT_RBN) {
+		/* Hamming */
+		writel(NAND_INT_CLR_RBN, nandi->base + NANDHAM_INT_CLR);
+		complete(&nandi->rbn_completed);
+	}
+
+	return IRQ_HANDLED;

Did you miss following comments ?
[RFC 05/47] mtd: nand: stm_nand_bch: IRQ support for ST's BCH NAND Controller driver
http://lists.infradead.org/pipermail/linux-mtd/2014-March/052916.html

Shouldn't IRQ_NONE be returned if no valid irq_status was found ?

I don't recall seeing these comments, so yes, I think they were
missed.  Will fix.

quoted

quoted

+/*
+ * BCH Operations
+ */
+static inline void bch_load_prog_cpu(struct nandi_controller *nandi,
+				     struct bch_prog *prog)
+{
+	uint32_t *src = (uint32_t *)prog;
+	uint32_t *dst = (uint32_t *)(nandi->base + NANDBCH_ADDRESS_REG_1);
+	int i;
+
+	for (i = 0; i < 16; i++) {
+		/* Skip registers marked as "reserved" */
+		if (i != 11 && i != 14)

Using macros for 11, 14, and 16 would make it more readable.

I think with the comment there, it's perfectly clear what's happening.

quoted

quoted

+			writel(*src, dst);
+		dst++;
+		src++;
+	}
+}
+
+static void bch_wait_seq(struct nandi_controller *nandi)
+{
+	int ret;
+
+	ret = wait_for_completion_timeout(&nandi->seq_completed, HZ/2);

Are you sure you want to use same timeout value for all operations
like ERASE, READ, WRITE ? because
(1) There is wide variance between:
- BLOCK_ERASE:  max(tBER) = 10ms) for MT29F4G08 Micron NAND
- PAGE_ERASE: max(tPROG) = 600usec for same Micron part.

(2) The value of HZ varies across kernel versions and hardware platforms.

I suggest you pass the timeout value in call to bch_wait_seq().
And this timeout value should be like 2x of typical value of which you found
during ONFI parsing, or from DT

How do you obtain these timings?  I don't see tBER or tPROG being used
anywhere in MTD.

[...]

quoted

quoted

+{
+	uint8_t *b = data;
+	int zeros = 0;
+	int i;
+
+	for (i = 0; i < page_size; i++) {
+		zeros += hweight8(~*b++);

Are you sure you want to use hweight8 ?
hweight32 or something should give better performance, plz check.
because this piece of code (check_xx_bitflips) sometimes becomes
bottle neck for READ path.

I'm not sure, no.  If I change it, how will I know if it's still doing
the correct thing or otherwise?

[...]

quoted

quoted

+	/* Load last page of block */
+	offs = (loff_t)block << chip->phys_erase_shift;
+	offs += mtd->erasesize - mtd->writesize;
+	if (bch_read_page(nandi, offs, buf) < 0) {

*Important*: You shouldn't call internal functions directly here..
Instead use chip->_read() OR mtd-read() that will:
- keep this code independent of ECC modes added in your driver in future.
- implicitly handle updating mtd->ecc_stat (like ecc_failed, bits_corrected).
- implicitly take care of address alignments checks and offset calculations.

<Same applies to other places where you have directly used bch_read_page())

Yourself and Brian seem to disagree on this point.  Which is correct?

quoted

quoted

+	/* Is block already considered bad? (will also catch invalid offsets) */
+	ret = mtd_block_isbad(mtd, offs);

You're violating some of the layering here; the low-level driver
probably shouldn't be calling the high-level mtd_*() APIs. On a similar
note, I'm not 100% confident that the nand_base/nand_bbt separation is
written cleanly enough for easy maintenance of your nand_base + ST BBT
code in parallel. I might need a second look at that, and I think
modularizing your BBT code to a separate file could help ease this a
little.

... here is the converse argument.

[...]

quoted

Are you actually looking for driver data, not platform data? (e.g.,
dev_set_drvdata() / platform_get_drvdata()) The two are a little
different, but your usage sounds like this more of a driver-private
description.

On closer inspection it appears that drvdata was already in use.  I've
not encompassed the NANDi information and the old pdata into one ddata
container.

[...]

Sorry, this review got delayed from my part..
But I hope I have covered most of the review in this and earlier responses.
if you are able to clean most of these then should be ready for 3.17+.

It's a bit late for v3.17 now, but let's see what we can do about
getting it in for v3.18.  Fingers crossed!

Please re-send the next version in similar squashed format, as its easy for review.
And once Brian is okay, may be then you can re-send individual patches.

Right.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
--
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