| 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: <374ea155-0970-38bd-470f-cc440ca0bab5@quicinc.com>
Date: Tue, 12 Nov 2024 17:45:07 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Miquel Raynal <miquel.raynal@...tlin.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 11/12/2024 12:00 AM, Miquel Raynal wrote:
> 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:
Ok
>>
>> - Remove register value indirection api
>
> API
Ok
>
>>
>> - Remove set_reg() api
>
> API
Ok
>
>>
>> - Convert read_loc_first & read_loc_last macro to function
>
> functions
Ok
>
>>
>> - Renamed multiple variables
>
> Rename
Ok
>
> ...
>
>> @@ -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
Ok
>
>> + * @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.
Ok
>
>> {
>> - 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
Ok
>
>
>> +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.
Ok
>
>> + * 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.
Ok
Thanks for the review. Will fix all the comments in next revision.
>
> Thanks,
> Miquèl
Powered by blists - more mailing lists