This patchset is based on Nowicki patchset:
[PATCH V1 00/11] MMCONFIG refactoring and ARM64 PCI hostbridge init based on ACPI
This patchset:
1) adds ACPI support for non ECAM PCI Host Bridge Controllers
2) adds ACPI support for Hip05 PCIe Host Bridge Controller
Gabriele Paoloni (1):
PCI/ACPI: Add ACPI support for non ECAM Host Bridge Controllers
Gabriele Paoloni (1):
PCI/ACPI: HiSi: Add ACPI support for HiSi PCIe Host Bridge
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/pci.c | 42 ++++++++
arch/arm64/kernel/pci_acpi_hisi.c | 198 ++++++++++++++++++++++++++++++++++++++
arch/arm64/kernel/pci_quirks.h | 24 +++++
drivers/acpi/pci_root.c | 4 +
include/acpi/acpi_drivers.h | 6 ++
6 files changed, 275 insertions(+)
create mode 100644 arch/arm64/kernel/pci_acpi_hisi.c
create mode 100644 arch/arm64/kernel/pci_quirks.h
--
1.9.1
This patch modifies the ARM64 architecure specific PCI framework to
support Host Bridge specific quirks. these quirks are need for
host bridge controllers that are not fully ECAM compliant.
The quirks array allows each vendor to define his own
acpi_scan_handler where its own pci_ops can be defined
and the global pointer "vendor_specific_ops" should be
set to them accordingly.
Signed-off-by: Gabriele Paoloni <[email protected]>
Signed-off-by: Liudongdong <[email protected]>
---
arch/arm64/kernel/pci.c | 41 +++++++++++++++++++++++++++++++++++++++++
arch/arm64/kernel/pci_quirks.h | 24 ++++++++++++++++++++++++
drivers/acpi/pci_root.c | 4 ++++
include/acpi/acpi_drivers.h | 6 ++++++
4 files changed, 75 insertions(+)
create mode 100644 arch/arm64/kernel/pci_quirks.h
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index 66cc1ae..d60edb4 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -23,6 +23,7 @@
#include <linux/slab.h>
#include <asm/pci-bridge.h>
+#include "pci_quirks.h"
/*
* Called after each bus is probed, but before its children are examined
@@ -230,6 +231,43 @@ static struct acpi_pci_root_ops acpi_pci_root_ops = {
.prepare_resources = pci_acpi_root_prepare_resources,
};
+/*
+ * List of acpi_scan_handlers according to the specific
+ * Host Bridge controllers that are non ECAM compliant
+ */
+static struct acpi_scan_handler *quirks_array[] = {
+ 0
+};
+
+void acpi_arm64_quirks(void)
+{
+ int i = 0;
+
+ while (quirks_array[i]) {
+ acpi_scan_add_handler(quirks_array[i]);
+ i++;
+ }
+
+}
+
+/*
+ * pci_ops specified by vendors that are not
+ * ECAM compliant
+ */
+struct pci_ops *vendor_specific_ops;
+
+/* function to set vendor specific ops */
+void set_quirk_pci_ops(struct pci_ops *quirk_ops)
+{
+ vendor_specific_ops = quirk_ops;
+}
+
+/* function to unset vendor specific ops */
+void unset_quirk_pci_ops(void)
+{
+ vendor_specific_ops = NULL;
+}
+
/* Root bridge scanning */
struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
{
@@ -253,6 +291,9 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
return NULL;
}
+ if (vendor_specific_ops)
+ acpi_pci_root_ops.pci_ops = vendor_specific_ops;
+
bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
/* After the PCI-E bus has been walked and all devices discovered,
diff --git a/arch/arm64/kernel/pci_quirks.h b/arch/arm64/kernel/pci_quirks.h
new file mode 100644
index 0000000..27055ff
--- /dev/null
+++ b/arch/arm64/kernel/pci_quirks.h
@@ -0,0 +1,24 @@
+/*
+ * PCIe host controller declarations for ACPI quirks
+ *
+ * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef PCI_QUIRKS_H
+#define PCI_QUIRKS_H
+
+/* declarations of vendor specific quirks */
+extern struct acpi_scan_handler pci_root_hisi_handler;
+
+/* function to set vendor specific ops */
+void set_quirk_pci_ops(struct pci_ops *quirk_ops);
+
+/* function to unset vendor specific ops */
+void unset_quirk_pci_ops(void);
+
+#endif /*PCI_QUIRKS_H*/
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index e682dc6..ff362bb3d 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -863,5 +863,9 @@ void __init acpi_pci_root_init(void)
return;
pci_acpi_crs_quirks();
+
+ /* Call quirks for non ECAM ARM64 Host Brdge controllers */
+ acpi_arm64_quirks();
+
acpi_scan_add_handler_with_hotplug(&pci_root_handler, "pci_root");
}
diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
index 29c6912..17f4a37 100644
--- a/include/acpi/acpi_drivers.h
+++ b/include/acpi/acpi_drivers.h
@@ -99,6 +99,12 @@ void pci_acpi_crs_quirks(void);
static inline void pci_acpi_crs_quirks(void) { }
#endif
+#ifdef CONFIG_ARM64
+void acpi_arm64_quirks(void);
+#else
+static inline void acpi_arm64_quirks(void) { }
+#endif
+
/* --------------------------------------------------------------------------
Processor
-------------------------------------------------------------------------- */
--
1.9.1
This patch adds ACPI support for HiSilicon PCIe Host Bridge
controller
Signed-off-by: Liudongdong <[email protected]>
Signed-off-by: Gabriele Paoloni <[email protected]>
---
MAINTAINERS | 8 ++
arch/arm64/kernel/Makefile | 1 +
arch/arm64/kernel/pci.c | 1 +
arch/arm64/kernel/pci_acpi_hisi.c | 211 ++++++++++++++++++++++++++++++++++++++
4 files changed, 221 insertions(+)
create mode 100644 arch/arm64/kernel/pci_acpi_hisi.c
diff --git a/MAINTAINERS b/MAINTAINERS
index 5f46784..b84f359 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -7943,6 +7943,14 @@ F: include/linux/pci*
F: arch/x86/pci/
F: arch/x86/kernel/quirks.c
+PCI ACPI DRIVER FOR HISILICON HIP05
+M: Dongdong Liu <[email protected]>
+M: Gabriele Paoloni <[email protected]>
+L: [email protected]
+L: [email protected]
+S: Maintained
+F: arch/arm64/kernel/pci_acpi_hisi.c
+
PCI DRIVER FOR ARM VERSATILE PLATFORM
M: Rob Herring <[email protected]>
L: [email protected]
diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile
index 22dc9bc..1c89d07 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -36,6 +36,7 @@ arm64-obj-$(CONFIG_EFI) += efi.o efi-stub.o efi-entry.o
arm64-obj-$(CONFIG_PCI) += pci.o
arm64-obj-$(CONFIG_ARMV8_DEPRECATED) += armv8_deprecated.o
arm64-obj-$(CONFIG_ACPI) += acpi.o
+arm64-obj-$(CONFIG_ACPI) += pci_acpi_hisi.o
obj-y += $(arm64-obj-y) vdso/
obj-m += $(arm64-obj-m)
diff --git a/arch/arm64/kernel/pci.c b/arch/arm64/kernel/pci.c
index d60edb4..391e743 100644
--- a/arch/arm64/kernel/pci.c
+++ b/arch/arm64/kernel/pci.c
@@ -236,6 +236,7 @@ static struct acpi_pci_root_ops acpi_pci_root_ops = {
* Host Bridge controllers that are non ECAM compliant
*/
static struct acpi_scan_handler *quirks_array[] = {
+ &pci_root_hisi_handler,
0
};
diff --git a/arch/arm64/kernel/pci_acpi_hisi.c b/arch/arm64/kernel/pci_acpi_hisi.c
new file mode 100644
index 0000000..c528604
--- /dev/null
+++ b/arch/arm64/kernel/pci_acpi_hisi.c
@@ -0,0 +1,211 @@
+/*
+ * PCIe host controller driver for HiSilicon Hip05 SoC
+ *
+ * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
+ *
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/types.h>
+#include <linux/pci.h>
+#include <linux/pci-acpi.h>
+#include <linux/acpi.h>
+#include <linux/ecam.h>
+
+#include "pci_quirks.h"
+
+
+#define MAX_PCIE_PORT_NUM 4
+static int acpi_pci_root_hisi_add(struct acpi_device *device,
+ const struct acpi_device_id *not_used);
+
+static void acpi_pci_root_hisi_remove(struct acpi_device *device);
+
+
+static const struct acpi_device_id root_hisi_device_ids[] = {
+ {"HISI0080", 0},
+ {"", 0},
+};
+
+struct acpi_scan_handler pci_root_hisi_handler = {
+ .ids = root_hisi_device_ids,
+ .attach = acpi_pci_root_hisi_add,
+ .detach = acpi_pci_root_hisi_remove,
+};
+
+static void __iomem *rc_base[MAX_PCIE_PORT_NUM];
+
+static void __iomem *
+pci_mcfg_dev_base(struct pci_bus *bus, unsigned int devfn, int offset)
+{
+ struct pci_mmcfg_region *cfg;
+
+ cfg = pci_mmconfig_lookup(pci_domain_nr(bus), bus->number);
+ if (cfg && cfg->virt)
+ return cfg->virt +
+ (PCI_MMCFG_BUS_OFFSET(bus->number) | (devfn << 12)) +
+ offset;
+ return NULL;
+}
+
+
+/* Hip05 PCIe host only supports 32-bit config access */
+static int hisi_pcie_cfg_read(void __iomem *addr, int where, int size,
+ u32 *val)
+{
+ u32 reg;
+ u32 reg_val;
+ void *walker = ®_val;
+
+ walker += (where & 0x3);
+ reg = where & ~0x3;
+ reg_val = readl(addr + reg);
+
+ if (size == 1)
+ *val = *(u8 __force *) walker;
+ else if (size == 2)
+ *val = *(u16 __force *) walker;
+ else if (size != 4)
+ return PCIBIOS_BAD_REGISTER_NUMBER;
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+
+/* Hip05 PCIe host only supports 32-bit config access */
+static int hisi_pcie_cfg_write(void __iomem *addr, int where, int size,
+ u32 val)
+{
+ u32 reg_val;
+ u32 reg;
+ void *walker = ®_val;
+
+ walker += (where & 0x3);
+ reg = where & ~0x3;
+ if (size == 4)
+ writel(val, addr + reg);
+ else if (size == 2) {
+ reg_val = readl(addr + reg);
+ *(u16 __force *) walker = val;
+ writel(reg_val, addr + reg);
+ } else if (size == 1) {
+ reg_val = readl(addr + reg);
+ *(u8 __force *) walker = val;
+ writel(reg_val, addr + reg);
+ } else
+ return PCIBIOS_BAD_REGISTER_NUMBER;
+
+ return PCIBIOS_SUCCESSFUL;
+}
+
+/*
+* hip05 support ECAM to access EP config space
+* but not support RC
+*/
+static int pci_read(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 *value)
+{
+ struct acpi_pci_root *root = bus->sysdata;
+ int ret;
+
+ if (bus->number == root->secondary.start)
+ ret = hisi_pcie_cfg_read(rc_base[root->segment], where, size,
+ value);
+ else
+ ret = pci_generic_config_read(bus, devfn, where, size, value);
+
+ return ret;
+}
+
+static int pci_write(struct pci_bus *bus, unsigned int devfn, int where,
+ int size, u32 value)
+{
+ struct acpi_pci_root *root = bus->sysdata;
+ int ret;
+
+ if (bus->number == root->secondary.start)
+ ret = hisi_pcie_cfg_write(rc_base[root->segment], where, size,
+ value);
+ else
+ ret = pci_generic_config_write(bus, devfn, where, size, value);
+ return ret;
+}
+
+static struct pci_ops pci_root_ops = {
+ .map_bus = pci_mcfg_dev_base,
+ .read = pci_read,
+ .write = pci_write,
+};
+
+
+/*
+* Sample DSDT (PCIe Root bus)
+*
+* Device (RC0)
+*{
+* Name (_HID, "HISI0081")
+* Name (_CID, "HISI0080")
+* Name(_SEG, 0)
+* Name (_DSD, Package () {
+* ToUUID("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
+* Package () {
+* Package () {"rc-base", 0xb0070000}
+* Package () {"rc-size", 0x10000}
+* })
+*}
+*Device (PCI0)
+*{
+* Name (_HID, "PNP0A08") // PCI Express Root Bridge
+* Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
+*.....
+*}
+*
+*use segment to distinguish pcie host controller
+*0-pcie0
+*1-pcie1
+*/
+static int acpi_pci_root_hisi_add(struct acpi_device *device,
+ const struct acpi_device_id *not_used)
+{
+ u32 base;
+ u32 size;
+ int ret;
+ unsigned long long segment;
+ acpi_status status;
+ acpi_handle handle = device->handle;
+
+ ret = fwnode_property_read_u32(&device->fwnode, "rc-base", &base);
+ if (ret) {
+ dev_err(&device->dev, "can't get rc-base\n");
+ return ret;
+ }
+
+ ret = fwnode_property_read_u32(&device->fwnode, "rc-size", &size);
+ if (ret) {
+ dev_err(&device->dev, "can't get rc-size\n");
+ return ret;
+ }
+
+ status = acpi_evaluate_integer(handle, METHOD_NAME__SEG, NULL,
+ &segment);
+ if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) {
+ dev_err(&device->dev, "can't evaluate _SEG\n");
+ return -ENODEV;
+ }
+
+ rc_base[segment] = ioremap(base, size);
+ set_quirk_pci_ops(&pci_root_ops);
+
+ return 0;
+}
+
+static void acpi_pci_root_hisi_remove(struct acpi_device *device)
+{
+ unset_quirk_pci_ops();
+}
--
1.9.1
Hi Gab,
On Thu, Dec 03, 2015 at 11:19:58PM +0800, Gabriele Paoloni wrote:
[...]
> +void acpi_arm64_quirks(void)
> +{
> + int i = 0;
> +
> + while (quirks_array[i]) {
> + acpi_scan_add_handler(quirks_array[i]);
> + i++;
> + }
> +
> +}
This code is not arm64 specific, and this is part of a wider complaint
I have about this patchset and PCI/ACPI code I see in general.
> +
> +/*
> + * pci_ops specified by vendors that are not
> + * ECAM compliant
> + */
> +struct pci_ops *vendor_specific_ops;
> +
> +/* function to set vendor specific ops */
> +void set_quirk_pci_ops(struct pci_ops *quirk_ops)
> +{
> + vendor_specific_ops = quirk_ops;
> +}
> +
> +/* function to unset vendor specific ops */
> +void unset_quirk_pci_ops(void)
> +{
> + vendor_specific_ops = NULL;
> +}
> +
> /* Root bridge scanning */
> struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> {
> @@ -253,6 +291,9 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> return NULL;
> }
>
> + if (vendor_specific_ops)
> + acpi_pci_root_ops.pci_ops = vendor_specific_ops;
You are relying on the scan handlers calling ordering here, which as far
as I know depends on the ACPI tables layout, this is not acceptable,
we need to find a more robust implementation.
Let's first rewind a bit though, to summarize:
1) we need a way to configure a "generic host controller" with host
controller specific config methods (ie ECAM, even though is a PCI
standard it is not standard enough for some designers)
2) we keep the generic "PNP0A03" matching to declare a host bridge and
related resources (?). Maybe we can have an HID matching the
"real" device (eg "HISI0081" and related DSD) and a CID set to "PNP0A03" ?
I do not want to end up with two device objects in the ACPI tables.
I think that all this code belongs in:
drivers/pci/host/pci-host-generic.c
and the quirks scan should be done _within_ the pci_acpi_scan_root()
that should be placed in drivers/pci/host/pci-host-generic.c (ACPI probe
path, to be created) so that, if quirks for config space have to be applied
they are applied there before calling acpi_pci_root_create() so that
ordering is guaranteed.
I will put together a proposal to define the way we specify HID and
related DSD properties for PCI host controllers and send it to
the ACPI working group for review.
Second, I am against merging _any_ ACPI/PCI code for arm64 before we
define a way for the kernel to detect if resources should be reassigned
or just claimed as they are, as set-up by BIOS.
The current approach, that relies on initcalls (and that's horrible) and
that reassigns everything by default has to be overhauled to match
what x86 does, which is sensible to me (tries to claim, and for
resources that fail, it reassigns).
I will give this more thought and send a proposal to the ACPI working
group for review, so that we make this part of the specs before any
PCI/ACPI code for ARM64 gets close to the mainline kernel.
Comments welcome.
Thanks,
Lorenzo
> +
> bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
>
> /* After the PCI-E bus has been walked and all devices discovered,
> diff --git a/arch/arm64/kernel/pci_quirks.h b/arch/arm64/kernel/pci_quirks.h
> new file mode 100644
> index 0000000..27055ff
> --- /dev/null
> +++ b/arch/arm64/kernel/pci_quirks.h
> @@ -0,0 +1,24 @@
> +/*
> + * PCIe host controller declarations for ACPI quirks
> + *
> + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#ifndef PCI_QUIRKS_H
> +#define PCI_QUIRKS_H
> +
> +/* declarations of vendor specific quirks */
> +extern struct acpi_scan_handler pci_root_hisi_handler;
> +
> +/* function to set vendor specific ops */
> +void set_quirk_pci_ops(struct pci_ops *quirk_ops);
> +
> +/* function to unset vendor specific ops */
> +void unset_quirk_pci_ops(void);
> +
> +#endif /*PCI_QUIRKS_H*/
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index e682dc6..ff362bb3d 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -863,5 +863,9 @@ void __init acpi_pci_root_init(void)
> return;
>
> pci_acpi_crs_quirks();
> +
> + /* Call quirks for non ECAM ARM64 Host Brdge controllers */
> + acpi_arm64_quirks();
> +
> acpi_scan_add_handler_with_hotplug(&pci_root_handler, "pci_root");
> }
> diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> index 29c6912..17f4a37 100644
> --- a/include/acpi/acpi_drivers.h
> +++ b/include/acpi/acpi_drivers.h
> @@ -99,6 +99,12 @@ void pci_acpi_crs_quirks(void);
> static inline void pci_acpi_crs_quirks(void) { }
> #endif
>
> +#ifdef CONFIG_ARM64
> +void acpi_arm64_quirks(void);
> +#else
> +static inline void acpi_arm64_quirks(void) { }
> +#endif
> +
> /* --------------------------------------------------------------------------
> Processor
> -------------------------------------------------------------------------- */
> --
> 1.9.1
>
On Thursday 03 December 2015 17:58:26 Lorenzo Pieralisi wrote:
> On Thu, Dec 03, 2015 at 11:19:58PM +0800, Gabriele Paoloni wrote:
> > +void acpi_arm64_quirks(void)
> > +{
> > + int i = 0;
> > +
> > + while (quirks_array[i]) {
> > + acpi_scan_add_handler(quirks_array[i]);
> > + i++;
> > + }
> > +
> > +}
>
> This code is not arm64 specific, and this is part of a wider complaint
> I have about this patchset and PCI/ACPI code I see in general.
Agreed. We certainly should try to reduce the number of architecture
specific hacks in arch/arm64/kernel/pci.c instead of adding to it.
Ideally we will remove that file soon after the existing align_resource,
enabled_device and add_device hacks can be removed.
> > @@ -253,6 +291,9 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> > return NULL;
> > }
> >
> > + if (vendor_specific_ops)
> > + acpi_pci_root_ops.pci_ops = vendor_specific_ops;
>
> You are relying on the scan handlers calling ordering here, which as far
> as I know depends on the ACPI tables layout, this is not acceptable,
> we need to find a more robust implementation.
>
> Let's first rewind a bit though, to summarize:
>
> 1) we need a way to configure a "generic host controller" with host
> controller specific config methods (ie ECAM, even though is a PCI
> standard it is not standard enough for some designers)
That code already exists, see drivers/acpi/pci_*.c
> 2) we keep the generic "PNP0A03" matching to declare a host bridge and
> related resources (?). Maybe we can have an HID matching the
> "real" device (eg "HISI0081" and related DSD) and a CID set to "PNP0A03" ?
> I do not want to end up with two device objects in the ACPI tables.
>
> I think that all this code belongs in:
>
> drivers/pci/host/pci-host-generic.c
I disagree:
> and the quirks scan should be done _within_ the pci_acpi_scan_root()
> that should be placed in drivers/pci/host/pci-host-generic.c (ACPI probe
> path, to be created) so that, if quirks for config space have to be applied
> they are applied there before calling acpi_pci_root_create() so that
> ordering is guaranteed.
pci-host-generic.c is just for standard PCI implementations, and it has
zero code that would be shared with ACPI: Most of the implementation
deals with parsing DT properties, and all that code is entirely differnet
for ACPI and already exists in drivers/acpi. The one thing that could be
shared is the ECAM config space access, but ACPI already needs something
else here because it requires access to the config space at early boot
time, way before we even load that driver, see raw_pci_read/raw_pci_write.
If there are parts missing in drivers/acpi to make it access PCI hosts,
they can be added there, possibly inside "#ifndef CONFIG_X86".
> I will put together a proposal to define the way we specify HID and
> related DSD properties for PCI host controllers and send it to
> the ACPI working group for review.
That also requires a change to SBSA, right? Today, SBSA assumes that
we have a standard PCI host that will work with any hardware independent
PCI implementation in an OS. We either have to give up on SBSA saying
much about how PCI hosts are implemented, or stop assuming that hardware
is SBSA compliant.
> Second, I am against merging _any_ ACPI/PCI code for arm64 before we
> define a way for the kernel to detect if resources should be reassigned
> or just claimed as they are, as set-up by BIOS.
Why would it ever reassign anything that has been set up by the BIOS?
We are still talking about server systems, right?
Arnd
On Thu, Dec 03, 2015 at 09:58:14PM +0100, Arnd Bergmann wrote:
[...]
> > Let's first rewind a bit though, to summarize:
> >
> > 1) we need a way to configure a "generic host controller" with host
> > controller specific config methods (ie ECAM, even though is a PCI
> > standard it is not standard enough for some designers)
>
> That code already exists, see drivers/acpi/pci_*.c
>
> > 2) we keep the generic "PNP0A03" matching to declare a host bridge and
> > related resources (?). Maybe we can have an HID matching the
> > "real" device (eg "HISI0081" and related DSD) and a CID set to "PNP0A03" ?
> > I do not want to end up with two device objects in the ACPI tables.
> >
> > I think that all this code belongs in:
> >
> > drivers/pci/host/pci-host-generic.c
>
> I disagree:
>
> > and the quirks scan should be done _within_ the pci_acpi_scan_root()
> > that should be placed in drivers/pci/host/pci-host-generic.c (ACPI probe
> > path, to be created) so that, if quirks for config space have to be applied
> > they are applied there before calling acpi_pci_root_create() so that
> > ordering is guaranteed.
>
> pci-host-generic.c is just for standard PCI implementations, and it has
> zero code that would be shared with ACPI: Most of the implementation
> deals with parsing DT properties, and all that code is entirely differnet
> for ACPI and already exists in drivers/acpi. The one thing that could be
> shared is the ECAM config space access, but ACPI already needs something
> else here because it requires access to the config space at early boot
> time, way before we even load that driver, see raw_pci_read/raw_pci_write.
Yes, I agree, basically ACPI has already a concept of "host generic"
layer, there is not much point in "merging" it with the pci-host-generic.c
driver. One thing is for certain: nothing in this and Tomasz patchsets is
arm64 specific, and should not live in arch/arm64.
Side note: for the time being raw_pci_read/write will stay empty on
arm64 till someone explains to me what they are used for, we are not
adding them just because they are there for x86, I enquired within
the ACPI spec working group and frankly I do not see a usage for those
on arm64.
> If there are parts missing in drivers/acpi to make it access PCI hosts,
> they can be added there, possibly inside "#ifndef CONFIG_X86".
Agreed.
> > I will put together a proposal to define the way we specify HID and
> > related DSD properties for PCI host controllers and send it to
> > the ACPI working group for review.
>
> That also requires a change to SBSA, right? Today, SBSA assumes that
> we have a standard PCI host that will work with any hardware independent
> PCI implementation in an OS. We either have to give up on SBSA saying
> much about how PCI hosts are implemented, or stop assuming that hardware
> is SBSA compliant.
It is not even a SBSA change, ECAM is a PCIe standard. I am fine with
NAK'ing all code that is not ECAM compliant, problem is, we are dealing
with HW quirks here, it is not something we can fix in FW either.
I do not think SBSA can rule out HW bugs (call them quirks if you wish),
because that's what we are dealing with here, the line between HW bugs
and designs that deliberately deviate from ECAM is thin.
> > Second, I am against merging _any_ ACPI/PCI code for arm64 before we
> > define a way for the kernel to detect if resources should be reassigned
> > or just claimed as they are, as set-up by BIOS.
>
> Why would it ever reassign anything that has been set up by the BIOS?
> We are still talking about server systems, right?
Do not ask me I agree 100% with you here :), but I can bet some systems
currently shipping with ACPI/PCI on ARM (not upstream) tend to be inherited
from DT where resources are _always_ reassigned and if we start claiming
them they till break in a spectacular way, someone has to update that
FW.
Does "booting with ACPI" implies "FW set-up resources - do not reassign" ?
That's an optimistic assumption IMHO. We either need a FW flag, or we just
force resource claiming on ACPI, and reassign the resources that could not
be claimed. We have to do it for ACPI only, on DT due to legacy we can't
do that anymore, we would break the world.
I am quite happy to force resource claiming when booting with ACPI,
since that will force FW developers to toe the line, what I am saying
is that it is not well defined, at all.
I rest my case: I am against merging _any_ ACPI/PCI code before we
define in stone when/if the kernel should reassign resources (answer
can be "never"), as soon as we merge a platform that requires resources
assignment to work we are stuck with it forever (see DT host controllers).
The early postings I reviewed they all reassign resources through initcalls,
for the records.
Thanks,
Lorenzo
Hi Lorenzo, many thanks for your review.
> -----Original Message-----
> From: Lorenzo Pieralisi [mailto:[email protected]]
> Sent: 03 December 2015 17:58
> To: Gabriele Paoloni
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected]; Wangyijing;
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linaro-
> [email protected]; Wangzhou (B); liudongdong (C); xuwei (O); Liguozhu
> (Kenneth)
> Subject: Re: [RFC PATCH 1/2] PCI/ACPI: Add ACPI support for non ECAM Host
> Bridge Controllers
>
> Hi Gab,
>
> On Thu, Dec 03, 2015 at 11:19:58PM +0800, Gabriele Paoloni wrote:
>
> [...]
>
> > +void acpi_arm64_quirks(void)
> > +{
> > + int i = 0;
> > +
> > + while (quirks_array[i]) {
> > + acpi_scan_add_handler(quirks_array[i]);
> > + i++;
> > + }
> > +
> > +}
>
> This code is not arm64 specific, and this is part of a wider complaint
> I have about this patchset and PCI/ACPI code I see in general.
Mmmm Ok, I just followed the fashion used by pci_acpi_crs_quirks()...
so I agree that it is not strictly related with the ARM64 ip but
are quirks for PCI controllers integrated with ARM64 ip...
However if we can spot a better place for the quirks I am happier
>
> > +
> > +/*
> > + * pci_ops specified by vendors that are not
> > + * ECAM compliant
> > + */
> > +struct pci_ops *vendor_specific_ops;
> > +
> > +/* function to set vendor specific ops */
> > +void set_quirk_pci_ops(struct pci_ops *quirk_ops)
> > +{
> > + vendor_specific_ops = quirk_ops;
> > +}
> > +
> > +/* function to unset vendor specific ops */
> > +void unset_quirk_pci_ops(void)
> > +{
> > + vendor_specific_ops = NULL;
> > +}
> > +
> > /* Root bridge scanning */
> > struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root *root)
> > {
> > @@ -253,6 +291,9 @@ struct pci_bus *pci_acpi_scan_root(struct acpi_pci_root
> *root)
> > return NULL;
> > }
> >
> > + if (vendor_specific_ops)
> > + acpi_pci_root_ops.pci_ops = vendor_specific_ops;
>
> You are relying on the scan handlers calling ordering here, which as far
> as I know depends on the ACPI tables layout, this is not acceptable,
Yes you're right
> we need to find a more robust implementation.
Can't we use the "_DEP" object to set a dependency between "Device (PCI0)"
and "Device (RC0)" (I am referring to the ACPI object example in patch 2/2)?
>
> Let's first rewind a bit though, to summarize:
>
> 1) we need a way to configure a "generic host controller" with host
> controller specific config methods (ie ECAM, even though is a PCI
> standard it is not standard enough for some designers)
Yes
> 2) we keep the generic "PNP0A03" matching to declare a host bridge and
> related resources (?). Maybe we can have an HID matching the
> "real" device (eg "HISI0081" and related DSD) and a CID set to "PNP0A03" ?
> I do not want to end up with two device objects in the ACPI tables.
Mmmm I don't see what's wrong with our approach of having
"Device (RC0)" for the specific host bridge HW and "Device (PCI0)"
for the generic one...
>
> I think that all this code belongs in:
>
> drivers/pci/host/pci-host-generic.c
>
> and the quirks scan should be done _within_ the pci_acpi_scan_root()
> that should be placed in drivers/pci/host/pci-host-generic.c (ACPI probe
> path, to be created) so that, if quirks for config space have to be applied
> they are applied there before calling acpi_pci_root_create() so that
> ordering is guaranteed.
>
> I will put together a proposal to define the way we specify HID and
> related DSD properties for PCI host controllers and send it to
> the ACPI working group for review.
Mmmm...what about instead using DMI decode to know which host bridge
controller lives in the HW (and therefore calling the respective quirk)?
So there would be no change to ACPI specs, am I right?
>
> Second, I am against merging _any_ ACPI/PCI code for arm64 before we
> define a way for the kernel to detect if resources should be reassigned
> or just claimed as they are, as set-up by BIOS.
>
> The current approach, that relies on initcalls (and that's horrible) and
> that reassigns everything by default has to be overhauled to match
> what x86 does, which is sensible to me (tries to claim, and for
> resources that fail, it reassigns).
>
> I will give this more thought and send a proposal to the ACPI working
> group for review, so that we make this part of the specs before any
> PCI/ACPI code for ARM64 gets close to the mainline kernel.
>
> Comments welcome.
>
> Thanks,
> Lorenzo
>
> > +
> > bus = acpi_pci_root_create(root, &acpi_pci_root_ops, info, root);
> >
> > /* After the PCI-E bus has been walked and all devices discovered,
> > diff --git a/arch/arm64/kernel/pci_quirks.h b/arch/arm64/kernel/pci_quirks.h
> > new file mode 100644
> > index 0000000..27055ff
> > --- /dev/null
> > +++ b/arch/arm64/kernel/pci_quirks.h
> > @@ -0,0 +1,24 @@
> > +/*
> > + * PCIe host controller declarations for ACPI quirks
> > + *
> > + * Copyright (C) 2015 HiSilicon Co., Ltd. http://www.hisilicon.com
> > + *
> > + *
> > + * This program is free software; you can redistribute it and/or modify
> > + * it under the terms of the GNU General Public License version 2 as
> > + * published by the Free Software Foundation.
> > + */
> > +
> > +#ifndef PCI_QUIRKS_H
> > +#define PCI_QUIRKS_H
> > +
> > +/* declarations of vendor specific quirks */
> > +extern struct acpi_scan_handler pci_root_hisi_handler;
> > +
> > +/* function to set vendor specific ops */
> > +void set_quirk_pci_ops(struct pci_ops *quirk_ops);
> > +
> > +/* function to unset vendor specific ops */
> > +void unset_quirk_pci_ops(void);
> > +
> > +#endif /*PCI_QUIRKS_H*/
> > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> > index e682dc6..ff362bb3d 100644
> > --- a/drivers/acpi/pci_root.c
> > +++ b/drivers/acpi/pci_root.c
> > @@ -863,5 +863,9 @@ void __init acpi_pci_root_init(void)
> > return;
> >
> > pci_acpi_crs_quirks();
> > +
> > + /* Call quirks for non ECAM ARM64 Host Brdge controllers */
> > + acpi_arm64_quirks();
> > +
> > acpi_scan_add_handler_with_hotplug(&pci_root_handler, "pci_root");
> > }
> > diff --git a/include/acpi/acpi_drivers.h b/include/acpi/acpi_drivers.h
> > index 29c6912..17f4a37 100644
> > --- a/include/acpi/acpi_drivers.h
> > +++ b/include/acpi/acpi_drivers.h
> > @@ -99,6 +99,12 @@ void pci_acpi_crs_quirks(void);
> > static inline void pci_acpi_crs_quirks(void) { }
> > #endif
> >
> > +#ifdef CONFIG_ARM64
> > +void acpi_arm64_quirks(void);
> > +#else
> > +static inline void acpi_arm64_quirks(void) { }
> > +#endif
> > +
> > /* ------------------------------------------------------------------------
> --
> > Processor
> > ------------------------------------------------------------------------
> -- */
> > --
> > 1.9.1
> >
On Friday 04 December 2015 12:04:04 Lorenzo Pieralisi wrote:
> On Thu, Dec 03, 2015 at 09:58:14PM +0100, Arnd Bergmann wrote:
> > pci-host-generic.c is just for standard PCI implementations, and it has
> > zero code that would be shared with ACPI: Most of the implementation
> > deals with parsing DT properties, and all that code is entirely differnet
> > for ACPI and already exists in drivers/acpi. The one thing that could be
> > shared is the ECAM config space access, but ACPI already needs something
> > else here because it requires access to the config space at early boot
> > time, way before we even load that driver, see raw_pci_read/raw_pci_write.
>
> Yes, I agree, basically ACPI has already a concept of "host generic"
> layer, there is not much point in "merging" it with the pci-host-generic.c
> driver. One thing is for certain: nothing in this and Tomasz patchsets is
> arm64 specific, and should not live in arch/arm64.
>
> Side note: for the time being raw_pci_read/write will stay empty on
> arm64 till someone explains to me what they are used for, we are not
> adding them just because they are there for x86, I enquired within
> the ACPI spec working group and frankly I do not see a usage for those
> on arm64.
I think this is mainly so AML can poke into PCI config space to
reconfigure things even during early boot, if necessary. You can
have PCI devices that are owned by ACPI and not to be touched by
the OS.
> > > I will put together a proposal to define the way we specify HID and
> > > related DSD properties for PCI host controllers and send it to
> > > the ACPI working group for review.
> >
> > That also requires a change to SBSA, right? Today, SBSA assumes that
> > we have a standard PCI host that will work with any hardware independent
> > PCI implementation in an OS. We either have to give up on SBSA saying
> > much about how PCI hosts are implemented, or stop assuming that hardware
> > is SBSA compliant.
>
> It is not even a SBSA change, ECAM is a PCIe standard. I am fine with
> NAK'ing all code that is not ECAM compliant, problem is, we are dealing
> with HW quirks here, it is not something we can fix in FW either.
>
> I do not think SBSA can rule out HW bugs (call them quirks if you wish),
> because that's what we are dealing with here, the line between HW bugs
> and designs that deliberately deviate from ECAM is thin.
Right, and some are further away from the standard than others.
> > > Second, I am against merging _any_ ACPI/PCI code for arm64 before we
> > > define a way for the kernel to detect if resources should be reassigned
> > > or just claimed as they are, as set-up by BIOS.
> >
> > Why would it ever reassign anything that has been set up by the BIOS?
> > We are still talking about server systems, right?
>
> Do not ask me I agree 100% with you here :), but I can bet some systems
> currently shipping with ACPI/PCI on ARM (not upstream) tend to be inherited
> from DT where resources are _always_ reassigned and if we start claiming
> them they till break in a spectacular way, someone has to update that
> FW.
>
> Does "booting with ACPI" implies "FW set-up resources - do not reassign" ?
I think that should be true on any server regardless of ACPI: if we
have a BIOS, we can expect it to do the job right. The reason we tend
to completely ignore any PCI setup on most embedded systems is that
we don't trust u-boot to do that right (or at all).
> That's an optimistic assumption IMHO. We either need a FW flag, or we just
> force resource claiming on ACPI, and reassign the resources that could not
> be claimed. We have to do it for ACPI only, on DT due to legacy we can't
> do that anymore, we would break the world.
Hmm, but having a flag in the ACPI tables for "BIOS is broken" won't
work if we require the BIOS to set that flag. In that case, we could
just fix the BIOS. ;-)
> I am quite happy to force resource claiming when booting with ACPI,
> since that will force FW developers to toe the line, what I am saying
> is that it is not well defined, at all.
>
> I rest my case: I am against merging _any_ ACPI/PCI code before we
> define in stone when/if the kernel should reassign resources (answer
> can be "never"), as soon as we merge a platform that requires resources
> assignment to work we are stuck with it forever (see DT host controllers).
Fair enough.
Arnd
Hi Lorenzo, Arnd (thanks to you both for looking at this)
> -----Original Message-----
> From: Arnd Bergmann [mailto:[email protected]]
> Sent: 04 December 2015 13:57
> To: Lorenzo Pieralisi
> Cc: [email protected]; Gabriele Paoloni; linux-
> [email protected]; [email protected];
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Wangyijing; Wangzhou (B); [email protected]; liudongdong (C);
> [email protected]; [email protected]; [email protected]; xuwei (O);
> Liguozhu (Kenneth); [email protected]
> Subject: Re: [RFC PATCH 1/2] PCI/ACPI: Add ACPI support for non ECAM
> Host Bridge Controllers
>
> On Friday 04 December 2015 12:04:04 Lorenzo Pieralisi wrote:
> > On Thu, Dec 03, 2015 at 09:58:14PM +0100, Arnd Bergmann wrote:
>
> > > pci-host-generic.c is just for standard PCI implementations, and it
> > > has zero code that would be shared with ACPI: Most of the
> > > implementation deals with parsing DT properties, and all that code
> > > is entirely differnet for ACPI and already exists in drivers/acpi.
> > > The one thing that could be shared is the ECAM config space access,
> > > but ACPI already needs something else here because it requires
> > > access to the config space at early boot time, way before we even
> load that driver, see raw_pci_read/raw_pci_write.
> >
> > Yes, I agree, basically ACPI has already a concept of "host generic"
> > layer, there is not much point in "merging" it with the
> > pci-host-generic.c driver. One thing is for certain: nothing in this
> > and Tomasz patchsets is
> > arm64 specific, and should not live in arch/arm64.
Ok so now I guess Tomasz is aware of this and probably he is reworking
his patchset to move his code into "drivers/acpi/pci_*",
Tomasz can you confirm this?
If this is the case I'll wait for his new patchset to come out and
rebase mine on top of his new one. Otherwise I should directly rework
his patchset but it's pointless if he's already doing it...
> >
> > Side note: for the time being raw_pci_read/write will stay empty on
> > arm64 till someone explains to me what they are used for, we are not
> > adding them just because they are there for x86, I enquired within
> the
> > ACPI spec working group and frankly I do not see a usage for those on
> > arm64.
>
> I think this is mainly so AML can poke into PCI config space to
> reconfigure things even during early boot, if necessary. You can have
> PCI devices that are owned by ACPI and not to be touched by the OS.
>
> > > > I will put together a proposal to define the way we specify HID
> > > > and related DSD properties for PCI host controllers and send it
> to
> > > > the ACPI working group for review.
About this, rather than modifying the ACPI specs, I would consider
using dmi_decode() to know which HW we got underneath...what about?
> > >
> > > That also requires a change to SBSA, right? Today, SBSA assumes
> that
> > > we have a standard PCI host that will work with any hardware
> > > independent PCI implementation in an OS. We either have to give up
> > > on SBSA saying much about how PCI hosts are implemented, or stop
> > > assuming that hardware is SBSA compliant.
> >
> > It is not even a SBSA change, ECAM is a PCIe standard. I am fine with
> > NAK'ing all code that is not ECAM compliant, problem is, we are
> > dealing with HW quirks here, it is not something we can fix in FW
> either.
> >
> > I do not think SBSA can rule out HW bugs (call them quirks if you
> > wish), because that's what we are dealing with here, the line between
> > HW bugs and designs that deliberately deviate from ECAM is thin.
>
> Right, and some are further away from the standard than others.
>
> > > > Second, I am against merging _any_ ACPI/PCI code for arm64 before
> > > > we define a way for the kernel to detect if resources should be
> > > > reassigned or just claimed as they are, as set-up by BIOS.
> > >
> > > Why would it ever reassign anything that has been set up by the
> BIOS?
> > > We are still talking about server systems, right?
> >
> > Do not ask me I agree 100% with you here :), but I can bet some
> > systems currently shipping with ACPI/PCI on ARM (not upstream) tend
> to
> > be inherited from DT where resources are _always_ reassigned and if
> we
> > start claiming them they till break in a spectacular way, someone has
> > to update that FW.
> >
> > Does "booting with ACPI" implies "FW set-up resources - do not
> reassign" ?
>
> I think that should be true on any server regardless of ACPI: if we
> have a BIOS, we can expect it to do the job right. The reason we tend
> to completely ignore any PCI setup on most embedded systems is that we
> don't trust u-boot to do that right (or at all).
>
> > That's an optimistic assumption IMHO. We either need a FW flag, or we
> > just force resource claiming on ACPI, and reassign the resources that
> > could not be claimed. We have to do it for ACPI only, on DT due to
> > legacy we can't do that anymore, we would break the world.
>
> Hmm, but having a flag in the ACPI tables for "BIOS is broken" won't
> work if we require the BIOS to set that flag. In that case, we could
> just fix the BIOS. ;-)
>
> > I am quite happy to force resource claiming when booting with ACPI,
> > since that will force FW developers to toe the line, what I am saying
> > is that it is not well defined, at all.
> >
> > I rest my case: I am against merging _any_ ACPI/PCI code before we
> > define in stone when/if the kernel should reassign resources (answer
> > can be "never"), as soon as we merge a platform that requires
> > resources assignment to work we are stuck with it forever (see DT
> host controllers).
>
> Fair enough.
Ok fine
>
> Arnd
On 12/03/2015 09:19 AM, Gabriele Paoloni wrote:
> This patch modifies the ARM64 architecure specific PCI framework to
> support Host Bridge specific quirks. these quirks are need for
> host bridge controllers that are not fully ECAM compliant.
> The quirks array allows each vendor to define his own
> acpi_scan_handler where its own pci_ops can be defined
> and the global pointer "vendor_specific_ops" should be
> set to them accordingly.
I have a similar set of changes working for a APM based platform. That
platform has the same problem, that the default config space access
methods don't work.
The one comment I have is that I've tried hard to set it up as a generic
quirk system for which the ACPI/PCI subsystem makes the decision about
which hardware quirk is being enabled. But its hard because the platform
in question claims PNP0A08 too, which IMHO is completely wrong if your
not actually compliant. OTOH, I don't want to base it off the DMI data
because I don't want it to be tied to a particular implementation.
Lucky, the host bridge VID/PID _CAN_ be read with the default ECAM
accessor. The only gocha is that the rescan needs to be restarted once
the ops are replaced.
So my question, is does that work for this device as well?
On 12/03/2015 02:58 PM, Arnd Bergmann wrote:
> On Thursday 03 December 2015 17:58:26 Lorenzo Pieralisi wrote:
>> I will put together a proposal to define the way we specify HID and
>> related DSD properties for PCI host controllers and send it to
>> the ACPI working group for review.
>
> That also requires a change to SBSA, right? Today, SBSA assumes that
> we have a standard PCI host that will work with any hardware independent
> PCI implementation in an OS. We either have to give up on SBSA saying
> much about how PCI hosts are implemented, or stop assuming that hardware
> is SBSA compliant.
Which would be standardizing nonstandard hardware. It would surprise me
if that got much traction.
On Friday 04 December 2015 14:46:19 Jeremy Linton wrote:
> On 12/03/2015 02:58 PM, Arnd Bergmann wrote:
> > On Thursday 03 December 2015 17:58:26 Lorenzo Pieralisi wrote:
> >> I will put together a proposal to define the way we specify HID and
> >> related DSD properties for PCI host controllers and send it to
> >> the ACPI working group for review.
> >
> > That also requires a change to SBSA, right? Today, SBSA assumes that
> > we have a standard PCI host that will work with any hardware independent
> > PCI implementation in an OS. We either have to give up on SBSA saying
> > much about how PCI hosts are implemented, or stop assuming that hardware
> > is SBSA compliant.
>
> Which would be standardizing nonstandard hardware. It would surprise me
> if that got much traction.
What do you suggest instead?
Arnd
On 12/04/2015 03:34 PM, Arnd Bergmann wrote:
> On Friday 04 December 2015 14:46:19 Jeremy Linton wrote:
>> On 12/03/2015 02:58 PM, Arnd Bergmann wrote:
>>> On Thursday 03 December 2015 17:58:26 Lorenzo Pieralisi wrote:
>>>> I will put together a proposal to define the way we specify HID and
>>>> related DSD properties for PCI host controllers and send it to
>>>> the ACPI working group for review.
>>>
>>> That also requires a change to SBSA, right? Today, SBSA assumes that
>>> we have a standard PCI host that will work with any hardware independent
>>> PCI implementation in an OS. We either have to give up on SBSA saying
>>> much about how PCI hosts are implemented, or stop assuming that hardware
>>> is SBSA compliant.
>>
>> Which would be standardizing nonstandard hardware. It would surprise me
>> if that got much traction.
>
> What do you suggest instead?
Arnd,
Well... I'm simply trying to say that IMHO involving a standards
committee to get involved with quirky hardware is a little unusual. They
didn't have to get involved for the dozens of pieces of hardware already
quirked by the PCI paths in linux.
So, in the end I think its more a question of finding an acceptable
solution given linux's bus/driver model. In that case I am 100% open to
constructive suggestions that will result in something that will be
accepted into mainline. AKA if someone says "do it this way and I will
take it" then I will go off an make that work. Put another way, I don't
think anyone likes the need for the existing quirking/blacklisting/etc
mechanisms for dealing with "buggy" hardware but that doesn't stop them
from being in the kernel.
For this particular problem, in the case of the APM part I have there
are probably a handful of ways to get it working. Mark Salter posted a
patch last year (based on ACPI OEM id) which could be rebased. That is
where I started recently, but deviated because of complaints on kernel
lists about it. Right now, I've been trying to delay the quirk detection
until after the scan has started so that I can use the root pcie VID/PID
and restart the scan once the correct ops functions have been installed.
Anyway, these two patches (and my unposted one) all have something in
common vs much of the existing quirk infrastructure. We are trying to
add a dynamic registration system so the quirks are isolated to the host
driver rather than hard-coded into the pcie subsystem. I think that is a
good thing. I can model them on the CRS quirks but I'm pretty sure that
is worse.
On 04.12.2015 17:22, Gabriele Paoloni wrote:
> Hi Lorenzo, Arnd (thanks to you both for looking at this)
>
>> -----Original Message-----
>> From: Arnd Bergmann [mailto:[email protected]]
>> Sent: 04 December 2015 13:57
>> To: Lorenzo Pieralisi
>> Cc: [email protected]; Gabriele Paoloni; linux-
>> [email protected]; [email protected];
>> [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> Wangyijing; Wangzhou (B); [email protected]; liudongdong (C);
>> [email protected]; [email protected]; [email protected]; xuwei (O);
>> Liguozhu (Kenneth); [email protected]
>> Subject: Re: [RFC PATCH 1/2] PCI/ACPI: Add ACPI support for non ECAM
>> Host Bridge Controllers
>>
>> On Friday 04 December 2015 12:04:04 Lorenzo Pieralisi wrote:
>>> On Thu, Dec 03, 2015 at 09:58:14PM +0100, Arnd Bergmann wrote:
>>
>>>> pci-host-generic.c is just for standard PCI implementations, and it
>>>> has zero code that would be shared with ACPI: Most of the
>>>> implementation deals with parsing DT properties, and all that code
>>>> is entirely differnet for ACPI and already exists in drivers/acpi.
>>>> The one thing that could be shared is the ECAM config space access,
>>>> but ACPI already needs something else here because it requires
>>>> access to the config space at early boot time, way before we even
>> load that driver, see raw_pci_read/raw_pci_write.
>>>
>>> Yes, I agree, basically ACPI has already a concept of "host generic"
>>> layer, there is not much point in "merging" it with the
>>> pci-host-generic.c driver. One thing is for certain: nothing in this
>>> and Tomasz patchsets is
>>> arm64 specific, and should not live in arch/arm64.
>
> Ok so now I guess Tomasz is aware of this and probably he is reworking
> his patchset to move his code into "drivers/acpi/pci_*",
> Tomasz can you confirm this?
Yes, working on it now and sorry for late response.
Tomasz