Re: [v6 1/5] PCI: Introduce generic capability search functions

[Hans Zhang <18255117159@163.com>]

On 2025/3/24 22:52, Ilpo Järvinen wrote:
>>>> --- a/drivers/pci/controller/Kconfig
>>>> +++ b/drivers/pci/controller/Kconfig
>>>> @@ -132,6 +132,23 @@ config PCI_HOST_GENERIC
>>>>    	  Say Y here if you want to support a simple generic PCI host
>>>>    	  controller, such as the one emulated by kvmtool.
>>>>    +config PCI_HOST_HELPERS
>>>> +	bool
>>>> +	prompt "PCI Host Controller Helper Functions" if EXPERT
>>>> + 	help
>>>> +	  This provides common infrastructure for PCI host controller drivers
>>>> to
>>>> +	  handle PCI capability scanning and other shared operations. The
>>>> helper
>>>> +	  functions eliminate code duplication across controller drivers.
>>>> +
>>>> +	  These functions are used by PCI controller drivers that need to scan
>>>> +	  PCI capabilities using controller-specific access methods (e.g. when
>>>> +	  the controller is behind a non-standard configuration space).
>>>> +
>>>> +	  If you are using any PCI host controller drivers that require these
>>>> +	  helpers (such as DesignWare, Cadence, etc), this will be
>>>> +	  automatically selected. Say N unless you are developing a custom PCI
>>>> +	  host controller driver.
>>>
>>> Hi,
>>>
>>> Does this need to be user selectable at all? What's the benefit? If
>>> somebody is developing a driver, they can just as well add the select
>>> clause in that driver to get it built.
>>>
>>
>> Dear Ilpo,
>>
>> Thanks your for reply. Only DWC and CDNS drivers are used here, what do you
>> suggest should be done?
> 
> Just make it only Kconfig select'able and not user selectable at all.
> 

Hi Ilpo,

Thanks your for reply. Will change.

Will delete it.
prompt "PCI Host Controller Helper Functions" if EXPERT

>>>> + * These interfaces resemble the pci_find_*capability() interfaces, but
>>>> these
>>>> + * are for configuring host controllers, which are bridges *to* PCI
>>>> devices but
>>>> + * are not PCI devices themselves.
>>>> + */
>>>> +static u8 __pci_host_bridge_find_next_cap(void *priv,
>>>> +					  pci_host_bridge_read_cfg read_cfg,
>>>> +					  u8 cap_ptr, u8 cap)
>>>> +{
>>>> +	u8 cap_id, next_cap_ptr;
>>>> +	u16 reg;
>>>> +
>>>> +	if (!cap_ptr)
>>>> +		return 0;
>>>> +
>>>> +	reg = read_cfg(priv, cap_ptr, 2);
>>>> +	cap_id = (reg & 0x00ff);
>>>> +
>>>> +	if (cap_id > PCI_CAP_ID_MAX)
>>>> +		return 0;
>>>> +
>>>> +	if (cap_id == cap)
>>>> +		return cap_ptr;
>>>> +
>>>> +	next_cap_ptr = (reg & 0xff00) >> 8;
>>>> +	return __pci_host_bridge_find_next_cap(priv, read_cfg, next_cap_ptr,
>>>> +					       cap);
>>>
>>> This is doing (tail) recursion?? Why??
>>>
>>> What should be done, IMO, is that code in __pci_find_next_cap_ttl()
>>> refactored such that it can be reused instead of duplicating it in a
>>> slightly different form here and the functions below.
>>>
>>> The capability list parser should be the same?
>>>
>>
>> The original function is in the following file:
>> drivers/pci/controller/dwc/pcie-designware.c
>> u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
>> u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
>>
>> CDNS has the same need to find the offset of the capability.
>>
>> We don't have pci_dev before calling pci_host_probe, but we want to get the
>> offset of the capability and configure some registers to initialize the root
>> port. Therefore, the __pci_find_next_cap_ttl function cannot be used. This is
>> also the purpose of dw_pcie_find_*capability.
> 
> __pci_find_next_cap_ttl() does not take pci_dev so I'm unsure if the
> problem is real or not?!?

__pci_find_next_cap_ttl uses pci_bus as the first argument, and other 
functions take pci_dev->bus as its first argument. Either way, either 
pci_bus or pci_dev is required, and before pcie enumeration, there was 
no pci_bus or pci_dev.

I replied to you in the patch email [v6 3/5], if I wasn't clear enough, 
please remind me and we'll discuss it again.

> 
>> The CDNS driver does not have a cdns_pcie_find_*capability function.
>> Therefore, separate the find capability, and then DWC and CDNS can be used at
>> the same time to reduce duplicate code.
>>
>>
>> Communication history:
>>
>> Bjorn HelgaasMarch 14, 2025, 8:31 p.m. UTC | #8
>> On Fri, Mar 14, 2025 at 06:35:11PM +0530, Manivannan Sadhasivam wrote:
>>> ...
>>
>>> Even though this patch is mostly for an out of tree controller
>>> driver which is not going to be upstreamed, the patch itself is
>>> serving some purpose. I really like to avoid the hardcoded offsets
>>> wherever possible. So I'm in favor of this patch.
>>>
>>> However, these newly introduced functions are a duplicated version
>>> of DWC functions. So we will end up with duplicated functions in
>>> multiple places. I'd like them to be moved (both this and DWC) to
>>> drivers/pci/pci.c if possible. The generic function
>>> *_find_capability() can accept the controller specific readl/ readw
>>> APIs and the controller specific private data.
>>
>> I agree, it would be really nice to share this code.
>>
>> It looks a little messy to deal with passing around pointers to
>> controller read ops, and we'll still end up with a lot of duplicated
>> code between __pci_find_next_cap() and __cdns_pcie_find_next_cap(),
>> etc.
>>
>> Maybe someday we'll make a generic way to access non-PCI "config"
>> space like this host controller space and PCIe RCRBs.
>>
>> Or if you add interfaces that accept read/write ops, maybe the
>> existing pci_find_capability() etc could be refactored on top of them
>> by passing in pci_bus_read_config_word() as the accessor.
> 
> At minimum, the loop in __pci_find_next_cap_ttl() could be turned into a
> macro similar to eg. read_poll_timeout() that takes the read function as
> an argument (read_poll_timeout() looks messy because it doesn't align
> backslashed to far right). That would avoid duplicating the parsing logic
> on C code level.
> 

The config space register cannot be read before PCIe enumeration. Only 
the read and write functions of the root port driver can be used.

Best regards,
Hans