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=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,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 EE6FEC4CEC7 for ; Sat, 14 Sep 2019 11:06:10 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id A5EC72053B for ; Sat, 14 Sep 2019 11:06:10 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=linaro.org header.i=@linaro.org header.b="YAVa8BZy" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728803AbfINLGJ (ORCPT ); Sat, 14 Sep 2019 07:06:09 -0400 Received: from mail-qt1-f195.google.com ([209.85.160.195]:33045 "EHLO mail-qt1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729170AbfINLGJ (ORCPT ); Sat, 14 Sep 2019 07:06:09 -0400 Received: by mail-qt1-f195.google.com with SMTP id r5so37194213qtd.0 for ; Sat, 14 Sep 2019 04:06:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=subject:to:references:cc:from:message-id:date:user-agent :mime-version:in-reply-to:content-transfer-encoding; bh=6/h10Ynvsmv4VGa6AAdcwVfIzqHMMfTN2SWUN9v/ft4=; b=YAVa8BZyz70qNb8OPb3RoGVGYDRZZFjz7+gGQWQrL32YsS6lvDhydX0Ts3SVG8gaY8 AgzDnuaIq0DJNZkBlQoQCzEdwCgfkVPBi7fA/sL9NzWXxTCzNi1wEfL0QpQ9KGtILcu2 ++B5O/64lKcryO/wB0lEHOiUjYCr3aGjKbMC4+qHCu3qJePc9OLVi3PvsFiWuo87HYLz XH3R9iVqpDGGB72O2YGCKAvat7rw1s4ri8xl3NGSVsn0iW1i+ejQs3Cu+vbfVtCZQ/ea 3yFq72h4K2D3TL1VRSlXkWPEyTblVBnRnlvjR0f9hWGO6wVZpLkHaRSYuj4HlGVV5I1G uSNg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:references:cc:from:message-id:date :user-agent:mime-version:in-reply-to:content-transfer-encoding; bh=6/h10Ynvsmv4VGa6AAdcwVfIzqHMMfTN2SWUN9v/ft4=; b=juMsIg8aDEU92G+1mW0PX0JFVDcND/xjiiZxsRpq9JbptCaBRs5m2Dj3dZbkl9g20e vyjJrym+9LnUPHKbGncpcAvC//l5okn1HYuE7YlsKUXUNGvKOlzDGt28/UOVPAT0PH4u g7I+tpY1Gi2pIQz3Us7lrafNqS2scInFz+Um6q+5Bz69NLVyf0IavGCC/qCLp8r7t8mf wp1gWnrb56KcrKPBwJp/j4fBfyGtQpPOCjm9Rrc0ZrQJ2BHmw1c8BjD3K3Wd6pakliul wnkJnDMeVb9zlo8ULzvzw2bWIg8RWvCy1zMMz959vHRyBeQPe92Q/e0aY32UAYINGbN9 jJhg== X-Gm-Message-State: APjAAAUPuOv4DAwu3imNezdxZ19Mzo40vpeq955JCAdl5D0Eo/FfJkqt lxk56JvLE+2UovBjKXWTABLLUQ== X-Google-Smtp-Source: APXvYqwkdpW7IOKNDyVstepZMTLaxaR6TiqZmaYWz81OgmensmzsmOmhpGt9Hn7w+HVRW75XI6TBYA== X-Received: by 2002:ac8:4787:: with SMTP id k7mr7799622qtq.58.1568459167960; Sat, 14 Sep 2019 04:06:07 -0700 (PDT) Received: from [192.168.1.169] (pool-71-255-246-27.washdc.fios.verizon.net. [71.255.246.27]) by smtp.gmail.com with ESMTPSA id v5sm19966877qtk.66.2019.09.14.04.06.06 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Sat, 14 Sep 2019 04:06:07 -0700 (PDT) Subject: Re: [PATCH 4/5] thermal: Add generic power domain warming device driver. To: Ulf Hansson References: <1568135676-9328-1-git-send-email-thara.gopinath@linaro.org> <1568135676-9328-5-git-send-email-thara.gopinath@linaro.org> <5D7AA7F9.1060603@linaro.org> Cc: Eduardo Valentin , Zhang Rui , Daniel Lezcano , Bjorn Andersson , Rob Herring , agross@kernel.org, amit.kucheria@verdurent.com, Mark Rutland , "Rafael J. Wysocki" , Linux PM , DTML , linux-arm-msm , Linux Kernel Mailing List From: Thara Gopinath Message-ID: <5D7CC99D.1030106@linaro.org> Date: Sat, 14 Sep 2019 07:06:05 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Sender: linux-arm-msm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On 09/13/2019 03:54 AM, Ulf Hansson wrote: > On Thu, 12 Sep 2019 at 22:18, Thara Gopinath wrote: >> >> On 09/12/2019 11:04 AM, Ulf Hansson wrote: >> >> Hi Ulf, >> >> Thanks for the review. >>> On Tue, 10 Sep 2019 at 19:14, Thara Gopinath wrote: >>>> >>>> Resources modeled as power domains in linux kenrel >>>> can be used to warm the SoC(eg. mx power domain on sdm845). >>>> To support this feature, introduce a generic power domain >>>> warming device driver that can be plugged into the thermal framework >>>> (The thermal framework itself requires further modifiction to >>>> support a warming device in place of a cooling device. >>>> Those extensions are not introduced in this patch series). >>>> >>>> Signed-off-by: Thara Gopinath >>>> --- >>>> v1->v2: >>>> - Make power domain based warming device driver a generic >>>> driver in the thermal framework. v1 implemented this as a >>>> Qualcomm specific driver. >>>> - Rename certain variables as per review suggestions on the >>>> mailing list. >>>> >>>> drivers/thermal/Kconfig | 11 +++ >>>> drivers/thermal/Makefile | 2 + >>>> drivers/thermal/pwr_domain_warming.c | 174 +++++++++++++++++++++++++++++++++++ >>>> 3 files changed, 187 insertions(+) >>>> create mode 100644 drivers/thermal/pwr_domain_warming.c >>>> >>>> diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig >>>> index 9966364..eeb6018 100644 >>>> --- a/drivers/thermal/Kconfig >>>> +++ b/drivers/thermal/Kconfig >>>> @@ -187,6 +187,17 @@ config DEVFREQ_THERMAL >>>> >>>> If you want this support, you should say Y here. >>>> >>>> +config PWR_DOMAIN_WARMING_THERMAL >>>> + bool "Power Domain based warming device" >>>> + depends on PM_GENERIC_DOMAINS >>>> + depends on PM_GENERIC_DOMAINS_OF >>> >>> PM_GENERIC_DOMAINS_OF can't be set unless PM_GENERIC_DOMAINS is set too. >>> >>> So I assume it's sufficient to depend on PM_GENERIC_DOMAINS_OF? >> >> Yes, you are right. I will change it. >>> >>>> + help >>>> + This implements the generic power domain based warming >>>> + mechanism through increasing the performance state of >>>> + a power domain. >>>> + >>>> + If you want this support, you should say Y here. >>>> + >>>> config THERMAL_EMULATION >>>> bool "Thermal emulation mode support" >>>> help >>>> diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile >>>> index 74a37c7..382c64a 100644 >>>> --- a/drivers/thermal/Makefile >>>> +++ b/drivers/thermal/Makefile >>>> @@ -27,6 +27,8 @@ thermal_sys-$(CONFIG_CLOCK_THERMAL) += clock_cooling.o >>>> # devfreq cooling >>>> thermal_sys-$(CONFIG_DEVFREQ_THERMAL) += devfreq_cooling.o >>>> >>>> +thermal_sys-$(CONFIG_PWR_DOMAIN_WARMING_THERMAL) += pwr_domain_warming.o >>>> + >>>> # platform thermal drivers >>>> obj-y += broadcom/ >>>> obj-$(CONFIG_THERMAL_MMIO) += thermal_mmio.o >>>> diff --git a/drivers/thermal/pwr_domain_warming.c b/drivers/thermal/pwr_domain_warming.c >>>> new file mode 100644 >>>> index 0000000..3dd792b >>>> --- /dev/null >>>> +++ b/drivers/thermal/pwr_domain_warming.c >>>> @@ -0,0 +1,174 @@ >>>> +// SPDX-License-Identifier: GPL-2.0 >>>> +/* >>>> + * Copyright (c) 2019, Linaro Ltd >>>> + */ >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> +#include >>>> + >>>> +struct pd_warming_device { >>>> + struct thermal_cooling_device *cdev; >>>> + struct device *dev; >>>> + int max_state; >>>> + int cur_state; >>>> + bool runtime_resumed; >>>> +}; >>>> + >>>> +static const struct of_device_id pd_wdev_match_table[] = { >>>> + { .compatible = "thermal-power-domain-wdev", .data = NULL }, >>>> + { } >>>> +}; >>>> +MODULE_DEVICE_TABLE(of, pd_wdev_match_table); >>>> + >>>> +static int pd_wdev_get_max_state(struct thermal_cooling_device *cdev, >>>> + unsigned long *state) >>>> +{ >>>> + struct pd_warming_device *pd_wdev = cdev->devdata; >>>> + >>>> + *state = pd_wdev->max_state; >>>> + return 0; >>>> +} >>>> + >>>> +static int pd_wdev_get_cur_state(struct thermal_cooling_device *cdev, >>>> + unsigned long *state) >>>> +{ >>>> + struct pd_warming_device *pd_wdev = cdev->devdata; >>>> + >>>> + *state = dev_pm_genpd_get_performance_state(pd_wdev->dev); >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int pd_wdev_set_cur_state(struct thermal_cooling_device *cdev, >>>> + unsigned long state) >>>> +{ >>>> + struct pd_warming_device *pd_wdev = cdev->devdata; >>>> + struct device *dev = pd_wdev->dev; >>>> + int ret; >>>> + >>>> + ret = dev_pm_genpd_set_performance_state(dev, state); >>>> + >>>> + if (ret) >>>> + return ret; >>>> + >>>> + if (state && !pd_wdev->runtime_resumed) { >>>> + ret = pm_runtime_get_sync(dev); >>>> + pd_wdev->runtime_resumed = true; >>>> + } else if (!state && pd_wdev->runtime_resumed) { >>>> + ret = pm_runtime_put(dev); >>>> + pd_wdev->runtime_resumed = false; >>>> + } >>>> + >>>> + return ret; >>>> +} >>>> + >>>> +static struct thermal_cooling_device_ops pd_warming_device_ops = { >>>> + .get_max_state = pd_wdev_get_max_state, >>>> + .get_cur_state = pd_wdev_get_cur_state, >>>> + .set_cur_state = pd_wdev_set_cur_state, >>>> +}; >>>> + >>>> +static int pd_wdev_create(struct device *dev, const char *name) >>>> +{ >>>> + struct pd_warming_device *pd_wdev; >>>> + int state_count; >>>> + >>>> + pd_wdev = devm_kzalloc(dev, sizeof(*pd_wdev), GFP_KERNEL); >>>> + if (!pd_wdev) >>>> + return -ENOMEM; >>>> + >>>> + state_count = dev_pm_genpd_performance_state_count(dev); >>>> + if (state_count < 0) >>>> + return state_count; >>>> + >>>> + pd_wdev->dev = dev; >>>> + pd_wdev->max_state = state_count - 1; >>>> + pd_wdev->runtime_resumed = false; >>>> + >>>> + pm_runtime_enable(dev); >>>> + >>>> + pd_wdev->cdev = thermal_of_cooling_device_register >>>> + (dev->of_node, name, >>>> + pd_wdev, >>>> + &pd_warming_device_ops); >>>> + if (IS_ERR(pd_wdev->cdev)) { >>>> + dev_err(dev, "unable to register %s cooling device\n", name); >>>> + pm_runtime_disable(dev); >>>> + >>>> + return PTR_ERR(pd_wdev->cdev); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int pd_wdev_probe(struct platform_device *pdev) >>>> +{ >>>> + struct device *dev = &pdev->dev, *pd_dev; >>>> + const char *pd_name; >>>> + int id, count, ret = 0; >>>> + >>>> + count = of_count_phandle_with_args(dev->of_node, "power-domains", >>>> + "#power-domain-cells"); >>> >>> Perhaps this should be converted to genpd OF helper function instead, >>> that allows the caller to know how many power-domains there are >>> specified for a device node. >> >> I am ok with this if you think that a OF helper to get the number of >> power domains is a useful helper in the genpd framework. I can add it as >> part of the next revision. Or do you want me to send it across separate? > > Feel free to include in the next version of the series. In case it's needed. Will do, if needed. (But as per below I am removing multiple PD support and hence this might not be needed) > >>> >>>> + >>>> + if (count > 1) { >>>> + for (id = 0; id < count; id++) { >>>> + ret = of_property_read_string_index >>>> + (dev->of_node, "power-domain-names", >>>> + id, &pd_name); >>>> + if (ret) { >>>> + dev_err(dev, "Error reading the power domain name %d\n", ret); >>>> + continue; >>>> + } >>> >>> It looks a bit awkward that you want to re-use the power-domain-names >>> as the name for the cooling (warming) device. This isn't really what >>> we use the "*-names" bindings for in general, I think. >>> >>> Anyway, if you want a name corresponding to the actual attached PM >>> domain, perhaps re-using "->name" from the struct generic_pm_domain is >>> better. We can add a genpd helper for that, no problem. Of course it >>> also means that you must call dev_pm_domain_attach_by_id() first, to >>> attach the device and then get the name of the genpd, but that should >>> be fine. >> >> Ya. I need a name corresponding to the power domain name (or something >> very close) to identify the actual warming device in the sysfs entries. >> I can use genpd->name and a helper function to achieve it. I can include >> it in Patch 1/5 where I add other helper functions. > > A separate patch please, but yeah, fold it in into @subject series. Sure! > >>> >>>> + >>>> + pd_dev = dev_pm_domain_attach_by_id(dev, id); >>>> + if (IS_ERR(pd_dev)) { >>>> + dev_err(dev, "Error attaching power domain %s %ld\n", pd_name, PTR_ERR(pd_dev)); >>>> + continue; >>>> + } >>>> + >>>> + ret = pd_wdev_create(pd_dev, pd_name); >>>> + if (ret) { >>>> + dev_err(dev, "Error building cooling device %s %d\n", pd_name, ret); >>>> + dev_pm_domain_detach(pd_dev, false); >>>> + continue; >>>> + } >>> >>> I am wondering about the use case of having multiple PM domains >>> attached to the cooling (warming) device. Is that really needed? >>> Perhaps you can elaborate on that a bit? >> Ya. I though about this as well. I don't have a use case. In my current >> case it is just one power domain on the SoC. But considering this is now >> a generic driver, in my opinion this has to be a generic solution. So if >> you think about this, the device should be able to specify any number of >> power domains that can behave as a warming device since a SoC can have >> any number of power domain based warming devices. May be one to warm up >> the cpus, one for gpus etc. > > I get that, but you can always have more than one warming device. Each > warming device would then be attached to a single PM domain. Or is > there a problem with that? > > In any case, if you don't have use case for multiple PM domains per > warming device at this point, I would rather keep it simple and start > to support only the single PM domain case. Ok. I will remove the support for multiple PM domains for now. > >> >> So another way of implementing this whole thing is to avoid having a >> special power domain warming device defined in the device tree. Instead, >> add a few new binding to the power-domain controller/provider entries >> to specify if a power domain controlled by the provider can act as a >> warming device or not. And have the initialization code for the power >> domain controller (of_genpd_add_provider_onecell or any other suitable >> API) register the specified power domain as a warming device. The DT >> entries should probably look something like below in the case. >> >> rpmhpd: power-controller { >> compatible = "qcom,sdm845-rpmhpd"; >> #power-domain-cells = <1>; >> hosts-warming-dev; >> warming-dev-names = "mx"; >> operating-points-v2 = <&rpmhpd_opp_table>; >> >> rpmhpd_opp_table: opp-table { >> compatible = "operating-points-v2"; >> .... >> >> And have the following in of_genpd_add_provider_onecell >> >> if (hosts-warming-dev) >> # loop through the warming-dev-names and register them as power domain >> warming devices. >> >> You think this is a better idea? > > Not really, but you need to re-direct that question to DT maintainers > if want a better answer. I will wait for the DT folks to take a look at this series. Hopefully DT folks will have some comments on the approach of a virtual device like this implementation vs specifying this info in the power domain controllers. I just wanted to run it by you to check whether you see any pros or cons from a genpd perspective. I will wait for a few more days for any additional review comments before sending v3 out. -- Warm Regards Thara