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@xxxxxxx>
> ---
>
> 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, ®ion->paddr, GFP_KERNEL);
dma->ll_rd_cnt = 1;
region = &dma->ll_region_rd[0];
region->sz = sz;
region->vaddr = dmam_alloc_coherent(dev, sz, ®ion->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
>
