* [PATCH v5 0/2] Add Mediatek MT8173 cpufreq driver @ 2015-07-01 2:16 Pi-Cheng Chen 2015-07-01 2:16 ` [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding Pi-Cheng Chen 2015-07-01 2:16 ` [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen 0 siblings, 2 replies; 10+ messages in thread From: Pi-Cheng Chen @ 2015-07-01 2:16 UTC (permalink / raw) To: linux-arm-kernel MT8173 is a ARMv8 based SoC with 2 clusters. All CPUs in a single cluster share the same power and clock domain. This series tries to add cpufreq support for MT8173 SoC. changes in v5: - Move resource allocation code from init() into probe() and remove some unused functions due to this change - Fix descriptions for device tree binding document - Address review comments for last version - Register CPU cooling device Changes in v4: - Add bindings for MT8173 cpufreq driver - Move OPP table back into device tree - Address comments for last version Changes in v3: - Implement MT8173 specific standalone cpufreq driver instead of using cpufreq-dt driver - Define OPP table in the driver source code until new OPP binding is ready Changes in v2: - Add intermediate frequency support in cpufreq-dt driver - Use voltage scaling code of cpufreq-dt for little cluster instead of implementaion in notifier of mtk-cpufreq driver - Code refinement for mtk-cpufreq driver Pi-Cheng Chen (2): dt-bindings: mediatek: Add MT8173 cpufreq driver binding cpufreq: mediatek: Add MT8173 cpufreq driver .../devicetree/bindings/cpufreq/cpufreq-mt8173.txt | 145 ++++++ drivers/cpufreq/Kconfig.arm | 7 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/mt8173-cpufreq.c | 520 +++++++++++++++++++++ 4 files changed, 673 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt create mode 100644 drivers/cpufreq/mt8173-cpufreq.c -- 1.9.1 ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding 2015-07-01 2:16 [PATCH v5 0/2] Add Mediatek MT8173 cpufreq driver Pi-Cheng Chen @ 2015-07-01 2:16 ` Pi-Cheng Chen 2015-07-08 11:19 ` Viresh Kumar 2015-07-01 2:16 ` [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen 1 sibling, 1 reply; 10+ messages in thread From: Pi-Cheng Chen @ 2015-07-01 2:16 UTC (permalink / raw) To: linux-arm-kernel This patch adds device tree binding document for MT8173 cpufreq driver. Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org> Reviewed-by: Michael Turquette <mturquette@baylibre.com> --- .../devicetree/bindings/cpufreq/cpufreq-mt8173.txt | 145 +++++++++++++++++++++ 1 file changed, 145 insertions(+) create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt new file mode 100644 index 0000000..65701c5 --- /dev/null +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt @@ -0,0 +1,145 @@ + +Mediatek MT8173 cpufreq driver +------------------------------ + +Mediatek MT8173 cpufreq driver for CPU frequency scaling. + +Required properties: +- clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock names. +- clock-names: Should contain the following: + "cpu" - The multiplexer for clock input of CPU cluster. + "intermediate" - A parent of "cpu" clock which is used as "intermediate" clock + source (usually MAINPLL) when the original CPU PLL is under + transition and not stable yet. + Please refer to Documentation/devicetree/bindings/clk/clock-bindings.txt for + generic clock consumer properties. +- operating-points: Please refer to Documentation/devicetree/bindings/power/opp.txt for + details. +- proc-supply: Regulator for Vproc of CPU cluster. + +Optional properties: +- sram-supply: Regulator for Vsram of CPU cluster. When present, the cpufreq driver + needs to do "voltage trace" to step by step scale up/down Vproc and + Vsram to fit SoC specific needs. When absent, the voltage scaling + flow is handled by hardware, hence no software "voltage trace" is + needed. +- #cooling-cells: +- cooling-min-level: +- cooling-max-level: + Please refer to Documentation/devicetree/bindings/thermal/thermal.txt. + +Example: +-------- + cpu0: cpu at 0 { + device_type = "cpu"; + compatible = "arm,cortex-a53"; + reg = <0x000>; + enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&infracfg CLK_INFRA_CA53SEL>, + <&apmixedsys CLK_APMIXED_MAINPLL>; + clock-names = "cpu", "intermediate"; + operating-points = < + 507000 859000 + 702000 908000 + 1001000 983000 + 1105000 1009000 + 1183000 1028000 + 1404000 1083000 + 1508000 1109000 + 1573000 1125000 + >; + #cooling-cells = <2>; + cooling-min-level = <0>; + cooling-max-level = <7>; + }; + + cpu1: cpu at 1 { + device_type = "cpu"; + compatible = "arm,cortex-a53"; + reg = <0x001>; + enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&infracfg CLK_INFRA_CA53SEL>, + <&apmixedsys CLK_APMIXED_MAINPLL>; + clock-names = "cpu", "intermediate"; + operating-points = < + 507000 859000 + 702000 908000 + 1001000 983000 + 1105000 1009000 + 1183000 1028000 + 1404000 1083000 + 1508000 1109000 + 1573000 1125000 + >; + #cooling-cells = <2>; + cooling-min-level = <0>; + cooling-max-level = <7>; + }; + + cpu2: cpu at 100 { + device_type = "cpu"; + compatible = "arm,cortex-a57"; + reg = <0x100>; + enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&infracfg CLK_INFRA_CA57SEL>, + <&apmixedsys CLK_APMIXED_MAINPLL>; + clock-names = "cpu", "intermediate"; + operating-points = < + 507000 828000 + 702000 867000 + 1001000 927000 + 1209000 968000 + 1404000 1007000 + 1612000 1049000 + 1807000 1089000 + 1989000 1125000 + >; + #cooling-cells = <2>; + cooling-min-level = <0>; + cooling-max-level = <7>; + }; + + cpu3: cpu at 101 { + device_type = "cpu"; + compatible = "arm,cortex-a57"; + reg = <0x101>; + enable-method = "psci"; + cpu-idle-states = <&CPU_SLEEP_0>; + clocks = <&infracfg CLK_INFRA_CA57SEL>, + <&apmixedsys CLK_APMIXED_MAINPLL>; + clock-names = "cpu", "intermediate"; + operating-points = < + 507000 828000 + 702000 867000 + 1001000 927000 + 1209000 968000 + 1404000 1007000 + 1612000 1049000 + 1807000 1089000 + 1989000 1125000 + >; + #cooling-cells = <2>; + cooling-min-level = <0>; + cooling-max-level = <7>; + }; + + &cpu0 { + proc-supply = <&mt6397_vpca15_reg>; + }; + + &cpu1 { + proc-supply = <&mt6397_vpca15_reg>; + }; + + &cpu2 { + proc-supply = <&da9211_vcpu_reg>; + sram-supply = <&mt6397_vsramca7_reg>; + }; + + &cpu3 { + proc-supply = <&da9211_vcpu_reg>; + sram-supply = <&mt6397_vsramca7_reg>; + }; -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding 2015-07-01 2:16 ` [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding Pi-Cheng Chen @ 2015-07-08 11:19 ` Viresh Kumar 2015-07-09 15:04 ` Michael Turquette 0 siblings, 1 reply; 10+ messages in thread From: Viresh Kumar @ 2015-07-08 11:19 UTC (permalink / raw) To: linux-arm-kernel On 01-07-15, 10:16, Pi-Cheng Chen wrote: > This patch adds device tree binding document for MT8173 cpufreq driver. > > Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org> > Reviewed-by: Michael Turquette <mturquette@baylibre.com> > --- > .../devicetree/bindings/cpufreq/cpufreq-mt8173.txt | 145 +++++++++++++++++++++ > 1 file changed, 145 insertions(+) > create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt > > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt > new file mode 100644 > index 0000000..65701c5 > --- /dev/null > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt > @@ -0,0 +1,145 @@ > + > +Mediatek MT8173 cpufreq driver > +------------------------------ > + > +Mediatek MT8173 cpufreq driver for CPU frequency scaling. > + > +Required properties: > +- clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock names. > +- clock-names: Should contain the following: > + "cpu" - The multiplexer for clock input of CPU cluster. > + "intermediate" - A parent of "cpu" clock which is used as "intermediate" clock > + source (usually MAINPLL) when the original CPU PLL is under > + transition and not stable yet. > + Please refer to Documentation/devicetree/bindings/clk/clock-bindings.txt for > + generic clock consumer properties. Don't have any intentions to halt this series anymore, I have irritated you enough already :) But, what about moving these bindings in something like a clock driver? @Mike: ? I am asking because these really belong to the clock driver, as I understood it from Mike. And clearly asked me to not take care of such things in cpufreq core/drivers. Another reason is that, later you will kill this driver one day and use cpufreq-dt. And then you will be required to move these bindings to a clock driver, as these will stay. -- viresh ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding 2015-07-08 11:19 ` Viresh Kumar @ 2015-07-09 15:04 ` Michael Turquette 0 siblings, 0 replies; 10+ messages in thread From: Michael Turquette @ 2015-07-09 15:04 UTC (permalink / raw) To: linux-arm-kernel Quoting Viresh Kumar (2015-07-08 04:19:00) > On 01-07-15, 10:16, Pi-Cheng Chen wrote: > > This patch adds device tree binding document for MT8173 cpufreq driver. > > > > Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org> > > Reviewed-by: Michael Turquette <mturquette@baylibre.com> > > --- > > .../devicetree/bindings/cpufreq/cpufreq-mt8173.txt | 145 +++++++++++++++++++++ > > 1 file changed, 145 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt > > > > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt > > new file mode 100644 > > index 0000000..65701c5 > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mt8173.txt > > @@ -0,0 +1,145 @@ > > + > > +Mediatek MT8173 cpufreq driver > > +------------------------------ > > + > > +Mediatek MT8173 cpufreq driver for CPU frequency scaling. > > + > > +Required properties: > > +- clocks: A list of phandle + clock-specifier pairs for the clocks listed in clock names. > > +- clock-names: Should contain the following: > > + "cpu" - The multiplexer for clock input of CPU cluster. > > + "intermediate" - A parent of "cpu" clock which is used as "intermediate" clock > > + source (usually MAINPLL) when the original CPU PLL is under > > + transition and not stable yet. > > + Please refer to Documentation/devicetree/bindings/clk/clock-bindings.txt for > > + generic clock consumer properties. > > Don't have any intentions to halt this series anymore, I have > irritated you enough already :) > > But, what about moving these bindings in something like a clock > driver? > > @Mike: ? Viresh, Pi-Cheng is using the consumer portion of the clock binding, and he is using it correctly. You can see this type of thing sprinkled all over. For instance, many I/O controller do this exact same thing. > > I am asking because these really belong to the clock driver, as I > understood it from Mike. And clearly asked me to not take care of such > things in cpufreq core/drivers. The clock driver is the "provider" and it is separate. This binding is the "consumer". > > Another reason is that, later you will kill this driver one day and > use cpufreq-dt. And then you will be required to move these bindings > to a clock driver, as these will stay. I'm not sure I follow. Again, the use of the consumer side of the clock binding is absolutely correct. Take a quick look at clock-bindings.txt and search for the section titled, "==Clock consumers==" for more info. Regards, Mike > > -- > viresh > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver 2015-07-01 2:16 [PATCH v5 0/2] Add Mediatek MT8173 cpufreq driver Pi-Cheng Chen 2015-07-01 2:16 ` [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding Pi-Cheng Chen @ 2015-07-01 2:16 ` Pi-Cheng Chen 2015-07-01 21:24 ` Alexey Klimov 2015-07-08 11:34 ` Viresh Kumar 1 sibling, 2 replies; 10+ messages in thread From: Pi-Cheng Chen @ 2015-07-01 2:16 UTC (permalink / raw) To: linux-arm-kernel This patch implements MT8173 cpufreq driver. Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org> --- drivers/cpufreq/Kconfig.arm | 7 + drivers/cpufreq/Makefile | 1 + drivers/cpufreq/mt8173-cpufreq.c | 520 +++++++++++++++++++++++++++++++++++++++ 3 files changed, 528 insertions(+) create mode 100644 drivers/cpufreq/mt8173-cpufreq.c diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm index 611cb09..2a305c0 100644 --- a/drivers/cpufreq/Kconfig.arm +++ b/drivers/cpufreq/Kconfig.arm @@ -141,6 +141,13 @@ config ARM_KIRKWOOD_CPUFREQ This adds the CPUFreq driver for Marvell Kirkwood SoCs. +config ARM_MT8173_CPUFREQ + bool "Mediatek MT8173 CPUFreq support" + depends on ARCH_MEDIATEK && REGULATOR + select PM_OPP + help + This adds the CPUFreq driver support for Mediatek MT8173 SoC. + config ARM_OMAP2PLUS_CPUFREQ bool "TI OMAP2+" depends on ARCH_OMAP2PLUS diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile index cdce92a..97f9a9b 100644 --- a/drivers/cpufreq/Makefile +++ b/drivers/cpufreq/Makefile @@ -63,6 +63,7 @@ obj-$(CONFIG_ARM_HISI_ACPU_CPUFREQ) += hisi-acpu-cpufreq.o obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) += imx6q-cpufreq.o obj-$(CONFIG_ARM_INTEGRATOR) += integrator-cpufreq.o obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c new file mode 100644 index 0000000..6284e67 --- /dev/null +++ b/drivers/cpufreq/mt8173-cpufreq.c @@ -0,0 +1,520 @@ +/* + * Copyright (c) 2015 Linaro Ltd. + * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org> + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License version 2 as + * published by the Free Software Foundation. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + */ + +#include <linux/clk.h> +#include <linux/cpu.h> +#include <linux/cpu_cooling.h> +#include <linux/cpufreq.h> +#include <linux/cpumask.h> +#include <linux/of.h> +#include <linux/platform_device.h> +#include <linux/pm_opp.h> +#include <linux/regulator/consumer.h> +#include <linux/slab.h> +#include <linux/thermal.h> + +#define MIN_VOLT_SHIFT (100000) +#define MAX_VOLT_SHIFT (200000) +#define MAX_VOLT_LIMIT (1150000) +#define VOLT_TOL (10000) + +/* + * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS + * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in + * Mediatek SoCs has two voltage inputs, Vproc and Vsram. In some cases the two + * voltage inputs need to be controlled under a hardware limitation: + * 100mV < Vsram - Vproc < 200mV + * + * When scaling the clock frequency of a CPU clock domain, the clock source + * needs to be switched to another stable PLL clock temporarily until + * the original PLL becomes stable@target frequency. + */ +struct mtk_cpu_dvfs_info { + struct device *cpu_dev; + struct regulator *proc_reg; + struct regulator *sram_reg; + struct clk *cpu_clk; + struct clk *inter_clk; + struct thermal_cooling_device *cdev; + int intermediate_voltage; + bool need_voltage_trace; +}; + +static int mtk_cpufreq_voltage_trace(struct mtk_cpu_dvfs_info *info, + int new_vproc) +{ + struct regulator *proc_reg = info->proc_reg; + struct regulator *sram_reg = info->sram_reg; + int old_vproc, old_vsram, new_vsram, vsram, vproc, ret; + + old_vproc = regulator_get_voltage(proc_reg); + old_vsram = regulator_get_voltage(sram_reg); + /* Vsram should not exceed the maximum allowed voltage of SoC. */ + new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT); + + if (old_vproc < new_vproc) { + /* + * When scaling up voltages, Vsram and Vproc scale up step + * by step. At each step, set Vsram to (Vproc + 200mV) first, + * then set Vproc to (Vsram - 100mV). + * Keep doing it until Vsram and Vproc hit target voltages. + */ + do { + old_vsram = regulator_get_voltage(sram_reg); + old_vproc = regulator_get_voltage(proc_reg); + + vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT); + + if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) { + vsram = MAX_VOLT_LIMIT; + + /* + * If the target Vsram hits the maximum voltage, + * try to set the exact voltage value first. + */ + ret = regulator_set_voltage(sram_reg, vsram, + vsram); + if (ret) + ret = regulator_set_voltage(sram_reg, + vsram - VOLT_TOL, + vsram); + + vproc = new_vproc; + } else { + ret = regulator_set_voltage(sram_reg, vsram, + vsram + VOLT_TOL); + + vproc = vsram - MIN_VOLT_SHIFT; + } + if (ret) + return ret; + + ret = regulator_set_voltage(proc_reg, vproc, + vproc + VOLT_TOL); + if (ret) { + regulator_set_voltage(sram_reg, old_vsram, + old_vsram); + return ret; + } + } while (vproc < new_vproc || vsram < new_vsram); + } else if (old_vproc > new_vproc) { + /* + * When scaling down voltages, Vsram and Vproc scale down step + * by step. At each step, set Vproc to (Vsram - 200mV) first, + * then set Vproc to (Vproc + 100mV). + * Keep doing it until Vsram and Vproc hit target voltages. + */ + do { + old_vproc = regulator_get_voltage(proc_reg); + old_vsram = regulator_get_voltage(sram_reg); + + vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT); + ret = regulator_set_voltage(proc_reg, vproc, + vproc + VOLT_TOL); + if (ret) + return ret; + + if (vproc == new_vproc) + vsram = new_vsram; + else + vsram = max(new_vsram, vproc + MIN_VOLT_SHIFT); + + if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) { + vsram = MAX_VOLT_LIMIT; + + /* + * If the target Vsram hits the maximum voltage, + * try to set the exact voltage value first. + */ + ret = regulator_set_voltage(sram_reg, vsram, + vsram); + if (ret) + ret = regulator_set_voltage(sram_reg, + vsram - VOLT_TOL, + vsram); + } else { + ret = regulator_set_voltage(sram_reg, vsram, + vsram + VOLT_TOL); + } + + if (ret) { + regulator_set_voltage(proc_reg, old_vproc, + old_vproc); + return ret; + } + } while (vproc > new_vproc + VOLT_TOL || + vsram > new_vsram + VOLT_TOL); + } + + return 0; +} + +static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) +{ + if (info->need_voltage_trace) + return mtk_cpufreq_voltage_trace(info, vproc); + else + return regulator_set_voltage(info->proc_reg, vproc, + vproc + VOLT_TOL); +} + +static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, + unsigned int index) +{ + struct cpufreq_frequency_table *freq_table = policy->freq_table; + struct clk *cpu_clk = policy->clk; + struct clk *armpll = clk_get_parent(cpu_clk); + struct mtk_cpu_dvfs_info *info = policy->driver_data; + struct device *cpu_dev = info->cpu_dev; + struct dev_pm_opp *opp; + long freq_hz, old_freq_hz; + int vproc, old_vproc, inter_vproc, target_vproc, ret; + + inter_vproc = info->intermediate_voltage; + + old_freq_hz = clk_get_rate(cpu_clk); + old_vproc = regulator_get_voltage(info->proc_reg); + + freq_hz = freq_table[index].frequency * 1000; + + rcu_read_lock(); + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); + if (IS_ERR(opp)) { + rcu_read_unlock(); + pr_err("cpu%d: failed to find OPP for %ld\n", + policy->cpu, freq_hz); + return PTR_ERR(opp); + } + vproc = dev_pm_opp_get_voltage(opp); + rcu_read_unlock(); + + /* + * If the new voltage or the intermediate voltage is higher than the + * current voltage, scale up voltage first. + */ + target_vproc = (inter_vproc > vproc) ? inter_vproc : vproc; + if (old_vproc < target_vproc) { + ret = mtk_cpufreq_set_voltage(info, target_vproc); + if (ret) { + pr_err("cpu%d: failed to scale up voltage!\n", + policy->cpu); + mtk_cpufreq_set_voltage(info, old_vproc); + return ret; + } + } + + /* Reparent the CPU clock to intermediate clock. */ + ret = clk_set_parent(cpu_clk, info->inter_clk); + if (ret) { + pr_err("cpu%d: failed to re-parent cpu clock!\n", + policy->cpu); + mtk_cpufreq_set_voltage(info, old_vproc); + WARN_ON(1); + return ret; + } + + /* Set the original PLL to target rate. */ + ret = clk_set_rate(armpll, freq_hz); + if (ret) { + pr_err("cpu%d: failed to scale cpu clock rate!\n", + policy->cpu); + clk_set_parent(cpu_clk, armpll); + mtk_cpufreq_set_voltage(info, old_vproc); + return ret; + } + + /* Set parent of CPU clock back to the original PLL. */ + ret = clk_set_parent(cpu_clk, armpll); + if (ret) { + pr_err("cpu%d: failed to re-parent cpu clock!\n", + policy->cpu); + mtk_cpufreq_set_voltage(info, inter_vproc); + WARN_ON(1); + return ret; + } + + /* + * If the new voltage is lower than the intermediate voltage or the + * original voltage, scale down to the new voltage. + */ + if (vproc < inter_vproc || vproc < old_vproc) { + ret = mtk_cpufreq_set_voltage(info, vproc); + if (ret) { + pr_err("cpu%d: failed to scale down voltage!\n", + policy->cpu); + clk_set_parent(cpu_clk, info->inter_clk); + clk_set_rate(armpll, old_freq_hz); + clk_set_parent(cpu_clk, armpll); + return ret; + } + } + + return 0; +} + +static void mtk_cpufreq_ready(struct cpufreq_policy *policy) +{ + struct mtk_cpu_dvfs_info *info = policy->driver_data; + struct device_node *np = of_node_get(info->cpu_dev->of_node); + + if (WARN_ON(!np)) + return; + + if (of_find_property(np, "#cooling-cells", NULL)) { + info->cdev = of_cpufreq_cooling_register(np, + policy->related_cpus); + + if (IS_ERR(info->cdev)) { + dev_err(info->cpu_dev, + "running cpufreq without cooling device: %ld\n", + PTR_ERR(info->cdev)); + + info->cdev = NULL; + } + } + + of_node_put(np); +} + +static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) +{ + struct device *cpu_dev; + struct regulator *proc_reg = ERR_PTR(-ENODEV); + struct regulator *sram_reg = ERR_PTR(-ENODEV); + struct clk *cpu_clk = ERR_PTR(-ENODEV); + struct clk *inter_clk = ERR_PTR(-ENODEV); + struct dev_pm_opp *opp; + unsigned long rate; + int ret; + + cpu_dev = get_cpu_device(cpu); + if (!cpu_dev) { + pr_err("failed to get cpu%d device\n", cpu); + return -ENODEV; + } + + ret = of_init_opp_table(cpu_dev); + if (ret) { + pr_warn("no OPP table for cpu%d\n", cpu); + return ret; + } + + cpu_clk = clk_get(cpu_dev, "cpu"); + if (IS_ERR(cpu_clk)) { + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER) + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu); + else + pr_err("failed to get cpu clk for cpu%d\n", cpu); + + ret = PTR_ERR(cpu_clk); + goto out_free_opp_table; + } + + inter_clk = clk_get(cpu_dev, "intermediate"); + if (IS_ERR(inter_clk)) { + if (PTR_ERR(inter_clk) == -EPROBE_DEFER) + pr_warn("intermediate clk for cpu%d not ready, retry.\n", + cpu); + else + pr_err("failed to get intermediate clk for cpu%d\n", + cpu); + + ret = PTR_ERR(inter_clk); + goto out_free_resources; + } + + proc_reg = regulator_get_exclusive(cpu_dev, "proc"); + if (IS_ERR(proc_reg)) { + if (PTR_ERR(proc_reg) == -EPROBE_DEFER) + pr_warn("proc regulator for cpu%d not ready, retry.\n", + cpu); + else + pr_err("failed to get proc regulator for cpu%d\n", + cpu); + + ret = PTR_ERR(proc_reg); + goto out_free_resources; + } + + /* Both presence and absence of sram regulator are valid cases. */ + sram_reg = regulator_get_exclusive(cpu_dev, "sram"); + + /* Search a safe voltage for intermediate frequency. */ + rate = clk_get_rate(inter_clk); + rcu_read_lock(); + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); + if (IS_ERR(opp)) { + pr_err("failed to get intermediate opp for cpu%d\n", cpu); + ret = PTR_ERR(opp); + goto out_free_resources; + } + info->intermediate_voltage = dev_pm_opp_get_voltage(opp); + rcu_read_unlock(); + + info->cpu_dev = cpu_dev; + info->proc_reg = proc_reg; + info->sram_reg = IS_ERR(sram_reg) ? NULL : sram_reg; + info->cpu_clk = cpu_clk; + info->inter_clk = inter_clk; + + /* + * If SRAM regulator is present, software "voltage trace" is needed + * for this CPU power domain. + */ + info->need_voltage_trace = !IS_ERR(sram_reg); + + return 0; + +out_free_resources: + if (!IS_ERR(proc_reg)) + regulator_put(proc_reg); + if (!IS_ERR(sram_reg)) + regulator_put(sram_reg); + if (!IS_ERR(cpu_clk)) + clk_put(cpu_clk); + if (!IS_ERR(inter_clk)) + clk_put(inter_clk); + +out_free_opp_table: + of_free_opp_table(cpu_dev); + + return ret; +} + +static void mtk_cpu_dvfs_info_release(struct mtk_cpu_dvfs_info *info) +{ + if (!IS_ERR(info->proc_reg)) + regulator_put(info->proc_reg); + if (!IS_ERR(info->sram_reg)) + regulator_put(info->sram_reg); + if (!IS_ERR(info->cpu_clk)) + clk_put(info->cpu_clk); + if (!IS_ERR(info->inter_clk)) + clk_put(info->inter_clk); + + of_free_opp_table(info->cpu_dev); +} + +static int mtk_cpufreq_init(struct cpufreq_policy *policy) +{ + struct mtk_cpu_dvfs_info *info; + struct cpufreq_frequency_table *freq_table; + int ret; + + info = kzalloc(sizeof(*info), GFP_KERNEL); + if (!info) + return -ENOMEM; + + ret = mtk_cpu_dvfs_info_init(info, policy->cpu); + if (ret) { + pr_err("%s failed to initialize dvfs info for cpu%d\n", + __func__, policy->cpu); + goto out_free_dvfs_info; + } + + ret = dev_pm_opp_init_cpufreq_table(info->cpu_dev, &freq_table); + if (ret) { + pr_err("failed to init cpufreq table for cpu%d: %d\n", + policy->cpu, ret); + goto out_release_dvfs_info; + } + + ret = cpufreq_table_validate_and_show(policy, freq_table); + if (ret) { + pr_err("%s: invalid frequency table: %d\n", __func__, ret); + goto out_free_cpufreq_table; + } + + /* CPUs in the same cluster share a clock and power domain. */ + cpumask_copy(policy->cpus, &cpu_topology[policy->cpu].core_sibling); + policy->driver_data = info; + policy->clk = info->cpu_clk; + + return 0; + +out_free_cpufreq_table: + dev_pm_opp_free_cpufreq_table(info->cpu_dev, &freq_table); + +out_release_dvfs_info: + mtk_cpu_dvfs_info_release(info); + +out_free_dvfs_info: + kfree(info); + + return ret; +} + +static int mtk_cpufreq_exit(struct cpufreq_policy *policy) +{ + struct mtk_cpu_dvfs_info *info = policy->driver_data; + + cpufreq_cooling_unregister(info->cdev); + dev_pm_opp_free_cpufreq_table(info->cpu_dev, &policy->freq_table); + mtk_cpu_dvfs_info_release(info); + kfree(info); + + return 0; +} + +static struct cpufreq_driver mt8173_cpufreq_driver = { + .flags = CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK, + .verify = cpufreq_generic_frequency_table_verify, + .target_index = mtk_cpufreq_set_target, + .get = cpufreq_generic_get, + .init = mtk_cpufreq_init, + .exit = mtk_cpufreq_exit, + .ready = mtk_cpufreq_ready, + .name = "mtk-cpufreq", + .attr = cpufreq_generic_attr, +}; + +static int mt8173_cpufreq_probe(struct platform_device *pdev) +{ + int ret; + + ret = cpufreq_register_driver(&mt8173_cpufreq_driver); + if (ret) + pr_err("failed to register mtk cpufreq driver\n"); + + return ret; +} + +static struct platform_driver mt8173_cpufreq_platdrv = { + .driver = { + .name = "mt8173-cpufreq", + }, + .probe = mt8173_cpufreq_probe, +}; + +static int mt8173_cpufreq_driver_init(void) +{ + struct platform_device *pdev; + int err; + + if (!of_machine_is_compatible("mediatek,mt8173")) + return -ENODEV; + + err = platform_driver_register(&mt8173_cpufreq_platdrv); + if (err) + return err; + + pdev = platform_device_register_simple("mt8173-cpufreq", -1, NULL, 0); + if (IS_ERR(pdev)) { + pr_err("failed to register mtk-cpufreq platform device\n"); + return PTR_ERR(pdev); + } + + return 0; +} +device_initcall(mt8173_cpufreq_driver_init); -- 1.9.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver 2015-07-01 2:16 ` [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen @ 2015-07-01 21:24 ` Alexey Klimov 2015-07-02 1:31 ` Pi-Cheng Chen 2015-07-08 11:34 ` Viresh Kumar 1 sibling, 1 reply; 10+ messages in thread From: Alexey Klimov @ 2015-07-01 21:24 UTC (permalink / raw) To: linux-arm-kernel Hi Pi-Cheng, On Wed, Jul 1, 2015 at 5:16 AM, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote: > This patch implements MT8173 cpufreq driver. > > Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org> > --- > drivers/cpufreq/Kconfig.arm | 7 + > drivers/cpufreq/Makefile | 1 + > drivers/cpufreq/mt8173-cpufreq.c | 520 +++++++++++++++++++++++++++++++++++++++ > 3 files changed, 528 insertions(+) > create mode 100644 drivers/cpufreq/mt8173-cpufreq.c > > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm > index 611cb09..2a305c0 100644 > --- a/drivers/cpufreq/Kconfig.arm > +++ b/drivers/cpufreq/Kconfig.arm > @@ -141,6 +141,13 @@ config ARM_KIRKWOOD_CPUFREQ > This adds the CPUFreq driver for Marvell Kirkwood > SoCs. > > +config ARM_MT8173_CPUFREQ > + bool "Mediatek MT8173 CPUFreq support" > + depends on ARCH_MEDIATEK && REGULATOR > + select PM_OPP > + help > + This adds the CPUFreq driver support for Mediatek MT8173 SoC. > + > config ARM_OMAP2PLUS_CPUFREQ > bool "TI OMAP2+" > depends on ARCH_OMAP2PLUS > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile > index cdce92a..97f9a9b 100644 > --- a/drivers/cpufreq/Makefile > +++ b/drivers/cpufreq/Makefile > @@ -63,6 +63,7 @@ obj-$(CONFIG_ARM_HISI_ACPU_CPUFREQ) += hisi-acpu-cpufreq.o > obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) += imx6q-cpufreq.o > obj-$(CONFIG_ARM_INTEGRATOR) += integrator-cpufreq.o > obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o > +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o > obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o > obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o > obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o > diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c > new file mode 100644 > index 0000000..6284e67 > --- /dev/null > +++ b/drivers/cpufreq/mt8173-cpufreq.c > @@ -0,0 +1,520 @@ > +/* > + * Copyright (c) 2015 Linaro Ltd. > + * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org> > + * > + * This program is free software; you can redistribute it and/or modify > + * it under the terms of the GNU General Public License version 2 as > + * published by the Free Software Foundation. > + * > + * This program is distributed in the hope that it will be useful, > + * but WITHOUT ANY WARRANTY; without even the implied warranty of > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the > + * GNU General Public License for more details. > + */ > + > +#include <linux/clk.h> > +#include <linux/cpu.h> > +#include <linux/cpu_cooling.h> > +#include <linux/cpufreq.h> > +#include <linux/cpumask.h> > +#include <linux/of.h> > +#include <linux/platform_device.h> > +#include <linux/pm_opp.h> > +#include <linux/regulator/consumer.h> > +#include <linux/slab.h> > +#include <linux/thermal.h> > + > +#define MIN_VOLT_SHIFT (100000) > +#define MAX_VOLT_SHIFT (200000) > +#define MAX_VOLT_LIMIT (1150000) > +#define VOLT_TOL (10000) > + > +/* > + * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS > + * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in > + * Mediatek SoCs has two voltage inputs, Vproc and Vsram. In some cases the two > + * voltage inputs need to be controlled under a hardware limitation: > + * 100mV < Vsram - Vproc < 200mV > + * > + * When scaling the clock frequency of a CPU clock domain, the clock source > + * needs to be switched to another stable PLL clock temporarily until > + * the original PLL becomes stable at target frequency. > + */ > +struct mtk_cpu_dvfs_info { > + struct device *cpu_dev; > + struct regulator *proc_reg; > + struct regulator *sram_reg; > + struct clk *cpu_clk; > + struct clk *inter_clk; > + struct thermal_cooling_device *cdev; > + int intermediate_voltage; > + bool need_voltage_trace; > +}; > + > +static int mtk_cpufreq_voltage_trace(struct mtk_cpu_dvfs_info *info, > + int new_vproc) > +{ > + struct regulator *proc_reg = info->proc_reg; > + struct regulator *sram_reg = info->sram_reg; > + int old_vproc, old_vsram, new_vsram, vsram, vproc, ret; > + > + old_vproc = regulator_get_voltage(proc_reg); > + old_vsram = regulator_get_voltage(sram_reg); > + /* Vsram should not exceed the maximum allowed voltage of SoC. */ > + new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT); > + > + if (old_vproc < new_vproc) { > + /* > + * When scaling up voltages, Vsram and Vproc scale up step > + * by step. At each step, set Vsram to (Vproc + 200mV) first, > + * then set Vproc to (Vsram - 100mV). > + * Keep doing it until Vsram and Vproc hit target voltages. > + */ > + do { > + old_vsram = regulator_get_voltage(sram_reg); > + old_vproc = regulator_get_voltage(proc_reg); > + > + vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT); > + > + if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) { > + vsram = MAX_VOLT_LIMIT; > + > + /* > + * If the target Vsram hits the maximum voltage, > + * try to set the exact voltage value first. > + */ > + ret = regulator_set_voltage(sram_reg, vsram, > + vsram); > + if (ret) > + ret = regulator_set_voltage(sram_reg, > + vsram - VOLT_TOL, > + vsram); > + > + vproc = new_vproc; > + } else { > + ret = regulator_set_voltage(sram_reg, vsram, > + vsram + VOLT_TOL); > + > + vproc = vsram - MIN_VOLT_SHIFT; > + } > + if (ret) > + return ret; > + > + ret = regulator_set_voltage(proc_reg, vproc, > + vproc + VOLT_TOL); > + if (ret) { > + regulator_set_voltage(sram_reg, old_vsram, > + old_vsram); > + return ret; > + } > + } while (vproc < new_vproc || vsram < new_vsram); > + } else if (old_vproc > new_vproc) { > + /* > + * When scaling down voltages, Vsram and Vproc scale down step > + * by step. At each step, set Vproc to (Vsram - 200mV) first, > + * then set Vproc to (Vproc + 100mV). > + * Keep doing it until Vsram and Vproc hit target voltages. > + */ > + do { > + old_vproc = regulator_get_voltage(proc_reg); > + old_vsram = regulator_get_voltage(sram_reg); > + > + vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT); > + ret = regulator_set_voltage(proc_reg, vproc, > + vproc + VOLT_TOL); > + if (ret) > + return ret; > + > + if (vproc == new_vproc) > + vsram = new_vsram; > + else > + vsram = max(new_vsram, vproc + MIN_VOLT_SHIFT); > + > + if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) { > + vsram = MAX_VOLT_LIMIT; > + > + /* > + * If the target Vsram hits the maximum voltage, > + * try to set the exact voltage value first. > + */ > + ret = regulator_set_voltage(sram_reg, vsram, > + vsram); > + if (ret) > + ret = regulator_set_voltage(sram_reg, > + vsram - VOLT_TOL, > + vsram); > + } else { > + ret = regulator_set_voltage(sram_reg, vsram, > + vsram + VOLT_TOL); > + } > + > + if (ret) { > + regulator_set_voltage(proc_reg, old_vproc, > + old_vproc); > + return ret; > + } > + } while (vproc > new_vproc + VOLT_TOL || > + vsram > new_vsram + VOLT_TOL); > + } > + > + return 0; > +} > + > +static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) > +{ > + if (info->need_voltage_trace) > + return mtk_cpufreq_voltage_trace(info, vproc); > + else > + return regulator_set_voltage(info->proc_reg, vproc, > + vproc + VOLT_TOL); > +} > + > +static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, > + unsigned int index) > +{ > + struct cpufreq_frequency_table *freq_table = policy->freq_table; > + struct clk *cpu_clk = policy->clk; > + struct clk *armpll = clk_get_parent(cpu_clk); > + struct mtk_cpu_dvfs_info *info = policy->driver_data; > + struct device *cpu_dev = info->cpu_dev; > + struct dev_pm_opp *opp; > + long freq_hz, old_freq_hz; > + int vproc, old_vproc, inter_vproc, target_vproc, ret; > + > + inter_vproc = info->intermediate_voltage; > + > + old_freq_hz = clk_get_rate(cpu_clk); > + old_vproc = regulator_get_voltage(info->proc_reg); > + > + freq_hz = freq_table[index].frequency * 1000; > + > + rcu_read_lock(); > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); > + if (IS_ERR(opp)) { > + rcu_read_unlock(); > + pr_err("cpu%d: failed to find OPP for %ld\n", > + policy->cpu, freq_hz); > + return PTR_ERR(opp); > + } > + vproc = dev_pm_opp_get_voltage(opp); > + rcu_read_unlock(); > + > + /* > + * If the new voltage or the intermediate voltage is higher than the > + * current voltage, scale up voltage first. > + */ > + target_vproc = (inter_vproc > vproc) ? inter_vproc : vproc; > + if (old_vproc < target_vproc) { > + ret = mtk_cpufreq_set_voltage(info, target_vproc); > + if (ret) { > + pr_err("cpu%d: failed to scale up voltage!\n", > + policy->cpu); > + mtk_cpufreq_set_voltage(info, old_vproc); > + return ret; > + } > + } > + > + /* Reparent the CPU clock to intermediate clock. */ > + ret = clk_set_parent(cpu_clk, info->inter_clk); > + if (ret) { > + pr_err("cpu%d: failed to re-parent cpu clock!\n", > + policy->cpu); > + mtk_cpufreq_set_voltage(info, old_vproc); > + WARN_ON(1); > + return ret; > + } > + > + /* Set the original PLL to target rate. */ > + ret = clk_set_rate(armpll, freq_hz); > + if (ret) { > + pr_err("cpu%d: failed to scale cpu clock rate!\n", > + policy->cpu); > + clk_set_parent(cpu_clk, armpll); > + mtk_cpufreq_set_voltage(info, old_vproc); > + return ret; > + } > + > + /* Set parent of CPU clock back to the original PLL. */ > + ret = clk_set_parent(cpu_clk, armpll); > + if (ret) { > + pr_err("cpu%d: failed to re-parent cpu clock!\n", > + policy->cpu); > + mtk_cpufreq_set_voltage(info, inter_vproc); > + WARN_ON(1); > + return ret; > + } > + > + /* > + * If the new voltage is lower than the intermediate voltage or the > + * original voltage, scale down to the new voltage. > + */ > + if (vproc < inter_vproc || vproc < old_vproc) { > + ret = mtk_cpufreq_set_voltage(info, vproc); > + if (ret) { > + pr_err("cpu%d: failed to scale down voltage!\n", > + policy->cpu); > + clk_set_parent(cpu_clk, info->inter_clk); > + clk_set_rate(armpll, old_freq_hz); > + clk_set_parent(cpu_clk, armpll); > + return ret; > + } > + } > + > + return 0; > +} > + > +static void mtk_cpufreq_ready(struct cpufreq_policy *policy) > +{ > + struct mtk_cpu_dvfs_info *info = policy->driver_data; > + struct device_node *np = of_node_get(info->cpu_dev->of_node); > + > + if (WARN_ON(!np)) > + return; > + > + if (of_find_property(np, "#cooling-cells", NULL)) { > + info->cdev = of_cpufreq_cooling_register(np, > + policy->related_cpus); > + > + if (IS_ERR(info->cdev)) { > + dev_err(info->cpu_dev, > + "running cpufreq without cooling device: %ld\n", > + PTR_ERR(info->cdev)); > + > + info->cdev = NULL; > + } > + } > + > + of_node_put(np); > +} > + > +static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > +{ > + struct device *cpu_dev; > + struct regulator *proc_reg = ERR_PTR(-ENODEV); > + struct regulator *sram_reg = ERR_PTR(-ENODEV); > + struct clk *cpu_clk = ERR_PTR(-ENODEV); > + struct clk *inter_clk = ERR_PTR(-ENODEV); > + struct dev_pm_opp *opp; > + unsigned long rate; > + int ret; > + > + cpu_dev = get_cpu_device(cpu); > + if (!cpu_dev) { > + pr_err("failed to get cpu%d device\n", cpu); > + return -ENODEV; > + } > + > + ret = of_init_opp_table(cpu_dev); > + if (ret) { > + pr_warn("no OPP table for cpu%d\n", cpu); > + return ret; > + } > + > + cpu_clk = clk_get(cpu_dev, "cpu"); > + if (IS_ERR(cpu_clk)) { > + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER) > + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu); > + else > + pr_err("failed to get cpu clk for cpu%d\n", cpu); > + > + ret = PTR_ERR(cpu_clk); > + goto out_free_opp_table; > + } > + > + inter_clk = clk_get(cpu_dev, "intermediate"); > + if (IS_ERR(inter_clk)) { > + if (PTR_ERR(inter_clk) == -EPROBE_DEFER) > + pr_warn("intermediate clk for cpu%d not ready, retry.\n", > + cpu); > + else > + pr_err("failed to get intermediate clk for cpu%d\n", > + cpu); > + > + ret = PTR_ERR(inter_clk); > + goto out_free_resources; > + } > + > + proc_reg = regulator_get_exclusive(cpu_dev, "proc"); > + if (IS_ERR(proc_reg)) { > + if (PTR_ERR(proc_reg) == -EPROBE_DEFER) > + pr_warn("proc regulator for cpu%d not ready, retry.\n", > + cpu); > + else > + pr_err("failed to get proc regulator for cpu%d\n", > + cpu); > + > + ret = PTR_ERR(proc_reg); > + goto out_free_resources; > + } > + > + /* Both presence and absence of sram regulator are valid cases. */ > + sram_reg = regulator_get_exclusive(cpu_dev, "sram"); > + > + /* Search a safe voltage for intermediate frequency. */ > + rate = clk_get_rate(inter_clk); > + rcu_read_lock(); > + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); > + if (IS_ERR(opp)) { > + pr_err("failed to get intermediate opp for cpu%d\n", cpu); > + ret = PTR_ERR(opp); > + goto out_free_resources; > + } Could you please check this error path? Probably you missed rcu_read_unlock() here. > + info->intermediate_voltage = dev_pm_opp_get_voltage(opp); > + rcu_read_unlock(); > + > + info->cpu_dev = cpu_dev; > + info->proc_reg = proc_reg; > + info->sram_reg = IS_ERR(sram_reg) ? NULL : sram_reg; > + info->cpu_clk = cpu_clk; > + info->inter_clk = inter_clk; > + > + /* > + * If SRAM regulator is present, software "voltage trace" is needed > + * for this CPU power domain. > + */ > + info->need_voltage_trace = !IS_ERR(sram_reg); > + > + return 0; > + > +out_free_resources: > + if (!IS_ERR(proc_reg)) > + regulator_put(proc_reg); > + if (!IS_ERR(sram_reg)) > + regulator_put(sram_reg); > + if (!IS_ERR(cpu_clk)) > + clk_put(cpu_clk); > + if (!IS_ERR(inter_clk)) > + clk_put(inter_clk); > + > +out_free_opp_table: > + of_free_opp_table(cpu_dev); > + > + return ret; > +} <snip> Best regards, Alexey Klimov ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver 2015-07-01 21:24 ` Alexey Klimov @ 2015-07-02 1:31 ` Pi-Cheng Chen 0 siblings, 0 replies; 10+ messages in thread From: Pi-Cheng Chen @ 2015-07-02 1:31 UTC (permalink / raw) To: linux-arm-kernel Hi Alexey, On Thu, Jul 2, 2015 at 5:24 AM, Alexey Klimov <klimov.linux@gmail.com> wrote: > Hi Pi-Cheng, > > On Wed, Jul 1, 2015 at 5:16 AM, Pi-Cheng Chen <pi-cheng.chen@linaro.org> wrote: >> This patch implements MT8173 cpufreq driver. >> >> Signed-off-by: Pi-Cheng Chen <pi-cheng.chen@linaro.org> >> --- >> drivers/cpufreq/Kconfig.arm | 7 + >> drivers/cpufreq/Makefile | 1 + >> drivers/cpufreq/mt8173-cpufreq.c | 520 +++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 528 insertions(+) >> create mode 100644 drivers/cpufreq/mt8173-cpufreq.c >> >> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm >> index 611cb09..2a305c0 100644 >> --- a/drivers/cpufreq/Kconfig.arm >> +++ b/drivers/cpufreq/Kconfig.arm >> @@ -141,6 +141,13 @@ config ARM_KIRKWOOD_CPUFREQ >> This adds the CPUFreq driver for Marvell Kirkwood >> SoCs. >> >> +config ARM_MT8173_CPUFREQ >> + bool "Mediatek MT8173 CPUFreq support" >> + depends on ARCH_MEDIATEK && REGULATOR >> + select PM_OPP >> + help >> + This adds the CPUFreq driver support for Mediatek MT8173 SoC. >> + >> config ARM_OMAP2PLUS_CPUFREQ >> bool "TI OMAP2+" >> depends on ARCH_OMAP2PLUS >> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile >> index cdce92a..97f9a9b 100644 >> --- a/drivers/cpufreq/Makefile >> +++ b/drivers/cpufreq/Makefile >> @@ -63,6 +63,7 @@ obj-$(CONFIG_ARM_HISI_ACPU_CPUFREQ) += hisi-acpu-cpufreq.o >> obj-$(CONFIG_ARM_IMX6Q_CPUFREQ) += imx6q-cpufreq.o >> obj-$(CONFIG_ARM_INTEGRATOR) += integrator-cpufreq.o >> obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ) += kirkwood-cpufreq.o >> +obj-$(CONFIG_ARM_MT8173_CPUFREQ) += mt8173-cpufreq.o >> obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ) += omap-cpufreq.o >> obj-$(CONFIG_ARM_PXA2xx_CPUFREQ) += pxa2xx-cpufreq.o >> obj-$(CONFIG_PXA3xx) += pxa3xx-cpufreq.o >> diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c >> new file mode 100644 >> index 0000000..6284e67 >> --- /dev/null >> +++ b/drivers/cpufreq/mt8173-cpufreq.c >> @@ -0,0 +1,520 @@ >> +/* >> + * Copyright (c) 2015 Linaro Ltd. >> + * Author: Pi-Cheng Chen <pi-cheng.chen@linaro.org> >> + * >> + * This program is free software; you can redistribute it and/or modify >> + * it under the terms of the GNU General Public License version 2 as >> + * published by the Free Software Foundation. >> + * >> + * This program is distributed in the hope that it will be useful, >> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >> + * GNU General Public License for more details. >> + */ >> + >> +#include <linux/clk.h> >> +#include <linux/cpu.h> >> +#include <linux/cpu_cooling.h> >> +#include <linux/cpufreq.h> >> +#include <linux/cpumask.h> >> +#include <linux/of.h> >> +#include <linux/platform_device.h> >> +#include <linux/pm_opp.h> >> +#include <linux/regulator/consumer.h> >> +#include <linux/slab.h> >> +#include <linux/thermal.h> >> + >> +#define MIN_VOLT_SHIFT (100000) >> +#define MAX_VOLT_SHIFT (200000) >> +#define MAX_VOLT_LIMIT (1150000) >> +#define VOLT_TOL (10000) >> + >> +/* >> + * The struct mtk_cpu_dvfs_info holds necessary information for doing CPU DVFS >> + * on each CPU power/clock domain of Mediatek SoCs. Each CPU cluster in >> + * Mediatek SoCs has two voltage inputs, Vproc and Vsram. In some cases the two >> + * voltage inputs need to be controlled under a hardware limitation: >> + * 100mV < Vsram - Vproc < 200mV >> + * >> + * When scaling the clock frequency of a CPU clock domain, the clock source >> + * needs to be switched to another stable PLL clock temporarily until >> + * the original PLL becomes stable at target frequency. >> + */ >> +struct mtk_cpu_dvfs_info { >> + struct device *cpu_dev; >> + struct regulator *proc_reg; >> + struct regulator *sram_reg; >> + struct clk *cpu_clk; >> + struct clk *inter_clk; >> + struct thermal_cooling_device *cdev; >> + int intermediate_voltage; >> + bool need_voltage_trace; >> +}; >> + >> +static int mtk_cpufreq_voltage_trace(struct mtk_cpu_dvfs_info *info, >> + int new_vproc) >> +{ >> + struct regulator *proc_reg = info->proc_reg; >> + struct regulator *sram_reg = info->sram_reg; >> + int old_vproc, old_vsram, new_vsram, vsram, vproc, ret; >> + >> + old_vproc = regulator_get_voltage(proc_reg); >> + old_vsram = regulator_get_voltage(sram_reg); >> + /* Vsram should not exceed the maximum allowed voltage of SoC. */ >> + new_vsram = min(new_vproc + MIN_VOLT_SHIFT, MAX_VOLT_LIMIT); >> + >> + if (old_vproc < new_vproc) { >> + /* >> + * When scaling up voltages, Vsram and Vproc scale up step >> + * by step. At each step, set Vsram to (Vproc + 200mV) first, >> + * then set Vproc to (Vsram - 100mV). >> + * Keep doing it until Vsram and Vproc hit target voltages. >> + */ >> + do { >> + old_vsram = regulator_get_voltage(sram_reg); >> + old_vproc = regulator_get_voltage(proc_reg); >> + >> + vsram = min(new_vsram, old_vproc + MAX_VOLT_SHIFT); >> + >> + if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) { >> + vsram = MAX_VOLT_LIMIT; >> + >> + /* >> + * If the target Vsram hits the maximum voltage, >> + * try to set the exact voltage value first. >> + */ >> + ret = regulator_set_voltage(sram_reg, vsram, >> + vsram); >> + if (ret) >> + ret = regulator_set_voltage(sram_reg, >> + vsram - VOLT_TOL, >> + vsram); >> + >> + vproc = new_vproc; >> + } else { >> + ret = regulator_set_voltage(sram_reg, vsram, >> + vsram + VOLT_TOL); >> + >> + vproc = vsram - MIN_VOLT_SHIFT; >> + } >> + if (ret) >> + return ret; >> + >> + ret = regulator_set_voltage(proc_reg, vproc, >> + vproc + VOLT_TOL); >> + if (ret) { >> + regulator_set_voltage(sram_reg, old_vsram, >> + old_vsram); >> + return ret; >> + } >> + } while (vproc < new_vproc || vsram < new_vsram); >> + } else if (old_vproc > new_vproc) { >> + /* >> + * When scaling down voltages, Vsram and Vproc scale down step >> + * by step. At each step, set Vproc to (Vsram - 200mV) first, >> + * then set Vproc to (Vproc + 100mV). >> + * Keep doing it until Vsram and Vproc hit target voltages. >> + */ >> + do { >> + old_vproc = regulator_get_voltage(proc_reg); >> + old_vsram = regulator_get_voltage(sram_reg); >> + >> + vproc = max(new_vproc, old_vsram - MAX_VOLT_SHIFT); >> + ret = regulator_set_voltage(proc_reg, vproc, >> + vproc + VOLT_TOL); >> + if (ret) >> + return ret; >> + >> + if (vproc == new_vproc) >> + vsram = new_vsram; >> + else >> + vsram = max(new_vsram, vproc + MIN_VOLT_SHIFT); >> + >> + if (vsram + VOLT_TOL >= MAX_VOLT_LIMIT) { >> + vsram = MAX_VOLT_LIMIT; >> + >> + /* >> + * If the target Vsram hits the maximum voltage, >> + * try to set the exact voltage value first. >> + */ >> + ret = regulator_set_voltage(sram_reg, vsram, >> + vsram); >> + if (ret) >> + ret = regulator_set_voltage(sram_reg, >> + vsram - VOLT_TOL, >> + vsram); >> + } else { >> + ret = regulator_set_voltage(sram_reg, vsram, >> + vsram + VOLT_TOL); >> + } >> + >> + if (ret) { >> + regulator_set_voltage(proc_reg, old_vproc, >> + old_vproc); >> + return ret; >> + } >> + } while (vproc > new_vproc + VOLT_TOL || >> + vsram > new_vsram + VOLT_TOL); >> + } >> + >> + return 0; >> +} >> + >> +static int mtk_cpufreq_set_voltage(struct mtk_cpu_dvfs_info *info, int vproc) >> +{ >> + if (info->need_voltage_trace) >> + return mtk_cpufreq_voltage_trace(info, vproc); >> + else >> + return regulator_set_voltage(info->proc_reg, vproc, >> + vproc + VOLT_TOL); >> +} >> + >> +static int mtk_cpufreq_set_target(struct cpufreq_policy *policy, >> + unsigned int index) >> +{ >> + struct cpufreq_frequency_table *freq_table = policy->freq_table; >> + struct clk *cpu_clk = policy->clk; >> + struct clk *armpll = clk_get_parent(cpu_clk); >> + struct mtk_cpu_dvfs_info *info = policy->driver_data; >> + struct device *cpu_dev = info->cpu_dev; >> + struct dev_pm_opp *opp; >> + long freq_hz, old_freq_hz; >> + int vproc, old_vproc, inter_vproc, target_vproc, ret; >> + >> + inter_vproc = info->intermediate_voltage; >> + >> + old_freq_hz = clk_get_rate(cpu_clk); >> + old_vproc = regulator_get_voltage(info->proc_reg); >> + >> + freq_hz = freq_table[index].frequency * 1000; >> + >> + rcu_read_lock(); >> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &freq_hz); >> + if (IS_ERR(opp)) { >> + rcu_read_unlock(); >> + pr_err("cpu%d: failed to find OPP for %ld\n", >> + policy->cpu, freq_hz); >> + return PTR_ERR(opp); >> + } >> + vproc = dev_pm_opp_get_voltage(opp); >> + rcu_read_unlock(); >> + >> + /* >> + * If the new voltage or the intermediate voltage is higher than the >> + * current voltage, scale up voltage first. >> + */ >> + target_vproc = (inter_vproc > vproc) ? inter_vproc : vproc; >> + if (old_vproc < target_vproc) { >> + ret = mtk_cpufreq_set_voltage(info, target_vproc); >> + if (ret) { >> + pr_err("cpu%d: failed to scale up voltage!\n", >> + policy->cpu); >> + mtk_cpufreq_set_voltage(info, old_vproc); >> + return ret; >> + } >> + } >> + >> + /* Reparent the CPU clock to intermediate clock. */ >> + ret = clk_set_parent(cpu_clk, info->inter_clk); >> + if (ret) { >> + pr_err("cpu%d: failed to re-parent cpu clock!\n", >> + policy->cpu); >> + mtk_cpufreq_set_voltage(info, old_vproc); >> + WARN_ON(1); >> + return ret; >> + } >> + >> + /* Set the original PLL to target rate. */ >> + ret = clk_set_rate(armpll, freq_hz); >> + if (ret) { >> + pr_err("cpu%d: failed to scale cpu clock rate!\n", >> + policy->cpu); >> + clk_set_parent(cpu_clk, armpll); >> + mtk_cpufreq_set_voltage(info, old_vproc); >> + return ret; >> + } >> + >> + /* Set parent of CPU clock back to the original PLL. */ >> + ret = clk_set_parent(cpu_clk, armpll); >> + if (ret) { >> + pr_err("cpu%d: failed to re-parent cpu clock!\n", >> + policy->cpu); >> + mtk_cpufreq_set_voltage(info, inter_vproc); >> + WARN_ON(1); >> + return ret; >> + } >> + >> + /* >> + * If the new voltage is lower than the intermediate voltage or the >> + * original voltage, scale down to the new voltage. >> + */ >> + if (vproc < inter_vproc || vproc < old_vproc) { >> + ret = mtk_cpufreq_set_voltage(info, vproc); >> + if (ret) { >> + pr_err("cpu%d: failed to scale down voltage!\n", >> + policy->cpu); >> + clk_set_parent(cpu_clk, info->inter_clk); >> + clk_set_rate(armpll, old_freq_hz); >> + clk_set_parent(cpu_clk, armpll); >> + return ret; >> + } >> + } >> + >> + return 0; >> +} >> + >> +static void mtk_cpufreq_ready(struct cpufreq_policy *policy) >> +{ >> + struct mtk_cpu_dvfs_info *info = policy->driver_data; >> + struct device_node *np = of_node_get(info->cpu_dev->of_node); >> + >> + if (WARN_ON(!np)) >> + return; >> + >> + if (of_find_property(np, "#cooling-cells", NULL)) { >> + info->cdev = of_cpufreq_cooling_register(np, >> + policy->related_cpus); >> + >> + if (IS_ERR(info->cdev)) { >> + dev_err(info->cpu_dev, >> + "running cpufreq without cooling device: %ld\n", >> + PTR_ERR(info->cdev)); >> + >> + info->cdev = NULL; >> + } >> + } >> + >> + of_node_put(np); >> +} >> + >> +static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) >> +{ >> + struct device *cpu_dev; >> + struct regulator *proc_reg = ERR_PTR(-ENODEV); >> + struct regulator *sram_reg = ERR_PTR(-ENODEV); >> + struct clk *cpu_clk = ERR_PTR(-ENODEV); >> + struct clk *inter_clk = ERR_PTR(-ENODEV); >> + struct dev_pm_opp *opp; >> + unsigned long rate; >> + int ret; >> + >> + cpu_dev = get_cpu_device(cpu); >> + if (!cpu_dev) { >> + pr_err("failed to get cpu%d device\n", cpu); >> + return -ENODEV; >> + } >> + >> + ret = of_init_opp_table(cpu_dev); >> + if (ret) { >> + pr_warn("no OPP table for cpu%d\n", cpu); >> + return ret; >> + } >> + >> + cpu_clk = clk_get(cpu_dev, "cpu"); >> + if (IS_ERR(cpu_clk)) { >> + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER) >> + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu); >> + else >> + pr_err("failed to get cpu clk for cpu%d\n", cpu); >> + >> + ret = PTR_ERR(cpu_clk); >> + goto out_free_opp_table; >> + } >> + >> + inter_clk = clk_get(cpu_dev, "intermediate"); >> + if (IS_ERR(inter_clk)) { >> + if (PTR_ERR(inter_clk) == -EPROBE_DEFER) >> + pr_warn("intermediate clk for cpu%d not ready, retry.\n", >> + cpu); >> + else >> + pr_err("failed to get intermediate clk for cpu%d\n", >> + cpu); >> + >> + ret = PTR_ERR(inter_clk); >> + goto out_free_resources; >> + } >> + >> + proc_reg = regulator_get_exclusive(cpu_dev, "proc"); >> + if (IS_ERR(proc_reg)) { >> + if (PTR_ERR(proc_reg) == -EPROBE_DEFER) >> + pr_warn("proc regulator for cpu%d not ready, retry.\n", >> + cpu); >> + else >> + pr_err("failed to get proc regulator for cpu%d\n", >> + cpu); >> + >> + ret = PTR_ERR(proc_reg); >> + goto out_free_resources; >> + } >> + >> + /* Both presence and absence of sram regulator are valid cases. */ >> + sram_reg = regulator_get_exclusive(cpu_dev, "sram"); >> + >> + /* Search a safe voltage for intermediate frequency. */ >> + rate = clk_get_rate(inter_clk); >> + rcu_read_lock(); >> + opp = dev_pm_opp_find_freq_ceil(cpu_dev, &rate); >> + if (IS_ERR(opp)) { >> + pr_err("failed to get intermediate opp for cpu%d\n", cpu); >> + ret = PTR_ERR(opp); >> + goto out_free_resources; >> + } > > Could you please check this error path? Probably you missed > rcu_read_unlock() here. Yes. rcu_read_unlock is missed here. I will add it. Thanks for your reviewing. Best Regards, Pi-Cheng > >> + info->intermediate_voltage = dev_pm_opp_get_voltage(opp); >> + rcu_read_unlock(); >> + >> + info->cpu_dev = cpu_dev; >> + info->proc_reg = proc_reg; >> + info->sram_reg = IS_ERR(sram_reg) ? NULL : sram_reg; >> + info->cpu_clk = cpu_clk; >> + info->inter_clk = inter_clk; >> + >> + /* >> + * If SRAM regulator is present, software "voltage trace" is needed >> + * for this CPU power domain. >> + */ >> + info->need_voltage_trace = !IS_ERR(sram_reg); >> + >> + return 0; >> + >> +out_free_resources: >> + if (!IS_ERR(proc_reg)) >> + regulator_put(proc_reg); >> + if (!IS_ERR(sram_reg)) >> + regulator_put(sram_reg); >> + if (!IS_ERR(cpu_clk)) >> + clk_put(cpu_clk); >> + if (!IS_ERR(inter_clk)) >> + clk_put(inter_clk); >> + >> +out_free_opp_table: >> + of_free_opp_table(cpu_dev); >> + >> + return ret; >> +} > > <snip> > > Best regards, > Alexey Klimov ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver 2015-07-01 2:16 ` [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen 2015-07-01 21:24 ` Alexey Klimov @ 2015-07-08 11:34 ` Viresh Kumar 2015-07-09 1:55 ` Pi-Cheng Chen 1 sibling, 1 reply; 10+ messages in thread From: Viresh Kumar @ 2015-07-08 11:34 UTC (permalink / raw) To: linux-arm-kernel On 01-07-15, 10:16, Pi-Cheng Chen wrote: > This patch implements MT8173 cpufreq driver. Now that you are going to resend this patchset, a few more comments. Please describe your SoC a bit here, so that reviewers know what are we going to implement. > +static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) > +{ > + struct device *cpu_dev; > + struct regulator *proc_reg = ERR_PTR(-ENODEV); > + struct regulator *sram_reg = ERR_PTR(-ENODEV); > + struct clk *cpu_clk = ERR_PTR(-ENODEV); > + struct clk *inter_clk = ERR_PTR(-ENODEV); > + struct dev_pm_opp *opp; > + unsigned long rate; > + int ret; > + > + cpu_dev = get_cpu_device(cpu); > + if (!cpu_dev) { > + pr_err("failed to get cpu%d device\n", cpu); > + return -ENODEV; > + } > + > + ret = of_init_opp_table(cpu_dev); > + if (ret) { > + pr_warn("no OPP table for cpu%d\n", cpu); > + return ret; > + } Initialize opp table only when you know other resources are available. i.e. we aren't going to abort due to EPROBE_DEFER. > + cpu_clk = clk_get(cpu_dev, "cpu"); > + if (IS_ERR(cpu_clk)) { > + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER) > + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu); > + else > + pr_err("failed to get cpu clk for cpu%d\n", cpu); > + > + ret = PTR_ERR(cpu_clk); > + goto out_free_opp_table; > + } > +} > +static int mt8173_cpufreq_probe(struct platform_device *pdev) > +{ > + int ret; > + > + ret = cpufreq_register_driver(&mt8173_cpufreq_driver); > + if (ret) > + pr_err("failed to register mtk cpufreq driver\n"); > + > + return ret; > +} > + > +static struct platform_driver mt8173_cpufreq_platdrv = { > + .driver = { > + .name = "mt8173-cpufreq", > + }, > + .probe = mt8173_cpufreq_probe, > +}; > + > +static int mt8173_cpufreq_driver_init(void) > +{ > + struct platform_device *pdev; > + int err; > + > + if (!of_machine_is_compatible("mediatek,mt8173")) > + return -ENODEV; Why do you need to check this ? Its going to be the right SoC, others can't even compile it. > + err = platform_driver_register(&mt8173_cpufreq_platdrv); > + if (err) > + return err; > + > + pdev = platform_device_register_simple("mt8173-cpufreq", -1, NULL, 0); > + if (IS_ERR(pdev)) { > + pr_err("failed to register mtk-cpufreq platform device\n"); > + return PTR_ERR(pdev); > + } This is simply crap. You register the driver and then its device from the same function. If you are so sure that this driver is required, then why to get into the shit of device-driver model ? Just call cpufreq_register_driver() from here :), no device/driver required. -- viresh ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver 2015-07-08 11:34 ` Viresh Kumar @ 2015-07-09 1:55 ` Pi-Cheng Chen 2015-07-09 5:17 ` Viresh Kumar 0 siblings, 1 reply; 10+ messages in thread From: Pi-Cheng Chen @ 2015-07-09 1:55 UTC (permalink / raw) To: linux-arm-kernel Hi Viresh, On Wed, Jul 8, 2015 at 7:34 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote: > On 01-07-15, 10:16, Pi-Cheng Chen wrote: >> This patch implements MT8173 cpufreq driver. > > Now that you are going to resend this patchset, a few more comments. > > Please describe your SoC a bit here, so that reviewers know what are > we going to implement. Yes. I will add it. > >> +static int mtk_cpu_dvfs_info_init(struct mtk_cpu_dvfs_info *info, int cpu) >> +{ >> + struct device *cpu_dev; >> + struct regulator *proc_reg = ERR_PTR(-ENODEV); >> + struct regulator *sram_reg = ERR_PTR(-ENODEV); >> + struct clk *cpu_clk = ERR_PTR(-ENODEV); >> + struct clk *inter_clk = ERR_PTR(-ENODEV); >> + struct dev_pm_opp *opp; >> + unsigned long rate; >> + int ret; >> + >> + cpu_dev = get_cpu_device(cpu); >> + if (!cpu_dev) { >> + pr_err("failed to get cpu%d device\n", cpu); >> + return -ENODEV; >> + } >> + >> + ret = of_init_opp_table(cpu_dev); >> + if (ret) { >> + pr_warn("no OPP table for cpu%d\n", cpu); >> + return ret; >> + } > > Initialize opp table only when you know other resources are available. > i.e. we aren't going to abort due to EPROBE_DEFER. Will do it. > >> + cpu_clk = clk_get(cpu_dev, "cpu"); >> + if (IS_ERR(cpu_clk)) { >> + if (PTR_ERR(cpu_clk) == -EPROBE_DEFER) >> + pr_warn("cpu clk for cpu%d not ready, retry.\n", cpu); >> + else >> + pr_err("failed to get cpu clk for cpu%d\n", cpu); >> + >> + ret = PTR_ERR(cpu_clk); >> + goto out_free_opp_table; >> + } >> +} > > >> +static int mt8173_cpufreq_probe(struct platform_device *pdev) >> +{ >> + int ret; >> + >> + ret = cpufreq_register_driver(&mt8173_cpufreq_driver); >> + if (ret) >> + pr_err("failed to register mtk cpufreq driver\n"); >> + >> + return ret; >> +} >> + >> +static struct platform_driver mt8173_cpufreq_platdrv = { >> + .driver = { >> + .name = "mt8173-cpufreq", >> + }, >> + .probe = mt8173_cpufreq_probe, >> +}; >> + >> +static int mt8173_cpufreq_driver_init(void) >> +{ >> + struct platform_device *pdev; >> + int err; >> + >> + if (!of_machine_is_compatible("mediatek,mt8173")) >> + return -ENODEV; > > Why do you need to check this ? Its going to be the right SoC, others > can't even compile it. Originally I was thinking this driver should be generic to other Mediatek SoCs, so I add this check to filter out those SoCs that are not yet confirmed with this driver supported. Since it's targeted to MT8173 only now, I will remove this check. > >> + err = platform_driver_register(&mt8173_cpufreq_platdrv); >> + if (err) >> + return err; >> + >> + pdev = platform_device_register_simple("mt8173-cpufreq", -1, NULL, 0); >> + if (IS_ERR(pdev)) { >> + pr_err("failed to register mtk-cpufreq platform device\n"); >> + return PTR_ERR(pdev); >> + } > > This is simply crap. You register the driver and then its device from > the same function. If you are so sure that this driver is required, > then why to get into the shit of device-driver model ? > > Just call cpufreq_register_driver() from here :), no device/driver > required. That's what I did in previous version. But the reason I use the device- driver model is to handle the defer probing issue. Since there's no arch/arm64/mach-mediatek/ directory to hold the device registration code anymore, no device tree way to match cpufreq driver (please correct me if there's any), and initcall seems not handle defer probing either, therefore I put both device and driver registration in this function. I know it's crappy. :( Do you have any suggestion to do it right and handle defer probing properly? Thanks. Best Regards, Pi-Cheng > > -- > viresh ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver 2015-07-09 1:55 ` Pi-Cheng Chen @ 2015-07-09 5:17 ` Viresh Kumar 0 siblings, 0 replies; 10+ messages in thread From: Viresh Kumar @ 2015-07-09 5:17 UTC (permalink / raw) To: linux-arm-kernel On 09-07-15, 09:55, Pi-Cheng Chen wrote: > That's what I did in previous version. But the reason I use the device- > driver model is to handle the defer probing issue. Since there's no > arch/arm64/mach-mediatek/ directory to hold the device registration > code anymore, no device tree way to match cpufreq driver > (please correct me if there's any), and initcall seems not handle > defer probing either, therefore I put both device and driver > registration in this function. I know it's crappy. :( > Do you have any suggestion to do it right and handle defer probing > properly? Sounds reasonable. Just add proper comment in code to explain that. -- viresh ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2015-07-09 15:04 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-07-01 2:16 [PATCH v5 0/2] Add Mediatek MT8173 cpufreq driver Pi-Cheng Chen 2015-07-01 2:16 ` [PATCH v5 1/2] dt-bindings: mediatek: Add MT8173 cpufreq driver binding Pi-Cheng Chen 2015-07-08 11:19 ` Viresh Kumar 2015-07-09 15:04 ` Michael Turquette 2015-07-01 2:16 ` [PATCH v5 2/2] cpufreq: mediatek: Add MT8173 cpufreq driver Pi-Cheng Chen 2015-07-01 21:24 ` Alexey Klimov 2015-07-02 1:31 ` Pi-Cheng Chen 2015-07-08 11:34 ` Viresh Kumar 2015-07-09 1:55 ` Pi-Cheng Chen 2015-07-09 5:17 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).