[PATCH V2] mtd: rawnand: qcom: Implement exec_op()
Md Sadre Alam
quic_mdalam at quicinc.com
Wed May 10 02:01:25 PDT 2023
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 at quicinc.com>
>> Signed-off-by: Sricharan R <quic_srichara at quicinc.com>
>> Signed-off-by: Md Sadre Alam <quic_mdalam at quicinc.com>
>> ---
>> 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.
>
More information about the linux-mtd
mailing list