[Linaro-acpi] [PATCH V1 10/11] pci, acpi: Provide generic way to assign bus domain number.

[Tomasz Nowicki]

Hi Liviu,

On 28.10.2015 12:38, Liviu.Dudau at 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 at 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 at arm.com>

Thanks Liviu!

Regards,
Tomasz