[PATCH 00/15][RFC] Add regulator devfreq support to Panfrost

[PATCH 00/15][RFC] Add regulator devfreq support to Panfrost

[Clément Péron]

Hi,

This serie cleans and adds regulator support to Panfrost devfreq.
This is mostly based on comment for the freshly introduced lima
devfreq.

We need to add regulator support because on Allwinner the GPU OPP
table defines both frequencies and voltages.

First patches [01-08] should not change the actual behavior
and introduce a proper panfrost_devfreq struct.

Fatches after are WIP and add regulator support.

However I got several issues first we need to avoid getting regulator
if devfreq get by itself the regulator, but as of today the OPP
framework only get and don't enable the regulator...
An HACK for now is to add regulator-always-on in the device-tree.

Then when I enable devfreq I got several faults like.
I'm totally noob on GPU sched/fault and couldn't be helpfull with this.

I got this running glmark2 on T720 (Allwinner H6) with Mesa 20.0.5.
# glmark2-es2-drm
=======================================================
    glmark2 2017.07
=======================================================
    OpenGL Information
    GL_VENDOR:     Panfrost
    GL_RENDERER:   Mali T720 (Panfrost)
    GL_VERSION:    OpenGL ES 2.0 Mesa 20.0.5
=======================================================

[   93.550063] panfrost 1800000.gpu: GPU Fault 0x00000088 (UNKNOWN) at 0x0000000080117100
[   94.045401] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x8, head=0x21d6c00, tail=0x21d6c00, sched_job=00000000e3c2132f

[  328.871070] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
[  328.871070] Reason: TODO
[  328.871070] raw fault status: 0xAA0003C2
[  328.871070] decoded fault status: SLAVE FAULT
[  328.871070] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
[  328.871070] access type 0x3: WRITE
[  328.871070] source id 0xAA00
[  329.373327] panfrost 1800000.gpu: gpu sched timeout, js=1, config=0x3700, status=0x8, head=0xa1a4900, tail=0xa1a4900, sched_job=000000007ac31097
[  329.386527] panfrost 1800000.gpu: js fault, js=0, status=DATA_INVALID_FAULT, head=0xa1a4c00, tail=0xa1a4c00
[  329.396293] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x58, head=0xa1a4c00, tail=0xa1a4c00, sched_job=0000000004c90381
[  329.411521] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
[  329.411521] Reason: TODO
[  329.411521] raw fault status: 0xAA0003C2
[  329.411521] decoded fault status: SLAVE FAULT
[  329.411521] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
[  329.411521] access type 0x3: WRITE
[  329.411521] source id 0xAA00

Thanks for your reviews, help on this serie,
Clement

Clément Péron (15):
  drm/panfrost: avoid static declaration
  drm/panfrost: clean headers in devfreq
  drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle
  drm/panfrost: introduce panfrost_devfreq struct
  drm/panfrost: use spinlock instead of atomic
  drm/panfrost: properly handle error in probe
  drm/panfrost: use device_property_present to check for OPP
  drm/panfrost: move devfreq_init()/fini() in device
  drm/panfrost: dynamically alloc regulators
  drm/panfrost: add regulators to devfreq
  drm/panfrost: set devfreq clock name
  arm64: defconfig: Enable devfreq cooling device
  arm64: dts: allwinner: h6: Add cooling map for GPU
  [DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table
  [DO NOT MERGE] arm64: dts: allwinner: force GPU regulator to be always

 .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |   1 +
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 102 ++++++++++
 arch/arm64/configs/defconfig                  |   1 +
 drivers/gpu/drm/panfrost/panfrost_devfreq.c   | 190 ++++++++++++------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h   |  32 ++-
 drivers/gpu/drm/panfrost/panfrost_device.c    |  56 ++++--
 drivers/gpu/drm/panfrost/panfrost_device.h    |  14 +-
 drivers/gpu/drm/panfrost/panfrost_drv.c       |  15 +-
 drivers/gpu/drm/panfrost/panfrost_job.c       |  10 +-
 9 files changed, 310 insertions(+), 111 deletions(-)

-- 
2.20.1

[PATCH 01/15] drm/panfrost: avoid static declaration

[Clément Péron]

This declaration can be avoided so change it.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 38 ++++++++++-----------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 413987038fbf..1b560b903ea6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -14,7 +14,24 @@
 #include "panfrost_gpu.h"
 #include "panfrost_regs.h"
 
-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev);
+static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
+{
+	ktime_t now;
+	ktime_t last;
+
+	if (!pfdev->devfreq.devfreq)
+		return;
+
+	now = ktime_get();
+	last = pfdev->devfreq.time_last_update;
+
+	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
+		pfdev->devfreq.busy_time += ktime_sub(now, last);
+	else
+		pfdev->devfreq.idle_time += ktime_sub(now, last);
+
+	pfdev->devfreq.time_last_update = now;
+}
 
 static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 				   u32 flags)
@@ -139,25 +156,6 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
 	devfreq_suspend_device(pfdev->devfreq.devfreq);
 }
 
-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
-{
-	ktime_t now;
-	ktime_t last;
-
-	if (!pfdev->devfreq.devfreq)
-		return;
-
-	now = ktime_get();
-	last = pfdev->devfreq.time_last_update;
-
-	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
-		pfdev->devfreq.busy_time += ktime_sub(now, last);
-	else
-		pfdev->devfreq.idle_time += ktime_sub(now, last);
-
-	pfdev->devfreq.time_last_update = now;
-}
-
 void panfrost_devfreq_record_busy(struct panfrost_device *pfdev)
 {
 	panfrost_devfreq_update_utilization(pfdev);
-- 
2.20.1

[PATCH 02/15] drm/panfrost: clean headers in devfreq

[Clément Péron]

Don't include not required headers and sort them.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 1b560b903ea6..df7b71da9a84 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -1,18 +1,14 @@
 // SPDX-License-Identifier: GPL-2.0
 /* Copyright 2019 Collabora ltd. */
+
+#include <linux/clk.h>
 #include <linux/devfreq.h>
 #include <linux/devfreq_cooling.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
-#include <linux/clk.h>
-#include <linux/regulator/consumer.h>
 
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
-#include "panfrost_features.h"
-#include "panfrost_issues.h"
-#include "panfrost_gpu.h"
-#include "panfrost_regs.h"
 
 static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
 {
-- 
2.20.1

[PATCH 03/15] drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle

[Clément Péron]

This use devfreq variable that will be lock with spinlock in future
patches. We should either introduce a function to access this one
but as devfreq is optional let's just remove it.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_job.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 7914b1570841..63e32a9f2749 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -581,10 +581,6 @@ int panfrost_job_is_idle(struct panfrost_device *pfdev)
 	struct panfrost_job_slot *js = pfdev->js;
 	int i;
 
-	/* Check whether the hardware is idle */
-	if (atomic_read(&pfdev->devfreq.busy_count))
-		return false;
-
 	for (i = 0; i < NUM_JOB_SLOTS; i++) {
 		/* If there are any jobs in the HW queue, we're not idle */
 		if (atomic_read(&js->queue[i].sched.hw_rq_count))
-- 
2.20.1

[PATCH 04/15] drm/panfrost: introduce panfrost_devfreq struct

[Clément Péron]

Introduce a proper panfrost_devfreq to deal with devfreq variables.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 76 ++++++++++++---------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h | 20 +++++-
 drivers/gpu/drm/panfrost/panfrost_device.h  | 11 +--
 drivers/gpu/drm/panfrost/panfrost_job.c     |  6 +-
 4 files changed, 66 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index df7b71da9a84..962550363391 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -10,23 +10,23 @@
 #include "panfrost_device.h"
 #include "panfrost_devfreq.h"
 
-static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
+static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfreq)
 {
 	ktime_t now;
 	ktime_t last;
 
-	if (!pfdev->devfreq.devfreq)
+	if (!pfdevfreq->devfreq)
 		return;
 
 	now = ktime_get();
-	last = pfdev->devfreq.time_last_update;
+	last = pfdevfreq->time_last_update;
 
-	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
-		pfdev->devfreq.busy_time += ktime_sub(now, last);
+	if (atomic_read(&pfdevfreq->busy_count) > 0)
+		pfdevfreq->busy_time += ktime_sub(now, last);
 	else
-		pfdev->devfreq.idle_time += ktime_sub(now, last);
+		pfdevfreq->idle_time += ktime_sub(now, last);
 
-	pfdev->devfreq.time_last_update = now;
+	pfdevfreq->time_last_update = now;
 }
 
 static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
@@ -47,30 +47,31 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
 	return 0;
 }
 
-static void panfrost_devfreq_reset(struct panfrost_device *pfdev)
+static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq)
 {
-	pfdev->devfreq.busy_time = 0;
-	pfdev->devfreq.idle_time = 0;
-	pfdev->devfreq.time_last_update = ktime_get();
+	pfdevfreq->busy_time = 0;
+	pfdevfreq->idle_time = 0;
+	pfdevfreq->time_last_update = ktime_get();
 }
 
 static int panfrost_devfreq_get_dev_status(struct device *dev,
 					   struct devfreq_dev_status *status)
 {
 	struct panfrost_device *pfdev = dev_get_drvdata(dev);
+	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
 
-	panfrost_devfreq_update_utilization(pfdev);
+	panfrost_devfreq_update_utilization(pfdevfreq);
 
 	status->current_frequency = clk_get_rate(pfdev->clock);
-	status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.busy_time,
-						   pfdev->devfreq.idle_time));
+	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
+						   pfdevfreq->idle_time));
 
-	status->busy_time = ktime_to_ns(pfdev->devfreq.busy_time);
+	status->busy_time = ktime_to_ns(pfdevfreq->busy_time);
 
-	panfrost_devfreq_reset(pfdev);
+	panfrost_devfreq_reset(pfdevfreq);
 
-	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n", status->busy_time,
-		status->total_time,
+	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
+		status->busy_time, status->total_time,
 		status->busy_time / (status->total_time / 100),
 		status->current_frequency / 1000 / 1000);
 
@@ -91,6 +92,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	struct device *dev = &pfdev->pdev->dev;
 	struct devfreq *devfreq;
 	struct thermal_cooling_device *cooling;
+	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
 
 	ret = dev_pm_opp_of_add_table(dev);
 	if (ret == -ENODEV) /* Optional, continue without devfreq */
@@ -98,7 +100,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	else if (ret)
 		return ret;
 
-	panfrost_devfreq_reset(pfdev);
+	panfrost_devfreq_reset(pfdevfreq);
 
 	cur_freq = clk_get_rate(pfdev->clock);
 
@@ -116,53 +118,59 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 		dev_pm_opp_of_remove_table(dev);
 		return PTR_ERR(devfreq);
 	}
-	pfdev->devfreq.devfreq = devfreq;
+	pfdevfreq->devfreq = devfreq;
 
 	cooling = of_devfreq_cooling_register(dev->of_node, devfreq);
 	if (IS_ERR(cooling))
 		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
 	else
-		pfdev->devfreq.cooling = cooling;
+		pfdevfreq->cooling = cooling;
 
 	return 0;
 }
 
 void panfrost_devfreq_fini(struct panfrost_device *pfdev)
 {
-	if (pfdev->devfreq.cooling)
-		devfreq_cooling_unregister(pfdev->devfreq.cooling);
+	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
+
+	if (pfdevfreq->cooling)
+		devfreq_cooling_unregister(pfdevfreq->cooling);
 	dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
 }
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
 {
-	if (!pfdev->devfreq.devfreq)
+	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
+
+	if (!pfdevfreq->devfreq)
 		return;
 
-	panfrost_devfreq_reset(pfdev);
+	panfrost_devfreq_reset(pfdevfreq);
 
-	devfreq_resume_device(pfdev->devfreq.devfreq);
+	devfreq_resume_device(pfdevfreq->devfreq);
 }
 
 void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
 {
-	if (!pfdev->devfreq.devfreq)
+	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
+
+	if (!pfdevfreq->devfreq)
 		return;
 
-	devfreq_suspend_device(pfdev->devfreq.devfreq);
+	devfreq_suspend_device(pfdevfreq->devfreq);
 }
 
-void panfrost_devfreq_record_busy(struct panfrost_device *pfdev)
+void panfrost_devfreq_record_busy(struct panfrost_devfreq *pfdevfreq)
 {
-	panfrost_devfreq_update_utilization(pfdev);
-	atomic_inc(&pfdev->devfreq.busy_count);
+	panfrost_devfreq_update_utilization(pfdevfreq);
+	atomic_inc(&pfdevfreq->busy_count);
 }
 
-void panfrost_devfreq_record_idle(struct panfrost_device *pfdev)
+void panfrost_devfreq_record_idle(struct panfrost_devfreq *pfdevfreq)
 {
 	int count;
 
-	panfrost_devfreq_update_utilization(pfdev);
-	count = atomic_dec_if_positive(&pfdev->devfreq.busy_count);
+	panfrost_devfreq_update_utilization(pfdevfreq);
+	count = atomic_dec_if_positive(&pfdevfreq->busy_count);
 	WARN_ON(count < 0);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index 0611beffc8d0..0697f8d5aa34 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -4,13 +4,29 @@
 #ifndef __PANFROST_DEVFREQ_H__
 #define __PANFROST_DEVFREQ_H__
 
+#include <linux/ktime.h>
+
+struct devfreq;
+struct thermal_cooling_device;
+
+struct panfrost_device;
+
+struct panfrost_devfreq {
+	struct devfreq *devfreq;
+	struct thermal_cooling_device *cooling;
+	ktime_t busy_time;
+	ktime_t idle_time;
+	ktime_t time_last_update;
+	atomic_t busy_count;
+};
+
 int panfrost_devfreq_init(struct panfrost_device *pfdev);
 void panfrost_devfreq_fini(struct panfrost_device *pfdev);
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev);
 void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
 
-void panfrost_devfreq_record_busy(struct panfrost_device *pfdev);
-void panfrost_devfreq_record_idle(struct panfrost_device *pfdev);
+void panfrost_devfreq_record_busy(struct panfrost_devfreq *devfreq);
+void panfrost_devfreq_record_idle(struct panfrost_devfreq *devfreq);
 
 #endif /* __PANFROST_DEVFREQ_H__ */
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index c30c719a8059..2efa59c9d1c5 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -13,6 +13,8 @@
 #include <drm/drm_mm.h>
 #include <drm/gpu_scheduler.h>
 
+#include "panfrost_devfreq.h"
+
 struct panfrost_device;
 struct panfrost_mmu;
 struct panfrost_job_slot;
@@ -107,14 +109,7 @@ struct panfrost_device {
 	struct list_head shrinker_list;
 	struct shrinker shrinker;
 
-	struct {
-		struct devfreq *devfreq;
-		struct thermal_cooling_device *cooling;
-		ktime_t busy_time;
-		ktime_t idle_time;
-		ktime_t time_last_update;
-		atomic_t busy_count;
-	} devfreq;
+	struct panfrost_devfreq pfdevfreq;
 };
 
 struct panfrost_mmu {
diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
index 63e32a9f2749..a67f3eac6a58 100644
--- a/drivers/gpu/drm/panfrost/panfrost_job.c
+++ b/drivers/gpu/drm/panfrost/panfrost_job.c
@@ -155,7 +155,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
 	}
 
 	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
-	panfrost_devfreq_record_busy(pfdev);
+	panfrost_devfreq_record_busy(&pfdev->pfdevfreq);
 
 	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
 	job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);
@@ -415,7 +415,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
 	}
 	spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
 
-	panfrost_devfreq_record_idle(pfdev);
+	panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 	panfrost_device_reset(pfdev);
 
 	for (i = 0; i < NUM_JOB_SLOTS; i++)
@@ -478,7 +478,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
 				pfdev->jobs[j] = NULL;
 
 				panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
-				panfrost_devfreq_record_idle(pfdev);
+				panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
 
 				dma_fence_signal_locked(job->done_fence);
 				pm_runtime_put_autosuspend(pfdev->dev);
-- 
2.20.1

[PATCH 05/15] drm/panfrost: use spinlock instead of atomic

[Clément Péron]

Convert busy_count to a simple int protected by spinlock.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 43 +++++++++++++++------
 drivers/gpu/drm/panfrost/panfrost_devfreq.h | 10 ++++-
 2 files changed, 41 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 962550363391..78753cfb59fb 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -12,16 +12,12 @@
 
 static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfreq)
 {
-	ktime_t now;
-	ktime_t last;
-
-	if (!pfdevfreq->devfreq)
-		return;
+	ktime_t now, last;
 
 	now = ktime_get();
 	last = pfdevfreq->time_last_update;
 
-	if (atomic_read(&pfdevfreq->busy_count) > 0)
+	if (pfdevfreq->busy_count > 0)
 		pfdevfreq->busy_time += ktime_sub(now, last);
 	else
 		pfdevfreq->idle_time += ktime_sub(now, last);
@@ -59,10 +55,14 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 {
 	struct panfrost_device *pfdev = dev_get_drvdata(dev);
 	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
+	unsigned long irqflags;
+
+	status->current_frequency = clk_get_rate(pfdev->clock);
+
+	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
 
 	panfrost_devfreq_update_utilization(pfdevfreq);
 
-	status->current_frequency = clk_get_rate(pfdev->clock);
 	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
 						   pfdevfreq->idle_time));
 
@@ -70,6 +70,8 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
 
 	panfrost_devfreq_reset(pfdevfreq);
 
+	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
+
 	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
 		status->busy_time, status->total_time,
 		status->busy_time / (status->total_time / 100),
@@ -100,6 +102,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	else if (ret)
 		return ret;
 
+	spin_lock_init(&pfdevfreq->lock);
+
 	panfrost_devfreq_reset(pfdevfreq);
 
 	cur_freq = clk_get_rate(pfdev->clock);
@@ -162,15 +166,32 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
 
 void panfrost_devfreq_record_busy(struct panfrost_devfreq *pfdevfreq)
 {
+	unsigned long irqflags;
+
+	if (!pfdevfreq->devfreq)
+		return;
+
+	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
+
 	panfrost_devfreq_update_utilization(pfdevfreq);
-	atomic_inc(&pfdevfreq->busy_count);
+
+	pfdevfreq->busy_count++;
+
+	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
 }
 
 void panfrost_devfreq_record_idle(struct panfrost_devfreq *pfdevfreq)
 {
-	int count;
+	unsigned long irqflags;
+
+	if (!pfdevfreq->devfreq)
+		return;
+
+	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
 
 	panfrost_devfreq_update_utilization(pfdevfreq);
-	count = atomic_dec_if_positive(&pfdevfreq->busy_count);
-	WARN_ON(count < 0);
+
+	WARN_ON(--pfdevfreq->busy_count < 0);
+
+	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index 0697f8d5aa34..e6629900a618 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -4,6 +4,7 @@
 #ifndef __PANFROST_DEVFREQ_H__
 #define __PANFROST_DEVFREQ_H__
 
+#include <linux/spinlock.h>
 #include <linux/ktime.h>
 
 struct devfreq;
@@ -14,10 +15,17 @@ struct panfrost_device;
 struct panfrost_devfreq {
 	struct devfreq *devfreq;
 	struct thermal_cooling_device *cooling;
+
 	ktime_t busy_time;
 	ktime_t idle_time;
 	ktime_t time_last_update;
-	atomic_t busy_count;
+	int busy_count;
+	/*
+	 * Protect busy_time, idle_time, time_last_update and busy_count
+	 * because these can be updated concurrently, for example by the GP
+	 * and PP interrupts.
+	 */
+	spinlock_t lock;
 };
 
 int panfrost_devfreq_init(struct panfrost_device *pfdev);
-- 
2.20.1

[PATCH 06/15] drm/panfrost: properly handle error in probe

[Clément Péron]

Introduce a boolean to know if opp table has been added.

With this, we can call panfrost_devfreq_fini() in case of error
and release what has been initialised.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 25 ++++++++++++++++-----
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |  1 +
 2 files changed, 20 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 78753cfb59fb..d9007f44b772 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -101,6 +101,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 		return 0;
 	else if (ret)
 		return ret;
+	pfdevfreq->opp_of_table_added = true;
 
 	spin_lock_init(&pfdevfreq->lock);
 
@@ -109,8 +110,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	cur_freq = clk_get_rate(pfdev->clock);
 
 	opp = devfreq_recommended_opp(dev, &cur_freq, 0);
-	if (IS_ERR(opp))
-		return PTR_ERR(opp);
+	if (IS_ERR(opp)) {
+		ret = PTR_ERR(opp);
+		goto err_fini;
+	}
 
 	panfrost_devfreq_profile.initial_freq = cur_freq;
 	dev_pm_opp_put(opp);
@@ -119,8 +122,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 					  DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
 	if (IS_ERR(devfreq)) {
 		DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
-		dev_pm_opp_of_remove_table(dev);
-		return PTR_ERR(devfreq);
+		ret = PTR_ERR(devfreq);
+		goto err_fini;
 	}
 	pfdevfreq->devfreq = devfreq;
 
@@ -131,15 +134,25 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 		pfdevfreq->cooling = cooling;
 
 	return 0;
+
+err_fini:
+	panfrost_devfreq_fini(pfdev);
+	return ret;
 }
 
 void panfrost_devfreq_fini(struct panfrost_device *pfdev)
 {
 	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
 
-	if (pfdevfreq->cooling)
+	if (pfdevfreq->cooling) {
 		devfreq_cooling_unregister(pfdevfreq->cooling);
-	dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
+		pfdevfreq->cooling = NULL;
+	}
+
+	if (pfdevfreq->opp_of_table_added) {
+		dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
+		pfdevfreq->opp_of_table_added = false;
+	}
 }
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index e6629900a618..add203cb00c2 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -15,6 +15,7 @@ struct panfrost_device;
 struct panfrost_devfreq {
 	struct devfreq *devfreq;
 	struct thermal_cooling_device *cooling;
+	bool opp_of_table_added;
 
 	ktime_t busy_time;
 	ktime_t idle_time;
-- 
2.20.1

[PATCH 07/15] drm/panfrost: use device_property_present to check for OPP

[Clément Péron]

Instead of expecting an error from dev_pm_opp_of_add_table()
do a simple device_property_present() check.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index d9007f44b772..fce21c682414 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	struct thermal_cooling_device *cooling;
 	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
 
-	ret = dev_pm_opp_of_add_table(dev);
-	if (ret == -ENODEV) /* Optional, continue without devfreq */
+	if (!device_property_present(dev, "operating-points-v2"))
+		/* Optional, continue without devfreq */
 		return 0;
-	else if (ret)
-		return ret;
-	pfdevfreq->opp_of_table_added = true;
 
 	spin_lock_init(&pfdevfreq->lock);
 
+	ret = dev_pm_opp_of_add_table(dev);
+	if (ret) {
+		DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
+		goto err_fini;
+	}
+	pfdevfreq->opp_of_table_added = true;
+
 	panfrost_devfreq_reset(pfdevfreq);
 
 	cur_freq = clk_get_rate(pfdev->clock);
-- 
2.20.1

[PATCH 08/15] drm/panfrost: move devfreq_init()/fini() in device

[Clément Péron]

Later we will introduce devfreq probing regulator if they
are present. As regulator should be probe only one time we
need to get this logic in the device_init().

panfrost_device is already taking care of devfreq_resume()
and devfreq_suspend(), so it's not totally illogic to move
the devfreq_init() and devfreq_fini() here.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++--------
 drivers/gpu/drm/panfrost/panfrost_drv.c    | 15 ++-------
 2 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 8136babd3ba9..f480127205d6 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 		return err;
 	}
 
+	err = panfrost_devfreq_init(pfdev);
+	if (err) {
+		dev_err(pfdev->dev, "devfreq init failed %d\n", err);
+		goto err_out0;
+	}
+
 	err = panfrost_regulator_init(pfdev);
 	if (err) {
 		dev_err(pfdev->dev, "regulator init failed %d\n", err);
-		goto err_out0;
+		goto err_out1;
 	}
 
 	err = panfrost_reset_init(pfdev);
 	if (err) {
 		dev_err(pfdev->dev, "reset init failed %d\n", err);
-		goto err_out1;
+		goto err_out2;
 	}
 
 	err = panfrost_pm_domain_init(pfdev);
 	if (err)
-		goto err_out2;
+		goto err_out3;
 
 	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_out3;
+		goto err_out4;
 	}
 
 	err = panfrost_gpu_init(pfdev);
 	if (err)
-		goto err_out3;
+		goto err_out4;
 
 	err = panfrost_mmu_init(pfdev);
 	if (err)
-		goto err_out4;
+		goto err_out5;
 
 	err = panfrost_job_init(pfdev);
 	if (err)
-		goto err_out5;
+		goto err_out6;
 
 	err = panfrost_perfcnt_init(pfdev);
 	if (err)
-		goto err_out6;
+		goto err_out7;
 
 	return 0;
-err_out6:
+err_out7:
 	panfrost_job_fini(pfdev);
-err_out5:
+err_out6:
 	panfrost_mmu_fini(pfdev);
-err_out4:
+err_out5:
 	panfrost_gpu_fini(pfdev);
-err_out3:
+err_out4:
 	panfrost_pm_domain_fini(pfdev);
-err_out2:
+err_out3:
 	panfrost_reset_fini(pfdev);
-err_out1:
+err_out2:
 	panfrost_regulator_fini(pfdev);
+err_out1:
+	panfrost_devfreq_fini(pfdev);
 err_out0:
 	panfrost_clk_fini(pfdev);
 	return err;
@@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
 	panfrost_gpu_fini(pfdev);
 	panfrost_pm_domain_fini(pfdev);
 	panfrost_reset_fini(pfdev);
+	panfrost_devfreq_fini(pfdev);
 	panfrost_regulator_fini(pfdev);
 	panfrost_clk_fini(pfdev);
 }
diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
index 882fecc33fdb..4dda68689015 100644
--- a/drivers/gpu/drm/panfrost/panfrost_drv.c
+++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
@@ -14,7 +14,6 @@
 #include <drm/drm_utils.h>
 
 #include "panfrost_device.h"
-#include "panfrost_devfreq.h"
 #include "panfrost_gem.h"
 #include "panfrost_mmu.h"
 #include "panfrost_job.h"
@@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev)
 		goto err_out0;
 	}
 
-	err = panfrost_devfreq_init(pfdev);
-	if (err) {
-		if (err != -EPROBE_DEFER)
-			dev_err(&pdev->dev, "Fatal error during devfreq init\n");
-		goto err_out1;
-	}
-
 	pm_runtime_set_active(pfdev->dev);
 	pm_runtime_mark_last_busy(pfdev->dev);
 	pm_runtime_enable(pfdev->dev);
@@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev)
 	 */
 	err = drm_dev_register(ddev, 0);
 	if (err < 0)
-		goto err_out2;
+		goto err_out1;
 
 	panfrost_gem_shrinker_init(ddev);
 
 	return 0;
 
-err_out2:
-	pm_runtime_disable(pfdev->dev);
-	panfrost_devfreq_fini(pfdev);
 err_out1:
+	pm_runtime_disable(pfdev->dev);
 	panfrost_device_fini(pfdev);
 err_out0:
 	drm_dev_put(ddev);
@@ -650,7 +640,6 @@ static int panfrost_remove(struct platform_device *pdev)
 	panfrost_gem_shrinker_cleanup(ddev);
 
 	pm_runtime_get_sync(pfdev->dev);
-	panfrost_devfreq_fini(pfdev);
 	panfrost_device_fini(pfdev);
 	pm_runtime_put_sync_suspend(pfdev->dev);
 	pm_runtime_disable(pfdev->dev);
-- 
2.20.1

[PATCH 09/15] drm/panfrost: dynamically alloc regulators

[Clément Péron]

We will later introduce regulators managed by OPP.

Only alloc regulators when it's needed. This also help use
to release the regulators only when they are allocated.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_device.c | 14 +++++++++-----
 drivers/gpu/drm/panfrost/panfrost_device.h |  3 +--
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index f480127205d6..67eedf64e82d 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -90,9 +90,11 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 {
 	int ret, i;
 
-	if (WARN(pfdev->comp->num_supplies > ARRAY_SIZE(pfdev->regulators),
-			"Too many supplies in compatible structure.\n"))
-		return -EINVAL;
+	pfdev->regulators = devm_kcalloc(pfdev->dev, pfdev->comp->num_supplies,
+					 sizeof(*pfdev->regulators),
+					 GFP_KERNEL);
+	if (!pfdev->regulators)
+		return -ENOMEM;
 
 	for (i = 0; i < pfdev->comp->num_supplies; i++)
 		pfdev->regulators[i].supply = pfdev->comp->supply_names[i];
@@ -117,8 +119,10 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
 
 static void panfrost_regulator_fini(struct panfrost_device *pfdev)
 {
-	regulator_bulk_disable(pfdev->comp->num_supplies,
-			pfdev->regulators);
+	if (!pfdev->regulators)
+		return;
+
+	regulator_bulk_disable(pfdev->comp->num_supplies, pfdev->regulators);
 }
 
 static void panfrost_pm_domain_fini(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
index 2efa59c9d1c5..953f7536a773 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.h
+++ b/drivers/gpu/drm/panfrost/panfrost_device.h
@@ -22,7 +22,6 @@ struct panfrost_job;
 struct panfrost_perfcnt;
 
 #define NUM_JOB_SLOTS 3
-#define MAX_REGULATORS 2
 #define MAX_PM_DOMAINS 3
 
 struct panfrost_features {
@@ -81,7 +80,7 @@ struct panfrost_device {
 	void __iomem *iomem;
 	struct clk *clock;
 	struct clk *bus_clock;
-	struct regulator_bulk_data regulators[MAX_REGULATORS];
+	struct regulator_bulk_data *regulators;
 	struct reset_control *rstc;
 	/* pm_domains for devices with more than one. */
 	struct device *pm_domain_devs[MAX_PM_DOMAINS];
-- 
2.20.1

[PATCH 10/15] drm/panfrost: add regulators to devfreq

[Clément Péron]

Some OPP tables specify voltage for each frequency. Devfreq can
handle these regulators but they should be get only 1 time to avoid
issue and know who is in charge.

If OPP table is probe don't init regulator.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 19 +++++++++++++++++++
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |  2 ++
 drivers/gpu/drm/panfrost/panfrost_device.c  | 11 +++++++----
 3 files changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index fce21c682414..9ffea0d4a087 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -93,6 +93,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 	unsigned long cur_freq;
 	struct device *dev = &pfdev->pdev->dev;
 	struct devfreq *devfreq;
+	struct opp_table *opp_table;
 	struct thermal_cooling_device *cooling;
 	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
 
@@ -102,6 +103,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 
 	spin_lock_init(&pfdevfreq->lock);
 
+	opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
+					      pfdev->comp->num_supplies);
+	if (IS_ERR(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		/* Continue if the optional regulator is missing */
+		if (ret != -ENODEV) {
+			DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
+			goto err_fini;
+		}
+	} else {
+		pfdevfreq->regulators_opp_table = opp_table;
+	}
+
 	ret = dev_pm_opp_of_add_table(dev);
 	if (ret) {
 		DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
@@ -157,6 +171,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
 		dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
 		pfdevfreq->opp_of_table_added = false;
 	}
+
+	if (pfdevfreq->regulators_opp_table) {
+		dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
+		pfdevfreq->regulators_opp_table = NULL;
+	}
 }
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index add203cb00c2..347cde4786cf 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -8,12 +8,14 @@
 #include <linux/ktime.h>
 
 struct devfreq;
+struct opp_table;
 struct thermal_cooling_device;
 
 struct panfrost_device;
 
 struct panfrost_devfreq {
 	struct devfreq *devfreq;
+	struct opp_table *regulators_opp_table;
 	struct thermal_cooling_device *cooling;
 	bool opp_of_table_added;
 
diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
index 67eedf64e82d..8b17fb2e3369 100644
--- a/drivers/gpu/drm/panfrost/panfrost_device.c
+++ b/drivers/gpu/drm/panfrost/panfrost_device.c
@@ -222,10 +222,13 @@ int panfrost_device_init(struct panfrost_device *pfdev)
 		goto err_out0;
 	}
 
-	err = panfrost_regulator_init(pfdev);
-	if (err) {
-		dev_err(pfdev->dev, "regulator init failed %d\n", err);
-		goto err_out1;
+	/* OPP will handle regulators */
+	if (!pfdev->pfdevfreq.opp_of_table_added) {
+		err = panfrost_regulator_init(pfdev);
+		if (err) {
+			dev_err(pfdev->dev, "regulator init failed %d\n", err);
+			goto err_out1;
+		}
 	}
 
 	err = panfrost_reset_init(pfdev);
-- 
2.20.1

[PATCH 11/15] drm/panfrost: set devfreq clock name

[Clément Péron]

Some SoCs have  several clocks defined and the OPP core
needs to know the exact name of the clk to use.

Set the clock name to "core".

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 +++++++++++++
 drivers/gpu/drm/panfrost/panfrost_devfreq.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
index 9ffea0d4a087..6bf3541b4d53 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
@@ -103,6 +103,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
 
 	spin_lock_init(&pfdevfreq->lock);
 
+	opp_table = dev_pm_opp_set_clkname(dev, "core");
+	if (IS_ERR(opp_table)) {
+		ret = PTR_ERR(opp_table);
+		goto err_fini;
+	}
+
+	pfdevfreq->clkname_opp_table = opp_table;
+
 	opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
 					      pfdev->comp->num_supplies);
 	if (IS_ERR(opp_table)) {
@@ -176,6 +184,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
 		dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
 		pfdevfreq->regulators_opp_table = NULL;
 	}
+
+	if (pfdevfreq->clkname_opp_table) {
+		dev_pm_opp_put_clkname(pfdevfreq->clkname_opp_table);
+		pfdevfreq->clkname_opp_table = NULL;
+	}
 }
 
 void panfrost_devfreq_resume(struct panfrost_device *pfdev)
diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
index 347cde4786cf..1f2475e1d034 100644
--- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
+++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
@@ -16,6 +16,7 @@ struct panfrost_device;
 struct panfrost_devfreq {
 	struct devfreq *devfreq;
 	struct opp_table *regulators_opp_table;
+	struct opp_table *clkname_opp_table;
 	struct thermal_cooling_device *cooling;
 	bool opp_of_table_added;
 
-- 
2.20.1

[PATCH 12/15] arm64: defconfig: Enable devfreq cooling device

[Clément Péron]

Devfreq cooling device framework is used in Panfrost
to throttle GPU in order to regulate its temperature.

Enable this driver for ARM64 SoC.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 66941024418c..42d85c2c0945 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -496,6 +496,7 @@ CONFIG_SENSORS_INA2XX=m
 CONFIG_SENSORS_INA3221=m
 CONFIG_THERMAL_GOV_POWER_ALLOCATOR=y
 CONFIG_CPU_THERMAL=y
+CONFIG_DEVFREQ_THERMAL=y
 CONFIG_THERMAL_EMULATION=y
 CONFIG_QORIQ_THERMAL=m
 CONFIG_SUN8I_THERMAL=y
-- 
2.20.1

[PATCH 13/15] arm64: dts: allwinner: h6: Add cooling map for GPU

[Clément Péron]

Add a simple cooling map for the GPU.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 22 ++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index 2e31632c6ca8..b26f735201c7 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -173,6 +173,7 @@
 			clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
 			clock-names = "core", "bus";
 			resets = <&ccu RST_BUS_GPU>;
+			#cooling-cells = <2>;
 			status = "disabled";
 		};
 
@@ -1002,6 +1003,27 @@
 			polling-delay-passive = <0>;
 			polling-delay = <0>;
 			thermal-sensors = <&ths 1>;
+
+			trips {
+				gpu_alert: gpu-alert {
+					temperature = <85000>;
+					hysteresis = <2000>;
+					type = "passive";
+				};
+
+				gpu-crit {
+					temperature = <100000>;
+					hysteresis = <0>;
+					type = "critical";
+				};
+			};
+
+			cooling-maps {
+				map0 {
+					trip = <&gpu_alert>;
+					cooling-device = <&gpu THERMAL_NO_LIMIT THERMAL_NO_LIMIT>;
+				};
+			};
 		};
 	};
 };
-- 
2.20.1

[PATCH 14/15] [DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table

[Clément Péron]

Add an Operating Performance Points table for the GPU to
enable Dynamic Voltage & Frequency Scaling on the H6.

The voltage range is set with minival voltage set to the target
and the maximal voltage set to 1.2V. This allow DVFS framework to
work properly on board with fixed regulator.

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi | 80 ++++++++++++++++++++
 1 file changed, 80 insertions(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
index b26f735201c7..85f43a4b651f 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi
@@ -173,6 +173,7 @@
 			clocks = <&ccu CLK_GPU>, <&ccu CLK_BUS_GPU>;
 			clock-names = "core", "bus";
 			resets = <&ccu RST_BUS_GPU>;
+			operating-points-v2 = <&gpu_opp_table>;
 			#cooling-cells = <2>;
 			status = "disabled";
 		};
@@ -1026,4 +1027,83 @@
 			};
 		};
 	};
+
+	gpu_opp_table: gpu-opp-table {
+		compatible = "operating-points-v2";
+
+		opp@216000000 {
+			opp-hz = /bits/ 64 <216000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@264000000 {
+			opp-hz = /bits/ 64 <264000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@312000000 {
+			opp-hz = /bits/ 64 <312000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@336000000 {
+			opp-hz = /bits/ 64 <336000000>;
+			opp-microvolt = <810000 810000 1200000>;
+		};
+
+		opp@360000000 {
+			opp-hz = /bits/ 64 <360000000>;
+			opp-microvolt = <820000 820000 1200000>;
+		};
+
+		opp@384000000 {
+			opp-hz = /bits/ 64 <384000000>;
+			opp-microvolt = <830000 830000 1200000>;
+		};
+
+		opp@408000000 {
+			opp-hz = /bits/ 64 <408000000>;
+			opp-microvolt = <840000 840000 1200000>;
+		};
+
+		opp@420000000 {
+			opp-hz = /bits/ 64 <420000000>;
+			opp-microvolt = <850000 850000 1200000>;
+		};
+
+		opp@432000000 {
+			opp-hz = /bits/ 64 <432000000>;
+			opp-microvolt = <860000 860000 1200000>;
+		};
+
+		opp@456000000 {
+			opp-hz = /bits/ 64 <456000000>;
+			opp-microvolt = <870000 870000 1200000>;
+		};
+
+		opp@504000000 {
+			opp-hz = /bits/ 64 <504000000>;
+			opp-microvolt = <890000 890000 1200000>;
+		};
+
+		opp@540000000 {
+			opp-hz = /bits/ 64 <540000000>;
+			opp-microvolt = <910000 910000 1200000>;
+		};
+
+		opp@576000000 {
+			opp-hz = /bits/ 64 <576000000>;
+			opp-microvolt = <930000 930000 1200000>;
+		};
+
+		opp@624000000 {
+			opp-hz = /bits/ 64 <624000000>;
+			opp-microvolt = <950000 950000 1200000>;
+		};
+
+		opp@756000000 {
+			opp-hz = /bits/ 64 <756000000>;
+			opp-microvolt = <1040000 1040000 1200000>;
+		};
+	};
 };
-- 
2.20.1

[PATCH 15/15] [DO NOT MERGE] arm64: dts: allwinner: force GPU regulator to be always

[Clément Péron]

Signed-off-by: Clément Péron <peron.clem@gmail.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
index 3f7ceeb1a767..14257f7476b8 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
+++ b/arch/arm64/boot/dts/allwinner/sun50i-h6-beelink-gs1.dts
@@ -245,6 +245,7 @@
 			};
 
 			reg_dcdcc: dcdcc {
+				regulator-always-on;
 				regulator-enable-ramp-delay = <32000>;
 				regulator-min-microvolt = <810000>;
 				regulator-max-microvolt = <1080000>;
-- 
2.20.1

Re: [PATCH 00/15][RFC] Add regulator devfreq support to Panfrost

[Tomeu Vizoso]

On 5/10/20 6:55 PM, Clément Péron wrote:
> Hi,
> 
> This serie cleans and adds regulator support to Panfrost devfreq.
> This is mostly based on comment for the freshly introduced lima
> devfreq.
> 
> We need to add regulator support because on Allwinner the GPU OPP
> table defines both frequencies and voltages.
> 
> First patches [01-08] should not change the actual behavior
> and introduce a proper panfrost_devfreq struct.
> 
> Fatches after are WIP and add regulator support.
> 
> However I got several issues first we need to avoid getting regulator
> if devfreq get by itself the regulator, but as of today the OPP
> framework only get and don't enable the regulator...
> An HACK for now is to add regulator-always-on in the device-tree.
> 
> Then when I enable devfreq I got several faults like.
> I'm totally noob on GPU sched/fault and couldn't be helpfull with this.

Do you know at which frequencies do the faults happen? From what I can 
see, it's just the GPU behaving erratically, and the CPU reading random 
values from the GPU registers. Given the subject of this series, I guess 
the GPU isn't getting enough power.

There could be a problem with the OPP table, might be a good idea to see 
what levels are problematic and try with a more conservative table.

Besides that, there could be a problem with clock frequency changes, or 
voltage changes. It may take some time for the final state to be stable, 
depending how the regulation happens.

Thanks,

Tomeu




> I got this running glmark2 on T720 (Allwinner H6) with Mesa 20.0.5.
> # glmark2-es2-drm
> =======================================================
>      glmark2 2017.07
> =======================================================
>      OpenGL Information
>      GL_VENDOR:     Panfrost
>      GL_RENDERER:   Mali T720 (Panfrost)
>      GL_VERSION:    OpenGL ES 2.0 Mesa 20.0.5
> =======================================================
> 
> [   93.550063] panfrost 1800000.gpu: GPU Fault 0x00000088 (UNKNOWN) at 0x0000000080117100
> [   94.045401] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x8, head=0x21d6c00, tail=0x21d6c00, sched_job=00000000e3c2132f
> 
> [  328.871070] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
> [  328.871070] Reason: TODO
> [  328.871070] raw fault status: 0xAA0003C2
> [  328.871070] decoded fault status: SLAVE FAULT
> [  328.871070] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
> [  328.871070] access type 0x3: WRITE
> [  328.871070] source id 0xAA00
> [  329.373327] panfrost 1800000.gpu: gpu sched timeout, js=1, config=0x3700, status=0x8, head=0xa1a4900, tail=0xa1a4900, sched_job=000000007ac31097
> [  329.386527] panfrost 1800000.gpu: js fault, js=0, status=DATA_INVALID_FAULT, head=0xa1a4c00, tail=0xa1a4c00
> [  329.396293] panfrost 1800000.gpu: gpu sched timeout, js=0, config=0x3700, status=0x58, head=0xa1a4c00, tail=0xa1a4c00, sched_job=0000000004c90381
> [  329.411521] panfrost 1800000.gpu: Unhandled Page fault in AS0 at VA 0x0000000000000000
> [  329.411521] Reason: TODO
> [  329.411521] raw fault status: 0xAA0003C2
> [  329.411521] decoded fault status: SLAVE FAULT
> [  329.411521] exception type 0xC2: TRANSLATION_FAULT_LEVEL2
> [  329.411521] access type 0x3: WRITE
> [  329.411521] source id 0xAA00
> 
> Thanks for your reviews, help on this serie,
> Clement
> 
> Clément Péron (15):
>    drm/panfrost: avoid static declaration
>    drm/panfrost: clean headers in devfreq
>    drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle
>    drm/panfrost: introduce panfrost_devfreq struct
>    drm/panfrost: use spinlock instead of atomic
>    drm/panfrost: properly handle error in probe
>    drm/panfrost: use device_property_present to check for OPP
>    drm/panfrost: move devfreq_init()/fini() in device
>    drm/panfrost: dynamically alloc regulators
>    drm/panfrost: add regulators to devfreq
>    drm/panfrost: set devfreq clock name
>    arm64: defconfig: Enable devfreq cooling device
>    arm64: dts: allwinner: h6: Add cooling map for GPU
>    [DO NOT MERGE] arm64: dts: allwinner: h6: Add GPU OPP table
>    [DO NOT MERGE] arm64: dts: allwinner: force GPU regulator to be always
> 
>   .../dts/allwinner/sun50i-h6-beelink-gs1.dts   |   1 +
>   arch/arm64/boot/dts/allwinner/sun50i-h6.dtsi  | 102 ++++++++++
>   arch/arm64/configs/defconfig                  |   1 +
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c   | 190 ++++++++++++------
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h   |  32 ++-
>   drivers/gpu/drm/panfrost/panfrost_device.c    |  56 ++++--
>   drivers/gpu/drm/panfrost/panfrost_device.h    |  14 +-
>   drivers/gpu/drm/panfrost/panfrost_drv.c       |  15 +-
>   drivers/gpu/drm/panfrost/panfrost_job.c       |  10 +-
>   9 files changed, 310 insertions(+), 111 deletions(-)
> 

Re: [PATCH 01/15] drm/panfrost: avoid static declaration

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> This declaration can be avoided so change it.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 38 ++++++++++-----------
>   1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 413987038fbf..1b560b903ea6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -14,7 +14,24 @@
>   #include "panfrost_gpu.h"
>   #include "panfrost_regs.h"
>   
> -static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev);
> +static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
> +{
> +	ktime_t now;
> +	ktime_t last;
> +
> +	if (!pfdev->devfreq.devfreq)
> +		return;
> +
> +	now = ktime_get();
> +	last = pfdev->devfreq.time_last_update;
> +
> +	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
> +		pfdev->devfreq.busy_time += ktime_sub(now, last);
> +	else
> +		pfdev->devfreq.idle_time += ktime_sub(now, last);
> +
> +	pfdev->devfreq.time_last_update = now;
> +}
>   
>   static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>   				   u32 flags)
> @@ -139,25 +156,6 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>   	devfreq_suspend_device(pfdev->devfreq.devfreq);
>   }
>   
> -static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
> -{
> -	ktime_t now;
> -	ktime_t last;
> -
> -	if (!pfdev->devfreq.devfreq)
> -		return;
> -
> -	now = ktime_get();
> -	last = pfdev->devfreq.time_last_update;
> -
> -	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
> -		pfdev->devfreq.busy_time += ktime_sub(now, last);
> -	else
> -		pfdev->devfreq.idle_time += ktime_sub(now, last);
> -
> -	pfdev->devfreq.time_last_update = now;
> -}
> -
>   void panfrost_devfreq_record_busy(struct panfrost_device *pfdev)
>   {
>   	panfrost_devfreq_update_utilization(pfdev);
> 

Re: [PATCH 02/15] drm/panfrost: clean headers in devfreq

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Don't include not required headers and sort them.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 8 ++------
>   1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 1b560b903ea6..df7b71da9a84 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -1,18 +1,14 @@
>   // SPDX-License-Identifier: GPL-2.0
>   /* Copyright 2019 Collabora ltd. */
> +
> +#include <linux/clk.h>
>   #include <linux/devfreq.h>
>   #include <linux/devfreq_cooling.h>
>   #include <linux/platform_device.h>
>   #include <linux/pm_opp.h>
> -#include <linux/clk.h>
> -#include <linux/regulator/consumer.h>
>   
>   #include "panfrost_device.h"
>   #include "panfrost_devfreq.h"
> -#include "panfrost_features.h"
> -#include "panfrost_issues.h"
> -#include "panfrost_gpu.h"
> -#include "panfrost_regs.h"
>   
>   static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
>   {
> 

Re: [PATCH 03/15] drm/panfrost: don't use pfdevfreq.busy_count to know if hw is idle

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> This use devfreq variable that will be lock with spinlock in future
> patches. We should either introduce a function to access this one
> but as devfreq is optional let's just remove it.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

As far as I can tell this should be safe. As you note this wouldn't work 
without devfreq anyway.

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_job.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 7914b1570841..63e32a9f2749 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -581,10 +581,6 @@ int panfrost_job_is_idle(struct panfrost_device *pfdev)
>   	struct panfrost_job_slot *js = pfdev->js;
>   	int i;
>   
> -	/* Check whether the hardware is idle */
> -	if (atomic_read(&pfdev->devfreq.busy_count))
> -		return false;
> -
>   	for (i = 0; i < NUM_JOB_SLOTS; i++) {
>   		/* If there are any jobs in the HW queue, we're not idle */
>   		if (atomic_read(&js->queue[i].sched.hw_rq_count))
> 

Re: [PATCH 04/15] drm/panfrost: introduce panfrost_devfreq struct

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Introduce a proper panfrost_devfreq to deal with devfreq variables.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 76 ++++++++++++---------
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h | 20 +++++-
>   drivers/gpu/drm/panfrost/panfrost_device.h  | 11 +--
>   drivers/gpu/drm/panfrost/panfrost_job.c     |  6 +-
>   4 files changed, 66 insertions(+), 47 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index df7b71da9a84..962550363391 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -10,23 +10,23 @@
>   #include "panfrost_device.h"
>   #include "panfrost_devfreq.h"
>   
> -static void panfrost_devfreq_update_utilization(struct panfrost_device *pfdev)
> +static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfreq)
>   {
>   	ktime_t now;
>   	ktime_t last;
>   
> -	if (!pfdev->devfreq.devfreq)
> +	if (!pfdevfreq->devfreq)
>   		return;
>   
>   	now = ktime_get();
> -	last = pfdev->devfreq.time_last_update;
> +	last = pfdevfreq->time_last_update;
>   
> -	if (atomic_read(&pfdev->devfreq.busy_count) > 0)
> -		pfdev->devfreq.busy_time += ktime_sub(now, last);
> +	if (atomic_read(&pfdevfreq->busy_count) > 0)
> +		pfdevfreq->busy_time += ktime_sub(now, last);
>   	else
> -		pfdev->devfreq.idle_time += ktime_sub(now, last);
> +		pfdevfreq->idle_time += ktime_sub(now, last);
>   
> -	pfdev->devfreq.time_last_update = now;
> +	pfdevfreq->time_last_update = now;
>   }
>   
>   static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
> @@ -47,30 +47,31 @@ static int panfrost_devfreq_target(struct device *dev, unsigned long *freq,
>   	return 0;
>   }
>   
> -static void panfrost_devfreq_reset(struct panfrost_device *pfdev)
> +static void panfrost_devfreq_reset(struct panfrost_devfreq *pfdevfreq)
>   {
> -	pfdev->devfreq.busy_time = 0;
> -	pfdev->devfreq.idle_time = 0;
> -	pfdev->devfreq.time_last_update = ktime_get();
> +	pfdevfreq->busy_time = 0;
> +	pfdevfreq->idle_time = 0;
> +	pfdevfreq->time_last_update = ktime_get();
>   }
>   
>   static int panfrost_devfreq_get_dev_status(struct device *dev,
>   					   struct devfreq_dev_status *status)
>   {
>   	struct panfrost_device *pfdev = dev_get_drvdata(dev);
> +	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>   
> -	panfrost_devfreq_update_utilization(pfdev);
> +	panfrost_devfreq_update_utilization(pfdevfreq);
>   
>   	status->current_frequency = clk_get_rate(pfdev->clock);
> -	status->total_time = ktime_to_ns(ktime_add(pfdev->devfreq.busy_time,
> -						   pfdev->devfreq.idle_time));
> +	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
> +						   pfdevfreq->idle_time));
>   
> -	status->busy_time = ktime_to_ns(pfdev->devfreq.busy_time);
> +	status->busy_time = ktime_to_ns(pfdevfreq->busy_time);
>   
> -	panfrost_devfreq_reset(pfdev);
> +	panfrost_devfreq_reset(pfdevfreq);
>   
> -	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n", status->busy_time,
> -		status->total_time,
> +	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
> +		status->busy_time, status->total_time,
>   		status->busy_time / (status->total_time / 100),
>   		status->current_frequency / 1000 / 1000);
>   
> @@ -91,6 +92,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   	struct device *dev = &pfdev->pdev->dev;
>   	struct devfreq *devfreq;
>   	struct thermal_cooling_device *cooling;
> +	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>   
>   	ret = dev_pm_opp_of_add_table(dev);
>   	if (ret == -ENODEV) /* Optional, continue without devfreq */
> @@ -98,7 +100,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   	else if (ret)
>   		return ret;
>   
> -	panfrost_devfreq_reset(pfdev);
> +	panfrost_devfreq_reset(pfdevfreq);
>   
>   	cur_freq = clk_get_rate(pfdev->clock);
>   
> @@ -116,53 +118,59 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   		dev_pm_opp_of_remove_table(dev);
>   		return PTR_ERR(devfreq);
>   	}
> -	pfdev->devfreq.devfreq = devfreq;
> +	pfdevfreq->devfreq = devfreq;
>   
>   	cooling = of_devfreq_cooling_register(dev->of_node, devfreq);
>   	if (IS_ERR(cooling))
>   		DRM_DEV_INFO(dev, "Failed to register cooling device\n");
>   	else
> -		pfdev->devfreq.cooling = cooling;
> +		pfdevfreq->cooling = cooling;
>   
>   	return 0;
>   }
>   
>   void panfrost_devfreq_fini(struct panfrost_device *pfdev)
>   {
> -	if (pfdev->devfreq.cooling)
> -		devfreq_cooling_unregister(pfdev->devfreq.cooling);
> +	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> +
> +	if (pfdevfreq->cooling)
> +		devfreq_cooling_unregister(pfdevfreq->cooling);
>   	dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
>   }
>   
>   void panfrost_devfreq_resume(struct panfrost_device *pfdev)
>   {
> -	if (!pfdev->devfreq.devfreq)
> +	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> +
> +	if (!pfdevfreq->devfreq)
>   		return;
>   
> -	panfrost_devfreq_reset(pfdev);
> +	panfrost_devfreq_reset(pfdevfreq);
>   
> -	devfreq_resume_device(pfdev->devfreq.devfreq);
> +	devfreq_resume_device(pfdevfreq->devfreq);
>   }
>   
>   void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>   {
> -	if (!pfdev->devfreq.devfreq)
> +	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> +
> +	if (!pfdevfreq->devfreq)
>   		return;
>   
> -	devfreq_suspend_device(pfdev->devfreq.devfreq);
> +	devfreq_suspend_device(pfdevfreq->devfreq);
>   }
>   
> -void panfrost_devfreq_record_busy(struct panfrost_device *pfdev)
> +void panfrost_devfreq_record_busy(struct panfrost_devfreq *pfdevfreq)
>   {
> -	panfrost_devfreq_update_utilization(pfdev);
> -	atomic_inc(&pfdev->devfreq.busy_count);
> +	panfrost_devfreq_update_utilization(pfdevfreq);
> +	atomic_inc(&pfdevfreq->busy_count);
>   }
>   
> -void panfrost_devfreq_record_idle(struct panfrost_device *pfdev)
> +void panfrost_devfreq_record_idle(struct panfrost_devfreq *pfdevfreq)
>   {
>   	int count;
>   
> -	panfrost_devfreq_update_utilization(pfdev);
> -	count = atomic_dec_if_positive(&pfdev->devfreq.busy_count);
> +	panfrost_devfreq_update_utilization(pfdevfreq);
> +	count = atomic_dec_if_positive(&pfdevfreq->busy_count);
>   	WARN_ON(count < 0);
>   }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index 0611beffc8d0..0697f8d5aa34 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -4,13 +4,29 @@
>   #ifndef __PANFROST_DEVFREQ_H__
>   #define __PANFROST_DEVFREQ_H__
>   
> +#include <linux/ktime.h>
> +
> +struct devfreq;
> +struct thermal_cooling_device;
> +
> +struct panfrost_device;
> +
> +struct panfrost_devfreq {
> +	struct devfreq *devfreq;
> +	struct thermal_cooling_device *cooling;
> +	ktime_t busy_time;
> +	ktime_t idle_time;
> +	ktime_t time_last_update;
> +	atomic_t busy_count;
> +};
> +
>   int panfrost_devfreq_init(struct panfrost_device *pfdev);
>   void panfrost_devfreq_fini(struct panfrost_device *pfdev);
>   
>   void panfrost_devfreq_resume(struct panfrost_device *pfdev);
>   void panfrost_devfreq_suspend(struct panfrost_device *pfdev);
>   
> -void panfrost_devfreq_record_busy(struct panfrost_device *pfdev);
> -void panfrost_devfreq_record_idle(struct panfrost_device *pfdev);
> +void panfrost_devfreq_record_busy(struct panfrost_devfreq *devfreq);
> +void panfrost_devfreq_record_idle(struct panfrost_devfreq *devfreq);
>   
>   #endif /* __PANFROST_DEVFREQ_H__ */
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index c30c719a8059..2efa59c9d1c5 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -13,6 +13,8 @@
>   #include <drm/drm_mm.h>
>   #include <drm/gpu_scheduler.h>
>   
> +#include "panfrost_devfreq.h"
> +
>   struct panfrost_device;
>   struct panfrost_mmu;
>   struct panfrost_job_slot;
> @@ -107,14 +109,7 @@ struct panfrost_device {
>   	struct list_head shrinker_list;
>   	struct shrinker shrinker;
>   
> -	struct {
> -		struct devfreq *devfreq;
> -		struct thermal_cooling_device *cooling;
> -		ktime_t busy_time;
> -		ktime_t idle_time;
> -		ktime_t time_last_update;
> -		atomic_t busy_count;
> -	} devfreq;
> +	struct panfrost_devfreq pfdevfreq;
>   };
>   
>   struct panfrost_mmu {
> diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c
> index 63e32a9f2749..a67f3eac6a58 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_job.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_job.c
> @@ -155,7 +155,7 @@ static void panfrost_job_hw_submit(struct panfrost_job *job, int js)
>   	}
>   
>   	cfg = panfrost_mmu_as_get(pfdev, &job->file_priv->mmu);
> -	panfrost_devfreq_record_busy(pfdev);
> +	panfrost_devfreq_record_busy(&pfdev->pfdevfreq);
>   
>   	job_write(pfdev, JS_HEAD_NEXT_LO(js), jc_head & 0xFFFFFFFF);
>   	job_write(pfdev, JS_HEAD_NEXT_HI(js), jc_head >> 32);
> @@ -415,7 +415,7 @@ static void panfrost_job_timedout(struct drm_sched_job *sched_job)
>   	}
>   	spin_unlock_irqrestore(&pfdev->js->job_lock, flags);
>   
> -	panfrost_devfreq_record_idle(pfdev);
> +	panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
>   	panfrost_device_reset(pfdev);
>   
>   	for (i = 0; i < NUM_JOB_SLOTS; i++)
> @@ -478,7 +478,7 @@ static irqreturn_t panfrost_job_irq_handler(int irq, void *data)
>   				pfdev->jobs[j] = NULL;
>   
>   				panfrost_mmu_as_put(pfdev, &job->file_priv->mmu);
> -				panfrost_devfreq_record_idle(pfdev);
> +				panfrost_devfreq_record_idle(&pfdev->pfdevfreq);
>   
>   				dma_fence_signal_locked(job->done_fence);
>   				pm_runtime_put_autosuspend(pfdev->dev);
> 

Re: [PATCH 05/15] drm/panfrost: use spinlock instead of atomic

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Convert busy_count to a simple int protected by spinlock.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

Looks like a fairly mechanical cleanup.

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 43 +++++++++++++++------
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h | 10 ++++-
>   2 files changed, 41 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 962550363391..78753cfb59fb 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -12,16 +12,12 @@
>   
>   static void panfrost_devfreq_update_utilization(struct panfrost_devfreq *pfdevfreq)
>   {
> -	ktime_t now;
> -	ktime_t last;
> -
> -	if (!pfdevfreq->devfreq)
> -		return;
> +	ktime_t now, last;
>   
>   	now = ktime_get();
>   	last = pfdevfreq->time_last_update;
>   
> -	if (atomic_read(&pfdevfreq->busy_count) > 0)
> +	if (pfdevfreq->busy_count > 0)
>   		pfdevfreq->busy_time += ktime_sub(now, last);
>   	else
>   		pfdevfreq->idle_time += ktime_sub(now, last);
> @@ -59,10 +55,14 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
>   {
>   	struct panfrost_device *pfdev = dev_get_drvdata(dev);
>   	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> +	unsigned long irqflags;
> +
> +	status->current_frequency = clk_get_rate(pfdev->clock);
> +
> +	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
>   
>   	panfrost_devfreq_update_utilization(pfdevfreq);
>   
> -	status->current_frequency = clk_get_rate(pfdev->clock);
>   	status->total_time = ktime_to_ns(ktime_add(pfdevfreq->busy_time,
>   						   pfdevfreq->idle_time));
>   
> @@ -70,6 +70,8 @@ static int panfrost_devfreq_get_dev_status(struct device *dev,
>   
>   	panfrost_devfreq_reset(pfdevfreq);
>   
> +	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
> +
>   	dev_dbg(pfdev->dev, "busy %lu total %lu %lu %% freq %lu MHz\n",
>   		status->busy_time, status->total_time,
>   		status->busy_time / (status->total_time / 100),
> @@ -100,6 +102,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   	else if (ret)
>   		return ret;
>   
> +	spin_lock_init(&pfdevfreq->lock);
> +
>   	panfrost_devfreq_reset(pfdevfreq);
>   
>   	cur_freq = clk_get_rate(pfdev->clock);
> @@ -162,15 +166,32 @@ void panfrost_devfreq_suspend(struct panfrost_device *pfdev)
>   
>   void panfrost_devfreq_record_busy(struct panfrost_devfreq *pfdevfreq)
>   {
> +	unsigned long irqflags;
> +
> +	if (!pfdevfreq->devfreq)
> +		return;
> +
> +	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
> +
>   	panfrost_devfreq_update_utilization(pfdevfreq);
> -	atomic_inc(&pfdevfreq->busy_count);
> +
> +	pfdevfreq->busy_count++;
> +
> +	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
>   }
>   
>   void panfrost_devfreq_record_idle(struct panfrost_devfreq *pfdevfreq)
>   {
> -	int count;
> +	unsigned long irqflags;
> +
> +	if (!pfdevfreq->devfreq)
> +		return;
> +
> +	spin_lock_irqsave(&pfdevfreq->lock, irqflags);
>   
>   	panfrost_devfreq_update_utilization(pfdevfreq);
> -	count = atomic_dec_if_positive(&pfdevfreq->busy_count);
> -	WARN_ON(count < 0);
> +
> +	WARN_ON(--pfdevfreq->busy_count < 0);
> +
> +	spin_unlock_irqrestore(&pfdevfreq->lock, irqflags);
>   }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index 0697f8d5aa34..e6629900a618 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -4,6 +4,7 @@
>   #ifndef __PANFROST_DEVFREQ_H__
>   #define __PANFROST_DEVFREQ_H__
>   
> +#include <linux/spinlock.h>
>   #include <linux/ktime.h>
>   
>   struct devfreq;
> @@ -14,10 +15,17 @@ struct panfrost_device;
>   struct panfrost_devfreq {
>   	struct devfreq *devfreq;
>   	struct thermal_cooling_device *cooling;
> +
>   	ktime_t busy_time;
>   	ktime_t idle_time;
>   	ktime_t time_last_update;
> -	atomic_t busy_count;
> +	int busy_count;
> +	/*
> +	 * Protect busy_time, idle_time, time_last_update and busy_count
> +	 * because these can be updated concurrently, for example by the GP
> +	 * and PP interrupts.
> +	 */
> +	spinlock_t lock;
>   };
>   
>   int panfrost_devfreq_init(struct panfrost_device *pfdev);
> 

Re: [PATCH 06/15] drm/panfrost: properly handle error in probe

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Introduce a boolean to know if opp table has been added.
> 
> With this, we can call panfrost_devfreq_fini() in case of error
> and release what has been initialised.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

LGTM:

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 25 ++++++++++++++++-----
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h |  1 +
>   2 files changed, 20 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 78753cfb59fb..d9007f44b772 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -101,6 +101,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   		return 0;
>   	else if (ret)
>   		return ret;
> +	pfdevfreq->opp_of_table_added = true;
>   
>   	spin_lock_init(&pfdevfreq->lock);
>   
> @@ -109,8 +110,10 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   	cur_freq = clk_get_rate(pfdev->clock);
>   
>   	opp = devfreq_recommended_opp(dev, &cur_freq, 0);
> -	if (IS_ERR(opp))
> -		return PTR_ERR(opp);
> +	if (IS_ERR(opp)) {
> +		ret = PTR_ERR(opp);
> +		goto err_fini;
> +	}
>   
>   	panfrost_devfreq_profile.initial_freq = cur_freq;
>   	dev_pm_opp_put(opp);
> @@ -119,8 +122,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   					  DEVFREQ_GOV_SIMPLE_ONDEMAND, NULL);
>   	if (IS_ERR(devfreq)) {
>   		DRM_DEV_ERROR(dev, "Couldn't initialize GPU devfreq\n");
> -		dev_pm_opp_of_remove_table(dev);
> -		return PTR_ERR(devfreq);
> +		ret = PTR_ERR(devfreq);
> +		goto err_fini;
>   	}
>   	pfdevfreq->devfreq = devfreq;
>   
> @@ -131,15 +134,25 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   		pfdevfreq->cooling = cooling;
>   
>   	return 0;
> +
> +err_fini:
> +	panfrost_devfreq_fini(pfdev);
> +	return ret;
>   }
>   
>   void panfrost_devfreq_fini(struct panfrost_device *pfdev)
>   {
>   	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>   
> -	if (pfdevfreq->cooling)
> +	if (pfdevfreq->cooling) {
>   		devfreq_cooling_unregister(pfdevfreq->cooling);
> -	dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
> +		pfdevfreq->cooling = NULL;
> +	}
> +
> +	if (pfdevfreq->opp_of_table_added) {
> +		dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
> +		pfdevfreq->opp_of_table_added = false;
> +	}
>   }
>   
>   void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index e6629900a618..add203cb00c2 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -15,6 +15,7 @@ struct panfrost_device;
>   struct panfrost_devfreq {
>   	struct devfreq *devfreq;
>   	struct thermal_cooling_device *cooling;
> +	bool opp_of_table_added;
>   
>   	ktime_t busy_time;
>   	ktime_t idle_time;
> 

Re: [PATCH 07/15] drm/panfrost: use device_property_present to check for OPP

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Instead of expecting an error from dev_pm_opp_of_add_table()
> do a simple device_property_present() check.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

I'm not sure I understand why this is better. We seem to have more code 
to do roughly the same thing just with the hard-coded 
"operating-points-v2" name (if there's ever a 'v3' we'll then have to 
update this).

Is the desire just to get an error on probe if the table is malformed? 
Have you hit this situation? If so this sounds like something which 
would be better fixed in the generic OPP code rather than Panfrost itself.

Steve

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++-----
>   1 file changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index d9007f44b772..fce21c682414 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   	struct thermal_cooling_device *cooling;
>   	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>   
> -	ret = dev_pm_opp_of_add_table(dev);
> -	if (ret == -ENODEV) /* Optional, continue without devfreq */
> +	if (!device_property_present(dev, "operating-points-v2"))
> +		/* Optional, continue without devfreq */
>   		return 0;
> -	else if (ret)
> -		return ret;
> -	pfdevfreq->opp_of_table_added = true;
>   
>   	spin_lock_init(&pfdevfreq->lock);
>   
> +	ret = dev_pm_opp_of_add_table(dev);
> +	if (ret) {
> +		DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
> +		goto err_fini;
> +	}
> +	pfdevfreq->opp_of_table_added = true;
> +
>   	panfrost_devfreq_reset(pfdevfreq);
>   
>   	cur_freq = clk_get_rate(pfdev->clock);
> 

Re: [PATCH 08/15] drm/panfrost: move devfreq_init()/fini() in device

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Later we will introduce devfreq probing regulator if they
> are present. As regulator should be probe only one time we
> need to get this logic in the device_init().
> 
> panfrost_device is already taking care of devfreq_resume()
> and devfreq_suspend(), so it's not totally illogic to move
> the devfreq_init() and devfreq_fini() here.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>   drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++--------
>   drivers/gpu/drm/panfrost/panfrost_drv.c    | 15 ++-------
>   2 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 8136babd3ba9..f480127205d6 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>   		return err;
>   	}
>   
> +	err = panfrost_devfreq_init(pfdev);
> +	if (err) {
> +		dev_err(pfdev->dev, "devfreq init failed %d\n", err);
> +		goto err_out0;
> +	}
> +
>   	err = panfrost_regulator_init(pfdev);
>   	if (err) {
>   		dev_err(pfdev->dev, "regulator init failed %d\n", err);
> -		goto err_out0;
> +		goto err_out1;

NIT: Rather than just renumbering these can we give them sensible names 
so we don't have this sort of refactoring in future?

>   	}
>   
>   	err = panfrost_reset_init(pfdev);
>   	if (err) {
>   		dev_err(pfdev->dev, "reset init failed %d\n", err);
> -		goto err_out1;
> +		goto err_out2;
>   	}
>   
>   	err = panfrost_pm_domain_init(pfdev);
>   	if (err)
> -		goto err_out2;
> +		goto err_out3;
>   
>   	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_out3;
> +		goto err_out4;
>   	}
>   
>   	err = panfrost_gpu_init(pfdev);
>   	if (err)
> -		goto err_out3;
> +		goto err_out4;
>   
>   	err = panfrost_mmu_init(pfdev);
>   	if (err)
> -		goto err_out4;
> +		goto err_out5;
>   
>   	err = panfrost_job_init(pfdev);
>   	if (err)
> -		goto err_out5;
> +		goto err_out6;
>   
>   	err = panfrost_perfcnt_init(pfdev);
>   	if (err)
> -		goto err_out6;
> +		goto err_out7;
>   
>   	return 0;
> -err_out6:
> +err_out7:
>   	panfrost_job_fini(pfdev);
> -err_out5:
> +err_out6:
>   	panfrost_mmu_fini(pfdev);
> -err_out4:
> +err_out5:
>   	panfrost_gpu_fini(pfdev);
> -err_out3:
> +err_out4:
>   	panfrost_pm_domain_fini(pfdev);
> -err_out2:
> +err_out3:
>   	panfrost_reset_fini(pfdev);
> -err_out1:
> +err_out2:
>   	panfrost_regulator_fini(pfdev);
> +err_out1:
> +	panfrost_devfreq_fini(pfdev);
>   err_out0:
>   	panfrost_clk_fini(pfdev);
>   	return err;
> @@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
>   	panfrost_gpu_fini(pfdev);
>   	panfrost_pm_domain_fini(pfdev);
>   	panfrost_reset_fini(pfdev);
> +	panfrost_devfreq_fini(pfdev);
>   	panfrost_regulator_fini(pfdev);
>   	panfrost_clk_fini(pfdev);
>   }
> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> index 882fecc33fdb..4dda68689015 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> @@ -14,7 +14,6 @@
>   #include <drm/drm_utils.h>
>   
>   #include "panfrost_device.h"
> -#include "panfrost_devfreq.h"
>   #include "panfrost_gem.h"
>   #include "panfrost_mmu.h"
>   #include "panfrost_job.h"
> @@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev)
>   		goto err_out0;
>   	}
>   
> -	err = panfrost_devfreq_init(pfdev);
> -	if (err) {
> -		if (err != -EPROBE_DEFER)
> -			dev_err(&pdev->dev, "Fatal error during devfreq init\n");

You seem to have lost the check for EPROBE_DEFER during the move.

> -		goto err_out1;
> -	}
> -
>   	pm_runtime_set_active(pfdev->dev);
>   	pm_runtime_mark_last_busy(pfdev->dev);
>   	pm_runtime_enable(pfdev->dev);
> @@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev)
>   	 */
>   	err = drm_dev_register(ddev, 0);
>   	if (err < 0)
> -		goto err_out2;
> +		goto err_out1;
>   
>   	panfrost_gem_shrinker_init(ddev);
>   
>   	return 0;
>   
> -err_out2:
> -	pm_runtime_disable(pfdev->dev);
> -	panfrost_devfreq_fini(pfdev);
>   err_out1:
> +	pm_runtime_disable(pfdev->dev);
>   	panfrost_device_fini(pfdev);
>   err_out0:
>   	drm_dev_put(ddev);
> @@ -650,7 +640,6 @@ static int panfrost_remove(struct platform_device *pdev)
>   	panfrost_gem_shrinker_cleanup(ddev);
>   
>   	pm_runtime_get_sync(pfdev->dev);
> -	panfrost_devfreq_fini(pfdev);
>   	panfrost_device_fini(pfdev);
>   	pm_runtime_put_sync_suspend(pfdev->dev);
>   	pm_runtime_disable(pfdev->dev);
> 

Re: [PATCH 09/15] drm/panfrost: dynamically alloc regulators

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> We will later introduce regulators managed by OPP.
> 
> Only alloc regulators when it's needed. This also help use
> to release the regulators only when they are allocated.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

LGTM:

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_device.c | 14 +++++++++-----
>   drivers/gpu/drm/panfrost/panfrost_device.h |  3 +--
>   2 files changed, 10 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index f480127205d6..67eedf64e82d 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -90,9 +90,11 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
>   {
>   	int ret, i;
>   
> -	if (WARN(pfdev->comp->num_supplies > ARRAY_SIZE(pfdev->regulators),
> -			"Too many supplies in compatible structure.\n"))
> -		return -EINVAL;
> +	pfdev->regulators = devm_kcalloc(pfdev->dev, pfdev->comp->num_supplies,
> +					 sizeof(*pfdev->regulators),
> +					 GFP_KERNEL);
> +	if (!pfdev->regulators)
> +		return -ENOMEM;
>   
>   	for (i = 0; i < pfdev->comp->num_supplies; i++)
>   		pfdev->regulators[i].supply = pfdev->comp->supply_names[i];
> @@ -117,8 +119,10 @@ static int panfrost_regulator_init(struct panfrost_device *pfdev)
>   
>   static void panfrost_regulator_fini(struct panfrost_device *pfdev)
>   {
> -	regulator_bulk_disable(pfdev->comp->num_supplies,
> -			pfdev->regulators);
> +	if (!pfdev->regulators)
> +		return;
> +
> +	regulator_bulk_disable(pfdev->comp->num_supplies, pfdev->regulators);
>   }
>   
>   static void panfrost_pm_domain_fini(struct panfrost_device *pfdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.h b/drivers/gpu/drm/panfrost/panfrost_device.h
> index 2efa59c9d1c5..953f7536a773 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.h
> @@ -22,7 +22,6 @@ struct panfrost_job;
>   struct panfrost_perfcnt;
>   
>   #define NUM_JOB_SLOTS 3
> -#define MAX_REGULATORS 2
>   #define MAX_PM_DOMAINS 3
>   
>   struct panfrost_features {
> @@ -81,7 +80,7 @@ struct panfrost_device {
>   	void __iomem *iomem;
>   	struct clk *clock;
>   	struct clk *bus_clock;
> -	struct regulator_bulk_data regulators[MAX_REGULATORS];
> +	struct regulator_bulk_data *regulators;
>   	struct reset_control *rstc;
>   	/* pm_domains for devices with more than one. */
>   	struct device *pm_domain_devs[MAX_PM_DOMAINS];
> 

Re: [PATCH 10/15] drm/panfrost: add regulators to devfreq

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Some OPP tables specify voltage for each frequency. Devfreq can
> handle these regulators but they should be get only 1 time to avoid
> issue and know who is in charge.
> 
> If OPP table is probe don't init regulator.
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

This looks like it should work - thanks for doing this!

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

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 19 +++++++++++++++++++
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h |  2 ++
>   drivers/gpu/drm/panfrost/panfrost_device.c  | 11 +++++++----
>   3 files changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index fce21c682414..9ffea0d4a087 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -93,6 +93,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   	unsigned long cur_freq;
>   	struct device *dev = &pfdev->pdev->dev;
>   	struct devfreq *devfreq;
> +	struct opp_table *opp_table;
>   	struct thermal_cooling_device *cooling;
>   	struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
>   
> @@ -102,6 +103,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   
>   	spin_lock_init(&pfdevfreq->lock);
>   
> +	opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
> +					      pfdev->comp->num_supplies);
> +	if (IS_ERR(opp_table)) {
> +		ret = PTR_ERR(opp_table);
> +		/* Continue if the optional regulator is missing */
> +		if (ret != -ENODEV) {
> +			DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
> +			goto err_fini;
> +		}
> +	} else {
> +		pfdevfreq->regulators_opp_table = opp_table;
> +	}
> +
>   	ret = dev_pm_opp_of_add_table(dev);
>   	if (ret) {
>   		DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
> @@ -157,6 +171,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
>   		dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
>   		pfdevfreq->opp_of_table_added = false;
>   	}
> +
> +	if (pfdevfreq->regulators_opp_table) {
> +		dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
> +		pfdevfreq->regulators_opp_table = NULL;
> +	}
>   }
>   
>   void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index add203cb00c2..347cde4786cf 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -8,12 +8,14 @@
>   #include <linux/ktime.h>
>   
>   struct devfreq;
> +struct opp_table;
>   struct thermal_cooling_device;
>   
>   struct panfrost_device;
>   
>   struct panfrost_devfreq {
>   	struct devfreq *devfreq;
> +	struct opp_table *regulators_opp_table;
>   	struct thermal_cooling_device *cooling;
>   	bool opp_of_table_added;
>   
> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> index 67eedf64e82d..8b17fb2e3369 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> @@ -222,10 +222,13 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>   		goto err_out0;
>   	}
>   
> -	err = panfrost_regulator_init(pfdev);
> -	if (err) {
> -		dev_err(pfdev->dev, "regulator init failed %d\n", err);
> -		goto err_out1;
> +	/* OPP will handle regulators */
> +	if (!pfdev->pfdevfreq.opp_of_table_added) {
> +		err = panfrost_regulator_init(pfdev);
> +		if (err) {
> +			dev_err(pfdev->dev, "regulator init failed %d\n", err);
> +			goto err_out1;
> +		}
>   	}
>   
>   	err = panfrost_reset_init(pfdev);
> 

Re: [PATCH 11/15] drm/panfrost: set devfreq clock name

[Steven Price]

On 10/05/2020 17:55, Clément Péron wrote:
> Some SoCs have  several clocks defined and the OPP core
> needs to know the exact name of the clk to use.
> 
> Set the clock name to "core".
> 
> Signed-off-by: Clément Péron <peron.clem@gmail.com>

This is unfortunately a regression for the RK3288. The device tree 
binding doesn't require "clock-names", and for the RK3288 it currently 
isn't specified. So this breaks the platform.

Adding the "clock-names" to the device tree 'fixes' it, but we really 
need to keep backwards compatibility.

Steve

> ---
>   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 +++++++++++++
>   drivers/gpu/drm/panfrost/panfrost_devfreq.h |  1 +
>   2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> index 9ffea0d4a087..6bf3541b4d53 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> @@ -103,6 +103,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
>   
>   	spin_lock_init(&pfdevfreq->lock);
>   
> +	opp_table = dev_pm_opp_set_clkname(dev, "core");
> +	if (IS_ERR(opp_table)) {
> +		ret = PTR_ERR(opp_table);
> +		goto err_fini;
> +	}
> +
> +	pfdevfreq->clkname_opp_table = opp_table;
> +
>   	opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
>   					      pfdev->comp->num_supplies);
>   	if (IS_ERR(opp_table)) {
> @@ -176,6 +184,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
>   		dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
>   		pfdevfreq->regulators_opp_table = NULL;
>   	}
> +
> +	if (pfdevfreq->clkname_opp_table) {
> +		dev_pm_opp_put_clkname(pfdevfreq->clkname_opp_table);
> +		pfdevfreq->clkname_opp_table = NULL;
> +	}
>   }
>   
>   void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index 347cde4786cf..1f2475e1d034 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -16,6 +16,7 @@ struct panfrost_device;
>   struct panfrost_devfreq {
>   	struct devfreq *devfreq;
>   	struct opp_table *regulators_opp_table;
> +	struct opp_table *clkname_opp_table;
>   	struct thermal_cooling_device *cooling;
>   	bool opp_of_table_added;
>   
> 

Re: [PATCH 05/15] drm/panfrost: use spinlock instead of atomic

[Robin Murphy]

On 2020-05-10 17:55, Clément Péron wrote:
> Convert busy_count to a simple int protected by spinlock.

A little more reasoning might be nice.

> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
[...]
> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> index 0697f8d5aa34..e6629900a618 100644
> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> @@ -4,6 +4,7 @@
>   #ifndef __PANFROST_DEVFREQ_H__
>   #define __PANFROST_DEVFREQ_H__
>   
> +#include <linux/spinlock.h>
>   #include <linux/ktime.h>
>   
>   struct devfreq;
> @@ -14,10 +15,17 @@ struct panfrost_device;
>   struct panfrost_devfreq {
>   	struct devfreq *devfreq;
>   	struct thermal_cooling_device *cooling;
> +
>   	ktime_t busy_time;
>   	ktime_t idle_time;
>   	ktime_t time_last_update;
> -	atomic_t busy_count;
> +	int busy_count;
> +	/*
> +	 * Protect busy_time, idle_time, time_last_update and busy_count
> +	 * because these can be updated concurrently, for example by the GP
> +	 * and PP interrupts.
> +	 */

Nit: this comment is clearly wrong, since we only have Job, GPU and MMU 
interrupts here. I guess if there is a race it would be between 
submission/completion/timeout on different job slots.

Given that, should this actually be considered a fix for 9e62b885f715 
("drm/panfrost: Simplify devfreq utilisation tracking")?

Robin.

Re: [PATCH 05/15] drm/panfrost: use spinlock instead of atomic

[Clément Péron]

Hi Robin,

On Fri, 29 May 2020 at 14:20, Robin Murphy <robin.murphy@arm.com> wrote:
>
> On 2020-05-10 17:55, Clément Péron wrote:
> > Convert busy_count to a simple int protected by spinlock.
>
> A little more reasoning might be nice.

I have follow the modification requested for lima devfreq and clearly
don't have any argument to switch to spinlock.

The Lima Maintainer asked to change witht the following reason :
"Better make this count a normal int which is also protected by the spinlock,
because current implementation can't protect atomic ops for state change
and busy idle check and we are using spinlock already"

>
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> [...]
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > index 0697f8d5aa34..e6629900a618 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > @@ -4,6 +4,7 @@
> >   #ifndef __PANFROST_DEVFREQ_H__
> >   #define __PANFROST_DEVFREQ_H__
> >
> > +#include <linux/spinlock.h>
> >   #include <linux/ktime.h>
> >
> >   struct devfreq;
> > @@ -14,10 +15,17 @@ struct panfrost_device;
> >   struct panfrost_devfreq {
> >       struct devfreq *devfreq;
> >       struct thermal_cooling_device *cooling;
> > +
> >       ktime_t busy_time;
> >       ktime_t idle_time;
> >       ktime_t time_last_update;
> > -     atomic_t busy_count;
> > +     int busy_count;
> > +     /*
> > +      * Protect busy_time, idle_time, time_last_update and busy_count
> > +      * because these can be updated concurrently, for example by the GP
> > +      * and PP interrupts.
> > +      */
>
> Nit: this comment is clearly wrong, since we only have Job, GPU and MMU
> interrupts here. I guess if there is a race it would be between
> submission/completion/timeout on different job slots.

It's copy/paste from lima I will update it,

>
> Given that, should this actually be considered a fix for 9e62b885f715
> ("drm/panfrost: Simplify devfreq utilisation tracking")?

I can't say if it can be considered as a fix, I didn't see any
improvement on my board before and after this patch.
I'm still facing some issue and didn't have time to fully investigate it.

Thanks for you review,


>
> Robin.

Re: [PATCH 11/15] drm/panfrost: set devfreq clock name

[Clément Péron]

Hi Steven,

On Thu, 28 May 2020 at 15:23, Steven Price <steven.price@arm.com> wrote:
>
> On 10/05/2020 17:55, Clément Péron wrote:
> > Some SoCs have  several clocks defined and the OPP core
> > needs to know the exact name of the clk to use.
> >
> > Set the clock name to "core".
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
>
> This is unfortunately a regression for the RK3288. The device tree
> binding doesn't require "clock-names", and for the RK3288 it currently
> isn't specified. So this breaks the platform.
>
> Adding the "clock-names" to the device tree 'fixes' it, but we really
> need to keep backwards compatibility.

Yes you're right, thanks for cathing this.

Regards,
Clement

>
> Steve
>
> > ---
> >   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 13 +++++++++++++
> >   drivers/gpu/drm/panfrost/panfrost_devfreq.h |  1 +
> >   2 files changed, 14 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index 9ffea0d4a087..6bf3541b4d53 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -103,6 +103,14 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >
> >       spin_lock_init(&pfdevfreq->lock);
> >
> > +     opp_table = dev_pm_opp_set_clkname(dev, "core");
> > +     if (IS_ERR(opp_table)) {
> > +             ret = PTR_ERR(opp_table);
> > +             goto err_fini;
> > +     }
> > +
> > +     pfdevfreq->clkname_opp_table = opp_table;
> > +
> >       opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
> >                                             pfdev->comp->num_supplies);
> >       if (IS_ERR(opp_table)) {
> > @@ -176,6 +184,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
> >               dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
> >               pfdevfreq->regulators_opp_table = NULL;
> >       }
> > +
> > +     if (pfdevfreq->clkname_opp_table) {
> > +             dev_pm_opp_put_clkname(pfdevfreq->clkname_opp_table);
> > +             pfdevfreq->clkname_opp_table = NULL;
> > +     }
> >   }
> >
> >   void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > index 347cde4786cf..1f2475e1d034 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > @@ -16,6 +16,7 @@ struct panfrost_device;
> >   struct panfrost_devfreq {
> >       struct devfreq *devfreq;
> >       struct opp_table *regulators_opp_table;
> > +     struct opp_table *clkname_opp_table;
> >       struct thermal_cooling_device *cooling;
> >       bool opp_of_table_added;
> >
> >
>

Re: [PATCH 10/15] drm/panfrost: add regulators to devfreq

[Clément Péron]

Hi Steven,

On Thu, 28 May 2020 at 15:23, Steven Price <steven.price@arm.com> wrote:
>
> On 10/05/2020 17:55, Clément Péron wrote:
> > Some OPP tables specify voltage for each frequency. Devfreq can
> > handle these regulators but they should be get only 1 time to avoid
> > issue and know who is in charge.
> >
> > If OPP table is probe don't init regulator.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
>
> This looks like it should work - thanks for doing this!

Yes but I'm not really happy how it's implemented.

Looks like a bit a workaround but didn't found a better solution.

Thanks for your review,
Clement

>
> Reviewed-by: Steven Price <steven.price@arm.com>
>
> > ---
> >   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 19 +++++++++++++++++++
> >   drivers/gpu/drm/panfrost/panfrost_devfreq.h |  2 ++
> >   drivers/gpu/drm/panfrost/panfrost_device.c  | 11 +++++++----
> >   3 files changed, 28 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index fce21c682414..9ffea0d4a087 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -93,6 +93,7 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >       unsigned long cur_freq;
> >       struct device *dev = &pfdev->pdev->dev;
> >       struct devfreq *devfreq;
> > +     struct opp_table *opp_table;
> >       struct thermal_cooling_device *cooling;
> >       struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> >
> > @@ -102,6 +103,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >
> >       spin_lock_init(&pfdevfreq->lock);
> >
> > +     opp_table = dev_pm_opp_set_regulators(dev, pfdev->comp->supply_names,
> > +                                           pfdev->comp->num_supplies);
> > +     if (IS_ERR(opp_table)) {
> > +             ret = PTR_ERR(opp_table);
> > +             /* Continue if the optional regulator is missing */
> > +             if (ret != -ENODEV) {
> > +                     DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n");
> > +                     goto err_fini;
> > +             }
> > +     } else {
> > +             pfdevfreq->regulators_opp_table = opp_table;
> > +     }
> > +
> >       ret = dev_pm_opp_of_add_table(dev);
> >       if (ret) {
> >               DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
> > @@ -157,6 +171,11 @@ void panfrost_devfreq_fini(struct panfrost_device *pfdev)
> >               dev_pm_opp_of_remove_table(&pfdev->pdev->dev);
> >               pfdevfreq->opp_of_table_added = false;
> >       }
> > +
> > +     if (pfdevfreq->regulators_opp_table) {
> > +             dev_pm_opp_put_regulators(pfdevfreq->regulators_opp_table);
> > +             pfdevfreq->regulators_opp_table = NULL;
> > +     }
> >   }
> >
> >   void panfrost_devfreq_resume(struct panfrost_device *pfdev)
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > index add203cb00c2..347cde4786cf 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
> > @@ -8,12 +8,14 @@
> >   #include <linux/ktime.h>
> >
> >   struct devfreq;
> > +struct opp_table;
> >   struct thermal_cooling_device;
> >
> >   struct panfrost_device;
> >
> >   struct panfrost_devfreq {
> >       struct devfreq *devfreq;
> > +     struct opp_table *regulators_opp_table;
> >       struct thermal_cooling_device *cooling;
> >       bool opp_of_table_added;
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index 67eedf64e82d..8b17fb2e3369 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -222,10 +222,13 @@ int panfrost_device_init(struct panfrost_device *pfdev)
> >               goto err_out0;
> >       }
> >
> > -     err = panfrost_regulator_init(pfdev);
> > -     if (err) {
> > -             dev_err(pfdev->dev, "regulator init failed %d\n", err);
> > -             goto err_out1;
> > +     /* OPP will handle regulators */
> > +     if (!pfdev->pfdevfreq.opp_of_table_added) {
> > +             err = panfrost_regulator_init(pfdev);
> > +             if (err) {
> > +                     dev_err(pfdev->dev, "regulator init failed %d\n", err);
> > +                     goto err_out1;
> > +             }
> >       }
> >
> >       err = panfrost_reset_init(pfdev);
> >
>

Re: [PATCH 08/15] drm/panfrost: move devfreq_init()/fini() in device

[Clément Péron]

Hi Steven

On Thu, 28 May 2020 at 15:22, Steven Price <steven.price@arm.com> wrote:
>
> On 10/05/2020 17:55, Clément Péron wrote:
> > Later we will introduce devfreq probing regulator if they
> > are present. As regulator should be probe only one time we
> > need to get this logic in the device_init().
> >
> > panfrost_device is already taking care of devfreq_resume()
> > and devfreq_suspend(), so it's not totally illogic to move
> > the devfreq_init() and devfreq_fini() here.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
> > ---
> >   drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++--------
> >   drivers/gpu/drm/panfrost/panfrost_drv.c    | 15 ++-------
> >   2 files changed, 25 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
> > index 8136babd3ba9..f480127205d6 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_device.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
> > @@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev)
> >               return err;
> >       }
> >
> > +     err = panfrost_devfreq_init(pfdev);
> > +     if (err) {
> > +             dev_err(pfdev->dev, "devfreq init failed %d\n", err);
> > +             goto err_out0;
> > +     }
> > +
> >       err = panfrost_regulator_init(pfdev);
> >       if (err) {
> >               dev_err(pfdev->dev, "regulator init failed %d\n", err);
> > -             goto err_out0;
> > +             goto err_out1;
>
> NIT: Rather than just renumbering these can we give them sensible names
> so we don't have this sort of refactoring in future?

Agree, I will change that in v2

>
> >       }
> >
> >       err = panfrost_reset_init(pfdev);
> >       if (err) {
> >               dev_err(pfdev->dev, "reset init failed %d\n", err);
> > -             goto err_out1;
> > +             goto err_out2;
> >       }
> >
> >       err = panfrost_pm_domain_init(pfdev);
> >       if (err)
> > -             goto err_out2;
> > +             goto err_out3;
> >
> >       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_out3;
> > +             goto err_out4;
> >       }
> >
> >       err = panfrost_gpu_init(pfdev);
> >       if (err)
> > -             goto err_out3;
> > +             goto err_out4;
> >
> >       err = panfrost_mmu_init(pfdev);
> >       if (err)
> > -             goto err_out4;
> > +             goto err_out5;
> >
> >       err = panfrost_job_init(pfdev);
> >       if (err)
> > -             goto err_out5;
> > +             goto err_out6;
> >
> >       err = panfrost_perfcnt_init(pfdev);
> >       if (err)
> > -             goto err_out6;
> > +             goto err_out7;
> >
> >       return 0;
> > -err_out6:
> > +err_out7:
> >       panfrost_job_fini(pfdev);
> > -err_out5:
> > +err_out6:
> >       panfrost_mmu_fini(pfdev);
> > -err_out4:
> > +err_out5:
> >       panfrost_gpu_fini(pfdev);
> > -err_out3:
> > +err_out4:
> >       panfrost_pm_domain_fini(pfdev);
> > -err_out2:
> > +err_out3:
> >       panfrost_reset_fini(pfdev);
> > -err_out1:
> > +err_out2:
> >       panfrost_regulator_fini(pfdev);
> > +err_out1:
> > +     panfrost_devfreq_fini(pfdev);
> >   err_out0:
> >       panfrost_clk_fini(pfdev);
> >       return err;
> > @@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
> >       panfrost_gpu_fini(pfdev);
> >       panfrost_pm_domain_fini(pfdev);
> >       panfrost_reset_fini(pfdev);
> > +     panfrost_devfreq_fini(pfdev);
> >       panfrost_regulator_fini(pfdev);
> >       panfrost_clk_fini(pfdev);
> >   }
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > index 882fecc33fdb..4dda68689015 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
> > @@ -14,7 +14,6 @@
> >   #include <drm/drm_utils.h>
> >
> >   #include "panfrost_device.h"
> > -#include "panfrost_devfreq.h"
> >   #include "panfrost_gem.h"
> >   #include "panfrost_mmu.h"
> >   #include "panfrost_job.h"
> > @@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev)
> >               goto err_out0;
> >       }
> >
> > -     err = panfrost_devfreq_init(pfdev);
> > -     if (err) {
> > -             if (err != -EPROBE_DEFER)
> > -                     dev_err(&pdev->dev, "Fatal error during devfreq init\n");
>
> You seem to have lost the check for EPROBE_DEFER during the move.

Correct, sorry for that, I will fix it in v2.

Thanks for your review,
Clement

>
> > -             goto err_out1;
> > -     }
> > -
> >       pm_runtime_set_active(pfdev->dev);
> >       pm_runtime_mark_last_busy(pfdev->dev);
> >       pm_runtime_enable(pfdev->dev);
> > @@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev)
> >        */
> >       err = drm_dev_register(ddev, 0);
> >       if (err < 0)
> > -             goto err_out2;
> > +             goto err_out1;
> >
> >       panfrost_gem_shrinker_init(ddev);
> >
> >       return 0;
> >
> > -err_out2:
> > -     pm_runtime_disable(pfdev->dev);
> > -     panfrost_devfreq_fini(pfdev);
> >   err_out1:
> > +     pm_runtime_disable(pfdev->dev);
> >       panfrost_device_fini(pfdev);
> >   err_out0:
> >       drm_dev_put(ddev);
> > @@ -650,7 +640,6 @@ static int panfrost_remove(struct platform_device *pdev)
> >       panfrost_gem_shrinker_cleanup(ddev);
> >
> >       pm_runtime_get_sync(pfdev->dev);
> > -     panfrost_devfreq_fini(pfdev);
> >       panfrost_device_fini(pfdev);
> >       pm_runtime_put_sync_suspend(pfdev->dev);
> >       pm_runtime_disable(pfdev->dev);
> >
>

Re: [PATCH 07/15] drm/panfrost: use device_property_present to check for OPP

[Clément Péron]

Hi Steven,

On Thu, 28 May 2020 at 15:22, Steven Price <steven.price@arm.com> wrote:
>
> On 10/05/2020 17:55, Clément Péron wrote:
> > Instead of expecting an error from dev_pm_opp_of_add_table()
> > do a simple device_property_present() check.
> >
> > Signed-off-by: Clément Péron <peron.clem@gmail.com>
>
> I'm not sure I understand why this is better. We seem to have more code
> to do roughly the same thing just with the hard-coded
> "operating-points-v2" name (if there's ever a 'v3' we'll then have to
> update this).
>
> Is the desire just to get an error on probe if the table is malformed?
> Have you hit this situation? If so this sounds like something which
> would be better fixed in the generic OPP code rather than Panfrost itself.

The idea was to avoid calling devfreq if there is no opp table.
But I think you're right we don't have to check for malformed
device-tree in the driver.

I will drop this patch,

Regards,
Clement


>
> Steve
>
> > ---
> >   drivers/gpu/drm/panfrost/panfrost_devfreq.c | 14 +++++++++-----
> >   1 file changed, 9 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > index d9007f44b772..fce21c682414 100644
> > --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c
> > @@ -96,15 +96,19 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev)
> >       struct thermal_cooling_device *cooling;
> >       struct panfrost_devfreq *pfdevfreq = &pfdev->pfdevfreq;
> >
> > -     ret = dev_pm_opp_of_add_table(dev);
> > -     if (ret == -ENODEV) /* Optional, continue without devfreq */
> > +     if (!device_property_present(dev, "operating-points-v2"))
> > +             /* Optional, continue without devfreq */
> >               return 0;
> > -     else if (ret)
> > -             return ret;
> > -     pfdevfreq->opp_of_table_added = true;
> >
> >       spin_lock_init(&pfdevfreq->lock);
> >
> > +     ret = dev_pm_opp_of_add_table(dev);
> > +     if (ret) {
> > +             DRM_DEV_ERROR(dev, "Couldn't add OPP table\n");
> > +             goto err_fini;
> > +     }
> > +     pfdevfreq->opp_of_table_added = true;
> > +
> >       panfrost_devfreq_reset(pfdevfreq);
> >
> >       cur_freq = clk_get_rate(pfdev->clock);
> >
>

Re: [PATCH 05/15] drm/panfrost: use spinlock instead of atomic

[Steven Price]

On 29/05/2020 13:35, Clément Péron wrote:
> Hi Robin,
> 
> On Fri, 29 May 2020 at 14:20, Robin Murphy <robin.murphy@arm.com> wrote:
>>
>> On 2020-05-10 17:55, Clément Péron wrote:
>>> Convert busy_count to a simple int protected by spinlock.
>>
>> A little more reasoning might be nice.
> 
> I have follow the modification requested for lima devfreq and clearly
> don't have any argument to switch to spinlock.
> 
> The Lima Maintainer asked to change witht the following reason :
> "Better make this count a normal int which is also protected by the spinlock,
> because current implementation can't protect atomic ops for state change
> and busy idle check and we are using spinlock already"
> 
>>
>>> Signed-off-by: Clément Péron <peron.clem@gmail.com>
>>> ---
>> [...]
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.h b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
>>> index 0697f8d5aa34..e6629900a618 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.h
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.h
>>> @@ -4,6 +4,7 @@
>>>    #ifndef __PANFROST_DEVFREQ_H__
>>>    #define __PANFROST_DEVFREQ_H__
>>>
>>> +#include <linux/spinlock.h>
>>>    #include <linux/ktime.h>
>>>
>>>    struct devfreq;
>>> @@ -14,10 +15,17 @@ struct panfrost_device;
>>>    struct panfrost_devfreq {
>>>        struct devfreq *devfreq;
>>>        struct thermal_cooling_device *cooling;
>>> +
>>>        ktime_t busy_time;
>>>        ktime_t idle_time;
>>>        ktime_t time_last_update;
>>> -     atomic_t busy_count;
>>> +     int busy_count;
>>> +     /*
>>> +      * Protect busy_time, idle_time, time_last_update and busy_count
>>> +      * because these can be updated concurrently, for example by the GP
>>> +      * and PP interrupts.
>>> +      */
>>
>> Nit: this comment is clearly wrong, since we only have Job, GPU and MMU
>> interrupts here. I guess if there is a race it would be between
>> submission/completion/timeout on different job slots.
> 
> It's copy/paste from lima I will update it,

Lima ('Utgard') has separate units for geometry and pixel processing 
(GP/PP). For Panfrost ('Midgard'/'Bifrost') we don't have that 
separation, however there are multiple job slots. which are implemented 
as multiple DRM schedulers. So the same fix is appropriate, but clearly 
I missed this comment because it's referring to GP/PP which don't exist 
for Midgard/Bifrost.

>>
>> Given that, should this actually be considered a fix for 9e62b885f715
>> ("drm/panfrost: Simplify devfreq utilisation tracking")?
> 
> I can't say if it can be considered as a fix, I didn't see any
> improvement on my board before and after this patch.
> I'm still facing some issue and didn't have time to fully investigate it.

Technically this is a fix - there's a small race which could cause the 
devfreq information to become corrupted. However it would resolve itself 
on the next devfreq interval when panfrost_devfreq_reset() is called. So 
the impact is very minor (devfreq gets some bogus figures). The 
important variable (busy_count) was already an atomic so won't be affected.

Steve

> Thanks for you review,
> 
> 
>>
>> Robin.

Re: [PATCH 08/15] drm/panfrost: move devfreq_init()/fini() in device

[Tomeu Vizoso]

On 5/29/20 2:38 PM, Clément Péron wrote:
> Hi Steven
> 
> On Thu, 28 May 2020 at 15:22, Steven Price <steven.price@arm.com> wrote:
>>
>> On 10/05/2020 17:55, Clément Péron wrote:
>>> Later we will introduce devfreq probing regulator if they
>>> are present. As regulator should be probe only one time we
>>> need to get this logic in the device_init().
>>>
>>> panfrost_device is already taking care of devfreq_resume()
>>> and devfreq_suspend(), so it's not totally illogic to move
>>> the devfreq_init() and devfreq_fini() here.
>>>
>>> Signed-off-by: Clément Péron <peron.clem@gmail.com>
>>> ---
>>>    drivers/gpu/drm/panfrost/panfrost_device.c | 37 ++++++++++++++--------
>>>    drivers/gpu/drm/panfrost/panfrost_drv.c    | 15 ++-------
>>>    2 files changed, 25 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_device.c b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> index 8136babd3ba9..f480127205d6 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_device.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_device.c
>>> @@ -212,59 +212,67 @@ int panfrost_device_init(struct panfrost_device *pfdev)
>>>                return err;
>>>        }
>>>
>>> +     err = panfrost_devfreq_init(pfdev);
>>> +     if (err) {
>>> +             dev_err(pfdev->dev, "devfreq init failed %d\n", err);
>>> +             goto err_out0;
>>> +     }
>>> +
>>>        err = panfrost_regulator_init(pfdev);
>>>        if (err) {
>>>                dev_err(pfdev->dev, "regulator init failed %d\n", err);
>>> -             goto err_out0;
>>> +             goto err_out1;
>>
>> NIT: Rather than just renumbering these can we give them sensible names
>> so we don't have this sort of refactoring in future?
> 
> Agree, I will change that in v2

FWIW, there's a sensible approach described in 
Documentation/process/coding-style.rst ("7) Centralized exiting of 
functions").

Regards,

Tomeu

> 
>>
>>>        }
>>>
>>>        err = panfrost_reset_init(pfdev);
>>>        if (err) {
>>>                dev_err(pfdev->dev, "reset init failed %d\n", err);
>>> -             goto err_out1;
>>> +             goto err_out2;
>>>        }
>>>
>>>        err = panfrost_pm_domain_init(pfdev);
>>>        if (err)
>>> -             goto err_out2;
>>> +             goto err_out3;
>>>
>>>        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_out3;
>>> +             goto err_out4;
>>>        }
>>>
>>>        err = panfrost_gpu_init(pfdev);
>>>        if (err)
>>> -             goto err_out3;
>>> +             goto err_out4;
>>>
>>>        err = panfrost_mmu_init(pfdev);
>>>        if (err)
>>> -             goto err_out4;
>>> +             goto err_out5;
>>>
>>>        err = panfrost_job_init(pfdev);
>>>        if (err)
>>> -             goto err_out5;
>>> +             goto err_out6;
>>>
>>>        err = panfrost_perfcnt_init(pfdev);
>>>        if (err)
>>> -             goto err_out6;
>>> +             goto err_out7;
>>>
>>>        return 0;
>>> -err_out6:
>>> +err_out7:
>>>        panfrost_job_fini(pfdev);
>>> -err_out5:
>>> +err_out6:
>>>        panfrost_mmu_fini(pfdev);
>>> -err_out4:
>>> +err_out5:
>>>        panfrost_gpu_fini(pfdev);
>>> -err_out3:
>>> +err_out4:
>>>        panfrost_pm_domain_fini(pfdev);
>>> -err_out2:
>>> +err_out3:
>>>        panfrost_reset_fini(pfdev);
>>> -err_out1:
>>> +err_out2:
>>>        panfrost_regulator_fini(pfdev);
>>> +err_out1:
>>> +     panfrost_devfreq_fini(pfdev);
>>>    err_out0:
>>>        panfrost_clk_fini(pfdev);
>>>        return err;
>>> @@ -278,6 +286,7 @@ void panfrost_device_fini(struct panfrost_device *pfdev)
>>>        panfrost_gpu_fini(pfdev);
>>>        panfrost_pm_domain_fini(pfdev);
>>>        panfrost_reset_fini(pfdev);
>>> +     panfrost_devfreq_fini(pfdev);
>>>        panfrost_regulator_fini(pfdev);
>>>        panfrost_clk_fini(pfdev);
>>>    }
>>> diff --git a/drivers/gpu/drm/panfrost/panfrost_drv.c b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> index 882fecc33fdb..4dda68689015 100644
>>> --- a/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> +++ b/drivers/gpu/drm/panfrost/panfrost_drv.c
>>> @@ -14,7 +14,6 @@
>>>    #include <drm/drm_utils.h>
>>>
>>>    #include "panfrost_device.h"
>>> -#include "panfrost_devfreq.h"
>>>    #include "panfrost_gem.h"
>>>    #include "panfrost_mmu.h"
>>>    #include "panfrost_job.h"
>>> @@ -606,13 +605,6 @@ static int panfrost_probe(struct platform_device *pdev)
>>>                goto err_out0;
>>>        }
>>>
>>> -     err = panfrost_devfreq_init(pfdev);
>>> -     if (err) {
>>> -             if (err != -EPROBE_DEFER)
>>> -                     dev_err(&pdev->dev, "Fatal error during devfreq init\n");
>>
>> You seem to have lost the check for EPROBE_DEFER during the move.
> 
> Correct, sorry for that, I will fix it in v2.
> 
> Thanks for your review,
> Clement
> 
>>
>>> -             goto err_out1;
>>> -     }
>>> -
>>>        pm_runtime_set_active(pfdev->dev);
>>>        pm_runtime_mark_last_busy(pfdev->dev);
>>>        pm_runtime_enable(pfdev->dev);
>>> @@ -625,16 +617,14 @@ static int panfrost_probe(struct platform_device *pdev)
>>>         */
>>>        err = drm_dev_register(ddev, 0);
>>>        if (err < 0)
>>> -             goto err_out2;
>>> +             goto err_out1;
>>>
>>>        panfrost_gem_shrinker_init(ddev);
>>>
>>>        return 0;
>>>
>>> -err_out2:
>>> -     pm_runtime_disable(pfdev->dev);
>>> -     panfrost_devfreq_fini(pfdev);
>>>    err_out1:
>>> +     pm_runtime_disable(pfdev->dev);
>>>        panfrost_device_fini(pfdev);
>>>    err_out0:
>>>        drm_dev_put(ddev);
>>> @@ -650,7 +640,6 @@ static int panfrost_remove(struct platform_device *pdev)
>>>        panfrost_gem_shrinker_cleanup(ddev);
>>>
>>>        pm_runtime_get_sync(pfdev->dev);
>>> -     panfrost_devfreq_fini(pfdev);
>>>        panfrost_device_fini(pfdev);
>>>        pm_runtime_put_sync_suspend(pfdev->dev);
>>>        pm_runtime_disable(pfdev->dev);
>>>
>>