cppc: advertise optional fast-channel doorbell (follow-up to autonomous mode) by icosta-sifive · Pull Request #119 · riscv-software-src/librpmi

icosta-sifive · 2026-08-31T08:32:25Z

This is a follow-up to the previous PR that added CPPC autonomous mode. While wiring up GET_FAST_CHANNEL_REGION for autonomous mode, one commit was forgotten. The optional fast-channel doorbell that the same service advertise, this PR fills that gap.

Until now librpmi hardcoded these fields to zero, so even with the rest of the fast-channel path in place a platform had no way to advertise a doorbell.

The doorbell lets the application processor signal the platform microcontroller
after updating its Perf Request fast-channel, instead of relying on polling. It
is described by:

librpmi was still hardcoding these fields to zero, so a platform could never advertise a doorbell even though the rest of the fast-channel path was in place.

Tests

Added test coverage for:

Doorbell advertisement in passive mode (32-bit doorbell)
Doorbell advertisement in autonomous mode (combined FLAGS[4:3] + doorbell bits)
S-mode access to GET_FAST_CHANNEL_REGION

icosta-sifive · 2026-08-31T08:34:57Z

@vlsunil FYI

saini-ranbirs · 2026-09-21T05:38:09Z

+		resp[7] = 0;
+		/* doorbell write value */
+		resp[8] = 0;
+	}


The comments for resp[6/7/8] are visible in close proximity through the if part, so else part even without comments wrt resp[6/7/8] and the resp[6] = resp[7]= resp[8] = 0; assignment should be good enough.

@saini-ranbirs I initially combined these assignments for conciseness, but reverted to separate assignments after running the full patch through Linux checkpatch.pl --strict, which flags chained assignments with MULTIPLE_ASSIGNMENTS. The behavior is unchanged; this is only to follow the project’s Linux-style formatting and keep the patch checkpatch-clean.

That's fine. However, the else part is for DB not supported. So, see if those comments there really make sense.

Agreed. Since the else path means the doorbell is not supported, the individual doorbell field comments are unnecessary there. I’ve removed them and kept the assignments separate to avoid the checkpatch MULTIPLE_ASSIGNMENTS warning.

As per spec, the doorbell details are unspecified and considered invalid if the Performance Request fast-channel
doorbell (FLAGS[0] = 0) is not supported and must not be used.

Hence, it seems safe even if else part assignments are totally removed. One more option is to set the resp_dlen appropriately based on DB supported or not supported as 9 or 6 times .. respectively.

Although those fields must not be used when the doorbell is unsupported, librpmi reuses the ACK buffer, so leaving words 6–8 unwritten could expose stale data. Also, the spec defines words 0–8 as the response layout, so I’d prefer to keep the fixed 9-word response and explicitly zero the unused fields.

saini-ranbirs · 2026-09-22T05:14:19Z


+	/* FLAGS[2:1] doorbell width, FLAGS[0] doorbell support (optional) */
+	if (db->flags & RPMI_CPPC_FST_CHN_DB_SUPP) {
+		flags |= RPMI_CPPC_FST_CHN_DB_SUPP | (db->flags & (0x3U << 1));


See if it's better to define (0x3U << 1) as a macro - say as RPMI_CPPC_FST_CHN_DB_REG_WIDTH_MASK?

and may I know why flags |= db->flags; is not sufficient here, I mean what else db->flags contain?

Added RPMI_CPPC_FST_CHN_DB_REG_WIDTH_MASK. Although flags |= db->flags works when the platform provides a valid value, rpmi_cppc_fastchan_doorbell.flags is an unrestricted 32-bit field. Explicitly copying only bits [2:0] ensures invalid platform data cannot corrupt the independently generated mode bits [4:3] or set reserved bits [31:5], which the RPMI specification requires to be zero. I think keeping the explicit mask is safer, but please let me know what you think. I can switch to flags |= db->flags if you prefer the simpler approach.

So, does that mean there is no validation of db->flags when the platform provides an invalid value?

Correct, there is currently no explicit validation of db->flags; the mask only prevents invalid bits from reaching the wire. On reconsideration, validating the platform data during rpmi_service_group_cppc_create() would be cleaner. Once validated, flags |= db->flags would be sufficient. I can update the implementation accordingly if you agree ?

To me, validating the platform data during rpmi_service_group_cppc_create() is better. It should allow group creation with valid set of values only.

Agreed, that makes sense. I’ve updated rpmi_service_group_cppc_create() to reject invalid doorbell flags during group creation. With that validation in place, the response path now simply uses flags |= db->flags. I also added tests for the invalid cases.

saini-ranbirs · 2026-09-22T05:54:11Z


-	/* FLAGS[4:3]: 0b00 = normal/passive, 0b01 = autonomous. No doorbell. */
+	/* FLAGS[4:3]: 0b00 = normal/passive, 0b01 = autonomous. */
 	flags = (cppcgrp->cppc_mode == RPMI_CPPC_AUTO_MODE) ? (0x01U << 3) : 0;


This of my review comment is out of context, but still can you analyze what does it take to just set flags = cppcgrp->cppc_mode; here?

Thought: RPMI_CPPC_AUTO_MODE can be defined bitwise and not enum like

#define RPMI_CPPC_AUTO_MODE (0x01U << 3)

If it seems worth, then this can be taken up separately.

I analyzed this and the current implementation is spec-compliant. I would prefer to keep enum rpmi_cppc_mode as the semantic API value and encode it into FLAGS only when building the response. Defining RPMI_CPPC_AUTO_MODE as (0x01U << 3) would couple the public mode value to this specific wire layout and change its value from 1 to 8. We could introduce a named macro for the FLAGS encoding as a separate cleanup, if useful.

I think macros should be used wrt FLAGS[4:3] instead of using (0x01U << 3) : 0 so that code can be self-descriptive.

vlsunil · 2026-09-22T14:10:05Z

+#define RPMI_CPPC_FST_CHN_MODE_AUTONOMOUS	(1U << 3)
+
+#define RPMI_CPPC_FST_CHN_DB_REG_WIDTH_MASK	(0x3U << 1)
+#define RPMI_CPPC_FST_CHN_DB_REG_08_BITS	(0U << 1)


Can you be consistent to add 0x for all macros?

Isn't it better to define BIT and GENMASK?

Updated the new macros to use hexadecimal values consistently. I kept the explicit shifts rather than BIT()/GENMASK() because those helpers are not currently provided by librpmi and this is a public header. Introducing generic helper macros here could conflict with the integrating firmware, while the explicit expressions match the existing header style.

Thats why we have librpmi_internal.h header where such macros can be used

Lets add RPMI_BIT() and RPMI_GENMASK() in librpmi_internal.h header.

vlsunil · 2026-09-22T14:21:57Z

+
+/** CPPC Perf Request fast-channel doorbell */
+struct rpmi_cppc_fastchan_doorbell {
+	/** doorbell flags */


It is not clear to me why Flags is made part of doorbell structure. While Flags has doorbell related info, it has other info as well, right?

This flags member contains only the doorbell-related fields: support in bit 0 and register width in bits [2:1]. It does not contain the complete response FLAGS value; CPPC mode in bits [4:3] is obtained separately from cppc_mode and combined when building the response. I’ve updated the member comment to clarify this.

This structure is platform firmware facing and "flags" is purely a RPMI message construct. Since its a structure we can add separate variables instead of making them bits in a abstract flags named variable.

struct rpmi_cppc_fastcnan_doorbell {
rpmi_bool_t db_supported; /* was FLAGS[0] /
rpmi_uint32_t perf_req_fastchan_width; / 8, 16, or 32 in FLAGS[2:1] */
...
}

Agreed, that’s a better approach. I replaced the flags field with separate db_supported and perf_req_fastchan_width fields in struct rpmi_cppc_fastchan_doorbell, and now convert them to the RPMI FLAGS bits internally when building the response.

vlsunil · 2026-09-22T14:27:23Z

+						      void *data,
+						      rpmi_uint16_t max_data_len)
+{
+	rpmi_uint32_t *exp = data;


I believe librpmi follows linux coding standards/formatting. Please check.

Good point. I reformatted the new test functions and ran the complete patch through Linux checkpatch.pl --strict; it now passes with 0 errors, 0 warnings, and 0 checks.

vlsunil · 2026-09-22T15:01:38Z

 	/* transition latency */
 	rpmi_uint32_t transition_latency;
+	/* Perf Request fast-channel doorbell (optional) */
+	struct rpmi_cppc_fastchan_doorbell doorbell;


Again, can you explain why it is added here?

I added it here because cppc_regs is the existing container for static CPPC platform data retained by the service group. The doorbell configuration has the same lifetime, and embedding it here avoids changing the rpmi_service_group_cppc_create() signature. I agree that the rpmi_cppc_regs name makes this less obvious. If you prefer stricter separation, I can pass the doorbell descriptor separately, but that would require changing the public create API.

I personally don't prefer to add fast channel related information here. Can it be added under rpmi_cppc_fastchan structure since this is fast channel specific information? Would that change the API?

Yes, that would require an API change because struct rpmi_cppc_fastchan is private and created internally, so the platform currently has no way to populate the doorbell information there. We would need to pass a doorbell descriptor, or a new public fast-channel configuration structure, to rpmi_service_group_cppc_create(), then store it in the private fast-channel context. I agree that this would provide cleaner ownership. If changing the public API is acceptable, I can rework it that way.

I agree with @vlsunil here, I prefer passing an optional const struct rpmi_cppc_fastchan_doorbell * to rpmi_service_group_cppc_create()

@slingappa @vlsunil In that case i will proceed with this changes even knowing that brakes API compatibility here

@slingappa @vlsunil As agreed, i’ve changed the API to pass an optional const struct rpmi_cppc_fastchan_doorbell * to rpmi_service_group_cppc_create(). The descriptor is now stored in the private fast-channel context instead of rpmi_cppc_regs; NULL represents no doorbell. All call sites and tests have been updated.

slingappa · 2026-09-23T04:45:48Z

 		return NULL;
 	}

+	if (cppc_regs->doorbell.flags &


The creation-time validation rejects reserved bits and width encoding 3, but it still accepts a nonzero width with RPMI_CPPC_FST_CHN_DB_SUPP clear (for example, RPMI_CPPC_FST_CHN_DB_REG_32_BITS). The current response path drops those width bits, so this is not a wire-level defect today, but it leaves the public descriptor semantically ambiguous. Could we reject nonzero width when support is clear, or add a test/documentation showing that the specification explicitly permits those bits to be ignored?

Good catch. I’ve updated the creation-time validation to reject a nonzero doorbell width when RPMI_CPPC_FST_CHN_DB_SUPP is clear, and added a negative test for this case. This keeps the public descriptor unambiguous: when the doorbell is unsupported, its flags must be zero.

slingappa · 2026-09-23T04:45:50Z

+	.lowest_freq = 1000000000U,
+	.nominal_freq = 2000000000U,
+	.transition_latency = 10,
+	.doorbell = {


Both new positive scenarios use only RPMI_CPPC_FST_CHN_DB_REG_32_BITS, but the public API defines three legal doorbell widths (8, 16, and 32 bits), and the implementation validates the width field. Could we add cases for the 8-bit and 16-bit encodings as well—ideally table-driven—while keeping the reserved-width rejection test? Otherwise a regression in FLAGS[2:1] for either legal variant would leave the suite green.

Good point. I’ve added table-driven positive tests for the 8-, 16-, and 32-bit doorbell width encodings, while retaining the reserved-width rejection test. This now covers every legal value of FLAGS[2:1].

saini-ranbirs · 2026-09-24T05:48:39Z

+	}
+
+	if (!(cppc_regs->doorbell.flags & RPMI_CPPC_FST_CHN_DB_SUPP) &&
+	    (cppc_regs->doorbell.flags & RPMI_CPPC_FST_CHN_DB_REG_WIDTH_MASK)) {


Shouldn't this validation be done before the cppc fastchannel doorbell width validation?

Agreed. Checking for a width without doorbell support first makes the validation flow clearer, so I’ve reordered the checks accordingly.

pathakraul · 2026-09-25T19:55:22Z

 		return NULL;
 	}

+	if (doorbell && !(doorbell->flags & RPMI_CPPC_FST_CHN_DB_SUPP)) {


This logic of validation and below will improve once you add db_supported and perf_req_fastchan_width fields in rpmi_cppc_fastchan_doorbell structure.

True, the validation is much clearer now. It checks db_supported directly and validates perf_req_fastchan_width as 8, 16, or 32, instead of decoding message flag bits at group creation

The CPPC GET_FAST_CHANNEL_REGION service defines an optional doorbell (FLAGS[0] support, FLAGS[2:1] width, and the doorbell address plus write value in response words 6-8) that lets the application processor signal the platform microcontroller after updating its Perf Request fast-channel, instead of relying on polling. librpmi previously hardcoded these fields to zero, so a platform could never advertise a doorbell. Add an optional doorbell descriptor (struct rpmi_cppc_fastchan_doorbell) embedded in struct rpmi_cppc_regs, mirroring how the performance service group carries its doorbell in static platform data. When RPMI_CPPC_FST_CHN_DB_SUPP is set, GET_FAST_CHANNEL_REGION reports the doorbell; otherwise the response is unchanged. The create() signature is not modified. librpmi only advertises the doorbell; raising the doorbell interrupt and invoking rpmi_context_process_group_events() from the handler remains a platform responsibility. Add tests for doorbell advertisement in passive and autonomous mode, and S-mode access to GET_FAST_CHANNEL_REGION. Signed-off-by: Ivo Costa <ivo.costa@sifive.com>

icosta-sifive marked this pull request as ready for review August 31, 2026 08:35

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch 2 times, most recently from cebb0da to c66846d Compare August 31, 2026 18:00

saini-ranbirs reviewed Sep 21, 2026

View reviewed changes

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from c66846d to f85ff66 Compare September 21, 2026 08:16

icosta-sifive requested a review from saini-ranbirs September 21, 2026 08:16

saini-ranbirs reviewed Sep 22, 2026

View reviewed changes

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from f85ff66 to bf4e7a7 Compare September 22, 2026 07:31

icosta-sifive requested a review from saini-ranbirs September 22, 2026 07:32

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from bf4e7a7 to abf6e0e Compare September 22, 2026 08:05

vlsunil reviewed Sep 22, 2026

View reviewed changes

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from abf6e0e to 807c178 Compare September 22, 2026 16:53

icosta-sifive requested a review from vlsunil September 22, 2026 16:54

slingappa reviewed Sep 23, 2026

View reviewed changes

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from 807c178 to 9b2df86 Compare September 23, 2026 07:39

icosta-sifive requested a review from slingappa September 23, 2026 07:41

saini-ranbirs reviewed Sep 24, 2026

View reviewed changes

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from 9b2df86 to 11647fd Compare September 24, 2026 13:04

icosta-sifive requested a review from saini-ranbirs September 24, 2026 13:05

pathakraul reviewed Sep 25, 2026

View reviewed changes

icosta-sifive force-pushed the dev/icosta/cppc_doorbell branch from 11647fd to d8a7a1c Compare September 26, 2026 07:08

icosta-sifive requested a review from pathakraul September 26, 2026 07:08

Conversation

icosta-sifive commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Tests

Uh oh!

icosta-sifive commented Aug 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saini-ranbirs Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saini-ranbirs Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icosta-sifive Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saini-ranbirs Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

icosta-sifive Sep 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

icosta-sifive commented Aug 31, 2026 •

edited

Loading

saini-ranbirs Sep 23, 2026 •

edited

Loading

saini-ranbirs Sep 22, 2026 •

edited

Loading

icosta-sifive Sep 22, 2026 •

edited

Loading

saini-ranbirs Sep 22, 2026 •

edited

Loading

icosta-sifive Sep 23, 2026 •

edited

Loading

pathakraul Sep 25, 2026 •

edited

Loading