linux-kernel - Re: [PATCH v4 2/5] drivers: mtd: nand: Add qpic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <93b08226-3297-2161-cc7d-d33d839c32f0@quicinc.com>
Date: Tue, 19 Mar 2024 15:55:20 +0530
From: Md Sadre Alam <quic_mdalam@...cinc.com>
To: Miquel Raynal <miquel.raynal@...tlin.com>
CC: <andersson@...nel.org>, <konrad.dybcio@...aro.org>, <broonie@...nel.org>,
        <robh@...nel.org>, <krzysztof.kozlowski+dt@...aro.org>,
        <conor+dt@...nel.org>, <richard@....at>, <vigneshr@...com>,
        <manivannan.sadhasivam@...aro.org>, <neil.armstrong@...aro.org>,
        <daniel@...rotopia.org>, <arnd@...db.de>,
        <chris.packham@...iedtelesis.co.nz>, <christophe.kerello@...s.st.com>,
        <linux-arm-msm@...r.kernel.org>, <linux-spi@...r.kernel.org>,
        <devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <linux-mtd@...ts.infradead.org>, <quic_srichara@...cinc.com>,
        <quic_varada@...cinc.com>
Subject: Re: [PATCH v4 2/5] drivers: mtd: nand: Add qpic_common API file



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@...cinc.com>
>> + *		Sricharan R             <quic_srichara@...cinc.com>
>> + *		Varadarajan Narayanan   <quic_varada@...cinc.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.