This patch series deals mainly with removing driver specific bad block
functions and some minor fixing/cleanup.
1. When initial support for this driver was added [1], then the
plan was to remove these driver specific functions when NAND base
layer has support for doing raw read/write for bad block related
things. Moving to raw read/write seems to take more time so this patch
series implemented alternate logic for reading/writing bad block with
normal read/write itself. This alternate logic relies on some
assumption which seems to be better than driver specific functions and
provide more functionality. Some discussion regarding it has already
happened in [2].
2. Some part of code is duplicated in every read/write function
so clean up the code by making helper function.
3. Bootloaders does access protection for some of the NAND
registers which generates few write errors if it is enabled.
4. Create RAM based BBT table. Flash based BBT table can’t be
used since our bootloader does not have support for the same.
**This patch series is rebased over linux-next with PATCH [3]
[1]: http://patchwork.ozlabs.org/patch/508565/
[2]: https://patchwork.ozlabs.org/patch/920563/
[3]: https://patchwork.ozlabs.org/patch/938631/
Abhishek Sahu (5):
mtd: rawnand: qcom: remove driver specific bad block check function
mtd: rawnand: qcom: remove driver specific block_markbad function
mtd: rawnand: qcom: fix NAND register write errors
mtd: rawnand: qcom: update BBT related flags
mtd: rawnand: qcom: reorganization by removing read/write helpers
drivers/mtd/nand/raw/qcom_nandc.c | 439 ++++++++++++++------------------------
1 file changed, 162 insertions(+), 277 deletions(-)
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation
Driver does not send the commands to NAND device for page
read/write operations in ->cmdfunc(). It just does some
minor variable initialization and rest of the things
are being done in actual ->read/write_oob[_raw].
The generic helper function calls for invoking commands during
page read/write are making this driver complicated. For QCOM NAND
driver, ->cmdfunc() does minor part of initialization and rest of
the initialization is performed by actual page read/write
functions. Also, ->read/write_oob() does not calls helper
function and all the initialization is being done in
->read/write_oob() itself.
Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to
explicitly send READ/PROG commands")', sending of commands has
been moved to driver for page read/write, so this patch does
following changes to make code more readable:
1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op()
helper functions which helps in removing code duplication in each
operation.
2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND
controller waits for BUSY signal to be de asserted and
automatically issues the READ STATUS command. Currently, driver
is storing this status privately and returns the same when status
command comes from helper function after program/erase operation.
Now, for write operations, the status can be returned from main
function itself, so storing status can be removed for program
operations.
Signed-off-by: Abhishek Sahu <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 222 ++++++++++++++++----------------------
1 file changed, 91 insertions(+), 131 deletions(-)
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 6fb85d3..f73ee0e 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host *host, int command)
host->last_command = command;
clear_read_regs(nandc);
-
- if (command == NAND_CMD_RESET || command == NAND_CMD_READID ||
- command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1)
- clear_bam_transaction(nandc);
+ clear_bam_transaction(nandc);
}
/*
- * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our
- * privately maintained status byte, this status byte can be read after
- * NAND_CMD_STATUS is called
+ * QCOM NAND controller by default issues READ STATUS command after PROGRAM
+ * PAGE/BLOCK ERASE operation and updates the same in its internal status
+ * register for last codeword. This function parses status for each CW and
+ * return actual status byte for write/erase operation.
*/
-static void parse_erase_write_errors(struct qcom_nand_host *host, int command)
+static u8 parse_erase_write_errors(struct qcom_nand_host *host, int num_cw)
{
struct nand_chip *chip = &host->chip;
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
- struct nand_ecc_ctrl *ecc = &chip->ecc;
- int num_cw;
int i;
+ u8 status = 0;
- num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1;
nandc_read_buffer_sync(nandc, true);
for (i = 0; i < num_cw; i++) {
u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]);
if (flash_status & FS_MPU_ERR)
- host->status &= ~NAND_STATUS_WP;
+ status &= ~NAND_STATUS_WP;
if (flash_status & FS_OP_ERR || (i == (num_cw - 1) &&
(flash_status &
FS_DEVICE_STS_ERR)))
- host->status |= NAND_STATUS_FAIL;
+ status |= NAND_STATUS_FAIL;
}
+
+ return status;
}
static void post_command(struct qcom_nand_host *host, int command)
@@ -1428,9 +1426,12 @@ static void post_command(struct qcom_nand_host *host, int command)
memcpy(nandc->data_buffer, nandc->reg_read_buf,
nandc->buf_count);
break;
- case NAND_CMD_PAGEPROG:
case NAND_CMD_ERASE1:
- parse_erase_write_errors(host, command);
+ /*
+ * update privately maintained status byte, this status byte can
+ * be read after NAND_CMD_STATUS is called.
+ */
+ host->status = parse_erase_write_errors(host, 1);
break;
default:
break;
@@ -1448,7 +1449,6 @@ static void qcom_nandc_command(struct mtd_info *mtd, unsigned int command,
{
struct nand_chip *chip = mtd_to_nand(mtd);
struct qcom_nand_host *host = to_qcom_nand_host(chip);
- struct nand_ecc_ctrl *ecc = &chip->ecc;
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
bool wait = false;
int ret = 0;
@@ -1477,23 +1477,6 @@ static void qcom_nandc_command(struct mtd_info *mtd, unsigned int command,
wait = true;
break;
- case NAND_CMD_READ0:
- /* we read the entire page for now */
- WARN_ON(column != 0);
-
- host->use_ecc = true;
- set_address(host, 0, page_addr);
- update_rw_regs(host, ecc->steps, true);
- break;
-
- case NAND_CMD_SEQIN:
- WARN_ON(column != 0);
- set_address(host, 0, page_addr);
- break;
-
- case NAND_CMD_PAGEPROG:
- case NAND_CMD_STATUS:
- case NAND_CMD_NONE:
default:
break;
}
@@ -1589,6 +1572,61 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
return 0;
}
+/* check if page write is successful */
+static int check_write_errors(struct qcom_nand_host *host, int cw_cnt)
+{
+ u8 status = parse_erase_write_errors(host, cw_cnt);
+
+ return (status & NAND_STATUS_FAIL) ? -EIO : 0;
+}
+
+/* performs the common init for page read/write operations */
+static void
+qcom_nand_init_page_op(struct qcom_nand_host *host, int num_cw, int page,
+ u16 addr, bool read)
+{
+ struct nand_chip *chip = &host->chip;
+ struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+
+ clear_read_regs(nandc);
+ clear_bam_transaction(nandc);
+ set_address(host, addr, page);
+ update_rw_regs(host, num_cw, read);
+ if (read)
+ config_nand_page_read(nandc);
+ else
+ config_nand_page_write(nandc);
+}
+
+/*
+ * Performs the page operation by submitting DMA descriptors and checks
+ * the errors. For read with ecc, the read function needs to do erased
+ * page detection so skips the error check.
+ */
+static int
+qcom_nand_start_page_op(struct qcom_nand_host *host, int num_cw, bool read)
+{
+ struct nand_chip *chip = &host->chip;
+ struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+ int ret;
+
+ ret = submit_descs(nandc);
+ free_descs(nandc);
+ if (ret) {
+ dev_err(nandc->dev, "%s operation failure\n",
+ read ? "READ" : "WRITE");
+ return ret;
+ }
+
+ if (!read)
+ return check_write_errors(host, num_cw);
+
+ if (!host->use_ecc)
+ return check_flash_errors(host, num_cw);
+
+ return 0;
+}
+
/* performs raw read for one codeword */
static int
qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
@@ -1598,15 +1636,10 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
struct nand_ecc_ctrl *ecc = &chip->ecc;
int data_size1, data_size2, oob_size1, oob_size2;
- int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
+ int reg_off = FLASH_BUF_ACC, read_loc = 0;
- nand_read_page_op(chip, page, 0, NULL, 0);
host->use_ecc = false;
-
- clear_bam_transaction(nandc);
- set_address(host, host->cw_size * cw, page);
- update_rw_regs(host, 1, true);
- config_nand_page_read(nandc);
+ qcom_nand_init_page_op(host, 1, page, host->cw_size * cw, true);
data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
oob_size1 = host->bbm_size;
@@ -1647,14 +1680,7 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
read_data_dma(nandc, reg_off, oob_buf + oob_size1, oob_size2, 0);
- ret = submit_descs(nandc);
- free_descs(nandc);
- if (ret) {
- dev_err(nandc->dev, "failure to read raw cw %d\n", cw);
- return ret;
- }
-
- return check_flash_errors(host, 1);
+ return qcom_nand_start_page_op(host, 1, true);
}
/*
@@ -1857,7 +1883,8 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;
int i, ret;
- config_nand_page_read(nandc);
+ host->use_ecc = true;
+ qcom_nand_init_page_op(host, ecc->steps, page, 0, true);
/* queue cmd descs for each codeword */
for (i = 0; i < ecc->steps; i++) {
@@ -1914,13 +1941,9 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
oob_buf += oob_size;
}
- ret = submit_descs(nandc);
- free_descs(nandc);
-
- if (ret) {
- dev_err(nandc->dev, "failure to read page/oob\n");
+ ret = qcom_nand_start_page_op(host, ecc->steps, true);
+ if (ret)
return ret;
- }
return parse_read_errors(host, data_buf_start, oob_buf_start, page);
}
@@ -1929,17 +1952,8 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
uint8_t *buf, int oob_required, int page)
{
- struct qcom_nand_host *host = to_qcom_nand_host(chip);
- struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
- u8 *data_buf, *oob_buf = NULL;
-
- nand_read_page_op(chip, page, 0, NULL, 0);
- data_buf = buf;
- oob_buf = oob_required ? chip->oob_poi : NULL;
-
- clear_bam_transaction(nandc);
-
- return read_page_ecc(host, data_buf, oob_buf, page);
+ return read_page_ecc(to_qcom_nand_host(chip), buf,
+ oob_required ? chip->oob_poi : NULL, page);
}
/* implements ecc->read_page_raw() */
@@ -1969,18 +1983,8 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
int page)
{
- struct qcom_nand_host *host = to_qcom_nand_host(chip);
- struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
- struct nand_ecc_ctrl *ecc = &chip->ecc;
-
- clear_read_regs(nandc);
- clear_bam_transaction(nandc);
-
- host->use_ecc = true;
- set_address(host, 0, page);
- update_rw_regs(host, ecc->steps, true);
-
- return read_page_ecc(host, NULL, chip->oob_poi, page);
+ return read_page_ecc(to_qcom_nand_host(chip), NULL,
+ chip->oob_poi, page);
}
/* implements ecc->write_page() */
@@ -1991,19 +1995,12 @@ static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
struct nand_ecc_ctrl *ecc = &chip->ecc;
u8 *data_buf, *oob_buf;
- int i, ret;
-
- nand_prog_page_begin_op(chip, page, 0, NULL, 0);
-
- clear_read_regs(nandc);
- clear_bam_transaction(nandc);
+ int i;
data_buf = (u8 *)buf;
oob_buf = chip->oob_poi;
-
host->use_ecc = true;
- update_rw_regs(host, ecc->steps, false);
- config_nand_page_write(nandc);
+ qcom_nand_init_page_op(host, ecc->steps, page, 0, false);
for (i = 0; i < ecc->steps; i++) {
int data_size, oob_size;
@@ -2041,16 +2038,7 @@ static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
oob_buf += oob_size;
}
- ret = submit_descs(nandc);
- if (ret)
- dev_err(nandc->dev, "failure to write page\n");
-
- free_descs(nandc);
-
- if (!ret)
- ret = nand_prog_page_end_op(chip);
-
- return ret;
+ return qcom_nand_start_page_op(host, ecc->steps, false);
}
/* implements ecc->write_page_raw() */
@@ -2062,18 +2050,13 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
struct nand_ecc_ctrl *ecc = &chip->ecc;
u8 *data_buf, *oob_buf;
- int i, ret;
-
- nand_prog_page_begin_op(chip, page, 0, NULL, 0);
- clear_read_regs(nandc);
- clear_bam_transaction(nandc);
+ int i;
data_buf = (u8 *)buf;
oob_buf = chip->oob_poi;
host->use_ecc = false;
- update_rw_regs(host, ecc->steps, false);
- config_nand_page_write(nandc);
+ qcom_nand_init_page_op(host, ecc->steps, page, 0, false);
for (i = 0; i < ecc->steps; i++) {
int data_size1, data_size2, oob_size1, oob_size2;
@@ -2113,16 +2096,7 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,
config_nand_cw_write(nandc);
}
- ret = submit_descs(nandc);
- if (ret)
- dev_err(nandc->dev, "failure to write raw page\n");
-
- free_descs(nandc);
-
- if (!ret)
- ret = nand_prog_page_end_op(chip);
-
- return ret;
+ return qcom_nand_start_page_op(host, ecc->steps, false);
}
/*
@@ -2140,9 +2114,6 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
struct nand_ecc_ctrl *ecc = &chip->ecc;
u8 *oob = chip->oob_poi, bbm_byte;
int data_size, oob_size, bbm_offset, write_size;
- int ret;
-
- clear_bam_transaction(nandc);
/*
* The NAND base layer calls ecc->write_oob() by setting bytes at
@@ -2183,24 +2154,13 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
write_size = data_size + oob_size;
}
- set_address(host, host->cw_size * (ecc->steps - 1), page);
- update_rw_regs(host, 1, false);
-
- config_nand_page_write(nandc);
+ qcom_nand_init_page_op(host, 1, page,
+ host->cw_size * (ecc->steps - 1), false);
write_data_dma(nandc, FLASH_BUF_ACC,
nandc->data_buffer, write_size, 0);
config_nand_cw_write(nandc);
- ret = submit_descs(nandc);
-
- free_descs(nandc);
-
- if (ret) {
- dev_err(nandc->dev, "failure to write oob\n");
- return -EIO;
- }
-
- return nand_prog_page_end_op(chip);
+ return qcom_nand_start_page_op(host, 1, false);
}
/*
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation
Fix the following NAND register write errors which will
be generated if access protection is enabled.
1. SFLASHC_BURST_CFG register is not available for supported
NAND contollers by this driver, so this can be removed.
2. NAND_CTRL is operational register and register writes to
operational registers should always be done through
command descriptors if BAM_MODE is already enabled.
With full boot chain, bootloader already enables
BAM_MODE so read the NAND_CTRL register value and write
only if BAM_MODE is not set.
Signed-off-by: Abhishek Sahu <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 11 ++++++++---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index df12cf3..9e6b383 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2693,15 +2693,20 @@ static int qcom_nandc_setup(struct qcom_nand_controller *nandc)
{
u32 nand_ctrl;
- /* kill onenand */
- nandc_write(nandc, SFLASHC_BURST_CFG, 0);
nandc_write(nandc, dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD),
NAND_DEV_CMD_VLD_VAL);
/* enable ADM or BAM DMA */
if (nandc->props->is_bam) {
nand_ctrl = nandc_read(nandc, NAND_CTRL);
- nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
+ /*
+ * Once BAM_MODE_EN bit is set then QPIC_NAND_CTRL register
+ * should be written with BAM instead of writel.
+ * Check if BAM_MODE_EN is already set by bootloader and write
+ * only if this bit is not set.
+ */
+ if (!(nand_ctrl & BAM_MODE_EN))
+ nandc_write(nandc, NAND_CTRL, nand_ctrl | BAM_MODE_EN);
} else {
nandc_write(nandc, NAND_FLASH_CHIP_SELECT, DM_EN);
}
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation
The QCOM NAND controller skips the bad block bytes when reading
with ECC enabled. The NAND base layer invokes ->read_oob() function
for bad block check which expects the BBM bytes to be copied at
badblockpos offset in OOB buffer. When initial support for this
driver was added, the driver specific function was added
temporarily for bad block check with assumption to change
for raw read in NAND base layer. Moving to raw read for bad block
check seems to take more time so this patch removes driver specific
bad block check function and uses following logic.
The QCOM NAND controller updates the BAD_BLOCK_STATUS bits value in
NAND_BUFFER_STATUS register with every page read operation. The OOB
buffer can be updated with this value while checking the status for
last codeword.
QCOM NAND controller layout does not support BBM byte at offset
other than zero so check the chip’s badblockpos and return probe
failure for all these chips.
Signed-off-by: Abhishek Sahu <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 118 ++++++++++----------------------------
1 file changed, 31 insertions(+), 87 deletions(-)
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 994f980..ea253ac 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -80,6 +80,8 @@
/* NAND_BUFFER_STATUS bits */
#define BS_UNCORRECTABLE_BIT BIT(8)
#define BS_CORRECTABLE_ERR_MSK 0x1f
+#define BAD_BLK_STATUS 16
+#define BAD_BLK_STATUS_2 24
/* NAND_DEVn_CFG0 bits */
#define DISABLE_STATUS_AFTER_WRITE 4
@@ -1742,16 +1744,14 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
unsigned int max_bitflips = 0, uncorrectable_cws = 0;
struct read_stats *buf;
bool flash_op_err = false, erased;
- int i;
+ int i, data_len, oob_len;
+ u32 flash, erased_cw, buffer = 0;
u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;
buf = (struct read_stats *)nandc->reg_read_buf;
nandc_read_buffer_sync(nandc, true);
for (i = 0; i < ecc->steps; i++, buf++) {
- u32 flash, buffer, erased_cw;
- int data_len, oob_len;
-
if (i == (ecc->steps - 1)) {
data_len = ecc->size - ((ecc->steps - 1) << 2);
oob_len = ecc->steps << 2;
@@ -1824,12 +1824,24 @@ static int parse_read_errors(struct qcom_nand_host *host, u8 *data_buf,
if (flash_op_err)
return -EIO;
- if (!uncorrectable_cws)
- return max_bitflips;
+ if (uncorrectable_cws) {
+ max_bitflips = check_for_erased_page(host, data_buf_start,
+ oob_buf_start, uncorrectable_cws,
+ page, max_bitflips);
+ }
- return check_for_erased_page(host, data_buf_start, oob_buf_start,
- uncorrectable_cws, page,
- max_bitflips);
+ /*
+ * Updates the BBM bytes in OOB buffer with BAD_BLK_STATUS bits value
+ * of NAND_BUFFER_STATUS register. With every page read operation,
+ * controller updates the bad block status data into this register.
+ */
+ if (oob_buf_start) {
+ oob_buf_start[0] = (buffer >> BAD_BLK_STATUS) & 0xff;
+ if (chip->options & NAND_BUSWIDTH_16)
+ oob_buf_start[1] = (buffer >> BAD_BLK_STATUS_2) & 0xff;
+ }
+
+ return max_bitflips;
}
/*
@@ -1913,41 +1925,6 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
return parse_read_errors(host, data_buf_start, oob_buf_start, page);
}
-/*
- * a helper that copies the last step/codeword of a page (containing free oob)
- * into our local buffer
- */
-static int copy_last_cw(struct qcom_nand_host *host, int page)
-{
- struct nand_chip *chip = &host->chip;
- struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
- struct nand_ecc_ctrl *ecc = &chip->ecc;
- int size;
- int ret;
-
- clear_read_regs(nandc);
-
- size = host->use_ecc ? host->cw_data : host->cw_size;
-
- /* prepare a clean read buffer */
- memset(nandc->data_buffer, 0xff, size);
-
- set_address(host, host->cw_size * (ecc->steps - 1), page);
- update_rw_regs(host, 1, true);
-
- config_nand_single_cw_page_read(nandc, host->use_ecc);
-
- read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer, size, 0);
-
- ret = submit_descs(nandc);
- if (ret)
- dev_err(nandc->dev, "failed to copy last codeword\n");
-
- free_descs(nandc);
-
- return ret;
-}
-
/* implements ecc->read_page() */
static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
uint8_t *buf, int oob_required, int page)
@@ -2197,44 +2174,6 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
return nand_prog_page_end_op(chip);
}
-static int qcom_nandc_block_bad(struct mtd_info *mtd, loff_t ofs)
-{
- struct nand_chip *chip = mtd_to_nand(mtd);
- struct qcom_nand_host *host = to_qcom_nand_host(chip);
- struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
- struct nand_ecc_ctrl *ecc = &chip->ecc;
- int page, ret, bbpos, bad = 0;
-
- page = (int)(ofs >> chip->page_shift) & chip->pagemask;
-
- /*
- * configure registers for a raw sub page read, the address is set to
- * the beginning of the last codeword, we don't care about reading ecc
- * portion of oob. we just want the first few bytes from this codeword
- * that contains the BBM
- */
- host->use_ecc = false;
-
- clear_bam_transaction(nandc);
- ret = copy_last_cw(host, page);
- if (ret)
- goto err;
-
- if (check_flash_errors(host, 1)) {
- dev_warn(nandc->dev, "error when trying to read BBM\n");
- goto err;
- }
-
- bbpos = mtd->writesize - host->cw_size * (ecc->steps - 1);
-
- bad = nandc->data_buffer[bbpos] != 0xff;
-
- if (chip->options & NAND_BUSWIDTH_16)
- bad = bad || (nandc->data_buffer[bbpos + 1] != 0xff);
-err:
- return bad;
-}
-
static int qcom_nandc_block_markbad(struct mtd_info *mtd, loff_t ofs)
{
struct nand_chip *chip = mtd_to_nand(mtd);
@@ -2501,6 +2440,12 @@ static int qcom_nand_host_setup(struct qcom_nand_host *host)
return ret;
}
+ if (chip->badblockpos != 0) {
+ dev_err(nandc->dev, "BBM at %d offset is not supported\n",
+ chip->badblockpos);
+ return -ENOTSUPP;
+ }
+
if (ecc->strength >= 8) {
/* 8 bit ECC defaults to BCH ECC on all platforms */
host->bch_enabled = true;
@@ -2813,14 +2758,13 @@ static int qcom_nand_host_init(struct qcom_nand_controller *nandc,
chip->get_features = nand_get_set_features_notsupp;
/*
- * the bad block marker is readable only when we read the last codeword
+ * the bad block marker is writable only when we write the last codeword
* of a page with ECC disabled. currently, the nand_base and nand_bbt
- * helpers don't allow us to read BB from a nand chip with ECC
- * disabled (MTD_OPS_PLACE_OOB is set by default). use the block_bad
- * and block_markbad helpers until we permanently switch to using
+ * helpers don't allow us to write BB from a nand chip with ECC
+ * disabled (MTD_OPS_PLACE_OOB is set by default). use the block_markbad
+ * helpers until we permanently switch to using
* MTD_OPS_RAW for all drivers (with the help of badblockbits)
*/
- chip->block_bad = qcom_nandc_block_bad;
chip->block_markbad = qcom_nandc_block_markbad;
chip->controller = &nandc->controller;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation
Remove the NAND_SKIP_BBTSCAN to use RAM based BBT.
Flash based BBT is not used since bootloaders
doesn't have support for the same.
Signed-off-by: Abhishek Sahu <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 9e6b383..6fb85d3 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2750,8 +2750,7 @@ static int qcom_nand_host_init(struct qcom_nand_controller *nandc,
chip->get_features = nand_get_set_features_notsupp;
chip->controller = &nandc->controller;
- chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER |
- NAND_SKIP_BBTSCAN;
+ chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER;
/* set up initial status value */
host->status = NAND_STATUS_READY | NAND_STATUS_WP;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation
The NAND base layer calls write_oob() by setting bytes at
chip->badblockpos with value non 0xFF for updating bad block status.
The QCOM NAND controller skips the bad block bytes while doing normal
write with ECC enabled. When initial support for this driver was
added, the driver specific function was added temporarily for
block_markbad() with assumption to change for raw read in NAND base
layer. Moving to raw read for block_markbad() seems to take more time
so this patch removes driver specific block_markbad() function by
using following HACK in write_oob() function.
Check for BBM bytes in OOB and accordingly do raw write for updating
BBM bytes in NAND flash or normal write for updating available OOB
bytes.
Signed-off-by: Abhishek Sahu <[email protected]>
---
drivers/mtd/nand/raw/qcom_nandc.c | 103 +++++++++++++++-----------------------
1 file changed, 40 insertions(+), 63 deletions(-)
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index ea253ac..df12cf3 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2138,28 +2138,57 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
struct qcom_nand_host *host = to_qcom_nand_host(chip);
struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
struct nand_ecc_ctrl *ecc = &chip->ecc;
- u8 *oob = chip->oob_poi;
- int data_size, oob_size;
+ u8 *oob = chip->oob_poi, bbm_byte;
+ int data_size, oob_size, bbm_offset, write_size;
int ret;
- host->use_ecc = true;
clear_bam_transaction(nandc);
- /* calculate the data and oob size for the last codeword/step */
- data_size = ecc->size - ((ecc->steps - 1) << 2);
- oob_size = mtd->oobavail;
+ /*
+ * The NAND base layer calls ecc->write_oob() by setting bytes at
+ * chip->badblockpos (chip->badblockpos will be 0 for QCOM NAND
+ * controller layout) in OOB buffer with value other that 0xFF
+ * for updating bad block status. QCOM NAND controller skips
+ * BBM bytes while writing with ECC, so following HACK has been
+ * added in this function for using generic block_markbad() function.
+ *
+ * Check for BBM bytes in OOB and accordingly do raw write for
+ * updating BBM bytes in NAND flash or normal write with ECC for
+ * updating available OOB bytes.
+ */
+ bbm_byte = oob[0];
+ if (chip->options & NAND_BUSWIDTH_16)
+ bbm_byte &= oob[1];
- memset(nandc->data_buffer, 0xff, host->cw_data);
- /* override new oob content to last codeword */
- mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob,
- 0, mtd->oobavail);
+ /* Write BBM bytes by doing raw write. */
+ if (bbm_byte != 0xff) {
+ host->use_ecc = false;
+ memset(nandc->data_buffer, 0xff, host->cw_size);
+ /* Determine the BBM bytes position and update the same */
+ bbm_offset = mtd->writesize - host->cw_size * (ecc->steps - 1);
+ memcpy(nandc->data_buffer + bbm_offset, oob, host->bbm_size);
+ write_size = host->cw_size;
+ /* Write OOB bytes by doing normal write with ECC */
+ } else {
+ host->use_ecc = true;
+ /* calculate the data and oob size for the last codeword/step */
+ data_size = ecc->size - ((ecc->steps - 1) << 2);
+ oob_size = mtd->oobavail;
+
+ memset(nandc->data_buffer, 0xff, host->cw_data);
+ /* override new oob content to last codeword */
+ mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size,
+ oob, 0, mtd->oobavail);
+
+ write_size = data_size + oob_size;
+ }
set_address(host, host->cw_size * (ecc->steps - 1), page);
update_rw_regs(host, 1, false);
config_nand_page_write(nandc);
write_data_dma(nandc, FLASH_BUF_ACC,
- nandc->data_buffer, data_size + oob_size, 0);
+ nandc->data_buffer, write_size, 0);
config_nand_cw_write(nandc);
ret = submit_descs(nandc);
@@ -2174,48 +2203,6 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
return nand_prog_page_end_op(chip);
}
-static int qcom_nandc_block_markbad(struct mtd_info *mtd, loff_t ofs)
-{
- struct nand_chip *chip = mtd_to_nand(mtd);
- struct qcom_nand_host *host = to_qcom_nand_host(chip);
- struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
- struct nand_ecc_ctrl *ecc = &chip->ecc;
- int page, ret;
-
- clear_read_regs(nandc);
- clear_bam_transaction(nandc);
-
- /*
- * to mark the BBM as bad, we flash the entire last codeword with 0s.
- * we don't care about the rest of the content in the codeword since
- * we aren't going to use this block again
- */
- memset(nandc->data_buffer, 0x00, host->cw_size);
-
- page = (int)(ofs >> chip->page_shift) & chip->pagemask;
-
- /* prepare write */
- host->use_ecc = false;
- set_address(host, host->cw_size * (ecc->steps - 1), page);
- update_rw_regs(host, 1, false);
-
- config_nand_page_write(nandc);
- write_data_dma(nandc, FLASH_BUF_ACC,
- nandc->data_buffer, host->cw_size, 0);
- config_nand_cw_write(nandc);
-
- ret = submit_descs(nandc);
-
- free_descs(nandc);
-
- if (ret) {
- dev_err(nandc->dev, "failure to update BBM\n");
- return -EIO;
- }
-
- return nand_prog_page_end_op(chip);
-}
-
/*
* the three functions below implement chip->read_byte(), chip->read_buf()
* and chip->write_buf() respectively. these aren't used for
@@ -2757,16 +2744,6 @@ static int qcom_nand_host_init(struct qcom_nand_controller *nandc,
chip->set_features = nand_get_set_features_notsupp;
chip->get_features = nand_get_set_features_notsupp;
- /*
- * the bad block marker is writable only when we write the last codeword
- * of a page with ECC disabled. currently, the nand_base and nand_bbt
- * helpers don't allow us to write BB from a nand chip with ECC
- * disabled (MTD_OPS_PLACE_OOB is set by default). use the block_markbad
- * helpers until we permanently switch to using
- * MTD_OPS_RAW for all drivers (with the help of badblockbits)
- */
- chip->block_markbad = qcom_nandc_block_markbad;
-
chip->controller = &nandc->controller;
chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER |
NAND_SKIP_BBTSCAN;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc.
is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi Abhishek,
Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
13:21:58 +0530:
> Remove the NAND_SKIP_BBTSCAN to use RAM based BBT.
Unless I am understanding it the wrong way, NAND_SKIP_BBTSCAN will skip
the scan of the on-chip BBT and will scan every block to construct a
RAM, based BBT thanks to the BBM.
So flash based BBT is already unused and removing this flag is a
mistake, right?
> Flash based BBT is not used since bootloaders
> doesn't have support for the same.
>
> Signed-off-by: Abhishek Sahu <[email protected]>
> ---
Thanks,
Miquèl
Boris,
Can you please check the change in qcom_nandc_write_oob() is
valid? I think it is but as this is a bit of a hack I prefer double checking.
Thanks,
Miquèl
Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
13:21:56 +0530:
> The NAND base layer calls write_oob() by setting bytes at
> chip->badblockpos with value non 0xFF for updating bad block status.
> The QCOM NAND controller skips the bad block bytes while doing normal
> write with ECC enabled. When initial support for this driver was
> added, the driver specific function was added temporarily for
> block_markbad() with assumption to change for raw read in NAND base
> layer. Moving to raw read for block_markbad() seems to take more time
> so this patch removes driver specific block_markbad() function by
> using following HACK in write_oob() function.
>
> Check for BBM bytes in OOB and accordingly do raw write for updating
> BBM bytes in NAND flash or normal write for updating available OOB
> bytes.
>
> Signed-off-by: Abhishek Sahu <[email protected]>
> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 103 +++++++++++++++-----------------------
> 1 file changed, 40 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index ea253ac..df12cf3 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2138,28 +2138,57 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> struct qcom_nand_host *host = to_qcom_nand_host(chip);
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> - u8 *oob = chip->oob_poi;
> - int data_size, oob_size;
> + u8 *oob = chip->oob_poi, bbm_byte;
> + int data_size, oob_size, bbm_offset, write_size;
> int ret;
>
> - host->use_ecc = true;
> clear_bam_transaction(nandc);
>
> - /* calculate the data and oob size for the last codeword/step */
> - data_size = ecc->size - ((ecc->steps - 1) << 2);
> - oob_size = mtd->oobavail;
> + /*
> + * The NAND base layer calls ecc->write_oob() by setting bytes at
> + * chip->badblockpos (chip->badblockpos will be 0 for QCOM NAND
> + * controller layout) in OOB buffer with value other that 0xFF
> + * for updating bad block status. QCOM NAND controller skips
> + * BBM bytes while writing with ECC, so following HACK has been
> + * added in this function for using generic block_markbad() function.
> + *
> + * Check for BBM bytes in OOB and accordingly do raw write for
> + * updating BBM bytes in NAND flash or normal write with ECC for
> + * updating available OOB bytes.
> + */
> + bbm_byte = oob[0];
> + if (chip->options & NAND_BUSWIDTH_16)
> + bbm_byte &= oob[1];
>
> - memset(nandc->data_buffer, 0xff, host->cw_data);
> - /* override new oob content to last codeword */
> - mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob,
> - 0, mtd->oobavail);
> + /* Write BBM bytes by doing raw write. */
> + if (bbm_byte != 0xff) {
> + host->use_ecc = false;
> + memset(nandc->data_buffer, 0xff, host->cw_size);
> + /* Determine the BBM bytes position and update the same */
> + bbm_offset = mtd->writesize - host->cw_size * (ecc->steps - 1);
> + memcpy(nandc->data_buffer + bbm_offset, oob, host->bbm_size);
> + write_size = host->cw_size;
> + /* Write OOB bytes by doing normal write with ECC */
> + } else {
> + host->use_ecc = true;
> + /* calculate the data and oob size for the last codeword/step */
> + data_size = ecc->size - ((ecc->steps - 1) << 2);
> + oob_size = mtd->oobavail;
> +
> + memset(nandc->data_buffer, 0xff, host->cw_data);
> + /* override new oob content to last codeword */
> + mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size,
> + oob, 0, mtd->oobavail);
> +
> + write_size = data_size + oob_size;
> + }
>
> set_address(host, host->cw_size * (ecc->steps - 1), page);
> update_rw_regs(host, 1, false);
>
> config_nand_page_write(nandc);
> write_data_dma(nandc, FLASH_BUF_ACC,
> - nandc->data_buffer, data_size + oob_size, 0);
> + nandc->data_buffer, write_size, 0);
> config_nand_cw_write(nandc);
>
> ret = submit_descs(nandc);
> @@ -2174,48 +2203,6 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> return nand_prog_page_end_op(chip);
> }
>
> -static int qcom_nandc_block_markbad(struct mtd_info *mtd, loff_t ofs)
> -{
> - struct nand_chip *chip = mtd_to_nand(mtd);
> - struct qcom_nand_host *host = to_qcom_nand_host(chip);
> - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> - struct nand_ecc_ctrl *ecc = &chip->ecc;
> - int page, ret;
> -
> - clear_read_regs(nandc);
> - clear_bam_transaction(nandc);
> -
> - /*
> - * to mark the BBM as bad, we flash the entire last codeword with 0s.
> - * we don't care about the rest of the content in the codeword since
> - * we aren't going to use this block again
> - */
> - memset(nandc->data_buffer, 0x00, host->cw_size);
> -
> - page = (int)(ofs >> chip->page_shift) & chip->pagemask;
> -
> - /* prepare write */
> - host->use_ecc = false;
> - set_address(host, host->cw_size * (ecc->steps - 1), page);
> - update_rw_regs(host, 1, false);
> -
> - config_nand_page_write(nandc);
> - write_data_dma(nandc, FLASH_BUF_ACC,
> - nandc->data_buffer, host->cw_size, 0);
> - config_nand_cw_write(nandc);
> -
> - ret = submit_descs(nandc);
> -
> - free_descs(nandc);
> -
> - if (ret) {
> - dev_err(nandc->dev, "failure to update BBM\n");
> - return -EIO;
> - }
> -
> - return nand_prog_page_end_op(chip);
> -}
> -
> /*
> * the three functions below implement chip->read_byte(), chip->read_buf()
> * and chip->write_buf() respectively. these aren't used for
> @@ -2757,16 +2744,6 @@ static int qcom_nand_host_init(struct qcom_nand_controller *nandc,
> chip->set_features = nand_get_set_features_notsupp;
> chip->get_features = nand_get_set_features_notsupp;
>
> - /*
> - * the bad block marker is writable only when we write the last codeword
> - * of a page with ECC disabled. currently, the nand_base and nand_bbt
> - * helpers don't allow us to write BB from a nand chip with ECC
> - * disabled (MTD_OPS_PLACE_OOB is set by default). use the block_markbad
> - * helpers until we permanently switch to using
> - * MTD_OPS_RAW for all drivers (with the help of badblockbits)
> - */
> - chip->block_markbad = qcom_nandc_block_markbad;
> -
> chip->controller = &nandc->controller;
> chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER |
> NAND_SKIP_BBTSCAN;
--
Miquel Raynal, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
Hi Abhishek,
Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
13:21:59 +0530:
> Driver does not send the commands to NAND device for page
> read/write operations in ->cmdfunc(). It just does some
> minor variable initialization and rest of the things
> are being done in actual ->read/write_oob[_raw].
Thank you for cleaning actively this driver. I think you are comfortable
enough now to switch to start the migration to ->exec_op().
I will take this patch as I think it shrinks a bit the driver (which is
inordinately huge) but I would like to get rid of any kind of hackish
->cmdfunc() implementation. Boris and I can help you doing that, you
can grep for drivers already converted and observe they structure
(marvell, fsmc, vf610).
>
> The generic helper function calls for invoking commands during
> page read/write are making this driver complicated. For QCOM NAND
> driver, ->cmdfunc() does minor part of initialization and rest of
> the initialization is performed by actual page read/write
> functions. Also, ->read/write_oob() does not calls helper
> function and all the initialization is being done in
> ->read/write_oob() itself.
>
> Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to
> explicitly send READ/PROG commands")', sending of commands has
> been moved to driver for page read/write, so this patch does
> following changes to make code more readable:
>
> 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op()
> helper functions which helps in removing code duplication in each
> operation.
>
> 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND
> controller waits for BUSY signal to be de asserted and
> automatically issues the READ STATUS command. Currently, driver
> is storing this status privately and returns the same when status
> command comes from helper function after program/erase operation.
> Now, for write operations, the status can be returned from main
> function itself, so storing status can be removed for program
> operations.
>
> Signed-off-by: Abhishek Sahu <[email protected]>
> ---
Thanks,
Miquèl
On Wed, 18 Jul 2018 23:15:26 +0200
Miquel Raynal <[email protected]> wrote:
> Hi Abhishek,
>
> Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
> 13:21:58 +0530:
>
> > Remove the NAND_SKIP_BBTSCAN to use RAM based BBT.
>
> Unless I am understanding it the wrong way, NAND_SKIP_BBTSCAN will skip
> the scan of the on-chip BBT and will scan every block to construct a
> RAM, based BBT thanks to the BBM.
>
> So flash based BBT is already unused and removing this flag is a
> mistake, right?
->scan_bbt() is also taking care of building the in-RAM BBT based on
BBM when no on-flash BBT is provided, so I think it's the right thing
to do.
>
> > Flash based BBT is not used since bootloaders
> > doesn't have support for the same.
> >
> > Signed-off-by: Abhishek Sahu <[email protected]>
> > ---
>
> Thanks,
> Miquèl
>
Hi Boris,
Boris Brezillon <[email protected]> wrote on Wed, 18 Jul 2018
23:36:37 +0200:
> On Wed, 18 Jul 2018 23:15:26 +0200
> Miquel Raynal <[email protected]> wrote:
>
> > Hi Abhishek,
> >
> > Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
> > 13:21:58 +0530:
> >
> > > Remove the NAND_SKIP_BBTSCAN to use RAM based BBT.
> >
> > Unless I am understanding it the wrong way, NAND_SKIP_BBTSCAN will skip
> > the scan of the on-chip BBT and will scan every block to construct a
> > RAM, based BBT thanks to the BBM.
> >
> > So flash based BBT is already unused and removing this flag is a
> > mistake, right?
>
> ->scan_bbt() is also taking care of building the in-RAM BBT based on
> BBM when no on-flash BBT is provided, so I think it's the right thing
> to do.
Oh right. Then doing so is harmless.
Thanks for the clarification.
Miquèl
Abhishek,
Miquel Raynal <[email protected]> wrote on Wed, 18 Jul 2018
23:41:44 +0200:
> Hi Boris,
>
> Boris Brezillon <[email protected]> wrote on Wed, 18 Jul 2018
> 23:36:37 +0200:
>
> > On Wed, 18 Jul 2018 23:15:26 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> > > Hi Abhishek,
> > >
> > > Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
> > > 13:21:58 +0530:
> > >
> > > > Remove the NAND_SKIP_BBTSCAN to use RAM based BBT.
> > >
> > > Unless I am understanding it the wrong way, NAND_SKIP_BBTSCAN will skip
> > > the scan of the on-chip BBT and will scan every block to construct a
> > > RAM, based BBT thanks to the BBM.
> > >
> > > So flash based BBT is already unused and removing this flag is a
> > > mistake, right?
> >
> > ->scan_bbt() is also taking care of building the in-RAM BBT based on
> > BBM when no on-flash BBT is provided, so I think it's the right thing
> > to do.
>
> Oh right. Then doing so is harmless.
Could you please update the commit log to reflect this aspect?
>
> Thanks for the clarification.
> Miquèl
On Wed, 18 Jul 2018 23:23:50 +0200
Miquel Raynal <[email protected]> wrote:
> Boris,
>
> Can you please check the change in qcom_nandc_write_oob() is
> valid? I think it is but as this is a bit of a hack I prefer double checking.
Indeed, it's hack-ish.
>
> Thanks,
> Miquèl
>
>
> Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
> 13:21:56 +0530:
>
> > The NAND base layer calls write_oob() by setting bytes at
> > chip->badblockpos with value non 0xFF for updating bad block status.
> > The QCOM NAND controller skips the bad block bytes while doing normal
> > write with ECC enabled. When initial support for this driver was
> > added, the driver specific function was added temporarily for
> > block_markbad() with assumption to change for raw read in NAND base
> > layer. Moving to raw read for block_markbad() seems to take more time
> > so this patch removes driver specific block_markbad() function by
> > using following HACK in write_oob() function.
> >
> > Check for BBM bytes in OOB and accordingly do raw write for updating
> > BBM bytes in NAND flash or normal write for updating available OOB
> > bytes.
Why don't we change that instead of patching the qcom driver to guess
when the core tries to mark a block bad? If you're afraid of breaking
existing drivers that might rely on the "write/read BBM in non-raw
mode" solution (I'm sure some drivers are), you can always add a new
flag in chip->options (NAND_ACCESS_BBM_IN_RAW_MODE) and only use raw
accessors when this flag is set.
> >
> > Signed-off-by: Abhishek Sahu <[email protected]>
> > ---
> > drivers/mtd/nand/raw/qcom_nandc.c | 103 +++++++++++++++-----------------------
> > 1 file changed, 40 insertions(+), 63 deletions(-)
> >
> > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> > index ea253ac..df12cf3 100644
> > --- a/drivers/mtd/nand/raw/qcom_nandc.c
> > +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> > @@ -2138,28 +2138,57 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> > struct qcom_nand_host *host = to_qcom_nand_host(chip);
> > struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> > struct nand_ecc_ctrl *ecc = &chip->ecc;
> > - u8 *oob = chip->oob_poi;
> > - int data_size, oob_size;
> > + u8 *oob = chip->oob_poi, bbm_byte;
> > + int data_size, oob_size, bbm_offset, write_size;
> > int ret;
> >
> > - host->use_ecc = true;
> > clear_bam_transaction(nandc);
> >
> > - /* calculate the data and oob size for the last codeword/step */
> > - data_size = ecc->size - ((ecc->steps - 1) << 2);
> > - oob_size = mtd->oobavail;
> > + /*
> > + * The NAND base layer calls ecc->write_oob() by setting bytes at
> > + * chip->badblockpos (chip->badblockpos will be 0 for QCOM NAND
> > + * controller layout) in OOB buffer with value other that 0xFF
> > + * for updating bad block status. QCOM NAND controller skips
> > + * BBM bytes while writing with ECC, so following HACK has been
> > + * added in this function for using generic block_markbad() function.
> > + *
> > + * Check for BBM bytes in OOB and accordingly do raw write for
> > + * updating BBM bytes in NAND flash or normal write with ECC for
> > + * updating available OOB bytes.
> > + */
> > + bbm_byte = oob[0];
> > + if (chip->options & NAND_BUSWIDTH_16)
> > + bbm_byte &= oob[1];
> >
> > - memset(nandc->data_buffer, 0xff, host->cw_data);
> > - /* override new oob content to last codeword */
> > - mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size, oob,
> > - 0, mtd->oobavail);
> > + /* Write BBM bytes by doing raw write. */
> > + if (bbm_byte != 0xff) {
> > + host->use_ecc = false;
> > + memset(nandc->data_buffer, 0xff, host->cw_size);
> > + /* Determine the BBM bytes position and update the same */
> > + bbm_offset = mtd->writesize - host->cw_size * (ecc->steps - 1);
> > + memcpy(nandc->data_buffer + bbm_offset, oob, host->bbm_size);
> > + write_size = host->cw_size;
> > + /* Write OOB bytes by doing normal write with ECC */
> > + } else {
> > + host->use_ecc = true;
> > + /* calculate the data and oob size for the last codeword/step */
> > + data_size = ecc->size - ((ecc->steps - 1) << 2);
> > + oob_size = mtd->oobavail;
> > +
> > + memset(nandc->data_buffer, 0xff, host->cw_data);
> > + /* override new oob content to last codeword */
> > + mtd_ooblayout_get_databytes(mtd, nandc->data_buffer + data_size,
> > + oob, 0, mtd->oobavail);
> > +
> > + write_size = data_size + oob_size;
> > + }
> >
> > set_address(host, host->cw_size * (ecc->steps - 1), page);
> > update_rw_regs(host, 1, false);
> >
> > config_nand_page_write(nandc);
> > write_data_dma(nandc, FLASH_BUF_ACC,
> > - nandc->data_buffer, data_size + oob_size, 0);
> > + nandc->data_buffer, write_size, 0);
> > config_nand_cw_write(nandc);
> >
> > ret = submit_descs(nandc);
> > @@ -2174,48 +2203,6 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> > return nand_prog_page_end_op(chip);
> > }
> >
> > -static int qcom_nandc_block_markbad(struct mtd_info *mtd, loff_t ofs)
> > -{
> > - struct nand_chip *chip = mtd_to_nand(mtd);
> > - struct qcom_nand_host *host = to_qcom_nand_host(chip);
> > - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> > - struct nand_ecc_ctrl *ecc = &chip->ecc;
> > - int page, ret;
> > -
> > - clear_read_regs(nandc);
> > - clear_bam_transaction(nandc);
> > -
> > - /*
> > - * to mark the BBM as bad, we flash the entire last codeword with 0s.
> > - * we don't care about the rest of the content in the codeword since
> > - * we aren't going to use this block again
> > - */
> > - memset(nandc->data_buffer, 0x00, host->cw_size);
> > -
> > - page = (int)(ofs >> chip->page_shift) & chip->pagemask;
> > -
> > - /* prepare write */
> > - host->use_ecc = false;
> > - set_address(host, host->cw_size * (ecc->steps - 1), page);
> > - update_rw_regs(host, 1, false);
> > -
> > - config_nand_page_write(nandc);
> > - write_data_dma(nandc, FLASH_BUF_ACC,
> > - nandc->data_buffer, host->cw_size, 0);
> > - config_nand_cw_write(nandc);
> > -
> > - ret = submit_descs(nandc);
> > -
> > - free_descs(nandc);
> > -
> > - if (ret) {
> > - dev_err(nandc->dev, "failure to update BBM\n");
> > - return -EIO;
> > - }
> > -
> > - return nand_prog_page_end_op(chip);
> > -}
> > -
> > /*
> > * the three functions below implement chip->read_byte(), chip->read_buf()
> > * and chip->write_buf() respectively. these aren't used for
> > @@ -2757,16 +2744,6 @@ static int qcom_nand_host_init(struct qcom_nand_controller *nandc,
> > chip->set_features = nand_get_set_features_notsupp;
> > chip->get_features = nand_get_set_features_notsupp;
> >
> > - /*
> > - * the bad block marker is writable only when we write the last codeword
> > - * of a page with ECC disabled. currently, the nand_base and nand_bbt
> > - * helpers don't allow us to write BB from a nand chip with ECC
> > - * disabled (MTD_OPS_PLACE_OOB is set by default). use the block_markbad
> > - * helpers until we permanently switch to using
> > - * MTD_OPS_RAW for all drivers (with the help of badblockbits)
> > - */
> > - chip->block_markbad = qcom_nandc_block_markbad;
> > -
> > chip->controller = &nandc->controller;
> > chip->options |= NAND_NO_SUBPAGE_WRITE | NAND_USE_BOUNCE_BUFFER |
> > NAND_SKIP_BBTSCAN;
>
>
>
On Fri, 6 Jul 2018 13:21:59 +0530
Abhishek Sahu <[email protected]> wrote:
> Driver does not send the commands to NAND device for page
> read/write operations in ->cmdfunc(). It just does some
> minor variable initialization and rest of the things
> are being done in actual ->read/write_oob[_raw].
>
> The generic helper function calls for invoking commands during
> page read/write are making this driver complicated. For QCOM NAND
> driver, ->cmdfunc() does minor part of initialization and rest of
> the initialization is performed by actual page read/write
> functions. Also, ->read/write_oob() does not calls helper
> function and all the initialization is being done in
> ->read/write_oob() itself.
This sounds hazardous in the long run. Some vendor specific commands
are re-using the READ0/READSTART semantic to read particular
registers/OTP sections programmed at flash production time. For these
operations, we don't want to go through the regular
chip->ecc.read_page[_raw]() hooks, and instead use
->cmdfunc()/->exec_op(). You probably don't have setups with such
NANDs yet, but that might be the case at some point.
As already suggested by Miqule, I strongly recommend that you work on
supporting ->exec_op() instead of trying to clean things up prematurely.
>
> Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to
> explicitly send READ/PROG commands")', sending of commands has
> been moved to driver for page read/write, so this patch does
> following changes to make code more readable:
>
> 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op()
> helper functions which helps in removing code duplication in each
> operation.
>
> 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND
> controller waits for BUSY signal to be de asserted and
> automatically issues the READ STATUS command. Currently, driver
> is storing this status privately and returns the same when status
> command comes from helper function after program/erase operation.
> Now, for write operations, the status can be returned from main
> function itself, so storing status can be removed for program
> operations.
>
> Signed-off-by: Abhishek Sahu <[email protected]>
> ---
> drivers/mtd/nand/raw/qcom_nandc.c | 222 ++++++++++++++++----------------------
> 1 file changed, 91 insertions(+), 131 deletions(-)
>
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index 6fb85d3..f73ee0e 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host *host, int command)
> host->last_command = command;
>
> clear_read_regs(nandc);
> -
> - if (command == NAND_CMD_RESET || command == NAND_CMD_READID ||
> - command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1)
> - clear_bam_transaction(nandc);
> + clear_bam_transaction(nandc);
> }
>
> /*
> - * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set our
> - * privately maintained status byte, this status byte can be read after
> - * NAND_CMD_STATUS is called
> + * QCOM NAND controller by default issues READ STATUS command after PROGRAM
> + * PAGE/BLOCK ERASE operation and updates the same in its internal status
> + * register for last codeword. This function parses status for each CW and
> + * return actual status byte for write/erase operation.
> */
> -static void parse_erase_write_errors(struct qcom_nand_host *host, int command)
> +static u8 parse_erase_write_errors(struct qcom_nand_host *host, int num_cw)
> {
> struct nand_chip *chip = &host->chip;
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> - struct nand_ecc_ctrl *ecc = &chip->ecc;
> - int num_cw;
> int i;
> + u8 status = 0;
>
> - num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1;
> nandc_read_buffer_sync(nandc, true);
>
> for (i = 0; i < num_cw; i++) {
> u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]);
>
> if (flash_status & FS_MPU_ERR)
> - host->status &= ~NAND_STATUS_WP;
> + status &= ~NAND_STATUS_WP;
>
> if (flash_status & FS_OP_ERR || (i == (num_cw - 1) &&
> (flash_status &
> FS_DEVICE_STS_ERR)))
> - host->status |= NAND_STATUS_FAIL;
> + status |= NAND_STATUS_FAIL;
> }
> +
> + return status;
> }
>
> static void post_command(struct qcom_nand_host *host, int command)
> @@ -1428,9 +1426,12 @@ static void post_command(struct qcom_nand_host *host, int command)
> memcpy(nandc->data_buffer, nandc->reg_read_buf,
> nandc->buf_count);
> break;
> - case NAND_CMD_PAGEPROG:
> case NAND_CMD_ERASE1:
> - parse_erase_write_errors(host, command);
> + /*
> + * update privately maintained status byte, this status byte can
> + * be read after NAND_CMD_STATUS is called.
> + */
> + host->status = parse_erase_write_errors(host, 1);
> break;
> default:
> break;
> @@ -1448,7 +1449,6 @@ static void qcom_nandc_command(struct mtd_info *mtd, unsigned int command,
> {
> struct nand_chip *chip = mtd_to_nand(mtd);
> struct qcom_nand_host *host = to_qcom_nand_host(chip);
> - struct nand_ecc_ctrl *ecc = &chip->ecc;
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> bool wait = false;
> int ret = 0;
> @@ -1477,23 +1477,6 @@ static void qcom_nandc_command(struct mtd_info *mtd, unsigned int command,
> wait = true;
> break;
>
> - case NAND_CMD_READ0:
> - /* we read the entire page for now */
> - WARN_ON(column != 0);
> -
> - host->use_ecc = true;
> - set_address(host, 0, page_addr);
> - update_rw_regs(host, ecc->steps, true);
> - break;
> -
> - case NAND_CMD_SEQIN:
> - WARN_ON(column != 0);
> - set_address(host, 0, page_addr);
> - break;
> -
> - case NAND_CMD_PAGEPROG:
> - case NAND_CMD_STATUS:
> - case NAND_CMD_NONE:
> default:
> break;
> }
> @@ -1589,6 +1572,61 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
> return 0;
> }
>
> +/* check if page write is successful */
> +static int check_write_errors(struct qcom_nand_host *host, int cw_cnt)
> +{
> + u8 status = parse_erase_write_errors(host, cw_cnt);
> +
> + return (status & NAND_STATUS_FAIL) ? -EIO : 0;
> +}
> +
> +/* performs the common init for page read/write operations */
> +static void
> +qcom_nand_init_page_op(struct qcom_nand_host *host, int num_cw, int page,
> + u16 addr, bool read)
> +{
> + struct nand_chip *chip = &host->chip;
> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> +
> + clear_read_regs(nandc);
> + clear_bam_transaction(nandc);
> + set_address(host, addr, page);
> + update_rw_regs(host, num_cw, read);
> + if (read)
> + config_nand_page_read(nandc);
> + else
> + config_nand_page_write(nandc);
> +}
> +
> +/*
> + * Performs the page operation by submitting DMA descriptors and checks
> + * the errors. For read with ecc, the read function needs to do erased
> + * page detection so skips the error check.
> + */
> +static int
> +qcom_nand_start_page_op(struct qcom_nand_host *host, int num_cw, bool read)
> +{
> + struct nand_chip *chip = &host->chip;
> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> + int ret;
> +
> + ret = submit_descs(nandc);
> + free_descs(nandc);
> + if (ret) {
> + dev_err(nandc->dev, "%s operation failure\n",
> + read ? "READ" : "WRITE");
> + return ret;
> + }
> +
> + if (!read)
> + return check_write_errors(host, num_cw);
> +
> + if (!host->use_ecc)
> + return check_flash_errors(host, num_cw);
> +
> + return 0;
> +}
> +
> /* performs raw read for one codeword */
> static int
> qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
> @@ -1598,15 +1636,10 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> int data_size1, data_size2, oob_size1, oob_size2;
> - int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
> + int reg_off = FLASH_BUF_ACC, read_loc = 0;
>
> - nand_read_page_op(chip, page, 0, NULL, 0);
> host->use_ecc = false;
> -
> - clear_bam_transaction(nandc);
> - set_address(host, host->cw_size * cw, page);
> - update_rw_regs(host, 1, true);
> - config_nand_page_read(nandc);
> + qcom_nand_init_page_op(host, 1, page, host->cw_size * cw, true);
>
> data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
> oob_size1 = host->bbm_size;
> @@ -1647,14 +1680,7 @@ static int check_flash_errors(struct qcom_nand_host *host, int cw_cnt)
>
> read_data_dma(nandc, reg_off, oob_buf + oob_size1, oob_size2, 0);
>
> - ret = submit_descs(nandc);
> - free_descs(nandc);
> - if (ret) {
> - dev_err(nandc->dev, "failure to read raw cw %d\n", cw);
> - return ret;
> - }
> -
> - return check_flash_errors(host, 1);
> + return qcom_nand_start_page_op(host, 1, true);
> }
>
> /*
> @@ -1857,7 +1883,8 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
> u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;
> int i, ret;
>
> - config_nand_page_read(nandc);
> + host->use_ecc = true;
> + qcom_nand_init_page_op(host, ecc->steps, page, 0, true);
>
> /* queue cmd descs for each codeword */
> for (i = 0; i < ecc->steps; i++) {
> @@ -1914,13 +1941,9 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
> oob_buf += oob_size;
> }
>
> - ret = submit_descs(nandc);
> - free_descs(nandc);
> -
> - if (ret) {
> - dev_err(nandc->dev, "failure to read page/oob\n");
> + ret = qcom_nand_start_page_op(host, ecc->steps, true);
> + if (ret)
> return ret;
> - }
>
> return parse_read_errors(host, data_buf_start, oob_buf_start, page);
> }
> @@ -1929,17 +1952,8 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
> static int qcom_nandc_read_page(struct mtd_info *mtd, struct nand_chip *chip,
> uint8_t *buf, int oob_required, int page)
> {
> - struct qcom_nand_host *host = to_qcom_nand_host(chip);
> - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> - u8 *data_buf, *oob_buf = NULL;
> -
> - nand_read_page_op(chip, page, 0, NULL, 0);
> - data_buf = buf;
> - oob_buf = oob_required ? chip->oob_poi : NULL;
> -
> - clear_bam_transaction(nandc);
> -
> - return read_page_ecc(host, data_buf, oob_buf, page);
> + return read_page_ecc(to_qcom_nand_host(chip), buf,
> + oob_required ? chip->oob_poi : NULL, page);
> }
>
> /* implements ecc->read_page_raw() */
> @@ -1969,18 +1983,8 @@ static int qcom_nandc_read_page_raw(struct mtd_info *mtd,
> static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip *chip,
> int page)
> {
> - struct qcom_nand_host *host = to_qcom_nand_host(chip);
> - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> - struct nand_ecc_ctrl *ecc = &chip->ecc;
> -
> - clear_read_regs(nandc);
> - clear_bam_transaction(nandc);
> -
> - host->use_ecc = true;
> - set_address(host, 0, page);
> - update_rw_regs(host, ecc->steps, true);
> -
> - return read_page_ecc(host, NULL, chip->oob_poi, page);
> + return read_page_ecc(to_qcom_nand_host(chip), NULL,
> + chip->oob_poi, page);
> }
>
> /* implements ecc->write_page() */
> @@ -1991,19 +1995,12 @@ static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> u8 *data_buf, *oob_buf;
> - int i, ret;
> -
> - nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> -
> - clear_read_regs(nandc);
> - clear_bam_transaction(nandc);
> + int i;
>
> data_buf = (u8 *)buf;
> oob_buf = chip->oob_poi;
> -
> host->use_ecc = true;
> - update_rw_regs(host, ecc->steps, false);
> - config_nand_page_write(nandc);
> + qcom_nand_init_page_op(host, ecc->steps, page, 0, false);
>
> for (i = 0; i < ecc->steps; i++) {
> int data_size, oob_size;
> @@ -2041,16 +2038,7 @@ static int qcom_nandc_write_page(struct mtd_info *mtd, struct nand_chip *chip,
> oob_buf += oob_size;
> }
>
> - ret = submit_descs(nandc);
> - if (ret)
> - dev_err(nandc->dev, "failure to write page\n");
> -
> - free_descs(nandc);
> -
> - if (!ret)
> - ret = nand_prog_page_end_op(chip);
> -
> - return ret;
> + return qcom_nand_start_page_op(host, ecc->steps, false);
> }
>
> /* implements ecc->write_page_raw() */
> @@ -2062,18 +2050,13 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,
> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> u8 *data_buf, *oob_buf;
> - int i, ret;
> -
> - nand_prog_page_begin_op(chip, page, 0, NULL, 0);
> - clear_read_regs(nandc);
> - clear_bam_transaction(nandc);
> + int i;
>
> data_buf = (u8 *)buf;
> oob_buf = chip->oob_poi;
>
> host->use_ecc = false;
> - update_rw_regs(host, ecc->steps, false);
> - config_nand_page_write(nandc);
> + qcom_nand_init_page_op(host, ecc->steps, page, 0, false);
>
> for (i = 0; i < ecc->steps; i++) {
> int data_size1, data_size2, oob_size1, oob_size2;
> @@ -2113,16 +2096,7 @@ static int qcom_nandc_write_page_raw(struct mtd_info *mtd,
> config_nand_cw_write(nandc);
> }
>
> - ret = submit_descs(nandc);
> - if (ret)
> - dev_err(nandc->dev, "failure to write raw page\n");
> -
> - free_descs(nandc);
> -
> - if (!ret)
> - ret = nand_prog_page_end_op(chip);
> -
> - return ret;
> + return qcom_nand_start_page_op(host, ecc->steps, false);
> }
>
> /*
> @@ -2140,9 +2114,6 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> struct nand_ecc_ctrl *ecc = &chip->ecc;
> u8 *oob = chip->oob_poi, bbm_byte;
> int data_size, oob_size, bbm_offset, write_size;
> - int ret;
> -
> - clear_bam_transaction(nandc);
>
> /*
> * The NAND base layer calls ecc->write_oob() by setting bytes at
> @@ -2183,24 +2154,13 @@ static int qcom_nandc_write_oob(struct mtd_info *mtd, struct nand_chip *chip,
> write_size = data_size + oob_size;
> }
>
> - set_address(host, host->cw_size * (ecc->steps - 1), page);
> - update_rw_regs(host, 1, false);
> -
> - config_nand_page_write(nandc);
> + qcom_nand_init_page_op(host, 1, page,
> + host->cw_size * (ecc->steps - 1), false);
> write_data_dma(nandc, FLASH_BUF_ACC,
> nandc->data_buffer, write_size, 0);
> config_nand_cw_write(nandc);
>
> - ret = submit_descs(nandc);
> -
> - free_descs(nandc);
> -
> - if (ret) {
> - dev_err(nandc->dev, "failure to write oob\n");
> - return -EIO;
> - }
> -
> - return nand_prog_page_end_op(chip);
> + return qcom_nand_start_page_op(host, 1, false);
> }
>
> /*
On 2018-07-19 03:12, Miquel Raynal wrote:
> Abhishek,
>
> Miquel Raynal <[email protected]> wrote on Wed, 18 Jul 2018
> 23:41:44 +0200:
>
>> Hi Boris,
>>
>> Boris Brezillon <[email protected]> wrote on Wed, 18 Jul
>> 2018
>> 23:36:37 +0200:
>>
>> > On Wed, 18 Jul 2018 23:15:26 +0200
>> > Miquel Raynal <[email protected]> wrote:
>> >
>> > > Hi Abhishek,
>> > >
>> > > Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
>> > > 13:21:58 +0530:
>> > >
>> > > > Remove the NAND_SKIP_BBTSCAN to use RAM based BBT.
>> > >
>> > > Unless I am understanding it the wrong way, NAND_SKIP_BBTSCAN will skip
>> > > the scan of the on-chip BBT and will scan every block to construct a
>> > > RAM, based BBT thanks to the BBM.
>> > >
>> > > So flash based BBT is already unused and removing this flag is a
>> > > mistake, right?
>> >
>> > ->scan_bbt() is also taking care of building the in-RAM BBT based on
>> > BBM when no on-flash BBT is provided, so I think it's the right thing
>> > to do.
>>
>> Oh right. Then doing so is harmless.
>
> Could you please update the commit log to reflect this aspect?
>
Thanks Miquel and Boris.
I will update the commit log.
Regards,
Abhishek
Hi Boris,
On 2018-07-19 03:13, Boris Brezillon wrote:
> On Wed, 18 Jul 2018 23:23:50 +0200
> Miquel Raynal <[email protected]> wrote:
>
>> Boris,
>>
>> Can you please check the change in qcom_nandc_write_oob() is
>> valid? I think it is but as this is a bit of a hack I prefer double
>> checking.
>
> Indeed, it's hack-ish.
>
>>
>> Thanks,
>> Miquèl
>>
>>
>> Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
>> 13:21:56 +0530:
>>
>> > The NAND base layer calls write_oob() by setting bytes at
>> > chip->badblockpos with value non 0xFF for updating bad block status.
>> > The QCOM NAND controller skips the bad block bytes while doing normal
>> > write with ECC enabled. When initial support for this driver was
>> > added, the driver specific function was added temporarily for
>> > block_markbad() with assumption to change for raw read in NAND base
>> > layer. Moving to raw read for block_markbad() seems to take more time
>> > so this patch removes driver specific block_markbad() function by
>> > using following HACK in write_oob() function.
>> >
>> > Check for BBM bytes in OOB and accordingly do raw write for updating
>> > BBM bytes in NAND flash or normal write for updating available OOB
>> > bytes.
>
> Why don't we change that instead of patching the qcom driver to guess
> when the core tries to mark a block bad? If you're afraid of breaking
> existing drivers that might rely on the "write/read BBM in non-raw
> mode" solution (I'm sure some drivers are), you can always add a new
> flag in chip->options (NAND_ACCESS_BBM_IN_RAW_MODE) and only use raw
> accessors when this flag is set.
>
We started with that Only
http://patchwork.ozlabs.org/patch/508565/
and since we didn't conclude, we went for driver
specific bad block check and mark bad block functions.
Now, we wanted to get rid of driver specific functions
1. For bad block check, we found the way to get the BBM bytes
with ECC read. Controller updates BBM in separate register
which we can read and update the same in OOB. Patch #1 of
series does the same.
2. For bad block mark, there is no way to update in ECC mode
that's why we went for HACK to get rid of driver specific
function.
If adding flag is fine now then this HACK won't be required.
Moving to RAW mode for every one still looks risky.
Thanks,
Abhishek
On Fri, 20 Jul 2018 17:46:38 +0530
Abhishek Sahu <[email protected]> wrote:
> Hi Boris,
>
> On 2018-07-19 03:13, Boris Brezillon wrote:
> > On Wed, 18 Jul 2018 23:23:50 +0200
> > Miquel Raynal <[email protected]> wrote:
> >
> >> Boris,
> >>
> >> Can you please check the change in qcom_nandc_write_oob() is
> >> valid? I think it is but as this is a bit of a hack I prefer double
> >> checking.
> >
> > Indeed, it's hack-ish.
> >
> >>
> >> Thanks,
> >> Miquèl
> >>
> >>
> >> Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
> >> 13:21:56 +0530:
> >>
> >> > The NAND base layer calls write_oob() by setting bytes at
> >> > chip->badblockpos with value non 0xFF for updating bad block status.
> >> > The QCOM NAND controller skips the bad block bytes while doing normal
> >> > write with ECC enabled. When initial support for this driver was
> >> > added, the driver specific function was added temporarily for
> >> > block_markbad() with assumption to change for raw read in NAND base
> >> > layer. Moving to raw read for block_markbad() seems to take more time
> >> > so this patch removes driver specific block_markbad() function by
> >> > using following HACK in write_oob() function.
> >> >
> >> > Check for BBM bytes in OOB and accordingly do raw write for updating
> >> > BBM bytes in NAND flash or normal write for updating available OOB
> >> > bytes.
> >
> > Why don't we change that instead of patching the qcom driver to guess
> > when the core tries to mark a block bad? If you're afraid of breaking
> > existing drivers that might rely on the "write/read BBM in non-raw
> > mode" solution (I'm sure some drivers are), you can always add a new
> > flag in chip->options (NAND_ACCESS_BBM_IN_RAW_MODE) and only use raw
> > accessors when this flag is set.
> >
>
> We started with that Only
>
> http://patchwork.ozlabs.org/patch/508565/
>
> and since we didn't conclude, we went for driver
> specific bad block check and mark bad block functions.
>
> Now, we wanted to get rid of driver specific functions
>
> 1. For bad block check, we found the way to get the BBM bytes
> with ECC read. Controller updates BBM in separate register
> which we can read and update the same in OOB. Patch #1 of
> series does the same.
>
> 2. For bad block mark, there is no way to update in ECC mode
> that's why we went for HACK to get rid of driver specific
> function.
>
> If adding flag is fine now then this HACK won't be required.
Yep. I'm fine with that. Can you rebase the patch you pointed out on top
of nand/next and move the flag to chip->options instead of
chip->bbt_options + prefix it with NAND_ instead of NAND_BBT_?
> Moving to RAW mode for every one still looks risky.
I agree.
On 2018-07-19 03:24, Boris Brezillon wrote:
> On Fri, 6 Jul 2018 13:21:59 +0530
> Abhishek Sahu <[email protected]> wrote:
>
>> Driver does not send the commands to NAND device for page
>> read/write operations in ->cmdfunc(). It just does some
>> minor variable initialization and rest of the things
>> are being done in actual ->read/write_oob[_raw].
>>
>> The generic helper function calls for invoking commands during
>> page read/write are making this driver complicated. For QCOM NAND
>> driver, ->cmdfunc() does minor part of initialization and rest of
>> the initialization is performed by actual page read/write
>> functions. Also, ->read/write_oob() does not calls helper
>> function and all the initialization is being done in
>> ->read/write_oob() itself.
>
> This sounds hazardous in the long run. Some vendor specific commands
> are re-using the READ0/READSTART semantic to read particular
> registers/OTP sections programmed at flash production time. For these
> operations, we don't want to go through the regular
> chip->ecc.read_page[_raw]() hooks, and instead use
> ->cmdfunc()/->exec_op(). You probably don't have setups with such
> NANDs yet, but that might be the case at some point.
>
Thanks Boris and Miquel for pointing this out.
Yes. We don't have that setup yet and the current driver code
also can't handle that condition since the command function was
not doing anything in this regard.
Since the current driver code was complicated with lot of
code duplication, so this patch was raised to cleanup few things.
> As already suggested by Miqule, I strongly recommend that you work on
> supporting ->exec_op() instead of trying to clean things up
> prematurely.
>
I did some analysis and it seems moving to ->exec_op() will help in
long run. I will start working on that but it will take some
time for us since It requires major code changes with lot of
testing. I will update you when we have RFC patch ready for this.
Meanwhile, I will address review comments in other patches of this
series and will post v2 after removing this patch from patch series.
Regards,
Abhishek
>>
>> Since after 'commit 25f815f66a14 ("mtd: nand: force drivers to
>> explicitly send READ/PROG commands")', sending of commands has
>> been moved to driver for page read/write, so this patch does
>> following changes to make code more readable:
>>
>> 1. Introduce qcom_nand_init_page_op() and qcom_nand_start_page_op()
>> helper functions which helps in removing code duplication in each
>> operation.
>>
>> 2. After issuing PROGRAM PAGE/BLOCK ERASE, QCOM NAND
>> controller waits for BUSY signal to be de asserted and
>> automatically issues the READ STATUS command. Currently, driver
>> is storing this status privately and returns the same when status
>> command comes from helper function after program/erase operation.
>> Now, for write operations, the status can be returned from main
>> function itself, so storing status can be removed for program
>> operations.
>>
>> Signed-off-by: Abhishek Sahu <[email protected]>
>> ---
>> drivers/mtd/nand/raw/qcom_nandc.c | 222
>> ++++++++++++++++----------------------
>> 1 file changed, 91 insertions(+), 131 deletions(-)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c
>> b/drivers/mtd/nand/raw/qcom_nandc.c
>> index 6fb85d3..f73ee0e 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -1382,39 +1382,37 @@ static void pre_command(struct qcom_nand_host
>> *host, int command)
>> host->last_command = command;
>>
>> clear_read_regs(nandc);
>> -
>> - if (command == NAND_CMD_RESET || command == NAND_CMD_READID ||
>> - command == NAND_CMD_PARAM || command == NAND_CMD_ERASE1)
>> - clear_bam_transaction(nandc);
>> + clear_bam_transaction(nandc);
>> }
>>
>> /*
>> - * this is called after NAND_CMD_PAGEPROG and NAND_CMD_ERASE1 to set
>> our
>> - * privately maintained status byte, this status byte can be read
>> after
>> - * NAND_CMD_STATUS is called
>> + * QCOM NAND controller by default issues READ STATUS command after
>> PROGRAM
>> + * PAGE/BLOCK ERASE operation and updates the same in its internal
>> status
>> + * register for last codeword. This function parses status for each
>> CW and
>> + * return actual status byte for write/erase operation.
>> */
>> -static void parse_erase_write_errors(struct qcom_nand_host *host, int
>> command)
>> +static u8 parse_erase_write_errors(struct qcom_nand_host *host, int
>> num_cw)
>> {
>> struct nand_chip *chip = &host->chip;
>> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> - struct nand_ecc_ctrl *ecc = &chip->ecc;
>> - int num_cw;
>> int i;
>> + u8 status = 0;
>>
>> - num_cw = command == NAND_CMD_PAGEPROG ? ecc->steps : 1;
>> nandc_read_buffer_sync(nandc, true);
>>
>> for (i = 0; i < num_cw; i++) {
>> u32 flash_status = le32_to_cpu(nandc->reg_read_buf[i]);
>>
>> if (flash_status & FS_MPU_ERR)
>> - host->status &= ~NAND_STATUS_WP;
>> + status &= ~NAND_STATUS_WP;
>>
>> if (flash_status & FS_OP_ERR || (i == (num_cw - 1) &&
>> (flash_status &
>> FS_DEVICE_STS_ERR)))
>> - host->status |= NAND_STATUS_FAIL;
>> + status |= NAND_STATUS_FAIL;
>> }
>> +
>> + return status;
>> }
>>
>> static void post_command(struct qcom_nand_host *host, int command)
>> @@ -1428,9 +1426,12 @@ static void post_command(struct qcom_nand_host
>> *host, int command)
>> memcpy(nandc->data_buffer, nandc->reg_read_buf,
>> nandc->buf_count);
>> break;
>> - case NAND_CMD_PAGEPROG:
>> case NAND_CMD_ERASE1:
>> - parse_erase_write_errors(host, command);
>> + /*
>> + * update privately maintained status byte, this status byte can
>> + * be read after NAND_CMD_STATUS is called.
>> + */
>> + host->status = parse_erase_write_errors(host, 1);
>> break;
>> default:
>> break;
>> @@ -1448,7 +1449,6 @@ static void qcom_nandc_command(struct mtd_info
>> *mtd, unsigned int command,
>> {
>> struct nand_chip *chip = mtd_to_nand(mtd);
>> struct qcom_nand_host *host = to_qcom_nand_host(chip);
>> - struct nand_ecc_ctrl *ecc = &chip->ecc;
>> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> bool wait = false;
>> int ret = 0;
>> @@ -1477,23 +1477,6 @@ static void qcom_nandc_command(struct mtd_info
>> *mtd, unsigned int command,
>> wait = true;
>> break;
>>
>> - case NAND_CMD_READ0:
>> - /* we read the entire page for now */
>> - WARN_ON(column != 0);
>> -
>> - host->use_ecc = true;
>> - set_address(host, 0, page_addr);
>> - update_rw_regs(host, ecc->steps, true);
>> - break;
>> -
>> - case NAND_CMD_SEQIN:
>> - WARN_ON(column != 0);
>> - set_address(host, 0, page_addr);
>> - break;
>> -
>> - case NAND_CMD_PAGEPROG:
>> - case NAND_CMD_STATUS:
>> - case NAND_CMD_NONE:
>> default:
>> break;
>> }
>> @@ -1589,6 +1572,61 @@ static int check_flash_errors(struct
>> qcom_nand_host *host, int cw_cnt)
>> return 0;
>> }
>>
>> +/* check if page write is successful */
>> +static int check_write_errors(struct qcom_nand_host *host, int
>> cw_cnt)
>> +{
>> + u8 status = parse_erase_write_errors(host, cw_cnt);
>> +
>> + return (status & NAND_STATUS_FAIL) ? -EIO : 0;
>> +}
>> +
>> +/* performs the common init for page read/write operations */
>> +static void
>> +qcom_nand_init_page_op(struct qcom_nand_host *host, int num_cw, int
>> page,
>> + u16 addr, bool read)
>> +{
>> + struct nand_chip *chip = &host->chip;
>> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> +
>> + clear_read_regs(nandc);
>> + clear_bam_transaction(nandc);
>> + set_address(host, addr, page);
>> + update_rw_regs(host, num_cw, read);
>> + if (read)
>> + config_nand_page_read(nandc);
>> + else
>> + config_nand_page_write(nandc);
>> +}
>> +
>> +/*
>> + * Performs the page operation by submitting DMA descriptors and
>> checks
>> + * the errors. For read with ecc, the read function needs to do
>> erased
>> + * page detection so skips the error check.
>> + */
>> +static int
>> +qcom_nand_start_page_op(struct qcom_nand_host *host, int num_cw, bool
>> read)
>> +{
>> + struct nand_chip *chip = &host->chip;
>> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> + int ret;
>> +
>> + ret = submit_descs(nandc);
>> + free_descs(nandc);
>> + if (ret) {
>> + dev_err(nandc->dev, "%s operation failure\n",
>> + read ? "READ" : "WRITE");
>> + return ret;
>> + }
>> +
>> + if (!read)
>> + return check_write_errors(host, num_cw);
>> +
>> + if (!host->use_ecc)
>> + return check_flash_errors(host, num_cw);
>> +
>> + return 0;
>> +}
>> +
>> /* performs raw read for one codeword */
>> static int
>> qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
>> @@ -1598,15 +1636,10 @@ static int check_flash_errors(struct
>> qcom_nand_host *host, int cw_cnt)
>> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> struct nand_ecc_ctrl *ecc = &chip->ecc;
>> int data_size1, data_size2, oob_size1, oob_size2;
>> - int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
>> + int reg_off = FLASH_BUF_ACC, read_loc = 0;
>>
>> - nand_read_page_op(chip, page, 0, NULL, 0);
>> host->use_ecc = false;
>> -
>> - clear_bam_transaction(nandc);
>> - set_address(host, host->cw_size * cw, page);
>> - update_rw_regs(host, 1, true);
>> - config_nand_page_read(nandc);
>> + qcom_nand_init_page_op(host, 1, page, host->cw_size * cw, true);
>>
>> data_size1 = mtd->writesize - host->cw_size * (ecc->steps - 1);
>> oob_size1 = host->bbm_size;
>> @@ -1647,14 +1680,7 @@ static int check_flash_errors(struct
>> qcom_nand_host *host, int cw_cnt)
>>
>> read_data_dma(nandc, reg_off, oob_buf + oob_size1, oob_size2, 0);
>>
>> - ret = submit_descs(nandc);
>> - free_descs(nandc);
>> - if (ret) {
>> - dev_err(nandc->dev, "failure to read raw cw %d\n", cw);
>> - return ret;
>> - }
>> -
>> - return check_flash_errors(host, 1);
>> + return qcom_nand_start_page_op(host, 1, true);
>> }
>>
>> /*
>> @@ -1857,7 +1883,8 @@ static int read_page_ecc(struct qcom_nand_host
>> *host, u8 *data_buf,
>> u8 *data_buf_start = data_buf, *oob_buf_start = oob_buf;
>> int i, ret;
>>
>> - config_nand_page_read(nandc);
>> + host->use_ecc = true;
>> + qcom_nand_init_page_op(host, ecc->steps, page, 0, true);
>>
>> /* queue cmd descs for each codeword */
>> for (i = 0; i < ecc->steps; i++) {
>> @@ -1914,13 +1941,9 @@ static int read_page_ecc(struct qcom_nand_host
>> *host, u8 *data_buf,
>> oob_buf += oob_size;
>> }
>>
>> - ret = submit_descs(nandc);
>> - free_descs(nandc);
>> -
>> - if (ret) {
>> - dev_err(nandc->dev, "failure to read page/oob\n");
>> + ret = qcom_nand_start_page_op(host, ecc->steps, true);
>> + if (ret)
>> return ret;
>> - }
>>
>> return parse_read_errors(host, data_buf_start, oob_buf_start, page);
>> }
>> @@ -1929,17 +1952,8 @@ static int read_page_ecc(struct qcom_nand_host
>> *host, u8 *data_buf,
>> static int qcom_nandc_read_page(struct mtd_info *mtd, struct
>> nand_chip *chip,
>> uint8_t *buf, int oob_required, int page)
>> {
>> - struct qcom_nand_host *host = to_qcom_nand_host(chip);
>> - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> - u8 *data_buf, *oob_buf = NULL;
>> -
>> - nand_read_page_op(chip, page, 0, NULL, 0);
>> - data_buf = buf;
>> - oob_buf = oob_required ? chip->oob_poi : NULL;
>> -
>> - clear_bam_transaction(nandc);
>> -
>> - return read_page_ecc(host, data_buf, oob_buf, page);
>> + return read_page_ecc(to_qcom_nand_host(chip), buf,
>> + oob_required ? chip->oob_poi : NULL, page);
>> }
>>
>> /* implements ecc->read_page_raw() */
>> @@ -1969,18 +1983,8 @@ static int qcom_nandc_read_page_raw(struct
>> mtd_info *mtd,
>> static int qcom_nandc_read_oob(struct mtd_info *mtd, struct nand_chip
>> *chip,
>> int page)
>> {
>> - struct qcom_nand_host *host = to_qcom_nand_host(chip);
>> - struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> - struct nand_ecc_ctrl *ecc = &chip->ecc;
>> -
>> - clear_read_regs(nandc);
>> - clear_bam_transaction(nandc);
>> -
>> - host->use_ecc = true;
>> - set_address(host, 0, page);
>> - update_rw_regs(host, ecc->steps, true);
>> -
>> - return read_page_ecc(host, NULL, chip->oob_poi, page);
>> + return read_page_ecc(to_qcom_nand_host(chip), NULL,
>> + chip->oob_poi, page);
>> }
>>
>> /* implements ecc->write_page() */
>> @@ -1991,19 +1995,12 @@ static int qcom_nandc_write_page(struct
>> mtd_info *mtd, struct nand_chip *chip,
>> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> struct nand_ecc_ctrl *ecc = &chip->ecc;
>> u8 *data_buf, *oob_buf;
>> - int i, ret;
>> -
>> - nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>> -
>> - clear_read_regs(nandc);
>> - clear_bam_transaction(nandc);
>> + int i;
>>
>> data_buf = (u8 *)buf;
>> oob_buf = chip->oob_poi;
>> -
>> host->use_ecc = true;
>> - update_rw_regs(host, ecc->steps, false);
>> - config_nand_page_write(nandc);
>> + qcom_nand_init_page_op(host, ecc->steps, page, 0, false);
>>
>> for (i = 0; i < ecc->steps; i++) {
>> int data_size, oob_size;
>> @@ -2041,16 +2038,7 @@ static int qcom_nandc_write_page(struct
>> mtd_info *mtd, struct nand_chip *chip,
>> oob_buf += oob_size;
>> }
>>
>> - ret = submit_descs(nandc);
>> - if (ret)
>> - dev_err(nandc->dev, "failure to write page\n");
>> -
>> - free_descs(nandc);
>> -
>> - if (!ret)
>> - ret = nand_prog_page_end_op(chip);
>> -
>> - return ret;
>> + return qcom_nand_start_page_op(host, ecc->steps, false);
>> }
>>
>> /* implements ecc->write_page_raw() */
>> @@ -2062,18 +2050,13 @@ static int qcom_nandc_write_page_raw(struct
>> mtd_info *mtd,
>> struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
>> struct nand_ecc_ctrl *ecc = &chip->ecc;
>> u8 *data_buf, *oob_buf;
>> - int i, ret;
>> -
>> - nand_prog_page_begin_op(chip, page, 0, NULL, 0);
>> - clear_read_regs(nandc);
>> - clear_bam_transaction(nandc);
>> + int i;
>>
>> data_buf = (u8 *)buf;
>> oob_buf = chip->oob_poi;
>>
>> host->use_ecc = false;
>> - update_rw_regs(host, ecc->steps, false);
>> - config_nand_page_write(nandc);
>> + qcom_nand_init_page_op(host, ecc->steps, page, 0, false);
>>
>> for (i = 0; i < ecc->steps; i++) {
>> int data_size1, data_size2, oob_size1, oob_size2;
>> @@ -2113,16 +2096,7 @@ static int qcom_nandc_write_page_raw(struct
>> mtd_info *mtd,
>> config_nand_cw_write(nandc);
>> }
>>
>> - ret = submit_descs(nandc);
>> - if (ret)
>> - dev_err(nandc->dev, "failure to write raw page\n");
>> -
>> - free_descs(nandc);
>> -
>> - if (!ret)
>> - ret = nand_prog_page_end_op(chip);
>> -
>> - return ret;
>> + return qcom_nand_start_page_op(host, ecc->steps, false);
>> }
>>
>> /*
>> @@ -2140,9 +2114,6 @@ static int qcom_nandc_write_oob(struct mtd_info
>> *mtd, struct nand_chip *chip,
>> struct nand_ecc_ctrl *ecc = &chip->ecc;
>> u8 *oob = chip->oob_poi, bbm_byte;
>> int data_size, oob_size, bbm_offset, write_size;
>> - int ret;
>> -
>> - clear_bam_transaction(nandc);
>>
>> /*
>> * The NAND base layer calls ecc->write_oob() by setting bytes at
>> @@ -2183,24 +2154,13 @@ static int qcom_nandc_write_oob(struct
>> mtd_info *mtd, struct nand_chip *chip,
>> write_size = data_size + oob_size;
>> }
>>
>> - set_address(host, host->cw_size * (ecc->steps - 1), page);
>> - update_rw_regs(host, 1, false);
>> -
>> - config_nand_page_write(nandc);
>> + qcom_nand_init_page_op(host, 1, page,
>> + host->cw_size * (ecc->steps - 1), false);
>> write_data_dma(nandc, FLASH_BUF_ACC,
>> nandc->data_buffer, write_size, 0);
>> config_nand_cw_write(nandc);
>>
>> - ret = submit_descs(nandc);
>> -
>> - free_descs(nandc);
>> -
>> - if (ret) {
>> - dev_err(nandc->dev, "failure to write oob\n");
>> - return -EIO;
>> - }
>> -
>> - return nand_prog_page_end_op(chip);
>> + return qcom_nand_start_page_op(host, 1, false);
>> }
>>
>> /*
Hi Abhishek,
On Fri, 20 Jul 2018 15:03:48 +0200
Boris Brezillon <[email protected]> wrote:
> On Fri, 20 Jul 2018 17:46:38 +0530
> Abhishek Sahu <[email protected]> wrote:
>
> > Hi Boris,
> >
> > On 2018-07-19 03:13, Boris Brezillon wrote:
> > > On Wed, 18 Jul 2018 23:23:50 +0200
> > > Miquel Raynal <[email protected]> wrote:
> > >
> > >> Boris,
> > >>
> > >> Can you please check the change in qcom_nandc_write_oob() is
> > >> valid? I think it is but as this is a bit of a hack I prefer double
> > >> checking.
> > >
> > > Indeed, it's hack-ish.
> > >
> > >>
> > >> Thanks,
> > >> Miquèl
> > >>
> > >>
> > >> Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
> > >> 13:21:56 +0530:
> > >>
> > >> > The NAND base layer calls write_oob() by setting bytes at
> > >> > chip->badblockpos with value non 0xFF for updating bad block status.
> > >> > The QCOM NAND controller skips the bad block bytes while doing normal
> > >> > write with ECC enabled. When initial support for this driver was
> > >> > added, the driver specific function was added temporarily for
> > >> > block_markbad() with assumption to change for raw read in NAND base
> > >> > layer. Moving to raw read for block_markbad() seems to take more time
> > >> > so this patch removes driver specific block_markbad() function by
> > >> > using following HACK in write_oob() function.
> > >> >
> > >> > Check for BBM bytes in OOB and accordingly do raw write for updating
> > >> > BBM bytes in NAND flash or normal write for updating available OOB
> > >> > bytes.
> > >
> > > Why don't we change that instead of patching the qcom driver to guess
> > > when the core tries to mark a block bad? If you're afraid of breaking
> > > existing drivers that might rely on the "write/read BBM in non-raw
> > > mode" solution (I'm sure some drivers are), you can always add a new
> > > flag in chip->options (NAND_ACCESS_BBM_IN_RAW_MODE) and only use raw
> > > accessors when this flag is set.
> > >
> >
> > We started with that Only
> >
> > http://patchwork.ozlabs.org/patch/508565/
> >
> > and since we didn't conclude, we went for driver
> > specific bad block check and mark bad block functions.
> >
> > Now, we wanted to get rid of driver specific functions
> >
> > 1. For bad block check, we found the way to get the BBM bytes
> > with ECC read. Controller updates BBM in separate register
> > which we can read and update the same in OOB. Patch #1 of
> > series does the same.
> >
> > 2. For bad block mark, there is no way to update in ECC mode
> > that's why we went for HACK to get rid of driver specific
> > function.
> >
> > If adding flag is fine now then this HACK won't be required.
>
> Yep. I'm fine with that. Can you rebase the patch you pointed out on top
> of nand/next and move the flag to chip->options instead of
> chip->bbt_options + prefix it with NAND_ instead of NAND_BBT_?
I'm currently trying to get rid of chip->block_bad() (now placed in
chip->legacy.block_bad()), and I wanted to know if you were still
planning to submit the changes we discussed in this thread. If you
don't have time, please let me know and I'll try to do it.
Thanks,
Boris
On 2018-11-04 21:26, Boris Brezillon wrote:
> Hi Abhishek,
>
> On Fri, 20 Jul 2018 15:03:48 +0200
> Boris Brezillon <[email protected]> wrote:
>
>> On Fri, 20 Jul 2018 17:46:38 +0530
>> Abhishek Sahu <[email protected]> wrote:
>>
>> > Hi Boris,
>> >
>> > On 2018-07-19 03:13, Boris Brezillon wrote:
>> > > On Wed, 18 Jul 2018 23:23:50 +0200
>> > > Miquel Raynal <[email protected]> wrote:
>> > >
>> > >> Boris,
>> > >>
>> > >> Can you please check the change in qcom_nandc_write_oob() is
>> > >> valid? I think it is but as this is a bit of a hack I prefer double
>> > >> checking.
>> > >
>> > > Indeed, it's hack-ish.
>> > >
>> > >>
>> > >> Thanks,
>> > >> Miquèl
>> > >>
>> > >>
>> > >> Abhishek Sahu <[email protected]> wrote on Fri, 6 Jul 2018
>> > >> 13:21:56 +0530:
>> > >>
>> > >> > The NAND base layer calls write_oob() by setting bytes at
>> > >> > chip->badblockpos with value non 0xFF for updating bad block status.
>> > >> > The QCOM NAND controller skips the bad block bytes while doing normal
>> > >> > write with ECC enabled. When initial support for this driver was
>> > >> > added, the driver specific function was added temporarily for
>> > >> > block_markbad() with assumption to change for raw read in NAND base
>> > >> > layer. Moving to raw read for block_markbad() seems to take more time
>> > >> > so this patch removes driver specific block_markbad() function by
>> > >> > using following HACK in write_oob() function.
>> > >> >
>> > >> > Check for BBM bytes in OOB and accordingly do raw write for updating
>> > >> > BBM bytes in NAND flash or normal write for updating available OOB
>> > >> > bytes.
>> > >
>> > > Why don't we change that instead of patching the qcom driver to guess
>> > > when the core tries to mark a block bad? If you're afraid of breaking
>> > > existing drivers that might rely on the "write/read BBM in non-raw
>> > > mode" solution (I'm sure some drivers are), you can always add a new
>> > > flag in chip->options (NAND_ACCESS_BBM_IN_RAW_MODE) and only use raw
>> > > accessors when this flag is set.
>> > >
>> >
>> > We started with that Only
>> >
>> > http://patchwork.ozlabs.org/patch/508565/
>> >
>> > and since we didn't conclude, we went for driver
>> > specific bad block check and mark bad block functions.
>> >
>> > Now, we wanted to get rid of driver specific functions
>> >
>> > 1. For bad block check, we found the way to get the BBM bytes
>> > with ECC read. Controller updates BBM in separate register
>> > which we can read and update the same in OOB. Patch #1 of
>> > series does the same.
>> >
>> > 2. For bad block mark, there is no way to update in ECC mode
>> > that's why we went for HACK to get rid of driver specific
>> > function.
>> >
>> > If adding flag is fine now then this HACK won't be required.
>>
>> Yep. I'm fine with that. Can you rebase the patch you pointed out on
>> top
>> of nand/next and move the flag to chip->options instead of
>> chip->bbt_options + prefix it with NAND_ instead of NAND_BBT_?
>
> I'm currently trying to get rid of chip->block_bad() (now placed in
> chip->legacy.block_bad()), and I wanted to know if you were still
> planning to submit the changes we discussed in this thread. If you
> don't have time, please let me know and I'll try to do it.
>
Sorry Boris, I couldn't work on these patches.
Currently, I am working on non open source projects so
I can't submit any patches in open source till this project
completion due to legal guidelines.
If this is urgent then you can try. I will help in
QCOM related stuffs and testing.
Thanks,
Abhishek