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

[Thomas Pedersen <twp@codeaurora.org>]

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?

-- 
thomas