Re: [PATCH v2 2/3] powercap: qcom: Add SPEL powercap driver

[Konrad Dybcio <konrad.dybcio@oss.qualcomm.com>]

On 6/19/26 10:39 PM, Manaf Meethalavalappu Pallikunhi wrote:
> The Qualcomm SoC Power and Electrical Limits (SPEL) provides hardware
> based power monitoring and limiting capabilities for various power
> domains including System, SoC, CPU clusters, GPU, and various other
> subsystems.
> 
> The driver integrates with the Linux powercap framework, exposing SPEL
> capabilities through powercap sysfs interfaces.
> 
> Signed-off-by: Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>
> ---

[...]

> +#include <linux/bitfield.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/mod_devicetable.h>
> +#include <linux/mutex.h>
> +#include <linux/platform_device.h>
> +#include <linux/powercap.h>
> +#include <linux/slab.h>
> +#include <linux/types.h>

Please ensure all the includes are necessary

> +
> +/* SPEL register bitmasks */
> +#define ENERGY_STATUS_MASK		GENMASK(31, 0)
> +
> +#define POWER_LIMIT_MASK		GENMASK(14, 0)
> +#define POWER_LIMIT_ENABLE		BIT(31)
> +
> +#define TIME_WINDOW_MASK_L		GENMASK(14, 0)
> +#define TIME_WINDOW_MASK_H		GENMASK(22, 16)

Is BIT(15) not part of this?

[...]

> +/* Domain configuration */
> +static const struct spel_domain_info domain_info[SPEL_DOMAIN_MAX] = {
> +	[SPEL_DOMAIN_SYS]	= { "sys", 0x40 },
> +	[SPEL_DOMAIN_SOC]	= { "soc", 0x00 },
> +	[SPEL_DOMAIN_CL0]	= { "cl0", 0x5c },
> +	[SPEL_DOMAIN_CL1]	= { "cl1", 0x60 },
> +	[SPEL_DOMAIN_CL2]	= { "cl2", 0x64 },
> +	[SPEL_DOMAIN_IGPU]	= { "igpu", 0x08 },
> +	[SPEL_DOMAIN_DGPU]	= { "dgpu", 0x44 },
> +	[SPEL_DOMAIN_NSP]	= { "nsp", 0x0c },
> +	[SPEL_DOMAIN_MMCX]	= { "mmcx", 0x10 },
> +	[SPEL_DOMAIN_INFRA]	= { "infra", 0x18 },
> +	[SPEL_DOMAIN_DRAM]	= { "dram", 0x1c },
> +	[SPEL_DOMAIN_MDM]	= { "mdm", 0x48 },
> +	[SPEL_DOMAIN_WLAN]	= { "wlan", 0x4c },
> +	[SPEL_DOMAIN_USB1]	= { "usb1", 0x50 },
> +	[SPEL_DOMAIN_USB2]	= { "usb2", 0x54 },
> +	[SPEL_DOMAIN_USB3]	= { "usb3", 0x58 },
> +};

I would expect that the names are going to stay common, but the offsets
will be different. This array should probably be called
glymur_domain_info[]. We may have another LUT just for names of indices
(i.e. [SPEL_DOMAIN_xxx] = "xxx")

> +
> +/**
> + * struct spel_constraint_info - Power limit constraint information
> + * @limit_offset:	Register offset for power limit value
> + * @time_window_offset:	Register offset for time window
> + * @supported_mask:	Bit mask in capability register
> + * @domain_id:		Domain this constraint applies to
> + * @pl_id:		Power limit ID (PL1, PL2, etc.)
> + */
> +struct spel_constraint_info {
> +	u32 limit_offset;
> +	u32 time_window_offset;
> +	u32 supported_mask;
> +	enum spel_domain_type domain_id;
> +	int pl_id;
> +};
> +
> +/* Constraint configuration */
> +static const struct spel_constraint_info constraints[] = {
> +	/* SYS domain constraints */
> +	{ 0x10, 0x70, BIT(0), SPEL_DOMAIN_SYS, POWER_LIMIT1 },
> +	{ 0x14, 0x74, BIT(1), SPEL_DOMAIN_SYS, POWER_LIMIT2 },
> +	{ 0x18, 0x78, BIT(2), SPEL_DOMAIN_SYS, POWER_LIMIT3 },
> +	{ 0x1c, 0x7c, BIT(3), SPEL_DOMAIN_SYS, POWER_LIMIT4 },
> +	/* SoC domain constraints */
> +	{ 0x00, 0x60, BIT(4), SPEL_DOMAIN_SOC, POWER_LIMIT1 },
> +	{ 0x04, 0x64, BIT(5), SPEL_DOMAIN_SOC, POWER_LIMIT2 },
> +	{ 0x08, 0x68, BIT(6), SPEL_DOMAIN_SOC, POWER_LIMIT3 },
> +	{ 0x0c, 0x6c, BIT(7), SPEL_DOMAIN_SOC, POWER_LIMIT4 },
> +};

Is this specific to Glymur, or SPEL-wide?

[...]

> +/**
> + * struct spel_system -	SPEL system

odd tab after '-'

[...]

> +	case PL_LIMIT:
> +		new_val = spel_unit_xlate(sd, POWER_UNIT, value, 1);
> +		if (new_val > FIELD_MAX(POWER_LIMIT_MASK))
> +			return -EINVAL;
> +		reg_val = (reg_val & ~POWER_LIMIT_MASK) | FIELD_PREP(POWER_LIMIT_MASK, new_val);

FIELD_MODIFY()

> +
> +		/*
> +		 * Enable/Disable PL based on the value:
> +		 * - If value is 0, disable the PL (clear enable bit)
> +		 * - If value is non-zero, enable the PL (set enable bit)
> +		 */
> +		if (new_val == 0)
> +			reg_val &= ~POWER_LIMIT_ENABLE;
> +		else
> +			reg_val |= POWER_LIMIT_ENABLE;

Likewise


> +
> +		writel(reg_val, reg_addr);
> +		return 0;
> +
> +	case PL_TIME_WINDOW:
> +		/*
> +		 * Encode time window: upper 7 bits to [22:16], lower 15 bits to [14:0]
> +		 */
> +		new_val = spel_unit_xlate(sd, TIME_UNIT, value, 1);
> +		if (new_val > TIME_WINDOW_MAX)
> +			return -EINVAL;
> +		/* Read-modify-write to preserve other bits */
> +		reg_val = (reg_val & ~(TIME_WINDOW_MASK_H | TIME_WINDOW_MASK_L)) |
> +			  FIELD_PREP(TIME_WINDOW_MASK_H, new_val >> 15) |
> +			  FIELD_PREP(TIME_WINDOW_MASK_L, new_val);

Also here

[...]

> +static void spel_detect_powerlimit(struct spel_domain *sd)
> +{
> +	struct spel_system *sp = sd->sp;
> +	u32 capabilities;
> +	int i, j;
> +
> +	capabilities = readl(sp->config_base + LIMITS_CAPABILITY_OFFSET);
> +
> +	/*
> +	 * Detect power limits from hardware capabilities.
> +	 * Start from index 1 (POWER_LIMIT2) since PL1 is always enabled in spel_init_domains().
> +	 */
> +	for (i = 1; i < ARRAY_SIZE(pl_names); i++) {

int i = POWER_LIMIT2

(yeah, nowadays you can finally declare the iterator inside the loop
in the kernel)

Konrad