| Message ID | 20220702090808.1221300-4-chenhuacai@loongson.cn (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | PCI: Loongson pci improvements and quirks | expand |
On Sat, Jul 02, 2022 at 05:08:04PM +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. > +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); > + } I rewrote this locally as: if (acpi_disabled) return (struct loongson_pci *)(bus->sysdata); cfg = bus->sysdata; return (struct loongson_pci *)(cfg->priv); to avoid the asymmetry of braces/no braces. > @@ -124,12 +138,14 @@ 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; I asked you about this before [1], but I don't understand the answer. Let's say the root bus is 40 and we have this: 40:00.0 Root Port to [bus 41] 41:00.0 NIC When we read the Vendor ID for 40:00.0: pci_loongson_map_bus(bus 40, 00.0, 0) if (pci_is_root_bus(bus)) # true busnum = 0; cfg0_map(priv, 0x00, 00.0, 0); if (bus != 0) # false ... addroff |= (0 << 16) | (0 << 8) | 0; but for 41:00.0: pci_loongson_map_bus(bus 41, 00.0, 0) if (pci_is_root_bus(bus)) # false ... cfg0_map(priv, 0x41, 00.0, 0); if (bus != 0) # true addroff |= BIT(24); addroff |= (0x41 << 16) | (0 << 8) | 0; Maybe the point is that for accesses to the root bus (which are always Type 0 accesses), you never put "bus << 16" into addroff, no matter what the actual root bus number is? If that's the case, I think you should instead make cfg0_map() look like this: cfg0_map(struct pci_bus *bus, ...) { unsigned long addroff = 0x0; if (!pci_is_root_bus(bus)) { addroff |= BIT(24); addroff |= (bus << 16); } addroff |= (devfn << 8) | where; return priv->cfg0_base + addroff; } Then you don't need to do the weird busnum override in pci_loongson_map_bus(), and the root bus checking is in one place (cfg0_map()) instead of being split between pci_loongson_map_bus() and cfg0_map(). Same for cfg1_map(), obviously. [1] https://lore.kernel.org/r/20220531230437.GA793965@bhelgaas
On 2022/7/13 上午11:37, Bjorn Helgaas wrote: > On Sat, Jul 02, 2022 at 05:08:04PM +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. > >> +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); >> + } > > I rewrote this locally as: > > if (acpi_disabled) > return (struct loongson_pci *)(bus->sysdata); > > cfg = bus->sysdata; > return (struct loongson_pci *)(cfg->priv); > > to avoid the asymmetry of braces/no braces. > Agree, I think we can change it as here in next version. >> @@ -124,12 +138,14 @@ 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; > > I asked you about this before [1], but I don't understand the answer. > > Let's say the root bus is 40 and we have this: > > 40:00.0 Root Port to [bus 41] > 41:00.0 NIC > > When we read the Vendor ID for 40:00.0: > > pci_loongson_map_bus(bus 40, 00.0, 0) > if (pci_is_root_bus(bus)) # true > busnum = 0; > cfg0_map(priv, 0x00, 00.0, 0); > if (bus != 0) # false > ... > addroff |= (0 << 16) | (0 << 8) | 0; > > but for 41:00.0: > > pci_loongson_map_bus(bus 41, 00.0, 0) > if (pci_is_root_bus(bus)) # false > ... > cfg0_map(priv, 0x41, 00.0, 0); > if (bus != 0) # true > addroff |= BIT(24); > addroff |= (0x41 << 16) | (0 << 8) | 0; > > Maybe the point is that for accesses to the root bus (which are always > Type 0 accesses), you never put "bus << 16" into addroff, no matter > what the actual root bus number is? > Yes, indeed. > If that's the case, I think you should instead make cfg0_map() look > like this: > > cfg0_map(struct pci_bus *bus, ...) > { > unsigned long addroff = 0x0; > > if (!pci_is_root_bus(bus)) { > addroff |= BIT(24); > addroff |= (bus << 16); > } > addroff |= (devfn << 8) | where; > return priv->cfg0_base + addroff; > } > > Then you don't need to do the weird busnum override in > pci_loongson_map_bus(), and the root bus checking is in one place > (cfg0_map()) instead of being split between pci_loongson_map_bus() and > cfg0_map(). Same for cfg1_map(), obviously. > Thanks very much for your suggestion, that looks more reasonable than before, we'll put pci_is_root_bus in cfg0_map/cfg1_map to check root bus as your code here. > [1] https://lore.kernel.org/r/20220531230437.GA793965@bhelgaas >
diff --git a/drivers/acpi/pci_mcfg.c b/drivers/acpi/pci_mcfg.c index 63b98eae5e75..860014b89b8e 100644 --- a/drivers/acpi/pci_mcfg.c +++ b/drivers/acpi/pci_mcfg.c @@ -172,6 +172,16 @@ static struct mcfg_fixup mcfg_quirks[] = { ALTRA_ECAM_QUIRK(1, 14), ALTRA_ECAM_QUIRK(1, 15), #endif /* ARM64 */ + +#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 565453882ffe..a1222fc15454 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,12 +138,14 @@ 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; /* * Do not read more than one device on the bus other than - * the host bus. For our hardware the root bus is always bus 0. + * the host bus. */ if (priv->data->flags & FLAG_DEV_FIX && !pci_is_root_bus(bus) && PCI_SLOT(devfn) > 0) @@ -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, + .init = loongson_pci_ecam_init, + .pci_ops = { + .map_bus = pci_loongson_map_bus, + .read = pci_generic_config_read, + .write = pci_generic_config_write, + } +}; + +#endif diff --git a/include/linux/pci-ecam.h b/include/linux/pci-ecam.h index adea5a4771cf..6b1301e2498e 100644 --- a/include/linux/pci-ecam.h +++ b/include/linux/pci-ecam.h @@ -87,6 +87,7 @@ extern const struct pci_ecam_ops xgene_v1_pcie_ecam_ops; /* APM X-Gene PCIe v1 * extern const struct pci_ecam_ops xgene_v2_pcie_ecam_ops; /* APM X-Gene PCIe v2.x */ extern const struct pci_ecam_ops al_pcie_ops; /* Amazon Annapurna Labs PCIe */ extern const struct pci_ecam_ops tegra194_pcie_ops; /* Tegra194 PCIe */ +extern const struct pci_ecam_ops loongson_pci_ecam_ops; /* Loongson PCIe */ #endif #if IS_ENABLED(CONFIG_PCI_HOST_COMMON)
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@loongson.cn> --- drivers/acpi/pci_mcfg.c | 10 +++++ drivers/pci/controller/Kconfig | 2 +- drivers/pci/controller/pci-loongson.c | 62 +++++++++++++++++++++++++-- include/linux/pci-ecam.h | 1 + 4 files changed, 71 insertions(+), 4 deletions(-)