Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Content-Type: text/plain;
        charset=utf-8
Mime-Version: 1.0 (Mac OS X Mail 13.4 \(3608.80.23.2.2\))
Subject: Re: [PATCH] mtd: rawnand: brcmnand: force raw OOB writes
From:   =?utf-8?Q?=C3=81lvaro_Fern=C3=A1ndez_Rojas?= <noltari@gmail.com>
In-Reply-To: <CAC=U0a0=7UHY2fH+2HS5hbxThe9rYvqTKggFJP4mm-5ib52dtA@mail.gmail.com>
Date:   Sun, 14 Jun 2020 10:53:09 +0200
Cc:     Brian Norris <computersforpeace@gmail.com>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        "R, Vignesh" <vigneshr@ti.com>,
        Sumit Semwal <sumit.semwal@linaro.org>,
        MTD Maling List <linux-mtd@lists.infradead.org>,
        bcm-kernel-feedback-list <bcm-kernel-feedback-list@broadcom.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
        linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linaro-mm-sig@lists.linaro.org
Content-Transfer-Encoding: quoted-printable
Message-Id: <F9D394A3-43EC-4E61-B4CA-C113C4516FB1@gmail.com>
References: <20200605170720.2478262-1-noltari@gmail.com>
 <CAC=U0a3xa9k76mxsiVKDyXsuvboZAyMkXT-S3-6oVZKjnWkeHA@mail.gmail.com>
 <05C962F2-7D3B-4103-91DD-8D31C439D521@gmail.com>
 <CAC=U0a0=7UHY2fH+2HS5hbxThe9rYvqTKggFJP4mm-5ib52dtA@mail.gmail.com>
To:     Kamal Dasu <kdasu.kdev@gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Kamal,

> El 13 jun 2020, a las 17:16, Kamal Dasu <kdasu.kdev@gmail.com> =
escribi=C3=B3:
>=20
> Alvaro,
>=20
>=20
> On Sat, Jun 13, 2020 at 5:01 AM =C3=81lvaro Fern=C3=A1ndez Rojas
> <noltari@gmail.com> wrote:
>>=20
>> Hi Kamal,
>>=20
>>> El 12 jun 2020, a las 20:47, Kamal Dasu <kdasu.kdev@gmail.com> =
escribi=C3=B3:
>>>=20
>>> On Fri, Jun 5, 2020 at 1:07 PM =C3=81lvaro Fern=C3=A1ndez Rojas =
<noltari@gmail.com> wrote:
>>>>=20
>>>> 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
>>>>=20
>>>> 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
>>>=20
>>> 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
>>=20
>> Are you sure about that? I think that HW ECC is only calculated for =
DATA, not for OOB.
>> The fact that we=E2=80=99re not writing any DATA seems to be the =
problem that is changing all ECC bytes to 0x00.
>>=20
>=20
> 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=3D0 (when present)
> does not allow for spare areas only writes.
>=20
>>> 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.
>>=20
>> No, sub-pages/oob areas read by JFFS2 with ECC enabled.
>> I don=E2=80=99t think this breaks anything at all, since I tested =
nandwrite with OOB enabled and everything was written perfectly.
>>=20
>=20
> 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=E2=80=99ll 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.

>=20
>> 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
>>=20
>=20
> 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=E2=80=99ll test this and report back once I get my new device.

>=20
>>>=20
>>>>=20
>>>> Signed-off-by: =C3=81lvaro Fern=C3=A1ndez Rojas <noltari@gmail.com>
>>>> ---
>>>> drivers/mtd/nand/raw/brcmnand/brcmnand.c | 9 +--------
>>>> 1 file changed, 1 insertion(+), 8 deletions(-)
>>>>=20
>>>> 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);
>>>> }
>>>>=20
>>>> -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 =3D 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 =3D brcmnand_write_oob_raw;
>>>>       chip->ecc.read_oob_raw =3D brcmnand_read_oob_raw;
>>>>       chip->ecc.read_oob =3D brcmnand_read_oob;
>>>> -       chip->ecc.write_oob =3D brcmnand_write_oob;
>>>> +       chip->ecc.write_oob =3D brcmnand_write_oob_raw;
>>>>=20
>>>>       chip->controller =3D &ctrl->controller;
>>>>=20
>>>> --
>>>> 2.26.2
>>>>=20
>>=20
>> Best regards,
>> =C3=81lvaro.