[haiku-commits] Re: haiku: hrev48180 - src/system/kernel/arch/arm
- From: Paweł Dziepak <pdziepak@xxxxxxxxxxx>
- To: haiku-commits@xxxxxxxxxxxxx
- Date: Fri, 31 Oct 2014 14:29:48 +0100
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:
