Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1659703 > unrolled thread
| Started by | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| First post | 2017-06-07 14:00 +0200 |
| Last post | 2017-06-11 22:20 +0200 |
| Articles | 20 on this page of 50 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH v5 00/23] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 22/23] mtd: nand: denali: use non-managed kmalloc() for DMA buffer Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 15/23] mtd: nand: denali: use interrupt instead of polling for bank reset Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 16/23] mtd: nand: denali: propagate page to helpers via function argument Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 18/23] mtd: nand: denali: use flag instead of register macro for direction Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 21/23] mtd: nand: denali: skip driver internal bounce buffer when possible Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 11/23] mtd: nand: denali: fix NAND_CMD_STATUS handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 19/23] mtd: nand: denali: fix raw and oob accessors for syndrome page layout Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
Re: [PATCH v5 19/23] mtd: nand: denali: fix raw and oob accessors for syndrome page layout Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 16:10 +0200
Re: [PATCH v5 19/23] mtd: nand: denali: fix raw and oob accessors for syndrome page layout Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 13:30 +0200
Re: [PATCH v5 19/23] mtd: nand: denali: fix raw and oob accessors for syndrome page layout Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 06:50 +0200
[PATCH v5 09/23] mtd: nand: denali: handle timing parameters by setup_data_interface() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 14/23] mtd: nand: denali: fix bank reset function to detect the number of chips Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 12/23] mtd: nand: denali: fix NAND_CMD_PARAM handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 17/23] mtd: nand: denali: merge struct nand_buf into struct denali_nand_info Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 06/23] mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
Re: [PATCH v5 06/23] mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 15:30 +0200
Re: [PATCH v5 06/23] mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 09:40 +0200
[PATCH v5 05/23] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
Re: [PATCH v5 05/23] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants Rob Herring <robh@kernel.org> - 2017-06-07 21:10 +0200
[PATCH v5 23/23] mtd: nand: denali: enable bad block table scan Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 01/23] mtd: nand: add generic helpers to check, match, maximize ECC settings Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 20/23] mtd: nand: denali: support hardware-assisted erased page detection Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 16:00 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 08:20 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 09:20 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 12:50 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 13:30 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 15:00 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 17:50 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 19:30 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 19:40 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-09 10:00 +0200
Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-13 06:50 +0200
[PATCH v5 04/23] mtd: nand: denali: remove Toshiba and Hynix specific fixup code Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 13/23] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:00 +0200
[PATCH v5 02/23] mtd: nand: add a shorthand to generate nand_ecc_caps structure Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:10 +0200
[PATCH v5 03/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:10 +0200
[PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 14:10 +0200
Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 15:40 +0200
Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 08:20 +0200
Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 09:10 +0200
Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 11:50 +0200
Re: [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 12:10 +0200
Re: [PATCH v5 00/23] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 08:20 +0200
Re: [PATCH v5 00/23] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 09:20 +0200
Re: [PATCH v5 00/23] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-08 09:20 +0200
Re: [PATCH v5 00/23] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-08 09:20 +0200
Re: [PATCH v5 00/23] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-11 22:20 +0200
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:00 +0200 |
| Subject | [PATCH v5 23/23] mtd: nand: denali: enable bad block table scan |
| Message-ID | <tPHN0-68d-31@gated-at.bofh.it> |
| In reply to | #1659703 |
Now this driver is ready to remove NAND_SKIP_BBTSCAN.
The BBT descriptors in denali.c are equivalent to the ones in
nand_bbt.c. There is no need to duplicate the equivalent structures.
The with-oob decriptors do not work for this driver anyway.
The bbt_pattern (offs = 8) and the version (veroffs = 12) area
overlaps the ECC area. Set NAND_BBT_NO_OOB flag to use the no_oob
variant of the BBT descriptors.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 31 ++-----------------------------
1 file changed, 2 insertions(+), 29 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 904f859e19b6..43006c506a6b 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1247,29 +1247,6 @@ static const struct mtd_ooblayout_ops denali_ooblayout_ops = {
.free = denali_ooblayout_free,
};
-static uint8_t bbt_pattern[] = {'B', 'b', 't', '0' };
-static uint8_t mirror_pattern[] = {'1', 't', 'b', 'B' };
-
-static struct nand_bbt_descr bbt_main_descr = {
- .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
- | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
- .offs = 8,
- .len = 4,
- .veroffs = 12,
- .maxblocks = 4,
- .pattern = bbt_pattern,
-};
-
-static struct nand_bbt_descr bbt_mirror_descr = {
- .options = NAND_BBT_LASTBLOCK | NAND_BBT_CREATE | NAND_BBT_WRITE
- | NAND_BBT_2BIT | NAND_BBT_VERSION | NAND_BBT_PERCHIP,
- .offs = 8,
- .len = 4,
- .veroffs = 12,
- .maxblocks = 4,
- .pattern = mirror_pattern,
-};
-
/* initialize driver data structures */
static void denali_drv_init(struct denali_nand_info *denali)
{
@@ -1412,13 +1389,9 @@ int denali_init(struct denali_nand_info *denali)
* bad block management.
*/
- /* Bad block management */
- chip->bbt_td = &bbt_main_descr;
- chip->bbt_md = &bbt_mirror_descr;
-
- /* skip the scan for now until we have OOB read and write support */
chip->bbt_options |= NAND_BBT_USE_FLASH;
- chip->options |= NAND_SKIP_BBTSCAN;
+ chip->bbt_options |= NAND_BBT_NO_OOB;
+
chip->ecc.mode = NAND_ECC_HW_SYNDROME;
/* no subpage writes on denali */
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:00 +0200 |
| Subject | [PATCH v5 01/23] mtd: nand: add generic helpers to check, match, maximize ECC settings |
| Message-ID | <tPHN1-68d-43@gated-at.bofh.it> |
| In reply to | #1659703 |
Driver are responsible for setting up ECC parameters correctly.
Those include:
- Check if ECC parameters specified (usually by DT) are valid
- Meet the chip's ECC requirement
- Maximize ECC strength if NAND_ECC_MAXIMIZE flag is set
The logic can be generalized by factoring out common code.
This commit adds 3 helpers to the NAND framework:
nand_check_ecc_caps - Check if preset step_size and strength are valid
nand_match_ecc_req - Match the chip's requirement
nand_maximize_ecc - Maximize the ECC strength
To use the helpers above, a driver needs to provide:
- Data array of supported ECC step size and strength
- A hook that calculates ECC bytes from the combination of
step_size and strength.
By using those helpers, code duplication among drivers will be
reduced.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5:
- Move oobavail from struct nand_ecc_caps to function argument
- Do not use goto label
- Drop mtd_info from func args
- Remove extra blank line
Changes in v4:
- Newly added
Changes in v3: None
Changes in v2: None
drivers/mtd/nand/nand_base.c | 220 +++++++++++++++++++++++++++++++++++++++++++
include/linux/mtd/nand.h | 33 +++++++
2 files changed, 253 insertions(+)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bdfa903cd355..dc0db1011761 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4509,6 +4509,226 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd)
}
}
+/**
+ * nand_check_ecc_caps - check the sanity of preset ECC settings
+ * @chip: nand chip info structure
+ * @caps: ECC caps info structure
+ * @oobavail: OOB size that the ECC engine can use
+ *
+ * When ECC step size and strength are already set, check if they are supported
+ * by the controller and the calculated ECC bytes fit within the chip's OOB.
+ * On success, the calculated ECC bytes is set.
+ */
+int nand_check_ecc_caps(struct nand_chip *chip,
+ const struct nand_ecc_caps *caps, int oobavail)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ const struct nand_ecc_step_info *stepinfo;
+ int preset_step = chip->ecc.size;
+ int preset_strength = chip->ecc.strength;
+ int nsteps, ecc_bytes;
+ int i, j;
+
+ if (WARN_ON(oobavail < 0))
+ return -EINVAL;
+
+ if (!preset_step || !preset_strength)
+ return -ENODATA;
+
+ nsteps = mtd->writesize / preset_step;
+
+ for (i = 0; i < caps->nstepinfos; i++) {
+ stepinfo = &caps->stepinfos[i];
+
+ if (stepinfo->stepsize != preset_step)
+ continue;
+
+ for (j = 0; j < stepinfo->nstrengths; j++) {
+ if (stepinfo->strengths[j] != preset_strength)
+ continue;
+
+ ecc_bytes = caps->calc_ecc_bytes(preset_step,
+ preset_strength);
+ if (WARN_ON_ONCE(ecc_bytes < 0))
+ return ecc_bytes;
+
+ if (ecc_bytes * nsteps > oobavail) {
+ pr_err("ECC (step, strength) = (%d, %d) does not fit in OOB",
+ preset_step, preset_strength);
+ return -ENOSPC;
+ }
+
+ chip->ecc.bytes = ecc_bytes;
+
+ return 0;
+ }
+ }
+
+ pr_err("ECC (step, strength) = (%d, %d) not supported on this controller",
+ preset_step, preset_strength);
+
+ return -ENOTSUPP;
+}
+EXPORT_SYMBOL_GPL(nand_check_ecc_caps);
+
+/**
+ * nand_match_ecc_req - meet the chip's requirement with least ECC bytes
+ * @chip: nand chip info structure
+ * @caps: ECC engine caps info structure
+ * @oobavail: OOB size that the ECC engine can use
+ *
+ * If a chip's ECC requirement is provided, try to meet it with the least
+ * number of ECC bytes (i.e. with the largest number of OOB-free bytes).
+ * On success, the chosen ECC settings are set.
+ */
+int nand_match_ecc_req(struct nand_chip *chip,
+ const struct nand_ecc_caps *caps, int oobavail)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ const struct nand_ecc_step_info *stepinfo;
+ int req_step = chip->ecc_step_ds;
+ int req_strength = chip->ecc_strength_ds;
+ int req_corr, step_size, strength, nsteps, ecc_bytes, ecc_bytes_total;
+ int best_step, best_strength, best_ecc_bytes;
+ int best_ecc_bytes_total = INT_MAX;
+ int i, j;
+
+ if (WARN_ON(oobavail < 0))
+ return -EINVAL;
+
+ /* No information provided by the NAND chip */
+ if (!req_step || !req_strength)
+ return -ENOTSUPP;
+
+ /* number of correctable bits the chip requires in a page */
+ req_corr = mtd->writesize / req_step * req_strength;
+
+ for (i = 0; i < caps->nstepinfos; i++) {
+ stepinfo = &caps->stepinfos[i];
+ step_size = stepinfo->stepsize;
+
+ for (j = 0; j < stepinfo->nstrengths; j++) {
+ strength = stepinfo->strengths[j];
+
+ /*
+ * If both step size and strength are smaller than the
+ * chip's requirement, it is not easy to compare the
+ * resulted reliability.
+ */
+ if (step_size < req_step && strength < req_strength)
+ continue;
+
+ if (mtd->writesize % step_size)
+ continue;
+
+ nsteps = mtd->writesize / step_size;
+
+ ecc_bytes = caps->calc_ecc_bytes(step_size, strength);
+ if (WARN_ON_ONCE(ecc_bytes < 0))
+ continue;
+ ecc_bytes_total = ecc_bytes * nsteps;
+
+ if (ecc_bytes_total > oobavail ||
+ strength * nsteps < req_corr)
+ continue;
+
+ /*
+ * We assume the best is to meet the chip's requrement
+ * with the least number of ECC bytes.
+ */
+ if (ecc_bytes_total < best_ecc_bytes_total) {
+ best_ecc_bytes_total = ecc_bytes_total;
+ best_step = step_size;
+ best_strength = strength;
+ best_ecc_bytes = ecc_bytes;
+ }
+ }
+ }
+
+ if (best_ecc_bytes_total == INT_MAX)
+ return -ENOTSUPP;
+
+ chip->ecc.size = best_step;
+ chip->ecc.strength = best_strength;
+ chip->ecc.bytes = best_ecc_bytes;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(nand_match_ecc_req);
+
+/**
+ * nand_maximize_ecc - choose the max ECC strength available
+ * @chip: nand chip info structure
+ * @caps: ECC engine caps info structure
+ * @oobavail: OOB size that the ECC engine can use
+ *
+ * Choose the max ECC strength that is supported on the controller, and can fit
+ * within the chip's OOB. On success, the chosen ECC settings are set.
+ */
+int nand_maximize_ecc(struct nand_chip *chip,
+ const struct nand_ecc_caps *caps, int oobavail)
+{
+ struct mtd_info *mtd = nand_to_mtd(chip);
+ const struct nand_ecc_step_info *stepinfo;
+ int step_size, strength, nsteps, ecc_bytes, corr;
+ int best_corr = 0;
+ int best_step = 0;
+ int best_strength, best_ecc_bytes;
+ int i, j;
+
+ if (WARN_ON(oobavail < 0))
+ return -EINVAL;
+
+ for (i = 0; i < caps->nstepinfos; i++) {
+ stepinfo = &caps->stepinfos[i];
+ step_size = stepinfo->stepsize;
+
+ /* If chip->ecc.size is already set, respect it */
+ if (chip->ecc.size && step_size != chip->ecc.size)
+ continue;
+
+ for (j = 0; j < stepinfo->nstrengths; j++) {
+ strength = stepinfo->strengths[j];
+
+ if (mtd->writesize % step_size)
+ continue;
+
+ nsteps = mtd->writesize / step_size;
+
+ ecc_bytes = caps->calc_ecc_bytes(step_size, strength);
+ if (WARN_ON_ONCE(ecc_bytes < 0))
+ continue;
+
+ if (ecc_bytes * nsteps > oobavail)
+ continue;
+
+ corr = strength * nsteps;
+
+ /*
+ * If the number of correctable bits is the same,
+ * bigger step_size has more reliability.
+ */
+ if (corr > best_corr ||
+ (corr == best_corr && step_size > best_step)) {
+ best_corr = corr;
+ best_step = step_size;
+ best_strength = strength;
+ best_ecc_bytes = ecc_bytes;
+ }
+ }
+ }
+
+ if (!best_corr)
+ return -ENOTSUPP;
+
+ chip->ecc.size = best_step;
+ chip->ecc.strength = best_strength;
+ chip->ecc.bytes = best_ecc_bytes;
+
+ return 0;
+}
+EXPORT_SYMBOL_GPL(nand_maximize_ecc);
+
/*
* Check if the chip configuration meet the datasheet requirements.
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index 8f67b1581683..c28a5792eed6 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -477,6 +477,30 @@ static inline void nand_hw_control_init(struct nand_hw_control *nfc)
}
/**
+ * struct nand_ecc_step_info - ECC step information of ECC engine
+ * @stepsize: data bytes per ECC step
+ * @strengths: array of supported strengths
+ * @nstrengths: number of supported strengths
+ */
+struct nand_ecc_step_info {
+ int stepsize;
+ const int *strengths;
+ int nstrengths;
+};
+
+/**
+ * struct nand_ecc_caps - capability of ECC engine
+ * @stepinfos: array of ECC step information
+ * @nstepinfos: number of ECC step information
+ * @calc_ecc_bytes: driver's hook to calculate ECC bytes per step
+ */
+struct nand_ecc_caps {
+ const struct nand_ecc_step_info *stepinfos;
+ int nstepinfos;
+ int (*calc_ecc_bytes)(int step_size, int strength);
+};
+
+/**
* struct nand_ecc_ctrl - Control structure for ECC
* @mode: ECC mode
* @algo: ECC algorithm
@@ -1244,6 +1268,15 @@ int nand_check_erased_ecc_chunk(void *data, int datalen,
void *extraoob, int extraooblen,
int threshold);
+int nand_check_ecc_caps(struct nand_chip *chip,
+ const struct nand_ecc_caps *caps, int oobavail);
+
+int nand_match_ecc_req(struct nand_chip *chip,
+ const struct nand_ecc_caps *caps, int oobavail);
+
+int nand_maximize_ecc(struct nand_chip *chip,
+ const struct nand_ecc_caps *caps, int oobavail);
+
/* Default write_oob implementation */
int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:00 +0200 |
| Subject | [PATCH v5 20/23] mtd: nand: denali: support hardware-assisted erased page detection |
| Message-ID | <tPHN1-68d-47@gated-at.bofh.it> |
| In reply to | #1659703 |
Recent versions of this IP support automatic erased page detection.
If an erased page is detected on reads, the controller does not set
INTR__ECC_UNCOR_ERR, but INTR__ERASED_PAGE.
The detection of erased page is based on the number of zeros in a
page; if the number of zeros is less than the value in the field
ERASED_THRESHOLD, the page is assumed as erased.
Please note ERASED_THRESHOLD specifies the number of zeros in a _page_
instead of an ECC chunk. Moreover, the controller does not provide a
way to know the actual number of bitflips.
Actually, an erased page (all 0xff) is not an ECC correctable pattern
on the Denali ECC engine. In other words, there is overlap between
the following two:
[1] a bit pattern reachable from a valid payload + ECC pattern within
ecc.strength bitflips
[2] a bit pattern reachable from an erased state (all 0xff) within
ecc.strength bitflips
So, this feature may intercept ECC correctable patterns, then replace
[1] with [2].
After all, this feature can work safely only when ECC_THRESHOLD == 1,
i.e. detect erased pages without any bitflips. This should be the
case most of the time. If there are some bitflips, the driver will
fallback to the software method by using nand_check_erased_ecc_chunk().
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5:
- Set ECC_THRESHOLD to 1
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 3 ++-
drivers/mtd/nand/denali.h | 5 +++++
2 files changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index dcadf9655d7a..90c702b9f14c 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1446,7 +1446,8 @@ int denali_init(struct denali_nand_info *denali)
"chosen ECC settings: step=%d, strength=%d, bytes=%d\n",
chip->ecc.size, chip->ecc.strength, chip->ecc.bytes);
- iowrite32(chip->ecc.strength, denali->flash_reg + ECC_CORRECTION);
+ iowrite32(MAKE_ECC_CORRECTION(chip->ecc.strength, 1),
+ denali->flash_reg + ECC_CORRECTION);
iowrite32(mtd->erasesize / mtd->writesize,
denali->flash_reg + PAGES_PER_BLOCK);
iowrite32(chip->options & NAND_BUSWIDTH_16 ? 1 : 0,
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index f5da52f09e34..657a794af695 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -110,6 +110,10 @@
#define ECC_CORRECTION 0x1b0
#define ECC_CORRECTION__VALUE GENMASK(4, 0)
+#define ECC_CORRECTION__ERASE_THRESHOLD GENMASK(31, 16)
+#define MAKE_ECC_CORRECTION(val, thresh) \
+ (((val) & (ECC_CORRECTION__VALUE)) | \
+ (((thresh) << 16) & (ECC_CORRECTION__ERASE_THRESHOLD)))
#define READ_MODE 0x1c0
#define READ_MODE__VALUE GENMASK(3, 0)
@@ -233,6 +237,7 @@
#define INTR__RST_COMP BIT(13)
#define INTR__PIPE_CMD_ERR BIT(14)
#define INTR__PAGE_XFER_INC BIT(15)
+#define INTR__ERASED_PAGE BIT(16)
#define PAGE_CNT(bank) (0x430 + (bank) * 0x50)
#define ERR_PAGE_ADDR(bank) (0x440 + (bank) * 0x50)
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:00 +0200 |
| Subject | [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tPHN1-68d-45@gated-at.bofh.it> |
| In reply to | #1659703 |
Simplify the interrupt handling and fix issues:
- The register field view of INTR_EN / INTR_STATUS is different
among IP versions. The global macro DENALI_IRQ_ALL is hard-coded
for Intel platforms. The interrupt mask should be determined at
run-time depending on the running platform.
- wait_for_irq() loops do {} while() until interested flags are
asserted. The logic can be simplified.
- The spin_lock() guard seems too complex (and suspicious in a race
condition if wait_for_completion_timeout() bails out by timeout).
- denali->complete is reused again and again, but reinit_completion()
is missing. Add it.
Re-work the code to make it more robust and easier to handle.
While we are here, also rename the jump label "failed_req_irq" to
more appropriate "disable_irq".
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 316 +++++++++++++++++-----------------------------
drivers/mtd/nand/denali.h | 1 +
2 files changed, 116 insertions(+), 201 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 8ad1e96f6d03..62798e6d7009 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -31,21 +31,6 @@ MODULE_LICENSE("GPL");
#define DENALI_NAND_NAME "denali-nand"
/*
- * We define a macro here that combines all interrupts this driver uses into
- * a single constant value, for convenience.
- */
-#define DENALI_IRQ_ALL (INTR__DMA_CMD_COMP | \
- INTR__ECC_TRANSACTION_DONE | \
- INTR__ECC_ERR | \
- INTR__PROGRAM_FAIL | \
- INTR__LOAD_COMP | \
- INTR__PROGRAM_COMP | \
- INTR__TIME_OUT | \
- INTR__ERASE_FAIL | \
- INTR__RST_COMP | \
- INTR__ERASE_COMP)
-
-/*
* indicates whether or not the internal value for the flash bank is
* valid or not
*/
@@ -71,20 +56,14 @@ static inline struct denali_nand_info *mtd_to_denali(struct mtd_info *mtd)
#define DENALI_READ 0
#define DENALI_WRITE 0x100
+#define DENALI_NR_BANKS 4
+
/*
* this is a helper macro that allows us to
* format the bank into the proper bits for the controller
*/
#define BANK(x) ((x) << 24)
-/* forward declarations */
-static void clear_interrupts(struct denali_nand_info *denali);
-static uint32_t wait_for_irq(struct denali_nand_info *denali,
- uint32_t irq_mask);
-static void denali_irq_enable(struct denali_nand_info *denali,
- uint32_t int_mask);
-static uint32_t read_interrupt_status(struct denali_nand_info *denali);
-
/*
* The bus interface clock, clk_x, is phase aligned with the core clock. The
* clk_x is an integral multiple N of the core clk. The value N is configured
@@ -143,22 +122,6 @@ static void read_status(struct denali_nand_info *denali)
write_byte_to_buf(denali, 0);
}
-/* resets a specific device connected to the core */
-static void reset_bank(struct denali_nand_info *denali)
-{
- uint32_t irq_status;
- uint32_t irq_mask = INTR__RST_COMP | INTR__TIME_OUT;
-
- clear_interrupts(denali);
-
- iowrite32(1 << denali->flash_bank, denali->flash_reg + DEVICE_RESET);
-
- irq_status = wait_for_irq(denali, irq_mask);
-
- if (irq_status & INTR__TIME_OUT)
- dev_err(denali->dev, "reset bank failed.\n");
-}
-
/* Reset the flash controller */
static uint16_t denali_nand_reset(struct denali_nand_info *denali)
{
@@ -201,169 +164,123 @@ static void detect_max_banks(struct denali_nand_info *denali)
denali->max_banks <<= 1;
}
-static void denali_set_intr_modes(struct denali_nand_info *denali,
- uint16_t INT_ENABLE)
+static void denali_enable_irq(struct denali_nand_info *denali)
{
- if (INT_ENABLE)
- iowrite32(1, denali->flash_reg + GLOBAL_INT_ENABLE);
- else
- iowrite32(0, denali->flash_reg + GLOBAL_INT_ENABLE);
-}
+ int i;
-/*
- * validation function to verify that the controlling software is making
- * a valid request
- */
-static inline bool is_flash_bank_valid(int flash_bank)
-{
- return flash_bank >= 0 && flash_bank < 4;
+ for (i = 0; i < DENALI_NR_BANKS; i++)
+ iowrite32(U32_MAX, denali->flash_reg + INTR_EN(i));
+ iowrite32(GLOBAL_INT_EN_FLAG, denali->flash_reg + GLOBAL_INT_ENABLE);
}
-static void denali_irq_init(struct denali_nand_info *denali)
+static void denali_disable_irq(struct denali_nand_info *denali)
{
- uint32_t int_mask;
int i;
- /* Disable global interrupts */
- denali_set_intr_modes(denali, false);
-
- int_mask = DENALI_IRQ_ALL;
-
- /* Clear all status bits */
- for (i = 0; i < denali->max_banks; ++i)
- iowrite32(0xFFFF, denali->flash_reg + INTR_STATUS(i));
-
- denali_irq_enable(denali, int_mask);
+ for (i = 0; i < DENALI_NR_BANKS; i++)
+ iowrite32(0, denali->flash_reg + INTR_EN(i));
+ iowrite32(0, denali->flash_reg + GLOBAL_INT_ENABLE);
}
-static void denali_irq_cleanup(int irqnum, struct denali_nand_info *denali)
+static void denali_clear_irq(struct denali_nand_info *denali,
+ int bank, uint32_t irq_status)
{
- denali_set_intr_modes(denali, false);
+ /* write one to clear bits */
+ iowrite32(irq_status, denali->flash_reg + INTR_STATUS(bank));
}
-static void denali_irq_enable(struct denali_nand_info *denali,
- uint32_t int_mask)
+static void denali_clear_irq_all(struct denali_nand_info *denali)
{
int i;
- for (i = 0; i < denali->max_banks; ++i)
- iowrite32(int_mask, denali->flash_reg + INTR_EN(i));
+ for (i = 0; i < DENALI_NR_BANKS; i++)
+ denali_clear_irq(denali, i, U32_MAX);
}
-/*
- * This function only returns when an interrupt that this driver cares about
- * occurs. This is to reduce the overhead of servicing interrupts
- */
-static inline uint32_t denali_irq_detected(struct denali_nand_info *denali)
+static irqreturn_t denali_isr(int irq, void *dev_id)
{
- return read_interrupt_status(denali) & DENALI_IRQ_ALL;
-}
+ struct denali_nand_info *denali = dev_id;
+ irqreturn_t ret = IRQ_NONE;
+ uint32_t irq_status;
+ int i;
-/* Interrupts are cleared by writing a 1 to the appropriate status bit */
-static inline void clear_interrupt(struct denali_nand_info *denali,
- uint32_t irq_mask)
-{
- uint32_t intr_status_reg;
+ spin_lock(&denali->irq_lock);
- intr_status_reg = INTR_STATUS(denali->flash_bank);
+ for (i = 0; i < DENALI_NR_BANKS; i++) {
+ irq_status = ioread32(denali->flash_reg + INTR_STATUS(i));
+ if (irq_status)
+ ret = IRQ_HANDLED;
- iowrite32(irq_mask, denali->flash_reg + intr_status_reg);
-}
+ denali_clear_irq(denali, i, irq_status);
-static void clear_interrupts(struct denali_nand_info *denali)
-{
- uint32_t status;
+ if (i != denali->flash_bank)
+ continue;
+
+ denali->irq_status |= irq_status;
- spin_lock_irq(&denali->irq_lock);
+ if (denali->irq_status & denali->irq_mask)
+ complete(&denali->complete);
+ }
- status = read_interrupt_status(denali);
- clear_interrupt(denali, status);
+ spin_unlock(&denali->irq_lock);
- denali->irq_status = 0x0;
- spin_unlock_irq(&denali->irq_lock);
+ return ret;
}
-static uint32_t read_interrupt_status(struct denali_nand_info *denali)
+static void denali_reset_irq(struct denali_nand_info *denali)
{
- uint32_t intr_status_reg;
-
- intr_status_reg = INTR_STATUS(denali->flash_bank);
+ unsigned long flags;
- return ioread32(denali->flash_reg + intr_status_reg);
+ spin_lock_irqsave(&denali->irq_lock, flags);
+ denali->irq_status = 0;
+ denali->irq_mask = 0;
+ spin_unlock_irqrestore(&denali->irq_lock, flags);
}
-/*
- * This is the interrupt service routine. It handles all interrupts
- * sent to this device. Note that on CE4100, this is a shared interrupt.
- */
-static irqreturn_t denali_isr(int irq, void *dev_id)
+static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
+ uint32_t irq_mask)
{
- struct denali_nand_info *denali = dev_id;
+ unsigned long time_left, flags;
uint32_t irq_status;
- irqreturn_t result = IRQ_NONE;
- spin_lock(&denali->irq_lock);
+ spin_lock_irqsave(&denali->irq_lock, flags);
- /* check to see if a valid NAND chip has been selected. */
- if (is_flash_bank_valid(denali->flash_bank)) {
- /*
- * check to see if controller generated the interrupt,
- * since this is a shared interrupt
- */
- irq_status = denali_irq_detected(denali);
- if (irq_status != 0) {
- /* handle interrupt */
- /* first acknowledge it */
- clear_interrupt(denali, irq_status);
- /*
- * store the status in the device context for someone
- * to read
- */
- denali->irq_status |= irq_status;
- /* notify anyone who cares that it happened */
- complete(&denali->complete);
- /* tell the OS that we've handled this */
- result = IRQ_HANDLED;
- }
+ irq_status = denali->irq_status;
+
+ if (irq_mask & irq_status) {
+ spin_unlock_irqrestore(&denali->irq_lock, flags);
+ return irq_status;
}
- spin_unlock(&denali->irq_lock);
- return result;
+
+ denali->irq_mask = irq_mask;
+ reinit_completion(&denali->complete);
+ spin_unlock_irqrestore(&denali->irq_lock, flags);
+
+ time_left = wait_for_completion_timeout(&denali->complete,
+ msecs_to_jiffies(1000));
+ if (!time_left) {
+ dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
+ denali->irq_mask);
+ return 0;
+ }
+
+ return denali->irq_status;
}
-static uint32_t wait_for_irq(struct denali_nand_info *denali, uint32_t irq_mask)
+/* resets a specific device connected to the core */
+static void reset_bank(struct denali_nand_info *denali)
{
- unsigned long comp_res;
- uint32_t intr_status;
- unsigned long timeout = msecs_to_jiffies(1000);
+ uint32_t irq_status;
- do {
- comp_res =
- wait_for_completion_timeout(&denali->complete, timeout);
- spin_lock_irq(&denali->irq_lock);
- intr_status = denali->irq_status;
-
- if (intr_status & irq_mask) {
- denali->irq_status &= ~irq_mask;
- spin_unlock_irq(&denali->irq_lock);
- /* our interrupt was detected */
- break;
- }
+ denali_reset_irq(denali);
- /*
- * these are not the interrupts you are looking for -
- * need to wait again
- */
- spin_unlock_irq(&denali->irq_lock);
- } while (comp_res != 0);
+ iowrite32(1 << denali->flash_bank, denali->flash_reg + DEVICE_RESET);
- if (comp_res == 0) {
- /* timeout */
- pr_err("timeout occurred, status = 0x%x, mask = 0x%x\n",
- intr_status, irq_mask);
+ irq_status = denali_wait_for_irq(denali,
+ INTR__RST_COMP | INTR__TIME_OUT);
- intr_status = 0;
- }
- return intr_status;
+ if (!(irq_status & INTR__RST_COMP))
+ dev_err(denali->dev, "reset bank failed.\n");
}
/*
@@ -397,7 +314,7 @@ static int denali_send_pipeline_cmd(struct denali_nand_info *denali,
setup_ecc_for_xfer(denali, ecc_en, transfer_spare);
- clear_interrupts(denali);
+ denali_reset_irq(denali);
addr = BANK(denali->flash_bank) | denali->page;
@@ -479,9 +396,9 @@ static int write_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
write_data_to_flash_mem(denali, buf, mtd->oobsize);
/* wait for operation to complete */
- irq_status = wait_for_irq(denali, irq_mask);
+ irq_status = denali_wait_for_irq(denali, irq_mask);
- if (irq_status == 0) {
+ if (!(irq_status & INTR__PROGRAM_COMP)) {
dev_err(denali->dev, "OOB write failed\n");
status = -EIO;
}
@@ -510,9 +427,9 @@ static void read_oob_data(struct mtd_info *mtd, uint8_t *buf, int page)
* can always use status0 bit as the
* mask is identical for each bank.
*/
- irq_status = wait_for_irq(denali, irq_mask);
+ irq_status = denali_wait_for_irq(denali, irq_mask);
- if (irq_status == 0)
+ if (!(irq_status & INTR__LOAD_COMP))
dev_err(denali->dev, "page on OOB timeout %d\n",
denali->page);
@@ -620,9 +537,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
unsigned int err_byte, err_sector, err_device;
uint8_t err_cor_value;
unsigned int prev_sector = 0;
+ uint32_t irq_status;
- /* read the ECC errors. we'll ignore them for now */
- denali_set_intr_modes(denali, false);
+ denali_reset_irq(denali);
do {
err_addr = ioread32(denali->flash_reg + ECC_ERROR_ADDRESS);
@@ -674,10 +591,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
* ECC_TRANSACTION_DONE interrupt, so here just wait for
* a while for this interrupt
*/
- while (!(read_interrupt_status(denali) & INTR__ECC_TRANSACTION_DONE))
- cpu_relax();
- clear_interrupts(denali);
- denali_set_intr_modes(denali, true);
+ irq_status = denali_wait_for_irq(denali, INTR__ECC_TRANSACTION_DONE);
+ if (!(irq_status & INTR__ECC_TRANSACTION_DONE))
+ return -EIO;
return max_bitflips;
}
@@ -778,15 +694,14 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
dma_sync_single_for_device(denali->dev, addr, size, DMA_TO_DEVICE);
- clear_interrupts(denali);
+ denali_reset_irq(denali);
denali_enable_dma(denali, true);
denali_setup_dma(denali, DENALI_WRITE);
/* wait for operation to complete */
- irq_status = wait_for_irq(denali, irq_mask);
-
- if (irq_status == 0) {
+ irq_status = denali_wait_for_irq(denali, irq_mask);
+ if (!(irq_status & INTR__DMA_CMD_COMP)) {
dev_err(denali->dev, "timeout on write_page (type = %d)\n",
raw_xfer);
ret = -EIO;
@@ -865,11 +780,11 @@ static int denali_read_page(struct mtd_info *mtd, struct nand_chip *chip,
denali_enable_dma(denali, true);
dma_sync_single_for_device(denali->dev, addr, size, DMA_FROM_DEVICE);
- clear_interrupts(denali);
+ denali_reset_irq(denali);
denali_setup_dma(denali, DENALI_READ);
/* wait for operation to complete */
- irq_status = wait_for_irq(denali, irq_mask);
+ irq_status = denali_wait_for_irq(denali, irq_mask);
dma_sync_single_for_cpu(denali->dev, addr, size, DMA_FROM_DEVICE);
@@ -901,6 +816,7 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
dma_addr_t addr = denali->buf.dma_buf;
size_t size = mtd->writesize + mtd->oobsize;
uint32_t irq_mask = INTR__DMA_CMD_COMP;
+ uint32_t irq_status;
denali->page = page;
@@ -909,11 +825,13 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
dma_sync_single_for_device(denali->dev, addr, size, DMA_FROM_DEVICE);
- clear_interrupts(denali);
+ denali_reset_irq(denali);
denali_setup_dma(denali, DENALI_READ);
/* wait for operation to complete */
- wait_for_irq(denali, irq_mask);
+ irq_status = denali_wait_for_irq(denali, irq_mask);
+ if (irq_status & INTR__DMA_CMD_COMP)
+ return -ETIMEDOUT;
dma_sync_single_for_cpu(denali->dev, addr, size, DMA_FROM_DEVICE);
@@ -940,9 +858,7 @@ static void denali_select_chip(struct mtd_info *mtd, int chip)
{
struct denali_nand_info *denali = mtd_to_denali(mtd);
- spin_lock_irq(&denali->irq_lock);
denali->flash_bank = chip;
- spin_unlock_irq(&denali->irq_lock);
}
static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip)
@@ -953,19 +869,19 @@ static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip)
static int denali_erase(struct mtd_info *mtd, int page)
{
struct denali_nand_info *denali = mtd_to_denali(mtd);
-
uint32_t cmd, irq_status;
- clear_interrupts(denali);
+ denali_reset_irq(denali);
/* setup page read request for access type */
cmd = MODE_10 | BANK(denali->flash_bank) | page;
index_addr(denali, cmd, 0x1);
/* wait for erase to complete or failure to occur */
- irq_status = wait_for_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
+ irq_status = denali_wait_for_irq(denali,
+ INTR__ERASE_COMP | INTR__ERASE_FAIL);
- return irq_status & INTR__ERASE_FAIL ? NAND_STATUS_FAIL : PASS;
+ return irq_status & INTR__ERASE_COMP ? 0 : NAND_STATUS_FAIL;
}
static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
@@ -1153,7 +1069,6 @@ static void denali_hw_init(struct denali_nand_info *denali)
/* Should set value for these registers when init */
iowrite32(0, denali->flash_reg + TWO_ROW_ADDR_CYCLES);
iowrite32(1, denali->flash_reg + ECC_ENABLE);
- denali_irq_init(denali);
}
int denali_calc_ecc_bytes(int step_size, int strength)
@@ -1265,9 +1180,6 @@ static void denali_drv_init(struct denali_nand_info *denali)
/* indicate that MTD has not selected a valid bank yet */
denali->flash_bank = CHIP_SELECT_INVALID;
-
- /* initialize our irq_status variable to indicate no interrupts */
- denali->irq_status = 0;
}
static int denali_multidev_fixup(struct denali_nand_info *denali)
@@ -1337,6 +1249,8 @@ int denali_init(struct denali_nand_info *denali)
denali_hw_init(denali);
denali_drv_init(denali);
+ denali_clear_irq_all(denali);
+
/* Request IRQ after all the hardware initialization is finished */
ret = devm_request_irq(denali->dev, denali->irq, denali_isr,
IRQF_SHARED, DENALI_NAND_NAME, denali);
@@ -1345,8 +1259,8 @@ int denali_init(struct denali_nand_info *denali)
return ret;
}
- /* now that our ISR is registered, we can enable interrupts */
- denali_set_intr_modes(denali, true);
+ denali_enable_irq(denali);
+
nand_set_flash_node(chip, denali->dev->of_node);
/* Fallback to the default name if DT did not give "label" property */
if (!mtd->name)
@@ -1368,7 +1282,7 @@ int denali_init(struct denali_nand_info *denali)
*/
ret = nand_scan_ident(mtd, denali->max_banks, NULL);
if (ret)
- goto failed_req_irq;
+ goto disable_irq;
/* allocate the right size buffer now */
devm_kfree(denali->dev, denali->buf.buf);
@@ -1377,7 +1291,7 @@ int denali_init(struct denali_nand_info *denali)
GFP_KERNEL);
if (!denali->buf.buf) {
ret = -ENOMEM;
- goto failed_req_irq;
+ goto disable_irq;
}
ret = dma_set_mask(denali->dev,
@@ -1385,7 +1299,7 @@ int denali_init(struct denali_nand_info *denali)
64 : 32));
if (ret) {
dev_err(denali->dev, "No usable DMA configuration\n");
- goto failed_req_irq;
+ goto disable_irq;
}
denali->buf.dma_buf = dma_map_single(denali->dev, denali->buf.buf,
@@ -1394,7 +1308,7 @@ int denali_init(struct denali_nand_info *denali)
if (dma_mapping_error(denali->dev, denali->buf.dma_buf)) {
dev_err(denali->dev, "Failed to map DMA buffer\n");
ret = -EIO;
- goto failed_req_irq;
+ goto disable_irq;
}
/*
@@ -1418,7 +1332,7 @@ int denali_init(struct denali_nand_info *denali)
ret = denali_ecc_setup(mtd, chip, denali);
if (ret) {
dev_err(denali->dev, "Failed to setup ECC settings.\n");
- goto failed_req_irq;
+ goto disable_irq;
}
dev_dbg(denali->dev,
@@ -1452,21 +1366,21 @@ int denali_init(struct denali_nand_info *denali)
ret = denali_multidev_fixup(denali);
if (ret)
- goto failed_req_irq;
+ goto disable_irq;
ret = nand_scan_tail(mtd);
if (ret)
- goto failed_req_irq;
+ goto disable_irq;
ret = mtd_device_register(mtd, NULL, 0);
if (ret) {
dev_err(denali->dev, "Failed to register MTD: %d\n", ret);
- goto failed_req_irq;
+ goto disable_irq;
}
return 0;
-failed_req_irq:
- denali_irq_cleanup(denali->irq, denali);
+disable_irq:
+ denali_disable_irq(denali);
return ret;
}
@@ -1484,7 +1398,7 @@ void denali_remove(struct denali_nand_info *denali)
int bufsize = mtd->writesize + mtd->oobsize;
nand_release(mtd);
- denali_irq_cleanup(denali->irq, denali);
+ denali_disable_irq(denali);
dma_unmap_single(denali->dev, denali->buf.dma_buf, bufsize,
DMA_BIDIRECTIONAL);
}
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index fb473895a79d..a0ac0f84f8b5 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -325,6 +325,7 @@ struct denali_nand_info {
/* elements used by ISR */
struct completion complete;
spinlock_t irq_lock;
+ uint32_t irq_mask;
uint32_t irq_status;
int irq;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 16:00 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tPJF8-7mI-19@gated-at.bofh.it> |
| In reply to | #1659724 |
On Wed, 7 Jun 2017 20:52:19 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> -/*
> - * This is the interrupt service routine. It handles all interrupts
> - * sent to this device. Note that on CE4100, this is a shared interrupt.
> - */
> -static irqreturn_t denali_isr(int irq, void *dev_id)
> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
> + uint32_t irq_mask)
> {
> - struct denali_nand_info *denali = dev_id;
> + unsigned long time_left, flags;
> uint32_t irq_status;
> - irqreturn_t result = IRQ_NONE;
>
> - spin_lock(&denali->irq_lock);
> + spin_lock_irqsave(&denali->irq_lock, flags);
>
> - /* check to see if a valid NAND chip has been selected. */
> - if (is_flash_bank_valid(denali->flash_bank)) {
> - /*
> - * check to see if controller generated the interrupt,
> - * since this is a shared interrupt
> - */
> - irq_status = denali_irq_detected(denali);
> - if (irq_status != 0) {
> - /* handle interrupt */
> - /* first acknowledge it */
> - clear_interrupt(denali, irq_status);
> - /*
> - * store the status in the device context for someone
> - * to read
> - */
> - denali->irq_status |= irq_status;
> - /* notify anyone who cares that it happened */
> - complete(&denali->complete);
> - /* tell the OS that we've handled this */
> - result = IRQ_HANDLED;
> - }
> + irq_status = denali->irq_status;
> +
> + if (irq_mask & irq_status) {
> + spin_unlock_irqrestore(&denali->irq_lock, flags);
> + return irq_status;
> }
> - spin_unlock(&denali->irq_lock);
> - return result;
> +
> + denali->irq_mask = irq_mask;
> + reinit_completion(&denali->complete);
These 2 instructions should be done before calling
denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
you might loose events if they happen between your irq_status read and
the reinit_completion() call. You should also clear existing interrupts
before launching your operation, otherwise you might wakeup on previous
events.
> + spin_unlock_irqrestore(&denali->irq_lock, flags);
> +
> + time_left = wait_for_completion_timeout(&denali->complete,
> + msecs_to_jiffies(1000));
> + if (!time_left) {
> + dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
> + denali->irq_mask);
> + return 0;
> + }
> +
> + return denali->irq_status;
> }
>
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-08 08:20 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tPYXv-yI-7@gated-at.bofh.it> |
| In reply to | #1659826 |
Hi Boris,
2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 7 Jun 2017 20:52:19 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>
>> -/*
>> - * This is the interrupt service routine. It handles all interrupts
>> - * sent to this device. Note that on CE4100, this is a shared interrupt.
>> - */
>> -static irqreturn_t denali_isr(int irq, void *dev_id)
>> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
>> + uint32_t irq_mask)
>> {
>> - struct denali_nand_info *denali = dev_id;
>> + unsigned long time_left, flags;
>> uint32_t irq_status;
>> - irqreturn_t result = IRQ_NONE;
>>
>> - spin_lock(&denali->irq_lock);
>> + spin_lock_irqsave(&denali->irq_lock, flags);
>>
>> - /* check to see if a valid NAND chip has been selected. */
>> - if (is_flash_bank_valid(denali->flash_bank)) {
>> - /*
>> - * check to see if controller generated the interrupt,
>> - * since this is a shared interrupt
>> - */
>> - irq_status = denali_irq_detected(denali);
>> - if (irq_status != 0) {
>> - /* handle interrupt */
>> - /* first acknowledge it */
>> - clear_interrupt(denali, irq_status);
>> - /*
>> - * store the status in the device context for someone
>> - * to read
>> - */
>> - denali->irq_status |= irq_status;
>> - /* notify anyone who cares that it happened */
>> - complete(&denali->complete);
>> - /* tell the OS that we've handled this */
>> - result = IRQ_HANDLED;
>> - }
>> + irq_status = denali->irq_status;
>> +
>> + if (irq_mask & irq_status) {
>> + spin_unlock_irqrestore(&denali->irq_lock, flags);
>> + return irq_status;
>> }
>> - spin_unlock(&denali->irq_lock);
>> - return result;
>> +
>> + denali->irq_mask = irq_mask;
>> + reinit_completion(&denali->complete);
>
> These 2 instructions should be done before calling
> denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
> you might loose events if they happen between your irq_status read and
> the reinit_completion() call.
No.
denali->irq_lock avoids a race between denali_isr() and
denali_wait_for_irq().
The line
denali->irq_status |= irq_status;
in denali_isr() accumulates all events that have happened
since denali_reset_irq().
If the interested IRQs have already happened
before denali_wait_for_irq(), it just return immediately
without using completion.
I do not mind adding a comment like below
if you think my intention is unclear, though.
/* Return immediately if interested IRQs have already happend. */
if (irq_mask & irq_status) {
spin_unlock_irqrestore(&denali->irq_lock, flags);
return irq_status;
}
> You should also clear existing interrupts
> before launching your operation, otherwise you might wakeup on previous
> events.
I do not see a point in your suggestion.
denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.
IRQ events triggered by previous events are accumulated in denali->irq_status.
denali_reset_irq() clears it.
denali->irq_status = 0;
Again, denali->irq_lock avoids a race between denali_reset_irq() and
denali_irq(),
so this works correctly.
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-08 09:20 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tPZTA-1ak-7@gated-at.bofh.it> |
| In reply to | #1660780 |
Le Thu, 8 Jun 2017 15:10:18 +0900,
Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
> Hi Boris,
>
>
> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 7 Jun 2017 20:52:19 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >
> >> -/*
> >> - * This is the interrupt service routine. It handles all interrupts
> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
> >> - */
> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
> >> + uint32_t irq_mask)
> >> {
> >> - struct denali_nand_info *denali = dev_id;
> >> + unsigned long time_left, flags;
> >> uint32_t irq_status;
> >> - irqreturn_t result = IRQ_NONE;
> >>
> >> - spin_lock(&denali->irq_lock);
> >> + spin_lock_irqsave(&denali->irq_lock, flags);
> >>
> >> - /* check to see if a valid NAND chip has been selected. */
> >> - if (is_flash_bank_valid(denali->flash_bank)) {
> >> - /*
> >> - * check to see if controller generated the interrupt,
> >> - * since this is a shared interrupt
> >> - */
> >> - irq_status = denali_irq_detected(denali);
> >> - if (irq_status != 0) {
> >> - /* handle interrupt */
> >> - /* first acknowledge it */
> >> - clear_interrupt(denali, irq_status);
> >> - /*
> >> - * store the status in the device context for someone
> >> - * to read
> >> - */
> >> - denali->irq_status |= irq_status;
> >> - /* notify anyone who cares that it happened */
> >> - complete(&denali->complete);
> >> - /* tell the OS that we've handled this */
> >> - result = IRQ_HANDLED;
> >> - }
> >> + irq_status = denali->irq_status;
> >> +
> >> + if (irq_mask & irq_status) {
> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> + return irq_status;
> >> }
> >> - spin_unlock(&denali->irq_lock);
> >> - return result;
> >> +
> >> + denali->irq_mask = irq_mask;
> >> + reinit_completion(&denali->complete);
> >
> > These 2 instructions should be done before calling
> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
> > you might loose events if they happen between your irq_status read and
> > the reinit_completion() call.
>
> No.
>
> denali->irq_lock avoids a race between denali_isr() and
> denali_wait_for_irq().
>
>
> The line
> denali->irq_status |= irq_status;
> in denali_isr() accumulates all events that have happened
> since denali_reset_irq().
>
> If the interested IRQs have already happened
> before denali_wait_for_irq(), it just return immediately
> without using completion.
>
> I do not mind adding a comment like below
> if you think my intention is unclear, though.
>
> /* Return immediately if interested IRQs have already happend. */
> if (irq_mask & irq_status) {
> spin_unlock_irqrestore(&denali->irq_lock, flags);
> return irq_status;
> }
>
>
My bad, I didn't notice you were releasing the lock after calling
reinit_completion(). I still find this solution more complex than my
proposal, but I don't care that much.
>
>
>
> > You should also clear existing interrupts
> > before launching your operation, otherwise you might wakeup on previous
> > events.
>
>
> I do not see a point in your suggestion.
>
> denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.
>
> IRQ events triggered by previous events are accumulated in denali->irq_status.
>
> denali_reset_irq() clears it.
>
> denali->irq_status = 0;
Well, it was just a precaution, in case some interrupts weren't cleared
during the previous test (for example if they were masked before the
event actually happened, which can occur if you have a timeout, but
the event is detected afterward).
>
>
> Again, denali->irq_lock avoids a race between denali_reset_irq() and
> denali_irq(),
> so this works correctly.
>
>
Anyway, you seem confident that you're doing the right thing, so I'll
let you decide what is appropriate and redirect any bug report to you if
that happens :-P.
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-08 12:50 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQ3aN-3bB-5@gated-at.bofh.it> |
| In reply to | #1660815 |
Hi Boris,
2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Le Thu, 8 Jun 2017 15:10:18 +0900,
> Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
>
>> Hi Boris,
>>
>>
>> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Wed, 7 Jun 2017 20:52:19 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >
>> >> -/*
>> >> - * This is the interrupt service routine. It handles all interrupts
>> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
>> >> - */
>> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
>> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
>> >> + uint32_t irq_mask)
>> >> {
>> >> - struct denali_nand_info *denali = dev_id;
>> >> + unsigned long time_left, flags;
>> >> uint32_t irq_status;
>> >> - irqreturn_t result = IRQ_NONE;
>> >>
>> >> - spin_lock(&denali->irq_lock);
>> >> + spin_lock_irqsave(&denali->irq_lock, flags);
>> >>
>> >> - /* check to see if a valid NAND chip has been selected. */
>> >> - if (is_flash_bank_valid(denali->flash_bank)) {
>> >> - /*
>> >> - * check to see if controller generated the interrupt,
>> >> - * since this is a shared interrupt
>> >> - */
>> >> - irq_status = denali_irq_detected(denali);
>> >> - if (irq_status != 0) {
>> >> - /* handle interrupt */
>> >> - /* first acknowledge it */
>> >> - clear_interrupt(denali, irq_status);
>> >> - /*
>> >> - * store the status in the device context for someone
>> >> - * to read
>> >> - */
>> >> - denali->irq_status |= irq_status;
>> >> - /* notify anyone who cares that it happened */
>> >> - complete(&denali->complete);
>> >> - /* tell the OS that we've handled this */
>> >> - result = IRQ_HANDLED;
>> >> - }
>> >> + irq_status = denali->irq_status;
>> >> +
>> >> + if (irq_mask & irq_status) {
>> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> + return irq_status;
>> >> }
>> >> - spin_unlock(&denali->irq_lock);
>> >> - return result;
>> >> +
>> >> + denali->irq_mask = irq_mask;
>> >> + reinit_completion(&denali->complete);
>> >
>> > These 2 instructions should be done before calling
>> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
>> > you might loose events if they happen between your irq_status read and
>> > the reinit_completion() call.
>>
>> No.
>>
>> denali->irq_lock avoids a race between denali_isr() and
>> denali_wait_for_irq().
>>
>>
>> The line
>> denali->irq_status |= irq_status;
>> in denali_isr() accumulates all events that have happened
>> since denali_reset_irq().
>>
>> If the interested IRQs have already happened
>> before denali_wait_for_irq(), it just return immediately
>> without using completion.
>>
>> I do not mind adding a comment like below
>> if you think my intention is unclear, though.
>>
>> /* Return immediately if interested IRQs have already happend. */
>> if (irq_mask & irq_status) {
>> spin_unlock_irqrestore(&denali->irq_lock, flags);
>> return irq_status;
>> }
>>
>>
>
> My bad, I didn't notice you were releasing the lock after calling
> reinit_completion(). I still find this solution more complex than my
> proposal, but I don't care that much.
At first, I implemented exactly like you suggested;
denali->irq_mask = irq_mask;
reinit_completion(&denali->complete)
in denali_reset_irq().
IIRC, things were like this.
Some time later, you memtioned to use ->cmd_ctrl
instead of ->cmdfunc.
Then I had a problem when I needed to implement
denali_check_irq() in
http://patchwork.ozlabs.org/patch/772395/
denali_wait_for_irq() is blocked until interested IRQ happens.
but ->dev_ready() hook should not be blocked.
It should return if R/B# transition has happened or not.
So, I accumulate IRQ events in denali->irq_status
that have happened since denali_reset_irq().
>>
>>
>>
>> > You should also clear existing interrupts
>> > before launching your operation, otherwise you might wakeup on previous
>> > events.
>>
>>
>> I do not see a point in your suggestion.
>>
>> denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.
>>
>> IRQ events triggered by previous events are accumulated in denali->irq_status.
>>
>> denali_reset_irq() clears it.
>>
>> denali->irq_status = 0;
>
> Well, it was just a precaution, in case some interrupts weren't cleared
> during the previous test (for example if they were masked before the
> event actually happened, which can occur if you have a timeout, but
> the event is detected afterward).
Turning on/off IRQ mask is problematic.
So I did not do that.
I enable IRQ mask in driver probe.
I think this approach is more robust when we consider race conditions
like you mentioned.
>>
>>
>> Again, denali->irq_lock avoids a race between denali_reset_irq() and
>> denali_irq(),
>> so this works correctly.
>>
>>
>
> Anyway, you seem confident that you're doing the right thing, so I'll
> let you decide what is appropriate and redirect any bug report to you if
> that happens :-P.
Yeah.
I came up with this solution after my long thought and efforts,
so I'd like to go with this.
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-08 13:30 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQ3Nv-3Dr-9@gated-at.bofh.it> |
| In reply to | #1661052 |
On Thu, 8 Jun 2017 19:41:39 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
>
> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > Le Thu, 8 Jun 2017 15:10:18 +0900,
> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
> >
> >> Hi Boris,
> >>
> >>
> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> > On Wed, 7 Jun 2017 20:52:19 +0900
> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >
> >> >
> >> >> -/*
> >> >> - * This is the interrupt service routine. It handles all interrupts
> >> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
> >> >> - */
> >> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
> >> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
> >> >> + uint32_t irq_mask)
> >> >> {
> >> >> - struct denali_nand_info *denali = dev_id;
> >> >> + unsigned long time_left, flags;
> >> >> uint32_t irq_status;
> >> >> - irqreturn_t result = IRQ_NONE;
> >> >>
> >> >> - spin_lock(&denali->irq_lock);
> >> >> + spin_lock_irqsave(&denali->irq_lock, flags);
> >> >>
> >> >> - /* check to see if a valid NAND chip has been selected. */
> >> >> - if (is_flash_bank_valid(denali->flash_bank)) {
> >> >> - /*
> >> >> - * check to see if controller generated the interrupt,
> >> >> - * since this is a shared interrupt
> >> >> - */
> >> >> - irq_status = denali_irq_detected(denali);
> >> >> - if (irq_status != 0) {
> >> >> - /* handle interrupt */
> >> >> - /* first acknowledge it */
> >> >> - clear_interrupt(denali, irq_status);
> >> >> - /*
> >> >> - * store the status in the device context for someone
> >> >> - * to read
> >> >> - */
> >> >> - denali->irq_status |= irq_status;
> >> >> - /* notify anyone who cares that it happened */
> >> >> - complete(&denali->complete);
> >> >> - /* tell the OS that we've handled this */
> >> >> - result = IRQ_HANDLED;
> >> >> - }
> >> >> + irq_status = denali->irq_status;
> >> >> +
> >> >> + if (irq_mask & irq_status) {
> >> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> >> + return irq_status;
> >> >> }
> >> >> - spin_unlock(&denali->irq_lock);
> >> >> - return result;
> >> >> +
> >> >> + denali->irq_mask = irq_mask;
> >> >> + reinit_completion(&denali->complete);
> >> >
> >> > These 2 instructions should be done before calling
> >> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
> >> > you might loose events if they happen between your irq_status read and
> >> > the reinit_completion() call.
> >>
> >> No.
> >>
> >> denali->irq_lock avoids a race between denali_isr() and
> >> denali_wait_for_irq().
> >>
> >>
> >> The line
> >> denali->irq_status |= irq_status;
> >> in denali_isr() accumulates all events that have happened
> >> since denali_reset_irq().
> >>
> >> If the interested IRQs have already happened
> >> before denali_wait_for_irq(), it just return immediately
> >> without using completion.
> >>
> >> I do not mind adding a comment like below
> >> if you think my intention is unclear, though.
> >>
> >> /* Return immediately if interested IRQs have already happend. */
> >> if (irq_mask & irq_status) {
> >> spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> return irq_status;
> >> }
> >>
> >>
> >
> > My bad, I didn't notice you were releasing the lock after calling
> > reinit_completion(). I still find this solution more complex than my
> > proposal, but I don't care that much.
>
>
> At first, I implemented exactly like you suggested;
> denali->irq_mask = irq_mask;
> reinit_completion(&denali->complete)
> in denali_reset_irq().
>
>
> IIRC, things were like this.
>
> Some time later, you memtioned to use ->cmd_ctrl
> instead of ->cmdfunc.
>
> Then I had a problem when I needed to implement
> denali_check_irq() in
> http://patchwork.ozlabs.org/patch/772395/
>
> denali_wait_for_irq() is blocked until interested IRQ happens.
> but ->dev_ready() hook should not be blocked.
> It should return if R/B# transition has happened or not.
Nope, it should return whether the NAND is ready or not, not whether a
busy -> ready transition occurred or not. It's typically done by
reading the NAND STATUS register or by checking the R/B pin status.
> So, I accumulate IRQ events in denali->irq_status
> that have happened since denali_reset_irq().
Yep, I see that.
>
>
>
> >>
> >>
> >>
> >> > You should also clear existing interrupts
> >> > before launching your operation, otherwise you might wakeup on previous
> >> > events.
> >>
> >>
> >> I do not see a point in your suggestion.
> >>
> >> denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.
> >>
> >> IRQ events triggered by previous events are accumulated in denali->irq_status.
> >>
> >> denali_reset_irq() clears it.
> >>
> >> denali->irq_status = 0;
> >
> > Well, it was just a precaution, in case some interrupts weren't cleared
> > during the previous test (for example if they were masked before the
> > event actually happened, which can occur if you have a timeout, but
> > the event is detected afterward).
>
> Turning on/off IRQ mask is problematic.
> So I did not do that.
I don't see why this is a problem. That's how it usually done.
>
> I enable IRQ mask in driver probe.
> I think this approach is more robust when we consider race conditions
> like you mentioned.
I'd like to hear more about the reasons you think it's more robust
than
* at-probe-time: mask all IRQs and reset IRQ status
* when doing a specific operation:
1/ reset irq status
2/ unmask relevant irqs (based on the operation you're doing)
3/ launch the operation
4/ wait for interrupts
5/ mask irqs and check the wait_for_completion() return code + irq
status
This approach shouldn't be racy, because you're resetting+unmasking
irqs before starting the real operation (the one supposed to generate
such interrupts). By doing that you also get rid of the extra
->irq_status field, and you don't have to check irq_status before
calling wait_for_completion().
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-08 15:00 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQ5cC-4oc-15@gated-at.bofh.it> |
| In reply to | #1661068 |
Hi Boris,
2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 8 Jun 2017 19:41:39 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>>
>> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > Le Thu, 8 Jun 2017 15:10:18 +0900,
>> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
>> >
>> >> Hi Boris,
>> >>
>> >>
>> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> > On Wed, 7 Jun 2017 20:52:19 +0900
>> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> >
>> >> >
>> >> >> -/*
>> >> >> - * This is the interrupt service routine. It handles all interrupts
>> >> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
>> >> >> - */
>> >> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
>> >> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
>> >> >> + uint32_t irq_mask)
>> >> >> {
>> >> >> - struct denali_nand_info *denali = dev_id;
>> >> >> + unsigned long time_left, flags;
>> >> >> uint32_t irq_status;
>> >> >> - irqreturn_t result = IRQ_NONE;
>> >> >>
>> >> >> - spin_lock(&denali->irq_lock);
>> >> >> + spin_lock_irqsave(&denali->irq_lock, flags);
>> >> >>
>> >> >> - /* check to see if a valid NAND chip has been selected. */
>> >> >> - if (is_flash_bank_valid(denali->flash_bank)) {
>> >> >> - /*
>> >> >> - * check to see if controller generated the interrupt,
>> >> >> - * since this is a shared interrupt
>> >> >> - */
>> >> >> - irq_status = denali_irq_detected(denali);
>> >> >> - if (irq_status != 0) {
>> >> >> - /* handle interrupt */
>> >> >> - /* first acknowledge it */
>> >> >> - clear_interrupt(denali, irq_status);
>> >> >> - /*
>> >> >> - * store the status in the device context for someone
>> >> >> - * to read
>> >> >> - */
>> >> >> - denali->irq_status |= irq_status;
>> >> >> - /* notify anyone who cares that it happened */
>> >> >> - complete(&denali->complete);
>> >> >> - /* tell the OS that we've handled this */
>> >> >> - result = IRQ_HANDLED;
>> >> >> - }
>> >> >> + irq_status = denali->irq_status;
>> >> >> +
>> >> >> + if (irq_mask & irq_status) {
>> >> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> >> + return irq_status;
>> >> >> }
>> >> >> - spin_unlock(&denali->irq_lock);
>> >> >> - return result;
>> >> >> +
>> >> >> + denali->irq_mask = irq_mask;
>> >> >> + reinit_completion(&denali->complete);
>> >> >
>> >> > These 2 instructions should be done before calling
>> >> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
>> >> > you might loose events if they happen between your irq_status read and
>> >> > the reinit_completion() call.
>> >>
>> >> No.
>> >>
>> >> denali->irq_lock avoids a race between denali_isr() and
>> >> denali_wait_for_irq().
>> >>
>> >>
>> >> The line
>> >> denali->irq_status |= irq_status;
>> >> in denali_isr() accumulates all events that have happened
>> >> since denali_reset_irq().
>> >>
>> >> If the interested IRQs have already happened
>> >> before denali_wait_for_irq(), it just return immediately
>> >> without using completion.
>> >>
>> >> I do not mind adding a comment like below
>> >> if you think my intention is unclear, though.
>> >>
>> >> /* Return immediately if interested IRQs have already happend. */
>> >> if (irq_mask & irq_status) {
>> >> spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> return irq_status;
>> >> }
>> >>
>> >>
>> >
>> > My bad, I didn't notice you were releasing the lock after calling
>> > reinit_completion(). I still find this solution more complex than my
>> > proposal, but I don't care that much.
>>
>>
>> At first, I implemented exactly like you suggested;
>> denali->irq_mask = irq_mask;
>> reinit_completion(&denali->complete)
>> in denali_reset_irq().
>>
>>
>> IIRC, things were like this.
>>
>> Some time later, you memtioned to use ->cmd_ctrl
>> instead of ->cmdfunc.
>>
>> Then I had a problem when I needed to implement
>> denali_check_irq() in
>> http://patchwork.ozlabs.org/patch/772395/
>>
>> denali_wait_for_irq() is blocked until interested IRQ happens.
>> but ->dev_ready() hook should not be blocked.
>> It should return if R/B# transition has happened or not.
>
> Nope, it should return whether the NAND is ready or not, not whether a
> busy -> ready transition occurred or not. It's typically done by
> reading the NAND STATUS register or by checking the R/B pin status.
Checking the R/B pin is probably impossible unless
the pin is changed into a GPIO port.
I also considered NAND_CMD_STATUS, but
I can not recall why I chose the current approach.
Perhaps I thought returning detected IRQ
is faster than accessing the chip for NAND_CMD_STATUS.
I can try NAND_CMD_STATUS approach if you like.
>> So, I accumulate IRQ events in denali->irq_status
>> that have happened since denali_reset_irq().
>
> Yep, I see that.
>
>>
>>
>>
>> >>
>> >>
>> >>
>> >> > You should also clear existing interrupts
>> >> > before launching your operation, otherwise you might wakeup on previous
>> >> > events.
>> >>
>> >>
>> >> I do not see a point in your suggestion.
>> >>
>> >> denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.
>> >>
>> >> IRQ events triggered by previous events are accumulated in denali->irq_status.
>> >>
>> >> denali_reset_irq() clears it.
>> >>
>> >> denali->irq_status = 0;
>> >
>> > Well, it was just a precaution, in case some interrupts weren't cleared
>> > during the previous test (for example if they were masked before the
>> > event actually happened, which can occur if you have a timeout, but
>> > the event is detected afterward).
>>
>> Turning on/off IRQ mask is problematic.
>> So I did not do that.
>
> I don't see why this is a problem. That's how it usually done.
>
>>
>> I enable IRQ mask in driver probe.
>> I think this approach is more robust when we consider race conditions
>> like you mentioned.
>
> I'd like to hear more about the reasons you think it's more robust
> than
>
> * at-probe-time: mask all IRQs and reset IRQ status
>
> * when doing a specific operation:
> 1/ reset irq status
> 2/ unmask relevant irqs (based on the operation you're doing)
> 3/ launch the operation
> 4/ wait for interrupts
> 5/ mask irqs and check the wait_for_completion() return code + irq
> status
>
> This approach shouldn't be racy, because you're resetting+unmasking
> irqs before starting the real operation (the one supposed to generate
> such interrupts). By doing that you also get rid of the extra
> ->irq_status field, and you don't have to check irq_status before
> calling wait_for_completion().
IIRC, I was thinking like this:
One IRQ line may be shared among multiple hardware including Denali.
denali_pci may do this.
The Denali IRQ handler need to check irq status
because it should return IRQ_HANDLED if the event comes from Denali controller.
Otherwise, the event comes from different hardware, so
Denali IRQ handler should return IRQ_NONE.
wait_for_completion_timeout() may bail out with timeout error,
then proceed to denali_reset_irq() for the next operation.
Afterwards, the event actually may happen, and invoke IRQ handler.
denali_reset_irq() and denali_isr() compete to grab the spin lock.
If denali_reset_irq() wins, it clears INTR_STATUS register
(if implemented like you suggested first) or changes IRQ mask for the
next event.
After that, denali_isr enters the critical section and checks IRQ bit
but at this moment, the IRQ bit has gone. So, it assumes this event
is not for Denali, so returns IRQ_NONE. Nobody returns IRQ_HANDLED.
Then, kernel will complain "irq *: nobody cared"
In my opinion, IRQ should be checked and cleared in one place
(in IRQ handler).
Enabling/disabling IRQ mask is not problem unless it masks out
already-asserted IRQ status bits.
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-08 17:50 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQ7R8-66J-11@gated-at.bofh.it> |
| In reply to | #1661155 |
On Thu, 8 Jun 2017 21:58:00 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 8 Jun 2017 19:41:39 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> Hi Boris,
> >>
> >>
> >> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> > Le Thu, 8 Jun 2017 15:10:18 +0900,
> >> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
> >> >
> >> >> Hi Boris,
> >> >>
> >> >>
> >> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> >> > On Wed, 7 Jun 2017 20:52:19 +0900
> >> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >> >
> >> >> >
> >> >> >> -/*
> >> >> >> - * This is the interrupt service routine. It handles all interrupts
> >> >> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
> >> >> >> - */
> >> >> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
> >> >> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
> >> >> >> + uint32_t irq_mask)
> >> >> >> {
> >> >> >> - struct denali_nand_info *denali = dev_id;
> >> >> >> + unsigned long time_left, flags;
> >> >> >> uint32_t irq_status;
> >> >> >> - irqreturn_t result = IRQ_NONE;
> >> >> >>
> >> >> >> - spin_lock(&denali->irq_lock);
> >> >> >> + spin_lock_irqsave(&denali->irq_lock, flags);
> >> >> >>
> >> >> >> - /* check to see if a valid NAND chip has been selected. */
> >> >> >> - if (is_flash_bank_valid(denali->flash_bank)) {
> >> >> >> - /*
> >> >> >> - * check to see if controller generated the interrupt,
> >> >> >> - * since this is a shared interrupt
> >> >> >> - */
> >> >> >> - irq_status = denali_irq_detected(denali);
> >> >> >> - if (irq_status != 0) {
> >> >> >> - /* handle interrupt */
> >> >> >> - /* first acknowledge it */
> >> >> >> - clear_interrupt(denali, irq_status);
> >> >> >> - /*
> >> >> >> - * store the status in the device context for someone
> >> >> >> - * to read
> >> >> >> - */
> >> >> >> - denali->irq_status |= irq_status;
> >> >> >> - /* notify anyone who cares that it happened */
> >> >> >> - complete(&denali->complete);
> >> >> >> - /* tell the OS that we've handled this */
> >> >> >> - result = IRQ_HANDLED;
> >> >> >> - }
> >> >> >> + irq_status = denali->irq_status;
> >> >> >> +
> >> >> >> + if (irq_mask & irq_status) {
> >> >> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> >> >> + return irq_status;
> >> >> >> }
> >> >> >> - spin_unlock(&denali->irq_lock);
> >> >> >> - return result;
> >> >> >> +
> >> >> >> + denali->irq_mask = irq_mask;
> >> >> >> + reinit_completion(&denali->complete);
> >> >> >
> >> >> > These 2 instructions should be done before calling
> >> >> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
> >> >> > you might loose events if they happen between your irq_status read and
> >> >> > the reinit_completion() call.
> >> >>
> >> >> No.
> >> >>
> >> >> denali->irq_lock avoids a race between denali_isr() and
> >> >> denali_wait_for_irq().
> >> >>
> >> >>
> >> >> The line
> >> >> denali->irq_status |= irq_status;
> >> >> in denali_isr() accumulates all events that have happened
> >> >> since denali_reset_irq().
> >> >>
> >> >> If the interested IRQs have already happened
> >> >> before denali_wait_for_irq(), it just return immediately
> >> >> without using completion.
> >> >>
> >> >> I do not mind adding a comment like below
> >> >> if you think my intention is unclear, though.
> >> >>
> >> >> /* Return immediately if interested IRQs have already happend. */
> >> >> if (irq_mask & irq_status) {
> >> >> spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> >> return irq_status;
> >> >> }
> >> >>
> >> >>
> >> >
> >> > My bad, I didn't notice you were releasing the lock after calling
> >> > reinit_completion(). I still find this solution more complex than my
> >> > proposal, but I don't care that much.
> >>
> >>
> >> At first, I implemented exactly like you suggested;
> >> denali->irq_mask = irq_mask;
> >> reinit_completion(&denali->complete)
> >> in denali_reset_irq().
> >>
> >>
> >> IIRC, things were like this.
> >>
> >> Some time later, you memtioned to use ->cmd_ctrl
> >> instead of ->cmdfunc.
> >>
> >> Then I had a problem when I needed to implement
> >> denali_check_irq() in
> >> http://patchwork.ozlabs.org/patch/772395/
> >>
> >> denali_wait_for_irq() is blocked until interested IRQ happens.
> >> but ->dev_ready() hook should not be blocked.
> >> It should return if R/B# transition has happened or not.
> >
> > Nope, it should return whether the NAND is ready or not, not whether a
> > busy -> ready transition occurred or not. It's typically done by
> > reading the NAND STATUS register or by checking the R/B pin status.
>
> Checking the R/B pin is probably impossible unless
> the pin is changed into a GPIO port.
>
> I also considered NAND_CMD_STATUS, but
> I can not recall why I chose the current approach.
> Perhaps I thought returning detected IRQ
> is faster than accessing the chip for NAND_CMD_STATUS.
>
> I can try NAND_CMD_STATUS approach if you like.
Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
are expected to wait for chip readiness before returning.
You could also implement ->waitfunc() using denali_wait_for_irq() if
you're able to detect R/B transitions, but I'm not sure it's worth it,
because you overload almost all the methods using this hook (the only
one remaining is ->onfi_set_features(), and using STATUS polling should
not be an issue in this case).
Implementing ->dev_ready() is not necessary. When not provided, the
core falls back to STATUS polling and you seem to support
NAND_CMD_STATUS in denali_cmdfunc(). Note that even if it's not fully
reliable in the current driver, you're switching to ->cmd_ctrl() at the
end of the series anyway, so we should be good after that.
>
>
>
>
>
> >> So, I accumulate IRQ events in denali->irq_status
> >> that have happened since denali_reset_irq().
> >
> > Yep, I see that.
> >
> >>
> >>
> >>
> >> >>
> >> >>
> >> >>
> >> >> > You should also clear existing interrupts
> >> >> > before launching your operation, otherwise you might wakeup on previous
> >> >> > events.
> >> >>
> >> >>
> >> >> I do not see a point in your suggestion.
> >> >>
> >> >> denali_isr() reads out IRQ_STATUS(i) and immediately clears IRQ bits.
> >> >>
> >> >> IRQ events triggered by previous events are accumulated in denali->irq_status.
> >> >>
> >> >> denali_reset_irq() clears it.
> >> >>
> >> >> denali->irq_status = 0;
> >> >
> >> > Well, it was just a precaution, in case some interrupts weren't cleared
> >> > during the previous test (for example if they were masked before the
> >> > event actually happened, which can occur if you have a timeout, but
> >> > the event is detected afterward).
> >>
> >> Turning on/off IRQ mask is problematic.
> >> So I did not do that.
> >
> > I don't see why this is a problem. That's how it usually done.
> >
> >>
> >> I enable IRQ mask in driver probe.
> >> I think this approach is more robust when we consider race conditions
> >> like you mentioned.
> >
> > I'd like to hear more about the reasons you think it's more robust
> > than
> >
> > * at-probe-time: mask all IRQs and reset IRQ status
> >
> > * when doing a specific operation:
> > 1/ reset irq status
> > 2/ unmask relevant irqs (based on the operation you're doing)
> > 3/ launch the operation
> > 4/ wait for interrupts
> > 5/ mask irqs and check the wait_for_completion() return code + irq
> > status
> >
> > This approach shouldn't be racy, because you're resetting+unmasking
> > irqs before starting the real operation (the one supposed to generate
> > such interrupts). By doing that you also get rid of the extra
> > ->irq_status field, and you don't have to check irq_status before
> > calling wait_for_completion().
>
>
> IIRC, I was thinking like this:
>
> One IRQ line may be shared among multiple hardware including Denali.
> denali_pci may do this.
>
> The Denali IRQ handler need to check irq status
> because it should return IRQ_HANDLED if the event comes from Denali controller.
> Otherwise, the event comes from different hardware, so
> Denali IRQ handler should return IRQ_NONE.
Correct.
>
> wait_for_completion_timeout() may bail out with timeout error,
> then proceed to denali_reset_irq() for the next operation.
Before calling denali_reset_irq() you should re-mask the irqs you
unmasked in #1. Actually, calling denali_reset_irq() after
wait_for_completion_timeout() is not even needed here because you'll
clear pending irqs before launching the next NAND command.
> Afterwards, the event actually may happen, and invoke IRQ handler.
Not if you masked IRQs after wait_for_completion_timeout() returned.
>
> denali_reset_irq() and denali_isr() compete to grab the spin lock.
>
> If denali_reset_irq() wins, it clears INTR_STATUS register
> (if implemented like you suggested first) or changes IRQ mask for the
> next event.
> After that, denali_isr enters the critical section and checks IRQ bit
> but at this moment, the IRQ bit has gone. So, it assumes this event
> is not for Denali, so returns IRQ_NONE. Nobody returns IRQ_HANDLED.
Not if you have masked the interrupts.
>
> Then, kernel will complain "irq *: nobody cared"
>
>
> In my opinion, IRQ should be checked and cleared in one place
> (in IRQ handler).
>
> Enabling/disabling IRQ mask is not problem unless it masks out
> already-asserted IRQ status bits.
Here is a patch to show you what I had in mind [1] (it applies on top
of this patch). AFAICT, there's no races, no interrupt loss, and you
get rid of the ->irq_mask/status/lock fields.
[1]http://code.bulix.org/fufia6-145571
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-08 19:30 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQ9pT-79G-1@gated-at.bofh.it> |
| In reply to | #1661412 |
Hi Boris
2017-06-09 0:43 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Thu, 8 Jun 2017 21:58:00 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris,
>>
>> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Thu, 8 Jun 2017 19:41:39 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >>
>> >> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> > Le Thu, 8 Jun 2017 15:10:18 +0900,
>> >> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
>> >> >
>> >> >> Hi Boris,
>> >> >>
>> >> >>
>> >> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> >> > On Wed, 7 Jun 2017 20:52:19 +0900
>> >> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> >> >
>> >> >> >
>> >> >> >> -/*
>> >> >> >> - * This is the interrupt service routine. It handles all interrupts
>> >> >> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
>> >> >> >> - */
>> >> >> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
>> >> >> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
>> >> >> >> + uint32_t irq_mask)
>> >> >> >> {
>> >> >> >> - struct denali_nand_info *denali = dev_id;
>> >> >> >> + unsigned long time_left, flags;
>> >> >> >> uint32_t irq_status;
>> >> >> >> - irqreturn_t result = IRQ_NONE;
>> >> >> >>
>> >> >> >> - spin_lock(&denali->irq_lock);
>> >> >> >> + spin_lock_irqsave(&denali->irq_lock, flags);
>> >> >> >>
>> >> >> >> - /* check to see if a valid NAND chip has been selected. */
>> >> >> >> - if (is_flash_bank_valid(denali->flash_bank)) {
>> >> >> >> - /*
>> >> >> >> - * check to see if controller generated the interrupt,
>> >> >> >> - * since this is a shared interrupt
>> >> >> >> - */
>> >> >> >> - irq_status = denali_irq_detected(denali);
>> >> >> >> - if (irq_status != 0) {
>> >> >> >> - /* handle interrupt */
>> >> >> >> - /* first acknowledge it */
>> >> >> >> - clear_interrupt(denali, irq_status);
>> >> >> >> - /*
>> >> >> >> - * store the status in the device context for someone
>> >> >> >> - * to read
>> >> >> >> - */
>> >> >> >> - denali->irq_status |= irq_status;
>> >> >> >> - /* notify anyone who cares that it happened */
>> >> >> >> - complete(&denali->complete);
>> >> >> >> - /* tell the OS that we've handled this */
>> >> >> >> - result = IRQ_HANDLED;
>> >> >> >> - }
>> >> >> >> + irq_status = denali->irq_status;
>> >> >> >> +
>> >> >> >> + if (irq_mask & irq_status) {
>> >> >> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> >> >> + return irq_status;
>> >> >> >> }
>> >> >> >> - spin_unlock(&denali->irq_lock);
>> >> >> >> - return result;
>> >> >> >> +
>> >> >> >> + denali->irq_mask = irq_mask;
>> >> >> >> + reinit_completion(&denali->complete);
>> >> >> >
>> >> >> > These 2 instructions should be done before calling
>> >> >> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
>> >> >> > you might loose events if they happen between your irq_status read and
>> >> >> > the reinit_completion() call.
>> >> >>
>> >> >> No.
>> >> >>
>> >> >> denali->irq_lock avoids a race between denali_isr() and
>> >> >> denali_wait_for_irq().
>> >> >>
>> >> >>
>> >> >> The line
>> >> >> denali->irq_status |= irq_status;
>> >> >> in denali_isr() accumulates all events that have happened
>> >> >> since denali_reset_irq().
>> >> >>
>> >> >> If the interested IRQs have already happened
>> >> >> before denali_wait_for_irq(), it just return immediately
>> >> >> without using completion.
>> >> >>
>> >> >> I do not mind adding a comment like below
>> >> >> if you think my intention is unclear, though.
>> >> >>
>> >> >> /* Return immediately if interested IRQs have already happend. */
>> >> >> if (irq_mask & irq_status) {
>> >> >> spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> >> return irq_status;
>> >> >> }
>> >> >>
>> >> >>
>> >> >
>> >> > My bad, I didn't notice you were releasing the lock after calling
>> >> > reinit_completion(). I still find this solution more complex than my
>> >> > proposal, but I don't care that much.
>> >>
>> >>
>> >> At first, I implemented exactly like you suggested;
>> >> denali->irq_mask = irq_mask;
>> >> reinit_completion(&denali->complete)
>> >> in denali_reset_irq().
>> >>
>> >>
>> >> IIRC, things were like this.
>> >>
>> >> Some time later, you memtioned to use ->cmd_ctrl
>> >> instead of ->cmdfunc.
>> >>
>> >> Then I had a problem when I needed to implement
>> >> denali_check_irq() in
>> >> http://patchwork.ozlabs.org/patch/772395/
>> >>
>> >> denali_wait_for_irq() is blocked until interested IRQ happens.
>> >> but ->dev_ready() hook should not be blocked.
>> >> It should return if R/B# transition has happened or not.
>> >
>> > Nope, it should return whether the NAND is ready or not, not whether a
>> > busy -> ready transition occurred or not. It's typically done by
>> > reading the NAND STATUS register or by checking the R/B pin status.
>>
>> Checking the R/B pin is probably impossible unless
>> the pin is changed into a GPIO port.
>>
>> I also considered NAND_CMD_STATUS, but
>> I can not recall why I chose the current approach.
>> Perhaps I thought returning detected IRQ
>> is faster than accessing the chip for NAND_CMD_STATUS.
>>
>> I can try NAND_CMD_STATUS approach if you like.
>
> Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
> inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
> perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
> are expected to wait for chip readiness before returning.
>
> You could also implement ->waitfunc() using denali_wait_for_irq() if
> you're able to detect R/B transitions,
R/B transition will set INTR__INT_ACT interrupt.
I think it is easy in my implementation of denali_wait_for_irq(),
like
denali_wait_for_irq(denali, INTR__INT_ACT);
But, you are suggesting me to change it.
In your way, you give IRQ masks to denali_reset_irq(), like
denali_reset_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
Then, we have no room of IRQ bit in denali_wait_for_irq().
How will you implement it?
> but I'm not sure it's worth it,
> because you overload almost all the methods using this hook (the only
> one remaining is ->onfi_set_features(), and using STATUS polling should
> not be an issue in this case).
>
> Implementing ->dev_ready() is not necessary. When not provided, the
> core falls back to STATUS polling and you seem to support
> NAND_CMD_STATUS in denali_cmdfunc(). Note that even if it's not fully
> reliable in the current driver, you're switching to ->cmd_ctrl() at the
> end of the series anyway, so we should be good after that.
->dev_ready() is optional, but we may end up with waiting more than needed.
case NAND_CMD_RESET:
if (chip->dev_ready)
break;
udelay(chip->chip_delay);
chip->chip_delay is probably set large enough, so this is not optimal.
If I add something more, the following two bugs were found by
denali_dev_ready().
commit 3158fa0e739615769cc047d2428f30f4c3b6640e
commit c5d664aa5a4c4b257a54eb35045031630d105f49
If NAND core is fine, denali_dev_ready() works fine too.
If not, it is a sign of bug of nand_command(_lp).
This is contributing to the core improvement.
>>
>> IIRC, I was thinking like this:
>>
>> One IRQ line may be shared among multiple hardware including Denali.
>> denali_pci may do this.
>>
>> The Denali IRQ handler need to check irq status
>> because it should return IRQ_HANDLED if the event comes from Denali controller.
>> Otherwise, the event comes from different hardware, so
>> Denali IRQ handler should return IRQ_NONE.
>
> Correct.
>
>>
>> wait_for_completion_timeout() may bail out with timeout error,
>> then proceed to denali_reset_irq() for the next operation.
>
> Before calling denali_reset_irq() you should re-mask the irqs you
> unmasked in #1. Actually, calling denali_reset_irq() after
> wait_for_completion_timeout() is not even needed here because you'll
> clear pending irqs before launching the next NAND command.
>
>> Afterwards, the event actually may happen, and invoke IRQ handler.
>
> Not if you masked IRQs after wait_for_completion_timeout() returned.
wait_for_completion_timeout(&denali->complete, msecs_to_jiffies(1000));
<<< WHAT IF IRQ EVENT HAPPENS HERE ? >>>
iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
Also, you ignore the return value of wait_for_completion_timeout(),
then drop my precious error message()
dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
denali->irq_mask)
> Here is a patch to show you what I had in mind [1] (it applies on top
> of this patch). AFAICT, there's no races, no interrupt loss, and you
> get rid of the ->irq_mask/status/lock fields.
>
> [1]http://code.bulix.org/fufia6-145571
>
Problem Scenario A
[1] wait_for_completion_timeout() exits with timeout.
[2] IRQ happens and denali_isr() is invoked
[3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
[4] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
ioread32(denali->flash_reg + INTR_EN(bank));
(status is set to 0 because INTR_EN(bank) is now 0)
[5] return IRQ_NONE;
[6] kernel complains "irq *: nobody cared"
Problem Scenario B (unlikely to happen, though)
[1] wait_for_completion_timeout() exits with timeout.
[2] IRQ happens and denali_isr() is invoked
[3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
[4] chip->select_chip(mtd, -1)
[5] denali->flash_bank = -1
[6] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
ioread32(denali->flash_reg + INTR_EN(bank));
( access to non-existing INTR_STATUS(-1) )
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-08 19:40 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQ9zA-7f5-27@gated-at.bofh.it> |
| In reply to | #1661509 |
2017-06-09 2:26 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> ->dev_ready() is optional, but we may end up with waiting more than needed.
>
> case NAND_CMD_RESET:
> if (chip->dev_ready)
> break;
> udelay(chip->chip_delay);
>
>
> chip->chip_delay is probably set large enough, so this is not optimal.
I misunderstood the code.
The following line will be the most of the part of delay.
nand_wait_status_ready(mtd, 250);
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-09 10:00 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tQmZP-7c4-9@gated-at.bofh.it> |
| In reply to | #1661509 |
Hi Masahiro,
On Fri, 9 Jun 2017 02:26:34 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris
>
> 2017-06-09 0:43 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Thu, 8 Jun 2017 21:58:00 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> Hi Boris,
> >>
> >> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> > On Thu, 8 Jun 2017 19:41:39 +0900
> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >
> >> >> Hi Boris,
> >> >>
> >> >>
> >> >> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> >> > Le Thu, 8 Jun 2017 15:10:18 +0900,
> >> >> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
> >> >> >
> >> >> >> Hi Boris,
> >> >> >>
> >> >> >>
> >> >> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> >> >> >> > On Wed, 7 Jun 2017 20:52:19 +0900
> >> >> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> >> >> >
> >> >> >> >
> >> >> >> >> -/*
> >> >> >> >> - * This is the interrupt service routine. It handles all interrupts
> >> >> >> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
> >> >> >> >> - */
> >> >> >> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
> >> >> >> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
> >> >> >> >> + uint32_t irq_mask)
> >> >> >> >> {
> >> >> >> >> - struct denali_nand_info *denali = dev_id;
> >> >> >> >> + unsigned long time_left, flags;
> >> >> >> >> uint32_t irq_status;
> >> >> >> >> - irqreturn_t result = IRQ_NONE;
> >> >> >> >>
> >> >> >> >> - spin_lock(&denali->irq_lock);
> >> >> >> >> + spin_lock_irqsave(&denali->irq_lock, flags);
> >> >> >> >>
> >> >> >> >> - /* check to see if a valid NAND chip has been selected. */
> >> >> >> >> - if (is_flash_bank_valid(denali->flash_bank)) {
> >> >> >> >> - /*
> >> >> >> >> - * check to see if controller generated the interrupt,
> >> >> >> >> - * since this is a shared interrupt
> >> >> >> >> - */
> >> >> >> >> - irq_status = denali_irq_detected(denali);
> >> >> >> >> - if (irq_status != 0) {
> >> >> >> >> - /* handle interrupt */
> >> >> >> >> - /* first acknowledge it */
> >> >> >> >> - clear_interrupt(denali, irq_status);
> >> >> >> >> - /*
> >> >> >> >> - * store the status in the device context for someone
> >> >> >> >> - * to read
> >> >> >> >> - */
> >> >> >> >> - denali->irq_status |= irq_status;
> >> >> >> >> - /* notify anyone who cares that it happened */
> >> >> >> >> - complete(&denali->complete);
> >> >> >> >> - /* tell the OS that we've handled this */
> >> >> >> >> - result = IRQ_HANDLED;
> >> >> >> >> - }
> >> >> >> >> + irq_status = denali->irq_status;
> >> >> >> >> +
> >> >> >> >> + if (irq_mask & irq_status) {
> >> >> >> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> >> >> >> + return irq_status;
> >> >> >> >> }
> >> >> >> >> - spin_unlock(&denali->irq_lock);
> >> >> >> >> - return result;
> >> >> >> >> +
> >> >> >> >> + denali->irq_mask = irq_mask;
> >> >> >> >> + reinit_completion(&denali->complete);
> >> >> >> >
> >> >> >> > These 2 instructions should be done before calling
> >> >> >> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
> >> >> >> > you might loose events if they happen between your irq_status read and
> >> >> >> > the reinit_completion() call.
> >> >> >>
> >> >> >> No.
> >> >> >>
> >> >> >> denali->irq_lock avoids a race between denali_isr() and
> >> >> >> denali_wait_for_irq().
> >> >> >>
> >> >> >>
> >> >> >> The line
> >> >> >> denali->irq_status |= irq_status;
> >> >> >> in denali_isr() accumulates all events that have happened
> >> >> >> since denali_reset_irq().
> >> >> >>
> >> >> >> If the interested IRQs have already happened
> >> >> >> before denali_wait_for_irq(), it just return immediately
> >> >> >> without using completion.
> >> >> >>
> >> >> >> I do not mind adding a comment like below
> >> >> >> if you think my intention is unclear, though.
> >> >> >>
> >> >> >> /* Return immediately if interested IRQs have already happend. */
> >> >> >> if (irq_mask & irq_status) {
> >> >> >> spin_unlock_irqrestore(&denali->irq_lock, flags);
> >> >> >> return irq_status;
> >> >> >> }
> >> >> >>
> >> >> >>
> >> >> >
> >> >> > My bad, I didn't notice you were releasing the lock after calling
> >> >> > reinit_completion(). I still find this solution more complex than my
> >> >> > proposal, but I don't care that much.
> >> >>
> >> >>
> >> >> At first, I implemented exactly like you suggested;
> >> >> denali->irq_mask = irq_mask;
> >> >> reinit_completion(&denali->complete)
> >> >> in denali_reset_irq().
> >> >>
> >> >>
> >> >> IIRC, things were like this.
> >> >>
> >> >> Some time later, you memtioned to use ->cmd_ctrl
> >> >> instead of ->cmdfunc.
> >> >>
> >> >> Then I had a problem when I needed to implement
> >> >> denali_check_irq() in
> >> >> http://patchwork.ozlabs.org/patch/772395/
> >> >>
> >> >> denali_wait_for_irq() is blocked until interested IRQ happens.
> >> >> but ->dev_ready() hook should not be blocked.
> >> >> It should return if R/B# transition has happened or not.
> >> >
> >> > Nope, it should return whether the NAND is ready or not, not whether a
> >> > busy -> ready transition occurred or not. It's typically done by
> >> > reading the NAND STATUS register or by checking the R/B pin status.
> >>
> >> Checking the R/B pin is probably impossible unless
> >> the pin is changed into a GPIO port.
> >>
> >> I also considered NAND_CMD_STATUS, but
> >> I can not recall why I chose the current approach.
> >> Perhaps I thought returning detected IRQ
> >> is faster than accessing the chip for NAND_CMD_STATUS.
> >>
> >> I can try NAND_CMD_STATUS approach if you like.
> >
> > Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
> > inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
> > perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
> > are expected to wait for chip readiness before returning.
> >
> > You could also implement ->waitfunc() using denali_wait_for_irq() if
> > you're able to detect R/B transitions,
>
> R/B transition will set INTR__INT_ACT interrupt.
>
> I think it is easy in my implementation of denali_wait_for_irq(),
> like
>
> denali_wait_for_irq(denali, INTR__INT_ACT);
>
>
>
> But, you are suggesting me to change it.
This is clearly not a hard requirement, I was just curious and wanted
to understand why you had such a convoluted interrupt handling design. I
think I now understand why (see below).
> In your way, you give IRQ masks to denali_reset_irq(), like
> denali_reset_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
>
> Then, we have no room of IRQ bit in denali_wait_for_irq().
>
> How will you implement it?
It should be pretty easy: just make sure you reset the INTR__INT_ACT
status flag before sending a command (->cmd_ctrl()), and then unmask the
INTR__INT_ACT in denali_waitfunc() just before calling
denali_wait_for_irqs(). This should guarantee that you don't loose any
events, while keeping the logic rather simple.
>
>
> > but I'm not sure it's worth it,
> > because you overload almost all the methods using this hook (the only
> > one remaining is ->onfi_set_features(), and using STATUS polling should
> > not be an issue in this case).
> >
> > Implementing ->dev_ready() is not necessary. When not provided, the
> > core falls back to STATUS polling and you seem to support
> > NAND_CMD_STATUS in denali_cmdfunc(). Note that even if it's not fully
> > reliable in the current driver, you're switching to ->cmd_ctrl() at the
> > end of the series anyway, so we should be good after that.
>
> ->dev_ready() is optional, but we may end up with waiting more than needed.
>
> case NAND_CMD_RESET:
> if (chip->dev_ready)
> break;
> udelay(chip->chip_delay);
>
>
> chip->chip_delay is probably set large enough, so this is not optimal.
That's true, this udelay should not be needed in your case.
>
>
> If I add something more, the following two bugs were found by
> denali_dev_ready().
>
> commit 3158fa0e739615769cc047d2428f30f4c3b6640e
> commit c5d664aa5a4c4b257a54eb35045031630d105f49
>
>
> If NAND core is fine, denali_dev_ready() works fine too.
>
> If not, it is a sign of bug of nand_command(_lp).
> This is contributing to the core improvement.
>
Had a second look at denali_dev_ready() and it seems to do the right
thing, so let's keep it like that.
>
> >>
> >> IIRC, I was thinking like this:
> >>
> >> One IRQ line may be shared among multiple hardware including Denali.
> >> denali_pci may do this.
> >>
> >> The Denali IRQ handler need to check irq status
> >> because it should return IRQ_HANDLED if the event comes from Denali controller.
> >> Otherwise, the event comes from different hardware, so
> >> Denali IRQ handler should return IRQ_NONE.
> >
> > Correct.
> >
> >>
> >> wait_for_completion_timeout() may bail out with timeout error,
> >> then proceed to denali_reset_irq() for the next operation.
> >
> > Before calling denali_reset_irq() you should re-mask the irqs you
> > unmasked in #1. Actually, calling denali_reset_irq() after
> > wait_for_completion_timeout() is not even needed here because you'll
> > clear pending irqs before launching the next NAND command.
> >
> >> Afterwards, the event actually may happen, and invoke IRQ handler.
> >
> > Not if you masked IRQs after wait_for_completion_timeout() returned.
>
>
> wait_for_completion_timeout(&denali->complete, msecs_to_jiffies(1000));
> <<< WHAT IF IRQ EVENT HAPPENS HERE ? >>>
> iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
You're right, the write to INTR_EN() should be protected by a
spin_lock_irqsave to prevent concurrency between the irq handler and
the thread executing this function (and we should also take the lock
from the irq handler when doing status & mask). I didn't consider the
SMP case when coding this approach (one CPU can handle the interrupt
while the other one continues executing this function after the
timeout).
>
>
>
>
> Also, you ignore the return value of wait_for_completion_timeout(),
> then drop my precious error message()
>
> dev_err(denali->dev, "timeout while waiting for irq 0x%x\n",
> denali->irq_mask)
Timeout can be detected by testing the status: if none of the flags we
were waiting for are set this is a timeout. Maybe I forgot to add this
message back though.
>
>
>
> > Here is a patch to show you what I had in mind [1] (it applies on top
> > of this patch). AFAICT, there's no races, no interrupt loss, and you
> > get rid of the ->irq_mask/status/lock fields.
> >
> > [1]http://code.bulix.org/fufia6-145571
> >
>
>
> Problem Scenario A
> [1] wait_for_completion_timeout() exits with timeout.
> [2] IRQ happens and denali_isr() is invoked
> [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
> [4] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
> ioread32(denali->flash_reg + INTR_EN(bank));
> (status is set to 0 because INTR_EN(bank) is now 0)
> [5] return IRQ_NONE;
> [6] kernel complains "irq *: nobody cared"
Okay, this is the part I initially misunderstood. Your goal is to never
ever return IRQ_NONE, while I was accepting to rarely return IRQ_NONE
in the unlikely interrupt-just-after-timeout case. Note that the kernel
irq infrastructure accepts rare occurrences or IRQ_NONE [1].
>
>
>
> Problem Scenario B (unlikely to happen, though)
> [1] wait_for_completion_timeout() exits with timeout.
> [2] IRQ happens and denali_isr() is invoked
> [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
> [4] chip->select_chip(mtd, -1)
> [5] denali->flash_bank = -1
> [6] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
> ioread32(denali->flash_reg + INTR_EN(bank));
> ( access to non-existing INTR_STATUS(-1) )
Wrapping the write INTR_EN() into a
spin_lock_irqsave/unlock_irqrestore() section and doing the same in the
interrupt handler (without irqsave/restore) should solve the problem.
This being said, I'm not asking you to change the code, I just wanted
to understand why you were doing it like that.
Thanks,
Boris
[1]http://elixir.free-electrons.com/linux/latest/source/kernel/irq/spurious.c#L407
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-13 06:50 +0200 |
| Subject | Re: [PATCH v5 10/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tRLW9-3nB-3@gated-at.bofh.it> |
| In reply to | #1662059 |
Hi Boris,
2017-06-09 16:58 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> Hi Masahiro,
>
> On Fri, 9 Jun 2017 02:26:34 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> Hi Boris
>>
>> 2017-06-09 0:43 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> > On Thu, 8 Jun 2017 21:58:00 +0900
>> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >
>> >> Hi Boris,
>> >>
>> >> 2017-06-08 20:26 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> > On Thu, 8 Jun 2017 19:41:39 +0900
>> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> >
>> >> >> Hi Boris,
>> >> >>
>> >> >>
>> >> >> 2017-06-08 16:12 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> >> > Le Thu, 8 Jun 2017 15:10:18 +0900,
>> >> >> > Masahiro Yamada <yamada.masahiro@socionext.com> a écrit :
>> >> >> >
>> >> >> >> Hi Boris,
>> >> >> >>
>> >> >> >>
>> >> >> >> 2017-06-07 22:57 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
>> >> >> >> > On Wed, 7 Jun 2017 20:52:19 +0900
>> >> >> >> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>> >> >> >> >
>> >> >> >> >
>> >> >> >> >> -/*
>> >> >> >> >> - * This is the interrupt service routine. It handles all interrupts
>> >> >> >> >> - * sent to this device. Note that on CE4100, this is a shared interrupt.
>> >> >> >> >> - */
>> >> >> >> >> -static irqreturn_t denali_isr(int irq, void *dev_id)
>> >> >> >> >> +static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
>> >> >> >> >> + uint32_t irq_mask)
>> >> >> >> >> {
>> >> >> >> >> - struct denali_nand_info *denali = dev_id;
>> >> >> >> >> + unsigned long time_left, flags;
>> >> >> >> >> uint32_t irq_status;
>> >> >> >> >> - irqreturn_t result = IRQ_NONE;
>> >> >> >> >>
>> >> >> >> >> - spin_lock(&denali->irq_lock);
>> >> >> >> >> + spin_lock_irqsave(&denali->irq_lock, flags);
>> >> >> >> >>
>> >> >> >> >> - /* check to see if a valid NAND chip has been selected. */
>> >> >> >> >> - if (is_flash_bank_valid(denali->flash_bank)) {
>> >> >> >> >> - /*
>> >> >> >> >> - * check to see if controller generated the interrupt,
>> >> >> >> >> - * since this is a shared interrupt
>> >> >> >> >> - */
>> >> >> >> >> - irq_status = denali_irq_detected(denali);
>> >> >> >> >> - if (irq_status != 0) {
>> >> >> >> >> - /* handle interrupt */
>> >> >> >> >> - /* first acknowledge it */
>> >> >> >> >> - clear_interrupt(denali, irq_status);
>> >> >> >> >> - /*
>> >> >> >> >> - * store the status in the device context for someone
>> >> >> >> >> - * to read
>> >> >> >> >> - */
>> >> >> >> >> - denali->irq_status |= irq_status;
>> >> >> >> >> - /* notify anyone who cares that it happened */
>> >> >> >> >> - complete(&denali->complete);
>> >> >> >> >> - /* tell the OS that we've handled this */
>> >> >> >> >> - result = IRQ_HANDLED;
>> >> >> >> >> - }
>> >> >> >> >> + irq_status = denali->irq_status;
>> >> >> >> >> +
>> >> >> >> >> + if (irq_mask & irq_status) {
>> >> >> >> >> + spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> >> >> >> + return irq_status;
>> >> >> >> >> }
>> >> >> >> >> - spin_unlock(&denali->irq_lock);
>> >> >> >> >> - return result;
>> >> >> >> >> +
>> >> >> >> >> + denali->irq_mask = irq_mask;
>> >> >> >> >> + reinit_completion(&denali->complete);
>> >> >> >> >
>> >> >> >> > These 2 instructions should be done before calling
>> >> >> >> > denali_wait_for_irq() (for example in denali_reset_irq()), otherwise
>> >> >> >> > you might loose events if they happen between your irq_status read and
>> >> >> >> > the reinit_completion() call.
>> >> >> >>
>> >> >> >> No.
>> >> >> >>
>> >> >> >> denali->irq_lock avoids a race between denali_isr() and
>> >> >> >> denali_wait_for_irq().
>> >> >> >>
>> >> >> >>
>> >> >> >> The line
>> >> >> >> denali->irq_status |= irq_status;
>> >> >> >> in denali_isr() accumulates all events that have happened
>> >> >> >> since denali_reset_irq().
>> >> >> >>
>> >> >> >> If the interested IRQs have already happened
>> >> >> >> before denali_wait_for_irq(), it just return immediately
>> >> >> >> without using completion.
>> >> >> >>
>> >> >> >> I do not mind adding a comment like below
>> >> >> >> if you think my intention is unclear, though.
>> >> >> >>
>> >> >> >> /* Return immediately if interested IRQs have already happend. */
>> >> >> >> if (irq_mask & irq_status) {
>> >> >> >> spin_unlock_irqrestore(&denali->irq_lock, flags);
>> >> >> >> return irq_status;
>> >> >> >> }
>> >> >> >>
>> >> >> >>
>> >> >> >
>> >> >> > My bad, I didn't notice you were releasing the lock after calling
>> >> >> > reinit_completion(). I still find this solution more complex than my
>> >> >> > proposal, but I don't care that much.
>> >> >>
>> >> >>
>> >> >> At first, I implemented exactly like you suggested;
>> >> >> denali->irq_mask = irq_mask;
>> >> >> reinit_completion(&denali->complete)
>> >> >> in denali_reset_irq().
>> >> >>
>> >> >>
>> >> >> IIRC, things were like this.
>> >> >>
>> >> >> Some time later, you memtioned to use ->cmd_ctrl
>> >> >> instead of ->cmdfunc.
>> >> >>
>> >> >> Then I had a problem when I needed to implement
>> >> >> denali_check_irq() in
>> >> >> http://patchwork.ozlabs.org/patch/772395/
>> >> >>
>> >> >> denali_wait_for_irq() is blocked until interested IRQ happens.
>> >> >> but ->dev_ready() hook should not be blocked.
>> >> >> It should return if R/B# transition has happened or not.
>> >> >
>> >> > Nope, it should return whether the NAND is ready or not, not whether a
>> >> > busy -> ready transition occurred or not. It's typically done by
>> >> > reading the NAND STATUS register or by checking the R/B pin status.
>> >>
>> >> Checking the R/B pin is probably impossible unless
>> >> the pin is changed into a GPIO port.
>> >>
>> >> I also considered NAND_CMD_STATUS, but
>> >> I can not recall why I chose the current approach.
>> >> Perhaps I thought returning detected IRQ
>> >> is faster than accessing the chip for NAND_CMD_STATUS.
>> >>
>> >> I can try NAND_CMD_STATUS approach if you like.
>> >
>> > Depends what you're trying to do. IIUC, you use denali_wait_for_irq()
>> > inside your ->reset()/->read/write_{page,oob}[_raw]() methods, which is
>> > perfectly fine (assuming CUSTOM_PAGE_ACCESS is set) since these hooks
>> > are expected to wait for chip readiness before returning.
>> >
>> > You could also implement ->waitfunc() using denali_wait_for_irq() if
>> > you're able to detect R/B transitions,
>>
>> R/B transition will set INTR__INT_ACT interrupt.
>>
>> I think it is easy in my implementation of denali_wait_for_irq(),
>> like
>>
>> denali_wait_for_irq(denali, INTR__INT_ACT);
>>
>>
>>
>> But, you are suggesting me to change it.
>
> This is clearly not a hard requirement, I was just curious and wanted
> to understand why you had such a convoluted interrupt handling design. I
> think I now understand why (see below).
>
>> In your way, you give IRQ masks to denali_reset_irq(), like
>> denali_reset_irq(denali, INTR__ERASE_COMP | INTR__ERASE_FAIL);
>>
>> Then, we have no room of IRQ bit in denali_wait_for_irq().
>>
>> How will you implement it?
>
> It should be pretty easy: just make sure you reset the INTR__INT_ACT
> status flag before sending a command (->cmd_ctrl()), and then unmask the
> INTR__INT_ACT in denali_waitfunc() just before calling
> denali_wait_for_irqs(). This should guarantee that you don't loose any
> events, while keeping the logic rather simple.
Right. This way will be possible.
One compromise I see is that
it sets INTR__INT_ACT (= wait for R/B# IRQ event) for all commands.
Some commands actually trigger R/B# transition, but some do not.
We can make it precise like nand_command_lp(),
but I do not want to write such a switch statement in my driver.
(this must be maintained for possible new command addition in the future)
Anyway, I will send v6 in my current approach.
>>
>>
>>
>> > Here is a patch to show you what I had in mind [1] (it applies on top
>> > of this patch). AFAICT, there's no races, no interrupt loss, and you
>> > get rid of the ->irq_mask/status/lock fields.
>> >
>> > [1]http://code.bulix.org/fufia6-145571
>> >
>>
>>
>> Problem Scenario A
>> [1] wait_for_completion_timeout() exits with timeout.
>> [2] IRQ happens and denali_isr() is invoked
>> [3] iowrite32(0, denali->flash_reg + INTR_EN(denali->flash_bank));
>> [4] status = ioread32(denali->flash_reg + INTR_STATUS(bank)) &
>> ioread32(denali->flash_reg + INTR_EN(bank));
>> (status is set to 0 because INTR_EN(bank) is now 0)
>> [5] return IRQ_NONE;
>> [6] kernel complains "irq *: nobody cared"
>
> Okay, this is the part I initially misunderstood. Your goal is to never
> ever return IRQ_NONE, while I was accepting to rarely return IRQ_NONE
> in the unlikely interrupt-just-after-timeout case. Note that the kernel
> irq infrastructure accepts rare occurrences or IRQ_NONE [1].
I wanted to be strict here.
But, I did not know the kernel is tolerant with rare IRQ_NONE.
Thanks for the pointer!
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:00 +0200 |
| Subject | [PATCH v5 04/23] mtd: nand: denali: remove Toshiba and Hynix specific fixup code |
| Message-ID | <tPHN1-68d-49@gated-at.bofh.it> |
| In reply to | #1659703 |
The Denali IP can automatically detect device parameters such as
page size, oob size, device width, etc. and this driver currently
relies on it. However, this hardware function is known to be
problematic.
[1] Due to a hardware bug, various misdetected cases were reported.
That is why get_toshiba_nand_para() and get_hynix_nand_para()
exist to fix-up the misdetected parameters. It is not realistic
to add a new NAND device to the *black list* every time we are
hit by a misdetected case. We would never be able to guarantee
that all cases are covered.
[2] Because this feature is unreliable, it is disabled on some
platforms.
The nand_scan_ident() detects device parameters in a more tested
way. The hardware should not set the device parameter registers in
a different, unreliable way. Instead, set the parameters from the
nand_scan_ident() back to the registers.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/mtd/nand/denali.c | 40 ++++++----------------------------------
1 file changed, 6 insertions(+), 34 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 0fff11faf603..991924b9ae2c 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -337,36 +337,6 @@ static void get_samsung_nand_para(struct denali_nand_info *denali,
}
}
-static void get_toshiba_nand_para(struct denali_nand_info *denali)
-{
- /*
- * Workaround to fix a controller bug which reports a wrong
- * spare area size for some kind of Toshiba NAND device
- */
- if ((ioread32(denali->flash_reg + DEVICE_MAIN_AREA_SIZE) == 4096) &&
- (ioread32(denali->flash_reg + DEVICE_SPARE_AREA_SIZE) == 64))
- iowrite32(216, denali->flash_reg + DEVICE_SPARE_AREA_SIZE);
-}
-
-static void get_hynix_nand_para(struct denali_nand_info *denali,
- uint8_t device_id)
-{
- switch (device_id) {
- case 0xD5: /* Hynix H27UAG8T2A, H27UBG8U5A or H27UCG8VFA */
- case 0xD7: /* Hynix H27UDG8VEM, H27UCG8UDM or H27UCG8V5A */
- iowrite32(128, denali->flash_reg + PAGES_PER_BLOCK);
- iowrite32(4096, denali->flash_reg + DEVICE_MAIN_AREA_SIZE);
- iowrite32(224, denali->flash_reg + DEVICE_SPARE_AREA_SIZE);
- iowrite32(0, denali->flash_reg + DEVICE_WIDTH);
- break;
- default:
- dev_warn(denali->dev,
- "Unknown Hynix NAND (Device ID: 0x%x).\n"
- "Will use default parameter values instead.\n",
- device_id);
- }
-}
-
/*
* determines how many NAND chips are connected to the controller. Note for
* Intel CE4100 devices we don't support more than one device.
@@ -453,10 +423,6 @@ static uint16_t denali_nand_timing_set(struct denali_nand_info *denali)
return FAIL;
} else if (maf_id == 0xEC) { /* Samsung NAND */
get_samsung_nand_para(denali, device_id);
- } else if (maf_id == 0x98) { /* Toshiba NAND */
- get_toshiba_nand_para(denali);
- } else if (maf_id == 0xAD) { /* Hynix NAND */
- get_hynix_nand_para(denali, device_id);
}
dev_info(denali->dev,
@@ -1622,6 +1588,12 @@ int denali_init(struct denali_nand_info *denali)
chip->ecc.size, chip->ecc.strength, chip->ecc.bytes);
iowrite32(chip->ecc.strength, denali->flash_reg + ECC_CORRECTION);
+ iowrite32(mtd->erasesize / mtd->writesize,
+ denali->flash_reg + PAGES_PER_BLOCK);
+ iowrite32(chip->options & NAND_BUSWIDTH_16 ? 1 : 0,
+ denali->flash_reg + DEVICE_WIDTH);
+ iowrite32(mtd->writesize, denali->flash_reg + DEVICE_MAIN_AREA_SIZE);
+ iowrite32(mtd->oobsize, denali->flash_reg + DEVICE_SPARE_AREA_SIZE);
iowrite32(chip->ecc.size, denali->flash_reg + CFG_DATA_BLOCK_SIZE);
iowrite32(chip->ecc.size, denali->flash_reg + CFG_LAST_DATA_BLOCK_SIZE);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:00 +0200 |
| Subject | [PATCH v5 13/23] mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc |
| Message-ID | <tPHN1-68d-51@gated-at.bofh.it> |
| In reply to | #1659703 |
The NAND_CMD_SET_FEATURES support is missing from denali_cmdfunc().
This is needed for nand_onfi_set_features().
Besides, we see /* TODO: Read OOB data */ comment line.
It would be possible to add more commands along with the current
implementation, but having ->cmd_ctrl() seems a better approach from
the discussion with Boris [1].
Rely on the default ->cmdfunc() from the framework and implement the
driver's own ->cmd_ctrl().
Also add ->write_byte(), which is needed for write direction commands.
[1] https://lkml.org/lkml/2017/3/15/97
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 104 +++++++++++++++++++++++-----------------------
1 file changed, 52 insertions(+), 52 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index c3382954cf27..95bce6b341d7 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -230,20 +230,16 @@ static uint32_t denali_wait_for_irq(struct denali_nand_info *denali,
return denali->irq_status;
}
-/* resets a specific device connected to the core */
-static void reset_bank(struct denali_nand_info *denali)
+static uint32_t denali_check_irq(struct denali_nand_info *denali)
{
+ unsigned long flags;
uint32_t irq_status;
- denali_reset_irq(denali);
-
- iowrite32(1 << denali->flash_bank, denali->flash_reg + DEVICE_RESET);
-
- irq_status = denali_wait_for_irq(denali,
- INTR__RST_COMP | INTR__TIME_OUT);
+ spin_lock_irqsave(&denali->irq_lock, flags);
+ irq_status = denali->irq_status;
+ spin_unlock_irqrestore(&denali->irq_lock, flags);
- if (!(irq_status & INTR__RST_COMP))
- dev_err(denali->dev, "reset bank failed.\n");
+ return irq_status;
}
/*
@@ -273,6 +269,42 @@ static uint8_t denali_read_byte(struct mtd_info *mtd)
return ioread32(denali->flash_mem + 0x10);
}
+static void denali_write_byte(struct mtd_info *mtd, uint8_t byte)
+{
+ struct denali_nand_info *denali = mtd_to_denali(mtd);
+
+ index_addr(denali, MODE_11 | BANK(denali->flash_bank) | 2, byte);
+}
+
+static void denali_cmd_ctrl(struct mtd_info *mtd, int dat, unsigned int ctrl)
+{
+ struct denali_nand_info *denali = mtd_to_denali(mtd);
+ uint32_t type;
+
+ if (ctrl & NAND_CLE)
+ type = 0;
+ else if (ctrl & NAND_ALE)
+ type = 1;
+ else
+ return;
+
+ /*
+ * Some commands are followed by chip->dev_ready or chip->waitfunc.
+ * irq_status must be cleared here to catch the R/B# interrupt later.
+ */
+ if (ctrl & NAND_CTRL_CHANGE)
+ denali_reset_irq(denali);
+
+ index_addr(denali, MODE_11 | BANK(denali->flash_bank) | type, dat);
+}
+
+static int denali_dev_ready(struct mtd_info *mtd)
+{
+ struct denali_nand_info *denali = mtd_to_denali(mtd);
+
+ return !!(denali_check_irq(denali) & INTR__INT_ACT);
+}
+
/*
* sends a pipeline command operation to the controller. See the Denali NAND
* controller's user guide for more information (section 4.2.3.6).
@@ -824,7 +856,13 @@ static void denali_select_chip(struct mtd_info *mtd, int chip)
static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip)
{
- return 0;
+ struct denali_nand_info *denali = mtd_to_denali(mtd);
+ uint32_t irq_status;
+
+ /* R/B# pin transitioned from low to high? */
+ irq_status = denali_wait_for_irq(denali, INTR__INT_ACT);
+
+ return irq_status & INTR__INT_ACT ? 0 : NAND_STATUS_FAIL;
}
static int denali_erase(struct mtd_info *mtd, int page)
@@ -845,46 +883,6 @@ static int denali_erase(struct mtd_info *mtd, int page)
return irq_status & INTR__ERASE_COMP ? 0 : NAND_STATUS_FAIL;
}
-static void denali_cmdfunc(struct mtd_info *mtd, unsigned int cmd, int col,
- int page)
-{
- struct denali_nand_info *denali = mtd_to_denali(mtd);
- uint32_t addr, irq_status;
- int wait_ready = 0;
-
- switch (cmd) {
- case NAND_CMD_PARAM:
- wait_ready = 1;
- break;
- case NAND_CMD_STATUS:
- case NAND_CMD_READID:
- break;
- case NAND_CMD_RESET:
- reset_bank(denali);
- break;
- case NAND_CMD_READOOB:
- /* TODO: Read OOB data */
- return;
- default:
- pr_err(": unsupported command received 0x%x\n", cmd);
- return;
- }
-
- denali_reset_irq(denali);
-
- addr = MODE_11 | BANK(denali->flash_bank);
- index_addr(denali, addr | 0, cmd);
- if (col != -1)
- index_addr(denali, addr | 1, col);
-
- if (!wait_ready)
- return;
-
- irq_status = denali_wait_for_irq(denali, INTR__INT_ACT);
- if (!(irq_status & INTR__INT_ACT))
- dev_err(denali->dev, "failed to issue command 0x%x\n", cmd);
-}
-
#define DIV_ROUND_DOWN_ULL(ll, d) \
({ unsigned long long _tmp = (ll); do_div(_tmp, d); _tmp; })
@@ -1224,8 +1222,10 @@ int denali_init(struct denali_nand_info *denali)
/* register the driver with the NAND core subsystem */
chip->select_chip = denali_select_chip;
- chip->cmdfunc = denali_cmdfunc;
chip->read_byte = denali_read_byte;
+ chip->write_byte = denali_write_byte;
+ chip->cmd_ctrl = denali_cmd_ctrl;
+ chip->dev_ready = denali_dev_ready;
chip->waitfunc = denali_waitfunc;
/* clk rate info is needed for setup_data_interface */
if (denali->clk_x_rate)
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:10 +0200 |
| Subject | [PATCH v5 02/23] mtd: nand: add a shorthand to generate nand_ecc_caps structure |
| Message-ID | <tPHWF-6rv-5@gated-at.bofh.it> |
| In reply to | #1659703 |
struct nand_ecc_caps was designed as flexible as possible to support
multiple stepsizes (like sunxi_nand.c).
So, we need to write multiple arrays even for the simplest case.
I guess many controllers support a single stepsize, so here is a
shorthand macro for the case.
It allows to describe like ...
NAND_ECC_CAPS_SINGLE(denali_pci_ecc_caps, denali_calc_ecc_bytes, 512, 8, 15);
... instead of
static const int denali_pci_ecc_strengths[] = {8, 15};
static const struct nand_ecc_step_info denali_pci_ecc_stepinfo = {
.stepsize = 512,
.strengths = denali_pci_ecc_strengths,
.nstrengths = ARRAY_SIZE(denali_pci_ecc_strengths),
};
static const struct nand_ecc_caps denali_pci_ecc_caps = {
.stepinfos = &denali_pci_ecc_stepinfo,
.nstepinfos = 1,
.calc_ecc_bytes = denali_calc_ecc_bytes,
};
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5:
- Newly added
Changes in v4: None
Changes in v3: None
Changes in v2: None
include/linux/mtd/nand.h | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/include/linux/mtd/nand.h b/include/linux/mtd/nand.h
index c28a5792eed6..c08954a39657 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -500,6 +500,20 @@ struct nand_ecc_caps {
int (*calc_ecc_bytes)(int step_size, int strength);
};
+/* a shorthand to generate struct nand_ecc_caps with only one ECC stepsize */
+#define NAND_ECC_CAPS_SINGLE(__name, __calc, __step, ...) \
+static const int __name##_strengths[] = { __VA_ARGS__ }; \
+static const struct nand_ecc_step_info __name##_stepinfo = { \
+ .stepsize = __step, \
+ .strengths = __name##_strengths, \
+ .nstrengths = ARRAY_SIZE(__name##_strengths), \
+}; \
+static const struct nand_ecc_caps __name = { \
+ .stepinfos = &__name##_stepinfo, \
+ .nstepinfos = 1, \
+ .calc_ecc_bytes = __calc, \
+}
+
/**
* struct nand_ecc_ctrl - Control structure for ECC
* @mode: ECC mode
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:10 +0200 |
| Subject | [PATCH v5 03/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tPHWG-6rv-15@gated-at.bofh.it> |
| In reply to | #1659703 |
This driver was originally written for the Intel MRST platform with
several platform-specific parameters hard-coded.
Currently, the ECC settings are hard-coded as follows:
#define ECC_SECTOR_SIZE 512
#define ECC_8BITS 14
#define ECC_15BITS 26
Therefore, the driver can only support two cases.
- ecc.size = 512, ecc.strength = 8 --> ecc.bytes = 14
- ecc.size = 512, ecc.strength = 15 --> ecc.bytes = 26
However, these are actually customizable parameters, for example,
UniPhier platform supports the following:
- ecc.size = 1024, ecc.strength = 8 --> ecc.bytes = 14
- ecc.size = 1024, ecc.strength = 16 --> ecc.bytes = 28
- ecc.size = 1024, ecc.strength = 24 --> ecc.bytes = 42
So, we need to handle the ECC parameters in a more generic manner.
Fortunately, the Denali User's Guide explains how to calculate the
ecc.bytes. The formula is:
ecc.bytes = 2 * CEIL(13 * ecc.strength / 16) (for ecc.size = 512)
ecc.bytes = 2 * CEIL(14 * ecc.strength / 16) (for ecc.size = 1024)
For DT platforms, it would be reasonable to allow DT to specify ECC
strength by either "nand-ecc-strength" or "nand-ecc-maximize". If
none of them is specified, the driver will try to meet the chip's ECC
requirement.
For PCI platforms, the max ECC strength is used to keep the original
behavior.
Newer versions of this IP need ecc.size and ecc.steps explicitly
set up via the following registers:
CFG_DATA_BLOCK_SIZE (0x6b0)
CFG_LAST_DATA_BLOCK_SIZE (0x6c0)
CFG_NUM_DATA_BLOCKS (0x6d0)
For older IP versions, write accesses to these registers are just
ignored.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
Acked-by: Rob Herring <robh@kernel.org>
---
Changes in v5:
- Simplify denali_calc_ecc_bytes()
- Adjust to the udpate of generic helpers
- export denali_calc_ecc_bytes()
Changes in v4:
- Rewrite by using generic helpers, nand_check_caps(),
nand_match_ecc_req(), nand_maximize_ecc().
Changes in v3:
- Move DENALI_CAP_ define out of struct denali_nand_info
- Use chip->ecc_step_ds as a hint to choose chip->ecc.size
where possible
Changes in v2:
- Change the capability prefix DENALI_CAPS_ -> DENALI_CAP_
- Make ECC 512 cap and ECC 1024 cap independent
- Set up three CFG_... registers
.../devicetree/bindings/mtd/denali-nand.txt | 7 ++
drivers/mtd/nand/denali.c | 87 +++++++++++++---------
drivers/mtd/nand/denali.h | 12 ++-
drivers/mtd/nand/denali_dt.c | 5 ++
drivers/mtd/nand/denali_pci.c | 4 +
5 files changed, 78 insertions(+), 37 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index e593bbeb2115..b7742a7363ea 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -7,6 +7,13 @@ Required properties:
- reg-names: Should contain the reg names "nand_data" and "denali_reg"
- interrupts : The interrupt number.
+Optional properties:
+ - nand-ecc-step-size: see nand.txt for details. If present, the value must be
+ 512 for "altr,socfpga-denali-nand"
+ - nand-ecc-strength: see nand.txt for details. Valid values are:
+ 8, 15 for "altr,socfpga-denali-nand"
+ - nand-ecc-maximize: see nand.txt for details
+
The device tree may optionally contain sub-nodes describing partitions of the
address space. See partition.txt for more detail.
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 16634df2e39a..0fff11faf603 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -886,8 +886,6 @@ static int denali_hw_ecc_fixup(struct mtd_info *mtd,
return max_bitflips;
}
-#define ECC_SECTOR_SIZE 512
-
#define ECC_SECTOR(x) (((x) & ECC_ERROR_ADDRESS__SECTOR_NR) >> 12)
#define ECC_BYTE(x) (((x) & ECC_ERROR_ADDRESS__OFFSET))
#define ECC_CORRECTION_VALUE(x) ((x) & ERR_CORRECTION_INFO__BYTEMASK)
@@ -899,6 +897,7 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
struct denali_nand_info *denali,
unsigned long *uncor_ecc_flags, uint8_t *buf)
{
+ unsigned int ecc_size = denali->nand.ecc.size;
unsigned int bitflips = 0;
unsigned int max_bitflips = 0;
uint32_t err_addr, err_cor_info;
@@ -928,9 +927,9 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
* an erased sector.
*/
*uncor_ecc_flags |= BIT(err_sector);
- } else if (err_byte < ECC_SECTOR_SIZE) {
+ } else if (err_byte < ecc_size) {
/*
- * If err_byte is larger than ECC_SECTOR_SIZE, means error
+ * If err_byte is larger than ecc_size, means error
* happened in OOB, so we ignore it. It's no need for
* us to correct it err_device is represented the NAND
* error bits are happened in if there are more than
@@ -939,7 +938,7 @@ static int denali_sw_ecc_fixup(struct mtd_info *mtd,
int offset;
unsigned int flips_in_byte;
- offset = (err_sector * ECC_SECTOR_SIZE + err_byte) *
+ offset = (err_sector * ecc_size + err_byte) *
denali->devnum + err_device;
/* correct the ECC error */
@@ -1345,13 +1344,39 @@ static void denali_hw_init(struct denali_nand_info *denali)
denali_irq_init(denali);
}
-/*
- * Althogh controller spec said SLC ECC is forceb to be 4bit,
- * but denali controller in MRST only support 15bit and 8bit ECC
- * correction
- */
-#define ECC_8BITS 14
-#define ECC_15BITS 26
+int denali_calc_ecc_bytes(int step_size, int strength)
+{
+ /* BCH code. Denali requires ecc.bytes to be multiple of 2 */
+ return DIV_ROUND_UP(strength * fls(step_size * 8), 16) * 2;
+}
+EXPORT_SYMBOL(denali_calc_ecc_bytes);
+
+static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
+ struct denali_nand_info *denali)
+{
+ int oobavail = mtd->oobsize - denali->bbtskipbytes;
+ int ret;
+
+ /*
+ * If .size and .strength are already set (usually by DT),
+ * check if they are supported by this controller.
+ */
+ if (chip->ecc.size && chip->ecc.strength)
+ return nand_check_ecc_caps(chip, denali->ecc_caps, oobavail);
+
+ /*
+ * We want .size and .strength closest to the chip's requirement
+ * unless NAND_ECC_MAXIMIZE is requested.
+ */
+ if (!(chip->ecc.options & NAND_ECC_MAXIMIZE)) {
+ ret = nand_match_ecc_req(chip, denali->ecc_caps, oobavail);
+ if (!ret)
+ return 0;
+ }
+
+ /* Max ECC strength is the last thing we can do */
+ return nand_maximize_ecc(chip, denali->ecc_caps, oobavail);
+}
static int denali_ooblayout_ecc(struct mtd_info *mtd, int section,
struct mtd_oob_region *oobregion)
@@ -1586,34 +1611,26 @@ int denali_init(struct denali_nand_info *denali)
/* no subpage writes on denali */
chip->options |= NAND_NO_SUBPAGE_WRITE;
- /*
- * Denali Controller only support 15bit and 8bit ECC in MRST,
- * so just let controller do 15bit ECC for MLC and 8bit ECC for
- * SLC if possible.
- * */
- if (!nand_is_slc(chip) &&
- (mtd->oobsize > (denali->bbtskipbytes +
- ECC_15BITS * (mtd->writesize /
- ECC_SECTOR_SIZE)))) {
- /* if MLC OOB size is large enough, use 15bit ECC*/
- chip->ecc.strength = 15;
- chip->ecc.bytes = ECC_15BITS;
- iowrite32(15, denali->flash_reg + ECC_CORRECTION);
- } else if (mtd->oobsize < (denali->bbtskipbytes +
- ECC_8BITS * (mtd->writesize /
- ECC_SECTOR_SIZE))) {
- pr_err("Your NAND chip OOB is not large enough to contain 8bit ECC correction codes");
+ ret = denali_ecc_setup(mtd, chip, denali);
+ if (ret) {
+ dev_err(denali->dev, "Failed to setup ECC settings.\n");
goto failed_req_irq;
- } else {
- chip->ecc.strength = 8;
- chip->ecc.bytes = ECC_8BITS;
- iowrite32(8, denali->flash_reg + ECC_CORRECTION);
}
+ dev_dbg(denali->dev,
+ "chosen ECC settings: step=%d, strength=%d, bytes=%d\n",
+ chip->ecc.size, chip->ecc.strength, chip->ecc.bytes);
+
+ iowrite32(chip->ecc.strength, denali->flash_reg + ECC_CORRECTION);
+
+ iowrite32(chip->ecc.size, denali->flash_reg + CFG_DATA_BLOCK_SIZE);
+ iowrite32(chip->ecc.size, denali->flash_reg + CFG_LAST_DATA_BLOCK_SIZE);
+ /* chip->ecc.steps is set by nand_scan_tail(); not available here */
+ iowrite32(mtd->writesize / chip->ecc.size,
+ denali->flash_reg + CFG_NUM_DATA_BLOCKS);
+
mtd_set_ooblayout(mtd, &denali_ooblayout_ops);
- /* override the default read operations */
- chip->ecc.size = ECC_SECTOR_SIZE;
chip->ecc.read_page = denali_read_page;
chip->ecc.read_page_raw = denali_read_page_raw;
chip->ecc.write_page = denali_write_page;
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index 37833535a7a3..a06ed741b550 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -259,6 +259,14 @@
#define ECC_COR_INFO__MAX_ERRORS GENMASK(6, 0)
#define ECC_COR_INFO__UNCOR_ERR BIT(7)
+#define CFG_DATA_BLOCK_SIZE 0x6b0
+
+#define CFG_LAST_DATA_BLOCK_SIZE 0x6c0
+
+#define CFG_NUM_DATA_BLOCKS 0x6d0
+
+#define CFG_META_DATA_SIZE 0x6e0
+
#define DMA_ENABLE 0x700
#define DMA_ENABLE__FLAG BIT(0)
@@ -301,8 +309,6 @@
#define MODE_10 0x08000000
#define MODE_11 0x0C000000
-#define ECC_SECTOR_SIZE 512
-
struct nand_buf {
int head;
int tail;
@@ -337,11 +343,13 @@ struct denali_nand_info {
int max_banks;
unsigned int revision;
unsigned int caps;
+ const struct nand_ecc_caps *ecc_caps;
};
#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
#define DENALI_CAP_DMA_64BIT BIT(1)
+int denali_calc_ecc_bytes(int step_size, int strength);
extern int denali_init(struct denali_nand_info *denali);
extern void denali_remove(struct denali_nand_info *denali);
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index b48430fe3cd4..bd1aa4cf4457 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -32,10 +32,14 @@ struct denali_dt {
struct denali_dt_data {
unsigned int revision;
unsigned int caps;
+ const struct nand_ecc_caps *ecc_caps;
};
+NAND_ECC_CAPS_SINGLE(denali_socfpga_ecc_caps, denali_calc_ecc_bytes,
+ 512, 8, 15);
static const struct denali_dt_data denali_socfpga_data = {
.caps = DENALI_CAP_HW_ECC_FIXUP,
+ .ecc_caps = &denali_socfpga_ecc_caps,
};
static const struct of_device_id denali_nand_dt_ids[] = {
@@ -64,6 +68,7 @@ static int denali_dt_probe(struct platform_device *pdev)
if (data) {
denali->revision = data->revision;
denali->caps = data->caps;
+ denali->ecc_caps = data->ecc_caps;
}
denali->platform = DT;
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index ac843238b77e..37dc0934c24c 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -27,6 +27,8 @@ static const struct pci_device_id denali_pci_ids[] = {
};
MODULE_DEVICE_TABLE(pci, denali_pci_ids);
+NAND_ECC_CAPS_SINGLE(denali_pci_ecc_caps, denali_calc_ecc_bytes, 512, 8, 15);
+
static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
int ret;
@@ -65,6 +67,8 @@ static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
pci_set_master(dev);
denali->dev = &dev->dev;
denali->irq = dev->irq;
+ denali->ecc_caps = &denali_pci_ecc_caps;
+ denali->nand.ecc.options |= NAND_ECC_MAXIMIZE;
ret = pci_request_regions(dev, DENALI_NAND_NAME);
if (ret) {
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 14:10 +0200 |
| Subject | [PATCH v5 07/23] mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() |
| Message-ID | <tPHWG-6rv-23@gated-at.bofh.it> |
| In reply to | #1659703 |
Currently, the error handling of denali_write_page(_raw) is a bit
complicated. If the program command fails, NAND_STATUS_FAIL is set
to the driver internal denali->status, then read out later by
denali_waitfunc().
We can avoid it by exploiting the nand_write_page() implementation.
If chip->ecc.write_page(_raw) returns negative code (i.e. -EIO), it
errors out immediately. This gives the same result as returning
NAND_STATUS_FAIL from chip->waitfunc. In either way, -EIO is
returned to the upper MTD layer.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v5: None
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 12 ++++--------
drivers/mtd/nand/denali.h | 1 -
2 files changed, 4 insertions(+), 9 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 1897fe238290..22acfc34b546 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -1005,6 +1005,7 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
size_t size = mtd->writesize + mtd->oobsize;
uint32_t irq_status;
uint32_t irq_mask = INTR__DMA_CMD_COMP | INTR__PROGRAM_FAIL;
+ int ret = 0;
denali->page = page;
@@ -1038,13 +1039,13 @@ static int write_page(struct mtd_info *mtd, struct nand_chip *chip,
if (irq_status == 0) {
dev_err(denali->dev, "timeout on write_page (type = %d)\n",
raw_xfer);
- denali->status = NAND_STATUS_FAIL;
+ ret = -EIO;
}
denali_enable_dma(denali, false);
dma_sync_single_for_cpu(denali->dev, addr, size, DMA_TO_DEVICE);
- return 0;
+ return ret;
}
/* NAND core entry points */
@@ -1196,12 +1197,7 @@ static void denali_select_chip(struct mtd_info *mtd, int chip)
static int denali_waitfunc(struct mtd_info *mtd, struct nand_chip *chip)
{
- struct denali_nand_info *denali = mtd_to_denali(mtd);
- int status = denali->status;
-
- denali->status = 0;
-
- return status;
+ return 0;
}
static int denali_erase(struct mtd_info *mtd, int page)
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index a06ed741b550..352d8328b94a 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -323,7 +323,6 @@ struct nand_buf {
struct denali_nand_info {
struct nand_chip nand;
int flash_bank; /* currently selected chip */
- int status;
int platform;
struct nand_buf buf;
struct device *dev;
--
2.7.4
[toc] | [prev] | [next] | [standalone]
Page 2 of 3 — ← Prev page 1 [2] 3 Next page →
Back to top | Article view | linux.kernel
csiph-web