[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [RFC PATCH v1 1/4] arm/pci: PCI setup and PCI host bridge discovery within XEN on ARM.

On 24/07/2020 15:44, Roger Pau Monné wrote:
diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
new file mode 100644
index 0000000000..358508b787
--- /dev/null
+++ b/xen/arch/arm/pci/Makefile
@@ -0,0 +1,4 @@
+obj-y += pci.o
+obj-y += pci-host-generic.o
+obj-y += pci-host-common.o
+obj-y += pci-access.o

The Kconfig option mentions the support being explicitly for ARM64,
would it be better to place the code in arch/arm/arm64 then?
I don't believe any of the code in this series is very arm64 specific. I guess it was just only tested on arm64. So I would rather keep that under arm/pci.

+    struct pci_host_bridge *bridge = pci_find_host_bridge(sbdf.seg, sbdf.bus);
+    if ( unlikely(!bridge) )
+    {
+        printk(XENLOG_ERR "Unable to find bridge for "PRI_pci"\n",
+                sbdf.seg, sbdf.bus, sbdf.dev, sbdf.fn);

I had a patch to add a custom modifier to out printf format in
order to handle pci_sbdf_t natively:


It missed maintainers Acks and was never committed. Since you are
doing a bunch of work here, and likely adding a lot of SBDF related
prints, feel free to import the modifier (%pp) and use in your code
(do not attempt to switch existing users, or it's likely to get
stuck again).

I forgot about this patch :/. It would be good to revive it. Which acks are you missing?


+static bool __init dt_pci_parse_bus_range(struct dt_device_node *dev,
+        struct pci_config_window *cfg)
+    const __be32 *cells;

It's my impression that while based on Linux this is not a verbatim
copy of a Linux file, and tries to adhere with the Xen coding style.
If so please use uint32_t here.

uint32_t would be incorrect because this is a 32-bit value always in big endian. I don't think we have other typedef to show it is a 32-bit BE value, so __be32 is the best choice.


+    if ( acpi_disabled )
+        dt_pci_init();
+    else
+        acpi_pci_init();

Isn't there an enum or something that tells you whether the system
description is coming from ACPI or from DT?

This if .. else seems fragile.

This is the common way we do it on Arm.... I would welcome any improvement, but I don't think this should be part of this work.


Julien Grall



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.