[Sound-open-firmware] [PATCH_V3 1/8] interrupt: refine irq structure and algorithm
rander.wang
rander.wang at linux.intel.com
Fri May 11 04:18:46 CEST 2018
On 5/10/2018 9:44 PM, Jie, Yang wrote:
>> -----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.
as we have discussed, parent connects to xtensa core, child
connects to parent in a link list. only the first
level of irqs is parent which is used to set interrupt in xtensa
core. other irqs are all children of corresponding parent,
and they are linked to the parent. you can refer to Linux kernel,
all the irqs are linked in a list attached to a root irq,
no recursive level.
>
>> 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