[PATCH 4/4] dma: X-Gene PCIE DMA Driver

[Arnd Bergmann]

On Wednesday 07 January 2015 16:55:33 Mayuresh Chitale wrote:
> 
> On Wed, Jan 7, 2015 at 2:05 PM, Arnd Bergmann <arnd at arndb.de> wrote:
> > On Wednesday 07 January 2015 10:58:57 Mayuresh Chitale wrote:
> >> This patch implements DMA engine API for DMA controller on APM
> >> X-Gene PCIe controller. DMA engine can support up to 4 channels per port
> >> and up to 2048 outstanding requests per channel.  This is intended
> >> to be used on ports that are configured in EP mode or to transfer
> >> data from a RC port that is connected to a X-Gene EP port.
> >
> > Linux currently does not support PCIe endpoints. Do you plan to submit
> > a driver for that as well?
> >
> >> +
> >> +static void xpdma_setup_dma_dev(struct dma_device *dev)
> >> +{
> >> +     dma_cap_zero(dev->cap_mask);
> >> +     dma_cap_set(DMA_SLAVE, dev->cap_mask);
> >> +     dma_cap_set(DMA_PRIVATE, dev->cap_mask);
> >> +
> >> +     dev->device_alloc_chan_resources = xpdma_alloc_chan_resources;
> >> +     dev->device_free_chan_resources = xpdma_free_chan_resources;
> >> +     dev->device_tx_status = xpdma_tx_status;
> >> +     dev->device_issue_pending = xpdma_issue_pending;
> >> +     dev->device_prep_slave_sg = xpdma_prep_slave_sg;
> >> +     dev->device_control = xpdma_device_control;
> >> +}
> >
> > You are setting up DMA_SLAVE here but do not use the DT binding
> > for slave DMA. Can your driver handle slave DMA? If it can,
> > you should use the appropriate binding, if not then don't
> > set DMA_SLAVE here.
> 
> Are you referring to  DT DMA helpers? If so, I will add it.

Yes, you will need a '#dma-cells' property in the device node,
and call of_dma_controller_register with an appropriate xlate
function so a slave can find the right channel.

> >> +static int xpdma_setup_dma_channel(struct platform_device *pdev,
> >> +             struct xpdma_port *port)
> >> +{
> >> +     int i, ret = 0;
> >> +     struct xpdma_chan *chan;
> >> +     resource_size_t dma_base;
> >> +
> >> +     dma_base = MAKE_U64(readl(port->cfg + PORT_CFG_HI), readl(port->cfg)) +
> >> +             CHAN_REG_BASE;
> >> +     port->dma = devm_ioremap(&pdev->dev, dma_base,
> >> +                     CHAN_REG_SIZE * MAX_DMA_CHAN);
> >> +     if (port->dma == NULL) {
> >> +             dev_err(&pdev->dev, "Could not get base addressc$\n");
> >> +             return -ENOMEM;
> >> +     }
> >
> > The registers are not listed in the 'reg' property. Why is that?
> Actually these registers have to accessed via the PCIe config space
> region for that port. Since the config space base address is
> programmed by host controller driver we can know the base address only
> by reading the corresponding config space bar at runtime.

I think I'm missing something important here. What exactly is the
interaction with PCI here? Is it possible to use the dmaengine
driver at all when the PCI host controller is not in endpoint mode?

Does the dmaengine show up as a PCI device when the PCI controller is
in host mode?

> > You should probably use devm_request_mem_region or devm_ioremap_resource
> > here to ensure manage the mmio range.
> >
> >> +static int xpdma_probe(struct platform_device *pdev)
> >> +{
> >> +     int err;
> >> +     u32 mask;
> >> +     struct resource *res;
> >> +     struct xpdma_port *port;
> >> +
> >> +     port = devm_kzalloc(&pdev->dev, sizeof(*port), GFP_KERNEL);
> >> +     if (!port)
> >> +             return -ENOMEM;
> >> +
> >> +     port->dma_dev.dev = &pdev->dev;
> >> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> >> +     if (!res) {
> >> +             dev_err(&pdev->dev, "No cfg resource\n");
> >> +             return -EINVAL;
> >> +     }
> >> +
> >> +     port->cfg = devm_ioremap(&pdev->dev, res->start, resource_size(res));
> >> +     if (IS_ERR(port->cfg))
> >> +             return PTR_ERR(port->cfg);
> >> +
> >> +     res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> >> +     if (!res) {
> >> +             dev_err(&pdev->dev, "No intr resource\n");
> >> +             return -EINVAL;
> >> +     }
> >
> > from binding, these two register ranges look like they are part of
> > another device. Which device is that:
> >
> > +                       reg = < 0x0 0x1f2b0154 0x0 0xc
> > +                               0x0 0x1f2b0058 0x0 0x8>;
> 
> These are registers inside PCIe host controller that contains
> information required such as the dma base address, interrupt
> configuration. So this will be used for the channels. I am not aware
> about syscon but will see if it better represents these registers.

If the dmaengine is tightly connected to the PCI host controller
and only usable in endpoint mode, maybe it's better to use a completely
different model here, depending on how the PCI endpoint programming
model will look like in the end. We are in fact in need of a generic
abstraction for endpoints. There are some beginnings in drivers/ntb,
and maybe it should build on that instead?

	Arnd