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

Md Sadre Alam
2024-03-19T10:25:56Z

On 3/15/2024 5:15 PM, Miquel Raynal wrote:
> 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.
Ok
>
>> +}
>> +
>> +/**
>> + * 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.
Ok
>
>> + *
>> + * 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?

This indirection I believe is needed by the write_reg_dma function,
wherein a bunch of registers are modified based on a starting register.
Can I change this in a separate cleanup series as a follow up to this?

>
>> + 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?
Ok
>
>> +
>> + 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@quicinc.com>
>> + * Sricharan R <quic_srichara@quicinc.com>
>> + * Varadarajan Narayanan <quic_varada@quicinc.com>
>> + *
>> + */
>> +#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?
Yes , since some generic structure used here.
>
>> +#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?
Ok , Will fix in next patch
>
>> + * @dev_cmd_reg_start - NAND_DEV_CMD_* registers starting offset
>> + * @is_bam - whether NAND controller is using BAM
>
> has_bam_support? supports_bam?
Ok
>
>> + * @is_qpic - whether NAND CTRL is part of qpic IP
>
> CTRL? do you mean controller?
Yes.
>
>> + * @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.
Ok
>
>> + */
>> +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.
ok
>
>
> 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)
Ok
> 2- then perform the move, which should not require changing the names
> of all the functions everywhere.
Ok
>
> Thanks,
> Miquèl

Thanks for reviewing. Will address all the comments in next patch series.

Reagrds,
Alam.