Re: [RFC PATCH 4/4] arm64, acpi, pci: Provide arch-specific calls for PCI host bridge dirver (PNP0A03).

[Tomasz Nowicki <tomasz.nowicki@linaro.org>]

On 07.11.2014 15:24, Arnd Bergmann wrote:
> On Friday 07 November 2014 14:27:56 Tomasz Nowicki wrote:
>>
>>   #ifdef CONFIG_PCI
>> +struct pci_controller {
>> +	struct acpi_device *companion;
>> +	int segment;
>> +	int node;		/* nearest node with memory or NUMA_NO_NODE for global allocation */
>> +};
>> +
>> +#define PCI_CONTROLLER(busdev) ((struct pci_controller *) busdev->sysdata)
>> +
>
> Don't use busdev->sysdata in architecture specific code, it belongs to the
> host bridge driver with the new model. For ACPI you don't have a host bridge
> driver, but it's better to keep these separate.
>
> The segment is always the same as the domain number, so just use that.
> The node and companion members here can get added to struct pci_host_bridge.

The reason why I put segment field to struct pci_controller is to 
initialize domain_nr of struct pci_bus being in pci_create_root_bus(), 
domain_nr can be used later on though. Correct me I am wrong.

Honestly I do not see the way to create root bus without e.g. 
sysdata.segment here.

>
>> @@ -43,7 +53,8 @@ resource_size_t pcibios_align_resource(void *data, const struct resource *res,
>>    */
>>   int pcibios_add_device(struct pci_dev *dev)
>>   {
>> -	dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>> +	if (acpi_disabled)
>> +		dev->irq = of_irq_parse_and_map_pci(dev, 0, 0);
>>
>>   	return 0;
>>   }
>
> How do you assign the irq number with ACPI? Do you only support MSI?

I missed that, will add in next ver.

>
>>   /*
>>    * raw_pci_read/write - Platform-specific PCI config space access.
>> - *
>> - * Default empty implementation.  Replace with an architecture-specific setup
>> - * routine, if necessary.
>>    */
>>   int raw_pci_read(unsigned int domain, unsigned int bus,
>>   		  unsigned int devfn, int reg, int len, u32 *val)
>>   {
>> -	return -EINVAL;
>> +	char __iomem *addr;
>> +
>> +	if (unlikely((bus > 255) || (devfn > 255) || (reg > 4095))) {
>> +err:		*val = -1;
>> +		return -EINVAL;
>> +	}
>> +
>> +	rcu_read_lock();
>> +	addr = pci_dev_base(domain, bus, devfn);
>> +	if (!addr) {
>> +		rcu_read_unlock();
>> +		goto err;
>> +	}
>
> The config space accessors should probably be shared with
> drivers/pci/host/pci-host-generic.c, e.g. by moving the rest of
> the new code in there as well, or by moving the config space
> accessors from that file to drivers/pci/mmconfig.c.

Tomasz