XEN during boot will read the PCI device tree node “reg” property
and will map the PCI config space to the XEN memory.
As of now only "pci-host-ecam-generic" compatible board is supported.
"linux,pci-domain" device tree property assigns a fixed PCI domain
number to a host bridge, otherwise an unstable (across boots) unique
number will be assigned by Linux. XEN access the PCI devices based on
Segment:Bus:Device:Function. Segment number in XEN is same as domain
number in Linux.Segment number and domain number has to be in sync
to access the correct PCI devices.
XEN will read the “linux,pci-domain” property from the device tree node
and configure the host bridge segment number accordingly. If this
property is not available XEN will allocate the unique segment number
to the host bridge.
dt_pci_bus_find_domain_nr(..) imported from the Linux source tree with
slight modification based on tag Linux v5.14.2
commit bbdd3de144fc142f2f4b9834c9241cc4e7f3d3fc.
Signed-off-by: Rahul Singh <rahul.singh@arm.com>
---
Change in v2:
- Add more info in commit msg
- Add callback to parse register index.
- Merge patch pci_ecam_operation into this patch to avoid confusion
- Add new struct in struct device for match table
---
xen/arch/arm/device.c | 2 +
xen/arch/arm/pci/Makefile | 5 +
xen/arch/arm/pci/ecam.c | 60 +++++++
xen/arch/arm/pci/pci-access.c | 83 +++++++++
xen/arch/arm/pci/pci-host-common.c | 254 ++++++++++++++++++++++++++++
xen/arch/arm/pci/pci-host-generic.c | 42 +++++
xen/include/asm-arm/device.h | 2 +
xen/include/asm-arm/pci.h | 59 +++++++
8 files changed, 507 insertions(+)
create mode 100644 xen/arch/arm/pci/ecam.c
create mode 100644 xen/arch/arm/pci/pci-access.c
create mode 100644 xen/arch/arm/pci/pci-host-common.c
create mode 100644 xen/arch/arm/pci/pci-host-generic.c
diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
index 70cd6c1a19..197bb3c6e8 100644
--- a/xen/arch/arm/device.c
+++ b/xen/arch/arm/device.c
@@ -44,6 +44,8 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
{
ASSERT(desc->init != NULL);
+ dev->dev.of_match_table = desc->dt_match;
+
return desc->init(dev, data);
}
diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
index a98035df4c..e86f2b46fd 100644
--- a/xen/arch/arm/pci/Makefile
+++ b/xen/arch/arm/pci/Makefile
@@ -1 +1,6 @@
obj-y += pci.o
+obj-y += pci-access.o
+obj-y += pci.o
+obj-y += pci-host-generic.o
+obj-y += pci-host-common.o
+obj-y += ecam.o
diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
new file mode 100644
index 0000000000..9b88b1ceda
--- /dev/null
+++ b/xen/arch/arm/pci/ecam.c
@@ -0,0 +1,60 @@
+/*
+ * Based on Linux drivers/pci/ecam.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/pci.h>
+#include <xen/sched.h>
+
+/*
+ * Function to implement the pci_ops ->map_bus method.
+ */
+void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
+ uint32_t sbdf, uint32_t where)
+{
+ const struct pci_config_window *cfg = bridge->cfg;
+ const struct pci_ecam_ops *ops = bridge->sysdata;
+ unsigned int devfn_shift = ops->bus_shift - 8;
+ void __iomem *base;
+
+ unsigned int busn = PCI_BUS(sbdf);
+
+ if ( busn < cfg->busn_start || busn > cfg->busn_end )
+ return NULL;
+
+ busn -= cfg->busn_start;
+ base = cfg->win + (busn << ops->bus_shift);
+
+ return base + (PCI_DEVFN2(sbdf) << devfn_shift) + where;
+}
+
+/* ECAM ops */
+const struct pci_ecam_ops pci_generic_ecam_ops = {
+ .bus_shift = 20,
+ .pci_ops = {
+ .map_bus = pci_ecam_map_bus,
+ .read = pci_generic_config_read,
+ .write = pci_generic_config_write,
+ }
+};
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
new file mode 100644
index 0000000000..04fe9fbf92
--- /dev/null
+++ b/xen/arch/arm/pci/pci-access.c
@@ -0,0 +1,83 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/pci.h>
+#include <asm/io.h>
+
+#define INVALID_VALUE (~0U)
+
+int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t reg, uint32_t len, uint32_t *value)
+{
+ void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
+
+ if ( !addr )
+ {
+ *value = INVALID_VALUE;
+ return -ENODEV;
+ }
+
+ switch ( len )
+ {
+ case 1:
+ *value = readb(addr);
+ break;
+ case 2:
+ *value = readw(addr);
+ break;
+ case 4:
+ *value = readl(addr);
+ break;
+ default:
+ ASSERT_UNREACHABLE();
+ }
+
+ return 0;
+}
+
+int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t reg, uint32_t len, uint32_t value)
+{
+ void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
+
+ if ( !addr )
+ return -ENODEV;
+
+ switch ( len )
+ {
+ case 1:
+ writeb(value, addr);
+ break;
+ case 2:
+ writew(value, addr);
+ break;
+ case 4:
+ writel(value, addr);
+ break;
+ default:
+ ASSERT_UNREACHABLE();
+ }
+
+ return 0;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
new file mode 100644
index 0000000000..4beec14f2f
--- /dev/null
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -0,0 +1,254 @@
+/*
+ * Based on Linux drivers/pci/ecam.c
+ * Based on Linux drivers/pci/controller/pci-host-common.c
+ * Based on Linux drivers/pci/controller/pci-host-generic.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <xen/init.h>
+#include <xen/pci.h>
+#include <xen/rwlock.h>
+#include <xen/sched.h>
+#include <xen/vmap.h>
+
+/*
+ * List for all the pci host bridges.
+ */
+
+static LIST_HEAD(pci_host_bridges);
+
+static atomic_t domain_nr = ATOMIC_INIT(-1);
+
+static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
+{
+ return ioremap_nocache(start, len);
+}
+
+static void pci_ecam_free(struct pci_config_window *cfg)
+{
+ if ( cfg->win )
+ iounmap(cfg->win);
+
+ xfree(cfg);
+}
+
+static struct pci_config_window * __init
+gen_pci_init(struct dt_device_node *dev, const struct pci_ecam_ops *ops)
+{
+ int err, cfg_reg_idx;
+ u32 bus_range[2];
+ paddr_t addr, size;
+ struct pci_config_window *cfg;
+
+ cfg = xzalloc(struct pci_config_window);
+ if ( !cfg )
+ return NULL;
+
+ err = dt_property_read_u32_array(dev, "bus-range", bus_range,
+ ARRAY_SIZE(bus_range));
+ if ( err ) {
+ cfg->busn_start = 0;
+ cfg->busn_end = 0xff;
+ printk(XENLOG_INFO "%s: No bus range found for pci controller\n",
+ dt_node_full_name(dev));
+ } else {
+ cfg->busn_start = bus_range[0];
+ cfg->busn_end = bus_range[1];
+ if ( cfg->busn_end > cfg->busn_start + 0xff )
+ cfg->busn_end = cfg->busn_start + 0xff;
+ }
+
+ if ( ops->cfg_reg_index )
+ {
+ cfg_reg_idx = ops->cfg_reg_index(dev);
+ if ( cfg_reg_idx < 0 )
+ goto err_exit;
+ }
+ else
+ cfg_reg_idx = 0;
+
+ /* Parse our PCI ecam register address */
+ err = dt_device_get_address(dev, cfg_reg_idx, &addr, &size);
+ if ( err )
+ goto err_exit;
+
+ cfg->phys_addr = addr;
+ cfg->size = size;
+
+ /*
+ * On 64-bit systems, we do a single ioremap for the whole config space
+ * since we have enough virtual address range available. On 32-bit, we
+ * ioremap the config space for each bus individually.
+ * As of now only 64-bit is supported 32-bit is not supported.
+ *
+ * TODO: For 32-bit implement the ioremap/iounmap of config space
+ * dynamically for each read/write call.
+ */
+ cfg->win = pci_remap_cfgspace(cfg->phys_addr, cfg->size);
+ if ( !cfg->win )
+ goto err_exit_remap;
+
+ printk("ECAM at [mem 0x%"PRIpaddr"-0x%"PRIpaddr"] for [bus %x-%x] \n",
+ cfg->phys_addr, cfg->phys_addr + cfg->size - 1,
+ cfg->busn_start, cfg->busn_end);
+
+ if ( ops->init )
+ {
+ err = ops->init(cfg);
+ if (err)
+ goto err_exit;
+ }
+
+ return cfg;
+
+err_exit_remap:
+ printk(XENLOG_ERR "ECAM ioremap failed\n");
+err_exit:
+ pci_ecam_free(cfg);
+
+ return NULL;
+}
+
+struct pci_host_bridge *pci_alloc_host_bridge(void)
+{
+ struct pci_host_bridge *bridge = xzalloc(struct pci_host_bridge);
+
+ if ( !bridge )
+ return NULL;
+
+ INIT_LIST_HEAD(&bridge->node);
+ bridge->bus_start = UINT8_MAX;
+ bridge->bus_end = UINT8_MAX;
+
+ return bridge;
+}
+
+void pci_add_host_bridge(struct pci_host_bridge *bridge)
+{
+ list_add_tail(&bridge->node, &pci_host_bridges);
+}
+
+static int pci_get_new_domain_nr(void)
+{
+ return atomic_inc_return(&domain_nr);
+}
+
+static int pci_bus_find_domain_nr(struct dt_device_node *dev)
+{
+ static int use_dt_domains = -1;
+ int domain;
+
+ domain = dt_get_pci_domain_nr(dev);
+
+ /*
+ * Check DT domain and use_dt_domains values.
+ *
+ * If DT domain property is valid (domain >= 0) and
+ * use_dt_domains != 0, the DT assignment is valid since this means
+ * we have not previously allocated a domain number by using
+ * pci_get_new_domain_nr(); we should also update use_dt_domains to
+ * 1, to indicate that we have just assigned a domain number from
+ * DT.
+ *
+ * If DT domain property value is not valid (ie domain < 0), and we
+ * have not previously assigned a domain number from DT
+ * (use_dt_domains != 1) we should assign a domain number by
+ * using the:
+ *
+ * pci_get_new_domain_nr()
+ *
+ * API and update the use_dt_domains value to keep track of method we
+ * are using to assign domain numbers (use_dt_domains = 0).
+ *
+ * All other combinations imply we have a platform that is trying
+ * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
+ * which is a recipe for domain mishandling and it is prevented by
+ * invalidating the domain value (domain = -1) and printing a
+ * corresponding error.
+ */
+ if ( domain >= 0 && use_dt_domains )
+ {
+ use_dt_domains = 1;
+ }
+ else if ( domain < 0 && use_dt_domains != 1 )
+ {
+ use_dt_domains = 0;
+ domain = pci_get_new_domain_nr();
+ }
+ else
+ {
+ domain = -1;
+ }
+
+ return domain;
+}
+
+int pci_host_common_probe(struct dt_device_node *dev, const void *data)
+{
+ struct pci_host_bridge *bridge;
+ struct pci_config_window *cfg;
+ struct pci_ecam_ops *ops;
+ const struct dt_device_match *of_id;
+ int err;
+
+ if ( dt_device_for_passthrough(dev) )
+ return 0;
+
+ of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
+ ops = (struct pci_ecam_ops *) of_id->data;
+
+ bridge = pci_alloc_host_bridge();
+ if ( !bridge )
+ return -ENOMEM;
+
+ /* Parse and map our Configuration Space windows */
+ cfg = gen_pci_init(dev, ops);
+ if ( !cfg )
+ {
+ err = -ENOMEM;
+ goto err_exit;
+ }
+
+ bridge->dt_node = dev;
+ bridge->cfg = cfg;
+ bridge->sysdata = ops;
+ bridge->ops = &ops->pci_ops;
+ bridge->bus_start = cfg->busn_start;
+ bridge->bus_end = cfg->busn_end;
+
+ bridge->segment = pci_bus_find_domain_nr(dev);
+ if ( bridge->segment < 0 )
+ {
+ printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
+ BUG();
+ }
+ pci_add_host_bridge(bridge);
+
+ return 0;
+
+err_exit:
+ xfree(bridge);
+
+ return err;
+}
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
new file mode 100644
index 0000000000..6b3288d6f3
--- /dev/null
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -0,0 +1,42 @@
+/*
+ * Based on Linux drivers/pci/controller/pci-host-common.c
+ * Based on Linux drivers/pci/controller/pci-host-generic.c
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program. If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <asm/device.h>
+#include <xen/pci.h>
+#include <asm/pci.h>
+
+static const struct dt_device_match gen_pci_dt_match[] = {
+ { .compatible = "pci-host-ecam-generic",
+ .data = &pci_generic_ecam_ops },
+
+ { },
+};
+
+DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
+.dt_match = gen_pci_dt_match,
+.init = pci_host_common_probe,
+DT_DEVICE_END
+
+/*
+ * Local variables:
+ * mode: C
+ * c-file-style: "BSD"
+ * c-basic-offset: 4
+ * tab-width: 4
+ * indent-tabs-mode: nil
+ * End:
+ */
diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
index 5ecd5e7bd1..582119c31e 100644
--- a/xen/include/asm-arm/device.h
+++ b/xen/include/asm-arm/device.h
@@ -16,6 +16,8 @@ struct device
enum device_type type;
#ifdef CONFIG_HAS_DEVICE_TREE
struct dt_device_node *of_node; /* Used by drivers imported from Linux */
+ /* Used by drivers imported from Linux */
+ const struct dt_device_match *of_match_table;
#endif
struct dev_archdata archdata;
struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
index f2f86be9bc..4b32c7088a 100644
--- a/xen/include/asm-arm/pci.h
+++ b/xen/include/asm-arm/pci.h
@@ -26,6 +26,65 @@ struct arch_pci_dev {
struct device dev;
};
+/*
+ * struct to hold the mappings of a config space window. This
+ * is expected to be used as sysdata for PCI controllers that
+ * use ECAM.
+ */
+struct pci_config_window {
+ paddr_t phys_addr;
+ paddr_t size;
+ uint8_t busn_start;
+ uint8_t busn_end;
+ void __iomem *win;
+};
+
+/*
+ * struct to hold pci host bridge information
+ * for a PCI controller.
+ */
+struct pci_host_bridge {
+ struct dt_device_node *dt_node; /* Pointer to the associated DT node */
+ struct list_head node; /* Node in list of host bridges */
+ uint16_t segment; /* Segment number */
+ uint8_t bus_start; /* Bus start of this bridge. */
+ uint8_t bus_end; /* Bus end of this bridge. */
+ struct pci_config_window* cfg; /* Pointer to the bridge config window */
+ void *sysdata; /* Pointer to the config space window*/
+ const struct pci_ops *ops;
+};
+
+struct pci_ops {
+ void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t offset);
+ int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t reg, uint32_t len, uint32_t *value);
+ int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t reg, uint32_t len, uint32_t value);
+};
+
+/*
+ * struct to hold pci ops and bus shift of the config window
+ * for a PCI controller.
+ */
+struct pci_ecam_ops {
+ unsigned int bus_shift;
+ struct pci_ops pci_ops;
+ int (*cfg_reg_index)(struct dt_device_node *dev);
+ int (*init)(struct pci_config_window *);
+};
+
+/* Default ECAM ops */
+extern const struct pci_ecam_ops pci_generic_ecam_ops;
+
+int pci_host_common_probe(struct dt_device_node *dev, const void *data);
+int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t reg, uint32_t len, uint32_t *value);
+int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
+ uint32_t reg, uint32_t len, uint32_t value);
+void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
+ uint32_t sbdf, uint32_t where);
+
static always_inline bool is_pci_passthrough_enabled(void)
{
return pci_passthrough_enabled;
--
2.17.1
On Wed, 22 Sep 2021, Rahul Singh wrote:
> XEN during boot will read the PCI device tree node “reg” property
> and will map the PCI config space to the XEN memory.
>
> As of now only "pci-host-ecam-generic" compatible board is supported.
>
> "linux,pci-domain" device tree property assigns a fixed PCI domain
> number to a host bridge, otherwise an unstable (across boots) unique
> number will be assigned by Linux. XEN access the PCI devices based on
> Segment:Bus:Device:Function. Segment number in XEN is same as domain
^ A segment ^ the ^ a
> number in Linux.Segment number and domain number has to be in sync
^ " "
> to access the correct PCI devices.
>
> XEN will read the “linux,pci-domain” property from the device tree node
> and configure the host bridge segment number accordingly. If this
> property is not available XEN will allocate the unique segment number
> to the host bridge.
>
> dt_pci_bus_find_domain_nr(..) imported from the Linux source tree with
> slight modification based on tag Linux v5.14.2
> commit bbdd3de144fc142f2f4b9834c9241cc4e7f3d3fc.
dt_pci_bus_find_domain_nr is not introduced by this patch any longer
> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
> ---
> Change in v2:
> - Add more info in commit msg
> - Add callback to parse register index.
> - Merge patch pci_ecam_operation into this patch to avoid confusion
> - Add new struct in struct device for match table
> ---
> xen/arch/arm/device.c | 2 +
> xen/arch/arm/pci/Makefile | 5 +
> xen/arch/arm/pci/ecam.c | 60 +++++++
> xen/arch/arm/pci/pci-access.c | 83 +++++++++
> xen/arch/arm/pci/pci-host-common.c | 254 ++++++++++++++++++++++++++++
> xen/arch/arm/pci/pci-host-generic.c | 42 +++++
> xen/include/asm-arm/device.h | 2 +
> xen/include/asm-arm/pci.h | 59 +++++++
> 8 files changed, 507 insertions(+)
> create mode 100644 xen/arch/arm/pci/ecam.c
> create mode 100644 xen/arch/arm/pci/pci-access.c
> create mode 100644 xen/arch/arm/pci/pci-host-common.c
> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>
> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
> index 70cd6c1a19..197bb3c6e8 100644
> --- a/xen/arch/arm/device.c
> +++ b/xen/arch/arm/device.c
> @@ -44,6 +44,8 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
> {
> ASSERT(desc->init != NULL);
>
> + dev->dev.of_match_table = desc->dt_match;
> +
> return desc->init(dev, data);
> }
>
> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
> index a98035df4c..e86f2b46fd 100644
> --- a/xen/arch/arm/pci/Makefile
> +++ b/xen/arch/arm/pci/Makefile
> @@ -1 +1,6 @@
> obj-y += pci.o
> +obj-y += pci-access.o
> +obj-y += pci.o
Added twice?
> +obj-y += pci-host-generic.o
> +obj-y += pci-host-common.o
> +obj-y += ecam.o
> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
> new file mode 100644
> index 0000000000..9b88b1ceda
> --- /dev/null
> +++ b/xen/arch/arm/pci/ecam.c
> @@ -0,0 +1,60 @@
> +/*
> + * Based on Linux drivers/pci/ecam.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/pci.h>
> +#include <xen/sched.h>
> +
> +/*
> + * Function to implement the pci_ops ->map_bus method.
> + */
> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> + uint32_t sbdf, uint32_t where)
> +{
> + const struct pci_config_window *cfg = bridge->cfg;
> + const struct pci_ecam_ops *ops = bridge->sysdata;
> + unsigned int devfn_shift = ops->bus_shift - 8;
> + void __iomem *base;
> +
> + unsigned int busn = PCI_BUS(sbdf);
> +
> + if ( busn < cfg->busn_start || busn > cfg->busn_end )
> + return NULL;
> +
> + busn -= cfg->busn_start;
> + base = cfg->win + (busn << ops->bus_shift);
> +
> + return base + (PCI_DEVFN2(sbdf) << devfn_shift) + where;
> +}
> +
> +/* ECAM ops */
> +const struct pci_ecam_ops pci_generic_ecam_ops = {
> + .bus_shift = 20,
> + .pci_ops = {
> + .map_bus = pci_ecam_map_bus,
> + .read = pci_generic_config_read,
> + .write = pci_generic_config_write,
> + }
> +};
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
> new file mode 100644
> index 0000000000..04fe9fbf92
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-access.c
> @@ -0,0 +1,83 @@
> +/*
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/pci.h>
> +#include <asm/io.h>
> +
> +#define INVALID_VALUE (~0U)
> +
> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t reg, uint32_t len, uint32_t *value)
> +{
> + void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
> +
> + if ( !addr )
> + {
> + *value = INVALID_VALUE;
> + return -ENODEV;
> + }
> +
> + switch ( len )
> + {
> + case 1:
> + *value = readb(addr);
> + break;
> + case 2:
> + *value = readw(addr);
> + break;
> + case 4:
> + *value = readl(addr);
> + break;
> + default:
> + ASSERT_UNREACHABLE();
> + }
> +
> + return 0;
> +}
> +
> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t reg, uint32_t len, uint32_t value)
^ code style
> +{
> + void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
> +
> + if ( !addr )
> + return -ENODEV;
> +
> + switch ( len )
> + {
> + case 1:
> + writeb(value, addr);
> + break;
> + case 2:
> + writew(value, addr);
> + break;
> + case 4:
> + writel(value, addr);
> + break;
> + default:
> + ASSERT_UNREACHABLE();
> + }
> +
> + return 0;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> new file mode 100644
> index 0000000000..4beec14f2f
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -0,0 +1,254 @@
> +/*
> + * Based on Linux drivers/pci/ecam.c
> + * Based on Linux drivers/pci/controller/pci-host-common.c
> + * Based on Linux drivers/pci/controller/pci-host-generic.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <xen/init.h>
> +#include <xen/pci.h>
> +#include <xen/rwlock.h>
> +#include <xen/sched.h>
> +#include <xen/vmap.h>
> +
> +/*
> + * List for all the pci host bridges.
> + */
> +
> +static LIST_HEAD(pci_host_bridges);
> +
> +static atomic_t domain_nr = ATOMIC_INIT(-1);
> +
> +static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
> +{
> + return ioremap_nocache(start, len);
> +}
> +
> +static void pci_ecam_free(struct pci_config_window *cfg)
> +{
> + if ( cfg->win )
> + iounmap(cfg->win);
> +
> + xfree(cfg);
> +}
> +
> +static struct pci_config_window * __init
> +gen_pci_init(struct dt_device_node *dev, const struct pci_ecam_ops *ops)
> +{
> + int err, cfg_reg_idx;
> + u32 bus_range[2];
> + paddr_t addr, size;
> + struct pci_config_window *cfg;
> +
> + cfg = xzalloc(struct pci_config_window);
> + if ( !cfg )
> + return NULL;
> +
> + err = dt_property_read_u32_array(dev, "bus-range", bus_range,
> + ARRAY_SIZE(bus_range));
> + if ( err ) {
> + cfg->busn_start = 0;
> + cfg->busn_end = 0xff;
> + printk(XENLOG_INFO "%s: No bus range found for pci controller\n",
> + dt_node_full_name(dev));
> + } else {
> + cfg->busn_start = bus_range[0];
> + cfg->busn_end = bus_range[1];
> + if ( cfg->busn_end > cfg->busn_start + 0xff )
> + cfg->busn_end = cfg->busn_start + 0xff;
> + }
> +
> + if ( ops->cfg_reg_index )
> + {
> + cfg_reg_idx = ops->cfg_reg_index(dev);
> + if ( cfg_reg_idx < 0 )
> + goto err_exit;
> + }
> + else
> + cfg_reg_idx = 0;
> +
> + /* Parse our PCI ecam register address */
> + err = dt_device_get_address(dev, cfg_reg_idx, &addr, &size);
> + if ( err )
> + goto err_exit;
> +
> + cfg->phys_addr = addr;
> + cfg->size = size;
> +
> + /*
> + * On 64-bit systems, we do a single ioremap for the whole config space
> + * since we have enough virtual address range available. On 32-bit, we
> + * ioremap the config space for each bus individually.
> + * As of now only 64-bit is supported 32-bit is not supported.
> + *
> + * TODO: For 32-bit implement the ioremap/iounmap of config space
> + * dynamically for each read/write call.
> + */
> + cfg->win = pci_remap_cfgspace(cfg->phys_addr, cfg->size);
> + if ( !cfg->win )
> + goto err_exit_remap;
> +
> + printk("ECAM at [mem 0x%"PRIpaddr"-0x%"PRIpaddr"] for [bus %x-%x] \n",
> + cfg->phys_addr, cfg->phys_addr + cfg->size - 1,
> + cfg->busn_start, cfg->busn_end);
> +
> + if ( ops->init )
> + {
> + err = ops->init(cfg);
> + if (err)
code style
> + goto err_exit;
> + }
This is unnecessary at the moment, right? Can we get rid of ops->init ?
> + return cfg;
> +
> +err_exit_remap:
> + printk(XENLOG_ERR "ECAM ioremap failed\n");
No need for err_exit_remap as pci_ecam_free can take care of both cases.
> +err_exit:
> + pci_ecam_free(cfg);
> +
> + return NULL;
> +}
> +
> +struct pci_host_bridge *pci_alloc_host_bridge(void)
> +{
> + struct pci_host_bridge *bridge = xzalloc(struct pci_host_bridge);
> +
> + if ( !bridge )
> + return NULL;
> +
> + INIT_LIST_HEAD(&bridge->node);
> + bridge->bus_start = UINT8_MAX;
> + bridge->bus_end = UINT8_MAX;
> +
> + return bridge;
> +}
> +
> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
> +{
> + list_add_tail(&bridge->node, &pci_host_bridges);
> +}
> +
> +static int pci_get_new_domain_nr(void)
> +{
> + return atomic_inc_return(&domain_nr);
> +}
> +
> +static int pci_bus_find_domain_nr(struct dt_device_node *dev)
> +{
> + static int use_dt_domains = -1;
> + int domain;
> +
> + domain = dt_get_pci_domain_nr(dev);
> +
> + /*
> + * Check DT domain and use_dt_domains values.
> + *
> + * If DT domain property is valid (domain >= 0) and
> + * use_dt_domains != 0, the DT assignment is valid since this means
> + * we have not previously allocated a domain number by using
> + * pci_get_new_domain_nr(); we should also update use_dt_domains to
> + * 1, to indicate that we have just assigned a domain number from
> + * DT.
> + *
> + * If DT domain property value is not valid (ie domain < 0), and we
> + * have not previously assigned a domain number from DT
> + * (use_dt_domains != 1) we should assign a domain number by
> + * using the:
> + *
> + * pci_get_new_domain_nr()
> + *
> + * API and update the use_dt_domains value to keep track of method we
> + * are using to assign domain numbers (use_dt_domains = 0).
> + *
> + * All other combinations imply we have a platform that is trying
> + * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
> + * which is a recipe for domain mishandling and it is prevented by
> + * invalidating the domain value (domain = -1) and printing a
> + * corresponding error.
> + */
> + if ( domain >= 0 && use_dt_domains )
> + {
> + use_dt_domains = 1;
> + }
> + else if ( domain < 0 && use_dt_domains != 1 )
> + {
> + use_dt_domains = 0;
> + domain = pci_get_new_domain_nr();
> + }
> + else
> + {
> + domain = -1;
> + }
> +
> + return domain;
> +}
> +
> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> +{
> + struct pci_host_bridge *bridge;
> + struct pci_config_window *cfg;
> + struct pci_ecam_ops *ops;
> + const struct dt_device_match *of_id;
> + int err;
> +
> + if ( dt_device_for_passthrough(dev) )
> + return 0;
> +
> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> + ops = (struct pci_ecam_ops *) of_id->data;
Do we really need dt_match_node and dev->dev.of_match_table to get
dt_device_match.data?
data is passed as a parameter to pci_host_common_probe, isn't it enough
to do:
ops = (struct pci_ecam_ops *) data;
> + bridge = pci_alloc_host_bridge();
> + if ( !bridge )
> + return -ENOMEM;
> +
> + /* Parse and map our Configuration Space windows */
> + cfg = gen_pci_init(dev, ops);
> + if ( !cfg )
> + {
> + err = -ENOMEM;
> + goto err_exit;
> + }
> +
> + bridge->dt_node = dev;
> + bridge->cfg = cfg;
> + bridge->sysdata = ops;
> + bridge->ops = &ops->pci_ops;
> + bridge->bus_start = cfg->busn_start;
> + bridge->bus_end = cfg->busn_end;
> +
> + bridge->segment = pci_bus_find_domain_nr(dev);
> + if ( bridge->segment < 0 )
> + {
> + printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
> + BUG();
> + }
> + pci_add_host_bridge(bridge);
> +
> + return 0;
> +
> +err_exit:
> + xfree(bridge);
> +
> + return err;
> +}
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
> new file mode 100644
> index 0000000000..6b3288d6f3
> --- /dev/null
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -0,0 +1,42 @@
> +/*
> + * Based on Linux drivers/pci/controller/pci-host-common.c
> + * Based on Linux drivers/pci/controller/pci-host-generic.c
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <asm/device.h>
> +#include <xen/pci.h>
> +#include <asm/pci.h>
> +
> +static const struct dt_device_match gen_pci_dt_match[] = {
> + { .compatible = "pci-host-ecam-generic",
> + .data = &pci_generic_ecam_ops },
> +
> + { },
> +};
> +
> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> +.dt_match = gen_pci_dt_match,
> +.init = pci_host_common_probe,
> +DT_DEVICE_END
> +
> +/*
> + * Local variables:
> + * mode: C
> + * c-file-style: "BSD"
> + * c-basic-offset: 4
> + * tab-width: 4
> + * indent-tabs-mode: nil
> + * End:
> + */
> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
> index 5ecd5e7bd1..582119c31e 100644
> --- a/xen/include/asm-arm/device.h
> +++ b/xen/include/asm-arm/device.h
> @@ -16,6 +16,8 @@ struct device
> enum device_type type;
> #ifdef CONFIG_HAS_DEVICE_TREE
> struct dt_device_node *of_node; /* Used by drivers imported from Linux */
> + /* Used by drivers imported from Linux */
> + const struct dt_device_match *of_match_table;
> #endif
> struct dev_archdata archdata;
> struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
> index f2f86be9bc..4b32c7088a 100644
> --- a/xen/include/asm-arm/pci.h
> +++ b/xen/include/asm-arm/pci.h
> @@ -26,6 +26,65 @@ struct arch_pci_dev {
> struct device dev;
> };
>
> +/*
> + * struct to hold the mappings of a config space window. This
> + * is expected to be used as sysdata for PCI controllers that
> + * use ECAM.
> + */
> +struct pci_config_window {
> + paddr_t phys_addr;
> + paddr_t size;
> + uint8_t busn_start;
> + uint8_t busn_end;
> + void __iomem *win;
> +};
> +
> +/*
> + * struct to hold pci host bridge information
> + * for a PCI controller.
> + */
> +struct pci_host_bridge {
> + struct dt_device_node *dt_node; /* Pointer to the associated DT node */
> + struct list_head node; /* Node in list of host bridges */
> + uint16_t segment; /* Segment number */
> + uint8_t bus_start; /* Bus start of this bridge. */
> + uint8_t bus_end; /* Bus end of this bridge. */
bus_start and bus_end are both here and also under pci_config_window.
Should we remove them from here (if not, then can we removed them from
struct pci_config_window)?
> + struct pci_config_window* cfg; /* Pointer to the bridge config window */
> + void *sysdata; /* Pointer to the config space window*/
> + const struct pci_ops *ops;
It looks like sysdata is unnecessary because we can get the right
pointer from ops, given that ops is pointing to a member of the struct
point by sysdata. In other words, if you use container_of(ops, struct
pci_ecam_ops, pci_ops) it should work?
> +};
> +
> +struct pci_ops {
> + void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t offset);
> + int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t reg, uint32_t len, uint32_t *value);
> + int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t reg, uint32_t len, uint32_t value);
> +};
> +
> +/*
> + * struct to hold pci ops and bus shift of the config window
> + * for a PCI controller.
> + */
> +struct pci_ecam_ops {
> + unsigned int bus_shift;
> + struct pci_ops pci_ops;
> + int (*cfg_reg_index)(struct dt_device_node *dev);
> + int (*init)(struct pci_config_window *);
init is unused, can we get rid of it?
> +};
> +
> +/* Default ECAM ops */
> +extern const struct pci_ecam_ops pci_generic_ecam_ops;
If we use container_of and get rid of sysdata, I wonder if we get avoid
exporting pci_generic_ecam_ops.
> +int pci_host_common_probe(struct dt_device_node *dev, const void *data);
This should be static and not exported
> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t reg, uint32_t len, uint32_t *value);
also this
> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> + uint32_t reg, uint32_t len, uint32_t value);
also this
> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> + uint32_t sbdf, uint32_t where);
also this
> static always_inline bool is_pci_passthrough_enabled(void)
> {
> return pci_passthrough_enabled;
> --
> 2.17.1
>
Hi, Stefano!
[snip]
>> +};
>> +
>> +/* Default ECAM ops */
>> +extern const struct pci_ecam_ops pci_generic_ecam_ops;
> If we use container_of and get rid of sysdata, I wonder if we get avoid
> exporting pci_generic_ecam_ops.
>
>
>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data);
> This should be static and not exported
>
>
>> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t *value);
> also this
>
>
>> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t value);
> also this
>
>
>> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>> + uint32_t sbdf, uint32_t where);
> also this
>
>> static always_inline bool is_pci_passthrough_enabled(void)
>> {
>> return pci_passthrough_enabled;
>> --
>> 2.17.1
All the above is still need to be exported as those are going to be used
by other bridge's implementations, see ZynqMP for instance later in the series.
I'll post a snippet from the future:
/* ECAM ops */
const struct pci_ecam_ops nwl_pcie_ops = {
.bus_shift = 20,
.cfg_reg_index = nwl_cfg_reg_index,
.pci_ops = {
.map_bus = pci_ecam_map_bus,
.read = pci_generic_config_read,
.write = pci_generic_config_write,
.need_p2m_mapping = pci_ecam_need_p2m_mapping,
}
};
DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
.dt_match = nwl_pcie_dt_match,
.init = pci_host_common_probe,
DT_DEVICE_END
On Thu, 23 Sep 2021, Oleksandr Andrushchenko wrote:
> Hi, Stefano!
>
> [snip]
>
>
> >> +};
> >> +
> >> +/* Default ECAM ops */
> >> +extern const struct pci_ecam_ops pci_generic_ecam_ops;
> > If we use container_of and get rid of sysdata, I wonder if we get avoid
> > exporting pci_generic_ecam_ops.
> >
> >
> >> +int pci_host_common_probe(struct dt_device_node *dev, const void *data);
> > This should be static and not exported
> >
> >
> >> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> + uint32_t reg, uint32_t len, uint32_t *value);
> > also this
> >
> >
> >> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
> >> + uint32_t reg, uint32_t len, uint32_t value);
> > also this
> >
> >
> >> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
> >> + uint32_t sbdf, uint32_t where);
> > also this
> >
> >> static always_inline bool is_pci_passthrough_enabled(void)
> >> {
> >> return pci_passthrough_enabled;
> >> --
> >> 2.17.1
>
> All the above is still need to be exported as those are going to be used
>
> by other bridge's implementations, see ZynqMP for instance later in the series.
>
> I'll post a snippet from the future:
>
> /* ECAM ops */
> const struct pci_ecam_ops nwl_pcie_ops = {
> .bus_shift = 20,
> .cfg_reg_index = nwl_cfg_reg_index,
> .pci_ops = {
> .map_bus = pci_ecam_map_bus,
> .read = pci_generic_config_read,
> .write = pci_generic_config_write,
> .need_p2m_mapping = pci_ecam_need_p2m_mapping,
> }
> };
>
> DT_DEVICE_START(pci_gen, "PCI HOST ZYNQMP", DEVICE_PCI)
> .dt_match = nwl_pcie_dt_match,
> .init = pci_host_common_probe,
> DT_DEVICE_END
OK then
Hi Stefano,
> On 23 Sep 2021, at 3:09 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Wed, 22 Sep 2021, Rahul Singh wrote:
>> XEN during boot will read the PCI device tree node “reg” property
>> and will map the PCI config space to the XEN memory.
>>
>> As of now only "pci-host-ecam-generic" compatible board is supported.
>>
>> "linux,pci-domain" device tree property assigns a fixed PCI domain
>> number to a host bridge, otherwise an unstable (across boots) unique
>> number will be assigned by Linux. XEN access the PCI devices based on
>> Segment:Bus:Device:Function. Segment number in XEN is same as domain
> ^ A segment ^ the ^ a
Ack.
>
>
>> number in Linux.Segment number and domain number has to be in sync
> ^ “ “
>
Ack.
>> to access the correct PCI devices.
>>
>> XEN will read the “linux,pci-domain” property from the device tree node
>> and configure the host bridge segment number accordingly. If this
>> property is not available XEN will allocate the unique segment number
>> to the host bridge.
>>
>> dt_pci_bus_find_domain_nr(..) imported from the Linux source tree with
>> slight modification based on tag Linux v5.14.2
>> commit bbdd3de144fc142f2f4b9834c9241cc4e7f3d3fc.
>
> dt_pci_bus_find_domain_nr is not introduced by this patch any longer
Ack.
>
>
>> Signed-off-by: Rahul Singh <rahul.singh@arm.com>
>> ---
>> Change in v2:
>> - Add more info in commit msg
>> - Add callback to parse register index.
>> - Merge patch pci_ecam_operation into this patch to avoid confusion
>> - Add new struct in struct device for match table
>> ---
>> xen/arch/arm/device.c | 2 +
>> xen/arch/arm/pci/Makefile | 5 +
>> xen/arch/arm/pci/ecam.c | 60 +++++++
>> xen/arch/arm/pci/pci-access.c | 83 +++++++++
>> xen/arch/arm/pci/pci-host-common.c | 254 ++++++++++++++++++++++++++++
>> xen/arch/arm/pci/pci-host-generic.c | 42 +++++
>> xen/include/asm-arm/device.h | 2 +
>> xen/include/asm-arm/pci.h | 59 +++++++
>> 8 files changed, 507 insertions(+)
>> create mode 100644 xen/arch/arm/pci/ecam.c
>> create mode 100644 xen/arch/arm/pci/pci-access.c
>> create mode 100644 xen/arch/arm/pci/pci-host-common.c
>> create mode 100644 xen/arch/arm/pci/pci-host-generic.c
>>
>> diff --git a/xen/arch/arm/device.c b/xen/arch/arm/device.c
>> index 70cd6c1a19..197bb3c6e8 100644
>> --- a/xen/arch/arm/device.c
>> +++ b/xen/arch/arm/device.c
>> @@ -44,6 +44,8 @@ int __init device_init(struct dt_device_node *dev, enum device_class class,
>> {
>> ASSERT(desc->init != NULL);
>>
>> + dev->dev.of_match_table = desc->dt_match;
>> +
>> return desc->init(dev, data);
>> }
>>
>> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile
>> index a98035df4c..e86f2b46fd 100644
>> --- a/xen/arch/arm/pci/Makefile
>> +++ b/xen/arch/arm/pci/Makefile
>> @@ -1 +1,6 @@
>> obj-y += pci.o
>> +obj-y += pci-access.o
>> +obj-y += pci.o
>
> Added twice?
Ack.
>
>
>> +obj-y += pci-host-generic.o
>> +obj-y += pci-host-common.o
>> +obj-y += ecam.o
>> diff --git a/xen/arch/arm/pci/ecam.c b/xen/arch/arm/pci/ecam.c
>> new file mode 100644
>> index 0000000000..9b88b1ceda
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/ecam.c
>> @@ -0,0 +1,60 @@
>> +/*
>> + * Based on Linux drivers/pci/ecam.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/pci.h>
>> +#include <xen/sched.h>
>> +
>> +/*
>> + * Function to implement the pci_ops ->map_bus method.
>> + */
>> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>> + uint32_t sbdf, uint32_t where)
>> +{
>> + const struct pci_config_window *cfg = bridge->cfg;
>> + const struct pci_ecam_ops *ops = bridge->sysdata;
>> + unsigned int devfn_shift = ops->bus_shift - 8;
>> + void __iomem *base;
>> +
>> + unsigned int busn = PCI_BUS(sbdf);
>> +
>> + if ( busn < cfg->busn_start || busn > cfg->busn_end )
>> + return NULL;
>> +
>> + busn -= cfg->busn_start;
>> + base = cfg->win + (busn << ops->bus_shift);
>> +
>> + return base + (PCI_DEVFN2(sbdf) << devfn_shift) + where;
>> +}
>> +
>> +/* ECAM ops */
>> +const struct pci_ecam_ops pci_generic_ecam_ops = {
>> + .bus_shift = 20,
>> + .pci_ops = {
>> + .map_bus = pci_ecam_map_bus,
>> + .read = pci_generic_config_read,
>> + .write = pci_generic_config_write,
>> + }
>> +};
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci-access.c b/xen/arch/arm/pci/pci-access.c
>> new file mode 100644
>> index 0000000000..04fe9fbf92
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-access.c
>> @@ -0,0 +1,83 @@
>> +/*
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/pci.h>
>> +#include <asm/io.h>
>> +
>> +#define INVALID_VALUE (~0U)
>> +
>> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t *value)
>> +{
>> + void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
>> +
>> + if ( !addr )
>> + {
>> + *value = INVALID_VALUE;
>> + return -ENODEV;
>> + }
>> +
>> + switch ( len )
>> + {
>> + case 1:
>> + *value = readb(addr);
>> + break;
>> + case 2:
>> + *value = readw(addr);
>> + break;
>> + case 4:
>> + *value = readl(addr);
>> + break;
>> + default:
>> + ASSERT_UNREACHABLE();
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t value)
> ^ code style
>
Ack.
>
>> +{
>> + void __iomem *addr = bridge->ops->map_bus(bridge, sbdf, reg);
>> +
>> + if ( !addr )
>> + return -ENODEV;
>> +
>> + switch ( len )
>> + {
>> + case 1:
>> + writeb(value, addr);
>> + break;
>> + case 2:
>> + writew(value, addr);
>> + break;
>> + case 4:
>> + writel(value, addr);
>> + break;
>> + default:
>> + ASSERT_UNREACHABLE();
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
>> new file mode 100644
>> index 0000000000..4beec14f2f
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-common.c
>> @@ -0,0 +1,254 @@
>> +/*
>> + * Based on Linux drivers/pci/ecam.c
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <xen/init.h>
>> +#include <xen/pci.h>
>> +#include <xen/rwlock.h>
>> +#include <xen/sched.h>
>> +#include <xen/vmap.h>
>> +
>> +/*
>> + * List for all the pci host bridges.
>> + */
>> +
>> +static LIST_HEAD(pci_host_bridges);
>> +
>> +static atomic_t domain_nr = ATOMIC_INIT(-1);
>> +
>> +static inline void __iomem *pci_remap_cfgspace(paddr_t start, size_t len)
>> +{
>> + return ioremap_nocache(start, len);
>> +}
>> +
>> +static void pci_ecam_free(struct pci_config_window *cfg)
>> +{
>> + if ( cfg->win )
>> + iounmap(cfg->win);
>> +
>> + xfree(cfg);
>> +}
>> +
>> +static struct pci_config_window * __init
>> +gen_pci_init(struct dt_device_node *dev, const struct pci_ecam_ops *ops)
>> +{
>> + int err, cfg_reg_idx;
>> + u32 bus_range[2];
>> + paddr_t addr, size;
>> + struct pci_config_window *cfg;
>> +
>> + cfg = xzalloc(struct pci_config_window);
>> + if ( !cfg )
>> + return NULL;
>> +
>> + err = dt_property_read_u32_array(dev, "bus-range", bus_range,
>> + ARRAY_SIZE(bus_range));
>> + if ( err ) {
>> + cfg->busn_start = 0;
>> + cfg->busn_end = 0xff;
>> + printk(XENLOG_INFO "%s: No bus range found for pci controller\n",
>> + dt_node_full_name(dev));
>> + } else {
>> + cfg->busn_start = bus_range[0];
>> + cfg->busn_end = bus_range[1];
>> + if ( cfg->busn_end > cfg->busn_start + 0xff )
>> + cfg->busn_end = cfg->busn_start + 0xff;
>> + }
>> +
>> + if ( ops->cfg_reg_index )
>> + {
>> + cfg_reg_idx = ops->cfg_reg_index(dev);
>> + if ( cfg_reg_idx < 0 )
>> + goto err_exit;
>> + }
>> + else
>> + cfg_reg_idx = 0;
>> +
>> + /* Parse our PCI ecam register address */
>> + err = dt_device_get_address(dev, cfg_reg_idx, &addr, &size);
>> + if ( err )
>> + goto err_exit;
>> +
>> + cfg->phys_addr = addr;
>> + cfg->size = size;
>> +
>> + /*
>> + * On 64-bit systems, we do a single ioremap for the whole config space
>> + * since we have enough virtual address range available. On 32-bit, we
>> + * ioremap the config space for each bus individually.
>> + * As of now only 64-bit is supported 32-bit is not supported.
>> + *
>> + * TODO: For 32-bit implement the ioremap/iounmap of config space
>> + * dynamically for each read/write call.
>> + */
>> + cfg->win = pci_remap_cfgspace(cfg->phys_addr, cfg->size);
>> + if ( !cfg->win )
>> + goto err_exit_remap;
>> +
>> + printk("ECAM at [mem 0x%"PRIpaddr"-0x%"PRIpaddr"] for [bus %x-%x] \n",
>> + cfg->phys_addr, cfg->phys_addr + cfg->size - 1,
>> + cfg->busn_start, cfg->busn_end);
>> +
>> + if ( ops->init )
>> + {
>> + err = ops->init(cfg);
>> + if (err)
>
> code style
Ack.
>
>
>> + goto err_exit;
>> + }
>
> This is unnecessary at the moment, right? Can we get rid of ops->init ?
No this is required for N1SDP board. Please check below patch.
https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
>
>
>> + return cfg;
>> +
>> +err_exit_remap:
>> + printk(XENLOG_ERR "ECAM ioremap failed\n");
>
> No need for err_exit_remap as pci_ecam_free can take care of both cases.
Ack.
>
>> +err_exit:
>> + pci_ecam_free(cfg);
>> +
>> + return NULL;
>> +}
>> +
>> +struct pci_host_bridge *pci_alloc_host_bridge(void)
>> +{
>> + struct pci_host_bridge *bridge = xzalloc(struct pci_host_bridge);
>> +
>> + if ( !bridge )
>> + return NULL;
>> +
>> + INIT_LIST_HEAD(&bridge->node);
>> + bridge->bus_start = UINT8_MAX;
>> + bridge->bus_end = UINT8_MAX;
>> +
>> + return bridge;
>> +}
>> +
>> +void pci_add_host_bridge(struct pci_host_bridge *bridge)
>> +{
>> + list_add_tail(&bridge->node, &pci_host_bridges);
>> +}
>> +
>> +static int pci_get_new_domain_nr(void)
>> +{
>> + return atomic_inc_return(&domain_nr);
>> +}
>> +
>> +static int pci_bus_find_domain_nr(struct dt_device_node *dev)
>> +{
>> + static int use_dt_domains = -1;
>> + int domain;
>> +
>> + domain = dt_get_pci_domain_nr(dev);
>> +
>> + /*
>> + * Check DT domain and use_dt_domains values.
>> + *
>> + * If DT domain property is valid (domain >= 0) and
>> + * use_dt_domains != 0, the DT assignment is valid since this means
>> + * we have not previously allocated a domain number by using
>> + * pci_get_new_domain_nr(); we should also update use_dt_domains to
>> + * 1, to indicate that we have just assigned a domain number from
>> + * DT.
>> + *
>> + * If DT domain property value is not valid (ie domain < 0), and we
>> + * have not previously assigned a domain number from DT
>> + * (use_dt_domains != 1) we should assign a domain number by
>> + * using the:
>> + *
>> + * pci_get_new_domain_nr()
>> + *
>> + * API and update the use_dt_domains value to keep track of method we
>> + * are using to assign domain numbers (use_dt_domains = 0).
>> + *
>> + * All other combinations imply we have a platform that is trying
>> + * to mix domain numbers obtained from DT and pci_get_new_domain_nr(),
>> + * which is a recipe for domain mishandling and it is prevented by
>> + * invalidating the domain value (domain = -1) and printing a
>> + * corresponding error.
>> + */
>> + if ( domain >= 0 && use_dt_domains )
>> + {
>> + use_dt_domains = 1;
>> + }
>> + else if ( domain < 0 && use_dt_domains != 1 )
>> + {
>> + use_dt_domains = 0;
>> + domain = pci_get_new_domain_nr();
>> + }
>> + else
>> + {
>> + domain = -1;
>> + }
>> +
>> + return domain;
>> +}
>> +
>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
>> +{
>> + struct pci_host_bridge *bridge;
>> + struct pci_config_window *cfg;
>> + struct pci_ecam_ops *ops;
>> + const struct dt_device_match *of_id;
>> + int err;
>> +
>> + if ( dt_device_for_passthrough(dev) )
>> + return 0;
>> +
>> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
>> + ops = (struct pci_ecam_ops *) of_id->data;
>
> Do we really need dt_match_node and dev->dev.of_match_table to get
> dt_device_match.data?
>
> data is passed as a parameter to pci_host_common_probe, isn't it enough
> to do:
>
> ops = (struct pci_ecam_ops *) data;
As of now not required but in future we might need it if we implement other ecam supported bridge
static const struct dt_device_match gen_pci_dt_match[] = {
{ .compatible = "pci-host-ecam-generic",
.data = &pci_generic_ecam_ops },
{ .compatible = "pci-host-cam-generic",
.data = &gen_pci_cfg_cam_bus_ops },
{ },
};
>
>
>> + bridge = pci_alloc_host_bridge();
>> + if ( !bridge )
>> + return -ENOMEM;
>> +
>> + /* Parse and map our Configuration Space windows */
>> + cfg = gen_pci_init(dev, ops);
>> + if ( !cfg )
>> + {
>> + err = -ENOMEM;
>> + goto err_exit;
>> + }
>> +
>> + bridge->dt_node = dev;
>> + bridge->cfg = cfg;
>> + bridge->sysdata = ops;
>> + bridge->ops = &ops->pci_ops;
>> + bridge->bus_start = cfg->busn_start;
>> + bridge->bus_end = cfg->busn_end;
>> +
>> + bridge->segment = pci_bus_find_domain_nr(dev);
>> + if ( bridge->segment < 0 )
>> + {
>> + printk(XENLOG_ERR "Inconsistent \"linux,pci-domain\" property in DT\n");
>> + BUG();
>> + }
>> + pci_add_host_bridge(bridge);
>> +
>> + return 0;
>> +
>> +err_exit:
>> + xfree(bridge);
>> +
>> + return err;
>> +}
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
>> new file mode 100644
>> index 0000000000..6b3288d6f3
>> --- /dev/null
>> +++ b/xen/arch/arm/pci/pci-host-generic.c
>> @@ -0,0 +1,42 @@
>> +/*
>> + * Based on Linux drivers/pci/controller/pci-host-common.c
>> + * Based on Linux drivers/pci/controller/pci-host-generic.c
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program. If not, see <http://www.gnu.org/licenses/>.
>> + */
>> +
>> +#include <asm/device.h>
>> +#include <xen/pci.h>
>> +#include <asm/pci.h>
>> +
>> +static const struct dt_device_match gen_pci_dt_match[] = {
>> + { .compatible = "pci-host-ecam-generic",
>> + .data = &pci_generic_ecam_ops },
>> +
>> + { },
>> +};
>> +
>> +DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
>> +.dt_match = gen_pci_dt_match,
>> +.init = pci_host_common_probe,
>> +DT_DEVICE_END
>> +
>> +/*
>> + * Local variables:
>> + * mode: C
>> + * c-file-style: "BSD"
>> + * c-basic-offset: 4
>> + * tab-width: 4
>> + * indent-tabs-mode: nil
>> + * End:
>> + */
>> diff --git a/xen/include/asm-arm/device.h b/xen/include/asm-arm/device.h
>> index 5ecd5e7bd1..582119c31e 100644
>> --- a/xen/include/asm-arm/device.h
>> +++ b/xen/include/asm-arm/device.h
>> @@ -16,6 +16,8 @@ struct device
>> enum device_type type;
>> #ifdef CONFIG_HAS_DEVICE_TREE
>> struct dt_device_node *of_node; /* Used by drivers imported from Linux */
>> + /* Used by drivers imported from Linux */
>> + const struct dt_device_match *of_match_table;
>> #endif
>> struct dev_archdata archdata;
>> struct iommu_fwspec *iommu_fwspec; /* per-device IOMMU instance data */
>> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h
>> index f2f86be9bc..4b32c7088a 100644
>> --- a/xen/include/asm-arm/pci.h
>> +++ b/xen/include/asm-arm/pci.h
>> @@ -26,6 +26,65 @@ struct arch_pci_dev {
>> struct device dev;
>> };
>>
>> +/*
>> + * struct to hold the mappings of a config space window. This
>> + * is expected to be used as sysdata for PCI controllers that
>> + * use ECAM.
>> + */
>> +struct pci_config_window {
>> + paddr_t phys_addr;
>> + paddr_t size;
>> + uint8_t busn_start;
>> + uint8_t busn_end;
>> + void __iomem *win;
>> +};
>> +
>> +/*
>> + * struct to hold pci host bridge information
>> + * for a PCI controller.
>> + */
>> +struct pci_host_bridge {
>> + struct dt_device_node *dt_node; /* Pointer to the associated DT node */
>> + struct list_head node; /* Node in list of host bridges */
>> + uint16_t segment; /* Segment number */
>> + uint8_t bus_start; /* Bus start of this bridge. */
>> + uint8_t bus_end; /* Bus end of this bridge. */
>
> bus_start and bus_end are both here and also under pci_config_window.
> Should we remove them from here (if not, then can we removed them from
> struct pci_config_window)?
Yes I will remove this.
>
>
>> + struct pci_config_window* cfg; /* Pointer to the bridge config window */
>> + void *sysdata; /* Pointer to the config space window*/
>> + const struct pci_ops *ops;
>
> It looks like sysdata is unnecessary because we can get the right
> pointer from ops, given that ops is pointing to a member of the struct
> point by sysdata. In other words, if you use container_of(ops, struct
> pci_ecam_ops, pci_ops) it should work?
>
Yes make sense I will remove this.
>
>> +};
>> +
>> +struct pci_ops {
>> + void __iomem *(*map_bus)(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t offset);
>> + int (*read)(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t *value);
>> + int (*write)(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t value);
>> +};
>> +
>> +/*
>> + * struct to hold pci ops and bus shift of the config window
>> + * for a PCI controller.
>> + */
>> +struct pci_ecam_ops {
>> + unsigned int bus_shift;
>> + struct pci_ops pci_ops;
>> + int (*cfg_reg_index)(struct dt_device_node *dev);
>> + int (*init)(struct pci_config_window *);
>
> init is unused, can we get rid of it?
>
>
>> +};
>> +
>> +/* Default ECAM ops */
>> +extern const struct pci_ecam_ops pci_generic_ecam_ops;
>
> If we use container_of and get rid of sysdata, I wonder if we get avoid
> exporting pci_generic_ecam_ops.
>
>
>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data);
>
> This should be static and not exported
>
We required this need to be exported as suggested by Oleksandr.
>
>> +int pci_generic_config_read(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t *value);
>
> also this
>
>
>> +int pci_generic_config_write(struct pci_host_bridge *bridge, uint32_t sbdf,
>> + uint32_t reg, uint32_t len, uint32_t value);
>
> also this
>
>
>> +void __iomem *pci_ecam_map_bus(struct pci_host_bridge *bridge,
>> + uint32_t sbdf, uint32_t where);
>
> also this
>
>> static always_inline bool is_pci_passthrough_enabled(void)
>> {
>> return pci_passthrough_enabled;
>> --
>> 2.17.1
Regards,
Rahul
On Thu, 23 Sep 2021, Rahul Singh wrote:
> >> + goto err_exit;
> >> + }
> >
> > This is unnecessary at the moment, right? Can we get rid of ops->init ?
>
> No this is required for N1SDP board. Please check below patch.
> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
OK
> >> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> >> +{
> >> + struct pci_host_bridge *bridge;
> >> + struct pci_config_window *cfg;
> >> + struct pci_ecam_ops *ops;
> >> + const struct dt_device_match *of_id;
> >> + int err;
> >> +
> >> + if ( dt_device_for_passthrough(dev) )
> >> + return 0;
> >> +
> >> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> >> + ops = (struct pci_ecam_ops *) of_id->data;
> >
> > Do we really need dt_match_node and dev->dev.of_match_table to get
> > dt_device_match.data?
> >
>
> > data is passed as a parameter to pci_host_common_probe, isn't it enough
> > to do:
> >
> > ops = (struct pci_ecam_ops *) data;
>
> As of now not required but in future we might need it if we implement other ecam supported bridge
>
> static const struct dt_device_match gen_pci_dt_match[] = {
> { .compatible = "pci-host-ecam-generic",
> .data = &pci_generic_ecam_ops },
>
> { .compatible = "pci-host-cam-generic",
> .data = &gen_pci_cfg_cam_bus_ops },
>
> { },
> };
Even if we add another ECAM-supported bridge, the following:
ops = (struct pci_ecam_ops *) data;
could still work, right? The probe function will directly receive as
parameter the .data pointer. You shouldn't need the indirection via
dt_match_node?
If you are worried about gen_pci_cfg_cam_bus_ops not being a struct
pci_ecam_ops: that problem can also be solved by making
gen_pci_cfg_cam_bus_ops a struct containinig a struct pci_ecam_ops.
Hi Stefano,
> On 23 Sep 2021, at 8:12 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Thu, 23 Sep 2021, Rahul Singh wrote:
>>>> + goto err_exit;
>>>> + }
>>>
>>> This is unnecessary at the moment, right? Can we get rid of ops->init ?
>>
>> No this is required for N1SDP board. Please check below patch.
>> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
>
> OK
>
>
>>>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
>>>> +{
>>>> + struct pci_host_bridge *bridge;
>>>> + struct pci_config_window *cfg;
>>>> + struct pci_ecam_ops *ops;
>>>> + const struct dt_device_match *of_id;
>>>> + int err;
>>>> +
>>>> + if ( dt_device_for_passthrough(dev) )
>>>> + return 0;
>>>> +
>>>> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
>>>> + ops = (struct pci_ecam_ops *) of_id->data;
>>>
>>> Do we really need dt_match_node and dev->dev.of_match_table to get
>>> dt_device_match.data?
>>>
>>
>>> data is passed as a parameter to pci_host_common_probe, isn't it enough
>>> to do:
>>>
>>> ops = (struct pci_ecam_ops *) data;
>>
>> As of now not required but in future we might need it if we implement other ecam supported bridge
>>
>> static const struct dt_device_match gen_pci_dt_match[] = {
>> { .compatible = "pci-host-ecam-generic",
>> .data = &pci_generic_ecam_ops },
>>
>> { .compatible = "pci-host-cam-generic",
>> .data = &gen_pci_cfg_cam_bus_ops },
>>
>> { },
>> };
>
> Even if we add another ECAM-supported bridge, the following:
>
> ops = (struct pci_ecam_ops *) data;
>
> could still work, right? The probe function will directly receive as
> parameter the .data pointer. You shouldn't need the indirection via
> dt_match_node?
As per my understanding probe function will not get .data pointer.Probe data argument is NULL in most of the cases in XEN
Please have a look once dt_pci_init() -> device_init(..) call flow implementation.
Regards,
Rahul
>
> If you are worried about gen_pci_cfg_cam_bus_ops not being a struct
> pci_ecam_ops: that problem can also be solved by making
> gen_pci_cfg_cam_bus_ops a struct containinig a struct pci_ecam_ops.
On Fri, 24 Sep 2021, Rahul Singh wrote:
> Hi Stefano,
>
> > On 23 Sep 2021, at 8:12 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> >
> > On Thu, 23 Sep 2021, Rahul Singh wrote:
> >>>> + goto err_exit;
> >>>> + }
> >>>
> >>> This is unnecessary at the moment, right? Can we get rid of ops->init ?
> >>
> >> No this is required for N1SDP board. Please check below patch.
> >> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
> >
> > OK
> >
> >
> >>>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> >>>> +{
> >>>> + struct pci_host_bridge *bridge;
> >>>> + struct pci_config_window *cfg;
> >>>> + struct pci_ecam_ops *ops;
> >>>> + const struct dt_device_match *of_id;
> >>>> + int err;
> >>>> +
> >>>> + if ( dt_device_for_passthrough(dev) )
> >>>> + return 0;
> >>>> +
> >>>> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> >>>> + ops = (struct pci_ecam_ops *) of_id->data;
> >>>
> >>> Do we really need dt_match_node and dev->dev.of_match_table to get
> >>> dt_device_match.data?
> >>>
> >>
> >>> data is passed as a parameter to pci_host_common_probe, isn't it enough
> >>> to do:
> >>>
> >>> ops = (struct pci_ecam_ops *) data;
> >>
> >> As of now not required but in future we might need it if we implement other ecam supported bridge
> >>
> >> static const struct dt_device_match gen_pci_dt_match[] = {
> >> { .compatible = "pci-host-ecam-generic",
> >> .data = &pci_generic_ecam_ops },
> >>
> >> { .compatible = "pci-host-cam-generic",
> >> .data = &gen_pci_cfg_cam_bus_ops },
> >>
> >> { },
> >> };
> >
> > Even if we add another ECAM-supported bridge, the following:
> >
> > ops = (struct pci_ecam_ops *) data;
> >
> > could still work, right? The probe function will directly receive as
> > parameter the .data pointer. You shouldn't need the indirection via
> > dt_match_node?
>
> As per my understanding probe function will not get .data pointer.Probe data argument is NULL in most of the cases in XEN
> Please have a look once dt_pci_init() -> device_init(..) call flow implementation.
You are right. Looking at the code, nobody is currently using
dt_device_match.data and it is clear why: it is not passed to the
device_desc.init function at all. As it is today, it is basically
useless.
And there is only one case where device_init has a non-NULL data
parameter and it is in xen/drivers/char/arm-uart.c. All the others are
not even using the data parameter of device_init.
I think we need to change device_init so that dt_device_match.data can
be useful. Sorry for the scope-creep but I think we should do the
following:
- do not add of_match_table to struct device
- add one more parameter to device_desc.init:
int (*init)(struct dt_device_node *dev, struct device_desc *desc, const void *data);
- change device_init to call desc->init with the right parameters:
desc->init(dev, desc, data);
This way pci_host_common_probe is just going to get a desc directly as
parameter. I think it would make a lot more sense from an interface
perspective. It does require a change in all the DT_DEVICE_START.init
functions adding a struct device_desc *desc parameter, but it should be
a mechanical change.
Alternatively we could just change device_init to pass
device_desc.dt_match.data when the data parameter is NULL but it feels
like a hack.
What do you think?
On Fri, 24 Sep 2021, Stefano Stabellini wrote:
> On Fri, 24 Sep 2021, Rahul Singh wrote:
> > Hi Stefano,
> >
> > > On 23 Sep 2021, at 8:12 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
> > >
> > > On Thu, 23 Sep 2021, Rahul Singh wrote:
> > >>>> + goto err_exit;
> > >>>> + }
> > >>>
> > >>> This is unnecessary at the moment, right? Can we get rid of ops->init ?
> > >>
> > >> No this is required for N1SDP board. Please check below patch.
> > >> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
> > >
> > > OK
> > >
> > >
> > >>>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> > >>>> +{
> > >>>> + struct pci_host_bridge *bridge;
> > >>>> + struct pci_config_window *cfg;
> > >>>> + struct pci_ecam_ops *ops;
> > >>>> + const struct dt_device_match *of_id;
> > >>>> + int err;
> > >>>> +
> > >>>> + if ( dt_device_for_passthrough(dev) )
> > >>>> + return 0;
> > >>>> +
> > >>>> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> > >>>> + ops = (struct pci_ecam_ops *) of_id->data;
> > >>>
> > >>> Do we really need dt_match_node and dev->dev.of_match_table to get
> > >>> dt_device_match.data?
> > >>>
> > >>
> > >>> data is passed as a parameter to pci_host_common_probe, isn't it enough
> > >>> to do:
> > >>>
> > >>> ops = (struct pci_ecam_ops *) data;
> > >>
> > >> As of now not required but in future we might need it if we implement other ecam supported bridge
> > >>
> > >> static const struct dt_device_match gen_pci_dt_match[] = {
> > >> { .compatible = "pci-host-ecam-generic",
> > >> .data = &pci_generic_ecam_ops },
> > >>
> > >> { .compatible = "pci-host-cam-generic",
> > >> .data = &gen_pci_cfg_cam_bus_ops },
> > >>
> > >> { },
> > >> };
> > >
> > > Even if we add another ECAM-supported bridge, the following:
> > >
> > > ops = (struct pci_ecam_ops *) data;
> > >
> > > could still work, right? The probe function will directly receive as
> > > parameter the .data pointer. You shouldn't need the indirection via
> > > dt_match_node?
> >
> > As per my understanding probe function will not get .data pointer.Probe data argument is NULL in most of the cases in XEN
> > Please have a look once dt_pci_init() -> device_init(..) call flow implementation.
>
> You are right. Looking at the code, nobody is currently using
> dt_device_match.data and it is clear why: it is not passed to the
> device_desc.init function at all. As it is today, it is basically
> useless.
>
> And there is only one case where device_init has a non-NULL data
> parameter and it is in xen/drivers/char/arm-uart.c. All the others are
> not even using the data parameter of device_init.
>
> I think we need to change device_init so that dt_device_match.data can
> be useful. Sorry for the scope-creep but I think we should do the
> following:
>
> - do not add of_match_table to struct device
>
> - add one more parameter to device_desc.init:
> int (*init)(struct dt_device_node *dev, struct device_desc *desc, const void *data);
>
> - change device_init to call desc->init with the right parameters:
> desc->init(dev, desc, data);
>
> This way pci_host_common_probe is just going to get a desc directly as
> parameter. I think it would make a lot more sense from an interface
> perspective. It does require a change in all the DT_DEVICE_START.init
> functions adding a struct device_desc *desc parameter, but it should be
> a mechanical change.
>
> Alternatively we could just change device_init to pass
> device_desc.dt_match.data when the data parameter is NULL but it feels
> like a hack.
>
>
> What do you think?
Another idea that doesn't require a device_desc.init change and also
doesn't require a change to struct device is the following:
diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
index a88f20175e..1aa0ef4c1e 100644
--- a/xen/arch/arm/pci/pci-host-common.c
+++ b/xen/arch/arm/pci/pci-host-common.c
@@ -205,8 +205,7 @@ int pci_host_common_probe(struct dt_device_node *dev, const void *data)
if ( dt_device_for_passthrough(dev) )
return 0;
- of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
- ops = (struct pci_ecam_ops *) of_id->data;
+ ops = (struct pci_ecam_ops *) data;
bridge = pci_alloc_host_bridge();
if ( !bridge )
diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
index 6b3288d6f3..66fb843f49 100644
--- a/xen/arch/arm/pci/pci-host-generic.c
+++ b/xen/arch/arm/pci/pci-host-generic.c
@@ -20,15 +20,19 @@
#include <asm/pci.h>
static const struct dt_device_match gen_pci_dt_match[] = {
- { .compatible = "pci-host-ecam-generic",
- .data = &pci_generic_ecam_ops },
-
+ { .compatible = "pci-host-ecam-generic" },
{ },
};
+static int pci_host_generic_probe(struct dt_device_node *dev,
+ const void *data)
+{
+ return pci_host_common_probe(dev, &pci_generic_ecam_ops);
+}
+
DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
.dt_match = gen_pci_dt_match,
-.init = pci_host_common_probe,
+.init = pci_host_generic_probe,
DT_DEVICE_END
/*
Hi Stefano,
> On 25 Sep 2021, at 12:26 am, Stefano Stabellini <sstabellini@kernel.org> wrote:
>
> On Fri, 24 Sep 2021, Stefano Stabellini wrote:
>> On Fri, 24 Sep 2021, Rahul Singh wrote:
>>> Hi Stefano,
>>>
>>>> On 23 Sep 2021, at 8:12 pm, Stefano Stabellini <sstabellini@kernel.org> wrote:
>>>>
>>>> On Thu, 23 Sep 2021, Rahul Singh wrote:
>>>>>>> + goto err_exit;
>>>>>>> + }
>>>>>>
>>>>>> This is unnecessary at the moment, right? Can we get rid of ops->init ?
>>>>>
>>>>> No this is required for N1SDP board. Please check below patch.
>>>>> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
>>>>
>>>> OK
>>>>
>>>>
>>>>>>> +int pci_host_common_probe(struct dt_device_node *dev, const void *data)
>>>>>>> +{
>>>>>>> + struct pci_host_bridge *bridge;
>>>>>>> + struct pci_config_window *cfg;
>>>>>>> + struct pci_ecam_ops *ops;
>>>>>>> + const struct dt_device_match *of_id;
>>>>>>> + int err;
>>>>>>> +
>>>>>>> + if ( dt_device_for_passthrough(dev) )
>>>>>>> + return 0;
>>>>>>> +
>>>>>>> + of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
>>>>>>> + ops = (struct pci_ecam_ops *) of_id->data;
>>>>>>
>>>>>> Do we really need dt_match_node and dev->dev.of_match_table to get
>>>>>> dt_device_match.data?
>>>>>>
>>>>>
>>>>>> data is passed as a parameter to pci_host_common_probe, isn't it enough
>>>>>> to do:
>>>>>>
>>>>>> ops = (struct pci_ecam_ops *) data;
>>>>>
>>>>> As of now not required but in future we might need it if we implement other ecam supported bridge
>>>>>
>>>>> static const struct dt_device_match gen_pci_dt_match[] = {
>>>>> { .compatible = "pci-host-ecam-generic",
>>>>> .data = &pci_generic_ecam_ops },
>>>>>
>>>>> { .compatible = "pci-host-cam-generic",
>>>>> .data = &gen_pci_cfg_cam_bus_ops },
>>>>>
>>>>> { },
>>>>> };
>>>>
>>>> Even if we add another ECAM-supported bridge, the following:
>>>>
>>>> ops = (struct pci_ecam_ops *) data;
>>>>
>>>> could still work, right? The probe function will directly receive as
>>>> parameter the .data pointer. You shouldn't need the indirection via
>>>> dt_match_node?
>>>
>>> As per my understanding probe function will not get .data pointer.Probe data argument is NULL in most of the cases in XEN
>>> Please have a look once dt_pci_init() -> device_init(..) call flow implementation.
>>
>> You are right. Looking at the code, nobody is currently using
>> dt_device_match.data and it is clear why: it is not passed to the
>> device_desc.init function at all. As it is today, it is basically
>> useless.
>>
>> And there is only one case where device_init has a non-NULL data
>> parameter and it is in xen/drivers/char/arm-uart.c. All the others are
>> not even using the data parameter of device_init.
>>
>> I think we need to change device_init so that dt_device_match.data can
>> be useful. Sorry for the scope-creep but I think we should do the
>> following:
>>
>> - do not add of_match_table to struct device
>>
>> - add one more parameter to device_desc.init:
>> int (*init)(struct dt_device_node *dev, struct device_desc *desc, const void *data);
>>
>> - change device_init to call desc->init with the right parameters:
>> desc->init(dev, desc, data);
>>
>> This way pci_host_common_probe is just going to get a desc directly as
>> parameter. I think it would make a lot more sense from an interface
>> perspective. It does require a change in all the DT_DEVICE_START.init
>> functions adding a struct device_desc *desc parameter, but it should be
>> a mechanical change.
>>
>> Alternatively we could just change device_init to pass
>> device_desc.dt_match.data when the data parameter is NULL but it feels
>> like a hack.
>>
>>
>> What do you think?
>
>
> Another idea that doesn't require a device_desc.init change and also
> doesn't require a change to struct device is the following:
>
>
> diff --git a/xen/arch/arm/pci/pci-host-common.c b/xen/arch/arm/pci/pci-host-common.c
> index a88f20175e..1aa0ef4c1e 100644
> --- a/xen/arch/arm/pci/pci-host-common.c
> +++ b/xen/arch/arm/pci/pci-host-common.c
> @@ -205,8 +205,7 @@ int pci_host_common_probe(struct dt_device_node *dev, const void *data)
> if ( dt_device_for_passthrough(dev) )
> return 0;
>
> - of_id = dt_match_node(dev->dev.of_match_table, dev->dev.of_node);
> - ops = (struct pci_ecam_ops *) of_id->data;
> + ops = (struct pci_ecam_ops *) data;
>
> bridge = pci_alloc_host_bridge();
> if ( !bridge )
> diff --git a/xen/arch/arm/pci/pci-host-generic.c b/xen/arch/arm/pci/pci-host-generic.c
> index 6b3288d6f3..66fb843f49 100644
> --- a/xen/arch/arm/pci/pci-host-generic.c
> +++ b/xen/arch/arm/pci/pci-host-generic.c
> @@ -20,15 +20,19 @@
> #include <asm/pci.h>
>
> static const struct dt_device_match gen_pci_dt_match[] = {
> - { .compatible = "pci-host-ecam-generic",
> - .data = &pci_generic_ecam_ops },
> -
> + { .compatible = "pci-host-ecam-generic" },
> { },
> };
>
> +static int pci_host_generic_probe(struct dt_device_node *dev,
> + const void *data)
> +{
> + return pci_host_common_probe(dev, &pci_generic_ecam_ops);
> +}
> +
> DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI)
> .dt_match = gen_pci_dt_match,
> -.init = pci_host_common_probe,
> +.init = pci_host_generic_probe,
> DT_DEVICE_END
>
> /*
I also think this is good idea to avoid modification for device_init(.) and struct device{} .
I will use this patch in next version.
Regards,
Rahul
On Fri, 24 Sep 2021, 23:42 Stefano Stabellini, <sstabellini@kernel.org>
wrote:
> On Fri, 24 Sep 2021, Rahul Singh wrote:
> > Hi Stefano,
> >
> > > On 23 Sep 2021, at 8:12 pm, Stefano Stabellini <sstabellini@kernel.org>
> wrote:
> > >
> > > On Thu, 23 Sep 2021, Rahul Singh wrote:
> > >>>> + goto err_exit;
> > >>>> + }
> > >>>
> > >>> This is unnecessary at the moment, right? Can we get rid of
> ops->init ?
> > >>
> > >> No this is required for N1SDP board. Please check below patch.
> > >>
> https://gitlab.com/rahsingh/xen-integration/-/commit/6379ba5764df33d57547087cff4ffc078dc515d5
> > >
> > > OK
> > >
> > >
> > >>>> +int pci_host_common_probe(struct dt_device_node *dev, const void
> *data)
> > >>>> +{
> > >>>> + struct pci_host_bridge *bridge;
> > >>>> + struct pci_config_window *cfg;
> > >>>> + struct pci_ecam_ops *ops;
> > >>>> + const struct dt_device_match *of_id;
> > >>>> + int err;
> > >>>> +
> > >>>> + if ( dt_device_for_passthrough(dev) )
> > >>>> + return 0;
> > >>>> +
> > >>>> + of_id = dt_match_node(dev->dev.of_match_table,
> dev->dev.of_node);
> > >>>> + ops = (struct pci_ecam_ops *) of_id->data;
> > >>>
> > >>> Do we really need dt_match_node and dev->dev.of_match_table to get
> > >>> dt_device_match.data?
> > >>>
> > >>
> > >>> data is passed as a parameter to pci_host_common_probe, isn't it
> enough
> > >>> to do:
> > >>>
> > >>> ops = (struct pci_ecam_ops *) data;
> > >>
> > >> As of now not required but in future we might need it if we implement
> other ecam supported bridge
> > >>
> > >> static const struct dt_device_match gen_pci_dt_match[] = {
>
> > >> { .compatible = "pci-host-ecam-generic",
>
> > >> .data = &pci_generic_ecam_ops },
> > >>
> > >> { .compatible = "pci-host-cam-generic",
> > >> .data = &gen_pci_cfg_cam_bus_ops },
>
> > >>
> > >> { },
>
> > >> };
> > >
> > > Even if we add another ECAM-supported bridge, the following:
> > >
> > > ops = (struct pci_ecam_ops *) data;
> > >
> > > could still work, right? The probe function will directly receive as
> > > parameter the .data pointer. You shouldn't need the indirection via
> > > dt_match_node?
> >
> > As per my understanding probe function will not get .data pointer.Probe
> data argument is NULL in most of the cases in XEN
> > Please have a look once dt_pci_init() -> device_init(..) call flow
> implementation.
>
> You are right. Looking at the code, nobody is currently using
> dt_device_match.data and it is clear why: it is not passed to the
> device_desc.init function at all. As it is today, it is basically
> useless.
>
IIRC it is used by the SMMU driver. But you need to lookup for the desc
manually in each init callback.
If I am not mistaken, this is how Linux is dealing with it as well.
However...
> And there is only one case where device_init has a non-NULL data
> parameter and it is in xen/drivers/char/arm-uart.c. All the others are
> not even using the data parameter of device_init.
> I think we need to change device_init so that dt_device_match.data can
> be useful. Sorry for the scope-creep but I think we should do the
> following:
>
> - do not add of_match_table to struct device
>
> - add one more parameter to device_desc.init:
> int (*init)(struct dt_device_node *dev, struct device_desc *desc, const
> void *data);
>
> - change device_init to call desc->init with the right parameters:
> desc->init(dev, desc, data);
>
> This way pci_host_common_probe is just going to get a desc directly as
> parameter. I think it would make a lot more sense from an interface
> perspective. It does require a change in all the DT_DEVICE_START.init
> functions adding a struct device_desc *desc parameter, but it should be
> a mechanical change.
>
> Alternatively we could just change device_init to pass
> device_desc.dt_match.data when the data parameter is NULL but it feels
> like a hack.
>
>
> What do you think?
... I like the idea of passing desc parameter (we could also simply pass
desc.data in an argument named "priv").
Cheers,
>
© 2016 - 2024 Red Hat, Inc.