Messages in this thread | | | Date | Mon, 29 May 2023 22:46:59 +0300 | Subject | Re: [PATCH v4 3/5] mtd: rawnand: meson: always read whole OOB bytes | From | Arseniy Krasnov <> |
| |
On 26.05.2023 20:09, Miquel Raynal wrote: > Hi Arseniy, > > avkrasnov@sberdevices.ru wrote on Tue, 23 May 2023 20:27:35 +0300: > >> On 22.05.2023 18:38, Miquel Raynal wrote: >>> Hi Arseniy, >>> >>> AVKrasnov@sberdevices.ru wrote on Mon, 15 May 2023 12:44:37 +0300: >>> >>>> This changes size of read access to OOB area by reading all bytes of >>>> OOB (free bytes + ECC engine bytes). >>> >>> This is normally up to the user (user in your case == jffs2). The >>> controller driver should expose a number of user accessible bytes and >>> then when users want the OOB area, they should access it entirely. On >>> top of that read, they can extract (or "write only") the user bytes. >> >> Sorry, I didn't get it. If driver exposes N bytes of user accessible bytes, >> I must always return whole OOB yes? E.g. N + rest of OOB > > Yes. At the NAND controller level, you get asked for either a page of > data (sometimes a subpage, but whatever), and/or the oob area. You need > to provide what is requested, no more, no less. The upper layers will > trim down what's uneeded and extract the bytes they want.
I see, so in this case I think this patch could be merged to the patch which changes OOB layout be moving it out of ECC area? Because driver MUST return all bytes of OOB area.
> >>>> Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> >>>> --- >>>> drivers/mtd/nand/raw/meson_nand.c | 24 ++++++++++++++++++++++++ >>>> 1 file changed, 24 insertions(+) >>>> >>>> diff --git a/drivers/mtd/nand/raw/meson_nand.c b/drivers/mtd/nand/raw/meson_nand.c >>>> index 8526a6b87720..a31106c943d7 100644 >>>> --- a/drivers/mtd/nand/raw/meson_nand.c >>>> +++ b/drivers/mtd/nand/raw/meson_nand.c >>>> @@ -755,6 +755,30 @@ static int __meson_nfc_read_oob(struct nand_chip *nand, int page, >>>> u32 oob_bytes; >>>> u32 page_size; >>>> int ret; >>>> + int i; >>>> + >>>> + /* Read ECC codes and user bytes. */ >>>> + for (i = 0; i < nand->ecc.steps; i++) { >>>> + u32 ecc_offs = nand->ecc.size * (i + 1) + >>>> + NFC_OOB_PER_ECC(nand) * i; >>>> + >>>> + ret = nand_read_page_op(nand, page, 0, NULL, 0); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + /* Use temporary buffer, because 'nand_change_read_column_op()' >>>> + * seems work with some alignment, so we can't read data to >>>> + * 'oob_buf' directly. >>> >>> DMA? >> >> Yes I guess, this address passed to exec_op code and used as DMA. > > If your controller uses DMA on exec_op accesses, then yes. Exec_op > reads/writes are usually small enough (or not time sensitive at all if > they are bigger) so it's not required to use DMA there. Anyhow, oob_buf > is suitable for DMA purposes, so I'm a bit surprised you need a bounce > buffer, if that's the only reason. Maybe you need a bounce buffer to > reorganize the data. That would be a much better explanation.
Yes! I remove this temporary buffer, seems my mistake! Without it everything works good, I'll remove it from the next version!
Thanks, Arseniy
> >>>> + */ >>>> + ret = nand_change_read_column_op(nand, ecc_offs, meson_chip->oob_buf, >>>> + NFC_OOB_PER_ECC(nand), false); >>>> + if (ret) >>>> + return ret; >>>> + >>>> + memcpy(oob_buf + i * NFC_OOB_PER_ECC(nand), >>>> + meson_chip->oob_buf, >>>> + NFC_OOB_PER_ECC(nand)); >>>> + } >>>> >>>> oob_bytes = meson_nfc_get_oob_bytes(nand); >>>> >>> >>> >>> Thanks, >>> Miquèl >> >> Thanks, Arseniy > > > Thanks, > Miquèl
| |