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

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

On 12/04/16 15:10, Vinod Koul wrote:
> On Mon, Apr 11, 2016 at 03:09:05PM +0100, Jon Hunter wrote:
>> On 05/04/16 22:36, Vinod Koul wrote:
>>> On Tue, Mar 15, 2016 at 03:56:29PM +0000, Jon Hunter wrote:

[snip]

>> So, AFAICT, dma_async_device_register() does not check to see if
>> device_prep_slave_sg() is valid AND none of the functions
>> dmaengine_prep_slave_single(), dmaengine_prep_slave_single() and
>> dmaengine_prep_rio_sg() check to see if the function pointer is valid
>> before calling chan->device->device_prep_slave_sg(). So it seems that we
>> always expect this function pointer to be valid.
>>
>> So should the inline functions ensure the function pointer is valid
>> before attempting to call them? If so I can add a patch for this.
>> Otherwise it seems the driver needs a stub. It would be a massive change
>> to add a new capability, say SLAVE_SG, and populate this for all
>> existing drivers.
> 
> No we should check here, it's indeed a miss, not sure why none complained
> about this. I was assuming this is due to caps not considering cyclic case,
> so I fixed that up.
> 
> I will fix these cases too, thanks for reporting

Ok, great.

>>>> +static const struct tegra_adma_chip_data tegra210_chip_data = {
>>>> +	.nr_channels = 22,
>>>> +};
>>>
>>> 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.

>>>> +	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?

Cheers
Jon