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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
