On 16/07/17 21:56, BALATON Zoltan wrote:
So does that mean we could theoretically get rid of all the other redundant data and put this pci_mem_base offset for SPARC64 in some #ifdefed code part (or in the undocumented register to counter the offset in Linux) to simplify the current situation where the ranges are defined in two different ways?
There was some code for config space registers already #ifdef'd for SPARC64 but the issue there was that with so many conditionals used for building up the PCI host bridge ranges that it was very difficult to see at a glance what the resulting values were.
Bear in mind that PPC also had its own hacks in that it has multiple IO memory spaces listed for the host bridge, so rather than keep going down this maze of conditional logic and #ifdef, I decided to explicitly list the values themselves which solves all the problems in one go.
If you wanted to better document the relationships between the host ranges properties and the corresponding values in pci_arch_t then you could use some #defines, but then if you're working down at that level regardless then I suspect this will be the least of your problems.
No I don't mean defines but something like this:
From: BALATON Zoltan balaton@eik.bme.hu Date: Sun, 16 Jul 2017 22:41:38 +0200 Subject: [PATCH] RFC patch to remove the now redundant io_base and io_len from pci_arch_t that are present in newly added pci ranges. This was only compile tested on PPC and not sure what to do with other redundandant addresses and lengths but similar handling might be possible for those too. Feel free to do anything with this patch.
Signed-off-by: BALATON Zoltan balaton@eik.bme.hu
arch/ppc/qemu/init.c | 12 ++---------- drivers/pci.c | 39 ++++++++++++++++++++++++++++++--------- include/drivers/pci.h | 19 ++++++++++--------- 3 files changed, 42 insertions(+), 28 deletions(-)
diff --git a/arch/ppc/qemu/init.c b/arch/ppc/qemu/init.c index b0c0cfc..a4746c2 100644 --- a/arch/ppc/qemu/init.c +++ b/arch/ppc/qemu/init.c @@ -104,8 +104,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0xc0000000, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0x10000000,
.io_base = 0x80000000,.io_len = 0x00010000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0x80000000, .len = 0x00010000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xc0100000, .len = 0x10000000 },@@ -124,8 +122,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xf2000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },@@ -144,8 +140,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xf2000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xf2000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0x80000000, .childaddr = 0x80000000, .len = 0x10000000 },@@ -164,8 +158,6 @@ static const pci_arch_t known_arch[] = { .host_pci_base = 0x0, .pci_mem_base = 0x80000000, .mem_len = 0x10000000,
.io_base = 0xfe000000,.io_len = 0x00800000, .host_ranges = { { .type = IO_SPACE, .parentaddr = 0, .childaddr = 0xfe000000, .len = 0x00800000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = 0xfd000000, .len = 0x01000000 },@@ -198,8 +190,8 @@ entry(void) arch = &known_arch[machine_id]; } }
- isa_io_base = arch->io_base;
- const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
- isa_io_base = range->childaddr;
#ifdef CONFIG_DEBUG_CONSOLE if (is_apple()) { diff --git a/drivers/pci.c b/drivers/pci.c index f6d9437..2f20dcc 100644 --- a/drivers/pci.c +++ b/drivers/pci.c @@ -140,14 +140,21 @@ static void dump_reg_property(const char* description, int nreg, u32 *reg)
static unsigned long pci_bus_addr_to_host_addr(int space, uint32_t ba) {
- if (space == IO_SPACE) {
return arch->io_base + (unsigned long)ba;- } else if (space == MEMORY_SPACE_32) {
return arch->host_pci_base + (unsigned long)ba;- } else {
- unsigned long host_addr = (unsigned long)ba;
- const pci_range_t *range = ob_pci_get_range(space, arch->host_ranges);
- switch (space) {
- case IO_SPACE:
host_addr += range->childaddr;break;- case MEMORY_SPACE_32:
host_addr += arch->host_pci_base;break;- default: /* Return unaltered to aid debugging property values */
return (unsigned long)ba;
break;- return host_addr;
}
static void @@ -491,6 +498,17 @@ static void pci_host_set_ranges(const pci_config_t *config) set_property(dev, "ranges", (char *)props, ncells * sizeof(props[0])); }
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]) +{
- const pci_range_t *range = NULL;
- int i;
- for (i = 0; i < 4 && ranges[i].type != SPACE_END; i++) {
if (ranges[i].type == type)range = &ranges[i];- }
- return range;
+}
int host_config_cb(const pci_config_t *config) { //XXX this overrides "reg" property @@ -988,11 +1006,12 @@ int ebus_config_cb(const pci_config_t *config)
int i82378_config_cb(const pci_config_t *config) {
- const pci_range_t *range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
#ifdef CONFIG_DRIVER_PC_SERIAL
- ob_pc_serial_init(config->path, "serial", arch->io_base, 0x3f8ULL, 0);
- ob_pc_serial_init(config->path, "serial", range->childaddr, 0x3f8ULL, 0);
#endif #ifdef CONFIG_DRIVER_PC_KBD
- ob_pc_kbd_init(config->path, "8042", NULL, arch->io_base, 0x60ULL, 0, 0);
- ob_pc_kbd_init(config->path, "8042", NULL, range->childaddr, 0x60ULL, 0, 0);
#endif #ifdef CONFIG_DRIVER_IDE ob_ide_init(config->path, 0x1f0, 0x3f6, 0x170, 0x376); @@ -1649,12 +1668,14 @@ static void ob_pci_set_available(phandle_t host, unsigned long mem_base, unsigne /* Create an available property for both memory and IO space */ uint32_t props[10]; int ncells;
const pci_range_t *range;
ncells = 0; ncells += pci_encode_phys_addr(props + ncells, 0, MEMORY_SPACE_32, 0, 0, mem_base); ncells += pci_encode_size(props + ncells, arch->mem_len - mem_base); ncells += pci_encode_phys_addr(props + ncells, 0, IO_SPACE, 0, 0, io_base);
- ncells += pci_encode_size(props + ncells, arch->io_len - io_base);
range = ob_pci_get_range(IO_SPACE, arch->host_ranges);
ncells += pci_encode_size(props + ncells, range->len - io_base);
set_property(host, "available", (char *)props, ncells * sizeof(props[0]));
} diff --git a/include/drivers/pci.h b/include/drivers/pci.h index e9f20a1..e267db2 100644 --- a/include/drivers/pci.h +++ b/include/drivers/pci.h @@ -1,19 +1,20 @@ #ifndef _H_PCI #define _H_PCI
-enum {
- CONFIGURATION_SPACE = 0,
- IO_SPACE = 1,
- MEMORY_SPACE_32 = 2,
- MEMORY_SPACE_64 = 3,
-}; +typedef enum {
- SPACE_END = 0,
- CONFIGURATION_SPACE,
- IO_SPACE,
- MEMORY_SPACE_32,
- MEMORY_SPACE_64
+} pci_range_type;
The space identifiers need to be identical to the current ones since these are well-known values used by client OSs to enumerate the space types from the device tree.
typedef uint32_t pci_addr;
typedef struct pci_range_t pci_range_t;
struct pci_range_t {
- unsigned int type;
- pci_range_type type; unsigned long childaddr; unsigned long parentaddr; unsigned long len;
@@ -32,14 +33,14 @@ struct pci_arch_t { unsigned long host_pci_base; /* offset of PCI memory space within host memory space */ unsigned long pci_mem_base; /* in PCI memory space */ unsigned long mem_len;
- unsigned long io_base;
- unsigned long io_len; pci_range_t host_ranges[4]; uint8_t irqs[4];
};
extern const pci_arch_t *arch;
+const pci_range_t *ob_pci_get_range(pci_range_type type, const pci_range_t ranges[]);
/* Device tree offsets */
#define PCI_INT_MAP_PCI0 0
I can see what you're trying to do here, however for me the part I like least about this is that it introduces asymmetry between the PCI IO and MMIO memory spaces. Effectively what you're doing is saying that for PCI IO memory you need to extract the information from the range whilst for PCI MMIO memory you need to use the standard struct values, which for me seems something that's easy to trip up developers.
If you could find a way to make the changes consistent between both MMIO and IO memory I think the patch will improve things, although you also need to handle the case for ARCH_HEATHROW where you have multiple 32-bit MMIO ranges and for SPARC64 whereby the MMIO child address is not the same as pci_mem_base.
Note that if you experiment further with this you can also see that .parentaddr is also equivalent to host_pci_base which can help simplify things further.
ATB,
Mark.