Message ID | 20230531124953.21007-4-quic_mdalam@quicinc.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | [v3,1/5] mtd: rawnand: qcom: Implement exec_op() | expand |
Hi Md, quic_mdalam@quicinc.com wrote on Wed, 31 May 2023 18:19:52 +0530: > This change will add exec_ops support for READ, WRITE, and ERASE > command. > > Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> > Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> > --- > Change in [v3] > > * Removed chip->cont_read.ongoing flag. > > * Removed pre_command from erase_etype_exec_ops. > > Change in [v2] > > * Missed to post Cover-letter, so posting v2 patch with cover-letter. > > Change in [v1] > > * Added initial support for exec_ops. > > drivers/mtd/nand/raw/qcom_nandc.c | 97 +++++++++++++++++++++++++++++-- > 1 file changed, 93 insertions(+), 4 deletions(-) > > diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c > index b4823b72fe73..7dc769f9e797 100644 > --- a/drivers/mtd/nand/raw/qcom_nandc.c > +++ b/drivers/mtd/nand/raw/qcom_nandc.c > @@ -1546,8 +1546,7 @@ static void pre_command(struct qcom_nand_host *host, int command) > > clear_read_regs(nandc); > > - if (command == NAND_CMD_ERASE1) > - clear_bam_transaction(nandc); > + clear_bam_transaction(nandc); > } > > /* > @@ -1764,7 +1763,7 @@ 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); > + nand_read_page_op(chip, page, 0, data_buf, mtd->writesize); > host->use_ecc = false; > > if (nandc->props->qpic_v2) > @@ -2181,14 +2180,23 @@ static void qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page) > static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf, > int oob_required, int page) > { > + struct mtd_info *mtd = nand_to_mtd(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 *data_buf, *oob_buf = NULL; > > if (host->nr_boot_partitions) > qcom_nandc_codeword_fixup(host, page); > > - nand_read_page_op(chip, page, 0, NULL, 0); > + nand_read_page_op(chip, page, 0, buf, mtd->writesize); > + 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; > > @@ -2258,6 +2266,9 @@ 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; > clear_read_regs(nandc); > clear_bam_transaction(nandc); > > @@ -3274,6 +3285,67 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_ > return ret; > } > > +static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_subop *subop) > +{ > + struct qcom_nand_host *host = to_qcom_nand_host(chip); > + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); > + struct qcom_op q_op; > + int ret = 0; > + > + qcom_parse_instructions(chip, subop, &q_op); > + > + q_op.cmd_reg |= PAGE_ACC | LAST_PAGE; > + > + nandc->buf_count = 0; > + nandc->buf_start = 0; > + host->use_ecc = false; > + clear_read_regs(nandc); > + clear_bam_transaction(nandc); > + > + nandc_set_reg(chip, NAND_FLASH_CMD, q_op.cmd_reg); > + nandc_set_reg(chip, NAND_ADDR0, q_op.addr1_reg); > + nandc_set_reg(chip, NAND_ADDR1, q_op.addr2_reg); > + nandc_set_reg(chip, NAND_DEV0_CFG0, > + host->cfg0_raw & ~(7 << CW_PER_PAGE)); > + nandc_set_reg(chip, NAND_DEV0_CFG1, host->cfg1_raw); > + nandc_set_reg(chip, NAND_EXEC_CMD, 1); > + > + write_reg_dma(nandc, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); > + write_reg_dma(nandc, NAND_DEV0_CFG0, 2, NAND_BAM_NEXT_SGL); > + write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); > + > + ret = submit_descs(nandc); > + if (ret) { > + dev_err(nandc->dev, "failure in sbumitting erase descriptor\n"); :) > + free_descs(nandc); > + goto err_out; > + } > + free_descs(nandc); > + > + ret = qcom_wait_rdy_poll(chip, q_op.rdy_timeout_ms); > + if (ret) > + goto err_out; > + > +err_out: > + return ret; > +} > + > +static int qcom_data_read_type_exec(struct nand_chip *chip, const struct nand_subop *subop) > +{ > + /* currently read_exec_op() return 0 , and all the read operation handle in > + * actual API itself > + */ > + return 0; Mmmh, I don't think this is gonna work. I don't understand what you're doing here. What is "actual API itself"? What is "read_exec_op"? I doubt I am going to like what all this means. Please don't make any assumptions on what could come next. The core asks you to do something, just do it. If you can't then the parsing will fail. If the core has a fallback it's fine. If the core does not, we can discuss it. But please don't do any guesses like that, this is *exactly* why we introduced exec_op in the first place: you have access to the whole operation, so please handle it correctly. > +} > + > +static int qcom_data_write_type_exec(struct nand_chip *chip, const struct nand_subop *subop) > +{ > + /* currently write_exec_op() return 0, and all the write operation handle in > + * actual API itself > + */ > + return 0; > +} > + > static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER( > NAND_OP_PARSER_PATTERN( > qcom_misc_cmd_type_exec, > @@ -3294,6 +3366,23 @@ static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER( > NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), > NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 512)), > + NAND_OP_PARSER_PATTERN( > + qcom_erase_cmd_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), > + NAND_OP_PARSER_PATTERN( > + qcom_data_read_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), > + NAND_OP_PARSER_PAT_CMD_ELEM(false), > + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), > + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)), > + NAND_OP_PARSER_PATTERN( > + qcom_data_write_type_exec, > + NAND_OP_PARSER_PAT_CMD_ELEM(true), > + NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYCLE)), > ); > > static int qcom_check_op(struct nand_chip *chip, Thanks, Miquèl
On 6/8/2023 7:33 PM, Miquel Raynal wrote: > Hi Md, > > quic_mdalam@quicinc.com wrote on Wed, 31 May 2023 18:19:52 +0530: > >> This change will add exec_ops support for READ, WRITE, and ERASE >> command. >> >> Co-developed-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com> >> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com> >> --- >> Change in [v3] >> >> * Removed chip->cont_read.ongoing flag. >> >> * Removed pre_command from erase_etype_exec_ops. >> >> Change in [v2] >> >> * Missed to post Cover-letter, so posting v2 patch with cover-letter. >> >> Change in [v1] >> >> * Added initial support for exec_ops. >> >> drivers/mtd/nand/raw/qcom_nandc.c | 97 +++++++++++++++++++++++++++++-- >> 1 file changed, 93 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c >> index b4823b72fe73..7dc769f9e797 100644 >> --- a/drivers/mtd/nand/raw/qcom_nandc.c >> +++ b/drivers/mtd/nand/raw/qcom_nandc.c >> @@ -1546,8 +1546,7 @@ static void pre_command(struct qcom_nand_host *host, int command) >> >> clear_read_regs(nandc); >> >> - if (command == NAND_CMD_ERASE1) >> - clear_bam_transaction(nandc); >> + clear_bam_transaction(nandc); >> } >> >> /* >> @@ -1764,7 +1763,7 @@ 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); >> + nand_read_page_op(chip, page, 0, data_buf, mtd->writesize); >> host->use_ecc = false; >> >> if (nandc->props->qpic_v2) >> @@ -2181,14 +2180,23 @@ static void qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page) >> static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf, >> int oob_required, int page) >> { >> + struct mtd_info *mtd = nand_to_mtd(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 *data_buf, *oob_buf = NULL; >> >> if (host->nr_boot_partitions) >> qcom_nandc_codeword_fixup(host, page); >> >> - nand_read_page_op(chip, page, 0, NULL, 0); >> + nand_read_page_op(chip, page, 0, buf, mtd->writesize); >> + 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; >> >> @@ -2258,6 +2266,9 @@ 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; >> clear_read_regs(nandc); >> clear_bam_transaction(nandc); >> >> @@ -3274,6 +3285,67 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_ >> return ret; >> } >> >> +static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_subop *subop) >> +{ >> + struct qcom_nand_host *host = to_qcom_nand_host(chip); >> + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); >> + struct qcom_op q_op; >> + int ret = 0; >> + >> + qcom_parse_instructions(chip, subop, &q_op); >> + >> + q_op.cmd_reg |= PAGE_ACC | LAST_PAGE; >> + >> + nandc->buf_count = 0; >> + nandc->buf_start = 0; >> + host->use_ecc = false; >> + clear_read_regs(nandc); >> + clear_bam_transaction(nandc); >> + >> + nandc_set_reg(chip, NAND_FLASH_CMD, q_op.cmd_reg); >> + nandc_set_reg(chip, NAND_ADDR0, q_op.addr1_reg); >> + nandc_set_reg(chip, NAND_ADDR1, q_op.addr2_reg); >> + nandc_set_reg(chip, NAND_DEV0_CFG0, >> + host->cfg0_raw & ~(7 << CW_PER_PAGE)); >> + nandc_set_reg(chip, NAND_DEV0_CFG1, host->cfg1_raw); >> + nandc_set_reg(chip, NAND_EXEC_CMD, 1); >> + >> + write_reg_dma(nandc, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); >> + write_reg_dma(nandc, NAND_DEV0_CFG0, 2, NAND_BAM_NEXT_SGL); >> + write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); >> + >> + ret = submit_descs(nandc); >> + if (ret) { >> + dev_err(nandc->dev, "failure in sbumitting erase descriptor\n"); > > :) Its spelling mistake. Will fix this in next patch V4. > >> + free_descs(nandc); >> + goto err_out; >> + } >> + free_descs(nandc); >> + >> + ret = qcom_wait_rdy_poll(chip, q_op.rdy_timeout_ms); >> + if (ret) >> + goto err_out; >> + >> +err_out: >> + return ret; >> +} >> + >> +static int qcom_data_read_type_exec(struct nand_chip *chip, const struct nand_subop *subop) >> +{ >> + /* currently read_exec_op() return 0 , and all the read operation handle in >> + * actual API itself >> + */ >> + return 0; > > Mmmh, I don't think this is gonna work. I don't understand what you're > doing here. What is "actual API itself"? What is "read_exec_op"? I > doubt I am going to like what all this means. Please don't make any > assumptions on what could come next. The core asks you to do something, > just do it. If you can't then the parsing will fail. If the core has a > fallback it's fine. If the core does not, we can discuss it. But please > don't do any guesses like that, this is *exactly* why we introduced > exec_op in the first place: you have access to the whole operation, so > please handle it correctly. Thanks, When Initailly I was started exec_op() implementation I thought that nand_read_page_op() will fail, so that I added the dummy qcom_data_read_type_exec() and qcom_data_write_type_exec() APIs for read/write. But as you said in above comment , we have to follow whatever core ask. So I removed these APIs and tested read/write working fine. So now these dummy APIs not at all needed.I think I misunderstood. I will keep nand_read_page_op() API in original state nand_read_page_op(chip, page, 0, NULL, 0); Also removed the pattern for read_page() and write_page() since its not at all needed. Will fix these all in patch V4. > >> +} >> + >> +static int qcom_data_write_type_exec(struct nand_chip *chip, const struct nand_subop *subop) >> +{ >> + /* currently write_exec_op() return 0, and all the write operation handle in >> + * actual API itself >> + */ >> + return 0; >> +} >> + >> static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER( >> NAND_OP_PARSER_PATTERN( >> qcom_misc_cmd_type_exec, >> @@ -3294,6 +3366,23 @@ static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER( >> NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), >> NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), >> NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 512)), >> + NAND_OP_PARSER_PATTERN( >> + qcom_erase_cmd_type_exec, >> + NAND_OP_PARSER_PAT_CMD_ELEM(false), >> + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), >> + NAND_OP_PARSER_PAT_CMD_ELEM(false), >> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), >> + NAND_OP_PARSER_PATTERN( >> + qcom_data_read_type_exec, >> + NAND_OP_PARSER_PAT_CMD_ELEM(false), >> + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), >> + NAND_OP_PARSER_PAT_CMD_ELEM(false), >> + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), >> + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)), >> + NAND_OP_PARSER_PATTERN( >> + qcom_data_write_type_exec, >> + NAND_OP_PARSER_PAT_CMD_ELEM(true), >> + NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYCLE)), >> ); >> >> static int qcom_check_op(struct nand_chip *chip, > > > Thanks, > Miquèl
diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c index b4823b72fe73..7dc769f9e797 100644 --- a/drivers/mtd/nand/raw/qcom_nandc.c +++ b/drivers/mtd/nand/raw/qcom_nandc.c @@ -1546,8 +1546,7 @@ static void pre_command(struct qcom_nand_host *host, int command) clear_read_regs(nandc); - if (command == NAND_CMD_ERASE1) - clear_bam_transaction(nandc); + clear_bam_transaction(nandc); } /* @@ -1764,7 +1763,7 @@ 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); + nand_read_page_op(chip, page, 0, data_buf, mtd->writesize); host->use_ecc = false; if (nandc->props->qpic_v2) @@ -2181,14 +2180,23 @@ static void qcom_nandc_codeword_fixup(struct qcom_nand_host *host, int page) static int qcom_nandc_read_page(struct nand_chip *chip, uint8_t *buf, int oob_required, int page) { + struct mtd_info *mtd = nand_to_mtd(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 *data_buf, *oob_buf = NULL; if (host->nr_boot_partitions) qcom_nandc_codeword_fixup(host, page); - nand_read_page_op(chip, page, 0, NULL, 0); + nand_read_page_op(chip, page, 0, buf, mtd->writesize); + 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; @@ -2258,6 +2266,9 @@ 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; clear_read_regs(nandc); clear_bam_transaction(nandc); @@ -3274,6 +3285,67 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_ return ret; } +static int qcom_erase_cmd_type_exec(struct nand_chip *chip, const struct nand_subop *subop) +{ + struct qcom_nand_host *host = to_qcom_nand_host(chip); + struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip); + struct qcom_op q_op; + int ret = 0; + + qcom_parse_instructions(chip, subop, &q_op); + + q_op.cmd_reg |= PAGE_ACC | LAST_PAGE; + + nandc->buf_count = 0; + nandc->buf_start = 0; + host->use_ecc = false; + clear_read_regs(nandc); + clear_bam_transaction(nandc); + + nandc_set_reg(chip, NAND_FLASH_CMD, q_op.cmd_reg); + nandc_set_reg(chip, NAND_ADDR0, q_op.addr1_reg); + nandc_set_reg(chip, NAND_ADDR1, q_op.addr2_reg); + nandc_set_reg(chip, NAND_DEV0_CFG0, + host->cfg0_raw & ~(7 << CW_PER_PAGE)); + nandc_set_reg(chip, NAND_DEV0_CFG1, host->cfg1_raw); + nandc_set_reg(chip, NAND_EXEC_CMD, 1); + + write_reg_dma(nandc, NAND_FLASH_CMD, 3, NAND_BAM_NEXT_SGL); + write_reg_dma(nandc, NAND_DEV0_CFG0, 2, NAND_BAM_NEXT_SGL); + write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL); + + ret = submit_descs(nandc); + if (ret) { + dev_err(nandc->dev, "failure in sbumitting erase descriptor\n"); + free_descs(nandc); + goto err_out; + } + free_descs(nandc); + + ret = qcom_wait_rdy_poll(chip, q_op.rdy_timeout_ms); + if (ret) + goto err_out; + +err_out: + return ret; +} + +static int qcom_data_read_type_exec(struct nand_chip *chip, const struct nand_subop *subop) +{ + /* currently read_exec_op() return 0 , and all the read operation handle in + * actual API itself + */ + return 0; +} + +static int qcom_data_write_type_exec(struct nand_chip *chip, const struct nand_subop *subop) +{ + /* currently write_exec_op() return 0, and all the write operation handle in + * actual API itself + */ + return 0; +} + static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER( NAND_OP_PARSER_PATTERN( qcom_misc_cmd_type_exec, @@ -3294,6 +3366,23 @@ static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER( NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 512)), + NAND_OP_PARSER_PATTERN( + qcom_erase_cmd_type_exec, + NAND_OP_PARSER_PAT_CMD_ELEM(false), + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), + NAND_OP_PARSER_PAT_CMD_ELEM(false), + NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)), + NAND_OP_PARSER_PATTERN( + qcom_data_read_type_exec, + NAND_OP_PARSER_PAT_CMD_ELEM(false), + NAND_OP_PARSER_PAT_ADDR_ELEM(false, MAX_ADDRESS_CYCLE), + NAND_OP_PARSER_PAT_CMD_ELEM(false), + NAND_OP_PARSER_PAT_WAITRDY_ELEM(true), + NAND_OP_PARSER_PAT_DATA_IN_ELEM(false, 2048)), + NAND_OP_PARSER_PATTERN( + qcom_data_write_type_exec, + NAND_OP_PARSER_PAT_CMD_ELEM(true), + NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYCLE)), ); static int qcom_check_op(struct nand_chip *chip,