| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <871pzh397j.fsf@bootlin.com>
Date: Mon, 11 Nov 2024 19:30:57 +0100
From: Miquel Raynal <miquel.raynal@...tlin.com>
To: Md Sadre Alam <quic_mdalam@...cinc.com>
Cc: <broonie@...nel.org>, <robh@...nel.org>, <krzk+dt@...nel.org>,
<conor+dt@...nel.org>, <andersson@...nel.org>,
<konradybcio@...nel.org>, <richard@....at>, <vigneshr@...com>,
<manivannan.sadhasivam@...aro.org>, <linux-arm-msm@...r.kernel.org>,
<linux-spi@...r.kernel.org>, <devicetree@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, <linux-mtd@...ts.infradead.org>,
<quic_srichara@...cinc.com>, <quic_varada@...cinc.com>
Subject: Re: [PATCH v13 2/8] mtd: rawnand: qcom: cleanup qcom_nandc driver
On 30/10/2024 at 17:49:13 +0530, Md Sadre Alam <quic_mdalam@...cinc.com> wrote:
> cleanup qcom_nandc driver as below
Perform a global cleanup of the Qualcomm NAND controller driver with the
following improvements:
>
> - Remove register value indirection api
API
>
> - Remove set_reg() api
API
>
> - Convert read_loc_first & read_loc_last macro to function
functions
>
> - Renamed multiple variables
Rename
...
> @@ -549,17 +535,17 @@ struct qcom_nand_host {
> * among different NAND controllers.
> * @ecc_modes - ecc mode for NAND
> * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
> - * @is_bam - whether NAND controller is using BAM
> - * @is_qpic - whether NAND CTRL is part of qpic IP
> - * @qpic_v2 - flag to indicate QPIC IP version 2
> + * @supports_bam - whether NAND controller is using BAM
Use the plain letters BAM acronym at least here
> + * @nandc_part_of_qpic - whether NAND controller is part of qpic IP
> + * @qpic_version2 - flag to indicate QPIC IP version 2
> * @use_codeword_fixup - whether NAND has different layout for boot partitions
> */
> struct qcom_nandc_props {
> u32 ecc_modes;
> u32 dev_cmd_reg_start;
> - bool is_bam;
> - bool is_qpic;
> - bool qpic_v2;
> + bool supports_bam;
> + bool nandc_part_of_qpic;
> + bool qpic_version2;
> bool use_codeword_fixup;
> };
>
> @@ -613,19 +599,11 @@ static void clear_bam_transaction(struct qcom_nand_controller *nandc)
> {
> struct bam_transaction *bam_txn = nandc->bam_txn;
>
> - if (!nandc->props->is_bam)
> + if (!nandc->props->supports_bam)
> return;
>
> - bam_txn->bam_ce_pos = 0;
> - bam_txn->bam_ce_start = 0;
> - bam_txn->cmd_sgl_pos = 0;
> - bam_txn->cmd_sgl_start = 0;
> - bam_txn->tx_sgl_pos = 0;
> - bam_txn->tx_sgl_start = 0;
> - bam_txn->rx_sgl_pos = 0;
> - bam_txn->rx_sgl_start = 0;
> + memset(&bam_txn->bam_ce_pos, 0, sizeof(u32) * 8);
> bam_txn->last_data_desc = NULL;
> - bam_txn->wait_second_completion = false;
>
> sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
> QPIC_PER_CW_CMD_SGL);
> @@ -640,17 +618,7 @@ static void qpic_bam_dma_done(void *data)
> {
> struct bam_transaction *bam_txn = data;
>
> - /*
> - * In case of data transfer with NAND, 2 callbacks will be generated.
> - * One for command channel and another one for data channel.
> - * If current transaction has data descriptors
> - * (i.e. wait_second_completion is true), then set this to false
> - * and wait for second DMA descriptor completion.
> - */
> - if (bam_txn->wait_second_completion)
> - bam_txn->wait_second_completion = false;
> - else
> - complete(&bam_txn->txn_done);
> + complete(&bam_txn->txn_done);
> }
>
> static inline struct qcom_nand_host *to_qcom_nand_host(struct nand_chip *chip)
> @@ -676,10 +644,9 @@ static inline void nandc_write(struct qcom_nand_controller *nandc, int offset,
> iowrite32(val, nandc->base + offset);
> }
>
> -static inline void nandc_read_buffer_sync(struct qcom_nand_controller *nandc,
> - bool is_cpu)
> +static inline void nandc_dev_to_mem(struct qcom_nand_controller *nandc, bool is_cpu)
No static inline in C code, you can also remove it.
> {
> - if (!nandc->props->is_bam)
> + if (!nandc->props->supports_bam)
> return;
>
> if (is_cpu)
> @@ -694,93 +661,90 @@ static inline void nandc_read_buffer_sync(struct qcom_nand_controller *nandc,
> DMA_FROM_DEVICE);
> }
...
> +/* Helper to check the code word, whether it is last cw or not */
Helper to check whether this is the last CW or not
> +static bool qcom_nandc_is_last_cw(struct nand_ecc_ctrl *ecc, int cw)
> +{
> + return cw == (ecc->steps - 1);
> }
>
> -static void nandc_set_reg(struct nand_chip *chip, int offset,
> - u32 val)
> +/**
> + * nandc_set_read_loc_first() - to set read location first register
> + * @chip: NAND Private Flash Chip Data
> + * @reg_base: location register base
> + * @cw_offset: code word offset
> + * @read_size: code word read length
> + * @is_last_read_loc: is this the last read location
> + *
> + * This function will set location register value
> + */
...
> if (host->use_ecc) {
> - cfg0 = (host->cfg0 & ~(7U << CW_PER_PAGE)) |
> - (num_cw - 1) << CW_PER_PAGE;
> + cfg0 = cpu_to_le32((host->cfg0 & ~(7U << CW_PER_PAGE)) |
> + (num_cw - 1) << CW_PER_PAGE);
>
> - cfg1 = host->cfg1;
> - ecc_bch_cfg = host->ecc_bch_cfg;
> + cfg1 = cpu_to_le32(host->cfg1);
> + ecc_bch_cfg = cpu_to_le32(host->ecc_bch_cfg);
> } else {
> - cfg0 = (host->cfg0_raw & ~(7U << CW_PER_PAGE)) |
> - (num_cw - 1) << CW_PER_PAGE;
> + cfg0 = cpu_to_le32((host->cfg0_raw & ~(7U << CW_PER_PAGE)) |
> + (num_cw - 1) << CW_PER_PAGE);
>
> - cfg1 = host->cfg1_raw;
> - ecc_bch_cfg = 1 << ECC_CFG_ECC_DISABLE;
> + cfg1 = cpu_to_le32(host->cfg1_raw);
> + ecc_bch_cfg = cpu_to_le32(1 << ECC_CFG_ECC_DISABLE);
> }
>
> - nandc_set_reg(chip, NAND_FLASH_CMD, cmd);
> - nandc_set_reg(chip, NAND_DEV0_CFG0, cfg0);
> - nandc_set_reg(chip, NAND_DEV0_CFG1, cfg1);
> - nandc_set_reg(chip, NAND_DEV0_ECC_CFG, ecc_bch_cfg);
> - if (!nandc->props->qpic_v2)
> - nandc_set_reg(chip, NAND_EBI2_ECC_BUF_CFG, host->ecc_buf_cfg);
> - nandc_set_reg(chip, NAND_FLASH_STATUS, host->clrflashstatus);
> - nandc_set_reg(chip, NAND_READ_STATUS, host->clrreadstatus);
> - nandc_set_reg(chip, NAND_EXEC_CMD, 1);
> + nandc->regs->cmd = cmd;
> + nandc->regs->cfg0 = cfg0;
> + nandc->regs->cfg1 = cfg1;
> + nandc->regs->ecc_bch_cfg = ecc_bch_cfg;
> +
> + if (!nandc->props->qpic_version2)
> + nandc->regs->ecc_buf_cfg = cpu_to_le32(host->ecc_buf_cfg);
> +
> + nandc->regs->clrflashstatus = cpu_to_le32(host->clrflashstatus);
> + nandc->regs->clrreadstatus = cpu_to_le32(host->clrreadstatus);
> + nandc->regs->exec = cpu_to_le32(1);
>
> if (read)
> nandc_set_read_loc(chip, cw, 0, 0, host->use_ecc ?
> @@ -1121,7 +1088,7 @@ static int read_reg_dma(struct qcom_nand_controller *nandc, int first,
> if (first == NAND_DEV_CMD_VLD || first == NAND_DEV_CMD1)
> first = dev_cmd_reg_addr(nandc, first);
>
> - if (nandc->props->is_bam)
> + if (nandc->props->supports_bam)
> return prep_bam_dma_desc_cmd(nandc, true, first, vaddr,
> num_regs, flags);
>
> @@ -1136,25 +1103,16 @@ static int read_reg_dma(struct qcom_nand_controller *nandc, int first,
> * write_reg_dma: prepares a descriptor to write a given number of
> * contiguous registers
> *
> + * @vaddr: contnigeous memory from where register value
> will
Please run a spell checker on your commits.
> + * be written
> * @first: offset of the first register in the contiguous block
> * @num_regs: number of registers to write
> * @flags: flags to control DMA descriptor preparation
> */
> -static int write_reg_dma(struct qcom_nand_controller *nandc, int first,
> - int num_regs, unsigned int flags)
> +static int write_reg_dma(struct qcom_nand_controller *nandc, __le32 *vaddr,
> + int first, int num_regs, unsigned int flags)
> {
> bool flow_control = false;
> - struct nandc_regs *regs = nandc->regs;
> - void *vaddr;
> -
> - vaddr = offset_to_nandc_reg(regs, first);
> -
> - if (first == NAND_ERASED_CW_DETECT_CFG) {
> - if (flags & NAND_ERASED_CW_SET)
> - vaddr = ®s->erased_cw_detect_cfg_set;
> - else
> - vaddr = ®s->erased_cw_detect_cfg_clr;
> - }
>
> if (first == NAND_EXEC_CMD)
> flags |= NAND_BAM_NWD;
> @@ -1165,7 +1123,7 @@ static int write_reg_dma(struct qcom_nand_controller *nandc, int first,
> if (first == NAND_DEV_CMD_VLD_RESTORE || first == NAND_DEV_CMD_VLD)
> first = dev_cmd_reg_addr(nandc, NAND_DEV_CMD_VLD);
>
> - if (nandc->props->is_bam)
> + if (nandc->props->supports_bam)
> return prep_bam_dma_desc_cmd(nandc, false, first, vaddr,
> num_regs, flags);
>
...
> @@ -2872,38 +2823,38 @@ static int qcom_param_page_type_exec(struct nand_chip *chip, const struct nand_
> 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, 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);
Please fix the coding style. The '|' should be at the end of the line.
Thanks,
Miquèl
Powered by blists - more mailing lists