diff mbox

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

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

Commit Message

Tomasz Nowicki Jan. 13, 2016, 1:20 p.m. UTC
Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
bridge initialization. They both use sysdata pill to pass down parent
device reference and both relay on NULL parent in pci_create_root_bus()
to validate sysdata content.

It looks hacky and prevent us from getting some frimware specific
info for PCI host controller e.g. bus domain number.
However, it seems we can 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>
---
 drivers/acpi/pci_root.c | 5 ++++-
 drivers/pci/probe.c     | 2 ++
 2 files changed, 6 insertions(+), 1 deletion(-)

Comments

Lorenzo Pieralisi Jan. 19, 2016, 4:02 p.m. UTC | #1
On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
> Currently we have two platforms (x86 & ia64) capable of PCI ACPI host
> bridge initialization. They both use sysdata pill to pass down parent

pill ?

> device reference and both relay on NULL parent in pci_create_root_bus()

s/relay/rely

> to validate sysdata content.
> 
> It looks hacky and prevent us from getting some frimware specific

"prevents us from getting some firmware..."

> info for PCI host controller e.g. bus domain number.
> However, it seems we can overcome that blocker by passing down parent

It does not seem, either this patch overcomes the issue and it is
accepted, or it does not and it is discarded.

> device via pci_create_root_bus parameter (as the ACPI device type)

I do not understand what you mean by "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>
> ---
>  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 */
> +	ACPI_COMPANION_SET(&device->dev, device);

I do not understand why the code above is needed, can you elaborate
please ?

Thanks,
Lorenzo

> +	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 553a029..cad836f 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -2107,6 +2107,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);
> -- 
> 1.9.1
>
Tomasz Nowicki Jan. 20, 2016, 11:20 a.m. UTC | #2
On 19.01.2016 17:02, Lorenzo Pieralisi wrote:
> On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
[...]
>> device via pci_create_root_bus parameter (as the ACPI device type)
>
> I do not understand what you mean by "as the ACPI device type".

See below.

>
>> 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>
>> ---
>>   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 */
>> +	ACPI_COMPANION_SET(&device->dev, device);
>
> I do not understand why the code above is needed, can you elaborate
> please ?
>

This makes sure that device->dev can be identified as ACPI device, so we 
can use to_acpi_device_node() and assign companion safely below.

>
>> +	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 553a029..cad836f 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2107,6 +2107,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);

pci_create_root_bus() can be called with DT device, then 
ACPI_COMPANION_SET assign no companion to bridge device.

Thanks,
Tomasz
Lorenzo Pieralisi Jan. 20, 2016, 12:38 p.m. UTC | #3
On Wed, Jan 20, 2016 at 12:20:07PM +0100, Tomasz Nowicki wrote:
> On 19.01.2016 17:02, Lorenzo Pieralisi wrote:
> >On Wed, Jan 13, 2016 at 02:20:57PM +0100, Tomasz Nowicki wrote:
> [...]
> >>device via pci_create_root_bus parameter (as the ACPI device type)
> >
> >I do not understand what you mean by "as the ACPI device type".
> 
> See below.
> 
> >
> >>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>
> >>---
> >>  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 */
> >>+	ACPI_COMPANION_SET(&device->dev, device);
> >
> >I do not understand why the code above is needed, can you elaborate
> >please ?
> >
> 
> This makes sure that device->dev can be identified as ACPI device,
> so we can use to_acpi_device_node() and assign companion safely
> below.

I do not follow. If you refer to the fwnode handle type, that is
already set by ACPI core code (before acpi_pci_root_add() is called,
in acpi_init_device_object()).

Did you add the code above to solve a real issue you encountered ?

Thanks,
Lorenzo
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 553a029..cad836f 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2107,6 +2107,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);