From f4cbb9498f516630e16a00c0de964d7d2af7c9d5 Mon Sep 17 00:00:00 2001 From: "Justin C. Miller" Date: Sun, 12 Jul 2020 17:43:37 -0700 Subject: [PATCH] [kernel] Fix clock period vs frequency error Calling `spinwait()` was hanging due to improper computation of the clock rate because justin did a dumb at math. Also the period can be greater than 1ns, so the clock's units were updated to microseconds. --- src/kernel/apic.cpp | 3 +-- src/kernel/clock.cpp | 4 ++-- src/kernel/clock.h | 12 ++++++------ src/kernel/device_manager.cpp | 16 +++++++++++++++- src/kernel/device_manager.h | 10 ++++++++++ src/kernel/hpet.cpp | 11 +++++------ src/kernel/hpet.h | 8 ++------ 7 files changed, 41 insertions(+), 23 deletions(-) diff --git a/src/kernel/apic.cpp b/src/kernel/apic.cpp index a39ee0a..e197f11 100644 --- a/src/kernel/apic.cpp +++ b/src/kernel/apic.cpp @@ -74,8 +74,7 @@ lapic::calibrate_timer() apic_write(m_base, lapic_timer_init, initial); uint64_t us = 200000; - uint64_t ns = us * 1000; - clock::get().spinwait(ns); + clock::get().spinwait(us); uint32_t remaining = apic_read(m_base, lapic_timer_cur); uint32_t ticks_total = initial - remaining; diff --git a/src/kernel/clock.cpp b/src/kernel/clock.cpp index b51b1c1..9196fa8 100644 --- a/src/kernel/clock.cpp +++ b/src/kernel/clock.cpp @@ -14,9 +14,9 @@ clock::clock(uint64_t rate, clock::source source_func, void *data) : } void -clock::spinwait(uint64_t ns) const +clock::spinwait(uint64_t us) const { - uint64_t when = m_source(m_data) + ns; + uint64_t when = m_source(m_data) + us; while (value() < when); } diff --git a/src/kernel/clock.h b/src/kernel/clock.h index bf4657f..1c35694 100644 --- a/src/kernel/clock.h +++ b/src/kernel/clock.h @@ -12,13 +12,13 @@ public: using source = uint64_t (*)(void*); /// Constructor. - /// \arg rate Number of source ticks per ns + /// \arg rate Number of source ticks per us /// \arg source Function for the clock source /// \arg data Data to pass to the source function clock(uint64_t rate, source source_func, void *data); /// Get the current value of the clock. - /// \returns Current value of the source, in ns + /// \returns Current value of the source, in us /// TODO: optimize divison by finding a multiply and /// shift value instead inline uint64_t value() const { return m_source(m_data) / m_rate; } @@ -28,15 +28,15 @@ public: inline void update() { m_current = value(); } /// Wait in a tight loop - /// \arg interval Time to wait, in ns - void spinwait(uint64_t ns) const; + /// \arg interval Time to wait, in us + void spinwait(uint64_t us) const; /// Get the master clock static clock & get() { return *s_instance; } private: - uint64_t m_current; ///< current ns count - uint64_t m_rate; ///< source ticks per ns + uint64_t m_current; ///< current us count + uint64_t m_rate; ///< source ticks per us void *m_data; source m_source; diff --git a/src/kernel/device_manager.cpp b/src/kernel/device_manager.cpp index 6d979de..5adf68f 100644 --- a/src/kernel/device_manager.cpp +++ b/src/kernel/device_manager.cpp @@ -47,7 +47,6 @@ acpi_table_header::validate(uint32_t expected_type) const return !expected_type || (expected_type == type); } - void irq2_callback(void *) { } @@ -344,6 +343,7 @@ device_manager::init_drivers() log::info(logs::clock, "Created master clock using HPET 0: Rate %d", h.rate()); } else { //TODO: APIC clock? + kassert(0, "No HPET master clock"); } } @@ -360,6 +360,20 @@ device_manager::install_irq(unsigned irq, const char *name, irq_callback cb, voi return true; } +bool +device_manager::uninstall_irq(unsigned irq, irq_callback cb, void *data) +{ + if (irq >= m_irqs.count()) + return false; + + const irq_allocation &existing = m_irqs[irq]; + if (existing.callback != cb || existing.data != data) + return false; + + m_irqs[irq] = {nullptr, nullptr, nullptr}; + return true; +} + bool device_manager::allocate_msi(const char *name, pci_device &device, irq_callback cb, void *data) { diff --git a/src/kernel/device_manager.h b/src/kernel/device_manager.h index 5464a3a..6fdea8c 100644 --- a/src/kernel/device_manager.h +++ b/src/kernel/device_manager.h @@ -55,6 +55,16 @@ public: irq_callback cb, void *data); + /// Uninstall an IRQ callback for a device + /// \arg irq IRQ to install the handler for + /// \arg cb Callback to call when the interrupt is received + /// \arg data Data to pass to the callback + /// \returns True if an IRQ was uninstalled successfully + bool uninstall_irq( + unsigned irq, + irq_callback cb, + void *data); + /// Allocate an MSI IRQ for a device /// \arg name Name of the interrupt, for display to user /// \arg device Device this MSI is being allocated for diff --git a/src/kernel/hpet.cpp b/src/kernel/hpet.cpp index 3d75bb2..d554077 100644 --- a/src/kernel/hpet.cpp +++ b/src/kernel/hpet.cpp @@ -37,7 +37,7 @@ hpet::hpet(uint8_t index, uint64_t *base) : m_timers = ((caps >> 8) & 0x1f) + 1; m_period = (caps >> 32); - // setup_timer_interrupts(0, 2, 1000000, true); + setup_timer_interrupts(0, 2, 1000, true); // bool installed = device_manager::get() // .install_irq(2, "HPET Timer", hpet_irq_callback, this); // kassert(installed, "Installing HPET IRQ handler"); @@ -57,6 +57,7 @@ hpet::hpet(uint8_t index, uint64_t *base) : log::debug(logs::timer, "HPET %d timer %d:", index, i); log::debug(logs::timer, " int type: %d", (config >> 1) & 1); + log::debug(logs::timer, " int enable: %d", (config >> 2) & 1); log::debug(logs::timer, " timer type: %d", (config >> 3) & 1); log::debug(logs::timer, " periodic cap: %d", (config >> 4) & 1); log::debug(logs::timer, " bits: %d", ((config >> 5) & 1) ? 64 : 32); @@ -72,10 +73,10 @@ hpet::hpet(uint8_t index, uint64_t *base) : void hpet::setup_timer_interrupts(unsigned timer, uint8_t irq, uint64_t interval, bool periodic) { - constexpr uint64_t femto_per_ns = 1000000ull; + constexpr uint64_t femto_per_us = 1000000000ull; *timer_comparator(m_base, timer) = *counter_value(m_base) + - (interval * femto_per_ns) / m_period; + (interval * femto_per_us) / m_period; *timer_config(m_base, timer) = (irq << 9) | ((periodic ? 1 : 0) << 3); } @@ -102,9 +103,7 @@ void hpet::enable() { log::debug(logs::timer, "HPET %d enabling", m_index); - - enable_timer(0); - *configuration(m_base) = 1; + *configuration(m_base) = (*configuration(m_base) & 0x3) | 1; } uint64_t diff --git a/src/kernel/hpet.h b/src/kernel/hpet.h index 025137a..c4f6170 100644 --- a/src/kernel/hpet.h +++ b/src/kernel/hpet.h @@ -20,12 +20,8 @@ public: /// Configure the timer and start it running. void enable(); - /// Wait in a tight loop while reading the HPET counter. - /// \arg ns Number of nanoseconds to wait - void spinwait(uint64_t ns) const; - - /// Get the timer rate in ticks per ns - inline uint64_t rate() const { return m_period * 1000000ull; } + /// Get the timer rate in ticks per us + inline uint64_t rate() const { return 1000000000/m_period; } /// Get the current timer value uint64_t value() const;