From 72112c0676487bf5d64c2ba3f6858d8c2007c516 Mon Sep 17 00:00:00 2001 From: Travis Geiselbrecht Date: Tue, 23 Sep 2025 23:16:55 -0700 Subject: [PATCH] [pci] little bit of code cleanup in the pci bus driver Mostly just suggestions by clang-tidy. No functional change. --- dev/bus/pci/bus_mgr/bridge.cpp | 38 +++++++++++++++---------------- dev/bus/pci/bus_mgr/bridge.h | 6 ++--- dev/bus/pci/bus_mgr/bus_mgr.cpp | 4 ++-- dev/bus/pci/bus_mgr/bus_mgr.h | 1 - dev/bus/pci/bus_mgr/device.cpp | 9 ++++---- dev/bus/pci/bus_mgr/device.h | 8 +++++++ dev/bus/pci/bus_mgr/resource.h | 4 ++-- dev/bus/pci/debug.cpp | 22 ++++++++++-------- dev/bus/pci/include/dev/bus/pci.h | 9 ++++---- dev/bus/pci/pci_priv.h | 4 ---- 10 files changed, 55 insertions(+), 50 deletions(-) diff --git a/dev/bus/pci/bus_mgr/bridge.cpp b/dev/bus/pci/bus_mgr/bridge.cpp index 0d199288..aa2fdf27 100644 --- a/dev/bus/pci/bus_mgr/bridge.cpp +++ b/dev/bus/pci/bus_mgr/bridge.cpp @@ -142,7 +142,7 @@ void bridge::extend_subordinate_range(uint8_t new_secondary_bus) { DEBUG_ASSERT(subordinate_bus() == new_secondary_bus); // tell the parent bridge that we have a new range - auto *parent_bridge = bus_->get_bridge(); + auto *parent_bridge = parent_bus()->get_bridge(); if (parent_bridge) { parent_bridge->extend_subordinate_range(new_secondary_bus); } @@ -154,12 +154,12 @@ void bridge::assign_bus_numbers(uint8_t primary, uint8_t secondary, uint8_t subo uint32_t temp; - pci_read_config_word(loc_, 0x18, &temp); + pci_read_config_word(loc(), 0x18, &temp); temp &= 0xff000000; // leave latency timer alone temp |= subordinate << 16; temp |= secondary << 8; temp |= primary << 0; - pci_write_config_word(loc_, 0x18, temp); + pci_write_config_word(loc(), 0x18, temp); // reread the config load_config(); @@ -174,7 +174,7 @@ void bridge::dump(size_t indent) { }; char str[14]; scoot(); - printf("bridge %s %04hx:%04hx primary bus %d child busses [%d..%d]\n", pci_loc_string(loc_, str), + printf("bridge %s %04hx:%04hx primary bus %d child busses [%d..%d]\n", pci_loc_string(loc(), str), vendor_id(), device_id(), primary_bus(), secondary_bus(), subordinate_bus()); @@ -186,11 +186,11 @@ void bridge::dump(size_t indent) { mr.base, mr.limit, ir.base, ir.limit, pr.base, pr.limit); for (size_t b = 0; b < 2; b++) { - if (bars_[b].valid) { + if (bar(b).valid) { for (size_t i = 0; i < indent + 1; i++) { printf(" "); } - printf("BAR %zu: addr %#" PRIx64 " size %#zx io %d valid %d\n", b, bars_[b].addr, bars_[b].size, bars_[b].io, bars_[b].valid); + printf("BAR %zu: addr %#" PRIx64 " size %#zx io %d valid %d\n", b, bar(b).addr, bar(b).size, bar(b).io, bar(b).valid); } } @@ -201,39 +201,39 @@ void bridge::dump(size_t indent) { // accessors to compute the io and memory range of the bridge bridge::range bridge::io_range() { - if (config_.type1.io_limit < config_.type1.io_base) { + if (config().type1.io_limit < config().type1.io_base) { return { 0, 0 }; } else { // TODO: handle 32bit io (does this really exist?) - return { ((uint32_t)config_.type1.io_base >> 4) << 12, - (((uint32_t)config_.type1.io_limit >> 4) << 12) | 0xfff }; + return { ((uint32_t)config().type1.io_base >> 4) << 12, + (((uint32_t)config().type1.io_limit >> 4) << 12) | 0xfff }; } } bridge::range bridge::mem_range() { - if (config_.type1.memory_limit < config_.type1.memory_base) { + if (config().type1.memory_limit < config().type1.memory_base) { return { 0, 0 }; } else { - return { ((uint32_t)config_.type1.memory_base >> 4) << 20, - (((uint32_t)config_.type1.memory_limit >> 4) << 20) | 0xfffff }; + return { ((uint32_t)config().type1.memory_base >> 4) << 20, + (((uint32_t)config().type1.memory_limit >> 4) << 20) | 0xfffff }; } } bridge::range bridge::prefetch_range() { - if (config_.type1.prefetchable_memory_limit < config_.type1.prefetchable_memory_base) { + if (config().type1.prefetchable_memory_limit < config().type1.prefetchable_memory_base) { return { 0, 0 }; } else { - bool is_64 = (config_.type1.prefetchable_memory_base & 0xf) == 1; + bool is_64 = (config().type1.prefetchable_memory_base & 0xf) == 1; uint64_t base, limit; - base = (((uint64_t)config_.type1.prefetchable_memory_base >> 4) << 20); + base = (((uint64_t)config().type1.prefetchable_memory_base >> 4) << 20); if (is_64) { - base |= (uint64_t)config_.type1.prefetchable_base_upper << 32; + base |= (uint64_t)config().type1.prefetchable_base_upper << 32; } - limit = (((uint64_t)config_.type1.prefetchable_memory_limit >> 4) << 20) | 0xfffff; + limit = (((uint64_t)config().type1.prefetchable_memory_limit >> 4) << 20) | 0xfffff; if (is_64) { - limit |= (uint64_t)config_.type1.prefetchable_limit_upper << 32; + limit |= (uint64_t)config().type1.prefetchable_limit_upper << 32; } return { base, limit }; } @@ -403,7 +403,7 @@ status_t bridge::assign_resource(bar_alloc_request *request, uint64_t address) { return ERR_NOT_SUPPORTED; } // assert that the device supports 64bit addresses - DEBUG_ASSERT((config_.type1.prefetchable_memory_base & 0xf) == 1); + DEBUG_ASSERT((config().type1.prefetchable_memory_base & 0xf) == 1); DEBUG_ASSERT(IS_ALIGNED(address, (1UL << 20))); DEBUG_ASSERT(IS_ALIGNED(request->size, (1UL << 20))); diff --git a/dev/bus/pci/bus_mgr/bridge.h b/dev/bus/pci/bus_mgr/bridge.h index ac656146..dc4f93ed 100644 --- a/dev/bus/pci/bus_mgr/bridge.h +++ b/dev/bus/pci/bus_mgr/bridge.h @@ -45,9 +45,9 @@ public: void dump(size_t indent = 0) override; // config accessors - uint8_t primary_bus() const { return config_.type1.primary_bus; } - uint8_t secondary_bus() const { return config_.type1.secondary_bus; } - uint8_t subordinate_bus() const { return config_.type1.subordinate_bus; } + uint8_t primary_bus() const { return config().type1.primary_bus; } + uint8_t secondary_bus() const { return config().type1.secondary_bus; } + uint8_t subordinate_bus() const { return config().type1.subordinate_bus; } template struct range { diff --git a/dev/bus/pci/bus_mgr/bus_mgr.cpp b/dev/bus/pci/bus_mgr/bus_mgr.cpp index add8ac4a..8ecf4ff5 100644 --- a/dev/bus/pci/bus_mgr/bus_mgr.cpp +++ b/dev/bus/pci/bus_mgr/bus_mgr.cpp @@ -34,12 +34,12 @@ namespace pci { bus *root = nullptr; list_node bus_list = LIST_INITIAL_VALUE(bus_list); +namespace { + uint8_t last_bus = 0; resource_allocator resources; -namespace { - // local helper routines // iterate all devices on all busses with the functor template diff --git a/dev/bus/pci/bus_mgr/bus_mgr.h b/dev/bus/pci/bus_mgr/bus_mgr.h index c4282a38..664dfe46 100644 --- a/dev/bus/pci/bus_mgr/bus_mgr.h +++ b/dev/bus/pci/bus_mgr/bus_mgr.h @@ -10,7 +10,6 @@ #include #include #include -#include #include #include diff --git a/dev/bus/pci/bus_mgr/device.cpp b/dev/bus/pci/bus_mgr/device.cpp index ac071d63..9463c72a 100644 --- a/dev/bus/pci/bus_mgr/device.cpp +++ b/dev/bus/pci/bus_mgr/device.cpp @@ -120,9 +120,10 @@ void device::dump(size_t indent) { printf(" "); } char str[14]; - printf("dev %s vid:pid %04hx:%04hx base:sub:intr %hhu:%hhu:%hhu %s%s\n", + printf("dev %s vid:pid %04hx:%04hx base:sub:intr %hhu:%hhu:%hhu int %u %s%s\n", pci_loc_string(loc_, str), config_.vendor_id, config_.device_id, base_class(), sub_class(), interface(), + config_.type0.interrupt_line, has_msi() ? "msi " : "", has_msix() ? "msix " : ""); for (size_t b = 0; b < countof(bars_); b++) { @@ -394,7 +395,7 @@ status_t device::load_bars() { bars_[i].prefetchable = bar_addr & (1<<3); bars_[i].size_64 = true; bars_[i].addr = bar_addr & ~0xf; - bars_[i].addr |= (uint64_t)config_.type0.base_addresses[i + 1] << 32; + bars_[i].addr |= static_cast(config_.type0.base_addresses[i + 1]) << 32; // probe size by writing all 1s and seeing what bits are masked uint64_t size; @@ -404,12 +405,12 @@ status_t device::load_bars() { size = size32; pci_write_config_word(loc_, PCI_CONFIG_BASE_ADDRESSES + i * 4 + 4, 0xffffffff); pci_read_config_word(loc_, PCI_CONFIG_BASE_ADDRESSES + i * 4 + 4, &size32); - size |= (uint64_t)size32 << 32; + size |= static_cast(size32) << 32; pci_write_config_word(loc_, PCI_CONFIG_BASE_ADDRESSES + i * 4, bars_[i].addr); pci_write_config_word(loc_, PCI_CONFIG_BASE_ADDRESSES + i * 4 + 4, bars_[i].addr >> 32); // mask out bottom bits, invert and add 1 to compute size - bars_[i].size = (~(size & ~(uint64_t)0b1111)) + 1; + bars_[i].size = (~(size & ~static_cast(0b1111))) + 1; bars_[i].valid = (bars_[i].size != 0); diff --git a/dev/bus/pci/bus_mgr/device.h b/dev/bus/pci/bus_mgr/device.h index ae2b9f28..32e6ca76 100644 --- a/dev/bus/pci/bus_mgr/device.h +++ b/dev/bus/pci/bus_mgr/device.h @@ -97,6 +97,14 @@ public: virtual void dump(size_t indent = 0); protected: + const pci_config_t &config() const { return config_; } + const pci_bar_t &bar(size_t index) const { + DEBUG_ASSERT(index < 6); + return bars_[index]; + } + bus *parent_bus() const { return bus_; } + +private: // let the bus device directly manipulate our list node friend class bus; list_node node = LIST_INITIAL_CLEARED_VALUE; diff --git a/dev/bus/pci/bus_mgr/resource.h b/dev/bus/pci/bus_mgr/resource.h index 07322164..71abdbaa 100644 --- a/dev/bus/pci/bus_mgr/resource.h +++ b/dev/bus/pci/bus_mgr/resource.h @@ -19,7 +19,7 @@ struct resource_range { uint64_t base; uint64_t size; - void dump() { + void dump() const { printf("resource type %d: base %#llx size %#llx\n", type, base, size); } @@ -32,7 +32,7 @@ struct resource_range_set { resource_range mmio_prefetchable; resource_range mmio64_prefetchable; - void dump() { + void dump() const { printf("resource range set:\n"); io.dump(); mmio.dump(); diff --git a/dev/bus/pci/debug.cpp b/dev/bus/pci/debug.cpp index e84bcf9d..2b7d3f35 100644 --- a/dev/bus/pci/debug.cpp +++ b/dev/bus/pci/debug.cpp @@ -17,19 +17,20 @@ #include #include +namespace { /* * enumerates pci devices */ -static void pci_list(void) { +void pci_list() { pci_location_t state; int busses = 0, devices = 0, ret; printf("Scanning (brute force)...\n"); - for (int segment = 0; segment <= (int)pci_get_last_segment(); segment++) { + for (int segment = 0; segment <= pci_get_last_segment(); segment++) { state.segment = segment; - for (int bus = 0; bus <= (int)pci_get_last_bus(); bus++) { + for (int bus = 0; bus <= pci_get_last_bus(); bus++) { state.bus = bus; busses++; @@ -83,7 +84,7 @@ error: * a somewhat fugly pci config space examine/modify command. this should probably * be broken up a bit. */ -static int pci_config(int argc, const console_cmd_args *argv) { +int pci_config(int argc, const console_cmd_args *argv) { pci_location_t loc; pci_config_t config; uint32_t offset; @@ -146,12 +147,11 @@ static int pci_config(int argc, const console_cmd_args *argv) { break; case 1: // 64 bit prefetchable addressing printf("prefetchable base=%llx prefetchable limit=%llx\n", - ((uint64_t)config.type1.prefetchable_memory_base & 0xfff0) << 16 | - ((uint64_t)config.type1.prefetchable_base_upper << 32), - ((uint64_t)config.type1.prefetchable_memory_limit & 0xfff0) << 16 | 0xf'ffff | - ((uint64_t)config.type1.prefetchable_limit_upper << 32)); + (static_cast(config.type1.prefetchable_memory_base) & 0xfff0) << 16 | + (static_cast(config.type1.prefetchable_base_upper) << 32), + (static_cast(config.type1.prefetchable_memory_limit) & 0xfff0) << 16 | 0xf'ffff | + (static_cast(config.type1.prefetchable_limit_upper) << 32)); break; - } } hexdump8_ex(&config, sizeof(config), 0); @@ -257,7 +257,7 @@ error: return -2; } -static int pci_cmd(int argc, const console_cmd_args *argv) { +int pci_cmd(int argc, const console_cmd_args *argv) { if (argc < 2) { printf("pci commands:\n"); usage: @@ -287,3 +287,5 @@ STATIC_COMMAND_START STATIC_COMMAND("pci", "pci toolbox", &pci_cmd) STATIC_COMMAND_END(pcitests); +} // namespace + diff --git a/dev/bus/pci/include/dev/bus/pci.h b/dev/bus/pci/include/dev/bus/pci.h index 0368205e..3ccd4dba 100644 --- a/dev/bus/pci/include/dev/bus/pci.h +++ b/dev/bus/pci/include/dev/bus/pci.h @@ -8,7 +8,6 @@ */ #pragma once -#include #include #include @@ -87,16 +86,16 @@ status_t pci_bus_mgr_find_device(pci_location_t *state, uint16_t device_id, uint status_t pci_bus_mgr_find_device_by_class(pci_location_t *state, uint8_t base_class, uint8_t subclass, uint8_t interface, size_t index); // set io and mem enable on the device -status_t pci_bus_mgr_enable_device(const pci_location_t loc); +status_t pci_bus_mgr_enable_device(pci_location_t loc); // read a list of up to 6 bars out of the device. each is marked with a valid bit -status_t pci_bus_mgr_read_bars(const pci_location_t loc, pci_bar_t bar[6]); +status_t pci_bus_mgr_read_bars(pci_location_t loc, pci_bar_t bar[6]); // try to allocate one or more msi vectors for this device -status_t pci_bus_mgr_allocate_msi(const pci_location_t loc, size_t num_requested, uint *irqbase); +status_t pci_bus_mgr_allocate_msi(pci_location_t loc, size_t num_requested, uint *irqbase); // allocate a regular irq for this device and return it in irqbase -status_t pci_bus_mgr_allocate_irq(const pci_location_t loc, uint *irqbase); +status_t pci_bus_mgr_allocate_irq(pci_location_t loc, uint *irqbase); // return a pointer to a formatted string const char *pci_loc_string(pci_location_t loc, char out_str[14]); diff --git a/dev/bus/pci/pci_priv.h b/dev/bus/pci/pci_priv.h index e62ba699..251419ab 100644 --- a/dev/bus/pci/pci_priv.h +++ b/dev/bus/pci/pci_priv.h @@ -8,13 +8,9 @@ */ #pragma once -#include -#include #include - #include "backend/bios32.h" #include "backend/ecam.h" #include "backend/type1.h" - #include "bus_mgr/bus_mgr.h"