Re: [PATCH-v5 2/4] OMAP3: Add support for NAND on ZOOM2/LDP boards

[Tony Lindgren <tony@atomide.com>]

* Vimal Singh <vimal.newwork@gmail.com> [091118 06:38]:
> Tony,
> 
> On Fri, Nov 13, 2009 at 2:14 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Vimal Singh <vimal.newwork@gmail.com> [091110 02:08]:
> >> From 6f535d7128ca392458dd0cb31d138cda84747c06 Mon Sep 17 00:00:00 2001
> >> From: Vimal Singh <vimalsingh@ti.com>
> >> Date: Tue, 10 Nov 2009 11:42:45 +0530
> >> Subject: [PATCH] OMAP3: Add support for NAND on ZOOM2/LDP boards
> 
> [...]
> 
> >> +static int omap_ldp_nand_unlock(struct mtd_info *mtd, loff_t ofs, uint64_t len)
> >> +{
> >> +     int ret = 0;
> >> +     int chipnr;
> >> +     int status;
> >> +     unsigned long page;
> >> +     struct nand_chip *this = mtd->priv;
> >> +     printk(KERN_INFO "nand_unlock: start: %08x, length: %d!\n",
> >> +                     (int)ofs, (int)len);
> >> +
> >> +     /* select the NAND device */
> >> +     chipnr = (int)(ofs >> this->chip_shift);
> >> +     this->select_chip(mtd, chipnr);
> >> +     /* check the WP bit */
> >> +     this->cmdfunc(mtd, NAND_CMD_STATUS, -1, -1);
> >> +     if ((this->read_byte(mtd) & 0x80) == 0) {
> >> +             printk(KERN_ERR "nand_unlock: Device is write protected!\n");
> >> +             ret = -EINVAL;
> >> +             goto out;
> >> +     }
> >> +
> >> +     if ((ofs & (mtd->writesize - 1)) != 0) {
> >> +             printk(KERN_ERR "nand_unlock: Start address must be"
> >> +                             "beginning of nand page!\n");
> >> +             ret = -EINVAL;
> >> +             goto out;
> >> +     }
> >> +
> >> +     if (len == 0 || (len & (mtd->writesize - 1)) != 0) {
> >> +             printk(KERN_ERR "nand_unlock: Length must be a multiple of "
> >> +                             "nand page size!\n");
> >> +             ret = -EINVAL;
> >> +             goto out;
> >> +     }
> >> +
> >> +     /* submit address of first page to unlock */
> >> +     page = (unsigned long)(ofs >> this->page_shift);
> >> +     this->cmdfunc(mtd, NAND_CMD_UNLOCK1, -1, page & this->pagemask);
> >> +
> >> +     /* submit ADDRESS of LAST page to unlock */
> >> +     page += (unsigned long)((ofs + len) >> this->page_shift) ;
> >> +     this->cmdfunc(mtd, NAND_CMD_UNLOCK2, -1, page & this->pagemask);
> >> +
> >> +     /* call wait ready function */
> >> +     status = this->waitfunc(mtd, this);
> >> +     udelay(1000);
> >> +     /* see if device thinks it succeeded */
> >> +     if (status & 0x01) {
> >> +             /* there was an error */
> >> +             printk(KERN_ERR "nand_unlock: error status =0x%08x ", status);
> >> +             ret = -EIO;
> >> +             goto out;
> >> +     }
> >> +
> >> + out:
> >> +     /* de-select the NAND device */
> >> +     this->select_chip(mtd, -1);
> >> +     return ret;
> >> +}
> >
> > Isn't the unlocking generic to the NAND device driver? Or is it somehow board
> > specific?
> 
> Yes, zoom2 boards come up with whole NAND locked, whereas it is not
> case for SDP boards.

But the procedure should be done under drivers/mtd I believe using some
standard tools.
 
> >
> >
> >> +static struct mtd_partition ldp_nand_partitions[] = {
> >> +     /* All the partition sizes are listed in terms of NAND block size */
> >> +     {
> >> +             .name           = "X-Loader-NAND",
> >> +             .offset         = 0,
> >> +             .size           = 4 * (64 * 2048),      /* 512KB, 0x80000 */
> >> +             .mask_flags     = MTD_WRITEABLE,        /* force read-only */
> >> +     },
> >> +     {
> >> +             .name           = "U-Boot-NAND",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0x80000 */
> >> +             .size           = 10 * (64 * 2048),     /* 1.25MB, 0x140000 */
> >> +             .mask_flags     = MTD_WRITEABLE,        /* force read-only */
> >> +     },
> >> +     {
> >> +             .name           = "Boot Env-NAND",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0x1c0000 */
> >> +             .size           = 2 * (64 * 2048),      /* 256KB, 0x40000 */
> >> +     },
> >> +     {
> >> +             .name           = "Kernel-NAND",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0x0200000*/
> >> +             .size           = 240 * (64 * 2048),    /* 30M, 0x1E00000 */
> >> +     },
> >> +#ifdef CONFIG_MACH_OMAP_ZOOM2
> >> +     {
> >> +             .name           = "system",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0x2000000 */
> >> +             .size           = 1280 * (64 * 2048),   /* 160M, 0xA000000 */
> >> +     },
> >> +     {
> >> +             .name           = "userdata",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0xC000000 */
> >> +             .size           = 256 * (64 * 2048),    /* 32M, 0x2000000 */
> >> +     },
> >> +     {
> >> +             .name           = "cache",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0xE000000 */
> >> +             .size           = 256 * (64 * 2048),    /* 32M, 0x2000000 */
> >> +     },
> >> +#else
> >> +     {
> >> +             .name           = "File System - NAND",
> >> +             .offset         = MTDPART_OFS_APPEND,   /* Offset = 0x2000000 */
> >> +             .size           = MTDPART_SIZ_FULL,     /* 96MB, 0x6000000 */
> >> +     },
> >> +#endif
> >
> > Please remove the ifdefs, you should be able to compile in all mach-omap2
> > boards into the same kernel binary. You should set the size dynamically as
> > needed.
> 
> We can always provide partitioning information from cmdline
> (mtdparts:). Which will anyway have higher precedence than this. Here
> are trying provide default static partitions. Which IMHO is fine.
> 
> see this too:
> http://marc.info/?l=linux-omap&m=125178914327011&w=2

No way we're adding any more of these ifdef else hacks.

The whole idea of the platform_data is to pass the board specific
configuration to the driver. The driver should be generic.


> 
> >
> >
> >> +};
> >> +
> >> +/* NAND chip access: 16 bit */
> >> +static struct omap_nand_platform_data ldp_nand_data = {
> >> +     .parts          = ldp_nand_partitions,
> >> +     .nr_parts       = ARRAY_SIZE(ldp_nand_partitions),
> >> +     .nand_setup     = NULL,
> >> +     .dma_channel    = -1,           /* disable DMA in OMAP NAND driver */
> >> +     .dev_ready      = NULL,
> >> +     .unlock         = omap_ldp_nand_unlock,
> >> +};
> >> +
> >> +static struct resource ldp_nand_resource = {
> >> +     .flags          = IORESOURCE_MEM,
> >> +};
> >> +
> >> +static struct platform_device ldp_nand_device = {
> >> +     .name           = "omap2-nand",
> >> +     .id             = 0,
> >> +     .dev            = {
> >> +     .platform_data  = &ldp_nand_data,
> >> +     },
> >> +     .num_resources  = 1,
> >> +     .resource       = &ldp_nand_resource,
> >> +};
> >> +
> >> +/**
> >> + * ldp430_flash_init - Identify devices connected to GPMC and register.
> >> + *
> >> + * @return - void.
> >> + */
> >> +void __init zoom_flash_init(void)
> >> +{
> >> +     u8 nandcs = GPMC_CS_NUM + 1;
> >> +     u32 gpmc_base_add = OMAP34XX_GPMC_VIRT;
> >> +
> >> +     /* pop nand part */
> >> +     nandcs = LDP3430_NAND_CS;
> >> +
> >> +     ldp_nand_data.cs = nandcs;
> >> +     ldp_nand_data.gpmc_cs_baseaddr = (void *)(gpmc_base_add +
> >> +                                     GPMC_CS0_BASE + nandcs * GPMC_CS_SIZE);
> >> +     ldp_nand_data.gpmc_baseaddr = (void *) (gpmc_base_add);
> >> +
> >> +     if (platform_device_register(&ldp_nand_device) < 0)
> >> +             printk(KERN_ERR "Unable to register NAND device\n");
> >> +}
> >
> > This too should use gpmc_cs_request().
> 
> This is just platform data, needed by nand driver (nand/omap2.c).
> 'gpmc_cs_request' is separately done in mentioned driver. And IMO that
> is the correct place to do this, as every time (when driver is
> compiled as module) inserting and de-inserting driver, will need this.

Without gpmc_cs_request the GPMC can be in uninitialized state.
You're basically relying on some hardcoded values from the bootloader,
which can be at whatever version and whatever bootloader. Not good.

Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html