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

[Kishon Vijay Abraham I]

Hi Alan,

On 30/05/19 11:26 PM, Alan Mikhak wrote:
> 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

No, you can't remove memcpy_toio/memcpy_fromio APIs. There could be platforms
without system DMA or they could have system DMA but without MEMCOPY channels
or without DMA in their PCI controller.
> 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.

I still have to look closer into your DMA patch but linked-list mode or single
block mode shouldn't be an user select-able option but should be determined by
the size of transfer.
> 
> 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.

yes, that would be my preferred method as well. In fact I had implemented
pci_epf_tx() in [1], as a way for pci-epf-test to pass buffer address to
endpoint controller driver. I had also implemented helpers for platforms using
system DMA (i.e uses DMAengine).

Thanks
Kishon

[1] ->
http://git.ti.com/cgit/cgit.cgi/ti-linux-kernel/ti-linux-kernel.git/tree/drivers/pci/endpoint/pci-epf-core.c?h=ti-linux-4.19.y
> 
> I would appreciate feedback and comment on such choices as part of this review.
> 
> Regards,
> Alan Mikhak
>