[RESEND PATCH v3 1/2] PM / Domains: Allow genpd to power on during the system PM phases
On Mon, May 30, 2016 at 3:13 PM, Ulf Hansson wrote: > If the PM domain is powered off when the first device starts its system PM > prepare phase, genpd prevents any further attempts to power on the PM > domain during the following system PM phases. Not until the system PM > complete phase is finalized for all devices in the PM domain, genpd again > allows it to be powered on. > > This behaviour needs to be changed, as a subsystem/driver for a device in > the same PM domain may still need to be able to serve requests in some of > the system PM phases. Accordingly, it may need to runtime resume its > device and thus also request the corresponding PM domain to be powered on. > > To deal with these scenarios, let's make the device operational in the > system PM prepare phase by runtime resuming it, no matter if the PM domain > is powered on or off. Changing this also enables us to remove genpd's > suspend_power_off flag, as it's being used to track this condition. > Additionally, we must allow the PM domain to be powered on via runtime PM > during the system PM phases. > > This change also requires a fix in the AMD ACP (Audio CoProcessor) drm > driver. It registers a genpd to model the ACP as a PM domain, but > unfortunately it's also abuses genpd's "internal" suspend_power_off flag > to deal with a corner case at system PM resume. > > More precisely, the so called SMU block powers on the ACP at system PM > resume, unconditionally if it's being used or not. This may lead to that > genpd's internal status of the power state, may not correctly reflect the > power state of the HW after a system PM resume. > > Because of changing the behaviour of genpd, by runtime resuming devices in > the prepare phase, the AMD ACP drm driver no longer have to deal with this > corner case. So let's just drop the related code in this driver. > > Cc: David Airlie > Cc: Alex Deucher > Cc: Christian König > Cc: Maruthi Srinivas Bayyavarapu > Cc: dri-devel at lists.freedesktop.org > Signed-off-by: Ulf Hansson > --- > > Changes in v3: > - Updated changelog. > > Changes in v2: > - Updated changelog. > - Added a fix in the AMD ACP (Audio CoProcessor) drm driver, which > registers a genpd. The fix removes the usage of genpd's internal > suspend_power_off flag as it's not needed after this change. Because > of > this change I am also requesting an ack from the drm driver > maintainer. > > > --- > drivers/base/power/domain.c | 84 > - > drivers/gpu/drm/amd/amdgpu/amdgpu_acp.c | 23 - > include/linux/pm_domain.h | 1 - > 3 files changed, 31 insertions(+), 77 deletions(-) > For the ACP part of DRM driver : Acked-by: Maruthi Bayyavarapu > > -- > To unsubscribe from this list: send the line "unsubscribe linux-pm" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Fri, Aug 21, 2015 at 4:48 AM, Mark Brown wrote:
>
> On Thu, Aug 20, 2015 at 05:36:34PM -0400, Alex Deucher wrote:
> > From: Maruthi Srinivas Bayyavarapu
> >
> > ACP IP block consists of dedicated DMA and I2S blocks. The PCM driver
> > provides the DMA and CPU DAI components to ALSA core.
>
> This is flagged as patch 3/3 but appears to be sent as a single patch.
> Please don't do this, the purpose of numbering patches is to help people
> tell which order they are in. Numbering only makes sense when you send
> more than one patch, if you are sending a single patch there is no need
> to number.
Ok, sorry. I will be more careful next time. Actually, two patches are
sent - one for
DRM and other ASoC. It should have been represented as 2/2.
>
>
> > +++ b/sound/soc/amd/Kconfig
> > @@ -0,0 +1,5 @@
> > +config SND_SOC_AMD_ACP
> > + tristate "AMD Audio Coprocessor support"
> > + depends on MFD_CORE
>
> What is the dependency on the MFD core?
None. Sorry, I forgot to remove this.
Actually, AMD DRM driver depends on MFD_CORE to create a platform device
for ASoC. This is already present in DRM patch.
>
>
> > +/*
> > + * AMD ALSA SoC PCM Driver
> > + *
> > + * Copyright 2014-2015 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the
> > "Software"),
> > + * to deal in the Software without restriction, including without
> > limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included
> > in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS
> > OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
>
> Still not happy with the licensing.
I will contact our internal teams and get back on this.
>
> > +static const struct snd_soc_component_driver dw_i2s_component = {
> > + .name = "dw-i2s",
> > +};
>
> We already have a driver for the DesignWare I2S controller. To repeat
> the concern I raised in a slightly different bit of the code last time:
>
> | This doesn't appear to be a designware-i2s driver, we already have one
> | of those?
>
Our IP block includes few AMD IPs along with DesignWare I2S IP. I have reused
code from Designware I2S controller from
sound/soc/dwc/designware_i2s.c and because
of the way IPs are coupled together, I couldn't use existing
Designware I2S driver as is.
I have given credit to the original author in DRM patch copyright
header, where register I2S read/writes
are made. Do I need to add the same header in ASoC driver too ?
>
> Like I say please stop ignoring review comments. The hw_params()
> certainly seems to bear more than a passing resemblance.
>
Iam sorry, this mistake won't be repeated.
>
> > +static void acp_pcm_period_elapsed(struct device *dev, u16 play_intr,
> > + u16 capture_intr)
> > +{
> > + struct snd_pcm_substream *substream;
> > + struct audio_drv_data *irq_data = dev_get_drvdata(dev);
> > +
> > + /* Inform ALSA about the period elapsed (one out of two periods) */
> > + if (play_intr)
> > + substream = irq_data->play_stream;
> > + else
> > + substream = irq_data->capture_stream;
>
> So you've replaced the two if statements with an if/else which means
> subsstream is now guaranteed to be initialized but perhaps not in the
> way the caller intended... I did find the caller (which got moved into
> another patch in this version) and it appears that the two u16s are
> actually used to pass boolean flags. The whole interface here now
> seems odd, what is going on here?
>
Agreed. I will modify this interface to use a bool, in next revision
>
> > + if (NULL != pg) {
>
> We st
[alsa-devel] [PATCH V5 3/3] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Tue, Aug 25, 2015 at 11:36 AM, Mark Brown wrote: > On Mon, Aug 24, 2015 at 04:08:31PM -0400, Alex Deucher wrote: >> On Fri, Aug 21, 2015 at 12:17 PM, Mark Brown wrote: > >> > What I'm looking for is actual code sharing where we use the same code >> > for the I2S controller block or a clear and documented understanding of >> > why it is not possible to share things. > >> Maruthi can clarify further, but it's not possible to use the >> designware driver directly because: >> 1. The i2s registers are within the same MMIO aperture as our other >> GPU registers. Our GPU driver is designed in such a way that the >> specific IP modules don't have direct access to the MMIO aperture. >> They use functions provided by the core driver to access registers. >> Thus the ACP IP module within the GPU driver does not have direct >> access to the mmio base pointer in order to pass it on. > > Please explain this in more detail, shared register ranges are very > common and are the sort of things MFDs are supposed to help with. > In our case, ACP I2S driver need not do a 'devm_ioremap_resource' to get mmio base. ACP audio IP (DMA + I2S+ Others) registers can be accessed, using GPU's MMIO base. During GPU driver design, it was decided that all the register access for entire GPU MMIO aperture (includes ACP and others) to be done in GPU module only. This is implemented in another patch in this patch series using a abstraction layer. A set of functions were defined for audio DMA and I2S functionality within GPU driver module and those function pointers were passed as platform data to ALSA PCM driver to handle both DMA and I2S. >> 2. The designware driver depends on the CLKDEV framework which we >> don't currently support. > > You need to support the clock API, it's very easy to do so so there is > no excuse for doing something custom here. > Codec acts as master in our case to provide clock to i2s controller and there wasn't a need to use clock APIs unlike in existing designware i2s driver. There is no custom implementation. >> 3. Our hardware does not support S16_LE > > If you have modified the designware IP to remove this support (why would > anyone do that?) it's a trivial quirk, if the restriction comes from > some other part of the system like the DMA driver then the constraint > will come from that part of the system. There is a bug in ACP SoC implementation (which combines internal DMA, designware I2S and other blocks) for 16bit and lower resolution. I felt , it would be better to limit functionality in I2S DAI capabilities. I will put this limitation in DMA driver capabilities, to represent overall sound card capabilities, if you suggest.
[alsa-devel] [PATCH 1/5] ASoC : dwc : support dw i2s in slave mode
On Mon, Oct 5, 2015 at 9:01 PM, Mark Brown wrote: > On Fri, Sep 25, 2015 at 05:48:22PM -0400, Alex Deucher wrote: >> From: Maruthi Srinivas Bayyavarapu >> >> dw i2s controller can work in slave mode, codec being master. >> dw i2s is made to support master/slave operation, by reading dwc >> register. > > I'll apply this but can you please send a followup implementing a > _dai_fmt() operation that checks to make sure that the mode we're > setting corresponds to what we read back from the hardware. Ok, I will send. > > ___ > Alsa-devel mailing list > Alsa-devel at alsa-project.org > http://mailman.alsa-project.org/mailman/listinfo/alsa-devel >
[alsa-devel] [PATCH 2/5] ASoC : dwc : support dw i2s in AMD platform
On Mon, Oct 5, 2015 at 9:11 PM, Mark Brown wrote:
> On Fri, Sep 25, 2015 at 05:48:23PM -0400, Alex Deucher wrote:
>> From: Maruthi Srinivas Bayyavarapu
>>
>> Vendor specific quirk was added to:
>> 1. Support AMD platform which has two dwc controllers with different
>>base address for playback and capture. Also, I2S_COMP_PARAM_*
>>registers offsets differed.
>
>> 2. Resume audio which was active before system suspend.
>>After 'resume', dwc need to be reconfigured with params
>>configured during 'hw_params' callback. With this, audio usecase
>>continues from where it got stopped because of 'suspend'.
>
> This is hard to review since there are several different changes in
> here, one change per commit is muuch more helpful. As is I'm not 100%
> sure exactly what each bit of the change is intended to do. One example
> here is that a separate patch splitting pbase and cbase would be really
> helpful.
Ok, I will split into multiple patches and send again.
>
>> --- a/include/sound/designware_i2s.h
>> +++ b/include/sound/designware_i2s.h
>> @@ -44,6 +44,9 @@ struct i2s_platform_data {
>> int channel;
>> u32 snd_fmts;
>> u32 snd_rates;
>> + #define DW_I2S_VENDOR_AMD (1 << 0)
>> + unsigned int quirks;
>
> I would expect to see a set of quirks with the AMD devices selecting
> those that apply to them rather than just a per vendor define - this
> is more generic and more maintainable long term. AMD may require
> different sets of quirks with some future device and systems from other
> vendors may require some but not all of the quirks that AMD requires.
Agreed. I will send a revised patch.
>
>> + if (dev->quirks & DW_I2S_VENDOR_AMD) {
>> + comp1 = i2s_read_reg(dev->i2s_pbase, 0x124);
>> + comp2 = i2s_read_reg(dev->i2s_pbase, 0x120);
>
> Please add defines for these registers. Rather than having the quirk
> code throughout the driver it might be clearer to have the comp1 and
> comp2 register offsets stored in the driver data so you can just do the
> actual quirk once at probe time.
Agreed. I will send a revised patch.
>
> ___
> Alsa-devel mailing list
> Alsa-devel at alsa-project.org
> http://mailman.alsa-project.org/mailman/listinfo/alsa-devel
>
[alsa-devel] [PATCH 8/8] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Thu, Oct 22, 2015 at 9:44 PM, Mark Brown 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_
[alsa-devel] [PATCH 8/8] ASoC: AMD: add AMD ASoC ACP-I2S driver
On Sat, Oct 24, 2015 at 1:01 AM, Mark Brown 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 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 0x0400. 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.
