Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7ae0a89d-2cbc-1e0c-527a-4d6b6218dc2d@amlogic.com>
Date:   Wed, 12 Apr 2023 20:28:09 +0800
User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:102.0) Gecko/20100101
 Thunderbird/102.9.1
Subject: Re: [PATCH v1 4/5] mtd: rawnand: meson: clear OOB buffer before read
Content-Language: en-US
To:     Dmitry Rokosov <ddrokosov@sberdevices.ru>
Cc:     Arseniy Krasnov <avkrasnov@sberdevices.ru>,
        Miquel Raynal <miquel.raynal@bootlin.com>,
        Richard Weinberger <richard@nod.at>,
        Vignesh Raghavendra <vigneshr@ti.com>,
        Neil Armstrong <neil.armstrong@linaro.org>,
        Kevin Hilman <khilman@baylibre.com>,
        Jerome Brunet <jbrunet@baylibre.com>,
        Martin Blumenstingl <martin.blumenstingl@googlemail.com>,
        Jianxin Pan <jianxin.pan@amlogic.com>,
        Yixun Lan <yixun.lan@amlogic.com>, oxffffaa@gmail.com,
        kernel@sberdevices.ru, linux-mtd@lists.infradead.org,
        linux-arm-kernel@lists.infradead.org,
        linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org
References: <20230412061700.1492474-1-AVKrasnov@sberdevices.ru>
 <20230412061700.1492474-5-AVKrasnov@sberdevices.ru>
 <20230412094400.3c82f631@xps-13>
 <ac4b66da-6a76-c2ec-7e21-31632f3448d5@sberdevices.ru>
 <20230412113654.183350d0@xps-13>
 <4eace0a0-f6af-7d99-a52f-7913a2139330@sberdevices.ru>
 <4baac18b-c223-d346-503c-f5f9ae49320d@amlogic.com>
 <fe03919a-3468-b79e-5971-1f7671684ee9@amlogic.com>
 <20230412114326.fn3nm5r3rzlnqgg7@CAB-WSD-L081021>
From:   Liang Yang <liang.yang@amlogic.com>
In-Reply-To: <20230412114326.fn3nm5r3rzlnqgg7@CAB-WSD-L081021>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?bzJTS09tdURDRUI3TXZrTXhDMmtZcytIUEZxSEgxUm1mWmIxaU11MkdZUnpu?=
 =?utf-8?B?OFRzS1h5dWpHNW04TDk0bEw0VkhTMDZIdUtGeU4zQ0djRUNCWS9CWkREbkoz?=
 =?utf-8?B?L29SakFVOXNXMFNsTTM1dndwcEFtaENvRVUvL0FkTzZZVlVQYkJQV0hYd1hh?=
 =?utf-8?B?YW0rUldoMjZGcjlOb2hqdmNDTnQ4UnNuY3JvNDl4MWFRZzhsTHNGL29TQnZa?=
 =?utf-8?B?alRWaS9VKzNsS0NnWnE5WTBURlBHL1NCSWo5RGNKVE5KbHlhNEtEdWM1NUpr?=
 =?utf-8?B?WGNHRlZUWFczNFAwMFJjemdZN1lIVk8rbmZnRXFjM24yUlpoaGRCSzJFdGF4?=
 =?utf-8?B?UXp6RWNzT21KVnVUd0pWd0pHUVowVk56a1h3eE5VYmRxQ2R2WGxzalVlRGI5?=
 =?utf-8?B?cUw3ZGJnbzBGR0tVbU45ck9sdmVkSHBEbFZtV25Bdll6ZlJsUjhNSjdueGlU?=
 =?utf-8?B?eEk3NUJJaGora2EzOC9pN1N4VGhZZ3VDd21xUXpNWlVLb1o4VEtYaHJNZFRX?=
 =?utf-8?B?MWlreXEzL1ozbEpiZm1LZnovZk8xZkM2ZFZKQTVxbFBHZjUzUUZEeVRoMDBG?=
 =?utf-8?B?cG9xcGtqWFVkU01zWVBZa3hwc0NuUnpObzN2eDVodnVxVFV6SW44WEZIWTdY?=
 =?utf-8?B?N21QUTZoVTdiWFl5a01RMlNuMFIrMzdla3RVSGluNW5UblNmSDBEbVhGUWU1?=
 =?utf-8?B?dnFNdWpmVFBzekxldXpUbEt3MXVUUVVmWmpEZGNDbTFLRkRmRU1ZSjBLK3BL?=
 =?utf-8?B?TUlwdE53VzB0SFpJQVNwU2RCSjl1MitkR1VwR0NLaXNPa0I4MlI2bjA4cmtq?=
 =?utf-8?B?cm5HYUpwOTZWc2l5UHA3WnVwcCtvSjdib2VDVnNxUkJsV2dVbE9zaW01RDVD?=
 =?utf-8?B?UnNRWjlWdUZ2ME5qNGxhWmRpQnM0c090SFFkbFU4aE5RQ2QrbU5DZ3dDVFgx?=
 =?utf-8?B?U0NVUHdLakh1blZGK01PYWlKcGVPUytCb1pxQ2Nxay9QRGRPcG1STTdicFZL?=
 =?utf-8?B?TW1GZ3NCejlrQ0hTZ0grNnJIdmdmdmdRUUlCTnQvVmNTZ0hPYlNhVjFTUUhV?=
 =?utf-8?B?N0pINzlaZ01IWmhVN3hRUzYrVnRjNkdXaTh2bHA4cGFlS2ZrNEdIbHh1Sk9p?=
 =?utf-8?B?dkNaR1JGWTFzaGJ0WCt4NVFJM3d6cnpObFZ2cWdzZWg3ZFlJdHQxVHJlMEN4?=
 =?utf-8?B?d3lqLytRSTRJNFg3dmU3dENKRUlVQ3g1OCtSRHBIWUorNCtaczgzcnBsbFhB?=
 =?utf-8?B?NzlNdU5wVXcyNzdIUTdJazRVTGsrQWY5Rjd0ZzU0aW85Tm9Pc1FGQ0pEWCsr?=
 =?utf-8?B?emZPQ3RkRHd5YS9tN1lyMXhMWlZFTUI4dEhrM0pRTHhWRXFlcFdzZ2J6QndC?=
 =?utf-8?B?MCtTdVZxbmlSY1RQZVZDdUFZOVZrQUg4ZmRsVlFIcW90Unk3VnVjelNzY2Qz?=
 =?utf-8?B?dUF2TERYb0sxUSsvWVVXUWE1MDZmS3BaVGl1dlN6N0ZnRFZPMkFESk5scDJm?=
 =?utf-8?B?NU1Help0QUxNTS9BYjMzUFRibWUyVDJZUTNBZk5GY01UaXZZR2FKcUNOSnF5?=
 =?utf-8?B?bEZ5T0RDelRNMWRhUll3bjhheGdicVpSMSs0ekVPcVd5QzJkM2ZDTk1PRDVB?=
 =?utf-8?B?YlhNU3dKejZYSTZwU01CcXJVMkxHb1M0ZTNndEZGam5sQ2lOUXhnQ0ZQVnZT?=
 =?utf-8?B?MW1vN2Y2UWVrL3FsenpSNUNYWVp2c01zS0hGZXBRN1ptN3RDQjVYalNSK2Jz?=
 =?utf-8?B?cHNhcG5lU3BRZnB5ekRoY3ZPeE5SSE10RlBQdis2T2pBSEJpUWNGK2FXNENV?=
 =?utf-8?B?UklDR3luMG95OE5LYi9mMHpFQUhJUVFldEM4ZDc3WFRhZmJ0NUtZT21XdEQr?=
 =?utf-8?B?SDZoSCtwT3lhQ0s3V2lUYmhja0tPalhFMUhhUEo1SWJSaFVpTkpTZzUrbTlO?=
 =?utf-8?B?elZSaUR3RGw3QlcyOW1tS2FVdmpISWJBeTBuZThCSWtNb1dhNGFjZ3VPTlg0?=
 =?utf-8?B?akJWMm0xVjJtNDNFV3gvYXdWVFBBZlNTZmhTWEJ6S0d3cHEwdVhodHBmT1px?=
 =?utf-8?B?OEF5bGoxY0dWaFp2K0E2d2JYSFFsTmxEdXF6d2ZkTHBWcGpXMnN4Q1h1R0N3?=
 =?utf-8?Q?SksdJnOYQpuyy/QVvvgUSQHJZ?=
X-OriginatorOrg: amlogic.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f5163f3b-04f5-48db-13d9-08db3b516307
X-MS-Exchange-CrossTenant-AuthSource: PS1PR03MB5034.apcprd03.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Apr 2023 12:28:14.5634
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 0df2add9-25ca-4b3a-acb4-c99ddf0b1114
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: W0F1e0ycX5RJkPnw2GFS/WTXSTB2w0cNcSZGgfi9rIShmRZVKkgZrqkPvjU/xRigbTPN3pzVxNEHmtxBzy4cvg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SG2PR03MB6801
X-Spam-Status: No, score=-3.0 required=5.0 tests=BAYES_00,NICE_REPLY_A,
        RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Dmitry,

On 2023/4/12 19:43, Dmitry Rokosov wrote:
> [ EXTERNAL EMAIL ]
> 
> Hello Liang,
> 
> On Wed, Apr 12, 2023 at 07:36:30PM +0800, Liang Yang wrote:
>> Hi,
>>
>> On 2023/4/12 18:51, Liang Yang wrote:
>>>>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>> b/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>> index f84a10238e4d..f2f2472cb511 100644
>>>>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>>>>>> @@ -858,9 +858,12 @@ static int
>>>>>>>> meson_nfc_read_page_sub(struct nand_chip *nand,
>>>>>>>>    static int meson_nfc_read_page_raw(struct nand_chip *nand, u8 *buf,
>>>>>>>>                       int oob_required, int page)
>>>>>>>>    {
>>>>>>>> +    struct mtd_info *mtd = nand_to_mtd(nand);
>>>>>>>>        u8 *oob_buf = nand->oob_poi;
>>>>>>>>        int ret;
>>>>>>>> +    memset(oob_buf, 0, mtd->oobsize);
>>>>>>>
>>>>>>> I'm surprised raw reads do not read the entire OOB?
>>>>>>
>>>>>> Yes! Seems in case of raw access (what i see in this driver)
>>>>>> number of OOB bytes read
>>>>>> still depends on ECC parameters: for each portion of data
>>>>>> covered with ECC code we can
>>>>>> read it's ECC code and "user bytes" from OOB - it is what i
>>>>>> see by dumping DMA buffer by
>>>>>> printk(). For example I'm working with 2K NAND pages, each
>>>>>> page has 2 x 1K ECC blocks.
>>>>>> For each ECC block I have 16 OOB bytes which I can access by
>>>>>> read/write. Each 16 bytes
>>>>>> contains 2 bytes of user's data and 14 bytes ECC codes. So
>>>>>> when I read page in raw mode
>>>>>> controller returns 32 bytes (2 x (2 + 14)) of OOB. While OOB
>>>>>> is reported as 64 bytes.
>>>>>
>>>>> In all modes, when you read OOB, you should get the full OOB. The fact
>>>>> that ECC correction is enabled or disabled does not matter. If the NAND
>>>>> features OOB sections of 64 bytes, you should get the 64 bytes.
>>>>>
>>>>> What happens sometimes, is that some of the bytes are not protected
>>>>> against bitflips, but the policy is to return the full buffer.
>>>>
>>>> Ok, so to clarify case for this NAND controller:
>>>> 1) In both ECC and raw modes i need to return the same raw OOB data
>>>> (e.g. user bytes
>>>>      + ECC codes)?
>>>> 2) If I have access to only 32 bytes of OOB (in case above), I must
>>>> report that size
>>>>      of OOB is only 32 bytes during initialization?
>>>>
>>>> Thanks, Arseniy
>>>
>>> Yes. it should return all the OOB data. i make a mistake on raw read and
>>> there is wrong code in meson_nfc_read_page_raw().
>>>       meson_nfc_get_data_oob(nand, buf, oob_buf);
>>> changed to:
>>>       if (oob_required)
>>>           memcpy(oob_buf, buf + mtd->writesize, mtd->oobsize)
>>
>> Sorry, please ignore this. the previous code is right.
>>
>> the controller changes the layout of one page; the physical layout is 2048
>> main data + 64 oob data. after writing into NAND page, it is stored
>> like this: 1024 main data + 2 user bytes + 14 ECC parity bytes + 1024 main
>> data + 2 user bytes + 14 ECC parity bytes. so that is right we only get 4
>> user bytes and 28 ECC parity bytes, total 32 bytes. that is the behavior of
>> the controller that transferring one ECC page(1KB) brings back only 2 user
>> bytes.
>>
>> because layout is changed by controller, so go back to the function.
>> meson_nfc_get_data_oob(nand, buf, oob_buf) try to get the right user and ecc
>> parity bytes from the right pos. after that, the other oob bytes is not
>> reading from NAND flash.
> 
> I have always been under the impression that NAND OOB layout falls under the
> responsibility of the flash driver. Is this specific to the Amlogic NAND,
> and does it map the flash layout to the internal controller layout?
> For example, different OOB layouts exist between Macronix and ESMT.
> 
> Apologies for any confusion, and thank you in advance for any help in
> clarifying this matter.
> 

This is the behavior of Amlogic NAND controller, not related to NAND 
device.

this design of Amlogic NAND controller is reducing the fifo(sram) size 
inside the controller.

To the NAND device with <2KB(main) + 64B(oob)> page, usually if the 
controller place user bytes and ECC parity bytes after 2048(column 
address). to finish the ECC calculation and checking, the controller has 
to read the data and its ECC parity bytes. if they are not placed 
continuously, the controllers have to read the whole page data including 
the oob data together or send random read command with column address. 
so consider that the controller needs a fifo of at least (2048 + 64) 
bytes inside. if the NAND device has bigger size of NAND page, the size 
of the fifo should be bigger.
in Amlogic NAND controller, after changing the data layout of the NAND 
page, it just needs <1KB + 16B> fifo size. the controller firstly sends 
only <1024 + 16> clocks and get 1st ECC page data and its ECC parity 
bytes, and then it can start ECC calculation and checking. after that or 
at the same time, loop next ECC page data and its ECC parity.

-- 
Thanks,
Liang