From: Eugeniy Paltsev <Eugeniy.Paltsev@synopsys.com> To: Vineet Gupta <Vineet.Gupta1@synopsys.com>, "linux-snps-arc@lists.infradead.org" <linux-snps-arc@lists.infradead.org> Cc: "hch@lst.de" <hch@lst.de>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>, "linux-arch@vger.kernel.org" <linux-arch@vger.kernel.org>, "Alexey.Brodkin@synopsys.com" <Alexey.Brodkin@synopsys.com> Subject: Re: [PATCH v2 2/4] ARC: allow to use IOC and non-IOC DMA devices simultaneously Date: Wed, 22 Aug 2018 18:40:27 +0000 [thread overview] Message-ID: <1534963226.3962.215.camel@synopsys.com> (raw) In-Reply-To: <81ddd506-1f7e-db82-4c77-ff08b1c15dd3@synopsys.com> Hi Vineet, On Mon, 2018-08-20 at 15:34 -0700, Vineet Gupta wrote: > On 08/13/2018 10:08 AM, Eugeniy Paltsev wrote: > > On Mon, 2018-08-13 at 16:24 +0000, Vineet Gupta wrote: > > > On 07/30/2018 09:26 AM, Eugeniy Paltsev wrote: > > > > @@ -1263,11 +1254,7 @@ void __init arc_cache_init_master(void) > > > > if (is_isa_arcv2() && ioc_enable) > > > > arc_ioc_setup(); > > > > > > > > - if (is_isa_arcv2() && ioc_enable) { > > > > - __dma_cache_wback_inv = __dma_cache_wback_inv_ioc; > > > > - __dma_cache_inv = __dma_cache_inv_ioc; > > > > - __dma_cache_wback = __dma_cache_wback_ioc; > > > > - } else if (is_isa_arcv2() && l2_line_sz && slc_enable) { > > For the casual reader I'd add a comment why this was deleted. Do you mean comment in commit description or comment right in sources? > > > > > + if (is_isa_arcv2() && l2_line_sz && slc_enable) { > > > > __dma_cache_wback_inv = __dma_cache_wback_inv_slc; > > > > __dma_cache_inv = __dma_cache_inv_slc; > > > > __dma_cache_wback = __dma_cache_wback_slc; > > [snip] > > > > > > > > > - /* > > > > - * IOC relies on all data (even coherent DMA data) being in cache > > > > - * Thus allocate normal cached memory > > > > - * > > > > - * The gains with IOC are two pronged: > > > > - * -For streaming data, elides need for cache maintenance, saving > > > > - * cycles in flush code, and bus bandwidth as all the lines of a > > > > - * buffer need to be flushed out to memory > > > > - * -For coherent data, Read/Write to buffers terminate early in cache > > > > - * (vs. always going to memory - thus are faster) > > > > - */ > > > > - if ((is_isa_arcv2() && ioc_enable) || > > > > - (attrs & DMA_ATTR_NON_CONSISTENT)) > > > > + if (attrs & DMA_ATTR_NON_CONSISTENT) > > > > need_coh = 0; > > > > > > [snip] > > > Yep, I tested that. > > And it works fine with both @ioc_enable == 0 and @ioc_enable == 1 > > Note that we check this variable in arch_setup_dma_ops() function now. > > > > So this arch_dma_{alloc,free} are used ONLY in case of software assisted cache maintenance. > > That's why we had to do MMU mapping to enforce non-cachability regardless of @ioc_enable. > > Reading kernel/dma/* I see what you mean. We check @ioc_enable at the time of > registering the dma op for coherent vs. non coherent case, so there's common vs. > ARC versions of alloc/free for coherent vs. noncoherent. Just to be sure that we understand both each other and source code correctly: - In coherent case we use dma_direct_* ops which doesn't use ARC alloc functions (arch_dma_{alloc|free}) - In non-coherent case we use dma_noncoherent_* ops which uses ARC alloc functions (arch_dma_{alloc|free}) > But then I'm curious why > do we bother to check the following in new arch_dma_(alloc|free) at all. > > if (attrs & DMA_ATTR_NON_CONSISTENT) > > Isn't it supposed to be NON_CONSISTENT always given the way new code works ? DMA_ATTR_NON_CONSISTENT flag is not related to IOC topic. It is a flag which we can pass to dma_{alloc|free}_attrs function from driver side. According to documentation: DMA_ATTR_NON_CONSISTENT: Lets the platform to choose to return either consistent or non-consistent memory as it sees fit. We check this flag in arch_dma_alloc (which are used in non-coherent case) to skip MMU mapping if we are advertised that consistency is not required. So, actually we can get rid of this flag checking in arch_dma_alloc and simply always do MMU mapping to enforce non-cachability and return non-cacheable memory even if DMA_ATTR_NON_CONSISTENT is passed. But I don't sure we want to do that. BTW: dma_alloc_coherent is simply dma_alloc_attrs with attrs == 0. -- Eugeniy Paltsev
WARNING: multiple messages have this Message-ID (diff)
From: Eugeniy.Paltsev@synopsys.com (Eugeniy Paltsev) To: linux-snps-arc@lists.infradead.org Subject: [PATCH v2 2/4] ARC: allow to use IOC and non-IOC DMA devices simultaneously Date: Wed, 22 Aug 2018 18:40:27 +0000 [thread overview] Message-ID: <1534963226.3962.215.camel@synopsys.com> (raw) In-Reply-To: <81ddd506-1f7e-db82-4c77-ff08b1c15dd3@synopsys.com> Hi Vineet, On Mon, 2018-08-20@15:34 -0700, Vineet Gupta wrote: > On 08/13/2018 10:08 AM, Eugeniy Paltsev wrote: > > On Mon, 2018-08-13@16:24 +0000, Vineet Gupta wrote: > > > On 07/30/2018 09:26 AM, Eugeniy Paltsev wrote: > > > > @@ -1263,11 +1254,7 @@ void __init arc_cache_init_master(void) > > > > if (is_isa_arcv2() && ioc_enable) > > > > arc_ioc_setup(); > > > > > > > > - if (is_isa_arcv2() && ioc_enable) { > > > > - __dma_cache_wback_inv = __dma_cache_wback_inv_ioc; > > > > - __dma_cache_inv = __dma_cache_inv_ioc; > > > > - __dma_cache_wback = __dma_cache_wback_ioc; > > > > - } else if (is_isa_arcv2() && l2_line_sz && slc_enable) { > > For the casual reader I'd add a comment why this was deleted. Do you mean comment in commit description or comment right in sources? > > > > > + if (is_isa_arcv2() && l2_line_sz && slc_enable) { > > > > __dma_cache_wback_inv = __dma_cache_wback_inv_slc; > > > > __dma_cache_inv = __dma_cache_inv_slc; > > > > __dma_cache_wback = __dma_cache_wback_slc; > > [snip] > > > > > > > > > - /* > > > > - * IOC relies on all data (even coherent DMA data) being in cache > > > > - * Thus allocate normal cached memory > > > > - * > > > > - * The gains with IOC are two pronged: > > > > - * -For streaming data, elides need for cache maintenance, saving > > > > - * cycles in flush code, and bus bandwidth as all the lines of a > > > > - * buffer need to be flushed out to memory > > > > - * -For coherent data, Read/Write to buffers terminate early in cache > > > > - * (vs. always going to memory - thus are faster) > > > > - */ > > > > - if ((is_isa_arcv2() && ioc_enable) || > > > > - (attrs & DMA_ATTR_NON_CONSISTENT)) > > > > + if (attrs & DMA_ATTR_NON_CONSISTENT) > > > > need_coh = 0; > > > > > > [snip] > > > Yep, I tested that. > > And it works fine with both @ioc_enable == 0 and @ioc_enable == 1 > > Note that we check this variable in arch_setup_dma_ops() function now. > > > > So this arch_dma_{alloc,free} are used ONLY in case of software assisted cache maintenance. > > That's why we had to do MMU mapping to enforce non-cachability regardless of @ioc_enable. > > Reading kernel/dma/* I see what you mean. We check @ioc_enable at the time of > registering the dma op for coherent vs. non coherent case, so there's common vs. > ARC versions of alloc/free for coherent vs. noncoherent. Just to be sure that we understand both each other and source code correctly: - In coherent case we use dma_direct_* ops which doesn't use ARC alloc functions (arch_dma_{alloc|free}) - In non-coherent case we use dma_noncoherent_* ops which uses ARC alloc functions (arch_dma_{alloc|free}) > But then I'm curious why > do we bother to check the following in new arch_dma_(alloc|free) at all. > > if (attrs & DMA_ATTR_NON_CONSISTENT) > > Isn't it supposed to be NON_CONSISTENT always given the way new code works ? DMA_ATTR_NON_CONSISTENT flag is not related to IOC topic. It is a flag which we can pass to dma_{alloc|free}_attrs function from driver side. According to documentation: DMA_ATTR_NON_CONSISTENT: Lets the platform to choose to return either consistent or non-consistent memory as it sees fit. We check this flag in arch_dma_alloc (which are used in non-coherent case) to skip MMU mapping if we are advertised that consistency is not required. So, actually we can get rid of this flag checking in arch_dma_alloc and simply always do MMU mapping to enforce non-cachability and return non-cacheable memory even if DMA_ATTR_NON_CONSISTENT is passed. But I don't sure we want to do that. BTW: dma_alloc_coherent is simply dma_alloc_attrs with attrs == 0. -- Eugeniy Paltsev
next prev parent reply other threads:[~2018-08-22 18:40 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-07-30 16:26 [PATCH v2 0/4] ARC: allow to use IOC and non-IOC DMA devices simultaneously Eugeniy Paltsev 2018-07-30 16:26 ` Eugeniy Paltsev 2018-07-30 16:26 ` [PATCH v2 1/4] ARC: DTS: mark DMA devices connected through IOC port as dma-coherent Eugeniy Paltsev 2018-07-30 16:26 ` Eugeniy Paltsev 2018-07-30 16:26 ` [PATCH v2 2/4] ARC: allow to use IOC and non-IOC DMA devices simultaneously Eugeniy Paltsev 2018-07-30 16:26 ` Eugeniy Paltsev 2018-08-13 16:24 ` Vineet Gupta 2018-08-13 16:24 ` Vineet Gupta 2018-08-13 16:24 ` Vineet Gupta 2018-08-13 17:08 ` Eugeniy Paltsev 2018-08-13 17:08 ` Eugeniy Paltsev 2018-08-13 17:08 ` Eugeniy Paltsev 2018-08-20 22:34 ` Vineet Gupta 2018-08-20 22:34 ` Vineet Gupta 2018-08-20 22:34 ` Vineet Gupta 2018-08-22 18:40 ` Eugeniy Paltsev [this message] 2018-08-22 18:40 ` Eugeniy Paltsev 2018-08-22 18:40 ` Eugeniy Paltsev 2018-08-23 14:05 ` hch 2018-08-23 14:05 ` hch 2018-08-23 14:05 ` hch 2018-09-04 18:13 ` Vineet Gupta 2018-09-04 18:13 ` Vineet Gupta 2018-09-04 18:13 ` Vineet Gupta 2018-07-30 16:26 ` [PATCH v2 3/4] ARC: IOC: panic if both IOC and ZONE_HIGHMEM enabled Eugeniy Paltsev 2018-07-30 16:26 ` Eugeniy Paltsev 2018-07-30 16:26 ` [PATCH v2 4/4] ARC: don't check for HIGHMEM pages in arch_dma_alloc Eugeniy Paltsev 2018-07-30 16:26 ` Eugeniy Paltsev 2018-07-31 8:08 ` Christoph Hellwig 2018-07-31 8:08 ` Christoph Hellwig 2018-08-13 16:19 ` [PATCH v2 0/4] ARC: allow to use IOC and non-IOC DMA devices simultaneously Vineet Gupta 2018-08-13 16:19 ` Vineet Gupta 2018-08-13 16:19 ` Vineet Gupta 2018-08-13 17:27 ` Eugeniy Paltsev 2018-08-13 17:27 ` Eugeniy Paltsev 2018-08-13 17:27 ` Eugeniy Paltsev 2018-08-13 17:39 ` Vineet Gupta 2018-08-13 17:39 ` Vineet Gupta 2018-08-13 17:39 ` Vineet Gupta 2018-09-04 20:14 ` Vineet Gupta 2018-09-04 20:14 ` Vineet Gupta 2018-09-04 20:14 ` Vineet Gupta 2018-09-04 21:11 ` Christoph Hellwig 2018-09-04 21:11 ` Christoph Hellwig 2018-09-04 21:34 ` Vineet Gupta 2018-09-04 21:34 ` Vineet Gupta 2018-09-04 21:34 ` Vineet Gupta 2018-09-04 21:38 ` Christoph Hellwig 2018-09-04 21:38 ` Christoph Hellwig 2018-09-04 21:38 ` Christoph Hellwig
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=1534963226.3962.215.camel@synopsys.com \ --to=eugeniy.paltsev@synopsys.com \ --cc=Alexey.Brodkin@synopsys.com \ --cc=Vineet.Gupta1@synopsys.com \ --cc=hch@lst.de \ --cc=linux-arch@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-snps-arc@lists.infradead.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.