From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-5.2 required=3.0 tests=HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id A0CE8C432C3 for ; Wed, 13 Nov 2019 17:18:18 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id EF658206EC for ; Wed, 13 Nov 2019 17:18:17 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728715AbfKMRSR (ORCPT ); Wed, 13 Nov 2019 12:18:17 -0500 Received: from mga02.intel.com ([134.134.136.20]:62542 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727729AbfKMRSP (ORCPT ); Wed, 13 Nov 2019 12:18:15 -0500 X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from fmsmga004.fm.intel.com ([10.253.24.48]) by orsmga101.jf.intel.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 13 Nov 2019 09:18:15 -0800 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.68,301,1569308400"; d="scan'208";a="229813256" Received: from dmsnyder-mobl1.amr.corp.intel.com (HELO [10.252.193.15]) ([10.252.193.15]) by fmsmga004.fm.intel.com with ESMTP; 13 Nov 2019 09:18:13 -0800 Subject: Re: [alsa-devel] [RESEND PATCH v5 6/6] ASoC: amd: Added ACP3x system resume and runtime pm To: Ravulapati Vishnu vardhan rao Cc: "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , Maruthi Bayyavarapu , Kuninori Morimoto , open list , Takashi Iwai , YueHaibing , Liam Girdwood , Akshu.Agrawal@amd.com, Sanju R Mehta , Mark Brown , djkurtz@google.com, Vijendar Mukunda , Alexander.Deucher@amd.com, Colin Ian King , Dan Carpenter References: <1573629249-13272-1-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> <1573629249-13272-7-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> From: Pierre-Louis Bossart Message-ID: <7161a222-8e88-704b-0a3e-a468a2639e4f@linux.intel.com> Date: Wed, 13 Nov 2019 10:39:53 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.1.2 MIME-Version: 1.0 In-Reply-To: <1573629249-13272-7-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > diff --git a/sound/soc/amd/raven/acp3x.h b/sound/soc/amd/raven/acp3x.h > index 01b283a..c40f960 100644 > --- a/sound/soc/amd/raven/acp3x.h > +++ b/sound/soc/amd/raven/acp3x.h > @@ -7,6 +7,7 @@ > > #include "chip_offset_byte.h" > > +#define DELAY 600 you probably want a prefix here to avoid conflicts > +static int acp3x_power_on(void __iomem *acp3x_base) > +{ > + u32 val; > + u32 timeout = 0; > + int ret = 0; > + > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + > + if (val == 0) > + return val; > + > + if (!((val & ACP_PGFSM_STATUS_MASK) == > + ACP_POWER_ON_IN_PROGRESS)) > + rv_writel(ACP_PGFSM_CNTL_POWER_ON_MASK, > + acp3x_base + mmACP_PGFSM_CONTROL); > + while (++timeout < DELAY) { so here timeout can reach 600. > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if (!val) > + break; > + udelay(1); > + if (timeout > 500) { but here you abort at 500. Looks like the first test is not needed? > + pr_err("ACP is Not Powered ON\n"); > + return -ETIMEDOUT; > + } > + } > +} > +static int acp3x_power_off(void __iomem *acp3x_base) > +{ > + u32 val; > + u32 timeout = 0; > + > + rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK, > + acp3x_base + mmACP_PGFSM_CONTROL); > + while (++timeout < DELAY) { same here > + val = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); > + if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF) > + return 0; > + udelay(1); > + if (timeout > 500) { > + pr_err("ACP is Not Powered OFF\n"); > + return -ETIMEDOUT; > + } > + } > +} > +static int acp3x_reset(void __iomem *acp3x_base) > +{ > + u32 val, timeout; > + > + rv_writel(1, acp3x_base + mmACP_SOFT_RESET); > + timeout = 0; > + while (++timeout < DELAY) { > + val = rv_readl(acp3x_base + mmACP_SOFT_RESET); > + if ((val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) || > + timeout > 100) { and here > + if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) > + break; > + return -ENODEV; > + } > + cpu_relax(); > + } > + rv_writel(0, acp3x_base + mmACP_SOFT_RESET); > + timeout = 0; > + while (++timeout < DELAY) { > + val = rv_readl(acp3x_base + mmACP_SOFT_RESET); > + if (!val) > + break; > + if (timeout > 100) and here. > @@ -133,9 +248,19 @@ static int snd_acp3x_probe(struct pci_dev *pci, > ret = -ENODEV; > goto unmap_mmio; > } > + pm_runtime_set_autosuspend_delay(&pci->dev, 10000); I think it's the largest value I've ever seen for autosuspend. It is necessary? If yes you should document why. > + pm_runtime_use_autosuspend(&pci->dev); > + pm_runtime_set_active(&pci->dev); > + pm_runtime_put_noidle(&pci->dev); > + pm_runtime_enable(&pci->dev); > return 0; > > unmap_mmio: > + ret = acp3x_deinit(adata->acp3x_base); > + if (ret) > + dev_err(&pci->dev, "ACP de-init failed\n"); > + else > + dev_info(&pci->dev, "ACP de-initialized\n"); dev_dbg? > pci_disable_msi(pci); > for (i = 0 ; i < ACP3x_DEVS ; i++) > platform_device_unregister(adata->pdev[i]); > @@ -148,23 +273,57 @@ static int snd_acp3x_probe(struct pci_dev *pci, > > return ret; > } > +static int snd_acp3x_suspend(struct device *dev) > +{ > + int status; > + struct acp3x_dev_data *adata = dev_get_drvdata(dev); > + > + status = acp3x_deinit(adata->acp3x_base); > + if (status) > + dev_err(dev, "ACP de-init failed\n"); > + else > + dev_info(dev, "ACP de-initialized\n"); dev_dbg? > static void snd_acp3x_remove(struct pci_dev *pci) > { > - int i; > + int i, ret; > struct acp3x_dev_data *adata = pci_get_drvdata(pci); > > if (adata->acp3x_audio_mode == ACP3x_I2S_MODE) { > for (i = 0 ; i < ACP3x_DEVS ; i++) > platform_device_unregister(adata->pdev[i]); > } > + ret = acp3x_deinit(adata->acp3x_base); > + if (ret) > + dev_err(&pci->dev, "ACP de-init failed\n"); > + else > + dev_info(&pci->dev, "ACP de-initialized\n"); dev_dbg?