[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <bee6ba78-d65a-4a04-b83f-3be0676e8ad8@163.com>
Date: Tue, 25 Mar 2025 10:58:27 +0800
From: Hans Zhang <18255117159@....com>
To: Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Cc: lpieralisi@...nel.org, kw@...ux.com, manivannan.sadhasivam@...aro.org,
robh@...nel.org, bhelgaas@...gle.com, jingoohan1@...il.com,
thomas.richard@...tlin.com, linux-pci@...r.kernel.org,
LKML <linux-kernel@...r.kernel.org>
Subject: Re: [v6 1/5] PCI: Introduce generic capability search functions
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
Powered by blists - more mailing lists