[PATCH 0/3] Bug Fixes for PRUSS irqchip driver

[PATCH 0/3] Bug Fixes for PRUSS irqchip driver

[MD Danish Anwar]

Hi All,
This series adds some bug fixes for PRUSS irqchip driver.

Thanks and Regards,
MD Danish Anwar

Grygorii Strashko (1):
  irqchip/irq-pruss-intc: Fix listed IRQ type in /proc/interrupts

Suman Anna (2):
  irqchip/irq-pruss-intc: Fix enabling of intc events
  irqchip/irq-pruss-intc: Fix processing of IEP interrupts

 drivers/irqchip/irq-pruss-intc.c | 47 ++++++++++++++++++++++++++++----
 1 file changed, 41 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH 1/3] irqchip/irq-pruss-intc: Fix enabling of intc events

[MD Danish Anwar]

From: Suman Anna <s-anna@ti.com>

PRUSS INTC events are enabled by default once IRQ events are mapped to
channel:host pair. This may cause issues with undesirable IRQs triggering
even before a PRU IRQ is requested which are silently processed by
pruss_intc_irq_handler().

Fix it by masking all events by default except those which are routed to
various PRU cores (Host IRQs 0, 1; 10 through 19 on K3 SoCs), and any
other reserved IRQs routed to other processors. The unmasking of IRQs is
the responsibility of Linux IRQ core when IRQ is actually requested.

Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/irqchip/irq-pruss-intc.c | 24 ++++++++++++++++++------
 1 file changed, 18 insertions(+), 6 deletions(-)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 0f64ecb9b1f4..7415817d4c6c 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -101,6 +101,7 @@ struct pruss_intc_match_data {
  * @soc_config: cached PRUSS INTC IP configuration data
  * @dev: PRUSS INTC device pointer
  * @lock: mutex to serialize interrupts mapping
+ * @irqs_reserved: bit-mask of reserved host interrupts
  */
 struct pruss_intc {
 	struct pruss_intc_map_record event_channel[MAX_PRU_SYS_EVENTS];
@@ -111,6 +112,7 @@ struct pruss_intc {
 	const struct pruss_intc_match_data *soc_config;
 	struct device *dev;
 	struct mutex lock; /* PRUSS INTC lock */
+	u8 irqs_reserved;
 };
 
 /**
@@ -178,6 +180,7 @@ static void pruss_intc_update_hmr(struct pruss_intc *intc, u8 ch, u8 host)
 static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 {
 	struct device *dev = intc->dev;
+	bool enable_hwirq = false;
 	u8 ch, host, reg_idx;
 	u32 val;
 
@@ -187,6 +190,9 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 
 	ch = intc->event_channel[hwirq].value;
 	host = intc->channel_host[ch].value;
+	enable_hwirq = (host < FIRST_PRU_HOST_INT ||
+			host >= FIRST_PRU_HOST_INT + MAX_NUM_HOST_IRQS ||
+			intc->irqs_reserved & BIT(host - FIRST_PRU_HOST_INT));
 
 	pruss_intc_update_cmr(intc, hwirq, ch);
 
@@ -194,8 +200,10 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 	val = BIT(hwirq  % 32);
 
 	/* clear and enable system event */
-	pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);
 	pruss_intc_write_reg(intc, PRU_INTC_SECR(reg_idx), val);
+	/* unmask only events going to various PRU and other cores by default */
+	if (enable_hwirq)
+		pruss_intc_write_reg(intc, PRU_INTC_ESR(reg_idx), val);
 
 	if (++intc->channel_host[ch].ref_count == 1) {
 		pruss_intc_update_hmr(intc, ch, host);
@@ -204,7 +212,8 @@ static void pruss_intc_map(struct pruss_intc *intc, unsigned long hwirq)
 		pruss_intc_write_reg(intc, PRU_INTC_HIEISR, host);
 	}
 
-	dev_dbg(dev, "mapped system_event = %lu channel = %d host = %d",
+	dev_dbg(dev, "mapped%s system_event = %lu channel = %d host = %d",
+		enable_hwirq ? " and enabled" : "",
 		hwirq, ch, host);
 
 	mutex_unlock(&intc->lock);
@@ -268,11 +277,14 @@ static void pruss_intc_init(struct pruss_intc *intc)
 
 	/*
 	 * configure polarity (SIPR register) to active high and
-	 * type (SITR register) to level interrupt for all system events
+	 * type (SITR register) to level interrupt for all system events,
+	 * and disable and clear all the system events
 	 */
 	for (i = 0; i < num_event_type_regs; i++) {
 		pruss_intc_write_reg(intc, PRU_INTC_SIPR(i), 0xffffffff);
 		pruss_intc_write_reg(intc, PRU_INTC_SITR(i), 0);
+		pruss_intc_write_reg(intc, PRU_INTC_ECR(i), 0xffffffff);
+		pruss_intc_write_reg(intc, PRU_INTC_SECR(i), 0xffffffff);
 	}
 
 	/* clear all interrupt channel map registers, 4 events per register */
@@ -521,7 +533,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
 	struct pruss_intc *intc;
 	struct pruss_host_irq_data *host_data;
 	int i, irq, ret;
-	u8 max_system_events, irqs_reserved = 0;
+	u8 max_system_events;
 
 	data = of_device_get_match_data(dev);
 	if (!data)
@@ -542,7 +554,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
 		return PTR_ERR(intc->base);
 
 	ret = of_property_read_u8(dev->of_node, "ti,irqs-reserved",
-				  &irqs_reserved);
+				  &intc->irqs_reserved);
 
 	/*
 	 * The irqs-reserved is used only for some SoC's therefore not having
@@ -561,7 +573,7 @@ static int pruss_intc_probe(struct platform_device *pdev)
 		return -ENOMEM;
 
 	for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
-		if (irqs_reserved & BIT(i))
+		if (intc->irqs_reserved & BIT(i))
 			continue;
 
 		irq = platform_get_irq_byname(pdev, irq_names[i]);
-- 
2.34.1

[PATCH 2/3] irqchip/irq-pruss-intc: Fix listed IRQ type in /proc/interrupts

[MD Danish Anwar]

From: Grygorii Strashko <grygorii.strashko@ti.com>

The PRUSS INTC driver doesn't have .irq_set_type() callback implemented and
supports only IRQ_TYPE_LEVEL_HIGH. This resulted in the IRQ properties not
being updated properly and the PRUSS INTC IRQs were listed incorrectly in
/proc/interrupts as Edge.

Example:
  218:          0  4b220000.interrupt-controller  26 Edge      pru10

Fix this by adding a simple .irq_set_type() implementation which checks the
requested IRQ triggering type.

Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/irqchip/irq-pruss-intc.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 7415817d4c6c..3cf684ede564 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -373,6 +373,14 @@ static int pruss_intc_irq_set_irqchip_state(struct irq_data *data,
 	return 0;
 }
 
+static int pruss_intc_irq_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	if (type != IRQ_TYPE_LEVEL_HIGH)
+		return -EINVAL;
+
+	return 0;
+}
+
 static struct irq_chip pruss_irqchip = {
 	.name			= "pruss-intc",
 	.irq_ack		= pruss_intc_irq_ack,
@@ -382,6 +390,7 @@ static struct irq_chip pruss_irqchip = {
 	.irq_release_resources	= pruss_intc_irq_relres,
 	.irq_get_irqchip_state	= pruss_intc_irq_get_irqchip_state,
 	.irq_set_irqchip_state	= pruss_intc_irq_set_irqchip_state,
+	.irq_set_type		= pruss_intc_irq_irq_set_type,
 };
 
 static int pruss_intc_validate_mapping(struct pruss_intc *intc, int event,
-- 
2.34.1

[PATCH 3/3] irqchip/irq-pruss-intc: Fix processing of IEP interrupts

[MD Danish Anwar]

From: Suman Anna <s-anna@ti.com>

It was discovered that IEP capture/compare IRQs (event #7 on all SoCs
and event #56 on K3 SoCs) are always triggered twice when PPS is
generated and CMP hit event detected by IEP.

An example of the problem is:
  pruss_intc_irq_handler
   generic_handle_irq
    handle_level_irq
      mask_ack_irq -> IRQ 7 masked and asked in INTC,
                      but it's not yet cleared on HW level
      handle_irq_event()
        <threaded on RT>
           icss_iep_cap_cmp_handler() -> IRQ 7 is actually processed in HW
        irq_finalize_oneshot()
         unmask_irq()
           pruss_intc_irq_unmask() -> IRQ 7 status is still observed as set

The solution is to actually ack these IRQs from pruss_intc_irq_unmask()
after the IRQ source is cleared in HW.

No public errata available for this yet.

Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Suman Anna <s-anna@ti.com>
Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
---
 drivers/irqchip/irq-pruss-intc.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
index 3cf684ede564..9907847dbda8 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -70,6 +70,8 @@
 #define MAX_PRU_SYS_EVENTS 160
 #define MAX_PRU_CHANNELS 20
 
+#define MAX_PRU_INT_EVENTS	64
+
 /**
  * struct pruss_intc_map_record - keeps track of actual mapping state
  * @value: The currently mapped value (channel or host)
@@ -85,10 +87,13 @@ struct pruss_intc_map_record {
  * @num_system_events: number of input system events handled by the PRUSS INTC
  * @num_host_events: number of host events (which is equal to number of
  *		     channels) supported by the PRUSS INTC
+ * @quirky_events: bitmask of events that need quirky IRQ handling (limited to
+ *		   (internal sources only for now, so 64 bits suffice)
  */
 struct pruss_intc_match_data {
 	u8 num_system_events;
 	u8 num_host_events;
+	u64 quirky_events;
 };
 
 /**
@@ -304,6 +309,10 @@ static void pruss_intc_irq_ack(struct irq_data *data)
 	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
 	unsigned int hwirq = data->hwirq;
 
+	if (hwirq < MAX_PRU_INT_EVENTS &&
+	    intc->soc_config->quirky_events & BIT_ULL(hwirq))
+		return;
+
 	pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
 }
 
@@ -320,6 +329,9 @@ static void pruss_intc_irq_unmask(struct irq_data *data)
 	struct pruss_intc *intc = irq_data_get_irq_chip_data(data);
 	unsigned int hwirq = data->hwirq;
 
+	if (hwirq < MAX_PRU_INT_EVENTS &&
+	    intc->soc_config->quirky_events & BIT_ULL(hwirq))
+		pruss_intc_write_reg(intc, PRU_INTC_SICR, hwirq);
 	pruss_intc_write_reg(intc, PRU_INTC_EISR, hwirq);
 }
 
@@ -644,11 +656,13 @@ static int pruss_intc_remove(struct platform_device *pdev)
 static const struct pruss_intc_match_data pruss_intc_data = {
 	.num_system_events = 64,
 	.num_host_events = 10,
+	.quirky_events = BIT_ULL(7), /* IEP capture/compare event */
 };
 
 static const struct pruss_intc_match_data icssg_intc_data = {
 	.num_system_events = 160,
 	.num_host_events = 20,
+	.quirky_events = BIT_ULL(7) | BIT_ULL(56), /* IEP{0,1} capture/compare events */
 };
 
 static const struct of_device_id pruss_intc_of_match[] = {
-- 
2.34.1

Re: [PATCH 3/3] irqchip/irq-pruss-intc: Fix processing of IEP interrupts

[Marc Zyngier]

On Tue, 19 Sep 2023 07:19:00 +0100,
MD Danish Anwar <danishanwar@ti.com> wrote:
> 
> From: Suman Anna <s-anna@ti.com>
> 
> It was discovered that IEP capture/compare IRQs (event #7 on all SoCs
> and event #56 on K3 SoCs) are always triggered twice when PPS is
> generated and CMP hit event detected by IEP.
> 
> An example of the problem is:
>   pruss_intc_irq_handler
>    generic_handle_irq
>     handle_level_irq
>       mask_ack_irq -> IRQ 7 masked and asked in INTC,
>                       but it's not yet cleared on HW level
>       handle_irq_event()
>         <threaded on RT>
>            icss_iep_cap_cmp_handler() -> IRQ 7 is actually processed in HW
>         irq_finalize_oneshot()
>          unmask_irq()
>            pruss_intc_irq_unmask() -> IRQ 7 status is still observed as set
> 
> The solution is to actually ack these IRQs from pruss_intc_irq_unmask()
> after the IRQ source is cleared in HW.

What you don't explain is whether the interrupt is level or edge
triggered? If it is level, then the "quirk" is that the interrupt
controller is slow to recognise that the level has changed. If it is
edge, this is a guaranteed recipe to lose interrupts.

Even if it is level, what happens if you issue a mask/unmask sequence
outside of the interrupt handling and that such an interrupt becomes
pending in between? Does this spurious ack have an effect on the now
pending interrupt?

> 
> No public errata available for this yet.
> 
> Fixes: 04e2d1e06978 ("irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS interrupts")
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>

Nit: drop the empty line after Fixes:.

> Signed-off-by: Suman Anna <s-anna@ti.com>
> Signed-off-by: MD Danish Anwar <danishanwar@ti.com>
> ---
>  drivers/irqchip/irq-pruss-intc.c | 14 ++++++++++++++
>  1 file changed, 14 insertions(+)
> 
> diff --git a/drivers/irqchip/irq-pruss-intc.c b/drivers/irqchip/irq-pruss-intc.c
> index 3cf684ede564..9907847dbda8 100644
> --- a/drivers/irqchip/irq-pruss-intc.c
> +++ b/drivers/irqchip/irq-pruss-intc.c
> @@ -70,6 +70,8 @@
>  #define MAX_PRU_SYS_EVENTS 160
>  #define MAX_PRU_CHANNELS 20
>  
> +#define MAX_PRU_INT_EVENTS	64
> +
>  /**
>   * struct pruss_intc_map_record - keeps track of actual mapping state
>   * @value: The currently mapped value (channel or host)
> @@ -85,10 +87,13 @@ struct pruss_intc_map_record {
>   * @num_system_events: number of input system events handled by the PRUSS INTC
>   * @num_host_events: number of host events (which is equal to number of
>   *		     channels) supported by the PRUSS INTC
> + * @quirky_events: bitmask of events that need quirky IRQ handling (limited to
> + *		   (internal sources only for now, so 64 bits suffice)
>   */
>  struct pruss_intc_match_data {
>  	u8 num_system_events;
>  	u8 num_host_events;
> +	u64 quirky_events;

Why limit this to the first 64 interrupts, while the intc can deal
with 160? What makes you confident that this is solely limited to this
particular source? And why this source only?

	M.

-- 
Without deviation from the norm, progress is not possible.