From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Ujfalusi Subject: Re: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver Date: Thu, 13 Mar 2014 13:56:56 +0200 Message-ID: <53219D08.9070308@ti.com> References: <1394702320-21743-1-git-send-email-peter.ujfalusi@ti.com> <1394702320-21743-15-git-send-email-peter.ujfalusi@ti.com> <53218835.3080909@metafoo.de> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: <53218835.3080909-Qo5EllUWu/uELgA04lAiVw@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: Errors-To: davinci-linux-open-source-bounces-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org To: Lars-Peter Clausen Cc: davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org, alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org, joelf-l0cyMroinI0@public.gmane.org, vinod.koul-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Jyri Sarha , Liam Girdwood , Tony Lindgren , Mark Brown , mporter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org, dmaengine-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, dan.j.williams-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: alsa-devel@alsa-project.org On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: > On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...] >> +static const struct snd_pcm_hardware edma_pcm_hardware =3D { + .info >> =3D SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID |= + >> SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | >> SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_INTERLEAVED, + >> .buffer_bytes_max =3D 128 * 1024, + .period_bytes_min =3D 32, + >> .period_bytes_max =3D 64 * 1024, + .periods_min =3D 2, + >> .periods_max =3D 19, /* Limit by edma dmaengine driver */ +}; > = > The idea is that we can auto-discover all the things using the > dma_slave_caps API. Too bad we removed the possibility to specify the > maximum number of segments from the API. Maybe we need to add it back. Is > the 19 a hard-limit or could it be worked around by software in the > dmaengine driver? The edma dmaengine driver will refuse to configure more than 20 paRAM slots= (1 for the channel + 19 for the periods). Depending on the SoC we might have different number of paRAM entries available. The intention with the limit w= as to prevent cases when we run out of paRAM entires for other devices because audio took most of them. >> + +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config >> =3D { + .pcm_hardware =3D &edma_pcm_hardware, + .prepare_slave_con= fig =3D >> snd_dmaengine_pcm_prepare_slave_config, + .prealloc_buffer_size =3D 1= 28 >> * 1024, > = > Unless there is a very good reason for exactly this size, just leave it 0 > and let the generic dmaengine driver use the default. I'd rather keep this here. Since I have the .pcm_hardware the core will ign= ore the snd_pcm_hardware.buffer_bytes_max when preallocating and it is in fact going to go for: prealloc_buffer_size =3D 512 * 1024; max_buffer_size =3D SIZE_MAX; While I have 128 * 1024 for the snd_pcm_hardware.buffer_bytes_max. I think as long as I have the .pcm_hardware provided from the edma-pcm driv= er I will have the .prealloc_buffer_size as well. >> +}; + +static const struct snd_dmaengine_pcm_config = >> edma_compat_dmaengine_pcm_config =3D { + .pcm_hardware =3D >> &edma_pcm_hardware, + .prepare_slave_config =3D >> snd_dmaengine_pcm_prepare_slave_config, + .compat_filter_fn =3D >> edma_filter_fn, + .prealloc_buffer_size =3D 128 * 1024, +}; > = > There is no need for different configs for DT and non-DT. > = >> + +int edma_pcm_platform_register(struct device *dev) +{ + if >> (dev->of_node) + return snd_dmaengine_pcm_register(dev, + >> &edma_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); > = > Since the edma dmaengine driver implements the slave cap API there is no > need to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But > since the edma driver sets the granularity to > DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this case the generic dmaengine will > not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE automatically since it assumes > that the dmaengine driver is capable of properly reporting the DMA > position. > = >> + else + return snd_dmaengine_pcm_register(dev, + >> &edma_compat_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | + >> SND_DMAENGINE_PCM_FLAG_NO_DT | + >> SND_DMAENGINE_PCM_FLAG_COMPAT); > = > = > If you set the flags to just SND_DMAENGINE_PCM_FLAG_COMPAT it will do the = > right thing in the generic dmaengine driver depending on whether > dev->of_node is set or not. I have missed these in the core. Makes the code simpler for sure. > There is also a devm_ version of snd_dmaengine_pcm_register() it probably = > makes sense to use it here. I forgot about the devm_ version. True, I'll use that instead. -- = P=E9ter From mboxrd@z Thu Jan 1 00:00:00 1970 From: peter.ujfalusi@ti.com (Peter Ujfalusi) Date: Thu, 13 Mar 2014 13:56:56 +0200 Subject: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver In-Reply-To: <53218835.3080909@metafoo.de> References: <1394702320-21743-1-git-send-email-peter.ujfalusi@ti.com> <1394702320-21743-15-git-send-email-peter.ujfalusi@ti.com> <53218835.3080909@metafoo.de> Message-ID: <53219D08.9070308@ti.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: > On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...] >> +static const struct snd_pcm_hardware edma_pcm_hardware = { + .info >> = SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + >> SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | >> SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_INTERLEAVED, + >> .buffer_bytes_max = 128 * 1024, + .period_bytes_min = 32, + >> .period_bytes_max = 64 * 1024, + .periods_min = 2, + >> .periods_max = 19, /* Limit by edma dmaengine driver */ +}; > > The idea is that we can auto-discover all the things using the > dma_slave_caps API. Too bad we removed the possibility to specify the > maximum number of segments from the API. Maybe we need to add it back. Is > the 19 a hard-limit or could it be worked around by software in the > dmaengine driver? The edma dmaengine driver will refuse to configure more than 20 paRAM slots (1 for the channel + 19 for the periods). Depending on the SoC we might have different number of paRAM entries available. The intention with the limit was to prevent cases when we run out of paRAM entires for other devices because audio took most of them. >> + +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config >> = { + .pcm_hardware = &edma_pcm_hardware, + .prepare_slave_config = >> snd_dmaengine_pcm_prepare_slave_config, + .prealloc_buffer_size = 128 >> * 1024, > > Unless there is a very good reason for exactly this size, just leave it 0 > and let the generic dmaengine driver use the default. I'd rather keep this here. Since I have the .pcm_hardware the core will ignore the snd_pcm_hardware.buffer_bytes_max when preallocating and it is in fact going to go for: prealloc_buffer_size = 512 * 1024; max_buffer_size = SIZE_MAX; While I have 128 * 1024 for the snd_pcm_hardware.buffer_bytes_max. I think as long as I have the .pcm_hardware provided from the edma-pcm driver I will have the .prealloc_buffer_size as well. >> +}; + +static const struct snd_dmaengine_pcm_config >> edma_compat_dmaengine_pcm_config = { + .pcm_hardware = >> &edma_pcm_hardware, + .prepare_slave_config = >> snd_dmaengine_pcm_prepare_slave_config, + .compat_filter_fn = >> edma_filter_fn, + .prealloc_buffer_size = 128 * 1024, +}; > > There is no need for different configs for DT and non-DT. > >> + +int edma_pcm_platform_register(struct device *dev) +{ + if >> (dev->of_node) + return snd_dmaengine_pcm_register(dev, + >> &edma_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); > > Since the edma dmaengine driver implements the slave cap API there is no > need to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But > since the edma driver sets the granularity to > DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this case the generic dmaengine will > not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE automatically since it assumes > that the dmaengine driver is capable of properly reporting the DMA > position. > >> + else + return snd_dmaengine_pcm_register(dev, + >> &edma_compat_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | + >> SND_DMAENGINE_PCM_FLAG_NO_DT | + >> SND_DMAENGINE_PCM_FLAG_COMPAT); > > > If you set the flags to just SND_DMAENGINE_PCM_FLAG_COMPAT it will do the > right thing in the generic dmaengine driver depending on whether > dev->of_node is set or not. I have missed these in the core. Makes the code simpler for sure. > There is also a devm_ version of snd_dmaengine_pcm_register() it probably > makes sense to use it here. I forgot about the devm_ version. True, I'll use that instead. -- P?ter From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753957AbaCML5f (ORCPT ); Thu, 13 Mar 2014 07:57:35 -0400 Received: from comal.ext.ti.com ([198.47.26.152]:37045 "EHLO comal.ext.ti.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753565AbaCML5d (ORCPT ); Thu, 13 Mar 2014 07:57:33 -0400 Message-ID: <53219D08.9070308@ti.com> Date: Thu, 13 Mar 2014 13:56:56 +0200 From: Peter Ujfalusi User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:24.0) Gecko/20100101 Thunderbird/24.3.0 MIME-Version: 1.0 To: Lars-Peter Clausen CC: , , , , , , , , Liam Girdwood , Jyri Sarha , Tony Lindgren , Mark Brown , , , Subject: Re: [alsa-devel] [PATCH 14/18] ASoC: davinci: Add edma dmaengine platform driver References: <1394702320-21743-1-git-send-email-peter.ujfalusi@ti.com> <1394702320-21743-15-git-send-email-peter.ujfalusi@ti.com> <53218835.3080909@metafoo.de> In-Reply-To: <53218835.3080909@metafoo.de> Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 03/13/2014 12:28 PM, Lars-Peter Clausen wrote: > On 03/13/2014 10:18 AM, Peter Ujfalusi wrote: [...] >> +static const struct snd_pcm_hardware edma_pcm_hardware = { + .info >> = SNDRV_PCM_INFO_MMAP | + SNDRV_PCM_INFO_MMAP_VALID | + >> SNDRV_PCM_INFO_BATCH | + SNDRV_PCM_INFO_PAUSE | >> SNDRV_PCM_INFO_RESUME | + SNDRV_PCM_INFO_INTERLEAVED, + >> .buffer_bytes_max = 128 * 1024, + .period_bytes_min = 32, + >> .period_bytes_max = 64 * 1024, + .periods_min = 2, + >> .periods_max = 19, /* Limit by edma dmaengine driver */ +}; > > The idea is that we can auto-discover all the things using the > dma_slave_caps API. Too bad we removed the possibility to specify the > maximum number of segments from the API. Maybe we need to add it back. Is > the 19 a hard-limit or could it be worked around by software in the > dmaengine driver? The edma dmaengine driver will refuse to configure more than 20 paRAM slots (1 for the channel + 19 for the periods). Depending on the SoC we might have different number of paRAM entries available. The intention with the limit was to prevent cases when we run out of paRAM entires for other devices because audio took most of them. >> + +static const struct snd_dmaengine_pcm_config edma_dmaengine_pcm_config >> = { + .pcm_hardware = &edma_pcm_hardware, + .prepare_slave_config = >> snd_dmaengine_pcm_prepare_slave_config, + .prealloc_buffer_size = 128 >> * 1024, > > Unless there is a very good reason for exactly this size, just leave it 0 > and let the generic dmaengine driver use the default. I'd rather keep this here. Since I have the .pcm_hardware the core will ignore the snd_pcm_hardware.buffer_bytes_max when preallocating and it is in fact going to go for: prealloc_buffer_size = 512 * 1024; max_buffer_size = SIZE_MAX; While I have 128 * 1024 for the snd_pcm_hardware.buffer_bytes_max. I think as long as I have the .pcm_hardware provided from the edma-pcm driver I will have the .prealloc_buffer_size as well. >> +}; + +static const struct snd_dmaengine_pcm_config >> edma_compat_dmaengine_pcm_config = { + .pcm_hardware = >> &edma_pcm_hardware, + .prepare_slave_config = >> snd_dmaengine_pcm_prepare_slave_config, + .compat_filter_fn = >> edma_filter_fn, + .prealloc_buffer_size = 128 * 1024, +}; > > There is no need for different configs for DT and non-DT. > >> + +int edma_pcm_platform_register(struct device *dev) +{ + if >> (dev->of_node) + return snd_dmaengine_pcm_register(dev, + >> &edma_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE); > > Since the edma dmaengine driver implements the slave cap API there is no > need to manually specify SND_DMAENGINE_PCM_FLAG_NO_RESIDUE manually. But > since the edma driver sets the granularity to > DMA_RESIDUE_GRANULARITY_DESCRIPTOR in this case the generic dmaengine will > not set SND_DMAENGINE_PCM_FLAG_NO_RESIDUE automatically since it assumes > that the dmaengine driver is capable of properly reporting the DMA > position. > >> + else + return snd_dmaengine_pcm_register(dev, + >> &edma_compat_dmaengine_pcm_config, + >> SND_DMAENGINE_PCM_FLAG_NO_RESIDUE | + >> SND_DMAENGINE_PCM_FLAG_NO_DT | + >> SND_DMAENGINE_PCM_FLAG_COMPAT); > > > If you set the flags to just SND_DMAENGINE_PCM_FLAG_COMPAT it will do the > right thing in the generic dmaengine driver depending on whether > dev->of_node is set or not. I have missed these in the core. Makes the code simpler for sure. > There is also a devm_ version of snd_dmaengine_pcm_register() it probably > makes sense to use it here. I forgot about the devm_ version. True, I'll use that instead. -- Péter