On Mon, Jan 30, 2023 at 11:52:07PM +0530, Sunil V L wrote: > When CONFIG_PCI is enabled, ACPI core expects few arch > functions related to PCI. Add those functions so that > ACPI core gets build. These are levraged from arm64. > > Signed-off-by: Sunil V L > --- > arch/riscv/kernel/Makefile | 1 + > arch/riscv/kernel/pci.c | 173 +++++++++++++++++++++++++++++++++++++ > 2 files changed, 174 insertions(+) > create mode 100644 arch/riscv/kernel/pci.c > diff --git a/arch/riscv/kernel/pci.c b/arch/riscv/kernel/pci.c > new file mode 100644 > index 000000000000..3388af3a67a0 > --- /dev/null > +++ b/arch/riscv/kernel/pci.c > @@ -0,0 +1,173 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Code borrowed from ARM64 > + * > + * Copyright (C) 2003 Anton Blanchard , IBM > + * Copyright (C) 2014 ARM Ltd. > + * Copyright (C) 2022-2023 Ventana Micro System Inc. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#ifdef CONFIG_ACPI Quickly checking against ARM64, they do not wrap the read/write functions in this ifdef, so why do we need to do so? > +/* > + * raw_pci_read/write - Platform-specific PCI config space access. > + */ > +int raw_pci_read(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 *val) > +{ > + struct pci_bus *b = pci_find_bus(domain, bus); > + > + if (!b) > + return PCIBIOS_DEVICE_NOT_FOUND; > + return b->ops->read(b, devfn, reg, len, val); A newline before the return would be appreciated by my eyes :) > +} > + > +int raw_pci_write(unsigned int domain, unsigned int bus, > + unsigned int devfn, int reg, int len, u32 val) Also, both read and write functions here appear to have incorrect alignment on the second lines. > +{ > + struct pci_bus *b = pci_find_bus(domain, bus); > + > + if (!b) > + return PCIBIOS_DEVICE_NOT_FOUND; > + return b->ops->write(b, devfn, reg, len, val); > +} > + > + Extra newline here too, looks to be exactly where you deleted the numa stuff from arm64 ;) > +struct acpi_pci_generic_root_info { > + struct acpi_pci_root_info common; > + struct pci_config_window *cfg; /* config space mapping */ > +}; > + > +int acpi_pci_bus_find_domain_nr(struct pci_bus *bus) > +{ > + struct pci_config_window *cfg = bus->sysdata; > + struct acpi_device *adev = to_acpi_device(cfg->parent); > + struct acpi_pci_root *root = acpi_driver_data(adev); > + > + return root->segment; > +} > + > +static int pci_acpi_root_prepare_resources(struct acpi_pci_root_info *ci) Rhetorical question perhaps, but what does "ci" mean? > +{ > + struct resource_entry *entry, *tmp; > + int status; > + > + status = acpi_pci_probe_root_resources(ci); > + resource_list_for_each_entry_safe(entry, tmp, &ci->resources) { > + if (!(entry->res->flags & IORESOURCE_WINDOW)) > + resource_list_destroy_entry(entry); > + } > + return status; Perhaps that extra newline from above could migrate down to the line above the return here. > +} > + > +/* > + * Lookup the bus range for the domain in MCFG, and set up config space > + * mapping. > + */ > +static struct pci_config_window * > +pci_acpi_setup_ecam_mapping(struct acpi_pci_root *root) This all fits on 1 line. > +{ > + struct device *dev = &root->device->dev; > + struct resource *bus_res = &root->secondary; > + u16 seg = root->segment; > + const struct pci_ecam_ops *ecam_ops; > + struct resource cfgres; > + struct acpi_device *adev; > + struct pci_config_window *cfg; > + int ret; > + > + ret = pci_mcfg_lookup(root, &cfgres, &ecam_ops); > + if (ret) { > + dev_err(dev, "%04x:%pR ECAM region not found\n", seg, bus_res); > + return NULL; > + } > + > + adev = acpi_resource_consumer(&cfgres); > + if (adev) > + dev_info(dev, "ECAM area %pR reserved by %s\n", &cfgres, > + dev_name(&adev->dev)); > + else > + dev_warn(dev, FW_BUG "ECAM area %pR not reserved in ACPI namespace\n", > + &cfgres); > + > + cfg = pci_ecam_create(dev, &cfgres, bus_res, ecam_ops); > + if (IS_ERR(cfg)) { > + dev_err(dev, "%04x:%pR error %ld mapping ECAM\n", seg, bus_res, > + PTR_ERR(cfg)); > + return NULL; > + } > + > + return cfg; > +} > + > +/* release_info: free resources allocated by init_info */ The fact that you haven't picked a consistent comment style for this functions really bothers my OCD. Yes, it may be copy-paste from arm64, but since this is "new code" I don't think there's harm in at least *starting* with something that looks cohesive. > +static void pci_acpi_generic_release_info(struct acpi_pci_root_info *ci) > +{ > + struct acpi_pci_generic_root_info *ri; > + > + ri = container_of(ci, struct acpi_pci_generic_root_info, common); > + pci_ecam_free(ri->cfg); > + kfree(ci->ops); > + kfree(ri); > +} > + > + Extra newline here. > +/* Interface called from ACPI code to setup PCI host controller */ > +struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root) > +{ > + struct acpi_pci_generic_root_info *ri; > + struct pci_bus *bus, *child; > + struct acpi_pci_root_ops *root_ops; > + struct pci_host_bridge *host; > + > + ri = kzalloc(sizeof(*ri), GFP_KERNEL); > + if (!ri) > + return NULL; > + > + root_ops = kzalloc(sizeof(*root_ops), GFP_KERNEL); > + if (!root_ops) { > + kfree(ri); > + return NULL; > + } > + > + ri->cfg = pci_acpi_setup_ecam_mapping(root); > + if (!ri->cfg) { > + kfree(ri); > + kfree(root_ops); > + return NULL; > + } > + > + root_ops->release_info = pci_acpi_generic_release_info; > + root_ops->prepare_resources = pci_acpi_root_prepare_resources; > + root_ops->pci_ops = (struct pci_ops *)&ri->cfg->ops->pci_ops; > + bus = acpi_pci_root_create(root, root_ops, &ri->common, ri->cfg); > + if (!bus) > + return NULL; > + > + /* If we must preserve the resource configuration, claim now */ > + host = pci_find_host_bridge(bus); > + if (host->preserve_config) > + pci_bus_claim_resources(bus); > + > + /* > + * Assign whatever was left unassigned. If we didn't claim above, > + * this will reassign everything. > + */ > + pci_assign_unassigned_root_bus_resources(bus); > + > + list_for_each_entry(child, &bus->children, node) > + pcie_bus_configure_settings(child); > + > + return bus; > +} Anyways, this does look to be "leveraged from arm64" as you say and I only had minor nits to comment about... Reviewed-by: Conor Dooley Cheers, Conor.