mtd: gpmi: issue with raw access support

mtd: gpmi: issue with raw access support

[Han Xu]

Hi Boris,

There is an issue on Kernel 4.1 with your gpmi raw access support
patch. I can understand with your implementation all data are stored
in BCH layout, even without HW ECC. But for NAND boot function on i.MX
family, it requires to write BOOT CONFIGURATION DATA at the beginning
of NAND chip, so ROM code could understand how to configure BCH
register to read data with proper ECC handling.

All these BOOT CONFIURATION DATA must be written with proper ROM
defined format, including data layout, parity checking algorithm( NOT
BCH ECC for some platforms). A user-space tool, named kobs-ng was
provided to generate all necessary data, and it called the mtd raw
functions to write the UNFORMATTED data to NAND chip.

With the new patch, all NAND boot function on kernel 4.1 failed since
ROM couldn't read the proper configuration. To solve the issue, I am
planning to export a switch in debugfs to let the user applications to
choose raw access mode, unformatted mode or BCH layout one, while I
got stuck.

The problem is how could I dynamically change the raw functions from
the new ones you provided to the default ones in nand_base. It more or
less a hacking either getting the pointer of functions
nand_read_page_raw /nand_write_page_raw or copy-pasting the implement
of these two functions to gpmi layer. So I was wondering if you have
any better idea for this issue? Thanks.

Re: mtd: gpmi: issue with raw access support

[Boris Brezillon]

Hi,

On Mon, 25 Jan 2016 15:53:23 -0600
Han Xu <xhnjupt@gmail.com> wrote:

> Hi Boris,
> 
> There is an issue on Kernel 4.1 with your gpmi raw access support
> patch. I can understand with your implementation all data are stored
> in BCH layout, even without HW ECC. But for NAND boot function on i.MX
> family, it requires to write BOOT CONFIGURATION DATA at the beginning
> of NAND chip, so ROM code could understand how to configure BCH
> register to read data with proper ECC handling.
> 
> All these BOOT CONFIURATION DATA must be written with proper ROM
> defined format, including data layout, parity checking algorithm( NOT
> BCH ECC for some platforms). A user-space tool, named kobs-ng was
> provided to generate all necessary data, and it called the mtd raw
> functions to write the UNFORMATTED data to NAND chip.

Yes, I had the same problem, actually I made 2 patches for kobs-ng
(I'll send them in reply to this email), but since the project seemed to
be unmaintained, I didn't submit it.

> 
> With the new patch, all NAND boot function on kernel 4.1 failed since
> ROM couldn't read the proper configuration. To solve the issue, I am
> planning to export a switch in debugfs to let the user applications to
> choose raw access mode, unformatted mode or BCH layout one, while I
> got stuck.

Hm, providing two different set of raw access functions is a bad idea
IMO. If we had a per-partition configuration infrastructure that could
be done, but so far we don't have that.

> 
> The problem is how could I dynamically change the raw functions from
> the new ones you provided to the default ones in nand_base. It more or
> less a hacking either getting the pointer of functions
> nand_read_page_raw /nand_write_page_raw or copy-pasting the implement
> of these two functions to gpmi layer. So I was wondering if you have
> any better idea for this issue? Thanks.

I recommend to patch kobs-ng, and let him reorganize the data to make
the ROM firmware happy. That's what I did in my patches, but IIRC, I
only took the "aligned ECC bytes" case into consideration, and this is
not necessarily true depending on the ECC config, so you'll have to
rework my patches to address that.

Best Regards,

Boris

-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[PATCH] remove deprecated oobinfo retrieval

[Boris Brezillon]

From: Boris BREZILLON <boris.brezillon@free-electrons.com>

This remove OOB info retrieval, which is not used anywhere else in the code
and can generate errors when called on newer NAND that have more than 32
bytes of ECC or 16 bytes available for user usage in the OOB area.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 src/mtd.c | 8 --------
 src/mtd.h | 1 -
 2 files changed, 9 deletions(-)

diff --git a/src/mtd.c b/src/mtd.c
index 02af33b..cf8428a 100644
--- a/src/mtd.c
+++ b/src/mtd.c
@@ -700,14 +700,6 @@ struct mtd_data *mtd_open(const struct mtd_config *cfg, int flags)
 			goto out;
 		}
 
-		/* keep original oobinfo */
-		r = ioctl(mp->fd, MEMGETOOBSEL, &mp->old_oobinfo);
-		if (r != 0) {
-			fprintf(stderr, "mtd: device %s can't ioctl MEMGETOOBSEL: %d\n",
-					mp->name, r);
-			goto out;
-		}
-
 		/* get info about the mtd device (partition) */
 		r = ioctl(mp->fd, MEMGETINFO, miu);
 		if (r != 0) {
diff --git a/src/mtd.h b/src/mtd.h
index 99d7887..946a49c 100644
--- a/src/mtd.h
+++ b/src/mtd.h
@@ -112,7 +112,6 @@ struct mtd_part {
 	int nrbad;
 
         int oobinfochanged;
-	struct nand_oobinfo old_oobinfo;
 	int ecc;
 };
 
-- 
1.9.1

[PATCH] Adapt raw page accesses to match the new raw_read/write implementation

[Boris Brezillon]

From: Boris BREZILLON <boris.brezillon@free-electrons.com>

The old raw access implementation (in GPMI driver) was considering that
data and OOB data were separated in their respective regions (the data
area and the OOB area of the page), which is not true.
They are actually interleaved this way:

METADATA + ((DATA + ECCBYTES) * N)

The new raw access implementation (in the GPMI driver) is hiding this weird
layout to MTD users by exposing a more common layout:

DATA + METADATA + (N * ECCBYTES)

Here METADATA + (N * ECCBYTES) are exposed as if they were stored in the
OOB area.

Unfortunately kobs-ng rely on this weird layout when accessing the NAND
in raw mode.
This patch take the new layout into account.

Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
---
 src/mtd.c | 97 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 92 insertions(+), 5 deletions(-)

diff --git a/src/mtd.c b/src/mtd.c
index 02af33b..f6fa962 100644
--- a/src/mtd.c
+++ b/src/mtd.c
@@ -254,14 +254,26 @@ int mtd_read_page(struct mtd_data *md, int chip, loff_t ofs, int ecc)
 
 	mtd_set_ecc_mode(md, ecc);
 
-	data = md->buf;
+	if (ecc) {
+		data = md->buf;
+	} else {
+		data = malloc(mtd_writesize(md) + mtd_oobsize(md));
+		if (!data) {
+			fprintf(stderr, "mtd: %s failed to allocate buffer\n", __func__);
+			return -1;
+		}
+	}
+
 	oobdata = data + mtd_writesize(md);
 
 	/* make sure it's aligned to a page */
-	if ((ofs % mtd_writesize(md)) != 0)
+	if ((ofs % mtd_writesize(md)) != 0) {
+		if (!ecc)
+			free(data);
 		return -1;
+	}
 
-	memset(md->buf, 0, mtd_writesize(md) + mtd_oobsize(md));
+	memset(data, 0, mtd_writesize(md) + mtd_oobsize(md));
 
 	size = mtd_writesize(md);
 
@@ -269,6 +281,33 @@ int mtd_read_page(struct mtd_data *md, int chip, loff_t ofs, int ecc)
 		r = pread(md->part[chip].fd, data, size, ofs);
 	} while (r == -1 && (errno == EAGAIN || errno == EBUSY));
 
+	if (!ecc) {
+		int i;
+		struct nfc_geometry *nfc_geo = &md->nfc_geometry;
+		int eccbytes = ((nfc_geo->ecc_strength * nfc_geo->gf_len) + 7) / 8;
+		int chunksize = nfc_geo->ecc_chunk_size_in_bytes;
+		int dst_offset = 0;
+
+		memcpy(md->buf, oobdata, nfc_geo->metadata_size_in_bytes);
+		dst_offset += nfc_geo->metadata_size_in_bytes;
+		for (i = 0; i < nfc_geo->ecc_chunk_count; i++) {
+			memcpy(md->buf + dst_offset, data + (i * chunksize), chunksize);
+			dst_offset += chunksize;
+			memcpy(md->buf + dst_offset,
+			       oobdata + nfc_geo->metadata_size_in_bytes + (i * eccbytes),
+			       eccbytes);
+			dst_offset += eccbytes;
+		}
+
+		if (mtd_writesize(md) + mtd_oobsize(md) > dst_offset) {
+			memcpy(md->buf + dst_offset,
+			       oobdata + nfc_geo->metadata_size_in_bytes + (i * eccbytes),
+			       mtd_writesize(md) + mtd_oobsize(md) - dst_offset);
+		}
+
+		free(data);
+	}
+
 	/* end of partition? */
 	if (r == 0)
 		return 0;
@@ -329,23 +368,71 @@ int mtd_write_page(struct mtd_data *md, int chip, loff_t ofs, int ecc)
 	int r;
 	const void *data;
 	const void *oobdata;
+	struct mtd_write_req ops;
 
 	mtd_set_ecc_mode(md, ecc);
 
-	data = md->buf;
-	oobdata = data + mtd_oobsize(md);
+	if (ecc) {
+		data = md->buf;
+		oobdata = data + mtd_writesize(md);
+	} else {
+		int i;
+		struct nfc_geometry *nfc_geo = &md->nfc_geometry;
+		int eccbytes = ((nfc_geo->ecc_strength * nfc_geo->gf_len) + 7) / 8;
+		int chunksize = nfc_geo->ecc_chunk_size_in_bytes;
+		int src_offset = 0;
+
+		data = malloc(mtd_writesize(md) + mtd_oobsize(md));
+		if (!data) {
+			fprintf(stderr, "mtd: %s failed to allocate buffer\n", __func__);
+			return -1;
+		}
+		oobdata = data + mtd_writesize(md);
+		memset(data, 0, mtd_writesize(md) + mtd_oobsize(md));
+		memcpy(oobdata, md->buf, nfc_geo->metadata_size_in_bytes);
+		src_offset += nfc_geo->metadata_size_in_bytes;
+		for (i = 0; i < nfc_geo->ecc_chunk_count; i++) {
+			memcpy(data + (i * chunksize), md->buf + src_offset, chunksize);
+			src_offset += chunksize;
+			memcpy(oobdata + nfc_geo->metadata_size_in_bytes + (i * eccbytes),
+			       md->buf + src_offset, eccbytes);
+			src_offset += eccbytes;
+		}
+
+		if (mtd_writesize(md) + mtd_oobsize(md) > src_offset) {
+			memcpy(oobdata + nfc_geo->metadata_size_in_bytes + (i * eccbytes),
+			       md->buf + src_offset,
+			       mtd_writesize(md) + mtd_oobsize(md) - src_offset);
+		}
+	}
 
 	/* make sure it's aligned to a page */
 	if ((ofs % mtd_writesize(md)) != 0) {
 		fprintf(stderr, "mtd: %s failed\n", __func__);
+		if (!ecc)
+			free(data);
 		return -1;
 	}
 
 	size = mtd_writesize(md);
 
+	ops.start = ofs;
+	ops.len = size;
+	ops.ooblen = ecc ? 0 : mtd_oobsize(md);
+	ops.usr_oob = (uint64_t)(unsigned long)oobdata;
+	ops.usr_data = (uint64_t)(unsigned long)data;
+	ops.mode = ecc ? MTD_OPS_AUTO_OOB : MTD_OPS_RAW;
+	r = ioctl(md->part[chip].fd, MEMWRITE, &ops);
+	if (!r)
+		r = size;
+	/*
 	do {
 		r = pwrite(md->part[chip].fd, data, size, ofs);
 	} while (r == -1 && (errno == EAGAIN || errno == EBUSY));
+	*/
+
+	if (!ecc)
+		free(data);
 
 	/* end of partition? */
 	if (r == 0) {
-- 
1.9.1

Re: [PATCH] remove deprecated oobinfo retrieval

[Brian Norris]

On Tue, Jan 26, 2016 at 07:42:16AM +0100, Boris Brezillon wrote:
> From: Boris BREZILLON <boris.brezillon@free-electrons.com>
> 
> This remove OOB info retrieval, which is not used anywhere else in the code
> and can generate errors when called on newer NAND that have more than 32
> bytes of ECC or 16 bytes available for user usage in the OOB area.
> 
> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> ---
>  src/mtd.c | 8 --------
>  src/mtd.h | 1 -
>  2 files changed, 9 deletions(-)

What source are you trying to patch? Doesn't look like Linux or
mtd-utils.

Re: [PATCH] remove deprecated oobinfo retrieval

[Han Xu]

No, it's a user-space tool named kobs-ng.

On Tue, Jan 26, 2016 at 1:59 PM, Brian Norris
<computersforpeace@gmail.com> wrote:
> On Tue, Jan 26, 2016 at 07:42:16AM +0100, Boris Brezillon wrote:
>> From: Boris BREZILLON <boris.brezillon@free-electrons.com>
>>
>> This remove OOB info retrieval, which is not used anywhere else in the code
>> and can generate errors when called on newer NAND that have more than 32
>> bytes of ECC or 16 bytes available for user usage in the OOB area.
>>
>> Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
>> ---
>>  src/mtd.c | 8 --------
>>  src/mtd.h | 1 -
>>  2 files changed, 9 deletions(-)
>
> What source are you trying to patch? Doesn't look like Linux or
> mtd-utils.

Re: [PATCH] remove deprecated oobinfo retrieval

[Boris Brezillon]

On Tue, 26 Jan 2016 11:59:41 -0800
Brian Norris <computersforpeace@gmail.com> wrote:

> On Tue, Jan 26, 2016 at 07:42:16AM +0100, Boris Brezillon wrote:
> > From: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > 
> > This remove OOB info retrieval, which is not used anywhere else in the code
> > and can generate errors when called on newer NAND that have more than 32
> > bytes of ECC or 16 bytes available for user usage in the OOB area.
> > 
> > Signed-off-by: Boris BREZILLON <boris.brezillon@free-electrons.com>
> > ---
> >  src/mtd.c | 8 --------
> >  src/mtd.h | 1 -
> >  2 files changed, 9 deletions(-)
> 
> What source are you trying to patch? Doesn't look like Linux or
> mtd-utils.

Nope, those patches apply on the kobs-ng project (BTW, didn't find any
git repo hosting this project).
I realize I shouldn't send those patches to the MTD ML (I just kept
the @ Han Xu sent his request to, including the ML).

Sorry for the inconvenience.


-- 
Boris Brezillon, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com