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

From: Alan Mikhak <alan.mikhak@sifive.com>

While reviewing the Synopsys Designware eDMA PCIe driver, I came across
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.

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.

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.

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.

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(-)

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

On Wed, Apr 15, 2020 at 5:13 AM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> Hi Alan,

Hi Gustavo,

Thank you for all your comments and feedback.

>
> 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

Corrected in v2 patch description.

>
> > 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
>

I have all of the above four use cases in mind as well. At the moment,
only #4 is possible with pcitest.

Use case #3 would need a new command line option for pcitest such as -L
to let its user specify linked list operationwhen used with dma in
conjunction with the existing -D option.

Use cases #1 and #2 would need another new command line option such as -R
to specify remotely initiated dma operation in conjunction with -D option.

New code in pci-epf-test and pci_endpoint_test drivers would be needed
to support use cases #1, #2, and #3. However, use case #4 should be
possible without modification to pci-epf-test or pci_endpoint_test as long
as the dmaengine channels become available on the endpoint side.

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

At the moment, pci-epf-test grabs the first available dma channel on the
endpoint side and uses it for either read, write, or copy operation. it is not
possible at the moment to specify which dma channel to use on the pcitest
command line. This may be possible by modifying the command line option
-D to also specify the name of one or more dma channels.

Also, pci-epf-test grabs the dma channel at bind time and holds on to it
until unloaded. This denies the use of the dma channel to others on the
endpoint side. However, it seems possible to grab and release the dma
channel only for the duration of each read, write, or copy test. These are
improvements that can come over time. It is great that pci-epf-test was
recently updated to include support for dma operations which makes such
improvements possible.

> 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.
>

Although these options could be set through DT or by configfs, another
option is to enable the user of pcitest to specify such parameters on
the command line when invoking each test from the host side.

> >
> > 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.

Thanks for this suggestion. I added the dw pointer check in v2 patch as
well as some others just to make sure. Since dw_edma_probe() is an
EXPORT_SYMBOL, I may call it from my platform_device driver to bring
up dma channels dynamically or based on DT information at kernel boot
time. I added checks to make sure all the required pointers are provided
by the caller.

>
> >       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.

Thanks in advance for your ACK and all the review comments.

>
> Regards,
> Gustavo

Hi Alan,

> > 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
> >
> 
> I have all of the above four use cases in mind as well. At the moment,
> only #4 is possible with pcitest.
> 
> Use case #3 would need a new command line option for pcitest such as -L
> to let its user specify linked list operationwhen used with dma in
> conjunction with the existing -D option.
> 
> Use cases #1 and #2 would need another new command line option such as -R
> to specify remotely initiated dma operation in conjunction with -D option.
> 
> New code in pci-epf-test and pci_endpoint_test drivers would be needed
> to support use cases #1, #2, and #3. However, use case #4 should be
> possible without modification to pci-epf-test or pci_endpoint_test as long
> as the dmaengine channels become available on the endpoint side.

I would suggest something like this:

-L option, local DMA triggering
-R option, remote DMA triggering
-W <n> option, to select the DMA write channel n => (0 ... 7) to be 
used
-R <n> option, to select the DMA read channel n => (0 ... 7) to be 
used
-K option, to use or not the linked list feature (K presence enables 
the LL use)
-T <n> option, to select which type of DMA transfer to be used => (n = 0 
- scatter-gather mode, 1 - cyclic mode)
-N <n> option, to define the number of cyclic transfers to perform in 
total
-C <n> option, to define the size of each chunk to be used
-t <time> option, to define a timeout for the DMA operation 

Also, the use of this options (especially when using the remote DMA 
option) should be checked through the pci_epc_get_features(), which means 
probably we need to pass the EP features capabilities to the 
pci_endpoint_test Driver, perhaps using some sets of registers on located 
on BAR0 or other.

> At the moment, pci-epf-test grabs the first available dma channel on the
> endpoint side and uses it for either read, write, or copy operation. it is not
> possible at the moment to specify which dma channel to use on the pcitest
> command line. This may be possible by modifying the command line option
> -D to also specify the name of one or more dma channels.

I'm assuming that behavior is due to your code, right? I'm not seen that 
behavior on the Kernel tree.
Check my previous suggestion, it should be something similar to what is 
been done while you select the MSI/MSI-X interrupt to trigger.

> Also, pci-epf-test grabs the dma channel at bind time and holds on to it
> until unloaded. This denies the use of the dma channel to others on the
> endpoint side. However, it seems possible to grab and release the dma
> channel only for the duration of each read, write, or copy test. These are
> improvements that can come over time. It is great that pci-epf-test was
> recently updated to include support for dma operations which makes such
> improvements possible.

Check my previous suggestion. I think by having a timeout for the DMA 
operation we can provide a way to release the dma channel.
Or we could provide some kind of heart beat, once again through some 
register in a BAR.

> > 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.
> >
> 
> Although these options could be set through DT or by configfs, another
> option is to enable the user of pcitest to specify such parameters on
> the command line when invoking each test from the host side.

That would be an easy and quick solution, but so far as I know there is a 
movement in the Kernel to avoid any configuration through module 
parameters. So I'm afraid that you have to choose by DT or configfs 
strategy. Kishon can help you on this matter, by telling you what he 
prefers.

Regards,
Gustavo

On Wed, Apr 15, 2020 at 11:17 AM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> Hi Alan,
>
> > > 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
> > >
> >
> > I have all of the above four use cases in mind as well. At the moment,
> > only #4 is possible with pcitest.
> >
> > Use case #3 would need a new command line option for pcitest such as -L
> > to let its user specify linked list operationwhen used with dma in
> > conjunction with the existing -D option.
> >
> > Use cases #1 and #2 would need another new command line option such as -R
> > to specify remotely initiated dma operation in conjunction with -D option.
> >
> > New code in pci-epf-test and pci_endpoint_test drivers would be needed
> > to support use cases #1, #2, and #3. However, use case #4 should be
> > possible without modification to pci-epf-test or pci_endpoint_test as long
> > as the dmaengine channels become available on the endpoint side.
>
> I would suggest something like this:
>
> -L option, local DMA triggering
> -R option, remote DMA triggering
> -W <n> option, to select the DMA write channel n => (0 ... 7) to be
> used
> -R <n> option, to select the DMA read channel n => (0 ... 7) to be
> used
> -K option, to use or not the linked list feature (K presence enables
> the LL use)
> -T <n> option, to select which type of DMA transfer to be used => (n = 0
> - scatter-gather mode, 1 - cyclic mode)
> -N <n> option, to define the number of cyclic transfers to perform in
> total
> -C <n> option, to define the size of each chunk to be used
> -t <time> option, to define a timeout for the DMA operation

That looks like a more complete set of command line options.

>
> Also, the use of this options (especially when using the remote DMA
> option) should be checked through the pci_epc_get_features(), which means
> probably we need to pass the EP features capabilities to the
> pci_endpoint_test Driver, perhaps using some sets of registers on located
> on BAR0 or other.

That is a great point. There may be changes required below pci-epf-test
in the endpoint framework stack.

>
> > At the moment, pci-epf-test grabs the first available dma channel on the
> > endpoint side and uses it for either read, write, or copy operation. it is not
> > possible at the moment to specify which dma channel to use on the pcitest
> > command line. This may be possible by modifying the command line option
> > -D to also specify the name of one or more dma channels.
>
> I'm assuming that behavior is due to your code, right? I'm not seen that
> behavior on the Kernel tree.
> Check my previous suggestion, it should be something similar to what is
> been done while you select the MSI/MSI-X interrupt to trigger.

I believe this behavior exists in the kernel tree because the call to
dma_request_chan_by_mask() always specifies channel zero. The user
of pcitest has no way of specifying which one of the available dma channels
to use.

>
> > Also, pci-epf-test grabs the dma channel at bind time and holds on to it
> > until unloaded. This denies the use of the dma channel to others on the
> > endpoint side. However, it seems possible to grab and release the dma
> > channel only for the duration of each read, write, or copy test. These are
> > improvements that can come over time. It is great that pci-epf-test was
> > recently updated to include support for dma operations which makes such
> > improvements possible.
>
> Check my previous suggestion. I think by having a timeout for the DMA
> operation we can provide a way to release the dma channel.
> Or we could provide some kind of heart beat, once again through some
> register in a BAR.

I believe this behavior exists in the kernel tree because the call to
dma_request_chan_by_mask() happens during the execution of
pci_epf_test_bind() and the call to dma_release_channel() happens
during the execution of pci_epf_test_unbind(). As long as pci-epf-test
is bound, I cannot use another program such as dmatest from the
endpoint-side command prompt to exercise the same channel.

What I was suggesting is perhaps pci-epf-test can be modified to
acquire and release the channel on each call to pci_epf_test_read(),
...write(), or ...copy() when the pcitest user specifies -D option.

>
> > > 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.
> > >
> >
> > Although these options could be set through DT or by configfs, another
> > option is to enable the user of pcitest to specify such parameters on
> > the command line when invoking each test from the host side.
>
> That would be an easy and quick solution, but so far as I know there is a
> movement in the Kernel to avoid any configuration through module
> parameters. So I'm afraid that you have to choose by DT or configfs
> strategy. Kishon can help you on this matter, by telling you what he
> prefers.

Thanks for that reminder. I will check before getting too invested in a
specific implementation. Just to clarify, I was suggesting giving the
user of pcitest a way to specify which one of the available dma channel to
use on each invocation of pcitest, not what dma channels are available on
the endpoint side. I assumed the strategy for which dma channels do
become present and available on endpoint would be by DT or configfs.

>
> Regards,
> Gustavo
>
>

Hi Alan,

> > > At the moment, pci-epf-test grabs the first available dma channel on the
> > > endpoint side and uses it for either read, write, or copy operation. it is not
> > > possible at the moment to specify which dma channel to use on the pcitest
> > > command line. This may be possible by modifying the command line option
> > > -D to also specify the name of one or more dma channels.
> >
> > I'm assuming that behavior is due to your code, right? I'm not seen that
> > behavior on the Kernel tree.
> > Check my previous suggestion, it should be something similar to what is
> > been done while you select the MSI/MSI-X interrupt to trigger.
> 
> I believe this behavior exists in the kernel tree because the call to
> dma_request_chan_by_mask() always specifies channel zero. The user
> of pcitest has no way of specifying which one of the available dma channels
> to use.

I think we were discussing different things. I was referring to the 
pci-epf-test code, that I wasn't being able to find any instruction to 
call the DMA driver which had the described behavior.

I think you can do it by doing this:

Pseudo code:

#define EDMA_TEST_CHANNEL_NAME			"dma%uchan%u"

static bool dw_edma_test_filter(struct dma_chan *chan, void *filter)
{
	if (strcmp(dev_name(chan->device->dev), EDMA_TEST_DEVICE_NAME) || 
strcmp(dma_chan_name(chan), filter))
		return false;

	return true;
}

static void dw_edma_test_thread_create(int id, int channel)
{
	struct dma_chan *chan;
	dma_cap_mask_t mask;
	char filter[20];

	dma_cap_zero(mask);
	dma_cap_set(DMA_SLAVE, mask);
	dma_cap_set(DMA_CYCLIC, mask);

	snprintf(filter, sizeof(filter), EDMA_TEST_CHANNEL_NAME, id, 
channel);
	chan = dma_request_channel(mask, dw_edma_test_filter, filter);

	[..]
}

> I believe this behavior exists in the kernel tree because the call to
> dma_request_chan_by_mask() happens during the execution of
> pci_epf_test_bind() and the call to dma_release_channel() happens
> during the execution of pci_epf_test_unbind(). As long as pci-epf-test
> is bound, I cannot use another program such as dmatest from the
> endpoint-side command prompt to exercise the same channel.

Ok, I understood it now. Right, you can't use the dmatest here, even 
because, as far as I know, it is only MEM TO MEM operations and we need 
DEVICE_TO_MEM and vice-versa.

> 
> What I was suggesting is perhaps pci-epf-test can be modified to
> acquire and release the channel on each call to pci_epf_test_read(),
> ...write(), or ...copy() when the pcitest user specifies -D option.

Right, you are on the right track.
Perhaps you could take a look at patch [1] that I have done some time ago 
for testing the eDMA, I think you have all the tools/guideline there to 
do this adaption.
Another thing, 

[1] https://patchwork.kernel.org/patch/10760521/

On Wed, Apr 15, 2020 at 1:58 PM Gustavo Pimentel
<Gustavo.Pimentel@synopsys.com> wrote:
>
> Hi Alan,
>
> > > > At the moment, pci-epf-test grabs the first available dma channel on the
> > > > endpoint side and uses it for either read, write, or copy operation. it is not
> > > > possible at the moment to specify which dma channel to use on the pcitest
> > > > command line. This may be possible by modifying the command line option
> > > > -D to also specify the name of one or more dma channels.
> > >
> > > I'm assuming that behavior is due to your code, right? I'm not seen that
> > > behavior on the Kernel tree.
> > > Check my previous suggestion, it should be something similar to what is
> > > been done while you select the MSI/MSI-X interrupt to trigger.
> >
> > I believe this behavior exists in the kernel tree because the call to
> > dma_request_chan_by_mask() always specifies channel zero. The user
> > of pcitest has no way of specifying which one of the available dma channels
> > to use.
>
> I think we were discussing different things. I was referring to the
> pci-epf-test code, that I wasn't being able to find any instruction to
> call the DMA driver which had the described behavior.
>
> I think you can do it by doing this:
>
> Pseudo code:
>
> #define EDMA_TEST_CHANNEL_NAME                  "dma%uchan%u"
>
> static bool dw_edma_test_filter(struct dma_chan *chan, void *filter)
> {
>         if (strcmp(dev_name(chan->device->dev), EDMA_TEST_DEVICE_NAME) ||
> strcmp(dma_chan_name(chan), filter))
>                 return false;
>
>         return true;
> }
>
> static void dw_edma_test_thread_create(int id, int channel)
> {
>         struct dma_chan *chan;
>         dma_cap_mask_t mask;
>         char filter[20];
>
>         dma_cap_zero(mask);
>         dma_cap_set(DMA_SLAVE, mask);
>         dma_cap_set(DMA_CYCLIC, mask);
>
>         snprintf(filter, sizeof(filter), EDMA_TEST_CHANNEL_NAME, id,
> channel);
>         chan = dma_request_channel(mask, dw_edma_test_filter, filter);
>
>         [..]
> }

Thanks Gustavo, This pseudo code is very useful. Now I know how to do
that part of the change.

What I have further in mind is to enable the pcitest user to specify some
arbitrary string with -D option to select one or more of the dma channels
that are available on the endpoint side. Since the user executes pcitest
from host-side command prompt and pci-epf-test executes in kernel on the
endpoint side, the messaging between userspace pcitest and kernel-space
pci_endpoint_test as well as the messaging across the bus between
pci_endpoint_test and pci-epf-test needs to be expanded to pass the user
string from the host to the endpoint. Upon receiving each read, write, or
copy message, pci-epf-test could then try to acquire the specified dma
channel and execute the user command or fail it if no such channel is
available at that moment.

>
> > I believe this behavior exists in the kernel tree because the call to
> > dma_request_chan_by_mask() happens during the execution of
> > pci_epf_test_bind() and the call to dma_release_channel() happens
> > during the execution of pci_epf_test_unbind(). As long as pci-epf-test
> > is bound, I cannot use another program such as dmatest from the
> > endpoint-side command prompt to exercise the same channel.
>
> Ok, I understood it now. Right, you can't use the dmatest here, even
> because, as far as I know, it is only MEM TO MEM operations and we need
> DEVICE_TO_MEM and vice-versa.
>
> >
> > What I was suggesting is perhaps pci-epf-test can be modified to
> > acquire and release the channel on each call to pci_epf_test_read(),
> > ...write(), or ...copy() when the pcitest user specifies -D option.
>
> Right, you are on the right track.
> Perhaps you could take a look at patch [1] that I have done some time ago
> for testing the eDMA, I think you have all the tools/guideline there to
> do this adaption.
> Another thing,
>
> [1] https://patchwork.kernel.org/patch/10760521/

Thanks for the guidance and reference code patch [1]. I will definitely
take a close look at [1].

>
>
>

On Wed, Apr 15, 2020 at 2:23 PM Alan Mikhak <alan.mikhak@sifive.com> wrote:
>
> On Wed, Apr 15, 2020 at 1:58 PM Gustavo Pimentel
> <Gustavo.Pimentel@synopsys.com> wrote:
> >
> > Hi Alan,
> >
> > > > > At the moment, pci-epf-test grabs the first available dma channel on the
> > > > > endpoint side and uses it for either read, write, or copy operation. it is not
> > > > > possible at the moment to specify which dma channel to use on the pcitest
> > > > > command line. This may be possible by modifying the command line option
> > > > > -D to also specify the name of one or more dma channels.
> > > >
> > > > I'm assuming that behavior is due to your code, right? I'm not seen that
> > > > behavior on the Kernel tree.
> > > > Check my previous suggestion, it should be something similar to what is
> > > > been done while you select the MSI/MSI-X interrupt to trigger.
> > >
> > > I believe this behavior exists in the kernel tree because the call to
> > > dma_request_chan_by_mask() always specifies channel zero. The user
> > > of pcitest has no way of specifying which one of the available dma channels
> > > to use.
> >
> > I think we were discussing different things. I was referring to the
> > pci-epf-test code, that I wasn't being able to find any instruction to
> > call the DMA driver which had the described behavior.
> >
> > I think you can do it by doing this:
> >
> > Pseudo code:
> >
> > #define EDMA_TEST_CHANNEL_NAME                  "dma%uchan%u"
> >
> > static bool dw_edma_test_filter(struct dma_chan *chan, void *filter)
> > {
> >         if (strcmp(dev_name(chan->device->dev), EDMA_TEST_DEVICE_NAME) ||
> > strcmp(dma_chan_name(chan), filter))
> >                 return false;
> >
> >         return true;
> > }
> >
> > static void dw_edma_test_thread_create(int id, int channel)
> > {
> >         struct dma_chan *chan;
> >         dma_cap_mask_t mask;
> >         char filter[20];
> >
> >         dma_cap_zero(mask);
> >         dma_cap_set(DMA_SLAVE, mask);
> >         dma_cap_set(DMA_CYCLIC, mask);
> >
> >         snprintf(filter, sizeof(filter), EDMA_TEST_CHANNEL_NAME, id,
> > channel);
> >         chan = dma_request_channel(mask, dw_edma_test_filter, filter);
> >
> >         [..]
> > }
>
> Thanks Gustavo, This pseudo code is very useful. Now I know how to do
> that part of the change.
>
> What I have further in mind is to enable the pcitest user to specify some
> arbitrary string with -D option to select one or more of the dma channels
> that are available on the endpoint side. Since the user executes pcitest
> from host-side command prompt and pci-epf-test executes in kernel on the
> endpoint side, the messaging between userspace pcitest and kernel-space
> pci_endpoint_test as well as the messaging across the bus between
> pci_endpoint_test and pci-epf-test needs to be expanded to pass the user
> string from the host to the endpoint. Upon receiving each read, write, or
> copy message, pci-epf-test could then try to acquire the specified dma
> channel and execute the user command or fail it if no such channel is
> available at that moment.
>
> >
> > > I believe this behavior exists in the kernel tree because the call to
> > > dma_request_chan_by_mask() happens during the execution of
> > > pci_epf_test_bind() and the call to dma_release_channel() happens
> > > during the execution of pci_epf_test_unbind(). As long as pci-epf-test
> > > is bound, I cannot use another program such as dmatest from the
> > > endpoint-side command prompt to exercise the same channel.
> >
> > Ok, I understood it now. Right, you can't use the dmatest here, even
> > because, as far as I know, it is only MEM TO MEM operations and we need
> > DEVICE_TO_MEM and vice-versa.
> >

On the platforms that I have this in mind for, I may not only have embedded
dma channels inside the PCIe controller but also platform dma channels. Either
type of dma channel can be used by pcitest whereas dmatest can only use
the type that can do MEM to MEM as you said. Either of these types can
be used to transfer data to or from the PCIe bus. I need to use both kinds
with pcitest to make sure the PCIe subsystem is ok.

> > >
> > > What I was suggesting is perhaps pci-epf-test can be modified to
> > > acquire and release the channel on each call to pci_epf_test_read(),
> > > ...write(), or ...copy() when the pcitest user specifies -D option.
> >
> > Right, you are on the right track.
> > Perhaps you could take a look at patch [1] that I have done some time ago
> > for testing the eDMA, I think you have all the tools/guideline there to
> > do this adaption.
> > Another thing,
> >
> > [1] https://patchwork.kernel.org/patch/10760521/
>
> Thanks for the guidance and reference code patch [1]. I will definitely
> take a close look at [1].
>
> >
> >
> >