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 ®s->cmd;
> + case NAND_ADDR0:
> + return ®s->addr0;
> + case NAND_ADDR1:
> + return ®s->addr1;
> + case NAND_FLASH_CHIP_SELECT:
> + return ®s->chip_sel;
> + case NAND_EXEC_CMD:
> + return ®s->exec;
> + case NAND_FLASH_STATUS:
> + return ®s->clrflashstatus;
> + case NAND_DEV0_CFG0:
> + return ®s->cfg0;
> + case NAND_DEV0_CFG1:
> + return ®s->cfg1;
> + case NAND_DEV0_ECC_CFG:
> + return ®s->ecc_bch_cfg;
> + case NAND_READ_STATUS:
> + return ®s->clrreadstatus;
> + case NAND_DEV_CMD1:
> + return ®s->cmd1;
> + case NAND_DEV_CMD1_RESTORE:
> + return ®s->orig_cmd1;
> + case NAND_DEV_CMD_VLD:
> + return ®s->vld;
> + case NAND_DEV_CMD_VLD_RESTORE:
> + return ®s->orig_vld;
> + case NAND_EBI2_ECC_BUF_CFG:
> + return ®s->ecc_buf_cfg;
> + case NAND_READ_LOCATION_0:
> + return ®s->read_location0;
> + case NAND_READ_LOCATION_1:
> + return ®s->read_location1;
> + case NAND_READ_LOCATION_2:
> + return ®s->read_location2;
> + case NAND_READ_LOCATION_3:
> + return ®s->read_location3;
> + case NAND_READ_LOCATION_LAST_CW_0:
> + return ®s->read_location_last0;
> + case NAND_READ_LOCATION_LAST_CW_1:
> + return ®s->read_location_last1;
> + case NAND_READ_LOCATION_LAST_CW_2:
> + return ®s->read_location_last2;
> + case NAND_READ_LOCATION_LAST_CW_3:
> + return ®s->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
