Skip to contentSkip to conversation

nand/denali: Adding Denali NAND driver support

PATCH Superseded Chin Liang See 3 participants 12 messages Jun 10, 2014 – Aug 20, 2014 1 message not in the archive

Subsystems (2)
Memory technology devices (MTD) NAND flash

v8 of 10 Superseded Latest is v10 →

lore
Share & tools
lore

Chin Liang See

author
#0 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

---
Changes for v8
- Applied Masahiro's patch as below
- Replaced "denali->foo" with "denali.foo"
- Fixed denali_write_oob() handler
- Made denali_read_oob() 10x faster
Changes for v7
- Adding Masahiro's code to support 64bit version controller
- Removed unused stub functions
- Enhanced the ECC calculation
Changes for v6
- Remove chip_delay as its unused
- Remove ECC bit assignment in nand_para functions Changes for v5
- Rename denali_nand to denali only
- Rename the macro for ctrl and data address Changes for v4
- Added cache flush to handle dcache enabled
- Used standard return where 0 for pass
- Removed unnecessary casting
- Used standard readl and writel
Changes for v3
- Fixed coding style
Changes for v2
- Enable this driver support for SOCFPGA

---
 drivers/mtd/nand/Makefile |    1 +
 drivers/mtd/nand/denali.c | 1214 +++++++++++++++++++++++++++++++++++++++++++++
 drivers/mtd/nand/denali.h |  490 ++++++++++++++++++
 3 files changed, 1705 insertions(+)
 create mode 100644 drivers/mtd/nand/denali.c
 create mode 100644 drivers/mtd/nand/denali.h
drivers/mtd/nand/Makefile +1 -0
index 02b149c..76ae105 100644
@@ -39,6 +39,7 @@ obj-$(CONFIG_NAND_ECC_BCH) += nand_bch.o
 obj-$(CONFIG_NAND_ATMEL) += atmel_nand.o
 obj-$(CONFIG_DRIVER_NAND_BFIN) += bfin_nand.o
 obj-$(CONFIG_NAND_DAVINCI) += davinci_nand.o
+obj-$(CONFIG_NAND_DENALI) += denali.o
 obj-$(CONFIG_NAND_FSL_ELBC) += fsl_elbc_nand.o
 obj-$(CONFIG_NAND_FSL_IFC) += fsl_ifc_nand.o
 obj-$(CONFIG_NAND_FSL_UPM) += fsl_upm.o
drivers/mtd/nand/denali.c new file +1214 -0

1,214 lines not on this page · raw message

drivers/mtd/nand/denali.h new file +490 -0

490 lines not on this page · raw message

Signature · 1 line
1.7.9.5
#1 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Chin,

Quoted 10 lines · On Tue, 10 Jun 2014 00:42:19 -0500 Chin Liang See <[email protected]> wrote:

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

Thanks for posting v8 when you are busy!

Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Scott,
Could you apply this patch please?

Best Regards
Masahiro Yamada

author
#2 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Quoted 20 lines · Masahiro Yamada

Hi Chin,

Quoted 10 lines · On Tue, 10 Jun 2014 00:42:19 -0500 Chin Liang See <[email protected]> wrote:

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

Thanks for posting v8 when you are busy!

Quoted 10 lines
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Scott,
Could you apply this patch please?

Thanks Masahiro

Hi Scott / Tom,

Can you help to apply this patch?
Thanks

Chin Liang

Quoted 4 lines

Best Regards
Masahiro Yamada

#3 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Chin,

Could you fix the problems Scott pointed out
and post v9?

I am really looking forward this patch on the mainstream and I believe we are almost there.

I need your help to get to our goal.

Thanks,

Masahiro Yamada

Quoted 46 lines · On Wed, 18 Jun 2014 20:42:32 -0500 Chin Liang See <[email protected]> wrote:
Quoted 20 lines · Masahiro Yamada

Hi Chin,

Quoted 10 lines · On Tue, 10 Jun 2014 00:42:19 -0500 Chin Liang See <[email protected]> wrote:

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

Thanks for posting v8 when you are busy!

Quoted 10 lines
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Scott,
Could you apply this patch please?

Thanks Masahiro

Hi Scott / Tom,

Can you help to apply this patch?
Thanks

Chin Liang

Quoted 4 lines

Best Regards
Masahiro Yamada

#4 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Chin Liang See,

Ping again.

Is your v9 patch ready?

We have made lots of effort
to review and test this driver.

I want this patch on the mainline
and I need your help for that.

Please send v9.

Thanks

Masahiro Yamada

Quoted 64 lines · On Fri, 11 Jul 2014 20:59:26 +0900 Masahiro Yamada <[email protected]>…

Hi Chin,

Could you fix the problems Scott pointed out
and post v9?

I am really looking forward this patch on the mainstream and I believe we are almost there.

I need your help to get to our goal.

Thanks,

Masahiro Yamada

Quoted 43 lines · On Wed, 18 Jun 2014 20:42:32 -0500 Chin Liang See <[email protected]> wrote:
Quoted 20 lines · Masahiro Yamada

Hi Chin,

Quoted 10 lines · On Tue, 10 Jun 2014 00:42:19 -0500 Chin Liang See <[email protected]> wrote:

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

Thanks for posting v8 when you are busy!

Quoted 10 lines
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Scott,
Could you apply this patch please?

Thanks Masahiro

Hi Scott / Tom,

Can you help to apply this patch?
Thanks

Chin Liang

Quoted 1 line
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

Best Regards
Masahiro Yamada

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

--
????????? ????LSI???
??????????
?????? ???????
????? <[email protected]>
?617-8520 ???????????1??
?? : 050-3783-5420
?? : 7-664-2813, ?????: 664-772

Best Regards
Masahiro Yamada

#5 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Scott,

Unforunately, the author of this patch, Chin Liang See is no longer responding in spite of pings several times.

I definitely need this driver.
I feel really sad to see all our efforts come to nothing.

How our situation's gonna work?

(Chin, please let me know your status if you are seeing this email.)

Best Regards
Masahiro Yamada

Quoted 107 lines · On Tue, 05 Aug 2014 17:39:07 +0900 Masahiro Yamada <[email protected]>…

Hi Chin Liang See,

Ping again.

Is your v9 patch ready?

We have made lots of effort
to review and test this driver.

I want this patch on the mainline
and I need your help for that.

Please send v9.

Thanks

Masahiro Yamada

Quoted 64 lines · On Fri, 11 Jul 2014 20:59:26 +0900 Masahiro Yamada <[email protected]>…

Hi Chin,

Could you fix the problems Scott pointed out
and post v9?

I am really looking forward this patch on the mainstream and I believe we are almost there.

I need your help to get to our goal.

Thanks,

Masahiro Yamada

Quoted 43 lines · On Wed, 18 Jun 2014 20:42:32 -0500 Chin Liang See <[email protected]> wrote:
Quoted 20 lines · Masahiro Yamada

Hi Chin,

Quoted 10 lines · On Tue, 10 Jun 2014 00:42:19 -0500 Chin Liang See <[email protected]> wrote:

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

Thanks for posting v8 when you are busy!

Quoted 10 lines
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Scott,
Could you apply this patch please?

Thanks Masahiro

Hi Scott / Tom,

Can you help to apply this patch?
Thanks

Chin Liang

Quoted 1 line
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

Best Regards
Masahiro Yamada

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

--

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

author
#6 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Masahiro,

Quoted 6 lines · Masahiro Yamada

Hi Scott,

Unforunately, the author of this patch, Chin Liang See is no longer responding in spite of pings several times.

Quoted 7 lines

I definitely need this driver.
I feel really sad to see all our efforts come to nothing.

How our situation's gonna work?

(Chin, please let me know your status if you are seeing this email.)

Sorry as I dropped this ball. Let me resolved Scott's comments and send the v9 tomorrow. Thanks

Chin Liang

Quoted 121 lines

Best Regards
Masahiro Yamada

Quoted 107 lines · On Tue, 05 Aug 2014 17:39:07 +0900 Masahiro Yamada <[email protected]>…

Hi Chin Liang See,

Ping again.

Is your v9 patch ready?

We have made lots of effort
to review and test this driver.

I want this patch on the mainline
and I need your help for that.

Please send v9.

Thanks

Masahiro Yamada

Quoted 64 lines · On Fri, 11 Jul 2014 20:59:26 +0900 Masahiro Yamada <[email protected]>…

Hi Chin,

Could you fix the problems Scott pointed out
and post v9?

I am really looking forward this patch on the mainstream and I believe we are almost there.

I need your help to get to our goal.

Thanks,

Masahiro Yamada

Quoted 43 lines · On Wed, 18 Jun 2014 20:42:32 -0500 Chin Liang See <[email protected]> wrote:
Quoted 20 lines · Masahiro Yamada

Hi Chin,

Quoted 10 lines · On Tue, 10 Jun 2014 00:42:19 -0500 Chin Liang See <[email protected]> wrote:

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>

Thanks for posting v8 when you are busy!

Quoted 10 lines
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Scott,
Could you apply this patch please?

Thanks Masahiro

Hi Scott / Tom,

Can you help to apply this patch?
Thanks

Chin Liang

Quoted 1 line
Quoted 8 lines
Quoted 3 lines
Quoted 3 lines

Best Regards
Masahiro Yamada

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

--

_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

#7 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

In reply to Chin Liang See
Quoted 3 lines · Chin Liang See

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

This is hard to parse. What exactly is required from include/configs and where is it documented?

I see that this driver exists in Linux... Is this patch related to a particular Linux SHA1?

Quoted 9 lines
Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>

Are these people on CC because they're involved in the U-Boot patch in some way, or is this just copy-and-paste from a Linux patch?

Quoted 8 lines
drivers/mtd/nand/denali.c new file +8 -0
+/* Reset the flash controller */
+static uint32_t denali_nand_reset(struct denali_nand_info *denali)
+{
+	uint32_t i;
+
+	for (i = 0; i < denali->max_banks; i++)
+		writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
+		       denali->flash_reg + INTR_STATUS(i));

drivers/mtd/nand/denali.c line 204

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+
+	for (i = 0; i < denali->max_banks; i++) {
+		writel(1 << i, denali->flash_reg + DEVICE_RESET);
+		while (!(readl(denali->flash_reg + INTR_STATUS(i)) &
+			(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
+			if (readl(denali->flash_reg + INTR_STATUS(i)) &
+				INTR_STATUS__TIME_OUT)
+				debug(KERN_DEBUG "NAND Reset operation "
+					"timed out on bank %d\n", i);
+	}

WARNING: quoted string split across lines
#283: FILE: drivers/mtd/nand/denali.c:203:
+ debug(KERN_DEBUG "NAND Reset operation "
+ "timed out on bank %d\n", i);

(likewise elsewhere)

This instance is not even from Linux -- and where does KERN_DEBUG come from? The Linux driver has never used it.

drivers/mtd/nand/denali.c line 246

Quoted 4 lines
drivers/mtd/nand/denali.c new file +4 -0
+#if ONFI_BLOOM_TIME
+	if ((en_hi * CLK_X) < (treh[mode] + 2))
+		en_hi++;
+#endif

When would this #if not be true?

Quoted 40 lines
drivers/mtd/nand/denali.c new file +40 -0
+	if ((en_lo + en_hi) * CLK_X < trc[mode])
+		en_lo += DIV_ROUND_UP((trc[mode] - (en_lo + en_hi) * CLK_X),
+				      CLK_X);
+
+	if ((en_lo + en_hi) < CLK_MULTI)
+		en_lo += CLK_MULTI - en_lo - en_hi;
+
+	while (dv_window < 8) {
+		data_invalid_rhoh = en_lo * CLK_X + trhoh[mode];
+
+		data_invalid_rloh = (en_lo + en_hi) * CLK_X + trloh[mode];
+
+		data_invalid =
+		    data_invalid_rhoh <
+		    data_invalid_rloh ? data_invalid_rhoh : data_invalid_rloh;
+
+		dv_window = data_invalid - trea[mode];
+
+		if (dv_window < 8)
+			en_lo++;
+	}
+
+	acc_clks = DIV_ROUND_UP(trea[mode], CLK_X);
+
+	while (((acc_clks * CLK_X) - trea[mode]) < 3)
+		acc_clks++;
+
+	if ((data_invalid - acc_clks * CLK_X) < 2)
+		debug(KERN_WARNING "%s, Line %d: Warning!\n",
+		      __FILE__, __LINE__);
+
+	addr_2_data = DIV_ROUND_UP(tadl[mode], CLK_X);
+	re_2_we = DIV_ROUND_UP(trhw[mode], CLK_X);
+	re_2_re = DIV_ROUND_UP(trhz[mode], CLK_X);
+	we_2_re = DIV_ROUND_UP(twhr[mode], CLK_X);
+	cs_cnt = DIV_ROUND_UP((tcs[mode] - trp[mode]), CLK_X);
+	if (!tclsrising)
+		cs_cnt = DIV_ROUND_UP(tcs[mode], CLK_X);
+	if (cs_cnt == 0)
+		cs_cnt = 1;

drivers/mtd/nand/denali.c line 297

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+
+	if (tcea[mode]) {
+		while (((cs_cnt * CLK_X) + trea[mode]) < tcea[mode])
+			cs_cnt++;
+	}
+
+#if MODE5_WORKAROUND
+	if (mode == 5)
+		acc_clks = 5;
+#endif

Likewise, this is dead code.

If these are meant to be configurable, they need to be named as config symbols, documented, and located in a config file rather than denali.h -- and ideally there should be at least one board that sets the option and at least one that does not set it, so that both possibilities get build coverage.

I see that the Linux driver has the same thing, so perhaps we can excuse it if it's not actually meant to be altered by a user, but in that case the driver ought to actually correspond to a specific SHA1 of the Linux driver.

drivers/mtd/nand/denali.c line 510

Quoted 7 lines
drivers/mtd/nand/denali.c new file +7 -0
+/* validation function to verify that the controlling software is making
+ * a valid request
+ */
+static inline bool is_flash_bank_valid(int flash_bank)
+{
+	return (flash_bank >= 0 && flash_bank < 4);
+}

ERROR: return is not a function, parentheses are not required #589: FILE: drivers/mtd/nand/denali.c:509:
+ return (flash_bank >= 0 && flash_bank < 4);

drivers/mtd/nand/denali.c line 739

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+/* setups the HW to perform the data DMA */
+static void denali_setup_dma(struct denali_nand_info *denali, int op)
+{
+	uint32_t mode;
+	const int page_count = 1;
+	uint32_t addr = (uint32_t)denali->buf.dma_buf;
+
+	flush_dcache_range(addr, addr + sizeof(denali->buf.dma_buf));
+
+#ifdef CONFIG_NAND_DENALI_64BIT

Needs to be documented -- and depending on what it means, should possibly be CONFIG_SYS_NAND_DENALI_64BIT. The Linux driver seems to only have the "else" variant.

drivers/mtd/nand/denali.c line 815

Quoted 12 lines
drivers/mtd/nand/denali.c new file +12 -0
+static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			const uint8_t *buf, bool raw_xfer, int oob_required)
+{
+	struct denali_nand_info *denali = mtd_to_denali(mtd);
+
+	uint32_t irq_status = 0;
+	uint32_t irq_mask = INTR_STATUS__DMA_CMD_COMP;
+
+	denali->status = 0;
+
+	/* copy buffer into DMA buffer */
+	memcpy((void *)denali->buf.dma_buf, buf, mtd->writesize);

Why do you have this cast?

drivers/mtd/nand/denali.c line 817

Quoted 1 line
drivers/mtd/nand/denali.c new file +1 -0
+	/* need extra memcpoy for raw transfer */

memcpoy?

drivers/mtd/nand/denali.c line 964

Quoted 5 lines
drivers/mtd/nand/denali.c new file +5 -0
+	/* check whether any ECC error */
+	if (irq_status & INTR_STATUS__ECC_UNCOR_ERR) {
+		/* is the ECC cause by erase page, check using read_page_raw */
+		debug("  Uncorrected ECC detected\n");
+		denali_read_page_raw(mtd, chip, buf, oob_required, denali->page);

WARNING: line over 80 characters
#1044: FILE: drivers/mtd/nand/denali.c:964:
+ denali_read_page_raw(mtd, chip, buf, oob_required, denali->page);

-Scott

#8 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Chin, Scott,

Quoted 2 lines · On Thu, 19 Jun 2014 20:39:20 -0500 Scott Wood <[email protected]> wrote:

I see that this driver exists in Linux... Is this patch related to a particular Linux SHA1?

If so, should it be described in git-log or the header part of denali.c ?

Quoted 24 lines
Quoted 8 lines
drivers/mtd/nand/denali.c new file +8 -0
+/* Reset the flash controller */
+static uint32_t denali_nand_reset(struct denali_nand_info *denali)
+{
+	uint32_t i;
+
+	for (i = 0; i < denali->max_banks; i++)
+		writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
+		       denali->flash_reg + INTR_STATUS(i));

drivers/mtd/nand/denali.c line 204

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+
+	for (i = 0; i < denali->max_banks; i++) {
+		writel(1 << i, denali->flash_reg + DEVICE_RESET);
+		while (!(readl(denali->flash_reg + INTR_STATUS(i)) &
+			(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
+			if (readl(denali->flash_reg + INTR_STATUS(i)) &
+				INTR_STATUS__TIME_OUT)
+				debug(KERN_DEBUG "NAND Reset operation "
+					"timed out on bank %d\n", i);
+	}

WARNING: quoted string split across lines
#283: FILE: drivers/mtd/nand/denali.c:203:

drivers/mtd/nand/denali.c new file +2 -0
+                               debug(KERN_DEBUG "NAND Reset operation "
+                                       "timed out on bank %d\n", i);
Quoted 4 lines

(likewise elsewhere)

This instance is not even from Linux -- and where does KERN_DEBUG come from? The Linux driver has never used it.

Looks like this part is like follows in the current Kernel:

                        dev_dbg(denali->dev,
                        "NAND Reset operation timed out on bank %d\n", i);

And it was as follows, prior to commit 7cfffac06ca0:

                        nand_dbg_print(NAND_DBG_WARN,
                        "NAND Reset operation timed out on bank %d\n", i);

I think we can use dev_dbg(...) in U-Boot too. ( <linux/compat.h> should be included in that case.)

If there is no reason to change it, let's stick to dev_dbg().

Quoted 11 lines

If these are meant to be configurable, they need to be named as config symbols, documented, and located in a config file rather than denali.h -- and ideally there should be at least one board that sets the option and at least one that does not set it, so that both possibilities get build coverage.

I see that the Linux driver has the same thing, so perhaps we can excuse it if it's not actually meant to be altered by a user, but in that case the driver ought to actually correspond to a specific SHA1 of the Linux driver.

Agreed.
It would make is easier to sync this file with the Kernel for future updates.

Quoted 6 lines
Quoted 6 lines
drivers/mtd/nand/denali.c new file +6 -0
+/* setups the HW to perform the data DMA */
+static void denali_setup_dma(struct denali_nand_info *denali, int op)
+{
+	uint32_t mode;
+	const int page_count = 1;
+	uint32_t addr = (uint32_t)denali->buf.dma_buf;
Quoted 8 lines

drivers/mtd/nand/denali.c line 739

Quoted 4 lines
drivers/mtd/nand/denali.c new file +4 -0
+
+	flush_dcache_range(addr, addr + sizeof(denali->buf.dma_buf));
+
+#ifdef CONFIG_NAND_DENALI_64BIT

Needs to be documented -- and depending on what it means, should possibly be CONFIG_SYS_NAND_DENALI_64BIT. The Linux driver seems to only have the "else" variant.

DMA setup sequence is completely different between 32bit bus IP core and 64bit bus IP core. That's why this #ifdef conditional is necessary.

CONFIG_NAND_DENALI_64BIT should be defined for 64bit bus IP core.

I will post Panasonic SoCs support series right after this patch is applied, enabling CONFIG_SYS_NAND_DENALI_64BIT.

I requested Chin to add this macro.
I can document it in a follow-up patch.
(Or if he could do that for me in v9, it would be really appriciated.)

Linux Kernel does not support 64bit bus Denali core yet, but I have a plan to post a patch to support it lator.

Best Regards
Masahiro Yamada

#9 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Quoted 10 lines · Masahiro Yamada

Hi Chin, Scott,

Quoted 2 lines · On Thu, 19 Jun 2014 20:39:20 -0500 Scott Wood <[email protected]> wrote:

I see that this driver exists in Linux... Is this patch related to a particular Linux SHA1?

If so, should it be described in git-log or the header part of denali.c ?

In the log.

-Scott

A message that is not in the archive (sent off-list or not yet received): 0016CF5815A1B142902817051AF62EB30E91F2@PG-ITEXCH01.altera.priv.altera.com

author
#11 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Quoted 11 lines · Chin Liang See

From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Friday, June 20, 2014 9:39 AM
To: Chin Liang See
Cc: ZY - u-boot
Subject: Re: [U-Boot,v8] nand/denali: Adding Denali NAND driver support

Quoted 3 lines · Chin Liang See

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

Quoted 6 lines

This is hard to parse. What exactly is required from include/configs and where is it documented?

I see that this driver exists in Linux... Is this patch related to a particular Linux SHA1?

Yup, this driver is leveraged from Linux. I will update the commit message to mention about this.

Quoted 10 lines
Quoted 9 lines
Signed-off-by Chin Liang See <[email protected]>
Cc Artem Bityutskiy <[email protected]>
Cc David Woodhouse <[email protected]>
Cc Brian Norris <[email protected]>
Cc Scott Wood <[email protected]>
Cc Masahiro Yamada <[email protected]>
Signed-off-by Masahiro Yamada <[email protected]>
Reviewed-by Masahiro Yamada <[email protected]>
Tested-by Masahiro Yamada <[email protected]>
Quoted 2 lines

Are these people on CC because they're involved in the U-Boot patch in some way, or is this just copy-and-paste from a Linux patch?

Actually they are part of reviewer for Linux Denali NAND driver. As they didn't response on this patch, I will remove them.

Quoted 25 lines
Quoted 9 lines
drivers/mtd/nand/denali.c new file +8 -0
+/* Reset the flash controller */
+static uint32_t denali_nand_reset(struct denali_nand_info *denali)
+{
+	uint32_t i;
+
+	for (i = 0; i < denali->max_banks; i++)
+		writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
+		       denali->flash_reg + INTR_STATUS(i));

drivers/mtd/nand/denali.c line 204

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+
+	for (i = 0; i < denali->max_banks; i++) {
+		writel(1 << i, denali->flash_reg + DEVICE_RESET);
+		while (!(readl(denali->flash_reg + INTR_STATUS(i)) &
+			(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
+			if (readl(denali->flash_reg + INTR_STATUS(i)) &
+				INTR_STATUS__TIME_OUT)
+				debug(KERN_DEBUG "NAND Reset operation "
+					"timed out on bank %d\n", i);
+	}

WARNING: quoted string split across lines
#283: FILE: drivers/mtd/nand/denali.c:203:

drivers/mtd/nand/denali.c new file +2 -0
+                               debug(KERN_DEBUG "NAND Reset operation "
+                                       "timed out on bank %d\n", i);
Quoted 4 lines

(likewise elsewhere)

This instance is not even from Linux -- and where does KERN_DEBUG come from? The Linux driver has never used it.

Its defined as empty. I will remove them.

Quoted 7 lines

drivers/mtd/nand/denali.c line 246

Quoted 5 lines
drivers/mtd/nand/denali.c new file +4 -0
+#if ONFI_BLOOM_TIME
+	if ((en_hi * CLK_X) < (treh[mode] + 2))
+		en_hi++;
+#endif

When would this #if not be true?

I suspect this code is added due to some issue the original developer hit during high speed data transfer. Nevertheless, its default in Linux too. With that, I will remove the macro.

Quoted 60 lines
Quoted 41 lines
drivers/mtd/nand/denali.c new file +40 -0
+	if ((en_lo + en_hi) * CLK_X < trc[mode])
+		en_lo += DIV_ROUND_UP((trc[mode] - (en_lo + en_hi) * CLK_X),
+				      CLK_X);
+
+	if ((en_lo + en_hi) < CLK_MULTI)
+		en_lo += CLK_MULTI - en_lo - en_hi;
+
+	while (dv_window < 8) {
+		data_invalid_rhoh = en_lo * CLK_X + trhoh[mode];
+
+		data_invalid_rloh = (en_lo + en_hi) * CLK_X + trloh[mode];
+
+		data_invalid =
+		    data_invalid_rhoh <
+		    data_invalid_rloh ? data_invalid_rhoh : data_invalid_rloh;
+
+		dv_window = data_invalid - trea[mode];
+
+		if (dv_window < 8)
+			en_lo++;
+	}
+
+	acc_clks = DIV_ROUND_UP(trea[mode], CLK_X);
+
+	while (((acc_clks * CLK_X) - trea[mode]) < 3)
+		acc_clks++;
+
+	if ((data_invalid - acc_clks * CLK_X) < 2)
+		debug(KERN_WARNING "%s, Line %d: Warning!\n",
+		      __FILE__, __LINE__);
+
+	addr_2_data = DIV_ROUND_UP(tadl[mode], CLK_X);
+	re_2_we = DIV_ROUND_UP(trhw[mode], CLK_X);
+	re_2_re = DIV_ROUND_UP(trhz[mode], CLK_X);
+	we_2_re = DIV_ROUND_UP(twhr[mode], CLK_X);
+	cs_cnt = DIV_ROUND_UP((tcs[mode] - trp[mode]), CLK_X);
+	if (!tclsrising)
+		cs_cnt = DIV_ROUND_UP(tcs[mode], CLK_X);
+	if (cs_cnt == 0)
+		cs_cnt = 1;

drivers/mtd/nand/denali.c line 297

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+
+	if (tcea[mode]) {
+		while (((cs_cnt * CLK_X) + trea[mode]) < tcea[mode])
+			cs_cnt++;
+	}
+
+#if MODE5_WORKAROUND
+	if (mode == 5)
+		acc_clks = 5;
+#endif

Likewise, this is dead code.

If these are meant to be configurable, they need to be named as config symbols, documented, and located in a config file rather than denali.h -- and ideally there should be at least one board that sets the option and at least one that does not set it, so that both possibilities get build coverage.

Quoted 4 lines

I see that the Linux driver has the same thing, so perhaps we can excuse it if it's not actually meant to be altered by a user, but in that case the driver ought to actually correspond to a specific SHA1 of the Linux driver.

It seems these debug code had upstreamed into Linux git. I will remove them. I also ensured all these similar macro are removed too.

Quoted 12 lines

drivers/mtd/nand/denali.c line 510

Quoted 8 lines
drivers/mtd/nand/denali.c new file +7 -0
+/* validation function to verify that the controlling software is making
+ * a valid request
+ */
+static inline bool is_flash_bank_valid(int flash_bank)
+{
+	return (flash_bank >= 0 && flash_bank < 4);
+}

ERROR: return is not a function, parentheses are not required #589: FILE: drivers/mtd/nand/denali.c:509:
+ return (flash_bank >= 0 && flash_bank < 4);

Removed

Quoted 15 lines

drivers/mtd/nand/denali.c line 739

Quoted 11 lines
drivers/mtd/nand/denali.c new file +10 -0
+/* setups the HW to perform the data DMA */
+static void denali_setup_dma(struct denali_nand_info *denali, int op)
+{
+	uint32_t mode;
+	const int page_count = 1;
+	uint32_t addr = (uint32_t)denali->buf.dma_buf;
+
+	flush_dcache_range(addr, addr + sizeof(denali->buf.dma_buf));
+
+#ifdef CONFIG_NAND_DENALI_64BIT

Needs to be documented -- and depending on what it means, should possibly be CONFIG_SYS_NAND_DENALI_64BIT. The Linux driver seems to only have the "else" variant.

Yup, I added SYS to ensure standard macro definition. 64 bit variant is the one used by Panasonic. I will add more explanation about 32 bit and 64 bit variant in comments.

Quoted 15 lines

drivers/mtd/nand/denali.c line 815

Quoted 13 lines
drivers/mtd/nand/denali.c new file +12 -0
+static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
+			const uint8_t *buf, bool raw_xfer, int oob_required)
+{
+	struct denali_nand_info *denali = mtd_to_denali(mtd);
+
+	uint32_t irq_status = 0;
+	uint32_t irq_mask = INTR_STATUS__DMA_CMD_COMP;
+
+	denali->status = 0;
+
+	/* copy buffer into DMA buffer */
+	memcpy((void *)denali->buf.dma_buf, buf, mtd->writesize);

Why do you have this cast?

Removed

Quoted 4 lines

drivers/mtd/nand/denali.c line 817

Quoted 2 lines
drivers/mtd/nand/denali.c new file +1 -0
+	/* need extra memcpoy for raw transfer */

memcpoy?

Fixed

Quoted 11 lines

drivers/mtd/nand/denali.c line 964

Quoted 6 lines
drivers/mtd/nand/denali.c new file +5 -0
+	/* check whether any ECC error */
+	if (irq_status & INTR_STATUS__ECC_UNCOR_ERR) {
+		/* is the ECC cause by erase page, check using read_page_raw */
+		debug("  Uncorrected ECC detected\n");
+		denali_read_page_raw(mtd, chip, buf, oob_required, denali->page);

WARNING: line over 80 characters
#1044: FILE: drivers/mtd/nand/denali.c:964:
+ denali_read_page_raw(mtd, chip, buf, oob_required, denali->page);

Fixed

Thanks
Chin Liang

Quoted 2 lines

-Scott

#12 lore raw

PATCH v8 1/1 nand/denali: Adding Denali NAND driver support

Hi Chin,

Quoted 15 lines · On Tue, 19 Aug 2014 04:18:44 -0500 Chin Liang See <[email protected]> wrote:
Quoted 14 lines · Chin Liang See

From: Scott Wood [mailto:scottwood at freescale.com]
Sent: Friday, June 20, 2014 9:39 AM
To: Chin Liang See
Cc: ZY - u-boot
Subject: Re: [U-Boot,v8] nand/denali: Adding Denali NAND driver support

Quoted 3 lines · Chin Liang See

To add the Denali NAND driver support into U-Boot. It required information such as register base address from configuration header file within include/configs folder.

This is hard to parse. What exactly is required from include/configs and where is it documented?

Quoted 7 lines
Quoted 3 lines

I see that this driver exists in Linux... Is this patch related to a particular Linux SHA1?

Yup, this driver is leveraged from Linux. I will update the commit message to mention about this.

I know this driver is leveraged from Linux.
Is it difficult to describe the particular SHA1 or tag you took it from?

Quoted 26 lines
Quoted 26 lines
Quoted 10 lines
drivers/mtd/nand/denali.c new file +8 -0
+/* Reset the flash controller */
+static uint32_t denali_nand_reset(struct denali_nand_info *denali)
+{
+	uint32_t i;
+
+	for (i = 0; i < denali->max_banks; i++)
+		writel(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT,
+		       denali->flash_reg + INTR_STATUS(i));

drivers/mtd/nand/denali.c line 204

Quoted 10 lines
drivers/mtd/nand/denali.c new file +10 -0
+
+	for (i = 0; i < denali->max_banks; i++) {
+		writel(1 << i, denali->flash_reg + DEVICE_RESET);
+		while (!(readl(denali->flash_reg + INTR_STATUS(i)) &
+			(INTR_STATUS__RST_COMP | INTR_STATUS__TIME_OUT)))
+			if (readl(denali->flash_reg + INTR_STATUS(i)) &
+				INTR_STATUS__TIME_OUT)
+				debug(KERN_DEBUG "NAND Reset operation "
+					"timed out on bank %d\n", i);
+	}

WARNING: quoted string split across lines
#283: FILE: drivers/mtd/nand/denali.c:203:

drivers/mtd/nand/denali.c new file +2 -0
+                               debug(KERN_DEBUG "NAND Reset operation "
+                                       "timed out on bank %d\n", i);
Quoted 6 lines
Quoted 4 lines

(likewise elsewhere)

This instance is not even from Linux -- and where does KERN_DEBUG come from? The Linux driver has never used it.

Its defined as empty. I will remove them.

I pointed out that you could still use dev_err() and dev_warn as Linux and I don't see the necessity to diverge from Linux here.

Anyway your way work enough...

Best Regards
Masahiro Yamada