Re: [PATCH V13 2/6] PCI: loongson: Add ACPI init support

[Bjorn Helgaas <helgaas@kernel.org>]

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@loongson.cn>
> ---
>  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.

> +#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?

>  	/*
>  	 * 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?

> +	.init	   = loongson_pci_ecam_init,
> +	.pci_ops   = {
> +		.map_bus = pci_loongson_map_bus,
> +		.read	 = pci_generic_config_read,
> +		.write	 = pci_generic_config_write,
> +	}
> +};