Re: [RFC PATCH 00/16] Refine PCI host bridge scan interfaces

[Yijing Wang]

On 2014/11/18 19:30, Arnd Bergmann wrote:
> On Tuesday 18 November 2014 19:17:32 Yijing Wang wrote:
>> On 2014/11/17 22:13, Arnd Bergmann wrote:
>>> On Monday 17 November 2014 18:21:34 Yijing Wang wrote:
>>>> This series is based Linux 3.18-rc1 and Lorenzo Pieralisi's
>>>> arm PCI domain cleanup patches, link: 
>>>> https://patchwork.ozlabs.org/patch/407585/
>>>>
>>>> Current pci scan interfaces like pci_scan_root_bus() and directly
>>>> call pci_create_root_bus()/pci_scan_child_bus() lack flexiblity.
>>>> Some platform infos like PCI domain and msi_chip have to be
>>>> associated to PCI bus by some arch specific function.
>>>> We want to make a generic pci_host_bridge, and make it hold
>>>> the platform infos or hook. Then we could eliminate the lots
>>>> of arch pci_domain_nr, also we could associate some platform 
>>>> ops something like pci_get_msi_chip(struct pci_dev *dev)
>>>> with pci_host_bridge to avoid introduce arch weak functions.
>>>>
>>>> This RFC version not for all platforms, just applied the new
>>>> scan interface in x86/arm/powerpc/ia64, I will refresh other
>>>> platforms after the core pci scan interfaces are ok.
>>>
>>> I think overall this is a good direction to take, in particular
>>> moving more things into struct pci_host_bridge so we can
>>> slim down the architecture specific code.
>>
>> Hi Arnd, thanks very much for your review and comments!
>>
>>>
>>> I don't particularly like the way you use the 'pci_host_info'
>>> to pass callback pointers and some of the generic information.
>>> This duplicates some of the issues we are currently trying
>>> to untangle in the arm32 code to make drivers easier to share
>>> between architectures.
>>
>> What arm32 code you are trying to untangle for example ?
> 
> We have a few problems that currently prevent us from using shared
> drivers across arm32 and arm64:
> 
> - arm32 has an architecture-defined pci_sys_data structure, but
>   we really want to have one that is defined by the host bridge driver
>   and that is architecture independent. Some core functions depend
>   on this structure at the moment, which Lorenzo is trying to
>   undo
> 
> - The pci_common_init interface on arm32 doesn't work well on
>   loadable drivers, it does not return an error, and it is built
>   around the assumption that you probe all pci host bridges at
>   the same time, while the standard Linux driver model assumes
>   that you probe one at a time.
> 
> - The way we pass a temporary structure (hw_pci) with function pointers
>   into the architecture code makes it relatively hard to follow
>   how the initialization sequence works.
>  
>> Introduce pci_host_info here because I want to make the PCI scan interfaces
>> simple to host drviers, host drivers only need to call one scan
>> interface(pci_scan_host_bridge), but from your comments,
>> The combination pci_create_host_bridge() + pci_scan_xx()
>> seems to be more popular.
> 
> Yes, I think a simpler interface structure would be better than trying
> to minimize the amount of code needed in drivers at the expense of
> interface complexity.
> 
>>> As a general approach, I'd rather see generic helper functions
>>> being exported by the PCI core that a driver may or may not
>>> call. 
>>> The way you split the interface between things that happen
>>> before scanning the buses (pci_create_host_bridge) and
>>> the actual scanning (__pci_create_root_bus, pci_scan_child_bus)
>>> seems very helpful and I think we can expand that concept further:
>>>
>>> - The normal pci_create_host_bridge() function can contain
>>>   all of the DT scanning functions (finding bus/mem/io resources, 
>>>   finding the msi-parent), while drivers that don't depend on DT
>>>   for this information can call the same function and fill the
>>>   same things after they have the pci_host_bridge pointer.
>>>
>>> - If a driver needs to set up mapping windows, it can do that after
>>>   calling pci_create_host_bridge(). E.g. all the dw_pcie glue drivers
>>>   can call a dw_pcie_setup_windows() function that takes the resources
>>>   out of the pci_host_bridge pointer before the bus is scanned.
>>>
>>> - The ACPI code can have a completely different way of creating
>>>   a struct pci_host_bridge, which is also passed into the same
>>>   bus scanning functions, but doesn't have to come from
>>>   pci_create_host_bridge.

Thanks for your explanation, I will consider these problems when I refactor the
core generic interfaces.

>>
>> I hope platforms with ACPI or DT could both use pci_create_host_bridge().
>> Why we need to use two different ways to process it ?
> 
> These are completely different use cases:
> 
> a) For DT, we want loadable device drivers that start by probing a host
>    bridge device which was added through the DT platform code. The
>    driver is self-contained, and eventually we want to be able to unload
>    it. We have lots of different per-soc drivers that require different
>    quirks
> 
> b) For ACPI, the interface is defined in the ACPI spec across architectures
>    and SoCs, we don't have host bridge drivers and the code that initializes
>    the PCI is required early during boot and called from architecture
>    code. There is no parent device, as ACPI sees PCI as a fundamental building
>    block by itself, and there are no drivers because the firmware does
>    the initial hardware setup, so we only have to access the config space.

Hmmm, I'm a little confused, so why you think ACPI host driver should not use
pci_create_host_bridge(), because ACPI PCI driver has no parent device ?

> 
> 	Arnd
> 
> .
> 


-- 
Thanks!
Yijing

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/