lkml.org 
[lkml]   [2020]   [Jun]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] mtd: rawnand: brcmnand: force raw OOB writes
From
Date
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.

\
 
 \ /
  Last update: 2020-06-14 10:54    [W:0.111 / U:2.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site