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

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

Hi Daniel,


On 5/27/2026 12:06 AM, Daniel Lezcano wrote:
> On 5/19/26 12:49, 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>
>> ---
>>   MAINTAINERS                  |   1 +
>>   drivers/powercap/Kconfig     |  13 +
>>   drivers/powercap/Makefile    |   1 +
>>   drivers/powercap/qcom_spel.c | 787 +++++++++++++++++++++++++++++++++ 
>> ++++++++++
>>   4 files changed, 802 insertions(+)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index c63f147e8c54..5c7542754ab6 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -22238,6 +22238,7 @@ M:    Manaf Meethalavalappu Pallikunhi 
>> <manaf.pallikunhi@oss.qualcomm.com>
>>   L:    linux-arm-msm@vger.kernel.org
>>   S:    Maintained
>>   F:    Documentation/devicetree/bindings/power/limits/qcom,spel.yaml
>> +F:    drivers/powercap/qcom_spel.c
>>   QUALCOMM PPE DRIVER
>>   M:    Luo Jie <quic_luoj@quicinc.com>
>> diff --git a/drivers/powercap/Kconfig b/drivers/powercap/Kconfig
>> index 03c4c796d993..e3a47c653499 100644
>> --- a/drivers/powercap/Kconfig
>> +++ b/drivers/powercap/Kconfig
>> @@ -93,4 +93,17 @@ config DTPM_DEVFREQ
>>       help
>>         This enables support for device power limitation based on
>>         energy model.
>> +
>> +config QCOM_SPEL
>> +    tristate "Qualcomm SPEL Powercap driver"
>> +    depends on ARM64 || COMPILE_TEST
>> +    help
>> +      This enables support for the Qualcomm SoC Power and Electrical
>> +      Limits (SPEL) hardware, which allows power limits to be
>> +      enforced and monitored on Qualcomm SoCs.
>> +
>> +      SPEL provides energy monitoring and power capping for multiple
>> +      domains including system, SoC, CPU clusters, GPU, and various
>> +      other subsystems.
>> +
>>   endif
>> diff --git a/drivers/powercap/Makefile b/drivers/powercap/Makefile
>> index 5ab0dce565b9..8235fb9d3df6 100644
>> --- a/drivers/powercap/Makefile
>> +++ b/drivers/powercap/Makefile
>> @@ -8,3 +8,4 @@ obj-$(CONFIG_INTEL_RAPL) += intel_rapl_msr.o
>>   obj-$(CONFIG_INTEL_RAPL_TPMI) += intel_rapl_tpmi.o
>>   obj-$(CONFIG_IDLE_INJECT) += idle_inject.o
>>   obj-$(CONFIG_ARM_SCMI_POWERCAP) += arm_scmi_powercap.o
>> +obj-$(CONFIG_QCOM_SPEL) += qcom_spel.o
>> diff --git a/drivers/powercap/qcom_spel.c b/drivers/powercap/qcom_spel.c
>> new file mode 100644
>> index 000000000000..fed5647959a5
>> --- /dev/null
>> +++ b/drivers/powercap/qcom_spel.c
>> @@ -0,0 +1,787 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Qualcomm SPEL (SoC Power and Electrical Limits) Driver
>> + *
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/bitmap.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>
>> +
>> +/* SPEL register bitmasks */
>> +#define ENERGY_STATUS_MASK        0xFFFFFFFF
>> +
>> +#define POWER_LIMIT_MASK        0x00007FFF
>> +#define POWER_LIMIT_ENABLE        BIT(31)
>> +
>> +#define TIME_WINDOW_MASK_L        0x00007FFF    /* bits [14:0] */
>> +#define TIME_WINDOW_MASK_H        0x007F0000    /* bits [22:16] */
>> +
>> +#define ENERGY_UNIT_OFFSET        16
>> +#define ENERGY_UNIT_MASK        0xF0000
>> +
>> +#define TIME_UNIT_OFFSET        8
>> +#define TIME_UNIT_MASK            0xF00
>> +
>> +#define POWER_UNIT_OFFSET        0
>> +#define POWER_UNIT_MASK            0x7
>> +
>> +#define LIMITS_CAPABILITY_OFFSET    0x20
>> +#define ENERGY_RPT_UNIT_OFFSET        0x04
>> +
>> +#define ENERGY_UNIT_SCALE        1000
>> +
>> +#define SPEL_DOMAIN_NAME_LENGTH        16
>> +
>> +/* Domain types */
>> +enum spel_domain_type {
>> +    SPEL_DOMAIN_SYS,
>> +    SPEL_DOMAIN_SOC,
>> +    SPEL_DOMAIN_CL0,
>> +    SPEL_DOMAIN_CL1,
>> +    SPEL_DOMAIN_CL2,
>> +    SPEL_DOMAIN_IGPU,
>> +    SPEL_DOMAIN_DGPU,
>> +    SPEL_DOMAIN_NSP,
>> +    SPEL_DOMAIN_MMCX,
>> +    SPEL_DOMAIN_INFRA,
>> +    SPEL_DOMAIN_DRAM,
>> +    SPEL_DOMAIN_MDM,
>> +    SPEL_DOMAIN_WLAN,
>> +    SPEL_DOMAIN_USB1,
>> +    SPEL_DOMAIN_USB2,
>> +    SPEL_DOMAIN_USB3,
>> +    SPEL_DOMAIN_MAX,
>> +};
>> +
>> +/* Power limit IDs */
>> +enum spel_power_limit_id {
>> +    POWER_LIMIT1,
>> +    POWER_LIMIT2,
>> +    POWER_LIMIT3,
>> +    POWER_LIMIT4,
>> +    NR_POWER_LIMITS,
>> +};
>> +
>> +/* Unit types for conversion */
>> +enum unit_type {
>> +    POWER_UNIT,
>> +    ENERGY_UNIT,
>> +    TIME_UNIT,
>> +};
>> +
>> +/* Power limit operation types */
>> +enum pl_ops_type {
>> +    PL_LIMIT,
>> +    PL_TIME_WINDOW,
>> +};
>> +
>> +static const char *pl_names[NR_POWER_LIMITS] = {
>> +    [POWER_LIMIT1] = "pl1",
>> +    [POWER_LIMIT2] = "pl2",
>> +    [POWER_LIMIT3] = "pl3",
>> +    [POWER_LIMIT4] = "pl4",
>> +};
> 
> Do you want to use NR_POWER_LIMITS instead of ARRAY_SIZE() ?

ACK, I will move to ARRAY_SIZE>
> Please unify the naming NR_POWER_LIMITS -> POWER_LIMITS_MAX

ACK

> 
>> +static const char *const spel_domain_names[] = {
>> +    "sys", "soc", "cl0", "cl1", "cl2", "igpu", "dgpu", "nsp",
>> +    "mmcx", "infra", "dram", "mdm", "wlan", "usb1", "usb2", "usb3",
>> +};
>> +
>> +/* Domain register offsets in node base */
>> +static const u32 domain_offsets[SPEL_DOMAIN_MAX] = {
>> +    [SPEL_DOMAIN_SYS]    = 0x40,
>> +    [SPEL_DOMAIN_SOC]    = 0x00,
>> +    [SPEL_DOMAIN_CL0]    = 0x5C,
>> +    [SPEL_DOMAIN_CL1]    = 0x60,
>> +    [SPEL_DOMAIN_CL2]    = 0x64,
>> +    [SPEL_DOMAIN_IGPU]    = 0x08,
>> +    [SPEL_DOMAIN_DGPU]    = 0x44,
>> +    [SPEL_DOMAIN_NSP]    = 0x0C,
>> +    [SPEL_DOMAIN_MMCX]    = 0x10,
>> +    [SPEL_DOMAIN_INFRA]    = 0x18,
>> +    [SPEL_DOMAIN_DRAM]    = 0x1C,
>> +    [SPEL_DOMAIN_MDM]    = 0x48,
>> +    [SPEL_DOMAIN_WLAN]    = 0x4C,
>> +    [SPEL_DOMAIN_USB1]    = 0x50,
>> +    [SPEL_DOMAIN_USB2]    = 0x54,
>> +    [SPEL_DOMAIN_USB3]    = 0x58,
>> +};
> 
> Same comment

ACK

> 
>> +/**
>> + * 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
> 
> Where is 'supported_mask' initialized?

It is part of constraint_info table and used as bit mask to compare 
againt capability config register value during init for each PL for 
different domains.

> 
>> + * @domain_id:        Domain this constraint applies to
>> + * @pl_id:        Power limit ID (PL1, PL2, etc.)
> 
> [ ... ]
> 
>> +
>> +/**
>> + * struct spel_domain - SPEL power domain
>> + * @power_zone:        Powercap zone
>> + * @lock:        Mutex protecting register access
>> + * @sp:            Parent system
>> + * @status_reg:        Energy counter register
>> + * @pl_name:        Power limit names
>> + * @name:        Domain name
>> + * @id:            Domain type ID
>> + */
>> +struct spel_domain {
> 
> [ ... ]
> 
>> +    struct spel_system *sp;
> 
> [ ... ]
> 
>> +struct spel_system {
>> +    struct spel_domain *domains;
> 
> [ ... ]
> 
>> +};
> 
> There is a cyclic dependency between struct spel_system <-> struct 
> spel_domain. Could it be solved ?

ACK

> 
>> +#define power_zone_to_spel_domain(_zone) \
>> +    container_of(_zone, struct spel_domain, power_zone)
>> +
>> +/* Helper functions */
>> +static bool is_pl_valid(struct spel_domain *sd, int pl)
>> +{
>> +    if (pl < POWER_LIMIT1 || pl >= NR_POWER_LIMITS)
>> +        return false;
> 
> The call to this function is strange. It is like you don't trust your 
> own code.

ACK, I will remove it in v2

> 
>> +    return sd->pl_name[pl] ? true : false;
>> +}
>> +
> 
> [ ... ]
> 
>> +
>> +    switch (pl_op) {
>> +    case PL_LIMIT:
>> +        value &= POWER_LIMIT_MASK;
>> +        if (xlate)
>> +            *data = spel_unit_xlate(sd, POWER_UNIT, value, 0);
>> +        else
>> +            *data = value;
>> +        break;
>> +    case PL_TIME_WINDOW:
>> +        /* Decode time window: bits [22:16] are upper 7 bits, [14:0] 
>> are lower 15 bits */
>> +        value = ((value & TIME_WINDOW_MASK_H) >> 16 << 15) |
>> +            (value & TIME_WINDOW_MASK_L);
> 
> [ ... ]
> 
>> +        reg_val = (reg_val & ~POWER_LIMIT_MASK) | new_val;
>> +
> 
> [ ... ]
> 
>> +        if (new_val == 0)
>> +            reg_val &= ~POWER_LIMIT_ENABLE;
>> +        else
>> +            reg_val |= POWER_LIMIT_ENABLE;
>> +        break;
> 
> [ ... ]
> 
>> +    case PL_TIME_WINDOW:
>> +        /*
>> +         * Encode time window: upper 7 bits to [22:16], lower 15 bits 
>> to [14:0]
>> +         * Time window register is separate from limit register 
>> (different offset),
>> +         * so we write only the time window bits without preserving 
>> any enable bit.
>> +         */
>> +        new_val = spel_unit_xlate(sd, TIME_UNIT, value, 1);
>> +        reg_val = (((new_val >> 15) & 0x7F) << 16) |
>> +              (new_val & 0x7FFF);
> 
> 
> The trend today is to use the FIELD_* macros for bits ops

ACK

> 
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    writel(reg_val, reg_addr);
>> +    return 0;
>> +}
>> +
>> +/* Powercap zone operations */
>> +static int spel_get_energy_counter(struct powercap_zone *power_zone, 
>> u64 *energy_raw)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
>> +    u64 value;
>> +
>> +    value = readl(sd->status_reg);
> 
> nit: seems an line in between would help for the readability

ACK

> 
>> +    *energy_raw = spel_unit_xlate(sd, ENERGY_UNIT, value, 0);
>> +
>> +    return 0;
>> +}
>> +
>> +static int spel_get_max_energy_counter(struct powercap_zone *pcd_dev, 
>> u64 *energy)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(pcd_dev);
>> +
>> +    *energy = spel_unit_xlate(sd, ENERGY_UNIT, ENERGY_STATUS_MASK, 0);
> 
> nit: missing line

ACK>
>> +    return 0;
>> +}
>> +
>> +static int spel_release_zone(struct powercap_zone *power_zone)
>> +{
>> +    return 0;
>> +}
>> +
>> +static int spel_find_nr_power_limit(struct spel_domain *sd)
>> +{
>> +    int i, nr_pl = 0;
>> +
>> +    for (i = 0; i < NR_POWER_LIMITS; i++) {
>> +        if (is_pl_valid(sd, i))
>> +            nr_pl++;
>> +    }
>> +
>> +    return nr_pl;
>> +}
>> +
>> +static const struct powercap_zone_ops zone_ops = {
>> +    .get_energy_uj = spel_get_energy_counter,
>> +    .get_max_energy_range_uj = spel_get_max_energy_counter,
>> +    .release = spel_release_zone,
>> +};
>> +
>> +/* Constraint operations */
>> +static int spel_constraint_to_pl(struct spel_domain *sd, int cid)
>> +{
>> +    int i, j;
> 
> 'j' name is misleading because it is usually used for nested 'for' blocks

ACK

> 
>> +    for (i = POWER_LIMIT1, j = 0; i < NR_POWER_LIMITS; i++) {
> 
> Do not rely on POWER_LIMIT1 because if someday it is moved in the enum, 
> all the code assuming it is zero will be broken

ACK

> 
>> +        if (is_pl_valid(sd, i) && j++ == cid)
>  > +            return i;> +    }
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int spel_set_power_limit(struct powercap_zone *power_zone, int 
>> cid,
>> +                u64 power_limit)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
>> +    int id;
>> +
>> +    id = spel_constraint_to_pl(sd, cid);
>> +    if (id < 0)
>> +        return id;
>> +
>> +    return spel_write_pl_data(sd, id, PL_LIMIT, power_limit);
>> +}
>> +
>> +static int spel_get_power_limit(struct powercap_zone *power_zone, int 
>> cid,
>> +                u64 *data)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
>> +    u64 val;
>> +    int ret, id;
>> +
>> +    id = spel_constraint_to_pl(sd, cid);
>> +    if (id < 0)
>> +        return id;
>> +
>> +    ret = spel_read_pl_data(sd, id, PL_LIMIT, true, &val);
>> +    if (!ret)
>> +        *data = val;
>> +
>> +    return ret;
>> +}
>> +
>> +static int spel_set_time_window(struct powercap_zone *power_zone, int 
>> cid,
>> +                u64 window)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
>> +    int id;
>> +
>> +    id = spel_constraint_to_pl(sd, cid);
>> +    if (id < 0)
>> +        return id;
>> +
>> +    return spel_write_pl_data(sd, id, PL_TIME_WINDOW, window);
>> +}
>> +
>> +static int spel_get_time_window(struct powercap_zone *power_zone, int 
>> cid,
>> +                u64 *data)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
>> +    u64 val;
>> +    int ret, id;
>> +
>> +    id = spel_constraint_to_pl(sd, cid);
>> +    if (id < 0)
>> +        return id;
>> +
>> +    ret = spel_read_pl_data(sd, id, PL_TIME_WINDOW, true, &val);
>> +    if (!ret)
>> +        *data = val;
>> +
>> +    return ret;
>> +}
>> +
>> +static const char *spel_get_constraint_name(struct powercap_zone 
>> *power_zone,
>> +                        int cid)
>> +{
>> +    struct spel_domain *sd = power_zone_to_spel_domain(power_zone);
>> +    int id;
>> +
>> +    id = spel_constraint_to_pl(sd, cid);
>> +    if (id >= 0)
>> +        return sd->pl_name[id];
>> +
>> +    return NULL;
>> +}
>> +
>> +static const struct powercap_zone_constraint_ops constraint_ops = {
>> +    .set_power_limit_uw = spel_set_power_limit,
>> +    .get_power_limit_uw = spel_get_power_limit,
>> +    .set_time_window_us = spel_set_time_window,
>> +    .get_time_window_us = spel_get_time_window,
>> +    .get_name = spel_get_constraint_name,
>> +};
>> +
>> +static void spel_init_domains(struct spel_system *sp)
>> +{
>> +    unsigned int i;
>> +
>> +    for (i = 0; i < SPEL_DOMAIN_MAX; i++) {
>> +        struct spel_domain *sd = &sp->domains[i];
>> +
>> +        sd->sp = sp;
>> +        snprintf(sd->name, SPEL_DOMAIN_NAME_LENGTH, "%s",
>> +             spel_domain_names[i]);
>> +        sd->id = i;
>> +        sd->status_reg = sp->node_base + domain_offsets[i];
>> +
>> +        /* PL1 is always supported (required for powercap 
>> registration) */
>> +        sp->limits[i] = BIT(POWER_LIMIT1);
>> +        sd->pl_name[POWER_LIMIT1] = pl_names[POWER_LIMIT1];
>> +    }
>> +}
>> +
>> +static int spel_check_unit(struct spel_system *sp)
>> +{
>> +    u32 value, shift;
>> +
>> +    /* Read power_unit and time_unit from offset 0x0 */
>> +    value = readl(sp->config_base);
>> +
>> +    /*
>> +     * Unit calculation: 1 / (2^shift)
>> +     * Masks limit: TIME_UNIT (4 bits, max 15), POWER_UNIT (3 bits, 
>> max 7).
>> +     */
>> +    shift = (value & POWER_UNIT_MASK) >> POWER_UNIT_OFFSET;
>> +    sp->power_unit = 1000000 / (1 << shift);
>> +
>> +    shift = (value & TIME_UNIT_MASK) >> TIME_UNIT_OFFSET;
>> +    sp->time_unit = 1000000 / (1 << shift);
>> +
>> +    /* Read energy_unit from ENERGY_RPT_UNIT_OFFSET */
>> +    value = readl(sp->config_base + ENERGY_RPT_UNIT_OFFSET);
>> +
>> +    /*
>> +     * Unit calculation: 1 / (2^shift)
>> +     * Masks limit: ENERGY_UNIT (4 bits, max 15).
>> +     */
>> +    shift = (value & ENERGY_UNIT_MASK) >> ENERGY_UNIT_OFFSET;
>> +    sp->energy_unit = ENERGY_UNIT_SCALE * 1000000 / (1 << shift);
>> +
>> +    dev_dbg(sp->dev, "Units: energy=%dnJ, time=%dus, power=%duW\n",
>> +        sp->energy_unit, sp->time_unit, sp->power_unit);
>> +
>> +    return 0;
>> +}
>> +
>> +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 */
>> +    for (i = POWER_LIMIT2; i < NR_POWER_LIMITS; i++) {
> 
> For my understanding, why is it starting at POWER_LIMIT2 ?

Since It is failing to register a powercap zone with 0 constraint, I 
need to add alteast one constraint for each domain. PL1 is added by 
default for all domains.

> 
>> +        for (j = 0; j < ARRAY_SIZE(constraints); j++) {
>> +            struct spel_constraint_info *ci = &constraints[j];
>> +
>> +            if (ci->domain_id == sd->id && ci->pl_id == i) {
>> +                if (capabilities & ci->supported_mask) {
>> +                    sp->limits[sd->id] |= BIT(i);
>> +                    sd->pl_name[i] = pl_names[i];
> 
> So, that explains the is_pl_valid()
> 
> Please do not use duplicated array with non-NULL pointer checks
> 
> Or sd->pl_name[] contains all the *valids* power limits, so its size is 
> different than (or equal to) pl_names. Or it is a fixed array with a 
> structure containing a flag telling if it is enabled or not.
> 
> No need to duplicate the array

ACK

> 
>> +                }
>> +                break;
>> +            }
>> +        }
>> +    }
>> +}
>> +
> 
> [ ... ]
> 
>> +static void spel_remove(struct platform_device *pdev)
>> +{
>> +    struct spel_system *sp = platform_get_drvdata(pdev);
>> +    int i;
>> +
>> +    if (!sp)
>> +        return;
> 
> Why test if the value is correct? There is no reason it changed after 
> 'probe' was successful

ACK

Thanks,
Manaf

> 
>> +
>> +    /* Unregister in reverse order: children first, then SOC, then 
>> SYS */
>> +    for (i = SPEL_DOMAIN_MAX - 1; i >= 0; i--)
>> +        powercap_unregister_zone(sp->control_type, &sp- 
>> >domains[i].power_zone);
>> +
>> +    powercap_unregister_control_type(sp->control_type);
>> +}
>> +
>> +static const struct of_device_id spel_of_match[] = {
>> +    { .compatible = "qcom,spel" },
>> +    { }
>> +};
>> +MODULE_DEVICE_TABLE(of, spel_of_match);
>> +
>> +static struct platform_driver spel_driver = {
>> +    .probe = spel_probe,
>> +    .remove = spel_remove,
>> +    .driver = {
>> +        .name = "qcom_spel",
>> +        .of_match_table = spel_of_match,
>> +    },
>> +};
>> +
>> +module_platform_driver(spel_driver);
>> +
>> +MODULE_DESCRIPTION("Qualcomm SPEL Powercap Driver");
>> +MODULE_LICENSE("GPL");
>>
>