Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1653203 > unrolled thread

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

Started byKiran Gunda <kgunda@codeaurora.org>
First post2017-05-30 14:50 +0200
Last post2017-06-05 08:40 +0200
Articles 5 — 3 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups Kiran Gunda <kgunda@codeaurora.org> - 2017-05-30 14:50 +0200
    Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups Stephen Boyd <sboyd@codeaurora.org> - 2017-05-31 03:50 +0200
      Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups kgunda@codeaurora.org - 2017-06-01 19:00 +0200
        Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups Stephen Boyd <sboyd@codeaurora.org> - 2017-06-02 20:40 +0200
          Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups kgunda@codeaurora.org - 2017-06-05 08:40 +0200

#1653203 — [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups

FromKiran Gunda <kgunda@codeaurora.org>
Date2017-05-30 14:50 +0200
Subject[PATCH V1 04/15] spmi: pmic-arb: optimize table lookups
Message-ID<tMOL1-7oW-41@gated-at.bofh.it>
From: Abhijeet Dharmapurikar <adharmap@codeaurora.org>

The current driver uses a mix of radix tree and a fwd lookup
table to translate between apid and ppid. It is buggy and confusing.

Instead simply use a radix tree for v1 hardware and use the
forward lookup table for v2.

Signed-off-by: Abhijeet Dharmapurikar <adharmap@codeaurora.org>
Signed-off-by: Kiran Gunda <kgunda@codeaurora.org>
---
 drivers/spmi/spmi-pmic-arb.c | 144 ++++++++++++++++++++++++++-----------------
 1 file changed, 88 insertions(+), 56 deletions(-)

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);
 	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,
-				u8 *apid)
-{
-	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);
+	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;
+	}
+
+	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;
+}
+
+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);
+	return 0;
+}
+
+static int
+pmic_arb_mode_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t *mode)
+{
+	u8 apid;
+	u8 owner;
+	int rc;
+
+	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
+	if (rc < 0)
+		return rc;
+
 	*mode = 0;
 	*mode |= S_IRUSR;
 
-	apid &= ~PMIC_ARB_CHAN_VALID;
 	owner = pa->apid_to_owner[apid];
 	if (owner == pa->ee)
 		*mode |= S_IWUSR;
@@ -833,15 +866,12 @@ static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
 static int
 pmic_arb_offset_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, u32 *offset)
 {
-	u16 ppid = (sid << 8) | (addr >> 8);
-	u16 apid;
+	u8 apid;
+	int rc;
 
-	apid = pa->ppid_to_apid[ppid];
-	if (!(apid & PMIC_ARB_CHAN_VALID))
-		apid = pmic_arb_find_apid(pa, ppid);
-	if (!(apid & PMIC_ARB_CHAN_VALID))
-		return -ENODEV;
-	apid &= ~PMIC_ARB_CHAN_VALID;
+	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
+	if (rc < 0)
+		return rc;
 
 	*offset = 0x1000 * pa->ee + 0x8000 * apid;
 	return 0;
@@ -898,6 +928,7 @@ static u32 pmic_arb_irq_clear_v2(u8 n)
 }
 
 static const struct pmic_arb_ver_ops pmic_arb_v1 = {
+	.ppid_to_apid		= pmic_arb_ppid_to_apid_v1,
 	.mode			= pmic_arb_mode_v1,
 	.non_data_cmd		= pmic_arb_non_data_cmd_v1,
 	.offset			= pmic_arb_offset_v1,
@@ -909,6 +940,7 @@ static u32 pmic_arb_irq_clear_v2(u8 n)
 };
 
 static const struct pmic_arb_ver_ops pmic_arb_v2 = {
+	.ppid_to_apid		= pmic_arb_ppid_to_apid_v2,
 	.mode			= pmic_arb_mode_v2,
 	.non_data_cmd		= pmic_arb_non_data_cmd_v2,
 	.offset			= pmic_arb_offset_v2,
-- 
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
--

[toc] | [next] | [standalone]

#1653766

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-05-31 03:50 +0200
Message-ID<tN0VP-6xv-1@gated-at.bofh.it>
In reply to#1653203
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.

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

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

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

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

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

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.

> +	return 0;
> +}
> +

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1655532

Fromkgunda@codeaurora.org
Date2017-06-01 19:00 +0200
Message-ID<tNBC1-58o-21@gated-at.bofh.it>
In reply to#1653766
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;
>> +}
>> +

[toc] | [prev] | [next] | [standalone]

#1656464

FromStephen Boyd <sboyd@codeaurora.org>
Date2017-06-02 20:40 +0200
Message-ID<tNZEm-4Mu-13@gated-at.bofh.it>
In reply to#1655532
On 06/01, kgunda@codeaurora.org wrote:
> 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
> >>@@ -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.

We don't pass it anywhere though? So I'm just saying have local
variables instead of the structure.


-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1657306

Fromkgunda@codeaurora.org
Date2017-06-05 08:40 +0200
Message-ID<tOTQe-7Nw-3@gated-at.bofh.it>
In reply to#1656464
On 2017-06-03 00:01, Stephen Boyd wrote:
> On 06/01, kgunda@codeaurora.org wrote:
>> 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
>> >>@@ -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.
> 
> We don't pass it anywhere though? So I'm just saying have local
> variables instead of the structure.
Sure. Thanks for the clarification.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web