Re: [PATCH v12 6/6] PCI: cadence: Use cdns_pcie_find_*capability to avoid hardcode.

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 7641 bytes --]

On Thu, 15 May 2025, Hans Zhang wrote:

> The PCIe capability/extended capability offsets are not guaranteed to be
> the same across all SoCs integrating the Cadence PCIe IP. Hence, use the
> cdns_pcie_find_{ext}_capability() APIs for finding them.

A minor point perhaps, but IMO, controller drivers should use the core's 
capability search regardless of the offset being same or not. :-)

> This avoids hardcoding the offsets in the driver.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
> Changes since v8 ~ v11:
> - None
> 
> Changes since v7:
> - Resolve compilation errors.
> 
> Changes since v6:
> https://lore.kernel.org/linux-pci/20250323164852.430546-4-18255117159@163.com/
> 
> - The patch commit message were modified.
> 
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@163.com
> 
> - Kconfig add "select PCI_HOST_HELPERS"
> ---
>  .../pci/controller/cadence/pcie-cadence-ep.c  | 40 +++++++++++--------
>  drivers/pci/controller/cadence/pcie-cadence.h |  5 ---
>  2 files changed, 23 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/pci/controller/cadence/pcie-cadence-ep.c b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> index 599ec4b1223e..5c4b2151d181 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence-ep.c
> +++ b/drivers/pci/controller/cadence/pcie-cadence-ep.c
> @@ -19,12 +19,13 @@
>  
>  static u8 cdns_pcie_get_fn_from_vfn(struct cdns_pcie *pcie, u8 fn, u8 vfn)
>  {
> -	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
>  	u32 first_vf_offset, stride;
> +	u16 cap;
>  
>  	if (vfn == 0)
>  		return fn;
>  
> +	cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
>  	first_vf_offset = cdns_pcie_ep_fn_readw(pcie, fn, cap + PCI_SRIOV_VF_OFFSET);
>  	stride = cdns_pcie_ep_fn_readw(pcie, fn, cap +  PCI_SRIOV_VF_STRIDE);
>  	fn = fn + first_vf_offset + ((vfn - 1) * stride);
> @@ -36,10 +37,11 @@ static int cdns_pcie_ep_write_header(struct pci_epc *epc, u8 fn, u8 vfn,
>  				     struct pci_epf_header *hdr)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> -	u32 cap = CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET;
>  	struct cdns_pcie *pcie = &ep->pcie;
>  	u32 reg;
> +	u16 cap;
>  
> +	cap = cdns_pcie_find_ext_capability(pcie, PCI_EXT_CAP_ID_SRIOV);
>  	if (vfn > 1) {
>  		dev_err(&epc->dev, "Only Virtual Function #1 has deviceID\n");
>  		return -EINVAL;
> @@ -224,9 +226,10 @@ static int cdns_pcie_ep_set_msi(struct pci_epc *epc, u8 fn, u8 vfn, u8 mmc)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct cdns_pcie *pcie = &ep->pcie;
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>  	u16 flags;
> +	u8 cap;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>  	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>  
>  	/*
> @@ -246,9 +249,10 @@ static int cdns_pcie_ep_get_msi(struct pci_epc *epc, u8 fn, u8 vfn)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct cdns_pcie *pcie = &ep->pcie;
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>  	u16 flags, mme;
> +	u8 cap;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>  	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>  
>  	/* Validate that the MSI feature is actually enabled. */
> @@ -269,9 +273,10 @@ static int cdns_pcie_ep_get_msix(struct pci_epc *epc, u8 func_no, u8 vfunc_no)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct cdns_pcie *pcie = &ep->pcie;
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>  	u32 val, reg;
> +	u8 cap;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
>  	func_no = cdns_pcie_get_fn_from_vfn(pcie, func_no, vfunc_no);
>  
>  	reg = cap + PCI_MSIX_FLAGS;
> @@ -290,9 +295,10 @@ static int cdns_pcie_ep_set_msix(struct pci_epc *epc, u8 fn, u8 vfn,
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
>  	struct cdns_pcie *pcie = &ep->pcie;
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>  	u32 val, reg;
> +	u8 cap;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
>  	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>  
>  	reg = cap + PCI_MSIX_FLAGS;
> @@ -378,11 +384,11 @@ static int cdns_pcie_ep_send_msi_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
>  				     u8 interrupt_num)
>  {
>  	struct cdns_pcie *pcie = &ep->pcie;
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>  	u16 flags, mme, data, data_mask;
> -	u8 msi_count;
>  	u64 pci_addr, pci_addr_mask = 0xff;
> +	u8 msi_count, cap;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>  	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>  
>  	/* Check whether the MSI feature has been enabled by the PCI host. */
> @@ -430,14 +436,14 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
>  				    u32 *msi_addr_offset)
>  {
>  	struct cdns_pcie_ep *ep = epc_get_drvdata(epc);
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET;
>  	struct cdns_pcie *pcie = &ep->pcie;
>  	u64 pci_addr, pci_addr_mask = 0xff;
>  	u16 flags, mme, data, data_mask;
> -	u8 msi_count;
> +	u8 msi_count, cap;
>  	int ret;
>  	int i;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSI);
>  	fn = cdns_pcie_get_fn_from_vfn(pcie, fn, vfn);
>  
>  	/* Check whether the MSI feature has been enabled by the PCI host. */
> @@ -480,16 +486,16 @@ static int cdns_pcie_ep_map_msi_irq(struct pci_epc *epc, u8 fn, u8 vfn,
>  static int cdns_pcie_ep_send_msix_irq(struct cdns_pcie_ep *ep, u8 fn, u8 vfn,
>  				      u16 interrupt_num)
>  {
> -	u32 cap = CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET;
>  	u32 tbl_offset, msg_data, reg;
>  	struct cdns_pcie *pcie = &ep->pcie;
>  	struct pci_epf_msix_tbl *msix_tbl;
>  	struct cdns_pcie_epf *epf;
>  	u64 pci_addr_mask = 0xff;
>  	u64 msg_addr;
> +	u8 bir, cap;
>  	u16 flags;
> -	u8 bir;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_MSIX);
>  	epf = &ep->epf[fn];
>  	if (vfn > 0)
>  		epf = &epf->epf[vfn - 1];
> @@ -563,7 +569,9 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  	int max_epfs = sizeof(epc->function_num_map) * 8;
>  	int ret, epf, last_fn;
>  	u32 reg, value;
> +	u8 cap;
>  
> +	cap = cdns_pcie_find_capability(pcie, PCI_CAP_ID_EXP);
>  	/*
>  	 * BIT(0) is hardwired to 1, hence function 0 is always enabled
>  	 * and can't be disabled anyway.
> @@ -587,12 +595,10 @@ static int cdns_pcie_ep_start(struct pci_epc *epc)
>  				continue;
>  
>  			value = cdns_pcie_ep_fn_readl(pcie, epf,
> -					CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
> -					PCI_EXP_DEVCAP);
> +						      cap + PCI_EXP_DEVCAP);
>  			value &= ~PCI_EXP_DEVCAP_FLR;
> -			cdns_pcie_ep_fn_writel(pcie, epf,
> -					CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET +
> -					PCI_EXP_DEVCAP, value);
> +			cdns_pcie_ep_fn_writel(pcie, epf, cap + PCI_EXP_DEVCAP,
> +					       value);
>  		}
>  	}
>  
> diff --git a/drivers/pci/controller/cadence/pcie-cadence.h b/drivers/pci/controller/cadence/pcie-cadence.h
> index 0a4a8bfd3174..e7c108f6e0b2 100644
> --- a/drivers/pci/controller/cadence/pcie-cadence.h
> +++ b/drivers/pci/controller/cadence/pcie-cadence.h
> @@ -125,11 +125,6 @@
>   */
>  #define CDNS_PCIE_EP_FUNC_BASE(fn)	(((fn) << 12) & GENMASK(19, 12))
>  
> -#define CDNS_PCIE_EP_FUNC_MSI_CAP_OFFSET	0x90
> -#define CDNS_PCIE_EP_FUNC_MSIX_CAP_OFFSET	0xb0
> -#define CDNS_PCIE_EP_FUNC_DEV_CAP_OFFSET	0xc0
> -#define CDNS_PCIE_EP_FUNC_SRIOV_CAP_OFFSET	0x200
> -
>  /*
>   * Endpoint PF Registers
>   */
> 

Nice to see these go away.

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

-- 
 i.