| Message ID | 20190221125806.28875-8-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | None | expand |
On Thu, 21 Feb 2019 13:57:59 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Before making use of the ECC engines, we must retrieve them. Add the > boilerplate for the ones already available: software engines (Hamming > and BCH). > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/ecc/engine.c | 14 ++++++++++++++ > include/linux/mtd/nand-sw-bch-engine.h | 3 +++ > include/linux/mtd/nand-sw-hamming-engine.h | 3 +++ > include/linux/mtd/nand.h | 3 +++ > 4 files changed, 23 insertions(+) > > diff --git a/drivers/mtd/nand/ecc/engine.c b/drivers/mtd/nand/ecc/engine.c > index 7dd3f9772698..318dbb2d56a2 100644 > --- a/drivers/mtd/nand/ecc/engine.c > +++ b/drivers/mtd/nand/ecc/engine.c > @@ -286,6 +286,20 @@ bool nand_ecc_correction_is_enough(struct nand_device *nand) > return corr >= ds_corr && conf->strength >= reqs->strength; > } > > +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand) What if you want to instantiate SW ECC with a custom layout? Can't we instead have a function that create a new engine dynamically? struct nand_ecc_engine * nand_ecc_create_sw_engine(struct nand_device* nand, enum nand_ecc_algo algo, struct mtd_ooblayout *layout); > +{ > + switch (nand->ecc.user_conf.algo) { Note that the conf is supposed to be passed afterwards, when the ctx is created, so you should check nand->ecc.user_conf directly here. > + case NAND_ECC_HAMMING: > + return ecc_sw_hamming_get_engine(); > + case NAND_ECC_BCH: > + return ecc_sw_bch_get_engine(); > + default: > + break; > + } > + > + return NULL; > +} > + > MODULE_LICENSE("GPL v2"); > MODULE_AUTHOR("Miquel Raynal <miquel.raynal@bootlin.com>"); > MODULE_DESCRIPTION("Generic ECC engine"); > diff --git a/include/linux/mtd/nand-sw-bch-engine.h b/include/linux/mtd/nand-sw-bch-engine.h > index e406aa53ec4e..38cdad117fea 100644 > --- a/include/linux/mtd/nand-sw-bch-engine.h > +++ b/include/linux/mtd/nand-sw-bch-engine.h > @@ -11,6 +11,9 @@ > #include <linux/mtd/nand.h> > #include <linux/bch.h> > > +/* Needed for cross inclusion with nand.h */ > +struct nand_device; > + > /** > * struct ecc_sw_bch_conf - private software BCH ECC engine structure > * @reqooblen: Save the actual user OOB length requested before overwriting it > diff --git a/include/linux/mtd/nand-sw-hamming-engine.h b/include/linux/mtd/nand-sw-hamming-engine.h > index 8df36d189482..51c5a2acee42 100644 > --- a/include/linux/mtd/nand-sw-hamming-engine.h > +++ b/include/linux/mtd/nand-sw-hamming-engine.h > @@ -12,6 +12,9 @@ > > #include <linux/mtd/nand.h> > > +/* Needed for cross inclusion with nand.h */ > +struct nand_device; > + > /** > * struct ecc_sw_hamming_conf - private software Hamming ECC engine structure > * @reqooblen: Save the actual user OOB length requested before overwriting it > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > index 4482eb2bbfd4..3abe113e4f06 100644 > --- a/include/linux/mtd/nand.h > +++ b/include/linux/mtd/nand.h > @@ -11,6 +11,8 @@ > #define __LINUX_MTD_NAND_H > > #include <linux/mtd/mtd.h> > +#include <linux/mtd/nand-sw-hamming-engine.h> > +#include <linux/mtd/nand-sw-bch-engine.h> > > struct nand_device; > > @@ -253,6 +255,7 @@ struct nand_ecc_engine { > }; > > void nand_ecc_read_user_conf(struct nand_device *nand); > +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand); > int nand_ecc_init_ctx(struct nand_device *nand); > void nand_ecc_cleanup_ctx(struct nand_device *nand); > int nand_ecc_prepare_io_req(struct nand_device *nand,
Hi Boris, Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 15:29:57 +0100: > On Thu, 21 Feb 2019 13:57:59 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Before making use of the ECC engines, we must retrieve them. Add the > > boilerplate for the ones already available: software engines (Hamming > > and BCH). > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/mtd/nand/ecc/engine.c | 14 ++++++++++++++ > > include/linux/mtd/nand-sw-bch-engine.h | 3 +++ > > include/linux/mtd/nand-sw-hamming-engine.h | 3 +++ > > include/linux/mtd/nand.h | 3 +++ > > 4 files changed, 23 insertions(+) > > > > diff --git a/drivers/mtd/nand/ecc/engine.c b/drivers/mtd/nand/ecc/engine.c > > index 7dd3f9772698..318dbb2d56a2 100644 > > --- a/drivers/mtd/nand/ecc/engine.c > > +++ b/drivers/mtd/nand/ecc/engine.c > > @@ -286,6 +286,20 @@ bool nand_ecc_correction_is_enough(struct nand_device *nand) > > return corr >= ds_corr && conf->strength >= reqs->strength; > > } > > > > +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand) > > What if you want to instantiate SW ECC with a custom layout? Can't we > instead have a function that create a new engine dynamically? > > struct nand_ecc_engine * > nand_ecc_create_sw_engine(struct nand_device* nand, > enum nand_ecc_algo algo, > struct mtd_ooblayout *layout); > > Right now, for both sw engines, a default layout is applied if there is none at engine initialization time. Also, do we really need a "create" helper? I don't see what's created there. Maybe you had something else in mind, and the ecc_sw_xxx_get_engine() approach do not match what you expected, so please tell me more about your idea, otherwise I don't see what a nand_ecc_create_sw_engine() would bring. > > > +{ > > + switch (nand->ecc.user_conf.algo) { > > Note that the conf is supposed to be passed afterwards, when the ctx is > created, so you should check nand->ecc.user_conf directly here. I think this is what I do so I suspect the above sentence is not what you actually meant? > > > + case NAND_ECC_HAMMING: > > + return ecc_sw_hamming_get_engine(); > > + case NAND_ECC_BCH: > > + return ecc_sw_bch_get_engine(); > > + default: > > + break; > > + } > > + > > + return NULL; > > +} > > + > > MODULE_LICENSE("GPL v2"); > > MODULE_AUTHOR("Miquel Raynal <miquel.raynal@bootlin.com>"); > > MODULE_DESCRIPTION("Generic ECC engine"); > > diff --git a/include/linux/mtd/nand-sw-bch-engine.h b/include/linux/mtd/nand-sw-bch-engine.h > > index e406aa53ec4e..38cdad117fea 100644 > > --- a/include/linux/mtd/nand-sw-bch-engine.h > > +++ b/include/linux/mtd/nand-sw-bch-engine.h > > @@ -11,6 +11,9 @@ > > #include <linux/mtd/nand.h> > > #include <linux/bch.h> > > > > +/* Needed for cross inclusion with nand.h */ > > +struct nand_device; > > + > > /** > > * struct ecc_sw_bch_conf - private software BCH ECC engine structure > > * @reqooblen: Save the actual user OOB length requested before overwriting it > > diff --git a/include/linux/mtd/nand-sw-hamming-engine.h b/include/linux/mtd/nand-sw-hamming-engine.h > > index 8df36d189482..51c5a2acee42 100644 > > --- a/include/linux/mtd/nand-sw-hamming-engine.h > > +++ b/include/linux/mtd/nand-sw-hamming-engine.h > > @@ -12,6 +12,9 @@ > > > > #include <linux/mtd/nand.h> > > > > +/* Needed for cross inclusion with nand.h */ > > +struct nand_device; > > + > > /** > > * struct ecc_sw_hamming_conf - private software Hamming ECC engine structure > > * @reqooblen: Save the actual user OOB length requested before overwriting it > > diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h > > index 4482eb2bbfd4..3abe113e4f06 100644 > > --- a/include/linux/mtd/nand.h > > +++ b/include/linux/mtd/nand.h > > @@ -11,6 +11,8 @@ > > #define __LINUX_MTD_NAND_H > > > > #include <linux/mtd/mtd.h> > > +#include <linux/mtd/nand-sw-hamming-engine.h> > > +#include <linux/mtd/nand-sw-bch-engine.h> > > > > struct nand_device; > > > > @@ -253,6 +255,7 @@ struct nand_ecc_engine { > > }; > > > > void nand_ecc_read_user_conf(struct nand_device *nand); > > +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand); > > int nand_ecc_init_ctx(struct nand_device *nand); > > void nand_ecc_cleanup_ctx(struct nand_device *nand); > > int nand_ecc_prepare_io_req(struct nand_device *nand, > Thanks, Miquèl
On Mon, 25 Feb 2019 16:49:33 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 > 15:29:57 +0100: > > > On Thu, 21 Feb 2019 13:57:59 +0100 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > Before making use of the ECC engines, we must retrieve them. Add the > > > boilerplate for the ones already available: software engines (Hamming > > > and BCH). > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/mtd/nand/ecc/engine.c | 14 ++++++++++++++ > > > include/linux/mtd/nand-sw-bch-engine.h | 3 +++ > > > include/linux/mtd/nand-sw-hamming-engine.h | 3 +++ > > > include/linux/mtd/nand.h | 3 +++ > > > 4 files changed, 23 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/ecc/engine.c b/drivers/mtd/nand/ecc/engine.c > > > index 7dd3f9772698..318dbb2d56a2 100644 > > > --- a/drivers/mtd/nand/ecc/engine.c > > > +++ b/drivers/mtd/nand/ecc/engine.c > > > @@ -286,6 +286,20 @@ bool nand_ecc_correction_is_enough(struct nand_device *nand) > > > return corr >= ds_corr && conf->strength >= reqs->strength; > > > } > > > > > > +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand) > > > > What if you want to instantiate SW ECC with a custom layout? Can't we > > instead have a function that create a new engine dynamically? > > > > struct nand_ecc_engine * > > nand_ecc_create_sw_engine(struct nand_device* nand, > > enum nand_ecc_algo algo, > > struct mtd_ooblayout *layout); > > > > > > Right now, for both sw engines, a default layout is applied if there is > none at engine initialization time. > > Also, do we really need a "create" helper? I don't see what's created > there. Maybe you had something else in mind, and the > ecc_sw_xxx_get_engine() approach do not match what you expected, so > please tell me more about your idea, otherwise I don't see what a > nand_ecc_create_sw_engine() would bring. The layout is part of the ECC engine properties, which I why I suggest to create one engine instance per user and specify the layout to use at instance creation time. > > > > > > +{ > > > + switch (nand->ecc.user_conf.algo) { > > > > Note that the conf is supposed to be passed afterwards, when the ctx is > > created, so you should check nand->ecc.user_conf directly here. > > I think this is what I do so I suspect the above sentence is not what > you actually meant? Sorry, I meant "should not". My point is, the user_conf should only be passed at context creation time, and should not be modified in-place by the caller, unless proven necessary. There's really 2 different steps that I think need to be isolated: 1/ retrieve/create an ECC engine instance (SW, HW-controller-side, on-die) 2/ ask this ECC engine instance to create a context out of a user conf Your user_conf seems 2 mix the 2 concepts: the engine to use, the strength/step-size you expect.
Hi Boris, Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 25 Feb 2019 17:13:30 +0100: > > > > +{ > > > > + switch (nand->ecc.user_conf.algo) { > > > > > > Note that the conf is supposed to be passed afterwards, when the ctx is > > > created, so you should check nand->ecc.user_conf directly here. > > > > I think this is what I do so I suspect the above sentence is not what > > you actually meant? > > Sorry, I meant "should not". My point is, the user_conf should only be > passed at context creation time, and should not be modified in-place by > the caller, unless proven necessary. > > There's really 2 different steps that I think need to be isolated: > > 1/ retrieve/create an ECC engine instance (SW, HW-controller-side, > on-die) > 2/ ask this ECC engine instance to create a context out of a user conf > > Your user_conf seems 2 mix the 2 concepts: the engine to use, the > strength/step-size you expect. No, I am not mixing things: the user might want a specific engine and a strength/step-size. All of this is what the user requests. In the ECC logic, I need to make a choice: I really need to check what the user request is in order to choose the provider. I am not changing anything here, just reading. Later the chosen ECC engine will be prompted to create a context with the rest of the user configuration. I don't think it is relevant here to split this structure.
diff --git a/drivers/mtd/nand/ecc/engine.c b/drivers/mtd/nand/ecc/engine.c index 7dd3f9772698..318dbb2d56a2 100644 --- a/drivers/mtd/nand/ecc/engine.c +++ b/drivers/mtd/nand/ecc/engine.c @@ -286,6 +286,20 @@ bool nand_ecc_correction_is_enough(struct nand_device *nand) return corr >= ds_corr && conf->strength >= reqs->strength; } +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand) +{ + switch (nand->ecc.user_conf.algo) { + case NAND_ECC_HAMMING: + return ecc_sw_hamming_get_engine(); + case NAND_ECC_BCH: + return ecc_sw_bch_get_engine(); + default: + break; + } + + return NULL; +} + MODULE_LICENSE("GPL v2"); MODULE_AUTHOR("Miquel Raynal <miquel.raynal@bootlin.com>"); MODULE_DESCRIPTION("Generic ECC engine"); diff --git a/include/linux/mtd/nand-sw-bch-engine.h b/include/linux/mtd/nand-sw-bch-engine.h index e406aa53ec4e..38cdad117fea 100644 --- a/include/linux/mtd/nand-sw-bch-engine.h +++ b/include/linux/mtd/nand-sw-bch-engine.h @@ -11,6 +11,9 @@ #include <linux/mtd/nand.h> #include <linux/bch.h> +/* Needed for cross inclusion with nand.h */ +struct nand_device; + /** * struct ecc_sw_bch_conf - private software BCH ECC engine structure * @reqooblen: Save the actual user OOB length requested before overwriting it diff --git a/include/linux/mtd/nand-sw-hamming-engine.h b/include/linux/mtd/nand-sw-hamming-engine.h index 8df36d189482..51c5a2acee42 100644 --- a/include/linux/mtd/nand-sw-hamming-engine.h +++ b/include/linux/mtd/nand-sw-hamming-engine.h @@ -12,6 +12,9 @@ #include <linux/mtd/nand.h> +/* Needed for cross inclusion with nand.h */ +struct nand_device; + /** * struct ecc_sw_hamming_conf - private software Hamming ECC engine structure * @reqooblen: Save the actual user OOB length requested before overwriting it diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 4482eb2bbfd4..3abe113e4f06 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -11,6 +11,8 @@ #define __LINUX_MTD_NAND_H #include <linux/mtd/mtd.h> +#include <linux/mtd/nand-sw-hamming-engine.h> +#include <linux/mtd/nand-sw-bch-engine.h> struct nand_device; @@ -253,6 +255,7 @@ struct nand_ecc_engine { }; void nand_ecc_read_user_conf(struct nand_device *nand); +struct nand_ecc_engine *nand_ecc_sw_get_engine(struct nand_device *nand); int nand_ecc_init_ctx(struct nand_device *nand); void nand_ecc_cleanup_ctx(struct nand_device *nand); int nand_ecc_prepare_io_req(struct nand_device *nand,
Before making use of the ECC engines, we must retrieve them. Add the boilerplate for the ones already available: software engines (Hamming and BCH). Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/nand/ecc/engine.c | 14 ++++++++++++++ include/linux/mtd/nand-sw-bch-engine.h | 3 +++ include/linux/mtd/nand-sw-hamming-engine.h | 3 +++ include/linux/mtd/nand.h | 3 +++ 4 files changed, 23 insertions(+)