Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains

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. 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".

Nicholas,

And the adding and removing of device links for power domains will be a 2 line change. I've been meaning to add a few more bindings like hwspinlocks and pinctrl. I can roll power domains support into that if you want.

-Saravana