From: Jon Hunter <jonathanh@nvidia.com> To: Peter Ujfalusi <peter.ujfalusi@ti.com>, Sameer Pujar <spujar@nvidia.com>, Vinod Koul <vkoul@kernel.org> Cc: <dan.j.williams@intel.com>, <tiwai@suse.com>, <dmaengine@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <sharadg@nvidia.com>, <rlokhande@nvidia.com>, <dramesh@nvidia.com>, <mkumard@nvidia.com>, linux-tegra <linux-tegra@vger.kernel.org> Subject: Re: [PATCH] [RFC] dmaengine: add fifo_size member Date: Fri, 7 Jun 2019 13:58:49 +0100 [thread overview] Message-ID: <a65f2b07-4a3a-7f83-e21f-8b374844a4b9@nvidia.com> (raw) In-Reply-To: <e67a2d7c-5bd1-93ad-fe75-afcab38bc17c@ti.com> On 07/06/2019 13:17, Peter Ujfalusi wrote: > > > On 07/06/2019 13.27, Jon Hunter wrote: >>>> Hrm, it is still not clear how all of these fits together. >>>> >>>> What happens if you configure ADMA side: >>>> BURST = 10 >>>> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */ >>>> *THRES = 5 >>>> >>>> And if you change the *THRES to 10? >>>> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?) >>>> And if you change the BURST to 5? >>>> >>>> In other words what is the relation between all of these? >>> >>> So the THRES values are only applicable when the FETCHING_POLICY (bit 31 >>> of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes; >>> a threshold based transfer mode or a burst based transfer mode. The >>> burst mode transfer data as and when there is room for a burst in the FIFO. >>> >>> We use the burst mode and so we really should not be setting the THRES >>> fields as they are not applicable. Oh well something else to correct, >>> but this is side issue. >>> >>>> There must be a rule and constraints around these and if we do really >>>> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a >>>> generic way so others could benefit without 'misusing' a fifo_size >>>> parameter for similar, but not quite fifo_size information. >>> >>> Yes I see what you are saying. One option would be to define both a >>> src/dst_maxburst and src/dst_minburst size. Then we could use max for >>> the FIFO size and min for the actual burst size. >> >> Actually, we don't even need to do that. We only use src_maxburst for >> DEV_TO_MEM and dst_maxburst for MEM_TO_DEV. I don't see any reason why >> we could not use both the src_maxburst for dst_maxburst for both >> DEV_TO_MEM and MEM_TO_DEV, where one represents the FIFO size and one >> represents that DMA burst size. >> >> Sorry should have thought of that before. Any objections to using these >> this way? Obviously we would document is clearly in the driver. > > Imho if you can explain it without using 'HACK' in the sentences it > might be OK, but it does not feel right. I don't perceive this as a hack. Although from looking at the description of the src/dst_maxburst these are burst size with regard to the device, so maybe it is a stretch. > However since your ADMA and ADMIF is highly coupled and it does needs > special maxburst information (burst and allocated FIFO depth) I would > rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV: > > ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff) > ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff) > > So lower 1 byte is the burst value you want from ADMA > the other 3 bytes are the allocated FIFO size for the given ADMAIF channel. > > Sure, you need a header for this to make sure there is no > misunderstanding between the two sides. I don't like this because as I mentioned to Dmitry, the ADMA can perform memory-to-memory transfers where such encoding would not be applicable. That does not align with the description in the include/linux/dmaengine.h either. > Or pass the allocated FIFO size via maxburst and then the ADMA driver > will pick a 'good/safe' burst value for it. > > Or new member, but do you need two of them for src/dst? Probably > fifo_depth is better word for it, or allocated_fifo_depth. Right, so looking at the struct dma_slave_config we have ... u32 src_maxburst; u32 dst_maxburst; u32 src_port_window_size; u32 dst_port_window_size; Now if we could make these window sizes a union like the following this could work ... diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 8fcdee1c0cf9..851251263527 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -360,8 +360,14 @@ struct dma_slave_config { enum dma_slave_buswidth dst_addr_width; u32 src_maxburst; u32 dst_maxburst; - u32 src_port_window_size; - u32 dst_port_window_size; + union { + u32 port_window_size; + u32 port_fifo_size; + } src; + union { + u32 port_window_size; + u32 port_fifo_size; + } dst; Cheers, Jon -- nvpublic
WARNING: multiple messages have this Message-ID (diff)
From: Jon Hunter <jonathanh@nvidia.com> To: Peter Ujfalusi <peter.ujfalusi@ti.com>, Sameer Pujar <spujar@nvidia.com>, Vinod Koul <vkoul@kernel.org> Cc: dan.j.williams@intel.com, tiwai@suse.com, dmaengine@vger.kernel.org, linux-kernel@vger.kernel.org, sharadg@nvidia.com, rlokhande@nvidia.com, dramesh@nvidia.com, mkumard@nvidia.com, linux-tegra <linux-tegra@vger.kernel.org> Subject: Re: [PATCH] [RFC] dmaengine: add fifo_size member Date: Fri, 7 Jun 2019 13:58:49 +0100 [thread overview] Message-ID: <a65f2b07-4a3a-7f83-e21f-8b374844a4b9@nvidia.com> (raw) In-Reply-To: <e67a2d7c-5bd1-93ad-fe75-afcab38bc17c@ti.com> On 07/06/2019 13:17, Peter Ujfalusi wrote: > > > On 07/06/2019 13.27, Jon Hunter wrote: >>>> Hrm, it is still not clear how all of these fits together. >>>> >>>> What happens if you configure ADMA side: >>>> BURST = 10 >>>> TX/RXSIZE = 100 (100 * 64 bytes?) /* FIFO_SIZE? */ >>>> *THRES = 5 >>>> >>>> And if you change the *THRES to 10? >>>> And if you change the TX/RXSIZE to 50 (50 * 64 bytes?) >>>> And if you change the BURST to 5? >>>> >>>> In other words what is the relation between all of these? >>> >>> So the THRES values are only applicable when the FETCHING_POLICY (bit 31 >>> of the CH_FIFO_CTRL) is set. The FETCHING_POLICY bit defines two modes; >>> a threshold based transfer mode or a burst based transfer mode. The >>> burst mode transfer data as and when there is room for a burst in the FIFO. >>> >>> We use the burst mode and so we really should not be setting the THRES >>> fields as they are not applicable. Oh well something else to correct, >>> but this is side issue. >>> >>>> There must be a rule and constraints around these and if we do really >>>> need a new parameter for ADMA's FIFO_SIZE I'd like it to be defined in a >>>> generic way so others could benefit without 'misusing' a fifo_size >>>> parameter for similar, but not quite fifo_size information. >>> >>> Yes I see what you are saying. One option would be to define both a >>> src/dst_maxburst and src/dst_minburst size. Then we could use max for >>> the FIFO size and min for the actual burst size. >> >> Actually, we don't even need to do that. We only use src_maxburst for >> DEV_TO_MEM and dst_maxburst for MEM_TO_DEV. I don't see any reason why >> we could not use both the src_maxburst for dst_maxburst for both >> DEV_TO_MEM and MEM_TO_DEV, where one represents the FIFO size and one >> represents that DMA burst size. >> >> Sorry should have thought of that before. Any objections to using these >> this way? Obviously we would document is clearly in the driver. > > Imho if you can explain it without using 'HACK' in the sentences it > might be OK, but it does not feel right. I don't perceive this as a hack. Although from looking at the description of the src/dst_maxburst these are burst size with regard to the device, so maybe it is a stretch. > However since your ADMA and ADMIF is highly coupled and it does needs > special maxburst information (burst and allocated FIFO depth) I would > rather use src_maxburst/dst_maxburst alone for DEV_TO_MEM/MEM_TO_DEV: > > ADMA_BURST_SIZE(maxburst) ((maxburst) & 0xff) > ADMA_FIFO_SIZE(maxburst) (((maxburst) >> 8) & 0xffffff) > > So lower 1 byte is the burst value you want from ADMA > the other 3 bytes are the allocated FIFO size for the given ADMAIF channel. > > Sure, you need a header for this to make sure there is no > misunderstanding between the two sides. I don't like this because as I mentioned to Dmitry, the ADMA can perform memory-to-memory transfers where such encoding would not be applicable. That does not align with the description in the include/linux/dmaengine.h either. > Or pass the allocated FIFO size via maxburst and then the ADMA driver > will pick a 'good/safe' burst value for it. > > Or new member, but do you need two of them for src/dst? Probably > fifo_depth is better word for it, or allocated_fifo_depth. Right, so looking at the struct dma_slave_config we have ... u32 src_maxburst; u32 dst_maxburst; u32 src_port_window_size; u32 dst_port_window_size; Now if we could make these window sizes a union like the following this could work ... diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h index 8fcdee1c0cf9..851251263527 100644 --- a/include/linux/dmaengine.h +++ b/include/linux/dmaengine.h @@ -360,8 +360,14 @@ struct dma_slave_config { enum dma_slave_buswidth dst_addr_width; u32 src_maxburst; u32 dst_maxburst; - u32 src_port_window_size; - u32 dst_port_window_size; + union { + u32 port_window_size; + u32 port_fifo_size; + } src; + union { + u32 port_window_size; + u32 port_fifo_size; + } dst; Cheers, Jon -- nvpublic
next prev parent reply other threads:[~2019-06-07 12:58 UTC|newest] Thread overview: 78+ messages / expand[flat|nested] mbox.gz Atom feed top 2019-04-30 11:30 [RFC] dmaengine: add fifo_size member Sameer Pujar 2019-04-30 11:30 ` [PATCH] " Sameer Pujar 2019-05-02 6:04 ` Vinod Koul 2019-05-02 6:04 ` [PATCH] " Vinod Koul 2019-05-02 10:53 ` Sameer Pujar 2019-05-02 12:25 ` Vinod Koul 2019-05-02 13:29 ` Sameer Pujar 2019-05-03 19:10 ` Peter Ujfalusi 2019-05-04 10:23 ` Vinod Koul 2019-05-06 13:04 ` Sameer Pujar 2019-05-06 15:50 ` Vinod Koul 2019-06-06 3:49 ` Sameer Pujar 2019-06-06 6:00 ` Peter Ujfalusi 2019-06-06 6:41 ` Sameer Pujar 2019-06-06 7:14 ` Jon Hunter 2019-06-06 10:22 ` Peter Ujfalusi 2019-06-06 10:49 ` Jon Hunter 2019-06-06 10:49 ` Jon Hunter 2019-06-06 11:54 ` Peter Ujfalusi 2019-06-06 11:54 ` Peter Ujfalusi 2019-06-06 12:37 ` Jon Hunter 2019-06-06 12:37 ` Jon Hunter 2019-06-06 13:45 ` Dmitry Osipenko 2019-06-06 13:55 ` Dmitry Osipenko 2019-06-06 14:26 ` Jon Hunter 2019-06-06 14:26 ` Jon Hunter 2019-06-06 14:36 ` Jon Hunter 2019-06-06 14:36 ` Jon Hunter 2019-06-06 14:36 ` Dmitry Osipenko 2019-06-06 14:47 ` Jon Hunter 2019-06-06 14:47 ` Jon Hunter 2019-06-06 14:25 ` Jon Hunter 2019-06-06 14:25 ` Jon Hunter 2019-06-06 15:18 ` Dmitry Osipenko 2019-06-06 16:32 ` Jon Hunter 2019-06-06 16:32 ` Jon Hunter 2019-06-06 16:44 ` Dmitry Osipenko 2019-06-06 16:53 ` Jon Hunter 2019-06-06 16:53 ` Jon Hunter 2019-06-06 17:25 ` Dmitry Osipenko 2019-06-06 17:56 ` Dmitry Osipenko 2019-06-07 9:24 ` Jon Hunter 2019-06-07 9:24 ` Jon Hunter 2019-06-07 5:50 ` Peter Ujfalusi 2019-06-07 5:50 ` Peter Ujfalusi 2019-06-07 9:18 ` Jon Hunter 2019-06-07 9:18 ` Jon Hunter 2019-06-07 10:27 ` Jon Hunter 2019-06-07 10:27 ` Jon Hunter 2019-06-07 12:17 ` Peter Ujfalusi 2019-06-07 12:17 ` Peter Ujfalusi 2019-06-07 12:58 ` Jon Hunter [this message] 2019-06-07 12:58 ` Jon Hunter 2019-06-07 13:35 ` Peter Ujfalusi 2019-06-07 13:35 ` Peter Ujfalusi 2019-06-07 20:53 ` Dmitry Osipenko 2019-06-10 8:01 ` Jon Hunter 2019-06-10 8:01 ` Jon Hunter 2019-06-10 7:59 ` Jon Hunter 2019-06-10 7:59 ` Jon Hunter 2019-06-13 4:43 ` Vinod Koul 2019-06-17 7:07 ` Sameer Pujar 2019-06-18 4:33 ` Vinod Koul 2019-06-20 10:29 ` Sameer Pujar 2019-06-24 6:26 ` Vinod Koul 2019-06-25 2:57 ` Sameer Pujar 2019-07-05 6:15 ` Sameer Pujar 2019-07-15 15:42 ` Sameer Pujar 2019-07-19 5:04 ` Vinod Koul 2019-07-23 5:54 ` Sameer Pujar 2019-07-29 6:10 ` Vinod Koul 2019-07-31 9:48 ` Jon Hunter 2019-07-31 15:16 ` Vinod Koul 2019-08-02 8:51 ` Jon Hunter 2019-08-08 12:38 ` Vinod Koul 2019-08-19 15:56 ` Jon Hunter 2019-08-20 11:05 ` Vinod Koul 2019-09-16 9:02 ` Sameer Pujar
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=a65f2b07-4a3a-7f83-e21f-8b374844a4b9@nvidia.com \ --to=jonathanh@nvidia.com \ --cc=dan.j.williams@intel.com \ --cc=dmaengine@vger.kernel.org \ --cc=dramesh@nvidia.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-tegra@vger.kernel.org \ --cc=mkumard@nvidia.com \ --cc=peter.ujfalusi@ti.com \ --cc=rlokhande@nvidia.com \ --cc=sharadg@nvidia.com \ --cc=spujar@nvidia.com \ --cc=tiwai@suse.com \ --cc=vkoul@kernel.org \ /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.