Re: [PATCH v4 2/5] drivers: mtd: nand: Add qpic_common API file

[Miquel Raynal <miquel.raynal@xxxxxxxxxxx>]

Hello,

> +/**
> + * qcom_qpic_bam_dma_done() - Callback for DMA descriptor completion
> + * @data: data pointer
> + *
> + * This function is a callback for DMA descriptor completion
> + */
> +void qcom_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);

Can't you just call "wait" and "complete" twice? It's supposed to be
handled by the API. This is totally racy.

> +}
> +
> +/**
> + * qcom_nandc_read_buffer_sync() - Check for dma sync for cpu or device
> + * @nandc: qpic nand controller
> + * @is_cpu: cpu or Device

? the naming is really strange dev_to_mem or something like that would
probably be more helpful.

> + *
> + * This function will check for dma sync for cpu or device
> + */
> +void qcom_nandc_read_buffer_sync(struct qcom_nand_controller *nandc,
> +				 bool is_cpu)
> +{
> +	if (!nandc->props->is_bam)
> +		return;
> +
> +	if (is_cpu)
> +		dma_sync_single_for_cpu(nandc->dev, nandc->reg_read_dma,
> +					MAX_REG_RD *
> +					sizeof(*nandc->reg_read_buf),
> +					DMA_FROM_DEVICE);
> +	else
> +		dma_sync_single_for_device(nandc->dev, nandc->reg_read_dma,
> +					   MAX_REG_RD *
> +					   sizeof(*nandc->reg_read_buf),
> +					   DMA_FROM_DEVICE);
> +}
> +
> +/**
> + * qcom_offset_to_nandc_reg() - Get the actual offset
> + * @regs: pointer to nandc_reg structure
> + * @offset: register offset
> + *
> + * This function will reurn the actual offset for qpic controller register
> + */
> +__le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset)
> +{
> +	switch (offset) {
> +	case NAND_FLASH_CMD:
> +		return &regs->cmd;
> +	case NAND_ADDR0:
> +		return &regs->addr0;
> +	case NAND_ADDR1:
> +		return &regs->addr1;
> +	case NAND_FLASH_CHIP_SELECT:
> +		return &regs->chip_sel;
> +	case NAND_EXEC_CMD:
> +		return &regs->exec;
> +	case NAND_FLASH_STATUS:
> +		return &regs->clrflashstatus;
> +	case NAND_DEV0_CFG0:
> +		return &regs->cfg0;
> +	case NAND_DEV0_CFG1:
> +		return &regs->cfg1;
> +	case NAND_DEV0_ECC_CFG:
> +		return &regs->ecc_bch_cfg;
> +	case NAND_READ_STATUS:
> +		return &regs->clrreadstatus;
> +	case NAND_DEV_CMD1:
> +		return &regs->cmd1;
> +	case NAND_DEV_CMD1_RESTORE:
> +		return &regs->orig_cmd1;
> +	case NAND_DEV_CMD_VLD:
> +		return &regs->vld;
> +	case NAND_DEV_CMD_VLD_RESTORE:
> +		return &regs->orig_vld;
> +	case NAND_EBI2_ECC_BUF_CFG:
> +		return &regs->ecc_buf_cfg;
> +	case NAND_READ_LOCATION_0:
> +		return &regs->read_location0;
> +	case NAND_READ_LOCATION_1:
> +		return &regs->read_location1;
> +	case NAND_READ_LOCATION_2:
> +		return &regs->read_location2;
> +	case NAND_READ_LOCATION_3:
> +		return &regs->read_location3;
> +	case NAND_READ_LOCATION_LAST_CW_0:
> +		return &regs->read_location_last0;
> +	case NAND_READ_LOCATION_LAST_CW_1:
> +		return &regs->read_location_last1;
> +	case NAND_READ_LOCATION_LAST_CW_2:
> +		return &regs->read_location_last2;
> +	case NAND_READ_LOCATION_LAST_CW_3:
> +		return &regs->read_location_last3;

Why do you need this indirection?

> +	default:
> +		return NULL;
> +	}
> +}
> +

...

> +/**
> + * qcom_clear_bam_transaction() - Clears the BAM transaction
> + * @nandc: qpic nand controller
> + *
> + * This function will clear the BAM transaction indexes.
> + */
> +void qcom_clear_bam_transaction(struct qcom_nand_controller *nandc)
> +{
> +	struct bam_transaction *bam_txn = nandc->bam_txn;
> +
> +	if (!nandc->props->is_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;
> +	bam_txn->last_data_desc = NULL;
> +	bam_txn->wait_second_completion = false;

What about using memset here?

> +
> +	sg_init_table(bam_txn->cmd_sgl, nandc->max_cwperpage *
> +		      QPIC_PER_CW_CMD_SGL);
> +	sg_init_table(bam_txn->data_sgl, nandc->max_cwperpage *
> +		      QPIC_PER_CW_DATA_SGL);
> +
> +	reinit_completion(&bam_txn->txn_done);
> +}

...

> diff --git a/include/linux/mtd/nand-qpic-common.h b/include/linux/mtd/nand-qpic-common.h
> new file mode 100644
> index 000000000000..aced15866627
> --- /dev/null
> +++ b/include/linux/mtd/nand-qpic-common.h
> @@ -0,0 +1,486 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * QCOM QPIC common APIs header file
> + *
> + * Copyright (c) 2023 Qualcomm Inc.
> + * Authors:     Md sadre Alam           <quic_mdalam@xxxxxxxxxxx>
> + *		Sricharan R             <quic_srichara@xxxxxxxxxxx>
> + *		Varadarajan Narayanan   <quic_varada@xxxxxxxxxxx>
> + *
> + */
> +#ifndef __MTD_NAND_QPIC_COMMON_H__
> +#define __MTD_NAND_QPIC_COMMON_H__
> +
> +#include <linux/bitops.h>
> +#include <linux/clk.h>
> +#include <linux/delay.h>
> +#include <linux/dmaengine.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/dma/qcom_adm.h>
> +#include <linux/dma/qcom_bam_dma.h>
> +#include <linux/module.h>
> +#include <linux/mtd/partitions.h>
> +#include <linux/mtd/rawnand.h>

You really need this?

> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +/* NANDc reg offsets */
> +#define	NAND_FLASH_CMD			0x00
> +#define	NAND_ADDR0			0x04
> +#define	NAND_ADDR1			0x08
> +#define	NAND_FLASH_CHIP_SELECT		0x0c
> +#define	NAND_EXEC_CMD			0x10
> +#define	NAND_FLASH_STATUS		0x14
> +#define	NAND_BUFFER_STATUS		0x18
> +#define	NAND_DEV0_CFG0			0x20
> +#define	NAND_DEV0_CFG1			0x24
> +#define	NAND_DEV0_ECC_CFG		0x28
> +#define	NAND_AUTO_STATUS_EN		0x2c
> +#define	NAND_DEV1_CFG0			0x30
> +#define	NAND_DEV1_CFG1			0x34
> +#define	NAND_READ_ID			0x40
> +#define	NAND_READ_STATUS		0x44
> +#define	NAND_DEV_CMD0			0xa0
> +#define	NAND_DEV_CMD1			0xa4
> +#define	NAND_DEV_CMD2			0xa8
> +#define	NAND_DEV_CMD_VLD		0xac
> +#define	SFLASHC_BURST_CFG		0xe0
> +#define	NAND_ERASED_CW_DETECT_CFG	0xe8
> +#define	NAND_ERASED_CW_DETECT_STATUS	0xec
> +#define	NAND_EBI2_ECC_BUF_CFG		0xf0
> +#define	FLASH_BUF_ACC			0x100
> +

...

> +/*
> + * This data type corresponds to the NAND controller properties which varies
> + * among different NAND controllers.
> + * @ecc_modes - ecc mode for NAND

Should this member be an enum?

> + * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
> + * @is_bam - whether NAND controller is using BAM

has_bam_support? supports_bam?

> + * @is_qpic - whether NAND CTRL is part of qpic IP

CTRL? do you mean controller?

> + * @qpic_v2 - flag to indicate QPIC IP version 2
> + * @use_codeword_fixup - whether NAND has different layout for boot partitions

The doc is clear but the member name is terrible. Please clarify the
naming.

> + */
> +struct qcom_nandc_props {
> +	u32 ecc_modes;
> +	u32 dev_cmd_reg_start;
> +	bool is_bam;
> +	bool is_qpic;
> +	bool qpic_v2;
> +	bool use_codeword_fixup;
> +};
> +
> +void config_nand_page_read(struct nand_chip *chip);
> +void qcom_qpic_bam_dma_done(void *data);
> +void qcom_nandc_read_buffer_sync(struct qcom_nand_controller *nandc, bool is_cpu);
> +__le32 *qcom_offset_to_nandc_reg(struct nandc_regs *regs, int offset);
> +int qcom_prep_adm_dma_desc(struct qcom_nand_controller *nandc, bool read,
> +			   int reg_off, const void *vaddr, int size,
> +			bool flow_control);
> +int qcom_submit_descs(struct qcom_nand_controller *nandc);
> +int qcom_prepare_bam_async_desc(struct qcom_nand_controller *nandc,
> +				struct dma_chan *chan, unsigned long flags);
> +int qcom_prep_bam_dma_desc_cmd(struct qcom_nand_controller *nandc, bool read,
> +			       int reg_off, const void *vaddr,
> +			int size, unsigned int flags);
> +int qcom_prep_bam_dma_desc_data(struct qcom_nand_controller *nandc, bool read,
> +				const void *vaddr,
> +			int size, unsigned int flags);
> +int qcom_read_reg_dma(struct qcom_nand_controller *nandc, int first,
> +		      int num_regs, unsigned int flags);
> +int qcom_write_reg_dma(struct qcom_nand_controller *nandc, int first,
> +		       int num_regs, unsigned int flags);
> +int qcom_read_data_dma(struct qcom_nand_controller *nandc, int reg_off,
> +		       const u8 *vaddr, int size, unsigned int flags);
> +int qcom_write_data_dma(struct qcom_nand_controller *nandc, int reg_off,
> +			const u8 *vaddr, int size, unsigned int flags);
> +struct bam_transaction *qcom_alloc_bam_transaction(struct qcom_nand_controller *nandc);
> +void qcom_clear_bam_transaction(struct qcom_nand_controller *nandc);
> +void qcom_nandc_unalloc(struct qcom_nand_controller *nandc);
> +int qcom_nandc_alloc(struct qcom_nand_controller *nandc);
> +void qcom_clear_read_regs(struct qcom_nand_controller *nandc);
> +void qcom_free_bam_transaction(struct qcom_nand_controller *nandc);
> +#endif

I made several requests on code that already exists, please add these
changes to your series.


Also, this patching being big, please split:
1- rename your all your symbols to start with the same prefix
(qcom_nand_ instead of nothing or just qcom)
2- then perform the move, which should not require changing the names
of all the functions everywhere.

Thanks,
Miquèl