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

[Vinod Koul <vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>]

On Mon, Apr 18, 2016 at 04:06:23PM +0100, Jon Hunter wrote:
> 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.

Yes but the idea was that xlate will help you to get the right channel. The
whole dmaengine property was supposed to help you with that

Please check the omap code which gets a specfic channel using this

Thanks
-- 
~Vinod