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@xxxxxxx>
> ---
> Changes since v8 ~ v11:
> - None
>
> Changes since v7:
> - Resolve compilation errors.
>
> Changes since v6:
> https://lore.kernel.org/linux-pci/20250323164852.430546-4-18255117159@xxxxxxx/
>
> - The patch commit message were modified.
>
> Changes since v5:
> https://lore.kernel.org/linux-pci/20250321163803.391056-4-18255117159@xxxxxxx
>
> - 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@xxxxxxxxxxxxxxx>
--
i.
