> > > > + 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. Thanks for your reminder. This code is used to get information whether PCIe have completed physical training. I should change them to call LINK_UP_GEN2 and LINK_UP_GEN4. I will modify this part to avoid confusion and add more comment, as you said. Thanks, Jacky