From: Miquel Raynal <miquel.raynal@bootlin.com> To: Boris Brezillon <bbrezillon@kernel.org> Cc: Mason Yang <masonccyang@mxic.com.tw>, Vignesh R <vigneshr@ti.com>, Tudor Ambarus <Tudor.Ambarus@microchip.com>, Julien Su <juliensu@mxic.com.tw>, Richard Weinberger <richard@nod.at>, Schrempf Frieder <frieder.schrempf@kontron.de>, Marek Vasut <marek.vasut@gmail.com>, linux-mtd@lists.infradead.org, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Brian Norris <computersforpeace@gmail.com>, David Woodhouse <dwmw2@infradead.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurations Date: Wed, 27 Feb 2019 15:07:13 +0100 [thread overview] Message-ID: <20190227150713.116a0ad3@xps13> (raw) In-Reply-To: <20190222154431.66c6911f@kernel.org> 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 ______________________________________________________ Linux MTD discussion mailing list http://lists.infradead.org/mailman/listinfo/linux-mtd/
WARNING: multiple messages have this Message-ID (diff)
From: Miquel Raynal <miquel.raynal@bootlin.com> To: Boris Brezillon <bbrezillon@kernel.org> Cc: Mason Yang <masonccyang@mxic.com.tw>, Vignesh R <vigneshr@ti.com>, Tudor Ambarus <Tudor.Ambarus@microchip.com>, Julien Su <juliensu@mxic.com.tw>, Richard Weinberger <richard@nod.at>, Schrempf Frieder <frieder.schrempf@kontron.de>, Marek Vasut <marek.vasut@gmail.com>, linux-mtd@lists.infradead.org, Thomas Petazzoni <thomas.petazzoni@bootlin.com>, Brian Norris <computersforpeace@gmail.com>, David Woodhouse <dwmw2@infradead.org>, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurations Date: Wed, 27 Feb 2019 15:07:13 +0100 [thread overview] Message-ID: <20190227150713.116a0ad3@xps13> (raw) In-Reply-To: <20190222154431.66c6911f@kernel.org> 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-02-27 14:07 UTC|newest] Thread overview: 72+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-02-21 12:57 [RFC PATCH 13/27] mtd: nand: ecc: Clarify the software Hamming introductory line Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 12:57 ` [RFC PATCH 14/27] mtd: nand: ecc: Turn the software Hamming implementation generic Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 13:22 ` Boris Brezillon 2019-02-21 13:22 ` Boris Brezillon 2019-02-21 12:57 ` [RFC PATCH 15/27] mtd: nand: Remove useless include about software Hamming ECC Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 12:57 ` [RFC PATCH 16/27] mtd: nand: ecc: Let the software BCH ECC engine be a module Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 13:48 ` Adam Ford 2019-02-21 13:48 ` Adam Ford 2019-02-21 14:02 ` Miquel Raynal 2019-02-21 14:02 ` Miquel Raynal 2019-02-22 14:24 ` Boris Brezillon 2019-02-22 14:24 ` Boris Brezillon 2019-02-21 12:57 ` [RFC PATCH 17/27] mtd: nand: ecc: Let the software Hamming ECC engine be unselected Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 13:20 ` Boris Brezillon 2019-02-21 13:20 ` Boris Brezillon 2019-02-21 13:35 ` Miquel Raynal 2019-02-21 13:35 ` Miquel Raynal 2019-02-21 13:41 ` Boris Brezillon 2019-02-21 13:41 ` Boris Brezillon 2019-02-21 13:46 ` Miquel Raynal 2019-02-21 13:46 ` Miquel Raynal 2019-02-21 12:57 ` [RFC PATCH 18/27] mtd: nand: ecc: Create the software BCH engine instance Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 12:57 ` [RFC PATCH 19/27] mtd: nand: ecc: Create the software Hamming " Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-21 12:57 ` [RFC PATCH 20/27] mtd: nand: Let software ECC engines be retrieved from the NAND core Miquel Raynal 2019-02-21 12:57 ` Miquel Raynal 2019-02-22 14:29 ` Boris Brezillon 2019-02-22 14:29 ` Boris Brezillon 2019-02-25 15:49 ` Miquel Raynal 2019-02-25 15:49 ` Miquel Raynal 2019-02-25 16:13 ` Boris Brezillon 2019-02-25 16:13 ` Boris Brezillon 2019-02-26 15:54 ` Miquel Raynal 2019-02-26 15:54 ` Miquel Raynal 2019-02-21 12:58 ` [RFC PATCH 21/27] mtd: spinand: Fix typo in comment Miquel Raynal 2019-02-21 12:58 ` Miquel Raynal 2019-02-22 14:31 ` Boris Brezillon 2019-02-22 14:31 ` Boris Brezillon 2019-02-21 12:58 ` [RFC PATCH 22/27] mtd: spinand: Let the SPI-NAND core flag a SPI-NAND chip Miquel Raynal 2019-02-21 12:58 ` Miquel Raynal 2019-02-22 14:33 ` Boris Brezillon 2019-02-22 14:33 ` Boris Brezillon 2019-02-21 12:58 ` [RFC PATCH 23/27] mtd: spinand: Move the ECC helper functions into a separate file Miquel Raynal 2019-02-21 12:58 ` Miquel Raynal 2019-02-21 12:58 ` [RFC PATCH 24/27] mtd: spinand: Instantiate a SPI-NAND on-die ECC engine Miquel Raynal 2019-02-21 12:58 ` Miquel Raynal 2019-02-22 14:38 ` Boris Brezillon 2019-02-22 14:38 ` Boris Brezillon 2019-02-21 12:58 ` [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurations Miquel Raynal 2019-02-21 12:58 ` Miquel Raynal 2019-02-22 14:44 ` Boris Brezillon 2019-02-22 14:44 ` Boris Brezillon 2019-02-25 16:01 ` Miquel Raynal 2019-02-25 16:01 ` Miquel Raynal 2019-02-25 16:34 ` Boris Brezillon 2019-02-25 16:34 ` Boris Brezillon 2019-02-25 18:48 ` Boris Brezillon 2019-02-25 18:48 ` Boris Brezillon 2019-02-26 15:59 ` [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurationsND Miquel Raynal 2019-02-26 15:59 ` Miquel Raynal 2019-02-26 16:04 ` Boris Brezillon 2019-02-26 16:04 ` Boris Brezillon 2019-02-27 14:07 ` Miquel Raynal [this message] 2019-02-27 14:07 ` [RFC PATCH 25/27] mtd: nand: Add helpers to manage ECC engines and configurations Miquel Raynal 2019-02-27 14:30 ` Boris Brezillon 2019-02-27 14:30 ` Boris Brezillon
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190227150713.116a0ad3@xps13 \ --to=miquel.raynal@bootlin.com \ --cc=Tudor.Ambarus@microchip.com \ --cc=bbrezillon@kernel.org \ --cc=computersforpeace@gmail.com \ --cc=dwmw2@infradead.org \ --cc=frieder.schrempf@kontron.de \ --cc=juliensu@mxic.com.tw \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-mtd@lists.infradead.org \ --cc=marek.vasut@gmail.com \ --cc=masonccyang@mxic.com.tw \ --cc=richard@nod.at \ --cc=thomas.petazzoni@bootlin.com \ --cc=vigneshr@ti.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.