Add cpufreq driver for the IBM PowerPC 750GX

PATCH Applied Kevin Diggs 4 participants 13 messages Aug 25, 2008 – Aug 28, 2008

Share & tools

Kevin Diggs

author

2008-08-25 10:54 UTC #0 lore raw

PATCH 2/4 Add cpufreq driver for the IBM PowerPC 750GX

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]>

DocBook/cf750gx.tmpl new file +441 -0

Index: Documentation/DocBook/cf750gx.tmpl
===================================================================
@@ -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&nbsp;</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>

DocBook/Makefile renamed from DocBook/Makefile.orig +1 -1

Index: Documentation/DocBook/Makefile
===================================================================
@@ -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):

powerpc/kernel/cpu/cpufreq/Kconfig new file +33 -0

Index: arch/powerpc/kernel/cpu/cpufreq/Kconfig
===================================================================
@@ -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

powerpc/kernel/cpu/cpufreq/Makefile new file +1 -0

Index: arch/powerpc/kernel/cpu/cpufreq/Makefile
===================================================================
@@ -0,0 +1 @@
+obj-$(CONFIG_CPU_FREQ_PPC_750GX_DUAL_PLL)	+= cf750gx.o

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +736 -0

736 lines not on this page · raw message

powerpc/kernel/cpu/Makefile new file +6 -0

Index: arch/powerpc/kernel/cpu/Makefile
===================================================================
@@ -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

Arnd Bergmann

2008-08-25 11:43 UTC #1 lore raw

Quoted 1 line · Kevin Diggs

+ * 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.

Quoted 1 line

+ * =A0Copyright (C) 2008 =A0 =A0 =A0 kevin Diggs

Most people list their email address here as well

Quoted 1 line

+#define cf750gxmChangingPll=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0(0x800000=

00)

Review

Quoted 3 lines

(quoted diff) +3 -0

+#define cf750gxmChangingPllBit=A0=A0=A0=A0=A0=A0=A0=A0=A0(31)
+#define cf750gxmTurningIdlePllOff=A0=A0=A0=A0=A0=A0(0x40000000)
+#define cf750gxmTurningIdlePllOffBit=A0=A0=A0(30)

constants should be ALL_CAPS, not sIllYCaPS.

Quoted 2 lines

+struct pll_750fgx_t {
+=A0=A0=A0=A0=A0=A0=A0unsigned short min_ratio;=A0=A0=A0=A0=A0=A0=A0/* mi=

n bus ratio */

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0unsigned short max_ratio;=A0=A0=A0=A0=A0=A0=A0/* ma=

x bus ratio */

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0unsigned int min_core;=A0=A0=A0=A0=A0=A0=A0=A0=A0=

=A0/* min core frequency per spec (KHz) */

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0unsigned int max_core;=A0=A0=A0=A0=A0=A0=A0=A0=A0=

=A0/* max core frequency per spec (KHz) */

Quoted 1 line

+};

please drop the _t at the end of the identifier.

powerpc/kernel/cpu/cpufreq/cf750gx.c line 57

Quoted 3 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +3 -0

+MODULE_AUTHOR("Kevin Diggs");
+MODULE_DESCRIPTION("750GX Dual PLL cpufreq driver");
+MODULE_LICENSE("GPL");

Move this to the end.

Review

Quoted 5 lines

(quoted diff) +5 -0

+struct cf750gx_t_call_data {
+=A0=A0=A0=A0=A0=A0=A0struct cpufreq_freqs freqs;
+=A0=A0=A0=A0=A0=A0=A0unsigned long current_pll;
+=A0=A0=A0=A0=A0=A0=A0int idle_pll_off;
+};

drop the _t here, or make explicit what is meant by it.

Review

Quoted 13 lines

(quoted diff) +13 -0

+static const struct pll_750fgx_t __initdata pll_750fx =3D {
+=A0=A0=A0=A0=A0=A0=A0.min_ratio =3D 2,
+=A0=A0=A0=A0=A0=A0=A0.max_ratio =3D 20,
+=A0=A0=A0=A0=A0=A0=A0.min_core =3D 400000,
+=A0=A0=A0=A0=A0=A0=A0.max_core =3D 800000,
+};
+
+static const struct pll_750fgx_t __initdata pll_750gx =3D {
+=A0=A0=A0=A0=A0=A0=A0.min_ratio =3D 2,
+=A0=A0=A0=A0=A0=A0=A0.max_ratio =3D 20,
+=A0=A0=A0=A0=A0=A0=A0.min_core =3D 500000,
+=A0=A0=A0=A0=A0=A0=A0.max_core =3D 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.

Review

Quoted 11 lines

(quoted diff) +11 -0

+static DECLARE_COMPLETION(cf750gx_v_exit_completion);
+
+static unsigned int override_min_core =3D 0;
+static unsigned int override_max_core =3D 0;
+static unsigned int minmaxmode =3D 0;
+
+static unsigned int cf750gx_v_min_core =3D 0;
+static unsigned int cf750gx_v_max_core =3D 0;
+static int cf750gx_v_idle_pll_off =3D 0;
+static int cf750gx_v_min_max_mode =3D 0;
+static unsigned long cf750gx_v_state_bits =3D 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.

powerpc/kernel/cpu/cpufreq/cf750gx.c line 98

Quoted 8 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +8 -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;

Also, in general, try to avoid global variables here, even=20 in file scope (static), but rather put all device specific data into a per-device data structure.

powerpc/kernel/cpu/cpufreq/cf750gx.c line 106

Quoted 7 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0

+static int cf750gx_pll_switch_cb(struct notifier_block *nb, unsigned long
+=A0=A0=A0=A0=A0=A0=A0action, 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.

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0cd =3D (struct cf750gx_t_call_data *)data;

data is a void pointer, so you don't need the cast, and shouldn't have it therefore.

Quoted 1 line

+static int cf750gx_pll_lock_cb(struct notifier_block *nb, unsigned long =

action,

powerpc/kernel/cpu/cpufreq/cf750gx.c

Quoted 5 lines

+=A0=A0=A0=A0=A0=A0=A0void *data)
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -0
+{
+struct cf750gx_t_call_data *cd;
+
+=A0=A0=A0=A0=A0=A0=A0cd =3D (struct cf750gx_t_call_data *)data;

same here.

Quoted 2 lines

+static int cf750gx_target(struct cpufreq_policy *policy, +=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0 =

=A0 =A0 =A0 unsigned int target_freq, unsigned int relation)

Review

Quoted 2 lines

(quoted diff) +2 -0

+{
+unsigned int next_index =3D 0;=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* Index =

into freq_table */

Quoted 1 line

+unsigned int next_freq =3D 0;=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0/* next=

frequency from perf table */

Quoted 1 line

+unsigned int next_perf_state =3D 0;=A0=A0=A0=A0=A0=A0/* Index from perf =

table */

Quoted 1 line

+int result =3D 0;

Don't initialize local variables in the declaration, as that will prevent the compiler from warning about uninitialized use.

powerpc/kernel/cpu/cpufreq/cf750gx.c

Quoted 5 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +5 -0

+unsigned int pll;
+unsigned int new_pll;
+unsigned int active_pll;
+struct cpufreq_freqs freqs;
+struct cpufreq_frequency_table *ft =3D cf750gx_v_f_table;

more whitespace damage. Maybe there is something wrong with your text editor.

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0dprintk(__FILE__">%s(, %u KHz, relation %u)-%d: =A0=

on cpu %d\n",

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0__func__, target_freq, rela=

tion, __LINE__, policy->cpu);

Quoted 2 lines

+
+=A0=A0=A0=A0=A0=A0=A0if (test_and_set_bit(cf750gxmChangingPllBit, &cf750=

gx_v_state_bits))

Review

Quoted 6 lines

+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return -EAGAIN;

(quoted diff) +5 -0

+
+=A0=A0=A0=A0=A0=A0=A0INIT_COMPLETION(cf750gx_v_exit_completion);
+
+=A0=A0=A0=A0=A0=A0=A0result =3D cpufreq_frequency_table_target(policy,
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=

=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ft,

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=

=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0target= _freq,

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=

=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0relati= on, &next_index);

Quoted 3 lines

+=A0=A0=A0=A0=A0=A0=A0if (unlikely(result))
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto cf750gxTargetNoFreq;

The unlikely() here looks like overoptimization, drop it in favor of readability unless you can measure a real-world difference.

Quoted 2 lines

+=A0=A0=A0=A0=A0=A0=A0if (active_pll) {
+=A0=A0=A0=A0=A0=A0=A0unsigned int current_state;

whitespace damage.

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0dprintk(__FILE__">%s()-%d: =A0Modifying PLL: =A00x%=

x\n", __func__, __LINE__,

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0new_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.

powerpc/kernel/cpu/cpufreq/cf750gx.c

Quoted 13 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +13 -0

+cf750gxTargetOut:
+=A0=A0=A0=A0=A0=A0=A0return result;
+
+cf750gxTargetNoFreq:
+=A0=A0=A0=A0=A0=A0=A0result =3D -ENODEV;
+
+=A0=A0=A0=A0=A0=A0=A0goto cf750gxTargetUnlock;
+cf750gxTargetFreqSet:
+=A0=A0=A0=A0=A0=A0=A0result =3D 0;
+
+=A0=A0=A0=A0=A0=A0=A0goto cf750gxTargetUnlock;
+cf750gxTargetUnlock:
+=A0=A0=A0=A0=A0=A0=A0clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_=

bits);

Quoted 3 lines

+=A0=A0=A0=A0=A0=A0=A0complete(&cf750gx_v_exit_completion); +
+=A0=A0=A0=A0=A0=A0=A0goto cf750gxTargetOut;

The conventional way to write this would be:

	result =3D -ENODEV;
	if (foo)
		goto out_unlock;

	result =3D 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;

Quoted 1 line

+/*=A0=A0=A0=A0=A0policy->cpuinfo.transition_latency =3D CPUFREQ_ETERNAL;=

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0policy->cpuinfo.transition_latency =3D pllif_get_la=

tency();

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.

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0result =3D cpufreq_frequency_table_cpuinfo(policy, =

cf750gx_v_f_table);

Quoted 4 lines

+=A0=A0=A0=A0=A0=A0=A0if (result)
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto err_freqfree;
+ +=A0=A0=A0=A0=A0=A0=A0cpufreq_frequency_table_get_attr(cf750gx_v_f_table,=

policy->cpu);

Quoted 2 lines

+
+=A0=A0=A0=A0=A0=A0=A0cf750gx_v_pll_switch_nb.notifier_call =3D cf750gx_p=

ll_switch_cb;

Quoted 1 line

+=A0=A0=A0=A0=A0=A0=A0cf750gx_v_pll_switch_nb.next =3D (struct notifier_b=

lock *)

Quoted 4 lines

+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0&cf750gx_v_switch_call_data;
+=A0=A0=A0=A0=A0=A0=A0cf750gx_v_pll_switch_nb.priority =3D 0;
+
+=A0=A0=A0=A0=A0=A0=A0result =3D pllif_register_pll_switch_cb(&cf750gx_v_=

pll_switch_nb);

Quoted 2 lines

+
+=A0=A0=A0=A0=A0=A0=A0cf750gx_v_pll_lock_nb.notifier_call =3D cf750gx_pll=

_lock_cb;

Quoted 2 lines

+=A0=A0=A0=A0=A0=A0=A0cf750gx_v_pll_lock_nb.next =3D
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0(struct notifier_block *)&c=

f750gx_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?

Quoted 3 lines

+=A0=A0=A0=A0=A0=A0=A0cf750gx_v_pll_lock_nb.priority =3D 0;
+
+=A0=A0=A0=A0=A0=A0=A0result =3D pllif_register_pll_lock_cb(&cf750gx_v_pl=

l_lock_nb);

powerpc/kernel/cpu/cpufreq/cf750gx.c line 398

Quoted 6 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +6 -0
+
+=A0=A0=A0=A0=A0=A0=A0return result;
+
+err_freqfree:
+=A0=A0=A0=A0=A0=A0=A0return result;
+}

The first 'return result' is redundant, drop it.

powerpc/kernel/cpu/cpufreq/cf750gx.c

Quoted 7 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0

+
+static int cf750gx_cpu_exit(struct cpufreq_policy *policy)
+{
+=A0=A0=A0=A0=A0=A0=A0dprintk("%s()\n", __func__);
+
+=A0=A0=A0=A0=A0=A0=A0/*
+=A0=A0=A0=A0=A0=A0=A0 * Wait for any active requests to ripple through b=

efore exiting

Quoted 2 lines

+=A0=A0=A0=A0=A0=A0=A0 */
+=A0=A0=A0=A0=A0=A0=A0wait_for_completion(&cf750gx_v_exit_completion);

This "wait for anything" use of wait_for_completion looks wrong,=20 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?

powerpc/kernel/cpu/cpufreq/cf750gx.c line 533

Quoted 7 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0

+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.

Quoted 4 lines

+=A0=A0=A0=A0=A0=A0=A0dprintk("%s()\n", __func__); +

+=A0=A0=A0=A0=A0=A0=A0if (!cpu_has_feature(CPU_FTR_DUAL_PLL_750FX))
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0return 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.

Quoted 4 lines

+=A0=A0=A0=A0=A0=A0=A0if (cf750gx_v_freq_table =3D=3D NULL) {
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0ret =3D -ENOMEM;
+=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0=A0goto ErrSimple;
+=A0=A0=A0=A0=A0=A0=A0}

ret =3D -ENOMEM;
if (!cf750gx_freq_table)

	goto err_simple;


	Arnd <><

Kevin Diggs

author

2008-08-26 00:59 UTC #2 lore raw

Quoted 9 lines · Arnd Bergmann

Quoted 1 line · Kevin Diggs
+ * 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.

Quoted 9 lines

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.
Quoted 1 line
+ * 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.

Quoted 9 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 43
Quoted 5 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -0
+#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?

Quoted 11 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 50

Quoted 7 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +6 -0

+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

Quoted 8 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 57
Quoted 4 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +3 -0
+MODULE_AUTHOR("Kevin Diggs");
+MODULE_DESCRIPTION("750GX Dual PLL cpufreq driver");
+MODULE_LICENSE("GPL");
Move this to the end.

done

Quoted 10 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 63
Quoted 6 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +5 -0
+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.

Quoted 11 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +10 -0

+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,

Quoted 9 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 77
Quoted 3 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +3 -0
+       .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).

Quoted 17 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 89
Quoted 12 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +11 -0
+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?

Quoted 14 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 98
Quoted 9 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +8 -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;
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?

Quoted 12 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 106
Quoted 8 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0
+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?

Quoted 3 lines

Quoted 2 lines
+ cd = (struct cf750gx_t_call_data *)data;

done

Quoted 4 lines

data is a void pointer, so you don't need the cast, and shouldn't have it therefore.

Quoted 10 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 162
Quoted 6 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +6 -0
+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

Quoted 13 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 203

Quoted 8 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0

+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

Quoted 11 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 208
Quoted 6 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +5 -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;
more whitespace damage. Maybe there is something wrong with your text editor.

Nope, just a faulty programmer ...

Quoted 12 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +11 -0

+       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,

Quoted 9 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 224
Quoted 4 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -0
+                                               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.

Quoted 7 lines

Review
Quoted 3 lines
(quoted diff) +2 -0
+       if (active_pll) {
+       unsigned int current_state;
whitespace damage.

same here ...

Quoted 9 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 303
Quoted 3 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +2 -0
+       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.

Quoted 31 lines

Quoted 7 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +6 -0

+cf750gxTargetOut:
+       return result;
+
+cf750gxTargetNoFreq:
+       result = -ENODEV;
+

powerpc/kernel/cpu/cpufreq/cf750gx.c line 343

Quoted 10 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +10 -0

+       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;

Quoted 6 lines

out_unlock:

	clear_bit(cf750gxmChangingPllBit, &cf750gx_v_state_bits);
	complete(&cf750gx_v_exit_completion);
out:
	return result;

I'll fix this.

Quoted 9 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 372
Quoted 3 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +2 -0
+/*     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.

Quoted 13 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c new file +12 -0

+       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);

Quoted 9 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 389
Quoted 4 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -0
+
+       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.

Quoted 14 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 398
Quoted 10 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +9 -0
+       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.

Quoted 15 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 407
Quoted 10 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +9 -0
+
+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.

Quoted 2 lines

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).

Quoted 19 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 533
Quoted 8 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0
+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.
powerpc/kernel/cpu/cpufreq/cf750gx.c line 538
Quoted 4 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -0
+       dprintk("%s()\n", __func__);
+
+       if (!cpu_has_feature(CPU_FTR_DUAL_PLL_750FX))
+               return 0;

Quoted 5 lines

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?

Quoted 11 lines

powerpc/kernel/cpu/cpufreq/cf750gx.c line 630
Quoted 5 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -0
+       if (cf750gx_v_freq_table == NULL) {
+               ret = -ENOMEM;
+               goto ErrSimple;
+       }
ret = -ENOMEM;
if (!cf750gx_freq_table)
	goto err_simple;

done

Quoted 4 lines

	Arnd <><

Arnd Bergmann

2008-08-26 11:29 UTC #3 lore raw

Quoted 7 lines · Kevin Diggs

Quoted 4 lines · Arnd Bergmann
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.

Quoted 5 lines

Quoted 2 lines
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.

Quoted 16 lines

Quoted 7 lines
Quoted 7 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0
+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;
Quoted 9 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 89
Quoted 4 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +4 -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.

Quoted 3 lines

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;

Quoted 5 lines

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.

Quoted 2 lines

..._idle_pll_off is a boolean for a sysfs attribute. 0 is the default of disabled.

Quoted 3 lines

..._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.

Quoted 5 lines

..._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.

Quoted 14 lines

Quoted 5 lines
Quoted 5 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +5 -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;
Quoted 9 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 98
Quoted 3 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +3 -0
+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.

Quoted 3 lines

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.

Quoted 11 lines

Quoted 11 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 106
Quoted 7 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0
+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.

Quoted 3 lines

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'.

Quoted 11 lines

Quoted 8 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 303
Quoted 2 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +2 -0
+       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.

Quoted 11 lines

Quoted 10 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 389
Quoted 5 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +5 -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.

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.

Quoted 18 lines

Quoted 14 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 407
Quoted 8 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +8 -0
+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.
Quoted 2 lines
What exactly are you trying to accomplish with this?
Originall I had something like:

Quoted 8 lines

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 <><

Kevin Diggs

author

2008-08-27 09:14 UTC #4 lore raw

Quoted 10 lines · Arnd Bergmann

Quoted 8 lines · Kevin Diggs
Quoted 5 lines · Arnd Bergmann
On Monday 25 August 2008, Kevin Diggs wrote:
Most people list their email address here as well

Quoted 8 lines

Quoted 2 lines
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.

Quoted 4 lines

Quoted 4 lines
Quoted 3 lines
drop the _t here, or make explicit what is meant by it.

Quoted 9 lines

Quoted 3 lines
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.

Quoted 27 lines

Quoted 18 lines
Quoted 17 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 89
Quoted 12 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +11 -0
+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.
Quoted 3 lines
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

Quoted 7 lines

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?

Quoted 11 lines

Quoted 4 lines
..._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.

Quoted 4 lines

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?

Quoted 13 lines

Quoted 13 lines
Quoted 12 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 106
Quoted 8 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +7 -0
+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.

Quoted 8 lines

Quoted 3 lines
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.

Quoted 10 lines

Quoted 10 lines
Quoted 9 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 303
Quoted 3 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +2 -0
+       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.

Quoted 7 lines

Quoted 3 lines
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).

Quoted 12 lines

Quoted 12 lines
Quoted 11 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 389
Quoted 6 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +5 -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?

Quoted 7 lines

Quoted 1 line
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.

Quoted 34 lines

Quoted 19 lines
Quoted 14 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c line 407
Quoted 9 lines
powerpc/kernel/cpu/cpufreq/cf750gx.c new file +8 -0
+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.
Quoted 2 lines
What exactly are you trying to accomplish with this?
Originall I had something like:
Quoted 8 lines
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:

Quoted 9 lines

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.

Quoted 3 lines

	Arnd <><

Arnd Bergmann

2008-08-27 11:34 UTC #5 lore raw

Quoted 7 lines · Kevin Diggs

Quoted 4 lines · Arnd Bergmann
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.

Quoted 10 lines

Quoted 10 lines
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,
};

Quoted 5 lines

Quoted 2 lines
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.

Quoted 5 lines

Quoted 5 lines
Quoted 3 lines
..._min_max_mode is a boolean to hold the state of minmaxmode. Seems to be only used to print the current value.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 5 lines

Quoted 3 lines
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.

Quoted 14 lines

Quoted 5 lines
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:
Quoted 9 lines
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.

Quoted 6 lines

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 <><

Geert Uytterhoeven

2008-08-27 11:40 UTC #6 lore raw

Quoted 10 lines · Arnd Bergmann

Quoted 6 lines · Kevin Diggs
Quoted 3 lines · Arnd Bergmann
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

Brad Boyer

2008-08-27 16:09 UTC #7 lore raw

Quoted 6 lines · Geert Uytterhoeven

Quoted 5 lines · Arnd Bergmann
Quoted 4 lines · Kevin Diggs
Quoted 3 lines · Arnd Bergmann
Are there even SMP boards based on a 750? I thought only 74xx and 603/604 were SMP capable.

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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.

I remember going to a talk by some of the Be engineers, and I think they said that the 603 had terrible SMP performance. My understanding was that Motorola recommended the 604 for SMP configurations but the 603 was much cheaper and mostly worked. The 750 is much more like the 603 than the 604.

Brad Boyer
[email protected]

Geert Uytterhoeven

2008-08-27 16:18 UTC #8 lore raw

Quoted 13 lines · Brad Boyer

Quoted 12 lines · Geert Uytterhoeven
Quoted 10 lines · Arnd Bergmann
Quoted 6 lines · Kevin Diggs
Quoted 3 lines · Arnd Bergmann
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.

Quoted 6 lines

Quoted 1 line
The BeBox had a dual 603.
I remember going to a talk by some of the Be engineers, and I think they said that the 603 had terrible SMP performance. My understanding was that Motorola recommended the 604 for SMP configurations but the 603 was much cheaper and mostly worked. The 750 is much more like the 603 than the 604.

Yes, the 603 was not designed for SMP boxes. But that didn't mean nobody had the courage to (mis)use them that way ;-)

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

Kevin Diggs

author

2008-08-27 21:03 UTC #9 lore raw

In reply to Arnd Bergmann

Quoted 13 lines · Arnd Bergmann

Quoted 9 lines · Kevin Diggs
Quoted 4 lines · Arnd Bergmann
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.

Quoted 9 lines

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".

Quoted 18 lines

Quoted 10 lines
Quoted 10 lines
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,
};
Quoted 6 lines
Quoted 2 lines
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?

Quoted 5 lines

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?

Quoted 16 lines

Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
..._min_max_mode is a boolean to hold the state of minmaxmode. Seems to be only used to print the current value.
Quoted 8 lines
Quoted 5 lines
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.

Quoted 9 lines

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.

Quoted 24 lines

Quoted 16 lines
Quoted 6 lines
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:
Quoted 9 lines
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.
Quoted 6 lines
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.

Quoted 4 lines

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?

Quoted 3 lines

	Arnd <><

Arnd Bergmann

2008-08-28 07:46 UTC #10 lore raw

Quoted 7 lines · Kevin Diggs

Quoted 6 lines · Arnd Bergmann
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.

Quoted 6 lines

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.

Quoted 13 lines

Quoted 4 lines
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.

Quoted 5 lines

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 <><

Kevin Diggs

author

2008-08-28 16:36 UTC #11 lore raw

Quoted 32 lines · Arnd Bergmann

Quoted 16 lines · Kevin Diggs
Quoted 4 lines · Arnd Bergmann
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.
Quoted 5 lines
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.

Quoted 4 lines

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.

Quoted 3 lines

	Arnd <><

Thanks for taking the time to review and for the comments!

kevin

Kevin Diggs

author

2008-08-27 21:06 UTC #12 lore raw

In reply to Arnd Bergmann

Quoted 113 lines · Arnd Bergmann

Quoted 9 lines · Kevin Diggs
Quoted 4 lines · Arnd Bergmann
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.
Quoted 9 lines
Quoted 9 lines
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,
};
Quoted 6 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 5 lines
Quoted 3 lines
..._min_max_mode is a boolean to hold the state of minmaxmode. Seems to be only used to print the current value.
Quoted 8 lines
Quoted 5 lines
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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 15 lines
Quoted 5 lines
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:
Quoted 9 lines
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.
Quoted 6 lines
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 <><