On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 16:49, BALATON Zoltan wrote:
On Sun, 16 Jul 2017, Mark Cave-Ayland wrote:
On 16/07/17 14:56, BALATON Zoltan wrote:
They are for PPC, but unfortunately not for SPARC64. That's why they were split out as separate values in the last set of patches because the existing logic couldn't handle all cases of multiple address spaces with different offsets :(
I don't see why SPARC64 is not also redundant. Here's its pci arch struct:
.pci = { .name = "SUNW,sabre", .vendor_id = PCI_VENDOR_ID_SUN, .device_id = PCI_DEVICE_ID_SUN_SABRE, .cfg_addr = APB_SPECIAL_BASE + 0x1000000ULL, // PCI bus configuration space .cfg_data = APB_MEM_BASE, // PCI bus memory space .cfg_base = APB_SPECIAL_BASE, .cfg_len = 0x1000000, .host_pci_base = APB_MEM_BASE, .pci_mem_base = 0x100000, /* avoid VGA at 0xa0000 */ .mem_len = 0xf0000000, .io_base = APB_SPECIAL_BASE + 0x2000000ULL, // PCI Bus I/O space .io_len = 0x1000000, .host_ranges = { { .type = CONFIGURATION_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x1000000ULL, .len = 0x1000000 }, { .type = IO_SPACE, .parentaddr = 0, .childaddr = APB_SPECIAL_BASE + 0x2000000ULL, .len = 0x1000000 }, { .type = MEMORY_SPACE_32, .parentaddr = 0, .childaddr = APB_MEM_BASE, .len = 0xf0000000 }, { .type = 0, .parentaddr = 0, .childaddr = 0, .len = 0 } }, .irqs = { 0, 1, 2, 3 }, },
The addresses and lengths here are also mostly the same as in host_ranges. Where does the difference come from and could we get rid of the individual *_base, *_addr and *_len fields and define everything in the host_ranges structure? (Maybe we need some additional offsets in the config space but that's all we should need I think.)
It looks like that now the host_ranges are used to create the ranges property while the other addresses and lengths are used to create the mapping but these two should match (because the ranges property is supposed to describe these mappings) so one should be computable from the other otherwise we may have a bug (or I'm missing something which is also likely as I know nothing about this). Therefore I think it should be possible to get rid of one of these. What am I missing?
For SPARC64 the issue is with pci_mem_base and how it is interpreted in Linux. Before the recent set of patches, the .childaddr for MEMORY_SPACE_32 was generated as APB_MEM_BASE + pci_mem_base, so in effect all we do is miss mapping the first PCI memory section.
However in Linux the kernel calculates the address of the host range is by taking the value of an undocumented register in the Simba host bridge (see simba_config_cb() for details) and then adding the offsets on top meaning that you end up with all your addresses out by pci_mem_base.
Hence the need to apply pci_mem_base as the starting value for assigning devices to PCI memory space, but not including it in the host ranges itself.
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;
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