[PATCH 0/7] OpenSBI PMU improvements

[PATCH 0/7] OpenSBI PMU improvements

[Anup Patel]

This series aims to improve SBI PMU implementation in following ways:
1) Optimize memory usage by reducing global data to track per-HART firmware
   counters (almost 80KB saved)
2) Allow platform to implement custom firmware counters and quirks related
   to hardware counters (This will be useful for T-Head)
3) Few other fixes for better SBI spec compliance

These patches can also be found in sbi_pmu_imp_v1 branch at:
https://github.com/avpatel/opensbi.git

Anup Patel (7):
  lib: sbi_pmu: Remove "event_idx" member from struct sbi_pmu_fw_event
  lib: sbi_pmu: Replace sbi_pmu_ctr_read() with sbi_pmu_ctr_fw_read()
  lib: sbi_pmu: Firmware counters are always 64 bits wide
  lib: sbi_pmu: Simplify FW counters to reduce memory usage
  lib: sbi_pmu: Add custom PMU device operations
  lib: sbi: Print platform PMU device at boot-time
  include: sbi: Reduce includes in sbi_pmu.h

 include/sbi/sbi_pmu.h   |  67 ++++++++++--
 lib/sbi/sbi_ecall_pmu.c |   3 +-
 lib/sbi/sbi_init.c      |   4 +
 lib/sbi/sbi_pmu.c       | 218 +++++++++++++++++++++++-----------------
 lib/sbi/sbi_trap.c      |   1 +
 5 files changed, 190 insertions(+), 103 deletions(-)

-- 
2.34.1

[PATCH 1/7] lib: sbi_pmu: Remove "event_idx" member from struct sbi_pmu_fw_event

[Anup Patel]

The "event_idx" member of struct sbi_pmu_fw_event is not used
anywhere so let us remove it.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 lib/sbi/sbi_pmu.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 5a294a2..535e5cc 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -35,9 +35,6 @@ struct sbi_pmu_hw_event {
 
 /** Representation of a firmware event */
 struct sbi_pmu_fw_event {
-	/* Event associated with the particular counter */
-	uint32_t event_idx;
-
 	/* Current value of the counter */
 	unsigned long curr_count;
 
-- 
2.34.1

[PATCH 2/7] lib: sbi_pmu: Replace sbi_pmu_ctr_read() with sbi_pmu_ctr_fw_read()

[Anup Patel]

The "read a firmware counter" SBI call should only work for firmware
counters so let us replace sbi_pmu_ctr_read() with sbi_pmu_ctr_fw_read()
which works only on firmware counters.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 include/sbi/sbi_pmu.h   |  2 +-
 lib/sbi/sbi_ecall_pmu.c |  3 ++-
 lib/sbi/sbi_pmu.c       | 45 +++++++----------------------------------
 3 files changed, 10 insertions(+), 40 deletions(-)

diff --git a/include/sbi/sbi_pmu.h b/include/sbi/sbi_pmu.h
index f5e3dbd..34336f7 100644
--- a/include/sbi/sbi_pmu.h
+++ b/include/sbi/sbi_pmu.h
@@ -53,7 +53,7 @@ int sbi_pmu_add_hw_event_counter_map(u32 eidx_start, u32 eidx_end, u32 cmap);
 
 int sbi_pmu_add_raw_event_counter_map(uint64_t select, uint64_t select_mask, u32 cmap);
 
-int sbi_pmu_ctr_read(uint32_t cidx, unsigned long *cval);
+int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval);
 
 int sbi_pmu_ctr_stop(unsigned long cidx_base, unsigned long cidx_mask,
 		     unsigned long flag);
diff --git a/lib/sbi/sbi_ecall_pmu.c b/lib/sbi/sbi_ecall_pmu.c
index 9ee9e81..826c8a8 100644
--- a/lib/sbi/sbi_ecall_pmu.c
+++ b/lib/sbi/sbi_ecall_pmu.c
@@ -51,7 +51,8 @@ static int sbi_ecall_pmu_handler(unsigned long extid, unsigned long funcid,
 
 		break;
 	case SBI_EXT_PMU_COUNTER_FW_READ:
-		ret = sbi_pmu_ctr_read(regs->a0, out_val);
+		ret = sbi_pmu_ctr_fw_read(regs->a0, &temp);
+		*out_val = temp;
 		break;
 	case SBI_EXT_PMU_COUNTER_START:
 
diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 535e5cc..250808e 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -167,50 +167,19 @@ static int pmu_ctr_validate(uint32_t cidx, uint32_t *event_idx_code)
 	return event_idx_type;
 }
 
-static int pmu_ctr_read_fw(uint32_t cidx, unsigned long *cval,
-			       uint32_t fw_evt_code)
-{
-	u32 hartid = current_hartid();
-	struct sbi_pmu_fw_event fevent;
-
-	fevent = fw_event_map[hartid][fw_evt_code];
-	*cval = fevent.curr_count;
-
-	return 0;
-}
-
-/* Add a hardware counter read for completeness for future purpose */
-static int pmu_ctr_read_hw(uint32_t cidx, uint64_t *cval)
-{
-	/* Check for invalid hw counter read requests */
-	if (unlikely(cidx == 1))
-		return SBI_EINVAL;
-#if __riscv_xlen == 32
-	uint32_t temp, temph = 0;
-
-	temp = csr_read_num(CSR_MCYCLE + cidx);
-	temph = csr_read_num(CSR_MCYCLEH + cidx);
-	*cval = ((uint64_t)temph << 32) | temp;
-#else
-	*cval = csr_read_num(CSR_MCYCLE + cidx);
-#endif
-
-	return 0;
-}
-
-int sbi_pmu_ctr_read(uint32_t cidx, unsigned long *cval)
+int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
 {
 	int event_idx_type;
 	uint32_t event_code;
-	uint64_t cval64;
+	u32 hartid = current_hartid();
+	struct sbi_pmu_fw_event *fevent;
 
 	event_idx_type = pmu_ctr_validate(cidx, &event_code);
-	if (event_idx_type < 0)
+	if (event_idx_type != SBI_PMU_EVENT_TYPE_FW)
 		return SBI_EINVAL;
-	else if (event_idx_type == SBI_PMU_EVENT_TYPE_FW)
-		pmu_ctr_read_fw(cidx, cval, event_code);
-	else
-		pmu_ctr_read_hw(cidx, &cval64);
+
+	fevent = &fw_event_map[hartid][event_code];
+	*cval = fevent->curr_count;
 
 	return 0;
 }
-- 
2.34.1

[PATCH 3/7] lib: sbi_pmu: Firmware counters are always 64 bits wide

[Anup Patel]

As-per SBI specification, all firmware counters are always 64 bits
wide so let us update the SBI PMU implementation to reflect this fact.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 lib/sbi/sbi_pmu.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 250808e..1c01d34 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -36,7 +36,7 @@ struct sbi_pmu_hw_event {
 /** Representation of a firmware event */
 struct sbi_pmu_fw_event {
 	/* Current value of the counter */
-	unsigned long curr_count;
+	uint64_t curr_count;
 
 	/* A flag indicating pmu event monitoring is started */
 	bool bStarted;
@@ -719,8 +719,8 @@ int sbi_pmu_ctr_get_info(uint32_t cidx, unsigned long *ctr_info)
 	} else {
 		/* it's a firmware counter */
 		cinfo.type = SBI_PMU_CTR_TYPE_FW;
-		/* Firmware counters are XLEN bits wide */
-		cinfo.width = BITS_PER_LONG - 1;
+		/* Firmware counters are always 64 bits wide */
+		cinfo.width = 63;
 	}
 
 	*ctr_info = cinfo.value;
-- 
2.34.1

[PATCH 4/7] lib: sbi_pmu: Simplify FW counters to reduce memory usage

[Anup Patel]

Currently, we have 32 elements (i.e. SBI_PMU_FW_EVENT_MAX) array of
"struct sbi_pmu_fw_event" for each of 128 possible HARTs
(i.e. SBI_HARTMASK_MAX_BITS).

To reduce memory usage of OpenSBI, we update FW counter implementation
as follows:
1) Remove SBI_PMU_FW_EVENT_MAX
2) Remove "struct sbi_pmu_fw_event"
3) Create per-HART bitmap of XLEN bits to track FW counters
   which are started on each HART
4) Create per-HART uint64_t array to track values of FW
   counters on each HART.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 include/sbi/sbi_pmu.h |  3 --
 lib/sbi/sbi_pmu.c     | 78 ++++++++++++++++++++-----------------------
 2 files changed, 36 insertions(+), 45 deletions(-)

diff --git a/include/sbi/sbi_pmu.h b/include/sbi/sbi_pmu.h
index 34336f7..39cf007 100644
--- a/include/sbi/sbi_pmu.h
+++ b/include/sbi/sbi_pmu.h
@@ -19,9 +19,6 @@
 /* Maximum number of hardware events that can mapped by OpenSBI */
 #define SBI_PMU_HW_EVENT_MAX 256
 
-/* Maximum number of firmware events that can mapped by OpenSBI */
-#define SBI_PMU_FW_EVENT_MAX 32
-
 /* Counter related macros */
 #define SBI_PMU_FW_CTR_MAX 16
 #define SBI_PMU_HW_CTR_MAX 32
diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 1c01d34..26c9406 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -33,15 +33,6 @@ struct sbi_pmu_hw_event {
 	uint64_t select_mask;
 };
 
-/** Representation of a firmware event */
-struct sbi_pmu_fw_event {
-	/* Current value of the counter */
-	uint64_t curr_count;
-
-	/* A flag indicating pmu event monitoring is started */
-	bool bStarted;
-};
-
 /* Information about PMU counters as per SBI specification */
 union sbi_pmu_ctr_info {
 	unsigned long value;
@@ -63,8 +54,14 @@ static struct sbi_pmu_hw_event hw_event_map[SBI_PMU_HW_EVENT_MAX] = {0};
 /* counter to enabled event mapping */
 static uint32_t active_events[SBI_HARTMASK_MAX_BITS][SBI_PMU_HW_CTR_MAX + SBI_PMU_FW_CTR_MAX];
 
-/* Contains all the information about firmwares events */
-static struct sbi_pmu_fw_event fw_event_map[SBI_HARTMASK_MAX_BITS][SBI_PMU_FW_EVENT_MAX] = {0};
+/* Bitmap of firmware counters started on each HART */
+#if SBI_PMU_FW_CTR_MAX >= BITS_PER_LONG
+#error "Can't handle firmware counters beyond BITS_PER_LONG"
+#endif
+static unsigned long fw_counters_started[SBI_HARTMASK_MAX_BITS];
+
+/* Values of firmwares counters on each HART */
+static uint64_t fw_counters_value[SBI_HARTMASK_MAX_BITS][SBI_PMU_FW_CTR_MAX] = {0};
 
 /* Maximum number of hardware events available */
 static uint32_t num_hw_events;
@@ -172,14 +169,12 @@ int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
 	int event_idx_type;
 	uint32_t event_code;
 	u32 hartid = current_hartid();
-	struct sbi_pmu_fw_event *fevent;
 
 	event_idx_type = pmu_ctr_validate(cidx, &event_code);
 	if (event_idx_type != SBI_PMU_EVENT_TYPE_FW)
 		return SBI_EINVAL;
 
-	fevent = &fw_event_map[hartid][event_code];
-	*cval = fevent->curr_count;
+	*cval = fw_counters_value[hartid][cidx - num_hw_ctrs];
 
 	return 0;
 }
@@ -333,16 +328,13 @@ skip_inhibit_update:
 	return 0;
 }
 
-static int pmu_ctr_start_fw(uint32_t cidx, uint32_t fw_evt_code,
-			    uint64_t ival, bool ival_update)
+static int pmu_ctr_start_fw(uint32_t cidx, uint64_t ival, bool ival_update)
 {
 	u32 hartid = current_hartid();
-	struct sbi_pmu_fw_event *fevent;
 
-	fevent = &fw_event_map[hartid][fw_evt_code];
 	if (ival_update)
-		fevent->curr_count = ival;
-	fevent->bStarted = TRUE;
+		fw_counters_value[hartid][cidx - num_hw_ctrs] = ival;
+	fw_counters_started[hartid] |= BIT(cidx - num_hw_ctrs);
 
 	return 0;
 }
@@ -369,7 +361,7 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
 			/* Continue the start operation for other counters */
 			continue;
 		else if (event_idx_type == SBI_PMU_EVENT_TYPE_FW)
-			ret = pmu_ctr_start_fw(cidx, event_code, ival, bUpdate);
+			ret = pmu_ctr_start_fw(cidx, ival, bUpdate);
 		else
 			ret = pmu_ctr_start_hw(cidx, ival, bUpdate);
 	}
@@ -399,11 +391,9 @@ static int pmu_ctr_stop_hw(uint32_t cidx)
 		return SBI_EALREADY_STOPPED;
 }
 
-static int pmu_ctr_stop_fw(uint32_t cidx, uint32_t fw_evt_code)
+static int pmu_ctr_stop_fw(uint32_t cidx)
 {
-	u32 hartid = current_hartid();
-
-	fw_event_map[hartid][fw_evt_code].bStarted = FALSE;
+	fw_counters_started[current_hartid()] &= ~BIT(cidx - num_hw_ctrs);
 
 	return 0;
 }
@@ -444,7 +434,7 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
 			continue;
 
 		else if (event_idx_type == SBI_PMU_EVENT_TYPE_FW)
-			ret = pmu_ctr_stop_fw(cidx, event_code);
+			ret = pmu_ctr_stop_fw(cidx);
 		else
 			ret = pmu_ctr_stop_hw(cidx);
 
@@ -624,8 +614,6 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
 	int ctr_idx = SBI_ENOTSUPP;
 	u32 hartid = current_hartid();
 	int event_type;
-	struct sbi_pmu_fw_event *fevent;
-	uint32_t fw_evt_code;
 
 	/* Do a basic sanity check of counter base & mask */
 	if ((cidx_base + sbi_fls(cidx_mask)) >= total_ctrs)
@@ -665,30 +653,36 @@ skip_match:
 		if (flags & SBI_PMU_CFG_FLAG_AUTO_START)
 			pmu_ctr_start_hw(ctr_idx, 0, false);
 	} else if (event_type == SBI_PMU_EVENT_TYPE_FW) {
-		fw_evt_code = get_cidx_code(event_idx);
-		fevent = &fw_event_map[hartid][fw_evt_code];
 		if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
-			fevent->curr_count = 0;
+			fw_counters_value[hartid][ctr_idx - num_hw_ctrs] = 0;
 		if (flags & SBI_PMU_CFG_FLAG_AUTO_START)
-			fevent->bStarted = TRUE;
+			fw_counters_started[hartid] |= BIT(ctr_idx - num_hw_ctrs);
 	}
 
 	return ctr_idx;
 }
 
-inline int sbi_pmu_ctr_incr_fw(enum sbi_pmu_fw_event_code_id fw_id)
+int sbi_pmu_ctr_incr_fw(enum sbi_pmu_fw_event_code_id fw_id)
 {
-	u32 hartid = current_hartid();
-	struct sbi_pmu_fw_event *fevent;
+	u32 cidx, hartid = current_hartid();
+	uint64_t *fcounter = NULL;
+
+	if (likely(!fw_counters_started[hartid]))
+		return 0;
 
 	if (unlikely(fw_id >= SBI_PMU_FW_MAX))
 		return SBI_EINVAL;
 
-	fevent = &fw_event_map[hartid][fw_id];
+	for (cidx = num_hw_ctrs; cidx < total_ctrs; cidx++) {
+		if (get_cidx_code(active_events[hartid][cidx]) == fw_id &&
+		    (fw_counters_started[hartid] & BIT(cidx - num_hw_ctrs))) {
+			fcounter = &fw_counters_value[hartid][cidx - num_hw_ctrs];
+			break;
+		}
+	}
 
-	/* PMU counters will be only enabled during performance debugging */
-	if (unlikely(fevent->bStarted))
-		fevent->curr_count++;
+	if (fcounter)
+		(*fcounter)++;
 
 	return 0;
 }
@@ -735,9 +729,9 @@ static void pmu_reset_event_map(u32 hartid)
 	/* Initialize the counter to event mapping table */
 	for (j = 3; j < total_ctrs; j++)
 		active_events[hartid][j] = SBI_PMU_EVENT_IDX_INVALID;
-	for (j = 0; j < SBI_PMU_FW_EVENT_MAX; j++)
-		sbi_memset(&fw_event_map[hartid][j], 0,
-			   sizeof(struct sbi_pmu_fw_event));
+	for (j = 0; j < SBI_PMU_FW_CTR_MAX; j++)
+		fw_counters_value[hartid][j] = 0;
+	fw_counters_started[hartid] = 0;
 }
 
 void sbi_pmu_exit(struct sbi_scratch *scratch)
-- 
2.34.1

[PATCH 5/7] lib: sbi_pmu: Add custom PMU device operations

[Anup Patel]

We extend SBI PMU implementation to allow custom PMU device operations
which a platform can use for platform specific quirks.

The custom PMU device operations added by this patch include:
1) Operations to allow a platform implement custom firmware events.
   These custom firmware events can be SBI vendor extension related
   events or platform specific per-HART events are not possible to
   count through HPM CSRs.
2) Operations to allow a platform implement custom way for enabling
   (or disabling) an overflow interrupt (e.g. T-Head C9xx).

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
---
 include/sbi/sbi_pmu.h |  57 ++++++++++++++++++++++
 lib/sbi/sbi_pmu.c     | 108 ++++++++++++++++++++++++++++++++++--------
 2 files changed, 144 insertions(+), 21 deletions(-)

diff --git a/include/sbi/sbi_pmu.h b/include/sbi/sbi_pmu.h
index 39cf007..d787575 100644
--- a/include/sbi/sbi_pmu.h
+++ b/include/sbi/sbi_pmu.h
@@ -25,6 +25,63 @@
 #define SBI_PMU_CTR_MAX	   (SBI_PMU_HW_CTR_MAX + SBI_PMU_FW_CTR_MAX)
 #define SBI_PMU_FIXED_CTR_MASK 0x07
 
+struct sbi_pmu_device {
+	/** Name of the PMU platform device */
+	char name[32];
+
+	/**
+	 * Validate event code of custom firmware event
+	 * Note: SBI_PMU_FW_MAX <= event_idx_code
+	 */
+	int (*fw_event_validate_code)(uint32_t event_idx_code);
+
+	/**
+	 * Match custom firmware counter with custom firmware event
+	 * Note: 0 <= counter_index < SBI_PMU_FW_CTR_MAX
+	 */
+	bool (*fw_counter_match_code)(uint32_t counter_index,
+				      uint32_t event_idx_code);
+
+	/**
+	 * Read value of custom firmware counter
+	 * Note: 0 <= counter_index < SBI_PMU_FW_CTR_MAX
+	 */
+	uint64_t (*fw_counter_read_value)(uint32_t counter_index);
+
+	/**
+	 * Start custom firmware counter
+	 * Note: SBI_PMU_FW_MAX <= event_idx_code
+	 * Note: 0 <= counter_index < SBI_PMU_FW_CTR_MAX
+	 */
+	int (*fw_counter_start)(uint32_t counter_index,
+				uint32_t event_idx_code,
+				uint64_t init_val, bool init_val_update);
+
+	/**
+	 * Stop custom firmware counter
+	 * Note: 0 <= counter_index < SBI_PMU_FW_CTR_MAX
+	 */
+	int (*fw_counter_stop)(uint32_t counter_index);
+
+	/**
+	 * Custom enable irq for hardware counter
+	 * Note: 0 <= counter_index < SBI_PMU_HW_CTR_MAX
+	 */
+	void (*hw_counter_enable_irq)(uint32_t counter_index);
+
+	/**
+	 * Custom disable irq for hardware counter
+	 * Note: 0 <= counter_index < SBI_PMU_HW_CTR_MAX
+	 */
+	void (*hw_counter_disable_irq)(uint32_t counter_index);
+};
+
+/** Get the PMU platform device */
+const struct sbi_pmu_device *sbi_pmu_get_device(void);
+
+/** Set the PMU platform device */
+void sbi_pmu_set_device(const struct sbi_pmu_device *dev);
+
 /** Initialize PMU */
 int sbi_pmu_init(struct sbi_scratch *scratch, bool cold_boot);
 
diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 26c9406..87e1ff8 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -48,6 +48,9 @@ union sbi_pmu_ctr_info {
 	};
 };
 
+/* Platform specific PMU device */
+static const struct sbi_pmu_device *pmu_dev = NULL;
+
 /* Mapping between event range and possible counters  */
 static struct sbi_pmu_hw_event hw_event_map[SBI_PMU_HW_EVENT_MAX] = {0};
 
@@ -113,7 +116,11 @@ static int pmu_event_validate(unsigned long event_idx)
 		event_idx_code_max = SBI_PMU_HW_GENERAL_MAX;
 		break;
 	case SBI_PMU_EVENT_TYPE_FW:
-		event_idx_code_max = SBI_PMU_FW_MAX;
+		if (SBI_PMU_FW_MAX <= event_idx_code &&
+		    pmu_dev && pmu_dev->fw_event_validate_code)
+			return pmu_dev->fw_event_validate_code(event_idx_code);
+		else
+			event_idx_code_max = SBI_PMU_FW_MAX;
 		break;
 	case SBI_PMU_EVENT_TYPE_HW_CACHE:
 		cache_ops_result = event_idx_code &
@@ -174,6 +181,11 @@ int sbi_pmu_ctr_fw_read(uint32_t cidx, uint64_t *cval)
 	if (event_idx_type != SBI_PMU_EVENT_TYPE_FW)
 		return SBI_EINVAL;
 
+	if (SBI_PMU_FW_MAX <= event_code &&
+	    pmu_dev && pmu_dev->fw_counter_read_value)
+		fw_counters_value[hartid][cidx - num_hw_ctrs] =
+			pmu_dev->fw_counter_read_value(cidx - num_hw_ctrs);
+
 	*cval = fw_counters_value[hartid][cidx - num_hw_ctrs];
 
 	return 0;
@@ -319,6 +331,8 @@ static int pmu_ctr_start_hw(uint32_t cidx, uint64_t ival, bool ival_update)
 
 	if (sbi_hart_has_extension(scratch, SBI_HART_EXT_SSCOFPMF))
 		pmu_ctr_enable_irq_hw(cidx);
+	if (pmu_dev && pmu_dev->hw_counter_enable_irq)
+		pmu_dev->hw_counter_enable_irq(cidx);
 	csr_write(CSR_MCOUNTINHIBIT, mctr_inhbt);
 
 skip_inhibit_update:
@@ -328,10 +342,21 @@ skip_inhibit_update:
 	return 0;
 }
 
-static int pmu_ctr_start_fw(uint32_t cidx, uint64_t ival, bool ival_update)
+static int pmu_ctr_start_fw(uint32_t cidx, uint32_t event_code,
+			    uint64_t ival, bool ival_update)
 {
+	int ret;
 	u32 hartid = current_hartid();
 
+	if (SBI_PMU_FW_MAX <= event_code &&
+	    pmu_dev && pmu_dev->fw_counter_start) {
+		ret = pmu_dev->fw_counter_start(cidx - num_hw_ctrs,
+						event_code,
+						ival, ival_update);
+		if (ret)
+			return ret;
+	}
+
 	if (ival_update)
 		fw_counters_value[hartid][cidx - num_hw_ctrs] = ival;
 	fw_counters_started[hartid] |= BIT(cidx - num_hw_ctrs);
@@ -361,7 +386,7 @@ int sbi_pmu_ctr_start(unsigned long cbase, unsigned long cmask,
 			/* Continue the start operation for other counters */
 			continue;
 		else if (event_idx_type == SBI_PMU_EVENT_TYPE_FW)
-			ret = pmu_ctr_start_fw(cidx, ival, bUpdate);
+			ret = pmu_ctr_start_fw(cidx, event_code, ival, bUpdate);
 		else
 			ret = pmu_ctr_start_hw(cidx, ival, bUpdate);
 	}
@@ -391,8 +416,17 @@ static int pmu_ctr_stop_hw(uint32_t cidx)
 		return SBI_EALREADY_STOPPED;
 }
 
-static int pmu_ctr_stop_fw(uint32_t cidx)
+static int pmu_ctr_stop_fw(uint32_t cidx, uint32_t event_code)
 {
+	int ret;
+
+	if (SBI_PMU_FW_MAX <= event_code &&
+	    pmu_dev && pmu_dev->fw_counter_stop) {
+		ret = pmu_dev->fw_counter_stop(cidx - num_hw_ctrs);
+		if (ret)
+			return ret;
+	}
+
 	fw_counters_started[current_hartid()] &= ~BIT(cidx - num_hw_ctrs);
 
 	return 0;
@@ -434,7 +468,7 @@ int sbi_pmu_ctr_stop(unsigned long cbase, unsigned long cmask,
 			continue;
 
 		else if (event_idx_type == SBI_PMU_EVENT_TYPE_FW)
-			ret = pmu_ctr_stop_fw(cidx);
+			ret = pmu_ctr_stop_fw(cidx, event_code);
 		else
 			ret = pmu_ctr_stop_hw(cidx);
 
@@ -481,6 +515,9 @@ static int pmu_update_hw_mhpmevent(struct sbi_pmu_hw_event *hw_evt, int ctr_idx,
 		mhpmevent_val = (mhpmevent_val & ~MHPMEVENT_SSCOF_MASK) |
 				 MHPMEVENT_MINH | MHPMEVENT_OF;
 
+	if (pmu_dev && pmu_dev->hw_counter_disable_irq)
+		pmu_dev->hw_counter_disable_irq(ctr_idx);
+
 	/* Update the inhibit flags based on inhibit flags received from supervisor */
 	pmu_update_inhibit_flags(flags, &mhpmevent_val);
 
@@ -588,21 +625,26 @@ static int pmu_ctr_find_hw(unsigned long cbase, unsigned long cmask, unsigned lo
  * Thus, select the first available fw counter after sanity
  * check.
  */
-static int pmu_ctr_find_fw(unsigned long cbase, unsigned long cmask, u32 hartid)
+static int pmu_ctr_find_fw(unsigned long cbase, unsigned long cmask,
+			   uint32_t event_code, u32 hartid)
 {
-	int i = 0;
-	int fw_base;
-	unsigned long ctr_mask = cmask << cbase;
+	int i, cidx;
 
-	if (cbase < num_hw_ctrs)
-		fw_base = num_hw_ctrs;
-	else
-		fw_base = cbase;
+	for_each_set_bit(i, &cmask, BITS_PER_LONG) {
+		cidx = i + cbase;
+		if (cidx < num_hw_ctrs || total_ctrs <= cidx)
+			continue;
+		if (active_events[hartid][i] != SBI_PMU_EVENT_IDX_INVALID)
+			continue;
+		if (SBI_PMU_FW_MAX <= event_code &&
+		    pmu_dev && pmu_dev->fw_counter_match_code) {
+			if (!pmu_dev->fw_counter_match_code(cidx - num_hw_ctrs,
+							    event_code))
+				continue;
+		}
 
-	for (i = fw_base; i < total_ctrs; i++)
-		if ((active_events[hartid][i] == SBI_PMU_EVENT_IDX_INVALID) &&
-		    ((1UL << i) & ctr_mask))
-			return i;
+		return i;
+	}
 
 	return SBI_ENOTSUPP;
 }
@@ -611,8 +653,8 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
 			  unsigned long flags, unsigned long event_idx,
 			  uint64_t event_data)
 {
-	int ctr_idx = SBI_ENOTSUPP;
-	u32 hartid = current_hartid();
+	int ret, ctr_idx = SBI_ENOTSUPP;
+	u32 event_code, hartid = current_hartid();
 	int event_type;
 
 	/* Do a basic sanity check of counter base & mask */
@@ -622,6 +664,7 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
 	event_type = pmu_event_validate(event_idx);
 	if (event_type < 0)
 		return SBI_EINVAL;
+	event_code = get_cidx_code(event_idx);
 
 	if (flags & SBI_PMU_CFG_FLAG_SKIP_MATCH) {
 		/* The caller wants to skip the match because it already knows the
@@ -636,7 +679,7 @@ int sbi_pmu_ctr_cfg_match(unsigned long cidx_base, unsigned long cidx_mask,
 
 	if (event_type == SBI_PMU_EVENT_TYPE_FW) {
 		/* Any firmware counter can be used track any firmware event */
-		ctr_idx = pmu_ctr_find_fw(cidx_base, cidx_mask, hartid);
+		ctr_idx = pmu_ctr_find_fw(cidx_base, cidx_mask, event_code, hartid);
 	} else {
 		ctr_idx = pmu_ctr_find_hw(cidx_base, cidx_mask, flags, event_idx,
 					  event_data);
@@ -655,8 +698,18 @@ skip_match:
 	} else if (event_type == SBI_PMU_EVENT_TYPE_FW) {
 		if (flags & SBI_PMU_CFG_FLAG_CLEAR_VALUE)
 			fw_counters_value[hartid][ctr_idx - num_hw_ctrs] = 0;
-		if (flags & SBI_PMU_CFG_FLAG_AUTO_START)
+		if (flags & SBI_PMU_CFG_FLAG_AUTO_START) {
+			if (SBI_PMU_FW_MAX <= event_code &&
+			    pmu_dev && pmu_dev->fw_counter_start) {
+				ret = pmu_dev->fw_counter_start(
+					ctr_idx - num_hw_ctrs, event_code,
+					fw_counters_value[hartid][ctr_idx - num_hw_ctrs],
+					true);
+				if (ret)
+					return ret;
+			}
 			fw_counters_started[hartid] |= BIT(ctr_idx - num_hw_ctrs);
+		}
 	}
 
 	return ctr_idx;
@@ -734,6 +787,19 @@ static void pmu_reset_event_map(u32 hartid)
 	fw_counters_started[hartid] = 0;
 }
 
+const struct sbi_pmu_device *sbi_pmu_get_device(void)
+{
+	return pmu_dev;
+}
+
+void sbi_pmu_set_device(const struct sbi_pmu_device *dev)
+{
+	if (!dev || pmu_dev)
+		return;
+
+	pmu_dev = dev;
+}
+
 void sbi_pmu_exit(struct sbi_scratch *scratch)
 {
 	u32 hartid = current_hartid();
-- 
2.34.1

[PATCH 6/7] lib: sbi: Print platform PMU device at boot-time

[Anup Patel]

Let us print the platform PMU device name at the boot-time so that users
know whether the underlying platform has custom per-HART PMU operations.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 lib/sbi/sbi_init.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
index d57efa7..a8500e5 100644
--- a/lib/sbi/sbi_init.c
+++ b/lib/sbi/sbi_init.c
@@ -63,6 +63,7 @@ static void sbi_boot_print_banner(struct sbi_scratch *scratch)
 static void sbi_boot_print_general(struct sbi_scratch *scratch)
 {
 	char str[128];
+	const struct sbi_pmu_device *pdev;
 	const struct sbi_hsm_device *hdev;
 	const struct sbi_ipi_device *idev;
 	const struct sbi_timer_device *tdev;
@@ -93,6 +94,9 @@ static void sbi_boot_print_general(struct sbi_scratch *scratch)
 	hdev = sbi_hsm_get_device();
 	sbi_printf("Platform HSM Device       : %s\n",
 		   (hdev) ? hdev->name : "---");
+	pdev = sbi_pmu_get_device();
+	sbi_printf("Platform PMU Device       : %s\n",
+		   (pdev) ? pdev->name : "---");
 	srdev = sbi_system_reset_get_device(SBI_SRST_RESET_TYPE_COLD_REBOOT, 0);
 	sbi_printf("Platform Reboot Device    : %s\n",
 		   (srdev) ? srdev->name : "---");
-- 
2.34.1

[PATCH 7/7] include: sbi: Reduce includes in sbi_pmu.h

[Anup Patel]

The sbi_pmu.h should only include minimal required headers whereas
sbi_pmu.c should include all required headers.

Signed-off-by: Anup Patel <apatel@ventanamicro.com>
Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
---
 include/sbi/sbi_pmu.h | 5 ++---
 lib/sbi/sbi_pmu.c     | 2 ++
 lib/sbi/sbi_trap.c    | 1 +
 3 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/include/sbi/sbi_pmu.h b/include/sbi/sbi_pmu.h
index d787575..a2ce42d 100644
--- a/include/sbi/sbi_pmu.h
+++ b/include/sbi/sbi_pmu.h
@@ -11,9 +11,8 @@
 #define __SBI_PMU_H__
 
 #include <sbi/sbi_types.h>
-#include <sbi/sbi_hartmask.h>
-#include <sbi/sbi_scratch.h>
-#include <sbi/sbi_ecall_interface.h>
+
+struct sbi_scratch;
 
 /* Event related macros */
 /* Maximum number of hardware events that can mapped by OpenSBI */
diff --git a/lib/sbi/sbi_pmu.c b/lib/sbi/sbi_pmu.c
index 87e1ff8..214d5e8 100644
--- a/lib/sbi/sbi_pmu.c
+++ b/lib/sbi/sbi_pmu.c
@@ -10,7 +10,9 @@
 #include <sbi/riscv_asm.h>
 #include <sbi/sbi_bitops.h>
 #include <sbi/sbi_console.h>
+#include <sbi/sbi_ecall_interface.h>
 #include <sbi/sbi_hart.h>
+#include <sbi/sbi_hartmask.h>
 #include <sbi/sbi_platform.h>
 #include <sbi/sbi_pmu.h>
 #include <sbi/sbi_scratch.h>
diff --git a/lib/sbi/sbi_trap.c b/lib/sbi/sbi_trap.c
index 0e30946..4c1339e 100644
--- a/lib/sbi/sbi_trap.c
+++ b/lib/sbi/sbi_trap.c
@@ -9,6 +9,7 @@
 
 #include <sbi/riscv_asm.h>
 #include <sbi/riscv_encoding.h>
+#include <sbi/sbi_bitops.h>
 #include <sbi/sbi_console.h>
 #include <sbi/sbi_ecall.h>
 #include <sbi/sbi_error.h>
-- 
2.34.1

[PATCH 5/7] lib: sbi_pmu: Add custom PMU device operations

[Andrew Jones]

On Thu, Aug 25, 2022 at 10:21:42AM +0530, Anup Patel wrote:
> We extend SBI PMU implementation to allow custom PMU device operations
> which a platform can use for platform specific quirks.
> 
> The custom PMU device operations added by this patch include:
> 1) Operations to allow a platform implement custom firmware events.
>    These custom firmware events can be SBI vendor extension related
>    events or platform specific per-HART events are not possible to
>    count through HPM CSRs.
> 2) Operations to allow a platform implement custom way for enabling
>    (or disabling) an overflow interrupt (e.g. T-Head C9xx).
> 
> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
> ---
>  include/sbi/sbi_pmu.h |  57 ++++++++++++++++++++++
>  lib/sbi/sbi_pmu.c     | 108 ++++++++++++++++++++++++++++++++++--------
>  2 files changed, 144 insertions(+), 21 deletions(-)
>

Reviewed-by: Andrew Jones <ajones@ventanamicro.com>

[PATCH 6/7] lib: sbi: Print platform PMU device at boot-time

[Anup Patel]

On Sat, Aug 27, 2022 at 4:56 AM Atish Patra <atishp@atishpatra.org> wrote:
>
>
>
> On Wed, Aug 24, 2022 at 9:52 PM Anup Patel <apatel@ventanamicro.com> wrote:
>>
>> Let us print the platform PMU device name at the boot-time so that users
>> know whether the underlying platform has custom per-HART PMU operations.
>>
>> Signed-off-by: Anup Patel <apatel@ventanamicro.com>
>> Reviewed-by: Andrew Jones <ajones@ventanamicro.com>
>> ---
>>  lib/sbi/sbi_init.c | 4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/lib/sbi/sbi_init.c b/lib/sbi/sbi_init.c
>> index d57efa7..a8500e5 100644
>> --- a/lib/sbi/sbi_init.c
>> +++ b/lib/sbi/sbi_init.c
>> @@ -63,6 +63,7 @@ static void sbi_boot_print_banner(struct sbi_scratch *scratch)
>>  static void sbi_boot_print_general(struct sbi_scratch *scratch)
>>  {
>>         char str[128];
>> +       const struct sbi_pmu_device *pdev;
>>         const struct sbi_hsm_device *hdev;
>>         const struct sbi_ipi_device *idev;
>>         const struct sbi_timer_device *tdev;
>> @@ -93,6 +94,9 @@ static void sbi_boot_print_general(struct sbi_scratch *scratch)
>>         hdev = sbi_hsm_get_device();
>>         sbi_printf("Platform HSM Device       : %s\n",
>>                    (hdev) ? hdev->name : "---");
>> +       pdev = sbi_pmu_get_device();
>> +       sbi_printf("Platform PMU Device       : %s\n",
>> +                  (pdev) ? pdev->name : "---");
>
>
> Just a nit pick thought ;)
> Should we only print the entire line if pdev is present ?

In general, I agree with your suggestion but for consistency
let us keep it as-is. You can send a separate patch print
device for various things only if xdev != NULL.

>
>>
>>         srdev = sbi_system_reset_get_device(SBI_SRST_RESET_TYPE_COLD_REBOOT, 0);
>>         sbi_printf("Platform Reboot Device    : %s\n",
>>                    (srdev) ? srdev->name : "---");
>> --
>> 2.34.1
>>
>
> Otherwise, LGTM.
>
> Reviewed-by: Atish Patra <atishp@rivosinc.com>
>
> --
> Regards,
> Atish

Regards,
Anup

[PATCH 0/7] OpenSBI PMU improvements

[Anup Patel]

On Thu, Aug 25, 2022 at 10:22 AM Anup Patel <apatel@ventanamicro.com> wrote:
>
> This series aims to improve SBI PMU implementation in following ways:
> 1) Optimize memory usage by reducing global data to track per-HART firmware
>    counters (almost 80KB saved)
> 2) Allow platform to implement custom firmware counters and quirks related
>    to hardware counters (This will be useful for T-Head)
> 3) Few other fixes for better SBI spec compliance
>
> These patches can also be found in sbi_pmu_imp_v1 branch at:
> https://github.com/avpatel/opensbi.git
>
> Anup Patel (7):
>   lib: sbi_pmu: Remove "event_idx" member from struct sbi_pmu_fw_event
>   lib: sbi_pmu: Replace sbi_pmu_ctr_read() with sbi_pmu_ctr_fw_read()
>   lib: sbi_pmu: Firmware counters are always 64 bits wide
>   lib: sbi_pmu: Simplify FW counters to reduce memory usage
>   lib: sbi_pmu: Add custom PMU device operations
>   lib: sbi: Print platform PMU device at boot-time
>   include: sbi: Reduce includes in sbi_pmu.h

Applied this series to the riscv/opensbi repo.

Andrew (Drew) had already reviewed most of the patches
before it landed on OpenSBI mailing list so I have added
his Reviewed-by.

Thanks,
Anup

>
>  include/sbi/sbi_pmu.h   |  67 ++++++++++--
>  lib/sbi/sbi_ecall_pmu.c |   3 +-
>  lib/sbi/sbi_init.c      |   4 +
>  lib/sbi/sbi_pmu.c       | 218 +++++++++++++++++++++++-----------------
>  lib/sbi/sbi_trap.c      |   1 +
>  5 files changed, 190 insertions(+), 103 deletions(-)
>
> --
> 2.34.1
>