From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups Date: Tue, 30 May 2017 18:44:03 -0700 Message-ID: <20170531014403.GV20170@codeaurora.org> References: <1496147943-25822-1-git-send-email-kgunda@codeaurora.org> <1496147943-25822-5-git-send-email-kgunda@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from smtp.codeaurora.org ([198.145.29.96]:41442 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750942AbdEaBoF (ORCPT ); Tue, 30 May 2017 21:44:05 -0400 Content-Disposition: inline In-Reply-To: <1496147943-25822-5-git-send-email-kgunda@codeaurora.org> Sender: linux-arm-msm-owner@vger.kernel.org List-Id: linux-arm-msm@vger.kernel.org To: Kiran Gunda Cc: Abhijeet Dharmapurikar , Subbaraman Narayanamurthy , Christophe JAILLET , David Collins , linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org, adharmap@quicinc.com, aghayal@qti.qualcomm.com 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