mbox series

[v13,0/8] Add QPIC SPI NAND driver

Message ID 20241030121919.865716-1-quic_mdalam@quicinc.com
Headers show
Series Add QPIC SPI NAND driver | expand

Message

Md Sadre Alam Oct. 30, 2024, 12:19 p.m. UTC 
v13:
 * Added Reviewed-by tag 
 * Added MODULE_DESCRIPTION() macro
 * Added 2024 Qualcomm Innovation Center Copyright
 * Changed return type of qcom_spi_cmd_mapping() from u32 to
   int to fix the kernel test bot warning
 * Changed type of variable cmd in qcom_spi_write_page() from u32
   to int
 * Removed unused variable s_op from qcom_spi_write_page()
 * Updated return value variable type from u32 to int in
   qcom_spi_send_cmdaddr()

v12:
 * Added EXPORT_SYMBOL() macro for all the api in qpic_common.c
 * Added MODULE_LICENSE() macro in qpic_common.c to build
   qpic_common.c as module as well
 * Removed bool type for CONFIG_MTD_NAND_QCOM to fix build error
   reported by kernel test bot
 * Added obj-$(CONFIG_MTD_NAND_QCOM) += qpic_common.o condition
   in Makefile to build qpic_common.c as built-in or as module
   based on CONFIG_MTD_NAND_QCOM
 * Added Reviewed-by tag
 * Added obj-$(CONFIG_SPI_QPIC_SNAND) += qpic_common.o in Makefile
   to build qpic_common.c based on CONFIG_SPI_QPIC_SNAND
 * Updated commit header and commit message
 * Removed sdhci node from rdp433.dts file 

v11:
 * Dropped Reviewed-by tag
 * Added soc based compatible "qcom,ipq9574-snand"
 * fixed build error reported by kernel test bot by
   changing statement "depends on MTD" to "selct MTD"
   in drivers/spi/Kconfig file

v10:
 * Fixed compilation warnings reported by kernel test robot
 * Added depends on CONFIG_MTD for qpic-spi nand driver
 * Removed extra bracket from statement if (i == (num_cw - 1))
   in qcom_spi_program_raw() api.

v9:
 * Fixed all the compilation warning reported by
   kernel test robot
  * Changed type of cmd1, vld to u32 from __le32 in qcom_nand_controller
   structure
 * Changed type of cfg0, cfg1, cfg0_raw, cfg1_raw, clrflashstatus,
   ecc_buf_cfg, ecc_bch_cfg, clrreadstatus to u32 in qcom_nand_host
   structure
 * In nandc_set_read_loc_first() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot
 * In nandc_set_read_loc_last() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot
 * Changed data type of cw_offset, read_size, is_last_read_loc to
   u32 in nandc_set_read_loc() api to fix compilation warning reported
   by kernel test bot
 * In set_address() api added cpu_to_le32() macro to fix compilation
   warning reported by kernel test bot
 * In update_rw_regs() api added cpu_to_le32() macro to fix compilation
   warning reported by kernel test bot
 * In qcom_op_cmd_mapping() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot
 * In qcom_read_status_exec() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot
 * In qcom_read_id_type_exec() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot
 * In qcom_misc_cmd_type_exec() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot
 * In qcom_param_page_type_exec() api added cpu_to_le32() macro to fix
   compilation warning reported by kernel test bot   
 * In update_rw_regs() api added cpu_to_le32() macro to fix compilation
   issue reported by kernel test bot
 * In qcom_param_page_type_exec() api added cpu_to_le32() macro to fix
   compilation issue reported by kernel test bot
 * Changed data type of addr1, addr2, cmd, to __le32 in qpic_spi_nand
   structure
 * In qcom_spi_set_read_loc_first() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_set_read_loc_last() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_init() api added cpu_to_le32() macro to fix compilation
   warning
 * In qcom_spi_ecc_init_ctx_pipelined() api removed unused variables
   reqs, user, step_size, strength and added cpu_to_le32() macro as well
   to fix compilation warning
 * In qcom_spi_read_last_cw() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_check_error() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_read_page_ecc() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_read_page_oob() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_program_raw() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_program_ecc() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_program_oob() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_send_cmdaddr() api added cpu_to_le32() macro to fix
   compilation warning
 * In qcom_spi_io_op() api added cpu_to_le32() macro to fix compilation
    warning
v8:
 * Fixed compilation warning reported by kernel test robot
 * Added "chip" description in nandc_set_read_loc_first()
 * Added "chip" description" in nandc_set_read_loc_last()
 * Changed data type of read_location0, read_location1,
   read_location2, read_location3, addr0, addr1, cmd, cfg0,
   cfg1, ecc_bch_cfg, ecc_buf_cfg, clrflashstatus, clrreadstatus,
   orig_cmd1, orig_vld to __le32 to fix compilation warning.
 * Included bitfield.h header file in spi-qpic-snand.c to
   fix compilation warning
 * Removed unused variable "steps" variable from 
   qcom_spi_ecc_init_ctx_pipelined()

v7:
 * Added read_oob() and write_oob() api
 * Added FIELD_PREP() in spi init
 * Made CONFIG_SPI_QPIC_SNAND and CONFIG_MTD_NAND_QCOM
   as bool type
 * Removed offset 0 in oob_ecc() layout
 * Handled multiple error condition

v6:
 * Added FIELD_PREP() and GENMASK() macro
 * Added qpic_spi_nand{..} structure for
   spi nand realted variables
 * Made qpic_common.c slectable based on
   either CONFIG_MTD_NAND_QCOM or CONFIG_SPI_QPIC_SNAND
 * Removed rawnand.h from qpic-common.h 
 * Removed partitions.h and rawnand.h form spi-qpic-snand.c
 * Added qcom_nand_unalloc() in remove()

v5:
 * Fixes nandbiterr issue
 * Added raw_read() and raw_write() API
 * Added qcom_ prefix to all the common API
 * Removed register indirection
 * Following tests for SPI-NAND devices passed

   - mtd_oobtest
   - mtd_pagetest
   - mtd_readtest
   - mtd_speedtest
   - mtd_stresstest
   - mtd_subpagetest
   - mtd_nandbiterrs
   - nandtest
   - nanddump
   - nandwrite
   - nandbiterr -i
   - mtd erase
   - mtd write
   - dd
   - hexddump

v4:
 * In this patch series fixes kernel doc for all the cmmon api
 * Also fixes dm-binding commit message
 * Fix qpic_common.c compilation based on config

v3:
 * In this patch series fixes multiple things like
   added clock-name, added _alloc_controller api instead
   of alloc_master, made common apis more generic etc.

 * Addressed all the comment from v2 patch series

v2:
 * https://lore.kernel.org/linux-arm-msm/20240215134856.1313239-1-quic_mdalam@quicinc.com/
 * In this series of patchs we have added basic working QPIC SPI NAND
   driver with READ, WRITE, ERASE etc functionality

 * Addressed all the comments given in RFC [v1] patch

v1:
 * https://lore.kernel.org/linux-arm-msm/20231031120307.1600689-1-quic_mdalam@quicinc.com/
 * Initial set of patches for handling QPIC SPI NAND.


Md Sadre Alam (8):
  spi: dt-bindings: Introduce qcom,spi-qpic-snand
  mtd: rawnand: qcom: cleanup qcom_nandc driver
  mtd: rawnand: qcom: Add qcom prefix to common api
  mtd: nand: Add qpic_common API file
  mtd: rawnand: qcom: use FIELD_PREP and GENMASK
  spi: spi-qpic: add driver for QCOM SPI NAND flash Interface
  arm64: dts: qcom: ipq9574: Add SPI nand support
  arm64: dts: qcom: ipq9574: Remove eMMC node

 .../bindings/spi/qcom,spi-qpic-snand.yaml     |   83 +
 .../boot/dts/qcom/ipq9574-rdp-common.dtsi     |   43 +
 arch/arm64/boot/dts/qcom/ipq9574-rdp433.dts   |   12 -
 arch/arm64/boot/dts/qcom/ipq9574.dtsi         |   27 +
 drivers/mtd/nand/Makefile                     |    6 +-
 drivers/mtd/nand/qpic_common.c                |  759 +++++++
 drivers/mtd/nand/raw/qcom_nandc.c             | 1763 +++--------------
 drivers/spi/Kconfig                           |    9 +
 drivers/spi/Makefile                          |    1 +
 drivers/spi/spi-qpic-snand.c                  | 1633 +++++++++++++++
 include/linux/mtd/nand-qpic-common.h          |  482 +++++
 11 files changed, 3365 insertions(+), 1453 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/spi/qcom,spi-qpic-snand.yaml
 create mode 100644 drivers/mtd/nand/qpic_common.c
 create mode 100644 drivers/spi/spi-qpic-snand.c
 create mode 100644 include/linux/mtd/nand-qpic-common.h

Comments

Miquel Raynal Nov. 11, 2024, 6:30 p.m. UTC | #1 
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
Miquel Raynal Nov. 13, 2024, 9:07 a.m. UTC | #2 
Hello,

On 12/11/2024 at 17:45:07 +0530, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:

> On 11/12/2024 12:00 AM, Miquel Raynal wrote:
>> 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:
> 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

In general when the main answer is "okay", you can probably just say a
sentence like "thanks for the comments I'll take them into account",
without answering to each and every comment. This way, it will be easier
for me to catch if you have further remarks.


Thanks,
Miquèl
Md Sadre Alam Nov. 13, 2024, 9:56 a.m. UTC | #3 
On 11/13/2024 2:37 PM, Miquel Raynal wrote:
> Hello,
> 
> On 12/11/2024 at 17:45:07 +0530, Md Sadre Alam <quic_mdalam@quicinc.com> wrote:
> 
>> On 11/12/2024 12:00 AM, Miquel Raynal wrote:
>>> 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:
>> 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
> 
> In general when the main answer is "okay", you can probably just say a
> sentence like "thanks for the comments I'll take them into account",
> without answering to each and every comment. This way, it will be easier
> for me to catch if you have further remarks.
Sorry, I will take care this from next revision onward.
> 
> 
> Thanks,
> Miquèl