Re: [PATCH RESEND v4 3/4] cpufreq: powernv: Add a trace print for the throttle event

2016-01-12 Thread Gautham R Shenoy 
Hi Shilpa,

Just saw this resend!

On Tue, Jan 12, 2016 at 04:24:26AM -0600, Shilpasri G Bhat wrote:
> Record the throttle event with a trace print replacing the printk,
> except for events like throttling below nominal and occ reset
> event which print a warning message.
> 
> Signed-off-by: Shilpasri G Bhat 
> ---

[..snip..]

> 
> -static void powernv_cpufreq_throttle_check(void *data)
> +static void powernv_cpufreq_check_pmax(void)
   ^^^
This function only contains code moved from
powernv_cpufreq_throttle_check with pr_crit/pr_warns replaced by
trace_powernv_throttle. Furthermore, it is not called from any other
place. Given that the original function was ~60 lines do we really
need to split it into two separate functions ? If yes, could it be an
inline function ?

>  {
>   unsigned int cpu = smp_processor_id();
>   unsigned int chip_id = pir_to_chip_id(hard_smp_processor_id());
> - unsigned long pmsr;
>   int pmsr_pmax, i;
> 
> - pmsr = get_pmspr(SPRN_PMSR);
> + pmsr_pmax = (s8)PMSR_MAX(get_pmspr(SPRN_PMSR));
> 
>   for (i = 0; i < nr_chips; i++)
>   if (chips[i].id == chip_id)
>   break;
> 
> - /* Check for Pmax Capping */
> - pmsr_pmax = (s8)PMSR_MAX(pmsr);
>   if (pmsr_pmax != powernv_pstate_info.max) {
>   if (chips[i].throttled)
> - goto next;
> + return;
> +
>   chips[i].throttled = true;
>   if (pmsr_pmax < powernv_pstate_info.nominal)
> - pr_crit("CPU %d on Chip %u has Pmax reduced below 
> nominal frequency (%d < %d)\n",
> - cpu, chips[i].id, pmsr_pmax,
> - powernv_pstate_info.nominal);
> - else
> - pr_info("CPU %d on Chip %u has Pmax reduced below turbo 
> frequency (%d < %d)\n",
> - cpu, chips[i].id, pmsr_pmax,
> - powernv_pstate_info.max);
> + pr_warn_once("CPU %d on Chip %u has Pmax reduced below 
> nominal frequency (%d < %d)\n",
> +  cpu, chips[i].id, pmsr_pmax,
> +  powernv_pstate_info.nominal);
> +
> + trace_powernv_throttle(chips[i].id,
> +throttle_reason[chips[i].throt_reason],
> +pmsr_pmax);
>   } else if (chips[i].throttled) {
>   chips[i].throttled = false;
> - pr_info("CPU %d on Chip %u has Pmax restored to %d\n", cpu,
> - chips[i].id, pmsr_pmax);
> + trace_powernv_throttle(chips[i].id,
> +throttle_reason[chips[i].throt_reason],
> +pmsr_pmax);
>  }
> +}
> +
> +static void powernv_cpufreq_throttle_check(void *data)
> +{
> + unsigned long pmsr;
> +
> + pmsr = get_pmspr(SPRN_PMSR);
> +
> + /* Check for Pmax Capping */
> + powernv_cpufreq_check_pmax();
  
If you want to retain this function, you could pass pmsr as an
argument instead of computing it afresh in
powernv_cpufreq_check_pmax()

>   /* Check if Psafe_mode_active is set in PMSR. */
> -next:
>   if (pmsr & PMSR_PSAFE_ENABLE) {
>   throttled = true;
>   pr_info("Pstate set to safe frequency\n");

--
Thanks and Regards
gautham.

___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev

[PATCH RESEND v4 3/4] cpufreq: powernv: Add a trace print for the throttle event

2016-01-12 Thread Shilpasri G Bhat 
Record the throttle event with a trace print replacing the printk,
except for events like throttling below nominal and occ reset
event which print a warning message.

Signed-off-by: Shilpasri G Bhat 
---
Changes from v3:
- Separate this patch to contain trace_point changes
- Move struct chip member 'restore' of type bool above 'mask' to reduce
  structure padding.

No changes from v2.

Changes from v1:
- As suggested by Paul Clarke replaced char * throttle_reason[][30] by 
  const char * const throttle_reason[].

 drivers/cpufreq/powernv-cpufreq.c | 95 ---
 1 file changed, 49 insertions(+), 46 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c 
b/drivers/cpufreq/powernv-cpufreq.c
index 597a084..c98a6e7 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -28,6 +28,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -44,12 +45,22 @@
 static struct cpufreq_frequency_table powernv_freqs[POWERNV_MAX_PSTATES+1];
 static bool rebooting, throttled, occ_reset;
 
+static const char * const throttle_reason[] = {
+   "No throttling",
+   "Power Cap",
+   "Processor Over Temperature",
+   "Power Supply Failure",
+   "Over Current",
+   "OCC Reset"
+};
+
 static struct chip {
unsigned int id;
bool throttled;
+   bool restore;
+   u8 throt_reason;
cpumask_t mask;
struct work_struct throttle;
-   bool restore;
 } *chips;
 
 static int nr_chips;
@@ -310,41 +321,49 @@ static inline unsigned int get_nominal_index(void)
return powernv_pstate_info.max - powernv_pstate_info.nominal;
 }
 
-static void powernv_cpufreq_throttle_check(void *data)
+static void powernv_cpufreq_check_pmax(void)
 {
unsigned int cpu = smp_processor_id();
unsigned int chip_id = pir_to_chip_id(hard_smp_processor_id());
-   unsigned long pmsr;
int pmsr_pmax, i;
 
-   pmsr = get_pmspr(SPRN_PMSR);
+   pmsr_pmax = (s8)PMSR_MAX(get_pmspr(SPRN_PMSR));
 
for (i = 0; i < nr_chips; i++)
if (chips[i].id == chip_id)
break;
 
-   /* Check for Pmax Capping */
-   pmsr_pmax = (s8)PMSR_MAX(pmsr);
if (pmsr_pmax != powernv_pstate_info.max) {
if (chips[i].throttled)
-   goto next;
+   return;
+
chips[i].throttled = true;
if (pmsr_pmax < powernv_pstate_info.nominal)
-   pr_crit("CPU %d on Chip %u has Pmax reduced below 
nominal frequency (%d < %d)\n",
-   cpu, chips[i].id, pmsr_pmax,
-   powernv_pstate_info.nominal);
-   else
-   pr_info("CPU %d on Chip %u has Pmax reduced below turbo 
frequency (%d < %d)\n",
-   cpu, chips[i].id, pmsr_pmax,
-   powernv_pstate_info.max);
+   pr_warn_once("CPU %d on Chip %u has Pmax reduced below 
nominal frequency (%d < %d)\n",
+cpu, chips[i].id, pmsr_pmax,
+powernv_pstate_info.nominal);
+
+   trace_powernv_throttle(chips[i].id,
+  throttle_reason[chips[i].throt_reason],
+  pmsr_pmax);
} else if (chips[i].throttled) {
chips[i].throttled = false;
-   pr_info("CPU %d on Chip %u has Pmax restored to %d\n", cpu,
-   chips[i].id, pmsr_pmax);
+   trace_powernv_throttle(chips[i].id,
+  throttle_reason[chips[i].throt_reason],
+  pmsr_pmax);
}
+}
+
+static void powernv_cpufreq_throttle_check(void *data)
+{
+   unsigned long pmsr;
+
+   pmsr = get_pmspr(SPRN_PMSR);
+
+   /* Check for Pmax Capping */
+   powernv_cpufreq_check_pmax();
 
/* Check if Psafe_mode_active is set in PMSR. */
-next:
if (pmsr & PMSR_PSAFE_ENABLE) {
throttled = true;
pr_info("Pstate set to safe frequency\n");
@@ -358,7 +377,7 @@ next:
 
if (throttled) {
pr_info("PMSR = %16lx\n", pmsr);
-   pr_crit("CPU Frequency could be throttled\n");
+   pr_warn("CPU Frequency could be throttled\n");
}
 }
 
@@ -449,15 +468,6 @@ void powernv_cpufreq_work_fn(struct work_struct *work)
}
 }
 
-static char throttle_reason[][30] = {
-   "No throttling",
-   "Power Cap",
-   "Processor Over Temperature",
-   "Power Supply Failure",
-   "Over Current",
-   "OCC Reset"
-};
-
 static int