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

[Manaf Meethalavalappu Pallikunhi <manaf.pallikunhi@oss.qualcomm.com>]

Hi Konrad,


On 6/22/2026 4:31 PM, Konrad Dybcio wrote:
> 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

I already checked it and removed unused headers

> 
>> +
>> +/* 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

Names also can be different here. For example, hawi, It has only subset 
of these domain and it doesn't have dgpu, it has only "gpu". cpu domain
names also different there.

> will be different. This array should probably be called
> glymur_domain_info[]. We may have another LUT just for names of indices

ACK for glymur_domain_info.
> (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?

So far, current targets share common spec and offsets for constraints.

> 
> [...]
> 
>> +/**
>> + * struct spel_system -	SPEL system
> 
> odd tab after '-'

ACK

> 
> [...]
> 
>> +	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()

ACK

> 
>> +
>> +		/*
>> +		 * 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

ACK

> 
> 
>> +
>> +		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

ACK

> 
> [...]
> 
>> +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)

ACK for declaring the iterator inside loop, but assigning to 
POWER_LIMIT2 is removed in this revision based on V1 comment.

Thanks,
Manaf

> 
> Konrad