soc/software: add irq_attach() / irq_detach()

[AndrewD]

cleaner mechanism for other software to use interrupts

NOTE: uart_isr(void) not changed to uart_isr(int) for compatibility with cpus that don't implement this interface yet.

This is a different approach to #1807 and #1735

[hvegh]

I like it, thank you very much!

I would add:

void uart_isr_new(int irq) { 
     uart_isr();
}
...
if (irq_attach)
		irq_attach(UART_INTERRUPT, uart_isr_new);
...

so we don't have to rely on the assumption the compiler will drop the irq parameter.

[AndrewD]

Next step is a cleanup pass: i was either going to do something like you proposed or the more complete solution of adjusting the function signature.

One concern with my proposal is that it increases ram usage for all targets by 128 byes. If this is a concern there are some clean ways to offer something like CONFIG_DYNAMIC_IRQ and have a constant table in rodata instead of bss.

[hvegh]

Could we limit the jump table to just the registered IRQ's?
The highest IRQ number can be automatically generated as a define in soc.h, see below.

This will limit ram demand to 4 bytes per interrupt, seams quite reasonable.

diff --git a/litex/soc/integration/soc.py b/litex/soc/integration/soc.py
index 2ed96f46..b71761cc 100644
--- a/litex/soc/integration/soc.py
+++ b/litex/soc/integration/soc.py
@@ -1278,6 +1278,7 @@ class SoC(LiteXModule, SoCCoreCompat):

         # SoC IRQ Interconnect ---------------------------------------------------------------------
         if hasattr(self, "cpu") and hasattr(self.cpu, "interrupt"):
+            self.add_constant("INTERRUPT_END", max(self.irq.locs.values()))
             for name, loc in sorted(self.irq.locs.items()):
                 if name in self.cpu.interrupts.keys():
                     continue

This will add #define INTERRUPT_END 2 to soc.h

[hvegh]

Some considerations for isr(void):

• Assert if irq within NR_IRQ
• NR_IRQ can be generated in soc.py see soc/software: add irq_attach() / irq_detach() #1815 (comment)

[hvegh]

Possibly need wrapper see #1815 (comment)

[AndrewD]

Maybe, or just change the function signature. I just avoided this initially to minimise diffs.

[hvegh]

Generate this define using soc.py #1815 (comment)?

[AndrewD]

Yes - I like that solution.

[hvegh]

Looking good, I have added my comments.

[AndrewD]

I'll make some tweaks soon, just not sure exactly when.

[hvegh]

Hmm just thinking here:

When calling my_isr(irq) the irq parameter is not that useful most of the time, you will already know which IRQ you signed up for. In simple cases where there is an 1-1 mapping beween ISR and the interrupt the irq parameter can be ignored.

Unless ... you register the same interrupt handler for multiple interrupt lines:

Then you will have to scan in your driver data for the structure/context for this particular interrupt.
These scans are costly, more importantly you would want these delay's as deterministic as possible. If you have to cycle trough an array to find the interrupt context, the difference between handling the first interrupt found and the last might be several cycles.

This extra jitter introduced by scanning could be reduced if we introduce a user_data pointer:

• We will leave out the irq parameter since it is not useful in simple cases anyway.
• We will replace the irq parameter with a user_data pointer
• Downside is irq_table will be doubled.
• Upside for generic isr functions:
  • less overhead cycles for generic irq handlers
  • less jitter when handling multiple interrupts within the same handler, typical use case maybe GPIO

We could even go the extra step to initialize a default interrupt handler at compile time removing the need for a NULL check in the main ISR:

Putting it all together:

typedef void (*isr_t)(void *);

Static initialization with a default handler is of course optional might complicate things a from an include perspective. But the nice thing is that it can be done at compile time... and it saves us a NULL check in the main ISR:

// generated by soc.py
void isr_default(void *);

static struct irq_table
{
        isr_t   isr;
        void    *user_data;
} irq_table[NR_IRQ] = {
        { isr_default, (void *)0 },
        { isr_default, (void *)1 },
        { isr_default, (void *)2 },
};

void isr_default(void *data)
{
        unsigned long irq = (unsigned long)data;
        irq_setmask(irq_getmask() & ~(1<<irq));
        printf("\n*** disabled spurious irq %ld ***\n", irq);
}

int irq_attach(unsigned long irq, isr_t isr, void *user_data)
{
     if(isr == NULL)
        return -EINVAL;
     if(irq >= NR_IRQ)
        return -EINVAL;
     irq_table[irq].isr = isr;
     irq_table[irq].user_data = user_data;
}

int irq_detach(unsigned long irq)
{
    return irq_attach(irq, isr_default, (void *)irq);
}

[hvegh]

I forgot the disable the interrupts when updating the irq_table in the above example..

[AndrewD]

Many isr interfaces include both irq and a content pointer. I'm not currently using either, but included irq as it was easy with minimal size implications and was compatible with the cpu interfaces that haven't been updated. It also allows for simple compile time integration if that is a requirement that needs to be maintained.

Yes a context pointer could be useful one day, but chose to not add something i have no need for (currently) in a first pass at this. Following the principal of not letting perfect get in the way of good 😊

Is this something you would actually use? I'm likely to need it at some stage but thia is just a minor infrastructure tweak I want to upstream ASAP to reduce the number of patches I'm maintaining in our trees.

[hvegh]

I must admit, I don't need perfect either ;)
Having an API for ISR registration is the main point. Thank you for the effort!
Lets upstream ASAP!

[AndrewD]

Final cleanup - I've dropped the unused irq parameter to minimise pointless changes. This or a content pointer can be reconsidered in the future.

[enjoy-digital]

Thanks @AndrewD, I just did this change on top of it: edc6871

[AndrewD]

I think NR_IRQ is a hangover from my time following linux coding styles :)