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 <h...@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: h...@denx.de