Re: [PATCH 7/7] pci: aspeed: Add ASPEED PCIe host controller driver

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

On Mon, 23 Jun 2025, Jacky Chou wrote:

> > > +static bool aspeed_ast2700_get_link(struct aspeed_pcie *pcie) {
> > > +	u32 reg;
> > > +	bool link;
> > > +
> > > +	regmap_read(pcie->pciephy, PEHR_MISC_300, &reg);
> > > +	if (reg & CAPABILITY_GEN2) {
> > > +		regmap_read(pcie->pciephy, PEHR_MISC_344, &reg);
> > > +		link = !!(reg & LINK_STATUS_GEN2);
> > > +	} else {
> > > +		regmap_read(pcie->pciephy, PEHR_MISC_358, &reg);
> > > +		link = !!(reg & LINK_STATUS_GEN4);
> > 
> > While I don't entirely know the meaning of these bits, what if the link is not
> > using maximum speed it is capable of, does this check misbehave?
> > 
> 
> In our AST2700, there are gen4 RC and gen2 RC.
> Therefore, here will get capability to confirm it is gen2 or gen4.
> And the link status is in different register.

Okay, but then I'm a bit worried the naming of the defines as "Link 
Status" has a well-known meaning in PCIe Spec so it is confusing to call 
something else LINK_STATUS_*. Does that name come from some AST spec? In 
not, change the define naming. If yes, you should prefix it with so it is 
clear this is not referring to a generic PCIe thing and I suggest also 
adding a comment above those defines too so anyone looking them later 
won't wonder if you're just duplicating something from the PCIe spec.

-- 
 i.