| Message ID | 20090723.001338.27942898.anemo@mba.ocn.ne.jp |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jul 22, 2009 at 8:43 PM, Atsushi Nemoto<anemo@mba.ocn.ne.jp> wrote: > On Wed, 22 Jul 2009 14:13:30 +0530, vimal singh <vimal.newwork@gmail.com> wrote: >> >> This can be handled by overriding 'chip->ecc.correct' in driver. You >> >> may reuse 'nand_correct_data()' code in the driver of 256 byte sector >> >> ECC. >> > >> > Yes, the driver can reuse nand_ecc code to implement its own >> > nand_correct rountine. >> > >> > Or how about adding support for 6byte-ecc/512byte-data to nand_ecc.c >> > to avoid code duplication? >> > >> > I mean something like this. If it looks acceptable, I will prepare a >> > patch including similer change to nand_calculate_ecc. >> >> I personally do not like any HW specific implementation going into the >> generic part >> of the code. This particular issue is specific to your HW, so better >> handle it in the >> driver only. > > OK, but I still feel duplicating nand_ecc code is not so good. How > about splitting nand_correct_data into two parts? A pure calculation > function and a wrapper for mtd interface. Like this: But I do not see any thing extra, which you achieve from this wrapper... Is this a prototype, and you want to handle above scenario in this wrapper (calling 'nand_correct_data' multiple times based on something, probably 'ecc.bytes')? > > diff --git a/drivers/mtd/nand/nand_ecc.c b/drivers/mtd/nand/nand_ecc.c > index c0cb87d..3920896 100644 > --- a/drivers/mtd/nand/nand_ecc.c > +++ b/drivers/mtd/nand/nand_ecc.c > @@ -425,14 +425,14 @@ EXPORT_SYMBOL(nand_calculate_ecc); > * > * Detect and correct a 1 bit error for 256/512 byte block > */ > -int nand_correct_data(struct mtd_info *mtd, unsigned char *buf, > - unsigned char *read_ecc, unsigned char *calc_ecc) > +int __nand_correct_data(unsigned char *buf, > + unsigned char *read_ecc, unsigned char *calc_ecc, > + unsigned int eccsize) > { > unsigned char b0, b1, b2, bit_addr; > unsigned int byte_addr; > /* 256 or 512 bytes/ecc */ > - const uint32_t eccsize_mult = > - (((struct nand_chip *)mtd->priv)->ecc.size) >> 8; > + const uint32_t eccsize_mult = eccsize >> 8; > > /* > * b0 to b2 indicate which bit is faulty (if any) > @@ -495,6 +495,16 @@ int nand_correct_data(struct mtd_info *mtd, unsigned char *buf, > printk(KERN_ERR "uncorrectable error : "); > return -1; > } > +EXPORT_SYMBOL(__nand_correct_data); > + > +int nand_correct_data(struct mtd_info *mtd, unsigned char *buf, > + unsigned char *read_ecc, unsigned char *calc_ecc) > +{ > + const uint32_t eccsize_mult = > + (((struct nand_chip *)mtd->priv)->ecc.size) >> 8; > + return __nand_correct_data(buf, read_ecc, calc_ecc, > + ((struct nand_chip *)mtd->priv)->ecc.size); > +} > EXPORT_SYMBOL(nand_correct_data); > > MODULE_LICENSE("GPL"); > > > There is a little bonus: the STANDALONE macro in nand_ecc.c can be > more useful with this change ;) > > --- > Atsushi Nemoto >
On Thu, 23 Jul 2009 12:34:43 +0530, vimal singh <vimal.newwork@gmail.com> wrote: > > OK, but I still feel duplicating nand_ecc code is not so good. How > > about splitting nand_correct_data into two parts? A pure calculation > > function and a wrapper for mtd interface. Like this: > > But I do not see any thing extra, which you achieve from this > wrapper... Is this a prototype, and you want to handle above scenario > in this wrapper (calling 'nand_correct_data' multiple times based on > something, probably 'ecc.bytes')? Yes, if we have __nand_correct_data(buf, read_ecc, calc_ecc, eccsize) which do job based on its eccsize argument, the ecc.correct function of the tmio_nand driver can be implemented like this: int tmio_nand_correct_data(struct mtd_info *mtd, unsigned char *buf, unsigned char *read_ecc, unsigned char *calc_ecc) { int r0, r1; /* assume ecc.size = 512 and ecc.bytes = 6 */ r0 = __nand_correct_data(buf, read_ecc, calc_ecc, 256); if (r0 < 0) return r0; r1 = __nand_correct_data(buf + 256, read_ecc + 3, calc_ecc + 3, 256); if (r1 < 0) return r1; return r0 | r1; } Note that this is not tested at all since I do not have tmio device. Of course if Dmitry had another idea, I will not intrude this approach. --- Atsushi Nemoto
2009/7/23 Atsushi Nemoto <anemo@mba.ocn.ne.jp>: > On Thu, 23 Jul 2009 12:34:43 +0530, vimal singh <vimal.newwork@gmail.com> wrote: >> > OK, but I still feel duplicating nand_ecc code is not so good. How >> > about splitting nand_correct_data into two parts? A pure calculation >> > function and a wrapper for mtd interface. Like this: >> >> But I do not see any thing extra, which you achieve from this >> wrapper... Is this a prototype, and you want to handle above scenario >> in this wrapper (calling 'nand_correct_data' multiple times based on >> something, probably 'ecc.bytes')? > > Yes, if we have __nand_correct_data(buf, read_ecc, calc_ecc, eccsize) > which do job based on its eccsize argument, the ecc.correct function > of the tmio_nand driver can be implemented like this: > > int tmio_nand_correct_data(struct mtd_info *mtd, unsigned char *buf, > unsigned char *read_ecc, unsigned char *calc_ecc) > { > int r0, r1; > > /* assume ecc.size = 512 and ecc.bytes = 6 */ > r0 = __nand_correct_data(buf, read_ecc, calc_ecc, 256); > if (r0 < 0) > return r0; > r1 = __nand_correct_data(buf + 256, read_ecc + 3, calc_ecc + 3, 256); > if (r1 < 0) > return r1; > return r0 | r1; > } > > Note that this is not tested at all since I do not have tmio device. This seems to be the thing that was initially ment by Zaurus kernels, so this if Ian doesn't object, Acked-by: Dmitry Eremin-Solenikov <dbaryshkov@gmail.com>
On Thu, Jul 23, 2009 at 8:41 PM, Atsushi Nemoto<anemo@mba.ocn.ne.jp> wrote: > On Thu, 23 Jul 2009 12:34:43 +0530, vimal singh <vimal.newwork@gmail.com> wrote: >> > OK, but I still feel duplicating nand_ecc code is not so good. How >> > about splitting nand_correct_data into two parts? A pure calculation >> > function and a wrapper for mtd interface. Like this: >> >> But I do not see any thing extra, which you achieve from this >> wrapper... Is this a prototype, and you want to handle above scenario >> in this wrapper (calling 'nand_correct_data' multiple times based on >> something, probably 'ecc.bytes')? > > Yes, if we have __nand_correct_data(buf, read_ecc, calc_ecc, eccsize) > which do job based on its eccsize argument, the ecc.correct function > of the tmio_nand driver can be implemented like this: > > int tmio_nand_correct_data(struct mtd_info *mtd, unsigned char *buf, > unsigned char *read_ecc, unsigned char *calc_ecc) > { > int r0, r1; > > /* assume ecc.size = 512 and ecc.bytes = 6 */ > r0 = __nand_correct_data(buf, read_ecc, calc_ecc, 256); > if (r0 < 0) > return r0; > r1 = __nand_correct_data(buf + 256, read_ecc + 3, calc_ecc + 3, 256); > if (r1 < 0) > return r1; > return r0 | r1; Better 'r0 + r1', returned value should represent number of error bits corrected. After fixing above comment: Acked-by: Vimal Singh <vimalsingh@ti.com> --- Regards, \/ | |\/| /-\ |_ ____ __o ------ -\<, ----- ( )/ ( )
diff --git a/drivers/mtd/nand/nand_ecc.c b/drivers/mtd/nand/nand_ecc.c index c0cb87d..3920896 100644 --- a/drivers/mtd/nand/nand_ecc.c +++ b/drivers/mtd/nand/nand_ecc.c @@ -425,14 +425,14 @@ EXPORT_SYMBOL(nand_calculate_ecc); * * Detect and correct a 1 bit error for 256/512 byte block */ -int nand_correct_data(struct mtd_info *mtd, unsigned char *buf, - unsigned char *read_ecc, unsigned char *calc_ecc) +int __nand_correct_data(unsigned char *buf, + unsigned char *read_ecc, unsigned char *calc_ecc, + unsigned int eccsize) { unsigned char b0, b1, b2, bit_addr; unsigned int byte_addr; /* 256 or 512 bytes/ecc */ - const uint32_t eccsize_mult = - (((struct nand_chip *)mtd->priv)->ecc.size) >> 8; + const uint32_t eccsize_mult = eccsize >> 8; /* * b0 to b2 indicate which bit is faulty (if any) @@ -495,6 +495,16 @@ int nand_correct_data(struct mtd_info *mtd, unsigned char *buf, printk(KERN_ERR "uncorrectable error : "); return -1; } +EXPORT_SYMBOL(__nand_correct_data); + +int nand_correct_data(struct mtd_info *mtd, unsigned char *buf, + unsigned char *read_ecc, unsigned char *calc_ecc) +{ + const uint32_t eccsize_mult = + (((struct nand_chip *)mtd->priv)->ecc.size) >> 8; + return __nand_correct_data(buf, read_ecc, calc_ecc, + ((struct nand_chip *)mtd->priv)->ecc.size); +} EXPORT_SYMBOL(nand_correct_data); MODULE_LICENSE("GPL");