RE: [PATCH RFC] dmaengine: dw-edma: Decouple dw-edma-core.c from struct pci_dev

[Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>]

Hi Alan,

On Wed, Apr 15, 2020 at 3:7:44, Alan Mikhak <alan.mikhak@sifive.com> 
wrote:

> From: Alan Mikhak <alan.mikhak@sifive.com>
> 
> While reviewing the Synopsys Designware eDMA PCIe driver, I came across

s/Designware/DesignWare

> the following field of struct dw_edma in dw-edma-core.h:
> 
> const struct dw_edma_core_ops	*ops;
> 
> I was unable to find a definition for struct dw_edma_core_ops. It was
> surprising that the kernel would build without failure even though the
> dw-edma driver was enabled with what seems to be an undefined struct.

Initially, that struct was aimed to have a set of function callbacks that 
would allow being set differently according to the eDMA version. At the 
time it was expected to have multiple cores that would need to execute 
different code sequences.

However, this approach was requested to be postponed on the patch review 
because at that time and even now there isn't an extra core that would 
justify this.

You caught some residue of that initial approach.

> The reason I was reviewing the dw-edma driver was to see if I could
> integrate it with pcitest to initiate dma operations from endpoint side.

Great! That task was on my backlog for a very long time that I wasn't 
able to develop due the lack of time.

> As I understand from reviewing the dw-edma code, it is designed to run
> on the host side of PCIe link to initiate DMA operations remotely using
> eDMA channels of PCIe controller on the endpoint side. I am not sure if
> my understanding is correct. I infer this from seeing that dw-edma uses
> struct pci_dev and accesses hardware registers of dma channels across the
> bus using BAR0 and BAR2.

You're correct.

> I was exploring what would need to change in dw-edma to integrate it with
> pci-epf-test to initiate dma operations locally from the endpoint side.
> One barrier I see is dw_edma_probe() and other functions in dw-edma-core.c
> depend on struct pci_dev. Hence, instead of posting a patch to remove the
> undefined and unused ops field, I would like to define the struct and use
> it to decouple dw-edma-core.c from struct pci_dev.
> 
> To my surprise, the kernel still builds without error even after defining
> struct dw_edma_core_ops in dw-edma-core.h and using it as in this patch.
> 
> I would appreciate any comments on my observations about the 'ops' field,
> decoupling dw-edma-core.c from struct pci_dev, and how best to use
> dw-edma with pcitest.

I like your approach, it separates the PCIe glue logic from the eDMA 
itself.
I would suggest that pcitest would have multiple options that could be 
triggered, for instance:
 1 - Execute Endpoint DMA (read/write) remotely with Linked List feature 
(from the Root Complex side)
 2 - Execute Endpoint DMA (read/write) remotely without Linked List 
feature (from the Root Complex side)
 3 - Execute Endpoint DMA (read/write) locally with Linked List feature
 4 - Execute Endpoint DMA (read/write) locally without Linked List 
feature

Also, don't forget the DMA has of having multiple channels for reading 
and writing (depending on the design) that can be triggered 
simultaneously.

Relative to the implementation of the options 3 and 4, I wonder if the 
linked list memory space and size could be set through the DT or by the 
configfs available on the pci-epf-test driver.

> 
> Signed-off-by: Alan Mikhak <alan.mikhak@sifive.com>
> ---
>  drivers/dma/dw-edma/dw-edma-core.c | 17 ++++++++++-------
>  drivers/dma/dw-edma/dw-edma-core.h |  4 ++++
>  drivers/dma/dw-edma/dw-edma-pcie.c | 10 ++++++++++
>  3 files changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index ff392c01bad1..9417a5feabbf 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -14,7 +14,7 @@
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
>  #include <linux/dma/edma.h>
> -#include <linux/pci.h>
> +#include <linux/dma-mapping.h>
>  
>  #include "dw-edma-core.h"
>  #include "dw-edma-v0-core.h"
> @@ -781,7 +781,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip,
>  
>  	if (dw->nr_irqs == 1) {
>  		/* Common IRQ shared among all channels */
> -		err = request_irq(pci_irq_vector(to_pci_dev(dev), 0),
> +		err = request_irq(dw->ops->irq_vector(dev, 0),
>  				  dw_edma_interrupt_common,
>  				  IRQF_SHARED, dw->name, &dw->irq[0]);
>  		if (err) {
> @@ -789,7 +789,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip,
>  			return err;
>  		}
>  
> -		get_cached_msi_msg(pci_irq_vector(to_pci_dev(dev), 0),
> +		get_cached_msi_msg(dw->ops->irq_vector(dev, 0),
>  				   &dw->irq[0].msi);
>  	} else {
>  		/* Distribute IRQs equally among all channels */
> @@ -804,7 +804,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip,
>  		dw_edma_add_irq_mask(&rd_mask, *rd_alloc, dw->rd_ch_cnt);
>  
>  		for (i = 0; i < (*wr_alloc + *rd_alloc); i++) {
> -			err = request_irq(pci_irq_vector(to_pci_dev(dev), i),
> +			err = request_irq(dw->ops->irq_vector(dev, i),
>  					  i < *wr_alloc ?
>  						dw_edma_interrupt_write :
>  						dw_edma_interrupt_read,
> @@ -815,7 +815,7 @@ static int dw_edma_irq_request(struct dw_edma_chip *chip,
>  				return err;
>  			}
>  
> -			get_cached_msi_msg(pci_irq_vector(to_pci_dev(dev), i),
> +			get_cached_msi_msg(dw->ops->irq_vector(dev, i),
>  					   &dw->irq[i].msi);
>  		}
>  
> @@ -833,6 +833,9 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	u32 rd_alloc = 0;
>  	int i, err;
>  
> +	if (!dw->ops || !dw->ops->irq_vector)
> +		return -EINVAL;
> +

I would suggest adding dw pointer check as well.

>  	raw_spin_lock_init(&dw->lock);
>  
>  	/* Find out how many write channels are supported by hardware */
> @@ -884,7 +887,7 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  
>  err_irq_free:
>  	for (i = (dw->nr_irqs - 1); i >= 0; i--)
> -		free_irq(pci_irq_vector(to_pci_dev(dev), i), &dw->irq[i]);
> +		free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]);
>  
>  	dw->nr_irqs = 0;
>  
> @@ -904,7 +907,7 @@ int dw_edma_remove(struct dw_edma_chip *chip)
>  
>  	/* Free irqs */
>  	for (i = (dw->nr_irqs - 1); i >= 0; i--)
> -		free_irq(pci_irq_vector(to_pci_dev(dev), i), &dw->irq[i]);
> +		free_irq(dw->ops->irq_vector(dev, i), &dw->irq[i]);
>  
>  	/* Power management */
>  	pm_runtime_disable(dev);
> diff --git a/drivers/dma/dw-edma/dw-edma-core.h b/drivers/dma/dw-edma/dw-edma-core.h
> index 4e5f9f6e901b..31fc50d31792 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.h
> +++ b/drivers/dma/dw-edma/dw-edma-core.h
> @@ -103,6 +103,10 @@ struct dw_edma_irq {
>  	struct dw_edma			*dw;
>  };
>  
> +struct dw_edma_core_ops {
> +	int	(*irq_vector)(struct device *dev, unsigned int nr);
> +};
> +
>  struct dw_edma {
>  	char				name[20];
>  
> diff --git a/drivers/dma/dw-edma/dw-edma-pcie.c b/drivers/dma/dw-edma/dw-edma-pcie.c
> index dc85f55e1bb8..1eafc602e17e 100644
> --- a/drivers/dma/dw-edma/dw-edma-pcie.c
> +++ b/drivers/dma/dw-edma/dw-edma-pcie.c
> @@ -54,6 +54,15 @@ static const struct dw_edma_pcie_data snps_edda_data = {
>  	.irqs				= 1,
>  };
>  
> +static int dw_edma_pcie_irq_vector(struct device *dev, unsigned int nr)
> +{
> +	return pci_irq_vector(to_pci_dev(dev), nr);
> +}
> +
> +static const struct dw_edma_core_ops dw_edma_pcie_core_ops = {
> +	.irq_vector = dw_edma_pcie_irq_vector,
> +};
> +
>  static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  			      const struct pci_device_id *pid)
>  {
> @@ -151,6 +160,7 @@ static int dw_edma_pcie_probe(struct pci_dev *pdev,
>  	dw->version = pdata->version;
>  	dw->mode = pdata->mode;
>  	dw->nr_irqs = nr_irqs;
> +	dw->ops = &dw_edma_pcie_core_ops;
>  
>  	/* Debug info */
>  	pci_dbg(pdev, "Version:\t%u\n", dw->version);
> -- 
> 2.7.4

In overall, nice patch, please fix that detail and I'll give my ACK.

Regards,
Gustavo