Re: U-Boot OMAP GPMC ECC change

[Roger Quadros <rogerq@kernel.org>]

Hi Colin,

On 12/05/2023 19:05, Colin Foster wrote:
> Hi Roger,
> 
> On Fri, May 12, 2023 at 02:53:07PM +0300, Roger Quadros wrote:
>>
>>
>> On 10/05/2023 18:38, Colin Foster wrote:
>>>
>>> This is still out-of-U-Boot. I have an include/configs/our_product.h
>>> file with this:
>>>
>>> """
>>> #define CFG_SYS_NAND_ECCPOS             {2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, \
>>>                                          13, 14, 15, 16, 17, 18, 19, 20, 21, 22, \
>>>                                          23, 24, 25, 26, 27, 28, 29, 30, 31, 32, \
>>>                                          33, 34, 35, 36, 37, 38, 39, 40, 41, \
>>>                                          42, 43, 44, 45, 46, 47, 48, 49, 50, 51, \
>>>                                          52, 53, 54, 55, 56, 57}
>>>
>>> #define CFG_SYS_NAND_ECCBYTES   14
>>> #define CFG_SYS_NAND_MAX_ECCPOS 57
>>
>> This should be 56 i.e. (57 - 2 + 1)
>> But it won't fix the issue you are facing. :P
> 
> Oh, good catch. I know when I was trying to get this working I had to
> play with these values quite a bit. I must have missed changing this at
> one point.
> 
>>
>>> #define CFG_SYS_NAND_ECCSIZE            512
>>> #define CFG_SYS_NAND_MAX_OOBFREE        2
>>> """
>>>
>>>
>>>> Can you please point me to the Linux device tree file if it exists?
>>>
>>> This is the latest submission. Still not accepted - I need to find time
>>> to button everything up and resubmit. My plan of attack was Kernel
>>> Acceptance, then U-Boot. Unfortunately my company lets the pesky
>>> "Shipping products" step get in the way :-)
>>>
>>> https://lkml.org/lkml/2023/2/22/939
>>>
>>> Or if you just want the ECC part:
>>>
>>> +	nandflash: nand@0,0 {
>>> +		compatible = "ti,omap2-nand";
>>> +		reg = <0 0 4>;
>>> +		interrupt-parent = <&gpmc>;
>>> +
>>> +		nand-bus-width = <16>;
>>> +		ti,nand-ecc-opt = "bch8";
>>> +		ti,elm-id=<&elm>;
>>> +		linux,mtd-name = "micron,nand";
>>>
>>>
>>> I think that's all the info you're looking for. Let me know if I missed
>>> something.
>>
>> Yes this is all I was looking for.
>>
>> Is CONFIG_NAND_OMAP_ECCSCHEME_BCH8_CODE_HW set in your u-boot config?
> 
> Yes, it is set. I've attached our .config file. I just made a change to
> CONFIG_NAND_OMAP_GPMC_PREFETCH as a test, which didn't fix the issue.
> 
> And as another sanity check, I reverted the patch and have functionality
> again:
> 

I just tested this on AM335x EVM which uses BCH8_CODE_HW but 8-bit NAND part.
I see that you are using 16-bit NAND.

One more difference in u-boot configuration. For me:
CONFIG_NAND_OMAP_GPMC_PREFETCH=y

Not sure if that matters but let's keep it set for now.

For debug can you please apply the patch (at end) to u-boot at commit a95410696d21
(before breakage) and run the test.

Test procedure:

> nand dump 0

> mmc dev 0 
	#replace 0 with whatever points to SD-card
> nand read $loadaddr 0 800
	#loadaddr is any address in RAM which is free for use.
> fatwrite mmc 0:1 $loadaddr nand.org 800


- restore NAND page 0

> fatload mmc 0:1 $loadaddr nand.org
> nand write $loadaddr 0 800


Now please run the below test on the offending commit 04fcd25873 after applying
the patch (at end).

> nand dump 0
> mmc dev 0
	#replace 0 with whatever points to SD-card
> fatload mmc 0:1 $loadaddr nand.org
> nand write $loadaddr 0 800


Please send me the logs of both tests and the nand.org file so I can try it out here. Thanks!


--- patch starts---
diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c
index be3cb3c601..ac06d5f019 100644
--- a/drivers/mtd/nand/raw/omap_gpmc.c
+++ b/drivers/mtd/nand/raw/omap_gpmc.c
@@ -300,6 +300,7 @@ static int omap_calculate_ecc(struct mtd_info *mtd, const uint8_t *dat,
 			ecc_code[i++] = (val >>  0) & 0xFF;
 			ptr--;
 		}
+		printf("ecc: %x\n", val);
 		break;
 	case OMAP_ECC_BCH16_CODE_HW:
 		val = readl(&gpmc_cfg->bch_result_4_6[0].bch_result_x[2]);