[Sound-open-firmware] [PATCH 2/4] interrupt: enable layer 2 interrupt support
Liam Girdwood
liam.r.girdwood at linux.intel.com
Wed Sep 20 13:06:02 CEST 2017
On Wed, 2017-09-20 at 18:44 +0800, Keyon Jie wrote:
>
> 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.
>
Thought more on this in general. What about :-
1) Clients call irq_clear().
2) irq_clear() calls platform_clear() and/or arch_clear()
3) platform_clear() does the 2nd level mapping and clearing.
arch_clear() does the 1st level mapping/clearing.
We repeat this for all other IRQ calls. clients call irq_() APIs, which
in turn can then call 2nd level platform and 1st level arch IRQ code.
This is probably a mix of your orig patch and my comments.
Liam
> >
> >>>
> >>>>
> >>>> #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