From: Kishon Vijay Abraham I <kishon@ti.com> To: Alan Mikhak <alan.mikhak@sifive.com> Cc: Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>, "arnd@arndb.de" <arnd@arndb.de>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "jingoohan1@gmail.com" <jingoohan1@gmail.com>, "bhelgaas@google.com" <bhelgaas@google.com>, "wen.yang99@zte.com.cn" <wen.yang99@zte.com.cn>, "kjlu@umn.edu" <kjlu@umn.edu>, "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>, "palmer@sifive.com" <palmer@sifive.com>, "paul.walmsley@sifive.com" <paul.walmsley@sifive.com>, Vinod Koul <vkoul@kernel.org> Subject: Re: [PATCH] PCI: endpoint: Add DMA to Linux PCI EP Framework Date: Fri, 31 May 2019 10:36:45 +0530 [thread overview] Message-ID: <75d578c2-a98c-d1ef-1633-6dc5dc3b0913@ti.com> (raw) In-Reply-To: <CABEDWGxLeD-K8PjkD5hPSTFGJKs2hxEaAVO+nE5eC9Nx2yw=ig@mail.gmail.com> 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@ti.com> wrote: >> >> +Vinod Koul >> >> Hi, >> >>>>> On Fri, May 24, 2019 at 1:59 AM Gustavo Pimentel >>>>> <Gustavo.Pimentel@synopsys.com> 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 >
WARNING: multiple messages have this Message-ID (diff)
From: Kishon Vijay Abraham I <kishon@ti.com> To: Alan Mikhak <alan.mikhak@sifive.com> Cc: "lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>, "arnd@arndb.de" <arnd@arndb.de>, "jingoohan1@gmail.com" <jingoohan1@gmail.com>, Gustavo Pimentel <Gustavo.Pimentel@synopsys.com>, "palmer@sifive.com" <palmer@sifive.com>, "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, Vinod Koul <vkoul@kernel.org>, "kjlu@umn.edu" <kjlu@umn.edu>, "paul.walmsley@sifive.com" <paul.walmsley@sifive.com>, "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>, "bhelgaas@google.com" <bhelgaas@google.com>, "linux-riscv@lists.infradead.org" <linux-riscv@lists.infradead.org>, "wen.yang99@zte.com.cn" <wen.yang99@zte.com.cn> Subject: Re: [PATCH] PCI: endpoint: Add DMA to Linux PCI EP Framework Date: Fri, 31 May 2019 10:36:45 +0530 [thread overview] Message-ID: <75d578c2-a98c-d1ef-1633-6dc5dc3b0913@ti.com> (raw) In-Reply-To: <CABEDWGxLeD-K8PjkD5hPSTFGJKs2hxEaAVO+nE5eC9Nx2yw=ig@mail.gmail.com> 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@ti.com> wrote: >> >> +Vinod Koul >> >> Hi, >> >>>>> On Fri, May 24, 2019 at 1:59 AM Gustavo Pimentel >>>>> <Gustavo.Pimentel@synopsys.com> 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 > _______________________________________________ linux-riscv mailing list linux-riscv@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-riscv
next prev parent reply other threads:[~2019-05-31 5:08 UTC|newest] Thread overview: 40+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-05-23 22:24 [PATCH] PCI: endpoint: Add DMA to Linux PCI EP Framework Alan Mikhak 2019-05-23 22:24 ` Alan Mikhak 2019-05-24 8:59 ` Gustavo Pimentel 2019-05-24 8:59 ` Gustavo Pimentel 2019-05-24 19:42 ` Alan Mikhak 2019-05-24 19:42 ` Alan Mikhak 2019-05-27 9:09 ` Gustavo Pimentel 2019-05-27 9:09 ` Gustavo Pimentel 2019-05-29 22:37 ` Alan Mikhak 2019-05-29 22:37 ` Alan Mikhak 2019-05-30 5:46 ` Kishon Vijay Abraham I 2019-05-30 5:46 ` Kishon Vijay Abraham I 2019-05-30 17:56 ` Alan Mikhak 2019-05-30 17:56 ` Alan Mikhak 2019-05-31 5:06 ` Kishon Vijay Abraham I [this message] 2019-05-31 5:06 ` Kishon Vijay Abraham I 2019-05-31 18:16 ` Alan Mikhak 2019-05-31 18:16 ` Alan Mikhak 2019-06-03 4:41 ` Kishon Vijay Abraham I 2019-06-03 4:41 ` Kishon Vijay Abraham I 2019-06-03 17:42 ` Alan Mikhak 2019-06-03 17:42 ` Alan Mikhak 2019-09-13 12:11 ` Kishon Vijay Abraham I 2019-09-13 12:11 ` Kishon Vijay Abraham I 2019-09-13 17:39 ` Alan Mikhak 2019-09-13 17:39 ` Alan Mikhak 2019-05-31 5:07 ` Vinod Koul 2019-05-31 5:07 ` Vinod Koul 2019-05-31 5:20 ` Kishon Vijay Abraham I 2019-05-31 5:20 ` Kishon Vijay Abraham I 2019-05-31 6:32 ` Vinod Koul 2019-05-31 6:32 ` Vinod Koul 2019-05-31 7:49 ` Arnd Bergmann 2019-05-31 7:49 ` Arnd Bergmann 2019-06-03 4:29 ` Kishon Vijay Abraham I 2019-06-03 4:29 ` Kishon Vijay Abraham I 2019-06-03 4:24 ` Kishon Vijay Abraham I 2019-06-03 4:24 ` Kishon Vijay Abraham I 2019-06-03 4:42 ` Vinod Koul 2019-06-03 4:42 ` Vinod Koul
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=75d578c2-a98c-d1ef-1633-6dc5dc3b0913@ti.com \ --to=kishon@ti.com \ --cc=Gustavo.Pimentel@synopsys.com \ --cc=alan.mikhak@sifive.com \ --cc=arnd@arndb.de \ --cc=bhelgaas@google.com \ --cc=gregkh@linuxfoundation.org \ --cc=jingoohan1@gmail.com \ --cc=kjlu@umn.edu \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-pci@vger.kernel.org \ --cc=linux-riscv@lists.infradead.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=palmer@sifive.com \ --cc=paul.walmsley@sifive.com \ --cc=vkoul@kernel.org \ --cc=wen.yang99@zte.com.cn \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.