Re: [v2] PCI: cadence: Add configuration space capability search API

[Hans Zhang <18255117159@163.com>]

On 2025/3/9 13:48, Siddharth Vadapalli wrote:
> On Sun, Mar 09, 2025 at 11:18:21AM +0800, Hans Zhang wrote:
>>
>>
>> On 2025/3/9 10:38, Siddharth Vadapalli wrote:
>>> On Sat, Mar 08, 2025 at 09:39:03PM +0800, Hans Zhang wrote:
>>>> Add configuration space capability search API using struct cdns_pcie*
>>>> pointer.
>>>>
>>>> The offset address of capability or extended capability designed by
>>>> different SOC design companies may not be the same. Therefore, a flexible
>>>> public API is required to find the offset address of a capability or
>>>> extended capability in the configuration space.
>>>>
>>>> Signed-off-by: Hans Zhang <18255117159@163.com>
>>>> ---
>>>> Changes since v1:
>>>> https://lore.kernel.org/linux-pci/20250123070935.1810110-1-18255117159@163.com
>>>>
>>>> - Added calling the new API in PCI-Cadence ep.c.
>>>> - Add a commit message reason for adding the API.
>>>
>>> In reply to your v1 patch, you have mentioned the following:
>>> "Our controller driver currently has no plans for upstream and needs to
>>> wait for notification from the boss."
>>> at:
>>> https://lore.kernel.org/linux-pci/fcfd4827-4d9e-4bcd-b1d0-8f9e349a6be7@163.com/
>>>
>>> Since you have posted this patch, does it mean that you will be
>>> upstreaming your driver as well? If not, we still end up in the same
>>> situation as earlier where the Upstream Linux has APIs to support a
>>> Downstream driver.
>>>
>>> Bjorn indicated the above already at:
>>> https://lore.kernel.org/linux-pci/20250123170831.GA1226684@bhelgaas/
>>> and you did agree to do so. But this patch has no reference to the
>>> upstream driver series which shall be making use of the APIs in this
>>> patch.
>>
>> Hi Siddharth,
>>
>>
>> Bjorn:
>>    If/when you upstream code that needs this interface, include this
>>    patch as part of the series.  As Siddharth pointed out, we avoid
>>    merging code that has no upstream users.
>>
>>
>> Hans: This user is: pcie-cadence-ep.c. I think this is an optimization of
>> Cadence common code. I think this is an optimization of Cadence common code.
>> Siddharth, what do you think?
> 
> This seems to be an extension of the driver rather than an optimization.
> At first glance, though it seems like this patch is enabling code-reuse,
> it is actually attempting to walk through the config space registers to
> identify a capability. Prior to this patch, those offsets were hard-coded,
> saving the trouble of having to walk through the capability pointers to
> arrive at the capability.

Hi Siddharth,

Prior to this patch, I don't think hard-coded is that reasonable. 
Because the SOC design of each company does not guarantee that the 
offset of each capability is the same. This parameter can be configured 
when selecting PCIe configuration options. The current code that just 
happens to hit the offset address is the same.

You can refer to the pci_find_*capability() or dw_pcie_find_*capability 
API. The meaning of their appearance is the same as what I said, and the 
design of each company may be different.

Best regards,
Hans

> 
> This patch will affect the following functions:
> 01. cdns_pcie_get_fn_from_vfn()
> 02. cdns_pcie_ep_write_header()
> 03. cdns_pcie_ep_set_msi()
> 04. cdns_pcie_ep_get_msi()
> 05. cdns_pcie_ep_get_msix()
> 06. cdns_pcie_ep_set_msix()
> 07. cdns_pcie_ep_send_msi_irq()
> 08. cdns_pcie_ep_map_msi_irq()
> 09. cdns_pcie_ep_send_msix_irq()
> 10. cdns_pcie_ep_start()
> which will now take longer to get to the capability whose offset was
> known. I understand that you wish to extend these functions to support
> your SoC where the offsets don't match the hard-coded ones.
>
In my opinion, hard-coded is not universal.

> I will let Bjorn and others share their views on this patch.
> 
> Regards,
> Siddharth.