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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 11/12/2024 12:00 AM, Miquel Raynal wrote:

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

Ok
Thanks for the review. Will fix all the comments in next revision.


Thanks,
Miquèl
[Index of Archives]     [Linux Kernel]     [Linux ARM (vger)]     [Linux ARM MSM]     [Linux Omap]     [Linux Arm]     [Linux Tegra]     [Fedora ARM]     [Linux for Samsung SOC]     [eCos]     [Linux Fastboot]     [Gcc Help]     [Git]     [DCCP]     [IETF Announce]     [Security]     [Linux MIPS]     [Yosemite Campsites]

  Powered by Linux