Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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 ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
On Wed, Aug 27, 2008 at 01:40:10PM +0200, Geert Uytterhoeven wrote: 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. 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] ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
On Wed, 27 Aug 2008, Brad Boyer wrote: On Wed, Aug 27, 2008 at 01:40:10PM +0200, Geert Uytterhoeven wrote: 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. 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___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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
___
Linuxppc-dev mailing list
Linuxppc-dev@ozlabs.org
https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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
[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] Index: Documentation/DocBook/cf750gx.tmpl === --- /dev/null 2004-08-10 18:55:00.0 -0700 +++ Documentation/DocBook/cf750gx.tmpl 2008-08-20 10:00:14.0 -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 + titlePowerPC 750GX (and FX?) cpufreq driver guide/title + + authorgroup + author +firstnameKevin/firstname +surnameDiggs/surname + /author + /authorgroup + +revhistory + revision + revnumber1.0nbsp;/revnumber + dateAugust 12, 2008/date + revremarkInitial revision posted to linuxppc-dev/revremark + /revision +/revhistory + + copyright + year2008/year + holderKevin 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 + titleIntroduction/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 + titleMajor 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 + titlecf750gx - 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 +
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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(0x8000)
+#define cf750gxmChangingPllBit (31)
+#define cf750gxmTurningIdlePllOff (0x4000)
+#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 = 40,
+ .max_core = 80,
+};
+
+static const struct pll_750fgx_t __initdata pll_750gx = {
+ .min_ratio = 2,
+ .max_ratio = 20,
+ .min_core = 50,
+ .max_core = 100,
+};
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);
+
+
Re: [PATCH 2/4] Add cpufreq driver for the IBM PowerPC 750GX
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(0x8000)
+#define cf750gxmChangingPllBit (31)
+#define cf750gxmTurningIdlePllOff (0x4000)
+#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 = 40,
+ .max_core = 80,
+};
+
+static const struct pll_750fgx_t __initdata pll_750gx = {
+ .min_ratio = 2,
+ .max_ratio = 20,
+ .min_core = 50,
+ .max_core = 100,
+};
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.
