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

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

On Wed, Mar 09, 2022 at 01:14:33PM -0600, Zhi Li wrote:
> 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.
> 

Urgh... In our kernel tree, my colleague merged two dma devices into one and I
failed to notice that :/

So yeah, it should work by default. Sorry for the noise.

Thanks,
Mani

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