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
--
To unsubscribe from this list: send the line "unsubscribe dmaengine" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
