| Message ID | 566c0525199f498f04422d4c3b2ddd7466648c20.1312965742.git.viresh.kumar@st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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?
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)
On Wed, Aug 10, 2011 at 2:59 PM, viresh kumar <viresh.kumar@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.
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.
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.
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.kumar@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.
On Wed, Aug 10, 2011 at 3:31 PM, Koul, Vinod <vinod.koul@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.
On Wed, Aug 10, 2011 at 03:44:13PM +0530, viresh kumar wrote: > 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. Right, and when you do, you'll probably have to go to a virtual channel implementation, which solves the problem of keeping a channel allocated and makes this patch redundant. I assert that any DMA engine implementation where request signals can be assigned dynamically to DMA channels should be using a virtual channel implementation.
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.koul@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.
On Wed, Aug 10, 2011 at 4:00 PM, Russell King - ARM Linux <linux@arm.linux.org.uk> wrote: > 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.kumar@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. IMO, a dmac driver developer sees only two aspects - "virtual channel" at frontend and "ReqSig/PhyChan" management at the backend. While ReqSig and Physical Channels are different h/w entities, the driver developer usually works having tied them together. For ex, PL330 has 8 physical channels(ARM calls them threads) and 32 peripheral interfaces(ReqSig), where as the dmac driver freely allocates 32 virtual channels and keeps in mind that only 8 peripheral interfaces can be active at any time. So I am unable to see how I said something different that you do.
On Wed, Aug 10, 2011 at 4:10 PM, Russell King - ARM Linux <linux@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.koul@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.
On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar <jassisinghbrar@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
On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar <jassisinghbrar@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.
On Wed, 2011-08-10 at 11:32 +0100, Russell King - ARM Linux wrote: > On Wed, Aug 10, 2011 at 03:44:13PM +0530, viresh kumar wrote: > > 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. > > Right, and when you do, you'll probably have to go to a virtual channel > implementation, which solves the problem of keeping a channel allocated > and makes this patch redundant. > > I assert that any DMA engine implementation where request signals can > be assigned dynamically to DMA channels should be using a virtual channel > implementation. Agreed, virtual channels can ensure that channels can be shared dynamically. If h/w has capability it should be able to do a spi transfer followed by emmc transfer ans so forth... Current model of giving client exclusive access to a channel doesn't allow this
On Thu, Aug 11, 2011 at 2:28 AM, Vinod Koul <vkoul@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.
On Wed, 2011-08-10 at 18:46 +0530, Jassi Brar wrote: > On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Wed, Aug 10, 2011 at 1:24 PM, Jassi Brar <jassisinghbrar@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 anybody, implements it anymore. > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
2011/8/10 Jassi Brar <jassisinghbrar@gmail.com>: > On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@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
On Thu, Aug 11, 2011 at 6:25 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > 2011/8/10 Jassi Brar <jassisinghbrar@gmail.com>: >> On Wed, Aug 10, 2011 at 5:24 PM, Linus Walleij <linus.walleij@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 ?
2011/8/11 Jassi Brar <jassisinghbrar@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. Thanks, Linus Walleij
On Thu, Aug 11, 2011 at 8:18 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > 2011/8/11 Jassi Brar <jassisinghbrar@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.
On Thu, 2011-08-11 at 16:48 +0200, Linus Walleij wrote: > 2011/8/11 Jassi Brar <jassisinghbrar@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 :-)
On Thu, 2011-08-11 at 00:29 +0530, Jassi Brar wrote: > On Thu, Aug 11, 2011 at 2:28 AM, Vinod Koul <vkoul@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...
On Tue, Aug 16, 2011 at 5:25 PM, Koul, Vinod <vinod.koul@intel.com> 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... Not really. Set by generic dmac driver in conjunction with platform (or maybe also board).... which would look at the type of controller it has and the CHAN_TODEV_ its client driver requests. S3C DMA API has been doing similar, albeit for fixed ReqSig->Peri map, for quite some time now. > 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 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.
On Tue, 2011-08-16 at 20:21 +0530, Jassi Brar wrote: > On Tue, Aug 16, 2011 at 5:25 PM, Koul, Vinod <vinod.koul@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.
diff --git a/drivers/spi/spi-pl022.c b/drivers/spi/spi-pl022.c index 01e84e3..a596b96 100644 --- a/drivers/spi/spi-pl022.c +++ b/drivers/spi/spi-pl022.c @@ -389,6 +389,7 @@ struct pl022 { struct sg_table sgt_rx; struct sg_table sgt_tx; char *dummypage; + dma_cap_mask_t mask; #endif }; @@ -1093,16 +1094,33 @@ err_alloc_rx_sg: static int __init pl022_dma_probe(struct pl022 *pl022) { - dma_cap_mask_t mask; + /* set dma mask */ + dma_cap_zero(pl022->mask); + dma_cap_set(DMA_SLAVE, pl022->mask); - /* Try to acquire a generic DMA engine slave channel */ - dma_cap_zero(mask); - dma_cap_set(DMA_SLAVE, mask); + pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); + if (!pl022->dummypage) { + dev_err(&pl022->adev->dev, + "Failed to work in dma mode, work without dma!\n"); + return -ENODEV; + } + + return 0; +} + +static int pl022_alloc_dmachan(struct pl022 *pl022) +{ /* - * We need both RX and TX channels to do DMA, else do none - * of them. + * Both channels should be allocated or not allocated. It is wrong if + * only one allocated */ - pl022->dma_rx_channel = dma_request_channel(mask, + if (pl022->dma_rx_channel && pl022->dma_tx_channel) + return 0; + + BUG_ON(pl022->dma_rx_channel || pl022->dma_tx_channel); + + /* We need both RX and TX channels to do DMA, else do none of them. */ + pl022->dma_rx_channel = dma_request_channel(pl022->mask, pl022->master_info->dma_filter, pl022->master_info->dma_rx_param); if (!pl022->dma_rx_channel) { @@ -1110,7 +1128,7 @@ static int __init pl022_dma_probe(struct pl022 *pl022) goto err_no_rxchan; } - pl022->dma_tx_channel = dma_request_channel(mask, + pl022->dma_tx_channel = dma_request_channel(pl022->mask, pl022->master_info->dma_filter, pl022->master_info->dma_tx_param); if (!pl022->dma_tx_channel) { @@ -1118,20 +1136,12 @@ static int __init pl022_dma_probe(struct pl022 *pl022) goto err_no_txchan; } - pl022->dummypage = kmalloc(PAGE_SIZE, GFP_KERNEL); - if (!pl022->dummypage) { - dev_dbg(&pl022->adev->dev, "no DMA dummypage!\n"); - goto err_no_dummypage; - } - - dev_info(&pl022->adev->dev, "setup for DMA on RX %s, TX %s\n", + dev_dbg(&pl022->adev->dev, "setup for DMA on RX %s, TX %s\n", dma_chan_name(pl022->dma_rx_channel), dma_chan_name(pl022->dma_tx_channel)); return 0; -err_no_dummypage: - dma_release_channel(pl022->dma_tx_channel); err_no_txchan: dma_release_channel(pl022->dma_rx_channel); pl022->dma_rx_channel = NULL; @@ -1143,22 +1153,30 @@ err_no_rxchan: static void terminate_dma(struct pl022 *pl022) { - struct dma_chan *rxchan = pl022->dma_rx_channel; - struct dma_chan *txchan = pl022->dma_tx_channel; - - dmaengine_terminate_all(rxchan); - dmaengine_terminate_all(txchan); - unmap_free_dma_scatter(pl022); + if (pl022->dma_rx_channel) + dmaengine_terminate_all(pl022->dma_rx_channel); + if (pl022->dma_tx_channel) + dmaengine_terminate_all(pl022->dma_tx_channel); } -static void pl022_dma_remove(struct pl022 *pl022) +static void pl022_free_dmachan(struct pl022 *pl022) { - if (pl022->busy) - terminate_dma(pl022); if (pl022->dma_tx_channel) dma_release_channel(pl022->dma_tx_channel); if (pl022->dma_rx_channel) dma_release_channel(pl022->dma_rx_channel); + + pl022->dma_rx_channel = pl022->dma_tx_channel = NULL; +} + +static void pl022_dma_remove(struct pl022 *pl022) +{ + if (pl022->busy) { + terminate_dma(pl022); + unmap_free_dma_scatter(pl022); + } + + pl022_free_dmachan(pl022); kfree(pl022->dummypage); } @@ -1176,6 +1194,19 @@ static inline int pl022_dma_probe(struct pl022 *pl022) static inline void pl022_dma_remove(struct pl022 *pl022) { } + +static inline int pl022_alloc_dmachan(struct pl022 *pl022) +{ + return 0; +} + +static inline void terminate_dma(struct pl022 *pl022) +{ +} + +static void pl022_free_dmachan(struct pl022 *pl022) +{ +} #endif /** @@ -1401,16 +1432,20 @@ static void do_interrupt_dma_transfer(struct pl022 *pl022) } /* If we're using DMA, set up DMA here */ if (pl022->cur_chip->enable_dma) { + int status = pl022_alloc_dmachan(pl022); + if (status) + goto switch_to_irq_mode; + /* Configure DMA transfer */ if (configure_dma(pl022)) { dev_dbg(&pl022->adev->dev, "configuration of DMA failed, fall back to interrupt mode\n"); - goto err_config_dma; + goto switch_to_irq_mode; } /* Disable interrupts in DMA mode, IRQ from DMA controller */ irqflags = DISABLE_ALL_INTERRUPTS; } -err_config_dma: +switch_to_irq_mode: /* Enable SSP, turn on interrupts */ writew((readw(SSP_CR1(pl022->virtbase)) | SSP_CR1_MASK_SSE), SSP_CR1(pl022->virtbase)); @@ -1513,7 +1548,14 @@ static void pump_messages(struct work_struct *work) spin_lock_irqsave(&pl022->queue_lock, flags); if (list_empty(&pl022->queue) || !pl022->running) { pl022->busy = false; + spin_unlock_irqrestore(&pl022->queue_lock, flags); + + /* free dma channels */ + if (pl022->master_info->enable_dma) { + terminate_dma(pl022); + pl022_free_dmachan(pl022); + } return; } /* Make sure we are not already running a message */