| Message ID | 2AF67696C6634054B7CFDF7A9C19865D@sisodomain.com |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 2009-06-12 at 15:56 +0530, Amul Saha wrote: > This patch now adds support for Flex-OneNAND to be used as a module, > it also supports Boundary setting at module insertion time > > Signed-off-by: Amul Kumar Saha <amul.saha at samsung.com> > Signed-off-by: Vishak G <vishak.g at samsung.com> > --- > onenand_base.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c > index 8d4c9c2..6d2086f 100644 > --- a/drivers/mtd/onenand/onenand_base.c > +++ b/drivers/mtd/onenand/onenand_base.c > @@ -20,6 +20,7 @@ > > #include <linux/kernel.h> > #include <linux/module.h> > +#include <linux/moduleparam.h> > #include <linux/init.h> > #include <linux/sched.h> > #include <linux/delay.h> > @@ -31,6 +32,16 @@ > > #include <asm/io.h> > > +static char *flex_bdry_info; > + > +module_param(flex_bdry_info, charp, 0400); > +MODULE_PARM_DESC(flex_bdry_info, "SLC Boundary information for Flex-OneNAND" > + "Syntax:flex_bdry_info=DIE_BDRY,LOCK,..." > + "DIE_BDRY: SLC boundary of the die" > + "LOCK: Locking information for SLC boundary" > + " : 0->Set boundary in unlocked status" > + " : 1->Set boundary in locked status"); > + > /* Default Flex-OneNAND boundary and lock respectively */ > static int flex_bdry[MAX_DIES * 2] = { -1, 0, -1, 0 }; > > @@ -3456,6 +3467,10 @@ int onenand_scan(struct mtd_info *mtd, int maxchips) > if (onenand_probe(mtd)) > return -ENXIO; > > +#ifdef MODULE > + flexonenand_setup(flex_bdry_info); > +#endif Why do you need this ifdef? What is the fundamental difference between onenand.ko as a module and onenand compiled-in?
Hi Artem, >> +#ifdef MODULE >> + flexonenand_setup(flex_bdry_info); >> +#endif > > Why do you need this ifdef? What is the fundamental difference between > onenand.ko as a module and onenand compiled-in? > flexonenand_setup( ) need not be called, when OneNAND is built-in. This function-call will cause overhead unwantedly on every boot, during OneNAND scan. flexonenand_setup( ) call is needed only when it has been built as a module. Regards, Amul Kumar Saha
On Fri, 2009-06-12 at 17:27 +0530, Amul Saha wrote: > Hi Artem, > > >> +#ifdef MODULE > >> + flexonenand_setup(flex_bdry_info); > >> +#endif > > > > Why do you need this ifdef? What is the fundamental difference between > > onenand.ko as a module and onenand compiled-in? > > > > flexonenand_setup( ) need not be called, when OneNAND is built-in. > This function-call will cause overhead unwantedly on every boot, during OneNAND scan. > > flexonenand_setup( ) call is needed only when it has been built as a module. Why?
On Fri, 2009-06-12 at 15:03 +0300, Artem Bityutskiy wrote: > > > flexonenand_setup( ) call is needed only when it has been built as a > module. > > Why? Because they're adding a 'flex_bdry_info' module parameter which is a string, that they then process just the same way as the original command line option. I think that's the wrong approach -- please use module_param_array() instead, so it sets the contents of the flex_bdry[] array directly and you can remove the flexonenand_setup() function completely. Please ensure that your email addresses in the Signed-off-by: line are correct, too. None of this 'user at domain' nonsense.
>> >> +#ifdef MODULE >> >> + flexonenand_setup(flex_bdry_info); >> >> +#endif >> > >> > Why do you need this ifdef? What is the fundamental difference between >> > onenand.ko as a module and onenand compiled-in? >> > >> >> flexonenand_setup( ) need not be called, when OneNAND is built-in. >> This function-call will cause overhead unwantedly on every boot, during OneNAND scan. >> >> flexonenand_setup( ) call is needed only when it has been built as a module. > > Why? > When Flex-OneNAND is built-in, the SLC boundary can be set by kernel command line. During the boot up time, flexonenand_setup() gets invoked by kernel, on parsing the kernel command line. But when Flex-OneNAND is built as a module, SLC boundary information is passed as module parameter. So in this case flexonenand_setup() has to be called explicitly during insomd time (module_init), to set the desired boundary. So without a compilation macro (#ifdef MODULE) flexonenand_setup() is invoked in onenand_scan, even when it is built-in which is not needed. I am not getting your point, could you please clarify? Regards, Amul Kumar Saha
On Fri, 2009-06-12 at 18:46 +0530, Amul Saha wrote: > >> >> +#ifdef MODULE > >> >> + flexonenand_setup(flex_bdry_info); > >> >> +#endif > >> > > >> > Why do you need this ifdef? What is the fundamental difference between > >> > onenand.ko as a module and onenand compiled-in? > >> > > >> > >> flexonenand_setup( ) need not be called, when OneNAND is built-in. > >> This function-call will cause overhead unwantedly on every boot, during OneNAND scan. > >> > >> flexonenand_setup( ) call is needed only when it has been built as a module. > > > > Why? > > > > When Flex-OneNAND is built-in, the SLC boundary can be set by kernel command line. > During the boot up time, flexonenand_setup() gets invoked by kernel, on parsing the kernel > command line. > > But when Flex-OneNAND is built as a module, SLC boundary information is passed as module > parameter. > So in this case flexonenand_setup() has to be called explicitly during insomd time > (module_init), to set the desired boundary. > So without a compilation macro (#ifdef MODULE) flexonenand_setup() is invoked in onenand_scan, > even when it is built-in which is not needed. > > I am not getting your point, could you please clarify? You can remove flexonenand_setup() and the __setup("onenand_brdy=") completely, and use _only_ "module_param_array()". That will work both for modules and for the built-in case.
Hi David, > > You can remove flexonenand_setup() and the __setup("onenand_brdy=") > completely, and use _only_ "module_param_array()". That will work both > for modules and for the built-in case. > OK , Got your point. Working towards it, will come up with the updated patch. Regards Amul Kumar Saha
On Fri, 2009-06-19 at 12:20 +0530, apgmoorthy wrote: > Hi David, > > Currenlty , Is there a way to use JFFS2 with MLC Memories ( NOP == 1 )? > > If not , Felt that any one of the following patch can be of use > > 1. http://lists.infradead.org/pipermail/linux-mtd/2007-December/020047.html > - Which removes the Clean Marker. There was a reason for the cleanmarker -- it saved us from using partly-erased blocks for data. Do you have some other way to guarantee that this won't happen? (Sorry, I think we may have had this discussion before, but I've forgotten the conclusion, if there was one. It was academic at the time, since we didn't have any solution for the paired-page insanity anyway. I was mostly just burying my head in the sand and hoping this stupid 'MLC' crap would go away.)
Hi David, On Tuesday, June 23, 2009 12:45 AM David Woodhouse wrote : >> If not , Felt that any one of the following patch can be of use >> >> 1. http://lists.infradead.org/pipermail/linux-mtd/2007-December/020047.html >> - Which removes the Clean Marker. >There was a reason for the cleanmarker -- it saved us from using partly-erased blocks for data. Do you have some other way to guarantee that this won't >happen? - How about the second One (Refer Below Links), which leaves 1page/block for Cleanmarker. http://lists.infradead.org/pipermail/linux-mtd/2008-September/023101.html http://lists.infradead.org/pipermail/linux-mtd/2008-September/023044.html >(Sorry, I think we may have had this discussion before, but I've forgotten the conclusion, if there was one. It was academic at the time, since we didn't >have any solution for the paired-page insanity anyway. - Right , I dont see any detailed discussion on this regard in the list. With Regards Moorthy
Your mail program is behaving very strangely -- your quotes are quite hard to read. Is there any chance you could use a better one? I've tried to clean it up, but it tends to make your replies hard to read. Some of your replies are lacking correct References: headers too, so the threading is broken. On Tue, 2009-06-23 at 18:22 +0530, apgmoorthy wrote: > >There was a reason for the cleanmarker -- it saved us from using > > partly-erased blocks for data. Do you have some other way to > > guarantee that this won't happen? > > - How about the second One (Refer Below Links), which leaves > 1page/block for Cleanmarker. > > http://lists.infradead.org/pipermail/linux-mtd/2008-September/023101.html http://lists.infradead.org/pipermail/linux-mtd/2008-September/023044.html Using one page per block for the cleanmarker would work, but seems wasteful. I can think of one more approach which we might take -- erase blocks only as we need them. So instead of erasing blocks without cleanmarker _immediately_ on startup, we just put them on a 'to be erased' list. Whenever the empty_list is getting short of blocks, we erase another block from the 'to be erased' list and move it to the empty list. It does mean that if you keep rebooting over and over again, you're probably going to increase your erase count. But with a long-lived system, it's likely to be more efficient than the large cleanmarker? What do you think?
On Tuesday, June 23, 2009 6:50 PM David Woodhouse wrote: > Your mail program is behaving very strangely -- your quotes are quite > hard to read. Is there any chance you could use a better one? - Earlier Mailer had some issues , Now using a different one. > So instead of erasing blocks without cleanmarker _immediately_ on > startup, we just put them on a 'to be erased' list. Whenever the > empty_list is getting short of blocks, we erase another block from the > 'to be erased' list and move it to the empty list. > > It does mean that if you keep rebooting over and over again, you're > probably going to increase your erase count. But with a long-lived > system, it's likely to be more efficient than the large cleanmarker? > > What do you think? > - Yes , the solution looks a better tradeoff. Will work in that direction and get a pacth. Later we can look into a way to optimize erase count. With Regards Moorthy
Hi David, We have come up with an alternate solution to support MLC devices on JFFS2. >> >> Currenlty , Is there a way to use JFFS2 with MLC Memories ( NOP == 1 )? >> >> If not , Felt that any one of the following patch can be of use >> >> 1. http://lists.infradead.org/pipermail/linux-mtd/2007-December/020047.html >> - Which removes the Clean Marker. > > There was a reason for the cleanmarker -- it saved us from using > partly-erased blocks for data. Do you have some other way to guarantee > that this won't happen? > > (Sorry, I think we may have had this discussion before, but I've > forgotten the conclusion, if there was one. It was academic at the time, > since we didn't have any solution for the paired-page insanity anyway. I > was mostly just burying my head in the sand and hoping this stupid 'MLC' > crap would go away.) New approach:- We will have something known as the TOKEN_BLOCK. TOKEN_BLOCK: A block used to store all the ready_to_use blocks related info. ready_to_use blocks: These are blocks which are erased and ready to use, but don't have a usual CLEANMARKER on them. To save NOP for the 1st page of each block. How it works: 1. Ask GC to provide a block, and use it as TOKEN_BLOCK on need basis. 2. In the provided block, we store all ready_to_use block info. 3. On remount; this info needs to be retreived. So, we shall make use of MAGIC_NUMBER signifying TOKEN_BLOCK, to identify the block containing the info. Scan has to be modified, to identify the TOKEN_BLOCK and act accordingly. Handling TOKEN_BLOCK 1. Make request for a block to be used as TOKEN_BLOCK. 2. List all the ready_to_use block info on the TOKEN_BLOCK contiguously. 3. Write the TOKEN_BLOCK's MAGIC_NUMBER in the OOB Area alongwith the ready_to_use info, in the Main Area. 4. On next update, ask GC to provide a fresh block. 5. Update this block accordingly as TOKEN_BLOCK. 6. Discard the previous TOKEN_BLOCK, and put it on dirty_list. Let me know, about the feasibility of this design from your perspective. This is just a base plan, and obvisouly there are a lot of things that have to be looked into, before getting into the real imlpementation. Regards, Amul Kumar Saha
Hi Amul, > 3. Write the TOKEN_BLOCK's MAGIC_NUMBER in the OOB Area alongwith the > ready_to_use info, in the Main Area. I think you should protect each ready_to_use block info in the TOKEN_BLOCK with a CRC - as it was a node of JFFS2 - against power loss corruption. > 4. On next update, ask GC to provide a fresh block. What do you mean ? Each time a block is erased do you update the TOKEN_BLOCK by using a new one ?
Hi, >> 3. Write the TOKEN_BLOCK's MAGIC_NUMBER in the OOB Area alongwith the >> ready_to_use info, in the Main Area. > I think you should protect each ready_to_use block info in the TOKEN_BLOCK > with a CRC - as it was a node of JFFS2 - against power loss corruption. > Sure, I'll take that into account. >> 4. On next update, ask GC to provide a fresh block. > What do you mean ? Each time a block is erased do you > update the TOKEN_BLOCK by using a new one ? > No. That would be done only during a proper unmount or idle time. As this block just contains ready_to_use block info, that can be reconstructed, on remount. Because any updation on fixed intervals will not be of use. Neither will it be feasbale to update the TOKEN_BLOCK on every erase. Please do comment. Regards, Amul
Ok, but at the remount how do you verify if the TOKEN_BLOCK is updated or not (in case of power loss) ? I suggest using a commit mark (one page wasted) at the end of the update procedure. So you'll have in the TOKEN_BLOCK the following layout: MAGIC_NUMBER of the TOKEN_BLOCK ready_to_use info commit mark What do you think ? 2010/4/5 Amul Kumar Saha <amul.saha@samsung.com>: > Hi, > >>> 3. Write the TOKEN_BLOCK's MAGIC_NUMBER in the OOB Area alongwith the >>> ready_to_use info, in the Main Area. >> I think you should protect each ready_to_use block info in the TOKEN_BLOCK >> with a CRC - as it was a node of JFFS2 - against power loss corruption. >> > > Sure, I'll take that into account. > >>> 4. On next update, ask GC to provide a fresh block. >> What do you mean ? Each time a block is erased do you >> update the TOKEN_BLOCK by using a new one ? >> > > No. > That would be done only during a proper unmount or idle time. > As this block just contains ready_to_use block info, > that can be reconstructed, on remount. > Because any updation on fixed intervals will not be of use. > Neither will it be feasbale to update the TOKEN_BLOCK on every erase. > Please do comment. > > Regards, > Amul > > >
Hi, >Ok, >but at the remount how do you verify if the TOKEN_BLOCK >is updated or not (in case of power loss) ? I suggest using >a commit mark (one page wasted) at the end of the update >procedure. >So you'll have in the TOKEN_BLOCK the following layout: > >MAGIC_NUMBER of the TOKEN_BLOCK >ready_to_use info >commit mark > >What do you think ? > Yes. However, the TOKEN_BLOCK update would then only be limited to mount and unmount. As writing a commit mark while the system is still running, would validate an invalid block on abrupt power-off. Regards, Amul
Hi, I agree with you, and the following is my thinking. Let's suppose we have a flash with 2KB page, 128KB block, 4096 blocks (512MB as size of the flash). The cleanmarker would take one page for every block, so the wasted space is 2KB*4096=8MB. Instead if we put the erased info in a single block (TOKEN_BLOCK) we waste only 128KB. But we need to consider the overhead in the management of such a block. We need to store information for 4096 blocks, for example an array of 4096 bit (1=erased, 0=not erased), protected by ECC (against read error) and CRC (against power loss in writing the array on the block). We need 2 pages for each instance of the array. The question is: how often do we need to update this array? If we choose to update each time it is needed (that is when a block is just erased and just before a block is taken from the free list), at the 32nd update we should erase the TOKEN_BLOCK and allocate a new one. IMO this overhead is unacceptable in terms of time performance. So we update the info only at the unmount and when the system is idle. Moreover, there is the problem of power loss between two updates: how to recognize at the mount that the array info is updated? In my opinion the solution is to write an invalidation mark (one page with all zeros) just after the instance of the array when the instance becomes invalid. Bye 2010/4/9 Amul Kumar Saha <amul.saha@samsung.com>: > Hi, > > >>Ok, >>but at the remount how do you verify if the TOKEN_BLOCK >>is updated or not (in case of power loss) ? I suggest using >>a commit mark (one page wasted) at the end of the update >>procedure. >>So you'll have in the TOKEN_BLOCK the following layout: >> >>MAGIC_NUMBER of the TOKEN_BLOCK >>ready_to_use info >>commit mark >> >>What do you think ? >> > > Yes. > However, the TOKEN_BLOCK update would then > only be limited to mount and unmount. > As writing a commit mark while the system is still running, > would validate an invalid block on abrupt power-off. > > Regards, > Amul > > >
yes, the update only at the unmount and the usage of the commit mark are ok for me. 2010/4/15 Raghav Gupta <raghav.gupta@partner.samsung.com>: > Resending it due to formatting issues. My bad. > > Hi, > >> I agree with you, and the following is my thinking. >> Let's suppose we have a flash with 2KB page, 128KB block, >> 4096 blocks (512MB as size of the flash). >> The cleanmarker would take one page for every block, so the >> wasted space is 2KB*4096=8MB. Instead if we put the erased >> info in a single block (TOKEN_BLOCK) we waste only 128KB. >> But we need to consider the overhead in the management of >> such a block. We need to store information for 4096 blocks, >> for example an array of 4096 bit (1=erased, 0=not erased), >> protected by ECC (against read error) and CRC (against >> power loss in writing the array on the block). We need 2 pages >> for each instance of the array. The question is: how often do we >> need to update this array? If we choose to update each time it is >> needed (that is when a block is just erased and just before a block >> is taken from the free list), at the 32nd update we should erase the >> TOKEN_BLOCK and allocate a new one. IMO this overhead is >> unacceptable in terms of time performance. So we update the info >> only at the unmount and when the system is idle. >> Moreover, there is the problem of power loss between two updates: >> how to recognize at the mount that the array info is updated? In my >> opinion the solution is to write an invalidation mark (one page with >> all zeros) just after the instance of the array when the instance >> becomes invalid. > > > Yes, I agree with you. The overhead involved in updating the token block > on every change in the "ready_to_use" list is unacceptable. > So, we can update the TOKEN_BLOCK only at unmount. > > Also we can authenticate that the TOKEN_BLOCK as valid > by using a "valid/invalid flag"(commit mark) > > And to guard against reading of an older version of > a TOKEN_BLOCK (on power failure) as valid by immediately > deleting the block on retrieval of the 'ready_to_use' list. > > Regards, > Raghav Gupta > > >
Resending the message due to a Mail Delivery error returned by raghav.gupta@partner.samsung.com. 2010/4/15 massimo cirillo <maxcir@gmail.com>: > yes, > the update only at the unmount and the usage of the commit > mark are ok for me. > > 2010/4/15 Raghav Gupta <raghav.gupta@partner.samsung.com>: >> Resending it due to formatting issues. My bad. >> >> Hi, >> >>> I agree with you, and the following is my thinking. >>> Let's suppose we have a flash with 2KB page, 128KB block, >>> 4096 blocks (512MB as size of the flash). >>> The cleanmarker would take one page for every block, so the >>> wasted space is 2KB*4096=8MB. Instead if we put the erased >>> info in a single block (TOKEN_BLOCK) we waste only 128KB. >>> But we need to consider the overhead in the management of >>> such a block. We need to store information for 4096 blocks, >>> for example an array of 4096 bit (1=erased, 0=not erased), >>> protected by ECC (against read error) and CRC (against >>> power loss in writing the array on the block). We need 2 pages >>> for each instance of the array. The question is: how often do we >>> need to update this array? If we choose to update each time it is >>> needed (that is when a block is just erased and just before a block >>> is taken from the free list), at the 32nd update we should erase the >>> TOKEN_BLOCK and allocate a new one. IMO this overhead is >>> unacceptable in terms of time performance. So we update the info >>> only at the unmount and when the system is idle. >>> Moreover, there is the problem of power loss between two updates: >>> how to recognize at the mount that the array info is updated? In my >>> opinion the solution is to write an invalidation mark (one page with >>> all zeros) just after the instance of the array when the instance >>> becomes invalid. >> >> >> Yes, I agree with you. The overhead involved in updating the token block >> on every change in the "ready_to_use" list is unacceptable. >> So, we can update the TOKEN_BLOCK only at unmount. >> >> Also we can authenticate that the TOKEN_BLOCK as valid >> by using a "valid/invalid flag"(commit mark) >> >> And to guard against reading of an older version of >> a TOKEN_BLOCK (on power failure) as valid by immediately >> deleting the block on retrieval of the 'ready_to_use' list. >> >> Regards, >> Raghav Gupta >> >> >> >
diff --git a/drivers/mtd/onenand/onenand_base.c b/drivers/mtd/onenand/onenand_base.c index 8d4c9c2..6d2086f 100644 --- a/drivers/mtd/onenand/onenand_base.c +++ b/drivers/mtd/onenand/onenand_base.c @@ -20,6 +20,7 @@ #include <linux/kernel.h> #include <linux/module.h> +#include <linux/moduleparam.h> #include <linux/init.h> #include <linux/sched.h> #include <linux/delay.h> @@ -31,6 +32,16 @@ #include <asm/io.h> +static char *flex_bdry_info; + +module_param(flex_bdry_info, charp, 0400); +MODULE_PARM_DESC(flex_bdry_info, "SLC Boundary information for Flex-OneNAND" + "Syntax:flex_bdry_info=DIE_BDRY,LOCK,..." + "DIE_BDRY: SLC boundary of the die" + "LOCK: Locking information for SLC boundary" + " : 0->Set boundary in unlocked status" + " : 1->Set boundary in locked status"); + /* Default Flex-OneNAND boundary and lock respectively */ static int flex_bdry[MAX_DIES * 2] = { -1, 0, -1, 0 }; @@ -3456,6 +3467,10 @@ int onenand_scan(struct mtd_info *mtd, int maxchips) if (onenand_probe(mtd)) return -ENXIO; +#ifdef MODULE + flexonenand_setup(flex_bdry_info); +#endif + /* Set Sync. Burst Read after probing */ if (this->mmcontrol) { printk(KERN_INFO "OneNAND Sync. Burst Read support\n");