Linux-mediatek Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
@ 2020-02-07  5:26 Nicolas Boichat
  2020-02-07  5:26 ` [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183 Nicolas Boichat
                   ` (8 more replies)
  0 siblings, 9 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

Hi!

Follow-up on the v3: https://patchwork.kernel.org/cover/11331343/.

The main purpose of this series is to upstream the dts change and the
binding document, but I wanted to see how far I could probe the GPU, to
check that the binding is indeed correct. The rest of the patches are
RFC/work-in-progress, but I think some of them could already be picked up.

So this is tested on MT8183 with a chromeos-4.19 kernel, and a ton of
backports to get the latest panfrost driver (I should probably try on
linux-next at some point but this was the path of least resistance).

I tested it as a module as it's more challenging (originally probing would
work built-in, on boot, but not as a module, as I didn't have the power
domain changes, and all power domains are on by default during boot).

Probing logs looks like this, currently. They look sane.
[  501.319728] panfrost 13040000.gpu: clock rate = 511999970
[  501.320041] panfrost 13040000.gpu: Linked as a consumer to regulator.14
[  501.320102] panfrost 13040000.gpu: Linked as a consumer to regulator.31
[  501.320651] panfrost 13040000.gpu: Linked as a consumer to genpd:0:13040000.gpu
[  501.320954] panfrost 13040000.gpu: Linked as a consumer to genpd:1:13040000.gpu
[  501.321062] panfrost 13040000.gpu: Linked as a consumer to genpd:2:13040000.gpu
[  501.321734] panfrost 13040000.gpu: mali-g72 id 0x6221 major 0x0 minor 0x3 status 0x0
[  501.321741] panfrost 13040000.gpu: features: 00000000,13de77ff, issues: 00000000,00000400
[  501.321747] panfrost 13040000.gpu: Features: L2:0x07120206 Shader:0x00000000 Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
[  501.321752] panfrost 13040000.gpu: shader_present=0x7 l2_present=0x1
[  501.324951] [drm] Initialized panfrost 1.1.0 20180908 for 13040000.gpu on minor 2

Some more changes are still required to get devfreq working, and of course
I do not have a userspace driver to test this with.

I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
maintainers decide.

Thanks!

Nicolas Boichat (7):
  dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
  arm64: dts: mt8183: Add node for the Mali GPU
  drm/panfrost: Improve error reporting in panfrost_gpu_power_on
  drm/panfrost: Add support for multiple regulators
  drm/panfrost: Add support for multiple power domains
  RFC: drm/panfrost: Add mt8183-mali compatible string
  RFC: drm/panfrost: devfreq: Add support for 2 regulators

 .../bindings/gpu/arm,mali-bifrost.yaml        |  25 ++++
 arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   7 +
 arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 105 +++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  17 +++
 drivers/gpu/drm/panfrost/panfrost_device.c    | 123 +++++++++++++++---
 drivers/gpu/drm/panfrost/panfrost_device.h    |  27 +++-
 drivers/gpu/drm/panfrost/panfrost_drv.c       |  41 ++++--
 drivers/gpu/drm/panfrost/panfrost_gpu.c       |  11 +-
 8 files changed, 326 insertions(+), 30 deletions(-)

-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-25 17:16   ` Rob Herring
  2020-02-07  5:26 ` [PATCH v4 2/7] arm64: dts: mt8183: Add node for the Mali GPU Nicolas Boichat
                   ` (7 subsequent siblings)
  8 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

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
+
+      required:
+        - sram-supply
+        - power-domains
+        - power-domains-names
 
 examples:
   - |
-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 2/7] arm64: dts: mt8183: Add node for the Mali GPU
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
  2020-02-07  5:26 ` [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183 Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-07  5:26 ` [PATCH v4 3/7] drm/panfrost: Improve error reporting in panfrost_gpu_power_on Nicolas Boichat
                   ` (6 subsequent siblings)
  8 siblings, 0 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

Add a basic GPU node for mt8183.

Signed-off-by: Nicolas Boichat <drinkcat@chromium.org>
Reviewed-by: Alyssa Rosenzweig <alyssa.rosenzweig@collabora.com>
---
Upstreaming what matches existing bindings from our Chromium OS tree:
https://chromium.googlesource.com/chromiumos/third_party/kernel/+/chromeos-4.19/arch/arm64/boot/dts/mediatek/mt8183.dtsi#1348

The evb part of this change depends on this patch to add PMIC dtsi:
https://patchwork.kernel.org/patch/10928161/

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.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#423)
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";

v4:
 - Add power-domain-names to describe the 3 domains.
   (kept Alyssa's reviewed-by as the change is minor)

v3:
 - No changes

v2:
 - Use sram instead of mali_sram as SRAM supply name.
 - Rename mali@ to gpu@.

 arch/arm64/boot/dts/mediatek/mt8183-evb.dts |   7 ++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi    | 105 ++++++++++++++++++++
 2 files changed, 112 insertions(+)

diff --git a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
index 1fb195c683c3d01..7d609e0cd9b4975 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8183-evb.dts
@@ -7,6 +7,7 @@
 
 /dts-v1/;
 #include "mt8183.dtsi"
+#include "mt6358.dtsi"
 
 / {
 	model = "MediaTek MT8183 evaluation board";
@@ -30,6 +31,12 @@ &auxadc {
 	status = "okay";
 };
 
+&gpu {
+	supply-names = "mali", "sram";
+	mali-supply = <&mt6358_vgpu_reg>;
+	sram-supply = <&mt6358_vsram_gpu_reg>;
+};
+
 &i2c0 {
 	pinctrl-names = "default";
 	pinctrl-0 = <&i2c_pins_0>;
diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 124f9d3e09f532c..74b5305f663f740 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -599,6 +599,111 @@ mfgcfg: syscon@13000000 {
 			#clock-cells = <1>;
 		};
 
+		gpu: gpu@13040000 {
+			compatible = "mediatek,mt8183-mali", "arm,mali-bifrost";
+			reg = <0 0x13040000 0 0x4000>;
+			interrupts =
+				<GIC_SPI 280 IRQ_TYPE_LEVEL_LOW>,
+				<GIC_SPI 279 IRQ_TYPE_LEVEL_LOW>,
+				<GIC_SPI 278 IRQ_TYPE_LEVEL_LOW>;
+			interrupt-names = "job", "mmu", "gpu";
+
+			clocks = <&topckgen CLK_TOP_MFGPLL_CK>;
+
+			power-domains =
+				<&scpsys MT8183_POWER_DOMAIN_MFG_CORE0>,
+				<&scpsys MT8183_POWER_DOMAIN_MFG_CORE1>,
+				<&scpsys MT8183_POWER_DOMAIN_MFG_2D>;
+			power-domain-names = "core0", "core1", "2d";
+
+			operating-points-v2 = <&gpu_opp_table>;
+		};
+
+		gpu_opp_table: opp_table0 {
+			compatible = "operating-points-v2";
+			opp-shared;
+
+			opp-300000000 {
+				opp-hz = /bits/ 64 <300000000>;
+				opp-microvolt = <625000>, <850000>;
+			};
+
+			opp-320000000 {
+				opp-hz = /bits/ 64 <320000000>;
+				opp-microvolt = <631250>, <850000>;
+			};
+
+			opp-340000000 {
+				opp-hz = /bits/ 64 <340000000>;
+				opp-microvolt = <637500>, <850000>;
+			};
+
+			opp-360000000 {
+				opp-hz = /bits/ 64 <360000000>;
+				opp-microvolt = <643750>, <850000>;
+			};
+
+			opp-380000000 {
+				opp-hz = /bits/ 64 <380000000>;
+				opp-microvolt = <650000>, <850000>;
+			};
+
+			opp-400000000 {
+				opp-hz = /bits/ 64 <400000000>;
+				opp-microvolt = <656250>, <850000>;
+			};
+
+			opp-420000000 {
+				opp-hz = /bits/ 64 <420000000>;
+				opp-microvolt = <662500>, <850000>;
+			};
+
+			opp-460000000 {
+				opp-hz = /bits/ 64 <460000000>;
+				opp-microvolt = <675000>, <850000>;
+			};
+
+			opp-500000000 {
+				opp-hz = /bits/ 64 <500000000>;
+				opp-microvolt = <687500>, <850000>;
+			};
+
+			opp-540000000 {
+				opp-hz = /bits/ 64 <540000000>;
+				opp-microvolt = <700000>, <850000>;
+			};
+
+			opp-580000000 {
+				opp-hz = /bits/ 64 <580000000>;
+				opp-microvolt = <712500>, <850000>;
+			};
+
+			opp-620000000 {
+				opp-hz = /bits/ 64 <620000000>;
+				opp-microvolt = <725000>, <850000>;
+			};
+
+			opp-653000000 {
+				opp-hz = /bits/ 64 <653000000>;
+				opp-microvolt = <743750>, <850000>;
+			};
+
+			opp-698000000 {
+				opp-hz = /bits/ 64 <698000000>;
+				opp-microvolt = <768750>, <868750>;
+			};
+
+			opp-743000000 {
+				opp-hz = /bits/ 64 <743000000>;
+				opp-microvolt = <793750>, <893750>;
+			};
+
+			opp-800000000 {
+				opp-hz = /bits/ 64 <800000000>;
+				opp-microvolt = <825000>, <925000>;
+			};
+		};
+
 		mmsys: syscon@14000000 {
 			compatible = "mediatek,mt8183-mmsys", "syscon";
 			reg = <0 0x14000000 0 0x1000>;
-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 3/7] drm/panfrost: Improve error reporting in panfrost_gpu_power_on
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
  2020-02-07  5:26 ` [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183 Nicolas Boichat
  2020-02-07  5:26 ` [PATCH v4 2/7] arm64: dts: mt8183: Add node for the Mali GPU Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-12 10:38   ` Matthias Brugger
  2020-02-07  5:26 ` [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators Nicolas Boichat
                   ` (5 subsequent siblings)
  8 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

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

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)
-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
                   ` (2 preceding siblings ...)
  2020-02-07  5:26 ` [PATCH v4 3/7] drm/panfrost: Improve error reporting in panfrost_gpu_power_on Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-10 11:31   ` Steven Price
  2020-02-10 14:00   ` Mark Brown
  2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
                   ` (4 subsequent siblings)
  8 siblings, 2 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

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>

---

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);
-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
                   ` (3 preceding siblings ...)
  2020-02-07  5:26 ` [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-07 13:52   ` Alyssa Rosenzweig
                     ` (3 more replies)
  2020-02-07  5:26 ` [PATCH v4 6/7] RFC: drm/panfrost: Add mt8183-mali compatible string Nicolas Boichat
                   ` (3 subsequent siblings)
  8 siblings, 4 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31

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[] = {
-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 6/7] RFC: drm/panfrost: Add mt8183-mali compatible string
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
                   ` (4 preceding siblings ...)
  2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-07  5:26 ` [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators Nicolas Boichat
                   ` (2 subsequent siblings)
  8 siblings, 0 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

For testing only, the driver doesn't really work yet, AFAICT.

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

---

v4:
 - Add power domain names.
v3:
 - Match mt8183-mali instead of bifrost, as we require special
   handling for the 2 regulators and 3 power domains.

 drivers/gpu/drm/panfrost/panfrost_drv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index a6e162236d67fdf..497c375932ad589 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -667,6 +667,15 @@ static const struct panfrost_compatible default_data = {
 	.pm_domain_names = NULL,
 };
 
+const char * const mediatek_mt8183_supplies[] = { "mali", "sram" };
+const char * const mediatek_mt8183_pm_domains[] = { "core0", "core1", "2d" };
+static const struct panfrost_compatible mediatek_mt8183_data = {
+	.num_supplies = ARRAY_SIZE(mediatek_mt8183_supplies),
+	.supply_names = mediatek_mt8183_supplies,
+	.num_pm_domains = 3,
+	.pm_domain_names = mediatek_mt8183_pm_domains,
+};
+
 static const struct of_device_id dt_match[] = {
 	{ .compatible = "arm,mali-t604", .data = &default_data, },
 	{ .compatible = "arm,mali-t624", .data = &default_data, },
@@ -677,6 +686,8 @@ static const struct of_device_id dt_match[] = {
 	{ .compatible = "arm,mali-t830", .data = &default_data, },
 	{ .compatible = "arm,mali-t860", .data = &default_data, },
 	{ .compatible = "arm,mali-t880", .data = &default_data, },
+	{ .compatible = "mediatek,mt8183-mali",
+		.data = &mediatek_mt8183_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, dt_match);
-- 
2.25.0.341.g760bfbb309-goog


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
                   ` (5 preceding siblings ...)
  2020-02-07  5:26 ` [PATCH v4 6/7] RFC: drm/panfrost: Add mt8183-mali compatible string Nicolas Boichat
@ 2020-02-07  5:26 ` Nicolas Boichat
  2020-02-12  8:49   ` Nicolas Boichat
  2020-02-07  6:17 ` [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Tomeu Vizoso
  2020-02-25 20:35 ` Rob Herring
  8 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  5:26 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

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.

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


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
                   ` (6 preceding siblings ...)
  2020-02-07  5:26 ` [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators Nicolas Boichat
@ 2020-02-07  6:17 ` Tomeu Vizoso
  2020-02-07  7:42   ` Nicolas Boichat
  2020-02-25 20:35 ` Rob Herring
  8 siblings, 1 reply; 35+ messages in thread
From: Tomeu Vizoso @ 2020-02-07  6:17 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, David Airlie,
	linux-kernel, Liam Girdwood, dri-devel, Steven Price, Mark Brown,
	linux-mediatek, Alyssa Rosenzweig, Daniel Vetter, hsinyi,
	Matthias Brugger, linux-arm-kernel

On 2/7/20 6:26 AM, Nicolas Boichat wrote:
> Hi!
> 
> Follow-up on the v3: https://patchwork.kernel.org/cover/11331343/.
> 
> The main purpose of this series is to upstream the dts change and the
> binding document, but I wanted to see how far I could probe the GPU, to
> check that the binding is indeed correct. The rest of the patches are
> RFC/work-in-progress, but I think some of them could already be picked up.
> 
> So this is tested on MT8183 with a chromeos-4.19 kernel, and a ton of
> backports to get the latest panfrost driver (I should probably try on
> linux-next at some point but this was the path of least resistance).
> 
> I tested it as a module as it's more challenging (originally probing would
> work built-in, on boot, but not as a module, as I didn't have the power
> domain changes, and all power domains are on by default during boot).
> 
> Probing logs looks like this, currently. They look sane.
> [  501.319728] panfrost 13040000.gpu: clock rate = 511999970
> [  501.320041] panfrost 13040000.gpu: Linked as a consumer to regulator.14
> [  501.320102] panfrost 13040000.gpu: Linked as a consumer to regulator.31
> [  501.320651] panfrost 13040000.gpu: Linked as a consumer to genpd:0:13040000.gpu
> [  501.320954] panfrost 13040000.gpu: Linked as a consumer to genpd:1:13040000.gpu
> [  501.321062] panfrost 13040000.gpu: Linked as a consumer to genpd:2:13040000.gpu
> [  501.321734] panfrost 13040000.gpu: mali-g72 id 0x6221 major 0x0 minor 0x3 status 0x0
> [  501.321741] panfrost 13040000.gpu: features: 00000000,13de77ff, issues: 00000000,00000400
> [  501.321747] panfrost 13040000.gpu: Features: L2:0x07120206 Shader:0x00000000 Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
> [  501.321752] panfrost 13040000.gpu: shader_present=0x7 l2_present=0x1
> [  501.324951] [drm] Initialized panfrost 1.1.0 20180908 for 13040000.gpu on minor 2
> 
> Some more changes are still required to get devfreq working, and of course
> I do not have a userspace driver to test this with.

Have you tried the Panfrost tests in IGT? They are atm quite basic, but 
could be interesting to check that the different HW units are correctly 
powered on.

Regards,

Tomeu

> I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
> useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
> maintainers decide.
> 
> Thanks!
> 
> Nicolas Boichat (7):
>    dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
>    arm64: dts: mt8183: Add node for the Mali GPU
>    drm/panfrost: Improve error reporting in panfrost_gpu_power_on
>    drm/panfrost: Add support for multiple regulators
>    drm/panfrost: Add support for multiple power domains
>    RFC: drm/panfrost: Add mt8183-mali compatible string
>    RFC: drm/panfrost: devfreq: Add support for 2 regulators
> 
>   .../bindings/gpu/arm,mali-bifrost.yaml        |  25 ++++
>   arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   7 +
>   arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 105 +++++++++++++++
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  17 +++
>   drivers/gpu/drm/panfrost/panfrost_device.c    | 123 +++++++++++++++---
>   drivers/gpu/drm/panfrost/panfrost_device.h    |  27 +++-
>   drivers/gpu/drm/panfrost/panfrost_drv.c       |  41 ++++--
>   drivers/gpu/drm/panfrost/panfrost_gpu.c       |  11 +-
>   8 files changed, 326 insertions(+), 30 deletions(-)
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
  2020-02-07  6:17 ` [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Tomeu Vizoso
@ 2020-02-07  7:42   ` Nicolas Boichat
  2020-02-07  8:13     ` Tomeu Vizoso
  0 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-07  7:42 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, David Airlie, lkml,
	Mark Brown, Liam Girdwood, dri-devel, Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

On Fri, Feb 7, 2020 at 2:18 PM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>
> On 2/7/20 6:26 AM, Nicolas Boichat wrote:
> > Hi!
> >
> > Follow-up on the v3: https://patchwork.kernel.org/cover/11331343/.
> >
> > The main purpose of this series is to upstream the dts change and the
> > binding document, but I wanted to see how far I could probe the GPU, to
> > check that the binding is indeed correct. The rest of the patches are
> > RFC/work-in-progress, but I think some of them could already be picked up.
> >
> > So this is tested on MT8183 with a chromeos-4.19 kernel, and a ton of
> > backports to get the latest panfrost driver (I should probably try on
> > linux-next at some point but this was the path of least resistance).
> >
> > I tested it as a module as it's more challenging (originally probing would
> > work built-in, on boot, but not as a module, as I didn't have the power
> > domain changes, and all power domains are on by default during boot).
> >
> > Probing logs looks like this, currently. They look sane.
> > [  501.319728] panfrost 13040000.gpu: clock rate = 511999970
> > [  501.320041] panfrost 13040000.gpu: Linked as a consumer to regulator.14
> > [  501.320102] panfrost 13040000.gpu: Linked as a consumer to regulator.31
> > [  501.320651] panfrost 13040000.gpu: Linked as a consumer to genpd:0:13040000.gpu
> > [  501.320954] panfrost 13040000.gpu: Linked as a consumer to genpd:1:13040000.gpu
> > [  501.321062] panfrost 13040000.gpu: Linked as a consumer to genpd:2:13040000.gpu
> > [  501.321734] panfrost 13040000.gpu: mali-g72 id 0x6221 major 0x0 minor 0x3 status 0x0
> > [  501.321741] panfrost 13040000.gpu: features: 00000000,13de77ff, issues: 00000000,00000400
> > [  501.321747] panfrost 13040000.gpu: Features: L2:0x07120206 Shader:0x00000000 Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
> > [  501.321752] panfrost 13040000.gpu: shader_present=0x7 l2_present=0x1
> > [  501.324951] [drm] Initialized panfrost 1.1.0 20180908 for 13040000.gpu on minor 2
> >
> > Some more changes are still required to get devfreq working, and of course
> > I do not have a userspace driver to test this with.
>
> Have you tried the Panfrost tests in IGT? They are atm quite basic, but
> could be interesting to check that the different HW units are correctly
> powered on.

I haven't, you mean this right?
https://gitlab.freedesktop.org/tomeu/igt-gpu-tools/tree/panfrost

Any specific test you have in mind?

Thanks,

> Regards,
>
> Tomeu
>
> > I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
> > useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
> > maintainers decide.
> >
> > Thanks!
> >
> > Nicolas Boichat (7):
> >    dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
> >    arm64: dts: mt8183: Add node for the Mali GPU
> >    drm/panfrost: Improve error reporting in panfrost_gpu_power_on
> >    drm/panfrost: Add support for multiple regulators
> >    drm/panfrost: Add support for multiple power domains
> >    RFC: drm/panfrost: Add mt8183-mali compatible string
> >    RFC: drm/panfrost: devfreq: Add support for 2 regulators
> >
> >   .../bindings/gpu/arm,mali-bifrost.yaml        |  25 ++++
> >   arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   7 +
> >   arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 105 +++++++++++++++
> >   drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  17 +++
> >   drivers/gpu/drm/panfrost/panfrost_device.c    | 123 +++++++++++++++---
> >   drivers/gpu/drm/panfrost/panfrost_device.h    |  27 +++-
> >   drivers/gpu/drm/panfrost/panfrost_drv.c       |  41 ++++--
> >   drivers/gpu/drm/panfrost/panfrost_gpu.c       |  11 +-
> >   8 files changed, 326 insertions(+), 30 deletions(-)
> >

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
  2020-02-07  7:42   ` Nicolas Boichat
@ 2020-02-07  8:13     ` Tomeu Vizoso
  2020-02-10  3:39       ` Nicolas Boichat
  0 siblings, 1 reply; 35+ messages in thread
From: Tomeu Vizoso @ 2020-02-07  8:13 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, David Airlie, lkml,
	Mark Brown, Liam Girdwood, dri-devel, Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

On 2/7/20 8:42 AM, Nicolas Boichat wrote:
> On Fri, Feb 7, 2020 at 2:18 PM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>
>> On 2/7/20 6:26 AM, Nicolas Boichat wrote:
>>> Hi!
>>>
>>> Follow-up on the v3: https://patchwork.kernel.org/cover/11331343/.
>>>
>>> The main purpose of this series is to upstream the dts change and the
>>> binding document, but I wanted to see how far I could probe the GPU, to
>>> check that the binding is indeed correct. The rest of the patches are
>>> RFC/work-in-progress, but I think some of them could already be picked up.
>>>
>>> So this is tested on MT8183 with a chromeos-4.19 kernel, and a ton of
>>> backports to get the latest panfrost driver (I should probably try on
>>> linux-next at some point but this was the path of least resistance).
>>>
>>> I tested it as a module as it's more challenging (originally probing would
>>> work built-in, on boot, but not as a module, as I didn't have the power
>>> domain changes, and all power domains are on by default during boot).
>>>
>>> Probing logs looks like this, currently. They look sane.
>>> [  501.319728] panfrost 13040000.gpu: clock rate = 511999970
>>> [  501.320041] panfrost 13040000.gpu: Linked as a consumer to regulator.14
>>> [  501.320102] panfrost 13040000.gpu: Linked as a consumer to regulator.31
>>> [  501.320651] panfrost 13040000.gpu: Linked as a consumer to genpd:0:13040000.gpu
>>> [  501.320954] panfrost 13040000.gpu: Linked as a consumer to genpd:1:13040000.gpu
>>> [  501.321062] panfrost 13040000.gpu: Linked as a consumer to genpd:2:13040000.gpu
>>> [  501.321734] panfrost 13040000.gpu: mali-g72 id 0x6221 major 0x0 minor 0x3 status 0x0
>>> [  501.321741] panfrost 13040000.gpu: features: 00000000,13de77ff, issues: 00000000,00000400
>>> [  501.321747] panfrost 13040000.gpu: Features: L2:0x07120206 Shader:0x00000000 Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
>>> [  501.321752] panfrost 13040000.gpu: shader_present=0x7 l2_present=0x1
>>> [  501.324951] [drm] Initialized panfrost 1.1.0 20180908 for 13040000.gpu on minor 2
>>>
>>> Some more changes are still required to get devfreq working, and of course
>>> I do not have a userspace driver to test this with.
>>
>> Have you tried the Panfrost tests in IGT? They are atm quite basic, but
>> could be interesting to check that the different HW units are correctly
>> powered on.
> 
> I haven't, you mean this right?
> https://gitlab.freedesktop.org/tomeu/igt-gpu-tools/tree/panfrost

Yes, though may be better to use the upstream repo:

https://gitlab.freedesktop.org/drm/igt-gpu-tools

> Any specific test you have in mind?

All the panfrost ones, but looks like panfrost_prime:gem-prime-import is 
failing atm:

https://lava.collabora.co.uk/scheduler/job/2214987

Cheers,

Tomeu

> Thanks,
> 
>> Regards,
>>
>> Tomeu
>>
>>> I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
>>> useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
>>> maintainers decide.
>>>
>>> Thanks!
>>>
>>> Nicolas Boichat (7):
>>>     dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
>>>     arm64: dts: mt8183: Add node for the Mali GPU
>>>     drm/panfrost: Improve error reporting in panfrost_gpu_power_on
>>>     drm/panfrost: Add support for multiple regulators
>>>     drm/panfrost: Add support for multiple power domains
>>>     RFC: drm/panfrost: Add mt8183-mali compatible string
>>>     RFC: drm/panfrost: devfreq: Add support for 2 regulators
>>>
>>>    .../bindings/gpu/arm,mali-bifrost.yaml        |  25 ++++
>>>    arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   7 +
>>>    arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 105 +++++++++++++++
>>>    drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  17 +++
>>>    drivers/gpu/drm/panfrost/panfrost_device.c    | 123 +++++++++++++++---
>>>    drivers/gpu/drm/panfrost/panfrost_device.h    |  27 +++-
>>>    drivers/gpu/drm/panfrost/panfrost_drv.c       |  41 ++++--
>>>    drivers/gpu/drm/panfrost/panfrost_gpu.c       |  11 +-
>>>    8 files changed, 326 insertions(+), 30 deletions(-)
>>>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
@ 2020-02-07 13:52   ` Alyssa Rosenzweig
  2020-02-09 12:43     ` Nicolas Boichat
  2020-02-07 14:25   ` Ulf Hansson
                     ` (2 subsequent siblings)
  3 siblings, 1 reply; 35+ messages in thread
From: Alyssa Rosenzweig @ 2020-02-07 13:52 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Mark Brown, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring, linux-mediatek, Daniel Vetter, hsinyi,
	Matthias Brugger, linux-arm-kernel

[-- Attachment #1.1: Type: text/plain, Size: 201 bytes --]

> +	for (i = 0; i < ARRAY_SIZE(pfdev->pm_domain_devs); i++) {
> +		if (!pfdev->pm_domain_devs[i])
> +			break;

I'm not totally familiar with this code, but should this be a break or
just a continue?


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
  2020-02-07 13:52   ` Alyssa Rosenzweig
@ 2020-02-07 14:25   ` Ulf Hansson
  2020-02-09 12:50     ` Nicolas Boichat
  2020-02-10 11:39   ` Steven Price
  2020-02-11 19:43   ` Rob Herring
  3 siblings, 1 reply; 35+ messages in thread
From: Ulf Hansson @ 2020-02-07 14:25 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, DTML, Tomeu Vizoso, David Airlie,
	Linux Kernel Mailing List, Mark Brown, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger, Linux ARM

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

>
> ---
>
> 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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31
>
> 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"))

Nitpick:
Not sure this deserves a WARN. Perhaps a regular dev_err() is sufficient.

> +               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[] = {
> --
> 2.25.0.341.g760bfbb309-goog
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07 13:52   ` Alyssa Rosenzweig
@ 2020-02-09 12:43     ` Nicolas Boichat
  0 siblings, 0 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-09 12:43 UTC (permalink / raw)
  To: Alyssa Rosenzweig
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Mark Brown, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Daniel Vetter,
	Hsin-Yi Wang, Matthias Brugger, linux-arm Mailing List

On Fri, Feb 7, 2020 at 9:52 PM Alyssa Rosenzweig
<alyssa.rosenzweig@collabora.com> wrote:
>
> > +     for (i = 0; i < ARRAY_SIZE(pfdev->pm_domain_devs); i++) {
> > +             if (!pfdev->pm_domain_devs[i])
> > +                     break;

(next time, please provide a tiny bit more context when quoting, I had
to look up to see where this comes from)
So this comes from panfrost_pm_domain_fini.

> I'm not totally familiar with this code, but should this be a break or
> just a continue?

Check how the domains are initialized in panfrost_pm_domain_init, they
are guaranteed to be "packed" at the beginning of the array, so there
can't be any holes, so break is safe.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07 14:25   ` Ulf Hansson
@ 2020-02-09 12:50     ` Nicolas Boichat
  2020-02-10  7:50       ` Ulf Hansson
  0 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-09 12:50 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Mark Rutland, DTML, Tomeu Vizoso, David Airlie,
	Linux Kernel Mailing List, Mark Brown, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger, Linux ARM

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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
  2020-02-07  8:13     ` Tomeu Vizoso
@ 2020-02-10  3:39       ` Nicolas Boichat
  2020-02-10  8:17         ` Tomeu Vizoso
  0 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-10  3:39 UTC (permalink / raw)
  To: Tomeu Vizoso
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, David Airlie, lkml,
	Mark Brown, Liam Girdwood, dri-devel, Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

On Fri, Feb 7, 2020 at 4:13 PM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>
> On 2/7/20 8:42 AM, Nicolas Boichat wrote:
> > On Fri, Feb 7, 2020 at 2:18 PM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> >>
> >>> Some more changes are still required to get devfreq working, and of course
> >>> I do not have a userspace driver to test this with.
> >>
> >> Have you tried the Panfrost tests in IGT? They are atm quite basic, but
> >> could be interesting to check that the different HW units are correctly
> >> powered on.
> >
> > I haven't, you mean this right?
> > https://gitlab.freedesktop.org/tomeu/igt-gpu-tools/tree/panfrost
>
> Yes, though may be better to use the upstream repo:
>
> https://gitlab.freedesktop.org/drm/igt-gpu-tools
>
> > Any specific test you have in mind?
>
> All the panfrost ones, but looks like panfrost_prime:gem-prime-import is
> failing atm:
>
> https://lava.collabora.co.uk/scheduler/job/2214987

(I first removed opp table from device tree to avoid constant spew
about devfreq not supporting 2 regulators, I should get around to fix
that...)

# /usr/libexec/igt-gpu-tools/panfrost_gem_new
IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
Starting subtest: gem-new-4096
Subtest gem-new-4096: SUCCESS (0.000s)
Starting subtest: gem-new-0
Subtest gem-new-0: SUCCESS (0.000s)
Starting subtest: gem-new-zeroed
Subtest gem-new-zeroed: SUCCESS (0.001s)
# /usr/libexec/igt-gpu-tools/panfrost_get_param
IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
Starting subtest: base-params
Subtest base-params: SUCCESS (0.000s)
Starting subtest: get-bad-param
Subtest get-bad-param: SUCCESS (0.000s)
Starting subtest: get-bad-padding
Subtest get-bad-padding: SUCCESS (0.000s)
# /usr/libexec/igt-gpu-tools/panfrost_prime
IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
Starting subtest: gem-prime-import
(panfrost_prime:1527) ioctl_wrappers-CRITICAL: Test assertion failure
function prime_fd_to_handle, file
../igt-gpu-tools-9999/lib/ioctl_wrappers.c:1336:
(panfrost_prime:1527) ioctl_wrappers-CRITICAL: Failed assertion:
igt_ioctl((fd), ((((2U|1U) << (((0+8)+8)+14)) | ((('d')) << (0+8)) |
(((0x2e)) << 0) | ((((sizeof(struct drm_prime_handle)))) <<
((0+8)+8)))), (&args)) == 0
(panfrost_prime:1527) ioctl_wrappers-CRITICAL: Last errno: 95,
Operation not supported
(panfrost_prime:1527) ioctl_wrappers-CRITICAL: error: -1 != 0
Stack trace:
Subtest gem-prime-import failed.
Subtest gem-prime-import: FAIL (0.004s)
(but that looks expected?)

Now the trickier ones, I guess we're either missing something, or my
dirty 4.19 backport is very broken:
# /usr/libexec/igt-gpu-tools/panfrost_submit
IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
Starting subtest: pan-submit
(panfrost_submit:1643) CRITICAL: Test assertion failure function
__real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:103:
(panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
&submit->args->out_sync, 1, abs_timeout(SHORT_TIME_NSEC), 0, NULL)
Stack trace:
Subtest pan-submit failed.
**** DEBUG ****
(panfrost_submit:1643) CRITICAL: Test assertion failure function
__real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:103:
(panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
&submit->args->out_sync, 1, abs_timeout(SHORT_TIME_NSEC), 0, NULL)
(panfrost_submit:1643) igt_core-INFO: Stack trace:
****  END  ****
Subtest pan-submit: FAIL (0.119s)
Starting subtest: pan-submit-error-no-jc
Subtest pan-submit-error-no-jc: SUCCESS (0.000s)
Starting subtest: pan-submit-error-bad-in-syncs
Subtest pan-submit-error-bad-in-syncs: SUCCESS (0.012s)
Starting subtest: pan-submit-error-bad-bo-handles
Subtest pan-submit-error-bad-bo-handles: SUCCESS (0.012s)
Starting subtest: pan-submit-error-bad-requirements
Subtest pan-submit-error-bad-requirements: SUCCESS (0.012s)
Starting subtest: pan-submit-error-bad-out-sync
Subtest pan-submit-error-bad-out-sync: SUCCESS (0.012s)
Starting subtest: pan-reset
(panfrost_submit:1643) CRITICAL: Test assertion failure function
__real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:173:
(panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
&submit->args->out_sync, 1, abs_timeout(BAD_JOB_TIME_NSEC), 0, NULL)
Stack trace:
Subtest pan-reset failed.
**** DEBUG ****
(panfrost_submit:1643) CRITICAL: Test assertion failure function
__real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:173:
(panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
&submit->args->out_sync, 1, abs_timeout(BAD_JOB_TIME_NSEC), 0, NULL)
(panfrost_submit:1643) igt_core-INFO: Stack trace:
****  END  ****
Subtest pan-reset: FAIL (0.840s)

The pan-submit case causes an MMU fault:
(full log: https://gist.github.com/drinkcat/1ae36cb1b1b71f30cc4fc29759612d76)

[ 1215.234937] [IGT] panfrost_submit: executing
[ 1215.318446] [IGT] panfrost_submit: starting subtest pan-submit
...
[ 1215.338644] panfrost 13040000.gpu: Unhandled Page fault in AS0 at
VA 0x000000FF00000000
               Reason: TODO
               raw fault status: 0xA002C0
               decoded fault status: SLAVE FAULT
               exception type 0xC0: UNKNOWN
               access type 0x2: READ
               source id 0xA0
[ 1215.444504] [IGT] panfrost_submit: exiting, ret=98
...
[ 1215.446902] panfrost 13040000.gpu: js fault, js=0,
status=JOB_BUS_FAULT, head=0x300b000, tail=0x300b000
[ 1215.446935] panfrost 13040000.gpu: Unhandled Page fault in AS0 at
VA 0x000000FF00000000
Reason: TODO
raw fault status: 0xA002C0
decoded fault status: SLAVE FAULT
exception type 0xC0: UNKNOWN
access type 0x2: READ
source id 0xA0

pan-reset failure looks similar:
https://gist.github.com/drinkcat/2d336d57e6b95262d83e7a28a409bc5b

Thanks,

> Cheers,
>
> Tomeu
>
> > Thanks,
> >
> >> Regards,
> >>
> >> Tomeu
> >>
> >>> I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
> >>> useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
> >>> maintainers decide.
> >>>
> >>> Thanks!
> >>>
> >>> Nicolas Boichat (7):
> >>>     dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
> >>>     arm64: dts: mt8183: Add node for the Mali GPU
> >>>     drm/panfrost: Improve error reporting in panfrost_gpu_power_on
> >>>     drm/panfrost: Add support for multiple regulators
> >>>     drm/panfrost: Add support for multiple power domains
> >>>     RFC: drm/panfrost: Add mt8183-mali compatible string
> >>>     RFC: drm/panfrost: devfreq: Add support for 2 regulators
> >>>
> >>>    .../bindings/gpu/arm,mali-bifrost.yaml        |  25 ++++
> >>>    arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   7 +
> >>>    arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 105 +++++++++++++++
> >>>    drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  17 +++
> >>>    drivers/gpu/drm/panfrost/panfrost_device.c    | 123 +++++++++++++++---
> >>>    drivers/gpu/drm/panfrost/panfrost_device.h    |  27 +++-
> >>>    drivers/gpu/drm/panfrost/panfrost_drv.c       |  41 ++++--
> >>>    drivers/gpu/drm/panfrost/panfrost_gpu.c       |  11 +-
> >>>    8 files changed, 326 insertions(+), 30 deletions(-)
> >>>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-09 12:50     ` Nicolas Boichat
@ 2020-02-10  7:50       ` Ulf Hansson
  0 siblings, 0 replies; 35+ messages in thread
From: Ulf Hansson @ 2020-02-10  7:50 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, DTML, Tomeu Vizoso, David Airlie,
	Linux Kernel Mailing List, Mark Brown, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger, Linux ARM

On Sun, 9 Feb 2020 at 13:50, Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> 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 ,-)

I see. It's not a big a deal, so feel free to keep as is.

[...]

Kind regards
Uffe

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
  2020-02-10  3:39       ` Nicolas Boichat
@ 2020-02-10  8:17         ` Tomeu Vizoso
  0 siblings, 0 replies; 35+ messages in thread
From: Tomeu Vizoso @ 2020-02-10  8:17 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, David Airlie, lkml,
	Mark Brown, Liam Girdwood, dri-devel, Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

On 2/10/20 4:39 AM, Nicolas Boichat wrote:
> On Fri, Feb 7, 2020 at 4:13 PM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>
>> On 2/7/20 8:42 AM, Nicolas Boichat wrote:
>>> On Fri, Feb 7, 2020 at 2:18 PM Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>>>
>>>>> Some more changes are still required to get devfreq working, and of course
>>>>> I do not have a userspace driver to test this with.
>>>>
>>>> Have you tried the Panfrost tests in IGT? They are atm quite basic, but
>>>> could be interesting to check that the different HW units are correctly
>>>> powered on.
>>>
>>> I haven't, you mean this right?
>>> https://gitlab.freedesktop.org/tomeu/igt-gpu-tools/tree/panfrost
>>
>> Yes, though may be better to use the upstream repo:
>>
>> https://gitlab.freedesktop.org/drm/igt-gpu-tools
>>
>>> Any specific test you have in mind?
>>
>> All the panfrost ones, but looks like panfrost_prime:gem-prime-import is
>> failing atm:
>>
>> https://lava.collabora.co.uk/scheduler/job/2214987
> 
> (I first removed opp table from device tree to avoid constant spew
> about devfreq not supporting 2 regulators, I should get around to fix
> that...)
> 
> # /usr/libexec/igt-gpu-tools/panfrost_gem_new
> IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
> Starting subtest: gem-new-4096
> Subtest gem-new-4096: SUCCESS (0.000s)
> Starting subtest: gem-new-0
> Subtest gem-new-0: SUCCESS (0.000s)
> Starting subtest: gem-new-zeroed
> Subtest gem-new-zeroed: SUCCESS (0.001s)
> # /usr/libexec/igt-gpu-tools/panfrost_get_param
> IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
> Starting subtest: base-params
> Subtest base-params: SUCCESS (0.000s)
> Starting subtest: get-bad-param
> Subtest get-bad-param: SUCCESS (0.000s)
> Starting subtest: get-bad-padding
> Subtest get-bad-padding: SUCCESS (0.000s)
> # /usr/libexec/igt-gpu-tools/panfrost_prime
> IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
> Starting subtest: gem-prime-import
> (panfrost_prime:1527) ioctl_wrappers-CRITICAL: Test assertion failure
> function prime_fd_to_handle, file
> ../igt-gpu-tools-9999/lib/ioctl_wrappers.c:1336:
> (panfrost_prime:1527) ioctl_wrappers-CRITICAL: Failed assertion:
> igt_ioctl((fd), ((((2U|1U) << (((0+8)+8)+14)) | ((('d')) << (0+8)) |
> (((0x2e)) << 0) | ((((sizeof(struct drm_prime_handle)))) <<
> ((0+8)+8)))), (&args)) == 0
> (panfrost_prime:1527) ioctl_wrappers-CRITICAL: Last errno: 95,
> Operation not supported
> (panfrost_prime:1527) ioctl_wrappers-CRITICAL: error: -1 != 0
> Stack trace:
> Subtest gem-prime-import failed.
> Subtest gem-prime-import: FAIL (0.004s)
> (but that looks expected?)

Yep, haven't gotten to investigate yet.

> Now the trickier ones, I guess we're either missing something, or my
> dirty 4.19 backport is very broken:

Damn, looks like the simple job we use to test submits doesn't work as-is 
on your GPU.

But things seem to work otherwise, so probably the kernel driver is fully 
functional with your changes.

Cheers,

Tomeu

> # /usr/libexec/igt-gpu-tools/panfrost_submit
> IGT-Version: 1.24-gd4d574a4 (arm) (Linux: 4.19.99 aarch64)
> Starting subtest: pan-submit
> (panfrost_submit:1643) CRITICAL: Test assertion failure function
> __real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:103:
> (panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
> &submit->args->out_sync, 1, abs_timeout(SHORT_TIME_NSEC), 0, NULL)
> Stack trace:
> Subtest pan-submit failed.
> **** DEBUG ****
> (panfrost_submit:1643) CRITICAL: Test assertion failure function
> __real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:103:
> (panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
> &submit->args->out_sync, 1, abs_timeout(SHORT_TIME_NSEC), 0, NULL)
> (panfrost_submit:1643) igt_core-INFO: Stack trace:
> ****  END  ****
> Subtest pan-submit: FAIL (0.119s)
> Starting subtest: pan-submit-error-no-jc
> Subtest pan-submit-error-no-jc: SUCCESS (0.000s)
> Starting subtest: pan-submit-error-bad-in-syncs
> Subtest pan-submit-error-bad-in-syncs: SUCCESS (0.012s)
> Starting subtest: pan-submit-error-bad-bo-handles
> Subtest pan-submit-error-bad-bo-handles: SUCCESS (0.012s)
> Starting subtest: pan-submit-error-bad-requirements
> Subtest pan-submit-error-bad-requirements: SUCCESS (0.012s)
> Starting subtest: pan-submit-error-bad-out-sync
> Subtest pan-submit-error-bad-out-sync: SUCCESS (0.012s)
> Starting subtest: pan-reset
> (panfrost_submit:1643) CRITICAL: Test assertion failure function
> __real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:173:
> (panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
> &submit->args->out_sync, 1, abs_timeout(BAD_JOB_TIME_NSEC), 0, NULL)
> Stack trace:
> Subtest pan-reset failed.
> **** DEBUG ****
> (panfrost_submit:1643) CRITICAL: Test assertion failure function
> __real_main86, file ../igt-gpu-tools-9999/tests/panfrost_submit.c:173:
> (panfrost_submit:1643) CRITICAL: Failed assertion: syncobj_wait(fd,
> &submit->args->out_sync, 1, abs_timeout(BAD_JOB_TIME_NSEC), 0, NULL)
> (panfrost_submit:1643) igt_core-INFO: Stack trace:
> ****  END  ****
> Subtest pan-reset: FAIL (0.840s)
> 
> The pan-submit case causes an MMU fault:
> (full log: https://gist.github.com/drinkcat/1ae36cb1b1b71f30cc4fc29759612d76)
> 
> [ 1215.234937] [IGT] panfrost_submit: executing
> [ 1215.318446] [IGT] panfrost_submit: starting subtest pan-submit
> ...
> [ 1215.338644] panfrost 13040000.gpu: Unhandled Page fault in AS0 at
> VA 0x000000FF00000000
>                 Reason: TODO
>                 raw fault status: 0xA002C0
>                 decoded fault status: SLAVE FAULT
>                 exception type 0xC0: UNKNOWN
>                 access type 0x2: READ
>                 source id 0xA0
> [ 1215.444504] [IGT] panfrost_submit: exiting, ret=98
> ...
> [ 1215.446902] panfrost 13040000.gpu: js fault, js=0,
> status=JOB_BUS_FAULT, head=0x300b000, tail=0x300b000
> [ 1215.446935] panfrost 13040000.gpu: Unhandled Page fault in AS0 at
> VA 0x000000FF00000000
> Reason: TODO
> raw fault status: 0xA002C0
> decoded fault status: SLAVE FAULT
> exception type 0xC0: UNKNOWN
> access type 0x2: READ
> source id 0xA0
> 
> pan-reset failure looks similar:
> https://gist.github.com/drinkcat/2d336d57e6b95262d83e7a28a409bc5b
> 
> Thanks,
> 
>> Cheers,
>>
>> Tomeu
>>
>>> Thanks,
>>>
>>>> Regards,
>>>>
>>>> Tomeu
>>>>
>>>>> I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
>>>>> useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
>>>>> maintainers decide.
>>>>>
>>>>> Thanks!
>>>>>
>>>>> Nicolas Boichat (7):
>>>>>      dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
>>>>>      arm64: dts: mt8183: Add node for the Mali GPU
>>>>>      drm/panfrost: Improve error reporting in panfrost_gpu_power_on
>>>>>      drm/panfrost: Add support for multiple regulators
>>>>>      drm/panfrost: Add support for multiple power domains
>>>>>      RFC: drm/panfrost: Add mt8183-mali compatible string
>>>>>      RFC: drm/panfrost: devfreq: Add support for 2 regulators
>>>>>
>>>>>     .../bindings/gpu/arm,mali-bifrost.yaml        |  25 ++++
>>>>>     arch/arm64/boot/dts/mediatek/mt8183-evb.dts   |   7 +
>>>>>     arch/arm64/boot/dts/mediatek/mt8183.dtsi      | 105 +++++++++++++++
>>>>>     drivers/gpu/drm/panfrost/panfrost_devfreq.c   |  17 +++
>>>>>     drivers/gpu/drm/panfrost/panfrost_device.c    | 123 +++++++++++++++---
>>>>>     drivers/gpu/drm/panfrost/panfrost_device.h    |  27 +++-
>>>>>     drivers/gpu/drm/panfrost/panfrost_drv.c       |  41 ++++--
>>>>>     drivers/gpu/drm/panfrost/panfrost_gpu.c       |  11 +-
>>>>>     8 files changed, 326 insertions(+), 30 deletions(-)
>>>>>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators
  2020-02-07  5:26 ` [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators Nicolas Boichat
@ 2020-02-10 11:31   ` Steven Price
  2020-02-10 14:00   ` Mark Brown
  1 sibling, 0 replies; 35+ messages in thread
From: Steven Price @ 2020-02-10 11:31 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, Liam Girdwood, dri-devel, linux-kernel, Mark Brown,
	linux-mediatek, Alyssa Rosenzweig, Daniel Vetter, hsinyi,
	Matthias Brugger, linux-arm-kernel

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


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
  2020-02-07 13:52   ` Alyssa Rosenzweig
  2020-02-07 14:25   ` Ulf Hansson
@ 2020-02-10 11:39   ` Steven Price
  2020-02-11 19:43   ` Rob Herring
  3 siblings, 0 replies; 35+ messages in thread
From: Steven Price @ 2020-02-10 11:39 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, Liam Girdwood, dri-devel, linux-kernel, Mark Brown,
	linux-mediatek, Alyssa Rosenzweig, Daniel Vetter, hsinyi,
	Matthias Brugger, linux-arm-kernel

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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31
> 
> 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[] = {
> 


_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators
  2020-02-07  5:26 ` [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators Nicolas Boichat
  2020-02-10 11:31   ` Steven Price
@ 2020-02-10 14:00   ` Mark Brown
  1 sibling, 0 replies; 35+ messages in thread
From: Mark Brown @ 2020-02-10 14:00 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

[-- Attachment #1.1: Type: text/plain, Size: 234 bytes --]

On Fri, Feb 07, 2020 at 01:26:24PM +0800, 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.

Reviwed-by: Mark Brown <broonie@kernel.org>


[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 170 bytes --]

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
                     ` (2 preceding siblings ...)
  2020-02-10 11:39   ` Steven Price
@ 2020-02-11 19:43   ` Rob Herring
  2020-02-11 20:08     ` Saravana Kannan
  3 siblings, 1 reply; 35+ messages in thread
From: Rob Herring @ 2020-02-11 19:43 UTC (permalink / raw)
  To: Nicolas Boichat, Saravana Kannan
  Cc: Mark Rutland, devicetree, Ulf Hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

+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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31
>
> 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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-11 19:43   ` Rob Herring
@ 2020-02-11 20:08     ` Saravana Kannan
  2020-02-12  1:58       ` Rob Herring
  2020-02-12  2:08       ` Nicolas Boichat
  0 siblings, 2 replies; 35+ messages in thread
From: Saravana Kannan @ 2020-02-11 20:08 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Ulf Hansson, Nicolas Boichat, Tomeu Vizoso, Greg Kroah-Hartman,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31
> >
> > 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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-11 20:08     ` Saravana Kannan
@ 2020-02-12  1:58       ` Rob Herring
  2020-02-12  2:10         ` Saravana Kannan
  2020-02-12  2:08       ` Nicolas Boichat
  1 sibling, 1 reply; 35+ messages in thread
From: Rob Herring @ 2020-02-12  1:58 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Ulf Hansson, Nicolas Boichat, Tomeu Vizoso, Greg Kroah-Hartman,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31
> > >
> > > 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).

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

Can drivers adjust the flags themselves? Just modify the flags rather
than trying to create new links?

Rob

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-11 20:08     ` Saravana Kannan
  2020-02-12  1:58       ` Rob Herring
@ 2020-02-12  2:08       ` Nicolas Boichat
  1 sibling, 0 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-12  2:08 UTC (permalink / raw)
  To: Saravana Kannan
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Ulf Hansson, Tomeu Vizoso, Greg Kroah-Hartman, David Airlie,
	linux-kernel, Mark Brown, Liam Girdwood, dri-devel, Steven Price,
	Rob Herring, moderated list:ARM/Mediatek SoC support,
	Alyssa Rosenzweig, Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Wed, Feb 12, 2020 at 4:09 AM 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>
> > >
> > > ---
> > > [snip]
> > > +               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.

Adding power domains makes sense to me, as there are a bunch of other
users (git grep dev_pm_domain_attach_by_name).

This seems to be a bit orthogonal to this patch though. If your
solution lands before this (and not something that is behind a command
line option), then I'm happy to make use of it. Either way, happy to
test things.

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains
  2020-02-12  1:58       ` Rob Herring
@ 2020-02-12  2:10         ` Saravana Kannan
  0 siblings, 0 replies; 35+ messages in thread
From: Saravana Kannan @ 2020-02-12  2:10 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland,
	open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
	Ulf Hansson, Nicolas Boichat, Tomeu Vizoso, Greg Kroah-Hartman,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

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/chromeos-4.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#31
> > > >
> > > > 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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators
  2020-02-07  5:26 ` [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators Nicolas Boichat
@ 2020-02-12  8:49   ` Nicolas Boichat
  2020-02-12  9:06     ` Viresh Kumar
  2020-02-12 18:14     ` Rob Herring
  0 siblings, 2 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-12  8:49 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Liam Girdwood, dri-devel, Steven Price,
	Mark Brown, moderated list:ARM/Mediatek SoC support,
	Alyssa Rosenzweig, Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

+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.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#423)
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
>

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators
  2020-02-12  8:49   ` Nicolas Boichat
@ 2020-02-12  9:06     ` Viresh Kumar
  2020-02-12 18:14     ` Rob Herring
  1 sibling, 0 replies; 35+ messages in thread
From: Viresh Kumar @ 2020-02-12  9:06 UTC (permalink / raw)
  To: Nicolas Boichat, sboyd
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Mark Brown, Liam Girdwood, dri-devel,
	Steven Price, Rob Herring,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List, vireshk

On Wed, Feb 12, 2020 at 2:19 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> +Viresh Kumar +Stephen Boyd for clock advice.

You missed adding us in the cc list of the email, fixed that now :)

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 3/7] drm/panfrost: Improve error reporting in panfrost_gpu_power_on
  2020-02-07  5:26 ` [PATCH v4 3/7] drm/panfrost: Improve error reporting in panfrost_gpu_power_on Nicolas Boichat
@ 2020-02-12 10:38   ` Matthias Brugger
  0 siblings, 0 replies; 35+ messages in thread
From: Matthias Brugger @ 2020-02-12 10:38 UTC (permalink / raw)
  To: Nicolas Boichat, Rob Herring
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, linux-arm-kernel



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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators
  2020-02-12  8:49   ` Nicolas Boichat
  2020-02-12  9:06     ` Viresh Kumar
@ 2020-02-12 18:14     ` Rob Herring
  2020-02-13  7:57       ` Nicolas Boichat
  1 sibling, 1 reply; 35+ messages in thread
From: Rob Herring @ 2020-02-12 18:14 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Liam Girdwood, dri-devel, Steven Price,
	Stephen Boyd, Viresh Kumar, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

On Wed, Feb 12, 2020 at 2:49 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> +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.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#423)
> 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?

Putting parent clocks into 'clocks' for a device is a pretty common
abuse. The 'assigned-clocks' binding is what's used for parent clock
setup. Not sure that's going to help here though. Is this dance
because the parent clock frequency can't be changed cleanly? If up to
me, I'd put that dance in the clock driver. The GPU shouldn't have to
care.

Rob

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators
  2020-02-12 18:14     ` Rob Herring
@ 2020-02-13  7:57       ` Nicolas Boichat
  2020-02-13  8:24         ` Nicolas Boichat
  0 siblings, 1 reply; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-13  7:57 UTC (permalink / raw)
  To: Rob Herring, Weiyi Lu, Nick Fan
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Liam Girdwood, dri-devel, Steven Price,
	Stephen Boyd, Viresh Kumar, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

+Weiyi Lu +Nick Fan @MTK who may have more ideas.

On Thu, Feb 13, 2020 at 2:14 AM Rob Herring <robh+dt@kernel.org> wrote:
>
> On Wed, Feb 12, 2020 at 2:49 AM Nicolas Boichat <drinkcat@chromium.org> wrote:
> >
> > +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.19/drivers/gpu/arm/midgard/platform/mediatek/mali_kbase_runtime_pm.c#423)
> > 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?
>
> Putting parent clocks into 'clocks' for a device is a pretty common
> abuse. The 'assigned-clocks' binding is what's used for parent clock
> setup. Not sure that's going to help here though. Is this dance
> because the parent clock frequency can't be changed cleanly?

Nick/Weiyi, any idea why we do that dance in the first place? (maybe
the PLL clock is unstable while it's being changed?)

If we really need it, can we move that logic to the clock core?

> If up to
> me, I'd put that dance in the clock driver. The GPU shouldn't have to
> care.
>
> Rob

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators
  2020-02-13  7:57       ` Nicolas Boichat
@ 2020-02-13  8:24         ` Nicolas Boichat
  0 siblings, 0 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-13  8:24 UTC (permalink / raw)
  To: Rob Herring
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Liam Girdwood, dri-devel, Steven Price,
	Stephen Boyd, Viresh Kumar, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

On Thu, Feb 13, 2020 at 3:57 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
> > > [snip]
> > > 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?

This one is my bad, I was missing this patch in my forklift to 4.19:
22bd4df9dadf46f drm/panfrost: devfreq: Round frequencies to OPPs

(should really try to boot that board on linux-next, but that's for
another time)

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
  2020-02-07  5:26 ` [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183 Nicolas Boichat
@ 2020-02-25 17:16   ` Rob Herring
  2020-02-26  0:55     ` Nicolas Boichat
  0 siblings, 1 reply; 35+ messages in thread
From: Rob Herring @ 2020-02-25 17:16 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, devicetree, ulf.hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown, linux-mediatek, Alyssa Rosenzweig,
	Daniel Vetter, hsinyi, Matthias Brugger, linux-arm-kernel

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.

Rob

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches)
  2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
                   ` (7 preceding siblings ...)
  2020-02-07  6:17 ` [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Tomeu Vizoso
@ 2020-02-25 20:35 ` Rob Herring
  8 siblings, 0 replies; 35+ messages in thread
From: Rob Herring @ 2020-02-25 20:35 UTC (permalink / raw)
  To: Nicolas Boichat
  Cc: Mark Rutland, devicetree, Ulf Hansson, Tomeu Vizoso,
	David Airlie, linux-kernel, Liam Girdwood, dri-devel,
	Steven Price, Mark Brown,
	moderated list:ARM/Mediatek SoC support, Alyssa Rosenzweig,
	Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE

On Thu, Feb 6, 2020 at 11:26 PM Nicolas Boichat <drinkcat@chromium.org> wrote:
>
> Hi!
>
> Follow-up on the v3: https://patchwork.kernel.org/cover/11331343/.
>
> The main purpose of this series is to upstream the dts change and the
> binding document, but I wanted to see how far I could probe the GPU, to
> check that the binding is indeed correct. The rest of the patches are
> RFC/work-in-progress, but I think some of them could already be picked up.
>
> So this is tested on MT8183 with a chromeos-4.19 kernel, and a ton of
> backports to get the latest panfrost driver (I should probably try on
> linux-next at some point but this was the path of least resistance).
>
> I tested it as a module as it's more challenging (originally probing would
> work built-in, on boot, but not as a module, as I didn't have the power
> domain changes, and all power domains are on by default during boot).
>
> Probing logs looks like this, currently. They look sane.
> [  501.319728] panfrost 13040000.gpu: clock rate = 511999970
> [  501.320041] panfrost 13040000.gpu: Linked as a consumer to regulator.14
> [  501.320102] panfrost 13040000.gpu: Linked as a consumer to regulator.31
> [  501.320651] panfrost 13040000.gpu: Linked as a consumer to genpd:0:13040000.gpu
> [  501.320954] panfrost 13040000.gpu: Linked as a consumer to genpd:1:13040000.gpu
> [  501.321062] panfrost 13040000.gpu: Linked as a consumer to genpd:2:13040000.gpu
> [  501.321734] panfrost 13040000.gpu: mali-g72 id 0x6221 major 0x0 minor 0x3 status 0x0
> [  501.321741] panfrost 13040000.gpu: features: 00000000,13de77ff, issues: 00000000,00000400
> [  501.321747] panfrost 13040000.gpu: Features: L2:0x07120206 Shader:0x00000000 Tiler:0x00000809 Mem:0x1 MMU:0x00002830 AS:0xff JS:0x7
> [  501.321752] panfrost 13040000.gpu: shader_present=0x7 l2_present=0x1
> [  501.324951] [drm] Initialized panfrost 1.1.0 20180908 for 13040000.gpu on minor 2
>
> Some more changes are still required to get devfreq working, and of course
> I do not have a userspace driver to test this with.
>
> I believe at least patches 1, 2, and 3 can be merged. 4 and 5 are mostly
> useful in conjunction with 6 and 7 (which are not ready yet), so I'll let
> maintainers decide.

I've applied 3, 4, and 5 to drm-misc-next. Patch 2 should go via Mediatek tree.

Rob

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183
  2020-02-25 17:16   ` Rob Herring
@ 2020-02-26  0:55     ` Nicolas Boichat
  0 siblings, 0 replies; 35+ messages in thread
From: Nicolas Boichat @ 2020-02-26  0:55 UTC (permalink / raw)
  To: Rob Herring, Nick Fan, Sj Huang
  Cc: Mark Rutland, Devicetree List, Ulf Hansson, Tomeu Vizoso,
	David Airlie, lkml, Liam Girdwood, dri-devel, Steven Price,
	Mark Brown, moderated list:ARM/Mediatek SoC support,
	Alyssa Rosenzweig, Daniel Vetter, Hsin-Yi Wang, Matthias Brugger,
	linux-arm Mailing List

+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

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, back to index

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-02-07  5:26 [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Nicolas Boichat
2020-02-07  5:26 ` [PATCH v4 1/7] dt-bindings: gpu: mali-bifrost: Add Mediatek MT8183 Nicolas Boichat
2020-02-25 17:16   ` Rob Herring
2020-02-26  0:55     ` Nicolas Boichat
2020-02-07  5:26 ` [PATCH v4 2/7] arm64: dts: mt8183: Add node for the Mali GPU Nicolas Boichat
2020-02-07  5:26 ` [PATCH v4 3/7] drm/panfrost: Improve error reporting in panfrost_gpu_power_on Nicolas Boichat
2020-02-12 10:38   ` Matthias Brugger
2020-02-07  5:26 ` [PATCH v4 4/7] drm/panfrost: Add support for multiple regulators Nicolas Boichat
2020-02-10 11:31   ` Steven Price
2020-02-10 14:00   ` Mark Brown
2020-02-07  5:26 ` [PATCH v4 5/7] drm/panfrost: Add support for multiple power domains Nicolas Boichat
2020-02-07 13:52   ` Alyssa Rosenzweig
2020-02-09 12:43     ` Nicolas Boichat
2020-02-07 14:25   ` Ulf Hansson
2020-02-09 12:50     ` Nicolas Boichat
2020-02-10  7:50       ` Ulf Hansson
2020-02-10 11:39   ` Steven Price
2020-02-11 19:43   ` Rob Herring
2020-02-11 20:08     ` Saravana Kannan
2020-02-12  1:58       ` Rob Herring
2020-02-12  2:10         ` Saravana Kannan
2020-02-12  2:08       ` Nicolas Boichat
2020-02-07  5:26 ` [PATCH v4 6/7] RFC: drm/panfrost: Add mt8183-mali compatible string Nicolas Boichat
2020-02-07  5:26 ` [PATCH v4 7/7] RFC: drm/panfrost: devfreq: Add support for 2 regulators Nicolas Boichat
2020-02-12  8:49   ` Nicolas Boichat
2020-02-12  9:06     ` Viresh Kumar
2020-02-12 18:14     ` Rob Herring
2020-02-13  7:57       ` Nicolas Boichat
2020-02-13  8:24         ` Nicolas Boichat
2020-02-07  6:17 ` [PATCH v4 0/7] Add dts for mt8183 GPU (and misc panfrost patches) Tomeu Vizoso
2020-02-07  7:42   ` Nicolas Boichat
2020-02-07  8:13     ` Tomeu Vizoso
2020-02-10  3:39       ` Nicolas Boichat
2020-02-10  8:17         ` Tomeu Vizoso
2020-02-25 20:35 ` Rob Herring

Linux-mediatek Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mediatek/0 linux-mediatek/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mediatek linux-mediatek/ https://lore.kernel.org/linux-mediatek \
		linux-mediatek@lists.infradead.org
	public-inbox-index linux-mediatek

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mediatek


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git