Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>
To:     Alan Mikhak <alan.mikhak@sifive.com>,
        "dmaengine@vger.kernel.org" <dmaengine@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "dan.j.williams@intel.com" <dan.j.williams@intel.com>,
        "vkoul@kernel.org" <vkoul@kernel.org>,
        "kishon@ti.com" <kishon@ti.com>,
        "paul.walmsley@sifive.com" <paul.walmsley@sifive.com>
Subject: RE: [PATCH RFC] dmaengine: dw-edma: Decouple dw-edma-core.c from
 struct pci_dev
Thread-Topic: [PATCH RFC] dmaengine: dw-edma: Decouple dw-edma-core.c from
 struct pci_dev
Thread-Index: AQHWEsrRt1OuNWp8Y0WOATK/ErlmDqh6DhYA
Date:   Wed, 15 Apr 2020 12:13:13 +0000
Message-ID: <DM5PR12MB1276CB8FA4457D4CDCE3137EDADB0@DM5PR12MB1276.namprd12.prod.outlook.com>
References: <1586916464-27727-1-git-send-email-alan.mikhak@sifive.com>
In-Reply-To: <1586916464-27727-1-git-send-email-alan.mikhak@sifive.com>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: synopsys.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 8b62488d-c6b1-4e18-06c9-08d7e1365efe
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Apr 2020 12:13:13.1375
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: c33c9f88-1eb7-4099-9700-16013fd9e8aa
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: q7Y5WixYDe10JfWCjhVzA1exbVjR5aPqgwZO09/eU64PIm7xGX2MRp1sY7PIyuFrD9mZMqtKzylfLEWPnfocUg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR12MB2389
X-OriginatorOrg: synopsys.com
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Alan,

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

> From: Alan Mikhak <alan.mikhak@sifive.com>
>=20
> 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:
>=20
> const struct dw_edma_core_ops	*ops;
>=20
> 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=20
would allow being set differently according to the eDMA version. At the=20
time it was expected to have multiple cores that would need to execute=20
different code sequences.

However, this approach was requested to be postponed on the patch review=20
because at that time and even now there isn't an extra core that would=20
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=20
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.
>=20
> 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.
>=20
> 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=20
itself.
I would suggest that pcitest would have multiple options that could be=20
triggered, for instance:
 1 - Execute Endpoint DMA (read/write) remotely with Linked List feature=20
(from the Root Complex side)
 2 - Execute Endpoint DMA (read/write) remotely without Linked List=20
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=20
feature

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

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

>=20
> 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(-)
>=20
> 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>
> =20
>  #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 *c=
hip,
> =20
>  	if (dw->nr_irqs =3D=3D 1) {
>  		/* Common IRQ shared among all channels */
> -		err =3D request_irq(pci_irq_vector(to_pci_dev(dev), 0),
> +		err =3D 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 *c=
hip,
>  			return err;
>  		}
> =20
> -		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 *c=
hip,
>  		dw_edma_add_irq_mask(&rd_mask, *rd_alloc, dw->rd_ch_cnt);
> =20
>  		for (i =3D 0; i < (*wr_alloc + *rd_alloc); i++) {
> -			err =3D request_irq(pci_irq_vector(to_pci_dev(dev), i),
> +			err =3D 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 *c=
hip,
>  				return err;
>  			}
> =20
> -			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);
>  		}
> =20
> @@ -833,6 +833,9 @@ int dw_edma_probe(struct dw_edma_chip *chip)
>  	u32 rd_alloc =3D 0;
>  	int i, err;
> =20
> +	if (!dw->ops || !dw->ops->irq_vector)
> +		return -EINVAL;
> +

I would suggest adding dw pointer check as well.

>  	raw_spin_lock_init(&dw->lock);
> =20
>  	/* Find out how many write channels are supported by hardware */
> @@ -884,7 +887,7 @@ int dw_edma_probe(struct dw_edma_chip *chip)
> =20
>  err_irq_free:
>  	for (i =3D (dw->nr_irqs - 1); i >=3D 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]);
> =20
>  	dw->nr_irqs =3D 0;
> =20
> @@ -904,7 +907,7 @@ int dw_edma_remove(struct dw_edma_chip *chip)
> =20
>  	/* Free irqs */
>  	for (i =3D (dw->nr_irqs - 1); i >=3D 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]);
> =20
>  	/* 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;
>  };
> =20
> +struct dw_edma_core_ops {
> +	int	(*irq_vector)(struct device *dev, unsigned int nr);
> +};
> +
>  struct dw_edma {
>  	char				name[20];
> =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 =
=3D {
>  	.irqs				=3D 1,
>  };
> =20
> +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 =3D {
> +	.irq_vector =3D 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 =3D pdata->version;
>  	dw->mode =3D pdata->mode;
>  	dw->nr_irqs =3D nr_irqs;
> +	dw->ops =3D &dw_edma_pcie_core_ops;
> =20
>  	/* Debug info */
>  	pci_dbg(pdev, "Version:\t%u\n", dw->version);
> --=20
> 2.7.4

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

Regards,
Gustavo