Re: [PATCH] PCI: endpoint: Add DMA to Linux PCI EP Framework

[Alan Mikhak]

On Wed, May 29, 2019 at 10:48 PM Kishon Vijay Abraham I <kishon@xxxxxx> wrote:
>
> +Vinod Koul
>
> Hi,
>
> >>> On Fri, May 24, 2019 at 1:59 AM Gustavo Pimentel
> >>> <Gustavo.Pimentel@xxxxxxxxxxxx> wrote:
> >>>>
> >>>> Hi Alan,
> >>>>
> >>>> This patch implementation is very HW implementation dependent and
> >>>> requires the DMA to exposed through PCIe BARs, which aren't always the
> >>>> case. Besides, you are defining some control bits on
> >>>> include/linux/pci-epc.h that may not have any meaning to other types of
> >>>> DMA.
> >>>>
> >>>> I don't think this was what Kishon had in mind when he developed the
> >>>> pcitest, but let see what Kishon was to say about it.
> >>>>
> >>>> I've developed a DMA driver for DWC PCI using Linux Kernel DMAengine API
> >>>> and which I submitted some days ago.
> >>>> By having a DMA driver which implemented using DMAengine API, means the
> >>>> pcitest can use the DMAengine client API, which will be completely
> >>>> generic to any other DMA implementation.
>
> right, my initial thought process was to use only dmaengine APIs in
> pci-epf-test so that the system DMA or DMA within the PCIe controller can be
> used transparently. But can we register DMA within the PCIe controller to the
> DMA subsystem? AFAIK only system DMA should register with the DMA subsystem.
> (ADMA in SDHCI doesn't use dmaengine). Vinod Koul can confirm.
>
> If DMA within the PCIe controller cannot be registered in DMA subsystem, we
> should use something like what Alan has done in this patch with dma_read ops.
> The dma_read ops implementation in the EP controller can either use dmaengine
> APIs or use the DMA within the PCIe controller.
>
> I'll review the patch separately.
>
> Thanks
> Kishon

Hi Kishon,

I have some improvements in mind for a v2 patch in response to
feedback from Gustavo Pimentel that the current implementation is HW
specific. I hesitate from submitting a v2 patch because it seems best
to seek comment on possible directions this may be taking.

One alternative is to wait for or modify test functions in
pci-epf-test.c to call DMAengine client APIs, if possible. I imagine
pci-epf-test.c test functions would still allocate the necessary local
buffer on the endpoint side for the same canned tests for everyone to
use. They would prepare the buffer in the existing manner by filling
it with random bytes and calculate CRC in the case of a write test.
However, they would then initiate DMA operations by using DMAengine
client APIs in a generic way instead of calling memcpy_toio() and
memcpy_fromio(). They would post-process the buffer in the existing
manner such as the checking for CRC in the case of a read test.
Finally, they would release the resources and report results back to
the user of pcitest across the PCIe bus through the existing methods.

Another alternative I have in mind for v2 is to change the struct
pci_epc_dma that this patch added to pci-epc.h from the following:

struct pci_epc_dma {
        u32     control;
        u32     size;
        u64     sar;
        u64     dar;
};

to something similar to the following:

struct pci_epc_dma {
        size_t  size;
        void *buffer;
        int flags;
};

The 'flags' field can be a bit field or separate boolean values to
specify such things as linked-list mode vs single-block, etc.
Associated #defines would be removed from pci-epc.h to be replaced if
needed with something generic. The 'size' field specifies the size of
DMA transfer that can fit in the buffer.

That way the dma test functions in pci-epf-test.c can simply kmalloc
and prepare a local buffer on the endpoint side for the DMA transfer
and pass its pointer down the stack using the 'buffer' field to lower
layers. This would allow different PCIe controller drivers to
implement DMA or not according to their needs. Each implementer can
decide to use DMAengine client API, which would be preferable, or
directly read or write to DMA hardware registers to suit their needs.

I would appreciate feedback and comment on such choices as part of this review.

Regards,
Alan Mikhak