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

[Hans Zhang <18255117159@163.com>]

On 10/27/26 21:12, Shawn Lin wrote:
> 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.

Hi,

Thanks for the review.


You are right that the cache must not turn a bogus value into a permanent
one. Some details and one change in v5:

- dw_pcie_get_pcie_cap() is only called at the sites that previously
   called dw_pcie_find_capability() directly, so this series does not add
   or move a DBI access. It does not try to fix the "DBI accessed while
   the controller is inaccessible" problem; that behavior is platform
   specific and pre-existing.
- dw_pcie_find_capability() already rejects offsets below the standard
   header, unaligned offsets, and entries whose capability ID is not
   PCI_CAP_ID_EXP, so a bogus value has to look like a valid capability
   entry to be returned at all.
- To make sure that a transient bogus read cannot stick anyway, v5 only
   caches the offset after the capability ID has been read back at that
   offset. If the read-back does not confirm PCI_CAP_ID_EXP, the offset is
   still used for the current access (same as the open coded search
   before), but it is not cached, and the next call searches again - the
   same self-healing behavior as the zero/not-found case.

If the DWC IP has a reliable way to tell whether the core is accessible
(the "TX Interface Progression Detection" feature you mentioned), that
would be even better than validating the value. I would be glad to use it
if you can share the details; it probably deserves a separate patch so
that all DBI accesses can be guarded, not only this lookup.

Best regards,
Hans

> 
> 
>> 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)