Hi, Bjorn, On Wed, Jun 1, 2022 at 7:04 AM Bjorn Helgaas <helgaas@xxxxxxxxxx> wrote: > > On Sat, Apr 30, 2022 at 04:48:42PM +0800, Huacai Chen wrote: > > Loongson PCH (LS7A chipset) will be used by both MIPS-based and > > LoongArch-based Loongson processors. MIPS-based Loongson uses FDT > > while LoongArch-base Loongson uses ACPI, this patch add ACPI init > > support for the driver in drivers/pci/controller/pci-loongson.c > > because it is currently FDT-only. > > > > LoongArch is a new RISC ISA, mainline support will come soon, and > > documentations are here (in translation): > > > > https://github.com/loongson/LoongArch-Documentation > > > > Signed-off-by: Huacai Chen <chenhuacai@xxxxxxxxxxx> > > --- > > drivers/acpi/pci_mcfg.c | 13 ++++++ > > drivers/pci/controller/Kconfig | 2 +- > > drivers/pci/controller/pci-loongson.c | 60 ++++++++++++++++++++++++++- > > include/linux/pci-ecam.h | 1 + > > 4 files changed, 73 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c > > index 53cab975f612..860014b89b8e 100644 > > --- a/drivers/acpi/pci_mcfg.c > > +++ b/drivers/acpi/pci_mcfg.c > > @@ -41,6 +41,8 @@ struct mcfg_fixup { > > static struct mcfg_fixup mcfg_quirks[] = { > > /* { OEM_ID, OEM_TABLE_ID, REV, SEGMENT, BUS_RANGE, ops, cfgres }, */ > > > > +#ifdef CONFIG_ARM64 > > + > > #define AL_ECAM(table_id, rev, seg, ops) \ > > { "AMAZON", table_id, rev, seg, MCFG_BUS_ANY, ops } > > > > @@ -169,6 +171,17 @@ static struct mcfg_fixup mcfg_quirks[] = { > > ALTRA_ECAM_QUIRK(1, 13), > > ALTRA_ECAM_QUIRK(1, 14), > > ALTRA_ECAM_QUIRK(1, 15), > > +#endif /* ARM64 */ > > The addition of the CONFIG_ARM64 #ifdefs should be its own separate > patch so it's not buried in this Loongson one. OK, thanks. > > > +#ifdef CONFIG_LOONGARCH > > +#define LOONGSON_ECAM_MCFG(table_id, seg) \ > > + { "LOONGS", table_id, 1, seg, MCFG_BUS_ANY, &loongson_pci_ecam_ops } > > + > > + LOONGSON_ECAM_MCFG("\0", 0), > > + LOONGSON_ECAM_MCFG("LOONGSON", 0), > > + LOONGSON_ECAM_MCFG("\0", 1), > > + LOONGSON_ECAM_MCFG("LOONGSON", 1), > > +#endif /* LOONGARCH */ > > }; > > > > static char mcfg_oem_id[ACPI_OEM_ID_SIZE]; > > diff --git a/drivers/pci/controller/Kconfig b/drivers/pci/controller/Kconfig > > index b8d96d38064d..9dbd73898b47 100644 > > --- a/drivers/pci/controller/Kconfig > > +++ b/drivers/pci/controller/Kconfig > > @@ -293,7 +293,7 @@ config PCI_HYPERV_INTERFACE > > config PCI_LOONGSON > > bool "LOONGSON PCI Controller" > > depends on MACH_LOONGSON64 || COMPILE_TEST > > - depends on OF > > + depends on OF || ACPI > > depends on PCI_QUIRKS > > default MACH_LOONGSON64 > > help > > diff --git a/drivers/pci/controller/pci-loongson.c b/drivers/pci/controller/pci-loongson.c > > index 0a947ace9478..adbfa4a2330f 100644 > > --- a/drivers/pci/controller/pci-loongson.c > > +++ b/drivers/pci/controller/pci-loongson.c > > @@ -9,6 +9,8 @@ > > #include <linux/of_pci.h> > > #include <linux/pci.h> > > #include <linux/pci_ids.h> > > +#include <linux/pci-acpi.h> > > +#include <linux/pci-ecam.h> > > > > #include "../pci.h" > > > > @@ -97,6 +99,18 @@ static void loongson_mrrs_quirk(struct pci_dev *dev) > > } > > DECLARE_PCI_FIXUP_ENABLE(PCI_ANY_ID, PCI_ANY_ID, loongson_mrrs_quirk); > > > > +static struct loongson_pci *pci_bus_to_loongson_pci(struct pci_bus *bus) > > +{ > > + struct pci_config_window *cfg; > > + > > + if (acpi_disabled) > > + return (struct loongson_pci *)(bus->sysdata); > > + else { > > + cfg = bus->sysdata; > > + return (struct loongson_pci *)(cfg->priv); > > + } > > +} > > + > > static void __iomem *cfg1_map(struct loongson_pci *priv, int bus, > > unsigned int devfn, int where) > > { > > @@ -124,8 +138,10 @@ static void __iomem *pci_loongson_map_bus(struct pci_bus *bus, unsigned int devf > > int where) > > { > > unsigned char busnum = bus->number; > > - struct pci_host_bridge *bridge = pci_find_host_bridge(bus); > > - struct loongson_pci *priv = pci_host_bridge_priv(bridge); > > + struct loongson_pci *priv = pci_bus_to_loongson_pci(bus); > > + > > + if (pci_is_root_bus(bus)) > > + busnum = 0; > > This is weird. The comment just below says "For our hardware the root > bus is always bus 0." Is that not true any more? Why would you > override the bus number? The below comment should be fixed. For multi pci domain machines, the root bus number isn't always 0, so we should override it. > > > /* > > * Do not read more than one device on the bus other than > > @@ -146,6 +162,8 @@ static void __iomem *pci_loongson_map_bus(struct pci_bus *bus, unsigned int devf > > return NULL; > > } > > > > +#ifdef CONFIG_OF > > + > > static int loongson_map_irq(const struct pci_dev *dev, u8 slot, u8 pin) > > { > > int irq; > > @@ -259,3 +277,41 @@ static struct platform_driver loongson_pci_driver = { > > .probe = loongson_pci_probe, > > }; > > builtin_platform_driver(loongson_pci_driver); > > + > > +#endif > > + > > +#ifdef CONFIG_ACPI > > + > > +static int loongson_pci_ecam_init(struct pci_config_window *cfg) > > +{ > > + struct device *dev = cfg->parent; > > + struct loongson_pci *priv; > > + struct loongson_pci_data *data; > > + > > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL); > > + if (!priv) > > + return -ENOMEM; > > + > > + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL); > > + if (!data) > > + return -ENOMEM; > > + > > + cfg->priv = priv; > > + data->flags = FLAG_CFG1; > > + priv->data = data; > > + priv->cfg1_base = cfg->win - (cfg->busr.start << 16); > > + > > + return 0; > > +} > > + > > +const struct pci_ecam_ops loongson_pci_ecam_ops = { > > + .bus_shift = 16, > > I can't remember the details of how this works. The standard PCIe > ECAM has: > > A[11:00] 12 bits of Register number/alignment > A[14:12] 3 bits of Function number > A[19:15] 5 bits of Device number > A[27:20] 8 bits of Bus number > > Doesn't a bus_shift of 16 mean there are only 16 bits available for > the register number, function, and device? So that would limit us to > 8 bits of register number? Do we enforce that somewhere? > > It seems like a limit on "where" should be enforced in > pci_ecam_map_bus() and other .map_bus() functions like > pci_loongson_map_bus(). Otherwise a config read at offset > 0x100 would read config space of the wrong device. > > Krzysztof, do you remember how this works? After a quick glance, it seems pci_ecam_map_bus() should be changed. :) Huacai > > > + .init = loongson_pci_ecam_init, > > + .pci_ops = { > > + .map_bus = pci_loongson_map_bus, > > + .read = pci_generic_config_read, > > + .write = pci_generic_config_write, > > + } > > +};
