Re: [PATCH] mtd: rawnand: brcmnand: force raw OOB writes

["Álvaro Fernández Rojas" <noltari@gmail.com>]

Hi Kamal,

> El 13 jun 2020, a las 17:16, Kamal Dasu <kdasu.kdev@gmail.com> escribió:
> 
> Alvaro,
> 
> 
> On Sat, Jun 13, 2020 at 5:01 AM Álvaro Fernández Rojas
> <noltari@gmail.com> wrote:
>> 
>> Hi Kamal,
>> 
>>> El 12 jun 2020, a las 20:47, Kamal Dasu <kdasu.kdev@gmail.com> escribió:
>>> 
>>> On Fri, Jun 5, 2020 at 1:07 PM Álvaro Fernández Rojas <noltari@gmail.com> 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@gmail.com/
>> 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@gmail.com>
>>>> ---
>>>> 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.