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

[Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>]

On Wed, Mar 09, 2022 at 12:31:57PM -0600, Zhi Li wrote:
> On Wed, Mar 9, 2022 at 6:02 AM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> 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@nxp.com>
> > > ---
> > >
> > > This patch depended on some ep enable patch for imx.
> > >
> > > 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)
> > > +{
> > > +     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;
> >
> > Unused?
> >
> > > +     int sz = PAGE_SIZE;
> > > +
> > > +     pcie_dma_offset = 0x970;
> >
> > Can you get this offset from the devicetree node of ep?
> >
> > > +
> > > +     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;
> >
> > Is this a hard limitation of the eDMA implementation or because of difficulties
> > in requesting the correct channel from client driver?
> >
> > If it's the latter, you could use my patch:
> 
> It is  because our hardware only has 1 channel.

Ah okay. It is fine if that's the case.

> 
> >
> > https://git.linaro.org/landing-teams/working/qualcomm/kernel.git/commit/?h=tracking-qcomlt-sdx55-drivers&id=c77ad9d929372b1ff495709714b24486d266a810
> 
> My problem is
> in   dw_edma_channel_setup()
> dma->directions = BIT(write ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV);
> 
> Already set direction.  why need overwrite default device_caps?
> 

The above sets both direction in the caps. That's fine if you want to verify
whether the channel is valid or not. But that won't help you want to choose the
correct channel.

In your case it will work because, read and write channel count is 1. But what
if the channel count is 8?

write channel - 0 to 7
read channel - 8 to 15

Now without identifying the channel direction, the client would be getting the
wrong one. For instance, if client requests read channel after write, the
dmaengine would pass 1 because the requested direction matches the caps and
that's wrong.

Thanks,
Mani 

> >
> > > +     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);
> >
> > Allocation could fail. Please add error checking here and below.
> >
> > Thanks,
> > Mani
> >
> > > +
> > > +     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);
> > > +
> > > +     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
> > >