Re: [PATCH V2] mtd: rawnand: qcom: Implement exec_op()
From: Md Sadre Alam
Date: Wed May 10 2023 - 05:02:03 EST
On 5/9/2023 4:38 PM, Manivannan Sadhasivam wrote:
On Wed, Apr 19, 2023 at 03:06:17PM +0530, Md Sadre Alam wrote:
Implement exec_op() so we can later get rid of the legacy interface
implementation.
It's good that you got the inspiration from previous exec_op() conversion
patches, but this one should've been splitted into atleast two patches as per
your patch description.
One introducing exec_op() and another getting rid of legacy interface.
Thanks Mani for reviewing. I am going to split the patches as below.
1) Introduce exec_op structure, parser, command mappping.
2) Add support for RESET, READID, STATUS exec_ops
3) Add support for READ_PARAM_PAGE exec_ops
4) Add support for ERASE, READ, WRITE exec_ops
5) Remove legacy interface
Co-developed-by: Sricharan R <quic_srichara@xxxxxxxxxxx>
Signed-off-by: Sricharan R <quic_srichara@xxxxxxxxxxx>
Signed-off-by: Md Sadre Alam <quic_mdalam@xxxxxxxxxxx>
---
Change in [V2]
* Address all Miquel comments from V1, regarding exec_ops design
* Redesign qpic nand driver with exec_ops using exec_ops parser, to
fit in exec_ops framework
drivers/mtd/nand/raw/qcom_nandc.c | 869 ++++++++++++++++++------------
1 file changed, 520 insertions(+), 349 deletions(-)
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 72d6168d8a1b..17279890454d 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -157,6 +157,7 @@
#define OP_PAGE_PROGRAM_WITH_ECC 0x7
#define OP_PROGRAM_PAGE_SPARE 0x9
#define OP_BLOCK_ERASE 0xa
+#define OP_CHECK_STATUS 0xc
#define OP_FETCH_ID 0xb
#define OP_RESET_DEVICE 0xd
@@ -235,6 +236,7 @@ nandc_set_reg(chip, reg, \
*/
#define NAND_ERASED_CW_SET BIT(4)
+#define MAX_ADDRESS_CYCLE 5
/*
* This data type corresponds to the BAM transaction which will be used for all
* NAND transfers.
@@ -432,6 +434,7 @@ struct qcom_nand_controller {
int reg_read_pos;
u32 cmd1, vld;
+ bool exec_opwrite;
Missing kdoc.
Will add in next patch
};
/*
@@ -447,6 +450,37 @@ struct qcom_nand_boot_partition {
u32 page_size;
};
+/*
+ * QCOM op for each exec_op transfer
What is "QCOM op"? Also please stick to "Qcom" everywhere.
It is exec_op ops for all instruction supported .
Will update in next patch.
+ *
+ * @addr1_reg: Address1 register value
+ *
+ * @addr2_reg: Address2 register value
+ *
+ * @cmd_reg: CMD register value
+ *
+ * @rdy_timeout_ms: wait ready timeout in ms
+ *
First letter caps for all instances.
will do in next patch
+ * @rdy_delay_ns: Additional delay in ns
+ *
+ * @data_instr_idx: data instruction index
+ *
+ * @data_instr: data instruction pointer
+ *
Do not leave this extra newline comment in-between.
will do in next patch
+ * @flag: flag for misc instruction
+ *
+ */
+struct qcom_op {
+ u32 addr1_reg;
+ u32 addr2_reg;
+ u32 cmd_reg;
+ unsigned int rdy_timeout_ms;
+ unsigned int rdy_delay_ns;
+ unsigned int data_instr_idx;
+ const struct nand_op_instr *data_instr;
Move the pointer to start of the struct.
will do in next patch.
+ u8 flag;
+};
+
/*
* NAND chip structure
*
@@ -765,7 +799,6 @@ static void set_address(struct qcom_nand_host *host, u16 column, int page)
if (chip->options & NAND_BUSWIDTH_16)
column >>= 1;
-
Sporadic?
will fix in next patch
nandc_set_reg(chip, NAND_ADDR0, page << 16 | column);
nandc_set_reg(chip, NAND_ADDR1, page >> 16 & 0xff);
}
@@ -1273,155 +1306,6 @@ static void config_nand_cw_write(struct nand_chip *chip)
write_reg_dma(nandc, NAND_READ_STATUS, 1, NAND_BAM_NEXT_SGL);
}
-/*
- * the following functions are used within chip->legacy.cmdfunc() to
- * perform different NAND_CMD_* commands
- */
-
-/* sets up descriptors for NAND_CMD_PARAM */
-static int nandc_param(struct qcom_nand_host *host)
As I pointed out above, you need to split these removals into a separate patch.
will do in next patch.
-{
- struct nand_chip *chip = &host->chip;
- struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
-
- /*
- * NAND_CMD_PARAM is called before we know much about the FLASH chip
- * in use. we configure the controller to perform a raw read of 512
- * bytes to read onfi params
- */
- if (nandc->props->qpic_v2)
- nandc_set_reg(chip, NAND_FLASH_CMD, OP_PAGE_READ_ONFI_READ |
- PAGE_ACC | LAST_PAGE);
- else
- nandc_set_reg(chip, NAND_FLASH_CMD, OP_PAGE_READ |
- PAGE_ACC | LAST_PAGE);
-
- nandc_set_reg(chip, NAND_ADDR0, 0);
- nandc_set_reg(chip, NAND_ADDR1, 0);
- nandc_set_reg(chip, NAND_DEV0_CFG0, 0 << CW_PER_PAGE
- | 512 << UD_SIZE_BYTES
- | 5 << NUM_ADDR_CYCLES
- | 0 << SPARE_SIZE_BYTES);
- nandc_set_reg(chip, NAND_DEV0_CFG1, 7 << NAND_RECOVERY_CYCLES
- | 0 << CS_ACTIVE_BSY
- | 17 << BAD_BLOCK_BYTE_NUM
- | 1 << BAD_BLOCK_IN_SPARE_AREA
- | 2 << WR_RD_BSY_GAP
- | 0 << WIDE_FLASH
- | 1 << DEV0_CFG1_ECC_DISABLE);
- if (!nandc->props->qpic_v2)
- nandc_set_reg(chip, NAND_EBI2_ECC_BUF_CFG, 1 << ECC_CFG_ECC_DISABLE);
-
- /* configure CMD1 and VLD for ONFI param probing in QPIC v1 */
- if (!nandc->props->qpic_v2) {
- nandc_set_reg(chip, NAND_DEV_CMD_VLD,
- (nandc->vld & ~READ_START_VLD));
- nandc_set_reg(chip, NAND_DEV_CMD1,
- (nandc->cmd1 & ~(0xFF << READ_ADDR))
- | NAND_CMD_PARAM << READ_ADDR);
- }
-
- nandc_set_reg(chip, NAND_EXEC_CMD, 1);
-
- if (!nandc->props->qpic_v2) {
- nandc_set_reg(chip, NAND_DEV_CMD1_RESTORE, nandc->cmd1);
- nandc_set_reg(chip, NAND_DEV_CMD_VLD_RESTORE, nandc->vld);
- }
-
- nandc_set_read_loc(chip, 0, 0, 0, 512, 1);
-
- if (!nandc->props->qpic_v2) {
- write_reg_dma(nandc, NAND_DEV_CMD_VLD, 1, 0);
- write_reg_dma(nandc, NAND_DEV_CMD1, 1, NAND_BAM_NEXT_SGL);
- }
-
- nandc->buf_count = 512;
- memset(nandc->data_buffer, 0xff, nandc->buf_count);
-
- config_nand_single_cw_page_read(chip, false, 0);
-
- read_data_dma(nandc, FLASH_BUF_ACC, nandc->data_buffer,
- nandc->buf_count, 0);
-
- /* restore CMD1 and VLD regs */
- if (!nandc->props->qpic_v2) {
- write_reg_dma(nandc, NAND_DEV_CMD1_RESTORE, 1, 0);
- write_reg_dma(nandc, NAND_DEV_CMD_VLD_RESTORE, 1, NAND_BAM_NEXT_SGL);
- }
-
- return 0;
-}
[...]
/*
* when using BCH ECC, the HW flags an error in NAND_FLASH_STATUS if it read
* an erased CW, and reports an erased CW in NAND_ERASED_CW_DETECT_STATUS.
@@ -1735,7 +1491,8 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
int ret, reg_off = FLASH_BUF_ACC, read_loc = 0;
int raw_cw = cw;
- nand_read_page_op(chip, page, 0, NULL, 0);
+ chip->cont_read.ongoing = false;
+ nand_read_page_op(chip, page, 0, data_buf, 2048);
How is this change related to exec_op() conversion? Looks like this and few
things below should be part of a separate patch.
Without this data_buf and proper length the read exec_op api is not getting called
from nand_base layer. So its needed. As i am separating patches , this change will be part
of read exec_op support patch.
host->use_ecc = false;
if (nandc->props->qpic_v2)
@@ -2047,7 +1804,9 @@ static int read_page_ecc(struct qcom_nand_host *host, u8 *data_buf,
return ret;
}
- return parse_read_errors(host, data_buf_start, oob_buf_start, page);
+ ret = parse_read_errors(host, data_buf_start, oob_buf_start, page);
+
+ return ret;
Why?
will fix in next patch
}
/*
@@ -2154,12 +1913,25 @@ static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf,
{
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 *data_buf, *oob_buf = NULL;
+ int ret = 0;
if (host->nr_boot_partitions)
qcom_nandc_codeword_fixup(host, page);
- nand_read_page_op(chip, page, 0, NULL, 0);
+ chip->cont_read.ongoing = false;
+ ret = nand_read_page_op(chip, page, 0, buf, 2048);
+ if (ret)
+ return ret;
+
+ nandc->buf_count = 0;
+ nandc->buf_start = 0;
+ host->use_ecc = true;
+ clear_read_regs(nandc);
+ set_address(host, 0, page);
+ update_rw_regs(host, ecc->steps, true, 0);
+
data_buf = buf;
oob_buf = oob_required ? chip->oob_poi : NULL;
@@ -2229,6 +2001,10 @@ static int qcom_nandc_write_page(struct nand_chip *chip, const uint8_t *buf,
nand_prog_page_begin_op(chip, page, 0, NULL, 0);
+ set_address(host, 0, page);
+ nandc->buf_count = 0;
+ nandc->buf_start = 0;
+
As said above, you need a separate patch for the above change. I do not get the
context of the above change.
And I stopped reviewing here. It's hard to review ~800 lines of change in one
shot. Please split this patch into sensible chunks so that it makes life easy
for reviewers.
- Mani
Sure will fix all comments in next patch and post it again.
Thanks,
Alam.