Some cleanups to the pciinit.c code.
These are some things I noticed while tracking down the recent bug reports caused by calling ALIGN_DOWN with a zero alignment.
This second version of the series is less ambitious than the first version - it's limited to simpler changes that are more obvious.
-Kevin
Kevin O'Connor (4): Use standard formatting for PCI info during PCI init pass. Separate pciinit.c into clearly delineated sections. Use pci->header_type in pci_bar() to avoid unnecessary pci_config_readb. Simplify pci_slot_get_irq().
src/pciinit.c | 301 +++++++++++++++++++++++++++++--------------------------- 1 files changed, 156 insertions(+), 145 deletions(-)
Format BDF (bus, device, fn), vendor:device, and prefmem debug output in a more user-readable format.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 12 +++++++----- 1 files changed, 7 insertions(+), 5 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index 0d8758e..aa7c6af 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -235,8 +235,8 @@ static void pci_bios_init_device(struct pci_device *pci) u16 bdf = pci->bdf; int pin, pic_irq;
- dprintf(1, "PCI: bus=%d devfn=0x%02x: vendor_id=0x%04x device_id=0x%04x\n" - , pci_bdf_to_bus(bdf), pci_bdf_to_devfn(bdf) + dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%04x:%04x\n" + , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf) , pci->vendor, pci->device); pci_init_device(pci_class_tbl, pci, NULL);
@@ -481,7 +481,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) dev->bars[i].size); dprintf(1, " bar %d, addr %x, size %x [%s]\n", i, addr, dev->bars[i].size, - dev->bars[i].addr & PCI_BASE_ADDRESS_SPACE_IO ? "io" : "mem"); + region_type_name[pci_addr_to_type(dev->bars[i].addr)]); pci_set_io_region_addr(bdf, i, addr);
if (dev->bars[i].is64) { @@ -507,9 +507,11 @@ static void pci_bios_map_device_in_bus(int bus) struct pci_device *pci;
foreachpci(pci) { - if (pci_bdf_to_bus(pci->bdf) != bus) + u16 bdf = pci->bdf; + if (pci_bdf_to_bus(bdf) != bus) continue; - dprintf(1, "PCI: map device bus %d, bfd 0x%x\n", bus, pci->bdf); + dprintf(1, "PCI: map device bdf=%02x:%02x.%x\n" + , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf)); pci_bios_map_device(&busses[bus], pci); } }
There are four separate phases of the current PCI initialization code: bus initialization, bus sizing, bar allocation, and misc device init. Move the code exclusively called in each phase next to each other, and clearly mark each section.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 221 +++++++++++++++++++++++++++++++-------------------------- 1 files changed, 121 insertions(+), 100 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index aa7c6af..a376ebc 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -45,11 +45,6 @@ static struct pci_bus { } *busses; static int busses_count;
-static void pci_bios_init_device_in_bus(int bus); -static void pci_bios_check_device_in_bus(int bus); -static void pci_bios_init_bus_bases(struct pci_bus *bus); -static void pci_bios_map_device_in_bus(int bus); - static int pci_size_to_index(u32 size, enum pci_region_type type) { int index = __fls(size); @@ -79,17 +74,6 @@ static enum pci_region_type pci_addr_to_type(u32 addr) return PCI_REGION_TYPE_MEM; }
-static u32 pci_size_roundup(u32 size) -{ - int index = __fls(size-1)+1; - return 0x1 << index; -} - -/* host irqs corresponding to PCI irqs A-D */ -const u8 pci_irqs[4] = { - 10, 10, 11, 11 -}; - static u32 pci_bar(u16 bdf, int region_num) { if (region_num != PCI_ROM_SLOT) { @@ -111,6 +95,16 @@ static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr) pci_config_writel(bdf, ofs, addr); }
+ +/**************************************************************** + * Misc. device init + ****************************************************************/ + +/* host irqs corresponding to PCI irqs A-D */ +const u8 pci_irqs[4] = { + 10, 10, 11, 11 +}; + /* return the global irq number corresponding to a given device irq pin. We could also use the bus number to have a more precise mapping. */ @@ -150,13 +144,6 @@ static const struct pci_device_id pci_isa_bridge_tbl[] = { PCI_DEVICE_END };
-#define PCI_IO_ALIGN 4096 -#define PCI_IO_SHIFT 8 -#define PCI_MEMORY_ALIGN (1UL << 20) -#define PCI_MEMORY_SHIFT 16 -#define PCI_PREF_MEMORY_ALIGN (1UL << 20) -#define PCI_PREF_MEMORY_SHIFT 16 - static void storage_ide_init(struct pci_device *pci, void *arg) { u16 bdf = pci->bdf; @@ -267,6 +254,11 @@ static void pci_bios_init_device_in_bus(int bus) } }
+ +/**************************************************************** + * Bus initialization + ****************************************************************/ + static void pci_bios_init_bus_rec(int bus, u8 *pci_bus) { @@ -336,6 +328,17 @@ pci_bios_init_bus(void) busses_count = pci_bus + 1; }
+ +/**************************************************************** + * Bus sizing + ****************************************************************/ + +static u32 pci_size_roundup(u32 size) +{ + int index = __fls(size-1)+1; + return 0x1 << index; +} + static void pci_bios_bus_get_bar(struct pci_bus *bus, int bdf, int bar, u32 *val, u32 *size) { @@ -370,15 +373,7 @@ static void pci_bios_bus_reserve(struct pci_bus *bus, int type, u32 size) bus->r[type].max = size; }
-static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size) -{ - u32 index, addr; - - index = pci_size_to_index(size, type); - addr = bus->r[type].bases[index]; - bus->r[type].bases[index] += pci_index_to_size(index, type); - return addr; -} +static void pci_bios_check_device_in_bus(int bus);
static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev) { @@ -430,6 +425,97 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev) } }
+static void pci_bios_check_device_in_bus(int bus) +{ + struct pci_device *pci; + + dprintf(1, "PCI: check devices bus %d\n", bus); + foreachpci(pci) { + if (pci_bdf_to_bus(pci->bdf) != bus) + continue; + pci_bios_check_device(&busses[bus], pci); + } +} + +#define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1) + +static int pci_bios_init_root_regions(u32 start, u32 end) +{ + struct pci_bus *bus = &busses[0]; + + bus->r[PCI_REGION_TYPE_IO].base = 0xc000; + + if (bus->r[PCI_REGION_TYPE_MEM].sum < bus->r[PCI_REGION_TYPE_PREFMEM].sum) { + bus->r[PCI_REGION_TYPE_MEM].base = + ROOT_BASE(end, + bus->r[PCI_REGION_TYPE_MEM].sum, + bus->r[PCI_REGION_TYPE_MEM].max); + bus->r[PCI_REGION_TYPE_PREFMEM].base = + ROOT_BASE(bus->r[PCI_REGION_TYPE_MEM].base, + bus->r[PCI_REGION_TYPE_PREFMEM].sum, + bus->r[PCI_REGION_TYPE_PREFMEM].max); + if (bus->r[PCI_REGION_TYPE_PREFMEM].base >= start) { + return 0; + } + } else { + bus->r[PCI_REGION_TYPE_PREFMEM].base = + ROOT_BASE(end, + bus->r[PCI_REGION_TYPE_PREFMEM].sum, + bus->r[PCI_REGION_TYPE_PREFMEM].max); + bus->r[PCI_REGION_TYPE_MEM].base = + ROOT_BASE(bus->r[PCI_REGION_TYPE_PREFMEM].base, + bus->r[PCI_REGION_TYPE_MEM].sum, + bus->r[PCI_REGION_TYPE_MEM].max); + if (bus->r[PCI_REGION_TYPE_MEM].base >= start) { + return 0; + } + } + return -1; +} + + +/**************************************************************** + * BAR assignment + ****************************************************************/ + +static void pci_bios_init_bus_bases(struct pci_bus *bus) +{ + u32 base, newbase, size; + int type, i; + + for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { + dprintf(1, " type %s max %x sum %x base %x\n", region_type_name[type], + bus->r[type].max, bus->r[type].sum, bus->r[type].base); + base = bus->r[type].base; + for (i = ARRAY_SIZE(bus->r[type].count)-1; i >= 0; i--) { + size = pci_index_to_size(i, type); + if (!bus->r[type].count[i]) + continue; + newbase = base + size * bus->r[type].count[i]; + dprintf(1, " size %8x: %d bar(s), %8x -> %8x\n", + size, bus->r[type].count[i], base, newbase - 1); + bus->r[type].bases[i] = base; + base = newbase; + } + } +} + +static u32 pci_bios_bus_get_addr(struct pci_bus *bus, int type, u32 size) +{ + u32 index, addr; + + index = pci_size_to_index(size, type); + addr = bus->r[type].bases[index]; + bus->r[type].bases[index] += pci_index_to_size(index, type); + return addr; +} + +#define PCI_IO_SHIFT 8 +#define PCI_MEMORY_SHIFT 16 +#define PCI_PREF_MEMORY_SHIFT 16 + +static void pci_bios_map_device_in_bus(int bus); + static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) { u16 bdf = dev->bdf; @@ -490,18 +576,6 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) } }
-static void pci_bios_check_device_in_bus(int bus) -{ - struct pci_device *pci; - - dprintf(1, "PCI: check devices bus %d\n", bus); - foreachpci(pci) { - if (pci_bdf_to_bus(pci->bdf) != bus) - continue; - pci_bios_check_device(&busses[bus], pci); - } -} - static void pci_bios_map_device_in_bus(int bus) { struct pci_device *pci; @@ -516,63 +590,10 @@ static void pci_bios_map_device_in_bus(int bus) } }
-static void pci_bios_init_bus_bases(struct pci_bus *bus) -{ - u32 base, newbase, size; - int type, i; - - for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { - dprintf(1, " type %s max %x sum %x base %x\n", region_type_name[type], - bus->r[type].max, bus->r[type].sum, bus->r[type].base); - base = bus->r[type].base; - for (i = ARRAY_SIZE(bus->r[type].count)-1; i >= 0; i--) { - size = pci_index_to_size(i, type); - if (!bus->r[type].count[i]) - continue; - newbase = base + size * bus->r[type].count[i]; - dprintf(1, " size %8x: %d bar(s), %8x -> %8x\n", - size, bus->r[type].count[i], base, newbase - 1); - bus->r[type].bases[i] = base; - base = newbase; - } - } -} - -#define ROOT_BASE(top, sum, max) ALIGN_DOWN((top)-(sum),(max) ?: 1) - -static int pci_bios_init_root_regions(u32 start, u32 end) -{ - struct pci_bus *bus = &busses[0]; - - bus->r[PCI_REGION_TYPE_IO].base = 0xc000;
- if (bus->r[PCI_REGION_TYPE_MEM].sum < bus->r[PCI_REGION_TYPE_PREFMEM].sum) { - bus->r[PCI_REGION_TYPE_MEM].base = - ROOT_BASE(end, - bus->r[PCI_REGION_TYPE_MEM].sum, - bus->r[PCI_REGION_TYPE_MEM].max); - bus->r[PCI_REGION_TYPE_PREFMEM].base = - ROOT_BASE(bus->r[PCI_REGION_TYPE_MEM].base, - bus->r[PCI_REGION_TYPE_PREFMEM].sum, - bus->r[PCI_REGION_TYPE_PREFMEM].max); - if (bus->r[PCI_REGION_TYPE_PREFMEM].base >= start) { - return 0; - } - } else { - bus->r[PCI_REGION_TYPE_PREFMEM].base = - ROOT_BASE(end, - bus->r[PCI_REGION_TYPE_PREFMEM].sum, - bus->r[PCI_REGION_TYPE_PREFMEM].max); - bus->r[PCI_REGION_TYPE_MEM].base = - ROOT_BASE(bus->r[PCI_REGION_TYPE_PREFMEM].base, - bus->r[PCI_REGION_TYPE_MEM].sum, - bus->r[PCI_REGION_TYPE_MEM].max); - if (bus->r[PCI_REGION_TYPE_MEM].base >= start) { - return 0; - } - } - return -1; -} +/**************************************************************** + * Main setup code + ****************************************************************/
void pci_setup(void)
Pass a 'struct pci_device' into pci_bar and update all callers.
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 52 +++++++++++++++++++++++----------------------------- 1 files changed, 23 insertions(+), 29 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index a376ebc..66577a3 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -74,25 +74,21 @@ static enum pci_region_type pci_addr_to_type(u32 addr) return PCI_REGION_TYPE_MEM; }
-static u32 pci_bar(u16 bdf, int region_num) +static u32 pci_bar(struct pci_device *pci, int region_num) { if (region_num != PCI_ROM_SLOT) { return PCI_BASE_ADDRESS_0 + region_num * 4; }
#define PCI_HEADER_TYPE_MULTI_FUNCTION 0x80 - u8 type = pci_config_readb(bdf, PCI_HEADER_TYPE); - type &= ~PCI_HEADER_TYPE_MULTI_FUNCTION; + u8 type = pci->header_type & ~PCI_HEADER_TYPE_MULTI_FUNCTION; return type == PCI_HEADER_TYPE_BRIDGE ? PCI_ROM_ADDRESS1 : PCI_ROM_ADDRESS; }
-static void pci_set_io_region_addr(u16 bdf, int region_num, u32 addr) +static void +pci_set_io_region_addr(struct pci_device *pci, int region_num, u32 addr) { - u32 ofs; - - ofs = pci_bar(bdf, region_num); - - pci_config_writel(bdf, ofs, addr); + pci_config_writel(pci->bdf, pci_bar(pci, region_num), addr); }
@@ -146,12 +142,11 @@ static const struct pci_device_id pci_isa_bridge_tbl[] = {
static void storage_ide_init(struct pci_device *pci, void *arg) { - u16 bdf = pci->bdf; /* IDE: we map it as in ISA mode */ - pci_set_io_region_addr(bdf, 0, PORT_ATA1_CMD_BASE); - pci_set_io_region_addr(bdf, 1, PORT_ATA1_CTRL_BASE); - pci_set_io_region_addr(bdf, 2, PORT_ATA2_CMD_BASE); - pci_set_io_region_addr(bdf, 3, PORT_ATA2_CTRL_BASE); + pci_set_io_region_addr(pci, 0, PORT_ATA1_CMD_BASE); + pci_set_io_region_addr(pci, 1, PORT_ATA1_CTRL_BASE); + pci_set_io_region_addr(pci, 2, PORT_ATA2_CMD_BASE); + pci_set_io_region_addr(pci, 3, PORT_ATA2_CTRL_BASE); }
/* PIIX3/PIIX4 IDE */ @@ -165,13 +160,13 @@ static void piix_ide_init(struct pci_device *pci, void *arg) static void pic_ibm_init(struct pci_device *pci, void *arg) { /* PIC, IBM, MPIC & MPIC2 */ - pci_set_io_region_addr(pci->bdf, 0, 0x80800000 + 0x00040000); + pci_set_io_region_addr(pci, 0, 0x80800000 + 0x00040000); }
static void apple_macio_init(struct pci_device *pci, void *arg) { /* macio bridge */ - pci_set_io_region_addr(pci->bdf, 0, 0x80800000); + pci_set_io_region_addr(pci, 0, 0x80800000); }
static const struct pci_device_id pci_class_tbl[] = { @@ -339,10 +334,11 @@ static u32 pci_size_roundup(u32 size) return 0x1 << index; }
-static void pci_bios_bus_get_bar(struct pci_bus *bus, int bdf, int bar, - u32 *val, u32 *size) +static void +pci_bios_get_bar(struct pci_device *pci, int bar, u32 *val, u32 *size) { - u32 ofs = pci_bar(bdf, bar); + u32 ofs = pci_bar(pci, bar); + u16 bdf = pci->bdf; u32 old = pci_config_readl(bdf, ofs); u32 mask;
@@ -377,10 +373,6 @@ static void pci_bios_check_device_in_bus(int bus);
static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev) { - u16 bdf = dev->bdf; - u32 limit; - int i,type; - if (dev->class == PCI_CLASS_BRIDGE_PCI) { if (dev->secondary_bus >= busses_count) { /* should never trigger */ @@ -390,8 +382,9 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev) } struct pci_bus *s = busses + dev->secondary_bus; pci_bios_check_device_in_bus(dev->secondary_bus); + int type; for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { - limit = (type == PCI_REGION_TYPE_IO) ? + u32 limit = (type == PCI_REGION_TYPE_IO) ? PCI_BRIDGE_IO_MIN : PCI_BRIDGE_MEM_MIN; s->r[type].size = s->r[type].sum; if (s->r[type].size < limit) @@ -407,9 +400,10 @@ static void pci_bios_check_device(struct pci_bus *bus, struct pci_device *dev) return; }
+ int i; for (i = 0; i < PCI_NUM_REGIONS; i++) { u32 val, size; - pci_bios_bus_get_bar(bus, bdf, i, &val, &size); + pci_bios_get_bar(dev, i, &val, &size); if (val == 0) { continue; } @@ -518,9 +512,6 @@ static void pci_bios_map_device_in_bus(int bus);
static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) { - u16 bdf = dev->bdf; - int type, i; - if (dev->class == PCI_CLASS_BRIDGE_PCI) { if (dev->secondary_bus >= busses_count) { return; @@ -528,6 +519,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) struct pci_bus *s = busses + dev->secondary_bus; u32 base, limit;
+ int type; for (type = 0; type < PCI_REGION_TYPE_COUNT; type++) { s->r[type].base = pci_bios_bus_get_addr(bus, type, s->r[type].size); } @@ -536,6 +528,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev)
base = s->r[PCI_REGION_TYPE_IO].base; limit = base + s->r[PCI_REGION_TYPE_IO].size - 1; + u16 bdf = dev->bdf; pci_config_writeb(bdf, PCI_IO_BASE, base >> PCI_IO_SHIFT); pci_config_writew(bdf, PCI_IO_BASE_UPPER16, 0); pci_config_writeb(bdf, PCI_IO_LIMIT, limit >> PCI_IO_SHIFT); @@ -557,6 +550,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) return; }
+ int i; for (i = 0; i < PCI_NUM_REGIONS; i++) { u32 addr; if (dev->bars[i].addr == 0) { @@ -568,7 +562,7 @@ static void pci_bios_map_device(struct pci_bus *bus, struct pci_device *dev) dprintf(1, " bar %d, addr %x, size %x [%s]\n", i, addr, dev->bars[i].size, region_type_name[pci_addr_to_type(dev->bars[i].addr)]); - pci_set_io_region_addr(bdf, i, addr); + pci_set_io_region_addr(dev, i, addr);
if (dev->bars[i].is64) { i++;
Signed-off-by: Kevin O'Connor kevin@koconnor.net --- src/pciinit.c | 20 +++++++------------- 1 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/src/pciinit.c b/src/pciinit.c index 66577a3..3c12602 100644 --- a/src/pciinit.c +++ b/src/pciinit.c @@ -101,13 +101,11 @@ const u8 pci_irqs[4] = { 10, 10, 11, 11 };
-/* return the global irq number corresponding to a given device irq - pin. We could also use the bus number to have a more precise - mapping. */ -static int pci_slot_get_pirq(u16 bdf, int irq_num) +// Return the global irq number corresponding to a host bus device irq pin. +static int pci_slot_get_irq(u16 bdf, int pin) { int slot_addend = pci_bdf_to_dev(bdf) - 1; - return (irq_num + slot_addend) & 3; + return pci_irqs[(pin - 1 + slot_addend) & 3]; }
/* PIIX3/PIIX4 PCI to ISA bridge */ @@ -215,23 +213,19 @@ static const struct pci_device_id pci_device_tbl[] = { static void pci_bios_init_device(struct pci_device *pci) { u16 bdf = pci->bdf; - int pin, pic_irq; - dprintf(1, "PCI: init bdf=%02x:%02x.%x id=%04x:%04x\n" , pci_bdf_to_bus(bdf), pci_bdf_to_dev(bdf), pci_bdf_to_fn(bdf) , pci->vendor, pci->device); + pci_init_device(pci_class_tbl, pci, NULL);
/* enable memory mappings */ pci_config_maskw(bdf, PCI_COMMAND, 0, PCI_COMMAND_IO | PCI_COMMAND_MEMORY);
/* map the interrupt */ - pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN); - if (pin != 0) { - pin = pci_slot_get_pirq(bdf, pin - 1); - pic_irq = pci_irqs[pin]; - pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pic_irq); - } + int pin = pci_config_readb(bdf, PCI_INTERRUPT_PIN); + if (pin != 0) + pci_config_writeb(bdf, PCI_INTERRUPT_LINE, pci_slot_get_irq(bdf, pin));
pci_init_device(pci_device_tbl, pci, NULL); }
-
Kevin O'Connor