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