On Tue, Apr 12, 2011 at 5:08 AM, Stefano Babic sbabic@denx.de wrote:
Ben Gardiner wrote:
Thanks for sharing this patch -- I have been using the "-O 2048" (VID header offset) option to prevent subpages here.
Yes, this works too, at least with Linux.
(being picky / for archival purposes) In u-boot, specifying the VID header offset as an argument to the ubi attach command -- i.e. 'ubi part <mtd_partition> 2048' -- also works (on da850evm, at least).
[...]
First let me say that I would be happy to have a fix for this merged -- so I think the change is "good enough" since it makes UBI work without specifying a VID header offset equal to the NAND page size. What follows is more topical musings that any particular criticisms that should be considered blockers of your patch.
I'm wondering about the long-term path for davinci NAND and subpage writes...
But the sentiment I've heard about adding an exception to NAND_CHIPOPTIONS_MSK as above is that we are mixing features of the NAND chip and features of the NAND controller (If you have a modern SLC NAND then your chip probably supports subpage writes whereas it is the controller that needs to be limited).
That is true. With this patch I constrained the SLC NAND on my board to be considered as MLC, as a MLC NAND cannot be accessed in subpage mode.
However, I set also a CONFIG_SYS_NAND_NO_SUBPAGE, and instead of dropping the option in the mask we could protect the code where the option is checked. In other words, we could change nand_base.c in this way:
#ifndef CONFIG_SYS_NAND_NO_SUBPAGE if (!(chip->options & NAND_NO_SUBPAGE_WRITE) && !(chip->cellinfo & NAND_CI_CELLTYPE_MSK)) { switch(chip->ecc.steps) { case 2: mtd->subpage_sft = 1; break; case 4: case 8: case 16: mtd->subpage_sft = 2; break; } } #endif
Then it could be clearer that the restriction is due to the NAND controller, and not to the chip itself.
I agree -- it keeps the chip and controller options separated.
What's more is that the davinci nand controller could do subpage writing if the writing operation were informed of the extents of the subpage being written instead of being handed a buffer with 0xFF in the non-target page areas. Which, I believe is Artem's primary motivation for the introduction of nand_write_subpage() to the davinci NAND controller driver [2].
Well, of course, if the davinci NAND can handle subpages, we can remove the limitation. What do you think to add this patch in the way I suggest now (without touching NAND_CHIPOPTIONS_MSK, that makes the MTD inconsistent with Linux) until a subpage_write is added to davinci ?
Yes, I like the way your proposed changes disabled subpage initialization instead of adding an exception to the chip options mask. I'm still OK with merging a patch to disable subpage writes on da850 -- as an interim solution. i.e. until a subpage_write is added to the mtd subsystem, and davinci-nand has an override and u-boot gets a similar change.
So if the nand_write_subpage family of functions was introduced to the Linux kernel instead of adding another exception to NAND_CHIPOPTIONS_MSK then we would have 3 ways to use UBI on davinci NAND:
- no patch: VID offset <page size> required
- chip NAND_NO_SUBPAGE_WRITE patch
- subpage writes support with nand_write_subpage()
systems with 2) or 3) could always use UBI as in 1) and a UBI volume made with 2) would need to be attached as in 1) on systems with 2) or 3) ; but a UBI volume made with 3) could not be used on systems with
- or 2) which means that we could not make use of the efficiency
benefits of nand_write_subpage() if/when it is available on systems which need access to UBI from U-boot.
I agree with your analyses. As personal feeling, solution one has the drawback that it cannot be used in u-boot, and it seems to me very easy to have a wrong UBI on the target. We can consider 2) as temporary solution, until 3) is available.
Thanks for confirming, Stefano.
I'm not intimately familiar with the ea20 so there could be additional quirks in the way -- but I can say that option 1) works in u-boot on the da850evm (+UI board). I use 'ubi part ubi_part 2048' -- ubi_part is the mtdpart name of my ubi part :)
Best Regards, Ben Gardiner
--- Nanometrics Inc. http://www.nanometrics.ca