Re: [PATCH V15 3/7] PCI: loongson: Add ACPI init support

[Bjorn Helgaas <helgaas@kernel.org>]

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