Re: [PATCH v2 15/20] mtd: spinand: Create direct mapping descriptors for ECC operations

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Boris,

boris.brezillon@collabora.com wrote on Fri, 26 Nov 2021 15:13:52 +0100:

> On Fri, 26 Nov 2021 12:39:19 +0100
> Miquel Raynal <miquel.raynal@bootlin.com> wrote:
> 
> > In order for pipelined ECC engines to be able to enable/disable the ECC
> > engine only when needed and avoid races when future parallel-operations
> > will be supported, we need to provide the information about the use of
> > the ECC engine in the direct mapping hooks. As direct mapping
> > configurations are meant to be static, it is best to create two new
> > mappings: one for regular 'raw' accesses and one for accesses involving
> > correction. It is up to the driver to use or not the new ECC enable
> > boolean contained in the spi-mem operation.
> > 
> > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com>
> > ---
> >  drivers/mtd/nand/spi/core.c | 28 ++++++++++++++++++++++++++--
> >  include/linux/mtd/spinand.h |  2 ++
> >  include/linux/spi/spi-mem.h |  3 +++  
> 
> I'd split that patch in 2: one adding the ecc_en field to spi_mem_op
> and the other one using it in spinand.

As this new spi_mem_op could not be used without the SPI-NAND changes
I decided to put everything in a single patch, but no problem, I'll
split it.

> 
> >  3 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c
> > index 7027c09925e2..10ccffb6bf0d 100644
> > --- a/drivers/mtd/nand/spi/core.c
> > +++ b/drivers/mtd/nand/spi/core.c
> > @@ -381,7 +381,10 @@ static int spinand_read_from_cache_op(struct spinand_device *spinand,
> >  		}
> >  	}
> >  
> > -	rdesc = spinand->dirmaps[req->pos.plane].rdesc;
> > +	if (req->mode == MTD_OPS_RAW)
> > +		rdesc = spinand->dirmaps[req->pos.plane].rdesc;
> > +	else
> > +		rdesc = spinand->dirmaps[req->pos.plane].rdesc_ecc;
> >  
> >  	while (nbytes) {
> >  		ret = spi_mem_dirmap_read(rdesc, column, nbytes, buf);
> > @@ -452,7 +455,10 @@ static int spinand_write_to_cache_op(struct spinand_device *spinand,
> >  			       req->ooblen);
> >  	}
> >  
> > -	wdesc = spinand->dirmaps[req->pos.plane].wdesc;
> > +	if (req->mode == MTD_OPS_RAW)
> > +		wdesc = spinand->dirmaps[req->pos.plane].wdesc;
> > +	else
> > +		wdesc = spinand->dirmaps[req->pos.plane].wdesc_ecc;
> >  
> >  	while (nbytes) {
> >  		ret = spi_mem_dirmap_write(wdesc, column, nbytes, buf);
> > @@ -866,6 +872,24 @@ static int spinand_create_dirmap(struct spinand_device *spinand,
> >  
> >  	spinand->dirmaps[plane].rdesc = desc;
> >  
> > +	info.op_tmpl = *spinand->op_templates.update_cache;
> > +	info.op_tmpl.ecc_en = true;
> > +	desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
> > +					  spinand->spimem, &info);
> > +	if (IS_ERR(desc))
> > +		return PTR_ERR(desc);
> > +
> > +	spinand->dirmaps[plane].wdesc_ecc = desc;
> > +
> > +	info.op_tmpl = *spinand->op_templates.read_cache;
> > +	info.op_tmpl.ecc_en = true;
> > +	desc = devm_spi_mem_dirmap_create(&spinand->spimem->spi->dev,
> > +					  spinand->spimem, &info);
> > +	if (IS_ERR(desc))
> > +		return PTR_ERR(desc);
> > +
> > +	spinand->dirmaps[plane].rdesc_ecc = desc;
> > +  
> 
> Direct mappings are not free (they might reserve a piece of MMIO
> address space depending on the spi-mem controller implementation), so
> I'd recommend creating those mapping only when strictly needed, that
> is, when dealing with a pipelined ECC.

That's what I tried to do in the first place, but unfortunately the
direct mappings are instantiated at the time the SPI controller gets
probed, which happens before the NAND flash being initialized and its
ECC engine being picked. Hence, at this point, we don't really know what
engine will be used with this ECC engine. I finally gave up and ended
up creating 4 dirmaps (per lun). I know it's not optimal and in most
cases consume more address space than needed but couldn't find a
better solution yet :/

> > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h
> > index 6988956b8492..3aa28240a77f 100644
> > --- a/include/linux/mtd/spinand.h
> > +++ b/include/linux/mtd/spinand.h
> > @@ -389,6 +389,8 @@ struct spinand_info {
> >  struct spinand_dirmap {
> >  	struct spi_mem_dirmap_desc *wdesc;
> >  	struct spi_mem_dirmap_desc *rdesc;
> > +	struct spi_mem_dirmap_desc *wdesc_ecc;
> > +	struct spi_mem_dirmap_desc *rdesc_ecc;
> >  };
> >  
> >  /**
> > diff --git a/include/linux/spi/spi-mem.h b/include/linux/spi/spi-mem.h
> > index 85e2ff7b840d..3be594be24c0 100644
> > --- a/include/linux/spi/spi-mem.h
> > +++ b/include/linux/spi/spi-mem.h
> > @@ -94,6 +94,7 @@ enum spi_mem_data_dir {
> >   *		 operation does not involve transferring data
> >   * @data.buf.in: input buffer (must be DMA-able)
> >   * @data.buf.out: output buffer (must be DMA-able)
> > + * @ecc_en: error correction is required
> >   */
> >  struct spi_mem_op {
> >  	struct {
> > @@ -126,6 +127,8 @@ struct spi_mem_op {
> >  			const void *out;
> >  		} buf;
> >  	} data;
> > +
> > +	bool ecc_en;
> >  };
> >  
> >  #define SPI_MEM_OP(__cmd, __addr, __dummy, __data)		\  
> 

Thanks,
Miquèl