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

[Andy Gross <andy.gross@linaro.org>]

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.

Regards,

Andy