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

[Siddharth Vadapalli]

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@xxxxxxx>
> > > ---
> > > Changes since v1:
> > > https://lore.kernel.org/linux-pci/20250123070935.1810110-1-18255117159@xxxxxxx
> > > 
> > > - 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@xxxxxxx/
> > 
> > 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.

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.

I will let Bjorn and others share their views on this patch.

Regards,
Siddharth.