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=-2.3 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 autolearn=no 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 C51F5C432C0 for ; Tue, 19 Nov 2019 14:04:33 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 86159222D3 for ; Tue, 19 Nov 2019 14:04:33 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=amdcloud.onmicrosoft.com header.i=@amdcloud.onmicrosoft.com header.b="YP6vYJCf" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727897AbfKSOEc (ORCPT ); Tue, 19 Nov 2019 09:04:32 -0500 Received: from mail-eopbgr720067.outbound.protection.outlook.com ([40.107.72.67]:44202 "EHLO NAM05-CO1-obe.outbound.protection.outlook.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1726378AbfKSOEc (ORCPT ); Tue, 19 Nov 2019 09:04:32 -0500 ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=MDGPsztHHG5e5+uWfYoVmnXytrS7nK+/2v+9v7HYoxOIFGECKYId4gfrivbgSlj2ppLtBXCTS43V6bO9Z3IzaZvf1jpn5IY03IWqguyf6x4C7eUxmKvxjCGg+gJLrBuXp6iQViaCLwrXdozq4qA2XxsALLCs8/1es7ikHg/AZ/MZblleufNWh5btENMRIajxEaNRW9SencKwGg0Ib7buPwam/zTRr7UtJgbyJxRttplomfGNIAmWGujoNxLT7JfHZsz53ck2oEMZi3C/VUnrpZz2FTbF7yMusUAtbWNK2E8MLnwcNA8Pujin0h+kV6wEl+f6W96uz8pK5oC64h158g== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=fTv4PfCiuPOLkulRAfJSxPVE7d3Bu4RRkM41LmjZFCk=; b=OI3gWJ/ViLvEdXcm6tZh0yoqnTHX/e4MX0eBZHsJ/rk7d8jJ6d4p4FzIahZ6Tx0cuCxnf/1zON4082csWRlgkoEuLkvYiIPxC5Ss404oSwDSb+xOXkleBQo9480Rw4lGU8ASwyeWx7jhGe4RWeJJjOoeVCqm30pWqk8NxDAnjHDAyplr9vrcuPmNY3tTYVOVQUxC1CBh+4hK4IvIuPUBlgK16ilnxlhG0z+XPrToCEfyr/89Y3oRDyB3aEl7uAmzOCuoz/qmt777DtWGUFzLuEIF/+dYFqZUqtEPtmUWJriMproozLrVDgb6Nsg19TWpYX5C4uogXgw6hFqlezccyA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=amd.com; dmarc=pass action=none header.from=amd.com; dkim=pass header.d=amd.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=amdcloud.onmicrosoft.com; s=selector2-amdcloud-onmicrosoft-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=fTv4PfCiuPOLkulRAfJSxPVE7d3Bu4RRkM41LmjZFCk=; b=YP6vYJCfJBp6Cn20N8w54n0Dq8C6ek8SJN1feGMfQuzFoKT3sFwWbnaotVTsPxoDOE7NYfet5b2e5frHqKB7RZsXqVqzAEzG9hB+ngHOpL7Kn8IDwIuzfkhum/Xkqt3jYTylfj8Rnvw7Y1fLmnckcwmf+Y83IkT/I/bDvgVfE04= Authentication-Results: spf=none (sender IP is ) smtp.mailfrom=Vishnuvardhanrao.Ravulapati@amd.com; Received: from CH2PR12MB3862.namprd12.prod.outlook.com (52.132.231.219) by CH2PR12MB3990.namprd12.prod.outlook.com (52.132.245.82) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2474.16; Tue, 19 Nov 2019 14:04:25 +0000 Received: from CH2PR12MB3862.namprd12.prod.outlook.com ([fe80::3d47:8737:20c4:1834]) by CH2PR12MB3862.namprd12.prod.outlook.com ([fe80::3d47:8737:20c4:1834%7]) with mapi id 15.20.2451.029; Tue, 19 Nov 2019 14:04:25 +0000 Subject: Re: [RESEND PATCH v9 6/6] ASoC: amd: Added ACP3x system resume and runtime pm From: vishnu To: Dan Carpenter , Ravulapati Vishnu vardhan rao Cc: Alexander.Deucher@amd.com, djkurtz@google.com, Akshu.Agrawal@amd.com, Liam Girdwood , Mark Brown , Jaroslav Kysela , Takashi Iwai , Vijendar Mukunda , YueHaibing , "Gustavo A. R. Silva" , Kuninori Morimoto , "moderated list:SOUND - SOC LAYER / DYNAMIC AUDIO POWER MANAGEM..." , open list References: <1574165476-24987-1-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> <1574165476-24987-7-git-send-email-Vishnuvardhanrao.Ravulapati@amd.com> <20191119123531.GA30789@kadam> <3321478e-de8f-2eb6-6e6f-6eb621b8434b@amd.com> Message-ID: Date: Tue, 19 Nov 2019 19:33:08 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.2.1 In-Reply-To: <3321478e-de8f-2eb6-6e6f-6eb621b8434b@amd.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 8bit X-ClientProxiedBy: PN1PR01CA0071.INDPRD01.PROD.OUTLOOK.COM (2603:1096:c00:1::11) To CH2PR12MB3862.namprd12.prod.outlook.com (2603:10b6:610:21::27) MIME-Version: 1.0 X-Originating-IP: [165.204.159.251] X-MS-PublicTrafficType: Email X-MS-Office365-Filtering-HT: Tenant X-MS-Office365-Filtering-Correlation-Id: d099d246-4023-46dc-a210-08d76cf9627e X-MS-TrafficTypeDiagnostic: CH2PR12MB3990:|CH2PR12MB3990: X-MS-Exchange-Transport-Forked: True X-Microsoft-Antispam-PRVS: X-MS-Oob-TLC-OOBClassifiers: OLM:5236; X-Forefront-PRVS: 022649CC2C X-Forefront-Antispam-Report: SFV:NSPM;SFS:(10009020)(4636009)(136003)(39860400002)(396003)(376002)(366004)(346002)(23433003)(199004)(189003)(186003)(58126008)(305945005)(110136005)(26005)(3846002)(11346002)(6506007)(386003)(6486002)(53546011)(446003)(23676004)(7736002)(36756003)(31696002)(316002)(6666004)(6436002)(52116002)(81156014)(5660300002)(6116002)(2486003)(2906002)(76176011)(66556008)(50466002)(66476007)(54906003)(66946007)(7416002)(229853002)(14444005)(4326008)(8936002)(47776003)(6512007)(14454004)(6246003)(65956001)(66066001)(81166006)(486006)(99286004)(2616005)(476003)(8676002)(65806001)(478600001)(6636002)(2870700001)(25786009)(31686004)(43062003)(32563001);DIR:OUT;SFP:1101;SCL:1;SRVR:CH2PR12MB3990;H:CH2PR12MB3862.namprd12.prod.outlook.com;FPR:;SPF:None;LANG:en;PTR:InfoNoRecords;A:1;MX:1; Received-SPF: None (protection.outlook.com: amd.com does not designate permitted sender hosts) X-MS-Exchange-SenderADCheck: 1 X-Microsoft-Antispam: BCL:0; X-Microsoft-Antispam-Message-Info: jgm92UTAKoFU+hjbmYadDb01XfZv7ApTIh4MiQ49Pr7HIDh7VzWD0nUD7kka84puNdJXz+vebooRF9QrXmuoEKAftRYEXZofH+5c+RUkf9ZaKlQAzfn9den3QS0VLjn4TkaTTSt8cBTtaraaO57iCbCssSkm8m9kB5VLivsaQUnUy8A1ST4dY1wsYEHjMwqL+gaPYUyd8N9WQXwXIXaZQLG0/lS477jvg64uRZA601Rf77A1yJBikQFxweDX7o/X8h8fJ9+WSObqCF9FGWUgmMKB4wJcj/229Ua3w6wBboiEYMI1KGQVUkJoNK9XKlm+jqCM7+pKKvNUUWNBwA7bGDdBdii1V6eUDNXYch9QCkfwlVPyZM94JABM3B2rHS6UTJbbPyurfOi9Wxico807A6yC7zBVBUkXscFzhfvQlu5JwLAUoKPIUGgAsoAtFi+h X-OriginatorOrg: amd.com X-MS-Exchange-CrossTenant-Network-Message-Id: d099d246-4023-46dc-a210-08d76cf9627e X-MS-Exchange-CrossTenant-OriginalArrivalTime: 19 Nov 2019 14:04:25.3316 (UTC) X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d X-MS-Exchange-CrossTenant-MailboxType: HOSTED X-MS-Exchange-CrossTenant-UserPrincipalName: TwLHRIR7abvSHGE40n2Ym8/LWvjSMuNjaYcyJtToVXFNXWNVIzTmZT/mf+V0TlTn1g7DvtGtwrNgHp39qdH2gg== X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH2PR12MB3990 Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 19/11/19 6:26 PM, vishnu wrote: > > > On 19/11/19 6:05 PM, Dan Carpenter wrote: >> I can't apply this because I'm not CC'd on patches 2-5. >> >> On Tue, Nov 19, 2019 at 05:41:16PM +0530, Ravulapati Vishnu vardhan >> rao wrote: >>> +static int acp3x_power_on(void __iomem *acp3x_base) >>> +{ >>> +    u32 val; >>> +    u32 timeout; >>> + >>> +    timeout = 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) { >> >> while (++timeout < 500) >> > > If I check with timeout<500 and in next condition i have > if(timeout >500) this never happens. > > Our intention is to wait for time out and exit. > >>> +        val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); >>                     ^^ >> Extra space character. >> >> >>> +        if (!val) >>> +            break; >> >> return 0; >> >>> +        udelay(1); >>> +        if (timeout > 500) { >>> +            pr_err("ACP is Not Powered ON\n"); >>> +            return -ETIMEDOUT; >>> +        } >>> +    } >>> +    return 0; >> >> Since we combined the ++timeout and the < 500 this becomes >> "return -ETIMEOUT;" here. >> >>> +} >>> + >>> +static int acp3x_power_off(void __iomem *acp3x_base) >>> +{ >>> +    u32 val; >>> +    u32 timeout, ret; >> >> Both ret and timeout should just be int.  Please update this throughout. >> >>> + >>> +    timeout = 0; >> >> Move the timeout = 0 next to the loop or put it in the initializer. >> >>> +    rv_writel(ACP_PGFSM_CNTL_POWER_OFF_MASK, >>> +            acp3x_base + mmACP_PGFSM_CONTROL); >>> +    while (++timeout) { >> >> while (++timeout < 500) { >> >>> +        val  = rv_readl(acp3x_base + mmACP_PGFSM_STATUS); >> >> Extra space char. >> >>> +        if ((val & ACP_PGFSM_STATUS_MASK) == ACP_POWERED_OFF) { >>> +            ret = 0; >>> +            break; >> >> return 0; >> >>> +        } >>> +        udelay(1); >>> +        if (timeout > 500) { >>> +            pr_err("ACP is Not Powered OFF\n"); >>> +            ret = -ETIMEDOUT; >>> +            break; >>> +        } >>> +    } >>> +    return ret; >>> +} >>> + >>> +static int acp3x_reset(void __iomem *acp3x_base) >>> +{ >>> +    u32 val, timeout; >>> + >>> +    rv_writel(1, acp3x_base + mmACP_SOFT_RESET); >>> +    timeout = 0; >>> +    while (++timeout) { >>> +        val = rv_readl(acp3x_base + mmACP_SOFT_RESET); >>> +        if ((val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) || >>> +                            timeout > 100) { >> >> This timeout > 100 limit was difficult to spot.  Like finding Waldo. >> >>> +            if (val & ACP3x_SOFT_RESET__SoftResetAudDone_MASK) >>> +                break; >> >> This is a duplicate condition. >> >>> +            return -ENODEV; >>> +        } >>> +        cpu_relax(); >>> +    } >>> +    rv_writel(0, acp3x_base + mmACP_SOFT_RESET); >>> +    timeout = 0; >>> +    while (++timeout) { >>> +        val = rv_readl(acp3x_base + mmACP_SOFT_RESET); >>> +        if (!val) >>> +            break; >>> +        if (timeout > 100) >>> +            return -ENODEV; >>> +        cpu_relax(); >>> +    } >>> +    return 0; >>> +} >>> + >>> +static int acp3x_init(void __iomem *acp3x_base) >>> +{ >>> +    int ret; >>> + >>> +    /* power on */ >>> +    ret = acp3x_power_on(acp3x_base); >>> +    if (ret) { >>> +        pr_err("ACP3x power on failed\n"); >>> +        return ret; >>> +    } >>> +    /* Reset */ >>> +    ret = acp3x_reset(acp3x_base); >>> +    if (ret) { >>> +        pr_err("ACP3x reset failed\n"); >>> +        return ret; >>> +    } >>> +    return 0; >>> +} >>> + >>> +static int acp3x_deinit(void __iomem *acp3x_base) >>> +{ >>> +    int ret; >>> + >>> +    /* Reset */ >>> +    ret = acp3x_reset(acp3x_base); >>> +    if (ret) { >>> +        pr_err("ACP3x reset failed\n"); >>> +        return ret; >>> +    } >>> +    /* power off */ >>> +    ret = acp3x_power_off(acp3x_base); >>> +    if (ret) { >>> +        pr_err("ACP3x power off failed\n"); >>> +        return ret; >>> +    } >>> +    return 0; >>> +} >>> + >>>   static int snd_acp3x_probe(struct pci_dev *pci, >>>                  const struct pci_device_id *pci_id) >>>   { >>> @@ -64,6 +186,9 @@ static int snd_acp3x_probe(struct pci_dev *pci, >>>       } >>>       pci_set_master(pci); >>>       pci_set_drvdata(pci, adata); >>> +    ret = acp3x_init(adata->acp3x_base); >>> +    if (ret) >>> +        goto disable_msi; >>>       val = rv_readl(adata->acp3x_base + mmACP_I2S_PIN_CONFIG); >>>       switch (val) { >>> @@ -73,7 +198,7 @@ static int snd_acp3x_probe(struct pci_dev *pci, >>>                         GFP_KERNEL); >>>           if (!adata->res) { >>>               ret = -ENOMEM; >>> -            goto disable_msi; >>> +            goto de_init; >>>           } >>>           adata->res[0].name = "acp3x_i2s_iomem"; >>> @@ -134,12 +259,23 @@ static int snd_acp3x_probe(struct pci_dev *pci, >>>           ret = -ENODEV; >>>           goto disable_msi; >>>       } >>> +    pm_runtime_set_autosuspend_delay(&pci->dev, 5000); >>> +    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; >>>   unregister_devs: >>>       if (val == I2S_MODE) >>>           for (i = 0 ; i < ACP3x_DEVS ; i++) >>>               platform_device_unregister(adata->pdev[i]); >>> +de_init: >>> +    ret = acp3x_deinit(adata->acp3x_base); >>> +    if (ret) >>> +        dev_err(&pci->dev, "ACP de-init failed\n"); >>> +    else >>> +        dev_dbg(&pci->dev, "ACP de-initialized\n"); >> >> >> We can't overwrite ret (probe failed even if deinit() succeeded).  I >> dont' know that the debug printk is useful. >> >> de_init: >>     if (acp3x_deinit(adata->acp3x_base)) >>         dev_err(&pci->dev, "ACP de-init failed in probe error >> handling\n"); >> >> >>>   disable_msi: >>>       pci_disable_msi(pci); >>>   release_regions: >>> @@ -150,15 +286,58 @@ static int snd_acp3x_probe(struct pci_dev *pci, >>>       return ret; >>>   } >>> +static int  snd_acp3x_suspend(struct device *dev) >>               ^^ >> Extra space char >> >>> +{ >>> +    int status; >> >> int ret; >> >>> +    struct acp3x_dev_data *adata; >>> + >>> +    adata = dev_get_drvdata(dev); >>> +    status = acp3x_deinit(adata->acp3x_base); >>> +    if (status) >>> +        dev_err(dev, "ACP de-init failed\n"); >>> +    else >>> +        dev_dbg(dev, "ACP de-initialized\n"); >>> + >>> +    return 0; >>> +} >>> + >>> +static int  snd_acp3x_resume(struct device *dev) >>               ^^ >> Extra space >> >>> +{ >>> +    int status; >>> +    struct acp3x_dev_data *adata; >>> + >>> +    adata = dev_get_drvdata(dev); >>> +    status = acp3x_init(adata->acp3x_base); >>> +    if (status) { >>> +        dev_err(dev, "ACP init failed\n"); >>> +        return status; >>> +    } >>> +    return 0; >>> +} >>> + >>> +static const struct dev_pm_ops acp3x_pm = { >>> +    .runtime_suspend = snd_acp3x_suspend, >>> +    .runtime_resume =  snd_acp3x_resume, >>> +    .resume =       snd_acp3x_resume, >> >> Fix whitespace. >> >>> +}; >>> + >>>   static void snd_acp3x_remove(struct pci_dev *pci) >>>   { >>> -    struct acp3x_dev_data *adata = pci_get_drvdata(pci); >> >> This was fine.  Leave it as-is. >> Actually I was reported by kbuild robot tool about ISO mixed forbids of initialization so I did this. >>> -    int i; >>> +    struct acp3x_dev_data *adata; >>> +    int i, ret; >>> +    adata = pci_get_drvdata(pci); >>>       if (adata->acp3x_audio_mode == ACP3x_I2S_MODE) { >>>           for (i = 0 ; i <  ACP3x_DEVS ; i++) >>                                  ^^ >> There is an extra space char here as well.  I guess I missed it when I >> reviewed patch 1. >> >>>               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_dbg(&pci->dev, "ACP de-initialized\n"); >> >> Put the printk in acp3x_deinit() itself and remove it from all the >> callers. >> >> regards, >> dan carpenter >>