Re: [PATCHv3 3/6] irqchip/irq-pruss-intc: Add support for shared and invalid interrupts
From: Suman Anna
Date: Sat Jul 25 2020 - 11:58:07 EST
Hi Marc,
On 7/17/20 6:02 AM, Marc Zyngier wrote:
On Fri, 10 Jul 2020 21:59:17 +0100,
Suman Anna <s-anna@xxxxxx> wrote:
Hi Suman,
[...]
Hi Marc,
On 7/2/20 12:44 PM, Marc Zyngier wrote:
On 2020-07-02 15:17, Grzegorz Jaszczyk wrote:
From: Suman Anna <s-anna@xxxxxx>
The PRUSS INTC has a fixed number of output interrupt lines that are
connected to a number of processors or other PRUSS instances or other
devices (like DMA) on the SoC. The output interrupt lines 2 through 9
are usually connected to the main Arm host processor and are referred
to as host interrupts 0 through 7 from ARM/MPU perspective.
All of these 8 host interrupts are not always exclusively connected
to the Arm interrupt controller. Some SoCs have some interrupt lines
not connected to the Arm interrupt controller at all, while a few others
have the interrupt lines connected to multiple processors in which they
need to be partitioned as per SoC integration needs. For example, AM437x
and 66AK2G SoCs have 2 PRUSS instances each and have the host interrupt 5
connected to the other PRUSS, while AM335x has host interrupt 0 shared
between MPU and TSC_ADC and host interrupts 6 & 7 shared between MPU and
a DMA controller.
Add support to the PRUSS INTC driver to allow both these shared and
invalid interrupts by not returning a failure if any of these interrupts
are skipped from the corresponding INTC DT node.
That's not exactly "adding support", is it? It really is "ignore these
interrupts because they are useless from the main CPU's perspective",
right?
Correct. We can rephrase this to something like
"Add logic to the PRUSS INTC driver to ignore.."
Signed-off-by: Suman Anna <s-anna@xxxxxx>
Signed-off-by: Grzegorz Jaszczyk <grzegorz.jaszczyk@xxxxxxxxxx>
---
v2->v3:
- Extra checks for (intc->irqs[i]) in error/remove path was moved from
 "irqchip/irq-pruss-intc: Add a PRUSS irqchip driver for PRUSS
 interrupts" to this patch
v1->v2:
- https://patchwork.kernel.org/patch/11069757/
---
Âdrivers/irqchip/irq-pruss-intc.c | 73
+++++++++++++++++++++++++++++++++++++---
Â1 file changed, 68 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-pruss-intc.c
b/drivers/irqchip/irq-pruss-intc.c
index fb3dda3..49c936f 100644
--- a/drivers/irqchip/irq-pruss-intc.c
+++ b/drivers/irqchip/irq-pruss-intc.c
@@ -65,11 +65,15 @@
 * @irqs: kernel irq numbers corresponding to PRUSS host interrupts
 * @base: base virtual address of INTC register space
 * @domain: irq domain for this interrupt controller
+ * @shared_intr: bit-map denoting if the MPU host interrupt is shared
nit: bitmap
ok
+ * @invalid_intr: bit-map denoting if host interrupt is not
connected to MPU
 */
Âstruct pruss_intc {
ÂÂÂÂ unsigned int irqs[MAX_NUM_HOST_IRQS];
ÂÂÂÂ void __iomem *base;
ÂÂÂÂ struct irq_domain *domain;
+ÂÂÂ u16 shared_intr;
+ÂÂÂ u16 invalid_intr;
Please represent bitmaps as an unsigned long.
ok. We have atmost 8 interrupts coming in, but agree on the change
since we are using the BIT() macro below.
Â};
Âstatic inline u32 pruss_intc_read_reg(struct pruss_intc *intc,
unsigned int reg)
@@ -222,7 +226,8 @@ static int pruss_intc_probe(struct
platform_device *pdev)
ÂÂÂÂÂÂÂÂ "host_intr4", "host_intr5", "host_intr6", "host_intr7", };
ÂÂÂÂ struct device *dev = &pdev->dev;
ÂÂÂÂ struct pruss_intc *intc;
-ÂÂÂ int i, irq;
+ÂÂÂ int i, irq, count;
+ÂÂÂ u8 temp_intr[MAX_NUM_HOST_IRQS] = { 0 };
ÂÂÂÂ intc = devm_kzalloc(dev, sizeof(*intc), GFP_KERNEL);
ÂÂÂÂ if (!intc)
@@ -235,6 +240,52 @@ static int pruss_intc_probe(struct
platform_device *pdev)
ÂÂÂÂÂÂÂÂ return PTR_ERR(intc->base);
ÂÂÂÂ }
+ÂÂÂ count = of_property_read_variable_u8_array(dev->of_node,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "ti,irqs-reserved",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ temp_intr, 0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MAX_NUM_HOST_IRQS);
+ÂÂÂ /*
+ÂÂÂÂ * The irqs-reserved is used only for some SoC's therefore not
having
+ÂÂÂÂ * this property is still valid
+ÂÂÂÂ */
+ÂÂÂ if (count == -EINVAL)
+ÂÂÂÂÂÂÂ count = 0;
+ÂÂÂ if (count < 0)
+ÂÂÂÂÂÂÂ return count;
+
+ÂÂÂ for (i = 0; i < count; i++) {
+ÂÂÂÂÂÂÂ if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_warn(dev, "ignoring invalid reserved irq %d\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ temp_intr[i]);
+ÂÂÂÂÂÂÂÂÂÂÂ continue;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ intc->invalid_intr |= BIT(temp_intr[i]);
+ÂÂÂ }
+
+ÂÂÂ count = of_property_read_variable_u8_array(dev->of_node,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ "ti,irqs-shared",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ temp_intr, 0,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ MAX_NUM_HOST_IRQS);
+ÂÂÂ /*
+ÂÂÂÂ * The irqs-shared is used only for some SoC's therefore not having
+ÂÂÂÂ * this property is still valid
+ÂÂÂÂ */
+ÂÂÂ if (count == -EINVAL)
+ÂÂÂÂÂÂÂ count = 0;
+ÂÂÂ if (count < 0)
+ÂÂÂÂÂÂÂ return count;
+
+ÂÂÂ for (i = 0; i < count; i++) {
+ÂÂÂÂÂÂÂ if (temp_intr[i] >= MAX_NUM_HOST_IRQS) {
+ÂÂÂÂÂÂÂÂÂÂÂ dev_warn(dev, "ignoring invalid shared irq %d\n",
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ temp_intr[i]);
+ÂÂÂÂÂÂÂÂÂÂÂ continue;
+ÂÂÂÂÂÂÂ }
+
+ÂÂÂÂÂÂÂ intc->shared_intr |= BIT(temp_intr[i]);
+ÂÂÂ }
+
You probably want to move this in a separate function, since you populate a
common structure.
ÂÂÂÂ pruss_intc_init(intc);
ÂÂÂÂ /* always 64 events */
@@ -244,8 +295,14 @@ static int pruss_intc_probe(struct
platform_device *pdev)
ÂÂÂÂÂÂÂÂ return -ENOMEM;
ÂÂÂÂ for (i = 0; i < MAX_NUM_HOST_IRQS; i++) {
+ÂÂÂÂÂÂÂ if (intc->invalid_intr & BIT(i))
+ÂÂÂÂÂÂÂÂÂÂÂ continue;
+
ÂÂÂÂÂÂÂÂ irq = platform_get_irq_byname(pdev, irq_names[i]);
ÂÂÂÂÂÂÂÂ if (irq <= 0) {
+ÂÂÂÂÂÂÂÂÂÂÂ if (intc->shared_intr & BIT(i))
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
I don't really understand why you are treating these "shared" interrupts
differently from the invalid ones. In all cases, they shouldn't be used.
The behavior is the same in how we handle it, but the difference is
that an "invalid" one is never even connected to the ARM interrupt
controller, while the "shared" one is a choice. So, unless this
interrupt is being used/handled by a different processor/entity, you
would not see this skipped from the dts node.
And I'm saying that all that matters is that you are discarding these
interrupts. Whether they are flagged invalid or shared, they are not
available to Linux. So the difference in handling is pointless and
only makes it harder to understand what you are doing.
The primary reason for using two properties and this logic was to
accurately describe the h/w and usage of these in the DT bindings to
distinguish the "never connected" vs the "optionally can be skipped"
interrupts rather than go by how these are handled in the driver. I feel
we will loose this description and make it confusing for SoC product
integration developers.
Greg is planning to consolidate these for the next version. It would
have been nice if we could have retained them.
regards
Suman