Re: [v6 3/5] PCI: cadence: Use common PCI host bridge APIs for finding the capabilities

[Hans Zhang <18255117159@163.com>]

On 2025/4/1 00:39, Ilpo Järvinen wrote:
>>>> +			read_cfg((priv), (devfn), __pos, 2, (u32 *)&__ent); \
>>>> +     \
>>>> +			__id = __ent & 0xff;                                \
>>>> +			if (__id == 0xff)                                   \
>>>> +				break;                                      \
>>>> +			if (__id == (cap)) {                                \
>>>> +				__found_pos = __pos;                        \
>>>> +				break;                                      \
>>>> +			}                                                   \
>>>> +			__pos = (__ent >> 8);                               \
>>>
>>> I'd add these into uapi/linux/pci_regs.h:
>>
>> This means that you will submit, and I will submit after you?
>> Or should I submit this series of patches together?
> 
> I commented these cleanup opportunities so that you could add them to
> your series. If I'd immediately start working on area/lines you're working
> with, it would just trigger conflicts so it's better the original author
> does the improvements within the series he/she is working with. It's a lot
> less work for the maintainer that way :-).
> 

Hi Ilpo,

Thanks your for reply. Thank you so much for your comments.

>>> I started to wonder though if the controller drivers could simply create
>>> an "early" struct pci_dev & pci_bus just so they can use the normal
>>> accessors while the real structs are not yet created. It looks not
>>> much is needed from those structs to let the accessors to work.
>>>
>>
>> Here are a few questions:
>> 1. We need to initialize some variables for pci_dev. For example,
>> dev->cfg_size needs to be initialized to 4K for PCIe.
>>
>> u16 pci_find_next_ext_capability(struct pci_dev *dev, u16 start, int cap)
>> {
>> 	......
>> 	if (dev->cfg_size <= PCI_CFG_SPACE_SIZE)
>> 		return 0;
>> 	......
>>
> 
> Sure, it would require some initialization of the struct (but not
> full init like the probe path does that does lots of setup too).
> 
>> 2. Create an "early" struct pci_dev & pci_bus for each SOC vendor (Qcom,
>> Rockchip, etc). It leads to a lot of code that feels weird.
> 
> The early pci_dev+pci_bus would be created by a helper in PCI core that
> initializes what is necessary for the supported set of early core
> functionality to work. The controller drivers themselves would just call
> that function.
> 

Ok, got it.

>> I still prefer the approach we are discussing now.
> 
> I'm not saying we should immediately head toward this new idea within this
> series because it's going to be relatively big change. But it's certainly
> something that looks worth exploring so that the current chicken-egg
> problem with controller drivers could be solved.
> 

Ok, I hope to have the opportunity to participate in the discussion 
together in the future.

>> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
>> index 2e9cf26a9ee9..68c111be521d 100644
>> --- a/drivers/pci/pci.h
>> +++ b/drivers/pci/pci.h
>> @@ -4,6 +4,65 @@
>>
>>   #include <linux/pci.h>
> 
> Make sure to add the necessary headers for the function/macros you're
> using so that things won't depend on the #include order in the .c file.
> 

Will do.

>>
>> +/* Ilpo: I'd add these into uapi/linux/pci_regs.h: */
>> +#define PCI_CAP_ID_MASK		0x00ff
>> +#define PCI_CAP_LIST_NEXT_MASK	0xff00
>> +
>> +/* Standard capability finder */
> 
> Capability
> 
> Always use the same capitalization as the specs do.
> 

Will change.

> You should probably write a kernel doc for this macro too.
> 

Will do.

> I'd put these macro around where pcie_cap_has_*() forward declarations
> are so that the initial define block is not split.
> 

Will change.

>> +#define PCI_FIND_NEXT_CAP_TTL(read_cfg, start, cap, args...)		\
>> +({									\
>> +	u8 __pos = (start);						\
>> +	int __ttl = PCI_FIND_CAP_TTL;					\
>> +	u16 __ent;							\
>> +	u8 __found_pos = 0;						\
>> +	u8 __id;							\
>> +									\
>> +	read_cfg(args, __pos, 1, (u32 *)&__pos);			\
>> +									\
>> +	while (__ttl--) {						\
>> +		if (__pos < PCI_STD_HEADER_SIZEOF)			\
>> +			break;						\
>> +		__pos = ALIGN_DOWN(__pos, 4);				\
>> +		read_cfg(args, __pos, 2, (u32 *)&__ent);		\
>> +		__id = FIELD_GET(PCI_CAP_ID_MASK, __ent);		\
>> +		if (__id == 0xff)					\
>> +			break;						\
>> +		if (__id == (cap)) {					\
>> +			__found_pos = __pos;				\
>> +			break;						\
>> +		}							\
>> +		__pos = FIELD_GET(PCI_CAP_LIST_NEXT_MASK, __ent);	\
>> +	}								\
>> +	__found_pos;							\
>> +})
>> +
>> +/* Extended capability finder */
>> +#define PCI_FIND_NEXT_EXT_CAPABILITY(read_cfg, start, cap, args...)	\
>> +({									\
>> +	u16 __pos = (start) ?: PCI_CFG_SPACE_SIZE;			\
>> +	u16 __found_pos = 0;						\
>> +	int __ttl, __ret;						\
>> +	u32 __header;							\
>> +									\
>> +	__ttl = (PCI_CFG_SPACE_EXP_SIZE - PCI_CFG_SPACE_SIZE) / 8;	\
>> +	while (__ttl-- > 0 && __pos >= PCI_CFG_SPACE_SIZE) {		\
>> +		__ret = read_cfg(args, __pos, 4, &__header);		\
>> +		if (__ret != PCIBIOS_SUCCESSFUL)			\
>> +			break;						\
>> +									\
>> +		if (__header == 0)					\
>> +			break;						\
>> +									\
>> +		if (PCI_EXT_CAP_ID(__header) == (cap) && __pos != start) {\
>> +			__found_pos = __pos;				\
>> +			break;						\
>> +		}							\
>> +									\
>> +		__pos = PCI_EXT_CAP_NEXT(__header);			\
>> +	}								\
>> +	__found_pos;							\
>> +})
>> +
>>   struct pcie_tlp_log;
>>
>>   /* Number of possible devfns: 0.0 to 1f.7 inclusive */
>>
>>
>> Looking forward to your latest suggestions.
> 
> This generally looked good, I didn't read with a very fine comb but just
> focused on the important bits. I'll take a more detailed look once you
> make the official submission.

Ok, I'm going to prepare the next version of patch. I hope you can 
review it again. Thank you very much


Best regards,
Hans