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

[Viacheslav]

Hi, Miquel!

24/11/2023 12.06, Miquel Raynal wrote:
> Hi Arseniy,
>
> avkrasnov@xxxxxxxxxxxxxxxxx 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 <avkrasnov@xxxxxxxxxxxxxxxxx>
> > > > > ---
> > > > >   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