On 2017-06-01 11:38, 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 412481d..b755c24 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -112,7 +123,8 @@ enum pmic_arb_cmd_op_code {
struct apid_data {
u16 ppid;
- u8 owner;
+ u8 write_owner;
+ u8 irq_owner;
How about irq_ee and write_ee instead?
Sure. will change it in the subsequent patch.
};
/**
static inline void pmic_arb_base_write(struct spmi_pmic_arb *pa,
@@ -705,11 +724,18 @@ static int
qpnpint_irq_domain_dt_translate(struct irq_domain *d,
(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",
+ "failed to xlate sid = 0x%x, periph = 0x%x, irq = %u rc = %d\n",
What is this change? Also, use %#x instead of 0x%x and the "
should line up with the ( on the previous line.
Sure. will change it in the subsequent patch.
intspec[0], intspec[1], intspec[2], rc);
return rc;
}
+ if (pa->apid_data[apid].irq_owner != pa->ee) {
+ dev_err(&pa->spmic->dev, "failed to xlate sid = 0x%x, periph =
0x%x, irq = %u: ee=%u but owner=%u\n",
+ intspec[0], intspec[1], intspec[2], pa->ee,
+ pa->apid_data[apid].irq_owner);
+ return -ENODEV;
+ }
+
/* Keep track of {max,min}_apid for bounding search during interrupt
*/
if (apid > pa->max_apid)
pa->max_apid = apid;
@@ -814,9 +841,11 @@ static u16 pmic_arb_find_apid(struct
spmi_pmic_arb *pa, u16 ppid)
for (apid = pa->last_apid; apid < pa->max_periph; apid++) {
regval = readl_relaxed(pa->cnfg +
SPMI_OWNERSHIP_TABLE_REG(apid));
- pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+ pa->apid_data[apid].irq_owner
+ = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+ pa->apid_data[apid].write_owner = pa->apid_data[apid].irq_owner;
Please use a local variable pointer *apid.
Sure. will change it in the subsequent patch.
- offset = PMIC_ARB_REG_CHNL(apid);
+ offset = pa->ver_ops->channel_map_offset(apid);
if (offset >= pa->core_size)
break;
@@ -854,27 +883,110 @@ static u16 pmic_arb_find_apid(struct
spmi_pmic_arb *pa, u16 ppid)
return 0;
}
+static int pmic_arb_read_apid_map_v5(struct spmi_pmic_arb *pa)
+{
+ u32 regval, offset;
+ u16 apid, prev_apid, ppid;
+ bool valid, is_irq_owner;
+
+ /*
+ * PMIC_ARB_REG_CHNL is a table in HW mapping APID (channel) to
PPID.
+ * ppid_to_apid is an in-memory invert of that table. In order to
allow
+ * multiple EE's to write to a single PPID in arbiter version 5,
there
Drop the apostrophe ^
Ok. will change it in the subsequent patch.
+ * 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 (apid = 0; apid < pa->max_periph; apid++) {
+ offset = pa->ver_ops->channel_map_offset(apid);
+ if (offset >= pa->core_size)
+ break;
+
+ regval = readl_relaxed(pa->core + offset);
+ if (!regval)
+ continue;
+ ppid = (regval >> 8) & PMIC_ARB_PPID_MASK;
+ is_irq_owner = PMIC_ARB_CHAN_IS_IRQ_OWNER(regval);
+
+ regval = readl_relaxed(pa->cnfg +
+ SPMI_OWNERSHIP_TABLE_REG(apid));
+ pa->apid_data[apid].write_owner
+ = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
Please use a pointer like *apid and *prev where *apid is
incremented during the for-loop. That way we have shorter lines
of code:
apid->write_owner = SPMI_OWNERSHIP_TABLE_REG(regval);
and
if (valid && is_irq_owner && prev->write_owner == pa->ee) {
obviously the existing apid will need to be renamed to i, but
that's ok because it's a counter.
Sure. will do it in the subsequent patch..
+
+ pa->apid_data[apid].irq_owner = is_irq_owner ?
+ pa->apid_data[apid].write_owner : INVALID_EE;
+
+ valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID;
+ prev_apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID;
+
+ if (valid && is_irq_owner &&
+ pa->apid_data[prev_apid].write_owner == pa->ee) {
+ /*
+ * Duplicate PPID mapping after the one for this EE;
+ * override the irq owner
+ */
+ pa->apid_data[prev_apid].irq_owner
+ = pa->apid_data[apid].irq_owner;
+ } else if (!valid || is_irq_owner) {
+ /* First PPID mapping or duplicate for another EE */
+ pa->ppid_to_apid[ppid] = apid | PMIC_ARB_CHAN_VALID;
+ }
+
+ pa->apid_data[apid].ppid = ppid;
+ pa->last_apid = apid;
+ }
+
+ /* Dump the mapping table for debug purposes. */
+ dev_dbg(&pa->spmic->dev, "PPID APID Write-EE IRQ-EE\n");
+ for (ppid = 0; ppid < PMIC_ARB_MAX_PPID; ppid++) {
+ valid = pa->ppid_to_apid[ppid] & PMIC_ARB_CHAN_VALID;
+ apid = pa->ppid_to_apid[ppid] & ~PMIC_ARB_CHAN_VALID;
+
+ if (valid)
+ dev_dbg(&pa->spmic->dev, "0x%03X %3u %2u %2u\n",
Same %#x story here.
Sure. will change it in the subsequent patch.
+ ppid, apid, pa->apid_data[apid].write_owner,
+ pa->apid_data[apid].irq_owner);
+ }
+
+ return 0;
+}
+
@@ -887,6 +999,27 @@ static u16 pmic_arb_find_apid(struct
spmi_pmic_arb *pa, u16 ppid)
return 0;
}
+/*
+ * v5 offset per ee and per apid for observer channels and per apid
for
+ * read/write channels.
+ */
+static int
+pmic_arb_offset_v5(struct spmi_pmic_arb *pa, u8 sid, u16 addr,
+ enum pmic_arb_channel ch_type, u32 *offset)
+{
+ u16 apid;
+ int rc;
+
+ rc = pmic_arb_ppid_to_apid_v5(pa, sid, addr, &apid);
+ if (rc < 0)
+ return rc;
+
+ *offset = (ch_type == PMIC_ARB_CHANNEL_OBS)
+ ? 0x10000 * pa->ee + 0x80 * apid
+ : 0x10000 * apid;
Please use a switch statement to handle the enum here. That way
we can get compiler checking to make sure all enumerations are
handled.
Sure. will change it in the subsequent patch.
+ return 0;
+}
+
static u32 pmic_arb_fmt_cmd_v1(u8 opc, u8 sid, u16 addr, u8 bc)
{
return (opc << 27) | ((sid & 0xf) << 20) | (addr << 4) | (bc & 0x7);
@@ -1033,11 +1213,14 @@ static int spmi_pmic_arb_probe(struct
platform_device *pdev)
if (hw_ver < PMIC_ARB_VERSION_V3_MIN)
pa->ver_ops = &pmic_arb_v2;
- else
+ else if (hw_ver < PMIC_ARB_VERSION_V5_MIN)
pa->ver_ops = &pmic_arb_v3;
+ else
+ pa->ver_ops = &pmic_arb_v5;
- /* the apid to ppid table starts at PMIC_ARB_REG_CHNL(0) */
- pa->max_periph = (pa->core_size - PMIC_ARB_REG_CHNL(0)) / 4;
+ /* the apid to ppid table starts at PMIC_ARB_REG_CHNL0 */
+ pa->max_periph
+ = (pa->core_size - pa->ver_ops->channel_map_offset(0)) / 4;
This is really ugly. Please grow a local variable so we can keep
things on one line.
Ok. will change it in the subsequent patch.
res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
"obsrvr");
@@ -1074,6 +1257,14 @@ static int spmi_pmic_arb_probe(struct
platform_device *pdev)
err = PTR_ERR(pa->intr);
goto err_put_ctrl;
}
+ pa->acc_status = pa->intr;
+
+ /*
+ * PMIC arbiter v5 groups the IRQ control registers in the same
hardware
+ * module as the read/write channels.
+ */
+ if (hw_ver >= PMIC_ARB_VERSION_V5_MIN)
+ pa->intr = pa->wr_base;
There's some weird things going on here. How about we make the
version ops return an __iomem pointer to the address instead of
an offset? That way we don't need to care what pa->intr is or add
pa->acc_status? That could be a patch early in the series that
adjusts the ops to return an iomem pointer, and perhaps also
update the 'n' variable to be 16 instead of 8 bits. We can also
express errors through iomem pointers with IS_ERR() checks, so it
nicely removes the need to retrieve offsets through references
sometimes too.
Yes. This looks good. Will change it in the subsequent patch series.
