On Mon, Nov 23, 2009 at 11:15 PM, Tony Lindgren <tony@xxxxxxxxxxx> wrote: > * 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. > I still think 'gpmc_cs_request' should be done from driver itself, why to get a resource allocated if driver itself is not present (in case driver is compile as a module and yet not inserted). As it is already done in 'onenand/omap2.c' and 'nand/omap2.c'. Though, for NOR we can do 'gpmc_cs_request' in 'board-*-flash.c' itself. As I did in my patch series V6. -- Regards, Vimal Singh > 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