Hi Kamal, > El 13 jun 2020, a las 17:16, Kamal Dasu <kdasu.kdev@xxxxxxxxx> escribió: > > Alvaro, > > > On Sat, Jun 13, 2020 at 5:01 AM Álvaro Fernández Rojas > <noltari@xxxxxxxxx> wrote: >> >> Hi Kamal, >> >>> El 12 jun 2020, a las 20:47, Kamal Dasu <kdasu.kdev@xxxxxxxxx> escribió: >>> >>> On Fri, Jun 5, 2020 at 1:07 PM Álvaro Fernández Rojas <noltari@xxxxxxxxx> wrote: >>>> >>>> MTD_OPS_AUTO_OOB is writting OOB with ECC enabled, which changes all ECC bytes >>>> from an erased page to 0x00 when JFFS2 cleanmarkers are added with mtd-utils. >>>> | BBI | JFFS2 | ECC | JFFS2 | Spare | >>>> 00000800 ff ff 19 85 20 03 00 00 00 00 00 00 08 ff ff ff >>>> >>>> However, if OOB is written with ECC disabled, the JFFS2 cleanmarkers are >>>> correctly written without changing the ECC bytes: >>>> | BBI | JFFS2 | ECC | JFFS2 | Spare | >>>> 00000800 ff ff 19 85 20 03 ff ff ff 00 00 00 08 ff ff ff >>> >>> Both brcmand_write_oob_raw() and brcmnand_write_oob() use >>> brcmnand_write() that uses PROGRAM_PAGE cmd, means also programs data >>> areas and ECC when enabled is always calculated on DATA+OOB. since >> >> Are you sure about that? I think that HW ECC is only calculated for DATA, not for OOB. >> The fact that we’re not writing any DATA seems to be the problem that is changing all ECC bytes to 0x00. >> > > Only true for 1-bit hamming code on early controller versions. For > BCH algorithms both data+oob are covered. And the controller driver > intentionally does not implement a spare area program cmd, even for > OOB writes. Also BRCMNAND_ACC_CONTROL_PARTIAL_PAGE=0 (when present) > does not allow for spare areas only writes. > >>> in both cases we only want to modify OOB, data is always assumed to be >>> 0xffs (means erased state). So as far as this api always is used on >>> the erased page it should be good. Are the sub-pages/oob areas read by >>> jffs2 also read without ecc enabled?. Just want to be sure that it >>> does not break any other utilities like nandwrite. >> >> No, sub-pages/oob areas read by JFFS2 with ECC enabled. >> I don’t think this breaks anything at all, since I tested nandwrite with OOB enabled and everything was written perfectly. >> > > Going by the commit message you are assuming 1-bit hamming code, that > is the only exception on early version controllers where only data was > covered for ecc calculations when enabled. > Which version of the controller are you using ?. Did you test with > BCH-4 or any BCH ?. All my devices use hamming ECC, even the ones with controller version v4.0 (BCM63268), which should also support BCH. Therefore I need to stick with hamming ECC if I want the bootloader to be able to boot the kernel. However, I should get a new device with BCH in a few days. I’ll do more tests once I get it, but if BCH also covers OOB, then we could conditionally force write_oob to RAW only if hamming is configured. > >> BTW, I tried another approach that forced MTD_OPS_AUTO_OOB to be written as raw OOB, but it was rejected: >> http://patchwork.ozlabs.org/project/linux-mtd/patch/20200504094253.2741109-1-noltari@xxxxxxxxx/ >> https://lkml.org/lkml/2020/5/4/215 >> > > Are there any other use cases other than jffs2 where only oob needs to > be modified ?. But surely intentionally disabling ecc instead of > forcing it is the way, but I am not sure it will still work for other > BCH algorithms though where both data and oob are covered. I’ll test this and report back once I get my new device. > >>> >>>> >>>> Signed-off-by: Álvaro Fernández Rojas <noltari@xxxxxxxxx> >>>> --- >>>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 9 +-------- >>>> 1 file changed, 1 insertion(+), 8 deletions(-) >>>> >>>> diff --git a/drivers/mtd/nand/raw/brcmnand/brcmnand.c b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>>> index 8f9ffb46a09f..566281770841 100644 >>>> --- a/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>>> +++ b/drivers/mtd/nand/raw/brcmnand/brcmnand.c >>>> @@ -2279,13 +2279,6 @@ static int brcmnand_write_page_raw(struct nand_chip *chip, const uint8_t *buf, >>>> return nand_prog_page_end_op(chip); >>>> } >>>> >>>> -static int brcmnand_write_oob(struct nand_chip *chip, int page) >>>> -{ >>>> - return brcmnand_write(nand_to_mtd(chip), chip, >>>> - (u64)page << chip->page_shift, NULL, >>>> - chip->oob_poi); >>>> -} >>>> - >>>> static int brcmnand_write_oob_raw(struct nand_chip *chip, int page) >>>> { >>>> struct mtd_info *mtd = nand_to_mtd(chip); >>>> @@ -2642,7 +2635,7 @@ static int brcmnand_init_cs(struct brcmnand_host *host, struct device_node *dn) >>>> chip->ecc.write_oob_raw = brcmnand_write_oob_raw; >>>> chip->ecc.read_oob_raw = brcmnand_read_oob_raw; >>>> chip->ecc.read_oob = brcmnand_read_oob; >>>> - chip->ecc.write_oob = brcmnand_write_oob; >>>> + chip->ecc.write_oob = brcmnand_write_oob_raw; >>>> >>>> chip->controller = &ctrl->controller; >>>> >>>> -- >>>> 2.26.2 >>>> >> >> Best regards, >> Álvaro. _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
