| Message ID | 20190221125806.28875-13-miquel.raynal@bootlin.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | None | expand |
On Thu, 21 Feb 2019 13:58:04 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Add the logic in the NAND core to find the right ECC engine depending > on the NAND chip requirements and the user desires. Right now, the > choice may be made between (more will come): > * software Hamming > * software BCH > * on-die (SPI-NAND devices only) > > Once the ECC engine has been found, the ECC engine must be > configured. > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > --- > drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ > include/linux/mtd/nand.h | 4 ++ > 2 files changed, 111 insertions(+) > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c > index 872d46b5fc0f..9feb118c9f68 100644 > --- a/drivers/mtd/nand/core.c > +++ b/drivers/mtd/nand/core.c > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) > } > EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); > > +/** > + * nanddev_find_ecc_engine() - Find a suitable ECC engine > + * @nand: NAND device > + */ > +static int nanddev_find_ecc_engine(struct nand_device *nand) Can we pass the conf in argument instead of reading it from nand->ecc.user_conf? > +{ > + bool is_spinand = mtd_type_is_spinand(&nand->mtd); And here is the reason for the SPINAND type. > + > + /* Read the user desires in terms of ECC engine/configuration */ > + nand_ecc_read_user_conf(nand); > + > + /* No ECC engine requestedn, let's return without error */ > + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) > + return 0; > + > + /* Raw NAND default mode is hardware */ > + if (!is_spinand && nand->ecc.user_conf.mode < 0) > + nand->ecc.user_conf.mode = NAND_ECC_HW; We should let the raw NAND layer take this decision (actually, it's even a raw NAND controller driver decision). Please complain if user_conf.mode is invalid. This way you won't need the SPINAND type you added in one of your previous patch. > + > + /* SPI-NAND default mode is on-die */ > + if (is_spinand && nand->ecc.user_conf.mode < 0) > + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; > + > + switch (nand->ecc.user_conf.mode) { > + case NAND_ECC_SOFT: > + nand->ecc.engine = nand_ecc_sw_get_engine(nand); > + break; > + case NAND_ECC_ON_DIE: > + if (is_spinand) > + nand->ecc.engine = spinand_ondie_ecc_get_engine(); So, maybe it's worth having the ondie ECC engine instance directly embedded in nand_device instead of spinand, or maybe just a pointer, so that you don't reserve extra space when the NAND device does not have on-die ECC. > + else > + pr_err("On-die ECC engines for non SPI devices not supported yet\n"); > + break; > + case NAND_ECC_HW: > + pr_err("Hardware ECC engines not supported yet\n"); > + break; > + default: > + pr_err("Missing ECC engine property\n"); > + } > + > + if (!nand->ecc.engine) > + return -EINVAL; > + > + return 0; > +}
Hi Boris, Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 15:44:31 +0100: > On Thu, 21 Feb 2019 13:58:04 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Add the logic in the NAND core to find the right ECC engine depending > > on the NAND chip requirements and the user desires. Right now, the > > choice may be made between (more will come): > > * software Hamming > > * software BCH > > * on-die (SPI-NAND devices only) > > > > Once the ECC engine has been found, the ECC engine must be > > configured. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/nand.h | 4 ++ > > 2 files changed, 111 insertions(+) > > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c > > index 872d46b5fc0f..9feb118c9f68 100644 > > --- a/drivers/mtd/nand/core.c > > +++ b/drivers/mtd/nand/core.c > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) > > } > > EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); > > > > +/** > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine > > + * @nand: NAND device > > + */ > > +static int nanddev_find_ecc_engine(struct nand_device *nand) > > Can we pass the conf in argument instead of reading it from > nand->ecc.user_conf? > > > +{ > > + bool is_spinand = mtd_type_is_spinand(&nand->mtd); > > And here is the reason for the SPINAND type. > > > + > > + /* Read the user desires in terms of ECC engine/configuration */ > > + nand_ecc_read_user_conf(nand); > > + > > + /* No ECC engine requestedn, let's return without error */ > > + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) > > + return 0; > > + > > + /* Raw NAND default mode is hardware */ > > + if (!is_spinand && nand->ecc.user_conf.mode < 0) > > + nand->ecc.user_conf.mode = NAND_ECC_HW; > > We should let the raw NAND layer take this decision (actually, it's > even a raw NAND controller driver decision). Please complain if > user_conf.mode is invalid. > This way you won't need the SPINAND type you added in one of your > previous patch. > > > + > > + /* SPI-NAND default mode is on-die */ > > + if (is_spinand && nand->ecc.user_conf.mode < 0) > > + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; > > + > > + switch (nand->ecc.user_conf.mode) { > > + case NAND_ECC_SOFT: > > + nand->ecc.engine = nand_ecc_sw_get_engine(nand); > > + break; > > + case NAND_ECC_ON_DIE: > > + if (is_spinand) > > + nand->ecc.engine = spinand_ondie_ecc_get_engine(); > > So, maybe it's worth having the ondie ECC engine instance directly > embedded in nand_device instead of spinand, or maybe just a pointer, so > that you don't reserve extra space when the NAND device does not have > on-die ECC. > > > + else > > + pr_err("On-die ECC engines for non SPI devices not supported yet\n"); > > + break; > > + case NAND_ECC_HW: > > + pr_err("Hardware ECC engines not supported yet\n"); > > + break; > > + default: > > + pr_err("Missing ECC engine property\n"); > > + } > > + > > + if (!nand->ecc.engine) > > + return -EINVAL; > > + > > + return 0; > > +} > I think this is the root patch were our ideas diverge. For me, each 'ECC engine' has a _get() helper and the NAND core decides which one to call to retrieve the right engine. Can you please explain what was your idea if this one does not fit? Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the user ECC mode and algo, so I cannot let the raw NAND core (or a raw NAND controller driver) or the SPI NAND core decide which mode is the default if not provided by the user. Thanks, Miquèl
On Mon, 25 Feb 2019 17:01:18 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 > 15:44:31 +0100: > > > On Thu, 21 Feb 2019 13:58:04 +0100 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > Add the logic in the NAND core to find the right ECC engine depending > > > on the NAND chip requirements and the user desires. Right now, the > > > choice may be made between (more will come): > > > * software Hamming > > > * software BCH > > > * on-die (SPI-NAND devices only) > > > > > > Once the ECC engine has been found, the ECC engine must be > > > configured. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ > > > include/linux/mtd/nand.h | 4 ++ > > > 2 files changed, 111 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c > > > index 872d46b5fc0f..9feb118c9f68 100644 > > > --- a/drivers/mtd/nand/core.c > > > +++ b/drivers/mtd/nand/core.c > > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) > > > } > > > EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); > > > > > > +/** > > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine > > > + * @nand: NAND device > > > + */ > > > +static int nanddev_find_ecc_engine(struct nand_device *nand) > > > > Can we pass the conf in argument instead of reading it from > > nand->ecc.user_conf? > > > > > +{ > > > + bool is_spinand = mtd_type_is_spinand(&nand->mtd); > > > > And here is the reason for the SPINAND type. > > > > > + > > > + /* Read the user desires in terms of ECC engine/configuration */ > > > + nand_ecc_read_user_conf(nand); > > > + > > > + /* No ECC engine requestedn, let's return without error */ > > > + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) > > > + return 0; > > > + > > > + /* Raw NAND default mode is hardware */ > > > + if (!is_spinand && nand->ecc.user_conf.mode < 0) > > > + nand->ecc.user_conf.mode = NAND_ECC_HW; > > > > We should let the raw NAND layer take this decision (actually, it's > > even a raw NAND controller driver decision). Please complain if > > user_conf.mode is invalid. > > This way you won't need the SPINAND type you added in one of your > > previous patch. > > > > > + > > > + /* SPI-NAND default mode is on-die */ > > > + if (is_spinand && nand->ecc.user_conf.mode < 0) > > > + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; > > > + > > > + switch (nand->ecc.user_conf.mode) { > > > + case NAND_ECC_SOFT: > > > + nand->ecc.engine = nand_ecc_sw_get_engine(nand); > > > + break; > > > + case NAND_ECC_ON_DIE: > > > + if (is_spinand) > > > + nand->ecc.engine = spinand_ondie_ecc_get_engine(); > > > > So, maybe it's worth having the ondie ECC engine instance directly > > embedded in nand_device instead of spinand, or maybe just a pointer, so > > that you don't reserve extra space when the NAND device does not have > > on-die ECC. > > > > > + else > > > + pr_err("On-die ECC engines for non SPI devices not supported yet\n"); > > > + break; > > > + case NAND_ECC_HW: > > > + pr_err("Hardware ECC engines not supported yet\n"); > > > + break; > > > + default: > > > + pr_err("Missing ECC engine property\n"); > > > + } > > > + > > > + if (!nand->ecc.engine) > > > + return -EINVAL; > > > + > > > + return 0; > > > +} > > > > I think this is the root patch were our ideas diverge. For me, each > 'ECC engine' has a _get() helper and the NAND core decides which one to > call to retrieve the right engine. Can you please explain what was your > idea if this one does not fit? Each class of ECC engine has its own way of getting a pointer to an ECC engine instance: - ondie: the engine is directly attached to the device and can be retrieved by accessing a nand_device field (nanddev->ondie_ecc?). - sw ECC: you can create a new instance for each device and possibly pass the OOB layout you want to use (assuming you don't want to use the default one) - HW-controller-side engine: refers to the controller device (parent node) or the device pointed by the ecc-engine DT prop (we'll probably need a way to pass this info when for non-DT platforms). For this one we'll add a nanddev_get_hw_ecc_engine() which will search for all registered HW ECC engines and try to match with some search keys (in case of DT, it's the ->of_node pointer address) Clearly, for SW-based ECC, you'll need more than just the nanddev object, as layouts can differ depending on the controller driver. So, what I'm suggesting is to have a nand_create_sw_ecc_engine(algo, layout) funtion that returns this ECC engine instance which the driver will then attach to the NAND device. > > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the > user ECC mode and algo, so I cannot let the raw NAND core (or a raw > NAND controller driver) or the SPI NAND core decide which mode is the > default if not provided by the user. Except this prop is optional in most cases, and the default value is not always the same, which is why I think this ECC engine retrieval step should be left to each sub-layer (and sometimes to the controller driver behind it). Maybe you can provide helpers to help with that, but I don't think taking this decision here, based on the bus type, is a good idea. And I also don't like the idea of adding a new SPINAND type.
On Mon, 25 Feb 2019 17:34:22 +0100 Boris Brezillon <boris.brezillon@collabora.com> wrote: > On Mon, 25 Feb 2019 17:01:18 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Hi Boris, > > > > Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 > > 15:44:31 +0100: > > > > > On Thu, 21 Feb 2019 13:58:04 +0100 > > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > > > Add the logic in the NAND core to find the right ECC engine depending > > > > on the NAND chip requirements and the user desires. Right now, the > > > > choice may be made between (more will come): > > > > * software Hamming > > > > * software BCH > > > > * on-die (SPI-NAND devices only) > > > > > > > > Once the ECC engine has been found, the ECC engine must be > > > > configured. > > > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > > --- > > > > drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ > > > > include/linux/mtd/nand.h | 4 ++ > > > > 2 files changed, 111 insertions(+) > > > > > > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c > > > > index 872d46b5fc0f..9feb118c9f68 100644 > > > > --- a/drivers/mtd/nand/core.c > > > > +++ b/drivers/mtd/nand/core.c > > > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) > > > > } > > > > EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); > > > > > > > > +/** > > > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine > > > > + * @nand: NAND device > > > > + */ > > > > +static int nanddev_find_ecc_engine(struct nand_device *nand) > > > > > > Can we pass the conf in argument instead of reading it from > > > nand->ecc.user_conf? > > > > > > > +{ > > > > + bool is_spinand = mtd_type_is_spinand(&nand->mtd); > > > > > > And here is the reason for the SPINAND type. > > > > > > > + > > > > + /* Read the user desires in terms of ECC engine/configuration */ > > > > + nand_ecc_read_user_conf(nand); > > > > + > > > > + /* No ECC engine requestedn, let's return without error */ > > > > + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) > > > > + return 0; > > > > + > > > > + /* Raw NAND default mode is hardware */ > > > > + if (!is_spinand && nand->ecc.user_conf.mode < 0) > > > > + nand->ecc.user_conf.mode = NAND_ECC_HW; > > > > > > We should let the raw NAND layer take this decision (actually, it's > > > even a raw NAND controller driver decision). Please complain if > > > user_conf.mode is invalid. > > > This way you won't need the SPINAND type you added in one of your > > > previous patch. > > > > > > > + > > > > + /* SPI-NAND default mode is on-die */ > > > > + if (is_spinand && nand->ecc.user_conf.mode < 0) > > > > + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; > > > > + > > > > + switch (nand->ecc.user_conf.mode) { > > > > + case NAND_ECC_SOFT: > > > > + nand->ecc.engine = nand_ecc_sw_get_engine(nand); > > > > + break; > > > > + case NAND_ECC_ON_DIE: > > > > + if (is_spinand) > > > > + nand->ecc.engine = spinand_ondie_ecc_get_engine(); > > > > > > So, maybe it's worth having the ondie ECC engine instance directly > > > embedded in nand_device instead of spinand, or maybe just a pointer, so > > > that you don't reserve extra space when the NAND device does not have > > > on-die ECC. > > > > > > > + else > > > > + pr_err("On-die ECC engines for non SPI devices not supported yet\n"); > > > > + break; > > > > + case NAND_ECC_HW: > > > > + pr_err("Hardware ECC engines not supported yet\n"); > > > > + break; > > > > + default: > > > > + pr_err("Missing ECC engine property\n"); > > > > + } > > > > + > > > > + if (!nand->ecc.engine) > > > > + return -EINVAL; > > > > + > > > > + return 0; > > > > +} > > > > > > > I think this is the root patch were our ideas diverge. For me, each > > 'ECC engine' has a _get() helper and the NAND core decides which one to > > call to retrieve the right engine. Can you please explain what was your > > idea if this one does not fit? > > Each class of ECC engine has its own way of getting a pointer to an ECC > engine instance: > > - ondie: the engine is directly attached to the device and can be > retrieved by accessing a nand_device field (nanddev->ondie_ecc?). > - sw ECC: you can create a new instance for each device and possibly > pass the OOB layout you want to use (assuming you don't want to use > the default one) > - HW-controller-side engine: refers to the controller device (parent > node) or the device pointed by the ecc-engine DT prop (we'll probably > need a way to pass this info when for non-DT platforms). For this one > we'll add a nanddev_get_hw_ecc_engine() which will search for all > registered HW ECC engines and try to match with some search keys (in > case of DT, it's the ->of_node pointer address) > > Clearly, for SW-based ECC, you'll need more than just the nanddev > object, as layouts can differ depending on the controller driver. So, > what I'm suggesting is to have a > > nand_create_sw_ecc_engine(algo, layout) > > funtion that returns this ECC engine instance which the driver will > then attach to the NAND device. I'm reconsidering what I said here. I guess having the layout set by the driver before nand_ecc_sw_get_engine() is called could do the trick. We just need to make sure the layout provided has enough space to store ECC bytes at context creation time. > > > > > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the > > user ECC mode and algo, so I cannot let the raw NAND core (or a raw > > NAND controller driver) or the SPI NAND core decide which mode is the > > default if not provided by the user. > > Except this prop is optional in most cases, and the default value is > not always the same, which is why I think this ECC engine retrieval step > should be left to each sub-layer (and sometimes to the controller driver > behind it). Maybe you can provide helpers to help with that, but I > don't think taking this decision here, based on the bus type, is a good > idea. And I also don't like the idea of adding a new SPINAND type. Hm, after thinking a bit more about it, maybe we could have something in-between: let the controller driver or sub-layer specify what the default values are (provider/mode and possibly algorithm if that makes sense) so that this generic function can use these defaults when nand->ecc.user_conf.{mode,algo} == UNKNOWN.
Hi Boris, Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 25 Feb 2019 19:48:03 +0100: > > > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the > > > user ECC mode and algo, so I cannot let the raw NAND core (or a raw > > > NAND controller driver) or the SPI NAND core decide which mode is the > > > default if not provided by the user. > > > > Except this prop is optional in most cases, and the default value is > > not always the same, which is why I think this ECC engine retrieval step > > should be left to each sub-layer (and sometimes to the controller driver > > behind it). Maybe you can provide helpers to help with that, but I > > don't think taking this decision here, based on the bus type, is a good > > idea. And I also don't like the idea of adding a new SPINAND type. > > Hm, after thinking a bit more about it, maybe we could have something > in-between: let the controller driver or sub-layer specify what the > default values are (provider/mode and possibly algorithm if that makes > sense) so that this generic function can use these defaults when > nand->ecc.user_conf.{mode,algo} == UNKNOWN. Having a "default_provider" in struct nand_ecc could to the trick. It would be filled by the controller drivers/sublayers before nanddev_ecc_engine_init() is called. Thanks, Miquèl
On Tue, 26 Feb 2019 16:59:23 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > Boris Brezillon <boris.brezillon@collabora.com> wrote on Mon, 25 Feb > 2019 19:48:03 +0100: > > > > > Also, the parsing of the DT (in nand_ecc_read_user_conf()) gives me the > > > > user ECC mode and algo, so I cannot let the raw NAND core (or a raw > > > > NAND controller driver) or the SPI NAND core decide which mode is the > > > > default if not provided by the user. > > > > > > Except this prop is optional in most cases, and the default value is > > > not always the same, which is why I think this ECC engine retrieval step > > > should be left to each sub-layer (and sometimes to the controller driver > > > behind it). Maybe you can provide helpers to help with that, but I > > > don't think taking this decision here, based on the bus type, is a good > > > idea. And I also don't like the idea of adding a new SPINAND type. > > > > Hm, after thinking a bit more about it, maybe we could have something > > in-between: let the controller driver or sub-layer specify what the > > default values are (provider/mode and possibly algorithm if that makes > > sense) so that this generic function can use these defaults when > > nand->ecc.user_conf.{mode,algo} == UNKNOWN. > > Having a "default_provider" in struct nand_ecc could to the trick. It > would be filled by the controller drivers/sublayers before > nanddev_ecc_engine_init() is called. You'll also need a default_algo, at least for SW ECC. Note that other props, like strength, step-size or flags (like the maximize flag) can be left undefined as well, so providing default vals for those ones (or deriving them from ECC requirements) might make sense too.
Hi Boris, Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 15:44:31 +0100: > On Thu, 21 Feb 2019 13:58:04 +0100 > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > Add the logic in the NAND core to find the right ECC engine depending > > on the NAND chip requirements and the user desires. Right now, the > > choice may be made between (more will come): > > * software Hamming > > * software BCH > > * on-die (SPI-NAND devices only) > > > > Once the ECC engine has been found, the ECC engine must be > > configured. > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > --- > > drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ > > include/linux/mtd/nand.h | 4 ++ > > 2 files changed, 111 insertions(+) > > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c > > index 872d46b5fc0f..9feb118c9f68 100644 > > --- a/drivers/mtd/nand/core.c > > +++ b/drivers/mtd/nand/core.c > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) > > } > > EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); > > > > +/** > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine > > + * @nand: NAND device > > + */ > > +static int nanddev_find_ecc_engine(struct nand_device *nand) > > Can we pass the conf in argument instead of reading it from > nand->ecc.user_conf? I just changed the root structures, there is a struct nand_ecc_conf defaults; entry now in the nand_ecc_engine structure, which every layer/driver can overwrite to inform the NAND core of a default value. Later in this function, I check user_conf (which I might call "desires" now that we have a "defaults" and a "requirements" entries), but if the value is missing I fallback to the "defaults" one if filled. Having all these structures being passed as parameters does not make sense to me so I would prefer sticking to the single "nand" parameter. > > > +{ > > + bool is_spinand = mtd_type_is_spinand(&nand->mtd); > > And here is the reason for the SPINAND type. This does not exist anymore as you suggested. > > > + > > + /* Read the user desires in terms of ECC engine/configuration */ > > + nand_ecc_read_user_conf(nand); > > + > > + /* No ECC engine requestedn, let's return without error */ > > + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) > > + return 0; > > + > > + /* Raw NAND default mode is hardware */ > > + if (!is_spinand && nand->ecc.user_conf.mode < 0) > > + nand->ecc.user_conf.mode = NAND_ECC_HW; > > We should let the raw NAND layer take this decision (actually, it's > even a raw NAND controller driver decision). Please complain if > user_conf.mode is invalid. > This way you won't need the SPINAND type you added in one of your > previous patch. See above for the solution I choose. > > > + > > + /* SPI-NAND default mode is on-die */ > > + if (is_spinand && nand->ecc.user_conf.mode < 0) > > + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; > > + > > + switch (nand->ecc.user_conf.mode) { > > + case NAND_ECC_SOFT: > > + nand->ecc.engine = nand_ecc_sw_get_engine(nand); > > + break; > > + case NAND_ECC_ON_DIE: > > + if (is_spinand) > > + nand->ecc.engine = spinand_ondie_ecc_get_engine(); > > So, maybe it's worth having the ondie ECC engine instance directly > embedded in nand_device instead of spinand, or maybe just a pointer, so > that you don't reserve extra space when the NAND device does not have > on-die ECC. It is now: the nand_ecc_engine structure features a *engine and a *ondie_engine pointer. The nand_ecc_get_ondie_engine(nand) helper just returns nand->ecc.ondie_engine. > > > + else > > + pr_err("On-die ECC engines for non SPI devices not supported yet\n"); > > + break; > > + case NAND_ECC_HW: > > + pr_err("Hardware ECC engines not supported yet\n"); > > + break; > > + default: > > + pr_err("Missing ECC engine property\n"); > > + } > > + > > + if (!nand->ecc.engine) > > + return -EINVAL; > > + > > + return 0; > > +} > Thanks, Miquèl
On Wed, 27 Feb 2019 15:07:13 +0100 Miquel Raynal <miquel.raynal@bootlin.com> wrote: > Hi Boris, > > Boris Brezillon <bbrezillon@kernel.org> wrote on Fri, 22 Feb 2019 > 15:44:31 +0100: > > > On Thu, 21 Feb 2019 13:58:04 +0100 > > Miquel Raynal <miquel.raynal@bootlin.com> wrote: > > > > > Add the logic in the NAND core to find the right ECC engine depending > > > on the NAND chip requirements and the user desires. Right now, the > > > choice may be made between (more will come): > > > * software Hamming > > > * software BCH > > > * on-die (SPI-NAND devices only) > > > > > > Once the ECC engine has been found, the ECC engine must be > > > configured. > > > > > > Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> > > > --- > > > drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ > > > include/linux/mtd/nand.h | 4 ++ > > > 2 files changed, 111 insertions(+) > > > > > > diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c > > > index 872d46b5fc0f..9feb118c9f68 100644 > > > --- a/drivers/mtd/nand/core.c > > > +++ b/drivers/mtd/nand/core.c > > > @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) > > > } > > > EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); > > > > > > +/** > > > + * nanddev_find_ecc_engine() - Find a suitable ECC engine > > > + * @nand: NAND device > > > + */ > > > +static int nanddev_find_ecc_engine(struct nand_device *nand) > > > > Can we pass the conf in argument instead of reading it from > > nand->ecc.user_conf? > > I just changed the root structures, there is a > > struct nand_ecc_conf defaults; > > entry now in the nand_ecc_engine structure, which every layer/driver > can overwrite to inform the NAND core of a default value. > > Later in this function, I check user_conf (which I might call > "desires" now that we have a "defaults" and a "requirements" entries), > but if the value is missing I fallback to the "defaults" one if filled. > Having all these structures being passed as parameters does not make > sense to me so I would prefer sticking to the single "nand" parameter. > > > > > > +{ > > > + bool is_spinand = mtd_type_is_spinand(&nand->mtd); > > > > And here is the reason for the SPINAND type. > > This does not exist anymore as you suggested. > > > > > > + > > > + /* Read the user desires in terms of ECC engine/configuration */ > > > + nand_ecc_read_user_conf(nand); > > > + > > > + /* No ECC engine requestedn, let's return without error */ > > > + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) > > > + return 0; > > > + > > > + /* Raw NAND default mode is hardware */ > > > + if (!is_spinand && nand->ecc.user_conf.mode < 0) > > > + nand->ecc.user_conf.mode = NAND_ECC_HW; > > > > We should let the raw NAND layer take this decision (actually, it's > > even a raw NAND controller driver decision). Please complain if > > user_conf.mode is invalid. > > This way you won't need the SPINAND type you added in one of your > > previous patch. > > See above for the solution I choose. > > > > > > + > > > + /* SPI-NAND default mode is on-die */ > > > + if (is_spinand && nand->ecc.user_conf.mode < 0) > > > + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; > > > + > > > + switch (nand->ecc.user_conf.mode) { > > > + case NAND_ECC_SOFT: > > > + nand->ecc.engine = nand_ecc_sw_get_engine(nand); > > > + break; > > > + case NAND_ECC_ON_DIE: > > > + if (is_spinand) > > > + nand->ecc.engine = spinand_ondie_ecc_get_engine(); > > > > So, maybe it's worth having the ondie ECC engine instance directly > > embedded in nand_device instead of spinand, or maybe just a pointer, so > > that you don't reserve extra space when the NAND device does not have > > on-die ECC. > > It is now: the nand_ecc_engine structure features a *engine and a > *ondie_engine pointer. > > The nand_ecc_get_ondie_engine(nand) helper just > returns nand->ecc.ondie_engine. Ack to all of this.
diff --git a/drivers/mtd/nand/core.c b/drivers/mtd/nand/core.c index 872d46b5fc0f..9feb118c9f68 100644 --- a/drivers/mtd/nand/core.c +++ b/drivers/mtd/nand/core.c @@ -207,6 +207,113 @@ int nanddev_mtd_max_bad_blocks(struct mtd_info *mtd, loff_t offs, size_t len) } EXPORT_SYMBOL_GPL(nanddev_mtd_max_bad_blocks); +/** + * nanddev_find_ecc_engine() - Find a suitable ECC engine + * @nand: NAND device + */ +static int nanddev_find_ecc_engine(struct nand_device *nand) +{ + bool is_spinand = mtd_type_is_spinand(&nand->mtd); + + /* Read the user desires in terms of ECC engine/configuration */ + nand_ecc_read_user_conf(nand); + + /* No ECC engine requestedn, let's return without error */ + if (nand->ecc.user_conf.mode == NAND_ECC_NONE) + return 0; + + /* Raw NAND default mode is hardware */ + if (!is_spinand && nand->ecc.user_conf.mode < 0) + nand->ecc.user_conf.mode = NAND_ECC_HW; + + /* SPI-NAND default mode is on-die */ + if (is_spinand && nand->ecc.user_conf.mode < 0) + nand->ecc.user_conf.mode = NAND_ECC_ON_DIE; + + switch (nand->ecc.user_conf.mode) { + case NAND_ECC_SOFT: + nand->ecc.engine = nand_ecc_sw_get_engine(nand); + break; + case NAND_ECC_ON_DIE: + if (is_spinand) + nand->ecc.engine = spinand_ondie_ecc_get_engine(); + else + pr_err("On-die ECC engines for non SPI devices not supported yet\n"); + break; + case NAND_ECC_HW: + pr_err("Hardware ECC engines not supported yet\n"); + break; + default: + pr_err("Missing ECC engine property\n"); + } + + if (!nand->ecc.engine) + return -EINVAL; + + return 0; +} + +/** + * nanddev_find_ecc_configuration() - Find a suitable ECC configuration + * @nand: NAND device + */ +static int nanddev_find_ecc_configuration(struct nand_device *nand) +{ + int ret; + + if (!nand->ecc.engine) + return -ENOTSUPP; + + ret = nand_ecc_init_ctx(nand); + if (ret) + return ret; + + if (!nand_ecc_correction_is_enough(nand)) + pr_warn("WARNING: %s: the ECC used on your system is too weak compared to the one required by the NAND chip\n", + nand->mtd.name); + + return 0; +} + +/** + * nanddev_ecc_engine_init() - Initialize an ECC engine for the chip + * @nand: NAND device + */ +int nanddev_ecc_engine_init(struct nand_device *nand) +{ + int ret; + + /* Look for the ECC engine to use */ + ret = nanddev_find_ecc_engine(nand); + if (ret) { + pr_err("No ECC engine found\n"); + return ret; + } + + /* No ECC engine requested */ + if (!nand->ecc.engine) + return 0; + + /* Configure the engine: balance user input and chip requirements */ + ret = nanddev_find_ecc_configuration(nand); + if (ret) { + pr_err("No suitable ECC configuration\n"); + return ret; + } + + return 0; +} + +/** + * nanddev_ecc_engine_cleanup() - Cleanup ECC engine initializations + * @nand: NAND device + */ +void nanddev_ecc_engine_cleanup(struct nand_device *nand) +{ + if (nand->ecc.engine) + nand_ecc_cleanup_ctx(nand); +} + /** * nanddev_init() - Initialize a NAND device * @nand: NAND device diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h index 534c07fab9de..bf949f55c139 100644 --- a/include/linux/mtd/nand.h +++ b/include/linux/mtd/nand.h @@ -844,6 +844,10 @@ bool nanddev_isreserved(struct nand_device *nand, const struct nand_pos *pos); int nanddev_erase(struct nand_device *nand, const struct nand_pos *pos); int nanddev_markbad(struct nand_device *nand, const struct nand_pos *pos); +/* ECC related functions */ +int nanddev_ecc_engine_init(struct nand_device *nand); +void nanddev_ecc_engine_cleanup(struct nand_device *nand); + /* BBT related functions */ enum nand_bbt_block_status { NAND_BBT_BLOCK_STATUS_UNKNOWN,
Add the logic in the NAND core to find the right ECC engine depending on the NAND chip requirements and the user desires. Right now, the choice may be made between (more will come): * software Hamming * software BCH * on-die (SPI-NAND devices only) Once the ECC engine has been found, the ECC engine must be configured. Signed-off-by: Miquel Raynal <miquel.raynal@bootlin.com> --- drivers/mtd/nand/core.c | 107 +++++++++++++++++++++++++++++++++++++++ include/linux/mtd/nand.h | 4 ++ 2 files changed, 111 insertions(+)