On Fri, Jul 17, 2009 at 02:53:55PM +0400, Ilya Yanok wrote:
+/* OOB placement block for use with hardware ecc generation */ +static struct nand_ecclayout nand_hw_eccoob = {
- .eccbytes = 5,
- .eccpos = {6, 7, 8, 9, 10},
- .oobfree = {{0, 5}, {11, 5}, }
+};
+#ifndef CONFIG_MXC_NAND_HWECC +static struct nand_ecclayout nand_hw_eccoob_soft = {
- .eccbytes = 6,
- .eccpos = {6, 7, 8, 9, 10, 11},
- .oobfree = {{0, 5}, {12, 4}, }
+}; +#endif
Why is one struct #ifndef, but the other isn't #ifdef?
+/*
- This function requests the NANDFC to initate the transfer
- of data currently in the NANDFC RAM buffer to the NAND device.
- */
+static void send_prog_page(struct mxc_nand_host *host, uint8_t buf_id,
int spare_only)+{
- MTDDEBUG(MTD_DEBUG_LEVEL3, "send_prog_page (%d)\n", spare_only);
- /* NANDFC buffer 0 is used for page read/write */
- writew(buf_id, &host->regs->nfc_buf_addr);
Comment does not match code.
- /*
* NANDFC buffer 1 is used for device status to prevent* corruption of read/write buffer on status requests.*/- writew(1, &host->regs->nfc_buf_addr);
From discussion on the previous patch:
But it looks like buffer 1 is used for data with large page flash.
Well, we save first word of the buffer and then recover it.
So then there's no longer any special reason to use buffer 1 for status, and that comment is misleading...
+static u_char mxc_nand_read_byte(struct mtd_info *mtd) +{
- struct nand_chip *nand_chip = mtd->priv;
- struct mxc_nand_host *host = nand_chip->priv;
- uint8_t ret = 0;
- uint16_t col, rd_word;
- uint16_t __iomem *main_buf =
(uint16_t __iomem *)host->regs->main_area0;- uint16_t __iomem *spare_buf =
(uint16_t __iomem *)host->regs->spare_area0;- /* Check for status request */
- if (host->status_request)
return get_dev_status(host) & 0xFF;- /* Get column for 16-bit access */
- col = host->col_addr >> 1;
- /* If we are accessing the spare region */
- if (host->spare_only)
rd_word = readw(&spare_buf[col]);- else
rd_word = readw(&main_buf[col]);- /* Pick upper/lower byte of word from RAM buffer */
- if (host->col_addr & 0x1)
ret = (rd_word >> 8) & 0xFF;- else
ret = rd_word & 0xFF;
Use a union, as in read_buf(). Otherwise, this breaks on big-endian -- you may not care, but it's better to not have such dependencies lurking in the code that could be reused who-knows-where.
- if (col & 1) {
rd_word = readw(p);ret = (rd_word >> 8) & 0xff;rd_word = readw(&p[1]);ret |= (rd_word << 8) & 0xff00;
Again, this is unnecessary, but if you insist, use a union.
+#ifdef CONFIG_MTD_NAND_MXC_FORCE_CE
- if (chip > 0) {
MTDDEBUG(MTD_DEBUG_LEVEL0,"ERROR: Illegal chip select (chip = %d)\n", chip);
If it's an error, that's not exactly debug (especially since there's no way to propagate the error upwards)...
return;- }
- if (chip == -1) {
writew(readw(&host->regs->nfc_config1) & ~NFC_CE,&host->regs->nfc_config1);return;- }
- writew(readw(&host->regs->nfc_config1) | NFC_CE,
&host->regs->nfc_config1);+#endif
#else?
if (column >= mtd->writesize) {/** before sending SEQIN command for partial write,* we need read one page out. FSL NFC does not support* partial write. It alway send out 512+ecc+512+ecc ...* for large page nand flash. But for small page nand* flash, it does support SPARE ONLY operation.*/if (host->pagesize_2k) {/* call ourself to read a page */mxc_nand_command(mtd, NAND_CMD_READ0, 0,page_addr);}
#ifdef CONFIG_MXC_NAND_HWECC?
- /* 50 us command delay time */
- this->chip_delay = 5;
5 or 50?
- host->pagesize_2k = 0;
Make this board-configurable. Has large page been tested? If not, perhaps it should stay out for now, given how weird it is on this controller.
-Scott