nds32: Perf support

arch/nds32/include/asm/pmu.h new file +15 -0

+#define PFM_CTL_OVF(idx)             PFM_CTL_mskOVF ## idx
+#define PFM_CTL_EN(idx)                      PFM_CTL_mskEN ## idx
+#define PFM_CTL_OFFSEL(idx)          PFM_CTL_offSEL ## idx
+#define PFM_CTL_IE(idx)                      PFM_CTL_mskIE ## idx
+#define PFM_CTL_KS(idx)                      PFM_CTL_mskKS ## idx
+#define PFM_CTL_KU(idx)                      PFM_CTL_mskKU ## idx
+#define PFM_CTL_SEL(idx)             PFM_CTL_mskSEL ## idx
+
+#define macro_expansion(macro_name, var, idx) do { \
+     switch (idx) { \
+     case 0:\
+             var = macro_name ## 0; \
+             break; \
+     case 1:\
+             var = macro_name ## 1; \

+ * @runtime_resume: an optional handler which will be called by the

+ *   runtime PM framework following a call to pm_runtime_get().
+ *   Note that if pm_runtime_get() is called more than once in
+ *   succession this handler will only be called once.
+ * @runtime_suspend: an optional handler which will be called by the
+ *   runtime PM framework following a call to pm_runtime_put().
+ *   Note that if pm_runtime_get() is called more than once in
+ *   succession this handler will only be called following the
+ *   final call to pm_runtime_put() that actually disables the
+ *   hardware.

+ */

arch/nds32/include/asm/pmu.h new file +16 -0

+struct nds32_pmu_platdata {
+     irqreturn_t (*handle_irq)(int irq, void *dev,
+                               irq_handler_t pmu_handler);
+     int (*runtime_resume)(struct device *dev);
+     int (*runtime_suspend)(struct device *dev);
+};
+
+/* The events for a given PMU register set. */
+struct pmu_hw_events {
+     /*
+      * The events that are active on the PMU for the given index.
+      */
+     struct perf_event **events;
+
+     /*
+      * A 1 bit for an index indicates that the counter is being used for

arch/nds32/include/asm/pmu.h new file +9 -0

+     char *name;
+      irqreturn_t (*handle_irq)(int irq_num, void *dev);
+     void (*enable)(struct perf_event *event);
+     void (*disable)(struct perf_event *event);
+     int (*get_event_idx)(struct pmu_hw_events *hw_events,
+                          struct perf_event *event);
+     int (*set_event_filter)(struct hw_perf_event *evt,
+                             struct perf_event_attr *attr);
+     u32 (*read_counter)(struct perf_event *event);

+ u64 max_period;

arch/nds32/include/asm/pmu.h new file +6 -0

+
+/* Get converted hardware event number */
+#define GET_CONVERTED_EVENT_HW_NUM(event) do { \
+     if ((event) == 0) { \
+             /*do nothing*/    \
+     } else if ((event) > SPAV3_0_SEL_BASE && event < SPAV3_0_SEL_LAST) { \

arch/nds32/kernel/perf_event_cpu.c new file +50 -0

+static irqreturn_t nds32_pmu_handle_irq(int irq_num, void *dev)
+{
+     u32 pfm;
+     struct perf_sample_data data;
+     struct nds32_pmu *cpu_pmu = (struct nds32_pmu *)dev;
+     struct pmu_hw_events *cpuc = cpu_pmu->get_hw_events();
+     struct pt_regs *regs;
+     int idx;
+     /*
+      * Get and reset the IRQ flags
+      */
+     pfm = nds32_pfm_getreset_flags();
+
+     /*
+      * Did an overflow occur?
+      */
+     if (!nds32_pfm_has_overflowed(pfm))
+             return IRQ_NONE;
+
+     /*
+      * Handle the counter(s) overflow(s)
+      */
+     regs = get_irq_regs();
+
+     for (idx = 0; idx < cpu_pmu->num_events; ++idx) {
+             struct perf_event *event = cpuc->events[idx];
+             struct hw_perf_event *hwc;
+
+             /* Ignore if we don't have an event. */
+             if (!event)
+                     continue;
+
+             /*
+              * We have a single interrupt for all counters. Check that
+              * each counter has overflowed before we process it.
+              */
+             if (!nds32_pfm_counter_has_overflowed(pfm, idx))
+                     continue;
+
+             hwc = &event->hw;
+             nds32_pmu_event_update(event);
+             perf_sample_data_init(&data, 0, hwc->last_period);
+             if (!nds32_pmu_event_set_period(event))
+                     continue;
+
+             if (perf_event_overflow(event, &data, regs))
+                     cpu_pmu->disable(event);
+     }
+
+     /*

arch/nds32/kernel/perf_event_cpu.c new file +35 -0

+static int cpu_pmu_request_irq(struct nds32_pmu *cpu_pmu, irq_handler_t handler)
+{
+     int i, err, irq, irqs;
+     struct platform_device *pmu_device = cpu_pmu->plat_device;
+
+     if (!pmu_device)
+             return -ENODEV;
+
+     irqs = min(pmu_device->num_resources, num_possible_cpus());
+     if (irqs < 1) {
+             pr_err("no irqs for PMUs defined\n");
+             return -ENODEV;
+     }
+
+     for (i = 0; i < irqs; ++i) {
+             err = 0;
+             irq = platform_get_irq(pmu_device, i);
+             if (irq < 0)
+                     continue;
+
+             /*
+              * If we have a single PMU interrupt that we can't shift,
+              * assume that we're running on a uniprocessor machine and
+              * continue. Otherwise, continue without this interrupt.
+              */
+             if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+                     pr_warning
+                         ("unable to set irq affinity (irq=%d, cpu=%u)\n",
+                          irq, i);
+                     continue;
+             }
+
+             err = request_irq(irq, handler, IRQF_NOBALANCING, "nds32-pfm",
+                               cpu_pmu);
+             if (err) {

HI,

Quoted 15 lines · Nickhu

arch/nds32/include/asm/pmu.h new file +15 -0

+#define PFM_CTL_OVF(idx)             PFM_CTL_mskOVF ## idx
+#define PFM_CTL_EN(idx)                      PFM_CTL_mskEN ## idx
+#define PFM_CTL_OFFSEL(idx)          PFM_CTL_offSEL ## idx
+#define PFM_CTL_IE(idx)                      PFM_CTL_mskIE ## idx
+#define PFM_CTL_KS(idx)                      PFM_CTL_mskKS ## idx
+#define PFM_CTL_KU(idx)                      PFM_CTL_mskKU ## idx
+#define PFM_CTL_SEL(idx)             PFM_CTL_mskSEL ## idx
+
+#define macro_expansion(macro_name, var, idx) do { \
+     switch (idx) { \
+     case 0:\
+             var = macro_name ## 0; \
+             break; \
+     case 1:\
+             var = macro_name ## 1; \

arch/nds32/include/asm/pmu.h line 49

Quoted 10 lines

arch/nds32/include/asm/pmu.h new file +10 -0

+             break; \
+     case 2:\
+             var = macro_name ## 2; \
+             break; \
+     default:\
+             pr_err("mask index=%d not in the range at %s,line %d\n", \
+                     idx, __FILE__, __LINE__); \
+             break; \
+     } \
+} while (0)

This macro makes things very difficult to understand, especially since in-context it's not clear what's happening to var.

Can you restructure your defintions so you can have things like:

/* valid for 0,1,2 */
#define
#define PFM_CTL_OVF(idx) BIT(6 + (idx))

... and avoid this macro entirely?

arch/nds32/include/asm/pmu.h line 62

Quoted 13 lines

arch/nds32/include/asm/pmu.h new file +13 -0

+
+enum { PFMC0, PFMC1, PFMC2, MAX_COUNTERS };
+
+/*
+ * Perf Events' indices
+ */
+#define NDS32_IDX_CYCLE_COUNTER                      0
+#define NDS32_IDX_COUNTER0                   1
+#define NDS32_IDX_COUNTER_LAST(cpu_pmu) \
+     (NDS32_IDX_CYCLE_COUNTER + (cpu_pmu)->num_events - 1)
+
+#define NDS32_MAX_COUNTERS                   32
+#define NDS32_COUNTER_MASK                   (NDS32_MAX_COUNTERS - 1)

Huh? You only seem to have three above (ignoring the fixed-purpose cycle counter).

These definitions aren't used anywhere, regadless.

arch/nds32/include/asm/pmu.h line 70

Quoted 8 lines

arch/nds32/include/asm/pmu.h new file +8 -0

+
+/*
+ * struct nds32_pmu_platdata - NDS32 PMU platform data
+ *
+ * @handle_irq: an optional handler which will be called from the
+ *   interrupt and passed the address of the low level handler,
+ *   and can be used to implement any platform specific handling
+ *   before or after calling it.

Will you actually need this? We got rid of it in the arm_pmu framework.

Quoted 27 lines

+ * @runtime_resume: an optional handler which will be called by the

+ *   runtime PM framework following a call to pm_runtime_get().
+ *   Note that if pm_runtime_get() is called more than once in
+ *   succession this handler will only be called once.
+ * @runtime_suspend: an optional handler which will be called by the
+ *   runtime PM framework following a call to pm_runtime_put().
+ *   Note that if pm_runtime_get() is called more than once in
+ *   succession this handler will only be called following the
+ *   final call to pm_runtime_put() that actually disables the
+ *   hardware.

+ */

arch/nds32/include/asm/pmu.h new file +16 -0

+struct nds32_pmu_platdata {
+     irqreturn_t (*handle_irq)(int irq, void *dev,
+                               irq_handler_t pmu_handler);
+     int (*runtime_resume)(struct device *dev);
+     int (*runtime_suspend)(struct device *dev);
+};
+
+/* The events for a given PMU register set. */
+struct pmu_hw_events {
+     /*
+      * The events that are active on the PMU for the given index.
+      */
+     struct perf_event **events;
+
+     /*
+      * A 1 bit for an index indicates that the counter is being used for

arch/nds32/include/asm/pmu.h line 107

Quoted 10 lines

arch/nds32/include/asm/pmu.h new file +10 -0

+      * an event. A 0 means that the counter can be used.
+      */
+     unsigned long *used_mask;
+
+     /*
+      * Hardware lock to serialize accesses to PMU registers. Needed for the
+      * read/modify/write sequences.
+      */
+     raw_spinlock_t pmu_lock;
+};

It looks like this is derived from the really early arm_pmu code, when we were trying to be far more flexible than was necessary. This can be simplified significantly.

e.g. since you know up-front the max number of counters, this can be:

struct pmu_hw_events {
        struct perf_event *events[MAX_COUNTERS];
        unsigned long mask[BITS_TO_LONGS(MAX_COUNTERS)];
        raw_spinlock_t pmu_lock;
};

... and you can avoid having to dynamically allocate each field separately.

I'm not sure the pmu_lock is even necessary in the asbence of NMI.

arch/nds32/include/asm/pmu.h line 112

Quoted 5 lines

arch/nds32/include/asm/pmu.h new file +5 -0

+
+struct nds32_pmu {
+     struct pmu pmu;
+     cpumask_t active_irqs;
+     cpumask_t supported_cpus;

Do you intend to support heterogeneous systems (e.g big.LITTLE)?

If not, you don't need the supported_cpus mask.

Quoted 9 lines

arch/nds32/include/asm/pmu.h new file +9 -0

+     char *name;
+      irqreturn_t (*handle_irq)(int irq_num, void *dev);
+     void (*enable)(struct perf_event *event);
+     void (*disable)(struct perf_event *event);
+     int (*get_event_idx)(struct pmu_hw_events *hw_events,
+                          struct perf_event *event);
+     int (*set_event_filter)(struct hw_perf_event *evt,
+                             struct perf_event_attr *attr);
+     u32 (*read_counter)(struct perf_event *event);

arch/nds32/include/asm/pmu.h line 131

Quoted 10 lines

arch/nds32/include/asm/pmu.h new file +10 -0

+     void (*write_counter)(struct perf_event *event, u32 val);
+     void (*start)(struct nds32_pmu *nds32_pmu);
+     void (*stop)(struct nds32_pmu *nds32_pmu);
+     void (*reset)(void *data);
+     int (*request_irq)(struct nds32_pmu *nds32_pmu, irq_handler_t handler);
+     void (*free_irq)(struct nds32_pmu *nds32_pmu);
+     int (*map_event)(struct perf_event *event);
+     int num_events;
+     atomic_t active_events;
+     struct mutex reserve_mutex;     /* mutex */

I don't think you need the reserve_mutex; for arm_pmu that was a holdover form when there were multiple frameworks competing to mange the PMU, and now there's only perf.

Quoted 1 line

+ u64 max_period;

It looks like this is always 0xffffffff, so you don't need a variable for that.

[...]

arch/nds32/include/asm/pmu.h line 247

Quoted 13 lines

arch/nds32/include/asm/pmu.h new file +13 -0

+/* Get converted event counter index */
+#define GET_CONVERTED_EVENT_IDX(event, idx) do { \
+     if ((event) > SPAV3_0_SEL_BASE && event < SPAV3_0_SEL_LAST) { \
+             idx = 0; \
+     } else if ((event) > SPAV3_1_SEL_BASE && event < SPAV3_1_SEL_LAST) { \
+             idx = 1; \
+     } else if ((event) > SPAV3_2_SEL_BASE && event < SPAV3_2_SEL_LAST) { \
+             idx = 2; \
+     } else { \
+             pr_err("GET_CONVERTED_EVENT_IDX PFM counter range error\n"); \
+             return -EPERM; \
+     } \
+} while (0)

This would be cleaner as a static inline.

Quoted 6 lines

arch/nds32/include/asm/pmu.h new file +6 -0

+
+/* Get converted hardware event number */
+#define GET_CONVERTED_EVENT_HW_NUM(event) do { \
+     if ((event) == 0) { \
+             /*do nothing*/    \
+     } else if ((event) > SPAV3_0_SEL_BASE && event < SPAV3_0_SEL_LAST) { \

arch/nds32/include/asm/pmu.h line 263

Quoted 10 lines

arch/nds32/include/asm/pmu.h new file +10 -0

+             (event) -= PFM_OFFSET_MAGIC_0; \
+     } else if ((event) > SPAV3_1_SEL_BASE && event < SPAV3_1_SEL_LAST) { \
+             (event) -= PFM_OFFSET_MAGIC_1; \
+     } else if ((event) > SPAV3_2_SEL_BASE && event < SPAV3_2_SEL_LAST) { \
+             (event) -= PFM_OFFSET_MAGIC_2; \
+     } else { \
+             pr_err(\
+             "GET_CONVERTED_EVENT_HW_NUM PFM counter range error\n"); \
+     } \
+} while (0)

Likewise, a static inline would be cleaner.

[...]

arch/nds32/kernel/perf_event_cpu.c line 30

Quoted 6 lines

arch/nds32/kernel/perf_event_cpu.c new file +6 -0

+/* Set at runtime when we know what CPU type we are. */
+static struct nds32_pmu *cpu_pmu;
+
+static DEFINE_PER_CPU(struct perf_event * [MAX_COUNTERS], hw_events);
+static DEFINE_PER_CPU(unsigned long[BITS_TO_LONGS(MAX_COUNTERS)], used_mask);
+static DEFINE_PER_CPU(struct pmu_hw_events, cpu_hw_events);

As before, I think you can fold the first wo fields into pmu_hw_events, then you only need one DEFINE_PER_CPU() here.

[...]

Quoted 50 lines

arch/nds32/kernel/perf_event_cpu.c new file +50 -0

+static irqreturn_t nds32_pmu_handle_irq(int irq_num, void *dev)
+{
+     u32 pfm;
+     struct perf_sample_data data;
+     struct nds32_pmu *cpu_pmu = (struct nds32_pmu *)dev;
+     struct pmu_hw_events *cpuc = cpu_pmu->get_hw_events();
+     struct pt_regs *regs;
+     int idx;
+     /*
+      * Get and reset the IRQ flags
+      */
+     pfm = nds32_pfm_getreset_flags();
+
+     /*
+      * Did an overflow occur?
+      */
+     if (!nds32_pfm_has_overflowed(pfm))
+             return IRQ_NONE;
+
+     /*
+      * Handle the counter(s) overflow(s)
+      */
+     regs = get_irq_regs();
+
+     for (idx = 0; idx < cpu_pmu->num_events; ++idx) {
+             struct perf_event *event = cpuc->events[idx];
+             struct hw_perf_event *hwc;
+
+             /* Ignore if we don't have an event. */
+             if (!event)
+                     continue;
+
+             /*
+              * We have a single interrupt for all counters. Check that
+              * each counter has overflowed before we process it.
+              */
+             if (!nds32_pfm_counter_has_overflowed(pfm, idx))
+                     continue;
+
+             hwc = &event->hw;
+             nds32_pmu_event_update(event);
+             perf_sample_data_init(&data, 0, hwc->last_period);
+             if (!nds32_pmu_event_set_period(event))
+                     continue;
+
+             if (perf_event_overflow(event, &data, regs))
+                     cpu_pmu->disable(event);
+     }
+
+     /*

arch/nds32/kernel/perf_event_cpu.c line 280

Quoted 10 lines

arch/nds32/kernel/perf_event_cpu.c new file +10 -0

+      * Handle the pending perf events.
+      *
+      * Note: this call *must* be run with interrupts disabled. For
+      * platforms that can have the PMU interrupts raised as an NMI, this
+      * will not work.
+      */
+     irq_work_run();
+
+     return IRQ_HANDLED;
+}

You'll want to stop all counters over the IRQ handler to ensure that groups are consistent.

[...]

arch/nds32/kernel/perf_event_cpu.c line 655

Quoted 8 lines

arch/nds32/kernel/perf_event_cpu.c new file +8 -0

+static int device_pmu_init(struct nds32_pmu *cpu_pmu)
+{
+     nds32_pmu_init(cpu_pmu);
+     /*
+      * This name should be devive-specific name, whatever you like :)
+      * I think "PMU" will be a good generic name.
+      */
+     cpu_pmu->name = "atcpmu";

This looks like it should have a more complete version string, e.g. with "spav3" in it.

[...]

arch/nds32/kernel/perf_event_cpu.c line 760

Quoted 6 lines

arch/nds32/kernel/perf_event_cpu.c new file +6 -0

+static int
+validate_event(struct pmu *pmu, struct pmu_hw_events *hw_events,
+            struct perf_event *event)
+{
+     struct nds32_pmu *nds32_pmu = to_nds32_pmu(event->pmu);
+     //struct pmu *leader_pmu = event->group_leader->pmu;

Code which is commented out should be deleted.

[...]

Quoted 39 lines

arch/nds32/kernel/perf_event_cpu.c new file +39 -0

+static int cpu_pmu_request_irq(struct nds32_pmu *cpu_pmu, irq_handler_t handler)
+{
+     int i, err, irq, irqs;
+     struct platform_device *pmu_device = cpu_pmu->plat_device;
+
+     if (!pmu_device)
+             return -ENODEV;
+
+     irqs = min(pmu_device->num_resources, num_possible_cpus());
+     if (irqs < 1) {
+             pr_err("no irqs for PMUs defined\n");
+             return -ENODEV;
+     }
+
+     for (i = 0; i < irqs; ++i) {
+             err = 0;
+             irq = platform_get_irq(pmu_device, i);
+             if (irq < 0)
+                     continue;
+
+             /*
+              * If we have a single PMU interrupt that we can't shift,
+              * assume that we're running on a uniprocessor machine and
+              * continue. Otherwise, continue without this interrupt.
+              */
+             if (irq_set_affinity(irq, cpumask_of(i)) && irqs > 1) {
+                     pr_warning
+                         ("unable to set irq affinity (irq=%d, cpu=%u)\n",
+                          irq, i);
+                     continue;
+             }
+
+             err = request_irq(irq, handler, IRQF_NOBALANCING, "nds32-pfm",
+                               cpu_pmu);
+             if (err) {
+                     pr_err("unable to request IRQ%d for NDS PMU counters\n",
+                            irq);
+                     return err;
+             }

arch/nds32/kernel/perf_event_cpu.c line 1130

Quoted 6 lines

arch/nds32/kernel/perf_event_cpu.c new file +6 -0

+
+             cpumask_set_cpu(i, &cpu_pmu->active_irqs);
+     }
+
+     return 0;
+}

Is nds32 SMP?

If not, this should be simplifed to expect one CPU alone.

If so, this needs to clarify which IRQ maps to which CPU with an explicit property in the DT, as we do for arm with interrupt-affinity.

Thanks,
Mark.
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.

Quoted 66 lines · Mark Rutland

Quoted 50 lines · Nickhu

arch/nds32/kernel/perf_event_cpu.c new file +50 -0

+static irqreturn_t nds32_pmu_handle_irq(int irq_num, void *dev)
+{
+     u32 pfm;
+     struct perf_sample_data data;
+     struct nds32_pmu *cpu_pmu = (struct nds32_pmu *)dev;
+     struct pmu_hw_events *cpuc = cpu_pmu->get_hw_events();
+     struct pt_regs *regs;
+     int idx;
+     /*
+      * Get and reset the IRQ flags
+      */
+     pfm = nds32_pfm_getreset_flags();
+
+     /*
+      * Did an overflow occur?
+      */
+     if (!nds32_pfm_has_overflowed(pfm))
+             return IRQ_NONE;
+
+     /*
+      * Handle the counter(s) overflow(s)
+      */
+     regs = get_irq_regs();
+
+     for (idx = 0; idx < cpu_pmu->num_events; ++idx) {
+             struct perf_event *event = cpuc->events[idx];
+             struct hw_perf_event *hwc;
+
+             /* Ignore if we don't have an event. */
+             if (!event)
+                     continue;
+
+             /*
+              * We have a single interrupt for all counters. Check that
+              * each counter has overflowed before we process it.
+              */
+             if (!nds32_pfm_counter_has_overflowed(pfm, idx))
+                     continue;
+
+             hwc = &event->hw;
+             nds32_pmu_event_update(event);
+             perf_sample_data_init(&data, 0, hwc->last_period);
+             if (!nds32_pmu_event_set_period(event))
+                     continue;
+
+             if (perf_event_overflow(event, &data, regs))
+                     cpu_pmu->disable(event);
+     }
+
+     /*

arch/nds32/kernel/perf_event_cpu.c line 280

Quoted 10 lines

arch/nds32/kernel/perf_event_cpu.c new file +10 -0

+      * Handle the pending perf events.
+      *
+      * Note: this call *must* be run with interrupts disabled. For
+      * platforms that can have the PMU interrupts raised as an NMI, this
+      * will not work.
+      */
+     irq_work_run();
+
+     return IRQ_HANDLED;
+}

You'll want to stop all counters over the IRQ handler to ensure that groups are consistent.

Quoted 1 line

[...]

Our implementation of IRQ handler references the irq_handler of arch/arm/kernel/perf_event_v7.c.

Do you mean that when one counter overflowed, other counters do not count the events caused by the counter overflow handler?

There two bitfield bug for perfomance counter
in bitfield.h:

	PFM_CTL_offSEL1		21 --> 16
	PFM_CTL_offSEL2		27 --> 22

This commit fix it.

Signed-off-by Nickhu <[email protected]>

---
 arch/nds32/include/asm/bitfield.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/nds32/include/asm/bitfield.h +2 -2

index 8e84fc385b94..19b2841219ad 100644
@@ -692,8 +692,8 @@
 #define PFM_CTL_offKU1		13	/* Enable user mode event counting for PFMC1 */
 #define PFM_CTL_offKU2		14	/* Enable user mode event counting for PFMC2 */
 #define PFM_CTL_offSEL0		15	/* The event selection for PFMC0 */
-#define PFM_CTL_offSEL1		21	/* The event selection for PFMC1 */
-#define PFM_CTL_offSEL2		27	/* The event selection for PFMC2 */
+#define PFM_CTL_offSEL1		16	/* The event selection for PFMC1 */
+#define PFM_CTL_offSEL2		22	/* The event selection for PFMC2 */
 /* bit 28:31 reserved */
 
 #define PFM_CTL_mskEN0		( 0x01  << PFM_CTL_offEN0 )

Signature · 1 line

2.17.0

Quoted 9 lines · Nickhu

There two bitfield bug for perfomance counter
in bitfield.h:

	PFM_CTL_offSEL1		21 --> 16
	PFM_CTL_offSEL2		27 --> 22

This commit fix it.

Signed-off-by Nickhu <[email protected]>

This patch should probably be move before the patch adding perf support. That way, perf support isn't broken at the point it is added.

Thanks,
Mark.

Quoted 18 lines

---
 arch/nds32/include/asm/bitfield.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/nds32/include/asm/bitfield.h +2 -2

index 8e84fc385b94..19b2841219ad 100644
@@ -692,8 +692,8 @@
 #define PFM_CTL_offKU1		13	/* Enable user mode event counting for PFMC1 */
 #define PFM_CTL_offKU2		14	/* Enable user mode event counting for PFMC2 */
 #define PFM_CTL_offSEL0		15	/* The event selection for PFMC0 */
-#define PFM_CTL_offSEL1		21	/* The event selection for PFMC1 */
-#define PFM_CTL_offSEL2		27	/* The event selection for PFMC2 */
+#define PFM_CTL_offSEL1		16	/* The event selection for PFMC1 */
+#define PFM_CTL_offSEL2		22	/* The event selection for PFMC2 */
 /* bit 28:31 reserved */
 

Quoted 4 lines

arch/nds32/include/asm/bitfield.h +0 -0

 #define PFM_CTL_mskEN0		( 0x01  << PFM_CTL_offEN0 )

Signature · 1 line

2.17.0

When there are multiple events map to the same counter, the counter counts inaccurately. This is because each counter only counts one event in the same time.
So when there are multiple events map to same counter, they have to take turns in each context.

There are two solution:
1. Print the error message when multiple events map to the same counter. But print the error message would let the program hang in loop. The ltp (linux test program) would be failed when the program hang in loop.

2. Don't print the error message, the ltp would pass. But the user need to have the knowledge that don't count the events which map to the same counter, or the user will get the inaccurate results.

We choose method 2 for the solution

Signed-off-by Nickhu <[email protected]>

---
 arch/nds32/include/asm/pmu.h       |  1 +
 arch/nds32/kernel/perf_event_cpu.c | 30 ++++++++++++++++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

arch/nds32/include/asm/pmu.h +1 -0

index 3fbbe97c2d42..e75ec34af5f6 100644
@@ -55,6 +55,7 @@ enum { PFMC0, PFMC1, PFMC2, MAX_COUNTERS };
  */
 #define NDS32_IDX_CYCLE_COUNTER			0
 #define NDS32_IDX_COUNTER0			1
+#define NDS32_IDX_COUNTER1			2
 #define NDS32_IDX_COUNTER_LAST(cpu_pmu) \
 	(NDS32_IDX_CYCLE_COUNTER + (cpu_pmu)->num_events - 1)
 

arch/nds32/kernel/perf_event_cpu.c +20 -10

index 7bb4ebb87b5c..e9a0d8bb2bc1 100644
@@ -566,16 +566,26 @@ static int nds32_pmu_get_event_idx(struct pmu_hw_events *cpuc,
 	/*
 	 * Try to get the counter for correpsonding event
 	 */
-	if (!test_and_set_bit(idx, cpuc->used_mask))
-		return idx;
-
-	/*
-	 * The counter is in use.
-	 * The system will hang in the loop.
-	 */
-	pr_err
-	("Multiple events map to one counter, the behavior is undefined.\n");
-	return -EPERM;
+	if (evtype == SPAV3_0_SEL_TOTAL_CYCLES) {
+		if (!test_and_set_bit(idx, cpuc->used_mask))
+			return idx;
+		if (!test_and_set_bit(NDS32_IDX_COUNTER0, cpuc->used_mask))
+			return NDS32_IDX_COUNTER0;
+		if (!test_and_set_bit(NDS32_IDX_COUNTER1, cpuc->used_mask))
+			return NDS32_IDX_COUNTER1;
+	} else if (evtype == SPAV3_1_SEL_COMPLETED_INSTRUCTION) {
+		if (!test_and_set_bit(idx, cpuc->used_mask))
+			return idx;
+		else if (!test_and_set_bit(NDS32_IDX_COUNTER1, cpuc->used_mask))
+			return NDS32_IDX_COUNTER1;
+		else if (!test_and_set_bit
+			 (NDS32_IDX_CYCLE_COUNTER, cpuc->used_mask))
+			return NDS32_IDX_CYCLE_COUNTER;
+	} else {
+		if (!test_and_set_bit(idx, cpuc->used_mask))
+			return idx;
+	}
+	return -EAGAIN;
 }
 
 static void nds32_pmu_start(struct nds32_pmu *cpu_pmu)

Signature · 1 line

2.17.0

Quoted 11 lines · Nickhu

When there are multiple events map to the same counter, the counter counts inaccurately. This is because each counter only counts one event in the same time.
So when there are multiple events map to same counter, they have to take turns in each context.

There are two solution:
1. Print the error message when multiple events map to the same counter. But print the error message would let the program hang in loop. The ltp (linux test program) would be failed when the program hang in loop.

Quoted 5 lines

2. Don't print the error message, the ltp would pass. But the user need to have the knowledge that don't count the events which map to the same counter, or the user will get the inaccurate results.

We choose method 2 for the solution

This is the correct solution. Perf exposes the active/enabled time in the perf event, so the user can determine that the event wasn't enabled all of the time.

This should be folded into the commit adding perf support.

Thanks,
Mark.

Quoted 59 lines

Signed-off-by Nickhu <[email protected]>

---
 arch/nds32/include/asm/pmu.h       |  1 +
 arch/nds32/kernel/perf_event_cpu.c | 30 ++++++++++++++++++++----------
 2 files changed, 21 insertions(+), 10 deletions(-)

arch/nds32/include/asm/pmu.h +1 -0

index 3fbbe97c2d42..e75ec34af5f6 100644
@@ -55,6 +55,7 @@ enum { PFMC0, PFMC1, PFMC2, MAX_COUNTERS };
  */
 #define NDS32_IDX_CYCLE_COUNTER			0
 #define NDS32_IDX_COUNTER0			1
+#define NDS32_IDX_COUNTER1			2
 #define NDS32_IDX_COUNTER_LAST(cpu_pmu) \
 	(NDS32_IDX_CYCLE_COUNTER + (cpu_pmu)->num_events - 1)
 

arch/nds32/kernel/perf_event_cpu.c +20 -10

index 7bb4ebb87b5c..e9a0d8bb2bc1 100644
@@ -566,16 +566,26 @@ static int nds32_pmu_get_event_idx(struct pmu_hw_events *cpuc,
 	/*
 	 * Try to get the counter for correpsonding event
 	 */
-	if (!test_and_set_bit(idx, cpuc->used_mask))
-		return idx;
-
-	/*
-	 * The counter is in use.
-	 * The system will hang in the loop.
-	 */
-	pr_err
-	("Multiple events map to one counter, the behavior is undefined.\n");
-	return -EPERM;
+	if (evtype == SPAV3_0_SEL_TOTAL_CYCLES) {
+		if (!test_and_set_bit(idx, cpuc->used_mask))
+			return idx;
+		if (!test_and_set_bit(NDS32_IDX_COUNTER0, cpuc->used_mask))
+			return NDS32_IDX_COUNTER0;
+		if (!test_and_set_bit(NDS32_IDX_COUNTER1, cpuc->used_mask))
+			return NDS32_IDX_COUNTER1;
+	} else if (evtype == SPAV3_1_SEL_COMPLETED_INSTRUCTION) {
+		if (!test_and_set_bit(idx, cpuc->used_mask))
+			return idx;
+		else if (!test_and_set_bit(NDS32_IDX_COUNTER1, cpuc->used_mask))
+			return NDS32_IDX_COUNTER1;
+		else if (!test_and_set_bit
+			 (NDS32_IDX_CYCLE_COUNTER, cpuc->used_mask))
+			return NDS32_IDX_CYCLE_COUNTER;
+	} else {
+		if (!test_and_set_bit(idx, cpuc->used_mask))
+			return idx;
+	}
+	return -EAGAIN;
 }
 

Quoted 4 lines

arch/nds32/kernel/perf_event_cpu.c +0 -0

 static void nds32_pmu_start(struct nds32_pmu *cpu_pmu)

Signature · 1 line

2.17.0

The document for how to add NDS32 PMU
in devicetree.

Signed-off-by Nickhu <[email protected]>

---
 Documentation/devicetree/bindings/nds32/pmu.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nds32/pmu.txt

Documentation/devicetree/bindings/nds32/pmu.txt new file +0 -0

new file mode 100644
index 000000000000..02762b850e59

Quoted 15 lines · Nickhu

The document for how to add NDS32 PMU
in devicetree.

Signed-off-by Nickhu <[email protected]>

---
 Documentation/devicetree/bindings/nds32/pmu.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nds32/pmu.txt

Documentation/devicetree/bindings/nds32/pmu.txt new file +0 -0

new file mode 100644
index 000000000000..02762b850e59

Documentation/devicetree/bindings/nds32/pmu.txt line 9

Quoted 9 lines

Documentation/devicetree/bindings/nds32/pmu.txt new file +9 -0

@@ -0,0 +1,17 @@
+* NDS32 Performance Monitor Units
+
+NDS32 core have a PMU for counting cpu and cache events like cache misses.
+The NDS32 PMU representation in the device tree should be done as under:
+
+Required properties:
+
+- compatilbe :
+	"andestech,atcpmu"

Which CPUs have this PMU?

I expected more specific strings, e.g. "andestech,n13-pmu" and/or "andestech,andestech,nds32v3-pmu".

Documentation/devicetree/bindings/nds32/pmu.txt line 17

Quoted 8 lines

Documentation/devicetree/bindings/nds32/pmu.txt new file +8 -0

+
+- interrupts : The interrupt number for NDS32 PMU is 13.
+
+Example:
+pmu{
+	compatible = "andestech,atcpmu";
+	interrupts = <13>;
+}

The driver tried to find multiple interrupts. Is there only a single interrupt in all cases?

Thanks,
Mark.

Hi Mark,

Quoted 28 lines · Mark Rutland

Quoted 15 lines · Nickhu

The document for how to add NDS32 PMU
in devicetree.

Signed-off-by Nickhu <[email protected]>

---
 Documentation/devicetree/bindings/nds32/pmu.txt | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/nds32/pmu.txt

Documentation/devicetree/bindings/nds32/pmu.txt new file +0 -0

new file mode 100644
index 000000000000..02762b850e59

Documentation/devicetree/bindings/nds32/pmu.txt line 9

Quoted 9 lines

Documentation/devicetree/bindings/nds32/pmu.txt new file +9 -0

@@ -0,0 +1,17 @@
+* NDS32 Performance Monitor Units
+
+NDS32 core have a PMU for counting cpu and cache events like cache misses.
+The NDS32 PMU representation in the device tree should be done as under:
+
+Required properties:
+
+- compatilbe :
+	"andestech,atcpmu"

Which CPUs have this PMU?

Quoted 3 lines

I expected more specific strings, e.g. "andestech,n13-pmu" and/or "andestech,andestech,nds32v3-pmu".

In nds32 V3, all of our CPU have PMU.
So I will change the string to andestech,nds32v3-pmu.

Quoted 11 lines

Documentation/devicetree/bindings/nds32/pmu.txt line 17

Quoted 7 lines

Documentation/devicetree/bindings/nds32/pmu.txt new file +7 -0

+- interrupts : The interrupt number for NDS32 PMU is 13.
+
+Example:
+pmu{
+	compatible = "andestech,atcpmu";
+	interrupts = <13>;
+}

The driver tried to find multiple interrupts. Is there only a single interrupt in all cases?

There is only overflow interrupt for performance couner in nds32 V3. I will modified the driver and prepare another patch.