Re: [PATCH v12 4/6] PCI: dwc: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang <18255117159@163.com>]

On 2025/6/3 17:42, Ilpo Järvinen wrote:
> On Thu, 15 May 2025, Hans Zhang wrote:
> 
>> Use the PCI core is now exposing generic macros for the host bridges to
>> search for the PCIe capabilities, make use of them in the DWC driver.
>>
>> Signed-off-by: Hans Zhang <18255117159@163.com>
>> ---
>> Changes since v11:
>> - Resolve compilation errors. s/dw_pcie_read_dbi/dw_pcie_read*_dbi
>>
>> Changes since v10:
>> - None
>>
>> Changes since v9:
>> - Resolved [v9 4/6] compilation error.
>>    The latest 6.15 rc1 merge __dw_pcie_find_vsec_capability, which uses
>>    dw_pcie_find_next_ext_capability.
>>
>> Changes since v8:
>> - None
>>
>> Changes since v7:
>> - Resolve compilation errors.
>>
>> Changes since v6:
>> https://lore.kernel.org/linux-pci/20250323164852.430546-3-18255117159@163.com/
>>
>> - The patch commit message were modified.
>>
>> Changes since v5:
>> https://lore.kernel.org/linux-pci/20250321163803.391056-3-18255117159@163.com/
>>
>> - Kconfig add "select PCI_HOST_HELPERS"
>> ---
>>   drivers/pci/controller/dwc/pcie-designware.c | 81 ++++----------------
>>   1 file changed, 14 insertions(+), 67 deletions(-)
>>
>> diff --git a/drivers/pci/controller/dwc/pcie-designware.c b/drivers/pci/controller/dwc/pcie-designware.c
>> index 97d76d3dc066..7939411a24eb 100644
>> --- a/drivers/pci/controller/dwc/pcie-designware.c
>> +++ b/drivers/pci/controller/dwc/pcie-designware.c
>> @@ -205,83 +205,30 @@ void dw_pcie_version_detect(struct dw_pcie *pci)
>>   		pci->type = ver;
>>   }
>>   
>> -/*
>> - * These interfaces resemble the pci_find_*capability() interfaces, but these
>> - * are for configuring host controllers, which are bridges *to* PCI devices but
>> - * are not PCI devices themselves.
>> - */
>> -static u8 __dw_pcie_find_next_cap(struct dw_pcie *pci, u8 cap_ptr,
>> -				  u8 cap)
>> +static int dw_pcie_read_cfg(void *priv, int where, int size, u32 *val)
>>   {
>> -	u8 cap_id, next_cap_ptr;
>> -	u16 reg;
>> -
>> -	if (!cap_ptr)
>> -		return 0;
>> +	struct dw_pcie *pci = priv;
>>   
>> -	reg = dw_pcie_readw_dbi(pci, cap_ptr);
>> -	cap_id = (reg & 0x00ff);
>> -
>> -	if (cap_id > PCI_CAP_ID_MAX)
>> -		return 0;
>> -
>> -	if (cap_id == cap)
>> -		return cap_ptr;
>> +	if (size == 4)
>> +		*val = dw_pcie_readl_dbi(pci, where);
>> +	else if (size == 2)
>> +		*val = dw_pcie_readw_dbi(pci, where);
>> +	else if (size == 1)
>> +		*val = dw_pcie_readb_dbi(pci, where);
> 
> Maybe here as well return error if the given size is invalid.

Dear Ilpo.

Will add return PCIBIOS_BAD_REGISTER_NUMBER.

>>   
>> -	next_cap_ptr = (reg & 0xff00) >> 8;
>> -	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
>> +	return PCIBIOS_SUCCESSFUL;
>>   }
>>   
>>   u8 dw_pcie_find_capability(struct dw_pcie *pci, u8 cap)
>>   {
>> -	u8 next_cap_ptr;
>> -	u16 reg;
>> -
>> -	reg = dw_pcie_readw_dbi(pci, PCI_CAPABILITY_LIST);
>> -	next_cap_ptr = (reg & 0x00ff);
>> -
>> -	return __dw_pcie_find_next_cap(pci, next_cap_ptr, cap);
>> +	return PCI_FIND_NEXT_CAP_TTL(dw_pcie_read_cfg, PCI_CAPABILITY_LIST, cap,
>> +				     pci);
>>   }
>>   EXPORT_SYMBOL_GPL(dw_pcie_find_capability);
>>   
>> -static u16 dw_pcie_find_next_ext_capability(struct dw_pcie *pci, u16 start,
>> -					    u8 cap)
>> -{
>> -	u32 header;
>> -	int ttl;
>> -	int pos = PCI_CFG_SPACE_SIZE;
>> -
>> -	/* minimum 8 bytes per capability */
>> -	ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;
>> -
>> -	if (start)
>> -		pos = start;
>> -
>> -	header = dw_pcie_readl_dbi(pci, pos);
>> -	/*
>> -	 * If we have no capabilities, this is indicated by cap ID,
>> -	 * cap version and next pointer all being 0.
>> -	 */
>> -	if (header == 0)
>> -		return 0;
>> -
>> -	while (ttl-- > 0) {
>> -		if (PCI_EXT_CAP_ID(header) == cap && pos != start)
>> -			return pos;
>> -
>> -		pos = PCI_EXT_CAP_NEXT(header);
>> -		if (pos < PCI_CFG_SPACE_SIZE)
>> -			break;
>> -
>> -		header = dw_pcie_readl_dbi(pci, pos);
>> -	}
>> -
>> -	return 0;
>> -}
>> -
>>   u16 dw_pcie_find_ext_capability(struct dw_pcie *pci, u8 cap)
>>   {
>> -	return dw_pcie_find_next_ext_capability(pci, 0, cap);
>> +	return PCI_FIND_NEXT_EXT_CAPABILITY(dw_pcie_read_cfg, 0, cap, pci);
>>   }
>>   EXPORT_SYMBOL_GPL(dw_pcie_find_ext_capability);
>>   
>> @@ -294,8 +241,8 @@ static u16 __dw_pcie_find_vsec_capability(struct dw_pcie *pci, u16 vendor_id,
>>   	if (vendor_id != dw_pcie_readw_dbi(pci, PCI_VENDOR_ID))
>>   		return 0;
>>   
>> -	while ((vsec = dw_pcie_find_next_ext_capability(pci, vsec,
>> -						       PCI_EXT_CAP_ID_VNDR))) {
>> +	while ((vsec = PCI_FIND_NEXT_EXT_CAPABILITY(
>> +			dw_pcie_read_cfg, vsec, PCI_EXT_CAP_ID_VNDR, pci))) {
> 
> Start the arguments from the first line and align the continuations to (.

Will change.

Best regards,
Hans

> 
>>   		header = dw_pcie_readl_dbi(pci, vsec + PCI_VNDR_HEADER);
>>   		if (PCI_VNDR_HEADER_ID(header) == vsec_id)
>>   			return vsec;
>>
>