On Fri, Feb 7, 2020 at 10:26 PM Ulf Hansson ulf.hansson@linaro.org wrote:
On Fri, 7 Feb 2020 at 06:27, 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
Besides a minor nitpick, feel free to add:
Reviewed-by: Ulf Hansson ulf.hansson@linaro.org
Kind regards Uffe
[snip]
+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"))Nitpick: Not sure this deserves a WARN. Perhaps a regular dev_err() is sufficient.
Ah well I had a BUG_ON before so presumably this is already a little better ,-)
You can only reach there if you set pfdev->comp->num_pm_domains > MAX_PM_DOMAINS in the currently matched struct panfrost_compatible (pfdev->comp->num_pm_domains == num_domains, and see below too), so the kernel code would actually be actually broken (not the device tree, nor anything that could be probed). So I'm wondering if the loudness of a WARN is better in this case? Arguable ,-)
return -EINVAL;[snip]
--- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -21,6 +21,7 @@ struct panfrost_perfcnt;
#define NUM_JOB_SLOTS 3 #define MAX_REGULATORS 2 +#define MAX_PM_DOMAINS 3
struct panfrost_features { u16 id; @@ -61,6 +62,13 @@ struct panfrost_compatible { /* Supplies count and names. */ int num_supplies; const char * const *supply_names;
/** Number of power domains required, note that values 0 and 1 are* handled identically, as only values > 1 need special handling.*/int num_pm_domains;/* Only required if num_pm_domains > 1. */const char * const *pm_domain_names;};
struct panfrost_device { @@ -73,6 +81,9 @@ struct panfrost_device { struct clk *bus_clock; struct regulator_bulk_data regulators[MAX_REGULATORS]; struct reset_control *rstc;
/* pm_domains for devices with more than one. */struct device *pm_domain_devs[MAX_PM_DOMAINS];struct device_link *pm_domain_links[MAX_PM_DOMAINS]; struct panfrost_features features; const struct panfrost_compatible *comp;diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index 4d08507526239f2..a6e162236d67fdf 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -663,6 +663,8 @@ const char * const default_supplies[] = { "mali" }; static const struct panfrost_compatible default_data = { .num_supplies = ARRAY_SIZE(default_supplies), .supply_names = default_supplies,
.num_pm_domains = 1, /* optional */.pm_domain_names = NULL,};
static const struct of_device_id dt_match[] = {
2.25.0.341.g760bfbb309-goog