| Message ID | 1357806863-6899-3-git-send-email-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi, On Thu, Jan 10, 2013 at 14:04:23, Shawn Guo wrote: > Update operating-points per hardware document and add support for > 1 GHz and 1.2 GHz frequencies. > > 400 MHz, 800 MHz and 1 GHz should be supported by all i.MX6Q chips, > while 1.2 GHz support needs to know from OTP fuse bit. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > arch/arm/boot/dts/imx6q.dtsi | 19 ++++++++---- > arch/arm/mach-imx/mach-imx6q.c | 65 ++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 78 insertions(+), 6 deletions(-) > > diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi > index d6265ca..17c5618 100644 > --- a/arch/arm/boot/dts/imx6q.dtsi > +++ b/arch/arm/boot/dts/imx6q.dtsi > @@ -38,12 +38,18 @@ > next-level-cache = <&L2>; > operating-points = < > /* kHz uV */ > - 792000 1100000 > + 996000 1250000 > + 792000 1150000 > 396000 950000 > - 198000 850000 > >; [...] > > +#define OCOTP_CFG3 0x440 > +#define OCOTP_CFG3_SPEED_SHIFT 16 > +#define OCOTP_CFG3_SPEED_1P2GHZ 0x3 > + > +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev) > +{ > + struct device_node *np; > + void __iomem *base; > + u32 val; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp"); > + if (!np) { > + pr_warn("failed to find ocotp node\n"); > + return; > + } > + > + base = of_iomap(np, 0); > + if (!base) { > + pr_warn("failed to map ocotp\n"); > + goto put_node; > + } > + > + val = readl_relaxed(base + OCOTP_CFG3); > + val >>= OCOTP_CFG3_SPEED_SHIFT; > + if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ) > + if (opp_add(cpu_dev, 1200000000, 1275000)) > + pr_warn("failed to add 1.2 GHz operating point\n"); > + I just happened to be thinking about the problem of enabling additional OPPs based on some Si rev info for TI's AM335x. The other approach could be to register additional OPPs and mark then as disabled to begin with. A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev info. If this approach was already discussed sorry for the noise. Regards, Vaibhav
On Thu, Jan 10, 2013 at 10:50:25AM +0000, Bedia, Vaibhav wrote: ... > > +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev) > > +{ > > + struct device_node *np; > > + void __iomem *base; > > + u32 val; > > + > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp"); > > + if (!np) { > > + pr_warn("failed to find ocotp node\n"); > > + return; > > + } > > + > > + base = of_iomap(np, 0); > > + if (!base) { > > + pr_warn("failed to map ocotp\n"); > > + goto put_node; > > + } > > + > > + val = readl_relaxed(base + OCOTP_CFG3); > > + val >>= OCOTP_CFG3_SPEED_SHIFT; > > + if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ) > > + if (opp_add(cpu_dev, 1200000000, 1275000)) > > + pr_warn("failed to add 1.2 GHz operating point\n"); > > + > > I just happened to be thinking about the problem of enabling additional OPPs based on > some Si rev info for TI's AM335x. > > The other approach could be to register additional OPPs and mark then as disabled to begin with. > A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev > info. May I ask the advantage of this approach? I hate to create platform_data merely for passing a platform specific hook to driver. Shawn
On Thu, Jan 10, 2013 at 16:37:45, Shawn Guo wrote: > On Thu, Jan 10, 2013 at 10:50:25AM +0000, Bedia, Vaibhav wrote: > ... > > > +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev) > > > +{ > > > + struct device_node *np; > > > + void __iomem *base; > > > + u32 val; > > > + > > > + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp"); > > > + if (!np) { > > > + pr_warn("failed to find ocotp node\n"); > > > + return; > > > + } > > > + > > > + base = of_iomap(np, 0); > > > + if (!base) { > > > + pr_warn("failed to map ocotp\n"); > > > + goto put_node; > > > + } > > > + > > > + val = readl_relaxed(base + OCOTP_CFG3); > > > + val >>= OCOTP_CFG3_SPEED_SHIFT; > > > + if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ) > > > + if (opp_add(cpu_dev, 1200000000, 1275000)) > > > + pr_warn("failed to add 1.2 GHz operating point\n"); > > > + > > > > I just happened to be thinking about the problem of enabling additional OPPs based on > > some Si rev info for TI's AM335x. > > > > The other approach could be to register additional OPPs and mark then as disabled to begin with. > > A platform specific hook could then be used to selectively enable the OPPs for based on the Si rev > > info. > > May I ask the advantage of this approach? I hate to create > platform_data merely for passing a platform specific hook to driver. > In the current approach the OPP data is split across DT and kernel code. If you take the other approach all OPP entries can reside in DT and for someone just looking at that file there's no confusion about what the kernel could potentially support. Whether a particular an OPP should be supported is best decided at runtime. Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. Regards, Vaibhav
On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote: > In the current approach the OPP data is split across DT and kernel code. If you take the > other approach all OPP entries can reside in DT and for someone just looking at that file > there's no confusion about what the kernel could potentially support. Whether a particular > an OPP should be supported is best decided at runtime. > Listing the OPP that some Si rev can not support in DT is also a confusion to people who is just looking at DTS. To me, the approach is not really doing anything better on this aspect. > Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be > restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. > I do not see anything wrong with platform adding the OPP it can support. Isn't omap_init_opp_table() being called by platform code? I do not see omap-cpufreq driver is calling a platform hook to do that. Sorry, I'm not really a fan of platform hook, and I have spent past couple years to minimize the use of it when converting IMX family to device tree. Shawn
On Thu, Jan 10, 2013 at 18:50:55, Shawn Guo wrote: > On Thu, Jan 10, 2013 at 01:02:30PM +0000, Bedia, Vaibhav wrote: > > In the current approach the OPP data is split across DT and kernel code. If you take the > > other approach all OPP entries can reside in DT and for someone just looking at that file > > there's no confusion about what the kernel could potentially support. Whether a particular > > an OPP should be supported is best decided at runtime. > > > Listing the OPP that some Si rev can not support in DT is also > a confusion to people who is just looking at DTS. To me, the approach > is not really doing anything better on this aspect. > I still think putting the OPP data in a single place is better. > > Also, IMHO letting platforms invoke opp_add() is open to abuse and platforms should be > > restricted to invoking opp_enable/disable to indicate what's possible on a particular Si rev. > > > I do not see anything wrong with platform adding the OPP it can support. > Isn't omap_init_opp_table() being called by platform code? I do not see > omap-cpufreq driver is calling a platform hook to do that. > > Sorry, I'm not really a fan of platform hook, and I have spent past > couple years to minimize the use of it when converting IMX family to > device tree. > I don't know what the OMAP plans are but with DT + generic driver adoption I would expect most of it to go away. And adding the platform hook with the right DT data is just preparing things for the future with 2 potential users right now. If OPP data across Si rev starts looking very different all entries can be placed in a single place and the platform code enables what are relevant. Unless there's a Si rev specific blob a bunch of opp_add() calls ignoring what was passed defeats the purpose of moving the information out of the kernel. Regards, Vaibhav
diff --git a/arch/arm/boot/dts/imx6q.dtsi b/arch/arm/boot/dts/imx6q.dtsi index d6265ca..17c5618 100644 --- a/arch/arm/boot/dts/imx6q.dtsi +++ b/arch/arm/boot/dts/imx6q.dtsi @@ -38,12 +38,18 @@ next-level-cache = <&L2>; operating-points = < /* kHz uV */ - 792000 1100000 + 996000 1250000 + 792000 1150000 396000 950000 - 198000 850000 >; clock-latency = <61036>; /* two CLK32 periods */ - cpu0-supply = <®_cpu>; + clocks = <&clks 104>, <&clks 6>, <&clks 16>, + <&clks 17>, <&clks 170>; + clock-names = "arm", "pll2_pfd2_396m", "step", + "pll1_sw", "pll1_sys"; + arm-supply = <®_arm>; + pu-supply = <®_pu>; + soc-supply = <®_soc>; }; cpu@1 { @@ -471,7 +477,7 @@ anatop-max-voltage = <2750000>; }; - reg_cpu: regulator-vddcore@140 { + reg_arm: regulator-vddcore@140 { compatible = "fsl,anatop-regulator"; regulator-name = "cpu"; regulator-min-microvolt = <725000>; @@ -485,7 +491,7 @@ anatop-max-voltage = <1450000>; }; - regulator-vddpu@140 { + reg_pu: regulator-vddpu@140 { compatible = "fsl,anatop-regulator"; regulator-name = "vddpu"; regulator-min-microvolt = <725000>; @@ -499,7 +505,7 @@ anatop-max-voltage = <1450000>; }; - regulator-vddsoc@140 { + reg_soc: regulator-vddsoc@140 { compatible = "fsl,anatop-regulator"; regulator-name = "vddsoc"; regulator-min-microvolt = <725000>; @@ -965,6 +971,7 @@ }; ocotp@021bc000 { + compatible = "fsl,imx6q-ocotp"; reg = <0x021bc000 0x4000>; }; diff --git a/arch/arm/mach-imx/mach-imx6q.c b/arch/arm/mach-imx/mach-imx6q.c index 4eb1b3a..16f9a13 100644 --- a/arch/arm/mach-imx/mach-imx6q.c +++ b/arch/arm/mach-imx/mach-imx6q.c @@ -12,6 +12,7 @@ #include <linux/clk.h> #include <linux/clkdev.h> +#include <linux/cpu.h> #include <linux/cpuidle.h> #include <linux/delay.h> #include <linux/export.h> @@ -22,6 +23,7 @@ #include <linux/of_address.h> #include <linux/of_irq.h> #include <linux/of_platform.h> +#include <linux/opp.h> #include <linux/phy.h> #include <linux/regmap.h> #include <linux/micrel_phy.h> @@ -209,9 +211,72 @@ static struct cpuidle_driver imx6q_cpuidle_driver = { .state_count = 1, }; +#define OCOTP_CFG3 0x440 +#define OCOTP_CFG3_SPEED_SHIFT 16 +#define OCOTP_CFG3_SPEED_1P2GHZ 0x3 + +static void __init imx6q_opp_check_1p2ghz(struct device *cpu_dev) +{ + struct device_node *np; + void __iomem *base; + u32 val; + + np = of_find_compatible_node(NULL, NULL, "fsl,imx6q-ocotp"); + if (!np) { + pr_warn("failed to find ocotp node\n"); + return; + } + + base = of_iomap(np, 0); + if (!base) { + pr_warn("failed to map ocotp\n"); + goto put_node; + } + + val = readl_relaxed(base + OCOTP_CFG3); + val >>= OCOTP_CFG3_SPEED_SHIFT; + if ((val & 0x3) == OCOTP_CFG3_SPEED_1P2GHZ) + if (opp_add(cpu_dev, 1200000000, 1275000)) + pr_warn("failed to add 1.2 GHz operating point\n"); + +put_node: + of_node_put(np); +} + +static void __init imx6q_opp_init(struct device *cpu_dev) +{ + struct device_node *np; + + np = of_find_node_by_path("/cpus/cpu@0"); + if (!np) { + pr_warn("failed to find cpu0 node\n"); + return; + } + + cpu_dev->of_node = np; + if (of_init_opp_table(cpu_dev)) { + pr_warn("failed to init OPP table\n"); + goto put_node; + } + + imx6q_opp_check_1p2ghz(cpu_dev); + +put_node: + of_node_put(np); +} + +struct platform_device imx6q_cpufreq_pdev = { + .name = "imx6q-cpufreq", +}; + static void __init imx6q_init_late(void) { imx_cpuidle_init(&imx6q_cpuidle_driver); + + if (IS_ENABLED(CONFIG_ARM_IMX6Q_CPUFREQ)) { + imx6q_opp_init(&imx6q_cpufreq_pdev.dev); + platform_device_register(&imx6q_cpufreq_pdev); + } } static void __init imx6q_map_io(void)
Update operating-points per hardware document and add support for 1 GHz and 1.2 GHz frequencies. 400 MHz, 800 MHz and 1 GHz should be supported by all i.MX6Q chips, while 1.2 GHz support needs to know from OTP fuse bit. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- arch/arm/boot/dts/imx6q.dtsi | 19 ++++++++---- arch/arm/mach-imx/mach-imx6q.c | 65 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 78 insertions(+), 6 deletions(-)