Re: [PATCH v5 4/7] dmaengine: ptdma: Extend ptdma to support multi-channel and version

[Basavaraj Natikar <bnatikar@amd.com>]

On 8/28/2024 11:16 PM, Vinod Koul wrote:
> On 08-07-24, 20:14, Basavaraj Natikar wrote:
>> To support multi-channel functionality with AE4DMA engine, extend the
>> PTDMA code with reusable components.
>>
>> Reviewed-by: Raju Rangoju <Raju.Rangoju@amd.com>
>> Signed-off-by: Basavaraj Natikar <Basavaraj.Natikar@amd.com>
>> ---
>>  drivers/dma/amd/ae4dma/ae4dma.h         |   2 +
>>  drivers/dma/amd/common/amd_dma.h        |   1 +
>>  drivers/dma/amd/ptdma/ptdma-dmaengine.c | 105 +++++++++++++++++++-----
>>  drivers/dma/amd/ptdma/ptdma.h           |   2 +
>>  4 files changed, 90 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/dma/amd/ae4dma/ae4dma.h b/drivers/dma/amd/ae4dma/ae4dma.h
>> index a63525792080..5f9dab5f05f4 100644
>> --- a/drivers/dma/amd/ae4dma/ae4dma.h
>> +++ b/drivers/dma/amd/ae4dma/ae4dma.h
>> @@ -24,6 +24,8 @@
>>  #define AE4_Q_BASE_H_OFF		0x1C
>>  #define AE4_Q_SZ			0x20
>>  
>> +#define AE4_DMA_VERSION			4
> Can it be driver data for the device?
> I am not seeing who sets this here?

This will be used in the functions below to
differentiate between PTDMA and AE4DMA functionality.
By default, without a version, it is PTDMA, and this
will be set in ae4_pci_probe in the next patch: 
https://lore.kernel.org/all/20240708144500.1523651-6-Basavaraj.Natikar@amd.com/#r.

>
>> +
>>  struct ae4_msix {
>>  	int msix_count;
>>  	struct msix_entry msix_entry[MAX_AE4_HW_QUEUES];
>> diff --git a/drivers/dma/amd/common/amd_dma.h b/drivers/dma/amd/common/amd_dma.h
>> index f9f396cd4371..396667e81e1a 100644
>> --- a/drivers/dma/amd/common/amd_dma.h
>> +++ b/drivers/dma/amd/common/amd_dma.h
>> @@ -21,6 +21,7 @@
>>  #include <linux/wait.h>
>>  
>>  #include "../ptdma/ptdma.h"
>> +#include "../ae4dma/ae4dma.h"
>>  #include "../../virt-dma.h"
>>  
>>  #endif
>> diff --git a/drivers/dma/amd/ptdma/ptdma-dmaengine.c b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>> index 66ea10499643..90ca02fd5f8f 100644
>> --- a/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>> +++ b/drivers/dma/amd/ptdma/ptdma-dmaengine.c
>> @@ -43,7 +43,24 @@ static void pt_do_cleanup(struct virt_dma_desc *vd)
>>  	kmem_cache_free(pt->dma_desc_cache, desc);
>>  }
>>  
>> -static int pt_dma_start_desc(struct pt_dma_desc *desc)
>> +static struct pt_cmd_queue *pt_get_cmd_queue(struct pt_device *pt, struct pt_dma_chan *chan)
>> +{
>> +	struct ae4_cmd_queue *ae4cmd_q;
>> +	struct pt_cmd_queue *cmd_q;
>> +	struct ae4_device *ae4;
>> +
>> +	if (pt->ver == AE4_DMA_VERSION) {
>> +		ae4 = container_of(pt, struct ae4_device, pt);
>> +		ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>> +		cmd_q = &ae4cmd_q->cmd_q;
>> +	} else {
>> +		cmd_q = &pt->cmd_q;
>> +	}
>> +
>> +	return cmd_q;
>> +}
>> +
>> +static int pt_dma_start_desc(struct pt_dma_desc *desc, struct pt_dma_chan *chan)
>>  {
>>  	struct pt_passthru_engine *pt_engine;
>>  	struct pt_device *pt;
>> @@ -54,7 +71,9 @@ static int pt_dma_start_desc(struct pt_dma_desc *desc)
>>  
>>  	pt_cmd = &desc->pt_cmd;
>>  	pt = pt_cmd->pt;
>> -	cmd_q = &pt->cmd_q;
>> +
>> +	cmd_q = pt_get_cmd_queue(pt, chan);
>> +
>>  	pt_engine = &pt_cmd->passthru;
>>  
>>  	pt->tdata.cmd = pt_cmd;
>> @@ -149,7 +168,7 @@ static void pt_cmd_callback(void *data, int err)
>>  		if (!desc)
>>  			break;
>>  
>> -		ret = pt_dma_start_desc(desc);
>> +		ret = pt_dma_start_desc(desc, chan);
>>  		if (!ret)
>>  			break;
>>  
>> @@ -184,7 +203,10 @@ static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>>  {
>>  	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>  	struct pt_passthru_engine *pt_engine;
>> +	struct pt_device *pt = chan->pt;
>> +	struct ae4_cmd_queue *ae4cmd_q;
>>  	struct pt_dma_desc *desc;
>> +	struct ae4_device *ae4;
>>  	struct pt_cmd *pt_cmd;
>>  
>>  	desc = pt_alloc_dma_desc(chan, flags);
>> @@ -192,7 +214,7 @@ static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>>  		return NULL;
>>  
>>  	pt_cmd = &desc->pt_cmd;
>> -	pt_cmd->pt = chan->pt;
>> +	pt_cmd->pt = pt;
>>  	pt_engine = &pt_cmd->passthru;
>>  	pt_cmd->engine = PT_ENGINE_PASSTHRU;
>>  	pt_engine->src_dma = src;
>> @@ -203,6 +225,14 @@ static struct pt_dma_desc *pt_create_desc(struct dma_chan *dma_chan,
>>  
>>  	desc->len = len;
>>  
>> +	if (pt->ver == AE4_DMA_VERSION) {
>> +		ae4 = container_of(pt, struct ae4_device, pt);
>> +		ae4cmd_q = &ae4->ae4cmd_q[chan->id];
>> +		mutex_lock(&ae4cmd_q->cmd_lock);
>> +		list_add_tail(&pt_cmd->entry, &ae4cmd_q->cmd);
>> +		mutex_unlock(&ae4cmd_q->cmd_lock);
>> +	}
>> +
>>  	return desc;
>>  }
>>  
>> @@ -260,8 +290,11 @@ static enum dma_status
>>  pt_tx_status(struct dma_chan *c, dma_cookie_t cookie,
>>  		struct dma_tx_state *txstate)
>>  {
>> -	struct pt_device *pt = to_pt_chan(c)->pt;
>> -	struct pt_cmd_queue *cmd_q = &pt->cmd_q;
>> +	struct pt_dma_chan *chan = to_pt_chan(c);
>> +	struct pt_device *pt = chan->pt;
>> +	struct pt_cmd_queue *cmd_q;
>> +
>> +	cmd_q = pt_get_cmd_queue(pt, chan);
>>  
>>  	pt_check_status_trans(pt, cmd_q);
>>  	return dma_cookie_status(c, cookie, txstate);
>> @@ -270,10 +303,13 @@ pt_tx_status(struct dma_chan *c, dma_cookie_t cookie,
>>  static int pt_pause(struct dma_chan *dma_chan)
>>  {
>>  	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> +	struct pt_device *pt = chan->pt;
>> +	struct pt_cmd_queue *cmd_q;
>>  	unsigned long flags;
>>  
>>  	spin_lock_irqsave(&chan->vc.lock, flags);
>> -	pt_stop_queue(&chan->pt->cmd_q);
>> +	cmd_q = pt_get_cmd_queue(pt, chan);
>> +	pt_stop_queue(cmd_q);
>>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>>  
>>  	return 0;
>> @@ -283,10 +319,13 @@ static int pt_resume(struct dma_chan *dma_chan)
>>  {
>>  	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>>  	struct pt_dma_desc *desc = NULL;
>> +	struct pt_device *pt = chan->pt;
>> +	struct pt_cmd_queue *cmd_q;
>>  	unsigned long flags;
>>  
>>  	spin_lock_irqsave(&chan->vc.lock, flags);
>> -	pt_start_queue(&chan->pt->cmd_q);
>> +	cmd_q = pt_get_cmd_queue(pt, chan);
>> +	pt_start_queue(cmd_q);
>>  	desc = pt_next_dma_desc(chan);
>>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>>  
>> @@ -300,11 +339,17 @@ static int pt_resume(struct dma_chan *dma_chan)
>>  static int pt_terminate_all(struct dma_chan *dma_chan)
>>  {
>>  	struct pt_dma_chan *chan = to_pt_chan(dma_chan);
>> +	struct pt_device *pt = chan->pt;
>> +	struct pt_cmd_queue *cmd_q;
>>  	unsigned long flags;
>> -	struct pt_cmd_queue *cmd_q = &chan->pt->cmd_q;
>>  	LIST_HEAD(head);
>>  
>> -	iowrite32(SUPPORTED_INTERRUPTS, cmd_q->reg_control + 0x0010);
>> +	cmd_q = pt_get_cmd_queue(pt, chan);
>> +	if (pt->ver == AE4_DMA_VERSION)
>> +		pt_stop_queue(cmd_q);
>> +	else
>> +		iowrite32(SUPPORTED_INTERRUPTS, cmd_q->reg_control + 0x0010);
>> +
>>  	spin_lock_irqsave(&chan->vc.lock, flags);
>>  	vchan_get_all_descriptors(&chan->vc, &head);
>>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>> @@ -317,14 +362,24 @@ static int pt_terminate_all(struct dma_chan *dma_chan)
>>  
>>  int pt_dmaengine_register(struct pt_device *pt)
>>  {
>> -	struct pt_dma_chan *chan;
>>  	struct dma_device *dma_dev = &pt->dma_dev;
>> -	char *cmd_cache_name;
>> +	struct ae4_cmd_queue *ae4cmd_q = NULL;
>> +	struct ae4_device *ae4 = NULL;
>> +	struct pt_dma_chan *chan;
>>  	char *desc_cache_name;
>> -	int ret;
>> +	char *cmd_cache_name;
>> +	int ret, i;
>> +
>> +	if (pt->ver == AE4_DMA_VERSION)
>> +		ae4 = container_of(pt, struct ae4_device, pt);
>> +
>> +	if (ae4)
> in which case would ae4 be null but you are on version 4?

Yes correct, this will be used in the functions to differentiate
between PTDMA and AE4DMA functionality. By default, without a
version (i.e., null for PTDMA and 4 for AE4DMA).

Thanks,
--
Basavaraj

>
>> +		pt->pt_dma_chan = devm_kcalloc(pt->dev, ae4->cmd_q_count,
>> +					       sizeof(*pt->pt_dma_chan), GFP_KERNEL);
>> +	else
>> +		pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
>> +					       GFP_KERNEL);
>>  
>> -	pt->pt_dma_chan = devm_kzalloc(pt->dev, sizeof(*pt->pt_dma_chan),
>> -				       GFP_KERNEL);
>>  	if (!pt->pt_dma_chan)
>>  		return -ENOMEM;
>>  
>> @@ -366,9 +421,6 @@ int pt_dmaengine_register(struct pt_device *pt)
>>  
>>  	INIT_LIST_HEAD(&dma_dev->channels);
>>  
>> -	chan = pt->pt_dma_chan;
>> -	chan->pt = pt;
>> -
>>  	/* Set base and prep routines */
>>  	dma_dev->device_free_chan_resources = pt_free_chan_resources;
>>  	dma_dev->device_prep_dma_memcpy = pt_prep_dma_memcpy;
>> @@ -380,8 +432,21 @@ int pt_dmaengine_register(struct pt_device *pt)
>>  	dma_dev->device_terminate_all = pt_terminate_all;
>>  	dma_dev->device_synchronize = pt_synchronize;
>>  
>> -	chan->vc.desc_free = pt_do_cleanup;
>> -	vchan_init(&chan->vc, dma_dev);
>> +	if (ae4) {
>> +		for (i = 0; i < ae4->cmd_q_count; i++) {
>> +			chan = pt->pt_dma_chan + i;
>> +			ae4cmd_q = &ae4->ae4cmd_q[i];
>> +			chan->id = ae4cmd_q->id;
>> +			chan->pt = pt;
>> +			chan->vc.desc_free = pt_do_cleanup;
>> +			vchan_init(&chan->vc, dma_dev);
>> +		}
>> +	} else {
>> +		chan = pt->pt_dma_chan;
>> +		chan->pt = pt;
>> +		chan->vc.desc_free = pt_do_cleanup;
>> +		vchan_init(&chan->vc, dma_dev);
>> +	}
>>  
>>  	ret = dma_async_device_register(dma_dev);
>>  	if (ret)
>> diff --git a/drivers/dma/amd/ptdma/ptdma.h b/drivers/dma/amd/ptdma/ptdma.h
>> index 2690a32fc7cb..a6990021fe2b 100644
>> --- a/drivers/dma/amd/ptdma/ptdma.h
>> +++ b/drivers/dma/amd/ptdma/ptdma.h
>> @@ -184,6 +184,7 @@ struct pt_dma_desc {
>>  struct pt_dma_chan {
>>  	struct virt_dma_chan vc;
>>  	struct pt_device *pt;
>> +	u32 id;
>>  };
>>  
>>  struct pt_cmd_queue {
>> @@ -262,6 +263,7 @@ struct pt_device {
>>  	unsigned long total_interrupts;
>>  
>>  	struct pt_tasklet_data tdata;
>> +	int ver;
>>  };
>>  
>>  /*
>> -- 
>> 2.25.1