RE: [PATCH] perf: add OMAP support for the new power events

[Santosh Shilimkar <santosh.shilimkar@ti.com>]

> -----Original Message-----
> From: Jean Pihet [mailto:jean.pihet@newoldbits.com]
> Sent: Monday, February 21, 2011 2:14 PM
> To: Santosh Shilimkar
> Cc: Kevin Hilman; Thomas Renninger; linux-omap@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org; Jean Pihet-XID
> Subject: Re: [PATCH] perf: add OMAP support for the new power events
>
> Hi Santosh,
>

[...]

> >> diff --git a/arch/arm/mach-omap2/pm34xx.c b/arch/arm/mach-
> >> omap2/pm34xx.c
> >> index 2f864e4..d1cc3f4 100644
> >> --- a/arch/arm/mach-omap2/pm34xx.c
> >> +++ b/arch/arm/mach-omap2/pm34xx.c
> >> @@ -29,6 +29,7 @@
> >>  #include <linux/delay.h>
> >>  #include <linux/slab.h>
> >>  #include <linux/console.h>
> >> +#include <trace/events/power.h>
> >>
> >>  #include <plat/sram.h>
> >>  #include "clockdomain.h"
> >> @@ -519,8 +520,14 @@ static void omap3_pm_idle(void)
> >>       if (omap_irq_pending() || need_resched())
> >>               goto out;
> >>
> >> +     trace_power_start(POWER_CSTATE, 1, smp_processor_id());
> >> +     trace_cpu_idle(1, smp_processor_id());
> >> +
> >
> > This default idle code won't be used when you enable the
> > CONFIG_CPUIDLE. That case the cpuidle34xx.c idle code gets
> > registered.
> That is correct. OMAP has a default idle handler (omap3_pm_idle) and
> a
> cpuidle handler (omap3_enter_idle in
> arch/arm/mach-omap2/cpuidle34xx.c).
>
> > Shouldn't you patch that code instead? This is more or less
> > dead code and it is just like default idle code when idle
> > drivers isn't registered.
> The cpuidle framework already is instrumented in a generic way. This
> code adds the instrumentation to the default idle handler so that
> all
> cases are covered. BTW the patch description gives that information.
>
> If there is dead code then it is not only the code from this patch
> but
> all the code for the default idle handler.
>
I read your change log. It says.

>> The trace points are for:
>> - default idle handler. Since the cpuidle framework is
>>   instrumented in the generic way there is no need to
>>   add trace points in the OMAP specific cpuidle handler;
Now code in cpuilde34xx.c is also OMAP specific and hence the
confusion at least for me.
Regarding dead code, I meant existing code of default handler.

Thanks for clarification.

Regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html