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

+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