[Sound-open-firmware] [PATCH_V3 1/8] interrupt: refine irq structure and algorithm
Jie, Yang
yang.jie at intel.com
Thu May 10 15:44:05 CEST 2018
>-----Original Message-----
>From: sound-open-firmware-bounces at alsa-project.org [mailto:sound-open-
>firmware-bounces at alsa-project.org] On Behalf Of Rander Wang
>Sent: Thursday, May 10, 2018 1:32 PM
>To: sound-open-firmware at alsa-project.org; marcin.maka at linux.intel.com
>Cc: Liam Girdwood <liam.r.girdwood at linux.intel.com>; Rander Wang
><rander.wang at linux.intel.com>
>Subject: [Sound-open-firmware] [PATCH_V3 1/8] interrupt: refine irq structure
>and algorithm
>
>The issue for original data structure and algorithm is that:
>(1)it only support 2 level interrupt architecture.
>(2)it doesn't support two HW interrupt triggered by one irq pin.
>
>Unify irq parent and child to irq_desc. Now each irq_desc maybe directly attach
>to xtensa core or another irq_desc.Each irq_desc may have 32 child list instead of
>32 child.
>
>---
>V2: check whether memory allocation failed
>V3: remove useless declaration in interrupt.h
>
>test on CNL & APL & BYT, pass
>SOF: master c1f2682c210201
>kernel: v4.14 d09db67c5a9d6d
>SOF-tools: master 13b56fa6047c566a
>
>Signed-off-by: Rander Wang <rander.wang at linux.intel.com>
>Signed-off-by: Liam Girdwood <liam.r.girdwood at linux.intel.com>
>---
> src/include/sof/interrupt.h | 29 +++++-----
> src/lib/interrupt.c | 126 +++++++++++++++++++++++++-------------------
> 2 files changed, 83 insertions(+), 72 deletions(-)
>
>diff --git a/src/include/sof/interrupt.h b/src/include/sof/interrupt.h index
>2dc8954..bbda635 100644
>--- a/src/include/sof/interrupt.h
>+++ b/src/include/sof/interrupt.h
>@@ -37,27 +37,29 @@
> #include <sof/trace.h>
> #include <sof/debug.h>
> #include <sof/lock.h>
>+#include <sof/list.h>
>
> #define trace_irq(__e) trace_event(TRACE_CLASS_IRQ, __e)
> #define trace_irq_error(__e) trace_error(TRACE_CLASS_IRQ, __e)
>
>-/* child interrupt source */
>-struct irq_child {
>- uint32_t enabled;
>+struct irq_desc {
>+ /* irq must be first for constructor */
>+ int irq; /* logical IRQ number */
>
>+ /* handler is optional for constructor */
> void (*handler)(void *arg);
> void *handler_arg;
>-};
>
>-/* parent source */
>-struct irq_parent {
>- int num;
>- void (*handler)(void *arg);
>- uint32_t enabled_count;
>+ /* to identify interrupt with the same IRQ */
>+ int id;
> spinlock_t lock;
>+ uint32_t enabled_count;
>+
>+ /* to link to other irq_desc */
>+ struct list_item irq_list;
>
> uint32_t num_children;
>- struct irq_child *child[PLATFORM_IRQ_CHILDREN];
>+ struct list_item child[PLATFORM_IRQ_CHILDREN];
> };
>
> int interrupt_register(uint32_t irq,
>@@ -86,11 +88,4 @@ static inline void interrupt_global_enable(uint32_t flags)
> arch_interrupt_global_enable(flags);
> }
>
>-/* called by platform interrupt ops */
>-int irq_register_child(struct irq_parent *parent, int irq,
>- void (*handler)(void *arg), void *arg);
>-void irq_unregister_child(struct irq_parent *parent, int irq); -uint32_t
>irq_enable_child(struct irq_parent *parent, int irq); -uint32_t
>irq_disable_child(struct irq_parent *parent, int irq);
>-
> #endif
>diff --git a/src/lib/interrupt.c b/src/lib/interrupt.c index e8e2899..a1f22eb 100644
>--- a/src/lib/interrupt.c
>+++ b/src/lib/interrupt.c
>@@ -38,127 +38,143 @@
> #include <stdint.h>
> #include <stdlib.h>
>
>-int irq_register_child(struct irq_parent *parent, int irq,
>- void (*handler)(void *arg), void *arg)
>+static int irq_register_child(struct irq_desc *parent, int irq,
>+ void (*handler)(void *arg), void *arg); static void
>+irq_unregister_child(struct irq_desc *parent, int irq); static uint32_t
>+irq_enable_child(struct irq_desc *parent, int irq); static uint32_t
>+irq_disable_child(struct irq_desc *parent, int irq);
>+
>+static int irq_register_child(struct irq_desc *parent, int irq,
>+ void (*handler)(void *arg), void *arg)
> {
> int ret = 0;
>+ struct irq_desc *child;
>
> if (parent == NULL)
> return -EINVAL;
>
> spin_lock(&parent->lock);
>
>- /* does child already exist ? */
>- if (parent->child[SOF_IRQ_BIT(irq)]) {
>- /* already registered, return */
>+ /* init child */
>+ child = rzalloc(RZONE_SYS, SOF_MEM_CAPS_RAM,
>+ sizeof(struct irq_desc));
>+ if (!child) {
>+ ret = -ENOMEM;
> goto finish;
> }
>
>- /* init child */
>- parent->child[SOF_IRQ_BIT(irq)] =
>- rzalloc(RZONE_SYS, SOF_MEM_CAPS_RAM,
>- sizeof(struct irq_child));
>- parent->child[SOF_IRQ_BIT(irq)]->enabled = 0;
>- parent->child[SOF_IRQ_BIT(irq)]->handler = handler;
>- parent->child[SOF_IRQ_BIT(irq)]->handler_arg = arg;
>+ child->enabled_count = 0;
>+ child->handler = handler;
>+ child->handler_arg = arg;
>+ child->id = SOF_IRQ_ID(irq);
>+
>+ list_item_append(&child->irq_list, &parent->child[SOF_IRQ_BIT(irq)]);
>
> /* do we need to register parent ? */
> if (parent->num_children == 0) {
>- ret = arch_interrupt_register(parent->num,
>- parent->handler, parent);
>+ ret = arch_interrupt_register(parent->irq,
>+ parent->handler, parent);
> }
>
> /* increment number of children */
>- parent->num_children += 1;
>+ parent->num_children++;
>
> finish:
> spin_unlock(&parent->lock);
> return ret;
>-
> }
>
>-void irq_unregister_child(struct irq_parent *parent, int irq)
>+static void irq_unregister_child(struct irq_desc *parent, int irq)
> {
> spin_lock(&parent->lock);
>+ struct irq_desc *child;
>+ struct list_item *clist;
>
> /* does child already exist ? */
>- if (!parent->child[SOF_IRQ_BIT(irq)])
>+ if (list_is_empty(&parent->child[SOF_IRQ_BIT(irq)]))
> goto finish;
>
>- /* free child */
>- parent->num_children -= 1;
>- rfree(parent->child[SOF_IRQ_BIT(irq)]);
>- parent->child[SOF_IRQ_BIT(irq)] = NULL;
>+ list_for_item(clist, &parent->child[SOF_IRQ_BIT(irq)]) {
>+ child = container_of(clist, struct irq_desc, irq_list);
>+
>+ if (SOF_IRQ_ID(irq) == child->id) {
>+ list_item_del(&child->irq_list);
>+ rfree(child);
>+ parent->num_children--;
>+ }
>+ }
>
> /*
> * unregister the root interrupt if the this l2 is
> * the last registered one.
> */
> if (parent->num_children == 0)
>- arch_interrupt_unregister(parent->num);
>+ arch_interrupt_unregister(parent->irq);
Is it possible here the parent also has parent? If so, we need a recursive calling here.
>
> finish:
> spin_unlock(&parent->lock);
> }
>
>-uint32_t irq_enable_child(struct irq_parent *parent, int irq)
>+static uint32_t irq_enable_child(struct irq_desc *parent, int irq)
> {
>- struct irq_child *child;
>+ struct irq_desc *child;
>+ struct list_item *clist;
>
> spin_lock(&parent->lock);
>
>- child = parent->child[SOF_IRQ_BIT(irq)];
>-
>- /* already enabled ? */
>- if (child->enabled)
>- goto finish;
>-
> /* enable the parent interrupt */
> if (parent->enabled_count == 0)
> arch_interrupt_enable_mask(1 << SOF_IRQ_NUMBER(irq));
>- child->enabled = 1;
>- parent->enabled_count++;
>
>- /* enable the child interrupt */
>- platform_interrupt_unmask(irq, 0);
>+ list_for_item(clist, &parent->child[SOF_IRQ_BIT(irq)]) {
>+ child = container_of(clist, struct irq_desc, irq_list);
>+
>+ if ((SOF_IRQ_ID(irq) == child->id) &&
>+ !child->enabled_count) {
>+ child->enabled_count = 1;
Is child here has its child possible?
>+ parent->enabled_count++;
>+
>+ /* enable the child interrupt */
>+ platform_interrupt_unmask(irq, 0);
>+ }
>+ }
>
>-finish:
> spin_unlock(&parent->lock);
> return 0;
>
> }
>
>-uint32_t irq_disable_child(struct irq_parent *parent, int irq)
>+static uint32_t irq_disable_child(struct irq_desc *parent, int irq)
> {
>- struct irq_child *child;
>+ struct irq_desc *child;
>+ struct list_item *clist;
>
> spin_lock(&parent->lock);
>
>- child = parent->child[SOF_IRQ_BIT(irq)];
>-
>- /* already disabled ? */
>- if (!child->enabled)
>- goto finish;
>+ list_for_item(clist, &parent->child[SOF_IRQ_BIT(irq)]) {
>+ child = container_of(clist, struct irq_desc, irq_list);
>
>- /* disable the child interrupt */
>- platform_interrupt_mask(irq, 0);
>- child->enabled = 0;
>+ if ((SOF_IRQ_ID(irq) == child->id) &&
>+ child->enabled_count) {
>+ child->enabled_count = 0;
Same question here.
Thanks,
~Keyon
>+ parent->enabled_count--;
>+ }
>+ }
>
>- /* disable the parent interrupt */
>- parent->enabled_count--;
>- if (parent->enabled_count == 0)
>+ if (parent->enabled_count == 0) {
>+ /* disable the child interrupt */
>+ platform_interrupt_mask(irq, 0);
> arch_interrupt_disable_mask(1 << SOF_IRQ_NUMBER(irq));
>+ }
>
>-finish:
> spin_unlock(&parent->lock);
> return 0;
>-
> }
>
> int interrupt_register(uint32_t irq,
> void (*handler)(void *arg), void *arg) {
>- struct irq_parent *parent;
>+ struct irq_desc *parent;
>
> /* no parent means we are registering DSP internal IRQ */
> parent = platform_irq_get_parent(irq); @@ -170,7 +186,7 @@ int
>interrupt_register(uint32_t irq,
>
> void interrupt_unregister(uint32_t irq) {
>- struct irq_parent *parent;
>+ struct irq_desc *parent;
>
> /* no parent means we are unregistering DSP internal IRQ */
> parent = platform_irq_get_parent(irq); @@ -182,7 +198,7 @@ void
>interrupt_unregister(uint32_t irq)
>
> uint32_t interrupt_enable(uint32_t irq) {
>- struct irq_parent *parent;
>+ struct irq_desc *parent;
>
> /* no parent means we are enabling DSP internal IRQ */
> parent = platform_irq_get_parent(irq); @@ -194,7 +210,7 @@ uint32_t
>interrupt_enable(uint32_t irq)
>
> uint32_t interrupt_disable(uint32_t irq) {
>- struct irq_parent *parent;
>+ struct irq_desc *parent;
>
> /* no parent means we are disabling DSP internal IRQ */
> parent = platform_irq_get_parent(irq);
>--
>2.14.1
>
>_______________________________________________
>Sound-open-firmware mailing list
>Sound-open-firmware at alsa-project.org
>http://mailman.alsa-project.org/mailman/listinfo/sound-open-firmware
More information about the Sound-open-firmware
mailing list