Re: [PATCH 3/4] mtd: nand: mxc_nand: support software ECC

[Miquel Raynal]

Hi Miquel,

miquel.raynal@xxxxxxxxxxx wrote on Mon, 6 May 2024 16:05:08 +0200:

> Hi Sascha,
> 
> s.hauer@xxxxxxxxxxxxxx wrote on Wed, 17 Apr 2024 09:13:30 +0200:
> 
> > To support software ECC we still need the driver provided read_oob,
> > read_page_raw and write_page_raw ops, so set them unconditionally
> > no matter which engine_type we use. The OOB layout on the other hand
> > represents the layout the i.MX ECC hardware uses, so set this only
> > when NAND_ECC_ENGINE_TYPE_ON_HOST is in use.
> > 
> > With these changes the driver can be used with software BCH ECC which
> > is useful for NAND chips that require a stronger ECC than the i.MX
> > hardware supports.
> > 
> > Signed-off-by: Sascha Hauer <s.hauer@xxxxxxxxxxxxxx>
> > ---
> >  drivers/mtd/nand/raw/mxc_nand.c | 9 +++++----
> >  1 file changed, 5 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/raw/mxc_nand.c b/drivers/mtd/nand/raw/mxc_nand.c
> > index fc70c65dea268..f44c130dca18d 100644
> > --- a/drivers/mtd/nand/raw/mxc_nand.c
> > +++ b/drivers/mtd/nand/raw/mxc_nand.c
> > @@ -1394,15 +1394,16 @@ static int mxcnd_attach_chip(struct nand_chip *chip)
> >  	chip->ecc.bytes = host->devtype_data->eccbytes;
> >  	host->eccsize = host->devtype_data->eccsize;
> >  	chip->ecc.size = 512;
> > -	mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> > +
> > +	chip->ecc.read_oob = mxc_nand_read_oob;
> > +	chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > +	chip->ecc.write_page_raw = mxc_nand_write_page_raw;

A second thought on this. Maybe you should consider keeping these for
on-host operations only.

The read/write_page_raw operations are supposed to detangle the data
organization to show a proper [all data][all oob] organization to the
user. But of course if the data is stored differently when using
software ECC, you'll expect the implementation to be different (and the
core provides such helpers, even though in your case they use RNDOUT
which is not yet supported).

> >  
> >  	switch (chip->ecc.engine_type) {
> >  	case NAND_ECC_ENGINE_TYPE_ON_HOST:
> > +		mtd_set_ooblayout(mtd, host->devtype_data->ooblayout);
> >  		chip->ecc.read_page = mxc_nand_read_page;
> > -		chip->ecc.read_page_raw = mxc_nand_read_page_raw;
> > -		chip->ecc.read_oob = mxc_nand_read_oob;
> >  		chip->ecc.write_page = mxc_nand_write_page_ecc;
> > -		chip->ecc.write_page_raw = mxc_nand_write_page_raw;
> >  		chip->ecc.write_oob = mxc_nand_write_oob;
> >  		break;  
> 
> You also need to disable the ECC engine by default (and then you're
> free to use the raw page helpers).
> 
> I thought patch 4 was needed for this patch to work, do you confirm?
> Otherwise with the little change requested I might also merge this one.
> 
> Thanks, Miquèl