Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups
From: kgunda
Date: Thu Jun 01 2017 - 12:53:30 EST
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;
+}
+