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

[Zhi Li <lznuaa@gmail.com>]

On Wed, Mar 9, 2022 at 12:42 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> 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.

Why set both direction?
           write ? DMA_DEV_TO_MEM : DMA_MEM_TO_DEV
Only one bit set

> 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?

I know my case is special one.  I just feel strange.

dma_async_device_register(dma); register two devices, one read, one write.

The default int dma_get_slave_caps(struct dma_chan *chan, struct
dma_slave_caps *caps)
{
          device = chan->device;
          caps->directions = device->directions;
}

It should return read/write devices's directions.

>
> 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.

Do you mean if I request a read after write can reproduce the problem?


>
> 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
> > > >