[haiku-commits] Re: haiku: hrev48180 - src/system/kernel/arch/arm

  • From: "Ithamar R. Adema" <ithamar@xxxxxxxxxxxxxxxxxxx>
  • To: haiku-commits@xxxxxxxxxxxxx
  • Date: Sat, 1 Nov 2014 14:01:44 +0100

Hi Pawel,

My C++ usage has been really low recently so let me first agree with all
the C++-ism's you are suggesting ;)
Most of this is conversion of old C code I had around so as soon as I've
got some time I'll make the changes you suggested.

I just wanted to get this code committed before it bitrots on my machine
for another year (until next BG coding sprint) :P

Thanks for reviewing!

Ithamar.


On Fri, Oct 31, 2014 at 2:29 PM, Paweł Dziepak <pdziepak@xxxxxxxxxxx> wrote:

> 2014-10-31 11:39 GMT+01:00 <ithamar@xxxxxxxxxxxxxxxxxxx>:
>
>> hrev48180 adds 1 changeset to branch 'master'
>> old head: 47c530330519332c91110ddf85d276d4f3835d31
>> new head: a52dd58d2da9adad93ad49b33181da57849c8ad5
>> overview:
>> http://cgit.haiku-os.org/haiku/log/?qt=range&q=a52dd58+%5E47c5303
>>
>>
>> ----------------------------------------------------------------------------
>> sinstance
>> a52dd58: ARM: kernel: introduce SoC abstraction
>>
>>   This introduces InterruptController and HardwareTimer classes to
>>   handle the SoC specific implementations of timers and ints for
>>   the ARM platform.
>>
>>   These could be improved and moved to a more 'generic' level once
>>   we're confident they are 'good enough'.
>>
>
> Really nice work! Some suggestions below.
>
>
>>
>>   NOTE: The OMAP timer implementation is fully untested and probably
>>         completely non-functional....
>>
>>                           [ Ithamar R. Adema <ithamar@xxxxxxxxxxxxxxxxxxx>
>> ]
>>
>>
>> ----------------------------------------------------------------------------
>>
>> Revision:    hrev48180
>> Commit:      a52dd58d2da9adad93ad49b33181da57849c8ad5
>> URL:         http://cgit.haiku-os.org/haiku/commit/?id=a52dd58
>> Author:      Ithamar R. Adema <ithamar@xxxxxxxxxxxxxxxxxxx>
>> Date:        Fri Oct 31 10:37:02 2014 UTC
>>
>>
>> ----------------------------------------------------------------------------
>>
>> 9 files changed, 618 insertions(+), 120 deletions(-)
>> src/system/kernel/arch/arm/Jamfile        |   6 +
>> src/system/kernel/arch/arm/arch_int.cpp   |  77 +++++-----
>> src/system/kernel/arch/arm/arch_timer.cpp | 108 ++++----------
>> src/system/kernel/arch/arm/soc.cpp        |   4 +
>> src/system/kernel/arch/arm/soc.h          |  68 +++++++++
>> src/system/kernel/arch/arm/soc_omap3.cpp  | 204 ++++++++++++++++++++++++++
>> src/system/kernel/arch/arm/soc_omap3.h    |  62 ++++++++
>> src/system/kernel/arch/arm/soc_pxa.cpp    | 154 +++++++++++++++++++
>> src/system/kernel/arch/arm/soc_pxa.h      |  55 +++++++
>>
>>
>> ----------------------------------------------------------------------------
>>
>> diff --git a/src/system/kernel/arch/arm/Jamfile
>> b/src/system/kernel/arch/arm/Jamfile
>> index c2ade91..62f6f6c 100644
>> --- a/src/system/kernel/arch/arm/Jamfile
>> +++ b/src/system/kernel/arch/arm/Jamfile
>> @@ -34,6 +34,12 @@ KernelMergeObject kernel_arch_arm.o :
>>         arch_atomic64.cpp
>>         arch_atomic32.cpp
>>
>> +       # SoC minimal kernel-required support
>> +       # (timers, interrupts, rtc?)
>> +       soc.cpp
>> +       soc_pxa.cpp
>> +       soc_omap3.cpp
>> +
>>         # paging
>>         arm_physical_page_mapper_large_memory.cpp
>>         ARMPagingMethod.cpp
>> diff --git a/src/system/kernel/arch/arm/arch_int.cpp
>> b/src/system/kernel/arch/arm/arch_int.cpp
>> index 1de4426..a776624 100644
>> --- a/src/system/kernel/arch/arm/arch_int.cpp
>> +++ b/src/system/kernel/arch/arm/arch_int.cpp
>> @@ -30,6 +30,11 @@
>>  #include <vm/VMAddressSpace.h>
>>  #include <string.h>
>>
>> +#include <drivers/bus/FDT.h>
>> +#include "soc.h"
>> +
>> +#include "soc_pxa.h"
>> +#include "soc_omap3.h"
>>
>>  #define TRACE_ARCH_INT
>>  #ifdef TRACE_ARCH_INT
>> @@ -42,17 +47,6 @@
>>  #define USER_VECTOR_ADDR_LOW   0x00000000
>>  #define USER_VECTOR_ADDR_HIGH  0xffff0000
>>
>> -#define PXA_INTERRUPT_PHYS_BASE        0x40D00000
>> -#define PXA_INTERRUPT_SIZE     0x00000034
>> -
>> -#define PXA_ICIP       0x00
>> -#define PXA_ICMR       0x01
>> -#define PXA_ICFP       0x03
>> -#define PXA_ICMR2      0x28
>> -
>> -static area_id sPxaInterruptArea;
>> -static uint32 *sPxaInterruptBase;
>> -
>>  extern int _vectors_start;
>>  extern int _vectors_end;
>>
>> @@ -60,6 +54,7 @@ static area_id sVectorPageArea;
>>  static void *sVectorPageAddress;
>>  static area_id sUserVectorPageArea;
>>  static void *sUserVectorPageAddress;
>> +static fdt_module_info *sFdtModule;
>>
>>  // An iframe stack used in the early boot process when we don't have
>>  // threads yet.
>> @@ -70,13 +65,9 @@ void
>>  arch_int_enable_io_interrupt(int irq)
>>
>
> This functions and its friends probably should be with 'inline' and inside
> some arch specific header.
>
>
>>  {
>>         TRACE(("arch_int_enable_io_interrupt(%d)\n", irq));
>> -
>> -       if (irq <= 31) {
>> -               sPxaInterruptBase[PXA_ICMR] |= 1 << irq;
>> -               return;
>> -       }
>> -sinstance
>> -       sPxaInterruptBase[PXA_ICMR2] |= 1 << (irq - 32);
>> +       InterruptController *ic = InterruptController::Get();
>>
>
> auto ic = InterruptController::Get();
>
>
>> +       if (ic != NULL)
>> +               ic->EnableInterrupt(irq);
>>  }
>>
>>
> I don't like these null checks. Is there any code that calls these
> functions too early (i.e. before the interrupt controller is created)? If
> yes, the initialization order probably should be fixed. If no, then we do
> not need these checks at all and just add a single 'if' after interrupt
> initialization code to make sure that exactly one interrupt controller has
> been found and initialized. The same applies to timers.
>
> On a side note, nullptr is much nicer that NULL because, unlike the
> latter, it is type safe.
>
>
>>
>> @@ -84,13 +75,9 @@ void
>>  arch_int_disable_io_interrupt(int irq)
>>  {
>>         TRACE(("arch_int_disable_io_interrupt(%d)\n", irq));
>> -
>> -       if (irq <= 31) {
>> -               sPxaInterruptBase[PXA_ICMR] &= ~(1 << irq);
>> -               return;
>> -       }
>> -
>> -       sPxaInterruptBase[PXA_ICMR2] &= ~(1 << (irq - 32));
>> +       InterruptController *ic = InterruptController::Get();
>> +       if (ic != NULL)
>> +               ic->DisableInterrupt(irq);
>>  }
>>
>>
>> @@ -129,6 +116,18 @@ arch_int_init(kernel_args *args)
>>  extern "C" void arm_vector_init(void);
>>
>>
>> +static struct fdt_device_info intc_table[] = {
>> +       {
>> +               .compatible = "marvell,pxa-intc",
>> +               .init = PXAInterruptController::Init,
>> +       }, {
>> +               .compatible = "ti,omap3-intc",
>> +               .init = OMAP3InterruptController::Init,
>> +       }
>> +};
>> +static int intc_count = sizeof(intc_table) / sizeof(struct
>> fdt_device_info);
>> +
>> +
>>  status_t
>>  arch_int_init_post_vm(kernel_args *args)
>>  {
>> @@ -136,7 +135,6 @@ arch_int_init_post_vm(kernel_args *args)
>>         sVectorPageArea = create_area("vectorpage", (void
>> **)&sVectorPageAddress,
>>                 B_ANY_ADDRESS, VECTORPAGE_SIZE, B_FULL_LOCK,
>>                 B_KERNEL_WRITE_AREA | B_KERNEL_READ_AREA);
>> -
>>         if (sVectorPageArea < 0)
>>                 panic("vector page could not be created!");
>>
>> @@ -166,14 +164,13 @@ arch_int_init_post_vm(kernel_args *args)
>>                         dprintf("Enabled high vectors\n");
>>         }
>>
>> -       sPxaInterruptArea = map_physical_memory("pxa_intc",
>> PXA_INTERRUPT_PHYS_BASE,
>> -               PXA_INTERRUPT_SIZE, 0, B_KERNEL_READ_AREA |
>> B_KERNEL_WRITE_AREA, (void**)&sPxaInterruptBase);
>> -
>> -       if (sPxaInterruptArea < 0)
>> -               return sPxaInterruptArea;
>> +       status_t rc = get_module(B_FDT_MODULE_NAME,
>> (module_info**)&sFdtModule);
>> +       if (rc != B_OK)
>> +               panic("Unable to get FDT module: %08lx!\n", rc);
>>
>> -       sPxaInterruptBase[PXA_ICMR] = 0;
>> -       sPxaInterruptBase[PXA_ICMR2] = 0;
>> +       rc = sFdtModule->setup_devices(intc_table, intc_count, NULL);
>> +       if (rc != B_OK)
>> +               panic("No interrupt controllers found!\n");
>>
>>         return B_OK;
>>  }
>> @@ -338,10 +335,9 @@ arch_arm_irq(struct iframe *iframe)
>>  {
>>         IFrameScope scope(iframe);
>>
>> -       for (int i=0; i < 32; i++) {
>> -               if (sPxaInterruptBase[PXA_ICIP] & (1 << i))
>> -                       int_io_interrupt_handler(i, true);
>> -       }
>> +       InterruptController *ic = InterruptController::Get();
>> +       if (ic != NULL)
>> +               ic->HandleInterrupt();
>>  }
>>
>>
>> @@ -350,10 +346,5 @@ arch_arm_fiq(struct iframe *iframe)
>>  {
>>         IFrameScope scope(iframe);
>>
>> -       for (int i=0; i < 32; i++) {
>> -               if (sPxaInterruptBase[PXA_ICIP] & (1 << i)) {
>> -                       dprintf("arch_arm_fiq: help me, FIQ %d was
>> triggered but no "
>> -                               "FIQ support!\n", i);
>> -               }
>> -       }
>> +       panic("FIQ not implemented yet!");
>>  }
>> diff --git a/src/system/kernel/arch/arm/arch_timer.cpp
>> b/src/system/kernel/arch/arm/arch_timer.cpp
>> index 01a6fb2..8225549 100644
>> --- a/src/system/kernel/arch/arm/arch_timer.cpp
>> +++ b/src/system/kernel/arch/arm/arch_timer.cpp
>> @@ -19,6 +19,11 @@
>>  #include <arch/timer.h>
>>  #include <arch/cpu.h>
>>
>> +#include <drivers/bus/FDT.h>
>> +#include "soc.h"
>> +
>> +#include "soc_pxa.h"
>> +#include "soc_omap3.h"
>>
>>  //#define TRACE_ARCH_TIMER
>>  #ifdef TRACE_ARCH_TIMER
>> @@ -27,99 +32,49 @@
>>  #      define TRACE(x) ;
>>  #endif
>>
>> +static fdt_module_info *sFdtModule;
>>
>> -#define PXA_TIMERS_PHYS_BASE   0x40A00000
>> -#define PXA_TIMERS_SIZE                B_PAGE_SIZE
>> -#define PXA_TIMERS_INTERRUPT   7 /* OST_4_11 */
>> -
>> -#define PXA_OSSR               0x05
>> -#define PXA_OIER               0x07
>> -#define PXA_OSCR4              0x10
>> -#define PXA_OSCR5              0x11
>> -#define PXA_OSMR4              0x20
>> -#define PXA_OSMR5              0x21
>> -#define PXA_OMCR4              0x30
>> -#define PXA_OMCR5              0x31
>> -
>> -#define PXA_RES_S      (3 << 0)
>> -#define PXA_RES_MS     (1 << 1)
>> -#define PXA_RES_US     (1 << 2)
>> -
>> -#define US2S(bt)       ((bt) / 1000000ULL)
>> -#define US2MS(bt)      ((bt) / 1000ULL)
>> -
>> -static area_id sPxaTimersArea = -1;
>> -static uint32 *sPxaTimersBase = NULL;
>> -static bigtime_t sSystemTime = 0;
>> -
>> -static int32
>> -pxa_timer_interrupt(void *data)
>> -{
>> -       if (sPxaTimersBase[PXA_OSSR] & (1 << 4)) {
>> -               sPxaTimersBase[PXA_OSSR] |= (1 << 4);
>> -               return timer_interrupt();
>> -       }
>> -
>> -       if (sPxaTimersBase[PXA_OSSR]  & (1 << 5)) {
>> -               sPxaTimersBase[PXA_OSSR] |= (1 << 5);
>> -               sSystemTime += UINT_MAX + 1ULL;
>> +static struct fdt_device_info intc_table[] = {
>> +       {
>> +               .compatible = "marvell,pxa-timers",     // XXX not in FDT
>> (also not in upstream!)
>> +               .init = PXATimer::Init,
>> +       }, {
>> +               .compatible = "ti,omap3430-timer",
>> +               .init = OMAP3Timer::Init,
>>         }
>> +};
>> +static int intc_count = sizeof(intc_table) / sizeof(struct
>> fdt_device_info);
>>
>
> I think that std::array<> would be a much better choice here. True, unless
> you need some scary looking helpers it needs an explicit specification of
> the number of the elements, but then it has a std::array<>::size() member
> function so that we can get rid of intc_count.
>
>
>>
>> -       return B_HANDLED_INTERRUPT;
>> -}
>>
>>  void
>>  arch_timer_set_hardware_timer(bigtime_t timeout)
>>  {
>> -       uint32 val = timeout & UINT_MAX;
>> -       uint32 res = PXA_RES_US;
>> -
>> -       if (timeout & ~UINT_MAX) {
>> -               // Does not fit, so scale resolution down to milliseconds
>> -               if (US2MS(timeout) & ~UINT_MAX) {
>> -                       // Still does not fit, scale down to seconds as
>> last ditch attempt
>> -                       val = US2S(timeout) & UINT_MAX;
>> -                       res = PXA_RES_S;
>> -               } else {
>> -                       // Fits in millisecond resolution
>> -                       val = US2MS(timeout) & UINT_MAX;
>> -                       res = PXA_RES_MS;
>> -               }
>> -       }
>> -
>> -       TRACE(("arch_timer_set_hardware_timer(val=%lu, res=%lu)\n", val,
>> res));
>> -       sPxaTimersBase[PXA_OIER] |= (1 << 4);
>> -       sPxaTimersBase[PXA_OMCR4] = res;
>> -       sPxaTimersBase[PXA_OSMR4] = val;
>> -       sPxaTimersBase[PXA_OSCR4] = 0; // start counting from 0 again
>> +       HardwareTimer *timer = HardwareTimer::Get();
>> +       if (timer != NULL)
>> +               timer->SetTimeout(timeout);
>>  }
>>
>>
>>  void
>>  arch_timer_clear_hardware_timer()
>>  {
>> -       TRACE(("arch_timer_clear_hardware_timer\n"));
>> -
>> -       sPxaTimersBase[PXA_OMCR4] = 0; // disable our timer
>> -       sPxaTimersBase[PXA_OIER] &= ~(1 << 4);
>> +       HardwareTimer *timer = HardwareTimer::Get();
>> +       if (timer != NULL)
>> +               timer->Clear();
>>  }
>>
>>  int
>>  arch_init_timer(kernel_args *args)
>>  {
>>         TRACE(("%s\n", __func__));
>> -       sPxaTimersArea = map_physical_memory("pxa_timers",
>> PXA_TIMERS_PHYS_BASE,
>> -               PXA_TIMERS_SIZE, 0, B_KERNEL_READ_AREA |
>> B_KERNEL_WRITE_AREA, (void**)&sPxaTimersBase);
>> -
>> -       if (sPxaTimersArea < 0)
>> -               return sPxaTimersArea;
>>
>> -       sPxaTimersBase[PXA_OIER] |= (1 << 5); // enable timekeeping timer
>> -       sPxaTimersBase[PXA_OMCR5] = PXA_RES_US | (1 << 7);
>> -       sPxaTimersBase[PXA_OSMR5] = UINT_MAX;
>> -       sPxaTimersBase[PXA_OSCR5] = 0;
>> +       status_t rc = get_module(B_FDT_MODULE_NAME,
>> (module_info**)&sFdtModule);
>> +       if (rc != B_OK)
>> +               panic("Unable to get FDT module: %08lx!\n", rc);
>>
>> -       install_io_interrupt_handler(PXA_TIMERS_INTERRUPT,
>> &pxa_timer_interrupt, NULL, 0);
>> +       rc = sFdtModule->setup_devices(intc_table, intc_count, NULL);
>> +       if (rc != B_OK)
>> +               panic("No interrupt controllers found!\n");
>>
>>         return B_OK;
>>  }
>> @@ -127,10 +82,9 @@ arch_init_timer(kernel_args *args)
>>  bigtime_t
>>  system_time(void)
>>  {
>> -       if (sPxaTimersArea < 0)
>> -               return 0;
>> +       HardwareTimer *timer = HardwareTimer::Get();
>> +       if (timer != NULL)
>> +               return timer->Time();
>>
>> -       return (sPxaTimersBase != NULL) ?
>> -               sSystemTime + sPxaTimersBase[PXA_OSCR5] :
>> -               0ULL;
>> +       return 0;
>>  }
>> diff --git a/src/system/kernel/arch/arm/soc.cpp
>> b/src/system/kernel/arch/arm/soc.cpp
>> new file mode 100644
>> index 0000000..54d48cb
>> --- /dev/null
>> +++ b/src/system/kernel/arch/arm/soc.cpp
>> @@ -0,0 +1,4 @@
>> +#include "soc.h"
>> +
>> +InterruptController *InterruptController::sInstance = NULL;
>> +HardwareTimer *HardwareTimer::sInstance = NULL;
>> diff --git a/src/system/kernel/arch/arm/soc.h
>> b/src/system/kernel/arch/arm/soc.h
>> new file mode 100644
>> index 0000000..4b1f4e8
>> --- /dev/null
>> +++ b/src/system/kernel/arch/arm/soc.h
>> @@ -0,0 +1,68 @@
>> +#ifndef ARCH_ARM_SOC_H
>> +#define ARCH_ARM_SOC_H
>> +
>> +class InterruptController;
>> +
>> +#include <drivers/bus/FDT.h>
>> +#include <private/kernel/int.h>
>> +#include <private/kernel/timer.h>
>> +
>> +// ------------------------------------------------------
>> InterruptController
>> +
>> +class InterruptController {
>> +public:
>> +       virtual void EnableInterrupt(int irq) = 0;
>> +       virtual void DisableInterrupt(int irq) = 0;
>> +
>> +       virtual void HandleInterrupt() = 0;
>>
>
> Copy and move constructors as well as assignment operator should be
> deleted.
>
>
>> +
>> +       static InterruptController* Get() {
>> +               return sInstance;
>> +       }
>> +
>> +protected:
>> +       InterruptController(fdt_module_info *fdtModule, fdt_device_node
>> node)
>> +               : fFDT(fdtModule), fNode(node) {
>> +               if (sInstance) {
>> +                       panic("Multiple InterruptController objects
>> created; that is currently unsupported!");
>> +               }
>> +               sInstance = this;
>> +       }
>> +
>> +       // Keep our node around as we might want to grab attributes from
>> it
>> +       fdt_module_info *fFDT;
>> +       fdt_device_node fNode;
>> +
>> +       static InterruptController *sInstance;
>> +};
>> +
>> +
>> +// ------------------------------------------------------ HardwareTimer
>> +
>> +class HardwareTimer {
>> +public:
>> +       virtual void SetTimeout(bigtime_t timeout) = 0;
>> +       virtual bigtime_t Time() = 0;
>> +       virtual void Clear() = 0;
>> +
>> +       static HardwareTimer* Get() {
>> +               return sInstance;
>> +       }
>> +
>> +protected:
>> +       HardwareTimer(fdt_module_info *fdtModule, fdt_device_node node)
>> +               : fFDT(fdtModule), fNode(node) {
>> +               if (sInstance) {
>> +                       panic("Multiple HardwareTimer objects created;
>> that is currently unsupported!");
>> +               }
>> +               sInstance = this;
>> +       }
>> +
>> +       // Keep our node around as we might want to grab attributes from
>> it
>> +       fdt_module_info *fFDT;
>> +       fdt_device_node fNode;
>> +
>> +       static HardwareTimer *sInstance;
>> +};
>> +
>> +#endif // ARCH_ARM_SOC_H
>> diff --git a/src/system/kernel/arch/arm/soc_omap3.cpp
>> b/src/system/kernel/arch/arm/soc_omap3.cpp
>> new file mode 100644
>> index 0000000..c31476e
>> --- /dev/null
>> +++ b/src/system/kernel/arch/arm/soc_omap3.cpp
>> @@ -0,0 +1,204 @@
>> +#include "soc_omap3.h"
>> +
>> +enum {
>> +       INTCPS_REVISION = 0,
>> +       INTCPS_SYSCONFIG = 4,
>> +       INTCPS_SYSSTATUS,
>> +       INTCPS_SIR_IRQ = 16,
>> +       INTCPS_SIR_FIQ = 17,
>> +       INTCPS_CONTROL = 18,
>> +       INTCPS_PROTECTION = 19,
>> +       INTCPS_IDLE = 20,
>> +       INTCPS_IRQ_PRIORITY = 24,
>> +       INTCPS_FIQ_PRIORITY = 25,
>> +       INTCPS_THRESHOLD = 26,
>> +       INTCPS_ITRn = 32,
>> +       INTCPS_MIRn = 33,
>> +       INTCPS_MIR_CLEARn = 34,
>> +       INTCPS_MIR_SETn = 35,
>> +       INTCPS_ISR_SETn = 36,
>> +       INTCPS_ISR_CLEARn = 37,
>> +       INTCPS_PENDING_IRQn = 38,
>> +       INTCPS_PENDING_FIQn = 39,
>> +       INTCPS_ILRm = 40,
>> +};
>> +
>> +
>> +void
>> +OMAP3InterruptController::EnableInterrupt(int irq)
>> +{
>> +       uint32 bit = irq % 32, bank = irq / 32;
>> +       fRegBase[INTCPS_MIR_CLEARn + (8 * bank)] = 1 << bit;
>> +}
>> +
>> +
>> +void
>> +OMAP3InterruptController::DisableInterrupt(int irq)
>> +{
>> +       uint32 bit = irq % 32, bank = irq / 32;
>> +       fRegBase[INTCPS_MIR_SETn + (8 * bank)] = 1 << bit;
>> +}
>> +
>> +
>> +void
>> +OMAP3InterruptController::HandleInterrupt()
>> +{
>> +       bool handledIRQ = false;
>> +       int irqnr = 0;
>> +
>> +       do {
>> +               for (uint32 i=0; i < fNumPending; i++) {
>> +                       irqnr = fRegBase[INTCPS_PENDING_IRQn + (8 * i)];
>> +                       if (irqnr)
>> +                               break;
>> +               }
>> +
>> +               if (!irqnr)
>> +                       break;
>> +
>> +               irqnr = fRegBase[INTCPS_SIR_IRQ];
>> +               irqnr &= 0x7f; /* ACTIVEIRQ */
>> +
>> +               if (irqnr) {
>> +                       int_io_interrupt_handler(irqnr, true);
>> +                       handledIRQ = true;
>> +               }
>> +       } while(irqnr);
>> +
>> +       // If IRQ got cleared before we could handle it, simply
>> +       // ack it.
>> +       if (!handledIRQ)
>> +               fRegBase[INTCPS_CONTROL] = 1;
>> +}
>> +
>> +
>> +void
>> +OMAP3InterruptController::SoftReset()
>> +{
>> +       uint32 tmp = fRegBase[INTCPS_REVISION] & 0xff;
>> +
>> +       dprintf("OMAP: INTC found at 0x%p (rev %ld.%ld)\n",
>> +               fRegBase, tmp >> 4, tmp & 0xf);
>> +
>> +       tmp = fRegBase[INTCPS_SYSCONFIG];
>> +       tmp |= 1 << 1;  /* soft reset */
>> +       fRegBase[INTCPS_SYSCONFIG] = tmp;
>> +
>> +        while (!(fRegBase[INTCPS_SYSSTATUS] & 0x1))
>> +                /* Wait for reset to complete */;
>>
>
> Isn't a compiler barrier needed here?
>
>
>> +
>> +        /* Enable autoidle */
>> +        fRegBase[INTCPS_SYSCONFIG] = 1;
>> +}
>> +
>> +
>> +OMAP3InterruptController::OMAP3InterruptController(fdt_module_info *fdt,
>> fdt_device_node node)
>> +       : InterruptController(fdt, node),
>> +       fNumPending(3)
>> +{
>> +       fRegArea = fFDT->map_reg_range(node, 0, (void**)&fRegBase);
>> +       if (fRegArea < 0)
>> +               panic("OMAP3InterruptController: cannot map registers!");
>> +
>> +       SoftReset();
>> +
>> +       // Enable protection (MPU registers only available in privileged
>> mode)
>> +       fRegBase[INTCPS_PROTECTION] |= 1;
>> +}
>> +
>> +
>> +enum {
>> +       TIDR = 0,
>> +       TIOCP_CFG = 4,
>> +       TISTAT,
>> +       TISR,
>> +       TIER,
>> +       TWER,
>> +       TCLR,
>> +       TCRR,
>> +       TLDR,
>> +       TTGR,
>> +       TWPS,
>> +       TMAR,
>> +       TCAR1,
>> +       TSICR,
>> +       TCAR2,
>> +       TPIR,
>> +       TNIR,
>> +       TCVR,
>> +       TOCR,
>> +       TOWR,
>> +};
>> +
>> +int32
>> +OMAP3Timer::_InterruptWrapper(void *data)
>> +{
>> +       return ((OMAP3Timer*)data)->HandleInterrupt();
>> +}
>> +
>> +
>> +int32
>> +OMAP3Timer::HandleInterrupt()
>> +{
>> +       uint32 ints = fRegBase[TISR] & 7;
>> +
>> +       if (ints & 1) { // Match?
>> +               dprintf("OMAP3Timer: match!\n");
>> +               timer_interrupt();
>> +       } else if (ints & 2) { // Overflow?
>> +               dprintf("OMAP3Timer: overflow!\n");
>> +               fSystemTime += UINT_MAX +1;
>> +       } else if (ints & 4) { // Capture?
>> +               dprintf("OMAP3Timer: capture!\n");
>> +       }
>> +
>> +       // clear interrupt
>> +       fRegBase[TISR] = ints;
>> +
>> +       return B_HANDLED_INTERRUPT;
>> +}
>> +
>> +
>> +void
>> +OMAP3Timer::SetTimeout(bigtime_t timeout)
>> +{
>> +       fRegBase[TMAR] = fRegBase[TCRR] + timeout / 1000ULL;
>> +       fRegBase[TIER] |= 1; // Enable match interrupt
>> +}
>> +
>> +
>> +bigtime_t
>> +OMAP3Timer::Time()
>> +{
>> +       return fSystemTime + fRegBase[TCRR];
>> +}
>> +
>> +
>> +void
>> +OMAP3Timer::Clear()
>> +{
>> +       fRegBase[TIER] &= ~1; // Disable match interrupt
>> +}
>> +
>> +
>> +OMAP3Timer::OMAP3Timer(fdt_module_info *fdtModule, fdt_device_node node)
>> +       : HardwareTimer(fdtModule, node),
>> +       fSystemTime(0)
>> +{
>> +       fRegArea = fFDT->map_reg_range(node, 0, (void**)&fRegBase);
>> +       if (fRegArea < 0)
>> +               panic("Cannot map OMAP3Timer registers!");
>> +
>> +       fInterrupt = fFDT->get_interrupt(node, 0);
>> +       if (fInterrupt < 0)
>> +               panic("Cannot get OMAP3Timer interrupt!");
>> +
>> +       uint32 rev = fRegBase[TIDR];
>> +       dprintf("OMAP: Found timer @ 0x%p, IRQ %d (rev %ld.%ld)\n",
>> fRegBase, fInterrupt, (rev >> 4) & 0xf, rev & 0xf);
>> +
>> +       // Let the timer run (so we can use it as clocksource)
>> +       fRegBase[TCLR] |= 1;
>> +       fRegBase[TIER] = 2; // Enable overflow interrupt
>> +
>> +       install_io_interrupt_handler(fInterrupt,
>> &OMAP3Timer::_InterruptWrapper, this, 0);specificaton
>> +}
>> diff --git a/src/system/kernel/arch/arm/soc_omap3.h
>> b/src/system/kernel/arch/arm/soc_omap3.h
>> new file mode 100644
>> index 0000000..369c0e4
>> --- /dev/null
>> +++ b/src/system/kernel/arch/arm/soc_omap3.h
>> @@ -0,0 +1,62 @@
>> +#ifndef ARCH_ARM_SOC_OMAP3_H
>> +#define ARCH_ARM_SOC_OMAP3_H
>> +
>> +class OMAP3InterruptController;
>> +
>> +#include "soc.h"
>> +
>> +#include <new>
>> +
>> +
>> +class OMAP3InterruptController : public InterruptController {
>>
>
> 'final' should be added.
>
>
>> +public:
>> +       void EnableInterrupt(int irq);
>> +       void DisableInterrupt(int irq);
>> +       void HandleInterrupt();
>>
>
> All these function declarations lack 'override' keyword.
>
>
>> +
>> +       static status_t Init(fdt_module_info *fdt, fdt_device_node node,
>> void *cookie) {
>> +               InterruptController *ic = new(std::nothrow)
>> OMAP3InterruptController(fdt, node);
>> +               // XXX implement InitCheck() functionality
>> +               return ic != NULL ? B_OK : B_NO_MEMORY;
>> +       }
>>
>
> Isn't it too complicated? Currently the initialization of interrupt
> controller is: ConcreteInterruptController::Init() ->
> ConcreteInterruptController::(constructor) ->
> InterruptController::(constructor). Do we really need to involve 3
> functions in that? Why can't we just have global pointer to the current, do
> all initialization stuff inside ConcreteInterruptController::Init() and
> have both constructors empty? True, there will be a duplication of the
> check whether another gGlobalPointerToInterruptController is nullptr in
> each implementation of ConcreteInterruptController, but duplication of a
> single assert() is IMO much better solution that needless complication of
> the code and scattering initialization logic among three functions.
>
>
>> +protected:
>> +       OMAP3InterruptController(fdt_module_info *fdt, fdt_device_node
>> node);
>> +
>> +       void SoftReset();
>> +
>> +       area_id fRegArea;
>> +       uint32 *fRegBase;
>> +       uint32 fNumPending;
>> +};
>> +
>> +class OMAP3Timer : public HardwareTimer {
>> +public:
>> +       void SetTimeout(bigtime_t timeout);
>> +       bigtime_t Time();
>> +       void Clear();
>> +
>> +       static status_t Init(fdt_module_info *fdt, fdt_device_node node,
>> void *cookie) {
>> +               if (sInstance == NULL) {
>> +                       OMAP3Timer *timer = new(std::nothrow)
>> OMAP3Timer(fdt, node);
>> +                       // XXX implement InitCheck() functionality
>> +                       return timer != NULL ? B_OK : B_NO_MEMORY;
>> +               } else {
>> +                       // XXX We have multiple timers; just create the
>> first one
>> +                       // and ignore the rest
>> +                       return B_OK;
>> +               }
>> +       }
>> +
>> +private:
>> +       OMAP3Timer(fdt_module_info *fdtModule, fdt_device_node node);
>> +
>> +       static int32 _InterruptWrapper(void *data);
>> +       int32 HandleInterrupt();
>> +
>> +       bigtime_t fSystemTime;
>> +       area_id fRegArea;
>> +       uint32 *fRegBase;
>> +       int fInterrupt;
>> +};
>> +
>> +#endif /* ARCH_ARM_SOC_OMAP3_H */
>> diff --git a/src/system/kernel/arch/arm/soc_pxa.cpp
>> b/src/system/kernel/arch/arm/soc_pxa.cpp
>> new file mode 100644
>> index 0000000..5a3730a
>> --- /dev/null
>> +++ b/src/system/kernel/arch/arm/soc_pxa.cpp
>> @@ -0,0 +1,154 @@
>> +#include "soc_pxa.h"
>> +
>> +/* PXA Interrupt Controller Registers */
>> +#define PXA_ICIP       0x00
>> +#define PXA_ICMR       0x01
>> +#define PXA_ICFP       0x03
>> +#define PXA_ICMR2      0x28
>>
>
> What abot using enum class instead?
>
> enum class pxa {
>     icip = 0,
>     icmr = 1,
>     icfp = 3,
>     icmr2 = 0x28,
> };
>
>
>> +
>> +void
>> +PXAInterruptController::EnableInterrupt(int irq)
>> +{
>> +       if (irq <= 31) {
>> +               fRegBase[PXA_ICMR] |= 1 << irq;
>> +               return;
>> +       }
>> +
>> +       fRegBase[PXA_ICMR2] |= 1 << (irq - 32);
>> +}
>> +
>> +
>> +void
>> +PXAInterruptController::DisableInterrupt(int irq)
>> +{
>> +       if (irq <= 31) {
>> +               fRegBase[PXA_ICMR] &= ~(1 << irq);
>> +               return;
>> +       }
>> +
>> +       fRegBase[PXA_ICMR2] &= ~(1 << (irq - 32));
>> +}
>> +
>> +
>> +void
>> +PXAInterruptController::HandleInterrupt()
>> +{
>> +       for (int i=0; i < 32; i++) {
>> +               if (fRegBase[PXA_ICIP] & (1 << i))
>> +                       int_io_interrupt_handler(i, true);
>> +       }
>> +}
>> +
>> +
>> +PXAInterruptController::PXAInterruptController(fdt_module_info *fdt,
>> fdt_device_node node)
>> +       : InterruptController(fdt, node) {
>> +       fRegArea = fFDT->map_reg_range(node, 0, (void**)&fRegBase);
>> +       if (fRegArea < 0)
>> +               panic("PXAInterruptController: cannot map registers!");
>> +
>> +       fRegBase[PXA_ICMR] = 0;
>> +       fRegBase[PXA_ICMR2] = 0;
>> +}
>> +
>> +#define PXA_TIMERS_INTERRUPT   7 /* OST_4_11 */
>> +
>> +#define PXA_OSSR       0x05
>> +#define PXA_OIER       0x07
>> +#define PXA_OSCR4      0x10
>> +#define PXA_OSCR5      0x11
>> +#define PXA_OSMR4      0x20
>> +#define PXA_OSMR5      0x21
>> +#define PXA_OMCR4      0x30
>> +#define PXA_OMCR5      0x31
>> +
>> +#define PXA_RES_S      (3 << 0)
>> +#define PXA_RES_MS     (1 << 1)
>> +#define PXA_RES_US     (1 << 2)
>> +
>> +#define US2S(bt)       ((bt) / 1000000ULL)
>> +#define US2MS(bt)      ((bt) / 1000ULL)
>>
>
> Again, enum classes and constexpr consts would be nicer than defines. US2S
> and US2MS should be a static inline functions, although use of
> std::chrono::duration probably would be even better.
>
>
>> +
>> +void
>> +PXATimer::SetTimeout(bigtime_t timeout)
>> +{
>> +       uint32 val = timeout & UINT_MAX;
>> +       uint32 res = PXA_RES_US;
>> +
>> +       if (timeout & ~UINT_MAX) {
>> +               // Does not fit, so scale resolution down to milliseconds
>> +               if (US2MS(timeout) & ~UINT_MAX) {
>> +                       // Still does not fit, scale down to seconds as
>> last ditch attempt
>> +                       val = US2S(timeout) & UINT_MAX;
>> +                       res = PXA_RES_S;
>> +               } else {
>> +                       // Fits in millisecond resolution
>> +                       val = US2MS(timeout) & UINT_MAX;
>> +                       res = PXA_RES_MS;
>> +               }
>> +       }
>> +
>> +       dprintf("arch_timer_set_hardware_timer(val=%lu, res=%lu)\n", val,
>> res);
>> +       fRegBase[PXA_OIER] |= (1 << 4);
>> +       fRegBase[PXA_OMCR4] = res;
>> +       fRegBase[PXA_OSMR4] = val;
>> +       fRegBase[PXA_OSCR4] = 0; // start counting from 0 again
>> +}
>> +
>> +void
>> +PXATimer::Clear()
>> +{
>> +       fRegBase[PXA_OMCR4] = 0; // disable our timer
>> +       fRegBase[PXA_OIER] &= ~(1 << 4);
>> +}
>> +
>> +
>> +bigtime_t
>> +PXATimer::Time()
>> +{
>> +       if (fRegArea < 0)
>> +               return 0;
>> +
>> +       return (fRegBase != NULL) ?
>> +               fSystemTime + fRegBase[PXA_OSCR5] :
>> +               0ULL;
>> +}
>> +
>> +
>> +int32
>> +PXATimer::_InterruptWrapper(void *data)
>> +{
>> +       return ((PXATimer*)data)->HandleInterrupt();
>> +}
>> +
>> +
>> +int32
>> +PXATimer::HandleInterrupt()
>> +{
>> +       if (fRegBase[PXA_OSSR] & (1 << 4)) {
>> +               fRegBase[PXA_OSSR] |= (1 << 4);
>> +               return timer_interrupt();
>> +       }
>> +
>> +       if (fRegBase[PXA_OSSR]  & (1 << 5)) {
>> +               fRegBase[PXA_OSSR] |= (1 << 5);
>>
>
> Magic values: (1 << 4) and (1 << 5).
>
>
>> +               fSystemTime += UINT_MAX + 1ULL;
>> +       }
>> +
>> +       return B_HANDLED_INTERRUPT;
>> +}
>> +
>> +
>> +PXATimer::PXATimer(fdt_module_info *fdt, fdt_device_node node)
>> +       : HardwareTimer(fdt, node)
>> +{
>> +       fRegArea = fFDT->map_reg_range(node, 0, (void**)&fRegBase);
>> +       if (fRegArea < 0)
>> +               panic("Cannot map PXATimer registers!");
>> +
>> +       fRegBase[PXA_OIER] |= (1 << 5); // enable timekeeping timer
>> +       fRegBase[PXA_OMCR5] = PXA_RES_US | (1 << 7);
>> +       fRegBase[PXA_OSMR5] = UINT_MAX;
>> +       fRegBase[PXA_OSCR5] = 0;
>> +
>> +       install_io_interrupt_handler(PXA_TIMERS_INTERRUPT,
>> &PXATimer::_InterruptWrapper, NULL, 0);
>> +}
>> diff --git a/src/system/kernel/arch/arm/soc_pxa.h
>> b/src/system/kernel/arch/arm/soc_pxa.h
>> new file mode 100644
>> index 0000000..996a312
>> --- /dev/null
>> +++ b/src/system/kernel/arch/arm/soc_pxa.h
>> @@ -0,0 +1,55 @@
>> +#ifndef ARCH_ARM_SOC_PXA_H
>> +#define ARCH_ARM_SOC_PXA_H
>> +
>> +class PXAInterruptController;
>> +
>> +#include "soc.h"
>> +
>> +#include <new>
>> +
>> +
>> +class PXAInterruptController : public InterruptController {
>> +public:
>> +       void EnableInterrupt(int irq);
>> +       void DisableInterrupt(int irq);
>> +       void HandleInterrupt();
>>
>
> 'final' and 'override'
>
>
>> +
>> +       static status_t Init(fdt_module_info *fdt, fdt_device_node node,
>> void *cookie) {
>> +               InterruptController *ic = new(std::nothrow)
>> PXAInterruptController(fdt, node);
>> +               // XXX implement InitCheck() functionality
>> +               return ic != NULL ? B_OK : B_NO_MEMORY;
>> +       }
>> +
>> +protected:
>> +       PXAInterruptController(fdt_module_info *fdt, fdt_device_node
>> node);
>> +
>> +       area_id fRegArea;
>> +       uint32 *fRegBase;
>> +};
>> +
>> +
>> +class PXATimer : public HardwareTimer {
>> +public:
>> +       void SetTimeout(bigtime_t timeout);
>> +       void Clear();
>> +       bigtime_t Time();
>> +
>> +       static status_t Init(fdt_module_info *fdt, fdt_device_node node,
>> void *cookie) {
>> +               PXATimer *timer = new(std::nothrow) PXATimer(fdt, node);
>> +               // XXX implement InitCheck() functionality
>> +               return timer != NULL ? B_OK : B_NO_MEMORY;
>> +       }
>> +
>> +protected:
>> +       PXATimer(fdt_module_info *fdt, fdt_device_node node);
>> +
>> +       area_id fRegArea;
>> +       uint32 *fRegBase;
>> +       bigtime_t fSystemTime;
>> +private:
>> +       static int32 _InterruptWrapper(void *data);
>> +       int32 HandleInterrupt();
>> +};
>> +
>> +
>> +#endif // ARCH_ARM_SOC_PXA_H
>>
>>
>>
>

Other related posts: