[PATCH v2] mtd/docg3: fixes and cleanups

[PATCH v2] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

This patch takes into account checkpatch, sparse and ECC
comments.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

---
Since V1: Stephen Rothwell comment
---
 drivers/mtd/devices/docg3.c |   16 ++++++++--------
 drivers/mtd/devices/docg3.h |    1 +
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index d6c9599..867226e 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -61,7 +61,7 @@
  *
  */
 
-static unsigned int reliable_mode = 0;
+static unsigned int reliable_mode;
 module_param(reliable_mode, uint, 0);
 MODULE_PARM_DESC(reliable_mode, "Set the docg3 mode (0=normal MLC, 1=fast, "
 		 "2=reliable) : MLC normal operations are in normal mode");
@@ -573,7 +573,7 @@ static int doc_read_page_ecc_init(struct docg3 *docg3, int len)
  */
 static int doc_write_page_ecc_init(struct docg3 *docg3, int len)
 {
-	doc_writew(docg3, !DOC_ECCCONF0_READ_MODE
+	doc_writew(docg3, DOC_ECCCONF0_WRITE_MODE
 		   | DOC_ECCCONF0_BCH_ENABLE | DOC_ECCCONF0_HAMMING_ENABLE
 		   | (len & DOC_ECCCONF0_DATA_BYTES_MASK),
 		   DOC_ECCCONF0);
@@ -763,14 +763,14 @@ static void doc_write_page_putbytes(struct docg3 *docg3, int len,
 /**
  * doc_get_bch_hw_ecc - Get hardware calculated BCH ECC
  * @docg3: the device
- * @syns:  the array of 7 integers where the hardware ecc will be stored
+ * @hwecc:  the array of 7 integers where the hardware ecc will be stored
  */
-static void doc_get_bch_hw_ecc(struct docg3 *docg3, u8 *syns)
+static void doc_get_bch_hw_ecc(struct docg3 *docg3, u8 *hwecc)
 {
 	int i;
 
 	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
-		syns[i] = doc_register_readb(docg3, DOC_BCH_HW_ECC(i));
+		hwecc[i] = doc_register_readb(docg3, DOC_BCH_HW_ECC(i));
 }
 
 /**
@@ -1248,7 +1248,7 @@ static int doc_write_page(struct docg3 *docg3, loff_t to, const u_char *buf,
 			  const u_char *oob, int autoecc)
 {
 	int block0, block1, page, ret, ofs = 0;
-	u8 syn[DOC_ECC_BCH_SIZE], hamming;
+	u8 hwecc[DOC_ECC_BCH_SIZE], hamming;
 
 	doc_dbg("doc_write_page(to=%lld)\n", to);
 	calc_block_sector(to, &block0, &block1, &page, &ofs, docg3->reliable);
@@ -1278,8 +1278,8 @@ static int doc_write_page(struct docg3 *docg3, loff_t to, const u_char *buf,
 					&hamming);
 		doc_delay(docg3, 2);
 
-		doc_get_bch_hw_ecc(docg3, syn);
-		doc_write_page_putbytes(docg3, DOC_LAYOUT_OOB_BCH_SZ, syn);
+		doc_get_bch_hw_ecc(docg3, hwecc);
+		doc_write_page_putbytes(docg3, DOC_LAYOUT_OOB_BCH_SZ, hwecc);
 		doc_delay(docg3, 2);
 
 		doc_write_page_putbytes(docg3, DOC_LAYOUT_OOB_UNUSED_SZ, oob);
diff --git a/drivers/mtd/devices/docg3.h b/drivers/mtd/devices/docg3.h
index b1ae919..db0da43 100644
--- a/drivers/mtd/devices/docg3.h
+++ b/drivers/mtd/devices/docg3.h
@@ -193,6 +193,7 @@
 /*
  * Flash register : DOC_ECCCONF0
  */
+#define DOC_ECCCONF0_WRITE_MODE		0x0000
 #define DOC_ECCCONF0_READ_MODE		0x8000
 #define DOC_ECCCONF0_AUTO_ECC_ENABLE	0x4000
 #define DOC_ECCCONF0_HAMMING_ENABLE	0x1000
-- 
1.7.5.4

Re: [PATCH v2] mtd/docg3: fixes and cleanups

[Artem Bityutskiy]

On Tue, 2011-11-29 at 23:09 +0100, Robert Jarzmik wrote:
> This patch takes into account checkpatch, sparse and ECC
> comments.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Could you please send a patch which cleanly applies to l2-mtd-2.6.git -
this does not. This one does not apply both before and after Dan's
patch.

Artem.

Re: [PATCH v2] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> On Tue, 2011-11-29 at 23:09 +0100, Robert Jarzmik wrote:
>> This patch takes into account checkpatch, sparse and ECC
>> comments.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Could you please send a patch which cleanly applies to l2-mtd-2.6.git -
> this does not. This one does not apply both before and after Dan's
> patch.

Yes, of course. I don't know what happened there.
The v3 in a next mail (which is what v2 should have been).

Cheers.

--
Robert

[PATCH v3] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

This patch takes into account checkpatch, sparse and ECC
comments.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

---
Since V1: Stephen Rothwell comment
Since V2: rebased on
git://git.infradead.org/users/dedekind/l2-mtd-2.6.git,
commit id cb6ef775e498d24e3686a37108f1cea4bebed896
---
 drivers/mtd/devices/docg3.c |   26 +++++++++++++-------------
 drivers/mtd/devices/docg3.h |    3 ++-
 2 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index f7490a014..2a32072 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -61,7 +61,7 @@
  *
  */
 
-static unsigned int reliable_mode = 0;
+static unsigned int reliable_mode;
 module_param(reliable_mode, uint, 0);
 MODULE_PARM_DESC(reliable_mode, "Set the docg3 mode (0=normal MLC, 1=fast, "
 		 "2=reliable) : MLC normal operations are in normal mode");
@@ -546,7 +546,7 @@ out:
  * @len: the number of bytes covered by the ECC (BCH covered)
  *
  * The function does initialize the hardware ECC engine to compute the Hamming
- * ECC (on 1 byte) and the BCH Syndroms (on 7 bytes).
+ * ECC (on 1 byte) and the BCH hardware ECC (on 7 bytes).
  *
  * Return 0 if succeeded, -EIO on error
  */
@@ -567,13 +567,13 @@ static int doc_read_page_ecc_init(struct docg3 *docg3, int len)
  * @len: the number of bytes covered by the ECC (BCH covered)
  *
  * The function does initialize the hardware ECC engine to compute the Hamming
- * ECC (on 1 byte) and the BCH Syndroms (on 7 bytes).
+ * ECC (on 1 byte) and the BCH hardware ECC (on 7 bytes).
  *
  * Return 0 if succeeded, -EIO on error
  */
 static int doc_write_page_ecc_init(struct docg3 *docg3, int len)
 {
-	doc_writew(docg3, !DOC_ECCCONF0_READ_MODE
+	doc_writew(docg3, DOC_ECCCONF0_WRITE_MODE
 		   | DOC_ECCCONF0_BCH_ENABLE | DOC_ECCCONF0_HAMMING_ENABLE
 		   | (len & DOC_ECCCONF0_DATA_BYTES_MASK),
 		   DOC_ECCCONF0);
@@ -614,7 +614,7 @@ static void doc_hamming_ecc_init(struct docg3 *docg3, int nb_bytes)
 }
 
 /**
- * doc_correct_data - Fix if need be read data from flash
+ * doc_ecc_bch_fix_data - Fix if need be read data from flash
  * @docg3: the device
  * @buf: the buffer of read data (512 + 7 + 1 bytes)
  * @hwecc: the hardware calculated ECC.
@@ -761,16 +761,16 @@ static void doc_write_page_putbytes(struct docg3 *docg3, int len,
 }
 
 /**
- * doc_get_hw_bch_syndroms - Get hardware calculated BCH syndroms
+ * doc_get_bch_hw_ecc - Get hardware calculated BCH ECC
  * @docg3: the device
- * @syns:  the array of 7 integers where the syndroms will be stored
+ * @hwecc:  the array of 7 integers where the hardware ecc will be stored
  */
-static void doc_get_hw_bch_syndroms(struct docg3 *docg3, u8 *syns)
+static void doc_get_bch_hw_ecc(struct docg3 *docg3, u8 *hwecc)
 {
 	int i;
 
 	for (i = 0; i < DOC_ECC_BCH_SIZE; i++)
-		syns[i] = doc_register_readb(docg3, DOC_BCH_SYNDROM(i));
+		hwecc[i] = doc_register_readb(docg3, DOC_BCH_HW_ECC(i));
 }
 
 /**
@@ -904,7 +904,7 @@ static int doc_read_oob(struct mtd_info *mtd, loff_t from,
 		doc_read_page_getbytes(docg3, DOC_LAYOUT_OOB_SIZE - nboob,
 				       NULL, 0);
 
-		doc_get_hw_bch_syndroms(docg3, hwecc);
+		doc_get_bch_hw_ecc(docg3, hwecc);
 		eccconf1 = doc_register_readb(docg3, DOC_ECCCONF1);
 
 		if (nboob >= DOC_LAYOUT_OOB_SIZE) {
@@ -1248,7 +1248,7 @@ static int doc_write_page(struct docg3 *docg3, loff_t to, const u_char *buf,
 			  const u_char *oob, int autoecc)
 {
 	int block0, block1, page, ret, ofs = 0;
-	u8 syn[DOC_ECC_BCH_SIZE], hamming;
+	u8 hwecc[DOC_ECC_BCH_SIZE], hamming;
 
 	doc_dbg("doc_write_page(to=%lld)\n", to);
 	calc_block_sector(to, &block0, &block1, &page, &ofs, docg3->reliable);
@@ -1278,8 +1278,8 @@ static int doc_write_page(struct docg3 *docg3, loff_t to, const u_char *buf,
 					&hamming);
 		doc_delay(docg3, 2);
 
-		doc_get_hw_bch_syndroms(docg3, syn);
-		doc_write_page_putbytes(docg3, DOC_LAYOUT_OOB_BCH_SZ, syn);
+		doc_get_bch_hw_ecc(docg3, hwecc);
+		doc_write_page_putbytes(docg3, DOC_LAYOUT_OOB_BCH_SZ, hwecc);
 		doc_delay(docg3, 2);
 
 		doc_write_page_putbytes(docg3, DOC_LAYOUT_OOB_UNUSED_SZ, oob);
diff --git a/drivers/mtd/devices/docg3.h b/drivers/mtd/devices/docg3.h
index a349915..db0da43 100644
--- a/drivers/mtd/devices/docg3.h
+++ b/drivers/mtd/devices/docg3.h
@@ -115,7 +115,7 @@
 #define DOC_ECCCONF1			0x1042
 #define DOC_ECCPRESET			0x1044
 #define DOC_HAMMINGPARITY		0x1046
-#define DOC_BCH_SYNDROM(idx)		(0x1048 + (idx << 0))
+#define DOC_BCH_HW_ECC(idx)		(0x1048 + idx)
 
 #define DOC_PROTECTION			0x1056
 #define DOC_DPS0_KEY			0x105c
@@ -193,6 +193,7 @@
 /*
  * Flash register : DOC_ECCCONF0
  */
+#define DOC_ECCCONF0_WRITE_MODE		0x0000
 #define DOC_ECCCONF0_READ_MODE		0x8000
 #define DOC_ECCCONF0_AUTO_ECC_ENABLE	0x4000
 #define DOC_ECCCONF0_HAMMING_ENABLE	0x1000
-- 
1.7.5.4

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Artem Bityutskiy]

On Fri, 2011-12-02 at 20:00 +0100, Robert Jarzmik wrote:
> This patch takes into account checkpatch, sparse and ECC
> comments.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>

Pushed to l2-mtd-2.6.git, thanks. But I still see this warning when
I compile on x86_64:

drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined but not used [-Wunused-function]

Artem.

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> Pushed to l2-mtd-2.6.git, thanks. But I still see this warning when
> I compile on x86_64:
>
> drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined
> but not used [-Wunused-function]
Yes, I know. As I have said previously to Stephen, this one will take a little
bit more time to fix.

I'm working on it, and as soon as I have something, I'll submit.

Cheers.

-- 
Robert

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> On Fri, 2011-12-02 at 20:00 +0100, Robert Jarzmik wrote:
>> This patch takes into account checkpatch, sparse and ECC
>> comments.
>> 
>> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
>
> Pushed to l2-mtd-2.6.git, thanks. But I still see this warning when
> I compile on x86_64:
>
> drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined
> but not used [-Wunused-function]

Hi Artem,

I have found no clean and good way to handle the erase count.  As a fallback,
please accept the following patch which removes the unused function. It's kind
of a "looser" approach, but that's the best I can do by now.

The patch is based on your current l2-tree (commit
af64b37f63189cef7575fb3f8e670df767c3e15b "mtdoops: fix the oops_page_used array
size").

Cheers.

--
Robert

>From c87ece988e535905702b40460c6685984f08db88 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Fri, 9 Dec 2011 17:02:49 +0100
Subject: [PATCH] mtd: docg3: remove unused function

As the MTD api has no use for the number of erase cycles
each block has endured, remove the function which calculated
that value.

If one day MTD api finds it usefull for wear levelling
algorithms to have this information, the function should be
put back in place.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/devices/docg3.c |   42 ------------------------------------------
 1 files changed, 0 insertions(+), 42 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 2a32072..f05c5bc 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1038,48 +1038,6 @@ static int doc_block_isbad(struct mtd_info *mtd, loff_t from)
 }
 
 /**
- * doc_get_erase_count - Get block erase count
- * @docg3: the device
- * @from: the offset in which the block is.
- *
- * Get the number of times a block was erased. The number is the maximum of
- * erase times between first and second plane (which should be equal normally).
- *
- * Returns The number of erases, or -EINVAL or -EIO on error.
- */
-static int doc_get_erase_count(struct docg3 *docg3, loff_t from)
-{
-	u8 buf[DOC_LAYOUT_WEAR_SIZE];
-	int ret, plane1_erase_count, plane2_erase_count;
-	int block0, block1, page, ofs;
-
-	doc_dbg("doc_get_erase_count(from=%lld, buf=%p)\n", from, buf);
-	if (from % DOC_LAYOUT_PAGE_SIZE)
-		return -EINVAL;
-	calc_block_sector(from, &block0, &block1, &page, &ofs, docg3->reliable);
-	if (block1 > docg3->max_block)
-		return -EINVAL;
-
-	ret = doc_reset_seq(docg3);
-	if (!ret)
-		ret = doc_read_page_prepare(docg3, block0, block1, page,
-					    ofs + DOC_LAYOUT_WEAR_OFFSET);
-	if (!ret)
-		ret = doc_read_page_getbytes(docg3, DOC_LAYOUT_WEAR_SIZE,
-					     buf, 1);
-	doc_read_page_finish(docg3);
-
-	if (ret || (buf[0] != DOC_ERASE_MARK) || (buf[2] != DOC_ERASE_MARK))
-		return -EIO;
-	plane1_erase_count = (u8)(~buf[1]) | ((u8)(~buf[4]) << 8)
-		| ((u8)(~buf[5]) << 16);
-	plane2_erase_count = (u8)(~buf[3]) | ((u8)(~buf[6]) << 8)
-		| ((u8)(~buf[7]) << 16);
-
-	return max(plane1_erase_count, plane2_erase_count);
-}
-
-/**
  * doc_get_op_status - get erase/write operation status
  * @docg3: the device
  *
-- 
1.7.5.4

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Artem Bityutskiy]

On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
> Artem Bityutskiy <dedekind1@gmail.com> writes:
> 
> > On Fri, 2011-12-02 at 20:00 +0100, Robert Jarzmik wrote:
> >> This patch takes into account checkpatch, sparse and ECC
> >> comments.
> >> 
> >> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> >
> > Pushed to l2-mtd-2.6.git, thanks. But I still see this warning when
> > I compile on x86_64:
> >
> > drivers/mtd/devices/docg3.c:1050:12: warning: ‘doc_get_erase_count’ defined
> > but not used [-Wunused-function]
> 
> Hi Artem,
> 
> I have found no clean and good way to handle the erase count.  As a fallback,
> please accept the following patch which removes the unused function. It's kind
> of a "looser" approach, but that's the best I can do by now.

What do you mean by "handle" ? Is the problem that docg3 maintains
per-eraseblock erase-counters and you do not know how to use them? May
be it is better to just embrace this function with "#ifdef 0" and add a
comment that we can do this but do not have a good use for this?

Artem.

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

Artem Bityutskiy <dedekind1@gmail.com> writes:

> On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
> What do you mean by "handle" ? Is the problem that docg3 maintains
> per-eraseblock erase-counters and you do not know how to use them?
Yes, exactly, docg3 maintains that counter for each block.

I thought that upper layers (UBI,...) could use both the erase counter and the
ECC number of flipped bits to select which block to erase next. After a quick
glance, I don't think they can do it.

> May be it is better to just embrace this function with "#ifdef 0" and add a
> comment that we can do this but do not have a good use for this?
Ah yes, I didn't think about that possibility. It's looks better, I'll submit
another patch with a comment and the #if 0.

Cheers.

-- 
Robert

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Mike Dunn]

On 12/12/2011 02:03 PM, Robert Jarzmik wrote:
> Artem Bityutskiy <dedekind1@gmail.com> writes:
>
>> On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
>> What do you mean by "handle" ? Is the problem that docg3 maintains
>> per-eraseblock erase-counters and you do not know how to use them?
> Yes, exactly, docg3 maintains that counter for each block.
>
> I thought that upper layers (UBI,...) could use both the erase counter and the
> ECC number of flipped bits to select which block to erase next. After a quick
> glance, I don't think they can do it.


The erase counters are surely used by the TrueFFS library.  The diskonchip and
TrueFFS were both developed by M-Sys.  But since it's a unique feature of this
device, it would probably be impractical for ubi or mtd to use them.  I'm
impressed that you discovered them, though.  I had no clue that they existed on
the diskonchip P3.  They are probably on the G4 too.


>> May be it is better to just embrace this function with "#ifdef 0" and add a
>> comment that we can do this but do not have a good use for this?
> Ah yes, I didn't think about that possibility. It's looks better, I'll submit
> another patch with a comment and the #if 0.


Yeah, leave it in, commented out.  But don't submit a patch that produces a
compiler warning!

Thanks,
Mike

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Robert Jarzmik]

> Artem Bityutskiy <dedekind1@gmail.com> writes:
>
>> On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
>> May be it is better to just embrace this function with "#ifdef 0" and add a
>> comment that we can do this but do not have a good use for this?
> Ah yes, I didn't think about that possibility. It's looks better, I'll submit
> another patch with a comment and the #if 0.
How about the following patch ?

Cheers.

-- 
Robert

>From 75cb409a2a98070584cbf7a9f8ccfe621cf29018 Mon Sep 17 00:00:00 2001
From: Robert Jarzmik <robert.jarzmik@free.fr>
Date: Fri, 9 Dec 2011 17:02:49 +0100
Subject: [PATCH] mtd: docg3: remove unused function

As the MTD api has no use for the number of erase cycles
each block has endured, remove the function which calculated
that value.

If one day MTD api finds it usefull for wear levelling
algorithms to have this information, the function should be
put back in place.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/mtd/devices/docg3.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/mtd/devices/docg3.c b/drivers/mtd/devices/docg3.c
index 2a32072..22d5099 100644
--- a/drivers/mtd/devices/docg3.c
+++ b/drivers/mtd/devices/docg3.c
@@ -1037,6 +1037,7 @@ static int doc_block_isbad(struct mtd_info *mtd, loff_t from)
 	return !is_good;
 }
 
+#if 0
 /**
  * doc_get_erase_count - Get block erase count
  * @docg3: the device
@@ -1078,6 +1079,7 @@ static int doc_get_erase_count(struct docg3 *docg3, loff_t from)
 
 	return max(plane1_erase_count, plane2_erase_count);
 }
+#endif
 
 /**
  * doc_get_op_status - get erase/write operation status
-- 
1.7.5.4

Re: [PATCH v3] mtd/docg3: fixes and cleanups

[Artem Bityutskiy]

[-- Attachment #1: Type: text/plain, Size: 575 bytes --]

On Fri, 2011-12-16 at 23:25 +0100, Robert Jarzmik wrote:
> > Artem Bityutskiy <dedekind1@gmail.com> writes:
> >
> >> On Fri, 2011-12-09 at 17:13 +0100, Robert Jarzmik wrote:
> >> May be it is better to just embrace this function with "#ifdef 0" and add a
> >> comment that we can do this but do not have a good use for this?
> > Ah yes, I didn't think about that possibility. It's looks better, I'll submit
> > another patch with a comment and the #if 0.
> How about the following patch ?

Pushed to l2-mtd-2.6.git, thanks!

-- 
Best Regards,
Artem Bityutskiy

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 836 bytes --]