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.
> 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.
> };
>
> /*
> @@ -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.
> + *
> + * @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.
> + * @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.
> + * @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.
> + 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?
> 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.
> -{
> - 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.
> 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?
> }
>
> /*
> @@ -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
--
மணிவண்ணன் சதாசிவம்
