Re: [PATCH V3 3/4] spmi: pmic-arb: add support for HW version 5

Stephen Boyd
2017-07-14T17:30:32Z

On 07/11, Kiran Gunda wrote:
> @@ -420,7 +440,8 @@ static int pmic_arb_write_cmd(struct spmi_controller *ctrl, u8 opc, u8 sid,
>

Mostly style nitpicks!

> /* Start the transaction */
> pmic_arb_base_write(pmic_arb, offset + PMIC_ARB_CMD, cmd);
> - rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr);
> + rc = pmic_arb_wait_for_done(ctrl, pmic_arb->wr_base, sid, addr,
> + PMIC_ARB_CHANNEL_RW);
> raw_spin_unlock_irqrestore(&pmic_arb->lock, flags);
>
> return rc;
> @@ -681,12 +702,19 @@ static int qpnpint_irq_domain_dt_translate(struct irq_domain *d,
> ppid = intspec[0] << 8 | intspec[1];
> rc = pmic_arb->ver_ops->ppid_to_apid(pmic_arb, ppid);
> if (rc < 0) {
> - dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = 0x%x, periph = 0x%x, irq = %x rc = %d\n",
> + dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u rc = %d\n",
> intspec[0], intspec[1], intspec[2], rc);

Unrelated change?

> return rc;
> }
>
> apid = rc;
> + if (pmic_arb->apid_data[apid].irq_ee != pmic_arb->ee) {
> + dev_err(&pmic_arb->spmic->dev, "failed to xlate sid = %#x, periph = %#x, irq = %u: ee=%u but owner=%u\n",
> + intspec[0], intspec[1], intspec[2], pmic_arb->ee,
> + pmic_arb->apid_data[apid].irq_ee);
> + return -ENODEV;
> + }
> +
> /* Keep track of {max,min}_apid for bounding search during interrupt */
> if (apid > pmic_arb->max_apid)
> pmic_arb->max_apid = apid;
> return apid_valid & ~PMIC_ARB_APID_VALID;
> }
>
> +static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pmic_arb)
> +{
> + struct apid_data *apid_info = pmic_arb->apid_data;
> + struct apid_data *prev_apid_info;
> + u16 i, j, ppid;
> + bool valid, is_irq_ee;
> + u32 regval, offset;
> +
> + /*
> + * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to PPID.

Is this comment stale? PMIC_ARB_REG_CHNL macro was deleted?

> + * ppid_to_apid is an in-memory invert of that table. In order to allow
> + * multiple EEs to write to a single PPID in arbiter version 5, there
> + * is more than one APID mapped to each PPID. The owner field for each
> + * of these mappings specifies the EE which is allowed to write to the
> + * APID. The owner of the last (highest) APID for a given PPID will
> + * receive interrupts from the PPID.
> + */
> + for (i = 0; ; i++, apid_info++) {
> + offset = pmic_arb->ver_ops->apid_map_offset(i);
> + if (offset >= pmic_arb->core_size)
> + break;
> +
> + regval = readl_relaxed(pmic_arb->core + offset);
> + if (!regval)
> + continue;
> + ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
> + is_irq_ee = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
> +
> + regval = readl_relaxed(pmic_arb->cnfg +
> + SPMI_OWNERSHIP_TABLE_REG(i));
> + apid_info->write_ee = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
> +
> + apid_info->irq_ee = is_irq_ee ?
> + apid_info->write_ee : INVALID_EE;

Perhaps apid_info can be renamed to apidd (for apid descriptor)
or apidi (for apid info) and then this line is short enough to
fit on one line?

> +
> + valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
> + j = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;

Maybe j can be 'apid'. Slightly more informative and usually 'j'
is reserved for iterating, which in this case we're not doing.
We're just directly indexing an apid into a table.

> + prev_apid_info = &pmic_arb->apid_data[j];
> +
> + if (valid && is_irq_ee &&
> + prev_apid_info->write_ee == pmic_arb->ee) {
> + /*
> + * Duplicate PPID mapping after the one for this EE;
> + * override the irq owner
> + */
> + prev_apid_info->irq_ee = apid_info->irq_ee;
> + } else if (!valid || is_irq_ee) {
> + /* First PPID mapping or duplicate for another EE */
> + pmic_arb->ppid_to_apid[ppid] = i | PMIC_ARB_APID_VALID;
> + }
> +
> + apid_info->ppid = ppid;
> + pmic_arb->last_apid = i;
> + }
> +
> + /* Dump the mapping table for debug purposes. */
> + dev_dbg(&pmic_arb->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
> + for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
> + valid = pmic_arb->ppid_to_apid[ppid] & PMIC_ARB_APID_VALID;
> + i = pmic_arb->ppid_to_apid[ppid] & ~PMIC_ARB_APID_VALID;
> + if (valid) {
> + apid_info = &pmic_arb->apid_data[i];
> + dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
> + ppid, i, apid_info->write_ee, apid_info->irq_ee);
> + }

Could be

for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
apid = pmic_arb->ppid_to_apid[ppid];
if (apid & PMIC_ARB_APID_VALID) {
apid &= ~PMIC_ARB_VALID;
apidd = &pmic_arb->apid_data[apid];
dev_dbg(&pmic_arb->spmic->dev, "%#03X %3u %2u %2u\n",
ppid, apid, apidd->write_ee, apidd->irq_ee);
}
}

Which maybe is clearer because it uses less local variables that
don't get used more than once.

--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,