Message-ID: <Pine.LNX.4.64.1203080902170.29847@axis700.grange>
Date:	Thu, 8 Mar 2012 11:16:20 +0100 (CET)
From:	Guennadi Liakhovetski <g.liakhovetski@....de>
To:	Vinod Koul <vinod.koul@...el.com>
cc:	Russell King - ARM Linux <linux@....linux.org.uk>,
	linux-kernel@...r.kernel.org,
	'Jassi Brar' <jassisinghbrar@...il.com>,
	Linus Walleij <linus.walleij@...aro.org>,
	Magnus Damm <magnus.damm@...il.com>,
	Paul Mundt <lethal@...ux-sh.org>
Subject: Re: [PATCH/RFC] dmaengine: add a slave parameter to __dma_request_channel()

On Thu, 8 Mar 2012, Vinod Koul wrote:

> On Wed, 2012-03-07 at 19:21 +0100, Guennadi Liakhovetski wrote:
> > > 
> > > Why are you thinking that the filter function implementation has to be
> > > provided by the peripheral driver?  That's just wrong and broken.
> > 
> > Again: because I don't like adding private APIs to a generic one.
> > 
> > > Think about it - how does the peripheral driver know what kind of dma
> > > channel its filter function has been passed - to give an example, if
> > > you built into your kernel support for the PL08x DMA engine, and lets
> > > say you had PL08x DMA engine hardware, how would your filter function
> > > decide whether it was one of your per-device channels or whether it
> > > was a PL08x DMA engine channel?
> > 
> > Sorry, there must be a confusion here: I was not proposing the above 
> > implementation for all hardware types, I don't have a good overview of all 
> > possible DMA engine scenarious and, fortunately, I don't have to implement 
> > anything that generic:-)
> > 
> > Even though I did write above "arch/arm/mach-shmobile/board-*.c" it 
> > probably wasn't clear enough: I was only talking about the shdma DMA 
> > engine driver and its clients. And so far on all sh-mobile hardware, that 
> > I'm aware of, we haven't been mixing DMA engine types on the same 
> > hardware. This is going to change soon, as soon as we get USBHS?-DMAC 
> > support in the kernel, but even then, those controllers will not be 
> > interchangeable: only USBHS devices will be served by USBHS-DMAC 
> > controllers, the rest can be served by any other controller. So, matching 
> > on a DMA controller device would perfectly suffice.
> > 
> > Of course, client drivers have no access to those device objects, that's 
> > why those lists have to be provided to them by the platform code.
> We are trying to solve this problem by making it a client or dmac
> problem rather than a platform problem. We *miss* the point here in
> discussion that platform *knows* the channel mapping and *not* dmac or
> client, so any solution not based on this would not work, so let the
> platform provide this to dmaengine.

I still have the impression, that my specific use-case (sh-mobile), where 
channels can be freely configured for use by _ANY_ client on one of 
_SEVERAL_ DMAC instances, is not fully understood or taken into account. 
For this driver any kind of fixed mapping means, that we'd have to use 
both virtual channels and controllers, adding _a lot_ of complexity to the 
DMAC driver and making the dmaengine core just an "obfuscation layer." 
Yes, I remember Russell proposing core helpers for this. They would help, 
but (1) when would they be available, (2) how well would they be suitable 
for us, (3) they'd take the coding / maintainance burden away, but 
wouldn't reduce complexity and run-time overhead.

Whereas on the other hand our case can be handled _very_ easily:

1. a client requests a channel of a specific type
2. one of channels of that type, residing on one of compatible 
   controllers, is allocated, configured and handed in to the client

That's it. No filtering, no post-allocation configuration - at least so 
far. And penalising such a simple case by forcing it to use virtual 
channels and filter through some unnatural mappings doesn't seem very 
productive to me.

Thanks
Guennadi

> We can have the map as* 
> [*with due credit to Linus Walleij, whose idea I have extended a small
> bit to have multiple channel and 1 to many mapping]
> 
> struct dmaengine_map {
> 	char *ch_name;
> 	char *client_name;
> 	char *dmac_name;
> 	unsigned int ch;
> };
> 
> struct dmaengine_map[] = {
> 	{
> 		.name = "MMC-RX",
> 		.client_name = "mmc.0",
> 		.dmac_name = "pl08x.0",
> 		.ch = 0;
> 	},
> 	/* mmc.0 device can use pl08x.0 controller ch 0 */
> 	{
> 		.name = "MMC-TX",
> 		.client_name = "mmc.0",
> 		.dmac_name = "pl08x.0",
> 		.ch = 1;
> 	},
> 	/* mmc.0 device can use pl08x.0 controller ch 1 */
> 	{
> 		.name = "SSP-TX",
> 		.client_name = "pl022.0",
> 		.dmac_name = "pl022.0",
> 		.ch = 1;
> 	},
> 	/* SSP-TX device can use pl022.0 controller ch 1 */
> 	{
> 		.name = "SSP-RX",
> 		.client_name = "pl022.0",
> 		.dmac_name = "pl022.0",
> 		.ch = 2;
> 	},
> 	/* SSP-TX device can use pl022.0 controller ch 2 */
> 	{
> 		.name = "MMC-TX",
> 		.client_name = "pl022.0",
> 		.dmac_name = "pl022.0",
> 		.ch = 2;
> 	},
> 	/* BTW I ahve ultra spl hardware where
> 	 * SSP-TX device can also use pl022.0 controller ch 2 */
> 	...
> };
> This also takes care care of many to 1 mapping where a channel can talk
> to multiple clients and dmaengine choose first in list.
> 
> If we do virtual channels (which I would advise) then we can have 1-1
> mapping, even otherwise dmaengine can pick first channel, and client has
> right to refuse (filter fn ofcourse!)
> 
> So we can add 
> int dmaengine_add_channel_map(struct dmaengine_map *map, unsigned int num_entries)
> {
> 	/* store this map into dmaengine and use for channle allocation */
> }
> This map can be given by device tree, board files, etc based on each
> what the respective arch deems the best way.
> 
> And based on yesterday discussion, I like Russell's idea of hiding
> dma_slave_config, so:
> 
> struct dma_chan *dma_request_channel_config(mask, fn, data, config)
> {
>         struct dma_chan *c = dma_request_channel(mask, fn, data);
> 
>         if (c) {
>                 if (dmaengine_slave_config(c, config)) {
>                         dma_release_channel(c);
>                         c = NULL;
>                 }
>         }
>         return c;
> }
> 
> where
> struct dma_chan *dma_request_channel(mask, fn, data)
> {
> 	for_each_match_in_map(c, map) {
> 		if (fn && ! fn(c, data))
> 			continue;
> 		return chan;
> 	}
> 	return NULL;
> }
> 
> At this point the client has the channel it needs to use .prepare_xxx
> API without the need of anything else...
> 
> Does this model fit all?
> 
> -- 
> ~Vinod
> 
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists