On Wed, Nov 18, 2009 at 10:37 PM, Tony Lindgren <[email protected]> wrote:
> * Vimal Singh <[email protected]> [091118 06:38]:
>> Tony,
>>
>> On Fri, Nov 13, 2009 at 2:14 AM, Tony Lindgren <[email protected]> wrote:
>> > * Vimal Singh <[email protected]> [091110 02:08]:
>> >> From 6f535d7128ca392458dd0cb31d138cda84747c06 Mon Sep 17 00:00:00 2001
>> >> From: Vimal Singh <[email protected]>
>> >> 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.
>
>> >
>> >
>> >> +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).
>
>>
>> >
>> >
>> >> +};
>> >> +
>> >> +/* 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).
--
Regards,
Vimal Singh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
