| Message ID | 20211207093422.166934-18-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | External ECC engines & Macronix support | expand |
On Tue, 7 Dec 2021 10:34:17 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > /** > 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; > }; I really think this should be in it's own commit. And you need to make sure all existing drivers reject operation that have ecc_en set to true (that shouldn't be too complicated since most of them use generic helpers to do the check).
Hi Boris, boris.brezillon@collabora.com wrote on Tue, 7 Dec 2021 10:46:27 +0100: > On Tue, 7 Dec 2021 10:34:17 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > /** > > 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; > > }; > > I really think this should be in it's own commit. Oh crap, I forgot about that. > And you need to make > sure all existing drivers reject operation that have ecc_en set to > true (that shouldn't be too complicated since most of them use generic > helpers to do the check). Yes, I can add this check as well. I will propose a first patch creating a spi-mem helper to do generic checks, given a number of additional parameters (dtr, ecc). The current helpers will call this generic function depending on what they check (support for dtr or not). Then I will export this generic helper and let the drivers use it directly if needed, in order to avoid the explosion of helpers in the core to check all the possible combinations. I'll certainly propose a v4 just with these patches and then apply them within the v3 if everyone agrees with the rest. Thanks, Miquèl
diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c index 715cad26fdef..eb999e47e978 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,31 @@ static int spinand_create_dirmap(struct spinand_device *spinand, spinand->dirmaps[plane].rdesc = desc; + if (nand->ecc.engine->integration != NAND_ECC_ENGINE_INTEGRATION_PIPELINED) { + spinand->dirmaps[plane].wdesc_ecc = spinand->dirmaps[plane].wdesc; + spinand->dirmaps[plane].rdesc_ecc = spinand->dirmaps[plane].rdesc; + + return 0; + } + + 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; + return 0; } 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) \
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. As dirmaps are not free (they consume a few pages of MMIO address space) and because these extra entries are only meant to be used by pipelined engines, let's limit their use to this specific type of engine and save a bit of memory with all the other setups. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/nand/spi/core.c | 35 +++++++++++++++++++++++++++++++++-- include/linux/mtd/spinand.h | 2 ++ include/linux/spi/spi-mem.h | 3 +++ 3 files changed, 38 insertions(+), 2 deletions(-)