[PATCH 0/3]mtd: rawnand: qcom: Fixes for exec_op

[PATCH 0/3]mtd: rawnand: qcom: Fixes for exec_op

[Md Sadre Alam]

This series fixes the following.

1. fixes parameter page read len for 4K nand
   in exec_op.

2. fixes raw_nand_read issue.

3. fixes READ path issue in exec_op.

Applied on top of [1]

This series tested on IPQ8074 2K and 4K nand with
mtd_test*.ko (mtd_nandbiterrs.ko, mtd_pagetest.ko, mtd_readtest.ko
mtd_subpagetest.ko, mtd_torturetest.ko, mtd_stresstest.ko). 

[1] https://lore.kernel.org/lkml/20230805174146.57006-1-manivannan.sadhasivam@linaro.org/

Md Sadre Alam (2):
  mtd: rawnand: qcom: Update read_loc size to 512
  mtd: rawnand: qcom: Clear buf_count and buf_start in raw read

Sricharan Ramabadhran (1):
  mtd: rawnand: qcom: Add read/read_start ops in exec_op path

 drivers/mtd/nand/raw/qcom_nandc.c | 99 +++++++++++++------------------
 1 file changed, 41 insertions(+), 58 deletions(-)

-- 
2.17.1

[PATCH 1/3] mtd: rawnand: qcom: Update read_loc size to 512

[Md Sadre Alam]

For parameter page read upper layer is passing len
as 256 bytes and if we try to configure 256 bytes
size in read loaction register then subsequent bam
transaction is getting timed out for 4K nand devices.
So update this length as one step size if its
less than NANDC_STEP_SIZE.

Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index d4ba0d04c970..413e214c8e87 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2885,6 +2885,9 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
 	op_id = q_op.data_instr_idx;
 	len = nand_subop_get_data_len(subop, op_id);
 
+	if (len < NANDC_STEP_SIZE)
+		len = NANDC_STEP_SIZE;
+
 	nandc_set_read_loc(chip, 0, 0, 0, len, 1);
 
 	if (!nandc->props->qpic_v2) {
-- 
2.17.1

Re: [PATCH 1/3] mtd: rawnand: qcom: Update read_loc size to 512

[Miquel Raynal]

Hi Md,

quic_mdalam@quicinc.com wrote on Fri, 18 Aug 2023 20:20:59 +0530:

> For parameter page read upper layer is passing len
> as 256 bytes and if we try to configure 256 bytes
> size in read loaction register then subsequent bam
> transaction is getting timed out for 4K nand devices.
> So update this length as one step size if its
> less than NANDC_STEP_SIZE.
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

I'm fine with patches 2 and 3 and will take them. But this one does not
seem legitimate. I don't like it. Are you sure the ECC engine was not
enabled when it timed out? Default should be having the ECC disabled
and it should just get enabled when you need it. There is no reason
why, specifically on NAND devices, it would not be possible to read 256
bytes.

> ---
>  drivers/mtd/nand/raw/qcom_nandc.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
> index d4ba0d04c970..413e214c8e87 100644
> --- a/drivers/mtd/nand/raw/qcom_nandc.c
> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
> @@ -2885,6 +2885,9 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>  	op_id = q_op.data_instr_idx;
>  	len = nand_subop_get_data_len(subop, op_id);
>  
> +	if (len < NANDC_STEP_SIZE)
> +		len = NANDC_STEP_SIZE;
> +
>  	nandc_set_read_loc(chip, 0, 0, 0, len, 1);
>  
>  	if (!nandc->props->qpic_v2) {


Thanks,
Miquèl

Re: [PATCH 1/3] mtd: rawnand: qcom: Update read_loc size to 512

[Md Sadre Alam]

On 8/19/2023 1:29 AM, Miquel Raynal wrote:
> Hi Md,
> 
> quic_mdalam@quicinc.com wrote on Fri, 18 Aug 2023 20:20:59 +0530:
> 
>> For parameter page read upper layer is passing len
>> as 256 bytes and if we try to configure 256 bytes
>> size in read loaction register then subsequent bam
>> transaction is getting timed out for 4K nand devices.
>> So update this length as one step size if its
>> less than NANDC_STEP_SIZE.
>>
>> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
>> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> 
> I'm fine with patches 2 and 3 and will take them. But this one does not
> seem legitimate. I don't like it. Are you sure the ECC engine was not
> enabled when it timed out? Default should be having the ECC disabled
> and it should just get enabled when you need it. There is no reason
> why, specifically on NAND devices, it would not be possible to read 256
> bytes.
> 

    Yes default ECC engine is disabled only. When length was 512 bytes for
    parameter page read it was causing BAM timeout error on SDX65, but it
    was working fine on IPQ807x.
    We will drop this patch for now. Let me check on SDX65 and try to fix
    all the error.

>> ---
>>   drivers/mtd/nand/raw/qcom_nandc.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
>> index d4ba0d04c970..413e214c8e87 100644
>> --- a/drivers/mtd/nand/raw/qcom_nandc.c
>> +++ b/drivers/mtd/nand/raw/qcom_nandc.c
>> @@ -2885,6 +2885,9 @@ static int qcom_param_page_type_exec(struct nand_chip *chip,  const struct nand_
>>   	op_id = q_op.data_instr_idx;
>>   	len = nand_subop_get_data_len(subop, op_id);
>>   
>> +	if (len < NANDC_STEP_SIZE)
>> +		len = NANDC_STEP_SIZE;
>> +
>>   	nandc_set_read_loc(chip, 0, 0, 0, len, 1);
>>   
>>   	if (!nandc->props->qpic_v2) {
> 
> 
> Thanks,
> Miquèl

[PATCH 2/3] mtd: rawnand: qcom: Clear buf_count and buf_start in raw read

[Md Sadre Alam]

Initialize buf_count and buf_start to 0 before starting the
raw read. If we will not initialize then read staus will get
updated with wrong value and we will see failure for even
successful raw read transaction.

Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 413e214c8e87..73dd1b4e4e31 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -1471,6 +1471,9 @@ qcom_nandc_read_cw_raw(struct mtd_info *mtd, struct nand_chip *chip,
 	int raw_cw = cw;
 
 	nand_read_page_op(chip, page, 0, NULL, 0);
+	nandc->buf_count = 0;
+	nandc->buf_start = 0;
+	clear_read_regs(nandc);
 	host->use_ecc = false;
 
 	if (nandc->props->qpic_v2)
-- 
2.17.1

Re: [PATCH 2/3] mtd: rawnand: qcom: Clear buf_count and buf_start in raw read

[Miquel Raynal]

On Fri, 2023-08-18 at 14:51:00 UTC, Md Sadre Alam wrote:
> Initialize buf_count and buf_start to 0 before starting the
> raw read. If we will not initialize then read staus will get
> updated with wrong value and we will see failure for even
> successful raw read transaction.
> 
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel

[PATCH 3/3] mtd: rawnand: qcom: Add read/read_start ops in exec_op path

[Md Sadre Alam]

From: Sricharan Ramabadhran <quic_srichara@quicinc.com>

READ/READ_START opcodes are not set in exec_op path.
Fixing that here.

While there, Steps to program the controller is common for
erase/reset/read/program page. So use a common pattern and
pull them under one function.

Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>
---
 drivers/mtd/nand/raw/qcom_nandc.c | 93 ++++++++++++-------------------
 1 file changed, 35 insertions(+), 58 deletions(-)

diff --git a/drivers/mtd/nand/raw/qcom_nandc.c b/drivers/mtd/nand/raw/qcom_nandc.c
index 73dd1b4e4e31..370a779610e2 100644
--- a/drivers/mtd/nand/raw/qcom_nandc.c
+++ b/drivers/mtd/nand/raw/qcom_nandc.c
@@ -2541,9 +2541,11 @@ static int qcom_nand_attach_chip(struct nand_chip *chip)
 	return 0;
 }
 
-static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 opcode,
+static int qcom_op_cmd_mapping(struct nand_chip *chip, u8 opcode,
 			       struct qcom_op *q_op)
 {
+	struct qcom_nand_controller *nandc = get_qcom_nand_controller(chip);
+	struct qcom_nand_host *host = to_qcom_nand_host(chip);
 	int cmd;
 
 	switch (opcode) {
@@ -2571,6 +2573,13 @@ static int qcom_op_cmd_mapping(struct qcom_nand_controller *nandc, u8 opcode,
 		q_op->flag = OP_PROGRAM_PAGE;
 		nandc->exec_opwrite = true;
 		break;
+	case NAND_CMD_READ0:
+	case NAND_CMD_READSTART:
+		if (host->use_ecc)
+			cmd = OP_PAGE_READ_WITH_ECC;
+		else
+			cmd = OP_PAGE_READ;
+		break;
 	default:
 		dev_err(nandc->dev, "Opcode not supported: %u\n", opcode);
 		return -EOPNOTSUPP;
@@ -2597,7 +2606,7 @@ static int qcom_parse_instructions(struct nand_chip *chip,
 
 		switch (instr->type) {
 		case NAND_OP_CMD_INSTR:
-			ret = qcom_op_cmd_mapping(nandc, instr->ctx.cmd.opcode, q_op);
+			ret = qcom_op_cmd_mapping(chip, instr->ctx.cmd.opcode, q_op);
 			if (ret < 0)
 				return ret;
 
@@ -2791,13 +2800,25 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub
 	struct qcom_nand_host *host = to_qcom_nand_host(chip);
 	struct qcom_op q_op = {};
 	int ret;
+	int instrs = 1;
 
 	ret = qcom_parse_instructions(chip, subop, &q_op);
 	if (ret)
 		return ret;
 
-	if (q_op.flag == OP_PROGRAM_PAGE)
+	if (q_op.flag == OP_PROGRAM_PAGE) {
 		goto wait_rdy;
+	} else if (q_op.cmd_reg == OP_BLOCK_ERASE) {
+		q_op.cmd_reg |= PAGE_ACC | LAST_PAGE;
+		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);
+		instrs = 3;
+	} else {
+		return 0;
+	}
 
 	nandc->buf_count = 0;
 	nandc->buf_start = 0;
@@ -2809,9 +2830,12 @@ static int qcom_misc_cmd_type_exec(struct nand_chip *chip, const struct nand_sub
 	nandc_set_reg(chip, NAND_FLASH_CMD, q_op.cmd_reg);
 	nandc_set_reg(chip, NAND_EXEC_CMD, 1);
 
-	write_reg_dma(nandc, NAND_FLASH_CMD, 1, NAND_BAM_NEXT_SGL);
-	write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
+	write_reg_dma(nandc, NAND_FLASH_CMD, instrs, NAND_BAM_NEXT_SGL);
+	(q_op.cmd_reg == OP_BLOCK_ERASE) ? write_reg_dma(nandc, NAND_DEV0_CFG0,
+	2, NAND_BAM_NEXT_SGL) : read_reg_dma(nandc,
+	NAND_FLASH_STATUS, 1, NAND_BAM_NEXT_SGL);
 
+	write_reg_dma(nandc, NAND_EXEC_CMD, 1, NAND_BAM_NEXT_SGL);
 	read_reg_dma(nandc, NAND_FLASH_STATUS, 1, NAND_BAM_NEXT_SGL);
 
 	ret = submit_descs(nandc);
@@ -2928,56 +2952,7 @@ 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;
-
-	ret = qcom_parse_instructions(chip, subop, &q_op);
-	if (ret)
-		return ret;
-
-	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 submitting erase descriptor\n");
-		goto err_out;
-	}
-
-	ret = qcom_wait_rdy_poll(chip, q_op.rdy_timeout_ms);
-	if (ret)
-		goto err_out;
-
-err_out:
-	return ret;
-}
-
 static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER(
-		NAND_OP_PARSER_PATTERN(
-			qcom_misc_cmd_type_exec,
-			NAND_OP_PARSER_PAT_CMD_ELEM(false),
-			NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
 		NAND_OP_PARSER_PATTERN(
 			qcom_read_id_type_exec,
 			NAND_OP_PARSER_PAT_CMD_ELEM(false),
@@ -2994,10 +2969,10 @@ static const struct nand_op_parser qcom_op_parser = NAND_OP_PARSER(
 			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),
+			qcom_misc_cmd_type_exec,
 			NAND_OP_PARSER_PAT_CMD_ELEM(false),
+			NAND_OP_PARSER_PAT_ADDR_ELEM(true, MAX_ADDRESS_CYCLE),
+			NAND_OP_PARSER_PAT_CMD_ELEM(true),
 			NAND_OP_PARSER_PAT_WAITRDY_ELEM(false)),
 		);
 
@@ -3018,7 +2993,9 @@ static int qcom_check_op(struct nand_chip *chip,
 			    instr->ctx.cmd.opcode != NAND_CMD_ERASE1 &&
 			    instr->ctx.cmd.opcode != NAND_CMD_ERASE2 &&
 			    instr->ctx.cmd.opcode != NAND_CMD_STATUS &&
-			    instr->ctx.cmd.opcode != NAND_CMD_PAGEPROG)
+			    instr->ctx.cmd.opcode != NAND_CMD_PAGEPROG &&
+			    instr->ctx.cmd.opcode != NAND_CMD_READ0 &&
+			    instr->ctx.cmd.opcode != NAND_CMD_READSTART)
 				return -EOPNOTSUPP;
 			break;
 		default:
-- 
2.17.1

Re: [PATCH 3/3] mtd: rawnand: qcom: Add read/read_start ops in exec_op path

[Miquel Raynal]

On Fri, 2023-08-18 at 14:51:01 UTC, Md Sadre Alam wrote:
> From: Sricharan Ramabadhran <quic_srichara@quicinc.com>
> 
> READ/READ_START opcodes are not set in exec_op path.
> Fixing that here.
> 
> While there, Steps to program the controller is common for
> erase/reset/read/program page. So use a common pattern and
> pull them under one function.
> 
> Signed-off-by: Md Sadre Alam <quic_mdalam@quicinc.com>
> Signed-off-by: Sricharan Ramabadhran <quic_srichara@quicinc.com>

Applied to https://git.kernel.org/pub/scm/linux/kernel/git/mtd/linux.git nand/next, thanks.

Miquel