This adds the actual cpufreq driver for the 750GX. It supports all integer
ratios that are valid for the processor model and bus frequency.
It has two modes of operation. Normal mode uses all valid frequencies. In
minmaxmode, only the minimum and maximum are used. This provides the ability
for very low latency frequency switches.
There is also a sysfs attribute to have it switch off the unused PLL for
additional power savings.
This does NOT support SMP.
My name is Kevin Diggs and I approve this patch.
Signed-off-by: Kevin Diggs <[email protected]>
Index: Documentation/DocBook/cf750gx.tmpl
===================================================================
--- /dev/null 2004-08-10 18:55:00.000000000 -0700
+++ Documentation/DocBook/cf750gx.tmpl 2008-08-20 10:00:14.000000000 -0700
@@ -0,0 +1,441 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!DOCTYPE book PUBLIC "-//OASIS//DTD DocBook XML V4.1.2//EN"
+ "http://www.oasis-open.org/docbook/xml/4.1.2/docbookx.dtd" []>
+
+<book id="ppc750gx-cpufreq-guide">
+ <bookinfo>
+ <title>PowerPC 750GX (and FX?) cpufreq driver guide</title>
+
+ <authorgroup>
+ <author>
+ <firstname>Kevin</firstname>
+ <surname>Diggs</surname>
+ </author>
+ </authorgroup>
+
+ <revhistory>
+ <revision>
+ <revnumber>1.0 </revnumber>
+ <date>August 12, 2008</date>
+ <revremark>Initial revision posted to linuxppc-dev</revremark>
+ </revision>
+ </revhistory>
+
+ <copyright>
+ <year>2008</year>
+ <holder>Kevin Diggs</holder>
+ </copyright>
+
+ <legalnotice>
+ <para>
+ This documentation is free software; you can redistribute
+ it and/or modify it under the terms of the GNU General Public
+ License as published by the Free Software Foundation; either
+ version 2 of the License, or (at your option) any later
+ version.
+ </para>
+
+ <para>
+ 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.
+ </para>
+
+ <para>
+ You should have received a copy of the GNU General Public
+ License along with this program; if not, write to the Free
+ Software Foundation, Inc., 59 Temple Place, Suite 330, Boston,
+ MA 02111-1307 USA
+ </para>
+
+ <para>
+ For more details see the file COPYING in the source
+ distribution of Linux.
+ </para>
+ </legalnotice>
+
+ <releaseinfo>
+ This is the first release of this document coincident with submission of the
+ driver for inclusion in the kernel.
+ </releaseinfo>
+
+ </bookinfo>
+
+ <toc></toc>
+
+ <chapter id="introduction">
+ <title>Introduction</title>
+ <para>
+ This guide documents the cpufreq driver for the IBM PowerPC 750GX processor.
+ It "should" also work with the 750FX but has not been tested.
+ </para>
+ <para>
+ The driver is split into two main parts. The first provides the low level
+ interface to the PLL configuration register (HID1 - SPR 1009). It is called
+ pll_if. The second is the actual cpufreq driver, called cf750gx. The pll_if
+ component handles the details of dealing with the PLL like PLL lock delay
+ requirements and preventing illegal operations. The cf750gx driver provides
+ the interface to the cpufreq subsystem. Under control of the specified
+ governor it will generate the required commands to switch the processor
+ frequency as requested and send them to pll_if to carry them out.
+ </para>
+ </chapter>
+
+ <chapter id="MajorComponents">
+ <title>Major Components</title>
+
+ <para>
+ The IBM 750GX (and FX) processor has a pair of PLLs that can be programmed to
+ operate at a number of different frequencies. The frequencies are specified
+ as ratios to the system bus and range from 2 to 20. From 2 to 10 it also
+ supports half ratios (i.e. 2.5, 3.5) though they are not supported in the
+ cpufreq driver due to a limitation of not being able to switch from one half
+ ratio directly to another. It takes 100 usec for a PLL to relock to a
+ new frequency before it can be used [750GX_ds2-17-05.pdf, Table 3-7,
+ page 18]. It takes 3 bus clocks for the cpu to switch from one PLL to
+ another [750GX_ds2-17-05.pdf, paragraph 3, page 44].
+ </para>
+
+ <para>
+ The cpufreq driver consists of two main parts:
+ </para>
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ cf750gx - the cpufreq driver module
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pll_if - a low level interface that encapsulates the details of dealing
+ with the PLL register
+ </para>
+ </listitem>
+ </itemizedlist>
+
+ <sect1 id="cf750gx">
+ <title>cf750gx - The CPUFreq 750GX driver</title>
+
+ <para>
+ cf750gx provides the standard entry points required of a cpufreq driver. They
+ are:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ cf750gx_verify - verify
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ cf750gx_target - target
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ cf750gx_cpu_init - cpu init
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ cf750gx_cpu_exit - cpu exit
+ </para>
+ </listitem>
+ </itemizedlist>
+
+ These routines perform functions as required by the cpufreq subsystem.
+ </para>
+
+ <para>
+ The driver functions in one of 2 modes: normal and minmax. In normal mode
+ it switches between the available frequencies as requested by the current
+ cpufreq subsystem governor. In minmax mode, it switches between the minimum
+ and maximum frequencies only.
+ </para>
+
+ <sect2 id="cf750gxModesNormal"><title>Normal Mode</title>
+ <para>
+ In normal mode the driver switches between all valid frequencies under
+ control of the selected governor. The list of valid frequencies is
+ determined at startup based on the cpu model (FX or GX) and the determined
+ bus frequency. The cpu model determines the minimum and maximum core
+ frequencies, though these can be overriden at start up via module
+ parameters. See Module Parameters. The bus frequency is obtained from
+ pll_if.
+ </para>
+ </sect2>
+
+ <sect2 id="cf750gxModesMinMax"><title>MinMax Mode</title>
+ <para>
+ In minmax mode the driver switches between the minimum and maximum core
+ frequencies only. This provides the capability for very low latency
+ frequency switches. See Module Parameters and sysfs Attributes.
+ </para>
+ </sect2>
+
+ <sect2 id="cf750gxModuleParameters"><title>Module Parameters</title>
+ <para>
+ cf750gx has 3 module parameters:
+ </para>
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ override_max_core
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ override_min_core
+ </para>
+ </listitem>
+ <listitem>
+ <para>
+ minmaxmode
+ </para>
+ </listitem>
+ </itemizedlist>
+
+ <sect3 id="cf750gxParamOverrideMaxCore"><title>override_max_core</title>
+ <para>
+ Normally the maximum frequency the cpu can be clocked is defined by the
+ cpu model. For the FX it is 800 MHz and 1,000 MHz for the GX. This
+ parameter allows you to override this maximum. It is specified in KHz.
+ </para>
+ </sect3>
+
+ <sect3 id="cf750gxParamOverrideMinCore"><title>override_min_core</title>
+ <para>
+ Likewise for cpu minimum frequency.
+ </para>
+
+ <note>
+ <para>
+ The 750GX on my PowerForce 750GX cpu upgrade seems stable at 250 MHz.
+ The spec says the minimum for the GX is 500 MHz.
+ </para>
+ </note>
+ </sect3>
+
+ <sect3 id="cf750gxParamMinMaxMode"><title>minmaxmode</title>
+ <para>
+ This boolean parameter allows one to enable minmax mode on startup.
+ </para>
+ </sect3>
+ </sect2>
+
+ <sect2 id="cf750gxsysfsAttributes"><title>sysfs Attributes</title>
+ <para>
+ cf750gx has 2 sysfs attributes that can be used to modify its operation.
+ They are:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ idle_pll_off
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ min_max_mode
+ </para>
+ </listitem>
+ </itemizedlist>
+
+ Both are booleans.
+ </para>
+
+ <sect3 id="IdlePllOffAttr"><title>idle_pll_off</title>
+ <para>
+ This boolean tells the driver to switch the idle PLL (the one NOT
+ currently used to drive the cpu core clock) off. Turning the unused PLL
+ off results in a modest power savings [750GX_ds2-17-05.pdf,
+ section 5.2.1, page 44].
+ </para>
+ </sect3>
+
+ <sect3 id="MinMaxModeAttr"><title>min_max_mode</title>
+ <para>
+ This attribute controls the same feature as the similarly named module
+ parameter.
+ </para>
+
+ <note>
+ <para>
+ In order to get this to work with the conservative governor, the
+ "freq_step" attribute must be set to a value that will pick the max
+ frequency from the 2 element frequency table. A good value is 80 (which
+ says to jump in frequency by 80% of the maximum on each change).
+ </para>
+ </note>
+ </sect3>
+ </sect2>
+ </sect1>
+
+ <sect1 id="pll_if">
+ <title>pll_if - The Low Level PLL Interface Module</title>
+ <para>
+ pll_if provides the choice of two interfaces to control the PLL register.
+ One is via a sysfs attribute. The other is through an exported, public
+ routine. At least one interface must be selected during kernel
+ configuration.
+ </para>
+
+ <sect2 id="sysfsInterface"><title>sysfs Interface - ppc750gxpll</title>
+ <para>
+ This interface provides a sysfs attribute that can be written to to
+ directly control the PLL configuration register. The upper 8 or so bits of
+ the PLL configuration register are read only. This interface takes
+ advantage of this by using these bits to define which of the writeable
+ bits contain valid data. The header file
+ <filename class="headerfile">include/asm-powerpc/pll_if.h</filename>
+ defines the following macros:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ PLL0_DO_CFG - PLL 0 config (ratio) valid
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ PLL0_DO_RNG - PLL 0 range valid
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ PLL1_DO_CFG - PLL 1 config (ratio) valid
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ PLL1_DO_RNG - PLL 1 range valid
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ PLL_DO_SEL - PLL selection bit valid
+ </para>
+ </listitem>
+ </itemizedlist>
+
+ See the PERL script pll.pl for an example of using the sysfs attribute.
+ </para>
+ </sect2>
+
+ <sect2 id="PublicAPI"><title>Exported, Public Functions Interface</title>
+ <para>
+ This interface provides a public API that other code can use. The list of
+ functions is:
+
+ <itemizedlist>
+ <listitem>
+ <para>
+ pllif_pack_state
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_get_latency
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_modify_PLL
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_get_bus_clock
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_cfg_to_freq
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_register_pll_switch_cb
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_unregister_pll_switch_cb
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_register_pll_lock_cb
+ </para>
+ </listitem>
+
+ <listitem>
+ <para>
+ pllif_unregister_pll_lock_cb
+ </para>
+ </listitem>
+ </itemizedlist>
+
+
+ The embedded documentation from
+ <filename class="headerfile">include/asm-powerpc/pll_if.h</filename>
+ and
+ <filename class="headerfile">arch/powerpc/kernel/cpu/pll_if.c</filename>
+ is included in the appendix.
+
+ </para>
+ </sect2>
+
+ <sect2 id="pllifModuleParameters"><title>Module Parameters</title>
+ <para>
+ pll_if has one tunable parameter. override_bus_clock allows you to specify
+ the bus frequency. This is useful if the code can not determine the value
+ or gets it wrong. It is in KHz.
+ </para>
+
+ <note>
+ <para>
+ This code was developed on a PowerMac 8600 with a PowerLogix 750GX
+ upgrade card in it. The method used in the code to get the bus frequency
+ is what worked on this system.
+ </para>
+ </note>
+ </sect2>
+ </sect1>
+ </chapter>
+
+ <chapter id="Misc"><title>Misc</title>
+ <sect1 id="pll.pl"><title>pll.pl - A User Friendly pll_if sysfs utility</title>
+ <para>
+ pll.pl is a PERL script that provides a user friendly interface to the
+ pll_if sysfs attribute, cf750gxpll. It provides options to query the
+ current PLL state, change the configuration of either PLL (when inactive),
+ and to switch the PLL in use. See the embedded documentation in pll.pl
+ for details.
+ </para>
+ </sect1>
+ </chapter>
+ <chapter id="Appendix"><title>Appendix</title>
+!Finclude/asm-powerpc/pll_if.h pllifGetLatency pllifPackState
+!Earch/powerpc/kernel/cpu/pll_if.c
+ </chapter>
+</book>
Index: Documentation/DocBook/Makefile
===================================================================
--- Documentation/DocBook/Makefile.orig 2008-08-13 02:18:51.000000000 -0700
+++ Documentation/DocBook/Makefile 2008-08-14 02:47:33.000000000 -0700
@@ -12,7 +12,7 @@ DOCBOOKS := wanbook.xml z8530book.xml mc
kernel-api.xml filesystems.xml lsm.xml usb.xml kgdb.xml \
gadget.xml libata.xml mtdnand.xml librs.xml rapidio.xml \
genericirq.xml s390-drivers.xml uio-howto.xml scsi.xml \
- mac80211.xml debugobjects.xml
+ mac80211.xml debugobjects.xml cf750gx.xml
###
# The build process is as follows (targets):
Index: arch/powerpc/kernel/cpu/cpufreq/Kconfig
===================================================================
--- /dev/null 2004-08-10 18:55:00.000000000 -0700
+++ arch/powerpc/kernel/cpu/cpufreq/Kconfig 2008-08-19 00:24:15.000000000 -0700
@@ -0,0 +1,33 @@
+#
+# CPU Frequency scaling
+#
+
+menu "CPU Frequency scaling"
+
+source "drivers/cpufreq/Kconfig"
+
+if CPU_FREQ
+
+comment "CPUFreq processor drivers"
+
+config CPU_FREQ_PPC_750GX_DUAL_PLL
+ tristate "PowerPC 750 FX/GX Dual PLL driver"
+ depends on !SMP
+ select PPC_750GX_DUAL_PLL_IF
+ select CPU_FREQ_TABLE
+ help
+ This driver adds a CPUFreq driver which utilizes the dual
+ PLLs found in the IBM 750 FX & GX cpus. This will automatically
+ select the pll_if module. It does NOT support SMP.
+
+ To compile this driver as a module, choose M here: the
+ module will be called cf750gx.
+
+ For details, take a look at <file:Documentation/cpu-freq/> and also
+ see the cf750gx DocBook.
+
+ If in doubt, say N.
+
+endif # CPU_FREQ
+
+endmenu
Index: arch/powerpc/kernel/cpu/cpufreq/Makefile
===================================================================
--- /dev/null 2004-08-10 18:55:00.000000000 -0700
+++ arch/powerpc/kernel/cpu/cpufreq/Makefile 2008-08-14 02:44:30.000000000 -0700
@@ -0,0 +1 @@
+obj-$(CONFIG_CPU_FREQ_PPC_750GX_DUAL_PLL) += cf750gx.o
Index: arch/powerpc/kernel/cpu/cpufreq/cf750gx.c
===================================================================
--- /dev/null 2004-08-10 18:55:00.000000000 -0700
+++ arch/powerpc/kernel/cpu/cpufreq/cf750gx.c 2008-08-22 15:54:48.000000000 -0700
@@ -0,0 +1,736 @@
+/*
+ * cf750gx.c - cpufreq driver for the dual PLLs in the 750gx
+ *
+ * Copyright (C) 2008 kevin Diggs
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or (at
+ * your option) any later version.
+ *
+ * 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.
+ *
+ * You should have received a copy of the GNU General Public License along
+ * with this program; if not, write to the Free Software Foundation, Inc.,
+ * 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA.
+ *
+ * ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/sched.h>
+#include <linux/cpufreq.h>
+#include <linux/compiler.h>
+#include <linux/dmi.h>
+#include <linux/delay.h>
+#include "linux/completion.h"
+
+#include <asm/processor.h>
+#include "asm/pll.h"
+#include "asm/pll_if.h"
+#include "asm/time.h"
+
+#define cf750gxmChangingPll (0x80000000)
+#define cf750gxmChangingPllBit (31)
+#define cf750gxmTurningIdlePllOff (0x40000000)
+#define cf750gxmTurningIdlePllOffBit (30)
+
+struct pll_750fgx_t {
+ unsigned short min_ratio; /* min bus ratio */
+ unsigned short max_ratio; /* max bus ratio */
+ unsigned int min_core; /* min core frequency per spec (KHz) */
+ unsigned int max_core; /* max core frequency per spec (KHz) */
+};
+
+#define dprintk(msg...) cpufreq_debug_printk(CPUFREQ_DEBUG_DRIVER, \
+ "ppc-750gx-cpufreq", msg)
+
+MODULE_AUTHOR("Kevin Diggs");
+MODULE_DESCRIPTION("750GX Dual PLL cpufreq driver");
+MODULE_LICENSE("GPL");
+
+struct cf750gx_t_call_data {
+ struct cpufreq_freqs freqs;
+ unsigned long current_pll;
+ int idle_pll_off;
+};
+
+static const struct pll_750fgx_t __initdata pll_750fx = {
+ .min_ratio = 2,
+ .max_ratio = 20,
+ .min_core = 400000,
+ .max_core = 800000,
+};
+
+static const struct pll_750fgx_t __initdata pll_750gx = {
+ .min_ratio = 2,
+ .max_ratio = 20,
+ .min_core = 500000,
+ .max_core = 1000000,
+};
+
+static DECLARE_COMPLETION(cf750gx_v_exit_completion);
+
+static unsigned int override_min_core = 0;
+static unsigned int override_max_core = 0;
+static unsigned int minmaxmode = 0;
+
+static unsigned int cf750gx_v_min_core = 0;
+static unsigned int cf750gx_v_max_core = 0;
+static int cf750gx_v_idle_pll_off = 0;
+static int cf750gx_v_min_max_mode = 0;
+static unsigned long cf750gx_v_state_bits = 0;
+
+static struct cpufreq_frequency_table *cf750gx_v_f_table;
+static struct cpufreq_frequency_table *cf750gx_v_freq_table;
+static struct cpufreq_frequency_table *cf750gx_v_min_max_freq_table;
+
+static struct cf750gx_t_call_data cf750gx_v_switch_call_data;
+static struct cf750gx_t_call_data cf750gx_v_lock_call_data;
+static struct notifier_block cf750gx_v_pll_switch_nb;
+static struct notifier_block cf750gx_v_pll_lock_nb;
+
+static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
+ action, void *data)
+{
+struct cf750gx_t_call_data *cd;
+unsigned int idle_pll;
+unsigned int pll_off_cmd;
+unsigned int new_pll;
+
+ cd = (struct cf750gx_t_call_data *)data;
+
+ dprintk("%s(): cd->idle_pll_off is %d, new PLL is 0x%x"
+ ", Switched to %d KHz\n", __func__, cd->idle_pll_off,
+ cd->current_pll, cd->freqs.new);
+
+ cpufreq_notify_transition(&cd->freqs, CPUFREQ_POSTCHANGE);
+
+ if (cd->idle_pll_off) {
+ /*
+ * Assemble call to turn idle PLL off. Use the value of
+ * current_pll in the call data thing.
+ */
+ idle_pll = get_next_PLL(cd->current_pll)^0x1;
+
+ pll_off_cmd = pllif_pack_state(0, 2);
+
+ /*
+ * Put in correct spot
+ */
+ if (idle_pll) {
+ /*
+ * Use PLL 1
+ */
+ new_pll = ((PLL1_DO_CFG|PLL1_DO_RNG)<<24);
+ } else {
+ /*
+ * Since active PLL is 1, switch off PLL 0
+ */
+ pll_off_cmd = pll_off_cmd<<(PLL0_CFG_SHIFT-
+ PLL1_CFG_SHIFT);
+
+ new_pll = (PLL0_DO_CFG|PLL0_DO_RNG)<<24;
+ }
+
+ new_pll = new_pll|pll_off_cmd;
+
+ set_bit(cf750gxmTurningIdlePllOffBit, &cf750gx_v_state_bits);
+
+ (void) pllif_modify_PLL(new_pll, 0);
+ } else {
+ clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
+
+ complete(&cf750gx_v_exit_completion);
+ }
+
+ return NOTIFY_OK;
+}
+
+static int cf750gx_pll_lock_cb(struct notifier_block *nb, unsigned long action,
+ void *data)
+{
+struct cf750gx_t_call_data *cd;
+
+ cd = (struct cf750gx_t_call_data *)data;
+
+ dprintk("%s(): cd->idle_pll_off is %d, new PLL is 0x%x"
+ ", Switched to %d KHz\n", __func__, cd->idle_pll_off,
+ cd->current_pll, cd->freqs.new);
+
+ if (cd->idle_pll_off) {
+ /*
+ * We want to turn off the inactive (idle) pll. Use the state
+ * bits global to figure out whether this call back is for the
+ * original, cpufreq ordered frequency change or our turning
+ * off of the idle PLL.
+ */
+ if (test_bit(cf750gxmTurningIdlePllOffBit,
+ &cf750gx_v_state_bits)) {
+ clear_bit(cf750gxmTurningIdlePllOffBit,
+ &cf750gx_v_state_bits);
+ clear_bit(cf750gxmChangingPllBit,
+ &cf750gx_v_state_bits);
+ complete(&cf750gx_v_exit_completion);
+ }
+ }
+
+ return NOTIFY_OK;
+}
+
+static inline const char *cf750gx_i_relation_str(unsigned int relation)
+{
+ switch (relation) {
+ case CPUFREQ_RELATION_L: return "CPUFREQ_RELATION_L";
+ case CPUFREQ_RELATION_H: return "CPUFREQ_RELATION_H";
+ default: return "relation ??";
+ }
+}
+
+static int cf750gx_target(struct cpufreq_policy *policy,
+ unsigned int target_freq, unsigned int relation)
+{
+unsigned int next_index = 0; /* Index into freq_table */
+unsigned int next_freq = 0; /* next frequency from perf table */
+unsigned int next_perf_state = 0; /* Index from perf table */
+int result = 0;
+unsigned int pll;
+unsigned int new_pll;
+unsigned int active_pll;
+struct cpufreq_freqs freqs;
+struct cpufreq_frequency_table *ft = cf750gx_v_f_table;
+
+ dprintk(__FILE__">%s(, %u KHz, relation %u)-%d: on cpu %d\n",
+ __func__, target_freq, relation, __LINE__, policy->cpu);
+
+ if (test_and_set_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits))
+ return -EAGAIN;
+
+ INIT_COMPLETION(cf750gx_v_exit_completion);
+
+ result = cpufreq_frequency_table_target(policy,
+ ft,
+ target_freq,
+ relation, &next_index);
+
+ if (unlikely(result))
+ goto cf750gxTargetNoFreq;
+
+ next_perf_state = ft[next_index].index;
+ next_freq = ft[next_index].frequency;
+ dprintk(__FILE__">%s()-%d: %d KHz (state=%x) selected from table "
+ "(%s)\n", __func__, __LINE__, next_freq, next_perf_state,
+ cf750gx_i_relation_str(relation));
+
+ pll = get_PLL();
+ active_pll = get_active_PLL(pll);
+
+#if 0
+#ifdef CONFIG_HOTPLUG_CPU
+ /* cpufreq holds the hotplug lock, so we are safe from here on */
+ cpus_and(online_policy_cpus, cpu_online_map, policy->cpus);
+#else
+ online_policy_cpus = policy->cpus;
+#endif
+#endif
+
+ if (pllif_pack_state(get_PLL_ratio(active_pll, pll), get_PLL_range(
+ active_pll, pll)) == next_perf_state) {
+ dprintk("Already at target state (%x)\n",
+ next_perf_state);
+ goto cf750gxTargetFreqSet;
+ }
+
+/* cpus_clear(cmd.mask); */
+
+ dprintk(__FILE__">%s()-%d: Current PLL: 0x%x\n", __func__, __LINE__,
+ pll);
+
+ if (active_pll) {
+ unsigned int current_state;
+
+ /*
+ * Since active PLL is 1, modify PLL 0. First see if it is
+ * already where we want it.
+ */
+ current_state = pllif_pack_state(get_PLL_ratio(0, pll),
+ get_PLL_range(0, pll));
+
+ dprintk(__FILE__">%s()-%d: pll 0 current state: 0x%x\n",
+ __func__, __LINE__, current_state);
+
+ if (current_state == next_perf_state) {
+ new_pll = PLL_DO_SEL<<24;
+ next_perf_state = 0;
+ } else {
+ next_perf_state = next_perf_state<<(PLL0_CFG_SHIFT-
+ PLL1_CFG_SHIFT);
+
+ new_pll = (PLL0_DO_CFG|PLL0_DO_RNG|PLL_DO_SEL)<<24;
+ }
+ } else {
+ unsigned int current_state;
+
+ /*
+ * Use PLL 1. Again, see if it is already at the desired
+ * configuration.
+ */
+ current_state = pllif_pack_state(get_PLL_ratio(1, pll),
+ get_PLL_range(1, pll));
+
+ dprintk(__FILE__">%s()-%d: pll 1 current state: 0x%x\n",
+ __func__, __LINE__, current_state);
+
+ if (current_state == next_perf_state) {
+ new_pll = (PLL_DO_SEL<<24)|PLL_SEL_MASK;
+ next_perf_state = 0;
+ } else {
+ new_pll = ((PLL1_DO_CFG|PLL1_DO_RNG|PLL_DO_SEL)<<24)|
+ PLL_SEL_MASK;
+ }
+ }
+
+ new_pll = new_pll|next_perf_state;
+
+ dprintk(__FILE__">%s()-%d: Modifying PLL: 0x%x\n", __func__, __LINE__,
+ new_pll);
+
+ freqs.old = pllif_cfg_to_freq(get_PLL_ratio(active_pll, pll))/1000;
+ freqs.new = ft[next_index].frequency;
+ freqs.cpu = 0;
+
+ cf750gx_v_switch_call_data.freqs = freqs;
+ cf750gx_v_switch_call_data.current_pll = new_pll;
+ cf750gx_v_switch_call_data.idle_pll_off = cf750gx_v_idle_pll_off;
+
+ cf750gx_v_lock_call_data.freqs = freqs;
+ cf750gx_v_lock_call_data.current_pll = new_pll;
+ cf750gx_v_lock_call_data.idle_pll_off = cf750gx_v_switch_call_data.
+ idle_pll_off;
+
+ dprintk(__FILE__">%s()-%d: freqs.old=%d, freqs.new=%d\n", __func__,
+ __LINE__, freqs.old, freqs.new);
+
+ cpufreq_notify_transition(&freqs, CPUFREQ_PRECHANGE);
+
+ result = pllif_modify_PLL(new_pll, 0);
+
+ if (result < 0)
+ goto cf750gxTargetUnlock;
+
+cf750gxTargetOut:
+ return result;
+
+cf750gxTargetNoFreq:
+ result = -ENODEV;
+
+ goto cf750gxTargetUnlock;
+cf750gxTargetFreqSet:
+ result = 0;
+
+ goto cf750gxTargetUnlock;
+cf750gxTargetUnlock:
+ clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
+ complete(&cf750gx_v_exit_completion);
+
+ goto cf750gxTargetOut;
+}
+
+static int cf750gx_verify(struct cpufreq_policy *policy)
+{
+ dprintk("%s()\n", __func__);
+
+ return cpufreq_frequency_table_verify(policy, cf750gx_v_f_table);
+}
+
+static int cf750gx_cpu_init(struct cpufreq_policy *policy)
+{
+unsigned int pll;
+unsigned int result = 0;
+
+ dprintk("%s()\n", __func__);
+
+ dprintk("policy->policy is %s (%d)\n", (policy->policy ==
+ CPUFREQ_POLICY_POWERSAVE || policy->policy ==
+ CPUFREQ_POLICY_PERFORMANCE)?(policy->policy ==
+ CPUFREQ_POLICY_POWERSAVE?"powersave":"performance"):
+ "undefined", policy->policy);
+
+ pll = get_PLL();
+
+ policy->cur = pllif_cfg_to_freq(get_PLL_ratio(get_active_PLL(pll),
+ pll))/1000;
+
+/* policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; */
+ policy->cpuinfo.transition_latency = pllif_get_latency();
+
+ result = cpufreq_frequency_table_cpuinfo(policy, cf750gx_v_f_table);
+ if (result)
+ goto err_freqfree;
+
+ cpufreq_frequency_table_get_attr(cf750gx_v_f_table, policy->cpu);
+
+ cf750gx_v_pll_switch_nb.notifier_call = cf750gx_pll_switch_cb;
+ cf750gx_v_pll_switch_nb.next = (struct notifier_block *)
+ &cf750gx_v_switch_call_data;
+ cf750gx_v_pll_switch_nb.priority = 0;
+
+ result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
+
+ cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
+ cf750gx_v_pll_lock_nb.next =
+ (struct notifier_block *)&cf750gx_v_lock_call_data;
+ cf750gx_v_pll_lock_nb.priority = 0;
+
+ result = pllif_register_pll_lock_cb(&cf750gx_v_pll_lock_nb);
+
+ return result;
+
+err_freqfree:
+ return result;
+}
+
+static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
+{
+ dprintk("%s()\n", __func__);
+
+ /*
+ * Wait for any active requests to ripple through before exiting
+ */
+ wait_for_completion(&cf750gx_v_exit_completion);
+
+ cpufreq_frequency_table_put_attr(policy->cpu);
+
+ pllif_unregister_pll_switch_cb(&cf750gx_v_pll_switch_nb);
+
+ pllif_unregister_pll_lock_cb(&cf750gx_v_pll_lock_nb);
+
+ cf750gx_v_pll_switch_nb.notifier_call = NULL;
+ cf750gx_v_pll_switch_nb.next = NULL;
+
+ cf750gx_v_pll_lock_nb.notifier_call = NULL;
+ cf750gx_v_pll_lock_nb.next = NULL;
+
+ return 0;
+}
+
+static int cf750gx_resume(struct cpufreq_policy *policy)
+{
+ return 0;
+}
+
+/*
+ * cf750gx_i_show_idle_pll_off - Show state of idle pll off boolean
+ */
+static ssize_t cf750gx_i_show_idle_pll_off(struct cpufreq_policy *policy,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", cf750gx_v_idle_pll_off);
+}
+
+static ssize_t cf750gx_i_store_pll_off(struct cpufreq_policy *policy,
+ const char *buf, size_t count)
+{
+ if (buf[0] == '0')
+ cf750gx_v_idle_pll_off = 0;
+ else if (buf[0] == '1')
+ cf750gx_v_idle_pll_off = !0;
+
+ return 1;
+}
+
+static struct freq_attr idle_pll_off = {
+ .attr = { .name = "idle_pll_off",
+ .mode = 0644,
+ },
+ .show = cf750gx_i_show_idle_pll_off,
+ .store = cf750gx_i_store_pll_off,
+};
+
+/*
+ * cf750gx_i_show_min_max_mode - Show state of min max mode boolean
+ */
+static ssize_t cf750gx_i_show_min_max_mode(struct cpufreq_policy *policy,
+ char *buf)
+{
+ return sprintf(buf, "%d\n", cf750gx_v_min_max_mode);
+}
+
+static ssize_t cf750gx_i_store_min_max_mode(struct cpufreq_policy *policy,
+ const char *buf, size_t count)
+{
+int result;
+
+ if (buf[0] == '0') {
+ cf750gx_v_min_max_mode = 0;
+
+ cf750gx_v_f_table = cf750gx_v_freq_table;
+
+ cpufreq_frequency_table_put_attr(policy->cpu);
+
+ cpufreq_frequency_table_get_attr(cf750gx_v_freq_table,
+ policy->cpu);
+
+ result = cpufreq_frequency_table_cpuinfo(policy,
+ cf750gx_v_freq_table);
+ } else if (buf[0] == '1') {
+ cf750gx_v_min_max_mode = !0;
+
+ cf750gx_v_f_table = cf750gx_v_min_max_freq_table;
+
+ cpufreq_frequency_table_put_attr(policy->cpu);
+
+ cpufreq_frequency_table_get_attr(cf750gx_v_min_max_freq_table,
+ policy->cpu);
+
+ result = cpufreq_frequency_table_cpuinfo(policy,
+ cf750gx_v_min_max_freq_table);
+ }
+
+ return 1;
+}
+
+static struct freq_attr min_max_mode = {
+ .attr = { .name = "min_max_mode",
+ .mode = 0644,
+ },
+ .show = cf750gx_i_show_min_max_mode,
+ .store = cf750gx_i_store_min_max_mode,
+};
+
+
+static struct freq_attr *cf750gxvAttr[] = {
+ &cpufreq_freq_attr_scaling_available_freqs,
+ &idle_pll_off,
+ &min_max_mode,
+ NULL,
+};
+
+static struct cpufreq_driver cf750gx_v_drv = {
+ .verify = cf750gx_verify,
+ .target = cf750gx_target,
+ .init = cf750gx_cpu_init,
+ .exit = cf750gx_cpu_exit,
+ .resume = cf750gx_resume,
+ .name = "ppc750gx-cpufreq",
+ .owner = THIS_MODULE,
+ .attr = cf750gxvAttr,
+};
+
+static int __init cf750gx_init(void)
+{
+int ret;
+unsigned int freq, i, j, rng, bus_clock;
+unsigned short min_ratio, max_ratio;
+struct cpufreq_frequency_table *tbp;
+const struct pll_750fgx_t *pll_defaults;
+
+ dprintk("%s()\n", __func__);
+
+ if (!cpu_has_feature(CPU_FTR_DUAL_PLL_750FX))
+ return 0;
+
+ /*
+ * Get processor min and max core frequencies. If they have not been
+ * overriden, then get them from version defaults.
+ */
+ if ((cur_cpu_spec->pvr_value>>16) == 0x7000)
+ pll_defaults = &pll_750fx;
+ else
+ pll_defaults = &pll_750gx;
+
+ cf750gx_v_min_core = override_min_core?override_min_core:pll_defaults->
+ min_core;
+ cf750gx_v_max_core = override_max_core?override_max_core:pll_defaults->
+ max_core;
+
+ dprintk(__FILE__">%s()-%d: cf750gx_v_min_core is %u, "
+ "cf750gx_v_max_core is %u\n", __func__, __LINE__,
+ cf750gx_v_min_core, cf750gx_v_max_core);
+ dprintk(__FILE__">%s()-%d: pll_defaults: min_ratio %d, max_ratio "
+ "%d\n", __func__, __LINE__, pll_defaults->min_ratio,
+ pll_defaults->max_ratio);
+
+ if (!cf750gx_v_min_core) {
+ dprintk("Can't determine minimum core frequency\n");
+ ret = -EINVAL;
+ goto ErrSimple;
+ }
+
+ if (!cf750gx_v_max_core) {
+ dprintk("Can't determine maximum core frequency\n");
+ ret = -EINVAL;
+ goto ErrSimple;
+ }
+
+ bus_clock = pllif_get_bus_clock()/1000;
+
+ /*
+ * Build maximum freq table. This will depend on the bus freq, the core
+ * frequency limits, and the ratios.
+ */
+ min_ratio = pll_defaults->min_ratio;
+ freq = min_ratio*bus_clock;
+
+ if (freq < cf750gx_v_min_core) {
+ /*
+ * Core min is above min ratio and bus speed clock. Find min
+ * ratio such that min ratio * bus speed >= core min.
+ */
+ min_ratio = cf750gx_v_min_core/bus_clock;
+ j = cf750gx_v_min_core%bus_clock;
+
+ if (j)
+ min_ratio++;
+ } else {
+ /*
+ * Core min is below min ratio and speed clock. Reset core min.
+ */
+ cf750gx_v_min_core = freq;
+ }
+
+ max_ratio = pll_defaults->max_ratio;
+ freq = max_ratio*bus_clock;
+
+ if (freq > cf750gx_v_max_core) {
+ /*
+ * Core max is below max ratio and bus speed. Find max ratio
+ * such that max ratio * bus speed <= core max.
+ */
+ max_ratio = cf750gx_v_max_core/bus_clock;
+ } else {
+ /*
+ * Core max is above max ratio and bus speed clock. Reset core
+ * max.
+ */
+ cf750gx_v_max_core = freq;
+ }
+
+ dprintk(__FILE__">%s()-%d: min_ratio is %d, max_ratio is %d\n",
+ __func__, __LINE__, min_ratio, max_ratio);
+
+ /*
+ * Bus ratios for the GX range from 2-20 for 19 INTEGER frequencies.
+ * The above checks may have changed this. There are max_ratio -
+ * min_ratio + 1 different frequencies.
+ */
+ j = max_ratio-min_ratio+1;
+ cf750gx_v_freq_table = kmalloc(sizeof(struct cpufreq_frequency_table)*
+ (j+5), GFP_KERNEL);
+ if (cf750gx_v_freq_table == NULL) {
+ ret = -ENOMEM;
+ goto ErrSimple;
+ }
+
+ dprintk(__FILE__">%s()-%d: cf750gx_v_freq_table=%p\n", __func__,
+ __LINE__, cf750gx_v_freq_table);
+
+ /*
+ * Use index of first entry to keep track of the count (one extra
+ * entry)
+ */
+ cf750gx_v_freq_table[0].frequency = CPUFREQ_ENTRY_INVALID;
+ cf750gx_v_freq_table[0].index = j;
+
+ /*
+ * Populate the table
+ */
+ for (tbp = cf750gx_v_freq_table+1, i = min_ratio; i <= max_ratio;
+ tbp++, i++) {
+ tbp->frequency = i*bus_clock;
+
+ /*
+ * I really don't know what the proper range values for 600 MHz
+ * and 900 MHz are? (The processor may also get pretty mad if
+ * you muck these up!)
+ */
+ if (tbp->frequency < 600000)
+ rng = 2;
+ else if (tbp->frequency < 900000)
+ rng = 0;
+ else
+ rng = 1;
+
+ /*
+ * The computation in the first argument converts the bus
+ * ratio value to the PLL configuration value needed for the
+ * given ratio
+ */
+ tbp->index = pllif_pack_state(i > 10?i+10:(i<<1), rng);
+
+ dprintk(__FILE__">%s()-%d: p->index=%x, ratio=%u, rng=%u\n",
+ __func__, __LINE__, tbp->index, i, rng);
+ }
+
+ /*
+ * 2nd extra array member
+ */
+ tbp->frequency = CPUFREQ_TABLE_END;
+
+ /*
+ * The other 3 extra array members are for the min max table.
+ */
+ cf750gx_v_min_max_freq_table = cf750gx_v_freq_table+
+ cf750gx_v_freq_table[0].index + 2;
+
+ cf750gx_v_min_max_freq_table[0] = cf750gx_v_freq_table[1];
+ cf750gx_v_min_max_freq_table[1] = cf750gx_v_freq_table[
+ cf750gx_v_freq_table[0].index];
+ cf750gx_v_min_max_freq_table[2].frequency = CPUFREQ_TABLE_END;
+
+ cf750gx_v_f_table = minmaxmode?cf750gx_v_min_max_freq_table:
+ cf750gx_v_freq_table;
+
+ ret = cpufreq_register_driver(&cf750gx_v_drv);
+
+ dprintk(__FILE__">%s()-%d: return from cpufreq_register_driver() is "
+ "%d\n", __func__, __LINE__, ret);
+
+ if (ret)
+ goto ErrFreqTable;
+
+ErrSimple:
+ return ret;
+ErrFreqTable:
+ kfree(cf750gx_v_freq_table);
+ return ret;
+}
+
+static void __exit cf750gx_exit(void)
+{
+ dprintk("%s()\n", __func__);
+
+ cpufreq_unregister_driver(&cf750gx_v_drv);
+
+ if (cf750gx_v_freq_table)
+ kfree(cf750gx_v_freq_table);
+
+ cf750gx_v_freq_table = cf750gx_v_min_max_freq_table =
+ cf750gx_v_f_table = NULL;
+
+ return;
+}
+
+module_param(override_max_core, uint, 0644);
+MODULE_PARM_DESC(override_max_core,
+ "clock frequency in KHz.");
+
+module_param(override_min_core, uint, 0644);
+MODULE_PARM_DESC(override_min_core,
+ "clock frequency in KHz.");
+
+module_param(minmaxmode, uint, 0644);
+MODULE_PARM_DESC(minmaxmode,
+ "Use only the minimum and maximum frequencies.");
+
+late_initcall(cf750gx_init);
+module_exit(cf750gx_exit);
+
+MODULE_ALIAS("dual-pll");
Index: arch/powerpc/kernel/cpu/Makefile
===================================================================
--- /dev/null 2004-08-10 18:55:00.000000000 -0700
+++ arch/powerpc/kernel/cpu/Makefile 2008-08-14 02:44:30.000000000 -0700
@@ -0,0 +1,6 @@
+#
+# Makefile for ppc CPU details and quirks
+#
+
+obj-$(CONFIG_CPU_FREQ) += cpufreq/
+obj-$(CONFIG_PPC_750GX_DUAL_PLL_IF) += pll_if.o
On Monday 25 August 2008, Kevin Diggs wrote:
> + * cf750gx.c - cpufreq driver for the dual PLLs in the 750gx
Thanks for posting this driver and for your attention for detail
and for documentation in particular. Few people bother to write
documentation at this level.
I don't understand enough of cpufreq or your hardware to comment
on that, but please let me give you a few hints on coding style.
> + * ?Copyright (C) 2008 ? ? ? kevin Diggs
Most people list their email address here as well
> +#define cf750gxmChangingPll????????????(0x80000000)
> +#define cf750gxmChangingPllBit?????????(31)
> +#define cf750gxmTurningIdlePllOff??????(0x40000000)
> +#define cf750gxmTurningIdlePllOffBit???(30)
constants should be ALL_CAPS, not sIllYCaPS.
> +struct pll_750fgx_t {
> +???????unsigned short min_ratio;???????/* min bus ratio */
> +???????unsigned short max_ratio;???????/* max bus ratio */
> +???????unsigned int min_core;??????????/* min core frequency per spec (KHz) */
> +???????unsigned int max_core;??????????/* max core frequency per spec (KHz) */
> +};
please drop the _t at the end of the identifier.
> +MODULE_AUTHOR("Kevin Diggs");
> +MODULE_DESCRIPTION("750GX Dual PLL cpufreq driver");
> +MODULE_LICENSE("GPL");
Move this to the end.
> +struct cf750gx_t_call_data {
> +???????struct cpufreq_freqs freqs;
> +???????unsigned long current_pll;
> +???????int idle_pll_off;
> +};
drop the _t here, or make explicit what is meant by it.
> +static const struct pll_750fgx_t __initdata pll_750fx = {
> +???????.min_ratio = 2,
> +???????.max_ratio = 20,
> +???????.min_core = 400000,
> +???????.max_core = 800000,
> +};
> +
> +static const struct pll_750fgx_t __initdata pll_750gx = {
> +???????.min_ratio = 2,
> +???????.max_ratio = 20,
> +???????.min_core = 500000,
> +???????.max_core = 1000000,
> +};
Are these correct on any board? If they can be different
depending on the board design, it would be better to get
this data from the device tree.
> +static DECLARE_COMPLETION(cf750gx_v_exit_completion);
> +
> +static unsigned int override_min_core = 0;
> +static unsigned int override_max_core = 0;
> +static unsigned int minmaxmode = 0;
> +
> +static unsigned int cf750gx_v_min_core = 0;
> +static unsigned int cf750gx_v_max_core = 0;
> +static int cf750gx_v_idle_pll_off = 0;
> +static int cf750gx_v_min_max_mode = 0;
> +static unsigned long cf750gx_v_state_bits = 0;
Is 0 a meaningful value for these? If it just means 'uninitialized',
then better don't initialize them in the first place, for clarity.
> +static struct cpufreq_frequency_table *cf750gx_v_f_table;
> +static struct cpufreq_frequency_table *cf750gx_v_freq_table;
> +static struct cpufreq_frequency_table *cf750gx_v_min_max_freq_table;
> +
> +static struct cf750gx_t_call_data cf750gx_v_switch_call_data;
> +static struct cf750gx_t_call_data cf750gx_v_lock_call_data;
> +static struct notifier_block cf750gx_v_pll_switch_nb;
> +static struct notifier_block cf750gx_v_pll_lock_nb;
Also, in general, try to avoid global variables here, even
in file scope (static), but rather put all device specific
data into a per-device data structure.
> +static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
> +???????action, void *data)
> +{
> +struct cf750gx_t_call_data *cd;
> +unsigned int idle_pll;
> +unsigned int pll_off_cmd;
> +unsigned int new_pll;
The whitespace appears damaged here.
> +???????cd = (struct cf750gx_t_call_data *)data;
data is a void pointer, so you don't need the cast, and shouldn't
have it therefore.
> +static int cf750gx_pll_lock_cb(struct notifier_block *nb, unsigned long action,
> +???????void *data)
> +{
> +struct cf750gx_t_call_data *cd;
> +
> +???????cd = (struct cf750gx_t_call_data *)data;
same here.
> +static int cf750gx_target(struct cpufreq_policy *policy,
> +??????????????????????? ? ? ? unsigned int target_freq, unsigned int relation)
> +{
> +unsigned int next_index = 0;???????????/* Index into freq_table */
> +unsigned int next_freq = 0;????????????/* next frequency from perf table */
> +unsigned int next_perf_state = 0;??????/* Index from perf table */
> +int result = 0;
Don't initialize local variables in the declaration, as that will prevent
the compiler from warning about uninitialized use.
> +unsigned int pll;
> +unsigned int new_pll;
> +unsigned int active_pll;
> +struct cpufreq_freqs freqs;
> +struct cpufreq_frequency_table *ft = cf750gx_v_f_table;
more whitespace damage. Maybe there is something wrong with your
text editor.
> +???????dprintk(__FILE__">%s(, %u KHz, relation %u)-%d: ?on cpu %d\n",
> +???????????????__func__, target_freq, relation, __LINE__, policy->cpu);
> +
> +???????if (test_and_set_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits))
> +???????????????return -EAGAIN;
> +
> +???????INIT_COMPLETION(cf750gx_v_exit_completion);
> +
> +???????result = cpufreq_frequency_table_target(policy,
> +???????????????????????????????????????????????ft,
> +???????????????????????????????????????????????target_freq,
> +???????????????????????????????????????????????relation, &next_index);
> +
> +???????if (unlikely(result))
> +???????????????goto cf750gxTargetNoFreq;
The unlikely() here looks like overoptimization, drop it in favor of
readability unless you can measure a real-world difference.
> +???????if (active_pll) {
> +???????unsigned int current_state;
whitespace damage.
> +???????dprintk(__FILE__">%s()-%d: ?Modifying PLL: ?0x%x\n", __func__, __LINE__,
> +???????????????new_pll);
Please go through all your dprintk and see if you really really need all of them.
Usually they are useful while you are doing the initial code, but only get in the
way as soon as it starts working.
> +cf750gxTargetOut:
> +???????return result;
> +
> +cf750gxTargetNoFreq:
> +???????result = -ENODEV;
> +
> +???????goto cf750gxTargetUnlock;
> +cf750gxTargetFreqSet:
> +???????result = 0;
> +
> +???????goto cf750gxTargetUnlock;
> +cf750gxTargetUnlock:
> +???????clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
> +???????complete(&cf750gx_v_exit_completion);
> +
> +???????goto cf750gxTargetOut;
The conventional way to write this would be:
result = -ENODEV;
if (foo)
goto out_unlock;
result = 0;
if (bar)
goto out_unlock;
return 0;
out_unlock:
clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
complete(&cf750gx_v_exit_completion);
out:
return result;
> +/*?????policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; */
> +???????policy->cpuinfo.transition_latency = pllif_get_latency();
The comment does not really explain anything. If you just want to disable
code, use #if 0, but better drop it right away and add a comment about
what might need changing.
> +???????result = cpufreq_frequency_table_cpuinfo(policy, cf750gx_v_f_table);
> +???????if (result)
> +???????????????goto err_freqfree;
> +
> +???????cpufreq_frequency_table_get_attr(cf750gx_v_f_table, policy->cpu);
> +
> +???????cf750gx_v_pll_switch_nb.notifier_call = cf750gx_pll_switch_cb;
> +???????cf750gx_v_pll_switch_nb.next = (struct notifier_block *)
> +???????????????&cf750gx_v_switch_call_data;
> +???????cf750gx_v_pll_switch_nb.priority = 0;
> +
> +???????result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
> +
> +???????cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
> +???????cf750gx_v_pll_lock_nb.next =
> +???????????????(struct notifier_block *)&cf750gx_v_lock_call_data;
These casts look wrong, cf750gx_v_lock_call_data is not a notifier_block.
What are you trying to do here?
> +???????cf750gx_v_pll_lock_nb.priority = 0;
> +
> +???????result = pllif_register_pll_lock_cb(&cf750gx_v_pll_lock_nb);
> +
> +???????return result;
> +
> +err_freqfree:
> +???????return result;
> +}
The first 'return result' is redundant, drop it.
> +
> +static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
> +{
> +???????dprintk("%s()\n", __func__);
> +
> +???????/*
> +??????? * Wait for any active requests to ripple through before exiting
> +??????? */
> +???????wait_for_completion(&cf750gx_v_exit_completion);
This "wait for anything" use of wait_for_completion looks wrong,
because once any other function has done the 'complete', you won't
wait here any more.
What exactly are you trying to accomplish with this?
> +static int __init cf750gx_init(void)
> +{
> +int ret;
> +unsigned int freq, i, j, rng, bus_clock;
> +unsigned short min_ratio, max_ratio;
> +struct cpufreq_frequency_table *tbp;
> +const struct pll_750fgx_t *pll_defaults;
whitespace.
> +???????dprintk("%s()\n", __func__);
> +
> +???????if (!cpu_has_feature(CPU_FTR_DUAL_PLL_750FX))
> +???????????????return 0;
Is this purely a feature of the CPU or does it need logic
in the board design? If you need external hardware for it,
you need to check the device tree for the presence of that
hardware.
> +???????if (cf750gx_v_freq_table == NULL) {
> +???????????????ret = -ENOMEM;
> +???????????????goto ErrSimple;
> +???????}
ret = -ENOMEM;
if (!cf750gx_freq_table)
goto err_simple;
Arnd <><
Arnd Bergmann wrote:
> On Monday 25 August 2008, Kevin Diggs wrote:
>
>>+ * cf750gx.c - cpufreq driver for the dual PLLs in the 750gx
>
>
> Thanks for posting this driver and for your attention for detail
> and for documentation in particular. Few people bother to write
> documentation at this level.
>
> I don't understand enough of cpufreq or your hardware to comment
> on that, but please let me give you a few hints on coding style.
>
>
>>+ * Copyright (C) 2008 kevin Diggs
>
>
> Most people list their email address here as well
>
For reasons I'd rather not go into, my email address is not likely
to remain valid for much longer.
>
>>+#define cf750gxmChangingPll (0x80000000)
>>+#define cf750gxmChangingPllBit (31)
>>+#define cf750gxmTurningIdlePllOff (0x40000000)
>>+#define cf750gxmTurningIdlePllOffBit (30)
>
>
> constants should be ALL_CAPS, not sIllYCaPS.
>
Are cf750gxm_CHANGING_PLL and cf750gxm_CHANGING_PLL_BIT_POS ok?
>
>>+struct pll_750fgx_t {
>>+ unsigned short min_ratio; /* min bus ratio */
>>+ unsigned short max_ratio; /* max bus ratio */
>>+ unsigned int min_core; /* min core frequency per spec (KHz) */
>>+ unsigned int max_core; /* max core frequency per spec (KHz) */
>>+};
>
>
> please drop the _t at the end of the identifier.
>
done
>
>>+MODULE_AUTHOR("Kevin Diggs");
>>+MODULE_DESCRIPTION("750GX Dual PLL cpufreq driver");
>>+MODULE_LICENSE("GPL");
>
>
> Move this to the end.
>
done
>
>>+struct cf750gx_t_call_data {
>>+ struct cpufreq_freqs freqs;
>>+ unsigned long current_pll;
>>+ int idle_pll_off;
>>+};
>
>
> drop the _t here, or make explicit what is meant by it.
>
Now that I look at it the _t is supposed to be at the end. It is
meant to indicate that this is a structure tag or type. I'll
remove it.
>
>>+static const struct pll_750fgx_t __initdata pll_750fx = {
>>+ .min_ratio = 2,
>>+ .max_ratio = 20,
>>+ .min_core = 400000,
>>+ .max_core = 800000,
>>+};
>>+
>>+static const struct pll_750fgx_t __initdata pll_750gx = {
>>+ .min_ratio = 2,
>>+ .max_ratio = 20,
>>+ .min_core = 500000,
>>+ .max_core = 1000000,
>>+};
>
>
> Are these correct on any board? If they can be different
> depending on the board design, it would be better to get
> this data from the device tree.
>
They are a spceification of the processor itself. Should be
the same for any board using the 750GX (or FX).
>
>>+static DECLARE_COMPLETION(cf750gx_v_exit_completion);
>>+
>>+static unsigned int override_min_core = 0;
>>+static unsigned int override_max_core = 0;
>>+static unsigned int minmaxmode = 0;
>>+
>>+static unsigned int cf750gx_v_min_core = 0;
>>+static unsigned int cf750gx_v_max_core = 0;
>>+static int cf750gx_v_idle_pll_off = 0;
>>+static int cf750gx_v_min_max_mode = 0;
>>+static unsigned long cf750gx_v_state_bits = 0;
>
>
> Is 0 a meaningful value for these? If it just means 'uninitialized',
> then better don't initialize them in the first place, for clarity.
>
The first 3 are module parameters. For the first 2, 0 means
that they were not set. minmaxmode is a boolean. 0 is the
default of disabled.
When I was initially writing the code I figured I would
need the min and max core frequencies in several places.
As it turns out they are only used in the code
initialization routine (cf750gx_init()). I have made
them locals.
..._idle_pll_off is a boolean for a sysfs attribute. 0 is
the default of disabled.
..._min_max_mode is a boolean to hold the state of
minmaxmode. Seems to be only used to print the current
value.
..._state_bits is a global to maintain state.
Does the PowerPC suffer any performance penalties when
accessing shorts compared to ints? Can I save a few bytes
by using shorts?
>
>>+static struct cpufreq_frequency_table *cf750gx_v_f_table;
>>+static struct cpufreq_frequency_table *cf750gx_v_freq_table;
>>+static struct cpufreq_frequency_table *cf750gx_v_min_max_freq_table;
>>+
>>+static struct cf750gx_t_call_data cf750gx_v_switch_call_data;
>>+static struct cf750gx_t_call_data cf750gx_v_lock_call_data;
>>+static struct notifier_block cf750gx_v_pll_switch_nb;
>>+static struct notifier_block cf750gx_v_pll_lock_nb;
>
>
> Also, in general, try to avoid global variables here, even
> in file scope (static), but rather put all device specific
> data into a per-device data structure.
>
How big of a problem is this? I regret the decision to rip
the SMP stuff out. But it is kinda done. If absolutely
necessary I can put these into a structure?
>
>>+static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
>>+ action, void *data)
>>+{
>>+struct cf750gx_t_call_data *cd;
>>+unsigned int idle_pll;
>>+unsigned int pll_off_cmd;
>>+unsigned int new_pll;
>
>
> The whitespace appears damaged here.
>
Just a coding style thing. I put declarations (or definitions -
I get the two confused?) on the same indent as the block they are
in. Is this a 15 yard illegal procedure penalty?
>
>>+ cd = (struct cf750gx_t_call_data *)data;
>
done
>
> data is a void pointer, so you don't need the cast, and shouldn't
> have it therefore.
>
>>+static int cf750gx_pll_lock_cb(struct notifier_block *nb, unsigned long action,
>>+ void *data)
>>+{
>>+struct cf750gx_t_call_data *cd;
>>+
>>+ cd = (struct cf750gx_t_call_data *)data;
>
>
> same here.
>
and done
>
>>+static int cf750gx_target(struct cpufreq_policy *policy,
>>+ unsigned int target_freq, unsigned int relation)
>>+{
>>+unsigned int next_index = 0; /* Index into freq_table */
>>+unsigned int next_freq = 0; /* next frequency from perf table */
>>+unsigned int next_perf_state = 0; /* Index from perf table */
>>+int result = 0;
>
>
> Don't initialize local variables in the declaration, as that will prevent
> the compiler from warning about uninitialized use.
>
done
>
>>+unsigned int pll;
>>+unsigned int new_pll;
>>+unsigned int active_pll;
>>+struct cpufreq_freqs freqs;
>>+struct cpufreq_frequency_table *ft = cf750gx_v_f_table;
>
>
> more whitespace damage. Maybe there is something wrong with your
> text editor.
>
Nope, just a faulty programmer ...
>
>>+ dprintk(__FILE__">%s(, %u KHz, relation %u)-%d: on cpu %d\n",
>>+ __func__, target_freq, relation, __LINE__, policy->cpu);
>>+
>>+ if (test_and_set_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits))
>>+ return -EAGAIN;
>>+
>>+ INIT_COMPLETION(cf750gx_v_exit_completion);
>>+
>>+ result = cpufreq_frequency_table_target(policy,
>>+ ft,
>>+ target_freq,
>>+ relation, &next_index);
>>+
>>+ if (unlikely(result))
>>+ goto cf750gxTargetNoFreq;
>
>
> The unlikely() here looks like overoptimization, drop it in favor of
> readability unless you can measure a real-world difference.
>
This was stolen from the ACPI Processor P-States Driver. Given the
frequency of calls I would guess it does not make a difference.
>
>>+ if (active_pll) {
>>+ unsigned int current_state;
>
>
> whitespace damage.
>
same here ...
>
>>+ dprintk(__FILE__">%s()-%d: Modifying PLL: 0x%x\n", __func__, __LINE__,
>>+ new_pll);
>
>
> Please go through all your dprintk and see if you really really need all of them.
> Usually they are useful while you are doing the initial code, but only get in the
> way as soon as it starts working.
>
This from a code readability standpoint? Or an efficiency one?
I think the cpufreq stuff has a debug configure option that
disables compilation of these unless enabled.
>
>>+cf750gxTargetOut:
>>+ return result;
>>+
>>+cf750gxTargetNoFreq:
>>+ result = -ENODEV;
>>+
>>+ goto cf750gxTargetUnlock;
>>+cf750gxTargetFreqSet:
>>+ result = 0;
>>+
>>+ goto cf750gxTargetUnlock;
>>+cf750gxTargetUnlock:
>>+ clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
>>+ complete(&cf750gx_v_exit_completion);
>>+
>>+ goto cf750gxTargetOut;
>
>
> The conventional way to write this would be:
>
> result = -ENODEV;
> if (foo)
> goto out_unlock;
>
> result = 0;
> if (bar)
> goto out_unlock;
>
> return 0;
>
> out_unlock:
> clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
> complete(&cf750gx_v_exit_completion);
> out:
> return result;
>
I'll fix this.
>
>>+/* policy->cpuinfo.transition_latency = CPUFREQ_ETERNAL; */
>>+ policy->cpuinfo.transition_latency = pllif_get_latency();
>
>
> The comment does not really explain anything. If you just want to disable
> code, use #if 0, but better drop it right away and add a comment about
> what might need changing.
>
deleted.
>
>>+ result = cpufreq_frequency_table_cpuinfo(policy, cf750gx_v_f_table);
>>+ if (result)
>>+ goto err_freqfree;
>>+
>>+ cpufreq_frequency_table_get_attr(cf750gx_v_f_table, policy->cpu);
>>+
>>+ cf750gx_v_pll_switch_nb.notifier_call = cf750gx_pll_switch_cb;
>>+ cf750gx_v_pll_switch_nb.next = (struct notifier_block *)
>>+ &cf750gx_v_switch_call_data;
>>+ cf750gx_v_pll_switch_nb.priority = 0;
>>+
>>+ result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
>>+
>>+ cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
>>+ cf750gx_v_pll_lock_nb.next =
>>+ (struct notifier_block *)&cf750gx_v_lock_call_data;
>
>
> These casts look wrong, cf750gx_v_lock_call_data is not a notifier_block.
> What are you trying to do here?
>
Just a little sneaky. I should document in the kernel doc though.
>
>>+ cf750gx_v_pll_lock_nb.priority = 0;
>>+
>>+ result = pllif_register_pll_lock_cb(&cf750gx_v_pll_lock_nb);
>>+
>>+ return result;
>>+
>>+err_freqfree:
>>+ return result;
>>+}
>
>
> The first 'return result' is redundant, drop it.
>
done.
>
>>+
>>+static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
>>+{
>>+ dprintk("%s()\n", __func__);
>>+
>>+ /*
>>+ * Wait for any active requests to ripple through before exiting
>>+ */
>>+ wait_for_completion(&cf750gx_v_exit_completion);
>
>
> This "wait for anything" use of wait_for_completion looks wrong,
> because once any other function has done the 'complete', you won't
> wait here any more.
>
> What exactly are you trying to accomplish with this?
>
Originall I had something like:
while(some_bit_is_still_set)
sleep
I think you suggested I use a completion because it is in
fact simpler than a sleep. Now that I think about it also seems
intuitive to give the system a hint as to when something will
be done. 'complete' just means there is no timer pending (unless,
of course, I screwed up the code).
>
>>+static int __init cf750gx_init(void)
>>+{
>>+int ret;
>>+unsigned int freq, i, j, rng, bus_clock;
>>+unsigned short min_ratio, max_ratio;
>>+struct cpufreq_frequency_table *tbp;
>>+const struct pll_750fgx_t *pll_defaults;
>
>
> whitespace.
>
>
>>+ dprintk("%s()\n", __func__);
>>+
>>+ if (!cpu_has_feature(CPU_FTR_DUAL_PLL_750FX))
>>+ return 0;
>
>
> Is this purely a feature of the CPU or does it need logic
> in the board design? If you need external hardware for it,
> you need to check the device tree for the presence of that
> hardware.
>
Purely a feature of the CPU. From what I know, voltage scaling
is an important part of reducing power comsumption. That would
be platform specific code. Anyone know if this is possible for
any 750GX based system? As far as I know it is not possible on
mine?
>
>>+ if (cf750gx_v_freq_table == NULL) {
>>+ ret = -ENOMEM;
>>+ goto ErrSimple;
>>+ }
>
>
> ret = -ENOMEM;
> if (!cf750gx_freq_table)
> goto err_simple;
>
done
>
> Arnd <><
>
>
On Tuesday 26 August 2008, Kevin Diggs wrote:
> Arnd Bergmann wrote:
> > On Monday 25 August 2008, Kevin Diggs wrote:
> > Most people list their email address here as well
> >
> For reasons I'd rather not go into, my email address is not likely
> to remain valid for much longer.
Maybe you should get a freemail account somewhere then.
It's better if people have a way to Cc the author of
a file when they make changes to it.
> > drop the _t here, or make explicit what is meant by it.
> >
> Now that I look at it the _t is supposed to be at the end. It is
> meant to indicate that this is a structure tag or type. I'll
> remove it.
Ok, thanks for the explanation. I now saw that you also
use '_v' for variables (I guess). These should probably
go the same way.
> >>+static DECLARE_COMPLETION(cf750gx_v_exit_completion);
> >>+
> >>+static unsigned int override_min_core = 0;
> >>+static unsigned int override_max_core = 0;
> >>+static unsigned int minmaxmode = 0;
> >>+
> >>+static unsigned int cf750gx_v_min_core = 0;
> >>+static unsigned int cf750gx_v_max_core = 0;
> >>+static int cf750gx_v_idle_pll_off = 0;
> >>+static int cf750gx_v_min_max_mode = 0;
> >>+static unsigned long cf750gx_v_state_bits = 0;
> >
> >
> > Is 0 a meaningful value for these? If it just means 'uninitialized',
> > then better don't initialize them in the first place, for clarity.
> >
> The first 3 are module parameters. For the first 2, 0 means
> that they were not set. minmaxmode is a boolean. 0 is the
> default of disabled.
Then at least for the first two, the common coding style would
be to leave out the '= 0'. For minmaxmode, the most expressive
way would be to define an enum, like
enum {
MODE_NORMAL,
MODE_MINMAX,
};
int minmaxmode = MODE_NORMAL;
> When I was initially writing the code I figured I would
> need the min and max core frequencies in several places.
> As it turns out they are only used in the code
> initialization routine (cf750gx_init()). I have made
> them locals.
ah, good.
> ..._idle_pll_off is a boolean for a sysfs attribute. 0 is
> the default of disabled.
ok
> ..._min_max_mode is a boolean to hold the state of
> minmaxmode. Seems to be only used to print the current
> value.
this looks like a duplicate then. why would you need both?
It seems really confusing to have both a cpufreq attribute
and a module attribute with the same name, writing to
different variables.
> ..._state_bits is a global to maintain state.
>
> Does the PowerPC suffer any performance penalties when
> accessing shorts compared to ints? Can I save a few bytes
> by using shorts?
Don't bother. Using int is the general way to say 'some number'.
If you use short, people will wonder why you require a 16 bit
number.
> >>+static struct cpufreq_frequency_table *cf750gx_v_f_table;
> >>+static struct cpufreq_frequency_table *cf750gx_v_freq_table;
> >>+static struct cpufreq_frequency_table *cf750gx_v_min_max_freq_table;
> >>+
> >>+static struct cf750gx_t_call_data cf750gx_v_switch_call_data;
> >>+static struct cf750gx_t_call_data cf750gx_v_lock_call_data;
> >>+static struct notifier_block cf750gx_v_pll_switch_nb;
> >>+static struct notifier_block cf750gx_v_pll_lock_nb;
> >
> >
> > Also, in general, try to avoid global variables here, even
> > in file scope (static), but rather put all device specific
> > data into a per-device data structure.
> >
> How big of a problem is this? I regret the decision to rip
> the SMP stuff out. But it is kinda done. If absolutely
> necessary I can put these into a structure?
Not a huge problem in this case, because it is not strictly a
device driver to start with. In most device drivers, you want
a strict separation between global (per-driver) data and
per-instance data.
Are there even SMP boards based on a 750? I thought only 74xx
and 603/604 were SMP capable.
> >>+static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
> >>+ action, void *data)
> >>+{
> >>+struct cf750gx_t_call_data *cd;
> >>+unsigned int idle_pll;
> >>+unsigned int pll_off_cmd;
> >>+unsigned int new_pll;
> >
> >
> > The whitespace appears damaged here.
> >
> Just a coding style thing. I put declarations (or definitions -
> I get the two confused?) on the same indent as the block they are
> in. Is this a 15 yard illegal procedure penalty?
I've never seen that style before. Better do what everyone
else does here, e.g. using 'indent -kr -i8'.
> >>+ dprintk(__FILE__">%s()-%d: Modifying PLL: 0x%x\n", __func__, __LINE__,
> >>+ new_pll);
> >
> >
> > Please go through all your dprintk and see if you really really need all of them.
> > Usually they are useful while you are doing the initial code, but only get in the
> > way as soon as it starts working.
> >
> This from a code readability standpoint? Or an efficiency one?
> I think the cpufreq stuff has a debug configure option that
> disables compilation of these unless enabled.
It's more about readability.
> >>+ result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
> >>+
> >>+ cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
> >>+ cf750gx_v_pll_lock_nb.next =
> >>+ (struct notifier_block *)&cf750gx_v_lock_call_data;
> >
> >
> > These casts look wrong, cf750gx_v_lock_call_data is not a notifier_block.
> > What are you trying to do here?
> >
> Just a little sneaky. I should document in the kernel doc though.
No, better avoid such hacks instead of documenting them. I think I
now understand what you do there, and if I got it right, you should
just pass two arguments to pllif_register_pll_switch_cb.
> >>+static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
> >>+{
> >>+ dprintk("%s()\n", __func__);
> >>+
> >>+ /*
> >>+ * Wait for any active requests to ripple through before exiting
> >>+ */
> >>+ wait_for_completion(&cf750gx_v_exit_completion);
> >
> >
> > This "wait for anything" use of wait_for_completion looks wrong,
> > because once any other function has done the 'complete', you won't
> > wait here any more.
> >
> > What exactly are you trying to accomplish with this?
> >
> Originall I had something like:
>
> while(some_bit_is_still_set)
> sleep
>
> I think you suggested I use a completion because it is in
> fact simpler than a sleep. Now that I think about it also seems
> intuitive to give the system a hint as to when something will
> be done. 'complete' just means there is no timer pending (unless,
> of course, I screwed up the code).
The completion would certainly be better than the sleep here, but
I think you shouldn't need that in the first place. AFAICT, you
have three different kinds of events that you need to protect
against, when some other code can call into your module:
1. timer function,
2. cpufreq notifier, and
3. sysfs attribute.
In each case, the problem is a race between two threads that you
need to close. In case of the timer, you need to call del_timer_sync
because the timers already have this method builtin. For the other
two, you already unregister the interfaces, which ought to be enough.
Arnd <><
Arnd Bergmann wrote:
> On Tuesday 26 August 2008, Kevin Diggs wrote:
>
>>Arnd Bergmann wrote:
>>
>>>On Monday 25 August 2008, Kevin Diggs wrote:
>
>
>>>Most people list their email address here as well
>>>
>>
>>For reasons I'd rather not go into, my email address is not likely
>>to remain valid for much longer.
>
>
> Maybe you should get a freemail account somewhere then.
> It's better if people have a way to Cc the author of
> a file when they make changes to it.
>
That won't really help either.
>
>>>drop the _t here, or make explicit what is meant by it.
>>>
>>
>>Now that I look at it the _t is supposed to be at the end. It is
>>meant to indicate that this is a structure tag or type. I'll
>>remove it.
>
>
> Ok, thanks for the explanation. I now saw that you also
> use '_v' for variables (I guess). These should probably
> go the same way.
>
Actually the _v means global variable. I would prefer to keep it.
>
>>>>+static DECLARE_COMPLETION(cf750gx_v_exit_completion);
>>>>+
>>>>+static unsigned int override_min_core = 0;
>>>>+static unsigned int override_max_core = 0;
>>>>+static unsigned int minmaxmode = 0;
>>>>+
>>>>+static unsigned int cf750gx_v_min_core = 0;
>>>>+static unsigned int cf750gx_v_max_core = 0;
>>>>+static int cf750gx_v_idle_pll_off = 0;
>>>>+static int cf750gx_v_min_max_mode = 0;
>>>>+static unsigned long cf750gx_v_state_bits = 0;
>>>
>>>
>>>Is 0 a meaningful value for these? If it just means 'uninitialized',
>>>then better don't initialize them in the first place, for clarity.
>>>
>>
>>The first 3 are module parameters. For the first 2, 0 means
>>that they were not set. minmaxmode is a boolean. 0 is the
>>default of disabled.
>
>
> Then at least for the first two, the common coding style would
> be to leave out the '= 0'. For minmaxmode, the most expressive
> way would be to define an enum, like
>
> enum {
> MODE_NORMAL,
> MODE_MINMAX,
> };
>
> int minmaxmode = MODE_NORMAL;
>
Since this is a boolean parameter I don't know? What if I change the
name to enable_minmax. And actually use the "bool" module parameter
type?
>
>>..._min_max_mode is a boolean to hold the state of
>>minmaxmode. Seems to be only used to print the current
>>value.
>
>
> this looks like a duplicate then. why would you need both?
> It seems really confusing to have both a cpufreq attribute
> and a module attribute with the same name, writing to
> different variables.
>
I probably don't need both? I kinda treat the variables tied to module
parameters as read only.
>
> Are there even SMP boards based on a 750? I thought only 74xx
> and 603/604 were SMP capable.
>
Not that I have heard of. I thought it was lacking some hardware that
was needed to do SMP? Can the 603 do SMP?
>
>>>>+static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
>>>>+ action, void *data)
>>>>+{
>>>>+struct cf750gx_t_call_data *cd;
>>>>+unsigned int idle_pll;
>>>>+unsigned int pll_off_cmd;
>>>>+unsigned int new_pll;
>>>
>>>
>>>The whitespace appears damaged here.
>>>
>>
>>Just a coding style thing. I put declarations (or definitions -
>>I get the two confused?) on the same indent as the block they are
>>in. Is this a 15 yard illegal procedure penalty?
>
>
> I've never seen that style before. Better do what everyone
> else does here, e.g. using 'indent -kr -i8'.
>
Ok, I'll fix this.
>
>>>>+ dprintk(__FILE__">%s()-%d: Modifying PLL: 0x%x\n", __func__, __LINE__,
>>>>+ new_pll);
>>>
>>>
>>>Please go through all your dprintk and see if you really really need all of them.
>>>Usually they are useful while you are doing the initial code, but only get in the
>>>way as soon as it starts working.
>>>
>>
>>This from a code readability standpoint? Or an efficiency one?
>>I think the cpufreq stuff has a debug configure option that
>>disables compilation of these unless enabled.
>
>
> It's more about readability.
>
I removed a few. Let me know if I should whack some more (and which ones).
>
>>>>+ result = pllif_register_pll_switch_cb(&cf750gx_v_pll_switch_nb);
>>>>+
>>>>+ cf750gx_v_pll_lock_nb.notifier_call = cf750gx_pll_lock_cb;
>>>>+ cf750gx_v_pll_lock_nb.next =
>>>>+ (struct notifier_block *)&cf750gx_v_lock_call_data;
>>>
>>>
>>>These casts look wrong, cf750gx_v_lock_call_data is not a notifier_block.
>>>What are you trying to do here?
>>>
>>
>>Just a little sneaky. I should document in the kernel doc though.
>
>
> No, better avoid such hacks instead of documenting them. I think I
> now understand what you do there, and if I got it right, you should
> just pass two arguments to pllif_register_pll_switch_cb.
>
I'll fix this.
>
>>>>+static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
>>>>+{
>>>>+ dprintk("%s()\n", __func__);
>>>>+
>>>>+ /*
>>>>+ * Wait for any active requests to ripple through before exiting
>>>>+ */
>>>>+ wait_for_completion(&cf750gx_v_exit_completion);
>>>
>>>
>>>This "wait for anything" use of wait_for_completion looks wrong,
>>>because once any other function has done the 'complete', you won't
>>>wait here any more.
>>>
>>>What exactly are you trying to accomplish with this?
>>>
>>
>>Originall I had something like:
>>
>> while(some_bit_is_still_set)
>> sleep
>>
>>I think you suggested I use a completion because it is in
>>fact simpler than a sleep. Now that I think about it also seems
>>intuitive to give the system a hint as to when something will
>>be done. 'complete' just means there is no timer pending (unless,
>>of course, I screwed up the code).
>
>
> The completion would certainly be better than the sleep here, but
> I think you shouldn't need that in the first place. AFAICT, you
> have three different kinds of events that you need to protect
> against, when some other code can call into your module:
>
> 1. timer function,
> 2. cpufreq notifier, and
> 3. sysfs attribute.
>
> In each case, the problem is a race between two threads that you
> need to close. In case of the timer, you need to call del_timer_sync
> because the timers already have this method builtin. For the other
> two, you already unregister the interfaces, which ought to be enough.
>
The choice I made here was to wait for the timer to fire rather than
to delete it. I think it is easier to just wait for it than to delete
it and try to get things back in order. Though since this is in a
module exit routine it probably does not matter. Also I would have to
check whether there was an analogous HRTimer call and call the right
one.
>
> Arnd <><
>
On Wednesday 27 August 2008, Kevin Diggs wrote:
> Arnd Bergmann wrote:
> > Ok, thanks for the explanation. I now saw that you also
> > use '_v' for variables (I guess). These should probably
> > go the same way.
> >
> Actually the _v means global variable. I would prefer to keep it.
The reasoning on Linux is that you can tell from the declaration
whether something is global or automatic. In fact, functions should
be so short that you can always see all automatic declarations
when you look at some code.
If you use nonstandard variable naming, people will never stop
asking you about that, so it's easier to just to the same as
everyone else.
> >
> > Then at least for the first two, the common coding style would
> > be to leave out the '= 0'. For minmaxmode, the most expressive
> > way would be to define an enum, like
> >
> > enum {
> > MODE_NORMAL,
> > MODE_MINMAX,
> > };
> >
> > int minmaxmode = MODE_NORMAL;
> >
> Since this is a boolean parameter I don't know? What if I change the
> name to enable_minmax. And actually use the "bool" module parameter
> type?
Module parameter names should be short, so just "minmax" would
be a good name, but better put the module_param() line right
after that. If it's a bool type, I would just leave out the
initialization.
> >>..._min_max_mode is a boolean to hold the state of
> >>minmaxmode. Seems to be only used to print the current
> >>value.
> >
> >
> > this looks like a duplicate then. why would you need both?
> > It seems really confusing to have both a cpufreq attribute
> > and a module attribute with the same name, writing to
> > different variables.
> >
> I probably don't need both? I kinda treat the variables tied to module
> parameters as read only.
But you have marked as read/write in the module_param line.
I would prefer to just have the module parameter and have
a tool to modify that one.
If a module parameter only makes sense as read-only, you
should mark it as 0444 instead of 0644, but this one looks
like it can be writable.
> > Are there even SMP boards based on a 750? I thought only 74xx
> > and 603/604 were SMP capable.
> >
> Not that I have heard of. I thought it was lacking some hardware that
> was needed to do SMP? Can the 603 do SMP?
No, I was wrong about the 603. The machine I was thinking of is actually
604e.
> > The completion would certainly be better than the sleep here, but
> > I think you shouldn't need that in the first place. AFAICT, you
> > have three different kinds of events that you need to protect
> > against, when some other code can call into your module:
> >
> > 1. timer function,
> > 2. cpufreq notifier, and
> > 3. sysfs attribute.
> >
> > In each case, the problem is a race between two threads that you
> > need to close. In case of the timer, you need to call del_timer_sync
> > because the timers already have this method builtin. For the other
> > two, you already unregister the interfaces, which ought to be enough.
> >
> The choice I made here was to wait for the timer to fire rather than
> to delete it. I think it is easier to just wait for it than to delete
> it and try to get things back in order. Though since this is in a
> module exit routine it probably does not matter. Also I would have to
> check whether there was an analogous HRTimer call and call the right
> one.
I think the module_exit() function should leave the frequency in a
well-defined state, so the easiest way to get there is probably
to delete the timer, and then manually set the frequency.
Arnd <><
On Wed, 27 Aug 2008, Arnd Bergmann wrote:
> On Wednesday 27 August 2008, Kevin Diggs wrote:
> > Arnd Bergmann wrote:
> > > Are there even SMP boards based on a 750? I thought only 74xx
> > > and 603/604 were SMP capable.
> > >
> > Not that I have heard of. I thought it was lacking some hardware that
> > was needed to do SMP? Can the 603 do SMP?
>
> No, I was wrong about the 603. The machine I was thinking of is actually
> 604e.
The BeBox had a dual 603.
With kind regards,
Geert Uytterhoeven
Software Architect
Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium
Phone: +32 (0)2 700 8453
Fax: +32 (0)2 700 8622
E-mail: [email protected]
Internet: http://www.sony-europe.com/
A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010
Arnd Bergmann wrote:
> On Wednesday 27 August 2008, Kevin Diggs wrote:
>
>>Arnd Bergmann wrote:
>
>
>>>Ok, thanks for the explanation. I now saw that you also
>>>use '_v' for variables (I guess). These should probably
>>>go the same way.
>>>
>>
>>Actually the _v means global variable. I would prefer to keep it.
>
>
> The reasoning on Linux is that you can tell from the declaration
> whether something is global or automatic. In fact, functions should
> be so short that you can always see all automatic declarations
> when you look at some code.
>
> If you use nonstandard variable naming, people will never stop
> asking you about that, so it's easier to just to the same as
> everyone else.
>
I will remove the "_v".
>
>>>Then at least for the first two, the common coding style would
>>>be to leave out the '= 0'. For minmaxmode, the most expressive
>>>way would be to define an enum, like
>>>
>>>enum {
>>> MODE_NORMAL,
>>> MODE_MINMAX,
>>>};
>>>
>>>int minmaxmode = MODE_NORMAL;
>>>
>>
>>Since this is a boolean parameter I don't know? What if I change the
>>name to enable_minmax. And actually use the "bool" module parameter
>>type?
>
>
> Module parameter names should be short, so just "minmax" would
> be a good name, but better put the module_param() line right
> after that. If it's a bool type, I would just leave out the
> initialization.
>
Ok. But leaving out the initialization will make me itch. Should I
also replace "override_min_core" with "mincore" (or "min_core")? And
"override_max_core" with "maxcore" (or "max_core")?
Leaving out the initializations makes me ... uneasy. It's ok to leave
them out if they are 0?
>
>>>>..._min_max_mode is a boolean to hold the state of
>>>>minmaxmode. Seems to be only used to print the current
>>>>value.
>>>
>>>
>>>this looks like a duplicate then. why would you need both?
>>>It seems really confusing to have both a cpufreq attribute
>>>and a module attribute with the same name, writing to
>>>different variables.
>>>
>>
>>I probably don't need both? I kinda treat the variables tied to module
>>parameters as read only.
>
>
> But you have marked as read/write in the module_param line.
>
> I would prefer to just have the module parameter and have
> a tool to modify that one.
>
> If a module parameter only makes sense as read-only, you
> should mark it as 0444 instead of 0644, but this one looks
> like it can be writable.
>
I meant I treat them as read only from the code. That is why I have a
separate variable to change from the sysfs routines. I'll eliminate it
if you like. I have removed the auto added sysfs attributes.
>
>>>The completion would certainly be better than the sleep here, but
>>>I think you shouldn't need that in the first place. AFAICT, you
>>>have three different kinds of events that you need to protect
>>>against, when some other code can call into your module:
>>>
>>>1. timer function,
>>>2. cpufreq notifier, and
>>>3. sysfs attribute.
>>>
>>>In each case, the problem is a race between two threads that you
>>>need to close. In case of the timer, you need to call del_timer_sync
>>>because the timers already have this method builtin. For the other
>>>two, you already unregister the interfaces, which ought to be enough.
>>>
>>
>>The choice I made here was to wait for the timer to fire rather than
>>to delete it. I think it is easier to just wait for it than to delete
>>it and try to get things back in order. Though since this is in a
>>module exit routine it probably does not matter. Also I would have to
>>check whether there was an analogous HRTimer call and call the right
>>one.
>
>
> I think the module_exit() function should leave the frequency in a
> well-defined state, so the easiest way to get there is probably
> to delete the timer, and then manually set the frequency.
>
I really don't follow you here? If I let the timer fire then the cpu
(and the cpufreq sub-system) will be left in a well-defined state. I
don't understand why you want me to delete the timer and then
basically do manually what it was going to do anyway. There are two
calls to cpufreq_notify_transition(). One just before the modify_PLL()
call, with CPUFREQ_PRECHANGE as an argument, and one in the
pll_switch_cb() routine, with CPUFREQ_POSTCHANGE as an argument. I
would need to make sure that these are matched up.
Even without the HRTimer stuff being used the timer fires in less than
4 ms (@ 250 HZ). So I can't see the user actually trying to interrupt
a frequency change. With HRTimers it is 100 us.
Can we please, please leave this part as is?
>
> Arnd <><
>
Arnd Bergmann wrote:
> On Wednesday 27 August 2008, Kevin Diggs wrote:
>
>>Arnd Bergmann wrote:
>
>
>>>Ok, thanks for the explanation. I now saw that you also
>>>use '_v' for variables (I guess). These should probably
>>>go the same way.
>>>
>>
>>Actually the _v means global variable. I would prefer to keep it.
>
>
> The reasoning on Linux is that you can tell from the declaration
> whether something is global or automatic. In fact, functions should
> be so short that you can always see all automatic declarations
> when you look at some code.
>
> If you use nonstandard variable naming, people will never stop
> asking you about that, so it's easier to just to the same as
> everyone else.
>
>
>>>Then at least for the first two, the common coding style would
>>>be to leave out the '= 0'. For minmaxmode, the most expressive
>>>way would be to define an enum, like
>>>
>>>enum {
>>> MODE_NORMAL,
>>> MODE_MINMAX,
>>>};
>>>
>>>int minmaxmode = MODE_NORMAL;
>>>
>>
>>Since this is a boolean parameter I don't know? What if I change the
>>name to enable_minmax. And actually use the "bool" module parameter
>>type?
>
>
> Module parameter names should be short, so just "minmax" would
> be a good name, but better put the module_param() line right
> after that. If it's a bool type, I would just leave out the
> initialization.
>
>
>>>>..._min_max_mode is a boolean to hold the state of
>>>>minmaxmode. Seems to be only used to print the current
>>>>value.
>>>
>>>
>>>this looks like a duplicate then. why would you need both?
>>>It seems really confusing to have both a cpufreq attribute
>>>and a module attribute with the same name, writing to
>>>different variables.
>>>
>>
>>I probably don't need both? I kinda treat the variables tied to module
>>parameters as read only.
>
>
> But you have marked as read/write in the module_param line.
>
> I would prefer to just have the module parameter and have
> a tool to modify that one.
>
> If a module parameter only makes sense as read-only, you
> should mark it as 0444 instead of 0644, but this one looks
> like it can be writable.
>
>
>>>Are there even SMP boards based on a 750? I thought only 74xx
>>>and 603/604 were SMP capable.
>>>
>>
>>Not that I have heard of. I thought it was lacking some hardware that
>>was needed to do SMP? Can the 603 do SMP?
>
>
> No, I was wrong about the 603. The machine I was thinking of is actually
> 604e.
>
>
>>>The completion would certainly be better than the sleep here, but
>>>I think you shouldn't need that in the first place. AFAICT, you
>>>have three different kinds of events that you need to protect
>>>against, when some other code can call into your module:
>>>
>>>1. timer function,
>>>2. cpufreq notifier, and
>>>3. sysfs attribute.
>>>
>>>In each case, the problem is a race between two threads that you
>>>need to close. In case of the timer, you need to call del_timer_sync
>>>because the timers already have this method builtin. For the other
>>>two, you already unregister the interfaces, which ought to be enough.
>>>
>>
>>The choice I made here was to wait for the timer to fire rather than
>>to delete it. I think it is easier to just wait for it than to delete
>>it and try to get things back in order. Though since this is in a
>>module exit routine it probably does not matter. Also I would have to
>>check whether there was an analogous HRTimer call and call the right
>>one.
>
>
> I think the module_exit() function should leave the frequency in a
> well-defined state, so the easiest way to get there is probably
> to delete the timer, and then manually set the frequency.
>
> Arnd <><
>
>
On Wednesday 27 August 2008, Kevin Diggs wrote:
> Arnd Bergmann wrote:
> >
> > Module parameter names should be short, so just "minmax" would
> > be a good name, but better put the module_param() line right
> > after that. If it's a bool type, I would just leave out the
> > initialization.
> >
> Ok. But leaving out the initialization will make me itch. Should I
> also replace "override_min_core" with "mincore" (or "min_core")? And
> "override_max_core" with "maxcore" (or "max_core")?
>
> Leaving out the initializations makes me ... uneasy. It's ok to leave
> them out if they are 0?
Yes, that's how global and static variables are defined in C.
Only automatic variables have undefined content.
> > I think the module_exit() function should leave the frequency in a
> > well-defined state, so the easiest way to get there is probably
> > to delete the timer, and then manually set the frequency.
> >
> I really don't follow you here? If I let the timer fire then the cpu
> (and the cpufreq sub-system) will be left in a well-defined state. I
> don't understand why you want me to delete the timer and then
> basically do manually what it was going to do anyway. There are two
> calls to cpufreq_notify_transition(). One just before the modify_PLL()
> call, with CPUFREQ_PRECHANGE as an argument, and one in the
> pll_switch_cb() routine, with CPUFREQ_POSTCHANGE as an argument. I
> would need to make sure that these are matched up.
>
> Even without the HRTimer stuff being used the timer fires in less than
> 4 ms (@ 250 HZ). So I can't see the user actually trying to interrupt
> a frequency change. With HRTimers it is 100 us.
>
> Can we please, please leave this part as is?
I'm still not convinced that it's actually correct if you call complete()
from the other places as well. You have three locations in your code where
you call complete() but only one for INIT_COMPLETION. The part that I don't
understand (and therefore don't expect other readers to understand) is how
the driver guarantees that only one complete() will be called on the
completion variable after it has been initialized.
What I meant with the well-defined state is that after unloading the module,
the CPU frequency should be the same as before loading the module, typically
the maximum frequency, but not the one that was set last.
Arnd <><
Arnd Bergmann wrote:
> On Wednesday 27 August 2008, Kevin Diggs wrote:
>
>>Arnd Bergmann wrote:
>>
>>>I think the module_exit() function should leave the frequency in a
>>>well-defined state, so the easiest way to get there is probably
>>>to delete the timer, and then manually set the frequency.
>>>
>>
>>I really don't follow you here? If I let the timer fire then the cpu
>>(and the cpufreq sub-system) will be left in a well-defined state. I
>>don't understand why you want me to delete the timer and then
>>basically do manually what it was going to do anyway. There are two
>>calls to cpufreq_notify_transition(). One just before the modify_PLL()
>>call, with CPUFREQ_PRECHANGE as an argument, and one in the
>>pll_switch_cb() routine, with CPUFREQ_POSTCHANGE as an argument. I
>>would need to make sure that these are matched up.
>>
>>Even without the HRTimer stuff being used the timer fires in less than
>>4 ms (@ 250 HZ). So I can't see the user actually trying to interrupt
>>a frequency change. With HRTimers it is 100 us.
>>
>>Can we please, please leave this part as is?
>
>
> I'm still not convinced that it's actually correct if you call complete()
> from the other places as well. You have three locations in your code where
> you call complete() but only one for INIT_COMPLETION. The part that I don't
> understand (and therefore don't expect other readers to understand) is how
> the driver guarantees that only one complete() will be called on the
> completion variable after it has been initialized.
>
> What I meant with the well-defined state is that after unloading the module,
> the CPU frequency should be the same as before loading the module, typically
> the maximum frequency, but not the one that was set last.
>
As you point out, there are three calls to complete().
The first is in the switch callback, in the idle_pll_off disabled branch.
This callback is triggered from the pll switch routine in pll_if. So that
means the call to _modify_PLL() in _target worked. So the complete() call
in _target will NOT be called. The complete() call in the lock callback
is only called in the idle_pll_off enabled branch.
As just mentioned, the second complete() call in the lock callback is
only called when idle_pll_off is enabled.
The final complete() call is in the unlock exit path in _target(). This
is an error path, where for one reason or another, there was no successful
call to _modify_PLL(). Thus there will be triggering of either callback.
There is another initialization of the completion: the DECLARE_COMPLETION().
I think I will deadlock if I get unloaded before _target() is ever called.
This can happen. I may add a test_bit(...changing_pll_bit) condition on the
wait_for_completion() call.
>
> Arnd <><
>
Thanks for taking the time to review and for the comments!
kevin