Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1658233 > unrolled thread
| Started by | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| First post | 2017-06-06 01:30 +0200 |
| Last post | 2017-06-07 09:30 +0200 |
| Articles | 20 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:30 +0200
[PATCH v4 11/23] mtd: nand: denali: rework interrupt handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:30 +0200
[PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:30 +0200
Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 00:10 +0200
Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 05:10 +0200
Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 09:10 +0200
Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 09:30 +0200
Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 09:50 +0200
[PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:30 +0200
Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-06 23:50 +0200
Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 03:50 +0200
Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 08:20 +0200
[PATCH v4 05/23] mtd: nand: denali: remove Toshiba and Hynix specific fixup code Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:30 +0200
[PATCH v4 15/23] mtd: nand: denali: fix bank reset function to detect the number of chips Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:30 +0200
[PATCH v4 06/23] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:40 +0200
[PATCH v4 16/23] mtd: nand: denali: use interrupt instead of polling for bank reset Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:40 +0200
[PATCH v4 13/23] mtd: nand: denali: fix NAND_CMD_PARAM handling Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-06 01:40 +0200
Re: [PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 00:10 +0200
Re: [PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb Masahiro Yamada <yamada.masahiro@socionext.com> - 2017-06-07 03:30 +0200
Re: [PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb Boris Brezillon <boris.brezillon@free-electrons.com> - 2017-06-07 09:30 +0200
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:30 +0200 |
| Subject | [PATCH v4 00/23] mtd: nand: denali: Denali NAND IP patch bomb |
| Message-ID | <tP9BD-S3-7@gated-at.bofh.it> |
This patch series intends to solve various problems.
[1] The driver just retrieves the OOB area as-is
whereas the controller uses syndrome page layout.
[2] Many NAND chip specific parameters are hard-coded in the driver.
[3] ONFi devices are not working
[4] It can not read Bad Block Marker
Outstanding changes are:
- Fix raw/oob callbacks for syndrome page layout
- Implement setup_data_interface() callback
- Fix/implement more commands for ONFi devices
- Allow to skip the driver internal bounce buffer
- Support PIO in case DMA is not supported
- Switch from ->cmdfunc over to ->cmd_ctrl
18 patches were merged at v2.
11 patches were merged at v3.
Here is the rest of the series.
v1: https://lkml.org/lkml/2016/11/26/144
v2: https://lkml.org/lkml/2017/3/22/804
v3: https://lkml.org/lkml/2017/3/30/90
Masahiro Yamada (23):
mtd: nand: denali_dt: clean up resource ioremap
mtd: nand: denali: use BIT() and GENMASK() for register macros
mtd: nand: add generic helpers to check, match, maximize ECC settings
mtd: nand: denali: avoid hard-coding ECC step, strength, bytes
mtd: nand: denali: remove Toshiba and Hynix specific fixup code
mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants
mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS
mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc()
mtd: nand: denali: remove unneeded find_valid_banks()
mtd: nand: denali: handle timing parameters by setup_data_interface()
mtd: nand: denali: rework interrupt handling
mtd: nand: denali: fix NAND_CMD_STATUS handling
mtd: nand: denali: fix NAND_CMD_PARAM handling
mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc
mtd: nand: denali: fix bank reset function to detect the number of
chips
mtd: nand: denali: use interrupt instead of polling for bank reset
mtd: nand: denali: propagate page to helpers via function argument
mtd: nand: denali: merge struct nand_buf into struct denali_nand_info
mtd: nand: denali: use flag instead of register macro for direction
mtd: nand: denali: fix raw and oob accessors for syndrome page layout
mtd: nand: denali: skip driver internal bounce buffer when possible
mtd: nand: denali: use non-managed kmalloc() for DMA buffer
mtd: nand: denali: enable bad block table scan
.../devicetree/bindings/mtd/denali-nand.txt | 13 +
drivers/mtd/nand/denali.c | 1701 +++++++++-----------
drivers/mtd/nand/denali.h | 294 ++--
drivers/mtd/nand/denali_dt.c | 54 +-
drivers/mtd/nand/denali_pci.c | 15 +-
drivers/mtd/nand/nand_base.c | 219 +++
include/linux/mtd/nand.h | 35 +
7 files changed, 1235 insertions(+), 1096 deletions(-)
--
2.7.4
[toc] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:30 +0200 |
| Subject | [PATCH v4 11/23] mtd: nand: denali: rework interrupt handling |
| Message-ID | <tP9BE-S3-47@gated-at.bofh.it> |
| In reply to | #1658233 |
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 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 1bb57de..e8e6667 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);
}
static int denali_calc_ecc_bytes(int step_size, int strength)
@@ -1281,9 +1196,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)
@@ -1353,6 +1265,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);
@@ -1361,8 +1275,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)
@@ -1384,7 +1298,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);
@@ -1393,7 +1307,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,
@@ -1401,7 +1315,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,
@@ -1410,7 +1324,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;
}
/*
@@ -1434,7 +1348,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,
@@ -1468,21 +1382,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;
}
@@ -1500,7 +1414,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 f0f4d75..a46473a 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 | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:30 +0200 |
| Subject | [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tP9BF-S3-59@gated-at.bofh.it> |
| In reply to | #1658233 |
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 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 | 103 ++++++++++++++-------
drivers/mtd/nand/denali.h | 11 ++-
drivers/mtd/nand/denali_dt.c | 8 ++
drivers/mtd/nand/denali_pci.c | 9 ++
5 files changed, 101 insertions(+), 37 deletions(-)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index e593bbe..b7742a7 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 16634df..3204c51 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,55 @@ 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
+static int denali_calc_ecc_bytes(int step_size, int strength)
+{
+ int coef;
+
+ switch (step_size) {
+ case 512:
+ coef = 13;
+ break;
+ case 1024:
+ coef = 14;
+ break;
+ default:
+ return -ENOTSUPP;
+ }
+
+ return DIV_ROUND_UP(strength * coef, 16) * 2;
+}
+
+static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
+ struct denali_nand_info *denali)
+{
+ struct nand_ecc_caps caps;
+ int ret;
+
+ caps.stepinfos = denali->stepinfo;
+ caps.nstepinfos = 1;
+ caps.calc_ecc_bytes = denali_calc_ecc_bytes;
+ caps.oob_reserve_bytes = denali->bbtskipbytes;
+
+ /*
+ * 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(mtd, chip, &caps);
+
+ /*
+ * 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(mtd, chip, &caps);
+ if (!ret)
+ return 0;
+ }
+
+ /* Max ECC strength is the last thing we can do */
+ return nand_maximize_ecc(mtd, chip, &caps);
+}
static int denali_ooblayout_ecc(struct mtd_info *mtd, int section,
struct mtd_oob_region *oobregion)
@@ -1586,34 +1627,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 3783353..5f08691 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,6 +343,7 @@ struct denali_nand_info {
int max_banks;
unsigned int revision;
unsigned int caps;
+ const struct nand_ecc_step_info *stepinfo;
};
#define DENALI_CAP_HW_ECC_FIXUP BIT(0)
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index b48430f..8c09bbe 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -32,10 +32,17 @@ struct denali_dt {
struct denali_dt_data {
unsigned int revision;
unsigned int caps;
+ struct nand_ecc_step_info stepinfo;
};
+static const int denali_socfpga_strengths[] = {8, 15};
static const struct denali_dt_data denali_socfpga_data = {
.caps = DENALI_CAP_HW_ECC_FIXUP,
+ .stepinfo = {
+ .stepsize = 512,
+ .strengths = denali_socfpga_strengths,
+ .nstrengths = ARRAY_SIZE(denali_socfpga_strengths),
+ },
};
static const struct of_device_id denali_nand_dt_ids[] = {
@@ -64,6 +71,7 @@ static int denali_dt_probe(struct platform_device *pdev)
if (data) {
denali->revision = data->revision;
denali->caps = data->caps;
+ denali->stepinfo = &data->stepinfo;
}
denali->platform = DT;
diff --git a/drivers/mtd/nand/denali_pci.c b/drivers/mtd/nand/denali_pci.c
index ac84323..e0d50b6 100644
--- a/drivers/mtd/nand/denali_pci.c
+++ b/drivers/mtd/nand/denali_pci.c
@@ -27,6 +27,13 @@ static const struct pci_device_id denali_pci_ids[] = {
};
MODULE_DEVICE_TABLE(pci, denali_pci_ids);
+static const int denali_pci_strengths[] = {8, 15};
+static const struct nand_ecc_step_info denali_pci_stepinfo = {
+ .stepsize = 512,
+ .strengths = denali_pci_strengths,
+ .nstrengths = ARRAY_SIZE(denali_pci_strengths),
+};
+
static int denali_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)
{
int ret;
@@ -65,6 +72,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->stepinfo = &denali_pci_stepinfo;
+ 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 | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 00:10 +0200 |
| Subject | Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tPuPM-6gH-23@gated-at.bofh.it> |
| In reply to | #1658235 |
On Tue, 6 Jun 2017 08:21:43 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 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 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 | 103 ++++++++++++++-------
> drivers/mtd/nand/denali.h | 11 ++-
> drivers/mtd/nand/denali_dt.c | 8 ++
> drivers/mtd/nand/denali_pci.c | 9 ++
> 5 files changed, 101 insertions(+), 37 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
> index e593bbe..b7742a7 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 16634df..3204c51 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,55 @@ 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
> +static int denali_calc_ecc_bytes(int step_size, int strength)
> +{
> + int coef;
> +
> + switch (step_size) {
> + case 512:
> + coef = 13;
> + break;
> + case 1024:
> + coef = 14;
> + break;
> + default:
> + return -ENOTSUPP;
> + }
> +
> + return DIV_ROUND_UP(strength * coef, 16) * 2;
or just
return DIV_ROUND_UP(strength * fls(8 * step_size), 16) * 2;
the array of supported step size/strength should guarantee that you're
called with unsupported settings.
> +}
> +
> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
> + struct denali_nand_info *denali)
> +{
> + struct nand_ecc_caps caps;
> + int ret;
> +
> + caps.stepinfos = denali->stepinfo;
> + caps.nstepinfos = 1;
> + caps.calc_ecc_bytes = denali_calc_ecc_bytes;
> + caps.oob_reserve_bytes = denali->bbtskipbytes;
If you get rid of this oob_reserve_bytes field, you can define caps as
a static const and even directly store ecc_caps in denali_nand_info.
> +
> + /*
> + * 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(mtd, chip, &caps);
> +
> + /*
> + * 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(mtd, chip, &caps);
> + if (!ret)
> + return 0;
> + }
> +
> + /* Max ECC strength is the last thing we can do */
> + return nand_maximize_ecc(mtd, chip, &caps);
> +}
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 05:10 +0200 |
| Subject | Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tPzw6-TK-5@gated-at.bofh.it> |
| In reply to | #1659253 |
Hi Boris,
2017-06-07 7:01 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Tue, 6 Jun 2017 08:21:43 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> 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 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 | 103 ++++++++++++++-------
>> drivers/mtd/nand/denali.h | 11 ++-
>> drivers/mtd/nand/denali_dt.c | 8 ++
>> drivers/mtd/nand/denali_pci.c | 9 ++
>> 5 files changed, 101 insertions(+), 37 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
>> index e593bbe..b7742a7 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 16634df..3204c51 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,55 @@ 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
>> +static int denali_calc_ecc_bytes(int step_size, int strength)
>> +{
>> + int coef;
>> +
>> + switch (step_size) {
>> + case 512:
>> + coef = 13;
>> + break;
>> + case 1024:
>> + coef = 14;
>> + break;
>> + default:
>> + return -ENOTSUPP;
>> + }
>> +
>> + return DIV_ROUND_UP(strength * coef, 16) * 2;
>
> or just
>
> return DIV_ROUND_UP(strength * fls(8 * step_size), 16) * 2;
Good idea.
I heard the Denali ECC engine uses BCH code.
I am not familiar with the algorithm,
but probably this generalized formula is correct.
>> +}
>> +
>> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
>> + struct denali_nand_info *denali)
>> +{
>> + struct nand_ecc_caps caps;
>> + int ret;
>> +
>> + caps.stepinfos = denali->stepinfo;
>> + caps.nstepinfos = 1;
>> + caps.calc_ecc_bytes = denali_calc_ecc_bytes;
>> + caps.oob_reserve_bytes = denali->bbtskipbytes;
>
> If you get rid of this oob_reserve_bytes field, you can define caps as
> a static const and even directly store ecc_caps in denali_nand_info.
To make caps static const, denali_calc_ecc_bytes must be exported
to be referenced from denali_dt/denali_pci.
I am reluctant to do it.
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 09:10 +0200 |
| Subject | Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tPDgl-3om-5@gated-at.bofh.it> |
| In reply to | #1659347 |
On Wed, 7 Jun 2017 12:09:31 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >> +
> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
> >> + struct denali_nand_info *denali)
> >> +{
> >> + struct nand_ecc_caps caps;
> >> + int ret;
> >> +
> >> + caps.stepinfos = denali->stepinfo;
> >> + caps.nstepinfos = 1;
> >> + caps.calc_ecc_bytes = denali_calc_ecc_bytes;
> >> + caps.oob_reserve_bytes = denali->bbtskipbytes;
> >
> > If you get rid of this oob_reserve_bytes field, you can define caps as
> > a static const and even directly store ecc_caps in denali_nand_info.
>
> To make caps static const, denali_calc_ecc_bytes must be exported
> to be referenced from denali_dt/denali_pci.
> I am reluctant to do it.
You already duplicate other information in denali_dt.c and
denali_pci.c, so what prevents you from duplicating this one-line
function?
Also, denali core already exports 2 functions, I don't see the problem
in exporting the common nand_ecc_caps object. Why are you reluctant to
that?
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 09:30 +0200 |
| Subject | Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tPDzH-3vN-5@gated-at.bofh.it> |
| In reply to | #1659427 |
Hi Boris,
2017-06-07 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Wed, 7 Jun 2017 12:09:31 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> >> +
>> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
>> >> + struct denali_nand_info *denali)
>> >> +{
>> >> + struct nand_ecc_caps caps;
>> >> + int ret;
>> >> +
>> >> + caps.stepinfos = denali->stepinfo;
>> >> + caps.nstepinfos = 1;
>> >> + caps.calc_ecc_bytes = denali_calc_ecc_bytes;
>> >> + caps.oob_reserve_bytes = denali->bbtskipbytes;
>> >
>> > If you get rid of this oob_reserve_bytes field, you can define caps as
>> > a static const and even directly store ecc_caps in denali_nand_info.
>>
>> To make caps static const, denali_calc_ecc_bytes must be exported
>> to be referenced from denali_dt/denali_pci.
>> I am reluctant to do it.
>
> You already duplicate other information in denali_dt.c and
> denali_pci.c,
The ECC step-size and strength are tightly associated to each IP variant.
I see duplication between denali_dt and denali_pci, but it is just because
Intel and Altera happened to have the same parameters.
On the other hand, denali_calc_ecc_bytes() is common to all variants
because ECC algorithm is not customizable.
> so what prevents you from duplicating this one-line
> function?
>
> Also, denali core already exports 2 functions,
They are entries for probe/remove.
> I don't see the problem
> in exporting the common nand_ecc_caps object. Why are you reluctant to
> that?
denali_calc_ecc_bytes() is independent of DT, PCI, or whatever.
I see less reason to expose it.
caps is only used on probing, so I used a local variable.
I do not think it is a big problem.
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 09:50 +0200 |
| Subject | Re: [PATCH v4 04/23] mtd: nand: denali: avoid hard-coding ECC step, strength, bytes |
| Message-ID | <tPDT3-3Fa-1@gated-at.bofh.it> |
| In reply to | #1659470 |
On Wed, 7 Jun 2017 16:21:15 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> Hi Boris,
>
>
> 2017-06-07 16:02 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Wed, 7 Jun 2017 12:09:31 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> >> +
> >> >> +static int denali_ecc_setup(struct mtd_info *mtd, struct nand_chip *chip,
> >> >> + struct denali_nand_info *denali)
> >> >> +{
> >> >> + struct nand_ecc_caps caps;
> >> >> + int ret;
> >> >> +
> >> >> + caps.stepinfos = denali->stepinfo;
> >> >> + caps.nstepinfos = 1;
> >> >> + caps.calc_ecc_bytes = denali_calc_ecc_bytes;
> >> >> + caps.oob_reserve_bytes = denali->bbtskipbytes;
> >> >
> >> > If you get rid of this oob_reserve_bytes field, you can define caps as
> >> > a static const and even directly store ecc_caps in denali_nand_info.
> >>
> >> To make caps static const, denali_calc_ecc_bytes must be exported
> >> to be referenced from denali_dt/denali_pci.
> >> I am reluctant to do it.
> >
> > You already duplicate other information in denali_dt.c and
> > denali_pci.c,
>
> The ECC step-size and strength are tightly associated to each IP variant.
> I see duplication between denali_dt and denali_pci, but it is just because
> Intel and Altera happened to have the same parameters.
It's still duplication.
>
> On the other hand, denali_calc_ecc_bytes() is common to all variants
> because ECC algorithm is not customizable.
Yes, I agree.
>
>
> > so what prevents you from duplicating this one-line
> > function?
> >
> > Also, denali core already exports 2 functions,
>
> They are entries for probe/remove.
>
> > I don't see the problem
> > in exporting the common nand_ecc_caps object. Why are you reluctant to
> > that?
>
> denali_calc_ecc_bytes() is independent of DT, PCI, or whatever.
> I see less reason to expose it.
I don't get that one. The fact that it's a generic implementation makes
it a good match for something you want to have in the core and expose
to DT/PCI implems.
>
> caps is only used on probing, so I used a local variable.
> I do not think it is a big problem.
>
It is to me, because you'll be the only user of the API at first, and
people tend to copy&paste code from other drivers.
nand_ecc_caps is really something that should be const and attached to
a specific IP revision.
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:30 +0200 |
| Subject | [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings |
| Message-ID | <tP9BE-S3-51@gated-at.bofh.it> |
| In reply to | #1658233 |
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 since the previous version:
- Step size info holds an array of associated strengths
- nand_match_ecc_req() does not take care of the case
where ecc_size/strength is already set
- Reflect more comments from Boris
Previous version:
http://patchwork.ozlabs.org/patch/752107/
Changes in v4: None
Changes in v3: None
Changes in v2: None
drivers/mtd/nand/nand_base.c | 219 +++++++++++++++++++++++++++++++++++++++++++
include/linux/mtd/nand.h | 35 +++++++
2 files changed, 254 insertions(+)
diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
index bdfa903..f2da4f2 100644
--- a/drivers/mtd/nand/nand_base.c
+++ b/drivers/mtd/nand/nand_base.c
@@ -4509,6 +4509,225 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd)
}
}
+/**
+ * nand_check_ecc_caps - check the sanity of preset ECC settings
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @caps: ECC caps info structure
+ *
+ * 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 mtd_info *mtd, struct nand_chip *chip,
+ const struct nand_ecc_caps *caps)
+{
+ const struct nand_ecc_step_info *stepinfo;
+ int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
+ int preset_step = chip->ecc.size;
+ int preset_strength = chip->ecc.strength;
+ int ecc_bytes;
+ int i, j;
+
+ if (WARN_ON(avail_oobsize < 0))
+ return -EINVAL;
+
+ if (!preset_step || !preset_strength)
+ return -ENODATA;
+
+ 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)
+ goto found;
+ }
+ }
+
+ pr_err("ECC (step, strength) = (%d, %d) not supported on this controller",
+ preset_step, preset_strength);
+
+ return -ENOTSUPP;
+
+found:
+ ecc_bytes = caps->calc_ecc_bytes(preset_step, preset_strength);
+ if (WARN_ON_ONCE(ecc_bytes < 0))
+ return ecc_bytes;
+
+ if (ecc_bytes * mtd->writesize / preset_step > avail_oobsize) {
+ 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;
+}
+EXPORT_SYMBOL_GPL(nand_check_ecc_caps);
+
+/**
+ * nand_match_ecc_req - meet the chip's requirement with least ECC bytes
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @caps: ECC engine caps info structure
+ *
+ * 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 mtd_info *mtd, struct nand_chip *chip,
+ const struct nand_ecc_caps *caps)
+{
+ const struct nand_ecc_step_info *stepinfo;
+ int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
+ int req_step = chip->ecc_step_ds;
+ int req_strength = chip->ecc_strength_ds;
+ int req_corr, step_size, strength, steps, 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(avail_oobsize < 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;
+
+ steps = 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 * steps;
+
+ if (ecc_bytes_total > avail_oobsize ||
+ strength * steps < 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
+ * @mtd: mtd info structure
+ * @chip: nand chip info structure
+ * @caps: ECC engine caps info structure
+ *
+ * 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 mtd_info *mtd, struct nand_chip *chip,
+ const struct nand_ecc_caps *caps)
+{
+ const struct nand_ecc_step_info *stepinfo;
+ int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
+ int step_size, strength, steps, ecc_bytes, corr;
+ int best_corr = 0;
+ int best_step = 0;
+ int best_strength, best_ecc_bytes;
+ int i, j;
+
+ if (WARN_ON(avail_oobsize < 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;
+
+ steps = mtd->writesize / step_size;
+
+ ecc_bytes = caps->calc_ecc_bytes(step_size, strength);
+ if (WARN_ON_ONCE(ecc_bytes < 0))
+ continue;
+
+ if (ecc_bytes * steps > avail_oobsize)
+ continue;
+
+ corr = strength * steps;
+
+ /*
+ * 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 8f67b15..97ccb76 100644
--- a/include/linux/mtd/nand.h
+++ b/include/linux/mtd/nand.h
@@ -477,6 +477,32 @@ 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
+ * @oob_reserve_bytes: number of bytes in OOB that must be reserved
+ */
+struct nand_ecc_caps {
+ const struct nand_ecc_step_info *stepinfos;
+ int nstepinfos;
+ int (*calc_ecc_bytes)(int step_size, int strength);
+ int oob_reserve_bytes;
+};
+
+/**
* struct nand_ecc_ctrl - Control structure for ECC
* @mode: ECC mode
* @algo: ECC algorithm
@@ -1244,6 +1270,15 @@ int nand_check_erased_ecc_chunk(void *data, int datalen,
void *extraoob, int extraooblen,
int threshold);
+int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip,
+ const struct nand_ecc_caps *caps);
+
+int nand_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip,
+ const struct nand_ecc_caps *caps);
+
+int nand_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip,
+ const struct nand_ecc_caps *caps);
+
/* 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 | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-06 23:50 +0200 |
| Subject | Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings |
| Message-ID | <tPuwp-5UH-11@gated-at.bofh.it> |
| In reply to | #1658236 |
On Tue, 6 Jun 2017 08:21:42 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 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 since the previous version:
>
> - Step size info holds an array of associated strengths
> - nand_match_ecc_req() does not take care of the case
> where ecc_size/strength is already set
> - Reflect more comments from Boris
>
> Previous version:
> http://patchwork.ozlabs.org/patch/752107/
>
>
> Changes in v4: None
> Changes in v3: None
> Changes in v2: None
>
> drivers/mtd/nand/nand_base.c | 219 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/mtd/nand.h | 35 +++++++
> 2 files changed, 254 insertions(+)
>
> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> index bdfa903..f2da4f2 100644
> --- a/drivers/mtd/nand/nand_base.c
> +++ b/drivers/mtd/nand/nand_base.c
> @@ -4509,6 +4509,225 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd)
> }
> }
>
> +/**
> + * nand_check_ecc_caps - check the sanity of preset ECC settings
> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @caps: ECC caps info structure
> + *
> + * 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 mtd_info *mtd, struct nand_chip *chip,
> + const struct nand_ecc_caps *caps)
> +{
> + const struct nand_ecc_step_info *stepinfo;
> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
> + int preset_step = chip->ecc.size;
> + int preset_strength = chip->ecc.strength;
> + int ecc_bytes;
> + int i, j;
> +
> + if (WARN_ON(avail_oobsize < 0))
> + return -EINVAL;
> +
> + if (!preset_step || !preset_strength)
> + return -ENODATA;
> +
> + 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)
> + goto found;
> + }
> + }
> +
> + pr_err("ECC (step, strength) = (%d, %d) not supported on this controller",
> + preset_step, preset_strength);
> +
> + return -ENOTSUPP;
> +
> +found:
I prefer something like:
if (i == caps->nstepinfos) {
pr_err(...);
return -ENOTSUPP;
}
...
instead of this 'found' label.
> + ecc_bytes = caps->calc_ecc_bytes(preset_step, preset_strength);
> + if (WARN_ON_ONCE(ecc_bytes < 0))
> + return ecc_bytes;
> +
> + if (ecc_bytes * mtd->writesize / preset_step > avail_oobsize) {
> + 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;
> +}
> +EXPORT_SYMBOL_GPL(nand_check_ecc_caps);
> +
> +/**
> + * nand_match_ecc_req - meet the chip's requirement with least ECC bytes
> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @caps: ECC engine caps info structure
> + *
> + * 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 mtd_info *mtd, struct nand_chip *chip,
> + const struct nand_ecc_caps *caps)
> +{
> + const struct nand_ecc_step_info *stepinfo;
> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
> + int req_step = chip->ecc_step_ds;
> + int req_strength = chip->ecc_strength_ds;
> + int req_corr, step_size, strength, steps, ecc_bytes, ecc_bytes_total;
> + int best_step, best_strength, best_ecc_bytes;
> + int best_ecc_bytes_total = INT_MAX;
Just nitpicking, but why not -1 instead of INT_MAX?
> + int i, j;
> +
> + if (WARN_ON(avail_oobsize < 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;
> +
> + steps = 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 * steps;
> +
> + if (ecc_bytes_total > avail_oobsize ||
> + strength * steps < 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
> + * @mtd: mtd info structure
> + * @chip: nand chip info structure
> + * @caps: ECC engine caps info structure
> + *
> + * 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 mtd_info *mtd, struct nand_chip *chip,
> + const struct nand_ecc_caps *caps)
> +{
> + const struct nand_ecc_step_info *stepinfo;
> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
> + int step_size, strength, steps, ecc_bytes, corr;
> + int best_corr = 0;
> + int best_step = 0;
> + int best_strength, best_ecc_bytes;
> + int i, j;
> +
> + if (WARN_ON(avail_oobsize < 0))
> + return -EINVAL;
> +
> + for (i = 0; i < caps->nstepinfos; i++) {
> + stepinfo = &caps->stepinfos[i];
> + step_size = stepinfo->stepsize;
> +
> +
Extra blank line here.
> + /* 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;
> +
> + steps = mtd->writesize / step_size;
> +
> + ecc_bytes = caps->calc_ecc_bytes(step_size, strength);
> + if (WARN_ON_ONCE(ecc_bytes < 0))
> + continue;
> +
> + if (ecc_bytes * steps > avail_oobsize)
> + continue;
> +
> + corr = strength * steps;
> +
> + /*
> + * 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 8f67b15..97ccb76 100644
> --- a/include/linux/mtd/nand.h
> +++ b/include/linux/mtd/nand.h
> @@ -477,6 +477,32 @@ 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
> + * @oob_reserve_bytes: number of bytes in OOB that must be reserved
> + */
> +struct nand_ecc_caps {
> + const struct nand_ecc_step_info *stepinfos;
> + int nstepinfos;
> + int (*calc_ecc_bytes)(int step_size, int strength);
> + int oob_reserve_bytes;
Why is this needed? I thought we agreed on passing oobavail as an
argument to these helper funcs. If a driver needs to reserve a few OOB
bytes, then doing mtd->oobsize - rsvd_bytes is not such a big deal.
> +};
> +
> +/**
> * struct nand_ecc_ctrl - Control structure for ECC
> * @mode: ECC mode
> * @algo: ECC algorithm
> @@ -1244,6 +1270,15 @@ int nand_check_erased_ecc_chunk(void *data, int datalen,
> void *extraoob, int extraooblen,
> int threshold);
>
> +int nand_check_ecc_caps(struct mtd_info *mtd, struct nand_chip *chip,
> + const struct nand_ecc_caps *caps);
> +
> +int nand_match_ecc_req(struct mtd_info *mtd, struct nand_chip *chip,
> + const struct nand_ecc_caps *caps);
> +
> +int nand_maximize_ecc(struct mtd_info *mtd, struct nand_chip *chip,
> + const struct nand_ecc_caps *caps);
> +
> /* Default write_oob implementation */
> int nand_write_oob_std(struct mtd_info *mtd, struct nand_chip *chip, int page);
>
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 03:50 +0200 |
| Subject | Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings |
| Message-ID | <tPygF-8kO-1@gated-at.bofh.it> |
| In reply to | #1659244 |
2017-06-07 6:47 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> On Tue, 6 Jun 2017 08:21:42 +0900
> Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
>
>> 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 since the previous version:
>>
>> - Step size info holds an array of associated strengths
>> - nand_match_ecc_req() does not take care of the case
>> where ecc_size/strength is already set
>> - Reflect more comments from Boris
>>
>> Previous version:
>> http://patchwork.ozlabs.org/patch/752107/
>>
>>
>> Changes in v4: None
>> Changes in v3: None
>> Changes in v2: None
>>
>> drivers/mtd/nand/nand_base.c | 219 +++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mtd/nand.h | 35 +++++++
>> 2 files changed, 254 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
>> index bdfa903..f2da4f2 100644
>> --- a/drivers/mtd/nand/nand_base.c
>> +++ b/drivers/mtd/nand/nand_base.c
>> @@ -4509,6 +4509,225 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd)
>> }
>> }
>>
>> +/**
>> + * nand_check_ecc_caps - check the sanity of preset ECC settings
>> + * @mtd: mtd info structure
>> + * @chip: nand chip info structure
>> + * @caps: ECC caps info structure
>> + *
>> + * 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 mtd_info *mtd, struct nand_chip *chip,
>> + const struct nand_ecc_caps *caps)
>> +{
>> + const struct nand_ecc_step_info *stepinfo;
>> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
>> + int preset_step = chip->ecc.size;
>> + int preset_strength = chip->ecc.strength;
>> + int ecc_bytes;
>> + int i, j;
>> +
>> + if (WARN_ON(avail_oobsize < 0))
>> + return -EINVAL;
>> +
>> + if (!preset_step || !preset_strength)
>> + return -ENODATA;
>> +
>> + 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)
>> + goto found;
>> + }
>> + }
>> +
>> + pr_err("ECC (step, strength) = (%d, %d) not supported on this controller",
>> + preset_step, preset_strength);
>> +
>> + return -ENOTSUPP;
>> +
>> +found:
>
> I prefer something like:
>
> if (i == caps->nstepinfos) {
> pr_err(...);
> return -ENOTSUPP;
> }
>
> ...
>
> instead of this 'found' label.
I want to bail-out if (step, strength) matches.
In this version, the for-loop is double-nested by "step" and "strength".
In C language, it is not possible to bail-out from multi-nested loop
with a single "break;" statement. That is why I used "found:" label to do it.
In my first version where there was a single for-loop,
I did not use the goto label.
http://patchwork.ozlabs.org/patch/752107/
Do you have any suggestion for cleaner implementation?
>> + ecc_bytes = caps->calc_ecc_bytes(preset_step, preset_strength);
>> + if (WARN_ON_ONCE(ecc_bytes < 0))
>> + return ecc_bytes;
>> +
>> + if (ecc_bytes * mtd->writesize / preset_step > avail_oobsize) {
>> + 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;
>> +}
>> +EXPORT_SYMBOL_GPL(nand_check_ecc_caps);
>> +
>> +/**
>> + * nand_match_ecc_req - meet the chip's requirement with least ECC bytes
>> + * @mtd: mtd info structure
>> + * @chip: nand chip info structure
>> + * @caps: ECC engine caps info structure
>> + *
>> + * 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 mtd_info *mtd, struct nand_chip *chip,
>> + const struct nand_ecc_caps *caps)
>> +{
>> + const struct nand_ecc_step_info *stepinfo;
>> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
>> + int req_step = chip->ecc_step_ds;
>> + int req_strength = chip->ecc_strength_ds;
>> + int req_corr, step_size, strength, steps, ecc_bytes, ecc_bytes_total;
>> + int best_step, best_strength, best_ecc_bytes;
>> + int best_ecc_bytes_total = INT_MAX;
>
> Just nitpicking, but why not -1 instead of INT_MAX?
Because nand_match_ecc_req() prefers a smaller ecc_bytes_total.
So I chose the largest int number as an init value.
If we started from -1, the following if-conditional would have no effect.
/*
* 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;
}
>> + int i, j;
>> +
>> + if (WARN_ON(avail_oobsize < 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;
>> +
>> + steps = 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 * steps;
>> +
>> + if (ecc_bytes_total > avail_oobsize ||
>> + strength * steps < 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
>> + * @mtd: mtd info structure
>> + * @chip: nand chip info structure
>> + * @caps: ECC engine caps info structure
>> + *
>> + * 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 mtd_info *mtd, struct nand_chip *chip,
>> + const struct nand_ecc_caps *caps)
>> +{
>> + const struct nand_ecc_step_info *stepinfo;
>> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
>> + int step_size, strength, steps, ecc_bytes, corr;
>> + int best_corr = 0;
>> + int best_step = 0;
>> + int best_strength, best_ecc_bytes;
>> + int i, j;
>> +
>> + if (WARN_ON(avail_oobsize < 0))
>> + return -EINVAL;
>> +
>> + for (i = 0; i < caps->nstepinfos; i++) {
>> + stepinfo = &caps->stepinfos[i];
>> + step_size = stepinfo->stepsize;
>> +
>> +
>
> Extra blank line here.
OK. I will remove it.
>> +
>> +/**
>> + * 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
>> + * @oob_reserve_bytes: number of bytes in OOB that must be reserved
>> + */
>> +struct nand_ecc_caps {
>> + const struct nand_ecc_step_info *stepinfos;
>> + int nstepinfos;
>> + int (*calc_ecc_bytes)(int step_size, int strength);
>> + int oob_reserve_bytes;
>
> Why is this needed? I thought we agreed on passing oobavail as an
> argument to these helper funcs. If a driver needs to reserve a few OOB
> bytes, then doing mtd->oobsize - rsvd_bytes is not such a big deal.
oobavail is really chip-dependent, so I agreed
that it can not be included in the caps struct.
Then, I flipped the logic.
The number of reserved bytes will be more chip-independent.
But, oob_reserve_bytes may not necessarily a fixed value.
I can pass oobavail as a function argument.
--
Best Regards
Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 08:20 +0200 |
| Subject | Re: [PATCH v4 03/23] mtd: nand: add generic helpers to check, match, maximize ECC settings |
| Message-ID | <tPCtZ-2PT-23@gated-at.bofh.it> |
| In reply to | #1659334 |
On Wed, 7 Jun 2017 10:48:33 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> 2017-06-07 6:47 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>:
> > On Tue, 6 Jun 2017 08:21:42 +0900
> > Masahiro Yamada <yamada.masahiro@socionext.com> wrote:
> >
> >> 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 since the previous version:
> >>
> >> - Step size info holds an array of associated strengths
> >> - nand_match_ecc_req() does not take care of the case
> >> where ecc_size/strength is already set
> >> - Reflect more comments from Boris
> >>
> >> Previous version:
> >> http://patchwork.ozlabs.org/patch/752107/
> >>
> >>
> >> Changes in v4: None
> >> Changes in v3: None
> >> Changes in v2: None
> >>
> >> drivers/mtd/nand/nand_base.c | 219 +++++++++++++++++++++++++++++++++++++++++++
> >> include/linux/mtd/nand.h | 35 +++++++
> >> 2 files changed, 254 insertions(+)
> >>
> >> diff --git a/drivers/mtd/nand/nand_base.c b/drivers/mtd/nand/nand_base.c
> >> index bdfa903..f2da4f2 100644
> >> --- a/drivers/mtd/nand/nand_base.c
> >> +++ b/drivers/mtd/nand/nand_base.c
> >> @@ -4509,6 +4509,225 @@ static int nand_set_ecc_soft_ops(struct mtd_info *mtd)
> >> }
> >> }
> >>
> >> +/**
> >> + * nand_check_ecc_caps - check the sanity of preset ECC settings
> >> + * @mtd: mtd info structure
> >> + * @chip: nand chip info structure
> >> + * @caps: ECC caps info structure
> >> + *
> >> + * 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 mtd_info *mtd, struct nand_chip *chip,
One more thing I didn't spot in my previous review: please only pass
chip here. mtd can be extracted using nand_to_mtd(chip). This is
applicable to all your helpers.
> >> + const struct nand_ecc_caps *caps)
> >> +{
> >> + const struct nand_ecc_step_info *stepinfo;
> >> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
> >> + int preset_step = chip->ecc.size;
> >> + int preset_strength = chip->ecc.strength;
> >> + int ecc_bytes;
> >> + int i, j;
> >> +
> >> + if (WARN_ON(avail_oobsize < 0))
> >> + return -EINVAL;
> >> +
> >> + if (!preset_step || !preset_strength)
> >> + return -ENODATA;
> >> +
> >> + 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)
> >> + goto found;
> >> + }
> >> + }
> >> +
> >> + pr_err("ECC (step, strength) = (%d, %d) not supported on this controller",
> >> + preset_step, preset_strength);
> >> +
> >> + return -ENOTSUPP;
> >> +
> >> +found:
> >
> > I prefer something like:
> >
> > if (i == caps->nstepinfos) {
> > pr_err(...);
> > return -ENOTSUPP;
> > }
> >
> > ...
> >
> > instead of this 'found' label.
>
>
> I want to bail-out if (step, strength) matches.
> In this version, the for-loop is double-nested by "step" and "strength".
> In C language, it is not possible to bail-out from multi-nested loop
> with a single "break;" statement. That is why I used "found:" label to do it.
You're right. I didn't pay attention to the nested for loop.
>
> In my first version where there was a single for-loop,
> I did not use the goto label.
> http://patchwork.ozlabs.org/patch/752107/
>
> Do you have any suggestion for cleaner implementation?
>
>
You can do:
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 > avail_oobsize) {
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;
>
> >> + ecc_bytes = caps->calc_ecc_bytes(preset_step, preset_strength);
> >> + if (WARN_ON_ONCE(ecc_bytes < 0))
> >> + return ecc_bytes;
> >> +
> >> + if (ecc_bytes * mtd->writesize / preset_step > avail_oobsize) {
> >> + 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;
> >> +}
> >> +EXPORT_SYMBOL_GPL(nand_check_ecc_caps);
> >> +
> >> +/**
> >> + * nand_match_ecc_req - meet the chip's requirement with least ECC bytes
> >> + * @mtd: mtd info structure
> >> + * @chip: nand chip info structure
> >> + * @caps: ECC engine caps info structure
> >> + *
> >> + * 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 mtd_info *mtd, struct nand_chip *chip,
> >> + const struct nand_ecc_caps *caps)
> >> +{
> >> + const struct nand_ecc_step_info *stepinfo;
> >> + int avail_oobsize = mtd->oobsize - caps->oob_reserve_bytes;
> >> + int req_step = chip->ecc_step_ds;
> >> + int req_strength = chip->ecc_strength_ds;
> >> + int req_corr, step_size, strength, steps, ecc_bytes, ecc_bytes_total;
> >> + int best_step, best_strength, best_ecc_bytes;
> >> + int best_ecc_bytes_total = INT_MAX;
> >
> > Just nitpicking, but why not -1 instead of INT_MAX?
>
> Because nand_match_ecc_req() prefers a smaller ecc_bytes_total.
> So I chose the largest int number as an init value.
> If we started from -1, the following if-conditional would have no effect.
Okay, that's a good reason :-).
>
> /*
> * 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;
> }
>
>
>
>
>
>
> >> + int i, j;
> >> +
> >> + if (WARN_ON(avail_oobsize < 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;
> >> +
> >> + steps = 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 * steps;
> >> +
> >> + if (ecc_bytes_total > avail_oobsize ||
> >> + strength * steps < 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);
> >> +
[...]
> >> +
> >> +/**
> >> + * 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
> >> + * @oob_reserve_bytes: number of bytes in OOB that must be reserved
> >> + */
> >> +struct nand_ecc_caps {
> >> + const struct nand_ecc_step_info *stepinfos;
> >> + int nstepinfos;
> >> + int (*calc_ecc_bytes)(int step_size, int strength);
> >> + int oob_reserve_bytes;
> >
> > Why is this needed? I thought we agreed on passing oobavail as an
> > argument to these helper funcs. If a driver needs to reserve a few OOB
> > bytes, then doing mtd->oobsize - rsvd_bytes is not such a big deal.
>
>
> oobavail is really chip-dependent, so I agreed
> that it can not be included in the caps struct.
>
> Then, I flipped the logic.
> The number of reserved bytes will be more chip-independent.
> But, oob_reserve_bytes may not necessarily a fixed value.
>
> I can pass oobavail as a function argument.
Yes please.
Thanks,
Boris
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:30 +0200 |
| Subject | [PATCH v4 05/23] mtd: nand: denali: remove Toshiba and Hynix specific fixup code |
| Message-ID | <tP9BE-S3-53@gated-at.bofh.it> |
| In reply to | #1658233 |
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 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 3204c51..422b6e4 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,
@@ -1638,6 +1604,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-06 01:30 +0200 |
| Subject | [PATCH v4 15/23] mtd: nand: denali: fix bank reset function to detect the number of chips |
| Message-ID | <tP9BF-S3-61@gated-at.bofh.it> |
| In reply to | #1658233 |
The nand_scan_ident() iterates over maxchips, and calls nand_reset()
for each. This driver currently passes the maximum number of banks
(=chip selects) supported by the controller as maxchips. So, maxchips
is typically 4 or 8. Usually, less number of NAND chips are connected
to the controller.
This can be a problem for ONFi devices. Now, this driver implements
->setup_data_interface() hook, so nand_setup_data_interface() issues
Set Features (0xEF) command, which waits until the chip returns R/B#
response. If no chip there, we know it never happens, but the driver
still ends up with waiting for a long time. It will finally bail-out
with timeout error and the driver will work with existing chips, but
unnecessary wait will give a bad user experience.
The denali_nand_reset() polls the INTR__RST_COMP and INTR__TIME_OUT
bits, but they are always set even if not NAND chip is connected to
that bank. To know the chip existence, INTR__INT_ACT bit must be
checked; this flag is set only when R/B# is toggled. Since the Reset
(0xFF) command toggles the R/B# pin, this can be used to know the
actual number of chips, and update denali->max_banks.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Boris mentioned this information can be retrieved from DT
(http://patchwork.ozlabs.org/patch/745118/), but I'd like to
take time for controller/chip decoupling. I am tackling on
that, but not completed yet.
I believe this commit stands for denali_pci, at least I do not
know how to get the number of chips from PCI.
Changes in v4:
- Reword commit-log
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 52 +++++++++++++++++++++--------------------------
1 file changed, 23 insertions(+), 29 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 083dfc7..775387e 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -85,33 +85,6 @@ static void index_addr(struct denali_nand_info *denali,
iowrite32(data, denali->flash_mem + 0x10);
}
-/* Reset the flash controller */
-static uint16_t denali_nand_reset(struct denali_nand_info *denali)
-{
- int i;
-
- for (i = 0; i < denali->max_banks; i++)
- iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
- denali->flash_reg + INTR_STATUS(i));
-
- for (i = 0; i < denali->max_banks; i++) {
- iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
- while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
- (INTR__RST_COMP | INTR__TIME_OUT)))
- cpu_relax();
- if (ioread32(denali->flash_reg + INTR_STATUS(i)) &
- INTR__TIME_OUT)
- dev_dbg(denali->dev,
- "NAND Reset operation timed out on bank %d\n", i);
- }
-
- for (i = 0; i < denali->max_banks; i++)
- iowrite32(INTR__RST_COMP | INTR__TIME_OUT,
- denali->flash_reg + INTR_STATUS(i));
-
- return PASS;
-}
-
/*
* Use the configuration feature register to determine the maximum number of
* banks that the hardware supports.
@@ -999,7 +972,28 @@ static int denali_setup_data_interface(struct mtd_info *mtd,
return 0;
}
-/* Initialization code to bring the device up to a known good state */
+static void denali_reset_banks(struct denali_nand_info *denali)
+{
+ int i;
+
+ denali_clear_irq_all(denali);
+
+ for (i = 0; i < denali->max_banks; i++) {
+ iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
+ while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
+ (INTR__RST_COMP | INTR__TIME_OUT)))
+ cpu_relax();
+ if (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
+ INTR__INT_ACT))
+ break;
+ }
+
+ dev_dbg(denali->dev, "%d chips connected\n", i);
+ denali->max_banks = i;
+
+ denali_clear_irq_all(denali);
+}
+
static void denali_hw_init(struct denali_nand_info *denali)
{
/*
@@ -1019,7 +1013,7 @@ static void denali_hw_init(struct denali_nand_info *denali)
denali->bbtskipbytes = ioread32(denali->flash_reg +
SPARE_AREA_SKIP_BYTES);
detect_max_banks(denali);
- denali_nand_reset(denali);
+ denali_reset_banks(denali);
iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED);
iowrite32(CHIP_EN_DONT_CARE__FLAG,
denali->flash_reg + CHIP_ENABLE_DONT_CARE);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:40 +0200 |
| Subject | [PATCH v4 06/23] mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants |
| Message-ID | <tP9Lj-Vk-1@gated-at.bofh.it> |
| In reply to | #1658233 |
Add two compatible strings for UniPhier SoC family.
"socionext,uniphier-denali-nand-v5a" is used on UniPhier sLD3, LD4,
Pro4, sLD8.
"socionext,uniphier-denali-nand-v5b" is used on UniPhier Pro5, PXs2,
LD6b, LD11, LD20.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v4:
- Adjusted to generic helpers for ECC engine caps
Changes in v3: None
Changes in v2:
- Change the compatible strings
- Fix the ecc_strength_capability
- Override revision number for the newer one
.../devicetree/bindings/mtd/denali-nand.txt | 6 +++++
drivers/mtd/nand/denali_dt.c | 31 ++++++++++++++++++++++
2 files changed, 37 insertions(+)
diff --git a/Documentation/devicetree/bindings/mtd/denali-nand.txt b/Documentation/devicetree/bindings/mtd/denali-nand.txt
index b7742a7..504291d 100644
--- a/Documentation/devicetree/bindings/mtd/denali-nand.txt
+++ b/Documentation/devicetree/bindings/mtd/denali-nand.txt
@@ -3,6 +3,8 @@
Required properties:
- compatible : should be one of the following:
"altr,socfpga-denali-nand" - for Altera SOCFPGA
+ "socionext,uniphier-denali-nand-v5a" - for Socionext UniPhier (v5a)
+ "socionext,uniphier-denali-nand-v5b" - for Socionext UniPhier (v5b)
- reg : should contain registers location and length for data and reg.
- reg-names: Should contain the reg names "nand_data" and "denali_reg"
- interrupts : The interrupt number.
@@ -10,8 +12,12 @@ Required properties:
Optional properties:
- nand-ecc-step-size: see nand.txt for details. If present, the value must be
512 for "altr,socfpga-denali-nand"
+ 1024 for "socionext,uniphier-denali-nand-v5a"
+ 1024 for "socionext,uniphier-denali-nand-v5b"
- nand-ecc-strength: see nand.txt for details. Valid values are:
8, 15 for "altr,socfpga-denali-nand"
+ 8, 16, 24 for "socionext,uniphier-denali-nand-v5a"
+ 8, 16 for "socionext,uniphier-denali-nand-v5b"
- nand-ecc-maximize: see nand.txt for details
The device tree may optionally contain sub-nodes describing partitions of the
diff --git a/drivers/mtd/nand/denali_dt.c b/drivers/mtd/nand/denali_dt.c
index 8c09bbe..38800ae 100644
--- a/drivers/mtd/nand/denali_dt.c
+++ b/drivers/mtd/nand/denali_dt.c
@@ -45,11 +45,42 @@ static const struct denali_dt_data denali_socfpga_data = {
},
};
+static const int denali_uniphier_strengths[] = {8, 16, 24};
+static const struct denali_dt_data denali_uniphier_v5a_data = {
+ .caps = DENALI_CAP_HW_ECC_FIXUP |
+ DENALI_CAP_DMA_64BIT,
+ .stepinfo = {
+ .stepsize = 1024,
+ .strengths = denali_uniphier_strengths,
+ .nstrengths = ARRAY_SIZE(denali_uniphier_strengths),
+ },
+};
+
+static const struct denali_dt_data denali_uniphier_v5b_data = {
+ .revision = 0x0501,
+ .caps = DENALI_CAP_HW_ECC_FIXUP |
+ DENALI_CAP_DMA_64BIT,
+ .stepinfo = {
+ .stepsize = 1024,
+ .strengths = denali_uniphier_strengths,
+ /* no support for strength=24 */
+ .nstrengths = ARRAY_SIZE(denali_uniphier_strengths) - 1,
+ },
+};
+
static const struct of_device_id denali_nand_dt_ids[] = {
{
.compatible = "altr,socfpga-denali-nand",
.data = &denali_socfpga_data,
},
+ {
+ .compatible = "socionext,uniphier-denali-nand-v5a",
+ .data = &denali_uniphier_v5a_data,
+ },
+ {
+ .compatible = "socionext,uniphier-denali-nand-v5b",
+ .data = &denali_uniphier_v5b_data,
+ },
{ /* sentinel */ }
};
MODULE_DEVICE_TABLE(of, denali_nand_dt_ids);
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:40 +0200 |
| Subject | [PATCH v4 16/23] mtd: nand: denali: use interrupt instead of polling for bank reset |
| Message-ID | <tP9Lj-Vk-13@gated-at.bofh.it> |
| In reply to | #1658233 |
The current bank reset implementation polls the INTR_STATUS register
until interested bits are set. This is not good because:
- polling simply wastes time-slice of the thread
- The while() loop may continue eternally if no bit is set, for
example, due to the controller problem. The denali_wait_for_irq()
uses wait_for_completion_timeout(), which is safer.
We can use interrupt by moving the denali_reset_bank() call below
the interrupt setup.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 28 ++++++++++++++--------------
1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 775387e..6dee168 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -974,24 +974,25 @@ static int denali_setup_data_interface(struct mtd_info *mtd,
static void denali_reset_banks(struct denali_nand_info *denali)
{
+ u32 irq_status;
int i;
- denali_clear_irq_all(denali);
-
for (i = 0; i < denali->max_banks; i++) {
- iowrite32(1 << i, denali->flash_reg + DEVICE_RESET);
- while (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
- (INTR__RST_COMP | INTR__TIME_OUT)))
- cpu_relax();
- if (!(ioread32(denali->flash_reg + INTR_STATUS(i)) &
- INTR__INT_ACT))
+ denali->flash_bank = i;
+
+ denali_reset_irq(denali);
+
+ iowrite32(DEVICE_RESET__BANK(i),
+ denali->flash_reg + DEVICE_RESET);
+
+ irq_status = denali_wait_for_irq(denali,
+ INTR__RST_COMP | INTR__INT_ACT | INTR__TIME_OUT);
+ if (!(irq_status & INTR__INT_ACT))
break;
}
dev_dbg(denali->dev, "%d chips connected\n", i);
denali->max_banks = i;
-
- denali_clear_irq_all(denali);
}
static void denali_hw_init(struct denali_nand_info *denali)
@@ -1013,7 +1014,6 @@ static void denali_hw_init(struct denali_nand_info *denali)
denali->bbtskipbytes = ioread32(denali->flash_reg +
SPARE_AREA_SKIP_BYTES);
detect_max_banks(denali);
- denali_reset_banks(denali);
iowrite32(0x0F, denali->flash_reg + RB_PIN_ENABLED);
iowrite32(CHIP_EN_DONT_CARE__FLAG,
denali->flash_reg + CHIP_ENABLE_DONT_CARE);
@@ -1147,9 +1147,6 @@ static void denali_drv_init(struct denali_nand_info *denali)
* element that might be access shared data (interrupt status)
*/
spin_lock_init(&denali->irq_lock);
-
- /* indicate that MTD has not selected a valid bank yet */
- denali->flash_bank = CHIP_SELECT_INVALID;
}
static int denali_multidev_fixup(struct denali_nand_info *denali)
@@ -1224,6 +1221,9 @@ int denali_init(struct denali_nand_info *denali)
}
denali_enable_irq(denali);
+ denali_reset_banks(denali);
+
+ denali->flash_bank = CHIP_SELECT_INVALID;
nand_set_flash_node(chip, denali->dev->of_node);
/* Fallback to the default name if DT did not give "label" property */
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-06 01:40 +0200 |
| Subject | [PATCH v4 13/23] mtd: nand: denali: fix NAND_CMD_PARAM handling |
| Message-ID | <tP9Lk-Vk-17@gated-at.bofh.it> |
| In reply to | #1658233 |
NAND_CMD_PARAM is not working at all due to multiple bugs.
[1] The command 0x90 issued instead of 0xec
The command code 0x90 is hard-code as
index_addr(denali, addr | 0, 0x90)
So, Read ID (0x90) command is sent to the device instead of Read
Parameter Page (0xec).
[2] only first 8 bytes are read
Even if [1] is fixed, the current implementation is problematic.
The only first 8 bytes are read by MAP11 command, and put into the
temporal buffer:
for (i = 0; i < 8; i++) {
index_addr_read_data(denali, addr | 2, &id);
write_byte_to_buf(denali, id);
}
Obviously, this is not sufficient for NAND_CMD_PARAM; the ONFi
parameters are 256-byte long. This is still insufficient.
As you see in nand_flash_detect_onfi() reads out (256 * 3) bytes
at maximum (Redundant Parameter Pages). However, changing the loop
to for (i = 0; i < 768; i++) is a crazy idea. At the point of the
chip->cmdfunc() call, we cannot know how many times chip->read_byte()
will be called. So, pre-reading enough number of bytes in the
chip->cmdfunc() is a design mistake.
[3] no wait for R/B#
The current code handles NAND_CMD_READID and NAND_CMD_PARAM in the
same way, but this is also wrong. The difference between them is
that Read ID command does not toggle R/B# whereas the Read Parameter
Page command requires R/B#. Without the wait for R/B# interrupt,
wrong data are retrieved.
In order to fix those problems, data read cycle of the MAP11 command
has been moved to chip->read_byte(). Data are read out as needed.
Another good thing is early temporal buffer is not needed any more.
The ugly devm_kzalloc()/devm_kfree() dance has been killed.
Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---
Changes in v4: None
Changes in v3: None
Changes in v2:
- Newly added
drivers/mtd/nand/denali.c | 95 +++++++++++++++--------------------------------
drivers/mtd/nand/denali.h | 2 -
2 files changed, 30 insertions(+), 67 deletions(-)
diff --git a/drivers/mtd/nand/denali.c b/drivers/mtd/nand/denali.c
index 372c871..4d46202 100644
--- a/drivers/mtd/nand/denali.c
+++ b/drivers/mtd/nand/denali.c
@@ -85,28 +85,6 @@ static void index_addr(struct denali_nand_info *denali,
iowrite32(data, denali->flash_mem + 0x10);
}
-/* Perform an indexed read of the device */
-static void index_addr_read_data(struct denali_nand_info *denali,
- uint32_t address, uint32_t *pdata)
-{
- iowrite32(address, denali->flash_mem);
- *pdata = ioread32(denali->flash_mem + 0x10);
-}
-
-/*
- * We need to buffer some data for some of the NAND core routines.
- * The operations manage buffering that data.
- */
-static void reset_buf(struct denali_nand_info *denali)
-{
- denali->buf.head = denali->buf.tail = 0;
-}
-
-static void write_byte_to_buf(struct denali_nand_info *denali, uint8_t byte)
-{
- denali->buf.buf[denali->buf.tail++] = byte;
-}
-
/* Reset the flash controller */
static uint16_t denali_nand_reset(struct denali_nand_info *denali)
{
@@ -286,6 +264,15 @@ static void setup_ecc_for_xfer(struct denali_nand_info *denali, bool ecc_en,
iowrite32(transfer_spare_flag, denali->flash_reg + TRANSFER_SPARE_REG);
}
+static uint8_t denali_read_byte(struct mtd_info *mtd)
+{
+ struct denali_nand_info *denali = mtd_to_denali(mtd);
+
+ iowrite32(MODE_11 | BANK(denali->flash_bank) | 2, denali->flash_mem);
+
+ return ioread32(denali->flash_mem + 0x10);
+}
+
/*
* sends a pipeline command operation to the controller. See the Denali NAND
* controller's user guide for more information (section 4.2.3.6).
@@ -828,17 +815,6 @@ static int denali_read_page_raw(struct mtd_info *mtd, struct nand_chip *chip,
return 0;
}
-static uint8_t denali_read_byte(struct mtd_info *mtd)
-{
- struct denali_nand_info *denali = mtd_to_denali(mtd);
- uint8_t result = 0xff;
-
- if (denali->buf.head < denali->buf.tail)
- result = denali->buf.buf[denali->buf.head++];
-
- return result;
-}
-
static void denali_select_chip(struct mtd_info *mtd, int chip)
{
struct denali_nand_info *denali = mtd_to_denali(mtd);
@@ -873,43 +849,40 @@ 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, id;
- int i;
+ uint32_t addr, irq_status;
+ int wait_ready = 0;
switch (cmd) {
- case NAND_CMD_STATUS:
- reset_buf(denali);
- addr = MODE_11 | BANK(denali->flash_bank);
- index_addr(denali, addr | 0, cmd);
- index_addr_read_data(denali, addr | 2, &id);
- write_byte_to_buf(denali, id);
+ case NAND_CMD_PARAM:
+ wait_ready = 1;
break;
+ case NAND_CMD_STATUS:
case NAND_CMD_READID:
- case NAND_CMD_PARAM:
- reset_buf(denali);
- /*
- * sometimes ManufactureId read from register is not right
- * e.g. some of Micron MT29F32G08QAA MLC NAND chips
- * So here we send READID cmd to NAND insteand
- */
- addr = MODE_11 | BANK(denali->flash_bank);
- index_addr(denali, addr | 0, 0x90);
- index_addr(denali, addr | 1, col);
- for (i = 0; i < 8; i++) {
- index_addr_read_data(denali, addr | 2, &id);
- write_byte_to_buf(denali, id);
- }
break;
case NAND_CMD_RESET:
reset_bank(denali);
break;
case NAND_CMD_READOOB:
/* TODO: Read OOB data */
- break;
+ return;
default:
pr_err(": unsupported command received 0x%x\n", cmd);
- break;
+ 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) \
@@ -1244,12 +1217,6 @@ int denali_init(struct denali_nand_info *denali)
struct mtd_info *mtd = nand_to_mtd(chip);
int ret;
- /* allocate a temporary buffer for nand_scan_ident() */
- denali->buf.buf = devm_kzalloc(denali->dev, PAGE_SIZE,
- GFP_DMA | GFP_KERNEL);
- if (!denali->buf.buf)
- return -ENOMEM;
-
mtd->dev.parent = denali->dev;
denali_hw_init(denali);
denali_drv_init(denali);
@@ -1289,8 +1256,6 @@ int denali_init(struct denali_nand_info *denali)
if (ret)
goto disable_irq;
- /* allocate the right size buffer now */
- devm_kfree(denali->dev, denali->buf.buf);
denali->buf.buf = devm_kzalloc(denali->dev,
mtd->writesize + mtd->oobsize,
GFP_KERNEL);
diff --git a/drivers/mtd/nand/denali.h b/drivers/mtd/nand/denali.h
index a46473a..54f21f4 100644
--- a/drivers/mtd/nand/denali.h
+++ b/drivers/mtd/nand/denali.h
@@ -306,8 +306,6 @@
#define MODE_11 0x0C000000
struct nand_buf {
- int head;
- int tail;
uint8_t *buf;
dma_addr_t dma_buf;
};
--
2.7.4
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 00:10 +0200 |
| Message-ID | <tPuPM-6gH-7@gated-at.bofh.it> |
| In reply to | #1658233 |
Hi Masahiro, On Tue, 6 Jun 2017 08:21:39 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > This patch series intends to solve various problems. > > [1] The driver just retrieves the OOB area as-is > whereas the controller uses syndrome page layout. > [2] Many NAND chip specific parameters are hard-coded in the driver. > [3] ONFi devices are not working > [4] It can not read Bad Block Marker > > Outstanding changes are: > - Fix raw/oob callbacks for syndrome page layout > - Implement setup_data_interface() callback > - Fix/implement more commands for ONFi devices > - Allow to skip the driver internal bounce buffer > - Support PIO in case DMA is not supported > - Switch from ->cmdfunc over to ->cmd_ctrl > > 18 patches were merged at v2. > 11 patches were merged at v3. > Here is the rest of the series. > > v1: https://lkml.org/lkml/2016/11/26/144 > v2: https://lkml.org/lkml/2017/3/22/804 > v3: https://lkml.org/lkml/2017/3/30/90 > > > Masahiro Yamada (23): > mtd: nand: denali_dt: clean up resource ioremap > mtd: nand: denali: use BIT() and GENMASK() for register macros > mtd: nand: add generic helpers to check, match, maximize ECC settings > mtd: nand: denali: avoid hard-coding ECC step, strength, bytes > mtd: nand: denali: remove Toshiba and Hynix specific fixup code > mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants > mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS > mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() > mtd: nand: denali: remove unneeded find_valid_banks() > mtd: nand: denali: handle timing parameters by setup_data_interface() > mtd: nand: denali: rework interrupt handling > mtd: nand: denali: fix NAND_CMD_STATUS handling > mtd: nand: denali: fix NAND_CMD_PARAM handling > mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc > mtd: nand: denali: fix bank reset function to detect the number of > chips > mtd: nand: denali: use interrupt instead of polling for bank reset > mtd: nand: denali: propagate page to helpers via function argument > mtd: nand: denali: merge struct nand_buf into struct denali_nand_info > mtd: nand: denali: use flag instead of register macro for direction > mtd: nand: denali: fix raw and oob accessors for syndrome page layout > mtd: nand: denali: skip driver internal bounce buffer when possible > mtd: nand: denali: use non-managed kmalloc() for DMA buffer > mtd: nand: denali: enable bad block table scan I'd like to apply as much patches as possible from this series (already applied patches 1 and 2). Can you point patches that actually depend on patches 3 and 4? > > .../devicetree/bindings/mtd/denali-nand.txt | 13 + > drivers/mtd/nand/denali.c | 1701 +++++++++----------- > drivers/mtd/nand/denali.h | 294 ++-- > drivers/mtd/nand/denali_dt.c | 54 +- > drivers/mtd/nand/denali_pci.c | 15 +- > drivers/mtd/nand/nand_base.c | 219 +++ > include/linux/mtd/nand.h | 35 + > 7 files changed, 1235 insertions(+), 1096 deletions(-) >
[toc] | [prev] | [next] | [standalone]
| From | Masahiro Yamada <yamada.masahiro@socionext.com> |
|---|---|
| Date | 2017-06-07 03:30 +0200 |
| Message-ID | <tPxXk-8e6-9@gated-at.bofh.it> |
| In reply to | #1659251 |
Hi Boris, 2017-06-07 7:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > Hi Masahiro, > > On Tue, 6 Jun 2017 08:21:39 +0900 > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > >> This patch series intends to solve various problems. >> >> [1] The driver just retrieves the OOB area as-is >> whereas the controller uses syndrome page layout. >> [2] Many NAND chip specific parameters are hard-coded in the driver. >> [3] ONFi devices are not working >> [4] It can not read Bad Block Marker >> >> Outstanding changes are: >> - Fix raw/oob callbacks for syndrome page layout >> - Implement setup_data_interface() callback >> - Fix/implement more commands for ONFi devices >> - Allow to skip the driver internal bounce buffer >> - Support PIO in case DMA is not supported >> - Switch from ->cmdfunc over to ->cmd_ctrl >> >> 18 patches were merged at v2. >> 11 patches were merged at v3. >> Here is the rest of the series. >> >> v1: https://lkml.org/lkml/2016/11/26/144 >> v2: https://lkml.org/lkml/2017/3/22/804 >> v3: https://lkml.org/lkml/2017/3/30/90 >> >> >> Masahiro Yamada (23): >> mtd: nand: denali_dt: clean up resource ioremap >> mtd: nand: denali: use BIT() and GENMASK() for register macros >> mtd: nand: add generic helpers to check, match, maximize ECC settings >> mtd: nand: denali: avoid hard-coding ECC step, strength, bytes >> mtd: nand: denali: remove Toshiba and Hynix specific fixup code >> mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants >> mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS >> mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() >> mtd: nand: denali: remove unneeded find_valid_banks() >> mtd: nand: denali: handle timing parameters by setup_data_interface() >> mtd: nand: denali: rework interrupt handling >> mtd: nand: denali: fix NAND_CMD_STATUS handling >> mtd: nand: denali: fix NAND_CMD_PARAM handling >> mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc >> mtd: nand: denali: fix bank reset function to detect the number of >> chips >> mtd: nand: denali: use interrupt instead of polling for bank reset >> mtd: nand: denali: propagate page to helpers via function argument >> mtd: nand: denali: merge struct nand_buf into struct denali_nand_info >> mtd: nand: denali: use flag instead of register macro for direction >> mtd: nand: denali: fix raw and oob accessors for syndrome page layout >> mtd: nand: denali: skip driver internal bounce buffer when possible >> mtd: nand: denali: use non-managed kmalloc() for DMA buffer >> mtd: nand: denali: enable bad block table scan > > I'd like to apply as much patches as possible from this series (already > applied patches 1 and 2). Can you point patches that actually depend on > patches 3 and 4? > I think 09 "mtd: nand: denali: remove unneeded find_valid_banks()" is applicable independently. I will try my best to work on v5 quickly. -- Best Regards Masahiro Yamada
[toc] | [prev] | [next] | [standalone]
| From | Boris Brezillon <boris.brezillon@free-electrons.com> |
|---|---|
| Date | 2017-06-07 09:30 +0200 |
| Message-ID | <tPDzH-3vN-13@gated-at.bofh.it> |
| In reply to | #1659332 |
On Wed, 7 Jun 2017 10:21:07 +0900 Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > Hi Boris, > > > 2017-06-07 7:09 GMT+09:00 Boris Brezillon <boris.brezillon@free-electrons.com>: > > Hi Masahiro, > > > > On Tue, 6 Jun 2017 08:21:39 +0900 > > Masahiro Yamada <yamada.masahiro@socionext.com> wrote: > > > >> This patch series intends to solve various problems. > >> > >> [1] The driver just retrieves the OOB area as-is > >> whereas the controller uses syndrome page layout. > >> [2] Many NAND chip specific parameters are hard-coded in the driver. > >> [3] ONFi devices are not working > >> [4] It can not read Bad Block Marker > >> > >> Outstanding changes are: > >> - Fix raw/oob callbacks for syndrome page layout > >> - Implement setup_data_interface() callback > >> - Fix/implement more commands for ONFi devices > >> - Allow to skip the driver internal bounce buffer > >> - Support PIO in case DMA is not supported > >> - Switch from ->cmdfunc over to ->cmd_ctrl > >> > >> 18 patches were merged at v2. > >> 11 patches were merged at v3. > >> Here is the rest of the series. > >> > >> v1: https://lkml.org/lkml/2016/11/26/144 > >> v2: https://lkml.org/lkml/2017/3/22/804 > >> v3: https://lkml.org/lkml/2017/3/30/90 > >> > >> > >> Masahiro Yamada (23): > >> mtd: nand: denali_dt: clean up resource ioremap > >> mtd: nand: denali: use BIT() and GENMASK() for register macros > >> mtd: nand: add generic helpers to check, match, maximize ECC settings > >> mtd: nand: denali: avoid hard-coding ECC step, strength, bytes > >> mtd: nand: denali: remove Toshiba and Hynix specific fixup code > >> mtd: nand: denali_dt: add compatible strings for UniPhier SoC variants > >> mtd: nand: denali: set NAND_ECC_CUSTOM_PAGE_ACCESS > >> mtd: nand: denali: do not propagate NAND_STATUS_FAIL to waitfunc() > >> mtd: nand: denali: remove unneeded find_valid_banks() > >> mtd: nand: denali: handle timing parameters by setup_data_interface() > >> mtd: nand: denali: rework interrupt handling > >> mtd: nand: denali: fix NAND_CMD_STATUS handling > >> mtd: nand: denali: fix NAND_CMD_PARAM handling > >> mtd: nand: denali: switch over to cmd_ctrl instead of cmdfunc > >> mtd: nand: denali: fix bank reset function to detect the number of > >> chips > >> mtd: nand: denali: use interrupt instead of polling for bank reset > >> mtd: nand: denali: propagate page to helpers via function argument > >> mtd: nand: denali: merge struct nand_buf into struct denali_nand_info > >> mtd: nand: denali: use flag instead of register macro for direction > >> mtd: nand: denali: fix raw and oob accessors for syndrome page layout > >> mtd: nand: denali: skip driver internal bounce buffer when possible > >> mtd: nand: denali: use non-managed kmalloc() for DMA buffer > >> mtd: nand: denali: enable bad block table scan > > > > I'd like to apply as much patches as possible from this series (already > > applied patches 1 and 2). Can you point patches that actually depend on > > patches 3 and 4? > > > > > I think > 09 "mtd: nand: denali: remove unneeded find_valid_banks()" > is applicable independently. > > I will try my best to work on v5 quickly. Applied patches 1 and 2. Can you send only patches 3 and 4 in your v5 or re-order patches to avoid the dependency on these patches.
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web