Re: [RESEND PATCH 1/5] dtbindings: qcom_adm: Fix channel specifiers

[Thomas Pedersen <twp@codeaurora.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 <andy.gross@linaro.org>
>> >>>>
>> >>>>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 <andy.gross@linaro.org>
>> >>>>Signed-off-by: Thomas Pedersen <twp@codeaurora.org>
>> >>>>---
>> >>>> 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