[PATCH] mtd: rawnand: omap_gpmc: fix BCH8 HW based correction

[Heiko Schocher]

Hello Roger,

On 21.11.23 22:56, Roger Quadros wrote:
> Hi,
> 
> On 15/11/2023 13:36, Heiko Schocher wrote:
>> Hello Roger,
>>
>> On 15.11.23 12:09, Roger Quadros wrote:
>>>
>>>
>>> On 15/11/2023 07:40, Heiko Schocher wrote:
>>>> commit 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction")
>>>>
>>>> broke AM335x based boards booting from NAND with ECC BCH8 code.
>>>>
>>>> Use omap_enable_hwecc() instead of omap_enable_hwecc_bch()
>>>> in SPL restores correct SPL nand_read_page functionality.
>>>>
>>>> Tested on draco thuban board.
>>>>
>>>> Signed-off-by: Heiko Schocher <hs at denx.de>
>>>>
>>>> ---
>>>> fix NAND boot for BCH8 based TI AM335x boards
>>>>
>>>> Fix is based on series from Enrico:
>>>>
>>>> https://lists.denx.de/pipermail/u-boot/2023-November/536793.html
>>>>
>>>> and fixes NAND boot for the draco thuban board. But this patch
>>>> apply also without the patches from above patchseries, see
>>>> azure build:
>>>>
>>>> https://dev.azure.com/hs0298/hs/_build/results?buildId=111&view=results
>>>>
>>>> which is clean.
>>>>
>>>> Above commit seems to change only U-Boot code and did not
>>>> adapt am335x_spl_bch.c, which breaks nand_read_page in SPL
>>>> code for AM335x based boards. So use in SPL "old" hw setup
>>>> and reading page from NAND in SPL works again.
>>>>
>>>>
>>>>  drivers/mtd/nand/raw/omap_gpmc.c | 4 ++++
>>>>  1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/mtd/nand/raw/omap_gpmc.c b/drivers/mtd/nand/raw/omap_gpmc.c
>>>> index 1a5ed0de31..c9b66dadbd 100644
>>>> --- a/drivers/mtd/nand/raw/omap_gpmc.c
>>>> +++ b/drivers/mtd/nand/raw/omap_gpmc.c
>>>> @@ -983,7 +983,11 @@ static int omap_select_ecc_scheme(struct nand_chip *nand,
>>>>  		nand->ecc.strength	= 8;
>>>>  		nand->ecc.size		= SECTOR_BYTES;
>>>>  		nand->ecc.bytes		= 14;
>>>> +#if defined(CONFIG_SPL_BUILD)
>>>> +		nand->ecc.hwctl		= omap_enable_hwecc;
>>>
>>> I don't think this is correct. omap_enable_hwecc does not setup
>>> the BCH settings and should be used only for 1-bit ECC i.e. OMAP_ECC_HAM1_CODE_*
>>>
>>> I'm sure it will break the boards not using
>>> CONFIG_SPL_NAND_AM33XX_BCH which is
>>> pretty much most TI boards that are not AM335x.
>>>
>>> Now, I need to identify why AM335x NAND is broken.
>>
>> Yes, I played with setting up stuff in drivers/mtd/nand/raw/am335x_spl_bch.c
>> but failed ...
>>
>> If I can help you testing, feel free to ask!
> 
> OK. So I did some tests and we are ending up in a different ECC config on AM335x.
> The ecc.steps is not set at all by the driver (it is at 0) and so so nsectors gets
> set to -1 and so 7 (0b111) which is wrong.
> 
> static void __maybe_unused omap_enable_hwecc_bch(struct mtd_info *mtd,
> 						 int mode)
> {
> ...
> 	case OMAP_ECC_BCH8_CODE_HW:
> 		bch_type = 1;
> 		nsectors = chip->ecc.steps;
> ...
> 	/* BCH configuration */
> 	val = ((1			<< 16) | /* enable BCH */
> 	       (bch_type		<< 12) | /* BCH4/BCH8/BCH16 */
> 	       (wr_mode			<<  8) | /* wrap mode */
> 	       (dev_width		<<  7) | /* bus width */
> -->	       (((nsectors - 1) & 0x7)	<<  4) | /* number of sectors */
> 	       (info->cs		<<  1) | /* ECC CS */
> 	       (0x1));	
> ...
> }
> 
> setting chip->ecc.steps needs to be fixed in original commit 
> c3754e9cc23a ("omap_gpmc: BCH8 support (ELM based)")
> 
> But this will still not fix the issue for AM335x as am335x_spl_bch.c
> expects 1 sector at a time ECC calculation.

Yep...

> I also realized that the error correction logic (ELM) is not in page mode
> and so is not suitable for multi-sector error correction.

Okay!

> This means we have to revert the commit
> 04fcd2587321 ("mtd: rawnand: omap_gpmc: Fix BCH6/16 HW based correction")
> 
> and go back to 1 sector BCH ECC generation and correction.

Uff... I did exactly this to get it working (in first shot) but feared
sending such a "drastic" patch, as I had the hope to fix "AM335x SPL code",
as U-Boot part works on AM335x...

What I wonder is that above commit introduced "omap_calculate_ecc_bch_multi"
but did not use it in SPL! For U-Boot code omap_calculate_ecc_bch_multi is
used in drivers/mtd/nand/raw/omap_gpmc.c omap_read_page_bch()

but drivers/mtd/nand/raw/am335x_spl_bch.c has no adaption ... so I tried
to adapt this file... but did not get it up working without digging
very deep into it...


> I will send a patch fix in a day or two after doing some tests at my end.

Okay, thanks!

bye,
Heiko
-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de