Re: [PATCH V4 2/3] dmaengine: tegra-adma: Add support for Tegra210 ADMA

[Jon Hunter <jonathanh-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>]

Hi Vinod,

Any more comments here? Or are you ok?

Cheers
Jon

On 14/04/16 12:04, Jon Hunter wrote:
> 
> On 13/04/16 14:49, Vinod Koul wrote:
>> On Tue, Apr 12, 2016 at 05:23:32PM +0100, Jon Hunter wrote:
>>>>>> why should this be hard coded in kernel and not queried from something like
>>>>>> DT? This case seems to be hardware property
>>>>>
>>>>> Originally, I did have this in DT, however, the Tegra maintainers prefer
>>>>> this and this is consistent with the other Tegra DMA driver (see
>>>>> driver/dma/tegra20-apb-dma.c) [0].
>>>>
>>>> But this creates a problem when you have next generation of controller with
>>>> different channel count!
>>>> How do we solve then?
>>>
>>> Same way we solve this for the tegra20-apb-dma driver by having
>>> different chip data per SoC in the driver ...
>>>
>>> 1259 /* Tegra20 specific DMA controller information */
>>> 1260 static const struct tegra_dma_chip_data tegra20_dma_chip_data = {
>>> 1261         .nr_channels            = 16,
>>> 1262         .channel_reg_size       = 0x20,
>>> 1263         .max_dma_count          = 1024UL * 64,
>>> 1264         .support_channel_pause  = false,
>>> 1265         .support_separate_wcount_reg = false,
>>> 1266 };
>>> 1267
>>> 1268 /* Tegra30 specific DMA controller information */
>>> 1269 static const struct tegra_dma_chip_data tegra30_dma_chip_data = {
>>> 1270         .nr_channels            = 32,
>>> 1271         .channel_reg_size       = 0x20,
>>> 1272         .max_dma_count          = 1024UL * 64,
>>> 1273         .support_channel_pause  = false,
>>> 1274         .support_separate_wcount_reg = false,
>>> 1275 };
>>> 1276
>>> 1277 /* Tegra114 specific DMA controller information */
>>> 1278 static const struct tegra_dma_chip_data tegra114_dma_chip_data = {
>>> 1279         .nr_channels            = 32,
>>> 1280         .channel_reg_size       = 0x20,
>>> 1281         .max_dma_count          = 1024UL * 64,
>>> 1282         .support_channel_pause  = true,
>>> 1283         .support_separate_wcount_reg = false,
>>> 1284 };
>>> 1285
>>> 1286 /* Tegra148 specific DMA controller information */
>>> 1287 static const struct tegra_dma_chip_data tegra148_dma_chip_data = {
>>> 1288         .nr_channels            = 32,
>>> 1289         .channel_reg_size       = 0x40,
>>> 1290         .max_dma_count          = 1024UL * 64,
>>> 1291         .support_channel_pause  = true,
>>> 1292         .support_separate_wcount_reg = true,
>>> 1293 };
>>>
>>> You may still say this should be in the DT, but the Tegra maintainers
>>> prefer this data in the driver.
>>
>> Okay I don't see a a rationale behind this not being in DT, Stephan?
> 
> Let me know what you think of Stephen's feedback and if you are OK with
> this.
> 
>>>>>>> +	dma_cap_set(DMA_SLAVE, tdma->dma_dev.cap_mask);
>>>>>>> +	dma_cap_set(DMA_PRIVATE, tdma->dma_dev.cap_mask);
>>>>>>> +	dma_cap_set(DMA_CYCLIC, tdma->dma_dev.cap_mask);
>>>>>>
>>>>>> I think you should not set DMA_SLAVE, do you need caps to be exported. I
>>>>>> think that should be exported for cyclic too, let me know if that was the
>>>>>> issue?
>>>>>>
>>>>>
>>>>> Why should I not be setting DMA_SLAVE? Should I not be calling
>>>>> dma_get_any_slave_channel() in the xlate?
>>>>>
>>>>> I think that I do want to set DMA_CYCLIC as well to ensure that we check
>>>>> that the device->device_prep_dma_cyclic() function pointer is populated
>>>>> when registering the DMA controller.
>>>>
>>>> Only setting DMA_CYCLIC should do, if you see any issues around that please
>>>> get back, we cna fix those :)
>>>
>>> It did not work for me because dma_get_any_slave_channel() wants a DMA
>>> device with the DMA_SLAVE bit capability set. So if I remove this above,
>>> then requesting the channel fails via dma_get_any_slave_channel() fails.
>>> Is there something I don't understand here?
>>
>> You should use dma_request_channel() we cleaned up the APIs and recommend to
>> use dma_request_channel() for slave usages. See Documentation update in 
>> a8135d0d79e9: (dmaengine: core: Introduce new, universal API to request a
>> channel)
> 
> I had a look at this, but actually, I don't think this is going to work.
> 
> Looking at dma_request_channel(), it is going to get a DMA channel that
> matches the mask for any DMA controller. In the xlate I already know
> which DMA controller I am and I just want one of the channels. The flow
> here is ...
> 
> dma_request_chan()
>   --> of_dma_request_slave_channel()
>     --> xlate()
>       --> dma_get_any_slave_channel()
> 
> There are several other DMA drivers that are calling
> dma_get_any_slave_channel() from their xlate function which makes sense
> because they are requesting one of their own channels.
> 
> I can understand that you wish to consolidate the APIs for requesting a
> channel, but it seems to me that you still need to have an API that DMA
> controller drivers can call where they can pass their dma_device
> structure to ensure you get a channel for the appropriate DMA controller.
> 
> Cheers
> Jon
> 
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>