diff mbox

[alsa-devel,2/3] ASoC: Davinci: pcm: add support for sram-support-less platforms

Message ID 506D552A.2000506@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Daniel Mack Oct. 4, 2012, 9:21 a.m. UTC
On 02.10.2012 18:50, Daniel Mack wrote:
> On 02.10.2012 18:41, Matt Porter wrote:
>> On Tue, Oct 02, 2012 at 03:42:47PM +0200, Daniel Mack wrote:
>>> On 02.10.2012 13:06, Sekhar Nori wrote:
>>>> On 10/2/2012 4:03 PM, Daniel Mack wrote:
>>>>> On 02.10.2012 11:37, Mark Brown wrote:
>>>>>> On Tue, Oct 02, 2012 at 10:48:53AM +0300, Peter Ujfalusi wrote:
>>>>>>
>>>>>>> I also agree that ifdef is not a good solution.
>>>>>>> It is better to have this information passed as device_data and via DT it can
>>>>>>> be decided based on the compatible property for the device.
>>>>>>
>>>>>> That's not really the problem here - the problem is that the APIs used
>>>>>> to get the SRAM are DaVinci only so it's not possible to build on OMAP
>>>>>> or other platforms.  The SRAM code needs to move to a standard API.
>>>>>
>>>>> What about following Matt Porter's idea and ignore the SRAM code
>>>>> entirely and port the entire PCM code to generic dmaengine code first?
>>>>> The EDMA driver needs to learn support for cyclic DMA for that, and I
>>>>> might give that a try in near future.
>>>>>
>>>>> Later on, the SRAM ping-pong code can get added back using genalloc
>>>>> functions, as Sekhar proposed. That needs to be done by someone who has
>>>>> access to a Davinci board though, I only have a AM33xx/OMAP here.
>>>>
>>>> We cannot "get rid" of SRAM code and add it back "later". It is required
>>>> for most DaVinci parts. The SRAM code can be converted to use genalloc
>>>> (conversion should be straightforward and I can help test it) and the
>>>> code that uses SRAM can probably keep using the private EDMA API till
>>>> the dmaengine EDMA driver has cyclic DMA support. Matt has already
>>>> posted patches to move private EDMA APIs to a common location. That
>>>> should keep AM335x build from breaking. Is this something that is feasible?
>>>
>>> Yes - by "later" I just meant in a subsequent patch. But you're probably
>>> right, we can also do that first.
>>>
>>> I'm looking at that right now and the problem seems that we don't have a
>>> sane way to dynamically look up gen_pools independently of the actual
>>> run-time platform. All users of gen_pools seem to know which platform
>>> they run on and access a platform-specific pool. So I don't currently
>>> see how we could implement multi-platform code, gen_pools are fine but
>>> don't solve the problem here.
>>>
>>> Would it be an idea to add a char* member to gen_pools and a function
>>> that can be used to dynamically look it up again? If a buffer with a
>>> certain name exists, we can use it and install that ping-pong buffer,
>>> otherwise we just don't. While that would be easy to do, it's a
>>> tree-wide change.
>>>
>>> Is there a better way that I miss?
>>
>> At the high level there's two platform models we have to handle, the
>> boot from board file !DT case, and then the boot from DT case. Since
>> Davinci is just starting DT conversion, we mostly care about the !DT
>> base in which the struct gen_pool * is passed in pdata to the ASoC
>> driver. It is then selectable on a per-platform basis where the decision
>> should be made.
>>
>> Given a separate discussion with Sekhar, we're only going to have one
>> SRAM pool on any DaVinci part right now...this was only a question on
>> L138 anyway. But regardless, the created gen_pool will be passed via
>> pdata.
> 
> I thought about this too, as mmp does it that way.
> 
>> Since DT conversion is starting and we need to consider that now,
>> the idea there is to use the DT-based generic sram driver [1] such that
>> when we do boot from DT on Davinci, the genpool is provided via phandle
>> and the pointer extracted with the OF helpers that are part of the
>> series.
> 
> A phandle is the cleanest way I think, yes.
> 
>> That's pretty much it. I'm reworking the backend support as discussed
>> with Sekhar wrt to my uio_pruss series. I can post a standalone series
>> that just replaces sram_* with genalloc for davinci ASoC.
> 
> As you can also test it, it would be easiest if you came up with a patch
> for that, yes. I can have a look at the dma bits laters, once my OMAP
> board finally works with the code as it currently stands. I'm still
> fighting with the mcasp driver right now ...

I quickly prepared two patches to change that, so that topic is out of
the way. But I did only compile-test them on OMAP - could you check on
your Davinci platform? Note that these apply on top of the patch in
discussion here (which isn't applied to the asoc tree yet).


Daniel

Comments

Matt Porter Oct. 4, 2012, 9:38 a.m. UTC | #1
On Oct 4, 2012, at 5:21 AM, Daniel Mack wrote:

> 
> 
> On 02.10.2012 18:50, Daniel Mack wrote:
>> On 02.10.2012 18:41, Matt Porter wrote:
>>> On Tue, Oct 02, 2012 at 03:42:47PM +0200, Daniel Mack wrote:
>>>> On 02.10.2012 13:06, Sekhar Nori wrote:
>>>>> On 10/2/2012 4:03 PM, Daniel Mack wrote:
>>>>>> On 02.10.2012 11:37, Mark Brown wrote:
>>>>>>> On Tue, Oct 02, 2012 at 10:48:53AM +0300, Peter Ujfalusi wrote:
>>>>>>> 
>>>>>>>> I also agree that ifdef is not a good solution.
>>>>>>>> It is better to have this information passed as device_data and via DT it can
>>>>>>>> be decided based on the compatible property for the device.
>>>>>>> 
>>>>>>> That's not really the problem here - the problem is that the APIs used
>>>>>>> to get the SRAM are DaVinci only so it's not possible to build on OMAP
>>>>>>> or other platforms.  The SRAM code needs to move to a standard API.
>>>>>> 
>>>>>> What about following Matt Porter's idea and ignore the SRAM code
>>>>>> entirely and port the entire PCM code to generic dmaengine code first?
>>>>>> The EDMA driver needs to learn support for cyclic DMA for that, and I
>>>>>> might give that a try in near future.
>>>>>> 
>>>>>> Later on, the SRAM ping-pong code can get added back using genalloc
>>>>>> functions, as Sekhar proposed. That needs to be done by someone who has
>>>>>> access to a Davinci board though, I only have a AM33xx/OMAP here.
>>>>> 
>>>>> We cannot "get rid" of SRAM code and add it back "later". It is required
>>>>> for most DaVinci parts. The SRAM code can be converted to use genalloc
>>>>> (conversion should be straightforward and I can help test it) and the
>>>>> code that uses SRAM can probably keep using the private EDMA API till
>>>>> the dmaengine EDMA driver has cyclic DMA support. Matt has already
>>>>> posted patches to move private EDMA APIs to a common location. That
>>>>> should keep AM335x build from breaking. Is this something that is feasible?
>>>> 
>>>> Yes - by "later" I just meant in a subsequent patch. But you're probably
>>>> right, we can also do that first.
>>>> 
>>>> I'm looking at that right now and the problem seems that we don't have a
>>>> sane way to dynamically look up gen_pools independently of the actual
>>>> run-time platform. All users of gen_pools seem to know which platform
>>>> they run on and access a platform-specific pool. So I don't currently
>>>> see how we could implement multi-platform code, gen_pools are fine but
>>>> don't solve the problem here.
>>>> 
>>>> Would it be an idea to add a char* member to gen_pools and a function
>>>> that can be used to dynamically look it up again? If a buffer with a
>>>> certain name exists, we can use it and install that ping-pong buffer,
>>>> otherwise we just don't. While that would be easy to do, it's a
>>>> tree-wide change.
>>>> 
>>>> Is there a better way that I miss?
>>> 
>>> At the high level there's two platform models we have to handle, the
>>> boot from board file !DT case, and then the boot from DT case. Since
>>> Davinci is just starting DT conversion, we mostly care about the !DT
>>> base in which the struct gen_pool * is passed in pdata to the ASoC
>>> driver. It is then selectable on a per-platform basis where the decision
>>> should be made.
>>> 
>>> Given a separate discussion with Sekhar, we're only going to have one
>>> SRAM pool on any DaVinci part right now...this was only a question on
>>> L138 anyway. But regardless, the created gen_pool will be passed via
>>> pdata.
>> 
>> I thought about this too, as mmp does it that way.
>> 
>>> Since DT conversion is starting and we need to consider that now,
>>> the idea there is to use the DT-based generic sram driver [1] such that
>>> when we do boot from DT on Davinci, the genpool is provided via phandle
>>> and the pointer extracted with the OF helpers that are part of the
>>> series.
>> 
>> A phandle is the cleanest way I think, yes.
>> 
>>> That's pretty much it. I'm reworking the backend support as discussed
>>> with Sekhar wrt to my uio_pruss series. I can post a standalone series
>>> that just replaces sram_* with genalloc for davinci ASoC.
>> 
>> As you can also test it, it would be easiest if you came up with a patch
>> for that, yes. I can have a look at the dma bits laters, once my OMAP
>> board finally works with the code as it currently stands. I'm still
>> fighting with the mcasp driver right now ...
> 
> I quickly prepared two patches to change that, so that topic is out of
> the way. But I did only compile-test them on OMAP - could you check on
> your Davinci platform? Note that these apply on top of the patch in
> discussion here (which isn't applied to the asoc tree yet).

I put a series together yesterday, just ran out of time to post
last night after testing. I'm posting that now...it's on top of my
uio_pruss/genalloc series and only addresss switching davinci-pcm to
genalloc (and actually enabling ping-pon from sram).

I'll take a look a your OMAP patches.

-Matt
Daniel Mack Oct. 4, 2012, 9:57 a.m. UTC | #2
On 04.10.2012 11:38, Porter, Matt wrote:
> 
> On Oct 4, 2012, at 5:21 AM, Daniel Mack wrote:
> 
>>
>>
>> On 02.10.2012 18:50, Daniel Mack wrote:
>>> On 02.10.2012 18:41, Matt Porter wrote:
>>>> On Tue, Oct 02, 2012 at 03:42:47PM +0200, Daniel Mack wrote:
>>>>> On 02.10.2012 13:06, Sekhar Nori wrote:
>>>>>> On 10/2/2012 4:03 PM, Daniel Mack wrote:
>>>>>>> On 02.10.2012 11:37, Mark Brown wrote:
>>>>>>>> On Tue, Oct 02, 2012 at 10:48:53AM +0300, Peter Ujfalusi wrote:
>>>>>>>>
>>>>>>>>> I also agree that ifdef is not a good solution.
>>>>>>>>> It is better to have this information passed as device_data and via DT it can
>>>>>>>>> be decided based on the compatible property for the device.
>>>>>>>>
>>>>>>>> That's not really the problem here - the problem is that the APIs used
>>>>>>>> to get the SRAM are DaVinci only so it's not possible to build on OMAP
>>>>>>>> or other platforms.  The SRAM code needs to move to a standard API.
>>>>>>>
>>>>>>> What about following Matt Porter's idea and ignore the SRAM code
>>>>>>> entirely and port the entire PCM code to generic dmaengine code first?
>>>>>>> The EDMA driver needs to learn support for cyclic DMA for that, and I
>>>>>>> might give that a try in near future.
>>>>>>>
>>>>>>> Later on, the SRAM ping-pong code can get added back using genalloc
>>>>>>> functions, as Sekhar proposed. That needs to be done by someone who has
>>>>>>> access to a Davinci board though, I only have a AM33xx/OMAP here.
>>>>>>
>>>>>> We cannot "get rid" of SRAM code and add it back "later". It is required
>>>>>> for most DaVinci parts. The SRAM code can be converted to use genalloc
>>>>>> (conversion should be straightforward and I can help test it) and the
>>>>>> code that uses SRAM can probably keep using the private EDMA API till
>>>>>> the dmaengine EDMA driver has cyclic DMA support. Matt has already
>>>>>> posted patches to move private EDMA APIs to a common location. That
>>>>>> should keep AM335x build from breaking. Is this something that is feasible?
>>>>>
>>>>> Yes - by "later" I just meant in a subsequent patch. But you're probably
>>>>> right, we can also do that first.
>>>>>
>>>>> I'm looking at that right now and the problem seems that we don't have a
>>>>> sane way to dynamically look up gen_pools independently of the actual
>>>>> run-time platform. All users of gen_pools seem to know which platform
>>>>> they run on and access a platform-specific pool. So I don't currently
>>>>> see how we could implement multi-platform code, gen_pools are fine but
>>>>> don't solve the problem here.
>>>>>
>>>>> Would it be an idea to add a char* member to gen_pools and a function
>>>>> that can be used to dynamically look it up again? If a buffer with a
>>>>> certain name exists, we can use it and install that ping-pong buffer,
>>>>> otherwise we just don't. While that would be easy to do, it's a
>>>>> tree-wide change.
>>>>>
>>>>> Is there a better way that I miss?
>>>>
>>>> At the high level there's two platform models we have to handle, the
>>>> boot from board file !DT case, and then the boot from DT case. Since
>>>> Davinci is just starting DT conversion, we mostly care about the !DT
>>>> base in which the struct gen_pool * is passed in pdata to the ASoC
>>>> driver. It is then selectable on a per-platform basis where the decision
>>>> should be made.
>>>>
>>>> Given a separate discussion with Sekhar, we're only going to have one
>>>> SRAM pool on any DaVinci part right now...this was only a question on
>>>> L138 anyway. But regardless, the created gen_pool will be passed via
>>>> pdata.
>>>
>>> I thought about this too, as mmp does it that way.
>>>
>>>> Since DT conversion is starting and we need to consider that now,
>>>> the idea there is to use the DT-based generic sram driver [1] such that
>>>> when we do boot from DT on Davinci, the genpool is provided via phandle
>>>> and the pointer extracted with the OF helpers that are part of the
>>>> series.
>>>
>>> A phandle is the cleanest way I think, yes.
>>>
>>>> That's pretty much it. I'm reworking the backend support as discussed
>>>> with Sekhar wrt to my uio_pruss series. I can post a standalone series
>>>> that just replaces sram_* with genalloc for davinci ASoC.
>>>
>>> As you can also test it, it would be easiest if you came up with a patch
>>> for that, yes. I can have a look at the dma bits laters, once my OMAP
>>> board finally works with the code as it currently stands. I'm still
>>> fighting with the mcasp driver right now ...
>>
>> I quickly prepared two patches to change that, so that topic is out of
>> the way. But I did only compile-test them on OMAP - could you check on
>> your Davinci platform? Note that these apply on top of the patch in
>> discussion here (which isn't applied to the asoc tree yet).
> 
> I put a series together yesterday, just ran out of time to post
> last night after testing. I'm posting that now...it's on top of my
> uio_pruss/genalloc series and only addresss switching davinci-pcm to
> genalloc (and actually enabling ping-pon from sram).

Ok, I don't care which version makes it in after all :)

> I'll take a look a your OMAP patches.

The patches I just sent out are only for Davinci and change the SRAM
functions to genalloc in the mcasp driver. They work just fine on OMAP
of course, as the code is disabled in there.


Daniel
Matt Porter Oct. 4, 2012, 10:28 a.m. UTC | #3
On Thu, Oct 04, 2012 at 11:57:30AM +0200, Daniel Mack wrote:
> On 04.10.2012 11:38, Porter, Matt wrote:
> > 
> > On Oct 4, 2012, at 5:21 AM, Daniel Mack wrote:
> > 
> >>
> >>
> >> On 02.10.2012 18:50, Daniel Mack wrote:
> >>> On 02.10.2012 18:41, Matt Porter wrote:
> >>>> On Tue, Oct 02, 2012 at 03:42:47PM +0200, Daniel Mack wrote:
> >>>>> On 02.10.2012 13:06, Sekhar Nori wrote:
> >>>>>> On 10/2/2012 4:03 PM, Daniel Mack wrote:
> >>>>>>> On 02.10.2012 11:37, Mark Brown wrote:
> >>>>>>>> On Tue, Oct 02, 2012 at 10:48:53AM +0300, Peter Ujfalusi wrote:
> >>>>>>>>
> >>>>>>>>> I also agree that ifdef is not a good solution.
> >>>>>>>>> It is better to have this information passed as device_data and via DT it can
> >>>>>>>>> be decided based on the compatible property for the device.
> >>>>>>>>
> >>>>>>>> That's not really the problem here - the problem is that the APIs used
> >>>>>>>> to get the SRAM are DaVinci only so it's not possible to build on OMAP
> >>>>>>>> or other platforms.  The SRAM code needs to move to a standard API.
> >>>>>>>
> >>>>>>> What about following Matt Porter's idea and ignore the SRAM code
> >>>>>>> entirely and port the entire PCM code to generic dmaengine code first?
> >>>>>>> The EDMA driver needs to learn support for cyclic DMA for that, and I
> >>>>>>> might give that a try in near future.
> >>>>>>>
> >>>>>>> Later on, the SRAM ping-pong code can get added back using genalloc
> >>>>>>> functions, as Sekhar proposed. That needs to be done by someone who has
> >>>>>>> access to a Davinci board though, I only have a AM33xx/OMAP here.
> >>>>>>
> >>>>>> We cannot "get rid" of SRAM code and add it back "later". It is required
> >>>>>> for most DaVinci parts. The SRAM code can be converted to use genalloc
> >>>>>> (conversion should be straightforward and I can help test it) and the
> >>>>>> code that uses SRAM can probably keep using the private EDMA API till
> >>>>>> the dmaengine EDMA driver has cyclic DMA support. Matt has already
> >>>>>> posted patches to move private EDMA APIs to a common location. That
> >>>>>> should keep AM335x build from breaking. Is this something that is feasible?
> >>>>>
> >>>>> Yes - by "later" I just meant in a subsequent patch. But you're probably
> >>>>> right, we can also do that first.
> >>>>>
> >>>>> I'm looking at that right now and the problem seems that we don't have a
> >>>>> sane way to dynamically look up gen_pools independently of the actual
> >>>>> run-time platform. All users of gen_pools seem to know which platform
> >>>>> they run on and access a platform-specific pool. So I don't currently
> >>>>> see how we could implement multi-platform code, gen_pools are fine but
> >>>>> don't solve the problem here.
> >>>>>
> >>>>> Would it be an idea to add a char* member to gen_pools and a function
> >>>>> that can be used to dynamically look it up again? If a buffer with a
> >>>>> certain name exists, we can use it and install that ping-pong buffer,
> >>>>> otherwise we just don't. While that would be easy to do, it's a
> >>>>> tree-wide change.
> >>>>>
> >>>>> Is there a better way that I miss?
> >>>>
> >>>> At the high level there's two platform models we have to handle, the
> >>>> boot from board file !DT case, and then the boot from DT case. Since
> >>>> Davinci is just starting DT conversion, we mostly care about the !DT
> >>>> base in which the struct gen_pool * is passed in pdata to the ASoC
> >>>> driver. It is then selectable on a per-platform basis where the decision
> >>>> should be made.
> >>>>
> >>>> Given a separate discussion with Sekhar, we're only going to have one
> >>>> SRAM pool on any DaVinci part right now...this was only a question on
> >>>> L138 anyway. But regardless, the created gen_pool will be passed via
> >>>> pdata.
> >>>
> >>> I thought about this too, as mmp does it that way.
> >>>
> >>>> Since DT conversion is starting and we need to consider that now,
> >>>> the idea there is to use the DT-based generic sram driver [1] such that
> >>>> when we do boot from DT on Davinci, the genpool is provided via phandle
> >>>> and the pointer extracted with the OF helpers that are part of the
> >>>> series.
> >>>
> >>> A phandle is the cleanest way I think, yes.
> >>>
> >>>> That's pretty much it. I'm reworking the backend support as discussed
> >>>> with Sekhar wrt to my uio_pruss series. I can post a standalone series
> >>>> that just replaces sram_* with genalloc for davinci ASoC.
> >>>
> >>> As you can also test it, it would be easiest if you came up with a patch
> >>> for that, yes. I can have a look at the dma bits laters, once my OMAP
> >>> board finally works with the code as it currently stands. I'm still
> >>> fighting with the mcasp driver right now ...
> >>
> >> I quickly prepared two patches to change that, so that topic is out of
> >> the way. But I did only compile-test them on OMAP - could you check on
> >> your Davinci platform? Note that these apply on top of the patch in
> >> discussion here (which isn't applied to the asoc tree yet).
> > 
> > I put a series together yesterday, just ran out of time to post
> > last night after testing. I'm posting that now...it's on top of my
> > uio_pruss/genalloc series and only addresss switching davinci-pcm to
> > genalloc (and actually enabling ping-pon from sram).
> 
> Ok, I don't care which version makes it in after all :)

Ok :)

> > I'll take a look a your OMAP patches.
> 
> The patches I just sent out are only for Davinci and change the SRAM
> functions to genalloc in the mcasp driver. They work just fine on OMAP
> of course, as the code is disabled in there.

Ok, same thing as I did and just sent out. These are tested on AM180x
and hook up ping-pong buffering so that there's actually one user of
all this ping-pong code. It's been sitting idle for a long time.

I didn't see your patches come by yet but will take a look to see if
we need to combine.

-Matt
Daniel Mack Oct. 4, 2012, 11:11 a.m. UTC | #4
On 04.10.2012 12:28, Matt Porter wrote:
> On Thu, Oct 04, 2012 at 11:57:30AM +0200, Daniel Mack wrote:
>> On 04.10.2012 11:38, Porter, Matt wrote:
>>>
>>> On Oct 4, 2012, at 5:21 AM, Daniel Mack wrote:
>>>
>>>>
>>>>
>>>> On 02.10.2012 18:50, Daniel Mack wrote:
>>>>> On 02.10.2012 18:41, Matt Porter wrote:
>>>>>> On Tue, Oct 02, 2012 at 03:42:47PM +0200, Daniel Mack wrote:
>>>>>>> On 02.10.2012 13:06, Sekhar Nori wrote:
>>>>>>>> On 10/2/2012 4:03 PM, Daniel Mack wrote:
>>>>>>>>> On 02.10.2012 11:37, Mark Brown wrote:
>>>>>>>>>> On Tue, Oct 02, 2012 at 10:48:53AM +0300, Peter Ujfalusi wrote:
>>>>>>>>>>
>>>>>>>>>>> I also agree that ifdef is not a good solution.
>>>>>>>>>>> It is better to have this information passed as device_data and via DT it can
>>>>>>>>>>> be decided based on the compatible property for the device.
>>>>>>>>>>
>>>>>>>>>> That's not really the problem here - the problem is that the APIs used
>>>>>>>>>> to get the SRAM are DaVinci only so it's not possible to build on OMAP
>>>>>>>>>> or other platforms.  The SRAM code needs to move to a standard API.
>>>>>>>>>
>>>>>>>>> What about following Matt Porter's idea and ignore the SRAM code
>>>>>>>>> entirely and port the entire PCM code to generic dmaengine code first?
>>>>>>>>> The EDMA driver needs to learn support for cyclic DMA for that, and I
>>>>>>>>> might give that a try in near future.
>>>>>>>>>
>>>>>>>>> Later on, the SRAM ping-pong code can get added back using genalloc
>>>>>>>>> functions, as Sekhar proposed. That needs to be done by someone who has
>>>>>>>>> access to a Davinci board though, I only have a AM33xx/OMAP here.
>>>>>>>>
>>>>>>>> We cannot "get rid" of SRAM code and add it back "later". It is required
>>>>>>>> for most DaVinci parts. The SRAM code can be converted to use genalloc
>>>>>>>> (conversion should be straightforward and I can help test it) and the
>>>>>>>> code that uses SRAM can probably keep using the private EDMA API till
>>>>>>>> the dmaengine EDMA driver has cyclic DMA support. Matt has already
>>>>>>>> posted patches to move private EDMA APIs to a common location. That
>>>>>>>> should keep AM335x build from breaking. Is this something that is feasible?
>>>>>>>
>>>>>>> Yes - by "later" I just meant in a subsequent patch. But you're probably
>>>>>>> right, we can also do that first.
>>>>>>>
>>>>>>> I'm looking at that right now and the problem seems that we don't have a
>>>>>>> sane way to dynamically look up gen_pools independently of the actual
>>>>>>> run-time platform. All users of gen_pools seem to know which platform
>>>>>>> they run on and access a platform-specific pool. So I don't currently
>>>>>>> see how we could implement multi-platform code, gen_pools are fine but
>>>>>>> don't solve the problem here.
>>>>>>>
>>>>>>> Would it be an idea to add a char* member to gen_pools and a function
>>>>>>> that can be used to dynamically look it up again? If a buffer with a
>>>>>>> certain name exists, we can use it and install that ping-pong buffer,
>>>>>>> otherwise we just don't. While that would be easy to do, it's a
>>>>>>> tree-wide change.
>>>>>>>
>>>>>>> Is there a better way that I miss?
>>>>>>
>>>>>> At the high level there's two platform models we have to handle, the
>>>>>> boot from board file !DT case, and then the boot from DT case. Since
>>>>>> Davinci is just starting DT conversion, we mostly care about the !DT
>>>>>> base in which the struct gen_pool * is passed in pdata to the ASoC
>>>>>> driver. It is then selectable on a per-platform basis where the decision
>>>>>> should be made.
>>>>>>
>>>>>> Given a separate discussion with Sekhar, we're only going to have one
>>>>>> SRAM pool on any DaVinci part right now...this was only a question on
>>>>>> L138 anyway. But regardless, the created gen_pool will be passed via
>>>>>> pdata.
>>>>>
>>>>> I thought about this too, as mmp does it that way.
>>>>>
>>>>>> Since DT conversion is starting and we need to consider that now,
>>>>>> the idea there is to use the DT-based generic sram driver [1] such that
>>>>>> when we do boot from DT on Davinci, the genpool is provided via phandle
>>>>>> and the pointer extracted with the OF helpers that are part of the
>>>>>> series.
>>>>>
>>>>> A phandle is the cleanest way I think, yes.
>>>>>
>>>>>> That's pretty much it. I'm reworking the backend support as discussed
>>>>>> with Sekhar wrt to my uio_pruss series. I can post a standalone series
>>>>>> that just replaces sram_* with genalloc for davinci ASoC.
>>>>>
>>>>> As you can also test it, it would be easiest if you came up with a patch
>>>>> for that, yes. I can have a look at the dma bits laters, once my OMAP
>>>>> board finally works with the code as it currently stands. I'm still
>>>>> fighting with the mcasp driver right now ...
>>>>
>>>> I quickly prepared two patches to change that, so that topic is out of
>>>> the way. But I did only compile-test them on OMAP - could you check on
>>>> your Davinci platform? Note that these apply on top of the patch in
>>>> discussion here (which isn't applied to the asoc tree yet).
>>>
>>> I put a series together yesterday, just ran out of time to post
>>> last night after testing. I'm posting that now...it's on top of my
>>> uio_pruss/genalloc series and only addresss switching davinci-pcm to
>>> genalloc (and actually enabling ping-pon from sram).
>>
>> Ok, I don't care which version makes it in after all :)
> 
> Ok :)
> 
>>> I'll take a look a your OMAP patches.
>>
>> The patches I just sent out are only for Davinci and change the SRAM
>> functions to genalloc in the mcasp driver. They work just fine on OMAP
>> of course, as the code is disabled in there.
> 
> Ok, same thing as I did and just sent out. These are tested on AM180x
> and hook up ping-pong buffering so that there's actually one user of
> all this ping-pong code. It's been sitting idle for a long time.
> 
> I didn't see your patches come by yet but will take a look to see if
> we need to combine.

I attached them to my mail for a quick review. I wanted to queue them up
to a bigger series I'll send out later ...


Daniel
diff mbox

Patch

From 465846f6fd1ea39534f32b984d352b5bc6928889 Mon Sep 17 00:00:00 2001
From: Daniel Mack <zonque@gmail.com>
Date: Thu, 4 Oct 2012 11:12:28 +0200
Subject: [PATCH 2/2] ALSA: ASoC: McASP: use gen_pool from platform data

This makes the gen_pool SRAM usage a runtime decision.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
 sound/soc/davinci/davinci-pcm.c | 59 +++++++++++++++++++++++++++++------------
 1 file changed, 42 insertions(+), 17 deletions(-)

diff --git a/sound/soc/davinci/davinci-pcm.c b/sound/soc/davinci/davinci-pcm.c
index 7ac5a19..f47b8f3 100644
--- a/sound/soc/davinci/davinci-pcm.c
+++ b/sound/soc/davinci/davinci-pcm.c
@@ -16,6 +16,7 @@ 
 #include <linux/slab.h>
 #include <linux/dma-mapping.h>
 #include <linux/kernel.h>
+#include <linux/genalloc.h>
 
 #include <sound/core.h>
 #include <sound/pcm.h>
@@ -261,26 +262,27 @@  static void davinci_pcm_dma_irq(unsigned link, u16 ch_status, void *data)
 	}
 }
 
-#if defined(CONFIG_SND_DAVINCI_HAVE_SRAM)
+#ifdef CONFIG_GENERIC_ALLOCATOR
 static int allocate_sram(struct snd_pcm_substream *substream, unsigned size,
-		struct snd_pcm_hardware *ppcm)
+			 struct snd_pcm_hardware *ppcm, struct gen_pool *pool)
 {
 	struct snd_dma_buffer *buf = &substream->dma_buffer;
 	struct snd_dma_buffer *iram_dma = NULL;
+	unsigned long iram_virt = 0;
 	dma_addr_t iram_phys = 0;
-	void *iram_virt = NULL;
 
 	if (buf->private_data || !size)
 		return 0;
 
 	ppcm->period_bytes_max = size;
-	iram_virt = sram_alloc(size, &iram_phys);
+	iram_virt = gen_pool_alloc(pool, size);
 	if (!iram_virt)
 		goto exit1;
+	iram_phys = gen_pool_virt_to_phys(pool, iram_virt);
 	iram_dma = kzalloc(sizeof(*iram_dma), GFP_KERNEL);
 	if (!iram_dma)
 		goto exit2;
-	iram_dma->area = iram_virt;
+	iram_dma->area = (void *) iram_virt;
 	iram_dma->addr = iram_phys;
 	memset(iram_dma->area, 0, size);
 	iram_dma->bytes = size;
@@ -288,12 +290,40 @@  static int allocate_sram(struct snd_pcm_substream *substream, unsigned size,
 	return 0;
 exit2:
 	if (iram_virt)
-		sram_free(iram_virt, size);
+		gen_pool_free(pool, iram_virt, size);
 exit1:
 	return -ENOMEM;
 }
+
+static void free_sram(struct snd_pcm_substream *substream)
+{
+	struct snd_soc_pcm_runtime *rtd = substream->private_data;
+	struct snd_dma_buffer *buf = &substream->dma_buffer;
+	struct snd_dma_buffer *iram_dma = buf->private_data;
+	struct davinci_pcm_dma_params *params, *pa;
+
+	if (!buf || !rtd || !iram_dma)
+		return;
+
+	pa = snd_soc_dai_get_dma_data(rtd->cpu_dai, substream);
+	params = &pa[substream->stream];
+
+	gen_pool_free(params->sram_pool, (unsigned long) iram_dma->area,
+		      iram_dma->bytes);
+}
+#else
+static int allocate_sram(struct snd_pcm_substream *substream, unsigned size,
+			 struct snd_pcm_hardware *ppcm, struct gen_pool *pool)
+{
+	return 0;
+}
+
+static void free_sram(struct snd_pcm_substream *substream)
+{
+}
 #endif
 
+
 /*
  * Only used with ping/pong.
  * This is called after runtime->dma_addr, period_bytes and data_type are valid
@@ -680,9 +710,11 @@  static int davinci_pcm_open(struct snd_pcm_substream *substream)
 
 	ppcm = (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) ?
 			&pcm_hardware_playback : &pcm_hardware_capture;
-#if defined(CONFIG_SND_DAVINCI_HAVE_SRAM)
-	allocate_sram(substream, params->sram_size, ppcm);
-#endif
+
+	if (params->sram_pool)
+		allocate_sram(substream, params->sram_size, ppcm,
+			      params->sram_pool);
+
 	snd_soc_set_runtime_hwparams(substream, ppcm);
 	/* ensure that buffer size is a multiple of period size */
 	ret = snd_pcm_hw_constraint_integer(runtime,
@@ -811,7 +843,6 @@  static void davinci_pcm_free(struct snd_pcm *pcm)
 	int stream;
 
 	for (stream = 0; stream < 2; stream++) {
-		struct snd_dma_buffer *iram_dma;
 		substream = pcm->streams[stream].substream;
 		if (!substream)
 			continue;
@@ -823,13 +854,7 @@  static void davinci_pcm_free(struct snd_pcm *pcm)
 		dma_free_writecombine(pcm->card->dev, buf->bytes,
 				      buf->area, buf->addr);
 		buf->area = NULL;
-#if defined(CONFIG_SND_DAVINCI_HAVE_SRAM)
-		iram_dma = buf->private_data;
-		if (iram_dma) {
-			sram_free(iram_dma->area, iram_dma->bytes);
-			kfree(iram_dma);
-		}
-#endif
+		free_sram(substream);
 	}
 }
 
-- 
1.7.11.4