Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <862dc646-4b80-4c6f-1e0f-99a498068f84@ti.com>
Date:   Wed, 13 Apr 2022 17:37:18 +0530
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.7.0
Subject: Re: [PATCH RFC v2] mmc: sdhci_am654: Add support for PM
 suspend/resume
Content-Language: en-US
To:     Ulf Hansson <ulf.hansson@linaro.org>
CC:     Vignesh Raghavendra <vigneshr@ti.com>,
        Kishon Vijay Abraham I <kishon@ti.com>,
        Adrian Hunter <adrian.hunter@intel.com>,
        <linux-mmc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
References: <20220408124338.27090-1-a-govindraju@ti.com>
 <CAPDyKFqy6sEtU57-QkZeCfMtJgcoHG8siRBieOUjkBZMfPge9Q@mail.gmail.com>
From:   Aswath Govindraju <a-govindraju@ti.com>
In-Reply-To: <CAPDyKFqy6sEtU57-QkZeCfMtJgcoHG8siRBieOUjkBZMfPge9Q@mail.gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk

Hi Uffe,

On 12/04/22 20:08, Ulf Hansson wrote:
> On Fri, 8 Apr 2022 at 14:43, Aswath Govindraju <a-govindraju@ti.com> wrote:
>>
>> Add support for suspend/resume and pm_runtime resume/suspend.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@ti.com>
>> ---
>>
>> Changes since v1:
>> - Removed System pm calls and used force_runtime_pm calls instead
>> - Added error handling at required places
>> - Added pm_runtime_sync call in the sdhci_am654_remove function
>> - Replaced pm_runtime_*_sync calls in probe with noidle and noresume
>>   calls
>> - Used MACRO SET_RUNTIME_PM_OPS to avoid build errors when CONFIG_PM
>>   is disabled
>>
>>  drivers/mmc/host/sdhci_am654.c | 146 +++++++++++++++++++++++++++++----
>>  1 file changed, 132 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index e54fe24d47e7..131b2b9ae0e7 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -84,6 +84,7 @@
>>  #define DRIVER_STRENGTH_40_OHM 0x4
>>
>>  #define CLOCK_TOO_SLOW_HZ      50000000
>> +#define SDHCI_AM654_AUTOSUSPEND_DELAY  100
>>
>>  /* Command Queue Host Controller Interface Base address */
>>  #define SDHCI_AM654_CQE_BASE_ADDR 0x200
>> @@ -791,16 +792,10 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>
>>         pltfm_host->clk = clk_xin;
>>
>> -       /* Clocks are enabled using pm_runtime */
>> -       pm_runtime_enable(dev);
>> -       ret = pm_runtime_resume_and_get(dev);
>> -       if (ret)
>> -               goto pm_runtime_disable;
>> -
>>         base = devm_platform_ioremap_resource(pdev, 1);
>>         if (IS_ERR(base)) {
>>                 ret = PTR_ERR(base);
>> -               goto pm_runtime_put;
>> +               goto err_pltfm_free;
>>         }
>>
>>         sdhci_am654->base = devm_regmap_init_mmio(dev, base,
>> @@ -808,31 +803,47 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>         if (IS_ERR(sdhci_am654->base)) {
>>                 dev_err(dev, "Failed to initialize regmap\n");
>>                 ret = PTR_ERR(sdhci_am654->base);
>> -               goto pm_runtime_put;
>> +               goto err_pltfm_free;
>>         }
>>
>>         ret = sdhci_am654_get_of_property(pdev, sdhci_am654);
>>         if (ret)
>> -               goto pm_runtime_put;
>> +               goto err_pltfm_free;
>>
>>         ret = mmc_of_parse(host->mmc);
>>         if (ret) {
>>                 dev_err(dev, "parsing dt failed (%d)\n", ret);
>> -               goto pm_runtime_put;
>> +               goto err_pltfm_free;
>>         }
>>
>>         host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>
>> +       pm_runtime_get_noresume(dev);
>> +       ret = pm_runtime_set_active(dev);
>> +       if (ret)
>> +               goto pm_put;
>> +       pm_runtime_enable(dev);
>> +       ret = clk_prepare_enable(pltfm_host->clk);
>> +       if (ret)
>> +               goto pm_disable;
>> +
>>         ret = sdhci_am654_init(host);
>>         if (ret)
>> -               goto pm_runtime_put;
>> +               goto clk_disable;
>>
>> +       /* Setting up autosuspend */
>> +       pm_runtime_set_autosuspend_delay(dev, SDHCI_AM654_AUTOSUSPEND_DELAY);
>> +       pm_runtime_use_autosuspend(dev);
>> +       pm_runtime_mark_last_busy(dev);
>> +       pm_runtime_put_autosuspend(dev);
>>         return 0;
>>
>> -pm_runtime_put:
>> -       pm_runtime_put_sync(dev);
>> -pm_runtime_disable:
>> +clk_disable:
>> +       clk_disable_unprepare(pltfm_host->clk);
>> +pm_disable:
>>         pm_runtime_disable(dev);
>> +pm_put:
>> +       pm_runtime_put_noidle(dev);
>>  err_pltfm_free:
>>         sdhci_pltfm_free(pdev);
>>         return ret;
>> @@ -841,23 +852,130 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>  static int sdhci_am654_remove(struct platform_device *pdev)
>>  {
>>         struct sdhci_host *host = platform_get_drvdata(pdev);
>> +       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>         int ret;
>>
>> +       ret = pm_runtime_get_sync(&pdev->dev);
> 
> pm_runtime_resume_and_get() is better suited here, as it helps us to
> restore the usage count in the error path.
> >> +       if (ret < 0)
>> +               return ret;
>> +
>>         sdhci_remove_host(host, true);
>>         ret = pm_runtime_put_sync(&pdev->dev);
> 
> We need to drop the above call to pm_runtime_put_sync(). Otherwise you
> may potentially end up gating the clock, in which case the below call
> to clk_disable_unprepare() becomes wrong.
> >>         if (ret < 0)
>>                 return ret;
>>
>> +       clk_disable_unprepare(pltfm_host->clk);
>>         pm_runtime_disable(&pdev->dev);
> 
> You need a call to pm_runtime_put_noidle() here to restore the RPM
> usage count to zero.
> 
>>         sdhci_pltfm_free(pdev);
>> +       return 0;
>> +}
> 
> [...]
> 
> Other than the rather minor things above, this looks good to me!
> 

Thank you for the review.

Regards,
Aswath

> Kind regards
> Uffe