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

[Huacai Chen <chenhuacai@gmail.com>]

Hi, Bjorn,

On Wed, Jun 1, 2022 at 7:04 AM Bjorn Helgaas <helgaas@kernel.org> wrote:
>
> 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.
OK, thanks.

>
> > +#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?
The below comment should be fixed. For multi pci domain machines, the
root bus number isn't always 0, so we should override it.

>
> >       /*
> >        * 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?
After a quick glance, it seems pci_ecam_map_bus() should be changed. :)

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