[PATCHv2 0/4] arm-cci500: Workaround pmu_event_set_period

[PATCHv2 0/4] arm-cci500: Workaround pmu_event_set_period

[Suzuki K. Poulose]

The CCI PMU driver sets the event counter to the half of the maximum
value(2^31) it can count before we start the counters via
pmu_event_set_period(). This is done to give us the best chance to
handle the overflow interrupt, taking care of extreme interrupt latencies.

However, CCI-500 comes with advanced power saving schemes, which disables
the clock to the event counters unless the counters are enabled to count
(PMCR.CEN). This prevents the driver from writing the period to the
counters before starting them.  Also, there is no way we can reset the
individual event counter to 0 (PMCR.RST resets all the counters, losing
their current readings). However the value of the counter is preserved and
could be read back, when the counters are not enabled.

So we cannot reliably use the counters and compute the number of events
generated during the sampling period since we don't have the value of the
counter at start.

Here are the possible solutions:

 1) Disable clock gating on CCI-500 by setting Control_Override_Reg[bit3].
    - The Control_Override_Reg is secure (and hence not programmable from
      Linux), and also has an impact on power consumption.

 2) Change the order of operations
	i.e,
	a) Program and enable individual counters
	b) Enable counting on all the counters by setting PMCR.CEN
	c) Write the period to the individual counters
	d) Disable the counters
    - This could cause in unnecessary noise in the other counters and is
      costly (we should repeat this for all enabled counters).

 3) Don't set the counter value, instead use the current count as the
    starting count and compute the delta at the end of sampling.

 4) Modified version of 2, which disables all the other counters, except
    the target counter, with the target counter programmed with an invalid
    event code(which guarantees that the counter won't change during the
    operation).

This patch implements option 4 for CCI-500. CCI-400 behavior remains
unchanged. The problem didn't surface on a fast model, but was revealed
on an FPGA model. Without this patch profiling on CCI-500 is broken and
should be fixed for 4.3.

Changes since V1:
 - Choose 4 instead of 3 above, suggested by Mark Rutland

Suzuki K. Poulose (4):
  arm-cci: Refactor CCI PMU code
  arm-cci: Get the status of a counter
  arm-cci: Add routines to enable/disable all counters
  arm-cci500: Work around PMU counter writes

 drivers/bus/arm-cci.c |  184 +++++++++++++++++++++++++++++++++++++++----------
 1 file changed, 147 insertions(+), 37 deletions(-)

-- 
1.7.9.5

[PATCHv2 1/4] arm-cci: Refactor CCI PMU code

[Suzuki K. Poulose]

This patch refactors the CCI PMU driver code a little bit to
make it easier share the code for enabling/disabling the CCI
PMU. This will be used by the hooks to work around the special cases
where writing to a counter is not always that easy(e.g, CCI-500)

No functional changes.

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: arm@kernel.org
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   94 +++++++++++++++++++++++++++++--------------------
 1 file changed, 56 insertions(+), 38 deletions(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 577cc4b..5435d87 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -738,6 +738,53 @@ static void pmu_free_irq(struct cci_pmu *cci_pmu)
 	}
 }
 
+/* Should be called with cci_pmu->hw_events->pmu_lock */
+static void __cci_pmu_enable(void)
+{
+	u32 val;
+
+	/* Enable all the PMU counters. */
+	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) | CCI_PMCR_CEN;
+	writel(val, cci_ctrl_base + CCI_PMCR);
+}
+
+/* Should be called with cci_pmu->hw_events->pmu_lock */
+static void __cci_pmu_disable(void)
+{
+	u32 val;
+
+	/* Disable all the PMU counters. */
+	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) & ~CCI_PMCR_CEN;
+	writel(val, cci_ctrl_base + CCI_PMCR);
+}
+
+static void cci_pmu_enable(struct pmu *pmu)
+{
+	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
+	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
+	int enabled = bitmap_weight(hw_events->used_mask, cci_pmu->num_cntrs);
+	unsigned long flags;
+
+	if (!enabled)
+		return;
+
+	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
+	__cci_pmu_enable();
+	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
+
+}
+
+static void cci_pmu_disable(struct pmu *pmu)
+{
+	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
+	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
+	__cci_pmu_disable();
+	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
+}
+
 static u32 pmu_read_counter(struct perf_event *event)
 {
 	struct cci_pmu *cci_pmu = to_cci_pmu(event->pmu);
@@ -754,16 +801,22 @@ static u32 pmu_read_counter(struct perf_event *event)
 	return value;
 }
 
+static void __pmu_write_counter(struct cci_pmu *cci_pmu, int idx, u32 value)
+{
+	pmu_write_register(cci_pmu, value, idx, CCI_PMU_CNTR);
+}
+
 static void pmu_write_counter(struct perf_event *event, u32 value)
 {
 	struct cci_pmu *cci_pmu = to_cci_pmu(event->pmu);
 	struct hw_perf_event *hw_counter = &event->hw;
 	int idx = hw_counter->idx;
 
-	if (unlikely(!pmu_is_valid_counter(cci_pmu, idx)))
+	if (unlikely(!pmu_is_valid_counter(cci_pmu, idx))) {
 		dev_err(&cci_pmu->plat_device->dev, "Invalid CCI PMU counter %d\n", idx);
-	else
-		pmu_write_register(cci_pmu, value, idx, CCI_PMU_CNTR);
+		return;
+	}
+	__pmu_write_counter(cci_pmu, idx, value);
 }
 
 static u64 pmu_event_update(struct perf_event *event)
@@ -869,41 +922,6 @@ static void hw_perf_event_destroy(struct perf_event *event)
 	}
 }
 
-static void cci_pmu_enable(struct pmu *pmu)
-{
-	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
-	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
-	int enabled = bitmap_weight(hw_events->used_mask, cci_pmu->num_cntrs);
-	unsigned long flags;
-	u32 val;
-
-	if (!enabled)
-		return;
-
-	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
-
-	/* Enable all the PMU counters. */
-	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) | CCI_PMCR_CEN;
-	writel(val, cci_ctrl_base + CCI_PMCR);
-	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
-
-}
-
-static void cci_pmu_disable(struct pmu *pmu)
-{
-	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
-	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
-	unsigned long flags;
-	u32 val;
-
-	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
-
-	/* Disable all the PMU counters. */
-	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) & ~CCI_PMCR_CEN;
-	writel(val, cci_ctrl_base + CCI_PMCR);
-	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
-}
-
 /*
  * Check if the idx represents a non-programmable counter.
  * All the fixed event counters are mapped before the programmable
-- 
1.7.9.5

[PATCHv2 2/4] arm-cci: Get the status of a counter

[Suzuki K. Poulose]

Add helper routines to get the counter status and the event
programmed on it.

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: arm@kernel.org
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 5435d87..1a75010 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -647,11 +647,21 @@ static void pmu_enable_counter(struct cci_pmu *cci_pmu, int idx)
 	pmu_write_register(cci_pmu, 1, idx, CCI_PMU_CNTR_CTRL);
 }
 
+static u32 pmu_get_counter_ctrl(struct cci_pmu *cci_pmu, int idx)
+{
+	return pmu_read_register(cci_pmu, idx, CCI_PMU_CNTR_CTRL) & 0x1;
+}
+
 static void pmu_set_event(struct cci_pmu *cci_pmu, int idx, unsigned long event)
 {
 	pmu_write_register(cci_pmu, event, idx, CCI_PMU_EVT_SEL);
 }
 
+static u32 pmu_get_event(struct cci_pmu *cci_pmu, int idx)
+{
+	return pmu_read_register(cci_pmu, idx, CCI_PMU_EVT_SEL);
+}
+
 /*
  * Returns the number of programmable counters actually implemented
  * by the cci
-- 
1.7.9.5

[PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters

[Suzuki K. Poulose]

Adds helper routines to manipulate the counter controls for
all the counters on the CCI PMU.

pmu_disable_counters_ctrl: Iterates over the counters,
checking the status of each counter and disabling any enabled
counters. For each such changed counter, the mask is updated so that
one can restore the state later using pmu_enable_counters_ctrl.

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: arm@kernel.org
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 1a75010..7f4a266 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -663,6 +663,36 @@ static u32 pmu_get_event(struct cci_pmu *cci_pmu, int idx)
 }
 
 /*
+ * Restore the status of the counters.
+ * For each counter set in the mask, enable the counter back.
+ */
+static void pmu_restore_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
+{
+	int i;
+
+	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
+		pmu_enable_counter(cci_pmu, i);
+}
+
+/*
+ * For all counters on the CCI-PMU, disable any 'enabled' counters,
+ * saving the changed counters in the mask, so that we can restore
+ * it later using pmu_restore_counters_ctrl.
+ */
+static void pmu_disable_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
+{
+	int i;
+
+	for (i = 0; i < cci_pmu->num_cntrs; i++) {
+		clear_bit(i, mask);
+		if (pmu_get_counter_ctrl(cci_pmu, i)) {
+			set_bit(i, mask);
+			pmu_disable_counter(cci_pmu, i);
+		}
+	}
+}
+
+/*
  * Returns the number of programmable counters actually implemented
  * by the cci
  */
-- 
1.7.9.5

[PATCHv2 4/4] arm-cci500: Work around PMU counter writes

[Suzuki K. Poulose]

The CCI PMU driver sets the event counter to the half of the maximum
value(2^31) it can count before we start the counters via
pmu_event_set_period(). This is done to give us the best chance to
handle the overflow interrupt, taking care of extreme interrupt latencies.

However, CCI-500 comes with advanced power saving schemes, which disables
the clock to the event counters unless the counters are enabled to count
(PMCR.CEN). This prevents the driver from writing the period to the
counters before starting them.  Also, there is no way we can reset the
individual event counter to 0 (PMCR.RST resets all the counters, losing
their current readings). However the value of the counter is preserved and
could be read back, when the counters are not enabled.

So we cannot reliably use the counters and compute the number of events
generated during the sampling period since we don't have the value of the
counter at start.

This patch works around this issue by changing writes to the counter
with the following steps only for th CCI-500.

 1) Disable all the counters (remembering any counters which were enabled)
 2) Save the current event and program the target counter to count an
    invalid event, which by spec is guaranteed to not-generate any events.
 3) Enable the target counter.
 4) Enable the CCI PMU
 5) Write to the target counter.
 6) Disable the CCI PMU and the target counter
 7) Restore the event back on the target counter.
 8) Restore the status of the all the counters

Cc: Punit Agrawal <punit.agrawal@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: arm@kernel.org
Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
---
 drivers/bus/arm-cci.c |   54 ++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
index 7f4a266..cbb54b9 100644
--- a/drivers/bus/arm-cci.c
+++ b/drivers/bus/arm-cci.c
@@ -128,6 +128,7 @@ struct cci_pmu_model {
 	struct event_range event_ranges[CCI_IF_MAX];
 	int (*validate_hw_event)(struct cci_pmu *, unsigned long);
 	int (*get_event_idx)(struct cci_pmu *, struct cci_pmu_hw_events *, unsigned long);
+	void (*write_counter)(struct cci_pmu *, int, u32);
 };
 
 static struct cci_pmu_model cci_pmu_models[];
@@ -472,6 +473,12 @@ static inline struct cci_pmu_model *probe_cci_model(struct platform_device *pdev
 #define CCI500_GLOBAL_PORT_MIN_EV	0x00
 #define CCI500_GLOBAL_PORT_MAX_EV	0x0f
 
+/*
+ * For a guranteed non-counting event, we use a master source with the highest
+ * possible event code, which has the least chances of being assigned a code for.
+ */
+#define CCI500_INVALID_EVENT		(CCI500_PORT_M0 << CCI500_PMU_EVENT_SOURCE_SHIFT | \
+					CCI500_PMU_EVENT_CODE_MASK << CCI500_PMU_EVENT_CODE_SHIFT)
 
 #define CCI500_GLOBAL_EVENT_EXT_ATTR_ENTRY(_name, _config) \
 	CCI_EXT_ATTR_ENTRY(_name, cci500_pmu_global_event_show, \
@@ -846,6 +853,47 @@ static void __pmu_write_counter(struct cci_pmu *cci_pmu, int idx, u32 value)
 	pmu_write_register(cci_pmu, value, idx, CCI_PMU_CNTR);
 }
 
+#ifdef CONFIG_ARM_CCI500_PMU
+
+/*
+ * CCI-500 has advanced power saving policies, which could gate the
+ * clocks to the PMU counters, which makes the writes to them ineffective.
+ * The only way to write to those counters is when the global counters
+ * are enabled and the particular counter is enabled.
+ *
+ * So we do the following :
+ *
+ * 1) Disable all the PMU counters, saving their current state
+ * 2) Save the programmed event, and write an invalid event code
+ *    to the event control register for the counter, so that the
+ *    counters are not modified.
+ * 3) Enable the counter control for the counter.
+ * 4) Enable the global PMU profiling
+ * 5) Set the counter value
+ * 6) Disable the counter, global PMU.
+ * 7) Restore the event in the target counter
+ * 8) Restore the status of the rest of the counters.
+ */
+static void cci500_write_counter(struct cci_pmu *cci_pmu, int idx, u32 value)
+{
+	unsigned long mask[BITS_TO_LONGS(cci_pmu->num_cntrs)];
+	u32 event;
+
+	memset(mask, 0, BITS_TO_LONGS(cci_pmu->num_cntrs) * sizeof(unsigned long));
+
+	pmu_disable_counters_ctrl(cci_pmu, mask);
+	event = pmu_get_event(cci_pmu, idx);
+	pmu_set_event(cci_pmu, idx, CCI500_INVALID_EVENT);
+	pmu_enable_counter(cci_pmu, idx);
+	__cci_pmu_enable();
+	__pmu_write_counter(cci_pmu, idx, value);
+	__cci_pmu_disable();
+	pmu_disable_counter(cci_pmu, idx);
+	pmu_set_event(cci_pmu, idx, event);
+	pmu_restore_counters_ctrl(cci_pmu, mask);
+}
+
+#endif	/* ARM_CCI500_PMU */
 static void pmu_write_counter(struct perf_event *event, u32 value)
 {
 	struct cci_pmu *cci_pmu = to_cci_pmu(event->pmu);
@@ -856,7 +904,10 @@ static void pmu_write_counter(struct perf_event *event, u32 value)
 		dev_err(&cci_pmu->plat_device->dev, "Invalid CCI PMU counter %d\n", idx);
 		return;
 	}
-	__pmu_write_counter(cci_pmu, idx, value);
+	if (cci_pmu->model->write_counter)
+		cci_pmu->model->write_counter(cci_pmu, idx, value);
+	else
+		__pmu_write_counter(cci_pmu, idx, value);
 }
 
 static u64 pmu_event_update(struct perf_event *event)
@@ -1458,6 +1509,7 @@ static struct cci_pmu_model cci_pmu_models[] = {
 			},
 		},
 		.validate_hw_event = cci500_validate_hw_event,
+		.write_counter = cci500_write_counter,
 	},
 #endif
 };
-- 
1.7.9.5

Re: [PATCHv2 0/4] arm-cci500: Workaround pmu_event_set_period

[Olof Johansson]

On Tue, Oct 20, 2015 at 02:05:22PM +0100, Suzuki K. Poulose wrote:
> The CCI PMU driver sets the event counter to the half of the maximum
> value(2^31) it can count before we start the counters via
> pmu_event_set_period(). This is done to give us the best chance to
> handle the overflow interrupt, taking care of extreme interrupt latencies.
> 
> However, CCI-500 comes with advanced power saving schemes, which disables
> the clock to the event counters unless the counters are enabled to count
> (PMCR.CEN). This prevents the driver from writing the period to the
> counters before starting them.  Also, there is no way we can reset the
> individual event counter to 0 (PMCR.RST resets all the counters, losing
> their current readings). However the value of the counter is preserved and
> could be read back, when the counters are not enabled.
> 
> So we cannot reliably use the counters and compute the number of events
> generated during the sampling period since we don't have the value of the
> counter at start.
> 
> Here are the possible solutions:
> 
>  1) Disable clock gating on CCI-500 by setting Control_Override_Reg[bit3].
>     - The Control_Override_Reg is secure (and hence not programmable from
>       Linux), and also has an impact on power consumption.
> 
>  2) Change the order of operations
> 	i.e,
> 	a) Program and enable individual counters
> 	b) Enable counting on all the counters by setting PMCR.CEN
> 	c) Write the period to the individual counters
> 	d) Disable the counters
>     - This could cause in unnecessary noise in the other counters and is
>       costly (we should repeat this for all enabled counters).
> 
>  3) Don't set the counter value, instead use the current count as the
>     starting count and compute the delta at the end of sampling.
> 
>  4) Modified version of 2, which disables all the other counters, except
>     the target counter, with the target counter programmed with an invalid
>     event code(which guarantees that the counter won't change during the
>     operation).
> 
> This patch implements option 4 for CCI-500. CCI-400 behavior remains
> unchanged. The problem didn't surface on a fast model, but was revealed
> on an FPGA model. Without this patch profiling on CCI-500 is broken and
> should be fixed for 4.3.

Hi,

Given that CCI-500 is only now showing up in FPGA models, and is new hardware
support and not a regression, combined with the late time in the release cycle
means this is really 4.4 material by now.

Mark, can you review given that you've been involved already, please? Happy to
apply for 4.4 if it's ready for it soon.


-Olof

Re: [PATCHv2 0/4] arm-cci500: Workaround pmu_event_set_period

[Suzuki K. Poulose]

On Thu, Oct 22, 2015 at 10:00:25AM -0700, Olof Johansson wrote:
> On Tue, Oct 20, 2015 at 02:05:22PM +0100, Suzuki K. Poulose wrote:
> Hi,
> 
> Given that CCI-500 is only now showing up in FPGA models, and is new hardware
> support and not a regression, combined with the late time in the release cycle
> means this is really 4.4 material by now.

Yes, this can wait till 4.4. I missed updating that part from v1.

Thanks
Suzuki

Re: [PATCHv2 1/4] arm-cci: Refactor CCI PMU code

[Mark Rutland]

On Tue, Oct 20, 2015 at 02:05:23PM +0100, Suzuki K. Poulose wrote:
> This patch refactors the CCI PMU driver code a little bit to
> make it easier share the code for enabling/disabling the CCI
> PMU. This will be used by the hooks to work around the special cases
> where writing to a counter is not always that easy(e.g, CCI-500)
> 
> No functional changes.
> 
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: arm@kernel.org
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/bus/arm-cci.c |   94 +++++++++++++++++++++++++++++--------------------
>  1 file changed, 56 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 577cc4b..5435d87 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -738,6 +738,53 @@ static void pmu_free_irq(struct cci_pmu *cci_pmu)
>  	}
>  }
>  
> +/* Should be called with cci_pmu->hw_events->pmu_lock */
> +static void __cci_pmu_enable(void)
> +{
> +	u32 val;
> +
> +	/* Enable all the PMU counters. */
> +	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) | CCI_PMCR_CEN;
> +	writel(val, cci_ctrl_base + CCI_PMCR);
> +}
> +
> +/* Should be called with cci_pmu->hw_events->pmu_lock */
> +static void __cci_pmu_disable(void)
> +{
> +	u32 val;
> +
> +	/* Disable all the PMU counters. */
> +	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) & ~CCI_PMCR_CEN;
> +	writel(val, cci_ctrl_base + CCI_PMCR);
> +}
> +
> +static void cci_pmu_enable(struct pmu *pmu)
> +{
> +	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
> +	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
> +	int enabled = bitmap_weight(hw_events->used_mask, cci_pmu->num_cntrs);
> +	unsigned long flags;
> +
> +	if (!enabled)
> +		return;
> +
> +	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
> +	__cci_pmu_enable();
> +	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
> +
> +}
> +
> +static void cci_pmu_disable(struct pmu *pmu)
> +{
> +	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
> +	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
> +	unsigned long flags;
> +
> +	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
> +	__cci_pmu_disable();
> +	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
> +}

Why are these moved up here? It makes the diff harder to read, and it
doesn't seem necessary in the context of this patch.

Would they otherwise have to move in a later patch? It might be better
to move them when required (without changes).

>  static u32 pmu_read_counter(struct perf_event *event)
>  {
>  	struct cci_pmu *cci_pmu = to_cci_pmu(event->pmu);
> @@ -754,16 +801,22 @@ static u32 pmu_read_counter(struct perf_event *event)
>  	return value;
>  }
>  
> +static void __pmu_write_counter(struct cci_pmu *cci_pmu, int idx, u32 value)
> +{
> +	pmu_write_register(cci_pmu, value, idx, CCI_PMU_CNTR);
> +}
> +
>  static void pmu_write_counter(struct perf_event *event, u32 value)
>  {
>  	struct cci_pmu *cci_pmu = to_cci_pmu(event->pmu);
>  	struct hw_perf_event *hw_counter = &event->hw;
>  	int idx = hw_counter->idx;
>  
> -	if (unlikely(!pmu_is_valid_counter(cci_pmu, idx)))
> +	if (unlikely(!pmu_is_valid_counter(cci_pmu, idx))) {
>  		dev_err(&cci_pmu->plat_device->dev, "Invalid CCI PMU counter %d\n", idx);
> -	else
> -		pmu_write_register(cci_pmu, value, idx, CCI_PMU_CNTR);
> +		return;
> +	}
> +	__pmu_write_counter(cci_pmu, idx, value);
>  }

While I don't disagree with the new structure of this function, the
reorganisation wasn't necessary. We only need to substitute
__pmu_write_counter in place of pmu_write_register.

I'm happy with splitting out the lower level accessors, but I think the
additional reshuffling makes this patch overly complex. I'd prefer the
minial facotring out if possible.

Thanks,
Mark.

>  
>  static u64 pmu_event_update(struct perf_event *event)
> @@ -869,41 +922,6 @@ static void hw_perf_event_destroy(struct perf_event *event)
>  	}
>  }
>  
> -static void cci_pmu_enable(struct pmu *pmu)
> -{
> -	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
> -	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
> -	int enabled = bitmap_weight(hw_events->used_mask, cci_pmu->num_cntrs);
> -	unsigned long flags;
> -	u32 val;
> -
> -	if (!enabled)
> -		return;
> -
> -	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
> -
> -	/* Enable all the PMU counters. */
> -	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) | CCI_PMCR_CEN;
> -	writel(val, cci_ctrl_base + CCI_PMCR);
> -	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
> -
> -}
> -
> -static void cci_pmu_disable(struct pmu *pmu)
> -{
> -	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
> -	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
> -	unsigned long flags;
> -	u32 val;
> -
> -	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
> -
> -	/* Disable all the PMU counters. */
> -	val = readl_relaxed(cci_ctrl_base + CCI_PMCR) & ~CCI_PMCR_CEN;
> -	writel(val, cci_ctrl_base + CCI_PMCR);
> -	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
> -}
> -
>  /*
>   * Check if the idx represents a non-programmable counter.
>   * All the fixed event counters are mapped before the programmable
> -- 
> 1.7.9.5
> 

Re: [PATCHv2 2/4] arm-cci: Get the status of a counter

[Mark Rutland]

On Tue, Oct 20, 2015 at 02:05:24PM +0100, Suzuki K. Poulose wrote:
> Add helper routines to get the counter status and the event
> programmed on it.
> 
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: arm@kernel.org
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/bus/arm-cci.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 5435d87..1a75010 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -647,11 +647,21 @@ static void pmu_enable_counter(struct cci_pmu *cci_pmu, int idx)
>  	pmu_write_register(cci_pmu, 1, idx, CCI_PMU_CNTR_CTRL);
>  }
>  
> +static u32 pmu_get_counter_ctrl(struct cci_pmu *cci_pmu, int idx)
> +{
> +	return pmu_read_register(cci_pmu, idx, CCI_PMU_CNTR_CTRL) & 0x1;
> +}

Given the function is called pmu_get_counter_ctrl, why the '& 1'?

Either this should return the raw value, or the function should be
renamed to something like pmu_counter_is_enabled, and made bool.

> +
>  static void pmu_set_event(struct cci_pmu *cci_pmu, int idx, unsigned long event)
>  {
>  	pmu_write_register(cci_pmu, event, idx, CCI_PMU_EVT_SEL);
>  }
>  
> +static u32 pmu_get_event(struct cci_pmu *cci_pmu, int idx)
> +{
> +	return pmu_read_register(cci_pmu, idx, CCI_PMU_EVT_SEL);
> +}
> +

Other than the above this looks fine to me.

Thanks,
Mark.

Re: [PATCHv2 1/4] arm-cci: Refactor CCI PMU code

[Suzuki K. Poulose]

On 04/11/15 18:01, Mark Rutland wrote:
> On Tue, Oct 20, 2015 at 02:05:23PM +0100, Suzuki K. Poulose wrote:
>> This patch refactors the CCI PMU driver code a little bit to
>> make it easier share the code for enabling/disabling the CCI
>> PMU. This will be used by the hooks to work around the special cases
>> where writing to a counter is not always that easy(e.g, CCI-500)
>>


>> +static void cci_pmu_disable(struct pmu *pmu)
>> +{
>> +	struct cci_pmu *cci_pmu = to_cci_pmu(pmu);
>> +	struct cci_pmu_hw_events *hw_events = &cci_pmu->hw_events;
>> +	unsigned long flags;
>> +
>> +	raw_spin_lock_irqsave(&hw_events->pmu_lock, flags);
>> +	__cci_pmu_disable();
>> +	raw_spin_unlock_irqrestore(&hw_events->pmu_lock, flags);
>> +}
>
> Why are these moved up here? It makes the diff harder to read, and it
> doesn't seem necessary in the context of this patch.
>
> Would they otherwise have to move in a later patch? It might be better
> to move them when required (without changes).

These will be used later in cci500 specific routines to write the counter.
I can move them later.


>> -	if (unlikely(!pmu_is_valid_counter(cci_pmu, idx)))
>> +	if (unlikely(!pmu_is_valid_counter(cci_pmu, idx))) {
>>   		dev_err(&cci_pmu->plat_device->dev, "Invalid CCI PMU counter %d\n", idx);
>> -	else
>> -		pmu_write_register(cci_pmu, value, idx, CCI_PMU_CNTR);
>> +		return;
>> +	}
>> +	__pmu_write_counter(cci_pmu, idx, value);
>>   }
>
> While I don't disagree with the new structure of this function, the
> reorganisation wasn't necessary. We only need to substitute
> __pmu_write_counter in place of pmu_write_register.

We will add a check in Patch4/4 to override the default method with a
CCI_PMU model specific method.

>
> I'm happy with splitting out the lower level accessors, but I think the
> additional reshuffling makes this patch overly complex. I'd prefer the
> minial facotring out if possible.

Ok, I will rearrange the patches to make the changes readable.

Thanks
Suzuki

Re: [PATCHv2 2/4] arm-cci: Get the status of a counter

[Suzuki K. Poulose]

On 04/11/15 18:06, Mark Rutland wrote:
> On Tue, Oct 20, 2015 at 02:05:24PM +0100, Suzuki K. Poulose wrote:
>> Add helper routines to get the counter status and the event
>> programmed on it.
>>

>> +static u32 pmu_get_counter_ctrl(struct cci_pmu *cci_pmu, int idx)
>> +{
>> +	return pmu_read_register(cci_pmu, idx, CCI_PMU_CNTR_CTRL) & 0x1;
>> +}
>
> Given the function is called pmu_get_counter_ctrl, why the '& 1'?

Thats because the Count Control has only 1 bit defined. The rest is RES0.

>
> Either this should return the raw value, or the function should be
> renamed to something like pmu_counter_is_enabled, and made bool.

Makes sense. I will change it to pmu_counter_is_enabled().

Thanks
Suzuki

Re: [PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters

[Mark Rutland]

On Tue, Oct 20, 2015 at 02:05:25PM +0100, Suzuki K. Poulose wrote:
> Adds helper routines to manipulate the counter controls for
> all the counters on the CCI PMU.
> 
> pmu_disable_counters_ctrl: Iterates over the counters,
> checking the status of each counter and disabling any enabled
> counters. For each such changed counter, the mask is updated so that
> one can restore the state later using pmu_enable_counters_ctrl.
> 
> Cc: Punit Agrawal <punit.agrawal@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: arm@kernel.org
> Signed-off-by: Suzuki K. Poulose <suzuki.poulose@arm.com>
> ---
>  drivers/bus/arm-cci.c |   30 ++++++++++++++++++++++++++++++
>  1 file changed, 30 insertions(+)
> 
> diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c
> index 1a75010..7f4a266 100644
> --- a/drivers/bus/arm-cci.c
> +++ b/drivers/bus/arm-cci.c
> @@ -663,6 +663,36 @@ static u32 pmu_get_event(struct cci_pmu *cci_pmu, int idx)
>  }
>  
>  /*
> + * Restore the status of the counters.
> + * For each counter set in the mask, enable the counter back.
> + */
> +static void pmu_restore_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
> +{
> +	int i;
> +
> +	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
> +		pmu_enable_counter(cci_pmu, i);
> +}
> +
> +/*
> + * For all counters on the CCI-PMU, disable any 'enabled' counters,
> + * saving the changed counters in the mask, so that we can restore
> + * it later using pmu_restore_counters_ctrl.
> + */
> +static void pmu_disable_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
> +{
> +	int i;
> +
> +	for (i = 0; i < cci_pmu->num_cntrs; i++) {
> +		clear_bit(i, mask);
> +		if (pmu_get_counter_ctrl(cci_pmu, i)) {
> +			set_bit(i, mask);
> +			pmu_disable_counter(cci_pmu, i);
> +		}
> +	}
> +}

I don't understand what's going on with the mask here. Why do we clear
ieach bit when the only user (introduced in the next patch) explicitly
clears the mask anyway?

Can we not get rid of the mask entirely? The combination of used_mask
and each event's hwc->state tells us which counters are actually in use.

Thanks,
Mark.

Re: [PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters

[Suzuki K. Poulose]

On 04/11/15 18:28, Mark Rutland wrote:
> On Tue, Oct 20, 2015 at 02:05:25PM +0100, Suzuki K. Poulose wrote:
>> Adds helper routines to manipulate the counter controls for
>> all the counters on the CCI PMU.
>>
>> pmu_disable_counters_ctrl: Iterates over the counters,
>> checking the status of each counter and disabling any enabled
>> counters. For each such changed counter, the mask is updated so that
>> one can restore the state later using pmu_enable_counters_ctrl.
>>

>>   /*
>> + * Restore the status of the counters.
>> + * For each counter set in the mask, enable the counter back.
>> + */
>> +static void pmu_restore_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
>> +{
>> +	int i;
>> +
>> +	for_each_set_bit(i, mask, cci_pmu->num_cntrs)
>> +		pmu_enable_counter(cci_pmu, i);
>> +}
>> +
>> +/*
>> + * For all counters on the CCI-PMU, disable any 'enabled' counters,
>> + * saving the changed counters in the mask, so that we can restore
>> + * it later using pmu_restore_counters_ctrl.
>> + */
>> +static void pmu_disable_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < cci_pmu->num_cntrs; i++) {
>> +		clear_bit(i, mask);
>> +		if (pmu_get_counter_ctrl(cci_pmu, i)) {
>> +			set_bit(i, mask);
>> +			pmu_disable_counter(cci_pmu, i);
>> +		}
>> +	}
>> +}
>
> I don't understand what's going on with the mask here. Why do we clear
> ieach bit when the only user (introduced in the next patch) explicitly
> clears the mask anyway?

To be more precise, it should have been :

	if (pmu_get_counter_ctrl(cci_pmu, i)) {
		set_bit(i, mask);
		pmu_disable_counter(cci_pmu, i);
	} else
		clear_bit(i, mask);

>
> Can we not get rid of the mask entirely? The combination of used_mask
> and each event's hwc->state tells us which counters are actually in use.

The problem is that neither hwc->state nor the cci_pmu->hw_events->events is
protected by pmu_lock, while enable/disable counter is. So we cannot really
rely on ((struct perf_event *)(cci_pmu->hw_events->events[counter]))->hw->state.

What we do above is, create a mask of the counters which are enabled at the
moment and disable all of them. We then program the counter and then re-enable
those which were enabled (as marked in the mask).

Suzuki


>
> Thanks,
> Mark.
>

Re: [PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters

[Suzuki K. Poulose]

On 05/11/15 10:14, Suzuki K. Poulose wrote:
> On 04/11/15 18:28, Mark Rutland wrote:
>> On Tue, Oct 20, 2015 at 02:05:25PM +0100, Suzuki K. Poulose wrote:
>>> Adds helper routines to manipulate the counter controls for
>>> all the counters on the CCI PMU.

>>> +static void pmu_disable_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
>>> +{
>>> +    int i;
>>> +
>>> +    for (i = 0; i < cci_pmu->num_cntrs; i++) {
>>> +        clear_bit(i, mask);
>>> +        if (pmu_get_counter_ctrl(cci_pmu, i)) {
>>> +            set_bit(i, mask);
>>> +            pmu_disable_counter(cci_pmu, i);
>>> +        }
>>> +    }
>>> +}
>>
>> I don't understand what's going on with the mask here. Why do we clear
>> ieach bit when the only user (introduced in the next patch) explicitly
>> clears the mask anyway?
>
> To be more precise, it should have been :
>
>      if (pmu_get_counter_ctrl(cci_pmu, i)) {
>          set_bit(i, mask);
>          pmu_disable_counter(cci_pmu, i);
>      } else
>          clear_bit(i, mask);

Forgot to mention, the explicit clearing is for the bits that may be
beyond the num_counters. Since we limit it to cci_pmu->num_cntrs here
we could get rid of that.
  
Thanks
Suzuki

Re: [PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters

[Mark Rutland]

> >>+static void pmu_disable_counters_ctrl(struct cci_pmu *cci_pmu, unsigned long *mask)
> >>+{
> >>+	int i;
> >>+
> >>+	for (i = 0; i < cci_pmu->num_cntrs; i++) {
> >>+		clear_bit(i, mask);
> >>+		if (pmu_get_counter_ctrl(cci_pmu, i)) {
> >>+			set_bit(i, mask);
> >>+			pmu_disable_counter(cci_pmu, i);
> >>+		}
> >>+	}
> >>+}
> >
> >I don't understand what's going on with the mask here. Why do we clear
> >ieach bit when the only user (introduced in the next patch) explicitly
> >clears the mask anyway?
> 
> To be more precise, it should have been :
> 
> 	if (pmu_get_counter_ctrl(cci_pmu, i)) {
> 		set_bit(i, mask);
> 		pmu_disable_counter(cci_pmu, i);
> 	} else
> 		clear_bit(i, mask);
> 
> >
> >Can we not get rid of the mask entirely? The combination of used_mask
> >and each event's hwc->state tells us which counters are actually in use.
> 
> The problem is that neither hwc->state nor the cci_pmu->hw_events->events is
> protected by pmu_lock, while enable/disable counter is. So we cannot really
> rely on ((struct perf_event *)(cci_pmu->hw_events->events[counter]))->hw->state.

They must be protected somehow, or we'd have races against cross-calls
and/or the interrupt handler.

Are we protected due to being cpu-affine with interrupts disabled when
modifying these, is there some other mechanism that protects us, or do
we have additional problems here?

Thanks,
Mark.

Re: [PATCHv2 3/4] arm-cci: Add routines to enable/disable all counters

[Suzuki K. Poulose]

On 05/11/15 17:27, Mark Rutland wrote:
>>> Can we not get rid of the mask entirely? The combination of used_mask
>>> and each event's hwc->state tells us which counters are actually in use.
>>
>> The problem is that neither hwc->state nor the cci_pmu->hw_events->events is
>> protected by pmu_lock, while enable/disable counter is. So we cannot really
>> rely on ((struct perf_event *)(cci_pmu->hw_events->events[counter]))->hw->state.
>
> They must be protected somehow, or we'd have races against cross-calls
> and/or the interrupt handler.

>
> Are we protected due to being cpu-affine with interrupts disabled when
> modifying these, is there some other mechanism that protects us, or do
> we have additional problems here?
>


Each perf_event is allocated a counter id atomically using the bit mask. So,
once the id is allocated nobody messes with that id from the PMU side. And,
the hw->state may have its own protection within the generic perf layer(which
I haven't checked).

Thanks
Suzuki