2023-11-24 07:59:50

by Arseniy Krasnov

Subject: Re: [PATCH v1] mtd: rawnand: meson: handle OOB buffer according OOB layout

Hello all, 2 weeks from 9.11, please ping

Thanks, Arseniy


On 09.11.2023 12:09, Arseniy Krasnov wrote:
> Hello, thanks for review!
>
> On 09.11.2023 11:06, Viacheslav Bocharov wrote:
>> Hi!
>>
>> On Thu, 2023-11-09 at 08:39 +0300, Arseniy Krasnov wrote:
>>> In case of MTD_OPS_AUTO_OOB mode, MTD/NAND layer fills/reads OOB buffer
>>> according current OOB layout so we need to follow it in the driver.
>>>
>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>> ---
>>> drivers/mtd/nand/raw/meson_nand.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>> index 561d46d860b7..0d4d358152d7 100644
>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>> @@ -510,7 +510,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>> __le64 *info;
>>> int i, count;
>>>
>>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (2 + nand->ecc.bytes)) {
>>> info = &meson_chip->info_buf[i];
>>> *info |= oob_buf[count];
>>> *info |= oob_buf[count + 1] << 8;
>> Seems something wrong with your logic here.
>> I think this code should most likely look like this:
>>
>> for (i = 0, count = 0; i < nand->ecc.steps; i++, count += nand->ecc.bytes) {
>> info = &meson_chip->info_buf[i];
>> *info |= oob_buf[count];
>> if (nand->ecc.bytes > 1)
>> *info |= oob_buf[count + 1] << 8;
>> }
>
> For 64 bytes OOB and 512 bytes ECC this driver reports free areas as:
>
> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
>
> where AA is free byte(user byte), BB - ECC codes. So to access user bytes
> we need bytes 0,1,16,17,32,33,48,49. nand->ecc.bytes == 14, so 'count' is
> increased at 16 every iteration, so i guess this is correct.
>
> WDYT?
>
> Thanks, Arseniy
>
>>
>>
>>> @@ -523,7 +523,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>> __le64 *info;
>>> int i, count;
>>>
>>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (2 + nand->ecc.bytes)) {
>>> info = &meson_chip->info_buf[i];
>>> oob_buf[count] = *info;
>>> oob_buf[count + 1] = *info >> 8;
>> And there:
>>
>> for (i = 0, count = 0; i < nand->ecc.steps; i++, count += nand->ecc.bytes) {
>> info = &meson_chip->info_buf[i];
>> oob_buf[count] = *info;
>> if (nand->ecc.bytes > 1)
>> oob_buf[count + 1] = *info >> 8;
>> }
>>
>>
>> This is more similar to the behavior of similar functions in the proprietary U-Boot.
>>
>> --
>> Viacheslav Bocharov
>>

2023-11-24 09:07:19

by Miquel Raynal

Subject: Re: [PATCH v1] mtd: rawnand: meson: handle OOB buffer according OOB layout

Hi Arseniy,

[email protected] wrote on Fri, 24 Nov 2023 10:50:54 +0300:

> Hello all, 2 weeks from 9.11, please ping

I'm waiting for Viacheslav.

>
> Thanks, Arseniy
>
>
> On 09.11.2023 12:09, Arseniy Krasnov wrote:
> > Hello, thanks for review!
> >
> > On 09.11.2023 11:06, Viacheslav Bocharov wrote:
> >> Hi!
> >>
> >> On Thu, 2023-11-09 at 08:39 +0300, Arseniy Krasnov wrote:
> >>> In case of MTD_OPS_AUTO_OOB mode, MTD/NAND layer fills/reads OOB buffer
> >>> according current OOB layout so we need to follow it in the driver.
> >>>
> >>> Signed-off-by: Arseniy Krasnov <[email protected]>
> >>> ---
> >>> drivers/mtd/nand/raw/meson_nand.c | 4 ++--
> >>> 1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
> >>> index 561d46d860b7..0d4d358152d7 100644
> >>> --- a/drivers/mtd/nand/raw/meson_nand.c
> >>> +++ b/drivers/mtd/nand/raw/meson_nand.c
> >>> @@ -510,7 +510,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>> __le64 *info;
> >>> int i, count;
> >>>
> >>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (2 + nand->ecc.bytes)) {
> >>> info = &meson_chip->info_buf[i];
> >>> *info |= oob_buf[count];
> >>> *info |= oob_buf[count + 1] << 8;
> >> Seems something wrong with your logic here.
> >> I think this code should most likely look like this:
> >>
> >> for (i = 0, count = 0; i < nand->ecc.steps; i++, count += nand->ecc.bytes) {
> >> info = &meson_chip->info_buf[i];
> >> *info |= oob_buf[count];
> >> if (nand->ecc.bytes > 1)
> >> *info |= oob_buf[count + 1] << 8;
> >> }
> >
> > For 64 bytes OOB and 512 bytes ECC this driver reports free areas as:
> >
> > AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> > AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> > AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> > AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
> >
> > where AA is free byte(user byte), BB - ECC codes. So to access user bytes
> > we need bytes 0,1,16,17,32,33,48,49. nand->ecc.bytes == 14, so 'count' is
> > increased at 16 every iteration, so i guess this is correct.
> >
> > WDYT?
> >
> > Thanks, Arseniy
> >
> >>
> >>
> >>> @@ -523,7 +523,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
> >>> __le64 *info;
> >>> int i, count;
> >>>
> >>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
> >>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (2 + nand->ecc.bytes)) {
> >>> info = &meson_chip->info_buf[i];
> >>> oob_buf[count] = *info;
> >>> oob_buf[count + 1] = *info >> 8;
> >> And there:
> >>
> >> for (i = 0, count = 0; i < nand->ecc.steps; i++, count += nand->ecc.bytes) {
> >> info = &meson_chip->info_buf[i];
> >> oob_buf[count] = *info;
> >> if (nand->ecc.bytes > 1)
> >> oob_buf[count + 1] = *info >> 8;
> >> }
> >>
> >>
> >> This is more similar to the behavior of similar functions in the proprietary U-Boot.
> >>
> >> --
> >> Viacheslav Bocharov
> >>


Thanks,
Miquèl

2023-11-24 10:15:33

by Viacheslav

Subject: Re: [PATCH v1] mtd: rawnand: meson: handle OOB buffer according OOB layout

Hi, Miquel!

24/11/2023 12.06, Miquel Raynal wrote:
> Hi Arseniy,
>
> [email protected] wrote on Fri, 24 Nov 2023 10:50:54 +0300:
>
>> Hello all, 2 weeks from 9.11, please ping
>
> I'm waiting for Viacheslav.

We discussed this update with Arseniy. My suggestion was based on the
difference in patch from the algorithms used in amlogic u-boot. Arseniy
claims that everything works for them.

Unfortunately, I don't have the devices available to test this.

>
>>
>> Thanks, Arseniy
>>
>>
>> On 09.11.2023 12:09, Arseniy Krasnov wrote:
>>> Hello, thanks for review!
>>>
>>> On 09.11.2023 11:06, Viacheslav Bocharov wrote:
>>>> Hi!
>>>>
>>>> On Thu, 2023-11-09 at 08:39 +0300, Arseniy Krasnov wrote:
>>>>> In case of MTD_OPS_AUTO_OOB mode, MTD/NAND layer fills/reads OOB buffer
>>>>> according current OOB layout so we need to follow it in the driver.
>>>>>
>>>>> Signed-off-by: Arseniy Krasnov <[email protected]>
>>>>> ---
>>>>> drivers/mtd/nand/raw/meson_nand.c | 4 ++--
>>>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c
>>>>> index 561d46d860b7..0d4d358152d7 100644
>>>>> --- a/drivers/mtd/nand/raw/meson_nand.c
>>>>> +++ b/drivers/mtd/nand/raw/meson_nand.c
>>>>> @@ -510,7 +510,7 @@ static void meson_nfc_set_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>>> __le64 *info;
>>>>> int i, count;
>>>>>
>>>>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (2 + nand->ecc.bytes)) {
>>>>> info = &meson_chip->info_buf[i];
>>>>> *info |= oob_buf[count];
>>>>> *info |= oob_buf[count + 1] << 8;
>>>> Seems something wrong with your logic here.
>>>> I think this code should most likely look like this:
>>>>
>>>> for (i = 0, count = 0; i < nand->ecc.steps; i++, count += nand->ecc.bytes) {
>>>> info = &meson_chip->info_buf[i];
>>>> *info |= oob_buf[count];
>>>> if (nand->ecc.bytes > 1)
>>>> *info |= oob_buf[count + 1] << 8;
>>>> }
>>>
>>> For 64 bytes OOB and 512 bytes ECC this driver reports free areas as:
>>>
>>> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
>>> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
>>> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
>>> AA AA BB BB BB BB BB BB BB BB BB BB BB BB BB BB
>>>
>>> where AA is free byte(user byte), BB - ECC codes. So to access user bytes
>>> we need bytes 0,1,16,17,32,33,48,49. nand->ecc.bytes == 14, so 'count' is
>>> increased at 16 every iteration, so i guess this is correct.
>>>
>>> WDYT?
>>>
>>> Thanks, Arseniy
>>>
>>>>
>>>>
>>>>> @@ -523,7 +523,7 @@ static void meson_nfc_get_user_byte(struct nand_chip *nand, u8 *oob_buf)
>>>>> __le64 *info;
>>>>> int i, count;
>>>>>
>>>>> - for (i = 0, count = 0; i < nand->ecc.steps; i++, count += 2) {
>>>>> + for (i = 0, count = 0; i < nand->ecc.steps; i++, count += (2 + nand->ecc.bytes)) {
>>>>> info = &meson_chip->info_buf[i];
>>>>> oob_buf[count] = *info;
>>>>> oob_buf[count + 1] = *info >> 8;
>>>> And there:
>>>>
>>>> for (i = 0, count = 0; i < nand->ecc.steps; i++, count += nand->ecc.bytes) {
>>>> info = &meson_chip->info_buf[i];
>>>> oob_buf[count] = *info;
>>>> if (nand->ecc.bytes > 1)
>>>> oob_buf[count + 1] = *info >> 8;
>>>> }
>>>>
>>>>
>>>> This is more similar to the behavior of similar functions in the proprietary U-Boot.
>>>>
>>>> --
>>>> Viacheslav Bocharov
>>>>
>
>
> Thanks,
> Miquèl

--
Viacheslav Bocharov