| Message ID | 1526898690-4018-1-git-send-email-ilialin@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Would have been better if you would have updated the subject as: [PATCH v10 10/15] cpufreq: Add Kryo CPU scaling driver On 21-05-18, 13:31, Ilia Lin wrote: > In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors, > the CPU frequency subset and voltage value of each OPP varies > based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables > defines the voltage and frequency value based on the msm-id in SMEM > and speedbin blown in the efuse combination. > The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC > to provide the OPP framework with required information. > This is used to determine the voltage and frequency value for each OPP of > operating-points-v2 table when it is parsed by the OPP framework. > > Signed-off-by: Ilia Lin <ilialin@codeaurora.org> > Acked-by: Viresh Kumar <viresh.kumar@linaro.org> Yes, I acked it earlier, but then comments came back. You should drop the tags in such cases normally. Anyway, the patch looks fine now. Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
On Mon, May 21, 2018 at 01:31:30PM +0300, Ilia Lin wrote: > +#define SILVER_LEAD 0 > +#define GOLD_LEAD 2 Okay, two different values here, but "GOLD_LEAD" appears unused. > + cpu_dev_silver = get_cpu_device(SILVER_LEAD); > + if (NULL == cpu_dev_silver) > + return -ENODEV; > + > + cpu_dev_gold = get_cpu_device(SILVER_LEAD); > + if (NULL == cpu_dev_gold) > + return -ENODEV; get_cpu_device() takes the logical CPU number. So the above gets CPU 0 each time, and so cpu_dev_silver == cpu_dev_gold here. So what's the point of the second get_cpu_device() ? If it's supposed to be: cpu_dev_gold = get_cpu_device(GOLD_LEAD); That would get CPU 2, but in terms of these defines, it doesn't make that much sense. What exactly does "silver lead" and "gold lead" refer to in these definitions? > + opp_silver = dev_pm_opp_set_supported_hw(cpu_dev_silver,&versions,1); > + if (IS_ERR(opp_silver)) { > + dev_err(cpu_dev_silver, "Failed to set supported hardware\n"); > + ret = PTR_ERR(opp_silver); > + goto free_np; > + } > + > + opp_gold = dev_pm_opp_set_supported_hw(cpu_dev_gold,&versions,1); > + if (IS_ERR(opp_gold)) { > + dev_err(cpu_dev_gold, "Failed to set supported hardware\n"); > + ret = PTR_ERR(opp_gold); > + goto free_opp_silver; > + } Given that cpu_dev_silver == cpu_dev_gold, doesn't the second call to dev_pm_opp_set_supported_hw() always fail, as opp_table->supported_hw will be set by the first call? To me, this driver looks completely useless as it will always fail to initialise, and I question whether this code has even been runtime tested.
You are right. cpu_dev_silver != cpu_dev_gold, and I found this with my tests as well. Thank you. > -----Original Message----- > From: Russell King - ARM Linux <linux@armlinux.org.uk> > Sent: Monday, May 21, 2018 13:54 > To: Ilia Lin <ilialin@codeaurora.org> > Cc: viresh.kumar@linaro.org; devicetree@vger.kernel.org; linux- > pm@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-soc@vger.kernel.org; linux- > clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] cpufreq: Add Kryo CPU scaling driver > > On Mon, May 21, 2018 at 01:31:30PM +0300, Ilia Lin wrote: > > +#define SILVER_LEAD 0 > > +#define GOLD_LEAD 2 > > Okay, two different values here, but "GOLD_LEAD" appears unused. > > > + cpu_dev_silver = get_cpu_device(SILVER_LEAD); > > + if (NULL == cpu_dev_silver) > > + return -ENODEV; > > + > > + cpu_dev_gold = get_cpu_device(SILVER_LEAD); > > + if (NULL == cpu_dev_gold) > > + return -ENODEV; > > get_cpu_device() takes the logical CPU number. So the above gets CPU 0 > each time, and so cpu_dev_silver == cpu_dev_gold here. So what's the > point of the second get_cpu_device() ? If it's supposed to be: > > cpu_dev_gold = get_cpu_device(GOLD_LEAD); > > That would get CPU 2, but in terms of these defines, it doesn't make that > much sense. What exactly does "silver lead" and "gold lead" refer to in these > definitions? > > > + opp_silver = > dev_pm_opp_set_supported_hw(cpu_dev_silver,&versions,1); > > + if (IS_ERR(opp_silver)) { > > + dev_err(cpu_dev_silver, "Failed to set supported > hardware\n"); > > + ret = PTR_ERR(opp_silver); > > + goto free_np; > > + } > > + > > + opp_gold = > dev_pm_opp_set_supported_hw(cpu_dev_gold,&versions,1); > > + if (IS_ERR(opp_gold)) { > > + dev_err(cpu_dev_gold, "Failed to set supported > hardware\n"); > > + ret = PTR_ERR(opp_gold); > > + goto free_opp_silver; > > + } > > Given that cpu_dev_silver == cpu_dev_gold, doesn't the second call to > dev_pm_opp_set_supported_hw() always fail, as opp_table- > >supported_hw will be set by the first call? > > To me, this driver looks completely useless as it will always fail to initialise, > and I question whether this code has even been runtime tested. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps > up According to speedtest.net: 8.21Mbps down 510kbps up
On Mon, May 21, 2018 at 02:05:41PM +0300, ilialin@codeaurora.org wrote: > You are right. > cpu_dev_silver != cpu_dev_gold, and I found this with my tests as well. > Thank you. > > > -----Original Message----- > > From: Russell King - ARM Linux <linux@armlinux.org.uk> > > Sent: Monday, May 21, 2018 13:54 > > To: Ilia Lin <ilialin@codeaurora.org> > > Cc: viresh.kumar@linaro.org; devicetree@vger.kernel.org; linux- > > pm@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux- > > kernel@vger.kernel.org; linux-soc@vger.kernel.org; linux- > > clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org > > Subject: Re: [PATCH] cpufreq: Add Kryo CPU scaling driver > > > > On Mon, May 21, 2018 at 01:31:30PM +0300, Ilia Lin wrote: > > > +#define SILVER_LEAD 0 > > > +#define GOLD_LEAD 2 > > > > Okay, two different values here, but "GOLD_LEAD" appears unused. > > > > > + cpu_dev_silver = get_cpu_device(SILVER_LEAD); > > > + if (NULL == cpu_dev_silver) > > > + return -ENODEV; > > > + > > > + cpu_dev_gold = get_cpu_device(SILVER_LEAD); > > > + if (NULL == cpu_dev_gold) > > > + return -ENODEV; > > > > get_cpu_device() takes the logical CPU number. So the above gets CPU 0 > > each time, and so cpu_dev_silver == cpu_dev_gold here. So what's the > > point of the second get_cpu_device() ? If it's supposed to be: > > > > cpu_dev_gold = get_cpu_device(GOLD_LEAD); > > > > That would get CPU 2, but in terms of these defines, it doesn't make that > > much sense. What exactly does "silver lead" and "gold lead" refer to in > these > > definitions? I think you still need to explain this.
There are 2 CPU clusters in the Kryo, CPU 0 and 1 are called Silver Cluster and CPU 2 and 3 - Gold Cluster. Each cluster has single clock. The clusters differ in terms of speed capabilities, computing power and power consumption. Therefore, I define separate OPP table for each cluster, and my driver will choose the appropriate OPP subset for each cluster. Lead refers to first CPU in the cluster. > -----Original Message----- > From: Russell King - ARM Linux <linux@armlinux.org.uk> > Sent: Monday, May 21, 2018 15:12 > To: ilialin@codeaurora.org > Cc: viresh.kumar@linaro.org; devicetree@vger.kernel.org; linux- > pm@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux- > kernel@vger.kernel.org; linux-soc@vger.kernel.org; linux- > clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org > Subject: Re: [PATCH] cpufreq: Add Kryo CPU scaling driver > > On Mon, May 21, 2018 at 02:05:41PM +0300, ilialin@codeaurora.org wrote: > > You are right. > > cpu_dev_silver != cpu_dev_gold, and I found this with my tests as well. > > Thank you. > > > > > -----Original Message----- > > > From: Russell King - ARM Linux <linux@armlinux.org.uk> > > > Sent: Monday, May 21, 2018 13:54 > > > To: Ilia Lin <ilialin@codeaurora.org> > > > Cc: viresh.kumar@linaro.org; devicetree@vger.kernel.org; linux- > > > pm@vger.kernel.org; linux-arm-msm@vger.kernel.org; linux- > > > kernel@vger.kernel.org; linux-soc@vger.kernel.org; linux- > > > clk@vger.kernel.org; linux-arm-kernel@lists.infradead.org > > > Subject: Re: [PATCH] cpufreq: Add Kryo CPU scaling driver > > > > > > On Mon, May 21, 2018 at 01:31:30PM +0300, Ilia Lin wrote: > > > > +#define SILVER_LEAD 0 > > > > +#define GOLD_LEAD 2 > > > > > > Okay, two different values here, but "GOLD_LEAD" appears unused. > > > > > > > + cpu_dev_silver = get_cpu_device(SILVER_LEAD); > > > > + if (NULL == cpu_dev_silver) > > > > + return -ENODEV; > > > > + > > > > + cpu_dev_gold = get_cpu_device(SILVER_LEAD); > > > > + if (NULL == cpu_dev_gold) > > > > + return -ENODEV; > > > > > > get_cpu_device() takes the logical CPU number. So the above gets > > > CPU 0 each time, and so cpu_dev_silver == cpu_dev_gold here. So > > > what's the point of the second get_cpu_device() ? If it's supposed to be: > > > > > > cpu_dev_gold = get_cpu_device(GOLD_LEAD); > > > > > > That would get CPU 2, but in terms of these defines, it doesn't make > > > that much sense. What exactly does "silver lead" and "gold lead" > > > refer to in > > these > > > definitions? > > I think you still need to explain this. > > -- > RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ > FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps > up According to speedtest.net: 8.21Mbps down 510kbps up
On Mon, May 21, 2018 at 03:35:07PM +0300, ilialin@codeaurora.org wrote: > There are 2 CPU clusters in the Kryo, CPU 0 and 1 are called Silver Cluster > and CPU 2 and 3 - Gold Cluster. Each cluster has single clock. The clusters > differ in terms of speed capabilities, computing power and power > consumption. Therefore, I define separate OPP table for each cluster, and my > driver will choose the appropriate OPP subset for each cluster. > Lead refers to first CPU in the cluster. Ah, that is really confusing. Lead can means many things. Maybe a little more verbosity with the names such as SILVER_CLUSTER_LEAD_CPU would help?
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index de55c7d..0bfd40e 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -124,6 +124,16 @@ config ARM_OMAP2PLUS_CPUFREQ depends on ARCH_OMAP2PLUS default ARCH_OMAP2PLUS +config ARM_QCOM_CPUFREQ_KRYO + bool "Qualcomm Kryo based CPUFreq" + depends on QCOM_QFPROM + depends on QCOM_SMEM + select PM_OPP + help + This adds the CPUFreq driver for Qualcomm Kryo 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 8d24ade..fb4a2ec 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -65,6 +65,7 @@ obj-$(CONFIG_MACH_MVEBU_V7) += mvebu-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_KRYO) += qcom-cpufreq-kryo.o obj-$(CONFIG_ARM_S3C2410_CPUFREQ) += s3c2410-cpufreq.o obj-$(CONFIG_ARM_S3C2412_CPUFREQ) += s3c2412-cpufreq.o obj-$(CONFIG_ARM_S3C2416_CPUFREQ) += s3c2416-cpufreq.o diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c index 3b585e4..77d6ab8 100644 --- a/drivers/cpufreq/cpufreq-dt-platdev.c +++ b/drivers/cpufreq/cpufreq-dt-platdev.c @@ -118,6 +118,9 @@ { .compatible = "nvidia,tegra124", }, + { .compatible = "qcom,apq8096", }, + { .compatible = "qcom,msm8996", }, + { .compatible = "st,stih407", }, { .compatible = "st,stih410", }, diff --git a/drivers/cpufreq/qcom-cpufreq-kryo.c b/drivers/cpufreq/qcom-cpufreq-kryo.c new file mode 100644 index 0000000..4e002d0b --- /dev/null +++ b/drivers/cpufreq/qcom-cpufreq-kryo.c @@ -0,0 +1,164 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (c) 2018, The Linux Foundation. All rights reserved. + */ + +/* + * In Certain QCOM SoCs like apq8096 and msm8996 that have KRYO processors, + * the CPU frequency subset and voltage value of each OPP varies + * based on the silicon variant in use. Qualcomm Process Voltage Scaling Tables + * defines the voltage and frequency value based on the msm-id in SMEM + * and speedbin blown in the efuse combination. + * The qcom-cpufreq-kryo driver reads the msm-id and efuse value from the SoC + * to provide the OPP framework with required information. + * This is used to determine the voltage and frequency value for each OPP of + * operating-points-v2 table when it is parsed by the OPP framework. + */ + +#include <linux/cpu.h> +#include <linux/err.h> +#include <linux/init.h> +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/nvmem-consumer.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/slab.h> +#include <linux/soc/qcom/smem.h> + +#define MSM_ID_SMEM 137 +#define SILVER_LEAD 0 +#define GOLD_LEAD 2 + +enum _msm_id { + MSM8996V3 = 0xF6ul, + APQ8096V3 = 0x123ul, + MSM8996SG = 0x131ul, + APQ8096SG = 0x138ul, +}; + +enum _msm8996_version { + MSM8996_V3, + MSM8996_SG, + NUM_OF_MSM8996_VERSIONS, +}; + +static enum _msm8996_version __init qcom_cpufreq_kryo_get_msm_id(void) +{ + size_t len; + u32 *msm_id; + enum _msm8996_version version; + + msm_id = qcom_smem_get(QCOM_SMEM_HOST_ANY, MSM_ID_SMEM, &len); + /* The first 4 bytes are format, next to them is the actual msm-id */ + msm_id++; + + switch ((enum _msm_id)*msm_id) { + case MSM8996V3: + case APQ8096V3: + version = MSM8996_V3; + break; + case MSM8996SG: + case APQ8096SG: + version = MSM8996_SG; + break; + default: + version = NUM_OF_MSM8996_VERSIONS; + } + + return version; +} + +static int __init qcom_cpufreq_kryo_driver_init(void) +{ + struct device *cpu_dev_silver, *cpu_dev_gold; + struct opp_table *opp_silver, *opp_gold; + enum _msm8996_version msm8996_version; + struct nvmem_cell *speedbin_nvmem; + struct platform_device *pdev; + struct device_node *np; + u8 *speedbin; + u32 versions; + size_t len; + int ret; + + cpu_dev_silver = get_cpu_device(SILVER_LEAD); + if (NULL == cpu_dev_silver) + return -ENODEV; + + cpu_dev_gold = get_cpu_device(SILVER_LEAD); + if (NULL == cpu_dev_gold) + return -ENODEV; + + msm8996_version = qcom_cpufreq_kryo_get_msm_id(); + if (NUM_OF_MSM8996_VERSIONS == msm8996_version) { + dev_err(cpu_dev_silver, "Not Snapdragon 820/821!"); + return -ENODEV; + } + + np = dev_pm_opp_of_get_opp_desc_node(cpu_dev_silver); + if (IS_ERR(np)) + return PTR_ERR(np); + + if (!of_device_is_compatible(np, "operating-points-v2-kryo-cpu")) { + ret = -ENOENT; + goto free_np; + } + + speedbin_nvmem = of_nvmem_cell_get(np, NULL); + if (IS_ERR(speedbin_nvmem)) { + ret = PTR_ERR(speedbin_nvmem); + dev_err(cpu_dev_silver, "Could not get nvmem cell: %d\n", ret); + goto free_np; + } + + speedbin = nvmem_cell_read(speedbin_nvmem, &len); + nvmem_cell_put(speedbin_nvmem); + + switch (msm8996_version) { + case MSM8996_V3: + versions = 1 << (unsigned int)(*speedbin); + break; + case MSM8996_SG: + versions = 1 << ((unsigned int)(*speedbin) + 4); + break; + default: + BUG(); + break; + } + + opp_silver = dev_pm_opp_set_supported_hw(cpu_dev_silver,&versions,1); + if (IS_ERR(opp_silver)) { + dev_err(cpu_dev_silver, "Failed to set supported hardware\n"); + ret = PTR_ERR(opp_silver); + goto free_np; + } + + opp_gold = dev_pm_opp_set_supported_hw(cpu_dev_gold,&versions,1); + if (IS_ERR(opp_gold)) { + dev_err(cpu_dev_gold, "Failed to set supported hardware\n"); + ret = PTR_ERR(opp_gold); + goto free_opp_silver; + } + + pdev = platform_device_register_simple("cpufreq-dt", -1, NULL, 0); + if (!IS_ERR(pdev)) + return 0; + + ret = PTR_ERR(pdev); + dev_err(cpu_dev_silver, "Failed to register platform device\n"); + dev_pm_opp_put_supported_hw(opp_gold); + +free_opp_silver: + dev_pm_opp_put_supported_hw(opp_silver); + +free_np: + of_node_put(np); + + return ret; +} +late_initcall(qcom_cpufreq_kryo_driver_init); + +MODULE_DESCRIPTION("Qualcomm Technologies, Inc. Kryo CPUfreq driver"); +MODULE_LICENSE("GPL v2");