On 2017-07-14 23:00, Stephen Boyd wrote:
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!
Will check and address in the next patch.
/* 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?
This just to print the irq number in decimal format.
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?
will remove this line in the next patch.
+ * 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?
ok. will change it in next patch.
+
+ 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.
Will change in the next patch.
+ 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.
yes. Will change it in the next patch.
