Messages in this thread | | | Date | Fri, 18 May 2018 00:16:07 -0700 | From | skannan@codeauro ... | Subject | Re: [PATCH] PM / devfreq: Add support for QCOM devfreq FW |
| |
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@codeaurora.org> >> --- >> .../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
| |