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@xxxxxxxxxx>
>>>>
>>>>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@xxxxxxxxxx>
>>>>Signed-off-by: Thomas Pedersen <twp@xxxxxxxxxxxxxx>
>>>>---
>>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
