Message-ID: <CAAhV-H74HJr0=8g0GtHj=zZH5nJijRpc90zLLRY_sXJfKFVtHA@mail.gmail.com>
Date: Wed, 3 Jul 2024 22:37:47 +0800
From: Huacai Chen <chenhuacai@...nel.org>
To: Viresh Kumar <viresh.kumar@...aro.org>
Cc: Huacai Chen <chenhuacai@...ngson.cn>, "Rafael J . Wysocki" <rafael@...nel.org>, loongarch@...ts.linux.dev, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	Xuerui Wang <kernel@...0n.name>, Jiaxun Yang <jiaxun.yang@...goat.com>, 
	Binbin Zhou <zhoubinbin@...ngson.cn>
Subject: Re: [PATCH V2 2/2] cpufreq: Add Loongson-3 CPUFreq driver support

Hi, Viresh,

On Wed, Jul 3, 2024 at 6:18 PM Viresh Kumar <viresh.kumar@...aro.org> wrote:
>
> On 02-07-24, 23:27, Huacai Chen wrote:
> > Some of LoongArch processors (Loongson-3 series) support DVFS, their
> > IOCSR.FEATURES has IOCSRF_FREQSCALE set. And they has a micro-core in
> > the package called SMC (System Management Controller), which can be
> > used to detect temperature, control fans, scale frequency and voltage,
> > etc.
> >
> > The Loongson-3 CPUFreq driver is very simple now, it communicate with
> > SMC, get DVFS info, set target frequency from CPUFreq core, and so on.
> >
> > There is a command list to interact with SMC, widely-used commands in
> > the CPUFreq driver include:
> >
> > CMD_GET_VERSION: Get SMC firmware version.
> >
> > CMD_GET_FEATURE: Get enabled SMC features.
> >
> > CMD_SET_FEATURE: Enable SMC features, such as basic DVFS, BOOST.
> >
> > CMD_GET_FREQ_LEVEL_NUM: Get the number of normal frequency levels.
> >
> > CMD_GET_FREQ_BOOST_NUM: Get the number of boost frequency levels.
> >
> > CMD_GET_FREQ_LEVEL_INFO: Get the detail info of a frequency level.
> >
> > CMD_GET_FREQ_INFO: Get the current frequency.
> >
> > CMD_SET_FREQ_INFO: Set the target frequency.
> >
> > In future we will add automatic frequency scaling, which is similar to
> > Intel's HWP (HardWare P-State).
> >
> > Signed-off-by: Binbin Zhou <zhoubinbin@...ngson.cn>
> > Signed-off-by: Huacai Chen <chenhuacai@...ngson.cn>
> > ---
> >  MAINTAINERS                         |   1 +
> >  drivers/cpufreq/Kconfig             |  12 +
> >  drivers/cpufreq/Makefile            |   1 +
> >  drivers/cpufreq/loongson3_cpufreq.c | 399 ++++++++++++++++++++++++++++
> >  4 files changed, 413 insertions(+)
> >  create mode 100644 drivers/cpufreq/loongson3_cpufreq.c
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index cd2ca0c3158e..2af33319f1ff 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -12968,6 +12968,7 @@ F:    Documentation/arch/loongarch/
> >  F:   Documentation/translations/zh_CN/arch/loongarch/
> >  F:   arch/loongarch/
> >  F:   drivers/*/*loongarch*
> > +F:   drivers/cpufreq/loongson3_cpufreq.c
> >
> >  LOONGSON GPIO DRIVER
> >  M:   Yinbo Zhu <zhuyinbo@...ngson.cn>
> > diff --git a/drivers/cpufreq/Kconfig b/drivers/cpufreq/Kconfig
> > index 94e55c40970a..10cda6f2fe1d 100644
> > --- a/drivers/cpufreq/Kconfig
> > +++ b/drivers/cpufreq/Kconfig
> > @@ -262,6 +262,18 @@ config LOONGSON2_CPUFREQ
> >         If in doubt, say N.
> >  endif
> >
> > +if LOONGARCH
> > +config LOONGSON3_CPUFREQ
> > +     tristate "Loongson3 CPUFreq Driver"
> > +     help
> > +       This option adds a CPUFreq driver for Loongson processors which
> > +       support software configurable cpu frequency.
> > +
> > +       Loongson-3 family processors support this feature.
> > +
> > +       If in doubt, say N.
> > +endif
> > +
> >  if SPARC64
> >  config SPARC_US3_CPUFREQ
> >       tristate "UltraSPARC-III CPU Frequency driver"
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index 8d141c71b016..0f184031dd12 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -103,6 +103,7 @@ obj-$(CONFIG_POWERNV_CPUFREQ)             += powernv-cpufreq.o
> >  # Other platform drivers
> >  obj-$(CONFIG_BMIPS_CPUFREQ)          += bmips-cpufreq.o
> >  obj-$(CONFIG_LOONGSON2_CPUFREQ)              += loongson2_cpufreq.o
> > +obj-$(CONFIG_LOONGSON3_CPUFREQ)              += loongson3_cpufreq.o
> >  obj-$(CONFIG_SH_CPU_FREQ)            += sh-cpufreq.o
> >  obj-$(CONFIG_SPARC_US2E_CPUFREQ)     += sparc-us2e-cpufreq.o
> >  obj-$(CONFIG_SPARC_US3_CPUFREQ)              += sparc-us3-cpufreq.o
> > diff --git a/drivers/cpufreq/loongson3_cpufreq.c b/drivers/cpufreq/loongson3_cpufreq.c
> > new file mode 100644
> > index 000000000000..6d7da2238542
> > --- /dev/null
> > +++ b/drivers/cpufreq/loongson3_cpufreq.c
> > @@ -0,0 +1,399 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * CPUFreq driver for the loongson-3 processors
>
> A full stop at the end please.
OK, thanks.

>
> > + *
> > + * All revisions of Loongson-3 processor support this feature.
>
> What do you mean by `feature` here ?
Means "cpu_has_freqscale", will modify in next version.

>
> > + *
> > + * Author: Huacai Chen <chenhuacai@...ngson.cn>
> > + * Copyright (C) 2020-2024 Loongson Technology Corporation Limited
>
> Can't really use 2020 here. We are adding the driver for the first
> time in 2024 only.
OK, thanks.

>
> > + */
> > +#include <linux/delay.h>
> > +#include <linux/module.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/units.h>
>
> Please keep them in alphabetical order.  And you are missing few other
> headers (which are getting included indirectly for now). Like for
> mutex. Please cross check all types you are using and include their
> headers directly.
OK, thanks.

>
> > +
> > +#include <asm/idle.h>
> > +#include <asm/loongarch.h>
> > +#include <asm/loongson.h>
> > +
> > +/* Message */
> > +union smc_message {
> > +     u32 value;
> > +     struct {
> > +             u32 id          : 4;
> > +             u32 info        : 4;
> > +             u32 val         : 16;
> > +             u32 cmd         : 6;
> > +             u32 extra       : 1;
> > +             u32 complete    : 1;
> > +     };
> > +};
> > +
> > +/* Command return values */
> > +#define CMD_OK                               0  /* No error */
> > +#define CMD_ERROR                    1  /* Regular error */
> > +#define CMD_NOCMD                    2  /* Command does not support */
> > +#define CMD_INVAL                    3  /* Invalid Parameter */
> > +
> > +/* Version commands */
> > +/*
> > + * CMD_GET_VERSION - Get interface version
> > + * Input: none
> > + * Output: version
> > + */
> > +#define CMD_GET_VERSION                      0x1
> > +
> > +/* Feature commands */
> > +/*
> > + * CMD_GET_FEATURE - Get feature state
> > + * Input: feature ID
> > + * Output: feature flag
> > + */
> > +#define CMD_GET_FEATURE                      0x2
> > +
> > +/*
> > + * CMD_SET_FEATURE - Set feature state
> > + * Input: feature ID, feature flag
> > + * output: none
> > + */
> > +#define CMD_SET_FEATURE                      0x3
> > +
> > +/* Feature IDs */
> > +#define FEATURE_SENSOR                       0
> > +#define FEATURE_FAN                  1
> > +#define FEATURE_DVFS                 2
> > +
> > +/* Sensor feature flags */
> > +#define FEATURE_SENSOR_ENABLE                BIT(0)
> > +#define FEATURE_SENSOR_SAMPLE                BIT(1)
> > +
> > +/* Fan feature flags */
> > +#define FEATURE_FAN_ENABLE           BIT(0)
> > +#define FEATURE_FAN_AUTO             BIT(1)
> > +
> > +/* DVFS feature flags */
> > +#define FEATURE_DVFS_ENABLE          BIT(0)
> > +#define FEATURE_DVFS_BOOST           BIT(1)
> > +#define FEATURE_DVFS_AUTO            BIT(2)
> > +#define FEATURE_DVFS_SINGLE_BOOST    BIT(3)
> > +
> > +/* Sensor commands */
> > +/*
> > + * CMD_GET_SENSOR_NUM - Get number of sensors
> > + * Input: none
> > + * Output: number
> > + */
> > +#define CMD_GET_SENSOR_NUM           0x4
> > +
> > +/*
> > + * CMD_GET_SENSOR_STATUS - Get sensor status
> > + * Input: sensor ID, type
> > + * Output: sensor status
> > + */
> > +#define CMD_GET_SENSOR_STATUS                0x5
> > +
> > +/* Sensor types */
> > +#define SENSOR_INFO_TYPE             0
> > +#define SENSOR_INFO_TYPE_TEMP                1
> > +
> > +/* Fan commands */
> > +/*
> > + * CMD_GET_FAN_NUM - Get number of fans
> > + * Input: none
> > + * Output: number
> > + */
> > +#define CMD_GET_FAN_NUM                      0x6
> > +
> > +/*
> > + * CMD_GET_FAN_INFO - Get fan status
> > + * Input: fan ID, type
> > + * Output: fan info
> > + */
> > +#define CMD_GET_FAN_INFO             0x7
> > +
> > +/*
> > + * CMD_SET_FAN_INFO - Set fan status
> > + * Input: fan ID, type, value
> > + * Output: none
> > + */
> > +#define CMD_SET_FAN_INFO             0x8
> > +
> > +/* Fan types */
> > +#define FAN_INFO_TYPE_LEVEL          0
> > +
> > +/* DVFS commands */
> > +/*
> > + * CMD_GET_FREQ_LEVEL_NUM - Get number of freq levels
> > + * Input: CPU ID
> > + * Output: number
> > + */
> > +#define CMD_GET_FREQ_LEVEL_NUM               0x9
> > +
> > +/*
> > + * CMD_GET_FREQ_BOOST_LEVEL - Get number of boost levels
> > + * Input: CPU ID
> > + * Output: number
> > + */
> > +#define CMD_GET_FREQ_BOOST_LEVEL     0x10
> > +
> > +/*
> > + * CMD_GET_FREQ_LEVEL_INFO - Get freq level info
> > + * Input: CPU ID, level ID
> > + * Output: level info
> > + */
> > +#define CMD_GET_FREQ_LEVEL_INFO              0x11
> > +
> > +/*
> > + * CMD_GET_FREQ_INFO - Get freq info
> > + * Input: CPU ID, type
> > + * Output: freq info
> > + */
> > +#define CMD_GET_FREQ_INFO            0x12
> > +
> > +/*
> > + * CMD_SET_FREQ_INFO - Set freq info
> > + * Input: CPU ID, type, value
> > + * Output: none
> > + */
> > +#define CMD_SET_FREQ_INFO            0x13
> > +
> > +/* Freq types */
> > +#define FREQ_INFO_TYPE_FREQ          0
> > +#define FREQ_INFO_TYPE_LEVEL         1
> > +
> > +#define FREQ_MAX_LEVEL                       (16 + 1)
> > +
> > +enum freq {
> > +     FREQ_LEV0, /* Reserved */
> > +     FREQ_LEV1, FREQ_LEV2, FREQ_LEV3, FREQ_LEV4,
> > +     FREQ_LEV5, FREQ_LEV6, FREQ_LEV7, FREQ_LEV8,
> > +     FREQ_LEV9, FREQ_LEV10, FREQ_LEV11, FREQ_LEV12,
> > +     FREQ_LEV13, FREQ_LEV14, FREQ_LEV15, FREQ_LEV16,
> > +     FREQ_RESV
> > +};
> > +
> > +static struct mutex cpufreq_mutex[MAX_PACKAGES];
> > +static struct cpufreq_driver loongson3_cpufreq_driver;
> > +static DEFINE_PER_CPU(struct cpufreq_frequency_table *, freq_table);
> > +
> > +static inline int do_service_request(u32 id, u32 info, u32 cmd, u32 val, u32 extra)
> > +{
> > +     int retries;
> > +     unsigned int cpu = smp_processor_id();
> > +     unsigned int package = cpu_data[cpu].package;
> > +     union smc_message msg, last;
> > +
> > +     mutex_lock(&cpufreq_mutex[package]);
> > +
> > +     last.value = iocsr_read32(LOONGARCH_IOCSR_SMCMBX);
> > +     if (!last.complete) {
> > +             mutex_unlock(&cpufreq_mutex[package]);
> > +             return -EPERM;
> > +     }
> > +
> > +     msg.id          = id;
> > +     msg.info        = info;
> > +     msg.cmd         = cmd;
> > +     msg.val         = val;
> > +     msg.extra       = extra;
> > +     msg.complete    = 0;
> > +
> > +     iocsr_write32(msg.value, LOONGARCH_IOCSR_SMCMBX);
> > +     iocsr_write32(iocsr_read32(LOONGARCH_IOCSR_MISC_FUNC) | IOCSR_MISC_FUNC_SOFT_INT,
> > +                   LOONGARCH_IOCSR_MISC_FUNC);
> > +
> > +     for (retries = 0; retries < 10000; retries++) {
> > +             msg.value = iocsr_read32(LOONGARCH_IOCSR_SMCMBX);
> > +             if (msg.complete)
> > +                     break;
> > +
> > +             usleep_range(8, 12);
> > +     }
> > +
> > +     if (!msg.complete || msg.cmd != CMD_OK) {
> > +             mutex_unlock(&cpufreq_mutex[package]);
> > +             return -EPERM;
> > +     }
> > +
> > +     mutex_unlock(&cpufreq_mutex[package]);
> > +
> > +     return msg.val;
>
> Thanks, this looks much better now.
>
> > +}
> > +
> > +static unsigned int loongson3_cpufreq_get(unsigned int cpu)
> > +{
> > +     int ret;
> > +
> > +     ret = do_service_request(cpu, FREQ_INFO_TYPE_FREQ, CMD_GET_FREQ_INFO, 0, 0);
> > +
> > +     return ret * KILO;
> > +}
> > +
> > +static int loongson3_cpufreq_set(struct cpufreq_policy *policy, int freq_level)
> > +{
> > +     return do_service_request(cpu_data[policy->cpu].core, FREQ_INFO_TYPE_LEVEL, CMD_SET_FREQ_INFO, freq_level, 0);
> > +}
> > +
> > +/*
> > + * Here we notify other drivers of the proposed change and the final change.
>
> What do you mean by other drivers here ? I would just drop the
> comment.
OK, it will be dropped.

>
> > + */
> > +static int loongson3_cpufreq_target(struct cpufreq_policy *policy, unsigned int index)
> > +{
> > +     /* setting the cpu frequency */
>
> The obvious comments can be dropped.
OK, it will be dropped.

>
> > +     return loongson3_cpufreq_set(policy, index);
>
> Why use a separate function for calling do_service_request() ? Just
> open code it here.
Hmm, there is a loongson3_cpufreq_get() function, so I make a
loongson3_cpufreq_set() function, too.

>
> > +}
> > +
> > +static int loongson3_cpufreq_get_freq_table(int cpu)
>
> If you want to simplify the naming a bit, you can just call all
> internal routines without `loongson3_cpufreq_` prefix. Just
> create_freq_table() would be appropriate here.
OK, I will rename this function.

>
> For all routines passed to core, via the cpufreq_driver pointers, you
> can keep using the prefix.
>
> > +{
> > +     int i, ret, boost_level, max_level, freq_level;
> > +     struct cpufreq_frequency_table *table;
> > +
> > +     if (per_cpu(freq_table, cpu))
> > +             return 0;
> > +
> > +     ret = do_service_request(cpu, 0, CMD_GET_FREQ_LEVEL_NUM, 0, 0);
> > +     if (ret < 0)
> > +             return ret;
> > +     max_level = ret;
> > +
> > +     ret = do_service_request(cpu, 0, CMD_GET_FREQ_BOOST_LEVEL, 0, 0);
> > +     if (ret < 0)
> > +             return ret;
> > +     boost_level = ret;
> > +
> > +     freq_level = min(max_level, FREQ_MAX_LEVEL);
> > +     table = kzalloc(sizeof(struct cpufreq_frequency_table) * (freq_level + 1), GFP_KERNEL);
>
> devm_kcalloc(pdev, ...) instead ?
I remember you told me this in V1, but devm_kalloc() needs a pdev
instance, which doesn't exist here, so I keep kzalloc().

>
> sizeof(*table) instead.
OK, thanks.

>
> Also please run `scripts/checkpatch.pl --strict` on your patches to
> find out general formatting issues.
OK, I will run checkpatch.pl.

>
> > +     if (!table)
> > +             return -ENOMEM;
> > +
> > +     for (i = 0; i < freq_level; i++) {
> > +             ret = do_service_request(cpu, FREQ_INFO_TYPE_FREQ, CMD_GET_FREQ_LEVEL_INFO, i, 0);
> > +             if (ret < 0) {
> > +                     kfree(table);
> > +                     return ret;
> > +             }
> > +
> > +             table[i].frequency = ret * KILO;
>
> > +             table[i].driver_data = FREQ_LEV0 + i;
>
> I don't think you are using this, you don't have to fill it at all.
OK, I will drop it.

>
> > +             table[i].flags = (i >= boost_level) ? CPUFREQ_BOOST_FREQ : 0;
> > +     }
> > +
> > +     table[freq_level].frequency = CPUFREQ_TABLE_END;
> > +     table[freq_level].driver_data = FREQ_RESV;
> > +     table[freq_level].flags = 0;
> > +
> > +     per_cpu(freq_table, cpu) = table;
> > +
> > +     return 0;
> > +}
> > +
> > +static int loongson3_cpufreq_cpu_init(struct cpufreq_policy *policy)
> > +{
> > +     int ret;
> > +
> > +     ret = loongson3_cpufreq_get_freq_table(policy->cpu);
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     policy->cur = loongson3_cpufreq_get(policy->cpu);
>
> cpufreq core does this during initialization. The drivers don't need
> to do it.
OK, I will drop it.

>
> > +     policy->cpuinfo.transition_latency = 10000;
> > +     policy->freq_table = per_cpu(freq_table, policy->cpu);
> > +     cpumask_copy(policy->cpus, topology_sibling_cpumask(policy->cpu));
> > +
> > +     if (policy_has_boost_freq(policy)) {
> > +             ret = cpufreq_enable_boost_support();
> > +             if (ret < 0) {
> > +                     pr_warn("cpufreq: Failed to enable boost: %d\n", ret);
> > +                     return ret;
> > +             }
> > +             loongson3_cpufreq_driver.boost_enabled = true;
> > +     }
> > +
> > +     return 0;
> > +}
> > +
> > +static int loongson3_cpufreq_cpu_exit(struct cpufreq_policy *policy)
> > +{
> > +     kfree(policy->freq_table);
> > +     return 0;
> > +}
> > +
> > +static int loongson3_cpufreq_cpu_online(struct cpufreq_policy *policy)
> > +{
> > +     return 0;
> > +}
> > +
> > +static int loongson3_cpufreq_cpu_offline(struct cpufreq_policy *policy)
> > +{
> > +     return 0;
> > +}
> > +
> > +static struct cpufreq_driver loongson3_cpufreq_driver = {
> > +     .name = "loongson3",
> > +     .flags = CPUFREQ_CONST_LOOPS,
> > +     .init = loongson3_cpufreq_cpu_init,
> > +     .exit = loongson3_cpufreq_cpu_exit,
> > +     .online = loongson3_cpufreq_cpu_online,
> > +     .offline = loongson3_cpufreq_cpu_offline,
> > +     .verify = cpufreq_generic_frequency_table_verify,
> > +     .target_index = loongson3_cpufreq_target,
> > +     .get = loongson3_cpufreq_get,
> > +     .attr = cpufreq_generic_attr,
> > +};
> > +
> > +static int configure_cpufreq_info(void)
> > +{
> > +     int ret;
> > +
> > +     ret = do_service_request(0, 0, CMD_GET_VERSION, 0, 0);
> > +     if (ret <= 0)
> > +             return -EPERM;
> > +
> > +     return do_service_request(FEATURE_DVFS, 0, CMD_SET_FEATURE, FEATURE_DVFS_ENABLE | FEATURE_DVFS_BOOST, 0);
> > +}
> > +
> > +static int loongson3_cpufreq_probe(struct platform_device *pdev)
> > +{
> > +     int i, ret;
> > +
> > +     ret = configure_cpufreq_info();
>
> Maybe just open code the function here.. It is just two function calls
> which are quite straight forward.
OK, thanks.

>
> > +     if (ret < 0)
> > +             return ret;
> > +
> > +     for (i = 0; i < MAX_PACKAGES; i++)
> > +             mutex_init(&cpufreq_mutex[i]);
>
> This must be initialized before calling configure_cpufreq_info() as
> you end up using them there.
OK, thanks.

Huacai
>
> > +     ret = cpufreq_register_driver(&loongson3_cpufreq_driver);
> > +     if (ret)
> > +             return ret;
> > +
> > +     pr_info("cpufreq: Loongson-3 CPU frequency driver.\n");
> > +
> > +     return 0;
> > +}
> > +
> > +static void loongson3_cpufreq_remove(struct platform_device *pdev)
> > +{
> > +     cpufreq_unregister_driver(&loongson3_cpufreq_driver);
> > +}
> > +
> > +static struct platform_device_id cpufreq_id_table[] = {
> > +     { "loongson3_cpufreq", },
> > +     { /* sentinel */ }
> > +};
> > +MODULE_DEVICE_TABLE(platform, cpufreq_id_table);
> > +
> > +static struct platform_driver loongson3_platform_driver = {
> > +     .driver = {
> > +             .name = "loongson3_cpufreq",
> > +     },
> > +     .id_table = cpufreq_id_table,
> > +     .probe = loongson3_cpufreq_probe,
> > +     .remove_new = loongson3_cpufreq_remove,
> > +};
> > +module_platform_driver(loongson3_platform_driver);
> > +
> > +MODULE_AUTHOR("Huacai Chen <chenhuacai@...ngson.cn>");
> > +MODULE_DESCRIPTION("CPUFreq driver for Loongson-3 processors");
> > +MODULE_LICENSE("GPL");
> > --
> > 2.43.0
>
> --
> viresh

Powered by blists - more mailing lists