Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1272213 > unrolled thread

Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Arnd Bergmann <arnd@arndb.de> - 2015-11-18 15:30 +0100
    Re: [PATCH 02/13] dmaengine: Introduce  dma_request_slave_channel_compat_reason() Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-11-18 15:50 +0100
      Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Arnd Bergmann <arnd@arndb.de> - 2015-11-18 16:10 +0100
        Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-18 16:50 +0100
          Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Arnd Bergmann <arnd@arndb.de> - 2015-11-18 17:00 +0100
            Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-18 17:10 +0100
            Re: [PATCH 02/13] dmaengine: Introduce  dma_request_slave_channel_compat_reason() Vinod Koul <vinod.koul@intel.com> - 2015-11-18 17:10 +0100
        Re: [PATCH 02/13] dmaengine: Introduce  dma_request_slave_channel_compat_reason() Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-11-19 11:40 +0100
          Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Arnd Bergmann <arnd@arndb.de> - 2015-11-19 12:30 +0100
            Re: [PATCH 02/13] dmaengine: Introduce  dma_request_slave_channel_compat_reason() Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-11-20 11:30 +0100
              Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Arnd Bergmann <arnd@arndb.de> - 2015-11-20 12:00 +0100
                Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-20 13:30 +0100
                  Re: [PATCH 02/13] dmaengine: Introduce  dma_request_slave_channel_compat_reason() Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-11-20 13:40 +0100
                    Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Andy Shevchenko <andy.shevchenko@gmail.com> - 2015-11-20 15:10 +0100
                Re: [PATCH 02/13] dmaengine: Introduce  dma_request_slave_channel_compat_reason() Peter Ujfalusi <peter.ujfalusi@ti.com> - 2015-11-20 14:00 +0100
                  Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Arnd Bergmann <arnd@arndb.de> - 2015-11-20 14:50 +0100

#1272213 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1272252 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272280

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272313

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.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272324

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272331

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.

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1272332 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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

-- 
~Vinod
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273014 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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...

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273056

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273953 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1273974

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274028

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?

-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274041 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274083

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.


-- 
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274055 — Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason()

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.

-- 
Péter
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1274071

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-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web