Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Tue, 2011-08-16 at 20:21 +0530, Jassi Brar wrote: On Tue, Aug 16, 2011 at 5:25 PM, Koul, Vinod vinod.k...@intel.com wrote: Sorry I still don't get this schema and how it can be scaled and be generic enough to let it carry with various implementations. Can you publish your complete idea rather than bits and pieces... I already explained the complete idea. I don't have any implementation yet. Clients and dmaengine.c is easier to manage. But changes to 20 dmac drivers is the biggest effort - though they anyway need such modifications if we are to have the DMAENGINE utopia someday. In free time, I will modify a dmac driver or two, but it might take prohibitively long if I am expected to update possibly all the 20 dmac drivers and the backend platforms by It would help if you can send RFC of changes in one driver and dmaengine changes. I want to get the dmaengine changes understood well and be able to deal with all scenarios we have. Without complete code its rather hard :( We can do these changes to other drivers over a period of time, that can be taken over a period of time and we can manage that, this part is easy. a) Making dmac drivers platform agnostic and for re-routable ReqSig-Peri map. That implies dmac drivers managing 'virtual-channel' front end and physical channel and ReqSig-Peri link management in the backend with help from platform. b) Modifying platforms/boards to pass channel map and link re-routing callback pointers to generic dmac drivers. -- ~Vinod -- Get a FREE DOWNLOAD! and learn more about uberSVN rich system, user administration capabilities and model configuration. Take the hassle out of deploying and managing Subversion and the tools developers use with it. http://p.sf.net/sfu/wandisco-d2d-2 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Thu, 2011-08-11 at 00:29 +0530, Jassi Brar wrote: On Thu, Aug 11, 2011 at 2:28 AM, Vinod Koul vk...@infradead.org wrote: The lookup of a device DMA channel should follow the design pattern set by regulators and clocks. Nopes. It depends upon the subsystem. We should strive towards making this scheme as 'standalone' as possible. Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection! On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3 can only use channels from controller 1, and the peripherals 4, 5 from controller 2 and so on. They _absolutely_ need channel from specific controller, so above suits it :). Btw all three controllers _have_exactly_same_caps_ so dma engine will not see any difference in channels. They don't know which peripherals are connected to them, that's platform information which this scheme seems to be suited to... Can you tell me how your scheme will work in this case? Actually the setup is quite simple. Say, the Peri-4 is the UART which could only be reached from DMAC-2. DMAENGINE should simply manage a pool of total channels in the platform, disregarding the dmacs they come from. Each channel in the common pool will have a 'capability-tag' of u32 type - which specifies various capabilities of the channel expressed in bit-fields. It is the job of platform (via dmac driver) to set 'tag' of each channel appropriately. Among others, a channel's reach to a particular device is a 'capability' (expressed in 7-bits DEV_TYPE field-mask of the 'tag') In your case, while setting up channels before adding them to the common pool, the platform (via dmac driver) will ensure exactly the channel which can reach UART has set DEV_UART value in the DEV_TYPE field of its capability-tag. After all the dmac instances have been probed, _exactly_ one channel in the pool will have the 'UART' capability mentioned in the DEV_TYPE field. The serial driver(client), will simply specify 'Ability to reach UART' as one of its requirements to the core - by setting the DEV_TYPE field with DEV_UART value of the 'request-tag' (a u32 number, say). While searching for appropriate channel for the serial client, the core will iterate over the pool and, of course, only one channel will have DEV_TYPE field set to DEV_UART - the one which platform decided! Please have a look at the end of https://lkml.org/lkml/2011/7/29/211 (esp the helpers) to get an idea of data structures involved. *** UART(client) driver snippet :- *** struct dma_chan *chan_rx; u32 req_cap; /* Reset the requirement list */ DCH_RESET_CAP(req_cap); /* Specify ability to reach UART as a requirement */ DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART); /* Specify we require to Read from UART */ DCH_REQCAP_P2M(req_cap); /* .. specify other requirements */ /* Ask for the channel */ chan_rx = dma_request_channel(req_cap); *** dmaengine.c *** struct dma_chan *dma_request_channel(u32 req_cap) // typedef u32 to look nice ;) { struct dma_chan *ch; /* !!! Not final implementation !!! */ list_for_each_entry(ch, channel_pool, chan_node) { if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch-tag)) continue; if ((IS_DCH_M2M(req_cap) !IS_DCH_M2M(ch-tag)) continue; if ((IS_DCH_M2P(req_cap) !IS_DCH_M2P(ch-tag)) continue; if ((IS_DCH_P2M(req_cap) !IS_DCH_P2M(ch-tag)) continue; if ((IS_DCH_P2P(req_cap) !IS_DCH_P2P(ch-tag)) continue; /* weed out channels that don't have further capabilities required */ /* At the end of checks this channel meets every requirement */ return ch; } /* Will never reach here in a bug-free platform */ return NULL; } ** DMAC - PLATFORM - BOARD *** Well, that is between dmac and platform and out of scope of DMAENGINE API. They can decide upon any data structure to pass the info needed. But at some point someone must set :- _Only_ for the channel of DMAC-2 that could talk to the UART. That is why I am skeptical about this implementation approach. Here CHAN_TODEV_UART is how peripheral is connected to DMAC which is a platform capability which magically needs to be published by generic DMAC driver and requested by UART driver... Sorry I still don't get this schema and how it can be scaled and be generic enough to let it carry with various implementations. Can you publish your complete idea rather than bits and pieces... -- ~Vinod -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
2011/8/10 Jassi Brar jassisinghb...@gmail.com: On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij linus.wall...@linaro.org wrote: Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record. It would *work* but the current proposal is *not elegant* IMO. would *work* - You could find no case that the scheme wouldn't support. Well there is the usecase where we have a lot of devices, but it is true we don't have that kind of hardware around. *not elegant* - Your opinion. Let us see. Well, yeah, such is life. Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection! I think I saw you proposal define REQ(MMC,2) for example, isn't that specifying the device? rxc = dma_request_slave_channel(dev, MMC-RX); txc = dma_request_slave_channel(dev, MMC-TX); Absolutely not-very-good ! We can do without the 'dev' argument. You still need to pass in something referring you back to the device. How does a client request duplex channel ? Currently the duplex channels using DMA-engine switch direction of the channel at runtime with the config call.0 There is a bidirectional define in dmaengine.h but I haven't figured out if that is useful for slave transfers at all really. Do you propose to implement a string parser in the core ?! Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree. I also recognized that you needed a priority scheme and a few other bits for the above, You didn't recognize. I told you. IIRC you suggested I should actually sell that point! http://en.wiktionary.org/wiki/recognise 1. (transitive) To match something or someone which one currently perceives to a memory of some previous encounter with the same entity. 2. (transitive) To acknowledge the existence or legality of something.; treat as worthy of consideration or valid. The US and a number of EU countries are expected to recognise Kosovo on Monday. 3. (transitive) To acknowledge or consider as something. 4. (transitive) To realise or discover the nature of something; apprehend quality in; realise or admit that. 5. (transitive) To give an award. Sorry for spelling with z. If the majority is happy with this scheme I can even try implementing it. Interesting! Only a few days ago you were seeing eternal sunshine in filter-functions... and now you plan to implement my proposal by replacing bit-fields with strings. Yes your argument for a better channel-lookup functionality are perfectly valid, that is what I mean when I say I recognize it. We only disagree on how to implement it. As for eternal sunshine, well, it rains here in Sweden today so the kernel is a pretty sunny place comparatively. Thanks, Linus Walleij -- Get a FREE DOWNLOAD! and learn more about uberSVN rich system, user administration capabilities and model configuration. Take the hassle out of deploying and managing Subversion and the tools developers use with it. http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Thu, Aug 11, 2011 at 6:25 PM, Linus Walleij linus.wall...@linaro.org wrote: 2011/8/10 Jassi Brar jassisinghb...@gmail.com: On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij linus.wall...@linaro.org wrote: Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record. It would *work* but the current proposal is *not elegant* IMO. would *work* - You could find no case that the scheme wouldn't support. Well there is the usecase where we have a lot of devices, but it is true we don't have that kind of hardware around. I am pissed off by Russell's allegation that { Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. } whereas what you say now, and when we discussed, does not repeat a single concern of his! You never say/said what 'problem' do you see. Russell has been skeptical if my idea would work at all for his versatile setups. While you say it would *work* but you just want the idea implemented using device pointer and strings ! Btw, do bring on any esoteric setup that you have in mind even if you never expect to have it in real. I am not married to my idea. I don't wanna push any further if it wouldn't work. The biggest challenge I see is the huge modification needed. Not some weird setup ! Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection! I think I saw you proposal define REQ(MMC,2) for example, isn't that specifying the device? Not using platform provided device pointers, but by using globally defined values. (See my last reply to Vinod's setup http://lists.infradead.org/pipermail/linux-arm-kernel/2011-August/060860.html) The notion of must-use device pointer sucks! More so when we can have simpler and at least as good implementation without using them ! Do you have any reason for using device pointer and strings, other than just because clock and regulator use them ?? rxc = dma_request_slave_channel(dev, MMC-RX); txc = dma_request_slave_channel(dev, MMC-TX); Absolutely not-very-good ! We can do without the 'dev' argument. You still need to pass in something referring you back to the device. Nopes. As I said please see my reply to Vinod's setup. Do you propose to implement a string parser in the core ?! Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree. Dude, I have utter disrespect for using strings in a case such as expressing requirements. I have already explained how we can easily and in a _better_ way do without them (again see my last reply to Vindo's setup). Tell me 1 reason why using strings, in this case, would be better ? -- Get a FREE DOWNLOAD! and learn more about uberSVN rich system, user administration capabilities and model configuration. Take the hassle out of deploying and managing Subversion and the tools developers use with it. http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Thu, 2011-08-11 at 16:48 +0200, Linus Walleij wrote: 2011/8/11 Jassi Brar jassisinghb...@gmail.com: Do you have any reason for using device pointer and strings, other than just because clock and regulator use them ?? Basically no. But I think these frameworks are very workable and proven to work in practice. So I like them. When setting up the platform the coder would to be aware that there are totally orthogonal concepts, which IMO makes things complicated. Do you propose to implement a string parser in the core ?! Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree. Dude, I have utter disrespect for using strings in a case such as expressing requirements. It's mainly a strcmp(), and it's only comparing to the well-established namespace that all devices have anyway, due to the way the device model is done. Basically when binding clocks or regulators it's: struct device *dev; strcmp(map_string, dev_name(dev)); By this time struct device exist of course, since it's the device driver calling to get its clock/regulator. dev_name() comes from linux/device.h and basically takes the kobject name or an optional initilizer name for the device. So the names are pretty static, you don't need to parse them, just compare. I have already explained how we can easily and in a _better_ way do without them (again see my last reply to Vindo's setup). Tell me 1 reason why using strings, in this case, would be better ? I have no other reasons than the above. People like Russell (clkdevice) and Liam Girdwood (regulator) who I know are smarter than me and have worked with these subsystems for years choose that model, so I trust their judgement. I would agree the overhead of using strings is negligible if you compare the benefits it gives in usability and being easy for someone to configure. Jassi being samsung asoc maintainer should already agree to that :-) -- ~Vinod -- Get a FREE DOWNLOAD! and learn more about uberSVN rich system, user administration capabilities and model configuration. Take the hassle out of deploying and managing Subversion and the tools developers use with it. http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Thu, Aug 11, 2011 at 8:18 PM, Linus Walleij linus.wall...@linaro.org wrote: 2011/8/11 Jassi Brar jassisinghb...@gmail.com: Do you have any reason for using device pointer and strings, other than just because clock and regulator use them ?? Basically no. Dear, I am speechless !! Best of luck. Do you propose to implement a string parser in the core ?! Yes, the clock and regulator framework already does that. But it is only used when you cannot pass in a struct device * directly, like from device tree. Dude, I have utter disrespect for using strings in a case such as expressing requirements. It's mainly a strcmp(), and it's only comparing to the well-established namespace that all devices have anyway, due to the way the device model is done. Basically when binding clocks or regulators it's: struct device *dev; strcmp(map_string, dev_name(dev)); By this time struct device exist of course, since it's the device driver calling to get its clock/regulator. dev_name() comes from linux/device.h and basically takes the kobject name or an optional initilizer name for the device. So the names are pretty static, you don't need to parse them, just compare. I think I know why and how clkdev and regulator use strings. Here we are talking about dmac possibly expressing 8 parameters as strings, not just 2. I have already explained how we can easily and in a _better_ way do without them (again see my last reply to Vindo's setup). Tell me 1 reason why using strings, in this case, would be better ? I have no other reasons than the above. People like Russell (clkdevice) and Liam Girdwood (regulator) who I know are smarter than me and have worked with these subsystems for years choose that model, so I trust their judgement. Let me provide you even more 'ammunition' ASoC and USB-Gadget employs strings too Though only Regulators, ASoC and CLKDEV(?) really _have-to_ employ strings. USB-Gadget use them mainly for historical reasons - we can very well replace that scheme with bit-fields as I propose. -- Get a FREE DOWNLOAD! and learn more about uberSVN rich system, user administration capabilities and model configuration. Take the hassle out of deploying and managing Subversion and the tools developers use with it. http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 02:20:59PM +0530, Viresh Kumar wrote: Currently we request DMA channels at probe time and free them at remove. They are always occupied, irrespective of their usage. They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. This actually results in better usage of the DMA controller, as the virtual channels can be assigned to physical channels dynamically according to what the physical channels are doing. Plus, actually, the idea that DMA channels should be a short-term thing is broken as soon as you start considering things like UARTs, where you need a channel tied up long-term for the receive side of things. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. @Dan/Vinod: What do you say? -- viresh -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote: On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. @Dan/Vinod: What do you say? I would agree on both counts :) In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer. But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization. So i would argue that there is no one size fits all here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so. On DMA Engine API, it doesn't force for any of the above. You are free to choose based on the usage and capability And on your patch, are you able to dynamically assign the channels for platform? What is the intended usage? (as Russell articulated it is bad to dynamically assign channel for something like uart) -- ~Vinod -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 2:59 PM, viresh kumar viresh.ku...@st.com wrote: On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. The requirement stems from the fact that most DMACs(esp third party) could be made to reroute req-signals by the platform, it has not much to do with the API. IMO, all dmac drivers should be implemented that way to be on the safer side. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On 08/10/2011 03:31 PM, Koul, Vinod wrote: And on your patch, are you able to dynamically assign the channels for platform? What is the intended usage? (as Russell articulated it is bad to dynamically assign channel for something like uart) Are you talking about channels or DMA request lines? For channels yes, we can always allocate channels as they are independent of peripherals. About request lines, they are muxed in our case between several peripherals, but support for that has to be added in dw_dmac. SPI is not as much heavily used as uart. So, it might be fine there to allocate channels dynamically. -- viresh -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 03:31:42PM +0530, Koul, Vinod wrote: I would agree on both counts :) In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer. But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization. If dw_dmac can assign channels dynamically at runtime to request signals, it is no different from pl08x, where we have essentially the same capability, and we do have the virtual channel support. The virtual channel support is far more flexible than picking a physical channel at allocation time, because it means you can reassign the virtual channel at any point when a transfer is not in progress. Plus it means that you don't have to keep doing the channel allocation, configuration and freeing on every transfer which would be hugely wasteful. Not to mention that it burdens peripheral drivers with unnecessary additional complexity - which means additional bugs. I would encourage all DMA engine drivers which have this capability to switch to a virtual channel setup to ensure maximum interoperability between different peripheral drivers. I'd also suggest that we probably want to make the virtual layer a library for DMA engine implementations to use. We really don't want every DMA engine implementation re-creating that support time and time again. I'll look into pulling the virtual channel stuff out of PL08x over the next month or so. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 03:39:28PM +0530, Jassi Brar wrote: On Wed, Aug 10, 2011 at 2:59 PM, viresh kumar viresh.ku...@st.com wrote: On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. The requirement stems from the fact that most DMACs(esp third party) could be made to reroute req-signals by the platform, it has not much to do with the API. IMO, all dmac drivers should be implemented that way to be on the safer side. No it isn't. It's to do with how the physical channels are used. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod vinod.k...@intel.com wrote: On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote: On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. @Dan/Vinod: What do you say? I would agree on both counts :) In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer. But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization. So i would argue that there is no one size fits all here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so. The idea is to have channel allocation as purely a s/w thing - no actual commitment of h/w resources. So we can afford to have channel allocated for the whole lifetime of a client. Some dmac drivers are written 'improperly', keeping in mind the platforms that have fixed ReqSig-Peri map and no more clients than actual req-sigs are active simultaneously. But such dmac drivers will fail if a new platform decides to hijack req-signals. So, imho, it is absolutely a good thing for every dmac driver to be designed for re-routable ReqSig-Peri map... which would force their design to allocate virtual/software channels to clients without commit much(any?) h/w resources. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 04:01:13PM +0530, Jassi Brar wrote: On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod vinod.k...@intel.com wrote: On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote: On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. @Dan/Vinod: What do you say? I would agree on both counts :) In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer. But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization. So i would argue that there is no one size fits all here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so. The idea is to have channel allocation as purely a s/w thing - no actual commitment of h/w resources. So we can afford to have channel allocated for the whole lifetime of a client. Some dmac drivers are written 'improperly', keeping in mind the platforms that have fixed ReqSig-Peri map and no more clients than actual req-sigs are active simultaneously. But such dmac drivers will fail if a new platform decides to hijack req-signals. So, imho, it is absolutely a good thing for every dmac driver to be designed for re-routable ReqSig-Peri map... which would force their design to allocate virtual/software channels to clients without commit much(any?) h/w resources. We discussed channel allocation at Linaro. However, I am now _really_ disappointed. I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. So that's essentially invalidated everything we discussed there as part of my thinking was if Linus is happy with it, then I am now convinced that you'll say *anything* just to get your idea into the kernel. So, the stakes have now been raised further for you: what I want to see from you is a _working_ _implementation_ for those three platforms which I outlined. I don't want more discussion. I want patches from you. Nothing else. We'll then review the code changes themselves rather than a vague idea communicated by email/verbally. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux li...@arm.linux.org.uk wrote: On Wed, Aug 10, 2011 at 04:01:13PM +0530, Jassi Brar wrote: On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod vinod.k...@intel.com wrote: On Wed, 2011-08-10 at 14:59 +0530, viresh kumar wrote: On 08/10/2011 02:30 PM, Russell King - ARM Linux wrote: They must be allocated when they are required and must be freed after we are done with transfers. So that they can be used by other users. Which DMA engine driver requires this? dw_dmac.c Normally, when we have DMA engine drivers with multiple request signals, the slave peripheral side publishes several virtual channels which are claimed by the peripheral drivers. This (amongst other things) allows the peripheral drivers to hold claim to one of the virtual channels all the time that it's required. If users of DMA expect DMA engine drivers to work this way, then we should have this mentioned clearly in DMA slave documentation. @Dan/Vinod: What do you say? I would agree on both counts :) In some cases it does make sense to hold the channel for the lifetime like uart or where the channel has been tied to an interface by SoC designer. But in some cases like dw_dmac it seems you can assign channels dynamically to each usage, and runtime allocation ensures we have best utilization. So i would argue that there is no one size fits all here, if you can manage channels dynamically and utilize more efficiently then go ahead, but if you cant (h/w and usage constraint) then you should not be forced to do so. The idea is to have channel allocation as purely a s/w thing - no actual commitment of h/w resources. So we can afford to have channel allocated for the whole lifetime of a client. Some dmac drivers are written 'improperly', keeping in mind the platforms that have fixed ReqSig-Peri map and no more clients than actual req-sigs are active simultaneously. But such dmac drivers will fail if a new platform decides to hijack req-signals. So, imho, it is absolutely a good thing for every dmac driver to be designed for re-routable ReqSig-Peri map... which would force their design to allocate virtual/software channels to clients without commit much(any?) h/w resources. We discussed channel allocation at Linaro. However, I am now _really_ disappointed. Honestly, I don't see how this deviates from my proposal and why you think a dmac driver designed for fixed ReqSig-Peri map is future-proof (my that very assertion made you really disappointed)! All the PL080 platforms that I have worked, had a fixed map...and I am sure you wouldn't have been happy looking at a dmac driver based upon that assumption. Similarly for any other dmac driver developer, it makes sense to be safe by assuming the dmac could have flexible map on some platform. And this precisely why I said { So, imho, it is absolutely a good thing for every dmac driver to be designed for re-routable ReqSig-Peri map... which would force their design to allocate virtual/software channels to clients without commit much(any?) h/w resources. } I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. So that's essentially invalidated everything we discussed there as part of my thinking was if Linus is happy with it, then IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details. Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar jassisinghb...@gmail.com wrote: On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. So that's essentially invalidated everything we discussed there as part of my thinking was if Linus is happy with it, then IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details. Bah now we have two Linus oracles in this world instead of just one... haha :-) Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record. It would *work* but the current proposal is *not elegant* IMO. Let me put it like this: Yes, there is a problem with how all platforms have to pass in a filter function and some opaque cookie for every driver to look up its DMA channel. You seem to want to address this problem, which is good. I think your scheme passing in a device ID and instance tuple e.g. REQ(MMC,2) would *work*, but I don't think it is a proper solution and I would never ACK it, as I said. The lookup of a device DMA channel should follow the design pattern set by regulators and clocks. Which means you use struct device * or alternatively a string (if the instance is not available) to look it up, such that: struct dma_slave_map { char name[16]; struct device *dev; char dev_name[16]; struct devive *dma_dev; char dma_dev_name[16]; }; struct dma_slave_map[] = { { .name = MMC-RX, .dev_name = mmc.0, .dma_dev_name = pl08x.0, }, { .name = MMC-TX, .dev_name = mmc.0, .dma_dev_name = pl08x.0, }, { .name = SSP-RX, .dev_name = pl022.0, .dma_dev_name = pl08x.1, }, ... }; The dmaengine core should take this mapping, and the device driver would only have to: struct dma_chan *rxc; struct dma_chan *txc; struct device *dev; rxc = dma_request_slave_channel(dev, MMC-RX); txc = dma_request_slave_channel(dev, MMC-TX); I also recognized that you needed a priority scheme and a few other bits for the above, so struct dma_slave_map may need a few more members, but the above would sure solve all usecases we have today. mem2mem could use the old request function and doesn't need anything like this. Pros: well established design pattern, channels tied to devices, intuitive for anyone who used clock or regulator frameworks. If the majority is happy with this scheme I can even try implementing it. Yours, Linus Walleij -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij linus.wall...@linaro.org wrote: On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar jassisinghb...@gmail.com wrote: On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. So that's essentially invalidated everything we discussed there as part of my thinking was if Linus is happy with it, then IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details. Bah now we have two Linus oracles in this world instead of just one... haha :-) Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record. It would *work* but the current proposal is *not elegant* IMO. would *work* - You could find no case that the scheme wouldn't support. *not elegant* - Your opinion. Let us see. I think your scheme passing in a device ID and instance tuple e.g. REQ(MMC,2) would *work*, but I don't think it is a proper solution and I would never ACK it, as I said. As much as I would feel 'deprived' of your ACK, I am not yet changing the implementation for that part. The lookup of a device DMA channel should follow the design pattern set by regulators and clocks. Nopes. It depends upon the subsystem. We should strive towards making this scheme as 'standalone' as possible. Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection! Also that way, a client is actually asking for a 'channel' rather than only specifying its requirements to the API and the API has enough information to return the appropriate channel(which is what I propose). Which means you use struct device * or alternatively a string (if the instance is not available) to look it up, such that: struct dma_slave_map { char name[16]; struct device *dev; char dev_name[16]; struct devive *dma_dev; char dma_dev_name[16]; }; struct dma_slave_map[] = { { .name = MMC-RX, .dev_name = mmc.0, .dma_dev_name = pl08x.0, }, { .name = MMC-TX, .dev_name = mmc.0, .dma_dev_name = pl08x.0, }, { .name = SSP-RX, .dev_name = pl022.0, .dma_dev_name = pl08x.1, }, 1) This is implementation detail. 2) Not a very good one at that. a) Just think how many bytes you take to specify a channel ? Mine takes less than 4 bytes for equivalent information. b) Think about the mess of string copy, compare etc, when we can simply extract and manipulate bit-fields from, say, u32? The dmaengine core should take this mapping, and the device driver would only have to: struct dma_chan *rxc; struct dma_chan *txc; struct device *dev; rxc = dma_request_slave_channel(dev, MMC-RX); txc = dma_request_slave_channel(dev, MMC-TX); Absolutely not-very-good ! We can do without the 'dev' argument. How does a client request duplex channel ? MMC-RXTX or MMC-TXRX or TXRX-MMC ? Do you propose to implement a string parser in the core ?! Not to mention how limited requirements this scheme could express to the core. I also recognized that you needed a priority scheme and a few other bits for the above, You didn't recognize. I told you. IIRC you suggested I should actually sell that point! The priority is an additional feature that easily helps a situation when a peri could be reached from two different dmacs and the board can prioritize an already busy dmac over the one that would only serve this peripheral - to save power by keeping the idle dmac off. Your string manipulation would only get messier if it had to support that feature. If the majority is happy with this scheme I can even try implementing it. Interesting! Only a few days ago you were seeing eternal sunshine in filter-functions... and now you plan to implement my proposal by replacing bit-fields with strings. Frankly, I don't care much who, if anybody, implements it anymore. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it. Learn more about uberSVN and get a free download at: http://p.sf.net/sfu/wandisco-dev2dev ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Wed, 2011-08-10 at 18:46 +0530, Jassi Brar wrote: On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij linus.wall...@linaro.org wrote: On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar jassisinghb...@gmail.com wrote: On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux I discussed this with Linus on the bus back from Cambridge in the evening, and it appears that the story you gave me was inaccurate - Linus had not agreed to your proposal and saw more or less the same problems with it which I've been on at you about via your other email alias/lkml. So that's essentially invalidated everything we discussed there as part of my thinking was if Linus is happy with it, then IIRC, Linus W mainly opined to involve device pointer during channel selection. Other than that I thought there was only ambiguity about implementation details. Bah now we have two Linus oracles in this world instead of just one... haha :-) Linus W, was there anything you said wouldn't work with the scheme ? Please tell now on the record. It would *work* but the current proposal is *not elegant* IMO. would *work* - You could find no case that the scheme wouldn't support. *not elegant* - Your opinion. Let us see. I think your scheme passing in a device ID and instance tuple e.g. REQ(MMC,2) would *work*, but I don't think it is a proper solution and I would never ACK it, as I said. As much as I would feel 'deprived' of your ACK, I am not yet changing the implementation for that part. The lookup of a device DMA channel should follow the design pattern set by regulators and clocks. Nopes. It depends upon the subsystem. We should strive towards making this scheme as 'standalone' as possible. Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection! On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3 can only use channels from controller 1, and the peripherals 4, 5 from controller 2 and so on. They _absolutely_ need channel from specific controller, so above suits it :). Btw all three controllers _have_exactly_same_caps_ so dma engine will not see any difference in channels. They don't know which peripherals are connected to them, that's platform information which this scheme seems to be suited to... Can you tell me how your scheme will work in this case? Also that way, a client is actually asking for a 'channel' rather than only specifying its requirements to the API and the API has enough information to return the appropriate channel(which is what I propose). Which means you use struct device * or alternatively a string (if the instance is not available) to look it up, such that: struct dma_slave_map { char name[16]; struct device *dev; char dev_name[16]; struct devive *dma_dev; char dma_dev_name[16]; }; struct dma_slave_map[] = { { .name = MMC-RX, .dev_name = mmc.0, .dma_dev_name = pl08x.0, }, { .name = MMC-TX, .dev_name = mmc.0, .dma_dev_name = pl08x.0, }, { .name = SSP-RX, .dev_name = pl022.0, .dma_dev_name = pl08x.1, }, 1) This is implementation detail. 2) Not a very good one at that. a) Just think how many bytes you take to specify a channel ? Mine takes less than 4 bytes for equivalent information. b) Think about the mess of string copy, compare etc, when we can simply extract and manipulate bit-fields from, say, u32? The dmaengine core should take this mapping, and the device driver would only have to: struct dma_chan *rxc; struct dma_chan *txc; struct device *dev; rxc = dma_request_slave_channel(dev, MMC-RX); txc = dma_request_slave_channel(dev, MMC-TX); Absolutely not-very-good ! We can do without the 'dev' argument. How does a client request duplex channel ? MMC-RXTX or MMC-TXRX or TXRX-MMC ? Do you propose to implement a string parser in the core ?! Not to mention how limited requirements this scheme could express to the core. I also recognized that you needed a priority scheme and a few other bits for the above, You didn't recognize. I told you. IIRC you suggested I should actually sell that point! The priority is an additional feature that easily helps a situation when a peri could be reached from two different dmacs and the board can prioritize an already busy dmac over the one that would only serve this peripheral - to save power by keeping the idle dmac off. Your string manipulation would only get messier if it had to support that feature. If the majority is happy with this scheme I can even try implementing it. Interesting! Only a few days ago you were seeing eternal sunshine in filter-functions... and now you plan to implement my proposal by replacing bit-fields with strings. Frankly, I don't care much who, if
Re: [PATCH V2 6/6] spi/spi-pl022: Request/free DMA channels as and when required.
On Thu, Aug 11, 2011 at 2:28 AM, Vinod Koul vk...@infradead.org wrote: The lookup of a device DMA channel should follow the design pattern set by regulators and clocks. Nopes. It depends upon the subsystem. We should strive towards making this scheme as 'standalone' as possible. Client having to specify the device it wants a channel for, is a waste - otherwise we don't fully get rid of platform assistance for channel selection! On one of the platforms I work on we have 3 DMACs, peripheral 1, 2 and 3 can only use channels from controller 1, and the peripherals 4, 5 from controller 2 and so on. They _absolutely_ need channel from specific controller, so above suits it :). Btw all three controllers _have_exactly_same_caps_ so dma engine will not see any difference in channels. They don't know which peripherals are connected to them, that's platform information which this scheme seems to be suited to... Can you tell me how your scheme will work in this case? Actually the setup is quite simple. Say, the Peri-4 is the UART which could only be reached from DMAC-2. DMAENGINE should simply manage a pool of total channels in the platform, disregarding the dmacs they come from. Each channel in the common pool will have a 'capability-tag' of u32 type - which specifies various capabilities of the channel expressed in bit-fields. It is the job of platform (via dmac driver) to set 'tag' of each channel appropriately. Among others, a channel's reach to a particular device is a 'capability' (expressed in 7-bits DEV_TYPE field-mask of the 'tag') In your case, while setting up channels before adding them to the common pool, the platform (via dmac driver) will ensure exactly the channel which can reach UART has set DEV_UART value in the DEV_TYPE field of its capability-tag. After all the dmac instances have been probed, _exactly_ one channel in the pool will have the 'UART' capability mentioned in the DEV_TYPE field. The serial driver(client), will simply specify 'Ability to reach UART' as one of its requirements to the core - by setting the DEV_TYPE field with DEV_UART value of the 'request-tag' (a u32 number, say). While searching for appropriate channel for the serial client, the core will iterate over the pool and, of course, only one channel will have DEV_TYPE field set to DEV_UART - the one which platform decided! Please have a look at the end of https://lkml.org/lkml/2011/7/29/211 (esp the helpers) to get an idea of data structures involved. *** UART(client) driver snippet :- *** struct dma_chan *chan_rx; u32 req_cap; /* Reset the requirement list */ DCH_RESET_CAP(req_cap); /* Specify ability to reach UART as a requirement */ DCH_REQCAP_DEV(req_cap, DCHAN_TODEV_UART); /* Specify we require to Read from UART */ DCH_REQCAP_P2M(req_cap); /* .. specify other requirements */ /* Ask for the channel */ chan_rx = dma_request_channel(req_cap); *** dmaengine.c *** struct dma_chan *dma_request_channel(u32 req_cap) // typedef u32 to look nice ;) { struct dma_chan *ch; /* !!! Not final implementation !!! */ list_for_each_entry(ch, channel_pool, chan_node) { if (GET_DEVTYPE(req_cap) != GET_DEVTYPE(ch-tag)) continue; if ((IS_DCH_M2M(req_cap) !IS_DCH_M2M(ch-tag)) continue; if ((IS_DCH_M2P(req_cap) !IS_DCH_M2P(ch-tag)) continue; if ((IS_DCH_P2M(req_cap) !IS_DCH_P2M(ch-tag)) continue; if ((IS_DCH_P2P(req_cap) !IS_DCH_P2P(ch-tag)) continue; /* weed out channels that don't have further capabilities required */ /* At the end of checks this channel meets every requirement */ return ch; } /* Will never reach here in a bug-free platform */ return NULL; } ** DMAC - PLATFORM - BOARD *** Well, that is between dmac and platform and out of scope of DMAENGINE API. They can decide upon any data structure to pass the info needed. But at some point someone must set :- _Only_ for the channel of DMAC-2 that could talk to the UART. { DCH_RESET_CAP(ch-tag); /* Declare 'capability' to talk to UART */ SET_DEVTYPE(ch-tag, DCHAN_TODEV_UART); /* Declare 'capability' to do RX i.e, P-M */ SET_DCHDIR(ch-tag, 0, 0, 1, 0); /* Declare other 'capabilities' of the channel */ /* Add enough dmac's private data to channel so as to be able to later figure out the ReqSig, DMAC etc associated with it */ /* Add the channel to the global pool in dmaengine.c */ } Please have a look at the end of https://lkml.org/lkml/2011/7/29/211 (esp the helpers) to get an idea of data structures involved. -- uberSVN's rich system and user administration capabilities and model configuration take the hassle out of deploying and managing Subversion and the tools developers use with it.