Re: [PATCH V1 04/15] spmi: pmic-arb: optimize table lookups
From: Stephen Boyd
Date: Tue May 30 2017 - 21:44:11 EST
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