Hi Azuma-san,
On Thu August 30 2007 6:18 am, takaki.azuma wrote:
> The patch replaces rtas_reset_signals() and rtas_activate_signals()
> with pfm_cell_reset_signals() and pfm_cell_activate_signals() respectively
> according to the platform identification. Moreover, there is a restriction
> in the combination in PM signal group of Cell. The pfm_cell_pmc_check()
> for the combination check is registered at the same time.
The general approach here is ok, but I would keep this limited to the
perfmon_cell files. Since the other powerpc models do not have a concept
of "activating signals", we probably don't want to add these function
pointers to the pfm_arch_pmu_info structure. Also, we might want to move the
existing "rtas" routines into the new perfmon_cell_signals.c to emphasize
that they are performing similar actions. I'm working on a slight
modification to this patch to show you what I mean. I'll post it later today
if I finish it.
> -- Takaki Azuma
>
> Signed-off-by: Takaki Azuma <[EMAIL PROTECTED]>
> Signed-off-by: Takayuki Uchikawa <[EMAIL PROTECTED]>
>
> Index: linux-2.6/include/asm-powerpc/perfmon.h
> ===================================================================
> --- linux-2.6.git/include/asm-powerpc/perfmon.h
> +++ linux-2.6/include/asm-powerpc/perfmon.h
> @@ -42,6 +42,8 @@ enum powerpc_pmu_type {
> PFM_POWERPC_PMU_CELL,
> };
>
> +typedef struct cell_rtas_arg * struct_cell_rtas_arg;
> +
Don't use typedef's in the kernel.
> struct pfm_arch_pmu_info {
> enum powerpc_pmu_type pmu_style;
>
> @@ -74,6 +76,11 @@ struct pfm_arch_pmu_info {
> struct task_struct *task);
> int (*unload_context)(struct pfm_context *ctx,
> struct task_struct *task);
> +
> +#ifdef CONFIG_PPC_CELL
> + int (*reset_signals)(u32 cpu);
> + int (*activate_signals)(struct_cell_rtas_arg signals, int num_signals);
> +#endif
> };
>
> #ifdef CONFIG_PPC32
> @@ -367,5 +374,19 @@ struct pfm_arch_context {
> #define PFM_ARCH_SMPL_ALIGN_SIZE 0
>
>
> +static inline int pfm_arch_reset_signals(int cpu)
> +{
> + struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
> +
> + return arch_info->reset_signals(cpu);
> +}
> +
> +static inline int pfm_arch_activate_signals(struct_cell_rtas_arg signals,
> int num_signals) +{
Be sure to keep lines to <= 80 characters. See the Documentation/CodingStyle
and scripts/Lindent files in the kernel source tree for more details.
> + struct pfm_arch_pmu_info *arch_info = pfm_pmu_conf->arch_info;
> +
> + return arch_info->activate_signals(signals, num_signals);
> +}
> +
> #endif /* __KERNEL__ */
> #endif /* _ASM_POWERPC_PERFMON_H_ */
> Index: linux-2.6/arch/powerpc/perfmon/Makefile
> ===================================================================
> --- linux-2.6.git/arch/powerpc/perfmon/Makefile
> +++ linux-2.6/arch/powerpc/perfmon/Makefile
> @@ -1,4 +1,4 @@
> obj-$(CONFIG_PERFMON) += perfmon.o
> obj-$(CONFIG_PERFMON_POWER5) += perfmon_power5.o
> obj-$(CONFIG_PERFMON_PPC32) += perfmon_ppc32.o
> -obj-$(CONFIG_PERFMON_CELL) += perfmon_cell.o
> +obj-$(CONFIG_PERFMON_CELL) += perfmon_cell.o perfmon_cell_signals.o
> Index: linux-2.6/arch/powerpc/perfmon/perfmon_cell.c
> ===================================================================
> --- linux-2.6.git/arch/powerpc/perfmon/perfmon_cell.c
> +++ linux-2.6/arch/powerpc/perfmon/perfmon_cell.c
> @@ -28,7 +28,10 @@
> #include <asm/cell-pmu.h>
> #include <asm/io.h>
> #include <asm/rtas.h>
> +#include <asm/machdep.h>
> +#include <asm/firmware.h>
> #include "../platforms/cell/cbe_regs.h"
> +#include "./perfmon_cell_signals.h"
>
> MODULE_AUTHOR("Kevin Corry <[EMAIL PROTECTED]>, "
> "Carl Love <[EMAIL PROTECTED]>");
> @@ -205,7 +208,7 @@ static void write_pm07_event(int cpu, un
> signal.signal_group = signal_number / 100;
> signal.bit = signal_number % 100;
>
> - rc = rtas_activate_signals(&signal, 1);
> + rc = pfm_arch_activate_signals(&signal, 1);
> if (rc) {
> PFM_WARN("%s(%d, %u, %lu): Error calling "
> "rtas_activate_signal(): %d\n", __FUNCTION__,
> @@ -352,7 +355,7 @@ void pfm_cell_restore_pmcs(struct pfm_ev
> num_used++;
> }
>
> - rc = rtas_activate_signals(signals, num_used);
> + rc = pfm_arch_activate_signals(signals, num_used);
> if (rc) {
> PFM_WARN("Error calling rtas_activate_signal(): %d\n", rc);
> /* FIX: We will also need this routine to be able to return
> @@ -382,7 +385,7 @@ static int pfm_cell_unload_context(struc
> struct task_struct *task)
> {
> if (task == current || ctx->flags.system) {
> - rtas_reset_signals(smp_processor_id());
> + pfm_arch_reset_signals(smp_processor_id());
> }
> return 0;
> }
> @@ -397,7 +400,7 @@ static int pfm_cell_unload_context(struc
> int pfm_cell_ctxswout_thread(struct task_struct *task,
> struct pfm_context *ctx, struct pfm_event_set *set)
> {
> - rtas_reset_signals(smp_processor_id());
> + pfm_arch_reset_signals(smp_processor_id());
> return 0;
> }
>
> @@ -553,6 +556,10 @@ static struct pfm_arch_pmu_info pfm_cell
> .restore_pmcs = pfm_cell_restore_pmcs,
> .ctxswout_thread = pfm_cell_ctxswout_thread,
> .unload_context = pfm_cell_unload_context,
> +#ifdef CONFIG_PPC_CELL
> + .reset_signals = rtas_reset_signals,
> + .activate_signals = rtas_activate_signals,
> +#endif
The #ifdef CONFIG_PPC_CELL is not necessary. The entire perfmon_cell.c file is
only built if CONFIG_PPC_CELL is defined.
> };
>
> static struct pfm_pmu_config pfm_cell_pmu_conf = {
> @@ -569,8 +576,25 @@ static struct pfm_pmu_config pfm_cell_pm
> .owner = THIS_MODULE,
> };
>
> +static inline void pfm_cell_platform_probe(void)
> +{
> + if (machine_is(celleb)) {
> + int cnum;
> +
> + pfm_cell_pmu_info.reset_signals = pfm_cell_reset_signals;
> + pfm_cell_pmu_info.activate_signals = pfm_cell_activate_signals;
> + pfm_cell_pmu_conf.pmc_write_check = pfm_cell_pmc_check;
> + for (cnum = NR_CTRS; cnum < (NR_CTRS * 2); cnum++)
> + pfm_cell_pmc_desc[cnum].type |= PFM_REG_WC;
> + }
> +}
> +
> static int __init pfm_cell_pmu_init_module(void)
> {
> +#ifdef CONFIG_PPC_CELL
> + pfm_cell_platform_probe();
> +#endif
> +
Same here with CONFIG_PPC_CELL.
> return pfm_pmu_register(&pfm_cell_pmu_conf);
> }
>
Thanks,
--
Kevin Corry
[EMAIL PROTECTED]
http://www.ibm.com/linux/
_______________________________________________
perfmon mailing list
[email protected]
http://www.hpl.hp.com/hosted/linux/mail-archives/perfmon/
