On Fri, Mar 4, 2022 at 3:33 PM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote:
>
> 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.
I did not realize It was missing some EP support patches in pci-imx6.c
upstream at v1.
Richard is working on this upstream work.
This patch don't impact previous patch 1-3 and 5
I will skip this patch next time and wait for richard finish imx6 EP
function upstream.
Other patches are independent and can be merged separately.
With patch 1-3 and 5, other platforms rather than imx that using
designware pci ip can be benefited.
>
> > 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
> >
