Hi Masahiro, On Fri, 2014-02-28 at 21:57 +0900, Masahiro Yamada wrote: > Hello Chin, > > > > > > > Where do you set nand->ecc.strength? > > > > > > > > I believe this is only applicable for NAND_ECC_HW_SYNDROME mode. We are > > > > using the NAND_ECC_HW (without the syndrome). Wonder you hit error > > > > during run? > > > > > > No, it must always be set for hardware ECC. Note the lack of a break; > > > before case NAND_ECC_HW_SYNDROME. > > > > Good catch, thanks Scott! > > ecc.strengh must be set for NAND_ECC_HW. > > Otherwise, error message will be displayed and reboot. > > NAND: NAND: Denali NAND controller > BUG: failure at drivers/mtd/nand/nand_base.c:3224/nand_scan_tail()! > BUG! > resetting ... > > You said this driver was tested against 3 different devices. > > In that case, I can't understand how your board passed > through nand_scan_tail(). > > Anyway, I modifed denali_nand_init() locally > to set nand->ecc.strength. >
Actually this first revision of this patch was last year. It was tested again old code base too that time. As I don't have the board with me now, might need your help for new changes testing. I believe we can work together to upstream this driver. > > > > Hi Masahiro, > > > > I rechecked my documentation and the value is 8. > > The data sector size is 512 bytes while ECC sector size is 14 bytes. > > With that, the controller able to auto correct up to 8 bits. > > This is how a page will look like > > > > 512 bytes data | 14 bytes ECC | 512 bytes data | 14 bytes ECC | 512 > > bytes data | 14 bytes ECC | 470 bytes data | 2 byte for bad block marker > > | 42 bytes data | 14 bytes ECC | unused > > > > FYI, my documentation is located at > > http://rocketboards.org/gitweb/?p=u-boot-socfpga.git;a=blob_plain;f=doc/README.SOCFPGA;hb=refs/heads/socfpga_v2013.01.01 > > For SOCFPAG, Denali controller IP is configured with > 512 byte ECC sector size. OK. > > I refer to "Denali NAND Flash Memory Controller User's Guide". > > Accoding to it, Denali's IP has 2 choice for ECC sector size: > 512 byte or 1024 byte. > > Panasonic's UniPhier SoCs adopt 1024 byte ECC sector size. > (CONFIG_NAND_DENALI_ECC_SIZE = nand->ecc.size = 1024 for us.) > ECC strength (nand->ecc.strength) is selectable from 8bit/16bit/24bit. > (They correspond to nand->ecc.bytes = 14, 28, 42, respectively) > > So, In our SoC s > 1024 byte data | {14 or 28 or 42 byte ECC} | 1024 byte data | > {14 or 28 or 42 byte ECC} ... Hmmm from Denali user guide dated Jul 20 2009, 1024 can use correct bit of 26 or 30 bits. 26 will yield 46 bytes of ECC sector size while 30 will yield 54 bytes sector size. > > > > #ifdef CONFIG_SYS_NAND_15BIT_HW_ECC_OOBFIRST > > #define ECC_15BITS 26 > > static struct nand_ecclayout nand_15bit_oob = { > > .eccbytes = ECC_15BITS, > > }; > > #else > > #define ECC_8BITS 14 > > static struct nand_ecclayout nand_8bit_oob = { > > .eccbytes = ECC_8BITS, > > }; > > #endif /* CONFIG_SYS_NAND_15BIT_HW_ECC_OOBFIRST */ > > I'm afraid this part works only for 512 byte ECC sector. >From the original comment, controller in MRST only support 15bit and 8bit ECC correction. So I am not sure this constrain might due to older version controller. Wonder you have any insight on this? > > Denali's document says > For 512B ECC sector size, > ecc.bytes = Ceiling to next word (13 * ecc.strength) > For 1024B ECC sector size, > ecc.bytes = Ceiling to next word (14 * ecc.strength) > > > > And denali_setup_dma_sequence() function > (Why did you rename this function?) > did not work either. > I needed to fix it locally. > > > So bad news is this version itself does not work for me. > Good news is I could adjust it locally and confirmed some features > worked. (But I think I need more test.) > > So, how will this situation work? > It turned out there are some differences between > two Denali hardwares and this driver works only for yours. > > You merge it first, and (if you don't mind) shall I modify it > in a more generic way to run on both hardwares? > Anyway will work for me. I can take your comments and change the driver accordingly too. > > If you want to run under SPL, there are some patches for that. Let me > > know if you need that. While for U-Boot, they are working fine. Probably > > Thanks for your offering help. > But I am not sure if SOCFPGA and UniPhier can share a SPL nand driver. > (Actually I have locally our own Denali driver for SPL. > And I have Denali driver for main U-Boot, which is adjusted for > our SoCs, too. > But I don't mind to switch onto your driver if it works for me.) > Oh for SPL, I can use the same driver too. Just that I would need to update the nand_spl_simple.c. The main changes is to use the HW ECC feature instead of getting software calculate and fix the ECC. FYI, the patch I made for SPL is located at http://rocketboards.org/gitweb/?p=u-boot-socfpga.git;a=commit;h=461a61b8f03d3b690de1f4ff007cd23fb80018a5 > > +#define CONFIG_SYS_NAND_USE_FLASH_BBT > > +#define CONFIG_SYS_NAND_REGS_BASE SOCFPGA_NAND_REGS_ADDRESS > > +#define CONFIG_SYS_NAND_DATA_BASE SOCFPGA_NAND_DATA_ADDRESS > > +#define CONFIG_SYS_NAND_BASE CONFIG_SYS_NAND_REGS_BASE > > Maybe > #define CONFIG_SYS_NAND_BASE (SOCFPGA_NAND_DATA_ADDRESS + 0x10) > ? > For SOCFPGA, the register and data base address have large address space in between them. End of day, it seems its tightly to hardware guys implementation. > > BTW, you changed all denali->foo to denali.foo. > It looks unnecessay to me. > Hmmm.. I suspect this changed when we declare denali as static global. Thanks Chin Liang > > Best Regards > Masahiro Yamada > > _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
