Re: [PATCH v11 1/5] riscv_cbqri: Add capacity controller core

["Radim Krčmář" <radim.krcmar@oss.qualcomm.com>]

2026-11-08T15:20:50-08:00, Drew Fustini <fustini@kernel.org>:
> Add the core for RISC-V CBQRI capacity controllers: the register
> layout, the capability probe, and the allocation operations that
> program a capacity block mask for an RCID and read it back.
>
> A discovery layer maps the register block and describes the controller
> in a cbqri_controller_info descriptor. riscv_cbqri_register_cc() probes
> the controller from that descriptor and adds it to the controller list,
> which stays private to the core. The core owns the controller state
> after registration, so nothing in it depends on how the controller was
> discovered. The caller keeps the register mapping alive.
>
> Co-developed-by: Adrien Ricciardi <aricciardi@baylibre.com>
> Signed-off-by: Adrien Ricciardi <aricciardi@baylibre.com>
> Signed-off-by: Drew Fustini <fustini@kernel.org>
> ---
> diff --git a/drivers/resctrl/cbqri_devices.c b/drivers/resctrl/cbqri_devices.c
> +static u64 cbqri_readq(void __iomem *addr)
> +{
> +	return lo_hi_readq(addr);
> +}
> +
> +static void cbqri_clear_cunits(struct cbqri_controller *ctrl)
> +{
> +	if (ctrl->cc.cunits) {
> +		writel(0, ctrl->base + CBQRI_CC_CUNITS_OFF);
> +		writel(0, ctrl->base + CBQRI_CC_CUNITS_OFF + 4);

nit: lo_hi_writeq?

> +int cbqri_apply_cache_config(struct cbqri_controller *ctrl, u32 closid,
> +			     const struct cbqri_cc_config *cfg)
> +{
> [...]
> +	mutex_lock(&ctrl->lock);
> [...]
> +out:
> +	mutex_unlock(&ctrl->lock);
> +	return err;

nit: guard the mutex?

> +static int cbqri_probe_feature(struct cbqri_controller *ctrl, int reg_offset,
> +			       int operation, int *status, bool *access_type_supported)
> +{
> +	const u64 active_mask = CBQRI_CONTROL_REGISTERS_OP_MASK |
> +				CBQRI_CONTROL_REGISTERS_AT_MASK |
> +				CBQRI_CONTROL_REGISTERS_RCID_MASK;
> +	u64 reg, saved_reg;
> +	int at;
> +
> +	/*
> +	 * Default the output to false so the status==0 (feature not
> +	 * implemented) path returns a deterministic value to the caller
> +	 * rather than leaving an uninitialized bool.
> +	 */
> +	*access_type_supported = false;
> +
> +	/* Keep the initial register value to preserve the WPRI fields */
> +	reg = cbqri_readq(ctrl->base + reg_offset);
> +	saved_reg = reg;
> +
> +	/* Drain any in-flight firmware op before issuing our own write. */
> +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &saved_reg) < 0) {
> +		pr_err("BUSY timeout before probe operation\n");
> +		return -EIO;
> +	}
> +
> +	/*
> +	 * Execute the requested operation with all active fields
> +	 * (OP/AT/RCID) zeroed except OP itself. Every bit not in
> +	 * active_mask is WPRI and gets carried over from saved_reg.
> +	 */
> +	reg = (saved_reg & ~active_mask) |
> +	      FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation);
> +	writel(lower_32_bits(reg), ctrl->base + reg_offset);
> +	if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> +		pr_err_ratelimited("BUSY timeout during operation\n");
> +		return -EIO;
> +	}
> +
> +	/* Get the operation status */
> +	*status = FIELD_GET(CBQRI_CONTROL_REGISTERS_STATUS_MASK, reg);
> +
> +	/*
> +	 * Check for AT support if the register is implemented
> +	 * (if not, the status value will remain 0)
> +	 */
> +	if (*status != 0) {
> +		/*
> +		 * Re-issue operation with AT=CODE so the controller
> +		 * latches AT=CODE on supported hardware (or resets it to 0
> +		 * on hardware that doesn't). OP must be a defined CBQRI op
> +		 * here. OP=0 is a no-op and would silently disable CDP.

OP=0 is a reserved for future standard use.
(Hopefully just fails, but it could be worse if it gets implemented as
 something other than no-op.)

> +		 */
> +		reg = (saved_reg & ~active_mask) |
> +		      FIELD_PREP(CBQRI_CONTROL_REGISTERS_OP_MASK, operation) |
> +		      FIELD_PREP(CBQRI_CONTROL_REGISTERS_AT_MASK,
> +				 CBQRI_CONTROL_REGISTERS_AT_CODE);
> +		writel(lower_32_bits(reg), ctrl->base + reg_offset);
> +		if (cbqri_wait_busy_flag(ctrl, reg_offset, &reg) < 0) {
> +			pr_err("BUSY timeout setting AT field\n");
> +			return -EIO;
> +		}
> +
> +		/*
> +		 * If the AT field value has been reset to zero,
> +		 * then AT support is not present
> +		 */
> +		at = FIELD_GET(CBQRI_CONTROL_REGISTERS_AT_MASK, reg);

I think it would be safer to also check the status field.

I'm not too clear whether the spec actually allows returning 5 (invalid
AT) when the AT is invalid, but an implementation that does that
wouldn't surprise me.

(For example when an implementation supports data AT and a custom AT, so
code AT isn't supported, but AT in general is -- it could keep the
failing code AT and just report the error.)

> +		if (at == CBQRI_CONTROL_REGISTERS_AT_CODE)
> +			*access_type_supported = true;
> +	}
> +
> +	/*
> +	 * Restore the original register value.
> +	 * Clear OP to avoid re-triggering the probe op.
> +	 */
> +	saved_reg &= ~CBQRI_CONTROL_REGISTERS_OP_MASK;
> +	writel(lower_32_bits(saved_reg), ctrl->base + reg_offset);

It would be better to do nothing than trigger an illegal OP.
All our writes should modify the non-WPRI bits anyway, so restoring them
does nothing.

> +	if (cbqri_wait_busy_flag(ctrl, reg_offset, NULL) < 0) {
> +		pr_err("BUSY timeout restoring register value\n");
> +		return -EIO;
> +	}
> +
> +	return 0;
> +}
> +
> +static int cbqri_probe_cc(struct cbqri_controller *ctrl)
> +{
> [...]
> +
> +	/*
> +	 * The driver programs the v1.0 register layout. Controllers that
> +	 * report 0.x differ only in capability bits that read as zero, so
> +	 * they are accepted. Anything newer than 1.0 may move fields.
> +	 */
> +	if (ver_major > 1 || (ver_major == 1 && ver_minor > 0)) {

I think we could accept ver_major == 1 even with ver_minor > 0 since the
minor specifications shouldn't be doing backward-incompatible changes.

(If we fix the OP=0, we shouldn't be invoking any other behavior that
 might change.)

I'd be more careful about accepting anything pre-1.0, since I would
expect there was a breaking change in the spec somewhere, and we could
program it incorrectly...  Do we even care about pre-1.0 hardware?

> +		pr_warn("CC at %pa reports CBQRI version %d.%d, skipping\n",
> +			&ctrl->addr, ver_major, ver_minor);
> +		return -ENODEV;
> +	}

Thanks.