Re: [PATCH V3 2/2] PCI/ACPI: hisi: Add ACPI support for HiSilicon SoCs Host Controllers

[Dongdong Liu <liudongdong3@huawei.com>]

Hi Lorenzo

Many thanks for your review.
在 2016/10/22 0:08, Lorenzo Pieralisi 写道:
> On Fri, Oct 21, 2016 at 02:12:44PM +0800, Dongdong Liu wrote:
>
> [...]
>
>>>> +static int hisi_pcie_init(struct pci_config_window *cfg)
>>>> +{
>>>> +       int ret;
>>>> +       struct acpi_device *adev = to_acpi_device(cfg->parent);
>>>
>>> Why is this expected to be struct acpi_device?
>>
>> I use this acpi device(Device (PCI2)) to get it's child acpi device(Device (RES2)), then
>> obtain rc base addresses from PNP0C02 as subdevice of PNP0A03.
>>
>> The procedure to get the value of cfg->parent is as the below.
>> arch/arm64/kernel/pci.c
>> pci_acpi_scan_root(struct acpi_pci_root *root)
>> 	--->pci_acpi_setup_ecam_mapping(root, ri);
>> 		--->pci_ecam_create(&root->device->dev, &cfgres, bus_res, ecam_ops);
>> 			--->cfg->parent = dev
>> ;
>> PCIe DSDT table defines as below.
>> Device (PCI2)
>> {
>> 	Name (_HID, "PNP0A08") // PCI Express Root Bridge
>> 	Name (_CID, "PNP0A03") // Compatible PCI Root Bridge
>> 	......
>> 	Device (RES2)
>> 	{
>> 		Name (_HID, "HISI0081") // HiSi PCIe RC config base address
>> 		Name (_CID, "PNP0C02") // Motherboard reserved resource
>> 		Name (_CRS, ResourceTemplate (){
>> 			Memory32Fixed (ReadWrite, 0xa00a0000 , 0x10000)
>> 	})
>> 	......
>> }
>>>
>>> Shouldn't it be a "physical" device whose companion is an ACPI device object?
>>
>> Sorry, I don't understand.  What do you mean ?
>
> I think Rafael meant the physical node (that in this case is the pci
> bridge device) that is associated with the acpi device (the struct
> pci_config_window.parent pointer points at the PNP0A03/PNP0A08 acpi
> device object though, not the RES2 acpi device that's what you need).
>
> Have a look at:
>
> Documentation/acpi/enumeration.txt
> Documentation/acpi/namespace.txt

Thanks for explaining this.

>
>> It should be a "physical" device. but I have not saw about
>> ACPI_COMPANION_SET() of this device.
>
> The PNP0A08 acpi_device (that is what is pointed at by
> struct acpi_device = to_acpi_device(pci_config_window.parent)
> is the respective pci bridge device companion.
>
> arch/arm64/kernel/pci.c (pcibios_root_bridge_prepare()).
>
> Now for something completely different :), your RES2 pseudo-device.
>
> IIUC platform device (physical node) is created by the ACPI enumeration
> code for your RES2 pseudo-device and its resources are already
> initialized (by parsing its _CRS) in acpi_create_platform_device().

This is not right. About RES2 pseudo-device,it's _CID is PNP0C02,
this will attach acpi_pnp_attach() (drivers/acpi/acpi_pnp.c) .
acpi_pnp_attach() return 1;ret = acpi_scan_attach_handler() will return 1.
So this will not call acpi_default_enumeration(),
this also will not call acpi_create_platform_device().

drivers/acpi/scan.c
static void acpi_bus_attach(struct acpi_device *device)
{
......
	ret = acpi_scan_attach_handler(device);
	if (ret < 0)
		return;

	device->flags.match_driver = true;
	if (!ret) {
		ret = device_attach(&device->dev);
		if (ret < 0)
			return;
	
		if (!ret && device->pnp.type.platform_id)
			acpi_default_enumeration(device);
	}
......
}

Dongdong
Thanks
>
> This means that by the time you match the PNP0A03/PNP0A08 child with an
> acpi_device with _HID == "HISI0081", you can get its associated
> physical node and get its resources IOMEM through the physical
> node (ie platform device and related resources) instead of re-parsing
> the _CRS if I am not mistaken (and that's an IF because I did not
> test it).

As above said this does not re-parse the _CRS for RES2 pseudo-device.

Dongdong
Thanks
>
> Regardless, I am not entirely sure there are kernel drivers/control
> paths already using this mechanism to handle child devices (keeping in
> mind that RES2 is not a real device at all, it is there to represent
> PNP0A03 sub-address space for PCI quirks), so it would be good to get
> Rafael's agreement on this approach to prevent abusing the current ACPI
> platform devices glue code assumptions.
>
> It would also be good to agree on the whole approach soundness.
>
> Thanks,
> Lorenzo
>
> .
>