Re: [U-Boot] [PATCH] V2 NAND_SPL support for phycore imx31to Scott Wood
In message on Friday 12 December 2008 04:55:49 Scott Wood wrote:
On Fri, Dec 05, 2008 at 02:13:51PM +0300, Maxim Artamonov wrote:
+#ifdef CONFIG_NAND_SPL +/* somewhat macro to reduce bin size for CONFIG_NAND_SPL*/ +.macro FILLREGS begreg, val, count, step
Why not use this macro always?
Why not? But, I think this 'll lose clearity of code. Let's discuss this.
@@ -32,6 +35,15 @@ #include <version.h> .globl _start _start: b reset +#ifdef CONFIG_NAND_SPL
- nop
- nop
- nop
- nop
- nop
- nop
- nop
+#else #ifdef CONFIG_ONENAND_IPL
#elif
Do NAND and OneNAND really have different requirements here?
@@ -156,9 +169,9 @@ relocate: /* relocate U-Boot to RAM */ adr r0, _start /* r0 <- current position of code */ ldr r1, _TEXT_BASE /* test if we run from flash or RAM */ cmp r0, r1 /* don't reloc during debug */ -#ifndef CONFIG_ONENAND_IPL +#if !defined(CONFIG_ONENAND_IPL) && !defined(CONFIG_NAND_SPL)
Should define one symbol that is set if either of the above are set, to simplify the ifdefs.
I don't work with OneNAND flash and reference to "hang" is deeply in source tree, and we have only 2kB on NAND in-place of 4kB on OneNand. Unhappily, I don't have anytime to learn about OneNand and to change its code.
+#define NFC_BUF_SIZE (*((volatile u16 *)(NFC_BASE_ADDR + 0xE00))) +#define NFC_BUF_ADDR (*((volatile u16 *)(NFC_BASE_ADDR + 0xE04))) ... +#define NFC_CONFIG1 (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1A))) +#define NFC_CONFIG2 (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1C)))
Use I/O accessors,
What you about, tell me please?
and move the NFC register layout to a non-arch-specific header file (preferably as a struct), as it's shared with other chips.
Which chips? This is a system's integration task, but I wrote only for imx-31 phycore. I don't understand in this.
#define CONFIG_EXTRA_ENV_SETTINGS \
- "bootargs_base=setenv bootargs console=ttySMX0,115200\0" \
- "bootargs_nfs=setenv bootargs $(bootargs) root=/dev/nfs ip=dhcp nfsroot=$(serverip):$(nfsrootfs),v3,tcp\0" \
- "bootargs_flash=setenv bootargs $(bootargs) root=/dev/mtdblock2 rootfstype=jffs2" \
- "bootargs_mtd=setenv bootargs $(bootargs) $(mtdparts)" \
- "jffs2=root-pcm037.jffs2\0" \
- "uimage=uImage-pcm037\0" \
- "nfsrootfs=/nfsroot\0" \ "bootcmd=run bootcmd_net\0" \
- "mtdids=nor0=phys_mapped_flash\0" \
- "bootargs_base=setenv bootargs console=ttymxc0,115200\0" \
- "bootargs_nfs=setenv bootargs $(bootargs) root=/dev/nfs ip=dhcp nfsroot=$(serverip):$(nfsrootfs),v3,tcp\0" \
- "bootargs_mtd=setenv bootargs $(bootargs) $(mtdparts)\0" \
- "bootargs_flash=setenv bootargs $(bootargs) root=/dev/mtdblock3 rootfstype=jffs2\0" \
There are changes in here that don't seem to be related to nand boot.
Yes, it's so. I'll change code.
+static void mx31_wait_ready(void) +{
- while (1)
if (NFC_CONFIG2 & NFC_INT){NFC_CONFIG2 = NFC_CONFIG2 & (~NFC_INT);/*int flag reset*/break;}+}
There's an extra indentation level in the if-body.
Braces around the while-body. Space before and after braces (here and elsewhere).
+static void mx31_nand_init(void) +{
- /*unlocking RAM Buff*/
- NFC_CONFIG = 0x2;
- /*hardware ECC checking and correct*/
- NFC_CONFIG1 = NFC_ECC_EN;
Space after /*, before */
It will apply.
- if (mx31_nand_check_ecc ())
while (1){break;}; /*!!!!!*/What is this?
Ok, let's it be infinitive while-cycle.
- p1 = (u32 *)MAIN_AREA0;
- p2 = (u32 *)buf;
- /*main copy loop from NAND-buffer to SDRAM memory*/
- for (i=0; i < (CFG_NAND_PAGE_SIZE / 4); i++){
*p2 = *p1;p1++;p2++;- }
- p1 = (u32 *)SPARE_AREA0;
- /*it is hardware specific code for 8-bit 512kB NAND-flash spare area*/
- p1++;
- a = *p1;
- a = (a & 0x0000ff00) >> 8;
- if (a != 0xff) /*bad block marker verify*/
return 1;/*potential bad block*/- return 0;
+}
+static int nand_load(unsigned int from, unsigned int size, unsigned char *buf) +{
- int i, bb;
- mx31_nand_init ();
- /*conver from to page number*/
- from = from / CFG_NAND_PAGE_SIZE;
- i = 0;
- while (i < (size/CFG_NAND_PAGE_SIZE)){
if ((from*CFG_NAND_PAGE_SIZE) >= CFG_NAND_CHIP_SIZE)return 2;/*memory segment violation*/bb = mx31_read_page (from, buf);/*checking first page of each block*//*if this page has bb marker, then skip whole block*/if ((!(from % CFG_NAND_PAGE_COUNT)) && bb){from = from + CFG_NAND_PAGE_COUNT;CONFIG_NAND_PAGES_PER_BLOCK would probably be a better name.
All right!
with regards, Maxim
On Wed, Jan 28, 2009 at 07:43:54AM +0300, Maxim Artamonov wrote:
In message on Friday 12 December 2008 04:55:49 Scott Wood wrote:
On Fri, Dec 05, 2008 at 02:13:51PM +0300, Maxim Artamonov wrote:
+#ifdef CONFIG_NAND_SPL +/* somewhat macro to reduce bin size for CONFIG_NAND_SPL*/ +.macro FILLREGS begreg, val, count, step
Why not use this macro always?
Why not? But, I think this 'll lose clearity of code. Let's discuss this.
You want unnecessary ifdefs to *increase* clarity?
+#define NFC_BUF_SIZE (*((volatile u16 *)(NFC_BASE_ADDR + 0xE00))) +#define NFC_BUF_ADDR (*((volatile u16 *)(NFC_BASE_ADDR + 0xE04))) ... +#define NFC_CONFIG1 (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1A))) +#define NFC_CONFIG2 (*((volatile u16 *)(NFC_BASE_ADDR + 0xE1C)))
Use I/O accessors,
What you about, tell me please?
Use readw()/writew().
and move the NFC register layout to a non-arch-specific header file (preferably as a struct), as it's shared with other chips.
Which chips?
There was a separate driver for the same NAND controller on 5xxx PowerPC posted.
This is a system's integration task, but I wrote only for imx-31 phycore.
That's what I'm complaining about. Please do not have tunnel vision such that you only see your own chip.
- if (mx31_nand_check_ecc ())
while (1){break;}; /*!!!!!*/What is this?
Ok, let's it be infinitive while-cycle.
Except it isn't, due to the break.
In any case, when there's something unusual enough to produce a comment of /*!!!!!*/, it would be much more helpful to replace that comment with *words* that describe what's going through your head. :-)
-Scott
participants (2)
-
Maxim Artamonov -
Scott Wood