Re: [PATCH v4 01/16] PCI: dwc: Add pcie_cap field and caching helper

[Shawn Lin <shawn.lin@rock-chips.com>]

Hi Hans,

在 2026/10/27 星期二 20:28, Hans Zhang 写道:
> The DWC PCIe core and its platform drivers repeatedly call
> dw_pcie_find_capability(pci, PCI_CAP_ID_EXP) to obtain the offset of the
> PCI Express Capability structure. Besides being verbose, that performs a
> DBI read every time, including in paths that run repeatedly such as link
> up polling, suspend/resume and interrupt handlers.
> 
> Add a pcie_cap field to struct dw_pcie and a helper
> dw_pcie_get_pcie_cap() that performs the lookup on first use and caches
> the result for the subsequent calls.
> 
> The lookup is deliberately done lazily, at the first place that needs the
> offset, instead of at a fixed point during controller initialization.
> This guarantees that the DBI read happens exactly when the open coded
> dw_pcie_find_capability() call it replaces used to happen, so no DBI
> access can be made before the controller clocks, resets and power are


With the controller inaccessible (clocks/power gated), what a DBI
read returns is platform dependent, most integrations just hang the
accessing CPU, like Rockchip's SoCs, others might return zero or
garbage. Zero self-heals as the commit message describes, but a garbage 
non-zero lookup would now be cached indefinitely, so every later caller
would use a bogus offset?

Perhaps the "TX Interface Progression Detection Features" of dwc IP
could help return a dedicated value? I don't know, so I need to check
it internally. I will get back to you later.


> enabled. This matters for endpoint controllers such as qcom-ep and
> tegra194, where DBI is only accessible after PERST# is deasserted, and it
> also means platform drivers do not have to call the helper from a
> specific callback at the right time.
> 
> The cached value is accessed with READ_ONCE()/WRITE_ONCE(), because the
> first lookup may be triggered from different contexts, for example the
> qcom-ep global IRQ and PERST# IRQ handlers. A concurrent lookup stores
> the same value, so this race is benign.
> 
> A return value of 0 means that the capability is not present. This
> cannot happen on real DWC hardware, where the PCI Express Capability is
> mandatory, so a negative result is not cached separately: it only causes
> the search to be repeated, which does not affect correctness.
> 
> Signed-off-by: Hans Zhang <18255117159@163.com>
> ---
>   drivers/pci/controller/dwc/pcie-designware.h | 34 ++++++++++++++++++++
>   1 file changed, 34 insertions(+)
> 
> diff --git a/drivers/pci/controller/dwc/pcie-designware.h b/drivers/pci/controller/dwc/pcie-designware.h
> index 0735ae940924..fa31e8b763bc 100644
> --- a/drivers/pci/controller/dwc/pcie-designware.h
> +++ b/drivers/pci/controller/dwc/pcie-designware.h
> @@ -592,6 +592,8 @@ struct dw_pcie {
>   	 * use_parent_dt_ranges to true to avoid this warning.
>   	 */
>   	bool			use_parent_dt_ranges;
> +
> +	u8			pcie_cap;	/* Offset of PCI_CAP_ID_EXP */
>   };
>   
>   #define to_dw_pcie_from_pp(port) container_of((port), struct dw_pcie, pp)
> @@ -830,6 +832,38 @@ static inline void dw_pcie_dbi_ro_wr_dis(struct dw_pcie *pci)
>   	dw_pcie_writel_dbi(pci, reg, val);
>   }
>   
> +/**
> + * dw_pcie_get_pcie_cap() - Return the PCIe Capability offset
> + * @pci: DWC instance
> + *
> + * The offset of the PCI Express Capability structure is read once from
> + * the DBI configuration space on the first call and cached in
> + * @pci->pcie_cap for the subsequent ones.
> + *
> + * The lookup is done lazily at the first place that actually needs the
> + * offset, so the DBI read happens exactly when it used to happen before
> + * caching was introduced, i.e. never before the controller's clocks,
> + * resets and power are enabled.
> + *
> + * DWC PCIe controllers always implement the PCI Express Capability, so a
> + * return value of 0 means that the hardware is not usable. Callers that
> + * check the returned offset keep working, and the (never expected)
> + * lookup is simply repeated on the next call.
> + *
> + * Return: the cached offset, or 0 if the capability is not present.
> + */
> +static inline u8 dw_pcie_get_pcie_cap(struct dw_pcie *pci)
> +{
> +	u8 cap = READ_ONCE(pci->pcie_cap);
> +
> +	if (!cap) {
> +		cap = dw_pcie_find_capability(pci, PCI_CAP_ID_EXP);
> +		WRITE_ONCE(pci->pcie_cap, cap);
> +	}
> +
> +	return cap;
> +}
> +
>   static inline int dw_pcie_start_link(struct dw_pcie *pci)
>   {
>   	if (pci->ops && pci->ops->start_link)