[Sound-open-firmware] [PATCH 2/4] interrupt: enable layer 2 interrupt support
Keyon Jie
yang.jie at linux.intel.com
Wed Sep 20 12:44:52 CEST 2017
On 2017年09月20日 17:57, Liam Girdwood wrote:
> On Wed, 2017-09-20 at 17:42 +0800, Keyon Jie wrote:
>>
>> On 2017年09月19日 22:53, Liam Girdwood wrote:
>>> On Tue, 2017-09-19 at 15:47 +0800, Keyon Jie wrote:
>>>> Make it possible to support layer 2 (e.g. external level 2/3/4,
>>>> which may include up to 16/32 sources) interrupt, which may be
>>>> used for timer/IPC/DMAC/SSP...
>>>>
>>>> Signed-off-by: Keyon Jie <yang.jie at linux.intel.com>
>>>> ---
>>>> src/drivers/dw-dma.c | 6 +-
>>>> src/include/reef/interrupt.h | 86 ++++++++++--------
>>>> src/lib/Makefile.am | 3 +-
>>>> src/lib/interrupt.c | 210 +++++++++++++++++++++++++++++++++++++++++++
>>>> 4 files changed, 264 insertions(+), 41 deletions(-)
>>>> create mode 100644 src/lib/interrupt.c
>>>>
>>>> diff --git a/src/drivers/dw-dma.c b/src/drivers/dw-dma.c
>>>> index e851e54..1020f66 100644
>>>> --- a/src/drivers/dw-dma.c
>>>> +++ b/src/drivers/dw-dma.c
>>>> @@ -330,7 +330,8 @@ static int dw_dma_start(struct dma *dma, int channel)
>>>> dw_write(dma, DW_CLEAR_ERR, 0x1 << channel);
>>>>
>>>> /* clear platform interrupt */
>>>> - platform_interrupt_clear(dma_irq(dma), 1 << channel);
>>>> + if (plf_irq_ops && plf_irq_ops->int_clear)
>>>> + plf_irq_ops->int_clear(dma_irq(dma), 1 << channel);
>>>
>>> This should still use platform_irq_clear(). platform_irq_clear will then
>>> do any ops if necessary.
>>
>> platform_irq_clear() is actually put into plf_irq_ops->int_clear, for
>> platforms which don't need clear, the int_clear may be set NULL.
>>
>
> Should be the other way around. platform_irq_clear() will wrap this
> 12st/2nd level logic since it only used on certain platforms.
I got your point now. let me remove this plf_irq_ops struct and only use
platform_irq_xxx() for all platforms.
>
>>>
>>>>
>>>> #if DW_USE_HW_LLI
>>>> /* TODO: Revisit: are we using LLP mode or single transfer ? */
>>>> @@ -829,7 +830,8 @@ static void dw_dma_irq_handler(void *data)
>>>>
>>>> /* clear platform and DSP interrupt */
>>>> pmask = status_block | status_tfr | status_err;
>>>> - platform_interrupt_clear(dma_irq(dma), pmask);
>>>> + if (plf_irq_ops && plf_irq_ops->int_clear)
>>>> + plf_irq_ops->int_clear(dma_irq(dma), pmask);
>>>>
>>>> for (i = 0; i < DW_MAX_CHAN; i++) {
>>>>
>>>> diff --git a/src/include/reef/interrupt.h b/src/include/reef/interrupt.h
>>>> index 3c78e77..8a9b0b6 100644
>>>> --- a/src/include/reef/interrupt.h
>>>> +++ b/src/include/reef/interrupt.h
>>>> @@ -35,53 +35,63 @@
>>>> #include <arch/interrupt.h>
>>>> #include <reef/trace.h>
>>>> #include <reef/debug.h>
>>>> +#include <reef/list.h>
>>>>
>>>> #define trace_irq(__e) trace_event(TRACE_CLASS_IRQ | __e)
>>>>
>>>> -static inline int interrupt_register(int irq,
>>>> - void(*handler)(void *arg), void *arg)
>>>> -{
>>>> - return arch_interrupt_register(irq, handler, arg);
>>>> -}
>>>> +typedef void (irq2_handler)(void *arg);
>>>> +typedef void (irq_num_handler)(void *arg);
>>>>
>>>> -static inline void interrupt_unregister(int irq)
>>>> -{
>>>> - arch_interrupt_unregister(irq);
>>>> -}
>>>> +/* source item under L2 interrupt aggregator */
>>>> +struct l2src_irq {
>>>> + uint32_t bit_id;
>>>> + uint32_t enabled;
>>>> + irq2_handler *handler;
>>>> + void *handler_arg;
>>>> + /* list in the registered irq of the same number */
>>>> + struct list_item list;
>>>> +};
>>>>
>>>> -static inline uint32_t interrupt_enable(uint32_t irq)
>>>> -{
>>>> - return arch_interrupt_enable_mask(1 << irq);
>>>> -}
>>>> +/* root interrupt item which has Level 2 sources */
>>>
>>> The naming "root" implies it's the level 1 IRQ ?
>>
>> yep.
>>
>>>
>>>> +struct l2root_irq {
>>>> + int num;
>>>> + irq_num_handler *handler;
>>>> + uint32_t src_count;
>>>> + uint32_t enabled_count;
>>>> + /* list in the registered l2 irq of the same number */
>>>> + struct list_item src_list;
>>>> +};
>>>>
>>>> -static inline uint32_t interrupt_disable(uint32_t irq)
>>>> -{
>>>> - return arch_interrupt_disable_mask(1 <<irq);
>>>> -}
>>>> +/* platform specific irq ops */
>>>> +struct plf_irq_ops {
>>>> + int (*int_register)(uint32_t irq, void(*handler)(void *a src/lib/Makefile.am | 3 +-rg), void *arg);
>>>> + void (*int_unregister)(uint32_t irq);
>>>> + uint32_t (*int_enable)(uint32_t irq);
>>>> + uint32_t (*int_disable)(uint32_t irq);
>>>>
>>>> -static inline void interrupt_set(int irq)
>>>> -{
>>>> - arch_interrupt_set(irq);
>>>> -}
>>>> + uint32_t (*get_enabled)(void);
>>>> + void (*int_clear)(uint32_t irq, uint32_t mask);
>>>> + void (*int_mask)(uint32_t irq);
>>>> + void (*int_unmask)(uint32_t irq);
>>>> +};
>>>>
>>>> -static inline void interrupt_clear(int irq)
>>>> -{
>>>> - arch_interrupt_clear(irq);
>>>> -}
>>>> +extern const struct plf_irq_ops *plf_irq_ops;
>>>>
>>>> -static inline uint32_t interrupt_global_disable(void)
>>>> -{
>>>> - return arch_interrupt_global_disable();
>>>> -}
>>>> +/* uniformed interrupt API called outside interrupt */
>>>> +int interrupt_register(int irq, void(*handler)(void *arg), void *arg);
>>>> +void interrupt_unregister(int irq);
>>>> +uint32_t interrupt_enable(uint32_t irq);
>>>> +uint32_t interrupt_disable(uint32_t irq);
>>>> +void interrupt_set(int irq);
>>>> +void interrupt_clear(int irq);
>>>> +uint32_t interrupt_global_disable(void);
>>>> +void interrupt_global_enable(uint32_t flags);
>>>>
>>>> -static inline void interrupt_global_enable(uint32_t flags)
>>>> -{
>>>> - arch_interrupt_global_enable(flags);
>>>> -}
>>>> -
>>>> -uint32_t platform_interrupt_get_enabled();
>>>> -void platform_interrupt_clear(uint32_t irq, uint32_t mask);
>>>> -void platform_interrupt_mask(uint32_t irq, uint32_t mask);
>>>> -void platform_interrupt_unmask(uint32_t irq, uint32_t mask);
>>>> +/* called by platform interrupt ops*/
>>>> +int irq_l2_register(int irq, struct l2root_irq *l2_int,
>>>> + void (*handler)(void *arg), void *arg);
>>>> +void irq_l2_unregister(int irq, struct l2root_irq *l2_int);
>>>> +uint32_t irq_l2_enable(int irq, struct l2root_irq *l2_int);
>>>> +uint32_t irq_l2_disable(int irq, struct l2root_irq *l2_int);
>>>>
>>>> #endif
>>>> diff --git a/src/lib/Makefile.am b/src/lib/Makefile.am
>>>> index acdbd85..9923467 100644
>>>> --- a/src/lib/Makefile.am
>>>> +++ b/src/lib/Makefile.am
>>>> @@ -6,7 +6,8 @@ libcore_a_SOURCES = \
>>>> work.c \
>>>> notifier.c \
>>>> trace.c \
>>>> - schedule.c
>>>> + schedule.c \
>>>> + interrupt.c
>>>>
>>>> libcore_a_CFLAGS = \
>>>> $(ARCH_CFLAGS) \
>>>> diff --git a/src/lib/interrupt.c b/src/lib/interrupt.c
>>>> new file mode 100644
>>>> index 0000000..a7f8c9f
>>>> --- /dev/null
>>>> +++ b/src/lib/interrupt.c
>>>> @@ -0,0 +1,210 @@
>>>> +/*
>>>> + * Copyright (c) 2017, Intel Corporation
>>>> + * All rights reserved.
>>>> + *
>>>> + * Redistribution and use in source and binary forms, with or without
>>>> + * modification, are permitted provided that the following conditions are met:
>>>> + * * Redistributions of source code must retain the above copyright
>>>> + * notice, this list of conditions and the following disclaimer.
>>>> + * * Redistributions in binary form must reproduce the above copyright
>>>> + * notice, this list of conditions and the following disclaimer in the
>>>> + * documentation and/or other materials provided with the distribution.
>>>> + * * Neither the name of the Intel Corporation nor the
>>>> + * names of its contributors may be used to endorse or promote products
>>>> + * derived from this software without specific prior written permission.
>>>> + *
>>>> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS"
>>>> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>>>> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
>>>> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR CONTRIBUTORS BE
>>>> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
>>>> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
>>>> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
>>>> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
>>>> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
>>>> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
>>>> + * POSSIBILITY OF SUCH DAMAGE.
>>>> + *
>>>> + * Author: Keyon Jie <yang.jie at linux.intel.com>
>>>> + *
>>>> + */
>>>> +
>>>> +#include <reef/interrupt.h>
>>>> +#include <reef/interrupt-map.h>
>>>> +#include <reef/alloc.h>
>>>> +#include <arch/interrupt.h>
>>>> +#include <platform/interrupt.h>
>>>> +#include <stdint.h>
>>>> +#include <stdlib.h>
>>>> +
>>>> +int irq_l2_register(int irq, struct l2root_irq *l2_int,
>>>> + void (*handler)(void *arg), void *arg)
>>>
>>> Please comment who calls this and why.
>>
>> OK, will add in next version.
>>
>>>
>>>> +{
>>>> + int ret = 0;
>>>> + struct l2src_irq *l2_src = NULL;
>>>> +
>>>> + if (l2_int == NULL)
>>>> + return -1;
>>>> +
>>>
>>> -EINVAL
>>
>> OK, will fix.
>>
>>>
>>>> + /* Todo: may need to check if this bit id has already been registered */
>>>> + l2_src = rzalloc(RZONE_SYS, RFLAGS_NONE, sizeof(struct l2src_irq));
>>>> + list_init(&l2_src->list);
>>>> + l2_src->bit_id = REEF_IRQ_BIT(irq);
>>>> + l2_src->enabled = 0;
>>>> + l2_src->handler = handler;
>>>> + l2_src->handler_arg = arg;
>>>> +
>>>> + if (l2_int->src_count == 0) {
>>>> + list_init(&l2_int->src_list);
>>>> + ret = arch_interrupt_register(l2_int->num,
>>>> + l2_int->handler, l2_int);
>>>
>>> error handling needed when ret is < 0
>>
>> We will return this ret and let caller handle it.
>> At the same time, arch_interrupt_register() won't fail atm.
>
> but it might in the future ? what happens if > 1 client tries to
> register using the same IRQ number ?
in this design, we can support multiple handlers/clients with the same
IRQ number, wrt how to handle this case, it is up to platform driver
itself, if you refer to our apl interrupt.c, all those handlers will be
called once the src interrupt happened.
>
>>
>>>
>>>> + }
>>>> +
>>>> + /* add to registered list */
>>>> + list_item_append(&l2_src->list, &l2_int->src_list);
>>>
>>> locking ?
>>
>> OK, let me add this.
>>
>>>
>>>> + l2_int->src_count += 1;
>>>> +
>>>
>>> ++
>>>
>>>> + return ret;
>>>> +
>>>> +}
>>>> +
>>>
>>> comment describing new api call
>>
>> same with register one, will describe there.
>>
>>>
>>>> +void irq_l2_unregister(int irq, struct l2root_irq *l2_int)
>>>> +{
>>>> + struct l2src_irq *l2_src = NULL;
>>>> + struct list_item *slist, *tmp;
>>>> +
>>>> + /* looking for the bit id l2 src */
>>>
>>> locking ?
>>
>> OK.
>>
>>>
>>>> + list_for_item_safe(slist, tmp, &l2_int->src_list) {
>>>> + l2_src = container_of(slist, struct l2src_irq, list);
>>>> + if (l2_src->bit_id != REEF_IRQ_BIT(irq))
>>>> + continue;
>>>> +
>>>> + /* delete from lists */
>>>> + list_item_del(&l2_src->list);
>>>> + l2_int->src_count -= 1;
>>>> +
>>>
>>> --
>>>
>>>> + /* free the l2 src which unregistered */
>>>> + if (l2_src->handler)
>>>> + l2_src->handler = NULL;
>>>> + rfree(l2_src);
>>>> + }
>>>> +
>>>
>>> comment
>>
>> OK.
>>
>>>
>>>> + if (l2_int->src_count == 0)
>>>> + arch_interrupt_unregister(l2_int->num);
>>>> +
>>>> +}
>>>> +
>>>> +uint32_t irq_l2_enable(int irq, struct l2root_irq *l2_int)
>>>
>>> irq_unmask() ? return void
>>
>> It will calculate the src masked count and unmask the root if needed.
>
> ok, so best rename the function to use "unmask" in the name
>
>>
>>>
>>>> +{
>>>> + struct l2src_irq *l2_src = NULL;
>>>> + struct list_item *slist;
>>>> +
>>>> + /* looking for the bit id l2 src */
>>>
>>> locking.
>>
>> OK.
>>
>>>
>>>> + list_for_item(slist, &l2_int->src_list) {
>>>> + l2_src = container_of(slist, struct l2src_irq, list);
>>>> + if (l2_src->bit_id != REEF_IRQ_BIT(irq))
>>>> + continue;
>>>> +
>>>> + if (!l2_src->enabled) {
>>>> + /* enable the root interrupt */
>>>
>>> best to name l1, l2 etc
>>
>> OK, let me change them to l1_int and l2_src.
>>
>>>
>>>> + if (l2_int->enabled_count == 0)
>>>> + arch_interrupt_enable_mask(1 <<
>>>> + REEF_IRQ_NUMBER(irq));
>>>> + /* enable the src interrupt */
>>>> + if (plf_irq_ops && plf_irq_ops->int_unmask)
>>>> + plf_irq_ops->int_unmask(irq);
>>>> + l2_src->enabled = 1;
>>>> + l2_int->enabled_count += 1;
>>>
>>> ++
>>>
>>>> + }
>>>> +
>>>> + }
>>>> +
>>>> + return 0;
>>>
>>>
>>>
>>>> +}
>>>> +
>>>> +uint32_t irq_l2_disable(int irq, struct l2root_irq *l2_int)
>>>> +{
>>>
>>> mask() and return void
>>
>> Returning uint32_t is aligned with arch_interrupt_global_disable().
>
> I think that returns flags with the current IRQ state ?
yes that's true. and arch_interrupt_disable_mask() also return flags
also. maybe we can return similar flags for l2 interrupt, so keep this
uint32_t at the moment?
#define arch_interrupt_disable_mask(mask) \
_xtos_ints_off(mask)
>
>>
>>>
>>>> + struct l2src_irq *l2_src = NULL;
>>>> + struct list_item *slist;
>>>> +
>>>> + /* looking for the bit id l2 src */
>>>> + list_for_item(slist, &l2_int->src_list) {
>>>> + l2_src = container_of(slist, struct l2src_irq, list);
>>>> + if (l2_src->bit_id != REEF_IRQ_BIT(irq))
>>>> + continue;
>>>> +
>>>> + if (l2_src->enabled) {
>>>> + /* disable the src interrupt */
>>>> + if (plf_irq_ops && plf_irq_ops->int_mask)
>>>> + plf_irq_ops->int_mask(irq);
>>>> + l2_src->enabled = 0;
>>>> + l2_int->enabled_count -= 1;
>>>> + /* disable the root interrupt */
>>>> + if (l2_int->enabled_count == 0)
>>>> + arch_interrupt_disable_mask(1 <<
>>>> + REEF_IRQ_NUMBER(irq));
>>>> + }
>>>> +
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +int interrupt_register(int irq,
>>>> + void (*handler)(void *arg), void *arg)
>>>> +{
>>>> + if (plf_irq_ops && plf_irq_ops->int_register)
>>>> + return plf_irq_ops->int_register(irq, handler, arg);
>>>> + else
>>>> + return arch_interrupt_register(irq, handler, arg);
>>>
>>> this if() will be known at build time. same for the rest
>>
>> even for byt we can use this ops or not. it should be known at build
>> time, but using macros (e.g. CONFIG_BAYTRAIL) looks not good and
>> checking it at run time may be more accurate, and, these functions won't
>> be called frequently.
>
> Best use the macro, as this also reduces code size. You dont need to use
> the macro in each call, but you could use it once over the whole file.
OK, though that may reduce the reuse of common code(need many
copy/paste), but it do benefit to small code size.
Thanks,
~Keyon
>
> Liam
>
>>
>> Thanks,
>> ~Keyon
>>
>>>
>>>> +}
>>>> +
>>>> +void interrupt_unregister(int irq)
>>>> +{
>>>> + if (plf_irq_ops && plf_irq_ops->int_unregister)
>>>> + plf_irq_ops->int_unregister(irq);
>>>> + else
>>>> + arch_interrupt_unregister(irq);
>>>> +}
>>>> +
>>>> +uint32_t interrupt_enable(uint32_t irq)
>>>> +{
>>>> + if (plf_irq_ops && plf_irq_ops->int_enable)
>>>> + return plf_irq_ops->int_enable(irq);
>>>> + else
>>>> + return arch_interrupt_enable_mask(1 << irq);
>>>> +}
>>>> +
>>>> +uint32_t interrupt_disable(uint32_t irq)
>>>> +{
>>>> + if (plf_irq_ops && plf_irq_ops->int_disable)
>>>> + return plf_irq_ops->int_disable(irq);
>>>> + else
>>>> + return arch_interrupt_disable_mask(1 << irq);
>>>> +}
>>>> +
>>>> +/* set/clear */
>>>> +void interrupt_set(int irq)
>>>> +{
>>>> + /* don't support set/clear l2 src irq */
>>>> + return arch_interrupt_set(REEF_IRQ_NUMBER(irq));
>>>> +}
>>>> +
>>>> +void interrupt_clear(int irq)
>>>> +{
>>>> + /* don't support set/clear l2 src irq */
>>>> + arch_interrupt_clear(REEF_IRQ_NUMBER(irq));
>>>> +}
>>>> +
>>>> +uint32_t interrupt_global_disable(void)
>>>> +{
>>>> + return arch_interrupt_global_disable();
>>>> +}
>>>> +
>>>> +void interrupt_global_enable(uint32_t flags)
>>>> +{
>>>> + arch_interrupt_global_enable(flags);
>>>> +}
>>>> +
>>>> +const struct plf_irq_ops *plf_irq_ops;
>>>> +
>>>
>>>
>>>
>
>
>
More information about the Sound-open-firmware
mailing list