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@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.

#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 ?

+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 *arg), 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@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.

+{

• int ret = 0;
• struct l2src_irq *l2_src = NULL;
• if (l2_int == NULL)

• return -1;
  

-EINVAL

• /* 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

• }
• /* add to registered list */
• list_item_append(&l2_src->list, &l2_int->src_list);

locking ?

• l2_int->src_count += 1;

++

• return ret;

+}

comment describing new api call

+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 ?

• 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

• 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

+{

• struct l2src_irq *l2_src = NULL;
• struct list_item *slist;
• /* looking for the bit id l2 src */

locking.

• 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

• 	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

• 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

+}

+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;

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@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.

...

...

#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@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 ?

...

...

• }
• /* 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 ?

...

...

• 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.

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;

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@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@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;

On Tue, 2017-09-19 at 15:47 +0800, Keyon Jie wrote:

We don't actually use layer 2 interrupt in baytrail, in the new layer 2 interrupt design, it will fallback to use the root arch interrupt.

This is not needed if you do the abstraction in platform.c instead of drivers.

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/platform/baytrail/platform.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/platform/baytrail/platform.c b/src/platform/baytrail/platform.c index b8c3e49..6726325 100644 --- a/src/platform/baytrail/platform.c +++ b/src/platform/baytrail/platform.c @@ -100,7 +100,7 @@ int platform_boot_complete(uint32_t boot_message) }

/* clear mask in PISR, bits are W1C in docs but some bits need preserved ?? */ -void platform_interrupt_clear(uint32_t irq, uint32_t mask) +void byt_interrupt_clear(uint32_t irq, uint32_t mask) { switch (irq) { case IRQ_NUM_EXT_DMAC0: @@ -123,21 +123,34 @@ void platform_interrupt_clear(uint32_t irq, uint32_t mask) }

/* TODO: expand this to 64 bit - should we just return mask of IRQ numbers */ -uint32_t platform_interrupt_get_enabled(void) +uint32_t byt_interrupt_get_enabled(void) { return shim_read(SHIM_PIMR); }

-void platform_interrupt_mask(uint32_t irq, uint32_t mask) +void byt_interrupt_mask(uint32_t irq) {

}

-void platform_interrupt_unmask(uint32_t irq, uint32_t mask) +void byt_interrupt_unmask(uint32_t irq) {

}

+static const struct plf_irq_ops byt_irq_ops = {

• .get_enabled = byt_interrupt_get_enabled,
• .int_clear = byt_interrupt_clear,
• .int_mask = byt_interrupt_mask,
• .int_unmask = byt_interrupt_unmask,

+};

+void byt_interrupt_init(void) +{

• plf_irq_ops = &byt_irq_ops;

+}

static struct timer platform_ext_timer = { .id = TIMER3, .irq = IRQ_NUM_EXT_TIMER, @@ -155,6 +168,8 @@ int platform_init(struct reef *reef) #error Undefined platform #endif

• byt_interrupt_init();

• trace_point(TRACE_BOOT_PLATFORM_MBOX);

  /* clear mailbox for early trace and debug */

On Wed, 2017-09-20 at 17:47 +0800, Keyon Jie wrote:

On 2017年09月19日 22:54, Liam Girdwood wrote:

...

On Tue, 2017-09-19 at 15:47 +0800, Keyon Jie wrote:

...

We don't actually use layer 2 interrupt in baytrail, in the new layer 2 interrupt design, it will fallback to use the root arch interrupt.

This is not needed if you do the abstraction in platform.c instead of drivers.

But doing that will make the APIs quite different for different platforms , and then calling to these APIs(e.g. from dw-dma.c) will need many #ifdef macros, which may lead code to be in bad readability.

I dont think so, but can you give an example.

Liam

Thanks, ~Keyon

...

...

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/platform/baytrail/platform.c | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/src/platform/baytrail/platform.c b/src/platform/baytrail/platform.c index b8c3e49..6726325 100644 --- a/src/platform/baytrail/platform.c +++ b/src/platform/baytrail/platform.c @@ -100,7 +100,7 @@ int platform_boot_complete(uint32_t boot_message) }

/* clear mask in PISR, bits are W1C in docs but some bits need preserved ?? */ -void platform_interrupt_clear(uint32_t irq, uint32_t mask) +void byt_interrupt_clear(uint32_t irq, uint32_t mask) { switch (irq) { case IRQ_NUM_EXT_DMAC0: @@ -123,21 +123,34 @@ void platform_interrupt_clear(uint32_t irq, uint32_t mask) }

/* TODO: expand this to 64 bit - should we just return mask of IRQ numbers */ -uint32_t platform_interrupt_get_enabled(void) +uint32_t byt_interrupt_get_enabled(void) { return shim_read(SHIM_PIMR); }

-void platform_interrupt_mask(uint32_t irq, uint32_t mask) +void byt_interrupt_mask(uint32_t irq) {

}

-void platform_interrupt_unmask(uint32_t irq, uint32_t mask) +void byt_interrupt_unmask(uint32_t irq) {

}

+static const struct plf_irq_ops byt_irq_ops = {

• .get_enabled = byt_interrupt_get_enabled,
• .int_clear = byt_interrupt_clear,
• .int_mask = byt_interrupt_mask,
• .int_unmask = byt_interrupt_unmask,

+};

+void byt_interrupt_init(void) +{

• plf_irq_ops = &byt_irq_ops;

+}

• static struct timer platform_ext_timer = { .id = TIMER3, .irq = IRQ_NUM_EXT_TIMER,

@@ -155,6 +168,8 @@ int platform_init(struct reef *reef) #error Undefined platform #endif

• byt_interrupt_init();

• trace_point(TRACE_BOOT_PLATFORM_MBOX);

  /* clear mailbox for early trace and debug */

On Tue, 2017-09-19 at 15:47 +0800, Keyon Jie wrote:

We don't use layer 2 interrupt for timer on baytrail actually, it will fallback to use the root arch interrupt there.

Lets do this after 64bit timer is applied.

Liam

Signed-off-by: Keyon Jie yang.jie@linux.intel.com

src/arch/xtensa/timer.c | 35 +++++++++++++++++++++++++++++++++++ src/include/reef/timer.h | 40 ++++++++-------------------------------- 2 files changed, 43 insertions(+), 32 deletions(-)

diff --git a/src/arch/xtensa/timer.c b/src/arch/xtensa/timer.c index 9395ca9..21b65c4 100644 --- a/src/arch/xtensa/timer.c +++ b/src/arch/xtensa/timer.c @@ -57,3 +57,38 @@ void arch_timer_set(struct timer *timer, unsigned int ticks) break; } }

+int timer_register(struct timer *timer,

• void (*handler)(void *arg), void *arg)

+{

• return interrupt_register(timer->irq, handler, arg);

+}

+void timer_unregister(struct timer *timer) +{

• interrupt_unregister(timer->irq);

+}

+void timer_enable(struct timer *timer) +{

• interrupt_enable(timer->irq);

+}

+void timer_disable(struct timer *timer) +{

• interrupt_disable(timer->irq);

+}

+void timer_set(struct timer *timer, unsigned int ticks) +{

• arch_timer_set(timer, ticks);

+}

+//void timer_set_ms(struct timer *timer, unsigned int ms);

+void timer_clear(struct timer *timer) +{

• interrupt_clear(timer->irq);

+}

diff --git a/src/include/reef/timer.h b/src/include/reef/timer.h index d10d5e2..0c0f0ea 100644 --- a/src/include/reef/timer.h +++ b/src/include/reef/timer.h @@ -32,40 +32,16 @@ #define __INCLUDE_TIMER__

#include <arch/timer.h> +#include <reef/interrupt.h> #include <stdint.h>

-static inline int timer_register(struct timer *timer,

• void(*handler)(void *arg), void *arg)

-{

• return arch_timer_register(timer, handler, arg);

-}

-static inline void timer_unregister(struct timer *timer) -{

• arch_timer_unregister(timer);

-}

-static inline void timer_enable(struct timer *timer) -{

• arch_timer_enable(timer);

-}

-static inline void timer_disable(struct timer *timer) -{

• arch_timer_disable(timer);

-}

-static inline void timer_set(struct timer *timer, unsigned int ticks) -{

• arch_timer_set(timer, ticks);

-}

-void timer_set_ms(struct timer *timer, unsigned int ms);

-static inline void timer_clear(struct timer *timer) -{

• arch_timer_clear(timer);

-} +int timer_register(struct timer *timer,

• void (*handler)(void *arg), void *arg);

+void timer_unregister(struct timer *timer); +void timer_enable(struct timer *timer); +void timer_disable(struct timer *timer); +void timer_set(struct timer *timer, unsigned int ticks); +void timer_clear(struct timer *timer);

unsigned int timer_get_count(struct timer *timer);