Re: [PATCH v2 4/5] PCI: imx6: add PCIe embedded DMA support

[Bjorn Helgaas <helgaas@kernel.org>]

On Thu, Mar 03, 2022 at 12:46:34PM -0600, Frank Li wrote:
> Add support for the DMA controller in the DesignWare PCIe core
> 
> The DMA can transfer data to any remote address location
> regardless PCI address space size.
> 
> Prepare struct dw_edma_chip() and call dw_edma_probe()
> 
> Signed-off-by: Frank Li <Frank.Li@nxp.com>
> ---
> 
> This patch depended on some ep enable patch for imx.

This makes it really hard to apply because we don't know *which* patch
this depends on.  Patches that depend on each other should be posted
in the same series, or at the very least, include lore URL to what
they depend on.

> Change from v1 to v2
> - rework commit message
> - align dw_edma_chip change
> 
>  drivers/pci/controller/dwc/pci-imx6.c | 51 +++++++++++++++++++++++++++
>  1 file changed, 51 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index efa8b81711090..7dc55986c947d 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -38,6 +38,7 @@
>  #include "../../pci.h"
>  
>  #include "pcie-designware.h"
> +#include "linux/dma/edma.h"
>  
>  #define IMX8MQ_PCIE_LINK_CAP_REG_OFFSET		0x7c
>  #define IMX8MQ_PCIE_LINK_CAP_L1EL_64US		GENMASK(18, 17)
> @@ -164,6 +165,8 @@ struct imx6_pcie {
>  	const struct imx6_pcie_drvdata *drvdata;
>  	struct regulator	*epdev_on;
>  	struct phy		*phy;
> +
> +	struct dw_edma_chip	dma_chip;
>  };
>  
>  /* Parameters for the waiting for PCIe PHY PLL to lock on i.MX7 */
> @@ -2031,6 +2034,51 @@ static const struct dw_pcie_ep_ops pcie_ep_ops = {
>  	.get_features = imx_pcie_ep_get_features,
>  };
>  
> +static int imx_dma_irq_vector(struct device *dev, unsigned int nr)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	return platform_get_irq_byname(pdev, "dma");
> +}
> +
> +static struct dw_edma_core_ops dma_ops = {
> +	.irq_vector = imx_dma_irq_vector,
> +};
> +
> +static int imx_add_pcie_dma(struct imx6_pcie *imx6_pcie)

Rename these functions to use "imx6" prefix, as the other functions in
the file do.  Mentioned this last time, too.  Also applies to
imx_add_pcie_ep() below; I guess that must be in the other
prerequisite patch?

> +{
> +	unsigned int pcie_dma_offset;
> +	struct dw_pcie *pci = imx6_pcie->pci;
> +	struct device *dev = pci->dev;
> +	struct dw_edma_chip *dma = &imx6_pcie->dma_chip;
> +	int i = 0;

No need to make a variable for this.  Just use 0 below, which will be
easier to read.

> +	int sz = PAGE_SIZE;
> +
> +	pcie_dma_offset = 0x970;

This would be better as a #define.  No point in making a local
variable on the stack.

> +	dma->dev = dev;
> +
> +	dma->reg_base = pci->dbi_base + pcie_dma_offset;
> +
> +	dma->ops = &dma_ops;
> +	dma->nr_irqs = 1;
> +
> +	dma->flags = DW_EDMA_CHIP_NO_MSI | DW_EDMA_CHIP_REG32BIT | DW_EDMA_CHIP_LOCAL_EP;
> +
> +	dma->ll_wr_cnt = dma->ll_rd_cnt=1;
> +	dma->ll_region_wr[0].sz = sz;
> +	dma->ll_region_wr[0].vaddr = dmam_alloc_coherent(dev, sz,
> +							 &dma->ll_region_wr[i].paddr,
> +							 GFP_KERNEL);
> +
> +	dma->ll_region_rd[0].sz = sz;
> +	dma->ll_region_rd[0].vaddr = dmam_alloc_coherent(dev, sz,
> +							 &dma->ll_region_rd[i].paddr,
> +							 GFP_KERNEL);

Wrap all the above to fit in 80 columns.

I would consider something like this to reduce some of the repetition:

  struct dw_edma_region *region;

  dma->ll_wr_cnt = 1;
  region = &dma->ll_region_wr[0];
  region->sz = sz;
  region->vaddr = dmam_alloc_coherent(dev, sz, &region->paddr, GFP_KERNEL);

  dma->ll_rd_cnt = 1;
  region = &dma->ll_region_rd[0];
  region->sz = sz;
  region->vaddr = dmam_alloc_coherent(dev, sz, &region->paddr, GFP_KERNEL);

> +
> +	return dw_edma_probe(dma);
> +}
> +
>  static int imx_add_pcie_ep(struct imx6_pcie *imx6_pcie,
>  					struct platform_device *pdev)
>  {
> @@ -2694,6 +2742,9 @@ static int imx6_pcie_probe(struct platform_device *pdev)
>  		goto err_ret;
>  	}
>  
> +	if (imx_add_pcie_dma(imx6_pcie))
> +		dev_info(dev, "pci edma probe failure\n");
> +
>  	return 0;
>  
>  err_ret:
> -- 
> 2.24.0.rc1
>