On Tue, Feb 11, 2020 at 5:58 PM Rob Herring robh+dt@kernel.org wrote:
On Tue, Feb 11, 2020 at 2:09 PM Saravana Kannan saravanak@google.com wrote:
On Tue, Feb 11, 2020 at 11:44 AM Rob Herring robh+dt@kernel.org wrote:
+Saravana
On Thu, Feb 6, 2020 at 11:27 PM Nicolas Boichat drinkcat@chromium.org wrote:
When there is a single power domain per device, the core will ensure the power domain is switched on (so it is technically equivalent to having not power domain specified at all).
However, when there are multiple domains, as in MT8183 Bifrost GPU, we need to handle them in driver code.
Signed-off-by: Nicolas Boichat drinkcat@chromium.org
The downstream driver we use on chromeos-4.19 currently uses 2 additional devices in device tree to accomodate for this [1], but I believe this solution is cleaner.
[1] https://chromium.googlesource.com/chromiumos/third_party/kernel/+/refs/heads...
v4:
- Match the exact power domain names as specified in the compatible struct, instead of just matching the number of power domains. [Review: Ulf Hansson]
- Dropped print and reordered function [Review: Steven Price]
- nits: Run through latest version of checkpatch:
- Use WARN instead of BUG_ON.
- Drop braces for single expression if block.
v3:
- Use the compatible matching data to specify the number of power domains. Note that setting 0 or 1 in num_pm_domains is equivalent as the core will handle these 2 cases in the exact same way (automatically, without driver intervention), and there should be no adverse consequence in this case (the concern is about switching on only some power domains and not others).
drivers/gpu/drm/panfrost/panfrost_device.c | 97 ++++++++++++++++++++-- drivers/gpu/drm/panfrost/panfrost_device.h | 11 +++ drivers/gpu/drm/panfrost/panfrost_drv.c | 2 + 3 files changed, 102 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 3720d50f6d9f965..8136babd3ba9935 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -5,6 +5,7 @@ #include <linux/clk.h> #include <linux/reset.h> #include <linux/platform_device.h> +#include <linux/pm_domain.h> #include <linux/regulator/consumer.h>
#include "panfrost_device.h" @@ -120,6 +121,79 @@ static void panfrost_regulator_fini(struct panfrost_device *pfdev) pfdev->regulators); }
+static void panfrost_pm_domain_fini(struct panfrost_device *pfdev) +{
int i;for (i = 0; i < ARRAY_SIZE(pfdev->pm_domain_devs); i++) {if (!pfdev->pm_domain_devs[i])break;if (pfdev->pm_domain_links[i])device_link_del(pfdev->pm_domain_links[i]);dev_pm_domain_detach(pfdev->pm_domain_devs[i], true);}+}
+static int panfrost_pm_domain_init(struct panfrost_device *pfdev) +{
int err;int i, num_domains;num_domains = of_count_phandle_with_args(pfdev->dev->of_node,"power-domains","#power-domain-cells");/** Single domain is handled by the core, and, if only a single power* the power domain is requested, the property is optional.*/if (num_domains < 2 && pfdev->comp->num_pm_domains < 2)return 0;if (num_domains != pfdev->comp->num_pm_domains) {dev_err(pfdev->dev,"Incorrect number of power domains: %d provided, %d needed\n",num_domains, pfdev->comp->num_pm_domains);return -EINVAL;}if (WARN(num_domains > ARRAY_SIZE(pfdev->pm_domain_devs),"Too many supplies in compatible structure.\n"))return -EINVAL;for (i = 0; i < num_domains; i++) {pfdev->pm_domain_devs[i] =dev_pm_domain_attach_by_name(pfdev->dev,pfdev->comp->pm_domain_names[i]);if (IS_ERR_OR_NULL(pfdev->pm_domain_devs[i])) {err = PTR_ERR(pfdev->pm_domain_devs[i]) ? : -ENODATA;pfdev->pm_domain_devs[i] = NULL;dev_err(pfdev->dev,"failed to get pm-domain %s(%d): %d\n",pfdev->comp->pm_domain_names[i], i, err);goto err;}pfdev->pm_domain_links[i] = device_link_add(pfdev->dev,pfdev->pm_domain_devs[i], DL_FLAG_PM_RUNTIME |DL_FLAG_STATELESS | DL_FLAG_RPM_ACTIVE);We're in the process of adding device links based on DT properties. Shouldn't we add power domains to that? See drivers/of/property.c for what's handled.
Rob,
drivers/of/property.c doesn't enable the RPM_ACTIVE AND PM_RUNTIME flags. Wanted to start off conservative.
I worry that you can't add it later without potentially breaking platforms.
I haven't checked, but I assume these flags make runtime PM honor device links? That seems like the more conservative option (more reasons why a device can't suspend).
Conservative as in, if of_devlink adds the RPM_ACTIVE flag, the drivers can't remove it.
But adding command line ops to change the default flags shouldn't be difficult. But before I do that, I want to change of_devlink to fw_devlink=<disabled|permissive|enabled>. May be I can extend that to "disabled, permissive, suspend, runtime".
I think any command line option should be debug primarily. It's kind of a big hammer.
It is a big hammer. But it's better than disabling of_devlink altogether. There is always going to be weird hardware that won't work with of_devlink if all the device link flags are set. They'll need some fix up of drivers and/or clean up of DT. And having different of_devlink command line options would at least let those hardware to run with as much of_devlink enabled as possible while working to get more fixes into the kernel. That way, we can make sure we don't regress further while trying to improve the support.
Can drivers adjust the flags themselves? Just modify the flags rather than trying to create new links?
They can, but only in an additive manner. And the way to do it would be to add a device link like usual and the framework takes care of combining the flags. That's why I don't set most of the flags by default.
-Saravana