Hi Alex,
$SUBJECT (arm/pci: Probe Open Firmware Device Tree) is too broad.
This patch adds DT probing for a specific device, a PCIe host
bridge. Even more specifically, a pci-host-ecam-generic compatible
one. Also, combing arm patches and pci probing patches isn't
necessary. We should add pci probing independently of arm (as it
can be used by other architectures) and then add arm patches that
make use it.
On Thu, Feb 11, 2016 at 09:25:01AM +0100, Alexander Gordeev wrote:
> This (and following PCI bus scan) commit is a glimpse of
> Linux implementation with almost all PCI data structures
> omitted - none of pci_host_bridge, pci_bus, pci_dev and
> pci_bus_resource are adopted. While gen_pci is just name-
> compatible and is basically a root pointer to underlying
> data describing PCI bus hierarchy in a minimalistic way.
>
> The Device Tree is expected to conform to PCI Bus Binding
> to Open Firmware with any deviation treated as a fatal
> errors.
>
> Cc: Thomas Huth <thuth@xxxxxxxxxx>
> Cc: Andrew Jones <drjones@xxxxxxxxxx>
> Signed-off-by: Alexander Gordeev <agordeev@xxxxxxxxxx>
> ---
> arm/pci-test.c | 21 +++++
> config/config-arm-common.mak | 5 +-
> lib/pci-host-generic.c | 219 +++++++++++++++++++++++++++++++++++++++++++
> lib/pci-host-generic.h | 73 +++++++++++++++
> lib/pci.h | 3 +
> 5 files changed, 320 insertions(+), 1 deletion(-)
> create mode 100644 arm/pci-test.c
> create mode 100644 lib/pci-host-generic.c
> create mode 100644 lib/pci-host-generic.h
>
> diff --git a/arm/pci-test.c b/arm/pci-test.c
> new file mode 100644
> index 0000000..db7d048
> --- /dev/null
> +++ b/arm/pci-test.c
> @@ -0,0 +1,21 @@
> +/*
> + * PCI bus operation test
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@xxxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include <libcflat.h>
> +#include <pci.h>
> +
> +int main(void)
> +{
> + int ret;
> +
> + ret = pci_probe();
I think the probe function should return an object. In this case a
"pci" object, which will actually be a member of a pci_host_bridge
object. The pci object will be the object that lib/pci.[ch] functions
target, whereas
struct pci_host_bridge *host = container_of(pci, struct pci_host_bridge, pci)
is the object that is targeted by lib/pci-host-generic.[ch] functions.
I was going to suggest we drop your set_pci/get_pci functions, but in
order to integrate with x86, I guess we should try to avoid the need
for passing around pci object pointers in the lib/pci.[ch] functions.
Let's instead move set_pci/get_pci into lib/pci.h, allowing a global
pci object to be set for those functions to operate on.
> + report("PCI device tree probing", ret);
The report string shouldn't report messages based on the internals
of the function it calls. If the implementation of pci_probe were to
change, i.e. no longer using a DT, then the message would be wrong.
> +
> + pci_shutdown();
This call should be implemented in lib/pci.c using pci_config_read,write.
You'll need to add pci_config_write to lib/x86/asm/pci.h too (at least
a stub).
> +
> + return report_summary();
> +}
> diff --git a/config/config-arm-common.mak b/config/config-arm-common.mak
> index 698555d..06ad346 100644
> --- a/config/config-arm-common.mak
> +++ b/config/config-arm-common.mak
> @@ -11,7 +11,8 @@ endif
>
> tests-common = \
> $(TEST_DIR)/selftest.flat \
> - $(TEST_DIR)/spinlock-test.flat
> + $(TEST_DIR)/spinlock-test.flat \
> + $(TEST_DIR)/pci-test.flat
>
> all: test_cases
>
> @@ -29,6 +30,7 @@ include config/asm-offsets.mak
>
> cflatobjs += lib/alloc.o
> cflatobjs += lib/devicetree.o
> +cflatobjs += lib/pci-host-generic.o
> cflatobjs += lib/virtio.o
> cflatobjs += lib/virtio-mmio.o
> cflatobjs += lib/chr-testdev.o
> @@ -70,3 +72,4 @@ test_cases: $(generated_files) $(tests-common) $(tests)
>
> $(TEST_DIR)/selftest.elf: $(cstart.o) $(TEST_DIR)/selftest.o
> $(TEST_DIR)/spinlock-test.elf: $(cstart.o) $(TEST_DIR)/spinlock-test.o
> +$(TEST_DIR)/pci-test.elf: $(cstart.o) $(TEST_DIR)/pci-test.o
As mentioned at the top, all the above arm changes should be in a separate
patch.
> diff --git a/lib/pci-host-generic.c b/lib/pci-host-generic.c
> new file mode 100644
> index 0000000..9bc6642
> --- /dev/null
> +++ b/lib/pci-host-generic.c
> @@ -0,0 +1,219 @@
> +/*
> + * PCI bus operation test
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@xxxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
> +#include "devicetree.h"
> +#include "asm/io.h"
> +#include "pci.h"
> +#include "pci-host-generic.h"
> +#include <linux/pci_regs.h>
> +
> +struct gen_pci *gen_pci = NULL;
This can be just 'struct pci', or 'pci_gen'. It's a global reference
so it's preferably prefixed by its domain.
> +
> +static struct gen_pci *get_pci(void)
> +{
> + return gen_pci;
> +}
> +
> +static void set_pci(struct gen_pci *pci)
> +{
> + gen_pci = pci;
> +}
The above should move to lib/pci.* Also please reverse the object/verb
in the name, i.e. pci_get, pci_set.
> +
> +static char *addr_space_desc[] = {
> + [PCI_RES_TYPE_CONF] = "CONF",
> + [PCI_RES_TYPE_IO] = "IO",
> + [PCI_RES_TYPE_MEM32] = "MEM32",
> + [PCI_RES_TYPE_MEM64] = "MEM64",
> + [PCI_RES_TYPE_PREFMEM32] = "MEM32/p",
> + [PCI_RES_TYPE_PREFMEM64] = "MEM64/p"
> +};
> +
> +static pci_res_type_t flags_to_type(u32 of_flags)
> +{
> + return ((of_flags & 0x40000000) >> 28) | ((of_flags >> 24) & 0x03);
> +}
The above flags and pci-res-types are DT specific. I'm not sure why you
opted to keep the flags, rather than decode them into fields (as I did
in the starter code I sent you). By decoding them we keep all DT stuff
in a single DT-probe function, rather than letting it ooze out into the
rest of the PCI code.
> +
> +static u32 from_fdt32(fdt32_t val)
> +{
> + return fdt32_to_cpu(val);
> +}
Sorry, but this is pointless, and drops the important 'to_cpu' part of
the function name, allowing us to know we're doing endian conversions,
if necessary.
> +
> +static u64 from_fdt64(fdt32_t high, fdt32_t low)
> +{
> + return ((u64)fdt32_to_cpu(high) << 32) | fdt32_to_cpu(low);
> +}
This has some more value, but for as little as we use it, I guess
I'd rather not introduce the helper.
> +
> +static void pci_host_addr_space_init(struct pci_addr_space as[], int nr_as,
> + fdt32_t rcells[], int nr_rcells)
This function is only called from one place, so why not just embed it at
the callsite? And, without "_dt_" in the name it's another example of DT
leakage into PCI general.
> +{
> + int i;
> +
> + /*
> + * The PCI binding claims the numerical representation of a PCI
> + * address consists of three cells, encoded as follows:
> + *
> + * phys.hi cell: npt000ss bbbbbbbb dddddfff rrrrrrrr
> + * phys.mid cell: hhhhhhhh hhhhhhhh hhhhhhhh hhhhhhhh
> + * phys.lo cell: llllllll llllllll llllllll llllllll
> + *
> + * PCI device bus address and flags are encoded into phys.high
> + * PCI 64 bit address is encoded into phys.mid and phys.low
> + */
> +
> + for (i = 0; i < nr_as; i++, as++) {
> + as->of_flags = from_fdt32(rcells[0]);
> + as->pci_range.start = from_fdt64(rcells[1], rcells[2]);
> +
> + if (nr_rcells == 6) {
> + as->cpu_range.start = from_fdt32(rcells[3]);
> + as->cpu_range.size = from_fdt64(rcells[4], rcells[5]);
> + } else {
> + as->cpu_range.start = from_fdt64(rcells[3], rcells[4]);
> + as->cpu_range.size = from_fdt64(rcells[5], rcells[6]);
> + }
> +
> + as->pci_range.size = as->cpu_range.size;
> + rcells += nr_rcells;
> + }
> +}
> +
> +static void gen_pci_print(struct gen_pci *pci)
This is a useful function to make public, but needs to be in lib/pci.*,
and it won't need to take a pci object pointer, because there we'll use
the global one set by pci_set()
> +{
> + struct pci_addr_space *as = &pci->addr_space[0];
> + int i;
> +
> + printf("PCIe start %016llx size %016llx "
> + "bus %02x bus_max %02x #spaces %d\n\n",
> + pci->cpu_range.start, pci->cpu_range.size,
> + pci->bus, pci->bus_max, pci->nr_addr_spaces);
> +
> + for (i = 0; i < pci->nr_addr_spaces; i++, as++) {
> + printf("%s address space:\n"
> + "CPU: start %016llx size %016llx\n"
> + "PCI: start %016llx size %016llx\n\n",
> + addr_space_desc[flags_to_type(as->of_flags)],
> + as->cpu_range.start, as->cpu_range.size,
> + as->pci_range.start, as->pci_range.size);
> + }
> +}
> +
> +/*
> + * Probe DT for a generic PCI host controller
> + * See kernel Documentation/devicetree/bindings/pci/host-generic-pci.txt
> + * and function gen_pci_probe() in drivers/pci/host/pci-host-generic.c
> + */
> +static struct gen_pci *gen_pci_probe(void)
I mentioned this before. This function should have a "_dt_" in the name.
> +{
> + struct gen_pci *pci;
> + const void *fdt = dt_fdt();
> + const struct fdt_property *prop;
> + struct dt_pbus_reg base;
> + struct dt_device dt_dev;
> + struct dt_bus dt_bus;
> + u32 bus, bus_max;
> + u32 nac, nsc, nac_root, nsc_root;
> + u32 nr_range_cells, nr_addr_spaces;
> + int ret, node, len;
> +
> + if (!dt_available()) {
> + printf("No device tree found");
> + return NULL;
> + }
> +
> + dt_bus_init_defaults(&dt_bus);
> + dt_device_init(&dt_dev, &dt_bus, NULL);
> +
> + node = fdt_path_offset(fdt, "/");
> + assert(node >= 0);
> +
> + ret = dt_get_nr_cells(node, &nac_root, &nsc_root);
> + assert(ret == 0);
> + assert(nac_root == 1 || nac_root == 2);
> +
> + node = fdt_subnode_offset(fdt, node, "pcie");
> + if (node == -FDT_ERR_NOTFOUND) {
> + printf("No PCIe controller found");
> + return NULL;
> + }
> + assert(node >= 0);
> +
> + prop = fdt_get_property(fdt, node, "device_type", &len);
> + if (!prop || len != 4 || strcmp((char*)prop->data, "pci")) {
> + printf("PCIe controller device_type is not \"pci\"");
> + return NULL;
> + }
The above error-return-null can be an assert. host-generic-pci.txt says
device_type "Must be" "pci".
> +
> + ret = fdt_node_check_compatible(fdt, node, "pci-host-ecam-generic");
> + assert(ret >= 0);
> + if (ret != 0) {
> + printf("PCIe controller is not ECAM compatible");
> + return NULL;
> + }
> +
> + dt_device_bind_node(&dt_dev, node);
> + ret = dt_pbus_get_base(&dt_dev, &base);
> + assert(ret == 0);
> +
> + prop = fdt_get_property(fdt, node, "bus-range", &len);
> + if (prop == NULL) {
> + assert(len == -FDT_ERR_NOTFOUND);
> + bus = 0x00;
> + bus_max = 0xff;
> + } else {
> + fdt32_t *data = (fdt32_t*)prop->data;
> + bus = fdt32_to_cpu(data[0]);
> + bus_max = fdt32_to_cpu(data[1]);
> + assert(bus <= bus_max);
> + }
> + assert(bus_max < base.size / PCI_ECAM_BUS_SIZE);
> +
> + ret = dt_get_nr_cells(node, &nac, &nsc);
> + assert(ret == 0);
> + assert(nac == 3 && nsc == 2);
> +
> + prop = fdt_get_property(fdt, node, "ranges", &len);
> + assert(prop != NULL);
> +
> + nr_range_cells = nac + nsc + nac_root;
> + nr_addr_spaces = (len / 4) / nr_range_cells;
> +
> + pci = malloc(sizeof(*pci) +
> + sizeof(pci->addr_space[0]) * nr_addr_spaces);
> + assert(pci != NULL);
> +
> + pci->cpu_range.start = base.addr;
> + pci->cpu_range.size = base.size;
> + pci->bus = bus;
> + pci->bus_max = bus_max;
> + pci->nr_addr_spaces = nr_addr_spaces;
> +
> + pci_host_addr_space_init(&pci->addr_space[0], nr_addr_spaces,
> + (fdt32_t*)prop->data, nr_range_cells);
The above are host-bridge properties, so they should go in a
pci_host_bridge object. I guess so far we don't have any pci object
state. Well, except for the bus number, which I guess can be maintained
redundantly. And, maybe the address ranges are pci object properties?
> + gen_pci_print(pci);
I don't think we should always print on every unittest boot. The
print function should be available for unittest debug though.
> +
> + return pci;
return &host->pci;
> +}
> +
> +bool pci_probe(void)
> +{
> + struct gen_pci *pci = get_pci();
> +
> + assert(pci == NULL);
> + pci = gen_pci_probe();
> + set_pci(pci);
> +
> + return (pci != NULL);
return pci;
> +}
> +
> +void pci_shutdown(void)
> +{
> + struct gen_pci *pci = get_pci();
> +
> + set_pci(NULL);
> + free(pci);
nit: normally free comes before the NULL setting.
> +}
> diff --git a/lib/pci-host-generic.h b/lib/pci-host-generic.h
> new file mode 100644
> index 0000000..f4ae3e4
> --- /dev/null
> +++ b/lib/pci-host-generic.h
> @@ -0,0 +1,73 @@
> +#ifndef PCI_HOST_GENERIC_H
> +#define PCI_HOST_GENERIC_H
> +/*
> + * PCI host bridge supporting structures and constants
> + *
> + * Copyright (C) 2016, Red Hat Inc, Alexander Gordeev <agordeev@xxxxxxxxxx>
> + *
> + * This work is licensed under the terms of the GNU LGPL, version 2.
> + */
> +#include "libcflat.h"
This should include "alloc.h" for phys_addr_t. Actually, I'm tempted to
move that typedef to libcflat.h, as we have several files now that only
include alloc.h for the typedef.
> +
> +struct pci_addr_range {
> + phys_addr_t start;
> + phys_addr_t size;
> +};
> +
> +struct pci_addr_space {
> + struct pci_addr_range cpu_range;
> + struct pci_addr_range pci_range;
> + u32 of_flags;
As mentioned above, we don't want OF specific flags in our common pci
data structures.
> +};
> +
> +/*
> + * See drivers/pci/host/pci-host-generic.c for the idea of what gen_pci
> + * structure is. Although it brings the very same semantics as Linux,
> + * it completely misses the kernel's data design in a bid to keep it as
> + * simple as possible.
> + */
> +struct gen_pci {
> + struct pci_addr_range cpu_range;
> + int bus;
> + int bus_max;
> + int nr_addr_spaces;
> + struct pci_addr_space addr_space[];
> +};
> +
> +typedef enum pci_res_type {
> + PCI_RES_TYPE_CONF = 0,
> + PCI_RES_TYPE_IO = 1,
> + PCI_RES_TYPE_MEM32 = 2,
> + PCI_RES_TYPE_MEM64 = 3,
> + PCI_RES_TYPE_PREFMEM32 = 6,
> + PCI_RES_TYPE_PREFMEM64 = 7
These values are also OF specific, i.e. 6 and 7 are the merged OF
prefetch and memtype flags. I'd rather not do that. prefetchable
(cacheable) can be a boolean, and aren't there already memtype defines
in linux/pci_regs.h we can use for a type field?
> +} pci_res_type_t;
> +
> +/*
> + * The following constants are derived from Linux, see this source:
> + *
> + * drivers/pci/host/pci-host-generic.c
> + * struct gen_pci_cfg_bus_ops::bus_shift
> + * int gen_pci_parse_map_cfg_windows(struct gen_pci *pci)
> + *
> + * Documentation/devicetree/bindings/pci/host-generic-pci.txt excerpt:
> + *
> + * Configuration Space is assumed to be memory-mapped (as opposed to being
> + * accessed via an ioport) and laid out with a direct correspondence to the
> + * geography of a PCI bus address by concatenating the various components to
> + * form an offset.
> + *
> + * For CAM, this 24-bit offset is:
> + *
> + * cfg_offset(bus, device, function, register) =
> + * bus << 16 | device << 11 | function << 8 | register
> + *
> + * Whilst ECAM extends this by 4 bits to accommodate 4k of function space:
> + *
> + * cfg_offset(bus, device, function, register) =
> + * bus << 20 | device << 15 | function << 12 | register
This is a nice comment, but I think it should go above some function that
implements it.
> + *
> + */
> +#define PCI_ECAM_BUS_SIZE (1 << 20)
How about creating a PCI_ECAM_BUS_SHIFT 20 define instead. I presume
we'll want to use it in a config_offset() function too. Well, we assume
the bus is always zero, so maybe we'd shortcut that, but still...
> +
> +#endif
> diff --git a/lib/pci.h b/lib/pci.h
> index 9160cfb..80d0d04 100644
> --- a/lib/pci.h
> +++ b/lib/pci.h
> @@ -39,4 +39,7 @@ struct pci_test_dev_hdr {
> uint8_t name[];
> };
>
> +extern bool pci_probe(void);
> +extern void pci_shutdown(void);
Yes, this is the right place to declare these functions.
> +
> #endif /* PCI_H */
> --
> 1.8.3.1
>
Thanks,
drew
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
