+Nick Fan +Sj Huang @ MTK

On Wed, Feb 26, 2020 at 1:16 AM Rob Herring robh@kernel.org wrote:

On Fri, Feb 07, 2020 at 01:26:21PM +0800, Nicolas Boichat wrote:

...

Define a compatible string for the Mali Bifrost GPU found in Mediatek's MT8183 SoCs.

Signed-off-by: Nicolas Boichat drinkcat@chromium.org Reviewed-by: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com

---

v4:

• Add power-domain-names description (kept Alyssa's reviewed-by as the change is minor)

v3:

• No change

.../bindings/gpu/arm,mali-bifrost.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 4ea6a8789699709..0d93b3981445977 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -17,6 +17,7 @@ properties: items: - enum: - amlogic,meson-g12a-mali

•      - mediatek,mt8183-mali
       - realtek,rtd1619-mali
       - rockchip,px30-mali
   - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
  

@@ -62,6 +63,30 @@ allOf: minItems: 2 required: - resets

• • if:

•  properties:
  

•    compatible:
  

•      contains:
  

•        const: mediatek,mt8183-mali
  

• then:

•  properties:
  

•    sram-supply: true
  

•    power-domains:
  

•      description:
  

•        List of phandle and PM domain specifier as documented in
  

•        Documentation/devicetree/bindings/power/power_domain.txt
  

•      minItems: 3
  

•      maxItems: 3
  

•    power-domain-names:
  

•      items:
  

•        - const: core0
  

•        - const: core1
  

•        - const: 2d
  

AFAIK, there's no '2d' block in bifrost GPUs. A power domain for each core group is correct though.

Good question... Hopefully Nick/SJ@MTK can comment, the non-upstream DTS has: gpu: mali@13040000 { compatible = "mediatek,mt8183-mali", "arm,mali-bifrost"; power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_CORE0>; ... }

gpu_core1: mali_gpu_core1 { compatible = "mediatek,gpu_core1"; power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_CORE1>; };

gpu_core2: mali_gpu_core2 { compatible = "mediatek,gpu_core2"; power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>; };

So I picked core0/core1/2d as names, but looking at this, it's likely core2 is more appropriate (and MT8183_POWER_DOMAIN_MFG_2D might just be a internal/legacy name, if there is no real 2d domain).

Thanks.

Rob

On Fri, Mar 6, 2020 at 10:34 AM Nick Fan nick.fan@mediatek.com wrote:

Sorry for my late reply. I have checked internally. The MT8183_POWER_DOMAIN_MFG_2D is just a legacy name, not really 2D domain.

If the naming too confusing, we can change this name to MT8183_POWER_DOMAIN_MFG_CORE2 for consistency.

Thanks! I think I'll keep MT8183_POWER_DOMAIN_MFG_2D (that's fine if that's the domain name you use internally in your HW design), but I'll modify power-domain-names to core0/1/2 in the binding.

Thanks

Nick Fan

On Wed, 2020-02-26 at 08:55 +0800, Nicolas Boichat wrote:

...

+Nick Fan +Sj Huang @ MTK

On Wed, Feb 26, 2020 at 1:16 AM Rob Herring robh@kernel.org wrote:

...

On Fri, Feb 07, 2020 at 01:26:21PM +0800, Nicolas Boichat wrote:

...

Define a compatible string for the Mali Bifrost GPU found in Mediatek's MT8183 SoCs.

Signed-off-by: Nicolas Boichat drinkcat@chromium.org Reviewed-by: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com

---

v4:

• Add power-domain-names description (kept Alyssa's reviewed-by as the change is minor)

v3:

• No change

.../bindings/gpu/arm,mali-bifrost.yaml | 25 +++++++++++++++++++ 1 file changed, 25 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml index 4ea6a8789699709..0d93b3981445977 100644 --- a/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml +++ b/Documentation/devicetree/bindings/gpu/arm,mali-bifrost.yaml @@ -17,6 +17,7 @@ properties: items: - enum: - amlogic,meson-g12a-mali

•      - mediatek,mt8183-mali
       - realtek,rtd1619-mali
       - rockchip,px30-mali
   - const: arm,mali-bifrost # Mali Bifrost GPU model/revision is fully discoverable
  

@@ -62,6 +63,30 @@ allOf: minItems: 2 required: - resets

• • if:

•  properties:
  

•    compatible:
  

•      contains:
  

•        const: mediatek,mt8183-mali
  

• then:

•  properties:
  

•    sram-supply: true
  

•    power-domains:
  

•      description:
  

•        List of phandle and PM domain specifier as documented in
  

•        Documentation/devicetree/bindings/power/power_domain.txt
  

•      minItems: 3
  

•      maxItems: 3
  

•    power-domain-names:
  

•      items:
  

•        - const: core0
  

•        - const: core1
  

•        - const: 2d
  

AFAIK, there's no '2d' block in bifrost GPUs. A power domain for each core group is correct though.

Good question... Hopefully Nick/SJ@MTK can comment, the non-upstream DTS has: gpu: mali@13040000 { compatible = "mediatek,mt8183-mali", "arm,mali-bifrost"; power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_CORE0>; ... }

gpu_core1: mali_gpu_core1 { compatible = "mediatek,gpu_core1"; power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_CORE1>; };

gpu_core2: mali_gpu_core2 { compatible = "mediatek,gpu_core2"; power-domains = <&scpsys MT8183_POWER_DOMAIN_MFG_2D>; };

So I picked core0/core1/2d as names, but looking at this, it's likely core2 is more appropriate (and MT8183_POWER_DOMAIN_MFG_2D might just be a internal/legacy name, if there is no real 2d domain).

Thanks.

...

Rob

On 07/02/2020 06:26, Nicolas Boichat wrote:

It is useful to know which component cannot be powered on.

Signed-off-by: Nicolas Boichat drinkcat@chromium.org Reviewed-by: Steven Price steven.price@arm.com Reviewed-by: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com

Reviewed-by: Matthias Brugger mbrugger@suse.com

---

Was useful when trying to probe Bifrost GPU, to understand what issue we are facing.

v4:

• No change

v3:

• Rebased on https://patchwork.kernel.org/patch/11325689/

drivers/gpu/drm/panfrost/panfrost_gpu.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_gpu.c b/drivers/gpu/drm/panfrost/panfrost_gpu.c index 460fc190de6e815..856f2fd1fa8ed27 100644 --- a/drivers/gpu/drm/panfrost/panfrost_gpu.c +++ b/drivers/gpu/drm/panfrost/panfrost_gpu.c @@ -308,17 +308,20 @@ void panfrost_gpu_power_on(struct panfrost_device *pfdev) gpu_write(pfdev, L2_PWRON_LO, pfdev->features.l2_present); ret = readl_relaxed_poll_timeout(pfdev->iomem + L2_READY_LO, val, val == pfdev->features.l2_present, 100, 1000);

• if (ret)

• dev_err(pfdev->dev, "error powering up gpu L2");
  
  

  gpu_write(pfdev, SHADER_PWRON_LO, pfdev->features.shader_present);

• ret |= readl_relaxed_poll_timeout(pfdev->iomem + SHADER_READY_LO,

• ret = readl_relaxed_poll_timeout(pfdev->iomem + SHADER_READY_LO, val, val == pfdev->features.shader_present, 100, 1000);

• if (ret)

• dev_err(pfdev->dev, "error powering up gpu shader");
  
  

  gpu_write(pfdev, TILER_PWRON_LO, pfdev->features.tiler_present);

• ret |= readl_relaxed_poll_timeout(pfdev->iomem + TILER_READY_LO,

• ret = readl_relaxed_poll_timeout(pfdev->iomem + TILER_READY_LO, val, val == pfdev->features.tiler_present, 100, 1000);

• if (ret)

• dev_err(pfdev->dev, "error powering up gpu");
  

• dev_err(pfdev->dev, "error powering up gpu tiler");
  

}

void panfrost_gpu_power_off(struct panfrost_device *pfdev)

On 07/02/2020 05:26, Nicolas Boichat wrote:

Some GPUs, namely, the bifrost/g72 part on MT8183, have a second regulator for their SRAM, let's add support for that.

We extend the framework in a generic manner so that we could support more than 2 regulators, if required.

Signed-off-by: Nicolas Boichat drinkcat@chromium.org

Reviewed-by: Steven Price steven.price@arm.com

Thanks,

Steve

---

v4:

• nits: Run through latest version of checkpatch:
  • Use WARN instead of BUG_ON.
  • Drop braces in single expression for loop.
  • *comp not * comp

v3:

• Make this more generic, by allowing any number of regulators (in practice we fix the maximum number of regulators to 2, but this could be increased easily).
• We only probe the second regulator if the device tree matching data asks for it.
• I couldn't find a way to detect the number of regulators in the device tree, if we wanted to refuse to probe the device if there are too many regulators, which might be required for safety, see the thread on v2 [1].
• The discussion also included the idea of a separate device tree entry for a "soft PDC", or at least a separate driver. I'm not sure to understand the full picture, and how different vendors implement this, so I'm still integrating everything in the main driver. I'd be happy to try to make mt8183 fit into such a framework after it's created, but I don't think I'm best placed to implement (and again, the main purpose of this was to test if the binding is correct).

[1] https://patchwork.kernel.org/patch/11322839/

drivers/gpu/drm/panfrost/panfrost_device.c | 26 +++++++++++++------- drivers/gpu/drm/panfrost/panfrost_device.h | 15 +++++++++++- drivers/gpu/drm/panfrost/panfrost_drv.c | 28 +++++++++++++++------- 3 files changed, 51 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c index 238fb6d54df4732..3720d50f6d9f965 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.c +++ b/drivers/gpu/drm/panfrost/panfrost_device.c @@ -87,18 +87,27 @@ static void panfrost_clk_fini(struct panfrost_device *pfdev)

static int panfrost_regulator_init(struct panfrost_device *pfdev) {

• int ret;

• int ret, i;

• pfdev->regulator = devm_regulator_get(pfdev->dev, "mali");
• if (IS_ERR(pfdev->regulator)) {

• ret = PTR_ERR(pfdev->regulator);
  

• dev_err(pfdev->dev, "failed to get regulator: %d\n", ret);
  

• if (WARN(pfdev->comp->num_supplies > ARRAY_SIZE(pfdev->regulators),

• 	"Too many supplies in compatible structure.\n"))
  

• return -EINVAL;
  

• for (i = 0; i < pfdev->comp->num_supplies; i++)

• pfdev->regulators[i].supply = pfdev->comp->supply_names[i];
  

• ret = devm_regulator_bulk_get(pfdev->dev,

• 		      pfdev->comp->num_supplies,
  

• 		      pfdev->regulators);
  

• if (ret < 0) {

• dev_err(pfdev->dev, "failed to get regulators: %d\n", ret);
  

  return ret; }

• ret = regulator_enable(pfdev->regulator);

• ret = regulator_bulk_enable(pfdev->comp->num_supplies,

• 		    pfdev->regulators);
  

  if (ret < 0) {

• dev_err(pfdev->dev, "failed to enable regulator: %d\n", ret);
  

• dev_err(pfdev->dev, "failed to enable regulators: %d\n", ret);
  

  return ret; }

@@ -107,7 +116,8 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)

static void panfrost_regulator_fini(struct panfrost_device *pfdev) {

• regulator_disable(pfdev->regulator);

• regulator_bulk_disable(pfdev->comp->num_supplies,

• 	pfdev->regulators);
  

}

int panfrost_device_init(struct panfrost_device *pfdev) diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index 06713811b92cdf7..c9468bc5573ac9d 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -7,6 +7,7 @@

#include <linux/atomic.h> #include <linux/io-pgtable.h> +#include <linux/regulator/consumer.h> #include <linux/spinlock.h> #include <drm/drm_device.h> #include <drm/drm_mm.h> @@ -19,6 +20,7 @@ struct panfrost_job; struct panfrost_perfcnt;

#define NUM_JOB_SLOTS 3 +#define MAX_REGULATORS 2

struct panfrost_features { u16 id; @@ -51,6 +53,16 @@ struct panfrost_features { unsigned long hw_issues[64 / BITS_PER_LONG]; };

+/*

• • Features that cannot be automatically detected and need matching using the
• • compatible string, typically SoC-specific.
• */

+struct panfrost_compatible {

• /* Supplies count and names. */
• int num_supplies;
• const char * const *supply_names;

+};

struct panfrost_device { struct device *dev; struct drm_device *ddev; @@ -59,10 +71,11 @@ struct panfrost_device { void __iomem *iomem; struct clk *clock; struct clk *bus_clock;

• struct regulator *regulator;

• struct regulator_bulk_data regulators[MAX_REGULATORS]; struct reset_control *rstc;

  struct panfrost_features features;

• const struct panfrost_compatible *comp;

  spinlock_t as_lock; unsigned long as_in_use_mask;

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c index b7a618db3ee223e..4d08507526239f2 100644 --- a/drivers/gpu/drm/panfrost/panfrost_drv.c +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c @@ -584,6 +584,10 @@ static int panfrost_probe(struct platform_device *pdev)

platform_set_drvdata(pdev, pfdev);

• pfdev->comp = of_device_get_match_data(&pdev->dev);
• if (!pfdev->comp)

• return -ENODEV;
  

• /* Allocate and initialze the DRM device. */ ddev = drm_dev_alloc(&panfrost_drm_driver, &pdev->dev); if (IS_ERR(ddev))

@@ -655,16 +659,22 @@ static int panfrost_remove(struct platform_device *pdev) return 0; }

+const char * const default_supplies[] = { "mali" }; +static const struct panfrost_compatible default_data = {

• .num_supplies = ARRAY_SIZE(default_supplies),
• .supply_names = default_supplies,

+};

static const struct of_device_id dt_match[] = {

• { .compatible = "arm,mali-t604" },
• { .compatible = "arm,mali-t624" },
• { .compatible = "arm,mali-t628" },
• { .compatible = "arm,mali-t720" },
• { .compatible = "arm,mali-t760" },
• { .compatible = "arm,mali-t820" },
• { .compatible = "arm,mali-t830" },
• { .compatible = "arm,mali-t860" },
• { .compatible = "arm,mali-t880" },

• { .compatible = "arm,mali-t604", .data = &default_data, },
• { .compatible = "arm,mali-t624", .data = &default_data, },
• { .compatible = "arm,mali-t628", .data = &default_data, },
• { .compatible = "arm,mali-t720", .data = &default_data, },
• { .compatible = "arm,mali-t760", .data = &default_data, },
• { .compatible = "arm,mali-t820", .data = &default_data, },
• { .compatible = "arm,mali-t830", .data = &default_data, },
• { .compatible = "arm,mali-t860", .data = &default_data, },
• { .compatible = "arm,mali-t880", .data = &default_data, }, {}

}; MODULE_DEVICE_TABLE(of, dt_match);

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

On 07/02/2020 05:26, Nicolas Boichat 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

LGTM!

Reviewed-by: Steven Price steven.price@arm.com

Thanks,

Steve

---

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);
  

• if (!pfdev->pm_domain_links[i]) {
  

• 	dev_err(pfdev->pm_domain_devs[i],
  

• 		"adding device link failed!\n");
  

• 	err = -ENODEV;
  

• 	goto err;
  

• }
  

• }
• return 0;

+err:

• panfrost_pm_domain_fini(pfdev);
• return err;

+}

int panfrost_device_init(struct panfrost_device *pfdev) { int err; @@ -150,37 +224,43 @@ int panfrost_device_init(struct panfrost_device *pfdev) goto err_out1; }

• err = panfrost_pm_domain_init(pfdev);
• if (err)

• goto err_out2;
  

• res = platform_get_resource(pfdev->pdev, IORESOURCE_MEM, 0); pfdev->iomem = devm_ioremap_resource(pfdev->dev, res); if (IS_ERR(pfdev->iomem)) { dev_err(pfdev->dev, "failed to ioremap iomem\n"); err = PTR_ERR(pfdev->iomem);

• goto err_out2;
  

• goto err_out3;
  

  }

  err = panfrost_gpu_init(pfdev); if (err)

• goto err_out2;
  

• goto err_out3;
  
  

  err = panfrost_mmu_init(pfdev); if (err)

• goto err_out3;
  

• goto err_out4;
  
  

  err = panfrost_job_init(pfdev); if (err)

• goto err_out4;
  

• goto err_out5;
  
  

  err = panfrost_perfcnt_init(pfdev); if (err)

• goto err_out5;
  

• goto err_out6;
  
  

  return 0;

-err_out5: +err_out6: panfrost_job_fini(pfdev); -err_out4: +err_out5: panfrost_mmu_fini(pfdev); -err_out3: +err_out4: panfrost_gpu_fini(pfdev); +err_out3:

• panfrost_pm_domain_fini(pfdev);

err_out2: panfrost_reset_fini(pfdev); err_out1: @@ -196,6 +276,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev) panfrost_job_fini(pfdev); panfrost_mmu_fini(pfdev); panfrost_gpu_fini(pfdev);

• panfrost_pm_domain_fini(pfdev); panfrost_reset_fini(pfdev); panfrost_regulator_fini(pfdev); panfrost_clk_fini(pfdev);

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index c9468bc5573ac9d..c30c719a805940a 100644 --- 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[] = {

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

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

+Viresh Kumar +Stephen Boyd for clock advice.

On Fri, Feb 7, 2020 at 1:27 PM Nicolas Boichat drinkcat@chromium.org wrote:

The Bifrost GPU on MT8183 uses 2 regulators (core and SRAM) for devfreq, and provides OPP table with 2 sets of voltages.

TODO: This is incomplete as we'll need add support for setting a pair of voltages as well.

So all we need for this to work (at least apparently, that is, I can change frequency) is this: https://lore.kernel.org/patchwork/patch/1192945/ (ah well, Viresh just replied, so, probably not, I'll check that out and use the correct API)

But then there's a slight problem: panfrost_devfreq uses a bunch of clk_get_rate calls, and the clock PLLs (at least on MTK platform) are never fully precise, so we get back 299999955 for 300 Mhz and 799999878 for 800 Mhz. That means that the kernel is unable to keep devfreq stats as neither of these values are in the table: [ 4802.470952] devfreq devfreq1: Couldn't update frequency transition information. The kbase driver fixes this by remembering the last set frequency, and reporting that to devfreq. Should we do that as well or is there a better fix?

Another thing that I'm not implementing is the dance that Mediatek does in their kbase driver when changing the clock (described in patch 2/7): "" The binding we use with out-of-tree Mali drivers includes more clocks, this is used for devfreq: the out-of-tree driver switches clk_mux to clk_sub_parent (26Mhz), adjusts clk_main_parent, then switches clk_mux back to clk_main_parent: (see https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4...) clocks = <&topckgen CLK_TOP_MFGPLL_CK>, <&topckgen CLK_TOP_MUX_MFG>, <&clk26m>, <&mfgcfg CLK_MFG_BG3D>; clock-names = "clk_main_parent", "clk_mux", "clk_sub_parent", "subsys_mfg_cg"; "" Is there a clean/simple way to implement this in the clock framework/device tree? Or should we implement something in the panfrost driver?

Thanks!

Signed-off-by: Nicolas Boichat drinkcat@chromium.org

---

drivers/gpu/drm/panfrost/panfrost_devfreq.c | 17 +++++++++++++++++ drivers/gpu/drm/panfrost/panfrost_device.h | 1 + 2 files changed, 18 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 413987038fbfccb..9c0987a3d71c597 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -79,6 +79,21 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) struct devfreq *devfreq; struct thermal_cooling_device *cooling;

•   /* If we have 2 regulator, we need an OPP table with 2 voltages. */
  

•   if (pfdev->comp->num_supplies > 1) {
  

•           pfdev->devfreq.dev_opp_table =
  

•                   dev_pm_opp_set_regulators(dev,
  

•                                   pfdev->comp->supply_names,
  

•                                   pfdev->comp->num_supplies);
  

•           if (IS_ERR(pfdev->devfreq.dev_opp_table)) {
  

•                   ret = PTR_ERR(pfdev->devfreq.dev_opp_table);
  

•                   pfdev->devfreq.dev_opp_table = NULL;
  

•                   dev_err(dev,
  

•                           "Failed to init devfreq opp table: %d\n", ret);
  

•                   return ret;
  

•           }
  

•   }
  

•   ret = dev_pm_opp_of_add_table(dev);
    if (ret == -ENODEV) /* Optional, continue without devfreq */
            return 0;
  

@@ -119,6 +134,8 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev) if (pfdev->devfreq.cooling) devfreq_cooling_unregister(pfdev->devfreq.cooling); dev_pm_opp_of_remove_table(&pfdev->pdev->dev);

•   if (pfdev->devfreq.dev_opp_table)
  

•           dev_pm_opp_put_regulators(pfdev->devfreq.dev_opp_table);
  

}

void panfrost_devfreq_resume(struct panfrost_device *pfdev) diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h index c30c719a805940a..5009a8b7c853ea1 100644 --- a/drivers/gpu/drm/panfrost/panfrost_device.h +++ b/drivers/gpu/drm/panfrost/panfrost_device.h @@ -110,6 +110,7 @@ struct panfrost_device { struct { struct devfreq *devfreq; struct thermal_cooling_device *cooling;

•           struct opp_table *dev_opp_table;
            ktime_t busy_time;
            ktime_t idle_time;
            ktime_t time_last_update;
  

-- 2.25.0.341.g760bfbb309-goog