From mboxrd@z Thu Jan 1 00:00:00 1970 From: Arnd Bergmann Subject: Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Date: Fri, 20 Nov 2015 11:58:57 +0100 Message-ID: <6118451.vaLZWOZEF5@wuerfel> References: <1432646768-12532-1-git-send-email-peter.ujfalusi@ti.com> <4533695.7ZVFN1S94o@wuerfel> <564EF502.6040708@ti.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: "devicetree@vger.kernel.org" , ALSA Development Mailing List , Vinod Koul , Linux MMC List , "linux-kernel@vger.kernel.org" , linux-spi , Tony Lindgren , Geert Uytterhoeven , linux-crypto@vger.kernel.org, "linux-serial@vger.kernel.org" , dmaengine@vger.kernel.org, Dan Williams , "linux-omap@vger.kernel.org" , Linux Media Mailing List To: Peter Ujfalusi Return-path: In-Reply-To: <564EF502.6040708@ti.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: alsa-devel-bounces@alsa-project.org Sender: alsa-devel-bounces@alsa-project.org List-Id: linux-crypto.vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1162113AbbKTK7k (ORCPT ); Fri, 20 Nov 2015 05:59:40 -0500 Received: from mout.kundenserver.de ([212.227.17.13]:59839 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1759620AbbKTK7R (ORCPT ); Fri, 20 Nov 2015 05:59:17 -0500 From: Arnd Bergmann To: Peter Ujfalusi Cc: Vinod Koul , Geert Uytterhoeven , Tony Lindgren , "devicetree@vger.kernel.org" , "linux-kernel@vger.kernel.org" , Dan Williams , dmaengine@vger.kernel.org, "linux-serial@vger.kernel.org" , "linux-omap@vger.kernel.org" , Linux MMC List , linux-crypto@vger.kernel.org, linux-spi , Linux Media Mailing List , ALSA Development Mailing List Subject: Re: [PATCH 02/13] dmaengine: Introduce dma_request_slave_channel_compat_reason() Date: Fri, 20 Nov 2015 11:58:57 +0100 Message-ID: <6118451.vaLZWOZEF5@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <564EF502.6040708@ti.com> References: <1432646768-12532-1-git-send-email-peter.ujfalusi@ti.com> <4533695.7ZVFN1S94o@wuerfel> <564EF502.6040708@ti.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:VhZ8oBDqW6ihNPfTBrjb3H0xzMbclW17N7hVWeso0idSrSuFSD/ F0URtoYBJNMYMTfxY589lIytC8VU61Ynm9wFAcsgeITn3VYzEGB4T2i9QzE+n83prdfC0/M o3GBcI4tCcfY+yqeYw9Q1K0SouRB8YSSRGT7GVdWj0hSxwi7Zb2PVIkrKA+ZBCElda7Km8Q jYNbMBgl3FjtHfGE2ut0Q== X-UI-Out-Filterresults: notjunk:1;V01:K0:b9P3tF6fZ1M=:NFDzLXMkt1KhmmyD6kbJCZ Mr9YOcRdsmG7SeymWNSzJb4a5oGFuMZ3jzdwuf1qGx3/dv+NLSaG0wusBDSkIOHev0y2haUat 2HZKzAlL2SiOtvApNgON7+cYzj/ViOwO8E2hVooA/o+uh96Z1rLr0IuzdTEli1cxLcm6d1a/m Pc285B+89kM6JXX1sfHQ+b3dNvH5IdYtNKJVCAAiMb0niZ1FRcaM5JCxHMja8nCeUbK21Z3Sx mGZvMfaqQGsXcmEP6gG9zY/yfKyY6U2qZkn50jI/uzrk/7ZXg3++UTGRzS6qdSvJPTPvkJ63y C+diBCEGW+BTS03CjR62TEE44yTjr+Yquhfokt779EFRVJmZLEp60ByTZDMEZ0F/f+OO4639c b1SsIdtHnCiAr8g/3akNsbQC1uuyMyUSgiahKYrmuzrgx81BMqhp8tNldlMkOM53zJWmaR4ZJ jstmSK5letr8SNMekuRz3OPZIL5ZXDfb/NcwMaJGnL5Rkuaa89rg/xWN0wvpnmlPvyRF4Ruc4 W+OjP3PpH3mHQVuaBWl5AP9aS5Qq71M2kflMgsoN5TlGddztEf6MsxQUWgJSEmnlLzGyD5UGQ 1AJtQDjIXm1KOL19EZBq/BXcubxz4A8etdL5/MvUNJL2FLQvvo+jtmmRPAwcBmiQoVnaw1baE 0jry1iYpD4e0USv3QGdzQFLRE0KMDVPqQleJrSJgkyjLXQWS7EZBgOIR7BMZrJUl2u0fuYuDM 1W6wWKHryQ7AhBt5 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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