On Thu, Oct 22, 2015 at 9:44 PM, Mark Brown <broonie@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxx
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
