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
