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

[Hans Zhang]

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