Received-SPF: pass (google.com: domain of linux-kernel+bounces-10919-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <6e0a104a-1f99-4686-ba76-99ef631b0d25@linux.alibaba.com>
Date: Mon, 25 Dec 2023 15:05:21 +0800
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH V2 2/2] dmaengine: sprd: optimize two stage transfer
 function
To: Kaiwei Liu <kaiwei.liu@unisoc.com>, Vinod Koul <vkoul@kernel.org>,
 Orson Zhai <orsonzhai@gmail.com>, Chunyan Zhang <zhang.lyra@gmail.com>
Cc: dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org,
 kaiwei liu <liukaiwei086@gmail.com>, Wenming Wu <wenming.wu@unisoc.com>
References: <20231222112746.9720-1-kaiwei.liu@unisoc.com>
From: Baolin Wang <baolin.wang@linux.alibaba.com>
In-Reply-To: <20231222112746.9720-1-kaiwei.liu@unisoc.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


On 12/22/2023 7:27 PM, Kaiwei Liu wrote:
> From: "kaiwei.liu" <kaiwei.liu@unisoc.com>
> 
> For SPRD DMA, it provides a function that one channel can start
> the second channel after completing the transmission, which we
> call two stage transfer mode. You can choose which channel can
> generate interrupt when finished. It can support up to two sets
> of such patterns.
> When configuring registers for two stage transfer mode, we need
> to set the mask bit to ensure that the setting are accurate. And
> we should clear the two stage transfer configuration when release
> DMA channel.
> The two stage transfer function is mainly used by SPRD audio, and
> now audio also requires that the data need to be accessed on the
> device side. So here use the src_port_window_size and dst_port_win-
> dow_size in the struct of dma_slave_config.
> 
> Signed-off-by: kaiwei.liu <kaiwei.liu@unisoc.com>

It seems you ignored my previous comments[1], please make sure they are 
addressed firstly.

[1] 
https://lore.kernel.org/all/522e9d29-fab2-5bb0-c2d3-9cf908007000@linux.alibaba.com/

> ---
> Change in V2
> -change because [PATCH 1/2]
> ---
>   drivers/dma/sprd-dma.c | 116 ++++++++++++++++++++++++-----------------
>   1 file changed, 69 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index cb48731d70b2..e9e113142fd2 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -68,6 +68,7 @@
>   #define SPRD_DMA_GLB_TRANS_DONE_TRG	BIT(18)
>   #define SPRD_DMA_GLB_BLOCK_DONE_TRG	BIT(17)
>   #define SPRD_DMA_GLB_FRAG_DONE_TRG	BIT(16)
> +#define SPRD_DMA_GLB_TRG_MASK		GENMASK(19, 16)
>   #define SPRD_DMA_GLB_TRG_OFFSET		16
>   #define SPRD_DMA_GLB_DEST_CHN_MASK	GENMASK(13, 8)
>   #define SPRD_DMA_GLB_DEST_CHN_OFFSET	8
> @@ -155,6 +156,13 @@
>   
>   #define SPRD_DMA_SOFTWARE_UID		0
>   
> +#define SPRD_DMA_SRC_CHN0_INT		9
> +#define SPRD_DMA_SRC_CHN1_INT		10
> +#define SPRD_DMA_DST_CHN0_INT		11
> +#define SPRD_DMA_DST_CHN1_INT		12
> +#define SPRD_DMA_2STAGE_SET		1
> +#define SPRD_DMA_2STAGE_CLEAR		0
> +
>   /* dma data width values */
>   enum sprd_dma_datawidth {
>   	SPRD_DMA_DATAWIDTH_1_BYTE,
> @@ -431,53 +439,57 @@ static enum sprd_dma_req_mode sprd_dma_get_req_type(struct sprd_dma_chn *schan)
>   	return (frag_reg >> SPRD_DMA_REQ_MODE_OFFSET) & SPRD_DMA_REQ_MODE_MASK;
>   }
>   
> -static int sprd_dma_set_2stage_config(struct sprd_dma_chn *schan)
> +static void sprd_dma_2stage_write(struct sprd_dma_chn *schan,
> +				  u32 config_type, u32 grp_offset)
>   {
>   	struct sprd_dma_dev *sdev = to_sprd_dma_dev(&schan->vc.chan);
> -	u32 val, chn = schan->chn_num + 1;
> -
> -	switch (schan->chn_mode) {
> -	case SPRD_DMA_SRC_CHN0:
> -		val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> -		val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> -		val |= SPRD_DMA_GLB_2STAGE_EN;
> -		if (schan->int_type != SPRD_DMA_NO_INT)
> -			val |= SPRD_DMA_GLB_SRC_INT;
> -
> -		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> -		break;
> -
> -	case SPRD_DMA_SRC_CHN1:
> -		val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> -		val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> -		val |= SPRD_DMA_GLB_2STAGE_EN;
> -		if (schan->int_type != SPRD_DMA_NO_INT)
> -			val |= SPRD_DMA_GLB_SRC_INT;
> -
> -		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> -		break;
> -
> -	case SPRD_DMA_DST_CHN0:
> -		val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> -			SPRD_DMA_GLB_DEST_CHN_MASK;
> -		val |= SPRD_DMA_GLB_2STAGE_EN;
> -		if (schan->int_type != SPRD_DMA_NO_INT)
> -			val |= SPRD_DMA_GLB_DEST_INT;
> -
> -		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP1, val, val);
> -		break;
> -
> -	case SPRD_DMA_DST_CHN1:
> -		val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> -			SPRD_DMA_GLB_DEST_CHN_MASK;
> -		val |= SPRD_DMA_GLB_2STAGE_EN;
> -		if (schan->int_type != SPRD_DMA_NO_INT)
> -			val |= SPRD_DMA_GLB_DEST_INT;
> -
> -		sprd_dma_glb_update(sdev, SPRD_DMA_GLB_2STAGE_GRP2, val, val);
> -		break;
> +	u32 mask_val;
> +	u32 chn = schan->chn_num + 1;
> +	u32 val = 0;
> +
> +	if (config_type == SPRD_DMA_2STAGE_SET) {
> +		if (schan->chn_mode == SPRD_DMA_SRC_CHN0 ||
> +		    schan->chn_mode == SPRD_DMA_SRC_CHN1) {
> +			val = chn & SPRD_DMA_GLB_SRC_CHN_MASK;
> +			val |= BIT(schan->trg_mode - 1) << SPRD_DMA_GLB_TRG_OFFSET;
> +			val |= SPRD_DMA_GLB_2STAGE_EN;
> +			if (schan->int_type & SPRD_DMA_SRC_CHN0_INT ||
> +			    schan->int_type & SPRD_DMA_SRC_CHN1_INT)
> +				val |= SPRD_DMA_GLB_SRC_INT;
> +			mask_val = SPRD_DMA_GLB_SRC_INT | SPRD_DMA_GLB_TRG_MASK |
> +				   SPRD_DMA_GLB_SRC_CHN_MASK;
> +		} else {
> +			val = (chn << SPRD_DMA_GLB_DEST_CHN_OFFSET) &
> +			       SPRD_DMA_GLB_DEST_CHN_MASK;
> +			val |= SPRD_DMA_GLB_2STAGE_EN;
> +			if (schan->int_type & SPRD_DMA_DST_CHN0_INT ||
> +			    schan->int_type & SPRD_DMA_DST_CHN1_INT)
> +				val |= SPRD_DMA_GLB_DEST_INT;
> +			mask_val = SPRD_DMA_GLB_DEST_INT | SPRD_DMA_GLB_DEST_CHN_MASK;
> +		}
> +	} else {
> +		if (schan->chn_mode == SPRD_DMA_SRC_CHN0 ||
> +		    schan->chn_mode == SPRD_DMA_SRC_CHN1)
> +			mask_val = SPRD_DMA_GLB_SRC_INT | SPRD_DMA_GLB_TRG_MASK |
> +				   SPRD_DMA_GLB_2STAGE_EN | SPRD_DMA_GLB_SRC_CHN_MASK;
> +		else
> +			mask_val = SPRD_DMA_GLB_DEST_INT | SPRD_DMA_GLB_2STAGE_EN |
> +				   SPRD_DMA_GLB_DEST_CHN_MASK;
> +	}
> +	sprd_dma_glb_update(sdev, grp_offset, mask_val, val);
> +}
>   
> -	default:
> +static int sprd_dma_2stage_config(struct sprd_dma_chn *schan, u32 config_type)
> +{
> +	struct sprd_dma_dev *sdev = to_sprd_dma_dev(&schan->vc.chan);
> +
> +	if (schan->chn_mode == SPRD_DMA_SRC_CHN0 ||
> +	    schan->chn_mode == SPRD_DMA_DST_CHN0)
> +		sprd_dma_2stage_write(schan, config_type, SPRD_DMA_GLB_2STAGE_GRP1);
> +	else if (schan->chn_mode == SPRD_DMA_SRC_CHN1 ||
> +		 schan->chn_mode == SPRD_DMA_DST_CHN1)
> +		sprd_dma_2stage_write(schan, config_type, SPRD_DMA_GLB_2STAGE_GRP2);
> +	else {
>   		dev_err(sdev->dma_dev.dev, "invalid channel mode setting %d\n",
>   			schan->chn_mode);
>   		return -EINVAL;
> @@ -545,7 +557,7 @@ static void sprd_dma_start(struct sprd_dma_chn *schan)
>   	 * Set 2-stage configuration if the channel starts one 2-stage
>   	 * transfer.
>   	 */
> -	if (schan->chn_mode && sprd_dma_set_2stage_config(schan))
> +	if (schan->chn_mode && sprd_dma_2stage_config(schan, SPRD_DMA_2STAGE_SET))
>   		return;
>   
>   	/*
> @@ -569,6 +581,12 @@ static void sprd_dma_stop(struct sprd_dma_chn *schan)
>   	sprd_dma_set_pending(schan, false);
>   	sprd_dma_unset_uid(schan);
>   	sprd_dma_clear_int(schan);
> +	/*
> +	 * If 2-stage transfer is used, the configuration must be clear
> +	 * when release DMA channel.
> +	 */
> +	if (schan->chn_mode)
> +		sprd_dma_2stage_config(schan, SPRD_DMA_2STAGE_CLEAR);
>   	schan->cur_desc = NULL;
>   }
>   
> @@ -757,7 +775,9 @@ static int sprd_dma_fill_desc(struct dma_chan *chan,
>   	phys_addr_t llist_ptr;
>   
>   	if (dir == DMA_MEM_TO_DEV) {
> -		src_step = sprd_dma_get_step(slave_cfg->src_addr_width);
> +		src_step = slave_cfg->src_port_window_size ?
> +			   slave_cfg->src_port_window_size :
> +			   sprd_dma_get_step(slave_cfg->src_addr_width);
>   		if (src_step < 0) {
>   			dev_err(sdev->dma_dev.dev, "invalid source step\n");
>   			return src_step;
> @@ -773,7 +793,9 @@ static int sprd_dma_fill_desc(struct dma_chan *chan,
>   		else
>   			dst_step = SPRD_DMA_NONE_STEP;
>   	} else {
> -		dst_step = sprd_dma_get_step(slave_cfg->dst_addr_width);
> +		dst_step = slave_cfg->dst_port_window_size ?
> +			   slave_cfg->dst_port_window_size :
> +			   sprd_dma_get_step(slave_cfg->dst_addr_width);
>   		if (dst_step < 0) {
>   			dev_err(sdev->dma_dev.dev, "invalid destination step\n");
>   			return dst_step;