Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
On Thu, Nov 15, 2012 at 02:25:23PM +0100, Andreas Bießmann wrote: Dear Ilya Yanok, On 07.11.2012 00:06, Ilya Yanok wrote: From: Mansoor Ahamed mansoor.aha...@ti.com This patch adds support for BCH8 error correction code to omap_gpmc driver. We use GPMC to generate codes/syndromes but we need ELM to find error locations from given syndrome. first of all, I wonder why this is so different than the kernel implementation for BCH. I mean the API (and content) of this and commit 8d602cf50d3bba864bc1438f486b626df69c87b3 mainline linux seems to differ. The main question coming to mind is: Is the resulting OOB layout compatible then? I think this has been mostly addressed now, but for clarity: - We do NOT want to have 1 layout used per NAND chip unless we must (historically we did because we had ROM that couldn't use 1bit ECC). - We DO want to utilize the HW as this is the only easy way to get a match with the BCH constants the ROM uses. - There are corresponding kernel patches already posted and working their way along. -- Tom signature.asc Description: Digital signature ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
On 21.11.12 18:04, Tom Rini wrote: On Thu, Nov 15, 2012 at 02:25:23PM +0100, Andreas Bießmann wrote: Dear Ilya Yanok, On 07.11.2012 00:06, Ilya Yanok wrote: From: Mansoor Ahamed mansoor.aha...@ti.com This patch adds support for BCH8 error correction code to omap_gpmc driver. We use GPMC to generate codes/syndromes but we need ELM to find error locations from given syndrome. first of all, I wonder why this is so different than the kernel implementation for BCH. I mean the API (and content) of this and commit 8d602cf50d3bba864bc1438f486b626df69c87b3 mainline linux seems to differ. The main question coming to mind is: Is the resulting OOB layout compatible then? I think this has been mostly addressed now, but for clarity: - We do NOT want to have 1 layout used per NAND chip unless we must (historically we did because we had ROM that couldn't use 1bit ECC). - We DO want to utilize the HW as this is the only easy way to get a match with the BCH constants the ROM uses. - There are corresponding kernel patches already posted and working their way along. I'm fine with all these three points. My question came up when I looked into this deeply the very first time cause BCH4/8 support was missing for OMAP35xx/AM37xx devices. I do have now working support for these (hw assisted BCH but sw correction) like the kernel does. It needs some final polishing however I will send it these days as RFC, would be great to get some feedback. Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
Dear Peter Korsgaard, On 16.11.12 16:59, Peter Korsgaard wrote: Andreas == Andreas Bießmann andreas.de...@googlemail.com writes: Hi, Please note that these patches are AM33XX-specific (as we are using ELM that, I think, just isn't available on OMAP3) so we use OOB layout that is compatible with AM33xx ROM boot code. Andreas You are right, ELM is not available in OMAP3 devices. It seems Andreas the ROM loader of these devices only support the 1-Bit Andreas Hamming, but is also different to the OOB layout used for the Andreas SW 1-bit hamming provided by the Kernel. So we get here a lot Andreas of different OOB layouts ... I wonder if we can stick to Andreas e.g. the generic SW BCH layout (of linux kernel) for all but Andreas the ROM partition (where the SPL is placed). So the SPL need Andreas to know just one mechanism but software modifying that place Andreas needs to know about the 'special' ROM layout. No, please not. Having more than 1 OOB layout on the same NAND device leads to all kind of complications. There has also been kernel patches posted for the ELM, so IMHO the only sane option for am33xx is BCH8 everywhere (with the ROM layout). Ok, I understand your point here. But there are device combinations out there that do not match! We have an AM37xx with some micron NAND that requires 1bit ECC for the first page (if less than 1000 erase cycle). But 4bit ECC for the rest of the device. With the 1bit for first page it does fit to the ROM requirements of that SoC but unfortunately we can not use the same OOB layout on the whole device, cause the ROM can only do the 1bit hamming ECC. I think there are other boards out there facing the same problem. I have to recheck for example the devkit8000 which is a mainlined development device some users based their products on (as we do). Actually, the only assumption the code does about the OOB layout is that ECC code occupies continuous area in the OOB. Andreas Well, but you have defined that for example it is written in Andreas big endian. Andreas I'm currently working on a omap3 enabled device that requires Andreas 4-bit ECC for all but the first block. And I'm searching for a Andreas clean solution that would be accepted mainline. I think it Andreas would be best to have the same OOB layout for the whole device Andreas but the SPL space (cause that needs to be read by ROM). The Andreas layout should be chosen at compile time of the SPL. What do Andreas you think about? So the only reason to not have the same OOB layout everywhere is because of ROM restrictions and that 1bit ECC isn't good enough anymore? That's my point. E.G. you actually would have prefered to use the ROM layout if it would have used something better like the am33xx ROM does. You are right. Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
Andreas == Andreas Bießmann andreas.de...@googlemail.com writes: Hi, No, please not. Having more than 1 OOB layout on the same NAND device leads to all kind of complications. There has also been kernel patches posted for the ELM, so IMHO the only sane option for am33xx is BCH8 everywhere (with the ROM layout). Andreas Ok, I understand your point here. But there are device Andreas combinations out there that do not match! We have an AM37xx Andreas with some micron NAND that requires 1bit ECC for the first Andreas page (if less than 1000 erase cycle). But 4bit ECC for the Andreas rest of the device. With the 1bit for first page it does fit Andreas to the ROM requirements of that SoC but unfortunately we can Andreas not use the same OOB layout on the whole device, cause the ROM Andreas can only do the 1bit hamming ECC. I think there are other Andreas boards out there facing the same problem. I have to recheck Andreas for example the devkit8000 which is a mainlined development Andreas device some users based their products on (as we do). Sure, reality tends to be more complicated than that. The different SoCs, Nand devices and projects have different requirements. Notice that this patch is specifically about the Error Location Module present on E.G. am33xx and omap4, where the ROM also uses BCH encoding. -- Bye, Peter Korsgaard ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
Dear Ilya Yanok, On 15.11.2012 21:08, Ilya Yanok wrote: Dear Andreas, On Thu, Nov 15, 2012 at 2:25 PM, Andreas Bießmann andreas.de...@googlemail.com mailto:andreas.de...@googlemail.com wrote: Dear Ilya Yanok, On 07.11.2012 00:06, Ilya Yanok wrote: From: Mansoor Ahamed mansoor.aha...@ti.com mailto:mansoor.aha...@ti.com This patch adds support for BCH8 error correction code to omap_gpmc driver. We use GPMC to generate codes/syndromes but we need ELM to find error locations from given syndrome. first of all, I wonder why this is so different than the kernel implementation for BCH. I mean the API (and content) of this and commit 8d602cf50d3bba864bc1438f486b626df69c87b3 mainline linux seems to differ. The main question coming to mind is: Is the resulting OOB layout compatible then? Please note that these patches are AM33XX-specific (as we are using ELM that, I think, just isn't available on OMAP3) so we use OOB layout that is compatible with AM33xx ROM boot code. You are right, ELM is not available in OMAP3 devices. It seems the ROM loader of these devices only support the 1-Bit Hamming, but is also different to the OOB layout used for the SW 1-bit hamming provided by the Kernel. So we get here a lot of different OOB layouts ... I wonder if we can stick to e.g. the generic SW BCH layout (of linux kernel) for all but the ROM partition (where the SPL is placed). So the SPL need to know just one mechanism but software modifying that place needs to know about the 'special' ROM layout. It's likely that this layout doesn't match with the current kernel layout as RBL uses strange 14th byte for BCH8 while only 13 bytes are needed. Sorry, what does RBL mean in that context? There are some patches for the kernel that make it use the same layout too but I don't if they are going to be accepted soon. Ok, that would be required to write the SPL to flash. Actually, the only assumption the code does about the OOB layout is that ECC code occupies continuous area in the OOB. Well, but you have defined that for example it is written in big endian. I'm currently working on a omap3 enabled device that requires 4-bit ECC for all but the first block. And I'm searching for a clean solution that would be accepted mainline. I think it would be best to have the same OOB layout for the whole device but the SPL space (cause that needs to be read by ROM). The layout should be chosen at compile time of the SPL. What do you think about? Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
Andreas == Andreas Bießmann andreas.de...@googlemail.com writes: Hi, Please note that these patches are AM33XX-specific (as we are using ELM that, I think, just isn't available on OMAP3) so we use OOB layout that is compatible with AM33xx ROM boot code. Andreas You are right, ELM is not available in OMAP3 devices. It seems Andreas the ROM loader of these devices only support the 1-Bit Andreas Hamming, but is also different to the OOB layout used for the Andreas SW 1-bit hamming provided by the Kernel. So we get here a lot Andreas of different OOB layouts ... I wonder if we can stick to Andreas e.g. the generic SW BCH layout (of linux kernel) for all but Andreas the ROM partition (where the SPL is placed). So the SPL need Andreas to know just one mechanism but software modifying that place Andreas needs to know about the 'special' ROM layout. No, please not. Having more than 1 OOB layout on the same NAND device leads to all kind of complications. There has also been kernel patches posted for the ELM, so IMHO the only sane option for am33xx is BCH8 everywhere (with the ROM layout). It's likely that this layout doesn't match with the current kernel layout as RBL uses strange 14th byte for BCH8 while only 13 bytes are needed. Andreas Sorry, what does RBL mean in that context? The ROM boot loader, E.G. the part loading the spl. Actually, the only assumption the code does about the OOB layout is that ECC code occupies continuous area in the OOB. Andreas Well, but you have defined that for example it is written in Andreas big endian. Andreas I'm currently working on a omap3 enabled device that requires Andreas 4-bit ECC for all but the first block. And I'm searching for a Andreas clean solution that would be accepted mainline. I think it Andreas would be best to have the same OOB layout for the whole device Andreas but the SPL space (cause that needs to be read by ROM). The Andreas layout should be chosen at compile time of the SPL. What do Andreas you think about? So the only reason to not have the same OOB layout everywhere is because of ROM restrictions and that 1bit ECC isn't good enough anymore? E.G. you actually would have prefered to use the ROM layout if it would have used something better like the am33xx ROM does. -- Bye, Peter Korsgaard ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
Dear Ilya Yanok, On 07.11.2012 00:06, Ilya Yanok wrote: From: Mansoor Ahamed mansoor.aha...@ti.com This patch adds support for BCH8 error correction code to omap_gpmc driver. We use GPMC to generate codes/syndromes but we need ELM to find error locations from given syndrome. first of all, I wonder why this is so different than the kernel implementation for BCH. I mean the API (and content) of this and commit 8d602cf50d3bba864bc1438f486b626df69c87b3 mainline linux seems to differ. The main question coming to mind is: Is the resulting OOB layout compatible then? Best regards Andreas Bießmann ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] [PATCH v2 6/8] omap_gpmc: BCH8 support (ELM based)
From: Mansoor Ahamed mansoor.aha...@ti.com
This patch adds support for BCH8 error correction code to omap_gpmc
driver. We use GPMC to generate codes/syndromes but we need ELM to find
error locations from given syndrome.
Signed-off-by: Mansoor Ahamed mansoor.aha...@ti.com
[ilya: merge it with omap_gpmc driver, some fixes and cleanup]
Signed-off-by: Ilya Yanok ilya.ya...@cogentembedded.com
---
drivers/mtd/nand/omap_gpmc.c | 403 +-
1 file changed, 402 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/omap_gpmc.c b/drivers/mtd/nand/omap_gpmc.c
index f1469d1..cee394e 100644
--- a/drivers/mtd/nand/omap_gpmc.c
+++ b/drivers/mtd/nand/omap_gpmc.c
@@ -29,6 +29,9 @@
#include linux/mtd/nand_ecc.h
#include linux/compiler.h
#include nand.h
+#ifdef CONFIG_AM33XX
+#include asm/arch/elm.h
+#endif
static uint8_t cs;
static __maybe_unused struct nand_ecclayout hw_nand_oob =
@@ -234,6 +237,370 @@ static void __maybe_unused omap_enable_hwecc(struct
mtd_info *mtd, int32_t mode)
}
}
+/*
+ * BCH8 support (needs ELM and thus AM33xx-only)
+ */
+#ifdef CONFIG_AM33XX
+struct nand_bch_priv {
+ uint8_t mode;
+ uint8_t type;
+ uint8_t nibbles;
+};
+
+/* bch types */
+#define ECC_BCH4 0
+#define ECC_BCH8 1
+#define ECC_BCH16 2
+
+/* BCH nibbles for diff bch levels */
+#define NAND_ECC_HW_BCH ((uint8_t)(NAND_ECC_HW_OOB_FIRST) + 1)
+#define ECC_BCH4_NIBBLES 13
+#define ECC_BCH8_NIBBLES 26
+#define ECC_BCH16_NIBBLES 52
+
+static struct nand_ecclayout hw_bch8_nand_oob = GPMC_NAND_HW_BCH8_ECC_LAYOUT;
+
+static struct nand_bch_priv bch_priv = {
+ .mode = NAND_ECC_HW_BCH,
+ .type = ECC_BCH8,
+ .nibbles = ECC_BCH8_NIBBLES
+};
+
+/*
+ * omap_read_bch8_result - Read BCH result for BCH8 level
+ *
+ * @mtd: MTD device structure
+ * @big_endian:When set read register 3 first
+ * @ecc_code: Read syndrome from BCH result registers
+ */
+static void omap_read_bch8_result(struct mtd_info *mtd, uint8_t big_endian,
+ uint8_t *ecc_code)
+{
+ uint32_t *ptr;
+ int8_t i = 0, j;
+
+ if (big_endian) {
+ ptr = gpmc_cfg-bch_result_0_3[0].bch_result_x[3];
+ ecc_code[i++] = readl(ptr) 0xFF;
+ ptr--;
+ for (j = 0; j 3; j++) {
+ ecc_code[i++] = (readl(ptr) 24) 0xFF;
+ ecc_code[i++] = (readl(ptr) 16) 0xFF;
+ ecc_code[i++] = (readl(ptr) 8) 0xFF;
+ ecc_code[i++] = readl(ptr) 0xFF;
+ ptr--;
+ }
+ } else {
+ ptr = gpmc_cfg-bch_result_0_3[0].bch_result_x[0];
+ for (j = 0; j 3; j++) {
+ ecc_code[i++] = readl(ptr) 0xFF;
+ ecc_code[i++] = (readl(ptr) 8) 0xFF;
+ ecc_code[i++] = (readl(ptr) 16) 0xFF;
+ ecc_code[i++] = (readl(ptr) 24) 0xFF;
+ ptr++;
+ }
+ ecc_code[i++] = readl(ptr) 0xFF;
+ ecc_code[i++] = 0; /* 14th byte is always zero */
+ }
+}
+
+/*
+ * omap_ecc_disable - Disable H/W ECC calculation
+ *
+ * @mtd: MTD device structure
+ *
+ */
+static void omap_ecc_disable(struct mtd_info *mtd)
+{
+ writel((readl(gpmc_cfg-ecc_config) ~0x1),
+ gpmc_cfg-ecc_config);
+}
+
+/*
+ * omap_rotate_ecc_bch - Rotate the syndrome bytes
+ *
+ * @mtd: MTD device structure
+ * @calc_ecc: ECC read from ECC registers
+ * @syndrome: Rotated syndrome will be retuned in this array
+ *
+ */
+static void omap_rotate_ecc_bch(struct mtd_info *mtd, uint8_t *calc_ecc,
+ uint8_t *syndrome)
+{
+ struct nand_chip *chip = mtd-priv;
+ struct nand_bch_priv *bch = chip-priv;
+ uint8_t n_bytes = 0;
+ int8_t i, j;
+
+ switch (bch-type) {
+ case ECC_BCH4:
+ n_bytes = 8;
+ break;
+
+ case ECC_BCH16:
+ n_bytes = 28;
+ break;
+
+ case ECC_BCH8:
+ default:
+ n_bytes = 13;
+ break;
+ }
+
+ for (i = 0, j = (n_bytes-1); i n_bytes; i++, j--)
+ syndrome[i] = calc_ecc[j];
+}
+
+/*
+ * omap_calculate_ecc_bch - Read BCH ECC result
+ *
+ * @mtd: MTD structure
+ * @dat: unused
+ * @ecc_code: ecc_code buffer
+ */
+static int omap_calculate_ecc_bch(struct mtd_info *mtd, const uint8_t *dat,
+ uint8_t *ecc_code)
+{
+ struct nand_chip *chip = mtd-priv;
+ struct nand_bch_priv *bch = chip-priv;
+ uint8_t big_endian = 1;
+ int8_t ret = 0;
+
+ if (bch-type == ECC_BCH8)
+ omap_read_bch8_result(mtd, big_endian, ecc_code);
+ else /* BCH4 and BCH16 currently not supported */
+ ret = -1;
+
+ /*
+* Stop reading
