Re: [PATCH v13 2/8] mtd: rawnand: qcom: cleanup qcom_nandc driver

From: Miquel Raynal <miquel.raynal@bootlin.com>
To: Md Sadre Alam <quic_mdalam@quicinc.com>
Cc: <broonie@kernel.org>,  <robh@kernel.org>,  <krzk+dt@kernel.org>,
	<conor+dt@kernel.org>,  <andersson@kernel.org>,
	<konradybcio@kernel.org>,  <richard@nod.at>,  <vigneshr@ti.com>,
	<manivannan.sadhasivam@linaro.org>,
	 <linux-arm-msm@vger.kernel.org>, <linux-spi@vger.kernel.org>,
	 <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	 <linux-mtd@lists.infradead.org>, <quic_srichara@quicinc.com>,
	 <quic_varada@quicinc.com>
Subject: Re: [PATCH v13 2/8] mtd: rawnand: qcom: cleanup qcom_nandc driver
Date: Mon, 11 Nov 2024 19:30:57 +0100	[thread overview]
Message-ID: <871pzh397j.fsf@bootlin.com> (raw)
In-Reply-To: <20241030121919.865716-3-quic_mdalam@quicinc.com> (Md Sadre Alam's message of "Wed, 30 Oct 2024 17:49:13 +0530")

On 30/10/2024 at 17:49:13 +0530, Md Sadre Alam <quic_mdalam@quicinc.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 = &regs->erased_cw_detect_cfg_set;
> -		else
> -			vaddr = &regs->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