Re: [PATCH v5 14/15] cpufreq: Add module to register cpufreq on Krait CPUs

Viresh Kumar
2017-12-20T03:36:17Z

On 19-12-17, 21:24, Sricharan R wrote:
> From: Stephen Boyd <sboyd@codeaurora.org>
>
> Register a cpufreq-generic device whenever we detect that a
> "qcom,krait" compatible CPU is present in DT.
>
> Cc: <devicetree@vger.kernel.org>
> [Sricharan: updated to use dev_pm_opp_set_prop_name]
> Signed-off-by: Sricharan R <sricharan@codeaurora.org>
> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org>
> ---
> drivers/cpufreq/Kconfig.arm | 9 ++
> drivers/cpufreq/Makefile | 1 +
> drivers/cpufreq/cpufreq-dt-platdev.c | 3 +-
> drivers/cpufreq/qcom-cpufreq.c | 171 +++++++++++++++++++++++++++++++++++
> 4 files changed, 183 insertions(+), 1 deletion(-)
> create mode 100644 drivers/cpufreq/qcom-cpufreq.c
>
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index bdce448..60f28e7 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -100,6 +100,15 @@ config ARM_OMAP2PLUS_CPUFREQ
> depends on ARCH_OMAP2PLUS
> default ARCH_OMAP2PLUS
>
> +config ARM_QCOM_CPUFREQ
> + tristate "Qualcomm based"

Qualcomm based ... ? You want to add something after this ?

And why tristate ? Do you really want to build a module for this ?

> + depends on ARCH_QCOM
> + select PM_OPP
> + help
> + This adds the CPUFreq driver for Qualcomm SoC based boards.
> +
> + If in doubt, say N.
> +
> config ARM_S3C_CPUFREQ
> bool
> help
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 812f9e0..1496464 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -62,6 +62,7 @@ obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ) += mediatek-cpufreq.o
> obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o
> obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o
> obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o
> +obj-$(CONFIG_ARM_QCOM_CPUFREQ) += qcom-cpufreq.o
> obj-$(CONFIG_ARM_S3C24XX_CPUFREQ) += s3c24xx-cpufreq.o
> obj-$(CONFIG_ARM_S3C24XX_CPUFREQ_DEBUGFS) += s3c24xx-cpufreq-debugfs.o
> obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index ecc56e2..032ac4f 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -118,7 +118,7 @@
> { .compatible = "ti,am33xx", },
> { .compatible = "ti,am43", },
> { .compatible = "ti,dra7", },
> -

Keep this blank line as is..

> + { .compatible = "qcom,ipq8064", },

And add another one here.

> { }
> };
>
> @@ -157,6 +157,7 @@ static int __init cpufreq_dt_platdev_init(void)
>
> create_pdev:
> of_node_put(np);
> +

Remove this.

> return PTR_ERR_OR_ZERO(platform_device_register_data(NULL, "cpufreq-dt",
> -1, data,
> sizeof(struct cpufreq_dt_platform_data)));
> diff --git a/drivers/cpufreq/qcom-cpufreq.c b/drivers/cpufreq/qcom-cpufreq.c
> new file mode 100644
> index 0000000..3e5583d
> --- /dev/null
> +++ b/drivers/cpufreq/qcom-cpufreq.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (c) 2013-2015, The Linux Foundation. All rights reserved.
> +
> +#include <linux/cpu.h>
> +#include <linux/err.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +#include "cpufreq-dt.h"
> +
> +static void __init get_krait_bin_format_a(int *speed, int *pvs, int *pvs_ver)
> +{
> + void __iomem *base;
> + u32 pte_efuse;
> +
> + *speed = *pvs = *pvs_ver = 0;
> +
> + base = ioremap(0x007000c0, 4);
> + if (!base) {
> + pr_warn("Unable to read efuse data. Defaulting to 0!\n");
> + return;
> + }
> +
> + pte_efuse = readl_relaxed(base);
> + iounmap(base);
> +
> + *speed = pte_efuse & 0xf;
> + if (*speed == 0xf)
> + *speed = (pte_efuse >> 4) & 0xf;
> +
> + if (*speed == 0xf) {
> + *speed = 0;
> + pr_warn("Speed bin: Defaulting to %d\n", *speed);
> + } else {
> + pr_info("Speed bin: %d\n", *speed);
> + }
> +
> + *pvs = (pte_efuse >> 10) & 0x7;
> + if (*pvs == 0x7)
> + *pvs = (pte_efuse >> 13) & 0x7;
> +
> + if (*pvs == 0x7) {
> + *pvs = 0;
> + pr_warn("PVS bin: Defaulting to %d\n", *pvs);
> + } else {
> + pr_info("PVS bin: %d\n", *pvs);
> + }
> +}
> +
> +static void __init get_krait_bin_format_b(int *speed, int *pvs, int *pvs_ver)
> +{
> + u32 pte_efuse, redundant_sel;
> + void __iomem *base;
> +
> + *speed = 0;
> + *pvs = 0;
> + *pvs_ver = 0;
> +
> + base = ioremap(0xfc4b80b0, 8);
> + if (!base) {
> + pr_warn("Unable to read efuse data. Defaulting to 0!\n");
> + return;
> + }
> +
> + pte_efuse = readl_relaxed(base);
> + redundant_sel = (pte_efuse >> 24) & 0x7;
> + *speed = pte_efuse & 0x7;
> + /* 4 bits of PVS are in efuse register bits 31, 8-6. */
> + *pvs = ((pte_efuse >> 28) & 0x8) | ((pte_efuse >> 6) & 0x7);
> + *pvs_ver = (pte_efuse >> 4) & 0x3;
> +
> + switch (redundant_sel) {
> + case 1:
> + *speed = (pte_efuse >> 27) & 0xf;
> + break;
> + case 2:
> + *pvs = (pte_efuse >> 27) & 0xf;
> + break;
> + }
> +
> + /* Check SPEED_BIN_BLOW_STATUS */
> + if (pte_efuse & BIT(3)) {
> + pr_info("Speed bin: %d\n", *speed);
> + } else {
> + pr_warn("Speed bin not set. Defaulting to 0!\n");
> + *speed = 0;
> + }
> +
> + /* Check PVS_BLOW_STATUS */
> + pte_efuse = readl_relaxed(base + 0x4) & BIT(21);
> + if (pte_efuse) {
> + pr_info("PVS bin: %d\n", *pvs);
> + } else {
> + pr_warn("PVS bin not set. Defaulting to 0!\n");
> + *pvs = 0;
> + }
> +
> + pr_info("PVS version: %d\n", *pvs_ver);
> + iounmap(base);
> +}
> +
> +static int __init qcom_cpufreq_populate_opps(void)
> +{
> + int speed, pvs, pvs_ver;
> + struct device_node *np;
> + struct device *dev;
> + int cpu = 0;
> + char pvs_name[] = "speedXX-pvsXX-vXX";
> +
> + np = of_find_node_by_name(NULL, "qcom,pvs");
> + if (!np)
> + return -ENODEV;
> +
> + if (of_property_read_bool(np, "qcom,pvs-format-a"))
> + get_krait_bin_format_a(&speed, &pvs, &pvs_ver);
> + else if (of_property_read_bool(np, "qcom,pvs-format-b"))
> + get_krait_bin_format_b(&speed, &pvs, &pvs_ver);
> + else
> + return -ENODEV;
> +
> + snprintf(pvs_name, sizeof(pvs_name), "speed%d-pvs%d-v%d",
> + speed, pvs, pvs_ver);
> +
> + for (cpu = 0; cpu < num_possible_cpus(); cpu++) {
> + dev = get_cpu_device(cpu);
> + if (!dev)
> + return -ENODEV;
> +
> + if (IS_ERR(dev_pm_opp_set_prop_name(dev, pvs_name)))
> + pr_warn("failed to add OPP name %s\n", pvs_name);
> + }
> +
> + return 0;
> +}
> +
> +static int __init qcom_cpufreq_driver_init(void)
> +{
> + struct device *cpu_dev;
> + struct device_node *np;
> + int ret;
> +
> + cpu_dev = get_cpu_device(0);
> + if (!cpu_dev)
> + return -ENODEV;
> +
> + np = of_node_get(cpu_dev->of_node);
> + if (!np)
> + return -ENOENT;
> +
> + if (!of_device_is_compatible(np, "qcom,krait")) {
> + of_node_put(np);
> + return -ENODEV;
> + }
> + of_node_put(np);
> +
> + ret = qcom_cpufreq_populate_opps();
> + if (ret)
> + return ret;
> +
> + return PTR_ERR(platform_device_register_simple("cpufreq-dt",
> + -1, NULL, 0));
> +}
> +module_init(qcom_cpufreq_driver_init);

As there is no module exit, it doesn't make sense to keep this as
tristate.

Looks mostly fine now other than these minor nits.

--
viresh