[PATCH 08/15] HiFive1: add irq dispatching function

[Gedare Bloom]

On Sat, Aug 19, 2017 at 11:08 PM, Hesham Almatary
<heshamelmatary at gmail.com> wrote:
> On Fri, Aug 18, 2017 at 1:11 AM, Gedare Bloom <gedare at rtems.org> wrote:
>> On Wed, Aug 16, 2017 at 11:12 AM, Denis Obrezkov
>> <denisobrezkov at gmail.com> wrote:
>>> ---
>>>  c/src/lib/libbsp/riscv32/hifive1/irq/irq.c | 90 ++++++++++++++++++++++++++++++
>>>  1 file changed, 90 insertions(+)
>>>  create mode 100644 c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>>>
>>> diff --git a/c/src/lib/libbsp/riscv32/hifive1/irq/irq.c b/c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>>> new file mode 100644
>>> index 0000000..6fd4e75
>>> --- /dev/null
>>> +++ b/c/src/lib/libbsp/riscv32/hifive1/irq/irq.c
>>> @@ -0,0 +1,90 @@
>>> +/**
>>> + * @file
>>> + *
>>> + * @ingroup riscv_interrupt
>>> + *
>>> + * @brief Interrupt support.
>>> + */
>>> +
>>> +/*
>>> + * RISCV CPU Dependent Source
>>> + *
>>> + * Copyright (c) 2015 University of York.
>>> + * Hesham ALMatary <hmka501 at york.ac.uk>
>>> + *
>>> + * Redistribution and use in source and binary forms, with or without
>>> + * modification, are permitted provided that the following conditions
>>> + * are met:
>>> + * 1. Redistributions of source code must retain the above copyright
>>> + *    notice, this list of conditions and the following disclaimer.
>>> + * 2. 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.
>>> + *
>>> + * THIS SOFTWARE IS PROVIDED BY THE AUTHOR 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 AUTHOR 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.
>>> + */
>>> +
>>> +#include <bsp/fe310.h>
>>> +#include <bsp/irq.h>
>>> +#include <bsp/irq-generic.h>
>>> +
>>> +/* Almost all of the jobs that the following functions should
>>> + * do are implemented in cpukit
>>> + */
>>> +
>>> +void bsp_interrupt_handler_default(rtems_vector_number vector)
>>> +{
>>> +    printk("spurious interrupt: %u\n", vector);
>>> +}
>>> +
>>> +rtems_status_code bsp_interrupt_facility_initialize()
>>> +{
>>> +  return 0;
>>> +}
>>> +
>>> +rtems_status_code bsp_interrupt_vector_enable(rtems_vector_number vector)
>>> +{
>>> +  return 0;
>>> +}
>>> +
>>> +rtems_status_code bsp_interrupt_vector_disable(rtems_vector_number vector)
>>> +{
>>> +  return 0;
>>> +}
>>> +
>>> +static uint32_t cause = 0;
>>> +volatile uint64_t * mtimecmp = (volatile uint64_t *)0x02004000;
>>> +volatile uint64_t * mtime = (volatile uint64_t *)0x0200bff8;
>>> +
> Avoid hard-coded values whenever possible, same for the rest of the patches.
>
>> What are these global variables used for?
>>
>>> +void handle_trap_new ()
>> What is the purpose of this function?
>>
>>> +{
>>> +    asm volatile ("csrr %0, mcause": "=r" (cause));
>> It would be good to have a static inline function for reading this register.
>>
> +1 Consider adding a utility .h file to read/write commonly used
> registers (e.g. mstatus)
>>> +    if (cause & MCAUSE_INT) {
>>> +      /* an interrupt occurred */
>>> +      if ((cause & MCAUSE_MTIME) == MCAUSE_MTIME) {
> I believe you don't need the " == MCAUSE_MTIME"
Keep it. We prefer the explicit check for bit-wise equality to the
flag in RTEMS style:
https://devel.rtems.org/wiki/Developer/Coding/Conventions#LanguageandCompiler

>>> +       /* Timer interrupt */
>>> +           (*mtimecmp) = (*mtime) + FE310_CLOCK_PERIOD;
>>> +        bsp_interrupt_handler_table[1].handler(bsp_interrupt_handler_table[1].arg);
>>> +      } else if ((cause & MCAUSE_MEXT) == MCAUSE_MEXT) {
>>> +             /*External interrupt */
>>> +          asm volatile ("csrci mie, 0x800");
> I assume you're only handling timer interrupts. It would be useful to
> tell the user (printk?) that this interrupt type is not handled
> proprely yet, and add a FIXME/TODO comment here.
>>> +      } else if ((cause & MCAUSE_MSWI) == MCAUSE_MSWI) {
>>> +             /* Software interrupt */
>>> +             volatile uint32_t * msip_reg = (volatile uint32_t *) 0x02000000;
>>> +             *msip_reg = 0;
>>> +      }
>>> +    } else {
>>> +      /* an exception occurred */
>>> +    }
>>> +
>>> +}
>>> --
>>> 2.1.4
>>>
>>> _______________________________________________
>>> devel mailing list
>>> devel at rtems.org
>>> http://lists.rtems.org/mailman/listinfo/devel
>> _______________________________________________
>> devel mailing list
>> devel at rtems.org
>> http://lists.rtems.org/mailman/listinfo/devel
>
>
>
> --
> Hesham