[02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()
diff mbox

Message ID 1432646768-12532-3-git-send-email-peter.ujfalusi@ti.com
State New
Headers show

Commit Message

Peter Ujfalusi May 26, 2015, 1:25 p.m. UTC
dma_request_slave_channel_compat() 'eats' up the returned error codes which
prevents drivers using the compat call to be able to do deferred probing.

The new wrapper is identical in functionality but it will return with error
code in case of failure and will pass the -EPROBE_DEFER to the caller in
case dma_request_slave_channel_reason() returned with it.

Signed-off-by: Peter Ujfalusi <peter.ujfalusi@ti.com>
---
 include/linux/dmaengine.h | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Comments

Vinod Koul May 29, 2015, 9:33 a.m. UTC | #1
On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
> dma_request_slave_channel_compat() 'eats' up the returned error codes which
> prevents drivers using the compat call to be able to do deferred probing.
> 
> The new wrapper is identical in functionality but it will return with error
> code in case of failure and will pass the -EPROBE_DEFER to the caller in
> case dma_request_slave_channel_reason() returned with it.
This is okay but am worried about one more warpper, how about fixing
dma_request_slave_channel_compat()
Geert Uytterhoeven May 29, 2015, 9:42 a.m. UTC | #2
On Fri, May 29, 2015 at 11:33 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
>> dma_request_slave_channel_compat() 'eats' up the returned error codes which
>> prevents drivers using the compat call to be able to do deferred probing.
>>
>> The new wrapper is identical in functionality but it will return with error
>> code in case of failure and will pass the -EPROBE_DEFER to the caller in
>> case dma_request_slave_channel_reason() returned with it.
> This is okay but am worried about one more warpper, how about fixing
> dma_request_slave_channel_compat()

Then all callers of dma_request_slave_channel_compat() have to be
modified to handle ERR_PTR first.

The same is true for (the existing) dma_request_slave_channel_reason()
vs. dma_request_slave_channel().

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Vinod Koul May 29, 2015, 10:18 a.m. UTC | #3
On Fri, May 29, 2015 at 11:42:27AM +0200, Geert Uytterhoeven wrote:
> On Fri, May 29, 2015 at 11:33 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> > On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
> >> dma_request_slave_channel_compat() 'eats' up the returned error codes which
> >> prevents drivers using the compat call to be able to do deferred probing.
> >>
> >> The new wrapper is identical in functionality but it will return with error
> >> code in case of failure and will pass the -EPROBE_DEFER to the caller in
> >> case dma_request_slave_channel_reason() returned with it.
> > This is okay but am worried about one more warpper, how about fixing
> > dma_request_slave_channel_compat()
> 
> Then all callers of dma_request_slave_channel_compat() have to be
> modified to handle ERR_PTR first.
> 
> The same is true for (the existing) dma_request_slave_channel_reason()
> vs. dma_request_slave_channel().
Good point, looking again, I think we should rather fix
dma_request_slave_channel_reason() as it was expected to return err code and
add new users. Anyway users of this API do expect the reason...
Peter Ujfalusi May 29, 2015, 2:32 p.m. UTC | #4
On 05/29/2015 01:18 PM, Vinod Koul wrote:
> On Fri, May 29, 2015 at 11:42:27AM +0200, Geert Uytterhoeven wrote:
>> On Fri, May 29, 2015 at 11:33 AM, Vinod Koul <vinod.koul@intel.com> wrote:
>>> On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
>>>> dma_request_slave_channel_compat() 'eats' up the returned error codes which
>>>> prevents drivers using the compat call to be able to do deferred probing.
>>>>
>>>> The new wrapper is identical in functionality but it will return with error
>>>> code in case of failure and will pass the -EPROBE_DEFER to the caller in
>>>> case dma_request_slave_channel_reason() returned with it.
>>> This is okay but am worried about one more warpper, how about fixing
>>> dma_request_slave_channel_compat()
>>
>> Then all callers of dma_request_slave_channel_compat() have to be
>> modified to handle ERR_PTR first.
>>
>> The same is true for (the existing) dma_request_slave_channel_reason()
>> vs. dma_request_slave_channel().
> Good point, looking again, I think we should rather fix
> dma_request_slave_channel_reason() as it was expected to return err code and
> add new users. Anyway users of this API do expect the reason...

Hrm, they are for different use.dma_request_slave_channel()/_reason() is for
drivers only working via DT or ACPI while
dma_request_slave_channel_compat()/_reason() is for drivers expected to run in
DT/ACPI or legacy mode as well.

I added the dma_request_slave_channel_compat_reason() because OMAP/daVinci
drivers are using this to request channels - they need to support DT and
legacy mode.

But it is doable to do this for both the non _compat and _compat version:
1. change all users to check IS_ERR_OR_NULL(chan)
 return the PTR_ERR if not NULL, or do whatever the driver was doing in case
of chan == NULL.
2. change the non _compat and _compat versions to do the same as the _reason
variants, #define the _reason ones to the non _reason names
3. Rename the _reason use to non _reason function in drivers
4. Remove the #defines for the _reason functions
5. Change the IS_ERR_OR_NULL(chan) to IS_ERR(chan) in all drivers
The result:
Both dma_request_slave_channel() and dma_request_slave_channel_compat() will
return ERR_PTR in case of failure or in success they will return the pinter to
chan.

Is this what you were asking?
It is a bit broader than what this series was doing: taking care of
OMAP/daVinci drivers for deferred probing regarding to dmaengine ;)
Vinod Koul June 2, 2015, 12:55 p.m. UTC | #5
On Fri, May 29, 2015 at 05:32:50PM +0300, Peter Ujfalusi wrote:
> On 05/29/2015 01:18 PM, Vinod Koul wrote:
> > On Fri, May 29, 2015 at 11:42:27AM +0200, Geert Uytterhoeven wrote:
> >> On Fri, May 29, 2015 at 11:33 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> >>> On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
> >>>> dma_request_slave_channel_compat() 'eats' up the returned error codes which
> >>>> prevents drivers using the compat call to be able to do deferred probing.
> >>>>
> >>>> The new wrapper is identical in functionality but it will return with error
> >>>> code in case of failure and will pass the -EPROBE_DEFER to the caller in
> >>>> case dma_request_slave_channel_reason() returned with it.
> >>> This is okay but am worried about one more warpper, how about fixing
> >>> dma_request_slave_channel_compat()
> >>
> >> Then all callers of dma_request_slave_channel_compat() have to be
> >> modified to handle ERR_PTR first.
> >>
> >> The same is true for (the existing) dma_request_slave_channel_reason()
> >> vs. dma_request_slave_channel().
> > Good point, looking again, I think we should rather fix
> > dma_request_slave_channel_reason() as it was expected to return err code and
> > add new users. Anyway users of this API do expect the reason...
> 
> Hrm, they are for different use.dma_request_slave_channel()/_reason() is for
> drivers only working via DT or ACPI while
> dma_request_slave_channel_compat()/_reason() is for drivers expected to run in
> DT/ACPI or legacy mode as well.
> 
> I added the dma_request_slave_channel_compat_reason() because OMAP/daVinci
> drivers are using this to request channels - they need to support DT and
> legacy mode.
I think we should hide these things behind the API and do this behind the
hood for ACPI/DT systems.

Also it makes sense to use right API and mark rest as depricated
> 
> But it is doable to do this for both the non _compat and _compat version:
> 1. change all users to check IS_ERR_OR_NULL(chan)
>  return the PTR_ERR if not NULL, or do whatever the driver was doing in case
> of chan == NULL.
> 2. change the non _compat and _compat versions to do the same as the _reason
> variants, #define the _reason ones to the non _reason names
> 3. Rename the _reason use to non _reason function in drivers
> 4. Remove the #defines for the _reason functions
> 5. Change the IS_ERR_OR_NULL(chan) to IS_ERR(chan) in all drivers
> The result:
> Both dma_request_slave_channel() and dma_request_slave_channel_compat() will
> return ERR_PTR in case of failure or in success they will return the pinter to
> chan.
> 
> Is this what you were asking?
> It is a bit broader than what this series was doing: taking care of
> OMAP/daVinci drivers for deferred probing regarding to dmaengine ;)
Yes but it would make sense right? I know it is a larger work but then we
wouldn't want another dma_request_slave_xxx API, at some point we have stop
it exapnding, perhpas now :)

Yes I am all ears to stage this work and not do transition gardually..
Peter Ujfalusi June 4, 2015, 3:58 p.m. UTC | #6
Vinod,

On 06/02/2015 03:55 PM, Vinod Koul wrote:
> On Fri, May 29, 2015 at 05:32:50PM +0300, Peter Ujfalusi wrote:
>> On 05/29/2015 01:18 PM, Vinod Koul wrote:
>>> On Fri, May 29, 2015 at 11:42:27AM +0200, Geert Uytterhoeven wrote:
>>>> On Fri, May 29, 2015 at 11:33 AM, Vinod Koul <vinod.koul@intel.com> wrote:
>>>>> On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
>>>>>> dma_request_slave_channel_compat() 'eats' up the returned error codes which
>>>>>> prevents drivers using the compat call to be able to do deferred probing.
>>>>>>
>>>>>> The new wrapper is identical in functionality but it will return with error
>>>>>> code in case of failure and will pass the -EPROBE_DEFER to the caller in
>>>>>> case dma_request_slave_channel_reason() returned with it.
>>>>> This is okay but am worried about one more warpper, how about fixing
>>>>> dma_request_slave_channel_compat()
>>>>
>>>> Then all callers of dma_request_slave_channel_compat() have to be
>>>> modified to handle ERR_PTR first.
>>>>
>>>> The same is true for (the existing) dma_request_slave_channel_reason()
>>>> vs. dma_request_slave_channel().
>>> Good point, looking again, I think we should rather fix
>>> dma_request_slave_channel_reason() as it was expected to return err code and
>>> add new users. Anyway users of this API do expect the reason...
>>
>> Hrm, they are for different use.dma_request_slave_channel()/_reason() is for
>> drivers only working via DT or ACPI while
>> dma_request_slave_channel_compat()/_reason() is for drivers expected to run in
>> DT/ACPI or legacy mode as well.
>>
>> I added the dma_request_slave_channel_compat_reason() because OMAP/daVinci
>> drivers are using this to request channels - they need to support DT and
>> legacy mode.
> I think we should hide these things behind the API and do this behind the
> hood for ACPI/DT systems.
> 
> Also it makes sense to use right API and mark rest as depricated

So to convert the dma_request_slave_channel_compat() and not to create _reason
variant?

Or to have single API to request channel? The problem with that is that we
need different parameters for legacy and DT for example.

>>
>> But it is doable to do this for both the non _compat and _compat version:
>> 1. change all users to check IS_ERR_OR_NULL(chan)
>>  return the PTR_ERR if not NULL, or do whatever the driver was doing in case
>> of chan == NULL.
>> 2. change the non _compat and _compat versions to do the same as the _reason
>> variants, #define the _reason ones to the non _reason names
>> 3. Rename the _reason use to non _reason function in drivers
>> 4. Remove the #defines for the _reason functions
>> 5. Change the IS_ERR_OR_NULL(chan) to IS_ERR(chan) in all drivers
>> The result:
>> Both dma_request_slave_channel() and dma_request_slave_channel_compat() will
>> return ERR_PTR in case of failure or in success they will return the pinter to
>> chan.
>>
>> Is this what you were asking?
>> It is a bit broader than what this series was doing: taking care of
>> OMAP/daVinci drivers for deferred probing regarding to dmaengine ;)
> Yes but it would make sense right? I know it is a larger work but then we
> wouldn't want another dma_request_slave_xxx API, at some point we have stop
> it exapnding, perhpas now :)

Yes, it make sense to get rid if the _reason() things and have the
dma_request_slave_channel() and dma_request_slave_channel_compat() return with
error code

One thing we need to do for this is to change the error codes coming back from
the _dt() and _acpi() calls when we boot in legacy mode. Right now the only
error code which comes back is -ENODEV and -EPROBE_DEFER. We need to
differentiate between 'real' errors and from the fact that we did not booted
with DT or the ACPI is not available.
IMHO if we boot with DT and the channel request fails with other than
-EPROBE_DEFER we should not go and try to get the channel via legacy API.

> Yes I am all ears to stage this work and not do transition gardually..
>
Vinod Koul June 12, 2015, 12:58 p.m. UTC | #7
On Thu, Jun 04, 2015 at 06:58:06PM +0300, Peter Ujfalusi wrote:
> Vinod,
> 
> On 06/02/2015 03:55 PM, Vinod Koul wrote:
> > On Fri, May 29, 2015 at 05:32:50PM +0300, Peter Ujfalusi wrote:
> >> On 05/29/2015 01:18 PM, Vinod Koul wrote:
> >>> On Fri, May 29, 2015 at 11:42:27AM +0200, Geert Uytterhoeven wrote:
> >>>> On Fri, May 29, 2015 at 11:33 AM, Vinod Koul <vinod.koul@intel.com> wrote:
> >>>>> On Tue, May 26, 2015 at 04:25:57PM +0300, Peter Ujfalusi wrote:
> >>>>>> dma_request_slave_channel_compat() 'eats' up the returned error codes which
> >>>>>> prevents drivers using the compat call to be able to do deferred probing.
> >>>>>>
> >>>>>> The new wrapper is identical in functionality but it will return with error
> >>>>>> code in case of failure and will pass the -EPROBE_DEFER to the caller in
> >>>>>> case dma_request_slave_channel_reason() returned with it.
> >>>>> This is okay but am worried about one more warpper, how about fixing
> >>>>> dma_request_slave_channel_compat()
> >>>>
> >>>> Then all callers of dma_request_slave_channel_compat() have to be
> >>>> modified to handle ERR_PTR first.
> >>>>
> >>>> The same is true for (the existing) dma_request_slave_channel_reason()
> >>>> vs. dma_request_slave_channel().
> >>> Good point, looking again, I think we should rather fix
> >>> dma_request_slave_channel_reason() as it was expected to return err code and
> >>> add new users. Anyway users of this API do expect the reason...
> >>
> >> Hrm, they are for different use.dma_request_slave_channel()/_reason() is for
> >> drivers only working via DT or ACPI while
> >> dma_request_slave_channel_compat()/_reason() is for drivers expected to run in
> >> DT/ACPI or legacy mode as well.
> >>
> >> I added the dma_request_slave_channel_compat_reason() because OMAP/daVinci
> >> drivers are using this to request channels - they need to support DT and
> >> legacy mode.
> > I think we should hide these things behind the API and do this behind the
> > hood for ACPI/DT systems.
> > 
> > Also it makes sense to use right API and mark rest as depricated
> 
> So to convert the dma_request_slave_channel_compat() and not to create _reason
> variant?
> 
> Or to have single API to request channel? The problem with that is that we
> need different parameters for legacy and DT for example.
Sorry this slipped thru

Thinking about it again, I think we should coverge to two APIs and mark the
legacy depracuated and look to convert folks and phase that out
Peter Ujfalusi June 22, 2015, 11:31 a.m. UTC | #8
On 06/12/2015 03:58 PM, Vinod Koul wrote:
> Sorry this slipped thru

I was away for a week anyways ;)

> Thinking about it again, I think we should coverge to two APIs and mark the
> legacy depracuated and look to convert folks and phase that out

Currently, w/o this series we have these APIs:
/* to be used with DT/ACPI */
dma_request_slave_channel(dev, name)		/* NULL on failure */
dma_request_slave_channel_reason(dev, name)	/* error code on failure */

/* Legacy mode only - no DT/ACPI lookup */
dma_request_channel(mask, fn, fn_param) /* NULL on failure */

/* to be used with DT/ACPI or legacy boot */
dma_request_slave_channel_compat(mask, fn, fn_param, dev, name)	/* NULL on
failure */

To request _any_ channel to be used for memcpy one has to use
dma_request_channel(mask, NULL, NULL);

If I did not missed something.

As we need different types of parameters for DT/ACPI and legacy (non DT/ACPI
lookup) and the good API names are already taken, we might need to settle:

/* to be used with DT/ACPI */
dma_request_slave_channel(dev, name) /* error code on failure */
- Convert users to check IS_ERR_OR_NULL() instead against NULL
- Mark dma_request_slave_channel_reason() deprecated and convert the current users

/* to be used with DT/ACPI or legacy boot */
dma_request_slave_channel_compat(mask, fn, fn_param, dev, name) /* error code
on failure */
- Convert users to check IS_ERR_OR_NULL() instead against NULL
- Do not try legacy mode if either OF or ACPI failed because of real error

/* Legacy mode only - no DT/ACPI lookup */
dma_request_channel_legacy(mask, fn, fn_param) /* error code on failure */
- convert users of dma_request_channel()
- mark dma_request_channel() deprecated

/* to be used to get a channel for memcpy for example */
dma_request_any_channel(mask) /* error code on failure */
- Convert current dma_request_channel(mask, NULL, NULL) users
I know, any of the other function could be prepared to handle this when
parameters are missing, but it is a bit cleaner to have separate API for this.

It would be nice to find another name for the
dma_request_slave_channel_compat() so with the new name we could have chance
to rearrange the parameters: (dev, name, mask, fn, fn_param)

We would end up with the following APIs, all returning with error code on failure:
dma_request_slave_channel(dev, name);
dma_request_channel_legacy(mask, fn, fn_param);
dma_request_slave_channel_compat(mask, fn, fn_param, dev, name);
dma_request_any_channel(mask);

What do you think?
Vinod Koul June 24, 2015, 4:24 p.m. UTC | #9
On Mon, Jun 22, 2015 at 02:31:00PM +0300, Peter Ujfalusi wrote:
> On 06/12/2015 03:58 PM, Vinod Koul wrote:
> > Sorry this slipped thru
> 
> I was away for a week anyways ;)
> 
> > Thinking about it again, I think we should coverge to two APIs and mark the
> > legacy depracuated and look to convert folks and phase that out
> 
> Currently, w/o this series we have these APIs:
> /* to be used with DT/ACPI */
> dma_request_slave_channel(dev, name)		/* NULL on failure */
> dma_request_slave_channel_reason(dev, name)	/* error code on failure */
> 
> /* Legacy mode only - no DT/ACPI lookup */
> dma_request_channel(mask, fn, fn_param) /* NULL on failure */
> 
> /* to be used with DT/ACPI or legacy boot */
> dma_request_slave_channel_compat(mask, fn, fn_param, dev, name)	/* NULL on
> failure */
> 
> To request _any_ channel to be used for memcpy one has to use
> dma_request_channel(mask, NULL, NULL);
> 
> If I did not missed something.
I dont think so :)

> As we need different types of parameters for DT/ACPI and legacy (non DT/ACPI
> lookup) and the good API names are already taken, we might need to settle:
> 
> /* to be used with DT/ACPI */
> dma_request_slave_channel(dev, name) /* error code on failure */
> - Convert users to check IS_ERR_OR_NULL() instead against NULL
> - Mark dma_request_slave_channel_reason() deprecated and convert the current users
> 
> /* to be used with DT/ACPI or legacy boot */
> dma_request_slave_channel_compat(mask, fn, fn_param, dev, name) /* error code
> on failure */
> - Convert users to check IS_ERR_OR_NULL() instead against NULL
> - Do not try legacy mode if either OF or ACPI failed because of real error
Should we keep the filter fn and an API for this, I am still not too sure
about that part. Anyway users should be on DT/ACPI. if someone wants filter
then let them use dma_request_channel()

> 
> /* Legacy mode only - no DT/ACPI lookup */
> dma_request_channel_legacy(mask, fn, fn_param) /* error code on failure */
> - convert users of dma_request_channel()
> - mark dma_request_channel() deprecated
Why should we create a new API, how about marking dma_request_channel() as
legacy and generic memcpy API and let other users be migrated?
> 
> /* to be used to get a channel for memcpy for example */
> dma_request_any_channel(mask) /* error code on failure */
> - Convert current dma_request_channel(mask, NULL, NULL) users
> I know, any of the other function could be prepared to handle this when
> parameters are missing, but it is a bit cleaner to have separate API for this.
Though it has merits but adds another API. We cna have internal
_dma_request_xxx API where parameters are missing and clean but to users
single API might be a better idea
> 
> It would be nice to find another name for the
> dma_request_slave_channel_compat() so with the new name we could have chance
> to rearrange the parameters: (dev, name, mask, fn, fn_param)
> 
> We would end up with the following APIs, all returning with error code on failure:
> dma_request_slave_channel(dev, name);
> dma_request_channel_legacy(mask, fn, fn_param);
> dma_request_slave_channel_compat(mask, fn, fn_param, dev, name);
> dma_request_any_channel(mask);
This is good idea but still we end up with 4 APIs. Why not just converge to
two API, one legacy + memcpy + filer fn and one untimate API for slave?

Internally we may have 4 APIs for cleaner handling...

Thoughts... ??
Arnd Bergmann June 25, 2015, 11:15 a.m. UTC | #10
On Wednesday 24 June 2015 21:54:01 Vinod Koul wrote:
> > It would be nice to find another name for the
> > dma_request_slave_channel_compat() so with the new name we could have chance
> > to rearrange the parameters: (dev, name, mask, fn, fn_param)
> > 
> > We would end up with the following APIs, all returning with error code on failure:
> > dma_request_slave_channel(dev, name);
> > dma_request_channel_legacy(mask, fn, fn_param);
> > dma_request_slave_channel_compat(mask, fn, fn_param, dev, name);
> > dma_request_any_channel(mask);
> This is good idea but still we end up with 4 APIs. Why not just converge to
> two API, one legacy + memcpy + filer fn and one untimate API for slave?
> 
> Internally we may have 4 APIs for cleaner handling...
> 

Not sure if it's realistic, but I think it would be nice to have
a way for converting the current slave drivers that use the
mask/filter/param API to the dev/name based API. We should
be able to do this by registering a lookup table from platform
code that translates one to the other, like we do with the
clkdev lookup to find a device clock based on a local identifier.

The main downside of this is that it's a lot of work if we want
to completely remove dma_request_channel() for slave drivers,
but it could be done more gradually.

Another upside is that we could come up with a mechanism to
avoid the link-time dependency on the filter-function that
causes problems when that filter is defined in a loadable
module for the dmaengine driver.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Ujfalusi Nov. 18, 2015, 2:21 p.m. UTC | #11
Hi Vinod,

bringing this old thread back to life as I just started to work on this.

On 06/24/2015 07:24 PM, Vinod Koul wrote:

>> We would end up with the following APIs, all returning with error code on failure:
>> dma_request_slave_channel(dev, name);
>> dma_request_channel_legacy(mask, fn, fn_param);
>> dma_request_slave_channel_compat(mask, fn, fn_param, dev, name);
>> dma_request_any_channel(mask);
> This is good idea but still we end up with 4 APIs. Why not just converge to
> two API, one legacy + memcpy + filer fn and one untimate API for slave?

Looked at the current API and it's use and, well, it is a bit confusing.

What I hoped that we can separate users to two category:
1. Slave channel requests, where we request a specific channel to handle HW
requests/triggers.
For this we could have:
dma_request_slave_channel(dev, name, fn, fn_param);

In DT/ACPI only drivers we can NULL out the fn and fn_param, in pure legacy
mode we null out the name, I would keep the dev so we could print dev specific
error in dmaengine core, but it could be optional, IN case of drivers used
both DT/ACPI and legacy mode all parameter can be filled and the core will
decide what to do.
For the legacy needs the dmaengine code would provide the mask dows with
DMA_SLAVE flag set.

2. non slave channel requests, where only the functionality matters, like
memcpy, interleaved, memset, etc.
We could have a simple:
dma_request_channel(mask);

But looking at the drivers using dmaengine legacy dma_request_channel() API:
Some sets DMA_INTERRUPT or DMA_PRIVATE or DMA_SG along with DMA_SLAVE:
drivers/misc/carma/carma-fpga.c			DMA_INTERRUPT|DMA_SLAVE|DMA_SG
drivers/misc/carma/carma-fpga-program.c		DMA_MEMCPY|DMA_SLAVE|DMA_SG
drivers/media/platform/soc_camera/mx3_camera.c	DMA_SLAVE|DMA_PRIVATE
sound/soc/intel/common/sst-firmware.c		DMA_SLAVE|DMA_MEMCPY

as examples.
Not sure how valid are these...

Some drivers do pass fn and fn_param when requesting channel for DMA_MEMCPY
drivers/misc/mic/host/mic_device.h
drivers/mtd/nand/fsmc_nand.c
sound/soc/intel/common/sst-firmware.c (well, this request DMA_SLAVE capability
at the same time).

Some driver sets the fn_param w/o fn, which means fn_param is ignored.

So the
dma_request_slave_channel(dev, name, fn, fn_param);
dma_request_channel(mask);

almost covers the current users and would be pretty clean ;)

If we add the mask to the slave channel API - which will become universal,
drop in replacement for dma_request_channel, and we might have only one API:

dma_request_channel(dev, name, mask, fn, fn_param);

> 
> Internally we may have 4 APIs for cleaner handling...
> 
> Thoughts... ??

Yes, as we need to arrange the code internally to keep things neat.
Arnd Bergmann Nov. 18, 2015, 2:29 p.m. UTC | #12
On Wednesday 18 November 2015 16:21:26 Peter Ujfalusi wrote:
> 2. non slave channel requests, where only the functionality matters, like
> memcpy, interleaved, memset, etc.
> We could have a simple:
> dma_request_channel(mask);
> 
> But looking at the drivers using dmaengine legacy dma_request_channel() API:
> Some sets DMA_INTERRUPT or DMA_PRIVATE or DMA_SG along with DMA_SLAVE:
> drivers/misc/carma/carma-fpga.c                 DMA_INTERRUPT|DMA_SLAVE|DMA_SG
> drivers/misc/carma/carma-fpga-program.c         DMA_MEMCPY|DMA_SLAVE|DMA_SG
> drivers/media/platform/soc_camera/mx3_camera.c  DMA_SLAVE|DMA_PRIVATE
> sound/soc/intel/common/sst-firmware.c           DMA_SLAVE|DMA_MEMCPY
> 
> as examples.
> Not sure how valid are these...

It's usually not much harder to separate out the legacy case from
the normal dma_request_slave_channel_reason(), so those drivers don't
really need to use the unified compat API.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Ujfalusi Nov. 18, 2015, 2:41 p.m. UTC | #13
On 11/18/2015 04:29 PM, Arnd Bergmann wrote:
> On Wednesday 18 November 2015 16:21:26 Peter Ujfalusi wrote:
>> 2. non slave channel requests, where only the functionality matters, like
>> memcpy, interleaved, memset, etc.
>> We could have a simple:
>> dma_request_channel(mask);
>>
>> But looking at the drivers using dmaengine legacy dma_request_channel() API:
>> Some sets DMA_INTERRUPT or DMA_PRIVATE or DMA_SG along with DMA_SLAVE:
>> drivers/misc/carma/carma-fpga.c                 DMA_INTERRUPT|DMA_SLAVE|DMA_SG
>> drivers/misc/carma/carma-fpga-program.c         DMA_MEMCPY|DMA_SLAVE|DMA_SG
>> drivers/media/platform/soc_camera/mx3_camera.c  DMA_SLAVE|DMA_PRIVATE
>> sound/soc/intel/common/sst-firmware.c           DMA_SLAVE|DMA_MEMCPY
>>
>> as examples.
>> Not sure how valid are these...
> 
> It's usually not much harder to separate out the legacy case from
> the normal dma_request_slave_channel_reason(), so those drivers don't
> really need to use the unified compat API.

The current dma_request_slave_channel()/_reason() is not the 'legacy' API.
Currently there is no way to get the reason why the dma channel request fails
when using the _compat() version of the API, which is used by drivers which
can be used in DT or in legacy mode as well. Sure, they all could have local
if(){}else{} for handling this, but it is not a nice thing.

As it was discussed instead of adding the _reason() version for the _compat
call, we should simplify the dmaengine API for getting the channel and at the
same time we will have ERR_PTR returned instead of NULL.
Arnd Bergmann Nov. 18, 2015, 3:07 p.m. UTC | #14
On Wednesday 18 November 2015 16:41:35 Peter Ujfalusi wrote:
> On 11/18/2015 04:29 PM, Arnd Bergmann wrote:
> > On Wednesday 18 November 2015 16:21:26 Peter Ujfalusi wrote:
> >> 2. non slave channel requests, where only the functionality matters, like
> >> memcpy, interleaved, memset, etc.
> >> We could have a simple:
> >> dma_request_channel(mask);
> >>
> >> But looking at the drivers using dmaengine legacy dma_request_channel() API:
> >> Some sets DMA_INTERRUPT or DMA_PRIVATE or DMA_SG along with DMA_SLAVE:
> >> drivers/misc/carma/carma-fpga.c                 DMA_INTERRUPT|DMA_SLAVE|DMA_SG
> >> drivers/misc/carma/carma-fpga-program.c         DMA_MEMCPY|DMA_SLAVE|DMA_SG
> >> drivers/media/platform/soc_camera/mx3_camera.c  DMA_SLAVE|DMA_PRIVATE
> >> sound/soc/intel/common/sst-firmware.c           DMA_SLAVE|DMA_MEMCPY
> >>
> >> as examples.
> >> Not sure how valid are these...

I just had a look myself. carma has been removed fortunately in linux-next,
so we don't have to worry about that any more.

I assume that the sst-firmware.c case is a mistake, it should just use a
plain DMA_SLAVE and not DMA_MEMCPY.

Aside from these, everyone else uses either DMA_CYCLIC in addition to
DMA_SLAVE, which seems valid, or they use DMA_PRIVATE, which I think is
redundant in slave drivers and can be removed.

> > It's usually not much harder to separate out the legacy case from
> > the normal dma_request_slave_channel_reason(), so those drivers don't
> > really need to use the unified compat API.
> 
> The current dma_request_slave_channel()/_reason() is not the 'legacy' API.
> Currently there is no way to get the reason why the dma channel request fails
> when using the _compat() version of the API, which is used by drivers which
> can be used in DT or in legacy mode as well. Sure, they all could have local
> if(){}else{} for handling this, but it is not a nice thing.
> 
> As it was discussed instead of adding the _reason() version for the _compat
> call, we should simplify the dmaengine API for getting the channel and at the
> same time we will have ERR_PTR returned instead of NULL.

What I meant was that we don't need to handle them with the unified
simple interface. The users of DMA_CYCLIC can just keep using
an internal helper that only deals with the legacy case, or use
dma_request_slave() or whatever is the new API for the DT case.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Nov. 18, 2015, 3:43 p.m. UTC | #15
On Wed, Nov 18, 2015 at 5:07 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 18 November 2015 16:41:35 Peter Ujfalusi wrote:
>> On 11/18/2015 04:29 PM, Arnd Bergmann wrote:
>> > On Wednesday 18 November 2015 16:21:26 Peter Ujfalusi wrote:
>> >> 2. non slave channel requests, where only the functionality matters, like
>> >> memcpy, interleaved, memset, etc.
>> >> We could have a simple:
>> >> dma_request_channel(mask);
>> >>
>> >> But looking at the drivers using dmaengine legacy dma_request_channel() API:
>> >> Some sets DMA_INTERRUPT or DMA_PRIVATE or DMA_SG along with DMA_SLAVE:
>> >> drivers/misc/carma/carma-fpga.c                 DMA_INTERRUPT|DMA_SLAVE|DMA_SG
>> >> drivers/misc/carma/carma-fpga-program.c         DMA_MEMCPY|DMA_SLAVE|DMA_SG
>> >> drivers/media/platform/soc_camera/mx3_camera.c  DMA_SLAVE|DMA_PRIVATE
>> >> sound/soc/intel/common/sst-firmware.c           DMA_SLAVE|DMA_MEMCPY
>> >>
>> >> as examples.
>> >> Not sure how valid are these...
>
> I just had a look myself. carma has been removed fortunately in linux-next,
> so we don't have to worry about that any more.
>
> I assume that the sst-firmware.c case is a mistake, it should just use a
> plain DMA_SLAVE and not DMA_MEMCPY.

Other way around.
Andy Shevchenko Nov. 18, 2015, 3:46 p.m. UTC | #16
On Wed, Nov 18, 2015 at 4:21 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> Hi Vinod,
>
> bringing this old thread back to life as I just started to work on this.

What I remember we need to convert drivers to use new API meanwhile it
is good to keep old one to avoid patch storm which does nothing useful
(IIRC Russel's opinion).

On the other hand there are a lot of drivers that are used on the set
of platforms starting from legacy and abandoned ones (like AVR32) to
relatively new and newest.

And I'm not a fan of those thousands of API calls either.
Arnd Bergmann Nov. 18, 2015, 3:51 p.m. UTC | #17
On Wednesday 18 November 2015 17:43:04 Andy Shevchenko wrote:
> >
> > I assume that the sst-firmware.c case is a mistake, it should just use a
> > plain DMA_SLAVE and not DMA_MEMCPY.
> 
> Other way around.
> 

Ok, I see. In that case I guess it also shouldn't call
dmaengine_slave_config(), right? I don't think that's valid
on a MEMCPY channel.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Nov. 18, 2015, 4 p.m. UTC | #18
On Wed, Nov 18, 2015 at 5:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Wednesday 18 November 2015 17:43:04 Andy Shevchenko wrote:
>> >
>> > I assume that the sst-firmware.c case is a mistake, it should just use a
>> > plain DMA_SLAVE and not DMA_MEMCPY.
>>
>> Other way around.
>>
>
> Ok, I see. In that case I guess it also shouldn't call
> dmaengine_slave_config(), right? I don't think that's valid
> on a MEMCPY channel.

Hmm… That's right, though I suspect still one thing why it's done this
way. Let's ask Vinod and Liam about that.
Vinod Koul Nov. 18, 2015, 4:06 p.m. UTC | #19
On Wed, Nov 18, 2015 at 04:51:54PM +0100, Arnd Bergmann wrote:
> On Wednesday 18 November 2015 17:43:04 Andy Shevchenko wrote:
> > >
> > > I assume that the sst-firmware.c case is a mistake, it should just use a
> > > plain DMA_SLAVE and not DMA_MEMCPY.
> > 
> > Other way around.
> > 
> 
> Ok, I see. In that case I guess it also shouldn't call
> dmaengine_slave_config(), right? I don't think that's valid
> on a MEMCPY channel.

Yes it is not valid. In this case the dma driver should invoke a generic memcpy
and not need slave parameters, knowing the hw and reason for this (firmware
download to DSP memory), this doesn't qualify for slave case.
In fact filter function doesn't need a channel, any channel in this
controller will be good
Peter Ujfalusi Nov. 19, 2015, 10:34 a.m. UTC | #20
On 11/18/2015 05:07 PM, Arnd Bergmann wrote:
> On Wednesday 18 November 2015 16:41:35 Peter Ujfalusi wrote:
>> On 11/18/2015 04:29 PM, Arnd Bergmann wrote:
>>> On Wednesday 18 November 2015 16:21:26 Peter Ujfalusi wrote:
>>>> 2. non slave channel requests, where only the functionality matters, like
>>>> memcpy, interleaved, memset, etc.
>>>> We could have a simple:
>>>> dma_request_channel(mask);
>>>>
>>>> But looking at the drivers using dmaengine legacy dma_request_channel() API:
>>>> Some sets DMA_INTERRUPT or DMA_PRIVATE or DMA_SG along with DMA_SLAVE:
>>>> drivers/misc/carma/carma-fpga.c                 DMA_INTERRUPT|DMA_SLAVE|DMA_SG
>>>> drivers/misc/carma/carma-fpga-program.c         DMA_MEMCPY|DMA_SLAVE|DMA_SG
>>>> drivers/media/platform/soc_camera/mx3_camera.c  DMA_SLAVE|DMA_PRIVATE
>>>> sound/soc/intel/common/sst-firmware.c           DMA_SLAVE|DMA_MEMCPY
>>>>
>>>> as examples.
>>>> Not sure how valid are these...
> 
> I just had a look myself. carma has been removed fortunately in linux-next,
> so we don't have to worry about that any more.
> 
> I assume that the sst-firmware.c case is a mistake, it should just use a
> plain DMA_SLAVE and not DMA_MEMCPY.
> 
> Aside from these, everyone else uses either DMA_CYCLIC in addition to
> DMA_SLAVE, which seems valid, or they use DMA_PRIVATE, which I think is
> redundant in slave drivers and can be removed.

Yep, CYCLIC. How could I forgot that ;)

>>> It's usually not much harder to separate out the legacy case from
>>> the normal dma_request_slave_channel_reason(), so those drivers don't
>>> really need to use the unified compat API.
>>
>> The current dma_request_slave_channel()/_reason() is not the 'legacy' API.
>> Currently there is no way to get the reason why the dma channel request fails
>> when using the _compat() version of the API, which is used by drivers which
>> can be used in DT or in legacy mode as well. Sure, they all could have local
>> if(){}else{} for handling this, but it is not a nice thing.
>>
>> As it was discussed instead of adding the _reason() version for the _compat
>> call, we should simplify the dmaengine API for getting the channel and at the
>> same time we will have ERR_PTR returned instead of NULL.
> 
> What I meant was that we don't need to handle them with the unified
> simple interface. The users of DMA_CYCLIC can just keep using
> an internal helper that only deals with the legacy case, or use
> dma_request_slave() or whatever is the new API for the DT case.

I think we can go with a single API, but I don't really like that:
dma_request_channel(dev, name, *mask, fn, fn_param);

This would cover all current uses being legacy, DT/ACPI, compat, etc:
dma_request_channel(NULL, NULL, &mask, fn, fn_param); /* Legacy slave */
dma_request_channel(NULL, NULL, &mask, NULL, NULL); /* memcpy. etc */
dma_request_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current slave */
dma_request_channel(dev, name, &mask, fn, fn_param); /* current compat */

Note, that we need "const dma_cap_mask_t *mask" to be able to make the mask
optional.

If we have two main APIs, one to request slave channels and one to get any
channel with given capability
dma_request_slave_channel(NULL, NULL, &mask, fn, fn_param); /* Legacy slave */
dma_request_slave_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current
							   slave */
dma_request_slave_channel(dev, name, &mask, fn, fn_param); /* current compat*/

This way we can omit the mask also in cases when the client only want to get
DMA_SLAVE, we can just build up the mask within the function. If the mask is
provided we would copy the bits from the provided mask, so for example if you
want to have DMA_SLAVE+DMA_CYCLIC, the driver only needs to pass DMA_CYCLIC,
the DMA_SLAVE is going to be set anyways.

dma_request_channel(mask); /* memcpy. etc, non slave mostly */

Not sure how to name this as reusing existing (good, descriptive) function
names would mean changes all over the kernel to start off this.

Not used and
request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
request_dma();
dma_channel_request();

All in all, not sure which way would be better...
Peter Ujfalusi Nov. 19, 2015, 10:36 a.m. UTC | #21
On 11/18/2015 05:46 PM, Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 4:21 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
>> Hi Vinod,
>>
>> bringing this old thread back to life as I just started to work on this.
> 
> What I remember we need to convert drivers to use new API meanwhile it
> is good to keep old one to avoid patch storm which does nothing useful
> (IIRC Russel's opinion).

I tend to agree. But we need to start converting the users at some point
either way.
Another issue is the fact that the current dmaengine API is using all the good
names I can think of ;)

> On the other hand there are a lot of drivers that are used on the set
> of platforms starting from legacy and abandoned ones (like AVR32) to
> relatively new and newest.
> 
> And I'm not a fan of those thousands of API calls either.
>
Arnd Bergmann Nov. 19, 2015, 11:25 a.m. UTC | #22
On Thursday 19 November 2015 12:34:22 Peter Ujfalusi wrote:
> 
> I think we can go with a single API, but I don't really like that:
> dma_request_channel(dev, name, *mask, fn, fn_param);
> 
> This would cover all current uses being legacy, DT/ACPI, compat, etc:
> dma_request_channel(NULL, NULL, &mask, fn, fn_param); /* Legacy slave */
> dma_request_channel(NULL, NULL, &mask, NULL, NULL); /* memcpy. etc */
> dma_request_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current slave */
> dma_request_channel(dev, name, &mask, fn, fn_param); /* current compat */
> 
> Note, that we need "const dma_cap_mask_t *mask" to be able to make the mask
> optional.

Right, that would work, but I also don't really like it.

> If we have two main APIs, one to request slave channels and one to get any
> channel with given capability
> dma_request_slave_channel(NULL, NULL, &mask, fn, fn_param); /* Legacy slave */
> dma_request_slave_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current
>                                                            slave */
> dma_request_slave_channel(dev, name, &mask, fn, fn_param); /* current compat*/
> 
> This way we can omit the mask also in cases when the client only want to get
> DMA_SLAVE, we can just build up the mask within the function. If the mask is
> provided we would copy the bits from the provided mask, so for example if you
> want to have DMA_SLAVE+DMA_CYCLIC, the driver only needs to pass DMA_CYCLIC,
> the DMA_SLAVE is going to be set anyways.

I think it's more logical here to have mask=NULL mean that we want DMA_SLAVE,
but otherwise pass the full mask as DMA_SLAVE|DMA_CYCLIC etc.

> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
> 
> Not sure how to name this as reusing existing (good, descriptive) function
> names would mean changes all over the kernel to start off this.
> 
> Not used and
> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
> request_dma();
> dma_channel_request();

dma_request_slavechan();
dma_request_slave();
dma_request_mask();

> All in all, not sure which way would be better...

I think I would prefer the simplest API to have only the dev+name
arguments, as we tend to move that way for all platforms anyway, and it
seems silly to have all drivers pass three NULL arguments all the time.
At the moment, there are 139 references to dma_request_slave_channel_*
in the kernel, and only 46 of them are dma_request_slave_channel_compat.
Out of those 46, a couple can already be converted back to use
dma_request_slave_channel() because the platform now only supports
devicetree based boots and will not go back to platform data.

How about something like

extern struct dma_chan *
__dma_request_chan(struct device *dev, const char *name,
		    const dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param);

static inline struct dma_chan *
dma_request_slavechan(struct device *dev, const char *name)
{
	return __dma_request_chan(dev, name, NULL, NULL, NULL);
}

static inline struct dma_chan *
dma_request_chan(const dma_cap_mask_t *mask)
{
	return __dma_request_chan(NULL, NULL, mask, NULL, NULL);
}

That way the vast majority of drivers can use one of the two nice interfaces
and the rest can be converted to use __dma_request_chan().

On a related topic, we had in the past considered providing a way for
platform code to register a lookup table of some sort, to associate
a device/name pair with a configuration. That would let us use the
simplified dma_request_slavechan(dev, name) pair everywhere. We could
use the same method that we have for clk_register_clkdevs() or
pinctrl_register_map().

Something like either

static struct dma_chan_map myplatform_dma_map[] = {
	{ .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, .arg = (void *)65, },
	{ .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, .arg = (void *)66, },
};

or

static struct dma_chan_map myplatform_dma_map[] = {
	{ .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", .req = 65, },
	{ .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0", .req = 66, },
};

we could even allow a combination of the two, so the simple case just specifies
master and req number, which requires changes to the dmaengine driver, but we could
also do a mass-conversion to the .filter/.arg variant.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Peter Ujfalusi Nov. 20, 2015, 10:25 a.m. UTC | #23
On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
>> If we have two main APIs, one to request slave channels and one to get any
>> channel with given capability
>> dma_request_slave_channel(NULL, NULL, &mask, fn, fn_param); /* Legacy slave */
>> dma_request_slave_channel(dev, name, NULL, NULL, NULL); /* DT/ACPI, current
>>                                                            slave */
>> dma_request_slave_channel(dev, name, &mask, fn, fn_param); /* current compat*/
>>
>> This way we can omit the mask also in cases when the client only want to get
>> DMA_SLAVE, we can just build up the mask within the function. If the mask is
>> provided we would copy the bits from the provided mask, so for example if you
>> want to have DMA_SLAVE+DMA_CYCLIC, the driver only needs to pass DMA_CYCLIC,
>> the DMA_SLAVE is going to be set anyways.
> 
> I think it's more logical here to have mask=NULL mean that we want DMA_SLAVE,
> but otherwise pass the full mask as DMA_SLAVE|DMA_CYCLIC etc.

Yep, could be, while I would write the core part to set the DMA_SLAVE
unconditionally anyways. If the API say it is dma_request_slavechan() it is
expected to get channel which is capable of DMA_SLAVE.

>> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
>>
>> Not sure how to name this as reusing existing (good, descriptive) function
>> names would mean changes all over the kernel to start off this.
>>
>> Not used and
>> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
>> request_dma();
>> dma_channel_request();
> 
> dma_request_slavechan();
> dma_request_slave();
> dma_request_mask();

Let me think aloud here a bit...
1. To request slave channel which will return you the channel your device is
bind via DT/ACPI or the platform map table you propose later:

dma_request_chan(struct device *dev, const char *name);

2. To request a channel (any channel) matching with the capabilities the
driver needs, like memcpy, memset, etc:

#define dma_request_chan_by_mask(mask) __dma_request_chan_by_mask(&(mask))
__dma_request_chan_by_mask(const dma_cap_mask_t *mask);

I think the dma_request_chan() does not need mask to be passed, since via this
we request a specific channel which has been defined/set by DT/ACPI or the
lookup map. We could add a mask parameter which could be used to sanity check
the channel we got against the capabilities the driver needs from the channel.
We currently do this in the drivers where the author wanted to make sure that
the channel is capable of what it should be capable.

So two API to request channel:
struct dma_chan *dma_request_chan(struct device *dev, const char *name);
struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask);

Both will return with the valid channel pointer or in case of failure with
ERR_PTR().

We need to go through the code in regards to return codes also to have sane
mapping.

> 
>> All in all, not sure which way would be better...
> 
> I think I would prefer the simplest API to have only the dev+name
> arguments, as we tend to move that way for all platforms anyway, and it
> seems silly to have all drivers pass three NULL arguments all the time.
> At the moment, there are 139 references to dma_request_slave_channel_*
> in the kernel, and only 46 of them are dma_request_slave_channel_compat.
> Out of those 46, a couple can already be converted back to use
> dma_request_slave_channel() because the platform now only supports
> devicetree based boots and will not go back to platform data.
> 
> How about something like
> 
> extern struct dma_chan *
> __dma_request_chan(struct device *dev, const char *name,
> 		    const dma_cap_mask_t *mask, dma_filter_fn fn, void *fn_param);
> 
> static inline struct dma_chan *
> dma_request_slavechan(struct device *dev, const char *name)
> {
> 	return __dma_request_chan(dev, name, NULL, NULL, NULL);
> }
> 
> static inline struct dma_chan *
> dma_request_chan(const dma_cap_mask_t *mask)
> {
> 	return __dma_request_chan(NULL, NULL, mask, NULL, NULL);
> }
> 
> That way the vast majority of drivers can use one of the two nice interfaces
> and the rest can be converted to use __dma_request_chan().
> 
> On a related topic, we had in the past considered providing a way for
> platform code to register a lookup table of some sort, to associate
> a device/name pair with a configuration. That would let us use the
> simplified dma_request_slavechan(dev, name) pair everywhere. We could
> use the same method that we have for clk_register_clkdevs() or
> pinctrl_register_map().
> 
> Something like either
> 
> static struct dma_chan_map myplatform_dma_map[] = {
> 	{ .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, .arg = (void *)65, },
> 	{ .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, .arg = (void *)66, },
> };
> 
> or
> 
> static struct dma_chan_map myplatform_dma_map[] = {
> 	{ .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", .req = 65, },
> 	{ .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0", .req = 66, },

sa11x0-dma expects the fn_param as string :o

> };

Basically we are deprecating the use of IORESOURCE_DMA?

For legacy the filter function is pretty much needed to handle the differences
between the platforms as not all of them does the filtering in a same way. So
the first type of map would be feasible IMHO.

> we could even allow a combination of the two, so the simple case just specifies
> master and req number, which requires changes to the dmaengine driver, but we could
> also do a mass-conversion to the .filter/.arg variant.

This will get rid of the need for the fn and fn_param parameters when
requesting dma channel, but it will not get rid of the exported function from
the dma engine drivers since in arch code we need to have visibility to the
filter_fn.
Arnd Bergmann Nov. 20, 2015, 10:58 a.m. UTC | #24
On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
> >> dma_request_channel(mask); /* memcpy. etc, non slave mostly */
> >>
> >> Not sure how to name this as reusing existing (good, descriptive) function
> >> names would mean changes all over the kernel to start off this.
> >>
> >> Not used and
> >> request_dma_channel(); /* as _irq/_mem_region/_resource, etc */
> >> request_dma();
> >> dma_channel_request();
> > 
> > dma_request_slavechan();
> > dma_request_slave();
> > dma_request_mask();
> 
> Let me think aloud here a bit...
> 1. To request slave channel which will return you the channel your device is
> bind via DT/ACPI or the platform map table you propose later:
> 
> dma_request_chan(struct device *dev, const char *name);
> 
> 2. To request a channel (any channel) matching with the capabilities the
> driver needs, like memcpy, memset, etc:
> 
> #define dma_request_chan_by_mask(mask) __dma_request_chan_by_mask(&(mask))
> __dma_request_chan_by_mask(const dma_cap_mask_t *mask);
> 
> I think the dma_request_chan() does not need mask to be passed, since via this
> we request a specific channel which has been defined/set by DT/ACPI or the
> lookup map. We could add a mask parameter which could be used to sanity check
> the channel we got against the capabilities the driver needs from the channel.
> We currently do this in the drivers where the author wanted to make sure that
> the channel is capable of what it should be capable.
> 
> So two API to request channel:
> struct dma_chan *dma_request_chan(struct device *dev, const char *name);
> struct dma_chan *dma_request_chan_by_mask(const dma_cap_mask_t *mask);
> 
> Both will return with the valid channel pointer or in case of failure with
> ERR_PTR().
> 
> We need to go through the code in regards to return codes also to have sane
> mapping.

Right.
> > That way the vast majority of drivers can use one of the two nice interfaces
> > and the rest can be converted to use __dma_request_chan().
> > 
> > On a related topic, we had in the past considered providing a way for
> > platform code to register a lookup table of some sort, to associate
> > a device/name pair with a configuration. That would let us use the
> > simplified dma_request_slavechan(dev, name) pair everywhere. We could
> > use the same method that we have for clk_register_clkdevs() or
> > pinctrl_register_map().
> > 
> > Something like either
> > 
> > static struct dma_chan_map myplatform_dma_map[] = {
> > 	{ .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, .arg = (void *)65, },
> > 	{ .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, .arg = (void *)66, },
> > };
> > 
> > or
> > 
> > static struct dma_chan_map myplatform_dma_map[] = {
> > 	{ .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", .req = 65, },
> > 	{ .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0", .req = 66, },
> 
> sa11x0-dma expects the fn_param as string :o

Some of them do, but the new API requires changes in both the DMA master and
slave drivers, so that could be changed if we wanted to, or we just allow 
both methods indefinitely and let sa11x0-dma pass the filterfn+data rather than
a number.

> > };
> 
> Basically we are deprecating the use of IORESOURCE_DMA?

I thought we already had ;-)

> For legacy the filter function is pretty much needed to handle the differences
> between the platforms as not all of them does the filtering in a same way. So
> the first type of map would be feasible IMHO.

It certainly makes the transition to a map table much easier.

> > we could even allow a combination of the two, so the simple case just specifies
> > master and req number, which requires changes to the dmaengine driver, but we could
> > also do a mass-conversion to the .filter/.arg variant.
> 
> This will get rid of the need for the fn and fn_param parameters when
> requesting dma channel, but it will not get rid of the exported function from
> the dma engine drivers since in arch code we need to have visibility to the
> filter_fn.

Correct. A lot of dmaengine drivers already need to be built-in so the platform
code can put a pointer to the filter function, so it would not be worse for them.

Another idea would be to remove the filter function from struct dma_chan_map
and pass the map through platform data to the dmaengine driver, which then
registers it to the core along with the mask. Something like:

/* platform code */
static struct dma_chan_map oma_dma_map[] = {
 	{ .devname = "omap-aes0", .slave = "tx", .arg = (void *)65, },
 	{ .devname = "omap-aes0", .slave = "rx", .arg = (void *)66, },
	...
	{},
};

static struct omap_system_dma_plat_info dma_plat_info __initdata = {
	.dma_map = &oma_dma_map,
	...
};      

machine_init(void)
{
	...
	platform_device_register_data(NULL, "omap-dma-engine", 0, &dma_plat_info, sizeof(dma_plat_info);
	...
}

/* dmaengine driver */

static int omap_dma_probe(struct platform_device *pdev)
{
	struct omap_system_dma_plat_info *pdata = dev_get_platdata(&pdev->dev);
	...

	dmam_register_platform_map(&pdev->dev, omap_dma_filter_fn, pdata->dma_map);
}

/* dmaengine core */

struct dma_map_list {
	struct list_head node;
	struct device *master;
	dma_filter_fn filter;
	struct dma_chan_map *map;
};

static LIST_HEAD(dma_map_list);
static DEFINE_MUTEX(dma_map_mutex);

int dmam_register_platform_map(struct device *dev, dma_filter_fn filter, struct dma_chan_map *map)
{
	struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);

	if (!list)
		return -ENOMEM;

	list->dev = dev;
	list->filter = filter;
	list->map = map;

	mutex_lock(&dma_map_mutex);
	list_add(&dma_map_list, &list->node);
	mutex_unlock(&dma_map_mutex);
}

Now we can completely remove the dependency on the filter function definition
from platform code and slave drivers.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Nov. 20, 2015, 12:24 p.m. UTC | #25
On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
>> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:

> Another idea would be to remove the filter function from struct dma_chan_map
> and pass the map through platform data

Why not unified device properties?
Peter Ujfalusi Nov. 20, 2015, 12:30 p.m. UTC | #26
On 11/20/2015 02:24 PM, Andy Shevchenko wrote:
> On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
>>> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
> 
>> Another idea would be to remove the filter function from struct dma_chan_map
>> and pass the map through platform data
> 
> Why not unified device properties?

Is this some Windows/ACPI feature? Quick search gives mostly MSDN and
Windows10 related links.

We only need dma_chan_map for platforms which has not been converted to DT and
still using legacy boot. Or platforms which can still boot in legacy mode. In
DT/ACPI mode we do not need this map at all.
Peter Ujfalusi Nov. 20, 2015, 12:52 p.m. UTC | #27
On 11/20/2015 12:58 PM, Arnd Bergmann wrote:
>>> That way the vast majority of drivers can use one of the two nice interfaces
>>> and the rest can be converted to use __dma_request_chan().
>>>
>>> On a related topic, we had in the past considered providing a way for
>>> platform code to register a lookup table of some sort, to associate
>>> a device/name pair with a configuration. That would let us use the
>>> simplified dma_request_slavechan(dev, name) pair everywhere. We could
>>> use the same method that we have for clk_register_clkdevs() or
>>> pinctrl_register_map().
>>>
>>> Something like either
>>>
>>> static struct dma_chan_map myplatform_dma_map[] = {
>>> 	{ .devname = "omap-aes0", .slave = "tx", .filter = omap_dma_filter_fn, .arg = (void *)65, },
>>> 	{ .devname = "omap-aes0", .slave = "rx", .filter = omap_dma_filter_fn, .arg = (void *)66, },
>>> };
>>>
>>> or
>>>
>>> static struct dma_chan_map myplatform_dma_map[] = {
>>> 	{ .devname = "omap-aes0", .slave = "tx", .master = "omap-dma-engine0", .req = 65, },
>>> 	{ .devname = "omap-aes0", .slave = "rx", .master = "omap-dma-engine0", .req = 66, },
>>
>> sa11x0-dma expects the fn_param as string :o
> 
> Some of them do, but the new API requires changes in both the DMA master and
> slave drivers, so that could be changed if we wanted to, or we just allow 
> both methods indefinitely and let sa11x0-dma pass the filterfn+data rather than
> a number.

Hrm, I would say that we need to push everyone to use the new API. sa11x0
should not be a big deal to fix IMHO and other users should be reasonably
simple to convert.

>>> };
>>
>> Basically we are deprecating the use of IORESOURCE_DMA?
> 
> I thought we already had ;-)

For DT boot, yes. Not for the legacy boot.

>> For legacy the filter function is pretty much needed to handle the differences
>> between the platforms as not all of them does the filtering in a same way. So
>> the first type of map would be feasible IMHO.
> 
> It certainly makes the transition to a map table much easier.

And the aim anyway is to convert everything to DT, right?

>>> we could even allow a combination of the two, so the simple case just specifies
>>> master and req number, which requires changes to the dmaengine driver, but we could
>>> also do a mass-conversion to the .filter/.arg variant.
>>
>> This will get rid of the need for the fn and fn_param parameters when
>> requesting dma channel, but it will not get rid of the exported function from
>> the dma engine drivers since in arch code we need to have visibility to the
>> filter_fn.
> 
> Correct. A lot of dmaengine drivers already need to be built-in so the platform
> code can put a pointer to the filter function, so it would not be worse for them.
> 
> Another idea would be to remove the filter function from struct dma_chan_map
> and pass the map through platform data to the dmaengine driver, which then
> registers it to the core along with the mask. Something like:
> 
> /* platform code */
> static struct dma_chan_map oma_dma_map[] = {
>  	{ .devname = "omap-aes0", .slave = "tx", .arg = (void *)65, },
>  	{ .devname = "omap-aes0", .slave = "rx", .arg = (void *)66, },
> 	...
> 	{},
> };
> 
> static struct omap_system_dma_plat_info dma_plat_info __initdata = {
> 	.dma_map = &oma_dma_map,
> 	...
> };      
> 
> machine_init(void)
> {
> 	...
> 	platform_device_register_data(NULL, "omap-dma-engine", 0, &dma_plat_info, sizeof(dma_plat_info);
> 	...
> }
> 
> /* dmaengine driver */
> 
> static int omap_dma_probe(struct platform_device *pdev)
> {
> 	struct omap_system_dma_plat_info *pdata = dev_get_platdata(&pdev->dev);
> 	...
> 
> 	dmam_register_platform_map(&pdev->dev, omap_dma_filter_fn, pdata->dma_map);
> }
> 
> /* dmaengine core */
> 
> struct dma_map_list {
> 	struct list_head node;
> 	struct device *master;
> 	dma_filter_fn filter;
> 	struct dma_chan_map *map;
> };
> 
> static LIST_HEAD(dma_map_list);
> static DEFINE_MUTEX(dma_map_mutex);
> 
> int dmam_register_platform_map(struct device *dev, dma_filter_fn filter, struct dma_chan_map *map)
> {
> 	struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);
> 
> 	if (!list)
> 		return -ENOMEM;
> 
> 	list->dev = dev;
> 	list->filter = filter;
> 	list->map = map;
> 
> 	mutex_lock(&dma_map_mutex);
> 	list_add(&dma_map_list, &list->node);
> 	mutex_unlock(&dma_map_mutex);
> }
> 
> Now we can completely remove the dependency on the filter function definition
> from platform code and slave drivers.

Sounds feasible for OMAP and daVinci and for others as well. I think ;)
I would go with this if someone asks my opinion :D

The core change to add the new API + the dma_map support should be pretty
straight forward. It can live alongside with the old API and we can phase out
the users of the old one.
The legacy support would need more time since we need to modify the arch codes
and the corresponding DMA drivers to get the map registered, but after that
the remaining drivers can be converted to use the new API.
Arnd Bergmann Nov. 20, 2015, 1:48 p.m. UTC | #28
On Friday 20 November 2015 14:52:03 Peter Ujfalusi wrote:
> 
> >> For legacy the filter function is pretty much needed to handle the differences
> >> between the platforms as not all of them does the filtering in a same way. So
> >> the first type of map would be feasible IMHO.
> > 
> > It certainly makes the transition to a map table much easier.
> 
> And the aim anyway is to convert everything to DT, right?

We won't be able to do that. Some architectures (avr32 and sh for instance)
use the dmaengine API but will likely never support DT. On ARM, at
least sa1100 is in the same category, probably also ep93xx and portions
of pxa, omap1 and davinci.

> > int dmam_register_platform_map(struct device *dev, dma_filter_fn filter, struct dma_chan_map *map)
> > {
> >       struct dma_map_list *list = kmalloc(sizeof(*list), GFP_KERNEL);
> > 
> >       if (!list)
> >               return -ENOMEM;
> > 
> >       list->dev = dev;
> >       list->filter = filter;
> >       list->map = map;
> > 
> >       mutex_lock(&dma_map_mutex);
> >       list_add(&dma_map_list, &list->node);
> >       mutex_unlock(&dma_map_mutex);
> > }
> > 
> > Now we can completely remove the dependency on the filter function definition
> > from platform code and slave drivers.
> 
> Sounds feasible for OMAP and daVinci and for others as well. I think 
> I would go with this if someone asks my opinion 

Ok.

> The core change to add the new API + the dma_map support should be pretty
> straight forward. It can live alongside with the old API and we can phase out
> the users of the old one.
> The legacy support would need more time since we need to modify the arch codes
> and the corresponding DMA drivers to get the map registered, but after that
> the remaining drivers can be converted to use the new API.

Right. It's not urgent and as long as we agree on the overall approach, we can
always do the platform support first and wait for the following merge window
before moving over the slave drivers.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Andy Shevchenko Nov. 20, 2015, 2:08 p.m. UTC | #29
On Fri, Nov 20, 2015 at 2:30 PM, Peter Ujfalusi <peter.ujfalusi@ti.com> wrote:
> On 11/20/2015 02:24 PM, Andy Shevchenko wrote:
>> On Fri, Nov 20, 2015 at 12:58 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>>> On Friday 20 November 2015 12:25:06 Peter Ujfalusi wrote:
>>>> On 11/19/2015 01:25 PM, Arnd Bergmann wrote:
>>
>>> Another idea would be to remove the filter function from struct dma_chan_map
>>> and pass the map through platform data
>>
>> Why not unified device properties?
>
> Is this some Windows/ACPI feature?

Nope.

Check drivers/base/property.c

> Quick search gives mostly MSDN and
> Windows10 related links.
>
> We only need dma_chan_map for platforms which has not been converted to DT and
> still using legacy boot. Or platforms which can still boot in legacy mode. In
> DT/ACPI mode we do not need this map at all.

Patch
diff mbox

diff --git a/include/linux/dmaengine.h b/include/linux/dmaengine.h
index abf63ceabef9..6c777394835c 100644
--- a/include/linux/dmaengine.h
+++ b/include/linux/dmaengine.h
@@ -1120,4 +1120,26 @@  static inline struct dma_chan
 
 	return __dma_request_channel(mask, fn, fn_param);
 }
+
+#define dma_request_slave_channel_compat_reason(mask, x, y, dev, name) \
+	__dma_request_slave_channel_compat_reason(&(mask), x, y, dev, name)
+
+static inline struct dma_chan
+*__dma_request_slave_channel_compat_reason(const dma_cap_mask_t *mask,
+				  dma_filter_fn fn, void *fn_param,
+				  struct device *dev, char *name)
+{
+	struct dma_chan *chan;
+
+	chan = dma_request_slave_channel_reason(dev, name);
+	/* Try via legacy API if not requesting for deferred probing */
+	if (IS_ERR(chan) && PTR_ERR(chan) != -EPROBE_DEFER)
+		chan = __dma_request_channel(mask, fn, fn_param);
+
+	if (!chan)
+		chan = ERR_PTR(-ENODEV);
+
+	return chan;
+}
+
 #endif /* DMAENGINE_H */