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, ®);
> > > + if (reg & CAPABILITY_GEN2) {
> > > + regmap_read(pcie->pciephy, PEHR_MISC_344, ®);
> > > + link = !!(reg & LINK_STATUS_GEN2);
> > > + } else {
> > > + regmap_read(pcie->pciephy, PEHR_MISC_358, ®);
> > > + 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.
