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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
