Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups

From: kgunda@codeaurora.org
To: Stephen Boyd <sboyd@codeaurora.org>
Cc: Abhijeet Dharmapurikar <adharmap@codeaurora.org>,
	Subbaraman Narayanamurthy <subbaram@codeaurora.org>,
	Christophe JAILLET <christophe.jaillet@wanadoo.fr>,
	David Collins <collinsd@codeaurora.org>,
	linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
	adharmap@quicinc.com, aghayal@qti.qualcomm.com,
	linux-arm-msm-owner@vger.kernel.org
Subject: Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups
Date: Thu, 01 Jun 2017 22:23:17 +0530	[thread overview]
Message-ID: <babbe9bf7bcdde8dc200c76c14ac328e@codeaurora.org> (raw)
In-Reply-To: <20170531014403.GV20170@codeaurora.org>

On 2017-05-31 07:14, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
>> diff --git a/drivers/spmi/spmi-pmic-arb.c 
>> b/drivers/spmi/spmi-pmic-arb.c
>> index 7201611..6320f1f 100644
>> --- a/drivers/spmi/spmi-pmic-arb.c
>> +++ b/drivers/spmi/spmi-pmic-arb.c
>> @@ -164,6 +164,8 @@ struct spmi_pmic_arb {
>>   *			on v2 offset of SPMI_PIC_IRQ_CLEARn.
>>   */
>>  struct pmic_arb_ver_ops {
>> +	int (*ppid_to_apid)(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
>> +			u8 *apid);
> 
> Nitpick: It's called "pa" but "dev" in the next line.
> 
Will rename the pa -> dev in the next patch.
>>  	int (*mode)(struct spmi_pmic_arb *dev, u8 sid, u16 addr,
>>  			mode_t *mode);
>>  	/* spmi commands (read_cmd, write_cmd, cmd) functionality */
>> @@ -657,42 +659,6 @@ struct spmi_pmic_arb_irq_spec {
>>  	unsigned irq:3;
>>  };
>> 
>> -static int search_mapping_table(struct spmi_pmic_arb *pa,
>> -				struct spmi_pmic_arb_irq_spec *spec,
> 
> Perhaps the spec should be removed at some point if this was the
> only place it was passed to.
> 
Yes. This is passed only in this function. You want to remove this
structure and pass the slave, per and irq as function parameters?
If so we will do it in the next patch.

>> -				u8 *apid)
> 
> This code looks mostly unchanged, so please leave it as it is and
> move the pmic_arb_ppid_to_apid_v1() function here so we can see
> what actually changed.
> 
Sure. Will do that in the next patch.
>> -{
>> -	u16 ppid = spec->slave << 8 | spec->per;
>> -	u32 *mapping_table = pa->mapping_table;
>> -	int index = 0, i;
>> -	u32 data;
>> -
>> -	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
>> -		if (!test_and_set_bit(index, pa->mapping_table_valid))
>> -			mapping_table[index] = readl_relaxed(pa->cnfg +
>> -						SPMI_MAPPING_TABLE_REG(index));
>> -
>> -		data = mapping_table[index];
>> -
>> -		if (ppid & BIT(SPMI_MAPPING_BIT_INDEX(data))) {
>> -			if (SPMI_MAPPING_BIT_IS_1_FLAG(data)) {
>> -				index = SPMI_MAPPING_BIT_IS_1_RESULT(data);
>> -			} else {
>> -				*apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
>> -				return 0;
>> -			}
>> -		} else {
>> -			if (SPMI_MAPPING_BIT_IS_0_FLAG(data)) {
>> -				index = SPMI_MAPPING_BIT_IS_0_RESULT(data);
>> -			} else {
>> -				*apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
>> -				return 0;
>> -			}
>> -		}
>> -	}
>> -
>> -	return -ENODEV;
>> -}
>> -
>>  static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>>  					   struct device_node *controller,
>>  					   const u32 *intspec,
>> @@ -702,7 +668,7 @@ static int qpnpint_irq_domain_dt_translate(struct 
>> irq_domain *d,
>>  {
>>  	struct spmi_pmic_arb *pa = d->host_data;
>>  	struct spmi_pmic_arb_irq_spec spec;
>> -	int err;
>> +	int rc;
>>  	u8 apid;
>> 
>>  	dev_dbg(&pa->spmic->dev,
>> @@ -720,11 +686,14 @@ static int 
>> qpnpint_irq_domain_dt_translate(struct irq_domain *d,
>>  	spec.per   = intspec[1];
>>  	spec.irq   = intspec[2];
>> 
>> -	err = search_mapping_table(pa, &spec, &apid);
>> -	if (err)
>> -		return err;
>> -
>> -	pa->apid_to_ppid[apid] = spec.slave << 8 | spec.per;
>> +	rc = pa->ver_ops->ppid_to_apid(pa, intspec[0],
>> +			(intspec[1] << 8), &apid);
>> +	if (rc < 0) {
>> +		dev_err(&pa->spmic->dev,
>> +		"failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
>> +		intspec[0], intspec[1], intspec[2], rc);
>> +		return rc;
>> +	}
>> 
>>  	/* Keep track of {max,min}_apid for bounding search during interrupt 
>> */
>>  	if (apid > pa->max_apid)
>> @@ -758,6 +727,54 @@ static int qpnpint_irq_domain_map(struct 
>> irq_domain *d,
>>  }
>> 
>>  static int
>> +pmic_arb_ppid_to_apid_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, 
>> u8 *apid)
>> +{
>> +	u16 ppid = sid << 8 | ((addr >> 8) & 0xFF);
> 
> The function is called ppid_to_apid, but we pass in a sid and
> addr. Just pass a ppid instead of both split out?
> 
Sure .. Will do that in the next patch.
>> +	u32 *mapping_table = pa->mapping_table;
>> +	int index = 0, i;
>> +	u16 apid_valid;
>> +	u32 data;
>> +
>> +	apid_valid = pa->ppid_to_apid[ppid];
>> +	if (apid_valid & PMIC_ARB_CHAN_VALID) {
>> +		*apid = (apid_valid & ~PMIC_ARB_CHAN_VALID);
>> +		return 0;
>> +	}
>> +
> 
> From here to...
> 
>> +	for (i = 0; i < SPMI_MAPPING_TABLE_TREE_DEPTH; ++i) {
>> +		if (!test_and_set_bit(index, pa->mapping_table_valid))
>> +			mapping_table[index] = readl_relaxed(pa->cnfg +
>> +						SPMI_MAPPING_TABLE_REG(index));
>> +
>> +		data = mapping_table[index];
>> +
>> +		if (ppid & BIT(SPMI_MAPPING_BIT_INDEX(data))) {
>> +			if (SPMI_MAPPING_BIT_IS_1_FLAG(data)) {
>> +				index = SPMI_MAPPING_BIT_IS_1_RESULT(data);
>> +			} else {
>> +				*apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
>> +				pa->ppid_to_apid[ppid]
>> +					= *apid | PMIC_ARB_CHAN_VALID;
>> +				pa->apid_to_ppid[*apid] = ppid;
>> +				return 0;
>> +			}
>> +		} else {
>> +			if (SPMI_MAPPING_BIT_IS_0_FLAG(data)) {
>> +				index = SPMI_MAPPING_BIT_IS_0_RESULT(data);
>> +			} else {
>> +				*apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
>> +				pa->ppid_to_apid[ppid]
>> +					= *apid | PMIC_ARB_CHAN_VALID;
>> +				pa->apid_to_ppid[*apid] = ppid;
>> +				return 0;
>> +			}
>> +		}
>> +	}
>> +
>> +	return -ENODEV;
>> +}
> 
> here is all unchanged? Oh except apid_to_ppid/ppid_to_apid is
> inserted.
> 
Yes. correct. Will move this part to the original place in the next 
patch.
>> +
>> +static int
>>  pmic_arb_mode_v1(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t 
>> *mode)
>>  {
>>  	*mode = S_IRUSR | S_IWUSR;
>> @@ -797,6 +814,7 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb 
>> *pa, u16 ppid)
>> 
>>  		id = (regval >> 8) & PMIC_ARB_PPID_MASK;
>>  		pa->ppid_to_apid[id] = apid | PMIC_ARB_CHAN_VALID;
>> +		pa->apid_to_ppid[apid] = id;
>>  		if (id == ppid) {
>>  			apid |= PMIC_ARB_CHAN_VALID;
>>  			break;
>> @@ -809,20 +827,35 @@ static u16 pmic_arb_find_apid(struct 
>> spmi_pmic_arb *pa, u16 ppid)
>> 
>> 
>>  static int
>> -pmic_arb_mode_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t 
>> *mode)
>> +pmic_arb_ppid_to_apid_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, 
>> u8 *apid)
>>  {
>>  	u16 ppid = (sid << 8) | (addr >> 8);
>> -	u16 apid;
>> -	u8 owner;
>> +	u16 apid_valid;
>> 
>> -	apid = pa->ppid_to_apid[ppid];
>> -	if (!(apid & PMIC_ARB_CHAN_VALID))
>> +	apid_valid = pa->ppid_to_apid[ppid];
>> +	if (!(apid_valid & PMIC_ARB_CHAN_VALID))
>> +		apid_valid = pmic_arb_find_apid(pa, ppid);
>> +	if (!(apid_valid & PMIC_ARB_CHAN_VALID))
>>  		return -ENODEV;
>> 
>> +	*apid = (apid_valid & ~PMIC_ARB_CHAN_VALID);
> 
> Useless parenthesis. Please remove.
> 
Sure. Will remove it in the next patch.
> Why can't we just return a number that's >= 0 for apid and < 0
> for an error from this op? Pointer passing is odd style.
> 
Sure. Will change it in the next patch.
>> +	return 0;
>> +}
>> +