Message-ID: <28e2020c-8622-4bc3-aa5f-516a7d2abdb1@oss.qualcomm.com>
Date: Sat, 1 Nov 2025 07:52:24 +0530
From: Jishnu Prakash <jishnu.prakash@....qualcomm.com>
To: Konrad Dybcio <konrad.dybcio@....qualcomm.com>,
        Stephen Boyd <sboyd@...nel.org>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley
 <conor+dt@...nel.org>,
        David Collins <david.collins@....qualcomm.com>
Cc: linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, aiqun.yu@....qualcomm.com,
        kamal.wadhwa@....qualcomm.com, jingyi.wang@....qualcomm.com
Subject: Re: [PATCH v3 3/3] spmi: spmi-pmic-arb: add support for PMIC arbiter
 v8

Hi Konrad,

On 10/27/2025 5:47 PM, Konrad Dybcio wrote:
> On 10/24/25 11:33 AM, Jishnu Prakash wrote:
>> From: David Collins <david.collins@....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@....qualcomm.com>
>> Signed-off-by: Kamal Wadhwa <kamal.wadhwa@....qualcomm.com>
>> Signed-off-by: Jishnu Prakash <jishnu.prakash@....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

I'll make all the changes you have suggested above in the next series.

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

devm_platform_ioremap_resource_byname() would work for mapping resources
directly under the main device node, like"chnl_map" , but in this case, the
resource "chnl_owner" is under a child node of the the main node, so we
need to use devm_of_iomap() here.

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

Are you suggesting we should try to have a common helper function for v5/v7
and v8, which does the bulk of these actions and can be given different
input arguments to distinguish the versions?

There is at least one more difference which I don't think we can easily
accommodate with parameters:

In pmic_arb_read_apid_map_v5(), we have:
regval = readl_relaxed(pmic_arb->core + offset);

In pmic_arb_read_apid_map_v8(), we have:
regval = readl_relaxed(pmic_arb->apid_map + offset);


But in any case, we would have to add `if` checks to distinguish arbiter
versions to use a helper function. This itself might not be a good idea as it
would break the abstraction already implemented in the probe, by having
PMIC version checking happen only in the probe, to select the correct set
of ver_ops functions for a particular version.


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

They are not exactly the same - v7 arbiter supports at most 2 buses,
so pmic_arb_init_apid_v7 needs to restrict the max value of index to 2.

Here too, using a common helper function would require us to add more
`if` checks for the version, which leads to the same kind of issue as above.


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

This is not exactly right, pmic_arb_apid_owner_v7 uses bus->cnfg
and pmic_arb_apid_owner_v8 uses bus->apid_owner at the same place.
They are both already single line functions, would it really help
to try to simplify them further?

Thanks,
Jishnu

> 
> Konrad

Powered by blists - more mailing lists