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
