Re: [PATCH] oprofile: VR5500 performance counter driver

[Robert Richter <robert.richter@amd.com>]

On 23.02.09 11:26:34, Mark Asselstine wrote:
> This is inspired by op_model_mipsxx.c with some modification
> in regards to register layout and overflow handling. This has
> been tested on a NEC VR5500 board and shown to produce sane
> results.

Mark,

I have looked at the differences between the VR5500 code and the
generic in op_model_mipsxx.c. If I am not wrong, only the interrupt
handling is different. This affects only vr5500_reg_setup() and
vr5500_perfcount_handler(). I think it would be better to implement
cpu checks in the generic functions or remap to cpu specific functions
during mipsxx_init(). This extension of the generic code is much more
maintainable. Also, there is less code in the end. See also my
comments below.

-Robert

> 
> Signed-off-by: Mark Asselstine <mark.asselstine@windriver.com>
> 
> diff --git a/arch/mips/oprofile/Makefile b/arch/mips/oprofile/Makefile
> index cfd4b60..977a828 100644
> --- a/arch/mips/oprofile/Makefile
> +++ b/arch/mips/oprofile/Makefile
> @@ -14,4 +14,5 @@ oprofile-$(CONFIG_CPU_MIPS32)		+= op_model_mipsxx.o
>  oprofile-$(CONFIG_CPU_MIPS64)		+= op_model_mipsxx.o
>  oprofile-$(CONFIG_CPU_R10000)		+= op_model_mipsxx.o
>  oprofile-$(CONFIG_CPU_SB1)		+= op_model_mipsxx.o
> +oprofile-$(CONFIG_CPU_VR5500)		+= op_model_vr5500.o

I could not find a Kconfig option for this.

>  oprofile-$(CONFIG_CPU_RM9000)		+= op_model_rm9000.o
> diff --git a/arch/mips/oprofile/common.c b/arch/mips/oprofile/common.c
> index e1bffab..68aad99 100644
> --- a/arch/mips/oprofile/common.c
> +++ b/arch/mips/oprofile/common.c
> @@ -17,6 +17,7 @@
>  
>  extern struct op_mips_model op_model_mipsxx_ops __attribute__((weak));
>  extern struct op_mips_model op_model_rm9000_ops __attribute__((weak));
> +extern struct op_mips_model op_model_vr5500_ops __attribute__((weak));
>  
>  static struct op_mips_model *model;
>  
> @@ -94,6 +95,10 @@ int __init oprofile_arch_init(struct oprofile_operations *ops)
>  	case CPU_RM9000:
>  		lmodel = &op_model_rm9000_ops;
>  		break;
> +
> +	case CPU_R5500:
> +		lmodel = &op_model_vr5500_ops;
> +		break;

Is there a reason for using _vr5500_ instead of _r5500_ for the
function and the filename?

>  	};
>  
>  
> diff --git a/arch/mips/oprofile/op_model_vr5500.c b/arch/mips/oprofile/op_model_vr5500.c
> new file mode 100644
> index 0000000..75fae6a
> --- /dev/null
> +++ b/arch/mips/oprofile/op_model_vr5500.c
> @@ -0,0 +1,179 @@
> +/*
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file "COPYING" in the main directory of this archive
> + * for more details.
> + *

Copyright statements from op_model_mipsxx.c should be added here.

> + * Copyright (c) 2009 Wind River Systems, Inc.
> + */
> +#include <linux/cpumask.h>
> +#include <linux/oprofile.h>
> +#include <linux/interrupt.h>
> +#include <linux/smp.h>
> +#include <asm/irq_regs.h>
> +
> +#include "op_impl.h"
> +
> +#define M_PERFCTL_EXL			(1UL      <<  0)
> +#define M_PERFCTL_KERNEL		(1UL      <<  1)
> +#define M_PERFCTL_SUPERVISOR		(1UL      <<  2)
> +#define M_PERFCTL_USER			(1UL      <<  3)
> +#define M_PERFCTL_INTERRUPT_ENABLE	(1UL      <<  4)
> +#define M_PERFCTL_INTERRUPT		(1UL      <<  5)
> +#define M_PERFCTL_EVENT(event)		(((event) & 0xf)  << 6)
> +#define M_PERFCTL_COUNT_ENABLE		(1UL      <<  10)
> +
> +#define NUM_COUNTERS                    2
> +
> +static int (*save_perf_irq) (void);
> +
> +#define __define_perf_accessors(r, n)    				\
> +									\
> +	static inline unsigned int r_c0_ ## r ## n(void)		\
> +	{								\
> +		return read_c0_ ## r ## n();				\
> +	}								\
> +									\
> +	static inline void w_c0_ ## r ## n(unsigned int value)		\
> +	{								\
> +		write_c0_ ## r ## n(value);				\
> +	}								\
> +
> +__define_perf_accessors(perfcntr, 0)
> +__define_perf_accessors(perfcntr, 1)
> +
> +__define_perf_accessors(perfctrl, 0)
> +__define_perf_accessors(perfctrl, 1)

I know this code is borrowed, but why not use write/read_c0_perfXXXX()
directly?

> +
> +struct op_mips_model op_model_vr5500_ops;
> +
> +static struct vr5500_register_config {
> +	unsigned int control[NUM_COUNTERS];
> +	unsigned int counter[NUM_COUNTERS];
> +} reg;
> +
> +/* Compute all of the registers in preparation for enabling profiling.  */
> +static void vr5500_reg_setup(struct op_counter_config *ctr)
> +{
> +	int i;
> +	unsigned int counters = NUM_COUNTERS;
> +
> +	/* Compute the performance counter control word.  */
> +	for (i = 0; i < counters; i++) {
> +		reg.control[i] = 0;
> +		reg.counter[i] = 0;
> +
> +		if (!ctr[i].enabled)
> +			continue;
> +
> +		reg.control[i] = M_PERFCTL_EVENT(ctr[i].event) |
> +		    M_PERFCTL_INTERRUPT_ENABLE | M_PERFCTL_COUNT_ENABLE;
> +		if (ctr[i].kernel)
> +			reg.control[i] |= M_PERFCTL_KERNEL;
> +		if (ctr[i].user)
> +			reg.control[i] |= M_PERFCTL_USER;
> +		if (ctr[i].exl)
> +			reg.control[i] |= M_PERFCTL_EXL;
> +
> +		reg.counter[i] = 0xffffffff - ctr[i].count + 1;
> +	}
> +}
> +
> +/* Program all of the registers in preparation for enabling profiling.  */
> +static void vr5500_cpu_setup(void *args)
> +{
> +	w_c0_perfctrl1(0);
> +	w_c0_perfcntr1(reg.counter[1]);
> +
> +	w_c0_perfctrl0(0);
> +	w_c0_perfcntr0(reg.counter[0]);
> +}
> +
> +/* Start all counters on current CPU */
> +static void vr5500_cpu_start(void *args)
> +{
> +	w_c0_perfctrl1(reg.control[1]);
> +	w_c0_perfctrl0(reg.control[0]);
> +}
> +
> +/* Stop all counters on current CPU */
> +static void vr5500_cpu_stop(void *args)
> +{
> +	w_c0_perfctrl1(0);
> +	w_c0_perfctrl0(0);
> +}
> +
> +static int vr5500_perfcount_handler(void)
> +{
> +	unsigned int control;
> +	unsigned int counter;
> +	int handled = IRQ_NONE;
> +	unsigned int counters = NUM_COUNTERS;
> +
> +	if (cpu_has_mips_r2 && !(read_c0_cause() & (1 << 26)))

Do not use magic numbers.

> +		return handled;
> +
> +	switch (counters) {

Since counters is a fix value the switch/case could be removed.

> +	#define HANDLE_COUNTER(n) 					\
> +	case n + 1:							\
> +		control = r_c0_perfctrl ## n();				\
> +		counter = r_c0_perfcntr ## n();				\
> +		if ((control & M_PERFCTL_INTERRUPT_ENABLE) &&		\
> +			(control & M_PERFCTL_INTERRUPT)) {		\
> +			oprofile_add_sample(get_irq_regs(), n);		\
> +			w_c0_perfcntr ## n(reg.counter[n]);		\
> +			w_c0_perfctrl ## n(control & ~M_PERFCTL_INTERRUPT); \
> +			handled = IRQ_HANDLED;				\
> +		}
> +	HANDLE_COUNTER(1)
> +	HANDLE_COUNTER(0)
> +	}

It is hard to see a loop here. I would like to prefer programming c
instead of macros unless there is a good reason to do so. Also, this
causes a checkpatch error.

> +
> +	return handled;
> +}
> +
> +static void reset_counters(void *arg)
> +{
> +	w_c0_perfctrl1(0);
> +	w_c0_perfcntr1(0);
> +
> +	w_c0_perfctrl0(0);
> +	w_c0_perfcntr0(0);
> +}
> +
> +static int __init vr5500_init(void)
> +{
> +	on_each_cpu(reset_counters, NULL, 1);
> +
> +	switch (current_cpu_type()) {
> +	case CPU_R5500:
> +		op_model_vr5500_ops.cpu_type = "mips/vr5500";
> +		break;
> +
> +	default:
> +		printk(KERN_ERR "Profiling unsupported for this CPU\n");
> +
> +		return -ENODEV;
> +	}
> +
> +	save_perf_irq = perf_irq;
> +	perf_irq = vr5500_perfcount_handler;
> +
> +	return 0;
> +}
> +
> +static void vr5500_exit(void)
> +{
> +	on_each_cpu(reset_counters, NULL, 1);
> +
> +	perf_irq = save_perf_irq;
> +}
> +
> +struct op_mips_model op_model_vr5500_ops = {
> +	.reg_setup = vr5500_reg_setup,
> +	.cpu_setup = vr5500_cpu_setup,
> +	.init = vr5500_init,
> +	.exit = vr5500_exit,
> +	.cpu_start = vr5500_cpu_start,
> +	.cpu_stop = vr5500_cpu_stop,
> +	.num_counters = NUM_COUNTERS,

Please align this vertically.

> +};
> 
> ------------------------------------------------------------------------------
> Open Source Business Conference (OSBC), March 24-25, 2009, San Francisco, CA
> -OSBC tackles the biggest issue in open source: Open Sourcing the Enterprise
> -Strategies to boost innovation and cut costs with open source participation
> -Receive a $600 discount off the registration fee with the source code: SFAD
> http://p.sf.net/sfu/XcvMzF8H
> _______________________________________________
> oprofile-list mailing list
> oprofile-list@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/oprofile-list
> 

-- 
Advanced Micro Devices, Inc.
Operating System Research Center
email: robert.richter@amd.com