| Message ID | 1445963922-22711-11-git-send-email-tn@semihalf.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote: > Architectures which support PCI_DOMAINS_GENERIC (like ARM64) > cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge > initialization since this function needs valid parent device reference > to be able to retrieve domain number (aka segment). > > We can omit that blocker and pass down host bridge device via > pci_create_root_bus parameter and then be able to evaluate _SEG method > being in pci_bus_assign_domain_nr. > > Note that _SEG method is optional, therefore _SEG absence means > that all PCI buses belong to domain 0. > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > --- > drivers/acpi/pci_root.c | 2 +- > drivers/pci/pci.c | 32 +++++++++++++++++++++++++++----- > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 850d7bf..e682dc6 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > > pci_acpi_root_add_resources(info); > pci_add_resource(&info->resources, &root->secondary); > - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, > + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, > sysdata, &info->resources); Not sure this change should be in this patch, I don't see the relation. To put it differently: I think the patch should introduce the retrieval of the domain number from _SEG method and leave the passing of a valid host bridge device to a more appropriate patch. > if (!bus) > goto out_release_info; > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 6a9a111..17d1857 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -25,6 +25,7 @@ > #include <linux/device.h> > #include <linux/pm_runtime.h> > #include <linux/pci_hotplug.h> > +#include <linux/acpi.h> > #include <asm-generic/pci-bridge.h> > #include <asm/setup.h> > #include "pci.h" > @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void) > void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > { > static int use_dt_domains = -1; > - int domain = of_get_pci_domain_nr(parent->of_node); > + int domain; > > /* > * Check DT domain and use_dt_domains values. > @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > * API and update the use_dt_domains value to keep track of method we > * are using to assign domain numbers (use_dt_domains = 0). > * > + * IF ACPI, we expect non-DT method (use_dt_domains == -1) > + * and call _SEG method for corresponding host bridge device. > + * If _SEG method does not exist, following ACPI spec (6.5.6) > + * all PCI buses belong to domain 0. > + * > * All other combinations imply we have a platform that is trying > - * to mix domain numbers obtained from DT and pci_get_new_domain_nr(), > - * which is a recipe for domain mishandling and it is prevented by > - * invalidating the domain value (domain = -1) and printing a > - * corresponding error. > + * to mix domain numbers obtained from DT, ACPI and > + * pci_get_new_domain_nr(), which is a recipe for domain mishandling and > + * it is prevented by invalidating the domain value (domain = -1) and > + * printing a corresponding error. > */ > + > + domain = of_get_pci_domain_nr(parent->of_node); Not sure what you've got here by splitting the original line into two other than an increase in the change count. Otherwise, it looks sensible. Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com> > if (domain >= 0 && use_dt_domains) { > use_dt_domains = 1; > +#ifdef CONFIG_ACPI > + } else if (!acpi_disabled && use_dt_domains == -1) { > + struct acpi_device *acpi_dev = to_acpi_device(parent); > + unsigned long long segment = 0; > + acpi_status status; > + > + status = acpi_evaluate_integer(acpi_dev->handle, > + METHOD_NAME__SEG, NULL, > + &segment); > + if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) > + dev_err(&acpi_dev->dev, "can't evaluate _SEG\n"); > + > + domain = segment; > +#endif > } else if (domain < 0 && use_dt_domains != 1) { > use_dt_domains = 0; > domain = pci_get_new_domain_nr(); > -- > 1.9.1 >
Hi Liviu, On 28.10.2015 12:38, Liviu.Dudau@arm.com wrote: > On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote: >> Architectures which support PCI_DOMAINS_GENERIC (like ARM64) >> cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge >> initialization since this function needs valid parent device reference >> to be able to retrieve domain number (aka segment). >> >> We can omit that blocker and pass down host bridge device via >> pci_create_root_bus parameter and then be able to evaluate _SEG method >> being in pci_bus_assign_domain_nr. >> >> Note that _SEG method is optional, therefore _SEG absence means >> that all PCI buses belong to domain 0. >> >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> --- >> drivers/acpi/pci_root.c | 2 +- >> drivers/pci/pci.c | 32 +++++++++++++++++++++++++++----- >> 2 files changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 850d7bf..e682dc6 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> >> pci_acpi_root_add_resources(info); >> pci_add_resource(&info->resources, &root->secondary); >> - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, >> + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, >> sysdata, &info->resources); > > Not sure this change should be in this patch, I don't see the relation. > > To put it differently: I think the patch should introduce the retrieval of the > domain number from _SEG method and leave the passing of a valid host bridge device > to a more appropriate patch. I wanted to highlight that ACPI kernel using PCI_DOMAINS_GENERIC needs to have both in place: 1. Obtaining domain from _SEG method 2. And host bridge device reference for which we can call _SEG. But you are right, it will be more clear if I split up patch and describe it in separate changelog. > > >> if (!bus) >> goto out_release_info; >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 6a9a111..17d1857 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -25,6 +25,7 @@ >> #include <linux/device.h> >> #include <linux/pm_runtime.h> >> #include <linux/pci_hotplug.h> >> +#include <linux/acpi.h> >> #include <asm-generic/pci-bridge.h> >> #include <asm/setup.h> >> #include "pci.h" >> @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void) >> void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) >> { >> static int use_dt_domains = -1; >> - int domain = of_get_pci_domain_nr(parent->of_node); >> + int domain; >> >> /* >> * Check DT domain and use_dt_domains values. >> @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) >> * API and update the use_dt_domains value to keep track of method we >> * are using to assign domain numbers (use_dt_domains = 0). >> * >> + * IF ACPI, we expect non-DT method (use_dt_domains == -1) >> + * and call _SEG method for corresponding host bridge device. >> + * If _SEG method does not exist, following ACPI spec (6.5.6) >> + * all PCI buses belong to domain 0. >> + * >> * All other combinations imply we have a platform that is trying >> - * to mix domain numbers obtained from DT and pci_get_new_domain_nr(), >> - * which is a recipe for domain mishandling and it is prevented by >> - * invalidating the domain value (domain = -1) and printing a >> - * corresponding error. >> + * to mix domain numbers obtained from DT, ACPI and >> + * pci_get_new_domain_nr(), which is a recipe for domain mishandling and >> + * it is prevented by invalidating the domain value (domain = -1) and >> + * printing a corresponding error. >> */ >> + >> + domain = of_get_pci_domain_nr(parent->of_node); > > Not sure what you've got here by splitting the original line into two other than an increase > in the change count. Yes, it does not make sense to split the original line. I will fix that. > > Otherwise, it looks sensible. > > Reviewed-by: Liviu Dudau <Liviu.Dudau@arm.com> Thanks Liviu! Regards, Tomasz
On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote: > Architectures which support PCI_DOMAINS_GENERIC (like ARM64) > cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge > initialization since this function needs valid parent device reference > to be able to retrieve domain number (aka segment). > > We can omit that blocker and pass down host bridge device via > pci_create_root_bus parameter and then be able to evaluate _SEG method > being in pci_bus_assign_domain_nr. > > Note that _SEG method is optional, therefore _SEG absence means > that all PCI buses belong to domain 0. > > Signed-off-by: Tomasz Nowicki <tn@semihalf.com> > --- > drivers/acpi/pci_root.c | 2 +- > drivers/pci/pci.c | 32 +++++++++++++++++++++++++++----- > 2 files changed, 28 insertions(+), 6 deletions(-) > > diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c > index 850d7bf..e682dc6 100644 > --- a/drivers/acpi/pci_root.c > +++ b/drivers/acpi/pci_root.c > @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, > > pci_acpi_root_add_resources(info); > pci_add_resource(&info->resources, &root->secondary); > - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, > + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, > sysdata, &info->resources); If I read x86 code correctly, they rely on the first argument to be NULL, I think you would break x86 by doing that, see: arch/x86/pci/acpi.c (pcibios_root_bridge_prepare()) By the way, can't we move the code setting up the ACPI_COMPANION to core PCI code and stop relying on sysdata for that ? Thanks, Lorenzo > if (!bus) > goto out_release_info; > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 6a9a111..17d1857 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -25,6 +25,7 @@ > #include <linux/device.h> > #include <linux/pm_runtime.h> > #include <linux/pci_hotplug.h> > +#include <linux/acpi.h> > #include <asm-generic/pci-bridge.h> > #include <asm/setup.h> > #include "pci.h" > @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void) > void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > { > static int use_dt_domains = -1; > - int domain = of_get_pci_domain_nr(parent->of_node); > + int domain; > > /* > * Check DT domain and use_dt_domains values. > @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) > * API and update the use_dt_domains value to keep track of method we > * are using to assign domain numbers (use_dt_domains = 0). > * > + * IF ACPI, we expect non-DT method (use_dt_domains == -1) > + * and call _SEG method for corresponding host bridge device. > + * If _SEG method does not exist, following ACPI spec (6.5.6) > + * all PCI buses belong to domain 0. > + * > * All other combinations imply we have a platform that is trying > - * to mix domain numbers obtained from DT and pci_get_new_domain_nr(), > - * which is a recipe for domain mishandling and it is prevented by > - * invalidating the domain value (domain = -1) and printing a > - * corresponding error. > + * to mix domain numbers obtained from DT, ACPI and > + * pci_get_new_domain_nr(), which is a recipe for domain mishandling and > + * it is prevented by invalidating the domain value (domain = -1) and > + * printing a corresponding error. > */ > + > + domain = of_get_pci_domain_nr(parent->of_node); > if (domain >= 0 && use_dt_domains) { > use_dt_domains = 1; > +#ifdef CONFIG_ACPI > + } else if (!acpi_disabled && use_dt_domains == -1) { > + struct acpi_device *acpi_dev = to_acpi_device(parent); > + unsigned long long segment = 0; > + acpi_status status; > + > + status = acpi_evaluate_integer(acpi_dev->handle, > + METHOD_NAME__SEG, NULL, > + &segment); > + if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) > + dev_err(&acpi_dev->dev, "can't evaluate _SEG\n"); > + > + domain = segment; > +#endif > } else if (domain < 0 && use_dt_domains != 1) { > use_dt_domains = 0; > domain = pci_get_new_domain_nr(); > -- > 1.9.1 >
On 03.11.2015 17:10, Lorenzo Pieralisi wrote: > On Tue, Oct 27, 2015 at 05:38:41PM +0100, Tomasz Nowicki wrote: >> Architectures which support PCI_DOMAINS_GENERIC (like ARM64) >> cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge >> initialization since this function needs valid parent device reference >> to be able to retrieve domain number (aka segment). >> >> We can omit that blocker and pass down host bridge device via >> pci_create_root_bus parameter and then be able to evaluate _SEG method >> being in pci_bus_assign_domain_nr. >> >> Note that _SEG method is optional, therefore _SEG absence means >> that all PCI buses belong to domain 0. >> >> Signed-off-by: Tomasz Nowicki <tn@semihalf.com> >> --- >> drivers/acpi/pci_root.c | 2 +- >> drivers/pci/pci.c | 32 +++++++++++++++++++++++++++----- >> 2 files changed, 28 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c >> index 850d7bf..e682dc6 100644 >> --- a/drivers/acpi/pci_root.c >> +++ b/drivers/acpi/pci_root.c >> @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, >> >> pci_acpi_root_add_resources(info); >> pci_add_resource(&info->resources, &root->secondary); >> - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, >> + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, >> sysdata, &info->resources); > > If I read x86 code correctly, they rely on the first argument to be > NULL, I think you would break x86 by doing that, see: > > arch/x86/pci/acpi.c (pcibios_root_bridge_prepare()) > > By the way, can't we move the code setting up the ACPI_COMPANION > to core PCI code and stop relying on sysdata for that ? > Yes, that will break x86&ia64 but as you said it may be overcome with consolidation of ACPI_COMPANION into the core code. Tomasz
diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c index 850d7bf..e682dc6 100644 --- a/drivers/acpi/pci_root.c +++ b/drivers/acpi/pci_root.c @@ -839,7 +839,7 @@ struct pci_bus *acpi_pci_root_create(struct acpi_pci_root *root, pci_acpi_root_add_resources(info); pci_add_resource(&info->resources, &root->secondary); - bus = pci_create_root_bus(NULL, busnum, ops->pci_ops, + bus = pci_create_root_bus(&device->dev, busnum, ops->pci_ops, sysdata, &info->resources); if (!bus) goto out_release_info; diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 6a9a111..17d1857 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -25,6 +25,7 @@ #include <linux/device.h> #include <linux/pm_runtime.h> #include <linux/pci_hotplug.h> +#include <linux/acpi.h> #include <asm-generic/pci-bridge.h> #include <asm/setup.h> #include "pci.h" @@ -4501,7 +4502,7 @@ int pci_get_new_domain_nr(void) void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) { static int use_dt_domains = -1; - int domain = of_get_pci_domain_nr(parent->of_node); + int domain; /* * Check DT domain and use_dt_domains values. @@ -4523,14 +4524,35 @@ void pci_bus_assign_domain_nr(struct pci_bus *bus, struct device *parent) * API and update the use_dt_domains value to keep track of method we * are using to assign domain numbers (use_dt_domains = 0). * + * IF ACPI, we expect non-DT method (use_dt_domains == -1) + * and call _SEG method for corresponding host bridge device. + * If _SEG method does not exist, following ACPI spec (6.5.6) + * all PCI buses belong to domain 0. + * * All other combinations imply we have a platform that is trying - * to mix domain numbers obtained from DT and pci_get_new_domain_nr(), - * which is a recipe for domain mishandling and it is prevented by - * invalidating the domain value (domain = -1) and printing a - * corresponding error. + * to mix domain numbers obtained from DT, ACPI and + * pci_get_new_domain_nr(), which is a recipe for domain mishandling and + * it is prevented by invalidating the domain value (domain = -1) and + * printing a corresponding error. */ + + domain = of_get_pci_domain_nr(parent->of_node); if (domain >= 0 && use_dt_domains) { use_dt_domains = 1; +#ifdef CONFIG_ACPI + } else if (!acpi_disabled && use_dt_domains == -1) { + struct acpi_device *acpi_dev = to_acpi_device(parent); + unsigned long long segment = 0; + acpi_status status; + + status = acpi_evaluate_integer(acpi_dev->handle, + METHOD_NAME__SEG, NULL, + &segment); + if (ACPI_FAILURE(status) && status != AE_NOT_FOUND) + dev_err(&acpi_dev->dev, "can't evaluate _SEG\n"); + + domain = segment; +#endif } else if (domain < 0 && use_dt_domains != 1) { use_dt_domains = 0; domain = pci_get_new_domain_nr();
Architectures which support PCI_DOMAINS_GENERIC (like ARM64) cannot call pci_bus_assign_domain_nr along ACPI PCI host bridge initialization since this function needs valid parent device reference to be able to retrieve domain number (aka segment). We can omit that blocker and pass down host bridge device via pci_create_root_bus parameter and then be able to evaluate _SEG method being in pci_bus_assign_domain_nr. Note that _SEG method is optional, therefore _SEG absence means that all PCI buses belong to domain 0. Signed-off-by: Tomasz Nowicki <tn@semihalf.com> --- drivers/acpi/pci_root.c | 2 +- drivers/pci/pci.c | 32 +++++++++++++++++++++++++++----- 2 files changed, 28 insertions(+), 6 deletions(-)