[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH v1 06/14] xen/arm: Add support for PCI ecam operations
Hi Stefano, > On 10 Sep 2021, at 12:21 am, Stefano Stabellini <sstabellini@xxxxxxxxxx> > wrote: > > On Thu, 19 Aug 2021, Rahul Singh wrote: >> Add support for PCI ecam operations to access the PCI >> configuration space. >> >> Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx> >> --- >> xen/arch/arm/pci/Makefile | 1 + >> xen/arch/arm/pci/ecam.c | 63 +++++++++++++++++++++++++++++ >> xen/arch/arm/pci/pci-access.c | 53 ++++++++++++++++++++++++ >> xen/arch/arm/pci/pci-host-common.c | 13 +++++- >> xen/arch/arm/pci/pci-host-generic.c | 8 +++- >> xen/include/asm-arm/pci.h | 32 +++++++++++++++ >> 6 files changed, 167 insertions(+), 3 deletions(-) >> create mode 100644 xen/arch/arm/pci/ecam.c >> >> diff --git a/xen/arch/arm/pci/Makefile b/xen/arch/arm/pci/Makefile >> index f3d97f859e..6f32fbbe67 100644 >> --- a/xen/arch/arm/pci/Makefile >> +++ b/xen/arch/arm/pci/Makefile >> @@ -2,3 +2,4 @@ obj-y += pci.o >> obj-y += pci-access.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..91c691b41f >> --- /dev/null >> +++ b/xen/arch/arm/pci/ecam.c >> @@ -0,0 +1,63 @@ >> +/* >> + * Copyright (C) 2021 Arm Ltd. >> + * >> + * Based on Linux drivers/pci/ecam.c >> + * Copyright 2016 Broadcom >> + * >> + * 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) > > Code style: alignment Ack. > > >> +{ >> + const struct pci_config_window *cfg = bridge->sysdata; >> + unsigned int devfn_shift = cfg->ops->bus_shift - 8; > > Is it a guarantee that devfn_shift == bus_shift - 8, or is it just so > for ECAM? Yes it is guarantee that "devfn_shift == bus_shift - 8” as bus number will be 8 bit always. bus_shift can be different based on vendor. https://elixir.bootlin.com/linux/latest/source/drivers/pci/ecam.c#L172 https://elixir.bootlin.com/linux/latest/source/drivers/pci/controller/pci-thunder-pem.c#L30 > > >> + void __iomem *base; >> + >> + pci_sbdf_t sbdf_t = (pci_sbdf_t) sbdf ; >> + unsigned int busn = sbdf_t.bus; >> + >> + if ( busn < cfg->busn_start || busn > cfg->busn_end ) > > Genuine question: should it be busn >= cfg->busn_end ? I don't know if > the range includes busn_end or not. cfg->bus_end includes the valid last bus number. "busn > cfg->busn_end" is valid check to confirm if we are not accessing pci device outside the valid bus number. > > >> + return NULL; >> + >> + busn -= cfg->busn_start; >> + base = cfg->win + (busn << cfg->ops->bus_shift); >> + >> + return base + (PCI_DEVFN(sbdf_t.dev, sbdf_t.fn) << devfn_shift) + where; >> +} > > I understand that the arm32 part is not implemented and not part of this > series, that's fine. However if the plan is that arm32 will dynamically > map each bus individually, then I imagine this function will have an > ioremap in the arm32 version. Which means that we also need an > unmap_bus call in struct pci_ops. I understand that pci_ecam_unmap_bus > would be a NOP today for arm64, but I think it makes sense to have it if > we want the API to be generic. As per my understanding we don’t need pci_ecam_unmap_bus(..) as I don’t see any use case to unmap the bus dynamically. We can add the support for per_bus_mapping for ARM32 once we implement arm32 part. Let me know your view on this. > > >> +/* 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 >> index b938047c03..f39f6a3a38 100644 >> --- a/xen/arch/arm/pci/pci-access.c >> +++ b/xen/arch/arm/pci/pci-access.c >> @@ -15,6 +15,59 @@ >> */ >> >> #include <xen/pci.h> >> +#include <asm/io.h> >> + >> +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 = ~0; > > Is this a standard error? If so, I think we should define it with a > macro (e.g. INVALID_PADDR). Ok. > > >> + return -ENODEV; >> + } >> + >> + switch (len) >> + { >> + case 1: >> + *value = readb(addr); >> + break; >> + case 2: >> + *value = readw(addr); >> + break; >> + case 4: >> + *value = readl(addr); >> + break; >> + default: >> + BUG(); > > A BUG here is harsh because it could be potentially guest-triggered. An > ASSERT would be better. Ok. > > >> + } >> + >> + 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: >> + BUG(); > > Same here Ok. > > >> + } >> + >> + return 0; >> +} >> >> static uint32_t pci_config_read(pci_sbdf_t sbdf, unsigned int reg, >> unsigned int len) >> diff --git a/xen/arch/arm/pci/pci-host-common.c >> b/xen/arch/arm/pci/pci-host-common.c >> index 9dd9b02271..c582527e92 100644 >> --- a/xen/arch/arm/pci/pci-host-common.c >> +++ b/xen/arch/arm/pci/pci-host-common.c >> @@ -68,6 +68,7 @@ static void pci_ecam_free(struct pci_config_window *cfg) >> } >> >> static struct pci_config_window *gen_pci_init(struct dt_device_node *dev, >> + const struct pci_ecam_ops >> *ops, >> int ecam_reg_idx) >> { >> int err; >> @@ -96,6 +97,7 @@ static struct pci_config_window *gen_pci_init(struct >> dt_device_node *dev, >> >> cfg->phys_addr = addr; >> cfg->size = size; >> + cfg->ops = ops; >> >> /* >> * On 64-bit systems, we do a single ioremap for the whole config space >> @@ -111,6 +113,13 @@ static struct pci_config_window *gen_pci_init(struct >> dt_device_node *dev, >> printk("ECAM at [mem %lx-%lx] 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: >> @@ -216,6 +225,7 @@ static int pci_bus_find_domain_nr(struct dt_device_node >> *dev) >> } >> >> int pci_host_common_probe(struct dt_device_node *dev, >> + const struct pci_ecam_ops *ops, >> int ecam_reg_idx) >> { >> struct pci_host_bridge *bridge; >> @@ -227,7 +237,7 @@ int pci_host_common_probe(struct dt_device_node *dev, >> return -ENOMEM; >> >> /* Parse and map our Configuration Space windows */ >> - cfg = gen_pci_init(dev, ecam_reg_idx); >> + cfg = gen_pci_init(dev, ops, ecam_reg_idx); >> if ( !cfg ) >> { >> err = -ENOMEM; >> @@ -236,6 +246,7 @@ int pci_host_common_probe(struct dt_device_node *dev, >> >> bridge->dt_node = dev; >> bridge->sysdata = cfg; >> + bridge->ops = &ops->pci_ops; >> bridge->bus_start = cfg->busn_start; >> bridge->bus_end = cfg->busn_end; >> >> diff --git a/xen/arch/arm/pci/pci-host-generic.c >> b/xen/arch/arm/pci/pci-host-generic.c >> index 13d0f7f999..2d652e8910 100644 >> --- a/xen/arch/arm/pci/pci-host-generic.c >> +++ b/xen/arch/arm/pci/pci-host-generic.c >> @@ -23,20 +23,24 @@ >> #include <asm/pci.h> >> >> static const struct dt_device_match gen_pci_dt_match[] = { >> - { .compatible = "pci-host-ecam-generic" }, >> + { .compatible = "pci-host-ecam-generic", >> + .data = &pci_generic_ecam_ops }, >> + >> { }, >> }; >> >> static int gen_pci_dt_init(struct dt_device_node *dev, const void *data) >> { >> const struct dt_device_match *of_id; >> + const struct pci_ecam_ops *ops; >> >> of_id = dt_match_node(gen_pci_dt_match, dev->dev.of_node); >> + ops = (struct pci_ecam_ops *) of_id->data; >> >> printk(XENLOG_INFO "Found PCI host bridge %s compatible:%s \n", >> dt_node_full_name(dev), of_id->compatible); >> >> - return pci_host_common_probe(dev, 0); >> + return pci_host_common_probe(dev, ops, 0); >> } >> >> DT_DEVICE_START(pci_gen, "PCI HOST GENERIC", DEVICE_PCI) >> diff --git a/xen/include/asm-arm/pci.h b/xen/include/asm-arm/pci.h >> index 58a51e724e..22866244d2 100644 >> --- a/xen/include/asm-arm/pci.h >> +++ b/xen/include/asm-arm/pci.h >> @@ -37,6 +37,7 @@ struct pci_config_window { >> uint8_t busn_start; >> uint8_t busn_end; >> void __iomem *win; >> + const struct pci_ecam_ops *ops; >> }; >> >> /* >> @@ -50,10 +51,41 @@ struct pci_host_bridge { >> u8 bus_start; /* Bus start of this bridge. */ >> u8 bus_end; /* Bus end of this bridge. */ >> 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 (*init)(struct pci_config_window *); > > Is this last member of the struct needed/used? Yes It will be used by some vendor specific board( N1SDP ). Please check below. https://git.linaro.org/landing-teams/working/arm/n1sdp-pcie-quirk.git/commit/?id=04b7e76d0fe6481a803f58e54e008a1489d713a5 Regards, Rahul
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |