Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter v8

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

On 10/24/25 11:33 AM, Jishnu Prakash wrote:
> From: David Collins <david.collins@oss.qualcomm.com>
> 
> PMIC arbiter v8 supports up to 4 SPMI buses and up to 8192 PMIC
> peripherals.  Its register map differs from v7 as several fields
> increased in size. Add support for PMIC arbiter version 8.
> 
> Signed-off-by: David Collins <david.collins@oss.qualcomm.com>
> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@oss.qualcomm.com>
> Signed-off-by: Jishnu Prakash <jishnu.prakash@oss.qualcomm.com>
> ---
>  drivers/spmi/spmi-pmic-arb.c | 324 +++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 294 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
> index 91581974ef84..612736973e4b 100644
> --- a/drivers/spmi/spmi-pmic-arb.c
> +++ b/drivers/spmi/spmi-pmic-arb.c
> @@ -3,6 +3,7 @@
>   * Copyright (c) 2012-2015, 2017, 2021, The Linux Foundation. All rights reserved.
>   */
>  #include <linux/bitmap.h>
> +#include <linux/bitfield.h>

bit'f'ield < bit'm'ap

[...]

>  #define spec_to_hwirq(slave_id, periph_id, irq_id, apid) \
> -	((((slave_id) & 0xF)   << 28) | \
> -	(((periph_id) & 0xFF)  << 20) | \
> -	(((irq_id)    & 0x7)   << 16) | \
> -	(((apid)      & 0x3FF) << 0))
> +	(FIELD_PREP(GENMASK(28, 24), (slave_id))  | \
> +	FIELD_PREP(GENMASK(23, 16), (periph_id)) | \
> +	FIELD_PREP(GENMASK(15, 13), (irq_id))    | \
> +	FIELD_PREP(GENMASK(12, 0),  (apid)))

I think this could be further improved by:

#define SOMETHING_SLAVE_ID_SOMETHING	GENMASK(28, 24)

and then below:

[...]

> -	if (intspec[0] > 0xF || intspec[1] > 0xFF || intspec[2] > 0x7)
> +	if (intspec[0] > 0x1F || intspec[1] > 0xFF || intspec[2] > 0x7)
>  		return -EINVAL;

we can use FIELD_MAX(SOMETHING...)

[...]

> +static int pmic_arb_get_core_resources_v8(struct platform_device *pdev,
> +					  void __iomem *core)
> +{
> +	struct spmi_pmic_arb *pmic_arb = platform_get_drvdata(pdev);
> +
> +	pmic_arb->apid_map = devm_platform_ioremap_resource_byname(pdev,
> +								   "chnl_map");

Feel free to unwrap this line

> +	if (IS_ERR(pmic_arb->apid_map))
> +		return PTR_ERR(pmic_arb->apid_map);
> +
> +	pmic_arb->core = core;
> +
> +	pmic_arb->max_periphs = PMIC_ARB_MAX_PERIPHS_V8;
> +
> +	return pmic_arb_get_obsrvr_chnls_v2(pdev);
> +}
> +
> +static int pmic_arb_get_bus_resources_v8(struct platform_device *pdev,
> +					 struct device_node *node,
> +					 struct spmi_pmic_arb_bus *bus)
> +{
> +	int index;
> +
> +	index = of_property_match_string(node, "reg-names", "chnl_owner");
> +	if (index < 0) {
> +		dev_err(&pdev->dev, "chnl_owner reg region missing\n");
> +		return -EINVAL;
> +	}
> +
> +	bus->apid_owner = devm_of_iomap(&pdev->dev, node, index, NULL);
> +
> +	return PTR_ERR_OR_ZERO(bus->apid_owner);

Is this any different chan using devm_platform_ioremap_resource_byname()
like you did above?


> +}
> +
> +static int pmic_arb_read_apid_map_v8(struct spmi_pmic_arb_bus *bus)
> +{
> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
> +	struct apid_data *apidd;
> +	struct apid_data *prev_apidd;
> +	u16 i, apid, ppid, apid_max;
> +	bool valid, is_irq_ee;
> +	u32 regval, offset;
> +
> +	/*
> +	 * In order to allow multiple EEs to write to a single PPID in arbiter
> +	 * version 8, there can be more than one APID mapped to each PPID.  The
> +	 * owner field for each of these mappings specifies the EE which is
> +	 * allowed to write to the APID.  The owner of the last (highest) APID
> +	 * which has the IRQ owner bit set for a given PPID will receive
> +	 * interrupts from the PPID.
> +	 *
> +	 * In arbiter version 8, the APID numbering space is divided between
> +	 * the SPMI buses according to this mapping:
> +	 * APID = 0     to N-1       --> bus 0
> +	 * APID = N     to N+M-1     --> bus 1
> +	 * APID = N+M   to N+M+P-1   --> bus 2
> +	 * APID = N+M+P to N+M+P+Q-1 --> bus 3
> +	 * where N = number of APIDs supported by bus 0
> +	 *       M = number of APIDs supported by bus 1
> +	 *       P = number of APIDs supported by bus 2
> +	 *       Q = number of APIDs supported by bus 3
> +	 */
> +	apidd = &bus->apid_data[bus->base_apid];
> +	apid_max = bus->base_apid + bus->apid_count;
> +	for (i = bus->base_apid; i < apid_max; i++, apidd++) {
> +		offset = pmic_arb->ver_ops->apid_map_offset(i);
> +		regval = readl_relaxed(pmic_arb->apid_map + offset);
> +		if (!regval)
> +			continue;
> +		ppid = regval & PMIC_ARB_V8_PPID_MASK;
> +		is_irq_ee = PMIC_ARB_V8_CHAN_IS_IRQ_OWNER(regval);

[...]


If you parametrize the masks, the diff against pmic_arb_read_apid_map_v5
is 3 lines of code instead


> +
> +	return 0;
> +}
> +
> +static int pmic_arb_init_apid_v8(struct spmi_pmic_arb_bus *bus, int index)
> +{
> +	struct spmi_pmic_arb *pmic_arb = bus->pmic_arb;
> +	int ret, i;
> +
> +	if (index < 0 || index >= PMIC_ARB_MAX_BUSES_V8) {
> +		dev_err(&bus->spmic->dev, "Unsupported bus index %d detected\n",
> +			index);
> +		return -EINVAL;
> +	}
> +
> +	bus->base_apid = 0;
> +	bus->apid_count = 0;
> +	for (i = 0; i <= index; i++) {
> +		bus->base_apid += bus->apid_count;
> +		bus->apid_count = readl_relaxed(pmic_arb->core + PMIC_ARB_FEATURES + i * 4) &
> +						PMIC_ARB_FEATURES_V8_PERIPH_MASK;

You can almost replace pmic_arb_init_apid_v7 with this impl

[...]

> +static void __iomem *
> +pmic_arb_apid_owner_v8(struct spmi_pmic_arb_bus *bus, u16 n)
> +{
> +	return bus->apid_owner + 0x4 * (n - bus->base_apid);
> +}

This is identical to pmic_arb_apid_owner_v7

Konrad