| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <9029896f-d6fb-e341-9544-1104ecfa9f8a@ti.com>
Date: Mon, 3 Apr 2023 23:12:41 +0530
From: Vignesh Raghavendra <vigneshr@...com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Adrian Hunter <adrian.hunter@...el.com>,
<linux-mmc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3] mmc: sdhci_am654: Add support for PM suspend/resume
Hi,
On 31/03/23 20:01, Ulf Hansson wrote:
> On Fri, 31 Mar 2023 at 12:17, Vignesh Raghavendra <vigneshr@...com> wrote:
>>
>> From: Aswath Govindraju <a-govindraju@...com>
>>
>> Add support for suspend/resume and pm_runtime resume/suspend.
>>
>> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
>> Signed-off-by: Georgi Vlaev <g-vlaev@...com>
>> Signed-off-by: Vignesh Raghavendra <vigneshr@...com>
>> ---
>>
>> Since RFC v2:
>> Address all comments around sdhci_am654_remove()
>> Set autosuspend_delay to -1 as SDHCI will host rootfs via SD/eMMC and
>> autosuspend can cause long latency for user interactive applications
>
> I am curious about the "long" latency. Do you have some data that you
> can share for how long it takes to runtime-resume the device?
>
As part of wider teting, I discovered that on certain boards (J721e EVM
for example), there is a latency of ~500ms for controller to start up
(HW limitation around card detect debounce timer [0]). This results in
momentary (but noticeable) lag when playing a video for example.
[0[
https://elixir.bootlin.com/linux/latest/source/drivers/mmc/host/sdhci_am654.c#L361
> Using -1 as the default value for the autosuspend_delay seems fine to
> me, but did you consider using a longer timeout than the common 100ms?
> Could that perhaps be a better default option?
Its more of the startup delay than how long controller should remain
active. So tuning autosuspend_delay does not seem to help.
>
> Of course, in the end this is a choice of performance in favor of
> wasting energy.
Yes, this is choice of opt-in vs opt-out. Users can tune the delay via
sysfs as needed.
I am setting it to -1 to avoid regressing any userspace apps (vs
enabling autosuspend and then asking impacted users to set to -1)
One solution is to use -1 on platforms where latency is significant
based on compatible / SoC detection?
Regards
Vignesh
>
> Kind regards
> Uffe
>
>>
>> RFC v2:
>> https://lore.kernel.org/all/20220408124338.27090-1-a-govindraju@ti.com/
>>
>> drivers/mmc/host/sdhci_am654.c | 147 +++++++++++++++++++++++++++++----
>> 1 file changed, 131 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 672d37ea98d0..7cdf0f54e3a5 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -85,6 +85,7 @@
>> #define DRIVER_STRENGTH_40_OHM 0x4
>>
>> #define CLOCK_TOO_SLOW_HZ 50000000
>> +#define SDHCI_AM654_AUTOSUSPEND_DELAY -1
>>
>> /* Command Queue Host Controller Interface Base address */
>> #define SDHCI_AM654_CQE_BASE_ADDR 0x200
>> @@ -808,16 +809,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,
>> @@ -825,31 +820,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_probe(dev, ret, "parsing dt failed\n");
>> - 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;
>> @@ -858,23 +869,127 @@ 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;
>>
>> - sdhci_remove_host(host, true);
>> - ret = pm_runtime_put_sync(&pdev->dev);
>> + ret = pm_runtime_resume_and_get(&pdev->dev);
>> if (ret < 0)
>> return ret;
>>
>> + sdhci_remove_host(host, true);
>> + clk_disable_unprepare(pltfm_host->clk);
>> pm_runtime_disable(&pdev->dev);
>> + pm_runtime_put_noidle(&pdev->dev);
>> sdhci_pltfm_free(pdev);
>> + return 0;
>> +}
>> +
>> +#ifdef CONFIG_PM
>> +static int sdhci_am654_restore(struct sdhci_host *host)
>> +{
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>> + u32 ctl_cfg_2 = 0;
>> + u32 val;
>> + int ret;
>> +
>> + if (sdhci_am654->flags & DLL_CALIB) {
>> + regmap_read(sdhci_am654->base, PHY_STAT1, &val);
>> + if (~val & CALDONE_MASK) {
>> + /* Calibrate IO lines */
>> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>> + PDB_MASK, PDB_MASK);
>> + ret = regmap_read_poll_timeout(sdhci_am654->base,
>> + PHY_STAT1, val,
>> + val & CALDONE_MASK,
>> + 1, 20);
>> + if (ret)
>> + return ret;
>> + }
>> + }
>> +
>> + /* Enable pins by setting IO mux to 0 */
>> + if (sdhci_am654->flags & IOMUX_PRESENT)
>> + regmap_update_bits(sdhci_am654->base, PHY_CTRL1,
>> + IOMUX_ENABLE_MASK, 0);
>>
>> + /* Set slot type based on SD or eMMC */
>> + if (host->mmc->caps & MMC_CAP_NONREMOVABLE)
>> + ctl_cfg_2 = SLOTTYPE_EMBEDDED;
>> +
>> + regmap_update_bits(sdhci_am654->base, CTL_CFG_2, SLOTTYPE_MASK,
>> + ctl_cfg_2);
>> +
>> + regmap_read(sdhci_am654->base, CTL_CFG_3, &val);
>> + if (~val & TUNINGFORSDR50_MASK)
>> + /* Enable tuning for SDR50 */
>> + regmap_update_bits(sdhci_am654->base, CTL_CFG_3, TUNINGFORSDR50_MASK,
>> + TUNINGFORSDR50_MASK);
>> +
>> + return 0;
>> +}
>> +
>> +static int sdhci_am654_runtime_suspend(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + int ret;
>> +
>> + if (host->tuning_mode != SDHCI_TUNING_MODE_3)
>> + mmc_retune_needed(host->mmc);
>> +
>> + ret = cqhci_suspend(host->mmc);
>> + if (ret)
>> + return ret;
>> +
>> + ret = sdhci_runtime_suspend_host(host);
>> + if (ret)
>> + return ret;
>> +
>> + /* disable the clock */
>> + clk_disable_unprepare(pltfm_host->clk);
>> return 0;
>> }
>>
>> +static int sdhci_am654_runtime_resume(struct device *dev)
>> +{
>> + struct sdhci_host *host = dev_get_drvdata(dev);
>> + struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> + int ret;
>> +
>> + /* Enable the clock */
>> + ret = clk_prepare_enable(pltfm_host->clk);
>> + if (ret)
>> + return ret;
>> +
>> + ret = sdhci_am654_restore(host);
>> + if (ret)
>> + return ret;
>> +
>> + ret = sdhci_runtime_resume_host(host, 0);
>> + if (ret)
>> + return ret;
>> +
>> + ret = cqhci_resume(host->mmc);
>> + if (ret)
>> + return ret;
>> +
>> + return 0;
>> +}
>> +#endif
>> +
>> +static const struct dev_pm_ops sdhci_am654_dev_pm_ops = {
>> + SET_RUNTIME_PM_OPS(sdhci_am654_runtime_suspend,
>> + sdhci_am654_runtime_resume, NULL)
>> + SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
>> + pm_runtime_force_resume)
>> +};
>> +
>> static struct platform_driver sdhci_am654_driver = {
>> .driver = {
>> .name = "sdhci-am654",
>> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
>> + .pm = &sdhci_am654_dev_pm_ops,
>> .of_match_table = sdhci_am654_of_match,
>> },
>> .probe = sdhci_am654_probe,
>> --
>> 2.40.0
>>
--
Regards
Vignesh
Powered by blists - more mailing lists