* Vimal Singh <vimal.newwork@xxxxxxxxx> [091119 00:52]:
> On Wed, Nov 18, 2009 at 10:37 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> > * Vimal Singh <vimal.newwork@xxxxxxxxx> [091118 06:38]:
> >> Tony,
> >>
> >> On Fri, Nov 13, 2009 at 2:14 AM, Tony Lindgren <tony@xxxxxxxxxxx> wrote:
> >> > * Vimal Singh <vimal.newwork@xxxxxxxxx> [091110 02:08]:
> >> >> From 6f535d7128ca392458dd0cb31d138cda84747c06 Mon Sep 17 00:00:00 2001
> >> >> From: Vimal Singh <vimalsingh@xxxxxx>
> >> >> 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.
>
> OK, I'll take this discussion to mtd mailing list. For now I'll remove
> this from here.
OK, I'd assume there's some standard way to handle this for all NOR
drivers.
> >
> >> >
> >> >
> >> >> +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.
> >
>
> Hmmm... In that case I'll remove extra partition informations (for
> zoom2 and sdp) and will have very only few common partitions here.
> Something like this:
>
> +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 */
> + },
> + {
> + .name = "File System - NAND",
> + .offset = MTDPART_OFS_APPEND, /* Offset = 0x2000000 */
> + .size = MTDPART_SIZ_FULL,
> + },
> +};
>
>
> And then, as I said earlier, if someone want different partitions, he
> can always pass his partition information from cmdline (bootargs).
Why don't you just set ldp_nand_paritions[X].size = something during init
based on the machine_is_XXX()?
> >
> >>
> >> >
> >> >
> >> >> +};
> >> >> +
> >> >> +/* 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.
>
> As I said, 'gpmc_cs_request' is done in the mentioned driver itself.
> So, GPMC (cs) gets initialized at that point. And this is exactly why
> I said "calling 'gpmc_cs_request' from driver is fine. Because every
> time you remove driver module (rmmod), you expect 'cs' for that driver
> gets disable (done in gpmc_cs_free) and if insert the driver again it
> gets enabled again (done in gpmc_cs_request).
Well you should be able to use totally standard MTD drivers
by doing only the omap specific initialization under mach-omap2.
For any new drivers, calling gpmc from the driver itself just seems
wrong. That's something we want to stop doing, not add more of it.
How about instead just set up a generic mach-omap2/gpmc-nor.c
and use some standard MTD driver?
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
