| Message ID | 20190304222841.13899-1-miquel.raynal@bootlin.com |
|---|---|
| Headers | show |
| Series | Introduce the generic ECC engine abstraction | expand |
On Mon, 4 Mar 2019 23:28:06 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > This structure might be used earlier in this file, let's move the > forward declaration at the top. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > include/linux/mtd/nand.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index cebc38b6d6f5..30f0fb02abe2 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -12,6 +12,8 @@ > > #include <linux/mtd/mtd.h> > > +struct nand_device; > + > /** > * struct nand_memory_organization - Memory organization structure > * @bits_per_cell: number of bits per NAND cell > @@ -133,8 +135,6 @@ struct nand_bbt { > unsigned long *cache; > }; > > -struct nand_device; > - > /** > * struct nand_ops - NAND operations > * @erase: erase a specific block. No need to check if the block is bad before
On Mon, 4 Mar 2019 23:28:07 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Use an extra level in Kconfig for all NAND related entries. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/mtd/nand/Kconfig | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/mtd/nand/Kconfig b/drivers/mtd/nand/Kconfig > index 495751ed3fd7..d2ef8b89568e 100644 > --- a/drivers/mtd/nand/Kconfig > +++ b/drivers/mtd/nand/Kconfig > @@ -1,6 +1,10 @@ > +menu "NAND" > + > config MTD_NAND_CORE > tristate > > source "drivers/mtd/nand/onenand/Kconfig" > source "drivers/mtd/nand/raw/Kconfig" > source "drivers/mtd/nand/spi/Kconfig" > + > +endmenu
On Mon, 4 Mar 2019 23:28:08 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Both oneNAND and raw NAND bits can be compiled if MTD is enabled ^ OneNAND ^ can't ^ disabled > because of the if/endif logic in drivers/mtd/Kconfig. There is no need > for an extra "depends on MTD" in their respective Kconfig files. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> With this addressed Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > --- > drivers/mtd/nand/onenand/Kconfig | 1 - > drivers/mtd/nand/raw/Kconfig | 1 - > 2 files changed, 2 deletions(-) > > diff --git a/drivers/mtd/nand/onenand/Kconfig b/drivers/mtd/nand/onenand/Kconfig > index 9dc15748947b..c168f3b4b296 100644 > --- a/drivers/mtd/nand/onenand/Kconfig > +++ b/drivers/mtd/nand/onenand/Kconfig > @@ -1,6 +1,5 @@ > menuconfig MTD_ONENAND > tristate "OneNAND Device Support" > - depends on MTD > depends on HAS_IOMEM > help > This enables support for accessing all type of OneNAND flash > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index 510a6b32820d..ebb8a3da9fa5 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -11,7 +11,6 @@ config MTD_NAND_ECC_SW_HAMMING_SMC > > menuconfig MTD_RAW_NAND > tristate "Raw/Parallel NAND Device Support" > - depends on MTD > select MTD_NAND_ECC_SW_HAMMING > help > This enables support for accessing all type of raw/parallel
On Mon, 4 Mar 2019 23:28:09 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Before introducing the generic ECC engine abstraction, we need to be > sure that raw NAND and SPI-NAND speak the same language: let's use the > generic NAND layer with the raw NAND part (the SPI-NAND one already > uses it). Nit: I'm not sure it's useful to compile/depends on the generic NAND layer until you actually start using it. I would have selected MTD_NAND_CORE in the patch that starts using some of the functions exposed there, but that's just a tiny detail. Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/raw/Kconfig | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/mtd/nand/raw/Kconfig b/drivers/mtd/nand/raw/Kconfig > index ebb8a3da9fa5..952dc50a8634 100644 > --- a/drivers/mtd/nand/raw/Kconfig > +++ b/drivers/mtd/nand/raw/Kconfig > @@ -11,6 +11,7 @@ config MTD_NAND_ECC_SW_HAMMING_SMC > > menuconfig MTD_RAW_NAND > tristate "Raw/Parallel NAND Device Support" > + select MTD_NAND_CORE > select MTD_NAND_ECC_SW_HAMMING > help > This enables support for accessing all type of raw/parallel
On Mon, 4 Mar 2019 23:28:10 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Use an enum to differentiate the type of I/O (reading or writing a > page). Also update the request iterator. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/spi/core.c | 4 ++-- > include/linux/mtd/nand.h | 13 +++++++++++-- > 2 files changed, 13 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/spi/core.c b/drivers/mtd/nand/spi/core.c > index ed5e340dff51..9ee192585854 100644 > --- a/drivers/mtd/nand/spi/core.c > +++ b/drivers/mtd/nand/spi/core.c > @@ -558,7 +558,7 @@ static int spinand_mtd_read(struct mtd_info *mtd, loff_t from, > > mutex_lock(&spinand->lock); > > - nanddev_io_for_each_page(nand, from, ops, &iter) { > + nanddev_io_for_each_page(nand, NAND_PAGE_READ, from, ops, &iter) { > ret = spinand_select_target(spinand, iter.req.pos.target); > if (ret) > break; > @@ -606,7 +606,7 @@ static int spinand_mtd_write(struct mtd_info *mtd, loff_t to, > > mutex_lock(&spinand->lock); > > - nanddev_io_for_each_page(nand, to, ops, &iter) { > + nanddev_io_for_each_page(nand, NAND_PAGE_WRITE, to, ops, &iter) { > ret = spinand_select_target(spinand, iter.req.pos.target); > if (ret) > break; > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 30f0fb02abe2..84ab76f34c74 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -82,8 +82,14 @@ struct nand_pos { > unsigned int page; > }; > > +enum nand_page_io_req_type { > + NAND_PAGE_READ = 0, > + NAND_PAGE_WRITE, > +}; Please add a kernel doc header. Once done you can add Reviewed-by: Boris Brezillon <boris.brezillon@collabora.com> > + > /** > * struct nand_page_io_req - NAND I/O request object > + * @type: the type of page I/O: read or write > * @pos: the position this I/O request is targeting > * @dataoffs: the offset within the page > * @datalen: number of data bytes to read from/write to this page > @@ -99,6 +105,7 @@ struct nand_pos { > * specific commands/operations. > */ > struct nand_page_io_req { > + enum nand_page_io_req_type type; > struct nand_pos pos; > unsigned int dataoffs; > unsigned int datalen; > @@ -624,11 +631,13 @@ static inline void nanddev_pos_next_page(struct nand_device *nand, > * layer. > */ > static inline void nanddev_io_iter_init(struct nand_device *nand, > + enum nand_page_io_req_type reqtype, > loff_t offs, struct mtd_oob_ops *req, > struct nand_io_iter *iter) > { > struct mtd_info *mtd = nanddev_to_mtd(nand); > > + iter->req.type = reqtype; > iter->req.mode = req->mode; > iter->req.dataoffs = nanddev_offs_to_pos(nand, offs, &iter->req.pos); > iter->req.ooboffs = req->ooboffs; > @@ -698,8 +707,8 @@ static inline bool nanddev_io_iter_end(struct nand_device *nand, > * > * Should be used for iterate over pages that are contained in an MTD request. > */ > -#define nanddev_io_for_each_page(nand, start, req, iter) \ > - for (nanddev_io_iter_init(nand, start, req, iter); \ > +#define nanddev_io_for_each_page(nand, type, start, req, iter) \ > + for (nanddev_io_iter_init(nand, type, start, req, iter); \ > !nanddev_io_iter_end(nand, iter); \ > nanddev_io_iter_next_page(nand, iter)) >
On Mon, 4 Mar 2019 23:28:11 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Prepare the migration to a generic ECC engine by renaming the > nand_ecc_req structure into nand_ecc_conf. This structure will be the > base of a wider 'nand_ecc' structure. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/linux/mtd/nand.h | 8 ++++---- > include/linux/mtd/spinand.h | 2 +- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 84ab76f34c74..78cf905083c9 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -123,11 +123,11 @@ struct nand_page_io_req { > }; > > /** > - * struct nand_ecc_req - NAND ECC requirements > + * struct nand_ecc_conf - NAND ECC configuration Maybe nand_ecc_info, nand_ecc_caps or nand_ecc_props would be more appropriate as _conf seems to imply you can change it, which is not the case when you use this struct to express chip requirements or when you use an on-die ECC which is not configurable. > * @strength: ECC strength > - * @step_size: ECC step/block size > + * @step_size: Number of bytes per step > */ > -struct nand_ecc_req { > +struct nand_ecc_conf { > unsigned int strength; > unsigned int step_size; > }; > @@ -186,7 +186,7 @@ struct nand_ops { > struct nand_device { > struct mtd_info mtd; > struct nand_memory_organization memorg; > - struct nand_ecc_req eccreq; > + struct nand_ecc_conf eccreq; > struct nand_row_converter rowconv; > struct nand_bbt bbt; > const struct nand_ops *ops; > diff --git a/include/linux/mtd/spinand.h b/include/linux/mtd/spinand.h > index b92e2aa955b6..3008de54f958 100644 > --- a/include/linux/mtd/spinand.h > +++ b/include/linux/mtd/spinand.h > @@ -263,7 +263,7 @@ struct spinand_info { > u8 devid; > u32 flags; > struct nand_memory_organization memorg; > - struct nand_ecc_req eccreq; > + struct nand_ecc_conf eccreq; > struct spinand_ecc_info eccinfo; > struct { > const struct spinand_op_variants *read_cache;
On Mon, 4 Mar 2019 23:28:16 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Fix a probable copy/paste error: the function works like > mtd_ooblayout_set_bytes(), not the *_get_bytes() alternate. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/mtdcore.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > index 21e3cdc04036..6ed48018163c 100644 > --- a/drivers/mtd/mtdcore.c > +++ b/drivers/mtd/mtdcore.c > @@ -1592,7 +1592,7 @@ EXPORT_SYMBOL_GPL(mtd_ooblayout_get_databytes); > * @start: first ECC byte to set > * @nbytes: number of ECC bytes to set Looks like the parameter descriptions are wrong too. > * > - * Works like mtd_ooblayout_get_bytes(), except it acts on free bytes. > + * Works like mtd_ooblayout_set_bytes(), except it acts on free bytes. > * > * Returns zero on success, a negative error code otherwise. > */
On Mon, 4 Mar 2019 23:28:12 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > In new code, the use of typedef is discouraged. Before moving this > section out of the raw NAND base, let's switch the nand_ecc_modes_t > type into a regular nand_ecc_mode enumeration. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/raw/nand_base.c | 4 ++-- > include/linux/mtd/rawnand.h | 6 +++--- > include/linux/platform_data/mtd-davinci.h | 2 +- > include/linux/platform_data/mtd-nand-s3c2410.h | 2 +- > 4 files changed, 7 insertions(+), 7 deletions(-) > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > index e14f02a01efd..05174c6a3099 100644 > --- a/drivers/mtd/nand/raw/nand_base.c > +++ b/drivers/mtd/nand/raw/nand_base.c > @@ -4881,8 +4881,8 @@ static int of_get_nand_ecc_mode(struct device_node *np) > > /* > * For backward compatibility we support few obsoleted values that don't > - * have their mappings into nand_ecc_modes_t anymore (they were merged > - * with other enums). > + * have their mappings into the nand_ecc_mode enum anymore (they were > + * merged with other enums). > */ > if (!strcasecmp(pm, "soft_bch")) > return NAND_ECC_SOFT; > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > index 14748183508b..c5bf6bb49329 100644 > --- a/include/linux/mtd/rawnand.h > +++ b/include/linux/mtd/rawnand.h > @@ -87,14 +87,14 @@ struct nand_chip; > /* > * Constants for ECC_MODES > */ > -typedef enum { > +enum nand_ecc_mode { > NAND_ECC_NONE, > NAND_ECC_SOFT, > NAND_ECC_HW, > NAND_ECC_HW_SYNDROME, > NAND_ECC_HW_OOB_FIRST, > NAND_ECC_ON_DIE, > -} nand_ecc_modes_t; > +}; Hm, I'm really not a big fan of this enum because it's mixing 2 different concepts: the type of ECC engine to use (on-die, hw-controller-side, software, no-ECC) and the layout of ECC/FREE bytes (_SYNDROME, _OOB_FIRST). I'd recommend creating a nand_ecc_engine_type enum: enum nand_ecc_engine_type { NAND_NO_ECC_ENGINE, NAND_SOFT_ECC_ENGINE, NAND_HW_ECC_ENGINE, NAND_ON_DIE_ECC_ENGINE, }; and then convert the raw NAND layer to this enum when the time comes. > > enum nand_ecc_algo { > NAND_ECC_UNKNOWN, > @@ -340,7 +340,7 @@ static const struct nand_ecc_caps __name = { \ > * @write_oob: function to write chip OOB data > */ > struct nand_ecc_ctrl { > - nand_ecc_modes_t mode; > + enum nand_ecc_mode mode; > enum nand_ecc_algo algo; > int steps; > int size; > diff --git a/include/linux/platform_data/mtd-davinci.h b/include/linux/platform_data/mtd-davinci.h > index 1bbfa27cccb4..e7457be12b8f 100644 > --- a/include/linux/platform_data/mtd-davinci.h > +++ b/include/linux/platform_data/mtd-davinci.h > @@ -81,7 +81,7 @@ struct davinci_nand_pdata { /* platform_data */ > * Newer ones also support 4-bit ECC, but are awkward > * using it with large page chips. > */ > - nand_ecc_modes_t ecc_mode; > + enum nand_ecc_mode ecc_mode; > u8 ecc_bits; > > /* e.g. NAND_BUSWIDTH_16 */ > diff --git a/include/linux/platform_data/mtd-nand-s3c2410.h b/include/linux/platform_data/mtd-nand-s3c2410.h > index f8c553f92655..ff6501c51244 100644 > --- a/include/linux/platform_data/mtd-nand-s3c2410.h > +++ b/include/linux/platform_data/mtd-nand-s3c2410.h > @@ -52,7 +52,7 @@ struct s3c2410_platform_nand { > > unsigned int ignore_unset_ecc:1; > > - nand_ecc_modes_t ecc_mode; > + enum nand_ecc_mode ecc_mode; > > int nr_sets; > struct s3c2410_nand_set *sets;
On Mon, 4 Mar 2019 23:28:14 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > In the nand_ecc_{mode,algo} enumerations, clarify the fact that the > value 0 will be used for invalid/uninitialized data. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > include/linux/mtd/rawnand.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > index d25e0c07b0ad..95b0c7114701 100644 > --- a/include/linux/mtd/rawnand.h > +++ b/include/linux/mtd/rawnand.h > @@ -88,7 +88,7 @@ struct nand_chip; > * Constants for ECC_MODES > */ > enum nand_ecc_mode { > - NAND_ECC_INVALID, > + NAND_ECC_INVALID = 0, > NAND_ECC_NONE, > NAND_ECC_SOFT, > NAND_ECC_HW, > @@ -98,7 +98,7 @@ enum nand_ecc_mode { > }; > > enum nand_ecc_algo { > - NAND_ECC_UNKNOWN, > + NAND_ECC_UNKNOWN = 0, > NAND_ECC_HAMMING, > NAND_ECC_BCH, > NAND_ECC_RS, Do we really need to do that? I think it's pretty common to leave the first entry unassigned when we want things to start at 0...
Hi Boris, Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 31 Mar 2019 13:32:46 +0200: > On Mon, 4 Mar 2019 23:28:16 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Fix a probable copy/paste error: the function works like > > mtd_ooblayout_set_bytes(), not the *_get_bytes() alternate. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/mtd/mtdcore.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/mtd/mtdcore.c b/drivers/mtd/mtdcore.c > > index 21e3cdc04036..6ed48018163c 100644 > > --- a/drivers/mtd/mtdcore.c > > +++ b/drivers/mtd/mtdcore.c > > @@ -1592,7 +1592,7 @@ EXPORT_SYMBOL_GPL(mtd_ooblayout_get_databytes); > > * @start: first ECC byte to set > > * @nbytes: number of ECC bytes to set > > Looks like the parameter descriptions are wrong too. Are you sure? I don't see where. > > > * > > - * Works like mtd_ooblayout_get_bytes(), except it acts on free bytes. > > + * Works like mtd_ooblayout_set_bytes(), except it acts on free bytes. > > * > > * Returns zero on success, a negative error code otherwise. > > */ > Thanks, Miquèl
Hi Boris, Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 31 Mar 2019 13:30:01 +0200: > On Mon, 4 Mar 2019 23:28:11 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Prepare the migration to a generic ECC engine by renaming the > > nand_ecc_req structure into nand_ecc_conf. This structure will be the > > base of a wider 'nand_ecc' structure. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > include/linux/mtd/nand.h | 8 ++++---- > > include/linux/mtd/spinand.h | 2 +- > > 2 files changed, 5 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > > index 84ab76f34c74..78cf905083c9 100644 > > --- a/include/linux/mtd/nand.h > > +++ b/include/linux/mtd/nand.h > > @@ -123,11 +123,11 @@ struct nand_page_io_req { > > }; > > > > /** > > - * struct nand_ecc_req - NAND ECC requirements > > + * struct nand_ecc_conf - NAND ECC configuration > > Maybe nand_ecc_info, nand_ecc_caps or nand_ecc_props would be more > appropriate as _conf seems to imply you can change it, which is not > the case when you use this struct to express chip requirements or when > you use an on-die ECC which is not configurable. nand_ecc_props sounds good to me! Thanks, Miquèl
Hi Boris, Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 31 Mar 2019 13:55:13 +0200: > On Mon, 4 Mar 2019 23:28:12 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > In new code, the use of typedef is discouraged. Before moving this > > section out of the raw NAND base, let's switch the nand_ecc_modes_t > > type into a regular nand_ecc_mode enumeration. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/mtd/nand/raw/nand_base.c | 4 ++-- > > include/linux/mtd/rawnand.h | 6 +++--- > > include/linux/platform_data/mtd-davinci.h | 2 +- > > include/linux/platform_data/mtd-nand-s3c2410.h | 2 +- > > 4 files changed, 7 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > index e14f02a01efd..05174c6a3099 100644 > > --- a/drivers/mtd/nand/raw/nand_base.c > > +++ b/drivers/mtd/nand/raw/nand_base.c > > @@ -4881,8 +4881,8 @@ static int of_get_nand_ecc_mode(struct device_node *np) > > > > /* > > * For backward compatibility we support few obsoleted values that don't > > - * have their mappings into nand_ecc_modes_t anymore (they were merged > > - * with other enums). > > + * have their mappings into the nand_ecc_mode enum anymore (they were > > + * merged with other enums). > > */ > > if (!strcasecmp(pm, "soft_bch")) > > return NAND_ECC_SOFT; > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > > index 14748183508b..c5bf6bb49329 100644 > > --- a/include/linux/mtd/rawnand.h > > +++ b/include/linux/mtd/rawnand.h > > @@ -87,14 +87,14 @@ struct nand_chip; > > /* > > * Constants for ECC_MODES > > */ > > -typedef enum { > > +enum nand_ecc_mode { > > NAND_ECC_NONE, > > NAND_ECC_SOFT, > > NAND_ECC_HW, > > NAND_ECC_HW_SYNDROME, > > NAND_ECC_HW_OOB_FIRST, > > NAND_ECC_ON_DIE, > > -} nand_ecc_modes_t; > > +}; > > Hm, I'm really not a big fan of this enum because it's mixing 2 > different concepts: the type of ECC engine to use (on-die, > hw-controller-side, software, no-ECC) and the layout of > ECC/FREE bytes (_SYNDROME, _OOB_FIRST). > > I'd recommend creating a nand_ecc_engine_type enum: > > enum nand_ecc_engine_type { > NAND_NO_ECC_ENGINE, > NAND_SOFT_ECC_ENGINE, > NAND_HW_ECC_ENGINE, > NAND_ON_DIE_ECC_ENGINE, > }; > > and then convert the raw NAND layer to this enum when the time comes. I started something but this goes way too far from what I want to achieve. I know it would be nice to have it but it has an increasingly number of side effects which scared me. The way the series is organized does not allow to easily ignore the raw NAND layer first and then convert it. I am giving up on this one for now, sorry. Thanks, Miquèl
On Fri, 3 May 2019 14:40:30 +0200 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 31 Mar > 2019 13:55:13 +0200: > > > On Mon, 4 Mar 2019 23:28:12 +0100 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > In new code, the use of typedef is discouraged. Before moving this > > > section out of the raw NAND base, let's switch the nand_ecc_modes_t > > > type into a regular nand_ecc_mode enumeration. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/mtd/nand/raw/nand_base.c | 4 ++-- > > > include/linux/mtd/rawnand.h | 6 +++--- > > > include/linux/platform_data/mtd-davinci.h | 2 +- > > > include/linux/platform_data/mtd-nand-s3c2410.h | 2 +- > > > 4 files changed, 7 insertions(+), 7 deletions(-) > > > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > > index e14f02a01efd..05174c6a3099 100644 > > > --- a/drivers/mtd/nand/raw/nand_base.c > > > +++ b/drivers/mtd/nand/raw/nand_base.c > > > @@ -4881,8 +4881,8 @@ static int of_get_nand_ecc_mode(struct device_node *np) > > > > > > /* > > > * For backward compatibility we support few obsoleted values that don't > > > - * have their mappings into nand_ecc_modes_t anymore (they were merged > > > - * with other enums). > > > + * have their mappings into the nand_ecc_mode enum anymore (they were > > > + * merged with other enums). > > > */ > > > if (!strcasecmp(pm, "soft_bch")) > > > return NAND_ECC_SOFT; > > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > > > index 14748183508b..c5bf6bb49329 100644 > > > --- a/include/linux/mtd/rawnand.h > > > +++ b/include/linux/mtd/rawnand.h > > > @@ -87,14 +87,14 @@ struct nand_chip; > > > /* > > > * Constants for ECC_MODES > > > */ > > > -typedef enum { > > > +enum nand_ecc_mode { > > > NAND_ECC_NONE, > > > NAND_ECC_SOFT, > > > NAND_ECC_HW, > > > NAND_ECC_HW_SYNDROME, > > > NAND_ECC_HW_OOB_FIRST, > > > NAND_ECC_ON_DIE, > > > -} nand_ecc_modes_t; > > > +}; > > > > Hm, I'm really not a big fan of this enum because it's mixing 2 > > different concepts: the type of ECC engine to use (on-die, > > hw-controller-side, software, no-ECC) and the layout of > > ECC/FREE bytes (_SYNDROME, _OOB_FIRST). > > > > I'd recommend creating a nand_ecc_engine_type enum: > > > > enum nand_ecc_engine_type { > > NAND_NO_ECC_ENGINE, > > NAND_SOFT_ECC_ENGINE, > > NAND_HW_ECC_ENGINE, > > NAND_ON_DIE_ECC_ENGINE, > > }; > > > > and then convert the raw NAND layer to this enum when the time comes. > > I started something but this goes way too far from what I want to > achieve. I know it would be nice to have it but it has an > increasingly number of side effects which scared me. The way the > series is organized does not allow to easily ignore the raw NAND layer > first and then convert it. I am giving up on this one for now, sorry. I don't think that's a wise choice. Why not focusing on nand_device/spinand with a clean implementation that does not try to convert the rawnand layer to this approach? The reason I initially started over with a new generic NAND layer instead of copying things from nand_base is that I wanted to avoid having to deal with stuff that were not so great in there, and clearly nand_ecc_mode is one of them.
Hi Boris, Boris Brezillon <boris.brezillon@collabora.com> wrote on Fri, 3 May 2019 14:50:06 +0200: > On Fri, 3 May 2019 14:40:30 +0200 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Hi Boris, > > > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Sun, 31 Mar > > 2019 13:55:13 +0200: > > > > > On Mon, 4 Mar 2019 23:28:12 +0100 > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > In new code, the use of typedef is discouraged. Before moving this > > > > section out of the raw NAND base, let's switch the nand_ecc_modes_t > > > > type into a regular nand_ecc_mode enumeration. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/mtd/nand/raw/nand_base.c | 4 ++-- > > > > include/linux/mtd/rawnand.h | 6 +++--- > > > > include/linux/platform_data/mtd-davinci.h | 2 +- > > > > include/linux/platform_data/mtd-nand-s3c2410.h | 2 +- > > > > 4 files changed, 7 insertions(+), 7 deletions(-) > > > > > > > > diff --git a/drivers/mtd/nand/raw/nand_base.c b/drivers/mtd/nand/raw/nand_base.c > > > > index e14f02a01efd..05174c6a3099 100644 > > > > --- a/drivers/mtd/nand/raw/nand_base.c > > > > +++ b/drivers/mtd/nand/raw/nand_base.c > > > > @@ -4881,8 +4881,8 @@ static int of_get_nand_ecc_mode(struct device_node *np) > > > > > > > > /* > > > > * For backward compatibility we support few obsoleted values that don't > > > > - * have their mappings into nand_ecc_modes_t anymore (they were merged > > > > - * with other enums). > > > > + * have their mappings into the nand_ecc_mode enum anymore (they were > > > > + * merged with other enums). > > > > */ > > > > if (!strcasecmp(pm, "soft_bch")) > > > > return NAND_ECC_SOFT; > > > > diff --git a/include/linux/mtd/rawnand.h b/include/linux/mtd/rawnand.h > > > > index 14748183508b..c5bf6bb49329 100644 > > > > --- a/include/linux/mtd/rawnand.h > > > > +++ b/include/linux/mtd/rawnand.h > > > > @@ -87,14 +87,14 @@ struct nand_chip; > > > > /* > > > > * Constants for ECC_MODES > > > > */ > > > > -typedef enum { > > > > +enum nand_ecc_mode { > > > > NAND_ECC_NONE, > > > > NAND_ECC_SOFT, > > > > NAND_ECC_HW, > > > > NAND_ECC_HW_SYNDROME, > > > > NAND_ECC_HW_OOB_FIRST, > > > > NAND_ECC_ON_DIE, > > > > -} nand_ecc_modes_t; > > > > +}; > > > > > > Hm, I'm really not a big fan of this enum because it's mixing 2 > > > different concepts: the type of ECC engine to use (on-die, > > > hw-controller-side, software, no-ECC) and the layout of > > > ECC/FREE bytes (_SYNDROME, _OOB_FIRST). > > > > > > I'd recommend creating a nand_ecc_engine_type enum: > > > > > > enum nand_ecc_engine_type { > > > NAND_NO_ECC_ENGINE, > > > NAND_SOFT_ECC_ENGINE, > > > NAND_HW_ECC_ENGINE, > > > NAND_ON_DIE_ECC_ENGINE, > > > }; > > > > > > and then convert the raw NAND layer to this enum when the time comes. > > > > I started something but this goes way too far from what I want to > > achieve. I know it would be nice to have it but it has an > > increasingly number of side effects which scared me. The way the > > series is organized does not allow to easily ignore the raw NAND layer > > first and then convert it. I am giving up on this one for now, sorry. > > I don't think that's a wise choice. Why not focusing on > nand_device/spinand with a clean implementation that does not try to > convert the rawnand layer to this approach? I know, and that was probably the best thing to do, unfortunately I started bigger because there was so much things to change, I got a bit lost at the beginning. I know this is not a valid reason but in practice I could not come with something lighter, ie. focused on SPI-NAND only. > The reason I initially > started over with a new generic NAND layer instead of copying things > from nand_base is that I wanted to avoid having to deal with stuff that > were not so great in there, and clearly nand_ecc_mode is one of them. I understand that so I think I managed to clean the situation with a preliminary series which I will send soon. Thanks, Miquèl
As of today, only raw NAND controllers used to feature an integrated ECC engine and so controller drivers always embedded some code to enable/disable the correction. This statement is no longer correct as SPI-NAND devices might not embed an on-die ECC engine and must make use of an external ECC engine. We figured there are three possible situations for (generic) NAND device: either the engine is 'on-die' (most of the SPI-NANDs, a few raw NANDs), or the engine is part of the host controller (most raw NANDs), or the engine may be external (SPI controllers might feature an ECC engine, or there are still the possibility to use software correction). To solve this situation, this is a proposal on how to make things work. We want to create an ECC engine object which has simple callbacks: * init/cleanup its context * prepare an I/O operation * finish an I/O operation Details about what is going to happen in these callbacks is described in drivers/mtd/nand/ecc/engine.c. The logic in this series is: 1/ Use the generic NAND core for all NAND devices (raw and SPI). 2/ Create the ECC engine interface in drivers/mtd/nand/ecc/ 3/ Move code in driver/mtd/nand/ecc. 4/ Make both software engines (Hamming and BCH) generic, move them in the ecc/ directory, clean them a bit and instantiate ECC engines. Write raw NAND helpers to use these two new engines. 5/ Isolate SPI-NAND on-die ECC engine in its own driver. 6/ Make use from the SPI-NAND layer of all the ECC engines listed above (on user request, people can now make use of soft BCH if they don't have an ECC-engine). This work is still WIP, I expect a few 0-day regressions, maybe the naming is not perfect but it gives an idea of what I would like to introduce. The next steps are: 1/ Migrate the raw NAND core to make a proper use of these ECC engines. 2/ Deprecate in the raw NAND subsystem the interfaces used until now (I expect we should get rid of a lot of boilerplate). 3/ Introduce an external hardware ECC engine driver. Thanks, Miquèl Changes in v2 ============= * SPDX license identifiers for soft BCH and Hamming: the license macro was right, "GPL" means "GPLv2 or higher", so do not change this portion. Also update the commit messages to fit the actual change. * Do not compile-in the NAND core by default, do it only for raw NAND. Remove the dependencies on CONFIG_MTD in a different patch. Also, keep an extra level of hierarchy in Kconfig for the NAND bits by adding a menu instead of a config. * Moved the standard OOB layouts in the ecc/engine.c driver instead of in the NAND core. * Used the nand_ecc_ prefix in most of the engines functions instead of just ecc_, which is now reserved for bare helpers. Get rid of the __ecc prefix. * In the sunxi NAND controller driver: moved the ECC structure from sunxi_nfc to sunxi_nand_chip as the ECC engine is per-chip and not per controller. * Software Hamming ECC engine is only enabled by default if raw NAND is also enabled. NDFC now selects the software Hamming ECC engine (instead of depending on it). * Mention in software BCH and Hamming Kconfig entries that booting from NAND is very likely to fail if the user selects these symbols as modules. * Added Boris Reviewed-by tag on the SPI-NAND typo fixing patch. * Renamed the "mode" into a "provider" entry in the ECC configuration structures. * Moved the "total" entry of the ECC configuration directly in the context structure (should probably not be public but let's keep it as is for now). * Split the generic ECC engine introduction into smaller patches to do some renaming aside. * Drop the "maximize" entry in the ECC engine configuration structure, keep using a flag like before. * Canceled the move of the SPI-NAND specific ECC engine out of the core file. * Amended the root ECC structures to have three nand_ecc_conf structures: one for the defaults, one for the chip requirements, one for the user desires. * Created a *ondie_engine pointer in the nand_ecc structure to save the on-die ECC engine, if any. For instance, saving a reference to this engine is done by the SPI-NAND core. * Dropped the SPI-NAND flag that was used to distinguish between NAND flavors from the NAND core, it should not be needed anymore. * Added an helper in the NAND core to put a reference on an ECC engine. This will be used by the hardware engines only. * Renamed the files ecc/sw-{bch,hamming}.c and their headers include/linux/mtd/nand-ecc-sw-{bch,hamming}-engine.h. * Created a MTD_NAND_ECC invisible Kconfig symbol. * Added plenty of missing EXPORT_SYMBOL{,_GPL}(). * Minor modifications so that everything still compiles even when modules and built-in drivers are mixed in Kconfig in the whole NAND directory. Miquel Raynal (36): mtd: nand: Move nand_device forward declaration to the top mtd: nand: Add an extra level in the Kconfig hierarchy mtd: nand: Drop useless 'depends on' in Kconfig mtd: rawnand: Use the NAND core mtd: nand: Add a NAND page I/O request type mtd: nand: Rename a core structure mtd: rawnand: Avoid a typedef mtd: rawnand: Add an invalid ECC mode to discriminate with valid ones mtd: rawnand: Clarify the values for invalid ECC mode/algo mtd: nand: Introduce the ECC engine abstraction mtd: Fix typo in mtd_ooblayout_set_databytes() description mtd: nand: Move standard OOB layouts to the generic ECC core mtd: nand: Move ECC specific functions to ecc/engine.c mtd: nand: ecc: Move BCH code into the ecc/ directory mtd: nand: ecc: Use SPDX license identifier for the software BCH code mtd: nand: ecc: Turn the software BCH implementation generic mtd: rawnand: Get rid of chip->ecc.priv mtd: nand: ecc: Move Hamming code into the ecc/ directory mtd: nand: ecc: Use SPDX license identifier for the software Hamming code mtd: nand: ecc: Clarify the software Hamming introductory line mtd: nand: ecc: Turn the software Hamming implementation generic mtd: nand: Remove useless include about software Hamming ECC mtd: nand: ecc: Let the software BCH ECC engine be a module mtd: nand: ecc: Let the software Hamming ECC engine be unselected mtd: nand: ecc: Create the software BCH engine instance mtd: nand: ecc: Create the software Hamming engine instance mtd: nand: Let software ECC engines be retrieved from the NAND core mtd: spinand: Fix typo in comment mtd: spinand: Move ECC related definitions earlier in the driver mtd: spinand: Instantiate a SPI-NAND on-die ECC engine mtd: nand: Let on-die ECC engines be retrieved from the NAND core mtd: rawnand: Fill a default ECC provider/algorithm mtd: spinand: Fill a default ECC provider/algorithm mtd: nand: Add helpers to manage ECC engines and configurations mtd: spinand: Use the external ECC engine logic mtd: spinand: Propagate ECC information to the MTD structure arch/arm/mach-s3c24xx/common-smdk.c | 1 - arch/arm/mach-s3c24xx/mach-anubis.c | 1 - arch/arm/mach-s3c24xx/mach-at2440evb.c | 1 - arch/arm/mach-s3c24xx/mach-bast.c | 1 - arch/arm/mach-s3c24xx/mach-gta02.c | 1 - arch/arm/mach-s3c24xx/mach-jive.c | 1 - arch/arm/mach-s3c24xx/mach-mini2440.c | 1 - arch/arm/mach-s3c24xx/mach-osiris.c | 1 - arch/arm/mach-s3c24xx/mach-qt2410.c | 1 - arch/arm/mach-s3c24xx/mach-rx3715.c | 1 - arch/arm/mach-s3c24xx/mach-vstms.c | 1 - drivers/mtd/mtdcore.c | 2 +- drivers/mtd/nand/Kconfig | 5 + drivers/mtd/nand/Makefile | 1 + drivers/mtd/nand/core.c | 124 ++++ drivers/mtd/nand/ecc/Kconfig | 41 ++ drivers/mtd/nand/ecc/Makefile | 5 + drivers/mtd/nand/ecc/engine.c | 481 +++++++++++++++ drivers/mtd/nand/ecc/sw-bch.c | 424 ++++++++++++++ .../nand/{raw/nand_ecc.c => ecc/sw-hamming.c} | 347 ++++++++--- drivers/mtd/nand/onenand/Kconfig | 1 - drivers/mtd/nand/raw/Kconfig | 26 +- drivers/mtd/nand/raw/Makefile | 2 - drivers/mtd/nand/raw/atmel/nand-controller.c | 12 +- drivers/mtd/nand/raw/cs553x_nand.c | 3 +- drivers/mtd/nand/raw/denali.c | 3 + drivers/mtd/nand/raw/denali_pci.c | 1 - drivers/mtd/nand/raw/fsl_elbc_nand.c | 1 - drivers/mtd/nand/raw/fsl_ifc_nand.c | 1 - drivers/mtd/nand/raw/fsl_upm.c | 1 - drivers/mtd/nand/raw/fsmc_nand.c | 3 +- drivers/mtd/nand/raw/gpmi-nand/gpmi-nand.c | 12 +- drivers/mtd/nand/raw/lpc32xx_mlc.c | 1 - drivers/mtd/nand/raw/lpc32xx_slc.c | 3 +- drivers/mtd/nand/raw/marvell_nand.c | 7 +- drivers/mtd/nand/raw/mtk_nand.c | 4 +- drivers/mtd/nand/raw/nand_base.c | 551 ++++++------------ drivers/mtd/nand/raw/nand_bch.c | 232 -------- drivers/mtd/nand/raw/nand_esmt.c | 11 +- drivers/mtd/nand/raw/nand_hynix.c | 41 +- drivers/mtd/nand/raw/nand_jedec.c | 4 +- drivers/mtd/nand/raw/nand_micron.c | 14 +- drivers/mtd/nand/raw/nand_onfi.c | 8 +- drivers/mtd/nand/raw/nand_samsung.c | 19 +- drivers/mtd/nand/raw/nand_toshiba.c | 13 +- drivers/mtd/nand/raw/nandsim.c | 3 +- drivers/mtd/nand/raw/ndfc.c | 3 +- drivers/mtd/nand/raw/omap2.c | 32 +- drivers/mtd/nand/raw/pasemi_nand.c | 1 - drivers/mtd/nand/raw/s3c2410.c | 1 - drivers/mtd/nand/raw/sharpsl.c | 3 +- drivers/mtd/nand/raw/sunxi_nand.c | 38 +- drivers/mtd/nand/raw/tegra_nand.c | 12 +- drivers/mtd/nand/raw/tmio_nand.c | 7 +- drivers/mtd/nand/raw/txx9ndfmc.c | 5 +- drivers/mtd/nand/spi/Kconfig | 1 + drivers/mtd/nand/spi/core.c | 301 ++++++---- drivers/mtd/nand/spi/macronix.c | 6 +- drivers/mtd/nand/spi/on-die-ecc-engine.c | 0 drivers/mtd/nand/spi/toshiba.c | 6 +- drivers/mtd/sm_ftl.c | 30 +- drivers/mtd/tests/mtd_nandecctest.c | 31 +- include/linux/mtd/nand-ecc-sw-bch.h | 80 +++ include/linux/mtd/nand-ecc-sw-hamming.h | 99 ++++ include/linux/mtd/nand.h | 150 ++++- include/linux/mtd/nand_bch.h | 69 --- include/linux/mtd/nand_ecc.h | 42 -- include/linux/mtd/rawnand.h | 42 +- include/linux/mtd/sharpsl.h | 1 - include/linux/mtd/spinand.h | 13 +- include/linux/platform_data/mtd-davinci.h | 2 +- .../linux/platform_data/mtd-nand-s3c2410.h | 2 +- 72 files changed, 2240 insertions(+), 1155 deletions(-) create mode 100644 drivers/mtd/nand/ecc/Kconfig create mode 100644 drivers/mtd/nand/ecc/Makefile create mode 100644 drivers/mtd/nand/ecc/engine.c create mode 100644 drivers/mtd/nand/ecc/sw-bch.c rename drivers/mtd/nand/{raw/nand_ecc.c => ecc/sw-hamming.c} (59%) delete mode 100644 drivers/mtd/nand/raw/nand_bch.c create mode 100644 drivers/mtd/nand/spi/on-die-ecc-engine.c create mode 100644 include/linux/mtd/nand-ecc-sw-bch.h create mode 100644 include/linux/mtd/nand-ecc-sw-hamming.h delete mode 100644 include/linux/mtd/nand_bch.h delete mode 100644 include/linux/mtd/nand_ecc.h