lkml.org 
[lkml]   [2018]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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

\
 
 \ /
  Last update: 2018-05-18 09:16    [W:0.057 / U:9.856 seconds]
©2003-2018 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site