Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add architectural support for shared interrupts #61278

Closed
LaurentiuM1234 opened this issue Aug 8, 2023 · 4 comments
Closed

Add architectural support for shared interrupts #61278

LaurentiuM1234 opened this issue Aug 8, 2023 · 4 comments
Labels
area: Interrupt Controller RFC Request For Comments: want input from the community

Comments

@LaurentiuM1234
Copy link
Collaborator

Problem description

Currently, there seems to be no architectural support for shared interrupts. As such, in the following sequence:

  1. irq_connect_dynamic(33, INTID_33_PRIO, some_handler, some_argument);
  2. irq_connect_dynamic(33, INTID_33_PRIO, some_other_handler, some_other_argument);

some_other_handler would overwrite some_handler. Currently there seem to be 2 ways to solve this:

  1. Make use of the intc_shared_irq.c interrupt controller => doesn't scale very well, seems to require a DT node per shared IRQ. Problematic on platforms which have multiple shared IRQs (e.g: i.MX8QM which has the same INTID for groups of x channels)
  2. Handle this yourself in your driver/application => somewhat difficult and annoying in situations such as the one described above. Needs to be done per module which uses a shared IRQ.

Proposed change

As as a way to solve the problem described above, we could add architectural support for shared interrupts. This could be achieved by simply registering a "dummy" ISR for an INTID and calling all registered ISRs for that INTID.

Detailed RFC

More specifically, if the SHARED_IRQ feature is enabled, we could overwrite z_isr_install() such that instead of registering the passed routine and argument in the ISR table, we could register our own "dummy" ISR and pass it a list of routine/arg pairs that it needs to invoke.

The following is a draft of how this could be achieved:

struct shared_irq_client {
    void (*routine)(const void *arg);
    const void *arg;
};

struct shared_irq_data {
    bool enabled;
    struct shared_irq_client clients[CONFIG_SHARED_IRQ_MAX_NUM_CLIENTS];
    uint32_t client_num;
};

static struct shared_irq_data shared_irq_table[CONFIG_NUM_IRQS];

void shared_irq_isr(const void *data)
{
    struct shared_irq_data *irq_data = data;
    int i;

    for (i = 0; i < irq_data->client_num; i++)
        irq_data->clients[i].routine(irq_data->clients[i].arg);
}

/* note: this definition of z_isr_install would overwrite the definition from sw_isr_common.c */
void z_isr_install(unsigned int irq, void (*routine)(const void *), const void *param)
{
    ... some code goes here ...
    /* note: this is only done once per INTID, upon enabling it for the first time */
    _sw_isr_table[table_idx].arg = irq_data;
    _sw_isr_table[table_idx].isr = shared_irq_isr;

    ... some code goes here ...
    /* irq_data points to the entry in shared_irq_table corresponding to passed IRQ */
    irq_data->clients[irq_data->client_num].routine = routine;
    irq_data->clients[irq_data->client_num].arg = param;
    irq_data->client_num++;
}

Dependencies

This feature would be something one can enable/disable with a configuration. As such, if CONFIG_SHARED_INTERRUPTS=y, z_isr_install would have the above definition. Otherwise, the default definition from sw_isr_common.c will be used. In theory there should be no dependencies.

Would this change be something desired by the community? If so, I can submit a PR after I have something functional.

@LaurentiuM1234 LaurentiuM1234 added the RFC Request For Comments: want input from the community label Aug 8, 2023
@github-actions
Copy link

github-actions bot commented Aug 8, 2023

Hi @LaurentiuM1234! We appreciate you submitting your first issue for our open-source project. 🌟

Even though I'm a bot, I can assure you that the whole community is genuinely grateful for your time and effort. 🤖💙

@lowlander
Copy link
Collaborator

With my current project I actually have a real world usecase for this. I need 3 i2c ports on the STM32G0B1, and ports 2 and 3 share IRQs. Using non-IRQ mode is not possible because port 2 needs to run in slave mode (which forces the needs for IRQs). Also I'll need the two CAN ports, and they also shared IRQ's.

So I hope the PR #61422 to this issue lands in main soon :-)

@dbaluta
Copy link
Collaborator

dbaluta commented Aug 25, 2023

@lowlander yes, this should land in zephyr once Laurentiu gets back from vacation next week. I wonder if you could do some testing and provide your feedback.

@LaurentiuM1234
Copy link
Collaborator Author

Feature was addressed in #61422, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: Interrupt Controller RFC Request For Comments: want input from the community
Projects
Status: Done
Development

No branches or pull requests

4 participants