[alsa-devel] [PATCH 8/8] ASoC: AMD: add AMD ASoC ACP-I2S driver

[maruthi srinivas]

On Sat, Oct 24, 2015 at 1:01 AM, Mark Brown <broonie at kernel.org> wrote:
> On Sat, Oct 24, 2015 at 12:20:09AM +0530, maruthi srinivas wrote:
>> On Thu, Oct 22, 2015 at 9:44 PM, Mark Brown <broonie at kernel.org> wrote:
>> > On Thu, Oct 08, 2015 at 12:12:40PM -0400, Alex Deucher wrote:
>
> Please document this clearly - your comment doesn't appear to relate to
> the case where system resume powers things on at all.
>
ok.

>> >> +/* Initialize the dma descriptors location in SRAM and page size */
>> >> +static void acp_dma_descr_init(void __iomem *acp_mmio)
>> >> +{
>> >> +     u32 sram_pte_offset = 0;
>> >> +
>> >> +     /* SRAM starts at 0x04000000. From that offset one page (4KB) left for
>> >> +      * filling DMA descriptors.sram_pte_offset = 0x04001000 , used for
>
>> > This is a device relative address rather than an absolute address?  A
>> > lot of these numbers seem kind of large...
>
>> That is SRAM block's offset address. Sorry. I didn't understand the
>> expected change, here.
>
> Offset with regard to what?  I'm asking if these addresses are within
> the IP or global.

mentioned addresses are offsets within the ACP IP.
>

>> >> +static void acp_turnoff_sram_banks(void __iomem *acp_mmio)
>> >> +{
>> >> +     /* Bank 0 : used for DMA descriptors
>> >> +      * Bank 1 to 4 : used for playback
>> >> +      * Bank 5 to 8 : used for capture
>> >> +      * Each bank is 8kB and max size allocated for playback/ capture is
>> >> +      * 16kB(max period size) * 2(max periods) reserved for playback/capture
>> >> +      * in ALSA driver
>> >> +      * Turn off all SRAM banks except above banks during playback/capture
>> >> +      */
>> >> +     u32 val, bank;
>
>> > I didn't see any runtime management of the other SRAM bank power, seems
>> > like that'd be a good idea?
>
>> SRAM banks are part of ACP IP. With ACP's runtime PM handling, all blocks
>> within ACP IP can be powered-off and on.
>
> So why can't that cope with these banks then?

Maybe Iam not clear before. I mean that memory banks which wont be needed
at all are turned off forever. Using runtime PM, when complete ACP IP gets
powered-off all the banks(including the ones used for play/capture) within IP
are turned off. When IP is runtime resumed, though all banks gets turned on,
the unused banks are turned off again. With this, Iam trying to achieve
runtime management.

>
>> >> +/* Update DMA postion in audio ring buffer at period level granularity.
>> >> + * This will be used by ALSA PCM driver
>> >> + */
>> >> +u32 acp_update_dma_pointer(void __iomem *acp_mmio, int direction,
>> >> +                               u32 period_size)
>> >> +{
>
>> > Don't limit the accuracy to period level if the harwdare can do better,
>> > report the current position as accurately as possible please.  This is
>> > also feeling like we've got an unneeded abstraction here - why was this
>> > not just directly the pointer operation?
>
>> The current version of hardware has the limitation of accuracy reporting.
>
> If the hardware is limited why does the comment suggest that we are
> limiting based on periods?

I will modify accordingly.