Re: [PATCH V2 1/2] Documentation: DT: Add binding documentation for NVIDIA ADMA

[Jon Hunter]

On 12/10/15 18:51, Stephen Warren wrote:
> On 10/12/2015 07:55 AM, Jon Hunter wrote:
>>
>> On 09/10/15 16:26, Stephen Warren wrote:
>>> On 10/09/2015 04:20 AM, Jon Hunter wrote:
>>>>
>>>> On 08/10/15 15:27, Stephen Warren wrote:
>>>>> On 10/08/2015 03:58 AM, Jon Hunter wrote:
>>>>
>>>> [snip]
>>>>
>>>>>> That's fine. From my perspective I don't have a strong objection
>>>>>> either
>>>>>> way, however, I can see that given that the name indicates rx or tx,
>>>>>> then the direction in the binding could be seen as redundant.
>>>>>>
>>>>>> So to confirm you are happy with the client bindings being as
>>>>>> follows?
>>>>>>
>>>>>> tegra_admaif: admaif@0x702d0000 {
>>>>>>          ...
>>>>>>         dmas = <&adma 1>, <&adma 1>, <&adma 2>, <&adma 2>,
>>>>>>                <&adma 3>, <&adma 3>, <&adma 4>, <&adma 4>,
>>>>>>                <&adma 5>, <&adma 5>, <&adma 6>, <&adma 6>,
>>>>>>                <&adma 7>, <&adma 7>, <&adma 8>, <&adma 8>,
>>>>>>                <&adma 9>, <&adma 9>, <&adma 10>, <&adma 10>;
>>>>>>         dma-names = "rx1", "tx1", "rx2", "tx2", "rx3", "tx3",
>>>>>>                     "rx4", "tx4", "rx5", "tx5", "rx6", "tx6",
>>>>>>                     "rx7", "tx7", "rx8", "tx8", "rx9", "tx9",
>>>>>>                     "rx10", "tx10";
>>>>>>         ...
>>>>>> };
>>>>>
>>>>> Yes, that looks good for the client binding.
>>>>
>>>> One more clarifying question ... should the xlate verify that no other
>>>> dma channel is using the same hardware request signal?
>>>>
>>>> I understand that typically the xlate decodes the binding to get the
>>>> channel info, but because this is invoked by dmaengine while allocating
>>>> a channel, I was wondering if we should prevent dmaengine allocating
>>>> more than one channel to be used with the same hardware request? If so,
>>>> then passing the direction to the xlate would be necessary (so I can
>>>> determine in the xlate that no one else is currently using this, which
>>>> is what I currently do).
>>>>
>>>> Alternatively, I could check that no one else is using the request
>>>> signal at a later when the transfer is being prepared.
>>>
>>> I think that handling this at prepare/usage time is probably most
>>> appropriate. That is the time when the resource conflict /actually/
>>> occurs.
>>
>> Although that makes sense, the more I look at this, the more I think it
>> should be handled during the channel allocate/free phases as it makes
>> sense to allocate the required resources then. It is probably simpler
>> and safer too.
> 
> I fail to see how it's simpler or safer. Everything is still 100% safe
> if the checks are handled when the channel is actually used. I think
> it's simpler too, since there's less to worry about in DT, and less
> state to carry around in the code.

Safer/simpler in the sense that there are less potential paths to deal
with (ie. completion paths). That's all.

>>> The only time when two clients would be given the same DMA request
>>> signal is if there are multiple different drivers that can DMA into the
>>> same FIFO in a time-multiplexed fashion. That seems pretty unlikely off
>>> the top of my head, but I don't think we want to actively ban that, in
>>> case we come up with a cunning use-case for it.
>>
>> I know this is purely an example, but if such a time-multiplexed scheme
>> was a real use-case, then it would seem more likely to have a shim layer
>> between the clients that talked to the dmaengine and hence, it would
>> still only be necessary for one client to interface to a given channel.
> 
> I don't agree at all. There's no reason why clients shouldn't simply go
> to the dmaengine code and request to use channels when they need them.
> Why would a shim layer be needed for that?

I was just thinking about the granularity of time-multiplexing and
overhead of swapping the sync back and forth versus formatting the
buffer prior to submitting to dmaengine.

>> What I don't like about the above binding is that someone can request
>> the dma channel "tx5" and then call dmaengine_prep_dma_cyclic() and say
>> you know what, I am gonna receive data instead.
> 
> It's always possible to write bugs. The DMA binding can't fix that.

Shame ;-)

>> That seems odd and I
>> think that such a scenario should be greeted with an error code of
>> -EINVAL. It seems to me that if channels are uni-directional (in the
>> sense you either use it for tx or rx), you should request the
>> appropriate channel for the direction you want and then set the
>> direction in dmaengine_prep_dma_cyclic() so that it matches and if it
>> does not then we return an error.
> 
> Channels (in HW) are uni-directional for a particular transfer, but can
> operate in any arbitrary combination of directions for different transfers.
> 
> Do note that the name "tx5" is something 100% isolated to the client of
> the DMA channel and meaningless for the DMA controller itself. This is
> simply a name that the client uses to look up data that it must pass to
> the DMA controller.

Yes.

>> So I still like the idea of the direction of the request being in the
>> binding so we know what the client intends (sorry to keep changing my
>> mind). Do you completely deplore the idea?
> 
> I still believe it's wrong yes.

Won't be my first time ;-)

I was trying to describe the sync and it seems for this DMA, to describe
each hardware sync you would need the index and direction. So putting
both in the binding does not seem entirely bonkers. However, I concede
that in this case the direction is somewhat negligible because as long
as the driver can handle any conflict between clients (w.r.t syncs), the
client just needs the index of the appropriate dma interface.

Jon
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/