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

[maruthi srinivas]

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:
>
>> ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver
>> provides the platform DMA component to ALSA core.
>
> Overall my main comment on a lot of this code is that it feels like we
> have created a lot of infrastructure that parallels standard Linux
> subsystems and interfaces without something that clearly shows why we're
> doing that.  There may be good reasons but they've not been articulated
> and it's making the code a lot more complex to follow and review.  We
> end up with multiple layers of abstraction and indirection that aren't
> explained.
>
> This patch is also rather large and appears to contain multiple
> components which could be split, there's at least the DMA driver and
> this abstraction layer than the DMA driver builds on.
>
>> +/* ACP DMA irq handler routine for playback, capture usecases */
>> +int dma_irq_handler(struct device *dev)
>> +{
>> +     u16 dscr_idx;
>> +     u32 intr_flag;
>
> This says it's an interrupt handler but it's using some custom,
> non-genirq interface?
>
Irq handling is based on parent device's (part of other subsystem)
provided interfaces. I will coordinate with others for this.

Do you mean, using virtual irq assignment for MFD devices
(ACP is a MFD device) and registering irq handler for it ?

>> +             /* Let ACP know the Allocated memory */
>> +             num_of_pages = PAGE_ALIGN(size) >> PAGE_SHIFT;
>> +
>> +             /* Fill the page table entries in ACP SRAM */
>> +             rtd->pg = pg;
>> +             rtd->size = size;
>> +             rtd->num_of_pages = num_of_pages;
>> +             rtd->direction = substream->stream;
>
> We never reference num_of_pages other than to assign it into the page
> table entry?
>
Sorry, I didn't understand your comment.
I used 'num_of_pages' to configure ACP audio device for accessing system
memory. The implementation is in 'acp_pte_config' function in the patch.

>> +static int acp_dma_close(struct snd_pcm_substream *substream)
>> +{
>> +     struct snd_pcm_runtime *runtime = substream->runtime;
>> +     struct audio_substream_data *rtd = runtime->private_data;
>> +     struct snd_soc_pcm_runtime *prtd = substream->private_data;
>> +
>> +     kfree(rtd);
>> +
>> +     pm_runtime_mark_last_busy(prtd->platform->dev);
>
> Why the _mark_last_busy() here?

I want to power off ACP audio IP, when the audio usecase is not active for
sometime (run time PM). I felt, 'close' is the correct place to mark this.
>
>> +     /* The following members gets populated in device 'open'
>> +      * function. Till then interrupts are disabled in 'acp_hw_init'
>> +      * and device doesn't generate any interrupts.
>> +      */
>> +
>> +     audio_drv_data->play_stream = NULL;
>> +     audio_drv_data->capture_stream = NULL;
>> +
>> +     audio_drv_data->iprv->dev = &pdev->dev;
>> +     audio_drv_data->iprv->acp_mmio = audio_drv_data->acp_mmio;
>> +     audio_drv_data->iprv->enable_intr = acp_enable_external_interrupts;
>> +     audio_drv_data->iprv->irq_handler = dma_irq_handler;
>
> I do not that we never seem to reset any of these in teardown paths and
> I am slightly worried about races with interrupt handling in teardown,
>
I will recheck this.

>> +static int acp_pcm_suspend(struct device *dev)
>> +{
>> +     bool pm_rts;
>> +     struct audio_drv_data *adata = dev_get_drvdata(dev);
>> +
>> +     pm_rts = pm_runtime_status_suspended(dev);
>> +     if (pm_rts == false)
>> +             acp_suspend(adata->acp_mmio);
>> +
>> +     return 0;
>> +}
>
> This appears to merely call into the parent/core device (at least it
> looks like this is the intention, there's a bunch of infrastructure fo
> the core device which appeaars to replicate standard infrastructure).
> Isn't whatever this eventually ends up doing handled by the parent
> device in the normal PM callbacks.
>

ACP device (child) can power off itself, when it receives suspend
request. So, the intention is to call 'acp_suspend' (defined in same patch)
from the 'suspend' callback of ACP device.

> This parallel infrastructure seems like it needs some motivation,
> especially given that when I look at the implementation of functions
> like amd_acp_suspend() and amd_acp_resume() in the preceeding patch they
> are empty and therefore do nothing (they're also not exported so I
> expect we get build errors if this is a module and the core isn't).
> The easiest thing is probably to remove the code until there is an
> immplementation and then review at that time.
>

There were two different functions with same name in two drivers
interacting here.That might have introduced some confusion.
Sorry, I will modify that.

Also, amd_acp_*() can be removed later, as they are not expected to add any
functonality in future. ACP device can be suspended/resumed using acp_*_tile().

>> +static int acp_pcm_resume(struct device *dev)
>> +{
>> +     bool pm_rts;
>> +     struct snd_pcm_substream *stream;
>> +     struct snd_pcm_runtime *rtd;
>> +     struct audio_substream_data *sdata;
>> +     struct audio_drv_data *adata = dev_get_drvdata(dev);
>> +
>> +     pm_rts = pm_runtime_status_suspended(dev);
>> +     if (pm_rts == true) {
>> +             /* Resumed from system wide suspend and there is
>> +              * no pending audio activity to resume. */
>> +             pm_runtime_disable(dev);
>> +             pm_runtime_set_active(dev);
>> +             pm_runtime_enable(dev);
>
> The above looks very strange - why are we bouncing runtime PM like this?

Sorry, I didn't understand your comment. I felt, steps mentioned in
kernel documentation :
http://lxr.free-electrons.com/source/Documentation/power/runtime_pm.txt#L634
is applicable in this scenario. I maybe wrong, but felt that is applicable.

>
>> +/* 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.

>
>> +u16 get_dscr_idx(void __iomem *acp_mmio, int direction)
>> +{
>> +     u16 dscr_idx;
>> +
>> +     if (direction == SNDRV_PCM_STREAM_PLAYBACK) {
>> +             dscr_idx = acp_reg_read(acp_mmio, mmACP_DMA_CUR_DSCR_13);
>> +             dscr_idx = (dscr_idx == PLAYBACK_START_DMA_DESCR_CH13) ?
>> +                             PLAYBACK_START_DMA_DESCR_CH12 :
>> +                             PLAYBACK_END_DMA_DESCR_CH12;
>
> Please write normal if statements rather than using the ternery
> operator.
>
Ok.

>> +/* Check whether ACP DMA interrupt (IOC) is generated or not */
>> +u32 acp_get_intr_flag(void __iomem *acp_mmio)
>> +{
>> +     u32 ext_intr_status;
>> +     u32 intr_gen;
>> +
>> +     ext_intr_status = acp_reg_read(acp_mmio, mmACP_EXTERNAL_INTR_STAT);
>> +     intr_gen = (((ext_intr_status &
>> +                   ACP_EXTERNAL_INTR_STAT__DMAIOCStat_MASK) >>
>> +                  ACP_EXTERNAL_INTR_STAT__DMAIOCStat__SHIFT));
>> +
>> +     return intr_gen;
>> +}
>
> Looking at a lot of the interrupt code I can't help but think there's a
> genirq interrupt controller lurking in here somewhere.
>
I will coordinate with other subsystem driver author, that this driver
is dependent on.

>> +     /*Invalidating the DAGB cache */
>> +     acp_reg_write(ENABLE, acp_mmio, mmACP_DAGB_ATU_CTRL);
>
> /* spaces around comments please */
>
>> +     if ((ch_num == ACP_TO_I2S_DMA_CH_NUM) ||
>> +         (ch_num == ACP_TO_SYSRAM_CH_NUM) ||
>> +         (ch_num == I2S_TO_ACP_DMA_CH_NUM))
>> +             dma_ctrl |= ACP_DMA_CNTL_0__DMAChIOCEn_MASK;
>> +     else
>> +             dma_ctrl &= ~ACP_DMA_CNTL_0__DMAChIOCEn_MASK;
>
> switch statement please.

Ok.
>
>> +     u32 delay_time = ACP_DMA_RESET_TIME;
>
>> +     /* check the channel status bit for some time and return the status */
>> +     while (0 < delay_time) {
>> +             dma_ch_sts = acp_reg_read(acp_mmio, mmACP_DMA_CH_STS);
>> +             if (!(dma_ch_sts & BIT(ch_num))) {
>> +                     /* clear the reset flag after successfully stopping
>> +                        the dma transfer and break from the loop */
>> +                     dma_ctrl &= ~ACP_DMA_CNTL_0__DMAChRst_MASK;
>> +
>> +                     acp_reg_write(dma_ctrl, acp_mmio, mmACP_DMA_CNTL_0
>> +                                                             + ch_num);
>> +                     break;
>> +             }
>> +             delay_time--;
>> +     }
>> +}
>
> This isn't really a time, it's a number of spins (the amount of time
> involved presumably depending on clock speed).  If this were a time I'd
> expect to see a delay or sleep in here.
>
> We're also falling off the end of the loop silently if the hardware
> fails to respond, if it's worth waiting for the hardware to do it's
> thing I'd expect it's also worth displaying an error if that happens.
> This is a common pattern in much of the rest of the driver.
>
Ok, will modify.

>> +/* power off a tile/block within ACP */
>> +static void acp_suspend_tile(void __iomem *acp_mmio, int tile)
>> +{
>> +     u32 val = 0;
>> +     u32 timeout = 0;
>> +
>> +     if ((tile  < ACP_TILE_P1) || (tile > ACP_TILE_DSP2))
>> +             return;
>> +
>> +     val = acp_reg_read(acp_mmio, mmACP_PGFSM_READ_REG_0 + tile);
>> +     val &= ACP_TILE_ON_MASK;
>
> This is definitely looking like a SoC that could benefit from the
> standard kernel power management infrastructure and/or DAPM.  There's a
> lot of code here that looks like it's following very common SoC design
> patterns and could benefit from using infrastructure more.
>
Sorry, I didn't understand. Could you help in adding more pointers on this.
The device can get powered-off, with this helper function, which can
be called from device 'suspend' callback.

>> +static void acp_resume_tile(void __iomem *acp_mmio, int tile)
>> +{
>> +     u32 val = 0;
>> +     u32 timeout = 0;
>> +
>> +     if ((tile  < ACP_TILE_P1) || (tile > ACP_TILE_DSP2))
>> +             return;
>
> Not worth printing an error if the user passed in something invalid?

Ok.

>
>> +/* Shutdown unused SRAM memory banks in 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.

>
>> +     /* initiailizing Garlic Control DAGB register */
>> +     acp_reg_write(ONION_CNTL_DEFAULT, acp_mmio, mmACP_AXI2DAGB_ONION_CNTL);
>> +
>> +     /* initiailizing Onion Control DAGB registers */
>> +     acp_reg_write(GARLIC_CNTL_DEFAULT, acp_mmio,
>> +                     mmACP_AXI2DAGB_GARLIC_CNTL);
>
> The comments don't match the code...

Oops, I will correct it.

>
>> +/* 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)
>> +{
>> +     u32 pos;
>> +     u16 dscr;
>> +     u32 mul;
>> +     u32 dma_config;
>> +
>> +     pos = 0;
>> +
>> +     if (direction == SNDRV_PCM_STREAM_PLAYBACK) {
>> +             dscr = acp_reg_read(acp_mmio, mmACP_DMA_CUR_DSCR_13);
>> +
>> +             mul = (dscr == PLAYBACK_START_DMA_DESCR_CH13) ? 0 : 1;
>> +             pos =  (mul * 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.
I will remove abstraction, if suggested.

>> +/* Wait for initial buffering to complete in HOST to SRAM DMA channel
>> + * for plaback usecase
>> + */
>> +void prebuffer_audio(void __iomem *acp_mmio)
>> +{
>> +     u32 dma_ch_sts;
>> +     u32 channel_mask = BIT(SYSRAM_TO_ACP_CH_NUM);
>> +
>> +     do {
>> +             /* Read the channel status to poll dma transfer completion
>> +              * (System RAM to SRAM)
>> +              * In this case, it will be runtime->start_threshold
>> +              * (2 ALSA periods) of transfer. Rendering starts after this
>> +              * threshold is met.
>> +              */
>> +             dma_ch_sts = acp_reg_read(acp_mmio, mmACP_DMA_CH_STS);
>> +             udelay(20);
>> +     } while (dma_ch_sts & channel_mask);
>
> This will hang hard if the hardware fails to respond for some reason,
> please have a timeout.  A cpu_relax() would also be friendly.
>
I will modify this.

>> +#define DISABLE                                      0
>> +#define ENABLE                                       1
>
> Please don't do this :(

Ok.
>
>> +#define STATUS_SUCCESS 0
>> +#define STATUS_UNSUCCESSFUL -1
>
> Please use normal Linux error codes.
>

Ok.
> _______________________________________________
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>