On Thu, Oct 22, 2015 at 9:44 PM, Mark Brown broonie@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 forThis 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;elsedma_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 stoppingthe 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@alsa-project.org http://mailman.alsa-project.org/mailman/listinfo/alsa-devel