| Message ID | 1449155999-220955-2-git-send-email-gabriele.paoloni@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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:lorenzo.pieralisi@arm.com] > Sent: 03 December 2015 17:58 > To: Gabriele Paoloni > Cc: bhelgaas@google.com; arnd@arndb.de; will.deacon@arm.com; > catalin.marinas@arm.com; hanjun.guo@linaro.org; Liviu.Dudau@arm.com; > tn@semihalf.com; jiang.liu@linux.intel.com; tglx@linutronix.de; Wangyijing; > linux-pci@vger.kernel.org; linux-arm-kernel@lists.infradead.org; linux- > acpi@vger.kernel.org; linux-kernel@vger.kernel.org; linaro- > acpi@lists.linaro.org; 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:arnd@arndb.de] > Sent: 04 December 2015 13:57 > To: Lorenzo Pieralisi > Cc: linux-arm-kernel@lists.infradead.org; Gabriele Paoloni; linux- > acpi@vger.kernel.org; linux-pci@vger.kernel.org; > catalin.marinas@arm.com; linaro-acpi@lists.linaro.org; > Liviu.Dudau@arm.com; linux-kernel@vger.kernel.org; will.deacon@arm.com; > Wangyijing; Wangzhou (B); hanjun.guo@linaro.org; liudongdong (C); > tn@semihalf.com; bhelgaas@google.com; tglx@linutronix.de; xuwei (O); > Liguozhu (Kenneth); jiang.liu@linux.intel.com > 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:arnd@arndb.de] >> Sent: 04 December 2015 13:57 >> To: Lorenzo Pieralisi >> Cc: linux-arm-kernel@lists.infradead.org; Gabriele Paoloni; linux- >> acpi@vger.kernel.org; linux-pci@vger.kernel.org; >> catalin.marinas@arm.com; linaro-acpi@lists.linaro.org; >> Liviu.Dudau@arm.com; linux-kernel@vger.kernel.org; will.deacon@arm.com; >> Wangyijing; Wangzhou (B); hanjun.guo@linaro.org; liudongdong (C); >> tn@semihalf.com; bhelgaas@google.com; tglx@linutronix.de; xuwei (O); >> Liguozhu (Kenneth); jiang.liu@linux.intel.com >> 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
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 -------------------------------------------------------------------------- */