From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Pedersen Subject: Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers Date: Thu, 04 Aug 2016 10:42:37 -0700 Message-ID: <990ca24c0586008f186c92f01c33ab24@codeaurora.org> References: <1467150186-11427-1-git-send-email-twp@codeaurora.org> <1467150186-11427-2-git-send-email-twp@codeaurora.org> <20160629210606.GB16832@hector.attlocal.net> <847cb2110a7980eed819100bd07e2054@codeaurora.org> <20160701190841.GA7295@hector.attlocal.net> <952bef3d58349bb7df262bd23e7a17f3@codeaurora.org> <20160801203754.GA25774@hector.attlocal.net> Reply-To: twp@codeaurora.org Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:49500 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S964796AbcHDRvF (ORCPT ); Thu, 4 Aug 2016 13:51:05 -0400 In-Reply-To: <20160801203754.GA25774@hector.attlocal.net> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Andy Gross Cc: linux-arm-msm@vger.kernel.org, linux@qca.qualcomm.com, Vinod Koul , Rob Herring , Mark Rutland , linux-arm-msm-owner@vger.kernel.org On 2016-08-01 13:37, Andy Gross wrote: > On Thu, Jul 28, 2016 at 03:16:07PM -0700, Thomas Pedersen wrote: >> On 2016-07-01 12:08, Andy Gross wrote: >> >On Fri, Jul 01, 2016 at 10:50:51AM -0700, Thomas Pedersen wrote: >> >>Hi Andy, >> >> >> >>On 2016-06-29 14:06, Andy Gross wrote: >> >>>On Tue, Jun 28, 2016 at 02:43:02PM -0700, Thomas Pedersen wrote: >> >>>>From: Andy Gross >> >>>> >> >>>>This patch removes the crci information from the dma >> >>>>channel property. At least one client device requires >> >>>>using more than one CRCI value for a channel. This does >> >>>>not match the current binding and the crci information >> >>>>needs to be removed. >> >>>> >> >>>>Instead, the client device will provide this information >> >>>>via other means. >> >>>> >> >>>>Signed-off-by: Andy Gross >> >>>>Signed-off-by: Thomas Pedersen >> >>>>--- >> >>>> Documentation/devicetree/bindings/dma/qcom_adm.txt | 16 >> >>>>++++++---------- >> >>>> 1 file changed, 6 insertions(+), 10 deletions(-) >> >>>> >> >>>>diff --git a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>index 9bcab91..38d45f8 100644 >> >>>>--- a/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>+++ b/Documentation/devicetree/bindings/dma/qcom_adm.txt >> >>>>@@ -4,8 +4,7 @@ Required properties: >> >>>> - compatible: must contain "qcom,adm" for IPQ/APQ8064 and MSM8960 >> >>>> - reg: Address range for DMA registers >> >>>> - interrupts: Should contain one interrupt shared by all channels >> >>>>-- #dma-cells: must be <2>. First cell denotes the channel number. >> >>>>Second cell >> >>>>- denotes CRCI (client rate control interface) flow control assignment. >> >>>>+- #dma-cells: must be <1>. First cell denotes the channel number. >> >>> >> >>>I've actually been thinking more about this. The crci being specified in >> >>>the >> >>>slave config is probably the wrong approach. What we really need is each >> >>>physical DMA channel to allow for virtual channels that allow for a CRCI >> >>>setting >> >>>(0 being no flow control). This would require the properties to continue >> >>>to be >> >>>2 for the dma definition. >> >> >> >>AFAICT the cmd-crci and data-crci properties in the NAND controller >> >>client >> >>are >> >>really just the slave_id for the DMA engine. Flow control is decided by >> >>some >> >>other heuristic such as whether it's a read/write/etc. command. >> >> >> >>>This would also require clients to get multiple references to the same DMA >> >>>channel if they require some communications with and without flow control. >> >>> >> >>>For NAND, this would mean having two channels for dma. One for flow >> >>>controlled >> >>>and one without. >> >> >> >>So the NAND DT would look something like: >> >> >> >> dmas = <&adm_dma 3 0>, <%&adm_dma 3 1>; >> >> dma-names = "rxtx", "rxtx_fc"; >> >> qcom,cmd-crci = <15>; >> >> qcom,data-crci = <3>; >> >> >> >>? We'd still need the cmd-crci and data-crci properties for the >> >>slave_id, >> >>unless >> >>I'm confusing something? >> > >> >No, what we would have are separate channels for cmd and data that would >> >be >> >virtual channels sharing the same hardware channel. At least that is what >> >I am >> >thinking. So >> > >> >dmas = <&adm 3 0>, <&adm 3 15>, <&adm 3 3>; >> >dma-names = "rxtx", "rxtx-cmd", "rxtx-data"; # insert better names >> >here >> > >> >All three would use channel 3, but the virtual channel would know about >> >the >> >crci. CRCI = 0 is no flow control. >> > >> >I need to look at the nand a little harder to see how they formulate the >> >dma >> >requests. >> >> Did you have a chance to look into this? If not, can you provide some >> more >> specific pointers (like would these virtual channels be in the NAND >> controller >> or ADM driver?), and I can give it a try? > > The virtual channels would be in the ADM driver. So when you specify > the dmas > in the DT, you'd specify adm, channel X, crci Y. This in turn would > return a > dma channel that encapsulates the channel + crci. This takes away the > requirement of having to do the slave_config. > > As it is right now with the current ADM driver, you grab one channel > and then > slave_config it before you add descriptors. This simplifies the number > of dma > channels, but complicates things by requiring you to constantly > slave_config if > you are going from crci, to no crci. > > Separating the channel+crci into a virtual channel means that the ADM > driver > itself will collate the requests as they come in, and apply them to the > actual > hardware channel. > > That make sense? If so, this requires modifying the DT doc to specify > the crci > in the binding, modifying the ADM driver to identify/add channels based > on the > channel + crci, and modifying the clients which use both flow control > and > non-flow control. OK thanks for clearing that up. Would be nice to avoid calling slave_config for each command. I'll give this a shot. -- thomas