diff mbox

[V4,11/23] pci, acpi: Move ACPI host bridge device companion assignment to core code.

Message ID 1454606941-9523-12-git-send-email-tn@semihalf.com (mailing list archive)
State New, archived
Headers show

Commit Message

Tomasz Nowicki Feb. 4, 2016, 5:28 p.m. UTC 
Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
bridge initialization. They both use sysdata to pass down parent
device reference and both rely on NULL parent in pci_create_root_bus()
to validate sysdata content.

It looks hacky and prevents us from getting some firmware specific
info for PCI host controller e.g. PCI bus domain number.
However, we overcome that blocker by passing down parent
device via pci_create_root_bus parameter (as the ACPI device type)
and using ACPI_COMPANION_SET in core code for ACPI boot method.
ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.

Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
Tested-by: Duc Dang <dhdang@apm.com>
Tested-by: Dongdong Liu <liudongdong3@huawei.com>
Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
Tested-by: Sinan Kaya <okaya@codeaurora.org>
---
 drivers/acpi/pci_root.c | 5 ++++-
 drivers/pci/probe.c     | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Lorenzo Pieralisi Feb. 9, 2016, 6:02 p.m. UTC | #1 
On Thu, Feb 04, 2016 at 06:28:49PM +0100, Tomasz Nowicki wrote:
> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
> bridge initialization. They both use sysdata to pass down parent
> device reference and both rely on NULL parent in pci_create_root_bus()
> to validate sysdata content.
> 
> It looks hacky and prevents us from getting some firmware specific
> info for PCI host controller e.g. PCI bus domain number.
> However, we overcome that blocker by passing down parent
> device via pci_create_root_bus parameter (as the ACPI device type)
> and using ACPI_COMPANION_SET in core code for ACPI boot method.
> ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.
> 
> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
> Tested-by: Duc Dang <dhdang@apm.com>
> Tested-by: Dongdong Liu <liudongdong3@huawei.com>
> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
> Tested-by: Sinan Kaya <okaya@codeaurora.org>
> ---
>  drivers/acpi/pci_root.c | 5 ++++-
>  drivers/pci/probe.c     | 2 ++
>  2 files changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index ae3fe4e..a65c8c2 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -846,7 +846,10 @@ 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,
> +
> +	/* Root bridge device needs to be sure of parent ACPI type */

You can improve the comment.

"pci_create_root_bus() needs to detect the parent device type,
so initialize its companion data accordingly".

> +	ACPI_COMPANION_SET(&device->dev, device);
> +	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/probe.c b/drivers/pci/probe.c
> index 6d7ab9b..81dd3a2 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2100,6 +2100,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>  	bridge->dev.parent = parent;
>  	bridge->dev.release = pci_release_host_bridge_dev;
>  	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
> +	ACPI_COMPANION_SET(&bridge->dev,
> +			   parent ? to_acpi_device_node(parent->fwnode) : NULL);
if (parent)
	ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent));

?

It seems fine to me, hopefully Rafael can have a look to countercheck.

Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>

>  	error = pcibios_root_bridge_prepare(bridge);
>  	if (error) {
>  		kfree(bridge);
> -- 
> 1.9.1
>
Tomasz Nowicki Feb. 10, 2016, 9:09 p.m. UTC | #2 
On 02/09/2016 07:02 PM, Lorenzo Pieralisi wrote:
> On Thu, Feb 04, 2016 at 06:28:49PM +0100, Tomasz Nowicki wrote:
>> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
>> bridge initialization. They both use sysdata to pass down parent
>> device reference and both rely on NULL parent in pci_create_root_bus()
>> to validate sysdata content.
>>
>> It looks hacky and prevents us from getting some firmware specific
>> info for PCI host controller e.g. PCI bus domain number.
>> However, we overcome that blocker by passing down parent
>> device via pci_create_root_bus parameter (as the ACPI device type)
>> and using ACPI_COMPANION_SET in core code for ACPI boot method.
>> ACPI_COMPANION_SET is safe to run for all cases DT, ACPI and DT&ACPI.
>>
>> Suggested-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
>> Signed-off-by: Tomasz Nowicki <tn@semihalf.com>
>> Tested-by: Duc Dang <dhdang@apm.com>
>> Tested-by: Dongdong Liu <liudongdong3@huawei.com>
>> Tested-by: Hanjun Guo <hanjun.guo@linaro.org>
>> Tested-by: Graeme Gregory <graeme.gregory@linaro.org>
>> Tested-by: Sinan Kaya <okaya@codeaurora.org>
>> ---
>>   drivers/acpi/pci_root.c | 5 ++++-
>>   drivers/pci/probe.c     | 2 ++
>>   2 files changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
>> index ae3fe4e..a65c8c2 100644
>> --- a/drivers/acpi/pci_root.c
>> +++ b/drivers/acpi/pci_root.c
>> @@ -846,7 +846,10 @@ 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,
>> +
>> +	/* Root bridge device needs to be sure of parent ACPI type */
> You can improve the comment.
>
> "pci_create_root_bus() needs to detect the parent device type,
> so initialize its companion data accordingly".
Agree, I will change comment.
>
>> +	ACPI_COMPANION_SET(&device->dev, device);
>> +	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/probe.c b/drivers/pci/probe.c
>> index 6d7ab9b..81dd3a2 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2100,6 +2100,8 @@ struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
>>   	bridge->dev.parent = parent;
>>   	bridge->dev.release = pci_release_host_bridge_dev;
>>   	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
>> +	ACPI_COMPANION_SET(&bridge->dev,
>> +			   parent ? to_acpi_device_node(parent->fwnode) : NULL);
> if (parent)
> 	ACPI_COMPANION_SET(&bridge->dev, ACPI_COMPANION(parent));
>
> ?
Looks better.
>
> It seems fine to me, hopefully Rafael can have a look to countercheck.
>
> Reviewed-by: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
Thanks,
Tomasz
diff mbox

Patch

diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
index ae3fe4e..a65c8c2 100644
--- a/drivers/acpi/pci_root.c
+++ b/drivers/acpi/pci_root.c
@@ -846,7 +846,10 @@  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,
+
+	/* Root bridge device needs to be sure of parent ACPI type */
+	ACPI_COMPANION_SET(&device->dev, device);
+	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/probe.c b/drivers/pci/probe.c
index 6d7ab9b..81dd3a2 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2100,6 +2100,8 @@  struct pci_bus *pci_create_root_bus(struct device *parent, int bus,
 	bridge->dev.parent = parent;
 	bridge->dev.release = pci_release_host_bridge_dev;
 	dev_set_name(&bridge->dev, "pci%04x:%02x", pci_domain_nr(b), bus);
+	ACPI_COMPANION_SET(&bridge->dev,
+			   parent ? to_acpi_device_node(parent->fwnode) : NULL);
 	error = pcibios_root_bridge_prepare(bridge);
 	if (error) {
 		kfree(bridge);