[V1,05/15] spmi: pmic-arb: cleanup unrequested irqs

Commit Message

Kiran Gunda May 30, 2017, 12:38 p.m. UTC

From: Abhijeet Dharmapurikar <[e-mail]>

We see a unmapped irqs trigger right around bootup. This could
likely be because the bootloader exited leaving the interrupts
in an unknown or unhandled state.  Ack and mask the interrupt
if one is found. A request_irq later will unmask it and also
setup proper mapping structures.

Also the current driver ensures that no read/write transaction
is in progress while it makes changes to the interrupt regions.
This is not necessary because read/writes over spmi and arbiter
interrupt control are independent operations. Hence, remove the
synchronized accesses to interrupt region.

Signed-off-by: Abhijeet Dharmapurikar <[e-mail]>
Signed-off-by: Kiran Gunda <[e-mail]>
---
 drivers/spmi/spmi-pmic-arb.c | 101 ++++++++++++++++++-------------------------
 1 file changed, 43 insertions(+), 58 deletions(-)

Comments

Stephen Boyd May 31, 2017, 1:57 a.m. UTC | #1

On 05/30, Kiran Gunda wrote:
> From: Abhijeet Dharmapurikar <[e-mail]>
> 
> We see a unmapped irqs trigger right around bootup. This could
> likely be because the bootloader exited leaving the interrupts
> in an unknown or unhandled state.  Ack and mask the interrupt
> if one is found. A request_irq later will unmask it and also
> setup proper mapping structures.

Do we have systems where this is causing an interrupt storm due
to a level high interrupt or something? Just plain acking and
masking irqs at boot if we don't have an irq descriptor created
yet doesn't sound like a good idea, because we'll lose all
interrupts that happen before this driver probes?

> 
> Also the current driver ensures that no read/write transaction
> is in progress while it makes changes to the interrupt regions.
> This is not necessary because read/writes over spmi and arbiter
> interrupt control are independent operations. Hence, remove the
> synchronized accesses to interrupt region.

That's another patch for this paragraph.

Kiran Gunda June 6, 2017, 10:50 a.m. UTC | #2

On 2017-05-31 07:27, Stephen Boyd wrote:
> On 05/30, Kiran Gunda wrote:
>> From: Abhijeet Dharmapurikar <[e-mail]>
>> 
>> We see a unmapped irqs trigger right around bootup. This could
>> likely be because the bootloader exited leaving the interrupts
>> in an unknown or unhandled state.  Ack and mask the interrupt
>> if one is found. A request_irq later will unmask it and also
>> setup proper mapping structures.
> 
> Do we have systems where this is causing an interrupt storm due
> to a level high interrupt or something? Just plain acking and
> masking irqs at boot if we don't have an irq descriptor created
> yet doesn't sound like a good idea, because we'll lose all
> interrupts that happen before this driver probes?
> 
Yes. There were instances of an interrupt storm without this patch.
There is an RT_STATUS register in the  peripheral address space which
maintains the real time state and can be read by the client driver
before it registers for the irq. Few client drivers such as charger
already doing this.
>> 
>> Also the current driver ensures that no read/write transaction
>> is in progress while it makes changes to the interrupt regions.
>> This is not necessary because read/writes over spmi and arbiter
>> interrupt control are independent operations. Hence, remove the
>> synchronized accesses to interrupt region.
> 
> That's another patch for this paragraph.
This patch is already pulled in to Greg's tree.
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Stephen Boyd June 13, 2017, 2:11 a.m. UTC | #3

On 06/06, [e-mail] wrote:
> On 2017-05-31 07:27, Stephen Boyd wrote:
> >On 05/30, Kiran Gunda wrote:
> >>From: Abhijeet Dharmapurikar <[e-mail]>
> >>
> >>We see a unmapped irqs trigger right around bootup. This could
> >>likely be because the bootloader exited leaving the interrupts
> >>in an unknown or unhandled state.  Ack and mask the interrupt
> >>if one is found. A request_irq later will unmask it and also
> >>setup proper mapping structures.
> >
> >Do we have systems where this is causing an interrupt storm due
> >to a level high interrupt or something? Just plain acking and
> >masking irqs at boot if we don't have an irq descriptor created
> >yet doesn't sound like a good idea, because we'll lose all
> >interrupts that happen before this driver probes?
> >
> Yes. There were instances of an interrupt storm without this patch.
> There is an RT_STATUS register in the  peripheral address space which
> maintains the real time state and can be read by the client driver
> before it registers for the irq. Few client drivers such as charger
> already doing this.

So you're saying that drivers need to read RT_STATUS to know if
they have a pending interrupt before requesting it? That sounds
bogus.

Kiran Gunda June 14, 2017, 3:04 p.m. UTC | #4

On 2017-06-13 07:41, Stephen Boyd wrote:
> On 06/06, [e-mail] wrote:
>> On 2017-05-31 07:27, Stephen Boyd wrote:
>> >On 05/30, Kiran Gunda wrote:
>> >>From: Abhijeet Dharmapurikar <[e-mail]>
>> >>
>> >>We see a unmapped irqs trigger right around bootup. This could
>> >>likely be because the bootloader exited leaving the interrupts
>> >>in an unknown or unhandled state.  Ack and mask the interrupt
>> >>if one is found. A request_irq later will unmask it and also
>> >>setup proper mapping structures.
>> >
>> >Do we have systems where this is causing an interrupt storm due
>> >to a level high interrupt or something? Just plain acking and
>> >masking irqs at boot if we don't have an irq descriptor created
>> >yet doesn't sound like a good idea, because we'll lose all
>> >interrupts that happen before this driver probes?
>> >
>> Yes. There were instances of an interrupt storm without this patch.
>> There is an RT_STATUS register in the  peripheral address space which
>> maintains the real time state and can be read by the client driver
>> before it registers for the irq. Few client drivers such as charger
>> already doing this.
> 
> So you're saying that drivers need to read RT_STATUS to know if
> they have a pending interrupt before requesting it? That sounds
> bogus.
In  many cases a PMIC module driver does need to know the initial state 
of
the hardware during driver initialization and the RT_STATUS register 
indicates
this (irrespective of the IRQ_EN status).  The  reset value of IRQ_EN = 
0 and if
an event occurs before we request an IRQ, there is no way to know the 
initial state
even after we request this irq as there will be no pending interrupt 
because the
HW default value of IRQ_EN = 0. This can be known only through reading 
the RT_STATUS.

I understand your concern of clearing the IRQ before we request it does 
not seem
a clean way, do you have any other recommendation of this could be 
handled ?
--
To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/spmi/spmi-pmic-arb.c b/drivers/spmi/spmi-pmic-arb.c
index 6320f1f..0a5728c 100644
--- a/drivers/spmi/spmi-pmic-arb.c
+++ b/drivers/spmi/spmi-pmic-arb.c
@@ -98,6 +98,11 @@  enum pmic_arb_cmd_op_code {
 
 struct pmic_arb_ver_ops;
 
+struct apid_data {
+	u16		ppid;
+	u8		owner;
+};
+
 /**
  * spmi_pmic_arb - SPMI PMIC Arbiter object
  *
@@ -115,7 +120,6 @@  enum pmic_arb_cmd_op_code {
  * @mapping_table:	in-memory copy of PPID -> APID mapping table.
  * @domain:		irq domain object for PMIC IRQ domain
  * @spmic:		SPMI controller object
- * @apid_to_ppid:	in-memory copy of APID -> PPID mapping table.
  * @ver_ops:		version dependent operations.
  * @ppid_to_apid	in-memory copy of PPID -> channel (APID) mapping table.
  *			v2 only.
@@ -138,11 +142,10 @@  struct spmi_pmic_arb {
 	DECLARE_BITMAP(mapping_table_valid, PMIC_ARB_MAX_PERIPHS);
 	struct irq_domain	*domain;
 	struct spmi_controller	*spmic;
-	u16			*apid_to_ppid;
 	const struct pmic_arb_ver_ops *ver_ops;
 	u16			*ppid_to_apid;
 	u16			last_apid;
-	u8			*apid_to_owner;
+	struct apid_data	apid_data[PMIC_ARB_MAX_PERIPHS];
 };
 
 /**
@@ -482,6 +485,28 @@  static void qpnpint_spmi_read(struct irq_data *d, u8 reg, void *buf, size_t len)
 				    d->irq);
 }
 
+static void cleanup_irq(struct spmi_pmic_arb *pa, u8 apid, int id)
+{
+	u16 ppid = pa->apid_data[apid].ppid;
+	u8 sid = ppid >> 8;
+	u8 per = ppid & 0xFF;
+	u8 irq_mask = BIT(id);
+
+	writel_relaxed(irq_mask, pa->intr + pa->ver_ops->irq_clear(apid));
+
+	if (pmic_arb_write_cmd(pa->spmic, SPMI_CMD_EXT_WRITEL, sid,
+			(per << 8) + QPNPINT_REG_LATCHED_CLR, &irq_mask, 1))
+		dev_err_ratelimited(&pa->spmic->dev,
+				"failed to ack irq_mask = 0x%x for ppid = %x\n",
+				irq_mask, ppid);
+
+	if (pmic_arb_write_cmd(pa->spmic, SPMI_CMD_EXT_WRITEL, sid,
+			       (per << 8) + QPNPINT_REG_EN_CLR, &irq_mask, 1))
+		dev_err_ratelimited(&pa->spmic->dev,
+				"failed to ack irq_mask = 0x%x for ppid = %x\n",
+				irq_mask, ppid);
+}
+
 static void periph_interrupt(struct spmi_pmic_arb *pa, u8 apid)
 {
 	unsigned int irq;
@@ -493,9 +518,13 @@  static void periph_interrupt(struct spmi_pmic_arb *pa, u8 apid)
 		id = ffs(status) - 1;
 		status &= ~BIT(id);
 		irq = irq_find_mapping(pa->domain,
-				       pa->apid_to_ppid[apid] << 16
+				       pa->apid_data[apid].ppid << 16
 				     | id << 8
 				     | apid);
+		if (irq == 0) {
+			cleanup_irq(pa, apid, id);
+			continue;
+		}
 		generic_handle_irq(irq);
 	}
 }
@@ -530,12 +559,9 @@  static void qpnpint_irq_ack(struct irq_data *d)
 	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
-	unsigned long flags;
 	u8 data;
 
-	raw_spin_lock_irqsave(&pa->lock, flags);
 	writel_relaxed(BIT(irq), pa->intr + pa->ver_ops->irq_clear(apid));
-	raw_spin_unlock_irqrestore(&pa->lock, flags);
 
 	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_LATCHED_CLR, &data, 1);
@@ -543,23 +569,9 @@  static void qpnpint_irq_ack(struct irq_data *d)
 
 static void qpnpint_irq_mask(struct irq_data *d)
 {
-	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
-	u8 apid = d->hwirq;
-	unsigned long flags;
-	u32 status;
-	u8 data;
+	u8 data = BIT(irq);
 
-	raw_spin_lock_irqsave(&pa->lock, flags);
-	status = readl_relaxed(pa->intr + pa->ver_ops->acc_enable(apid));
-	if (status & SPMI_PIC_ACC_ENABLE_BIT) {
-		status = status & ~SPMI_PIC_ACC_ENABLE_BIT;
-		writel_relaxed(status, pa->intr +
-			       pa->ver_ops->acc_enable(apid));
-	}
-	raw_spin_unlock_irqrestore(&pa->lock, flags);
-
-	data = BIT(irq);
 	qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1);
 }
 
@@ -568,20 +580,12 @@  static void qpnpint_irq_unmask(struct irq_data *d)
 	struct spmi_pmic_arb *pa = irq_data_get_irq_chip_data(d);
 	u8 irq  = d->hwirq >> 8;
 	u8 apid = d->hwirq;
-	unsigned long flags;
-	u32 status;
-	u8 data;
+	u8 data = BIT(irq);
 
-	raw_spin_lock_irqsave(&pa->lock, flags);
-	status = readl_relaxed(pa->intr + pa->ver_ops->acc_enable(apid));
-	if (!(status & SPMI_PIC_ACC_ENABLE_BIT)) {
-		writel_relaxed(status | SPMI_PIC_ACC_ENABLE_BIT,
-				pa->intr + pa->ver_ops->acc_enable(apid));
-	}
-	raw_spin_unlock_irqrestore(&pa->lock, flags);
+	writel_relaxed(SPMI_PIC_ACC_ENABLE_BIT,
+		pa->intr + pa->ver_ops->acc_enable(apid));
 
-	data = BIT(irq);
-	qpnpint_spmi_write(d, QPNPINT_REG_EN_SET, &data, 1);
+	qpnpint_spmi_write(d, QPNPINT_REG_EN_CLR, &data, 1);
 }
 
 static void qpnpint_irq_enable(struct irq_data *d)
@@ -755,7 +759,7 @@  static int qpnpint_irq_domain_map(struct irq_domain *d,
 				*apid = SPMI_MAPPING_BIT_IS_1_RESULT(data);
 				pa->ppid_to_apid[ppid]
 					= *apid | PMIC_ARB_CHAN_VALID;
-				pa->apid_to_ppid[*apid] = ppid;
+				pa->apid_data[*apid].ppid = ppid;
 				return 0;
 			}
 		} else {
@@ -765,7 +769,7 @@  static int qpnpint_irq_domain_map(struct irq_domain *d,
 				*apid = SPMI_MAPPING_BIT_IS_0_RESULT(data);
 				pa->ppid_to_apid[ppid]
 					= *apid | PMIC_ARB_CHAN_VALID;
-				pa->apid_to_ppid[*apid] = ppid;
+				pa->apid_data[*apid].ppid = ppid;
 				return 0;
 			}
 		}
@@ -802,7 +806,7 @@  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_to_owner[apid] = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
+		pa->apid_data[apid].owner = SPMI_OWNERSHIP_PERIPH2OWNER(regval);
 
 		offset = PMIC_ARB_REG_CHNL(apid);
 		if (offset >= pa->core_size)
@@ -814,7 +818,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;
+		pa->apid_data[apid].ppid = id;
 		if (id == ppid) {
 			apid |= PMIC_ARB_CHAN_VALID;
 			break;
@@ -846,7 +850,6 @@  static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
 pmic_arb_mode_v2(struct spmi_pmic_arb *pa, u8 sid, u16 addr, mode_t *mode)
 {
 	u8 apid;
-	u8 owner;
 	int rc;
 
 	rc = pmic_arb_ppid_to_apid_v2(pa, sid, addr, &apid);
@@ -856,8 +859,7 @@  static u16 pmic_arb_find_apid(struct spmi_pmic_arb *pa, u16 ppid)
 	*mode = 0;
 	*mode |= S_IRUSR;
 
-	owner = pa->apid_to_owner[apid];
-	if (owner == pa->ee)
+	if (pa->ee == pa->apid_data[apid].owner)
 		*mode |= S_IWUSR;
 	return 0;
 }
@@ -1028,15 +1030,6 @@  static int spmi_pmic_arb_probe(struct platform_device *pdev)
 			err = -ENOMEM;
 			goto err_put_ctrl;
 		}
-
-		pa->apid_to_owner = devm_kcalloc(&ctrl->dev,
-						 pa->max_periph,
-						 sizeof(*pa->apid_to_owner),
-						 GFP_KERNEL);
-		if (!pa->apid_to_owner) {
-			err = -ENOMEM;
-			goto err_put_ctrl;
-		}
 	}
 
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "intr");
@@ -1088,14 +1081,6 @@  static int spmi_pmic_arb_probe(struct platform_device *pdev)
 
 	pa->ee = ee;
 
-	pa->apid_to_ppid = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS,
-					    sizeof(*pa->apid_to_ppid),
-					    GFP_KERNEL);
-	if (!pa->apid_to_ppid) {
-		err = -ENOMEM;
-		goto err_put_ctrl;
-	}
-
 	pa->mapping_table = devm_kcalloc(&ctrl->dev, PMIC_ARB_MAX_PERIPHS - 1,
 					sizeof(*pa->mapping_table), GFP_KERNEL);
 	if (!pa->mapping_table) {