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
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: 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
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
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
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, 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 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
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
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. >
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 there
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, > +