Re: [PATCH] PM / devfreq: Add support for QCOM devfreq FW

From: skannan
Date: Fri May 18 2018 - 02:19:55 EST

On 2018-05-17 19:28, Chanwoo Choi wrote:
Hi,

On 2018ë 05ì 17ì 15:02, Saravana Kannan wrote:
The firmware present in some QCOM chipsets offloads the steps necessary for
changing the frequency of some devices (Eg: L3). This driver implements the
devfreq interface for this firmware so that various governors could be used
to scale the frequency of these devices.

The description doesn't include what kind of firmware. You have to explain
the type and role of firmware.

Not exactly sure what you mean. I described exactly what the firmware does. Not sure what you mean by what kind of firmware. I just understand the interface with it -- not the implementation of the firmware or hardware.

And it doesn't contain the description
of correlation
between 'qcom,devfreq-fw' and 'qcom,devfreq-fw-voter'.


Will do.


Signed-off-by: Saravana Kannan <skannan@xxxxxxxxxxxxxx>
---
.../bindings/devfreq/devfreq-qcom-fw.txt | 31 ++
drivers/devfreq/Kconfig | 14 +
drivers/devfreq/Makefile | 1 +
drivers/devfreq/devfreq_qcom_fw.c | 326 +++++++++++++++++++++
4 files changed, 372 insertions(+)
create mode 100644 Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
create mode 100644 drivers/devfreq/devfreq_qcom_fw.c

diff --git a/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
new file mode 100644
index 0000000..5e1aecf
--- /dev/null
+++ b/Documentation/devicetree/bindings/devfreq/devfreq-qcom-fw.txt
@@ -0,0 +1,31 @@
+QCOM Devfreq FW device
+
+Some Qualcomm Technologies, Inc. (QTI) chipsets have a FW that offloads the
+child devices of the corresponding qcom,devfreq-fw device.
+
+Required properties:
+- compatible: Must be "qcom,devfreq-fw" or "qcom,devfreq-fw-voter"

The use of 'devfreq' word is not proper because 'devfreq' is framework name.
I think you have to use the specific SoC name for the compatible.

I don't think it's mandatory to use chip name. Typically you pick the IP name. This IP is a firmware just for scaling device frequency - so called it qcom,devfreq-fw. I'll see what the DT maintainers say about this.

In the future,
if you need to support the new SoC with this driver, just you can add
the new compatible.

+Only for qcom,devfreq-fw:
+- reg: Pairs of physical base addresses and region sizes of
+ memory mapped registers.
+- reg-names: Names of the bases for the above registers. Expected
+ bases are: "en-base", "lut-base" and "perf-base".

It is not possible to understand what are meaning of "en-base",
"lut-base" and "perf-base".
because they depend on specific H/W specification. You have to add the
detailed description
why they are necessary. Also, you should explain whether thery are
mandatory or optional.

The are all mandatory, that's why I didn't have the "Optional" heading. I can add descriptions.


+
+Example:
+
+ qcom,devfreq-l3 {
+ compatible = "qcom,devfreq-fw";
+ reg-names = "en-base", "lut-base", "perf-base";
+ reg = <0x18321000 0x4>, <0x18321110 0x600>, <0x18321920 0x4>;
+
+ qcom,cpu0-l3 {
+ compatible = "qcom,devfreq-fw-voter";
+ };
+
+ qcom,cpu4-l3 {
+ compatible = "qcom,devfreq-fw-voter";
+ };
+ };
diff --git a/drivers/devfreq/Kconfig b/drivers/devfreq/Kconfig
index 6a172d3..8503018 100644
--- a/drivers/devfreq/Kconfig
+++ b/drivers/devfreq/Kconfig
@@ -113,6 +113,20 @@ config ARM_RK3399_DMC_DEVFREQ
It sets the frequency for the memory controller and reads the usage counts
from hardware.

+config ARM_QCOM_DEVFREQ_FW
+ bool "Qualcomm Technologies Inc. DEVFREQ FW driver"
+ depends on ARCH_QCOM
+ select DEVFREQ_GOV_PERFORMANCE
+ select DEVFREQ_GOV_POWERSAVE
+ select DEVFREQ_GOV_USERSPACE
+ default n
+ help
+ The firmware present in some QCOM chipsets offloads the steps
+ necessary for changing the frequency of some devices (Eg: L3). This
+ driver implements the devfreq interface for this firmware so that
+ various governors could be used to scale the frequency of these
+ devices.

As I commented, you need to add a description of what kind of firmwar.

Again, not sure what you mean by "what kind of firmware". It's exactly what I've described here. Can you please elaborate?

+
source "drivers/devfreq/event/Kconfig"

endif # PM_DEVFREQ
diff --git a/drivers/devfreq/Makefile b/drivers/devfreq/Makefile
index 32b8d4d..f1cc8990 100644
--- a/drivers/devfreq/Makefile
+++ b/drivers/devfreq/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_DEVFREQ_GOV_PASSIVE) += governor_passive.o
obj-$(CONFIG_ARM_EXYNOS_BUS_DEVFREQ) += exynos-bus.o
obj-$(CONFIG_ARM_RK3399_DMC_DEVFREQ) += rk3399_dmc.o
obj-$(CONFIG_ARM_TEGRA_DEVFREQ) += tegra-devfreq.o
+obj-$(CONFIG_ARM_QCOM_DEVFREQ_FW) += devfreq_qcom_fw.o

# DEVFREQ Event Drivers
obj-$(CONFIG_PM_DEVFREQ_EVENT) += event/
diff --git a/drivers/devfreq/devfreq_qcom_fw.c b/drivers/devfreq/devfreq_qcom_fw.c
new file mode 100644
index 0000000..3e85f76
--- /dev/null
+++ b/drivers/devfreq/devfreq_qcom_fw.c
@@ -0,0 +1,326 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.

Is it right? or Qualcomm?

Yup, it's right.


+ */
+
+#include <linux/err.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/list.h>
+#include <linux/devfreq.h>
+#include <linux/pm_opp.h>
+
+#define INIT_RATE 300000000UL
+#define XO_RATE 19200000UL
+#define LUT_MAX_ENTRIES 40U
+#define LUT_ROW_SIZE 32

I don't know what are the meaning of 'XO', 'LUT'.

XO is a very common term for crystal oscillator. I don't want to change these symbols.
LUT stands for Look up table. But I can rename it to FREQ_TBL_.

Thanks,
Saravana