| Message ID | 20230515094440.3552094-4-AVKrasnov@sberdevices.ru |
|---|---|
| State | Changes Requested |
| Delegated to: | Miquel Raynal |
| Headers | show |
| Series | refactoring and fix for Meson NAND | expand |
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. > 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? > + */ > + 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
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 > >> 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. > >> + */ >> + 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
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. > >> 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. > >> + */ > >> + 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
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
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. + */ + 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);
This changes size of read access to OOB area by reading all bytes of OOB (free bytes + ECC engine bytes). Signed-off-by: Arseniy Krasnov <AVKrasnov@sberdevices.ru> --- drivers/mtd/nand/raw/meson_nand.c | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)